Re: [PATCH] spi: sh-msiof: Fix the limit of data length when calculating the length of words

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Hoan,

On Thu, Apr 4, 2019 at 2:48 AM Hoan <na-hoan@xxxxxxxxxxx> wrote:
> On 2019/04/03 18:24, Geert Uytterhoeven wrote:
> > On Wed, Apr 3, 2019 at 11:17 AM Nguyen An Hoan <na-hoan@xxxxxxxxxxx> wrote:
> >> From: Hoan Nguyen An <na-hoan@xxxxxxxxxxx>
> >>
> >> We can use each word (data length) of 32bits (4 bytes),
> >> so that if the length is greater than 3bytes, we can
> >> align it with 4bytes of words.
> >>
> >> Signed-off-by: Hoan Nguyen An <na-hoan@xxxxxxxxxxx>
> > Thanks for your patch!
> >
> >> ---
> >>   drivers/spi/spi-sh-msiof.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> >> index e2eb466..1552c14 100644
> >> --- a/drivers/spi/spi-sh-msiof.c
> >> +++ b/drivers/spi/spi-sh-msiof.c
> >> @@ -930,7 +930,7 @@ static int sh_msiof_transfer_one(struct spi_controller *ctlr,
> >>          if (!spi_controller_is_slave(p->ctlr))
> >>                  sh_msiof_spi_set_clk_regs(p, clk_get_rate(p->clk), t->speed_hz);
> >>
> >> -       while (ctlr->dma_tx && len > 15) {
> >> +       while (ctlr->dma_tx && len > 3) {
> > This check is here to avoid using DMA (which incurs DMA setup overhead)
> > for short transfers.
> > Have you measured the performance impact of your change?
> >
> > Perhaps the code should be changed to:
> >
> >      #define DMA_MIN_LEN     16
> >
> >      while (ctlr->dma_tx && len >= DMA_MIN_LEN) {
> >
>
> Here the DMA feature for this driver has been initialized with the
> probe() function,
> which means that DMA will, if possible, use default.
> But when you add a patch to support DMA, You said DMA is only effective
> with 32 bits, so that DMA here serves data as multiples of 32 bits.
> I don't think the data length causes a cost for DMA when it's always
> used by default
> if possible (was initialized with the probe () function) and if the data
> is a 32bits
> multiple. In the opposite direction, have you measured the performance
> of using DMA
> if the data is greater than or equal to 16 bytes instead of 4 bytes?

commit b0d0ce8b6b91a0f6f99045b6019fc4c824634fb4
Author: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
Date:   Mon Jun 30 12:10:24 2014 +0200

    spi: sh-msiof: Add DMA support

    Add DMA support to the MSIOF driver using platform data.

    As MSIOF DMA is limited to 32-bit words (requiring byte/wordswapping for
    smaller wordsizes), and the group length is limited to 256 words, DMA is
    performed on two fixed pages, allocated and mapped at driver initialization
    time.

    Performance figures (in Mbps) on r8a7791/koelsch at different SPI clock
    frequencies for 1024-byte and 4096-byte transfers:

                       1024 bytes           4096 bytes
      -  3.25 MHz: PIO  2.1, DMA  2.6 | PIO  2.8, DMA  3.1
      -  6.5  MHz: PIO  3.2, DMA  4.4 | PIO  5.0, DMA  5.9
      - 13    MHz: PIO  4.2, DMA  6.6 | PIO  8.2, DMA 10.7
      - 26    MHz: PIO  5.9, DMA 10.4 | PIO 12.4, DMA 18.4

    Note that DMA is only faster than PIO for transfers that exceed the FIFO
    size (typically 64 words / 256 bytes).

    Also note that large transfers (larger than the group length for DMA, or
    larger than the FIFO size for PIO), should use cs-gpio (with the
    appropriate pinmux setup), as the hardware chipselect will be deasserted in
    between chunks.

    Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
    Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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