On Wed, Jun 07, 2023 at 08:24:04PM +0200, Konrad Dybcio wrote:

On 7.06.2023 12:56, Varadarajan Narayanan wrote:

quoted

Add USB phy and controller nodes

Signed-off-by: Varadarajan Narayanan <redacted>
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index c2d6cc65..3183357 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi

@@ -383,6 +383,61 @@
 				status = "disabled";
 			};
 		};
+
+		usb_0_m31phy: hs_m31phy@7b000 {
+			compatible = "qcom,ipq5332-m31-usb-hsphy";
+			reg = <0x0007b000 0x12C>,

random uppercase hex

Ok.

quoted

+			      <0x08af8800 0x400>;
+			reg-names = "m31usb_phy_base",
+				    "qscratch_base";
+			phy_type= "utmi";

Missing space before '='

Ok.

quoted

+
+			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
+			reset-names = "usb2_phy_reset";
+
+			status = "okay";

If you're only defining the node, it's enabled by default

In this case, you'd probably want to disable it by default.

Ok.

quoted

+		};
+
+		usb2: usb2@8a00000 {
+			compatible = "qcom,ipq5332-dwc3", "qcom,dwc3";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;

Please push these 3 properties to the end

And add status = "disabled" below them.

Ok.

quoted

+
+			reg = <0x08af8800 0x100>;
+
+			clocks = <&gcc GCC_USB0_MASTER_CLK>,
+				<&gcc GCC_SNOC_USB_CLK>,
+				<&gcc GCC_USB0_SLEEP_CLK>,
+				<&gcc GCC_USB0_MOCK_UTMI_CLK>;
+

Please remove this newline.

quoted

+			clock-names = "core",
+				"iface",
+				"sleep",
+				"mock_utmi";

Please align this, and all other similar lists.

Ok.

quoted

+
+			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;

interrupts-extended is unnecessary with just a single interrupt
source.. you can make it `interrupts` and drop the GIC reference.

It would also be nice to push the interrupt properties below 'reg'.
We're working on documenting and automating checking the preferred
property order.

Ok.

quoted

+			interrupt-names = "pwr_event";
+
+			resets = <&gcc GCC_USB_BCR>;
+
+			qcom,select-utmi-as-pipe-clk;
+
+			usb2_0_dwc: usb@8a00000 {
+				compatible = "snps,dwc3";
+				reg = <0x08a00000 0xe000>;
+				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
+				clock-names = "ref";
+				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+				usb-phy = <&usb_0_m31phy>;
+				tx-fifo-resize;
+				snps,is-utmi-l1-suspend;
+				snps,hird-threshold = /bits/ 8 <0x0>;
+				snps,dis_u2_susphy_quirk;
+				snps,dis_u3_susphy_quirk;
+				snps,ref-clock-period-ns = <21>;

1/21 is 0.0476..  that doesn't seem to correspond to the ref
clk frequency?

dwc3_ref_clk_period() prefers ref clock's rate over ref-clock-period-ns.
Since ref clock is available this is not used. Will remove this.

Thanks
Varada

Konrad

quoted

+			};
+		};
 	};

 	timer {

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy