[v3] spi: spi-imx: Fix spi_bus_clk if requested clock is higher than input clock

  From: Frieder Schrempf <frieder.schrempf@kontron.de>

In case the requested bus clock is higher than the input clock, the correct
dividers (pre = 0, post = 0) are returned from mx51_ecspi_clkdiv(), but
*fres is left uninitialized and therefore contains an arbitrary value.

This causes trouble for the recently introduced PIO polling feature as the
value in spi_imx->spi_bus_clk is used there to calculate for which
transfers to enable PIO polling.

Fix this by setting *fres even if no clock dividers are in use.

This issue was observed on Kontron BL i.MX8MM with an SPI peripheral clock set
to 50 MHz by default and a requested SPI bus clock of 80 MHz for the SPI NOR
flash.

With the fix applied the debug message from mx51_ecspi_clkdiv() now prints the
following:

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

Fixes: 6fd8b8503a0d ("spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds")
Fixes: 07e759387788 ("spi: spi-imx: add PIO polling support")
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: David Jander <david@protonic.nl>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: stable@vger.kernel.org
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Fabio Estevam <festevam@gmail.com>
---

Changes for v3:

* Add back the Fixes tag for commit 6fd8b8503a0d
* Add Fabio's Tested-by (Thanks!)

Changes for v2:

* Remove the reference and the Fixes tag for commit 6fd8b8503a0d as it is
  incorrect.
---
 drivers/spi/spi-imx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
  

On 11/15/22 19:10, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> In case the requested bus clock is higher than the input clock, the correct
> dividers (pre = 0, post = 0) are returned from mx51_ecspi_clkdiv(), but
> *fres is left uninitialized and therefore contains an arbitrary value.
> 
> This causes trouble for the recently introduced PIO polling feature as the
> value in spi_imx->spi_bus_clk is used there to calculate for which
> transfers to enable PIO polling.
> 
> Fix this by setting *fres even if no clock dividers are in use.
> 
> This issue was observed on Kontron BL i.MX8MM with an SPI peripheral clock set
> to 50 MHz by default and a requested SPI bus clock of 80 MHz for the SPI NOR
> flash.
> 
> With the fix applied the debug message from mx51_ecspi_clkdiv() now prints the
> following:
> 
> spi_imx 30820000.spi: mx51_ecspi_clkdiv: fin: 50000000, fspi: 50000000,
> post: 0, pre: 0
> 
> Fixes: 6fd8b8503a0d ("spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds")
> Fixes: 07e759387788 ("spi: spi-imx: add PIO polling support")
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: David Jander <david@protonic.nl>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Marek Vasut <marex@denx.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Fabio Estevam <festevam@gmail.com>
> ---
> 
> Changes for v3:
> 
> * Add back the Fixes tag for commit 6fd8b8503a0d
> * Add Fabio's Tested-by (Thanks!)
> 
> Changes for v2:
> 
> * Remove the reference and the Fixes tag for commit 6fd8b8503a0d as it is
>    incorrect.
> ---
>   drivers/spi/spi-imx.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 30d82cc7300b..468ce0a2b282 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -444,8 +444,7 @@ static unsigned int mx51_ecspi_clkdiv(struct spi_imx_data *spi_imx,
>   	unsigned int pre, post;
>   	unsigned int fin = spi_imx->spi_clk;
>   
> -	if (unlikely(fspi > fin))
> -		return 0;
> +	fspi = min(fspi, fin);
>   
>   	post = fls(fin) - fls(fspi);
>   	if (fin > fspi << post)

Can you also test the SPI flash at some 100 kHz, just to see whether it 
still works properly ? (to retain behavior fixed first in 6fd8b8503a0dc 
("spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds") )

The fix here does look fine by me however.
  

On 16.11.22 00:49, Marek Vasut wrote:
> On 11/15/22 19:10, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> In case the requested bus clock is higher than the input clock, the
>> correct
>> dividers (pre = 0, post = 0) are returned from mx51_ecspi_clkdiv(), but
>> *fres is left uninitialized and therefore contains an arbitrary value.
>>
>> This causes trouble for the recently introduced PIO polling feature as
>> the
>> value in spi_imx->spi_bus_clk is used there to calculate for which
>> transfers to enable PIO polling.
>>
>> Fix this by setting *fres even if no clock dividers are in use.
>>
>> This issue was observed on Kontron BL i.MX8MM with an SPI peripheral
>> clock set
>> to 50 MHz by default and a requested SPI bus clock of 80 MHz for the
>> SPI NOR
>> flash.
>>
>> With the fix applied the debug message from mx51_ecspi_clkdiv() now
>> prints the
>> following:
>>
>> spi_imx 30820000.spi: mx51_ecspi_clkdiv: fin: 50000000, fspi: 50000000,
>> post: 0, pre: 0
>>
>> Fixes: 6fd8b8503a0d ("spi: spi-imx: Fix out-of-order CS/SCLK operation
>> at low speeds")
>> Fixes: 07e759387788 ("spi: spi-imx: add PIO polling support")
>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>> Cc: David Jander <david@protonic.nl>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> Tested-by: Fabio Estevam <festevam@gmail.com>
>> ---
>>
>> Changes for v3:
>>
>> * Add back the Fixes tag for commit 6fd8b8503a0d
>> * Add Fabio's Tested-by (Thanks!)
>>
>> Changes for v2:
>>
>> * Remove the reference and the Fixes tag for commit 6fd8b8503a0d as it is
>>    incorrect.
>> ---
>>   drivers/spi/spi-imx.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
>> index 30d82cc7300b..468ce0a2b282 100644
>> --- a/drivers/spi/spi-imx.c
>> +++ b/drivers/spi/spi-imx.c
>> @@ -444,8 +444,7 @@ static unsigned int mx51_ecspi_clkdiv(struct
>> spi_imx_data *spi_imx,
>>       unsigned int pre, post;
>>       unsigned int fin = spi_imx->spi_clk;
>>   -    if (unlikely(fspi > fin))
>> -        return 0;
>> +    fspi = min(fspi, fin);
>>         post = fls(fin) - fls(fspi);
>>       if (fin > fspi << post)
> 
> Can you also test the SPI flash at some 100 kHz, just to see whether it
> still works properly ? (to retain behavior fixed first in 6fd8b8503a0dc
> ("spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds") )
> 
> The fix here does look fine by me however.

I successfully tested at 100 kHZ SPI bus clock. As in this case fspi is
lower than fin, the patch doesn't change anything in the code path and
therefore the behavior introduced in 6fd8b8503a0dc stays the same as
without the patch.
  

On 11/16/22 09:17, Frieder Schrempf wrote:
> On 16.11.22 00:49, Marek Vasut wrote:
>> On 11/15/22 19:10, Frieder Schrempf wrote:
>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>
>>> In case the requested bus clock is higher than the input clock, the
>>> correct
>>> dividers (pre = 0, post = 0) are returned from mx51_ecspi_clkdiv(), but
>>> *fres is left uninitialized and therefore contains an arbitrary value.
>>>
>>> This causes trouble for the recently introduced PIO polling feature as
>>> the
>>> value in spi_imx->spi_bus_clk is used there to calculate for which
>>> transfers to enable PIO polling.
>>>
>>> Fix this by setting *fres even if no clock dividers are in use.
>>>
>>> This issue was observed on Kontron BL i.MX8MM with an SPI peripheral
>>> clock set
>>> to 50 MHz by default and a requested SPI bus clock of 80 MHz for the
>>> SPI NOR
>>> flash.
>>>
>>> With the fix applied the debug message from mx51_ecspi_clkdiv() now
>>> prints the
>>> following:
>>>
>>> spi_imx 30820000.spi: mx51_ecspi_clkdiv: fin: 50000000, fspi: 50000000,
>>> post: 0, pre: 0
>>>
>>> Fixes: 6fd8b8503a0d ("spi: spi-imx: Fix out-of-order CS/SCLK operation
>>> at low speeds")
>>> Fixes: 07e759387788 ("spi: spi-imx: add PIO polling support")
>>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Cc: David Jander <david@protonic.nl>
>>> Cc: Fabio Estevam <festevam@gmail.com>
>>> Cc: Mark Brown <broonie@kernel.org>
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>> Tested-by: Fabio Estevam <festevam@gmail.com>
>>> ---
>>>
>>> Changes for v3:
>>>
>>> * Add back the Fixes tag for commit 6fd8b8503a0d
>>> * Add Fabio's Tested-by (Thanks!)
>>>
>>> Changes for v2:
>>>
>>> * Remove the reference and the Fixes tag for commit 6fd8b8503a0d as it is
>>>     incorrect.
>>> ---
>>>    drivers/spi/spi-imx.c | 3 +--
>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
>>> index 30d82cc7300b..468ce0a2b282 100644
>>> --- a/drivers/spi/spi-imx.c
>>> +++ b/drivers/spi/spi-imx.c
>>> @@ -444,8 +444,7 @@ static unsigned int mx51_ecspi_clkdiv(struct
>>> spi_imx_data *spi_imx,
>>>        unsigned int pre, post;
>>>        unsigned int fin = spi_imx->spi_clk;
>>>    -    if (unlikely(fspi > fin))
>>> -        return 0;
>>> +    fspi = min(fspi, fin);
>>>          post = fls(fin) - fls(fspi);
>>>        if (fin > fspi << post)
>>
>> Can you also test the SPI flash at some 100 kHz, just to see whether it
>> still works properly ? (to retain behavior fixed first in 6fd8b8503a0dc
>> ("spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds") )
>>
>> The fix here does look fine by me however.
> 
> I successfully tested at 100 kHZ SPI bus clock. As in this case fspi is
> lower than fin, the patch doesn't change anything in the code path and
> therefore the behavior introduced in 6fd8b8503a0dc stays the same as
> without the patch.

Acked-by: Marek Vasut <marex@denx.de>

Thanks for the extra check !
  

On Tue, 15 Nov 2022 19:10:00 +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> In case the requested bus clock is higher than the input clock, the correct
> dividers (pre = 0, post = 0) are returned from mx51_ecspi_clkdiv(), but
> *fres is left uninitialized and therefore contains an arbitrary value.
> 
> This causes trouble for the recently introduced PIO polling feature as the
> value in spi_imx->spi_bus_clk is used there to calculate for which
> transfers to enable PIO polling.
> 
> [...]

Applied to

   broonie/spi.git for-next

Thanks!

[1/1] spi: spi-imx: Fix spi_bus_clk if requested clock is higher than input clock
      commit: db2d2dc9a0b58c6faefb6b002fdbed4f0362d1a4

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark