i2c: aspeed: fix master pending state handling
diff mbox series

Message ID 20191009212034.20325-1-jae.hyun.yoo@linux.intel.com
State Accepted
Headers show
Series
  • i2c: aspeed: fix master pending state handling
Related show

Commit Message

Jae Hyun Yoo Oct. 9, 2019, 9:20 p.m. UTC
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(-)

Comments

Brendan Higgins Oct. 10, 2019, 12:32 a.m. UTC | #1
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!
Tao Ren Oct. 10, 2019, 1:26 a.m. UTC | #2
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!
>
Wolfram Sang Oct. 10, 2019, 6:37 a.m. UTC | #3
> 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.
Joel Stanley Oct. 10, 2019, 6:48 a.m. UTC | #4
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.
Jae Hyun Yoo Oct. 10, 2019, 6:22 p.m. UTC | #5
> 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
Tao Ren Oct. 10, 2019, 9:20 p.m. UTC | #6
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;
>  	}
>  
>
Jae Hyun Yoo Oct. 10, 2019, 10:04 p.m. UTC | #7
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
Tao Ren Oct. 10, 2019, 11:11 p.m. UTC | #8
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
Jae Hyun Yoo Oct. 10, 2019, 11:16 p.m. UTC | #9
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
Tao Ren Oct. 10, 2019, 11:24 p.m. UTC | #10
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
Tao Ren Oct. 10, 2019, 11:29 p.m. UTC | #11
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
Jae Hyun Yoo Oct. 10, 2019, 11:52 p.m. UTC | #12
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
Brendan Higgins Oct. 16, 2019, 7:20 p.m. UTC | #13
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?
Wolfram Sang Oct. 21, 2019, 12:09 p.m. UTC | #14
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!

Patch
diff mbox series

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;
 	}