----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/165/#review177 -----------------------------------------------------------
src/arch/arm/faults.cc <http://reviews.m5sim.org/r/165/#comment314> Rather than putting pc in a temp variable for the DPRINTF and having to protect it like this, it would probably be simpler and cleaner to just read it in place in the one place it's passed as an argument. src/arch/arm/faults.cc <http://reviews.m5sim.org/r/165/#comment313> This line is too long. src/arch/arm/linux/atag.hh <http://reviews.m5sim.org/r/165/#comment316> It looks like each 32 bit field has a defined purpose (tag, flags, etc.) I'd be warmer and fuzzier if this was a structure with named elements instead of a uint32_t array. Perhaps it could be called AtagHeaderStruct or something like that, and they'd inherit from each other to build on a common base structure on up. Also, if the name AtagHeader, etc., weren't chosen to match names in Linux (I suspect they were, so this doesn't matter) then they could stand to be a little more descriptive. It's not clear at all what that means. I'm guessing address tag header, but I really can't tell. A brief comment explaining what these objects are used for would help. src/arch/arm/linux/atag.hh <http://reviews.m5sim.org/r/165/#comment317> It seems a little weird to put the initializer on a new line but the empty function body at the end of the line. If this is mandated by the style guide then fine, but it seems a little wonky. src/arch/arm/linux/atag.hh <http://reviews.m5sim.org/r/165/#comment318> What does this hardcoded constant mean? Why isn't it read from anything, or at least verified to be the same as that something even if a particular value is expected? This same comment applies later on as well. src/arch/arm/linux/atag.hh <http://reviews.m5sim.org/r/165/#comment319> Ah, so atag is a bootloader thing. This comment isn't particularly helpful, but I'll leave judgment of its value to you. In any case, explanatory comments like this would be better at the top, where you see them and at least know they're there before being baffled by the rest :-) src/arch/arm/system.cc <http://reviews.m5sim.org/r/165/#comment315> I like this fix. This is much better. - Gabe On 2010-08-13 09:44:46, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/165/ > ----------------------------------------------------------- > > (Updated 2010-08-13 09:44:46) > > > Review request for Default. > > > Summary > ------- > > ARM: Add system for ARM/Linux and bootstrapping > > > Diffs > ----- > > src/arch/arm/ArmSystem.py 3c48b2b3cb83 > src/arch/arm/SConscript 3c48b2b3cb83 > src/arch/arm/faults.cc 3c48b2b3cb83 > src/arch/arm/linux/atag.hh PRE-CREATION > src/arch/arm/linux/system.hh PRE-CREATION > src/arch/arm/linux/system.cc PRE-CREATION > src/arch/arm/system.cc 3c48b2b3cb83 > > Diff: http://reviews.m5sim.org/r/165/diff > > > Testing > ------- > > > Thanks, > > Ali > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
