On Mon, Sep 18, 2023 at 08:45:42PM -0700, Jan Bottorff wrote: > > > > > > > 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(); [...] > I did find the below text in the Arm Architectural Reference Manual (DDI > 0487I.a) section K13.4 "Using a mailbox to send an interrupt". It was nearly > the same wording as the ARM barrier document I previously referenced at > https://developer.arm.com/documentation/genc007826/latest/ This too says a > DSB barrier is required for memory updates to be observable in the ISR. > > " > K13.4 Using a mailbox to send an interrupt > In some message passing systems, it is common for one observer to update > memory and then notify a second observer of the update by sending an > interrupt, using a mailbox. Although a memory access might be made to > initiate the sending of the mailbox interrupt, a DSB instruction is > required to ensure the completion of previous memory accesses. > > Therefore, the following sequence is required to ensure that P2 observes the > updated value: > > AArch32 > P1 > STR R5, [R1] ; message stored to shared memory location > DSB ST > STR R0, [R4] ; R4 contains the address of a mailbox > P2 > ; interrupt service routine > LDR R5, [R1] > > AArch64 > P1 > STR W5, [X1] ; message stored to shared memory location > DSB ST > STR W0, [X4] ; R4 contains the address of a mailbox > P2 > ; interrupt service routine > LDR W5, [X1] > " Will convinced me in the past that a DMB is sufficient here unless the peripheral is CPU-local. The Arm ARM is not entirely clear here. > I hear your concern about how this barrier in platform portable code may > impact platforms other than the one I'm trying to fix. It almost seems like > there is some missing type of barrier macro that on ARM64 does what is > required for cases like this and on other platforms does whatever is > appropriate for that platform, often nothing. I also agree that a wmb() in the i2c driver is not the more elegant fix. For similar reasons, we hid barriers in the write*() macros, drivers need to stay architecture-agnostic as much as possible. Where does the regmap_write() end up? I think the barrier should be somewhere down this path. -- Catalin
