* [PATCH 0/3] i2c: mv64xxx: Support for I2C unstuck
@ 2023-09-26 23:47 Chris Packham
2023-09-26 23:47 ` [PATCH 1/3] dt-bindings: i2c: mv64xxx: update bindings for unstuck register Chris Packham
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Chris Packham @ 2023-09-26 23:47 UTC (permalink / raw
To: gregory.clement, andi.shyti, robh+dt, krzysztof.kozlowski+dt,
conor+dt, pierre.gondois
Cc: linux-i2c, devicetree, linux-kernel, Chris Packham
Some newer Marvell SoCs natively support a recovery mechanisim. This can be
used as an alternative to the generic pinctrl/GPIO recovery mechanism used on
the older SoCs.
Chris Packham (3):
dt-bindings: i2c: mv64xxx: update bindings for unstuck register
arm64: dts: marvell: AC5: use I2C unstuck function
i2c: mv64xxx: add support for FSM based recovery
.../bindings/i2c/marvell,mv64xxx-i2c.yaml | 5 +-
arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 14 ++--
drivers/i2c/busses/i2c-mv64xxx.c | 71 +++++++++++++++++--
3 files changed, 75 insertions(+), 15 deletions(-)
--
2.42.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] dt-bindings: i2c: mv64xxx: update bindings for unstuck register
2023-09-26 23:47 [PATCH 0/3] i2c: mv64xxx: Support for I2C unstuck Chris Packham
@ 2023-09-26 23:47 ` Chris Packham
2023-09-27 15:05 ` Conor Dooley
2023-09-26 23:48 ` [PATCH 2/3] arm64: dts: marvell: AC5: use I2C unstuck function Chris Packham
2023-09-26 23:48 ` [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery Chris Packham
2 siblings, 1 reply; 8+ messages in thread
From: Chris Packham @ 2023-09-26 23:47 UTC (permalink / raw
To: gregory.clement, andi.shyti, robh+dt, krzysztof.kozlowski+dt,
conor+dt, pierre.gondois
Cc: linux-i2c, devicetree, linux-kernel, Chris Packham
Some newer Marvell SoCs support an "unstuck" function for I2C bus
recovery. This is an alternative to the generic GPIO based recovery that
the older SoCs use. The unstuck register falls outside of the usual
address block for the I2C controller so requires an additional cell in
the register property. This is optional and does not need to be
supplied.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
.../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
index 984fc1ed3ec6..461d1c9ee3f7 100644
--- a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
@@ -45,7 +45,10 @@ properties:
auto-detects this and sets it appropriately.
reg:
- maxItems: 1
+ minItems: 1
+ items:
+ - description: I2C controller registers
+ - description: I2C unstuck register
interrupts:
maxItems: 1
--
2.42.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] arm64: dts: marvell: AC5: use I2C unstuck function
2023-09-26 23:47 [PATCH 0/3] i2c: mv64xxx: Support for I2C unstuck Chris Packham
2023-09-26 23:47 ` [PATCH 1/3] dt-bindings: i2c: mv64xxx: update bindings for unstuck register Chris Packham
@ 2023-09-26 23:48 ` Chris Packham
2023-09-26 23:48 ` [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery Chris Packham
2 siblings, 0 replies; 8+ messages in thread
From: Chris Packham @ 2023-09-26 23:48 UTC (permalink / raw
To: gregory.clement, andi.shyti, robh+dt, krzysztof.kozlowski+dt,
conor+dt, pierre.gondois
Cc: linux-i2c, devicetree, linux-kernel, Chris Packham
The AC5 SoC supports using a controller based I2C unstuck function for
recovery. Use this instead of the generic GPIO recovery.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
index c9ce1010c415..e52d3c3496d5 100644
--- a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
@@ -137,7 +137,7 @@ mdio: mdio@22004 {
i2c0: i2c@11000{
compatible = "marvell,mv78230-i2c";
- reg = <0x11000 0x20>;
+ reg = <0x11000 0x20>, <0x110a0 0x4>;
#address-cells = <1>;
#size-cells = <0>;
@@ -146,17 +146,14 @@ i2c0: i2c@11000{
interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
clock-frequency=<100000>;
- pinctrl-names = "default", "gpio";
+ pinctrl-names = "default";
pinctrl-0 = <&i2c0_pins>;
- pinctrl-1 = <&i2c0_gpio>;
- scl-gpios = <&gpio0 26 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
- sda-gpios = <&gpio0 27 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
status = "disabled";
};
i2c1: i2c@11100{
compatible = "marvell,mv78230-i2c";
- reg = <0x11100 0x20>;
+ reg = <0x11100 0x20>, <0x110a4 0x4>;
#address-cells = <1>;
#size-cells = <0>;
@@ -165,11 +162,8 @@ i2c1: i2c@11100{
interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
clock-frequency=<100000>;
- pinctrl-names = "default", "gpio";
+ pinctrl-names = "default";
pinctrl-0 = <&i2c1_pins>;
- pinctrl-1 = <&i2c1_gpio>;
- scl-gpios = <&gpio0 20 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
- sda-gpios = <&gpio0 21 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
status = "disabled";
};
--
2.42.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery
2023-09-26 23:47 [PATCH 0/3] i2c: mv64xxx: Support for I2C unstuck Chris Packham
2023-09-26 23:47 ` [PATCH 1/3] dt-bindings: i2c: mv64xxx: update bindings for unstuck register Chris Packham
2023-09-26 23:48 ` [PATCH 2/3] arm64: dts: marvell: AC5: use I2C unstuck function Chris Packham
@ 2023-09-26 23:48 ` Chris Packham
2023-10-05 21:58 ` Andi Shyti
2 siblings, 1 reply; 8+ messages in thread
From: Chris Packham @ 2023-09-26 23:48 UTC (permalink / raw
To: gregory.clement, andi.shyti, robh+dt, krzysztof.kozlowski+dt,
conor+dt, pierre.gondois
Cc: linux-i2c, devicetree, linux-kernel, Chris Packham
Some newer Marvell SoCs (AC5 and CN9130, possibly more) support a I2C
unstuck function. This provides a recovery function as part of the FSM
as an alternative to changing pinctrl modes and using the generic GPIO
based recovery. Allow for using this by adding an optional resource to
the platform data which contains the address of the I2C unstuck register
for the I2C controller.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
drivers/i2c/busses/i2c-mv64xxx.c | 71 ++++++++++++++++++++++++++++++--
1 file changed, 67 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index fd8403b07fa6..4345ab19b89c 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -21,6 +21,7 @@
#include <linux/pm_runtime.h>
#include <linux/reset.h>
#include <linux/io.h>
+#include <linux/iopoll.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
@@ -82,6 +83,13 @@
/* Bridge Status values */
#define MV64XXX_I2C_BRIDGE_STATUS_ERROR BIT(0)
+/* Unstuck Register values */
+#define MV64XXX_I2C_UNSTUCK_TRIGGER BIT(0)
+#define MV64XXX_I2C_UNSTUCK_ON_GOING BIT(1)
+#define MV64XXX_I2C_UNSTUCK_ERROR BIT(2)
+#define MV64XXX_I2C_UNSTUCK_COUNT(val) ((val & 0xf0) >> 4)
+#define MV64XXX_I2C_UNSTUCK_INPROGRESS (MV64XXX_I2C_UNSTUCK_TRIGGER|MV64XXX_I2C_UNSTUCK_ON_GOING)
+
/* Driver states */
enum {
MV64XXX_I2C_STATE_INVALID,
@@ -126,6 +134,7 @@ struct mv64xxx_i2c_data {
u32 aborting;
u32 cntl_bits;
void __iomem *reg_base;
+ void __iomem *unstuck_reg;
struct mv64xxx_i2c_regs reg_offsets;
u32 addr1;
u32 addr2;
@@ -735,6 +744,33 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
return false;
}
+static int
+mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)
+{
+ struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
+ int ret;
+ u32 val;
+
+ dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
+ writel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);
+ ret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,
+ !(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),
+ 1000, 5000);
+ if (ret) {
+ dev_err(&adap->dev, "recovery timeout\n");
+ return ret;
+ }
+
+ if (val & MV64XXX_I2C_UNSTUCK_ERROR) {
+ dev_err(&adap->dev, "recovery failed\n");
+ return -EBUSY;
+ }
+
+ dev_info(&adap->dev, "recovery complete after %d pulses\n", MV64XXX_I2C_UNSTUCK_COUNT(val));
+
+ return 0;
+}
+
/*
*****************************************************************************
*
@@ -914,7 +950,8 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
drv_data->errata_delay = true;
}
- if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) {
+ if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c") ||
+ of_device_is_compatible(np, "marvell,armada-8k-i2c")) {
drv_data->offload_enabled = false;
/* The delay is only needed in standard mode (100kHz) */
if (bus_freq <= I2C_MAX_STANDARD_MODE_FREQ)
@@ -936,8 +973,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
}
#endif /* CONFIG_OF */
-static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,
- struct device *dev)
+static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,
+ struct device *dev)
+{
+ struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
+
+ dev_info(dev, "using FSM for recovery\n");
+ rinfo->recover_bus = mv64xxx_i2c_recover_bus;
+ drv_data->adapter.bus_recovery_info = rinfo;
+
+ return 0;
+
+}
+
+static int mv64xxx_i2c_init_gpio_recovery_info(struct mv64xxx_i2c_data *drv_data,
+ struct device *dev)
{
struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
@@ -986,6 +1036,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
{
struct mv64xxx_i2c_data *drv_data;
struct mv64xxx_i2c_pdata *pdata = dev_get_platdata(&pd->dev);
+ struct resource *res;
int rc;
if ((!pdata && !pd->dev.of_node))
@@ -1000,6 +1051,14 @@ mv64xxx_i2c_probe(struct platform_device *pd)
if (IS_ERR(drv_data->reg_base))
return PTR_ERR(drv_data->reg_base);
+ /* optional unstuck support */
+ res = platform_get_resource(pd, IORESOURCE_MEM, 1);
+ if (res) {
+ drv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);
+ if (IS_ERR(drv_data->unstuck_reg))
+ return PTR_ERR(drv_data->unstuck_reg);
+ }
+
strscpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
sizeof(drv_data->adapter.name));
@@ -1037,7 +1096,11 @@ mv64xxx_i2c_probe(struct platform_device *pd)
return rc;
}
- rc = mv64xxx_i2c_init_recovery_info(drv_data, &pd->dev);
+ if (drv_data->unstuck_reg)
+ rc = mv64xxx_i2c_init_fsm_recovery_info(drv_data, &pd->dev);
+ else
+ rc = mv64xxx_i2c_init_gpio_recovery_info(drv_data, &pd->dev);
+
if (rc == -EPROBE_DEFER)
return rc;
--
2.42.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] dt-bindings: i2c: mv64xxx: update bindings for unstuck register
2023-09-26 23:47 ` [PATCH 1/3] dt-bindings: i2c: mv64xxx: update bindings for unstuck register Chris Packham
@ 2023-09-27 15:05 ` Conor Dooley
0 siblings, 0 replies; 8+ messages in thread
From: Conor Dooley @ 2023-09-27 15:05 UTC (permalink / raw
To: Chris Packham
Cc: gregory.clement, andi.shyti, robh+dt, krzysztof.kozlowski+dt,
conor+dt, pierre.gondois, linux-i2c, devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]
On Wed, Sep 27, 2023 at 12:47:59PM +1300, Chris Packham wrote:
> Some newer Marvell SoCs support an "unstuck" function for I2C bus
> recovery. This is an alternative to the generic GPIO based recovery that
> the older SoCs use. The unstuck register falls outside of the usual
> address block for the I2C controller so requires an additional cell in
> the register property. This is optional and does not need to be
> supplied.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> ---
> .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> index 984fc1ed3ec6..461d1c9ee3f7 100644
> --- a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> @@ -45,7 +45,10 @@ properties:
> auto-detects this and sets it appropriately.
>
> reg:
> - maxItems: 1
> + minItems: 1
> + items:
> + - description: I2C controller registers
> + - description: I2C unstuck register
>
> interrupts:
> maxItems: 1
> --
> 2.42.0
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery
2023-09-26 23:48 ` [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery Chris Packham
@ 2023-10-05 21:58 ` Andi Shyti
2023-10-05 22:39 ` Chris Packham
0 siblings, 1 reply; 8+ messages in thread
From: Andi Shyti @ 2023-10-05 21:58 UTC (permalink / raw
To: Chris Packham
Cc: gregory.clement, robh+dt, krzysztof.kozlowski+dt, conor+dt,
pierre.gondois, linux-i2c, devicetree, linux-kernel
Hi Chris,
Looks good, just a few questions.
> +static int
> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)
> +{
> + struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> + int ret;
> + u32 val;
> +
> + dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
> + writel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);
> + ret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,
> + !(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),
> + 1000, 5000);
here you are busy looping for 1ms between reads which is a long
time. Why not using read_poll_timeout() instead?
> + if (ret) {
> + dev_err(&adap->dev, "recovery timeout\n");
> + return ret;
> + }
> +
> + if (val & MV64XXX_I2C_UNSTUCK_ERROR) {
> + dev_err(&adap->dev, "recovery failed\n");
> + return -EBUSY;
> + }
> +
> + dev_info(&adap->dev, "recovery complete after %d pulses\n", MV64XXX_I2C_UNSTUCK_COUNT(val));
dev_dbg?
> + return 0;
> +}
> +
[...]
> - if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) {
> + if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c") ||
> + of_device_is_compatible(np, "marvell,armada-8k-i2c")) {
should this be part of a different patch?
> drv_data->offload_enabled = false;
> /* The delay is only needed in standard mode (100kHz) */
> if (bus_freq <= I2C_MAX_STANDARD_MODE_FREQ)
> @@ -936,8 +973,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
> }
> #endif /* CONFIG_OF */
>
> -static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,
> - struct device *dev)
> +static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,
> + struct device *dev)
> +{
> + struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
> +
> + dev_info(dev, "using FSM for recovery\n");
dev_dbg?
> + rinfo->recover_bus = mv64xxx_i2c_recover_bus;
> + drv_data->adapter.bus_recovery_info = rinfo;
> +
> + return 0;
> +
> +}
> +
[...]
> + /* optional unstuck support */
> + res = platform_get_resource(pd, IORESOURCE_MEM, 1);
> + if (res) {
> + drv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);
> + if (IS_ERR(drv_data->unstuck_reg))
> + return PTR_ERR(drv_data->unstuck_reg);
OK, we failed to ioremap... but instead of returning an error,
wouldn't it be better to just set unstuck_reg to NULL and move
forward without unstuck support?
Maybe you will stil crash later because something might have
happened, but failing on purpose on an optional feature looks a
bit too drastic to me. What do you think?
Thanks,
Andi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery
2023-10-05 21:58 ` Andi Shyti
@ 2023-10-05 22:39 ` Chris Packham
2023-10-05 23:07 ` Andi Shyti
0 siblings, 1 reply; 8+ messages in thread
From: Chris Packham @ 2023-10-05 22:39 UTC (permalink / raw
To: Andi Shyti
Cc: gregory.clement@bootlin.com, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
pierre.gondois@arm.com, linux-i2c@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On 6/10/23 10:58, Andi Shyti wrote:
> Hi Chris,
>
> Looks good, just a few questions.
>
>> +static int
>> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)
>> +{
>> + struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
>> + int ret;
>> + u32 val;
>> +
>> + dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
>> + writel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);
>> + ret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,
>> + !(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),
>> + 1000, 5000);
> here you are busy looping for 1ms between reads which is a long
> time. Why not using read_poll_timeout() instead?
I needed to use the atomic variant because this ends up getting called
from an interrupt handler (mv64xxx_i2c_intr() -> mv64xxx_i2c_fsm()). I
probably don't need to wait so long between reads those times were just
pulled out of thin air. In my experimentation the faults that can be
cleared do so within a couple of clocks, if it hasn't cleared within 8
clocks it's not going to.
>> + if (ret) {
>> + dev_err(&adap->dev, "recovery timeout\n");
>> + return ret;
>> + }
>> +
>> + if (val & MV64XXX_I2C_UNSTUCK_ERROR) {
>> + dev_err(&adap->dev, "recovery failed\n");
>> + return -EBUSY;
>> + }
>> +
>> + dev_info(&adap->dev, "recovery complete after %d pulses\n", MV64XXX_I2C_UNSTUCK_COUNT(val));
> dev_dbg?
ack.
>> + return 0;
>> +}
>> +
> [...]
>
>> - if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) {
>> + if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c") ||
>> + of_device_is_compatible(np, "marvell,armada-8k-i2c")) {
> should this be part of a different patch?
Yes sorry. Originally I was going to use a new compatible to indicate
the unstuck support but went with the 2nd reg cell so this is unnecessary.
>
>> drv_data->offload_enabled = false;
>> /* The delay is only needed in standard mode (100kHz) */
>> if (bus_freq <= I2C_MAX_STANDARD_MODE_FREQ)
>> @@ -936,8 +973,21 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>> }
>> #endif /* CONFIG_OF */
>>
>> -static int mv64xxx_i2c_init_recovery_info(struct mv64xxx_i2c_data *drv_data,
>> - struct device *dev)
>> +static int mv64xxx_i2c_init_fsm_recovery_info(struct mv64xxx_i2c_data *drv_data,
>> + struct device *dev)
>> +{
>> + struct i2c_bus_recovery_info *rinfo = &drv_data->rinfo;
>> +
>> + dev_info(dev, "using FSM for recovery\n");
> dev_dbg?
>
>> + rinfo->recover_bus = mv64xxx_i2c_recover_bus;
>> + drv_data->adapter.bus_recovery_info = rinfo;
>> +
>> + return 0;
>> +
>> +}
>> +
> [...]
>
>> + /* optional unstuck support */
>> + res = platform_get_resource(pd, IORESOURCE_MEM, 1);
>> + if (res) {
>> + drv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);
>> + if (IS_ERR(drv_data->unstuck_reg))
>> + return PTR_ERR(drv_data->unstuck_reg);
> OK, we failed to ioremap... but instead of returning an error,
> wouldn't it be better to just set unstuck_reg to NULL and move
> forward without unstuck support?
>
> Maybe you will stil crash later because something might have
> happened, but failing on purpose on an optional feature looks a
> bit too drastic to me. What do you think?
Personally I think if the reg property is supplied in the dts we'd
better be able to use it. If the feature is not wanted then the way to
indicate this is by supplying only one reg cell.
I'd be happy with a dev_warn() and unstuck_reg = NULL if that helps get
this landed.
>
> Thanks,
> Andi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery
2023-10-05 22:39 ` Chris Packham
@ 2023-10-05 23:07 ` Andi Shyti
0 siblings, 0 replies; 8+ messages in thread
From: Andi Shyti @ 2023-10-05 23:07 UTC (permalink / raw
To: Chris Packham
Cc: gregory.clement@bootlin.com, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
pierre.gondois@arm.com, linux-i2c@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Hi Chris,
> >> +static int
> >> +mv64xxx_i2c_recover_bus(struct i2c_adapter *adap)
> >> +{
> >> + struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> >> + int ret;
> >> + u32 val;
> >> +
> >> + dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
> >> + writel(MV64XXX_I2C_UNSTUCK_TRIGGER, drv_data->unstuck_reg);
> >> + ret = readl_poll_timeout_atomic(drv_data->unstuck_reg, val,
> >> + !(val & MV64XXX_I2C_UNSTUCK_INPROGRESS),
> >> + 1000, 5000);
> > here you are busy looping for 1ms between reads which is a long
> > time. Why not using read_poll_timeout() instead?
>
> I needed to use the atomic variant because this ends up getting called
> from an interrupt handler (mv64xxx_i2c_intr() -> mv64xxx_i2c_fsm()). I
> probably don't need to wait so long between reads those times were just
> pulled out of thin air. In my experimentation the faults that can be
> cleared do so within a couple of clocks, if it hasn't cleared within 8
> clocks it's not going to.
It's still a long time to wait in atomic context...
readl_poll_timeout_atomic() waits in udelays, where the maximum
accepted waiting time is 10us. Here you are waiting 100 times
more.
If we can't be within that value I would rather use a thread.
Or, you could also consider using threaded_irq()... but this
might have a bit of a higher impact.
[...]
> >> + /* optional unstuck support */
> >> + res = platform_get_resource(pd, IORESOURCE_MEM, 1);
> >> + if (res) {
> >> + drv_data->unstuck_reg = devm_ioremap_resource(&pd->dev, res);
> >> + if (IS_ERR(drv_data->unstuck_reg))
> >> + return PTR_ERR(drv_data->unstuck_reg);
> > OK, we failed to ioremap... but instead of returning an error,
> > wouldn't it be better to just set unstuck_reg to NULL and move
> > forward without unstuck support?
> >
> > Maybe you will stil crash later because something might have
> > happened, but failing on purpose on an optional feature looks a
> > bit too drastic to me. What do you think?
>
> Personally I think if the reg property is supplied in the dts we'd
> better be able to use it. If the feature is not wanted then the way to
> indicate this is by supplying only one reg cell.
>
> I'd be happy with a dev_warn() and unstuck_reg = NULL if that helps get
> this landed.
Don't ahve a strong opinion... as you like. Mine is just an
opinion and your argument is valid :-)
Andi
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-10-05 23:10 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-26 23:47 [PATCH 0/3] i2c: mv64xxx: Support for I2C unstuck Chris Packham
2023-09-26 23:47 ` [PATCH 1/3] dt-bindings: i2c: mv64xxx: update bindings for unstuck register Chris Packham
2023-09-27 15:05 ` Conor Dooley
2023-09-26 23:48 ` [PATCH 2/3] arm64: dts: marvell: AC5: use I2C unstuck function Chris Packham
2023-09-26 23:48 ` [PATCH 3/3] i2c: mv64xxx: add support for FSM based recovery Chris Packham
2023-10-05 21:58 ` Andi Shyti
2023-10-05 22:39 ` Chris Packham
2023-10-05 23:07 ` Andi Shyti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).