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

[Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>]

On 15.11.22 16:46, Marc Kleine-Budde wrote:
> On 15.11.2022 15:46:28, Frieder Schrempf wrote:
>> 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.
> 
> no - exceeding the FIFO size it not a problem. If you limit polling to
> FIFO size you effectively disable it for your use 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.
> 
> With a proper clock rate, and the default 30µs limit results in no
> polling for you.
> 
>> 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.
> 
> One of the performance benefits of polling is that you don't get all the
> IRQs needed for refilling the FIFO, please keep this as is.
> 
>> Both fixes are quite simple (see diff below) and if you agree I will
>> send them as formal patches.
> 
> Please just the first one. But let's fix polling support, too. I'll send
> a patch.

In the meantime I already found out that the second point and my fix is
wrong. Thanks for clarifying.

> 
>>
>> 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;
> 
> This should equal:
> fspi = min(fspi, fin);

Of course!

> 
> Good catch! Please make this a separate patch and send it upstream!

Will do.

> Can you enable debugging and post the output of the dev_dbg() in that
> function.

Without the fix there's no output as the dev_dbg() is skipped, with the
fix there is this in my case:

spi_imx 30820000.spi: mx51_ecspi_clkdiv: fin: 50000000, fspi: 50000000,
post: 0, pre: 0