| Message ID | 20240328085041.2916899-3-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: keystone: Fix pci_ops for AM654x SoC | expand |
On Thu, Mar 28, 2024 at 02:20:41PM +0530, Siddharth Vadapalli wrote: > In the process of converting .scan_bus() callbacks to .add_bus(), the > ks_pcie_v3_65_scan_bus() function was changed to ks_pcie_v3_65_add_bus(). > The .scan_bus() method belonged to ks_pcie_host_ops which was specific > to controller version 3.65a, while the .add_bus() method had been added > to ks_pcie_ops which is shared between the controller versions 3.65a and > 4.90a. Neither the older ks_pcie_v3_65_scan_bus() method, nor the newer > ks_pcie_v3_65_add_bus() method is applicable to the controller version > 4.90a which is present in AM654x SoCs. > > Thus, as a fix, remove "ks_pcie_v3_65_add_bus()" and move its contents > to the .msi_init callback "ks_pcie_msi_host_init()" which is specific to > the 3.65a controller. > > Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus") > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > Suggested-by: Bjorn Helgaas <helgaas@kernel.org> > Suggested-by: Niklas Cassel <cassel@kernel.org> > Reviewed-by: Niklas Cassel <cassel@kernel.org> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> Thanks for splitting this into two patches. Krzysztof has applied both to pci/controller/keystone and we hope to merge them for v6.10. I *would* like the commit log to be at a little higher level if possible. Right now it's a detailed description at the level of the code edits, but it doesn't say *why* we want this change. I think the first cut at this was https://lore.kernel.org/linux-pci/20231011123451.34827-1-s-vadapalli@ti.com/t/#u, which mentioned Completion Timeouts during MSI-X configuration and 45 second delays during boot. IIUC, prior to 6ab15b5e7057, ks_pcie_v3_65_scan_bus() initialized BAR 0 and was only used for v3.65a devices. 6ab15b5e7057 renamed it to ks_pcie_v3_65_add_bus() and called it for both v3.65a and v4.90a. I think the problem is that in the current code, the ks_pcie_ops.add_bus() method (ks_pcie_v3_65_add_bus()) is used for all devices (both v3.65a and v4.90a). So I guess doing the BAR 0 setup on v4.90a broke something there? I'm not quite clear on the mechanism, but it would be helpful to at least know what's wrong and on what platform. E.g., currently v4.90 suffers Completion Timeouts and 45 second boot delays? And this patch fixes that? > --- > drivers/pci/controller/dwc/pci-keystone.c | 52 ++++++++--------------- > 1 file changed, 18 insertions(+), 34 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 5c073e520628..6cb3a4713009 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -289,6 +289,24 @@ static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie) > > static int ks_pcie_msi_host_init(struct dw_pcie_rp *pp) > { > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct keystone_pcie *ks_pcie = to_keystone_pcie(pci); > + > + /* Configure and set up BAR0 */ > + ks_pcie_set_dbi_mode(ks_pcie); > + > + /* Enable BAR0 */ > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); > + > + ks_pcie_clear_dbi_mode(ks_pcie); > + > + /* > + * For BAR0, just setting bus address for inbound writes (MSI) should > + * be sufficient. Use physical address to avoid any conflicts. > + */ > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); > + > pp->msi_irq_chip = &ks_pcie_msi_irq_chip; > return dw_pcie_allocate_domains(pp); > } > @@ -445,44 +463,10 @@ static struct pci_ops ks_child_pcie_ops = { > .write = pci_generic_config_write, > }; > > -/** > - * ks_pcie_v3_65_add_bus() - keystone add_bus post initialization > - * @bus: A pointer to the PCI bus structure. > - * > - * This sets BAR0 to enable inbound access for MSI_IRQ register > - */ > -static int ks_pcie_v3_65_add_bus(struct pci_bus *bus) > -{ > - struct dw_pcie_rp *pp = bus->sysdata; > - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > - struct keystone_pcie *ks_pcie = to_keystone_pcie(pci); > - > - if (!pci_is_root_bus(bus)) > - return 0; > - > - /* Configure and set up BAR0 */ > - ks_pcie_set_dbi_mode(ks_pcie); > - > - /* Enable BAR0 */ > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); > - > - ks_pcie_clear_dbi_mode(ks_pcie); > - > - /* > - * For BAR0, just setting bus address for inbound writes (MSI) should > - * be sufficient. Use physical address to avoid any conflicts. > - */ > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); > - > - return 0; > -} > - > static struct pci_ops ks_pcie_ops = { > .map_bus = dw_pcie_own_conf_map_bus, > .read = pci_generic_config_read, > .write = pci_generic_config_write, > - .add_bus = ks_pcie_v3_65_add_bus, > }; > > /** > -- > 2.40.1 >
On Mon, May 13, 2024 at 04:53:50PM -0500, Bjorn Helgaas wrote: > On Thu, Mar 28, 2024 at 02:20:41PM +0530, Siddharth Vadapalli wrote: > > In the process of converting .scan_bus() callbacks to .add_bus(), the > > ks_pcie_v3_65_scan_bus() function was changed to ks_pcie_v3_65_add_bus(). > > The .scan_bus() method belonged to ks_pcie_host_ops which was specific > > to controller version 3.65a, while the .add_bus() method had been added > > to ks_pcie_ops which is shared between the controller versions 3.65a and > > 4.90a. Neither the older ks_pcie_v3_65_scan_bus() method, nor the newer > > ks_pcie_v3_65_add_bus() method is applicable to the controller version > > 4.90a which is present in AM654x SoCs. > > > > Thus, as a fix, remove "ks_pcie_v3_65_add_bus()" and move its contents > > to the .msi_init callback "ks_pcie_msi_host_init()" which is specific to > > the 3.65a controller. > > > > Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus") > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > Suggested-by: Bjorn Helgaas <helgaas@kernel.org> > > Suggested-by: Niklas Cassel <cassel@kernel.org> > > Reviewed-by: Niklas Cassel <cassel@kernel.org> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > Thanks for splitting this into two patches. Krzysztof has applied > both to pci/controller/keystone and we hope to merge them for v6.10. > > I *would* like the commit log to be at a little higher level if > possible. Right now it's a detailed description at the level of the > code edits, but it doesn't say *why* we want this change. > > I think the first cut at this was > https://lore.kernel.org/linux-pci/20231011123451.34827-1-s-vadapalli@ti.com/t/#u, > which mentioned Completion Timeouts during MSI-X configuration and 45 > second delays during boot. > > IIUC, prior to 6ab15b5e7057, ks_pcie_v3_65_scan_bus() initialized BAR > 0 and was only used for v3.65a devices. 6ab15b5e7057 renamed it to > ks_pcie_v3_65_add_bus() and called it for both v3.65a and v4.90a. > > I think the problem is that in the current code, the > ks_pcie_ops.add_bus() method (ks_pcie_v3_65_add_bus()) is used for all > devices (both v3.65a and v4.90a). So I guess doing the BAR 0 setup on > v4.90a broke something there? BAR0 was set to a different value on AM654x SoC which has the v4.90a controller, which is identical to what is set even for the v3.65a controller. The difference is that BAR0 is programmed to a different value for enabling inbound MSI writes on top of the common configuration performed for BAR0. Common configuration for BAR0: ks_pcie_probe dw_pcie_host_init dw_pcie_setup_rc ... /* Setup RC BARs */ dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004); dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1, 0x00000000); ... dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0); ... MSI specific configuration of BAR0 performed after the common configuration via the ks_pcie_v3_65_scan_bus() callback: /* Configure and set up BAR0 */ ks_pcie_set_dbi_mode(ks_pcie); /* Enable BAR0 */ dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); ks_pcie_clear_dbi_mode(ks_pcie); /* * For BAR0, just setting bus address for inbound writes (MSI) should * be sufficient. Use physical address to avoid any conflicts. */ dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); The above configuration of BAR0 shouldn't be performed for AM654x SoC. While I am not certain, the timeouts are probably a result of the BAR being programmed to a wrong value which results in a "no match" outcome. > > I'm not quite clear on the mechanism, but it would be helpful to at > least know what's wrong and on what platform. E.g., currently v4.90 > suffers Completion Timeouts and 45 second boot delays? And this patch > fixes that? Yes, the Completion Timeouts cause the 45 second boot delays and this patch fixes that. Regards, Siddharth.
On Tue, May 14, 2024 at 05:41:48PM +0530, Siddharth Vadapalli wrote: > On Mon, May 13, 2024 at 04:53:50PM -0500, Bjorn Helgaas wrote: > > On Thu, Mar 28, 2024 at 02:20:41PM +0530, Siddharth Vadapalli wrote: > > > In the process of converting .scan_bus() callbacks to .add_bus(), the > > > ks_pcie_v3_65_scan_bus() function was changed to ks_pcie_v3_65_add_bus(). > > > The .scan_bus() method belonged to ks_pcie_host_ops which was specific > > > to controller version 3.65a, while the .add_bus() method had been added > > > to ks_pcie_ops which is shared between the controller versions 3.65a and > > > 4.90a. Neither the older ks_pcie_v3_65_scan_bus() method, nor the newer > > > ks_pcie_v3_65_add_bus() method is applicable to the controller version > > > 4.90a which is present in AM654x SoCs. > > > > > > Thus, as a fix, remove "ks_pcie_v3_65_add_bus()" and move its contents > > > to the .msi_init callback "ks_pcie_msi_host_init()" which is specific to > > > the 3.65a controller. > > > > > > Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus") > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > Suggested-by: Bjorn Helgaas <helgaas@kernel.org> > > > Suggested-by: Niklas Cassel <cassel@kernel.org> > > > Reviewed-by: Niklas Cassel <cassel@kernel.org> > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > > Thanks for splitting this into two patches. Krzysztof has applied > > both to pci/controller/keystone and we hope to merge them for v6.10. > > > > I *would* like the commit log to be at a little higher level if > > possible. Right now it's a detailed description at the level of the > > code edits, but it doesn't say *why* we want this change. > > > > I think the first cut at this was > > https://lore.kernel.org/linux-pci/20231011123451.34827-1-s-vadapalli@ti.com/t/#u, > > which mentioned Completion Timeouts during MSI-X configuration and 45 > > second delays during boot. > > > > IIUC, prior to 6ab15b5e7057, ks_pcie_v3_65_scan_bus() initialized BAR > > 0 and was only used for v3.65a devices. 6ab15b5e7057 renamed it to > > ks_pcie_v3_65_add_bus() and called it for both v3.65a and v4.90a. > > > > I think the problem is that in the current code, the > > ks_pcie_ops.add_bus() method (ks_pcie_v3_65_add_bus()) is used for all > > devices (both v3.65a and v4.90a). So I guess doing the BAR 0 setup on > > v4.90a broke something there? > > BAR0 was set to a different value on AM654x SoC which has the v4.90a > controller, which is identical to what is set even for the v3.65a > controller. The difference is that BAR0 is programmed to a different > value for enabling inbound MSI writes on top of the common configuration > performed for BAR0. > > Common configuration for BAR0: > ks_pcie_probe > dw_pcie_host_init > dw_pcie_setup_rc > ... > /* Setup RC BARs */ > dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004); > dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1, 0x00000000); Tangential questions that don't need to be addressed for this patch: There's a bunch of code between these writes and the one below. Does that code in the middle depend on the above, or should all three of these writes be together? Is there some magic in writing PCI_BASE_ADDRESS_0 twice? This is common code for all DWC devices, so it must work OK, but it looks strange. > dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0); What's the net effect of these three writes? Disabling BAR 0 and BAR 1, as suggested at [1, 2]? [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-keystone.c?id=v6.9#n399 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-rcar-gen4.c?id=v6.9#n280 > MSI specific configuration of BAR0 performed after the common > configuration via the ks_pcie_v3_65_scan_bus() callback: > /* Configure and set up BAR0 */ > ks_pcie_set_dbi_mode(ks_pcie); > > /* Enable BAR0 */ > dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); > dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); I assume that in DBI mode, this actually writes a mask, not a value? If writing a 0xfff mask means that the low 12 bits could not be set, maybe this configures it to be a 4K memory BAR? But is there some reason to do two writes to the same register? > ks_pcie_clear_dbi_mode(ks_pcie); > > /* > * For BAR0, just setting bus address for inbound writes (MSI) should > * be sufficient. Use physical address to avoid any conflicts. > */ > dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); After clearing DBI mode, this looks like it would set BAR 0 to the ks_pcie->app.start address, which makes some sense, since the low 4 bits would all be zero (assuming the address is page aligned), which would make it a 32-bit memory BAR. > The above configuration of BAR0 shouldn't be performed for AM654x SoC. > While I am not certain, the timeouts are probably a result of the BAR > being programmed to a wrong value which results in a "no match" outcome. > > > I'm not quite clear on the mechanism, but it would be helpful to at > > least know what's wrong and on what platform. E.g., currently v4.90 > > suffers Completion Timeouts and 45 second boot delays? And this patch > > fixes that? > > Yes, the Completion Timeouts cause the 45 second boot delays and this > patch fixes that. And this problem happens on AM654x/v4.90a, right? I really want the commit log to say what platform is affected! Maybe something like this? PCI: keystone: Enable BAR 0 only for v3.65a The BAR 0 initialization done by ks_pcie_v3_65_add_bus() should happen for v3.65a devices only. On other devices, BAR 0 should be left disabled, as it is by dw_pcie_setup_rc(). After 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus"), ks_pcie_v3_65_add_bus() enabled BAR 0 for both v3.65a and v4.90a devices. On the AM654x SoC, which uses v4.90a, enabling BAR 0 causes Completion Timeouts when setting up MSI-X. These timeouts delay boot of the AM654x by about 45 seconds. Move the BAR 0 initialization to ks_pcie_msi_host_init(), which is only used for v3.65a devices, and remove ks_pcie_v3_65_add_bus().
On Tue, May 14, 2024 at 04:14:54PM -0500, Bjorn Helgaas wrote: > On Tue, May 14, 2024 at 05:41:48PM +0530, Siddharth Vadapalli wrote: > > On Mon, May 13, 2024 at 04:53:50PM -0500, Bjorn Helgaas wrote: > ... > > > I'm not quite clear on the mechanism, but it would be helpful to at > > > least know what's wrong and on what platform. E.g., currently v4.90 > > > suffers Completion Timeouts and 45 second boot delays? And this patch > > > fixes that? > > > > Yes, the Completion Timeouts cause the 45 second boot delays and this > > patch fixes that. > > And this problem happens on AM654x/v4.90a, right? I really want the > commit log to say what platform is affected! > > Maybe something like this? > > PCI: keystone: Enable BAR 0 only for v3.65a > > The BAR 0 initialization done by ks_pcie_v3_65_add_bus() should > happen for v3.65a devices only. On other devices, BAR 0 should be > left disabled, as it is by dw_pcie_setup_rc(). > > After 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() > callback to use add_bus"), ks_pcie_v3_65_add_bus() enabled BAR 0 for > both v3.65a and v4.90a devices. On the AM654x SoC, which uses > v4.90a, enabling BAR 0 causes Completion Timeouts when setting up > MSI-X. These timeouts delay boot of the AM654x by about 45 seconds. > > Move the BAR 0 initialization to ks_pcie_msi_host_init(), which is > only used for v3.65a devices, and remove ks_pcie_v3_65_add_bus(). I haven't heard anything so I amended it to the above. But please correct me if it's wrong. Bjorn
On Wed, May 15, 2024 at 02:26:14PM -0500, Bjorn Helgaas wrote: > On Tue, May 14, 2024 at 04:14:54PM -0500, Bjorn Helgaas wrote: > > On Tue, May 14, 2024 at 05:41:48PM +0530, Siddharth Vadapalli wrote: > > > On Mon, May 13, 2024 at 04:53:50PM -0500, Bjorn Helgaas wrote: > > ... > > > > > I'm not quite clear on the mechanism, but it would be helpful to at > > > > least know what's wrong and on what platform. E.g., currently v4.90 > > > > suffers Completion Timeouts and 45 second boot delays? And this patch > > > > fixes that? > > > > > > Yes, the Completion Timeouts cause the 45 second boot delays and this > > > patch fixes that. > > > > And this problem happens on AM654x/v4.90a, right? I really want the > > commit log to say what platform is affected! > > > > Maybe something like this? > > > > PCI: keystone: Enable BAR 0 only for v3.65a > > > > The BAR 0 initialization done by ks_pcie_v3_65_add_bus() should > > happen for v3.65a devices only. On other devices, BAR 0 should be > > left disabled, as it is by dw_pcie_setup_rc(). > > > > After 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() > > callback to use add_bus"), ks_pcie_v3_65_add_bus() enabled BAR 0 for > > both v3.65a and v4.90a devices. On the AM654x SoC, which uses > > v4.90a, enabling BAR 0 causes Completion Timeouts when setting up > > MSI-X. These timeouts delay boot of the AM654x by about 45 seconds. > > > > Move the BAR 0 initialization to ks_pcie_msi_host_init(), which is > > only used for v3.65a devices, and remove ks_pcie_v3_65_add_bus(). > > I haven't heard anything so I amended it to the above. But please > correct me if it's wrong. I would suggest specifying the failing combination since I do not know if there is another device that is using v4.90a but doesn't see this issue. What is certain is that this issue is seen with the v4.90a controller on AM654x platform. Despite the PCIe Controller version remaining the same across different platforms, it might be possible that not all features supported by the PCIe Controller are enabled on all platforms. For that reason, it appears to me that the subject could be: PCI: keystone: Don't enable BAR 0 for AM654x which implicitly indicates the combination as well (v4.90a on AM654x). The commit message's contents could be reduced to: After 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus"), ks_pcie_v3_65_add_bus() enabled BAR 0 for both v3.65a and v4.90a devices. On the AM654x SoC, which uses v4.90a, enabling BAR 0 causes Completion Timeouts when setting up MSI-X. These timeouts delay boot of the AM654x by about 45 seconds. Move the BAR 0 initialization to ks_pcie_msi_host_init(), which is only used for v3.65a devices, and remove ks_pcie_v3_65_add_bus(). by dropping: The BAR 0 initialization done by ks_pcie_v3_65_add_bus() should happen for v3.65a devices only. On other devices, BAR 0 should be left disabled, as it is by dw_pcie_setup_rc(). The reason behind dropping the above paragraph is that BAR 0 could probably be enabled on other controller versions as well, but not on the v4.90a controller on the AM654x SoC. Thank you Bjorn, for enhancing the commit message. Regards, Siddharth.
On Thu, May 16, 2024 at 11:07:27AM +0530, Siddharth Vadapalli wrote: > On Wed, May 15, 2024 at 02:26:14PM -0500, Bjorn Helgaas wrote: > > On Tue, May 14, 2024 at 04:14:54PM -0500, Bjorn Helgaas wrote: > > > On Tue, May 14, 2024 at 05:41:48PM +0530, Siddharth Vadapalli wrote: > > > > On Mon, May 13, 2024 at 04:53:50PM -0500, Bjorn Helgaas wrote: > > > ... > > > > > > > I'm not quite clear on the mechanism, but it would be helpful to at > > > > > least know what's wrong and on what platform. E.g., currently v4.90 > > > > > suffers Completion Timeouts and 45 second boot delays? And this patch > > > > > fixes that? > > > > > > > > Yes, the Completion Timeouts cause the 45 second boot delays and this > > > > patch fixes that. > > > > > > And this problem happens on AM654x/v4.90a, right? I really want the > > > commit log to say what platform is affected! > > > > > > Maybe something like this? > > > > > > PCI: keystone: Enable BAR 0 only for v3.65a > > > > > > The BAR 0 initialization done by ks_pcie_v3_65_add_bus() should > > > happen for v3.65a devices only. On other devices, BAR 0 should be > > > left disabled, as it is by dw_pcie_setup_rc(). > > > > > > After 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() > > > callback to use add_bus"), ks_pcie_v3_65_add_bus() enabled BAR 0 for > > > both v3.65a and v4.90a devices. On the AM654x SoC, which uses > > > v4.90a, enabling BAR 0 causes Completion Timeouts when setting up > > > MSI-X. These timeouts delay boot of the AM654x by about 45 seconds. > > > > > > Move the BAR 0 initialization to ks_pcie_msi_host_init(), which is > > > only used for v3.65a devices, and remove ks_pcie_v3_65_add_bus(). > > > > I haven't heard anything so I amended it to the above. But please > > correct me if it's wrong. > > I would suggest specifying the failing combination since I do not know > if there is another device that is using v4.90a but doesn't see this > issue. What is certain is that this issue is seen with the v4.90a > controller on AM654x platform. Despite the PCIe Controller version > remaining the same across different platforms, it might be possible > that not all features supported by the PCIe Controller are enabled on > all platforms. For that reason, it appears to me that the subject could > be: > > PCI: keystone: Don't enable BAR 0 for AM654x > > which implicitly indicates the combination as well (v4.90a on AM654x). > > The commit message's contents could be reduced to: > > After 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() > callback to use add_bus"), ks_pcie_v3_65_add_bus() enabled BAR 0 for > both v3.65a and v4.90a devices. On the AM654x SoC, which uses > v4.90a, enabling BAR 0 causes Completion Timeouts when setting up > MSI-X. These timeouts delay boot of the AM654x by about 45 seconds. > > Move the BAR 0 initialization to ks_pcie_msi_host_init(), which is > only used for v3.65a devices, and remove ks_pcie_v3_65_add_bus(). > > by dropping: > > The BAR 0 initialization done by ks_pcie_v3_65_add_bus() should > happen for v3.65a devices only. On other devices, BAR 0 should be > left disabled, as it is by dw_pcie_setup_rc(). > > The reason behind dropping the above paragraph is that BAR 0 could > probably be enabled on other controller versions as well, but not on the > v4.90a controller on the AM654x SoC. Thanks, that makes good sense, I changed the subject and dropped that paragraph. Bjorn
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index 5c073e520628..6cb3a4713009 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -289,6 +289,24 @@ static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie) static int ks_pcie_msi_host_init(struct dw_pcie_rp *pp) { + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + struct keystone_pcie *ks_pcie = to_keystone_pcie(pci); + + /* Configure and set up BAR0 */ + ks_pcie_set_dbi_mode(ks_pcie); + + /* Enable BAR0 */ + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); + + ks_pcie_clear_dbi_mode(ks_pcie); + + /* + * For BAR0, just setting bus address for inbound writes (MSI) should + * be sufficient. Use physical address to avoid any conflicts. + */ + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); + pp->msi_irq_chip = &ks_pcie_msi_irq_chip; return dw_pcie_allocate_domains(pp); } @@ -445,44 +463,10 @@ static struct pci_ops ks_child_pcie_ops = { .write = pci_generic_config_write, }; -/** - * ks_pcie_v3_65_add_bus() - keystone add_bus post initialization - * @bus: A pointer to the PCI bus structure. - * - * This sets BAR0 to enable inbound access for MSI_IRQ register - */ -static int ks_pcie_v3_65_add_bus(struct pci_bus *bus) -{ - struct dw_pcie_rp *pp = bus->sysdata; - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); - struct keystone_pcie *ks_pcie = to_keystone_pcie(pci); - - if (!pci_is_root_bus(bus)) - return 0; - - /* Configure and set up BAR0 */ - ks_pcie_set_dbi_mode(ks_pcie); - - /* Enable BAR0 */ - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); - - ks_pcie_clear_dbi_mode(ks_pcie); - - /* - * For BAR0, just setting bus address for inbound writes (MSI) should - * be sufficient. Use physical address to avoid any conflicts. - */ - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); - - return 0; -} - static struct pci_ops ks_pcie_ops = { .map_bus = dw_pcie_own_conf_map_bus, .read = pci_generic_config_read, .write = pci_generic_config_write, - .add_bus = ks_pcie_v3_65_add_bus, }; /**
In the process of converting .scan_bus() callbacks to .add_bus(), the ks_pcie_v3_65_scan_bus() function was changed to ks_pcie_v3_65_add_bus(). The .scan_bus() method belonged to ks_pcie_host_ops which was specific to controller version 3.65a, while the .add_bus() method had been added to ks_pcie_ops which is shared between the controller versions 3.65a and 4.90a. Neither the older ks_pcie_v3_65_scan_bus() method, nor the newer ks_pcie_v3_65_add_bus() method is applicable to the controller version 4.90a which is present in AM654x SoCs. Thus, as a fix, remove "ks_pcie_v3_65_add_bus()" and move its contents to the .msi_init callback "ks_pcie_msi_host_init()" which is specific to the 3.65a controller. Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus") Suggested-by: Serge Semin <fancer.lancer@gmail.com> Suggested-by: Bjorn Helgaas <helgaas@kernel.org> Suggested-by: Niklas Cassel <cassel@kernel.org> Reviewed-by: Niklas Cassel <cassel@kernel.org> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- drivers/pci/controller/dwc/pci-keystone.c | 52 ++++++++--------------- 1 file changed, 18 insertions(+), 34 deletions(-)