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