| Message ID | 20190116193958.6049-1-jae.hyun.yoo@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: aspeed: Add multi-master use case support | expand |
Hi Brendan, Can you please review this patch? Thanks, Jae On 1/16/2019 11:39 AM, Jae Hyun Yoo wrote: > In multi-master environment, this driver's master cannot know > exactly when a peer master sends data to this driver's slave so > cases can be happened that this master tries sending data through > the master_xfer function but slave data from a peer master is still > being processed or slave xfer is started by a peer immediately > after it queues a master command. To support multi-master use cases > properly, this H/W provides arbitration in physical level and it > provides priority based command handling too to avoid conflicts in > multi-master environment, means that if a master and a slave events > happen at the same time, H/W will handle a higher priority event > first and a pending event will be handled when bus comes back to > the idle state. > > To support this H/W feature properly, this patch adds the 'pending' > state of master and its handling code so that the pending master > xfer can be continued after slave operation properly. > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > --- > drivers/i2c/busses/i2c-aspeed.c | 118 +++++++++++++++++++++++++------- > 1 file changed, 92 insertions(+), 26 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 8dc9161ced38..9253ebccb008 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -117,6 +117,7 @@ > > enum aspeed_i2c_master_state { > ASPEED_I2C_MASTER_INACTIVE, > + ASPEED_I2C_MASTER_PENDING, > ASPEED_I2C_MASTER_START, > ASPEED_I2C_MASTER_TX_FIRST, > ASPEED_I2C_MASTER_TX, > @@ -126,12 +127,13 @@ enum aspeed_i2c_master_state { > }; > > enum aspeed_i2c_slave_state { > - ASPEED_I2C_SLAVE_STOP, > + ASPEED_I2C_SLAVE_INACTIVE, > ASPEED_I2C_SLAVE_START, > ASPEED_I2C_SLAVE_READ_REQUESTED, > ASPEED_I2C_SLAVE_READ_PROCESSED, > ASPEED_I2C_SLAVE_WRITE_REQUESTED, > ASPEED_I2C_SLAVE_WRITE_RECEIVED, > + ASPEED_I2C_SLAVE_STOP, > }; > > struct aspeed_i2c_bus { > @@ -156,6 +158,8 @@ struct aspeed_i2c_bus { > int cmd_err; > /* Protected only by i2c_lock_bus */ > int master_xfer_result; > + /* Multi-master */ > + bool multi_master; > #if IS_ENABLED(CONFIG_I2C_SLAVE) > struct i2c_client *slave; > enum aspeed_i2c_slave_state slave_state; > @@ -251,7 +255,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > } > > /* Slave is not currently active, irq was for someone else. */ > - if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) > + if (bus->slave_state == ASPEED_I2C_SLAVE_INACTIVE) > return irq_handled; > > dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n", > @@ -277,16 +281,15 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP; > bus->slave_state = ASPEED_I2C_SLAVE_STOP; > } > - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) { > + if (irq_status & ASPEED_I2CD_INTR_TX_NAK && > + bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) { > 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) > + if (unlikely(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); > @@ -294,9 +297,12 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG); > break; > case ASPEED_I2C_SLAVE_READ_PROCESSED: > - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK)) > + if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) { > dev_err(bus->dev, > "Expected ACK after processed read.\n"); > + break; > + } > + irq_handled |= ASPEED_I2CD_INTR_TX_ACK; > i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value); > writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG); > writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG); > @@ -310,10 +316,15 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > break; > case ASPEED_I2C_SLAVE_STOP: > i2c_slave_event(slave, I2C_SLAVE_STOP, &value); > + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; > + break; > + case ASPEED_I2C_SLAVE_START: > + /* Slave was just started. Waiting for the next event. */; > break; > default: > - dev_err(bus->dev, "unhandled slave_state: %d\n", > + dev_err(bus->dev, "unknown slave_state: %d\n", > bus->slave_state); > + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; > break; > } > > @@ -329,6 +340,17 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus) > u8 slave_addr = i2c_8bit_addr_from_msg(msg); > > bus->master_state = ASPEED_I2C_MASTER_START; > + > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > + /* > + * If it's requested in the middle of a slave session, set the master > + * state to 'pending' then H/W will continue handling this master > + * command when the bus comes back to the idle state. > + */ > + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) > + bus->master_state = ASPEED_I2C_MASTER_PENDING; > +#endif /* CONFIG_I2C_SLAVE */ > + > bus->buf_index = 0; > > if (msg->flags & I2C_M_RD) { > @@ -384,10 +406,6 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > bus->master_state = ASPEED_I2C_MASTER_INACTIVE; > 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; > } > > /* > @@ -399,11 +417,32 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > 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; > + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) { > + bus->cmd_err = ret; > + bus->master_state = ASPEED_I2C_MASTER_INACTIVE; > + goto out_complete; > + } > + } > + > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > + /* > + * A pending master command will be started by H/W when the bus comes > + * back to idle state after completing a slave operation so change the > + * master state from 'pending' to 'start' at here if slave is inactive. > + */ > + if (bus->master_state == ASPEED_I2C_MASTER_PENDING) { > + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) > + goto out_no_complete; > + > + bus->master_state = ASPEED_I2C_MASTER_START; > } > +#endif /* CONFIG_I2C_SLAVE */ > + > + /* Master is not currently active, irq was for someone else. */ > + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE || > + bus->master_state == ASPEED_I2C_MASTER_PENDING) > + goto out_no_complete; > > /* We are in an invalid state; reset bus to a known state. */ > if (!bus->msgs) { > @@ -423,6 +462,20 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > * then update the state and handle the new state below. > */ > if (bus->master_state == ASPEED_I2C_MASTER_START) { > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > + /* > + * If a peer master starts a xfer immediately after it queues a > + * master command, change its state to 'pending' then H/W will > + * continue the queued master xfer just after completing the > + * slave mode session. > + */ > + if (unlikely(irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH)) { > + bus->master_state = ASPEED_I2C_MASTER_PENDING; > + dev_dbg(bus->dev, > + "master goes pending due to a slave start\n"); > + goto out_no_complete; > + } > +#endif /* CONFIG_I2C_SLAVE */ > if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) { > if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_NAK))) { > bus->cmd_err = -ENXIO; > @@ -566,7 +619,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) > * interrupt bits. Each case needs to be handled using corresponding > * handlers depending on the current state. > */ > - if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) { > + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE && > + bus->master_state != ASPEED_I2C_MASTER_PENDING) { > irq_handled = aspeed_i2c_master_irq(bus, irq_remaining); > irq_remaining &= ~irq_handled; > if (irq_remaining) > @@ -601,15 +655,15 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, > { > struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); > unsigned long time_left, flags; > - int ret = 0; > + int ret; > > 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) { > + /* If bus is busy in a single master environment, attempt recovery. */ > + if (!bus->multi_master && > + (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) > @@ -629,10 +683,20 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, > time_left = wait_for_completion_timeout(&bus->cmd_complete, > bus->adap.timeout); > > - if (time_left == 0) > + if (time_left == 0) { > + /* > + * If timed out and bus is still busy in a multi master > + * environment, attempt recovery at here. > + */ > + if (bus->multi_master && > + (readl(bus->base + ASPEED_I2C_CMD_REG) & > + ASPEED_I2CD_BUS_BUSY_STS)) > + ret = aspeed_i2c_recover_bus(bus); > + > return -ETIMEDOUT; > - else > - return bus->master_xfer_result; > + } > + > + return bus->master_xfer_result; > } > > static u32 aspeed_i2c_functionality(struct i2c_adapter *adap) > @@ -672,7 +736,7 @@ static int aspeed_i2c_reg_slave(struct i2c_client *client) > __aspeed_i2c_reg_slave(bus, client->addr); > > bus->slave = client; > - bus->slave_state = ASPEED_I2C_SLAVE_STOP; > + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; > spin_unlock_irqrestore(&bus->lock, flags); > > return 0; > @@ -827,7 +891,9 @@ 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; > + else > fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS; > > /* Enable Master Mode */ >
On Wed, Jan 16, 2019 at 11:40 AM Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote: > > In multi-master environment, this driver's master cannot know > exactly when a peer master sends data to this driver's slave so > cases can be happened that this master tries sending data through > the master_xfer function but slave data from a peer master is still > being processed or slave xfer is started by a peer immediately > after it queues a master command. To support multi-master use cases > properly, this H/W provides arbitration in physical level and it > provides priority based command handling too to avoid conflicts in > multi-master environment, means that if a master and a slave events > happen at the same time, H/W will handle a higher priority event > first and a pending event will be handled when bus comes back to > the idle state. > > To support this H/W feature properly, this patch adds the 'pending' > state of master and its handling code so that the pending master > xfer can be continued after slave operation properly. > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > --- > drivers/i2c/busses/i2c-aspeed.c | 118 +++++++++++++++++++++++++------- > 1 file changed, 92 insertions(+), 26 deletions(-) > Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
On Wed, Jan 16, 2019 at 11:39:58AM -0800, Jae Hyun Yoo wrote: > In multi-master environment, this driver's master cannot know > exactly when a peer master sends data to this driver's slave so > cases can be happened that this master tries sending data through > the master_xfer function but slave data from a peer master is still > being processed or slave xfer is started by a peer immediately > after it queues a master command. To support multi-master use cases > properly, this H/W provides arbitration in physical level and it > provides priority based command handling too to avoid conflicts in > multi-master environment, means that if a master and a slave events > happen at the same time, H/W will handle a higher priority event > first and a pending event will be handled when bus comes back to > the idle state. > > To support this H/W feature properly, this patch adds the 'pending' > state of master and its handling code so that the pending master > xfer can be continued after slave operation properly. > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> Hmmm, the #ifdeffery quite increases with this patch. Maybe it is easier to select I2C_SLAVE in Kconfig? I'll let you decide. My code checkers found this: CPPCHECK drivers/i2c/busses/i2c-aspeed.c:694:10: warning: Variable 'ret' is assigned a value that is never used. [unreadVariable]
> Hmmm, the #ifdeffery quite increases with this patch. Maybe it is easier > to select I2C_SLAVE in Kconfig? I'll let you decide. > > My code checkers found this: > > CPPCHECK > drivers/i2c/busses/i2c-aspeed.c:694:10: warning: Variable 'ret' is assigned a value that is never used. [unreadVariable] > Hi Wolfram, Yes, cppcheck pointed it out correctly. The assignment on the variable 'ret' is not needed at there. Actually, the code should not be wrapped using I2C_SLAVE because a bus can also be in multi-master environment even when slave is not enabled. So the warning can be removed by below fix. --- diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 9253ebccb008..882c4ab1d18a 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -655,7 +655,6 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, { struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); unsigned long time_left, flags; - int ret; spin_lock_irqsave(&bus->lock, flags); bus->cmd_err = 0; @@ -664,6 +663,8 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, if (!bus->multi_master && (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS)) { + int ret; + spin_unlock_irqrestore(&bus->lock, flags); ret = aspeed_i2c_recover_bus(bus); if (ret) @@ -691,7 +692,7 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, if (bus->multi_master && (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS)) - ret = aspeed_i2c_recover_bus(bus); + aspeed_i2c_recover_bus(bus); return -ETIMEDOUT; --- The reason of adding changes at line 658 and 667 is to remove below cppcheck warning when the change at 694 is applied. [drivers/i2c/busses/i2c-aspeed.c:670]: (style) The scope of the variable 'ret' can be reduced. Can you please apply above changes on top of this patch? Or I can submit v2 with it. Thanks, Jae
> Can you please apply above changes on top of this patch? Or I can submit > v2 with it. Please send v2.
On 2/11/2019 10:16 AM, Wolfram Sang wrote: > >> Can you please apply above changes on top of this patch? Or I can submit >> v2 with it. > > Please send v2. > Okay, I'll send v2. Thanks, Jae
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 8dc9161ced38..9253ebccb008 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -117,6 +117,7 @@ enum aspeed_i2c_master_state { ASPEED_I2C_MASTER_INACTIVE, + ASPEED_I2C_MASTER_PENDING, ASPEED_I2C_MASTER_START, ASPEED_I2C_MASTER_TX_FIRST, ASPEED_I2C_MASTER_TX, @@ -126,12 +127,13 @@ enum aspeed_i2c_master_state { }; enum aspeed_i2c_slave_state { - ASPEED_I2C_SLAVE_STOP, + ASPEED_I2C_SLAVE_INACTIVE, ASPEED_I2C_SLAVE_START, ASPEED_I2C_SLAVE_READ_REQUESTED, ASPEED_I2C_SLAVE_READ_PROCESSED, ASPEED_I2C_SLAVE_WRITE_REQUESTED, ASPEED_I2C_SLAVE_WRITE_RECEIVED, + ASPEED_I2C_SLAVE_STOP, }; struct aspeed_i2c_bus { @@ -156,6 +158,8 @@ struct aspeed_i2c_bus { int cmd_err; /* Protected only by i2c_lock_bus */ int master_xfer_result; + /* Multi-master */ + bool multi_master; #if IS_ENABLED(CONFIG_I2C_SLAVE) struct i2c_client *slave; enum aspeed_i2c_slave_state slave_state; @@ -251,7 +255,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) } /* Slave is not currently active, irq was for someone else. */ - if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) + if (bus->slave_state == ASPEED_I2C_SLAVE_INACTIVE) return irq_handled; dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n", @@ -277,16 +281,15 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP; bus->slave_state = ASPEED_I2C_SLAVE_STOP; } - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) { + if (irq_status & ASPEED_I2CD_INTR_TX_NAK && + bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) { 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) + if (unlikely(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); @@ -294,9 +297,12 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG); break; case ASPEED_I2C_SLAVE_READ_PROCESSED: - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK)) + if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) { dev_err(bus->dev, "Expected ACK after processed read.\n"); + break; + } + irq_handled |= ASPEED_I2CD_INTR_TX_ACK; i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value); writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG); writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG); @@ -310,10 +316,15 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) break; case ASPEED_I2C_SLAVE_STOP: i2c_slave_event(slave, I2C_SLAVE_STOP, &value); + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; + break; + case ASPEED_I2C_SLAVE_START: + /* Slave was just started. Waiting for the next event. */; break; default: - dev_err(bus->dev, "unhandled slave_state: %d\n", + dev_err(bus->dev, "unknown slave_state: %d\n", bus->slave_state); + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; break; } @@ -329,6 +340,17 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus) u8 slave_addr = i2c_8bit_addr_from_msg(msg); bus->master_state = ASPEED_I2C_MASTER_START; + +#if IS_ENABLED(CONFIG_I2C_SLAVE) + /* + * If it's requested in the middle of a slave session, set the master + * state to 'pending' then H/W will continue handling this master + * command when the bus comes back to the idle state. + */ + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) + bus->master_state = ASPEED_I2C_MASTER_PENDING; +#endif /* CONFIG_I2C_SLAVE */ + bus->buf_index = 0; if (msg->flags & I2C_M_RD) { @@ -384,10 +406,6 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) bus->master_state = ASPEED_I2C_MASTER_INACTIVE; 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; } /* @@ -399,11 +417,32 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) 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; + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) { + bus->cmd_err = ret; + bus->master_state = ASPEED_I2C_MASTER_INACTIVE; + goto out_complete; + } + } + +#if IS_ENABLED(CONFIG_I2C_SLAVE) + /* + * A pending master command will be started by H/W when the bus comes + * back to idle state after completing a slave operation so change the + * master state from 'pending' to 'start' at here if slave is inactive. + */ + if (bus->master_state == ASPEED_I2C_MASTER_PENDING) { + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) + goto out_no_complete; + + bus->master_state = ASPEED_I2C_MASTER_START; } +#endif /* CONFIG_I2C_SLAVE */ + + /* Master is not currently active, irq was for someone else. */ + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE || + bus->master_state == ASPEED_I2C_MASTER_PENDING) + goto out_no_complete; /* We are in an invalid state; reset bus to a known state. */ if (!bus->msgs) { @@ -423,6 +462,20 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) * then update the state and handle the new state below. */ if (bus->master_state == ASPEED_I2C_MASTER_START) { +#if IS_ENABLED(CONFIG_I2C_SLAVE) + /* + * If a peer master starts a xfer immediately after it queues a + * master command, change its state to 'pending' then H/W will + * continue the queued master xfer just after completing the + * slave mode session. + */ + if (unlikely(irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH)) { + bus->master_state = ASPEED_I2C_MASTER_PENDING; + dev_dbg(bus->dev, + "master goes pending due to a slave start\n"); + goto out_no_complete; + } +#endif /* CONFIG_I2C_SLAVE */ if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) { if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_NAK))) { bus->cmd_err = -ENXIO; @@ -566,7 +619,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) * interrupt bits. Each case needs to be handled using corresponding * handlers depending on the current state. */ - if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) { + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE && + bus->master_state != ASPEED_I2C_MASTER_PENDING) { irq_handled = aspeed_i2c_master_irq(bus, irq_remaining); irq_remaining &= ~irq_handled; if (irq_remaining) @@ -601,15 +655,15 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, { struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); unsigned long time_left, flags; - int ret = 0; + int ret; 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) { + /* If bus is busy in a single master environment, attempt recovery. */ + if (!bus->multi_master && + (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) @@ -629,10 +683,20 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, time_left = wait_for_completion_timeout(&bus->cmd_complete, bus->adap.timeout); - if (time_left == 0) + if (time_left == 0) { + /* + * If timed out and bus is still busy in a multi master + * environment, attempt recovery at here. + */ + if (bus->multi_master && + (readl(bus->base + ASPEED_I2C_CMD_REG) & + ASPEED_I2CD_BUS_BUSY_STS)) + ret = aspeed_i2c_recover_bus(bus); + return -ETIMEDOUT; - else - return bus->master_xfer_result; + } + + return bus->master_xfer_result; } static u32 aspeed_i2c_functionality(struct i2c_adapter *adap) @@ -672,7 +736,7 @@ static int aspeed_i2c_reg_slave(struct i2c_client *client) __aspeed_i2c_reg_slave(bus, client->addr); bus->slave = client; - bus->slave_state = ASPEED_I2C_SLAVE_STOP; + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; spin_unlock_irqrestore(&bus->lock, flags); return 0; @@ -827,7 +891,9 @@ 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; + else fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS; /* Enable Master Mode */
In multi-master environment, this driver's master cannot know exactly when a peer master sends data to this driver's slave so cases can be happened that this master tries sending data through the master_xfer function but slave data from a peer master is still being processed or slave xfer is started by a peer immediately after it queues a master command. To support multi-master use cases properly, this H/W provides arbitration in physical level and it provides priority based command handling too to avoid conflicts in multi-master environment, means that if a master and a slave events happen at the same time, H/W will handle a higher priority event first and a pending event will be handled when bus comes back to the idle state. To support this H/W feature properly, this patch adds the 'pending' state of master and its handling code so that the pending master xfer can be continued after slave operation properly. Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> --- drivers/i2c/busses/i2c-aspeed.c | 118 +++++++++++++++++++++++++------- 1 file changed, 92 insertions(+), 26 deletions(-)