[U-Boot-Users] cfi_flash is now working with 64 bit port width
Hi,
I have resolved my problem with cfi_flash driver. I would like to share what I have learned during my struggle with cfi_flash driver.
The posts for that problem;
http://sourceforge.net/mailarchive/message.php?msg_id=11994639
http://sourceforge.net/mailarchive/message.php?msg_id=12058552
What caused the problem and how is it solved:
Flash chips in my board are connected to 64 bit data bus. And the problem is simply about writing 64 bit data at once. cfi_flash driver used "unsigned long long" variables for 64 bit data and write them to the bus simply using long long pointers (For me that is a bug. And I would like to release a patch for it). Any integer definition (like int, long, long long etc.) makes gcc to use 32 bit registers of the cpu (for 32 bit ppc's). Long long is 64 bit on the memory that is true, but transfering a long long requires two cycles. To write a 64 bit data at once we have to use floating point registers. A way of using these registers in C is to define the variables as "double". But somehow defining double didn't work in u-boot or with my ppc_82xx-gcc compiler (eldk's compiler). An other way is to use assembly floating point instructions but double precision forms. Such as "lfd" and "strfd" commands for ppc have to be used.
Jerry Van Baren had warned me previously but I couldn't make sense with his suggestions at the moment. What make me confused that writing flash commands from u-boot shell by using mw.l command was working for protect and erase operations. Since mw.l makes 32 bit bus transaction, for example a 0x60606060 command was beeing recognized by all the four flash chips. On the other way a 0x00600060 command is caused failure (I had expected that it will work two out of four flash chips). Now when I solved the problem by using floating point registers, I understood Jerry's suggestions exactly! You can reach his post by the link. (http://sourceforge.net/mailarchive/message.php?msg_id=11994640) But I still can not solve the magic that while sending 0x60606060 by mw.l command works but 0x00600060 not!
Lessons:
So lesson-1: if hardware people gives you a 64 bit bus, use double or directly asm floating point instructions.
I think cfi_driver works with the boards that the flash chips are selected by chip select signals for each 32 bit portion of the data bus. In our board all the flash chips are connected to the same chip select signal that caused the problem.
Lesson-2: first of all examine your hardware well and be aware of your hardware.
Thanks for your help: Jerry Van Baren, Yuli Barcohen, Richard Woodruff.
The patch (unformatted):
1. The following function is added.
static void write_via_fpu (vu_long * addr, ulong * data)
{
__asm__ __volatile__ ("lfd 1, 0(%0)"::"r" (data));
__asm__ __volatile__ ("stfd 1, 0(%0)"::"r" (addr));
}
2. in /drivers/cfi_flash.c
line 850;
- *addr.llp = cword.ll;
+ write_via_fpu((vu_long*)addr.llp, (ulong*)&cword.ll);
line 1159;
- cptr.llp[0] = cword.ll;
+ write_via_fpu((vu_long*)cptr.llp, (ulong*)&cword.ll);
P.S. this function will work if fpu is enabled. If not it may be enabled by setting the fpu bit in msr as follows;
unsigned long msr;
msr = get_msr();
set_msr(msr | MSR_FP);
*_msr functions may be found by grepping the uboot directory.
3. The patch is not tested when CFG_FLASH_USE_BUFFER_WRITE is defined.
Same changes as in 2 should be done for that case.
If those changes are accepted, I can arrange a formal patch.
Finally, I would like to ask about the Tolunay Orkun's patch about tout in flash_status_check function. what is the last thought for that patch
Is it planned to be released? Because it's really a bug! (Last post for the thread; http://sourceforge.net/mailarchive/message.php?msg_id=11936556)
Best Wishes.
Yusuf.
###################################################################### Dikkat:
Bu elektronik posta mesaji kisisel ve ozeldir. Eger size gonderilmediyse lutfen gondericiyi bilgilendirip mesaji siliniz. Firmamiza gelen ve giden mesajlar virus taramasindan gecirilmekte, guvenlik nedeni ile kontrol edilerek saklanmaktadir. Mesajdaki gorusler ve bakis acisi gondericiye ait olup Aselsan A.S. resmi gorusu olmak zorunda degildir.
###################################################################### Attention:
This e-mail message is privileged and confidential. If you are not the intended recipient please delete the message and notify the sender. E-mails to and from the company are monitored for operational reasons and in accordance with lawful business practices. Any views or opinions presented are solely those of the author and do not necessarily represent the views of the company.
######################################################################
AFAIK CFI flash driver is used not only on PowerPC platforms so using PPC assembler is problematic. Probably a CFG_ option and proper #ifdefs are needed. The fact that mw.l worked can indicate that the problem is not only in single 64-bit cycle. Since writing 0x60606060 worked but 0x00600060 did not, I'd suspect that byte lanes on the data bus are swapped (quite common with 16-bit chips).
Yusuf Ibrahim Ozkok wrote:
The patch (unformatted):
- The following function is added. static void write_via_fpu (vu_long * addr, ulong * data) { __asm__ __volatile__ ("lfd 1, 0(%0)"::"r" (data)); __asm__ __volatile__ ("stfd 1, 0(%0)"::"r" (addr)); }
This is probably not acceptable for cfi_flash.c. cfi_flash.c is used by multiple CPU architectures so PowerPC assembly cannot be used. You have to find a solution based on "C" only.
- in /drivers/cfi_flash.c
line 850; - *addr.llp = cword.ll; + write_via_fpu((vu_long*)addr.llp, (ulong*)&cword.ll);
line 1159; - cptr.llp[0] = cword.ll; + write_via_fpu((vu_long*)cptr.llp, (ulong*)&cword.ll);
How did you use "double" and it did not work? Please give example of the work you tried...
If you tried to assign the value to the double variable, the internal format will be different (IEEE floating point normalized). You can copy your intended data to the double variable via memcpy() or use a union of "long long" and "double", assign to "long long" member and use "double" one for storing.
Finally, I would like to ask about the Tolunay Orkun's patch about tout in flash_status_check function. what is the last thought for that patch
Is it planned to be released? Because it's really a bug! (Last post for the thread; http://sourceforge.net/mailarchive/message.php?msg_id=11936556)
I've not forgotten that. I've been extremely busy. I will try to get the patch in a couple of days.
Attention: This e-mail message is privileged and confidential. If you are not the intended recipient please delete the message and notify the sender.
Please get this crap out of mails sent to public mailing lists. By sending to public lists you cannot make any claim confidentiality... If you have anything truely confidential do not expect others to protect it. Just do not send it to the public lists...
Best regards, Tolunay
In message 42C4592E.7080009@orkun.us you wrote:
__asm__ __volatile__ ("lfd 1, 0(%0)"::"r" (data)); __asm__ __volatile__ ("stfd 1, 0(%0)"::"r" (addr)); }This is probably not acceptable for cfi_flash.c. cfi_flash.c is used by multiple CPU architectures so PowerPC assembly cannot be used. You have to find a solution based on "C" only.
...which probably does not exist, so this is a valid and working approach, although incomplete. Appropriate code for other archi- tectures can be added later. At least for MIPS. Or is there an ARM processor with 64 bit data bus?
How did you use "double" and it did not work? Please give example of the work you tried...
It did not work in the intended sense as the compiler did not generate any FP instructions - which is to be expected as we explicitely tell him to use -msoft-float.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
In message 42C4592E.7080009@orkun.us you wrote:
__asm__ __volatile__ ("lfd 1, 0(%0)"::"r" (data)); __asm__ __volatile__ ("stfd 1, 0(%0)"::"r" (addr)); }This is probably not acceptable for cfi_flash.c. cfi_flash.c is used by multiple CPU architectures so PowerPC assembly cannot be used. You have to find a solution based on "C" only.
...which probably does not exist, so this is a valid and working approach, although incomplete. Appropriate code for other archi- tectures can be added later. At least for MIPS. Or is there an ARM processor with 64 bit data bus?
Well, I really don't know if ARM has 64 bit data bus. Maybe there is maybe not.
How did you use "double" and it did not work? Please give example of the work you tried...
It did not work in the intended sense as the compiler did not generate any FP instructions - which is to be expected as we explicitely tell him to use -msoft-float.
Bummer! :(
I think in this case instead of bloating the cfi_flash.c we could allow user to define preprocessor macros in this board file for flash access so if those macros are defined, cfi_flash.c would use them and exclude its own built-in ones.
That way, any board with custom data bus (reversed lanes for example) or address bus (messed up address line) arrangements or specialized handling (need floating point) as in this case would override them easily and we would not end up blocks of #if/#elif/#else/#endif etc. in the cfi_flash.c. The board would also enable FP unit in its board file or within these functions if necessary as well.
What do you think?
I can submit a patch to do this while I am working on my other long pending patch this weekend. I promise I will get it done this time.
Best regards,
Wolfgang Denk
In message 42C469BD.4010109@orkun.us you wrote:
I think in this case instead of bloating the cfi_flash.c we could allow user to define preprocessor macros in this board file for flash access so if those macros are defined, cfi_flash.c would use them and exclude its own built-in ones.
Oh no, please don't do this!
That way, any board with custom data bus (reversed lanes for example) or address bus (messed up address line) arrangements or specialized handling (need floating point) as in this case would override them easily and we would not end up blocks of #if/#elif/#else/#endif etc. in
No. The whole idea of the CFI flash driver is to have standard code for all boards. If your design does not fit the standard, then provide your own non-standard driver. Don't add complexity to the CFI driver - it has just a single advantage (being "standard"), so please don't drop this.
the cfi_flash.c. The board would also enable FP unit in its board file or within these functions if necessary as well.
"enabling FP" has nothing to do with this. The FPU is never disabled or so. It's just that the compiler is told to never emit any FP instructions :-)
I can submit a patch to do this while I am working on my other long pending patch this weekend. I promise I will get it done this time.
No, please don't go this way.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
That way, any board with custom data bus (reversed lanes for example) or address bus (messed up address line) arrangements or specialized handling (need floating point) as in this case would override them easily and we would not end up blocks of #if/#elif/#else/#endif etc. in
No. The whole idea of the CFI flash driver is to have standard code for all boards. If your design does not fit the standard, then provide your own non-standard driver. Don't add complexity to the CFI driver - it has just a single advantage (being "standard"), so please don't drop this.
So, you are saying: it is better to have variations of cfi_flash.c in board directories and maintenance problems because of code forking of cfi_flash.c code is better than having the possibility of board specific read/write access functions. By doing this we not only would save the day and solve his problem but make cfi_flash.c useful to a broader application.
For some reason, my mind works to factor common code. It does not make sense for me but you are the boss.
the cfi_flash.c. The board would also enable FP unit in its board file or within these functions if necessary as well.
"enabling FP" has nothing to do with this. The FPU is never disabled or so. It's just that the compiler is told to never emit any FP instructions :-)
He had to modify MSR before using FP instructions as well. That was what I was pointing to.
Best regards, Tolunay
Tolunay Orkun wrote:
This is probably not acceptable for cfi_flash.c. cfi_flash.c is used by multiple CPU architectures so PowerPC assembly cannot be used. You have to find a solution based on "C" only.
I think the best is to keep cfi_flash.c as is, and to copy it into /board/flash.c and made the changes for custom needs in that file, for the sake of keeping simplicity of generic drivers. But some descriptive warnings and comments may be added at the beggining of cfi_flash for possible non standart designs.
Tolunay Orkun wrote:
How did you use "double" and it did not work? Please give example of the work you tried...
First I had simply tried to cast data to double just before had been writing to the bus.
in cfi_flash.c on line850 (in flash_write_cmd()) - *addr.llp = cword.ll; + *addr.llp = (double) cword.ll;
But it caused a very strange think. While compiling it passed all the make steps (compiled, linked ...) but at the lines to generate srec and bin formatted files
(ppc_82xx-objcopy --gap-fill=0xff -O srec u-boot u-boot.srec, ppc_82xx-objcopy --gap-fill=0xff -O binary u-boot u-boot.bin)
the host machine(the redhat WS 3.0 machine where I made the compilation) is locked. than I exclude these lines from the makefile planning to do the work from command prompth.
When I run the first line, It worked for a few minutes and produced an srec file of about 2 gigabytes length. (It might be bigger but stopped with an out of space error.) (ppc_82xx-objcopy --gap-fill=0xff -O srec u-boot u-boot.srec)
Then running the second line was completely locked the machine. I had had to hard reset it.
I couldn't find a resonable explanation for that circumstance.
Secondly I made some trials; Here is the code;
double doubleVar is defined.
doubleVar = 8245; printf("yio: size of double: %d, doubleVar1=%d:\n", sizeof(double), doubleVar); doubleVar = 1.25; printf("yio: doubleVar2=%f, %d:\n", sizeof(double), doubleVar, doubleVar); memcpy(&doubleVar, &cword.ll, 8); printf("yio: doubleVar3=%f, %d, 0x%08x:\n", sizeof(double), doubleVar, doubleVar, doubleVar);
printf("yio: Before bus write: 0x%08x, 0x%08x, 0x%08x, 0x%08x\n", *addr.dp, cword.d, *addr.llp, cword.ll); *addr.dp = cword.d; printf("yio: After Bus write: 0x%08x, 0x%08x, 0x%08x, 0x%08x\n", *addr.dp, cword.d, *addr.llp, cword.ll);
and here is the console output.
fwrite addr ff040000 cmd 50 0050005000500050 64 bit x 16 bit yio: size of double: 8, doubleVar1=1086331520: yio: doubleVar2=%f, 8: yio: doubleVar3=%f, 8, 0x00500050: yio: Before bus write: 0x0000000a, 0x008000a2, 0x008000a2, 0x00500050 yio: After Bus write: 0x0000000a, 0xffff5678, 0xffff5678, 0x00500050
But I think, as Wolfgang mentioned, the problem is that; I ignored or frankly speaking didn't recognized -msoft-float option given to gcc. since -msoft-float is used, floating point instructions are not compiled as expected by gcc. (an example "%f" in printf is printed onto console as exactly "%f" :-) )
Tolunay Orkun wrote:
You can copy your intended data to the double variable via memcpy() or use a union of "long long" and "double", assign to "long long" member and use "double" one for storing.
Using double in a union is also a kind of casting. So it produced similar results. memcpy copies the data to where the double pointer points. But writing to the bus with double pointer will not change the result, because with -msoft-float option gcc will never use floating point asm instructions which are needed for single 64 bit bus transactions.
Tolunay Orkun wrote:
Please get this crap out of mails sent to public mailing lists. By sending to public lists you cannot make any claim confidentiality... If you have anything truely confidential do not expect others to protect it. Just do not send it to the public lists...
I'm really sorry but have nothing to do with these nonsense words at the end of my e-mails. They are automatically added by our mail monitor program. I have told to our network managers to define some rules not to add these words while sending to the public lists. But they told me that it's a company decision and they can't define such special exceptional rules in the system. And I have also no chance to use any other mail account, because all are prohibited and blocked. :-(
So, unfortunately I have to continue in that way :-( and want you to excuse me. List admins are always free to clip that part, before publishing the mail.
Regards. Yusuf.
###################################################################### Dikkat:
Bu elektronik posta mesaji kisisel ve ozeldir. Eger size gonderilmediyse lutfen gondericiyi bilgilendirip mesaji siliniz. Firmamiza gelen ve giden mesajlar virus taramasindan gecirilmekte, guvenlik nedeni ile kontrol edilerek saklanmaktadir. Mesajdaki gorusler ve bakis acisi gondericiye ait olup Aselsan A.S. resmi gorusu olmak zorunda degildir.
###################################################################### Attention:
This e-mail message is privileged and confidential. If you are not the intended recipient please delete the message and notify the sender. E-mails to and from the company are monitored for operational reasons and in accordance with lawful business practices. Any views or opinions presented are solely those of the author and do not necessarily represent the views of the company.
######################################################################
In message 00a301c57e57$4bf8ac90$0606010a@aselsan.com.tr you wrote:
When I run the first line, It worked for a few minutes and produced an srec file of about 2 gigabytes length. (It might be bigger but stopped with an out of space error.) (ppc_82xx-objcopy --gap-fill=0xff -O srec u-boot u-boot.srec)
The binutils have some serious problems; it is not too difficult to send them into infinite loops crerating infinite output images.
But I think, as Wolfgang mentioned, the problem is that; I ignored or frankly speaking didn't recognized -msoft-float option given to gcc. since -msoft-float is used, floating point instructions are not compiled as expected by gcc. (an example "%f" in printf is printed onto console as exactly "%f" :-) )
Now this has a much simpler reason: we don;t support FP in U-Boot at all, so it is missing completely from the printf() implementation, too. This has nothing to do with the -msoft-float flag being used or not.
Best regards,
Wolfgang Denk
participants (5)
-
Tolunay Orkun -
Tolunay Orkun -
Wolfgang Denk -
Yuli Barcohen -
Yusuf Ibrahim Ozkok