[PATCH RESEND] PCI: mvebu - Support a bridge with no IO port window
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Thu Oct 3 08:06:48 EDT 2013
Dear Jason Gunthorpe,
On Tue, 1 Oct 2013 11:58:01 -0600, Jason Gunthorpe wrote:
> Make pcie-io-aperture and the IO port MBUS ID in ranges optional.
> If not provided the bridge reports to Linux that IO space mapping is
> not supported and refuses to configure an IO mbus window.
>
> This allows both complete disable (do not specify pcie-io-aperture) and
> per-port disable (do not specify a IO target ranges entry for the port)
>
> Most PCIE devices these days do not require IO support to function,
> so having an option to disable it in the driver is useful.
>
> This depends on 'bus: mvebu-mbus: Fix optional pcie-mem/io-aperture properties'
> to work properly.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe at obsidianresearch.com>
This doesn't work here, I get multiple warnings "Attempt to set IO when
IO is disabled" :
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at /home/thomas/projets/linux-2.6/drivers/pci/host/pci-mvebu.c:302 mvebu_pcie_wr_conf+0x2c0/0x44c()
Device: mvebu-pcie
Attempt to set IO when IO is disabled
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-rc1-00003-ga85b362 #6
[<c0015798>] (unwind_backtrace+0x0/0xf8) from [<c0011770>] (show_stack+0x10/0x14)
[<c0011770>] (show_stack+0x10/0x14) from [<c0372c08>] (dump_stack+0x70/0x8c)
[<c0372c08>] (dump_stack+0x70/0x8c) from [<c001d5bc>] (warn_slowpath_common+0x64/0x88)
[<c001d5bc>] (warn_slowpath_common+0x64/0x88) from [<c001d674>] (warn_slowpath_fmt+0x30/0x40)
[<c001d674>] (warn_slowpath_fmt+0x30/0x40) from [<c0187080>] (mvebu_pcie_wr_conf+0x2c0/0x44c)
[<c0187080>] (mvebu_pcie_wr_conf+0x2c0/0x44c) from [<c01770fc>] (pci_bus_write_config_word+0x60/0x78)
[<c01770fc>] (pci_bus_write_config_word+0x60/0x78) from [<c03708b4>] (__pci_bus_size_bridges+0x708/0x7c0)
[<c03708b4>] (__pci_bus_size_bridges+0x708/0x7c0) from [<c03701e0>] (__pci_bus_size_bridges+0x34/0x7c0)
[<c03701e0>] (__pci_bus_size_bridges+0x34/0x7c0) from [<c00134c4>] (pci_common_init_dev+0x1b0/0x2dc)
[<c00134c4>] (pci_common_init_dev+0x1b0/0x2dc) from [<c04a3f68>] (mvebu_pcie_probe+0x444/0x48c)
[<c04a3f68>] (mvebu_pcie_probe+0x444/0x48c) from [<c01b69dc>] (platform_drv_probe+0x18/0x1c)
[<c01b69dc>] (platform_drv_probe+0x18/0x1c) from [<c01b5858>] (driver_probe_device+0xf4/0x210)
[<c01b5858>] (driver_probe_device+0xf4/0x210) from [<c01b5a00>] (__driver_attach+0x8c/0x90)
[<c01b5a00>] (__driver_attach+0x8c/0x90) from [<c01b3eb8>] (bus_for_each_dev+0x54/0x88)
[<c01b3eb8>] (bus_for_each_dev+0x54/0x88) from [<c01b4f60>] (bus_add_driver+0xd4/0x258)
[<c01b4f60>] (bus_add_driver+0xd4/0x258) from [<c01b6038>] (driver_register+0x78/0xf4)
[<c01b6038>] (driver_register+0x78/0xf4) from [<c01b6bc0>] (platform_driver_probe+0x1c/0xa0)
[<c01b6bc0>] (platform_driver_probe+0x1c/0xa0) from [<c0008878>] (do_one_initcall+0xe4/0x140)
[<c0008878>] (do_one_initcall+0xe4/0x140) from [<c0492b60>] (kernel_init_freeable+0xfc/0x1c4)
[<c0492b60>] (kernel_init_freeable+0xfc/0x1c4) from [<c036f0cc>] (kernel_init+0x8/0xe4)
[<c036f0cc>] (kernel_init+0x8/0xe4) from [<c000e3d8>] (ret_from_fork+0x14/0x3c)
---[ end trace 70bc10e370e10347 ]---
> +static inline bool mvebu_has_ioport(struct mvebu_pcie_port *port)
> +{
> + return port->io_target == -1 && port->io_attr == -1;
Are you sure the logic is not reverted here? I.e, this shouldn't be !=
-1 in both cases?
> +}
> +
> static bool mvebu_pcie_link_up(struct mvebu_pcie_port *port)
> {
> return !(readl(port->base + PCIE_STAT_OFF) & PCIE_STAT_LINK_DOWN);
> @@ -292,6 +297,12 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
> return;
> }
>
> + if (!mvebu_has_ioport(port)) {
> + dev_WARN(&port->pcie->pdev->dev,
> + "Attempt to set IO when IO is disabled\n");
> + return;
> + }
> +
> /*
> * We read the PCI-to-PCI bridge emulated registers, and
> * calculate the base address and size of the address decoding
> @@ -405,9 +416,12 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
> break;
>
> case PCI_IO_BASE:
> - *value = (bridge->secondary_status << 16 |
> - bridge->iolimit << 8 |
> - bridge->iobase);
> + if (!mvebu_has_ioport(port))
> + *value = 0;
> + else
> + *value = (bridge->secondary_status << 16 |
> + bridge->iolimit << 8 |
> + bridge->iobase);
While I do understand that you're returning 0 for iolimit and iobase,
I'm not sure why the secondary status is affected by the value of
mvebu_has_ioport().
> static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
> - unsigned long type, int *tgt, int *attr)
> + unsigned long type,
> + unsigned int *tgt,
> + unsigned int *attr)
> {
> const int na = 3, ns = 2;
> const __be32 *range;
> int rlen, nranges, rangesz, pna, i;
>
> + *tgt = -1;
> + *attr = -1;
> +
> range = of_get_property(np, "ranges", &rlen);
> if (!range)
> return -EINVAL;
> @@ -798,16 +821,15 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
> }
>
> mvebu_mbus_get_pcie_io_aperture(&pcie->io);
> - if (resource_size(&pcie->io) == 0) {
> - dev_err(&pdev->dev, "invalid I/O aperture size\n");
> - return -EINVAL;
> - }
>
> - pcie->realio.flags = pcie->io.flags;
> - pcie->realio.start = PCIBIOS_MIN_IO;
> - pcie->realio.end = min_t(resource_size_t,
> - IO_SPACE_LIMIT,
> - resource_size(&pcie->io));
> + if (resource_size(&pcie->io) != 0) {
> + pcie->realio.flags = pcie->io.flags;
> + pcie->realio.start = PCIBIOS_MIN_IO;
> + pcie->realio.end = min_t(resource_size_t,
> + IO_SPACE_LIMIT,
> + resource_size(&pcie->io));
> + } else
> + pcie->realio = pcie->io;
>
> /* Get the bus range */
> ret = of_pci_parse_bus_range(np, &pcie->busn);
> @@ -864,12 +886,12 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
> continue;
> }
>
> - ret = mvebu_get_tgt_attr(np, port->devfn, IORESOURCE_IO,
> - &port->io_target, &port->io_attr);
> - if (ret < 0) {
> - dev_err(&pdev->dev, "PCIe%d.%d: cannot get tgt/attr for io window\n",
> - port->port, port->lane);
> - continue;
> + if (resource_size(&pcie->io) != 0)
> + mvebu_get_tgt_attr(np, port->devfn, IORESOURCE_IO,
> + &port->io_target, &port->io_attr);
> + else {
> + port->io_target = -1;
> + port->io_attr = -1;
> }
>
> port->base = mvebu_pcie_map_registers(pdev, child, port);
Thanks!
Thomas
--
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
More information about the linux-arm-kernel
mailing list