| Message ID | 1652963695-10109-1-git-send-email-quic_harshq@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support for multiport controller | expand |
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@quicinc.com> Please do run dt_binding_check on your bindings, even though they are RFCs. > --- > .../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? > + 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. > + > + 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. 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 >
Hi Harsh, On Thu, May 19, 2022 at 06:04:54PM +0530, 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@quicinc.com> > --- > .../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: > + 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 > + > + num-ssphy: > + description: Total number of SS-PHYs defined by the multiport controller. > + $ref: /schemas/types.yaml#/definitions/uint32 > + Do we need this properties at all? Atleast your next patch in this series is not parsing those properties. The idea I believe is to maintain the same usb-phy / phys semantics as of today. i.e we expect first PHY to be USB2 PHY and 2nd PHY to be USB3 PHY. Obviously, we need to make sure that all ports defined under multiport node are passing PHYs without any holes. For example, if the controller has 3 ports and passing phys for 1st and 3rd port is not acceptible. In any case we need to know the number of HS and SS PHYs so that the GUSB2PHYCFG/GUSB3PIPECTL are configured correctly, irrespective of how we handle phy(s) in this node. Can you please clarify on the need for num-hsphy and num-ssphy and what happens for USB2 only ports? > + 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 > + Thanks, Pavan
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@quicinc.com> > 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 >>
On Thu, May 19, 2022 at 06:04:54PM +0530, 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@quicinc.com> > --- > .../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: > + 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 > + > + num-ssphy: > + description: Total number of SS-PHYs defined by the multiport controller. > + $ref: /schemas/types.yaml#/definitions/uint32 These seem redundant. Can't you count ports/phys? > + > + mport: > + description: Each mport node represents one port of the multiport controller. > + patternProperties: "^mport@[0-9a-f]+$" Think about how the USB device binding fits into this. A hub vs. multiple root ports doesn't seem much different. Rob
On 5/23/2022 5:55 PM, Rob Herring wrote: > On Thu, May 19, 2022 at 06:04:54PM +0530, 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@quicinc.com> >> --- >> .../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: >> + 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 >> + >> + num-ssphy: >> + description: Total number of SS-PHYs defined by the multiport controller. >> + $ref: /schemas/types.yaml#/definitions/uint32 > These seem redundant. Can't you count ports/phys? Counting ports will not suffice as there could be devices which has different numbers of HS and SS PHYs. Consider a case like a Quadport multiport controller where we have the 1st & 2nd port supporting SS and 3rd & 4th port supporting HS only. So we would need the proper number of HS and SS PHYs. Currently in the RFC v2 we are calculating the exact number of PHYS supported by each port by counting the phandles we have defined. > >> + >> + mport: >> + description: Each mport node represents one port of the multiport controller. >> + patternProperties: "^mport@[0-9a-f]+$" > Think about how the USB device binding fits into this. A hub vs. > multiple root ports doesn't seem much different. A hub will have multiple ports but only one HS and SS PHYs at the root port supporting it. With Multiport Controller, each port will have its own PHY [whether HS only or both HS & SS] > > Rob
Currently the DWC3 driver supports only single port controller which requires at most two PHYs ie HS and SS PHYs. There are SoCs that has DWC3 controller with multiple ports that can operate in host mode. Some of the port supports both SS+HS and other port supports only HS mode. This change refactors the PHY logic to support multiport controller. The patches have gone through basic sanity only. For any multiport controller we would define a new node "multiport" inside dwc3 and then add subsequent "mport" nodes inside it for individual ports that it supports. Now each individual "mport" defines the PHYs that it supports. Looking for comments/feedback on the device tree bindings. Once the bindings are locked, we can further factor the code. e.g. Consider a Dual port controller where each port supports HS+SS multiport { mp_1: mport@1 { usb-phys = <usb2_phy0>, <usb3_phy0>; }; mp_2: mport@2 { usb-phys = <usb2_phy1>, <usb3_phy1>; }; }; Harsh Agarwal (2): dt-bindings: usb: dwc3: Add support for multiport related properties usb: dwc3: Refactor PHY logic to support Multiport Controller .../devicetree/bindings/usb/snps,dwc3.yaml | 55 +++++ drivers/usb/dwc3/core.c | 259 ++++++++++++++++----- drivers/usb/dwc3/core.h | 8 +- drivers/usb/dwc3/drd.c | 8 +- drivers/usb/dwc3/gadget.c | 4 +- 5 files changed, 264 insertions(+), 70 deletions(-)