Re: [U-Boot-Users] Question about CFG_ENV_ADDR during RAMBOOT
We also have board with NAND FLASH only and the number of those boards will grow in the near future. The NAND command interface of course need a little more typing, but I always configure a few environment variables to update the images in the NAND FLASH. Something like:
=> setenv nload 'tftp 200000 board/u-boot.bin' => setenv nupdate 'nand erase 0 40000;nand write 200000 0 40000' => setenv nupd 'run nload nupdate'
Yes, yes :-) Here I agree with Wolfgand and Stefan. U-Boot should provide commands which reflect principle of device without any abstraction. Knowing principle means usually less bad surprises coming out from some hidden abstraction implementation details. That's good for power users.
=> OK, then tell me how to compare a 6 MB file in flash with a 6MB file in SDRAM when your SDRAM is 8 MB.
We are *not* running on PC's.
There is nothing that stops "power users" from copying from dataflash to SDRAM and then doing a compare. Power Users are limited by implementations which waste enormous resources.
I would have more respects for exhibited views if writes to parallel flash is removed from cp.b... Parallel flash is clearly not "memory" for write purposes, only for read purposes. Its existence in cp.b will force endless #ifdefs or support for parallel flash will have to remain and bloat code.
You could then argue that support for parallel flash should be removed from all commands to remove inconsistencies...
I do not think this will happen because it is *useful* and because this affects boards which people that makes decisions works with this. They do not work with dataflash and do not care about people that wants this.
I think that consistency in argumentation is a reasonable demand.
* BYTE WRITE
Why should you waste time on copying dataflash to SDRAM when you can do operations inside the internal SRAM.
It is not desirable to transfer 20,000 bits between CPU and dataflash when 100-200 are sufficient. Personally I would like everyone that did not bother to read and under stand the AT45 dataflash datasheets to do so before sending comments about how the internal implementation should look like.
Best Regards Ulf Samuelsson
In message 8r0FHjm57tdb.nQDcqRvm@mailout.dof.se you wrote:
=> OK, then tell me how to compare a 6 MB file in flash with a 6MB file in SDRAM when your SDRAM is 8 MB.
How are you doing it now? By doing it in chunks of a reasonable size.
It is not desirable to transfer 20,000 bits between CPU and dataflash when 100-200 are sufficient. Personally I would like everyone that did not bother to read and under stand the AT45 dataflash datasheets to do so before sending comments about how the internal implementation should look like.
Note that we don't discuss inplementation yet. We just try to define an API that fits into the rest of U-Boot. Once we have an agreement (or at least a decision), you are welcome to help implementing this as efficiently as possible.
Best regards,
Wolfgang Denk
On Sat, May 26, 2007 at 12:53:31PM +0200, Ulf Samuelsson wrote:
=> OK, then tell me how to compare a 6 MB file in flash with a 6MB file in SDRAM when your SDRAM is 8 MB.
Chunk by chunk? But indeed, there is a little problem. It would be usefull to show address which contains diferences from the beginning of dataflash (or dataflash partition)...
We are *not* running on PC's.
Perhaps it would help, if we could avoid ourselves to state the obvious...
There is nothing that stops "power users" from copying from dataflash to SDRAM and then doing a compare. Power Users are limited by implementations which waste enormous resources.
Anyone who copies whole file into RAM and then does compare has either a lot of RAM or doesn't deserve to be called power user.
Lines 303-308 of common/cmd_mem.c reads (cmp command): #ifdef CONFIG_HAS_DATAFLASH if (addr_dataflash(addr1) | addr_dataflash(addr2)){ puts ("Comparison with DataFlash space not supported.\n\r"); return 0; } #endif
On my boards it leads to: # cmp.l 10000000 C0000000 10 Comparison with DataFlash space not supported.
Therefore it seems I have to copy to RAM anyway and it doesn't seem to be fixed by any patch at ftp 81.80.104.162.
I would have more respects for exhibited views if writes to parallel flash is removed from cp.b... Parallel flash is clearly not "memory" for write purposes, only for read purposes. Its existence in cp.b will force endless #ifdefs or support for parallel flash will have to remain and bloat code.
Erm... This interface is finished, contains finite number of features and certainly finite numbers of #ifdefs. This number could be reduced even more by removing hacks for MMC.
You could then argue that support for parallel flash should be removed from all commands to remove inconsistencies...I do not think this will happen because it is *useful* and because this affects boards which people that makes decisions works with this. They do not work with dataflash and do not care about people that wants this.
I have two AT91RM9200 based boards here. One boots from dataflash, stores enviroment here and loads kernel from NAND. Sure I care about dataflash support...
I think that consistency in argumentation is a reasonable demand.
- BYTE WRITE
Why should you waste time on copying dataflash to SDRAM when you can do operations inside the internal SRAM.
It is not desirable to transfer 20,000 bits between CPU and dataflash when 100-200 are sufficient.
Could you please point exactly where I suggested such behaviour?
Best regards, ladis
On Sat, May 26, 2007 at 12:53:31PM +0200, Ulf Samuelsson wrote:
=> OK, then tell me how to compare a 6 MB file in flash with a 6MB file in SDRAM when your SDRAM is 8 MB.
Chunk by chunk?
Yes, but not by doing that manually. It was suggested that you do not need any command to do that compare since you can copy from dataflash to SDRAM. If you use 64 kB buffers, then you have to type in 96 U.boot commands to do that single compare.
But indeed, there is a little problem. It would be usefull to show address which contains diferences from the beginning of dataflash (or dataflash partition)...
We are *not* running on PC's.
Anyone who copies whole file into RAM and then does compare has either a lot of RAM or doesn't deserve to be called power user.
The amount of flash and ram is typically determined by the need of the application, and not by the qualifications of the person running U-boot.
Lines 303-308 of common/cmd_mem.c reads (cmp command): #ifdef CONFIG_HAS_DATAFLASH if (addr_dataflash(addr1) | addr_dataflash(addr2)){ puts ("Comparison with DataFlash space not supported.\n\r"); return 0; } #endif
On my boards it leads to: # cmp.l 10000000 C0000000 10 Comparison with DataFlash space not supported.
Therefore it seems I have to copy to RAM anyway and it doesn't seem to be fixed by any patch at ftp 81.80.104.162.
That is funny because I have done that patch.
I would have more respects for exhibited views if writes to parallel flash is removed from cp.b... Parallel flash is clearly not "memory" for write purposes, only for read purposes. Its existence in cp.b will force endless #ifdefs or support for parallel flash will have to remain and bloat code.
Erm... This interface is finished, contains finite number of features and certainly finite numbers of #ifdefs. This number could be reduced even more by removing hacks for MMC.
It is not finished, since people would like to "clean" up the memory commands but you are avoiding the point, in favour of advocacy.
Parallel flash CANNOT be written in the same way as SDRAM. Why should then parallel flash be considered as memory.
I really do not advocate that this is changed, but I would like to see some consistency in arguments.
You could then argue that support for parallel flash should be removed from all commands to remove inconsistencies...I do not think this will happen because it is *useful* and because this affects boards which people that makes decisions works with this. They do not work with dataflash and do not care about people that wants this.
I have two AT91RM9200 based boards here. One boots from dataflash, stores enviroment here and loads kernel from NAND. Sure I care about dataflash support...
I think that consistency in argumentation is a reasonable demand.
- BYTE WRITE
Why should you waste time on copying dataflash to SDRAM when you can do operations inside the internal SRAM.
It is not desirable to transfer 20,000 bits between CPU and dataflash when 100-200 are sufficient.
Could you please point exactly where I suggested such behaviour?
Wolfgang suggested byte writes should be implemented as 1) Copy to SDRAM 2) Modify SDRAM 3) Write to Dataflash
which will result in above inefficiency.
Best regards, ladis
Personally, I would like to see an architecture where every memory registers itself in a common database.
When a memory command is executed, the command should be split the memory areas involved into pages, and execute the command on each page.
Each driver would, if neccessary copy its data into a SDRAM buffer before use.
cp.b
for each registred memory do if(src = parse(parameter 1)) != FAIL) break; end if done if(src == FAIL) then src = SDRAM end if
for each registred memory do if(dst = parse(parameter 2)) != FAIL) break; end if; done if(dst == FAIL) then dst = SDRAM end if
/* Now you have src pointing to a descriptor for the first parameter and dst pointing to a descriptor for the second parameter. - Without any ifdefs, and easily extendible. */ pageesize = determine_pagesize(src,dst);
for(i = 0; i < size; i += pagesize) { Ensure that src is accessible using a memory pointer to SDRAM * maybe by reading from storage to SDRAM * if in SDRAM/par flash memory, just return pointer Copy page to destination, using access routine defined in storage driver. }
Best Regards Ulf Samuelsson
On Mon, May 28, 2007 at 04:08:50PM +0200, Ulf Samuelsson wrote:
On Sat, May 26, 2007 at 12:53:31PM +0200, Ulf Samuelsson wrote:
=> OK, then tell me how to compare a 6 MB file in flash with a 6MB file in SDRAM when your SDRAM is 8 MB.
Chunk by chunk?
Yes, but not by doing that manually. It was suggested that you do not need any command to do that compare since you can copy from dataflash to SDRAM. If you use 64 kB buffers, then you have to type in 96 U.boot commands to do that single compare.
Not if you could run command in a loop.
But indeed, there is a little problem. It would be usefull to show address which contains diferences from the beginning of dataflash (or dataflash partition)...
We are *not* running on PC's.
Anyone who copies whole file into RAM and then does compare has either a lot of RAM or doesn't deserve to be called power user.
The amount of flash and ram is typically determined by the need of the application, and not by the qualifications of the person running U-boot.
Sure. Point was that sane power user will compare chunk by chunk (6 MB file and 8 MB RAM case).
Lines 303-308 of common/cmd_mem.c reads (cmp command): #ifdef CONFIG_HAS_DATAFLASH if (addr_dataflash(addr1) | addr_dataflash(addr2)){ puts ("Comparison with DataFlash space not supported.\n\r"); return 0; } #endif
On my boards it leads to: # cmp.l 10000000 C0000000 10 Comparison with DataFlash space not supported.
Therefore it seems I have to copy to RAM anyway and it doesn't seem to be fixed by any patch at ftp 81.80.104.162.
That is funny because I have done that patch.
Could you point me to it, please?
[snip]
Erm... This interface is finished, contains finite number of features and certainly finite numbers of #ifdefs. This number could be reduced even more by removing hacks for MMC.
It is not finished, since people would like to "clean" up the memory commands but you are avoiding the point, in favour of advocacy.
NOR interface is finished.
Parallel flash CANNOT be written in the same way as SDRAM. Why should then parallel flash be considered as memory.
Mapped into address space, CAN be written in the same way as SDRAM. Note that I'm not doing any advocacy here, I want acceptable solution without drawbacks.
I really do not advocate that this is changed, but I would like to see some consistency in arguments.
See below and try to imagine what would need to be done to support 64bit virtual address space.
[snip]
Wolfgang suggested byte writes should be implemented as
- Copy to SDRAM
- Modify SDRAM
- Write to Dataflash
which will result in above inefficiency.
That's only suggestion.
Personally, I would like to see an architecture where every memory registers itself in a common database.
And memory is even IDE disc as you suggested in your database example?
When a memory command is executed, the command should be split the memory areas involved into pages, and execute the command on each page.
So here you basicaly proposing something like u-boot virtual memory. What if I have storage greater than addresable area? NAND chips are pretty close to that for example (and even today it is quite easy to build such board using few more NAND chips) and consider design with 64MB of RAM, 2GB NAND and 1G SD card and 1G USB stick. Will you move to 64bit address space? In bootloader? Bootloader has always been something simple, usefull only to boot application which does the job. Perhaps it would help if you could express what you expect from a bootloader...
Each driver would, if neccessary copy its data into a SDRAM buffer before use.
cp.b
for each registred memory do if(src = parse(parameter 1)) != FAIL) break; end if done if(src == FAIL) then src = SDRAM end if for each registred memory do if(dst = parse(parameter 2)) != FAIL) break; end if; done if(dst == FAIL) then dst = SDRAM end if /* Now you have src pointing to a descriptor for the first parameter and dst pointing to a descriptor for the second parameter. - Without any ifdefs, and easily extendible. */ pageesize = determine_pagesize(src,dst); for(i = 0; i < size; i += pagesize) { Ensure that src is accessible using a memory pointer to SDRAM * maybe by reading from storage to SDRAM * if in SDRAM/par flash memory, just return pointer Copy page to destination, using access routine defined in storage driver. }
That's it... Above example needs 64bit virtual address space.
Principle described above has few serious drawbacks. Storage is no longer storage, but randomly mapped memory. As a u-boot programer you have to determine such memory map and as a u-boot user you have to remember it. If you are using partitions you probably want to pass such partitions mapping to linux. Curently it very easy and comfortale, you can copy kernel and rootfs image to partion refered by name, without even knowing where it is. And btw, where would you map OOB area of NAND chip?
Best regards, ladis
On 5/28/07, Ladislav Michl ladis@linux-mips.org wrote:
Principle described above has few serious drawbacks. Storage is no longer storage, but randomly mapped memory. As a u-boot programer you have to determine such memory map and as a u-boot user you have to remember it.
Exactly. Giving storage devices which aren't memory mapped a "virtual" address is a bad idea. And the database example is crap because it depends on hardware paging, which isn't being proposed here (how many architectures supported by u-boot can do 64-bit virtual address in hardware?)
That said, I do think Ulf has a point -- being able to compare a block of data in nand- or dataflash with something in memory (or other kinds of storage) is a pretty neat feature. But it should not be done by using "virtual" addresses (due to the problems described above), and it should be implemented in a generic way. Pouring #ifdefs all over cmd_mem.c is short-sighted and selfish.
I'm not sure if anyone's suggested this before, or whether it was well-received or not, but why don't we generalize the cmd_mem syntax a little? For example, let every memory address can be prefixed by an optional "storage specifier" and a ':' character. The default "storage specifier" is "mem", so that if it is omitted everything will work as before, but you can also do things like
cp.b df0:0x4200 0x85000000 cmp.l 0x24000000 nand1.1:0x123456789
and so on. Whatever comes after ':' is passed to the storage handler as a string, so if some storage types need 64-bit offsets, the handler can support it without disturbing anything else.
What do you think about something like that? The actual syntax probably needs some work to get right, and we need to take care to ensure backwards compatibility of course.
This does actually come quite close to Ulf's proposal, but instead of requiring the user to remember a "virtual address", we instead require him to enter a logical device and an offset, ie.. kind of a simple VFS without any actual files involved...
Haavard
On 5/28/07, Ladislav Michl ladis@linux-mips.org wrote:
Principle described above has few serious drawbacks. Storage is no longer storage, but randomly mapped memory. As a u-boot programer you have to determine such memory map and as a u-boot user you have to remember it.
Exactly. Giving storage devices which aren't memory mapped a "virtual" address is a bad idea. And the database example is crap because it depends on hardware paging, which isn't being proposed here (how many architectures supported by u-boot can do 64-bit virtual address in hardware?)
That said, I do think Ulf has a point -- being able to compare a block of data in nand- or dataflash with something in memory (or other kinds of storage) is a pretty neat feature. But it should not be done by using "virtual" addresses (due to the problems described above), and it should be implemented in a generic way. Pouring #ifdefs all over cmd_mem.c is short-sighted and selfish.
My proposal removes all #ifdefs from memory commands.
I'm not sure if anyone's suggested this before, or whether it was well-received or not, but why don't we generalize the cmd_mem syntax a little? For example, let every memory address can be prefixed by an optional "storage specifier" and a ':' character. The default "storage specifier" is "mem", so that if it is omitted everything will work as before, but you can also do things like
cp.b df0:0x4200 0x85000000 cmp.l 0x24000000 nand1.1:0x123456789
Which is 100% compatible with my proposal.
"cp.b df0:0x4200 0x85000000 ${size}"
is decoded by the command parser and the "cp" command is selected.
"cp" sends "df0:0x4200" down the chain of registred memory/storage devices. The NAND flash driver will not understand "df0" so it ignores the parameter. The dataflash driver detects that this is a dataflash and decides it is a success and returns a pointer to a descriptor allowing the "cp" command to read/write dataflash.
Then "cp" sends down the "0x85000000" parameter, and it is decided that this is located in SDRAM.
"cp" then calls the dataflash specific copy to sdram routine (from the descriptor) using 0x85000000 and ${size} as parameters.
The dataflash copy routine will (in the AT91 case) just set up the initial address and copy chunks of 64 kB using the DMA controller (PDC).
No need for any #ifdefs to handle this as far as I see.
and so on. Whatever comes after ':' is passed to the storage handler as a string, so if some storage types need 64-bit offsets, the handler can support it without disturbing anything else.
What do you think about something like that? The actual syntax probably needs some work to get right, and we need to take care to ensure backwards compatibility of course.
This does actually come quite close to Ulf's proposal, but instead of requiring the user to remember a "virtual address", we instead require him to enter a logical device and an offset, ie.. kind of a simple VFS without any actual files involved...
I never said that you need to supply an address, you need to supply a parameter, but the syntax of this can be driver specific.
Haavard
Best Regards Ulf Samuelsson
On Mon, May 28, 2007 at 06:56:30PM +0200, Ulf Samuelsson wrote:
I'm not sure if anyone's suggested this before, or whether it was well-received or not, but why don't we generalize the cmd_mem syntax a little? For example, let every memory address can be prefixed by an optional "storage specifier" and a ':' character. The default "storage specifier" is "mem", so that if it is omitted everything will work as before, but you can also do things like
As far as I remember it was proposed by Tolunay Orkun in 'Atmel Dataflash hooks' thread: http://article.gmane.org/gmane.comp.boot-loaders.u-boot/26126 There was no reaction.
cp.b df0:0x4200 0x85000000 cmp.l 0x24000000 nand1.1:0x123456789
Which is 100% compatible with my proposal.
"cp.b df0:0x4200 0x85000000 ${size}"
Okay, I'm sorry for not understanding you correctly. I have no objections to this proposal (as I have no customers and can easily start using new syntax :-))
Best regards, ladis
In message 20070528193940.GA3014@linux-mips.org you wrote:
I'm not sure if anyone's suggested this before, or whether it was well-received or not, but why don't we generalize the cmd_mem syntax a little? For example, let every memory address can be prefixed by an optional "storage specifier" and a ':' character. The default "storage specifier" is "mem", so that if it is omitted everything will work as before, but you can also do things like
As far as I remember it was proposed by Tolunay Orkun in 'Atmel Dataflash hooks' thread: http://article.gmane.org/gmane.comp.boot-loaders.u-boot/26126 There was no reaction.
No explicit reaction, maybe.
But my position on this is clear: the "mem" commands are supposed to work on memory (bus addressable memaory that is, as previously defined a couple of times). No such prefixes will be accepted.
Instead, support for storage devices shall be handled by the respective storage device commands, like "ide", "usb", "nand", and (to be implemented) "dataflash" (or whatever name will be chosen).
Best regards,
Wolfgang Denk
This patch is obviously not going to be accepted in the main tree...
Here is my patch to allow * cp.b from SDRAM to dataflash with CRC calculation and storage * cp.b from dataflash to SDRAM with CRC calculation and check * compare dataflash to SDRAM. Looks like it disappeared from the 1.1.2-atmel version by mistake.
The reason for adding the CRC is that the CRC check used when booting the kernel does not really trigger the user to think along the right lines. If you write the kernel after the root fs, you overwrite the beginning of the root fs if the kernel does not fit. If you write the kernel before you write the root fs, you will overwrite the last part of the kernel if the kernel does not fit.
By doing the crc check when copying from the dataflash to SDRAM you immediately pinpoint the problem. If U-Boot contained a command to write the kernel to dataflash, then this command could check that the image fits into the partition allocated for the kernel, and would report a "kernel too large" warning message.
diff -urN u-boot-1.1.4-0rig/common/cmd_mem.c u-boot-1.1.4/common/cmd_mem.c --- u-boot-1.1.4-0rig/common/cmd_mem.c 2006-08-18 18:35:32.000000000 +0200 +++ u-boot-1.1.4/common/cmd_mem.c 2006-08-18 19:32:33.000000000 +0200 @@ -64,6 +64,7 @@ } #endif
+ #if (CONFIG_COMMANDS & CFG_CMD_MEMORY)
#ifdef CMD_MEM_DEBUG @@ -71,6 +72,63 @@ #else #define PRINTF(fmt,args...) #endif +#ifdef CONFIG_HAS_DATAFLASH +#define DF_PAGE_VALID 0x01 +typedef unsigned char dfpagebuf[CFG_DATAFLASH_PAGE_SIZE]; +dfpagebuf dfcache[3]; // Cache buffers for dataflash +unsigned char dfvalid[3]; // if DF_PAGE_VALID is set, then the, cache buffer contents are valid +unsigned char dflru[3]; // Least recently used algorithm to determine which buffer to reuse +unsigned int dfpage[3]; // TLB for cache, tells which page is loaded into dfcache + +void flush_dfcache(void) +{ + dfvalid[0] = dfvalid[1] = dfvalid[2] = 0; + dflru[0] = 0; + dflru[1] = 1; + dflru[2] = 2; +} + +ulong translate(unsigned int addr) +{ + unsigned int page = (addr) / CFG_DATAFLASH_PAGE_SIZE; + unsigned int index = (addr) - (page * CFG_DATAFLASH_PAGE_SIZE); + unsigned int i,rc; + for(i = 0; i < 2; i++) { + if((page == dfpage[i]) && (dfvalid[i] & DF_PAGE_VALID)) { + if(dflru[0] == i) { + // nothing + } else if(dflru[1] == i) { + // swap with 0 + dflru[1] = dflru[0]; + dflru[0] = i; + } else { + // shift down + dflru[2] = dflru[1]; + dflru[1] = dflru[0]; + dflru[0] = i; + } + return (unsigned int) &dfcache[i][index]; + } + } + // not found + // use buffer found in dflru[2] + i = dflru[2]; + if ((rc = read_dataflash(addr, CFG_DATAFLASH_PAGE_SIZE, dfcache[i])) == DATAFLASH_OK){ + dflru[2] = dflru[1]; + dflru[1] = dflru[0]; + dflru[0] = i; + dfvalid[i] |= DF_PAGE_VALID; + return (unsigned int) &dfcache[i][index]; + } + return 0; +} +void df_writed(unsigned char *ldest, ulong laddr) +{ + +} +void df_writew(unsigned char *ldest,ushort laddr) {} +void df_writeb(unsigned char *ldest, u_char laddr) {} +#endif
static int mod_mem(cmd_tbl_t *, int, int, int, char *[]);
@@ -316,6 +374,7 @@ int do_mem_cmp (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { ulong addr1, addr2, count, ngood; + ulong laddr1, laddr2; int size; int rcode = 0;
@@ -338,12 +397,57 @@ count = simple_strtoul(argv[3], NULL, 16);
#ifdef CONFIG_HAS_DATAFLASH +/* if (addr_dataflash(addr1) | addr_dataflash(addr2)){ puts ("Comparison with DataFlash space not supported.\n\r"); return 0; } +*/ + if (addr_dataflash(addr1) | addr_dataflash(addr2)){ + flush_dfcache(); + ngood = 0; + while (count-- > 0) { + laddr1 = translate((unsigned int) addr1); + laddr2 = translate((unsigned int) addr2); + if (size == 4) { + ulong word1 = *(ulong *)laddr1; + ulong word2 = *(ulong *)laddr2; + if (word1 != word2) { + printf("word at 0x%08lx (0x%08lx) " + "!= word at 0x%08lx (0x%08lx)\n", + laddr1, word1, laddr2, word2); + rcode = 1; + break; + } + } + else if (size == 2) { + ushort hword1 = *(ushort *)laddr1; + ushort hword2 = *(ushort *)laddr2; + if (hword1 != hword2) { + printf("halfword at 0x%08lx (0x%04x) " + "!= halfword at 0x%08lx (0x%04x)\n", + laddr1, hword1, laddr2, hword2); + rcode = 1; + break; + } + } + else { + u_char byte1 = *(u_char *)laddr1; + u_char byte2 = *(u_char *)laddr2; + if (byte1 != byte2) { + printf("byte at 0x%08lx (0x%02x) " + "!= byte at 0x%08lx (0x%02x)\n", + laddr1, byte1, laddr2, byte2); + rcode = 1; + break; + } + } + ngood++; + addr1 += size; + addr2 += size; + } + } else { #endif - ngood = 0;
while (count-- > 0) { @@ -384,6 +488,10 @@ addr1 += size; addr2 += size; } +#ifdef CONFIG_HAS_DATAFLASH + } +#endif +
printf("Total of %ld %s%s were the same\n", ngood, size == 4 ? "word" : size == 2 ? "halfword" : "byte", @@ -484,11 +592,30 @@ /* Check if we are copying from RAM or Flash to DataFlash */ if (addr_dataflash(dest) && !addr_dataflash(addr)){ int rc; +#ifdef CONFIG_DATAFLASH_CRC + unsigned int crc; + unsigned char *p; + puts ("Copy to DataFlash with CRC... "); + crc = crc32 (0, (const uchar *) addr, count*size); + // Save CRC in small endian format + printf("0x%08x written to [%08x] ... ",crc,(unsigned int) (dest + count * size)); + p = (unsigned char *) addr+(count*size); + *p++ = (unsigned char) crc & 0xff; + crc >>= 8; + + *p++ = (unsigned char) crc & 0xff; + crc >>= 8;
- puts ("Copy to DataFlash... "); + *p++ = (unsigned char) crc & 0xff; + crc >>= 8;
- rc = write_dataflash (dest, addr, count*size); + *p = (unsigned char) crc & 0xff;
+ rc = write_dataflash (dest, addr, count*size+4); +#else + puts ("Copy to DataFlash... "); + rc = write_dataflash (dest, addr, count*size); +#endif if (rc != 1) { dataflash_perror (rc); return (1); @@ -504,17 +631,70 @@ #endif ){ int rc; +#ifdef CONFIG_DATAFLASH_CRC + unsigned char *p; + unsigned int crc,oldcrc; + puts ("Copy from DataFlash with CRC..."); + rc = read_dataflash(addr, (count * size)+4, (char *) dest); + if (rc != 1) { + dataflash_perror (rc); + return (1); + } + // Read CRC in small endian format, MSB first + + p = (unsigned char *) dest + (count * size) + 3; + oldcrc = *p--; + oldcrc = (oldcrc << 8) | *p--; + oldcrc = (oldcrc << 8) | *p--; + oldcrc = (oldcrc << 8) | *p; + crc = crc32 (0, (const uchar *) dest, count*size); + if(crc != oldcrc) { + printf("\n\rBad CRC, %x expected, found %x... \n\r",oldcrc,crc); + return 1; + } else { + printf("[0x%x]",crc); + } +#else + puts ("Copy from DataFlash..."); rc = read_dataflash(addr, count * size, (char *) dest); if (rc != 1) { dataflash_perror (rc); return (1); } +#endif + if (rc != 1) { + dataflash_perror (rc); + return (1); + } + puts ("done\n\r"); return 0; }
if (addr_dataflash(addr) && addr_dataflash(dest)){ - puts ("Unsupported combination of source/destination.\n\r"); + puts ("Cannot copy between two dataflash areas\n\r"); return 1; +#if 0 + unsigned int addr_page = (unsigned int) addr / CFG_DATAFLASH_PAGE_SIZE ; + unsigned int dest_page = (unsigned int) dest / CFG_DATAFLASH_PAGE_SIZE; + unsigned char *ldest,*laddr; if(addr_page == dest_page) + puts ("Cannot copy within same page\n\r"); + return 1; + } + flush_dfcache(); + // Read into buffer 0 + while (count-- > 0) { + laddr = translate(addr); // Read source page if not inside + ldest = translate(dest); // Read dest page if not inside + if (size == 4) + df_writed(ldest, *((ulong *)laddr); + else if (size == 2) + df_writew(ldest,*((ushort *)laddr); + else + df_writeb(ldest, *((u_char *)laddr); + addr += size; + dest += size; + } +#endif } #endif
In message 465CA599.7070704@atmel.com you wrote:
This patch is obviously not going to be accepted in the main tree...
You are right.
The coding style violations alone (trailing white space, C++ comments, broken indentation, incorrect brace style, ...) were reason enough to reject it.
There are other, more serious reasons (like adding dataflash specific type definitions and code to a global (and basicly unrelated) file, and of course the whole concept of adding such code to the "memory" commands where it does not belong. This has been discussed before.
For the record: I hereby reject this patch. Please implement this function as part of a dataflash specific subcommand, following the example of NAND flash. Also clean up the codingstyle before resubmitting.
Some more comments below...
Here is my patch to allow
- cp.b from SDRAM to dataflash with CRC calculation and storage
- cp.b from dataflash to SDRAM with CRC calculation and check
- compare dataflash to SDRAM.
Looks like it disappeared from the 1.1.2-atmel version by mistake.
The reason for adding the CRC is that the CRC check used when booting the kernel does not really trigger the user to think along the right lines. If you write the kernel after the root fs, you overwrite the beginning of the root fs if the kernel does not fit. If you write the kernel before you write the root fs, you will overwrite the last part of the kernel if the kernel does not fit.
If you want to do something like this (opinions about usefulnes will probably differ - I'm not going to discuss this here), you don't need to add special commands. You should be able to use the existing commands, eventually packed into a macro definition.
By doing the crc check when copying from the dataflash to SDRAM you immediately pinpoint the problem.
I don't understand why your appended CRC word is in any way better than our embedded / prepended one, the result of the check will be the same in each case. If you need check the CRC independetly from the boot command, you can always use the "imi" command, or "crc" itself.
If U-Boot contained a command to write the kernel to dataflash, then this command could check that the image fits into the partition allocated for the kernel, and would report a "kernel too large" warning message.
This is not an issue of a special write command, but of such a concept of partitions in flash memory which, at the moment, does not exist.
Best regards,
Wolfgang Denk
If you want to do something like this (opinions about usefulnes will probably differ - I'm not going to discuss this here), you don't need to add special commands. You should be able to use the existing commands, eventually packed into a macro definition.
* It is not a special command, it is a configurable side effect of reading/writing to dataflash.
* running mkimage on the host will of course generate the crc check but U-boot does not know that they thing you are copying to storage is an image so it will not check crc automatically.
For your recommendation to work, people have to suspect that the things stored in flash is bad. In my experience, people do not realize that the image is bad, and think they have made a mistake when generating the kernel or the root fs and spend significant time trying to figure out what the problem is and then call me. If I generate/check the crc on all writes/reads, then this support problem will more or less go away. With your proposal, the support problem will remain.
I can of course generate macros, but again, people will want to do their own macros, and they will run into the problem.
Reducing support, is very important, and is worth the extra code space needed.
By doing the crc check when copying from the dataflash to SDRAM you immediately pinpoint the problem.
I don't understand why your appended CRC word is in any way better than our embedded / prepended one, the result of the check will be the same in each case. If you need check the CRC independetly from the boot command, you can always use the "imi" command, or "crc" itself.
If U-Boot contained a command to write the kernel to dataflash, then this command could check that the image fits into the partition allocated for the kernel, and would report a "kernel too large" warning message.
This is not an issue of a special write command, but of such a concept of partitions in flash memory which, at the moment, does not exist.
The current dataflash drivers divides the flash into partitions.
In my private version, the partitions can have a name, and the address of the partition is automatically stored in an environment variable with this name,
It would be fairly easy to implement a command which flashes the kernel/rootfs, to check that they fit into the named partition.
Best regards,
Wolfgang Denk
Dear Ulf,
in message 465CB41F.3050002@atmel.com you wrote:
- It is not a special command, it is a configurable side effect of reading/writing to dataflash.
It is a special command, as "normal" copy operations don't do this.
- running mkimage on the host will of course generate the crc check but U-boot does not know that they thing you are copying to storage is an image so it will not check crc automatically.
It will check it automatically whenever you use such an image, for example as part of a "bootm" or an "iminfo" command.
For your recommendation to work, people have to suspect that the things stored in flash is bad.
If you get an error message that says "bad CRC" you should indeed suspect that your image might have been corrupted.
In my experience, people do not realize that the image is bad, and think they have made a mistake when generating the kernel or the root fs and spend significant time trying to figure out what the problem is and then call me.
Write a FAQ?
If I generate/check the crc on all writes/reads, then this support problem will more or less go away. With your proposal, the support problem will remain.
I cannot follow this argument. For me there is no difference between both CRC tests, except thatone is completely nonstandard and causes a mess of code in areas where it does not belong.
This is not an issue of a special write command, but of such a concept of partitions in flash memory which, at the moment, does not exist.
The current dataflash drivers divides the flash into partitions.
In my private version, the partitions can have a name, and the address of the partition is automatically stored in an environment variable with this name,
I guess that you probably invented your own implementation, or did you extend the "mtdparts" command for this purpose?
It would be fairly easy to implement a command which flashes the kernel/rootfs, to check that they fit into the named partition.
In the former case, please rewrite your code to fit into the existing mtdparts framework. In the second case, please post your patches.
Best regards,
Wolfgang Denk
Wolfgang Denk skrev:
Dear Ulf,
in message 465CB41F.3050002@atmel.com you wrote:
- It is not a special command, it is a configurable side effect of reading/writing to dataflash.
It is a special command, as "normal" copy operations don't do this.
- running mkimage on the host will of course generate the crc check but U-boot does not know that they thing you are copying to storage is an image so it will not check crc automatically.
It will check it automatically whenever you use such an image, for example as part of a "bootm" or an "iminfo" command.
And as I pointed out, this generates a problem. The problem is less when the kernel is overwritten than when the root fs is written.
For your recommendation to work, people have to suspect that the things stored in flash is bad.
If you get an error message that says "bad CRC" you should indeed suspect that your image might have been corrupted.
In my experience, people do not realize that the image is bad, and think they have made a mistake when generating the kernel or the root fs and spend significant time trying to figure out what the problem is and then call me.
Write a FAQ?
This assumes people find and read the FAQ, before they call me. They don't...
If I generate/check the crc on all writes/reads, then this support problem will more or less go away. With your proposal, the support problem will remain.
I cannot follow this argument. For me there is no difference between both CRC tests, except thatone is completely nonstandard and causes a mess of code in areas where it does not belong.
The difference is how much knowledge you require from the user. The average newbie does not know that you have added a CRC to the image and is less aware that you can check the CRC using suggested commands.
This difference can be measured in the number of incoming telephones calls.
This is not an issue of a special write command, but of such a concept of partitions in flash memory which, at the moment, does not exist.
The current dataflash drivers divides the flash into partitions.
In my private version, the partitions can have a name, and the address of the partition is automatically stored in an environment variable with this name,
I guess that you probably invented your own implementation, or did you extend the "mtdparts" command for this purpose?
No, this is part of "drivers/dataflash.c" in the current trunk This was added to u-boot 11 Jun 2003 according to CHANGELOG The mtdparts command was added after 11 Jan 2005 according the CHANGELOG
The named partitions is a small extension of "drivers/dataflash.c"
It would be fairly easy to implement a command which flashes the kernel/rootfs, to check that they fit into the named partition.
In the former case, please rewrite your code to fit into the existing mtdparts framework. In the second case, please post your patches.
As you see above, I am extending existing code, which is not mtdparts and since this extends the dataflash support you will reject it, so it is pointless to even try to submit a patch for inclusion in main trunk.
The reason I sent the patch was to allow people to extend their own u-boot version. I will make sure the patch is available in my own version as well.
The dataflash partitioning scheme is static, which is a disadvantage, but If I fix anything, I probably make the partition sizes a configurable item in my buildroot and generate the warning there, if the kernel size is non compliant.
Anyway mtdparts seems to depend on JFFS2. That dependency needs to go away first. If you want to store EXT2 fs in the rootfs partition, you should not have to add JFFS2 code.
Best regards,
Wolfgang Denk
In message 465CC968.6090801@atmel.com you wrote:
I guess that you probably invented your own implementation, or did you extend the "mtdparts" command for this purpose?
No, this is part of "drivers/dataflash.c" in the current trunk This was added to u-boot 11 Jun 2003 according to CHANGELOG The mtdparts command was added after 11 Jan 2005 according the CHANGELOG
The named partitions is a small extension of "drivers/dataflash.c"
OK, so we have here another area in the staflash support that shall be cleaned up and merged into one, common implementation. Please move this into the "mtdparts" support.
In the former case, please rewrite your code to fit into the existing mtdparts framework. In the second case, please post your patches.
As you see above, I am extending existing code, which is not mtdparts
From the maintenance point of view it is better to avoid multiple
different and incompatible implementations of the same feature. As your partition support cannot provide the fuctions that are addressed by the "mtdparts" implementation, while "mtdparts" can replace yours, both implementations should be merged into the "mtdparts" command.
and since this extends the dataflash support you will reject it, so it is pointless to even try to submit a patch for inclusion in main trunk.
I do not reject dataflash support poer se. Please try to understand that. As maintainer of the whole project I just cannot accept that each maintainer of a group of boards comes up with differeing implementations of certain functions or with a diverging design philosophy.
You know exactly what is wanted, so maybe you can try to start contributing to the needed new parts instead of trying to extend parts that have been declared to be candidates for replacement. That would be much more useful and less frustrating for everybody.
The dataflash partitioning scheme is static, which is a disadvantage, but If I fix anything, I probably make the partition sizes a configurable item in my buildroot and generate the warning there, if the kernel size is non compliant.
If you fix anything, then please by making it compatible with the poublic U-Boot source tree,i. e. by using the mtdparts command for this purpose.
Anyway mtdparts seems to depend on JFFS2. That dependency needs to go away first.
Agreed. Patches are welcome.
If you want to store EXT2 fs in the rootfs partition, you should not have to add JFFS2 code.
Agreed.
Best regards,
Wolfgang Denk
On Wed, May 30, 2007 at 08:57:50AM +0200, Wolfgang Denk wrote:
In message 465CC968.6090801@atmel.com you wrote:
The dataflash partitioning scheme is static, which is a disadvantage, but If I fix anything, I probably make the partition sizes a configurable item in my buildroot and generate the warning there, if the kernel size is non compliant.
If you fix anything, then please by making it compatible with the poublic U-Boot source tree,i. e. by using the mtdparts command for this purpose.
Here it might be usefull to avoid modifications of certain partition sizes. For example partition containing environment shouldn't be modified, because code which read it from U-Boot has size and offset hardcoded, therefore modifying it will result to bad surprise...
Best regards, ladis
In message 20070530105256.GA7649@michl.2n.cz you wrote:
If you fix anything, then please by making it compatible with the poublic U-Boot source tree,i. e. by using the mtdparts command for this purpose.
Here it might be usefull to avoid modifications of certain partition sizes. For example partition containing environment shouldn't be modified, because code which read it from U-Boot has size and offset hardcoded, therefore modifying it will result to bad surprise...
The user might prepare for the installation of a new version of U-Boot where the environment is in new sectors (for example, because U-Boot has grown after adding new features - been there before), or for switching from single to redundant environment, or ...
So actually the user might want to do some clever thing. Please don;t prevent him from doing that.
Best regards,
Wolfgang Denk
----- Original Message ----- From: "Wolfgang Denk" wd@denx.de To: "Ladislav Michl" ladis@linux-mips.org Cc: "Ulf Samuelsson" ulf@atmel.com; "uboot" u-boot-users@lists.sourceforge.net Sent: Wednesday, May 30, 2007 3:43 PM Subject: Re: [U-Boot-Users] Question about CFG_ENV_ADDR during RAMBOOT
In message 20070530105256.GA7649@michl.2n.cz you wrote:
If you fix anything, then please by making it compatible with the poublic U-Boot source tree,i. e. by using the mtdparts command for this purpose.
Here it might be usefull to avoid modifications of certain partition sizes. For example partition containing environment shouldn't be modified, because code which read it from U-Boot has size and offset hardcoded, therefore modifying it will result to bad surprise...
The user might prepare for the installation of a new version of U-Boot where the environment is in new sectors (for example, because U-Boot has grown after adding new features - been there before), or for switching from single to redundant environment, or ...
So actually the user might want to do some clever thing. Please don;t prevent him from doing that.
Since everything I work on is dataflash based, U-Boot executes from SDRAM. While certain partitions are write protected, you an easily unprotect any or all of the dataflash pages.
From U-Boot you then can download the new U-Boot image to SDRAM using tftp
and then cp.b (at least for now :-) to dataflash and then reboot.
The new image has defined it own partitions, and the environment area (defined by the new image) is initialized according to compile time values of the new image
While the named partitions will initialize some environment variables, there is nothing that forces the user to actually use these in the bootargs or anywhere else. I want the novice user to have a straightforward path to boot Linux without hassle. This does not block anyone else from shooting themselves in the foot.
Best regards,
Wolfgang Denk
--
Best Regards Ulf Samuelsson ulf@atmel.com Atmel Nordic AB Mail: Box 2033, 174 02 Sundbyberg, Sweden Visit: Kavallerivägen 24, 174 58 Sundbyberg, Sweden Phone +46 (8) 441 54 22 Fax +46 (8) 441 54 29 GSM +46 (706) 22 44 57
Technical support when I am not available: AT90 AVR Applications Group: mailto:avr@atmel.com AT91 ARM Applications Group: mailto:at91support@atmel.com AVR32 Applications Group mailto:avr32@atmel.com http://www.avrfreaks.net/; http://avr32linux.org/ http://www.at91.com/ ; ftp://at91dist:distrib@81.80.104.162/
participants (4)
-
Håvard Skinnemoen -
Ladislav Michl -
Ulf Samuelsson -
Wolfgang Denk