[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Jason McMullan]

Rewrite the nand_wait() FL_ERASING case to handle CFG_HZ values in the
MHZ range. This is needed for mips processors, as the timer's timebase
ticks at CPU clock frequency.

Signed-off-by: Jason McMullan <mcmullan@netapp.com>
---
 drivers/mtd/nand/nand_base.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 2da1d46..ac690ac 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -837,10 +837,17 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *this, int state)
 {
 	unsigned long	timeo;
 
+#if CFG_HZ > 100000
+	if (state == FL_ERASING)
+ 		timeo = (CFG_HZ / 1000) * 400;
+	else
+		timeo = (CFG_HZ / 1000) * 20;
+#else
 	if (state == FL_ERASING)
  		timeo = (CFG_HZ * 400) / 1000;
 	else
 		timeo = (CFG_HZ * 20) / 1000;
+#endif
 
 	if ((state == FL_ERASING) && (this->options & NAND_IS_AND))
 		this->cmdfunc(mtd, NAND_CMD_STATUS_MULTI, -1, -1);
-- 
1.5.4.3

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Scott Wood]

Jason McMullan wrote:
> Rewrite the nand_wait() FL_ERASING case to handle CFG_HZ values in the
> MHZ range. This is needed for mips processors, as the timer's timebase
> ticks at CPU clock frequency.

Even though it's MIPS that needs it, it should be flagged as a NAND 
patch since that's the code it touches.

> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 2da1d46..ac690ac 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -837,10 +837,17 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *this, int state)
>  {
>  	unsigned long	timeo;
>  
> +#if CFG_HZ > 100000
> +	if (state == FL_ERASING)
> + 		timeo = (CFG_HZ / 1000) * 400;
> +	else
> +		timeo = (CFG_HZ / 1000) * 20;
> +#else
>  	if (state == FL_ERASING)
>   		timeo = (CFG_HZ * 400) / 1000;
>  	else
>  		timeo = (CFG_HZ * 20) / 1000;
> +#endif


How about this?

if (state == FL_ERASING)
	timeo = CFG_HZ * 2 / 5;
else
	timeo = CFG_HZ / 50

If we have CFG_HZ values that are within a factor of 2 of wrapping 
around, the platform should probably do some downward scaling (or we 
should think about 64-bit timestamps)...

-Scott

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' valuesin the MHZ range for NAND delays

[McMullan, Jason]

On Mon, 2008-05-19 at 15:26 -0500, Scott Wood wrote:
> Even though it's MIPS that needs it, it should be flagged as a NAND 
> patch since that's the code it touches.

Totally agree.

> How about this?
> 
> if (state == FL_ERASING)
> 	timeo = CFG_HZ * 2 / 5;
> else
> 	timeo = CFG_HZ / 50
> 
> If we have CFG_HZ values that are within a factor of 2 of wrapping 
> around, the platform should probably do some downward scaling (or we 
> should think about 64-bit timestamps)...


Much better than my original patch. Should I revert, retry, and resend?

--
Jason McMullan
MTS SW
System Firmware

NetApp
724.741.5011    Fax
724.741.5166    Direct
412.656.3519    Mobile
jason.mcmullan at netapp.com
www.netapp.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080519/2b2ed3a5/attachment.pgp 

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' valuesin the MHZ range for NAND delays

[Scott Wood]

McMullan, Jason wrote:
> On Mon, 2008-05-19 at 15:26 -0500, Scott Wood wrote:
>> Even though it's MIPS that needs it, it should be flagged as a NAND 
>> patch since that's the code it touches.
> 
> Totally agree.
> 
>> How about this?
>>
>> if (state == FL_ERASING)
>> 	timeo = CFG_HZ * 2 / 5;
>> else
>> 	timeo = CFG_HZ / 50
>>
>> If we have CFG_HZ values that are within a factor of 2 of wrapping 
>> around, the platform should probably do some downward scaling (or we 
>> should think about 64-bit timestamps)...
> 
> 
> Much better than my original patch. Should I revert, retry, and resend?

Sure.

-Scott

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Wolfgang Denk]

In message <4831E28E.5060005@freescale.com> you wrote:
>
> if (state == FL_ERASING)
> 	timeo = CFG_HZ * 2 / 5;
> else
> 	timeo = CFG_HZ / 50
> 
> If we have CFG_HZ values that are within a factor of 2 of wrapping 
> around, the platform should probably do some downward scaling (or we 
> should think about 64-bit timestamps)...

Not needed. CFG_HZ == 1000 for all sane ports.

Broken ports should be fixed.

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
People are always a lot more complicated than you  think.  It's  very
important to remember that.             - Terry Pratchett, _Truckers_

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Scott Wood]

Wolfgang Denk wrote:
> In message <4831E28E.5060005@freescale.com> you wrote:
>> if (state == FL_ERASING)
>> 	timeo = CFG_HZ * 2 / 5;
>> else
>> 	timeo = CFG_HZ / 50
>>
>> If we have CFG_HZ values that are within a factor of 2 of wrapping 
>> around, the platform should probably do some downward scaling (or we 
>> should think about 64-bit timestamps)...
> 
> Not needed. CFG_HZ == 1000 for all sane ports.
> 
> Broken ports should be fixed.

Maybe we should define it in a non-board-specific header, so as to make 
the intent clear that it not actually be configurable?

-Scott

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Wolfgang Denk]

In message <4831E665.3030003@freescale.com> you wrote:
>
> > Broken ports should be fixed.
> 
> Maybe we should define it in a non-board-specific header, so as to make 
> the intent clear that it not actually be configurable?

Good idea. But the change fill break some 100 boards.

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
Einstein argued that there must be simplified explanations of nature,
because God is not capricious or arbitrary. No  such  faith  comforts
the software engineer.                             - Fred Brooks, Jr.

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Scott Wood]

Wolfgang Denk wrote:
> In message <4831E665.3030003@freescale.com> you wrote:
>>> Broken ports should be fixed.
>> Maybe we should define it in a non-board-specific header, so as to make 
>> the intent clear that it not actually be configurable?
> 
> Good idea. But the change fill break some 100 boards.

Yeah, I didn't say it'd be painless. :-)

How about something like this:

#ifndef CFG_HZ
#define CFG_HZ 1000
#elif CFG_HZ != 1000
#warning CFG_HZ must be 1000
#endif

-Scott

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Wolfgang Denk]

Dear Scott,

in message <4831EDEE.4010706@freescale.com> you wrote:
>
> > Good idea. But the change fill break some 100 boards.
> 
> Yeah, I didn't say it'd be painless. :-)
> 
> How about something like this:
> 
> #ifndef CFG_HZ
> #define CFG_HZ 1000
> #elif CFG_HZ != 1000
> #warning CFG_HZ must be 1000
> #endif

I'm still waiting for your patch :-)

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
        Ordnung ist die Lust der Vernunft,
        aber Unordnung die Wonne der Phantasie         - Paul Claudel

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Wolfgang Denk]

In message <20080519201124.6F09D10376@mcmullan-linux.hq.netapp.com> you wrote:
> Rewrite the nand_wait() FL_ERASING case to handle CFG_HZ values in the
> MHZ range. This is needed for mips processors, as the timer's timebase
> ticks at CPU clock frequency.

NAK.

This is fundamentally broken, as CFG_HZ must not have "values in  the
MHZ range'. Please consider it a constant with the value 1000.

Nonconforming ports should be fixed instead of adding code that
supports this brokenness.

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
Backed up the system lately?

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Haavard Skinnemoen]

Wolfgang Denk <wd@denx.de> wrote:
> This is fundamentally broken, as CFG_HZ must not have "values in  the
> MHZ range'. Please consider it a constant with the value 1000.

Is it acceptable to define it as 1024 on some boards? It's more accurate
if you're running the timer from a 32.768 kHz oscillator (e.g. an RTC),
and it makes timing calculations cheaper too...

Haavard

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Wolfgang Denk]

In message <20080520102743.6557124b@hskinnemo-gx745.norway.atmel.com> you wrote:
>
> Is it acceptable to define it as 1024 on some boards? It's more accurate
> if you're running the timer from a 32.768 kHz oscillator (e.g. an RTC),
> and it makes timing calculations cheaper too...

We use a millisecond tick, so CFG_HZ == 1000.

Accuracy is not really an issue in almost all cases.

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
A witty saying proves nothing, but saying  something  pointless  gets
people's attention.

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Haavard Skinnemoen]

Wolfgang Denk <wd@denx.de> wrote:
> In message <20080520102743.6557124b@hskinnemo-gx745.norway.atmel.com> you wrote:
> >
> > Is it acceptable to define it as 1024 on some boards? It's more accurate
> > if you're running the timer from a 32.768 kHz oscillator (e.g. an RTC),
> > and it makes timing calculations cheaper too...  
> 
> We use a millisecond tick, so CFG_HZ == 1000.
> 
> Accuracy is not really an issue in almost all cases.

In which cases _is_ it an issue?

Haavard

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Wolfgang Denk]

In message <20080520112214.413ef45e@hskinnemo-gx745.norway.atmel.com> you wrote:
>
> > Accuracy is not really an issue in almost all cases.
> 
> In which cases _is_ it an issue?

Only in those unknown to me :-)

Sorry, what I meant was: I am not aware of any cases  where  accuracy
is  really  an issue, but I don't know 100% of the code so there is a
slight chance that someone might have  made  (incorrect)  assumptions
about any guaranteed accuracy of the timer tick.

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 speed of time is one second per second.

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Alessandro Rubini]

> +#if CFG_HZ > 100000

Shouldn't this be

	if (CFG_HZ > 100000)

instead?

It's a constant expression, and the compiler optimizes the conditional
out. Using if() instead of #if ensures that all the code is always parsed,
so the compiler can review it all and signal any problem. Not to mention
aesthetics.

/alessandro

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Wolfgang Denk]

In message <20080519204436.GA3521@mail.gnudd.com> you wrote:
> 
> > +#if CFG_HZ > 100000
> 
> Shouldn't this be
> 
> 	if (CFG_HZ > 100000)
> 
> instead?
> 
> It's a constant expression, and the compiler optimizes the conditional
> out. Using if() instead of #if ensures that all the code is always parsed,
> so the compiler can review it all and signal any problem. Not to mention
> aesthetics.

If possible, we *want* the preprocessor to remove any code that does
not apply for a specific configuration. Generating code just bloats
the memory footprint.

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
Houston, Tranquillity Base here.  The Eagle has landed.
                                                    -- Neil Armstrong

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Alessandro Rubini]

> If possible, we *want* the preprocessor to remove any code that does
> not apply for a specific configuration. Generating code just bloats
> the memory footprint.

Definitely. 

But constant expressions are evaluated at compile time. So the dead
branch (if or else) doesn't generate any object code. There is no
memory footprint overhead.

When possible, I prefer to use C conditionals rather than preprocessor
conditionals. I raised the point because I see u-boot wants to get rid
of preprocessor mess when it brings no cost.

/alessandro

[U-Boot-Users] [PATCH] mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

[Wolfgang Denk]

In message <20080519210409.GA3803@mail.gnudd.com> you wrote:
> 
> > If possible, we *want* the preprocessor to remove any code that does
> > not apply for a specific configuration. Generating code just bloats
> > the memory footprint.
> 
> Definitely. 
> 
> But constant expressions are evaluated at compile time. So the dead
> branch (if or else) doesn't generate any object code. There is no
> memory footprint overhead.
> 
> When possible, I prefer to use C conditionals rather than preprocessor
> conditionals. I raised the point because I see u-boot wants to get rid
> of preprocessor mess when it brings no cost.

In general, you are of course right.

In this specific case, I feel an immediate allergic reaction  because
it makes the code look even more as if CFG_HZ was a variable ;-)

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
Es sind ?berhaupt nur die Dummk?pfe, die sich den Befehlen der  M?ch-
tigen  widersetzen.  Um  sie  zu ruinieren ist es genug, ihre Befehle
treu zu erf?llen.                  - Peter Hacks: "Die sch?ne Helena"