| Message ID | 20220624203151.2026355-1-pdel@fb.com |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: i2c: Fix DMA len write-enable bit handling | expand |
> On Jun 24, 2022, at 1:31 PM, Peter Delevoryas <pdel@fb.com> wrote: > > I noticed i2c rx transfers were getting shortened to "1" on Zephyr. It > seems to be because the Zephyr i2c driver sets the RX DMA len with the > RX field write-enable bit set (bit 31) to avoid a read-modify-write. [1] > > /* 0x1C : I2CM Master DMA Transfer Length Register */ > > I think we should be checking the write-enable bits on the incoming > value, not checking the register array. I'm not sure we're even writing > the write-enable bits to the register array, actually. > > [1] https://github.com/AspeedTech-BMC/zephyr/blob/db3dbcc9c52e67a47180890ac938ed380b33f91c/drivers/i2c/i2c_aspeed.c#L145-L148 Arg, forgot this: Fixes: ba2cccd64e90f34 ("aspeed: i2c: Add new mode support”) Should I resend as v2? Thanks, Peter > > Signed-off-by: Peter Delevoryas <pdel@fb.com> > --- > hw/i2c/aspeed_i2c.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c > index 37ae1f2e04..c4fce7474a 100644 > --- a/hw/i2c/aspeed_i2c.c > +++ b/hw/i2c/aspeed_i2c.c > @@ -644,18 +644,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset, > RX_BUF_LEN) + 1; > break; > case A_I2CM_DMA_LEN: > - w1t = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T) || > - ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T); > + w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) || > + FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T); > /* If none of the w1t bits are set, just write to the reg as normal. */ > if (!w1t) { > bus->regs[R_I2CM_DMA_LEN] = value; > break; > } > - if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) { > + if (FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) { > ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN, > FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN)); > } > - if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) { > + if (FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) { > ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN, > FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN)); > } > -- > 2.30.2 >
On 6/24/22 22:34, Peter Delevoryas wrote: > > >> On Jun 24, 2022, at 1:31 PM, Peter Delevoryas <pdel@fb.com> wrote: >> >> I noticed i2c rx transfers were getting shortened to "1" on Zephyr. It >> seems to be because the Zephyr i2c driver sets the RX DMA len with the >> RX field write-enable bit set (bit 31) to avoid a read-modify-write. [1] >> >> /* 0x1C : I2CM Master DMA Transfer Length Register */ >> >> I think we should be checking the write-enable bits on the incoming >> value, not checking the register array. I'm not sure we're even writing >> the write-enable bits to the register array, actually. >> >> [1] https://github.com/AspeedTech-BMC/zephyr/blob/db3dbcc9c52e67a47180890ac938ed380b33f91c/drivers/i2c/i2c_aspeed.c#L145-L148 > > Arg, forgot this: > > Fixes: ba2cccd64e90f34 ("aspeed: i2c: Add new mode support”) > > Should I resend as v2? No. patchwork did it : http://patchwork.ozlabs.org/project/qemu-devel/patch/20220624203151.2026355-1-pdel@fb.com/ Thanks, C. > Thanks, > Peter > >> >> Signed-off-by: Peter Delevoryas <pdel@fb.com> >> --- >> hw/i2c/aspeed_i2c.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c >> index 37ae1f2e04..c4fce7474a 100644 >> --- a/hw/i2c/aspeed_i2c.c >> +++ b/hw/i2c/aspeed_i2c.c >> @@ -644,18 +644,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset, >> RX_BUF_LEN) + 1; >> break; >> case A_I2CM_DMA_LEN: >> - w1t = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T) || >> - ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T); >> + w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) || >> + FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T); >> /* If none of the w1t bits are set, just write to the reg as normal. */ >> if (!w1t) { >> bus->regs[R_I2CM_DMA_LEN] = value; >> break; >> } >> - if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) { >> + if (FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) { >> ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN, >> FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN)); >> } >> - if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) { >> + if (FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) { >> ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN, >> FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN)); >> } >> -- >> 2.30.2 >> >
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index 37ae1f2e04..c4fce7474a 100644 --- a/hw/i2c/aspeed_i2c.c +++ b/hw/i2c/aspeed_i2c.c @@ -644,18 +644,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset, RX_BUF_LEN) + 1; break; case A_I2CM_DMA_LEN: - w1t = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T) || - ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T); + w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) || + FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T); /* If none of the w1t bits are set, just write to the reg as normal. */ if (!w1t) { bus->regs[R_I2CM_DMA_LEN] = value; break; } - if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) { + if (FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) { ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN, FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN)); } - if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) { + if (FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) { ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN, FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN)); }
I noticed i2c rx transfers were getting shortened to "1" on Zephyr. It seems to be because the Zephyr i2c driver sets the RX DMA len with the RX field write-enable bit set (bit 31) to avoid a read-modify-write. [1] /* 0x1C : I2CM Master DMA Transfer Length Register */ I think we should be checking the write-enable bits on the incoming value, not checking the register array. I'm not sure we're even writing the write-enable bits to the register array, actually. [1] https://github.com/AspeedTech-BMC/zephyr/blob/db3dbcc9c52e67a47180890ac938ed380b33f91c/drivers/i2c/i2c_aspeed.c#L145-L148 Signed-off-by: Peter Delevoryas <pdel@fb.com> --- hw/i2c/aspeed_i2c.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)