[PATCH v5 0/4] mtd: nand: jz4780: Add NAND and BCH drivers

[PATCH v5 0/4] mtd: nand: jz4780: Add NAND and BCH drivers

[Alex Smith]

Hi,

This series adds support for the BCH controller and NAND devices on
the Ingenic JZ4780 SoC.

Tested on the MIPS Creator Ci20 board. All dependencies are now in
mainline.

This version of the series is based on current mainline (pre 4.3-rc1).

Review and feedback welcome.

Thanks,
Alex

Alex Smith (4):
  mtd: nand: increase ready wait timeout and report timeouts
  dt-bindings: binding for jz4780-{nand,bch}
  mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs
  MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

 .../bindings/mtd/ingenic,jz4780-nand.txt           |  57 ++++
 arch/mips/boot/dts/ingenic/ci20.dts                |  51 +++
 arch/mips/boot/dts/ingenic/jz4780.dtsi             |  26 ++
 drivers/mtd/nand/Kconfig                           |   7 +
 drivers/mtd/nand/Makefile                          |   1 +
 drivers/mtd/nand/jz4780_bch.c                      | 348 +++++++++++++++++++
 drivers/mtd/nand/jz4780_bch.h                      |  42 +++
 drivers/mtd/nand/jz4780_nand.c                     | 378 +++++++++++++++++++++
 drivers/mtd/nand/nand_base.c                       |  14 +-
 9 files changed, 921 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
 create mode 100644 drivers/mtd/nand/jz4780_bch.c
 create mode 100644 drivers/mtd/nand/jz4780_bch.h
 create mode 100644 drivers/mtd/nand/jz4780_nand.c

-- 
2.5.0

[PATCH v5 1/4] mtd: nand: increase ready wait timeout and report timeouts

[Alex Smith]

If nand_wait_ready() times out, this is silently ignored, and its
caller will then proceed to read from/write to the chip before it is
ready. This can potentially result in corruption with no indication as
to why.

While a 20ms timeout seems like it should be plenty enough, certain
behaviour can cause it to timeout much earlier than expected. The
situation which prompted this change was that CPU 0, which is
responsible for updating jiffies, was holding interrupts disabled
for a fairly long time while writing to the console during a printk,
causing several jiffies updates to be delayed. If CPU 1 happens to
enter the timeout loop in nand_wait_ready() just before CPU 0 re-
enables interrupts and updates jiffies, CPU 1 will immediately time
out when the delayed jiffies updates are made. The result of this is
that nand_wait_ready() actually waits less time than the NAND chip
would normally take to be ready, and then read_page() proceeds to
read out bad data from the chip.

The situation described above may seem unlikely, but in fact it can be
reproduced almost every boot on the MIPS Creator Ci20.

Debugging this was made more difficult by the misleading comment above
nand_wait_ready() stating "The timeout is caught later" - no timeout
was ever reported, leading me away from the real source of the problem.

Therefore, this patch increases the timeout to 200ms. This should be
enough to cover cases where jiffies updates get delayed. Additionally,
add a pr_warn() when a timeout does occur so that it is easier to
pinpoint any problems in future caused by the chip not becoming ready.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: linux-mtd@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
v4 -> v5:
 - Remove spurious change.
 - Add Ezequiel's Reviewed-by.

v3 -> v4:
 - New patch to fix issue encountered in external Ci20 3.18 kernel
   branch which also applies upstream.
---
 drivers/mtd/nand/nand_base.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ceb68ca8277a..07b831b94e5c 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -543,11 +543,16 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
 	}
 }
 
-/* Wait for the ready pin, after a command. The timeout is caught later. */
+/**
+ * nand_wait_ready - [GENERIC] Wait for the ready pin after commands.
+ * @mtd: MTD device structure
+ *
+ * Wait for the ready pin after a command, and warn if a timeout occurs.
+ */
 void nand_wait_ready(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
-	unsigned long timeo = jiffies + msecs_to_jiffies(20);
+	unsigned long timeo = jiffies + msecs_to_jiffies(200);
 
 	/* 400ms timeout */
 	if (in_interrupt() || oops_in_progress)
@@ -557,9 +562,12 @@ void nand_wait_ready(struct mtd_info *mtd)
 	/* Wait until command is processed or timeout occurs */
 	do {
 		if (chip->dev_ready(mtd))
-			break;
+			goto out;
 		touch_softlockup_watchdog();
 	} while (time_before(jiffies, timeo));
+
+	pr_warn("timeout while waiting for chip to become ready\n");
+out:
 	led_trigger_event(nand_led_trigger, LED_OFF);
 }
 EXPORT_SYMBOL_GPL(nand_wait_ready);
-- 
2.5.0

Re: [PATCH v5 1/4] mtd: nand: increase ready wait timeout and report timeouts

[Brian Norris]

+ Niklas

On Tue, Sep 08, 2015 at 10:10:50AM +0100, Alex Smith wrote:
> If nand_wait_ready() times out, this is silently ignored, and its
> caller will then proceed to read from/write to the chip before it is
> ready. This can potentially result in corruption with no indication as
> to why.
> 
> While a 20ms timeout seems like it should be plenty enough, certain
> behaviour can cause it to timeout much earlier than expected. The
> situation which prompted this change was that CPU 0, which is
> responsible for updating jiffies, was holding interrupts disabled
> for a fairly long time while writing to the console during a printk,
> causing several jiffies updates to be delayed. If CPU 1 happens to
> enter the timeout loop in nand_wait_ready() just before CPU 0 re-
> enables interrupts and updates jiffies, CPU 1 will immediately time
> out when the delayed jiffies updates are made. The result of this is
> that nand_wait_ready() actually waits less time than the NAND chip
> would normally take to be ready, and then read_page() proceeds to
> read out bad data from the chip.
> 
> The situation described above may seem unlikely, but in fact it can be
> reproduced almost every boot on the MIPS Creator Ci20.
> 
> Debugging this was made more difficult by the misleading comment above
> nand_wait_ready() stating "The timeout is caught later" - no timeout
> was ever reported, leading me away from the real source of the problem.
> 
> Therefore, this patch increases the timeout to 200ms. This should be
> enough to cover cases where jiffies updates get delayed. Additionally,
> add a pr_warn() when a timeout does occur so that it is easier to
> pinpoint any problems in future caused by the chip not becoming ready.

Did you examine other solutions? I've seen patches for hrtimer support
previously:

http://patchwork.ozlabs.org/patch/160333/
http://patchwork.ozlabs.org/patch/431066/

A few things have been cleaned up since then, so some of the initial
objections to the hrtimer patch don't make sense anymore, I believe.

Anyway, I think just increasing the timeout looks OK to me (as long as
we never have a 200ms jiffies jump... can this happen??), so hrtimer may
be over-engineering. I just want to make sure both options have been
considered before officially choosing one over the other.

Brian

> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
> v4 -> v5:
>  - Remove spurious change.
>  - Add Ezequiel's Reviewed-by.
> 
> v3 -> v4:
>  - New patch to fix issue encountered in external Ci20 3.18 kernel
>    branch which also applies upstream.
> ---
>  drivers/mtd/nand/nand_base.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index ceb68ca8277a..07b831b94e5c 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -543,11 +543,16 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
>  	}
>  }
>  
> -/* Wait for the ready pin, after a command. The timeout is caught later. */
> +/**
> + * nand_wait_ready - [GENERIC] Wait for the ready pin after commands.
> + * @mtd: MTD device structure
> + *
> + * Wait for the ready pin after a command, and warn if a timeout occurs.
> + */
>  void nand_wait_ready(struct mtd_info *mtd)
>  {
>  	struct nand_chip *chip = mtd->priv;
> -	unsigned long timeo = jiffies + msecs_to_jiffies(20);
> +	unsigned long timeo = jiffies + msecs_to_jiffies(200);
>  
>  	/* 400ms timeout */
>  	if (in_interrupt() || oops_in_progress)
> @@ -557,9 +562,12 @@ void nand_wait_ready(struct mtd_info *mtd)
>  	/* Wait until command is processed or timeout occurs */
>  	do {
>  		if (chip->dev_ready(mtd))
> -			break;
> +			goto out;
>  		touch_softlockup_watchdog();
>  	} while (time_before(jiffies, timeo));
> +
> +	pr_warn("timeout while waiting for chip to become ready\n");
> +out:
>  	led_trigger_event(nand_led_trigger, LED_OFF);
>  }
>  EXPORT_SYMBOL_GPL(nand_wait_ready);
> -- 
> 2.5.0
> 

Re: [PATCH v5 1/4] mtd: nand: increase ready wait timeout and report timeouts

[Niklas Cassel]

On 09/10/2015 01:49 AM, Brian Norris wrote:
> + Niklas
> 
> On Tue, Sep 08, 2015 at 10:10:50AM +0100, Alex Smith wrote:
>> If nand_wait_ready() times out, this is silently ignored, and its
>> caller will then proceed to read from/write to the chip before it is
>> ready. This can potentially result in corruption with no indication as
>> to why.
>>
>> While a 20ms timeout seems like it should be plenty enough, certain
>> behaviour can cause it to timeout much earlier than expected. The
>> situation which prompted this change was that CPU 0, which is
>> responsible for updating jiffies, was holding interrupts disabled
>> for a fairly long time while writing to the console during a printk,
>> causing several jiffies updates to be delayed. If CPU 1 happens to
>> enter the timeout loop in nand_wait_ready() just before CPU 0 re-
>> enables interrupts and updates jiffies, CPU 1 will immediately time
>> out when the delayed jiffies updates are made. The result of this is
>> that nand_wait_ready() actually waits less time than the NAND chip
>> would normally take to be ready, and then read_page() proceeds to
>> read out bad data from the chip.
>>
>> The situation described above may seem unlikely, but in fact it can be
>> reproduced almost every boot on the MIPS Creator Ci20.
>>
>> Debugging this was made more difficult by the misleading comment above
>> nand_wait_ready() stating "The timeout is caught later" - no timeout
>> was ever reported, leading me away from the real source of the problem.
>>
>> Therefore, this patch increases the timeout to 200ms. This should be
>> enough to cover cases where jiffies updates get delayed. Additionally,
>> add a pr_warn() when a timeout does occur so that it is easier to
>> pinpoint any problems in future caused by the chip not becoming ready.
> 
> Did you examine other solutions? I've seen patches for hrtimer support
> previously:
> 
> http://patchwork.ozlabs.org/patch/160333/
> http://patchwork.ozlabs.org/patch/431066/
> 
> A few things have been cleaned up since then, so some of the initial
> objections to the hrtimer patch don't make sense anymore, I believe.
> 
> Anyway, I think just increasing the timeout looks OK to me (as long as
> we never have a 200ms jiffies jump... can this happen??), so hrtimer may
> be over-engineering. I just want to make sure both options have been
> considered before officially choosing one over the other.
> 
> Brian
> 
>> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Brian Norris <computersforpeace@gmail.com>
>> Cc: linux-mtd@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> v4 -> v5:
>>  - Remove spurious change.
>>  - Add Ezequiel's Reviewed-by.
>>
>> v3 -> v4:
>>  - New patch to fix issue encountered in external Ci20 3.18 kernel
>>    branch which also applies upstream.
>> ---
>>  drivers/mtd/nand/nand_base.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index ceb68ca8277a..07b831b94e5c 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -543,11 +543,16 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
>>  	}
>>  }
>>  
>> -/* Wait for the ready pin, after a command. The timeout is caught later. */
>> +/**
>> + * nand_wait_ready - [GENERIC] Wait for the ready pin after commands.
>> + * @mtd: MTD device structure
>> + *
>> + * Wait for the ready pin after a command, and warn if a timeout occurs.
>> + */
>>  void nand_wait_ready(struct mtd_info *mtd)
>>  {
>>  	struct nand_chip *chip = mtd->priv;
>> -	unsigned long timeo = jiffies + msecs_to_jiffies(20);
>> +	unsigned long timeo = jiffies + msecs_to_jiffies(200);
Sample the jiffies value just before the do-while loop, just like in nand_wait()
and nand_wait_status_ready().

>>  
>>  	/* 400ms timeout */
The timeout is 400 ms when called from interrupt context, and 200 ms
when called from non-interrupt context. The timeout value should not depend on us being in
irq context or not. (This is also how it works for nand_wait())

Since the timeout is only for worst case, it should not affect best case, so use
400 ms for both contexts. Change the timeout to always be 400 ms also in nand_wait().

nand_wait_ready() and nand_wait_status_ready() uses a do-while loop,
nand_wait() uses a normal while loop.
For consistency, change nand_wait() to also use a do-while loop.

>>  	if (in_interrupt() || oops_in_progress)
>> @@ -557,9 +562,12 @@ void nand_wait_ready(struct mtd_info *mtd)
>>  	/* Wait until command is processed or timeout occurs */
>>  	do {
>>  		if (chip->dev_ready(mtd))
>> -			break;
>> +			goto out;
>>  		touch_softlockup_watchdog();
To not lock the cpu, especially on non-preempt kernels, use cond_resched instead of
touch_softlockup_watchdog, just like nand_wait() does.

>>  	} while (time_before(jiffies, timeo));
>> +
>> +	pr_warn("timeout while waiting for chip to become ready\n");
Rate limit the printk.

>> +out:
>>  	led_trigger_event(nand_led_trigger, LED_OFF);
>>  }
>>  EXPORT_SYMBOL_GPL(nand_wait_ready);
>> -- 
>> 2.5.0
>>


The patch would then look something like this:

--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -544,23 +544,32 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
 	}
 }
 
-/* Wait for the ready pin, after a command. The timeout is caught later. */
+/**
+ * nand_wait_ready - [GENERIC] Wait for the ready pin after commands.
+ * @mtd: MTD device structure
+ *
+ * Wait for the ready pin after a command, and warn if a timeout occurs.
+ */
 void nand_wait_ready(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
-	unsigned long timeo = jiffies + msecs_to_jiffies(20);
+	unsigned long timeo = 400;
 
 	/* 400ms timeout */
 	if (in_interrupt() || oops_in_progress)
-		return panic_nand_wait_ready(mtd, 400);
+		return panic_nand_wait_ready(mtd, timeo);
 
 	led_trigger_event(nand_led_trigger, LED_FULL);
 	/* Wait until command is processed or timeout occurs */
+	timeo = jiffies + msecs_to_jiffies(timeo);
 	do {
 		if (chip->dev_ready(mtd))
-			break;
-		touch_softlockup_watchdog();
+			goto out;
+		cond_resched();
 	} while (time_before(jiffies, timeo));
+
+	pr_warn_ratelimited("timeout while waiting for chip to become ready\n");
+out:
 	led_trigger_event(nand_led_trigger, LED_OFF);
 }
 EXPORT_SYMBOL_GPL(nand_wait_ready);
@@ -886,15 +895,12 @@ static void panic_nand_wait(struct mtd_info *mtd, struct nand_chip *chip,
  * @mtd: MTD device structure
  * @chip: NAND chip structure
  *
- * Wait for command done. This applies to erase and program only. Erase can
- * take up to 400ms and program up to 20ms according to general NAND and
- * SmartMedia specs.
+ * Wait for command done. This applies to erase and program only.
  */
 static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 {
-
-	int status, state = chip->state;
-	unsigned long timeo = (state == FL_ERASING ? 400 : 20);
+	int status;
+	unsigned long timeo = 400;
 
 	led_trigger_event(nand_led_trigger, LED_FULL);
 
@@ -910,7 +916,7 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 		panic_nand_wait(mtd, chip, timeo);
 	else {
 		timeo = jiffies + msecs_to_jiffies(timeo);
-		while (time_before(jiffies, timeo)) {
+		do {
 			if (chip->dev_ready) {
 				if (chip->dev_ready(mtd))
 					break;
@@ -919,7 +925,7 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 					break;
 			}
 			cond_resched();
-		}
+		} while (time_before(jiffies, timeo));
 	}
 	led_trigger_event(nand_led_trigger, LED_OFF);
 
-- 

Tested locally and works just as good as the hrtimer patch that I sent out a couple of months ago.

Re: [PATCH v5 1/4] mtd: nand: increase ready wait timeout and report timeouts

[Alex Smith]

On 10 September 2015 at 00:49, Brian Norris <computersforpeace@gmail.com> wrote:
> + Niklas
>
> On Tue, Sep 08, 2015 at 10:10:50AM +0100, Alex Smith wrote:
>> If nand_wait_ready() times out, this is silently ignored, and its
>> caller will then proceed to read from/write to the chip before it is
>> ready. This can potentially result in corruption with no indication as
>> to why.
>>
>> While a 20ms timeout seems like it should be plenty enough, certain
>> behaviour can cause it to timeout much earlier than expected. The
>> situation which prompted this change was that CPU 0, which is
>> responsible for updating jiffies, was holding interrupts disabled
>> for a fairly long time while writing to the console during a printk,
>> causing several jiffies updates to be delayed. If CPU 1 happens to
>> enter the timeout loop in nand_wait_ready() just before CPU 0 re-
>> enables interrupts and updates jiffies, CPU 1 will immediately time
>> out when the delayed jiffies updates are made. The result of this is
>> that nand_wait_ready() actually waits less time than the NAND chip
>> would normally take to be ready, and then read_page() proceeds to
>> read out bad data from the chip.
>>
>> The situation described above may seem unlikely, but in fact it can be
>> reproduced almost every boot on the MIPS Creator Ci20.
>>
>> Debugging this was made more difficult by the misleading comment above
>> nand_wait_ready() stating "The timeout is caught later" - no timeout
>> was ever reported, leading me away from the real source of the problem.
>>
>> Therefore, this patch increases the timeout to 200ms. This should be
>> enough to cover cases where jiffies updates get delayed. Additionally,
>> add a pr_warn() when a timeout does occur so that it is easier to
>> pinpoint any problems in future caused by the chip not becoming ready.
>
> Did you examine other solutions? I've seen patches for hrtimer support
> previously:
>
> http://patchwork.ozlabs.org/patch/160333/
> http://patchwork.ozlabs.org/patch/431066/
>
> A few things have been cleaned up since then, so some of the initial
> objections to the hrtimer patch don't make sense anymore, I believe.
>
> Anyway, I think just increasing the timeout looks OK to me (as long as
> we never have a 200ms jiffies jump... can this happen??), so hrtimer may
> be over-engineering. I just want to make sure both options have been
> considered before officially choosing one over the other.
>
> Brian

Hi Brian, Niklas,

I'm no expert in the matter but I feel like using a hrtimer here would
indeed be over-engineering and could potentially add overhead to the
"normal" case where the chip becomes ready well before the timeout
expires? Just increasing the timeout seems like a simpler solution
that solves the problem. I think that a jiffies jump of a few hundred
milliseconds is extremely unlikely and would indicate something else
that needs to be fixed (i.e. in the SMP case I had it would mean that
the CPU which is supposed to update jiffies has interrupts disabled
for hundreds of milliseconds).

Niklas: If I update the patch based on your suggestions would you be
happy to go with that rather than your hrtimer patch?

Thanks,
Alex

>
>> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Brian Norris <computersforpeace@gmail.com>
>> Cc: linux-mtd@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> v4 -> v5:
>>  - Remove spurious change.
>>  - Add Ezequiel's Reviewed-by.
>>
>> v3 -> v4:
>>  - New patch to fix issue encountered in external Ci20 3.18 kernel
>>    branch which also applies upstream.
>> ---
>>  drivers/mtd/nand/nand_base.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index ceb68ca8277a..07b831b94e5c 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -543,11 +543,16 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
>>       }
>>  }
>>
>> -/* Wait for the ready pin, after a command. The timeout is caught later. */
>> +/**
>> + * nand_wait_ready - [GENERIC] Wait for the ready pin after commands.
>> + * @mtd: MTD device structure
>> + *
>> + * Wait for the ready pin after a command, and warn if a timeout occurs.
>> + */
>>  void nand_wait_ready(struct mtd_info *mtd)
>>  {
>>       struct nand_chip *chip = mtd->priv;
>> -     unsigned long timeo = jiffies + msecs_to_jiffies(20);
>> +     unsigned long timeo = jiffies + msecs_to_jiffies(200);
>>
>>       /* 400ms timeout */
>>       if (in_interrupt() || oops_in_progress)
>> @@ -557,9 +562,12 @@ void nand_wait_ready(struct mtd_info *mtd)
>>       /* Wait until command is processed or timeout occurs */
>>       do {
>>               if (chip->dev_ready(mtd))
>> -                     break;
>> +                     goto out;
>>               touch_softlockup_watchdog();
>>       } while (time_before(jiffies, timeo));
>> +
>> +     pr_warn("timeout while waiting for chip to become ready\n");
>> +out:
>>       led_trigger_event(nand_led_trigger, LED_OFF);
>>  }
>>  EXPORT_SYMBOL_GPL(nand_wait_ready);
>> --
>> 2.5.0
>>

Re: [PATCH v5 1/4] mtd: nand: increase ready wait timeout and report timeouts

[Niklas Cassel]

On 09/15/2015 11:38 AM, Alex Smith wrote:
> On 10 September 2015 at 00:49, Brian Norris <computersforpeace@gmail.com> wrote:
>> + Niklas
>>
>> On Tue, Sep 08, 2015 at 10:10:50AM +0100, Alex Smith wrote:
>>> If nand_wait_ready() times out, this is silently ignored, and its
>>> caller will then proceed to read from/write to the chip before it is
>>> ready. This can potentially result in corruption with no indication as
>>> to why.
>>>
>>> While a 20ms timeout seems like it should be plenty enough, certain
>>> behaviour can cause it to timeout much earlier than expected. The
>>> situation which prompted this change was that CPU 0, which is
>>> responsible for updating jiffies, was holding interrupts disabled
>>> for a fairly long time while writing to the console during a printk,
>>> causing several jiffies updates to be delayed. If CPU 1 happens to
>>> enter the timeout loop in nand_wait_ready() just before CPU 0 re-
>>> enables interrupts and updates jiffies, CPU 1 will immediately time
>>> out when the delayed jiffies updates are made. The result of this is
>>> that nand_wait_ready() actually waits less time than the NAND chip
>>> would normally take to be ready, and then read_page() proceeds to
>>> read out bad data from the chip.
>>>
>>> The situation described above may seem unlikely, but in fact it can be
>>> reproduced almost every boot on the MIPS Creator Ci20.
>>>
>>> Debugging this was made more difficult by the misleading comment above
>>> nand_wait_ready() stating "The timeout is caught later" - no timeout
>>> was ever reported, leading me away from the real source of the problem.
>>>
>>> Therefore, this patch increases the timeout to 200ms. This should be
>>> enough to cover cases where jiffies updates get delayed. Additionally,
>>> add a pr_warn() when a timeout does occur so that it is easier to
>>> pinpoint any problems in future caused by the chip not becoming ready.
>>
>> Did you examine other solutions? I've seen patches for hrtimer support
>> previously:
>>
>> http://patchwork.ozlabs.org/patch/160333/
>> http://patchwork.ozlabs.org/patch/431066/
>>
>> A few things have been cleaned up since then, so some of the initial
>> objections to the hrtimer patch don't make sense anymore, I believe.
>>
>> Anyway, I think just increasing the timeout looks OK to me (as long as
>> we never have a 200ms jiffies jump... can this happen??), so hrtimer may
>> be over-engineering. I just want to make sure both options have been
>> considered before officially choosing one over the other.
>>
>> Brian
> 
> Hi Brian, Niklas,
> 
> I'm no expert in the matter but I feel like using a hrtimer here would
> indeed be over-engineering and could potentially add overhead to the
> "normal" case where the chip becomes ready well before the timeout
> expires? Just increasing the timeout seems like a simpler solution
> that solves the problem. I think that a jiffies jump of a few hundred
> milliseconds is extremely unlikely and would indicate something else
> that needs to be fixed (i.e. in the SMP case I had it would mean that
> the CPU which is supposed to update jiffies has interrupts disabled
> for hundreds of milliseconds).
> 
> Niklas: If I update the patch based on your suggestions would you be
> happy to go with that rather than your hrtimer patch?

Yes.

I've tested the patch inlined in the end of
http://marc.info/?l=linux-kernel&m=144197105326420
and it works just as good as the hrtimer patch that I sent out a couple of months ago.

(For our use-case where irqs were sometimes disabled for more than 20 ms.)

[PATCH v5 2/4] dt-bindings: binding for jz4780-{nand,bch}

[Alex Smith]

Add DT bindings for NAND devices connected to the NEMC on JZ4780 SoCs,
as well as the hardware BCH controller, used by the jz4780_{nand,bch}
drivers.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: linux-mtd@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
v4 -> v5:
 - Rename ingenic,bch-device to ingenic,bch-controller to fit with
   existing convention.

v3 -> v4:
 - No change

v2 -> v3:
 - Rebase to 4.0-rc6
 - Changed ingenic,ecc-size to common nand-ecc-step-size
 - Changed ingenic,ecc-strength to common nand-ecc-strength
 - Changed ingenic,busy-gpio to common rb-gpios
 - Changed ingenic,wp-gpio to common wp-gpios

v1 -> v2:
 - Rebase to 4.0-rc3
---
 .../bindings/mtd/ingenic,jz4780-nand.txt           | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
new file mode 100644
index 000000000000..568e502f6448
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
@@ -0,0 +1,57 @@
+* Ingenic JZ4780 NAND/BCH
+
+This file documents the device tree bindings for NAND flash devices on the
+JZ4780. NAND devices are connected to the NEMC controller (described in
+memory-controllers/ingenic,jz4780-nemc.txt), and thus NAND device nodes must
+be children of the NEMC node.
+
+Required NAND device properties:
+- compatible: Should be set to "ingenic,jz4780-nand".
+- reg: For each bank with a NAND chip attached, should specify a bank number,
+  an offset of 0 and a size of 0x1000000 (i.e. the whole NEMC bank).
+
+Optional NAND device properties:
+- ingenic,bch-controller: To make use of the hardware BCH controller, this
+  property must contain a phandle for the BCH controller node. The required
+  properties for this node are described below. If this is not specified,
+  software BCH will be used instead.
+- nand-ecc-step-size: ECC block size in bytes.
+- nand-ecc-strength: ECC strength (max number of correctable bits).
+- rb-gpios: GPIO specifier for the busy pin.
+- wp-gpios: GPIO specifier for the write protect pin.
+
+Example:
+
+nemc: nemc@13410000 {
+	...
+
+	nand: nand@1 {
+		compatible = "ingenic,jz4780-nand";
+		reg = <1 0 0x1000000>;	/* Bank 1 */
+
+		ingenic,bch-controller = <&bch>;
+		nand-ecc-step-size = <1024>;
+		nand-ecc-strength = <24>;
+
+		rb-gpios = <&gpa 20 GPIO_ACTIVE_LOW>;
+		wp-gpios = <&gpf 22 GPIO_ACTIVE_LOW>;
+	};
+};
+
+The BCH controller is a separate SoC component used for error correction on
+NAND devices. The following is a description of the device properties for a
+BCH controller.
+
+Required BCH properties:
+- compatible: Should be set to "ingenic,jz4780-bch".
+- reg: Should specify the BCH controller registers location and length.
+- clocks: Clock for the BCH controller.
+
+Example:
+
+bch: bch@134d0000 {
+	compatible = "ingenic,jz4780-bch";
+	reg = <0x134d0000 0x10000>;
+
+	clocks = <&cgu JZ4780_CLK_BCH>;
+};
-- 
2.5.0

[PATCH v5 3/4] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Alex Smith]

Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
well as the hardware BCH controller. DMA is not currently implemented.

While older 47xx SoCs also have a BCH controller, they are incompatible
with the one in the 4780 due to differing register/bit positions, which
would make implementing a common driver for them quite messy.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: linux-mtd@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
v4 -> v5:
 - Bump RB_DELAY up to be sufficient for the driver to work without a
   busy GPIO available.
 - Use readl_poll_timeout instead of custom polling loop.
 - Remove useless printks.
 - Change a BUG_ON to WARN_ON.
 - Remove use of of_translate_address(), use standard platform resource
   APIs.
 - Add DRV_NAME define to avoid duplication of the same string.

v3 -> v4:
 - Rebase to 4.2-rc4
 - Change ECC_HW_OOB_FIRST to ECC_HW, reading OOB first is not necessary.
 - Fix argument to get_device() in jz4780_bch_get()

v2 -> v3:
 - Rebase to 4.0-rc6
 - Reflect binding changes
 - get/put_device in bch get/release
 - Removed empty .remove() callback
 - Removed .owner
 - Set mtd->dev.parent

v1 -> v2:
 - Fixed module license macro
 - Rebase to 4.0-rc3
---
 drivers/mtd/nand/Kconfig       |   7 +
 drivers/mtd/nand/Makefile      |   1 +
 drivers/mtd/nand/jz4780_bch.c  | 348 +++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/jz4780_bch.h  |  42 +++++
 drivers/mtd/nand/jz4780_nand.c | 378 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 776 insertions(+)
 create mode 100644 drivers/mtd/nand/jz4780_bch.c
 create mode 100644 drivers/mtd/nand/jz4780_bch.h
 create mode 100644 drivers/mtd/nand/jz4780_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 3324281d1f53..8ffd5cd2a92b 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -508,6 +508,13 @@ config MTD_NAND_JZ4740
 	help
 		Enables support for NAND Flash on JZ4740 SoC based boards.
 
+config MTD_NAND_JZ4780
+	tristate "Support for NAND on JZ4780 SoC"
+	depends on MACH_JZ4780 && JZ4780_NEMC
+	help
+	  Enables support for NAND Flash connected to the NEMC on JZ4780 SoC
+	  based boards, using the BCH controller for hardware error correction.
+
 config MTD_NAND_FSMC
 	tristate "Support for NAND on ST Micros FSMC"
 	depends on PLAT_SPEAR || ARCH_NOMADIK || ARCH_U8500 || MACH_U300
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 1f897ec3c242..3ee2fd6bc641 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_MTD_NAND_NUC900)		+= nuc900_nand.o
 obj-$(CONFIG_MTD_NAND_MPC5121_NFC)	+= mpc5121_nfc.o
 obj-$(CONFIG_MTD_NAND_RICOH)		+= r852.o
 obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
+obj-$(CONFIG_MTD_NAND_JZ4780)		+= jz4780_nand.o jz4780_bch.o
 obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
 obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
 obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
diff --git a/drivers/mtd/nand/jz4780_bch.c b/drivers/mtd/nand/jz4780_bch.c
new file mode 100644
index 000000000000..6ed4ad024e83
--- /dev/null
+++ b/drivers/mtd/nand/jz4780_bch.c
@@ -0,0 +1,348 @@
+/*
+ * JZ4780 BCH controller
+ *
+ * Copyright (c) 2015 Imagination Technologies
+ * Author: Alex Smith <alex.smith@imgtec.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+#include "jz4780_bch.h"
+
+#define BCH_BHCR			0x0
+#define BCH_BHCCR			0x8
+#define BCH_BHCNT			0xc
+#define BCH_BHDR			0x10
+#define BCH_BHPAR0			0x14
+#define BCH_BHERR0			0x84
+#define BCH_BHINT			0x184
+#define BCH_BHINTES			0x188
+#define BCH_BHINTEC			0x18c
+#define BCH_BHINTE			0x190
+
+#define BCH_BHCR_BSEL_SHIFT		4
+#define BCH_BHCR_BSEL_MASK		(0x7f << BCH_BHCR_BSEL_SHIFT)
+#define BCH_BHCR_ENCE			BIT(2)
+#define BCH_BHCR_INIT			BIT(1)
+#define BCH_BHCR_BCHE			BIT(0)
+
+#define BCH_BHCNT_PARITYSIZE_SHIFT	16
+#define BCH_BHCNT_PARITYSIZE_MASK	(0x7f << BCH_BHCNT_PARITYSIZE_SHIFT)
+#define BCH_BHCNT_BLOCKSIZE_SHIFT	0
+#define BCH_BHCNT_BLOCKSIZE_MASK	(0x7ff << BCH_BHCNT_BLOCKSIZE_SHIFT)
+
+#define BCH_BHERR_MASK_SHIFT		16
+#define BCH_BHERR_MASK_MASK		(0xffff << BCH_BHERR_MASK_SHIFT)
+#define BCH_BHERR_INDEX_SHIFT		0
+#define BCH_BHERR_INDEX_MASK		(0x7ff << BCH_BHERR_INDEX_SHIFT)
+
+#define BCH_BHINT_ERRC_SHIFT		24
+#define BCH_BHINT_ERRC_MASK		(0x7f << BCH_BHINT_ERRC_SHIFT)
+#define BCH_BHINT_TERRC_SHIFT		16
+#define BCH_BHINT_TERRC_MASK		(0x7f << BCH_BHINT_TERRC_SHIFT)
+#define BCH_BHINT_DECF			BIT(3)
+#define BCH_BHINT_ENCF			BIT(2)
+#define BCH_BHINT_UNCOR			BIT(1)
+#define BCH_BHINT_ERR			BIT(0)
+
+#define BCH_CLK_RATE			(200 * 1000 * 1000)
+
+/* Timeout for BCH calculation/correction in microseconds. */
+#define BCH_TIMEOUT			100000
+
+struct jz4780_bch {
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static void jz4780_bch_init(struct jz4780_bch *bch,
+			    struct jz4780_bch_params *params, bool encode)
+{
+	uint32_t reg;
+
+	/* Clear interrupt status. */
+	writel(readl(bch->base + BCH_BHINT), bch->base + BCH_BHINT);
+
+	/* Set up BCH count register. */
+	reg = params->size << BCH_BHCNT_BLOCKSIZE_SHIFT;
+	reg |= params->bytes << BCH_BHCNT_PARITYSIZE_SHIFT;
+	writel(reg, bch->base + BCH_BHCNT);
+
+	/* Initialise and enable BCH. */
+	reg = BCH_BHCR_BCHE | BCH_BHCR_INIT;
+	reg |= params->strength << BCH_BHCR_BSEL_SHIFT;
+	if (encode)
+		reg |= BCH_BHCR_ENCE;
+	writel(reg, bch->base + BCH_BHCR);
+}
+
+static void jz4780_bch_disable(struct jz4780_bch *bch)
+{
+	writel(readl(bch->base + BCH_BHINT), bch->base + BCH_BHINT);
+	writel(BCH_BHCR_BCHE, bch->base + BCH_BHCCR);
+}
+
+static void jz4780_bch_write_data(struct jz4780_bch *bch, const void *buf,
+				  size_t size)
+{
+	size_t size32 = size / sizeof(uint32_t);
+	size_t size8 = size & (sizeof(uint32_t) - 1);
+	const uint32_t *src32;
+	const uint8_t *src8;
+
+	src32 = (const uint32_t *)buf;
+	while (size32--)
+		writel(*src32++, bch->base + BCH_BHDR);
+
+	src8 = (const uint8_t *)src32;
+	while (size8--)
+		writeb(*src8++, bch->base + BCH_BHDR);
+}
+
+static void jz4780_bch_read_parity(struct jz4780_bch *bch, void *buf,
+				   size_t size)
+{
+	size_t size32 = size / sizeof(uint32_t);
+	size_t size8 = size & (sizeof(uint32_t) - 1);
+	uint32_t *dest32;
+	uint8_t *dest8;
+	uint32_t val, offset = 0;
+
+	dest32 = (uint32_t *)buf;
+	while (size32--) {
+		*dest32++ = readl(bch->base + BCH_BHPAR0 + offset);
+		offset += sizeof(uint32_t);
+	}
+
+	dest8 = (uint8_t *)dest32;
+	val = readl(bch->base + BCH_BHPAR0 + offset);
+	switch (size8) {
+	case 3:
+		dest8[2] = (val >> 16) & 0xff;
+	case 2:
+		dest8[1] = (val >> 8) & 0xff;
+	case 1:
+		dest8[0] = val & 0xff;
+		break;
+	}
+}
+
+static bool jz4780_bch_wait_complete(struct jz4780_bch *bch, unsigned int irq,
+				     uint32_t *status)
+{
+	uint32_t reg;
+	int ret;
+
+	/*
+	 * While we could use use interrupts here and sleep until the operation
+	 * completes, the controller works fairly quickly (usually a few
+	 * microseconds), so the overhead of sleeping until we get an interrupt
+	 * actually quite noticeably decreases performance.
+	 */
+	ret = readl_poll_timeout(bch->base + BCH_BHINT, reg,
+				 (reg & irq) == irq, 0, BCH_TIMEOUT);
+	if (ret)
+		return false;
+
+	if (status)
+		*status = reg;
+
+	writel(reg, bch->base + BCH_BHINT);
+	return true;
+}
+
+/**
+ * jz4780_bch_calculate() - calculate ECC for a data buffer
+ * @dev: BCH device.
+ * @params: BCH parameters.
+ * @buf: input buffer with raw data.
+ * @ecc_code: output buffer with ECC.
+ *
+ * Return: 0 on success, -ETIMEDOUT if timed out while waiting for BCH
+ * controller.
+ */
+int jz4780_bch_calculate(struct device *dev, struct jz4780_bch_params *params,
+			 const uint8_t *buf, uint8_t *ecc_code)
+{
+	struct jz4780_bch *bch = dev_get_drvdata(dev);
+	int ret = 0;
+
+	jz4780_bch_init(bch, params, true);
+	jz4780_bch_write_data(bch, buf, params->size);
+
+	if (jz4780_bch_wait_complete(bch, BCH_BHINT_ENCF, NULL)) {
+		jz4780_bch_read_parity(bch, ecc_code, params->bytes);
+	} else {
+		dev_err(dev, "timed out while calculating ECC\n");
+		ret = -ETIMEDOUT;
+	}
+
+	jz4780_bch_disable(bch);
+	return ret;
+}
+EXPORT_SYMBOL(jz4780_bch_calculate);
+
+/**
+ * jz4780_bch_correct() - detect and correct bit errors
+ * @dev: BCH device.
+ * @params: BCH parameters.
+ * @buf: raw data read from the chip.
+ * @ecc_code: ECC read from the chip.
+ *
+ * Given the raw data and the ECC read from the NAND device, detects and
+ * corrects errors in the data.
+ *
+ * Return: the number of bit errors corrected, or -1 if there are too many
+ * errors to correct or we timed out waiting for the controller.
+ */
+int jz4780_bch_correct(struct device *dev, struct jz4780_bch_params *params,
+		       uint8_t *buf, uint8_t *ecc_code)
+{
+	struct jz4780_bch *bch = dev_get_drvdata(dev);
+	uint32_t reg, mask, index;
+	int i, ret, count;
+
+	jz4780_bch_init(bch, params, false);
+	jz4780_bch_write_data(bch, buf, params->size);
+	jz4780_bch_write_data(bch, ecc_code, params->bytes);
+
+	if (!jz4780_bch_wait_complete(bch, BCH_BHINT_DECF, &reg)) {
+		dev_err(dev, "timed out while correcting data\n");
+		ret = -1;
+		goto out;
+	}
+
+	if (reg & BCH_BHINT_UNCOR) {
+		dev_warn(dev, "uncorrectable ECC error\n");
+		ret = -1;
+		goto out;
+	}
+
+	/* Correct any detected errors. */
+	if (reg & BCH_BHINT_ERR) {
+		count = (reg & BCH_BHINT_ERRC_MASK) >> BCH_BHINT_ERRC_SHIFT;
+		ret = (reg & BCH_BHINT_TERRC_MASK) >> BCH_BHINT_TERRC_SHIFT;
+
+		for (i = 0; i < count; i++) {
+			reg = readl(bch->base + BCH_BHERR0 + (i * 4));
+			mask = (reg & BCH_BHERR_MASK_MASK) >>
+						BCH_BHERR_MASK_SHIFT;
+			index = (reg & BCH_BHERR_INDEX_MASK) >>
+						BCH_BHERR_INDEX_SHIFT;
+			buf[(index * 2) + 0] ^= mask;
+			buf[(index * 2) + 1] ^= mask >> 8;
+		}
+	} else {
+		ret = 0;
+	}
+
+out:
+	jz4780_bch_disable(bch);
+	return ret;
+}
+EXPORT_SYMBOL(jz4780_bch_correct);
+
+/**
+ * jz4780_bch_get() - get the BCH controller device
+ * @np: BCH device tree node.
+ * @dev: where to store pointer to BCH controller device.
+ *
+ * Gets the BCH controller device from the specified device tree node. The
+ * device must be released with jz4780_bch_release() when it is no longer being
+ * used.
+ *
+ * Return: 0 on success, -EPROBE_DEFER if the controller has not yet been
+ * initialised.
+ */
+int jz4780_bch_get(struct device_node *np, struct device **dev)
+{
+	struct platform_device *pdev;
+	struct jz4780_bch *bch;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev))
+		return -EPROBE_DEFER;
+
+	get_device(&pdev->dev);
+
+	bch = platform_get_drvdata(pdev);
+	clk_prepare_enable(bch->clk);
+
+	*dev = &pdev->dev;
+	return 0;
+}
+EXPORT_SYMBOL(jz4780_bch_get);
+
+/**
+ * jz4780_bch_release() - release the BCH controller device
+ * @dev: BCH device.
+ */
+void jz4780_bch_release(struct device *dev)
+{
+	struct jz4780_bch *bch = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(bch->clk);
+	put_device(dev);
+}
+EXPORT_SYMBOL(jz4780_bch_release);
+
+static int jz4780_bch_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct jz4780_bch *bch;
+	struct resource *res;
+
+	bch = devm_kzalloc(dev, sizeof(*bch), GFP_KERNEL);
+	if (!bch)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	bch->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(bch->base))
+		return PTR_ERR(bch->base);
+
+	jz4780_bch_disable(bch);
+
+	bch->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(bch->clk)) {
+		dev_err(dev, "failed to get clock: %ld\n", PTR_ERR(bch->clk));
+		return PTR_ERR(bch->clk);
+	}
+
+	clk_set_rate(bch->clk, BCH_CLK_RATE);
+
+	platform_set_drvdata(pdev, bch);
+	return 0;
+}
+
+static const struct of_device_id jz4780_bch_dt_match[] = {
+	{ .compatible = "ingenic,jz4780-bch" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, jz4780_bch_dt_match);
+
+static struct platform_driver jz4780_bch_driver = {
+	.probe		= jz4780_bch_probe,
+	.driver	= {
+		.name	= "jz4780-bch",
+		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
+	},
+};
+module_platform_driver(jz4780_bch_driver);
+
+MODULE_AUTHOR("Alex Smith <alex.smith@imgtec.com>");
+MODULE_DESCRIPTION("Ingenic JZ4780 BCH error correction driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mtd/nand/jz4780_bch.h b/drivers/mtd/nand/jz4780_bch.h
new file mode 100644
index 000000000000..a5dfde59f6ea
--- /dev/null
+++ b/drivers/mtd/nand/jz4780_bch.h
@@ -0,0 +1,42 @@
+/*
+ * JZ4780 BCH controller
+ *
+ * Copyright (c) 2015 Imagination Technologies
+ * Author: Alex Smith <alex.smith@imgtec.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#ifndef __DRIVERS_MTD_NAND_JZ4780_BCH_H__
+#define __DRIVERS_MTD_NAND_JZ4780_BCH_H__
+
+#include <linux/types.h>
+
+struct device;
+struct device_node;
+
+/**
+ * struct jz4780_bch_params - BCH parameters
+ * @size: data bytes per ECC step.
+ * @bytes: ECC bytes per step.
+ * @strength: number of correctable bits per ECC step.
+ */
+struct jz4780_bch_params {
+	int size;
+	int bytes;
+	int strength;
+};
+
+extern int jz4780_bch_calculate(struct device *dev,
+				struct jz4780_bch_params *params,
+				const uint8_t *buf, uint8_t *ecc_code);
+extern int jz4780_bch_correct(struct device *dev,
+			      struct jz4780_bch_params *params, uint8_t *buf,
+			      uint8_t *ecc_code);
+
+extern int jz4780_bch_get(struct device_node *np, struct device **dev);
+extern void jz4780_bch_release(struct device *dev);
+
+#endif /* __DRIVERS_MTD_NAND_JZ4780_BCH_H__ */
diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
new file mode 100644
index 000000000000..97bd2869bedb
--- /dev/null
+++ b/drivers/mtd/nand/jz4780_nand.c
@@ -0,0 +1,378 @@
+/*
+ * JZ4780 NAND driver
+ *
+ * Copyright (c) 2015 Imagination Technologies
+ * Author: Alex Smith <alex.smith@imgtec.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/of_mtd.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+
+#include <linux/jz4780-nemc.h>
+
+#include "jz4780_bch.h"
+
+#define DRV_NAME	"jz4780-nand"
+
+#define OFFSET_DATA	0x00000000
+#define OFFSET_CMD	0x00400000
+#define OFFSET_ADDR	0x00800000
+
+/* Command delay when there is no R/B pin (in microseconds). */
+#define RB_DELAY	100
+
+struct jz4780_nand_chip {
+	unsigned int bank;
+	void __iomem *base;
+};
+
+struct jz4780_nand {
+	struct mtd_info mtd;
+	struct nand_chip chip;
+
+	struct device *dev;
+	struct device *bch;
+
+	struct nand_ecclayout ecclayout;
+
+	int busy_gpio;
+	int wp_gpio;
+	unsigned int busy_gpio_active_low: 1;
+	unsigned int wp_gpio_active_low: 1;
+	unsigned int reading: 1;
+
+	unsigned int num_banks;
+	int selected;
+	struct jz4780_nand_chip chips[];
+};
+
+static inline struct jz4780_nand *to_jz4780_nand(struct mtd_info *mtd)
+{
+	return container_of(mtd, struct jz4780_nand, mtd);
+}
+
+static void jz4780_nand_select_chip(struct mtd_info *mtd, int chipnr)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+	struct jz4780_nand_chip *chip;
+
+	if (chipnr == -1) {
+		/* Ensure the currently selected chip is deasserted. */
+		if (nand->selected >= 0) {
+			chip = &nand->chips[nand->selected];
+			jz4780_nemc_assert(nand->dev, chip->bank, false);
+		}
+	} else {
+		chip = &nand->chips[chipnr];
+		nand->chip.IO_ADDR_R = chip->base + OFFSET_DATA;
+		nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
+	}
+
+	nand->selected = chipnr;
+}
+
+static void jz4780_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
+				 unsigned int ctrl)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+	struct jz4780_nand_chip *chip;
+
+	if (WARN_ON(nand->selected < 0))
+		return;
+
+	chip = &nand->chips[nand->selected];
+
+	if (ctrl & NAND_CTRL_CHANGE) {
+		if (ctrl & NAND_ALE)
+			nand->chip.IO_ADDR_W = chip->base + OFFSET_ADDR;
+		else if (ctrl & NAND_CLE)
+			nand->chip.IO_ADDR_W = chip->base + OFFSET_CMD;
+		else
+			nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
+
+		jz4780_nemc_assert(nand->dev, chip->bank, ctrl & NAND_NCE);
+	}
+
+	if (cmd != NAND_CMD_NONE)
+		writeb(cmd, nand->chip.IO_ADDR_W);
+}
+
+static int jz4780_nand_dev_ready(struct mtd_info *mtd)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+
+	return !(gpio_get_value(nand->busy_gpio) ^ nand->busy_gpio_active_low);
+}
+
+static void jz4780_nand_ecc_hwctl(struct mtd_info *mtd, int mode)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+
+	nand->reading = (mode == NAND_ECC_READ);
+}
+
+static int jz4780_nand_ecc_calculate(struct mtd_info *mtd, const uint8_t *dat,
+				     uint8_t *ecc_code)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+	struct jz4780_bch_params params;
+
+	/*
+	 * Don't need to generate the ECC when reading, BCH does it for us as
+	 * part of decoding/correction.
+	 */
+	if (nand->reading)
+		return 0;
+
+	params.size = nand->chip.ecc.size;
+	params.bytes = nand->chip.ecc.bytes;
+	params.strength = nand->chip.ecc.strength;
+
+	return jz4780_bch_calculate(nand->bch, &params, dat, ecc_code);
+}
+
+static int jz4780_nand_ecc_correct(struct mtd_info *mtd, uint8_t *dat,
+				   uint8_t *read_ecc, uint8_t *calc_ecc)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+	struct jz4780_bch_params params;
+
+	params.size = nand->chip.ecc.size;
+	params.bytes = nand->chip.ecc.bytes;
+	params.strength = nand->chip.ecc.strength;
+
+	return jz4780_bch_correct(nand->bch, &params, dat, read_ecc);
+}
+
+static void jz4780_nand_init_ecc(struct jz4780_nand *nand, struct device *dev)
+{
+	struct mtd_info *mtd = &nand->mtd;
+	struct nand_chip *chip = &nand->chip;
+	struct nand_ecclayout *layout = &nand->ecclayout;
+	uint32_t start, i;
+
+	chip->ecc.size = of_get_nand_ecc_step_size(dev->of_node);
+	if (chip->ecc.size < 0)
+		chip->ecc.size = 1024;
+
+	chip->ecc.strength = of_get_nand_ecc_strength(dev->of_node);
+	if (chip->ecc.strength < 0)
+		chip->ecc.strength = 24;
+
+	chip->ecc.bytes = fls(1 + 8 * chip->ecc.size) * chip->ecc.strength / 8;
+
+	dev_info(dev, "using %s BCH (strength %d, size %d, bytes %d)\n",
+		 (nand->bch) ? "hardware" : "software", chip->ecc.strength,
+		 chip->ecc.size, chip->ecc.bytes);
+
+	if (nand->bch) {
+		chip->ecc.mode = NAND_ECC_HW;
+		chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
+		chip->ecc.calculate = jz4780_nand_ecc_calculate;
+		chip->ecc.correct = jz4780_nand_ecc_correct;
+	} else {
+		chip->ecc.mode = NAND_ECC_SOFT_BCH;
+	}
+
+	/* Generate ECC layout. ECC codes are right aligned in the OOB area. */
+	layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes;
+	start = mtd->oobsize - layout->eccbytes;
+	for (i = 0; i < layout->eccbytes; i++)
+		layout->eccpos[i] = start + i;
+
+	layout->oobfree[0].offset = 2;
+	layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 2;
+
+	chip->ecc.layout = layout;
+}
+
+static int jz4780_nand_init_chips(struct jz4780_nand *nand,
+				  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct jz4780_nand_chip *chip;
+	const __be32 *prop;
+	struct resource *res;
+	int i = 0;
+
+	/*
+	 * Iterate over each bank assigned to this device and request resources.
+	 * The bank numbers may not be consecutive, but nand_scan_ident()
+	 * expects chip numbers to be, so fill out a consecutive array of chips
+	 * which map chip number to actual bank number.
+	 */
+	while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
+		chip = &nand->chips[i];
+		chip->bank = of_read_number(prop, 1);
+
+		jz4780_nemc_set_type(nand->dev, chip->bank,
+				     JZ4780_NEMC_BANK_NAND);
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		chip->base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(chip->base)) {
+			dev_err(dev, "failed to map bank %u: %ld\n",
+				chip->bank, PTR_ERR(chip->base));
+			return PTR_ERR(chip->base);
+		}
+
+		i++;
+	}
+
+	return 0;
+}
+
+static int jz4780_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int num_banks;
+	struct jz4780_nand *nand;
+	struct device_node *bch_np;
+	struct mtd_info *mtd;
+	struct nand_chip *chip;
+	enum of_gpio_flags flags;
+	struct mtd_part_parser_data ppdata;
+	int ret;
+
+	num_banks = jz4780_nemc_num_banks(dev);
+
+	nand = devm_kzalloc(dev,
+			sizeof(*nand) + (sizeof(nand->chips[0]) * num_banks),
+			GFP_KERNEL);
+	if (!nand)
+		return -ENOMEM;
+
+	nand->dev = dev;
+	nand->num_banks = num_banks;
+	nand->selected = -1;
+
+	/* Look for the BCH controller. */
+	bch_np = of_parse_phandle(dev->of_node, "ingenic,bch-controller", 0);
+	if (bch_np) {
+		ret = jz4780_bch_get(bch_np, &nand->bch);
+		of_node_put(bch_np);
+		if (ret)
+			return ret;
+	}
+
+	mtd = &nand->mtd;
+	chip = &nand->chip;
+	mtd->priv = chip;
+	mtd->owner = THIS_MODULE;
+	mtd->name = DRV_NAME;
+	mtd->dev.parent = dev;
+
+	chip->chip_delay = RB_DELAY;
+	chip->options = NAND_NO_SUBPAGE_WRITE;
+	chip->bbt_options = NAND_BBT_USE_FLASH;
+	chip->select_chip = jz4780_nand_select_chip;
+	chip->cmd_ctrl = jz4780_nand_cmd_ctrl;
+
+	nand->busy_gpio = of_get_named_gpio_flags(dev->of_node,
+						  "rb-gpios",
+						  0, &flags);
+	if (gpio_is_valid(nand->busy_gpio)) {
+		ret = devm_gpio_request(dev, nand->busy_gpio, "NAND busy");
+		if (ret) {
+			dev_err(dev, "failed to request busy GPIO %d: %d\n",
+				nand->busy_gpio, ret);
+			goto err_release_bch;
+		}
+
+		nand->busy_gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
+		gpio_direction_input(nand->busy_gpio);
+
+		chip->dev_ready = jz4780_nand_dev_ready;
+	}
+
+	nand->wp_gpio = of_get_named_gpio_flags(dev->of_node, "wp-gpios",
+						0, &flags);
+	if (gpio_is_valid(nand->wp_gpio)) {
+		ret = devm_gpio_request(dev, nand->wp_gpio, "NAND WP");
+		if (ret) {
+			dev_err(dev, "failed to request WP GPIO %d: %d\n",
+				nand->wp_gpio, ret);
+			goto err_release_bch;
+		}
+
+		nand->wp_gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
+		gpio_direction_output(nand->wp_gpio, nand->wp_gpio_active_low);
+	}
+
+	ret = jz4780_nand_init_chips(nand, pdev);
+	if (ret)
+		goto err_release_bch;
+
+	ret = nand_scan_ident(mtd, num_banks, NULL);
+	if (ret)
+		goto err_release_bch;
+
+	jz4780_nand_init_ecc(nand, dev);
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
+		goto err_release_bch;
+
+	ppdata.of_node = dev->of_node;
+	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+	if (ret)
+		goto err_release_nand;
+
+	platform_set_drvdata(pdev, nand);
+	return 0;
+
+err_release_nand:
+	nand_release(mtd);
+
+err_release_bch:
+	if (nand->bch)
+		jz4780_bch_release(nand->bch);
+
+	return ret;
+}
+
+static int jz4780_nand_remove(struct platform_device *pdev)
+{
+	struct jz4780_nand *nand = platform_get_drvdata(pdev);
+
+	if (nand->bch)
+		jz4780_bch_release(nand->bch);
+
+	return 0;
+}
+
+static const struct of_device_id jz4780_nand_dt_match[] = {
+	{ .compatible = "ingenic,jz4780-nand" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, jz4780_nand_dt_match);
+
+static struct platform_driver jz4780_nand_driver = {
+	.probe		= jz4780_nand_probe,
+	.remove		= jz4780_nand_remove,
+	.driver	= {
+		.name	= DRV_NAME,
+		.of_match_table = of_match_ptr(jz4780_nand_dt_match),
+	},
+};
+module_platform_driver(jz4780_nand_driver);
+
+MODULE_AUTHOR("Alex Smith <alex.smith@imgtec.com>");
+MODULE_DESCRIPTION("Ingenic JZ4780 NAND driver");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

Re: [PATCH v5 3/4] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Ezequiel Garcia]

On 08 Sep 10:10 AM, Alex Smith wrote:
> Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
> well as the hardware BCH controller. DMA is not currently implemented.
> 
> While older 47xx SoCs also have a BCH controller, they are incompatible
> with the one in the 4780 due to differing register/bit positions, which
> would make implementing a common driver for them quite messy.
> 
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
> v4 -> v5:
>  - Bump RB_DELAY up to be sufficient for the driver to work without a
>    busy GPIO available.
>  - Use readl_poll_timeout instead of custom polling loop.
>  - Remove useless printks.
>  - Change a BUG_ON to WARN_ON.
>  - Remove use of of_translate_address(), use standard platform resource
>    APIs.
>  - Add DRV_NAME define to avoid duplication of the same string.
> 
> v3 -> v4:
>  - Rebase to 4.2-rc4
>  - Change ECC_HW_OOB_FIRST to ECC_HW, reading OOB first is not necessary.
>  - Fix argument to get_device() in jz4780_bch_get()
> 
> v2 -> v3:
>  - Rebase to 4.0-rc6
>  - Reflect binding changes
>  - get/put_device in bch get/release
>  - Removed empty .remove() callback
>  - Removed .owner
>  - Set mtd->dev.parent
> 
> v1 -> v2:
>  - Fixed module license macro
>  - Rebase to 4.0-rc3
> ---
>  drivers/mtd/nand/Kconfig       |   7 +
>  drivers/mtd/nand/Makefile      |   1 +
>  drivers/mtd/nand/jz4780_bch.c  | 348 +++++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/jz4780_bch.h  |  42 +++++
>  drivers/mtd/nand/jz4780_nand.c | 378 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 776 insertions(+)
>  create mode 100644 drivers/mtd/nand/jz4780_bch.c
>  create mode 100644 drivers/mtd/nand/jz4780_bch.h
>  create mode 100644 drivers/mtd/nand/jz4780_nand.c
> 
[..]
> +
> +static int jz4780_nand_init_chips(struct jz4780_nand *nand,
> +				  struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct jz4780_nand_chip *chip;
> +	const __be32 *prop;
> +	struct resource *res;
> +	int i = 0;
> +
> +	/*
> +	 * Iterate over each bank assigned to this device and request resources.
> +	 * The bank numbers may not be consecutive, but nand_scan_ident()
> +	 * expects chip numbers to be, so fill out a consecutive array of chips
> +	 * which map chip number to actual bank number.
> +	 */
> +	while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
> +		chip = &nand->chips[i];
> +		chip->bank = of_read_number(prop, 1);
> +
> +		jz4780_nemc_set_type(nand->dev, chip->bank,
> +				     JZ4780_NEMC_BANK_NAND);
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		chip->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(chip->base)) {
> +			dev_err(dev, "failed to map bank %u: %ld\n",
> +				chip->bank, PTR_ERR(chip->base));

You don't need to print anything on devm_ioremap_resource errors.

The rest looks good:

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v5 3/4] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Ezequiel Garcia]

Brian,

On 9 September 2015 at 11:24, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 08 Sep 10:10 AM, Alex Smith wrote:
>> Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
>> well as the hardware BCH controller. DMA is not currently implemented.
>>
>> While older 47xx SoCs also have a BCH controller, they are incompatible
>> with the one in the 4780 due to differing register/bit positions, which
>> would make implementing a common driver for them quite messy.
>>
>> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
>> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Brian Norris <computersforpeace@gmail.com>
>> Cc: linux-mtd@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> v4 -> v5:
>>  - Bump RB_DELAY up to be sufficient for the driver to work without a
>>    busy GPIO available.
>>  - Use readl_poll_timeout instead of custom polling loop.
>>  - Remove useless printks.
>>  - Change a BUG_ON to WARN_ON.
>>  - Remove use of of_translate_address(), use standard platform resource
>>    APIs.
>>  - Add DRV_NAME define to avoid duplication of the same string.
>>
>> v3 -> v4:
>>  - Rebase to 4.2-rc4
>>  - Change ECC_HW_OOB_FIRST to ECC_HW, reading OOB first is not necessary.
>>  - Fix argument to get_device() in jz4780_bch_get()
>>
>> v2 -> v3:
>>  - Rebase to 4.0-rc6
>>  - Reflect binding changes
>>  - get/put_device in bch get/release
>>  - Removed empty .remove() callback
>>  - Removed .owner
>>  - Set mtd->dev.parent
>>
>> v1 -> v2:
>>  - Fixed module license macro
>>  - Rebase to 4.0-rc3
>> ---
>>  drivers/mtd/nand/Kconfig       |   7 +
>>  drivers/mtd/nand/Makefile      |   1 +
>>  drivers/mtd/nand/jz4780_bch.c  | 348 +++++++++++++++++++++++++++++++++++++
>>  drivers/mtd/nand/jz4780_bch.h  |  42 +++++
>>  drivers/mtd/nand/jz4780_nand.c | 378 +++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 776 insertions(+)
>>  create mode 100644 drivers/mtd/nand/jz4780_bch.c
>>  create mode 100644 drivers/mtd/nand/jz4780_bch.h
>>  create mode 100644 drivers/mtd/nand/jz4780_nand.c
>>
> [..]
>> +
>> +static int jz4780_nand_init_chips(struct jz4780_nand *nand,
>> +                               struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct jz4780_nand_chip *chip;
>> +     const __be32 *prop;
>> +     struct resource *res;
>> +     int i = 0;
>> +
>> +     /*
>> +      * Iterate over each bank assigned to this device and request resources.
>> +      * The bank numbers may not be consecutive, but nand_scan_ident()
>> +      * expects chip numbers to be, so fill out a consecutive array of chips
>> +      * which map chip number to actual bank number.
>> +      */
>> +     while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
>> +             chip = &nand->chips[i];
>> +             chip->bank = of_read_number(prop, 1);
>> +
>> +             jz4780_nemc_set_type(nand->dev, chip->bank,
>> +                                  JZ4780_NEMC_BANK_NAND);
>> +
>> +             res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>> +             chip->base = devm_ioremap_resource(dev, res);
>> +             if (IS_ERR(chip->base)) {
>> +                     dev_err(dev, "failed to map bank %u: %ld\n",
>> +                             chip->bank, PTR_ERR(chip->base));
>
> You don't need to print anything on devm_ioremap_resource errors.
>
> The rest looks good:
>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Whatever the outcome of the discussion on patch 1/4
"mtd: nand: increase ready wait timeout and report timeouts"
is, the jz4780-specific patches should remain unaffected to it.

In fact, I believe patch 1/4 shouldn't even be on the same series.
So, maybe you can consider taking the rest (or not) independently
of that patch?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v5 3/4] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Brian Norris]

On Mon, Sep 14, 2015 at 03:38:48PM -0300, Ezequiel Garcia wrote:
> On 9 September 2015 at 11:24, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> Whatever the outcome of the discussion on patch 1/4
> "mtd: nand: increase ready wait timeout and report timeouts"
> is, the jz4780-specific patches should remain unaffected to it.
> 
> In fact, I believe patch 1/4 shouldn't even be on the same series.
> So, maybe you can consider taking the rest (or not) independently
> of that patch?

Sure, I'll consider the fate of patch 1 separately from 2-4.

Re: [PATCH v5 3/4] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Alex Smith]

On 9 September 2015 at 15:24, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 08 Sep 10:10 AM, Alex Smith wrote:
>> Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
>> well as the hardware BCH controller. DMA is not currently implemented.
>>
>> While older 47xx SoCs also have a BCH controller, they are incompatible
>> with the one in the 4780 due to differing register/bit positions, which
>> would make implementing a common driver for them quite messy.
>>
>> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
>> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Brian Norris <computersforpeace@gmail.com>
>> Cc: linux-mtd@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> v4 -> v5:
>>  - Bump RB_DELAY up to be sufficient for the driver to work without a
>>    busy GPIO available.
>>  - Use readl_poll_timeout instead of custom polling loop.
>>  - Remove useless printks.
>>  - Change a BUG_ON to WARN_ON.
>>  - Remove use of of_translate_address(), use standard platform resource
>>    APIs.
>>  - Add DRV_NAME define to avoid duplication of the same string.
>>
>> v3 -> v4:
>>  - Rebase to 4.2-rc4
>>  - Change ECC_HW_OOB_FIRST to ECC_HW, reading OOB first is not necessary.
>>  - Fix argument to get_device() in jz4780_bch_get()
>>
>> v2 -> v3:
>>  - Rebase to 4.0-rc6
>>  - Reflect binding changes
>>  - get/put_device in bch get/release
>>  - Removed empty .remove() callback
>>  - Removed .owner
>>  - Set mtd->dev.parent
>>
>> v1 -> v2:
>>  - Fixed module license macro
>>  - Rebase to 4.0-rc3
>> ---
>>  drivers/mtd/nand/Kconfig       |   7 +
>>  drivers/mtd/nand/Makefile      |   1 +
>>  drivers/mtd/nand/jz4780_bch.c  | 348 +++++++++++++++++++++++++++++++++++++
>>  drivers/mtd/nand/jz4780_bch.h  |  42 +++++
>>  drivers/mtd/nand/jz4780_nand.c | 378 +++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 776 insertions(+)
>>  create mode 100644 drivers/mtd/nand/jz4780_bch.c
>>  create mode 100644 drivers/mtd/nand/jz4780_bch.h
>>  create mode 100644 drivers/mtd/nand/jz4780_nand.c
>>
> [..]
>> +
>> +static int jz4780_nand_init_chips(struct jz4780_nand *nand,
>> +                               struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct jz4780_nand_chip *chip;
>> +     const __be32 *prop;
>> +     struct resource *res;
>> +     int i = 0;
>> +
>> +     /*
>> +      * Iterate over each bank assigned to this device and request resources.
>> +      * The bank numbers may not be consecutive, but nand_scan_ident()
>> +      * expects chip numbers to be, so fill out a consecutive array of chips
>> +      * which map chip number to actual bank number.
>> +      */
>> +     while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
>> +             chip = &nand->chips[i];
>> +             chip->bank = of_read_number(prop, 1);
>> +
>> +             jz4780_nemc_set_type(nand->dev, chip->bank,
>> +                                  JZ4780_NEMC_BANK_NAND);
>> +
>> +             res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>> +             chip->base = devm_ioremap_resource(dev, res);
>> +             if (IS_ERR(chip->base)) {
>> +                     dev_err(dev, "failed to map bank %u: %ld\n",
>> +                             chip->bank, PTR_ERR(chip->base));
>
> You don't need to print anything on devm_ioremap_resource errors.
>
> The rest looks good:
>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks Ezequiel, I'll resend with the printk removed soon.

Alex

> --
> Ezequiel Garcia, VanguardiaSur
> www.vanguardiasur.com.ar

Re: [PATCH v5 3/4] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Brian Norris]

On Tue, Sep 08, 2015 at 10:10:52AM +0100, Alex Smith wrote:
> Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
> well as the hardware BCH controller. DMA is not currently implemented.
> 
> While older 47xx SoCs also have a BCH controller, they are incompatible
> with the one in the 4780 due to differing register/bit positions, which
> would make implementing a common driver for them quite messy.
> 
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
> v4 -> v5:
>  - Bump RB_DELAY up to be sufficient for the driver to work without a
>    busy GPIO available.
>  - Use readl_poll_timeout instead of custom polling loop.
>  - Remove useless printks.
>  - Change a BUG_ON to WARN_ON.
>  - Remove use of of_translate_address(), use standard platform resource
>    APIs.
>  - Add DRV_NAME define to avoid duplication of the same string.
> 
> v3 -> v4:
>  - Rebase to 4.2-rc4
>  - Change ECC_HW_OOB_FIRST to ECC_HW, reading OOB first is not necessary.
>  - Fix argument to get_device() in jz4780_bch_get()
> 
> v2 -> v3:
>  - Rebase to 4.0-rc6
>  - Reflect binding changes
>  - get/put_device in bch get/release
>  - Removed empty .remove() callback
>  - Removed .owner
>  - Set mtd->dev.parent
> 
> v1 -> v2:
>  - Fixed module license macro
>  - Rebase to 4.0-rc3
> ---
>  drivers/mtd/nand/Kconfig       |   7 +
>  drivers/mtd/nand/Makefile      |   1 +
>  drivers/mtd/nand/jz4780_bch.c  | 348 +++++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/jz4780_bch.h  |  42 +++++
>  drivers/mtd/nand/jz4780_nand.c | 378 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 776 insertions(+)
>  create mode 100644 drivers/mtd/nand/jz4780_bch.c
>  create mode 100644 drivers/mtd/nand/jz4780_bch.h
>  create mode 100644 drivers/mtd/nand/jz4780_nand.c
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 3324281d1f53..8ffd5cd2a92b 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -508,6 +508,13 @@ config MTD_NAND_JZ4740
>  	help
>  		Enables support for NAND Flash on JZ4740 SoC based boards.
>  
> +config MTD_NAND_JZ4780
> +	tristate "Support for NAND on JZ4780 SoC"
> +	depends on MACH_JZ4780 && JZ4780_NEMC

IMO, it'd be nice if this would compile test on other platforms. It'd
help make things a little closer to ready for if there is ever a
multiplatform effort for MIPS like there is for ARM. So you'd need:

	depends on JZ4780_NEMC
	depends on MACH_JZ4780 || COMPILE_TEST

or similar.

But then, you'd need to change the NEMC Kconfig to allow COMPILE_TEST
too. So it's not really worth too much to me right now. Just wanted to
throw my 2 cents out there, and you can do what you want with it.

> +	help
> +	  Enables support for NAND Flash connected to the NEMC on JZ4780 SoC
> +	  based boards, using the BCH controller for hardware error correction.
> +
>  config MTD_NAND_FSMC
>  	tristate "Support for NAND on ST Micros FSMC"
>  	depends on PLAT_SPEAR || ARCH_NOMADIK || ARCH_U8500 || MACH_U300
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 1f897ec3c242..3ee2fd6bc641 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_MTD_NAND_NUC900)		+= nuc900_nand.o
>  obj-$(CONFIG_MTD_NAND_MPC5121_NFC)	+= mpc5121_nfc.o
>  obj-$(CONFIG_MTD_NAND_RICOH)		+= r852.o
>  obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
> +obj-$(CONFIG_MTD_NAND_JZ4780)		+= jz4780_nand.o jz4780_bch.o
>  obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
>  obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
>  obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
> diff --git a/drivers/mtd/nand/jz4780_bch.c b/drivers/mtd/nand/jz4780_bch.c
> new file mode 100644
> index 000000000000..6ed4ad024e83
> --- /dev/null
> +++ b/drivers/mtd/nand/jz4780_bch.c
> @@ -0,0 +1,348 @@
> +/*
> + * JZ4780 BCH controller
> + *
> + * Copyright (c) 2015 Imagination Technologies
> + * Author: Alex Smith <alex.smith@imgtec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +
> +#include "jz4780_bch.h"
> +
> +#define BCH_BHCR			0x0
> +#define BCH_BHCCR			0x8
> +#define BCH_BHCNT			0xc
> +#define BCH_BHDR			0x10
> +#define BCH_BHPAR0			0x14
> +#define BCH_BHERR0			0x84
> +#define BCH_BHINT			0x184
> +#define BCH_BHINTES			0x188
> +#define BCH_BHINTEC			0x18c
> +#define BCH_BHINTE			0x190
> +
> +#define BCH_BHCR_BSEL_SHIFT		4
> +#define BCH_BHCR_BSEL_MASK		(0x7f << BCH_BHCR_BSEL_SHIFT)
> +#define BCH_BHCR_ENCE			BIT(2)
> +#define BCH_BHCR_INIT			BIT(1)
> +#define BCH_BHCR_BCHE			BIT(0)

Use of BIT() suggests you need <linux/bitops.h>. I suppose you're
covered by implicit includes for now.

> +
> +#define BCH_BHCNT_PARITYSIZE_SHIFT	16
> +#define BCH_BHCNT_PARITYSIZE_MASK	(0x7f << BCH_BHCNT_PARITYSIZE_SHIFT)
> +#define BCH_BHCNT_BLOCKSIZE_SHIFT	0
> +#define BCH_BHCNT_BLOCKSIZE_MASK	(0x7ff << BCH_BHCNT_BLOCKSIZE_SHIFT)
> +
> +#define BCH_BHERR_MASK_SHIFT		16
> +#define BCH_BHERR_MASK_MASK		(0xffff << BCH_BHERR_MASK_SHIFT)
> +#define BCH_BHERR_INDEX_SHIFT		0
> +#define BCH_BHERR_INDEX_MASK		(0x7ff << BCH_BHERR_INDEX_SHIFT)
> +
> +#define BCH_BHINT_ERRC_SHIFT		24
> +#define BCH_BHINT_ERRC_MASK		(0x7f << BCH_BHINT_ERRC_SHIFT)
> +#define BCH_BHINT_TERRC_SHIFT		16
> +#define BCH_BHINT_TERRC_MASK		(0x7f << BCH_BHINT_TERRC_SHIFT)
> +#define BCH_BHINT_DECF			BIT(3)
> +#define BCH_BHINT_ENCF			BIT(2)
> +#define BCH_BHINT_UNCOR			BIT(1)
> +#define BCH_BHINT_ERR			BIT(0)
> +
> +#define BCH_CLK_RATE			(200 * 1000 * 1000)
> +
> +/* Timeout for BCH calculation/correction in microseconds. */
> +#define BCH_TIMEOUT			100000
> +
> +struct jz4780_bch {
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static void jz4780_bch_init(struct jz4780_bch *bch,
> +			    struct jz4780_bch_params *params, bool encode)
> +{
> +	uint32_t reg;
> +
> +	/* Clear interrupt status. */
> +	writel(readl(bch->base + BCH_BHINT), bch->base + BCH_BHINT);
> +
> +	/* Set up BCH count register. */
> +	reg = params->size << BCH_BHCNT_BLOCKSIZE_SHIFT;
> +	reg |= params->bytes << BCH_BHCNT_PARITYSIZE_SHIFT;
> +	writel(reg, bch->base + BCH_BHCNT);
> +
> +	/* Initialise and enable BCH. */
> +	reg = BCH_BHCR_BCHE | BCH_BHCR_INIT;
> +	reg |= params->strength << BCH_BHCR_BSEL_SHIFT;
> +	if (encode)
> +		reg |= BCH_BHCR_ENCE;
> +	writel(reg, bch->base + BCH_BHCR);
> +}
> +
> +static void jz4780_bch_disable(struct jz4780_bch *bch)
> +{
> +	writel(readl(bch->base + BCH_BHINT), bch->base + BCH_BHINT);
> +	writel(BCH_BHCR_BCHE, bch->base + BCH_BHCCR);
> +}
> +
> +static void jz4780_bch_write_data(struct jz4780_bch *bch, const void *buf,
> +				  size_t size)
> +{
> +	size_t size32 = size / sizeof(uint32_t);
> +	size_t size8 = size & (sizeof(uint32_t) - 1);
> +	const uint32_t *src32;
> +	const uint8_t *src8;
> +
> +	src32 = (const uint32_t *)buf;
> +	while (size32--)
> +		writel(*src32++, bch->base + BCH_BHDR);
> +
> +	src8 = (const uint8_t *)src32;
> +	while (size8--)
> +		writeb(*src8++, bch->base + BCH_BHDR);
> +}
> +
> +static void jz4780_bch_read_parity(struct jz4780_bch *bch, void *buf,
> +				   size_t size)
> +{
> +	size_t size32 = size / sizeof(uint32_t);
> +	size_t size8 = size & (sizeof(uint32_t) - 1);
> +	uint32_t *dest32;
> +	uint8_t *dest8;
> +	uint32_t val, offset = 0;
> +
> +	dest32 = (uint32_t *)buf;
> +	while (size32--) {
> +		*dest32++ = readl(bch->base + BCH_BHPAR0 + offset);
> +		offset += sizeof(uint32_t);
> +	}
> +
> +	dest8 = (uint8_t *)dest32;
> +	val = readl(bch->base + BCH_BHPAR0 + offset);
> +	switch (size8) {
> +	case 3:
> +		dest8[2] = (val >> 16) & 0xff;
> +	case 2:
> +		dest8[1] = (val >> 8) & 0xff;
> +	case 1:
> +		dest8[0] = val & 0xff;
> +		break;
> +	}
> +}
> +
> +static bool jz4780_bch_wait_complete(struct jz4780_bch *bch, unsigned int irq,
> +				     uint32_t *status)
> +{
> +	uint32_t reg;
> +	int ret;
> +
> +	/*
> +	 * While we could use use interrupts here and sleep until the operation
> +	 * completes, the controller works fairly quickly (usually a few
> +	 * microseconds), so the overhead of sleeping until we get an interrupt
> +	 * actually quite noticeably decreases performance.
> +	 */
> +	ret = readl_poll_timeout(bch->base + BCH_BHINT, reg,
> +				 (reg & irq) == irq, 0, BCH_TIMEOUT);
> +	if (ret)
> +		return false;
> +
> +	if (status)
> +		*status = reg;
> +
> +	writel(reg, bch->base + BCH_BHINT);
> +	return true;
> +}
> +
> +/**
> + * jz4780_bch_calculate() - calculate ECC for a data buffer
> + * @dev: BCH device.
> + * @params: BCH parameters.
> + * @buf: input buffer with raw data.
> + * @ecc_code: output buffer with ECC.
> + *
> + * Return: 0 on success, -ETIMEDOUT if timed out while waiting for BCH
> + * controller.
> + */
> +int jz4780_bch_calculate(struct device *dev, struct jz4780_bch_params *params,
> +			 const uint8_t *buf, uint8_t *ecc_code)
> +{
> +	struct jz4780_bch *bch = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	jz4780_bch_init(bch, params, true);
> +	jz4780_bch_write_data(bch, buf, params->size);
> +
> +	if (jz4780_bch_wait_complete(bch, BCH_BHINT_ENCF, NULL)) {
> +		jz4780_bch_read_parity(bch, ecc_code, params->bytes);
> +	} else {
> +		dev_err(dev, "timed out while calculating ECC\n");
> +		ret = -ETIMEDOUT;
> +	}
> +
> +	jz4780_bch_disable(bch);
> +	return ret;
> +}
> +EXPORT_SYMBOL(jz4780_bch_calculate);
> +
> +/**
> + * jz4780_bch_correct() - detect and correct bit errors
> + * @dev: BCH device.
> + * @params: BCH parameters.
> + * @buf: raw data read from the chip.
> + * @ecc_code: ECC read from the chip.
> + *
> + * Given the raw data and the ECC read from the NAND device, detects and
> + * corrects errors in the data.
> + *
> + * Return: the number of bit errors corrected, or -1 if there are too many
> + * errors to correct or we timed out waiting for the controller.
> + */
> +int jz4780_bch_correct(struct device *dev, struct jz4780_bch_params *params,
> +		       uint8_t *buf, uint8_t *ecc_code)
> +{
> +	struct jz4780_bch *bch = dev_get_drvdata(dev);
> +	uint32_t reg, mask, index;
> +	int i, ret, count;
> +
> +	jz4780_bch_init(bch, params, false);
> +	jz4780_bch_write_data(bch, buf, params->size);
> +	jz4780_bch_write_data(bch, ecc_code, params->bytes);
> +
> +	if (!jz4780_bch_wait_complete(bch, BCH_BHINT_DECF, &reg)) {
> +		dev_err(dev, "timed out while correcting data\n");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	if (reg & BCH_BHINT_UNCOR) {
> +		dev_warn(dev, "uncorrectable ECC error\n");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	/* Correct any detected errors. */
> +	if (reg & BCH_BHINT_ERR) {
> +		count = (reg & BCH_BHINT_ERRC_MASK) >> BCH_BHINT_ERRC_SHIFT;
> +		ret = (reg & BCH_BHINT_TERRC_MASK) >> BCH_BHINT_TERRC_SHIFT;
> +
> +		for (i = 0; i < count; i++) {
> +			reg = readl(bch->base + BCH_BHERR0 + (i * 4));
> +			mask = (reg & BCH_BHERR_MASK_MASK) >>
> +						BCH_BHERR_MASK_SHIFT;
> +			index = (reg & BCH_BHERR_INDEX_MASK) >>
> +						BCH_BHERR_INDEX_SHIFT;
> +			buf[(index * 2) + 0] ^= mask;
> +			buf[(index * 2) + 1] ^= mask >> 8;
> +		}
> +	} else {
> +		ret = 0;
> +	}
> +
> +out:
> +	jz4780_bch_disable(bch);
> +	return ret;
> +}
> +EXPORT_SYMBOL(jz4780_bch_correct);
> +
> +/**
> + * jz4780_bch_get() - get the BCH controller device
> + * @np: BCH device tree node.
> + * @dev: where to store pointer to BCH controller device.
> + *
> + * Gets the BCH controller device from the specified device tree node. The
> + * device must be released with jz4780_bch_release() when it is no longer being
> + * used.
> + *
> + * Return: 0 on success, -EPROBE_DEFER if the controller has not yet been
> + * initialised.
> + */
> +int jz4780_bch_get(struct device_node *np, struct device **dev)
> +{
> +	struct platform_device *pdev;
> +	struct jz4780_bch *bch;
> +
> +	pdev = of_find_device_by_node(np);
> +	if (!pdev || !platform_get_drvdata(pdev))
> +		return -EPROBE_DEFER;
> +
> +	get_device(&pdev->dev);
> +
> +	bch = platform_get_drvdata(pdev);
> +	clk_prepare_enable(bch->clk);
> +
> +	*dev = &pdev->dev;
> +	return 0;
> +}
> +EXPORT_SYMBOL(jz4780_bch_get);
> +
> +/**
> + * jz4780_bch_release() - release the BCH controller device
> + * @dev: BCH device.
> + */
> +void jz4780_bch_release(struct device *dev)
> +{
> +	struct jz4780_bch *bch = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(bch->clk);
> +	put_device(dev);
> +}
> +EXPORT_SYMBOL(jz4780_bch_release);
> +
> +static int jz4780_bch_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct jz4780_bch *bch;
> +	struct resource *res;
> +
> +	bch = devm_kzalloc(dev, sizeof(*bch), GFP_KERNEL);
> +	if (!bch)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	bch->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(bch->base))
> +		return PTR_ERR(bch->base);
> +
> +	jz4780_bch_disable(bch);
> +
> +	bch->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(bch->clk)) {
> +		dev_err(dev, "failed to get clock: %ld\n", PTR_ERR(bch->clk));
> +		return PTR_ERR(bch->clk);
> +	}
> +
> +	clk_set_rate(bch->clk, BCH_CLK_RATE);
> +
> +	platform_set_drvdata(pdev, bch);
> +	return 0;
> +}
> +
> +static const struct of_device_id jz4780_bch_dt_match[] = {
> +	{ .compatible = "ingenic,jz4780-bch" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, jz4780_bch_dt_match);
> +
> +static struct platform_driver jz4780_bch_driver = {
> +	.probe		= jz4780_bch_probe,
> +	.driver	= {
> +		.name	= "jz4780-bch",
> +		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
> +	},
> +};
> +module_platform_driver(jz4780_bch_driver);
> +
> +MODULE_AUTHOR("Alex Smith <alex.smith@imgtec.com>");
> +MODULE_DESCRIPTION("Ingenic JZ4780 BCH error correction driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mtd/nand/jz4780_bch.h b/drivers/mtd/nand/jz4780_bch.h
> new file mode 100644
> index 000000000000..a5dfde59f6ea
> --- /dev/null
> +++ b/drivers/mtd/nand/jz4780_bch.h
> @@ -0,0 +1,42 @@
> +/*
> + * JZ4780 BCH controller
> + *
> + * Copyright (c) 2015 Imagination Technologies
> + * Author: Alex Smith <alex.smith@imgtec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#ifndef __DRIVERS_MTD_NAND_JZ4780_BCH_H__
> +#define __DRIVERS_MTD_NAND_JZ4780_BCH_H__
> +
> +#include <linux/types.h>
> +
> +struct device;
> +struct device_node;
> +
> +/**
> + * struct jz4780_bch_params - BCH parameters
> + * @size: data bytes per ECC step.
> + * @bytes: ECC bytes per step.
> + * @strength: number of correctable bits per ECC step.
> + */
> +struct jz4780_bch_params {
> +	int size;
> +	int bytes;
> +	int strength;
> +};
> +
> +extern int jz4780_bch_calculate(struct device *dev,
> +				struct jz4780_bch_params *params,
> +				const uint8_t *buf, uint8_t *ecc_code);
> +extern int jz4780_bch_correct(struct device *dev,
> +			      struct jz4780_bch_params *params, uint8_t *buf,
> +			      uint8_t *ecc_code);
> +
> +extern int jz4780_bch_get(struct device_node *np, struct device **dev);
> +extern void jz4780_bch_release(struct device *dev);
> +
> +#endif /* __DRIVERS_MTD_NAND_JZ4780_BCH_H__ */
> diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
> new file mode 100644
> index 000000000000..97bd2869bedb
> --- /dev/null
> +++ b/drivers/mtd/nand/jz4780_nand.c
> @@ -0,0 +1,378 @@
> +/*
> + * JZ4780 NAND driver
> + *
> + * Copyright (c) 2015 Imagination Technologies
> + * Author: Alex Smith <alex.smith@imgtec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_mtd.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +
> +#include <linux/jz4780-nemc.h>
> +
> +#include "jz4780_bch.h"
> +
> +#define DRV_NAME	"jz4780-nand"
> +
> +#define OFFSET_DATA	0x00000000
> +#define OFFSET_CMD	0x00400000
> +#define OFFSET_ADDR	0x00800000
> +
> +/* Command delay when there is no R/B pin (in microseconds). */
> +#define RB_DELAY	100
> +
> +struct jz4780_nand_chip {
> +	unsigned int bank;
> +	void __iomem *base;
> +};
> +
> +struct jz4780_nand {
> +	struct mtd_info mtd;
> +	struct nand_chip chip;
> +
> +	struct device *dev;
> +	struct device *bch;
> +
> +	struct nand_ecclayout ecclayout;
> +
> +	int busy_gpio;
> +	int wp_gpio;
> +	unsigned int busy_gpio_active_low: 1;
> +	unsigned int wp_gpio_active_low: 1;
> +	unsigned int reading: 1;
> +
> +	unsigned int num_banks;
> +	int selected;
> +	struct jz4780_nand_chip chips[];
> +};
> +
> +static inline struct jz4780_nand *to_jz4780_nand(struct mtd_info *mtd)
> +{
> +	return container_of(mtd, struct jz4780_nand, mtd);
> +}
> +
> +static void jz4780_nand_select_chip(struct mtd_info *mtd, int chipnr)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +	struct jz4780_nand_chip *chip;
> +
> +	if (chipnr == -1) {
> +		/* Ensure the currently selected chip is deasserted. */
> +		if (nand->selected >= 0) {
> +			chip = &nand->chips[nand->selected];
> +			jz4780_nemc_assert(nand->dev, chip->bank, false);
> +		}
> +	} else {
> +		chip = &nand->chips[chipnr];
> +		nand->chip.IO_ADDR_R = chip->base + OFFSET_DATA;
> +		nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
> +	}
> +
> +	nand->selected = chipnr;
> +}
> +
> +static void jz4780_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
> +				 unsigned int ctrl)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +	struct jz4780_nand_chip *chip;
> +
> +	if (WARN_ON(nand->selected < 0))
> +		return;
> +
> +	chip = &nand->chips[nand->selected];
> +
> +	if (ctrl & NAND_CTRL_CHANGE) {
> +		if (ctrl & NAND_ALE)
> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_ADDR;
> +		else if (ctrl & NAND_CLE)
> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_CMD;
> +		else
> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
> +
> +		jz4780_nemc_assert(nand->dev, chip->bank, ctrl & NAND_NCE);
> +	}
> +
> +	if (cmd != NAND_CMD_NONE)
> +		writeb(cmd, nand->chip.IO_ADDR_W);
> +}
> +
> +static int jz4780_nand_dev_ready(struct mtd_info *mtd)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +
> +	return !(gpio_get_value(nand->busy_gpio) ^ nand->busy_gpio_active_low);
> +}
> +
> +static void jz4780_nand_ecc_hwctl(struct mtd_info *mtd, int mode)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +
> +	nand->reading = (mode == NAND_ECC_READ);
> +}
> +
> +static int jz4780_nand_ecc_calculate(struct mtd_info *mtd, const uint8_t *dat,
> +				     uint8_t *ecc_code)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +	struct jz4780_bch_params params;
> +
> +	/*
> +	 * Don't need to generate the ECC when reading, BCH does it for us as
> +	 * part of decoding/correction.
> +	 */
> +	if (nand->reading)
> +		return 0;
> +
> +	params.size = nand->chip.ecc.size;
> +	params.bytes = nand->chip.ecc.bytes;
> +	params.strength = nand->chip.ecc.strength;
> +
> +	return jz4780_bch_calculate(nand->bch, &params, dat, ecc_code);
> +}
> +
> +static int jz4780_nand_ecc_correct(struct mtd_info *mtd, uint8_t *dat,
> +				   uint8_t *read_ecc, uint8_t *calc_ecc)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +	struct jz4780_bch_params params;
> +
> +	params.size = nand->chip.ecc.size;
> +	params.bytes = nand->chip.ecc.bytes;
> +	params.strength = nand->chip.ecc.strength;
> +
> +	return jz4780_bch_correct(nand->bch, &params, dat, read_ecc);
> +}
> +
> +static void jz4780_nand_init_ecc(struct jz4780_nand *nand, struct device *dev)
> +{
> +	struct mtd_info *mtd = &nand->mtd;
> +	struct nand_chip *chip = &nand->chip;
> +	struct nand_ecclayout *layout = &nand->ecclayout;
> +	uint32_t start, i;
> +
> +	chip->ecc.size = of_get_nand_ecc_step_size(dev->of_node);
> +	if (chip->ecc.size < 0)
> +		chip->ecc.size = 1024;
> +
> +	chip->ecc.strength = of_get_nand_ecc_strength(dev->of_node);
> +	if (chip->ecc.strength < 0)
> +		chip->ecc.strength = 24;

Can you make use of nand_dt_init()? That means you'd also need to
specify the generic "nand-ecc-mode" property in your DT, then initialize
chip->flash_node before running nand_scan_ident().

> +
> +	chip->ecc.bytes = fls(1 + 8 * chip->ecc.size) * chip->ecc.strength / 8;
> +
> +	dev_info(dev, "using %s BCH (strength %d, size %d, bytes %d)\n",
> +		 (nand->bch) ? "hardware" : "software", chip->ecc.strength,
> +		 chip->ecc.size, chip->ecc.bytes);
> +
> +	if (nand->bch) {
> +		chip->ecc.mode = NAND_ECC_HW;
> +		chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
> +		chip->ecc.calculate = jz4780_nand_ecc_calculate;
> +		chip->ecc.correct = jz4780_nand_ecc_correct;
> +	} else {
> +		chip->ecc.mode = NAND_ECC_SOFT_BCH;
> +	}
> +
> +	/* Generate ECC layout. ECC codes are right aligned in the OOB area. */
> +	layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes;
> +	start = mtd->oobsize - layout->eccbytes;
> +	for (i = 0; i < layout->eccbytes; i++)
> +		layout->eccpos[i] = start + i;
> +
> +	layout->oobfree[0].offset = 2;
> +	layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 2;
> +
> +	chip->ecc.layout = layout;
> +}
> +
> +static int jz4780_nand_init_chips(struct jz4780_nand *nand,
> +				  struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct jz4780_nand_chip *chip;
> +	const __be32 *prop;
> +	struct resource *res;
> +	int i = 0;
> +
> +	/*
> +	 * Iterate over each bank assigned to this device and request resources.
> +	 * The bank numbers may not be consecutive, but nand_scan_ident()
> +	 * expects chip numbers to be, so fill out a consecutive array of chips
> +	 * which map chip number to actual bank number.
> +	 */

Hmm, this is an interesting point. Do we really want to lump multiple
banks in the same device tree node? I've seen the history (nand_scan*()
supports multipe chips in a single nand_scan*() call) but this can be
limiting. What if you have two non-identical flash?

IOW, would following a pattern like the following make more sense? With
these, each separate flash gets its own device node:

https://www.kernel.org/doc/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
https://www.kernel.org/doc/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt

> +	while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
> +		chip = &nand->chips[i];
> +		chip->bank = of_read_number(prop, 1);
> +
> +		jz4780_nemc_set_type(nand->dev, chip->bank,
> +				     JZ4780_NEMC_BANK_NAND);
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		chip->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(chip->base)) {
> +			dev_err(dev, "failed to map bank %u: %ld\n",
> +				chip->bank, PTR_ERR(chip->base));
> +			return PTR_ERR(chip->base);
> +		}
> +
> +		i++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int jz4780_nand_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	unsigned int num_banks;
> +	struct jz4780_nand *nand;
> +	struct device_node *bch_np;
> +	struct mtd_info *mtd;
> +	struct nand_chip *chip;
> +	enum of_gpio_flags flags;
> +	struct mtd_part_parser_data ppdata;
> +	int ret;
> +
> +	num_banks = jz4780_nemc_num_banks(dev);

Should we handle the num_banks==0 case?

> +
> +	nand = devm_kzalloc(dev,
> +			sizeof(*nand) + (sizeof(nand->chips[0]) * num_banks),
> +			GFP_KERNEL);
> +	if (!nand)
> +		return -ENOMEM;
> +
> +	nand->dev = dev;
> +	nand->num_banks = num_banks;
> +	nand->selected = -1;
> +
> +	/* Look for the BCH controller. */
> +	bch_np = of_parse_phandle(dev->of_node, "ingenic,bch-controller", 0);
> +	if (bch_np) {
> +		ret = jz4780_bch_get(bch_np, &nand->bch);
> +		of_node_put(bch_np);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	mtd = &nand->mtd;
> +	chip = &nand->chip;
> +	mtd->priv = chip;
> +	mtd->owner = THIS_MODULE;
> +	mtd->name = DRV_NAME;
> +	mtd->dev.parent = dev;
> +
> +	chip->chip_delay = RB_DELAY;
> +	chip->options = NAND_NO_SUBPAGE_WRITE;
> +	chip->bbt_options = NAND_BBT_USE_FLASH;

Might use the "nand-on-flash-bbt" DT property, and nand_dt_init() will
handle this for you automagically?

> +	chip->select_chip = jz4780_nand_select_chip;
> +	chip->cmd_ctrl = jz4780_nand_cmd_ctrl;
> +
> +	nand->busy_gpio = of_get_named_gpio_flags(dev->of_node,
> +						  "rb-gpios",
> +						  0, &flags);

Note (for future reference, not for your immediate action): this binding
is shared with at least sunxi-nand. We could probably share the (simple)
code for it too by moving this to nand_base.

> +	if (gpio_is_valid(nand->busy_gpio)) {
> +		ret = devm_gpio_request(dev, nand->busy_gpio, "NAND busy");
> +		if (ret) {
> +			dev_err(dev, "failed to request busy GPIO %d: %d\n",
> +				nand->busy_gpio, ret);
> +			goto err_release_bch;
> +		}
> +
> +		nand->busy_gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
> +		gpio_direction_input(nand->busy_gpio);
> +
> +		chip->dev_ready = jz4780_nand_dev_ready;
> +	}
> +
> +	nand->wp_gpio = of_get_named_gpio_flags(dev->of_node, "wp-gpios",
> +						0, &flags);

Another note (again, not necessarily for your your immediate action):
this binding was requested for other drivers. Having some kind of
support code for it in nand_base could be helpful, even if it's just as
simple as to disable WP at startup. I have also seen cases where users
want a way to control WP policy -- e.g., to only disable WP when
reprogramming, so that it's more difficult to experience spurious writes
to the flash due to flaky HW. So handling that in the core driver could
be useful. But not your problem.

> +	if (gpio_is_valid(nand->wp_gpio)) {
> +		ret = devm_gpio_request(dev, nand->wp_gpio, "NAND WP");
> +		if (ret) {
> +			dev_err(dev, "failed to request WP GPIO %d: %d\n",
> +				nand->wp_gpio, ret);
> +			goto err_release_bch;
> +		}
> +
> +		nand->wp_gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
> +		gpio_direction_output(nand->wp_gpio, nand->wp_gpio_active_low);
> +	}
> +
> +	ret = jz4780_nand_init_chips(nand, pdev);
> +	if (ret)
> +		goto err_release_bch;
> +
> +	ret = nand_scan_ident(mtd, num_banks, NULL);
> +	if (ret)
> +		goto err_release_bch;
> +
> +	jz4780_nand_init_ecc(nand, dev);
> +
> +	ret = nand_scan_tail(mtd);
> +	if (ret)
> +		goto err_release_bch;
> +
> +	ppdata.of_node = dev->of_node;
> +	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
> +	if (ret)
> +		goto err_release_nand;
> +
> +	platform_set_drvdata(pdev, nand);
> +	return 0;
> +
> +err_release_nand:
> +	nand_release(mtd);
> +
> +err_release_bch:
> +	if (nand->bch)
> +		jz4780_bch_release(nand->bch);
> +
> +	return ret;
> +}
> +
> +static int jz4780_nand_remove(struct platform_device *pdev)
> +{
> +	struct jz4780_nand *nand = platform_get_drvdata(pdev);
> +
> +	if (nand->bch)
> +		jz4780_bch_release(nand->bch);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id jz4780_nand_dt_match[] = {
> +	{ .compatible = "ingenic,jz4780-nand" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, jz4780_nand_dt_match);
> +
> +static struct platform_driver jz4780_nand_driver = {
> +	.probe		= jz4780_nand_probe,
> +	.remove		= jz4780_nand_remove,
> +	.driver	= {
> +		.name	= DRV_NAME,
> +		.of_match_table = of_match_ptr(jz4780_nand_dt_match),
> +	},
> +};
> +module_platform_driver(jz4780_nand_driver);
> +
> +MODULE_AUTHOR("Alex Smith <alex.smith@imgtec.com>");
> +MODULE_DESCRIPTION("Ingenic JZ4780 NAND driver");
> +MODULE_LICENSE("GPL v2");

Brian

Re: [PATCH v5 3/4] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Boris Brezillon]

Brian, Alex,

On Mon, 21 Sep 2015 15:08:06 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Tue, Sep 08, 2015 at 10:10:52AM +0100, Alex Smith wrote:
> > Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
> > well as the hardware BCH controller. DMA is not currently implemented.
> > 
> > While older 47xx SoCs also have a BCH controller, they are incompatible
> > with the one in the 4780 due to differing register/bit positions, which
> > would make implementing a common driver for them quite messy.
> > 
> > Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> > Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: linux-mtd@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > v4 -> v5:
> >  - Bump RB_DELAY up to be sufficient for the driver to work without a
> >    busy GPIO available.
> >  - Use readl_poll_timeout instead of custom polling loop.
> >  - Remove useless printks.
> >  - Change a BUG_ON to WARN_ON.
> >  - Remove use of of_translate_address(), use standard platform resource
> >    APIs.
> >  - Add DRV_NAME define to avoid duplication of the same string.
> > 
> > v3 -> v4:
> >  - Rebase to 4.2-rc4
> >  - Change ECC_HW_OOB_FIRST to ECC_HW, reading OOB first is not necessary.
> >  - Fix argument to get_device() in jz4780_bch_get()
> > 
> > v2 -> v3:
> >  - Rebase to 4.0-rc6
> >  - Reflect binding changes
> >  - get/put_device in bch get/release
> >  - Removed empty .remove() callback
> >  - Removed .owner
> >  - Set mtd->dev.parent
> > 
> > v1 -> v2:
> >  - Fixed module license macro
> >  - Rebase to 4.0-rc3
> > ---
> >  drivers/mtd/nand/Kconfig       |   7 +
> >  drivers/mtd/nand/Makefile      |   1 +
> >  drivers/mtd/nand/jz4780_bch.c  | 348 +++++++++++++++++++++++++++++++++++++
> >  drivers/mtd/nand/jz4780_bch.h  |  42 +++++
> >  drivers/mtd/nand/jz4780_nand.c | 378 +++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 776 insertions(+)
> >  create mode 100644 drivers/mtd/nand/jz4780_bch.c
> >  create mode 100644 drivers/mtd/nand/jz4780_bch.h
> >  create mode 100644 drivers/mtd/nand/jz4780_nand.c
> > 

[...]


> > +static void jz4780_nand_init_ecc(struct jz4780_nand *nand, struct device *dev)
> > +{
> > +	struct mtd_info *mtd = &nand->mtd;
> > +	struct nand_chip *chip = &nand->chip;
> > +	struct nand_ecclayout *layout = &nand->ecclayout;
> > +	uint32_t start, i;
> > +
> > +	chip->ecc.size = of_get_nand_ecc_step_size(dev->of_node);
> > +	if (chip->ecc.size < 0)
> > +		chip->ecc.size = 1024;
> > +
> > +	chip->ecc.strength = of_get_nand_ecc_strength(dev->of_node);
> > +	if (chip->ecc.strength < 0)
> > +		chip->ecc.strength = 24;
> 
> Can you make use of nand_dt_init()? That means you'd also need to
> specify the generic "nand-ecc-mode" property in your DT, then initialize
> chip->flash_node before running nand_scan_ident().

Also, remember to initialize ->ecc.mode to its default value before
calling nand_scan_ident(), otherwise you won't be able to know whether
ECC_NONE was selected on purpose (nand-ecc-mode = "none" in your DT) or
not.

You'll also have to change your 'chip->ecc.size < 0' and
'chip->ecc.strength < 0' tests into '!chip->ecc.size' and
'!chip->ecc.strength'


[...]

> > +static int jz4780_nand_init_chips(struct jz4780_nand *nand,
> > +				  struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct jz4780_nand_chip *chip;
> > +	const __be32 *prop;
> > +	struct resource *res;
> > +	int i = 0;
> > +
> > +	/*
> > +	 * Iterate over each bank assigned to this device and request resources.
> > +	 * The bank numbers may not be consecutive, but nand_scan_ident()
> > +	 * expects chip numbers to be, so fill out a consecutive array of chips
> > +	 * which map chip number to actual bank number.
> > +	 */
> 
> Hmm, this is an interesting point. Do we really want to lump multiple
> banks in the same device tree node? I've seen the history (nand_scan*()
> supports multipe chips in a single nand_scan*() call) but this can be
> limiting. What if you have two non-identical flash?
> 
> IOW, would following a pattern like the following make more sense? With
> these, each separate flash gets its own device node:
> 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
> https://www.kernel.org/doc/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt

Actually, we support this kind of things in the sunxi_nand driver too.
The only valid use case I see for this representation is when you have a
NAND chip embedding several dies, and thus exposing several CS lines.
IMHO, those chips should still be represented as a single entity, and
they need to be attached to several CS lines. I guess that's what
you're trying to support here (tell me if I'm wrong).

[...]

> 
> > +	chip->select_chip = jz4780_nand_select_chip;
> > +	chip->cmd_ctrl = jz4780_nand_cmd_ctrl;
> > +
> > +	nand->busy_gpio = of_get_named_gpio_flags(dev->of_node,
> > +						  "rb-gpios",
> > +						  0, &flags);
> 
> Note (for future reference, not for your immediate action): this binding
> is shared with at least sunxi-nand. We could probably share the (simple)
> code for it too by moving this to nand_base.

I totally agree. That implies adding an rb_gpio (or rb_gpios, since
some chips have several of them) in the nand_chip struct, and then
parsing the property in nand_dt_init(), so nothing impossible here.
Maybe we should also provide a default dev_ready() implementation when
this gpio is specified.

> 
> > +	if (gpio_is_valid(nand->busy_gpio)) {
> > +		ret = devm_gpio_request(dev, nand->busy_gpio, "NAND busy");
> > +		if (ret) {
> > +			dev_err(dev, "failed to request busy GPIO %d: %d\n",
> > +				nand->busy_gpio, ret);
> > +			goto err_release_bch;
> > +		}
> > +
> > +		nand->busy_gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
> > +		gpio_direction_input(nand->busy_gpio);
> > +
> > +		chip->dev_ready = jz4780_nand_dev_ready;
> > +	}
> > +
> > +	nand->wp_gpio = of_get_named_gpio_flags(dev->of_node, "wp-gpios",
> > +						0, &flags);
> 
> Another note (again, not necessarily for your your immediate action):
> this binding was requested for other drivers. Having some kind of
> support code for it in nand_base could be helpful, even if it's just as
> simple as to disable WP at startup. I have also seen cases where users
> want a way to control WP policy -- e.g., to only disable WP when
> reprogramming, so that it's more difficult to experience spurious writes
> to the flash due to flaky HW. So handling that in the core driver could
> be useful. But not your problem.

Yep, I guess it's pretty much the same as for the RB gpios.

Best Regards,

Boris

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

[PATCH v5 4/4] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[Alex Smith]

Add device tree nodes for the NEMC and BCH to the JZ4780 device tree,
and make use of them in the Ci20 device tree to add a node for the
board's NAND.

Note that since the pinctrl driver is not yet upstream, this includes
neither pin configuration nor busy/write-protect GPIO pins for the
NAND. Use of the NAND relies on the boot loader to have left the pins
configured in a usable state, which should be the case when booted
from the NAND.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: linux-mtd@lists.infradead.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
---
v4 -> v5:
 - New patch adding DT nodes for the NAND so that the driver can be
   tested.
---
 arch/mips/boot/dts/ingenic/ci20.dts    | 51 ++++++++++++++++++++++++++++++++++
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 26 +++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index 9fcb9e7d1f57..c0c8a64a3522 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -42,3 +42,54 @@
 &uart4 {
 	status = "okay";
 };
+
+&nemc {
+	status = "okay";
+
+	nand: nand@1 {
+		compatible = "ingenic,jz4780-nand";
+		reg = <1 0 0x1000000>;
+
+		ingenic,nemc-tAS = <10>;
+		ingenic,nemc-tAH = <5>;
+		ingenic,nemc-tBP = <10>;
+		ingenic,nemc-tAW = <15>;
+		ingenic,nemc-tSTRV = <100>;
+
+		ingenic,bch-controller = <&bch>;
+		ingenic,ecc-size = <1024>;
+		ingenic,ecc-strength = <24>;
+
+		#address-cells = <2>;
+		#size-cells = <2>;
+
+		partition@0 {
+			label = "u-boot-spl";
+			reg = <0x0 0x0 0x0 0x800000>;
+		};
+
+		partition@0x800000 {
+			label = "u-boot";
+			reg = <0x0 0x800000 0x0 0x200000>;
+		};
+
+		partition@0xa00000 {
+			label = "u-boot-env";
+			reg = <0x0 0xa00000 0x0 0x200000>;
+		};
+
+		partition@0xc00000 {
+			label = "boot";
+			reg = <0x0 0xc00000 0x0 0x4000000>;
+		};
+
+		partition@0x8c00000 {
+			label = "system";
+			reg = <0x0 0x4c00000 0x1 0xfb400000>;
+		};
+	};
+};
+
+&bch {
+	status = "okay";
+};
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 65389f602733..b868b429add2 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -108,4 +108,30 @@
 
 		status = "disabled";
 	};
+
+	nemc: nemc@13410000 {
+		compatible = "ingenic,jz4780-nemc";
+		reg = <0x13410000 0x10000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <1 0 0x1b000000 0x1000000
+			  2 0 0x1a000000 0x1000000
+			  3 0 0x19000000 0x1000000
+			  4 0 0x18000000 0x1000000
+			  5 0 0x17000000 0x1000000
+			  6 0 0x16000000 0x1000000>;
+
+		clocks = <&cgu JZ4780_CLK_NEMC>;
+
+		status = "disabled";
+	};
+
+	bch: bch@134d0000 {
+		compatible = "ingenic,jz4780-bch";
+		reg = <0x134d0000 0x10000>;
+
+		clocks = <&cgu JZ4780_CLK_BCH>;
+
+		status = "disabled";
+	};
 };
-- 
2.5.0