Re: [PATCH] power: supply: axp288-charger: Optimize register reading method
From: Hans de Goede
Date: Tue Oct 12 2021 - 03:53:00 EST
Hi,
On 10/12/21 7:45 AM, Kate Hsuan wrote:
> The original implementation access the charger the same register value
> several times to get the charger status, such as online, enabled, and
> bus limits. It takes a long time and bandwidth for every "status get"
> operation.
>
> To reduce the access of the register and save bandwidth, this commit
> integrated every read operation into only one "register value get"
> operation and cache them in the variables. Once the "get properties"
> is requested from the user space, the cached information can be returned
> immediately.
>
> I2C access between Linux kernel and P-Unit is improved by explicitly taking
> semaphore once for the entire set of register accesses in the new
> axp288_charger_usb_update_property() function. The I2C-Bus to the XPower
> AXP288 is shared between the Linux kernel and SoCs P-Unit. The P-Unit
> has a semaphore which the kernel must "lock" before it may use the bus.
> If not explicitly taken by the I2C-Driver, then this semaphore is
> automatically taken by the I2C-bus-driver for each I2C-transfer. In
> other words, the semaphore will be locked and released several times for
> entire set of register accesses.
>
> Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
I've also given this a test run on a device with an AXP288 PMIC:
Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Regards,
Hans
> ---
> drivers/power/supply/axp288_charger.c | 150 +++++++++++++++++---------
> 1 file changed, 99 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index b9553be9bed5..fd4983c98fd9 100644
> --- a/drivers/power/supply/axp288_charger.c
> +++ b/drivers/power/supply/axp288_charger.c
> @@ -22,6 +22,7 @@
> #include <linux/mfd/axp20x.h>
> #include <linux/extcon.h>
> #include <linux/dmi.h>
> +#include <asm/iosf_mbi.h>
>
> #define PS_STAT_VBUS_TRIGGER BIT(0)
> #define PS_STAT_BAT_CHRG_DIR BIT(2)
> @@ -95,6 +96,8 @@
> #define CV_4200MV 4200 /* 4200mV */
> #define CV_4350MV 4350 /* 4350mV */
>
> +#define AXP288_REG_UPDATE_INTERVAL (60 * HZ)
> +
> #define AXP288_EXTCON_DEV_NAME "axp288_extcon"
> #define USB_HOST_EXTCON_HID "INT3496"
> #define USB_HOST_EXTCON_NAME "INT3496:00"
> @@ -118,6 +121,7 @@ struct axp288_chrg_info {
> struct regmap_irq_chip_data *regmap_irqc;
> int irq[CHRG_INTR_END];
> struct power_supply *psy_usb;
> + struct mutex lock;
>
> /* OTG/Host mode */
> struct {
> @@ -138,6 +142,12 @@ struct axp288_chrg_info {
> int cv;
> int max_cc;
> int max_cv;
> +
> + unsigned long last_updated;
> + unsigned int input_status;
> + unsigned int op_mode;
> + unsigned int backend_control;
> + bool valid;
> };
>
> static inline int axp288_charger_set_cc(struct axp288_chrg_info *info, int cc)
> @@ -197,11 +207,8 @@ static inline int axp288_charger_set_cv(struct axp288_chrg_info *info, int cv)
> static int axp288_charger_get_vbus_inlmt(struct axp288_chrg_info *info)
> {
> unsigned int val;
> - int ret;
>
> - ret = regmap_read(info->regmap, AXP20X_CHRG_BAK_CTRL, &val);
> - if (ret < 0)
> - return ret;
> + val = info->backend_control;
>
> val >>= CHRG_VBUS_ILIM_BIT_POS;
> switch (val) {
> @@ -297,55 +304,34 @@ static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
>
> static int axp288_charger_is_present(struct axp288_chrg_info *info)
> {
> - int ret, present = 0;
> - unsigned int val;
> -
> - ret = regmap_read(info->regmap, AXP20X_PWR_INPUT_STATUS, &val);
> - if (ret < 0)
> - return ret;
> + int present = 0;
>
> - if (val & PS_STAT_VBUS_PRESENT)
> + if (info->input_status & PS_STAT_VBUS_PRESENT)
> present = 1;
> return present;
> }
>
> static int axp288_charger_is_online(struct axp288_chrg_info *info)
> {
> - int ret, online = 0;
> - unsigned int val;
> -
> - ret = regmap_read(info->regmap, AXP20X_PWR_INPUT_STATUS, &val);
> - if (ret < 0)
> - return ret;
> + int online = 0;
>
> - if (val & PS_STAT_VBUS_VALID)
> + if (info->input_status & PS_STAT_VBUS_VALID)
> online = 1;
> return online;
> }
>
> static int axp288_get_charger_health(struct axp288_chrg_info *info)
> {
> - int ret, pwr_stat, chrg_stat;
> int health = POWER_SUPPLY_HEALTH_UNKNOWN;
> - unsigned int val;
>
> - ret = regmap_read(info->regmap, AXP20X_PWR_INPUT_STATUS, &val);
> - if ((ret < 0) || !(val & PS_STAT_VBUS_PRESENT))
> + if (!(info->input_status & PS_STAT_VBUS_PRESENT))
> goto health_read_fail;
> - else
> - pwr_stat = val;
>
> - ret = regmap_read(info->regmap, AXP20X_PWR_OP_MODE, &val);
> - if (ret < 0)
> - goto health_read_fail;
> - else
> - chrg_stat = val;
> -
> - if (!(pwr_stat & PS_STAT_VBUS_VALID))
> + if (!(info->input_status & PS_STAT_VBUS_VALID))
> health = POWER_SUPPLY_HEALTH_DEAD;
> - else if (chrg_stat & CHRG_STAT_PMIC_OTP)
> + else if (info->op_mode & CHRG_STAT_PMIC_OTP)
> health = POWER_SUPPLY_HEALTH_OVERHEAT;
> - else if (chrg_stat & CHRG_STAT_BAT_SAFE_MODE)
> + else if (info->op_mode & CHRG_STAT_BAT_SAFE_MODE)
> health = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> else
> health = POWER_SUPPLY_HEALTH_GOOD;
> @@ -362,30 +348,86 @@ static int axp288_charger_usb_set_property(struct power_supply *psy,
> int ret = 0;
> int scaled_val;
>
> + mutex_lock(&info->lock);
> switch (psp) {
> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> scaled_val = min(val->intval, info->max_cc);
> scaled_val = DIV_ROUND_CLOSEST(scaled_val, 1000);
> ret = axp288_charger_set_cc(info, scaled_val);
> - if (ret < 0)
> + if (ret < 0) {
> dev_warn(&info->pdev->dev, "set charge current failed\n");
> + goto out;
> + }
> break;
> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> scaled_val = min(val->intval, info->max_cv);
> scaled_val = DIV_ROUND_CLOSEST(scaled_val, 1000);
> ret = axp288_charger_set_cv(info, scaled_val);
> - if (ret < 0)
> + if (ret < 0) {
> dev_warn(&info->pdev->dev, "set charge voltage failed\n");
> + goto out;
> + }
> break;
> case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> ret = axp288_charger_set_vbus_inlmt(info, val->intval);
> - if (ret < 0)
> + if (ret < 0) {
> dev_warn(&info->pdev->dev, "set input current limit failed\n");
> + goto out;
> + }
> + info->valid = false;
> break;
> default:
> ret = -EINVAL;
> }
>
> +out:
> + mutex_unlock(&info->lock);
> + return ret;
> +}
> +
> +static int axp288_charger_reg_readb(struct axp288_chrg_info *info, int reg, unsigned int *ret_val)
> +{
> + int ret;
> +
> + ret = regmap_read(info->regmap, reg, ret_val);
> + if (ret < 0) {
> + dev_err(&info->pdev->dev, "Error %d on reading value from register 0x%04x\n",
> + ret,
> + reg);
> + return ret;
> + }
> + return 0;
> +}
> +
> +static int axp288_charger_usb_update_property(struct axp288_chrg_info *info)
> +{
> + int ret = 0;
> +
> + if (info->valid && time_before(jiffies, info->last_updated + AXP288_REG_UPDATE_INTERVAL))
> + return 0;
> +
> + dev_dbg(&info->pdev->dev, "Charger updating register values...\n");
> +
> + ret = iosf_mbi_block_punit_i2c_access();
> + if (ret < 0)
> + return ret;
> +
> + ret = axp288_charger_reg_readb(info, AXP20X_PWR_INPUT_STATUS, &info->input_status);
> + if (ret < 0)
> + goto out;
> +
> + ret = axp288_charger_reg_readb(info, AXP20X_PWR_OP_MODE, &info->op_mode);
> + if (ret < 0)
> + goto out;
> +
> + ret = axp288_charger_reg_readb(info, AXP20X_CHRG_BAK_CTRL, &info->backend_control);
> + if (ret < 0)
> + goto out;
> +
> + info->last_updated = jiffies;
> + info->valid = true;
> +out:
> + iosf_mbi_unblock_punit_i2c_access();
> return ret;
> }
>
> @@ -396,6 +438,11 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
> struct axp288_chrg_info *info = power_supply_get_drvdata(psy);
> int ret;
>
> + mutex_lock(&info->lock);
> + ret = axp288_charger_usb_update_property(info);
> + if (ret < 0)
> + goto out;
> +
> switch (psp) {
> case POWER_SUPPLY_PROP_PRESENT:
> /* Check for OTG case first */
> @@ -403,10 +450,7 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
> val->intval = 0;
> break;
> }
> - ret = axp288_charger_is_present(info);
> - if (ret < 0)
> - return ret;
> - val->intval = ret;
> + val->intval = axp288_charger_is_present(info);
> break;
> case POWER_SUPPLY_PROP_ONLINE:
> /* Check for OTG case first */
> @@ -414,10 +458,7 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
> val->intval = 0;
> break;
> }
> - ret = axp288_charger_is_online(info);
> - if (ret < 0)
> - return ret;
> - val->intval = ret;
> + val->intval = axp288_charger_is_online(info);
> break;
> case POWER_SUPPLY_PROP_HEALTH:
> val->intval = axp288_get_charger_health(info);
> @@ -435,16 +476,15 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
> val->intval = info->max_cv * 1000;
> break;
> case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> - ret = axp288_charger_get_vbus_inlmt(info);
> - if (ret < 0)
> - return ret;
> - val->intval = ret;
> + val->intval = axp288_charger_get_vbus_inlmt(info);
> break;
> default:
> - return -EINVAL;
> + ret = -EINVAL;
> }
>
> - return 0;
> +out:
> + mutex_unlock(&info->lock);
> + return ret;
> }
>
> static int axp288_charger_property_is_writeable(struct power_supply *psy,
> @@ -540,7 +580,9 @@ static irqreturn_t axp288_charger_irq_thread_handler(int irq, void *dev)
> dev_warn(&info->pdev->dev, "Spurious Interrupt!!!\n");
> goto out;
> }
> -
> + mutex_lock(&info->lock);
> + info->valid = false;
> + mutex_unlock(&info->lock);
> power_supply_changed(info->psy_usb);
> out:
> return IRQ_HANDLED;
> @@ -613,6 +655,9 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
> if (!(val & PS_STAT_VBUS_VALID)) {
> dev_dbg(&info->pdev->dev, "USB charger disconnected\n");
> axp288_charger_enable_charger(info, false);
> + mutex_lock(&info->lock);
> + info->valid = false;
> + mutex_unlock(&info->lock);
> power_supply_changed(info->psy_usb);
> return;
> }
> @@ -644,6 +689,9 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
> dev_err(&info->pdev->dev,
> "error setting current limit (%d)\n", ret);
>
> + mutex_lock(&info->lock);
> + info->valid = false;
> + mutex_unlock(&info->lock);
> power_supply_changed(info->psy_usb);
> }
>
>