diff mbox series

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

Message ID 20201211144236.3825-2-nadeem@cadence.com
State New
Headers show
Series PCI: cadence: Retrain Link to work around Gen2 | expand

Commit Message

Nadeem Athani Dec. 11, 2020, 2:42 p.m. UTC
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 <nadeem@cadence.com>
---
 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Rob Herring Dec. 11, 2020, 5:02 p.m. UTC | #1
On Fri, Dec 11, 2020 at 9:03 AM Nadeem Athani <nadeem@cadence.com> 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 <nadeem@cadence.com>
> ---
>  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
Nadeem Athani Dec. 12, 2020, 7:07 a.m. UTC | #2
Hi Rob / Kishon,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, December 11, 2020 10:32 PM
> To: Athani Nadeem Ladkhan <nadeem@cadence.com>
> Cc: Tom Joseph <tjoseph@cadence.com>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; PCI
> <linux-pci@vger.kernel.org>; linux-kernel@vger.kernel.org; Kishon Vijay
> Abraham I <kishon@ti.com>; devicetree@vger.kernel.org; Milind Parab
> <mparab@cadence.com>; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>; Parshuram Raju Thombare
> <pthombar@cadence.com>
> 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 <nadeem@cadence.com>
> 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 <nadeem@cadence.com>
> > ---
> >  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
Kishon Vijay Abraham I Dec. 14, 2020, 4:20 a.m. UTC | #3
Hi Nadeem,

On 12/12/20 12:37 pm, Athani Nadeem Ladkhan wrote:
> Hi Rob / Kishon,
> 
>> -----Original Message-----
>> From: Rob Herring <robh@kernel.org>
>> Sent: Friday, December 11, 2020 10:32 PM
>> To: Athani Nadeem Ladkhan <nadeem@cadence.com>
>> Cc: Tom Joseph <tjoseph@cadence.com>; Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; PCI
>> <linux-pci@vger.kernel.org>; linux-kernel@vger.kernel.org; Kishon Vijay
>> Abraham I <kishon@ti.com>; devicetree@vger.kernel.org; Milind Parab
>> <mparab@cadence.com>; Swapnil Kashinath Jakhade
>> <sjakhade@cadence.com>; Parshuram Raju Thombare
>> <pthombar@cadence.com>
>> 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 <nadeem@cadence.com>
>> 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 <nadeem@cadence.com>
>>> ---
>>>  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
Rob Herring Dec. 14, 2020, 3:05 p.m. UTC | #4
On Sun, Dec 13, 2020 at 10:21 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi Nadeem,
>
> On 12/12/20 12:37 pm, Athani Nadeem Ladkhan wrote:
> > Hi Rob / Kishon,
> >
> >> -----Original Message-----
> >> From: Rob Herring <robh@kernel.org>
> >> Sent: Friday, December 11, 2020 10:32 PM
> >> To: Athani Nadeem Ladkhan <nadeem@cadence.com>
> >> Cc: Tom Joseph <tjoseph@cadence.com>; Lorenzo Pieralisi
> >> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; PCI
> >> <linux-pci@vger.kernel.org>; linux-kernel@vger.kernel.org; Kishon Vijay
> >> Abraham I <kishon@ti.com>; devicetree@vger.kernel.org; Milind Parab
> >> <mparab@cadence.com>; Swapnil Kashinath Jakhade
> >> <sjakhade@cadence.com>; Parshuram Raju Thombare
> >> <pthombar@cadence.com>
> >> 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 <nadeem@cadence.com>
> >> 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 <nadeem@cadence.com>
> >>> ---
> >>>  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
Kishon Vijay Abraham I Dec. 15, 2020, 7:08 a.m. UTC | #5
Hi,

On 14/12/20 8:35 pm, Rob Herring wrote:
> On Sun, Dec 13, 2020 at 10:21 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Nadeem,
>>
>> On 12/12/20 12:37 pm, Athani Nadeem Ladkhan wrote:
>>> Hi Rob / Kishon,
>>>
>>>> -----Original Message-----
>>>> From: Rob Herring <robh@kernel.org>
>>>> Sent: Friday, December 11, 2020 10:32 PM
>>>> To: Athani Nadeem Ladkhan <nadeem@cadence.com>
>>>> Cc: Tom Joseph <tjoseph@cadence.com>; Lorenzo Pieralisi
>>>> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; PCI
>>>> <linux-pci@vger.kernel.org>; linux-kernel@vger.kernel.org; Kishon Vijay
>>>> Abraham I <kishon@ti.com>; devicetree@vger.kernel.org; Milind Parab
>>>> <mparab@cadence.com>; Swapnil Kashinath Jakhade
>>>> <sjakhade@cadence.com>; Parshuram Raju Thombare
>>>> <pthombar@cadence.com>
>>>> 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 <nadeem@cadence.com>
>>>> 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 <nadeem@cadence.com>
>>>>> ---
>>>>>  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/20201215070009.27937-1-kishon@ti.com

Thank You,
Kishon
diff mbox series

Patch

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