2021-06-29 17:15:28

by Hans de Goede

[permalink] [raw]
Subject: [PATCH v2] mfd: axp20x: Update AXP288 volatile ranges

On Cherry Trail devices with an AXP288 PMIC the external SD-card slot
used the AXP's DLDO2 as card-voltage and either DLDO3 or GPIO1LDO
(GPIO1 pin in low noise LDO mode) as signal-voltage.

These regulators are turned on/off and in case of the signal-voltage
also have their output-voltage changed by the _PS0 and _PS3 power-
management ACPI methods on the MMC-controllers ACPI fwnode as well as
by the _DSM ACPI method for changing the signal voltage.

The AML code implementing these methods is directly accessing the
PMIC through ACPI I2C OpRegion accesses, instead of using the special
PMIC OpRegion handled by drivers/acpi/pmic/intel_pmic_xpower.c .

This means that the contents of the involved PMIC registers can change
without the change being made through the regmap interface, so regmap
should not cache the contents of these registers.

Mark the regulator power on/off, the regulator voltage control and the
GPIO1 control registers as volatile, to avoid regmap caching them.

Specifically this fixes an issue on some models where the i915 driver
toggles another LDO using the same on/off register on/off through
MIPI sequences (through intel_soc_pmic_exec_mipi_pmic_seq_element())
which then writes back a cached on/off register-value where the
card-voltage is off causing the external sdcard slot to stop working
when the screen goes blank, or comes back on again.

The regulator register-range now marked volatile also includes the
buck regulator control registers. This is done on purpose these are
normally not touched by the AML code, but they are updated directly
by the SoC's PUNIT which means that they may also change without going
through regmap.

Note the AXP288 PMIC is only used on Bay- and Cherry-Trail platforms,
so even though this is an ACPI specific problem there is no need to
make the new volatile ranges conditional since these platforms always
use ACPI.

Fixes: dc91c3b6fe66 ("mfd: axp20x: Mark AXP20X_VBUS_IPSOUT_MGMT as volatile")
Fixes: cd53216625a0 ("mfd: axp20x: Fix axp288 volatile ranges")
Reported-and-tested-by: Clamshell <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
Changes in v2:
- Add second Fixes tag
- Add comment to commit msg about the regulator register-range also
including the buck-regulator control registers
---
drivers/mfd/axp20x.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 3eae04e24ac8..db6a21465594 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -125,12 +125,13 @@ static const struct regmap_range axp288_writeable_ranges[] = {

static const struct regmap_range axp288_volatile_ranges[] = {
regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP288_POWER_REASON),
+ regmap_reg_range(AXP22X_PWR_OUT_CTRL1, AXP22X_ALDO3_V_OUT),
regmap_reg_range(AXP288_BC_GLOBAL, AXP288_BC_GLOBAL),
regmap_reg_range(AXP288_BC_DET_STAT, AXP20X_VBUS_IPSOUT_MGMT),
regmap_reg_range(AXP20X_CHRG_BAK_CTRL, AXP20X_CHRG_BAK_CTRL),
regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IPSOUT_V_HIGH_L),
regmap_reg_range(AXP20X_TIMER_CTRL, AXP20X_TIMER_CTRL),
- regmap_reg_range(AXP22X_GPIO_STATE, AXP22X_GPIO_STATE),
+ regmap_reg_range(AXP20X_GPIO1_CTRL, AXP22X_GPIO_STATE),
regmap_reg_range(AXP288_RT_BATT_V_H, AXP288_RT_BATT_V_L),
regmap_reg_range(AXP20X_FG_RES, AXP288_FG_CC_CAP_REG),
};
--
2.31.1


2021-06-30 15:24:13

by Chen-Yu Tsai

[permalink] [raw]
Subject: Re: [PATCH v2] mfd: axp20x: Update AXP288 volatile ranges

On Wed, Jun 30, 2021 at 1:12 AM Hans de Goede <[email protected]> wrote:
>
> On Cherry Trail devices with an AXP288 PMIC the external SD-card slot
> used the AXP's DLDO2 as card-voltage and either DLDO3 or GPIO1LDO
> (GPIO1 pin in low noise LDO mode) as signal-voltage.
>
> These regulators are turned on/off and in case of the signal-voltage
> also have their output-voltage changed by the _PS0 and _PS3 power-
> management ACPI methods on the MMC-controllers ACPI fwnode as well as
> by the _DSM ACPI method for changing the signal voltage.
>
> The AML code implementing these methods is directly accessing the
> PMIC through ACPI I2C OpRegion accesses, instead of using the special
> PMIC OpRegion handled by drivers/acpi/pmic/intel_pmic_xpower.c .
>
> This means that the contents of the involved PMIC registers can change
> without the change being made through the regmap interface, so regmap
> should not cache the contents of these registers.
>
> Mark the regulator power on/off, the regulator voltage control and the
> GPIO1 control registers as volatile, to avoid regmap caching them.
>
> Specifically this fixes an issue on some models where the i915 driver
> toggles another LDO using the same on/off register on/off through
> MIPI sequences (through intel_soc_pmic_exec_mipi_pmic_seq_element())
> which then writes back a cached on/off register-value where the
> card-voltage is off causing the external sdcard slot to stop working
> when the screen goes blank, or comes back on again.
>
> The regulator register-range now marked volatile also includes the
> buck regulator control registers. This is done on purpose these are
> normally not touched by the AML code, but they are updated directly
> by the SoC's PUNIT which means that they may also change without going
> through regmap.
>
> Note the AXP288 PMIC is only used on Bay- and Cherry-Trail platforms,
> so even though this is an ACPI specific problem there is no need to
> make the new volatile ranges conditional since these platforms always
> use ACPI.
>
> Fixes: dc91c3b6fe66 ("mfd: axp20x: Mark AXP20X_VBUS_IPSOUT_MGMT as volatile")
> Fixes: cd53216625a0 ("mfd: axp20x: Fix axp288 volatile ranges")
> Reported-and-tested-by: Clamshell <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>

Reviewed-by: Chen-Yu Tsai <[email protected]>

2021-07-16 07:57:36

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v2] mfd: axp20x: Update AXP288 volatile ranges

On Tue, 29 Jun 2021, Hans de Goede wrote:

> On Cherry Trail devices with an AXP288 PMIC the external SD-card slot
> used the AXP's DLDO2 as card-voltage and either DLDO3 or GPIO1LDO
> (GPIO1 pin in low noise LDO mode) as signal-voltage.
>
> These regulators are turned on/off and in case of the signal-voltage
> also have their output-voltage changed by the _PS0 and _PS3 power-
> management ACPI methods on the MMC-controllers ACPI fwnode as well as
> by the _DSM ACPI method for changing the signal voltage.
>
> The AML code implementing these methods is directly accessing the
> PMIC through ACPI I2C OpRegion accesses, instead of using the special
> PMIC OpRegion handled by drivers/acpi/pmic/intel_pmic_xpower.c .
>
> This means that the contents of the involved PMIC registers can change
> without the change being made through the regmap interface, so regmap
> should not cache the contents of these registers.
>
> Mark the regulator power on/off, the regulator voltage control and the
> GPIO1 control registers as volatile, to avoid regmap caching them.
>
> Specifically this fixes an issue on some models where the i915 driver
> toggles another LDO using the same on/off register on/off through
> MIPI sequences (through intel_soc_pmic_exec_mipi_pmic_seq_element())
> which then writes back a cached on/off register-value where the
> card-voltage is off causing the external sdcard slot to stop working
> when the screen goes blank, or comes back on again.
>
> The regulator register-range now marked volatile also includes the
> buck regulator control registers. This is done on purpose these are
> normally not touched by the AML code, but they are updated directly
> by the SoC's PUNIT which means that they may also change without going
> through regmap.
>
> Note the AXP288 PMIC is only used on Bay- and Cherry-Trail platforms,
> so even though this is an ACPI specific problem there is no need to
> make the new volatile ranges conditional since these platforms always
> use ACPI.
>
> Fixes: dc91c3b6fe66 ("mfd: axp20x: Mark AXP20X_VBUS_IPSOUT_MGMT as volatile")
> Fixes: cd53216625a0 ("mfd: axp20x: Fix axp288 volatile ranges")
> Reported-and-tested-by: Clamshell <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>


Applied, thanks.

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog