Re: Fw: [PATCH linux dev-4.10] drivers: i2c: fsi: Add proper abort method

Re: Fw: [PATCH linux dev-4.10] drivers: i2c: fsi: Add proper abort method

[Christopher Bostic]

Reviewed-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>
>
> On 10/20/2017 12:15 AM, Andrew Jeffery wrote:
> > On Thu, 2017-10-12 at 15:42 -0500, Eddie James wrote:
> >> From: "Edward A. James" <eajames@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@us.ibm.com>
> >> ---
> >>   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..6c582d8 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;
> >> + }
> >> +
> >> + 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)
> >> +{
> >> + 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_INTERRUPTIBLE);
> >> + schedule_timeout(I2C_LOCAL_WAIT_TIMEOUT);
> > Shouldn't we use the uninterruptible state to ensure we get at least
> > I2C_LOCAL_WAIT_TIMEOUT in the face of signals?
>
> Good point, I'll switch that.
>
> Thanks!
> Eddie
>
> >
> > Andrew
>
>
>