| Message ID | 20210706160923.20273-1-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | e38ba404f20c4beb1a5d4547567d2934a5b95843 |
| Headers | show |
| Series | [v2,1/2] ACPI / PMIC: XPower: optimize I2C-bus accesses | expand |
On Tue, Jul 06, 2021 at 06:09:23PM +0200, Hans de Goede wrote: > The I2C-bus to the XPower AXP288 is shared between the Linux kernel and > the SoCs P-Unit. The P-Unit 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 P-Unit semaphore dance twice. ... The idea for the further improvement > + 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; > + } We have this in intel_pmic.c. Can we reorganize the code the way we will have this check solely in the intel_pmic.c?
Hi, On 7/6/21 6:35 PM, Andy Shevchenko wrote: > On Tue, Jul 06, 2021 at 06:09:23PM +0200, Hans de Goede wrote: >> The I2C-bus to the XPower AXP288 is shared between the Linux kernel and >> the SoCs P-Unit. The P-Unit 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 P-Unit semaphore dance twice. > > ... > > The idea for the further improvement > >> + 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; >> + } > > We have this in intel_pmic.c. Can we reorganize the code the way we will have > this check solely in the intel_pmic.c? No, the drivers/acpi/pmic/intel_pmic_chtwc.c implementation accepts multiple i2c addresses because the whiskey cove has multiple register banks split over different i2c-addressses. Regards, Hans
On Tue, Jul 06, 2021 at 08:27:55PM +0200, Hans de Goede wrote: > On 7/6/21 6:35 PM, Andy Shevchenko wrote: > > On Tue, Jul 06, 2021 at 06:09:23PM +0200, Hans de Goede wrote: ... > > The idea for the further improvement > > > >> + 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; > >> + } > > > > We have this in intel_pmic.c. Can we reorganize the code the way we will have > > this check solely in the intel_pmic.c? > > No, the drivers/acpi/pmic/intel_pmic_chtwc.c implementation accepts multiple > i2c addresses because the whiskey cove has multiple register banks split > over different i2c-addressses. I think it's still possible (by modifying the field to be an array of accepted addresses). Anyway, it's matter outside of this patch series and we have time to think about it. -- With Best Regards, Andy Shevchenko
On Tue, Jul 6, 2021 at 6:09 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The I2C-bus to the XPower AXP288 is shared between the Linux kernel and > the SoCs P-Unit. The P-Unit 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. > > Add iosf_mbi_block_punit_i2c_access() calls around groups of register > accesses, so that the P-Unit semaphore only needs to be taken once > for each group of register accesses. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Do not hold the P-Unit sempahore over the usleep_range() in > intel_xpower_pmic_get_raw_temp() > --- > drivers/acpi/pmic/intel_pmic_xpower.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c > index a091d5a8392c..5750c5e7d4c6 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; > @@ -234,6 +236,11 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) > return ret; > > if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { > + /* > + * AXP288_ADC_TS_PIN_CTRL reads are cached by the regmap, so > + * this does to a single I2C-transfer, and thus there is no > + * need to explicitly call iosf_mbi_block_punit_i2c_access(). > + */ > ret = regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, > AXP288_ADC_TS_CURRENT_ON_OFF_MASK, > AXP288_ADC_TS_CURRENT_ON_ONDEMAND); > @@ -244,6 +251,10 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) > usleep_range(6000, 10000); > } > > + ret = iosf_mbi_block_punit_i2c_access(); > + if (ret) > + return ret; > + > ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); > if (ret == 0) > ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f); > @@ -254,6 +265,8 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) > AXP288_ADC_TS_CURRENT_ON); > } > > + iosf_mbi_unblock_punit_i2c_access(); > + > return ret; > } > > -- Applied along with the [2/2] as 5.15 material, thanks!
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c index a091d5a8392c..5750c5e7d4c6 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; @@ -234,6 +236,11 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) return ret; if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { + /* + * AXP288_ADC_TS_PIN_CTRL reads are cached by the regmap, so + * this does to a single I2C-transfer, and thus there is no + * need to explicitly call iosf_mbi_block_punit_i2c_access(). + */ ret = regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_CURRENT_ON_OFF_MASK, AXP288_ADC_TS_CURRENT_ON_ONDEMAND); @@ -244,6 +251,10 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) usleep_range(6000, 10000); } + ret = iosf_mbi_block_punit_i2c_access(); + if (ret) + return ret; + ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); if (ret == 0) ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f); @@ -254,6 +265,8 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) AXP288_ADC_TS_CURRENT_ON); } + iosf_mbi_unblock_punit_i2c_access(); + return ret; }