| Message ID | 1652963695-10109-2-git-send-email-quic_harshq@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for multiport controller | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Thu, 19 May 2022 18:04:54 +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(+) > 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:8: [warning] wrong indentation: expected 6 but found 7 (indentation) ./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:368:10: [warning] wrong indentation: expected 11 but found 9 (indentation) ./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:370:11: [warning] wrong indentation: expected 11 but found 10 (indentation) ./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:409:1: [error] syntax error: found character '\t' that cannot start any token (syntax) dtschema/dtc warnings/errors: make[1]: *** Deleting file 'Documentation/devicetree/bindings/usb/snps,dwc3.example.dts' Traceback (most recent call last): File "/usr/local/bin/dt-extract-example", line 52, in <module> binding = yaml.load(open(args.yamlfile, encoding='utf-8').read()) File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/main.py", line 434, in load return constructor.get_single_data() File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 119, in get_single_data node = self.composer.get_single_node() File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node File "_ruamel_yaml.pyx", line 773, in _ruamel_yaml.CParser._compose_node File "_ruamel_yaml.pyx", line 852, in _ruamel_yaml.CParser._compose_sequence_node File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event ruamel.yaml.scanner.ScannerError: while scanning a block scalar in "<unicode string>", line 401, column 5 found a tab character where an indentation space is expected in "<unicode string>", line 409, column 1 make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/usb/snps,dwc3.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/snps,dwc3.yaml: while scanning a block scalar in "<unicode string>", line 401, column 5 found a tab character where an indentation space is expected in "<unicode string>", line 409, column 1 ./Documentation/devicetree/bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/samsung,exynos-dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/qcom,dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/fsl,imx8mp-dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/ti,keystone-dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/intel,keembay-dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: ignoring, error parsing file make: *** [Makefile:1401: dt_binding_check] Error 2 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 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
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 + + 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>; + }; + + }; + }; ...
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(+)