arm: iproc: add NAND driver

From: Jiandong Zheng <[email protected]>

Add support for the iproc NAND, and enable on Cygnus and NSP boards.

Signed-off-by Jiandong Zheng <[email protected]>

Signed-off-by Steve Rae <[email protected]>

---
There was a previous attempt to implement this "iproc NAND" (see: http://patchwork.ozlabs.org/patch/505399), however, due to the amount of changes required, it seemed better to implement the code in a series of steps. This is the first step, where the "iproc_nand.c" is essentially an empty file (with one function required to allow this commit to build successfully).

 arch/arm/include/asm/arch-bcmcygnus/configs.h |  11 +++
 arch/arm/include/asm/arch-bcmnsp/configs.h    |  11 +++
 drivers/mtd/nand/Makefile                     |   1 +
 drivers/mtd/nand/iproc_nand.c                 |  11 +++
 drivers/mtd/nand/iproc_nand_cygnus.h          | 111 +++++++++++++++++++++++++
 drivers/mtd/nand/iproc_nand_ns_plus.h         | 113 ++++++++++++++++++++++++++
 6 files changed, 258 insertions(+)

Quoted 14 lines · Steve Rae

From: Jiandong Zheng <[email protected]>

Add support for the iproc NAND, and enable on Cygnus and NSP boards.

Signed-off-by Jiandong Zheng <[email protected]>

Signed-off-by Steve Rae <[email protected]>

---
There was a previous attempt to implement this "iproc NAND" (see: http://patchwork.ozlabs.org/patch/505399), however, due to the amount of changes required, it seemed better to implement the code in a series of steps. This is the first step, where the "iproc_nand.c" is essentially an empty file (with one function required to allow this commit to build successfully).

Quoted 7 lines

 arch/arm/include/asm/arch-bcmcygnus/configs.h |  11 +++
 arch/arm/include/asm/arch-bcmnsp/configs.h    |  11 +++
 drivers/mtd/nand/Makefile                     |   1 +
 drivers/mtd/nand/iproc_nand.c                 |  11 +++
 drivers/mtd/nand/iproc_nand_cygnus.h          | 111 +++++++++++++++++++++++++
 drivers/mtd/nand/iproc_nand_ns_plus.h         | 113 ++++++++++++++++++++++++++
 6 files changed, 258 insertions(+)

We need to be adding Kconfig entries here at a minimum.

--
Tom

Is it expected the _every_ CONFIG_* and CONFIG_SYS_* will be moved to Kconfig? What about #defines that will absolutely never used by anyone else? for example, I (may) have a code fragment that distinguishes between "CONFIG_CYGNUS" and "CONFIG_NSPLUS"...
Would these need to be Kconfig? or would that just clutter Kconfig with useless entries?

Quoted 6 lines · Steve Rae

Is it expected the _every_ CONFIG_* and CONFIG_SYS_* will be moved to Kconfig? What about #defines that will absolutely never used by anyone else? for example, I (may) have a code fragment that distinguishes between "CONFIG_CYGNUS" and "CONFIG_NSPLUS"...
Would these need to be Kconfig? or would that just clutter Kconfig with useless entries?

Well, at some point yes, pretty much everything will be in Kconfig, like the Linux Kernel does. I suppose one way to think of it is that if it's not something that should be configured but is rather a property of the device, it doesn't belong as CONFIG_ it should just be a define in the header. Take a look at the other NAND drivers which have been converted and be at least as converted as they are. More conversions are always appreciated but we have to keep at least a minimal level :) Thanks!

--
Tom

From: Jiandong Zheng <[email protected]>

Add support for the iproc NAND, and enable on Cygnus and NSP boards.

Signed-off-by Jiandong Zheng <[email protected]>

Signed-off-by Steve Rae <[email protected]>

---
There was a previous attempt to implement this "iproc NAND" (see: http://patchwork.ozlabs.org/patch/505399), however, due to the amount of changes required, it seemed better to implement the code in a series of steps. This is the first step, where the "iproc_nand.c" is essentially an empty file (with one function required to allow this commit to build successfully).

Quoted 23 lines · Steve Rae

Quoted 15 lines · Scott Wood

Quoted 6 lines · Steve Rae

From: Jiandong Zheng <[email protected]>

Add support for the iproc NAND, and enable on Cygnus and NSP boards.

Signed-off-by Jiandong Zheng <[email protected]>

Signed-off-by Steve Rae <[email protected]>

Quoted 7 lines

---
There was a previous attempt to implement this "iproc NAND" (see: http://patchwork.ozlabs.org/patch/505399), however, due to the amount of changes required, it seemed better to implement the code in a series of steps. This is the first step, where the "iproc_nand.c" is essentially an empty file (with one function required to allow this commit to build successfully).

Quoted 6 lines

I don't see the value of applying a such a do-nothing patch. It's fine to leave out unnecessary features, things that improve performance, etc. but to
be applied a patchset should accomplish something useful and correct, not just
be a staging area for future patches.

Quoted 5 lines

I agree -- but with the previous attempt, there where so many things that went wrong, that I cannot comprehend what is needed.
So, I wanted to break it down into pieces, so that I could get clear responses to help me get it right.
right now, I understand that I need to move certain defines to Kconfig ....

Go through the previous comments and address (or respond to) them one by one, then submit again. If you want to break it into multiple patches, that's fine as long as the intermediate states are sane, but it should all be submitted at once as part of a patchset (again, except for unnecessary features).

Also, please keep discussion on the mailing list.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +0 -0

index 3c07160..3bf7395 100644
@@ -10,6 +10,7 @@
 #include <asm/iproc-common/configs.h>
 

Quoted 6 lines

Quoted 4 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h line 13

Quoted 2 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +1 -0

 /* uArchitecture specifics */
+#include <../../../drivers/mtd/nand/iproc_nand_cygnus.h>

No.

this "include" line is unacceptable?

Using "../../.." to reach into a code directory's private headers is unacceptable, yes.

Quoted 1 line

could you propose something that would work?

If you want the info to be in the driver directory, use an ifdef there, based on a config symbol. This seems like the better approach.

If you want the info to come via the config header, move it to someplace the config header can properly include.

-Scott

Quoted 36 lines · Scott Wood

Quoted 28 lines · Steve Rae

Quoted 17 lines · Scott Wood

Quoted 6 lines · Steve Rae

From: Jiandong Zheng <[email protected]>

Add support for the iproc NAND, and enable on Cygnus and NSP boards.

Signed-off-by Jiandong Zheng <[email protected]>

Signed-off-by Steve Rae <[email protected]>

Quoted 9 lines

---
There was a previous attempt to implement this "iproc NAND" (see: http://patchwork.ozlabs.org/patch/505399), however, due to the amount of changes required, it seemed better to implement the code in a series of steps. This is the first step, where the "iproc_nand.c"
is essentially an empty file (with one function required to allow this
commit to build successfully).

Quoted 9 lines

I don't see the value of applying a such a do-nothing patch. It's fine to
leave out unnecessary features, things that improve performance, etc. but
to
be applied a patchset should accomplish something useful and correct, not
just
be a staging area for future patches.

Quoted 6 lines

I agree -- but with the previous attempt, there where so many things that went wrong, that I cannot comprehend what is needed. So, I wanted to break it down into pieces, so that I could get clear responses to help me get it right.
right now, I understand that I need to move certain defines to Kconfig
....

Quoted 7 lines

Go through the previous comments and address (or respond to) them one by one,
then submit again. If you want to break it into multiple patches, that's fine
as long as the intermediate states are sane, but it should all be submitted at
once as part of a patchset (again, except for unnecessary features).

OK - that was my plan (to address every previous comment)... I was hoping to get "incremental" comments to indicate that I was resolving them acceptably...
My plan was to increment v2, v3, vxxx until it was acceptable. Would this be OK?

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +0 -0

index 3c07160..3bf7395 100644
@@ -10,6 +10,7 @@
 #include <asm/iproc-common/configs.h>
 

Quoted 6 lines

Quoted 4 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h line 13

Quoted 2 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +1 -0

 /* uArchitecture specifics */
+#include <../../../drivers/mtd/nand/iproc_nand_cygnus.h>

No.

this "include" line is unacceptable?

Quoted 8 lines

Using "../../.." to reach into a code directory's private headers is unacceptable, yes.

Quoted 1 line

could you propose something that would work?

If you want the info to be in the driver directory, use an ifdef there, based
on a config symbol. This seems like the better approach.

Maybe I misinterpreted the comments related to:

(quoted diff) +7 -0

+#if defined(CONFIG_CYGNUS)
+#include "iproc_nand_cygnus.h"
+#elif defined(CONFIG_NS_PLUS)
+#include "iproc_nand_ns_plus.h"
+#else
+#error "Unsupported configuration"
+#endif

Scott - I thought the you did not like this logic -- now I am thinking you didn't like the "CONFIG_*" names....
I'm guessing that the names should be:
CONFIG_SYS_BCM_CYGNUS
CONFIG_SYS_BCM_NSPLUS
and that they should be added to Kconfig?

Quoted 45 lines · Steve Rae

Quoted 36 lines · Scott Wood

Quoted 28 lines · Steve Rae

Quoted 17 lines · Scott Wood

Quoted 6 lines · Steve Rae

From: Jiandong Zheng <[email protected]>

Add support for the iproc NAND, and enable on Cygnus and NSP boards.

Signed-off-by Jiandong Zheng <[email protected]>

Signed-off-by Steve Rae <[email protected]>

Quoted 9 lines

---
There was a previous attempt to implement this "iproc NAND" (see: http://patchwork.ozlabs.org/patch/505399), however, due to the amount of changes required, it seemed better to implement the code in a series of steps. This is the first step, where the "iproc_nand.c"
is essentially an empty file (with one function required to allow this
commit to build successfully).

Quoted 9 lines

I don't see the value of applying a such a do-nothing patch. It's fine to
leave out unnecessary features, things that improve performance, etc. but
to
be applied a patchset should accomplish something useful and correct, not
just
be a staging area for future patches.

Quoted 6 lines

I agree -- but with the previous attempt, there where so many things that went wrong, that I cannot comprehend what is needed. So, I wanted to break it down into pieces, so that I could get clear responses to help me get it right.
right now, I understand that I need to move certain defines to Kconfig
....

Quoted 7 lines

Go through the previous comments and address (or respond to) them one by one,
then submit again. If you want to break it into multiple patches, that's fine
as long as the intermediate states are sane, but it should all be submitted at
once as part of a patchset (again, except for unnecessary features).

Quoted 5 lines

OK - that was my plan (to address every previous comment)... I was hoping to get "incremental" comments to indicate that I was resolving them acceptably...
My plan was to increment v2, v3, vxxx until it was acceptable. Would this be OK?

It's OK if you mark them as [RFC PATCH] so it's clear that you don't mean them to be applied, only commented on -- and include a TODO list so we know what you plan to address before dropping the "RFC".

Or just include a code fragment when replying to feedback, with a comment like, "Is this what you're looking for?"

Quoted 34 lines

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +0 -0

index 3c07160..3bf7395 100644
@@ -10,6 +10,7 @@
 #include <asm/iproc-common/configs.h>
 

Quoted 6 lines

Quoted 4 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h line 13

Quoted 2 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +1 -0

 /* uArchitecture specifics */
+#include <../../../drivers/mtd/nand/iproc_nand_cygnus.h>

No.

this "include" line is unacceptable?

Quoted 8 lines

Using "../../.." to reach into a code directory's private headers is unacceptable, yes.

Quoted 1 line

could you propose something that would work?

If you want the info to be in the driver directory, use an ifdef there, based
on a config symbol. This seems like the better approach.

Maybe I misinterpreted the comments related to:

(quoted diff) +7 -0

+#if defined(CONFIG_CYGNUS)
+#include "iproc_nand_cygnus.h"
+#elif defined(CONFIG_NS_PLUS)
+#include "iproc_nand_ns_plus.h"
+#else
+#error "Unsupported configuration"
+#endif

Quoted 6 lines

Scott - I thought the you did not like this logic -- now I am thinking you didn't like the "CONFIG_*" names....
I'm guessing that the names should be:
CONFIG_SYS_BCM_CYGNUS
CONFIG_SYS_BCM_NSPLUS
and that they should be added to Kconfig?

Correct.

-Scott

Quoted 45 lines · Steve Rae

Quoted 36 lines · Scott Wood

Quoted 28 lines · Steve Rae

Quoted 17 lines · Scott Wood

Quoted 6 lines · Steve Rae

From: Jiandong Zheng <[email protected]>

Add support for the iproc NAND, and enable on Cygnus and NSP boards.

Signed-off-by Jiandong Zheng <[email protected]>

Signed-off-by Steve Rae <[email protected]>

Quoted 9 lines

---
There was a previous attempt to implement this "iproc NAND" (see: http://patchwork.ozlabs.org/patch/505399), however, due to the amount of changes required, it seemed better to implement the code in a series of steps. This is the first step, where the "iproc_nand.c"
is essentially an empty file (with one function required to allow this
commit to build successfully).

Quoted 9 lines

I don't see the value of applying a such a do-nothing patch. It's fine to
leave out unnecessary features, things that improve performance, etc. but
to
be applied a patchset should accomplish something useful and correct, not
just
be a staging area for future patches.

Quoted 6 lines

I agree -- but with the previous attempt, there where so many things that went wrong, that I cannot comprehend what is needed. So, I wanted to break it down into pieces, so that I could get clear responses to help me get it right.
right now, I understand that I need to move certain defines to Kconfig
....

Quoted 7 lines

Go through the previous comments and address (or respond to) them one by one,
then submit again. If you want to break it into multiple patches, that's fine
as long as the intermediate states are sane, but it should all be submitted at
once as part of a patchset (again, except for unnecessary features).

Quoted 5 lines

OK - that was my plan (to address every previous comment)... I was hoping to get "incremental" comments to indicate that I was resolving them acceptably...
My plan was to increment v2, v3, vxxx until it was acceptable. Would this be OK?

It's OK if you mark them as [RFC PATCH] so it's clear that you don't mean them to be applied, only commented on -- and include a TODO list so we know what you plan to address before dropping the "RFC".

Or just include a code fragment when replying to feedback, with a comment like, "Is this what you're looking for?"

Quoted 34 lines

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +0 -0

index 3c07160..3bf7395 100644
@@ -10,6 +10,7 @@
 #include <asm/iproc-common/configs.h>
 

Quoted 6 lines

Quoted 4 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h line 13

Quoted 2 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +1 -0

 /* uArchitecture specifics */
+#include <../../../drivers/mtd/nand/iproc_nand_cygnus.h>

No.

this "include" line is unacceptable?

Quoted 8 lines

Using "../../.." to reach into a code directory's private headers is unacceptable, yes.

Quoted 1 line

could you propose something that would work?

If you want the info to be in the driver directory, use an ifdef there, based
on a config symbol. This seems like the better approach.

Maybe I misinterpreted the comments related to:

(quoted diff) +7 -0

+#if defined(CONFIG_CYGNUS)
+#include "iproc_nand_cygnus.h"
+#elif defined(CONFIG_NS_PLUS)
+#include "iproc_nand_ns_plus.h"
+#else
+#error "Unsupported configuration"
+#endif

Quoted 6 lines

Scott - I thought the you did not like this logic -- now I am thinking you didn't like the "CONFIG_*" names....
I'm guessing that the names should be:
CONFIG_SYS_BCM_CYGNUS
CONFIG_SYS_BCM_NSPLUS
and that they should be added to Kconfig?

Correct.

-Scott

Quoted 102 lines · Steve Rae

Quoted 45 lines · Scott Wood

Quoted 36 lines · Steve Rae

Quoted 28 lines · Scott Wood

Quoted 17 lines · Steve Rae

Quoted 6 lines

From: Jiandong Zheng <[email protected]>

Add support for the iproc NAND, and enable on Cygnus and NSP boards.

Signed-off-by Jiandong Zheng <[email protected]>

Signed-off-by Steve Rae <[email protected]>

Quoted 9 lines

---
There was a previous attempt to implement this "iproc NAND" (see: http://patchwork.ozlabs.org/patch/505399), however, due to the amount of changes required, it seemed better to implement the code in a series of steps. This is the first step, where the "iproc_nand.c"
is essentially an empty file (with one function required to allow this
commit to build successfully).

Quoted 9 lines

I don't see the value of applying a such a do-nothing patch. It's fine to
leave out unnecessary features, things that improve performance, etc. but
to
be applied a patchset should accomplish something useful and correct, not
just
be a staging area for future patches.

Quoted 6 lines

I agree -- but with the previous attempt, there where so many things that went wrong, that I cannot comprehend what is needed. So, I wanted to break it down into pieces, so that I could get clear responses to help me get it right.
right now, I understand that I need to move certain defines to Kconfig
....

Quoted 7 lines

Go through the previous comments and address (or respond to) them one by one,
then submit again. If you want to break it into multiple patches, that's fine
as long as the intermediate states are sane, but it should all be submitted at
once as part of a patchset (again, except for unnecessary features).

Quoted 5 lines

OK - that was my plan (to address every previous comment)... I was hoping to get "incremental" comments to indicate that I was resolving them acceptably...
My plan was to increment v2, v3, vxxx until it was acceptable. Would this be OK?

It's OK if you mark them as [RFC PATCH] so it's clear that you don't mean them to be applied, only commented on -- and include a TODO list so we know what you plan to address before dropping the "RFC".

Or just include a code fragment when replying to feedback, with a comment like, "Is this what you're looking for?"

Quoted 34 lines

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +0 -0

index 3c07160..3bf7395 100644
@@ -10,6 +10,7 @@
 #include <asm/iproc-common/configs.h>
 

Quoted 6 lines

Quoted 4 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h line 13

Quoted 2 lines

arch/arm/include/asm/arch-bcmcygnus/configs.h +1 -0

 /* uArchitecture specifics */
+#include <../../../drivers/mtd/nand/iproc_nand_cygnus.h>

No.

this "include" line is unacceptable?

Quoted 8 lines

Using "../../.." to reach into a code directory's private headers is unacceptable, yes.

Quoted 1 line

could you propose something that would work?

If you want the info to be in the driver directory, use an ifdef there, based
on a config symbol. This seems like the better approach.

Maybe I misinterpreted the comments related to:

(quoted diff) +7 -0

+#if defined(CONFIG_CYGNUS)
+#include "iproc_nand_cygnus.h"
+#elif defined(CONFIG_NS_PLUS)
+#include "iproc_nand_ns_plus.h"
+#else
+#error "Unsupported configuration"
+#endif

Quoted 6 lines

Scott - I thought the you did not like this logic -- now I am thinking you didn't like the "CONFIG_*" names....
I'm guessing that the names should be:
CONFIG_SYS_BCM_CYGNUS
CONFIG_SYS_BCM_NSPLUS
and that they should be added to Kconfig?

Correct.

Quoted 1 line

-Scott