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.
>
> 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);
Good catch! Please make this a separate patch and send it upstream!
Can you enable debugging and post the output of the dev_dbg() in that
function.
>
> 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)
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
