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

[Christopher Bostic <cbostic@linux.vnet.ibm.com>]

On 2/21/17 7:00 PM, Joel Stanley wrote:
> On Wed, Feb 22, 2017 at 8:20 AM, Christopher Bostic
> <cbostic@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@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?

Hi Joel,

Yes, since the gpio master is requiring a core function,  similar to 
fsi_master_register()/unregister()

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