Re: [PATCH] spi: spi-imx: Revert "spi: spi-imx: add PIO polling support"

[Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>]

Hi Marc,

On 15.11.22 13:55, Marc Kleine-Budde wrote:
> On 15.11.2022 11:51:53, Frieder Schrempf wrote:
>> On 14.11.22 12:29, Mark Brown wrote:
>>> On Mon, Nov 14, 2022 at 09:30:26AM +0100, Frieder Schrempf wrote:
>>>
>>>> As far as I know Fabio also discovered that disabling SDMA also fixes
>>>> the problem.
>>>
>>>> I guess I will try to repeat some tests on latest master and see if
>>>> there is anything that makes things work again without reducing the
>>>> clock. If anyone has some more ideas of how to fix this properly, please
>>>> let me know. If nothing else helps we could also reduce the SPI clock.
>>>
>>> It sounds like the commit can stay and that everyone is happy
>>> that the issue is that the the commit made things run faster and
>>> exposed some other misconfiguration for these systems?
>>
>> Honestly I'm not really sure how to proceed.
>>
>> My first impression was to keep the PIO polling support with its
>> benefits if there's just this single issue with the SPI NOR on our board
>> and assuming that the performance improvements uncovered a bug somewhere
>> else. But at the moment I'm not quite sure this is really the case.
>>
>> I did another test on v6.1-rc5 and disabling either PIO polling
>> (spi-imx.polling_limit_us=0) or DMA (spi-imx.use_dma=0), or both of them
>> makes reading the SPI NOR work again.
> 
> That was a good hint, I think I've found something.
> 
> Can you check if this fixes your problem? Just a quick hack to, a proper
> solution needs some more love.
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 30d82cc7300b..76021b9bb445 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1270,9 +1270,22 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>                 spi_imx->dynamic_burst = 0;
>         }
>  
> -       if (spi_imx_can_dma(spi_imx->controller, spi, t))
> -               spi_imx->usedma = true;
> -       else
> +       if (spi_imx_can_dma(spi_imx->controller, spi, t)) {
> +               unsigned long hz_per_byte, byte_limit;
> +
> +               /*
> +                * Calculate the estimated time in us the transfer runs. Find
> +                * the number of Hz per byte per polling limit.
> +                */
> +               hz_per_byte = polling_limit_us ? ((8 + 4) * USEC_PER_SEC) / polling_limit_us : 0;
> +               byte_limit = hz_per_byte ? t->effective_speed_hz / hz_per_byte : 1;
> +
> +               /* run in polling mode for short transfers */
> +               if (t->len < byte_limit)
> +                       spi_imx->usedma = false;
> +               else
> +                       spi_imx->usedma = true;
> +       } else
>                 spi_imx->usedma = false;
>  
>         spi_imx->rx_only = ((t->tx_buf == NULL)
> @@ -1597,8 +1610,8 @@ static int spi_imx_transfer_one(struct spi_controller *controller,
>         struct spi_imx_data *spi_imx = spi_controller_get_devdata(spi->controller);
>         unsigned long hz_per_byte, byte_limit;
>  
> -       spi_imx_setupxfer(spi, transfer);
>         transfer->effective_speed_hz = spi_imx->spi_bus_clk;
> +       spi_imx_setupxfer(spi, transfer);
>  
>         /* flush rxfifo before transfer */
>         while (spi_imx->devtype_data->rx_available(spi_imx))
> 

Thanks for the patch, but unfortunately this doesn't help. I did some
more debugging and it looks like there are two problems.

In my case on i.MX8MM the SPI is fed by a 50 MHz peripheral clock.
Requesting 80 MHz for the SPI NOR triggers the fspi > fin condition in
mx51_ecspi_clkdiv() [1] which in turn leaves *fres uninitialized causing
spi_imx->spi_bus_clk to be set to an arbitrary/random value in
mx51_ecspi_prepare_transfer() [2].

This in turn messes up the calculation for the PIO polling byte limit.
In my case the limit was usually somewhere around 8000 bytes, so the
4096 byte SPI NOR messages get transferred via PIO polling.

Having large and inefficient polling transfers shouldn't be a problem
and lead to corrupted data, but I suspect that it doesn't work because
the transfer size exceeds the FIFO size in this case.

If my conclusions are correct there are two fixes required (though for
my use case each one of the alone is enough to make things work):

1. Make sure spi_bus_clk is correct even if the requested bus clock
exceeds the input clock.

2. Limit byte_limit for PIO polling calculation to a maximum of
fifo_size, so we don't try to poll for transfers that don't fit into the
FIFO.

Both fixes are quite simple (see diff below) and if you agree I will
send them as formal patches.

Thanks
Frieder

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi-imx.c#L447
[2]
https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi-imx.c#L650

--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -445,7 +445,7 @@ static unsigned int mx51_ecspi_clkdiv(struct
spi_imx_data *spi_imx,
        unsigned int fin = spi_imx->spi_clk;

        if (unlikely(fspi > fin))
-               return 0;
+               fspi = fin;

        post = fls(fin) - fls(fspi);
        if (fin > fspi << post)
@@ -1613,6 +1613,7 @@ static int spi_imx_transfer_one(struct
spi_controller *controller,
         */
        hz_per_byte = polling_limit_us ? ((8 + 4) * USEC_PER_SEC) /
polling_limit_us : 0;
        byte_limit = hz_per_byte ? transfer->effective_speed_hz /
hz_per_byte : 1;
+       byte_limit = min(byte_limit, spi_imx->devtype_data->fifo_size);

        /* run in polling mode for short transfers */
        if (transfer->len < byte_limit)