Re: [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
Thanks James for the review. Please see my inline answers.
On 7/23/2018 11:10 AM, James Feist wrote:
On 07/23/2018 10:48 AM, 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 in multi-master environment
much more
Thanks! Will fix it.
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
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++--
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c
b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 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_NAK BIT(1)
#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+#define ASPEED_I2CD_INTR_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 | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ 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)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
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) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= 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 false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct
aspeed_i2c_bus *bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
dev_err(bus->dev, "Unexpected ACK on read request.\n");
bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
-
i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, );
writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
Re: [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
Thanks James for the review. Please see my inline answers.
On 7/23/2018 11:10 AM, James Feist wrote:
On 07/23/2018 10:48 AM, 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 in multi-master environment
much more
Thanks! Will fix it.
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
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++--
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c
b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 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_NAK BIT(1)
#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+#define ASPEED_I2CD_INTR_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 | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ 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)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
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) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= 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 false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct
aspeed_i2c_bus *bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
dev_err(bus->dev, "Unexpected ACK on read request.\n");
bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
-
i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, );
writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
Re: [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
On 07/23/2018 10:48 AM, 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 in multi-master environment
much more
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
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++--
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 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_NAK BIT(1)
#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+#define ASPEED_I2CD_INTR_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 | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ 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)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
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) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= 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 false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus
*bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+
Re: [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
On 07/23/2018 10:48 AM, 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 in multi-master environment
much more
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
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++--
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 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_NAK BIT(1)
#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+#define ASPEED_I2CD_INTR_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 | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ 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)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
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) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= 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 false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus
*bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+
[PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
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 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
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++--
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 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_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 | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ 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)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
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) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= 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 false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus
*bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ if (bus->irq_status &
[PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
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 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
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++--
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 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_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 | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ 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)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
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) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= 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 false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus
*bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ if (bus->irq_status &
