| Message ID | 20170906070507.26223-3-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dirk, 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> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07.09.2017 10:31, Geert Uytterhoeven wrote: > Hi Dirk, > > 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? Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dirk, 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? 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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07.09.2017 10:39, Geert Uytterhoeven wrote: > Hi Dirk, > > 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. Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dirk, 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07.09.2017 10:59, Geert Uytterhoeven wrote: > Hi Dirk, > > 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. Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dirk, 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... Note that this test does not exercise the code path affected by this particular patch ;-) 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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03.01.2018 18:25, Geert Uytterhoeven wrote: > 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. First, many thanks for looking into this! > Do you agree? If there are no more issues, then I agree ;) So lets rephrase that a little: We'll test what we have, now. And in case we find any remaining issues, we'll come back with more details, then. What will help us to look at the remaining patches. I apologize already now in case testing will take some time ;) Anyhow, many thanks again for looking into this. Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dirk, On Thu, Jan 4, 2018 at 8:19 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: > On 03.01.2018 18:25, Geert Uytterhoeven wrote: >> 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: >>>>>>> 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. > > First, many thanks for looking into this! > >> Do you agree? > > If there are no more issues, then I agree ;) So lets rephrase that a little: > We'll test what we have, now. And in case we find any remaining issues, > we'll come back with more details, then. What will help us to look at the > remaining patches. Sure, if more issues can be identified, we should look into fixing them as well. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 {