| Message ID | 1457648790-2112-1-git-send-email-srae@broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Mar 10, 2016 at 02:26:27PM -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). > > 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.
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? Thanks for clarifying! On Fri, Mar 11, 2016 at 10:02 AM, Tom Rini <trini@konsulko.com> wrote: > On Thu, Mar 10, 2016 at 02:26:27PM -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). >> >> 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
On Fri, Mar 11, 2016 at 10:07:46AM -0800, Steve Rae wrote: > 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!
Thanks for this clarification... On Fri, Mar 11, 2016 at 10:18 AM, Tom Rini <trini@konsulko.com> wrote: > On Fri, Mar 11, 2016 at 10:07:46AM -0800, Steve Rae wrote: > >> 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
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. > 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. -Scott
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.
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> /* Serial Info */ /* Post pad 3 bytes after each reg addr */ @@ -33,4 +34,14 @@ #define CONFIG_CMD_PING #define CONFIG_CMD_MII +/* NAND configuration */ +#define CONFIG_CMD_NAND +#define CONFIG_NAND_IPROC +#define CONFIG_SYS_NAND_IPROC_TIMING_MODE 5 +#define CONFIG_SYS_NAND_BASE 0 +#define CONFIG_SYS_MAX_NAND_DEVICE 1 +#define CONFIG_SYS_MAX_NAND_CHIPS 1 +#define CONFIG_SYS_NAND_SELF_INIT +#define CONFIG_SYS_NAND_ONFI_DETECTION + #endif /* __ARCH_CONFIGS_H */ diff --git a/arch/arm/include/asm/arch-bcmnsp/configs.h b/arch/arm/include/asm/arch-bcmnsp/configs.h index 786deae..aeaea6a 100644 --- a/arch/arm/include/asm/arch-bcmnsp/configs.h +++ b/arch/arm/include/asm/arch-bcmnsp/configs.h @@ -10,6 +10,7 @@ #include <asm/iproc-common/configs.h> /* uArchitecture specifics */ +#include <../../../drivers/mtd/nand/iproc_nand_ns_plus.h> /* Serial Info */ /* no padding */ @@ -19,4 +20,14 @@ #define CONFIG_CONS_INDEX 1 #define CONFIG_SYS_NS16550_COM1 0x18000300 +/* NAND configuration */ +#define CONFIG_CMD_NAND +#define CONFIG_NAND_IPROC +#define CONFIG_SYS_NAND_IPROC_TIMING_MODE 5 +#define CONFIG_SYS_NAND_BASE 0 +#define CONFIG_SYS_MAX_NAND_DEVICE 1 +#define CONFIG_SYS_MAX_NAND_CHIPS 1 +#define CONFIG_SYS_NAND_SELF_INIT +#define CONFIG_SYS_NAND_ONFI_DETECTION + #endif /* __ARCH_CONFIGS_H */ diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile index 6fb3718..bb3adbc 100644 --- a/drivers/mtd/nand/Makefile +++ b/drivers/mtd/nand/Makefile @@ -50,6 +50,7 @@ obj-$(CONFIG_NAND_FSL_ELBC) += fsl_elbc_nand.o obj-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_nand.o obj-$(CONFIG_NAND_FSL_UPM) += fsl_upm.o obj-$(CONFIG_NAND_FSMC) += fsmc_nand.o +obj-$(CONFIG_NAND_IPROC) += iproc_nand.o obj-$(CONFIG_NAND_JZ4740) += jz4740_nand.o obj-$(CONFIG_NAND_KB9202) += kb9202_nand.o obj-$(CONFIG_NAND_KIRKWOOD) += kirkwood_nand.o diff --git a/drivers/mtd/nand/iproc_nand.c b/drivers/mtd/nand/iproc_nand.c new file mode 100644 index 0000000..74f885a --- /dev/null +++ b/drivers/mtd/nand/iproc_nand.c @@ -0,0 +1,11 @@ +/* + * Copyright 2015 Broadcom Corporation. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> + +void board_nand_init(void) +{ +} diff --git a/drivers/mtd/nand/iproc_nand_cygnus.h b/drivers/mtd/nand/iproc_nand_cygnus.h new file mode 100644 index 0000000..26a00a9 --- /dev/null +++ b/drivers/mtd/nand/iproc_nand_cygnus.h @@ -0,0 +1,111 @@ +/* + * Copyright 2015 Broadcom Corporation. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _IPROC_NAND_CYGNUS_H_ +#define _IPROC_NAND_CYGNUS_H_ + +/* + * SoC specific definitions (Cygnus) + */ + +#define REG_NAND_BASE 0x18046000 +#define REG_NAND_IDM_BASE 0xf8105000 + +#define NAND_STRAP_TYPE_MASK 0x000f0000 +#define NAND_STRAP_TYPE_SHIFT 16 +#define NAND_STRAP_PAGE_MASK 0x00300000 +#define NAND_STRAP_PAGE_SHIFT 20 +#define NAND_STRAP_WIDTH_MASK 0x01000000 +#define NAND_STRAP_WIDTH_SHIFT 24 + +#define NAND_STRAP_TYPE_DATA \ +/* sector_1k, ecclevel, spare_size */ \ +{ \ + { 0, 0, 16 }, \ + { 0, 1, 16 }, \ + { 0, 4, 16 }, \ + { 0, 8, 16 }, \ + { 0, 8, 27 }, \ + { 0, 12, 27 }, \ + { 1, 12, 27 }, \ + { 1, 15, 27 }, \ + { 1, 20, 45 } \ +} + +#define NAND_STRAP_PAGE_DATA \ +{ \ + 1024, 2048, 4096, 8192 \ +} + +/* + * iProc NAND timing configurations for ONFI timing modes [0-5] + * + * Clock tick = 10ns + * Multiplier: + * x1: tWP tWH tRP tREH tCLH tALH + * x2: tCS tADL tWB tWHR + */ +#define NAND_TIMING_DATA \ +{ \ + /* ONFI timing mode 0 : */ \ + /* tWC=100ns tWP=50ns tWH=30ns */ \ + /* tRC=100ns tRP=50ns tREH=30ns */ \ + /* tCS=70ns tCLH=20ns tALH=20ns tADL=200ns */ \ + /* tWB=200ns tWHR=120ns tREA=40ns */ \ + { \ + .timing1 = 0x6565435b, \ + .timing2 = 0x00001e85, \ + }, \ + /* ONFI timing mode 1 : */ \ + /* tWC=45 tWP=25ns tWH=15ns */ \ + /* tRC=50 tRP=25ns tREH=15ns */ \ + /* tCS=35ns tCLH=10ns tALH=10ns tADL=100ns */ \ + /* tWB=100ns tWHR=80ns tREA=30ns */ \ + { \ + .timing1 = 0x33333236, \ + .timing2 = 0x00001064, \ + }, \ + /* ONFI timing mode 2 : */ \ + /* tWC=35ns tWP=17ns tWH=15ns */ \ + /* tRC=35ns tRP=17ns tREH=15ns */ \ + /* tCS=25ns tCLH=10ns tALH=10ns tADL=100ns */ \ + /* tWB=100ns tWHR=80ns tREA=25ns */ \ + { \ + .timing1 = 0x32322226, \ + .timing2 = 0x00001063, \ + }, \ + /* ONFI timing mode 3 : */ \ + /* tWC=30ns tWP=15ns tWH=10ns */ \ + /* tRC=30ns tRP=15ns tREH=10ns */ \ + /* tCS=25ns tCLH=5ns tALH=5ns tADL=100ns */ \ + /* tWB=100ns tWHR=60ns tREA=20ns */ \ + { \ + .timing1 = 0x22222126, \ + .timing2 = 0x00001043, \ + }, \ + /* ONFI timing mode 4 : */ \ + /* tWC=25ns tWP=12ns tWH=10ns */ \ + /* tRC=25ns tRP=12ns tREH=10ns */ \ + /* tCS=20ns tCLH=5ns tALH=5ns tADL=70ns */ \ + /* tWB=100ns tWHR=60ns tREA=20ns */ \ + { \ + .timing1 = 0x21212114, \ + .timing2 = 0x00001042, \ + }, \ + /* ONFI timing mode 5 : */ \ + /* tWC=20ns tWP=10ns tWH=7ns */ \ + /* tRC=20ns tRP=10ns tREH=7ns */ \ + /* tCS=15ns tCLH=5ns tALH=5ns tADL=70ns */ \ + /* tWB=100ns tWHR=60ns tREA=16ns */ \ + { \ + .timing1 = 0x11111114, \ + .timing2 = 0x00001042, \ + }, \ +} + +#define NAND_MAX_CS 2 + +#endif /* _IPROC_NAND_CYGNUS_H_ */ diff --git a/drivers/mtd/nand/iproc_nand_ns_plus.h b/drivers/mtd/nand/iproc_nand_ns_plus.h new file mode 100644 index 0000000..41d5da7 --- /dev/null +++ b/drivers/mtd/nand/iproc_nand_ns_plus.h @@ -0,0 +1,113 @@ +/* + * Copyright 2015 Broadcom Corporation. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _IPROC_NAND_NS_PLUS_H_ +#define _IPROC_NAND_NS_PLUS_H_ + +/* + * SoC specific definitions (NorthStar Plus) + */ + +#define REG_NAND_BASE 0x18026000 +#define REG_NAND_IDM_BASE 0x1811b000 +#define REG_NAND_STRAPS_BASE 0x1803f2a0 + +#define NAND_STRAP_TYPE_MASK 0x0000f000 +#define NAND_STRAP_TYPE_SHIFT 12 +#define NAND_STRAP_PAGE_MASK 0x00000c00 +#define NAND_STRAP_PAGE_SHIFT 10 +/* No bus width strap */ +#define NAND_STRAP_WIDTH_MASK 0x0 +#define NAND_STRAP_WIDTH_SHIFT 0 + +#define NAND_STRAP_TYPE_DATA \ +/* sector_1k, ecclevel, spare_size */ \ +{ \ + { 0, 0, 16 }, \ + { 0, 15, 16 }, /* Hamming ECC */ \ + { 0, 4, 16 }, \ + { 0, 8, 16 }, \ + { 0, 8, 27 }, \ + { 0, 12, 27 }, \ + { 1, 12, 27 }, \ + { 1, 15, 27 }, \ + { 1, 20, 45 } \ +} + +#define NAND_STRAP_PAGE_DATA \ +{ \ + 2048, 2048, 4096, 8192 \ +} + +/* + * iProc NAND timing configurations for ONFI timing modes [0-5] + * + * Clock tick = 4ns + * Multiplier: + * x1: tWP tWH tRP tREH tCLH tALH + * x4: tCS tADL tWB tWHR + */ +#define NAND_TIMING_DATA \ +{ \ + /* ONFI timing mode 0 : */ \ + /* tWC=100ns tWP=50ns tWH=30ns */ \ + /* tRC=100ns tRP=50ns tREH=30ns */ \ + /* tCS=70ns tCLH=20ns tALH=20ns tADL=200ns */ \ + /* tWB=200ns tWHR=120ns tREA=40ns */ \ + { \ + .timing1 = 0xfafa558d, \ + .timing2 = 0x00001a85, \ + }, \ + /* ONFI timing mode 1 : */ \ + /* tWC=45 tWP=25ns tWH=15ns */ \ + /* tRC=50 tRP=25ns tREH=15ns */ \ + /* tCS=35ns tCLH=10ns tALH=10ns tADL=100ns */ \ + /* tWB=100ns tWHR=80ns tREA=30ns */ \ + { \ + .timing1 = 0x85853347, \ + .timing2 = 0x00000e64, \ + }, \ + /* ONFI timing mode 2 : */ \ + /* tWC=35ns tWP=17ns tWH=15ns */ \ + /* tRC=35ns tRP=17ns tREH=15ns */ \ + /* tCS=25ns tCLH=10ns tALH=10ns tADL=100ns */ \ + /* tWB=100ns tWHR=80ns tREA=25ns */ \ + { \ + .timing1 = 0x54542347, \ + .timing2 = 0x00000e63, \ + }, \ + /* ONFI timing mode 3 : */ \ + /* tWC=30ns tWP=15ns tWH=10ns */ \ + /* tRC=30ns tRP=15ns tREH=10ns */ \ + /* tCS=25ns tCLH=5ns tALH=5ns tADL=100ns */ \ + /* tWB=100ns tWHR=60ns tREA=20ns */ \ + { \ + .timing1 = 0x44442237, \ + .timing2 = 0x00000e43, \ + }, \ + /* ONFI timing mode 4 : */ \ + /* tWC=25ns tWP=12ns tWH=10ns */ \ + /* tRC=25ns tRP=12ns tREH=10ns */ \ + /* tCS=20ns tCLH=5ns tALH=5ns tADL=70ns */ \ + /* tWB=100ns tWHR=60ns tREA=20ns */ \ + { \ + .timing1 = 0x43432235, \ + .timing2 = 0x00000e42, \ + }, \ + /* ONFI timing mode 5 : */ \ + /* tWC=20ns tWP=10ns tWH=7ns */ \ + /* tRC=20ns tRP=10ns tREH=7ns */ \ + /* tCS=15ns tCLH=5ns tALH=5ns tADL=70ns */ \ + /* tWB=100ns tWHR=60ns tREA=16ns */ \ + { \ + .timing1 = 0x32321225, \ + .timing2 = 0x00000e42, \ + }, \ +} + +#define NAND_MAX_CS 1 + +#endif /* _IPROC_NAND_NS_PLUS_H_ */