On Mon, 2013-03-04 at 14:12 -0700, Jack Thomasson wrote: > On Sunday, 03 Mar 2013 11:33:42 Jeremy Bennett wrote: > > 1. Could you update the documentation (in the doc folder) to match. > > hmmm, i thought the changes i submitted for doc/or1ksim.texi were sufficient. > what more do you want updated?
Hi Jack, Apologies - I managed to miss a part of the patch when reviewing it. These changes are fine, but could you add a small note about how endianness is dealt with (along the lines you give below) - I can see this being a possible cause of confusion. What is missing is the ChangeLog entry. Could you add this. > > 2. Could you supply a git patch if at all possible. The main repo is at > > https://github.com/openrisc/or1ksim. > > yep, can do. i'll wait on the answer regarding the docs before submitting. Docs will be fine with a note about endianness - look forward to the patch. > > One day the problem will go away, when we can drop the or32 branch. > > so maybe i'll submit against just or1k to accelerate the process a tiny bit. I'm not sure it accelerates things-:( But I'll don the merge into or32. We're glad to have the patch in any form. > > I have some questions/commentary on your code. You don't need to do > > anything about these - it's just to help my understanding. > > > > 1. peripheral/memory.c: I'm curious about the use of "seed" for the file > > handle name. Personally I'd have defined a variable "fh" (or similar) to > > avoid confusion. > > i thought about adding another variable as well but realized the file > is seeding the memory so it works. > > > 2. peripheral/memory.c: I'm not sure worrying about endianness is a good > > thing here. I would be inclined to say that the file is just a byte > > image, and it is up to the user to ensure the sequence of bytes matches > > the endianness of the machine. It will mean the file can always be used > > for other tools as a true image of memory for the particular machine. It > > is trivial for a user to have an external utility to swap byte order to > > create a new file if that is what they need. > > if i seed memory with a file containing "AbCd" and use a simple > program like the attached dumpROM.c, i expect to get "AbCd". on my > x86_64-unknown-linux-gnu host without swabbing i get "dCbA". that will > be confusing for users not familiar with endianness. we could send > them haring off through the user manual and the file could be > converted offline but it seems kinder to the user to do it for them. > > > 3. Is a straight binary file what you want? Had you considered > > supporting the image formats used by Verilog $readmemh and $readmemb. > > Maybe these should be further memory options, "readmemh" and "readmemb" > > for the future. > > well, to an old Un*x hacker, _ALL_ files are binary :{)}. my source > data is a pcap file so definitely "straight binary". > > i am not that familiar with Verilog, barely learning VHDL right now to > tell the truth, but i did look up these with Google and they are > definitely not what i want. i agree they deserve to be implemented but > i expect the endianness problem to show up here as well so better to > have a correct implementation already coded in memory.c. > > :{)} Thanks for all your work. I look forward to merging in the patch very shortly. Best wishes, Jeremy -- Tel: +44 (1590) 610184 Cell: +44 (7970) 676050 SkypeID: jeremybennett Email: [email protected] Web: www.embecosm.com _______________________________________________ OpenRISC mailing list [email protected] http://lists.openrisc.net/listinfo/openrisc
