| Message ID | 20220908171153.670762-2-nfraprado@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: mediatek: asurada: Enable internal display | expand |
Il 08/09/22 19:11, Nícolas F. R. A. Prado ha scritto: > Add the regulators present on the Asurada platform that are used to > power the internal and external displays. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > .../boot/dts/mediatek/mt8192-asurada.dtsi | 114 ++++++++++++++++++ > 1 file changed, 114 insertions(+) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > index 4b314435f8fd..1d99e470ea1a 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi ..snip.. > @@ -56,6 +116,18 @@ pp3300_ldo_z: regulator-3v3-z { > vin-supply = <&ppvar_sys>; > }; > Can you please add a comment here advertising that this regulator will not only provide power to the MIPI bridge, but *also* to the display panel itself? This is to make sure that everyone understands what's going on, and also that we ourselves don't forget about that. Probably something like: /* pp3300_mipibrdg also enables pp3300_panel */ I would then propose to add a "regulator-fixed" that has no GPIO but with vin-supply as this one. pp3300_panel: regulator-3v3-panel { compatible = "regulator-fixed"; regulator-name = "pp3300_panel"; regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; vin-supply = <&pp3300_mipibrdg>; }; I would also test assigning this regulator to the panel node, as this will make sure to cover future corner cases (think about PM suspend/resume). P.S.: If you add the pp3300_panel regulator-fixed with that vin-supply, maybe the proposed comment would become a bit overkill. Your choice! Cheers, Angelo > + pp3300_mipibrdg: regulator-3v3-mipibrdg { > + compatible = "regulator-fixed"; > + regulator-name = "pp3300_mipibrdg"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pp3300_mipibrdg_en_pins>; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + enable-active-high; > + regulator-boot-on; > + gpio = <&pio 127 GPIO_ACTIVE_HIGH>; > + }; > + > /* separately switched 3.3V power rail */ > pp3300_u: regulator-3v3-u { > compatible = "regulator-fixed";
On Fri, Sep 09, 2022 at 09:46:33AM +0200, AngeloGioacchino Del Regno wrote: > Il 08/09/22 19:11, Nícolas F. R. A. Prado ha scritto: > > Add the regulators present on the Asurada platform that are used to > > power the internal and external displays. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > --- > > > > .../boot/dts/mediatek/mt8192-asurada.dtsi | 114 ++++++++++++++++++ > > 1 file changed, 114 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > > index 4b314435f8fd..1d99e470ea1a 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > > +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > > ..snip.. > > > @@ -56,6 +116,18 @@ pp3300_ldo_z: regulator-3v3-z { > > vin-supply = <&ppvar_sys>; > > }; > > Can you please add a comment here advertising that this regulator > will not only provide power to the MIPI bridge, but *also* to the > display panel itself? > > This is to make sure that everyone understands what's going on, and > also that we ourselves don't forget about that. > > Probably something like: > /* pp3300_mipibrdg also enables pp3300_panel */ > > I would then propose to add a "regulator-fixed" that has no GPIO > but with vin-supply as this one. > > pp3300_panel: regulator-3v3-panel { > compatible = "regulator-fixed"; > regulator-name = "pp3300_panel"; > regulator-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > > vin-supply = <&pp3300_mipibrdg>; > }; > > I would also test assigning this regulator to the panel node, as this > will make sure to cover future corner cases (think about PM suspend/resume). > > P.S.: If you add the pp3300_panel regulator-fixed with that vin-supply, > maybe the proposed comment would become a bit overkill. Your choice! Hi Angelo, thanks for the feedback. I think the current layout makes more sense based on my understanding of the power routing here: a single power line output by the pp3300_mipibrdg regulator powers both the ANX chip as well as the panel. So I'm going to keep it the way it is for now. If there are any other concerns please let me know. Thanks, Nícolas > > Cheers, > Angelo > > > + pp3300_mipibrdg: regulator-3v3-mipibrdg { > > + compatible = "regulator-fixed"; > > + regulator-name = "pp3300_mipibrdg"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pp3300_mipibrdg_en_pins>; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + enable-active-high; > > + regulator-boot-on; > > + gpio = <&pio 127 GPIO_ACTIVE_HIGH>; > > + }; > > + > > /* separately switched 3.3V power rail */ > > pp3300_u: regulator-3v3-u { > > compatible = "regulator-fixed"; >
Il 21/09/22 15:48, Nícolas F. R. A. Prado ha scritto: > On Fri, Sep 09, 2022 at 09:46:33AM +0200, AngeloGioacchino Del Regno wrote: >> Il 08/09/22 19:11, Nícolas F. R. A. Prado ha scritto: >>> Add the regulators present on the Asurada platform that are used to >>> power the internal and external displays. >>> >>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> >>> >>> --- >>> >>> .../boot/dts/mediatek/mt8192-asurada.dtsi | 114 ++++++++++++++++++ >>> 1 file changed, 114 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi >>> index 4b314435f8fd..1d99e470ea1a 100644 >>> --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi >>> +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi >> >> ..snip.. >> >>> @@ -56,6 +116,18 @@ pp3300_ldo_z: regulator-3v3-z { >>> vin-supply = <&ppvar_sys>; >>> }; >> >> Can you please add a comment here advertising that this regulator >> will not only provide power to the MIPI bridge, but *also* to the >> display panel itself? >> >> This is to make sure that everyone understands what's going on, and >> also that we ourselves don't forget about that. >> >> Probably something like: >> /* pp3300_mipibrdg also enables pp3300_panel */ >> >> I would then propose to add a "regulator-fixed" that has no GPIO >> but with vin-supply as this one. >> >> pp3300_panel: regulator-3v3-panel { >> compatible = "regulator-fixed"; >> regulator-name = "pp3300_panel"; >> regulator-min-microvolt = <3300000>; >> regulator-max-microvolt = <3300000>; >> >> vin-supply = <&pp3300_mipibrdg>; >> }; >> >> I would also test assigning this regulator to the panel node, as this >> will make sure to cover future corner cases (think about PM suspend/resume). >> >> P.S.: If you add the pp3300_panel regulator-fixed with that vin-supply, >> maybe the proposed comment would become a bit overkill. Your choice! > > Hi Angelo, > > thanks for the feedback. > > I think the current layout makes more sense based on my understanding of the > power routing here: a single power line output by the pp3300_mipibrdg regulator > powers both the ANX chip as well as the panel. So I'm going to keep it the way > it is for now. If there are any other concerns please let me know. > As we discussed, I agree on this decision. Please go on. Cheers! > Thanks, > Nícolas > >> >> Cheers, >> Angelo >> >>> + pp3300_mipibrdg: regulator-3v3-mipibrdg { >>> + compatible = "regulator-fixed"; >>> + regulator-name = "pp3300_mipibrdg"; >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&pp3300_mipibrdg_en_pins>; >>> + regulator-min-microvolt = <3300000>; >>> + regulator-max-microvolt = <3300000>; >>> + enable-active-high; >>> + regulator-boot-on; >>> + gpio = <&pio 127 GPIO_ACTIVE_HIGH>; >>> + }; >>> + >>> /* separately switched 3.3V power rail */ >>> pp3300_u: regulator-3v3-u { >>> compatible = "regulator-fixed"; >>
Hi, On Fri, Sep 9, 2022 at 1:12 AM Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote: > > Add the regulators present on the Asurada platform that are used to > power the internal and external displays. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > .../boot/dts/mediatek/mt8192-asurada.dtsi | 114 ++++++++++++++++++ > 1 file changed, 114 insertions(+) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > index 4b314435f8fd..1d99e470ea1a 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > @@ -23,6 +23,42 @@ memory@40000000 { > reg = <0 0x40000000 0 0x80000000>; > }; > > + pp1000_dpbrdg: regulator-1v0-dpbrdg { > + compatible = "regulator-fixed"; > + regulator-name = "pp1000_dpbrdg"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pp1000_dpbrdg_en_pins>; > + regulator-min-microvolt = <1000000>; > + regulator-max-microvolt = <1000000>; This is fed by a rail called PP1350_VS2, which is from the MT6359 PMIC. And this regulator is a proper LDO. > + enable-active-high; > + regulator-boot-on; > + gpio = <&pio 19 GPIO_ACTIVE_HIGH>; > + }; > + > + pp1000_mipibrdg: regulator-1v0-mipibrdg { > + compatible = "regulator-fixed"; > + regulator-name = "pp1000_mipibrdg"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pp1000_mipibrdg_en_pins>; > + regulator-min-microvolt = <1000000>; > + regulator-max-microvolt = <1000000>; This is fed by a rail called PP1350_VS2, which is from the MT6359 PMIC. And this regulator is a proper LDO. > + enable-active-high; > + regulator-boot-on; > + gpio = <&pio 129 GPIO_ACTIVE_HIGH>; > + }; > + > + pp1800_dpbrdg: regulator-1v8-dpbrdg { > + compatible = "regulator-fixed"; > + regulator-name = "pp1800_dpbrdg"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pp1800_dpbrdg_en_pins>; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; This regulator is only a power switch. Please drop the min/max properties. This is fed by a rail called PP1800_VIO18_U, which is from an LDO on the MT6359 PMIC. > + enable-active-high; > + regulator-boot-on; > + gpio = <&pio 126 GPIO_ACTIVE_HIGH>; > + }; > + > /* system wide LDO 1.8V power rail */ > pp1800_ldo_g: regulator-1v8-g { > compatible = "regulator-fixed"; > @@ -34,6 +70,30 @@ pp1800_ldo_g: regulator-1v8-g { > vin-supply = <&pp3300_g>; > }; > > + pp1800_mipibrdg: regulator-1v8-mipibrdg { > + compatible = "regulator-fixed"; > + regulator-name = "pp1800_mipibrdg"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pp1800_mipibrdg_en_pins>; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; This regulator is only a power switch. Please drop the min/max properties. This is fed by a rail called PP1800_VIO18_U, which is from an LDO on the MT6359 PMIC. > + enable-active-high; > + regulator-boot-on; > + gpio = <&pio 128 GPIO_ACTIVE_HIGH>; > + }; > + > + pp3300_dpbrdg: regulator-3v3-dpbrdg { > + compatible = "regulator-fixed"; > + regulator-name = "pp3300_dpbrdg"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pp3300_dpbrdg_en_pins>; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; This regulator is only a power switch. Please drop the min/max properties. This is fed by a rail called PP3300_G, which is already described below. > + enable-active-high; > + regulator-boot-on; > + gpio = <&pio 26 GPIO_ACTIVE_HIGH>; > + }; > + > /* system wide switching 3.3V power rail */ > pp3300_g: regulator-3v3-g { > compatible = "regulator-fixed"; > @@ -56,6 +116,18 @@ pp3300_ldo_z: regulator-3v3-z { > vin-supply = <&ppvar_sys>; > }; > > + pp3300_mipibrdg: regulator-3v3-mipibrdg { > + compatible = "regulator-fixed"; > + regulator-name = "pp3300_mipibrdg"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pp3300_mipibrdg_en_pins>; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; This regulator is only a power switch. Please drop the min/max properties. This is fed by a rail called PP3300_G, which is already described above. ChenYu > + enable-active-high; > + regulator-boot-on; > + gpio = <&pio 127 GPIO_ACTIVE_HIGH>; > + }; > + > /* separately switched 3.3V power rail */ > pp3300_u: regulator-3v3-u { > compatible = "regulator-fixed"; > @@ -719,6 +791,48 @@ pins-wifi-kill { > }; > }; > > + pp1000_dpbrdg_en_pins: pp1000-dpbrdg-en-pins { > + pins-en { > + pinmux = <PINMUX_GPIO19__FUNC_GPIO19>; > + output-low; > + }; > + }; > + > + pp1000_mipibrdg_en_pins: pp1000-mipibrdg-en-pins { > + pins-en { > + pinmux = <PINMUX_GPIO129__FUNC_GPIO129>; > + output-low; > + }; > + }; > + > + pp1800_dpbrdg_en_pins: pp1800-dpbrdg-en-pins { > + pins-en { > + pinmux = <PINMUX_GPIO126__FUNC_GPIO126>; > + output-low; > + }; > + }; > + > + pp1800_mipibrdg_en_pins: pp1800-mipibrd-en-pins { > + pins-en { > + pinmux = <PINMUX_GPIO128__FUNC_GPIO128>; > + output-low; > + }; > + }; > + > + pp3300_dpbrdg_en_pins: pp3300-dpbrdg-en-pins { > + pins-en { > + pinmux = <PINMUX_GPIO26__FUNC_GPIO26>; > + output-low; > + }; > + }; > + > + pp3300_mipibrdg_en_pins: pp3300-mipibrdg-en-pins { > + pins-en { > + pinmux = <PINMUX_GPIO127__FUNC_GPIO127>; > + output-low; > + }; > + }; > + > pp3300_wlan_pins: pp3300-wlan-pins { > pins-pcie-en-pp3300-wlan { > pinmux = <PINMUX_GPIO143__FUNC_GPIO143>; > -- > 2.37.3 >
Hi Chen-Yu, thank you for the insights on the power supply hierarchy. Just a couple further questions below. On Wed, Sep 21, 2022 at 10:20:43PM +0800, Chen-Yu Tsai wrote: > Hi, > > On Fri, Sep 9, 2022 at 1:12 AM Nícolas F. R. A. Prado > <nfraprado@collabora.com> wrote: > > > > Add the regulators present on the Asurada platform that are used to > > power the internal and external displays. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > --- > > > > .../boot/dts/mediatek/mt8192-asurada.dtsi | 114 ++++++++++++++++++ > > 1 file changed, 114 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > > index 4b314435f8fd..1d99e470ea1a 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > > +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi > > @@ -23,6 +23,42 @@ memory@40000000 { > > reg = <0 0x40000000 0 0x80000000>; > > }; > > > > + pp1000_dpbrdg: regulator-1v0-dpbrdg { > > + compatible = "regulator-fixed"; > > + regulator-name = "pp1000_dpbrdg"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pp1000_dpbrdg_en_pins>; > > + regulator-min-microvolt = <1000000>; > > + regulator-max-microvolt = <1000000>; > > This is fed by a rail called PP1350_VS2, which is from the MT6359 PMIC. > And this regulator is a proper LDO. So, we should have an additional regulator node here called pp1350_vs2 which will feed into pp1000_dpbrdg and that is itself fed in from mt6359_vs2_buck_reg (from mt6359.dtsi). Is that right? Also, is PP1350_VS2 just a simple switch or an LDO? > [..] > > + pp1800_dpbrdg: regulator-1v8-dpbrdg { > > + compatible = "regulator-fixed"; > > + regulator-name = "pp1800_dpbrdg"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pp1800_dpbrdg_en_pins>; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > This regulator is only a power switch. Please drop the min/max properties. > This is fed by a rail called PP1800_VIO18_U, which is from an LDO on the > MT6359 PMIC. Similarly, we should have a pp1800_vio18_u node that is fed in by mt6359_vio18_ldo_reg, right? And is it a switch or an LDO? Thanks, Nícolas > > > + enable-active-high; > > + regulator-boot-on; > > + gpio = <&pio 126 GPIO_ACTIVE_HIGH>; > > + }; > > + [..]
diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi index 4b314435f8fd..1d99e470ea1a 100644 --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi @@ -23,6 +23,42 @@ memory@40000000 { reg = <0 0x40000000 0 0x80000000>; }; + pp1000_dpbrdg: regulator-1v0-dpbrdg { + compatible = "regulator-fixed"; + regulator-name = "pp1000_dpbrdg"; + pinctrl-names = "default"; + pinctrl-0 = <&pp1000_dpbrdg_en_pins>; + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <1000000>; + enable-active-high; + regulator-boot-on; + gpio = <&pio 19 GPIO_ACTIVE_HIGH>; + }; + + pp1000_mipibrdg: regulator-1v0-mipibrdg { + compatible = "regulator-fixed"; + regulator-name = "pp1000_mipibrdg"; + pinctrl-names = "default"; + pinctrl-0 = <&pp1000_mipibrdg_en_pins>; + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <1000000>; + enable-active-high; + regulator-boot-on; + gpio = <&pio 129 GPIO_ACTIVE_HIGH>; + }; + + pp1800_dpbrdg: regulator-1v8-dpbrdg { + compatible = "regulator-fixed"; + regulator-name = "pp1800_dpbrdg"; + pinctrl-names = "default"; + pinctrl-0 = <&pp1800_dpbrdg_en_pins>; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + enable-active-high; + regulator-boot-on; + gpio = <&pio 126 GPIO_ACTIVE_HIGH>; + }; + /* system wide LDO 1.8V power rail */ pp1800_ldo_g: regulator-1v8-g { compatible = "regulator-fixed"; @@ -34,6 +70,30 @@ pp1800_ldo_g: regulator-1v8-g { vin-supply = <&pp3300_g>; }; + pp1800_mipibrdg: regulator-1v8-mipibrdg { + compatible = "regulator-fixed"; + regulator-name = "pp1800_mipibrdg"; + pinctrl-names = "default"; + pinctrl-0 = <&pp1800_mipibrdg_en_pins>; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + enable-active-high; + regulator-boot-on; + gpio = <&pio 128 GPIO_ACTIVE_HIGH>; + }; + + pp3300_dpbrdg: regulator-3v3-dpbrdg { + compatible = "regulator-fixed"; + regulator-name = "pp3300_dpbrdg"; + pinctrl-names = "default"; + pinctrl-0 = <&pp3300_dpbrdg_en_pins>; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + enable-active-high; + regulator-boot-on; + gpio = <&pio 26 GPIO_ACTIVE_HIGH>; + }; + /* system wide switching 3.3V power rail */ pp3300_g: regulator-3v3-g { compatible = "regulator-fixed"; @@ -56,6 +116,18 @@ pp3300_ldo_z: regulator-3v3-z { vin-supply = <&ppvar_sys>; }; + pp3300_mipibrdg: regulator-3v3-mipibrdg { + compatible = "regulator-fixed"; + regulator-name = "pp3300_mipibrdg"; + pinctrl-names = "default"; + pinctrl-0 = <&pp3300_mipibrdg_en_pins>; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + enable-active-high; + regulator-boot-on; + gpio = <&pio 127 GPIO_ACTIVE_HIGH>; + }; + /* separately switched 3.3V power rail */ pp3300_u: regulator-3v3-u { compatible = "regulator-fixed"; @@ -719,6 +791,48 @@ pins-wifi-kill { }; }; + pp1000_dpbrdg_en_pins: pp1000-dpbrdg-en-pins { + pins-en { + pinmux = <PINMUX_GPIO19__FUNC_GPIO19>; + output-low; + }; + }; + + pp1000_mipibrdg_en_pins: pp1000-mipibrdg-en-pins { + pins-en { + pinmux = <PINMUX_GPIO129__FUNC_GPIO129>; + output-low; + }; + }; + + pp1800_dpbrdg_en_pins: pp1800-dpbrdg-en-pins { + pins-en { + pinmux = <PINMUX_GPIO126__FUNC_GPIO126>; + output-low; + }; + }; + + pp1800_mipibrdg_en_pins: pp1800-mipibrd-en-pins { + pins-en { + pinmux = <PINMUX_GPIO128__FUNC_GPIO128>; + output-low; + }; + }; + + pp3300_dpbrdg_en_pins: pp3300-dpbrdg-en-pins { + pins-en { + pinmux = <PINMUX_GPIO26__FUNC_GPIO26>; + output-low; + }; + }; + + pp3300_mipibrdg_en_pins: pp3300-mipibrdg-en-pins { + pins-en { + pinmux = <PINMUX_GPIO127__FUNC_GPIO127>; + output-low; + }; + }; + pp3300_wlan_pins: pp3300-wlan-pins { pins-pcie-en-pp3300-wlan { pinmux = <PINMUX_GPIO143__FUNC_GPIO143>;
Add the regulators present on the Asurada platform that are used to power the internal and external displays. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- .../boot/dts/mediatek/mt8192-asurada.dtsi | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+)