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

[Jean Pihet <jean.pihet@xxxxxxxxxxxxxx>]

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?

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