| Message ID | 20210702165052.81750-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ACPI / PMIC: XPower: optimize I2C-bus accesses | expand |
On Fri, Jul 2, 2021 at 7:50 PM Hans de Goede <hdegoede@redhat.com> 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. We usually spell "P-Unit" instead of "PUNIT". Otherwise it looks good to me, thanks! Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > 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; > } > > -- > 2.31.1 >
On Fri, Jul 2, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> 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. > > The default exec_mipi_pmic_seq_element implementation from > drivers/acpi/pmic/intel_pmic.c does a regmap_update_bits() call and > the involved registers are typically marked as volatile in the regmap, > so this leads to 2 I2C-bus accesses. > > Add a XPower AXP288 specific implementation of exec_mipi_pmic_seq_element > which calls iosf_mbi_block_punit_i2c_access() calls before the > regmap_update_bits() call to avoid having to do the whole expensive > acquire PUNIT semaphore dance twice. Same as per patch 1. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/pmic/intel_pmic_xpower.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c > index 644a495a4f13..93c516ad361e 100644 > --- a/drivers/acpi/pmic/intel_pmic_xpower.c > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c > @@ -266,10 +266,34 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) > return ret; > } > > +static int intel_xpower_exec_mipi_pmic_seq_element(struct regmap *regmap, > + u16 i2c_address, u32 reg_address, > + u32 value, u32 mask) > +{ > + int ret; > + > + if (i2c_address != 0x34) { > + pr_err("%s: Unexpected i2c-addr: 0x%02x (reg-addr 0x%x value 0x%x mask 0x%x)\n", > + __func__, i2c_address, reg_address, value, mask); > + return -ENXIO; > + } > + > + ret = iosf_mbi_block_punit_i2c_access(); > + if (ret) > + return ret; > + > + ret = regmap_update_bits(regmap, reg_address, mask, value); > + > + iosf_mbi_unblock_punit_i2c_access(); > + > + return ret; > +} > + > static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = { > .get_power = intel_xpower_pmic_get_power, > .update_power = intel_xpower_pmic_update_power, > .get_raw_temp = intel_xpower_pmic_get_raw_temp, > + .exec_mipi_pmic_seq_element = intel_xpower_exec_mipi_pmic_seq_element, > .power_table = power_table, > .power_table_count = ARRAY_SIZE(power_table), > .thermal_table = thermal_table, > -- > 2.31.1 >
Hi, On 7/2/21 7:00 PM, Andy Shevchenko wrote: > On Fri, Jul 2, 2021 at 7:58 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 7/2/21 6:54 PM, Andy Shevchenko wrote: >>> On Fri, Jul 2, 2021 at 7:50 PM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> >> Thank you for the review. >> >> Is your Reviewed-by for just this patch, or for the series ? > > I have not seen a cover letter (is there one?), so by default (i.e. if > not told otherwise) it applies only to this patch. Right, that is why I asked, I had not seen your 2/2 reply yet, I guess I should have just waited a bit longer, sorry about the noise. Regards, Hans
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@redhat.com> 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; > } > >
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; }
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@redhat.com> --- drivers/acpi/pmic/intel_pmic_xpower.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)