| Message ID | 20220620230109.986298-1-dinguyen@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv6,1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms | expand |
On 6/21/22 02:01, Dinh Nguyen wrote: > The I2C pins on the SoCFPGA platforms do not go through a GPIO module, > thus cannot be recovered by the default method of by doing a GPIO access. > Only a reset of the I2C IP block can a recovery be successful, so this > change effectively resets the I2C controller, NOT any attached clients. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> > --- > v6: Updated commit log to emphasize this change only resets the I2C > controller and not any attached clients. > v5: removed strayed nextline > v4: re-arrange code per Andy Shevchenko's recommendation > v3: simplify the function > update commit message > v2: remove change to MODEL_MASK > s/i2c_custom_scl_recovery/i2c_socfpga_scl_recovery > --- > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-master.c | 49 ++++++++++++++++++--- > drivers/i2c/busses/i2c-designware-platdrv.c | 1 + > 3 files changed, 45 insertions(+), 6 deletions(-) > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Mon, Jun 20, 2022 at 06:01:08PM -0500, Dinh Nguyen wrote: > The I2C pins on the SoCFPGA platforms do not go through a GPIO module, > thus cannot be recovered by the default method of by doing a GPIO access. > Only a reset of the I2C IP block can a recovery be successful, so this > change effectively resets the I2C controller, NOT any attached clients. I am afraid here is a serious misunderstanding. The I2C bus recovery procedure is a documented mechanism how to get a stalled bus back in the case that a client device holds SDA low. This mechanism consists of 9 SCL pulses. A reset of the IP core is *not a recovery*. If SocFPGA cannot togle SCL in some way, it cannot do recovery and adap->bus_recovery_info should be NULL. Or did I miss something? > +static int i2c_socfpga_scl_recovery(struct i2c_adapter *adap) > +{ > + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; > + > + bri->prepare_recovery(adap); > + bri->unprepare_recovery(adap); > + > + return 0; > +} See, this function is named scl_recovery, but there is no SCL involved. This is why I think there is the misunderstanding here.
On 6/21/22 16:37, Wolfram Sang wrote: > On Mon, Jun 20, 2022 at 06:01:08PM -0500, Dinh Nguyen wrote: >> The I2C pins on the SoCFPGA platforms do not go through a GPIO module, >> thus cannot be recovered by the default method of by doing a GPIO access. >> Only a reset of the I2C IP block can a recovery be successful, so this >> change effectively resets the I2C controller, NOT any attached clients. > > I am afraid here is a serious misunderstanding. The I2C bus recovery > procedure is a documented mechanism how to get a stalled bus back in the > case that a client device holds SDA low. This mechanism consists of 9 > SCL pulses. A reset of the IP core is *not a recovery*. If SocFPGA > cannot togle SCL in some way, it cannot do recovery and > adap->bus_recovery_info should be NULL. Or did I miss something? From the original code, the first mechanism to a recovery is to acquire a GPIO for the SCL line and send the 9 SCL pulses, after that, it does a reset of the I2C module. For the SOCFPGA part, there is no GPIO line for the SCL, thus the I2C module cannot even get a reset. This code allows the function to reset the I2C module for SOCFPGA, which is the 2nd part of the recovery process. > >> +static int i2c_socfpga_scl_recovery(struct i2c_adapter *adap) >> +{ >> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; >> + >> + bri->prepare_recovery(adap); >> + bri->unprepare_recovery(adap); >> + >> + return 0; >> +} > > See, this function is named scl_recovery, but there is no SCL involved. > This is why I think there is the misunderstanding here. > I understand your point here. Perhaps just call it i2c_socfpga_recovery()? Dinh
> From the original code, the first mechanism to a recovery is to acquire a > GPIO for the SCL line and send the 9 SCL pulses, after that, it does a reset > of the I2C module. For the SOCFPGA part, there is no GPIO line for the SCL, > thus the I2C module cannot even get a reset. This code allows the function > to reset the I2C module for SOCFPGA, which is the 2nd part of the recovery > process. The second part is totally useless if the client device is holding SDA low. Which is exactly the situation that recovery tries to fix. As I said, if you can't control SCL, you don't have recovery. > > See, this function is named scl_recovery, but there is no SCL involved. > > This is why I think there is the misunderstanding here. > > > > I understand your point here. Perhaps just call it i2c_socfpga_recovery()? No. adap->bus_recovery_info should be NULL.
Hi Wolfram, On 6/22/22 15:07, Wolfram Sang wrote: > >> From the original code, the first mechanism to a recovery is to acquire a >> GPIO for the SCL line and send the 9 SCL pulses, after that, it does a reset >> of the I2C module. For the SOCFPGA part, there is no GPIO line for the SCL, >> thus the I2C module cannot even get a reset. This code allows the function >> to reset the I2C module for SOCFPGA, which is the 2nd part of the recovery >> process. > > The second part is totally useless if the client device is holding SDA > low. Which is exactly the situation that recovery tries to fix. As I > said, if you can't control SCL, you don't have recovery. > This is recovery of the master and not the slave. We have a customer that is the using I2C with the signals routed through the FPGA, and thus are not GPIO. During a timeout, with this code, the driver is able to recover the master. Dinh
Hi Dinh,
> This is recovery of the master and not the slave. We have a customer that
This is a different issue then. bus_recovery_info is only for stuck
clients, as I mentioned before. Resetting the controller can be done
anytime inside the driver by calling some reset routine. You don't need
an I2C core framework for that.
But when to do this reset, and how this relates to real bus recovery,
that you need to deal with the designware maintainers. I don't know the
HW at all.
All the best,
Wolfram
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 70b80e710990..7b22ec1d6a96 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -303,6 +303,7 @@ struct dw_i2c_dev { #define MODEL_MSCC_OCELOT BIT(8) #define MODEL_BAIKAL_BT1 BIT(9) #define MODEL_AMD_NAVI_GPU BIT(10) +#define MODEL_SOCFPGA BIT(11) #define MODEL_MASK GENMASK(11, 8) /* diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 44a94b225ed8..fa2ea162acbd 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -813,10 +813,26 @@ static void i2c_dw_unprepare_recovery(struct i2c_adapter *adap) i2c_dw_init_master(dev); } -static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) +static int i2c_socfpga_scl_recovery(struct i2c_adapter *adap) +{ + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; + + bri->prepare_recovery(adap); + bri->unprepare_recovery(adap); + + return 0; +} + +static int i2c_dw_init_socfpga_recovery_info(struct dw_i2c_dev *dev, + struct i2c_bus_recovery_info *rinfo) +{ + rinfo->recover_bus = i2c_socfpga_scl_recovery; + return 1; +} + +static int i2c_dw_init_generic_recovery_info(struct dw_i2c_dev *dev, + struct i2c_bus_recovery_info *rinfo) { - struct i2c_bus_recovery_info *rinfo = &dev->rinfo; - struct i2c_adapter *adap = &dev->adapter; struct gpio_desc *gpio; gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH); @@ -831,13 +847,34 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) rinfo->sda_gpiod = gpio; rinfo->recover_bus = i2c_generic_scl_recovery; - rinfo->prepare_recovery = i2c_dw_prepare_recovery; - rinfo->unprepare_recovery = i2c_dw_unprepare_recovery; - adap->bus_recovery_info = rinfo; dev_info(dev->dev, "running with gpio recovery mode! scl%s", rinfo->sda_gpiod ? ",sda" : ""); + return 1; +} + +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) +{ + struct i2c_bus_recovery_info *rinfo = &dev->rinfo; + struct i2c_adapter *adap = &dev->adapter; + int ret; + + switch (dev->flags & MODEL_MASK) { + case MODEL_SOCFPGA: + ret = i2c_dw_init_socfpga_recovery_info(dev, rinfo); + break; + default: + ret = i2c_dw_init_generic_recovery_info(dev, rinfo); + break; + } + if (ret <= 0) + return ret; + + rinfo->prepare_recovery = i2c_dw_prepare_recovery; + rinfo->unprepare_recovery = i2c_dw_unprepare_recovery; + adap->bus_recovery_info = rinfo; + return 0; } diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 70ade5306e45..b33e015e6732 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -153,6 +153,7 @@ static const struct of_device_id dw_i2c_of_match[] = { { .compatible = "snps,designware-i2c", }, { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT }, { .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 }, + { .compatible = "intel,socfpga-i2c", .data = (void *)MODEL_SOCFPGA }, {}, }; MODULE_DEVICE_TABLE(of, dw_i2c_of_match);