[PATCH v2 1/2] ACPI / PMIC: XPower: optimize I2C-bus accesses

[PATCH v2 1/2] ACPI / PMIC: XPower: optimize I2C-bus accesses

[Hans de Goede]

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;
 }
 
-- 
2.31.1

[PATCH v2 2/2] ACPI / PMIC: XPower: optimize MIPI PMIQ sequence I2C-bus accesses

[Hans de Goede]

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.

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 5750c5e7d4c6..cbe08e600fa3 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -270,10 +270,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

Re: [PATCH v2 2/2] ACPI / PMIC: XPower: optimize MIPI PMIQ sequence I2C-bus accesses

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] ACPI / PMIC: XPower: optimize MIPI PMIQ sequence I2C-bus accesses

[Hans de Goede]

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

Re: [PATCH v2 2/2] ACPI / PMIC: XPower: optimize MIPI PMIQ sequence I2C-bus accesses

[Andy Shevchenko]

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

Re: [PATCH v2 1/2] ACPI / PMIC: XPower: optimize I2C-bus accesses

[Rafael J. Wysocki]

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!