[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c755ee45-fb6d-1f7c-2a3b-bcaa9d292c1b@nvidia.com>
Date: Wed, 30 Nov 2022 21:43:45 +0530
From: Manikanta Maddireddy <mmaddireddy@...dia.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
vkoul@...nel.org, kishon@...nel.org,
krzysztof.kozlowski+dt@...aro.org, thierry.reding@...il.com,
jonathanh@...dia.com, vidyas@...dia.com
Cc: linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, bhelgaas@...gle.com,
lpieralisi@...nel.org, nkristam@...dia.com
Subject: Re: [PATCH 1/3] dt-bindings: PHY: P2U: Add PCIe lane margining
support
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@...nel.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@...dia.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
>
Powered by blists - more mailing lists