[PATCH 2/2] spi: s3c64xx: update flush_fifo timeout code

[Andi Shyti]

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 at 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