* [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
@ 2020-08-20 10:02 ` Jaakko Laine
0 siblings, 0 replies; 20+ messages in thread
From: Jaakko Laine @ 2020-08-20 10:02 UTC (permalink / raw)
To: wsa
Cc: Jaakko Laine, michal.simek, shubhrajyoti.datta, linux-arm-kernel,
linux-i2c
I2C master operating in multimaster mode can get stuck
indefinitely if I2C start is detected on bus, but no master
has a transaction going.
This is a weakness in I2C standard, which defines no way
to recover, since all masters are indefinitely disallowed
from interrupting the currently operating master. A start
condition can be created for example by an electromagnetic
discharge applied near physical I2C lines. Or a already
operating master could get reset immediately after sending
a start.
If it is known during device tree creation that only a single
I2C master will be present on the bus, this deadlock of the
I2C bus could be avoided in the driver by ignoring the
bus_is_busy register of the xiic, since bus can never be
reserved by any other master.
This patch adds this support for detecting single-master flag
in device tree and when provided, improves I2C reliability by
ignoring the therefore unnecessary xiic bus_is_busy register.
Error can be reproduced by pulling I2C SDA -line temporarily low
by shorting it to ground, while linux I2C master is operating on
it using the xiic driver. The application using the bus will
start receiving linux error code 16: "Device or resource busy"
indefinitely:
kernel: pca953x 0-0020: failed writing register
app: Error writing file, error: 16
With multi-master disabled device will instead receive error
code 5: "I/O error" while SDA is grounded, but recover normal
operation once short is removed.
kernel: pca953x 0-0020: failed reading register
app: Error reading file, error: 5
Signed-off-by: Jaakko Laine <ext-jaakko.laine@vaisala.com>
---
drivers/i2c/busses/i2c-xiic.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 10380531d45c..5d06e6cc5d5c 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -58,6 +58,7 @@ enum xiic_endian {
* @rx_msg: Current RX message
* @rx_pos: Position within current RX message
* @endianness: big/little-endian byte order
+ * @singlemaster: Indicates bus is single master
* @clk: Pointer to AXI4-lite input clock
*/
struct xiic_i2c {
@@ -73,6 +74,7 @@ struct xiic_i2c {
struct i2c_msg *rx_msg;
int rx_pos;
enum xiic_endian endianness;
+ bool singlemaster;
struct clk *clk;
};
@@ -521,19 +523,26 @@ static int xiic_bus_busy(struct xiic_i2c *i2c)
static int xiic_busy(struct xiic_i2c *i2c)
{
int tries = 3;
- int err;
+ int err = 0;
if (i2c->tx_msg)
return -EBUSY;
- /* for instance if previous transfer was terminated due to TX error
- * it might be that the bus is on it's way to become available
- * give it at most 3 ms to wake
+ /* In single master mode bus can only be busy, when in use by this
+ * driver. If the register indicates bus being busy for some reason we
+ * should ignore it, since bus will never be released and i2c will be
+ * stuck forever.
*/
- err = xiic_bus_busy(i2c);
- while (err && tries--) {
- msleep(1);
+ if (!i2c->singlemaster) {
+ /* for instance if previous transfer was terminated due to TX
+ * error it might be that the bus is on it's way to become
+ * available give it at most 3 ms to wake
+ */
err = xiic_bus_busy(i2c);
+ while (err && tries--) {
+ msleep(1);
+ err = xiic_bus_busy(i2c);
+ }
}
return err;
@@ -811,6 +820,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
goto err_clk_dis;
}
+ i2c->singlemaster =
+ of_property_read_bool(pdev->dev.of_node, "single-master");
+
/*
* Detect endianness
* Try to reset the TX FIFO. Then check the EMPTY flag. If it is not
--
2.19.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
2020-08-20 10:02 ` Jaakko Laine
@ 2020-08-25 13:24 ` Wolfram Sang
-1 siblings, 0 replies; 20+ messages in thread
From: Wolfram Sang @ 2020-08-25 13:24 UTC (permalink / raw)
To: Jaakko Laine
Cc: shubhrajyoti.datta, linux-i2c, michal.simek, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]
On Thu, Aug 20, 2020 at 01:02:41PM +0300, Jaakko Laine wrote:
> I2C master operating in multimaster mode can get stuck
> indefinitely if I2C start is detected on bus, but no master
> has a transaction going.
>
> This is a weakness in I2C standard, which defines no way
> to recover, since all masters are indefinitely disallowed
> from interrupting the currently operating master. A start
> condition can be created for example by an electromagnetic
> discharge applied near physical I2C lines. Or a already
> operating master could get reset immediately after sending
> a start.
>
> If it is known during device tree creation that only a single
> I2C master will be present on the bus, this deadlock of the
> I2C bus could be avoided in the driver by ignoring the
> bus_is_busy register of the xiic, since bus can never be
> reserved by any other master.
You could even initiate a recovery procedure if it is a device pulling
SDA low.
> This patch adds this support for detecting single-master flag
> in device tree and when provided, improves I2C reliability by
> ignoring the therefore unnecessary xiic bus_is_busy register.
>
> Error can be reproduced by pulling I2C SDA -line temporarily low
> by shorting it to ground, while linux I2C master is operating on
> it using the xiic driver. The application using the bus will
> start receiving linux error code 16: "Device or resource busy"
> indefinitely:
>
> kernel: pca953x 0-0020: failed writing register
> app: Error writing file, error: 16
>
> With multi-master disabled device will instead receive error
> code 5: "I/O error" while SDA is grounded, but recover normal
> operation once short is removed.
>
> kernel: pca953x 0-0020: failed reading register
> app: Error reading file, error: 5
>
> Signed-off-by: Jaakko Laine <ext-jaakko.laine@vaisala.com>
Again, looks good to me and matches what we discussed and prepared.
Let's see what Michal et al. think.
Happy hacking!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
@ 2020-08-25 13:24 ` Wolfram Sang
0 siblings, 0 replies; 20+ messages in thread
From: Wolfram Sang @ 2020-08-25 13:24 UTC (permalink / raw)
To: Jaakko Laine
Cc: michal.simek, shubhrajyoti.datta, linux-arm-kernel, linux-i2c
[-- Attachment #1.1: Type: text/plain, Size: 1992 bytes --]
On Thu, Aug 20, 2020 at 01:02:41PM +0300, Jaakko Laine wrote:
> I2C master operating in multimaster mode can get stuck
> indefinitely if I2C start is detected on bus, but no master
> has a transaction going.
>
> This is a weakness in I2C standard, which defines no way
> to recover, since all masters are indefinitely disallowed
> from interrupting the currently operating master. A start
> condition can be created for example by an electromagnetic
> discharge applied near physical I2C lines. Or a already
> operating master could get reset immediately after sending
> a start.
>
> If it is known during device tree creation that only a single
> I2C master will be present on the bus, this deadlock of the
> I2C bus could be avoided in the driver by ignoring the
> bus_is_busy register of the xiic, since bus can never be
> reserved by any other master.
You could even initiate a recovery procedure if it is a device pulling
SDA low.
> This patch adds this support for detecting single-master flag
> in device tree and when provided, improves I2C reliability by
> ignoring the therefore unnecessary xiic bus_is_busy register.
>
> Error can be reproduced by pulling I2C SDA -line temporarily low
> by shorting it to ground, while linux I2C master is operating on
> it using the xiic driver. The application using the bus will
> start receiving linux error code 16: "Device or resource busy"
> indefinitely:
>
> kernel: pca953x 0-0020: failed writing register
> app: Error writing file, error: 16
>
> With multi-master disabled device will instead receive error
> code 5: "I/O error" while SDA is grounded, but recover normal
> operation once short is removed.
>
> kernel: pca953x 0-0020: failed reading register
> app: Error reading file, error: 5
>
> Signed-off-by: Jaakko Laine <ext-jaakko.laine@vaisala.com>
Again, looks good to me and matches what we discussed and prepared.
Let's see what Michal et al. think.
Happy hacking!
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
2020-08-25 13:24 ` Wolfram Sang
@ 2020-08-26 11:57 ` Laine Jaakko EXT
-1 siblings, 0 replies; 20+ messages in thread
From: Laine Jaakko EXT @ 2020-08-26 11:57 UTC (permalink / raw)
To: Wolfram Sang
Cc: shubhrajyoti.datta@gmail.com, linux-i2c@vger.kernel.org,
michal.simek@xilinx.com, linux-arm-kernel@lists.infradead.org
> > If it is known during device tree creation that only a single
> > I2C master will be present on the bus, this deadlock of the
> > I2C bus could be avoided in the driver by ignoring the
> > bus_is_busy register of the xiic, since bus can never be
> > reserved by any other master.
> You could even initiate a recovery procedure if it is a device pulling
> SDA low.
In the case we actually observed xiic got permanently stuck, when I2C bus
was temporarily and indirectly affected by external voltage pulse. This can
happen in single-master or multi-master bus. It does seem that no extra
recovery behavior is needed in this case. If we are able to determine that bus
is not actually busy, we can start using it. Indicating that bus is single-master
was a handy way to determine that. This issue was bad for us because
even if SDA was not grounded anymore, bus was still indicated to be busy by
FPGA register and driver would not continue.
In multi-master case you would need some kind of timeout after which bus
bus_is_busy is ignored and recovery attempted. This is ugly since it would be
a non-standard behavior and intrusive to other masters on bus.
In single-master case, if some slave device on bus would spontaneously
pull SDA to ground when clock line is not pulsed, bus_is_busy could be triggered.
In this case we could attempt some kind of recovery behavior. I guess this often
means attempting to pulse the clock line to get the slave to release SDA.
In my knowledge pulsing the clock line can help if slave device on bus has missed
some clock signal edges (or is answering with more bits than expected for some reason)
and is holding SDA down in an attempt to communicate a data bit to i2c -master.
Extra pulses in clock line can then allow the slave to finish transmission and stop
pulling SDA low. I however doubt that this type of recovery would be likely to help
if I2C -slave spontaneously pulls SDA low. This would however be a very badly
misbehaving slave -device, so it´s hard to speculate what it will do and what specific
recovery might help.
There is too much speculation for me to attempt or test bus recovery in this case,
so I would leave it out of this change. If somebody notices and is able to test a case,
where some specific extra recovery would be helpful, I would suggest to considered it
later.
-Jaakko
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
@ 2020-08-26 11:57 ` Laine Jaakko EXT
0 siblings, 0 replies; 20+ messages in thread
From: Laine Jaakko EXT @ 2020-08-26 11:57 UTC (permalink / raw)
To: Wolfram Sang
Cc: michal.simek@xilinx.com, shubhrajyoti.datta@gmail.com,
linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org
> > If it is known during device tree creation that only a single
> > I2C master will be present on the bus, this deadlock of the
> > I2C bus could be avoided in the driver by ignoring the
> > bus_is_busy register of the xiic, since bus can never be
> > reserved by any other master.
> You could even initiate a recovery procedure if it is a device pulling
> SDA low.
In the case we actually observed xiic got permanently stuck, when I2C bus
was temporarily and indirectly affected by external voltage pulse. This can
happen in single-master or multi-master bus. It does seem that no extra
recovery behavior is needed in this case. If we are able to determine that bus
is not actually busy, we can start using it. Indicating that bus is single-master
was a handy way to determine that. This issue was bad for us because
even if SDA was not grounded anymore, bus was still indicated to be busy by
FPGA register and driver would not continue.
In multi-master case you would need some kind of timeout after which bus
bus_is_busy is ignored and recovery attempted. This is ugly since it would be
a non-standard behavior and intrusive to other masters on bus.
In single-master case, if some slave device on bus would spontaneously
pull SDA to ground when clock line is not pulsed, bus_is_busy could be triggered.
In this case we could attempt some kind of recovery behavior. I guess this often
means attempting to pulse the clock line to get the slave to release SDA.
In my knowledge pulsing the clock line can help if slave device on bus has missed
some clock signal edges (or is answering with more bits than expected for some reason)
and is holding SDA down in an attempt to communicate a data bit to i2c -master.
Extra pulses in clock line can then allow the slave to finish transmission and stop
pulling SDA low. I however doubt that this type of recovery would be likely to help
if I2C -slave spontaneously pulls SDA low. This would however be a very badly
misbehaving slave -device, so it´s hard to speculate what it will do and what specific
recovery might help.
There is too much speculation for me to attempt or test bus recovery in this case,
so I would leave it out of this change. If somebody notices and is able to test a case,
where some specific extra recovery would be helpful, I would suggest to considered it
later.
-Jaakko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
2020-08-26 11:57 ` Laine Jaakko EXT
@ 2020-08-26 12:05 ` Wolfram Sang
-1 siblings, 0 replies; 20+ messages in thread
From: Wolfram Sang @ 2020-08-26 12:05 UTC (permalink / raw)
To: Laine Jaakko EXT
Cc: shubhrajyoti.datta@gmail.com, linux-i2c@vger.kernel.org,
michal.simek@xilinx.com, linux-arm-kernel@lists.infradead.org
[-- Attachment #1: Type: text/plain, Size: 2848 bytes --]
Hi Jaako,
> > You could even initiate a recovery procedure if it is a device pulling
> > SDA low.
>
> In the case we actually observed xiic got permanently stuck, when I2C bus
> was temporarily and indirectly affected by external voltage pulse. This can
> happen in single-master or multi-master bus. It does seem that no extra
> recovery behavior is needed in this case. If we are able to determine that bus
> is not actually busy, we can start using it. Indicating that bus is single-master
> was a handy way to determine that. This issue was bad for us because
> even if SDA was not grounded anymore, bus was still indicated to be busy by
> FPGA register and driver would not continue.
Okay, it was just a suggestion, definately not something I will require
to be added in this patch. Your reasoning makes sense to me.
> In multi-master case you would need some kind of timeout after which bus
> bus_is_busy is ignored and recovery attempted. This is ugly since it would be
> a non-standard behavior and intrusive to other masters on bus.
Yes, this is an issue. My best bet is to use the adapter->timeout value
for that because it is configurable and already used as kind-of I2C
timeout otherwise as well.
> In single-master case, if some slave device on bus would spontaneously
> pull SDA to ground when clock line is not pulsed, bus_is_busy could be triggered.
> In this case we could attempt some kind of recovery behavior. I guess this often
> means attempting to pulse the clock line to get the slave to release SDA.
Yes, as defined in the I2C specification even.
> In my knowledge pulsing the clock line can help if slave device on bus has missed
> some clock signal edges (or is answering with more bits than expected for some reason)
> and is holding SDA down in an attempt to communicate a data bit to i2c -master.
> Extra pulses in clock line can then allow the slave to finish transmission and stop
> pulling SDA low. I however doubt that this type of recovery would be likely to help
> if I2C -slave spontaneously pulls SDA low. This would however be a very badly
> misbehaving slave -device, so it´s hard to speculate what it will do and what specific
> recovery might help.
As the above standard says, you can try sending pulses but if that does
not help, then you should reset the device. In my experience, most I2C
devices are not wired to be reset externally, so we have the pulse
toggling mechanism in the I2C core to try as much as we can.
> There is too much speculation for me to attempt or test bus recovery in this case,
> so I would leave it out of this change. If somebody notices and is able to test a case,
> where some specific extra recovery would be helpful, I would suggest to considered it
> later.
Totally fine with me.
Thanks,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
@ 2020-08-26 12:05 ` Wolfram Sang
0 siblings, 0 replies; 20+ messages in thread
From: Wolfram Sang @ 2020-08-26 12:05 UTC (permalink / raw)
To: Laine Jaakko EXT
Cc: michal.simek@xilinx.com, shubhrajyoti.datta@gmail.com,
linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org
[-- Attachment #1.1: Type: text/plain, Size: 2848 bytes --]
Hi Jaako,
> > You could even initiate a recovery procedure if it is a device pulling
> > SDA low.
>
> In the case we actually observed xiic got permanently stuck, when I2C bus
> was temporarily and indirectly affected by external voltage pulse. This can
> happen in single-master or multi-master bus. It does seem that no extra
> recovery behavior is needed in this case. If we are able to determine that bus
> is not actually busy, we can start using it. Indicating that bus is single-master
> was a handy way to determine that. This issue was bad for us because
> even if SDA was not grounded anymore, bus was still indicated to be busy by
> FPGA register and driver would not continue.
Okay, it was just a suggestion, definately not something I will require
to be added in this patch. Your reasoning makes sense to me.
> In multi-master case you would need some kind of timeout after which bus
> bus_is_busy is ignored and recovery attempted. This is ugly since it would be
> a non-standard behavior and intrusive to other masters on bus.
Yes, this is an issue. My best bet is to use the adapter->timeout value
for that because it is configurable and already used as kind-of I2C
timeout otherwise as well.
> In single-master case, if some slave device on bus would spontaneously
> pull SDA to ground when clock line is not pulsed, bus_is_busy could be triggered.
> In this case we could attempt some kind of recovery behavior. I guess this often
> means attempting to pulse the clock line to get the slave to release SDA.
Yes, as defined in the I2C specification even.
> In my knowledge pulsing the clock line can help if slave device on bus has missed
> some clock signal edges (or is answering with more bits than expected for some reason)
> and is holding SDA down in an attempt to communicate a data bit to i2c -master.
> Extra pulses in clock line can then allow the slave to finish transmission and stop
> pulling SDA low. I however doubt that this type of recovery would be likely to help
> if I2C -slave spontaneously pulls SDA low. This would however be a very badly
> misbehaving slave -device, so it´s hard to speculate what it will do and what specific
> recovery might help.
As the above standard says, you can try sending pulses but if that does
not help, then you should reset the device. In my experience, most I2C
devices are not wired to be reset externally, so we have the pulse
toggling mechanism in the I2C core to try as much as we can.
> There is too much speculation for me to attempt or test bus recovery in this case,
> so I would leave it out of this change. If somebody notices and is able to test a case,
> where some specific extra recovery would be helpful, I would suggest to considered it
> later.
Totally fine with me.
Thanks,
Wolfram
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
2020-08-20 10:02 ` Jaakko Laine
@ 2020-08-26 11:55 ` Michal Simek
-1 siblings, 0 replies; 20+ messages in thread
From: Michal Simek @ 2020-08-26 11:55 UTC (permalink / raw)
To: Jaakko Laine, wsa
Cc: shubhrajyoti.datta, linux-i2c, michal.simek, linux-arm-kernel
On 20. 08. 20 12:02, Jaakko Laine wrote:
> I2C master operating in multimaster mode can get stuck
> indefinitely if I2C start is detected on bus, but no master
> has a transaction going.
>
> This is a weakness in I2C standard, which defines no way
> to recover, since all masters are indefinitely disallowed
> from interrupting the currently operating master. A start
> condition can be created for example by an electromagnetic
> discharge applied near physical I2C lines. Or a already
> operating master could get reset immediately after sending
> a start.
>
> If it is known during device tree creation that only a single
> I2C master will be present on the bus, this deadlock of the
> I2C bus could be avoided in the driver by ignoring the
> bus_is_busy register of the xiic, since bus can never be
> reserved by any other master.
>
> This patch adds this support for detecting single-master flag
> in device tree and when provided, improves I2C reliability by
> ignoring the therefore unnecessary xiic bus_is_busy register.
>
> Error can be reproduced by pulling I2C SDA -line temporarily low
> by shorting it to ground, while linux I2C master is operating on
> it using the xiic driver. The application using the bus will
> start receiving linux error code 16: "Device or resource busy"
> indefinitely:
>
> kernel: pca953x 0-0020: failed writing register
> app: Error writing file, error: 16
>
> With multi-master disabled device will instead receive error
> code 5: "I/O error" while SDA is grounded, but recover normal
> operation once short is removed.
>
> kernel: pca953x 0-0020: failed reading register
> app: Error reading file, error: 5
>
> Signed-off-by: Jaakko Laine <ext-jaakko.laine@vaisala.com>
> ---
> drivers/i2c/busses/i2c-xiic.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 10380531d45c..5d06e6cc5d5c 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -58,6 +58,7 @@ enum xiic_endian {
> * @rx_msg: Current RX message
> * @rx_pos: Position within current RX message
> * @endianness: big/little-endian byte order
> + * @singlemaster: Indicates bus is single master
> * @clk: Pointer to AXI4-lite input clock
> */
> struct xiic_i2c {
> @@ -73,6 +74,7 @@ struct xiic_i2c {
> struct i2c_msg *rx_msg;
> int rx_pos;
> enum xiic_endian endianness;
> + bool singlemaster;
I would understand if this is placed above rx_msg to fill that 4bytes
hole in the structure. But this location doesn't make any sense.
The best would be to move state to the end and add this bool behind it.
To have nicely packed structure like this
on 64bit system.
struct xiic_i2c {
struct device * dev; /* 0 8 */
void * base; /* 8 8 */
wait_queue_head_t wait; /* 16 24 */
struct i2c_adapter adap; /* 40 1032 */
/* --- cacheline 16 boundary (1024 bytes) was 48 bytes ago --- */
struct i2c_msg * tx_msg; /* 1072 8 */
struct mutex lock; /* 1080 32 */
/* --- cacheline 17 boundary (1088 bytes) was 24 bytes ago --- */
unsigned int tx_pos; /* 1112 4 */
unsigned int nmsgs; /* 1116 4 */
struct i2c_msg * rx_msg; /* 1120 8 */
int rx_pos; /* 1128 4 */
enum xiic_endian endianness; /* 1132 4 */
struct clk * clk; /* 1136 8 */
enum xilinx_i2c_state state; /* 1144 4 */
bool singlemaster; /* 1148 1 */
/* size: 1152, cachelines: 18, members: 14 */
/* padding: 3 */
};
on 32bit system.
struct xiic_i2c {
struct device * dev; /* 0 4 */
void * base; /* 4 4 */
wait_queue_head_t wait; /* 8 24 */
struct i2c_adapter adap; /* 32 508 */
/* --- cacheline 8 boundary (512 bytes) was 28 bytes ago --- */
struct i2c_msg * tx_msg; /* 540 4 */
struct mutex lock; /* 544 28 */
unsigned int tx_pos; /* 572 4 */
/* --- cacheline 9 boundary (576 bytes) --- */
unsigned int nmsgs; /* 576 4 */
struct i2c_msg * rx_msg; /* 580 4 */
int rx_pos; /* 584 4 */
enum xiic_endian endianness; /* 588 4 */
struct clk * clk; /* 592 4 */
enum xilinx_i2c_state state; /* 596 4 */
bool singlemaster; /* 600 1 */
/* size: 604, cachelines: 10, members: 14 */
/* padding: 3 */
/* last cacheline: 28 bytes */
};
> struct clk *clk;
> };
>
> @@ -521,19 +523,26 @@ static int xiic_bus_busy(struct xiic_i2c *i2c)
> static int xiic_busy(struct xiic_i2c *i2c)
> {
> int tries = 3;
> - int err;
> + int err = 0;
>
> if (i2c->tx_msg)
> return -EBUSY;
>
> - /* for instance if previous transfer was terminated due to TX error
> - * it might be that the bus is on it's way to become available
> - * give it at most 3 ms to wake
> + /* In single master mode bus can only be busy, when in use by this
> + * driver. If the register indicates bus being busy for some reason we
> + * should ignore it, since bus will never be released and i2c will be
> + * stuck forever.
> */
> - err = xiic_bus_busy(i2c);
> - while (err && tries--) {
> - msleep(1);
> + if (!i2c->singlemaster) {
> + /* for instance if previous transfer was terminated due to TX
> + * error it might be that the bus is on it's way to become
> + * available give it at most 3 ms to wake
> + */
> err = xiic_bus_busy(i2c);
> + while (err && tries--) {
> + msleep(1);
> + err = xiic_bus_busy(i2c);
> + }
> }
I would prefer to write this differently.
if (i2c->singlemaster)
return 0;
Followed by origin code. Patch will be smaller and you don't need to add
one more level of indentation.
>
> return err;
> @@ -811,6 +820,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> goto err_clk_dis;
> }
>
> + i2c->singlemaster =
> + of_property_read_bool(pdev->dev.of_node, "single-master");
> +
> /*
> * Detect endianness
> * Try to reset the TX FIFO. Then check the EMPTY flag. If it is not
>
Thanks,
Michal
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
@ 2020-08-26 11:55 ` Michal Simek
0 siblings, 0 replies; 20+ messages in thread
From: Michal Simek @ 2020-08-26 11:55 UTC (permalink / raw)
To: Jaakko Laine, wsa
Cc: michal.simek, shubhrajyoti.datta, linux-arm-kernel, linux-i2c
On 20. 08. 20 12:02, Jaakko Laine wrote:
> I2C master operating in multimaster mode can get stuck
> indefinitely if I2C start is detected on bus, but no master
> has a transaction going.
>
> This is a weakness in I2C standard, which defines no way
> to recover, since all masters are indefinitely disallowed
> from interrupting the currently operating master. A start
> condition can be created for example by an electromagnetic
> discharge applied near physical I2C lines. Or a already
> operating master could get reset immediately after sending
> a start.
>
> If it is known during device tree creation that only a single
> I2C master will be present on the bus, this deadlock of the
> I2C bus could be avoided in the driver by ignoring the
> bus_is_busy register of the xiic, since bus can never be
> reserved by any other master.
>
> This patch adds this support for detecting single-master flag
> in device tree and when provided, improves I2C reliability by
> ignoring the therefore unnecessary xiic bus_is_busy register.
>
> Error can be reproduced by pulling I2C SDA -line temporarily low
> by shorting it to ground, while linux I2C master is operating on
> it using the xiic driver. The application using the bus will
> start receiving linux error code 16: "Device or resource busy"
> indefinitely:
>
> kernel: pca953x 0-0020: failed writing register
> app: Error writing file, error: 16
>
> With multi-master disabled device will instead receive error
> code 5: "I/O error" while SDA is grounded, but recover normal
> operation once short is removed.
>
> kernel: pca953x 0-0020: failed reading register
> app: Error reading file, error: 5
>
> Signed-off-by: Jaakko Laine <ext-jaakko.laine@vaisala.com>
> ---
> drivers/i2c/busses/i2c-xiic.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 10380531d45c..5d06e6cc5d5c 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -58,6 +58,7 @@ enum xiic_endian {
> * @rx_msg: Current RX message
> * @rx_pos: Position within current RX message
> * @endianness: big/little-endian byte order
> + * @singlemaster: Indicates bus is single master
> * @clk: Pointer to AXI4-lite input clock
> */
> struct xiic_i2c {
> @@ -73,6 +74,7 @@ struct xiic_i2c {
> struct i2c_msg *rx_msg;
> int rx_pos;
> enum xiic_endian endianness;
> + bool singlemaster;
I would understand if this is placed above rx_msg to fill that 4bytes
hole in the structure. But this location doesn't make any sense.
The best would be to move state to the end and add this bool behind it.
To have nicely packed structure like this
on 64bit system.
struct xiic_i2c {
struct device * dev; /* 0 8 */
void * base; /* 8 8 */
wait_queue_head_t wait; /* 16 24 */
struct i2c_adapter adap; /* 40 1032 */
/* --- cacheline 16 boundary (1024 bytes) was 48 bytes ago --- */
struct i2c_msg * tx_msg; /* 1072 8 */
struct mutex lock; /* 1080 32 */
/* --- cacheline 17 boundary (1088 bytes) was 24 bytes ago --- */
unsigned int tx_pos; /* 1112 4 */
unsigned int nmsgs; /* 1116 4 */
struct i2c_msg * rx_msg; /* 1120 8 */
int rx_pos; /* 1128 4 */
enum xiic_endian endianness; /* 1132 4 */
struct clk * clk; /* 1136 8 */
enum xilinx_i2c_state state; /* 1144 4 */
bool singlemaster; /* 1148 1 */
/* size: 1152, cachelines: 18, members: 14 */
/* padding: 3 */
};
on 32bit system.
struct xiic_i2c {
struct device * dev; /* 0 4 */
void * base; /* 4 4 */
wait_queue_head_t wait; /* 8 24 */
struct i2c_adapter adap; /* 32 508 */
/* --- cacheline 8 boundary (512 bytes) was 28 bytes ago --- */
struct i2c_msg * tx_msg; /* 540 4 */
struct mutex lock; /* 544 28 */
unsigned int tx_pos; /* 572 4 */
/* --- cacheline 9 boundary (576 bytes) --- */
unsigned int nmsgs; /* 576 4 */
struct i2c_msg * rx_msg; /* 580 4 */
int rx_pos; /* 584 4 */
enum xiic_endian endianness; /* 588 4 */
struct clk * clk; /* 592 4 */
enum xilinx_i2c_state state; /* 596 4 */
bool singlemaster; /* 600 1 */
/* size: 604, cachelines: 10, members: 14 */
/* padding: 3 */
/* last cacheline: 28 bytes */
};
> struct clk *clk;
> };
>
> @@ -521,19 +523,26 @@ static int xiic_bus_busy(struct xiic_i2c *i2c)
> static int xiic_busy(struct xiic_i2c *i2c)
> {
> int tries = 3;
> - int err;
> + int err = 0;
>
> if (i2c->tx_msg)
> return -EBUSY;
>
> - /* for instance if previous transfer was terminated due to TX error
> - * it might be that the bus is on it's way to become available
> - * give it at most 3 ms to wake
> + /* In single master mode bus can only be busy, when in use by this
> + * driver. If the register indicates bus being busy for some reason we
> + * should ignore it, since bus will never be released and i2c will be
> + * stuck forever.
> */
> - err = xiic_bus_busy(i2c);
> - while (err && tries--) {
> - msleep(1);
> + if (!i2c->singlemaster) {
> + /* for instance if previous transfer was terminated due to TX
> + * error it might be that the bus is on it's way to become
> + * available give it at most 3 ms to wake
> + */
> err = xiic_bus_busy(i2c);
> + while (err && tries--) {
> + msleep(1);
> + err = xiic_bus_busy(i2c);
> + }
> }
I would prefer to write this differently.
if (i2c->singlemaster)
return 0;
Followed by origin code. Patch will be smaller and you don't need to add
one more level of indentation.
>
> return err;
> @@ -811,6 +820,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> goto err_clk_dis;
> }
>
> + i2c->singlemaster =
> + of_property_read_bool(pdev->dev.of_node, "single-master");
> +
> /*
> * Detect endianness
> * Try to reset the TX FIFO. Then check the EMPTY flag. If it is not
>
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
2020-08-26 11:55 ` Michal Simek
@ 2020-08-26 13:03 ` Laine Jaakko EXT
-1 siblings, 0 replies; 20+ messages in thread
From: Laine Jaakko EXT @ 2020-08-26 13:03 UTC (permalink / raw)
To: Michal Simek, wsa@the-dreams.de
Cc: shubhrajyoti.datta@gmail.com, linux-i2c@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
> > diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> > index 10380531d45c..5d06e6cc5d5c 100644
> > --- a/drivers/i2c/busses/i2c-xiic.c
> > +++ b/drivers/i2c/busses/i2c-xiic.c
> > @@ -58,6 +58,7 @@ enum xiic_endian {
> > * @rx_msg: Current RX message
> > * @rx_pos: Position within current RX message
> > * @endianness: big/little-endian byte order
> > + * @singlemaster: Indicates bus is single master
> > * @clk: Pointer to AXI4-lite input clock
> > */
> > struct xiic_i2c {
> > @@ -73,6 +74,7 @@ struct xiic_i2c {
> > struct i2c_msg *rx_msg;
> > int rx_pos;
> > enum xiic_endian endianness;
> > + bool singlemaster;
>
> I would understand if this is placed above rx_msg to fill that 4bytes
> hole in the structure. But this location doesn't make any sense.
Thanks for pointing out the struct alignment issue. I agree.
> The best would be to move state to the end and add this bool behind it.
> To have nicely packed structure like this
I will move state to the end in separate commit and add single-master after that in V3
as suggested.
> > - err = xiic_bus_busy(i2c);
> > - while (err && tries--) {
> > - msleep(1);
> > + if (!i2c->singlemaster) {
> > + /* for instance if previous transfer was terminated due to TX
> > + * error it might be that the bus is on it's way to become
> > + * available give it at most 3 ms to wake
> > + */
> > err = xiic_bus_busy(i2c);
> > + while (err && tries--) {
> > + msleep(1);
> > + err = xiic_bus_busy(i2c);
> > + }
> > }
>
> I would prefer to write this differently.
> if (i2c->singlemaster)
> return 0;
>
> Followed by origin code. Patch will be smaller and you don't need to add
> one more level of indentation.
That sounds better and easier than current version, thanks.
I will make the change in V3.
-Jaakko
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [PATCH v2 2/2] i2c: xiic: Support forcing single-master in DT
@ 2020-08-26 13:03 ` Laine Jaakko EXT
0 siblings, 0 replies; 20+ messages in thread
From: Laine Jaakko EXT @ 2020-08-26 13:03 UTC (permalink / raw)
To: Michal Simek, wsa@the-dreams.de
Cc: shubhrajyoti.datta@gmail.com,
linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org
> > diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> > index 10380531d45c..5d06e6cc5d5c 100644
> > --- a/drivers/i2c/busses/i2c-xiic.c
> > +++ b/drivers/i2c/busses/i2c-xiic.c
> > @@ -58,6 +58,7 @@ enum xiic_endian {
> > * @rx_msg: Current RX message
> > * @rx_pos: Position within current RX message
> > * @endianness: big/little-endian byte order
> > + * @singlemaster: Indicates bus is single master
> > * @clk: Pointer to AXI4-lite input clock
> > */
> > struct xiic_i2c {
> > @@ -73,6 +74,7 @@ struct xiic_i2c {
> > struct i2c_msg *rx_msg;
> > int rx_pos;
> > enum xiic_endian endianness;
> > + bool singlemaster;
>
> I would understand if this is placed above rx_msg to fill that 4bytes
> hole in the structure. But this location doesn't make any sense.
Thanks for pointing out the struct alignment issue. I agree.
> The best would be to move state to the end and add this bool behind it.
> To have nicely packed structure like this
I will move state to the end in separate commit and add single-master after that in V3
as suggested.
> > - err = xiic_bus_busy(i2c);
> > - while (err && tries--) {
> > - msleep(1);
> > + if (!i2c->singlemaster) {
> > + /* for instance if previous transfer was terminated due to TX
> > + * error it might be that the bus is on it's way to become
> > + * available give it at most 3 ms to wake
> > + */
> > err = xiic_bus_busy(i2c);
> > + while (err && tries--) {
> > + msleep(1);
> > + err = xiic_bus_busy(i2c);
> > + }
> > }
>
> I would prefer to write this differently.
> if (i2c->singlemaster)
> return 0;
>
> Followed by origin code. Patch will be smaller and you don't need to add
> one more level of indentation.
That sounds better and easier than current version, thanks.
I will make the change in V3.
-Jaakko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread