[PATCH linux dev-4.10 v2] drivers: i2c: fsi: Add proper abort method
Eddie James
eajames at linux.vnet.ibm.com
Wed Oct 25 02:39:16 AEDT 2017
On 10/22/2017 07:31 PM, Andrew Jeffery wrote:
> Hi Eddie,
>
> Apologies for not being more thorough initially, I'm back again with comments.
>
> On Fri, 2017-10-20 at 15:02 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames at us.ibm.com>
>>
>> Driver wasn't cleaning up on timeout or in an error situation properly.
>> Need to do a full reset if we fail in order to re-stablish a good state
>> of the engine.
>>
>> Signed-off-by: Edward A. James <eajames at us.ibm.com>
>> ---
>>
>> Changes since v1:
>> * switch to TASK_UNINTERRUPTIBLE for waiting for command complete
>>
>> drivers/i2c/busses/i2c-fsi.c | 259 +++++++++++++++++++++++++++++++------------
>> 1 file changed, 191 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
>> index 1af9c01..fa44017 100644
>> --- a/drivers/i2c/busses/i2c-fsi.c
>> +++ b/drivers/i2c/busses/i2c-fsi.c
>> @@ -133,6 +133,11 @@
>> #define I2C_ESTAT_SELF_BUSY 0x00000040
>> #define I2C_ESTAT_VERSION 0x0000001f
>>
>> +#define I2C_PORT_BUSY_RESET 0x80000000
>> +
>> +#define I2C_LOCAL_WAIT_TIMEOUT 2 /* jiffies */
>> +#define I2C_ABORT_TIMEOUT msecs_to_jiffies(100)
>> +
>> struct fsi_i2c_master {
>> struct fsi_device *fsi;
>> u8 fifo_size;
>> @@ -351,22 +356,185 @@ static int fsi_i2c_read_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
>> return rc;
>> }
>>
>> +static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
>> +{
>> + int i, rc;
>> + u32 mode, dummy = 0;
>> +
>> + rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
>> + if (rc)
>> + return rc;
>> +
>> + mode |= I2C_MODE_DIAG;
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
>> + if (rc)
>> + return rc;
>> +
>> + for (i = 0; i < 9; ++i) {
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
>> + if (rc)
>> + return rc;
> I take it we're okay with cycling the bus as fast as we can, and not try to
> maintain any particular rate? Not suggesting you need to change anything, just
> asking the question.
Yep, the spec doesn't give any indication of needing to rate-limit this
toggling. This is always how it's been done for this master, and it
seems to work.
>
>> + }
>> +
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SDA, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SDA, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + mode &= ~I2C_MODE_DIAG;
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
>> +
>> + return rc;
>> +}
>> +
>> +static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
>> +{
> I can't see that this (the whole reset sequence) is called with pre-emption
> disabled, perhaps we should do so? It looks like this will be called in process
> context through fsi_i2c_xfer(). Stalling the recovery will just hold up other
> processes wanting to use the bus as we'll hold the bus mutex across the task
> switch.
Well, presumably we want to block other access to the bus? We need to
get the master back in a good state before releasing it for other transfers.
>
>> + int rc;
>> + u32 mode, stat, dummy = 0;
>> +
>> + /* reset engine */
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + /* re-init engine */
>> + rc = fsi_i2c_dev_init(i2c);
>> + if (rc)
>> + return rc;
>> +
>> + rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
>> + if (rc)
>> + return rc;
>> +
>> + /* set port; default after reset is 0 */
>> + if (port) {
>> + mode = SETFIELD(I2C_MODE_PORT, mode, port);
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
>> + if (rc)
>> + return rc;
>> + }
>> +
>> + /* reset busy register; hw workaround */
>> + dummy = I2C_PORT_BUSY_RESET;
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_PORT_BUSY, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + /* force bus reset */
>> + rc = fsi_i2c_reset_bus(i2c);
>> + if (rc)
>> + return rc;
>> +
>> + /* reset errors */
>> + dummy = 0;
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + /* wait for command complete */
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(I2C_LOCAL_WAIT_TIMEOUT);
>> +
>> + rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
>> + if (rc)
>> + return rc;
>> +
>> + if (stat & I2C_STAT_CMD_COMP)
>> + return 0;
>> +
>> + /* failed to get command complete; reset engine again */
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
>> + if (rc)
>> + return rc;
>> +
>> + /* re-init engine again */
>> + rc = fsi_i2c_dev_init(i2c);
>> + if (rc)
>> + return rc;
>> +
>> + return 0;
>> +}
>> +
>> +static int fsi_i2c_abort(struct fsi_i2c_port *port, u32 status)
>> +{
>> + int rc;
>> + unsigned long start;
>> + u32 cmd = I2C_CMD_WITH_STOP;
>> + struct fsi_device *fsi = port->master->fsi;
>> +
>> + rc = fsi_i2c_reset(port->master, port->port);
>> + if (rc)
>> + return rc;
>> +
>> + /* skip final stop command for these errors */
>> + if (status & (I2C_STAT_PARITY | I2C_STAT_LOST_ARB | I2C_STAT_STOP_ERR))
>> + return 0;
>> +
>> + rc = fsi_i2c_write_reg(fsi, I2C_FSI_CMD, &cmd);
>> + if (rc)
>> + return rc;
>> +
>> + start = jiffies;
>> +
>> + do {
>> + rc = fsi_i2c_read_reg(fsi, I2C_FSI_STAT, &status);
>> + if (rc)
>> + return rc;
>> +
>> + if (status & I2C_STAT_CMD_COMP)
>> + return 0;
>> +
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + if (schedule_timeout(I2C_LOCAL_WAIT_TIMEOUT) > 0)
>> + return -EINTR;
>> +
>> + } while (time_after(start + I2C_ABORT_TIMEOUT, jiffies));
> I'm a bit ignorant here, but why do we expect the abort operation to take up to
> 100ms? You mentioned you invented the numbers, but it's not clear what the
> justification is.
I used the same timeout used in the FSP driver, which seems to work. I
really have no other justification for this value.
>
>> +
>> + return -ETIME;
>> +}
>> +
>> static int fsi_i2c_handle_status(struct fsi_i2c_port *port,
>> struct i2c_msg *msg, u32 status)
>> {
>> int rc;
>> u8 fifo_count;
>> - struct fsi_i2c_master *i2c = port->master;
>> - u32 dummy = 0;
>>
>> if (status & I2C_STAT_ERR) {
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
>> + rc = fsi_i2c_abort(port, status);
>> if (rc)
>> return rc;
>>
>> + if (status & I2C_STAT_INV_CMD)
>> + return -EINVAL;
>> +
>> + if (status & (I2C_STAT_PARITY | I2C_STAT_BE_OVERRUN |
>> + I2C_STAT_BE_ACCESS))
>> + return -EPROTO;
>> +
>> if (status & I2C_STAT_NACK)
>> return -EFAULT;
> EFAULT is "Bad address", but slaves can NACK things that aren't addresses.
> As an example i2c-aspeed calls a NACK an EIO.
True, but I dislike returning EIO as it's somewhat of a default option
and makes it very difficult to determine what actually went wrong. How
about ENXIO?
>
>>
>> + if (status & I2C_STAT_LOST_ARB)
>> + return -ECANCELED;
> So checking around the tree, all of i2c-aspeed, i2c-designware-core,
> i2c-hix5hd2, i2c-kempld to name a few (I stopped checking at that point) use
> -EAGAIN for arbitration loss. I don't think -ECANCELLED is appropriate, and
> certainly think that -EAGAIN is what we want: Nothing went wrong aside from
> this master lost the arbitration race, so the caller should really just retry.
Good point, however, for this master, it can often return arbitration
lost if the clock or data line is stuck, and retries will not work. I'll
switch it to EAGAIN, callers shouldn't try too many times hopefully...
Thanks!
Eddie
>
>> +
>> + if (status & I2C_STAT_STOP_ERR)
>> + return -EBADMSG;
>> +
>> return -EIO;
>> }
>>
>> @@ -396,9 +564,9 @@ static int fsi_i2c_handle_status(struct fsi_i2c_port *port,
>> static int fsi_i2c_wait(struct fsi_i2c_port *port, struct i2c_msg *msg,
>> unsigned long timeout)
>> {
>> - const unsigned long local_timeout = 2; /* jiffies */
>> u32 status = 0;
>> - int rc;
>> + int rc, rc_abort;
>> + unsigned long start = jiffies;
>>
>> do {
>> rc = fsi_i2c_read_reg(port->master->fsi, I2C_FSI_STAT,
>> @@ -419,13 +587,21 @@ static int fsi_i2c_wait(struct fsi_i2c_port *port, struct i2c_msg *msg,
>> continue;
>> }
>>
>> - set_current_state(TASK_UNINTERRUPTIBLE);
>> - schedule_timeout(local_timeout);
>> - timeout = (timeout < local_timeout) ? 0 :
>> - timeout - local_timeout;
>> - } while (timeout);
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + if (schedule_timeout(I2C_LOCAL_WAIT_TIMEOUT) > 0) {
>> + rc = -EINTR;
>> + goto abort;
>> + }
>> + } while (time_after(start + timeout, jiffies));
>>
>> - return -ETIME;
>> + rc = -ETIME;
>> +
>> +abort:
>> + rc_abort = fsi_i2c_abort(port, status);
>> + if (rc_abort)
>> + return rc_abort;
>> +
>> + return rc;
>> }
>>
>> static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>> @@ -469,72 +645,19 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
>> | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
>> }
>>
>> -static int fsi_i2c_low_level_recover_bus(struct fsi_i2c_master *i2c)
>> -{
>> - int i, rc;
>> - u32 mode, dummy = 0;
>> -
>> - rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
>> - if (rc)
>> - return rc;
>> -
>> - mode |= I2C_MODE_DIAG;
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
>> - if (rc)
>> - return rc;
>> -
>> - for (i = 0; i < 9; ++i) {
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
>> - if (rc)
>> - return rc;
>> -
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
>> - if (rc)
>> - return rc;
>> - }
>> -
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
>> - if (rc)
>> - return rc;
>> -
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SDA, &dummy);
>> - if (rc)
>> - return rc;
>> -
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
>> - if (rc)
>> - return rc;
>> -
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SDA, &dummy);
>> - if (rc)
>> - return rc;
>> -
>> - mode &= ~I2C_MODE_DIAG;
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
>> -
>> - return rc;
>> -}
>> -
>> static int fsi_i2c_recover_bus(struct i2c_adapter *adap)
>> {
>> int rc;
>> - u32 dummy = 0;
>> struct fsi_i2c_port *port = adap->algo_data;
>> - struct fsi_i2c_master *i2c = port->master;
>> + struct fsi_i2c_master *master = port->master;
>>
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
>> + rc = fsi_i2c_lock_master(master, adap->timeout);
>> if (rc)
>> return rc;
>>
>> - rc = fsi_i2c_dev_init(i2c);
>> - if (rc)
>> - return rc;
>> + rc = fsi_i2c_reset(master, port->port);
>>
>> - rc = fsi_i2c_low_level_recover_bus(i2c);
>> - if (rc)
>> - return rc;
>> -
>> - rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
>> + fsi_i2c_unlock_master(master);
>>
>> return rc;
>> }
> Cheers,
>
> Andrew
More information about the openbmc
mailing list