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

[Siddharth Vadapalli <s-vadapalli@ti.com>]

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.