[PATCH v3] i2c: designware: add a new bit check for IC_CON control

[PATCH v3] i2c: designware: add a new bit check for IC_CON control

[Shyam Sundar S K]

On some AMD platforms, based on the new designware datasheet,
BIOS sets the BIT(11) within the IC_CON register to advertise
the "bus clear feature capability".

AMD/Designware datasheet says:

Bit(11) BUS_CLEAR_FEATURE_CTRL. Read-write,Volatile. Reset: 0.
Description: In Master mode:
- 1'b1: Bus Clear Feature is enabled.
- 1'b0: Bus Clear Feature is Disabled.
In Slave mode, this register bit is not applicable.

On AMD platform designs:
1. BIOS programs the BUS_CLEAR_FEATURE_CTRL and enables the detection
of SCL/SDA stuck low.
2. Whenever the stuck low is detected, the SMU FW shall do the bus
recovery procedure.

Currently, the way in which the "master_cfg" is built in the driver, it
overrides the BUS_CLEAR_FEATURE_CTRL advertised by BIOS and the SMU FW
cannot initiate the bus recovery if the stuck low is detected.

Hence add a check in i2c_dw_probe_master() that if the BIOS
advertises the bus clear feature, let driver not ignore it and
adapt accordingly.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
v2->v3:
- Use regmap_read() instead of ioread32()
- Move the proposed changes to i2c_dw_probe_master() and protect the
  regmap read calls with acquire/release lock calls.

v1->v2:
- Update the commit message

 drivers/i2c/busses/i2c-designware-core.h   |  1 +
 drivers/i2c/busses/i2c-designware-master.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 95ebc5eaa5d1..a7ec8d5d0e72 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -37,6 +37,7 @@
 #define DW_IC_CON_STOP_DET_IFADDRESSED		BIT(7)
 #define DW_IC_CON_TX_EMPTY_CTRL			BIT(8)
 #define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL		BIT(9)
+#define DW_IC_CON_BUS_CLEAR_CTRL		BIT(11)
 
 #define DW_IC_DATA_CMD_DAT			GENMASK(7, 0)
 
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 45f569155bfe..5db71e0a424a 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -865,6 +865,7 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
 {
 	struct i2c_adapter *adap = &dev->adapter;
 	unsigned long irq_flags;
+	u32 ic_con;
 	int ret;
 
 	init_completion(&dev->cmd_complete);
@@ -884,6 +885,24 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
 	if (ret)
 		return ret;
 
+	/*
+	 * On AMD platforms BIOS advertises the bus clear feature
+	 * and enables the SCL/SDA stuck low. SMU FW does the
+	 * bus recovery process. Driver should not ignore this BIOS
+	 * advertisement of bus clear feature.
+	 */
+	ret = i2c_dw_acquire_lock(dev);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(dev->map, DW_IC_CON, &ic_con);
+	i2c_dw_release_lock(dev);
+	if (ret)
+		return ret;
+
+	if (ic_con & DW_IC_CON_BUS_CLEAR_CTRL)
+		dev->master_cfg |= DW_IC_CON_BUS_CLEAR_CTRL;
+
 	ret = dev->init(dev);
 	if (ret)
 		return ret;
-- 
2.25.1

Re: [PATCH v3] i2c: designware: add a new bit check for IC_CON control

[Andy Shevchenko]

On Tue, Jan 24, 2023 at 03:36:50PM +0530, Shyam Sundar S K wrote:
> On some AMD platforms, based on the new designware datasheet,
> BIOS sets the BIT(11) within the IC_CON register to advertise
> the "bus clear feature capability".
> 
> AMD/Designware datasheet says:
> 
> Bit(11) BUS_CLEAR_FEATURE_CTRL. Read-write,Volatile. Reset: 0.
> Description: In Master mode:
> - 1'b1: Bus Clear Feature is enabled.
> - 1'b0: Bus Clear Feature is Disabled.
> In Slave mode, this register bit is not applicable.
> 
> On AMD platform designs:
> 1. BIOS programs the BUS_CLEAR_FEATURE_CTRL and enables the detection
> of SCL/SDA stuck low.
> 2. Whenever the stuck low is detected, the SMU FW shall do the bus
> recovery procedure.
> 
> Currently, the way in which the "master_cfg" is built in the driver, it
> overrides the BUS_CLEAR_FEATURE_CTRL advertised by BIOS and the SMU FW
> cannot initiate the bus recovery if the stuck low is detected.
> 
> Hence add a check in i2c_dw_probe_master() that if the BIOS
> advertises the bus clear feature, let driver not ignore it and
> adapt accordingly.

Fine with me (see one nit-pick below)

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> v2->v3:
> - Use regmap_read() instead of ioread32()
> - Move the proposed changes to i2c_dw_probe_master() and protect the
>   regmap read calls with acquire/release lock calls.
> 
> v1->v2:
> - Update the commit message
> 
>  drivers/i2c/busses/i2c-designware-core.h   |  1 +
>  drivers/i2c/busses/i2c-designware-master.c | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 95ebc5eaa5d1..a7ec8d5d0e72 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -37,6 +37,7 @@
>  #define DW_IC_CON_STOP_DET_IFADDRESSED		BIT(7)
>  #define DW_IC_CON_TX_EMPTY_CTRL			BIT(8)
>  #define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL		BIT(9)
> +#define DW_IC_CON_BUS_CLEAR_CTRL		BIT(11)
>  
>  #define DW_IC_DATA_CMD_DAT			GENMASK(7, 0)
>  
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index 45f569155bfe..5db71e0a424a 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -865,6 +865,7 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
>  {
>  	struct i2c_adapter *adap = &dev->adapter;
>  	unsigned long irq_flags;
> +	u32 ic_con;
>  	int ret;
>  
>  	init_completion(&dev->cmd_complete);
> @@ -884,6 +885,24 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
>  	if (ret)
>  		return ret;

> +	/*
> +	 * On AMD platforms BIOS advertises the bus clear feature
> +	 * and enables the SCL/SDA stuck low. SMU FW does the
> +	 * bus recovery process. Driver should not ignore this BIOS
> +	 * advertisement of bus clear feature.
> +	 */

Move this comment closer to the actual regmap_read() call.

Perhaps you may add here the following

	/* Lock the bus for accessing DW_IC_CON */

> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(dev->map, DW_IC_CON, &ic_con);
> +	i2c_dw_release_lock(dev);
> +	if (ret)
> +		return ret;
> +
> +	if (ic_con & DW_IC_CON_BUS_CLEAR_CTRL)
> +		dev->master_cfg |= DW_IC_CON_BUS_CLEAR_CTRL;
> +
>  	ret = dev->init(dev);
>  	if (ret)
>  		return ret;
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] i2c: designware: add a new bit check for IC_CON control

[Andy Shevchenko]

On Tue, Jan 24, 2023 at 12:27:37PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 24, 2023 at 03:36:50PM +0530, Shyam Sundar S K wrote:

...

> > +	u32 ic_con;


One more nit-pick:

	unsigned int ic_con;

to be same as regmap_read() uses.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] i2c: designware: add a new bit check for IC_CON control

[Andy Shevchenko]

On Tue, Jan 24, 2023 at 04:03:55PM +0530, Shyam Sundar S K wrote:
> On 1/24/2023 3:59 PM, Andy Shevchenko wrote:
> > On Tue, Jan 24, 2023 at 12:27:37PM +0200, Andy Shevchenko wrote:
> >> On Tue, Jan 24, 2023 at 03:36:50PM +0530, Shyam Sundar S K wrote:

...

> >>> +	u32 ic_con;
> > 
> > One more nit-pick:
> > 
> > 	unsigned int ic_con;
> 
> is u32 not typedef of unsigned int?

You should not care about implementation details. You call an API that uses
unsigned int, why not using the correct type from the beginning?

> > to be same as regmap_read() uses.

-- 
With Best Regards,
Andy Shevchenko