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