Re: [RFC 1/2] dt-bindings: usb: dwc3: Add support for multiport related properties

[Harsh Agarwal]

On 5/21/2022 8:35 AM, Bjorn Andersson wrote:
> On Thu 19 May 05:34 PDT 2022, Harsh Agarwal wrote:
>
> > Added support for multiport, mport, num-ssphy and num-hsphy
> > properties. These properties are used to support devices having
> > a multiport controller.
> >
> > Signed-off-by: Harsh Agarwal <quic_harshq@xxxxxxxxxxx>
> Please do run dt_binding_check on your bindings, even though they are
> RFCs.
There was some dt_binding package issue so I could not run it.
Thought to check this later since this is RFC.
Sure, I will check this.
> > ---
> >   .../devicetree/bindings/usb/snps,dwc3.yaml         | 55 ++++++++++++++++++++++
> >   1 file changed, 55 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > index f4471f8..39c61483 100644
> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > @@ -341,6 +341,35 @@ properties:
> >         This port is used with the 'usb-role-switch' property  to connect the
> >         dwc3 to type C connector.
> >   
> > +  multiport:
> Why are you inventing an of_graph lookalike here?

Not really an of_graph lookalike, here we just wanted to add "multiport" 
node which would have "mport" nodes for every port that it supports.
We can treat this "multiport" like an object encapsulating other "mport" 
objects.

> > +    description:
> > +      If a single USB controller supports multiple ports, then it's referred to as
> > +      a multiport controller. Each port of the multiport controller can support
> > +      either High Speed or Super Speed or both and have their own PHY phandles. Each
> > +      port is represented by "mport" node and all the "mport" nodes are grouped
> > +      together inside the "multiport" node where individual "mport" node defines the
> > +      PHYs supported by that port.
> > +    required:
> > +      - mport
> > +
> > +  num-hsphy:
> > +    description: Total number of HS-PHYs defined by the multiport controller.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> I'm expecting that you wont' have any superspeed-only ports. As such
> this number would imply be the number of ports listed under the node.

Yes we can say that num_hsphy is equal to the number of "mport" nodes as 
every node will
have a HSPHY.

> > +
> > +  num-ssphy:
> > +    description: Total number of SS-PHYs defined by the multiport controller.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> Can you please explain why it's necessary to specify usb_nop_phy?
> Wouldn't it be possible to omit the phy in the case of a HS-only port?
> In which case this could just be calculated as well.
Yes we can ignore usb_nop_phy. It's not really necessary at places where 
we don't have a proper PHY in place.
> Regards,
> Bjorn
>
> > +
> > +  mport:
> > +    description: Each mport node represents one port of the multiport controller.
> > +    patternProperties: "^mport@[0-9a-f]+$"
> > +    oneOf:
> > +       - required:
> > +         - usb-phy
> > +       - required:
> > +          - phys
> > +          - phy-names
> > +
> >   unevaluatedProperties: false
> >   
> >   required:
> > @@ -369,4 +398,30 @@ examples:
> >         snps,dis_u2_susphy_quirk;
> >         snps,dis_enblslpm_quirk;
> >       };
> > +  - |
> > +    usb@4a000000 {
> > +      compatible = "snps,dwc3";
> > +      reg = <0x4a000000 0xcfff>;
> > +      interrupts = <0 92 4>;
> > +
> > +      multiport {
> > +
> > +	MP_1: mport@1 {
> > +          usb-phy = <&usb2_phy0>, <&usb3_phy0>;
> > +	};
> > +
> > +	MP_2: mport@2 {
> > +          usb-phy = <&usb2_phy1>, <&usb3_phy1>;
> > +	};
> > +
> > +	MP_3: mport@3 {
> > +          usb-phy = <&usb2_phy2>, <&usb_nop_phy>;
> > +	};
> > +
> > +	MP_4: mport@4 {
> > +          usb-phy = <&usb2_phy3>, <&usb_nop_phy>;
> > +	};
> > +
> > +      };
> > +    };
> >   ...
> > --
> > 2.7.4