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

[PATCH] 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".

Since the current driver implementation completely ignores what
is advertised by BIOS, we just build the master_cfg and
overwrite the entire thing into IC_CON during
i2c_dw_configure_master().

Since, the bus clear feature is not enabled, sometimes there is
no way to reset if the BIT(11) is not set.

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.

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>
---
 drivers/i2c/busses/i2c-designware-core.h   | 1 +
 drivers/i2c/busses/i2c-designware-master.c | 5 +++++
 2 files changed, 6 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..a45d4248caeb 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,10 @@ void i2c_dw_configure_master(struct dw_i2c_dev *dev)
 
 	dev->mode = DW_IC_MASTER;
 
+	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] i2c: designware: add a new bit check for IC_CON control

[Jarkko Nikula]

Hi

On 1/16/23 06:35, 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".
> 
> Since the current driver implementation completely ignores what
> is advertised by BIOS, we just build the master_cfg and
> overwrite the entire thing into IC_CON during
> i2c_dw_configure_master().
> 
> Since, the bus clear feature is not enabled, sometimes there is
> no way to reset if the BIT(11) is not set.
> 
> 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.
> 
> 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>
> ---
>   drivers/i2c/busses/i2c-designware-core.h   | 1 +
>   drivers/i2c/busses/i2c-designware-master.c | 5 +++++
>   2 files changed, 6 insertions(+)
> 
Is this change alone enough? I understood from the specification that 
the SCL/SDA stuck low timeout registers should be set and a bus recovery 
procedure (additional code) is required.

Jarkko

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

[Shyam Sundar S K]

On 1/16/2023 3:08 PM, Jarkko Nikula wrote:
> Hi
> 
> On 1/16/23 06:35, 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".
>>
>> Since the current driver implementation completely ignores what
>> is advertised by BIOS, we just build the master_cfg and
>> overwrite the entire thing into IC_CON during
>> i2c_dw_configure_master().
>>
>> Since, the bus clear feature is not enabled, sometimes there is
>> no way to reset if the BIT(11) is not set.
>>
>> 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.
>>
>> 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>
>> ---
>>   drivers/i2c/busses/i2c-designware-core.h   | 1 +
>>   drivers/i2c/busses/i2c-designware-master.c | 5 +++++
>>   2 files changed, 6 insertions(+)
>>
> Is this change alone enough? I understood from the specification that
> the SCL/SDA stuck low timeout registers should be set and a bus recovery
> procedure (additional code) is required.

Double checked with our HW and FW teams, and understand that (atleast in
AMD platform designs):

1. BIOS actually programs the BUS_CLEAR_FEATURE_CTRL and also 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 this proposed check should be sufficient enough.

Thanks,
Shyam

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

[Andy Shevchenko]

On Tue, Jan 17, 2023 at 04:01:21PM +0530, Shyam Sundar S K wrote:
> On 1/16/2023 3:08 PM, Jarkko Nikula wrote:
> > On 1/16/23 06:35, 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".
> >>
> >> Since the current driver implementation completely ignores what
> >> is advertised by BIOS, we just build the master_cfg and
> >> overwrite the entire thing into IC_CON during
> >> i2c_dw_configure_master().
> >>
> >> Since, the bus clear feature is not enabled, sometimes there is
> >> no way to reset if the BIT(11) is not set.
> >>
> >> 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.
> >>
> >> 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.

...

> > Is this change alone enough? I understood from the specification that
> > the SCL/SDA stuck low timeout registers should be set and a bus recovery
> > procedure (additional code) is required.
> 
> Double checked with our HW and FW teams, and understand that (atleast in
> AMD platform designs):
> 
> 1. BIOS actually programs the BUS_CLEAR_FEATURE_CTRL and also 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 this proposed check should be sufficient enough.

Maybe you should elaborate this in the commit message and/or the code.

-- 
With Best Regards,
Andy Shevchenko

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

[Jarkko Nikula]

On 1/17/23 12:38, Andy Shevchenko wrote:
> On Tue, Jan 17, 2023 at 04:01:21PM +0530, Shyam Sundar S K wrote:
>> Double checked with our HW and FW teams, and understand that (atleast in
>> AMD platform designs):
>>
>> 1. BIOS actually programs the BUS_CLEAR_FEATURE_CTRL and also 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 this proposed check should be sufficient enough.
> 
> Maybe you should elaborate this in the commit message and/or the code.
> 
Yes indeed. Makes obvious this is not dead code if somebody looks only 
the code and specification.

It is also important in the future if somebody adds the same 
functionality into driver that this AMD platform must be taken into 
account in that code change. I.e. no two instances trying to do the same 
recovery.

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

[Shyam Sundar S K]

On 1/17/2023 4:51 PM, Jarkko Nikula wrote:
> On 1/17/23 12:38, Andy Shevchenko wrote:
>> On Tue, Jan 17, 2023 at 04:01:21PM +0530, Shyam Sundar S K wrote:
>>> Double checked with our HW and FW teams, and understand that (atleast in
>>> AMD platform designs):
>>>
>>> 1. BIOS actually programs the BUS_CLEAR_FEATURE_CTRL and also 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 this proposed check should be sufficient enough.
>>
>> Maybe you should elaborate this in the commit message and/or the code.
>>
> Yes indeed. Makes obvious this is not dead code if somebody looks only
> the code and specification.
> 
> It is also important in the future if somebody adds the same
> functionality into driver that this AMD platform must be taken into
> account in that code change. I.e. no two instances trying to do the same
> recovery.

Makes sense. Shall re-spin a v2 with the commit message edits.

Thanks,
Shyam