| Message ID | 1654709787-23686-2-git-send-email-quic_harshq@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for multiport controller | expand |
On Wed, 08 Jun 2022 23:06:25 +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(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:367:9: [warning] wrong indentation: expected 10 but found 8 (indentation) ./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:369:9: [warning] wrong indentation: expected 10 but found 8 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
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. > + 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? > + - 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"; Rob
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 ? 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. Do you feel we still might have a problem with multiport node ? >> + 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 ? Internally we use usb-phy with our downstream GLUE driver > >> + - 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). 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. We cannot break existing implementation and so we thought of going with the "multiport" node approach, listing below some flexibility : 1. Better representation of the PHYs and it's relation with a port. 2. Here for each port we pick the first PHY as HS and 2nd PHY as SS as we have been doing traditionally. So for "usb-phy" we need not care how the PHY handles are named. 3. It's future proof incase we need to add additional properties specific to a port. We can just add those properties inside MP_1 or MP_2 etc. Though nothing like this has yet been implemented. Also agree that there are multiple ways to approach this problem and that's why we had RFC tag earlier to get feedback on the same. > Rob
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. > We cannot break existing implementation and so we thought of going with the > "multiport" node approach, listing below some flexibility : How would this break? > 1. Better representation of the PHYs and it's relation with a port. > 2. Here for each port we pick the first PHY as HS and 2nd PHY as SS as we > have been doing traditionally. > So for "usb-phy" we need not care how the PHY handles are named. > > 3. It's future proof incase we need to add additional properties specific to > a port. We can just add those properties inside MP_1 or MP_2 etc. > Though nothing like this has yet been implemented. Then you have to consider how the standard USB device binding fits into this, and it needs to work for anyone with multiple ports. The usb-hcd.yaml schema already defines that child nodes represent a USB device attached to a port on the host. 'reg' is the port number. Rob
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. 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"; Please let me know if you prefer any approach here or have any suggestions. > > >> We cannot break existing implementation and so we thought of going with the >> "multiport" node approach, listing below some flexibility : > How would this break? > >> 1. Better representation of the PHYs and it's relation with a port. >> 2. Here for each port we pick the first PHY as HS and 2nd PHY as SS as we >> have been doing traditionally. >> So for "usb-phy" we need not care how the PHY handles are named. >> >> 3. It's future proof incase we need to add additional properties specific to >> a port. We can just add those properties inside MP_1 or MP_2 etc. >> Though nothing like this has yet been implemented. > Then you have to consider how the standard USB device binding fits into > this, and it needs to work for anyone with multiple ports. The > usb-hcd.yaml schema already defines that child nodes represent a USB > device attached to a port on the host. 'reg' is the port number. > > Rob
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
Attempt to revive this thread. Hi Rob, Apologies for the delay in pursuing this thread further. Neither Harsh nor me were able to pursue this since July because of some other work load. On 7/7/2022 3:39 AM, Rob Herring wrote: > 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. > If we are concentrating only on generic phy's do we need to refrain from making any changes to downstream phy part of the driver code in core.c as done in this RFC series ? I wanted to update this series after addressing review comments. If we don't need to, then I can revert changes done to dwc->usb2->phy and dwc->usb3_phy in this series. >> 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. > Thanks for the review. How about we do the following: Assuming we have 3 ports where first port is HS+SS capable and the other two are only HS capable. We can implement phys and phy-names as : phy = <&usb-hs-phy1>,<&usb-ss-phy1>, <&usb-hs-phy2>, <&usb-hs-phy3>, phy-names = "usb2-phy-port0", "usb3-phy-port0", "usb2-phy-port1", "usb2-phy-port2"; Since the driver code mandates that the phy-names are supposed to be "usb2-phy" and "usb3-phy", we can be sure that the first part of every phy name starts with "usb2-phy" or "usb3-phy" and we can append -portX at end of each phy name to differentiate them as shown in the above example. In the driver code we can iterate over each of the phy-names property and string compare them with "usb2-phy" and "usb3-phy" to identify whether it is HS/SS. That way even if we have only one-port the code would still hold good. Let me know if that approach would be fine. Once again, sorry for delaying this thread. Regards, Krishna, > Rob
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: + 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 + - required: + - phys + - phy-names + unevaluatedProperties: false required: @@ -371,4 +397,31 @@ 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: mport1 { + usb-phy = <&usb2_phy0>, <&usb3_phy0>; + /* Can define Generic PHYs also */ + }; + + MP_2: mport2 { + usb-phy = <&usb2_phy1>, <&usb3_phy1>; + }; + + MP_3: mport3 { + usb-phy = <&usb2_phy2>; + }; + + MP_4: mport4 { + usb-phy = <&usb2_phy3>; + }; + + }; + }; ...
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(+)