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

[Schwarz, Konrad]

> -----Original Message-----
> From: Paul Brook [mailto:address@hidden
> 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.

My target operating system is not Linux, see below.  Note that the
Altera Excalibur device is actually just a chip, not a whole board;
Altera wedded an ARM core (hard macro) to its FPGA technology in this
product.  (The code of course doesn't emulate the FPGA, however, it is
straightforward to emulate peripherals that have been implemented in the
FPGA within the QEMU framework.)

But I think the QEMU "machine" abstraction and the split between
X_init() and X_device_init() still works well in this case: all I need
to do is to remove the "static" storage class specifier of the routine
altera_excalibur_device_init() to make the device available as the heart
of a different board.

> 
> 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.

I am using an operating system that implements the "OSEK/VDX" OS
standard (http://www.osek-vdx.org).  At some point, I may be able to
open source my implementation.  Our use case is simulation of
(relatively) small electronic control units for the automotive industry.

I understand your concerns about bitrot, but see no perfect answer.  I
could provide you with bootable images that use OSEK OS.  Come to think
of it,  I *do* have a Linux 2.4 kernel that runs on the hardware that
this patch is emulating, that should work, but I haven't tested it.
(Also, the U-Boot loader from Denx http://www.denx.de runs on the real
hardware).  Would that be acceptable to you?  However, that would
probably be at odds with the loading strategy I've used.

I would prefer to keep things the way they are; perhaps I could provide
you with a TCL or DejaGNU regression test that utilizes OSEK (you name
it).  If someone else picks up the ball and wants Linux on this machine,
we could find a better answer then---currently, I see no way to find
time to port Linux to this machine.  

> 
> 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?

Excalibur Devices Hardware Reference Manual, v3.1, November 2002, pg 100
Bit 0 (LSb) is the EN bit:

"EN     R/W     Enable.  Setting this bit enables decoding of this
memory range."

If you suggested "3 & ..." as an alignedness check of the address, I
have to admit to not knowing off hand what exactly the HW does in this
case.

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

The reason for this is because I was not able to find a prototype in
QEMU 0.8.0 cpu-all.h that allows one to change or to unregister physical
memory.  

> 
> - 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.

Do you mean CP15 register 0 for reporting the ID or what?

> 
> - 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.
>

The board variants are actually device variants.  There are 1 and a half
locations where the device variants actually come into play, but as you
suggest, a symbolic name would be better.
 
> - There are several chunks of code surrounded by #if 0 for no apparent

> reason.

The reason was documentation, some registers are read-only and I wanted
to make sure that I hadn't forgotten any.  The way it is now allows one
to find each register quickly, in both the read and write paths and see
that a conscious decision regarding its use was made.  I can present and
document this better.

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

Structure members that model hardware registers use the exact spelling
of the defining Altera Excalibur manual.  I would like to keep that.  I
could prefix each such structure member with a lower case prefix, to
avoid name clashes with macros, if you like.

> 
> - 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.

This is part of my "improvement plan for the universe" :-).

> I'm guessing the -O0 and -fno-omit-frame-pointer are for debugging, so

> should be removed before submission.

I haven't tested without those flags yet.

> Paul

Since other things have higher priority for me now, would it be ok if I
resubmitted in a few weeks time?

Regards,

Konrad