Message-ID: <CAH=2Ntw7Zy8nyDwNEnYQ=j=+25hjZsw3TGUW2TmT3+FvrLNu=A@mail.gmail.com>
Date:   Thu, 21 Apr 2022 01:03:09 +0530
From:   Bhupesh Sharma <bhupesh.sharma@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...ainline.org>
Cc:     linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
        bhupesh.linux@...il.com, linux-kernel@...r.kernel.org,
        vkoul@...nel.org, bjorn.andersson@...aro.org,
        Marijn Suijten <marijn.suijten@...ainline.org>
Subject: Re: [PATCH v2 1/1] arm64: dts: qcom: sm8350-sagami: usb qmp phy node
 - add 'vdda-pll-supply' & 'vdda-phy-supply'

Hi Konrad,

Thanks for your comments. Please see my comments below:

On Wed, 20 Apr 2022 at 22:30, Konrad Dybcio
<konrad.dybcio@...ainline.org> wrote:
>
>
> On 19/04/2022 22:58, Bhupesh Sharma wrote:
> > As suggested by Bjorn during review of [1], the 'vdda-pll-supply' &
> > 'vdda-phy-supply' supplies denote the power for the bus and the
> > clock of the usb qmp phy and are used by the qcom qmp phy driver.
> >
> > So, its safe to assume that the two regulators are the same as on
> > the MTP. So let's wire them up in the same way as the MTP.
> >
> > In absence of the same 'make dtbs_check' leads to following warnings:
> >
> > arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami-pdx215.dt.yaml:
> >   phy-wrapper@...9000: 'vdda-phy-supply' is a required property
> >
> > arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami-pdx215.dt.yaml:
> >   phy-wrapper@...9000: 'vdda-pll-supply' is a required property
> >
> > [1]. https://lore.kernel.org/lkml/20220228123019.382037-9-bhupesh.sharma@linaro.org/
> >
> > Cc: Bjorn Andersson <bjorn.andersson@...aro.org>
> > Cc: konrad.dybcio@...ainline.org
> > Cc: Vinod Koul <vkoul@...nel.org>
> > Cc: Marijn Suijten <marijn.suijten@...ainline.org>
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@...aro.org>
> > ---
> > Changes since v1:
> > -----------------
> > - v1 can be found here: https://www.spinics.net/lists/linux-arm-msm/msg108467.html
> > - Fixed the commit message to read usb qmp phy instead of ufs phy (which
> >    was introduced erroraneously in the commit log).
> >
> >   .../dts/qcom/sm8350-sony-xperia-sagami.dtsi   | 25 +++++++++++++++++++
> >   1 file changed, 25 insertions(+)
>
> Hi, I actually threw together a patch with [almost] all RPMh regulators
> on Sagami quite a while ago (if thunderbird doesn't explode, you should
> find it as an attachment to this message), but I haven't sent it, as
> pm8350c_l3 and pmr735a_s3 are broken on the .c driver side (the step
> numbers and/or values are not correct), but Qualcomm in their infinite
> Qualcommery will not let us, mere mortals, access the PDFs that contain
> the correct specifications, so I have to wait for somebody with access
> to them to pick them up. That said, the values you supplied seem correct
> (say for lack of allow-set-load which will bite when (if?) UFS is ever
> functional upstream on this piece of Japanese engineering), but adding
> the configuration of all regulators at once just seems more complete to me..

The complete set of regulator enablement was something on my mind and
which I tried before sending out the smaller (only relevant to USB)
patch.

My main concern was leaving the upstream kernel on the board in an
unbootable form, as I don't have the sagami board to test the changes
on.

That said, if you intend to send the complete regulator enablement
patch for the sagami board, it would be probably better in the longer
run (as currently any change to the common binding, dts or driver
files is held up by "make dtbs_check" warnings during upstream
review).

> On a note, USB - among other more or less necessary peripherals - along
> with its supplies, is brought up in XBL (or bootrom if you jump to EDL),
> so not setting these supplies in Linux does not bite in any way, shape
> or form other than "make dtbs_check" screaming.

I agree. If you want, you can include my simple patch as the followup
(just enabling the right regulators for the usb qmp phy), to your big
patch (which enables the complete regulator support). It should be
fine as well, as we eventually want to use the regulators defined in
the 'big' patch for some 'real' peripheral nodes like the usb qmp phy.

Please let me know your views.

Regards,
Bhupesh

> I'm not against this patch, but once again, even though this point may
> not sound very convincing to you all, I think it would be "nice" to
> configure all regulators in one go.
>
>
> Konrad
>
>
> > diff --git a/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi b/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi
> > index 90b13cbe2fa6..238ac9380ca2 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi
> > @@ -3,6 +3,7 @@
> >    * Copyright (c) 2021, Konrad Dybcio <konrad.dybcio@...ainline.org>
> >    */
> >
> > +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> >   #include "sm8350.dtsi"
> >   #include "pm8350.dtsi"
> >   #include "pm8350b.dtsi"
> > @@ -75,6 +76,27 @@ ramoops@...00000 {
> >       };
> >   };
> >
> > +&apps_rsc {
> > +     pm8350-rpmh-regulators {
> > +             compatible = "qcom,pm8350-rpmh-regulators";
> > +             qcom,pmic-id = "b";
> > +
> > +             vreg_l1b_0p88: ldo1 {
> > +                     regulator-name = "vreg_l1b_0p88";
> > +                     regulator-min-microvolt = <912000>;
> > +                     regulator-max-microvolt = <920000>;
> > +                     regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > +             };
> > +
> > +             vreg_l6b_1p2: ldo6 {
> > +                     regulator-name = "vreg_l6b_1p2";
> > +                     regulator-min-microvolt = <1200000>;
> > +                     regulator-max-microvolt = <1208000>;
> > +                     regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > +             };
> > +     };
> > +};
> > +
> >   &adsp {
> >       status = "okay";
> >       firmware-name = "qcom/adsp.mbn";
> > @@ -256,4 +278,7 @@ &usb_1_hsphy {
> >
> >   &usb_1_qmpphy {
> >       status = "okay";
> > +
> > +     vdda-phy-supply = <&vreg_l6b_1p2>;
> > +     vdda-pll-supply = <&vreg_l1b_0p88>;
> >   };

Powered by blists - more mailing lists