Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751395AbbEYOa4 (ORCPT ); Mon, 25 May 2015 10:30:56 -0400 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:47040 "EHLO mx08-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751126AbbEYOaw (ORCPT ); Mon, 25 May 2015 10:30:52 -0400 Message-ID: <556331A7.60909@st.com> Date: Mon, 25 May 2015 16:28:55 +0200 From: Fabrice Gasnier User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Bjorn Helgaas , Gabriel FERNANDEZ CC: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Srinivas Kandagatla , Maxime Coquelin , Patrice Chotard , Russell King , Jingoo Han , Lucas Stach , Kishon Vijay Abraham I , Andrew Morton , "David S. Miller" , Greg KH , Mauro Carvalho Chehab , Joe Perches , Tejun Heo , Arnd Bergmann , Viresh Kumar , Thierry Reding , Phil Edworthy , Minghuan Lian , Tanmay Inamdar , , Sachin Kamat , Andrew Lunn , Liviu Dudau , , , , , , Lee Jones , Gabriel Fernandez Subject: Re: [PATCH v3 4/5] pci: designware: remove pci_common_init_dev() References: <1428657168-12495-1-git-send-email-gabriel.fernandez@linaro.org> <1428657168-12495-5-git-send-email-gabriel.fernandez@linaro.org> <20150506192201.GF24643@google.com> In-Reply-To: <20150506192201.GF24643@google.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.48.254.79] X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.14.151,1.0.33,0.0.0000 definitions=2015-05-25_02:2015-05-25,2015-05-25,1970-01-01 signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5970 Lines: 141 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/