[{"id":1776963,"web_url":"http://patchwork.ozlabs.org/comment/1776963/","msgid":"<1506596321.16112.156.camel@linux.intel.com>","list_archive_url":null,"date":"2017-09-28T10:58:41","subject":"Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option","submitter":{"id":8583,"url":"http://patchwork.ozlabs.org/api/people/8583/","name":"Andy Shevchenko","email":"andriy.shevchenko@linux.intel.com"},"content":"On Wed, 2017-08-30 at 14:17 +0800, Phil Reid wrote:\n> From: Tim Sander \n> \n> This patch contains much input from Phil Reid and has been tested\n> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the\n> SCL and SDA GPIO's. I am still a little unsure about the recover\n> in the timeout case (i2c-designware-core.c:770) as i could not\n> test this codepath.\n\n \n> -\tif (abort_source & DW_IC_TX_ARB_LOST)\n> +\tif (abort_source & DW_IC_TX_ARB_LOST) {\n> +\t\ti2c_recover_bus(&dev->adapter);\n> \t\treturn -EAGAIN;\n\n> -\telse if (abort_source & DW_IC_TX_ABRT_GCALL_READ)\n> +\t} else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)\n\nelse is redundant.\n\n> \t\treturn -EINVAL; /* wrong msgs[] data */\n> \telse\n\nDitto.\n\n> \t\treturn -EIO;\n\n> +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)\n> +{\n> +\tstruct i2c_bus_recovery_info *rinfo = &dev->rinfo;\n> +\tstruct i2c_adapter *adap = &dev->adapter;\n> +\tstruct gpio_desc *gpio;\n> +\tint r;\n> +\n> +\tgpio = devm_gpiod_get(dev->dev, \"scl\", GPIOD_OUT_HIGH);\n> +\tif (IS_ERR(gpio)) {\n> +\t\tr = PTR_ERR(gpio);\n\n> +\t\tif ((r == -ENOENT) || (r == -ENOENT))\n\nCopy'n'paste typo?\n\n> +\t\t\treturn 0;\n> +\t\treturn r;\n> +\t}\n> +\trinfo->scl_gpiod = gpio;\n> +\n> +\tgpio = devm_gpiod_get_optional(dev->dev, \"sda\", GPIOD_IN);\n> +\tif (IS_ERR(gpio))\n> +\t\treturn PTR_ERR(gpio);\n> +\trinfo->sda_gpiod = gpio;\n> +\n> +\trinfo->recover_bus = i2c_generic_scl_recovery;\n> +\trinfo->prepare_recovery = i2c_dw_prepare_recovery;\n> +\trinfo->unprepare_recovery = i2c_dw_unprepare_recovery;\n> +\tadap->bus_recovery_info = rinfo;\n> +\n\n> +\tdev_info(dev->dev,\n> +\t\t\"adapter: %s running with gpio recovery mode! scl:%i\n> sda:%i\\n\",\n> +\t\tadap->name, !!rinfo->scl_gpiod, !!rinfo->sda_gpiod);\n\nInstead of doing numbers, better just to list available descriptors,\ne.g.\n\n...(\"... %s scl\\n\", rinfo->sda_gpiod ? \"sda,\");\n\nNo need to explain that scl doesn't need any check here.\n\nAnd I'm not sure why do you need adap->name here. Can you show an\nexample of output from your test platform?\n\n> +\tif (!ret)\n> +\t\tret = i2c_dw_init_recovery_info(dev);\n\nBetter to \nif (ret)\n return ret;\n\nreturn i2c...();\n\n> +\n> \treturn ret;","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2sN24154z9t2Q\n\tfor ;\n\tThu, 28 Sep 2017 21:07:14 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751831AbdI1LHN (ORCPT );\n\tThu, 28 Sep 2017 07:07:13 -0400","from mga03.intel.com ([134.134.136.65]:62630 \"EHLO mga03.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751816AbdI1LHM (ORCPT );\n\tThu, 28 Sep 2017 07:07:12 -0400","from fmsmga003.fm.intel.com ([10.253.24.29])\n\tby orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t28 Sep 2017 04:07:11 -0700","from smile.fi.intel.com (HELO smile) ([10.237.72.86])\n\tby FMSMGA003.fm.intel.com with ESMTP; 28 Sep 2017 04:07:09 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,449,1500966000\"; d=\"scan'208\";a=\"904716196\"","Message-ID":"<1506596321.16112.156.camel@linux.intel.com>","Subject":"Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option","From":"Andy Shevchenko ","To":"Phil Reid , jarkko.nikula@linux.intel.com,\n\tmika.westerberg@linux.intel.com, wsa@the-dreams.de,\n\ttim@krieglstein.org, linux-i2c@vger.kernel.org","Date":"Thu, 28 Sep 2017 13:58:41 +0300","In-Reply-To":"<1504073857-122449-5-git-send-email-preid@electromag.com.au>","References":"<1504073857-122449-1-git-send-email-preid@electromag.com.au>\n\t<1504073857-122449-5-git-send-email-preid@electromag.com.au>","Organization":"Intel Finland Oy","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.26.0-1 ","Mime-Version":"1.0","Content-Transfer-Encoding":"7bit","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1777026,"web_url":"http://patchwork.ozlabs.org/comment/1777026/","msgid":"","list_archive_url":null,"date":"2017-09-28T13:21:16","subject":"Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option","submitter":{"id":43309,"url":"http://patchwork.ozlabs.org/api/people/43309/","name":"Jarkko Nikula","email":"jarkko.nikula@linux.intel.com"},"content":"On 08/30/2017 09:17 AM, Phil Reid wrote:\n> From: Tim Sander \n> \n> This patch contains much input from Phil Reid and has been tested\n> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the\n> SCL and SDA GPIO's. I am still a little unsure about the recover\n> in the timeout case (i2c-designware-core.c:770) as i could not\n> test this codepath.\n> \n> Signed-off-by: Tim Sander \n> Signed-off-by: Phil Reid \n> ---\n> drivers/i2c/busses/i2c-designware-common.c | 11 ++++--\n> drivers/i2c/busses/i2c-designware-core.h | 1 +\n> drivers/i2c/busses/i2c-designware-master.c | 57 ++++++++++++++++++++++++++++++\n> 3 files changed, 66 insertions(+), 3 deletions(-)\n> \nWhile taking into account Andy's comments please modify the last \nsentence in the above commit log - i2c-designware-core.c doesn't exist \nanymore. Maybe better is to have the uncertainty documented as a \n\"REVISIT:\" comment in the code etc.\n\n> @@ -254,9 +258,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)\n> \tfor_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))\n> \t\tdev_err(dev->dev, \"%s: %s\\n\", __func__, abort_sources[i]);\n> \n> -\tif (abort_source & DW_IC_TX_ARB_LOST)\n> +\tif (abort_source & DW_IC_TX_ARB_LOST) {\n> +\t\ti2c_recover_bus(&dev->adapter);\n\nAre you sure about doing recovery for arbitration lost case? To me it \nseems wrong to do it if another master is accessing the bus.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2wLn6Tn8z9tXj\n\tfor ;\n\tThu, 28 Sep 2017 23:21:21 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753181AbdI1NVT (ORCPT );\n\tThu, 28 Sep 2017 09:21:19 -0400","from mga11.intel.com ([192.55.52.93]:19091 \"EHLO mga11.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753259AbdI1NVT (ORCPT );\n\tThu, 28 Sep 2017 09:21:19 -0400","from fmsmga002.fm.intel.com ([10.253.24.26])\n\tby fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t28 Sep 2017 06:21:18 -0700","from mylly.fi.intel.com (HELO [10.237.72.59]) ([10.237.72.59])\n\tby fmsmga002.fm.intel.com with ESMTP; 28 Sep 2017 06:21:17 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,450,1500966000\"; d=\"scan'208\";a=\"1224780759\"","Subject":"Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option","To":"Phil Reid ,\n\tandriy.shevchenko@linux.intel.com, mika.westerberg@linux.intel.com,\n\twsa@the-dreams.de, tim@krieglstein.org, linux-i2c@vger.kernel.org","References":"<1504073857-122449-1-git-send-email-preid@electromag.com.au>\n\t<1504073857-122449-5-git-send-email-preid@electromag.com.au>","From":"Jarkko Nikula ","Message-ID":"","Date":"Thu, 28 Sep 2017 16:21:16 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<1504073857-122449-5-git-send-email-preid@electromag.com.au>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1777376,"web_url":"http://patchwork.ozlabs.org/comment/1777376/","msgid":"<22b2a4a3-6b79-3845-586c-fe98c75411fe@electromag.com.au>","list_archive_url":null,"date":"2017-09-29T07:00:53","subject":"Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option","submitter":{"id":66145,"url":"http://patchwork.ozlabs.org/api/people/66145/","name":"Phil Reid","email":"preid@electromag.com.au"},"content":"Thanks for the review.\n\nOn 28/09/2017 18:58, Andy Shevchenko wrote:\n> On Wed, 2017-08-30 at 14:17 +0800, Phil Reid wrote:\n>> From: Tim Sander \n>>\n>> This patch contains much input from Phil Reid and has been tested\n>> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the\n>> SCL and SDA GPIO's. I am still a little unsure about the recover\n>> in the timeout case (i2c-designware-core.c:770) as i could not\n>> test this codepath.\n> \n> \n>> -\tif (abort_source & DW_IC_TX_ARB_LOST)\n>> +\tif (abort_source & DW_IC_TX_ARB_LOST) {\n>> +\t\ti2c_recover_bus(&dev->adapter);\n>> \t\treturn -EAGAIN;\n> \n>> -\telse if (abort_source & DW_IC_TX_ABRT_GCALL_READ)\n>> +\t} else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)\n> \n> else is redundant.\n> \n>> \t\treturn -EINVAL; /* wrong msgs[] data */\n>> \telse\n> \n> Ditto.\nYep.\n> \n>> \t\treturn -EIO;\n> \n>> +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)\n>> +{\n>> +\tstruct i2c_bus_recovery_info *rinfo = &dev->rinfo;\n>> +\tstruct i2c_adapter *adap = &dev->adapter;\n>> +\tstruct gpio_desc *gpio;\n>> +\tint r;\n>> +\n>> +\tgpio = devm_gpiod_get(dev->dev, \"scl\", GPIOD_OUT_HIGH);\n>> +\tif (IS_ERR(gpio)) {\n>> +\t\tr = PTR_ERR(gpio);\n> \n>> +\t\tif ((r == -ENOENT) || (r == -ENOENT))\n> \n> Copy'n'paste typo?\nYep.\n\n> \n>> +\t\t\treturn 0;\n>> +\t\treturn r;\n>> +\t}\n>> +\trinfo->scl_gpiod = gpio;\n>> +\n>> +\tgpio = devm_gpiod_get_optional(dev->dev, \"sda\", GPIOD_IN);\n>> +\tif (IS_ERR(gpio))\n>> +\t\treturn PTR_ERR(gpio);\n>> +\trinfo->sda_gpiod = gpio;\n>> +\n>> +\trinfo->recover_bus = i2c_generic_scl_recovery;\n>> +\trinfo->prepare_recovery = i2c_dw_prepare_recovery;\n>> +\trinfo->unprepare_recovery = i2c_dw_unprepare_recovery;\n>> +\tadap->bus_recovery_info = rinfo;\n>> +\n> \n>> +\tdev_info(dev->dev,\n>> +\t\t\"adapter: %s running with gpio recovery mode! scl:%i\n>> sda:%i\\n\",\n>> +\t\tadap->name, !!rinfo->scl_gpiod, !!rinfo->sda_gpiod);\n> \n> Instead of doing numbers, better just to list available descriptors,\n> e.g.\n> \n> ...(\"... %s scl\\n\", rinfo->sda_gpiod ? \"sda,\");\nOk.\n\n> \n> No need to explain that scl doesn't need any check here.\n> \n> And I'm not sure why do you need adap->name here. Can you show an\n> example of output from your test platform?\n\nGood question. can't see a need.\n\n> \n>> +\tif (!ret)\n>> +\t\tret = i2c_dw_init_recovery_info(dev);\n> \n> Better to\n> if (ret)\n> return ret;\n> \n> return i2c...();\n> \n>> +\n>> \treturn ret;\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3MsQ1300z9t2c\n\tfor ;\n\tFri, 29 Sep 2017 17:00:58 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750847AbdI2HA5 (ORCPT );\n\tFri, 29 Sep 2017 03:00:57 -0400","from anchovy2.45ru.net.au ([203.30.46.146]:54528 \"EHLO\n\tanchovy.45ru.net.au\" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org\n\twith ESMTP id S1750709AbdI2HA4 (ORCPT\n\t); Fri, 29 Sep 2017 03:00:56 -0400","(qmail 22830 invoked by uid 5089); 29 Sep 2017 07:00:55 -0000","by simscan 1.2.0 ppid: 22768, pid: 22769, t: 0.0469s\n\tscanners: regex: 1.2.0 attach: 1.2.0 clamav: 0.88.3/m:40/d:1950","from unknown (HELO ?192.168.0.122?)\n\t(preid@electromag.com.au@203.59.230.133)\n\tby anchovy3.45ru.net.au with ESMTPA; 29 Sep 2017 07:00:54 -0000"],"Subject":"Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option","To":"Andy Shevchenko ,\n\tjarkko.nikula@linux.intel.com, mika.westerberg@linux.intel.com,\n\twsa@the-dreams.de, tim@krieglstein.org, linux-i2c@vger.kernel.org","References":"<1504073857-122449-1-git-send-email-preid@electromag.com.au>\n\t<1504073857-122449-5-git-send-email-preid@electromag.com.au>\n\t<1506596321.16112.156.camel@linux.intel.com>","From":"Phil Reid ","Message-ID":"<22b2a4a3-6b79-3845-586c-fe98c75411fe@electromag.com.au>","Date":"Fri, 29 Sep 2017 15:00:53 +0800","User-Agent":"Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<1506596321.16112.156.camel@linux.intel.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-AU","Content-Transfer-Encoding":"7bit","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1781235,"web_url":"http://patchwork.ozlabs.org/comment/1781235/","msgid":"","list_archive_url":null,"date":"2017-10-06T05:56:35","subject":"Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option","submitter":{"id":66145,"url":"http://patchwork.ozlabs.org/api/people/66145/","name":"Phil Reid","email":"preid@electromag.com.au"},"content":"On 28/09/2017 21:21, Jarkko Nikula wrote:\n> On 08/30/2017 09:17 AM, Phil Reid wrote:\n>> From: Tim Sander \n>>\n>> This patch contains much input from Phil Reid and has been tested\n>> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the\n>> SCL and SDA GPIO's. I am still a little unsure about the recover\n>> in the timeout case (i2c-designware-core.c:770) as i could not\n>> test this codepath.\n>>\n>> Signed-off-by: Tim Sander \n>> Signed-off-by: Phil Reid \n>> ---\n>>   drivers/i2c/busses/i2c-designware-common.c | 11 ++++--\n>>   drivers/i2c/busses/i2c-designware-core.h   |  1 +\n>>   drivers/i2c/busses/i2c-designware-master.c | 57 ++++++++++++++++++++++++++++++\n>>   3 files changed, 66 insertions(+), 3 deletions(-)\n>>\n> While taking into account Andy's comments please modify the last sentence in the above commit log - i2c-designware-core.c doesn't exist anymore. Maybe better is \n> to have the uncertainty documented as a \"REVISIT:\" comment in the code etc.\n> \n>> @@ -254,9 +258,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)\n>>       for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))\n>>           dev_err(dev->dev, \"%s: %s\\n\", __func__, abort_sources[i]);\n>> -    if (abort_source & DW_IC_TX_ARB_LOST)\n>> +    if (abort_source & DW_IC_TX_ARB_LOST) {\n>> +        i2c_recover_bus(&dev->adapter);\n> \n> Are you sure about doing recovery for arbitration lost case? To me it seems wrong to do it if another master is accessing the bus.\n> \nyes I think your right.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y7f5z6Z2jz9t2h\n\tfor ;\n\tFri, 6 Oct 2017 16:56:39 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750791AbdJFF4i (ORCPT );\n\tFri, 6 Oct 2017 01:56:38 -0400","from anchovy3.45ru.net.au ([203.30.46.155]:52838 \"EHLO\n\tanchovy.45ru.net.au\" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org\n\twith ESMTP id S1750732AbdJFF4h (ORCPT\n\t); Fri, 6 Oct 2017 01:56:37 -0400","(qmail 4157 invoked by uid 5089); 6 Oct 2017 05:56:35 -0000","by simscan 1.2.0 ppid: 4089, pid: 4090, t: 0.0341s\n\tscanners: regex: 1.2.0 attach: 1.2.0 clamav: 0.88.3/m:40/d:1950","from unknown (HELO ?192.168.0.122?)\n\t(preid@electromag.com.au@203.59.230.133)\n\tby anchovy2.45ru.net.au with ESMTPA; 6 Oct 2017 05:56:35 -0000"],"Subject":"Re: [PATCH v3 4/4] i2c: designware: add i2c gpio recovery option","To":"Jarkko Nikula ,\n\tandriy.shevchenko@linux.intel.com, mika.westerberg@linux.intel.com,\n\twsa@the-dreams.de, tim@krieglstein.org, linux-i2c@vger.kernel.org","References":"<1504073857-122449-1-git-send-email-preid@electromag.com.au>\n\t<1504073857-122449-5-git-send-email-preid@electromag.com.au>\n\t","From":"Phil Reid ","Message-ID":"","Date":"Fri, 6 Oct 2017 13:56:35 +0800","User-Agent":"Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-AU","Content-Transfer-Encoding":"8bit","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}}]