[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Patch that adds machine "Altera Excalibur"
From: |
Paul Brook |
Subject: |
Re: [Qemu-devel] Patch that adds machine "Altera Excalibur" |
Date: |
Sun, 16 Apr 2006 17:24:56 +0100 |
User-agent: |
KMail/1.9.1 |
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