Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7ce2fd0f-621d-dfbb-2ec0-53ccbb3308ae@linaro.org>
Date:   Wed, 30 Nov 2022 16:15:58 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH 1/3] dt-bindings: PHY: P2U: Add PCIe lane margining
 support
Content-Language: en-US
To:     Manikanta Maddireddy <mmaddireddy@nvidia.com>, vkoul@kernel.org,
        kishon@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        thierry.reding@gmail.com, jonathanh@nvidia.com, vidyas@nvidia.com
Cc:     linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
        linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org, bhelgaas@google.com,
        lpieralisi@kernel.org, nkristam@nvidia.com
References: <20221124083510.3008139-1-mmaddireddy@nvidia.com>
 <20221124083510.3008139-2-mmaddireddy@nvidia.com>
 <0b203c11-851c-fd6c-faeb-e283b158f1c8@linaro.org>
 <b81c69e0-13a2-6a25-21c7-c0d8804f9c3b@nvidia.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <b81c69e0-13a2-6a25-21c7-c0d8804f9c3b@nvidia.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.


>>
>>
>>> 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