[PATCH v2 0/2] Fixes for PCI Keystone driver

[PATCH v2 0/2] Fixes for PCI Keystone driver

[Siddharth Vadapalli]

Hello,

This series adds fixes for the PCI Keystone driver.
The v1 of this series was posted on the 24th of March 2021:
https://lore.kernel.org/r/20210324121901.1856-1-kishon@ti.com/
There are no changes since v1 except for rebasing the series on the
latest linux-next.

There were no open comments on the v1 series so it appears to me that
the series got missed when merging patches to the PCI tree. I am
reposting the series after rebasing it on linux-next tagged next-20240523.

I don't have the device corresponding to the older "ti,keystone-pcie"
compatible so I wasn't able to test this series (specifically patch 2
of this series). I have only compile-tested this series and logically
verified it for correctness to the best of my knowledge.

Regards,
Siddharth.

Kishon Vijay Abraham I (2):
  PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie"
    compatible
  PCI: keystone: Add link up check in ks_child_pcie_ops.map_bus()

 drivers/pci/controller/dwc/pci-keystone.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie" compatible

[Siddharth Vadapalli]

From: Kishon Vijay Abraham I <kishon@ti.com>

commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x
Platforms") introduced configuring "enum dw_pcie_device_mode" as part of
device data ("struct ks_pcie_of_data"). However it failed to set mode
for "ti,keystone-pcie" compatible. Set mode as RootComplex for
"ti,keystone-pcie" compatible here.

Fixes: 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x Platforms")
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index d3a7d14ee685..3184546ba3b6 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -1064,6 +1064,7 @@ static int ks_pcie_am654_set_mode(struct device *dev,
 
 static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
 	.host_ops = &ks_pcie_host_ops,
+	.mode = DW_PCIE_RC_TYPE,
 	.version = DW_PCIE_VER_365A,
 };
 
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie" compatible

[Bjorn Helgaas]

On Fri, May 24, 2024 at 04:27:13PM +0530, Siddharth Vadapalli wrote:
> From: Kishon Vijay Abraham I <kishon@ti.com>
> 
> commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x
> Platforms") introduced configuring "enum dw_pcie_device_mode" as part of
> device data ("struct ks_pcie_of_data"). However it failed to set mode
> for "ti,keystone-pcie" compatible. Set mode as RootComplex for
> "ti,keystone-pcie" compatible here.

23284ad677a9 appeared in v5.10.  

But I guess RC support has not been broken since v5.10 because we
never used ks_pcie_rc_of_data.mode anyway?

It looks like the only use is here:

  #define DW_PCIE_VER_365A                0x3336352a
  #define DW_PCIE_VER_480A                0x3438302a

  ks_pcie_probe
  {
    ...
    mode = data->mode;
    ...
    if (dw_pcie_ver_is_ge(pci, 480A))
      ret = ks_pcie_am654_set_mode(dev, mode);
    else
      ret = ks_pcie_set_mode(dev);

so we don't even look at .mode unless the version is v4.80a or later,
and this is v3.65a?

So this is basically a cosmetic fix (but still worth doing for
readability!) and doesn't need a stable backport, right?

If so, I might amend the commit log to mention the fact that this
doesn't actually fix a functional issue.

> Fixes: 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x Platforms")
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index d3a7d14ee685..3184546ba3b6 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -1064,6 +1064,7 @@ static int ks_pcie_am654_set_mode(struct device *dev,
>  
>  static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
>  	.host_ops = &ks_pcie_host_ops,
> +	.mode = DW_PCIE_RC_TYPE,
>  	.version = DW_PCIE_VER_365A,
>  };
>  
> -- 
> 2.40.1
> 

Re: [PATCH v2 1/2] PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie" compatible

[Siddharth Vadapalli]

On Tue, Nov 05, 2024 at 06:57:58PM -0600, Bjorn Helgaas wrote:

Hello Bjorn,

> On Fri, May 24, 2024 at 04:27:13PM +0530, Siddharth Vadapalli wrote:
> > From: Kishon Vijay Abraham I <kishon@ti.com>
> > 
> > commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x
> > Platforms") introduced configuring "enum dw_pcie_device_mode" as part of
> > device data ("struct ks_pcie_of_data"). However it failed to set mode
> > for "ti,keystone-pcie" compatible. Set mode as RootComplex for
> > "ti,keystone-pcie" compatible here.
> 
> 23284ad677a9 appeared in v5.10.  
> 
> But I guess RC support has not been broken since v5.10 because we
> never used ks_pcie_rc_of_data.mode anyway?
> 
> It looks like the only use is here:
> 
>   #define DW_PCIE_VER_365A                0x3336352a
>   #define DW_PCIE_VER_480A                0x3438302a
> 
>   ks_pcie_probe
>   {
>     ...
>     mode = data->mode;
>     ...
>     if (dw_pcie_ver_is_ge(pci, 480A))
>       ret = ks_pcie_am654_set_mode(dev, mode);
>     else
>       ret = ks_pcie_set_mode(dev);

"mode" is used later on during probe at:

....
	switch (mode) {
	case DW_PCIE_RC_TYPE:
	...
	case DW_PCIE_EP_TYPE:
	...
	default:
		dev_err(dev, "INVALID device type %d\n", mode);
	}
....

> 
> so we don't even look at .mode unless the version is v4.80a or later,
> and this is v3.65a?
> 
> So this is basically a cosmetic fix (but still worth doing for
> readability!) and doesn't need a stable backport, right?

I suppose that "data->mode" will default to zero for v3.65a prior to
this commit, corresponding to "DW_PCIE_UNKNOWN_TYPE" rather than the
correct value of "DW_PCIE_RC_TYPE". Since I don't have an SoC with the
v3.65a version of the controller, I cannot test it out, but I presume
that the "INVALID device type 0" error will be displayed. Though the probe
will not fail since the "default" case doesn't return an error code, the
controller probably will not be functional as the configuration associated
with the "DW_PCIE_RC_TYPE" case has been skipped. Hence, I believe that
this fix should be backported.

Regards,
Siddharth.

Re: [PATCH v2 1/2] PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie" compatible

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 11:36:38AM +0530, Siddharth Vadapalli wrote:
> On Tue, Nov 05, 2024 at 06:57:58PM -0600, Bjorn Helgaas wrote:
> > On Fri, May 24, 2024 at 04:27:13PM +0530, Siddharth Vadapalli wrote:
> > > From: Kishon Vijay Abraham I <kishon@ti.com>
> > > 
> > > commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x
> > > Platforms") introduced configuring "enum dw_pcie_device_mode" as part of
> > > device data ("struct ks_pcie_of_data"). However it failed to set mode
> > > for "ti,keystone-pcie" compatible. Set mode as RootComplex for
> > > "ti,keystone-pcie" compatible here.
> > 
> > 23284ad677a9 appeared in v5.10.  
> > 
> > But I guess RC support has not been broken since v5.10 because we
> > never used ks_pcie_rc_of_data.mode anyway?
> > 
> > It looks like the only use is here:
> > 
> >   #define DW_PCIE_VER_365A                0x3336352a
> >   #define DW_PCIE_VER_480A                0x3438302a
> > 
> >   ks_pcie_probe
> >   {
> >     ...
> >     mode = data->mode;
> >     ...
> >     if (dw_pcie_ver_is_ge(pci, 480A))
> >       ret = ks_pcie_am654_set_mode(dev, mode);
> >     else
> >       ret = ks_pcie_set_mode(dev);
> 
> "mode" is used later on during probe at:
> 
> ....
> 	switch (mode) {
> 	case DW_PCIE_RC_TYPE:
> 	...
> 	case DW_PCIE_EP_TYPE:
> 	...
> 	default:
> 		dev_err(dev, "INVALID device type %d\n", mode);
> 	}
> ....

How did I miss that? :)  It is literally two lines down.

> > so we don't even look at .mode unless the version is v4.80a or later,
> > and this is v3.65a?
> > 
> > So this is basically a cosmetic fix (but still worth doing for
> > readability!) and doesn't need a stable backport, right?
> 
> I suppose that "data->mode" will default to zero for v3.65a prior to
> this commit, corresponding to "DW_PCIE_UNKNOWN_TYPE" rather than the
> correct value of "DW_PCIE_RC_TYPE". Since I don't have an SoC with the
> v3.65a version of the controller, I cannot test it out, but I presume
> that the "INVALID device type 0" error will be displayed. Though the probe
> will not fail since the "default" case doesn't return an error code, the
> controller probably will not be functional as the configuration associated
> with the "DW_PCIE_RC_TYPE" case has been skipped. Hence, I believe that
> this fix should be backported.

I guess nobody really cares too much since it's been broken for almost
four years.

But indeed, sounds like it should have a stable tag and maybe a commit
log hint about what the failure looks like.

Thanks!

Bjorn

Re: [PATCH v2 1/2] PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie" compatible

[Krzysztof Wilczyński]

Hello,

[...]
> > I suppose that "data->mode" will default to zero for v3.65a prior to
> > this commit, corresponding to "DW_PCIE_UNKNOWN_TYPE" rather than the
> > correct value of "DW_PCIE_RC_TYPE". Since I don't have an SoC with the
> > v3.65a version of the controller, I cannot test it out, but I presume
> > that the "INVALID device type 0" error will be displayed. Though the probe
> > will not fail since the "default" case doesn't return an error code, the
> > controller probably will not be functional as the configuration associated
> > with the "DW_PCIE_RC_TYPE" case has been skipped. Hence, I believe that
> > this fix should be backported.
> 
> I guess nobody really cares too much since it's been broken for almost
> four years.
> 
> But indeed, sounds like it should have a stable tag and maybe a commit
> log hint about what the failure looks like.

Added Cc for stable releases.  Siddharth, let me know how to update the
commit log per Bjorn feedback, so I can do it directly on the branch.

Thank you!

	Krzysztof

Re: [PATCH v2 1/2] PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie" compatible

[Siddharth Vadapalli]

On Thu, Nov 07, 2024 at 01:05:20AM +0900, Krzysztof Wilczyński wrote:

Hello Krzysztof,

> Hello,
> 
> [...]
> > > I suppose that "data->mode" will default to zero for v3.65a prior to
> > > this commit, corresponding to "DW_PCIE_UNKNOWN_TYPE" rather than the
> > > correct value of "DW_PCIE_RC_TYPE". Since I don't have an SoC with the
> > > v3.65a version of the controller, I cannot test it out, but I presume
> > > that the "INVALID device type 0" error will be displayed. Though the probe
> > > will not fail since the "default" case doesn't return an error code, the
> > > controller probably will not be functional as the configuration associated
> > > with the "DW_PCIE_RC_TYPE" case has been skipped. Hence, I believe that
> > > this fix should be backported.
> > 
> > I guess nobody really cares too much since it's been broken for almost
> > four years.
> > 
> > But indeed, sounds like it should have a stable tag and maybe a commit
> > log hint about what the failure looks like.
> 
> Added Cc for stable releases.  Siddharth, let me know how to update the
> commit log per Bjorn feedback, so I can do it directly on the branch.

The existing commit message could be replaced by the following:

------------------------------------------------------------------------
commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x
Platforms") introduced configuring "enum dw_pcie_device_mode" as part of
device data ("struct ks_pcie_of_data"). However it failed to set the mode
for "ti,keystone-pcie" compatible.

Since the mode defaults to "DW_PCIE_UNKNOWN_TYPE", the following error
message is displayed:
	"INVALID device type 0"
for the v3.65a controller. Despite the driver probing successfully, the
controller may not be functional in the Root Complex mode of operation.

So, set the mode as Root Complex for "ti,keystone-pcie" compatible to fix
this.
------------------------------------------------------------------------

Regards,
Siddharth.

Re: [PATCH v2 1/2] PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie" compatible

[Krzysztof Wilczyński]

Hello,

> > Added Cc for stable releases.  Siddharth, let me know how to update the
> > commit log per Bjorn feedback, so I can do it directly on the branch.
> 
> The existing commit message could be replaced by the following:
> 
> ------------------------------------------------------------------------
> commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x
> Platforms") introduced configuring "enum dw_pcie_device_mode" as part of
> device data ("struct ks_pcie_of_data"). However it failed to set the mode
> for "ti,keystone-pcie" compatible.
> 
> Since the mode defaults to "DW_PCIE_UNKNOWN_TYPE", the following error
> message is displayed:
> 	"INVALID device type 0"
> for the v3.65a controller. Despite the driver probing successfully, the
> controller may not be functional in the Root Complex mode of operation.
> 
> So, set the mode as Root Complex for "ti,keystone-pcie" compatible to fix
> this.
> ------------------------------------------------------------------------

Done.  See the following:

  - https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/keystone&id=5a938ed9481b0c06cb97aec45e722a80568256fd

Thank you!

	Krzysztof

Re: [PATCH v2 1/2] PCI: keystone: Set mode as RootComplex for "ti,keystone-pcie" compatible

[Siddharth Vadapalli]

On Fri, Nov 08, 2024 at 12:51:44AM +0900, Krzysztof Wilczyński wrote:
> Hello,
> 
> > > Added Cc for stable releases.  Siddharth, let me know how to update the
> > > commit log per Bjorn feedback, so I can do it directly on the branch.
> > 
> > The existing commit message could be replaced by the following:
> > 
> > ------------------------------------------------------------------------
> > commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x
> > Platforms") introduced configuring "enum dw_pcie_device_mode" as part of
> > device data ("struct ks_pcie_of_data"). However it failed to set the mode
> > for "ti,keystone-pcie" compatible.
> > 
> > Since the mode defaults to "DW_PCIE_UNKNOWN_TYPE", the following error
> > message is displayed:
> > 	"INVALID device type 0"
> > for the v3.65a controller. Despite the driver probing successfully, the
> > controller may not be functional in the Root Complex mode of operation.
> > 
> > So, set the mode as Root Complex for "ti,keystone-pcie" compatible to fix
> > this.
> > ------------------------------------------------------------------------
> 
> Done.  See the following:
> 
>   - https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/keystone&id=5a938ed9481b0c06cb97aec45e722a80568256fd

LGTM. Thank you for updating the commit message :)

Regards,
Siddharth.

[PATCH v2 2/2] PCI: keystone: Add link up check in ks_child_pcie_ops.map_bus()

[Siddharth Vadapalli]

From: Kishon Vijay Abraham I <kishon@ti.com>

K2G forwards the error triggered by a link-down state (e.g., no connected
endpoint device) on the system bus for PCI configuration transactions;
these errors are reported as an SError at system level, which is fatal and
hangs the system. So fix it similar to how it was done in designware core
driver commit 15b23906347c ("PCI: dwc: Add link up check in
dw_child_pcie_ops.map_bus()")

Fixes: 10a797c6e54a ("PCI: dwc: keystone: Use pci_ops for config space accessors")
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 3184546ba3b6..ca476fa584e7 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -430,6 +430,17 @@ static void __iomem *ks_pcie_other_map_bus(struct pci_bus *bus,
 	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
 	u32 reg;
 
+	/*
+	 * Checking whether the link is up here is a last line of defense
+	 * against platforms that forward errors on the system bus as
+	 * SError upon PCI configuration transactions issued when the link
+	 * is down. This check is racy by definition and does not stop
+	 * the system from triggering an SError if the link goes down
+	 * after this check is performed.
+	 */
+	if (!dw_pcie_link_up(pci))
+		return NULL;
+
 	reg = CFG_BUS(bus->number) | CFG_DEVICE(PCI_SLOT(devfn)) |
 		CFG_FUNC(PCI_FUNC(devfn));
 	if (!pci_is_root_bus(bus->parent))
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] Fixes for PCI Keystone driver

[Krzysztof Wilczyński]

> This series adds fixes for the PCI Keystone driver.
> The v1 of this series was posted on the 24th of March 2021:
> https://lore.kernel.org/r/20210324121901.1856-1-kishon@ti.com/
> There are no changes since v1 except for rebasing the series on the
> latest linux-next.
> 
> There were no open comments on the v1 series so it appears to me that
> the series got missed when merging patches to the PCI tree. I am
> reposting the series after rebasing it on linux-next tagged next-20240523.
> 
> I don't have the device corresponding to the older "ti,keystone-pcie"
> compatible so I wasn't able to test this series (specifically patch 2
> of this series). I have only compile-tested this series and logically
> verified it for correctness to the best of my knowledge.

Applied to controller/keystone, thank you!

[01/02] PCI: keystone: Set mode as Root Complex for "ti,keystone-pcie" compatible
        https://git.kernel.org/pci/pci/c/7a941315dd2a

[02/02] PCI: keystone: Add link up check in ks_pcie_other_map_bus()
        https://git.kernel.org/pci/pci/c/76a496ad8536

	Krzysztof