[PATCH 0/1] RFC: Need feedback on PCI dt binding property

[PATCH 0/1] RFC: Need feedback on PCI dt binding property

[Jim Quinlan]

We'd like to get early feedback on a dt binding property.  We cannot
submit the code with it as there is a backlog of commits that must
be submitted first.  We are just looking for some initial comments.

Jim Quinlan (1):
  RFC: dt bindings: Add property "brcm,gen3-eq-presets"

 .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.43.0

[PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Jim Quinlan]

Support configuration of the GEN3 preset equalization settings, aka the
Lane Equalization Control Register(s) of the Secondary PCI Express
Extended Capability.  These registers are of type HwInit/RsvdP and
typically set by FW.  In our case they are set by our RC host bridge
driver using internal registers.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 0925c520195a..f965ad57f32f 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -104,6 +104,18 @@ properties:
     minItems: 1
     maxItems: 3
 
+  brcm,gen3-eq-presets:
+    description: |
+      A u16 array giving the GEN3 equilization presets, one for each lane.
+      These values are destined for the 16bit registers known as the
+      Lane Equalization Control Register(s) of the Secondary PCI Express
+      Extended Capability.  In the array, lane 0 is first term, lane 1 next,
+      etc. The contents of the entries reflect what is necessary for
+      the current board and SoC, and the details of each preset are
+      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
+
+    $ref: /schemas/types.yaml#/definitions/uint16-array
+
 required:
   - compatible
   - reg
-- 
2.43.0

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Bjorn Helgaas]

On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
> Support configuration of the GEN3 preset equalization settings, aka the
> Lane Equalization Control Register(s) of the Secondary PCI Express
> Extended Capability.  These registers are of type HwInit/RsvdP and
> typically set by FW.  In our case they are set by our RC host bridge
> driver using internal registers.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 0925c520195a..f965ad57f32f 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -104,6 +104,18 @@ properties:
>      minItems: 1
>      maxItems: 3
>  
> +  brcm,gen3-eq-presets:
> +    description: |
> +      A u16 array giving the GEN3 equilization presets, one for each lane.
> +      These values are destined for the 16bit registers known as the
> +      Lane Equalization Control Register(s) of the Secondary PCI Express
> +      Extended Capability.  In the array, lane 0 is first term, lane 1 next,
> +      etc. The contents of the entries reflect what is necessary for
> +      the current board and SoC, and the details of each preset are
> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.

s/equilization/equalization/

The spec citation ("PCI base spec r3.0") isn't quite right since
Conventional PCI doesn't have lanes.  These registers *are* defined in
PCIe r3.0, sec 7.27.4, but that's 14 years old.  It would be more
helpful to use a current spec version like PCIe r6.2, sec 7.7.3.4.

Since there's nothing about these registers that is brcm-specific
(other than the fact that they are typically set by firmware on
non-brcm platforms), it would be nice if we could give it a non-brcm
name.

Similarly, I think it would be nice to drop "gen3" from the name (and
the description).  The registers *were* added in PCIe r3.0, which also
added the 8 GT/s rate, and the description in PCIe r6.2, sec 7.7.3.4
does mention 8.0 GT/s specifically, but sec 4.2.4 says equalization
applies to "8.0 GT/s and higher data rates," so it's definitely not
limited to gen3.

Bjorn

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Rob Herring]

On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
> Support configuration of the GEN3 preset equalization settings, aka the
> Lane Equalization Control Register(s) of the Secondary PCI Express
> Extended Capability.  These registers are of type HwInit/RsvdP and
> typically set by FW.  In our case they are set by our RC host bridge
> driver using internal registers.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 0925c520195a..f965ad57f32f 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -104,6 +104,18 @@ properties:
>      minItems: 1
>      maxItems: 3
>  
> +  brcm,gen3-eq-presets:
> +    description: |
> +      A u16 array giving the GEN3 equilization presets, one for each lane.
> +      These values are destined for the 16bit registers known as the
> +      Lane Equalization Control Register(s) of the Secondary PCI Express
> +      Extended Capability.  In the array, lane 0 is first term, lane 1 next,
> +      etc. The contents of the entries reflect what is necessary for
> +      the current board and SoC, and the details of each preset are
> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.

If these are defined by the PCIe spec, then why is it Broadcom specific 
property?

> +
> +    $ref: /schemas/types.yaml#/definitions/uint16-array

minItems: 1
maxItems: 16

Last I saw, you can only have up to 16 lanes.

Rob

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Krishna Chaitanya Chundru]

On 10/22/2024 12:33 AM, Rob Herring wrote:
> On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
>> Support configuration of the GEN3 preset equalization settings, aka the
>> Lane Equalization Control Register(s) of the Secondary PCI Express
>> Extended Capability.  These registers are of type HwInit/RsvdP and
>> typically set by FW.  In our case they are set by our RC host bridge
>> driver using internal registers.
>>
>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>> ---
>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> index 0925c520195a..f965ad57f32f 100644
>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> @@ -104,6 +104,18 @@ properties:
>>       minItems: 1
>>       maxItems: 3
>>   
>> +  brcm,gen3-eq-presets:
>> +    description: |
>> +      A u16 array giving the GEN3 equilization presets, one for each lane.
>> +      These values are destined for the 16bit registers known as the
>> +      Lane Equalization Control Register(s) of the Secondary PCI Express
>> +      Extended Capability.  In the array, lane 0 is first term, lane 1 next,
>> +      etc. The contents of the entries reflect what is necessary for
>> +      the current board and SoC, and the details of each preset are
>> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
> 
> If these are defined by the PCIe spec, then why is it Broadcom specific
> property?
> 
Hi Rob,

qcom pcie driver also needs to program these presets as you suggested
this can go to common pci bridge binding.

from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
through P10) and where as data rates of 64.0 GT/s use different class of
presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
have optional preset hints (Table 4-24).

And there is possibility that for each data rate we may require
different preset configuration.

Can we have a dt binding for each data rate of 16 byte array.
like gen3-eq-preset array, gen4-eq-preset array etc.

- Krishna Chaitanya
>> +
>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
> 
> minItems: 1
> maxItems: 16
> 
> Last I saw, you can only have up to 16 lanes.
> 
> Rob
> 

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[James Quinlan]

On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
>
>
> On 10/22/2024 12:33 AM, Rob Herring wrote:
>> On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
>>> Support configuration of the GEN3 preset equalization settings, aka the
>>> Lane Equalization Control Register(s) of the Secondary PCI Express
>>> Extended Capability.  These registers are of type HwInit/RsvdP and
>>> typically set by FW.  In our case they are set by our RC host bridge
>>> driver using internal registers.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 
>>> ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
>>> b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 0925c520195a..f965ad57f32f 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> @@ -104,6 +104,18 @@ properties:
>>>       minItems: 1
>>>       maxItems: 3
>>>   +  brcm,gen3-eq-presets:
>>> +    description: |
>>> +      A u16 array giving the GEN3 equilization presets, one for 
>>> each lane.
>>> +      These values are destined for the 16bit registers known as the
>>> +      Lane Equalization Control Register(s) of the Secondary PCI 
>>> Express
>>> +      Extended Capability.  In the array, lane 0 is first term, 
>>> lane 1 next,
>>> +      etc. The contents of the entries reflect what is necessary for
>>> +      the current board and SoC, and the details of each preset are
>>> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
>>
>> If these are defined by the PCIe spec, then why is it Broadcom specific
>> property?
Yes, I will remove the "brcm," prefix.
>>
> Hi Rob,
>
> qcom pcie driver also needs to program these presets as you suggested
> this can go to common pci bridge binding.
>
> from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
> of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
> through P10) and where as data rates of 64.0 GT/s use different class of
> presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
> have optional preset hints (Table 4-24).
>
> And there is possibility that for each data rate we may require
> different preset configuration.
>
> Can we have a dt binding for each data rate of 16 byte array.
> like gen3-eq-preset array, gen4-eq-preset array etc.

Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.

Keep in mind that this is an RFC; I have a backlog of commit submissions 
before I can submit the code that uses this DT property.  If you 
(Krishna) want to submit something now I'd be quite happy to go with 
that.  I don't believe it is acceptable to submit a bindings commit w/o 
code that uses it (if I'm incorrect I'll be glad to do a V2).

Regards,

Jim Quinlan
Broadcom STB/CM

>
> - Krishna Chaitanya
>>> +
>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
>>
>> minItems: 1
>> maxItems: 16
>>
>> Last I saw, you can only have up to 16 lanes.
>>
>> Rob
>>

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Krishna Chaitanya Chundru]

On 10/29/2024 12:21 AM, James Quinlan wrote:
> On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 10/22/2024 12:33 AM, Rob Herring wrote:
>>> On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
>>>> Support configuration of the GEN3 preset equalization settings, aka the
>>>> Lane Equalization Control Register(s) of the Secondary PCI Express
>>>> Extended Capability.  These registers are of type HwInit/RsvdP and
>>>> typically set by FW.  In our case they are set by our RC host bridge
>>>> driver using internal registers.
>>>>
>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>> ---
>>>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 
>>>> ++++++++++++
>>>>   1 file changed, 12 insertions(+)
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
>>>> b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>> index 0925c520195a..f965ad57f32f 100644
>>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>> @@ -104,6 +104,18 @@ properties:
>>>>       minItems: 1
>>>>       maxItems: 3
>>>>   +  brcm,gen3-eq-presets:
>>>> +    description: |
>>>> +      A u16 array giving the GEN3 equilization presets, one for 
>>>> each lane.
>>>> +      These values are destined for the 16bit registers known as the
>>>> +      Lane Equalization Control Register(s) of the Secondary PCI 
>>>> Express
>>>> +      Extended Capability.  In the array, lane 0 is first term, 
>>>> lane 1 next,
>>>> +      etc. The contents of the entries reflect what is necessary for
>>>> +      the current board and SoC, and the details of each preset are
>>>> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
>>>
>>> If these are defined by the PCIe spec, then why is it Broadcom specific
>>> property?
> Yes, I will remove the "brcm," prefix.
>>>
>> Hi Rob,
>>
>> qcom pcie driver also needs to program these presets as you suggested
>> this can go to common pci bridge binding.
>>
>> from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
>> of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
>> through P10) and where as data rates of 64.0 GT/s use different class of
>> presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
>> have optional preset hints (Table 4-24).
>>
>> And there is possibility that for each data rate we may require
>> different preset configuration.
>>
>> Can we have a dt binding for each data rate of 16 byte array.
>> like gen3-eq-preset array, gen4-eq-preset array etc.
> 
> Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.
> 
> Keep in mind that this is an RFC; I have a backlog of commit submissions 
> before I can submit the code that uses this DT property.  If you 
> (Krishna) want to submit something now I'd be quite happy to go with 
> that.  I don't believe it is acceptable to submit a bindings commit w/o 
> code that uses it (if I'm incorrect I'll be glad to do a V2).
> 
Hi Jim,

I submitted a pull request for this. if you have any other suggestions
or if we need to have any other details we can update this pull request.
https://github.com/devicetree-org/dt-schema/pull/146

- Krishna Chaitanya.
> Regards,
> 
> Jim Quinlan
> Broadcom STB/CM
> 
>>
>> - Krishna Chaitanya
>>>> +
>>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
>>>
>>> minItems: 1
>>> maxItems: 16
>>>
>>> Last I saw, you can only have up to 16 lanes.
>>>
>>> Rob
>>>
> 

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Jim Quinlan]

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

On Tue, Oct 29, 2024 at 1:17 AM Krishna Chaitanya Chundru
<quic_krichai@quicinc.com> wrote:
>
>
>
> On 10/29/2024 12:21 AM, James Quinlan wrote:
> > On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
> >>
> >>
> >> On 10/22/2024 12:33 AM, Rob Herring wrote:
> >>> On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
> >>>> Support configuration of the GEN3 preset equalization settings, aka the
> >>>> Lane Equalization Control Register(s) of the Secondary PCI Express
> >>>> Extended Capability.  These registers are of type HwInit/RsvdP and
> >>>> typically set by FW.  In our case they are set by our RC host bridge
> >>>> driver using internal registers.
> >>>>
> >>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>>> ---
> >>>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12
> >>>> ++++++++++++
> >>>>   1 file changed, 12 insertions(+)
> >>>>
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>>> b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>>> index 0925c520195a..f965ad57f32f 100644
> >>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>>> @@ -104,6 +104,18 @@ properties:
> >>>>       minItems: 1
> >>>>       maxItems: 3
> >>>>   +  brcm,gen3-eq-presets:
> >>>> +    description: |
> >>>> +      A u16 array giving the GEN3 equilization presets, one for
> >>>> each lane.
> >>>> +      These values are destined for the 16bit registers known as the
> >>>> +      Lane Equalization Control Register(s) of the Secondary PCI
> >>>> Express
> >>>> +      Extended Capability.  In the array, lane 0 is first term,
> >>>> lane 1 next,
> >>>> +      etc. The contents of the entries reflect what is necessary for
> >>>> +      the current board and SoC, and the details of each preset are
> >>>> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
> >>>
> >>> If these are defined by the PCIe spec, then why is it Broadcom specific
> >>> property?
> > Yes, I will remove the "brcm," prefix.
> >>>
> >> Hi Rob,
> >>
> >> qcom pcie driver also needs to program these presets as you suggested
> >> this can go to common pci bridge binding.
> >>
> >> from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
> >> of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
> >> through P10) and where as data rates of 64.0 GT/s use different class of
> >> presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
> >> have optional preset hints (Table 4-24).
> >>
> >> And there is possibility that for each data rate we may require
> >> different preset configuration.
> >>
> >> Can we have a dt binding for each data rate of 16 byte array.
> >> like gen3-eq-preset array, gen4-eq-preset array etc.
> >
> > Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.
> >
> > Keep in mind that this is an RFC; I have a backlog of commit submissions
> > before I can submit the code that uses this DT property.  If you
> > (Krishna) want to submit something now I'd be quite happy to go with
> > that.  I don't believe it is acceptable to submit a bindings commit w/o
> > code that uses it (if I'm incorrect I'll be glad to do a V2).
> >
> Hi Jim,
>
> I submitted a pull request for this. if you have any other suggestions
> or if we need to have any other details we can update this pull request.
> https://github.com/devicetree-org/dt-schema/pull/146


Hi Krishna,
Thanks for doing this.   However, why a u8 array?  The registers are
defined as u16 so it would be more natural to use a u16 array, each
entry giving
all of the data for a single lane.  In our implementation we read a
u16 and we write it to the register -- what advantage is there by
using a u8 array?

Also if there are 16 lanes, you will need 32 maxItems, correct?

Regards,
Jim Quinlan
Broadcom STB/CM
>
>
>
> - Krishna Chaitanya.
> > Regards,
> >
> > Jim Quinlan
> > Broadcom STB/CM
> >
> >>
> >> - Krishna Chaitanya
> >>>> +
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
> >>>
> >>> minItems: 1
> >>> maxItems: 16
> >>>
> >>> Last I saw, you can only have up to 16 lanes.
> >>>
> >>> Rob
> >>>
> >

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Bjorn Helgaas]

On Tue, Oct 29, 2024 at 10:22:36AM -0400, Jim Quinlan wrote:
> On Tue, Oct 29, 2024 at 1:17 AM Krishna Chaitanya Chundru
> <quic_krichai@quicinc.com> wrote:
> > On 10/29/2024 12:21 AM, James Quinlan wrote:
> > > On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
> > >> On 10/22/2024 12:33 AM, Rob Herring wrote:
> > >>> On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
> > >>>> Support configuration of the GEN3 preset equalization settings, aka the
> > >>>> Lane Equalization Control Register(s) of the Secondary PCI Express
> > >>>> Extended Capability.  These registers are of type HwInit/RsvdP and
> > >>>> typically set by FW.  In our case they are set by our RC host bridge
> > >>>> driver using internal registers.
> > >>>>
> > >>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > >>>> ---
> > >>>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12
> > >>>> ++++++++++++
> > >>>>   1 file changed, 12 insertions(+)
> > >>>>
> > >>>> diff --git
> > >>>> a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > >>>> b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > >>>> index 0925c520195a..f965ad57f32f 100644
> > >>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > >>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > >>>> @@ -104,6 +104,18 @@ properties:
> > >>>>       minItems: 1
> > >>>>       maxItems: 3
> > >>>>   +  brcm,gen3-eq-presets:
> > >>>> +    description: |
> > >>>> +      A u16 array giving the GEN3 equilization presets, one for
> > >>>> each lane.
> > >>>> +      These values are destined for the 16bit registers known as the
> > >>>> +      Lane Equalization Control Register(s) of the Secondary PCI
> > >>>> Express
> > >>>> +      Extended Capability.  In the array, lane 0 is first term,
> > >>>> lane 1 next,
> > >>>> +      etc. The contents of the entries reflect what is necessary for
> > >>>> +      the current board and SoC, and the details of each preset are
> > >>>> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
> > >>>
> > >>> If these are defined by the PCIe spec, then why is it Broadcom specific
> > >>> property?
> > > Yes, I will remove the "brcm," prefix.
> > >>>
> > >> Hi Rob,
> > >>
> > >> qcom pcie driver also needs to program these presets as you suggested
> > >> this can go to common pci bridge binding.
> > >>
> > >> from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
> > >> of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
> > >> through P10) and where as data rates of 64.0 GT/s use different class of
> > >> presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
> > >> have optional preset hints (Table 4-24).
> > >>
> > >> And there is possibility that for each data rate we may require
> > >> different preset configuration.
> > >>
> > >> Can we have a dt binding for each data rate of 16 byte array.
> > >> like gen3-eq-preset array, gen4-eq-preset array etc.
> > >
> > > Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.
> > >
> > > Keep in mind that this is an RFC; I have a backlog of commit submissions
> > > before I can submit the code that uses this DT property.  If you
> > > (Krishna) want to submit something now I'd be quite happy to go with
> > > that.  I don't believe it is acceptable to submit a bindings commit w/o
> > > code that uses it (if I'm incorrect I'll be glad to do a V2).
> > >
> > I submitted a pull request for this. if you have any other suggestions
> > or if we need to have any other details we can update this pull request.
> > https://github.com/devicetree-org/dt-schema/pull/146
> 
> Thanks for doing this.   However, why a u8 array?  The registers are
> defined as u16 so it would be more natural to use a u16 array, each
> entry giving
> all of the data for a single lane.  In our implementation we read a
> u16 and we write it to the register -- what advantage is there by
> using a u8 array?
> 
> Also if there are 16 lanes, you will need 32 maxItems, correct?

I added these questions to the github PR.

Also, why does it define gen3-eq-presets, gen4-eq-presets,
gen5-eq-presets, and gen6-eq-presets?  I think there's only a single
place to write these values (the Lane Equalization Control registers,
PCIe r6.0, sec 7.7.3.4), isn't here?  How would software choose the
correct values to use?

> > >> - Krishna Chaitanya
> > >>>> +
> > >>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
> > >>>
> > >>> minItems: 1
> > >>> maxItems: 16
> > >>>
> > >>> Last I saw, you can only have up to 16 lanes.
> > >>>
> > >>> Rob
> > >>>
> > >

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Krishna Chaitanya Chundru]

On 10/29/2024 8:18 PM, Bjorn Helgaas wrote:
> On Tue, Oct 29, 2024 at 10:22:36AM -0400, Jim Quinlan wrote:
>> On Tue, Oct 29, 2024 at 1:17 AM Krishna Chaitanya Chundru
>> <quic_krichai@quicinc.com> wrote:
>>> On 10/29/2024 12:21 AM, James Quinlan wrote:
>>>> On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
>>>>> On 10/22/2024 12:33 AM, Rob Herring wrote:
>>>>>> On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
>>>>>>> Support configuration of the GEN3 preset equalization settings, aka the
>>>>>>> Lane Equalization Control Register(s) of the Secondary PCI Express
>>>>>>> Extended Capability.  These registers are of type HwInit/RsvdP and
>>>>>>> typically set by FW.  In our case they are set by our RC host bridge
>>>>>>> driver using internal registers.
>>>>>>>
>>>>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12
>>>>>>> ++++++++++++
>>>>>>>    1 file changed, 12 insertions(+)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>>>> b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>>>> index 0925c520195a..f965ad57f32f 100644
>>>>>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>>>> @@ -104,6 +104,18 @@ properties:
>>>>>>>        minItems: 1
>>>>>>>        maxItems: 3
>>>>>>>    +  brcm,gen3-eq-presets:
>>>>>>> +    description: |
>>>>>>> +      A u16 array giving the GEN3 equilization presets, one for
>>>>>>> each lane.
>>>>>>> +      These values are destined for the 16bit registers known as the
>>>>>>> +      Lane Equalization Control Register(s) of the Secondary PCI
>>>>>>> Express
>>>>>>> +      Extended Capability.  In the array, lane 0 is first term,
>>>>>>> lane 1 next,
>>>>>>> +      etc. The contents of the entries reflect what is necessary for
>>>>>>> +      the current board and SoC, and the details of each preset are
>>>>>>> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
>>>>>>
>>>>>> If these are defined by the PCIe spec, then why is it Broadcom specific
>>>>>> property?
>>>> Yes, I will remove the "brcm," prefix.
>>>>>>
>>>>> Hi Rob,
>>>>>
>>>>> qcom pcie driver also needs to program these presets as you suggested
>>>>> this can go to common pci bridge binding.
>>>>>
>>>>> from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
>>>>> of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
>>>>> through P10) and where as data rates of 64.0 GT/s use different class of
>>>>> presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
>>>>> have optional preset hints (Table 4-24).
>>>>>
>>>>> And there is possibility that for each data rate we may require
>>>>> different preset configuration.
>>>>>
>>>>> Can we have a dt binding for each data rate of 16 byte array.
>>>>> like gen3-eq-preset array, gen4-eq-preset array etc.
>>>>
>>>> Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.
>>>>
>>>> Keep in mind that this is an RFC; I have a backlog of commit submissions
>>>> before I can submit the code that uses this DT property.  If you
>>>> (Krishna) want to submit something now I'd be quite happy to go with
>>>> that.  I don't believe it is acceptable to submit a bindings commit w/o
>>>> code that uses it (if I'm incorrect I'll be glad to do a V2).
>>>>
>>> I submitted a pull request for this. if you have any other suggestions
>>> or if we need to have any other details we can update this pull request.
>>> https://github.com/devicetree-org/dt-schema/pull/146
>>
>> Thanks for doing this.   However, why a u8 array?  The registers are
>> defined as u16 so it would be more natural to use a u16 array, each
>> entry giving
>> all of the data for a single lane.  In our implementation we read a
>> u16 and we write it to the register -- what advantage is there by
>> using a u8 array?
>>
>> Also if there are 16 lanes, you will need 32 maxItems, correct?
> 
> I added these questions to the github PR.
> 
> Also, why does it define gen3-eq-presets, gen4-eq-presets,
> gen5-eq-presets, and gen6-eq-presets?  I think there's only a single
> place to write these values (the Lane Equalization Control registers,
> PCIe r6.0, sec 7.7.3.4), isn't here?  How would software choose the
> correct values to use?
> 
Hi Jim & Bjorn,

7.7.3.4 Lane Equalization Control Register is for gen3 speed, for gen4
we had a new capability register 7.7.5.9 16.0 GT/s Lane Equalization 
Control Register for gen5 we have 7.7.6.9 32.0 GT/s Lane Equalization 
Control Register.

for gen3 we should not use uint8 as gen3 as transmitter preset and
receiver preset hint thus for each lane we need to have uint16 instead
of uint8 as we are defining per lane property in driver where we use
each uint16 value of array needs to be mapped correctly in the register.
If we have only support for single lane then last 16 bits becomes
invalid. so for 16 lanes we need to uint16 array each of uint16 instead
of uint8. ( I will modify it to uint16 instead of uint16)

for remaining gen speeds we don't have receiver preset hint, and it
requires only 8 bits to represent a lane only. so i will use only uint8
for remaining gen speeds.

Bjorn,

from PCIe spec 8.3.3.3 Tx Equalization Presets for 8.0, 16.0, 32.0, and
64.0 GT/ tells which preset value we neeed to use based upon different
parameters. Based upon the type of the board hardware one has to
calculate the preset value and provide them using these properties.

- Krishna Chaitanya.

>>>>> - Krishna Chaitanya
>>>>>>> +
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
>>>>>>
>>>>>> minItems: 1
>>>>>> maxItems: 16
>>>>>>
>>>>>> Last I saw, you can only have up to 16 lanes.
>>>>>>
>>>>>> Rob
>>>>>>
>>>>
> 
> 

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Bjorn Helgaas]

On Tue, Oct 29, 2024 at 08:52:15PM +0530, Krishna Chaitanya Chundru wrote:
> On 10/29/2024 8:18 PM, Bjorn Helgaas wrote:
> > On Tue, Oct 29, 2024 at 10:22:36AM -0400, Jim Quinlan wrote:
> > > On Tue, Oct 29, 2024 at 1:17 AM Krishna Chaitanya Chundru
> > > <quic_krichai@quicinc.com> wrote:
> > > > On 10/29/2024 12:21 AM, James Quinlan wrote:
> > > > > On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
> > > > > > On 10/22/2024 12:33 AM, Rob Herring wrote:
> > > > > > > On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
> > > > > > > > Support configuration of the GEN3 preset equalization settings, aka the
> > > > > > > > Lane Equalization Control Register(s) of the Secondary PCI Express
> > > > > > > > Extended Capability.  These registers are of type HwInit/RsvdP and
> > > > > > > > typically set by FW.  In our case they are set by our RC host bridge
> > > > > > > > driver using internal registers.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > > > > > ---
> > > > > > > >    .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12
> > > > > > > > ++++++++++++
> > > > > > > >    1 file changed, 12 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git
> > > > > > > > a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > index 0925c520195a..f965ad57f32f 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > @@ -104,6 +104,18 @@ properties:
> > > > > > > >        minItems: 1
> > > > > > > >        maxItems: 3
> > > > > > > >    +  brcm,gen3-eq-presets:
> > > > > > > > +    description: |
> > > > > > > > +      A u16 array giving the GEN3 equilization presets, one for
> > > > > > > > each lane.
> > > > > > > > +      These values are destined for the 16bit registers known as the
> > > > > > > > +      Lane Equalization Control Register(s) of the Secondary PCI
> > > > > > > > Express
> > > > > > > > +      Extended Capability.  In the array, lane 0 is first term,
> > > > > > > > lane 1 next,
> > > > > > > > +      etc. The contents of the entries reflect what is necessary for
> > > > > > > > +      the current board and SoC, and the details of each preset are
> > > > > > > > +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
> > > > > > > 
> > > > > > > If these are defined by the PCIe spec, then why is it Broadcom specific
> > > > > > > property?
> > > > > Yes, I will remove the "brcm," prefix.
> > > > > > > 
> > > > > > Hi Rob,
> > > > > > 
> > > > > > qcom pcie driver also needs to program these presets as you suggested
> > > > > > this can go to common pci bridge binding.
> > > > > > 
> > > > > > from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
> > > > > > of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
> > > > > > through P10) and where as data rates of 64.0 GT/s use different class of
> > > > > > presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
> > > > > > have optional preset hints (Table 4-24).
> > > > > > 
> > > > > > And there is possibility that for each data rate we may require
> > > > > > different preset configuration.
> > > > > > 
> > > > > > Can we have a dt binding for each data rate of 16 byte array.
> > > > > > like gen3-eq-preset array, gen4-eq-preset array etc.
> > > > > 
> > > > > Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.
> > > > > 
> > > > > Keep in mind that this is an RFC; I have a backlog of commit submissions
> > > > > before I can submit the code that uses this DT property.  If you
> > > > > (Krishna) want to submit something now I'd be quite happy to go with
> > > > > that.  I don't believe it is acceptable to submit a bindings commit w/o
> > > > > code that uses it (if I'm incorrect I'll be glad to do a V2).
> > > > > 
> > > > I submitted a pull request for this. if you have any other suggestions
> > > > or if we need to have any other details we can update this pull request.
> > > > https://github.com/devicetree-org/dt-schema/pull/146
> > > 
> > > Thanks for doing this.   However, why a u8 array?  The registers are
> > > defined as u16 so it would be more natural to use a u16 array, each
> > > entry giving
> > > all of the data for a single lane.  In our implementation we read a
> > > u16 and we write it to the register -- what advantage is there by
> > > using a u8 array?
> > > 
> > > Also if there are 16 lanes, you will need 32 maxItems, correct?
> > 
> > I added these questions to the github PR.
> > 
> > Also, why does it define gen3-eq-presets, gen4-eq-presets,
> > gen5-eq-presets, and gen6-eq-presets?  I think there's only a single
> > place to write these values (the Lane Equalization Control registers,
> > PCIe r6.0, sec 7.7.3.4), isn't here?  How would software choose the
> > correct values to use?
> 
> 7.7.3.4 Lane Equalization Control Register is for gen3 speed, for gen4
> we had a new capability register 7.7.5.9 16.0 GT/s Lane Equalization Control
> Register for gen5 we have 7.7.6.9 32.0 GT/s Lane Equalization Control
> Register.

Oh, thank you!  I completely missed those new registers.

> for gen3 we should not use uint8 as gen3 as transmitter preset and
> receiver preset hint thus for each lane we need to have uint16 instead
> of uint8 as we are defining per lane property in driver where we use
> each uint16 value of array needs to be mapped correctly in the register.
> If we have only support for single lane then last 16 bits becomes
> invalid. so for 16 lanes we need to uint16 array each of uint16 instead
> of uint8. ( I will modify it to uint16 instead of uint16)
> 
> for remaining gen speeds we don't have receiver preset hint, and it
> requires only 8 bits to represent a lane only. so i will use only uint8
> for remaining gen speeds.
> 
> Bjorn,
> 
> from PCIe spec 8.3.3.3 Tx Equalization Presets for 8.0, 16.0, 32.0, and
> 64.0 GT/ tells which preset value we neeed to use based upon different
> parameters. Based upon the type of the board hardware one has to
> calculate the preset value and provide them using these properties.

Thanks, I had looked at 8.3.3.3, but missed the extra 16, 32, and 64
GT/s registers because 8.3.3.3 looks more like guidance for hardware
design and firmware to calculate the values, and it doesn't mention
the registers the OS would program.

Sorry for confusing things!

Bjorn

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Bjorn Helgaas]

On Tue, Oct 29, 2024 at 10:40:32AM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 29, 2024 at 08:52:15PM +0530, Krishna Chaitanya Chundru wrote:
> > On 10/29/2024 8:18 PM, Bjorn Helgaas wrote:
> > > On Tue, Oct 29, 2024 at 10:22:36AM -0400, Jim Quinlan wrote:
> > > > On Tue, Oct 29, 2024 at 1:17 AM Krishna Chaitanya Chundru
> > > > <quic_krichai@quicinc.com> wrote:
> > > > > On 10/29/2024 12:21 AM, James Quinlan wrote:
> > > > > > On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
> > > > > > > On 10/22/2024 12:33 AM, Rob Herring wrote:
> > > > > > > > On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
> > > > > > > > > Support configuration of the GEN3 preset equalization settings, aka the
> > > > > > > > > Lane Equalization Control Register(s) of the Secondary PCI Express
> > > > > > > > > Extended Capability.  These registers are of type HwInit/RsvdP and
> > > > > > > > > typically set by FW.  In our case they are set by our RC host bridge
> > > > > > > > > driver using internal registers.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > > > > > > ---
> > > > > > > > >    .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12
> > > > > > > > > ++++++++++++
> > > > > > > > >    1 file changed, 12 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git
> > > > > > > > > a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > > b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > > index 0925c520195a..f965ad57f32f 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > > @@ -104,6 +104,18 @@ properties:
> > > > > > > > >        minItems: 1
> > > > > > > > >        maxItems: 3
> > > > > > > > >    +  brcm,gen3-eq-presets:
> > > > > > > > > +    description: |
> > > > > > > > > +      A u16 array giving the GEN3 equilization presets, one for
> > > > > > > > > each lane.
> > > > > > > > > +      These values are destined for the 16bit registers known as the
> > > > > > > > > +      Lane Equalization Control Register(s) of the Secondary PCI
> > > > > > > > > Express
> > > > > > > > > +      Extended Capability.  In the array, lane 0 is first term,
> > > > > > > > > lane 1 next,
> > > > > > > > > +      etc. The contents of the entries reflect what is necessary for
> > > > > > > > > +      the current board and SoC, and the details of each preset are
> > > > > > > > > +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
> > > > > > > > 
> > > > > > > > If these are defined by the PCIe spec, then why is it Broadcom specific
> > > > > > > > property?
> > > > > > Yes, I will remove the "brcm," prefix.
> > > > > > > > 
> > > > > > > Hi Rob,
> > > > > > > 
> > > > > > > qcom pcie driver also needs to program these presets as you suggested
> > > > > > > this can go to common pci bridge binding.
> > > > > > > 
> > > > > > > from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
> > > > > > > of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
> > > > > > > through P10) and where as data rates of 64.0 GT/s use different class of
> > > > > > > presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
> > > > > > > have optional preset hints (Table 4-24).
> > > > > > > 
> > > > > > > And there is possibility that for each data rate we may require
> > > > > > > different preset configuration.
> > > > > > > 
> > > > > > > Can we have a dt binding for each data rate of 16 byte array.
> > > > > > > like gen3-eq-preset array, gen4-eq-preset array etc.
> > > > > > 
> > > > > > Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.
> > > > > > 
> > > > > > Keep in mind that this is an RFC; I have a backlog of commit submissions
> > > > > > before I can submit the code that uses this DT property.  If you
> > > > > > (Krishna) want to submit something now I'd be quite happy to go with
> > > > > > that.  I don't believe it is acceptable to submit a bindings commit w/o
> > > > > > code that uses it (if I'm incorrect I'll be glad to do a V2).
> > > > > > 
> > > > > I submitted a pull request for this. if you have any other suggestions
> > > > > or if we need to have any other details we can update this pull request.
> > > > > https://github.com/devicetree-org/dt-schema/pull/146
> > > > 
> > > > Thanks for doing this.   However, why a u8 array?  The registers are
> > > > defined as u16 so it would be more natural to use a u16 array, each
> > > > entry giving
> > > > all of the data for a single lane.  In our implementation we read a
> > > > u16 and we write it to the register -- what advantage is there by
> > > > using a u8 array?
> > > > 
> > > > Also if there are 16 lanes, you will need 32 maxItems, correct?
> > > 
> > > I added these questions to the github PR.
> > > 
> > > Also, why does it define gen3-eq-presets, gen4-eq-presets,
> > > gen5-eq-presets, and gen6-eq-presets?  I think there's only a single
> > > place to write these values (the Lane Equalization Control registers,
> > > PCIe r6.0, sec 7.7.3.4), isn't here?  How would software choose the
> > > correct values to use?
> ...

Oh, one more thing: I guess "gen3", "gen4", etc. are well-entrenched
terms in the industry, and they're probably fine in marketing
materials.

But I really don't like them in device trees or drivers because the
spec doesn't use those terms.  In fact, the spec specifically advises
*avoiding* them (PCIe r6.0, sec 1.2 footnote):

  Terms like “PCIe Gen3” are ambiguous and should be avoided. For
  example, “gen3” could mean (1) compliant with Base 3.0, (2)
  compliant with Base 3.1 (last revision of 3.x), (3) compliant with
  Base 3.0 and supporting 8.0 GT/s, (4) compliant with Base 3.0 or
  later and supporting 8.0 GT/s, ....

As a practical matter, those terms make it hard to use the spec: where
do I go to learn how to use "gen4-eq-presets"?  There's no instance of
"gen4" in the PCIe spec.  AFAICT, all I can do is look up the PCIe
r4.0 spec and try to figure out what was added in that revision, which
is a real hassle.

Bjorn

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Krishna Chaitanya Chundru]

On 10/29/2024 9:25 PM, Bjorn Helgaas wrote:
> On Tue, Oct 29, 2024 at 10:40:32AM -0500, Bjorn Helgaas wrote:
>> On Tue, Oct 29, 2024 at 08:52:15PM +0530, Krishna Chaitanya Chundru wrote:
>>> On 10/29/2024 8:18 PM, Bjorn Helgaas wrote:
>>>> On Tue, Oct 29, 2024 at 10:22:36AM -0400, Jim Quinlan wrote:
>>>>> On Tue, Oct 29, 2024 at 1:17 AM Krishna Chaitanya Chundru
>>>>> <quic_krichai@quicinc.com> wrote:
>>>>>> On 10/29/2024 12:21 AM, James Quinlan wrote:
>>>>>>> On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
>>>>>>>> On 10/22/2024 12:33 AM, Rob Herring wrote:
>>>>>>>>> On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
>>>>>>>>>> Support configuration of the GEN3 preset equalization settings, aka the
>>>>>>>>>> Lane Equalization Control Register(s) of the Secondary PCI Express
>>>>>>>>>> Extended Capability.  These registers are of type HwInit/RsvdP and
>>>>>>>>>> typically set by FW.  In our case they are set by our RC host bridge
>>>>>>>>>> driver using internal registers.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>>>>>>>> ---
>>>>>>>>>>     .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12
>>>>>>>>>> ++++++++++++
>>>>>>>>>>     1 file changed, 12 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git
>>>>>>>>>> a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>>>>>>> b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>>>>>>> index 0925c520195a..f965ad57f32f 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>>>>>>> @@ -104,6 +104,18 @@ properties:
>>>>>>>>>>         minItems: 1
>>>>>>>>>>         maxItems: 3
>>>>>>>>>>     +  brcm,gen3-eq-presets:
>>>>>>>>>> +    description: |
>>>>>>>>>> +      A u16 array giving the GEN3 equilization presets, one for
>>>>>>>>>> each lane.
>>>>>>>>>> +      These values are destined for the 16bit registers known as the
>>>>>>>>>> +      Lane Equalization Control Register(s) of the Secondary PCI
>>>>>>>>>> Express
>>>>>>>>>> +      Extended Capability.  In the array, lane 0 is first term,
>>>>>>>>>> lane 1 next,
>>>>>>>>>> +      etc. The contents of the entries reflect what is necessary for
>>>>>>>>>> +      the current board and SoC, and the details of each preset are
>>>>>>>>>> +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
>>>>>>>>>
>>>>>>>>> If these are defined by the PCIe spec, then why is it Broadcom specific
>>>>>>>>> property?
>>>>>>> Yes, I will remove the "brcm," prefix.
>>>>>>>>>
>>>>>>>> Hi Rob,
>>>>>>>>
>>>>>>>> qcom pcie driver also needs to program these presets as you suggested
>>>>>>>> this can go to common pci bridge binding.
>>>>>>>>
>>>>>>>> from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
>>>>>>>> of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
>>>>>>>> through P10) and where as data rates of 64.0 GT/s use different class of
>>>>>>>> presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
>>>>>>>> have optional preset hints (Table 4-24).
>>>>>>>>
>>>>>>>> And there is possibility that for each data rate we may require
>>>>>>>> different preset configuration.
>>>>>>>>
>>>>>>>> Can we have a dt binding for each data rate of 16 byte array.
>>>>>>>> like gen3-eq-preset array, gen4-eq-preset array etc.
>>>>>>>
>>>>>>> Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.
>>>>>>>
>>>>>>> Keep in mind that this is an RFC; I have a backlog of commit submissions
>>>>>>> before I can submit the code that uses this DT property.  If you
>>>>>>> (Krishna) want to submit something now I'd be quite happy to go with
>>>>>>> that.  I don't believe it is acceptable to submit a bindings commit w/o
>>>>>>> code that uses it (if I'm incorrect I'll be glad to do a V2).
>>>>>>>
>>>>>> I submitted a pull request for this. if you have any other suggestions
>>>>>> or if we need to have any other details we can update this pull request.
>>>>>> https://github.com/devicetree-org/dt-schema/pull/146
>>>>>
>>>>> Thanks for doing this.   However, why a u8 array?  The registers are
>>>>> defined as u16 so it would be more natural to use a u16 array, each
>>>>> entry giving
>>>>> all of the data for a single lane.  In our implementation we read a
>>>>> u16 and we write it to the register -- what advantage is there by
>>>>> using a u8 array?
>>>>>
>>>>> Also if there are 16 lanes, you will need 32 maxItems, correct?
>>>>
>>>> I added these questions to the github PR.
>>>>
>>>> Also, why does it define gen3-eq-presets, gen4-eq-presets,
>>>> gen5-eq-presets, and gen6-eq-presets?  I think there's only a single
>>>> place to write these values (the Lane Equalization Control registers,
>>>> PCIe r6.0, sec 7.7.3.4), isn't here?  How would software choose the
>>>> correct values to use?
>> ...
> 
> Oh, one more thing: I guess "gen3", "gen4", etc. are well-entrenched
> terms in the industry, and they're probably fine in marketing
> materials.
> 
> But I really don't like them in device trees or drivers because the
> spec doesn't use those terms.  In fact, the spec specifically advises
> *avoiding* them (PCIe r6.0, sec 1.2 footnote):
> 
>    Terms like “PCIe Gen3” are ambiguous and should be avoided. For
>    example, “gen3” could mean (1) compliant with Base 3.0, (2)
>    compliant with Base 3.1 (last revision of 3.x), (3) compliant with
>    Base 3.0 and supporting 8.0 GT/s, (4) compliant with Base 3.0 or
>    later and supporting 8.0 GT/s, ....
> 
> As a practical matter, those terms make it hard to use the spec: where
> do I go to learn how to use "gen4-eq-presets"?  There's no instance of
> "gen4" in the PCIe spec.  AFAICT, all I can do is look up the PCIe
> r4.0 spec and try to figure out what was added in that revision, which
> is a real hassle.
> 
is it fine if I change names from gen3-eq-presets, gen4-eq-presets etc
to 8gts-eq-presets, 16gts-eq-presets  etc.

If above names are fine I will update the patch.

- Krishna Chaitanya.
> Bjorn

Re: [PATCH 1/1] RFC: dt bindings: Add property "brcm,gen3-eq-presets"

[Bjorn Helgaas]

On Tue, Oct 29, 2024 at 10:24:48PM +0530, Krishna Chaitanya Chundru wrote:
> On 10/29/2024 9:25 PM, Bjorn Helgaas wrote:
> > On Tue, Oct 29, 2024 at 10:40:32AM -0500, Bjorn Helgaas wrote:
> > > On Tue, Oct 29, 2024 at 08:52:15PM +0530, Krishna Chaitanya Chundru wrote:
> > > > On 10/29/2024 8:18 PM, Bjorn Helgaas wrote:
> > > > > On Tue, Oct 29, 2024 at 10:22:36AM -0400, Jim Quinlan wrote:
> > > > > > On Tue, Oct 29, 2024 at 1:17 AM Krishna Chaitanya Chundru
> > > > > > <quic_krichai@quicinc.com> wrote:
> > > > > > > On 10/29/2024 12:21 AM, James Quinlan wrote:
> > > > > > > > On 10/24/24 21:08, Krishna Chaitanya Chundru wrote:
> > > > > > > > > On 10/22/2024 12:33 AM, Rob Herring wrote:
> > > > > > > > > > On Fri, Oct 18, 2024 at 02:22:45PM -0400, Jim Quinlan wrote:
> > > > > > > > > > > Support configuration of the GEN3 preset equalization settings, aka the
> > > > > > > > > > > Lane Equalization Control Register(s) of the Secondary PCI Express
> > > > > > > > > > > Extended Capability.  These registers are of type HwInit/RsvdP and
> > > > > > > > > > > typically set by FW.  In our case they are set by our RC host bridge
> > > > > > > > > > > driver using internal registers.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > > > > > > > > ---
> > > > > > > > > > >     .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12
> > > > > > > > > > > ++++++++++++
> > > > > > > > > > >     1 file changed, 12 insertions(+)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git
> > > > > > > > > > > a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > > > > b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > > > > index 0925c520195a..f965ad57f32f 100644
> > > > > > > > > > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > > > > > > > > @@ -104,6 +104,18 @@ properties:
> > > > > > > > > > >         minItems: 1
> > > > > > > > > > >         maxItems: 3
> > > > > > > > > > >     +  brcm,gen3-eq-presets:
> > > > > > > > > > > +    description: |
> > > > > > > > > > > +      A u16 array giving the GEN3 equilization presets, one for
> > > > > > > > > > > each lane.
> > > > > > > > > > > +      These values are destined for the 16bit registers known as the
> > > > > > > > > > > +      Lane Equalization Control Register(s) of the Secondary PCI
> > > > > > > > > > > Express
> > > > > > > > > > > +      Extended Capability.  In the array, lane 0 is first term,
> > > > > > > > > > > lane 1 next,
> > > > > > > > > > > +      etc. The contents of the entries reflect what is necessary for
> > > > > > > > > > > +      the current board and SoC, and the details of each preset are
> > > > > > > > > > > +      described in Section 7.27.4 of the PCI base spec, Revision 3.0.
> > > > > > > > > > 
> > > > > > > > > > If these are defined by the PCIe spec, then why is it Broadcom specific
> > > > > > > > > > property?
> > > > > > > > Yes, I will remove the "brcm," prefix.
> > > > > > > > > > 
> > > > > > > > > Hi Rob,
> > > > > > > > > 
> > > > > > > > > qcom pcie driver also needs to program these presets as you suggested
> > > > > > > > > this can go to common pci bridge binding.
> > > > > > > > > 
> > > > > > > > > from PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4.2 for data rates
> > > > > > > > > of  8.0 GT/s, 16.0 GT/s, and 32.0 GT/s uses one class of preset (P0
> > > > > > > > > through P10) and where as data rates of 64.0 GT/s use different class of
> > > > > > > > > presets (Q0 through Q10) (Table 4-23). And data rates of 8.0 GT/s also
> > > > > > > > > have optional preset hints (Table 4-24).
> > > > > > > > > 
> > > > > > > > > And there is possibility that for each data rate we may require
> > > > > > > > > different preset configuration.
> > > > > > > > > 
> > > > > > > > > Can we have a dt binding for each data rate of 16 byte array.
> > > > > > > > > like gen3-eq-preset array, gen4-eq-preset array etc.
> > > > > > > > 
> > > > > > > > Yes, that was the idea when using "genX-eq-preset", for X in {3,4...}.
> > > > > > > > 
> > > > > > > > Keep in mind that this is an RFC; I have a backlog of commit submissions
> > > > > > > > before I can submit the code that uses this DT property.  If you
> > > > > > > > (Krishna) want to submit something now I'd be quite happy to go with
> > > > > > > > that.  I don't believe it is acceptable to submit a bindings commit w/o
> > > > > > > > code that uses it (if I'm incorrect I'll be glad to do a V2).
> > > > > > > > 
> > > > > > > I submitted a pull request for this. if you have any other suggestions
> > > > > > > or if we need to have any other details we can update this pull request.
> > > > > > > https://github.com/devicetree-org/dt-schema/pull/146
> > > > > > 
> > > > > > Thanks for doing this.   However, why a u8 array?  The registers are
> > > > > > defined as u16 so it would be more natural to use a u16 array, each
> > > > > > entry giving
> > > > > > all of the data for a single lane.  In our implementation we read a
> > > > > > u16 and we write it to the register -- what advantage is there by
> > > > > > using a u8 array?
> > > > > > 
> > > > > > Also if there are 16 lanes, you will need 32 maxItems, correct?
> > > > > 
> > > > > I added these questions to the github PR.
> > > > > 
> > > > > Also, why does it define gen3-eq-presets, gen4-eq-presets,
> > > > > gen5-eq-presets, and gen6-eq-presets?  I think there's only a single
> > > > > place to write these values (the Lane Equalization Control registers,
> > > > > PCIe r6.0, sec 7.7.3.4), isn't here?  How would software choose the
> > > > > correct values to use?
> > > ...
> > 
> > Oh, one more thing: I guess "gen3", "gen4", etc. are well-entrenched
> > terms in the industry, and they're probably fine in marketing
> > materials.
> > 
> > But I really don't like them in device trees or drivers because the
> > spec doesn't use those terms.  In fact, the spec specifically advises
> > *avoiding* them (PCIe r6.0, sec 1.2 footnote):
> > 
> >    Terms like “PCIe Gen3” are ambiguous and should be avoided. For
> >    example, “gen3” could mean (1) compliant with Base 3.0, (2)
> >    compliant with Base 3.1 (last revision of 3.x), (3) compliant with
> >    Base 3.0 and supporting 8.0 GT/s, (4) compliant with Base 3.0 or
> >    later and supporting 8.0 GT/s, ....
> > 
> > As a practical matter, those terms make it hard to use the spec: where
> > do I go to learn how to use "gen4-eq-presets"?  There's no instance of
> > "gen4" in the PCIe spec.  AFAICT, all I can do is look up the PCIe
> > r4.0 spec and try to figure out what was added in that revision, which
> > is a real hassle.
> > 
> is it fine if I change names from gen3-eq-presets, gen4-eq-presets etc
> to 8gts-eq-presets, 16gts-eq-presets  etc.

Those would be fine with me.  If names starting with digits don't
work, things like "eq-presets-8gts" would also be fine.