[PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[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 <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(-)

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);
-- 
2.25.1

[PATCHv6 2/2] dt-bindings: i2c: dw: Add Intel's SoCFPGA I2C controller

[Dinh Nguyen]

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.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
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(+)

diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
index d9293c57f573..a130059e97ab 100644
--- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
@@ -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
-- 
2.25.1

Re: [PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[Jarkko Nikula]

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>

Re: [PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[Dinh Nguyen]

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

Re: [PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[Dinh Nguyen]

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

Re: [PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[Dinh Nguyen]

On 7/12/22 07:41, Dinh Nguyen wrote:
> 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.
> 

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

Re: [PATCHv6 1/2] i2c: designware: introduce a custom scl recovery for SoCFPGA platforms

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]