Re: [PATCH v12 2/4] mtd: spi-nor: add spi-mem support in cadence-quadspi controller driver

[<Tudor.Ambarus@microchip.com>]

Hi,

On Tuesday, March 10, 2020 3:52:11 AM EET Ramuthevar, Vadivel MuruganX wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> From: Ramuthevar Vadivel Murugan
> <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> This patch adds a spi-mem framework adaptation over cadence-quadspi driver.

you need to specify on which versions of the controller you tested this.

> 
> Signed-off-by: Ramuthevar Vadivel Murugan
> <vadivel.muruganx.ramuthevar@linux.intel.com> Signed-off-by: Vignesh
> Raghavendra <vigneshr@ti.com>
> ---
>  drivers/mtd/spi-nor/cadence-quadspi.c | 538
> +++++++++++++--------------------- 1 file changed, 209 insertions(+), 329
> deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c
> b/drivers/mtd/spi-nor/cadence-quadspi.c index 494dcab4aaaa..7b52e109036e
> 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -3,6 +3,8 @@

cut

>  struct cqspi_st {
> @@ -70,23 +66,20 @@ struct cqspi_st {
>         void __iomem            *ahb_base;
>         resource_size_t         ahb_size;
>         struct completion       transfer_complete;
> -       struct mutex            bus_mutex;

are we now supporting just a single flash on the bus? Does 
CQSPI_MAX_CHIPSELECT make sense anymore?

> 
>         struct dma_chan         *rx_chan;
>         struct completion       rx_dma_complete;
>         dma_addr_t              mmap_phys_base;
> 
>         int                     current_cs;
> -       int                     current_page_size;
> -       int                     current_erase_size;
> -       int                     current_addr_width;
> -       unsigned long           master_ref_clk_hz;
>         bool                    is_decoded_cs;
> +       unsigned long           master_ref_clk_hz;

don't do changes for free, keep it were it was.

>         u32                     fifo_depth;
>         u32                     fifo_width;
>         bool                    rclk_en;
>         u32                     trigger_address;
>         u32                     wr_delay;
> +       bool                    use_dac_mode;
>         struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
>  };

cut

> -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)
> +               reg |= (dummy_clk & CQSPI_REG_RD_INSTR_DUMMY_MASK)
> +                      << CQSPI_REG_RD_INSTR_DUMMY_LSB;

nit: we usually keep the operator on the first line

> 
>         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;
> @@ -528,13 +508,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;

nit: unrelated change. You can fix all the checkpatch warnings in the driver 
at the beginning of the series in one dedicated patch, if you care of course, 
but don't do it here.

cut

> -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);
> -
> -       cqspi->is_decoded_cs = of_property_read_bool(np,
> "cdns,is-decoded-cs"); +       struct device *dev = &cqspi->pdev->dev;

you dropped the reading of this property, but you kept the is_decoded_cs 
member, shouldn't you drop the latter too? I guess this deserves a dedicated 
patch.

cut

> 
> -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;
> 
> @@ -1211,131 +1126,82 @@ 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;

why do you print this just on defer?

> +
> +               return ret;

not initializing completion on errors needs a dedicated patch

>         }
>         init_completion(&cqspi->rx_dma_complete);
> +
> +       return 0;
>  }

cut

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

don't forget to free the master on following errors

> +       master->mode_bits = SPI_RX_QUAD | SPI_TX_DUAL | 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) {
>                 dev_err(dev, "Cannot get mandatory OF data.\n");
>                 return -ENODEV;
> @@ -1390,13 +1256,13 @@ static int cqspi_probe(struct platform_device *pdev)
> rstc = devm_reset_control_get_optional_exclusive(dev, "qspi"); if
> (IS_ERR(rstc)) {
>                 dev_err(dev, "Cannot get QSPI reset.\n");
> -               return PTR_ERR(rstc);
> +               goto probe_reset_failed;
>         }
> 
>         rstc_ocp = devm_reset_control_get_optional_exclusive(dev,
> "qspi-ocp"); if (IS_ERR(rstc_ocp)) {
>                 dev_err(dev, "Cannot get QSPI OCP reset.\n");
> -               return PTR_ERR(rstc_ocp);
> +               goto probe_reset_failed;

these 2 goto statements need a dedicated patch.

>         }
> 
>         reset_control_assert(rstc);
> @@ -1407,15 +1273,21 @@ static int cqspi_probe(struct platform_device *pdev)
> 
>         cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
>         ddata  = of_device_get_match_data(dev);
> -       if (ddata && (ddata->quirks & CQSPI_NEEDS_WR_DELAY))
> -               cqspi->wr_delay = 5 * DIV_ROUND_UP(NSEC_PER_SEC,
> -                                                 
> cqspi->master_ref_clk_hz); +       if (ddata) {
> +               if (ddata->quirks & CQSPI_NEEDS_WR_DELAY)
> +                       cqspi->wr_delay = 5 * DIV_ROUND_UP(NSEC_PER_SEC,
> +                                               cqspi->master_ref_clk_hz);
> +               if (ddata->hwcaps_mask & CQSPI_SUPPORTS_OCTAL)
> +                       master->mode_bits |= SPI_RX_OCTAL;
> +               if (!(ddata->quirks & CQSPI_DISABLE_DAC_MODE))
> +                       cqspi->use_dac_mode = true;
> +       }
> 
>         ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
>                                pdev->name, cqspi);
>         if (ret) {
>                 dev_err(dev, "Cannot request IRQ.\n");
> -               goto probe_irq_failed;
> +               goto probe_reset_failed;
>         }
> 
>         cqspi_wait_idle(cqspi);
> @@ -1423,16 +1295,28 @@ static int cqspi_probe(struct platform_device *pdev)
> cqspi->current_cs = -1;
>         cqspi->sclk = 0;
> 
> -       ret = cqspi_setup_flash(cqspi, np);
> +       ret = cqspi_setup_flash(cqspi);
>         if (ret) {
> -               dev_err(dev, "Cadence QSPI NOR probe failed %d\n", ret);
> +               dev_err(dev, "failed to setup flash parameters %d\n", ret);
>                 goto probe_setup_failed;
>         }
> 
> -       return ret;
> +       if (cqspi->use_dac_mode) {
> +               ret = cqspi_request_mmap_dma(cqspi);

the driver was requesting the mmap for each available flash and now you do it 
once, which is great, but this too has to be made in a dedicated patch.

> +               if (ret == -EPROBE_DEFER)
> +                       goto probe_setup_failed;
> +       }
> +
> +       ret = devm_spi_register_master(dev, master);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to register SPI ctlr %d\n",
> ret); +               goto probe_setup_failed;
> +       }
> +
> +       return 0;
>  probe_setup_failed:
>         cqspi_controller_enable(cqspi, 0);
> -probe_irq_failed:
> +probe_reset_failed:
>         clk_disable_unprepare(cqspi->clk);
>  probe_clk_failed:
>         pm_runtime_put_sync(dev);
> @@ -1443,11 +1327,6 @@ static int cqspi_probe(struct platform_device *pdev)
>  static int cqspi_remove(struct platform_device *pdev)
>  {
>         struct cqspi_st *cqspi = platform_get_drvdata(pdev);
> -       int i;
> -
> -       for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
> -               if (cqspi->f_pdata[i].registered)
> -                       mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
> 
>         cqspi_controller_enable(cqspi, 0);
> 
> @@ -1490,16 +1369,15 @@ static const struct dev_pm_ops cqspi__dev_pm_ops = {
> #endif
> 
>  static const struct cqspi_driver_platdata cdns_qspi = {
> -       .hwcaps_mask = CQSPI_BASE_HWCAPS_MASK,
> +       .quirks = CQSPI_DISABLE_DAC_MODE,

The logic around CQSPI_DISABLE_DAC_MODE needs a dedicated patch.

Cheers,
ta