[PATCH 1/3] hwmon: pmbus: Reduce clear fault page invocations

[PATCH 1/3] hwmon: pmbus: Reduce clear fault page invocations

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Observing I2C traffic revealed consecutive transmission of CLEAR_FAULT
commands. While this doesn't cause issues, it extends driver probe time.

Avoid invoking pmbus_clear_fault_page for virtual registers, as they're
managed by the driver, not the chip.

TEST: Verified using an I2C bus analyzer that only one CLEAR_FAULT
      command is send instead 5 in a row.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 69a4e62b6c8d..cbfabdd69056 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -561,7 +561,8 @@ static bool pmbus_check_register(struct i2c_client *client,
 		rv = pmbus_check_status_cml(client);
 	if (rv < 0 && (data->flags & PMBUS_READ_STATUS_AFTER_FAILED_CHECK))
 		data->read_status(client, -1);
-	pmbus_clear_fault_page(client, -1);
+	if (reg < PMBUS_VIRT_BASE)
+		pmbus_clear_fault_page(client, -1);
 	return rv >= 0;
 }
 

base-commit: fc8df28be7155ddcf8731b4c85fd3372811ba5ac
-- 
2.41.0

[PATCH 2/3] hwmon: pmbus: Drop unnecessary clear fault page

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

The pmbus_check_byte_register function already calls clear
fault page, so there's no need to do it again in
pmbus_identify_common.

TEST: Verified using an I2C bus analyser to confirm that only
      one clear fault page is being send instead of two in a row.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index cbfabdd69056..1363d9f89181 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2541,7 +2541,6 @@ static int pmbus_identify_common(struct i2c_client *client,
 		}
 	}
 
-	pmbus_clear_fault_page(client, page);
 	return 0;
 }
 
-- 
2.41.0

[PATCH 3/3] hwmon: pmbus: Fix -EIO seen on pli1209

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

After doing performance optimizations the pli1209 driver failed to
probe with a probabilty of 2%. It wasn't able to read the PMBUS_OPERATION
register due to an -EIO error.

An investigation showed that the PLI1209 takes 230 usec to execute the
CLEAR_FAULTS command. During that time it's busy and NACKs all requests
on the SMBUS interface. It also NACKs reads on PMBUS_STATUS_BYTE
making it impossible to poll the BUSY flag.

Add a custom write_data function to just wait for not BUSY unconditionally
after sending a CLEAR_FAULTS command.

TEST: Verified using an I2C bus analyser that no more NACKs are seen after
      sending a CLEAR_FAULTS command.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pli1209bc.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/hwmon/pmbus/pli1209bc.c b/drivers/hwmon/pmbus/pli1209bc.c
index 7d8bd3167b21..c95433790b11 100644
--- a/drivers/hwmon/pmbus/pli1209bc.c
+++ b/drivers/hwmon/pmbus/pli1209bc.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2022 9elements GmbH
  */
 
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pmbus.h>
@@ -53,6 +54,30 @@ static int pli1209bc_read_word_data(struct i2c_client *client, int page,
 	}
 }
 
+static int pli1209bc_write_byte(struct i2c_client *client, int page, u8 reg)
+{
+	int ret;
+
+	switch (reg) {
+	case PMBUS_CLEAR_FAULTS:
+		ret = pmbus_write_byte(client, page, reg);
+		/*
+		 * PLI1209 takes 230 usec to execute the CLEAR_FAULTS command.
+		 * During that time it's busy and NACKs all requests on the
+		 * SMBUS interface. It also NACKs reads on PMBUS_STATUS_BYTE
+		 * making it impossible to poll the BUSY flag.
+		 *
+		 * Just wait for not BUSY unconditionally.
+		 */
+		usleep_range(250, 300);
+		break;
+	default:
+		ret = -ENODATA;
+		break;
+	}
+	return ret;
+}
+
 #if IS_ENABLED(CONFIG_SENSORS_PLI1209BC_REGULATOR)
 static const struct regulator_desc pli1209bc_reg_desc = {
 	.name = "vout2",
@@ -102,6 +127,7 @@ static struct pmbus_driver_info pli1209bc_info = {
 	    | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
 	    | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT,
 	.read_word_data = pli1209bc_read_word_data,
+	.write_byte = pli1209bc_write_byte,
 #if IS_ENABLED(CONFIG_SENSORS_PLI1209BC_REGULATOR)
 	.num_regulators = 1,
 	.reg_desc = &pli1209bc_reg_desc,
-- 
2.41.0

Re: [PATCH 1/3] hwmon: pmbus: Reduce clear fault page invocations

[Guenter Roeck]

On Thu, Aug 17, 2023 at 11:25:24AM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Observing I2C traffic revealed consecutive transmission of CLEAR_FAULT
> commands. While this doesn't cause issues, it extends driver probe time.
> 
> Avoid invoking pmbus_clear_fault_page for virtual registers, as they're
> managed by the driver, not the chip.
> 
> TEST: Verified using an I2C bus analyzer that only one CLEAR_FAULT
>       command is send instead 5 in a row.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> base-commit: fc8df28be7155ddcf8731b4c85fd3372811ba5ac
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 69a4e62b6c8d..cbfabdd69056 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -561,7 +561,8 @@ static bool pmbus_check_register(struct i2c_client *client,
>  		rv = pmbus_check_status_cml(client);
>  	if (rv < 0 && (data->flags & PMBUS_READ_STATUS_AFTER_FAILED_CHECK))
>  		data->read_status(client, -1);
> -	pmbus_clear_fault_page(client, -1);
> +	if (reg < PMBUS_VIRT_BASE)
> +		pmbus_clear_fault_page(client, -1);
>  	return rv >= 0;
>  }
>  

Re: [PATCH 2/3] hwmon: pmbus: Drop unnecessary clear fault page

[Guenter Roeck]

On Thu, Aug 17, 2023 at 11:25:25AM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> The pmbus_check_byte_register function already calls clear
> fault page, so there's no need to do it again in
> pmbus_identify_common.
> 
> TEST: Verified using an I2C bus analyser to confirm that only
>       one clear fault page is being send instead of two in a row.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index cbfabdd69056..1363d9f89181 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2541,7 +2541,6 @@ static int pmbus_identify_common(struct i2c_client *client,
>  		}
>  	}
>  
> -	pmbus_clear_fault_page(client, page);
>  	return 0;
>  }
>  

Re: [PATCH 3/3] hwmon: pmbus: Fix -EIO seen on pli1209

[Guenter Roeck]

On Thu, Aug 17, 2023 at 11:25:26AM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> After doing performance optimizations the pli1209 driver failed to
> probe with a probabilty of 2%. It wasn't able to read the PMBUS_OPERATION
> register due to an -EIO error.
> 
> An investigation showed that the PLI1209 takes 230 usec to execute the
> CLEAR_FAULTS command. During that time it's busy and NACKs all requests
> on the SMBUS interface. It also NACKs reads on PMBUS_STATUS_BYTE
> making it impossible to poll the BUSY flag.
> 
> Add a custom write_data function to just wait for not BUSY unconditionally
> after sending a CLEAR_FAULTS command.
> 
> TEST: Verified using an I2C bus analyser that no more NACKs are seen after
>       sending a CLEAR_FAULTS command.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pli1209bc.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pli1209bc.c b/drivers/hwmon/pmbus/pli1209bc.c
> index 7d8bd3167b21..c95433790b11 100644
> --- a/drivers/hwmon/pmbus/pli1209bc.c
> +++ b/drivers/hwmon/pmbus/pli1209bc.c
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2022 9elements GmbH
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pmbus.h>
> @@ -53,6 +54,30 @@ static int pli1209bc_read_word_data(struct i2c_client *client, int page,
>  	}
>  }
>  
> +static int pli1209bc_write_byte(struct i2c_client *client, int page, u8 reg)
> +{
> +	int ret;
> +
> +	switch (reg) {
> +	case PMBUS_CLEAR_FAULTS:
> +		ret = pmbus_write_byte(client, page, reg);
> +		/*
> +		 * PLI1209 takes 230 usec to execute the CLEAR_FAULTS command.
> +		 * During that time it's busy and NACKs all requests on the
> +		 * SMBUS interface. It also NACKs reads on PMBUS_STATUS_BYTE
> +		 * making it impossible to poll the BUSY flag.
> +		 *
> +		 * Just wait for not BUSY unconditionally.
> +		 */
> +		usleep_range(250, 300);
> +		break;
> +	default:
> +		ret = -ENODATA;
> +		break;
> +	}
> +	return ret;
> +}
> +
>  #if IS_ENABLED(CONFIG_SENSORS_PLI1209BC_REGULATOR)
>  static const struct regulator_desc pli1209bc_reg_desc = {
>  	.name = "vout2",
> @@ -102,6 +127,7 @@ static struct pmbus_driver_info pli1209bc_info = {
>  	    | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
>  	    | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT,
>  	.read_word_data = pli1209bc_read_word_data,
> +	.write_byte = pli1209bc_write_byte,
>  #if IS_ENABLED(CONFIG_SENSORS_PLI1209BC_REGULATOR)
>  	.num_regulators = 1,
>  	.reg_desc = &pli1209bc_reg_desc,