[PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[Nobuhiro Iwamatsu]

This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to
master_flags, will be transmit mode. However, the current code, when
transmit mode and buffer is NULL, will be set to value of receive mode
size.
This is when the SPI_MASTER_MUST_TX is set to master_flags, sets to
transmit and receive either small size of FIFO, so as not to set the
actual size larger than value of FIFO.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
---
 drivers/spi/spi-sh-msiof.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index e57eec0..260f433 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -606,12 +606,26 @@ static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p,
 {
 	int fifo_shift;
 	int ret;
+	int rx_words = min_t(int, words, p->rx_fifo_size);
+	int tx_words = min_t(int, words, p->tx_fifo_size);
 
-	/* limit maximum word transfer to rx/tx fifo size */
-	if (tx_buf)
-		words = min_t(int, words, p->tx_fifo_size);
-	if (rx_buf)
-		words = min_t(int, words, p->rx_fifo_size);
+	/*
+	 * limit maximum word transfer to rx/tx fifo size.
+	 *
+	 * If SPI_MASTER_MUST_TX was enabled in master_flags, words was
+	 * set to small value of FIFO.
+	 */
+	if (p->chipdata->master_flags & SPI_MASTER_MUST_TX) {
+		if (rx_words > tx_words)
+			words = tx_words;
+		else
+			words = rx_words;
+	} else {
+		if (tx_buf)
+			words = tx_words;
+		if (rx_buf)
+			words = rx_words;
+	}
 
 	/* the fifo contents need shifting */
 	fifo_shift = 32 - bits;
-- 
2.1.3

Re: [PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

On Thu, Mar 12, 2015 at 11:31:16AM +0900, Nobuhiro Iwamatsu wrote:
> This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to
> master_flags, will be transmit mode. However, the current code, when
> transmit mode and buffer is NULL, will be set to value of receive mode
> size.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[Geert Uytterhoeven]

Hi Iwamatsu-san,

On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.yj@renesas.com> wrote:
> This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to
> master_flags, will be transmit mode. However, the current code, when
> transmit mode and buffer is NULL, will be set to value of receive mode
> size.

If SPI_MASTER_MUST_TX is set, tx_buf will never be NULL.
If an SPI slave driver submits a receive-only request, the SPI core will
provide a dummy buffer (spi_master.dummy_tx).

> This is when the SPI_MASTER_MUST_TX is set to master_flags, sets to
> transmit and receive either small size of FIFO, so as not to set the
> actual size larger than value of FIFO.
>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
> ---
>  drivers/spi/spi-sh-msiof.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index e57eec0..260f433 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -606,12 +606,26 @@ static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p,
>  {
>         int fifo_shift;
>         int ret;
> +       int rx_words = min_t(int, words, p->rx_fifo_size);
> +       int tx_words = min_t(int, words, p->tx_fifo_size);
>
> -       /* limit maximum word transfer to rx/tx fifo size */
> -       if (tx_buf)
> -               words = min_t(int, words, p->tx_fifo_size);
> -       if (rx_buf)
> -               words = min_t(int, words, p->rx_fifo_size);
> +       /*
> +        * limit maximum word transfer to rx/tx fifo size.
> +        *
> +        * If SPI_MASTER_MUST_TX was enabled in master_flags, words was
> +        * set to small value of FIFO.
> +        */
> +       if (p->chipdata->master_flags & SPI_MASTER_MUST_TX) {
> +               if (rx_words > tx_words)
> +                       words = tx_words;
> +               else
> +                       words = rx_words;
> +       } else {
> +               if (tx_buf)
> +                       words = tx_words;
> +               if (rx_buf)
> +                       words = rx_words;
> +       }
>
>         /* the fifo contents need shifting */
>         fifo_shift = 32 - bits;

Sorry, I fail to see what exactly this is fixing.

If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either
  a) transmit-only.
  b) bidirectional (transmit buffer may be a dummy, provided by the SPI core).

For case a, only the TX FIFO size matters.
  - The original code ignored the RX FIFO size (rx_buf == NULL),
  - After your change, it always uses the minimum of the TX and RX FIFO sizes
    (granted, the RX FIFO size is larger, so this doesn't make a difference on
    current hardware).

For case b, both FIFO sizes matter, and the original code handled that fine
(tx_buf != NULL, rx_buf != NULL).

What am I missing?
Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI
core?

I've just verified that with today's tree (renesas-drivers-2015-03-12-v4.0-rc3),
SPI_MASTER_MUST_TX works fine, and a dummy tx_buf is passed when needed.

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

Re: [PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

On Thu, Mar 12, 2015 at 01:35:10PM +0100, Geert Uytterhoeven wrote:
> On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu

> > This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to
> > master_flags, will be transmit mode. However, the current code, when
> > transmit mode and buffer is NULL, will be set to value of receive mode
> > size.

> If SPI_MASTER_MUST_TX is set, tx_buf will never be NULL.
> If an SPI slave driver submits a receive-only request, the SPI core will
> provide a dummy buffer (spi_master.dummy_tx).

Right, my reading of the changelog was that this was a red herring.

> > +       int rx_words = min_t(int, words, p->rx_fifo_size);
> > +       int tx_words = min_t(int, words, p->tx_fifo_size);

> Sorry, I fail to see what exactly this is fixing.

> If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either
>   a) transmit-only.
>   b) bidirectional (transmit buffer may be a dummy, provided by the SPI core).

> For case a, only the TX FIFO size matters.
>   - The original code ignored the RX FIFO size (rx_buf == NULL),
>   - After your change, it always uses the minimum of the TX and RX FIFO sizes
>     (granted, the RX FIFO size is larger, so this doesn't make a difference on
>     current hardware).

...

> What am I missing?
> Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI
> core?

My understanding was that it was just about making sure that the driver
picked the minimum of the two FIFO sizes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[Nobuhiro Iwamatsu]

Hi,

Thanks for your comment.

2015-03-12 21:35 GMT+09:00 Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>:
> Hi Iwamatsu-san,
>
> On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.yj-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
>> This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to
>> master_flags, will be transmit mode. However, the current code, when
>> transmit mode and buffer is NULL, will be set to value of receive mode
>> size.
>
> If SPI_MASTER_MUST_TX is set, tx_buf will never be NULL.
> If an SPI slave driver submits a receive-only request, the SPI core will
> provide a dummy buffer (spi_master.dummy_tx).

I see.
>
>> This is when the SPI_MASTER_MUST_TX is set to master_flags, sets to
>> transmit and receive either small size of FIFO, so as not to set the
>> actual size larger than value of FIFO.
>>
>> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  drivers/spi/spi-sh-msiof.c | 24 +++++++++++++++++++-----
>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>> index e57eec0..260f433 100644
>> --- a/drivers/spi/spi-sh-msiof.c
>> +++ b/drivers/spi/spi-sh-msiof.c
>> @@ -606,12 +606,26 @@ static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p,
>>  {
>>         int fifo_shift;
>>         int ret;
>> +       int rx_words = min_t(int, words, p->rx_fifo_size);
>> +       int tx_words = min_t(int, words, p->tx_fifo_size);
>>
>> -       /* limit maximum word transfer to rx/tx fifo size */
>> -       if (tx_buf)
>> -               words = min_t(int, words, p->tx_fifo_size);
>> -       if (rx_buf)
>> -               words = min_t(int, words, p->rx_fifo_size);
>> +       /*
>> +        * limit maximum word transfer to rx/tx fifo size.
>> +        *
>> +        * If SPI_MASTER_MUST_TX was enabled in master_flags, words was
>> +        * set to small value of FIFO.
>> +        */
>> +       if (p->chipdata->master_flags & SPI_MASTER_MUST_TX) {
>> +               if (rx_words > tx_words)
>> +                       words = tx_words;
>> +               else
>> +                       words = rx_words;
>> +       } else {
>> +               if (tx_buf)
>> +                       words = tx_words;
>> +               if (rx_buf)
>> +                       words = rx_words;
>> +       }
>>
>>         /* the fifo contents need shifting */
>>         fifo_shift = 32 - bits;
>
> Sorry, I fail to see what exactly this is fixing.
>
> If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either
>   a) transmit-only.
>   b) bidirectional (transmit buffer may be a dummy, provided by the SPI core).
>
> For case a, only the TX FIFO size matters.
>   - The original code ignored the RX FIFO size (rx_buf == NULL),
>   - After your change, it always uses the minimum of the TX and RX FIFO sizes
>     (granted, the RX FIFO size is larger, so this doesn't make a difference on
>     current hardware).

Yes.

>
> For case b, both FIFO sizes matter, and the original code handled that fine
> (tx_buf != NULL, rx_buf != NULL).
>
> What am I missing?
> Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI
> core?

When tx_buf != NULL and rx_buf != NULL, current code uses FIFO size of
rx_buffer.
Since TX FIFO size is smaller than RX FIFO  size, corrent code set the
wrong value to SITMDR2 register.
Therefore, this patch selects a smaller FIFO size, adds the function to set.

Does this has become a description?

>
> I've just verified that with today's tree (renesas-drivers-2015-03-12-v4.0-rc3),
> SPI_MASTER_MUST_TX works fine, and a dummy tx_buf is passed when needed.

I think correctly work on hardware. However, driver has been set to
the correct register value?

>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert

Best regards,
  Nobuhiro

-- 
Nobuhiro Iwamatsu
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[Geert Uytterhoeven]

Hi Iwamatsu-san,

On Mon, Mar 16, 2015 at 2:18 AM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.yj@renesas.com> wrote:
> 2015-03-12 21:35 GMT+09:00 Geert Uytterhoeven <geert@linux-m68k.org>:
>> On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu
>> <nobuhiro.iwamatsu.yj@renesas.com> wrote:

>>> -       /* limit maximum word transfer to rx/tx fifo size */
>>> -       if (tx_buf)
>>> -               words = min_t(int, words, p->tx_fifo_size);
>>> -       if (rx_buf)
>>> -               words = min_t(int, words, p->rx_fifo_size);

>> Sorry, I fail to see what exactly this is fixing.
>>
>> If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either
>>   a) transmit-only.
>>   b) bidirectional (transmit buffer may be a dummy, provided by the SPI core).
>>
>> For case a, only the TX FIFO size matters.
>>   - The original code ignored the RX FIFO size (rx_buf == NULL),
>>   - After your change, it always uses the minimum of the TX and RX FIFO sizes
>>     (granted, the RX FIFO size is larger, so this doesn't make a difference on
>>     current hardware).
>
> Yes.
>
>>
>> For case b, both FIFO sizes matter, and the original code handled that fine
>> (tx_buf != NULL, rx_buf != NULL).
>>
>> What am I missing?
>> Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI
>> core?
>
> When tx_buf != NULL and rx_buf != NULL, current code uses FIFO size of
> rx_buffer.
> Since TX FIFO size is smaller than RX FIFO  size, corrent code set the
> wrong value to SITMDR2 register.

if tx_buf != NULL and rx_buf != NULL, current code does:

    words = min_t(int, words, p->tx_fifo_size);
    words = min_t(int, words, p->rx_fifo_size);

Hence words will be the minimum of the original value of words, tx_fifo_size,
and rx_fifo_size. What's wrong about that?

> Therefore, this patch selects a smaller FIFO size, adds the function to set.
>
> Does this has become a description?
>
>>
>> I've just verified that with today's tree (renesas-drivers-2015-03-12-v4.0-rc3),
>> SPI_MASTER_MUST_TX works fine, and a dummy tx_buf is passed when needed.
>
> I think correctly work on hardware. However, driver has been set to
> the correct register value?

I printed the value of words, which is passed to sh_msiof_spi_set_mode_regs().

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

Re: [PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[Nobuhiro Iwamatsu]

Hi,

(2015/03/16 16:50), Geert Uytterhoeven wrote:
> Hi Iwamatsu-san,
>
> On Mon, Mar 16, 2015 at 2:18 AM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.yj@renesas.com>  wrote:
>> 2015-03-12 21:35 GMT+09:00 Geert Uytterhoeven<geert@linux-m68k.org>:
>>> On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu
>>> <nobuhiro.iwamatsu.yj@renesas.com>  wrote:
>
>>>> -       /* limit maximum word transfer to rx/tx fifo size */
>>>> -       if (tx_buf)
>>>> -               words = min_t(int, words, p->tx_fifo_size);
>>>> -       if (rx_buf)
>>>> -               words = min_t(int, words, p->rx_fifo_size);
>
>>> Sorry, I fail to see what exactly this is fixing.
>>>
>>> If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either
>>>    a) transmit-only.
>>>    b) bidirectional (transmit buffer may be a dummy, provided by the SPI core).
>>>
>>> For case a, only the TX FIFO size matters.
>>>    - The original code ignored the RX FIFO size (rx_buf == NULL),
>>>    - After your change, it always uses the minimum of the TX and RX FIFO sizes
>>>      (granted, the RX FIFO size is larger, so this doesn't make a difference on
>>>      current hardware).
>>
>> Yes.
>>
>>>
>>> For case b, both FIFO sizes matter, and the original code handled that fine
>>> (tx_buf != NULL, rx_buf != NULL).
>>>
>>> What am I missing?
>>> Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI
>>> core?
>>
>> When tx_buf != NULL and rx_buf != NULL, current code uses FIFO size of
>> rx_buffer.
>> Since TX FIFO size is smaller than RX FIFO  size, corrent code set the
>> wrong value to SITMDR2 register.
>
> if tx_buf != NULL and rx_buf != NULL, current code does:
>
>      words = min_t(int, words, p->tx_fifo_size);
>      words = min_t(int, words, p->rx_fifo_size);
>
> Hence words will be the minimum of the original value of words, tx_fifo_size,
> and rx_fifo_size. What's wrong about that?
>

I see. I understood about this.
Sorry, my bad.

Mark, if you do not apply this patch to your repository, could you ignore this?

>> Therefore, this patch selects a smaller FIFO size, adds the function to set.
>>
>> Does this has become a description?
>>
>>>
>>> I've just verified that with today's tree (renesas-drivers-2015-03-12-v4.0-rc3),
>>> SPI_MASTER_MUST_TX works fine, and a dummy tx_buf is passed when needed.
>>
>> I think correctly work on hardware. However, driver has been set to
>> the correct register value?
>
> I printed the value of words, which is passed to sh_msiof_spi_set_mode_regs().
>
> Gr{oetje,eeting}s,
>
>                          Geert
>

Best regards,
   Nobuhiro

Re: [PATCH] spi: sh-msiof: Fix limit maximum word transfer size of FIFO size

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

On Wed, Mar 18, 2015 at 12:50:31PM +0900, Nobuhiro Iwamatsu wrote:
> (2015/03/16 16:50), Geert Uytterhoeven wrote:

> I see. I understood about this.
> Sorry, my bad.

> Mark, if you do not apply this patch to your repository, could you ignore this?

OK, I'll drop it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]