Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/14/2018 6:23 AM, Guenter Roeck wrote:
On 09/13/2018 10:38 PM, Cédric Le Goater wrote:
That seems to suggest that none of the status bits auto-clears, and
that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
You are right. I just pushed another version of the previous patch
with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
- bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can
you give
it a try ?
Works fine for me for all affected qemu platforms.
How do you want to proceed with the qemu patches ? I attached my patches
for reference. Maybe you can add them to your tree if they are ok and
submit
the entire series together to the qemu mailing list ?
yes. They are pushed in my aspeed-3.1 branch. I will send the series
on the list.
Excellent. Thanks a lot!
Guenter
Awesome! Many thanks to Guenter, Cédric and Joel. I really appreciate
it.
Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/14/2018 6:23 AM, Guenter Roeck wrote:
On 09/13/2018 10:38 PM, Cédric Le Goater wrote:
That seems to suggest that none of the status bits auto-clears, and
that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
You are right. I just pushed another version of the previous patch
with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
- bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can
you give
it a try ?
Works fine for me for all affected qemu platforms.
How do you want to proceed with the qemu patches ? I attached my patches
for reference. Maybe you can add them to your tree if they are ok and
submit
the entire series together to the qemu mailing list ?
yes. They are pushed in my aspeed-3.1 branch. I will send the series
on the list.
Excellent. Thanks a lot!
Guenter
Awesome! Many thanks to Guenter, Cédric and Joel. I really appreciate
it.
Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/13/2018 10:38 PM, Cédric Le Goater wrote:
That seems to suggest that none of the status bits auto-clears, and that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
You are right. I just pushed another version of the previous patch with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
- bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
it a try ?
Works fine for me for all affected qemu platforms.
How do you want to proceed with the qemu patches ? I attached my patches
for reference. Maybe you can add them to your tree if they are ok and submit
the entire series together to the qemu mailing list ?
yes. They are pushed in my aspeed-3.1 branch. I will send the series
on the list.
Excellent. Thanks a lot!
Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/13/2018 10:38 PM, Cédric Le Goater wrote:
That seems to suggest that none of the status bits auto-clears, and that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
You are right. I just pushed another version of the previous patch with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
- bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
it a try ?
Works fine for me for all affected qemu platforms.
How do you want to proceed with the qemu patches ? I attached my patches
for reference. Maybe you can add them to your tree if they are ok and submit
the entire series together to the qemu mailing list ?
yes. They are pushed in my aspeed-3.1 branch. I will send the series
on the list.
Excellent. Thanks a lot!
Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
>>> That seems to suggest that none of the status bits auto-clears, and that
>>> the above code clearing intr_status should be removed entirely.
>>> Am I missing something ?
>>
>> You are right. I just pushed another version of the previous patch with this
>> new hunk :
>>
>> @@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
>> {
>> bus->cmd &= ~0x;
>> bus->cmd |= value & 0x;
>> - bus->intr_status = 0;
>> if (bus->cmd & I2CD_M_START_CMD) {
>> uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>
>>
>> The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
>> it a try ?
>>
>
> Works fine for me for all affected qemu platforms.
>
> How do you want to proceed with the qemu patches ? I attached my patches
> for reference. Maybe you can add them to your tree if they are ok and submit
> the entire series together to the qemu mailing list ?
yes. They are pushed in my aspeed-3.1 branch. I will send the series
on the list.
Thanks,
C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
>>> That seems to suggest that none of the status bits auto-clears, and that
>>> the above code clearing intr_status should be removed entirely.
>>> Am I missing something ?
>>
>> You are right. I just pushed another version of the previous patch with this
>> new hunk :
>>
>> @@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
>> {
>> bus->cmd &= ~0x;
>> bus->cmd |= value & 0x;
>> - bus->intr_status = 0;
>> if (bus->cmd & I2CD_M_START_CMD) {
>> uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>
>>
>> The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
>> it a try ?
>>
>
> Works fine for me for all affected qemu platforms.
>
> How do you want to proceed with the qemu patches ? I attached my patches
> for reference. Maybe you can add them to your tree if they are ok and submit
> the entire series together to the qemu mailing list ?
yes. They are pushed in my aspeed-3.1 branch. I will send the series
on the list.
Thanks,
C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/13/2018 09:35 AM, Cédric Le Goater wrote:
On 09/13/2018 05:57 PM, Guenter Roeck wrote:
On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
On 09/13/2018 03:33 PM, Guenter Roeck wrote:
[ ... ]
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus,
uint64_t value)
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
- bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
it deserves a comment to understand which scenario we are trying to handle.
Ok. FWIW, I wonder if intr_status should be touched here in the first place,
but I neither have the hardware nor a datasheet, so I don't know if any bits
are auto-cleared.
I just pushed a patch on my branch with some more explanation :
https://github.com/legoater/qemu/commits/aspeed-3.1
That seems to suggest that none of the status bits auto-clears, and that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
You are right. I just pushed another version of the previous patch with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
-bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
it a try ?
Works fine for me for all affected qemu platforms.
How do you want to proceed with the qemu patches ? I attached my patches
for reference. Maybe you can add them to your tree if they are ok and submit
the entire series together to the qemu mailing list ?
Thanks,
Guenter
>From 9eb05b7f6d7ceb2919fa8b865772ab9207d1afed Mon Sep 17 00:00:00 2001
From: Guenter Roeck
Date: Wed, 12 Sep 2018 16:35:20 -0700
Subject: [PATCH 1/2] aspeed/i2c: Handle receive command in separate function
Receive command handling may have to be deferred if a previous receive
done interrupt was not yet acknowledged. Move receive command handling
into a separate function to prepare for the necessary changes.
Signed-off-by: Guenter Roeck
---
hw/i2c/aspeed_i2c.c | 37 +
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index de6b083..d81f865 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -192,6 +192,26 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
}
+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+{
+int ret;
+
+aspeed_i2c_set_state(bus, I2CD_MRXD);
+ret = i2c_recv(bus->bus);
+if (ret < 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
+ret = 0xff;
+} else {
+bus->intr_status |= I2CD_INTR_RX_DONE;
+}
+bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
+i2c_nack(bus->bus);
+}
+bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
+aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+}
+
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
@@ -238,22 +258,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
}
if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
-int ret;
-
-aspeed_i2c_set_state(bus, I2CD_MRXD);
-ret = i2c_recv(bus->bus);
-if (ret < 0) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
-ret = 0xff;
-} else {
-bus->intr_status |= I2CD_INTR_RX_DONE;
-}
-bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
-if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
-i2c_nack(bus->bus);
-}
-bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
-aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+aspeed_i2c_handle_rx_cmd(bus);
}
if (bus->cmd & I2CD_M_STOP_CMD) {
--
2.7.4
>From b5e1879de9c347ab03a09d71f12d40aad0a16d6d Mon Sep 17 00:00:00 2001
From: Guenter Roeck
Date: Wed, 12 Sep 2018 14:19:30 -0700
Subject: [PATCH 2/2] aspeed/i2c: Fix receive done interrupt handling
The AST2500 datasheet says:
I2CD10 Interrupt Status Register
bit 2 Receive Done Interrupt status
S/W needs to clear this status bit to allow next data receiving
The Rx interrrupt done interrupt status bit needs to be cleared
explicitly before the next byte can be received, and must therefore
not be auto-cleared. Also, receiving the next byte must be delayed
until the bit has been cleared.
Signed-off-by: Guenter Roeck
---
hw/i2c/aspeed_i2c.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/13/2018 09:35 AM, Cédric Le Goater wrote:
On 09/13/2018 05:57 PM, Guenter Roeck wrote:
On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
On 09/13/2018 03:33 PM, Guenter Roeck wrote:
[ ... ]
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus,
uint64_t value)
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
- bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
it deserves a comment to understand which scenario we are trying to handle.
Ok. FWIW, I wonder if intr_status should be touched here in the first place,
but I neither have the hardware nor a datasheet, so I don't know if any bits
are auto-cleared.
I just pushed a patch on my branch with some more explanation :
https://github.com/legoater/qemu/commits/aspeed-3.1
That seems to suggest that none of the status bits auto-clears, and that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
You are right. I just pushed another version of the previous patch with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
-bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
it a try ?
Works fine for me for all affected qemu platforms.
How do you want to proceed with the qemu patches ? I attached my patches
for reference. Maybe you can add them to your tree if they are ok and submit
the entire series together to the qemu mailing list ?
Thanks,
Guenter
>From 9eb05b7f6d7ceb2919fa8b865772ab9207d1afed Mon Sep 17 00:00:00 2001
From: Guenter Roeck
Date: Wed, 12 Sep 2018 16:35:20 -0700
Subject: [PATCH 1/2] aspeed/i2c: Handle receive command in separate function
Receive command handling may have to be deferred if a previous receive
done interrupt was not yet acknowledged. Move receive command handling
into a separate function to prepare for the necessary changes.
Signed-off-by: Guenter Roeck
---
hw/i2c/aspeed_i2c.c | 37 +
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index de6b083..d81f865 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -192,6 +192,26 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
}
+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+{
+int ret;
+
+aspeed_i2c_set_state(bus, I2CD_MRXD);
+ret = i2c_recv(bus->bus);
+if (ret < 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
+ret = 0xff;
+} else {
+bus->intr_status |= I2CD_INTR_RX_DONE;
+}
+bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
+i2c_nack(bus->bus);
+}
+bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
+aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+}
+
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
@@ -238,22 +258,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
}
if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
-int ret;
-
-aspeed_i2c_set_state(bus, I2CD_MRXD);
-ret = i2c_recv(bus->bus);
-if (ret < 0) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
-ret = 0xff;
-} else {
-bus->intr_status |= I2CD_INTR_RX_DONE;
-}
-bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
-if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
-i2c_nack(bus->bus);
-}
-bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
-aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+aspeed_i2c_handle_rx_cmd(bus);
}
if (bus->cmd & I2CD_M_STOP_CMD) {
--
2.7.4
>From b5e1879de9c347ab03a09d71f12d40aad0a16d6d Mon Sep 17 00:00:00 2001
From: Guenter Roeck
Date: Wed, 12 Sep 2018 14:19:30 -0700
Subject: [PATCH 2/2] aspeed/i2c: Fix receive done interrupt handling
The AST2500 datasheet says:
I2CD10 Interrupt Status Register
bit 2 Receive Done Interrupt status
S/W needs to clear this status bit to allow next data receiving
The Rx interrrupt done interrupt status bit needs to be cleared
explicitly before the next byte can be received, and must therefore
not be auto-cleared. Also, receiving the next byte must be delayed
until the bit has been cleared.
Signed-off-by: Guenter Roeck
---
hw/i2c/aspeed_i2c.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/13/2018 9:51 AM, Cédric Le Goater wrote: Hello ! On 09/13/2018 06:31 PM, Jae Hyun Yoo wrote: Hi Cédric, On 9/12/2018 10:47 PM, Cédric Le Goater wrote: On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. Let me share my test result. Your code change works on 100KHz bus speed but doesn't work well on 1MHz bus speed. Investigated that interrupt handling is fast enough in 100KHz test but in 1MHz, most of data is corrupted because the bit is cleared at the beginning of interrupt handler so it allows receiving of the next data but the interrupt handler isn't fast enough to read the data buffer on time. I checked this problem on BMC-ME channel which ME sends lots of IPMB packets to BMC at 1MHz speed. You could simply check the data corruption problem on the BMC-ME channel. OK. My thought is, the current code is right for real Aspeed I2C hardware. It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the actual Aspeed I2C hardware correctly. That might be very well possible yes. it also misses support for the slave mode and the DMA registers. Yes, it would be good if qemu's Aspeed I2C model supports slave mode. Since the current linux Aspeed I2C driver supports byte transfer mode only, so DMA transfer mode support in qemu could be considered later. The Aspeed SDK already does, so yes, we will need to consider it. Yes, Aspeed SDK does but the driver in upstream doesn't at this time. So we will need to consider it because byte mode makes too many interrupt calls which is not good for performance. Implementing pool mode and DMA mode for linux Aspeed I2C are in my backlog at this moment. Is there a QEMU model for an I2C/IPMB device ? Not sure because I'm not familiar with QEMU. Need to check. Thanks, Jae There is already a large IPMI framework in QEMU, that would be interesting. to extend. Thanks, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/13/2018 9:51 AM, Cédric Le Goater wrote: Hello ! On 09/13/2018 06:31 PM, Jae Hyun Yoo wrote: Hi Cédric, On 9/12/2018 10:47 PM, Cédric Le Goater wrote: On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. Let me share my test result. Your code change works on 100KHz bus speed but doesn't work well on 1MHz bus speed. Investigated that interrupt handling is fast enough in 100KHz test but in 1MHz, most of data is corrupted because the bit is cleared at the beginning of interrupt handler so it allows receiving of the next data but the interrupt handler isn't fast enough to read the data buffer on time. I checked this problem on BMC-ME channel which ME sends lots of IPMB packets to BMC at 1MHz speed. You could simply check the data corruption problem on the BMC-ME channel. OK. My thought is, the current code is right for real Aspeed I2C hardware. It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the actual Aspeed I2C hardware correctly. That might be very well possible yes. it also misses support for the slave mode and the DMA registers. Yes, it would be good if qemu's Aspeed I2C model supports slave mode. Since the current linux Aspeed I2C driver supports byte transfer mode only, so DMA transfer mode support in qemu could be considered later. The Aspeed SDK already does, so yes, we will need to consider it. Yes, Aspeed SDK does but the driver in upstream doesn't at this time. So we will need to consider it because byte mode makes too many interrupt calls which is not good for performance. Implementing pool mode and DMA mode for linux Aspeed I2C are in my backlog at this moment. Is there a QEMU model for an I2C/IPMB device ? Not sure because I'm not familiar with QEMU. Need to check. Thanks, Jae There is already a large IPMI framework in QEMU, that would be interesting. to extend. Thanks, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
Hello ! On 09/13/2018 06:31 PM, Jae Hyun Yoo wrote: > Hi Cédric, > > On 9/12/2018 10:47 PM, Cédric Le Goater wrote: >> On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote: >>> On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 4:33 PM, Guenter Roeck wrote: >> Looking into the patch, clearing the interrupt status at the end of an >> interrupt handler is always suspicious and tends to result in race >> conditions (because additional interrupts may have arrived while handling >> the existing interrupts, or because interrupt handling itself may trigger >> another interrupt). With that in mind, the following patch fixes the >> problem for me. >> >> Guenter >> >> --- >> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c >> b/drivers/i2c/busses/i2c-aspeed.c >> index c258c4d9a4c0..c488e6950b7c 100644 >> --- a/drivers/i2c/busses/i2c-aspeed.c >> +++ b/drivers/i2c/busses/i2c-aspeed.c >> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void >> *dev_id) >> spin_lock(>lock); >> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); >> + /* Ack all interrupt bits. */ >> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); >> irq_remaining = irq_received; >> #if IS_ENABLED(CONFIG_I2C_SLAVE) >> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void >> *dev_id) >> "irq handled != irq. expected 0x%08x, but was 0x%08x\n", >> irq_received, irq_handled); >> - /* Ack all interrupt bits. */ >> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); >> spin_unlock(>lock); >> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; >> } >> > > My intention of putting the code at the end of interrupt handler was, > to reduce possibility of combined irq calls which is explained in this > patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. >>> >>> Well, that's generally right but not always. Sometimes that depends on >>> hardware and Aspeed I2C is the case. >>> >>> This is a description from Aspeed AST2500 datasheet: >>> I2CD10 Interrupt Status Register >>> bit 2 Receive Done Interrupt status >>> S/W needs to clear this status bit to allow next data receiving. >>> >>> It means, driver should hold this bit to prevent transition of hardware >>> state machine until the driver handles received data, so the bit should >>> be cleared at the end of interrupt handler. >>> >>> Let me share my test result. Your code change works on 100KHz bus speed >>> but doesn't work well on 1MHz bus speed. Investigated that interrupt >>> handling is fast enough in 100KHz test but in 1MHz, most of data is >>> corrupted because the bit is cleared at the beginning of interrupt >>> handler so it allows receiving of the next data but the interrupt >>> handler isn't fast enough to read the data buffer on time. I checked >>> this problem on BMC-ME channel which ME sends lots of IPMB packets to >>> BMC at 1MHz speed. You could simply check the data corruption problem on >>> the BMC-ME channel. >> >> OK. >> >>> My thought is, the current code is right for real Aspeed I2C hardware. >>> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the >>> actual Aspeed I2C hardware correctly. >> >> That might be very well possible yes. it also misses support for the slave >> mode and the DMA registers. >> > > Yes, it would be good if qemu's Aspeed I2C model supports slave mode. > Since the current linux Aspeed I2C driver supports byte transfer mode > only, so DMA transfer mode support in qemu could be considered later. The Aspeed SDK already does, so yes, we will need to consider it. > Implementing pool mode and DMA mode for linux Aspeed I2C are in my > backlog at this moment. Is there a QEMU model for an I2C/IPMB device ? There is already a large IPMI framework in QEMU, that would be interesting. to extend. Thanks, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
Hello ! On 09/13/2018 06:31 PM, Jae Hyun Yoo wrote: > Hi Cédric, > > On 9/12/2018 10:47 PM, Cédric Le Goater wrote: >> On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote: >>> On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 4:33 PM, Guenter Roeck wrote: >> Looking into the patch, clearing the interrupt status at the end of an >> interrupt handler is always suspicious and tends to result in race >> conditions (because additional interrupts may have arrived while handling >> the existing interrupts, or because interrupt handling itself may trigger >> another interrupt). With that in mind, the following patch fixes the >> problem for me. >> >> Guenter >> >> --- >> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c >> b/drivers/i2c/busses/i2c-aspeed.c >> index c258c4d9a4c0..c488e6950b7c 100644 >> --- a/drivers/i2c/busses/i2c-aspeed.c >> +++ b/drivers/i2c/busses/i2c-aspeed.c >> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void >> *dev_id) >> spin_lock(>lock); >> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); >> + /* Ack all interrupt bits. */ >> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); >> irq_remaining = irq_received; >> #if IS_ENABLED(CONFIG_I2C_SLAVE) >> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void >> *dev_id) >> "irq handled != irq. expected 0x%08x, but was 0x%08x\n", >> irq_received, irq_handled); >> - /* Ack all interrupt bits. */ >> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); >> spin_unlock(>lock); >> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; >> } >> > > My intention of putting the code at the end of interrupt handler was, > to reduce possibility of combined irq calls which is explained in this > patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. >>> >>> Well, that's generally right but not always. Sometimes that depends on >>> hardware and Aspeed I2C is the case. >>> >>> This is a description from Aspeed AST2500 datasheet: >>> I2CD10 Interrupt Status Register >>> bit 2 Receive Done Interrupt status >>> S/W needs to clear this status bit to allow next data receiving. >>> >>> It means, driver should hold this bit to prevent transition of hardware >>> state machine until the driver handles received data, so the bit should >>> be cleared at the end of interrupt handler. >>> >>> Let me share my test result. Your code change works on 100KHz bus speed >>> but doesn't work well on 1MHz bus speed. Investigated that interrupt >>> handling is fast enough in 100KHz test but in 1MHz, most of data is >>> corrupted because the bit is cleared at the beginning of interrupt >>> handler so it allows receiving of the next data but the interrupt >>> handler isn't fast enough to read the data buffer on time. I checked >>> this problem on BMC-ME channel which ME sends lots of IPMB packets to >>> BMC at 1MHz speed. You could simply check the data corruption problem on >>> the BMC-ME channel. >> >> OK. >> >>> My thought is, the current code is right for real Aspeed I2C hardware. >>> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the >>> actual Aspeed I2C hardware correctly. >> >> That might be very well possible yes. it also misses support for the slave >> mode and the DMA registers. >> > > Yes, it would be good if qemu's Aspeed I2C model supports slave mode. > Since the current linux Aspeed I2C driver supports byte transfer mode > only, so DMA transfer mode support in qemu could be considered later. The Aspeed SDK already does, so yes, we will need to consider it. > Implementing pool mode and DMA mode for linux Aspeed I2C are in my > backlog at this moment. Is there a QEMU model for an I2C/IPMB device ? There is already a large IPMI framework in QEMU, that would be interesting. to extend. Thanks, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/13/2018 05:57 PM, Guenter Roeck wrote:
> On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
>> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> [ ... ]
> /*
> * The state machine needs some refinement. It is only used to track
> * invalid STOP commands for the moment.
> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
> *bus, uint64_t value)
> {
> bus->cmd &= ~0x;
> bus->cmd |= value & 0x;
> - bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
it deserves a comment to understand which scenario we are trying to handle.
>>>
>>> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
>>> but I neither have the hardware nor a datasheet, so I don't know if any bits
>>> are auto-cleared.
>>
>> I just pushed a patch on my branch with some more explanation :
>>
>> https://github.com/legoater/qemu/commits/aspeed-3.1
>>
> That seems to suggest that none of the status bits auto-clears, and that
> the above code clearing intr_status should be removed entirely.
> Am I missing something ?
You are right. I just pushed another version of the previous patch with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
-bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
it a try ?
Thanks,
C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/13/2018 05:57 PM, Guenter Roeck wrote:
> On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
>> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> [ ... ]
> /*
> * The state machine needs some refinement. It is only used to track
> * invalid STOP commands for the moment.
> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
> *bus, uint64_t value)
> {
> bus->cmd &= ~0x;
> bus->cmd |= value & 0x;
> - bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
it deserves a comment to understand which scenario we are trying to handle.
>>>
>>> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
>>> but I neither have the hardware nor a datasheet, so I don't know if any bits
>>> are auto-cleared.
>>
>> I just pushed a patch on my branch with some more explanation :
>>
>> https://github.com/legoater/qemu/commits/aspeed-3.1
>>
> That seems to suggest that none of the status bits auto-clears, and that
> the above code clearing intr_status should be removed entirely.
> Am I missing something ?
You are right. I just pushed another version of the previous patch with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
-bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
it a try ?
Thanks,
C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
Hi Cédric, On 9/12/2018 10:47 PM, Cédric Le Goater wrote: On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. Let me share my test result. Your code change works on 100KHz bus speed but doesn't work well on 1MHz bus speed. Investigated that interrupt handling is fast enough in 100KHz test but in 1MHz, most of data is corrupted because the bit is cleared at the beginning of interrupt handler so it allows receiving of the next data but the interrupt handler isn't fast enough to read the data buffer on time. I checked this problem on BMC-ME channel which ME sends lots of IPMB packets to BMC at 1MHz speed. You could simply check the data corruption problem on the BMC-ME channel. OK. My thought is, the current code is right for real Aspeed I2C hardware. It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the actual Aspeed I2C hardware correctly. That might be very well possible yes. it also misses support for the slave mode and the DMA registers. Yes, it would be good if qemu's Aspeed I2C model supports slave mode. Since the current linux Aspeed I2C driver supports byte transfer mode only, so DMA transfer mode support in qemu could be considered later. Implementing pool mode and DMA mode for linux Aspeed I2C are in my backlog at this moment. Thanks, Jae Thanks for the info, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
Hi Cédric, On 9/12/2018 10:47 PM, Cédric Le Goater wrote: On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. Let me share my test result. Your code change works on 100KHz bus speed but doesn't work well on 1MHz bus speed. Investigated that interrupt handling is fast enough in 100KHz test but in 1MHz, most of data is corrupted because the bit is cleared at the beginning of interrupt handler so it allows receiving of the next data but the interrupt handler isn't fast enough to read the data buffer on time. I checked this problem on BMC-ME channel which ME sends lots of IPMB packets to BMC at 1MHz speed. You could simply check the data corruption problem on the BMC-ME channel. OK. My thought is, the current code is right for real Aspeed I2C hardware. It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the actual Aspeed I2C hardware correctly. That might be very well possible yes. it also misses support for the slave mode and the DMA registers. Yes, it would be good if qemu's Aspeed I2C model supports slave mode. Since the current linux Aspeed I2C driver supports byte transfer mode only, so DMA transfer mode support in qemu could be considered later. Implementing pool mode and DMA mode for linux Aspeed I2C are in my backlog at this moment. Thanks, Jae Thanks for the info, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> On 09/12/2018 10:45 PM, Cédric Le Goater wrote
>
> [ ... ]
>
>>> ---
>>> qemu:
>>>
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index c762c73..0d4aa08 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>>> return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>>> }
>>> +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
>>> +{
>>> + int ret;
>>> +
>>> + if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
>>> + return;
>>> + }
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>>
>>> + if (bus->intr_status & I2CD_INTR_RX_DONE) {
>>> + return;
>>> + }
>>
>> should be handled in aspeed_i2c_bus_handle_cmd() I think
>>
>
> I moved those two checks into the calling code.
ok
>>> + aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> + ret = i2c_recv(bus->bus);
>>> + if (ret < 0) {
>>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> + ret = 0xff;
>>> + } else {
>>> + bus->intr_status |= I2CD_INTR_RX_DONE;
>>> + }
>>> + bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> + if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> + i2c_nack(bus->bus);
>>> + }
>>> + bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> + aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> +}
>>> +
>>> /*
>>> * The state machine needs some refinement. It is only used to track
>>> * invalid STOP commands for the moment.
>>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
>>> *bus, uint64_t value)
>>> {
>>> bus->cmd &= ~0x;
>>> bus->cmd |= value & 0x;
>>> - bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>>
>
> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> but I neither have the hardware nor a datasheet, so I don't know if any bits
> are auto-cleared.
I just pushed a patch on my branch with some more explanation :
https://github.com/legoater/qemu/commits/aspeed-3.1
>
>>> if (bus->cmd & I2CD_M_START_CMD) {
>>> uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
>>> *bus, uint64_t value)
>>> }
>>> if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
>>> - int ret;
>>> -
>>> - aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> - ret = i2c_recv(bus->bus);
>>> - if (ret < 0) {
>>> - qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> - ret = 0xff;
>>> - } else {
>>> - bus->intr_status |= I2CD_INTR_RX_DONE;
>>> - }
>>> - bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> - if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> - i2c_nack(bus->bus);
>>> - }
>>> - bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> - aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> + aspeed_i2c_handle_rx_cmd(bus);
>>> }
>>> if (bus->cmd & I2CD_M_STOP_CMD) {
>>> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
>>> offset,
>>> uint64_t value, unsigned size)
>>> {
>>> AspeedI2CBus *bus = opaque;
>>> + int status;
>>> switch (offset) {
>>> case I2CD_FUN_CTRL_REG:
>>> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
>>> offset,
>>> bus->intr_ctrl = value & 0x7FFF;
>>> break;
>>> case I2CD_INTR_STS_REG:
>>> + status = bus->intr_status;
>>> bus->intr_status &= ~(value & 0x7FFF);
>>> - bus->controller->intr_status &= ~(1 << bus->id);
>>> - qemu_irq_lower(bus->controller->irq);
>>> + if (!bus->intr_status) {
>>> + bus->controller->intr_status &= ~(1 << bus->id);
>>> + qemu_irq_lower(bus->controller->irq);
>>> + }
>>
>> That part below is indeed something to fix. I had a similar patch.
>>
>
> Should I split it out as separate patch ? Alternatively, if you submitted
> your patch already, I'll be happy to use it as base for mine.
See below.
Thanks a lot,
C.
>From dea796e8d35ba7a70e4b14fbc30ff970a347b195 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?=
Date: Thu, 13 Sep 2018 17:44:32 +0200
Subject: [PATCH] aspeed/i2c: lower bus interrupt when all interrupts have been
cleared
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Also include some documentation on the interrupt status bits and how
they should be cleared. Also, the model does not implement correctly
the RX_DONE bit behavior which should be
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
[ ... ]
> >>> /*
> >>> * The state machine needs some refinement. It is only used to track
> >>> * invalid STOP commands for the moment.
> >>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
> >>> *bus, uint64_t value)
> >>> {
> >>> bus->cmd &= ~0x;
> >>> bus->cmd |= value & 0x;
> >>> - bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
> >>
> >> it deserves a comment to understand which scenario we are trying to handle.
> >>
> >
> > Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> > but I neither have the hardware nor a datasheet, so I don't know if any bits
> > are auto-cleared.
>
> I just pushed a patch on my branch with some more explanation :
>
> https://github.com/legoater/qemu/commits/aspeed-3.1
>
That seems to suggest that none of the status bits auto-clears, and that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
Thanks,
Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> On 09/12/2018 10:45 PM, Cédric Le Goater wrote
>
> [ ... ]
>
>>> ---
>>> qemu:
>>>
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index c762c73..0d4aa08 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>>> return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>>> }
>>> +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
>>> +{
>>> + int ret;
>>> +
>>> + if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
>>> + return;
>>> + }
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>>
>>> + if (bus->intr_status & I2CD_INTR_RX_DONE) {
>>> + return;
>>> + }
>>
>> should be handled in aspeed_i2c_bus_handle_cmd() I think
>>
>
> I moved those two checks into the calling code.
ok
>>> + aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> + ret = i2c_recv(bus->bus);
>>> + if (ret < 0) {
>>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> + ret = 0xff;
>>> + } else {
>>> + bus->intr_status |= I2CD_INTR_RX_DONE;
>>> + }
>>> + bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> + if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> + i2c_nack(bus->bus);
>>> + }
>>> + bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> + aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> +}
>>> +
>>> /*
>>> * The state machine needs some refinement. It is only used to track
>>> * invalid STOP commands for the moment.
>>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
>>> *bus, uint64_t value)
>>> {
>>> bus->cmd &= ~0x;
>>> bus->cmd |= value & 0x;
>>> - bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>>
>
> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> but I neither have the hardware nor a datasheet, so I don't know if any bits
> are auto-cleared.
I just pushed a patch on my branch with some more explanation :
https://github.com/legoater/qemu/commits/aspeed-3.1
>
>>> if (bus->cmd & I2CD_M_START_CMD) {
>>> uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
>>> *bus, uint64_t value)
>>> }
>>> if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
>>> - int ret;
>>> -
>>> - aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> - ret = i2c_recv(bus->bus);
>>> - if (ret < 0) {
>>> - qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> - ret = 0xff;
>>> - } else {
>>> - bus->intr_status |= I2CD_INTR_RX_DONE;
>>> - }
>>> - bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> - if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> - i2c_nack(bus->bus);
>>> - }
>>> - bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> - aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> + aspeed_i2c_handle_rx_cmd(bus);
>>> }
>>> if (bus->cmd & I2CD_M_STOP_CMD) {
>>> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
>>> offset,
>>> uint64_t value, unsigned size)
>>> {
>>> AspeedI2CBus *bus = opaque;
>>> + int status;
>>> switch (offset) {
>>> case I2CD_FUN_CTRL_REG:
>>> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
>>> offset,
>>> bus->intr_ctrl = value & 0x7FFF;
>>> break;
>>> case I2CD_INTR_STS_REG:
>>> + status = bus->intr_status;
>>> bus->intr_status &= ~(value & 0x7FFF);
>>> - bus->controller->intr_status &= ~(1 << bus->id);
>>> - qemu_irq_lower(bus->controller->irq);
>>> + if (!bus->intr_status) {
>>> + bus->controller->intr_status &= ~(1 << bus->id);
>>> + qemu_irq_lower(bus->controller->irq);
>>> + }
>>
>> That part below is indeed something to fix. I had a similar patch.
>>
>
> Should I split it out as separate patch ? Alternatively, if you submitted
> your patch already, I'll be happy to use it as base for mine.
See below.
Thanks a lot,
C.
>From dea796e8d35ba7a70e4b14fbc30ff970a347b195 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?=
Date: Thu, 13 Sep 2018 17:44:32 +0200
Subject: [PATCH] aspeed/i2c: lower bus interrupt when all interrupts have been
cleared
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Also include some documentation on the interrupt status bits and how
they should be cleared. Also, the model does not implement correctly
the RX_DONE bit behavior which should be
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
[ ... ]
> >>> /*
> >>> * The state machine needs some refinement. It is only used to track
> >>> * invalid STOP commands for the moment.
> >>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
> >>> *bus, uint64_t value)
> >>> {
> >>> bus->cmd &= ~0x;
> >>> bus->cmd |= value & 0x;
> >>> - bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
> >>
> >> it deserves a comment to understand which scenario we are trying to handle.
> >>
> >
> > Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> > but I neither have the hardware nor a datasheet, so I don't know if any bits
> > are auto-cleared.
>
> I just pushed a patch on my branch with some more explanation :
>
> https://github.com/legoater/qemu/commits/aspeed-3.1
>
That seems to suggest that none of the status bits auto-clears, and that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
Thanks,
Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/12/2018 10:45 PM, Cédric Le Goater wrote
[ ... ]
---
qemu:
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c762c73..0d4aa08 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
}
+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+{
+int ret;
+
+if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
+return;
+}
it deserves a comment to understand which scenario we are trying to handle.
+if (bus->intr_status & I2CD_INTR_RX_DONE) {
+return;
+}
should be handled in aspeed_i2c_bus_handle_cmd() I think
I moved those two checks into the calling code.
+aspeed_i2c_set_state(bus, I2CD_MRXD);
+ret = i2c_recv(bus->bus);
+if (ret < 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
+ret = 0xff;
+} else {
+bus->intr_status |= I2CD_INTR_RX_DONE;
+}
+bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
+i2c_nack(bus->bus);
+}
+bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
+aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+}
+
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus,
uint64_t value)
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
-bus->intr_status = 0;> +bus->intr_status &= I2CD_INTR_RX_DONE;
it deserves a comment to understand which scenario we are trying to handle.
Ok. FWIW, I wonder if intr_status should be touched here in the first place,
but I neither have the hardware nor a datasheet, so I don't know if any bits
are auto-cleared.
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
@@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus,
uint64_t value)
}
if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
-int ret;
-
-aspeed_i2c_set_state(bus, I2CD_MRXD);
-ret = i2c_recv(bus->bus);
-if (ret < 0) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
-ret = 0xff;
-} else {
-bus->intr_status |= I2CD_INTR_RX_DONE;
-}
-bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
-if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
-i2c_nack(bus->bus);
-}
-bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
-aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+aspeed_i2c_handle_rx_cmd(bus);
}
if (bus->cmd & I2CD_M_STOP_CMD) {
@@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
offset,
uint64_t value, unsigned size)
{
AspeedI2CBus *bus = opaque;
+int status;
switch (offset) {
case I2CD_FUN_CTRL_REG:
@@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
offset,
bus->intr_ctrl = value & 0x7FFF;
break;
case I2CD_INTR_STS_REG:
+status = bus->intr_status;
bus->intr_status &= ~(value & 0x7FFF);
-bus->controller->intr_status &= ~(1 << bus->id);
-qemu_irq_lower(bus->controller->irq);
+if (!bus->intr_status) {
+bus->controller->intr_status &= ~(1 << bus->id);
+qemu_irq_lower(bus->controller->irq);
+}
That part below is indeed something to fix. I had a similar patch.
Should I split it out as separate patch ? Alternatively, if you submitted
your patch already, I'll be happy to use it as base for mine.
Thanks,
Guenter
+if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status &
I2CD_INTR_RX_DONE)) {
+aspeed_i2c_handle_rx_cmd(bus);
+aspeed_i2c_bus_raise_interrupt(bus);
+}
ok.
Thanks for looking into this.
C.
break;
case I2CD_DEV_ADDR_REG:
qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/12/2018 10:45 PM, Cédric Le Goater wrote
[ ... ]
---
qemu:
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c762c73..0d4aa08 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
}
+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+{
+int ret;
+
+if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
+return;
+}
it deserves a comment to understand which scenario we are trying to handle.
+if (bus->intr_status & I2CD_INTR_RX_DONE) {
+return;
+}
should be handled in aspeed_i2c_bus_handle_cmd() I think
I moved those two checks into the calling code.
+aspeed_i2c_set_state(bus, I2CD_MRXD);
+ret = i2c_recv(bus->bus);
+if (ret < 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
+ret = 0xff;
+} else {
+bus->intr_status |= I2CD_INTR_RX_DONE;
+}
+bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
+i2c_nack(bus->bus);
+}
+bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
+aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+}
+
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus,
uint64_t value)
{
bus->cmd &= ~0x;
bus->cmd |= value & 0x;
-bus->intr_status = 0;> +bus->intr_status &= I2CD_INTR_RX_DONE;
it deserves a comment to understand which scenario we are trying to handle.
Ok. FWIW, I wonder if intr_status should be touched here in the first place,
but I neither have the hardware nor a datasheet, so I don't know if any bits
are auto-cleared.
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
@@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus,
uint64_t value)
}
if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
-int ret;
-
-aspeed_i2c_set_state(bus, I2CD_MRXD);
-ret = i2c_recv(bus->bus);
-if (ret < 0) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
-ret = 0xff;
-} else {
-bus->intr_status |= I2CD_INTR_RX_DONE;
-}
-bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
-if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
-i2c_nack(bus->bus);
-}
-bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
-aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+aspeed_i2c_handle_rx_cmd(bus);
}
if (bus->cmd & I2CD_M_STOP_CMD) {
@@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
offset,
uint64_t value, unsigned size)
{
AspeedI2CBus *bus = opaque;
+int status;
switch (offset) {
case I2CD_FUN_CTRL_REG:
@@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
offset,
bus->intr_ctrl = value & 0x7FFF;
break;
case I2CD_INTR_STS_REG:
+status = bus->intr_status;
bus->intr_status &= ~(value & 0x7FFF);
-bus->controller->intr_status &= ~(1 << bus->id);
-qemu_irq_lower(bus->controller->irq);
+if (!bus->intr_status) {
+bus->controller->intr_status &= ~(1 << bus->id);
+qemu_irq_lower(bus->controller->irq);
+}
That part below is indeed something to fix. I had a similar patch.
Should I split it out as separate patch ? Alternatively, if you submitted
your patch already, I'll be happy to use it as base for mine.
Thanks,
Guenter
+if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status &
I2CD_INTR_RX_DONE)) {
+aspeed_i2c_handle_rx_cmd(bus);
+aspeed_i2c_bus_raise_interrupt(bus);
+}
ok.
Thanks for looking into this.
C.
break;
case I2CD_DEV_ADDR_REG:
qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote: > On 9/11/2018 6:34 PM, Guenter Roeck wrote: >> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: >>> On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } >>> >>> My intention of putting the code at the end of interrupt handler was, >>> to reduce possibility of combined irq calls which is explained in this >>> patch. But YES, I agree with you. It could make a potential race >> >> Hmm, yes, but that doesn't explain why it would make sense to acknowledge >> the interrupt late. The interrupt ack only means "I am going to handle these >> interrupts". If additional interrupts arrive while the interrupt handler >> is active, those will have to be acknowledged separately. >> >> Sure, there is a risk that an interrupt arrives while the handler is >> running, and that it is handled but not acknowledged. That can happen >> with pretty much all interrupt handlers, and there are mitigations to >> limit the impact (for example, read the interrupt status register in >> a loop until no more interrupts are pending). But acknowledging >> an interrupt that was possibly not handled is always bad idea. > > Well, that's generally right but not always. Sometimes that depends on > hardware and Aspeed I2C is the case. > > This is a description from Aspeed AST2500 datasheet: > I2CD10 Interrupt Status Register > bit 2 Receive Done Interrupt status > S/W needs to clear this status bit to allow next data receiving. > > It means, driver should hold this bit to prevent transition of hardware > state machine until the driver handles received data, so the bit should > be cleared at the end of interrupt handler. > > Let me share my test result. Your code change works on 100KHz bus speed > but doesn't work well on 1MHz bus speed. Investigated that interrupt > handling is fast enough in 100KHz test but in 1MHz, most of data is > corrupted because the bit is cleared at the beginning of interrupt > handler so it allows receiving of the next data but the interrupt > handler isn't fast enough to read the data buffer on time. I checked > this problem on BMC-ME channel which ME sends lots of IPMB packets to > BMC at 1MHz speed. You could simply check the data corruption problem on > the BMC-ME channel. OK. > My thought is, the current code is right for real Aspeed I2C hardware. > It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the > actual Aspeed I2C hardware correctly. That might be very well possible yes. it also misses support for the slave mode and the DMA registers. Thanks for the info, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote: > On 9/11/2018 6:34 PM, Guenter Roeck wrote: >> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: >>> On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } >>> >>> My intention of putting the code at the end of interrupt handler was, >>> to reduce possibility of combined irq calls which is explained in this >>> patch. But YES, I agree with you. It could make a potential race >> >> Hmm, yes, but that doesn't explain why it would make sense to acknowledge >> the interrupt late. The interrupt ack only means "I am going to handle these >> interrupts". If additional interrupts arrive while the interrupt handler >> is active, those will have to be acknowledged separately. >> >> Sure, there is a risk that an interrupt arrives while the handler is >> running, and that it is handled but not acknowledged. That can happen >> with pretty much all interrupt handlers, and there are mitigations to >> limit the impact (for example, read the interrupt status register in >> a loop until no more interrupts are pending). But acknowledging >> an interrupt that was possibly not handled is always bad idea. > > Well, that's generally right but not always. Sometimes that depends on > hardware and Aspeed I2C is the case. > > This is a description from Aspeed AST2500 datasheet: > I2CD10 Interrupt Status Register > bit 2 Receive Done Interrupt status > S/W needs to clear this status bit to allow next data receiving. > > It means, driver should hold this bit to prevent transition of hardware > state machine until the driver handles received data, so the bit should > be cleared at the end of interrupt handler. > > Let me share my test result. Your code change works on 100KHz bus speed > but doesn't work well on 1MHz bus speed. Investigated that interrupt > handling is fast enough in 100KHz test but in 1MHz, most of data is > corrupted because the bit is cleared at the beginning of interrupt > handler so it allows receiving of the next data but the interrupt > handler isn't fast enough to read the data buffer on time. I checked > this problem on BMC-ME channel which ME sends lots of IPMB packets to > BMC at 1MHz speed. You could simply check the data corruption problem on > the BMC-ME channel. OK. > My thought is, the current code is right for real Aspeed I2C hardware. > It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the > actual Aspeed I2C hardware correctly. That might be very well possible yes. it also misses support for the slave mode and the DMA registers. Thanks for the info, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/12/2018 10:30 PM, Guenter Roeck wrote: > On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote: >> On 9/12/2018 12:58 PM, Guenter Roeck wrote: >>> On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: > On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: >> On 9/11/2018 4:33 PM, Guenter Roeck wrote: >>> Looking into the patch, clearing the interrupt status at the end of an >>> interrupt handler is always suspicious and tends to result in race >>> conditions (because additional interrupts may have arrived while >>> handling >>> the existing interrupts, or because interrupt handling itself may >>> trigger >>> another interrupt). With that in mind, the following patch fixes the >>> problem for me. >>> >>> Guenter >>> >>> --- >>> >>> diff --git a/drivers/i2c/busses/i2c-aspeed.c >>> b/drivers/i2c/busses/i2c-aspeed.c >>> index c258c4d9a4c0..c488e6950b7c 100644 >>> --- a/drivers/i2c/busses/i2c-aspeed.c >>> +++ b/drivers/i2c/busses/i2c-aspeed.c >>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void >>> *dev_id) >>> spin_lock(>lock); >>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); >>> + /* Ack all interrupt bits. */ >>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); >>> irq_remaining = irq_received; >>> #if IS_ENABLED(CONFIG_I2C_SLAVE) >>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void >>> *dev_id) >>> "irq handled != irq. expected 0x%08x, but was >>> 0x%08x\n", >>> irq_received, irq_handled); >>> - /* Ack all interrupt bits. */ >>> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); >>> spin_unlock(>lock); >>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; >>> } >>> >> >> My intention of putting the code at the end of interrupt handler was, >> to reduce possibility of combined irq calls which is explained in this >> patch. But YES, I agree with you. It could make a potential race > > Hmm, yes, but that doesn't explain why it would make sense to acknowledge > the interrupt late. The interrupt ack only means "I am going to handle > these > interrupts". If additional interrupts arrive while the interrupt handler > is active, those will have to be acknowledged separately. > > Sure, there is a risk that an interrupt arrives while the handler is > running, and that it is handled but not acknowledged. That can happen > with pretty much all interrupt handlers, and there are mitigations to > limit the impact (for example, read the interrupt status register in > a loop until no more interrupts are pending). But acknowledging > an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. >>> That makes sense. Does that apply to the other status bits as well ? >>> Reason for asking is that the current code actually gets stuck >>> in transmit, not receive. >>> >> Only bit 2 has that description in datasheet. Is slave config enabled >> for QEMU build? Does that get stuck in master sending or slave >> receiving? >> > qemu does not support slave mode. Linux gets stuck in master tx. > > I played with the code on both sides. I had to make changes in both > the linux kernel and in qemu to get the code to work again. > See attached. > > Guenter > > --- > Linux: > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index c258c4d9a4c0..3d518e09369f 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > > spin_lock(>lock); > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > + /* Ack all interrupts except for Rx done */ > + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, > +bus->base + ASPEED_I2C_INTR_STS_REG); > irq_remaining = irq_received; > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > irq_received,
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/12/2018 10:30 PM, Guenter Roeck wrote: > On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote: >> On 9/12/2018 12:58 PM, Guenter Roeck wrote: >>> On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: > On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: >> On 9/11/2018 4:33 PM, Guenter Roeck wrote: >>> Looking into the patch, clearing the interrupt status at the end of an >>> interrupt handler is always suspicious and tends to result in race >>> conditions (because additional interrupts may have arrived while >>> handling >>> the existing interrupts, or because interrupt handling itself may >>> trigger >>> another interrupt). With that in mind, the following patch fixes the >>> problem for me. >>> >>> Guenter >>> >>> --- >>> >>> diff --git a/drivers/i2c/busses/i2c-aspeed.c >>> b/drivers/i2c/busses/i2c-aspeed.c >>> index c258c4d9a4c0..c488e6950b7c 100644 >>> --- a/drivers/i2c/busses/i2c-aspeed.c >>> +++ b/drivers/i2c/busses/i2c-aspeed.c >>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void >>> *dev_id) >>> spin_lock(>lock); >>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); >>> + /* Ack all interrupt bits. */ >>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); >>> irq_remaining = irq_received; >>> #if IS_ENABLED(CONFIG_I2C_SLAVE) >>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void >>> *dev_id) >>> "irq handled != irq. expected 0x%08x, but was >>> 0x%08x\n", >>> irq_received, irq_handled); >>> - /* Ack all interrupt bits. */ >>> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); >>> spin_unlock(>lock); >>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; >>> } >>> >> >> My intention of putting the code at the end of interrupt handler was, >> to reduce possibility of combined irq calls which is explained in this >> patch. But YES, I agree with you. It could make a potential race > > Hmm, yes, but that doesn't explain why it would make sense to acknowledge > the interrupt late. The interrupt ack only means "I am going to handle > these > interrupts". If additional interrupts arrive while the interrupt handler > is active, those will have to be acknowledged separately. > > Sure, there is a risk that an interrupt arrives while the handler is > running, and that it is handled but not acknowledged. That can happen > with pretty much all interrupt handlers, and there are mitigations to > limit the impact (for example, read the interrupt status register in > a loop until no more interrupts are pending). But acknowledging > an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. >>> That makes sense. Does that apply to the other status bits as well ? >>> Reason for asking is that the current code actually gets stuck >>> in transmit, not receive. >>> >> Only bit 2 has that description in datasheet. Is slave config enabled >> for QEMU build? Does that get stuck in master sending or slave >> receiving? >> > qemu does not support slave mode. Linux gets stuck in master tx. > > I played with the code on both sides. I had to make changes in both > the linux kernel and in qemu to get the code to work again. > See attached. > > Guenter > > --- > Linux: > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index c258c4d9a4c0..3d518e09369f 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > > spin_lock(>lock); > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > + /* Ack all interrupts except for Rx done */ > + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, > +bus->base + ASPEED_I2C_INTR_STS_REG); > irq_remaining = irq_received; > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > irq_received,
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, Sep 12, 2018 at 03:31:06PM -0700, Jae Hyun Yoo wrote:
> >
> >I played with the code on both sides. I had to make changes in both
> >the linux kernel and in qemu to get the code to work again.
> >See attached.
> >
> >Guenter
> >
> >---
> >Linux:
> >
> >diff --git a/drivers/i2c/busses/i2c-aspeed.c
> >b/drivers/i2c/busses/i2c-aspeed.c
> >index c258c4d9a4c0..3d518e09369f 100644
> >--- a/drivers/i2c/busses/i2c-aspeed.c
> >+++ b/drivers/i2c/busses/i2c-aspeed.c
> >@@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> >*dev_id)
> > spin_lock(>lock);
> > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >+/* Ack all interrupts except for Rx done */
> >+writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> >+ bus->base + ASPEED_I2C_INTR_STS_REG);
> > irq_remaining = irq_received;
> > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >@@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> >*dev_id)
> > "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> > irq_received, irq_handled);
> >-/* Ack all interrupt bits. */
> >-writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >+/* Ack Rx done */
> >+if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> >+writel(ASPEED_I2CD_INTR_RX_DONE,
> >+ bus->base + ASPEED_I2C_INTR_STS_REG);
> > spin_unlock(>lock);
> > return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> > }
> >
> >---
> >qemu:
> >
> >diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> >index c762c73..0d4aa08 100644
> >--- a/hw/i2c/aspeed_i2c.c
> >+++ b/hw/i2c/aspeed_i2c.c
> >@@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
> > return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
> > }
> >+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
> >+{
> >+int ret;
> >+
> >+if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> >+return;
> >+}
> >+if (bus->intr_status & I2CD_INTR_RX_DONE) {
> >+return;
> >+}
> >+
> >+aspeed_i2c_set_state(bus, I2CD_MRXD);
> >+ret = i2c_recv(bus->bus);
> >+if (ret < 0) {
> >+qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> >+ret = 0xff;
> >+} else {
> >+bus->intr_status |= I2CD_INTR_RX_DONE;
> >+}
> >+bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> >+if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> >+i2c_nack(bus->bus);
> >+}
> >+bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> >+aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> >+}
> >+
> > /*
> > * The state machine needs some refinement. It is only used to track
> > * invalid STOP commands for the moment.
> >@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus,
> >uint64_t value)
> > {
> > bus->cmd &= ~0x;
> > bus->cmd |= value & 0x;
> >-bus->intr_status = 0;
> >+bus->intr_status &= I2CD_INTR_RX_DONE;
> > if (bus->cmd & I2CD_M_START_CMD) {
> > uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> >@@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
> >*bus, uint64_t value)
> > }
> > if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> >-int ret;
> >-
> >-aspeed_i2c_set_state(bus, I2CD_MRXD);
> >-ret = i2c_recv(bus->bus);
> >-if (ret < 0) {
> >-qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> >-ret = 0xff;
> >-} else {
> >-bus->intr_status |= I2CD_INTR_RX_DONE;
> >-}
> >-bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> >-if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> >-i2c_nack(bus->bus);
> >-}
> >-bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> >-aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> >+aspeed_i2c_handle_rx_cmd(bus);
> > }
> > if (bus->cmd & I2CD_M_STOP_CMD) {
> >@@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
> >offset,
> > uint64_t value, unsigned size)
> > {
> > AspeedI2CBus *bus = opaque;
> >+int status;
> > switch (offset) {
> > case I2CD_FUN_CTRL_REG:
> >@@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
> >offset,
> > bus->intr_ctrl = value & 0x7FFF;
> > break;
> > case I2CD_INTR_STS_REG:
> >+status = bus->intr_status;
> > bus->intr_status &= ~(value & 0x7FFF);
> >-bus->controller->intr_status &= ~(1 << bus->id);
> >-qemu_irq_lower(bus->controller->irq);
> >+if (!bus->intr_status) {
> >+bus->controller->intr_status &= ~(1 << bus->id);
> >+qemu_irq_lower(bus->controller->irq);
> >+}
> >+if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status &
>
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, Sep 12, 2018 at 03:31:06PM -0700, Jae Hyun Yoo wrote:
> >
> >I played with the code on both sides. I had to make changes in both
> >the linux kernel and in qemu to get the code to work again.
> >See attached.
> >
> >Guenter
> >
> >---
> >Linux:
> >
> >diff --git a/drivers/i2c/busses/i2c-aspeed.c
> >b/drivers/i2c/busses/i2c-aspeed.c
> >index c258c4d9a4c0..3d518e09369f 100644
> >--- a/drivers/i2c/busses/i2c-aspeed.c
> >+++ b/drivers/i2c/busses/i2c-aspeed.c
> >@@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> >*dev_id)
> > spin_lock(>lock);
> > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >+/* Ack all interrupts except for Rx done */
> >+writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> >+ bus->base + ASPEED_I2C_INTR_STS_REG);
> > irq_remaining = irq_received;
> > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >@@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> >*dev_id)
> > "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> > irq_received, irq_handled);
> >-/* Ack all interrupt bits. */
> >-writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >+/* Ack Rx done */
> >+if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> >+writel(ASPEED_I2CD_INTR_RX_DONE,
> >+ bus->base + ASPEED_I2C_INTR_STS_REG);
> > spin_unlock(>lock);
> > return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> > }
> >
> >---
> >qemu:
> >
> >diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> >index c762c73..0d4aa08 100644
> >--- a/hw/i2c/aspeed_i2c.c
> >+++ b/hw/i2c/aspeed_i2c.c
> >@@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
> > return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
> > }
> >+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
> >+{
> >+int ret;
> >+
> >+if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> >+return;
> >+}
> >+if (bus->intr_status & I2CD_INTR_RX_DONE) {
> >+return;
> >+}
> >+
> >+aspeed_i2c_set_state(bus, I2CD_MRXD);
> >+ret = i2c_recv(bus->bus);
> >+if (ret < 0) {
> >+qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> >+ret = 0xff;
> >+} else {
> >+bus->intr_status |= I2CD_INTR_RX_DONE;
> >+}
> >+bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> >+if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> >+i2c_nack(bus->bus);
> >+}
> >+bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> >+aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> >+}
> >+
> > /*
> > * The state machine needs some refinement. It is only used to track
> > * invalid STOP commands for the moment.
> >@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus,
> >uint64_t value)
> > {
> > bus->cmd &= ~0x;
> > bus->cmd |= value & 0x;
> >-bus->intr_status = 0;
> >+bus->intr_status &= I2CD_INTR_RX_DONE;
> > if (bus->cmd & I2CD_M_START_CMD) {
> > uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> >@@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus
> >*bus, uint64_t value)
> > }
> > if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> >-int ret;
> >-
> >-aspeed_i2c_set_state(bus, I2CD_MRXD);
> >-ret = i2c_recv(bus->bus);
> >-if (ret < 0) {
> >-qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> >-ret = 0xff;
> >-} else {
> >-bus->intr_status |= I2CD_INTR_RX_DONE;
> >-}
> >-bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> >-if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> >-i2c_nack(bus->bus);
> >-}
> >-bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> >-aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> >+aspeed_i2c_handle_rx_cmd(bus);
> > }
> > if (bus->cmd & I2CD_M_STOP_CMD) {
> >@@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
> >offset,
> > uint64_t value, unsigned size)
> > {
> > AspeedI2CBus *bus = opaque;
> >+int status;
> > switch (offset) {
> > case I2CD_FUN_CTRL_REG:
> >@@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr
> >offset,
> > bus->intr_ctrl = value & 0x7FFF;
> > break;
> > case I2CD_INTR_STS_REG:
> >+status = bus->intr_status;
> > bus->intr_status &= ~(value & 0x7FFF);
> >-bus->controller->intr_status &= ~(1 << bus->id);
> >-qemu_irq_lower(bus->controller->irq);
> >+if (!bus->intr_status) {
> >+bus->controller->intr_status &= ~(1 << bus->id);
> >+qemu_irq_lower(bus->controller->irq);
> >+}
> >+if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status &
>
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/12/2018 1:30 PM, Guenter Roeck wrote: On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote: On 9/12/2018 12:58 PM, Guenter Roeck wrote: On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. That makes sense. Does that apply to the other status bits as well ? Reason for asking is that the current code actually gets stuck in transmit, not receive. Only bit 2 has that description in datasheet. Is slave config enabled for QEMU build? Does that get stuck in master sending or slave receiving? qemu does not support slave mode. Linux gets stuck in master tx. I played with the code on both sides. I had to make changes in both the linux kernel and in qemu to get the code to work again. See attached. Guenter --- Linux: diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..3d518e09369f 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupts except for Rx done */ + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, + bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack Rx done */ + if (irq_received & ASPEED_I2CD_INTR_RX_DONE) + writel(ASPEED_I2CD_INTR_RX_DONE, + bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } --- qemu: diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index c762c73..0d4aa08 100644 --- a/hw/i2c/aspeed_i2c.c +++ b/hw/i2c/aspeed_i2c.c @@
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/12/2018 1:30 PM, Guenter Roeck wrote: On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote: On 9/12/2018 12:58 PM, Guenter Roeck wrote: On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. That makes sense. Does that apply to the other status bits as well ? Reason for asking is that the current code actually gets stuck in transmit, not receive. Only bit 2 has that description in datasheet. Is slave config enabled for QEMU build? Does that get stuck in master sending or slave receiving? qemu does not support slave mode. Linux gets stuck in master tx. I played with the code on both sides. I had to make changes in both the linux kernel and in qemu to get the code to work again. See attached. Guenter --- Linux: diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..3d518e09369f 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupts except for Rx done */ + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, + bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack Rx done */ + if (irq_received & ASPEED_I2CD_INTR_RX_DONE) + writel(ASPEED_I2CD_INTR_RX_DONE, + bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } --- qemu: diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index c762c73..0d4aa08 100644 --- a/hw/i2c/aspeed_i2c.c +++ b/hw/i2c/aspeed_i2c.c @@
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote: > On 9/12/2018 12:58 PM, Guenter Roeck wrote: > >On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: > >>On 9/11/2018 6:34 PM, Guenter Roeck wrote: > >>>On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 4:33 PM, Guenter Roeck wrote: > >Looking into the patch, clearing the interrupt status at the end of an > >interrupt handler is always suspicious and tends to result in race > >conditions (because additional interrupts may have arrived while handling > >the existing interrupts, or because interrupt handling itself may trigger > >another interrupt). With that in mind, the following patch fixes the > >problem for me. > > > >Guenter > > > >--- > > > >diff --git a/drivers/i2c/busses/i2c-aspeed.c > >b/drivers/i2c/busses/i2c-aspeed.c > >index c258c4d9a4c0..c488e6950b7c 100644 > >--- a/drivers/i2c/busses/i2c-aspeed.c > >+++ b/drivers/i2c/busses/i2c-aspeed.c > >@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >*dev_id) > > spin_lock(>lock); > > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > >+/* Ack all interrupt bits. */ > >+writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > > irq_remaining = irq_received; > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > >@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >*dev_id) > > "irq handled != irq. expected 0x%08x, but was > > 0x%08x\n", > > irq_received, irq_handled); > >-/* Ack all interrupt bits. */ > >-writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > > spin_unlock(>lock); > > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > > } > > > > My intention of putting the code at the end of interrupt handler was, > to reduce possibility of combined irq calls which is explained in this > patch. But YES, I agree with you. It could make a potential race > >>> > >>>Hmm, yes, but that doesn't explain why it would make sense to acknowledge > >>>the interrupt late. The interrupt ack only means "I am going to handle > >>>these > >>>interrupts". If additional interrupts arrive while the interrupt handler > >>>is active, those will have to be acknowledged separately. > >>> > >>>Sure, there is a risk that an interrupt arrives while the handler is > >>>running, and that it is handled but not acknowledged. That can happen > >>>with pretty much all interrupt handlers, and there are mitigations to > >>>limit the impact (for example, read the interrupt status register in > >>>a loop until no more interrupts are pending). But acknowledging > >>>an interrupt that was possibly not handled is always bad idea. > >> > >>Well, that's generally right but not always. Sometimes that depends on > >>hardware and Aspeed I2C is the case. > >> > >>This is a description from Aspeed AST2500 datasheet: > >> I2CD10 Interrupt Status Register > >> bit 2 Receive Done Interrupt status > >> S/W needs to clear this status bit to allow next data receiving. > >> > >>It means, driver should hold this bit to prevent transition of hardware > >>state machine until the driver handles received data, so the bit should > >>be cleared at the end of interrupt handler. > >> > >That makes sense. Does that apply to the other status bits as well ? > >Reason for asking is that the current code actually gets stuck > >in transmit, not receive. > > > Only bit 2 has that description in datasheet. Is slave config enabled > for QEMU build? Does that get stuck in master sending or slave > receiving? > qemu does not support slave mode. Linux gets stuck in master tx. I played with the code on both sides. I had to make changes in both the linux kernel and in qemu to get the code to work again. See attached. Guenter --- Linux: diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..3d518e09369f 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupts except for Rx done */ + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, + bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack Rx done */ + if
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote: > On 9/12/2018 12:58 PM, Guenter Roeck wrote: > >On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: > >>On 9/11/2018 6:34 PM, Guenter Roeck wrote: > >>>On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 4:33 PM, Guenter Roeck wrote: > >Looking into the patch, clearing the interrupt status at the end of an > >interrupt handler is always suspicious and tends to result in race > >conditions (because additional interrupts may have arrived while handling > >the existing interrupts, or because interrupt handling itself may trigger > >another interrupt). With that in mind, the following patch fixes the > >problem for me. > > > >Guenter > > > >--- > > > >diff --git a/drivers/i2c/busses/i2c-aspeed.c > >b/drivers/i2c/busses/i2c-aspeed.c > >index c258c4d9a4c0..c488e6950b7c 100644 > >--- a/drivers/i2c/busses/i2c-aspeed.c > >+++ b/drivers/i2c/busses/i2c-aspeed.c > >@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >*dev_id) > > spin_lock(>lock); > > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > >+/* Ack all interrupt bits. */ > >+writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > > irq_remaining = irq_received; > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > >@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >*dev_id) > > "irq handled != irq. expected 0x%08x, but was > > 0x%08x\n", > > irq_received, irq_handled); > >-/* Ack all interrupt bits. */ > >-writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > > spin_unlock(>lock); > > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > > } > > > > My intention of putting the code at the end of interrupt handler was, > to reduce possibility of combined irq calls which is explained in this > patch. But YES, I agree with you. It could make a potential race > >>> > >>>Hmm, yes, but that doesn't explain why it would make sense to acknowledge > >>>the interrupt late. The interrupt ack only means "I am going to handle > >>>these > >>>interrupts". If additional interrupts arrive while the interrupt handler > >>>is active, those will have to be acknowledged separately. > >>> > >>>Sure, there is a risk that an interrupt arrives while the handler is > >>>running, and that it is handled but not acknowledged. That can happen > >>>with pretty much all interrupt handlers, and there are mitigations to > >>>limit the impact (for example, read the interrupt status register in > >>>a loop until no more interrupts are pending). But acknowledging > >>>an interrupt that was possibly not handled is always bad idea. > >> > >>Well, that's generally right but not always. Sometimes that depends on > >>hardware and Aspeed I2C is the case. > >> > >>This is a description from Aspeed AST2500 datasheet: > >> I2CD10 Interrupt Status Register > >> bit 2 Receive Done Interrupt status > >> S/W needs to clear this status bit to allow next data receiving. > >> > >>It means, driver should hold this bit to prevent transition of hardware > >>state machine until the driver handles received data, so the bit should > >>be cleared at the end of interrupt handler. > >> > >That makes sense. Does that apply to the other status bits as well ? > >Reason for asking is that the current code actually gets stuck > >in transmit, not receive. > > > Only bit 2 has that description in datasheet. Is slave config enabled > for QEMU build? Does that get stuck in master sending or slave > receiving? > qemu does not support slave mode. Linux gets stuck in master tx. I played with the code on both sides. I had to make changes in both the linux kernel and in qemu to get the code to work again. See attached. Guenter --- Linux: diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..3d518e09369f 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupts except for Rx done */ + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, + bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack Rx done */ + if
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/12/2018 12:58 PM, Guenter Roeck wrote: On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. That makes sense. Does that apply to the other status bits as well ? Reason for asking is that the current code actually gets stuck in transmit, not receive. Only bit 2 has that description in datasheet. Is slave config enabled for QEMU build? Does that get stuck in master sending or slave receiving? Thanks, Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/12/2018 12:58 PM, Guenter Roeck wrote: On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. That makes sense. Does that apply to the other status bits as well ? Reason for asking is that the current code actually gets stuck in transmit, not receive. Only bit 2 has that description in datasheet. Is slave config enabled for QEMU build? Does that get stuck in master sending or slave receiving? Thanks, Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 6:34 PM, Guenter Roeck wrote: > >On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: > >>On 9/11/2018 4:33 PM, Guenter Roeck wrote: > >>>Looking into the patch, clearing the interrupt status at the end of an > >>>interrupt handler is always suspicious and tends to result in race > >>>conditions (because additional interrupts may have arrived while handling > >>>the existing interrupts, or because interrupt handling itself may trigger > >>>another interrupt). With that in mind, the following patch fixes the > >>>problem for me. > >>> > >>>Guenter > >>> > >>>--- > >>> > >>>diff --git a/drivers/i2c/busses/i2c-aspeed.c > >>>b/drivers/i2c/busses/i2c-aspeed.c > >>>index c258c4d9a4c0..c488e6950b7c 100644 > >>>--- a/drivers/i2c/busses/i2c-aspeed.c > >>>+++ b/drivers/i2c/busses/i2c-aspeed.c > >>>@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >>>*dev_id) > >>> spin_lock(>lock); > >>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > >>>+ /* Ack all interrupt bits. */ > >>>+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > >>> irq_remaining = irq_received; > >>> #if IS_ENABLED(CONFIG_I2C_SLAVE) > >>>@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >>>*dev_id) > >>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > >>> irq_received, irq_handled); > >>>- /* Ack all interrupt bits. */ > >>>- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > >>> spin_unlock(>lock); > >>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > >>> } > >>> > >> > >>My intention of putting the code at the end of interrupt handler was, > >>to reduce possibility of combined irq calls which is explained in this > >>patch. But YES, I agree with you. It could make a potential race > > > >Hmm, yes, but that doesn't explain why it would make sense to acknowledge > >the interrupt late. The interrupt ack only means "I am going to handle these > >interrupts". If additional interrupts arrive while the interrupt handler > >is active, those will have to be acknowledged separately. > > > >Sure, there is a risk that an interrupt arrives while the handler is > >running, and that it is handled but not acknowledged. That can happen > >with pretty much all interrupt handlers, and there are mitigations to > >limit the impact (for example, read the interrupt status register in > >a loop until no more interrupts are pending). But acknowledging > >an interrupt that was possibly not handled is always bad idea. > > Well, that's generally right but not always. Sometimes that depends on > hardware and Aspeed I2C is the case. > > This is a description from Aspeed AST2500 datasheet: > I2CD10 Interrupt Status Register > bit 2 Receive Done Interrupt status > S/W needs to clear this status bit to allow next data receiving. > > It means, driver should hold this bit to prevent transition of hardware > state machine until the driver handles received data, so the bit should > be cleared at the end of interrupt handler. > That makes sense. Does that apply to the other status bits as well ? Reason for asking is that the current code actually gets stuck in transmit, not receive. Thanks, Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 6:34 PM, Guenter Roeck wrote: > >On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: > >>On 9/11/2018 4:33 PM, Guenter Roeck wrote: > >>>Looking into the patch, clearing the interrupt status at the end of an > >>>interrupt handler is always suspicious and tends to result in race > >>>conditions (because additional interrupts may have arrived while handling > >>>the existing interrupts, or because interrupt handling itself may trigger > >>>another interrupt). With that in mind, the following patch fixes the > >>>problem for me. > >>> > >>>Guenter > >>> > >>>--- > >>> > >>>diff --git a/drivers/i2c/busses/i2c-aspeed.c > >>>b/drivers/i2c/busses/i2c-aspeed.c > >>>index c258c4d9a4c0..c488e6950b7c 100644 > >>>--- a/drivers/i2c/busses/i2c-aspeed.c > >>>+++ b/drivers/i2c/busses/i2c-aspeed.c > >>>@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >>>*dev_id) > >>> spin_lock(>lock); > >>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > >>>+ /* Ack all interrupt bits. */ > >>>+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > >>> irq_remaining = irq_received; > >>> #if IS_ENABLED(CONFIG_I2C_SLAVE) > >>>@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >>>*dev_id) > >>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > >>> irq_received, irq_handled); > >>>- /* Ack all interrupt bits. */ > >>>- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > >>> spin_unlock(>lock); > >>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > >>> } > >>> > >> > >>My intention of putting the code at the end of interrupt handler was, > >>to reduce possibility of combined irq calls which is explained in this > >>patch. But YES, I agree with you. It could make a potential race > > > >Hmm, yes, but that doesn't explain why it would make sense to acknowledge > >the interrupt late. The interrupt ack only means "I am going to handle these > >interrupts". If additional interrupts arrive while the interrupt handler > >is active, those will have to be acknowledged separately. > > > >Sure, there is a risk that an interrupt arrives while the handler is > >running, and that it is handled but not acknowledged. That can happen > >with pretty much all interrupt handlers, and there are mitigations to > >limit the impact (for example, read the interrupt status register in > >a loop until no more interrupts are pending). But acknowledging > >an interrupt that was possibly not handled is always bad idea. > > Well, that's generally right but not always. Sometimes that depends on > hardware and Aspeed I2C is the case. > > This is a description from Aspeed AST2500 datasheet: > I2CD10 Interrupt Status Register > bit 2 Receive Done Interrupt status > S/W needs to clear this status bit to allow next data receiving. > > It means, driver should hold this bit to prevent transition of hardware > state machine until the driver handles received data, so the bit should > be cleared at the end of interrupt handler. > That makes sense. Does that apply to the other status bits as well ? Reason for asking is that the current code actually gets stuck in transmit, not receive. Thanks, Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. Let me share my test result. Your code change works on 100KHz bus speed but doesn't work well on 1MHz bus speed. Investigated that interrupt handling is fast enough in 100KHz test but in 1MHz, most of data is corrupted because the bit is cleared at the beginning of interrupt handler so it allows receiving of the next data but the interrupt handler isn't fast enough to read the data buffer on time. I checked this problem on BMC-ME channel which ME sends lots of IPMB packets to BMC at 1MHz speed. You could simply check the data corruption problem on the BMC-ME channel. My thought is, the current code is right for real Aspeed I2C hardware. It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the actual Aspeed I2C hardware correctly. Thanks, Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/11/2018 6:34 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Well, that's generally right but not always. Sometimes that depends on hardware and Aspeed I2C is the case. This is a description from Aspeed AST2500 datasheet: I2CD10 Interrupt Status Register bit 2 Receive Done Interrupt status S/W needs to clear this status bit to allow next data receiving. It means, driver should hold this bit to prevent transition of hardware state machine until the driver handles received data, so the bit should be cleared at the end of interrupt handler. Let me share my test result. Your code change works on 100KHz bus speed but doesn't work well on 1MHz bus speed. Investigated that interrupt handling is fast enough in 100KHz test but in 1MHz, most of data is corrupted because the bit is cleared at the beginning of interrupt handler so it allows receiving of the next data but the interrupt handler isn't fast enough to read the data buffer on time. I checked this problem on BMC-ME channel which ME sends lots of IPMB packets to BMC at 1MHz speed. You could simply check the data corruption problem on the BMC-ME channel. My thought is, the current code is right for real Aspeed I2C hardware. It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the actual Aspeed I2C hardware correctly. Thanks, Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/12/2018 01:33 AM, Guenter Roeck wrote: > On Wed, Sep 12, 2018 at 08:23:29AM +0930, Joel Stanley wrote: >> On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo >> wrote: >>> >>> On 9/11/2018 1:41 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: >> > I checked this patch again but it doesn't have any change that could > affect to the probing flow. I'll debug the issue on qemu 3.0 environment > and will share if I find something. > The problem may be that qemu and the new code disagree how interrupts should be generated and handled, and the new code does not handle the interrupts it receives from the simulated hardware. This will result in i2c device probe failure, which in turn can cause all kinds of problems. >>> >>> Yes, that makes sense. Looks like it should be reverted until the issue >>> is fixed. Will submit a patch to revert it. >> >> Let's not rush. The qemu model was written in order to allow us to >> test the kernel code, and was validated by the kernel driver we have. >> We've had situations in the past (with the i2c driver in fact) where a >> change in the driver required an update of the model to be more >> accurate. >> >> I suggest we wait until Cedric has a chance to look at the issue >> before reverting the patch. >> > > Looking into the patch, clearing the interrupt status at the end of an > interrupt handler is always suspicious and tends to result in race yes. That happened in the past with the I2C aspeed driver. I can not find the thread anymore but we had to move up the ack of the interrupts. QEMU tends to be much faster to fire interrupts than real HW. > conditions (because additional interrupts may have arrived while handling > the existing interrupts, or because interrupt handling itself may trigger > another interrupt). With that in mind, the following patch fixes the > problem for me. Acked-by: Cédric Le Goater Thanks, C. > Guenter > > --- > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index c258c4d9a4c0..c488e6950b7c 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > > spin_lock(>lock); > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > + /* Ack all interrupt bits. */ > + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > irq_remaining = irq_received; > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > irq_received, irq_handled); > > - /* Ack all interrupt bits. */ > - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > spin_unlock(>lock); > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > } >
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/12/2018 01:33 AM, Guenter Roeck wrote: > On Wed, Sep 12, 2018 at 08:23:29AM +0930, Joel Stanley wrote: >> On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo >> wrote: >>> >>> On 9/11/2018 1:41 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: >> > I checked this patch again but it doesn't have any change that could > affect to the probing flow. I'll debug the issue on qemu 3.0 environment > and will share if I find something. > The problem may be that qemu and the new code disagree how interrupts should be generated and handled, and the new code does not handle the interrupts it receives from the simulated hardware. This will result in i2c device probe failure, which in turn can cause all kinds of problems. >>> >>> Yes, that makes sense. Looks like it should be reverted until the issue >>> is fixed. Will submit a patch to revert it. >> >> Let's not rush. The qemu model was written in order to allow us to >> test the kernel code, and was validated by the kernel driver we have. >> We've had situations in the past (with the i2c driver in fact) where a >> change in the driver required an update of the model to be more >> accurate. >> >> I suggest we wait until Cedric has a chance to look at the issue >> before reverting the patch. >> > > Looking into the patch, clearing the interrupt status at the end of an > interrupt handler is always suspicious and tends to result in race yes. That happened in the past with the I2C aspeed driver. I can not find the thread anymore but we had to move up the ack of the interrupts. QEMU tends to be much faster to fire interrupts than real HW. > conditions (because additional interrupts may have arrived while handling > the existing interrupts, or because interrupt handling itself may trigger > another interrupt). With that in mind, the following patch fixes the > problem for me. Acked-by: Cédric Le Goater Thanks, C. > Guenter > > --- > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index c258c4d9a4c0..c488e6950b7c 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > > spin_lock(>lock); > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > + /* Ack all interrupt bits. */ > + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > irq_remaining = irq_received; > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > irq_received, irq_handled); > > - /* Ack all interrupt bits. */ > - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > spin_unlock(>lock); > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > } >
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 4:33 PM, Guenter Roeck wrote: > >Looking into the patch, clearing the interrupt status at the end of an > >interrupt handler is always suspicious and tends to result in race > >conditions (because additional interrupts may have arrived while handling > >the existing interrupts, or because interrupt handling itself may trigger > >another interrupt). With that in mind, the following patch fixes the > >problem for me. > > > >Guenter > > > >--- > > > >diff --git a/drivers/i2c/busses/i2c-aspeed.c > >b/drivers/i2c/busses/i2c-aspeed.c > >index c258c4d9a4c0..c488e6950b7c 100644 > >--- a/drivers/i2c/busses/i2c-aspeed.c > >+++ b/drivers/i2c/busses/i2c-aspeed.c > >@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >*dev_id) > > spin_lock(>lock); > > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > >+/* Ack all interrupt bits. */ > >+writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > > irq_remaining = irq_received; > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > >@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >*dev_id) > > "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > > irq_received, irq_handled); > >-/* Ack all interrupt bits. */ > >-writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > > spin_unlock(>lock); > > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > > } > > > > My intention of putting the code at the end of interrupt handler was, > to reduce possibility of combined irq calls which is explained in this > patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Thanks, Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 4:33 PM, Guenter Roeck wrote: > >Looking into the patch, clearing the interrupt status at the end of an > >interrupt handler is always suspicious and tends to result in race > >conditions (because additional interrupts may have arrived while handling > >the existing interrupts, or because interrupt handling itself may trigger > >another interrupt). With that in mind, the following patch fixes the > >problem for me. > > > >Guenter > > > >--- > > > >diff --git a/drivers/i2c/busses/i2c-aspeed.c > >b/drivers/i2c/busses/i2c-aspeed.c > >index c258c4d9a4c0..c488e6950b7c 100644 > >--- a/drivers/i2c/busses/i2c-aspeed.c > >+++ b/drivers/i2c/busses/i2c-aspeed.c > >@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >*dev_id) > > spin_lock(>lock); > > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > >+/* Ack all interrupt bits. */ > >+writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > > irq_remaining = irq_received; > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > >@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > >*dev_id) > > "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > > irq_received, irq_handled); > >-/* Ack all interrupt bits. */ > >-writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); > > spin_unlock(>lock); > > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > > } > > > > My intention of putting the code at the end of interrupt handler was, > to reduce possibility of combined irq calls which is explained in this > patch. But YES, I agree with you. It could make a potential race Hmm, yes, but that doesn't explain why it would make sense to acknowledge the interrupt late. The interrupt ack only means "I am going to handle these interrupts". If additional interrupts arrive while the interrupt handler is active, those will have to be acknowledged separately. Sure, there is a risk that an interrupt arrives while the handler is running, and that it is handled but not acknowledged. That can happen with pretty much all interrupt handlers, and there are mitigations to limit the impact (for example, read the interrupt status register in a loop until no more interrupts are pending). But acknowledging an interrupt that was possibly not handled is always bad idea. Thanks, Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race condition as you pointed out. I tested your code change and checked that it works well. Let me take more sufficient test on real H/W. Will share the test result. Thanks a lot! Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/11/2018 4:33 PM, Guenter Roeck wrote: Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } My intention of putting the code at the end of interrupt handler was, to reduce possibility of combined irq calls which is explained in this patch. But YES, I agree with you. It could make a potential race condition as you pointed out. I tested your code change and checked that it works well. Let me take more sufficient test on real H/W. Will share the test result. Thanks a lot! Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, Sep 12, 2018 at 08:23:29AM +0930, Joel Stanley wrote: > On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo > wrote: > > > > On 9/11/2018 1:41 PM, Guenter Roeck wrote: > > > On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: > > > >> I checked this patch again but it doesn't have any change that could > > >> affect to the probing flow. I'll debug the issue on qemu 3.0 environment > > >> and will share if I find something. > > >> > > > The problem may be that qemu and the new code disagree how interrupts > > > should be generated and handled, and the new code does not handle the > > > interrupts it receives from the simulated hardware. This will result > > > in i2c device probe failure, which in turn can cause all kinds of > > > problems. > > > > > > > Yes, that makes sense. Looks like it should be reverted until the issue > > is fixed. Will submit a patch to revert it. > > Let's not rush. The qemu model was written in order to allow us to > test the kernel code, and was validated by the kernel driver we have. > We've had situations in the past (with the i2c driver in fact) where a > change in the driver required an update of the model to be more > accurate. > > I suggest we wait until Cedric has a chance to look at the issue > before reverting the patch. > Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; }
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, Sep 12, 2018 at 08:23:29AM +0930, Joel Stanley wrote: > On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo > wrote: > > > > On 9/11/2018 1:41 PM, Guenter Roeck wrote: > > > On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: > > > >> I checked this patch again but it doesn't have any change that could > > >> affect to the probing flow. I'll debug the issue on qemu 3.0 environment > > >> and will share if I find something. > > >> > > > The problem may be that qemu and the new code disagree how interrupts > > > should be generated and handled, and the new code does not handle the > > > interrupts it receives from the simulated hardware. This will result > > > in i2c device probe failure, which in turn can cause all kinds of > > > problems. > > > > > > > Yes, that makes sense. Looks like it should be reverted until the issue > > is fixed. Will submit a patch to revert it. > > Let's not rush. The qemu model was written in order to allow us to > test the kernel code, and was validated by the kernel driver we have. > We've had situations in the past (with the i2c driver in fact) where a > change in the driver required an update of the model to be more > accurate. > > I suggest we wait until Cedric has a chance to look at the issue > before reverting the patch. > Looking into the patch, clearing the interrupt status at the end of an interrupt handler is always suspicious and tends to result in race conditions (because additional interrupts may have arrived while handling the existing interrupts, or because interrupt handling itself may trigger another interrupt). With that in mind, the following patch fixes the problem for me. Guenter --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index c258c4d9a4c0..c488e6950b7c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) spin_lock(>lock); irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); + /* Ack all interrupt bits. */ + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); irq_remaining = irq_received; #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) "irq handled != irq. expected 0x%08x, but was 0x%08x\n", irq_received, irq_handled); - /* Ack all interrupt bits. */ - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG); spin_unlock(>lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; }
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo wrote: > > On 9/11/2018 1:41 PM, Guenter Roeck wrote: > > On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: > >> I checked this patch again but it doesn't have any change that could > >> affect to the probing flow. I'll debug the issue on qemu 3.0 environment > >> and will share if I find something. > >> > > The problem may be that qemu and the new code disagree how interrupts > > should be generated and handled, and the new code does not handle the > > interrupts it receives from the simulated hardware. This will result > > in i2c device probe failure, which in turn can cause all kinds of > > problems. > > > > Yes, that makes sense. Looks like it should be reverted until the issue > is fixed. Will submit a patch to revert it. Let's not rush. The qemu model was written in order to allow us to test the kernel code, and was validated by the kernel driver we have. We've had situations in the past (with the i2c driver in fact) where a change in the driver required an update of the model to be more accurate. I suggest we wait until Cedric has a chance to look at the issue before reverting the patch. Cheers, Joel
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo wrote: > > On 9/11/2018 1:41 PM, Guenter Roeck wrote: > > On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: > >> I checked this patch again but it doesn't have any change that could > >> affect to the probing flow. I'll debug the issue on qemu 3.0 environment > >> and will share if I find something. > >> > > The problem may be that qemu and the new code disagree how interrupts > > should be generated and handled, and the new code does not handle the > > interrupts it receives from the simulated hardware. This will result > > in i2c device probe failure, which in turn can cause all kinds of > > problems. > > > > Yes, that makes sense. Looks like it should be reverted until the issue > is fixed. Will submit a patch to revert it. Let's not rush. The qemu model was written in order to allow us to test the kernel code, and was validated by the kernel driver we have. We've had situations in the past (with the i2c driver in fact) where a change in the driver required an update of the model to be more accurate. I suggest we wait until Cedric has a chance to look at the issue before reverting the patch. Cheers, Joel
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/11/2018 1:41 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 11:37 AM, Guenter Roeck wrote: Hi, On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: In most of cases, interrupt bits are set one by one but there are also a lot of other cases that Aspeed I2C IP sends multiple interrupt bits with combining master and slave events using a single interrupt call. It happens much more in multi-master environment than single-master. For an example, when master is waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along with the NORMAL_STOP in case of an another master immediately sends data just after acquiring the bus. In this case, the NORMAL_STOP interrupt should be handled by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be handled by slave_irq. This commit modifies irq hadling logic to handle the master/slave combined events properly. Signed-off-by: Jae Hyun Yoo Reviewed-by: Brendan Higgins This patch causes a boot stall when booting witherspoon-bmc with qemu v3.0, and all i2c device probes fail with error -110 (timeout). Bisect log is attached for reference. With the same kernel configuration (aspeed_g5_defconfig), ast2500-evb and romulus-bmc are still able to boot. palmetto-bmc with aspeed_g4_defconfig also appears to work. Is this a problem with qemu ? Should I drop the qemu test for witherspoon-bmc starting with the next kernel release ? Thanks, Guenter Hi Guenter, Thanks for your report. I checked this patch again but it doesn't have any change that could affect to the probing flow. I'll debug the issue on qemu 3.0 environment and will share if I find something. The problem may be that qemu and the new code disagree how interrupts should be generated and handled, and the new code does not handle the interrupts it receives from the simulated hardware. This will result in i2c device probe failure, which in turn can cause all kinds of problems. Yes, that makes sense. Looks like it should be reverted until the issue is fixed. Will submit a patch to revert it. Thanks, Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/11/2018 1:41 PM, Guenter Roeck wrote: On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: On 9/11/2018 11:37 AM, Guenter Roeck wrote: Hi, On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: In most of cases, interrupt bits are set one by one but there are also a lot of other cases that Aspeed I2C IP sends multiple interrupt bits with combining master and slave events using a single interrupt call. It happens much more in multi-master environment than single-master. For an example, when master is waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along with the NORMAL_STOP in case of an another master immediately sends data just after acquiring the bus. In this case, the NORMAL_STOP interrupt should be handled by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be handled by slave_irq. This commit modifies irq hadling logic to handle the master/slave combined events properly. Signed-off-by: Jae Hyun Yoo Reviewed-by: Brendan Higgins This patch causes a boot stall when booting witherspoon-bmc with qemu v3.0, and all i2c device probes fail with error -110 (timeout). Bisect log is attached for reference. With the same kernel configuration (aspeed_g5_defconfig), ast2500-evb and romulus-bmc are still able to boot. palmetto-bmc with aspeed_g4_defconfig also appears to work. Is this a problem with qemu ? Should I drop the qemu test for witherspoon-bmc starting with the next kernel release ? Thanks, Guenter Hi Guenter, Thanks for your report. I checked this patch again but it doesn't have any change that could affect to the probing flow. I'll debug the issue on qemu 3.0 environment and will share if I find something. The problem may be that qemu and the new code disagree how interrupts should be generated and handled, and the new code does not handle the interrupts it receives from the simulated hardware. This will result in i2c device probe failure, which in turn can cause all kinds of problems. Yes, that makes sense. Looks like it should be reverted until the issue is fixed. Will submit a patch to revert it. Thanks, Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 11:37 AM, Guenter Roeck wrote: > >Hi, > > > >On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: > >>In most of cases, interrupt bits are set one by one but there are > >>also a lot of other cases that Aspeed I2C IP sends multiple > >>interrupt bits with combining master and slave events using a > >>single interrupt call. It happens much more in multi-master > >>environment than single-master. For an example, when master is > >>waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, > >>SLAVE_MATCH and RX_DONE interrupts could come along with the > >>NORMAL_STOP in case of an another master immediately sends data > >>just after acquiring the bus. In this case, the NORMAL_STOP > >>interrupt should be handled by master_irq and the SLAVE_MATCH and > >>RX_DONE interrupts should be handled by slave_irq. This commit > >>modifies irq hadling logic to handle the master/slave combined > >>events properly. > >> > >>Signed-off-by: Jae Hyun Yoo > >>Reviewed-by: Brendan Higgins > > > >This patch causes a boot stall when booting witherspoon-bmc with > >qemu v3.0, and all i2c device probes fail with error -110 (timeout). > >Bisect log is attached for reference. > > > >With the same kernel configuration (aspeed_g5_defconfig), > >ast2500-evb and romulus-bmc are still able to boot. > >palmetto-bmc with aspeed_g4_defconfig also appears to work. > > > >Is this a problem with qemu ? Should I drop the qemu test > >for witherspoon-bmc starting with the next kernel release ? > > > >Thanks, > >Guenter > > > > Hi Guenter, > > Thanks for your report. > > I checked this patch again but it doesn't have any change that could > affect to the probing flow. I'll debug the issue on qemu 3.0 environment > and will share if I find something. > The problem may be that qemu and the new code disagree how interrupts should be generated and handled, and the new code does not handle the interrupts it receives from the simulated hardware. This will result in i2c device probe failure, which in turn can cause all kinds of problems. Thanks, Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote: > On 9/11/2018 11:37 AM, Guenter Roeck wrote: > >Hi, > > > >On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: > >>In most of cases, interrupt bits are set one by one but there are > >>also a lot of other cases that Aspeed I2C IP sends multiple > >>interrupt bits with combining master and slave events using a > >>single interrupt call. It happens much more in multi-master > >>environment than single-master. For an example, when master is > >>waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, > >>SLAVE_MATCH and RX_DONE interrupts could come along with the > >>NORMAL_STOP in case of an another master immediately sends data > >>just after acquiring the bus. In this case, the NORMAL_STOP > >>interrupt should be handled by master_irq and the SLAVE_MATCH and > >>RX_DONE interrupts should be handled by slave_irq. This commit > >>modifies irq hadling logic to handle the master/slave combined > >>events properly. > >> > >>Signed-off-by: Jae Hyun Yoo > >>Reviewed-by: Brendan Higgins > > > >This patch causes a boot stall when booting witherspoon-bmc with > >qemu v3.0, and all i2c device probes fail with error -110 (timeout). > >Bisect log is attached for reference. > > > >With the same kernel configuration (aspeed_g5_defconfig), > >ast2500-evb and romulus-bmc are still able to boot. > >palmetto-bmc with aspeed_g4_defconfig also appears to work. > > > >Is this a problem with qemu ? Should I drop the qemu test > >for witherspoon-bmc starting with the next kernel release ? > > > >Thanks, > >Guenter > > > > Hi Guenter, > > Thanks for your report. > > I checked this patch again but it doesn't have any change that could > affect to the probing flow. I'll debug the issue on qemu 3.0 environment > and will share if I find something. > The problem may be that qemu and the new code disagree how interrupts should be generated and handled, and the new code does not handle the interrupts it receives from the simulated hardware. This will result in i2c device probe failure, which in turn can cause all kinds of problems. Thanks, Guenter
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/11/2018 11:37 AM, Guenter Roeck wrote: Hi, On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: In most of cases, interrupt bits are set one by one but there are also a lot of other cases that Aspeed I2C IP sends multiple interrupt bits with combining master and slave events using a single interrupt call. It happens much more in multi-master environment than single-master. For an example, when master is waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along with the NORMAL_STOP in case of an another master immediately sends data just after acquiring the bus. In this case, the NORMAL_STOP interrupt should be handled by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be handled by slave_irq. This commit modifies irq hadling logic to handle the master/slave combined events properly. Signed-off-by: Jae Hyun Yoo Reviewed-by: Brendan Higgins This patch causes a boot stall when booting witherspoon-bmc with qemu v3.0, and all i2c device probes fail with error -110 (timeout). Bisect log is attached for reference. With the same kernel configuration (aspeed_g5_defconfig), ast2500-evb and romulus-bmc are still able to boot. palmetto-bmc with aspeed_g4_defconfig also appears to work. Is this a problem with qemu ? Should I drop the qemu test for witherspoon-bmc starting with the next kernel release ? Thanks, Guenter Hi Guenter, Thanks for your report. I checked this patch again but it doesn't have any change that could affect to the probing flow. I'll debug the issue on qemu 3.0 environment and will share if I find something. Thanks, Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/11/2018 11:37 AM, Guenter Roeck wrote: Hi, On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: In most of cases, interrupt bits are set one by one but there are also a lot of other cases that Aspeed I2C IP sends multiple interrupt bits with combining master and slave events using a single interrupt call. It happens much more in multi-master environment than single-master. For an example, when master is waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along with the NORMAL_STOP in case of an another master immediately sends data just after acquiring the bus. In this case, the NORMAL_STOP interrupt should be handled by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be handled by slave_irq. This commit modifies irq hadling logic to handle the master/slave combined events properly. Signed-off-by: Jae Hyun Yoo Reviewed-by: Brendan Higgins This patch causes a boot stall when booting witherspoon-bmc with qemu v3.0, and all i2c device probes fail with error -110 (timeout). Bisect log is attached for reference. With the same kernel configuration (aspeed_g5_defconfig), ast2500-evb and romulus-bmc are still able to boot. palmetto-bmc with aspeed_g4_defconfig also appears to work. Is this a problem with qemu ? Should I drop the qemu test for witherspoon-bmc starting with the next kernel release ? Thanks, Guenter Hi Guenter, Thanks for your report. I checked this patch again but it doesn't have any change that could affect to the probing flow. I'll debug the issue on qemu 3.0 environment and will share if I find something. Thanks, Jae
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/11/2018 08:37 PM, Guenter Roeck wrote: > Hi, > > On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: >> In most of cases, interrupt bits are set one by one but there are >> also a lot of other cases that Aspeed I2C IP sends multiple >> interrupt bits with combining master and slave events using a >> single interrupt call. It happens much more in multi-master >> environment than single-master. For an example, when master is >> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, >> SLAVE_MATCH and RX_DONE interrupts could come along with the >> NORMAL_STOP in case of an another master immediately sends data >> just after acquiring the bus. In this case, the NORMAL_STOP >> interrupt should be handled by master_irq and the SLAVE_MATCH and >> RX_DONE interrupts should be handled by slave_irq. This commit >> modifies irq hadling logic to handle the master/slave combined >> events properly. >> >> Signed-off-by: Jae Hyun Yoo >> Reviewed-by: Brendan Higgins > > This patch causes a boot stall when booting witherspoon-bmc with > qemu v3.0, and all i2c device probes fail with error -110 (timeout). > Bisect log is attached for reference. > > With the same kernel configuration (aspeed_g5_defconfig), > ast2500-evb and romulus-bmc are still able to boot. > palmetto-bmc with aspeed_g4_defconfig also appears to work. > > Is this a problem with qemu ? Should I drop the qemu test > for witherspoon-bmc starting with the next kernel release ? The QEMU model for the Aspeed I2C controller does not support slave mode or DMA registers. That might be the issue. I will check what this patch does when I have sometime. Thanks, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 09/11/2018 08:37 PM, Guenter Roeck wrote: > Hi, > > On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: >> In most of cases, interrupt bits are set one by one but there are >> also a lot of other cases that Aspeed I2C IP sends multiple >> interrupt bits with combining master and slave events using a >> single interrupt call. It happens much more in multi-master >> environment than single-master. For an example, when master is >> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, >> SLAVE_MATCH and RX_DONE interrupts could come along with the >> NORMAL_STOP in case of an another master immediately sends data >> just after acquiring the bus. In this case, the NORMAL_STOP >> interrupt should be handled by master_irq and the SLAVE_MATCH and >> RX_DONE interrupts should be handled by slave_irq. This commit >> modifies irq hadling logic to handle the master/slave combined >> events properly. >> >> Signed-off-by: Jae Hyun Yoo >> Reviewed-by: Brendan Higgins > > This patch causes a boot stall when booting witherspoon-bmc with > qemu v3.0, and all i2c device probes fail with error -110 (timeout). > Bisect log is attached for reference. > > With the same kernel configuration (aspeed_g5_defconfig), > ast2500-evb and romulus-bmc are still able to boot. > palmetto-bmc with aspeed_g4_defconfig also appears to work. > > Is this a problem with qemu ? Should I drop the qemu test > for witherspoon-bmc starting with the next kernel release ? The QEMU model for the Aspeed I2C controller does not support slave mode or DMA registers. That might be the issue. I will check what this patch does when I have sometime. Thanks, C.
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
Hi, On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: > In most of cases, interrupt bits are set one by one but there are > also a lot of other cases that Aspeed I2C IP sends multiple > interrupt bits with combining master and slave events using a > single interrupt call. It happens much more in multi-master > environment than single-master. For an example, when master is > waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, > SLAVE_MATCH and RX_DONE interrupts could come along with the > NORMAL_STOP in case of an another master immediately sends data > just after acquiring the bus. In this case, the NORMAL_STOP > interrupt should be handled by master_irq and the SLAVE_MATCH and > RX_DONE interrupts should be handled by slave_irq. This commit > modifies irq hadling logic to handle the master/slave combined > events properly. > > Signed-off-by: Jae Hyun Yoo > Reviewed-by: Brendan Higgins This patch causes a boot stall when booting witherspoon-bmc with qemu v3.0, and all i2c device probes fail with error -110 (timeout). Bisect log is attached for reference. With the same kernel configuration (aspeed_g5_defconfig), ast2500-evb and romulus-bmc are still able to boot. palmetto-bmc with aspeed_g4_defconfig also appears to work. Is this a problem with qemu ? Should I drop the qemu test for witherspoon-bmc starting with the next kernel release ? Thanks, Guenter --- # bad: [09c0888767529cdb382f34452819e42d1a66a114] Add linux-next specific files for 20180911 # good: [11da3a7f84f19c26da6f86af878298694ede0804] Linux 4.19-rc3 git bisect start 'HEAD' 'v4.19-rc3' # bad: [a2ebc71cf97bed9b453318418e4a281434565e8b] Merge remote-tracking branch 'nfc-next/master' git bisect bad a2ebc71cf97bed9b453318418e4a281434565e8b # good: [6fde463b32bf4105c28c0a297a5b66aca5d6ecd4] Merge remote-tracking branch 's390/features' git bisect good 6fde463b32bf4105c28c0a297a5b66aca5d6ecd4 # bad: [136fd6d530a3ae0dd003984f683345cfe88c01f3] Merge remote-tracking branch 'v4l-dvb/master' git bisect bad 136fd6d530a3ae0dd003984f683345cfe88c01f3 # good: [c7ae95368af43c08f5f615b00f2f7bf2e9c45788] Merge remote-tracking branch 'v9fs/9p-next' git bisect good c7ae95368af43c08f5f615b00f2f7bf2e9c45788 # good: [4c640c41381e47b328c6507bcf534812761256cd] Merge branch 'for-4.19/fixes' into for-next git bisect good 4c640c41381e47b328c6507bcf534812761256cd # good: [5bc91f70c5ecc2bc5967b98ce7fa4e55ad230d99] Merge remote-tracking branch 'hid/for-next' git bisect good 5bc91f70c5ecc2bc5967b98ce7fa4e55ad230d99 # bad: [657b9d37406ed1625d469db0fd356e364dc75dd8] Merge remote-tracking branch 'hwmon-staging/hwmon-next' git bisect bad 657b9d37406ed1625d469db0fd356e364dc75dd8 # bad: [fc9f90ddace238716cfcbd00d51428ee8baa12c7] Merge branch 'i2c/for-current' into i2c/for-next git bisect bad fc9f90ddace238716cfcbd00d51428ee8baa12c7 # good: [34b7be301d4c5d85d1d093d2faf856f3d727416f] Merge branch 'i2c/for-current' into i2c/for-next git bisect good 34b7be301d4c5d85d1d093d2faf856f3d727416f # bad: [3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd] i2c: aspeed: Handle master/slave combined irq events properly git bisect bad 3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd # good: [fc66b39fe36acfd06f716e338de7cd8f9550fad2] i2c: mediatek: Use DMA safe buffers for i2c transactions git bisect good fc66b39fe36acfd06f716e338de7cd8f9550fad2 # first bad commit: [3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd] i2c: aspeed: Handle master/slave combined irq events properly
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
Hi, On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: > In most of cases, interrupt bits are set one by one but there are > also a lot of other cases that Aspeed I2C IP sends multiple > interrupt bits with combining master and slave events using a > single interrupt call. It happens much more in multi-master > environment than single-master. For an example, when master is > waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, > SLAVE_MATCH and RX_DONE interrupts could come along with the > NORMAL_STOP in case of an another master immediately sends data > just after acquiring the bus. In this case, the NORMAL_STOP > interrupt should be handled by master_irq and the SLAVE_MATCH and > RX_DONE interrupts should be handled by slave_irq. This commit > modifies irq hadling logic to handle the master/slave combined > events properly. > > Signed-off-by: Jae Hyun Yoo > Reviewed-by: Brendan Higgins This patch causes a boot stall when booting witherspoon-bmc with qemu v3.0, and all i2c device probes fail with error -110 (timeout). Bisect log is attached for reference. With the same kernel configuration (aspeed_g5_defconfig), ast2500-evb and romulus-bmc are still able to boot. palmetto-bmc with aspeed_g4_defconfig also appears to work. Is this a problem with qemu ? Should I drop the qemu test for witherspoon-bmc starting with the next kernel release ? Thanks, Guenter --- # bad: [09c0888767529cdb382f34452819e42d1a66a114] Add linux-next specific files for 20180911 # good: [11da3a7f84f19c26da6f86af878298694ede0804] Linux 4.19-rc3 git bisect start 'HEAD' 'v4.19-rc3' # bad: [a2ebc71cf97bed9b453318418e4a281434565e8b] Merge remote-tracking branch 'nfc-next/master' git bisect bad a2ebc71cf97bed9b453318418e4a281434565e8b # good: [6fde463b32bf4105c28c0a297a5b66aca5d6ecd4] Merge remote-tracking branch 's390/features' git bisect good 6fde463b32bf4105c28c0a297a5b66aca5d6ecd4 # bad: [136fd6d530a3ae0dd003984f683345cfe88c01f3] Merge remote-tracking branch 'v4l-dvb/master' git bisect bad 136fd6d530a3ae0dd003984f683345cfe88c01f3 # good: [c7ae95368af43c08f5f615b00f2f7bf2e9c45788] Merge remote-tracking branch 'v9fs/9p-next' git bisect good c7ae95368af43c08f5f615b00f2f7bf2e9c45788 # good: [4c640c41381e47b328c6507bcf534812761256cd] Merge branch 'for-4.19/fixes' into for-next git bisect good 4c640c41381e47b328c6507bcf534812761256cd # good: [5bc91f70c5ecc2bc5967b98ce7fa4e55ad230d99] Merge remote-tracking branch 'hid/for-next' git bisect good 5bc91f70c5ecc2bc5967b98ce7fa4e55ad230d99 # bad: [657b9d37406ed1625d469db0fd356e364dc75dd8] Merge remote-tracking branch 'hwmon-staging/hwmon-next' git bisect bad 657b9d37406ed1625d469db0fd356e364dc75dd8 # bad: [fc9f90ddace238716cfcbd00d51428ee8baa12c7] Merge branch 'i2c/for-current' into i2c/for-next git bisect bad fc9f90ddace238716cfcbd00d51428ee8baa12c7 # good: [34b7be301d4c5d85d1d093d2faf856f3d727416f] Merge branch 'i2c/for-current' into i2c/for-next git bisect good 34b7be301d4c5d85d1d093d2faf856f3d727416f # bad: [3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd] i2c: aspeed: Handle master/slave combined irq events properly git bisect bad 3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd # good: [fc66b39fe36acfd06f716e338de7cd8f9550fad2] i2c: mediatek: Use DMA safe buffers for i2c transactions git bisect good fc66b39fe36acfd06f716e338de7cd8f9550fad2 # first bad commit: [3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd] i2c: aspeed: Handle master/slave combined irq events properly
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: > In most of cases, interrupt bits are set one by one but there are > also a lot of other cases that Aspeed I2C IP sends multiple > interrupt bits with combining master and slave events using a > single interrupt call. It happens much more in multi-master > environment than single-master. For an example, when master is > waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, > SLAVE_MATCH and RX_DONE interrupts could come along with the > NORMAL_STOP in case of an another master immediately sends data > just after acquiring the bus. In this case, the NORMAL_STOP > interrupt should be handled by master_irq and the SLAVE_MATCH and > RX_DONE interrupts should be handled by slave_irq. This commit > modifies irq hadling logic to handle the master/slave combined > events properly. > > Signed-off-by: Jae Hyun Yoo Applied to for-next, thanks! signature.asc Description: PGP signature
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote: > In most of cases, interrupt bits are set one by one but there are > also a lot of other cases that Aspeed I2C IP sends multiple > interrupt bits with combining master and slave events using a > single interrupt call. It happens much more in multi-master > environment than single-master. For an example, when master is > waiting for a NORMAL_STOP interrupt in its MASTER_STOP state, > SLAVE_MATCH and RX_DONE interrupts could come along with the > NORMAL_STOP in case of an another master immediately sends data > just after acquiring the bus. In this case, the NORMAL_STOP > interrupt should be handled by master_irq and the SLAVE_MATCH and > RX_DONE interrupts should be handled by slave_irq. This commit > modifies irq hadling logic to handle the master/slave combined > events properly. > > Signed-off-by: Jae Hyun Yoo Applied to for-next, thanks! signature.asc Description: PGP signature
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
Please don't quote the whole message. Everyone else has to scroll down a lot to find the relevant information. Thanks! Will keep that in mind. Thanks Wolfram!
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
Please don't quote the whole message. Everyone else has to scroll down a lot to find the relevant information. Thanks! Will keep that in mind. Thanks Wolfram!
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
> > Looks awesome! Thanks! > > > > Reviewed-by: Brendan Higgins > > > > Thanks a lot for your review Brendan! Please don't quote the whole message. Everyone else has to scroll down a lot to find the relevant information. Thanks! signature.asc Description: PGP signature
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
> > Looks awesome! Thanks! > > > > Reviewed-by: Brendan Higgins > > > > Thanks a lot for your review Brendan! Please don't quote the whole message. Everyone else has to scroll down a lot to find the relevant information. Thanks! signature.asc Description: PGP signature
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/6/2018 10:26 AM, Brendan Higgins wrote:
On Thu, Aug 23, 2018 at 3:58 PM Jae Hyun Yoo
wrote:
In most of cases, interrupt bits are set one by one but there are
also a lot of other cases that Aspeed I2C IP sends multiple
interrupt bits with combining master and slave events using a
single interrupt call. It happens much more in multi-master
environment than single-master. For an example, when master is
waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
SLAVE_MATCH and RX_DONE interrupts could come along with the
NORMAL_STOP in case of an another master immediately sends data
just after acquiring the bus. In this case, the NORMAL_STOP
interrupt should be handled by master_irq and the SLAVE_MATCH and
RX_DONE interrupts should be handled by slave_irq. This commit
modifies irq hadling logic to handle the master/slave combined
events properly.
Signed-off-by: Jae Hyun Yoo
---
Changes since v5:
- Changed variable names in irq hanlders to represent proper meaning.
- Fixed an error printing message again to make it use the irq_handled variable.
Changes since v4:
- Fixed an error printing message that handlers didn't handle all interrupts.
Changes since v3:
- Fixed typos in a comment.
Changes since v2:
- Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
- Removed a member irq_status from the struct aspeed_i2c_bus and changed
master_irq and slave_irq handlers to make them return status_ack.
- Added a comment to explain why it needs to try both irq handlers.
Changes since v1:
- Fixed a grammar issue in commit message.
- Added a missing line feed character into a message printing.
drivers/i2c/busses/i2c-aspeed.c | 131 ++--
1 file changed, 76 insertions(+), 55 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a4f956c6d567..c258c4d9a4c0 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -82,6 +82,11 @@
#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
#define ASPEED_I2CD_INTR_TX_NAKBIT(1)
#define ASPEED_I2CD_INTR_TX_ACKBIT(0)
+#define ASPEED_I2CD_INTR_MASTER_ERRORS\
+ (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
+ASPEED_I2CD_INTR_SCL_TIMEOUT |\
+ASPEED_I2CD_INTR_ABNORMAL | \
+ASPEED_I2CD_INTR_ARBIT_LOSS)
#define ASPEED_I2CD_INTR_ALL
\
(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
\
ASPEED_I2CD_INTR_BUS_RECOVER_DONE |
\
@@ -227,32 +232,26 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus
*bus)
}
#if IS_ENABLED(CONFIG_I2C_SLAVE)
-static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, irq_handled = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return 0;
command = readl(bus->base + ASPEED_I2C_CMD_REG);
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
/* Slave was requested, restart state machine. */
if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
- status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+ irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
/* Slave is not currently active, irq was for someone else. */
- if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
- irq_handled = false;
- goto out;
- }
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
+ return irq_handled;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
irq_status, command);
@@ -269,31 +268,31 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus
*bus)
bus->slave_state =
ASPEED_I2C_SLAVE_WRITE_REQUESTED;
}
- status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+ irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
}
/* Slave was asked to stop. */
if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
- status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+ irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
- status_ack |=
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On 9/6/2018 10:26 AM, Brendan Higgins wrote:
On Thu, Aug 23, 2018 at 3:58 PM Jae Hyun Yoo
wrote:
In most of cases, interrupt bits are set one by one but there are
also a lot of other cases that Aspeed I2C IP sends multiple
interrupt bits with combining master and slave events using a
single interrupt call. It happens much more in multi-master
environment than single-master. For an example, when master is
waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
SLAVE_MATCH and RX_DONE interrupts could come along with the
NORMAL_STOP in case of an another master immediately sends data
just after acquiring the bus. In this case, the NORMAL_STOP
interrupt should be handled by master_irq and the SLAVE_MATCH and
RX_DONE interrupts should be handled by slave_irq. This commit
modifies irq hadling logic to handle the master/slave combined
events properly.
Signed-off-by: Jae Hyun Yoo
---
Changes since v5:
- Changed variable names in irq hanlders to represent proper meaning.
- Fixed an error printing message again to make it use the irq_handled variable.
Changes since v4:
- Fixed an error printing message that handlers didn't handle all interrupts.
Changes since v3:
- Fixed typos in a comment.
Changes since v2:
- Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
- Removed a member irq_status from the struct aspeed_i2c_bus and changed
master_irq and slave_irq handlers to make them return status_ack.
- Added a comment to explain why it needs to try both irq handlers.
Changes since v1:
- Fixed a grammar issue in commit message.
- Added a missing line feed character into a message printing.
drivers/i2c/busses/i2c-aspeed.c | 131 ++--
1 file changed, 76 insertions(+), 55 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a4f956c6d567..c258c4d9a4c0 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -82,6 +82,11 @@
#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
#define ASPEED_I2CD_INTR_TX_NAKBIT(1)
#define ASPEED_I2CD_INTR_TX_ACKBIT(0)
+#define ASPEED_I2CD_INTR_MASTER_ERRORS\
+ (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
+ASPEED_I2CD_INTR_SCL_TIMEOUT |\
+ASPEED_I2CD_INTR_ABNORMAL | \
+ASPEED_I2CD_INTR_ARBIT_LOSS)
#define ASPEED_I2CD_INTR_ALL
\
(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
\
ASPEED_I2CD_INTR_BUS_RECOVER_DONE |
\
@@ -227,32 +232,26 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus
*bus)
}
#if IS_ENABLED(CONFIG_I2C_SLAVE)
-static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, irq_handled = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return 0;
command = readl(bus->base + ASPEED_I2C_CMD_REG);
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
/* Slave was requested, restart state machine. */
if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
- status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+ irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
/* Slave is not currently active, irq was for someone else. */
- if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
- irq_handled = false;
- goto out;
- }
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
+ return irq_handled;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
irq_status, command);
@@ -269,31 +268,31 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus
*bus)
bus->slave_state =
ASPEED_I2C_SLAVE_WRITE_REQUESTED;
}
- status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+ irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
}
/* Slave was asked to stop. */
if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
- status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+ irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
- status_ack |=
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Thu, Aug 23, 2018 at 3:58 PM Jae Hyun Yoo
wrote:
>
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much more in multi-master
> environment than single-master. For an example, when master is
> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
> SLAVE_MATCH and RX_DONE interrupts could come along with the
> NORMAL_STOP in case of an another master immediately sends data
> just after acquiring the bus. In this case, the NORMAL_STOP
> interrupt should be handled by master_irq and the SLAVE_MATCH and
> RX_DONE interrupts should be handled by slave_irq. This commit
> modifies irq hadling logic to handle the master/slave combined
> events properly.
>
> Signed-off-by: Jae Hyun Yoo
> ---
> Changes since v5:
> - Changed variable names in irq hanlders to represent proper meaning.
> - Fixed an error printing message again to make it use the irq_handled
> variable.
>
> Changes since v4:
> - Fixed an error printing message that handlers didn't handle all interrupts.
>
> Changes since v3:
> - Fixed typos in a comment.
>
> Changes since v2:
> - Changed the name of ASPEED_I2CD_INTR_ERRORS to
> ASPEED_I2CD_INTR_MASTER_ERRORS
> - Removed a member irq_status from the struct aspeed_i2c_bus and changed
> master_irq and slave_irq handlers to make them return status_ack.
> - Added a comment to explain why it needs to try both irq handlers.
>
> Changes since v1:
> - Fixed a grammar issue in commit message.
> - Added a missing line feed character into a message printing.
>
> drivers/i2c/busses/i2c-aspeed.c | 131 ++--
> 1 file changed, 76 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index a4f956c6d567..c258c4d9a4c0 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> #define ASPEED_I2CD_INTR_TX_NAKBIT(1)
> #define ASPEED_I2CD_INTR_TX_ACKBIT(0)
> +#define ASPEED_I2CD_INTR_MASTER_ERRORS
> \
> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
> \
> +ASPEED_I2CD_INTR_SCL_TIMEOUT |
> \
> +ASPEED_I2CD_INTR_ABNORMAL |
> \
> +ASPEED_I2CD_INTR_ARBIT_LOSS)
> #define ASPEED_I2CD_INTR_ALL
> \
> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
> \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE |
> \
> @@ -227,32 +232,26 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus
> *bus)
> }
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> -static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> +static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> - u32 command, irq_status, status_ack = 0;
> + u32 command, irq_handled = 0;
> struct i2c_client *slave = bus->slave;
> - bool irq_handled = true;
> u8 value;
>
> - if (!slave) {
> - irq_handled = false;
> - goto out;
> - }
> + if (!slave)
> + return 0;
>
> command = readl(bus->base + ASPEED_I2C_CMD_REG);
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>
> /* Slave was requested, restart state machine. */
> if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> - status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> + irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> bus->slave_state = ASPEED_I2C_SLAVE_START;
> }
>
> /* Slave is not currently active, irq was for someone else. */
> - if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> - irq_handled = false;
> - goto out;
> - }
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> + return irq_handled;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> irq_status, command);
> @@ -269,31 +268,31 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus
> *bus)
> bus->slave_state =
>
> ASPEED_I2C_SLAVE_WRITE_REQUESTED;
> }
> - status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> + irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
> }
>
> /* Slave was asked to stop. */
> if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> - status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> + irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>
Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
On Thu, Aug 23, 2018 at 3:58 PM Jae Hyun Yoo
wrote:
>
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much more in multi-master
> environment than single-master. For an example, when master is
> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
> SLAVE_MATCH and RX_DONE interrupts could come along with the
> NORMAL_STOP in case of an another master immediately sends data
> just after acquiring the bus. In this case, the NORMAL_STOP
> interrupt should be handled by master_irq and the SLAVE_MATCH and
> RX_DONE interrupts should be handled by slave_irq. This commit
> modifies irq hadling logic to handle the master/slave combined
> events properly.
>
> Signed-off-by: Jae Hyun Yoo
> ---
> Changes since v5:
> - Changed variable names in irq hanlders to represent proper meaning.
> - Fixed an error printing message again to make it use the irq_handled
> variable.
>
> Changes since v4:
> - Fixed an error printing message that handlers didn't handle all interrupts.
>
> Changes since v3:
> - Fixed typos in a comment.
>
> Changes since v2:
> - Changed the name of ASPEED_I2CD_INTR_ERRORS to
> ASPEED_I2CD_INTR_MASTER_ERRORS
> - Removed a member irq_status from the struct aspeed_i2c_bus and changed
> master_irq and slave_irq handlers to make them return status_ack.
> - Added a comment to explain why it needs to try both irq handlers.
>
> Changes since v1:
> - Fixed a grammar issue in commit message.
> - Added a missing line feed character into a message printing.
>
> drivers/i2c/busses/i2c-aspeed.c | 131 ++--
> 1 file changed, 76 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index a4f956c6d567..c258c4d9a4c0 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> #define ASPEED_I2CD_INTR_TX_NAKBIT(1)
> #define ASPEED_I2CD_INTR_TX_ACKBIT(0)
> +#define ASPEED_I2CD_INTR_MASTER_ERRORS
> \
> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
> \
> +ASPEED_I2CD_INTR_SCL_TIMEOUT |
> \
> +ASPEED_I2CD_INTR_ABNORMAL |
> \
> +ASPEED_I2CD_INTR_ARBIT_LOSS)
> #define ASPEED_I2CD_INTR_ALL
> \
> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
> \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE |
> \
> @@ -227,32 +232,26 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus
> *bus)
> }
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> -static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> +static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> - u32 command, irq_status, status_ack = 0;
> + u32 command, irq_handled = 0;
> struct i2c_client *slave = bus->slave;
> - bool irq_handled = true;
> u8 value;
>
> - if (!slave) {
> - irq_handled = false;
> - goto out;
> - }
> + if (!slave)
> + return 0;
>
> command = readl(bus->base + ASPEED_I2C_CMD_REG);
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>
> /* Slave was requested, restart state machine. */
> if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> - status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> + irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> bus->slave_state = ASPEED_I2C_SLAVE_START;
> }
>
> /* Slave is not currently active, irq was for someone else. */
> - if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> - irq_handled = false;
> - goto out;
> - }
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> + return irq_handled;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> irq_status, command);
> @@ -269,31 +268,31 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus
> *bus)
> bus->slave_state =
>
> ASPEED_I2C_SLAVE_WRITE_REQUESTED;
> }
> - status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> + irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
> }
>
> /* Slave was asked to stop. */
> if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> - status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> + irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>
