[{"id":3194019,"web_url":"http://patchwork.ozlabs.org/comment/3194019/","msgid":"<20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>","list_archive_url":null,"date":"2023-10-05T21:58:32","subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","submitter":{"id":85956,"url":"http://patchwork.ozlabs.org/api/people/85956/","name":"Andi Shyti","email":"andi.shyti@kernel.org"},"content":"Hi Chris,\n\nLooks good, just a few questions.\n\n> +static int\n> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)\n> +{\n> +\tstruct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);\n> +\tint ret;\n> +\tu32 val;\n> +\n> +\tdev_dbg(&adap->dev, \"Trying i2c bus recovery\\n\");\n> +\twritel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);\n> +\tret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,\n> +\t\t\t\t\t!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),\n> +\t\t\t\t\t1000, 5000);\n\nhere you are busy looping for 1ms between reads which is a long\ntime. Why not using read_poll_timeout() instead?\n\n> +\tif (ret) {\n> +\t\tdev_err(&adap->dev, \"recovery timeout\\n\");\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (val & MV64XXX_I2C_UNSTUCK_ERROR) {\n> +\t\tdev_err(&adap->dev, \"recovery failed\\n\");\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n> +\tdev_info(&adap->dev, \"recovery complete after %d pulses\\n\", MV64XXX_I2C_UNSTUCK_COUNT(val));\n\ndev_dbg?\n\n> +\treturn 0;\n> +}\n> +\n\n[...]\n\n> -\tif (of_device_is_compatible(np, \"marvell,mv78230-a0-i2c\")) {\n> +\tif (of_device_is_compatible(np, \"marvell,mv78230-a0-i2c\") ||\n> +\t of_device_is_compatible(np, \"marvell,armada-8k-i2c\")) {\n\nshould this be part of a different patch?\n\n> \t\tdrv_data->offload_enabled = false;\n> \t\t/* The delay is only needed in standard mode (100kHz) */\n> \t\tif (bus_freq <= I2C_MAX_STANDARD_MODE_FREQ)\n> @@ -936,8 +973,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,\n> }\n> #endif /* CONFIG_OF */\n> \n> -static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,\n> -\t\t\t\t\t struct device *dev)\n> +static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,\n> +\t\t\t\t\t struct device *dev)\n> +{\n> +\tstruct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;\n> +\n> +\tdev_info(dev, \"using FSM for recovery\\n\");\n\ndev_dbg?\n\n> +\trinfo->recover_bus = mv64xxx_i2c_recover_bus;\n> +\tdrv_data->adapter.bus_recovery_info = rinfo;\n> +\n> +\treturn 0;\n> +\n> +}\n> +\n\n[...]\n\n> +\t/* optional unstuck support */\n> +\tres = platform_get_resource(pd, IORESOURCE_MEM, 1);\n> +\tif (res) {\n> +\t\tdrv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);\n> +\t\tif (IS_ERR(drv_data->unstuck_reg))\n> +\t\t\treturn PTR_ERR(drv_data->unstuck_reg);\n\nOK, we failed to ioremap... but instead of returning an error,\nwouldn't it be better to just set unstuck_reg to NULL and move\nforward without unstuck support?\n\nMaybe you will stil crash later because something might have\nhappened, but failing on purpose on an optional feature looks a\nbit too drastic to me. What do you think?\n\nThanks,\nAndi","headers":{"Return-Path":"\n ","X-Original-To":["incoming-dt@patchwork.ozlabs.org","devicetree@vger.kernel.org"],"Delivered-To":"patchwork-incoming-dt@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=lG/Y1i8O;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=permerror (SPF Permanent Error: More than 10 MX records returned)\n smtp.mailfrom=vger.kernel.org (client-ip=2604:1380:45e3:2400::1;\n helo=sv.mirrors.kernel.org;\n envelope-from=devicetree+bounces-6320-incoming-dt=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"lG/Y1i8O\""],"Received":["from sv.mirrors.kernel.org (sv.mirrors.kernel.org\n [IPv6:2604:1380:45e3:2400::1])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4S1lnH2hXBz1yqH\n\tfor ;\n Fri, 6 Oct 2023 08:58:42 +1100 (AEDT)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sv.mirrors.kernel.org (Postfix) with ESMTP id D4AB7281F93\n\tfor ; Thu, 5 Oct 2023 21:58:40 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 1256D42C11;\n\tThu, 5 Oct 2023 21:58:40 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id BA98E42BF4\n\tfor ; Thu, 5 Oct 2023 21:58:39 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 44926C433C8;\n\tThu, 5 Oct 2023 21:58:37 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1696543119;\n\tbh=wg9da7L1CQtw+tuYOmZfz5J+NgLFvScI+3C0EoG/geo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lG/Y1i8OpAQPQat88kfyzi7NWqcNQ9Cz13NjQkWCQDOP2ullmbktnb7+j7T7OUkOb\n\t f5JlDNF7/gNrGv6WsIbY9PvLXmfivEukYrAgLUezsgB+ory5ukH15S8Xv4FryuoHvm\n\t cV6XP4Z6Hh8AKexq5/ZCHA5kDyDTcR1bhFJGILAMXqivj54d0rEwKfK6rNAAZDIKju\n\t 4UmdJQrB8vGY0vpbClN4hFjfJpvZNUa0FqIuD82KqRjuSbdHiAIHw7AxVLYJ98X3s9\n\t Ahf54l68UPAnMvovAyIJZLRuXff0bqHURlk7YlgOcDStezWyy6odd8F6NpHTigPFWo\n\t 88Q1dlHsLuGAw==","Date":"Thu, 5 Oct 2023 23:58:32 +0200","From":"Andi Shyti ","To":"Chris Packham ","Cc":"gregory.clement@bootlin.com, robh+dt@kernel.org,\n\tkrzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,\n\tpierre.gondois@arm.com, linux-i2c@vger.kernel.org,\n\tdevicetree@vger.kernel.org, linux-kernel@vger.kernel.org","Subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","Message-ID":"<20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>","References":"<20230926234801.4078042-1-chris.packham@alliedtelesis.co.nz>\n <20230926234801.4078042-4-chris.packham@alliedtelesis.co.nz>","Precedence":"bulk","X-Mailing-List":"devicetree@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20230926234801.4078042-4-chris.packham@alliedtelesis.co.nz>"}},{"id":3194032,"web_url":"http://patchwork.ozlabs.org/comment/3194032/","msgid":"<57c27eb5-1145-4a84-a7b6-ff785d7a1eeb@alliedtelesis.co.nz>","list_archive_url":null,"date":"2023-10-05T22:39:03","subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","submitter":{"id":27499,"url":"http://patchwork.ozlabs.org/api/people/27499/","name":"Chris Packham","email":"chris.packham@alliedtelesis.co.nz"},"content":"On 6/10/23 10:58, Andi Shyti wrote:\n> Hi Chris,\n>\n> Looks good, just a few questions.\n>\n>> +static int\n>> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)\n>> +{\n>> +\tstruct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);\n>> +\tint ret;\n>> +\tu32 val;\n>> +\n>> +\tdev_dbg(&adap->dev, \"Trying i2c bus recovery\\n\");\n>> +\twritel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);\n>> +\tret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,\n>> +\t\t\t\t\t!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),\n>> +\t\t\t\t\t1000, 5000);\n> here you are busy looping for 1ms between reads which is a long\n> time. Why not using read_poll_timeout() instead?\n\nI needed to use the atomic variant because this ends up getting called \nfrom an interrupt handler (mv64xxx_i2c_intr() -> mv64xxx_i2c_fsm()). I \nprobably don't need to wait so long between reads those times were just \npulled out of thin air. In my experimentation the faults that can be \ncleared do so within a couple of clocks, if it hasn't cleared within 8 \nclocks it's not going to.\n\n>> +\tif (ret) {\n>> +\t\tdev_err(&adap->dev, \"recovery timeout\\n\");\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tif (val & MV64XXX_I2C_UNSTUCK_ERROR) {\n>> +\t\tdev_err(&adap->dev, \"recovery failed\\n\");\n>> +\t\treturn -EBUSY;\n>> +\t}\n>> +\n>> +\tdev_info(&adap->dev, \"recovery complete after %d pulses\\n\", MV64XXX_I2C_UNSTUCK_COUNT(val));\n> dev_dbg?\nack.\n>> +\treturn 0;\n>> +}\n>> +\n> [...]\n>\n>> -\tif (of_device_is_compatible(np, \"marvell,mv78230-a0-i2c\")) {\n>> +\tif (of_device_is_compatible(np, \"marvell,mv78230-a0-i2c\") ||\n>> +\t of_device_is_compatible(np, \"marvell,armada-8k-i2c\")) {\n> should this be part of a different patch?\n\nYes sorry. Originally I was going to use a new compatible to indicate \nthe unstuck support but went with the 2nd reg cell so this is unnecessary.\n\n>\n>> \t\tdrv_data->offload_enabled = false;\n>> \t\t/* The delay is only needed in standard mode (100kHz) */\n>> \t\tif (bus_freq <= I2C_MAX_STANDARD_MODE_FREQ)\n>> @@ -936,8 +973,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,\n>> }\n>> #endif /* CONFIG_OF */\n>> \n>> -static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,\n>> -\t\t\t\t\t struct device *dev)\n>> +static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,\n>> +\t\t\t\t\t struct device *dev)\n>> +{\n>> +\tstruct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;\n>> +\n>> +\tdev_info(dev, \"using FSM for recovery\\n\");\n> dev_dbg?\n>\n>> +\trinfo->recover_bus = mv64xxx_i2c_recover_bus;\n>> +\tdrv_data->adapter.bus_recovery_info = rinfo;\n>> +\n>> +\treturn 0;\n>> +\n>> +}\n>> +\n> [...]\n>\n>> +\t/* optional unstuck support */\n>> +\tres = platform_get_resource(pd, IORESOURCE_MEM, 1);\n>> +\tif (res) {\n>> +\t\tdrv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);\n>> +\t\tif (IS_ERR(drv_data->unstuck_reg))\n>> +\t\t\treturn PTR_ERR(drv_data->unstuck_reg);\n> OK, we failed to ioremap... but instead of returning an error,\n> wouldn't it be better to just set unstuck_reg to NULL and move\n> forward without unstuck support?\n>\n> Maybe you will stil crash later because something might have\n> happened, but failing on purpose on an optional feature looks a\n> bit too drastic to me. What do you think?\n\nPersonally I think if the reg property is supplied in the dts we'd \nbetter be able to use it. If the feature is not wanted then the way to \nindicate this is by supplying only one reg cell.\n\nI'd be happy with a dev_warn() and unstuck_reg = NULL if that helps get \nthis landed.\n\n>\n> Thanks,\n> Andi","headers":{"Return-Path":"\n ","X-Original-To":["incoming-dt@patchwork.ozlabs.org","devicetree@vger.kernel.org"],"Delivered-To":"patchwork-incoming-dt@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=alliedtelesis.co.nz header.i=@alliedtelesis.co.nz\n header.a=rsa-sha256 header.s=mail181024 header.b=E1XFAgsZ;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=permerror (SPF Permanent Error: More than 10 MX records returned)\n smtp.mailfrom=vger.kernel.org (client-ip=2604:1380:45e3:2400::1;\n helo=sv.mirrors.kernel.org;\n envelope-from=devicetree+bounces-6323-incoming-dt=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=alliedtelesis.co.nz\n header.i=@alliedtelesis.co.nz header.b=\"E1XFAgsZ\""],"Received":["from sv.mirrors.kernel.org (sv.mirrors.kernel.org\n [IPv6:2604:1380:45e3:2400::1])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature ECDSA (secp384r1))\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4S1mh56WbJz1yqH\n\tfor ;\n Fri, 6 Oct 2023 09:39:17 +1100 (AEDT)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sv.mirrors.kernel.org (Postfix) with ESMTP id F2721281FB3\n\tfor ; Thu, 5 Oct 2023 22:39:15 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 7808842BFA;\n\tThu, 5 Oct 2023 22:39:15 +0000 (UTC)","from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net\n [23.128.96.19])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id BF790154AB\n\tfor ; Thu, 5 Oct 2023 22:39:11 +0000 (UTC)","from gate2.alliedtelesis.co.nz (gate2.alliedtelesis.co.nz\n [202.36.163.20])\n\tby lindbergh.monkeyblade.net (Postfix) with ESMTPS id EC8BDE9\n\tfor ; Thu, 5 Oct 2023 15:39:08 -0700 (PDT)","from svr-chch-seg1.atlnz.lc (mmarshal3.atlnz.lc [10.32.18.43])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest\n SHA256)\n\t(Client did not present a certificate)\n\tby gate2.alliedtelesis.co.nz (Postfix) with ESMTPS id DDFD42C07F0;\n\tFri, 6 Oct 2023 11:39:04 +1300 (NZDT)","from svr-chch-ex2.atlnz.lc (Not Verified[2001:df5:b000:bc8::76]) by\n svr-chch-seg1.atlnz.lc with Trustwave SEG (v8,2,6,11305)\n\tid ; Fri, 06 Oct 2023 11:39:04 +1300","from svr-chch-ex1.atlnz.lc (2001:df5:b000:bc8::77) by\n svr-chch-ex2.atlnz.lc (2001:df5:b000:bc8::76) with Microsoft SMTP Server\n (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id\n 15.2.1118.37; Fri, 6 Oct 2023 11:39:04 +1300","from svr-chch-ex2.atlnz.lc (2001:df5:b000:bc8::76) by\n svr-chch-ex1.atlnz.lc (2001:df5:b000:bc8::77) with Microsoft SMTP Server\n (TLS) id 15.0.1497.48; Fri, 6 Oct 2023 11:39:04 +1300","from svr-chch-ex2.atlnz.lc ([fe80::a9eb:c9b7:8b52:9567]) by\n svr-chch-ex2.atlnz.lc ([fe80::a9eb:c9b7:8b52:9567%15]) with mapi id\n 15.02.1118.037; Fri, 6 Oct 2023 11:39:04 +1300"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=alliedtelesis.co.nz;\n\ts=mail181024; t=1696545544;\n\tbh=Jln0tzUpoR2xr/aeOoHAnxoXo48BHvn0YAHWLviV5iA=;\n\th=From:To:CC:Subject:Date:References:In-Reply-To:From;\n\tb=E1XFAgsZxIZ6ukokAFv30wxjhY9eMRuvSAg2OsJM5WbeqXUceAeGCbXvFbE4C3aNv\n\t 8klwjIHEbo6/BIj8NSnYX7dz7ZiT8saxcrtGtLf4fqZrkma/krIK48VqCkNPZvjmCv\n\t iy2nPoAhbv83VU5ICi+PddeFUfnnjx3ZCVdCkQCha4nwBUvGjIS5TLYXivjA4FwWaA\n\t THaCSdMa12jUmqHp3bW00gTibJ0DiMoMg9qhEFPrJcLD2qMUb7K1000Lu3bbiqi6eq\n\t XlgnJnjCREXpNtpx9YPsIn7TQdfAKOxUqsFWMlSqVWOlcTcMF7U1jurWvZVWEtBAAd\n\t ZfcrAiho28kog==","From":"Chris Packham ","To":"Andi Shyti ","CC":"\"gregory.clement@bootlin.com\" ,\n\t\"robh+dt@kernel.org\" ,\n\t\"krzysztof.kozlowski+dt@linaro.org\" ,\n\t\"conor+dt@kernel.org\" , \"pierre.gondois@arm.com\"\n\t, \"linux-i2c@vger.kernel.org\"\n\t, \"devicetree@vger.kernel.org\"\n\t, \"linux-kernel@vger.kernel.org\"\n\t","Subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","Thread-Topic":"[PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","Thread-Index":"AQHZ8NPnYKzvy/9jwESCWo7X3TBvqLA68pMAgAALUoA=","Date":"Thu, 5 Oct 2023 22:39:03 +0000","Message-ID":"<57c27eb5-1145-4a84-a7b6-ff785d7a1eeb@alliedtelesis.co.nz>","References":"<20230926234801.4078042-1-chris.packham@alliedtelesis.co.nz>\n <20230926234801.4078042-4-chris.packham@alliedtelesis.co.nz>\n <20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>","In-Reply-To":"<20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>","Accept-Language":"en-NZ, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.33.22.30]","Content-Type":"text/plain; charset=\"utf-8\"","Content-ID":"","Content-Transfer-Encoding":"base64","Precedence":"bulk","X-Mailing-List":"devicetree@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","X-SEG-SpamProfiler-Analysis":"v=2.3 cv=Vf2Jw2h9 c=1 sm=1 tr=0\n a=Xf/6aR1Nyvzi7BryhOrcLQ==:117 a=xqWC_Br6kY4A:10 a=75chYTbOgJ0A:10\n a=IkcTkHD0fZMA:10 a=bhdUkHdE2iEA:10 a=FqOU3ekQ0Ek1LeW_-iAA:9\n a=QEXdDO2ut3YA:10","X-SEG-SpamProfiler-Score":"0","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n\tDKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,\n\tSPF_HELO_PASS,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no\n\tversion=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n\tlindbergh.monkeyblade.net"}},{"id":3194039,"web_url":"http://patchwork.ozlabs.org/comment/3194039/","msgid":"<20231005230737.6j57y7d27vnsbws3@zenone.zhora.eu>","list_archive_url":null,"date":"2023-10-05T23:07:37","subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","submitter":{"id":85956,"url":"http://patchwork.ozlabs.org/api/people/85956/","name":"Andi Shyti","email":"andi.shyti@kernel.org"},"content":"Hi Chris,\n\n> >> +static int\n> >> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)\n> >> +{\n> >> +\tstruct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);\n> >> +\tint ret;\n> >> +\tu32 val;\n> >> +\n> >> +\tdev_dbg(&adap->dev, \"Trying i2c bus recovery\\n\");\n> >> +\twritel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);\n> >> +\tret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,\n> >> +\t\t\t\t\t!(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),\n> >> +\t\t\t\t\t1000, 5000);\n> > here you are busy looping for 1ms between reads which is a long\n> > time. Why not using read_poll_timeout() instead?\n> \n> I needed to use the atomic variant because this ends up getting called \n> from an interrupt handler (mv64xxx_i2c_intr() -> mv64xxx_i2c_fsm()). I \n> probably don't need to wait so long between reads those times were just \n> pulled out of thin air. In my experimentation the faults that can be \n> cleared do so within a couple of clocks, if it hasn't cleared within 8 \n> clocks it's not going to.\n\nIt's still a long time to wait in atomic context...\nreadl_poll_timeout_atomic() waits in udelays, where the maximum\naccepted waiting time is 10us. Here you are waiting 100 times\nmore.\n\nIf we can't be within that value I would rather use a thread.\n\nOr, you could also consider using threaded_irq()... but this\nmight have a bit of a higher impact.\n\n[...]\n\n> >> +\t/* optional unstuck support */\n> >> +\tres = platform_get_resource(pd, IORESOURCE_MEM, 1);\n> >> +\tif (res) {\n> >> +\t\tdrv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);\n> >> +\t\tif (IS_ERR(drv_data->unstuck_reg))\n> >> +\t\t\treturn PTR_ERR(drv_data->unstuck_reg);\n> > OK, we failed to ioremap... but instead of returning an error,\n> > wouldn't it be better to just set unstuck_reg to NULL and move\n> > forward without unstuck support?\n> >\n> > Maybe you will stil crash later because something might have\n> > happened, but failing on purpose on an optional feature looks a\n> > bit too drastic to me. What do you think?\n> \n> Personally I think if the reg property is supplied in the dts we'd \n> better be able to use it. If the feature is not wanted then the way to \n> indicate this is by supplying only one reg cell.\n> \n> I'd be happy with a dev_warn() and unstuck_reg = NULL if that helps get \n> this landed.\n\nDon't ahve a strong opinion... as you like. Mine is just an\nopinion and your argument is valid :-)\n\nAndi","headers":{"Return-Path":"\n ","X-Original-To":["incoming-dt@patchwork.ozlabs.org","devicetree@vger.kernel.org"],"Delivered-To":"patchwork-incoming-dt@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=eqVpbGcZ;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=permerror (SPF Permanent Error: More than 10 MX records returned)\n smtp.mailfrom=vger.kernel.org (client-ip=2604:1380:45e3:2400::1;\n helo=sv.mirrors.kernel.org;\n envelope-from=devicetree+bounces-6324-incoming-dt=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"eqVpbGcZ\""],"Received":["from sv.mirrors.kernel.org (sv.mirrors.kernel.org\n [IPv6:2604:1380:45e3:2400::1])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4S1nK55gpZz1yqD\n\tfor ;\n Fri, 6 Oct 2023 10:07:53 +1100 (AEDT)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sv.mirrors.kernel.org (Postfix) with ESMTP id C0D8A28200C\n\tfor ; Thu, 5 Oct 2023 23:07:47 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id BC9CC43681;\n\tThu, 5 Oct 2023 23:07:43 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 6CF7314001\n\tfor ; Thu, 5 Oct 2023 23:07:43 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 66543C433C7;\n\tThu, 5 Oct 2023 23:07:41 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1696547262;\n\tbh=SLxoCaj8+sjzohhZAKAeitCTwO69mwCyvpBXx/t3mn8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eqVpbGcZYRKBnm1peUNSFdaEbGBkiVzzK9XT0+U9GtZSBzyVT0XT1/fzM3QYQIzD7\n\t vp5vXGgdVO60OwsJDq2ekLnAWy6jMdflpjNR1ZN3hZKSUSyrOJ73vLU/ULSgUW2etD\n\t a6R32TzhXCg0Z6lGQ+QIsQK5IWGpCKZ/cfWr147n68P4T5zOZVA8QQhjLNJkFwM5Jg\n\t sSbEJ+TXGxZSgB87hJpTNNdNav1mL6YthEqe8ZqDpehIRgd8kIwFyavyJt62J1DMH7\n\t Q3k4e3SEynwm59DGiIZoUlRjI4/+Ho2UFrFfg7y/HV5HJt98OtESzPpgP6bHyId4GA\n\t 1i0TZKOZ/Mjew==","Date":"Fri, 6 Oct 2023 01:07:37 +0200","From":"Andi Shyti ","To":"Chris Packham ","Cc":"\"gregory.clement@bootlin.com\" ,\n\t\"robh+dt@kernel.org\" ,\n\t\"krzysztof.kozlowski+dt@linaro.org\" ,\n\t\"conor+dt@kernel.org\" ,\n\t\"pierre.gondois@arm.com\" ,\n\t\"linux-i2c@vger.kernel.org\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ","Subject":"Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery","Message-ID":"<20231005230737.6j57y7d27vnsbws3@zenone.zhora.eu>","References":"<20230926234801.4078042-1-chris.packham@alliedtelesis.co.nz>\n <20230926234801.4078042-4-chris.packham@alliedtelesis.co.nz>\n <20231005215832.p4mxov6occzqmj2k@zenone.zhora.eu>\n <57c27eb5-1145-4a84-a7b6-ff785d7a1eeb@alliedtelesis.co.nz>","Precedence":"bulk","X-Mailing-List":"devicetree@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<57c27eb5-1145-4a84-a7b6-ff785d7a1eeb@alliedtelesis.co.nz>"}}]