Message-ID: <CAA8EJppf+j6H8vPOrer1Oj6SuM=qHKXoBahtCaCr7an-cbpyOQ@mail.gmail.com>
Date: Sat, 17 Feb 2024 03:01:29 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, 
	Krishna Kurapati <quic_kriskura@...cinc.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Rob Herring <robh+dt@...nel.org>, 
	Konrad Dybcio <konrad.dybcio@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, quic_ppratap@...cinc.com, 
	quic_jackp@...cinc.com
Subject: Re: [PATCH v2 2/2] arm64: dts: qcom: sa8295p: Enable tertiary
 controller and its 4 USB ports

On Sat, 17 Feb 2024 at 01:33, Bjorn Andersson <andersson@...nel.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@...cinc.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

Powered by blists - more mailing lists