Re: [PATCH 1/3] dt-bindings: PHY: P2U: Add PCIe lane margining support

[Manikanta Maddireddy <mmaddireddy@nvidia.com>]

On 11/30/2022 8:45 PM, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 30/11/2022 16:11, Manikanta Maddireddy wrote:
>> Thank you for quick review. I will wait for other reviewers to review
>> patch 2 & 3.
>> I will address all review comments and sendnew revision.
>>
>> On 11/24/2022 2:15 PM, Krzysztof Kozlowski wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 24/11/2022 09:35, Manikanta Maddireddy wrote:
>>>> Tegra234 supports PCIe lane margining. P2U HW acts as a relay to exchange
>>> typo: merging?
>> It is not typo, it is PCIe feature lane margining.
>> https://pcisig.com/pushing-limits-understanding-lane-margining-pcie%C2%AE
>>>> margin control data and margin status between PCIe controller and UPHY.
>>> Please use scripts/get_maintainers.pl to get a list of necessary people
>>> and lists to CC.  It might happen, that command when run on an older
>>> kernel, gives you outdated entries.  Therefore please be sure you base
>>> your patches on recent Linux kernel.
>> I verified these patches on 6.0.0-rc6 kernel and executed get_maintainers.pl
>> script on it. Did I miss anyone here?
> Yes. At least Rob, maybe more. You need to CC all
> maintainers/reviewers/supporters and all mailing lists.
>
> It's not my task to verify each of these addresses to check whether you
> really missed someone or not. I spotted at least one missing address so
> just run get_maintainers.pl and use all entries from there.
I rechecked my "git send-email" command, I did add Rob's email. In my 
Linux file path, I found a file with name "--to=robh+dt@kernel.org".
I am not sure what mistake I made during "git send-email". I will do 
dry-run before sending v2.

I rechecked mailing list from get_maintainers.pl, I added everyone from 
the list expect Tegra developers who contributed to the file 
tegra234.dtsi, but not for PCIe.
>
>>>
>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>> ---
>>>>    .../bindings/phy/phy-tegra194-p2u.yaml        | 50 +++++++++++++++++++
>>>>    1 file changed, 50 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/phy-tegra194-p2u.yaml b/Documentation/devicetree/bindings/phy/phy-tegra194-p2u.yaml
>>>> index 4dc5205d893b..0ba3f6a0b474 100644
>>>> --- a/Documentation/devicetree/bindings/phy/phy-tegra194-p2u.yaml
>>>> +++ b/Documentation/devicetree/bindings/phy/phy-tegra194-p2u.yaml
>>>> @@ -40,6 +40,51 @@ properties:
>>>>      '#phy-cells':
>>>>        const: 0
>>>>
>>>> +  interrupts:
>>>> +    items:
>>>> +      description: P2U interrupt for Gen4 lane margining functionality.
>>> typo: merging?
>> It is not typo, it is PCIe feature lane margining.
>> https://pcisig.com/pushing-limits-understanding-lane-margining-pcie%C2%AE
>>>> +
>>>> +  interrupt-names:
>>>> +    items:
>>>> +      - const: intr
>>> Drop entire property, not really useful.
>> In driver, I am using platform_get_irq_byname(), I will change it to
>> platform_get_irq()
>> and drop this property.
>>>> +
>>>> +  nvidia,bpmp:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>>> +    description: Must contain a pair of phandles to BPMP controller node followed by P2U ID.
>>>> +    items:
>>>> +      - items:
>>>> +          - description: phandle to BPMP controller node
>>>> +          - description: P2U instance ID
>>>> +            maximum: 24
>>>> +
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - nvidia,tegra194-p2u
>>>> +    then:
>>>> +      required:
>>>> +        - reg
>>>> +        - reg-names
>>>> +        - '#phy-cells'
>>> That's not how it should be done. You have only two variants here, so
>>> add a "required:" block with above and only one if:then: clause for
>>> interrupts and nvidia,bpmp.
>>>
>>> Requiring reg/reg-names/phy-cells should be in separate patch with its
>>> own reasoning.
>> Ok, I will create two separate patches and add if:then clause only for
>> tegra234.
>> As per understanding final change will look like below, right?
>>
>>
>> required:
>>     - reg
>>     - reg-names
>>     - '#phy-cells'
>>
>> allOf:
>>     - if:
>>         properties:
>>           compatible:
>>             contains:
>>               enum:
>>                 - nvidia,tegra234-p2u
>>       then:
>>         required:
>>           - interrupts
>>           - nvidia,bpmp
> yes
>
> Best regards,
> Krzysztof
>