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

[Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>]

On 15.11.22 17:52, Marc Kleine-Budde wrote:
> On 15.11.2022 15:46:28, Frieder Schrempf wrote:
>>> 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.
> 
> Can you try this one?
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 30d82cc7300b..d45da1d0ac1d 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1607,6 +1607,13 @@ static int spi_imx_transfer_one(struct spi_controller *controller,
>         if (spi_imx->slave_mode)
>                 return spi_imx_pio_transfer_slave(spi, transfer);
>  
> +       /*
> +        * If we decided in spi_imx_can_dma() that we want to do a DMA
> +        * transfer, the message has already been mapped, so we have
> +        * to do the DMA transfer now.
> +        */
> +       if (spi_imx->usedma)
> +               return spi_imx_dma_transfer(spi_imx, transfer);
>         /*
>          * Calculate the estimated time in us the transfer runs. Find
>          * the number of Hz per byte per polling limit.
> @@ -1618,9 +1625,6 @@ static int spi_imx_transfer_one(struct spi_controller *controller,
>         if (transfer->len < byte_limit)
>                 return spi_imx_poll_transfer(spi, transfer);
>  
> -       if (spi_imx->usedma)
> -               return spi_imx_dma_transfer(spi_imx, transfer);
> -
>         return spi_imx_pio_transfer(spi, transfer);
>  }
> 
> The problem is: we decide on DMA in spi_imx_can_dma() the SPI frameworks
> maps the message, and then calls spi_imx_transfer_one(). We cannot
> operate with the CPU in the memory mapped to the DMA engine.
> 
> This should fix the problem without any additional patches.

This does fix the issue. My previous patch had the same result, as
spi_imx_can_dma() returns false if transfer size is below fifo_size. But
of course this is the correct fix with the correct explanation.