Re: [Qemu-devel] Patch that adds machine "Altera Excalibur"

[Paul Brook <paul@codesourcery.com>]

On Monday 10 April 2006 12:26, Schwarz, Konrad wrote:
> Hello,
> this patch adds support for the Altera Excalibur device (an FPGA that
> supports the ARM922T core).

Are there any plans to update the linux kernel support for this board?
It doesn't seem to be supported in linux 2.6.

What are you using the emulation for? I'm worried that if there's no way for 
other people (eg. Me and Fabrice) to test the code then it's just going to 
bitrot.

A few points on the patch itself. Some of them are cosmetic, but I'd still 
like to get them resolved before applying the patch.

+static uint32_t
+altera_excalibur_read32 (void *const opaque, target_phys_addr_t const offset)
+{
+       if (!(1 & e->MMAP_REGISTERS))

Shouldn't this be &3?

- Changing the memory map registers appears to be only half-implemented.

- Qemu can now support different ARM CPU cores relatively easily, , so you 
should be able to get it to report the correct ID. Enforcing v4t only is 
harder, but less important.

- Please use 4 spaces for code indent, not tabs.

- This is just ugly:
+# define       e       ((struct altera_excalibur_state *) opaque)
Use a local variable like the existing code.

- There appears to be support for different board variants, scattered in 
several different places and #if 0'ed out. This should at least be controlled 
by a single #define, preferably a runtime option.

- There are several chunks of code surrounded by #if 0 for no apparent reason.

- Token names in ALL_CAPS should only be used for preprocessor macros and 
constants, not field names.

- Mangling CFLAGS for one object file is not acceptable.
+altera-excalibur.o: CFLAGS += -Wno-parentheses -O0 -fno-omit-frame-pointer
The warnings produced by -Wparentheses should IMHO be fixed, not ignored. Some 
of the C operator precedence rules are non-obvious so it's best to be 
explicit.
I'm guessing the -O0 and -fno-omit-frame-pointer are for debugging, so should 
be removed before submission.

Paul