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

[PATCH v2 0/2] 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 platform.

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

Changes in v2:
SA8540 Ride related code changes have been dropped and will pushed later
due to unavailability of hardware with either Andrew or me for testing the
gpio hog changes suggested in v1.
Implemented vbus boost regulators as fixed regulators instead of modelling
their enable pins as pinctrl nodes.

Link to v1:
https://lore.kernel.org/all/20240206114745.1388491-1-quic_kriskura@quicinc.com/

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

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 | 83 ++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi   | 82 +++++++++++++++++++++++
 2 files changed, 165 insertions(+)

-- 
2.34.1

[PATCH v2 1/2] 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

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

[Johan Hovold]

On Tue, Feb 13, 2024 at 01:57:23PM +0530, Krishna Kurapati wrote:
> 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

"interrupts" and "PHYs"

> platforms.

But I suggest you just reword this along the lines of

	Add USB DWC3 multiport controller.

as it's assumed that you'll describe resources like interrupts and PHYs.

Perhaps no need to mention SA8295p either as this change is needed (and
correct) also for sc8280xp proper.

> 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 {

> +			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>,

These should all be IRQ_TYPE_EDGE_BOTH as DP/DM interrupts may need to
trigger also on falling edges.

> +					      <&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";

Johan

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

[Krishna Kurapati]

Multiport USB controller (host-only) of SA8295 ADP has 4 Type-A ports
exposed for connecting peripherals. The VBUS to these peripherals is
provided by TPS2559QWDRCTQ1 regulators connected to these ports. Each
regulator has an enable pin controlled by PMM8540. Since these regulators
are GPIO controlled regulators, model them as fixed regulators and keep
them Always-On at boot since we are wakeup capable and we don't need to
turn them off on suspend. Also since we don't enter device mode, these
regulators can be kept on.

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

diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
index fd253942e5e5..49418843c214 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"
@@ -108,6 +109,46 @@ edp3_connector_in: endpoint {
 			};
 		};
 	};
+
+	regulator-usb2-vbus {
+		compatible = "regulator-fixed";
+		regulator-name = "USB2_VBUS";
+		gpio = <&pmm8540c_gpios 9 GPIO_ACTIVE_HIGH>;
+		pinctrl-0 = <&usb2_en>;
+		pinctrl-names = "default";
+		enable-active-high;
+		regulator-always-on;
+	};
+
+	regulator-usb3-vbus {
+		compatible = "regulator-fixed";
+		regulator-name = "USB3_VBUS";
+		gpio = <&pmm8540e_gpios 5 GPIO_ACTIVE_HIGH>;
+		pinctrl-0 = <&usb3_en>;
+		pinctrl-names = "default";
+		enable-active-high;
+		regulator-always-on;
+	};
+
+	regulator-usb4-vbus {
+		compatible = "regulator-fixed";
+		regulator-name = "USB4_VBUS";
+		gpio = <&pmm8540g_gpios 5 GPIO_ACTIVE_HIGH>;
+		pinctrl-0 = <&usb4_en>;
+		pinctrl-names = "default";
+		enable-active-high;
+		regulator-always-on;
+	};
+
+	regulator-usb5-vbus {
+		compatible = "regulator-fixed";
+		regulator-name = "USB5_VBUS";
+		gpio = <&pmm8540g_gpios 9 GPIO_ACTIVE_HIGH>;
+		pinctrl-0 = <&usb5_en>;
+		pinctrl-names = "default";
+		enable-active-high;
+		regulator-always-on;
+	};
 };
 
 &apps_rsc {
@@ -584,6 +625,10 @@ &usb_1_qmpphy {
 	status = "okay";
 };
 
+&usb_2 {
+	status = "okay";
+};
+
 &usb_2_hsphy0 {
 	vdda-pll-supply = <&vreg_l5a>;
 	vdda18-supply = <&vreg_l7g>;
@@ -636,6 +681,44 @@ &xo_board_clk {
 
 /* PINCTRL */
 
+&pmm8540c_gpios {
+	usb2_en: usb2-en-state {
+		pins = "gpio9";
+		function = "normal";
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+		output-enable;
+		power-source = <0>;
+	};
+};
+
+&pmm8540e_gpios {
+	usb3_en: usb3-en-state {
+		pins = "gpio5";
+		function = "normal";
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+		output-enable;
+		power-source = <0>;
+	};
+};
+
+&pmm8540g_gpios {
+	usb4_en: usb4-en-state {
+		pins = "gpio5";
+		function = "normal";
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+		output-enable;
+		power-source = <0>;
+	};
+
+	usb5_en: usb5-en-state {
+		pins = "gpio9";
+		function = "normal";
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+		output-enable;
+		power-source = <0>;
+	};
+};
+
 &tlmm {
 	pcie2a_default: pcie2a-default-state {
 		clkreq-n-pins {
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 13/02/2024 09:27, Krishna Kurapati wrote:
> Multiport USB controller (host-only) of SA8295 ADP has 4 Type-A ports
> exposed for connecting peripherals. The VBUS to these peripherals is
> provided by TPS2559QWDRCTQ1 regulators connected to these ports. Each
> regulator has an enable pin controlled by PMM8540. Since these regulators
> are GPIO controlled regulators, model them as fixed regulators and keep
> them Always-On at boot since we are wakeup capable and we don't need to
> turn them off on suspend. Also since we don't enter device mode, these
> regulators can be kept on.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 83 ++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> index fd253942e5e5..49418843c214 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"
> @@ -108,6 +109,46 @@ edp3_connector_in: endpoint {
>  			};
>  		};
>  	};
> +
> +	regulator-usb2-vbus {
> +		compatible = "regulator-fixed";
> +		regulator-name = "USB2_VBUS";
> +		gpio = <&pmm8540c_gpios 9 GPIO_ACTIVE_HIGH>;
> +		pinctrl-0 = <&usb2_en>;
> +		pinctrl-names = "default";
> +		enable-active-high;
> +		regulator-always-on;
> +	};
> +
> +	regulator-usb3-vbus {
> +		compatible = "regulator-fixed";
> +		regulator-name = "USB3_VBUS";
> +		gpio = <&pmm8540e_gpios 5 GPIO_ACTIVE_HIGH>;
> +		pinctrl-0 = <&usb3_en>;
> +		pinctrl-names = "default";
> +		enable-active-high;
> +		regulator-always-on;
> +	};
> +
> +	regulator-usb4-vbus {
> +		compatible = "regulator-fixed";
> +		regulator-name = "USB4_VBUS";
> +		gpio = <&pmm8540g_gpios 5 GPIO_ACTIVE_HIGH>;
> +		pinctrl-0 = <&usb4_en>;
> +		pinctrl-names = "default";
> +		enable-active-high;
> +		regulator-always-on;
> +	};
> +
> +	regulator-usb5-vbus {
> +		compatible = "regulator-fixed";
> +		regulator-name = "USB5_VBUS";
> +		gpio = <&pmm8540g_gpios 9 GPIO_ACTIVE_HIGH>;
> +		pinctrl-0 = <&usb5_en>;
> +		pinctrl-names = "default";
> +		enable-active-high;
> +		regulator-always-on;

Why all these regulators are always on? If USB controller does not probe
for any reason, why keeping them enabled? These must not be always-on,
but instead used by connector as VBUS supply (or by whatever you have
there for USB).

Best regards,
Krzysztof

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

[Bjorn Andersson]

On Tue, Feb 13, 2024 at 09:39:51AM +0100, Krzysztof Kozlowski wrote:
> On 13/02/2024 09:27, Krishna Kurapati wrote:
> > Multiport USB controller (host-only) of SA8295 ADP has 4 Type-A ports
> > exposed for connecting peripherals. The VBUS to these peripherals is
> > provided by TPS2559QWDRCTQ1 regulators connected to these ports. Each
> > regulator has an enable pin controlled by PMM8540. Since these regulators
> > are GPIO controlled regulators, model them as fixed regulators and keep
> > them Always-On at boot since we are wakeup capable and we don't need to
> > turn them off on suspend. Also since we don't enter device mode, these
> > regulators can be kept on.
> > 
> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 83 ++++++++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > index fd253942e5e5..49418843c214 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"
> > @@ -108,6 +109,46 @@ edp3_connector_in: endpoint {
> >  			};
> >  		};
> >  	};
> > +
> > +	regulator-usb2-vbus {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "USB2_VBUS";
> > +		gpio = <&pmm8540c_gpios 9 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-0 = <&usb2_en>;
> > +		pinctrl-names = "default";
> > +		enable-active-high;
> > +		regulator-always-on;
> > +	};
> > +
> > +	regulator-usb3-vbus {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "USB3_VBUS";
> > +		gpio = <&pmm8540e_gpios 5 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-0 = <&usb3_en>;
> > +		pinctrl-names = "default";
> > +		enable-active-high;
> > +		regulator-always-on;
> > +	};
> > +
> > +	regulator-usb4-vbus {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "USB4_VBUS";
> > +		gpio = <&pmm8540g_gpios 5 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-0 = <&usb4_en>;
> > +		pinctrl-names = "default";
> > +		enable-active-high;
> > +		regulator-always-on;
> > +	};
> > +
> > +	regulator-usb5-vbus {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "USB5_VBUS";
> > +		gpio = <&pmm8540g_gpios 9 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-0 = <&usb5_en>;
> > +		pinctrl-names = "default";
> > +		enable-active-high;
> > +		regulator-always-on;
> 
> Why all these regulators are always on? If USB controller does not probe
> for any reason, why keeping them enabled? These must not be always-on,
> but instead used by connector as VBUS supply (or by whatever you have
> there for USB).
> 

I'm not too concerned about keeping the lights on in this scenario, but
if we can describe this properly let's do so (and let's do so on other
boards with connectors as well).

We'd have a set of usb-a-connector nodes, that we can tie to the nodes
in the USB/phy, and the supply. But so far we've associated a connector
with a port manager, here we don't have one of those, so where would the
node reside and who should acquire and drive the vbus-supply?

Regards,
Bjorn

> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 15/02/2024 03:41, Bjorn Andersson wrote:
> On Tue, Feb 13, 2024 at 09:39:51AM +0100, Krzysztof Kozlowski wrote:
>> On 13/02/2024 09:27, Krishna Kurapati wrote:
>>> Multiport USB controller (host-only) of SA8295 ADP has 4 Type-A ports
>>> exposed for connecting peripherals. The VBUS to these peripherals is
>>> provided by TPS2559QWDRCTQ1 regulators connected to these ports. Each
>>> regulator has an enable pin controlled by PMM8540. Since these regulators
>>> are GPIO controlled regulators, model them as fixed regulators and keep
>>> them Always-On at boot since we are wakeup capable and we don't need to
>>> turn them off on suspend. Also since we don't enter device mode, these
>>> regulators can be kept on.
>>>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 83 ++++++++++++++++++++++++
>>>  1 file changed, 83 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>>> index fd253942e5e5..49418843c214 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"
>>> @@ -108,6 +109,46 @@ edp3_connector_in: endpoint {
>>>  			};
>>>  		};
>>>  	};
>>> +
>>> +	regulator-usb2-vbus {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "USB2_VBUS";
>>> +		gpio = <&pmm8540c_gpios 9 GPIO_ACTIVE_HIGH>;
>>> +		pinctrl-0 = <&usb2_en>;
>>> +		pinctrl-names = "default";
>>> +		enable-active-high;
>>> +		regulator-always-on;
>>> +	};
>>> +
>>> +	regulator-usb3-vbus {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "USB3_VBUS";
>>> +		gpio = <&pmm8540e_gpios 5 GPIO_ACTIVE_HIGH>;
>>> +		pinctrl-0 = <&usb3_en>;
>>> +		pinctrl-names = "default";
>>> +		enable-active-high;
>>> +		regulator-always-on;
>>> +	};
>>> +
>>> +	regulator-usb4-vbus {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "USB4_VBUS";
>>> +		gpio = <&pmm8540g_gpios 5 GPIO_ACTIVE_HIGH>;
>>> +		pinctrl-0 = <&usb4_en>;
>>> +		pinctrl-names = "default";
>>> +		enable-active-high;
>>> +		regulator-always-on;
>>> +	};
>>> +
>>> +	regulator-usb5-vbus {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "USB5_VBUS";
>>> +		gpio = <&pmm8540g_gpios 9 GPIO_ACTIVE_HIGH>;
>>> +		pinctrl-0 = <&usb5_en>;
>>> +		pinctrl-names = "default";
>>> +		enable-active-high;
>>> +		regulator-always-on;
>>
>> Why all these regulators are always on? If USB controller does not probe
>> for any reason, why keeping them enabled? These must not be always-on,
>> but instead used by connector as VBUS supply (or by whatever you have
>> there for USB).
>>
> 
> I'm not too concerned about keeping the lights on in this scenario, but
> if we can describe this properly let's do so (and let's do so on other
> boards with connectors as well).
> 
> We'd have a set of usb-a-connector nodes, that we can tie to the nodes
> in the USB/phy, and the supply. But so far we've associated a connector
> with a port manager, here we don't have one of those, so where would the
> node reside and who should acquire and drive the vbus-supply?

usb-connector binding has vbus-supply and its node could be top-level.
However don't some USB phys also take that regulator?

Best regards,
Krzysztof

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

[Bjorn Andersson]

On Thu, Feb 15, 2024 at 09:19:39AM +0100, Krzysztof Kozlowski wrote:
> On 15/02/2024 03:41, Bjorn Andersson wrote:
> > On Tue, Feb 13, 2024 at 09:39:51AM +0100, Krzysztof Kozlowski wrote:
> >> On 13/02/2024 09:27, Krishna Kurapati wrote:
> >>> Multiport USB controller (host-only) of SA8295 ADP has 4 Type-A ports
> >>> exposed for connecting peripherals. The VBUS to these peripherals is
> >>> provided by TPS2559QWDRCTQ1 regulators connected to these ports. Each
> >>> regulator has an enable pin controlled by PMM8540. Since these regulators
> >>> are GPIO controlled regulators, model them as fixed regulators and keep
> >>> them Always-On at boot since we are wakeup capable and we don't need to
> >>> turn them off on suspend. Also since we don't enter device mode, these
> >>> regulators can be kept on.
> >>>
> >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> >>> ---
> >>>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 83 ++++++++++++++++++++++++
> >>>  1 file changed, 83 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> >>> index fd253942e5e5..49418843c214 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"
> >>> @@ -108,6 +109,46 @@ edp3_connector_in: endpoint {
> >>>  			};
> >>>  		};
> >>>  	};
> >>> +
> >>> +	regulator-usb2-vbus {
> >>> +		compatible = "regulator-fixed";
> >>> +		regulator-name = "USB2_VBUS";
> >>> +		gpio = <&pmm8540c_gpios 9 GPIO_ACTIVE_HIGH>;
> >>> +		pinctrl-0 = <&usb2_en>;
> >>> +		pinctrl-names = "default";
> >>> +		enable-active-high;
> >>> +		regulator-always-on;
> >>> +	};
> >>> +
> >>> +	regulator-usb3-vbus {
> >>> +		compatible = "regulator-fixed";
> >>> +		regulator-name = "USB3_VBUS";
> >>> +		gpio = <&pmm8540e_gpios 5 GPIO_ACTIVE_HIGH>;
> >>> +		pinctrl-0 = <&usb3_en>;
> >>> +		pinctrl-names = "default";
> >>> +		enable-active-high;
> >>> +		regulator-always-on;
> >>> +	};
> >>> +
> >>> +	regulator-usb4-vbus {
> >>> +		compatible = "regulator-fixed";
> >>> +		regulator-name = "USB4_VBUS";
> >>> +		gpio = <&pmm8540g_gpios 5 GPIO_ACTIVE_HIGH>;
> >>> +		pinctrl-0 = <&usb4_en>;
> >>> +		pinctrl-names = "default";
> >>> +		enable-active-high;
> >>> +		regulator-always-on;
> >>> +	};
> >>> +
> >>> +	regulator-usb5-vbus {
> >>> +		compatible = "regulator-fixed";
> >>> +		regulator-name = "USB5_VBUS";
> >>> +		gpio = <&pmm8540g_gpios 9 GPIO_ACTIVE_HIGH>;
> >>> +		pinctrl-0 = <&usb5_en>;
> >>> +		pinctrl-names = "default";
> >>> +		enable-active-high;
> >>> +		regulator-always-on;
> >>
> >> Why all these regulators are always on? If USB controller does not probe
> >> for any reason, why keeping them enabled? These must not be always-on,
> >> but instead used by connector as VBUS supply (or by whatever you have
> >> there for USB).
> >>
> > 
> > I'm not too concerned about keeping the lights on in this scenario, but
> > if we can describe this properly let's do so (and let's do so on other
> > boards with connectors as well).
> > 
> > We'd have a set of usb-a-connector nodes, that we can tie to the nodes
> > in the USB/phy, and the supply. But so far we've associated a connector
> > with a port manager, here we don't have one of those, so where would the
> > node reside and who should acquire and drive the vbus-supply?
> 
> usb-connector binding has vbus-supply and its node could be top-level.

Introducing usb-connector nodes toplevel, with vbus-supply sounds
reasonable. But to my knowledge there's today no way to acquire a
handle to that regulator, unless you have a struct device for the
connector node.

> However don't some USB phys also take that regulator?
> 

I don't think it's appropriate to add the supply to any of the phys,
some of the connectors has 2 phys others has 1 phy.

Representing the vbus in the connector but driving it from the phy
(we will need to figure out which one) sounds reasonable. We just need
to make sure this doesn't conflict with the fact that some TCPM
implementations also seems to drive vbus.


I would like this to be properly modelled, but it seems reasonable to
punt that to the todo list for now.

Regards,
Bjorn

> Best regards,
> Krzysztof
> 

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

[Dmitry Baryshkov]

On Sat, 17 Feb 2024 at 01:33, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 15, 2024 at 09:19:39AM +0100, Krzysztof Kozlowski wrote:
> > On 15/02/2024 03:41, Bjorn Andersson wrote:
> > > On Tue, Feb 13, 2024 at 09:39:51AM +0100, Krzysztof Kozlowski wrote:
> > >> On 13/02/2024 09:27, Krishna Kurapati wrote:
> > >>> Multiport USB controller (host-only) of SA8295 ADP has 4 Type-A ports
> > >>> exposed for connecting peripherals. The VBUS to these peripherals is
> > >>> provided by TPS2559QWDRCTQ1 regulators connected to these ports. Each
> > >>> regulator has an enable pin controlled by PMM8540. Since these regulators
> > >>> are GPIO controlled regulators, model them as fixed regulators and keep
> > >>> them Always-On at boot since we are wakeup capable and we don't need to
> > >>> turn them off on suspend. Also since we don't enter device mode, these
> > >>> regulators can be kept on.
> > >>>
> > >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > >>> ---
> > >>>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 83 ++++++++++++++++++++++++
> > >>>  1 file changed, 83 insertions(+)
> > >>>
> > >>> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > >>> index fd253942e5e5..49418843c214 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"
> > >>> @@ -108,6 +109,46 @@ edp3_connector_in: endpoint {
> > >>>                   };
> > >>>           };
> > >>>   };
> > >>> +
> > >>> + regulator-usb2-vbus {
> > >>> +         compatible = "regulator-fixed";
> > >>> +         regulator-name = "USB2_VBUS";
> > >>> +         gpio = <&pmm8540c_gpios 9 GPIO_ACTIVE_HIGH>;
> > >>> +         pinctrl-0 = <&usb2_en>;
> > >>> +         pinctrl-names = "default";
> > >>> +         enable-active-high;
> > >>> +         regulator-always-on;
> > >>> + };
> > >>> +
> > >>> + regulator-usb3-vbus {
> > >>> +         compatible = "regulator-fixed";
> > >>> +         regulator-name = "USB3_VBUS";
> > >>> +         gpio = <&pmm8540e_gpios 5 GPIO_ACTIVE_HIGH>;
> > >>> +         pinctrl-0 = <&usb3_en>;
> > >>> +         pinctrl-names = "default";
> > >>> +         enable-active-high;
> > >>> +         regulator-always-on;
> > >>> + };
> > >>> +
> > >>> + regulator-usb4-vbus {
> > >>> +         compatible = "regulator-fixed";
> > >>> +         regulator-name = "USB4_VBUS";
> > >>> +         gpio = <&pmm8540g_gpios 5 GPIO_ACTIVE_HIGH>;
> > >>> +         pinctrl-0 = <&usb4_en>;
> > >>> +         pinctrl-names = "default";
> > >>> +         enable-active-high;
> > >>> +         regulator-always-on;
> > >>> + };
> > >>> +
> > >>> + regulator-usb5-vbus {
> > >>> +         compatible = "regulator-fixed";
> > >>> +         regulator-name = "USB5_VBUS";
> > >>> +         gpio = <&pmm8540g_gpios 9 GPIO_ACTIVE_HIGH>;
> > >>> +         pinctrl-0 = <&usb5_en>;
> > >>> +         pinctrl-names = "default";
> > >>> +         enable-active-high;
> > >>> +         regulator-always-on;
> > >>
> > >> Why all these regulators are always on? If USB controller does not probe
> > >> for any reason, why keeping them enabled? These must not be always-on,
> > >> but instead used by connector as VBUS supply (or by whatever you have
> > >> there for USB).
> > >>
> > >
> > > I'm not too concerned about keeping the lights on in this scenario, but
> > > if we can describe this properly let's do so (and let's do so on other
> > > boards with connectors as well).
> > >
> > > We'd have a set of usb-a-connector nodes, that we can tie to the nodes
> > > in the USB/phy, and the supply. But so far we've associated a connector
> > > with a port manager, here we don't have one of those, so where would the
> > > node reside and who should acquire and drive the vbus-supply?
> >
> > usb-connector binding has vbus-supply and its node could be top-level.
>
> Introducing usb-connector nodes toplevel, with vbus-supply sounds
> reasonable. But to my knowledge there's today no way to acquire a
> handle to that regulator, unless you have a struct device for the
> connector node.
>
> > However don't some USB phys also take that regulator?
> >
>
> I don't think it's appropriate to add the supply to any of the phys,
> some of the connectors has 2 phys others has 1 phy.
>
> Representing the vbus in the connector but driving it from the phy
> (we will need to figure out which one) sounds reasonable. We just need
> to make sure this doesn't conflict with the fact that some TCPM
> implementations also seems to drive vbus.

I think vbus can be toggled from the dwc3 controller itself rather
than from the PHY.

>
>
> I would like this to be properly modelled, but it seems reasonable to
> punt that to the todo list for now.



-- 
With best wishes
Dmitry

Re: [PATCH v2 0/2] Add DT support for Multiport controller on SC8280XP

[Johan Hovold]

On Tue, Feb 13, 2024 at 01:57:22PM +0530, Krishna Kurapati wrote:
> 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 platform.
> 
> Till v13 the DT was pushed along with driver code. Separate the DT changes
> from driver update and pushing it as this series.

Then this is version 15, don't reset the version number and hide all the
work that has gone into getting where we are today (e.g. by removing the
changelog).

> Changes in v2:
> SA8540 Ride related code changes have been dropped and will pushed later
> due to unavailability of hardware with either Andrew or me for testing the
> gpio hog changes suggested in v1.
> Implemented vbus boost regulators as fixed regulators instead of modelling
> their enable pins as pinctrl nodes.
> 
> Link to v1:
> https://lore.kernel.org/all/20240206114745.1388491-1-quic_kriskura@quicinc.com/
> 
> [1]: https://lore.kernel.org/all/20240206051825.1038685-1-quic_kriskura@quicinc.com/

Also make sure to include reviewers that have spend time on your series
as I'm sure I've asked you before.

Please fix in a v16.

Johan