Re: [U-Boot] Breakage on arm/next
Hi Tom,
Amul is out-of-office for sometime. Excuse us for the delay.
In rebasing arm/next against u-boot/next. There is a general error with targets that use onenand. This includes the targets nk8815_onenand omap3_evm smdkc100
I believe the error is from
commit c758e947aa7d39a2be607ecdedd818ad300807b2 Author: Amul Kumar Saha <amul.saha at samsung.com> Date: Wed Nov 4 10:38:46 2009 +0530
ENV Variable support for Flex-OneNAND
Define and use CONFIG_ENV_ADDR_FLEX and CONFIG_ENV_SIZE_FLEX for storing environment variables.
The error comes since the Macros are defined in "configs/apollon.h". And it works fine for Apollon.
I have tried with "make smdkc100_config" and did 'make all' and confirmed the error reported.
Please send a patch for this problem as soon as possible.
Moving these Macro definitions to "include/linux/mtd/onenand.h" looks more viable. I can send across the patch. Please comment.
With Regards Moorthy
apgmoorthy wrote:
Hi Tom,
<snip>
Moving these Macro definitions to "include/linux/mtd/onenand.h" looks more viable. I can send across the patch. Please comment.
Could the macros defined in apollo.h also be defined in the other target board's config file ?
Thank you for looking into this Tom
With Regards Moorthy
Could the macros defined in apollo.h also be defined in the other target board's config file ?
I don't think so (my board is one of the affected ones).
The macros are CONFIG_ENV_ADDR_FLEX and CONFIG_ENV_SIZE_FLEX . I just don't have the flex device.
In the commit that introduced the problem, I see code like this:
env_addr = CONFIG_ENV_ADDR; + if (FLEXONENAND(this)) + env_addr = CONFIG_ENV_ADDR_FLEX;
So why should CONFIG_ENV_ADDR_FLEX have a different name from CONFIG_ENV_ADDR ? Same applies to CONFIG_ENV_SIZE_FLEX.
I think c758e947aa7d39a2be607ecdedd818ad300807b2 should be reverted and done differently. If I got my reasoning right, the first hunk should go and the next one:
instr.len = CONFIG_ENV_SIZE; + if (FLEXONENAND(this)) { + env_addr = CONFIG_ENV_ADDR_FLEX; + instr.len = CONFIG_ENV_SIZE_FLEX; + instr.len <<= onenand_mtd.eraseregions[0].numblocks == 1 ? + 1 : 0; + }
Shoul just become
+ if (FLEXONENAND(this)) + instr.len <<= onenand_mtd.eraseregions[0].numblocks == 1 ? 1 : 0;
This has no adverse effect on other boards and handles the flex specifics, withouth adding two unneeded macros.
/alessandro
Hi,
-----Original Message----- From: rubini@gnudd.com [mailto:rubini@gnudd.com] On Behalf Of Alessandro Rubini Sent: Tuesday, December 01, 2009 9:36 PM To: Tom.Rix@windriver.com Cc: moorthy.apg@samsung.com; scottwood@freescale.com; u-boot@lists.denx.de; kyungmin.park@samsung.com Subject: Re: [U-Boot] Breakage on arm/next
Could the macros defined in apollo.h also be defined in the other target board's config file ?
I don't think so (my board is one of the affected ones).
The macros are CONFIG_ENV_ADDR_FLEX and CONFIG_ENV_SIZE_FLEX . I just don't have the flex device.
I get u.
In the commit that introduced the problem, I see code like this:
env_addr = CONFIG_ENV_ADDR;
if (FLEXONENAND(this))env_addr = CONFIG_ENV_ADDR_FLEX;So why should CONFIG_ENV_ADDR_FLEX have a different name from CONFIG_ENV_ADDR ? Same applies to CONFIG_ENV_SIZE_FLEX.
Erasesizes differ in Flex-OneNAND from OneNAND. So Default Macros cannot be used.
I think c758e947aa7d39a2be607ecdedd818ad300807b2 should be reverted and done differently. If I got my reasoning right, the first hunk should go and the next one:
instr.len = CONFIG_ENV_SIZE;
if (FLEXONENAND(this)) {env_addr = CONFIG_ENV_ADDR_FLEX;instr.len = CONFIG_ENV_SIZE_FLEX;instr.len <<=onenand_mtd.eraseregions[0].numblocks == 1 ?
1 : 0;}Shoul just become
if (FLEXONENAND(this))instr.len <<=onenand_mtd.eraseregions[0].numblocks == 1
- ? 1 : 0;
This has no adverse effect on other boards and handles the flex specifics, withouth adding two unneeded macros.
The Erase Sizes of Flex-OneNAND are 256K for SLC and 512K for MLC regions. And always 0th Block belongs to SLC region whereas, 1st Block can be of SLC or MLC region in a Die. So without these Macros Flex-OneNAND specifics cannot be handled.
I felt moving these macros "include/linux/mtd/onenand.h" will be ideal in this scenario.
With Regards Moorthy
apgmoorthy wrote:
I felt moving these macros "include/linux/mtd/onenand.h" will be ideal in this scenario.
Are they going to be the same on all boards? We let the board determine the environment location for other types of storage.
How about just using CONFIG_ENV_ADDR/CONFIG_ENV_SIZE? On boards that must dynamically support multiple possibilities, define it as an expression that returns the right thing.
-Scot
Hi Scott,
Are they going to be the same on all boards? We let the board determine the environment location for other types of storage.
OK
How about just using CONFIG_ENV_ADDR/CONFIG_ENV_SIZE? On boards that must dynamically support multiple possibilities, define it as an expression that returns the right thing.
If the macros are not favoured to get consensus , let the code use CONFIG_ENV_ADDR/CONFIG_ENV_SIZE and incase of Flex-OneNAND increase it by one more fold.
something Like
Hunk 1: env_addr = CONFIG_ENV_ADDR; + if (FLEXONENAND(this)) + env_addr <<= 1;
Hunk 2: + if (FLEXONENAND(this)) { + env_addr <<= 1; + instr.len <<= onenand_mtd.eraseregions[0].numblocks == 1 ? + 2 : 1; + }
This should not break any other Board with OneNAND support. Please comment. (Somehow I still feel Macros can be Cleaner way.)
With Regards Moorthy
apgmoorthy wrote:
Hi Scott,
Are they going to be the same on all boards? We let the board determine the environment location for other types of storage.
OK
How about just using CONFIG_ENV_ADDR/CONFIG_ENV_SIZE? On boards that must dynamically support multiple possibilities, define it as an expression that returns the right thing.
If the macros are not favoured to get consensus , let the code use CONFIG_ENV_ADDR/CONFIG_ENV_SIZE and incase of Flex-OneNAND increase it by one more fold.
something Like
Hunk 1: env_addr = CONFIG_ENV_ADDR;
if (FLEXONENAND(this))env_addr <<= 1;Hunk 2:
if (FLEXONENAND(this)) {env_addr <<= 1;instr.len <<= onenand_mtd.eraseregions[0].numblocks == 1 ?2 : 1;}This should not break any other Board with OneNAND support. Please comment. (Somehow I still feel Macros can be Cleaner way.)
Why is the address automatically doubled on flex? I think this really needs to be something board-specified.
-Scott
Hi Scott,
Hunk 1: env_addr = CONFIG_ENV_ADDR;
if (FLEXONENAND(this))env_addr <<= 1;Hunk 2:
if (FLEXONENAND(this)) {env_addr <<= 1;instr.len <<=onenand_mtd.eraseregions[0].numblocks == 1 ?
2 : 1;}This should not break any other Board with OneNAND support.
Please comment.
(Somehow I still feel Macros can be Cleaner way.)
Why is the address automatically doubled on flex? I think this really needs to be something board-specified.
Please excuse me for the Delay.
Flex-OneNAND device's erasesize itself is double considered to OneNAND. Like , In SLC region of Flex-OneNAND size is 256K and in MLC region it is 512K. In case of OneNAND erasesize is 128K and it is just SLC.
That's the reason size doubles for the Env in SLC and for MLC increased by one more fold.
In reply to Alessandro's mail , I have given the same details.
With Regards Moorthy
apgmoorthy wrote:
Hi Scott,
Hunk 1: env_addr = CONFIG_ENV_ADDR;
if (FLEXONENAND(this))env_addr <<= 1;Hunk 2:
if (FLEXONENAND(this)) {env_addr <<= 1;instr.len <<=onenand_mtd.eraseregions[0].numblocks == 1 ?
2 : 1;}This should not break any other Board with OneNAND support.
Please comment.
(Somehow I still feel Macros can be Cleaner way.)
Why is the address automatically doubled on flex? I think this really needs to be something board-specified.
Please excuse me for the Delay.
Flex-OneNAND device's erasesize itself is double considered to OneNAND.
That doesn't mean that all data you're storing is double the size. A board may want to keep the byte offset the same, and let the block number change.
Like , In SLC region of Flex-OneNAND size is 256K and in MLC region it is 512K. In case of OneNAND erasesize is 128K and it is just SLC.
Suppose I have a 256K U-Boot. I want CONFIG_ENV_ADDR to be 256K regardless, which would be block 1 for flex SLC or block 2 for regular OneNAND.
If I have a 512K U-Boot, then the byte offset would be the same for MLC as well.
-Scott
Hi Scott,
Hunk 2:
if (FLEXONENAND(this)) {env_addr <<= 1;instr.len <<=onenand_mtd.eraseregions[0].numblocks == 1 ?
2 : 1;}This should not break any other Board with OneNAND support.
Please comment.
(Somehow I still feel Macros can be Cleaner way.)
Why is the address automatically doubled on flex? I think this really needs to be something board-specified.
Please excuse me for the Delay.
Flex-OneNAND device's erasesize itself is double considered
to OneNAND.
That doesn't mean that all data you're storing is double the size. A board may want to keep the byte offset the same, and let the block number change.
Like , In SLC region of Flex-OneNAND size is 256K and in
MLC region it
is 512K. In case of OneNAND erasesize is 128K and it is just SLC.
Suppose I have a 256K U-Boot. I want CONFIG_ENV_ADDR to be 256K regardless, which would be block 1 for flex SLC or block 2 for regular OneNAND.
If I have a 512K U-Boot, then the byte offset would be the same for MLC as well.
You are right , Sorry , I missed it. This is one of the scenarios which compelled us to use seperate Flex-OneNAND Env Macros different from OneNANDs , when suggesting a alternative somehow missed it.
Going back to the suggestion you have given couple of mails back in the same chain. That there should be a macro which should return the ENV size for Flex. In that case i just saw something like below should do.
Hunk 1: env_addr += CONFIG_ENV_ADDR & (onenand_mtd.eraseregions[0].erasesize-1);
Hunk 2: env_addr += CONFIG_ENV_ADDR & (onenand_mtd.eraseregions[0].erasesize-1); instr.len = onenand_mtd.eraseregions[0].erasesize; instr.len <<= onenand_mtd.eraseregions[0].numblocks == 1 ? 1 : 0;
Please comment.
With Regards Moorthy
On Mon, Dec 21, 2009 at 04:30:40PM +0530, apgmoorthy wrote:
Hunk 1: env_addr += CONFIG_ENV_ADDR & (onenand_mtd.eraseregions[0].erasesize-1);
Hunk 2: env_addr += CONFIG_ENV_ADDR & (onenand_mtd.eraseregions[0].erasesize-1);
I'd say it should be the board config file's responsibility to provide a CONFIG_ENV_ADDR that is properly block-aligned, regardless of what sort of flash it's using.
instr.len = onenand_mtd.eraseregions[0].erasesize;
It's unlikely at these block sizes, but theoretically the environment could span multiple erase blocks.
Again, the board config file should supply a suitable CONFIG_ENV_SIZE.
-Scott
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Tom Sent: Tuesday, December 01, 2009 8:10 PM To: apgmoorthy Cc: 'Scott Wood'; u-boot@lists.denx.de; kyungmin.park@samsung.com Subject: Re: [U-Boot] Breakage on arm/next
apgmoorthy wrote:
Hi Tom,
<snip>
Moving these Macro definitions to
"include/linux/mtd/onenand.h" looks more viable.
I can send across the patch. Please comment.
Could the macros defined in apollo.h also be defined in the other target board's config file ?
Thank you for looking into this Tom
Is there any update on the fix/proposal?
I am trying to build for omap3_evm; but see the same problem. My repo is currently at: bb3bcfa : Merge branch 'next' of ../next a200a7c : Update CHANGELOG; prepare Prepare v2009.11
Best regards, Sanjeev
With Regards Moorthy
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Sanjeev,
Is there any update on the fix/proposal?
I am trying to build for omap3_evm; but see the same problem. My repo is currently at: bb3bcfa : Merge branch 'next' of ../next a200a7c : Update CHANGELOG; prepare Prepare v2009.11
As Scott pointed out rightly, my previous suggestion missed to see certain scenario.I will send the fix shortly. Sorry for the Delay.
With Regards Moorthy
participants (5)
-
Alessandro Rubini -
apgmoorthy -
Premi, Sanjeev -
Scott Wood -
Tom