| Message ID | 20230929084209.3033093-1-quic_ipkumar@quicinc.com |
|---|---|
| Headers | show |
| Series | Enable USB3 for Qualcomm IPQ5332 | expand |
On 29/09/2023 11:42, Praveenkumar I wrote: > Add USB Super-Speed UNIPHY node and populate the phandle on > gcc node for the parent clock map. > > Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > index d3fef2f80a81..b08ffd8c094e 100644 > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > @@ -158,6 +158,29 @@ usbphy0: phy@7b000 { > status = "disabled"; > }; > > + usbphy1: phy@4b0000 { Are there other USB PHYs on this platform? > + compatible = "qcom,ipq5332-usb-uniphy"; > + reg = <0x4b0000 0x800>; > + > + clocks = <&gcc GCC_PCIE3X1_PHY_AHB_CLK>, > + <&gcc GCC_USB0_PHY_CFG_AHB_CLK>, > + <&gcc GCC_USB0_PIPE_CLK>; > + clock-names = "ahb", > + "cfg_ahb", > + "pipe"; > + > + resets = <&gcc GCC_USB0_PHY_BCR>; > + > + #clock-cells = <0>; > + clock-output-names = "usb0_pipe_clk_src"; I'm not sure, what is the best approach her. For QMP USB and PCIe PHYs we had to use fixed names historically. On the other hand for QMP DP clocks we are fine with the generated names. I'd prefer the latter case. > + > + qcom,phy-usb-mux-sel = <&tcsr 0x10540>; > + > + #phy-cells = <0>; > + > + status = "disabled"; > + }; > + > qfprom: efuse@a4000 { > compatible = "qcom,ipq5332-qfprom", "qcom,qfprom"; > reg = <0x000a4000 0x721>; > @@ -200,7 +223,7 @@ gcc: clock-controller@1800000 { > <&sleep_clk>, > <0>, > <0>, > - <0>; > + <&usbphy1>; > }; > > tcsr_mutex: hwlock@1905000 {
On 29/09/2023 11:42, Praveenkumar I wrote: > Add aux and lfps clocks in USB node for Super-Speed support. > > Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > index b08ffd8c094e..1813b9fa4bb5 100644 > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > @@ -336,11 +336,16 @@ usb: usb@8af8800 { > clocks = <&gcc GCC_USB0_MASTER_CLK>, > <&gcc GCC_SNOC_USB_CLK>, > <&gcc GCC_USB0_SLEEP_CLK>, > - <&gcc GCC_USB0_MOCK_UTMI_CLK>; > + <&gcc GCC_USB0_MOCK_UTMI_CLK>, > + <&gcc GCC_USB0_AUX_CLK>, > + <&gcc GCC_USB0_LFPS_CLK>; This looks like a strange change. Usually the DTB is considered to be the ABI, so older DTBs should continue to work with newer kernels. Is there a reason why the AUX and LFPS clocks were not a part of the original submission? > + > clock-names = "core", > "iface", > "sleep", > - "mock_utmi"; > + "mock_utmi", > + "aux", > + "lfps"; > > resets = <&gcc GCC_USB_BCR>; >
On 29/09/2023 16:31, Praveenkumar I wrote: > > > On 9/29/2023 6:44 PM, Konrad Dybcio wrote: >> On 29.09.2023 10:42, Praveenkumar I wrote: >>> Add UNIPHY node in USB to support Super-speed. As the SS PHY has >>> pipe clock, removed "qcom,select-utmi-as-pipe-clk" flag. >>> >>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> >>> --- >> Patches 6 and 7 should be swapped, otherwise you may get no >> USB with this commit. Incremental patches must not break >> functionality, unless it is truly inevitable. > Understood. Will swap the 6 and 7 patches in the update. But just swapping the patches will not work, the patch for the board file will break compilation. I think you have to squash them. > > -- > Thanks, > Praveenkumar >> >> Konrad >
On 29/09/2023 11:42, Praveenkumar I wrote: > Add UNIPHY node in USB to support Super-speed. As the SS PHY has > pipe clock, removed "qcom,select-utmi-as-pipe-clk" flag. > > Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > index 1813b9fa4bb5..8fe4e45bfc18 100644 > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > @@ -349,8 +349,6 @@ usb: usb@8af8800 { > > resets = <&gcc GCC_USB_BCR>; > > - qcom,select-utmi-as-pipe-clk; > - > #address-cells = <1>; > #size-cells = <1>; > ranges; > @@ -363,8 +361,8 @@ usb_dwc: usb@8a00000 { > clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; > clock-names = "ref"; > interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; > - phy-names = "usb2-phy"; > - phys = <&usbphy0>; > + phy-names = "usb2-phy", "usb3-phy"; > + phys = <&usbphy0>, <&usbphy1>; Ah, I see now. Maybe usbphy_ss_0 or something like that would be a better label for this PHY. I'd expect usbphy1 to be used for other host than usbphy0. > tx-fifo-resize; > snps,is-utmi-l1-suspend; > snps,hird-threshold = /bits/ 8 <0x0>;
On 9/30/2023 10:56 PM, Dmitry Baryshkov wrote: > On 29/09/2023 16:31, Praveenkumar I wrote: >> >> >> On 9/29/2023 6:44 PM, Konrad Dybcio wrote: >>> On 29.09.2023 10:42, Praveenkumar I wrote: >>>> Add UNIPHY node in USB to support Super-speed. As the SS PHY has >>>> pipe clock, removed "qcom,select-utmi-as-pipe-clk" flag. >>>> >>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> >>>> --- >>> Patches 6 and 7 should be swapped, otherwise you may get no >>> USB with this commit. Incremental patches must not break >>> functionality, unless it is truly inevitable. >> Understood. Will swap the 6 and 7 patches in the update. > > But just swapping the patches will not work, the patch for the board > file will break compilation. I think you have to squash them. I think swapping will work as the PHY node in the base dtsi added separately in patch 3. If compilation fails, will squash them. - Praveenkumar > >> >> -- >> Thanks, >> Praveenkumar >>> >>> Konrad >> >
On 9/30/2023 8:26 PM, Krzysztof Kozlowski wrote: > On 29/09/2023 10:42, Praveenkumar I wrote: >> Document the Qualcomm USB3 22ull UNIPHY present in IPQ5332. >> >> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> >> --- >> .../bindings/phy/qcom,ipq5332-usb-uniphy.yaml | 83 +++++++++++++++++++ >> 1 file changed, 83 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/phy/qcom,ipq5332-usb-uniphy.yaml > Filename should match compatible. Will address it. > >> diff --git a/Documentation/devicetree/bindings/phy/qcom,ipq5332-usb-uniphy.yaml b/Documentation/devicetree/bindings/phy/qcom,ipq5332-usb-uniphy.yaml >> new file mode 100644 >> index 000000000000..90434cee9cdd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/phy/qcom,ipq5332-usb-uniphy.yaml >> @@ -0,0 +1,83 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/phy/qcom,ipq5332-usb-uniphy.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm USB Super-Speed UNIPHY >> + >> +maintainers: >> + - Praveenkumar I <quic_ipkumar@quicinc.com> >> + - Varadarajan Narayanan <quic_varada@quicinc.com> >> + >> +description: >> + USB Super-Speed UNIPHY found in Qualcomm IPQ5332, IPQ5018 SoCs. >> + >> +properties: >> + compatible: >> + items: > Drop items, not needed. Sure, will drop. > >> + - const: qcom,ipq5332-usb-ssphy >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 3 >> + >> + clock-names: >> + items: >> + - const: ahb >> + - const: cfg_ahb >> + - const: pipe >> + >> + resets: >> + maxItems: 1 >> + >> + vdd-supply: >> + description: >> + Phandle to 5V regulator supply to PHY digital circuit. >> + >> + qcom,phy-usb-mux-sel: >> + description: PHY Mux Selection for USB >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + items: >> + - items: >> + - description: phandle of TCSR syscon >> + - description: offset of PHY Mux selection register >> + >> + "#clock-cells": >> + const: 0 >> + >> + clock-output-names: >> + maxItems: 1 >> + >> + "#phy-cells": >> + const: 0 > You miss required: block. Sure, will add. -- Thanks, Praveenkumar > > Best regards, > Krzysztof >
On 9/30/2023 10:52 PM, Dmitry Baryshkov wrote: > On 29/09/2023 11:42, Praveenkumar I wrote: >> Add USB Super-Speed UNIPHY node and populate the phandle on >> gcc node for the parent clock map. >> >> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 25 ++++++++++++++++++++++++- >> 1 file changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> index d3fef2f80a81..b08ffd8c094e 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> @@ -158,6 +158,29 @@ usbphy0: phy@7b000 { >> status = "disabled"; >> }; >> + usbphy1: phy@4b0000 { > > Are there other USB PHYs on this platform? No. Only two USB PHYs. > >> + compatible = "qcom,ipq5332-usb-uniphy"; >> + reg = <0x4b0000 0x800>; >> + >> + clocks = <&gcc GCC_PCIE3X1_PHY_AHB_CLK>, >> + <&gcc GCC_USB0_PHY_CFG_AHB_CLK>, >> + <&gcc GCC_USB0_PIPE_CLK>; >> + clock-names = "ahb", >> + "cfg_ahb", >> + "pipe"; >> + >> + resets = <&gcc GCC_USB0_PHY_BCR>; >> + >> + #clock-cells = <0>; >> + clock-output-names = "usb0_pipe_clk_src"; > > I'm not sure, what is the best approach her. For QMP USB and PCIe PHYs > we had to use fixed names historically. On the other hand for QMP DP > clocks we are fine with the generated names. I'd prefer the latter case. Sure, will use the generated names. > >> + >> + qcom,phy-usb-mux-sel = <&tcsr 0x10540>; >> + >> + #phy-cells = <0>; >> + >> + status = "disabled"; >> + }; >> + >> qfprom: efuse@a4000 { >> compatible = "qcom,ipq5332-qfprom", "qcom,qfprom"; >> reg = <0x000a4000 0x721>; >> @@ -200,7 +223,7 @@ gcc: clock-controller@1800000 { >> <&sleep_clk>, >> <0>, >> <0>, >> - <0>; >> + <&usbphy1>; >> }; >> tcsr_mutex: hwlock@1905000 { > -- Thanks, Praveenkumar
On 9/30/2023 10:57 PM, Dmitry Baryshkov wrote: > On 29/09/2023 11:42, Praveenkumar I wrote: >> Add UNIPHY node in USB to support Super-speed. As the SS PHY has >> pipe clock, removed "qcom,select-utmi-as-pipe-clk" flag. >> >> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> index 1813b9fa4bb5..8fe4e45bfc18 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> @@ -349,8 +349,6 @@ usb: usb@8af8800 { >> resets = <&gcc GCC_USB_BCR>; >> - qcom,select-utmi-as-pipe-clk; >> - >> #address-cells = <1>; >> #size-cells = <1>; >> ranges; >> @@ -363,8 +361,8 @@ usb_dwc: usb@8a00000 { >> clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; >> clock-names = "ref"; >> interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; >> - phy-names = "usb2-phy"; >> - phys = <&usbphy0>; >> + phy-names = "usb2-phy", "usb3-phy"; >> + phys = <&usbphy0>, <&usbphy1>; > > Ah, I see now. Maybe usbphy_ss_0 or something like that would be a > better label for this PHY. I'd expect usbphy1 to be used for other > host than usbphy0. Sure, will change it. > >> tx-fifo-resize; >> snps,is-utmi-l1-suspend; >> snps,hird-threshold = /bits/ 8 <0x0>; > -- Thanks, Praveenkumar
On 9/30/2023 10:55 PM, Dmitry Baryshkov wrote: > On 29/09/2023 11:42, Praveenkumar I wrote: >> Add aux and lfps clocks in USB node for Super-Speed support. >> >> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> index b08ffd8c094e..1813b9fa4bb5 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> @@ -336,11 +336,16 @@ usb: usb@8af8800 { >> clocks = <&gcc GCC_USB0_MASTER_CLK>, >> <&gcc GCC_SNOC_USB_CLK>, >> <&gcc GCC_USB0_SLEEP_CLK>, >> - <&gcc GCC_USB0_MOCK_UTMI_CLK>; >> + <&gcc GCC_USB0_MOCK_UTMI_CLK>, >> + <&gcc GCC_USB0_AUX_CLK>, >> + <&gcc GCC_USB0_LFPS_CLK>; > > This looks like a strange change. Usually the DTB is considered to be > the ABI, so older DTBs should continue to work with newer kernels. Is > there a reason why the AUX and LFPS clocks were not a part of the > original submission? This AUX and LFPS clocks are required only when USB controller uses the UNIPHY and works in 3.0. Original change added 2.0 support and used m31-phy. > >> + >> clock-names = "core", >> "iface", >> "sleep", >> - "mock_utmi"; >> + "mock_utmi", >> + "aux", >> + "lfps"; >> resets = <&gcc GCC_USB_BCR>; > -- Thanks, Praveenkumar
On 9/30/2023 10:55 PM, Dmitry Baryshkov wrote: > On 29/09/2023 11:42, Praveenkumar I wrote: >> Add aux and lfps clocks in USB node for Super-Speed support. >> >> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/ipq5332.dtsi | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> index b08ffd8c094e..1813b9fa4bb5 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi >> @@ -336,11 +336,16 @@ usb: usb@8af8800 { >> clocks = <&gcc GCC_USB0_MASTER_CLK>, >> <&gcc GCC_SNOC_USB_CLK>, >> <&gcc GCC_USB0_SLEEP_CLK>, >> - <&gcc GCC_USB0_MOCK_UTMI_CLK>; >> + <&gcc GCC_USB0_MOCK_UTMI_CLK>, >> + <&gcc GCC_USB0_AUX_CLK>, >> + <&gcc GCC_USB0_LFPS_CLK>; > > This looks like a strange change. Usually the DTB is considered to be > the ABI, so older DTBs should continue to work with newer kernels. Is > there a reason why the AUX and LFPS clocks were not a part of the > original submission? This AUX and LFPS clocks are required only when USB controller uses the UNIPHY and works in 3.0. Original change added 2.0 support and used m31-phy. > >> + >> clock-names = "core", >> "iface", >> "sleep", >> - "mock_utmi"; >> + "mock_utmi", >> + "aux", >> + "lfps"; >> resets = <&gcc GCC_USB_BCR>; > -- Thanks, Praveenkumar