Re: [PATCH 1/2] ACPI / PMIC: XPower: optimize I2C-bus accesses
From: Hans de Goede <hidden>
Date: 2021-07-05 18:16:53
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 <redacted>
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
quoted hunk ↗ jump to hunk
--- 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; }