| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3360641.Vn3sISamPi@192.168.0.120>
Date: Thu, 19 Mar 2020 08:09:48 +0000
From: <Tudor.Ambarus@...rochip.com>
To: <linux-mtd@...ts.infradead.org>,
<vadivel.muruganx.ramuthevar@...ux.intel.com>
CC: <linux-kernel@...r.kernel.org>, <linux-spi@...r.kernel.org>,
<broonie@...nel.org>, <vigneshr@...com>, <robh+dt@...nel.org>,
<marex@...x.de>, <devicetree@...r.kernel.org>,
<tien.fong.chee@...el.com>, <tudor.ambarus@...il.com>,
<boris.brezillon@...e-electrons.com>, <richard@....at>,
<qi-ming.wu@...el.com>, <simon.k.r.goldschmidt@...il.com>,
<dinguyen@...nel.org>, <miquel.raynal@...tlin.com>,
<cheol.yong.kim@...el.com>, <cyrille.pitchen@...el.com>,
<computersforpeace@...il.com>, <dwmw2@...radead.org>,
<david.oberhollenzer@...ma-star.at>
Subject: Re: [PATCH v12 2/4] mtd: spi-nor: add spi-mem support in
cadence-quadspi controller driver
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@...ux.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@...ux.intel.com> Signed-off-by: Vignesh
> Raghavendra <vigneshr@...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
Powered by blists - more mailing lists