| Message ID | 20240924134009.116247-3-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] spi: s3c64xx: fix timeout counters in flush_fifo | expand |
On Tue, Sep 24, 2024 at 02:40:09PM GMT, Ben Dooks wrote: > The code that checks for loops in the s3c6xx_flush_fifo() checks > for loops being non-zero as a timeout, however the code /could/ > finish with loops being zero and the fifo being flushed... what is the possibility of this case? > Also, it would be useful to know what is left in the fifo for this > error case, so update the checks to see what is left, and then also > print the number of entries. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > drivers/spi/spi-s3c64xx.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 6ab416a33966..7b244e1fd58a 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) > val = readl(regs + S3C64XX_SPI_STATUS); > } while (TX_FIFO_LVL(val, sdd) && --loops); > > - if (loops == 0) > - dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n"); > + if (TX_FIFO_LVL(val, sdd)) > + dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd)); > > /* Flush RxFIFO*/ > loops = msecs_to_loops(1); > @@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) > break; > } while (--loops); > > - if (loops == 0) > - dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n"); > + if (RX_FIFO_LVL(val, sdd)) > + dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd)); This change doesn't super excite me, but it's fine. Please add a comment explaining the case when loops is '0' and the FIFO is flushed. With the comment given, you can have my r-b. Thanks, Andi > val = readl(regs + S3C64XX_SPI_CH_CFG); > val &= ~S3C64XX_SPI_CH_SW_RST; > -- > 2.37.2.352.g3c44437643 >
On 30/09/2024 23:51, Andi Shyti wrote: > On Tue, Sep 24, 2024 at 02:40:09PM GMT, Ben Dooks wrote: >> The code that checks for loops in the s3c6xx_flush_fifo() checks >> for loops being non-zero as a timeout, however the code /could/ >> finish with loops being zero and the fifo being flushed... > > what is the possibility of this case? Not sure, currently we're trying to debug a customer's setup where we're seeing some weird issues with SPI. This was found during a look into the code awaiting hardware access. The flush count was simply an inspection and it seemed like a good idea to fix the initial issue and then if there was an issue to print something more useful than a simple error message. >> Also, it would be useful to know what is left in the fifo for this >> error case, so update the checks to see what is left, and then also >> print the number of entries. >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> drivers/spi/spi-s3c64xx.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >> index 6ab416a33966..7b244e1fd58a 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) >> val = readl(regs + S3C64XX_SPI_STATUS); >> } while (TX_FIFO_LVL(val, sdd) && --loops); >> >> - if (loops == 0) >> - dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n"); >> + if (TX_FIFO_LVL(val, sdd)) >> + dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd)); >> >> /* Flush RxFIFO*/ >> loops = msecs_to_loops(1); >> @@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) >> break; >> } while (--loops); >> >> - if (loops == 0) >> - dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n"); >> + if (RX_FIFO_LVL(val, sdd)) >> + dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd)); > > This change doesn't super excite me, but it's fine. Please add a > comment explaining the case when loops is '0' and the FIFO is > flushed. > > With the comment given, you can have my r-b. Ok, will look at sending a second version later this week. > > Thanks, > Andi > >> val = readl(regs + S3C64XX_SPI_CH_CFG); >> val &= ~S3C64XX_SPI_CH_SW_RST; >> -- >> 2.37.2.352.g3c44437643 >> >
Hi Ben, On Tue, Oct 01, 2024 at 08:01:48AM GMT, Ben Dooks wrote: > On 30/09/2024 23:51, Andi Shyti wrote: > > On Tue, Sep 24, 2024 at 02:40:09PM GMT, Ben Dooks wrote: > > > The code that checks for loops in the s3c6xx_flush_fifo() checks > > > for loops being non-zero as a timeout, however the code /could/ > > > finish with loops being zero and the fifo being flushed... > > > > what is the possibility of this case? > > Not sure, currently we're trying to debug a customer's setup where > we're seeing some weird issues with SPI. This was found during a > look into the code awaiting hardware access. > > The flush count was simply an inspection and it seemed like a good > idea to fix the initial issue and then if there was an issue to > print something more useful than a simple error message. > > > > Also, it would be useful to know what is left in the fifo for this > > > error case, so update the checks to see what is left, and then also > > > print the number of entries. > > > > > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > > > --- > > > drivers/spi/spi-s3c64xx.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > > > index 6ab416a33966..7b244e1fd58a 100644 > > > --- a/drivers/spi/spi-s3c64xx.c > > > +++ b/drivers/spi/spi-s3c64xx.c > > > @@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) > > > val = readl(regs + S3C64XX_SPI_STATUS); > > > } while (TX_FIFO_LVL(val, sdd) && --loops); > > > - if (loops == 0) > > > - dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n"); > > > + if (TX_FIFO_LVL(val, sdd)) > > > + dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd)); > > > /* Flush RxFIFO*/ > > > loops = msecs_to_loops(1); > > > @@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) > > > break; > > > } while (--loops); > > > - if (loops == 0) > > > - dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n"); > > > + if (RX_FIFO_LVL(val, sdd)) > > > + dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd)); > > > > This change doesn't super excite me, but it's fine. Please add a > > comment explaining the case when loops is '0' and the FIFO is > > flushed. > > > > With the comment given, you can have my r-b. > > Ok, will look at sending a second version later this week. I actually think that these two patches can be squashed into a single one, I don't see much reason for having it double. Even better, I think the first patch is not needed at all with this new one. Right? Andi > > > > Thanks, > > Andi > > > > > val = readl(regs + S3C64XX_SPI_CH_CFG); > > > val &= ~S3C64XX_SPI_CH_SW_RST; > > > -- > > > 2.37.2.352.g3c44437643 > > > > > > > > -- > Ben Dooks http://www.codethink.co.uk/ > Senior Engineer Codethink - Providing Genius > > https://www.codethink.co.uk/privacy.html
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 6ab416a33966..7b244e1fd58a 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) val = readl(regs + S3C64XX_SPI_STATUS); } while (TX_FIFO_LVL(val, sdd) && --loops); - if (loops == 0) - dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n"); + if (TX_FIFO_LVL(val, sdd)) + dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd)); /* Flush RxFIFO*/ loops = msecs_to_loops(1); @@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) break; } while (--loops); - if (loops == 0) - dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n"); + if (RX_FIFO_LVL(val, sdd)) + dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd)); val = readl(regs + S3C64XX_SPI_CH_CFG); val &= ~S3C64XX_SPI_CH_SW_RST;
The code that checks for loops in the s3c6xx_flush_fifo() checks for loops being non-zero as a timeout, however the code /could/ finish with loops being zero and the fifo being flushed... Also, it would be useful to know what is left in the fifo for this error case, so update the checks to see what is left, and then also print the number of entries. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- drivers/spi/spi-s3c64xx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)