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

[Amit Pundir <amit.pundir@linaro.org>]

Hi,

On Fri, 12 Feb 2021 at 23:11, AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> 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.

Thank you for the information.

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

Exactly what I was looking for. Thanks again.

>
> 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";
> >>>
> >>
>