Re: [PATCH v3] arm64: dts: qcom: sdm845-xiaomi-beryllium: Add DSI and panel bits

[AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>]

Il 12/02/21 10:24, Amit Pundir ha scritto:
> Hi,
> 
> On Thu, 11 Feb 2021 at 00:25, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>>
>> Il 10/02/21 09:18, Amit Pundir ha scritto:
>>> From: Sumit Semwal <sumit.semwal@linaro.org>
>>>
>>> Enabling the Display panel for beryllium requires DSI
>>> labibb regulators and panel dts nodes to be added.
>>> It is also required to keep some of the regulators as
>>> always-on.
>>>
>>> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
>>> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
>>> ---
>>
>> Hello!
>> Your patch looks good, however, I have a few concerns...
>>
>>> v3: Addressed Konrad's concerns. Configured labibb regulators
>>>       explicitly based on downstream microvolt values. Display
>>>       comes up fine with default discharge-resistor-kohms and
>>>       soft-start-us properties, so didn't touch them.
>>>       Smoke tested on next-20210209.
>>> v2: Rebased to mainline (v5.11-rc6) and fixed build warnings.
>>>
>>>    .../boot/dts/qcom/sdm845-xiaomi-beryllium.dts      | 64 ++++++++++++++++++++++
>>>    1 file changed, 64 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
>>> index 86cbae63eaf7..5ac049a247e1 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
>>> @@ -157,6 +157,14 @@
>>>                        regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>>>                };
>>>
>>> +             vreg_l14a_1p8: ldo14 {
>>> +                     regulator-min-microvolt = <1800000>;
>>> +                     regulator-max-microvolt = <1800000>;
>>> +                     regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>>> +                     regulator-boot-on;
>>> +                     regulator-always-on;
>>> +             };
>>> +
>>>                vreg_l17a_1p3: ldo17 {
>>>                        regulator-min-microvolt = <1304000>;
>>>                        regulator-max-microvolt = <1304000>;
>>> @@ -191,6 +199,7 @@
>>>                        regulator-min-microvolt = <1200000>;
>>>                        regulator-max-microvolt = <1200000>;
>>>                        regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>>> +                     regulator-boot-on;
>>>                };
>>>        };
>>>    };
>>> @@ -200,6 +209,43 @@
>>>        firmware-name = "qcom/sdm845/cdsp.mdt";
>>>    };
>>>
>>> +&dsi0 {
>>> +     status = "okay";
>>> +     vdda-supply = <&vreg_l26a_1p2>;
>>> +
>>> +     #address-cells = <1>;
>>> +     #size-cells = <0>;
>>> +
>>> +     panel@0 {
>>> +             compatible = "tianma,fhd-video";
>>> +             reg = <0>;
>>> +             vddi0-supply = <&vreg_l14a_1p8>;
>>> +             vddpos-supply = <&lab>;
>>> +             vddneg-supply = <&ibb>;
>>> +
>>> +             #address-cells = <1>;
>>> +             #size-cells = <0>;
>>> +
>>> +             reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
>>> +
>>> +             port {
>>> +                     tianma_nt36672a_in_0: endpoint {
>>> +                             remote-endpoint = <&dsi0_out>;
>>> +                     };
>>> +             };
>>> +     };
>>> +};
>>> +
>>> +&dsi0_out {
>>> +     remote-endpoint = <&tianma_nt36672a_in_0>;
>>> +     data-lanes = <0 1 2 3>;
>>> +};
>>> +
>>> +&dsi0_phy {
>>> +     status = "okay";
>>> +     vdds-supply = <&vreg_l1a_0p875>;
>>> +};
>>> +
>>>    &gcc {
>>>        protected-clocks = <GCC_QSPI_CORE_CLK>,
>>>                           <GCC_QSPI_CORE_CLK_SRC>,
>>> @@ -215,6 +261,24 @@
>>>        };
>>>    };
>>>
>>> +&ibb {
>>> +     regulator-min-microvolt = <4600000>;
>>> +     regulator-max-microvolt = <6000000>;
>>> +};
>>> +
>>
>> I think you want to also configure overvoltage and overcurrent
>> protection values for both LAB and IBB, as these regulators may be a bit
>> dangerous if used without.
> 
> Can you point me to the relevant DT properties please. I didn't find
> any DT properties which set the over voltage/current protection
> properties explicitly in upstream as well as in downstream kernel.
> Plus I also do not see "regulator-min/max-microamp" values set
> downstream, otherwise I'd have used that as well atleast.
> 

The regulator-{min/max}-microamp is the equivalent of the downstream
"qcom,qpnp-lab-limit-maximum-current" and 
"qcom,qpnp-ibb-limit-maximum-current.

I am sorry if we haven't sent our patch series that will introduce the
Sony MSM8998 Yoshino and SDM630/636 Nile and Ganges platforms, which
are actually using the driver that I've sent fully, as these would have
been a nice reference for you.

In any case, I can point you at our public repositories and their
downstream equivalents...

Here you find the downstream configuration for LAB/IBB:
https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.7.1.r1/arch/arm64/boot/dts/qcom/msm8998-yoshino-common.dtsi#L3158

...And here you find the SoMainline upstream stuff for the same:
https://github.com/SoMainline/linux/blob/SoMainline/v5.11-rc4/arch/arm64/boot/dts/qcom/msm8998-sony-xperia-yoshino.dtsi#L332

https://github.com/SoMainline/linux/blob/SoMainline/v5.11-rc4/arch/arm64/boot/dts/qcom/msm8998-sony-xperia-yoshino-maple.dts#L50

I hope that clears out your doubts and difficulties about implementing
the protections on labibb.

Yours,
-- Angelo

> Regards,
> Amit Pundir
> 
>> Besides that, even if it wouldn't be that dangerous, since the
>> protection features are present, it would be nice to configure them
>> properly as in the rare event that something bad happens, you would be
>> able to save the hardware (or at least have a chance to!).
>>
>>> +&lab {
>>> +     regulator-min-microvolt = <4600000>;
>>> +     regulator-max-microvolt = <6000000>;
>>> +};
>>> +
>>
>> Same here.
>>
>> Yours,
>> -- Angelo
>>
>>> +&mdss {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&mdss_mdp {
>>> +     status = "okay";
>>> +};
>>> +
>>>    &mss_pil {
>>>        status = "okay";
>>>        firmware-name = "qcom/sdm845/mba.mbn", "qcom/sdm845/modem.mdt";
>>>
>>