[PATCH 0/3] Add DT support for Multiport controller on SC8280XP

[PATCH 0/3] Add DT support for Multiport controller on SC8280XP

[Krishna Kurapati]

Series [1] introduces binding and driver support for DWC3 Multiport
controllers. This series adds support for tertiary controller of SC8280
and enabled multiport controller for SA8295P-ADP/ SA8540P-RIDE platforms.

Till v13 the DT was pushed along with driver code. Separate the DT changes
from driver update and pushing it as this series.

[1]: https://lore.kernel.org/all/20240206051825.1038685-1-quic_kriskura@quicinc.com/

Andrew Halaney (1):
  arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb
    controller

Krishna Kurapati (2):
  arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280
  arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB
    ports

 arch/arm64/boot/dts/qcom/sa8295p-adp.dts  | 49 ++++++++++++++
 arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 ++++++
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi    | 82 +++++++++++++++++++++++
 3 files changed, 152 insertions(+)

-- 
2.34.1

[PATCH 1/3] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

[Krishna Kurapati]

Add USB and DWC3 node for tertiary port of SC8280 along with multiport
IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
platforms.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 82 ++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index febf28356ff8..29dbf2a9cdba 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3331,6 +3331,88 @@ system-cache-controller@9200000 {
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		usb_2: usb@a4f8800 {
+			compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
+			reg = <0 0x0a4f8800 0 0x400>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+
+			clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>,
+				 <&gcc GCC_USB30_MP_MASTER_CLK>,
+				 <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>,
+				 <&gcc GCC_USB30_MP_SLEEP_CLK>,
+				 <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>,
+				 <&gcc GCC_SYS_NOC_USB_AXI_CLK>;
+			clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi",
+				      "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys";
+
+			assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+					  <&gcc GCC_USB30_MP_MASTER_CLK>;
+			assigned-clock-rates = <19200000>, <200000000>;
+
+			interrupts-extended = <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 860 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 859 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 127 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 126 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 129 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 128 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 131 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 130 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 133 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 132 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 17 IRQ_TYPE_LEVEL_HIGH>;
+
+			interrupt-names = "pwr_event_1", "pwr_event_2",
+					  "pwr_event_3", "pwr_event_4",
+					  "hs_phy_1",	 "hs_phy_2",
+					  "hs_phy_3",	 "hs_phy_4",
+					  "dp_hs_phy_1", "dm_hs_phy_1",
+					  "dp_hs_phy_2", "dm_hs_phy_2",
+					  "dp_hs_phy_3", "dm_hs_phy_3",
+					  "dp_hs_phy_4", "dm_hs_phy_4",
+					  "ss_phy_1",	 "ss_phy_2";
+
+			power-domains = <&gcc USB30_MP_GDSC>;
+			required-opps = <&rpmhpd_opp_nom>;
+
+			resets = <&gcc GCC_USB30_MP_BCR>;
+
+			interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt SLAVE_EBI1 0>,
+					<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_MP 0>;
+			interconnect-names = "usb-ddr", "apps-usb";
+
+			wakeup-source;
+
+			status = "disabled";
+
+			usb_2_dwc3: usb@a400000 {
+				compatible = "snps,dwc3";
+				reg = <0 0x0a400000 0 0xcd00>;
+				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
+				iommus = <&apps_smmu 0x800 0x0>;
+				phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
+				       <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
+				       <&usb_2_hsphy2>,
+				       <&usb_2_hsphy3>;
+				phy-names = "usb2-0", "usb3-0",
+					    "usb2-1", "usb3-1",
+					    "usb2-2",
+					    "usb2-3";
+				dr_mode = "host";
+			};
+		};
+
 		usb_0: usb@a6f8800 {
 			compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3";
 			reg = <0 0x0a6f8800 0 0x400>;
-- 
2.34.1

[PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Krishna Kurapati]

Enable tertiary controller for SA8295P (based on SC8280XP).
Add pinctrl support for usb ports to provide VBUS to connected peripherals.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
index fd253942e5e5..6da444042f82 100644
--- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
@@ -9,6 +9,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
 #include <dt-bindings/spmi/spmi.h>
+#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
 
 #include "sa8540p.dtsi"
 #include "sa8540p-pmics.dtsi"
@@ -584,6 +585,16 @@ &usb_1_qmpphy {
 	status = "okay";
 };
 
+&usb_2 {
+	pinctrl-0 = <&usb2_en>,
+		    <&usb3_en>,
+		    <&usb4_en>,
+		    <&usb5_en>;
+	pinctrl-names = "default";
+
+	status = "okay";
+};
+
 &usb_2_hsphy0 {
 	vdda-pll-supply = <&vreg_l5a>;
 	vdda18-supply = <&vreg_l7g>;
@@ -636,6 +647,44 @@ &xo_board_clk {
 
 /* PINCTRL */
 
+&pmm8540c_gpios {
+	usb2_en: usb2-en-state {
+		pins = "gpio9";
+		function = "normal";
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+		output-high;
+		power-source = <0>;
+	};
+};
+
+&pmm8540e_gpios {
+	usb3_en: usb3-en-state {
+		pins = "gpio5";
+		function = "normal";
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+		output-high;
+		power-source = <0>;
+	};
+};
+
+&pmm8540g_gpios {
+	usb4_en: usb4-en-state {
+		pins = "gpio5";
+		function = "normal";
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+		output-high;
+		power-source = <0>;
+	};
+
+	usb5_en: usb5-en-state {
+		pins = "gpio9";
+		function = "normal";
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+		output-high;
+		power-source = <0>;
+	};
+};
+
 &tlmm {
 	pcie2a_default: pcie2a-default-state {
 		clkreq-n-pins {
-- 
2.34.1

[PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Krishna Kurapati]

From: Andrew Halaney <ahalaney@redhat.com>

There is now support for the multiport USB controller this uses so
enable it.

The board only has a single port hooked up (despite it being wired up to
the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
which by default on boot is selected to mux properly. Grab the gpio
controlling that and ensure it stays in the right position so USB 2.0
continues to be routed from the external port to the SoC.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
Co-developed-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
index b04f72ec097c..eed1ddc29bc1 100644
--- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
@@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
 	status = "okay";
 };
 
+&usb_2 {
+	pinctrl-0 = <&usb2_en>;
+	pinctrl-names = "default";
+
+	status = "okay";
+};
+
+&usb_2_dwc3 {
+	phy-names = "usb2-port0", "usb3-port0";
+	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
+};
+
 &xo_board_clk {
 	clock-frequency = <38400000>;
 };
@@ -655,4 +667,13 @@ wake-pins {
 			bias-pull-up;
 		};
 	};
+
+	usb2_en: usb2-en-state {
+		/* TS3USB221A USB2.0 mux select */
+		pins = "gpio24";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+		output-low;
+	};
 };
-- 
2.34.1

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Dmitry Baryshkov]

On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
>
> Enable tertiary controller for SA8295P (based on SC8280XP).
> Add pinctrl support for usb ports to provide VBUS to connected peripherals.

These are not just pinctrl entries. They hide VBUS regulators. Please
implement them properly as corresponding vbus regulators.

>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> index fd253942e5e5..6da444042f82 100644
> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> @@ -9,6 +9,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>  #include <dt-bindings/spmi/spmi.h>
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
>
>  #include "sa8540p.dtsi"
>  #include "sa8540p-pmics.dtsi"
> @@ -584,6 +585,16 @@ &usb_1_qmpphy {
>         status = "okay";
>  };
>
> +&usb_2 {
> +       pinctrl-0 = <&usb2_en>,
> +                   <&usb3_en>,
> +                   <&usb4_en>,
> +                   <&usb5_en>;
> +       pinctrl-names = "default";
> +
> +       status = "okay";
> +};
> +
>  &usb_2_hsphy0 {
>         vdda-pll-supply = <&vreg_l5a>;
>         vdda18-supply = <&vreg_l7g>;
> @@ -636,6 +647,44 @@ &xo_board_clk {
>
>  /* PINCTRL */
>
> +&pmm8540c_gpios {
> +       usb2_en: usb2-en-state {
> +               pins = "gpio9";
> +               function = "normal";
> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> +               output-high;
> +               power-source = <0>;
> +       };
> +};
> +
> +&pmm8540e_gpios {
> +       usb3_en: usb3-en-state {
> +               pins = "gpio5";
> +               function = "normal";
> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> +               output-high;
> +               power-source = <0>;
> +       };
> +};
> +
> +&pmm8540g_gpios {
> +       usb4_en: usb4-en-state {
> +               pins = "gpio5";
> +               function = "normal";
> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> +               output-high;
> +               power-source = <0>;
> +       };
> +
> +       usb5_en: usb5-en-state {
> +               pins = "gpio9";
> +               function = "normal";
> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> +               output-high;
> +               power-source = <0>;
> +       };
> +};
> +
>  &tlmm {
>         pcie2a_default: pcie2a-default-state {
>                 clkreq-n-pins {
> --
> 2.34.1
>
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Krishna Kurapati PSSNV]

On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
>>
>> Enable tertiary controller for SA8295P (based on SC8280XP).
>> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
> 
> These are not just pinctrl entries. They hide VBUS regulators. Please
> implement them properly as corresponding vbus regulators.
> 

Hi Dmitry. Apologies, can you elaborate on your comment. I thought this 
implementation was fine as Konrad reviewed it in v13 [1]. I removed his 
RB tag as I made one change of dropping "_state" in labels.

[1]: 
https://lore.kernel.org/all/7141c2dd-9dcd-4186-ba83-829fe925e464@linaro.org/

Regards,
Krishna,

>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> index fd253942e5e5..6da444042f82 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> @@ -9,6 +9,7 @@
>>   #include <dt-bindings/gpio/gpio.h>
>>   #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>>   #include <dt-bindings/spmi/spmi.h>
>> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
>>
>>   #include "sa8540p.dtsi"
>>   #include "sa8540p-pmics.dtsi"
>> @@ -584,6 +585,16 @@ &usb_1_qmpphy {
>>          status = "okay";
>>   };
>>
>> +&usb_2 {
>> +       pinctrl-0 = <&usb2_en>,
>> +                   <&usb3_en>,
>> +                   <&usb4_en>,
>> +                   <&usb5_en>;
>> +       pinctrl-names = "default";
>> +
>> +       status = "okay";
>> +};
>> +
>>   &usb_2_hsphy0 {
>>          vdda-pll-supply = <&vreg_l5a>;
>>          vdda18-supply = <&vreg_l7g>;
>> @@ -636,6 +647,44 @@ &xo_board_clk {
>>
>>   /* PINCTRL */
>>
>> +&pmm8540c_gpios {
>> +       usb2_en: usb2-en-state {
>> +               pins = "gpio9";
>> +               function = "normal";
>> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
>> +               output-high;
>> +               power-source = <0>;
>> +       };
>> +};
>> +
>> +&pmm8540e_gpios {
>> +       usb3_en: usb3-en-state {
>> +               pins = "gpio5";
>> +               function = "normal";
>> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
>> +               output-high;
>> +               power-source = <0>;
>> +       };
>> +};
>> +
>> +&pmm8540g_gpios {
>> +       usb4_en: usb4-en-state {
>> +               pins = "gpio5";
>> +               function = "normal";
>> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
>> +               output-high;
>> +               power-source = <0>;
>> +       };
>> +
>> +       usb5_en: usb5-en-state {
>> +               pins = "gpio9";
>> +               function = "normal";
>> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
>> +               output-high;
>> +               power-source = <0>;
>> +       };
>> +};
>> +
>>   &tlmm {
>>          pcie2a_default: pcie2a-default-state {
>>                  clkreq-n-pins {
>> --
>> 2.34.1
>>
>>
> 
> 

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Dmitry Baryshkov]

On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> wrote:
>
>
>
> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
> > On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
> >>
> >> Enable tertiary controller for SA8295P (based on SC8280XP).
> >> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
> >
> > These are not just pinctrl entries. They hide VBUS regulators. Please
> > implement them properly as corresponding vbus regulators.
> >
>
> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this
> implementation was fine as Konrad reviewed it in v13 [1]. I removed his
> RB tag as I made one change of dropping "_state" in labels.

My comment is pretty simple: if I'm not mistaken, your DT doesn't
reflect your hardware design.
You have actual VBUS regulators driven by these GPIO pins. Is this correct?
If so, you should describe them properly in the device tree rather
than describing them just as USB host's pinctrl state.

>
> [1]:
> https://lore.kernel.org/all/7141c2dd-9dcd-4186-ba83-829fe925e464@linaro.org/
>
> Regards,
> Krishna,
>
> >>
> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> >> ---
> >>   arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++
> >>   1 file changed, 49 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> >> index fd253942e5e5..6da444042f82 100644
> >> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> >> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> >> @@ -9,6 +9,7 @@
> >>   #include <dt-bindings/gpio/gpio.h>
> >>   #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> >>   #include <dt-bindings/spmi/spmi.h>
> >> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> >>
> >>   #include "sa8540p.dtsi"
> >>   #include "sa8540p-pmics.dtsi"
> >> @@ -584,6 +585,16 @@ &usb_1_qmpphy {
> >>          status = "okay";
> >>   };
> >>
> >> +&usb_2 {
> >> +       pinctrl-0 = <&usb2_en>,
> >> +                   <&usb3_en>,
> >> +                   <&usb4_en>,
> >> +                   <&usb5_en>;
> >> +       pinctrl-names = "default";
> >> +
> >> +       status = "okay";
> >> +};
> >> +
> >>   &usb_2_hsphy0 {
> >>          vdda-pll-supply = <&vreg_l5a>;
> >>          vdda18-supply = <&vreg_l7g>;
> >> @@ -636,6 +647,44 @@ &xo_board_clk {
> >>
> >>   /* PINCTRL */
> >>
> >> +&pmm8540c_gpios {
> >> +       usb2_en: usb2-en-state {
> >> +               pins = "gpio9";
> >> +               function = "normal";
> >> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> >> +               output-high;
> >> +               power-source = <0>;
> >> +       };
> >> +};
> >> +
> >> +&pmm8540e_gpios {
> >> +       usb3_en: usb3-en-state {
> >> +               pins = "gpio5";
> >> +               function = "normal";
> >> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> >> +               output-high;
> >> +               power-source = <0>;
> >> +       };
> >> +};
> >> +
> >> +&pmm8540g_gpios {
> >> +       usb4_en: usb4-en-state {
> >> +               pins = "gpio5";
> >> +               function = "normal";
> >> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> >> +               output-high;
> >> +               power-source = <0>;
> >> +       };
> >> +
> >> +       usb5_en: usb5-en-state {
> >> +               pins = "gpio9";
> >> +               function = "normal";
> >> +               qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> >> +               output-high;
> >> +               power-source = <0>;
> >> +       };
> >> +};
> >> +
> >>   &tlmm {
> >>          pcie2a_default: pcie2a-default-state {
> >>                  clkreq-n-pins {
> >> --
> >> 2.34.1
> >>
> >>
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[neil.armstrong]

On 06/02/2024 12:47, Krishna Kurapati wrote:
> From: Andrew Halaney <ahalaney@redhat.com>
> 
> There is now support for the multiport USB controller this uses so
> enable it.
> 
> The board only has a single port hooked up (despite it being wired up to
> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> which by default on boot is selected to mux properly. Grab the gpio
> controlling that and ensure it stays in the right position so USB 2.0
> continues to be routed from the external port to the SoC.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> Co-developed-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index b04f72ec097c..eed1ddc29bc1 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
>   	status = "okay";
>   };
>   
> +&usb_2 {
> +	pinctrl-0 = <&usb2_en>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +};
> +
> +&usb_2_dwc3 {
> +	phy-names = "usb2-port0", "usb3-port0";
> +	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> +};
> +
>   &xo_board_clk {
>   	clock-frequency = <38400000>;
>   };
> @@ -655,4 +667,13 @@ wake-pins {
>   			bias-pull-up;
>   		};
>   	};
> +
> +	usb2_en: usb2-en-state {
> +		/* TS3USB221A USB2.0 mux select */
> +		pins = "gpio24";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;
> +	};
>   };

Isn't gpio-hog the preferred way to describe that ?

Neil

Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Dmitry Baryshkov]

On Tue, 6 Feb 2024 at 15:28, <neil.armstrong@linaro.org> wrote:
>
> On 06/02/2024 12:47, Krishna Kurapati wrote:
> > From: Andrew Halaney <ahalaney@redhat.com>
> >
> > There is now support for the multiport USB controller this uses so
> > enable it.
> >
> > The board only has a single port hooked up (despite it being wired up to
> > the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> > which by default on boot is selected to mux properly. Grab the gpio
> > controlling that and ensure it stays in the right position so USB 2.0
> > continues to be routed from the external port to the SoC.

What is connected to the other port of the MUX?

> >
> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > Co-developed-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > ---
> >   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > index b04f72ec097c..eed1ddc29bc1 100644
> > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
> >       status = "okay";
> >   };
> >
> > +&usb_2 {
> > +     pinctrl-0 = <&usb2_en>;
> > +     pinctrl-names = "default";
> > +
> > +     status = "okay";
> > +};
> > +
> > +&usb_2_dwc3 {
> > +     phy-names = "usb2-port0", "usb3-port0";
> > +     phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> > +};
> > +
> >   &xo_board_clk {
> >       clock-frequency = <38400000>;
> >   };
> > @@ -655,4 +667,13 @@ wake-pins {
> >                       bias-pull-up;
> >               };
> >       };
> > +
> > +     usb2_en: usb2-en-state {
> > +             /* TS3USB221A USB2.0 mux select */
> > +             pins = "gpio24";
> > +             function = "gpio";
> > +             drive-strength = <2>;
> > +             bias-disable;
> > +             output-low;
> > +     };
> >   };
>
> Isn't gpio-hog the preferred way to describe that ?

That depends. As this pinctrl describes board configuration, I'd agree
with Neil.


-- 
With best wishes
Dmitry

Re: Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Andrew Halaney]

On Tue, Feb 06, 2024 at 03:30:32PM +0200, Dmitry Baryshkov wrote:
> On Tue, 6 Feb 2024 at 15:28, <neil.armstrong@linaro.org> wrote:
> >
> > On 06/02/2024 12:47, Krishna Kurapati wrote:
> > > From: Andrew Halaney <ahalaney@redhat.com>
> > >
> > > There is now support for the multiport USB controller this uses so
> > > enable it.
> > >
> > > The board only has a single port hooked up (despite it being wired up to
> > > the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> > > which by default on boot is selected to mux properly. Grab the gpio
> > > controlling that and ensure it stays in the right position so USB 2.0
> > > continues to be routed from the external port to the SoC.
> 
> What is connected to the other port of the MUX?

/me blows off the dust on the schematic

It's a 1:2 mux, and one option is the out the board, the other
is a test point I believe if I'm reading things right, although its not
labeled so I'm not sure anyone would actually find it on the board.

> 
> > >
> > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > > Co-developed-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > ---
> > >   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
> > >   1 file changed, 21 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > index b04f72ec097c..eed1ddc29bc1 100644
> > > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
> > >       status = "okay";
> > >   };
> > >
> > > +&usb_2 {
> > > +     pinctrl-0 = <&usb2_en>;
> > > +     pinctrl-names = "default";
> > > +
> > > +     status = "okay";
> > > +};
> > > +
> > > +&usb_2_dwc3 {
> > > +     phy-names = "usb2-port0", "usb3-port0";
> > > +     phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> > > +};
> > > +
> > >   &xo_board_clk {
> > >       clock-frequency = <38400000>;
> > >   };
> > > @@ -655,4 +667,13 @@ wake-pins {
> > >                       bias-pull-up;
> > >               };
> > >       };
> > > +
> > > +     usb2_en: usb2-en-state {
> > > +             /* TS3USB221A USB2.0 mux select */
> > > +             pins = "gpio24";
> > > +             function = "gpio";
> > > +             drive-strength = <2>;
> > > +             bias-disable;
> > > +             output-low;
> > > +     };
> > >   };
> >
> > Isn't gpio-hog the preferred way to describe that ?
> 
> That depends. As this pinctrl describes board configuration, I'd agree
> with Neil.

I unfortunately don't have the experience with gpio-hog to weigh in
here, but wouldn't be opposed to Krishna switching it if that's what's
recommended for this type of thing.

> 
> 
> -- 
> With best wishes
> Dmitry
> 

Re: Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Dmitry Baryshkov]

On Wed, 7 Feb 2024 at 02:10, Andrew Halaney <ahalaney@redhat.com> wrote:
>
> On Tue, Feb 06, 2024 at 03:30:32PM +0200, Dmitry Baryshkov wrote:
> > On Tue, 6 Feb 2024 at 15:28, <neil.armstrong@linaro.org> wrote:
> > >
> > > On 06/02/2024 12:47, Krishna Kurapati wrote:
> > > > From: Andrew Halaney <ahalaney@redhat.com>
> > > >
> > > > There is now support for the multiport USB controller this uses so
> > > > enable it.
> > > >
> > > > The board only has a single port hooked up (despite it being wired up to
> > > > the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> > > > which by default on boot is selected to mux properly. Grab the gpio
> > > > controlling that and ensure it stays in the right position so USB 2.0
> > > > continues to be routed from the external port to the SoC.
> >
> > What is connected to the other port of the MUX?
>
> /me blows off the dust on the schematic
>
> It's a 1:2 mux, and one option is the out the board, the other
> is a test point I believe if I'm reading things right, although its not
> labeled so I'm not sure anyone would actually find it on the board.

Ack, this definitely looks like a static configuration.

>
> >
> > > >
> > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > > > Co-developed-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > ---
> > > >   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
> > > >   1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > > index b04f72ec097c..eed1ddc29bc1 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > > @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
> > > >       status = "okay";
> > > >   };
> > > >
> > > > +&usb_2 {
> > > > +     pinctrl-0 = <&usb2_en>;
> > > > +     pinctrl-names = "default";
> > > > +
> > > > +     status = "okay";
> > > > +};
> > > > +
> > > > +&usb_2_dwc3 {
> > > > +     phy-names = "usb2-port0", "usb3-port0";
> > > > +     phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> > > > +};
> > > > +
> > > >   &xo_board_clk {
> > > >       clock-frequency = <38400000>;
> > > >   };
> > > > @@ -655,4 +667,13 @@ wake-pins {
> > > >                       bias-pull-up;
> > > >               };
> > > >       };
> > > > +
> > > > +     usb2_en: usb2-en-state {
> > > > +             /* TS3USB221A USB2.0 mux select */
> > > > +             pins = "gpio24";
> > > > +             function = "gpio";
> > > > +             drive-strength = <2>;
> > > > +             bias-disable;
> > > > +             output-low;
> > > > +     };
> > > >   };
> > >
> > > Isn't gpio-hog the preferred way to describe that ?
> >
> > That depends. As this pinctrl describes board configuration, I'd agree
> > with Neil.
>
> I unfortunately don't have the experience with gpio-hog to weigh in
> here, but wouldn't be opposed to Krishna switching it if that's what's
> recommended for this type of thing.

Quoting gpio.txt:

The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
providing automatic GPIO request and configuration as part of the
gpio-controller's driver probe function.

See sdm845-pinctrl.yaml for an example of the gpio-hog node.

>
> >
> >
> > --
> > With best wishes
> > Dmitry
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Krishna Kurapati PSSNV]

On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote:
> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV
> <quic_kriskura@quicinc.com> wrote:
>>
>>
>>
>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
>>>>
>>>> Enable tertiary controller for SA8295P (based on SC8280XP).
>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
>>>
>>> These are not just pinctrl entries. They hide VBUS regulators. Please
>>> implement them properly as corresponding vbus regulators.
>>>
>>
>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this
>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his
>> RB tag as I made one change of dropping "_state" in labels.
> 
> My comment is pretty simple: if I'm not mistaken, your DT doesn't
> reflect your hardware design.
> You have actual VBUS regulators driven by these GPIO pins. Is this correct?
> If so, you should describe them properly in the device tree rather
> than describing them just as USB host's pinctrl state.
> 

Hi Dmitry,

  I have very little idea about the gpio controller regulators. I will 
go through it and see how I can implement it. I just found this : 
https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt

One query. If we model it as a regulator, do we need to add it as a 
supply and call regulator_enable in dwc3_qcom probe again ?

Regards,
Krishna,

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Dmitry Baryshkov]

On Thu, 8 Feb 2024 at 04:40, Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> wrote:
> On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote:
> > On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV
> > <quic_kriskura@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
> >>>>
> >>>> Enable tertiary controller for SA8295P (based on SC8280XP).
> >>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
> >>>
> >>> These are not just pinctrl entries. They hide VBUS regulators. Please
> >>> implement them properly as corresponding vbus regulators.
> >>>
> >>
> >> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this
> >> implementation was fine as Konrad reviewed it in v13 [1]. I removed his
> >> RB tag as I made one change of dropping "_state" in labels.
> >
> > My comment is pretty simple: if I'm not mistaken, your DT doesn't
> > reflect your hardware design.
> > You have actual VBUS regulators driven by these GPIO pins. Is this correct?
> > If so, you should describe them properly in the device tree rather
> > than describing them just as USB host's pinctrl state.
> >
>
> Hi Dmitry,
>
>   I have very little idea about the gpio controller regulators. I will
> go through it and see how I can implement it. I just found this :
> https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt

Much simpler, it can be found at
Documentation/devicetree/bindings/regulator/fixed-regulator.yaml

> One query. If we model it as a regulator, do we need to add it as a
> supply and call regulator_enable in dwc3_qcom probe again ?

Not in probe(), but yes. It needs to be enabled when the VBUS has to
be powered up, when the device is initialised or switched to the host
mode, and disabled when the VBUS has to be powered down, if the device
is being switched to the device mode.

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Krishna Kurapati PSSNV]

On 2/8/2024 10:11 AM, Dmitry Baryshkov wrote:
> On Thu, 8 Feb 2024 at 04:40, Krishna Kurapati PSSNV
> <quic_kriskura@quicinc.com> wrote:
>> On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote:
>>> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV
>>> <quic_kriskura@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
>>>>>>
>>>>>> Enable tertiary controller for SA8295P (based on SC8280XP).
>>>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
>>>>>
>>>>> These are not just pinctrl entries. They hide VBUS regulators. Please
>>>>> implement them properly as corresponding vbus regulators.
>>>>>
>>>>
>>>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this
>>>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his
>>>> RB tag as I made one change of dropping "_state" in labels.
>>>
>>> My comment is pretty simple: if I'm not mistaken, your DT doesn't
>>> reflect your hardware design.
>>> You have actual VBUS regulators driven by these GPIO pins. Is this correct?
>>> If so, you should describe them properly in the device tree rather
>>> than describing them just as USB host's pinctrl state.
>>>
>>
>> Hi Dmitry,
>>
>>    I have very little idea about the gpio controller regulators. I will
>> go through it and see how I can implement it. I just found this :
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
> 
> Much simpler, it can be found at
> Documentation/devicetree/bindings/regulator/fixed-regulator.yaml

Thanks for the reference.

> 
>> One query. If we model it as a regulator, do we need to add it as a
>> supply and call regulator_enable in dwc3_qcom probe again ?
> 
> Not in probe(), but yes. It needs to be enabled when the VBUS has to
> be powered up, when the device is initialised or switched to the host
> mode, and disabled when the VBUS has to be powered down, if the device
> is being switched to the device mode.
> 

Actually since we never go to device mode, can't we just stick to this 
pinctrl approach and skip turning on regulator in driver ?

Regards,
Krishna,

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Dmitry Baryshkov]

On Thu, 8 Feb 2024 at 06:48, Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> wrote:
>
>
>
> On 2/8/2024 10:11 AM, Dmitry Baryshkov wrote:
> > On Thu, 8 Feb 2024 at 04:40, Krishna Kurapati PSSNV
> > <quic_kriskura@quicinc.com> wrote:
> >> On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV
> >>> <quic_kriskura@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
> >>>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
> >>>>>>
> >>>>>> Enable tertiary controller for SA8295P (based on SC8280XP).
> >>>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
> >>>>>
> >>>>> These are not just pinctrl entries. They hide VBUS regulators. Please
> >>>>> implement them properly as corresponding vbus regulators.
> >>>>>
> >>>>
> >>>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this
> >>>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his
> >>>> RB tag as I made one change of dropping "_state" in labels.
> >>>
> >>> My comment is pretty simple: if I'm not mistaken, your DT doesn't
> >>> reflect your hardware design.
> >>> You have actual VBUS regulators driven by these GPIO pins. Is this correct?
> >>> If so, you should describe them properly in the device tree rather
> >>> than describing them just as USB host's pinctrl state.
> >>>
> >>
> >> Hi Dmitry,
> >>
> >>    I have very little idea about the gpio controller regulators. I will
> >> go through it and see how I can implement it. I just found this :
> >> https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
> >
> > Much simpler, it can be found at
> > Documentation/devicetree/bindings/regulator/fixed-regulator.yaml
>
> Thanks for the reference.
>
> >
> >> One query. If we model it as a regulator, do we need to add it as a
> >> supply and call regulator_enable in dwc3_qcom probe again ?
> >
> > Not in probe(), but yes. It needs to be enabled when the VBUS has to
> > be powered up, when the device is initialised or switched to the host
> > mode, and disabled when the VBUS has to be powered down, if the device
> > is being switched to the device mode.
> >
>
> Actually since we never go to device mode, can't we just stick to this
> pinctrl approach and skip turning on regulator in driver ?

Scroll several emails back. DT should describe the hardware. Hardware
has VBUS regulators.

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Krishna Kurapati PSSNV]

On 2/8/2024 10:37 AM, Dmitry Baryshkov wrote:
> On Thu, 8 Feb 2024 at 06:48, Krishna Kurapati PSSNV
> <quic_kriskura@quicinc.com> wrote:
>>
>>
>>
>> On 2/8/2024 10:11 AM, Dmitry Baryshkov wrote:
>>> On Thu, 8 Feb 2024 at 04:40, Krishna Kurapati PSSNV
>>> <quic_kriskura@quicinc.com> wrote:
>>>> On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV
>>>>> <quic_kriskura@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
>>>>>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
>>>>>>>>
>>>>>>>> Enable tertiary controller for SA8295P (based on SC8280XP).
>>>>>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
>>>>>>>
>>>>>>> These are not just pinctrl entries. They hide VBUS regulators. Please
>>>>>>> implement them properly as corresponding vbus regulators.
>>>>>>>
>>>>>>
>>>>>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this
>>>>>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his
>>>>>> RB tag as I made one change of dropping "_state" in labels.
>>>>>
>>>>> My comment is pretty simple: if I'm not mistaken, your DT doesn't
>>>>> reflect your hardware design.
>>>>> You have actual VBUS regulators driven by these GPIO pins. Is this correct?
>>>>> If so, you should describe them properly in the device tree rather
>>>>> than describing them just as USB host's pinctrl state.
>>>>>
>>>>
>>>> Hi Dmitry,
>>>>
>>>>     I have very little idea about the gpio controller regulators. I will
>>>> go through it and see how I can implement it. I just found this :
>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
>>>
>>> Much simpler, it can be found at
>>> Documentation/devicetree/bindings/regulator/fixed-regulator.yaml
>>
>> Thanks for the reference.
>>
>>>
>>>> One query. If we model it as a regulator, do we need to add it as a
>>>> supply and call regulator_enable in dwc3_qcom probe again ?
>>>
>>> Not in probe(), but yes. It needs to be enabled when the VBUS has to
>>> be powered up, when the device is initialised or switched to the host
>>> mode, and disabled when the VBUS has to be powered down, if the device
>>> is being switched to the device mode.
>>>
>>
>> Actually since we never go to device mode, can't we just stick to this
>> pinctrl approach and skip turning on regulator in driver ?
> 
> Scroll several emails back. DT should describe the hardware. Hardware
> has VBUS regulators.
> 

Hi Dmitry,

I dug up the schematic and I see that the gpio's we are using in this 
patch are actually "Enable Pins" to an external chip that provides vbus 
to the peripherals connected. So from the perspective of SoC, it is a 
GPIO and not to be represented as a regulator I believe.

Regards,
Krishna,

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Krzysztof Kozlowski]

On 08/02/2024 10:07, Krishna Kurapati PSSNV wrote:
>>>>
>>>>> One query. If we model it as a regulator, do we need to add it as a
>>>>> supply and call regulator_enable in dwc3_qcom probe again ?
>>>>
>>>> Not in probe(), but yes. It needs to be enabled when the VBUS has to
>>>> be powered up, when the device is initialised or switched to the host
>>>> mode, and disabled when the VBUS has to be powered down, if the device
>>>> is being switched to the device mode.
>>>>
>>>
>>> Actually since we never go to device mode, can't we just stick to this
>>> pinctrl approach and skip turning on regulator in driver ?
>>
>> Scroll several emails back. DT should describe the hardware. Hardware
>> has VBUS regulators.
>>
> 
> Hi Dmitry,
> 
> I dug up the schematic and I see that the gpio's we are using in this 
> patch are actually "Enable Pins" to an external chip that provides vbus 
> to the peripherals connected. So from the perspective of SoC, it is a 
> GPIO and not to be represented as a regulator I believe.

According to such logic nothing is a regulator... sorry, you just
described regulator.

Best regards,
Krzysztof

Re: Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Andrew Halaney]

On Wed, Feb 07, 2024 at 03:32:03AM +0200, Dmitry Baryshkov wrote:
> On Wed, 7 Feb 2024 at 02:10, Andrew Halaney <ahalaney@redhat.com> wrote:
> >
> > On Tue, Feb 06, 2024 at 03:30:32PM +0200, Dmitry Baryshkov wrote:
> > > On Tue, 6 Feb 2024 at 15:28, <neil.armstrong@linaro.org> wrote:
> > > >
> > > > On 06/02/2024 12:47, Krishna Kurapati wrote:
> > > > > From: Andrew Halaney <ahalaney@redhat.com>
> > > > >
> > > > > There is now support for the multiport USB controller this uses so
> > > > > enable it.
> > > > >
> > > > > The board only has a single port hooked up (despite it being wired up to
> > > > > the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> > > > > which by default on boot is selected to mux properly. Grab the gpio
> > > > > controlling that and ensure it stays in the right position so USB 2.0
> > > > > continues to be routed from the external port to the SoC.
> > >
> > > What is connected to the other port of the MUX?
> >
> > /me blows off the dust on the schematic
> >
> > It's a 1:2 mux, and one option is the out the board, the other
> > is a test point I believe if I'm reading things right, although its not
> > labeled so I'm not sure anyone would actually find it on the board.
> 
> Ack, this definitely looks like a static configuration.

Krishna, when you make v2 can you update the wording about the USB 2.0
mux? Maybe something like "which by default on boot is selected to mux
to the external port on the board (with the other option being a test
point)." instead of the wording I originally had? That way the
information Dmitry requested here is easily accessible in the future.

> 
> >
> > >
> > > > >
> > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > > > > Co-developed-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > ---
> > > > >   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
> > > > >   1 file changed, 21 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > > > index b04f72ec097c..eed1ddc29bc1 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > > > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > > > > @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
> > > > >       status = "okay";
> > > > >   };
> > > > >
> > > > > +&usb_2 {
> > > > > +     pinctrl-0 = <&usb2_en>;
> > > > > +     pinctrl-names = "default";
> > > > > +
> > > > > +     status = "okay";
> > > > > +};
> > > > > +
> > > > > +&usb_2_dwc3 {
> > > > > +     phy-names = "usb2-port0", "usb3-port0";
> > > > > +     phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> > > > > +};
> > > > > +
> > > > >   &xo_board_clk {
> > > > >       clock-frequency = <38400000>;
> > > > >   };
> > > > > @@ -655,4 +667,13 @@ wake-pins {
> > > > >                       bias-pull-up;
> > > > >               };
> > > > >       };
> > > > > +
> > > > > +     usb2_en: usb2-en-state {
> > > > > +             /* TS3USB221A USB2.0 mux select */
> > > > > +             pins = "gpio24";
> > > > > +             function = "gpio";
> > > > > +             drive-strength = <2>;
> > > > > +             bias-disable;
> > > > > +             output-low;
> > > > > +     };
> > > > >   };
> > > >
> > > > Isn't gpio-hog the preferred way to describe that ?
> > >
> > > That depends. As this pinctrl describes board configuration, I'd agree
> > > with Neil.
> >
> > I unfortunately don't have the experience with gpio-hog to weigh in
> > here, but wouldn't be opposed to Krishna switching it if that's what's
> > recommended for this type of thing.
> 
> Quoting gpio.txt:
> 
> The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> providing automatic GPIO request and configuration as part of the
> gpio-controller's driver probe function.
> 
> See sdm845-pinctrl.yaml for an example of the gpio-hog node.

Thanks, that seems like the way to go. Krishna please take note of this
for v2!

> 
> >
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >
> >
> 
> 
> -- 
> With best wishes
> Dmitry
> 

Re: Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Bjorn Andersson]

On Tue, Feb 06, 2024 at 03:24:45PM +0200, Dmitry Baryshkov wrote:
> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV
> <quic_kriskura@quicinc.com> wrote:
> >
> >
> >
> > On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
> > > On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
> > >>
> > >> Enable tertiary controller for SA8295P (based on SC8280XP).
> > >> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
> > >
> > > These are not just pinctrl entries. They hide VBUS regulators. Please
> > > implement them properly as corresponding vbus regulators.
> > >
> >
> > Hi Dmitry. Apologies, can you elaborate on your comment. I thought this
> > implementation was fine as Konrad reviewed it in v13 [1]. I removed his
> > RB tag as I made one change of dropping "_state" in labels.
> 
> My comment is pretty simple: if I'm not mistaken, your DT doesn't
> reflect your hardware design.
> You have actual VBUS regulators driven by these GPIO pins. Is this correct?
> If so, you should describe them properly in the device tree rather
> than describing them just as USB host's pinctrl state.
> 

This is correct, these gpios are wired to the enable-pin of a set of
regulators providing the VBUS voltage. Please, Krishna, represent them
as always-on fixed-regulators instead.

Regards,
Bjorn

Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

[Krishna Kurapati PSSNV]

On 2/9/2024 8:11 AM, Bjorn Andersson wrote:
> On Tue, Feb 06, 2024 at 03:24:45PM +0200, Dmitry Baryshkov wrote:
>> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV
>> <quic_kriskura@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote:
>>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote:
>>>>>
>>>>> Enable tertiary controller for SA8295P (based on SC8280XP).
>>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
>>>>
>>>> These are not just pinctrl entries. They hide VBUS regulators. Please
>>>> implement them properly as corresponding vbus regulators.
>>>>
>>>
>>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this
>>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his
>>> RB tag as I made one change of dropping "_state" in labels.
>>
>> My comment is pretty simple: if I'm not mistaken, your DT doesn't
>> reflect your hardware design.
>> You have actual VBUS regulators driven by these GPIO pins. Is this correct?
>> If so, you should describe them properly in the device tree rather
>> than describing them just as USB host's pinctrl state.
>>
> 
> This is correct, these gpios are wired to the enable-pin of a set of
> regulators providing the VBUS voltage. Please, Krishna, represent them
> as always-on fixed-regulators instead.
> 
Hi Dmitry, Krzysztof, Bjorn,

  Thanks for the review on this patch. Will convert the pinctrl DT's to 
regulator entries.

Regards,
Krishna,

Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Krishna Kurapati PSSNV]

> Krishna, when you make v2 can you update the wording about the USB 2.0
> mux? Maybe something like "which by default on boot is selected to mux
> to the external port on the board (with the other option being a test
> point)." instead of the wording I originally had? That way the
> information Dmitry requested here is easily accessible in the future.
> 
>>
>>>

[...]

>>>>>>    };
>>>>>
>>>>> Isn't gpio-hog the preferred way to describe that ?
>>>>
>>>> That depends. As this pinctrl describes board configuration, I'd agree
>>>> with Neil.
>>>
>>> I unfortunately don't have the experience with gpio-hog to weigh in
>>> here, but wouldn't be opposed to Krishna switching it if that's what's
>>> recommended for this type of thing.
>>
>> Quoting gpio.txt:
>>
>> The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
>> providing automatic GPIO request and configuration as part of the
>> gpio-controller's driver probe function.
>>
>> See sdm845-pinctrl.yaml for an example of the gpio-hog node.
> 
> Thanks, that seems like the way to go. Krishna please take note of this
> for v2!
> 

Hi Andrew,

  Can you help test the following patch. It is just an add-on to your 
original one. I don't have a SA8540P Ride at the moment and getting one 
might take time. Incase you can confirm this patch is working. I can 
push v2 of this series.


diff --git 
a/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml 
b/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
index ed344deaf8b9..aa42ac5a3197 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
@@ -36,6 +36,10 @@ patternProperties:
              $ref: "#/$defs/qcom-sc8280xp-tlmm-state"
          additionalProperties: false

+  "-hog(-[0-9]+)?$":
+    required:
+      - gpio-hog
+
  $defs:
    qcom-sc8280xp-tlmm-state:
      type: object
diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts 
b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
index b04f72ec097c..aa0cec0b4cc2 100644
--- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
@@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
         status = "okay";
  };

+&usb_2 {
+       pinctrl-0 = <&usb2_en_state>;
+       pinctrl-names = "default";
+
+       status = "okay";
+};
+
+&usb_2_dwc3 {
+       phy-names = "usb2-port0", "usb3-port0";
+       phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
+};
+
  &xo_board_clk {
         clock-frequency = <38400000>;
  };
@@ -655,4 +667,19 @@ wake-pins {
                         bias-pull-up;
                 };
         };
+
+       usb2-en-hog {
+               gpio-hog;
+               gpios = <24 GPIO_ACTIVE_LOW>;
+               output-low;
+       };
+
+       usb2_en_state: usb2-en-state {
+               /* TS3USB221A USB2.0 mux select */
+               pins = "gpio24";
+               function = "gpio";
+               drive-strength = <2>;
+               bias-disable;
+               output-low;
+       };


Regards,
Krishna,

Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Dmitry Baryshkov]

On Sat, 10 Feb 2024 at 12:44, Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> wrote:
>
> > Krishna, when you make v2 can you update the wording about the USB 2.0
> > mux? Maybe something like "which by default on boot is selected to mux
> > to the external port on the board (with the other option being a test
> > point)." instead of the wording I originally had? That way the
> > information Dmitry requested here is easily accessible in the future.
> >
> >>
> >>>
>
> [...]
>
> >>>>>>    };
> >>>>>
> >>>>> Isn't gpio-hog the preferred way to describe that ?
> >>>>
> >>>> That depends. As this pinctrl describes board configuration, I'd agree
> >>>> with Neil.
> >>>
> >>> I unfortunately don't have the experience with gpio-hog to weigh in
> >>> here, but wouldn't be opposed to Krishna switching it if that's what's
> >>> recommended for this type of thing.
> >>
> >> Quoting gpio.txt:
> >>
> >> The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> >> providing automatic GPIO request and configuration as part of the
> >> gpio-controller's driver probe function.
> >>
> >> See sdm845-pinctrl.yaml for an example of the gpio-hog node.
> >
> > Thanks, that seems like the way to go. Krishna please take note of this
> > for v2!
> >
>
> Hi Andrew,
>
>   Can you help test the following patch. It is just an add-on to your
> original one. I don't have a SA8540P Ride at the moment and getting one
> might take time. Incase you can confirm this patch is working. I can
> push v2 of this series.
>
>
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
> b/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
> index ed344deaf8b9..aa42ac5a3197 100644
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
> @@ -36,6 +36,10 @@ patternProperties:
>               $ref: "#/$defs/qcom-sc8280xp-tlmm-state"
>           additionalProperties: false
>
> +  "-hog(-[0-9]+)?$":
> +    required:
> +      - gpio-hog
> +
>   $defs:
>     qcom-sc8280xp-tlmm-state:
>       type: object
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index b04f72ec097c..aa0cec0b4cc2 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
>          status = "okay";
>   };
>
> +&usb_2 {
> +       pinctrl-0 = <&usb2_en_state>;
> +       pinctrl-names = "default";
> +
> +       status = "okay";
> +};
> +
> +&usb_2_dwc3 {
> +       phy-names = "usb2-port0", "usb3-port0";
> +       phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> +};
> +
>   &xo_board_clk {
>          clock-frequency = <38400000>;
>   };
> @@ -655,4 +667,19 @@ wake-pins {
>                          bias-pull-up;
>                  };
>          };
> +
> +       usb2-en-hog {
> +               gpio-hog;
> +               gpios = <24 GPIO_ACTIVE_LOW>;
> +               output-low;
> +       };
> +
> +       usb2_en_state: usb2-en-state {

If you are using gpio-hog, you don't need this state. The pinctrl /
gpio core will use the hog instead.

> +               /* TS3USB221A USB2.0 mux select */
> +               pins = "gpio24";
> +               function = "gpio";
> +               drive-strength = <2>;
> +               bias-disable;
> +               output-low;
> +       };
>
>
> Regards,
> Krishna,



-- 
With best wishes
Dmitry

Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Andrew Halaney]

On Sat, Feb 10, 2024 at 04:13:51PM +0530, Krishna Kurapati PSSNV wrote:
> > Krishna, when you make v2 can you update the wording about the USB 2.0
> > mux? Maybe something like "which by default on boot is selected to mux
> > to the external port on the board (with the other option being a test
> > point)." instead of the wording I originally had? That way the
> > information Dmitry requested here is easily accessible in the future.
> > 
> > > 
> > > > 
> 
> [...]
> 
> > > > > > >    };
> > > > > > 
> > > > > > Isn't gpio-hog the preferred way to describe that ?
> > > > > 
> > > > > That depends. As this pinctrl describes board configuration, I'd agree
> > > > > with Neil.
> > > > 
> > > > I unfortunately don't have the experience with gpio-hog to weigh in
> > > > here, but wouldn't be opposed to Krishna switching it if that's what's
> > > > recommended for this type of thing.
> > > 
> > > Quoting gpio.txt:
> > > 
> > > The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> > > providing automatic GPIO request and configuration as part of the
> > > gpio-controller's driver probe function.
> > > 
> > > See sdm845-pinctrl.yaml for an example of the gpio-hog node.
> > 
> > Thanks, that seems like the way to go. Krishna please take note of this
> > for v2!
> > 
> 
> Hi Andrew,
> 
>  Can you help test the following patch. It is just an add-on to your
> original one. I don't have a SA8540P Ride at the moment and getting one
> might take time. Incase you can confirm this patch is working. I can push v2
> of this series.

I just realized that unfortunately I no longer have access to a
sa8540p-ride, and I'm not sure if I'll regain access.

So I would not be opposed to dropping this patch altogether and someone
dealing with sa8540p-ride when they can test it :/

Sorry,
Andrew

> 
> 
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
> b/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
> index ed344deaf8b9..aa42ac5a3197 100644
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sc8280xp-tlmm.yaml
> @@ -36,6 +36,10 @@ patternProperties:
>              $ref: "#/$defs/qcom-sc8280xp-tlmm-state"
>          additionalProperties: false
> 
> +  "-hog(-[0-9]+)?$":
> +    required:
> +      - gpio-hog
> +
>  $defs:
>    qcom-sc8280xp-tlmm-state:
>      type: object
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index b04f72ec097c..aa0cec0b4cc2 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
>         status = "okay";
>  };
> 
> +&usb_2 {
> +       pinctrl-0 = <&usb2_en_state>;
> +       pinctrl-names = "default";
> +
> +       status = "okay";
> +};
> +
> +&usb_2_dwc3 {
> +       phy-names = "usb2-port0", "usb3-port0";
> +       phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> +};
> +
>  &xo_board_clk {
>         clock-frequency = <38400000>;
>  };
> @@ -655,4 +667,19 @@ wake-pins {
>                         bias-pull-up;
>                 };
>         };
> +
> +       usb2-en-hog {
> +               gpio-hog;
> +               gpios = <24 GPIO_ACTIVE_LOW>;
> +               output-low;
> +       };
> +
> +       usb2_en_state: usb2-en-state {
> +               /* TS3USB221A USB2.0 mux select */
> +               pins = "gpio24";
> +               function = "gpio";
> +               drive-strength = <2>;
> +               bias-disable;
> +               output-low;
> +       };
> 
> 
> Regards,
> Krishna,
> 

Re: [PATCH 3/3] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

[Krishna Kurapati PSSNV]

On 2/13/2024 12:47 AM, Andrew Halaney wrote:

>>
>> Hi Andrew,
>>
>>   Can you help test the following patch. It is just an add-on to your
>> original one. I don't have a SA8540P Ride at the moment and getting one
>> might take time. Incase you can confirm this patch is working. I can push v2
>> of this series.
> 
> I just realized that unfortunately I no longer have access to a
> sa8540p-ride, and I'm not sure if I'll regain access.
> 
> So I would not be opposed to dropping this patch altogether and someone
> dealing with sa8540p-ride when they can test it :/
> 

Hi Andrew,

  It would take time for me to get my hands on one of them. I can take 
up this patch once I get access to hw. In the meantime I can push the 
first two and get this series with.

Regards,
Krishna,