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

[Marek Vasut]

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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxx>
> > > Cc: David Jander <david@xxxxxxxxxxx>
> > > Cc: Fabio Estevam <festevam@xxxxxxxxx>
> > > Cc: Mark Brown <broonie@xxxxxxxxxx>
> > > Cc: Marek Vasut <marex@xxxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
> > > Tested-by: Fabio Estevam <festevam@xxxxxxxxx>
> > > ---
> > >
> > > 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@xxxxxxx>

Thanks for the extra check !