Re: [PATCH RFT] spi: dw: Fix off-by-one for checking fifo depth

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Sat, 2014-12-20 at 10:17 +0800, Axel Lin wrote:
> 2014-12-19 19:46 GMT+08:00 Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>:
> > On Fri, 2014-12-19 at 16:01 +0800, Axel Lin wrote:
> >> The fifo depth could be from 2 to 256 from HW spec, so fix off-by-one for
> >> checking fifo depth.
> >> Without this patch, fifo is 258 after for loop iteration for the "no-match"
> >> case. So below line does not catch the "no-match" case.
> >>         dws->fifo_len = (fifo == 257) ? 0 : fifo;
> >
> > Seems reasonable, but never tried because in case of Medfield device we
> > have fifo size defined.
> >
> > I would try this in January.
> 
> hi Andy,
> 
> I check the code again and I think what current code does is:
> It tries to find the highest valid fifo depth so it checks the value it wrote
> to DW_SPI_TXFLTR. I think the valid value range is 2 ~ 256.
> So it will break out when writing 257 to DW_SPI_TXFLTR.

I think it will be considered as 1 by HW that kinda not what we want.

> 
> There is an off-by-one in dws->fifo_len setting because it assumes the
> latest register write fails so the latest valid value is fifo - 1.
> 
> So I think you can set dws->fifo_len to 0 to test current behavior first.
> 
> I guess below change should work, please test this instead my previous patch.

Something wrong with formatting below, but don't worry I applied it
manually and retested.

> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index d0d5542..1a0f266 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -621,13 +621,13 @@ static void spi_hw_init(struct dw_spi *dws)
>   if (!dws->fifo_len) {
>   u32 fifo;
> 
> - for (fifo = 2; fifo <= 257; fifo++) {
> + for (fifo = 2; fifo <= 256; fifo++) {
>   dw_writew(dws, DW_SPI_TXFLTR, fifo);
>   if (fifo != dw_readw(dws, DW_SPI_TXFLTR))
>   break;
>   }
> 
> - dws->fifo_len = (fifo == 257) ? 0 : fifo;
> + dws->fifo_len = (fifo == 2) ? 0 : fifo - 1;
>   dw_writew(dws, DW_SPI_TXFLTR, 0);
>   }
>  }

So, my diff looks like:

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 549f5c96..83d17e38 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -595,7 +595,7 @@ static void dw_spi_cleanup(struct spi_device *spi)
 }
 
 /* Restart the controller, disable all interrupts, clean rx fifo */
-static void spi_hw_init(struct dw_spi *dws)
+static void spi_hw_init(struct device *dev, struct dw_spi *dws)
 {
        dw_spi_disable_intr(dws);
 
@@ -606,14 +606,15 @@ static void spi_hw_init(struct dw_spi *dws)
        if (!dws->fifo_len) {
                u32 fifo;
 
-               for (fifo = 2; fifo <= 257; fifo++) {
+               for (fifo = 2; fifo <= 256; fifo++) {
                        dw_writew(dws, DW_SPI_TXFLTR, fifo);
                        if (fifo != dw_readw(dws, DW_SPI_TXFLTR))
                                break;
                }
-
-               dws->fifo_len = (fifo == 257) ? 0 : fifo;
                dw_writew(dws, DW_SPI_TXFLTR, 0);
+
+               dws->fifo_len = (fifo == 2) ? 0 : fifo - 1;
+               dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
        }
 }
 
@@ -653,7 +654,7 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
        master->dev.of_node = dev->of_node;
 
        /* Basic HW init */
-       spi_hw_init(dws);
+       spi_hw_init(dev, dws);
 
        if (dws->dma_ops && dws->dma_ops->dma_init) {
                ret = dws->dma_ops->dma_init(dws);
@@ -718,7 +719,7 @@ int dw_spi_resume_host(struct dw_spi *dws)
 {
        int ret;
 
-       spi_hw_init(dws);
+       spi_hw_init(&dws->master->dev, dws);
        ret = spi_master_resume(dws->master);
        if (ret)
                dev_err(&dws->master->dev, "fail to start queue (%d)\n", ret);

Feel free to amend it if needed. For the fix itself get my

Reviewed-and-tested-by: Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx>


-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html