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

[PATCH v2] 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_configure_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>
---

v1->v2:
- Update the commit message

 drivers/i2c/busses/i2c-designware-core.h   |  1 +
 drivers/i2c/busses/i2c-designware-master.c | 11 +++++++++++
 2 files changed, 12 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..b98cab722b4d 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -780,6 +780,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 void i2c_dw_configure_master(struct dw_i2c_dev *dev)
 {
 	struct i2c_timings *t = &dev->timings;
+	u32 ic_con;
 
 	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
 
@@ -788,6 +789,16 @@ void i2c_dw_configure_master(struct dw_i2c_dev *dev)
 
 	dev->mode = DW_IC_MASTER;
 
+	/*
+	 * 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.
+	 */
+	ic_con = ioread32(dev->base + DW_IC_CON);
+	if (ic_con & DW_IC_CON_BUS_CLEAR_CTRL)
+		dev->master_cfg |= DW_IC_CON_BUS_CLEAR_CTRL;
+
 	switch (t->bus_freq_hz) {
 	case I2C_MAX_STANDARD_MODE_FREQ:
 		dev->master_cfg |= DW_IC_CON_SPEED_STD;
-- 
2.25.1

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

[Andy Shevchenko]

On Tue, Jan 17, 2023 at 05:58:01PM +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_configure_master() that if the BIOS
> advertises the bus clear feature, let driver not ignore it and
> adapt accordingly.

...

> +	ic_con = ioread32(dev->base + DW_IC_CON);

Any particular reason why regmap_read() can't be used?

-- 
With Best Regards,
Andy Shevchenko

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

[Shyam Sundar S K]

On 1/17/2023 8:38 PM, Andy Shevchenko wrote:
> On Tue, Jan 17, 2023 at 05:58:01PM +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_configure_master() that if the BIOS
>> advertises the bus clear feature, let driver not ignore it and
>> adapt accordingly.
> 
> ...
> 
>> +	ic_con = ioread32(dev->base + DW_IC_CON);
> 
> Any particular reason why regmap_read() can't be used?

Yes. init_regmap() happens at a later stage in dw_i2c_plat_probe() and
i2c_dw_configure() gets called first.

So dev->map will not be initialized to use regmap_read().

In order to use regmap_read() instead of ioread32() in this case, we
have to defer calling i2c_dw_configure()

Something like this.

--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -311,8 +311,6 @@ static int dw_i2c_plat_probe(struct platform_device
*pdev)
        if (ret)
                goto exit_reset;

-       i2c_dw_configure(dev);
-
        /* Optional interface clock */
        dev->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
        if (IS_ERR(dev->pclk)) {
@@ -378,6 +376,7 @@ static int dw_i2c_plat_probe(struct platform_device
*pdev)
        if (ret)
                goto exit_probe;

+       i2c_dw_configure(dev);
        return ret;

 exit_probe:


What are your thoughts? Should I send a patch to reorder the call flow?

Thanks,
Shyam

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

[Andy Shevchenko]

On Wed, Jan 18, 2023 at 07:27:06PM +0530, Shyam Sundar S K wrote:
> On 1/17/2023 8:38 PM, Andy Shevchenko wrote:
> > On Tue, Jan 17, 2023 at 05:58:01PM +0530, Shyam Sundar S K wrote:

...

> >> +	ic_con = ioread32(dev->base + DW_IC_CON);
> > 
> > Any particular reason why regmap_read() can't be used?
> 
> Yes. init_regmap() happens at a later stage in dw_i2c_plat_probe() and
> i2c_dw_configure() gets called first.
> 
> So dev->map will not be initialized to use regmap_read().
> 
> In order to use regmap_read() instead of ioread32() in this case, we
> have to defer calling i2c_dw_configure()

I think we need to try to be consistent with IO accessors across the driver
which means to try hard to have regmap being initialised beforehand or other
functions being moved accordingly. However, it seems a bit non-trivial
ordering case and I leave this to you, I²C maintainers and this driver
maintainer to decide how to proceed.

-- 
With Best Regards,
Andy Shevchenko

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

[Shyam Sundar S K]

On 1/18/2023 8:04 PM, Andy Shevchenko wrote:
> On Wed, Jan 18, 2023 at 07:27:06PM +0530, Shyam Sundar S K wrote:
>> On 1/17/2023 8:38 PM, Andy Shevchenko wrote:
>>> On Tue, Jan 17, 2023 at 05:58:01PM +0530, Shyam Sundar S K wrote:
> 
> ...
> 
>>>> +	ic_con = ioread32(dev->base + DW_IC_CON);
>>>
>>> Any particular reason why regmap_read() can't be used?
>>
>> Yes. init_regmap() happens at a later stage in dw_i2c_plat_probe() and
>> i2c_dw_configure() gets called first.
>>
>> So dev->map will not be initialized to use regmap_read().
>>
>> In order to use regmap_read() instead of ioread32() in this case, we
>> have to defer calling i2c_dw_configure()
> 
> I think we need to try to be consistent with IO accessors across the driver
> which means to try hard to have regmap being initialised beforehand or other
> functions being moved accordingly. However, it seems a bit non-trivial
> ordering case and I leave this to you, I²C maintainers and this driver
> maintainer to decide how to proceed.
> 

Jarkko, How would you like me to proceed? Would you be OK to pull this
change without regmap_read() or do you like me to submit a patch for
reordering the _configure_* call ?

Thanks,
Shyam

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

[Jarkko Nikula]

On 1/19/23 05:44, Shyam Sundar S K wrote:
> 
> 
> On 1/18/2023 8:04 PM, Andy Shevchenko wrote:
>> On Wed, Jan 18, 2023 at 07:27:06PM +0530, Shyam Sundar S K wrote:
>>> On 1/17/2023 8:38 PM, Andy Shevchenko wrote:
>>>> On Tue, Jan 17, 2023 at 05:58:01PM +0530, Shyam Sundar S K wrote:
>>> In order to use regmap_read() instead of ioread32() in this case, we
>>> have to defer calling i2c_dw_configure()
>>
Comment to change in your previous mail: Moving i2c_dw_configure() after 
i2c_dw_probe() will cause regression to high speed mode since 
DW_IC_CON_SPEED_HIGH is not set when i2c_dw_set_timings_master() is called.

>> I think we need to try to be consistent with IO accessors across the driver
>> which means to try hard to have regmap being initialised beforehand or other
>> functions being moved accordingly. However, it seems a bit non-trivial
>> ordering case and I leave this to you, I²C maintainers and this driver
>> maintainer to decide how to proceed.
>>
Yeah, we don't want to potentially cause a regression on those machines 
that use swapped or word accessors by reading and writing a wrong bit in 
the DW_IC_CON.

> 
> Jarkko, How would you like me to proceed? Would you be OK to pull this
> change without regmap_read() or do you like me to submit a patch for
> reordering the _configure_* call ?
> 
I think safe place to do this is between i2c_dw_init_regmap() and 
dev->init() calls in i2c_dw_probe_master(). Then regmap is initialized 
and the DW_IC_CON is not yet written.

Not sure would reading DW_IC_CON require 
i2c_dw_acquire_lock()/i2c_dw_release_lock() but I would play safe.