Errors were happening in the ISR that looked like corrupted
memory. This was because writes from the core enabling interrupts
where not yet visible to the core running the ISR. A write barrier
assures writes to driver data structures are visible to all cores
before interrupts are enabled.
The ARM Barrier Litmus Tests and Cookbook has an example under
Sending Interrupts and Barriers that matches the usage in this
driver. That document says a DSB barrier is required.
Signed-off-by: Jan Bottorff <[email protected]>
---
drivers/i2c/busses/i2c-designware-master.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index ca1035e010c7..1694ac6bb592 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -248,6 +248,14 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
/* Dummy read to avoid the register getting stuck on Bay Trail */
regmap_read(dev->map, DW_IC_ENABLE_STATUS, &dummy);
+ /*
+ * To guarantee data written by the current core is visible to
+ * all cores, a write barrier is required. This needs to be
+ * before an interrupt causes execution on another core.
+ * For ARM processors, this needs to be a DSB barrier.
+ */
+ wmb();
+
/* Clear and enable interrupts */
regmap_read(dev->map, DW_IC_CLR_INTR, &dummy);
regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK);
--
2.41.0
Hi Jan,
On 13/09/2023 03:03, Jan Bottorff wrote:
> Errors were happening in the ISR that looked like corrupted
> memory. This was because writes from the core enabling interrupts
> where not yet visible to the core running the ISR. A write barrier
Typo where => were
> assures writes to driver data structures are visible to all cores
> before interrupts are enabled.
Maybe rephrase this using the direct style describing what the commit
does like "Add a write barrier before enabling interrupts to assure [...]"
>
> The ARM Barrier Litmus Tests and Cookbook has an example under
> Sending Interrupts and Barriers that matches the usage in this
> driver. That document says a DSB barrier is required.
>
> Signed-off-by: Jan Bottorff <[email protected]>
> ---
> drivers/i2c/busses/i2c-designware-master.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index ca1035e010c7..1694ac6bb592 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -248,6 +248,14 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
> /* Dummy read to avoid the register getting stuck on Bay Trail */
> regmap_read(dev->map, DW_IC_ENABLE_STATUS, &dummy);
>
> + /*
> + * To guarantee data written by the current core is visible to
> + * all cores, a write barrier is required. This needs to be
> + * before an interrupt causes execution on another core.
> + * For ARM processors, this needs to be a DSB barrier.
> + */
> + wmb();
> +
> /* Clear and enable interrupts */
> regmap_read(dev->map, DW_IC_CLR_INTR, &dummy);
> regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK);
Apart from the commit message it looks good to me.
If I understand correctly without this wmb() it is possible that the
writes to dev->msg_write_idx , dev->msg_read_idx = 0 etc would not yet
be visible to another CPU running the ISR handler right after enabling
those.
Reviewed-by: Yann Sionneau <[email protected]>
Tested-by: Yann Sionneau <[email protected]>
Thanks!
Regards,
--
Yann