Re: [PATCH] spi: imx: mx51-ecspi: Fix low-speed CONFIGREG delay calculation

[Marek Vasut <marex@denx.de>]

On 7/19/21 10:20 AM, Uwe Kleine-König wrote:

[...]

>> @@ -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?

I don't think so, what kind of spi_message would that be ?

And even if it was zero, the delay would be 2000000/~0U , so zero as 
well, which I suppose is the best we can do in such a case.

>> +
>> +	delay = (2 * 1000000) / min_speed_hz;
> 
> Orthogonal to your change: I wonder if we need to round up the division
> here.

That is not necessary, since the delay here is twice what it needs to be 
(because we really do not know what happens in the hardware internally).

>>   	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 */

This whole if/else is in fact based on 
Documentation/timers/timers-howto.rst , which says use usleep_range() 
for delays above 10uS or so.

The comment should be updated, but that's a separate patch.

-- 
Best regards,
Marek Vasut

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-56 Email: marex@denx.de