Re: [PATCH v4 1/2] dt-bindings: usb: qcom,dwc3: Convert USB DWC3 bindings

From: Sandeep Maheswaram (Temp)
Date: Tue Mar 24 2020 - 00:52:11 EST

Hi Stephen,

Can you check my reply to the review comments and let me know how to proceeed.

Thanks

Sandeep

On 3/13/2020 5:41 PM, Sandeep Maheswaram (Temp) wrote:
Hi Stephen,

On 2/11/2020 3:01 AM, Stephen Boyd wrote:
Quoting Sandeep Maheswaram (2020-02-09 22:36:44)
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
new file mode 100644
index 0000000..0353401
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -0,0 +1,155 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SuperSpeed DWC3 USB SoC controller
+
+maintainers:
+Â - Manu Gautam <mgautam@xxxxxxxxxxxxxx>
+
+properties:
+Â compatible:
+ÂÂÂ items:
+ÂÂÂÂÂ - enum:
+ÂÂÂÂÂÂÂÂÂ - qcom,msm8996-dwc3
+ÂÂÂÂÂÂÂÂÂ - qcom,msm8998-dwc3
+ÂÂÂÂÂÂÂÂÂ - qcom,sdm845-dwc3
+ÂÂÂÂÂ - const: qcom,dwc3
+
+Â reg:
+ÂÂÂ description: Offset and length of register set for QSCRATCH wrapper
+ÂÂÂ maxItems: 1
+
+Â "#address-cells":
+ÂÂÂ enum: [ 1, 2 ]
+
+Â "#size-cells":
+ÂÂÂ enum: [ 1, 2 ]
+
+Â power-domains:
+ÂÂÂ description: specifies a phandle to PM domain provider node
+ÂÂÂ maxItems: 1
+
+Â clocks:
+ÂÂÂ description:
+ÂÂÂÂÂ A list of phandle and clock-specifier pairs for the clocks
+ÂÂÂÂÂ listed in clock-names.
+ÂÂÂ items:
+ÂÂÂÂÂ - description: System Config NOC clock.
+ÂÂÂÂÂ - description: Master/Core clock, has to be >= 125 MHz
+ÂÂÂÂÂÂÂÂÂ for SS operation and >= 60MHz for HS operation.
+ÂÂÂÂÂ - description: System bus AXI clock.
+ÂÂÂÂÂ - description: Mock utmi clock needed for ITP/SOF generation
+ÂÂÂÂÂÂÂÂÂ in host mode.Its frequency should be 19.2MHz.
Please add a space between the end of sentence and next one.
will do in next version

+ÂÂÂÂÂ - description: Sleep clock, used for wakeup when
+ÂÂÂÂÂÂÂÂÂ USB3 core goes into low power mode (U3).
+
+Â clock-names:
+ÂÂÂ items:
+ÂÂÂÂÂ - const: cfg_noc
+ÂÂÂÂÂ - const: core
+ÂÂÂÂÂ - const: iface
+ÂÂÂÂÂ - const: mock_utmi
+ÂÂÂÂÂ - const: sleep
+
+Â assigned-clocks:
+ÂÂÂ items:
+ÂÂÂÂÂ - description: Phandle to MOCK_UTMI_CLK.
+ÂÂÂÂÂ - description: Phandle to MASTER_CLK.
It's a phandle and clock specifier pair, not always just a phandle.
Maybe the base schema can enforce that somehow, but the description
isn't accurate.
will do in next version

+
+Â assigned-clock-rates:
+ÂÂÂ items:
+ÂÂÂÂÂ - description: Must be 19.2MHz (19200000).
+ÂÂÂÂÂ - description: Must be >= 60 MHz in HS mode, >= 125 MHz in SS mode.
Can this be more strict? I see in [1] that it was suggested to update
the schema checker. Did you try that?

Tried that but need to add maximum value also and even after that getting some errors as below.

/Documentation/devicetree/bindings/usb/qcom,dwc3.example.dt.yaml: usb@a6f8800: assigned-clock-rates: Additional items are not allowed ([150000000] was unexpected)

/local/mnt/workspace/sandeep/bu_build/src/third_party/kernel/linux-next/Documentation/devicetree/bindings/usb/qcom,dwc3.example.dt.yaml: usb@a6f8800: assigned-clock-rates:0: [19200000] is too short

+
+Â resets:
+ÂÂÂ maxItems: 1
+
+Â interrupts:
+ÂÂÂ items:
+ÂÂÂÂÂ - description: The interrupt that is asserted
+ÂÂÂÂÂÂÂÂÂ when a wakeup event is received on USB2 bus.
+ÂÂÂÂÂ - description: The interrupt that is asserted
+ÂÂÂÂÂÂÂÂÂ when a wakeup event is received on USB3 bus.
+ÂÂÂÂÂ - description: Wakeup event on DM line.
+ÂÂÂÂÂ - description: Wakeup event on DP line.
+
+Â interrupt-names:
+ÂÂÂ items:
+ÂÂÂÂÂ - const: hs_phy_irq
+ÂÂÂÂÂ - const: ss_phy_irq
+ÂÂÂÂÂ - const: dm_hs_phy_irq
+ÂÂÂÂÂ - const: dp_hs_phy_irq
+
+Â qcom,select-utmi-as-pipe-clk:
+ÂÂÂ description:
+ÂÂÂÂÂ If present, disable USB3 pipe_clk requirement.
+ÂÂÂÂÂ Used when dwc3 operates without SSPHY and only
+ÂÂÂÂÂ HS/FS/LS modes are supported.
+ÂÂÂ type: boolean
+
+# Required child node:
+
+patternProperties:
+Â "^dwc3@[0-9a-f]+$":
+ÂÂÂ type: object
+ÂÂÂ description:
+ÂÂÂÂÂ A child node must exist to represent the core DWC3 IP block
+ÂÂÂÂÂ The content of the node is defined in dwc3.txt.
+
+required:
+Â - compatible
+Â - reg
+Â - "#address-cells"
+Â - "#size-cells"
+Â - power-domains
+Â - clocks
+Â - clock-names
Why aren't interrupts required? They're always present, aren't they?
In qcom,dwc3.txt file interrupts are mentioned in Optional properties and I also didnt find any interrupts in 8996.dtsi

+
+examples:
+Â - |
+ÂÂÂ #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+ÂÂÂ #include <dt-bindings/interrupt-controller/arm-gic.h>
It would be good to include <dt-bindings/interrupt-controller/irq.h>
here too, just in case someone wants to move that include out of
arm-gic.h, which is possible.

+ÂÂÂ usb_1: usb@a6f8800 {
Can we drop the phandle? It's not used.
will do in next version

+ÂÂÂÂÂÂÂ compatible = "qcom,sdm845-dwc3", "qcom,dwc3";
+ÂÂÂÂÂÂÂ reg = <0 0x0a6f8800 0 0x400>;
+
+ÂÂÂÂÂÂÂ #address-cells = <2>;
+ÂÂÂÂÂÂÂ #size-cells = <2>;
+
+ÂÂÂÂÂÂÂ clocks = <&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <&gcc GCC_USB30_PRIM_MASTER_CLK>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <&gcc GCC_USB30_PRIM_SLEEP_CLK>;
+ÂÂÂÂÂÂÂ clock-names = "cfg_noc", "core", "iface", "mock_utmi",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "sleep";
Spacing looks off. Are there tabs?
will correct in next version

+
+ÂÂÂÂÂÂÂ assigned-clocks = <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <&gcc GCC_USB30_PRIM_MASTER_CLK>;
+ÂÂÂÂÂÂÂ assigned-clock-rates = <19200000>, <150000000>;
+
+ÂÂÂÂÂÂÂ interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
+ÂÂÂÂÂÂÂ interrupt-names = "hs_phy_irq", "ss_phy_irq",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "dm_hs_phy_irq", "dp_hs_phy_irq";
Same spacing nit
will correct in next version

+
+ÂÂÂÂÂÂÂÂÂÂÂ power-domains = <&gcc USB30_PRIM_GDSC>;
+
+ÂÂÂÂÂÂÂÂÂÂÂ resets = <&gcc GCC_USB30_PRIM_BCR>;
+
+ÂÂÂÂÂÂÂÂÂÂÂ usb_1_dwc3: dwc3@a600000 {
Drop this phandle too? It isn't used.
will correct in next version

+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ compatible = "snps,dwc3";
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ reg = <0 0x0a600000 0 0xcd00>;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ iommus = <&apps_smmu 0x740 0>;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ snps,dis_u2_susphy_quirk;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ snps,dis_enblslpm_quirk;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ phy-names = "usb2-phy", "usb3-phy";
+ÂÂÂÂÂÂÂÂÂÂÂ };
[1] https://lkml.kernel.org/r/20191218221310.GA4624@bogus

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation