[PATCH linux dev-4.7 v2 5/7] drivers/fsi: Add retry on bus error detect

[Joel Stanley]

On Wed, Feb 22, 2017 at 8:20 AM, Christopher Bostic
<cbostic at linux.vnet.ibm.com> wrote:
> After each gpio master read/write check for bus errors.  If
> errors detected then clean it up and retry the original operation
> again.  If second attempt succeeds then don't report original
> error to caller.
>
> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
> ---
>  drivers/fsi/fsi-core.c        |  4 ++++
>  drivers/fsi/fsi-master-gpio.c | 24 ++++++++++++++++++++++--
>  drivers/fsi/fsi-master.h      |  3 +++
>  3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 360c02a..428675f 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -639,6 +639,10 @@ static int fsi_master_break(struct fsi_master *master, int link)
>         return 0;
>  }
>
> +void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
> +{
> +}
> +
>  static int fsi_master_scan(struct fsi_master *master)
>  {
>         int link, slave_id, rc;
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index 8aec538..5b502eb 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -381,7 +381,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>                 return -ENODEV;
>
>         build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +       if (rc) {
> +               fsi_master_handle_error(&master->master, addr);
> +
> +               /* Try again */
> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +               if (rc)
> +                       fsi_master_handle_error(&master->master, addr);
> +       }
> +
> +       return rc;
>  }
>
>  static int fsi_master_gpio_write(struct fsi_master *_master, int link,
> @@ -395,7 +405,17 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>                 return -ENODEV;
>
>         build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +       if (rc) {
> +               fsi_master_handle_error(&master->master, addr);
> +
> +               /* Try again */

It's obvious from the code that we're trying again. You could change
the comment to indicate why we are trying again.

> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +               if (rc)
> +                       fsi_master_handle_error(&master->master, addr);
> +       }

You're repeating yourself here. Perhaps a loop?

You have the same sequence in fsi_master_gpio_read. It would be nice
to not repeat the sequence.

> +
> +       return rc;
>  }
>
>  /*
> diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
> index 4a3176b..4ed416c 100644
> --- a/drivers/fsi/fsi-master.h
> +++ b/drivers/fsi/fsi-master.h
> @@ -19,6 +19,8 @@
>
>  #include <linux/device.h>
>
> +#include "fsi-master.h"

This looks wrong.

> +
>  /* Control Registers */
>  #define FSI_MMODE              0x0             /* R/W: mode */
>  #define FSI_MDLYR              0x4             /* R/W: delay */
> @@ -85,6 +87,7 @@ struct fsi_master {
>  extern int fsi_master_register(struct fsi_master *master);
>  extern void fsi_master_unregister(struct fsi_master *master);
>  extern int fsi_master_start_ipoll(struct fsi_master *master);
> +extern void fsi_master_handle_error(struct fsi_master *master, uint32_t addr);

Does this need to be added to the header?

>
>  /**
>   * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x,
> --
> 1.8.2.2
>