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@xxxxxxx>
> Cc: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> ---
> 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/ |
Attachment:
signature.asc
Description: PGP signature
