Re: [U-Boot] [PATCH 1/4] arm: iproc: add NAND driver
On Fri, Mar 11, 2016 at 11:55 AM, Scott Wood oss@buserror.net wrote:
On Fri, 2016-03-11 at 11:47 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:29 AM, Scott Wood oss@buserror.net wrote:
On Thu, 2016-03-10 at 14:26 -0800, Steve Rae wrote:
From: Jiandong Zheng jdzheng@broadcom.com
Add support for the iproc NAND, and enable on Cygnus and NSP boards.
Signed-off-by: Jiandong Zheng jdzheng@broadcom.com Signed-off-by: Steve Rae srae@broadcom.com
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).
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.
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).
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?
Also, please keep discussion on the mailing list.
Sorry....
diff --git a/arch/arm/include/asm/arch-bcmcygnus/configs.h b/arch/arm/include/asm/arch-bcmcygnus/configs.h index 3c07160..3bf7395 100644 --- a/arch/arm/include/asm/arch-bcmcygnus/configs.h +++ b/arch/arm/include/asm/arch-bcmcygnus/configs.h @@ -10,6 +10,7 @@ #include <asm/iproc-common/configs.h>
/* 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.
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:
+#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?
If you want the info to come via the config header, move it to someplace the config header can properly include.
-Scott
On Fri, 2016-03-11 at 12:13 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:55 AM, Scott Wood oss@buserror.net wrote:
On Fri, 2016-03-11 at 11:47 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:29 AM, Scott Wood oss@buserror.net wrote:
On Thu, 2016-03-10 at 14:26 -0800, Steve Rae wrote:
From: Jiandong Zheng jdzheng@broadcom.com
Add support for the iproc NAND, and enable on Cygnus and NSP boards.
Signed-off-by: Jiandong Zheng jdzheng@broadcom.com Signed-off-by: Steve Rae srae@broadcom.com
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).
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.
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).
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?"
diff --git a/arch/arm/include/asm/arch-bcmcygnus/configs.h b/arch/arm/include/asm/arch-bcmcygnus/configs.h index 3c07160..3bf7395 100644 --- a/arch/arm/include/asm/arch-bcmcygnus/configs.h +++ b/arch/arm/include/asm/arch-bcmcygnus/configs.h @@ -10,6 +10,7 @@ #include <asm/iproc-common/configs.h>
/* 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.
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:
+#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?
Correct.
-Scott
On Fri, Mar 11, 2016 at 12:18 PM, Scott Wood oss@buserror.net wrote:
On Fri, 2016-03-11 at 12:13 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:55 AM, Scott Wood oss@buserror.net wrote:
On Fri, 2016-03-11 at 11:47 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:29 AM, Scott Wood oss@buserror.net wrote:
On Thu, 2016-03-10 at 14:26 -0800, Steve Rae wrote:
From: Jiandong Zheng jdzheng@broadcom.com
Add support for the iproc NAND, and enable on Cygnus and NSP boards.
Signed-off-by: Jiandong Zheng jdzheng@broadcom.com Signed-off-by: Steve Rae srae@broadcom.com
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).
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.
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).
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?"
I understand - I think I can proceed now -- THANKS!
diff --git a/arch/arm/include/asm/arch-bcmcygnus/configs.h b/arch/arm/include/asm/arch-bcmcygnus/configs.h index 3c07160..3bf7395 100644 --- a/arch/arm/include/asm/arch-bcmcygnus/configs.h +++ b/arch/arm/include/asm/arch-bcmcygnus/configs.h @@ -10,6 +10,7 @@ #include <asm/iproc-common/configs.h>
/* 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.
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:
+#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?
Correct.
-Scott
On Sat, Mar 12, 2016 at 9:18 AM, Scott Wood oss@buserror.net wrote:
On Fri, 2016-03-11 at 12:13 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:55 AM, Scott Wood oss@buserror.net wrote:
On Fri, 2016-03-11 at 11:47 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:29 AM, Scott Wood oss@buserror.net wrote:
On Thu, 2016-03-10 at 14:26 -0800, Steve Rae wrote:
From: Jiandong Zheng jdzheng@broadcom.com
Add support for the iproc NAND, and enable on Cygnus and NSP boards.
Signed-off-by: Jiandong Zheng jdzheng@broadcom.com Signed-off-by: Steve Rae srae@broadcom.com
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).
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.
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).
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?"
diff --git a/arch/arm/include/asm/arch-bcmcygnus/configs.h b/arch/arm/include/asm/arch-bcmcygnus/configs.h index 3c07160..3bf7395 100644 --- a/arch/arm/include/asm/arch-bcmcygnus/configs.h +++ b/arch/arm/include/asm/arch-bcmcygnus/configs.h @@ -10,6 +10,7 @@ #include <asm/iproc-common/configs.h>
/* 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.
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:
+#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?
Correct.
-Scott
Hi Steve,
Where did this get to? I find myself in need of a NAND driver for a BCM58525 and this seems to be relevant.
(trying newer address for Steve, sorry for the spam)
On Sat, Mar 12, 2016 at 9:18 AM, Scott Wood oss@buserror.net wrote:
On Fri, 2016-03-11 at 12:13 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:55 AM, Scott Wood oss@buserror.net wrote:
On Fri, 2016-03-11 at 11:47 -0800, Steve Rae wrote:
On Fri, Mar 11, 2016 at 11:29 AM, Scott Wood oss@buserror.net wrote:
On Thu, 2016-03-10 at 14:26 -0800, Steve Rae wrote: > From: Jiandong Zheng jdzheng@broadcom.com > > Add support for the iproc NAND, and enable on Cygnus and NSP boards. > > Signed-off-by: Jiandong Zheng jdzheng@broadcom.com > Signed-off-by: Steve Rae srae@broadcom.com > --- > 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).
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.
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).
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?"
> diff --git a/arch/arm/include/asm/arch-bcmcygnus/configs.h > b/arch/arm/include/asm/arch-bcmcygnus/configs.h > index 3c07160..3bf7395 100644 > --- a/arch/arm/include/asm/arch-bcmcygnus/configs.h > +++ b/arch/arm/include/asm/arch-bcmcygnus/configs.h > @@ -10,6 +10,7 @@ > #include <asm/iproc-common/configs.h> > > /* 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.
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:
+#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?
Correct.
-Scott
Hi Steve,
Where did this get to? I find myself in need of a NAND driver for a BCM58525 and this seems to be relevant.
participants (3)
-
Chris Packham -
Scott Wood -
Steve Rae