[U-Boot-Users] FIX: dataflash.c

[U-Boot-Users] FIX: dataflash.c

[Wolfgang Denk]

In message <20070818231206.GA4040@gandalf.sssup.it> you wrote:
> Change in dataflash.c for fixing
> 
> Signed-off-by: Trimarchi Michael <trimarchimichael@yahoo.it>
> 
> ---
> 
> --- drivers/dataflash.c.orig	2007-08-18 17:36:08.000000000 +0200
> +++ drivers/dataflash.c	2007-08-18 17:37:05.000000000 +0200
> @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
>  static AT91S_DataFlash DataFlashInst;
>  
>  #ifdef CONFIG_AT91SAM9260EK
> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> +int cs[][2] = {
>  	{CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},	/* Logical adress, CS */
>  	{CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
>  };
>  #elif defined(CONFIG_AT91SAM9263EK)
> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> +int cs[][2] = {
>  	{CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}	/* Logical adress, CS */
>  };
>  #else
> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> +int cs[][2] = {
>  	{CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},	/* Logical adress, CS */
>  	{CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
>  };

I hereby reject this patch.

Replacing a configuration option by a hardwired constant which is
probably wrong on most of the boards is definitely a Bad Thing.

Also, you did not even mention what sort of problem you are trying to
fix.

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
There is an order of things in this universe.
	-- Apollo, "Who Mourns for Adonais?" stardate 3468.1

[U-Boot-Users] FIX: dataflash.c

[Michael Trimarchi]

Wolfgang Denk wrote:
> In message <20070818231206.GA4040@gandalf.sssup.it> you wrote:
>   
>> Change in dataflash.c for fixing
>>
>> Signed-off-by: Trimarchi Michael <trimarchimichael@yahoo.it>
>>
>> ---
>>
>> --- drivers/dataflash.c.orig	2007-08-18 17:36:08.000000000 +0200
>> +++ drivers/dataflash.c	2007-08-18 17:37:05.000000000 +0200
>> @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
>>  static AT91S_DataFlash DataFlashInst;
>>  
>>  #ifdef CONFIG_AT91SAM9260EK
>> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> +int cs[][2] = {
>>  	{CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},	/* Logical adress, CS */
>>  	{CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
>>  };
>>  #elif defined(CONFIG_AT91SAM9263EK)
>> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> +int cs[][2] = {
>>  	{CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}	/* Logical adress, CS */
>>  };
>>  #else
>> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> +int cs[][2] = {
>>  	{CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},	/* Logical adress, CS */
>>  	{CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
>>  };
>>     
>
> I hereby reject this patch.
>
> Replacing a configuration option by a hardwired constant which is
> probably wrong on most of the boards is definitely a Bad Thing.
>   
The length of the array row is fixed and it is two... It can't be different.

Regards Michael

[U-Boot-Users] FIX: dataflash.c

[Ulf Samuelsson]

>>> --- drivers/dataflash.c.orig 2007-08-18 17:36:08.000000000 +0200
>>> +++ drivers/dataflash.c 2007-08-18 17:37:05.000000000 +0200
>>> @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
>>>  static AT91S_DataFlash DataFlashInst;
>>>  
>>>  #ifdef CONFIG_AT91SAM9260EK
>>> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>>> +int cs[][2] = {
>>>  {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}, /* Logical adress, CS */
>>>  {CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
>>>  };
>>>  #elif defined(CONFIG_AT91SAM9263EK)
>>> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>>> +int cs[][2] = {
>>>  {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0} /* Logical adress, CS */
>>>  };
>>>  #else
>>> -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>>> +int cs[][2] = {
>>>  {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}, /* Logical adress, CS */
>>>  {CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
>>>  };
>>>     
>>
>> I hereby reject this patch.
>>
>> Replacing a configuration option by a hardwired constant which is
>> probably wrong on most of the boards is definitely a Bad Thing.
>>   
> The length of the array row is fixed and it is two... It can't be different.
> 

Yes, but that means that you leave the config/*.h info dangling.
With the current code you can see how many dataflash are supported.

I agree with Wolfgang (wow, that is an experience!) that the patch should be rejected.

Best Regards
Ulf Samuelsson

[U-Boot-Users] FIX: dataflash.c

[Michael Trimarchi]

> I hereby reject this patch.
>
> Replacing a configuration option by a hardwired constant which is
> probably wrong on most of the boards is definitely a Bad Thing.
>
>   
ok
> Also, you did not even mention what sort of problem you are trying to
> fix.
>
>   
It fix an invalid use of a pointer and and invalid use of an array.

regards michael

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dataflash.at41.patch
Type: text/x-patch
Size: 1038 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070819/345eeafe/attachment.bin 

[U-Boot-Users] FIX: dataflash.c

[Ulf Samuelsson]

>  for (i = 0; i < CFG_MAX_DATAFLASH_BANKS; i++)
>  if ( dataflash_info[i].id
> - && ((((int) addr) & 0xFF000000) ==
> + && ((((unsigned int) *addr) & 0xFF000000) ==
>  dataflash_info[i].logical_address)) {
>  addr_valid = 1;
>  break;
>
> It fix an invalid use of a pointer and and invalid use of an array.
> 
> regards michael
> 
>

AFAIK, This patch is introducing a bug.

The intention of the code is to check if  "addr" is within
0xC0000000..0xCFFFFFFF    or
0xD0000000..0xDFFFFFFF.

Your patch will make the ARM core *read* from whereever 'addr' is pointing at.

'addr' is an address specified by the user!

You do not know *where* is it located, and if the ARM reads
from an arbitrary address, there is a big chance that it will trap...

Best Regards
Ulf Samuelsson

[U-Boot-Users] FIX: dataflash.c

[Michael Trimarchi]

Ulf Samuelsson wrote:
>>  for (i = 0; i < CFG_MAX_DATAFLASH_BANKS; i++)
>>  if ( dataflash_info[i].id
>> - && ((((int) addr) & 0xFF000000) ==
>> + && ((((unsigned int) *addr) & 0xFF000000) ==
>>  dataflash_info[i].logical_address)) {
>>  addr_valid = 1;
>>  break;
>>
>> It fix an invalid use of a pointer and and invalid use of an array.
>>
>> regards michael
>>
>>
>>     
>
> AFAIK, This patch is introducing a bug.
>
> The intention of the code is to check if  "addr" is within
> 0xC0000000..0xCFFFFFFF    or
> 0xD0000000..0xDFFFFFFF.
>
> Your patch will make the ARM core *read* from whereever 'addr' is pointing at.
>
>   
The patch is only reversed.
- new code
+ old code
Regards Michael

[U-Boot-Users] FIX: dataflash.c

[Michael Trimarchi]

Ulf Samuelsson wrote:
>>  for (i = 0; i < CFG_MAX_DATAFLASH_BANKS; i++)
>>  if ( dataflash_info[i].id
>> - && ((((int) addr) & 0xFF000000) ==
>> + && ((((unsigned int) *addr) & 0xFF000000) ==
>>  dataflash_info[i].logical_address)) {
>>  addr_valid = 1;
>>  break;
>>
>> It fix an invalid use of a pointer and and invalid use of an array.
>>
>> regards michael
>>
>>
>>     
>
> AFAIK, This patch is introducing a bug.
>
> The intention of the code is to check if  "addr" is within
> 0xC0000000..0xCFFFFFFF    or
> 0xD0000000..0xDFFFFFFF.
>
> Your patch will make the ARM core *read* from whereever 'addr' is pointing at.
>
> 'addr' is an address specified by the user!
>
> You do not know *where* is it located, and if the ARM reads
> from an arbitrary address, there is a big chance that it will trap...
>
> Best Regards
> Ulf Samuelsson
>
>
>   
* addr is the value of the logical address to check. addr is the address 
of the variable that contain the logical address.
I think that my patch is ok but reversed .orig .new.
Regards Michael

[U-Boot-Users] FIX: dataflash.c

[Håvard Skinnemoen]

On 8/19/07, Wolfgang Denk <wd@denx.de> wrote:
> In message <20070818231206.GA4040@gandalf.sssup.it> you wrote:
> > --- drivers/dataflash.c.orig  2007-08-18 17:36:08.000000000 +0200
> > +++ drivers/dataflash.c       2007-08-18 17:37:05.000000000 +0200
> > @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
> >  static AT91S_DataFlash DataFlashInst;
> >
> >  #ifdef CONFIG_AT91SAM9260EK
> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > +int cs[][2] = {
> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
> >       {CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
> >  };
> >  #elif defined(CONFIG_AT91SAM9263EK)
> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > +int cs[][2] = {
> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}       /* Logical adress, CS */
> >  };
> >  #else
> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > +int cs[][2] = {
> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
> >       {CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
> >  };

Why not call it dataflash_cs[] and move it into the board code? It
already pollutes the namespace, is already board-dependent (hence the
#ifdefs) and is apparently buggy?

Then the board code is responsible for defining a
CFG_MAX_DATAFLASH_BANKS symbol which corresponds with the actual
number of entries in the array. Of course, it can still be buggy, but
at least it's contained within the board code.

Haavard

> I hereby reject this patch.
>
> Replacing a configuration option by a hardwired constant which is
> probably wrong on most of the boards is definitely a Bad Thing.
>
> Also, you did not even mention what sort of problem you are trying to
> fix.
>
> 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
> There is an order of things in this universe.
>         -- Apollo, "Who Mourns for Adonais?" stardate 3468.1
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users
>

[U-Boot-Users] FIX: dataflash.c

[Wolfgang Denk]

In message <1defaf580708191437p19e70b02t43f5bcb5dc986e5c@mail.gmail.com> you wrote:
> On 8/19/07, Wolfgang Denk <wd@denx.de> wrote:
> > In message <20070818231206.GA4040@gandalf.sssup.it> you wrote:
> > > --- drivers/dataflash.c.orig  2007-08-18 17:36:08.000000000 +0200
> > > +++ drivers/dataflash.c       2007-08-18 17:37:05.000000000 +0200
> > > @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
> > >  static AT91S_DataFlash DataFlashInst;
> > >
> > >  #ifdef CONFIG_AT91SAM9260EK
> > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > > +int cs[][2] = {
> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
> > >  };
> > >  #elif defined(CONFIG_AT91SAM9263EK)
> > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > > +int cs[][2] = {
> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}       /* Logical adress, CS */
> > >  };
> > >  #else
> > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > > +int cs[][2] = {
> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
> > >  };
> 
> Why not call it dataflash_cs[] and move it into the board code? It
> already pollutes the namespace, is already board-dependent (hence the
> #ifdefs) and is apparently buggy?

Excellent idea.

> Then the board code is responsible for defining a
> CFG_MAX_DATAFLASH_BANKS symbol which corresponds with the actual
> number of entries in the array. Of course, it can still be buggy, but
> at least it's contained within the board code.

I agree completely.


Who will provide such a 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
Der Horizont vieler Menschen ist ein Kreis mit Radius Null --
und das nennen sie ihren Standpunkt.

[U-Boot-Users] FIX: dataflash.c

[trimarchi at gandalf.sssup.it]

Quoting Wolfgang Denk <wd@denx.de>:

> In message   
> <1defaf580708191437p19e70b02t43f5bcb5dc986e5c@mail.gmail.com> you   
> wrote:
>> On 8/19/07, Wolfgang Denk <wd@denx.de> wrote:
>> > In message <20070818231206.GA4040@gandalf.sssup.it> you wrote:
>> > > --- drivers/dataflash.c.orig  2007-08-18 17:36:08.000000000 +0200
>> > > +++ drivers/dataflash.c       2007-08-18 17:37:05.000000000 +0200
>> > > @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
>> > >  static AT91S_DataFlash DataFlashInst;
>> > >
>> > >  #ifdef CONFIG_AT91SAM9260EK
>> > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> > > +int cs[][2] = {
>> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
>> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
>> > >  };
>> > >  #elif defined(CONFIG_AT91SAM9263EK)
>> > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> > > +int cs[][2] = {
>> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}       /* Logical adress, CS */
>> > >  };
>> > >  #else
>> > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> > > +int cs[][2] = {
>> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
>> > >       {CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
>> > >  };
>>
>> Why not call it dataflash_cs[] and move it into the board code? It
>> already pollutes the namespace, is already board-dependent (hence the
>> #ifdefs) and is apparently buggy?
>
> Excellent idea.
>
I think that the dataflash layer must be rewritten and he must conformed
to the flash layer.

Regards Michael


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.

[U-Boot-Users] FIX: dataflash.c

[Ulf Samuelsson]

m?n 2007-08-20 klockan 01:29 +0200 skrev Wolfgang Denk:
> In message <1defaf580708191437p19e70b02t43f5bcb5dc986e5c@mail.gmail.com> you wrote:
> > On 8/19/07, Wolfgang Denk <wd@denx.de> wrote:
> > > In message <20070818231206.GA4040@gandalf.sssup.it> you wrote:
> > > > --- drivers/dataflash.c.orig  2007-08-18 17:36:08.000000000 +0200
> > > > +++ drivers/dataflash.c       2007-08-18 17:37:05.000000000 +0200
> > > > @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
> > > >  static AT91S_DataFlash DataFlashInst;
> > > >
> > > >  #ifdef CONFIG_AT91SAM9260EK
> > > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > > > +int cs[][2] = {
> > > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
> > > >       {CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
> > > >  };
> > > >  #elif defined(CONFIG_AT91SAM9263EK)
> > > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > > > +int cs[][2] = {
> > > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}       /* Logical adress, CS */
> > > >  };
> > > >  #else
> > > > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> > > > +int cs[][2] = {
> > > >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
> > > >       {CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
> > > >  };
> > 
> > Why not call it dataflash_cs[] and move it into the board code? It
> > already pollutes the namespace, is already board-dependent (hence the
> > #ifdefs) and is apparently buggy?
> 
> Excellent idea.
> 
> > Then the board code is responsible for defining a
> > CFG_MAX_DATAFLASH_BANKS symbol which corresponds with the actual
> > number of entries in the array. Of course, it can still be buggy, but
> > at least it's contained within the board code.
> 
> I agree completely.
> 
> 
> Who will provide such a patch?

I can do it, in a week or two.

> 
> Best regards,
> 
> Wolfgang Denk
> 

[U-Boot-Users] FIX: dataflash.c

[Wolfgang Denk]

Dear Ulf,

in message <1187587574.31921.58.camel@elrond.sweden.atmel.com> you wrote:
>
> > > Why not call it dataflash_cs[] and move it into the board code? It
> > > already pollutes the namespace, is already board-dependent (hence the
> > > #ifdefs) and is apparently buggy?
> > 
> > Excellent idea.
> > 
> > > Then the board code is responsible for defining a
> > > CFG_MAX_DATAFLASH_BANKS symbol which corresponds with the actual
> > > number of entries in the array. Of course, it can still be buggy, but
> > > at least it's contained within the board code.
> > 
> > I agree completely.
> > 
> > 
> > Who will provide such a patch?
> 
> I can do it, in a week or two.

Just to make sure I understand correctly: are you working on this?
Any estimates when it might be ready?

Thanks in advance.

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
"Probably the best operating system in the world  is  the  [operating
system] made for the PDP-11 by Bell Laboratories."
                                           - Ted Nelson, October 1977

[U-Boot-Users] FIX: dataflash.c

[trimarchi at gandalf.sssup.it]

Quoting H?vard Skinnemoen <hskinnemoen@gmail.com>:

> On 8/19/07, Wolfgang Denk <wd@denx.de> wrote:
>> In message <20070818231206.GA4040@gandalf.sssup.it> you wrote:
>> > --- drivers/dataflash.c.orig  2007-08-18 17:36:08.000000000 +0200
>> > +++ drivers/dataflash.c       2007-08-18 17:37:05.000000000 +0200
>> > @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
>> >  static AT91S_DataFlash DataFlashInst;
>> >
>> >  #ifdef CONFIG_AT91SAM9260EK
>> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> > +int cs[][2] = {
>> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
>> >       {CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
>> >  };
>> >  #elif defined(CONFIG_AT91SAM9263EK)
>> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> > +int cs[][2] = {
>> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}       /* Logical adress, CS */
>> >  };
>> >  #else
>> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
>> > +int cs[][2] = {
>> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
>> >       {CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
>> >  };
>
> Why not call it dataflash_cs[] and move it into the board code? It
> already pollutes the namespace, is already board-dependent (hence the
> #ifdefs) and is apparently buggy?
>
> Then the board code is responsible for defining a
> CFG_MAX_DATAFLASH_BANKS symbol which corresponds with the actual
> number of entries in the array. Of course, it can still be buggy, but
> at least it's contained within the board code.
>
> Haavard

I send the patch just to say that the implementation does't work in  
the u-boot git code when CFG_MAX_DATAFLASH_BANKS is egual to 1. I find  
this fixing for me and send to the mailing list for a better solution.  
There are a lot of problem in the implementation of the dataflash  
layer. Take the cmd_mem.c file and It simple to observ that is not  
possible to use a dataflash cp operation to memory without define a  
dummy flash with parameters like CONFIG_MAX_BANKS set to 0.
Regards Michael



----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.

[U-Boot-Users] FIX: dataflash.c

[Ulf Samuelsson]

m?n 2007-08-20 klockan 01:40 +0200 skrev trimarchi at gandalf.sssup.it:
> Quoting H?vard Skinnemoen <hskinnemoen@gmail.com>:
> 
> > On 8/19/07, Wolfgang Denk <wd@denx.de> wrote:
> >> In message <20070818231206.GA4040@gandalf.sssup.it> you wrote:
> >> > --- drivers/dataflash.c.orig  2007-08-18 17:36:08.000000000 +0200
> >> > +++ drivers/dataflash.c       2007-08-18 17:37:05.000000000 +0200
> >> > @@ -27,16 +27,16 @@ AT91S_DATAFLASH_INFO dataflash_info[CFG_
> >> >  static AT91S_DataFlash DataFlashInst;
> >> >
> >> >  #ifdef CONFIG_AT91SAM9260EK
> >> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> >> > +int cs[][2] = {
> >> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
> >> >       {CFG_DATAFLASH_LOGIC_ADDR_CS1, 1}
> >> >  };
> >> >  #elif defined(CONFIG_AT91SAM9263EK)
> >> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> >> > +int cs[][2] = {
> >> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0}       /* Logical adress, CS */
> >> >  };
> >> >  #else
> >> > -int cs[][CFG_MAX_DATAFLASH_BANKS] = {
> >> > +int cs[][2] = {
> >> >       {CFG_DATAFLASH_LOGIC_ADDR_CS0, 0},      /* Logical adress, CS */
> >> >       {CFG_DATAFLASH_LOGIC_ADDR_CS3, 3}
> >> >  };
> >
> > Why not call it dataflash_cs[] and move it into the board code? It
> > already pollutes the namespace, is already board-dependent (hence the
> > #ifdefs) and is apparently buggy?
> >
> > Then the board code is responsible for defining a
> > CFG_MAX_DATAFLASH_BANKS symbol which corresponds with the actual
> > number of entries in the array. Of course, it can still be buggy, but
> > at least it's contained within the board code.
> >
> > Haavard
> 
> I send the patch just to say that the implementation does't work in  
> the u-boot git code when CFG_MAX_DATAFLASH_BANKS is egual to 1. I find  
> this fixing for me and send to the mailing list for a better solution.  
> There are a lot of problem in the implementation of the dataflash  
> layer. Take the cmd_mem.c file and It simple to observ that is not  
> possible to use a dataflash cp operation to memory without define a  
> dummy flash with parameters like CONFIG_MAX_BANKS set to 0.
> Regards Michael
> 

Yes, but you fix it in the wrong way.

You remove the error message by introducing an
inconsistency between the configuration and the actual code.

BR
Ulf Samuelsson