lkml.org 
[lkml]   [2015]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v3 4/5] pci: designware: remove pci_common_init_dev()
    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@st.com>

    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
    >


    \
     
     \ /
      Last update: 2015-05-06 22:01    [W:3.302 / U:1.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site