| Message ID | 20191206160007.331801-3-jean.pihet@newoldbits.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TI QSPI: Add support for large flash devices | expand |
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:" [...] > -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? > ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG); > >
Hi Vignesh, On Mon, Dec 9, 2019 at 11:27 AM Vignesh Raghavendra <vigneshr@ti.com> 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). 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). 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
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@ti.com> 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 >
Hi Vignesh, On Tue, Dec 10, 2019 at 9:40 AM Vignesh Raghavendra <vigneshr@ti.com> 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@ti.com> 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
On 10/12/19 5:40 pm, Jean Pihet wrote: > Hi Vignesh, > > On Tue, Dec 10, 2019 at 9:40 AM Vignesh Raghavendra <vigneshr@ti.com> 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@ti.com> 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
diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c index 4680dad38ab2..13916232a959 100644 --- a/drivers/spi/spi-ti-qspi.c +++ b/drivers/spi/spi-ti-qspi.c @@ -524,6 +524,33 @@ static void ti_qspi_setup_mmap_read(struct spi_device *spi, u8 opcode, QSPI_SPI_SETUP_REG(spi->chip_select)); } +static int ti_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) +{ + struct ti_qspi *qspi = spi_controller_get_devdata(mem->spi->master); + size_t max_len; + + if (op->data.dir == SPI_MEM_DATA_IN) { + if (op->addr.val < qspi->mmap_size) { + /* Limit MMIO to the mmaped region */ + if (op->addr.val + op->data.nbytes > qspi->mmap_size) { + max_len = qspi->mmap_size - op->addr.val; + op->data.nbytes = min(op->data.nbytes, max_len); + } + } else { + /* + * Use fallback mode (SW generated transfers) above the + * mmaped region. + * Adjust size to comply with the QSPI max frame length. + */ + max_len = QSPI_FRAME * op->data.buswidth; + max_len -= 1 + op->addr.nbytes + op->dummy.nbytes; + op->data.nbytes = min(op->data.nbytes, max_len); + } + } + + return 0; +} + static int ti_qspi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op) { @@ -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); ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);