* [PATCH] i2c: aspeed: fix master pending state handling
@ 2019-10-09 21:20 Jae Hyun Yoo
2019-10-10 0:32 ` Brendan Higgins
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Jae Hyun Yoo @ 2019-10-09 21:20 UTC (permalink / raw)
To: Brendan Higgins, Wolfram Sang, Benjamin Herrenschmidt,
Joel Stanley, Rob Herring, Mark Rutland, Andrew Jeffery, Tao Ren,
Cedric Le Goater
Cc: devicetree, Jae Hyun Yoo, linux-aspeed, openbmc, linux-i2c,
linux-arm-kernel
In case of master pending state, it should not trigger a master
command, otherwise data could be corrupted because this H/W shares
the same data buffer for slave and master operations. It also means
that H/W command queue handling is unreliable because of the buffer
sharing issue. To fix this issue, it clears command queue if a
master command is queued in pending state to use S/W solution
instead of H/W command queue handling. Also, it refines restarting
mechanism of the pending master command.
Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 54 +++++++++++++++++++++------------
1 file changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index fa66951b05d0..7b098ff5f5dd 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -108,6 +108,12 @@
#define ASPEED_I2CD_S_TX_CMD BIT(2)
#define ASPEED_I2CD_M_TX_CMD BIT(1)
#define ASPEED_I2CD_M_START_CMD BIT(0)
+#define ASPEED_I2CD_MASTER_CMDS_MASK \
+ (ASPEED_I2CD_M_STOP_CMD | \
+ ASPEED_I2CD_M_S_RX_CMD_LAST | \
+ ASPEED_I2CD_M_RX_CMD | \
+ ASPEED_I2CD_M_TX_CMD | \
+ ASPEED_I2CD_M_START_CMD)
/* 0x18 : I2CD Slave Device Address Register */
#define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
@@ -336,18 +342,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
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)
+ if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
bus->master_state = ASPEED_I2C_MASTER_PENDING;
+ return;
+ }
#endif /* CONFIG_I2C_SLAVE */
+ bus->master_state = ASPEED_I2C_MASTER_START;
bus->buf_index = 0;
if (msg->flags & I2C_M_RD) {
@@ -422,20 +429,6 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
}
}
-#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)
@@ -462,11 +455,15 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
#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.
+ * master command, clear the queued master command and change
+ * its state to 'pending'. To simplify handling of pending
+ * cases, it uses S/W solution instead of H/W command queue
+ * handling.
*/
if (unlikely(irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH)) {
+ writel(readl(bus->base + ASPEED_I2C_CMD_REG) &
+ ~ASPEED_I2CD_MASTER_CMDS_MASK,
+ bus->base + ASPEED_I2C_CMD_REG);
bus->master_state = ASPEED_I2C_MASTER_PENDING;
dev_dbg(bus->dev,
"master goes pending due to a slave start\n");
@@ -629,6 +626,14 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
irq_handled |= aspeed_i2c_master_irq(bus,
irq_remaining);
}
+
+ /*
+ * Start a pending master command at here if a slave operation is
+ * completed.
+ */
+ if (bus->master_state == ASPEED_I2C_MASTER_PENDING &&
+ bus->slave_state == ASPEED_I2C_SLAVE_INACTIVE)
+ aspeed_i2c_do_start(bus);
#else
irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
#endif /* CONFIG_I2C_SLAVE */
@@ -691,6 +696,15 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
ASPEED_I2CD_BUS_BUSY_STS))
aspeed_i2c_recover_bus(bus);
+ /*
+ * If timed out and the state is still pending, drop the pending
+ * master command.
+ */
+ spin_lock_irqsave(&bus->lock, flags);
+ if (bus->master_state == ASPEED_I2C_MASTER_PENDING)
+ bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ spin_unlock_irqrestore(&bus->lock, flags);
+
return -ETIMEDOUT;
}
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-09 21:20 [PATCH] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
@ 2019-10-10 0:32 ` Brendan Higgins
2019-10-10 1:26 ` Tao Ren
2019-10-10 6:37 ` Wolfram Sang
2019-10-10 6:48 ` Joel Stanley
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Brendan Higgins @ 2019-10-10 0:32 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Wolfram Sang,
Andrew Jeffery, Benjamin Herrenschmidt, openbmc, linux-i2c,
Rob Herring, Joel Stanley, Tao Ren, linux-arm-kernel,
Cedric Le Goater
On Wed, Oct 09, 2019 at 02:20:34PM -0700, Jae Hyun Yoo wrote:
> In case of master pending state, it should not trigger a master
> command, otherwise data could be corrupted because this H/W shares
> the same data buffer for slave and master operations. It also means
> that H/W command queue handling is unreliable because of the buffer
> sharing issue. To fix this issue, it clears command queue if a
> master command is queued in pending state to use S/W solution
> instead of H/W command queue handling. Also, it refines restarting
> mechanism of the pending master command.
>
> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
We don't have any multi-master setups, can we get a Tested-by?
Wolfram, since this is a bugfix, can we get this in 5.4?
Thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 0:32 ` Brendan Higgins
@ 2019-10-10 1:26 ` Tao Ren
2019-10-10 6:37 ` Wolfram Sang
1 sibling, 0 replies; 15+ messages in thread
From: Tao Ren @ 2019-10-10 1:26 UTC (permalink / raw)
To: Brendan Higgins, Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Wolfram Sang,
Andrew Jeffery, Benjamin Herrenschmidt, openbmc, linux-i2c,
Rob Herring, Joel Stanley, linux-arm-kernel, Cedric Le Goater
On 10/9/19 5:32 PM, Brendan Higgins wrote:
> On Wed, Oct 09, 2019 at 02:20:34PM -0700, Jae Hyun Yoo wrote:
>> In case of master pending state, it should not trigger a master
>> command, otherwise data could be corrupted because this H/W shares
>> the same data buffer for slave and master operations. It also means
>> that H/W command queue handling is unreliable because of the buffer
>> sharing issue. To fix this issue, it clears command queue if a
>> master command is queued in pending state to use S/W solution
>> instead of H/W command queue handling. Also, it refines restarting
>> mechanism of the pending master command.
>>
>> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>
> We don't have any multi-master setups, can we get a Tested-by?
I've applied the patch to my tree and I'm looking for a minipack BMC
(milti-master) to test the patch. Will come back with results tomorrow.
Cheers,
Tao
> Wolfram, since this is a bugfix, can we get this in 5.4?
>
> Thanks!
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 0:32 ` Brendan Higgins
2019-10-10 1:26 ` Tao Ren
@ 2019-10-10 6:37 ` Wolfram Sang
1 sibling, 0 replies; 15+ messages in thread
From: Wolfram Sang @ 2019-10-10 6:37 UTC (permalink / raw)
To: Brendan Higgins
Cc: Mark Rutland, devicetree, Jae Hyun Yoo, linux-aspeed,
Andrew Jeffery, Benjamin Herrenschmidt, openbmc, linux-i2c,
Rob Herring, Joel Stanley, Tao Ren, linux-arm-kernel,
Cedric Le Goater
[-- Attachment #1.1: Type: text/plain, Size: 122 bytes --]
> Wolfram, since this is a bugfix, can we get this in 5.4?
Of course! Just giving Tao Ren some time for the Tested-by.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-09 21:20 [PATCH] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
2019-10-10 0:32 ` Brendan Higgins
@ 2019-10-10 6:48 ` Joel Stanley
2019-10-10 18:22 ` Jae Hyun Yoo
2019-10-10 21:20 ` Tao Ren
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Joel Stanley @ 2019-10-10 6:48 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Wolfram Sang,
Andrew Jeffery, Benjamin Herrenschmidt, OpenBMC Maillist,
Brendan Higgins, Rob Herring, Cedric Le Goater, Tao Ren,
Linux ARM, linux-i2c
On Wed, 9 Oct 2019 at 21:20, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>
> In case of master pending state, it should not trigger a master
> command, otherwise data could be corrupted because this H/W shares
> the same data buffer for slave and master operations. It also means
> that H/W command queue handling is unreliable because of the buffer
> sharing issue. To fix this issue, it clears command queue if a
> master command is queued in pending state to use S/W solution
> instead of H/W command queue handling. Also, it refines restarting
> mechanism of the pending master command.
>
> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Acked-by: Joel Stanley <joel@jms.id.au>
While reviewing I was concerned about the locking in
aspeed_i2c_master_xfer. It's a bit hairy, and I am not convinced it is
without bugs.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 6:48 ` Joel Stanley
@ 2019-10-10 18:22 ` Jae Hyun Yoo
0 siblings, 0 replies; 15+ messages in thread
From: Jae Hyun Yoo @ 2019-10-10 18:22 UTC (permalink / raw)
To: Joel Stanley
Cc: Mark Rutland, devicetree, linux-aspeed, Wolfram Sang,
Andrew Jeffery, Benjamin Herrenschmidt, OpenBMC Maillist,
Brendan Higgins, Rob Herring, Cedric Le Goater, Tao Ren,
Linux ARM, linux-i2c
> Acked-by: Joel Stanley <joel@jms.id.au>
Thanks!
> While reviewing I was concerned about the locking in
> aspeed_i2c_master_xfer. It's a bit hairy, and I am not convinced it is
> without bugs.
The locking was already in there in aspeed_i2c_master_xfer but it
unlocks the protection to make a waiting for a completion. This code
is after the waiting so I added the locking to protect
'bus->master_state' variable which can be accessed from driver context
and from interrupt context.
Thanks,
Jae
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-09 21:20 [PATCH] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
2019-10-10 0:32 ` Brendan Higgins
2019-10-10 6:48 ` Joel Stanley
@ 2019-10-10 21:20 ` Tao Ren
2019-10-10 22:04 ` Jae Hyun Yoo
2019-10-10 23:29 ` Tao Ren
2019-10-21 12:09 ` Wolfram Sang
4 siblings, 1 reply; 15+ messages in thread
From: Tao Ren @ 2019-10-10 21:20 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Wolfram Sang,
Benjamin Herrenschmidt, Joel Stanley, Rob Herring, Mark Rutland,
Andrew Jeffery, Cedric Le Goater
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/9/19 2:20 PM, Jae Hyun Yoo wrote:
> In case of master pending state, it should not trigger a master
> command, otherwise data could be corrupted because this H/W shares
> the same data buffer for slave and master operations. It also means
> that H/W command queue handling is unreliable because of the buffer
> sharing issue. To fix this issue, it clears command queue if a
> master command is queued in pending state to use S/W solution
> instead of H/W command queue handling. Also, it refines restarting
> mechanism of the pending master command.
>
> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 54 +++++++++++++++++++++------------
> 1 file changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index fa66951b05d0..7b098ff5f5dd 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -108,6 +108,12 @@
> #define ASPEED_I2CD_S_TX_CMD BIT(2)
> #define ASPEED_I2CD_M_TX_CMD BIT(1)
> #define ASPEED_I2CD_M_START_CMD BIT(0)
> +#define ASPEED_I2CD_MASTER_CMDS_MASK \
> + (ASPEED_I2CD_M_STOP_CMD | \
> + ASPEED_I2CD_M_S_RX_CMD_LAST | \
> + ASPEED_I2CD_M_RX_CMD | \
> + ASPEED_I2CD_M_TX_CMD | \
> + ASPEED_I2CD_M_START_CMD)
>
> /* 0x18 : I2CD Slave Device Address Register */
> #define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
> @@ -336,18 +342,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
> struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> 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)
> + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
> bus->master_state = ASPEED_I2C_MASTER_PENDING;
> + return;
> + }
> #endif /* CONFIG_I2C_SLAVE */
>
> + bus->master_state = ASPEED_I2C_MASTER_START;
> bus->buf_index = 0;
>
> if (msg->flags & I2C_M_RD) {
> @@ -422,20 +429,6 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> }
> }
>
> -#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)
> @@ -462,11 +455,15 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> #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.
> + * master command, clear the queued master command and change
> + * its state to 'pending'. To simplify handling of pending
> + * cases, it uses S/W solution instead of H/W command queue
> + * handling.
> */
> if (unlikely(irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH)) {
> + writel(readl(bus->base + ASPEED_I2C_CMD_REG) &
> + ~ASPEED_I2CD_MASTER_CMDS_MASK,
> + bus->base + ASPEED_I2C_CMD_REG);
Sorry for the late comments (just noticed this line while testing the patch):
I assume this line is aimed at stopping the running master commands, but as per
AST2500 datasheet, it's NOP to write 0 to MASTER_STOP/MASTER_RX/MASTER_TX bits.
Maybe all we need is writing 1 to MASTER_STOP field?
Cheers,
Tao
> bus->master_state = ASPEED_I2C_MASTER_PENDING;
> dev_dbg(bus->dev,
> "master goes pending due to a slave start\n");
> @@ -629,6 +626,14 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> irq_handled |= aspeed_i2c_master_irq(bus,
> irq_remaining);
> }
> +
> + /*
> + * Start a pending master command at here if a slave operation is
> + * completed.
> + */
> + if (bus->master_state == ASPEED_I2C_MASTER_PENDING &&
> + bus->slave_state == ASPEED_I2C_SLAVE_INACTIVE)
> + aspeed_i2c_do_start(bus);
> #else
> irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
> #endif /* CONFIG_I2C_SLAVE */
> @@ -691,6 +696,15 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> ASPEED_I2CD_BUS_BUSY_STS))
> aspeed_i2c_recover_bus(bus);
>
> + /*
> + * If timed out and the state is still pending, drop the pending
> + * master command.
> + */
> + spin_lock_irqsave(&bus->lock, flags);
> + if (bus->master_state == ASPEED_I2C_MASTER_PENDING)
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> return -ETIMEDOUT;
> }
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 21:20 ` Tao Ren
@ 2019-10-10 22:04 ` Jae Hyun Yoo
2019-10-10 23:11 ` Tao Ren
0 siblings, 1 reply; 15+ messages in thread
From: Jae Hyun Yoo @ 2019-10-10 22:04 UTC (permalink / raw)
To: Tao Ren, Brendan Higgins, Wolfram Sang, Benjamin Herrenschmidt,
Joel Stanley, Rob Herring, Mark Rutland, Andrew Jeffery,
Cedric Le Goater
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/10/2019 2:20 PM, Tao Ren wrote:
> On 10/9/19 2:20 PM, Jae Hyun Yoo wrote:
[...]
>> /*
>> * 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.
>> + * master command, clear the queued master command and change
>> + * its state to 'pending'. To simplify handling of pending
>> + * cases, it uses S/W solution instead of H/W command queue
>> + * handling.
>> */
>> if (unlikely(irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH)) {
>> + writel(readl(bus->base + ASPEED_I2C_CMD_REG) &
>> + ~ASPEED_I2CD_MASTER_CMDS_MASK,
>> + bus->base + ASPEED_I2C_CMD_REG);
>
> Sorry for the late comments (just noticed this line while testing the patch):
>
> I assume this line is aimed at stopping the running master commands, but as per
> AST2500 datasheet, it's NOP to write 0 to MASTER_STOP/MASTER_RX/MASTER_TX bits.
> Maybe all we need is writing 1 to MASTER_STOP field?
There could be two pending cases:
1. Master goes to pending before it triggers a command if a slave
operation is already initiated.
2. Master goes to pending after it triggered a command if a peer
master immediately sends something just after the master command
triggering.
Above code is for the latter case. H/W handles the case priority based
so the slave event will be handled first, and then the master command
will be handled when the slave operation is completed. Problem is,
this H/W shares the same buffer for master and slave operations so
it's unreliable. Above code just removes the master command from the
command register to prevent this H/W command handling of pending events.
Instead, it restarts the master command using a call of
aspeed_i2c_do_start when the slave operation is completed.
Thanks,
Jae
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 22:04 ` Jae Hyun Yoo
@ 2019-10-10 23:11 ` Tao Ren
2019-10-10 23:16 ` Jae Hyun Yoo
0 siblings, 1 reply; 15+ messages in thread
From: Tao Ren @ 2019-10-10 23:11 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Wolfram Sang,
Benjamin Herrenschmidt, Joel Stanley, Rob Herring, Mark Rutland,
Andrew Jeffery, Cedric Le Goater
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/10/19 3:04 PM, Jae Hyun Yoo wrote:
> On 10/10/2019 2:20 PM, Tao Ren wrote:
>> On 10/9/19 2:20 PM, Jae Hyun Yoo wrote:
> [...]
>>> /*
>>> * 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.
>>> + * master command, clear the queued master command and change
>>> + * its state to 'pending'. To simplify handling of pending
>>> + * cases, it uses S/W solution instead of H/W command queue
>>> + * handling.
>>> */
>>> if (unlikely(irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH)) {
>>> + writel(readl(bus->base + ASPEED_I2C_CMD_REG) &
>>> + ~ASPEED_I2CD_MASTER_CMDS_MASK,
>>> + bus->base + ASPEED_I2C_CMD_REG);
>>
>> Sorry for the late comments (just noticed this line while testing the patch):
>>
>> I assume this line is aimed at stopping the running master commands, but as per
>> AST2500 datasheet, it's NOP to write 0 to MASTER_STOP/MASTER_RX/MASTER_TX bits.
>> Maybe all we need is writing 1 to MASTER_STOP field?
>
> There could be two pending cases:
> 1. Master goes to pending before it triggers a command if a slave
> operation is already initiated.
> 2. Master goes to pending after it triggered a command if a peer
> master immediately sends something just after the master command
> triggering.
>
> Above code is for the latter case. H/W handles the case priority based
> so the slave event will be handled first, and then the master command
> will be handled when the slave operation is completed. Problem is,
> this H/W shares the same buffer for master and slave operations so
> it's unreliable. Above code just removes the master command from the
> command register to prevent this H/W command handling of pending events.
> Instead, it restarts the master command using a call of aspeed_i2c_do_start when the slave operation is completed.
Thanks for the clarify, Jae. I mean clearing these bits has no effect to
hardware according to aspeed datasheet; in other word, master command cannot
be removed from command register by this statement.
For example, below is the description for MASTER_STOP_CMD(I2CD14, bit 5):
0: NOP
1: Issue Master Stop Command
This register will be automatically cleared by H/W when Stop Command has
been issues.
Cheers,
Tao
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 23:11 ` Tao Ren
@ 2019-10-10 23:16 ` Jae Hyun Yoo
2019-10-10 23:24 ` Tao Ren
0 siblings, 1 reply; 15+ messages in thread
From: Jae Hyun Yoo @ 2019-10-10 23:16 UTC (permalink / raw)
To: Tao Ren, Brendan Higgins, Wolfram Sang, Benjamin Herrenschmidt,
Joel Stanley, Rob Herring, Mark Rutland, Andrew Jeffery,
Cedric Le Goater
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/10/2019 4:11 PM, Tao Ren wrote:
> On 10/10/19 3:04 PM, Jae Hyun Yoo wrote:
>> On 10/10/2019 2:20 PM, Tao Ren wrote:
>>> On 10/9/19 2:20 PM, Jae Hyun Yoo wrote:
>> [...]
>>>> /*
>>>> * 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.
>>>> + * master command, clear the queued master command and change
>>>> + * its state to 'pending'. To simplify handling of pending
>>>> + * cases, it uses S/W solution instead of H/W command queue
>>>> + * handling.
>>>> */
>>>> if (unlikely(irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH)) {
>>>> + writel(readl(bus->base + ASPEED_I2C_CMD_REG) &
>>>> + ~ASPEED_I2CD_MASTER_CMDS_MASK,
>>>> + bus->base + ASPEED_I2C_CMD_REG);
>>>
>>> Sorry for the late comments (just noticed this line while testing the patch):
>>>
>>> I assume this line is aimed at stopping the running master commands, but as per
>>> AST2500 datasheet, it's NOP to write 0 to MASTER_STOP/MASTER_RX/MASTER_TX bits.
>>> Maybe all we need is writing 1 to MASTER_STOP field?
>>
>> There could be two pending cases:
>> 1. Master goes to pending before it triggers a command if a slave
>> operation is already initiated.
>> 2. Master goes to pending after it triggered a command if a peer
>> master immediately sends something just after the master command
>> triggering.
>>
>> Above code is for the latter case. H/W handles the case priority based
>> so the slave event will be handled first, and then the master command
>> will be handled when the slave operation is completed. Problem is,
>> this H/W shares the same buffer for master and slave operations so
>> it's unreliable. Above code just removes the master command from the
>> command register to prevent this H/W command handling of pending events.
>> Instead, it restarts the master command using a call of aspeed_i2c_do_start when the slave operation is completed.
>
> Thanks for the clarify, Jae. I mean clearing these bits has no effect to
> hardware according to aspeed datasheet; in other word, master command cannot
> be removed from command register by this statement.
>
> For example, below is the description for MASTER_STOP_CMD(I2CD14, bit 5):
>
> 0: NOP
> 1: Issue Master Stop Command
> This register will be automatically cleared by H/W when Stop Command has
> been issues.
It's removing before H/W fetches the the command so the pending command
isn't cleared by H/W at the timing. If we send a stop command at here,
the bus will be messed up.
Thanks,
Jae
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 23:16 ` Jae Hyun Yoo
@ 2019-10-10 23:24 ` Tao Ren
0 siblings, 0 replies; 15+ messages in thread
From: Tao Ren @ 2019-10-10 23:24 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Wolfram Sang,
Benjamin Herrenschmidt, Joel Stanley, Rob Herring, Mark Rutland,
Andrew Jeffery, Cedric Le Goater
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/10/19 4:16 PM, Jae Hyun Yoo wrote:
> On 10/10/2019 4:11 PM, Tao Ren wrote:
>> On 10/10/19 3:04 PM, Jae Hyun Yoo wrote:
>>> On 10/10/2019 2:20 PM, Tao Ren wrote:
>>>> On 10/9/19 2:20 PM, Jae Hyun Yoo wrote:
>>> [...]
>>>>> /*
>>>>> * 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.
>>>>> + * master command, clear the queued master command and change
>>>>> + * its state to 'pending'. To simplify handling of pending
>>>>> + * cases, it uses S/W solution instead of H/W command queue
>>>>> + * handling.
>>>>> */
>>>>> if (unlikely(irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH)) {
>>>>> + writel(readl(bus->base + ASPEED_I2C_CMD_REG) &
>>>>> + ~ASPEED_I2CD_MASTER_CMDS_MASK,
>>>>> + bus->base + ASPEED_I2C_CMD_REG);
>>>>
>>>> Sorry for the late comments (just noticed this line while testing the patch):
>>>>
>>>> I assume this line is aimed at stopping the running master commands, but as per
>>>> AST2500 datasheet, it's NOP to write 0 to MASTER_STOP/MASTER_RX/MASTER_TX bits.
>>>> Maybe all we need is writing 1 to MASTER_STOP field?
>>>
>>> There could be two pending cases:
>>> 1. Master goes to pending before it triggers a command if a slave
>>> operation is already initiated.
>>> 2. Master goes to pending after it triggered a command if a peer
>>> master immediately sends something just after the master command
>>> triggering.
>>>
>>> Above code is for the latter case. H/W handles the case priority based
>>> so the slave event will be handled first, and then the master command
>>> will be handled when the slave operation is completed. Problem is,
>>> this H/W shares the same buffer for master and slave operations so
>>> it's unreliable. Above code just removes the master command from the
>>> command register to prevent this H/W command handling of pending events.
>>> Instead, it restarts the master command using a call of aspeed_i2c_do_start when the slave operation is completed.
>>
>> Thanks for the clarify, Jae. I mean clearing these bits has no effect to
>> hardware according to aspeed datasheet; in other word, master command cannot
>> be removed from command register by this statement.
>>
>> For example, below is the description for MASTER_STOP_CMD(I2CD14, bit 5):
>>
>> 0: NOP
>> 1: Issue Master Stop Command
>> This register will be automatically cleared by H/W when Stop Command has
>> been issues.
>
> It's removing before H/W fetches the the command so the pending command
> isn't cleared by H/W at the timing. If we send a stop command at here, the bus will be messed up.
I see. I didn't know we could clear the bits before hardware fetches them.
Cheers,
Tao
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-09 21:20 [PATCH] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
` (2 preceding siblings ...)
2019-10-10 21:20 ` Tao Ren
@ 2019-10-10 23:29 ` Tao Ren
2019-10-10 23:52 ` Jae Hyun Yoo
2019-10-21 12:09 ` Wolfram Sang
4 siblings, 1 reply; 15+ messages in thread
From: Tao Ren @ 2019-10-10 23:29 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Wolfram Sang,
Benjamin Herrenschmidt, Joel Stanley, Rob Herring, Mark Rutland,
Andrew Jeffery, Cedric Le Goater
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/9/19 2:20 PM, Jae Hyun Yoo wrote:
> In case of master pending state, it should not trigger a master
> command, otherwise data could be corrupted because this H/W shares
> the same data buffer for slave and master operations. It also means
> that H/W command queue handling is unreliable because of the buffer
> sharing issue. To fix this issue, it clears command queue if a
> master command is queued in pending state to use S/W solution
> instead of H/W command queue handling. Also, it refines restarting
> mechanism of the pending master command.
>
> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Tested-by: Tao Ren <taoren@fb.com>
Tested the patch on Facebook Minipack BMC (multi-master i2c-0 environment) and
everything looks good to me.
Cheers,
Tao
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 23:29 ` Tao Ren
@ 2019-10-10 23:52 ` Jae Hyun Yoo
2019-10-16 19:20 ` Brendan Higgins
0 siblings, 1 reply; 15+ messages in thread
From: Jae Hyun Yoo @ 2019-10-10 23:52 UTC (permalink / raw)
To: Tao Ren, Brendan Higgins, Wolfram Sang, Benjamin Herrenschmidt,
Joel Stanley, Rob Herring, Mark Rutland, Andrew Jeffery,
Cedric Le Goater
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/10/2019 4:29 PM, Tao Ren wrote:
> On 10/9/19 2:20 PM, Jae Hyun Yoo wrote:
>> In case of master pending state, it should not trigger a master
>> command, otherwise data could be corrupted because this H/W shares
>> the same data buffer for slave and master operations. It also means
>> that H/W command queue handling is unreliable because of the buffer
>> sharing issue. To fix this issue, it clears command queue if a
>> master command is queued in pending state to use S/W solution
>> instead of H/W command queue handling. Also, it refines restarting
>> mechanism of the pending master command.
>>
>> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>
> Tested-by: Tao Ren <taoren@fb.com>
>
> Tested the patch on Facebook Minipack BMC (multi-master i2c-0 environment) and
> everything looks good to me.
Thanks for sharing the test result and your code reviews.
Best,
Jae
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-10 23:52 ` Jae Hyun Yoo
@ 2019-10-16 19:20 ` Brendan Higgins
0 siblings, 0 replies; 15+ messages in thread
From: Brendan Higgins @ 2019-10-16 19:20 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Wolfram Sang,
Andrew Jeffery, Tao Ren, openbmc, linux-i2c, Rob Herring,
Joel Stanley, Benjamin Herrenschmidt, linux-arm-kernel,
Cedric Le Goater
On Thu, Oct 10, 2019 at 4:52 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> On 10/10/2019 4:29 PM, Tao Ren wrote:
> > On 10/9/19 2:20 PM, Jae Hyun Yoo wrote:
> >> In case of master pending state, it should not trigger a master
> >> command, otherwise data could be corrupted because this H/W shares
> >> the same data buffer for slave and master operations. It also means
> >> that H/W command queue handling is unreliable because of the buffer
> >> sharing issue. To fix this issue, it clears command queue if a
> >> master command is queued in pending state to use S/W solution
> >> instead of H/W command queue handling. Also, it refines restarting
> >> mechanism of the pending master command.
> >>
> >> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
> >> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> >
> > Tested-by: Tao Ren <taoren@fb.com>
> >
> > Tested the patch on Facebook Minipack BMC (multi-master i2c-0 environment) and
> > everything looks good to me.
Looks like we got the necessary tested-bys and reviewed-bys. Can we
apply this for next?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] i2c: aspeed: fix master pending state handling
2019-10-09 21:20 [PATCH] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
` (3 preceding siblings ...)
2019-10-10 23:29 ` Tao Ren
@ 2019-10-21 12:09 ` Wolfram Sang
4 siblings, 0 replies; 15+ messages in thread
From: Wolfram Sang @ 2019-10-21 12:09 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, openbmc, Brendan Higgins, linux-i2c,
Rob Herring, Joel Stanley, Tao Ren, linux-arm-kernel,
Cedric Le Goater
[-- Attachment #1.1: Type: text/plain, Size: 764 bytes --]
On Wed, Oct 09, 2019 at 02:20:34PM -0700, Jae Hyun Yoo wrote:
> In case of master pending state, it should not trigger a master
> command, otherwise data could be corrupted because this H/W shares
> the same data buffer for slave and master operations. It also means
> that H/W command queue handling is unreliable because of the buffer
> sharing issue. To fix this issue, it clears command queue if a
> master command is queued in pending state to use S/W solution
> instead of H/W command queue handling. Also, it refines restarting
> mechanism of the pending master command.
>
> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Applied to for-current, thanks!
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-10-21 12:10 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 21:20 [PATCH] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
2019-10-10 0:32 ` Brendan Higgins
2019-10-10 1:26 ` Tao Ren
2019-10-10 6:37 ` Wolfram Sang
2019-10-10 6:48 ` Joel Stanley
2019-10-10 18:22 ` Jae Hyun Yoo
2019-10-10 21:20 ` Tao Ren
2019-10-10 22:04 ` Jae Hyun Yoo
2019-10-10 23:11 ` Tao Ren
2019-10-10 23:16 ` Jae Hyun Yoo
2019-10-10 23:24 ` Tao Ren
2019-10-10 23:29 ` Tao Ren
2019-10-10 23:52 ` Jae Hyun Yoo
2019-10-16 19:20 ` Brendan Higgins
2019-10-21 12:09 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).