[PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[Hauke Mehrtens]

These patches are in OpenWrt for years now and should go upstream. They 
are fixing some problems in the NAND driver.

Hauke Mehrtens (1):
  MTD: xway: add some more documentation

John Crispin (5):
  MTD: xway: fix invalid operator
  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/xway_nand.c | 67 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 16 deletions(-)

-- 
2.8.1

[PATCH 1/6] 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 0cf0ac0..ccac19c 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -19,10 +19,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

[PATCH 2/6] MTD: xway: fix invalid operator

[Hauke Mehrtens]

From: John Crispin <john@phrozen.org>

xway_read_byte should use a logic or and not an add operator when
working out the NAND address. The NAND address bits are used to
activate the pins to the NAND flash.

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

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index ccac19c..0ab6e83 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -134,7 +134,7 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
 	int ret;
 
 	spin_lock_irqsave(&ebu_lock, flags);
-	ret = ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
+	ret = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));
 	spin_unlock_irqrestore(&ebu_lock, flags);
 
 	return ret;
-- 
2.8.1

[PATCH 3/6] 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 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index 0ab6e83..79fecc8 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -64,6 +64,8 @@
 #define NAND_CON_CSMUX		(1 << 1)
 #define NAND_CON_NANDM		1
 
+static u32 xway_latchcmd;
+
 static void xway_reset_chip(struct nand_chip *chip)
 {
 	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
@@ -104,17 +106,15 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 	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;
+			xway_latchcmd = NAND_WRITE_CMD;
+		else if (ctrl & NAND_ALE)
+			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 | xway_latchcmd));
 		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
 			;
 		spin_unlock_irqrestore(&ebu_lock, flags);
-- 
2.8.1

[PATCH 4/6] 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 79fecc8..d44e00d 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -69,16 +69,22 @@ static u32 xway_latchcmd;
 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

[PATCH 5/6] 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 | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index d44e00d..bc4018e 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -146,6 +146,32 @@ 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->priv;
+	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->priv;
+	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);
+}
+
 static int xway_nand_probe(struct platform_device *pdev)
 {
 	struct nand_chip *this = platform_get_drvdata(pdev);
@@ -187,6 +213,8 @@ static struct platform_nand_data xway_nand_data = {
 		.dev_ready	= xway_dev_ready,
 		.select_chip	= xway_select_chip,
 		.read_byte	= xway_read_byte,
+		.read_buf	= xway_read_buf,
+		.write_buf	= xway_write_buf,
 	}
 };
 
-- 
2.8.1

[PATCH 6/6] 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 | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
index bc4018e..a6ea80c 100644
--- a/drivers/mtd/nand/xway_nand.c
+++ b/drivers/mtd/nand/xway_nand.c
@@ -90,13 +90,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;
@@ -109,7 +112,6 @@ 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;
-	unsigned long flags;
 
 	if (ctrl & NAND_CTRL_CHANGE) {
 		if (ctrl & NAND_CLE)
@@ -119,11 +121,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 | xway_latchcmd));
 		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
 			;
-		spin_unlock_irqrestore(&ebu_lock, flags);
 	}
 }
 
@@ -136,12 +136,9 @@ 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;
 }
@@ -150,26 +147,20 @@ static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len)
 {
 	struct nand_chip *this = mtd->priv;
 	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->priv;
 	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);
 }
 
 static int xway_nand_probe(struct platform_device *pdev)
-- 
2.8.1

Re: [PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[Boris Brezillon]

Hi Hauke,

On Sun,  5 Jun 2016 23:20:03 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> These patches are in OpenWrt for years now and should go upstream. They 
> are fixing some problems in the NAND driver.

You forgot to add the NAND maintainers (Richard and I). Next
time, please use get_maintainer.pl to find the person you should send
your patches to.

Thanks,

Boris

> 
> Hauke Mehrtens (1):
>   MTD: xway: add some more documentation
> 
> John Crispin (5):
>   MTD: xway: fix invalid operator
>   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/xway_nand.c | 67 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 16 deletions(-)
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/6] MTD: xway: fix invalid operator

[Boris Brezillon]

On Sun,  5 Jun 2016 23:20:05 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> From: John Crispin <john@phrozen.org>
> 
> xway_read_byte should use a logic or and not an add operator when
> working out the NAND address. The NAND address bits are used to
> activate the pins to the NAND flash.
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/nand/xway_nand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index ccac19c..0ab6e83 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -134,7 +134,7 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
>  	int ret;
>  
>  	spin_lock_irqsave(&ebu_lock, flags);
> -	ret = ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
> +	ret = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));

It's doing exactly the same, isn't it? What's the rationale behind this
change?

>  	spin_unlock_irqrestore(&ebu_lock, flags);
>  
>  	return ret;



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris Brezillon]

On Sun,  5 Jun 2016 23:20:06 +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 | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index 0ab6e83..79fecc8 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -64,6 +64,8 @@
>  #define NAND_CON_CSMUX		(1 << 1)
>  #define NAND_CON_NANDM		1
>  
> +static u32 xway_latchcmd;

Please avoid using global variables. This field should be part of your
private nand_chip struct.

> +
>  static void xway_reset_chip(struct nand_chip *chip)
>  {
>  	unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W;
> @@ -104,17 +106,15 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  	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;
> +			xway_latchcmd = NAND_WRITE_CMD;
> +		else if (ctrl & NAND_ALE)
> +			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 | xway_latchcmd));
>  		while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0)
>  			;
>  		spin_unlock_irqrestore(&ebu_lock, flags);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris Brezillon]

On Sun,  5 Jun 2016 23:20:08 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> 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 | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> index d44e00d..bc4018e 100644
> --- a/drivers/mtd/nand/xway_nand.c
> +++ b/drivers/mtd/nand/xway_nand.c
> @@ -146,6 +146,32 @@ 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->priv;

You should now use mtd_to_nand(). Please rebase your work on top of
nand/next [1], and I'm pretty sure you'll get a NULL pointer.

> +	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->priv;
> +	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);
> +}
> +
>  static int xway_nand_probe(struct platform_device *pdev)
>  {
>  	struct nand_chip *this = platform_get_drvdata(pdev);
> @@ -187,6 +213,8 @@ static struct platform_nand_data xway_nand_data = {
>  		.dev_ready	= xway_dev_ready,
>  		.select_chip	= xway_select_chip,
>  		.read_byte	= xway_read_byte,
> +		.read_buf	= xway_read_buf,
> +		.write_buf	= xway_write_buf,
>  	}
>  };
>  

[1]https://github.com/linux-nand/linux/tree/nand/next

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[Boris Brezillon]

On Sun,  5 Jun 2016 23:20:03 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> These patches are in OpenWrt for years now and should go upstream. They 
> are fixing some problems in the NAND driver.

Just had a closer look at the xway NAND controller driver, and it's
just a big pile of hacks :-(. I'll take those patches if nobody is
willing to maintain this driver, but honestly, I'd prefer a complete
rework of the driver.

> 
> Hauke Mehrtens (1):
>   MTD: xway: add some more documentation
> 
> John Crispin (5):
>   MTD: xway: fix invalid operator
>   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/xway_nand.c | 67 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 16 deletions(-)
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[John Crispin]

On 07/06/2016 11:48, Boris Brezillon wrote:
> On Sun,  5 Jun 2016 23:20:03 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
>> These patches are in OpenWrt for years now and should go upstream. They 
>> are fixing some problems in the NAND driver.
> 
> Just had a closer look at the xway NAND controller driver, and it's
> just a big pile of hacks :-(. I'll take those patches if nobody is
> willing to maintain this driver, but honestly, I'd prefer a complete
> rework of the driver.
> 

Hi Boris,

it is indeed a horrific pile of doo doo. it has grown historically over
a few years and then became sort of abandoned. i have been keeping it
artificially alive inside openwrt as we have users with boards that have
nand. i dont even own a lantiq board with nand, so patches were sort of
merged on cruise control and with compile testing only.

the SoCs have 2 ways of controlling the nand core. the easy one is this,
which is basically nothing more than a nand flash aware 16bit
intel/hitachi bus interafec called EBU. There is a more advanced dma
based way of doing nand I/O though.

ideally there should be a driver for the so called "high speed nand"
interface which would allow us to nuke this one.

	John

>>
>> Hauke Mehrtens (1):
>>   MTD: xway: add some more documentation
>>
>> John Crispin (5):
>>   MTD: xway: fix invalid operator
>>   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/xway_nand.c | 67 +++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 51 insertions(+), 16 deletions(-)
>>
> 
> 
> 

Re: [PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[Hauke Mehrtens]

On 06/07/2016 12:12 PM, John Crispin wrote:
> 
> 
> On 07/06/2016 11:48, Boris Brezillon wrote:
>> On Sun,  5 Jun 2016 23:20:03 +0200
>> Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>
>>> These patches are in OpenWrt for years now and should go upstream. They 
>>> are fixing some problems in the NAND driver.
>>
>> Just had a closer look at the xway NAND controller driver, and it's
>> just a big pile of hacks :-(. I'll take those patches if nobody is
>> willing to maintain this driver, but honestly, I'd prefer a complete
>> rework of the driver.
>>
> 
> Hi Boris,
> 
> it is indeed a horrific pile of doo doo. it has grown historically over
> a few years and then became sort of abandoned. i have been keeping it
> artificially alive inside openwrt as we have users with boards that have
> nand. i dont even own a lantiq board with nand, so patches were sort of
> merged on cruise control and with compile testing only.
> 
> the SoCs have 2 ways of controlling the nand core. the easy one is this,
> which is basically nothing more than a nand flash aware 16bit
> intel/hitachi bus interafec called EBU. There is a more advanced dma
> based way of doing nand I/O though.
> 
> ideally there should be a driver for the so called "high speed nand"
> interface which would allow us to nuke this one.

Hi,

I want to look into other drivers first before looking into the DMA nand
driver, it took me some time to understand how this driver works and the
controller is strange and also this driver is strange. I was wondering
how this went into mainline kernel. ;-)

I could/would send a patch which converts this from some hack to the
generic platform driver to a normal platform driver, it will probably
add ~50 lines of code, but makes it a lot easier to understand.

I will also try to make handling of the IO_ADDR_R look better with less
casts.

What else do you not like about this driver when we still use this
hardware interface?

Hauke

Re: [PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[Hauke Mehrtens]

On 06/07/2016 11:24 AM, Boris Brezillon wrote:
> Hi Hauke,
> 
> On Sun,  5 Jun 2016 23:20:03 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
>> These patches are in OpenWrt for years now and should go upstream. They 
>> are fixing some problems in the NAND driver.
> 
> You forgot to add the NAND maintainers (Richard and I). Next
> time, please use get_maintainer.pl to find the person you should send
> your patches to.
> 

Sorry, will do next time.

Hauke

Re: [PATCH 2/6] MTD: xway: fix invalid operator

[Hauke Mehrtens]

On 06/07/2016 11:28 AM, Boris Brezillon wrote:
> On Sun,  5 Jun 2016 23:20:05 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
>> From: John Crispin <john@phrozen.org>
>>
>> xway_read_byte should use a logic or and not an add operator when
>> working out the NAND address. The NAND address bits are used to
>> activate the pins to the NAND flash.
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  drivers/mtd/nand/xway_nand.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
>> index ccac19c..0ab6e83 100644
>> --- a/drivers/mtd/nand/xway_nand.c
>> +++ b/drivers/mtd/nand/xway_nand.c
>> @@ -134,7 +134,7 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
>>  	int ret;
>>  
>>  	spin_lock_irqsave(&ebu_lock, flags);
>> -	ret = ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
>> +	ret = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));
> 
> It's doing exactly the same, isn't it? What's the rationale behind this
> change?

Yes that is correct, this is only a style change.

In the other places we are also using the bool operations and this
address space is not a list of registers, but some address bits are used
to activate or deactivate some pins and all data written to this address
range is handled in the same way.

Hauke

Re: [PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[Boris Brezillon]

On Tue, 7 Jun 2016 19:36:15 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> On 06/07/2016 12:12 PM, John Crispin wrote:
> > 
> > 
> > On 07/06/2016 11:48, Boris Brezillon wrote:  
> >> On Sun,  5 Jun 2016 23:20:03 +0200
> >> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> >>  
> >>> These patches are in OpenWrt for years now and should go upstream. They 
> >>> are fixing some problems in the NAND driver.  
> >>
> >> Just had a closer look at the xway NAND controller driver, and it's
> >> just a big pile of hacks :-(. I'll take those patches if nobody is
> >> willing to maintain this driver, but honestly, I'd prefer a complete
> >> rework of the driver.
> >>  
> > 
> > Hi Boris,
> > 
> > it is indeed a horrific pile of doo doo. it has grown historically over
> > a few years and then became sort of abandoned. i have been keeping it
> > artificially alive inside openwrt as we have users with boards that have
> > nand. i dont even own a lantiq board with nand, so patches were sort of
> > merged on cruise control and with compile testing only.
> > 
> > the SoCs have 2 ways of controlling the nand core. the easy one is this,
> > which is basically nothing more than a nand flash aware 16bit
> > intel/hitachi bus interafec called EBU. There is a more advanced dma
> > based way of doing nand I/O though.
> > 
> > ideally there should be a driver for the so called "high speed nand"
> > interface which would allow us to nuke this one.  
> 
> Hi,
> 
> I want to look into other drivers first before looking into the DMA nand
> driver, it took me some time to understand how this driver works and the
> controller is strange and also this driver is strange. I was wondering
> how this went into mainline kernel. ;-)

I'm asking myself the same question, maybe the framework was not so 

> 
> I could/would send a patch which converts this from some hack to the
> generic platform driver to a normal platform driver, it will probably
> add ~50 lines of code, but makes it a lot easier to understand.

Yep, that would be a good start.

> 
> I will also try to make handling of the IO_ADDR_R look better with less
> casts.

Actually, I would get rid of ->IO_ADDR_X completely. and implement my
own set of ->read/write_buf() functions using a private __iomem pointer.

> 
> What else do you not like about this driver when we still use this
> hardware interface?

Well, the first thing would be to properly separate the controller and
chip concepts, and replacing all global variables by you own private
chip structure that would be dynamically allocated at probe time.

Also, I see that this driver is mixing platform specific initialization
[1] and driver implementation, but I guess this is part of your first
suggestion (moving to a real platform driver and getting rid of the
platform_nand abstraction layer).

Once you're there, you'll be able to add extra features, like DMA
support.

If you need an example, you can have a look at the sunxi_nand driver.

Regards,

Boris

[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/xway_nand.c#L163


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/6] MTD: xway: fix invalid operator

[Boris Brezillon]

On Tue, 7 Jun 2016 19:40:14 +0200
Hauke Mehrtens <hauke@hauke-m.de> wrote:

> On 06/07/2016 11:28 AM, Boris Brezillon wrote:
> > On Sun,  5 Jun 2016 23:20:05 +0200
> > Hauke Mehrtens <hauke@hauke-m.de> wrote:
> >   
> >> From: John Crispin <john@phrozen.org>
> >>
> >> xway_read_byte should use a logic or and not an add operator when
> >> working out the NAND address. The NAND address bits are used to
> >> activate the pins to the NAND flash.
> >>
> >> Signed-off-by: John Crispin <john@phrozen.org>
> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >> ---
> >>  drivers/mtd/nand/xway_nand.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c
> >> index ccac19c..0ab6e83 100644
> >> --- a/drivers/mtd/nand/xway_nand.c
> >> +++ b/drivers/mtd/nand/xway_nand.c
> >> @@ -134,7 +134,7 @@ static unsigned char xway_read_byte(struct mtd_info *mtd)
> >>  	int ret;
> >>  
> >>  	spin_lock_irqsave(&ebu_lock, flags);
> >> -	ret = ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA));
> >> +	ret = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA));  
> > 
> > It's doing exactly the same, isn't it? What's the rationale behind this
> > change?  
> 
> Yes that is correct, this is only a style change.
> 
> In the other places we are also using the bool operations and this
> address space is not a list of registers, but some address bits are used
> to activate or deactivate some pins and all data written to this address
> range is handled in the same way.

Well, I find it clearer to use the '+' operator when manipulating
pointer addresses, but maybe that's just a matter of taste.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 0/6] MTD: xway: updates from OpenWrt/LEDE

[Boris Brezillon]

On Tue, 7 Jun 2016 21:01:55 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Tue, 7 Jun 2016 19:36:15 +0200
> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
> > On 06/07/2016 12:12 PM, John Crispin wrote:  
> > > 
> > > 
> > > On 07/06/2016 11:48, Boris Brezillon wrote:    
> > >> On Sun,  5 Jun 2016 23:20:03 +0200
> > >> Hauke Mehrtens <hauke@hauke-m.de> wrote:
> > >>    
> > >>> These patches are in OpenWrt for years now and should go upstream. They 
> > >>> are fixing some problems in the NAND driver.    
> > >>
> > >> Just had a closer look at the xway NAND controller driver, and it's
> > >> just a big pile of hacks :-(. I'll take those patches if nobody is
> > >> willing to maintain this driver, but honestly, I'd prefer a complete
> > >> rework of the driver.
> > >>    
> > > 
> > > Hi Boris,
> > > 
> > > it is indeed a horrific pile of doo doo. it has grown historically over
> > > a few years and then became sort of abandoned. i have been keeping it
> > > artificially alive inside openwrt as we have users with boards that have
> > > nand. i dont even own a lantiq board with nand, so patches were sort of
> > > merged on cruise control and with compile testing only.
> > > 
> > > the SoCs have 2 ways of controlling the nand core. the easy one is this,
> > > which is basically nothing more than a nand flash aware 16bit
> > > intel/hitachi bus interafec called EBU. There is a more advanced dma
> > > based way of doing nand I/O though.
> > > 
> > > ideally there should be a driver for the so called "high speed nand"
> > > interface which would allow us to nuke this one.    
> > 
> > Hi,
> > 
> > I want to look into other drivers first before looking into the DMA nand
> > driver, it took me some time to understand how this driver works and the
> > controller is strange and also this driver is strange. I was wondering
> > how this went into mainline kernel. ;-)  
> 
> I'm asking myself the same question, maybe the framework was not so 
> 
> > 
> > I could/would send a patch which converts this from some hack to the
> > generic platform driver to a normal platform driver, it will probably
> > add ~50 lines of code, but makes it a lot easier to understand.  
> 
> Yep, that would be a good start.
> 
> > 
> > I will also try to make handling of the IO_ADDR_R look better with less
> > casts.  
> 
> Actually, I would get rid of ->IO_ADDR_X completely. and implement my
> own set of ->read/write_buf() functions using a private __iomem pointer.
> 
> > 
> > What else do you not like about this driver when we still use this
> > hardware interface?  
> 
> Well, the first thing would be to properly separate the controller and
> chip concepts, and replacing all global variables by you own private
> chip structure that would be dynamically allocated at probe time.
> 
> Also, I see that this driver is mixing platform specific initialization
> [1] and driver implementation, but I guess this is part of your first
> suggestion (moving to a real platform driver and getting rid of the
> platform_nand abstraction layer).
> 
> Once you're there, you'll be able to add extra features, like DMA
> support.
> 
> If you need an example, you can have a look at the sunxi_nand driver.
> 

BTW, thanks for volunteering for this rework. Let me know if you need
help.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com