| Message ID | 20200310015213.1734-1-vadivel.muruganx.ramuthevar@linux.intel.com |
|---|---|
| Headers | show |
| Series | spi: cadence-quadspi: Add support for the Cadence QSPI controller | expand |
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
On 19/03/20 1:39 pm, Tudor.Ambarus@microchip.com wrote: > 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? > Driver still supports multiple CS but SPI core takes care of serialization by holding bus_lock_mutex in spi_mem_access_start() So, I don't see a need for this mutex [...] > > 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. > is_decoded_cs cannot be supported with spi-mem as this requires knowlegde of flash geometry which is not available via spi-mem I don't see any user of decoded-cs in the kernel. So, IMO its okay to drop entire support in a patch prior to converting driver to spi-mem. [...] >> @@ -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. > Not really, current driver does: if (!cqspi->rx_chan) cqspi_request_mmap_dma(cqspi); So, cqspi_request_mmap_dma() is not called again if it succeeds for at least one flash. Regards Vignesh
Add support for the Cadence QSPI controller. This controller is present in the Intel Lightning Mountain(LGM) SoCs, Altera and TI SoCs. This driver does not support generic SPI and also the implementation only supports spi-mem interface to replace the existing driver in mtd/spi-nor/cadence-quadspi.c, which supports SPI-NOR memory. So we have decided that adapt spi-mem framework with existing driver to support SPI-NOR and SPI-NAND flash memories. changes from v11 -- Boris suggested to split the patches -- split 3 patches like below i) spi-mem adaptation ii) convert the existing drivers to spi based. ii) add intel changes to above 2 patches. -- Rob's build issue resolved changes from v10 -- Rob's review comments update in dt_schema yaml. -- add valid contraints constraints. changes from v9: -- Mark's review comments address -- add check to shared interrupt handler -- add check decoder if present -- add error check for quirks capability data -- remove the existing cadence_quadspi.c from drivers/mtd/spi-nor path -- remove CONFIG macro from Kconfig in the above path -- remove cadence_quadspi.o build option from Makefile in the above path changes from v8: -- remove the depends MTD macro -- comment into C++ style -- remove spaces and tabs where not applicable. -- align the macro string as same as existing one. -- replace QUAD to op->data.buswidth variable. -- add CQSPI_NEEDS_ADDR_SWAP instead of soc_selection variable changes from v7: -- remove addr_buf kept like as original -- drop bus-num, chipselect variable -- add soc_selection varible to differetiate the features -- replace dev->ddev in dma function -- add seperate function to handle the 24bit slave device address translation for lgm soc -- correct sentence seems incomplete in Kconfig -- add cqspi->soc_selection check to keep the original TI platform working code. changes from v6: -- Add the Signed-off-by Vignesh in commit message -- bus_num, num_chipselect added to avoid the garbage bus number during the probe and spi_register. -- master mode bits updated -- address sequence is different from TI and Intel SoC Ip handling so modified as per Intel and differentiating by use_dac_mode variable. -- dummy cycles also different b/w two platforms, so keeping separate check -- checkpatch errors which are intentional left as is for better readability changes from v5: -- kbuild test robot warnings fixed -- Add Reported-By: Dan Carpenter <dan.carpenter@oracle.com> changes from v4: -- kbuild test robot warnings fixed -- Add Reborted-by: tag changes from v3: spi-cadence-quadspi.c -- static to all functions wrt to local to the file. -- Prefix cqspi_ and make the function static -- cmd_ops, data_ops and dummy_ops dropped -- addr_ops kept since it is required for address calculation. -- devm_ used for supported functions , removed legacy API's -- removed "indirect" name from functions -- replaced by master->mode_bits = SPI_RX_QUAD | SPI_TX_DUAL | SPI_RX_DUAL | SPI_RX_OCTAL; as per Vignesh susggestion -- removed free functions since devm_ handles automatically. -- dropped all unused Macros YAML file update: -- cadence,qspi.yaml file name replace by cdns,qspi-nor.yaml -- compatible string updated as per Vignesh suggestion -- for single entry, removed descriptions -- removed optional parameters Build Result: linux$ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml dt_binding_check CHKDT Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml SCHEMA Documentation/devicetree/bindings/processed-schema.yaml DTC Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dt.yaml CHECK Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dt.yaml Ramuthevar Vadivel Murugan (2): dt-bindings: spi: Add schema for Cadence QSPI Controller driver spi: cadence-quadspi: Add support for the Cadence QSPI controller .../devicetree/bindings/mtd/cadence-quadspi.txt | 67 --- .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 142 +++++ drivers/mtd/spi-nor/Kconfig | 11 - drivers/mtd/spi-nor/Makefile | 1 - drivers/spi/Kconfig | 10 + drivers/spi/Makefile | 1 + .../spi-cadence-quadspi.c} | 641 ++++++++++----------- 7 files changed, 456 insertions(+), 417 deletions(-) delete mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt create mode 100644 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml rename drivers/{mtd/spi-nor/cadence-quadspi.c => spi/spi-cadence-quadspi.c} (73%) Ramuthevar Vadivel Murugan (4): dt-bindings: spi: Add schema for Cadence QSPI Controller driver mtd: spi-nor: add spi-mem support in cadence-quadspi controller driver spi: cadence-quadspi: Add support for the Cadence QSPI controller spi: cadence-quadspi: Add qspi support for Intel LGM SoC .../devicetree/bindings/mtd/cadence-quadspi.txt | 67 --- .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 127 ++++ drivers/mtd/spi-nor/Kconfig | 11 - drivers/mtd/spi-nor/Makefile | 1 - drivers/spi/Kconfig | 10 + drivers/spi/Makefile | 1 + .../spi-cadence-quadspi.c} | 643 ++++++++++----------- 7 files changed, 442 insertions(+), 418 deletions(-) delete mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt create mode 100644 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml rename drivers/{mtd/spi-nor/cadence-quadspi.c => spi/spi-cadence-quadspi.c} (73%)