Hi,
On 7/2/21 6:50 PM, Hans de Goede wrote:
> The I2C-bus to the XPower AXP288 is shared between the Linux kernel and
> the SoCs PUNIT. The PUNIT has a semaphore which the kernel must "lock"
> before it may use the bus and while the kernel holds the semaphore the CPU
> and GPU power-states must not be changed otherwise the system will freeze.
>
> This is a complex process, which is quite expensive. This is all done by
> iosf_mbi_block_punit_i2c_access(). To ensure that no unguarded I2C-bus
> accesses happen, iosf_mbi_block_punit_i2c_access() gets called by the
> I2C-bus-driver for every I2C transfer. Because this is so expensive it
> is allowed to call iosf_mbi_block_punit_i2c_access() in a nested
> fashion, so that higher-level code which does multiple I2C-transfers can
> call it once for a group of transfers, turning the calls done by the
> I2C-bus-driver into no-ops.
>
> Move / add iosf_mbi_block_punit_i2c_access() calls in / to the XPower
> OpRegion code so that the PUNIT semaphore only needs to be taken once
> for each OpRegion access.
>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
I just noticed that one of the blocks has a usleep_range(6000, 10000)
in there which means that we now hold the P-Unit semaphore over the
sleep, which is not good.
Self nack. I'll send out an updated series fixing this.
Regards,
Hans
> ---
> drivers/acpi/pmic/intel_pmic_xpower.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
> index a091d5a8392c..644a495a4f13 100644
> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> @@ -178,15 +178,17 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
> {
> int data, ret;
>
> - /* GPIO1 LDO regulator needs special handling */
> - if (reg == XPOWER_GPI1_CTRL)
> - return regmap_update_bits(regmap, reg, GPI1_LDO_MASK,
> - on ? GPI1_LDO_ON : GPI1_LDO_OFF);
> -
> ret = iosf_mbi_block_punit_i2c_access();
> if (ret)
> return ret;
>
> + /* GPIO1 LDO regulator needs special handling */
> + if (reg == XPOWER_GPI1_CTRL) {
> + ret = regmap_update_bits(regmap, reg, GPI1_LDO_MASK,
> + on ? GPI1_LDO_ON : GPI1_LDO_OFF);
> + goto out;
> + }
> +
> if (regmap_read(regmap, reg, &data)) {
> ret = -EIO;
> goto out;
> @@ -218,6 +220,10 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
> int ret, adc_ts_pin_ctrl;
> u8 buf[2];
>
> + ret = iosf_mbi_block_punit_i2c_access();
> + if (ret)
> + return ret;
> +
> /*
> * The current-source used for the battery temp-sensor (TS) is shared
> * with the GPADC. For proper fuel-gauge and charger operation the TS
> @@ -231,14 +237,14 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
> */
> ret = regmap_read(regmap, AXP288_ADC_TS_PIN_CTRL, &adc_ts_pin_ctrl);
> if (ret)
> - return ret;
> + goto out;
>
> if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) {
> ret = regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL,
> AXP288_ADC_TS_CURRENT_ON_OFF_MASK,
> AXP288_ADC_TS_CURRENT_ON_ONDEMAND);
> if (ret)
> - return ret;
> + goto out;
>
> /* Wait a bit after switching the current-source */
> usleep_range(6000, 10000);
> @@ -254,6 +260,9 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
> AXP288_ADC_TS_CURRENT_ON);
> }
>
> +out:
> + iosf_mbi_unblock_punit_i2c_access();
> +
> return ret;
> }
>
>
