[PATCH linux dev-4.10 v2] drivers: i2c: fsi: Add proper abort method
Andrew Jeffery
andrew at aj.id.au
Mon Oct 23 11:31:15 AEDT 2017
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.
> + }
> +
> + 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.
> + 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.
> +
> + 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.
>
> + 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.
> +
> + 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20171023/a236ec26/attachment-0001.sig>
More information about the openbmc
mailing list