[U-Boot] [PATCH] spi: designware_spi: Fix off-by-one for checking fifo depth

[Axel Lin]

2014-12-19 15:35 GMT+08:00 Stefan Roese <sr at denx.de>:
> Hi Axel,
>
>
> On 19.12.2014 05:40, Axel Lin wrote:
>>
>> The depth could be from 2 to 256 from HW spec, so fix off-by-one for
>> checking
>> fifo depth.
>>
>> Signed-off-by: Axel Lin <axel.lin at ingics.com>
>> ---
>> Hi,
>> I don't have this hardware handy, so please test it.
>>   drivers/spi/designware_spi.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
>> index 98c9f03..14d0c23 100644
>> --- a/drivers/spi/designware_spi.c
>> +++ b/drivers/spi/designware_spi.c
>> @@ -164,7 +164,7 @@ static void spi_hw_init(struct dw_spi_priv *priv)
>>         if (!priv->fifo_len) {
>>                 u32 fifo;
>>
>> -               for (fifo = 2; fifo <= 257; fifo++) {
>> +               for (fifo = 2; fifo <= 256; fifo++) {
>>                         dw_writew(priv, DW_SPI_TXFLTR, fifo);
>>                         if (fifo != dw_readw(priv, DW_SPI_TXFLTR))
>>                                 break;
>
>
> Please note that this code was not written by myself but is based on the
> Linux kernel version. And from my current understanding, this code tries to
> detect the FIFO size. So this 257 being bigger than the max value of 256 is
> most likely correct. Also another 257 is a few lines below your code:
>
>                 dws->fifo_len = (fifo == 257) ? 0 : fifo;

hi Stefan,

After the for loop iteration with "no-match" case, the fifo variable will be
258 in original code. So above code won't catch such case.

>
> So I don't think that we need to change this here.
>
> BTW: It might be better to send this patch (or a question about this 256 vs
> 257) to the SPI Linux mailing list. The maintainer there will have deeper
> knowledge about this issue.

Well, I'll send similar patch for linux driver for review (in spi maillist).
Thanks.