Hi Dirk,
On Thu, Sep 7, 2017 at 11:12 AM, Geert Uytterhoeven
<[email protected]> wrote:
> On Thu, Sep 7, 2017 at 11:05 AM, Dirk Behme <[email protected]> wrote:
>> On 07.09.2017 10:59, Geert Uytterhoeven wrote:
>>> On Thu, Sep 7, 2017 at 10:42 AM, Dirk Behme <[email protected]>
>>> wrote:
>>>> On 07.09.2017 10:39, Geert Uytterhoeven wrote:
>>>>> On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <[email protected]>
>>>>> wrote:
>>>>>> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>>>>>>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[email protected]>
>>>>>>> wrote:
>>>>>>>> From: Hiromitsu Yamasaki <[email protected]>
>>>>>>>> DMA supports 32-bit words only,
>>>>>>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>>>>>>
>>>>>>>> Signed-off-by: Hiromitsu Yamasaki <[email protected]>
>>>>>>>> Signed-off-by: Dirk Behme <[email protected]>
>>>>>>>> ---
>>>>>>>> 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 <[email protected]>
>>>>>>
>>>>>> 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 -- [email protected]
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
