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

[Manikanta Maddireddy]

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?
> > Signed-off-by: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx>
> > ---
> >   .../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

> Best regards,
> Krzysztof