[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Scott Wood]

The mtdparts code depends on the devices referred to by
partition specs actually existing, both for error checking,
when the spread feature is used, for bad block checking.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 common/cmd_mtdparts.c |    5 +----
 include/nand.h        |    6 ++++++
 2 files changed, 7 insertions(+), 4 deletions(-)

Applied to u-boot-nand-flash

diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
index 5481c88..8e8ba47 100644
--- a/common/cmd_mtdparts.c
+++ b/common/cmd_mtdparts.c
@@ -96,15 +96,11 @@
 #include <linux/err.h>
 #include <linux/mtd/mtd.h>
 
-#if defined(CONFIG_CMD_NAND)
 #include <linux/mtd/nand.h>
 #include <nand.h>
-#endif
 
-#if defined(CONFIG_CMD_ONENAND)
 #include <linux/mtd/onenand.h>
 #include <onenand_uboot.h>
-#endif
 
 /* special size referring to all the remaining space in a partition */
 #define SIZE_REMAINING		0xFFFFFFFF
@@ -1711,6 +1707,7 @@ int mtdparts_init(void)
 		memset(last_ids, 0, MTDIDS_MAXLEN);
 		memset(last_parts, 0, MTDPARTS_MAXLEN);
 		memset(last_partition, 0, PARTITION_MAXLEN);
+		nand_init();
 		initialized = 1;
 	}
 
diff --git a/include/nand.h b/include/nand.h
index a452411..5ced821 100644
--- a/include/nand.h
+++ b/include/nand.h
@@ -24,7 +24,13 @@
 #ifndef _NAND_H_
 #define _NAND_H_
 
+#ifdef CONFIG_CMD_NAND
 extern void nand_init(void);
+#else
+static inline void nand_init(void)
+{
+}
+#endif
 
 #include <linux/mtd/compat.h>
 #include <linux/mtd/mtd.h>
-- 
1.7.0.4

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Wolfgang Denk]

Dear Scott Wood,

In message <20101015185902.GA7581@udp111988uds.am.freescale.net> you wrote:
> The mtdparts code depends on the devices referred to by
> partition specs actually existing, both for error checking,
> when the spread feature is used, for bad block checking.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
>  common/cmd_mtdparts.c |    5 +----
>  include/nand.h        |    6 ++++++
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> Applied to u-boot-nand-flash
> 
> diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
> index 5481c88..8e8ba47 100644
> --- a/common/cmd_mtdparts.c
> +++ b/common/cmd_mtdparts.c
> @@ -96,15 +96,11 @@
>  #include <linux/err.h>
>  #include <linux/mtd/mtd.h>
>  
> -#if defined(CONFIG_CMD_NAND)
>  #include <linux/mtd/nand.h>
>  #include <nand.h>
> -#endif
>  
> -#if defined(CONFIG_CMD_ONENAND)
>  #include <linux/mtd/onenand.h>
>  #include <onenand_uboot.h>
> -#endif

I don't like this.  We should include this stuff only on systems that
really need it.

>  /* special size referring to all the remaining space in a partition */
>  #define SIZE_REMAINING		0xFFFFFFFF
> @@ -1711,6 +1707,7 @@ int mtdparts_init(void)
>  		memset(last_ids, 0, MTDIDS_MAXLEN);
>  		memset(last_parts, 0, MTDPARTS_MAXLEN);
>  		memset(last_partition, 0, PARTITION_MAXLEN);
> +		nand_init();
>  		initialized = 1;
>  	}

I don't like this either.  I don't want to see a nand_init() for
systems that have no NAND at all (not even an empty one).


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If you can, help others. If you can't, at least don't hurt  others."
- the Dalai Lama

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Scott Wood]

On Fri, 15 Oct 2010 21:36:40 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Scott Wood,
> 
> In message <20101015185902.GA7581@udp111988uds.am.freescale.net> you wrote:
> > The mtdparts code depends on the devices referred to by
> > partition specs actually existing, both for error checking,
> > when the spread feature is used, for bad block checking.
> > 
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> > ---
> >  common/cmd_mtdparts.c |    5 +----
> >  include/nand.h        |    6 ++++++
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > Applied to u-boot-nand-flash
> > 
> > diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
> > index 5481c88..8e8ba47 100644
> > --- a/common/cmd_mtdparts.c
> > +++ b/common/cmd_mtdparts.c
> > @@ -96,15 +96,11 @@
> >  #include <linux/err.h>
> >  #include <linux/mtd/mtd.h>
> >  
> > -#if defined(CONFIG_CMD_NAND)
> >  #include <linux/mtd/nand.h>
> >  #include <nand.h>
> > -#endif
> >  
> > -#if defined(CONFIG_CMD_ONENAND)
> >  #include <linux/mtd/onenand.h>
> >  #include <onenand_uboot.h>
> > -#endif
> 
> I don't like this.  We should include this stuff only on systems that
> really need it.

Since when do we ifdef headers?

> >  /* special size referring to all the remaining space in a partition */
> >  #define SIZE_REMAINING		0xFFFFFFFF
> > @@ -1711,6 +1707,7 @@ int mtdparts_init(void)
> >  		memset(last_ids, 0, MTDIDS_MAXLEN);
> >  		memset(last_parts, 0, MTDPARTS_MAXLEN);
> >  		memset(last_partition, 0, PARTITION_MAXLEN);
> > +		nand_init();
> >  		initialized = 1;
> >  	}
> 
> I don't like this either.  I don't want to see a nand_init() for
> systems that have no NAND at all (not even an empty one).

What would you suggest instead?

-Scott

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Mike Frysinger]

On Friday, October 15, 2010 15:36:40 Wolfgang Denk wrote:
> Scott Wood wrote:
> > The mtdparts code depends on the devices referred to by
> > partition specs actually existing, both for error checking,
> > when the spread feature is used, for bad block checking.
> > 
> > @@ -1711,6 +1707,7 @@ int mtdparts_init(void)
> >  		memset(last_ids, 0, MTDIDS_MAXLEN);
> >  		memset(last_parts, 0, MTDPARTS_MAXLEN);
> >  		memset(last_partition, 0, PARTITION_MAXLEN);
> > 
> > +		nand_init();
> >  		initialized = 1;
> >  	}
> 
> I don't like this either.  I don't want to see a nand_init() for
> systems that have no NAND at all (not even an empty one).

i disagree ... sprinkling #ifdef's throughout the code makes it a lot harder 
to read, maintain, and validate across multiple configurations.  you're 
suggesting we do:
#ifdef CONFIG_CMD_NAND
		nand_init();
#endif

it makes more sense to me to hide this in the header (which Scott has done) 
and let the compiler/code optimize dead crap away.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101015/eaabdd58/attachment.pgp 

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Wolfgang Denk]

Dear Mike Frysinger,

In message <201010151608.30637.vapier@gentoo.org> you wrote:
>
> > > @@ -1711,6 +1707,7 @@ int mtdparts_init(void)
> > >  		memset(last_ids, 0, MTDIDS_MAXLEN);
> > >  		memset(last_parts, 0, MTDPARTS_MAXLEN);
> > >  		memset(last_partition, 0, PARTITION_MAXLEN);
> > > 
> > > +		nand_init();
> > >  		initialized = 1;
> > >  	}
> > 
> > I don't like this either.  I don't want to see a nand_init() for
> > systems that have no NAND at all (not even an empty one).
>
> i disagree ... sprinkling #ifdef's throughout the code makes it a lot harder 
> to read, maintain, and validate across multiple configurations.  you're 
> suggesting we do:
> #ifdef CONFIG_CMD_NAND
> 		nand_init();
> #endif

No, I'm not. I did not suggest anything like that.

> it makes more sense to me to hide this in the header (which Scott has done)>  
> and let the compiler/code optimize dead crap away.

Why do we need an explicit call to nand_init() at all?

Why cannot the NAND routines check internally if they have been
initialized yet, and run nand_init() if and when needed?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The use of COBOL cripples the mind; its teaching  should,  therefore,
be regarded as a criminal offense.                   - E. W. Dijkstra

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Scott Wood]

On Fri, 15 Oct 2010 23:39:52 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Mike Frysinger,
> 
> > it makes more sense to me to hide this in the header (which Scott has done)>  
> > and let the compiler/code optimize dead crap away.
> 
> Why do we need an explicit call to nand_init() at all?
> 
> Why cannot the NAND routines check internally if they have been
> initialized yet, and run nand_init() if and when needed?

mtdparts doesn't make any calls directly to NAND routines (other than
this new call to nand_init).  It looks for registered MTD devices.

Until nand_init runs, you won't have any NAND mtd devices -- and no
NAND function will be invoked where such initialization could be done.

-Scott

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Wolfgang Denk]

Dear Scott Wood,

In message <20101015164801.3fd031b7@udp111988uds.am.freescale.net> you wrote:
>
> > > it makes more sense to me to hide this in the header (which Scott has done)>  
> > > and let the compiler/code optimize dead crap away.
> > 
> > Why do we need an explicit call to nand_init() at all?
> > 
> > Why cannot the NAND routines check internally if they have been
> > initialized yet, and run nand_init() if and when needed?
> 
> mtdparts doesn't make any calls directly to NAND routines (other than
> this new call to nand_init).  It looks for registered MTD devices.
> 
> Until nand_init runs, you won't have any NAND mtd devices -- and no
> NAND function will be invoked where such initialization could be done.

This looks like a broken design to me.


Assume we add this call here; would it then not also be needed in the
'static' version of mtdparts_init() in "common/cmd_jffs2.c" (whatever
'static' is supposed to mean) ?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In the realm of scientific observation, luck is granted only to those
who are prepared.                                     - Louis Pasteur

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Scott Wood]

On Sat, 16 Oct 2010 00:17:01 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Scott Wood,
> 
> In message <20101015164801.3fd031b7@udp111988uds.am.freescale.net> you wrote:
> >
> > > Why do we need an explicit call to nand_init() at all?
> > > 
> > > Why cannot the NAND routines check internally if they have been
> > > initialized yet, and run nand_init() if and when needed?
> > 
> > mtdparts doesn't make any calls directly to NAND routines (other than
> > this new call to nand_init).  It looks for registered MTD devices.
> > 
> > Until nand_init runs, you won't have any NAND mtd devices -- and no
> > NAND function will be invoked where such initialization could be done.
> 
> This looks like a broken design to me.

What would you rather see in its place?

> Assume we add this call here; would it then not also be needed in the
> 'static' version of mtdparts_init() in "common/cmd_jffs2.c" (whatever
> 'static' is supposed to mean) ?

Yes, it seems so.  Is there a good reason why jffs2 has its own
implementation of this stuff?

yaffs_StartUp is in a similar situation.

-Scott

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Wolfgang Denk]

Dear Scott Wood,

In message <20101015173533.4e1d8bcb@udp111988uds.am.freescale.net> you wrote:
>
> > This looks like a broken design to me.
> 
> What would you rather see in its place?

Good question. I have to admit that I don't know all this code and
it's embroilments too well. There is this with MTD and that without,
there is this for JFFS2 and that without. I lost track in that maze
long ago.

Eventually we should have some mtd_init() call which does this, but
then, I'm not sure if there might not be a case ot mtdparts without
MTD.

> > Assume we add this call here; would it then not also be needed in the
> > 'static' version of mtdparts_init() in "common/cmd_jffs2.c" (whatever
> > 'static' is supposed to mean) ?
> 
> Yes, it seems so.  Is there a good reason why jffs2 has its own
> implementation of this stuff?

Too many people working on differnt parts of the code, without
looking over their respective rims ?  Sorry, I never understood.
I think Stefan spent already some time to clean up parts of the mess,
but this probably needs more effort.

> yaffs_StartUp is in a similar situation.

Oh dear. It doesn't come to an end.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What is wanted is not the will to believe,  but the will to find out,
which is the exact opposite.
		        -- Bertrand Russell, "Skeptical Essays", 1928

[U-Boot] [PATCH] mtdparts: Call nand_init() during mtdparts_init().

[Scott Wood]

On Sat, 16 Oct 2010 00:48:11 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Scott Wood,
> 
> In message <20101015173533.4e1d8bcb@udp111988uds.am.freescale.net> you wrote:
> >
> > > This looks like a broken design to me.
> > 
> > What would you rather see in its place?
> 
> Good question. I have to admit that I don't know all this code and
> it's embroilments too well. There is this with MTD and that without,
> there is this for JFFS2 and that without. I lost track in that maze
> long ago.
> 
> Eventually we should have some mtd_init() call which does this, 

OK, so replace the nand_init() with a call to a new mtd_init(), that
currently just calls nand_init()?

The parts that access nand_info[] directly rather than using the MTD
interface would use nand_init() directly.

> but then, I'm not sure if there might not be a case ot mtdparts without
> MTD.

There's an existing dependency there -- if that dependency is
conditionalized/removed in the future, then doing the same to its call
to mtd_init() would be part of that.

> > > Assume we add this call here; would it then not also be needed in the
> > > 'static' version of mtdparts_init() in "common/cmd_jffs2.c" (whatever
> > > 'static' is supposed to mean) ?
> > 
> > Yes, it seems so.  Is there a good reason why jffs2 has its own
> > implementation of this stuff?
> 
> Too many people working on differnt parts of the code, without
> looking over their respective rims ?  Sorry, I never understood.
> I think Stefan spent already some time to clean up parts of the mess,
> but this probably needs more effort.
> 
> > yaffs_StartUp is in a similar situation.
> 
> Oh dear. It doesn't come to an end.

Some grepping turns up omap_nand_switch_ecc, and lcd_show_board_info in
a few boards -- but I think that's it.

-Scott