[PATCH v7 0/2] PCI: keystone: Fix pci_ops for AM654x SoC

[PATCH v7 0/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Siddharth Vadapalli]

Hello,

This series is based on linux-next tagged next-20240328.

v6:
https://lore.kernel.org/r/20240326144258.2404433-1-s-vadapalli@ti.com/
Changes since v6:
- The v6 patch has been split into two patches based on Bjorn's
  suggestion at:
  https://lore.kernel.org/r/20240326232403.GA1502764@bhelgaas/

Regards,
Siddharth.

Siddharth Vadapalli (2):
  PCI: keystone: Relocate ks_pcie_set/clear_dbi_mode()
  PCI: keystone: Fix pci_ops for AM654x SoC

 drivers/pci/controller/dwc/pci-keystone.c | 136 ++++++++++------------
 1 file changed, 60 insertions(+), 76 deletions(-)

-- 
2.40.1

[PATCH v7 1/2] PCI: keystone: Relocate ks_pcie_set/clear_dbi_mode()

[Siddharth Vadapalli]

Relocate ks_pcie_set_dbi_mode() and ks_pcie_clear_dbi_mode() to avoid
forward declaration in a subsequent patch. No functional change intended.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 84 +++++++++++------------
 1 file changed, 42 insertions(+), 42 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 844de4418724..5c073e520628 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -245,6 +245,48 @@ static struct irq_chip ks_pcie_msi_irq_chip = {
 	.irq_unmask = ks_pcie_msi_unmask,
 };
 
+/**
+ * ks_pcie_set_dbi_mode() - Set DBI mode to access overlaid BAR mask registers
+ * @ks_pcie: A pointer to the keystone_pcie structure which holds the KeyStone
+ *	     PCIe host controller driver information.
+ *
+ * Since modification of dbi_cs2 involves different clock domain, read the
+ * status back to ensure the transition is complete.
+ */
+static void ks_pcie_set_dbi_mode(struct keystone_pcie *ks_pcie)
+{
+	u32 val;
+
+	val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
+	val |= DBI_CS2;
+	ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);
+
+	do {
+		val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
+	} while (!(val & DBI_CS2));
+}
+
+/**
+ * ks_pcie_clear_dbi_mode() - Disable DBI mode
+ * @ks_pcie: A pointer to the keystone_pcie structure which holds the KeyStone
+ *	     PCIe host controller driver information.
+ *
+ * Since modification of dbi_cs2 involves different clock domain, read the
+ * status back to ensure the transition is complete.
+ */
+static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie)
+{
+	u32 val;
+
+	val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
+	val &= ~DBI_CS2;
+	ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);
+
+	do {
+		val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
+	} while (val & DBI_CS2);
+}
+
 static int ks_pcie_msi_host_init(struct dw_pcie_rp *pp)
 {
 	pp->msi_irq_chip = &ks_pcie_msi_irq_chip;
@@ -340,48 +382,6 @@ static const struct irq_domain_ops ks_pcie_intx_irq_domain_ops = {
 	.xlate = irq_domain_xlate_onetwocell,
 };
 
-/**
- * ks_pcie_set_dbi_mode() - Set DBI mode to access overlaid BAR mask registers
- * @ks_pcie: A pointer to the keystone_pcie structure which holds the KeyStone
- *	     PCIe host controller driver information.
- *
- * Since modification of dbi_cs2 involves different clock domain, read the
- * status back to ensure the transition is complete.
- */
-static void ks_pcie_set_dbi_mode(struct keystone_pcie *ks_pcie)
-{
-	u32 val;
-
-	val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
-	val |= DBI_CS2;
-	ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);
-
-	do {
-		val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
-	} while (!(val & DBI_CS2));
-}
-
-/**
- * ks_pcie_clear_dbi_mode() - Disable DBI mode
- * @ks_pcie: A pointer to the keystone_pcie structure which holds the KeyStone
- *	     PCIe host controller driver information.
- *
- * Since modification of dbi_cs2 involves different clock domain, read the
- * status back to ensure the transition is complete.
- */
-static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie)
-{
-	u32 val;
-
-	val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
-	val &= ~DBI_CS2;
-	ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);
-
-	do {
-		val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
-	} while (val & DBI_CS2);
-}
-
 static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
 {
 	u32 val;
-- 
2.40.1

[PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Siddharth Vadapalli]

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(-)

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

Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Bjorn Helgaas]

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
> 

Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Siddharth Vadapalli]

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.

Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Bjorn Helgaas]

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().

Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Bjorn Helgaas]

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

Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Siddharth Vadapalli]

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.

Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Bjorn Helgaas]

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

Re: [PATCH v7 0/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Krzysztof Wilczyński]

> This series is based on linux-next tagged next-20240328.

Applied to controller/keystone, thank you!

[01/02] PCI: keystone: Relocate ks_pcie_set/clear_dbi_mode()
        https://git.kernel.org/pci/pci/c/390f4969f26f
[02/02] PCI: keystone: Fix pci_ops for AM654x SoC
        https://git.kernel.org/pci/pci/c/9e6ffee1f846

	Krzysztof