| Message ID | 20170221215032.79282-6-cbostic@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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? > > /** > * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x, > -- > 1.8.2.2 >
Hi Chris, > --- 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; Lets avoid the repeated code: const static int master_retries = 2; [...] for (retries = 0; retries < master_retries; retries++) { rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val); if (!rc) break; fsi_master_handle_error(&master->master, addr); } return rc; [or even better, we may want to put this in a helper, for use by both call sites] Also, is there any condition where fsi_master_handle_error() knows that the FSI bus is totally hosed, and there's no point retrying? Cheers, Jeremy
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. Hi Joel will update. >> + 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. OK, will add a loop here. >> + >> + 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? Probably not, will remove. Thanks, Chris >> /** >> * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x, >> -- >> 1.8.2.2 >>
On 2/21/17 7:13 PM, Jeremy Kerr wrote: > Hi Chris, > >> --- 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; > Lets avoid the repeated code: > > const static int master_retries = 2; > > [...] > > for (retries = 0; retries < master_retries; retries++) { > rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val); > if (!rc) > break; > fsi_master_handle_error(&master->master, addr); > } > > return rc; > > [or even better, we may want to put this in a helper, for use by both > call sites] Hi Jeremy, Will change to a loop. > Also, is there any condition where fsi_master_handle_error() knows that > the FSI bus is totally hosed, and there's no point retrying? There's not anything defined to flag a bad bus -will look into it. Thanks, Chris > Cheers, > > > Jeremy >
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 >>
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 */ + rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL); + if (rc) + fsi_master_handle_error(&master->master, addr); + } + + 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" + /* 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); /** * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x,
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(-)