Re: [PATCH] arm64: dts: imx8mp: Describe VDD_ARM run and standby voltage for Data Modul i.MX8M Plus eDM SBC

[Shawn Guo <shawnguo@kernel.org>]

On Thu, Oct 12, 2023 at 05:39:08AM +0200, Marek Vasut wrote:
> On 10/11/23 04:12, Shawn Guo wrote:
> > On Tue, Oct 10, 2023 at 03:42:14PM +0200, Marek Vasut wrote:
> > > On 10/10/23 02:58, Shawn Guo wrote:
> > > > On Mon, Oct 09, 2023 at 04:03:01PM +0200, Marek Vasut wrote:
> > > > > On 10/9/23 14:36, Shawn Guo wrote:
> > > > > > On Sun, Oct 08, 2023 at 08:37:34PM +0200, Marek Vasut wrote:
> > > > > > > On 9/24/23 16:21, Shawn Guo wrote:
> > > > > > > > On Thu, Aug 31, 2023 at 08:18:50PM +0200, Marek Vasut wrote:
> > > > > > > > > Describe VDD_ARM (BUCK2) run and standby voltage in DT.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > > > > > ---
> > > > > > > > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > > > > > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > > > > > > Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
> > > > > > > > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > > > > > > > Cc: Magnus Damm <magnus.damm@gmail.com>
> > > > > > > > > Cc: Marek Vasut <marex@denx.de>
> > > > > > > > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > > > > > > > Cc: Peng Fan <peng.fan@nxp.com>
> > > > > > > > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > > > > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > > > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > > > > > Cc: devicetree@vger.kernel.org
> > > > > > > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > > > > > > ---
> > > > > > > > >      arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc.dts | 2 ++
> > > > > > > > >      1 file changed, 2 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc.dts b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc.dts
> > > > > > > > > index 13674dc64be9d..d98a040860a48 100644
> > > > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc.dts
> > > > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc.dts
> > > > > > > > > @@ -362,6 +362,8 @@ buck1: BUCK1 {	/* VDD_SOC (dual-phase with BUCK3) */
> > > > > > > > >      			};
> > > > > > > > >      			buck2: BUCK2 {	/* VDD_ARM */
> > > > > > > > > +				nxp,dvs-run-voltage = <950000>;
> > > > > > > > > +				nxp,dvs-standby-voltage = <850000>;
> > > > > > > > 
> > > > > > > > Buck2 is not turned off in DSM on i.MX8MP?
> > > > > > > 
> > > > > > > It is turned off in SUSPEND/SNVS/OFF , not in IDLE/RUN .
> > > > > > 
> > > > > > Right.  But nxp,dvs-standby-voltage specifies the voltage when PMIC
> > > > > > is in STANDBY mode.  My understanding is that the SoC will be in SUSPEND
> > > > > > state while PMIC is in STANDBY mode.
> > > > > 
> > > > > I agree
> > > > > 
> > > > > > Is it possible that the SoC in
> > > > > > IDLE/RUN while PMIC is in STANDBY mode at all?
> > > > > 
> > > > > No, I don't think so, but there's still the PMIC part:
> > > > > 
> > > > > https://www.nxp.com/docs/en/data-sheet/PCA9450.pdf
> > > > > 
> > > > > 7.3.7 STANDBY mode
> > > > > "
> > > > > PCA9450 transitions to STANDBY mode from RUN mode when
> > > > > both PMIC_ON_REQ and PMIC_STBY_REQ are driven high. BUCK1
> > > > > and BUCK3 output voltage is set to BUCK1OUT_DVS1 and
> > > > > BUCK3OUT_DVS1 and BUCK2 are turned off when DVS_CTRL bit
> > > > > in each BUCKx_CTRL register is configured to 1.
> > > > > "
> > > > > 
> > > > > Specifically
> > > > > "
> > > > > BUCK2 are turned off when DVS_CTRL bit in each
> > > > > BUCKx_CTRL register is configured to 1.
> > > > > "
> > > > > 
> > > > > 8.2.19 0x13 BUCK2CTRL
> > > > > "
> > > > > 4
> > > > > DVS_CTRL
> > > > > DVS Control configuration
> > > > > 0b = BUCK voltage is determined by BUCK2VOUT_DVS0 register regardless of
> > > > > PMIC_STBY_REQ
> > > > > 1b = DVS control through PMIC_STBY_REQ
> > > > > "
> > > > > 
> > > > > Notice that the reset-default is '0b' , so unless the PMIC is reconfigured,
> > > > > the BUCK2 will stay powered on even in STANDBY/SUSPEND.
> > > > 
> > > > Hmm, isn't B2_ENMODE controlling on/off of BUCK2?
> > > > 
> > > > BUCK2 enable mode
> > > > 00b = OFF
> > > > 01b = ON by PMIC_ON_REQ = H
> > > > 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L (default)
> > > > 11b = Always ON
> > > > 
> > > > So unless you reconfigure the field, BUCK2 will be off when PMIC_STBY_REQ
> > > > goes high, right?
> > > 
> > > I think so.
> > > 
> > > But since the buck2 in DVS behavior is user-configurable, and this can be
> > > configured by the bootloader, I would say that it is a good idea to keep the
> > > buck2 DVS value defined in the DT, rather than keep it undefined.
> > > 
> > > Also note that this board is a development kit, so we just don't know what
> > > users might do with it.
> > 
> > Honestly, I just do not see any reason why one would change the most
> > power efficient default configuration to something consuming much more
> > power.
> > 
> > Also, even if DVS control is enabled, 0.85 V is already the target
> > voltage for STANDBY mode by default.
> > 
> > Bottom line is that we should have some comment on this change, because
> > it makes eDM SBC so unique among all those PCA9450 based boards and
> > makes people like me wonder why eDM SBC board needs this setting.
> 
> This is definitely not unique, a lot of i.MX8MP boards configure both run
> and standby voltage for buck2 as well, even the NXP EVK does:
> 
> next-20231011$ git grep -l dvs-standby-voltage
> arch/arm64/boot/dts/freescale/imx8mp*
> arch/arm64/boot/dts/freescale/imx8mp-beacon-som.dtsi
> arch/arm64/boot/dts/freescale/imx8mp-debix-model-a.dts
> arch/arm64/boot/dts/freescale/imx8mp-debix-som-a.dtsi
> arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> arch/arm64/boot/dts/freescale/imx8mp-icore-mx8mp.dtsi
> arch/arm64/boot/dts/freescale/imx8mp-phycore-som.dtsi
> arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi
> arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts
> arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi

Ah, I should have done the grep myself.

> So, can this patch go in as-is or do you want additional comment in V2?

Ok, if board maintainers do not care about the cpu cycles wasted in
pca9450 driver buck_set_dvs() function.  I think I can live with this
unnecessary DT setting.

Shawn