Received-SPF: pass (google.com: domain of linux-kernel+bounces-55014-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240206114745.1388491-1-quic_kriskura@quicinc.com>
 <20240206114745.1388491-3-quic_kriskura@quicinc.com> <CAA8EJpoed-hu4hPXAcwQxmJAaNRwJ2y5q9qybWaPP8bdMnz_oA@mail.gmail.com>
 <0470a930-d629-4467-b619-58d3e76f59a7@quicinc.com>
In-Reply-To: <0470a930-d629-4467-b619-58d3e76f59a7@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Tue, 6 Feb 2024 15:24:45 +0200
Message-ID: <CAA8EJppJAdHXoVs_2VqQf=_Wk_LoEcNMY2H-Xzqu8KzeaN8i0g@mail.gmail.com>
Subject: Re: [PATCH 2/3] arm64: dts: qcom: sa8295p: Enable tertiary controller
 and its 4 USB ports
To: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Cc: Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Rob Herring <robh+dt@kernel.org>, 
	Bjorn Andersson <andersson@kernel.org>, Conor Dooley <conor+dt@kernel.org>, linux-kernel@vger.kernel.org, 
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, 
	quic_ppratap@quicinc.com, quic_jackp@quicinc.com
Content-Type: text/plain; charset="UTF-8"

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