RFC: i2c designware gpio recovery

RFC: i2c designware gpio recovery

[Tim Sander]

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

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 7a3faa551cf8..b98fab40ce9a 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -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;
 
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index d9aaf1790e0e..8bdf51e19f21 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -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
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 79c4b4ea0539..28ed9e886983 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -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)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d2402bbf6729..e1fc1d2a9548 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -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);
 
-- 
2.7.4

Re: RFC: i2c designware gpio recovery

[Tim Sander]

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

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 2c43c4d85dee..5f28632bc88c 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -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";
-- 
2.7.4

Re: RFC: i2c designware gpio recovery

[Phil Reid]

G'day Tim,


On 29/04/2017 00:14, Tim Sander wrote:
> 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.
There's a few thing like that where the bootloader has been expected to setup the resets etc.


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



> 
> Best regards
> Tim
> ---
>   arch/arm/boot/dts/socfpga.dtsi | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 2c43c4d85dee..5f28632bc88c 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -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";
> 


-- 
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au

Re: RFC: i2c designware gpio recovery

[Tim Sander]

Good Day Phil

Am Montag, 1. Mai 2017, 09:57:35 CEST schrieb Phil Reid:
> > 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.
Yes, but if the resets are not connected in the device tree, the linux drivers
are not going to use them?

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

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

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

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

Best regards
Tim

Re: RFC: i2c designware gpio recovery

[Phil Reid]

G'day Tim,

On 1/05/2017 21:31, Tim Sander wrote:
> Good Day Phil
> 
> Am Montag, 1. Mai 2017, 09:57:35 CEST schrieb Phil Reid:
>>> 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.
> 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.

> 
>>> 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?
> 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.
I think I understand. What soft core gpio controller are you using?

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

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

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.

-- 
Regards
Phil Reid

Re: RFC: i2c designware gpio recovery

[Tim Sander]

Good Day Phil

Am Mittwoch, 3. Mai 2017, 09:30:50 CEST schrieb Phil Reid:
> G'day Tim,
> 
> On 1/05/2017 21:31, Tim Sander wrote:
> > Good Day Phil
> > 
> > Am Montag, 1. Mai 2017, 09:57:35 CEST schrieb Phil Reid:
> >>> 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.
> > 
> > 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.

> >>> 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?
> > 
> > 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.
> 
> I think I understand. What soft core gpio controller are you using?
I am using the standard altera fpga gpios.

> >> 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.
> > 
> >> 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?
> 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
Mh, there is not much to see in Volume 3. Just one paragraph and then a 
very encouraging closing line:
"Do not modify the pin multiplexing selection registers after I/O configuration."

I find the following lines in my favorite bootloader a little more enlightening:
The following function:
https://git.pengutronix.de/cgit/barebox/tree/arch/arm/mach-socfpga/system-manager.c
get feed with data from e.g.:
https://git.pengutronix.de/cgit/barebox/tree/arch/arm/boards/terasic-de0-nano-soc/pinmux_config.c
which doesn't look like beeing very memory mapped?

> >> 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?
> 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.
Thanks, will take a look.

Best regards
Tim

Re: RFC: i2c designware gpio recovery

[Phil Reid]

G'day Tim,

Sorry for the delay in looking at this.
My device is currently running a 4.9 kernel and I had to backport the cahnges to the driver
to get things running with your patch.

In general the code works and the bus recovers now.
I've been using the i2c gpio bus driver because the dw wouldn't do recovery.
But this looks alot nicer.


On 4/05/2017 03:04, Tim Sander wrote:
>>>>> 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.
>>>
>>> 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.
> 
>>>>> 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?
>>>
>>> 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.
>>
>> I think I understand. What soft core gpio controller are you using?
> I am using the standard altera fpga gpios.
> 


I dug into things a little and found the following init function works without requiring modification to the core.
The GPIO config (open drain or not etc) can be put in the device tree.

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);
	return gpiod_get_value_cansleep(dev->gpio_scl);
}

static void i2c_dw_set_scl(struct i2c_adapter *adap, int val)
{
	struct platform_device *pdev = to_platform_device(&adap->dev);
	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
	gpiod_set_value_cansleep(dev->gpio_scl, val);
}

static int i2c_dw_get_sda(struct i2c_adapter *adap)
{
	struct platform_device *pdev = to_platform_device(&adap->dev);
	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
	return gpiod_get_value_cansleep(dev->gpio_sda);
}

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))
		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);
	rinfo->set_scl  = i2c_dw_set_scl;
	rinfo->get_scl  = i2c_dw_get_scl;
	rinfo->get_sda  = i2c_dw_get_sda;

	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,
		"adapter: %s running with gpio recovery mode! scl:%i sda:%i \n",
		adap->name, rinfo->scl_gpio, rinfo->sda_gpio);

	return 0;
};

A small modification to the i2c-core could be done in i2c_init_recovery to allow:
	rinfo->recover_bus == i2c_generic_scl_recovery
when scl_gpio is also set and fallback to using the core set / get scl / sda calls
Which would remove the need for the above i2c_dw_* functions.
I wouldn't think that would cause any problems.



-- 
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au

[PATCH] i2c-designware: add i2c gpio recovery option

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

Signed-off-by: Tim Sander <tim@krieglstein.org>
---
 drivers/i2c/busses/i2c-designware-core.c    | 14 ++++-
 drivers/i2c/busses/i2c-designware-core.h    |  4 ++
 drivers/i2c/busses/i2c-designware-platdrv.c | 90 ++++++++++++++++++++++++++++-
 3 files changed, 104 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 7a3faa551cf8..f955f29ff8e7 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -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,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);
@@ -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;
@@ -766,6 +773,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;
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index d9aaf1790e0e..cedc895a795d 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -23,6 +23,7 @@
  */
 
 #include <linux/i2c.h>
+#include <linux/gpio.h>
 
 #define DW_IC_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C |                    \
                                        I2C_FUNC_SMBUS_BYTE |           \
@@ -126,6 +127,9 @@ 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;
+       struct  gpio_desc       *gpio_sda;
+       struct  gpio_desc       *gpio_scl;
 };
 
 #define ACCESS_SWAP            0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 79c4b4ea0539..b2d5adc8df2b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -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,88 @@ 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_get_scl(struct i2c_adapter *adap)
+{
+       struct platform_device *pdev = to_platform_device(&adap->dev);
+       struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+
+       return gpiod_get_value_cansleep(dev->gpio_scl);
+}
+
+static void i2c_dw_set_scl(struct i2c_adapter *adap, int val)
+{
+       struct platform_device *pdev = to_platform_device(&adap->dev);
+       struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+
+       gpiod_set_value_cansleep(dev->gpio_scl, val);
+}
+
+static int i2c_dw_get_sda(struct i2c_adapter *adap)
+{
+       struct platform_device *pdev = to_platform_device(&adap->dev);
+       struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+
+       return gpiod_get_value_cansleep(dev->gpio_sda);
+}
+
+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))
+               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);
+       rinfo->set_scl  = i2c_dw_set_scl;
+       rinfo->get_scl  = i2c_dw_get_scl;
+       rinfo->get_sda  = i2c_dw_get_sda;
+
+       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,
+               "adapter: %s running with gpio recovery mode! scl:%i sda:%i \n",
+               adap->name, rinfo->scl_gpio, rinfo->sda_gpio);
+
+       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 +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");
 
        if (dev->pm_runtime_disabled) {
                pm_runtime_forbid(&pdev->dev);
@@ -295,11 +379,15 @@ 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;
 
-       return r;
+       return 0;
 
 exit_probe:
        if (!dev->pm_runtime_disabled)
-- 
2.7.4

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Andy Shevchenko]

On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote:
> 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.

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

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

Redundant.

> +                               return 0;
>                 }

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

>                 timeout--;
>                 usleep_range(1000, 1100);

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

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. 

>                 return -EIO;

> +#include <linux/gpio.h>

I think it should be

#include <linux/gpio/consumer.h>

> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -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. 

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

> +}
> +
> +

Remove extra line.

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

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

> +       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().

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

> +               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?!

> +};

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

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

Remove extra space.

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

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Phil Reid]

G'day Andy,

Thanks for the review.

On 10/05/2017 21:13, Andy Shevchenko wrote:
> On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote:
>> 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.
> 
>> @@ -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.
> 
>> @@ -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
> 
> Redundant.
> 
>> +                               return 0;
>>                  }
> 
> Actually I would rather refactor first above function:
> 1) to be do {} while();
> 2) to have invariant condition out of the loop.
> 
>>                  timeout--;
>>                  usleep_range(1000, 1100);
> 
>> @@ -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
> 
> 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.
> 
>>                  return -EIO;
> 
>> +#include <linux/gpio.h>
> 
> I think it should be
> 
> #include <linux/gpio/consumer.h>
> 
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -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.
> 
>> +void i2c_dw_unprepare_recovery(struct i2c_adapter *adap)
>> +{
> 
>> +}
>> +
>> +
> 
> Remove extra line.
> 
>> +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.
> 
>> +
>> +       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);
> 
>> +       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

> 
>> +               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.
> 
>> +               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->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;
}



> 
>> +};
> 
>> @@ -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.
> 
>>   
>> +       r = i2c_dw_init_recovery_info(dev, adap);
>> +       if (r  == -EPROBE_DEFER)
> 
> Remove extra space.
> 
>> +               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.
> 


-- 
Regards
Phil Reid

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Andy Shevchenko]

On Thu, 2017-05-11 at 09:24 +0800, Phil Reid wrote:
> G'day Andy,
> 
> Thanks for the review.

You're welcome, just don't forget to remove the parts that are out of
scope and/or you agree with.

> On 10/05/2017 21:13, Andy Shevchenko wrote:
> > On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote:

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

Why?!

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

> 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

You need something like

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

> > > +               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-
> >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;
> }


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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Andy Shevchenko]

On Thu, 2017-05-11 at 16:53 +0300, Andy Shevchenko wrote:
> 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.

For better transition it might be worth to create gpiod_ support in
parallel with existing and convert existing clients case-by-case if
needed, while discourage people to use gpio_ API.

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

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Phil Reid]

On 11/05/2017 21:53, Andy Shevchenko wrote:
>>>> +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.
> Why?!
> 
> _optional()*implies*  that all rest calls will go fine and do nothing.
> 
>> 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
> 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.

-- 
Regards
Phil Reid

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Andy Shevchenko]

On Fri, 2017-05-12 at 09:49 +0800, Phil Reid wrote:
> On 11/05/2017 21:53, Andy Shevchenko wrote:
> > > > > +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.
> > 
> > Why?!
> > 
> > _optional()*implies*  that all rest calls will go fine and do
> > nothing.
> > 
> > > 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
> > 
> > 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.

No, it doesn't seem like a bug to me. If you don't use legacy API it
would be fine.

Summarize this discussion, I would rather go this way:
1) introduce gpiod_ based API in I2C core for recovering;
2) rebase your patch on top of that change.

It would be beneficial in a long term for everyone.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: RFC: i2c designware gpio recovery

[Phil Reid]

On 28/04/2017 23:43, Tim Sander wrote:
> Hi
G'day Tim,

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.

> 
> 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(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 7a3faa551cf8..b98fab40ce9a 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -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;
>   
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index d9aaf1790e0e..8bdf51e19f21 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -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
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 79c4b4ea0539..28ed9e886983 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -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)
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d2402bbf6729..e1fc1d2a9548 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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);
>   
> 


-- 
Regards
Phil Reid