* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-11 23:58 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180911233302.GA18799@roeck-us.net>
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(&bus->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(&bus->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
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-11 23:33 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACPK8Xdv3aP_zOHAL0WMgwvkpBHAUesU7OdhW==jgvXjRmt-Zw@mail.gmail.com>
On Wed, Sep 12, 2018 at 08:23:29AM +0930, Joel Stanley wrote:
> On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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(&bus->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(&bus->lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
^ permalink raw reply related
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Joel Stanley @ 2018-09-11 22:53 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <e5135af4-9105-214f-5de2-65cc03804222@linux.intel.com>
On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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
^ permalink raw reply
* [PATCH] Revert "i2c: aspeed: Handle master/slave combined irq events properly"
From: Jae Hyun Yoo @ 2018-09-11 22:24 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <e5135af4-9105-214f-5de2-65cc03804222@linux.intel.com>
This reverts commit 3e9efc3299dd "i2c: aspeed: Handle master/slave
combined irq events properly" to fix an issue which is reported
from QEMU v3.0 environment.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Cc: Guenter Roeck <linux@roeck-us.net>
---
drivers/i2c/busses/i2c-aspeed.c | 131 ++++++++++++++------------------
1 file changed, 55 insertions(+), 76 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index c258c4d9a4c0..a4f956c6d567 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -82,11 +82,6 @@
#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_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 | \
@@ -232,26 +227,32 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
}
#if IS_ENABLED(CONFIG_I2C_SLAVE)
-static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
+static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
{
- u32 command, irq_handled = 0;
+ u32 command, irq_status, status_ack = 0;
struct i2c_client *slave = bus->slave;
+ bool irq_handled = true;
u8 value;
- if (!slave)
- return 0;
+ if (!slave) {
+ irq_handled = false;
+ goto out;
+ }
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) {
- irq_handled |= 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)
- return irq_handled;
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+ irq_handled = false;
+ goto out;
+ }
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
irq_status, command);
@@ -268,31 +269,31 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
bus->slave_state =
ASPEED_I2C_SLAVE_WRITE_REQUESTED;
}
- irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
+ status_ack |= ASPEED_I2CD_INTR_RX_DONE;
}
/* Slave was asked to stop. */
if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
- irq_handled |= 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) {
- irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
+ status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
- irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
if (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, &value);
writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
break;
case ASPEED_I2C_SLAVE_READ_PROCESSED:
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
dev_err(bus->dev,
"Expected ACK after processed read.\n");
@@ -316,6 +317,13 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
break;
}
+ if (status_ack != irq_status)
+ dev_err(bus->dev,
+ "irq handled != irq. expected %x, but was %x\n",
+ irq_status, status_ack);
+ writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
+
+out:
return irq_handled;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -372,21 +380,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
return 0;
}
-static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
+static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
{
- u32 irq_handled = 0, command = 0;
+ u32 irq_status, status_ack = 0, command = 0;
struct i2c_msg *msg;
u8 recv_byte;
int ret;
+ irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ /* Ack all interrupt bits. */
+ writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
+
if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
- irq_handled |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
+ status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
goto out_complete;
- } else {
- /* Master is not currently active, irq was for someone else. */
- if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
- goto out_no_complete;
}
/*
@@ -395,22 +403,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
* INACTIVE state.
*/
ret = aspeed_i2c_is_irq_error(irq_status);
- if (ret) {
+ if (ret < 0) {
dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
irq_status);
bus->cmd_err = ret;
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
- irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
goto out_complete;
}
/* We are in an invalid state; reset bus to a known state. */
if (!bus->msgs) {
- dev_err(bus->dev, "bus in unknown state. irq_status: 0x%x\n",
- irq_status);
+ dev_err(bus->dev, "bus in unknown state\n");
bus->cmd_err = -EIO;
- if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
- bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
+ if (bus->master_state != ASPEED_I2C_MASTER_STOP)
aspeed_i2c_do_stop(bus);
goto out_no_complete;
}
@@ -423,18 +428,13 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
*/
if (bus->master_state == ASPEED_I2C_MASTER_START) {
if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_NAK))) {
- bus->cmd_err = -ENXIO;
- bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
- goto out_complete;
- }
pr_devel("no slave present at %02x\n", msg->addr);
- irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
+ status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->cmd_err = -ENXIO;
aspeed_i2c_do_stop(bus);
goto out_no_complete;
}
- irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
if (msg->len == 0) { /* SMBUS_QUICK */
aspeed_i2c_do_stop(bus);
goto out_no_complete;
@@ -449,13 +449,13 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
case ASPEED_I2C_MASTER_TX:
if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
dev_dbg(bus->dev, "slave NACKed TX\n");
- irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
+ status_ack |= ASPEED_I2CD_INTR_TX_NAK;
goto error_and_stop;
} else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
dev_err(bus->dev, "slave failed to ACK TX\n");
goto error_and_stop;
}
- irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
/* fallthrough intended */
case ASPEED_I2C_MASTER_TX_FIRST:
if (bus->buf_index < msg->len) {
@@ -478,7 +478,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
dev_err(bus->dev, "master failed to RX\n");
goto error_and_stop;
}
- irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
+ status_ack |= ASPEED_I2CD_INTR_RX_DONE;
recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
msg->buf[bus->buf_index++] = recv_byte;
@@ -506,13 +506,11 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
goto out_no_complete;
case ASPEED_I2C_MASTER_STOP:
if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
- dev_err(bus->dev,
- "master failed to STOP. irq_status:0x%x\n",
- irq_status);
+ dev_err(bus->dev, "master failed to STOP\n");
bus->cmd_err = -EIO;
/* Do not STOP as we have already tried. */
} else {
- irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
+ status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
}
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
@@ -542,52 +540,33 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
bus->master_xfer_result = bus->msgs_index + 1;
complete(&bus->cmd_complete);
out_no_complete:
- return irq_handled;
+ if (irq_status != status_ack)
+ dev_err(bus->dev,
+ "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+ irq_status, status_ack);
+ return !!irq_status;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
- u32 irq_received, irq_remaining, irq_handled;
+ bool ret;
spin_lock(&bus->lock);
- irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
- irq_remaining = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
- /*
- * In most cases, interrupt bits will be set one by one, although
- * multiple interrupt bits could be set at the same time. It's also
- * possible that master interrupt bits could be set along with slave
- * interrupt bits. Each case needs to be handled using corresponding
- * handlers depending on the current state.
- */
- if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
- irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
- irq_remaining &= ~irq_handled;
- if (irq_remaining)
- irq_handled |= aspeed_i2c_slave_irq(bus, irq_remaining);
- } else {
- irq_handled = aspeed_i2c_slave_irq(bus, irq_remaining);
- irq_remaining &= ~irq_handled;
- if (irq_remaining)
- irq_handled |= aspeed_i2c_master_irq(bus,
- irq_remaining);
+ if (aspeed_i2c_slave_irq(bus)) {
+ dev_dbg(bus->dev, "irq handled by slave.\n");
+ ret = true;
+ goto out;
}
-#else
- irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
#endif /* CONFIG_I2C_SLAVE */
- irq_remaining &= ~irq_handled;
- if (irq_remaining)
- dev_err(bus->dev,
- "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
- irq_received, irq_handled);
+ ret = aspeed_i2c_master_irq(bus);
- /* Ack all interrupt bits. */
- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
+out:
spin_unlock(&bus->lock);
- return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
+ return ret ? IRQ_HANDLED : IRQ_NONE;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.18.0
^ permalink raw reply related
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-11 22:18 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180911204107.GA26017@roeck-us.net>
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 <jae.hyun.yoo@linux.intel.com>
>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>>>
>>> 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
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-11 20:41 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1f34fe8c-69ef-5f2d-25dc-d5f6037cc558@linux.intel.com>
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 <jae.hyun.yoo@linux.intel.com>
> >>Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> >
> >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
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-11 20:30 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180911183734.GA21976@roeck-us.net>
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 <jae.hyun.yoo@linux.intel.com>
>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>
> 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
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Cédric Le Goater @ 2018-09-11 18:45 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180911183734.GA21976@roeck-us.net>
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 <jae.hyun.yoo@linux.intel.com>
>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>
> 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.
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-11 18:37 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180823225731.19063-1-jae.hyun.yoo@linux.intel.com>
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 <jae.hyun.yoo@linux.intel.com>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
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
^ permalink raw reply
* [RFC PATCH i2c-next 2/2] i2c: aspeed: Add bus idle waiting logic for multi-master use cases
From: Jae Hyun Yoo @ 2018-09-10 21:45 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180910214519.14126-1-jae.hyun.yoo@linux.intel.com>
In multi-master environment, this driver side master cannot know
exactly when peer master sends data to this side master so a case
can be happened that this master tries to send data through the
master_xfer function but slave data from peer master is still
being processed by this driver.
To prevent state corruption in the case, this patch adds checking
if any slave operation is ongoing and it wait up to the timeout
duration before starting a master_xfer operation.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 70 ++++++++++++++++++++++++++-------
1 file changed, 55 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index c258c4d9a4c0..73359eda98be 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -12,6 +12,7 @@
#include <linux/clk.h>
#include <linux/completion.h>
+#include <linux/delay.h>
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/i2c.h>
@@ -99,6 +100,7 @@
ASPEED_I2CD_INTR_TX_ACK)
/* 0x14 : I2CD Command/Status Register */
+#define ASPEED_I2CD_XFER_MODE_STS_MASK GENMASK(22, 19)
#define ASPEED_I2CD_SCL_LINE_STS BIT(18)
#define ASPEED_I2CD_SDA_LINE_STS BIT(17)
#define ASPEED_I2CD_BUS_BUSY_STS BIT(16)
@@ -115,6 +117,10 @@
/* 0x18 : I2CD Slave Device Address Register */
#define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
+/* Timeout for bus busy checking */
+#define ASPEED_I2CD_IDLE_WAIT_TIMEOUT_MS_DEFAULT 100 /* 100 ms */
+#define ASPEED_I2CD_BUS_BUSY_CHECK_INTERVAL_US 10000 /* 10 ms */
+
enum aspeed_i2c_master_state {
ASPEED_I2C_MASTER_INACTIVE,
ASPEED_I2C_MASTER_START,
@@ -155,6 +161,9 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ /* Multi-master */
+ bool multi_master;
+ u32 idle_wait_timeout_ms;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -590,27 +599,49 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
+static int aspeed_i2c_check_bus_busy(struct aspeed_i2c_bus *bus)
+{
+ u32 status_check_mask = ASPEED_I2CD_BUS_BUSY_STS;
+ ktime_t timeout;
+
+ if (bus->multi_master) {
+ might_sleep();
+ timeout = ktime_add_ms(ktime_get(), bus->idle_wait_timeout_ms);
+ /*
+ * ASPEED_I2CD_XFER_MODE_STS_MASK is marked as
+ * 'for debugging purpose only' in datasheet but ASPEED
+ * confirmed that this reflects real information and good to be
+ * used in practical code. It will be used only in multi-master
+ * use cases.
+ */
+ status_check_mask |= ASPEED_I2CD_XFER_MODE_STS_MASK;
+ }
+
+ for (;;) {
+ if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
+ status_check_mask))
+ return 0;
+ if (!bus->multi_master)
+ break;
+ if (ktime_compare(ktime_get(), timeout) > 0)
+ break;
+ usleep_range((ASPEED_I2CD_BUS_BUSY_CHECK_INTERVAL_US >> 2) + 1,
+ ASPEED_I2CD_BUS_BUSY_CHECK_INTERVAL_US);
+ }
+
+ return aspeed_i2c_recover_bus(bus);
+}
+
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
{
struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
unsigned long time_left, flags;
- int ret = 0;
-
- spin_lock_irqsave(&bus->lock, flags);
- bus->cmd_err = 0;
- /* If bus is busy, attempt recovery. We assume a single master
- * environment.
- */
- if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS) {
- spin_unlock_irqrestore(&bus->lock, flags);
- ret = aspeed_i2c_recover_bus(bus);
- if (ret)
- return ret;
- spin_lock_irqsave(&bus->lock, flags);
- }
+ if (aspeed_i2c_check_bus_busy(bus))
+ return -EAGAIN;
+ spin_lock_irqsave(&bus->lock, flags);
bus->cmd_err = 0;
bus->msgs = msgs;
bus->msgs_index = 0;
@@ -798,8 +829,17 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
if (ret < 0)
return ret;
- if (!of_property_read_bool(pdev->dev.of_node, "multi-master"))
+ if (of_property_read_bool(pdev->dev.of_node, "multi-master")) {
+ bus->multi_master = true;
+ ret = of_property_read_u32(pdev->dev.of_node,
+ "idle-wait-timeout-ms",
+ &bus->idle_wait_timeout_ms);
+ if (ret)
+ bus->idle_wait_timeout_ms =
+ ASPEED_I2CD_IDLE_WAIT_TIMEOUT_MS_DEFAULT;
+ } else {
fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS;
+ }
/* Enable Master Mode */
writel(readl(bus->base + ASPEED_I2C_FUN_CTRL_REG) | fun_ctrl_reg,
--
2.18.0
^ permalink raw reply related
* [RFC PATCH i2c-next 1/2] dt-bindings: i2c: aspeed: Add 'idle-wait-timeout-ms' setting
From: Jae Hyun Yoo @ 2018-09-10 21:45 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180910214519.14126-1-jae.hyun.yoo@linux.intel.com>
This commit adds 'idle-wait-timeout-ms' setting which can be used
for bus idle waiting logic in multi-master environment.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
Documentation/devicetree/bindings/i2c/i2c-aspeed.txt | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
index 8fbd8633a387..42ecaaf67172 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -13,9 +13,13 @@ Required Properties:
- interrupts : interrupt number
Optional Properties:
-- bus-frequency : frequency of the bus clock in Hz defaults to 100 kHz when not
- specified
-- multi-master : states that there is another master active on this bus.
+- bus-frequency : frequency of the bus clock in Hz defaults to 100 kHz
+ when not specified
+- multi-master : states that there is another master active on this
+ bus.
+- idle-wait-timeout-ms : bus idle waiting timeout in milliseconds when
+ multi-master is set, defaults to 100 ms when not
+ specified.
Example:
--
2.18.0
^ permalink raw reply related
* [RFC PATCH i2c-next 0/2] i2c: aspeed: Add bus idle waiting logic for multi-master use cases
From: Jae Hyun Yoo @ 2018-09-10 21:45 UTC (permalink / raw)
To: linux-aspeed
In multi-master environment, this driver side master cannot know
exactly when peer master sends data to this side master so a case
can be happened that this master tries to send data through the
master_xfer function but slave data from peer master is still
being processed by this driver.
To prevent state corruption in the case, this patch adds checking
if any slave operation is ongoing and it wait up to the timeout
duration before starting a master_xfer operation.
Please review this patch set.
Thanks,
-Jae
Jae Hyun Yoo (2):
dt-bindings: i2c: aspeed: Add 'idle-wait-timeout-ms' setting
i2c: aspeed: Add bus idle waiting logic for multi-master use cases
.../devicetree/bindings/i2c/i2c-aspeed.txt | 10 ++-
drivers/i2c/busses/i2c-aspeed.c | 70 +++++++++++++++----
2 files changed, 62 insertions(+), 18 deletions(-)
--
2.18.0
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Wolfram Sang @ 2018-09-06 18:40 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180823225731.19063-1-jae.hyun.yoo@linux.intel.com>
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 <jae.hyun.yoo@linux.intel.com>
Applied to for-next, thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20180906/e639f9f3/attachment.sig>
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-06 18:33 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180906180837.GA8607@kunai>
>
> 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!
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Wolfram Sang @ 2018-09-06 18:08 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <eab56d58-0703-d201-dc9a-f9fd257e96cf@linux.intel.com>
> > Looks awesome! Thanks!
> >
> > Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> >
>
> 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!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20180906/997d89f1/attachment.sig>
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-06 17:32 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g46fRrYW8E8FsPB=5fWSGxemgc4ENc4D2fT7yBfH6p4ERg@mail.gmail.com>
On 9/6/2018 10:26 AM, Brendan Higgins wrote:
> On Thu, Aug 23, 2018 at 3:58 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> 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 <jae.hyun.yoo@linux.intel.com>
>> ---
>> 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_NAK BIT(1)
>> #define ASPEED_I2CD_INTR_TX_ACK BIT(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 |= ASPEED_I2CD_INTR_TX_NAK;
>> + irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> }
>> + if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> + irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>>
>> switch (bus->slave_state) {
>> case ASPEED_I2C_SLAVE_READ_REQUESTED:
>> if (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, &value);
>> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>> break;
>> case ASPEED_I2C_SLAVE_READ_PROCESSED:
>> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> dev_err(bus->dev,
>> "Expected ACK after processed read.\n");
>> @@ -317,13 +316,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> break;
>> }
>>
>> - if (status_ack != irq_status)
>> - dev_err(bus->dev,
>> - "irq handled != irq. expected %x, but was %x\n",
>> - irq_status, status_ack);
>> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -out:
>> return irq_handled;
>> }
>> #endif /* CONFIG_I2C_SLAVE */
>> @@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> return 0;
>> }
>>
>> -static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> +static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> {
>> - u32 irq_status, status_ack = 0, command = 0;
>> + u32 irq_handled = 0, command = 0;
>> struct i2c_msg *msg;
>> u8 recv_byte;
>> int ret;
>>
>> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> - /* Ack all interrupt bits. */
>> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> - status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> + irq_handled |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> goto out_complete;
>> + } else {
>> + /* Master is not currently active, irq was for someone else. */
>> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
>> + goto out_no_complete;
>> }
>>
>> /*
>> @@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> * INACTIVE state.
>> */
>> ret = aspeed_i2c_is_irq_error(irq_status);
>> - if (ret < 0) {
>> + if (ret) {
>> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> irq_status);
>> bus->cmd_err = ret;
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> + irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
>> goto out_complete;
>> }
>>
>> /* We are in an invalid state; reset bus to a known state. */
>> if (!bus->msgs) {
>> - dev_err(bus->dev, "bus in unknown state\n");
>> + dev_err(bus->dev, "bus in unknown state. irq_status: 0x%x\n",
>> + irq_status);
>> bus->cmd_err = -EIO;
>> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
>> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
>> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>> aspeed_i2c_do_stop(bus);
>> goto out_no_complete;
>> }
>> @@ -428,13 +423,18 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> */
>> if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> + if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_NAK))) {
>> + bus->cmd_err = -ENXIO;
>> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> + goto out_complete;
>> + }
>> pr_devel("no slave present at %02x\n", msg->addr);
>> - status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> + irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->cmd_err = -ENXIO;
>> aspeed_i2c_do_stop(bus);
>> goto out_no_complete;
>> }
>> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> + irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>> if (msg->len == 0) { /* SMBUS_QUICK */
>> aspeed_i2c_do_stop(bus);
>> goto out_no_complete;
>> @@ -449,13 +449,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> case ASPEED_I2C_MASTER_TX:
>> if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> dev_dbg(bus->dev, "slave NACKed TX\n");
>> - status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> + irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> goto error_and_stop;
>> } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> dev_err(bus->dev, "slave failed to ACK TX\n");
>> goto error_and_stop;
>> }
>> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> + irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>> /* fallthrough intended */
>> case ASPEED_I2C_MASTER_TX_FIRST:
>> if (bus->buf_index < msg->len) {
>> @@ -478,7 +478,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> dev_err(bus->dev, "master failed to RX\n");
>> goto error_and_stop;
>> }
>> - status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> + irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>>
>> recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> msg->buf[bus->buf_index++] = recv_byte;
>> @@ -506,11 +506,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> goto out_no_complete;
>> case ASPEED_I2C_MASTER_STOP:
>> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> - dev_err(bus->dev, "master failed to STOP\n");
>> + dev_err(bus->dev,
>> + "master failed to STOP. irq_status:0x%x\n",
>> + irq_status);
>> bus->cmd_err = -EIO;
>> /* Do not STOP as we have already tried. */
>> } else {
>> - status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> + irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> }
>>
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> @@ -540,33 +542,52 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> bus->master_xfer_result = bus->msgs_index + 1;
>> complete(&bus->cmd_complete);
>> out_no_complete:
>> - if (irq_status != status_ack)
>> - dev_err(bus->dev,
>> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> - irq_status, status_ack);
>> - return !!irq_status;
>> + return irq_handled;
>> }
>>
>> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> {
>> struct aspeed_i2c_bus *bus = dev_id;
>> - bool ret;
>> + u32 irq_received, irq_remaining, irq_handled;
>>
>> spin_lock(&bus->lock);
>> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> + irq_remaining = irq_received;
>>
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> - if (aspeed_i2c_slave_irq(bus)) {
>> - dev_dbg(bus->dev, "irq handled by slave.\n");
>> - ret = true;
>> - goto out;
>> + /*
>> + * In most cases, interrupt bits will be set one by one, although
>> + * multiple interrupt bits could be set at the same time. It's also
>> + * possible that master interrupt bits could be set along with slave
>> + * interrupt bits. Each case needs to be handled using corresponding
>> + * handlers depending on the current state.
>> + */
>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> + irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
>> + irq_remaining &= ~irq_handled;
>> + if (irq_remaining)
>> + irq_handled |= aspeed_i2c_slave_irq(bus, irq_remaining);
>> + } else {
>> + irq_handled = aspeed_i2c_slave_irq(bus, irq_remaining);
>> + irq_remaining &= ~irq_handled;
>> + if (irq_remaining)
>> + irq_handled |= aspeed_i2c_master_irq(bus,
>> + irq_remaining);
>> }
>> +#else
>> + irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
>> #endif /* CONFIG_I2C_SLAVE */
>>
>> - ret = aspeed_i2c_master_irq(bus);
>> + irq_remaining &= ~irq_handled;
>> + if (irq_remaining)
>> + dev_err(bus->dev,
>> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> + irq_received, irq_handled);
>>
>> -out:
>> + /* Ack all interrupt bits. */
>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>> spin_unlock(&bus->lock);
>> - return ret ? IRQ_HANDLED : IRQ_NONE;
>> + return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>> }
>>
>> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>> --
>> 2.18.0
>>
>
> Looks awesome! Thanks!
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>
Thanks a lot for your review Brendan!
-Jae
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Brendan Higgins @ 2018-09-06 17:26 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180823225731.19063-1-jae.hyun.yoo@linux.intel.com>
On Thu, Aug 23, 2018 at 3:58 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> 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 <jae.hyun.yoo@linux.intel.com>
> ---
> 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_NAK BIT(1)
> #define ASPEED_I2CD_INTR_TX_ACK BIT(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 |= ASPEED_I2CD_INTR_TX_NAK;
> + irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>
> switch (bus->slave_state) {
> case ASPEED_I2C_SLAVE_READ_REQUESTED:
> if (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, &value);
> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
> break;
> case ASPEED_I2C_SLAVE_READ_PROCESSED:
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> @@ -317,13 +316,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> break;
> }
>
> - if (status_ack != irq_status)
> - dev_err(bus->dev,
> - "irq handled != irq. expected %x, but was %x\n",
> - irq_status, status_ack);
> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> -out:
> return irq_handled;
> }
> #endif /* CONFIG_I2C_SLAVE */
> @@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
> return 0;
> }
>
> -static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> - u32 irq_status, status_ack = 0, command = 0;
> + u32 irq_handled = 0, command = 0;
> struct i2c_msg *msg;
> u8 recv_byte;
> int ret;
>
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> - /* Ack all interrupt bits. */
> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> - status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> + irq_handled |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> goto out_complete;
> + } else {
> + /* Master is not currently active, irq was for someone else. */
> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
> + goto out_no_complete;
> }
>
> /*
> @@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * INACTIVE state.
> */
> ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
> goto out_complete;
> }
>
> /* We are in an invalid state; reset bus to a known state. */
> if (!bus->msgs) {
> - dev_err(bus->dev, "bus in unknown state\n");
> + dev_err(bus->dev, "bus in unknown state. irq_status: 0x%x\n",
> + irq_status);
> bus->cmd_err = -EIO;
> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> }
> @@ -428,13 +423,18 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_NAK))) {
> + bus->cmd_err = -ENXIO;
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + goto out_complete;
> + }
> pr_devel("no slave present at %02x\n", msg->addr);
> - status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> + irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> }
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
> if (msg->len == 0) { /* SMBUS_QUICK */
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> @@ -449,13 +449,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> case ASPEED_I2C_MASTER_TX:
> if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> dev_dbg(bus->dev, "slave NACKed TX\n");
> - status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> + irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> goto error_and_stop;
> } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> dev_err(bus->dev, "slave failed to ACK TX\n");
> goto error_and_stop;
> }
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
> /* fallthrough intended */
> case ASPEED_I2C_MASTER_TX_FIRST:
> if (bus->buf_index < msg->len) {
> @@ -478,7 +478,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> dev_err(bus->dev, "master failed to RX\n");
> goto error_and_stop;
> }
> - status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> + irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>
> recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
> msg->buf[bus->buf_index++] = recv_byte;
> @@ -506,11 +506,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> goto out_no_complete;
> case ASPEED_I2C_MASTER_STOP:
> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> - dev_err(bus->dev, "master failed to STOP\n");
> + dev_err(bus->dev,
> + "master failed to STOP. irq_status:0x%x\n",
> + irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> - status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> + irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
> }
>
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> @@ -540,33 +542,52 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> bus->master_xfer_result = bus->msgs_index + 1;
> complete(&bus->cmd_complete);
> out_no_complete:
> - if (irq_status != status_ack)
> - dev_err(bus->dev,
> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> - irq_status, status_ack);
> - return !!irq_status;
> + return irq_handled;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received, irq_remaining, irq_handled;
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - ret = true;
> - goto out;
> + /*
> + * In most cases, interrupt bits will be set one by one, although
> + * multiple interrupt bits could be set at the same time. It's also
> + * possible that master interrupt bits could be set along with slave
> + * interrupt bits. Each case needs to be handled using corresponding
> + * handlers depending on the current state.
> + */
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
> + irq_remaining &= ~irq_handled;
> + if (irq_remaining)
> + irq_handled |= aspeed_i2c_slave_irq(bus, irq_remaining);
> + } else {
> + irq_handled = aspeed_i2c_slave_irq(bus, irq_remaining);
> + irq_remaining &= ~irq_handled;
> + if (irq_remaining)
> + irq_handled |= aspeed_i2c_master_irq(bus,
> + irq_remaining);
> }
> +#else
> + irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + irq_remaining &= ~irq_handled;
> + if (irq_remaining)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_handled);
>
> -out:
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> - return ret ? IRQ_HANDLED : IRQ_NONE;
> + return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> --
> 2.18.0
>
Looks awesome! Thanks!
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
^ permalink raw reply
* [PATCH 1/2] misc: aspeed: Add Aspeed UART routing control driver.
From: Linus Walleij @ 2018-09-05 19:27 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CABoTLcQpyaLUtdSPJQ4v5_Ds2o54sDJxk+9_etthwQwWRoYK-Q@mail.gmail.com>
On Wed, Sep 5, 2018 at 3:45 PM Oskar Senft <osk@google.com> wrote:
> I just had a look at the upstream kernel - there's no drivers/soc/aspeed.
> I did find drivers/soc, but no subdirectory for aspeed. Should I create
> that with my commit?
Yeah just do it, someone has to be the first.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 1/2] misc: aspeed: Add Aspeed UART routing control driver.
From: Oskar Senft @ 2018-09-05 13:45 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CABoTLcRpQhC7L+Yk2uR91TJSB21i0cp+3BH5cgZWYOWmN6PzVQ@mail.gmail.com>
I just had a look at the upstream kernel - there's no drivers/soc/aspeed. I
did find drivers/soc, but no subdirectory for aspeed. Should I create that
with my commit?
Oskar.
On Tue, Sep 4, 2018 at 8:19 PM Oskar Senft <osk@google.com> wrote:
> Thanks for the input!
>
> I had a look at get_maintainer.pl, but thought I'd send it to you and
> Linus first to get initial input since we had already talked about it.
>
> I'll move it to drivers/soc/aspeed and will resend.
>
> For clarification: Should I send to ALL of the people and lists that
> get_maintainer.pl spits out?
>
> Thanks
> Oskar.
>
> On Tue, Sep 4, 2018 at 8:15 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>> On Wed, 5 Sep 2018, at 07:25, Linus Walleij wrote:
>> > On Tue, Sep 4, 2018 at 10:20 PM Oskar Senft <osk@google.com> wrote:
>> >
>> > > This driver adds sysfs files that allow the BMC userspace to configure
>> > > how UARTs and physical serial I/O ports are routed.
>> > >
>> > > Tested: Checked correct behavior (both read & write) on TYAN S7106
>> > > board by manually changing routing settings and confirming that bits
>> > > flow as expected. Tested for UART1 and UART3 as this board doesn't
>> have
>> > > the other UARTs wired up in a testable way.
>> > >
>> > > Signed-off-by: Oskar Senft <osk@google.com>
>> >
>> > After reading some other messages about this I realize it's not very
>> > much you can do with the standard frameworks and you definately
>> > need something like this. Not like I have any better idea.
>> >
>> > Arnd and Greg are misc maintainers so not you will need to
>> > talk to them.
>>
>> Yes. I'm certainly not a person that can merge them.
>>
>> Oskar: Please add at least Greg and Arnd as Linus suggests, and in the
>> future make use of ./scripts/get_maintainer.pl to identify people and
>> lists who should receive the patches.
>>
>> >
>> > I would opt to put it in drivers/soc/aspeed though.
>>
>> Yep, drivers/soc/aspeed seems like a better place. There are other
>> drivers in misc likely should be moved there as well.
>>
>> Thanks for your feedback Linus.
>>
>> Andrew
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20180905/4c4250e1/attachment-0001.html>
^ permalink raw reply
* [PATCH 1/2] misc: aspeed: Add Aspeed UART routing control driver.
From: Linus Walleij @ 2018-09-05 6:06 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CABoTLcRpQhC7L+Yk2uR91TJSB21i0cp+3BH5cgZWYOWmN6PzVQ@mail.gmail.com>
On Wed, Sep 5, 2018 at 2:19 AM Oskar Senft <osk@google.com> wrote:
> For clarification: Should I send to ALL of the people and lists that get_maintainer.pl spits out?
It's more like a suggestion. Definately the names coming from
MAINTAINERS, the rest is "good to have". If it becomes too many
just strip some off randomly, or look who changed the file(s) most
recently.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 1/2] misc: aspeed: Add Aspeed UART routing control driver.
From: Oskar Senft @ 2018-09-05 0:19 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1536106554.3176480.1496936032.1C7BEEDC@webmail.messagingengine.com>
Thanks for the input!
I had a look at get_maintainer.pl, but thought I'd send it to you and Linus
first to get initial input since we had already talked about it.
I'll move it to drivers/soc/aspeed and will resend.
For clarification: Should I send to ALL of the people and lists that
get_maintainer.pl spits out?
Thanks
Oskar.
On Tue, Sep 4, 2018 at 8:15 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> On Wed, 5 Sep 2018, at 07:25, Linus Walleij wrote:
> > On Tue, Sep 4, 2018 at 10:20 PM Oskar Senft <osk@google.com> wrote:
> >
> > > This driver adds sysfs files that allow the BMC userspace to configure
> > > how UARTs and physical serial I/O ports are routed.
> > >
> > > Tested: Checked correct behavior (both read & write) on TYAN S7106
> > > board by manually changing routing settings and confirming that bits
> > > flow as expected. Tested for UART1 and UART3 as this board doesn't have
> > > the other UARTs wired up in a testable way.
> > >
> > > Signed-off-by: Oskar Senft <osk@google.com>
> >
> > After reading some other messages about this I realize it's not very
> > much you can do with the standard frameworks and you definately
> > need something like this. Not like I have any better idea.
> >
> > Arnd and Greg are misc maintainers so not you will need to
> > talk to them.
>
> Yes. I'm certainly not a person that can merge them.
>
> Oskar: Please add at least Greg and Arnd as Linus suggests, and in the
> future make use of ./scripts/get_maintainer.pl to identify people and
> lists who should receive the patches.
>
> >
> > I would opt to put it in drivers/soc/aspeed though.
>
> Yep, drivers/soc/aspeed seems like a better place. There are other drivers
> in misc likely should be moved there as well.
>
> Thanks for your feedback Linus.
>
> Andrew
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20180904/f4f51d53/attachment-0001.html>
^ permalink raw reply
* [PATCH 1/2] misc: aspeed: Add Aspeed UART routing control driver.
From: Andrew Jeffery @ 2018-09-05 0:15 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACRpkdamHdGH9bANugQchYLWFS_mde6MmcbvBgcG-hrhvGVFWQ@mail.gmail.com>
On Wed, 5 Sep 2018, at 07:25, Linus Walleij wrote:
> On Tue, Sep 4, 2018 at 10:20 PM Oskar Senft <osk@google.com> wrote:
>
> > This driver adds sysfs files that allow the BMC userspace to configure
> > how UARTs and physical serial I/O ports are routed.
> >
> > Tested: Checked correct behavior (both read & write) on TYAN S7106
> > board by manually changing routing settings and confirming that bits
> > flow as expected. Tested for UART1 and UART3 as this board doesn't have
> > the other UARTs wired up in a testable way.
> >
> > Signed-off-by: Oskar Senft <osk@google.com>
>
> After reading some other messages about this I realize it's not very
> much you can do with the standard frameworks and you definately
> need something like this. Not like I have any better idea.
>
> Arnd and Greg are misc maintainers so not you will need to
> talk to them.
Yes. I'm certainly not a person that can merge them.
Oskar: Please add at least Greg and Arnd as Linus suggests, and in the future make use of ./scripts/get_maintainer.pl to identify people and lists who should receive the patches.
>
> I would opt to put it in drivers/soc/aspeed though.
Yep, drivers/soc/aspeed seems like a better place. There are other drivers in misc likely should be moved there as well.
Thanks for your feedback Linus.
Andrew
^ permalink raw reply
* [PATCH 1/2] misc: aspeed: Add Aspeed UART routing control driver.
From: Linus Walleij @ 2018-09-04 21:55 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180904202018.198707-2-osk@google.com>
On Tue, Sep 4, 2018 at 10:20 PM Oskar Senft <osk@google.com> wrote:
> This driver adds sysfs files that allow the BMC userspace to configure
> how UARTs and physical serial I/O ports are routed.
>
> Tested: Checked correct behavior (both read & write) on TYAN S7106
> board by manually changing routing settings and confirming that bits
> flow as expected. Tested for UART1 and UART3 as this board doesn't have
> the other UARTs wired up in a testable way.
>
> Signed-off-by: Oskar Senft <osk@google.com>
After reading some other messages about this I realize it's not very
much you can do with the standard frameworks and you definately
need something like this. Not like I have any better idea.
Arnd and Greg are misc maintainers so not you will need to
talk to them.
I would opt to put it in drivers/soc/aspeed though.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 2/2] arch: arm: boot: dts: aspeed: Add UART routing driver to Aspeed G5 DTS.
From: Oskar Senft @ 2018-09-04 20:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180904202018.198707-1-osk@google.com>
Signed-off-by: Oskar Senft <osk@google.com>
---
.../bindings/misc/aspeed,uart-routing.txt | 22 +++++++++++++++++++
arch/arm/boot/dts/aspeed-g5.dtsi | 6 +++++
2 files changed, 28 insertions(+)
create mode 100644 Documentation/devicetree/bindings/misc/aspeed,uart-routing.txt
diff --git a/Documentation/devicetree/bindings/misc/aspeed,uart-routing.txt b/Documentation/devicetree/bindings/misc/aspeed,uart-routing.txt
new file mode 100644
index 000000000000..d0a93173593c
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed,uart-routing.txt
@@ -0,0 +1,22 @@
+* ASPEED AST2500 UART routing
+
+This file describes the bindings for the UART routing selector present
+in the AST2500 BMC SoC.
+
+The Aspeed AST2500 allows to dynamically route the inputs for the built-in
+UARTS and physical serial I/O ports.
+
+This allows, for example, to connect the output of UART to another UART.
+This can be used to enable host<->BMC communication via UARTs, e.g. to allow
+access to the host's serial console.
+
+Required properties:
+- reg: address and length of the register for the device.
+- compatible: "aspeed,ast2500-cvic"
+
+Example:
+
+ uart_routing: uart_routing at 1e78909c {
+ compatible = "aspeed,ast2500-uart-routing";
+ reg = <0x1e78909c 0x4>;
+ };
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 87fdc146ff52..03094e8878eb 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -375,6 +375,12 @@
status = "disabled";
};
};
+
+ uart_routing: uart_routing at 9c {
+ compatible = "aspeed,ast2500-uart-routing";
+ reg = <0x9c 0x4>;
+ status = "disabled";
+ };
};
uart2: serial at 1e78d000 {
--
2.19.0.rc1.350.ge57e33dbd1-goog
^ permalink raw reply related
* [PATCH 1/2] misc: aspeed: Add Aspeed UART routing control driver.
From: Oskar Senft @ 2018-09-04 20:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180904202018.198707-1-osk@google.com>
This driver adds sysfs files that allow the BMC userspace to configure
how UARTs and physical serial I/O ports are routed.
Tested: Checked correct behavior (both read & write) on TYAN S7106
board by manually changing routing settings and confirming that bits
flow as expected. Tested for UART1 and UART3 as this board doesn't have
the other UARTs wired up in a testable way.
Signed-off-by: Oskar Senft <osk@google.com>
---
.../stable/sysfs-driver-aspeed-uart-routing | 14 +
.../misc-devices/aspeed-uart-routing.txt | 49 +++
drivers/misc/Kconfig | 10 +
drivers/misc/Makefile | 1 +
drivers/misc/aspeed-uart-routing.c | 384 ++++++++++++++++++
5 files changed, 458 insertions(+)
create mode 100644 Documentation/ABI/stable/sysfs-driver-aspeed-uart-routing
create mode 100644 Documentation/misc-devices/aspeed-uart-routing.txt
create mode 100644 drivers/misc/aspeed-uart-routing.c
diff --git a/Documentation/ABI/stable/sysfs-driver-aspeed-uart-routing b/Documentation/ABI/stable/sysfs-driver-aspeed-uart-routing
new file mode 100644
index 000000000000..5068737d9c12
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-aspeed-uart-routing
@@ -0,0 +1,14 @@
+What: /sys/bus/platform/drivers/aspeed-uart-routing/*/io*
+Date: August 2018
+Contact: Oskar Senft <osk@google.com>
+Description: Configures the input source for the specific physical
+ serial I/O port.
+Users: OpenBMC. Proposed changes should be mailed to
+ openbmc at lists.ozlabs.org
+
+What: /sys/bus/platform/drivers/aspeed-uart-routing/*/uart*
+Date: August 2018
+Contact: Oskar Senft <osk@google.com>
+Description: Configures the input source for the specific UART.
+Users: OpenBMC. Proposed changes should be mailed to
+ openbmc at lists.ozlabs.org
diff --git a/Documentation/misc-devices/aspeed-uart-routing.txt b/Documentation/misc-devices/aspeed-uart-routing.txt
new file mode 100644
index 000000000000..afaf17cb7eda
--- /dev/null
+++ b/Documentation/misc-devices/aspeed-uart-routing.txt
@@ -0,0 +1,49 @@
+Kernel driver aspeed-uart-routing
+=================================
+
+Supported chips:
+ASPEED AST2500
+
+Author:
+Google LLC
+
+Description
+-----------
+
+The Aspeed AST2500 allows to dynamically route the inputs for the built-in
+UARTS and physical serial I/O ports.
+
+This allows, for example, to connect the output of UART to another UART.
+This can be used to enable host<->BMC communication via UARTs, e.g. to allow
+access to the host's serial console.
+
+This driver is for the BMC side. The sysfs files allow the BMC userspace
+which owns the system configuration policy, to configure how UARTs and
+physical serial I/O ports are routed.
+
+The driver provides the following files in sysfs:
+uart1 Configure the input signal to UART1.
+uart2 Configure the input signal to UART2.
+uart3 Configure the input signal to UART3.
+uart4 Configure the input signal to UART4.
+uart5 Configure the input signal to UART5.
+io1 Configure the input signal to physical serial port 1.
+io2 Configure the input signal to physical serial port 2.
+io3 Configure the input signal to physical serial port 3.
+io4 Configure the input signal to physical serial port 4.
+io5 Configure the input signal to physical serial port 5.
+
+When read, each file shows the list of available options with the currently
+selected option marked by square brackets "[]". The list of available options
+depends on the selected file.
+
+Example:
+$ cat /sys/bus/platform/drivers/aspeed-uart-routing/*.uart_routing/uart1
+[io1] io2 io3 io4 uart2 uart3 uart4 io6
+
+In this case, UART1 gets its input signal from IO1 (physical serial port 1).
+
+$ echo -n "uart3" \
+ >/sys/bus/platform/drivers/aspeed-uart-routing/*.uart_routing/uart1
+$ cat /sys/bus/platform/drivers/aspeed-uart-routing/*.uart_routing/uart1
+io1 io2 io3 io4 uart2 [uart3] uart4 io6
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 3726eacdf65d..89d42cd6b790 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -501,6 +501,16 @@ config ASPEED_LPC_SNOOP
allows the BMC to listen on and save the data written by
the host to an arbitrary LPC I/O port.
+config ASPEED_UART_ROUTING
+ tristate "Aspeed ast2500 UART routing control"
+ depends on (ARCH_ASPEED || COMPILE_TEST)
+ help
+ Provides a driver to configure UART routing on Aspeed BMC platforms
+ at runtime through sysfs. This allows, for example, to connect the
+ output of UART to another UART. This can be used to enable host<->BMC
+ communication via UARTs, e.g. to allow access to the host's serial
+ console.
+
config PCI_ENDPOINT_TEST
depends on PCI
select CRC32
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index af22bbc3d00c..9705182b317f 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
obj-$(CONFIG_CXL_BASE) += cxl/
obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
+obj-$(CONFIG_ASPEED_UART_ROUTING) += aspeed-uart-routing.o
obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
obj-$(CONFIG_OCXL) += ocxl/
obj-$(CONFIG_MISC_RTSX) += cardreader/
diff --git a/drivers/misc/aspeed-uart-routing.c b/drivers/misc/aspeed-uart-routing.c
new file mode 100644
index 000000000000..381bac19e4af
--- /dev/null
+++ b/drivers/misc/aspeed-uart-routing.c
@@ -0,0 +1,384 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UART Routing driver for Aspeed AST2500
+ *
+ * Copyright (c) 2018 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+/* The Aspeed AST2500 allows to dynamically route the inputs for the built-in
+ * UARTS and physical serial I/O ports.
+ *
+ * This allows, for example, to connect the output of UART to another UART.
+ * This can be used to enable host<->BMC communication via UARTs, e.g. to allow
+ * access to the host's serial console.
+ *
+ * This driver is for the BMC side. The sysfs files allow the BMC userspace
+ * which owns the system configuration policy, to configure how UARTs and
+ * physical serial I/O ports are routed.
+ */
+
+#define ASPEED_HICRA_IO1 "io1"
+#define ASPEED_HICRA_IO2 "io2"
+#define ASPEED_HICRA_IO3 "io3"
+#define ASPEED_HICRA_IO4 "io4"
+#define ASPEED_HICRA_IO5 "io5"
+#define ASPEED_HICRA_IO6 "io6"
+#define ASPEED_HICRA_UART1 "uart1"
+#define ASPEED_HICRA_UART2 "uart2"
+#define ASPEED_HICRA_UART3 "uart3"
+#define ASPEED_HICRA_UART4 "uart4"
+#define ASPEED_HICRA_UART5 "uart5"
+
+struct aspeed_uart_routing {
+ struct device *dev;
+ void __iomem *regs;
+ spinlock_t lock;
+};
+
+struct aspeed_uart_routing_selector {
+ struct device_attribute dev_attr;
+ int shift;
+ int mask;
+ const char * const options[];
+};
+
+#define to_routing_selector(_dev_attr) \
+ container_of(_dev_attr, struct aspeed_uart_routing_selector, dev_attr)
+
+
+static ssize_t aspeed_uart_routing_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf);
+
+static ssize_t aspeed_uart_routing_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count);
+
+#define ROUTING_ATTR(_name) { \
+ .attr = {.name = _name, \
+ .mode = VERIFY_OCTAL_PERMISSIONS(0644) }, \
+ .show = aspeed_uart_routing_show, \
+ .store = aspeed_uart_routing_store, \
+}
+
+static struct aspeed_uart_routing_selector uart5_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_UART5),
+ .shift = 28,
+ .mask = 0xf,
+ .options = {
+ ASPEED_HICRA_IO5, // 0
+ ASPEED_HICRA_IO1, // 1
+ ASPEED_HICRA_IO2, // 2
+ ASPEED_HICRA_IO3, // 3
+ ASPEED_HICRA_IO4, // 4
+ ASPEED_HICRA_UART1, // 5
+ ASPEED_HICRA_UART2, // 6
+ ASPEED_HICRA_UART3, // 7
+ ASPEED_HICRA_UART4, // 8
+ ASPEED_HICRA_IO6, // 9
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector uart4_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_UART4),
+ .shift = 25,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_IO4, // 0
+ ASPEED_HICRA_IO1, // 1
+ ASPEED_HICRA_IO2, // 2
+ ASPEED_HICRA_IO3, // 3
+ ASPEED_HICRA_UART1, // 4
+ ASPEED_HICRA_UART2, // 5
+ ASPEED_HICRA_UART3, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector uart3_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_UART3),
+ .shift = 22,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_IO3, // 0
+ ASPEED_HICRA_IO4, // 1
+ ASPEED_HICRA_IO1, // 2
+ ASPEED_HICRA_IO2, // 3
+ ASPEED_HICRA_UART4, // 4
+ ASPEED_HICRA_UART1, // 5
+ ASPEED_HICRA_UART2, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector uart2_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_UART2),
+ .shift = 19,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_IO2, // 0
+ ASPEED_HICRA_IO3, // 1
+ ASPEED_HICRA_IO4, // 2
+ ASPEED_HICRA_IO1, // 3
+ ASPEED_HICRA_UART3, // 4
+ ASPEED_HICRA_UART4, // 5
+ ASPEED_HICRA_UART1, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector uart1_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_UART1),
+ .shift = 16,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_IO1, // 0
+ ASPEED_HICRA_IO2, // 1
+ ASPEED_HICRA_IO3, // 2
+ ASPEED_HICRA_IO4, // 3
+ ASPEED_HICRA_UART2, // 4
+ ASPEED_HICRA_UART3, // 5
+ ASPEED_HICRA_UART4, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector io5_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_IO5),
+ .shift = 12,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_UART5, // 0
+ ASPEED_HICRA_UART1, // 1
+ ASPEED_HICRA_UART2, // 2
+ ASPEED_HICRA_UART3, // 3
+ ASPEED_HICRA_UART4, // 4
+ ASPEED_HICRA_IO1, // 5
+ ASPEED_HICRA_IO3, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector io4_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_IO4),
+ .shift = 9,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_UART4, // 0
+ ASPEED_HICRA_UART5, // 1
+ ASPEED_HICRA_UART1, // 2
+ ASPEED_HICRA_UART2, // 3
+ ASPEED_HICRA_UART3, // 4
+ ASPEED_HICRA_IO1, // 5
+ ASPEED_HICRA_IO2, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector io3_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_IO3),
+ .shift = 6,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_UART3, // 0
+ ASPEED_HICRA_UART4, // 1
+ ASPEED_HICRA_UART5, // 2
+ ASPEED_HICRA_UART1, // 3
+ ASPEED_HICRA_UART2, // 4
+ ASPEED_HICRA_IO1, // 5
+ ASPEED_HICRA_IO2, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector io2_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_IO2),
+ .shift = 3,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_UART2, // 0
+ ASPEED_HICRA_UART3, // 1
+ ASPEED_HICRA_UART4, // 2
+ ASPEED_HICRA_UART5, // 3
+ ASPEED_HICRA_UART1, // 4
+ ASPEED_HICRA_IO3, // 5
+ ASPEED_HICRA_IO4, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+static struct aspeed_uart_routing_selector io1_sel = {
+ .dev_attr = ROUTING_ATTR(ASPEED_HICRA_IO1),
+ .shift = 0,
+ .mask = 0x7,
+ .options = {
+ ASPEED_HICRA_UART1, // 0
+ ASPEED_HICRA_UART2, // 1
+ ASPEED_HICRA_UART3, // 2
+ ASPEED_HICRA_UART4, // 3
+ ASPEED_HICRA_UART5, // 4
+ ASPEED_HICRA_IO3, // 5
+ ASPEED_HICRA_IO4, // 6
+ ASPEED_HICRA_IO6, // 7
+ NULL, // NULL termination
+ },
+};
+
+
+static struct attribute *aspeed_uart_routing_attrs[] = {
+ &uart1_sel.dev_attr.attr,
+ &uart2_sel.dev_attr.attr,
+ &uart3_sel.dev_attr.attr,
+ &uart4_sel.dev_attr.attr,
+ &uart5_sel.dev_attr.attr,
+ &io1_sel.dev_attr.attr,
+ &io2_sel.dev_attr.attr,
+ &io3_sel.dev_attr.attr,
+ &io4_sel.dev_attr.attr,
+ &io5_sel.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group aspeed_uart_routing_attr_group = {
+ .attrs = aspeed_uart_routing_attrs,
+};
+
+static ssize_t aspeed_uart_routing_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct aspeed_uart_routing *uart_routing = dev_get_drvdata(dev);
+ struct aspeed_uart_routing_selector *sel = to_routing_selector(attr);
+ int val, pos, len;
+
+ val = (readl(uart_routing->regs) >> sel->shift) & sel->mask;
+
+ len = 0;
+ for (pos = 0; sel->options[pos] != NULL; ++pos) {
+ if (pos == val) {
+ len += snprintf(buf + len, PAGE_SIZE - 1 - len,
+ "[%s] ", sel->options[pos]);
+ } else {
+ len += snprintf(buf + len, PAGE_SIZE - 1 - len,
+ "%s ", sel->options[pos]);
+ }
+ }
+
+ if (val >= pos) {
+ len += snprintf(buf + len, PAGE_SIZE - 1 - len,
+ "[unknown(%d)]", val);
+ }
+
+ len += snprintf(buf + len, PAGE_SIZE - 1 - len, "\n");
+
+ return len;
+}
+
+static ssize_t aspeed_uart_routing_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct aspeed_uart_routing *uart_routing = dev_get_drvdata(dev);
+ struct aspeed_uart_routing_selector *sel = to_routing_selector(attr);
+ int val;
+ u32 reg;
+
+ val = match_string(sel->options, -1, buf);
+ if (val < 0) {
+ dev_err(dev, "invalid value \"%s\"\n", buf);
+ return -EINVAL;
+ }
+
+ spin_lock(&uart_routing->lock);
+ reg = readl(uart_routing->regs);
+ // Zero out existing value in specified bits.
+ reg &= ~(sel->mask << sel->shift);
+ // Set new value in specified bits.
+ reg |= (val & sel->mask) << sel->shift;
+ writel(reg, uart_routing->regs);
+ spin_unlock(&uart_routing->lock);
+
+ return count;
+}
+
+static int aspeed_uart_routing_probe(struct platform_device *pdev)
+{
+ struct aspeed_uart_routing *uart_routing;
+ struct resource *res;
+ int rc;
+
+ uart_routing = devm_kzalloc(&pdev->dev,
+ sizeof(*uart_routing),
+ GFP_KERNEL);
+ if (!uart_routing)
+ return -ENOMEM;
+
+ spin_lock_init(&uart_routing->lock);
+ uart_routing->dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ uart_routing->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(uart_routing->regs))
+ return PTR_ERR(uart_routing->regs);
+
+ rc = sysfs_create_group(&uart_routing->dev->kobj,
+ &aspeed_uart_routing_attr_group);
+ if (rc < 0)
+ return rc;
+
+ platform_set_drvdata(pdev, uart_routing);
+
+ return 0;
+}
+
+static int aspeed_uart_routing_remove(struct platform_device *pdev)
+{
+ struct aspeed_uart_routing *uart_routing = platform_get_drvdata(pdev);
+
+ sysfs_remove_group(&uart_routing->dev->kobj,
+ &aspeed_uart_routing_attr_group);
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_uart_routing_table[] = {
+ { .compatible = "aspeed,ast2500-uart-routing" },
+ { },
+};
+
+static struct platform_driver aspeed_uart_routing_driver = {
+ .driver = {
+ .name = "aspeed-uart-routing",
+ .of_match_table = aspeed_uart_routing_table,
+ },
+ .probe = aspeed_uart_routing_probe,
+ .remove = aspeed_uart_routing_remove,
+};
+
+module_platform_driver(aspeed_uart_routing_driver);
+
+MODULE_AUTHOR("Oskar Senft <osk@google.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Driver to configure Aspeed UART routing");
--
2.19.0.rc1.350.ge57e33dbd1-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox