Re: [PATCH v2 5/6] mtd: spi-nor: Convert cadence-quadspi to use spi-mem framework

From: Tudor.Ambarus
Date: Sat May 30 2020 - 09:50:29 EST

Next message: Jens Axboe: "Re: [PATCH v3 0/2] CQ-seq only based timeouts"
Previous message: Toralf FÃrster: "Re: fatal: unable to access 'https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/': SSL certificate problem: certificate has expired"
In reply to: Vignesh Raghavendra: "[PATCH v2 5/6] mtd: spi-nor: Convert cadence-quadspi to use spi-mem framework"
Next in thread: Vignesh Raghavendra: "[PATCH v2 6/6] spi: Move cadence-quadspi driver to drivers/spi/"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Vignesh,

On Tuesday, May 26, 2020 12:36:03 PM EEST Vignesh Raghavendra wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> From: Ramuthevar Vadivel Murugan
> <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
>
> Move cadence-quadspi driver to use spi-mem framework. This is required
> to make the driver support for SPI NAND flashes in future.
>
> Driver is feature compliant with existing SPI NOR version.
>
> Signed-off-by: Ramuthevar Vadivel Murugan
> <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> Signed-off-by: Vignesh
> Raghavendra <vigneshr@xxxxxx>
> ---
> .../mtd/spi-nor/controllers/cadence-quadspi.c | 469 +++++++-----------
> 1 file changed, 183 insertions(+), 286 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/controllers/cadence-quadspi.c
> b/drivers/mtd/spi-nor/controllers/cadence-quadspi.c index
> 608ca657ff7f..c1df4b221889 100644
> --- a/drivers/mtd/spi-nor/controllers/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/controllers/cadence-quadspi.c
> @@ -3,6 +3,8 @@
> * Driver for Cadence QSPI Controller
> *
> * Copyright Altera Corporation (C) 2012-2014. All rights reserved.
> + * Copyright Intel Corporation (C) 2019-2020. All rights reserved.
> + * Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com
> */
> #include <linux/clk.h>
> #include <linux/completion.h>
> @@ -17,9 +19,6 @@
> #include <linux/jiffies.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> -#include <linux/mtd/mtd.h>
> -#include <linux/mtd/partitions.h>
> -#include <linux/mtd/spi-nor.h>
> #include <linux/of_device.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> @@ -27,6 +26,7 @@
> #include <linux/reset.h>
> #include <linux/sched.h>
> #include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> #include <linux/timer.h>
>
> #define CQSPI_NAME "cadence-qspi"
> @@ -36,16 +36,12 @@
> #define CQSPI_NEEDS_WR_DELAY BIT(0)
> #define CQSPI_DISABLE_DAC_MODE BIT(1)
>
> -/* Capabilities mask */
> -#define CQSPI_BASE_HWCAPS_MASK \
> - (SNOR_HWCAPS_READ | SNOR_HWCAPS_READ_FAST | \
> - SNOR_HWCAPS_READ_1_1_2 | SNOR_HWCAPS_READ_1_1_4 | \
> - SNOR_HWCAPS_PP)
> +/* Capabilities */
> +#define CQSPI_SUPPORTS_OCTAL BIT(0)
>
> struct cqspi_st;
>
> struct cqspi_flash_pdata {
> - struct spi_nor nor;
> struct cqspi_st *cqspi;
> u32 clk_rate;
> u32 read_delay;
> @@ -58,7 +54,6 @@ struct cqspi_flash_pdata {
> u8 data_width;
> u8 cs;
> bool registered;

you can drop this, as it is no longer used

> - bool use_direct_mode;
> };
>
> struct cqspi_st {
> @@ -71,7 +66,6 @@ struct cqspi_st {
> void __iomem *ahb_base;
> resource_size_t ahb_size;
> struct completion transfer_complete;
> - struct mutex bus_mutex;
>
> struct dma_chan *rx_chan;
> struct completion rx_dma_complete;
> @@ -85,6 +79,7 @@ struct cqspi_st {
> bool rclk_en;
> u32 trigger_address;
> u32 wr_delay;
> + bool use_dac_mode;

is use_dac_mode better than use_direct_mode? If you prefer "dac", maybe you
can rename this variable in 2/6

> struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
> };
>
> @@ -283,9 +278,8 @@ static irqreturn_t cqspi_irq_handler(int this_irq, void
> *dev) return IRQ_HANDLED;
> }
>
> -static unsigned int cqspi_calc_rdreg(struct spi_nor *nor)
> +static unsigned int cqspi_calc_rdreg(struct cqspi_flash_pdata *f_pdata)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> u32 rdreg = 0;
>
> rdreg |= f_pdata->inst_width << CQSPI_REG_RD_INSTR_TYPE_INSTR_LSB;
> @@ -352,19 +346,21 @@ static int cqspi_exec_flash_cmd(struct cqspi_st
> *cqspi, unsigned int reg) return cqspi_wait_idle(cqspi);
> }
>
> -static int cqspi_command_read(struct spi_nor *nor, u8 opcode,
> - u8 *rxbuf, size_t n_rx)
> +static int cqspi_command_read(struct cqspi_flash_pdata *f_pdata,
> + const struct spi_mem_op *op)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> void __iomem *reg_base = cqspi->iobase;
> + u8 *rxbuf = op->data.buf.in;
> + u8 opcode = op->cmd.opcode;

dedicated variable for opcode is not really needed, it is used only once.

> + size_t n_rx = op->data.nbytes;
> unsigned int rdreg;
> unsigned int reg;
> size_t read_len;
> int status;
>
> if (!n_rx || n_rx > CQSPI_STIG_DATA_LEN_MAX || !rxbuf) {
> - dev_err(nor->dev,
> + dev_err(&cqspi->pdev->dev,
> "Invalid input argument, len %zu rxbuf 0x%p\n",
> n_rx, rxbuf);
> return -EINVAL;
> @@ -372,7 +368,7 @@ static int cqspi_command_read(struct spi_nor *nor, u8
> opcode,
>
> reg = opcode << CQSPI_REG_CMDCTRL_OPCODE_LSB;
>
> - rdreg = cqspi_calc_rdreg(nor);
> + rdreg = cqspi_calc_rdreg(f_pdata);
> writel(rdreg, reg_base + CQSPI_REG_RD_INSTR);
>
> reg |= (0x1 << CQSPI_REG_CMDCTRL_RD_EN_LSB);
> @@ -401,25 +397,35 @@ static int cqspi_command_read(struct spi_nor *nor, u8
> opcode, return 0;
> }
>
> -static int cqspi_command_write(struct spi_nor *nor, const u8 opcode,
> - const u8 *txbuf, size_t n_tx)
> +static int cqspi_command_write(struct cqspi_flash_pdata *f_pdata,
> + const struct spi_mem_op *op)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> void __iomem *reg_base = cqspi->iobase;
> + const u8 opcode = op->cmd.opcode;

dedicated variable for opcode is not really needed, it is used only once.

> + const u8 *txbuf = op->data.buf.out;
> + size_t n_tx = op->data.nbytes;
> unsigned int reg;
> unsigned int data;
> size_t write_len;
> - int ret;
>
> if (n_tx > CQSPI_STIG_DATA_LEN_MAX || (n_tx && !txbuf)) {
> - dev_err(nor->dev,
> + dev_err(&cqspi->pdev->dev,
> "Invalid input argument, cmdlen %zu txbuf 0x%p\n",
> n_tx, txbuf);
> return -EINVAL;
> }
>
> reg = opcode << CQSPI_REG_CMDCTRL_OPCODE_LSB;
> +
> + if (op->addr.nbytes) {
> + reg |= (0x1 << CQSPI_REG_CMDCTRL_ADDR_EN_LSB);
> + reg |= ((op->addr.nbytes - 1) &
> CQSPI_REG_CMDCTRL_ADD_BYTES_MASK)
> + <<
> CQSPI_REG_CMDCTRL_ADD_BYTES_LSB;

you have a 80 chars checkpatch warning here, maybe replace it with smth like
reg |= ((op->addr.nbytes - 1) &
CQSPI_REG_CMDCTRL_ADD_BYTES_MASK) <<
CQSPI_REG_CMDCTRL_ADD_BYTES_LSB;

> +
> + writel(op->addr.val, reg_base + CQSPI_REG_CMDADDRESS);
> + }
> +
> if (n_tx) {
> reg |= (0x1 << CQSPI_REG_CMDCTRL_WR_EN_LSB);
> reg |= ((n_tx - 1) & CQSPI_REG_CMDCTRL_WR_BYTES_MASK)
> @@ -437,73 +443,46 @@ static int cqspi_command_write(struct spi_nor *nor,
> const u8 opcode, writel(data, reg_base + CQSPI_REG_CMDWRITEDATAUPPER); }
> }
> - ret = cqspi_exec_flash_cmd(cqspi, reg);
> - return ret;
> -}
> -
> -static int cqspi_command_write_addr(struct spi_nor *nor,
> - const u8 opcode, const unsigned int
> addr) -{
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> - struct cqspi_st *cqspi = f_pdata->cqspi;
> - void __iomem *reg_base = cqspi->iobase;
> - unsigned int reg;
> -
> - reg = opcode << CQSPI_REG_CMDCTRL_OPCODE_LSB;
> - reg |= (0x1 << CQSPI_REG_CMDCTRL_ADDR_EN_LSB);
> - reg |= ((nor->addr_width - 1) & CQSPI_REG_CMDCTRL_ADD_BYTES_MASK)
> - << CQSPI_REG_CMDCTRL_ADD_BYTES_LSB;
> -
> - writel(addr, reg_base + CQSPI_REG_CMDADDRESS);
>
> return cqspi_exec_flash_cmd(cqspi, reg);
> }
>
> -static int cqspi_read_setup(struct spi_nor *nor)
> +static int cqspi_read_setup(struct cqspi_flash_pdata *f_pdata,
> + const struct spi_mem_op *op)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> void __iomem *reg_base = cqspi->iobase;
> unsigned int dummy_clk = 0;
> unsigned int reg;
>
> - reg = nor->read_opcode << CQSPI_REG_RD_INSTR_OPCODE_LSB;
> - reg |= cqspi_calc_rdreg(nor);
> + reg = op->cmd.opcode << CQSPI_REG_RD_INSTR_OPCODE_LSB;
> + reg |= cqspi_calc_rdreg(f_pdata);
>
> /* Setup dummy clock cycles */
> - dummy_clk = nor->read_dummy;
> + dummy_clk = op->dummy.nbytes * 8;
> if (dummy_clk > CQSPI_DUMMY_CLKS_MAX)
> dummy_clk = CQSPI_DUMMY_CLKS_MAX;
>
> - if (dummy_clk / 8) {
> - reg |= (1 << CQSPI_REG_RD_INSTR_MODE_EN_LSB);
> - /* Set mode bits high to ensure chip doesn't enter XIP */
> - writel(0xFF, reg_base + CQSPI_REG_MODE_BIT);
> -
> - /* Need to subtract the mode byte (8 clocks). */
> - if (f_pdata->inst_width != CQSPI_INST_TYPE_QUAD)
> - dummy_clk -= 8;
> -
> - if (dummy_clk)
> - reg |= (dummy_clk & CQSPI_REG_RD_INSTR_DUMMY_MASK)
> - << CQSPI_REG_RD_INSTR_DUMMY_LSB;
> - }
> + if (dummy_clk / 8)

if (dummy_clk) should be enough. dummy_clk is either zero, or a multiple of
eight, or CQSPI_DUMMY_CLKS_MAX.

> + reg |= (dummy_clk & CQSPI_REG_RD_INSTR_DUMMY_MASK)
> + << CQSPI_REG_RD_INSTR_DUMMY_LSB;

>
> writel(reg, reg_base + CQSPI_REG_RD_INSTR);
>
> /* Set address width */
> reg = readl(reg_base + CQSPI_REG_SIZE);
> reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
> - reg |= (nor->addr_width - 1);
> + reg |= (op->addr.nbytes - 1);
> writel(reg, reg_base + CQSPI_REG_SIZE);
> return 0;
> }
>
> -static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
> - loff_t from_addr, const size_t n_rx)
> +static int cqspi_indirect_read_execute(struct cqspi_flash_pdata *f_pdata,
> + u8 *rxbuf, loff_t from_addr,
> + const size_t n_rx)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> + struct device *dev = &cqspi->pdev->dev;
> void __iomem *reg_base = cqspi->iobase;
> void __iomem *ahb_base = cqspi->ahb_base;
> unsigned int remaining = n_rx;
> @@ -526,13 +505,13 @@ static int cqspi_indirect_read_execute(struct spi_nor
> *nor, u8 *rxbuf,
>
> while (remaining > 0) {
> if (!wait_for_completion_timeout(&cqspi->transfer_complete,
> - msecs_to_jiffies(CQSPI_READ_TIMEOUT_MS)))
> +
> msecs_to_jiffies(CQSPI_READ_TIMEOUT_MS))) ret = -ETIMEDOUT;
>
> bytes_to_read = cqspi_get_rd_sram_level(cqspi);
>
> if (ret && bytes_to_read == 0) {
> - dev_err(nor->dev, "Indirect read timeout, no
> bytes\n"); + dev_err(dev, "Indirect read timeout, no
> bytes\n"); goto failrd;
> }
>
> @@ -568,8 +547,7 @@ static int cqspi_indirect_read_execute(struct spi_nor
> *nor, u8 *rxbuf, ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_INDIRECTRD,
> CQSPI_REG_INDIRECTRD_DONE_MASK, 0);
> if (ret) {
> - dev_err(nor->dev,
> - "Indirect read completion error (%i)\n", ret);
> + dev_err(dev, "Indirect read completion error (%i)\n", ret);
> goto failrd;
> }
>
> @@ -591,32 +569,32 @@ static int cqspi_indirect_read_execute(struct spi_nor
> *nor, u8 *rxbuf, return ret;
> }
>
> -static int cqspi_write_setup(struct spi_nor *nor)
> +static int cqspi_write_setup(struct cqspi_flash_pdata *f_pdata,
> + const struct spi_mem_op *op)
> {
> unsigned int reg;
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> void __iomem *reg_base = cqspi->iobase;
>
> /* Set opcode. */
> - reg = nor->program_opcode << CQSPI_REG_WR_INSTR_OPCODE_LSB;
> + reg = op->cmd.opcode << CQSPI_REG_WR_INSTR_OPCODE_LSB;
> writel(reg, reg_base + CQSPI_REG_WR_INSTR);
> - reg = cqspi_calc_rdreg(nor);
> + reg = cqspi_calc_rdreg(f_pdata);
> writel(reg, reg_base + CQSPI_REG_RD_INSTR);
>
> reg = readl(reg_base + CQSPI_REG_SIZE);
> reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
> - reg |= (nor->addr_width - 1);
> + reg |= (op->addr.nbytes - 1);
> writel(reg, reg_base + CQSPI_REG_SIZE);
> return 0;
> }
>
> -static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t
> to_addr, - const u8 *txbuf, const
> size_t n_tx) +static int cqspi_indirect_write_execute(struct
> cqspi_flash_pdata *f_pdata, + loff_t
> to_addr, const u8 *txbuf, + const
> size_t n_tx)
> {
> - const unsigned int page_size = nor->page_size;
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> + struct device *dev = &cqspi->pdev->dev;
> void __iomem *reg_base = cqspi->iobase;
> unsigned int remaining = n_tx;
> unsigned int write_bytes;
> @@ -646,7 +624,7 @@ static int cqspi_indirect_write_execute(struct spi_nor
> *nor, loff_t to_addr, while (remaining > 0) {
> size_t write_words, mod_bytes;
>
> - write_bytes = remaining > page_size ? page_size : remaining;
> + write_bytes = remaining;
> write_words = write_bytes / 4;
> mod_bytes = write_bytes % 4;
> /* Write 4 bytes at a time then single bytes. */
> @@ -663,8 +641,8 @@ static int cqspi_indirect_write_execute(struct spi_nor
> *nor, loff_t to_addr, }
>
> if (!wait_for_completion_timeout(&cqspi->transfer_complete,
> - msecs_to_jiffies(CQSPI_TIMEOUT_MS)))
> { - dev_err(nor->dev, "Indirect write timeout\n"); +
>
> msecs_to_jiffies(CQSPI_TIMEOUT_MS))) { + dev_err(dev,
> "Indirect write timeout\n");
> ret = -ETIMEDOUT;
> goto failwr;
> }
> @@ -679,8 +657,7 @@ static int cqspi_indirect_write_execute(struct spi_nor
> *nor, loff_t to_addr, ret = cqspi_wait_for_bit(reg_base +
> CQSPI_REG_INDIRECTWR,
> CQSPI_REG_INDIRECTWR_DONE_MASK, 0);
> if (ret) {
> - dev_err(nor->dev,
> - "Indirect write completion error (%i)\n", ret);
> + dev_err(dev, "Indirect write completion error (%i)\n", ret);
> goto failwr;
> }
>
> @@ -704,9 +681,8 @@ static int cqspi_indirect_write_execute(struct spi_nor
> *nor, loff_t to_addr, return ret;
> }
>
> -static void cqspi_chipselect(struct spi_nor *nor)
> +static void cqspi_chipselect(struct cqspi_flash_pdata *f_pdata)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> void __iomem *reg_base = cqspi->iobase;
> unsigned int chip_select = f_pdata->cs;
> @@ -745,9 +721,8 @@ static unsigned int calculate_ticks_for_ns(const
> unsigned int ref_clk_hz, return ticks;
> }
>
> -static void cqspi_delay(struct spi_nor *nor)
> +static void cqspi_delay(struct cqspi_flash_pdata *f_pdata)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> void __iomem *iobase = cqspi->iobase;
> const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
> @@ -831,11 +806,10 @@ static void cqspi_controller_enable(struct cqspi_st
> *cqspi, bool enable) writel(reg, reg_base + CQSPI_REG_CONFIG);
> }
>
> -static void cqspi_configure(struct spi_nor *nor)
> +static void cqspi_configure(struct cqspi_flash_pdata *f_pdata,
> + unsigned long sclk)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> - const unsigned int sclk = f_pdata->clk_rate;
> int switch_cs = (cqspi->current_cs != f_pdata->cs);
> int switch_ck = (cqspi->sclk != sclk);
>
> @@ -845,14 +819,14 @@ static void cqspi_configure(struct spi_nor *nor)
> /* Switch chip select. */
> if (switch_cs) {
> cqspi->current_cs = f_pdata->cs;
> - cqspi_chipselect(nor);
> + cqspi_chipselect(f_pdata);
> }
>
> /* Setup baudrate divisor and delays */
> if (switch_ck) {
> cqspi->sclk = sclk;
> cqspi_config_baudrate_div(cqspi);
> - cqspi_delay(nor);
> + cqspi_delay(f_pdata);
> cqspi_readdata_capture(cqspi, !cqspi->rclk_en,
> f_pdata->read_delay);
> }
> @@ -861,26 +835,25 @@ static void cqspi_configure(struct spi_nor *nor)
> cqspi_controller_enable(cqspi, 1);
> }
>
> -static int cqspi_set_protocol(struct spi_nor *nor, const int read)
> +static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
> + const struct spi_mem_op *op)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> -
> f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
> f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
> f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
>
> - if (read) {
> - switch (nor->read_proto) {
> - case SNOR_PROTO_1_1_1:
> + if (op->data.dir == SPI_MEM_DATA_IN) {
> + switch (op->data.buswidth) {
> + case 1:
> f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> break;
> - case SNOR_PROTO_1_1_2:
> + case 2:
> f_pdata->data_width = CQSPI_INST_TYPE_DUAL;
> break;
> - case SNOR_PROTO_1_1_4:
> + case 4:
> f_pdata->data_width = CQSPI_INST_TYPE_QUAD;
> break;
> - case SNOR_PROTO_1_1_8:
> + case 8:
> f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> break;
> default:
> @@ -888,36 +861,32 @@ static int cqspi_set_protocol(struct spi_nor *nor,
> const int read) }
> }
>
> - cqspi_configure(nor);
> -
> return 0;
> }
>
> -static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
> - size_t len, const u_char *buf)
> +static ssize_t cqspi_write(struct cqspi_flash_pdata *f_pdata,
> + const struct spi_mem_op *op)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> + loff_t to = op->addr.val;
> + size_t len = op->data.nbytes;
> + const u_char *buf = op->data.buf.out;
> int ret;
>
> - ret = cqspi_set_protocol(nor, 0);
> + ret = cqspi_set_protocol(f_pdata, op);
> if (ret)
> return ret;
>
> - ret = cqspi_write_setup(nor);
> + ret = cqspi_write_setup(f_pdata, op);
> if (ret)
> return ret;
>
> - if (f_pdata->use_direct_mode) {
> + if (cqspi->use_dac_mode && ((to + len) <= cqspi->ahb_size)) {
> memcpy_toio(cqspi->ahb_base + to, buf, len);
> - ret = cqspi_wait_idle(cqspi);
> - } else {
> - ret = cqspi_indirect_write_execute(nor, to, buf, len);
> + return cqspi_wait_idle(cqspi);
> }
> - if (ret)
> - return ret;
>
> - return len;
> + return cqspi_indirect_write_execute(f_pdata, to, buf, len);
> }
>
> static void cqspi_rx_dma_callback(void *param)
> @@ -927,11 +896,11 @@ static void cqspi_rx_dma_callback(void *param)
> complete(&cqspi->rx_dma_complete);
> }
>
> -static int cqspi_direct_read_execute(struct spi_nor *nor, u_char *buf,
> - loff_t from, size_t len)
> +static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
> + u_char *buf, loff_t from, size_t len)
> {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> struct cqspi_st *cqspi = f_pdata->cqspi;
> + struct device *dev = &cqspi->pdev->dev;
> enum dma_ctrl_flags flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
> dma_addr_t dma_src = (dma_addr_t)cqspi->mmap_phys_base + from;
> int ret = 0;
> @@ -944,15 +913,15 @@ static int cqspi_direct_read_execute(struct spi_nor
> *nor, u_char *buf, return 0;
> }
>
> - dma_dst = dma_map_single(nor->dev, buf, len, DMA_FROM_DEVICE);
> - if (dma_mapping_error(nor->dev, dma_dst)) {
> - dev_err(nor->dev, "dma mapping failed\n");
> + dma_dst = dma_map_single(dev, buf, len, DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, dma_dst)) {
> + dev_err(dev, "dma mapping failed\n");
> return -ENOMEM;
> }
> tx = dmaengine_prep_dma_memcpy(cqspi->rx_chan, dma_dst, dma_src,
> len, flags);
> if (!tx) {
> - dev_err(nor->dev, "device_prep_dma_memcpy error\n");
> + dev_err(dev, "device_prep_dma_memcpy error\n");
> ret = -EIO;
> goto err_unmap;
> }
> @@ -964,7 +933,7 @@ static int cqspi_direct_read_execute(struct spi_nor
> *nor, u_char *buf,
>
> ret = dma_submit_error(cookie);
> if (ret) {
> - dev_err(nor->dev, "dma_submit_error %d\n", cookie);
> + dev_err(dev, "dma_submit_error %d\n", cookie);
> ret = -EIO;
> goto err_unmap;
> }
> @@ -973,99 +942,68 @@ static int cqspi_direct_read_execute(struct spi_nor
> *nor, u_char *buf, if
> (!wait_for_completion_timeout(&cqspi->rx_dma_complete,
> msecs_to_jiffies(len))) {
> dmaengine_terminate_sync(cqspi->rx_chan);
> - dev_err(nor->dev, "DMA wait_for_completion_timeout\n");
> + dev_err(dev, "DMA wait_for_completion_timeout\n");
> ret = -ETIMEDOUT;
> goto err_unmap;
> }
>
> err_unmap:
> - dma_unmap_single(nor->dev, dma_dst, len, DMA_FROM_DEVICE);
> + dma_unmap_single(dev, dma_dst, len, DMA_FROM_DEVICE);
>
> return ret;
> }
>
> -static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
> - size_t len, u_char *buf)
> -{
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> - int ret;
> -
> - ret = cqspi_set_protocol(nor, 1);
> - if (ret)
> - return ret;
> -
> - ret = cqspi_read_setup(nor);
> - if (ret)
> - return ret;
> -
> - if (f_pdata->use_direct_mode)
> - ret = cqspi_direct_read_execute(nor, buf, from, len);
> - else
> - ret = cqspi_indirect_read_execute(nor, buf, from, len);
> - if (ret)
> - return ret;
> -
> - return len;
> -}
> -
> -static int cqspi_erase(struct spi_nor *nor, loff_t offs)
> +static ssize_t cqspi_read(struct cqspi_flash_pdata *f_pdata,
> + const struct spi_mem_op *op)
> {
> + struct cqspi_st *cqspi = f_pdata->cqspi;
> + loff_t from = op->addr.val;
> + size_t len = op->data.nbytes;
> + u_char *buf = op->data.buf.in;
> int ret;
>
> - ret = cqspi_set_protocol(nor, 0);
> + ret = cqspi_set_protocol(f_pdata, op);
> if (ret)
> return ret;
>
> - /* Send write enable, then erase commands. */
> - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN, NULL, 0);

Dropping the Write Enable for erases can be done in a separate patch, since it
is already done in the SPI NOR core. This would ease the review.

> + ret = cqspi_read_setup(f_pdata, op);
> if (ret)
> return ret;
>
> - /* Set up command buffer. */
> - ret = cqspi_command_write_addr(nor, nor->erase_opcode, offs);
> - if (ret)
> - return ret;
> + if (cqspi->use_dac_mode && ((from + len) <= cqspi->ahb_size))
> + return cqspi_direct_read_execute(f_pdata, buf, from, len);
>
> - return 0;
> + return cqspi_indirect_read_execute(f_pdata, buf, from, len);
> }
>
> -static int cqspi_prep(struct spi_nor *nor)
> +static int cqspi_mem_process(struct spi_mem *mem, const struct spi_mem_op
> *op) {
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> - struct cqspi_st *cqspi = f_pdata->cqspi;
> -
> - mutex_lock(&cqspi->bus_mutex);
> -
> - return 0;
> -}
> + struct cqspi_st *cqspi = spi_master_get_devdata(mem->spi->master);
> + struct cqspi_flash_pdata *f_pdata;
>
> -static void cqspi_unprep(struct spi_nor *nor)
> -{
> - struct cqspi_flash_pdata *f_pdata = nor->priv;
> - struct cqspi_st *cqspi = f_pdata->cqspi;
> + f_pdata = &cqspi->f_pdata[mem->spi->chip_select];
> + cqspi_configure(f_pdata, mem->spi->max_speed_hz);
>
> - mutex_unlock(&cqspi->bus_mutex);
> -}
> + if (op->data.dir == SPI_MEM_DATA_IN && op->data.buf.in) {
> + if (!op->addr.nbytes)
> + return cqspi_command_read(f_pdata, op);
>
> -static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, size_t
> len) -{
> - int ret;
> + return cqspi_read(f_pdata, op);
> + }
>
> - ret = cqspi_set_protocol(nor, 0);
> - if (!ret)
> - ret = cqspi_command_read(nor, opcode, buf, len);
> + if (!op->addr.nbytes || !op->data.buf.out)
> + return cqspi_command_write(f_pdata, op);
>
> - return ret;
> + return cqspi_write(f_pdata, op);
> }
>
> -static int cqspi_write_reg(struct spi_nor *nor, u8 opcode, const u8 *buf,
> - size_t len)
> +static int cqspi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op
> *op) {
> int ret;
>
> - ret = cqspi_set_protocol(nor, 0);
> - if (!ret)
> - ret = cqspi_command_write(nor, opcode, buf, len);
> + ret = cqspi_mem_process(mem, op);
> + if (ret)
> + dev_err(&mem->spi->dev, "operation failed with %d\n", ret);
>
> return ret;
> }
> @@ -1107,26 +1045,26 @@ static int cqspi_of_get_flash_pdata(struct
> platform_device *pdev, return 0;
> }
>
> -static int cqspi_of_get_pdata(struct platform_device *pdev)
> +static int cqspi_of_get_pdata(struct cqspi_st *cqspi)
> {
> - struct device_node *np = pdev->dev.of_node;
> - struct cqspi_st *cqspi = platform_get_drvdata(pdev);
> + struct device *dev = &cqspi->pdev->dev;
> + struct device_node *np = dev->of_node;
>
> cqspi->is_decoded_cs = of_property_read_bool(np,
> "cdns,is-decoded-cs");
>
> if (of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth))
> { - dev_err(&pdev->dev, "couldn't determine fifo-depth\n"); +
> dev_err(dev, "couldn't determine fifo-depth\n");
> return -ENXIO;
> }
>
> if (of_property_read_u32(np, "cdns,fifo-width", &cqspi->fifo_width))
> { - dev_err(&pdev->dev, "couldn't determine fifo-width\n"); +
> dev_err(dev, "couldn't determine fifo-width\n");
> return -ENXIO;
> }
>
> if (of_property_read_u32(np, "cdns,trigger-address",
> &cqspi->trigger_address)) {
> - dev_err(&pdev->dev, "couldn't determine trigger-address\n");
> + dev_err(dev, "couldn't determine trigger-address\n");
> return -ENXIO;
> }
>
> @@ -1169,7 +1107,7 @@ static void cqspi_controller_init(struct cqspi_st
> *cqspi) cqspi_controller_enable(cqspi, 1);
> }
>
> -static void cqspi_request_mmap_dma(struct cqspi_st *cqspi)
> +static int cqspi_request_mmap_dma(struct cqspi_st *cqspi)
> {
> dma_cap_mask_t mask;
>
> @@ -1178,133 +1116,79 @@ static void cqspi_request_mmap_dma(struct cqspi_st
> *cqspi)
>
> cqspi->rx_chan = dma_request_chan_by_mask(&mask);
> if (IS_ERR(cqspi->rx_chan)) {
> - dev_err(&cqspi->pdev->dev, "No Rx DMA available\n");
> + int ret = PTR_ERR(cqspi->rx_chan);
> +
> + if (ret != -EPROBE_DEFER)
> + dev_err(&cqspi->pdev->dev, "No Rx DMA available\n");
> cqspi->rx_chan = NULL;
> - return;
> + return ret;
> }
> init_completion(&cqspi->rx_dma_complete);
> +
> + return 0;
> }

Can we have the EPROBE_DEFER changes in a dedicated patch?

>
> -static const struct spi_nor_controller_ops cqspi_controller_ops = {
> - .prepare = cqspi_prep,
> - .unprepare = cqspi_unprep,
> - .read_reg = cqspi_read_reg,
> - .write_reg = cqspi_write_reg,
> - .read = cqspi_read,
> - .write = cqspi_write,
> - .erase = cqspi_erase,
> +static const struct spi_controller_mem_ops cqspi_mem_ops = {
> + .exec_op = cqspi_exec_mem_op,
> };
>
> -static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node
> *np) +static int cqspi_setup_flash(struct cqspi_st *cqspi)
> {
> struct platform_device *pdev = cqspi->pdev;
> struct device *dev = &pdev->dev;
> - const struct cqspi_driver_platdata *ddata;
> - struct spi_nor_hwcaps hwcaps;
> + struct device_node *np = dev->of_node;
> struct cqspi_flash_pdata *f_pdata;
> - struct spi_nor *nor;
> - struct mtd_info *mtd;
> unsigned int cs;
> - int i, ret;
> -
> - ddata = of_device_get_match_data(dev);
> - if (!ddata) {
> - dev_err(dev, "Couldn't find driver data\n");
> - return -EINVAL;
> - }
> - hwcaps.mask = ddata->hwcaps_mask;
>
> /* Get flash device data */
> for_each_available_child_of_node(dev->of_node, np) {
> - ret = of_property_read_u32(np, "reg", &cs);
> - if (ret) {
> + if (of_property_read_u32(np, "reg", &cs)) {
> dev_err(dev, "Couldn't determine chip select.\n");
> - goto err;
> + continue;

reg is mandatory, you should keep the behaviour as it was.

> }
>
> if (cs >= CQSPI_MAX_CHIPSELECT) {
> - ret = -EINVAL;
> dev_err(dev, "Chip select %d out of range.\n", cs);
> - goto err;
> + continue;
> }
>
> f_pdata = &cqspi->f_pdata[cs];
> f_pdata->cqspi = cqspi;
> f_pdata->cs = cs;
>
> - ret = cqspi_of_get_flash_pdata(pdev, f_pdata, np);
> - if (ret)
> - goto err;
> -
> - nor = &f_pdata->nor;
> - mtd = &nor->mtd;
> -
> - mtd->priv = nor;
> -
> - nor->dev = dev;
> - spi_nor_set_flash_node(nor, np);
> - nor->priv = f_pdata;
> - nor->controller_ops = &cqspi_controller_ops;
> -
> - mtd->name = devm_kasprintf(dev, GFP_KERNEL, "%s.%d",
> - dev_name(dev), cs);
> - if (!mtd->name) {
> - ret = -ENOMEM;
> - goto err;
> - }
> -
> - ret = spi_nor_scan(nor, NULL, &hwcaps);
> - if (ret)
> - goto err;
> -
> - ret = mtd_device_register(mtd, NULL, 0);
> - if (ret)
> - goto err;
> -
> - f_pdata->registered = true;
> -
> - if (mtd->size <= cqspi->ahb_size &&
> - !(ddata->quirks & CQSPI_DISABLE_DAC_MODE)) {
> - f_pdata->use_direct_mode = true;
> - dev_dbg(nor->dev, "using direct mode for %s\n",
> - mtd->name);
> -
> - if (!cqspi->rx_chan)
> - cqspi_request_mmap_dma(cqspi);
> - }
> + return cqspi_of_get_flash_pdata(pdev, f_pdata, np);

drop the return. you now parse just the first discovered flash, whereas before
your changes we parsed all the described flashes.

> }
>
> return 0;
> -
> -err:
> - for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
> - if (cqspi->f_pdata[i].registered)
> - mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
> - return ret;
> }
>
> static int cqspi_probe(struct platform_device *pdev)
> {
> - struct device_node *np = pdev->dev.of_node;
> + const struct cqspi_driver_platdata *ddata;
> + struct reset_control *rstc, *rstc_ocp;
> struct device *dev = &pdev->dev;
> + struct spi_master *master;
> + struct resource *res_ahb;
> struct cqspi_st *cqspi;
> struct resource *res;
> - struct resource *res_ahb;
> - struct reset_control *rstc, *rstc_ocp;
> - const struct cqspi_driver_platdata *ddata;
> int ret;
> int irq;
>
> - cqspi = devm_kzalloc(dev, sizeof(*cqspi), GFP_KERNEL);
> - if (!cqspi)
> + master = spi_alloc_master(&pdev->dev, sizeof(*cqspi));
> + if (!master) {
> + dev_err(&pdev->dev, "spi_alloc_master failed\n");
> return -ENOMEM;
> + }
> + master->mode_bits = SPI_RX_QUAD | SPI_RX_DUAL;
> + master->mem_ops = &cqspi_mem_ops;
> + master->dev.of_node = pdev->dev.of_node;
> +
> + cqspi = spi_master_get_devdata(master);
>
> - mutex_init(&cqspi->bus_mutex);
> cqspi->pdev = pdev;
> - platform_set_drvdata(pdev, cqspi);
>
> /* Obtain configuration from OF. */
> - ret = cqspi_of_get_pdata(pdev);
> + ret = cqspi_of_get_pdata(cqspi);
> if (ret) {

spi_controller_put() to not leak memory.

Cheers,
ta

Next message: Jens Axboe: "Re: [PATCH v3 0/2] CQ-seq only based timeouts"
Previous message: Toralf FÃrster: "Re: fatal: unable to access 'https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/': SSL certificate problem: certificate has expired"
In reply to: Vignesh Raghavendra: "[PATCH v2 5/6] mtd: spi-nor: Convert cadence-quadspi to use spi-mem framework"
Next in thread: Vignesh Raghavendra: "[PATCH v2 6/6] spi: Move cadence-quadspi driver to drivers/spi/"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]