[U-Boot] [PATCH 2/2] spi: cadence-qspi: Add direct mode support
Simon Goldschmidt
simon.k.r.goldschmidt at gmail.com
Fri Oct 18 12:42:32 UTC 2019
On Fri, Oct 18, 2019 at 2:40 PM Vignesh Raghavendra <vigneshr at ti.com> wrote:
>
> Hi,
>
> On 18/10/19 2:34 PM, Simon Goldschmidt wrote:
> > On Thu, Oct 17, 2019 at 2:55 PM Simon Goldschmidt
> > <simon.k.r.goldschmidt at gmail.com> wrote:
> >>
> >> On Thu, Oct 17, 2019 at 2:44 PM Vignesh Raghavendra <vigneshr at ti.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 17/10/19 5:09 PM, Simon Goldschmidt wrote:
> >>>> On Mon, Oct 14, 2019 at 3:27 PM Vignesh Raghavendra <vigneshr at ti.com> wrote:
> >>>>>
> >>>>> Add support for Direct Access Controller mode of Cadence QSPI. This
> >>>>> allows MMIO access to SPI NOR flash providing better read performance.
> >>>>>
> >>>>> Signed-off-by: Vignesh R <vigneshr at ti.com>
> >>>>> Signed-off-by: Vignesh Raghavendra <vigneshr at ti.com>
> >>>>
> >>>> I've tested this on my socfpga cyclone5 board with an mt25ql256a (running at
> >>>> 50MHz) and it seems to work fine.
> >>>>
> >>>> However, I had the impression it was a bit slower, not faster, although I
> >>>> haven't measured, and running at 50MHz with 4 data lines, reading the whole
> >>>> flash takes about 1.5 seconds only, so without actually measureing it, it's
> >>>> hard to tell if this performance is really worth the 440 bytes of extra code
> >>>> it adds?
> >>>>
> >>>
> >>> I should have noted in the commit msg that direct mode is used only when
> >>> AHB window is at least 8MB. Working with smaller window sizes would mean
> >>> code would fall back to indirect mode most of the time.
> >>> I see that socfgpa don't seem to have large AHB windows and therefore
> >>> would not use direct access mode.
> >>
> >> Aha. Yes, socfpga gen5 has a 1MB window only, I think. So the speed hasn't
> >> changed.
> >>
> >>>
> >>> On TI platforms its possible to use DMA to read data from memory mapped
> >>> region using mem to mem DMA channels which improves throughput by 5 times.
> >>
> >> Hmm, I'm using a QSPI at 50 MHz and transferring 31 MByte takes about 1.5
> >> seconds. The maximum speed of that setup would be 25 MByte/s without any
> >> overhead (1.24 seconds for 31 MB). There's no way I could achieve an improvement
> >> by 5 on my platform!
> >>
> >>>
> >>>> Note that I'm already short on RAM in SPL (socfpga gen5), so these 440 bytes
> >>>> do hurt. Note that patch 1/2 of this series reduced SPL code size by 320 bytes
> >>>> for me :-)
> >>>>
> >>>
> >>> Hmm, I read this as patch 1 brings down memory consumption by 320 bytes
> >>> and patch 2 adds 440 bytes (delta being +120 bytes for entire series).
> >>> I can see if size can be optimized, but would like to avoid #ifdef'ery
> >>> within the driver if possible.
> >>
> >> Well, I can only say I'm currently struggling to keep SPI-NOR and MMC enabled
> >> at the same time in socfpga_socrates (moving more code to DM). And even if it
> >> sounds like not much, 440 bytes *are* much for me at this point.
> >>
> >> I still would prefer having a Kconfig option for this that can be disabled
> >> for socfpga.
> >
> > Oh well, maybe just go ahead adding this and I'll try how it fits. We can alway
> > add reduced functionality later by checking for CONFIG_SPL_SPI_FLASH_TINY,
> > whicht might be a better config option to guard this than something cadence-qspi
> > specific.
> >
>
> Okay, thanks! Guarding with CONFIG_SPL_SPI_FLASH_TINY as and where
> required seems fair idea. BTW, it would be great if you could confirm
> Quad read works fine when spi-tx-bus-width = 1 and spi-rx-bus-width = 4
> as I suggested in other thread?
Yes, I'll do that when I find the time. Unfortunately, I can only do
that on the devices
at work, not at home. I also would like to check the memory-mapped code works on
socfpga by removing the 8MB size check, does that make sense?
Regards,
Simon
>
> Regards
> Vignesh
>
> > Regards,
> > Simon
> >
> >>
> >> Regards,
> >> Simon
> >>
> >>>
> >>> Regards
> >>> Vignesh
> >>>
> >>>> Regards,
> >>>> Simon
> >>>>
> >>>>
> >>>>> ---
> >>>>> drivers/spi/cadence_qspi.c | 40 ++++++++++++----------
> >>>>> drivers/spi/cadence_qspi.h | 19 ++++++-----
> >>>>> drivers/spi/cadence_qspi_apb.c | 61 +++++++++++++++++++++++++++++-----
> >>>>> 3 files changed, 87 insertions(+), 33 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> >>>>> index 673a2e9a6c4c..6c98cbf39ae4 100644
> >>>>> --- a/drivers/spi/cadence_qspi.c
> >>>>> +++ b/drivers/spi/cadence_qspi.c
> >>>>> @@ -12,12 +12,13 @@
> >>>>> #include <spi.h>
> >>>>> #include <spi-mem.h>
> >>>>> #include <linux/errno.h>
> >>>>> +#include <linux/sizes.h>
> >>>>> #include "cadence_qspi.h"
> >>>>>
> >>>>> #define CQSPI_STIG_READ 0
> >>>>> #define CQSPI_STIG_WRITE 1
> >>>>> -#define CQSPI_INDIRECT_READ 2
> >>>>> -#define CQSPI_INDIRECT_WRITE 3
> >>>>> +#define CQSPI_READ 2
> >>>>> +#define CQSPI_WRITE 3
> >>>>>
> >>>>> static int cadence_spi_write_speed(struct udevice *bus, uint hz)
> >>>>> {
> >>>>> @@ -189,6 +190,7 @@ static int cadence_spi_remove(struct udevice *dev)
> >>>>>
> >>>>> static int cadence_spi_set_mode(struct udevice *bus, uint mode)
> >>>>> {
> >>>>> + struct cadence_spi_platdata *plat = bus->platdata;
> >>>>> struct cadence_spi_priv *priv = dev_get_priv(bus);
> >>>>>
> >>>>> /* Disable QSPI */
> >>>>> @@ -197,6 +199,10 @@ static int cadence_spi_set_mode(struct udevice *bus, uint mode)
> >>>>> /* Set SPI mode */
> >>>>> cadence_qspi_apb_set_clk_mode(priv->regbase, mode);
> >>>>>
> >>>>> + /* Enable Direct Access Controller */
> >>>>> + if (plat->use_dac_mode)
> >>>>> + cadence_qspi_apb_dac_mode_enable(priv->regbase);
> >>>>> +
> >>>>> /* Enable QSPI */
> >>>>> cadence_qspi_apb_controller_enable(priv->regbase);
> >>>>>
> >>>>> @@ -221,12 +227,12 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi,
> >>>>> if (!op->addr.nbytes)
> >>>>> mode = CQSPI_STIG_READ;
> >>>>> else
> >>>>> - mode = CQSPI_INDIRECT_READ;
> >>>>> + mode = CQSPI_READ;
> >>>>> } else {
> >>>>> if (!op->addr.nbytes || !op->data.buf.out)
> >>>>> mode = CQSPI_STIG_WRITE;
> >>>>> else
> >>>>> - mode = CQSPI_INDIRECT_WRITE;
> >>>>> + mode = CQSPI_WRITE;
> >>>>> }
> >>>>>
> >>>>> switch (mode) {
> >>>>> @@ -236,19 +242,15 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi,
> >>>>> case CQSPI_STIG_WRITE:
> >>>>> err = cadence_qspi_apb_command_write(base, op);
> >>>>> break;
> >>>>> - case CQSPI_INDIRECT_READ:
> >>>>> - err = cadence_qspi_apb_indirect_read_setup(plat, op);
> >>>>> - if (!err) {
> >>>>> - err = cadence_qspi_apb_indirect_read_execute
> >>>>> - (plat, op->data.nbytes, op->data.buf.in);
> >>>>> - }
> >>>>> + case CQSPI_READ:
> >>>>> + err = cadence_qspi_apb_read_setup(plat, op);
> >>>>> + if (!err)
> >>>>> + err = cadence_qspi_apb_read_execute(plat, op);
> >>>>> break;
> >>>>> - case CQSPI_INDIRECT_WRITE:
> >>>>> - err = cadence_qspi_apb_indirect_write_setup(plat, op);
> >>>>> - if (!err) {
> >>>>> - err = cadence_qspi_apb_indirect_write_execute
> >>>>> - (plat, op->data.nbytes, op->data.buf.out);
> >>>>> - }
> >>>>> + case CQSPI_WRITE:
> >>>>> + err = cadence_qspi_apb_write_setup(plat, op);
> >>>>> + if (!err)
> >>>>> + err = cadence_qspi_apb_write_execute(plat, op);
> >>>>> break;
> >>>>> default:
> >>>>> err = -1;
> >>>>> @@ -264,13 +266,17 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
> >>>>> ofnode subnode;
> >>>>>
> >>>>> plat->regbase = (void *)devfdt_get_addr_index(bus, 0);
> >>>>> - plat->ahbbase = (void *)devfdt_get_addr_index(bus, 1);
> >>>>> + plat->ahbbase = (void *)devfdt_get_addr_size_index(bus, 1,
> >>>>> + &plat->ahbsize);
> >>>>> plat->is_decoded_cs = dev_read_bool(bus, "cdns,is-decoded-cs");
> >>>>> plat->fifo_depth = dev_read_u32_default(bus, "cdns,fifo-depth", 128);
> >>>>> plat->fifo_width = dev_read_u32_default(bus, "cdns,fifo-width", 4);
> >>>>> plat->trigger_address = dev_read_u32_default(bus,
> >>>>> "cdns,trigger-address",
> >>>>> 0);
> >>>>> + /* Use DAC mode only when MMIO window is at least 8M wide */
> >>>>> + if (plat->ahbsize >= SZ_8M)
> >>>>> + plat->use_dac_mode = true;
> >>>>>
> >>>>> /* All other paramters are embedded in the child node */
> >>>>> subnode = dev_read_first_subnode(bus);
> >>>>> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
> >>>>> index e655b027d788..619f0bed8efd 100644
> >>>>> --- a/drivers/spi/cadence_qspi.h
> >>>>> +++ b/drivers/spi/cadence_qspi.h
> >>>>> @@ -23,6 +23,8 @@ struct cadence_spi_platdata {
> >>>>> u32 fifo_depth;
> >>>>> u32 fifo_width;
> >>>>> u32 trigger_address;
> >>>>> + fdt_addr_t ahbsize;
> >>>>> + bool use_dac_mode;
> >>>>>
> >>>>> /* Flash parameters */
> >>>>> u32 page_size;
> >>>>> @@ -52,20 +54,21 @@ struct cadence_spi_priv {
> >>>>> void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat);
> >>>>> void cadence_qspi_apb_controller_enable(void *reg_base_addr);
> >>>>> void cadence_qspi_apb_controller_disable(void *reg_base_addr);
> >>>>> +void cadence_qspi_apb_dac_mode_enable(void *reg_base);
> >>>>>
> >>>>> int cadence_qspi_apb_command_read(void *reg_base_addr,
> >>>>> const struct spi_mem_op *op);
> >>>>> int cadence_qspi_apb_command_write(void *reg_base_addr,
> >>>>> const struct spi_mem_op *op);
> >>>>>
> >>>>> -int cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat,
> >>>>> - const struct spi_mem_op *op);
> >>>>> -int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
> >>>>> - unsigned int rxlen, u8 *rxbuf);
> >>>>> -int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
> >>>>> - const struct spi_mem_op *op);
> >>>>> -int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
> >>>>> - unsigned int txlen, const u8 *txbuf);
> >>>>> +int cadence_qspi_apb_read_setup(struct cadence_spi_platdata *plat,
> >>>>> + const struct spi_mem_op *op);
> >>>>> +int cadence_qspi_apb_read_execute(struct cadence_spi_platdata *plat,
> >>>>> + const struct spi_mem_op *op);
> >>>>> +int cadence_qspi_apb_write_setup(struct cadence_spi_platdata *plat,
> >>>>> + const struct spi_mem_op *op);
> >>>>> +int cadence_qspi_apb_write_execute(struct cadence_spi_platdata *plat,
> >>>>> + const struct spi_mem_op *op);
> >>>>>
> >>>>> void cadence_qspi_apb_chipselect(void *reg_base,
> >>>>> unsigned int chip_select, unsigned int decoder_enable);
> >>>>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
> >>>>> index 8dd0495dfcf4..fd491f2c8104 100644
> >>>>> --- a/drivers/spi/cadence_qspi_apb.c
> >>>>> +++ b/drivers/spi/cadence_qspi_apb.c
> >>>>> @@ -189,6 +189,15 @@ void cadence_qspi_apb_controller_disable(void *reg_base)
> >>>>> writel(reg, reg_base + CQSPI_REG_CONFIG);
> >>>>> }
> >>>>>
> >>>>> +void cadence_qspi_apb_dac_mode_enable(void *reg_base)
> >>>>> +{
> >>>>> + unsigned int reg;
> >>>>> +
> >>>>> + reg = readl(reg_base + CQSPI_REG_CONFIG);
> >>>>> + reg |= CQSPI_REG_CONFIG_DIRECT;
> >>>>> + writel(reg, reg_base + CQSPI_REG_CONFIG);
> >>>>> +}
> >>>>> +
> >>>>> /* Return 1 if idle, otherwise return 0 (busy). */
> >>>>> static unsigned int cadence_qspi_wait_idle(void *reg_base)
> >>>>> {
> >>>>> @@ -512,8 +521,8 @@ int cadence_qspi_apb_command_write(void *reg_base, const struct spi_mem_op *op)
> >>>>> }
> >>>>>
> >>>>> /* Opcode + Address (3/4 bytes) + dummy bytes (0-4 bytes) */
> >>>>> -int cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat,
> >>>>> - const struct spi_mem_op *op)
> >>>>> +int cadence_qspi_apb_read_setup(struct cadence_spi_platdata *plat,
> >>>>> + const struct spi_mem_op *op)
> >>>>> {
> >>>>> unsigned int reg;
> >>>>> unsigned int rd_reg;
> >>>>> @@ -577,8 +586,9 @@ static int cadence_qspi_wait_for_data(struct cadence_spi_platdata *plat)
> >>>>> return -ETIMEDOUT;
> >>>>> }
> >>>>>
> >>>>> -int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
> >>>>> - unsigned int n_rx, u8 *rxbuf)
> >>>>> +static int
> >>>>> +cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
> >>>>> + unsigned int n_rx, u8 *rxbuf)
> >>>>> {
> >>>>> unsigned int remaining = n_rx;
> >>>>> unsigned int bytes_to_read = 0;
> >>>>> @@ -639,9 +649,26 @@ failrd:
> >>>>> return ret;
> >>>>> }
> >>>>>
> >>>>> +int cadence_qspi_apb_read_execute(struct cadence_spi_platdata *plat,
> >>>>> + const struct spi_mem_op *op)
> >>>>> +{
> >>>>> + u32 from = op->addr.val;
> >>>>> + void *buf = op->data.buf.in;
> >>>>> + size_t len = op->data.nbytes;
> >>>>> +
> >>>>> + if (plat->use_dac_mode && (from + len < plat->ahbsize)) {
> >>>>> + memcpy_fromio(buf, plat->ahbbase + from, len);
> >>>>> + if (!cadence_qspi_wait_idle(plat->regbase))
> >>>>> + return -EIO;
> >>>>> + return 0;
> >>>>> + }
> >>>>> +
> >>>>> + return cadence_qspi_apb_indirect_read_execute(plat, len, buf);
> >>>>> +}
> >>>>> +
> >>>>> /* Opcode + Address (3/4 bytes) */
> >>>>> -int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
> >>>>> - const struct spi_mem_op *op)
> >>>>> +int cadence_qspi_apb_write_setup(struct cadence_spi_platdata *plat,
> >>>>> + const struct spi_mem_op *op)
> >>>>> {
> >>>>> unsigned int reg;
> >>>>>
> >>>>> @@ -662,8 +689,9 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> -int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
> >>>>> - unsigned int n_tx, const u8 *txbuf)
> >>>>> +static int
> >>>>> +cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
> >>>>> + unsigned int n_tx, const u8 *txbuf)
> >>>>> {
> >>>>> unsigned int page_size = plat->page_size;
> >>>>> unsigned int remaining = n_tx;
> >>>>> @@ -735,6 +763,23 @@ failwr:
> >>>>> return ret;
> >>>>> }
> >>>>>
> >>>>> +int cadence_qspi_apb_write_execute(struct cadence_spi_platdata *plat,
> >>>>> + const struct spi_mem_op *op)
> >>>>> +{
> >>>>> + u32 to = op->addr.val;
> >>>>> + const void *buf = op->data.buf.out;
> >>>>> + size_t len = op->data.nbytes;
> >>>>> +
> >>>>> + if (plat->use_dac_mode && (to + len < plat->ahbsize)) {
> >>>>> + memcpy_toio(plat->ahbbase + to, buf, len);
> >>>>> + if (!cadence_qspi_wait_idle(plat->regbase))
> >>>>> + return -EIO;
> >>>>> + return 0;
> >>>>> + }
> >>>>> +
> >>>>> + return cadence_qspi_apb_indirect_write_execute(plat, len, buf);
> >>>>> +}
> >>>>> +
> >>>>> void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy)
> >>>>> {
> >>>>> unsigned int reg;
> >>>>> --
> >>>>> 2.23.0
> >>>>>
> >>>
> >>> --
> >>> Regards
> >>> Vignesh
>
> --
> Regards
> Vignesh
More information about the U-Boot
mailing list