2020-12-11 14:50:12

by Athani Nadeem Ladkhan

[permalink] [raw]
Subject: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around Gen2 training defect.

Cadence controller will not initiate autonomous speed change if strapped as
Gen2. The Retrain Link bit is set as quirk to enable this speed change.
Adding a quirk flag based on a new compatible string.

Signed-off-by: Nadeem Athani <[email protected]>
---
Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
index 293b8ec318bc..204d78f9efe3 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
@@ -15,7 +15,9 @@ allOf:

properties:
compatible:
- const: cdns,cdns-pcie-host
+ enum:
+ - cdns,cdns-pcie-host
+ - cdns,cdns-pcie-host-quirk-retrain

reg:
maxItems: 2
--
2.15.0


2020-12-11 18:54:13

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around Gen2 training defect.

On Fri, Dec 11, 2020 at 9:03 AM Nadeem Athani <[email protected]> wrote:
>
> Cadence controller will not initiate autonomous speed change if strapped as
> Gen2. The Retrain Link bit is set as quirk to enable this speed change.
> Adding a quirk flag based on a new compatible string.
>
> Signed-off-by: Nadeem Athani <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> index 293b8ec318bc..204d78f9efe3 100644
> --- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> @@ -15,7 +15,9 @@ allOf:
>
> properties:
> compatible:
> - const: cdns,cdns-pcie-host
> + enum:
> + - cdns,cdns-pcie-host
> + - cdns,cdns-pcie-host-quirk-retrain

So, we'll just keep adding quirk strings on to the compatible? I don't
think so. Compatible strings should map to a specific
implementation/platform and quirks can then be implied from them. This
is the only way we can implement quirks in the OS without firmware
(DT) changes.

Rob

2020-12-13 14:52:18

by Athani Nadeem Ladkhan

[permalink] [raw]
Subject: RE: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around Gen2 training defect.

Hi Rob / Kishon,

> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: Friday, December 11, 2020 10:32 PM
> To: Athani Nadeem Ladkhan <[email protected]>
> Cc: Tom Joseph <[email protected]>; Lorenzo Pieralisi
> <[email protected]>; Bjorn Helgaas <[email protected]>; PCI
> <[email protected]>; [email protected]; Kishon Vijay
> Abraham I <[email protected]>; [email protected]; Milind Parab
> <[email protected]>; Swapnil Kashinath Jakhade
> <[email protected]>; Parshuram Raju Thombare
> <[email protected]>
> Subject: Re: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around
> Gen2 training defect.
>
> EXTERNAL MAIL
>
>
> On Fri, Dec 11, 2020 at 9:03 AM Nadeem Athani <[email protected]>
> wrote:
> >
> > Cadence controller will not initiate autonomous speed change if
> > strapped as Gen2. The Retrain Link bit is set as quirk to enable this speed
> change.
> > Adding a quirk flag based on a new compatible string.
> >
> > Signed-off-by: Nadeem Athani <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml | 4
> > +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> > b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> > index 293b8ec318bc..204d78f9efe3 100644
> > --- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> > +++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> > @@ -15,7 +15,9 @@ allOf:
> >
> > properties:
> > compatible:
> > - const: cdns,cdns-pcie-host
> > + enum:
> > + - cdns,cdns-pcie-host
> > + - cdns,cdns-pcie-host-quirk-retrain
>
> So, we'll just keep adding quirk strings on to the compatible? I don't think so.
> Compatible strings should map to a specific implementation/platform and
> quirks can then be implied from them. This is the only way we can implement
> quirks in the OS without firmware
> (DT) changes.
Ok, I will change the compatible string to " ti,j721e-pcie-host" in place of " cdns,cdns-pcie-host-quirk-retrain" .
@Kishon Vijay Abraham I: Is this fine? Or will you suggest an appropriate name?

Nadeem
>
> Rob

2020-12-14 09:47:59

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around Gen2 training defect.

Hi Nadeem,

On 12/12/20 12:37 pm, Athani Nadeem Ladkhan wrote:
> Hi Rob / Kishon,
>
>> -----Original Message-----
>> From: Rob Herring <[email protected]>
>> Sent: Friday, December 11, 2020 10:32 PM
>> To: Athani Nadeem Ladkhan <[email protected]>
>> Cc: Tom Joseph <[email protected]>; Lorenzo Pieralisi
>> <[email protected]>; Bjorn Helgaas <[email protected]>; PCI
>> <[email protected]>; [email protected]; Kishon Vijay
>> Abraham I <[email protected]>; [email protected]; Milind Parab
>> <[email protected]>; Swapnil Kashinath Jakhade
>> <[email protected]>; Parshuram Raju Thombare
>> <[email protected]>
>> Subject: Re: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around
>> Gen2 training defect.
>>
>> EXTERNAL MAIL
>>
>>
>> On Fri, Dec 11, 2020 at 9:03 AM Nadeem Athani <[email protected]>
>> wrote:
>>>
>>> Cadence controller will not initiate autonomous speed change if
>>> strapped as Gen2. The Retrain Link bit is set as quirk to enable this speed
>> change.
>>> Adding a quirk flag based on a new compatible string.
>>>
>>> Signed-off-by: Nadeem Athani <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml | 4
>>> +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>> b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>> index 293b8ec318bc..204d78f9efe3 100644
>>> --- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>> @@ -15,7 +15,9 @@ allOf:
>>>
>>> properties:
>>> compatible:
>>> - const: cdns,cdns-pcie-host
>>> + enum:
>>> + - cdns,cdns-pcie-host
>>> + - cdns,cdns-pcie-host-quirk-retrain
>>
>> So, we'll just keep adding quirk strings on to the compatible? I don't think so.
>> Compatible strings should map to a specific implementation/platform and
>> quirks can then be implied from them. This is the only way we can implement
>> quirks in the OS without firmware
>> (DT) changes.
> Ok, I will change the compatible string to " ti,j721e-pcie-host" in place of " cdns,cdns-pcie-host-quirk-retrain" .
> @Kishon Vijay Abraham I: Is this fine? Or will you suggest an appropriate name?

IMHO it should be something like "cdns,cdns-pcie-host-vX", since the
quirk itself is not specific to TI platform rather Cadence IP version.

Thank You,
Kishon

2020-12-14 15:31:43

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around Gen2 training defect.

On Sun, Dec 13, 2020 at 10:21 PM Kishon Vijay Abraham I <[email protected]> wrote:
>
> Hi Nadeem,
>
> On 12/12/20 12:37 pm, Athani Nadeem Ladkhan wrote:
> > Hi Rob / Kishon,
> >
> >> -----Original Message-----
> >> From: Rob Herring <[email protected]>
> >> Sent: Friday, December 11, 2020 10:32 PM
> >> To: Athani Nadeem Ladkhan <[email protected]>
> >> Cc: Tom Joseph <[email protected]>; Lorenzo Pieralisi
> >> <[email protected]>; Bjorn Helgaas <[email protected]>; PCI
> >> <[email protected]>; [email protected]; Kishon Vijay
> >> Abraham I <[email protected]>; [email protected]; Milind Parab
> >> <[email protected]>; Swapnil Kashinath Jakhade
> >> <[email protected]>; Parshuram Raju Thombare
> >> <[email protected]>
> >> Subject: Re: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around
> >> Gen2 training defect.
> >>
> >> EXTERNAL MAIL
> >>
> >>
> >> On Fri, Dec 11, 2020 at 9:03 AM Nadeem Athani <[email protected]>
> >> wrote:
> >>>
> >>> Cadence controller will not initiate autonomous speed change if
> >>> strapped as Gen2. The Retrain Link bit is set as quirk to enable this speed
> >> change.
> >>> Adding a quirk flag based on a new compatible string.
> >>>
> >>> Signed-off-by: Nadeem Athani <[email protected]>
> >>> ---
> >>> Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml | 4
> >>> +++-
> >>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> >>> b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> >>> index 293b8ec318bc..204d78f9efe3 100644
> >>> --- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
> >>> @@ -15,7 +15,9 @@ allOf:
> >>>
> >>> properties:
> >>> compatible:
> >>> - const: cdns,cdns-pcie-host
> >>> + enum:
> >>> + - cdns,cdns-pcie-host
> >>> + - cdns,cdns-pcie-host-quirk-retrain
> >>
> >> So, we'll just keep adding quirk strings on to the compatible? I don't think so.
> >> Compatible strings should map to a specific implementation/platform and
> >> quirks can then be implied from them. This is the only way we can implement
> >> quirks in the OS without firmware
> >> (DT) changes.
> > Ok, I will change the compatible string to " ti,j721e-pcie-host" in place of " cdns,cdns-pcie-host-quirk-retrain" .
> > @Kishon Vijay Abraham I: Is this fine? Or will you suggest an appropriate name?
>
> IMHO it should be something like "cdns,cdns-pcie-host-vX", since the
> quirk itself is not specific to TI platform rather Cadence IP version.

That's fine if Cadence has a need for it, but for TI platforms use the
TI compatible string. ECOs on version X IP without changing X is not
uncommon.

Rob

2020-12-15 07:15:23

by Kishon Vijay Abraham I

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around Gen2 training defect.

Hi,

On 14/12/20 8:35 pm, Rob Herring wrote:
> On Sun, Dec 13, 2020 at 10:21 PM Kishon Vijay Abraham I <[email protected]> wrote:
>>
>> Hi Nadeem,
>>
>> On 12/12/20 12:37 pm, Athani Nadeem Ladkhan wrote:
>>> Hi Rob / Kishon,
>>>
>>>> -----Original Message-----
>>>> From: Rob Herring <[email protected]>
>>>> Sent: Friday, December 11, 2020 10:32 PM
>>>> To: Athani Nadeem Ladkhan <[email protected]>
>>>> Cc: Tom Joseph <[email protected]>; Lorenzo Pieralisi
>>>> <[email protected]>; Bjorn Helgaas <[email protected]>; PCI
>>>> <[email protected]>; [email protected]; Kishon Vijay
>>>> Abraham I <[email protected]>; [email protected]; Milind Parab
>>>> <[email protected]>; Swapnil Kashinath Jakhade
>>>> <[email protected]>; Parshuram Raju Thombare
>>>> <[email protected]>
>>>> Subject: Re: [PATCH v4 1/2] dt-bindings: pci: Retrain Link to work around
>>>> Gen2 training defect.
>>>>
>>>> EXTERNAL MAIL
>>>>
>>>>
>>>> On Fri, Dec 11, 2020 at 9:03 AM Nadeem Athani <[email protected]>
>>>> wrote:
>>>>>
>>>>> Cadence controller will not initiate autonomous speed change if
>>>>> strapped as Gen2. The Retrain Link bit is set as quirk to enable this speed
>>>> change.
>>>>> Adding a quirk flag based on a new compatible string.
>>>>>
>>>>> Signed-off-by: Nadeem Athani <[email protected]>
>>>>> ---
>>>>> Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml | 4
>>>>> +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>>>> b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>>>> index 293b8ec318bc..204d78f9efe3 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>>>> +++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>>>> @@ -15,7 +15,9 @@ allOf:
>>>>>
>>>>> properties:
>>>>> compatible:
>>>>> - const: cdns,cdns-pcie-host
>>>>> + enum:
>>>>> + - cdns,cdns-pcie-host
>>>>> + - cdns,cdns-pcie-host-quirk-retrain
>>>>
>>>> So, we'll just keep adding quirk strings on to the compatible? I don't think so.
>>>> Compatible strings should map to a specific implementation/platform and
>>>> quirks can then be implied from them. This is the only way we can implement
>>>> quirks in the OS without firmware
>>>> (DT) changes.
>>> Ok, I will change the compatible string to " ti,j721e-pcie-host" in place of " cdns,cdns-pcie-host-quirk-retrain" .
>>> @Kishon Vijay Abraham I: Is this fine? Or will you suggest an appropriate name?
>>
>> IMHO it should be something like "cdns,cdns-pcie-host-vX", since the
>> quirk itself is not specific to TI platform rather Cadence IP version.
>
> That's fine if Cadence has a need for it, but for TI platforms use the
> TI compatible string. ECOs on version X IP without changing X is not
> uncommon.

Okay. I re-worked the patch to be applicable only to TI's J721E SoC
http://lore.kernel.org/r/[email protected]

Thank You,
Kishon