| Message ID | 14f60578e2935c0844537eab162af3afa52ffe39.1686126439.git.quic_varada@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Enable IPQ5332 USB2 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Wed, 07 Jun 2023 16:26:06 +0530, Varadarajan Narayanan wrote: > Document the M31 USB2 phy present in IPQ5332 > > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > .../devicetree/bindings/phy/qcom,m31.yaml | 69 ++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml > 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: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/qcom,m31.example.dtb: hs_m31phy@5b00: status:0: 'ok' is not one of ['okay', 'disabled', 'reserved'] From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/dt-core.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/14f60578e2935c0844537eab162af3afa52ffe39.1686126439.git.quic_varada@quicinc.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. 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 after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 07/06/2023 12:56, Varadarajan Narayanan wrote: > Document the M31 USB2 phy present in IPQ5332 Full stop. > > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > .../devicetree/bindings/phy/qcom,m31.yaml | 69 ++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml > > diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml > new file mode 100644 > index 0000000..8ad4ba4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml > @@ -0,0 +1,69 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > + Drop stray blank lines. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm M31 USB PHY What is M31? > + > +maintainers: > + - Sricharan Ramabadhran <quic_srichara@quicinc.com> > + - Varadarajan Narayanan <quic_varada@quicinc.org> > + > +description: > + This file contains documentation for the USB M31 PHY found in Qualcomm Drop redundant parts ("This file contains documentation for the"). > + IPQ5018, IPQ5332 SoCs. > + > +properties: > + compatible: > + oneOf: That's not oneOf. > + - items: No items. > + - enum: > + - qcom,m31-usb-hsphy I am confused what's this. If m31 is coming from some IP block provider, then you are using wrong vendor prefix. https://www.m31tech.com/download_file/M31_USB.pdf > + - qcom,ipq5332-m31-usb-hsphy This confuses me even more. IPQ m31? > + > + reg: > + description: > + Offset and length of the M31 PHY register set Drop description, obvious. > + maxItems: 2 > + > + reg-names: > + items: > + - const: m31usb_phy_base > + - const: qscratch_base Drop "_base" from both. > + > + phy_type: > + oneOf: > + - items: > + - enum: > + - utmi > + - ulpi This does not belong to phy, but to USB node. > + > + resets: > + maxItems: 1 > + description: > + List of phandles and reset pairs, one for each entry in reset-names. Drop useless description. > + > + reset-names: > + items: > + - const: > + usb2_phy_reset Drop entire reset-names. > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> > + hs_m31phy_0: hs_m31phy@5b00 { Node names should be generic. See also explanation and list of examples in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Also, no underscores in node names. > + compatible = "qcom,m31-usb-hsphy"; > + reg = <0x5b000 0x3000>, This is not what your unit address is saying. > + <0x08af8800 0x400>; Align it. > + reg-names = "m31usb_phy_base", > + "qscratch_base"; Align it. > + phy_type = "utmi"; > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > + reset-names = "usb2_phy_reset"; > + > + status = "ok"; Drop. > + }; Best regards, Krzysztof
On Wed, Jun 07, 2023 at 08:31:50PM +0200, Krzysztof Kozlowski wrote: > On 07/06/2023 12:56, Varadarajan Narayanan wrote: > > Document the M31 USB2 phy present in IPQ5332 > > Full stop. Ok. > > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > .../devicetree/bindings/phy/qcom,m31.yaml | 69 ++++++++++++++++++++++ > > 1 file changed, 69 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml > > > > diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml > > new file mode 100644 > > index 0000000..8ad4ba4 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml > > @@ -0,0 +1,69 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > + > > Drop stray blank lines. Ok. > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Qualcomm M31 USB PHY > > What is M31? M31 is an external IP integrated into IPQ5332 SoC. > > + > > +maintainers: > > + - Sricharan Ramabadhran <quic_srichara@quicinc.com> > > + - Varadarajan Narayanan <quic_varada@quicinc.org> > > + > > +description: > > + This file contains documentation for the USB M31 PHY found in Qualcomm > > Drop redundant parts ("This file contains documentation for the"). Ok. > > + IPQ5018, IPQ5332 SoCs. > > + > > +properties: > > + compatible: > > + oneOf: > > That's not oneOf. Ok. > > + - items: > > No items. Ok. > > + - enum: > > + - qcom,m31-usb-hsphy > > I am confused what's this. If m31 is coming from some IP block provider, > then you are using wrong vendor prefix. > https://www.m31tech.com/download_file/M31_USB.pdf > > > > + - qcom,ipq5332-m31-usb-hsphy > > This confuses me even more. IPQ m31? Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively. Will that be acceptable? > > + > > + reg: > > + description: > > + Offset and length of the M31 PHY register set > > Drop description, obvious. Ok. > > + maxItems: 2 > > + > > + reg-names: > > + items: > > + - const: m31usb_phy_base > > + - const: qscratch_base > > Drop "_base" from both. Ok. Will drop qscratch_base. This is in the controller space. Should not come here. > > + > > + phy_type: > > + oneOf: > > + - items: > > + - enum: > > + - utmi > > + - ulpi > > This does not belong to phy, but to USB node. This is used by the driver to set a bit during phy init. Hence have it as a replication of the USB node's entry. If this is not permissible, is there some way to get this from the USB node, or any other alternative mechanism? > > + > > + resets: > > + maxItems: 1 > > + description: > > + List of phandles and reset pairs, one for each entry in reset-names. > > Drop useless description. Ok. > > + > > + reset-names: > > + items: > > + - const: > > + usb2_phy_reset > > Drop entire reset-names. Ok. > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> > > + hs_m31phy_0: hs_m31phy@5b00 { > > Node names should be generic. See also explanation and list of examples > in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > Also, no underscores in node names. Will change this as usbphy0:hs_m31phy@7b000 > > + compatible = "qcom,m31-usb-hsphy"; > > + reg = <0x5b000 0x3000>, > > This is not what your unit address is saying. Will change to 0x0007b000. > > + <0x08af8800 0x400>; > > Align it. Will drop this. This is in the controller space. > > + reg-names = "m31usb_phy_base", > > + "qscratch_base"; > > Align it. Ok. > > + phy_type = "utmi"; > > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > > + reset-names = "usb2_phy_reset"; > > + > > + status = "ok"; > > Drop. Ok. Hopefully will get an alternative for the phy_type. Thanks Varada > > + }; > > Best regards, > Krzysztof >
On 15/06/2023 07:27, Varadarajan Narayanan wrote: >>> + - enum: >>> + - qcom,m31-usb-hsphy >> >> I am confused what's this. If m31 is coming from some IP block provider, >> then you are using wrong vendor prefix. >> https://www.m31tech.com/download_file/M31_USB.pdf >> >> >>> + - qcom,ipq5332-m31-usb-hsphy >> >> This confuses me even more. IPQ m31? > > Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively. > Will that be acceptable? m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31 device have some name/version/model? If it is not really known, then I would just propose to go with qcom,ipq5332-usb-hsphy. Skip generic compatible ("usb-hsphy") entirely. And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create something similar with difference in the hyphen. Just use device specific compatible thus device specific filename. > >>> + >>> + reg: >>> + description: >>> + Offset and length of the M31 PHY register set >> >> Drop description, obvious. > > Ok. > >>> + maxItems: 2 >>> + >>> + reg-names: >>> + items: >>> + - const: m31usb_phy_base >>> + - const: qscratch_base >> >> Drop "_base" from both. > > Ok. Will drop qscratch_base. This is in the controller space. > Should not come here. Then drop reg-names entirely. > >>> + >>> + phy_type: >>> + oneOf: >>> + - items: >>> + - enum: >>> + - utmi >>> + - ulpi >> >> This does not belong to phy, but to USB node. > > This is used by the driver to set a bit during phy init. Hence > have it as a replication of the USB node's entry. If this is not > permissible, is there some way to get this from the USB node, > or any other alternative mechanism? Shouldn't USB controller choose what type of PHY type it wants? > >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> >>> + hs_m31phy_0: hs_m31phy@5b00 { >> >> Node names should be generic. See also explanation and list of examples >> in DT specification: >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >> >> Also, no underscores in node names. > > Will change this as usbphy0:hs_m31phy@7b000 This does not solve my comments. I did not write "label" but "node name". Best regards, Krzysztof
On Sat, Jun 17, 2023 at 10:48:41AM +0200, Krzysztof Kozlowski wrote: > On 15/06/2023 07:27, Varadarajan Narayanan wrote: > >>> + - enum: > >>> + - qcom,m31-usb-hsphy > >> > >> I am confused what's this. If m31 is coming from some IP block provider, > >> then you are using wrong vendor prefix. > >> https://www.m31tech.com/download_file/M31_USB.pdf > >> > >> > >>> + - qcom,ipq5332-m31-usb-hsphy > >> > >> This confuses me even more. IPQ m31? > > > > Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively. > > Will that be acceptable? > > m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31 > device have some name/version/model? If it is not really known, then I > would just propose to go with qcom,ipq5332-usb-hsphy. > > Skip generic compatible ("usb-hsphy") entirely. Ok. > And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create > something similar with difference in the hyphen. Just use device > specific compatible thus device specific filename. qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the driver we are introducing is for UTMI. We would have to modify phy-qcom-usb-hs.c to accomodate M31. Will that be acceptable to phy-qcom-usb-hs.c owners/maintainers? > >>> + > >>> + reg: > >>> + description: > >>> + Offset and length of the M31 PHY register set > >> > >> Drop description, obvious. > > > > Ok. > > > >>> + maxItems: 2 > >>> + > >>> + reg-names: > >>> + items: > >>> + - const: m31usb_phy_base > >>> + - const: qscratch_base > >> > >> Drop "_base" from both. > > > > Ok. Will drop qscratch_base. This is in the controller space. > > Should not come here. > > Then drop reg-names entirely. Ok. > >>> + > >>> + phy_type: > >>> + oneOf: > >>> + - items: > >>> + - enum: > >>> + - utmi > >>> + - ulpi > >> > >> This does not belong to phy, but to USB node. > > > > This is used by the driver to set a bit during phy init. Hence > > have it as a replication of the USB node's entry. If this is not > > permissible, is there some way to get this from the USB node, > > or any other alternative mechanism? > > Shouldn't USB controller choose what type of PHY type it wants? Will remove this. IPQ5332 uses it in UTMI mode only. > >>> + > >>> +additionalProperties: false > >>> + > >>> +examples: > >>> + - | > >>> + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> > >>> + hs_m31phy_0: hs_m31phy@5b00 { > >> > >> Node names should be generic. See also explanation and list of examples > >> in DT specification: > >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > >> > >> Also, no underscores in node names. > > > > Will change this as usbphy0:hs_m31phy@7b000 > > This does not solve my comments. I did not write "label" but "node name". Sorry. will fix it. Thanks Varada
On 20/06/2023 11:32, Varadarajan Narayanan wrote: > >> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create >> something similar with difference in the hyphen. Just use device >> specific compatible thus device specific filename. > > qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the > driver we are introducing is for UTMI. We would have to > modify phy-qcom-usb-hs.c to accomodate M31. Will that be > acceptable to phy-qcom-usb-hs.c owners/maintainers? We don't talk about drivers here but bindings. Why would you need to modify the driver when introducing new binding for different device? Best regards, Krzysztof
On Wed, Jun 21, 2023 at 10:43:38AM +0200, Krzysztof Kozlowski wrote: > On 20/06/2023 11:32, Varadarajan Narayanan wrote: > > > > >> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create > >> something similar with difference in the hyphen. Just use device > >> specific compatible thus device specific filename. > > > > qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the > > driver we are introducing is for UTMI. We would have to > > modify phy-qcom-usb-hs.c to accomodate M31. Will that be > > acceptable to phy-qcom-usb-hs.c owners/maintainers? > > We don't talk about drivers here but bindings. Why would you need to > modify the driver when introducing new binding for different device? Sorry. I misunderstood your feedback as "use the existing bindings". Will name the bindings file as qcom,ipq5332-usb-hsphy.yaml and post the next version. Thanks Varada
diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml new file mode 100644 index 0000000..8ad4ba4 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml @@ -0,0 +1,69 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) + +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm M31 USB PHY + +maintainers: + - Sricharan Ramabadhran <quic_srichara@quicinc.com> + - Varadarajan Narayanan <quic_varada@quicinc.org> + +description: + This file contains documentation for the USB M31 PHY found in Qualcomm + IPQ5018, IPQ5332 SoCs. + +properties: + compatible: + oneOf: + - items: + - enum: + - qcom,m31-usb-hsphy + - qcom,ipq5332-m31-usb-hsphy + + reg: + description: + Offset and length of the M31 PHY register set + maxItems: 2 + + reg-names: + items: + - const: m31usb_phy_base + - const: qscratch_base + + phy_type: + oneOf: + - items: + - enum: + - utmi + - ulpi + + resets: + maxItems: 1 + description: + List of phandles and reset pairs, one for each entry in reset-names. + + reset-names: + items: + - const: + usb2_phy_reset + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> + hs_m31phy_0: hs_m31phy@5b00 { + compatible = "qcom,m31-usb-hsphy"; + reg = <0x5b000 0x3000>, + <0x08af8800 0x400>; + reg-names = "m31usb_phy_base", + "qscratch_base"; + phy_type = "utmi"; + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; + reset-names = "usb2_phy_reset"; + + status = "ok"; + };