From: Frieder Schrempf <[email protected]>
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: 07e759387788 ("spi: spi-imx: add PIO polling support")
Cc: Marc Kleine-Budde <[email protected]>
Cc: David Jander <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: [email protected]
Signed-off-by: Frieder Schrempf <[email protected]>
---
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)
--
2.38.1
On 15.11.2022 17:26:53, Frieder Schrempf wrote:
> From: Frieder Schrempf <[email protected]>
>
> 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: 07e759387788 ("spi: spi-imx: add PIO polling support")
The *fres parameter was introduced in:
| Fixes: 6fd8b8503a0d ("spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds")
The exiting code:
| if (unlikely(fspi > fin))
| return 0;
was not sufficient any more and should be fixed.
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 |
On 15.11.22 17:54, Marc Kleine-Budde wrote:
> On 15.11.2022 17:26:53, Frieder Schrempf wrote:
>> From: Frieder Schrempf <[email protected]>
>>
>> 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: 07e759387788 ("spi: spi-imx: add PIO polling support")
You want me to remove this tag?
> The *fres parameter was introduced in:
>
> | Fixes: 6fd8b8503a0d ("spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds")
and instead add back this tag? I wasn't really sure about that.
>
> The exiting code:
>
> | if (unlikely(fspi > fin))
> | return 0;
>
> was not sufficient any more and should be fixed.
You want me to add this in the description? Or is this just the
explanation for why 6fd8b8503a0d should be in the Fixes tag?
On Tue, Nov 15, 2022 at 1:27 PM Frieder Schrempf <[email protected]> wrote:
>
> From: Frieder Schrempf <[email protected]>
>
> 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: 07e759387788 ("spi: spi-imx: add PIO polling support")
> Cc: Marc Kleine-Budde <[email protected]>
> Cc: David Jander <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Marek Vasut <[email protected]>
> Cc: [email protected]
> Signed-off-by: Frieder Schrempf <[email protected]>
Thanks for the fix:
Tested-by: Fabio Estevam <[email protected]>
On 15.11.2022 18:14:38, Frieder Schrempf wrote:
> On 15.11.22 17:54, Marc Kleine-Budde wrote:
> > On 15.11.2022 17:26:53, Frieder Schrempf wrote:
> >> From: Frieder Schrempf <[email protected]>
> >>
> >> 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: 07e759387788 ("spi: spi-imx: add PIO polling support")
>
> You want me to remove this tag?
>
> > The *fres parameter was introduced in:
> >
> > | Fixes: 6fd8b8503a0d ("spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds")
>
> and instead add back this tag? I wasn't really sure about that.
Keep both.
> >
> > The exiting code:
> >
> > | if (unlikely(fspi > fin))
> > | return 0;
> >
> > was not sufficient any more and should be fixed.
>
> You want me to add this in the description? Or is this just the
> explanation for why 6fd8b8503a0d should be in the Fixes tag?
No need to add the explanation to the patch.
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 |