Messages in this thread Patch in this message | | | Date | Wed, 20 Apr 2022 21:45:32 +0200 | Subject | Re: [PATCH v2 1/1] arm64: dts: qcom: sm8350-sagami: usb qmp phy node - add 'vdda-pll-supply' & 'vdda-phy-supply' | From | Konrad Dybcio <> |
| |
On 20/04/2022 21:33, Bhupesh Sharma wrote: > Hi Konrad, > > Thanks for your comments. Please see my comments below: > > On Wed, 20 Apr 2022 at 22:30, Konrad Dybcio > <konrad.dybcio@somainline.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@88e9000: 'vdda-phy-supply' is a required property >>> >>> arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami-pdx215.dt.yaml: >>> phy-wrapper@88e9000: '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@linaro.org> >>> Cc: konrad.dybcio@somainline.org >>> Cc: Vinod Koul <vkoul@kernel.org> >>> Cc: Marijn Suijten <marijn.suijten@somainline.org> >>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.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.
Yeah, messing with power rails on somebody else's device is stressful in general, especially with downstream kernels #including 9999 other DTs, it's really easy to make a mistake.. Personally, I sometimes even dump the regulator settings from syfs on a running device just to be ultra sure.
> > 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 will send this patch (of course rebased) along with the add-all-regulators one, once the .c driver is fixed then.
Thanks,
Konrad
>> 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@somainline.org> >>> */ >>> >>> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h> >>> #include "sm8350.dtsi" >>> #include "pm8350.dtsi" >>> #include "pm8350b.dtsi" >>> @@ -75,6 +76,27 @@ ramoops@ffc00000 { >>> }; >>> }; >>> >>> +&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>; >>> };
| |