Re: [PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Rob Herring <robh@kernel.org>]

On Mon, Jun 27, 2022 at 06:36:53PM +0530, Harsh Agarwal wrote:
> 
> On 6/10/2022 10:52 PM, Rob Herring wrote:
> > On Fri, Jun 10, 2022 at 05:25:25PM +0530, Harsh Agarwal wrote:
> > > On 6/9/2022 9:08 PM, Rob Herring wrote:
> > > > On Wed, Jun 08, 2022 at 11:06:25PM +0530, Harsh Agarwal wrote:
> > > > > Added support for multiport, mport, num_usb2_phy and num_usb3_phy
> > > > > properties. These properties are used to support devices having
> > > > > a multiport controller.
> > > > > 
> > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > > > ---
> > > > >    .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
> > > > >    1 file changed, 53 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > > index d41265b..9332fa2 100644
> > > > > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > > @@ -343,6 +343,32 @@ properties:
> > > > >          This port is used with the 'usb-role-switch' property  to connect the
> > > > >          dwc3 to type C connector.
> > > > > +  multiport:
> > > > Again, I don't think this is going to play well if you need to describe
> > > > USB devices in your DT. For example, a USB hub with additional DT
> > > > properties.
> > > Thanks for the review Rob.
> > > Can you please explain why would one want to describe a USB hub in device
> > > tree ?
> > Because someone soldered a hub on the board and then connected extra
> > things like resets, GPIOs, supplies which are all outside of standard
> > USB. It's quite common...
> > 
> > There's some flavors of Beagle boards that have a USB ethernet on board.
> > Guess what, they skipped out on a eeprom and so the device and a MAC
> > address has to be described in DT (if you want a stable MAC addr).
> > 
> > > IF USB hub is attached to a root port , it would be enumerated by the SW. I
> > > am not clear how DT is coming
> > > into picture. Even if there was a scenario to add DT properties for a hub,
> > > then this multiport node would be like a nop
> > > as it just helps us to get the PHY phandles in a proper way.
> > It won't be enumerated by the SW if it has to be powered on first using
> > non-standard resources.
> > 
> > > Do you feel we still might have a problem with multiport node ?
> > A board design could have a hub or device on any or all your ports.
> > 
> > > > > +    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.
> > > > > +
> > > > > +  num_usb2_phy:
> > > > > +    description: Total number of HS-PHYs defined by the multiport controller.
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +
> > > > > +  num_usb3_phy:
> > > > > +    description: Total number of SS-PHYs defined by the multiport controller.
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +
> > > > > +  mport:
> > > > > +    description: Each mport node represents one port of the multiport controller.
> > > > > +    oneOf:
> > > > > +      - required:
> > > > > +        - usb-phy
> > > > This is deprecated. Why are you adding it?
> > > Do you mean "usb-phy" is deprecated ?
> > It is replaced by 'phys'. Any new user should use 'phys'.
> > 
> > > Internally we use usb-phy with our downstream GLUE driver
> > Upstream does not care about that.
> > 
> > > > > +      - required:
> > > > > +        - phys
> > > > > +        - phy-names
> > > > Other multi port USB hosts just have a list of phys. Why can't you just
> > > > use phy-names to identify each phy:
> > > > 
> > > > phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
> > > >     "port3-hs";
> > > With the above method we would have to do some kind of string parsing on the
> > > phy-names to get the HS and SS PHYs as we need to cater to different
> > > combinations of Ports ( some support HS+SS , other supports SS only).
> > You are doing string parsing anyways to get the child nodes and
> > properties.
> > 
> > > So one challenge here is with the "usb-phy". There we directly define the
> > > phy phandles and that might/might-not have proper sub-strings. eg
> > > USB_QMP_PHY . So extracting PHYS could be tricky if the phy-handle does not
> > > have proper substring like "SS" "HS" etc.
> > The schema can and should enforce that you have the proper strings.
> Hi Rob,
> Apologies for replying late.
> 
> I get your concern. Yes we can remove the "multiport" node and instead
> define the
> USB phy phandles all in one place. Still I would need to add support for
> both generic-phy and
> usb-phy framework as downstream many vendors are using "usb-phy" and it's
> supported by ACK as well.

Upstream is not concerned with downstream. The generic PHY has replaced 
usb-phy for many years now.

Furthermore, if downstream was using documented bindings, then we 
wouldn't be having this conversation.

> This would not regress anything with Generic PHY.
> 
> @Greg can you please comment as ACK has support for usb-phy framework.
> 
> Now coming to implementation, let's consider a 4 port USB multiport
> controller having
> 4 HS PHYs and 2 SS PHYs.  We can have two approaches here
> 
> #1 -> If we could mandate using "HS" or "SS" as substring in
> phy-names or usb-phy, then we can calculate number of HS and SS phy and also
> get
> corresponding PHY nodes. Only concern here is that downstream vendors might
> need
> to change their existing usb-phy names and add proper substring if they are
> not doing so ;
> 
> phy = <&usb-hs-phy>,<&usb-ss-phy>,
>       <&usb-hs-phy1>, <&usb-ss-phy1>,
>       <&usb-hs-phy2>, <&usb-hs-phy3>;
> 
> phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
>    "port3-hs";
> 
> 
> OR
> 
> 
> #2-> We could mandate defining the USB phy in HS - SS pairs.
> For ports that has only HS PHY, we would need to define usb_nop_phy in SS
> place.
> Then we can calculate the number of HS & SS phys and get corresponding
> PHY nodes by using simple fact that HS phy would be defined at odd places &
> SS phy defined at even. Here substrings are not mandated.
> 
> phy = <&usb-hs-phy>,<&usb-qmp-phy>,
>       <&usb-hs-phy1>, <&usb-qmp-phy1>,
>       <&usb-hs-phy2>, <&usb_nop_phy>
>       <&usb-hs-phy3>, <&usb_nop_phy>;
> 
> phy-names = "port0-hs", "port0-ss",
> 	    "port1-hs", "port1-ss",
> 	    "port2-hs", "usb-nop",
> 	    "port3-hs", "usb-nop";

The whole reason for -names is to avoid something like this with filler 
entries. So I prefer #1 as I suggested.

Rob