[U-Boot] Breakage on arm/next

[U-Boot] Breakage on arm/next

[Tom]

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

The error message is

env_onenand.c: In function 'env_relocate_spec':
env_onenand.c:70: error: 'CONFIG_ENV_ADDR_FLEX' undeclared (first use in this 
function)
env_onenand.c:70: error: (Each undeclared identifier is reported only once
env_onenand.c:70: error: for each function it appears in.)
env_onenand.c: In function 'saveenv':
env_onenand.c:106: error: 'CONFIG_ENV_ADDR_FLEX' undeclared (first use in this 
function)
env_onenand.c:107: error: 'CONFIG_ENV_SIZE_FLEX' undeclared (first use in this 
function)

I believe the error is from

commit c758e947aa7d39a2be607ecdedd818ad300807b2
Author: Amul Kumar Saha <amul.saha@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.

     Signed-off-by: Rohit Hagargundgi <h.rohit@samsung.com>
     Signed-off-by: Amul Kumar Saha <amul.saha@samsung.com>

In general MAKEALL should be used to regression test your changes.

Please send a patch for this problem as soon as possible.
Tom

[U-Boot] Breakage on arm/next

[apgmoorthy]

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@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

[U-Boot] Breakage on arm/next

[Tom]

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

[U-Boot] Breakage on arm/next

[Alessandro Rubini]

> 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

[U-Boot] Breakage on arm/next

[apgmoorthy]

 
Hi,

> -----Original Message-----
> From: rubini at gnudd.com [mailto:rubini at gnudd.com] On Behalf Of 
> Alessandro Rubini
> Sent: Tuesday, December 01, 2009 9:36 PM
> To: Tom.Rix at windriver.com
> Cc: moorthy.apg at samsung.com; scottwood at freescale.com; 
> u-boot at lists.denx.de; kyungmin.park at 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

[U-Boot] Breakage on arm/next

[Scott Wood]

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

[U-Boot] Breakage on arm/next

[apgmoorthy]

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

[U-Boot] Breakage on arm/next

[Scott Wood]

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

[U-Boot] Breakage on arm/next

[apgmoorthy]

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 

[U-Boot] Breakage on arm/next

[Scott Wood]

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

[U-Boot] Breakage on arm/next

[apgmoorthy]

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

[U-Boot] Breakage on arm/next

[Scott Wood]

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

[U-Boot] Breakage on arm/next

[Premi, Sanjeev]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Tom
> Sent: Tuesday, December 01, 2009 8:10 PM
> To: apgmoorthy
> Cc: 'Scott Wood'; u-boot at lists.denx.de; kyungmin.park at 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] Breakage on arm/next

[apgmoorthy]

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