Re: [PATCH 2/3] TI QSPI: support large flash devices

[Vignesh Raghavendra <vigneshr@xxxxxx>]

On 10/12/19 5:40 pm, Jean Pihet wrote:
> Hi Vignesh,
> 
> On Tue, Dec 10, 2019 at 9:40 AM Vignesh Raghavendra <vigneshr@xxxxxx> wrote:
>>
>> Hi Jean,
>>
>> On 09/12/19 5:12 pm, Jean Pihet wrote:
>>> Hi Vignesh,
>>>
>>> On Mon, Dec 9, 2019 at 11:27 AM Vignesh Raghavendra <vigneshr@xxxxxx> wrote:
>>>>
>>>>
>>>>
>>>> On 06/12/19 9:30 pm, Jean Pihet wrote:
>>>>> The TI QSPI IP has limitations:
>>>>> - the MMIO region is 64MB in size
>>>>> - in non-MMIO mode, the transfer can handle 4096 words max.
>>>>>
>>>>> Add support for bigger devices.
>>>>> Use MMIO and DMA transfers below the 64MB boundary, use
>>>>> software generated transfers above.
>>>>
>>>> Could you change the $subject prefix to be "spi: spi-ti-qspi:"
>>>
>>> Yes sure.
>>>
>>>>
>>>>
>>>> [...]
>>>>
>>>>> -574,6 +601,7 @@ static int ti_qspi_exec_mem_op(struct spi_mem *mem,
>>>>>
>>>>>  static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
>>>>>       .exec_op = ti_qspi_exec_mem_op,
>>>>> +     .adjust_op_size = ti_qspi_adjust_op_size,
>>>>>  };
>>>>>
>>>>>  static int ti_qspi_start_transfer_one(struct spi_master *master,
>>>>> @@ -599,12 +627,11 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
>>>>>       frame_len_words = 0;
>>>>>       list_for_each_entry(t, &m->transfers, transfer_list)
>>>>>               frame_len_words += t->len / (t->bits_per_word >> 3);
>>>>> -     frame_len_words = min_t(unsigned int, frame_len_words, QSPI_FRAME);
>>>>>
>>>>>       /* setup command reg */
>>>>>       qspi->cmd = 0;
>>>>>       qspi->cmd |= QSPI_EN_CS(spi->chip_select);
>>>>> -     qspi->cmd |= QSPI_FLEN(frame_len_words);
>>>>> +     qspi->cmd |= QSPI_FLEN(QSPI_FRAME);
>>>>>
>>>>
>>>> Hmm, change itself is harmless. But what is the motivation behind the
>>>> change?
>>> Indeed this change does not hurt but is required to prevent an invalid
>>> FLEN value.
>>>
>>> Here are the details:
>>> - t->len is in bytes,
>>> - the length passed to qspi_transfer_msg is in bytes,
>>> - frame_len_words is the number of words in the SPI model,
>>> - the words as used by the TI QSPI IP is not the same as
>>> frame_len_words.
>>
>>> In TI QSPI the word size depends on the number of
>>> I/Os i in use (SPI_NBITS_xxx and op->data.buswidth).
>>>
>>
>> How did you get this impression? Word size is independent of number of
>> SPI I/O lines in use. There can be a slave of 32 bit word size on a
>> 1 bit SPI bus (SPI_NBITS_SINGLE).
>>
>> Flash devices have word size of 1 byte (IOW byte addressable)
>> but buswidth can be 1/2/4. A Quad IO flash does not mean word size of 32 bit
>> word size.
> 
> The assumption is that every transfer is based on 8 clock cycles (byte
> addressable) as for the SPI flash and that
> the optimization is about getting the most data bytes from every
> transfer (32 bits in quad I/O, 16 bits in dual, 8 bits in single).
> Is this a valid assumption?
> 

No, data phase does not have be 8 clock cycles long. This is only true
in case of 1 bit mode where 8 clock cycles are required to transfer a
full byte. In case of Quad mode, only 4 clock cycles are enough to
transfer 2 bytes of data with WLEN set to 1 byte (FLEN or number of
words read should be even).

Have you tried setting WLEN = 1 in Quad mode? Did you see any issue with
that configuration?

Regards
Vignesh

>>
>>
>>> For example a quad I/O transfer with t->bits_per_word=8 generates 4
>>> bytes of data every 8 clock cycles.
>>
>>> In this case frame_len_words =
>>> 16384. The maximum of bytes transferred by TI QSPI is 4096 * 4 =
>>> 16384. Setting FLEN to frame_len_words leads to an invalid value (max
>>> value is 0xFFF + 1).
>>>
>>
>> But we have:
>>
>>         list_for_each_entry(t, &m->transfers, transfer_list)
>>                 frame_len_words += t->len / (t->bits_per_word >> 3);
>>
>>         /* Clamps max words to 4096 words */
>>         frame_len_words = min_t(unsigned int, frame_len_words, QSPI_FRAME);
>>
>>         /* setup command reg */
>>         qspi->cmd = 0;
>>         qspi->cmd |= QSPI_EN_CS(spi->chip_select);
>>
>>         /* FLEN field is set to max of 4095 bytes as */
>>         qspi->cmd |= QSPI_FLEN(frame_len_words);
>>
>> So, without changes in patch 3/3, there is no need to set FLEN to be set to 0xFFF.
>> But FLEN needs to be set to even words in case of dual/quad mode as per AM437x TRM
> Correct but in that case the number of bytes received for every
> transfer is not optimum, i.e. 4x more data
> could be transferred in quad I/O.
> 
> The solution is to calculate frame_len_words from the bits_per_word
> and rx_nbits.
> 
> What do you think?
> 
> Thanks & regards,
> Jean
> 
>>
>>> So this changes takes the I/O mode into account by limiting the
>>> maximum number of bytes per frame in ti_qspi_adjust_op_size, by
>>> programming FLEN with the maximum allowed value and by stopping the
>>> transfer when the data is transferred.
>>>
>>> Does this make sense?
>>
>>>
>>> Regards,
>>> Jean
>>>
>>
>> --
>> Regards
>> Vignesh

-- 
Regards
Vignesh