| Message ID | 1b48e737aa14f5b5539cbf04d473182121d5b1ad.1686126439.git.quic_varada@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable IPQ5332 USB2 | expand |
On 07/06/2023 13:56, Varadarajan Narayanan wrote: > Add USB phy and controller nodes > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > 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>, > + <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 = "okay"; > + }; > + > + usb2: usb2@8a00000 { > + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + 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 indent these values. > + > + clock-names = "core", > + "iface", > + "sleep", > + "mock_utmi"; Please indent these values. > + > + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; No PHY IRQs? > + 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>; > + }; > + }; > }; > > timer {
On 7.06.2023 12:56, Varadarajan Narayanan wrote: > Add USB phy and controller nodes > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > 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 > + <0x08af8800 0x400>; > + reg-names = "m31usb_phy_base", > + "qscratch_base"; > + phy_type= "utmi"; Missing space before '=' > + > + 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. > + }; > + > + 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. > + > + 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. > + clock-names = "core", > + "iface", > + "sleep", > + "mock_utmi"; Please align this, and all other similar lists. > + > + 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. > + 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? Konrad > + }; > + }; > }; > > timer {
On 07/06/2023 12:56, Varadarajan Narayanan wrote: > Add USB phy and controller nodes > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > 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 { 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 > + compatible = "qcom,ipq5332-m31-usb-hsphy"; > + reg = <0x0007b000 0x12C>, > + <0x08af8800 0x400>; Lowercase hex only. > + reg-names = "m31usb_phy_base", > + "qscratch_base"; > + phy_type= "utmi"; > + > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > + reset-names = "usb2_phy_reset"; > + > + status = "okay"; It's by default. Drop. > + }; > + > + usb2: usb2@8a00000 { It does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). 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 > + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + reg = <0x08af8800 0x100>; reg is always after compatible. Ranges is third. Then you will spot that address is wrong. > + > + clocks = <&gcc GCC_USB0_MASTER_CLK>, > + <&gcc GCC_SNOC_USB_CLK>, > + <&gcc GCC_USB0_SLEEP_CLK>, > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; Fix alignment. > + > + clock-names = "core", > + "iface", > + "sleep", > + "mock_utmi"; Fix alignment. > + > + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "pwr_event"; > + Best regards, Krzysztof
On Wed, Jun 07, 2023 at 02:23:20PM +0300, Dmitry Baryshkov wrote: > On 07/06/2023 13:56, Varadarajan Narayanan wrote: > >Add USB phy and controller nodes > > > >Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > >--- > > 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>, > >+ <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 = "okay"; > >+ }; > >+ > >+ usb2: usb2@8a00000 { > >+ compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > >+ #address-cells = <1>; > >+ #size-cells = <1>; > >+ ranges; > >+ > >+ 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 indent these values. Ok. > >+ > >+ clock-names = "core", > >+ "iface", > >+ "sleep", > >+ "mock_utmi"; > > Please indent these values. Ok. > >+ > >+ interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; > > No PHY IRQs? Will add. Thanks Varada > >+ 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>; > >+ }; > >+ }; > > }; > > timer { > > -- > With best wishes > Dmitry >
On Wed, Jun 07, 2023 at 08:24:04PM +0200, Konrad Dybcio wrote: > > > On 7.06.2023 12:56, Varadarajan Narayanan wrote: > > Add USB phy and controller nodes > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > 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. > > + <0x08af8800 0x400>; > > + reg-names = "m31usb_phy_base", > > + "qscratch_base"; > > + phy_type= "utmi"; > Missing space before '=' Ok. > > + > > + 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. > > + }; > > + > > + 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. > > + > > + 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. > > > + clock-names = "core", > > + "iface", > > + "sleep", > > + "mock_utmi"; > Please align this, and all other similar lists. Ok. > > + > > + 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. > > + 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 > > + }; > > + }; > > }; > > > > timer {
On Wed, Jun 07, 2023 at 08:35:09PM +0200, Krzysztof Kozlowski wrote: > On 07/06/2023 12:56, Varadarajan Narayanan wrote: > > Add USB phy and controller nodes > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > 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 { > > 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 Ok. > > + compatible = "qcom,ipq5332-m31-usb-hsphy"; > > + reg = <0x0007b000 0x12C>, > > + <0x08af8800 0x400>; > > Lowercase hex only. Ok. > > + reg-names = "m31usb_phy_base", > > + "qscratch_base"; > > + phy_type= "utmi"; > > + > > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > > + reset-names = "usb2_phy_reset"; > > + > > + status = "okay"; > > It's by default. Drop. Ok. > > + }; > > + > > + usb2: usb2@8a00000 { > > It does not look like you tested the DTS against bindings. Please run > `make dtbs_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > > 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 Ok. > > + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + > > + reg = <0x08af8800 0x100>; > > reg is always after compatible. Ranges is third. Then you will spot that > address is wrong. Ok. > > + > > + clocks = <&gcc GCC_USB0_MASTER_CLK>, > > + <&gcc GCC_SNOC_USB_CLK>, > > + <&gcc GCC_USB0_SLEEP_CLK>, > > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; > > Fix alignment. Ok. > > + > > + clock-names = "core", > > + "iface", > > + "sleep", > > + "mock_utmi"; > > Fix alignment. Ok. > > + > > + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "pwr_event"; > > + Thanks Varada > Best regards, > Krzysztof >
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>, + <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 = "okay"; + }; + + usb2: usb2@8a00000 { + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + 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>; + + clock-names = "core", + "iface", + "sleep", + "mock_utmi"; + + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; + 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>; + }; + }; }; timer {
Add USB phy and controller nodes Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> --- arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)