[PATCH V12 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller
Marek Vasut
marex at denx.de
Mon Jul 18 02:35:59 PDT 2016
On 07/18/2016 02:52 AM, Brian Norris wrote:
> Hi,
Hi,
> Some trivial comments. I have some proposed diffs, and if y'all are OK
> with them, I might apply this with some fixups.
[...]
>> +static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
>> +{
>> + unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
>> + u32 val;
>> +
>> + while (1) {
>> + val = readl(reg);
>> + if (clear)
>> + val = ~val;
>> + val &= mask;
>> +
>> + if (val == mask)
>> + return 0;
>> +
>> + if (time_after(jiffies, end))
>> + return -ETIMEDOUT;
>> + }
>
> This could all be replaced with readl_poll_timeout(), couldn't it?
Oh nice, I didn't know we had generic implementation. Yes, let's use it.
Minor nit to it inline.
> Like
> the following diff (I won't apply this one now; if anything, maybe I'll
> send a follow-up patch for review):
>
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index d403ba7b8f43..87586baaae9e 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -22,6 +22,7 @@
> #include <linux/errno.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/jiffies.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -226,21 +227,11 @@ struct cqspi_st {
>
> static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
> {
> - unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
> u32 val;
>
> - while (1) {
> - val = readl(reg);
> - if (clear)
> - val = ~val;
> - val &= mask;
> -
> - if (val == mask)
> - return 0;
> -
> - if (time_after(jiffies, end))
> - return -ETIMEDOUT;
> - }
> + return readl_poll_timeout(reg, val,
> + (val & mask) == (clear ? ~mask : mask),
You probably mean "((clear ? ~val : val) & mask) == mask" here.
> + 0, CQSPI_TIMEOUT_MS * 1000);
> }
[...]
>> +static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>> +{
>> + struct platform_device *pdev = cqspi->pdev;
>> + struct device *dev = &pdev->dev;
>> + struct cqspi_flash_pdata *f_pdata;
>> + struct spi_nor *nor;
>> + struct mtd_info *mtd;
>> + unsigned int cs;
>> + int i, ret;
>> +
>> + /* Get flash device data */
>> + for_each_available_child_of_node(dev->of_node, np) {
>> + if (of_property_read_u32(np, "reg", &cs)) {
>> + dev_err(dev, "Couldn't determine chip select.\n");
>> + goto err;
>> + }
>> +
>> + if (cs > CQSPI_MAX_CHIPSELECT) {
>> + dev_err(dev, "Chip select %d out of range.\n", cs);
>> + goto err;
>> + }
>> +
>> + 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->read_reg = cqspi_read_reg;
>> + nor->write_reg = cqspi_write_reg;
>> + nor->read = cqspi_read;
>> + nor->write = cqspi_write;
>> + nor->erase = cqspi_erase;
>> + nor->prepare = cqspi_prep;
>> + nor->unprepare = cqspi_unprep;
>> +
>> + mtd->name = kasprintf(GFP_KERNEL, "%s.%d", dev_name(dev), cs);
>
> Might be easier to just use devm_kasprintf()?
Yes, I think that'd work.
>> + if (!mtd->name) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
>> + if (ret)
>> + goto err;
>> +
>> + ret = mtd_device_register(mtd, NULL, 0);
>> + if (ret)
>> + goto err;
>> + }
>> +
>> + return 0;
>> +
>> +err:
>> + for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
>> + if (cqspi->f_pdata[i].nor.mtd.name) {
>
> Chekcing for 'name' doens't really handle all error paths right. If
> spi_nor_scan() or mtd_device_register() fails, then you'll be trying to
> unregister an unregistered MTD.
Oh, snap. OK.
>> + mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
>> + kfree(cqspi->f_pdata[i].nor.mtd.name);
>> + }
>> + return ret;
>> +}
>> +
>> +static int cqspi_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct device *dev = &pdev->dev;
>> + struct cqspi_st *cqspi;
>> + struct resource *res;
>> + struct resource *res_ahb;
>> + int ret;
>> + int irq;
>> +
>> + cqspi = devm_kzalloc(dev, sizeof(*cqspi), GFP_KERNEL);
>> + if (!cqspi)
>> + return -ENOMEM;
>> +
>> + mutex_init(&cqspi->bus_mutex);
>> + cqspi->pdev = pdev;
>> + platform_set_drvdata(pdev, cqspi);
>> +
>> + /* Obtain configuration from OF. */
>> + ret = cqspi_of_get_pdata(pdev);
>> + if (ret) {
>> + dev_err(dev, "Cannot get mandatory OF data.\n");
>> + return -ENODEV;
>> + }
>> +
>> + /* Obtain QSPI clock. */
>> + cqspi->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(cqspi->clk)) {
>> + dev_err(dev, "Cannot claim QSPI clock.\n");
>> + return PTR_ERR(cqspi->clk);
>> + }
>> +
>> + /* Obtain and remap controller address. */
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + cqspi->iobase = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(cqspi->iobase)) {
>> + dev_err(dev, "Cannot remap controller address.\n");
>> + return PTR_ERR(cqspi->iobase);
>> + }
>> +
>> + /* Obtain and remap AHB address. */
>> + res_ahb = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + cqspi->ahb_base = devm_ioremap_resource(dev, res_ahb);
>> + if (IS_ERR(cqspi->ahb_base)) {
>> + dev_err(dev, "Cannot remap AHB address.\n");
>> + return PTR_ERR(cqspi->ahb_base);
>> + }
>> +
>> + init_completion(&cqspi->transfer_complete);
>> +
>> + /* Obtain IRQ line. */
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + dev_err(dev, "Cannot obtain IRQ.\n");
>> + return -ENXIO;
>> + }
>> +
>> + ret = clk_prepare_enable(cqspi->clk);
>> + if (ret) {
>> + dev_err(dev, "Cannot enable QSPI clock.\n");
>> + return ret;
>> + }
>> +
>> + cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
>> +
>> + 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;
>> + }
>> +
>> + cqspi_wait_idle(cqspi);
>> + cqspi_controller_init(cqspi);
>> + cqspi->current_cs = -1;
>> + cqspi->sclk = 0;
>> +
>> + ret = cqspi_setup_flash(cqspi, np);
>> + if (ret) {
>> + dev_err(dev, "Cadence QSPI NOR probe failed %d\n", ret);
>> + goto probe_setup_failed;
>> + }
>> +
>> + return ret;
>> +probe_irq_failed:
>> + cqspi_controller_enable(cqspi, 0);
>> +probe_setup_failed:
>> + clk_disable_unprepare(cqspi->clk);
>> + return ret;
>> +}
>> +
>> +static int cqspi_remove(struct platform_device *pdev)
>> +{
>> + struct cqspi_st *cqspi = platform_get_drvdata(pdev);
>> + int i;
>> +
>> + cqspi_controller_enable(cqspi, 0);
>
> I think you want to disable the controller *after* unregistering the
> MTDs, no?
That's a bit of a misnomer, the second parameter is "bool enable", so
this disables the controller.
>> +
>> + for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
>> + if (cqspi->f_pdata[i].nor.mtd.name) {
>> + mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
>> + kfree(cqspi->f_pdata[i].nor.mtd.name);
>> + }
>> +
>> + clk_disable_unprepare(cqspi->clk);
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int cqspi_suspend(struct device *dev)
>> +{
>> + struct cqspi_st *cqspi = dev_get_drvdata(dev);
>> +
>> + cqspi_controller_enable(cqspi, 0);
>> + return 0;
>> +}
>> +
>> +static int cqspi_resume(struct device *dev)
>> +{
>> + struct cqspi_st *cqspi = dev_get_drvdata(dev);
>> +
>> + cqspi_controller_enable(cqspi, 1);
>> + return 0;
>> +}
>> +
>> +static const struct dev_pm_ops cqspi__dev_pm_ops = {
>> + .suspend = cqspi_suspend,
>> + .resume = cqspi_resume,
>> +};
>> +
>> +#define CQSPI_DEV_PM_OPS (&cqspi__dev_pm_ops)
>> +#else
>> +#define CQSPI_DEV_PM_OPS NULL
>> +#endif
>> +
>> +static struct of_device_id const cqspi_dt_ids[] = {
>> + {.compatible = "cdns,qspi-nor",},
>> + { /* end of table */ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, cqspi_dt_ids);
>> +
>> +static struct platform_driver cqspi_platform_driver = {
>> + .probe = cqspi_probe,
>> + .remove = cqspi_remove,
>> + .driver = {
>> + .name = CQSPI_NAME,
>> + .pm = CQSPI_DEV_PM_OPS,
>> + .of_match_table = cqspi_dt_ids,
>> + },
>> +};
>> +
>> +module_platform_driver(cqspi_platform_driver);
>> +
>> +MODULE_DESCRIPTION("Cadence QSPI Controller Driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:" CQSPI_NAME);
>> +MODULE_AUTHOR("Ley Foon Tan <lftan at altera.com>");
>> +MODULE_AUTHOR("Graham Moore <grmoore at opensource.altera.com>");
>
> So, I'm thinking of applying this patch with the following diff:
>
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index 2fff31c0b9c3..d403ba7b8f43 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -53,6 +53,7 @@ struct cqspi_flash_pdata {
> u8 addr_width;
> u8 data_width;
> u8 cs;
> + bool registered;
This feels a bit odd, but I don't have a better solution now.
> };
>
> struct cqspi_st {
> @@ -1111,7 +1112,8 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
> nor->prepare = cqspi_prep;
> nor->unprepare = cqspi_unprep;
>
> - mtd->name = kasprintf(GFP_KERNEL, "%s.%d", dev_name(dev), cs);
> + mtd->name = devm_kasprintf(dev, GFP_KERNEL, "%s.%d",
> + dev_name(dev), cs);
> if (!mtd->name) {
> ret = -ENOMEM;
> goto err;
> @@ -1124,16 +1126,16 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
> ret = mtd_device_register(mtd, NULL, 0);
> if (ret)
> goto err;
> +
> + f_pdata->registered = true;
> }
>
> return 0;
>
> err:
> for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
> - if (cqspi->f_pdata[i].nor.mtd.name) {
> + if (cqspi->f_pdata[i].registered)
> mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
> - kfree(cqspi->f_pdata[i].nor.mtd.name);
> - }
> return ret;
> }
>
> @@ -1233,13 +1235,11 @@ static int cqspi_remove(struct platform_device *pdev)
> struct cqspi_st *cqspi = platform_get_drvdata(pdev);
> int i;
>
> - cqspi_controller_enable(cqspi, 0);
> -
> for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
> - if (cqspi->f_pdata[i].nor.mtd.name) {
> + if (cqspi->f_pdata[i].registered)
> mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
> - kfree(cqspi->f_pdata[i].nor.mtd.name);
> - }
> +
> + cqspi_controller_enable(cqspi, 0);
>
> clk_disable_unprepare(cqspi->clk);
>
>
Thanks
--
Best regards,
Marek Vasut
More information about the linux-mtd
mailing list