Re: [PATCH 4/4] PCI: ixp4xx: Add a new driver for IXP4xx

[Arnd Bergmann <arnd@arndb.de>]

On Mon, May 3, 2021 at 11:16 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> This adds a new PCI controller driver for the Intel IXP4xx
> (IX425, IXP435 etc), based on the XScale microarchitecture.
>
> This replaces the old driver in arch/arm/mach-ixp4xx/common-pci.c
> which utilized the ARM-specific BIOS32 PCI framework,
> and all parameterization for such things as memory and
> IO space as well as interrupt swizzling is done from the
> device tree.

Nice work!

> The __raw_writel() and __raw_readl() are used for accessing
> the PCI controller for the same reason that these accessors
> are used in the timer, IRQ and GPIO drivers: the platform
> will alter its address bus pattern based on whether the
> system is booted in big- or little-endian mode. For this
> reason all register on IXP4xx must always be accessed in
> native (CPU) endianness.

Can you add a pair of inline function that wraps these into
a driver specific helper with a comment?

> This driver supports 64MB of PCI memory space, but not the
> indirect access of 1GB that is available in the old driver.
> We can address that later if and only if there are users
> that need all 1GB of PCI address space.

> +
> +       ret = pci_scan_root_bus_bridge(host);
> +       if (ret) {
> +               dev_err(dev, "failed to scan host: %d\n", ret);
> +               return ret;
> +       }
> +
> +       p->bus = host->bus;

I don't think you need to store the bus separately, just use
host->bus everywhere.

> +       pci_bus_assign_resources(p->bus);
> +       pci_bus_add_devices(p->bus);

Can you call pci_host_probe() instead of open-coding it here?

> +
> +static struct platform_driver ixp4xx_pci_driver = {
> +       .driver = {
> +               .name = "ixp4xx-pci",
> +               .of_match_table = of_match_ptr(ixp4xx_pci_of_match),
> +               .suppress_bind_attrs = true,
> +       },
> +       .probe  = ixp4xx_pci_probe,
> +};
> +builtin_platform_driver(ixp4xx_pci_driver);

It should be possible to make it a loadable module, after Rob Herring
fixed some of the bugs around that.

        Arnd