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

[Vignesh Raghavendra <vigneshr@xxxxxx>]

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.


> 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

> 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