lkml.org 
[lkml]   [2015]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 4/5] pci: designware: remove pci_common_init_dev()
Hi Bjorn,

On 05/06/2015 09:22 PM, Bjorn Helgaas wrote:
> 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.

Thanks you for your review !

Sorry for the late reply, from your detailed analysis bellow, yes, it
looks like pci_common_init_dev isn't
completely equivalent.
I'm wondering about PCI_PROBE_ONLY flag...

The idea in the first place, to move to generic probing directly, instead of
pci_common_init_dev() was to avoid being assigned default I/O (e.g. in
bios32.c).
Please refer to discussion with Arnd :
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317726.html
But, I don't see how to be fully compatible (e.g. ARM specific option
like PCI_PROBE_ONLY)
without calling pci_common_init_dev() or duplicating code from bios32.c.

Maybe should I fall back to the first idea of using specifc handling of
an "empty" IO space, and keep pci_common_init_dev() ?

Please advise,

BR,
Fabrice

> 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?



\
 
 \ /
  Last update: 2015-05-25 16:41    [W:0.061 / U:0.780 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site