diff mbox

i2c: designware-platdrv: implement bus recovery

Message ID 1460642696-1858-1-git-send-email-jszhang@marvell.com
State Changes Requested
Headers show

Commit Message

Jisheng Zhang April 14, 2016, 2:04 p.m. UTC
Implement bus recovery methods for i2c designware so we can recover
from situations where SCL/SDA are stuck low.

The recovery method is similar as i2c-imx: "config the i2c pinctrl to
gpio mode by calling pinctrl sleep set function, and then use GPIO to
emulate the i2c protocol to send nine dummy clock to recover i2c
device. After recovery, set i2c pinctrl to default group setting.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
depends on runtime pm patches
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422202.html
 .../devicetree/bindings/i2c/i2c-designware.txt     | 12 ++++++
 drivers/i2c/busses/i2c-designware-core.c           |  6 ++-
 drivers/i2c/busses/i2c-designware-core.h           |  4 ++
 drivers/i2c/busses/i2c-designware-platdrv.c        | 50 ++++++++++++++++++++++
 4 files changed, 71 insertions(+), 1 deletion(-)

Comments

kernel test robot April 14, 2016, 2:29 p.m. UTC | #1
Hi Jisheng,

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.6-rc3 next-20160414]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Jisheng-Zhang/i2c-designware-platdrv-implement-bus-recovery/20160414-221615
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux i2c/for-next
config: xtensa-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All error/warnings (new ones prefixed by >>):

   drivers/i2c/busses/i2c-designware-platdrv.c: In function 'i2c_dw_plat_prepare_recovery':
>> drivers/i2c/busses/i2c-designware-platdrv.c:156:2: error: implicit declaration of function 'pinctrl_select_state' [-Werror=implicit-function-declaration]
     pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
     ^
   drivers/i2c/busses/i2c-designware-platdrv.c: In function 'i2c_dw_plat_init_recovery_info':
>> drivers/i2c/busses/i2c-designware-platdrv.c:171:2: error: implicit declaration of function 'pinctrl_lookup_state' [-Werror=implicit-function-declaration]
     dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
     ^
>> drivers/i2c/busses/i2c-designware-platdrv.c:172:4: error: 'PINCTRL_STATE_DEFAULT' undeclared (first use in this function)
       PINCTRL_STATE_DEFAULT);
       ^
   drivers/i2c/busses/i2c-designware-platdrv.c:172:4: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/i2c/busses/i2c-designware-platdrv.c:173:25: warning: assignment makes pointer from integer without a cast
     dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
                            ^
   drivers/i2c/busses/i2c-designware-platdrv.c: In function 'dw_i2c_plat_probe':
>> drivers/i2c/busses/i2c-designware-platdrv.c:212:2: error: implicit declaration of function 'devm_pinctrl_get' [-Werror=implicit-function-declaration]
     dev->pinctrl = devm_pinctrl_get(&pdev->dev);
     ^
   drivers/i2c/busses/i2c-designware-platdrv.c:212:15: warning: assignment makes pointer from integer without a cast
     dev->pinctrl = devm_pinctrl_get(&pdev->dev);
                  ^
   cc1: some warnings being treated as errors

vim +/pinctrl_select_state +156 drivers/i2c/busses/i2c-designware-platdrv.c

   150	}
   151	
   152	static void i2c_dw_plat_prepare_recovery(struct i2c_adapter *adap)
   153	{
   154		struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
   155	
 > 156		pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
   157	}
   158	
   159	static void i2c_dw_plat_unprepare_recovery(struct i2c_adapter *adap)
   160	{
   161		struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
   162	
   163		pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
   164	}
   165	
   166	static void i2c_dw_plat_init_recovery_info(struct dw_i2c_dev *dev,
   167						   struct platform_device *pdev)
   168	{
   169		struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
   170	
 > 171		dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
 > 172				PINCTRL_STATE_DEFAULT);
 > 173		dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
   174				"gpio");
   175		rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios", 0);
   176		rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios", 0);
   177	
   178		if (!gpio_is_valid(rinfo->sda_gpio) ||
   179		    !gpio_is_valid(rinfo->scl_gpio) ||
   180		    IS_ERR(dev->pinctrl_pins_default) ||
   181		    IS_ERR(dev->pinctrl_pins_gpio)) {
   182			dev_dbg(&pdev->dev, "recovery information incomplete\n");
   183			return;
   184		}
   185	
   186		dev_dbg(&pdev->dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
   187			rinfo->sda_gpio, rinfo->scl_gpio);
   188	
   189		rinfo->prepare_recovery = i2c_dw_plat_prepare_recovery;
   190		rinfo->unprepare_recovery = i2c_dw_plat_unprepare_recovery;
   191		rinfo->recover_bus = i2c_generic_gpio_recovery;
   192		dev->adapter.bus_recovery_info = rinfo;
   193	}
   194	
   195	static int dw_i2c_plat_probe(struct platform_device *pdev)
   196	{
   197		struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
   198		struct dw_i2c_dev *dev;
   199		struct i2c_adapter *adap;
   200		struct resource *mem;
   201		int irq, r;
   202		u32 clk_freq, ht = 0;
   203	
   204		irq = platform_get_irq(pdev, 0);
   205		if (irq < 0)
   206			return irq;
   207	
   208		dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
   209		if (!dev)
   210			return -ENOMEM;
   211	
 > 212		dev->pinctrl = devm_pinctrl_get(&pdev->dev);
   213		if (IS_ERR(dev->pinctrl))
   214			return PTR_ERR(dev->pinctrl);
   215	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Mika Westerberg April 14, 2016, 2:39 p.m. UTC | #2
On Thu, Apr 14, 2016 at 10:04:56PM +0800, Jisheng Zhang wrote:
> Implement bus recovery methods for i2c designware so we can recover
> from situations where SCL/SDA are stuck low.
> 
> The recovery method is similar as i2c-imx: "config the i2c pinctrl to
> gpio mode by calling pinctrl sleep set function, and then use GPIO to
> emulate the i2c protocol to send nine dummy clock to recover i2c
> device. After recovery, set i2c pinctrl to default group setting.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> depends on runtime pm patches
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422202.html
>  .../devicetree/bindings/i2c/i2c-designware.txt     | 12 ++++++
>  drivers/i2c/busses/i2c-designware-core.c           |  6 ++-
>  drivers/i2c/busses/i2c-designware-core.h           |  4 ++
>  drivers/i2c/busses/i2c-designware-platdrv.c        | 50 ++++++++++++++++++++++
>  4 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index fee26dc..51a55c6 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -20,6 +20,13 @@ Optional properties :
>   - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
>     This value which is by default 300ns is used to compute the tHIGH period.
>  
> + - scl-gpios: specify the gpio related to SCL pin
> +
> + - sda-gpios: specify the gpio related to SDA pin
> +
> + - pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
> +   bus recovery, call it "gpio" state
> +
>  Example :
>  
>  	i2c@f0000 {
> @@ -42,4 +49,9 @@ Example :
>  		i2c-sda-hold-time-ns = <300>;
>  		i2c-sda-falling-time-ns = <300>;
>  		i2c-scl-falling-time-ns = <300>;
> +		pinctrl-names = "default", "gpio";
> +		pinctrl-0 = <&pinctrl_i2c1_default>;
> +		pinctrl-1 = <&pinctrl_i2c1_gpio>;
> +		scl-gpios = <&porta 26 GPIO_ACTIVE_HIGH>;
> +		sda-gpios = <&porta 27 GPIO_ACTIVE_HIGH>;
>  	};
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 4255eaa..be40de1 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -399,7 +399,10 @@ 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;
>  		}
>  		timeout--;
>  		usleep_range(1000, 1100);
> @@ -665,6 +668,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	/* wait for tx to complete */
>  	if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) {
>  		dev_err(dev->dev, "controller timed out\n");
> +		i2c_recover_bus(&dev->adapter);
>  		/* i2c_dw_init implicitly disables the adapter */
>  		i2c_dw_init(dev);
>  		ret = -ETIMEDOUT;
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index cd409e7..26c84ee 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -104,6 +104,10 @@ struct dw_i2c_dev {
>  	u16			fs_lcnt;
>  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
>  	void			(*release_lock)(struct dw_i2c_dev *dev);
> +	struct i2c_bus_recovery_info rinfo;
> +	struct pinctrl		*pinctrl;
> +	struct pinctrl_state	*pinctrl_pins_default;
> +	struct pinctrl_state	*pinctrl_pins_gpio;
>  	bool			pm_runtime_disabled;
>  };
>  
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 1488cea..62bded9 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -33,6 +33,7 @@
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
>  #include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
> @@ -148,6 +149,49 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
>  	return 0;
>  }
>  
> +static void i2c_dw_plat_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> +}
> +
> +static void i2c_dw_plat_unprepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> +}
> +
> +static void i2c_dw_plat_init_recovery_info(struct dw_i2c_dev *dev,
> +					   struct platform_device *pdev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> +			PINCTRL_STATE_DEFAULT);
> +	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> +			"gpio");
> +	rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios", 0);
> +	rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios", 0);
> +
> +	if (!gpio_is_valid(rinfo->sda_gpio) ||
> +	    !gpio_is_valid(rinfo->scl_gpio) ||
> +	    IS_ERR(dev->pinctrl_pins_default) ||
> +	    IS_ERR(dev->pinctrl_pins_gpio)) {
> +		dev_dbg(&pdev->dev, "recovery information incomplete\n");
> +		return;
> +	}
> +
> +	dev_dbg(&pdev->dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
> +		rinfo->sda_gpio, rinfo->scl_gpio);
> +
> +	rinfo->prepare_recovery = i2c_dw_plat_prepare_recovery;
> +	rinfo->unprepare_recovery = i2c_dw_plat_unprepare_recovery;
> +	rinfo->recover_bus = i2c_generic_gpio_recovery;

This whole thing is DT specific and I don't see how we can do the same
for ACPI. Should it be enabled only when the controller was enumerated
from DT?

> +	dev->adapter.bus_recovery_info = rinfo;
> +}
> +
>  static int dw_i2c_plat_probe(struct platform_device *pdev)
>  {
>  	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -165,6 +209,12 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  	if (!dev)
>  		return -ENOMEM;
>  
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(dev->pinctrl))
> +		return PTR_ERR(dev->pinctrl);
> +
> +	i2c_dw_plat_init_recovery_info(dev, pdev);
> +
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	dev->base = devm_ioremap_resource(&pdev->dev, mem);
>  	if (IS_ERR(dev->base))
> -- 
> 2.8.0.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martinez Kristofer April 14, 2016, 2:58 p.m. UTC | #3
On Thu, Apr 14, 2016 at 10:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Apr 14, 2016 at 10:04:56PM +0800, Jisheng Zhang wrote:
>> Implement bus recovery methods for i2c designware so we can recover
>> from situations where SCL/SDA are stuck low.
>>
>> The recovery method is similar as i2c-imx: "config the i2c pinctrl to
>> gpio mode by calling pinctrl sleep set function, and then use GPIO to
>> emulate the i2c protocol to send nine dummy clock to recover i2c
>> device. After recovery, set i2c pinctrl to default group setting.
>>
>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>> ---
>> depends on runtime pm patches
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422202.html
>>  .../devicetree/bindings/i2c/i2c-designware.txt     | 12 ++++++
>>  drivers/i2c/busses/i2c-designware-core.c           |  6 ++-
>>  drivers/i2c/busses/i2c-designware-core.h           |  4 ++
>>  drivers/i2c/busses/i2c-designware-platdrv.c        | 50 ++++++++++++++++++++++
>>  4 files changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
>> index fee26dc..51a55c6 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
>> @@ -20,6 +20,13 @@ Optional properties :
>>   - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
>>     This value which is by default 300ns is used to compute the tHIGH period.
>>
>> + - scl-gpios: specify the gpio related to SCL pin
>> +
>> + - sda-gpios: specify the gpio related to SDA pin
>> +
>> + - pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
>> +   bus recovery, call it "gpio" state
>> +
>>  Example :
>>
>>       i2c@f0000 {
>> @@ -42,4 +49,9 @@ Example :
>>               i2c-sda-hold-time-ns = <300>;
>>               i2c-sda-falling-time-ns = <300>;
>>               i2c-scl-falling-time-ns = <300>;
>> +             pinctrl-names = "default", "gpio";
>> +             pinctrl-0 = <&pinctrl_i2c1_default>;
>> +             pinctrl-1 = <&pinctrl_i2c1_gpio>;
>> +             scl-gpios = <&porta 26 GPIO_ACTIVE_HIGH>;
>> +             sda-gpios = <&porta 27 GPIO_ACTIVE_HIGH>;
>>       };
>> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
>> index 4255eaa..be40de1 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.c
>> +++ b/drivers/i2c/busses/i2c-designware-core.c
>> @@ -399,7 +399,10 @@ 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;
>>               }
>>               timeout--;
>>               usleep_range(1000, 1100);
>> @@ -665,6 +668,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>       /* wait for tx to complete */
>>       if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) {
>>               dev_err(dev->dev, "controller timed out\n");
>> +             i2c_recover_bus(&dev->adapter);
>>               /* i2c_dw_init implicitly disables the adapter */
>>               i2c_dw_init(dev);
>>               ret = -ETIMEDOUT;
>> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
>> index cd409e7..26c84ee 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -104,6 +104,10 @@ struct dw_i2c_dev {
>>       u16                     fs_lcnt;
>>       int                     (*acquire_lock)(struct dw_i2c_dev *dev);
>>       void                    (*release_lock)(struct dw_i2c_dev *dev);
>> +     struct i2c_bus_recovery_info rinfo;
>> +     struct pinctrl          *pinctrl;
>> +     struct pinctrl_state    *pinctrl_pins_default;
>> +     struct pinctrl_state    *pinctrl_pins_gpio;
>>       bool                    pm_runtime_disabled;
>>  };
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 1488cea..62bded9 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -33,6 +33,7 @@
>>  #include <linux/err.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/of.h>
>> +#include <linux/of_gpio.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pm.h>
>>  #include <linux/pm_runtime.h>
>> @@ -148,6 +149,49 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
>>       return 0;
>>  }
>>
>> +static void i2c_dw_plat_prepare_recovery(struct i2c_adapter *adap)
>> +{
>> +     struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
>> +
>> +     pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
>> +}
>> +
>> +static void i2c_dw_plat_unprepare_recovery(struct i2c_adapter *adap)
>> +{
>> +     struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
>> +
>> +     pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
>> +}
>> +
>> +static void i2c_dw_plat_init_recovery_info(struct dw_i2c_dev *dev,
>> +                                        struct platform_device *pdev)
>> +{
>> +     struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>> +
>> +     dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
>> +                     PINCTRL_STATE_DEFAULT);
>> +     dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
>> +                     "gpio");
>> +     rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios", 0);
>> +     rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios", 0);
>> +
>> +     if (!gpio_is_valid(rinfo->sda_gpio) ||
>> +         !gpio_is_valid(rinfo->scl_gpio) ||
>> +         IS_ERR(dev->pinctrl_pins_default) ||
>> +         IS_ERR(dev->pinctrl_pins_gpio)) {
>> +             dev_dbg(&pdev->dev, "recovery information incomplete\n");
>> +             return;
>> +     }
>> +
>> +     dev_dbg(&pdev->dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
>> +             rinfo->sda_gpio, rinfo->scl_gpio);
>> +
>> +     rinfo->prepare_recovery = i2c_dw_plat_prepare_recovery;
>> +     rinfo->unprepare_recovery = i2c_dw_plat_unprepare_recovery;
>> +     rinfo->recover_bus = i2c_generic_gpio_recovery;
>
> This whole thing is DT specific and I don't see how we can do the same
> for ACPI. Should it be enabled only when the controller was enumerated
> from DT?
>
For ACPI, probably result in the panic-alike function calling chain at
i2c_dw_init --> dev->get_clk_rate_khz(dev).
Mika, I think you need to take ACPI into account while not focus on DT only

KM

>> +     dev->adapter.bus_recovery_info = rinfo;
>> +}
>> +
>>  static int dw_i2c_plat_probe(struct platform_device *pdev)
>>  {
>>       struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> @@ -165,6 +209,12 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>>       if (!dev)
>>               return -ENOMEM;
>>
>> +     dev->pinctrl = devm_pinctrl_get(&pdev->dev);
>> +     if (IS_ERR(dev->pinctrl))
>> +             return PTR_ERR(dev->pinctrl);
>> +
>> +     i2c_dw_plat_init_recovery_info(dev, pdev);
>> +
>>       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       dev->base = devm_ioremap_resource(&pdev->dev, mem);
>>       if (IS_ERR(dev->base))
>> --
>> 2.8.0.rc3
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) April 14, 2016, 5:34 p.m. UTC | #4
On Thu, Apr 14, 2016 at 10:04:56PM +0800, Jisheng Zhang wrote:
> Implement bus recovery methods for i2c designware so we can recover
> from situations where SCL/SDA are stuck low.
> 
> The recovery method is similar as i2c-imx: "config the i2c pinctrl to
> gpio mode by calling pinctrl sleep set function, and then use GPIO to
> emulate the i2c protocol to send nine dummy clock to recover i2c
> device. After recovery, set i2c pinctrl to default group setting.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> depends on runtime pm patches
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422202.html
>  .../devicetree/bindings/i2c/i2c-designware.txt     | 12 ++++++
>  drivers/i2c/busses/i2c-designware-core.c           |  6 ++-
>  drivers/i2c/busses/i2c-designware-core.h           |  4 ++
>  drivers/i2c/busses/i2c-designware-platdrv.c        | 50 ++++++++++++++++++++++
>  4 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index fee26dc..51a55c6 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -20,6 +20,13 @@ Optional properties :
>   - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
>     This value which is by default 300ns is used to compute the tHIGH period.
>  
> + - scl-gpios: specify the gpio related to SCL pin
> +
> + - sda-gpios: specify the gpio related to SDA pin
> +
> + - pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
> +   bus recovery, call it "gpio" state
> +

Make these common properties in i2c.txt (and a separate patch).

>  Example :
>  
>  	i2c@f0000 {
> @@ -42,4 +49,9 @@ Example :
>  		i2c-sda-hold-time-ns = <300>;
>  		i2c-sda-falling-time-ns = <300>;
>  		i2c-scl-falling-time-ns = <300>;
> +		pinctrl-names = "default", "gpio";
> +		pinctrl-0 = <&pinctrl_i2c1_default>;
> +		pinctrl-1 = <&pinctrl_i2c1_gpio>;
> +		scl-gpios = <&porta 26 GPIO_ACTIVE_HIGH>;
> +		sda-gpios = <&porta 27 GPIO_ACTIVE_HIGH>;
>  	};
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jisheng Zhang April 15, 2016, 6:08 a.m. UTC | #5
Dear Mika,

On Thu, 14 Apr 2016 17:39:16 +0300 Mika Westerberg wrote:

> On Thu, Apr 14, 2016 at 10:04:56PM +0800, Jisheng Zhang wrote:
> > Implement bus recovery methods for i2c designware so we can recover
> > from situations where SCL/SDA are stuck low.
> > 
> > The recovery method is similar as i2c-imx: "config the i2c pinctrl to
> > gpio mode by calling pinctrl sleep set function, and then use GPIO to
> > emulate the i2c protocol to send nine dummy clock to recover i2c
> > device. After recovery, set i2c pinctrl to default group setting.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> > depends on runtime pm patches
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422202.html
> >  .../devicetree/bindings/i2c/i2c-designware.txt     | 12 ++++++
> >  drivers/i2c/busses/i2c-designware-core.c           |  6 ++-
> >  drivers/i2c/busses/i2c-designware-core.h           |  4 ++
> >  drivers/i2c/busses/i2c-designware-platdrv.c        | 50 ++++++++++++++++++++++
> >  4 files changed, 71 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > index fee26dc..51a55c6 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > @@ -20,6 +20,13 @@ Optional properties :
> >   - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
> >     This value which is by default 300ns is used to compute the tHIGH period.
> >  
> > + - scl-gpios: specify the gpio related to SCL pin
> > +
> > + - sda-gpios: specify the gpio related to SDA pin
> > +
> > + - pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
> > +   bus recovery, call it "gpio" state
> > +
> >  Example :
> >  
> >  	i2c@f0000 {
> > @@ -42,4 +49,9 @@ Example :
> >  		i2c-sda-hold-time-ns = <300>;
> >  		i2c-sda-falling-time-ns = <300>;
> >  		i2c-scl-falling-time-ns = <300>;
> > +		pinctrl-names = "default", "gpio";
> > +		pinctrl-0 = <&pinctrl_i2c1_default>;
> > +		pinctrl-1 = <&pinctrl_i2c1_gpio>;
> > +		scl-gpios = <&porta 26 GPIO_ACTIVE_HIGH>;
> > +		sda-gpios = <&porta 27 GPIO_ACTIVE_HIGH>;
> >  	};
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> > index 4255eaa..be40de1 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -399,7 +399,10 @@ 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;
> >  		}
> >  		timeout--;
> >  		usleep_range(1000, 1100);
> > @@ -665,6 +668,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  	/* wait for tx to complete */
> >  	if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) {
> >  		dev_err(dev->dev, "controller timed out\n");
> > +		i2c_recover_bus(&dev->adapter);
> >  		/* i2c_dw_init implicitly disables the adapter */
> >  		i2c_dw_init(dev);
> >  		ret = -ETIMEDOUT;
> > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> > index cd409e7..26c84ee 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.h
> > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > @@ -104,6 +104,10 @@ struct dw_i2c_dev {
> >  	u16			fs_lcnt;
> >  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
> >  	void			(*release_lock)(struct dw_i2c_dev *dev);
> > +	struct i2c_bus_recovery_info rinfo;
> > +	struct pinctrl		*pinctrl;
> > +	struct pinctrl_state	*pinctrl_pins_default;
> > +	struct pinctrl_state	*pinctrl_pins_gpio;
> >  	bool			pm_runtime_disabled;
> >  };
> >  
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 1488cea..62bded9 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/err.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/of.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm.h>
> >  #include <linux/pm_runtime.h>
> > @@ -148,6 +149,49 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
> >  	return 0;
> >  }
> >  
> > +static void i2c_dw_plat_prepare_recovery(struct i2c_adapter *adap)
> > +{
> > +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > +
> > +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> > +}
> > +
> > +static void i2c_dw_plat_unprepare_recovery(struct i2c_adapter *adap)
> > +{
> > +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > +
> > +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> > +}
> > +
> > +static void i2c_dw_plat_init_recovery_info(struct dw_i2c_dev *dev,
> > +					   struct platform_device *pdev)
> > +{
> > +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> > +
> > +	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> > +			PINCTRL_STATE_DEFAULT);
> > +	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> > +			"gpio");
> > +	rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios", 0);
> > +	rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios", 0);
> > +
> > +	if (!gpio_is_valid(rinfo->sda_gpio) ||
> > +	    !gpio_is_valid(rinfo->scl_gpio) ||
> > +	    IS_ERR(dev->pinctrl_pins_default) ||
> > +	    IS_ERR(dev->pinctrl_pins_gpio)) {
> > +		dev_dbg(&pdev->dev, "recovery information incomplete\n");
> > +		return;

For ACPI, we should return here, right?

> > +	}
> > +
> > +	dev_dbg(&pdev->dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
> > +		rinfo->sda_gpio, rinfo->scl_gpio);
> > +
> > +	rinfo->prepare_recovery = i2c_dw_plat_prepare_recovery;
> > +	rinfo->unprepare_recovery = i2c_dw_plat_unprepare_recovery;
> > +	rinfo->recover_bus = i2c_generic_gpio_recovery;  
> 
> This whole thing is DT specific and I don't see how we can do the same
> for ACPI. Should it be enabled only when the controller was enumerated
> from DT?

Per my understanding, for ACPI, the pinctrl_pins_default, pinctrl_pins_gpio,
sda_gpio and scl_gpio will be invalid or err, so we won't setup rinfo
at all. However, I have no acpi platform which have i2c designware to double
confirm.

> 
> > +	dev->adapter.bus_recovery_info = rinfo;
> > +}
> > +
> >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> >  {
> >  	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > @@ -165,6 +209,12 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >  	if (!dev)
> >  		return -ENOMEM;
> >  
> > +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +	if (IS_ERR(dev->pinctrl))
> > +		return PTR_ERR(dev->pinctrl);
> > +
> > +	i2c_dw_plat_init_recovery_info(dev, pdev);
> > +
> >  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	dev->base = devm_ioremap_resource(&pdev->dev, mem);
> >  	if (IS_ERR(dev->base))
> > -- 
> > 2.8.0.rc3  

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg April 15, 2016, 7:07 a.m. UTC | #6
On Fri, Apr 15, 2016 at 02:08:43PM +0800, Jisheng Zhang wrote:
> Dear Mika,
> 
> On Thu, 14 Apr 2016 17:39:16 +0300 Mika Westerberg wrote:
> 
> > On Thu, Apr 14, 2016 at 10:04:56PM +0800, Jisheng Zhang wrote:
> > > Implement bus recovery methods for i2c designware so we can recover
> > > from situations where SCL/SDA are stuck low.
> > > 
> > > The recovery method is similar as i2c-imx: "config the i2c pinctrl to
> > > gpio mode by calling pinctrl sleep set function, and then use GPIO to
> > > emulate the i2c protocol to send nine dummy clock to recover i2c
> > > device. After recovery, set i2c pinctrl to default group setting.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > > depends on runtime pm patches
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422202.html
> > >  .../devicetree/bindings/i2c/i2c-designware.txt     | 12 ++++++
> > >  drivers/i2c/busses/i2c-designware-core.c           |  6 ++-
> > >  drivers/i2c/busses/i2c-designware-core.h           |  4 ++
> > >  drivers/i2c/busses/i2c-designware-platdrv.c        | 50 ++++++++++++++++++++++
> > >  4 files changed, 71 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > index fee26dc..51a55c6 100644
> > > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > @@ -20,6 +20,13 @@ Optional properties :
> > >   - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
> > >     This value which is by default 300ns is used to compute the tHIGH period.
> > >  
> > > + - scl-gpios: specify the gpio related to SCL pin
> > > +
> > > + - sda-gpios: specify the gpio related to SDA pin
> > > +
> > > + - pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
> > > +   bus recovery, call it "gpio" state
> > > +
> > >  Example :
> > >  
> > >  	i2c@f0000 {
> > > @@ -42,4 +49,9 @@ Example :
> > >  		i2c-sda-hold-time-ns = <300>;
> > >  		i2c-sda-falling-time-ns = <300>;
> > >  		i2c-scl-falling-time-ns = <300>;
> > > +		pinctrl-names = "default", "gpio";
> > > +		pinctrl-0 = <&pinctrl_i2c1_default>;
> > > +		pinctrl-1 = <&pinctrl_i2c1_gpio>;
> > > +		scl-gpios = <&porta 26 GPIO_ACTIVE_HIGH>;
> > > +		sda-gpios = <&porta 27 GPIO_ACTIVE_HIGH>;
> > >  	};
> > > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> > > index 4255eaa..be40de1 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.c
> > > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > > @@ -399,7 +399,10 @@ 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;
> > >  		}
> > >  		timeout--;
> > >  		usleep_range(1000, 1100);
> > > @@ -665,6 +668,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > >  	/* wait for tx to complete */
> > >  	if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) {
> > >  		dev_err(dev->dev, "controller timed out\n");
> > > +		i2c_recover_bus(&dev->adapter);
> > >  		/* i2c_dw_init implicitly disables the adapter */
> > >  		i2c_dw_init(dev);
> > >  		ret = -ETIMEDOUT;
> > > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> > > index cd409e7..26c84ee 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > @@ -104,6 +104,10 @@ struct dw_i2c_dev {
> > >  	u16			fs_lcnt;
> > >  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
> > >  	void			(*release_lock)(struct dw_i2c_dev *dev);
> > > +	struct i2c_bus_recovery_info rinfo;
> > > +	struct pinctrl		*pinctrl;
> > > +	struct pinctrl_state	*pinctrl_pins_default;
> > > +	struct pinctrl_state	*pinctrl_pins_gpio;
> > >  	bool			pm_runtime_disabled;
> > >  };
> > >  
> > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 1488cea..62bded9 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -33,6 +33,7 @@
> > >  #include <linux/err.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/of.h>
> > > +#include <linux/of_gpio.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm.h>
> > >  #include <linux/pm_runtime.h>
> > > @@ -148,6 +149,49 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
> > >  	return 0;
> > >  }
> > >  
> > > +static void i2c_dw_plat_prepare_recovery(struct i2c_adapter *adap)
> > > +{
> > > +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > > +
> > > +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> > > +}
> > > +
> > > +static void i2c_dw_plat_unprepare_recovery(struct i2c_adapter *adap)
> > > +{
> > > +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > > +
> > > +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> > > +}
> > > +
> > > +static void i2c_dw_plat_init_recovery_info(struct dw_i2c_dev *dev,
> > > +					   struct platform_device *pdev)
> > > +{
> > > +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> > > +
> > > +	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> > > +			PINCTRL_STATE_DEFAULT);
> > > +	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> > > +			"gpio");
> > > +	rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios", 0);
> > > +	rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios", 0);
> > > +
> > > +	if (!gpio_is_valid(rinfo->sda_gpio) ||
> > > +	    !gpio_is_valid(rinfo->scl_gpio) ||
> > > +	    IS_ERR(dev->pinctrl_pins_default) ||
> > > +	    IS_ERR(dev->pinctrl_pins_gpio)) {
> > > +		dev_dbg(&pdev->dev, "recovery information incomplete\n");
> > > +		return;
> 
> For ACPI, we should return here, right?

I hope so. And I do not want to see any useless messages about
incomplete recovery information on my PC.

> > > +	}
> > > +
> > > +	dev_dbg(&pdev->dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
> > > +		rinfo->sda_gpio, rinfo->scl_gpio);
> > > +
> > > +	rinfo->prepare_recovery = i2c_dw_plat_prepare_recovery;
> > > +	rinfo->unprepare_recovery = i2c_dw_plat_unprepare_recovery;
> > > +	rinfo->recover_bus = i2c_generic_gpio_recovery;  
> > 
> > This whole thing is DT specific and I don't see how we can do the same
> > for ACPI. Should it be enabled only when the controller was enumerated
> > from DT?
> 
> Per my understanding, for ACPI, the pinctrl_pins_default, pinctrl_pins_gpio,
> sda_gpio and scl_gpio will be invalid or err, so we won't setup rinfo
> at all. However, I have no acpi platform which have i2c designware to double
> confirm.

What happens to those of_get_ functions when you pass NULL of_node? If
they do not crash then I suppose it is fine.

Anyhow it might be good idea to do something like

	if (!pdev->dev.of_node)
		return;

in the beginning of that function.

> > 
> > > +	dev->adapter.bus_recovery_info = rinfo;
> > > +}
> > > +
> > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > >  {
> > >  	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > > @@ -165,6 +209,12 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> > >  	if (!dev)
> > >  		return -ENOMEM;
> > >  
> > > +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > +	if (IS_ERR(dev->pinctrl))
> > > +		return PTR_ERR(dev->pinctrl);
> > > +
> > > +	i2c_dw_plat_init_recovery_info(dev, pdev);
> > > +
> > >  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >  	dev->base = devm_ioremap_resource(&pdev->dev, mem);
> > >  	if (IS_ERR(dev->base))
> > > -- 
> > > 2.8.0.rc3  
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jisheng Zhang April 15, 2016, 7:30 a.m. UTC | #7
On Fri, 15 Apr 2016 10:07:46 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Fri, Apr 15, 2016 at 02:08:43PM +0800, Jisheng Zhang wrote:
> > Dear Mika,
> > 
> > On Thu, 14 Apr 2016 17:39:16 +0300 Mika Westerberg wrote:
> >   
> > > On Thu, Apr 14, 2016 at 10:04:56PM +0800, Jisheng Zhang wrote:  
> > > > Implement bus recovery methods for i2c designware so we can recover
> > > > from situations where SCL/SDA are stuck low.
> > > > 
> > > > The recovery method is similar as i2c-imx: "config the i2c pinctrl to
> > > > gpio mode by calling pinctrl sleep set function, and then use GPIO to
> > > > emulate the i2c protocol to send nine dummy clock to recover i2c
> > > > device. After recovery, set i2c pinctrl to default group setting.
> > > > 
> > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > > ---
> > > > depends on runtime pm patches
> > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422202.html
> > > >  .../devicetree/bindings/i2c/i2c-designware.txt     | 12 ++++++
> > > >  drivers/i2c/busses/i2c-designware-core.c           |  6 ++-
> > > >  drivers/i2c/busses/i2c-designware-core.h           |  4 ++
> > > >  drivers/i2c/busses/i2c-designware-platdrv.c        | 50 ++++++++++++++++++++++
> > > >  4 files changed, 71 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > > index fee26dc..51a55c6 100644
> > > > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > > @@ -20,6 +20,13 @@ Optional properties :
> > > >   - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
> > > >     This value which is by default 300ns is used to compute the tHIGH period.
> > > >  
> > > > + - scl-gpios: specify the gpio related to SCL pin
> > > > +
> > > > + - sda-gpios: specify the gpio related to SDA pin
> > > > +
> > > > + - pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
> > > > +   bus recovery, call it "gpio" state
> > > > +
> > > >  Example :
> > > >  
> > > >  	i2c@f0000 {
> > > > @@ -42,4 +49,9 @@ Example :
> > > >  		i2c-sda-hold-time-ns = <300>;
> > > >  		i2c-sda-falling-time-ns = <300>;
> > > >  		i2c-scl-falling-time-ns = <300>;
> > > > +		pinctrl-names = "default", "gpio";
> > > > +		pinctrl-0 = <&pinctrl_i2c1_default>;
> > > > +		pinctrl-1 = <&pinctrl_i2c1_gpio>;
> > > > +		scl-gpios = <&porta 26 GPIO_ACTIVE_HIGH>;
> > > > +		sda-gpios = <&porta 27 GPIO_ACTIVE_HIGH>;
> > > >  	};
> > > > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> > > > index 4255eaa..be40de1 100644
> > > > --- a/drivers/i2c/busses/i2c-designware-core.c
> > > > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > > > @@ -399,7 +399,10 @@ 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;
> > > >  		}
> > > >  		timeout--;
> > > >  		usleep_range(1000, 1100);
> > > > @@ -665,6 +668,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > > >  	/* wait for tx to complete */
> > > >  	if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) {
> > > >  		dev_err(dev->dev, "controller timed out\n");
> > > > +		i2c_recover_bus(&dev->adapter);
> > > >  		/* i2c_dw_init implicitly disables the adapter */
> > > >  		i2c_dw_init(dev);
> > > >  		ret = -ETIMEDOUT;
> > > > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> > > > index cd409e7..26c84ee 100644
> > > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > > @@ -104,6 +104,10 @@ struct dw_i2c_dev {
> > > >  	u16			fs_lcnt;
> > > >  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
> > > >  	void			(*release_lock)(struct dw_i2c_dev *dev);
> > > > +	struct i2c_bus_recovery_info rinfo;
> > > > +	struct pinctrl		*pinctrl;
> > > > +	struct pinctrl_state	*pinctrl_pins_default;
> > > > +	struct pinctrl_state	*pinctrl_pins_gpio;
> > > >  	bool			pm_runtime_disabled;
> > > >  };
> > > >  
> > > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > > index 1488cea..62bded9 100644
> > > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > > @@ -33,6 +33,7 @@
> > > >  #include <linux/err.h>
> > > >  #include <linux/interrupt.h>
> > > >  #include <linux/of.h>
> > > > +#include <linux/of_gpio.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/pm.h>
> > > >  #include <linux/pm_runtime.h>
> > > > @@ -148,6 +149,49 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static void i2c_dw_plat_prepare_recovery(struct i2c_adapter *adap)
> > > > +{
> > > > +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > > > +
> > > > +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> > > > +}
> > > > +
> > > > +static void i2c_dw_plat_unprepare_recovery(struct i2c_adapter *adap)
> > > > +{
> > > > +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > > > +
> > > > +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> > > > +}
> > > > +
> > > > +static void i2c_dw_plat_init_recovery_info(struct dw_i2c_dev *dev,
> > > > +					   struct platform_device *pdev)
> > > > +{
> > > > +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> > > > +
> > > > +	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> > > > +			PINCTRL_STATE_DEFAULT);
> > > > +	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> > > > +			"gpio");
> > > > +	rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios", 0);
> > > > +	rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios", 0);
> > > > +
> > > > +	if (!gpio_is_valid(rinfo->sda_gpio) ||
> > > > +	    !gpio_is_valid(rinfo->scl_gpio) ||
> > > > +	    IS_ERR(dev->pinctrl_pins_default) ||
> > > > +	    IS_ERR(dev->pinctrl_pins_gpio)) {
> > > > +		dev_dbg(&pdev->dev, "recovery information incomplete\n");
> > > > +		return;  
> > 
> > For ACPI, we should return here, right?  
> 
> I hope so. And I do not want to see any useless messages about
> incomplete recovery information on my PC.

Got your points.

> 
> > > > +	}
> > > > +
> > > > +	dev_dbg(&pdev->dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
> > > > +		rinfo->sda_gpio, rinfo->scl_gpio);
> > > > +
> > > > +	rinfo->prepare_recovery = i2c_dw_plat_prepare_recovery;
> > > > +	rinfo->unprepare_recovery = i2c_dw_plat_unprepare_recovery;
> > > > +	rinfo->recover_bus = i2c_generic_gpio_recovery;    
> > > 
> > > This whole thing is DT specific and I don't see how we can do the same
> > > for ACPI. Should it be enabled only when the controller was enumerated
> > > from DT?  
> > 
> > Per my understanding, for ACPI, the pinctrl_pins_default, pinctrl_pins_gpio,
> > sda_gpio and scl_gpio will be invalid or err, so we won't setup rinfo
> > at all. However, I have no acpi platform which have i2c designware to double
> > confirm.  
> 
> What happens to those of_get_ functions when you pass NULL of_node? If
> they do not crash then I suppose it is fine.

I just checked the code,
If CONFIG_OF isn't enabled, of_get_* will return error.
If CONFIG_OF is enabled, but NULL of_node is passed, of_get_* will also return
error, so it's fine ;)

> 
> Anyhow it might be good idea to do something like
> 
> 	if (!pdev->dev.of_node)
> 		return;
> 
> in the beginning of that function.

Sounds a good idea, will do in next version.

Thanks for your review,
Jisheng
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index fee26dc..51a55c6 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -20,6 +20,13 @@  Optional properties :
  - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
    This value which is by default 300ns is used to compute the tHIGH period.
 
+ - scl-gpios: specify the gpio related to SCL pin
+
+ - sda-gpios: specify the gpio related to SDA pin
+
+ - pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
+   bus recovery, call it "gpio" state
+
 Example :
 
 	i2c@f0000 {
@@ -42,4 +49,9 @@  Example :
 		i2c-sda-hold-time-ns = <300>;
 		i2c-sda-falling-time-ns = <300>;
 		i2c-scl-falling-time-ns = <300>;
+		pinctrl-names = "default", "gpio";
+		pinctrl-0 = <&pinctrl_i2c1_default>;
+		pinctrl-1 = <&pinctrl_i2c1_gpio>;
+		scl-gpios = <&porta 26 GPIO_ACTIVE_HIGH>;
+		sda-gpios = <&porta 27 GPIO_ACTIVE_HIGH>;
 	};
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 4255eaa..be40de1 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -399,7 +399,10 @@  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;
 		}
 		timeout--;
 		usleep_range(1000, 1100);
@@ -665,6 +668,7 @@  i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	/* wait for tx to complete */
 	if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) {
 		dev_err(dev->dev, "controller timed out\n");
+		i2c_recover_bus(&dev->adapter);
 		/* i2c_dw_init implicitly disables the adapter */
 		i2c_dw_init(dev);
 		ret = -ETIMEDOUT;
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index cd409e7..26c84ee 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -104,6 +104,10 @@  struct dw_i2c_dev {
 	u16			fs_lcnt;
 	int			(*acquire_lock)(struct dw_i2c_dev *dev);
 	void			(*release_lock)(struct dw_i2c_dev *dev);
+	struct i2c_bus_recovery_info rinfo;
+	struct pinctrl		*pinctrl;
+	struct pinctrl_state	*pinctrl_pins_default;
+	struct pinctrl_state	*pinctrl_pins_gpio;
 	bool			pm_runtime_disabled;
 };
 
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 1488cea..62bded9 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -33,6 +33,7 @@ 
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
@@ -148,6 +149,49 @@  static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
 	return 0;
 }
 
+static void i2c_dw_plat_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
+}
+
+static void i2c_dw_plat_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
+}
+
+static void i2c_dw_plat_init_recovery_info(struct dw_i2c_dev *dev,
+					   struct platform_device *pdev)
+{
+	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
+			PINCTRL_STATE_DEFAULT);
+	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
+			"gpio");
+	rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios", 0);
+	rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios", 0);
+
+	if (!gpio_is_valid(rinfo->sda_gpio) ||
+	    !gpio_is_valid(rinfo->scl_gpio) ||
+	    IS_ERR(dev->pinctrl_pins_default) ||
+	    IS_ERR(dev->pinctrl_pins_gpio)) {
+		dev_dbg(&pdev->dev, "recovery information incomplete\n");
+		return;
+	}
+
+	dev_dbg(&pdev->dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
+		rinfo->sda_gpio, rinfo->scl_gpio);
+
+	rinfo->prepare_recovery = i2c_dw_plat_prepare_recovery;
+	rinfo->unprepare_recovery = i2c_dw_plat_unprepare_recovery;
+	rinfo->recover_bus = i2c_generic_gpio_recovery;
+	dev->adapter.bus_recovery_info = rinfo;
+}
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -165,6 +209,12 @@  static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (!dev)
 		return -ENOMEM;
 
+	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(dev->pinctrl))
+		return PTR_ERR(dev->pinctrl);
+
+	i2c_dw_plat_init_recovery_info(dev, pdev);
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	dev->base = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(dev->base))