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

[Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>]

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