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

[Vignesh Raghavendra <vigneshr@xxxxxx>]

On 30/05/20 7:20 pm, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> 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

I will just keep "use_direct_mode" as is to minimize churn

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

I did this intentionaly in view of 2 byte opcodes being added for Octal
DDR mode

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

Same here...

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

Yes, will fix..

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


Will do.. Was just using the same logic as previous driver.

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

OK...

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

Ok..

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

Ok..

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

Right, this is a bug, will fix in v3... This should only return on error:
	int ret;

	ret = cqspi_of_get_flash_pdata(pdev, f_pdata, np);
	if (ret)
		return ret;

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

Ah, yes!

Thanks for the review!

Regards
Vignesh