On Sun, Aug 30, 2020 at 06:38:48PM +0200, Luca Ceresoli wrote: > If this routine sleeps because it was scheduled out, it might miss DONE > going asserted and consider it a timeout. This would potentially make the > code return an error even when programming succeeded. Rewrite the loop to > always check DONE after checking if timeout expired so this cannot happen > anymore. > > While there, also add error checking for gpiod_get_value(). Also avoid > checking the DONE GPIO in two places, which would make the error-checking > code duplicated and more annoying. > > The new loop it written to still guarantee that we apply 8 extra CCLK > cycles after DONE has gone asserted, which is required by the hardware. > > Reported-by: Tom Rix <[email protected]> > Reviewed-by: Tom Rix <[email protected]> > Signed-off-by: Luca Ceresoli <[email protected]> > > --- > > Changes in v4: > - add Reviewed-by Tom Rix > - fix uninitialized variable > (Reported-by: kernel test robot <[email protected]>) > > Changes in v3: > - completely rewrite the loop after Tom pointed out the 'sleep' bug > > This patch is new in v2 > --- > drivers/fpga/xilinx-spi.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c > index 01f494172379..fba8eb4866a7 100644 > --- a/drivers/fpga/xilinx-spi.c > +++ b/drivers/fpga/xilinx-spi.c > @@ -151,22 +151,29 @@ static int xilinx_spi_write_complete(struct > fpga_manager *mgr, > struct fpga_image_info *info) > { > struct xilinx_spi_conf *conf = mgr->priv; > - unsigned long timeout; > + unsigned long timeout = jiffies + > usecs_to_jiffies(info->config_complete_timeout_us); > + bool expired = false; > + int done; > int ret; > > - if (gpiod_get_value(conf->done)) > - return xilinx_spi_apply_cclk_cycles(conf); > + /* > + * This loop is carefully written such that if the driver is > + * scheduled out for more than 'timeout', we still check for DONE > + * before giving up and we apply 8 extra CCLK cycles in all cases. > + */ > + while (!expired) { > + expired = time_after(jiffies, timeout); > > - timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us); > - > - while (time_before(jiffies, timeout)) { > + done = get_done_gpio(mgr); > + if (done < 0) > + return done; > > ret = xilinx_spi_apply_cclk_cycles(conf); > if (ret) > return ret; > > - if (gpiod_get_value(conf->done)) > - return xilinx_spi_apply_cclk_cycles(conf); > + if (done) > + return 0; > } > > dev_err(&mgr->dev, "Timeout after config data transfer\n"); > -- > 2.28.0 >
Applied to for-next, Thanks
