[U-Boot] [RFC PATCH] dm:spi:fsl_qspi add DM support
Peng Fan
B51431 at freescale.com
Sat Jan 24 16:00:31 CET 2015
Hi, Simon
On 1/23/2015 6:29 AM, Simon Glass wrote:
> Hi Peng,
>
> On 16 January 2015 at 22:59, Peng Fan <Peng.Fan at freescale.com> wrote:
>> Hi Simon ,Jagan
>>
>> This patch is based on git://git.denx.de/u-boot-spi.git master branch,
>> since some fsl_qspi's new feature is still in this git repo and have
>> not been merged to mainline.
>> I saw Simon sent out a new patch that remove the per_child_auto_alloc_size
>> from the platforms' driver code and move it to spi uclass driver. In
>> this patch I do not remove it, since this is a RFC version, and Jagan's
>> spi git repo still has it. I can remove it in formal version if needed.
>> Please take your time to review and comment this patch.
>>
>> Thanks.
>>
>> This patch adds DM support for fsl_qspi driver, the DM part needs
>> device tree support.
>>
>> Partial of the original driver code is reused, such as the AHB part,
>> the LUT initialization and etc. The driver now supports new DM and original
>> driver by define "CONFIG_DM_SPI". Until device tree is integrated, the
>> original part can be discarded.
>>
>> The driver code file is now as following:
>> "
>>
>> Common code that needs both by DM or original driver code.
>>
>> #if defined(CONFIG_DM_SPI)
>>
>> DM part
>>
>> #else
>>
>> Original driver code
>>
>> #endif
>> "
>>
>> In DM part, some reconstruction is done. A fsl_qspi_runcmd is included to
>> simplify the code, but not the original qspi_op_xxxxs. fsl_qspi_get_seqid
>> is included to get seqid, but not hardcoded in original qspi_op_xxxxs.
> Not sure where this driver is going - I'm happy to pick it up if you
> work things out with Jagan. For now I'll just make a few comments, in
> case they are useful.
Thanks for your comments. Since some features in spi repo of Jagan are
not into mainline, if DM support is not based on with the spi repo, more
work from your side will needed to resolve merging conflict. Also i'd
like to do this with the new qspi driver.
>
>> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
>> ---
>> drivers/spi/fsl_qspi.c | 420 +++++++++++++++++++++++++++++++++++++++++++++++--
>> drivers/spi/fsl_qspi.h | 1 +
>> 2 files changed, 405 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
>> index 5e0b069..ee151b3 100644
>> --- a/drivers/spi/fsl_qspi.c
>> +++ b/drivers/spi/fsl_qspi.c
>> @@ -13,6 +13,13 @@
>> #include <linux/sizes.h>
>> #include "fsl_qspi.h"
>>
>> +#ifdef CONFIG_DM_SPI
> Shouldn't need these #ifdefs around header files.
will remove uneccesary `ifdefs`
>
>> +#include <dm.h>
>> +#include <fdtdec.h>
>> +#include <asm/errno.h>
>> +#include <spi_flash.h>
>> +#endif
>> +
>> #define RX_BUFFER_SIZE 0x80
>> #ifdef CONFIG_MX6SX
>> #define TX_BUFFER_SIZE 0x200
>> @@ -71,27 +78,41 @@
>> #define qspi_write32 out_be32
>> #endif
>>
>> -static unsigned long spi_bases[] = {
>> - QSPI0_BASE_ADDR,
>> -#ifdef CONFIG_MX6SX
>> - QSPI1_BASE_ADDR,
>> -#endif
>> -};
>> +#ifdef CONFIG_DM_SPI
>> +DECLARE_GLOBAL_DATA_PTR;
>> +#define QUADSPI_AHBMAP_BANK_MAXSIZE SZ_64M
>>
>> -static unsigned long amba_bases[] = {
>> - QSPI0_AMBA_BASE,
>> -#ifdef CONFIG_MX6SX
>> - QSPI1_AMBA_BASE,
>> -#endif
>> +struct fsl_qspi_platdata {
>> + u32 max_hz;
>> + u32 reg_base;
>> + u32 amba_base;
>> + u32 flash_num;
>> };
>>
>> struct fsl_qspi {
>> + u32 reg_base;
>> + u32 amba_base;
> Should these be structure pointers?
Yeah. using structure pointer for reg_base is better.
>
>> + size_t cmd_len;
>> + u8 cmd_buf[32];
>> + size_t data_len;
>> + int qspi_is_init;
>> + size_t flash_size;
>> + u32 bank_memmap_phy[4];
>> + int cs;
> Do you need cs in here? The per-child platform data has it if you base
> it on u-boot-dm/i2c-working (which is what I suggest).
I need `cs` to configure qspi controller each time `sf probe bus:cs`. If
not use `cs` to configure controller, wrong chips will be accessed. I'll
look into i2c-working branch to see how to use it.
>
>> + u32 sf_addr;
>> + int flash_num;
>> + u8 cur_seqid;
>> + u32 freq;
>> +};
>> +#else
> Will there still be non-driver-model users of this driver? Can we
> convert them over?
yeah. The platforms that using this driver does not support DM/DT in
their default configuration file, so i'd like this driver support DM and
no-DM.
>
>> +struct fsl_qspi {
>> struct spi_slave slave;
>> unsigned long reg_base;
>> unsigned long amba_base;
>> u32 sf_addr;
>> u8 cur_seqid;
>> };
>> +#endif
>>
>> /* QSPI support swapping the flash read/write data
>> * in hardware for LS102xA, but not for VF610 */
>> @@ -104,11 +125,6 @@ static inline u32 qspi_endian_xchg(u32 data)
>> #endif
>> }
>>
>> -static inline struct fsl_qspi *to_qspi_spi(struct spi_slave *slave)
>> -{
>> - return container_of(slave, struct fsl_qspi, slave);
>> -}
>> -
>> static void qspi_set_lut(struct fsl_qspi *qspi)
>> {
>> struct fsl_qspi_regs *regs = (struct fsl_qspi_regs *)qspi->reg_base;
>> @@ -367,6 +383,377 @@ static void qspi_init_ahb_read(struct fsl_qspi_regs *regs)
>> }
>> #endif
>>
>> +#ifdef CONFIG_DM_SPI
>> +/* Get the SEQID for the command */
>> +static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
> s/q/priv/ or something a bit longer than one character.
ok.
>
>> +{
>> + switch (cmd) {
>> + case QSPI_CMD_FAST_READ:
>> + case QSPI_CMD_FAST_READ_4B:
>> + return SEQID_FAST_READ;
>> + case QSPI_CMD_WREN:
>> + return SEQID_WREN;
>> + case QSPI_CMD_RDSR:
>> + return SEQID_RDSR;
>> + case QSPI_CMD_SE:
>> + return SEQID_SE;
>> + case QSPI_CMD_PP:
>> + case QSPI_CMD_PP_4B:
>> + return SEQID_PP;
>> + case QSPI_CMD_RDID:
>> + return SEQID_RDID;
>> + case QSPI_CMD_BE_4K:
>> + return SEQID_BE_4K;
>> +#ifdef CONFIG_SPI_FLASH_BAR
>> + case QSPI_CMD_BRRD:
>> + return SEQID_BRRD;
>> + case QSPI_CMD_BRWR:
>> + return SEQID_BRWR;
>> + case QSPI_CMD_RDEAR:
>> + return SEQID_RDEAR;
>> + case QSPI_CMD_WREAR:
>> + return SEQID_WREAR;
>> +#endif
>> + default:
>> + break;
>> + }
>> + return -1;
>> +}
>> +
>> +/* Read out the data from the QUADSPI_RBDR buffer registers. */
>> +static void fsl_qspi_ip_read(struct fsl_qspi *q, int len, u8 *rxbuf)
>> +{
>> + struct fsl_qspi_regs *regs = (struct fsl_qspi_regs *)q->reg_base;
>> + u32 tmp;
>> + int i = 0;
>> +
>> + while (len > 0) {
>> + tmp = qspi_read32(®s->rbdr[i]);
>> + tmp = qspi_endian_xchg(tmp);
>> +
>> + if (len >= 4) {
>> + memcpy(rxbuf, &tmp, 4);
>> + rxbuf += 4;
>> + } else {
>> + memcpy(rxbuf, &tmp, len);
>> + break;
>> + }
>> +
>> + len -= 4;
>> + i++;
>> + }
>> +}
>> +
>> +/* Write data to the QUADSPI_TBDR buffer registers. */
>> +static void fsl_qspi_write_data(struct fsl_qspi *q, int len, u8 *txbuf)
>> +{
>> + struct fsl_qspi_regs *regs = (struct fsl_qspi_regs *)q->reg_base;
>> + u32 tmp;
>> + u32 t1, t2;
>> + int j;
>> +
>> + tmp = qspi_read32(®s->mcr);
>> + qspi_write32(®s->mcr, tmp | QSPI_MCR_CLR_RXF_MASK |
>> + QSPI_MCR_CLR_TXF_MASK);
>> +
>> + /* fill the TX data to the FIFO */
>> + t2 = len % 4;
>> + t1 = len >> 2; /* 4 Bytes aligned */
>> +
>> + for (j = 0; j < t1; j++) {
> What is 11?
ok. should use a better name, but not t1 or t2.
>
>> + memcpy(&tmp, txbuf, 4);
>> + tmp = qspi_endian_xchg(tmp);
>> + qspi_write32(®s->tbdr, tmp);
>> + txbuf += 4;
>> + }
>> +
>> + if (t2) {
>> + tmp = 0;
>> + memcpy(&tmp, txbuf, t2);
>> + tmp = qspi_endian_xchg(tmp);
>> + qspi_write32(®s->tbdr, tmp);
>> + }
>> +}
>> +
>> +static int fsl_qspi_runcmd(struct fsl_qspi *q, u8 cmd, int len)
>> +{
>> + struct fsl_qspi_regs *regs = (struct fsl_qspi_regs *)q->reg_base;
>> + int seqid;
>> + u32 reg;
>> +
>> + /* Wait previous cmd completed */
>> + while (qspi_read32(®s->sr) & QSPI_SR_BUSY_MASK)
>> + ;
> Should check for timeout here?
Yeah.
>> +
>> + /* save the reg */
>> + reg = qspi_read32(®s->mcr);
>> +
>> + qspi_write32(®s->sfar, q->amba_base + q->sf_addr);
>> + qspi_write32(®s->rbct, QSPI_RBCT_WMRK_MASK | QSPI_RBCT_RXBRD_USEIPS);
>> + qspi_write32(®s->mcr, reg | QSPI_MCR_CLR_RXF_MASK);
>> +
>> + /* trigger the LUT now */
>> + seqid = fsl_qspi_get_seqid(q, cmd);
>> + qspi_write32(®s->ipcr, (seqid << QSPI_IPCR_SEQID_SHIFT) | len);
>> +
>> + /* Wait until completed */
>> + while (qspi_read32(®s->sr) & QSPI_SR_BUSY_MASK)
>> + ;
>> +
>> + /* restore the MCR */
>> + qspi_write32(®s->mcr, reg);
>> +
>> +#ifdef CONFIG_SYS_FSL_QSPI_AHB
>> + /* After switch BANK, AHB buffer should also be invalid. */
>> + if ((QSPI_CMD_SE == cmd) || (QSPI_CMD_PP == cmd) ||
>> + (QSPI_CMD_BE_4K == cmd) || (QSPI_CMD_WREAR == cmd) ||
>> + (QSPI_CMD_BRWR == cmd))
>> + qspi_ahb_invalid(q);
>> +#endif
>> + return 0;
>> +}
>> +
>> +int fsl_qspi_init(struct fsl_qspi *qspi)
>> +{
>> + struct fsl_qspi_regs *regs;
>> + u32 smpr_val;
>> +
>> + regs = (struct fsl_qspi_regs *)qspi->reg_base;
>> +
>> + qspi_write32(®s->mcr, QSPI_MCR_RESERVED_MASK | QSPI_MCR_MDIS_MASK);
>> +
>> + smpr_val = qspi_read32(®s->smpr);
>> + qspi_write32(®s->smpr, smpr_val & ~(QSPI_SMPR_FSDLY_MASK |
>> + QSPI_SMPR_FSPHS_MASK | QSPI_SMPR_HSENA_MASK));
>> + qspi_write32(®s->mcr, QSPI_MCR_RESERVED_MASK);
>> +
>> + /* flash size is still using macro definition. */
>> + if (qspi->flash_num == 2) {
>> + qspi_write32(®s->sfa1ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE);
>> + qspi_write32(®s->sfa2ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE);
>> + qspi_write32(®s->sfb1ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE * 2);
>> + qspi_write32(®s->sfb2ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE * 2);
>> + } else if (qspi->flash_num == 4) {
>> + qspi_write32(®s->sfa1ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE);
>> + qspi_write32(®s->sfa2ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE * 2);
>> + qspi_write32(®s->sfb1ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE * 3);
>> + qspi_write32(®s->sfb2ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE * 4);
>> + } else {
>> + qspi_write32(®s->sfa1ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE);
>> + qspi_write32(®s->sfa2ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE);
>> + qspi_write32(®s->sfb1ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE);
>> + qspi_write32(®s->sfb2ad, qspi->amba_base +
>> + FSL_QSPI_FLASH_SIZE);
>> + }
>> +
>> + qspi_set_lut(qspi);
>> +
>> + smpr_val = qspi_read32(®s->smpr);
>> + smpr_val &= ~QSPI_SMPR_DDRSMP_MASK;
>> + qspi_write32(®s->smpr, smpr_val);
>> + qspi_write32(®s->mcr, QSPI_MCR_RESERVED_MASK | QSPI_MCR_END_CFD_LE);
>> +
>> +#ifdef CONFIG_SYS_FSL_QSPI_AHB
>> + qspi_init_ahb_read(regs);
>> +#endif
>> + return 0;
>> +}
>> +
>> +static int fsl_qspi_probe(struct udevice *bus)
>> +{
>> + struct fsl_qspi_platdata *plat = bus->platdata;
>> + struct fsl_qspi *priv = dev_get_priv(bus);
>> +
>> + priv->reg_base = plat->reg_base;
>> + priv->amba_base = plat->amba_base;
>> + priv->flash_num = plat->flash_num;
>> +
>> + if (!priv->qspi_is_init) {
>> + /* Initialization */
>> + fsl_qspi_init(priv);
>> + priv->qspi_is_init = 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int fsl_qspi_child_pre_probe(struct udevice *dev)
>> +{
>> + struct udevice *bus = dev_get_parent(dev);
>> + struct fsl_qspi *priv = dev_get_priv(bus);
>> + struct spi_slave *slave = dev_get_parentdata(dev);
>> + int cs = spi_chip_select(dev);
>> +
>> + slave->max_write_size = TX_BUFFER_SIZE;
>> +
>> + priv->cs = cs;
>> + priv->amba_base = priv->bank_memmap_phy[priv->cs];
>> +
>> + return 0;
>> +}
>> +
>> +static int fsl_qspi_xfer(struct udevice *dev, unsigned int bitlen,
>> + const void *dout, void *din, unsigned long flags)
>> +{
>> + struct udevice *bus = dev->parent;
>> + struct fsl_qspi *q = dev_get_priv(bus);
>> + u32 len = DIV_ROUND_UP(bitlen, 8);
>> + int ret = 0;
>> + u8 *buf;
>> + static u32 addr;
>> + static u8 opcode;
>> +
>> + if (!opcode && (flags & SPI_XFER_BEGIN)) {
>> + /* CMD PHASE */
>> + buf = (u8 *)dout;
>> + opcode = buf[0];
>> + if (len > 1) {
>> + addr = buf[1] << 16 | buf[2] << 8 | buf[3];
>> + q->sf_addr = addr;
>> + }
>> +
>> + /* Only transfer CMD */
>> + if (flags & SPI_XFER_END)
>> + ret = fsl_qspi_runcmd(q, opcode, 0);
>> +
>> + } else if (opcode) {
>> + /* Data phase */
>> + if (din) {
>> + /* read*/
>> + buf = (u8 *)din;
>> +#ifdef CONFIG_SYS_FSL_QSPI_AHB
>> + if (QSPI_CMD_FAST_READ == opcode) {
>> + qspi_ahb_read(q, buf, len);
>> + } else {
>> + ret = fsl_qspi_runcmd(q, opcode, len);
>> + if (!ret)
>> + fsl_qspi_ip_read(q, len, buf);
>> + }
>> +#else
>> + ret = fsl_qspi_runcmd(q, opcode, len);
>> + if (!ret)
>> + fsl_qspi_ip_read(q, len, buf);
>> +#endif
>> + } else if (dout) {
>> + /* Write data, First prepare data */
>> + buf = (u8 *)dout;
>> + fsl_qspi_write_data(q, len, buf);
>> + /* Run page program cmd */
>> + ret = fsl_qspi_runcmd(q, opcode, len);
>> + }
>> + }
>> +
>> + if (ret || (flags & SPI_XFER_END)) {
>> + opcode = 0;
>> + addr = 0;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int fsl_qspi_ofdata_to_platdata(struct udevice *bus)
>> +{
>> + struct fsl_qspi_platdata *plat = bus->platdata;
>> + const void *blob = gd->fdt_blob;
>> + int node = bus->of_offset;
>> + int subnode, flash_num = 0;
>> + u32 data[4];
>> + int ret;
>> +
>> + ret = fdtdec_get_int_array(blob, node, "reg", data, ARRAY_SIZE(data));
>> + if (ret) {
>> + printf("Error: can't get base addresses (ret = %d)!\n", ret);
>> + return -ENODEV;
> I think the device is there but it is wrong, so these should be -EINVAL
ok.
>
> Also better to avoid printf() and use debug(), for code size reasons I think.
Yeah. good suggestion.
>> + }
>> +
>> + /* Count flash numbers */
>> + fdt_for_each_subnode(blob, subnode, node)
>> + ++flash_num;
>> +
>> + if (flash_num == 0) {
>> + printf("Error: Missing flashes!\n");
>> + return -ENODEV;
>> + }
>> +
>> + plat->reg_base = data[0];
>> + plat->amba_base = data[2];
>> + plat->flash_num = flash_num;
>> +
>> + plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
>> + 50000000);
>> +
>> + debug("%s: reg_base=0x%x amba_base=0x%x max-frequency=%d\n",
>> + __func__, plat->reg_base, plat->amba_base, plat->max_hz);
>> +
>> + return 0;
>> +}
>> +
>> +static int fsl_qspi_set_speed(struct udevice *bus, u32 speed)
>> +{
>> + return 0;
>> +}
>> +
>> +static int fsl_qspi_set_mode(struct udevice *bus, u32 mode)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct dm_spi_ops fsl_qspi_ops = {
>> + .xfer = fsl_qspi_xfer,
>> + .set_speed = fsl_qspi_set_speed,
>> + .set_mode = fsl_qspi_set_mode,
>> +};
>> +
>> +static const struct udevice_id fsl_qspi_ids[] = {
>> + { .compatible = "fsl,vf610-qspi" },
>> + { .compatible = "fsl,imx6sx-qspi" },
>> + { }
>> +};
>> +
>> +U_BOOT_DRIVER(fsl_qspi) = {
>> + .name = "fsl_qspi",
>> + .id = UCLASS_SPI,
>> + .of_match = fsl_qspi_ids,
>> + .ops = &fsl_qspi_ops,
>> + .ofdata_to_platdata = fsl_qspi_ofdata_to_platdata,
>> + .platdata_auto_alloc_size = sizeof(struct fsl_qspi_platdata),
>> + .priv_auto_alloc_size = sizeof(struct fsl_qspi),
>> + .per_child_auto_alloc_size = sizeof(struct spi_slave),
>> + .probe = fsl_qspi_probe,
>> + .child_pre_probe = fsl_qspi_child_pre_probe,
>> +};
>> +
>> +#else
>> +static unsigned long spi_bases[] = {
>> + QSPI0_BASE_ADDR,
>> +#ifdef CONFIG_MX6SX
>> + QSPI1_BASE_ADDR,
>> +#endif
>> +};
>> +
>> +static unsigned long amba_bases[] = {
>> + QSPI0_AMBA_BASE,
>> +#ifdef CONFIG_MX6SX
>> + QSPI1_AMBA_BASE,
>> +#endif
>> +};
>> +
>> +static inline struct fsl_qspi *to_qspi_spi(struct spi_slave *slave)
>> +{
>> + return container_of(slave, struct fsl_qspi, slave);
>> +}
>> +
>> void spi_init()
>> {
>> /* do nothing */
>> @@ -782,3 +1169,4 @@ void spi_release_bus(struct spi_slave *slave)
>> {
>> /* Nothing to do */
>> }
>> +#endif
>> diff --git a/drivers/spi/fsl_qspi.h b/drivers/spi/fsl_qspi.h
>> index 6cb3610..ef4d4b5 100644
>> --- a/drivers/spi/fsl_qspi.h
>> +++ b/drivers/spi/fsl_qspi.h
>> @@ -104,6 +104,7 @@ struct fsl_qspi_regs {
>>
>> #define QSPI_RBCT_RXBRD_SHIFT 8
>> #define QSPI_RBCT_RXBRD_USEIPS (1 << QSPI_RBCT_RXBRD_SHIFT)
>> +#define QSPI_RBCT_WMRK_MASK 0x1F
>>
>> #define QSPI_SR_BUSY_SHIFT 0
>> #define QSPI_SR_BUSY_MASK (1 << QSPI_SR_BUSY_SHIFT)
>> --
>> 1.8.4
>>
>>
> Regards,
> Simon
Thanks,
Peng.
More information about the U-Boot
mailing list