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

[Krishna Chaitanya Chundru <quic_krichai@quicinc.com>]

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