On Fri, Apr 10, 2015 at 11:12:47AM +0200, Gabriel FERNANDEZ wrote:
> Call directly pci_*() instead of using pci_common_init_dev().
> Enforce error handling in probe.
> It also allows st pcie driver not to declare IO space:
> pci_common_init_dev() is assigning one by default.
Can you verify that none of the other users (dra7xx, exynos, etc.) depends
on the default I/O space, and mention that here?
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx>
This requires an ack from Jingoo (DesignWare maintainer), of course.
I think this code is used by dra7xx, exynos, imx6, spear13xx, keystone, and
layerscape. It'd be nice to have some review and testing from them, too.
> ---
> drivers/pci/host/pcie-designware.c | 62 ++++++++++++++++++++------------------
> drivers/pci/host/pcie-designware.h | 1 +
> 2 files changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 1f4ea6f..d4b1bf7 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -67,8 +67,6 @@
> #define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16)
> #define PCIE_ATU_UPPER_TARGET 0x91C
>
> -static struct hw_pci dw_pci;
> -
> static unsigned long global_io_offset;
>
> static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> @@ -342,6 +340,9 @@ static const struct irq_domain_ops msi_domain_ops = {
> .map = dw_pcie_msi_map,
> };
>
> +static int dw_pcie_setup(struct pci_sys_data *sys);
> +static struct pci_bus *dw_pcie_scan_bus(struct pci_sys_data *sys);
> +
> int __init dw_pcie_host_init(struct pcie_port *pp)
> {
> struct device_node *np = pp->dev->of_node;
> @@ -352,6 +353,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> u32 val, na, ns;
> const __be32 *addrp;
> int i, index, ret;
> + struct list_head *resources = &pp->sysdata.resources;
> +
> + INIT_LIST_HEAD(resources);
>
> /* Find the address cell size and the number of cells in order to get
> * the untranslated address.
> @@ -504,13 +508,17 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>
> #ifdef CONFIG_PCI_MSI
> dw_pcie_msi_chip.dev = pp->dev;
> - dw_pci.msi_ctrl = &dw_pcie_msi_chip;
> + pp->sysdata.msi_ctrl = &dw_pcie_msi_chip;
> #endif
>
> - dw_pci.nr_controllers = 1;
> - dw_pci.private_data = (void **)&pp;
> + pp->sysdata.private_data = pp;
>
> - pci_common_init_dev(pp->dev, &dw_pci);
> + ret = dw_pcie_setup(&pp->sysdata);
It's not completely obvious that replacing pci_common_init_dev() with
dw_pcie_setup() is equivalent. Here are my notes from trying to convince
myself that this is correct. I think your changelog could stand some
enhancement. It would be ideal if you could break this into a few smaller
patches that were more obviously correct, but I suspect it's too
intertwined to do that.
Here's an outline of pci_common_init_dev():
pci_common_init_dev(parent, hw)
pci_add_flags(PCI_REASSIGN_ALL_RSRC) # [1]
if (hw->preinit)
hw->preinit # [2]
pcibios_init_hw
for (nr = 0; nr < hw->nr_controllers; # [3]
sys = kzalloc # [4]
sys->msi_ctrl = hw->msi_ctrl # [5]
sys->busnr = busnr # [6]
sys->swizzle = hw->swizzle # [7]
sys->map_irq = hw->map_irq # [8]
sys->align_resource = hw->align_resource # [9]
INIT_LIST_HEAD(&sys->resources) # [10]
sys->private_data = hw->private_data # [11]
hw->setup # [12]
pcibios_init_resources # [13]
hw->scan # [14]
if (hw->postinit)
hw->postinit # [15]
pci_fixup_irqs # [16]
list_for_each_entry(sys, &head, # [17]
if (!pci_has_flag(PCI_PROBE_ONLY)) # [18]
pci_bus_size_bridges # [19]
pci_bus_assign_resources
pci_bus_add_devices
list_for_each_entry(sys, &head,
...
pcie_bus_configure_settings # [20]
[1] You don't set PCI_REASSIGN_ALL_RSRC; have you verified that nobody
cares about that?
[2] dw_pci.preinit was NULL, so skipping this doesn't matter; looks OK.
[3] dw_pci.nr_controllers was always 1, so skipping the loop doesn't
matter; looks OK.
[4] You added struct pci_sys_data sysdata as a member of struct
pcie_port, so it is now allocated by dw_pcie_host_init() callers, e.g.,
st_pcie_probe(); looks OK.
[5] You set pp->sysdata.msi_ctrl in dw_pcie_host_init() instead of
pcibios_init_hw(); looks OK.
[6] Since dw_pci.nr_controllers was always 1, sys->busnr was always 0.
You don't set sys->busnr, so it will retain its initial zero value. OK, I
guess. It's still a little broken that we have both pp->busn and
pp->sysdata.busnr, but you didn't break it and it's OK that you didn't
change anything in this regard.
[7] dw_pci.swizzle was NULL, so pcibios_swizzle() would default to
pci_common_swizzle(), which is what you use when you call pci_fixup_irqs()
in dw_pcie_scan_bus(); looks OK.
[8] dw_pci.map_irq was dw_pcie_map_irq() (which used
of_irq_parse_and_map_pci()) and sys->map_irq was used by pcibios_map_irq().
You call pci_fixup_irqs() and pass a pointer to of_irq_parse_and_map_pci();
looks OK.
[9] dw_pci.align_resource was NULL, and I assume
pcie_port.sysdata.align_resource was initialized to zeroes; looks OK.
[10] You call INIT_LIST_HEAD() in dw_pcie_host_init() instead of
pcibios_init_hw(); looks OK.
[11] You set sys->private_data in dw_pcie_host_init() instead of
pcibios_init_hw(); looks OK.
[12] dw_pci.setup was dw_pcie_setup(), and you call that from
dw_pcie_host_init(); looks OK.
[13] You no longer call pcibios_init_resources(). You don't want the
default I/O space, which makes sense; looks OK (but you need to audit other
users and make sure they don't need it either).
[14] dw_pci.scan was dw_pcie_scan_bus(), and you call that from
dw_pcie_host_init(); looks OK.
[15] dw_pci.postinit was NULL, so skipping this doesn't matter; looks OK.
[16] You call pci_fixup_irqs() from dw_pcie_scan_bus() instead of
pci_common_init_dev(); looks OK.
[17] "head" is a local list in pci_common_init_dev(), and you don't need
anything similar because dw_pcie_host_init() is called once per host
bridge; looks OK.
[18] You don't test PCI_PROBE_ONLY; it looks like it can still be set by
booting with "pci=firmware". So previously, "pci=firmware" prevented
resource assignment, but it no longer does. Is that right? Is that what
you intend?
[19] Instead of pci_bus_size_bridges() and pci_bus_assign_resources(), you
call pci_assign_unassigned_bus_resources() from dw_pcie_scan_bus(). That
seems like an improvement because it holds pci_bus_sem and supplies
add_list; looks OK.
[20] I think you no longer call pcie_bus_configure_settings(). That
configured MPS settings, and I think you still want to do that, don't you?
> + if (ret)
> + return ret;
> +
> + if (!dw_pcie_scan_bus(&pp->sysdata))
> + return -ENXIO;
>
> return 0;
> }
> @@ -701,15 +709,19 @@ static struct pci_ops dw_pcie_ops = {
> .write = dw_pcie_wr_conf,
> };
>
> -static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> +static int dw_pcie_setup(struct pci_sys_data *sys)
> {
> struct pcie_port *pp;
> + int err;
>
> pp = sys_to_pcie(sys);
>
> if (global_io_offset < SZ_1M && pp->io_size > 0) {
> sys->io_offset = global_io_offset - pp->io_bus_addr;
> - pci_ioremap_io(global_io_offset, pp->io_base);
> + err = pci_ioremap_io(global_io_offset, pp->io_base);
> + if (err)
> + return err;
> +
> global_io_offset += SZ_64K;
> pci_add_resource_offset(&sys->resources, &pp->io,
> sys->io_offset);
> @@ -719,10 +731,10 @@ static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
> pci_add_resource(&sys->resources, &pp->busn);
>
> - return 1;
> + return 0;
> }
>
> -static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> +static struct pci_bus *dw_pcie_scan_bus(struct pci_sys_data *sys)
> {
> struct pci_bus *bus;
> struct pcie_port *pp = sys_to_pcie(sys);
> @@ -738,27 +750,13 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> if (bus && pp->ops->scan_bus)
> pp->ops->scan_bus(pp);
>
> - return bus;
> -}
> -
> -static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> -{
> - struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
> - int irq;
> -
> - irq = of_irq_parse_and_map_pci(dev, slot, pin);
> - if (!irq)
> - irq = pp->irq;
> + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> + pci_assign_unassigned_bus_resources(bus);
> + pci_bus_add_devices(bus);
>
> - return irq;
> + return bus;
> }
>
> -static struct hw_pci dw_pci = {
> - .setup = dw_pcie_setup,
> - .scan = dw_pcie_scan_bus,
> - .map_irq = dw_pcie_map_irq,
> -};
> -
> void dw_pcie_setup_rc(struct pcie_port *pp)
> {
> u32 val;
> @@ -822,8 +820,12 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> /* setup command register */
> dw_pcie_readl_rc(pp, PCI_COMMAND, &val);
> val &= 0xffff0000;
> - val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
> - PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
> +
> + if (!pp->io_size)
> + val |= PCI_COMMAND_IO;
> +
> + val |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
> +
> dw_pcie_writel_rc(pp, val, PCI_COMMAND);
> }
>
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index d0bbd27..45bc2c2 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -46,6 +46,7 @@ struct pcie_port {
> struct resource io;
> struct resource mem;
> struct resource busn;
> + struct pci_sys_data sysdata;
> int irq;
> u32 lanes;
> struct pcie_host_ops *ops;
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
