[U-Boot-Users] [PATCH] 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 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);
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
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@netapp.com www.netapp.com
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
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
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
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
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
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
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
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
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
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
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
+#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
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
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
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
participants (7)
-
Alessandro Rubini -
Alessandro Rubini -
Haavard Skinnemoen -
Jason McMullan -
McMullan, Jason
-
Scott Wood -
Wolfgang Denk