[PATCH v2 0/8] MTD: xway: fix driver

[PATCH v2 0/8] MTD: xway: fix driver

[Hauke Mehrtens]

Without these patches the driver does not work for me.
Some of these patches are in OpenWrt for years now and should go 
upstream. In addition this converts it from some hack with the 
plat_nand driver to a normal platform driver.

changes since:
v1:
 - convert to normal platform driver
 - do not use global variable xway_latchcmd
 - use mtd_to_nand()

Hauke Mehrtens (4):
  MTD: xway: convert to normal platform driver
  MTD: xway: add some more documentation
  MTD: xway: extract read and write function
  MTD: xway: use global NAND_CMD_RESET define

John Crispin (4):
  MTD: xway: the latched command should be persistent
  MTD: xway: remove endless loop
  MTD: xway: add missing write_buf and read_buf to nand driver
  MTD: xway: fix nand locking

 drivers/mtd/nand/Kconfig     |   1 -
 drivers/mtd/nand/xway_nand.c | 206 ++++++++++++++++++++++++++++++-------------
 2 files changed, 147 insertions(+), 60 deletions(-)

-- 
2.8.1

[PATCH v2 1/8] MTD: xway: convert to normal platform driver

[Hauke Mehrtens]

Instead of hacking this into the plat_nand driver just make this a
normal nand driver.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/nand/Kconfig     |   1 -
 drivers/mtd/nand/xway_nand.c | 119 ++++++++++++++++++++++++++++++-------------
 2 files changed, 84 insertions(+), 36 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f05e0e9..d7de2c97 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -539,7 +539,6 @@ config MTD_NAND_FSMC
 config MTD_NAND_XWAY
 	tristate "Support for NAND on Lantiq XWAY SoC"
 	depends on LANTIQ && SOC_TYPE_XWAY
-	select MTD_NAND_PLATFORM
 	help
 	  Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached
 	  to the External Bus Unit (EBU).
diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index 0cf0ac0..867a636 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -4,6 +4,7 @@
  *  by the Free Software Foundation.
  *
  *  Copyright © 2012 John Crispin <blogic@openwrt.org>
+ *  Copyright © 2016 Hauke Mehrtens <hauke@hauke-m.de>
  */
 
 #include <linux/mtd/nand.h>
@@ -54,6 +55,10 @@
 #define NAND_CON_CSMUX		(1 << 1)
 #define NAND_CON_NANDM		1
 
+struct xway_nand_data {
+	struct nand_chip	chip;
+};
+
 static void xway_reset_chip(struct nand_chip *chip)
 {
 	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
@@ -130,14 +135,50 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
 	return ret;
 }
 
+/*
+ * Probe for the NAND device.
+ */
 static int xway_nand_probe(struct platform_device *pdev)
 {
-	struct nand_chip *this = platform_get_drvdata(pdev);
-	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
-	const __be32 *cs = of_get_property(pdev->dev.of_node,
-					"lantiq,cs", NULL);
+	struct xway_nand_data *data;
+	struct mtd_info *mtd;
+	struct resource *res;
+	int err = 0;
+	void __iomem *nandaddr;
+	const __be32 *cs;
 	u32 cs_flag = 0;
 
+	/* Allocate memory for the device structure (and zero it) */
+	data = devm_kzalloc(&pdev->dev, sizeof(struct xway_nand_data),
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	nandaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nandaddr))
+		return PTR_ERR(nandaddr);
+
+	nand_set_flash_node(&data->chip, pdev->dev.of_node);
+	mtd = nand_to_mtd(&data->chip);
+	mtd->dev.parent = &pdev->dev;
+
+	data->chip.IO_ADDR_R = nandaddr;
+	data->chip.IO_ADDR_W = nandaddr;
+	data->chip.cmd_ctrl = xway_cmd_ctrl;
+	data->chip.dev_ready = xway_dev_ready;
+	data->chip.select_chip = xway_select_chip;
+	data->chip.read_byte = xway_read_byte;
+	data->chip.chip_delay = 30;
+
+	data->chip.ecc.mode = NAND_ECC_SOFT;
+	data->chip.ecc.algo = NAND_ECC_HAMMING;
+
+	platform_set_drvdata(pdev, data);
+	nand_set_controller_data(&data->chip, data);
+
+	cs = of_get_property(pdev->dev.of_node, "lantiq,cs", NULL);
+
 	/* load our CS from the DT. Either we find a valid 1 or default to 0 */
 	if (cs && (*cs == 1))
 		cs_flag = NAND_CON_IN_CS1 | NAND_CON_OUT_CS1;
@@ -155,43 +196,51 @@ static int xway_nand_probe(struct platform_device *pdev)
 		| cs_flag, EBU_NAND_CON);
 
 	/* finish with a reset */
-	xway_reset_chip(this);
-
-	return 0;
-}
+	xway_reset_chip(&data->chip);
 
-static struct platform_nand_data xway_nand_data = {
-	.chip = {
-		.nr_chips		= 1,
-		.chip_delay		= 30,
-	},
-	.ctrl = {
-		.probe		= xway_nand_probe,
-		.cmd_ctrl	= xway_cmd_ctrl,
-		.dev_ready	= xway_dev_ready,
-		.select_chip	= xway_select_chip,
-		.read_byte	= xway_read_byte,
+	/* Scan to find existence of the device */
+	if (nand_scan(mtd, 1)) {
+		err = -ENXIO;
+		goto out;
 	}
-};
+
+	err = mtd_device_register(mtd, NULL, 0);
+
+	if (!err)
+		return 0;
+
+	nand_release(mtd);
+out:
+	return err;
+}
 
 /*
- * Try to find the node inside the DT. If it is available attach out
- * platform_nand_data
+ * Remove a NAND device.
  */
-static int __init xway_register_nand(void)
+static int xway_nand_remove(struct platform_device *pdev)
 {
-	struct device_node *node;
-	struct platform_device *pdev;
-
-	node = of_find_compatible_node(NULL, NULL, "lantiq,nand-xway");
-	if (!node)
-		return -ENOENT;
-	pdev = of_find_device_by_node(node);
-	if (!pdev)
-		return -EINVAL;
-	pdev->dev.platform_data = &xway_nand_data;
-	of_node_put(node);
+	struct xway_nand_data *data = platform_get_drvdata(pdev);
+
+	nand_release(nand_to_mtd(&data->chip));
+
 	return 0;
 }
 
-subsys_initcall(xway_register_nand);
+static const struct of_device_id xway_nand_match[] = {
+	{ .compatible = "lantiq,nand-xway" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xway_nand_match);
+
+static struct platform_driver xway_nand_driver = {
+	.probe	= xway_nand_probe,
+	.remove	= xway_nand_remove,
+	.driver	= {
+		.name		= "lantiq,nand-xway",
+		.of_match_table = xway_nand_match,
+	},
+};
+
+module_platform_driver(xway_nand_driver);
+
+MODULE_LICENSE("GPL");
-- 
2.8.1

Re: [PATCH v2 1/8] MTD: xway: convert to normal platform driver

[Boris Brezillon]

Hi Hauke,

On Sat, 18 Jun 2016 21:14:05 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> Instead of hacking this into the plat_nand driver just make this a
> normal nand driver.

Thanks for taking care of that.

> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/Kconfig     |   1 -
>  drivers/mtd/nand/xway_nand.c | 119 ++++++++++++++++++++++++++++++-------------
>  2 files changed, 84 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index f05e0e9..d7de2c97 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -539,7 +539,6 @@ config MTD_NAND_FSMC
>  config MTD_NAND_XWAY
>  	tristate "Support for NAND on Lantiq XWAY SoC"
>  	depends on LANTIQ && SOC_TYPE_XWAY
> -	select MTD_NAND_PLATFORM
>  	help
>  	  Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached
>  	  to the External Bus Unit (EBU).
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index 0cf0ac0..867a636 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -4,6 +4,7 @@
>   *  by the Free Software Foundation.
>   *
>   *  Copyright © 2012 John Crispin <blogic@openwrt.org>
> + *  Copyright © 2016 Hauke Mehrtens <hauke@hauke-m.de>
>   */
>  
>  #include <linux/mtd/nand.h>
> @@ -54,6 +55,10 @@
>  #define NAND_CON_CSMUX		(1 << 1)
>  #define NAND_CON_NANDM		1
>  
> +struct xway_nand_data {
> +	struct nand_chip	chip;
> +};
> +
>  static void xway_reset_chip(struct nand_chip *chip)
>  {
>  	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
> @@ -130,14 +135,50 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
>  	return ret;
>  }
>  
> +/*
> + * Probe for the NAND device.
> + */
>  static int xway_nand_probe(struct platform_device *pdev)
>  {
> -	struct nand_chip *this = platform_get_drvdata(pdev);
> -	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
> -	const __be32 *cs = of_get_property(pdev->dev.of_node,
> -					"lantiq,cs", NULL);
> +	struct xway_nand_data *data;
> +	struct mtd_info *mtd;
> +	struct resource *res;
> +	int err = 0;
> +	void __iomem *nandaddr;
> +	const __be32 *cs;
>  	u32 cs_flag = 0;
>  
> +	/* Allocate memory for the device structure (and zero it) */
> +	data = devm_kzalloc(&pdev->dev, sizeof(struct xway_nand_data),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	nandaddr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(nandaddr))
> +		return PTR_ERR(nandaddr);
> +
> +	nand_set_flash_node(&data->chip, pdev->dev.of_node);
> +	mtd = nand_to_mtd(&data->chip);
> +	mtd->dev.parent = &pdev->dev;
> +
> +	data->chip.IO_ADDR_R = nandaddr;
> +	data->chip.IO_ADDR_W = nandaddr;
> +	data->chip.cmd_ctrl = xway_cmd_ctrl;
> +	data->chip.dev_ready = xway_dev_ready;
> +	data->chip.select_chip = xway_select_chip;
> +	data->chip.read_byte = xway_read_byte;
> +	data->chip.chip_delay = 30;
> +
> +	data->chip.ecc.mode = NAND_ECC_SOFT;
> +	data->chip.ecc.algo = NAND_ECC_HAMMING;
> +
> +	platform_set_drvdata(pdev, data);
> +	nand_set_controller_data(&data->chip, data);
> +
> +	cs = of_get_property(pdev->dev.of_node, "lantiq,cs", NULL);

It may be a good time to switch to of_property_read_u32() (here or in a
separate patch).

> +
>  	/* load our CS from the DT. Either we find a valid 1 or default to 0 */
>  	if (cs && (*cs == 1))
>  		cs_flag = NAND_CON_IN_CS1 | NAND_CON_OUT_CS1;
> @@ -155,43 +196,51 @@ static int xway_nand_probe(struct platform_device *pdev)
>  		| cs_flag, EBU_NAND_CON);
>  
>  	/* finish with a reset */
> -	xway_reset_chip(this);
> -
> -	return 0;
> -}
> +	xway_reset_chip(&data->chip);
>  
> -static struct platform_nand_data xway_nand_data = {
> -	.chip = {
> -		.nr_chips		= 1,
> -		.chip_delay		= 30,
> -	},
> -	.ctrl = {
> -		.probe		= xway_nand_probe,
> -		.cmd_ctrl	= xway_cmd_ctrl,
> -		.dev_ready	= xway_dev_ready,
> -		.select_chip	= xway_select_chip,
> -		.read_byte	= xway_read_byte,
> +	/* Scan to find existence of the device */
> +	if (nand_scan(mtd, 1)) {

How about returning nand_scan() ret code if it's negative.

> +		err = -ENXIO;
> +		goto out;

This out label seems useless here, just return the err code directly.

>  	}
> -};
> +
> +	err = mtd_device_register(mtd, NULL, 0);
> +
> +	if (!err)
> +		return 0;
> +
> +	nand_release(mtd);

I'd prefer

	if (err)
		nand_release(mtd);

	return err;

> +out:
> +	return err;
> +}
>  
>  /*
> - * Try to find the node inside the DT. If it is available attach out
> - * platform_nand_data
> + * Remove a NAND device.
>   */
> -static int __init xway_register_nand(void)
> +static int xway_nand_remove(struct platform_device *pdev)
>  {
> -	struct device_node *node;
> -	struct platform_device *pdev;
> -
> -	node = of_find_compatible_node(NULL, NULL, "lantiq,nand-xway");
> -	if (!node)
> -		return -ENOENT;
> -	pdev = of_find_device_by_node(node);
> -	if (!pdev)
> -		return -EINVAL;
> -	pdev->dev.platform_data = &xway_nand_data;
> -	of_node_put(node);
> +	struct xway_nand_data *data = platform_get_drvdata(pdev);
> +
> +	nand_release(nand_to_mtd(&data->chip));
> +
>  	return 0;
>  }
>  
> -subsys_initcall(xway_register_nand);
> +static const struct of_device_id xway_nand_match[] = {
> +	{ .compatible = "lantiq,nand-xway" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xway_nand_match);
> +
> +static struct platform_driver xway_nand_driver = {
> +	.probe	= xway_nand_probe,
> +	.remove	= xway_nand_remove,
> +	.driver	= {
> +		.name		= "lantiq,nand-xway",
> +		.of_match_table = xway_nand_match,
> +	},
> +};
> +
> +module_platform_driver(xway_nand_driver);
> +
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 1/8] MTD: xway: convert to normal platform driver

[Hauke Mehrtens]

On 06/19/2016 01:32 PM, Boris Brezillon wrote:
> Hi Hauke,
> 
> On Sat, 18 Jun 2016 21:14:05 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
>> Instead of hacking this into the plat_nand driver just make this a
>> normal nand driver.
> 
> Thanks for taking care of that.
> 
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  drivers/mtd/nand/Kconfig     |   1 -
>>  drivers/mtd/nand/xway_nand.c | 119 ++++++++++++++++++++++++++++++-------------
>>  2 files changed, 84 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index f05e0e9..d7de2c97 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -539,7 +539,6 @@ config MTD_NAND_FSMC
>>  config MTD_NAND_XWAY
>>  	tristate "Support for NAND on Lantiq XWAY SoC"
>>  	depends on LANTIQ && SOC_TYPE_XWAY
>> -	select MTD_NAND_PLATFORM
>>  	help
>>  	  Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached
>>  	  to the External Bus Unit (EBU).
>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
>> index 0cf0ac0..867a636 100644
>> --- a/drivers/mtd/nand/xway_nand.c
>> +++ b/drivers/mtd/nand/xway_nand.c
>> @@ -4,6 +4,7 @@
>>   *  by the Free Software Foundation.
>>   *
>>   *  Copyright © 2012 John Crispin <blogic@openwrt.org>
>> + *  Copyright © 2016 Hauke Mehrtens <hauke@hauke-m.de>
>>   */
>>  
>>  #include <linux/mtd/nand.h>
>> @@ -54,6 +55,10 @@
>>  #define NAND_CON_CSMUX		(1 << 1)
>>  #define NAND_CON_NANDM		1
>>  
>> +struct xway_nand_data {
>> +	struct nand_chip	chip;
>> +};
>> +
>>  static void xway_reset_chip(struct nand_chip *chip)
>>  {
>>  	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
>> @@ -130,14 +135,50 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
>>  	return ret;
>>  }
>>  
>> +/*
>> + * Probe for the NAND device.
>> + */
>>  static int xway_nand_probe(struct platform_device *pdev)
>>  {
>> -	struct nand_chip *this = platform_get_drvdata(pdev);
>> -	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
>> -	const __be32 *cs = of_get_property(pdev->dev.of_node,
>> -					"lantiq,cs", NULL);
>> +	struct xway_nand_data *data;
>> +	struct mtd_info *mtd;
>> +	struct resource *res;
>> +	int err = 0;
>> +	void __iomem *nandaddr;
>> +	const __be32 *cs;
>>  	u32 cs_flag = 0;
>>  
>> +	/* Allocate memory for the device structure (and zero it) */
>> +	data = devm_kzalloc(&pdev->dev, sizeof(struct xway_nand_data),
>> +			    GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	nandaddr = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(nandaddr))
>> +		return PTR_ERR(nandaddr);
>> +
>> +	nand_set_flash_node(&data->chip, pdev->dev.of_node);
>> +	mtd = nand_to_mtd(&data->chip);
>> +	mtd->dev.parent = &pdev->dev;
>> +
>> +	data->chip.IO_ADDR_R = nandaddr;
>> +	data->chip.IO_ADDR_W = nandaddr;
>> +	data->chip.cmd_ctrl = xway_cmd_ctrl;
>> +	data->chip.dev_ready = xway_dev_ready;
>> +	data->chip.select_chip = xway_select_chip;
>> +	data->chip.read_byte = xway_read_byte;
>> +	data->chip.chip_delay = 30;
>> +
>> +	data->chip.ecc.mode = NAND_ECC_SOFT;
>> +	data->chip.ecc.algo = NAND_ECC_HAMMING;
>> +
>> +	platform_set_drvdata(pdev, data);
>> +	nand_set_controller_data(&data->chip, data);
>> +
>> +	cs = of_get_property(pdev->dev.of_node, "lantiq,cs", NULL);
> 
> It may be a good time to switch to of_property_read_u32() (here or in a
> separate patch).

I will change this.

>> +
>>  	/* load our CS from the DT. Either we find a valid 1 or default to 0 */
>>  	if (cs && (*cs == 1))
>>  		cs_flag = NAND_CON_IN_CS1 | NAND_CON_OUT_CS1;
>> @@ -155,43 +196,51 @@ static int xway_nand_probe(struct platform_device *pdev)
>>  		| cs_flag, EBU_NAND_CON);
>>  
>>  	/* finish with a reset */
>> -	xway_reset_chip(this);
>> -
>> -	return 0;
>> -}
>> +	xway_reset_chip(&data->chip);
>>  
>> -static struct platform_nand_data xway_nand_data = {
>> -	.chip = {
>> -		.nr_chips		= 1,
>> -		.chip_delay		= 30,
>> -	},
>> -	.ctrl = {
>> -		.probe		= xway_nand_probe,
>> -		.cmd_ctrl	= xway_cmd_ctrl,
>> -		.dev_ready	= xway_dev_ready,
>> -		.select_chip	= xway_select_chip,
>> -		.read_byte	= xway_read_byte,
>> +	/* Scan to find existence of the device */
>> +	if (nand_scan(mtd, 1)) {
> 
> How about returning nand_scan() ret code if it's negative.

I will do that.

> 
>> +		err = -ENXIO;
>> +		goto out;
> 
> This out label seems useless here, just return the err code directly.

I will remove this.

> 
>>  	}
>> -};
>> +
>> +	err = mtd_device_register(mtd, NULL, 0);
>> +
>> +	if (!err)
>> +		return 0;
>> +
>> +	nand_release(mtd);
> 
> I'd prefer
> 
> 	if (err)
> 		nand_release(mtd);
> 
> 	return err;
> 

I will change that.

>> +out:
>> +	return err;
>> +}
>>  
>>  /*
>> - * Try to find the node inside the DT. If it is available attach out
>> - * platform_nand_data
>> + * Remove a NAND device.
>>   */
>> -static int __init xway_register_nand(void)
>> +static int xway_nand_remove(struct platform_device *pdev)
>>  {
>> -	struct device_node *node;
>> -	struct platform_device *pdev;
>> -
>> -	node = of_find_compatible_node(NULL, NULL, "lantiq,nand-xway");
>> -	if (!node)
>> -		return -ENOENT;
>> -	pdev = of_find_device_by_node(node);
>> -	if (!pdev)
>> -		return -EINVAL;
>> -	pdev->dev.platform_data = &xway_nand_data;
>> -	of_node_put(node);
>> +	struct xway_nand_data *data = platform_get_drvdata(pdev);
>> +
>> +	nand_release(nand_to_mtd(&data->chip));
>> +
>>  	return 0;
>>  }
>>  
>> -subsys_initcall(xway_register_nand);
>> +static const struct of_device_id xway_nand_match[] = {
>> +	{ .compatible = "lantiq,nand-xway" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, xway_nand_match);
>> +
>> +static struct platform_driver xway_nand_driver = {
>> +	.probe	= xway_nand_probe,
>> +	.remove	= xway_nand_remove,
>> +	.driver	= {
>> +		.name		= "lantiq,nand-xway",
>> +		.of_match_table = xway_nand_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(xway_nand_driver);
>> +
>> +MODULE_LICENSE("GPL");
> 

[PATCH v2 2/8] MTD: xway: add some more documentation

[Hauke Mehrtens]

This adds some register documentation which should make it easier to
understand how this controller works.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/nand/xway_nand.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index 867a636..2889e48 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -20,10 +20,20 @@
 #define EBU_NAND_ECC0		0xB8
 #define EBU_NAND_ECC_AC		0xBC
 
-/* nand commands */
+/*
+ * nand commands
+ * The pins of the NAND chip are selected based on the address bits of the
+ * "register" read and write. There are no special registers, but an
+ * address range and the lower address bits are used to activate the
+ * correct line. For example when the bit (1 << 2) is set in the address
+ * the ALE pin will be activated.
+ */
 #define NAND_CMD_ALE		(1 << 2)
 #define NAND_CMD_CLE		(1 << 3)
 #define NAND_CMD_CS		(1 << 4)
+#define NAND_CMD_SE		(1 << 5)
+#define NAND_CMD_WP		(1 << 6)
+#define NAND_CMD_PRE		(1 << 7)
 #define NAND_WRITE_CMD_RESET	0xff
 #define NAND_WRITE_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
 #define NAND_WRITE_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)
-- 
2.8.1

Re: [PATCH v2 2/8] MTD: xway: add some more documentation

[Boris Brezillon]

On Sat, 18 Jun 2016 21:14:06 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> This adds some register documentation which should make it easier to
> understand how this controller works.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/xway_nand.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index 867a636..2889e48 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -20,10 +20,20 @@
>  #define EBU_NAND_ECC0		0xB8
>  #define EBU_NAND_ECC_AC		0xBC
>  
> -/* nand commands */
> +/*
> + * nand commands
> + * The pins of the NAND chip are selected based on the address bits of the
> + * "register" read and write. There are no special registers, but an
> + * address range and the lower address bits are used to activate the
> + * correct line. For example when the bit (1 << 2) is set in the address
> + * the ALE pin will be activated.
> + */
>  #define NAND_CMD_ALE		(1 << 2)
>  #define NAND_CMD_CLE		(1 << 3)
>  #define NAND_CMD_CS		(1 << 4)
> +#define NAND_CMD_SE		(1 << 5)
> +#define NAND_CMD_WP		(1 << 6)
> +#define NAND_CMD_PRE		(1 << 7)

I see what _WP means, but what about _PRE and _SE? Can you document
these features?

>  #define NAND_WRITE_CMD_RESET	0xff
>  #define NAND_WRITE_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
>  #define NAND_WRITE_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)

[PATCH v2 3/8] MTD: xway: the latched command should be persistent

[Hauke Mehrtens]

From: John Crispin <john@phrozen.org>

If they are not persistent a different address will be used and the
controller will deactivate some pins.

Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/nand/xway_nand.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index 2889e48..346781a 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -67,6 +67,7 @@
 
 struct xway_nand_data {
 	struct nand_chip	chip;
+	u32			xway_latchcmd;
 };
 
 static void xway_reset_chip(struct nand_chip *chip)
@@ -104,22 +105,21 @@ static void xway_select_chip(struct mtd_info *mtd, int chip)
 
 static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 {
-	struct nand_chip *this = mtd_to_nand(mtd);
-	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct xway_nand_data *data = nand_get_controller_data(chip);
+	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
 	unsigned long flags;
 
 	if (ctrl & NAND_CTRL_CHANGE) {
-		nandaddr &= ~(NAND_WRITE_CMD | NAND_WRITE_ADDR);
 		if (ctrl & NAND_CLE)
-			nandaddr |= NAND_WRITE_CMD;
-		else
-			nandaddr |= NAND_WRITE_ADDR;
-		this->IO_ADDR_W = (void __iomem *) nandaddr;
+			data->xway_latchcmd = NAND_WRITE_CMD;
+		else if (ctrl & NAND_ALE)
+			data->xway_latchcmd = NAND_WRITE_ADDR;
 	}
 
 	if (cmd != NAND_CMD_NONE) {
 		spin_lock_irqsave(&ebu_lock, flags);
-		writeb(cmd, this->IO_ADDR_W);
+		writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd));
 		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
 			;
 		spin_unlock_irqrestore(&ebu_lock, flags);
-- 
2.8.1

Re: [PATCH v2 3/8] MTD: xway: the latched command should be persistent

[Boris Brezillon]

On Sat, 18 Jun 2016 21:14:07 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> From: John Crispin <john@phrozen.org>
> 
> If they are not persistent a different address will be used and the
> controller will deactivate some pins.
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/xway_nand.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index 2889e48..346781a 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -67,6 +67,7 @@
>  
>  struct xway_nand_data {
>  	struct nand_chip	chip;
> +	u32			xway_latchcmd;
>  };
>  
>  static void xway_reset_chip(struct nand_chip *chip)
> @@ -104,22 +105,21 @@ static void xway_select_chip(struct mtd_info *mtd, int chip)
>  
>  static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  {
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct xway_nand_data *data = nand_get_controller_data(chip);
> +	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
>  	unsigned long flags;
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
> -		nandaddr &= ~(NAND_WRITE_CMD | NAND_WRITE_ADDR);
>  		if (ctrl & NAND_CLE)
> -			nandaddr |= NAND_WRITE_CMD;
> -		else
> -			nandaddr |= NAND_WRITE_ADDR;
> -		this->IO_ADDR_W = (void __iomem *) nandaddr;
> +			data->xway_latchcmd = NAND_WRITE_CMD;
> +		else if (ctrl & NAND_ALE)
> +			data->xway_latchcmd = NAND_WRITE_ADDR;
>  	}
>  
>  	if (cmd != NAND_CMD_NONE) {
>  		spin_lock_irqsave(&ebu_lock, flags);
> -		writeb(cmd, this->IO_ADDR_W);
> +		writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd));
>  		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
>  			;
>  		spin_unlock_irqrestore(&ebu_lock, flags);

Not sure what you're trying to do exactly. AFAICS, your controller seems
to handle each addr and cmd cycles independently, so all you have to do
is something like that:

	/*
	 * I'm assuming baseaddr has been initialized to the NAND
	 * controller base address.
	 */

	if (cmd == NAND_CMD_NONE)
		return;

	spin_lock_irqsave(&ebu_lock, flags);
	if (ctrl & NAND_CLE)
		writeb(cmd, baseaddr + NAND_WRITE_CMD);
	else
		writeb(cmd, baseaddr + NAND_WRITE_ADDR);

	while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
		;
	spin_unlock_irqrestore(&ebu_lock, flags);

Re: [PATCH v2 3/8] MTD: xway: the latched command should be persistent

[Hauke Mehrtens]

On 06/19/2016 01:52 PM, Boris Brezillon wrote:
> On Sat, 18 Jun 2016 21:14:07 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
>> From: John Crispin <john@phrozen.org>
>>
>> If they are not persistent a different address will be used and the
>> controller will deactivate some pins.
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  drivers/mtd/nand/xway_nand.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
>> index 2889e48..346781a 100644
>> --- a/drivers/mtd/nand/xway_nand.c
>> +++ b/drivers/mtd/nand/xway_nand.c
>> @@ -67,6 +67,7 @@
>>  
>>  struct xway_nand_data {
>>  	struct nand_chip	chip;
>> +	u32			xway_latchcmd;
>>  };
>>  
>>  static void xway_reset_chip(struct nand_chip *chip)
>> @@ -104,22 +105,21 @@ static void xway_select_chip(struct mtd_info *mtd, int chip)
>>  
>>  static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>>  {
>> -	struct nand_chip *this = mtd_to_nand(mtd);
>> -	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	struct xway_nand_data *data = nand_get_controller_data(chip);
>> +	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
>>  	unsigned long flags;
>>  
>>  	if (ctrl & NAND_CTRL_CHANGE) {
>> -		nandaddr &= ~(NAND_WRITE_CMD | NAND_WRITE_ADDR);
>>  		if (ctrl & NAND_CLE)
>> -			nandaddr |= NAND_WRITE_CMD;
>> -		else
>> -			nandaddr |= NAND_WRITE_ADDR;
>> -		this->IO_ADDR_W = (void __iomem *) nandaddr;
>> +			data->xway_latchcmd = NAND_WRITE_CMD;
>> +		else if (ctrl & NAND_ALE)
>> +			data->xway_latchcmd = NAND_WRITE_ADDR;
>>  	}
>>  
>>  	if (cmd != NAND_CMD_NONE) {
>>  		spin_lock_irqsave(&ebu_lock, flags);
>> -		writeb(cmd, this->IO_ADDR_W);
>> +		writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd));
>>  		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
>>  			;
>>  		spin_unlock_irqrestore(&ebu_lock, flags);
> 
> Not sure what you're trying to do exactly. AFAICS, your controller seems
> to handle each addr and cmd cycles independently, so all you have to do
> is something like that:
> 
> 	/*
> 	 * I'm assuming baseaddr has been initialized to the NAND
> 	 * controller base address.
> 	 */
> 
> 	if (cmd == NAND_CMD_NONE)
> 		return;
> 
> 	spin_lock_irqsave(&ebu_lock, flags);
> 	if (ctrl & NAND_CLE)
> 		writeb(cmd, baseaddr + NAND_WRITE_CMD);
> 	else
> 		writeb(cmd, baseaddr + NAND_WRITE_ADDR);
> 
> 	while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
> 		;
> 	spin_unlock_irqrestore(&ebu_lock, flags);
> 

Is NAND_CLE always set when the line to the NAND flash chip should be
set? Then this should work and we ignore NAND_CTRL_CHANGE .

Hauke

Re: [PATCH v2 3/8] MTD: xway: the latched command should be persistent

[Boris Brezillon]

On Sun, 19 Jun 2016 14:04:51 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> On 06/19/2016 01:52 PM, Boris Brezillon wrote:
> > On Sat, 18 Jun 2016 21:14:07 +0200
> > Hauke Mehrtens <hauke@hauke-m.de> wrote:
> >   
> >> From: John Crispin <john@phrozen.org>
> >>
> >> If they are not persistent a different address will be used and the
> >> controller will deactivate some pins.
> >>
> >> Signed-off-by: John Crispin <john@phrozen.org>
> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >> ---
> >>  drivers/mtd/nand/xway_nand.c | 16 ++++++++--------
> >>  1 file changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> >> index 2889e48..346781a 100644
> >> --- a/drivers/mtd/nand/xway_nand.c
> >> +++ b/drivers/mtd/nand/xway_nand.c
> >> @@ -67,6 +67,7 @@
> >>  
> >>  struct xway_nand_data {
> >>  	struct nand_chip	chip;
> >> +	u32			xway_latchcmd;
> >>  };
> >>  
> >>  static void xway_reset_chip(struct nand_chip *chip)
> >> @@ -104,22 +105,21 @@ static void xway_select_chip(struct mtd_info *mtd, int chip)
> >>  
> >>  static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
> >>  {
> >> -	struct nand_chip *this = mtd_to_nand(mtd);
> >> -	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
> >> +	struct nand_chip *chip = mtd_to_nand(mtd);
> >> +	struct xway_nand_data *data = nand_get_controller_data(chip);
> >> +	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
> >>  	unsigned long flags;
> >>  
> >>  	if (ctrl & NAND_CTRL_CHANGE) {
> >> -		nandaddr &= ~(NAND_WRITE_CMD | NAND_WRITE_ADDR);
> >>  		if (ctrl & NAND_CLE)
> >> -			nandaddr |= NAND_WRITE_CMD;
> >> -		else
> >> -			nandaddr |= NAND_WRITE_ADDR;
> >> -		this->IO_ADDR_W = (void __iomem *) nandaddr;
> >> +			data->xway_latchcmd = NAND_WRITE_CMD;
> >> +		else if (ctrl & NAND_ALE)
> >> +			data->xway_latchcmd = NAND_WRITE_ADDR;
> >>  	}
> >>  
> >>  	if (cmd != NAND_CMD_NONE) {
> >>  		spin_lock_irqsave(&ebu_lock, flags);
> >> -		writeb(cmd, this->IO_ADDR_W);
> >> +		writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd));
> >>  		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
> >>  			;
> >>  		spin_unlock_irqrestore(&ebu_lock, flags);  
> > 
> > Not sure what you're trying to do exactly. AFAICS, your controller seems
> > to handle each addr and cmd cycles independently, so all you have to do
> > is something like that:
> > 
> > 	/*
> > 	 * I'm assuming baseaddr has been initialized to the NAND
> > 	 * controller base address.
> > 	 */
> > 
> > 	if (cmd == NAND_CMD_NONE)
> > 		return;
> > 
> > 	spin_lock_irqsave(&ebu_lock, flags);
> > 	if (ctrl & NAND_CLE)
> > 		writeb(cmd, baseaddr + NAND_WRITE_CMD);
> > 	else
> > 		writeb(cmd, baseaddr + NAND_WRITE_ADDR);
> > 
> > 	while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
> > 		;
> > 	spin_unlock_irqrestore(&ebu_lock, flags);
> >   
> 
> Is NAND_CLE always set when the line to the NAND flash chip should be
> set?

Yes it is (see the nand_command() [1] and nand_command_lp() [2]
implementations).

> Then this should work and we ignore NAND_CTRL_CHANGE .
> 
> Hauke

[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L588
[2]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L691

[PATCH v2 4/8] MTD: xway: remove endless loop

[Hauke Mehrtens]

From: John Crispin <john@phrozen.org>

The reset loop logic could run into a endless loop. Lets fix it as
requested.
http://lists.infradead.org/pipermail/linux-mtd/2012-September/044240.html

Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/nand/xway_nand.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index 346781a..61176c4 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -73,16 +73,22 @@ struct xway_nand_data {
 static void xway_reset_chip(struct nand_chip *chip)
 {
 	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
+	unsigned long timeout;
 	unsigned long flags;
 
 	nandaddr &= ~NAND_WRITE_ADDR;
 	nandaddr |= NAND_WRITE_CMD;
 
 	/* finish with a reset */
+	timeout = jiffies + msecs_to_jiffies(20);
+
 	spin_lock_irqsave(&ebu_lock, flags);
 	writeb(NAND_WRITE_CMD_RESET, (void __iomem *) nandaddr);
-	while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
-		;
+	do {
+		if ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
+			break;
+		cond_resched();
+	} while (!time_after_eq(jiffies, timeout));
 	spin_unlock_irqrestore(&ebu_lock, flags);
 }
 
-- 
2.8.1

Re: [PATCH v2 4/8] MTD: xway: remove endless loop

[Boris Brezillon]

On Sat, 18 Jun 2016 21:14:08 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> From: John Crispin <john@phrozen.org>
> 
> The reset loop logic could run into a endless loop. Lets fix it as
> requested.
> http://lists.infradead.org/pipermail/linux-mtd/2012-September/044240.html
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/xway_nand.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index 346781a..61176c4 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -73,16 +73,22 @@ struct xway_nand_data {
>  static void xway_reset_chip(struct nand_chip *chip)
>  {
>  	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
> +	unsigned long timeout;
>  	unsigned long flags;
>  
>  	nandaddr &= ~NAND_WRITE_ADDR;
>  	nandaddr |= NAND_WRITE_CMD;
>  
>  	/* finish with a reset */
> +	timeout = jiffies + msecs_to_jiffies(20);
> +
>  	spin_lock_irqsave(&ebu_lock, flags);
>  	writeb(NAND_WRITE_CMD_RESET, (void __iomem *) nandaddr);
> -	while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
> -		;
> +	do {
> +		if ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
> +			break;
> +		cond_resched();
> +	} while (!time_after_eq(jiffies, timeout));
>  	spin_unlock_irqrestore(&ebu_lock, flags);
>  }
>  

AFAICS, this function is doing exactly what
->cmdfunc(NAND_CMD_RESET, -1 , -1) does, except for the NAND_WAIT_WR_C
test? What is this bit representing?

I'd really prefer if you kill this function and just call ->cmdfunc()
instead.

Re: [PATCH v2 4/8] MTD: xway: remove endless loop

[Hauke Mehrtens]

On 06/19/2016 02:14 PM, Boris Brezillon wrote:
> On Sat, 18 Jun 2016 21:14:08 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
>> From: John Crispin <john@phrozen.org>
>>
>> The reset loop logic could run into a endless loop. Lets fix it as
>> requested.
>> http://lists.infradead.org/pipermail/linux-mtd/2012-September/044240.html
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  drivers/mtd/nand/xway_nand.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
>> index 346781a..61176c4 100644
>> --- a/drivers/mtd/nand/xway_nand.c
>> +++ b/drivers/mtd/nand/xway_nand.c
>> @@ -73,16 +73,22 @@ struct xway_nand_data {
>>  static void xway_reset_chip(struct nand_chip *chip)
>>  {
>>  	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
>> +	unsigned long timeout;
>>  	unsigned long flags;
>>  
>>  	nandaddr &= ~NAND_WRITE_ADDR;
>>  	nandaddr |= NAND_WRITE_CMD;
>>  
>>  	/* finish with a reset */
>> +	timeout = jiffies + msecs_to_jiffies(20);
>> +
>>  	spin_lock_irqsave(&ebu_lock, flags);
>>  	writeb(NAND_WRITE_CMD_RESET, (void __iomem *) nandaddr);
>> -	while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
>> -		;
>> +	do {
>> +		if ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
>> +			break;
>> +		cond_resched();
>> +	} while (!time_after_eq(jiffies, timeout));
>>  	spin_unlock_irqrestore(&ebu_lock, flags);
>>  }
>>  
> 
> AFAICS, this function is doing exactly what
> ->cmdfunc(NAND_CMD_RESET, -1 , -1) does, except for the NAND_WAIT_WR_C
> test? What is this bit representing?
> 
> I'd really prefer if you kill this function and just call ->cmdfunc()
> instead.
> 

NAND_WAIT_WR_C indicates that the previous operation is finished.

When this is not needed I can call the generic function.

Hauke

Re: [PATCH v2 4/8] MTD: xway: remove endless loop

[Boris Brezillon]

On Sun, 19 Jun 2016 14:32:56 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> On 06/19/2016 02:14 PM, Boris Brezillon wrote:
> > On Sat, 18 Jun 2016 21:14:08 +0200
> > Hauke Mehrtens <hauke@hauke-m.de> wrote:
> >   
> >> From: John Crispin <john@phrozen.org>
> >>
> >> The reset loop logic could run into a endless loop. Lets fix it as
> >> requested.
> >> http://lists.infradead.org/pipermail/linux-mtd/2012-September/044240.html
> >>
> >> Signed-off-by: John Crispin <john@phrozen.org>
> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >> ---
> >>  drivers/mtd/nand/xway_nand.c | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> >> index 346781a..61176c4 100644
> >> --- a/drivers/mtd/nand/xway_nand.c
> >> +++ b/drivers/mtd/nand/xway_nand.c
> >> @@ -73,16 +73,22 @@ struct xway_nand_data {
> >>  static void xway_reset_chip(struct nand_chip *chip)
> >>  {
> >>  	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
> >> +	unsigned long timeout;
> >>  	unsigned long flags;
> >>  
> >>  	nandaddr &= ~NAND_WRITE_ADDR;
> >>  	nandaddr |= NAND_WRITE_CMD;
> >>  
> >>  	/* finish with a reset */
> >> +	timeout = jiffies + msecs_to_jiffies(20);
> >> +
> >>  	spin_lock_irqsave(&ebu_lock, flags);
> >>  	writeb(NAND_WRITE_CMD_RESET, (void __iomem *) nandaddr);
> >> -	while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
> >> -		;
> >> +	do {
> >> +		if ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
> >> +			break;
> >> +		cond_resched();
> >> +	} while (!time_after_eq(jiffies, timeout));
> >>  	spin_unlock_irqrestore(&ebu_lock, flags);
> >>  }
> >>    
> > 
> > AFAICS, this function is doing exactly what  
> > ->cmdfunc(NAND_CMD_RESET, -1 , -1) does, except for the NAND_WAIT_WR_C  
> > test? What is this bit representing?
> > 
> > I'd really prefer if you kill this function and just call ->cmdfunc()
> > instead.
> >   
> 
> NAND_WAIT_WR_C indicates that the previous operation is finished.

Does "previous operation done" implies "NAND device is ready" (R/B pin
set to ready state and NAND_CMD_STATUS return ready)?
If that's not the case, then I'm not sure it's safe to reset the NAND
without waiting for the device to be ready, so I'd recommend using
->cmdfunc(RESET) in that case.

[PATCH v2 5/8] MTD: xway: add missing write_buf and read_buf to nand driver

[Hauke Mehrtens]

From: John Crispin <john@phrozen.org>

Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/nand/xway_nand.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index 61176c4..1511bdb 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -151,6 +151,33 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
 	return ret;
 }
 
+
+static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&ebu_lock, flags);
+	for (i = 0; i < len; i++)
+		buf[i] = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));
+	spin_unlock_irqrestore(&ebu_lock, flags);
+}
+
+static void xway_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&ebu_lock, flags);
+	for (i = 0; i < len; i++)
+		ltq_w8(buf[i], (void __iomem *)(nandaddr | NAND_WRITE_DATA));
+	spin_unlock_irqrestore(&ebu_lock, flags);
+}
+
 /*
  * Probe for the NAND device.
  */
@@ -184,6 +211,8 @@ static int xway_nand_probe(struct platform_device *pdev)
 	data->chip.cmd_ctrl = xway_cmd_ctrl;
 	data->chip.dev_ready = xway_dev_ready;
 	data->chip.select_chip = xway_select_chip;
+	data->chip.write_buf = xway_write_buf;
+	data->chip.read_buf = xway_read_buf;
 	data->chip.read_byte = xway_read_byte;
 	data->chip.chip_delay = 30;
 
-- 
2.8.1

Re: [PATCH v2 5/8] MTD: xway: add missing write_buf and read_buf to nand driver

[Boris Brezillon]

On Sat, 18 Jun 2016 21:14:09 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

Please add a commit message.

> From: John Crispin <john@phrozen.org>
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/xway_nand.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index 61176c4..1511bdb 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -151,6 +151,33 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
>  	return ret;
>  }
>  
> +
> +static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
> +	unsigned long flags;
> +	int i;
> +
> +	spin_lock_irqsave(&ebu_lock, flags);
> +	for (i = 0; i < len; i++)
> +		buf[i] = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));

Please remove these useless cast and use the + operator.

		buf[i] = ltq_r8(this->IO_ADDR_R + NAND_READ_DATA);

> +	spin_unlock_irqrestore(&ebu_lock, flags);

Reading data through the NAND bus can be quite long, do you really need
to take a spinlock and disable irqs for while doing that?
A mutex would be more appropriate IMO.

> +}
> +
> +static void xway_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
> +	unsigned long flags;
> +	int i;
> +
> +	spin_lock_irqsave(&ebu_lock, flags);
> +	for (i = 0; i < len; i++)
> +		ltq_w8(buf[i], (void __iomem *)(nandaddr | NAND_WRITE_DATA));

Ditto.

> +	spin_unlock_irqrestore(&ebu_lock, flags);
> +}
> +
>  /*
>   * Probe for the NAND device.
>   */
> @@ -184,6 +211,8 @@ static int xway_nand_probe(struct platform_device *pdev)
>  	data->chip.cmd_ctrl = xway_cmd_ctrl;
>  	data->chip.dev_ready = xway_dev_ready;
>  	data->chip.select_chip = xway_select_chip;
> +	data->chip.write_buf = xway_write_buf;
> +	data->chip.read_buf = xway_read_buf;
>  	data->chip.read_byte = xway_read_byte;
>  	data->chip.chip_delay = 30;
>  

[PATCH v2 6/8] MTD: xway: fix nand locking

[Hauke Mehrtens]

From: John Crispin <john@phrozen.org>

The external Bus Unit (EBU) can control different flash devices, but
these NAND flash commands have to be atomic and should not be
interrupted in between. Lock the EBU from the beginning of the command
till the end by moving the lock to the chip select.

Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/nand/xway_nand.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index 1511bdb..064ee41 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip)
 
 static void xway_select_chip(struct mtd_info *mtd, int chip)
 {
+	static unsigned long csflags;
 
 	switch (chip) {
 	case -1:
 		ltq_ebu_w32_mask(NAND_CON_CE, 0, EBU_NAND_CON);
 		ltq_ebu_w32_mask(NAND_CON_NANDM, 0, EBU_NAND_CON);
+		spin_unlock_irqrestore(&ebu_lock, csflags);
 		break;
 	case 0:
+		spin_lock_irqsave(&ebu_lock, csflags);
 		ltq_ebu_w32_mask(0, NAND_CON_NANDM, EBU_NAND_CON);
 		ltq_ebu_w32_mask(0, NAND_CON_CE, EBU_NAND_CON);
 		break;
@@ -114,7 +117,6 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	struct xway_nand_data *data = nand_get_controller_data(chip);
 	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
-	unsigned long flags;
 
 	if (ctrl & NAND_CTRL_CHANGE) {
 		if (ctrl & NAND_CLE)
@@ -124,11 +126,9 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 	}
 
 	if (cmd != NAND_CMD_NONE) {
-		spin_lock_irqsave(&ebu_lock, flags);
 		writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd));
 		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
 			;
-		spin_unlock_irqrestore(&ebu_lock, flags);
 	}
 }
 
@@ -141,14 +141,8 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
 	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&ebu_lock, flags);
-	ret = ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
-	spin_unlock_irqrestore(&ebu_lock, flags);
 
-	return ret;
+	return ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
 }
 
 
@@ -156,26 +150,20 @@ static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
 	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
-	unsigned long flags;
 	int i;
 
-	spin_lock_irqsave(&ebu_lock, flags);
 	for (i = 0; i < len; i++)
 		buf[i] = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));
-	spin_unlock_irqrestore(&ebu_lock, flags);
 }
 
 static void xway_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
 	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
-	unsigned long flags;
 	int i;
 
-	spin_lock_irqsave(&ebu_lock, flags);
 	for (i = 0; i < len; i++)
 		ltq_w8(buf[i], (void __iomem *)(nandaddr | NAND_WRITE_DATA));
-	spin_unlock_irqrestore(&ebu_lock, flags);
 }
 
 /*
-- 
2.8.1

Re: [PATCH v2 6/8] MTD: xway: fix nand locking

[Boris Brezillon]

On Sat, 18 Jun 2016 21:14:10 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> From: John Crispin <john@phrozen.org>
> 
> The external Bus Unit (EBU) can control different flash devices, but
> these NAND flash commands have to be atomic and should not be
> interrupted in between. Lock the EBU from the beginning of the command
> till the end by moving the lock to the chip select.
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/xway_nand.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index 1511bdb..064ee41 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip)
>  
>  static void xway_select_chip(struct mtd_info *mtd, int chip)
>  {
> +	static unsigned long csflags;
>  
>  	switch (chip) {
>  	case -1:
>  		ltq_ebu_w32_mask(NAND_CON_CE, 0, EBU_NAND_CON);
>  		ltq_ebu_w32_mask(NAND_CON_NANDM, 0, EBU_NAND_CON);
> +		spin_unlock_irqrestore(&ebu_lock, csflags);
>  		break;
>  	case 0:
> +		spin_lock_irqsave(&ebu_lock, csflags);
>  		ltq_ebu_w32_mask(0, NAND_CON_NANDM, EBU_NAND_CON);
>  		ltq_ebu_w32_mask(0, NAND_CON_CE, EBU_NAND_CON);

So now the spinlock is taken (and irq disabled) during the whole NAND
operation. Ouch! I hope you don't have real-time constraints in your
system.

>  		break;
> @@ -114,7 +117,6 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  	struct xway_nand_data *data = nand_get_controller_data(chip);
>  	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
> -	unsigned long flags;
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
>  		if (ctrl & NAND_CLE)
> @@ -124,11 +126,9 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  	}
>  
>  	if (cmd != NAND_CMD_NONE) {
> -		spin_lock_irqsave(&ebu_lock, flags);
>  		writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd));
>  		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
>  			;
> -		spin_unlock_irqrestore(&ebu_lock, flags);
>  	}
>  }
>  
> @@ -141,14 +141,8 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
> -	unsigned long flags;
> -	int ret;
> -
> -	spin_lock_irqsave(&ebu_lock, flags);
> -	ret = ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
> -	spin_unlock_irqrestore(&ebu_lock, flags);
>  
> -	return ret;
> +	return ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
>  }
>  
>  
> @@ -156,26 +150,20 @@ static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
> -	unsigned long flags;
>  	int i;
>  
> -	spin_lock_irqsave(&ebu_lock, flags);
>  	for (i = 0; i < len; i++)
>  		buf[i] = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));
> -	spin_unlock_irqrestore(&ebu_lock, flags);
>  }
>  
>  static void xway_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
> -	unsigned long flags;
>  	int i;
>  
> -	spin_lock_irqsave(&ebu_lock, flags);
>  	for (i = 0; i < len; i++)
>  		ltq_w8(buf[i], (void __iomem *)(nandaddr | NAND_WRITE_DATA));
> -	spin_unlock_irqrestore(&ebu_lock, flags);
>  }
>  
>  /*

Re: [PATCH v2 6/8] MTD: xway: fix nand locking

[Richard Weinberger]

Am 19.06.2016 um 14:41 schrieb Boris Brezillon:
> On Sat, 18 Jun 2016 21:14:10 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
>> From: John Crispin <john@phrozen.org>
>>
>> The external Bus Unit (EBU) can control different flash devices, but
>> these NAND flash commands have to be atomic and should not be
>> interrupted in between. Lock the EBU from the beginning of the command
>> till the end by moving the lock to the chip select.
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  drivers/mtd/nand/xway_nand.c | 20 ++++----------------
>>  1 file changed, 4 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
>> index 1511bdb..064ee41 100644
>> --- a/drivers/mtd/nand/xway_nand.c
>> +++ b/drivers/mtd/nand/xway_nand.c
>> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip)
>>  
>>  static void xway_select_chip(struct mtd_info *mtd, int chip)
>>  {
>> +	static unsigned long csflags;

Why is csflags static? AFAICT this is racy then...

Thanks,
//richard

Re: [PATCH v2 6/8] MTD: xway: fix nand locking

[Hauke Mehrtens]

On 06/19/2016 02:53 PM, Richard Weinberger wrote:
> Am 19.06.2016 um 14:41 schrieb Boris Brezillon:
>> On Sat, 18 Jun 2016 21:14:10 +0200
>> Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>
>>> From: John Crispin <john@phrozen.org>
>>>
>>> The external Bus Unit (EBU) can control different flash devices, but
>>> these NAND flash commands have to be atomic and should not be
>>> interrupted in between. Lock the EBU from the beginning of the command
>>> till the end by moving the lock to the chip select.
>>>
>>> Signed-off-by: John Crispin <john@phrozen.org>
>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>> ---
>>>  drivers/mtd/nand/xway_nand.c | 20 ++++----------------
>>>  1 file changed, 4 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
>>> index 1511bdb..064ee41 100644
>>> --- a/drivers/mtd/nand/xway_nand.c
>>> +++ b/drivers/mtd/nand/xway_nand.c
>>> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip)
>>>  
>>>  static void xway_select_chip(struct mtd_info *mtd, int chip)
>>>  {
>>> +	static unsigned long csflags;
> 
> Why is csflags static? AFAICT this is racy then...

Because the lock is taken when the chip is selected and it is unlocked
when the chip select is deactivated again. This should be part of the
private driver struct.

Hauke

Re: [PATCH v2 6/8] MTD: xway: fix nand locking

[Boris Brezillon]

On Sun, 19 Jun 2016 14:56:03 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> On 06/19/2016 02:53 PM, Richard Weinberger wrote:
> > Am 19.06.2016 um 14:41 schrieb Boris Brezillon:  
> >> On Sat, 18 Jun 2016 21:14:10 +0200
> >> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> >>  
> >>> From: John Crispin <john@phrozen.org>
> >>>
> >>> The external Bus Unit (EBU) can control different flash devices, but
> >>> these NAND flash commands have to be atomic and should not be
> >>> interrupted in between. Lock the EBU from the beginning of the command
> >>> till the end by moving the lock to the chip select.
> >>>
> >>> Signed-off-by: John Crispin <john@phrozen.org>
> >>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >>> ---
> >>>  drivers/mtd/nand/xway_nand.c | 20 ++++----------------
> >>>  1 file changed, 4 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> >>> index 1511bdb..064ee41 100644
> >>> --- a/drivers/mtd/nand/xway_nand.c
> >>> +++ b/drivers/mtd/nand/xway_nand.c
> >>> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip)
> >>>  
> >>>  static void xway_select_chip(struct mtd_info *mtd, int chip)
> >>>  {
> >>> +	static unsigned long csflags;  
> > 
> > Why is csflags static? AFAICT this is racy then...  
> 
> Because the lock is taken when the chip is selected and it is unlocked
> when the chip select is deactivated again. This should be part of the
> private driver struct.

static here means that you have a single instance for all callers. This
is working fine because you have a lock at the NAND core level, and you
only have a single controller, but you definitely don't want to rely on
that in the long run.

In any case, using a static variable to save irqflags sounds like a
bad idea. This should always be saved on the stack to limit the time
spent in spin_lock_irqsave()/spin_unlock_irqrestored() critical
sections.
Which bring us back to my first question: are you sure you want to
disable irqs for such a long/unbounded time?

Re: [PATCH v2 6/8] MTD: xway: fix nand locking

[Boris Brezillon]

On Sun, 19 Jun 2016 14:53:40 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am 19.06.2016 um 14:41 schrieb Boris Brezillon:
> > On Sat, 18 Jun 2016 21:14:10 +0200
> > Hauke Mehrtens <hauke@hauke-m.de> wrote:
> >   
> >> From: John Crispin <john@phrozen.org>
> >>
> >> The external Bus Unit (EBU) can control different flash devices, but
> >> these NAND flash commands have to be atomic and should not be
> >> interrupted in between. Lock the EBU from the beginning of the command
> >> till the end by moving the lock to the chip select.
> >>
> >> Signed-off-by: John Crispin <john@phrozen.org>
> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >> ---
> >>  drivers/mtd/nand/xway_nand.c | 20 ++++----------------
> >>  1 file changed, 4 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> >> index 1511bdb..064ee41 100644
> >> --- a/drivers/mtd/nand/xway_nand.c
> >> +++ b/drivers/mtd/nand/xway_nand.c
> >> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip)
> >>  
> >>  static void xway_select_chip(struct mtd_info *mtd, int chip)
> >>  {
> >> +	static unsigned long csflags;  
> 
> Why is csflags static? AFAICT this is racy then...

Oh, nice catch! It's definitely wrong.

> 
> Thanks,
> //richard

[PATCH v2 7/8] MTD: xway: extract read and write function

[Hauke Mehrtens]

Extract the functions to read and write to the register of the NAND
flash controller.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/nand/xway_nand.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index 064ee41..b3badf9 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -70,6 +70,22 @@ struct xway_nand_data {
 	u32			xway_latchcmd;
 };
 
+static u8 xway_readb(struct mtd_info *mtd, int op)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_R;
+
+	return readb((void __iomem *)(nandaddr | op));
+}
+
+static void xway_writeb(struct mtd_info *mtd, int op, u8 value)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
+
+	writeb(value, (void __iomem *)(nandaddr | op));
+}
+
 static void xway_reset_chip(struct nand_chip *chip)
 {
 	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
@@ -116,7 +132,6 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 {
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	struct xway_nand_data *data = nand_get_controller_data(chip);
-	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
 
 	if (ctrl & NAND_CTRL_CHANGE) {
 		if (ctrl & NAND_CLE)
@@ -126,7 +141,7 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 	}
 
 	if (cmd != NAND_CMD_NONE) {
-		writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd));
+		xway_writeb(mtd, data->xway_latchcmd, cmd);
 		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
 			;
 	}
@@ -139,31 +154,23 @@ static int xway_dev_ready(struct mtd_info *mtd)
 
 static unsigned char xway_read_byte(struct mtd_info *mtd)
 {
-	struct nand_chip *this = mtd_to_nand(mtd);
-	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
-
-	return ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
+	return xway_readb(mtd, NAND_READ_DATA);
 }
 
-
 static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len)
 {
-	struct nand_chip *this = mtd_to_nand(mtd);
-	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
 	int i;
 
 	for (i = 0; i < len; i++)
-		buf[i] = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));
+		buf[i] = xway_readb(mtd, NAND_READ_DATA);
 }
 
 static void xway_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
 {
-	struct nand_chip *this = mtd_to_nand(mtd);
-	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
 	int i;
 
 	for (i = 0; i < len; i++)
-		ltq_w8(buf[i], (void __iomem *)(nandaddr | NAND_WRITE_DATA));
+		xway_writeb(mtd, NAND_WRITE_DATA, buf[i]);
 }
 
 /*
-- 
2.8.1

Re: [PATCH v2 7/8] MTD: xway: extract read and write function

[Boris Brezillon]

On Sat, 18 Jun 2016 21:14:11 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> Extract the functions to read and write to the register of the NAND
> flash controller.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/xway_nand.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index 064ee41..b3badf9 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -70,6 +70,22 @@ struct xway_nand_data {
>  	u32			xway_latchcmd;
>  };
>  
> +static u8 xway_readb(struct mtd_info *mtd, int op)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_R;
> +
> +	return readb((void __iomem *)(nandaddr | op));

Please use the + operator and drop the cast.

> +}
> +
> +static void xway_writeb(struct mtd_info *mtd, int op, u8 value)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
> +
> +	writeb(value, (void __iomem *)(nandaddr | op));

Ditto.

> +}
> +
>  static void xway_reset_chip(struct nand_chip *chip)
>  {
>  	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
> @@ -116,7 +132,6 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  {
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  	struct xway_nand_data *data = nand_get_controller_data(chip);
> -	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
>  		if (ctrl & NAND_CLE)
> @@ -126,7 +141,7 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  	}
>  
>  	if (cmd != NAND_CMD_NONE) {
> -		writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd));
> +		xway_writeb(mtd, data->xway_latchcmd, cmd);
>  		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
>  			;
>  	}
> @@ -139,31 +154,23 @@ static int xway_dev_ready(struct mtd_info *mtd)
>  
>  static unsigned char xway_read_byte(struct mtd_info *mtd)
>  {
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
> -
> -	return ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
> +	return xway_readb(mtd, NAND_READ_DATA);
>  }
>  
> -
>  static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len)
>  {
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	unsigned long nandaddr = (unsigned long) this->IO_ADDR_R;
>  	int i;
>  
>  	for (i = 0; i < len; i++)
> -		buf[i] = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));
> +		buf[i] = xway_readb(mtd, NAND_READ_DATA);
>  }
>  
>  static void xway_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
>  {
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	unsigned long nandaddr = (unsigned long) this->IO_ADDR_W;
>  	int i;
>  
>  	for (i = 0; i < len; i++)
> -		ltq_w8(buf[i], (void __iomem *)(nandaddr | NAND_WRITE_DATA));
> +		xway_writeb(mtd, NAND_WRITE_DATA, buf[i]);
>  }
>  
>  /*

[PATCH v2 8/8] MTD: xway: use global NAND_CMD_RESET define

[Hauke Mehrtens]

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/nand/xway_nand.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index b3badf9..7244bb8 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -34,7 +34,6 @@
 #define NAND_CMD_SE		(1 << 5)
 #define NAND_CMD_WP		(1 << 6)
 #define NAND_CMD_PRE		(1 << 7)
-#define NAND_WRITE_CMD_RESET	0xff
 #define NAND_WRITE_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
 #define NAND_WRITE_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)
 #define NAND_WRITE_DATA		(NAND_CMD_CS)
@@ -99,7 +98,7 @@ static void xway_reset_chip(struct nand_chip *chip)
 	timeout = jiffies + msecs_to_jiffies(20);
 
 	spin_lock_irqsave(&ebu_lock, flags);
-	writeb(NAND_WRITE_CMD_RESET, (void __iomem *) nandaddr);
+	writeb(NAND_CMD_RESET, (void __iomem *) nandaddr);
 	do {
 		if ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
 			break;
-- 
2.8.1

Re: [PATCH v2 8/8] MTD: xway: use global NAND_CMD_RESET define

[Boris Brezillon]

On Sat, 18 Jun 2016 21:14:12 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/xway_nand.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index b3badf9..7244bb8 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -34,7 +34,6 @@
>  #define NAND_CMD_SE		(1 << 5)
>  #define NAND_CMD_WP		(1 << 6)
>  #define NAND_CMD_PRE		(1 << 7)
> -#define NAND_WRITE_CMD_RESET	0xff
>  #define NAND_WRITE_CMD		(NAND_CMD_CS | NAND_CMD_CLE)
>  #define NAND_WRITE_ADDR		(NAND_CMD_CS | NAND_CMD_ALE)
>  #define NAND_WRITE_DATA		(NAND_CMD_CS)
> @@ -99,7 +98,7 @@ static void xway_reset_chip(struct nand_chip *chip)
>  	timeout = jiffies + msecs_to_jiffies(20);
>  
>  	spin_lock_irqsave(&ebu_lock, flags);
> -	writeb(NAND_WRITE_CMD_RESET, (void __iomem *) nandaddr);
> +	writeb(NAND_CMD_RESET, (void __iomem *) nandaddr);

That's a good intention, but as I said, I think we can get rid of the
whole function and use ->cmdfunc(RESET) instead.

>  	do {
>  		if ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
>  			break;

Re: [PATCH v2 0/8] MTD: xway: fix driver

[Boris Brezillon]

Hi Hauke,

On Sat, 18 Jun 2016 21:14:04 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> Without these patches the driver does not work for me.
> Some of these patches are in OpenWrt for years now and should go 
> upstream. In addition this converts it from some hack with the 
> plat_nand driver to a normal platform driver.

Thanks for the cleanup.

Still, I think we could go further. For example, you could get rid of
the IO_R/W_ADDR assignment and have your own ->iomem field in
xway_nand_data.
And I'd also like to see a clean nand_controller/nand_chip separation,
as done in other drivers (brcm, sunxi, qcom, ...), and that would be
even better if you could support a new binding where the NAND
controller and NAND chip are properly separated.
Note that these changes can be done incrementally and won't prevent the
inclusion of the patches you've already posted.

The last thing that is really bothering me is the ebu spinlock and its
implications on the whole system responsiveness.

Could you tell me more about this EBU. Do you really have to make it a
spinlock, and do you really have to disable irqs?

Regards,

Boris

> 
> changes since:
> v1:
>  - convert to normal platform driver
>  - do not use global variable xway_latchcmd
>  - use mtd_to_nand()
> 
> Hauke Mehrtens (4):
>   MTD: xway: convert to normal platform driver
>   MTD: xway: add some more documentation
>   MTD: xway: extract read and write function
>   MTD: xway: use global NAND_CMD_RESET define
> 
> John Crispin (4):
>   MTD: xway: the latched command should be persistent
>   MTD: xway: remove endless loop
>   MTD: xway: add missing write_buf and read_buf to nand driver
>   MTD: xway: fix nand locking
> 
>  drivers/mtd/nand/Kconfig     |   1 -
>  drivers/mtd/nand/xway_nand.c | 206 ++++++++++++++++++++++++++++++-------------
>  2 files changed, 147 insertions(+), 60 deletions(-)
> 

Re: [PATCH v2 0/8] MTD: xway: fix driver

[Hauke Mehrtens]

On 06/19/2016 02:50 PM, Boris Brezillon wrote:
> Hi Hauke,
> 
> On Sat, 18 Jun 2016 21:14:04 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
>> Without these patches the driver does not work for me.
>> Some of these patches are in OpenWrt for years now and should go 
>> upstream. In addition this converts it from some hack with the 
>> plat_nand driver to a normal platform driver.
> 
> Thanks for the cleanup.
> 
> Still, I think we could go further. For example, you could get rid of
> the IO_R/W_ADDR assignment and have your own ->iomem field in
> xway_nand_data.

Ok that should be possible.

> And I'd also like to see a clean nand_controller/nand_chip separation,
> as done in other drivers (brcm, sunxi, qcom, ...), and that would be
> even better if you could support a new binding where the NAND
> controller and NAND chip are properly separated.
> Note that these changes can be done incrementally and won't prevent the
> inclusion of the patches you've already posted.
> 
> The last thing that is really bothering me is the ebu spinlock and its
> implications on the whole system responsiveness.
> 
> Could you tell me more about this EBU. Do you really have to make it a
> spinlock, and do you really have to disable irqs?

The External Bus unit (EBU) is used to connect NAND and API flash chips
and is used in some strange setups as a GPIO controller.

I will try to make it a mutex and see what happens.

> 
> Regards,
> 
> Boris
> 
>>
>> changes since:
>> v1:
>>  - convert to normal platform driver
>>  - do not use global variable xway_latchcmd
>>  - use mtd_to_nand()
>>
>> Hauke Mehrtens (4):
>>   MTD: xway: convert to normal platform driver
>>   MTD: xway: add some more documentation
>>   MTD: xway: extract read and write function
>>   MTD: xway: use global NAND_CMD_RESET define
>>
>> John Crispin (4):
>>   MTD: xway: the latched command should be persistent
>>   MTD: xway: remove endless loop
>>   MTD: xway: add missing write_buf and read_buf to nand driver
>>   MTD: xway: fix nand locking
>>
>>  drivers/mtd/nand/Kconfig     |   1 -
>>  drivers/mtd/nand/xway_nand.c | 206 ++++++++++++++++++++++++++++++-------------
>>  2 files changed, 147 insertions(+), 60 deletions(-)
>>
>