Re: [RFC PATCH] mtd: nand: Add OX820 NAND Support

[Neil Armstrong <narmstrong@baylibre.com>]

Hi Boris,

On 10/18/2016 10:17 PM, Boris Brezillon wrote:
> Hi Neil,
> 
> On Tue, 18 Oct 2016 11:09:27 +0200
> Neil Armstrong <narmstrong@baylibre.com> wrote:
> 
>> Add NAND driver to support the Oxford Semiconductor OX820 NAND Controller.
>> This is a simple memory mapped NAND controller with single chip select and
>> software ECC.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  .../devicetree/bindings/mtd/oxnas-nand.txt         |  24 ++++
>>  drivers/mtd/nand/Kconfig                           |   5 +
>>  drivers/mtd/nand/Makefile                          |   1 +
>>  drivers/mtd/nand/oxnas_nand.c                      | 144 +++++++++++++++++++++
>>  4 files changed, 174 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mtd/oxnas-nand.txt
>>  create mode 100644 drivers/mtd/nand/oxnas_nand.c
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/oxnas-nand.txt b/Documentation/devicetree/bindings/mtd/oxnas-nand.txt
>> new file mode 100644
>> index 0000000..83b684d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/oxnas-nand.txt
>> @@ -0,0 +1,24 @@
>> +* Oxford Semiconductor OXNAS NAND Controller
>> +
>> +Please refer to nand.txt for generic information regarding MTD NAND bindings.
>> +
>> +Required properties:
>> + - compatible: "oxsemi,ox820-nand"
>> + - reg: Base address and length for NAND mapped memory.
>> +
>> +Optional Properties:
>> + - clocks: phandle to the NAND gate clock if needed.
>> + - resets: phandle to the NAND reset control if needed.
>> +
>> +Example:
>> +
>> +nand: nand@41000000 {
>> +	compatible = "oxsemi,ox820-nand";
>> +	reg = <0x41000000 0x100000>;
>> +	nand-ecc-mode = "soft";
>> +	clocks = <&stdclk CLK_820_NAND>;
>> +	resets = <&reset RESET_NAND>;
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +	status = "disabled";
>> +};
> 
> Can you switch to new DT representation for NAND controllers, with one
> node for the NAND controller and NAND devices connected to this NAND
> controller defined as sub-nodes of this NAND controller [1]?

Yes, I was wondering if this existed... my bad, next time I will search further.

> 
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index 7b7a887..c023125 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -426,6 +426,11 @@ config MTD_NAND_ORION
>>  	  No board specific support is done by this driver, each board
>>  	  must advertise a platform_device for the driver to attach.
>>  
>> +config MTD_NAND_OXNAS
>> +	tristate "NAND Flash support for Oxford Semiconductor SoC"
>> +	help
>> +	  This enables the NAND flash controller on Oxford Semiconductor SoCs.
>> +
>>  config MTD_NAND_FSL_ELBC
>>  	tristate "NAND support for Freescale eLBC controllers"
>>  	depends on FSL_SOC
>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> index cafde6f..05fc054 100644
>> --- a/drivers/mtd/nand/Makefile
>> +++ b/drivers/mtd/nand/Makefile
>> @@ -35,6 +35,7 @@ obj-$(CONFIG_MTD_NAND_TMIO)		+= tmio_nand.o
>>  obj-$(CONFIG_MTD_NAND_PLATFORM)		+= plat_nand.o
>>  obj-$(CONFIG_MTD_NAND_PASEMI)		+= pasemi_nand.o
>>  obj-$(CONFIG_MTD_NAND_ORION)		+= orion_nand.o
>> +obj-$(CONFIG_MTD_NAND_OXNAS)		+= oxnas_nand.o
>>  obj-$(CONFIG_MTD_NAND_FSL_ELBC)		+= fsl_elbc_nand.o
>>  obj-$(CONFIG_MTD_NAND_FSL_IFC)		+= fsl_ifc_nand.o
>>  obj-$(CONFIG_MTD_NAND_FSL_UPM)		+= fsl_upm.o
>> diff --git a/drivers/mtd/nand/oxnas_nand.c b/drivers/mtd/nand/oxnas_nand.c
>> new file mode 100644
>> index 0000000..ee402ab
>> --- /dev/null
>> +++ b/drivers/mtd/nand/oxnas_nand.c
>> @@ -0,0 +1,144 @@
>> +/*
>> + * Oxford Semiconductor OXNAS NAND driver
>> + *
>> + * Heavily based on plat_nand.c :
>> + * Author: Vitaly Wool <vitalywool@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/reset.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/nand.h>
>> +#include <linux/mtd/partitions.h>
>> +
>> +/* nand commands */
>> +#define NAND_CMD_ALE		BIT(18)
>> +#define NAND_CMD_CLE		BIT(19)
>> +#define NAND_CMD_CS		0
> 
> I guess this is zero here because you only support connecting a NAND to
> CS0.
> It's probably something like
> 
> OX820_NAND_CS(x)		((x) << CS_FIELD_SHIFT)

The hardware seems to be able to drive multiple chip selects, but no implementation does this.
We will stick to CS0 only here for test reasons.

> 
>> +#define NAND_CMD_RESET		0xff
> 
> Why do you have to redefine it? it's already defined here [2].
> 
>> +#define NAND_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
>> +#define NAND_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)
>> +#define NAND_DATA		(NAND_CMD_CS)
> 
> Please prefix those macros with OX820, has stated above, this can
> conflict with generic definitions.
> Also, I'm not sure you should pass the CS information here.
> 
>> +
>> +struct oxnas_nand_data {
>> +	struct nand_chip	chip;
>> +	void __iomem		*io_base;
>> +	struct clk		*clk;
>> +};
> 
> Even if your driver does not seem to support connecting several chips
> to the same controller, I'd like you to clearly separate the NAND
> controller and NAND chip objects:
> 
> #define OXNAS_NAND_MAX_CHIPS	1
> 
> struct oxnas_nand_controller {
> 	struct nand_hw_control base;
> 	void __iomem *io_base;
> 	struct clk *clk;
> 	struct nand_chip *chips[OXNAS_NAND_MAX_CHIPS];
> }

Ok.

> 
>> +
>> +static void oxnas_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
>> +				unsigned int ctrl)
>> +{
>> +	struct nand_chip *this = mtd->priv;
>> +	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
> 
> Please use the ->io_base field directly, and do not use ->IO_ADDR_W/R
> (I'd like to get rid of them at some point).

Yes, it seems cleaner.

> 
> Also, declare the nandaddr as a void __iomem *, and use the '+'
> operator instead of '|'.
> 
>> +
>> +	if (ctrl & NAND_CTRL_CHANGE) {
>> +		nandaddr &= ~(NAND_CMD | NAND_ADDR);
> 
> This is not needed.
> 
>> +		if (ctrl & NAND_CLE)
>> +			nandaddr |= NAND_CMD;
>> +		else if (ctrl & NAND_ALE)
>> +			nandaddr |= NAND_ADDR;
>> +		this->IO_ADDR_W = (void __iomem *) nandaddr;
> 
> This is not needed, is it?

No more if we stick to ->io_base

> 
>> +	}
>> +
>> +	if (cmd != NAND_CMD_NONE)
>> +		writeb(cmd, (void __iomem *) nandaddr);
>> +}
>> +
>> +/*
>> + * Probe for the NAND device.
>> + */
>> +static int oxnas_nand_probe(struct platform_device *pdev)
>> +{
>> +	struct oxnas_nand_data *data;
>> +	struct mtd_info *mtd;
>> +	struct resource *res;
>> +	int err = 0;
>> +
>> +	/* Allocate memory for the device structure (and zero it) */
>> +	data = devm_kzalloc(&pdev->dev, sizeof(struct oxnas_nand_data),
>> +			    GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	data->io_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(data->io_base))
>> +		return PTR_ERR(data->io_base);
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(data->clk))
>> +		data->clk = NULL;
>> +
>> +	nand_set_flash_node(&data->chip, pdev->dev.of_node);
>> +	mtd = nand_to_mtd(&data->chip);
>> +	mtd->dev.parent = &pdev->dev;
>> +	mtd->priv = &data->chip;
>> +
>> +	data->chip.IO_ADDR_R = data->io_base;
>> +	data->chip.IO_ADDR_W = data->io_base;
> 
> As I said above, don't use these fields.
> 
>> +	data->chip.cmd_ctrl = oxnas_nand_cmd_ctrl;
>> +	data->chip.chip_delay = 30;
>> +	data->chip.ecc.mode = NAND_ECC_SOFT;
>> +	data->chip.ecc.algo = NAND_ECC_HAMMING;
> 
> Probably a good idea to support soft ECC as well...

This was taken from plat_nand.c, I was not sure if it was necessary, will remove this and rely on DT attributes.

> 
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	clk_prepare_enable(data->clk);
>> +	device_reset_optional(&pdev->dev);
>> +
>> +	/* Scan to find existence of the device */
>> +	if (nand_scan(mtd, 1)) {
> 
> Why not returning nand_scan() error code?

Same, bad plat_nand.c copy/paste.

> 
>> +		err = -ENXIO;
>> +		goto out;
> 
> Return directly here.
> 
>> +	}
>> +
>> +	err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
> 
> Use mtd_device_register() here.
> 
>> +	if (!err)
>> +		return err;
>> +
>> +	nand_release(mtd);
> 
> Why not
> 
> 	if (err)
> 		nand_release(mtd);
> 
>> +out:
> 
> Drop this label.

Will refactor this error management.

> 
>> +	return err;
>> +}
>> +
>> +static int oxnas_nand_remove(struct platform_device *pdev)
>> +{
>> +	struct oxnas_nand_data *data = platform_get_drvdata(pdev);
>> +
>> +	nand_release(nand_to_mtd(&data->chip));
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id oxnas_nand_match[] = {
>> +	{ .compatible = "oxsemi,ox820-nand" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, oxnas_nand_match);
>> +
>> +static struct platform_driver oxnas_nand_driver = {
>> +	.probe	= oxnas_nand_probe,
>> +	.remove	= oxnas_nand_remove,
>> +	.driver	= {
>> +		.name		= "oxnas_nand",
>> +		.of_match_table = oxnas_nand_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(oxnas_nand_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Vitaly Wool");
>> +MODULE_DESCRIPTION("Oxnas NAND driver");
>> +MODULE_ALIAS("platform:oxnas_nand");
> 
> Thanks,
> 
> Boris

Thanks,
Neil

> 
> [1]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/nand.txt?id=refs/tags/v4.9-rc1#n57
> [2]http://lxr.free-electrons.com/source/include/linux/mtd/nand.h#L90
>