* [PATCH] aspeed: i2c: Fix DMA len write-enable bit handling
@ 2022-06-24 20:31 Peter Delevoryas
2022-06-24 20:34 ` Peter Delevoryas
0 siblings, 1 reply; 3+ messages in thread
From: Peter Delevoryas @ 2022-06-24 20:31 UTC (permalink / raw)
Cc: pdel, clg, andrew, joel, qemu-arm-nongnu.org, qemu-devel
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] aspeed: i2c: Fix DMA len write-enable bit handling
2022-06-24 20:31 [PATCH] aspeed: i2c: Fix DMA len write-enable bit handling Peter Delevoryas
@ 2022-06-24 20:34 ` Peter Delevoryas
2022-06-24 21:30 ` Cédric Le Goater
0 siblings, 1 reply; 3+ messages in thread
From: Peter Delevoryas @ 2022-06-24 20:34 UTC (permalink / raw)
Cc: Peter Delevoryas, Cédric Le Goater, Andrew Jeffery,
Joel Stanley, qemu-arm-nongnu.org@devvm9194.prn0.facebook.com,
Cameron Esfahani via
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] aspeed: i2c: Fix DMA len write-enable bit handling
2022-06-24 20:34 ` Peter Delevoryas
@ 2022-06-24 21:30 ` Cédric Le Goater
0 siblings, 0 replies; 3+ messages in thread
From: Cédric Le Goater @ 2022-06-24 21:30 UTC (permalink / raw)
To: Peter Delevoryas
Cc: Andrew Jeffery, Joel Stanley,
qemu-arm-nongnu.org@devvm9194.prn0.facebook.com,
Cameron Esfahani via
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
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-24 21:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-24 20:31 [PATCH] aspeed: i2c: Fix DMA len write-enable bit handling Peter Delevoryas
2022-06-24 20:34 ` Peter Delevoryas
2022-06-24 21:30 ` Cédric Le Goater
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.