i2c: designware: introduce a custom scl recovery for SoCFPGA platforms
Subsystems (4)
PATCH v6 1/2 i2c: designware: introduce a custom scl recovery for SoCFPGA platforms
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.
---
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(-)
drivers/i2c/busses/i2c-designware-core.h +1 -0
@@ -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)/*
drivers/i2c/busses/i2c-designware-master.c +43 -6
66 lines not on this page · raw message
drivers/i2c/busses/i2c-designware-platdrv.c +1 -0
@@ -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);
Signature · 1 line
2.25.1
PATCH v6 2/2 dt-bindings: i2c: dw: Add Intel's SoCFPGA I2C controller
The I2C pins on Intel's SoCFPGA platform are not connected to GPIOs and thus cannot be recovered by the standard GPIO method.
Document the "intel,socfpga-i2c" binding.
---
v6: no changes
v5: no changes
v4: no changes
v3: no changes
v2: Added Acked-by
--- Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml +2 -0
@@ -33,6 +33,8 @@ properties: - const: snps,designware-i2c - description: Baikal-T1 SoC System I2C controller const: baikal,bt1-sys-i2c + - description: Intel's SoCFPGA I2C controller + const: intel,socfpga-i2c reg: minItems: 1
Signature · 1 line
2.25.1
Quoted 16 lines · Dinh Nguyen
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 <[email protected]>Signed-off-by Dinh Nguyen <[email protected]>---
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 toMODEL_MASK
s/i2c_custom_scl_recovery/i2c_socfpga_scl_recovery
Quoted 6 lines
--- 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(-)
Quoted 4 lines · Dinh Nguyen
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?
drivers/i2c/busses/i2c-designware-master.c static void i2c_dw_unprepare_recovery(struct i2c_adapter *adap) · line 824
Quoted 9 lines
drivers/i2c/busses/i2c-designware-master.c +9
-0+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.
Quoted 6 lines · Wolfram Sang
Quoted 4 lines · Dinh Nguyen
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.
Quoted 6 lines
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_infoshould 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.
Quoted 14 lines
drivers/i2c/busses/i2c-designware-master.c static void i2c_dw_unprepare_recovery(struct i2c_adapter *adap) · line 824
Quoted 10 lines
drivers/i2c/busses/i2c-designware-master.c +9
-0+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
Quoted 7 lines
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.
Quoted 5 lines
Quoted 3 lines
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,
Quoted 8 lines · Wolfram Sang
Quoted 7 lines
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.
Quoted 4 lines
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
Quoted 11 lines · Dinh Nguyen
Hi Wolfram,
Quoted 8 lines · Wolfram Sang
Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Adding a bit more, because of patch:
ca382f5b38f3 ("i2c: designware: add i2c gpio recovery option")
the driver is now not able to reset the controller at all because it has placed a strict dependency on getting a GPIO. Before this patch, during a timeout, there was a simple call to i2c_dw_init_master(), which ultimately resets the master.
Dinh
Hi Dinh,
Quoted 1 line
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