On Sun, Jul 18, 2021 at 11:11:43PM +0200, Marek Vasut wrote:
> The spi_imx->spi_bus_clk may be uninitialized and thus also zero in
> mx51_ecspi_prepare_message(), which would lead to division by zero
> in kernel. Since bitbang .setup_transfer callback which initializes
> the spi_imx->spi_bus_clk is called after bitbang prepare_message
> callback, iterate over all the transfers in spi_message, find the
> one with lowest bus frequency, and use that bus frequency for the
> delay calculation.
> 
> Note that it is not possible to move this CONFIGREG delay back into
> the .setup_transfer callback, because that is invoked too late, after
> the GPIO chipselects were already configured.
> 
> Fixes: 135cbd378eab ("spi: imx: mx51-ecspi: Reinstate low-speed CONFIGREG delay")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Mark Brown <broonie@kernel.org>
> ---
> Sigh ...
> ---
>  drivers/spi/spi-imx.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 4aee3db6d6df..e18338fc3108 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -505,7 +505,9 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
>  				      struct spi_message *msg)
>  {
>  	struct spi_device *spi = msg->spi;
> +	struct spi_transfer *xfer;
>  	u32 ctrl = MX51_ECSPI_CTRL_ENABLE;
> +	u32 min_speed_hz = ~0U;
>  	u32 testreg, delay;
>  	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
>  
> @@ -578,7 +580,13 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
>  	 * the SPI communication as the device on the other end would consider
>  	 * the change of SCLK polarity as a clock tick already.
>  	 */
> -	delay = (2 * 1000000) / spi_imx->spi_bus_clk;
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		if (!xfer->speed_hz)
> +			continue;
> +		min_speed_hz = min(xfer->speed_hz, min_speed_hz);
> +	}

Can it happen that all transfer's spped_hz are zero?

> +
> +	delay = (2 * 1000000) / min_speed_hz;

Orthogonal to your change: I wonder if we need to round up the division
here.

>  	if (likely(delay < 10))	/* SCLK is faster than 100 kHz */
>  		udelay(delay);
>  	else			/* SCLK is _very_ slow */

Also the comments are wrong here. Is SCLK is 150 kHz we have
min_speed_hz = 150000, right? Then delay becomes 13 and the slow freq
path is entered. The right comment (when keeping delay = (2 * 1000000) /
min_speed_hz) would be

	if (likely(delay < 10)) /* SCLK is faster than 181.818 kHz */

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |