Date:   Wed, 20 Apr 2022 21:45:32 +0200
From:   Konrad Dybcio <konrad.dybcio@...ainline.org>
To:     Bhupesh Sharma <bhupesh.sharma@...aro.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'


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@...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.

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@...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