Skip to contentSkip to conversation

mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

PATCH New Jason McMullan 7 participants 18 messages May 16, 2008 – Jun 11, 2008

Subsystems (2)
Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

Jason McMullan

author
#0 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

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 <[email protected]>
---
 drivers/mtd/nand/nand_base.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
drivers/mtd/nand/nand_base.c +7 -0
index 2da1d46..ac690ac 100644
@@ -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);
Signature · 1 line
1.5.4.3
#1 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 3 lines · 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.

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

Quoted 9 lines
drivers/mtd/nand/nand_base.c +1 -0
index 2da1d46..ac690ac 100644
@@ -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

drivers/mtd/nand/nand_base.c static int nand_wait(struct mtd_info *mtd, struct nand_chip *this, int state) · line 850

Quoted 10 lines
drivers/mtd/nand/nand_base.c +6 -0
+	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

#2 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' valuesin the MHZ range for NAND delays

Quoted 2 lines · Scott Wood

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.

Quoted 10 lines

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

#3 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' valuesin the MHZ range for NAND delays

Quoted 13 lines · Jason
Quoted 2 lines · Scott Wood

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.

Quoted 7 lines

How about this?

if (state == FL_ERASING)

	timeo = CFG_HZ * 2 / 5;
else
	timeo = CFG_HZ / 50
Quoted 6 lines
Quoted 3 lines

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

#4 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

In reply to Scott Wood
Quoted 9 lines · In message <[email protected]> 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

Signature · 5 lines
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_
#5 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 6 lines · Wolfgang Denk
Quoted 5 lines · In message <[email protected]> you wrote:

if (state == FL_ERASING)

	timeo = CFG_HZ * 2 / 5;
else
	timeo = CFG_HZ / 50
Quoted 7 lines
Quoted 3 lines

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

#6 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 5 lines · In message <[email protected]> you wrote:
Quoted 2 lines

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

Signature · 6 lines
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.
#7 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 6 lines · Wolfgang Denk
Quoted 3 lines · In message <[email protected]> you wrote:
Quoted 1 line

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

#8 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Dear Scott,

Quoted 5 lines · in message <[email protected]> you wrote:
Quoted 2 lines

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

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

Quoted 7 lines

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

Signature · 5 lines
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
NAK
#9 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

In reply to Jason McMullan
Quoted 3 lines · In message <[email protected]> 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

Signature · 4 lines
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?
#10 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 2 lines · Wolfgang Denk

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

#11 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 4 lines · In message <[email protected]> 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

Signature · 5 lines
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.
#12 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 9 lines · Wolfgang Denk
Quoted 4 lines · In message <[email protected]> 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

#13 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 4 lines · In message <[email protected]> you wrote:
Quoted 2 lines

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

Signature · 4 lines
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.
#14 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

In reply to Jason McMullan
Quoted 2 lines

+#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

#15 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 5 lines · In message <[email protected]> you wrote:
Quoted 2 lines

+#if CFG_HZ > 100000

Shouldn't this be

Quoted 8 lines
	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

Signature · 5 lines
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
#16 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 4 lines

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

#17 lore raw

PATCH 1/1 mips: tolerate the MIPS 'CFG_HZ' values in the MHZ range for NAND delays

Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 4 lines

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.

Quoted 7 lines

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

Signature · 6 lines
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"