| Message ID | 20230116043500.413499-1-Shyam-sundar.S-k@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: designware: add a new bit check for IC_CON control | expand |
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.
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.
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;
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(+)