Re: [U-Boot-Users] FIX: dataflash.c
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
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
--- 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
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
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
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
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
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@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@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
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
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.
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
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
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.
mån 2007-08-20 klockan 01:40 +0200 skrev trimarchi@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
participants (5)
-
Håvard Skinnemoen -
Michael Trimarchi -
trimarchi＠gandalf.sssup.it
-
Ulf Samuelsson -
Wolfgang Denk