Re: [PATCH 2/8] spi: sh-msiof: Fix DMA transfer size check

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Dirk Behme <dirk.behme@de.bosch.com>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Subject: Re: [PATCH 2/8] spi: sh-msiof: Fix DMA transfer size check
Date: Wed, 3 Jan 2018 18:25:17 +0100	[thread overview]
Message-ID: <CAMuHMdVvk2_0T7HdJbmd_QZ+7OtXAQYo3GZ6Xt2QPmd0N=1_qg@mail.gmail.com> (raw)
In-Reply-To: <CAMuHMdXQKj3F02zur3BZ95Et+btQD=rOfpMUC8cFWBnwF4Qxdw@mail.gmail.com>

Hi Dirk,

On Thu, Sep 7, 2017 at 11:12 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Thu, Sep 7, 2017 at 11:05 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> On 07.09.2017 10:59, Geert Uytterhoeven wrote:
>>> On Thu, Sep 7, 2017 at 10:42 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>> On 07.09.2017 10:39, Geert Uytterhoeven wrote:
>>>>> On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>>>> wrote:
>>>>>> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>>>>>>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>>>>>> wrote:
>>>>>>>> From: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
>>>>>>>> DMA supports 32-bit words only,
>>>>>>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>>>>>>
>>>>>>>> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
>>>>>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>>>>>> ---
>>>>>>>>     drivers/spi/spi-sh-msiof.c | 2 +-
>>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>>>>>>>> index 2b4d3a520176..f9300fdf41e5 100644
>>>>>>>> --- a/drivers/spi/spi-sh-msiof.c
>>>>>>>> +++ b/drivers/spi/spi-sh-msiof.c
>>>>>>>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct
>>>>>>>> spi_master
>>>>>>>> *master,
>>>>>>>>                                    break;
>>>>>>>>                            copy32 = copy_bswap32;
>>>>>>>>                    } else if (bits <= 16) {
>>>>>>>> -                       if (l & 1)
>>>>>>>> +                       if (l & 3)
>>>>>>>>                                    break;
>>>>>>>>                            copy32 = copy_wswap32;
>>>>>>>>                    } else {
>>>>>>>
>>>>>>> Looks OK, as l is in bytes, not 16-bit words.
>>>>>>>
>>>>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>
>>>>>> What do you think about CCing this to -stable?
>>>>>
>>>>> Sounds OK. Have you tested this?
>>>>
>>>> I tested all 8 patches together if they fix the issues I observed with
>>>> plain
>>>> mainline.
>>>
>>> Could you please elaborate about the issues you observed with plain
>>> mainline?
>>> It may help to identify which patches are responsible for fixing them.
>>
>> I've been told that an easy test case is to just cat random data (any mid
>> sized file) into SPI. E.g.:
>>
>> # cat /proc/cpuinfo > /dev/spidev32764.2
>> [  504.544948] spi_sh_msiof e6e90000.spi: failed to shut down hardware
>> [  504.551265] spidev spi32764.2: SPI transfer failed: -110
>> [  504.556625] spi_master spi32764: failed to transfer one message from
>> queue
>> [  504.564857] spi_sh_msiof e6e90000.spi: failed to shut down hardware
>> [  504.571177] spidev spi32764.2: SPI transfer failed: -110
>> [  504.576528] spi_master spi32764: failed to transfer one message from
>> queue
>> cat: write error: Connection timed out
>>
>> done on plain 4.13-rc6.
>
> Thank you, that's very valuable information!
> We'll give it a try...

After some investigation, this is caused by DMA completion triggering
too early for TX-only transfers.  This should indeed be fixed by "[PATCH 5/8]
spi: sh-msiof: Wait for Tx FIFO empty after DMA" you extracted from the BSP.

As that patch needs changes to apply (a) on current spi/for-next and (b)
without other patches from your series, I reworked it into "[PATCH] spi:
sh-msiof: Fix timeout failures for TX-only DMA transfers".

As for the other patches from your series:
  - Upstream already has commit 36735783fdb599c9 ("spi: sh-msiof: Fix DMA
    transfer size check"),
  - spi/for-next has commit b8761434bdec32fa ("spi: sh-msiof: Implement
    cs-gpios configuration").
The remaining were either rejected, retracted, or it is unclear which problem
(if any) they are really fixing.

Do you agree?
Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds