Query regarding the use of pcie-designware-plat.c file

Query regarding the use of pcie-designware-plat.c file

[Vidya Sagar]

Hi,
I would like to know what is the use of pcie-designware-plat.c file. 
This looks like a skeleton file and can't really work with any specific 
hardware as such.
Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT 
is enabled in a system where a Synopsys DesignWare IP based PCIe 
controller is present and its configuration is enabled (Ex:- Tegra194 
system with CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen 
that the probe of pcie-designware-plat.c called first (because all DWC 
based PCIe controller nodes have "snps,dw-pcie" compatibility string) 
and can crash the system.
One solution to this issue is to remove the "snps,dw-pcie" from the 
compatibility string (as was done through the commit f9f711efd441 
("arm64: tegra: Fix Tegra194 PCIe compatible string") but it seems like 
a localized fix for Tegra194 where the issue potentially is global, as 
in, the crash can happen on any platform.
So, wondering if the config option CONFIG_PCIE_DW_PLAT can be removed 
altogether for pcie-designware-plat.c?

Thanks,
Vidya Sagar

RE: Query regarding the use of pcie-designware-plat.c file

[Gustavo Pimentel]

Hi Vidya,

The pcie-designware-plat.c is the driver for the Synopsys PCIe RC IP 
prototype.

-Gustavo

On Tue, Jun 8, 2021 at 20:22:37, Vidya Sagar <vidyas@nvidia.com> wrote:

> Hi,
> I would like to know what is the use of pcie-designware-plat.c file. 
> This looks like a skeleton file and can't really work with any specific 
> hardware as such.
> Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT 
> is enabled in a system where a Synopsys DesignWare IP based PCIe 
> controller is present and its configuration is enabled (Ex:- Tegra194 
> system with CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen 
> that the probe of pcie-designware-plat.c called first (because all DWC 
> based PCIe controller nodes have "snps,dw-pcie" compatibility string) 
> and can crash the system.
> One solution to this issue is to remove the "snps,dw-pcie" from the 
> compatibility string (as was done through the commit f9f711efd441 
> ("arm64: tegra: Fix Tegra194 PCIe compatible string") but it seems like 
> a localized fix for Tegra194 where the issue potentially is global, as 
> in, the crash can happen on any platform.
> So, wondering if the config option CONFIG_PCIE_DW_PLAT can be removed 
> altogether for pcie-designware-plat.c?
> 
> Thanks,
> Vidya Sagar

Re: Query regarding the use of pcie-designware-plat.c file

[Vidya Sagar]

On 6/9/2021 2:47 AM, Gustavo Pimentel wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hi Vidya,
> 
> The pcie-designware-plat.c is the driver for the Synopsys PCIe RC IP
> prototype.
Thanks for the info Gustavo.
But, I don't see any DT file having only "snps,dw-pcie" compatibility 
string. All the DT files that have "snps,dw-pci" compatibility string 
also have their platform specific compatibility string and their 
respective host controller drivers. Also, it is the platform specific 
compatibility string that is used for binding purpose with their 
respective drivers and not the "snps,dw-pcie". So, wondering when will 
pcie-designware-plat.c be used as there is not DT file which has only 
"snps,dw-pcie" as the compatibility string.

- Vidya Sagar
> 
> -Gustavo
> 
> On Tue, Jun 8, 2021 at 20:22:37, Vidya Sagar <vidyas@nvidia.com> wrote:
> 
>> Hi,
>> I would like to know what is the use of pcie-designware-plat.c file.
>> This looks like a skeleton file and can't really work with any specific
>> hardware as such.
>> Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT
>> is enabled in a system where a Synopsys DesignWare IP based PCIe
>> controller is present and its configuration is enabled (Ex:- Tegra194
>> system with CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen
>> that the probe of pcie-designware-plat.c called first (because all DWC
>> based PCIe controller nodes have "snps,dw-pcie" compatibility string)
>> and can crash the system.
>> One solution to this issue is to remove the "snps,dw-pcie" from the
>> compatibility string (as was done through the commit f9f711efd441
>> ("arm64: tegra: Fix Tegra194 PCIe compatible string") but it seems like
>> a localized fix for Tegra194 where the issue potentially is global, as
>> in, the crash can happen on any platform.
>> So, wondering if the config option CONFIG_PCIE_DW_PLAT can be removed
>> altogether for pcie-designware-plat.c?
>>
>> Thanks,
>> Vidya Sagar
> 
> 

Re: Query regarding the use of pcie-designware-plat.c file

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

On Wed, Jun 09, 2021 at 10:56:48AM +0530, Vidya Sagar wrote:
> 
> 
> On 6/9/2021 2:47 AM, Gustavo Pimentel wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Vidya,
> > 
> > The pcie-designware-plat.c is the driver for the Synopsys PCIe RC IP
> > prototype.
> Thanks for the info Gustavo.
> But, I don't see any DT file having only "snps,dw-pcie" compatibility
> string. All the DT files that have "snps,dw-pci" compatibility string also
> have their platform specific compatibility string and their respective host
> controller drivers. Also, it is the platform specific compatibility string
> that is used for binding purpose with their respective drivers and not the
> "snps,dw-pcie". So, wondering when will pcie-designware-plat.c be used as
> there is not DT file which has only "snps,dw-pcie" as the compatibility
> string.

Sounds to me like we have two options:

  1. If there's indeed real hardware that's identified by the existing
     "snps,dw-pcie" compatible string, then it's wrong for other devices
     to list that in their compatible string because they are likely not
     compatible with that (i.e. they might be from a register point of
     view, but at least from an integration point of view they usually
     differ).

  2. If "snps,dw-pcie" is meant to describe the fact that these are all
     based off the same IP but may be differently integrated, then there
     should be no driver matching on that compatible string.

Option 2 is not very robust because somebody could easily add a matching
driver at some point in the future. Also, if we don't match on a
compatible string there's not a lot of use in listing it in DT in the
first place.

So I think option 1 would be preferred.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Query regarding the use of pcie-designware-plat.c file

[Bjorn Helgaas]

On Wed, Jun 09, 2021 at 12:52:37AM +0530, Vidya Sagar wrote:
> Hi,
> I would like to know what is the use of pcie-designware-plat.c file. This
> looks like a skeleton file and can't really work with any specific hardware
> as such.
> Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT is
> enabled in a system where a Synopsys DesignWare IP based PCIe controller is
> present and its configuration is enabled (Ex:- Tegra194 system with
> CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen that the probe of
> pcie-designware-plat.c called first (because all DWC based PCIe controller
> nodes have "snps,dw-pcie" compatibility string) and can crash the system.

What's the crash?  If a device claims to be compatible with
"snps,dw-pcie" and pcie-designware-plat.c claims to know how to
operate "snps,dw-pcie" devices, it seems like something is wrong.

"snps,dw-pcie" is a generic device type, so pcie-designware-plat.c
might not know how to operate device-specific details of some of those
devices, but basic functionality should work and it certainly
shouldn't crash.

Bjorn

Re: Query regarding the use of pcie-designware-plat.c file

[Jon Hunter]

On 09/06/2021 17:30, Bjorn Helgaas wrote:
> On Wed, Jun 09, 2021 at 12:52:37AM +0530, Vidya Sagar wrote:
>> Hi,
>> I would like to know what is the use of pcie-designware-plat.c file. This
>> looks like a skeleton file and can't really work with any specific hardware
>> as such.
>> Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT is
>> enabled in a system where a Synopsys DesignWare IP based PCIe controller is
>> present and its configuration is enabled (Ex:- Tegra194 system with
>> CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen that the probe of
>> pcie-designware-plat.c called first (because all DWC based PCIe controller
>> nodes have "snps,dw-pcie" compatibility string) and can crash the system.
> 
> What's the crash?  If a device claims to be compatible with
> "snps,dw-pcie" and pcie-designware-plat.c claims to know how to
> operate "snps,dw-pcie" devices, it seems like something is wrong.
> 
> "snps,dw-pcie" is a generic device type, so pcie-designware-plat.c
> might not know how to operate device-specific details of some of those
> devices, but basic functionality should work and it certainly
> shouldn't crash.

It is not really a crash but a hang when trying to access the hardware
before it has been properly initialised.

The scenario I saw was that if the Tegra194 PCIe driver was built as a
module but the pcie-designware-plat.c was built into the kernel, then on
boot we would attempt to probe the pcie-designware-plat.c driver because
module was not available yet and this would hang. Hence, I removed the
"snps,dw-pcie" compatible string for Tegra194 to avoid this and ONLY
probe the Tegra194 PCIe driver.

Sagar is wondering why this hang is only seen/reported for Tegra and
could this happen to other platforms? I think that is potentially could.

Jon

-- 
nvpublic

Re: Query regarding the use of pcie-designware-plat.c file

[Vidya Sagar]

Bjorn / Lorenzo,
Could you please comment on the options proposed by Thierry?

- Vidya Sagar

On 6/9/2021 3:19 PM, Thierry Reding wrote:
> On Wed, Jun 09, 2021 at 10:56:48AM +0530, Vidya Sagar wrote:
>>
>>
>> On 6/9/2021 2:47 AM, Gustavo Pimentel wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> Hi Vidya,
>>>
>>> The pcie-designware-plat.c is the driver for the Synopsys PCIe RC IP
>>> prototype.
>> Thanks for the info Gustavo.
>> But, I don't see any DT file having only "snps,dw-pcie" compatibility
>> string. All the DT files that have "snps,dw-pci" compatibility string also
>> have their platform specific compatibility string and their respective host
>> controller drivers. Also, it is the platform specific compatibility string
>> that is used for binding purpose with their respective drivers and not the
>> "snps,dw-pcie". So, wondering when will pcie-designware-plat.c be used as
>> there is not DT file which has only "snps,dw-pcie" as the compatibility
>> string.
> 
> Sounds to me like we have two options:
> 
>    1. If there's indeed real hardware that's identified by the existing
>       "snps,dw-pcie" compatible string, then it's wrong for other devices
>       to list that in their compatible string because they are likely not
>       compatible with that (i.e. they might be from a register point of
>       view, but at least from an integration point of view they usually
>       differ).
> 
>    2. If "snps,dw-pcie" is meant to describe the fact that these are all
>       based off the same IP but may be differently integrated, then there
>       should be no driver matching on that compatible string.
> 
> Option 2 is not very robust because somebody could easily add a matching
> driver at some point in the future. Also, if we don't match on a
> compatible string there's not a lot of use in listing it in DT in the
> first place.
> 
> So I think option 1 would be preferred.
> 
> Thierry
> 

Re: Query regarding the use of pcie-designware-plat.c file

[Bjorn Helgaas]

On Wed, Jun 09, 2021 at 05:54:38PM +0100, Jon Hunter wrote:
> 
> On 09/06/2021 17:30, Bjorn Helgaas wrote:
> > On Wed, Jun 09, 2021 at 12:52:37AM +0530, Vidya Sagar wrote:
> >> Hi,
> >> I would like to know what is the use of pcie-designware-plat.c file. This
> >> looks like a skeleton file and can't really work with any specific hardware
> >> as such.
> >> Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT is
> >> enabled in a system where a Synopsys DesignWare IP based PCIe controller is
> >> present and its configuration is enabled (Ex:- Tegra194 system with
> >> CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen that the probe of
> >> pcie-designware-plat.c called first (because all DWC based PCIe controller
> >> nodes have "snps,dw-pcie" compatibility string) and can crash the system.
> > 
> > What's the crash?  If a device claims to be compatible with
> > "snps,dw-pcie" and pcie-designware-plat.c claims to know how to
> > operate "snps,dw-pcie" devices, it seems like something is wrong.
> > 
> > "snps,dw-pcie" is a generic device type, so pcie-designware-plat.c
> > might not know how to operate device-specific details of some of those
> > devices, but basic functionality should work and it certainly
> > shouldn't crash.
> 
> It is not really a crash but a hang when trying to access the hardware
> before it has been properly initialised.

This doesn't really answer my question.

If the hardware claims to be compatible with "snps,dw-pcie" and a
driver knows how to operate "snps,dw-pcie" devices, it should work.

If the hardware requires initialization that is not part of the
"snps,dw-pcie" programming model, it should not claim to be compatible
with "snps,dw-pcie".  Or, if pcie-designware-plat.c is missing some
init that *is* part of the programming model, maybe it needs to be
enhanced?

> The scenario I saw was that if the Tegra194 PCIe driver was built as a
> module but the pcie-designware-plat.c was built into the kernel, then on
> boot we would attempt to probe the pcie-designware-plat.c driver because
> module was not available yet and this would hang. Hence, I removed the
> "snps,dw-pcie" compatible string for Tegra194 to avoid this and ONLY
> probe the Tegra194 PCIe driver.

Maybe something like driver_override (I know this is supported via
sysfs, but maybe not via a kernel parameter) or a module parameter for
pcie-designware-plat.c to keep it from claiming devices?

> Sagar is wondering why this hang is only seen/reported for Tegra and
> could this happen to other platforms? I think that is potentially could.

Maybe pcie-designware-plat.c works on other platforms, i.e., they
don't require the hardware init?

Re: Query regarding the use of pcie-designware-plat.c file

[Rob Herring]

On Tue, Jun 15, 2021 at 1:42 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Jun 09, 2021 at 05:54:38PM +0100, Jon Hunter wrote:
> >
> > On 09/06/2021 17:30, Bjorn Helgaas wrote:
> > > On Wed, Jun 09, 2021 at 12:52:37AM +0530, Vidya Sagar wrote:
> > >> Hi,
> > >> I would like to know what is the use of pcie-designware-plat.c file. This
> > >> looks like a skeleton file and can't really work with any specific hardware
> > >> as such.
> > >> Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT is
> > >> enabled in a system where a Synopsys DesignWare IP based PCIe controller is
> > >> present and its configuration is enabled (Ex:- Tegra194 system with
> > >> CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen that the probe of
> > >> pcie-designware-plat.c called first (because all DWC based PCIe controller
> > >> nodes have "snps,dw-pcie" compatibility string) and can crash the system.

Linux provides no guarantees of what driver will probe if multiple
match. This has been a known 'problem' forever.

> > > What's the crash?  If a device claims to be compatible with
> > > "snps,dw-pcie" and pcie-designware-plat.c claims to know how to
> > > operate "snps,dw-pcie" devices, it seems like something is wrong.
> > >
> > > "snps,dw-pcie" is a generic device type, so pcie-designware-plat.c
> > > might not know how to operate device-specific details of some of those
> > > devices, but basic functionality should work and it certainly
> > > shouldn't crash.
> >
> > It is not really a crash but a hang when trying to access the hardware
> > before it has been properly initialised.
>
> This doesn't really answer my question.
>
> If the hardware claims to be compatible with "snps,dw-pcie" and a
> driver knows how to operate "snps,dw-pcie" devices, it should work.

I try to tell people that generic compatibles like this are pointless...

> If the hardware requires initialization that is not part of the
> "snps,dw-pcie" programming model, it should not claim to be compatible
> with "snps,dw-pcie".  Or, if pcie-designware-plat.c is missing some
> init that *is* part of the programming model, maybe it needs to be
> enhanced?

The driver will only work on an implementation with no clocks (though
the binding defines clocks), resets, phys, host specific registers or
interrupt muxing. That's practically no one. We could probably enhance
the driver to handle the first 3 if there's not differing sequencing
requirements. If we did this, we'd move the specific compatibles to
the plat driver, not add "snps,dw-pcie" to DT (because internal kernel
refactoring shouldn't require a DT change).

> > The scenario I saw was that if the Tegra194 PCIe driver was built as a
> > module but the pcie-designware-plat.c was built into the kernel, then on
> > boot we would attempt to probe the pcie-designware-plat.c driver because
> > module was not available yet and this would hang. Hence, I removed the
> > "snps,dw-pcie" compatible string for Tegra194 to avoid this and ONLY
> > probe the Tegra194 PCIe driver.
>
> Maybe something like driver_override (I know this is supported via
> sysfs, but maybe not via a kernel parameter) or a module parameter for
> pcie-designware-plat.c to keep it from claiming devices?

We could simply bail probe if "snps,dw-pcie" is not the most specific
compatible. I don't think we have a ready made helper for that, but it
would be just checking "compatible" string index 0 against
"snps,dw-pcie".

> > Sagar is wondering why this hang is only seen/reported for Tegra and
> > could this happen to other platforms? I think that is potentially could.
>
> Maybe pcie-designware-plat.c works on other platforms, i.e., they
> don't require the hardware init?

At least upstream, I don't think there are any. But a lot of platforms
have 'snps,dw-pcie' as a fallback.

Rob

Re: Query regarding the use of pcie-designware-plat.c file

[Jon Hunter]

On 15/06/2021 20:42, Bjorn Helgaas wrote:
> On Wed, Jun 09, 2021 at 05:54:38PM +0100, Jon Hunter wrote:
>>
>> On 09/06/2021 17:30, Bjorn Helgaas wrote:
>>> On Wed, Jun 09, 2021 at 12:52:37AM +0530, Vidya Sagar wrote:
>>>> Hi,
>>>> I would like to know what is the use of pcie-designware-plat.c file. This
>>>> looks like a skeleton file and can't really work with any specific hardware
>>>> as such.
>>>> Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT is
>>>> enabled in a system where a Synopsys DesignWare IP based PCIe controller is
>>>> present and its configuration is enabled (Ex:- Tegra194 system with
>>>> CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen that the probe of
>>>> pcie-designware-plat.c called first (because all DWC based PCIe controller
>>>> nodes have "snps,dw-pcie" compatibility string) and can crash the system.
>>>
>>> What's the crash?  If a device claims to be compatible with
>>> "snps,dw-pcie" and pcie-designware-plat.c claims to know how to
>>> operate "snps,dw-pcie" devices, it seems like something is wrong.
>>>
>>> "snps,dw-pcie" is a generic device type, so pcie-designware-plat.c
>>> might not know how to operate device-specific details of some of those
>>> devices, but basic functionality should work and it certainly
>>> shouldn't crash.
>>
>> It is not really a crash but a hang when trying to access the hardware
>> before it has been properly initialised.
> 
> This doesn't really answer my question.
> 
> If the hardware claims to be compatible with "snps,dw-pcie" and a
> driver knows how to operate "snps,dw-pcie" devices, it should work.
> 
> If the hardware requires initialization that is not part of the
> "snps,dw-pcie" programming model, it should not claim to be compatible
> with "snps,dw-pcie".  Or, if pcie-designware-plat.c is missing some
> init that *is* part of the programming model, maybe it needs to be
> enhanced?

Right and this is exactly why I removed the compatible string
"snps,dw-pcie" for Tegra194 because it is clear that this does not work
and hence not compatible.

Sagar is purely trying to understand if this is case of other devices as
well or just Tegra194.

Jon

-- 
nvpublic

Re: Query regarding the use of pcie-designware-plat.c file

[Vidya Sagar]

Given Rob's latest reply, I think Jon's patch addressed the issue in the 
correct way. And IIUC, I think this is how it should be for all Synopsys 
DesignWare implementations unless the implementation really claims that 
it can even work with the generic pcie-designware-plat.c file, otherwise 
it shouldn't contain 'snps,dw-pcie' compatibility string.
IMHO, the addition of 'snps,dw-pcie' along with the respective platform 
specific compatibility string should be strictly restricted.
I think this thread is considered to be concluded at this point.

Thanks,
Vidya Sagar

On 6/16/2021 1:24 PM, Jon Hunter wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 15/06/2021 20:42, Bjorn Helgaas wrote:
>> On Wed, Jun 09, 2021 at 05:54:38PM +0100, Jon Hunter wrote:
>>>
>>> On 09/06/2021 17:30, Bjorn Helgaas wrote:
>>>> On Wed, Jun 09, 2021 at 12:52:37AM +0530, Vidya Sagar wrote:
>>>>> Hi,
>>>>> I would like to know what is the use of pcie-designware-plat.c file. This
>>>>> looks like a skeleton file and can't really work with any specific hardware
>>>>> as such.
>>>>> Some context for this mail thread is, if the config CONFIG_PCIE_DW_PLAT is
>>>>> enabled in a system where a Synopsys DesignWare IP based PCIe controller is
>>>>> present and its configuration is enabled (Ex:- Tegra194 system with
>>>>> CONFIG_PCIE_TEGRA194_HOST enabled), then, it can so happen that the probe of
>>>>> pcie-designware-plat.c called first (because all DWC based PCIe controller
>>>>> nodes have "snps,dw-pcie" compatibility string) and can crash the system.
>>>>
>>>> What's the crash?  If a device claims to be compatible with
>>>> "snps,dw-pcie" and pcie-designware-plat.c claims to know how to
>>>> operate "snps,dw-pcie" devices, it seems like something is wrong.
>>>>
>>>> "snps,dw-pcie" is a generic device type, so pcie-designware-plat.c
>>>> might not know how to operate device-specific details of some of those
>>>> devices, but basic functionality should work and it certainly
>>>> shouldn't crash.
>>>
>>> It is not really a crash but a hang when trying to access the hardware
>>> before it has been properly initialised.
>>
>> This doesn't really answer my question.
>>
>> If the hardware claims to be compatible with "snps,dw-pcie" and a
>> driver knows how to operate "snps,dw-pcie" devices, it should work.
>>
>> If the hardware requires initialization that is not part of the
>> "snps,dw-pcie" programming model, it should not claim to be compatible
>> with "snps,dw-pcie".  Or, if pcie-designware-plat.c is missing some
>> init that *is* part of the programming model, maybe it needs to be
>> enhanced?
> 
> Right and this is exactly why I removed the compatible string
> "snps,dw-pcie" for Tegra194 because it is clear that this does not work
> and hence not compatible.
> 
> Sagar is purely trying to understand if this is case of other devices as
> well or just Tegra194.
> 
> Jon
> 
> --
> nvpublic
>