Re: [PATCH 2/2] mtd: atmel-quadspi: add driver for Atmel QSPI controller

[Brian Norris <computersforpeace@gmail.com>]

Hi,

On Mon, Jun 13, 2016 at 05:10:26PM +0200, Cyrille Pitchen wrote:
> This driver add support to the new Atmel QSPI controller embedded into
> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
> controller.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  drivers/mtd/spi-nor/Kconfig         |   9 +
>  drivers/mtd/spi-nor/Makefile        |   1 +
>  drivers/mtd/spi-nor/atmel-quadspi.c | 741 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 751 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/atmel-quadspi.c
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index d42c98e1f581..c546efd1357b 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -38,6 +38,15 @@ config SPI_FSL_QUADSPI
>  	  This controller does not support generic SPI. It only supports
>  	  SPI NOR.
>  
> +config SPI_ATMEL_QUADSPI
> +	tristate "Atmel Quad SPI Controller"
> +	depends on ARCH_AT91 || (ARM && COMPILE_TEST)
> +	depends on OF && HAS_IOMEM
> +	help
> +	  This enables support for the Quad SPI controller in master mode.
> +	  This driver does not support generic SPI. The implementation only
> +	  supports SPI NOR.
> +
>  config SPI_NXP_SPIFI
>  	tristate "NXP SPI Flash Interface (SPIFI)"
>  	depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 0bf3a7f81675..1525913698ad 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>  obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
>  obj-$(CONFIG_MTD_MT81xx_NOR)    += mtk-quadspi.o
>  obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
> +obj-$(CONFIG_SPI_ATMEL_QUADSPI)	+= atmel-quadspi.o
> diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c
> new file mode 100644
> index 000000000000..06d1bf276dd0
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c
> @@ -0,0 +1,741 @@

[...]

> +struct atmel_qspi_command {
> +	union {
> +		struct {
> +			u32	instruction:1;
> +			u32	address:3;
> +			u32	mode:1;
> +			u32	dummy:1;
> +			u32	data:1;
> +			u32	reserved:25;
> +		}		bits;

Are you sure this bitfield is going to do what you want, all the time?
What about on big-endian architectures? And are you guaranteed it'll
pack properly, with no padding? IIRC, the answer to the first 2 is "no",
and I have no idea about the 3rd. Honestly, I've been scared away from
using bitfields on anything where the ordering mattered, and I thought
that's because I was hurt by the lack of guarantee once. But I easily
could be misguided.

> +		u32	word;
> +	}	enable;
> +	u8	instruction;
> +	u8	mode;
> +	u8	num_mode_cycles;
> +	u8	num_dummy_cycles;
> +	u32	address;
> +
> +	size_t		buf_len;
> +	const void	*tx_buf;
> +	void		*rx_buf;
> +};
> +

[...]

> +static int atmel_qspi_probe(struct platform_device *pdev)
> +{
> +	struct device_node *child, *np = pdev->dev.of_node;
> +	char modalias[SPI_NAME_SIZE];
> +	const char *name = NULL;
> +	struct atmel_qspi *aq;
> +	struct resource *res;
> +	struct spi_nor *nor;
> +	struct mtd_info *mtd;
> +	int irq, err = 0;
> +
> +	if (of_get_child_count(np) != 1)
> +		return -ENODEV;
> +	child = of_get_next_child(np, NULL);
> +
> +	aq = devm_kzalloc(&pdev->dev, sizeof(*aq), GFP_KERNEL);
> +	if (!aq) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	platform_set_drvdata(pdev, aq);
> +	init_completion(&aq->cmd_completion);
> +	aq->pdev = pdev;
> +
> +	/* Map the registers */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base");
> +	aq->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(aq->regs)) {
> +		dev_err(&pdev->dev, "missing registers\n");
> +		err = PTR_ERR(aq->regs);
> +		goto exit;
> +	}
> +
> +	/* Map the AHB memory */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap");
> +	aq->mem = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(aq->mem)) {
> +		dev_err(&pdev->dev, "missing AHB memory\n");
> +		err = PTR_ERR(aq->mem);
> +		goto exit;
> +	}
> +
> +	/* Get the peripheral clock */
> +	aq->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(aq->clk)) {
> +		dev_err(&pdev->dev, "missing peripheral clock\n");
> +		err = PTR_ERR(aq->clk);
> +		goto exit;
> +	}
> +
> +	/* Enable the peripheral clock */
> +	err = clk_prepare_enable(aq->clk);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to enable the peripheral clock\n");
> +		goto exit;
> +	}
> +
> +	/* Request the IRQ */
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "missing IRQ\n");
> +		err = irq;
> +		goto disable_clk;
> +	}
> +	err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt,
> +			       0, dev_name(&pdev->dev), aq);
> +	if (err)
> +		goto disable_clk;
> +
> +	/* Setup the spi-nor */
> +	nor = &aq->nor;
> +	mtd = &nor->mtd;
> +
> +	nor->dev = &pdev->dev;
> +	spi_nor_set_flash_node(nor, child);
> +	nor->priv = aq;
> +	mtd->priv = nor;
> +
> +	nor->read_reg = atmel_qspi_read_reg;
> +	nor->write_reg = atmel_qspi_write_reg;
> +	nor->read = atmel_qspi_read;
> +	nor->write = atmel_qspi_write;
> +	nor->erase = atmel_qspi_erase;
> +
> +	err = of_modalias_node(child, modalias, sizeof(modalias));
> +	if (err < 0)
> +		goto disable_clk;
> +
> +	if (strcmp(modalias, "spi-nor"))
> +		name = modalias;

What is this modalias handling for? Are you trying to support passing in
a flash-specific string, like m25p80 does? And you're enforcing that to
be only the first entry in the 'compatible' list? Seems fragile; m25p80
is a special case (and it's already fragile) that I'd rather not
imitate.

I understand that we discussed extending the use of compatible for
spi-nor drivers, but I still don't think that issue is resolved. And if
we are going to extend the use of compatible, then I think we should
fixup all the spi-nor drivers in the same way.

> +
> +	err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate);
> +	if (err < 0)
> +		goto disable_clk;
> +
> +	err = atmel_qspi_init(aq);
> +	if (err)
> +		goto disable_clk;
> +
> +	err = spi_nor_scan(nor, name, SPI_NOR_QUAD);
> +	if (err)
> +		goto disable_clk;
> +
> +	err = mtd_device_register(mtd, NULL, 0);
> +	if (err)
> +		goto disable_clk;
> +
> +	of_node_put(child);
> +
> +	return 0;
> +
> +disable_clk:
> +	clk_disable_unprepare(aq->clk);
> +exit:
> +	of_node_put(child);
> +
> +	return err;
> +}
> +
> +static int atmel_qspi_remove(struct platform_device *pdev)
> +{
> +	struct atmel_qspi *aq = platform_get_drvdata(pdev);
> +
> +	mtd_device_unregister(&aq->nor.mtd);
> +	qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS);
> +	clk_disable_unprepare(aq->clk);
> +	return 0;
> +}
> +
> +
> +static const struct of_device_id atmel_qspi_dt_ids[] = {
> +	{ .compatible = "atmel,sama5d2-qspi" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, atmel_qspi_dt_ids);
> +
> +static struct platform_driver atmel_qspi_driver = {
> +	.driver = {
> +		.name	= "atmel_qspi",
> +		.of_match_table	= atmel_qspi_dt_ids,
> +	},
> +	.probe		= atmel_qspi_probe,
> +	.remove		= atmel_qspi_remove,
> +};
> +module_platform_driver(atmel_qspi_driver);
> +
> +MODULE_AUTHOR("Cyrille Pitchen <cyrille.pitchen@atmel.com>");
> +MODULE_DESCRIPTION("Atmel QSPI Controller driver");
> +MODULE_LICENSE("GPL v2");

I'm likely to apply this driver, with the following diff (the bitfield
issue (if there is one) is less of a maintenance issue), barring any
major objections.

diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c
index 06d1bf276dd0..47937d9beec6 100644
--- a/drivers/mtd/spi-nor/atmel-quadspi.c
+++ b/drivers/mtd/spi-nor/atmel-quadspi.c
@@ -591,8 +591,6 @@ static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id)
 static int atmel_qspi_probe(struct platform_device *pdev)
 {
 	struct device_node *child, *np = pdev->dev.of_node;
-	char modalias[SPI_NAME_SIZE];
-	const char *name = NULL;
 	struct atmel_qspi *aq;
 	struct resource *res;
 	struct spi_nor *nor;
@@ -673,13 +671,6 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 	nor->write = atmel_qspi_write;
 	nor->erase = atmel_qspi_erase;
 
-	err = of_modalias_node(child, modalias, sizeof(modalias));
-	if (err < 0)
-		goto disable_clk;
-
-	if (strcmp(modalias, "spi-nor"))
-		name = modalias;
-
 	err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate);
 	if (err < 0)
 		goto disable_clk;
@@ -688,7 +679,7 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 	if (err)
 		goto disable_clk;
 
-	err = spi_nor_scan(nor, name, SPI_NOR_QUAD);
+	err = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
 	if (err)
 		goto disable_clk;