RFC: i2c designware gpio recovery

Hi

After sending this mail i just found out how i could reset the i2c-1 controller manually with devmem 0xffd05014 32 0x2000
devmem 0xffd05014 32 0

So i took a look into the device tree file socfpga.dtsi and found that the reset lines where not defined (although available in the corresponding reset manager). Is there a reason for this? Other components are connected.

However with the patch below my previously sent patch works!

If there is interest in would cleanup the patch and send it in for mainlining. I think the most unacceptable part would be this line:
+ ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN |
My gpio drivers refuse to work as output as they have no open drain mode. So i wonder how to get this solved in a clean manner.

Best regards
Tim

---
  arch/arm/boot/dts/socfpga.dtsi | 4 ++++
  1 file changed, 4 insertions(+)

arch/arm/boot/dts/socfpga.dtsi +4 -0

index 2c43c4d85dee..5f28632bc88c 100644
@@ -643,6 +643,7 @@
                         #size-cells = <0>;
                         compatible = "snps,designware-i2c";
                         reg = <0xffc04000 0x1000>;
+                       resets = <&rst I2C0_RESET>;
                         clocks = <&l4_sp_clk>;
                         interrupts = <0 158 0x4>;
                         status = "disabled";

@@ -653,6 +654,7 @@
                         #size-cells = <0>;
                         compatible = "snps,designware-i2c";
                         reg = <0xffc05000 0x1000>;
+                       resets = <&rst I2C1_RESET>;
                         clocks = <&l4_sp_clk>;
                         interrupts = <0 159 0x4>;
                         status = "disabled";

@@ -663,6 +665,7 @@
                         #size-cells = <0>;
                         compatible = "snps,designware-i2c";
                         reg = <0xffc06000 0x1000>;
+                       resets = <&rst I2C2_RESET>;
                         clocks = <&l4_sp_clk>;
                         interrupts = <0 160 0x4>;
                         status = "disabled";

@@ -673,6 +676,7 @@
                         #size-cells = <0>;
                         compatible = "snps,designware-i2c";
                         reg = <0xffc07000 0x1000>;
+                       resets = <&rst I2C3_RESET>;
                         clocks = <&l4_sp_clk>;
                         interrupts = <0 161 0x4>;
                         status = "disabled";

Quoted 4 lines

So i took a look into the device tree file socfpga.dtsi and found that the reset lines where not defined (although available in the corresponding reset manager). Is there a reason for this? Other components are connected.

There's a few thing like that where the bootloader has been expected to setup the resets etc.

Quoted 7 lines

However with the patch below my previously sent patch works!

If there is interest in would cleanup the patch and send it in for mainlining. I think the most unacceptable part would be this line:
+ ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN |
My gpio drivers refuse to work as output as they have no open drain mode. So i wonder how to get this solved in a clean manner.

I thought the gpio system would emulate open drain by switching the pin between an input and output driven low in this case. How are you configuring the GPIO's in the FPGA?

Given a couple of days I can test this on some flack i2c hardware I have with a Cyclone-V SOC. I'm interested in the functionality as well.

For i2c that are connected to the dedicated HPS pins it should be possible to reconfigure the pin mux controller (see system manager) in the HPS to avoid the need to go thru the fpga to get direct control. The docs say this is "unsupport" but I've done some test and it does seem to work.

I'm guess
the no support is in a similar vain to the emac ptp FPGA interface couldn't be used when the HPS pin where used. But that got changed when the user's proved otherwise. There's just no pin ctrl driver yet to manage it.

Good Day Phil

Am Montag, 1. Mai 2017, 09:57:35 CEST schrieb Phil Reid:

Quoted 5 lines

Quoted 4 lines

So i took a look into the device tree file socfpga.dtsi and found that the reset lines where not defined (although available in the corresponding reset manager). Is there a reason for this? Other components are connected.

Quoted 2 lines

There's a few thing like that where the bootloader has been expected to setup the resets etc.

Yes, but if the resets are not connected in the device tree, the linux drivers are not going to use them?

Quoted 9 lines

Quoted 8 lines

However with the patch below my previously sent patch works!

If there is interest in would cleanup the patch and send it in for mainlining. I think the most unacceptable part would be this line:
+ ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN |
My gpio drivers refuse to work as output as they have no open drain mode. So i wonder how to get this solved in a clean manner.

Quoted 3 lines

I thought the gpio system would emulate open drain by switching the pin between an input and output driven low in this case. How are you configuring the GPIO's in the FPGA?

I don't switch to GPIO mode. As the I2C logic is only pulling active low, i only do a wired and with the gpio (implemented in the fpga) port output on the output enable line for the SCL output. SDA is only an additional input for the second in fpga gpio port.

A picture should make it a clearer:

gpio scl--\
i2c scl --&---i2c mode output pin (configured as fpga loan)

In my case the original i2c pins where occupied by some other logic conflicting so the i2c pins had to be shifted to some other pins using fpga logic. So it was just a matter of adding a two port gpio port.

Quoted 3 lines

Given a couple of days I can test this on some flack i2c hardware I have with a Cyclone-V SOC. I'm interested in the functionality as well.

Sounds good. If you need some further input how i have configured the fpga drop me a line.

Quoted 4 lines

For i2c that are connected to the dedicated HPS pins it should be possible to reconfigure the pin mux controller (see system manager) in the HPS to avoid the need to go thru the fpga to get direct control. The docs say this is "unsupport" but I've done some test and it does seem to work.

As far as i know the internal jtag chain is only used in the bootloader and there is no linux driver? But it shouldn't be a too big problem to port it to linux.

What i am unsure about is the fact that the internal jtag chain which controls the pinmuxing might wreak havoc on other pin states if you reconfigure it?

Quoted 5 lines

I'm guess
the no support is in a similar vain to the emac ptp FPGA interface couldn't be used when the HPS pin where used. But that got changed when the user's proved otherwise. There's just no pin ctrl driver yet to manage it.

I am interested in this ptp solution too. Is there anything on the way to mainline?

G'day Tim,

Quoted 12 lines · Tim Sander

Good Day Phil

Am Montag, 1. Mai 2017, 09:57:35 CEST schrieb Phil Reid:

Quoted 8 lines

Quoted 5 lines

So i took a look into the device tree file socfpga.dtsi and found that the
reset lines where not defined (although available in the corresponding reset manager). Is there a reason for this? Other components are connected.

There's a few thing like that where the bootloader has been expected to setup the resets etc.

Quoted 2 lines

Yes, but if the resets are not connected in the device tree, the linux drivers are not going to use them?

Yes, so they should be added. I don't think we should assume the bootloader sets things up. But that doesn't seem to have been the assumption with the Alter SOC's.

Quoted 20 lines

Quoted 9 lines

Quoted 8 lines

However with the patch below my previously sent patch works!

If there is interest in would cleanup the patch and send it in for mainlining. I think the most unacceptable part would be this line:
+ ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN |
My gpio drivers refuse to work as output as they have no open drain mode.
So i wonder how to get this solved in a clean manner.

Quoted 3 lines

I thought the gpio system would emulate open drain by switching the pin between an input and output driven low in this case. How are you configuring the GPIO's in the FPGA?

I don't switch to GPIO mode. As the I2C logic is only pulling active low, i only do a wired and with the gpio (implemented in the fpga) port output on the output enable line for the SCL output. SDA is only an additional input for the second in fpga gpio port.

A picture should make it a clearer:

Quoted 6 lines

gpio scl--\
i2c scl --&---i2c mode output pin (configured as fpga loan)

In my case the original i2c pins where occupied by some other logic conflicting so the i2c pins had to be shifted to some other pins using fpga logic. So it was just a matter of adding a two port gpio port.

I think I understand. What soft core gpio controller are you using?

Quoted 17 lines

Quoted 2 lines

Given a couple of days I can test this on some flack i2c hardware I have with a Cyclone-V SOC. I'm interested in the functionality as well.

Sounds good. If you need some further input how i have configured the fpga drop me a line.

Quoted 6 lines

For i2c that are connected to the dedicated HPS pins it should be possible
to reconfigure the pin mux controller (see system manager) in the HPS to avoid the need to go thru the fpga to get direct control. The docs say this
is "unsupport" but I've done some test and it does seem to work.

As far as i know the internal jtag chain is only used in the bootloader and there is no linux driver? But it shouldn't be a too big problem to port it to linux.

Quoted 3 lines

What i am unsure about is the fact that the internal jtag chain which controls the pinmuxing might wreak havoc on other pin states if you reconfigure it?

Have a look at the Cyclone V handbook "pin mux control Group REgister Descriptions" From what I can see the chain is used to configure IO standards and drive strength. But not the actual muxes

Quoted 6 lines

Quoted 5 lines

I'm guess
the no support is in a similar vain to the emac ptp FPGA interface couldn't
be used when the HPS pin where used. But that got changed when the user's proved otherwise. There's just no pin ctrl driver yet to manage it.

Quoted 2 lines

I am interested in this ptp solution too. Is there anything on the way to mainline?

This was working the last time I tried it. I submitted a couple of minor patches for it a while ago. My hardware has a DSA switch attached to the ethernet port and so far I haven't figured out how to enable ptp when using the virtual lan ports on the DSA. But it worked fine when directly connected to a phy.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

So i took a look into the device tree file socfpga.dtsi and found that the
reset lines where not defined (although available in the corresponding reset manager). Is there a reason for this? Other components are connected.

There's a few thing like that where the bootloader has been expected to setup the resets etc.

Quoted 6 lines

Quoted 2 lines

Yes, but if the resets are not connected in the device tree, the linux drivers are not going to use them?

Yes, so they should be added. I don't think we should assume the bootloader sets things up. But that doesn't seem to have been the assumption with the Alter SOC's.

I will prepare a patch for this.

Quoted 23 lines

Quoted 23 lines

Quoted 9 lines

Quoted 8 lines

However with the patch below my previously sent patch works!

If there is interest in would cleanup the patch and send it in for mainlining. I think the most unacceptable part would be this line:
+ ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN |
My gpio drivers refuse to work as output as they have no open drain mode.
So i wonder how to get this solved in a clean manner.

Quoted 3 lines

I thought the gpio system would emulate open drain by switching the pin between an input and output driven low in this case. How are you configuring the GPIO's in the FPGA?

I don't switch to GPIO mode. As the I2C logic is only pulling active low, i only do a wired and with the gpio (implemented in the fpga) port output on the output enable line for the SCL output. SDA is only an additional input for the second in fpga gpio port.

A picture should make it a clearer:

gpio scl--\
i2c scl --&---i2c mode output pin (configured as fpga loan)

Quoted 5 lines

Quoted 3 lines

In my case the original i2c pins where occupied by some other logic conflicting so the i2c pins had to be shifted to some other pins using fpga logic. So it was just a matter of adding a two port gpio port.

I think I understand. What soft core gpio controller are you using?

I am using the standard altera fpga gpios.

This patch contains much input from Phil Reid and has been tested on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the SCL and SDA GPIO's. I am still a little unsure about the recover in the timeout case (i2c-designware-core.c:770) as i could not test this codepath.

drivers/i2c/busses/i2c-designware-core.c +0 -0

@@ -317,6 +317,7 @@ static void i2c_dw_release_lock(struct dw_i2c_dev
 *dev)
                 dev->release_lock(dev);
  }

+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data

drivers/i2c/busses/i2c-designware-core.c +0 -0

@@ -463,7 +464,12 @@ static int i2c_dw_wait_bus_not_busy(struct
 dw_i2c_dev *dev)
         while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
 
                 if (timeout <= 0) {

                        dev_warn(dev->dev, "timeout waiting for bus ready\n");
-                       return -ETIMEDOUT;
+                       i2c_recover_bus(&dev->adapter); +
+                       if (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY)
+                               return -ETIMEDOUT;

+                       else

+                               return 0;
                }

                timeout--;
                usleep_range(1000, 1100);

(quoted diff) +0 -0

@@ -719,9 +725,10 @@ static int i2c_dw_handle_tx_abort(struct
 dw_i2c_dev *dev)
         for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
                 dev_err(dev->dev, "%s: %s\n", __func__,
 abort_sources[i]);

-       if (abort_source & DW_IC_TX_ARB_LOST)
+       if (abort_source & DW_IC_TX_ARB_LOST) {
+               i2c_recover_bus(&dev->adapter);
                return -EAGAIN;

-       else if (abort_source & DW_IC_TX_ABRT_GCALL_READ) +       } else if (abort_source & DW_IC_TX_ABRT_GCALL_READ) return -EINVAL; /* wrong msgs[] data */ else

                return -EIO;

+#include <linux/gpio.h>

drivers/i2c/busses/i2c-designware-platdrv.c +3 -0

+
+       return gpiod_get_value_cansleep(dev->gpio_scl);
+}
 

+               return PTR_ERR(dev->gpio_scl);
+
+       dev->gpio_sda = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);

+       if (IS_ERR_OR_NULL(dev->gpio_sda))

+               return PTR_ERR(dev->gpio_sda);

+       rinfo->scl_gpio = desc_to_gpio(dev->gpio_scl); +       rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);

+       r = i2c_dw_init_recovery_info(dev, adap);
+       if (r  == -EPROBE_DEFER)

+               goto exit_probe;
+
        r = i2c_dw_probe(dev);
        if (r)
                goto exit_probe;

-       return r;
+       return 0;

Quoted 5 lines · Tim Sander

This patch contains much input from Phil Reid and has been tested on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the SCL and SDA GPIO's. I am still a little unsure about the recover in the timeout case (i2c-designware-core.c:770) as i could not test this codepath.

Since it's not an RFC anymore let me do some comments on the below.

Quoted 9 lines

drivers/i2c/busses/i2c-designware-core.c +1 -0

@@ -317,6 +317,7 @@ static void i2c_dw_release_lock(struct dw_i2c_dev
 *dev)
                 dev->release_lock(dev);
  }
 
+
  /**
   * i2c_dw_init() - initialize the designware i2c master hardware

* @dev: device private data

This doesn't belong to the change.

Quoted 12 lines

drivers/i2c/busses/i2c-designware-core.c +3 -1

@@ -463,7 +464,12 @@ static int i2c_dw_wait_bus_not_busy(struct
 dw_i2c_dev *dev)
         while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
 
                 if (timeout <= 0) {
                         dev_warn(dev->dev, "timeout waiting for bus
 ready\n");
-                       return -ETIMEDOUT;
+                       i2c_recover_bus(&dev->adapter);
+
+                       if (dw_readl(dev, DW_IC_STATUS) &

DW_IC_STATUS_ACTIVITY)

drivers/i2c/busses/i2c-designware-core.c static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev) · line 471

Quoted 3 lines

drivers/i2c/busses/i2c-designware-core.c +2 -0

+                               return -ETIMEDOUT;
 
+                       else

Redundant.

Quoted 2 lines

+                               return 0;
                 }

Actually I would rather refactor first above function:
1) to be do {} while();
2) to have invariant condition out of the loop.

Quoted 16 lines

                 timeout--;
                 usleep_range(1000, 1100);

drivers/i2c/busses/i2c-designware-core.c +3 -2

@@ -719,9 +725,10 @@ static int i2c_dw_handle_tx_abort(struct
 dw_i2c_dev *dev)
         for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
                 dev_err(dev->dev, "%s: %s\n", __func__,
 abort_sources[i]);
 
-       if (abort_source & DW_IC_TX_ARB_LOST)
+       if (abort_source & DW_IC_TX_ARB_LOST) {
+               i2c_recover_bus(&dev->adapter);
                 return -EAGAIN;
 
-       else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
+       } else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)

Quoted 2 lines

                 return -EINVAL; /* wrong msgs[] data */
         else

Both else:s are redundant.

	if (abort_source & DW_IC_TX_ARB_LOST) {
                i2c_recover_bus(&dev->adapter);
                 return -EAGAIN;
	}

         if (abort_source & DW_IC_TX_ABRT_GCALL_READ)

...

Though I may agree on leaving them here for sake of keeping less lines of code.

Quoted 3 lines

                 return -EIO;

+#include <linux/gpio.h>

I think it should be

#include <linux/gpio/consumer.h>

drivers/i2c/busses/i2c-designware-platdrv.c line 45

Quoted 8 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -0

@@ -41,6 +41,7 @@
  #include <linux/reset.h>
  #include <linux/slab.h>
  #include <linux/acpi.h>
 
+#include <linux/of_gpio.h>

No, please don't.

In recent code we try to avoid OF/ACPI/platform specific bits if there is a common resource provider (and API) for that. GPIO is the case.

drivers/i2c/busses/i2c-designware-platdrv.c static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id) · line 202

Quoted 6 lines

drivers/i2c/busses/i2c-designware-platdrv.c +5 -0

+void i2c_dw_unprepare_recovery(struct i2c_adapter *adap)
+{
 
+}
+
+

Remove extra line.

drivers/i2c/busses/i2c-designware-platdrv.c static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id) · line 206

Quoted 4 lines

drivers/i2c/busses/i2c-designware-platdrv.c +4 -0

+static int i2c_dw_get_scl(struct i2c_adapter *adap)
+{
+       struct platform_device *pdev = to_platform_device(&adap->dev);
+       struct dw_i2c_dev *dev = platform_get_drvdata(pdev);

struct dw_i2c_dev *dev = i2c_get_adapdata(adap); ?

Ditto for all occurrences in the code.

Quoted 12 lines

drivers/i2c/busses/i2c-designware-platdrv.c +11 -0

+
+       return gpiod_get_value_cansleep(dev->gpio_scl);
+}
 
+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
+                                    struct i2c_adapter *adap)
+{
+       struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+       dev->gpio_scl = devm_gpiod_get_optional(dev->dev,
+                                               "scl",
+                                               GPIOD_OUT_HIGH);

Quoted 1 line

+ if (IS_ERR_OR_NULL(dev->gpio_scl))

This is wrong. You should not use this macro in most cases. And especially it breaks the logic behind _optional().

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c +3 -0

+               return PTR_ERR(dev->gpio_scl);
+
+       dev->gpio_sda = devm_gpiod_get_optional(dev->dev, "sda",

GPIOD_IN);

+ if (IS_ERR_OR_NULL(dev->gpio_sda))

Ditto.

drivers/i2c/busses/i2c-designware-platdrv.c static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id) · line 243

Quoted 4 lines

drivers/i2c/busses/i2c-designware-platdrv.c +3 -0

+               return PTR_ERR(dev->gpio_sda);
 
+       rinfo->scl_gpio = desc_to_gpio(dev->gpio_scl);
+       rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);

Why?!

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct · line 373

Quoted 10 lines

drivers/i2c/busses/i2c-designware-platdrv.c +2 -0

+};

@@ -285,6 +368,7 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
         adap->class = I2C_CLASS_DEPRECATED;
         ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
         adap->dev.of_node = pdev->dev.of_node;
+       snprintf(adap->name, sizeof(adap->name), "Designware i2c
 adapter");

It looks like a separate change.

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 383

Quoted 3 lines

drivers/i2c/busses/i2c-designware-platdrv.c +2 -0

+       r = i2c_dw_init_recovery_info(dev, adap);
+       if (r  == -EPROBE_DEFER)

Remove extra space.

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev)

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c +3 -1

+               goto exit_probe;
+
         r = i2c_dw_probe(dev);
         if (r)
                 goto exit_probe;
 
 
-       return r;
+       return 0;

Doesn't belong to the change.

Don't change arbitrary typos or do small "improvements" in the change which is not about them.

G'day Andy,

Thanks for the review.

Quoted 14 lines · Tim Sander

drivers/i2c/busses/i2c-designware-platdrv.c static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id) · line 235

Quoted 10 lines

drivers/i2c/busses/i2c-designware-platdrv.c +9 -0

+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
+                                    struct i2c_adapter *adap)
+{
+       struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+       dev->gpio_scl = devm_gpiod_get_optional(dev->dev,
+                                               "scl",
+                                               GPIOD_OUT_HIGH);
+       if (IS_ERR_OR_NULL(dev->gpio_scl))

This is wrong. You should not use this macro in most cases. And especially it breaks the logic behind _optional().

My logic here was that if the gpio is optional return null we return 0.

which is an okay status.
But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've never
quite wrapped my head around why that's the case.

But the probe function only bails out if this returns EPROBE_DEFER. Not sure that's the best approach

Quoted 5 lines

Quoted 3 lines

+               return PTR_ERR(dev->gpio_sda); +       rinfo->scl_gpio = desc_to_gpio(dev->gpio_scl); +       rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);

Why?!

 From my first attempt, didn't remove it from the example I sent.

We could change i2c_init_recovery to something like the following then the gpio set / getter could use the default functions. Not sure the code is completely correct but hopefully you get the concept.

static void i2c_init_recovery(struct i2c_adapter *adap)

{
	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
	char *err_str;

	if (!bri)
		return;

	if (!bri->recover_bus) {
		err_str = "no recover_bus() found";
		goto err;
	}

	/* bail out if either no gpio or no set/get callback. */
	if (!gpio_is_valid(bri->scl_gpio) && (!bri->set_scl || !bri-

Quoted 1 line

get_scl)) {

		if (!gpio_is_valid(bri->scl_gpio))
			err_str = "invalid SCL gpio";
		else
			err_str = "no {get|set}_scl() found";
		goto err;
	}

	if (gpio_is_valid(bri->sda_gpio))
		bri->get_sda = get_sda_gpio_value;

	if (gpio_is_valid(bri->scl_gpio)) {
		bri->get_scl = get_scl_gpio_value;

		bri->set_scl = set_scl_gpio_value;
	}

	return;
  err:
	dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
	adap->bus_recovery_info = NULL;
}

On Thu, 2017-05-11 at 09:24 +0800, Phil Reid wrote:

I have briefly looked at the current code.
So, my suggestion is to switch to gpio descriptors in current code and then rebase your stuff on top.

I wouldn't encourage people to continue using legacy GPIO API.

Quoted 11 lines

Quoted 11 lines

drivers/i2c/busses/i2c-designware-platdrv.c static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id) · line 235

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c +9 -0

+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
+                                    struct i2c_adapter *adap)
+{
+       struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+       dev->gpio_scl = devm_gpiod_get_optional(dev->dev,
+                                               "scl",
+                                               GPIOD_OUT_HIGH);
+       if (IS_ERR_OR_NULL(dev->gpio_scl))

This is wrong. You should not use this macro in most cases. And especially it breaks the logic behind _optional().

Quoted 2 lines

My logic here was that if the gpio is optional return null we return 0.

Why?!

_optional()*implies* that all rest calls will go fine and do nothing.

Quoted 5 lines

which is an okay status.
But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've never
quite wrapped my head around why that's the case.

Quoted 2 lines

But the probe function only bails out if this returns EPROBE_DEFER. Not sure that's the best approach

You need something like

desc = devm_gpiod_get_optional(...);
if (IS_ERR(desc))
return PTR_ERR(desc);

Quoted 32 lines · Andy Shevchenko

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

drivers/i2c/busses/i2c-designware-platdrv.c +5 -0

+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
+                                    struct i2c_adapter *adap)
+{
+       struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+

Quoted 8 lines

Quoted 5 lines

+       dev->gpio_scl = devm_gpiod_get_optional(dev->dev, +                                               "scl", +                                               GPIOD_OUT_HIGH );
+       if (IS_ERR_OR_NULL(dev->gpio_scl))

This is wrong. You should not use this macro in most cases. And especially it breaks the logic behind _optional().

Quoted 3 lines

My logic here was that if the gpio is optional return null we return
0.

Why?!

_optional()*implies*  that all rest calls will go fine and do nothing.

Quoted 8 lines

which is an okay status.
But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've never
quite wrapped my head around why that's the case.

But the probe function only bails out if this returns EPROBE_DEFER.
Not sure that's the best approach

Quoted 6 lines

You need something like

desc = devm_gpiod_get_optional(...);
if (IS_ERR(desc))
  return PTR_ERR(desc);

I found that continuing without the check on null results in a kernel panic for a dereferenced null pointer.
So something in the gpiolib doesn't treat a null desc as optional.

It was probably this code:
int desc_to_gpio(const struct gpio_desc *desc)

{
	return desc->gdev->base + (desc - &desc->gdev->descs[0]);
}

So perhaps this should return an invalid gpio number when desc == null

I don't know what the intents are, so don't know if its a "bug" or  by design.

Hi

I have tried to add a gpio recovery gpio controller to the designware i2c driver. The attempt is attached below. I have a Intel(Altera) Cyclone V SOC Platform attached to a buggy power supply which gives a lockup on the i2c controller as a external device gives to much noise on the signal and destroys a clock signal on its way to a i2c device.
I don't care to much about this buggy power supply but as the cable to one i2c-slave is rather long i fear that power surge conformance tests might give also some problems. So i would like to be safe than sorry and recover from this problem.

I have created two gpio ports in fpga and have routed the designware pins through the fpga. I can now read SDA input status and control SCL via these gpios. The recovery gets triggered and after that i get lots of:
i2c_designware ffc05000.i2c: controller timed out
so i guess that my i2c_dw_unprepare_recovery does not enought to get the controller back.

I have also noticed that there does not seem do be a reset controller in the standard configuration. so reset_control_(de)assert(i_dev->rst) seems to do nothing.

I have verified that the recovery of the bus works and if i do a warm reboot the i2c-bus is working again. Which it doesn't without recovery. So i am pretty sure that the recovery works as far as the i2c-slave is not pulling down SDA and that my gpio pins are in the correct state that they would not interfere with the i2c-operation of the controller.

Any ideas what i can do to get the controller back up running with some special treatment in i2c_dw_(un)prepare_recovery without having to resort to a warm reboot?

Best regards
Tim

---
  drivers/i2c/busses/i2c-designware-core.c    | 15 ++++++--
  drivers/i2c/busses/i2c-designware-core.h    |  1 +
  drivers/i2c/busses/i2c-designware-platdrv.c | 60 ++++++++++++++++++++++++++++-
  drivers/i2c/i2c-core.c                      | 10 ++++-
  4 files changed, 78 insertions(+), 8 deletions(-)

drivers/i2c/busses/i2c-designware-core.c +11 -4

index 7a3faa551cf8..b98fab40ce9a 100644
@@ -317,6 +317,7 @@ static void i2c_dw_release_lock(struct dw_i2c_dev *dev)
                 dev->release_lock(dev);
  }
 
+
  /**
   * i2c_dw_init() - initialize the designware i2c master hardware
   * @dev: device private data

@@ -463,7 +464,11 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
         while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
                 if (timeout <= 0) {
                         dev_warn(dev->dev, "timeout waiting for bus ready\n");
-                       return -ETIMEDOUT;
+                       i2c_recover_bus(&dev->adapter);
+
+                       if (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY)
+                               return -EIO;
+                       else return 0;
                 }
                 timeout--;
                 usleep_range(1000, 1100);

@@ -719,9 +724,10 @@ static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
         for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
                 dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
 
-       if (abort_source & DW_IC_TX_ARB_LOST)
+       if (abort_source & DW_IC_TX_ARB_LOST) {
+               i2c_recover_bus(&dev->adapter);
                 return -EAGAIN;
-       else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
+       } else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
                 return -EINVAL; /* wrong msgs[] data */
         else
                 return -EIO;

@@ -766,6 +772,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
         if (!wait_for_completion_timeout(&dev->cmd_complete, adap->timeout)) {
                 dev_err(dev->dev, "controller timed out\n");
                 /* i2c_dw_init implicitly disables the adapter */
+               //i2c_recover_bus(&dev->adapter);
                 i2c_dw_init(dev);
                 ret = -ETIMEDOUT;
                 goto done;

@@ -825,7 +832,7 @@ static const struct i2c_algorithm i2c_dw_algo = {
         .functionality  = i2c_dw_func,
  };
 
-static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
+u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
  {
         u32 stat;
 

drivers/i2c/busses/i2c-designware-core.h +1 -0

index d9aaf1790e0e..8bdf51e19f21 100644
@@ -126,6 +126,7 @@ struct dw_i2c_dev {
         int                     (*acquire_lock)(struct dw_i2c_dev *dev);
         void                    (*release_lock)(struct dw_i2c_dev *dev);
         bool                    pm_runtime_disabled;
+       struct                  i2c_bus_recovery_info rinfo;
  };
 
  #define ACCESS_SWAP            0x00000001

drivers/i2c/busses/i2c-designware-platdrv.c +58 -2

index 79c4b4ea0539..28ed9e886983 100644
@@ -41,6 +41,7 @@
  #include <linux/reset.h>
  #include <linux/slab.h>
  #include <linux/acpi.h>
+#include <linux/of_gpio.h>
  #include <linux/platform_data/i2c-designware.h>
  #include "i2c-designware-core.h"
 

@@ -174,6 +175,55 @@ static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
         }
  }
 
+/*
+ * This routine does i2c bus recovery by using i2c_generic_gpio_recovery
+ * which is provided by I2C Bus recovery infrastructure.
+ */
+static void i2c_dw_prepare_recovery(struct i2c_adapter *adap)
+{
+       struct platform_device *pdev = to_platform_device(&adap->dev);
+       struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+
+       i2c_dw_disable(i_dev);
+       reset_control_assert(i_dev->rst);
+       i2c_dw_plat_prepare_clk(i_dev, false);
+}
+
+void i2c_dw_unprepare_recovery(struct i2c_adapter *adap)
+{
+       struct platform_device *pdev = to_platform_device(&adap->dev);
+       struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+
+       i2c_dw_plat_prepare_clk(i_dev, true);
+       reset_control_deassert(i_dev->rst);
+       i2c_dw_init(i_dev);
+}
+
+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev, struct i2c_adapter *adap)
+{
+       struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+       rinfo->recover_bus = i2c_generic_gpio_recovery;
+       rinfo->prepare_recovery = i2c_dw_prepare_recovery;
+       rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
+
+       dev->rinfo.scl_gpio = of_get_named_gpio(dev->dev->of_node, "scl-gpios", 0);
+       if ( dev->rinfo.scl_gpio == -EPROBE_DEFER ) {
+               return -EPROBE_DEFER;
+       }
+       dev->rinfo.sda_gpio = of_get_named_gpio(dev->dev->of_node, "sda-gpios", 0);
+       if ( dev->rinfo.sda_gpio == -EPROBE_DEFER ) {
+               return -EPROBE_DEFER;
+       }
+       if ( !gpio_is_valid(dev->rinfo.scl_gpio) || !gpio_is_valid(dev->rinfo.sda_gpio) )
+               return -ENODEV;
+
+       dev_info(dev->dev,"adapter: %s running with gpio recovery mode! scl:%i sda:%i\n",adap->name,dev->rinfo.scl_gpio,dev->rinfo.sda_gpio);
+       adap->bus_recovery_info = &dev->rinfo;
+
+       return 0;
+};
+
  static int dw_i2c_plat_probe(struct platform_device *pdev)
  {
         struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);

@@ -285,6 +335,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
         adap->class = I2C_CLASS_DEPRECATED;
         ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
         adap->dev.of_node = pdev->dev.of_node;
+        snprintf(adap->name, sizeof(adap->name), "Designware i2c adapter");
 
         if (dev->pm_runtime_disabled) {
                 pm_runtime_forbid(&pdev->dev);

@@ -295,11 +346,16 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
                 pm_runtime_enable(&pdev->dev);
         }
 
+        r = i2c_dw_init_recovery_info(dev,adap);
+        if (r  == -EPROBE_DEFER)
+          goto exit_probe ;
+
+
         r = i2c_dw_probe(dev);
         if (r)
-               goto exit_probe;
+          goto exit_probe;
 
-       return r;
+       return 0 ;
 
  exit_probe:
         if (!dev->pm_runtime_disabled)

drivers/i2c/i2c-core.c +8 -2

index d2402bbf6729..e1fc1d2a9548 100644
@@ -739,6 +739,7 @@ static int get_scl_gpio_value(struct i2c_adapter *adap)
 
  static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
  {
+       dev_err(&adap->dev,"set scl:%i value: %i\n",adap->bus_recovery_info->scl_gpio, val);
         gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
  }
 

@@ -753,7 +754,7 @@ static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
         struct device *dev = &adap->dev;
         int ret = 0;
 
-       ret = gpio_request_one(bri->scl_gpio, GPIOF_OPEN_DRAIN |
+       ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN |
                         GPIOF_OUT_INIT_HIGH, "i2c-scl");
         if (ret) {
                 dev_warn(dev, "Can't get SCL gpio: %d\n", bri->scl_gpio);

@@ -807,8 +808,10 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
         while (i++ < RECOVERY_CLK_CNT * 2) {
                 if (val) {
                         /* Break if SDA is high */
-                       if (bri->get_sda && bri->get_sda(adap))
+                       if (bri->get_sda && bri->get_sda(adap)) {
+                                       dev_err(&adap->dev,"sda is high done\n");
                                         break;
+                       }
                         /* SCL shouldn't be low here */
                         if (!bri->get_scl(adap)) {
                                 dev_err(&adap->dev,

@@ -823,6 +826,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
                 ndelay(RECOVERY_NDELAY);
         }
 
+       dev_err(&adap->dev,"recovery cycle\n");
         if (bri->unprepare_recovery)
                 bri->unprepare_recovery(adap);
 

@@ -839,10 +843,12 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
  {
         int ret;
 
+
         ret = i2c_get_gpios_for_recovery(adap);
         if (ret)
                 return ret;
 
+       dev_err(&adap->dev,"i2c_generic_gpio_recovery have gpios\n");
         ret = i2c_generic_recovery(adap);
         i2c_put_gpios_for_recovery(adap);