diff mbox

[RFC] mtd: nand: Add OX820 NAND Support

Message ID 20161018090927.1990-1-narmstrong@baylibre.com
State Superseded
Headers show

Commit Message

Neil Armstrong Oct. 18, 2016, 9:09 a.m. UTC
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

Comments

Daniel Golle Oct. 18, 2016, 11:08 a.m. UTC | #1
Hi Neil,

great to see progress towards supporting OX820!
The NAND driver I hacked up for Kernel 4.1 and 4.4 in OpenWrt/LEDE
looks very similar, see

https://git.lede-project.org/?p=lede/dangole/staging.git;a=blob;f=target/linux/oxnas/files/drivers/mtd/nand/oxnas_nand.c;h=f5a142950e32227fee304de731e619278350a91b;hb=refs/heads/oxnas-nand

To me therefore this looks quite good, just one small question below.

Cheers

Daniel


On Tue, Oct 18, 2016 at 11:09:27AM +0200, Neil Armstrong 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";
> +};
> 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
> +#define NAND_CMD_RESET		0xff
> +#define NAND_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
> +#define NAND_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)
> +#define NAND_DATA		(NAND_CMD_CS)
> +
> +struct oxnas_nand_data {
> +	struct nand_chip	chip;
> +	void __iomem		*io_base;

Why do you redundantly keep io_base in platform data rather than
just using chip.IO_ADDR_R and >chip.IO_ADDR_W?


> +	struct clk		*clk;
> +};
> +
> +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;
> +
> +	if (ctrl & NAND_CTRL_CHANGE) {
> +		nandaddr &= ~(NAND_CMD | NAND_ADDR);
> +		if (ctrl & NAND_CLE)
> +			nandaddr |= NAND_CMD;
> +		else if (ctrl & NAND_ALE)
> +			nandaddr |= NAND_ADDR;
> +		this->IO_ADDR_W = (void __iomem *) nandaddr;
> +	}
> +
> +	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;
> +	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;
> +
> +	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)) {
> +		err = -ENXIO;
> +		goto out;
> +	}
> +
> +	err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
> +	if (!err)
> +		return err;
> +
> +	nand_release(mtd);
> +out:
> +	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");
> -- 
> 2.7.0
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Neil Armstrong Oct. 18, 2016, 11:24 a.m. UTC | #2
On 10/18/2016 01:08 PM, Daniel Golle wrote:
> Hi Neil,
> 
> great to see progress towards supporting OX820!
> The NAND driver I hacked up for Kernel 4.1 and 4.4 in OpenWrt/LEDE
> looks very similar, see
> 
> https://git.lede-project.org/?p=lede/dangole/staging.git;a=blob;f=target/linux/oxnas/files/drivers/mtd/nand/oxnas_nand.c;h=f5a142950e32227fee304de731e619278350a91b;hb=refs/heads/oxnas-nand
> 
> To me therefore this looks quite good, just one small question below.

Hi Daniel,

Yes, I work step by step, I hope to have something booting for 4.10 !

Indeed, they look identical except the part_probes[] stuff, are they necessary ?

My primary source of code was Ma Haiju's tree and OPenWRT's tree, but would some people like to push some of the openwrt driver upstream somehow ?

Thanks,
Neil

> 
> Cheers
> 
> Daniel
> 
> 
> On Tue, Oct 18, 2016 at 11:09:27AM +0200, Neil Armstrong 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/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>

Hmm, I forgot the OpenWRT and Ma Haijun copyrights here...

>> + *
>> + * 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
>> +#define NAND_CMD_RESET		0xff
>> +#define NAND_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
>> +#define NAND_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)
>> +#define NAND_DATA		(NAND_CMD_CS)
>> +
>> +struct oxnas_nand_data {
>> +	struct nand_chip	chip;
>> +	void __iomem		*io_base;
> 
> Why do you redundantly keep io_base in platform data rather than
> just using chip.IO_ADDR_R and >chip.IO_ADDR_W?

For no reason since it's not used in oxnas_nand_cmd_ctrl...
Will remove !

> 
> 
>> +	struct clk		*clk;
>> +};
>> +
>> +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;
>> +
>> +	if (ctrl & NAND_CTRL_CHANGE) {
>> +		nandaddr &= ~(NAND_CMD | NAND_ADDR);
>> +		if (ctrl & NAND_CLE)
>> +			nandaddr |= NAND_CMD;
>> +		else if (ctrl & NAND_ALE)
>> +			nandaddr |= NAND_ADDR;
>> +		this->IO_ADDR_W = (void __iomem *) nandaddr;
>> +	}
>> +
>> +	if (cmd != NAND_CMD_NONE)
>> +		writeb(cmd, (void __iomem *) nandaddr);
>> +}
[...]
Daniel Golle Oct. 18, 2016, 11:51 a.m. UTC | #3
Hi Neil,

On Tue, Oct 18, 2016 at 01:24:22PM +0200, Neil Armstrong wrote:
> On 10/18/2016 01:08 PM, Daniel Golle wrote:
> > Hi Neil,
> > 
> > great to see progress towards supporting OX820!
> > The NAND driver I hacked up for Kernel 4.1 and 4.4 in OpenWrt/LEDE
> > looks very similar, see
> > 
> > https://git.lede-project.org/?p=lede/dangole/staging.git;a=blob;f=target/linux/oxnas/files/drivers/mtd/nand/oxnas_nand.c;h=f5a142950e32227fee304de731e619278350a91b;hb=refs/heads/oxnas-nand
> > 
> > To me therefore this looks quite good, just one small question below.
> 
> Hi Daniel,
> 
> Yes, I work step by step, I hope to have something booting for 4.10 !
> 
> Indeed, they look identical except the part_probes[] stuff, are they necessary ?

Nah, this was dropped around kernel 4.7 from most drivers in favour of
using the default partition probes (ie. cmdline and dt) afaik.

> 
> My primary source of code was Ma Haiju's tree and OPenWRT's tree, but would some people like to push some of the openwrt driver upstream somehow ?

I was planning this somehow, but lack the resources to seriously move
forward. I've mostly been focussing on cleaning up Ma Haiju's code and
improving support for the SATA driver (ie. adding support for both
ports) and re-factoring the stmmac-based Ethernet driver to match the
current kernel driver's style.
However, I'm still using platform-specific includes (hardware.h) and
thus look forward to rebase SATA and Ethernet support on top of your
tree, as that would unluck ox820 support in multi-v6 instead of having
a platform-specific kernel.

Currently there are problems with the NAND driver failing to detect the
chip on non-PCIe platform like the PogoPlug V3 (non-Pro), MitraStar
STG-212 and probably other non-PCIe boards which I didn't check yet.
This magically happened when switching from kernel 4.1 to 4.4, I'm
bisecting this since, but the problem seems to be hidden somewhere
between the lines (ie. probe-order-related or such)...
Did you test your NAND driver on any non-PCIe boards and did it work?


For a more or less complete history of the changes I made since
branching of Ma Haijun's tree, see

https://git.lede-project.org/?p=lede/dangole/staging.git;a=history;f=target/linux/oxnas;hb=refs/heads/oxnas-nand

and for the very early history, prior to the merge into the official
OpenWrt tree, see

http://gitorious.org/openwrt-oxnas/openwrt-oxnas.git


Cheers


Daniel

> 
> Thanks,
> Neil
> 
> > 
> > Cheers
> > 
> > Daniel
> > 
> > 
> > On Tue, Oct 18, 2016 at 11:09:27AM +0200, Neil Armstrong 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/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>
> 
> Hmm, I forgot the OpenWRT and Ma Haijun copyrights here...
> 
> >> + *
> >> + * 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
> >> +#define NAND_CMD_RESET		0xff
> >> +#define NAND_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
> >> +#define NAND_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)
> >> +#define NAND_DATA		(NAND_CMD_CS)
> >> +
> >> +struct oxnas_nand_data {
> >> +	struct nand_chip	chip;
> >> +	void __iomem		*io_base;
> > 
> > Why do you redundantly keep io_base in platform data rather than
> > just using chip.IO_ADDR_R and >chip.IO_ADDR_W?
> 
> For no reason since it's not used in oxnas_nand_cmd_ctrl...
> Will remove !
> 
> > 
> > 
> >> +	struct clk		*clk;
> >> +};
> >> +
> >> +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;
> >> +
> >> +	if (ctrl & NAND_CTRL_CHANGE) {
> >> +		nandaddr &= ~(NAND_CMD | NAND_ADDR);
> >> +		if (ctrl & NAND_CLE)
> >> +			nandaddr |= NAND_CMD;
> >> +		else if (ctrl & NAND_ALE)
> >> +			nandaddr |= NAND_ADDR;
> >> +		this->IO_ADDR_W = (void __iomem *) nandaddr;
> >> +	}
> >> +
> >> +	if (cmd != NAND_CMD_NONE)
> >> +		writeb(cmd, (void __iomem *) nandaddr);
> >> +}
> [...]
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Neil Armstrong Oct. 18, 2016, 12:08 p.m. UTC | #4
On 10/18/2016 01:51 PM, Daniel Golle wrote:
> Hi Neil,
> 
> On Tue, Oct 18, 2016 at 01:24:22PM +0200, Neil Armstrong wrote:
>> On 10/18/2016 01:08 PM, Daniel Golle wrote:
>>> Hi Neil,
>>>
>>> great to see progress towards supporting OX820!
>>> The NAND driver I hacked up for Kernel 4.1 and 4.4 in OpenWrt/LEDE
>>> looks very similar, see
>>>
>>> https://git.lede-project.org/?p=lede/dangole/staging.git;a=blob;f=target/linux/oxnas/files/drivers/mtd/nand/oxnas_nand.c;h=f5a142950e32227fee304de731e619278350a91b;hb=refs/heads/oxnas-nand
>>>
>>> To me therefore this looks quite good, just one small question below.
>>
>> Hi Daniel,
>>
>> Yes, I work step by step, I hope to have something booting for 4.10 !
>>
>> Indeed, they look identical except the part_probes[] stuff, are they necessary ?
> 
> Nah, this was dropped around kernel 4.7 from most drivers in favour of
> using the default partition probes (ie. cmdline and dt) afaik.
> 
>>
>> My primary source of code was Ma Haiju's tree and OPenWRT's tree, but would some people like to push some of the openwrt driver upstream somehow ?
> 
> I was planning this somehow, but lack the resources to seriously move
> forward. I've mostly been focussing on cleaning up Ma Haiju's code and
> improving support for the SATA driver (ie. adding support for both
> ports) and re-factoring the stmmac-based Ethernet driver to match the
> current kernel driver's style.
> However, I'm still using platform-specific includes (hardware.h) and
> thus look forward to rebase SATA and Ethernet support on top of your
> tree, as that would unluck ox820 support in multi-v6 instead of having
> a platform-specific kernel.

I made a fork of the SATA driver for the OX810SE who needs the external DMA
to work since it lacks the SATA-sgdma as used in OX820.
But it seems having Ethernet support would be easy, I can rework it and post it.

Yes, It would be great to sync with the upstream tree at some point, please tell
me how my work could be useful.

I only own a PogoPlug v3 to support the basic OX820, but I will lack hardware to
support PCIe and the dual SATA setup at some point.

The next work I plan to do once the basic OX820 boot is merged is to push the USB,
and the PLL support in the clock driver. The SATA and PCIe would be a natural following work.

> 
> Currently there are problems with the NAND driver failing to detect the
> chip on non-PCIe platform like the PogoPlug V3 (non-Pro), MitraStar
> STG-212 and probably other non-PCIe boards which I didn't check yet.
> This magically happened when switching from kernel 4.1 to 4.4, I'm
> bisecting this since, but the problem seems to be hidden somewhere
> between the lines (ie. probe-order-related or such)...
> Did you test your NAND driver on any non-PCIe boards and did it work?

Actually I tested only on non-PCIe, since I only own a PogoPlug v3 (non-pro) and the
probe works perfectly on 4.9-rc1 :
[...]
nand: Could not find valid ONFI parameter page; aborting
nand: device found, Manufacturer ID: 0xad, Chip ID: 0xf1
nand: Hynix NAND 128MiB 3,3V 8-bit
nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
Scanning device for bad blocks
Bad eraseblock 511 at 0x000003fe0000
2 ofpart partitions found on MTD device 41000000.nand
Creating 2 MTD partitions on "41000000.nand":
0x000000000000-0x000000e00000 : "boot"
0x000000e00000-0x000008000000 : "ubi"
[...]
# ubiattach -p /dev/mtd1
ubi0: attaching mtd1
ubi0: scanning is finished
ubi0 warning: ubi_eba_init: cannot reserve enough PEBs for bad PEB handling, reserved 9, need 19
ubi0: attached mtd1 (name "ubi", size 114 MiB)
ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes
ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512
ubi0: VID header offset: 512 (aligned 512), data offset: 2048
ubi0: good PEBs: 911, bad PEBs: 1, corrupted PEBs: 0
ubi0: user volume: 1, internal volumes: 1, max. volumes count: 128
ubi0: max/mean erase counter: 2/0, WL threshold: 4096, image sequence number: 0
ubi0: available PEBs: 0, total reserved PEBs: 911, PEBs reserved for bad PEB handling: 9
ubi0: background thread "ubi_bgt0d" started, PID 824
UBI device number 0, total 911 LEBs (117540864 bytes, 112.1 MiB), available 0 LEBs (0 bytes), LEB size 129024 bytes (126.0 KiB)
[...]

> 
> 
> For a more or less complete history of the changes I made since
> branching of Ma Haijun's tree, see
> 
> https://git.lede-project.org/?p=lede/dangole/staging.git;a=history;f=target/linux/oxnas;hb=refs/heads/oxnas-nand
> 
> and for the very early history, prior to the merge into the official
> OpenWrt tree, see
> 
> http://gitorious.org/openwrt-oxnas/openwrt-oxnas.git
> 

Great, thanks for the hints.

I did set up a ML for the upstream work at linux-oxnas@lists.tuxfamily.org and a patchwork interface https://patchwork.kernel.org/project/linux-oxnas/list/ if it can be helpful.

I also idle on #linux-oxnas on freenode...

Neil
Boris Brezillon Oct. 18, 2016, 8:17 p.m. UTC | #5
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]?

> 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)

> +#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];
}

> +
> +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).

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?

> +	}
> +
> +	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...

> +
> +	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?

> +		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.

> +	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

[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
Rob Herring (Arm) Oct. 19, 2016, 3:17 a.m. UTC | #6
On Tue, Oct 18, 2016 at 11:09:27AM +0200, Neil Armstrong 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 ++++

Acked-by: Rob Herring <robh@kernel.org>

>  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
Neil Armstrong Oct. 19, 2016, 9:29 a.m. UTC | #7
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
>
Boris Brezillon Oct. 19, 2016, 11:13 a.m. UTC | #8
On Wed, 19 Oct 2016 11:29:59 +0200
Neil Armstrong <narmstrong@baylibre.com> wrote:

> 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.

No problem. That's what reviews are here for ;-).

[...]
> >> diff --git a/drivers/mtd/nand/oxnas_nand.c b/drivers/mtd/nand/oxnas_nand.c

[...]

> >> +
> >> +/* 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.

Then no need to define this _CS flag.

[...]

> >> +	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...  

I meant BCH, not soft :-).

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

Yes, that's probably the best solution.

Regards,

Boris
diff mbox

Patch

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";
+};
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
+#define NAND_CMD_RESET		0xff
+#define NAND_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
+#define NAND_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)
+#define NAND_DATA		(NAND_CMD_CS)
+
+struct oxnas_nand_data {
+	struct nand_chip	chip;
+	void __iomem		*io_base;
+	struct clk		*clk;
+};
+
+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;
+
+	if (ctrl & NAND_CTRL_CHANGE) {
+		nandaddr &= ~(NAND_CMD | NAND_ADDR);
+		if (ctrl & NAND_CLE)
+			nandaddr |= NAND_CMD;
+		else if (ctrl & NAND_ALE)
+			nandaddr |= NAND_ADDR;
+		this->IO_ADDR_W = (void __iomem *) nandaddr;
+	}
+
+	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;
+	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;
+
+	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)) {
+		err = -ENXIO;
+		goto out;
+	}
+
+	err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
+	if (!err)
+		return err;
+
+	nand_release(mtd);
+out:
+	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");