diff mbox series

spi: sh-msiof: Fix the limit of data length when calculating the length of words

Message ID 1554283043-9206-1-git-send-email-na-hoan@jinso.co.jp (mailing list archive)
State New, archived
Headers show
Series spi: sh-msiof: Fix the limit of data length when calculating the length of words | expand

Commit Message

グェン・アン・ホァン April 3, 2019, 9:17 a.m. UTC 
From: Hoan Nguyen An <na-hoan@jinso.co.jp>

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@jinso.co.jp>
---
 drivers/spi/spi-sh-msiof.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven April 3, 2019, 9:24 a.m. UTC | #1 
Hi Hoan,

On Wed, Apr 3, 2019 at 11:17 AM Nguyen An Hoan <na-hoan@jinso.co.jp> wrote:
> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
>
> 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@jinso.co.jp>

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) {

>                 /*
>                  *  DMA supports 32-bit words only, hence pack 8-bit and 16-bit
>                  *  words, with byte resp. word swapping.
> @@ -974,7 +974,7 @@ static int sh_msiof_transfer_one(struct spi_controller *ctlr,
>                         return 0;
>         }
>
> -       if (bits <= 8 && len > 15) {
> +       if (bits <= 8 && len > 3) {

Likewise.

>                 bits = 32;
>                 swab = true;
>         } else {

Gr{oetje,eeting}s,

                        Geert
グェン・アン・ホァン April 4, 2019, 12:48 a.m. UTC | #2 
Dear Geert-san,

Always thanks for your replies!

On 2019/04/03 18:24, Geert Uytterhoeven wrote:
> Hi Hoan,
>
> On Wed, Apr 3, 2019 at 11:17 AM Nguyen An Hoan <na-hoan@jinso.co.jp> wrote:
>> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
>>
>> 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@jinso.co.jp>
> 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?


Thank You!

Hoan.


>>                  /*
>>                   *  DMA supports 32-bit words only, hence pack 8-bit and 16-bit
>>                   *  words, with byte resp. word swapping.
>> @@ -974,7 +974,7 @@ static int sh_msiof_transfer_one(struct spi_controller *ctlr,
>>                          return 0;
>>          }
>>
>> -       if (bits <= 8 && len > 15) {
>> +       if (bits <= 8 && len > 3) {
> Likewise.
>
>>                  bits = 32;
>>                  swab = true;
>>          } else {
> Gr{oetje,eeting}s,
>
>                          Geert
>
Geert Uytterhoeven April 4, 2019, 7:29 a.m. UTC | #3 
Hi Hoan,

On Thu, Apr 4, 2019 at 2:48 AM Hoan <na-hoan@jinso.co.jp> wrote:
> On 2019/04/03 18:24, Geert Uytterhoeven wrote:
> > On Wed, Apr 3, 2019 at 11:17 AM Nguyen An Hoan <na-hoan@jinso.co.jp> wrote:
> >> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> >>
> >> 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@jinso.co.jp>
> > 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@glider.be>
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@glider.be>
    Signed-off-by: Mark Brown <broonie@linaro.org>

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

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) {
 		/*
 		 *  DMA supports 32-bit words only, hence pack 8-bit and 16-bit
 		 *  words, with byte resp. word swapping.
@@ -974,7 +974,7 @@  static int sh_msiof_transfer_one(struct spi_controller *ctlr,
 			return 0;
 	}
 
-	if (bits <= 8 && len > 15) {
+	if (bits <= 8 && len > 3) {
 		bits = 32;
 		swab = true;
 	} else {