[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH 15/15] ppc: Add aCube Sam460ex board

From: BALATON Zoltan
Subject: Re: [Qemu-ppc] [PATCH 15/15] ppc: Add aCube Sam460ex board
Date: Wed, 23 Aug 2017 14:47:42 +0200 (CEST)
User-agent: Alpine 2.21 (BSF 202 2017-01-01)

On Wed, 23 Aug 2017, François Revol wrote:
Le 23/08/2017 à 13:12, BALATON Zoltan a écrit :
What's the connection with mips_malta?

The board's firmware wants to see SPD EEPROMs of the connected memory
while initialising the memory controller. This is why we need to
implement SDRAM controller, I2C and SPD EEPROMs. MIPS malta board had
already SPD EEPROM implementation so this is based on that. The comment
just indicates where this code comes from.

Indeed, and I copy-pasted from elsewhere for this.

+        fprintf(stderr, "qemu: Error registering flash memory.\n");

Use error_report() instead, please.

I guess this didn't exist back when I started writing it...

No problem, I can take care of these.

+/* Create reset TLB entries for BookE, mapping only the flash
memory.  */
+static void mmubooke_create_initial_mapping_uboot(CPUPPCState *env)
+    ppcemb_tlb_t *tlb = &env->tlb.tlbe[0];
+    /* on reset the flash is mapped by a shadow TLB,
+     * but since we don't implement them we need to use
+     * the same values U-Boot will use to avoid a fault.
+     */

Usually the reset state of the MMU is handled in the cpu code rather
than the board code.  Is there a specific reason you need it in the
board code here?

I'm not sure, probably lack of a better place. The ppc440_bamboo board
this is based on has it the same way in the board code. Maybe this could
be cleaned up when someone wants to QOMify the SoC models sometimes.

Thing is, the code allows both booting with U-Boot and with a kernel
directly, and the MMU mapping differ in those cases.

Maybe the CPU reset should use the U-Boot setup and the kernel boot
would just overwrite it?

+        env->nip = bi->entry;
+        /* Create a mapping for the kernel.  */
+        mmubooke_create_initial_mapping(env, 0, 0);
+        env->gpr[6] = tswap32(EPAPR_MAGIC);

I'm pretty sure the tswap can't be right here.  env->gpr is in host
native order and I'd expect the constant to be as well.

I know nothing about this, maybe Francois remembers why it's there. But
booting linux with -kernel works so it's probably either correct or does
not matter.

Absolutely no idea. It seems to be there from the first commit in my own
history here.

I don't recall testing booting linux at all though.
Linux does check the magic, so it'd be weird if it booted:


Is this code used on Sam460 at all? Is U-Boot ePAPR compliant firmware? Isn't that only needed on OpenFirmware?

But maybe it got added later than the version you tested?

I've tried some of these: http://www.supertuxkart-amiga.de/amiga/sam.html#downloads which also have kernel 4.5 so that's fairly recent. These kernels are "u-boot legacy uImage" so maybe they don't need ePAPR magic? Are there some docs on what the kernel expects on this board or it has to be dug out from U-Boot?

At least my current Haiku port ignores the magic for now.

+        env->gpr[7] = (16 << 20) - 8; /*bi->ima_size;*/

So, entering the kernel directly can be useful, particularly during
early development.  However, having both firmware and non-firmware
entry paths can lead to confusing bugs if there's a subtle difference
between the initial (to the kernel) states between the two paths.  For
that reason, the usual preferred way to implement -kernel is to still
run the usual firmware, but use some way of telling it to boot
immediately into the supplied kernel.

I won't object to merging it this way - just a wanrning that this may
bite you in the future if you're not careful.

Warning taken, at this point until firmware cannot reliably boot things
having another way to test is useful to have. In the future when booting
via firmware works well we can figure out what to do with this.

Possibly we could dig the U-Boot environment...

+    memory_region_init_alias(isa, NULL, "isa_mmio", get_system_io(),
+                             0, 0x10000);
+    memory_region_add_subregion(get_system_memory(), 0xc08000000, isa);

Does it really make sense to just embed the ISA IO space here, rather
than actually instanting a PCI<->ISA bridge?

I'm not sure if this is correct but I don't know how it's handled on
real hardware. The board does not have ISA and I don't think it has a
bridge but the IO space appears at this location according to the
datasheet (In System Memory Address Map it's listed as PCI I/O
0xc08000000-0xc0800ffff) and clients expect PCI card's io registers to
be accessible here. If anyone knows how it's done on real hardware and
if there's a better way to model this please let me know.

Indeed it's the PCI I/O space, maybe it's just copy-paste error.

As for how to declare it, keep in mind this code is years old and I
fixed it several times when compilation broke, without really reading
the documentation (actually, do we have proper documentation for the
internal API?), and it kept changing over the years.

And I've also changed it while cleaning up and getting it to work with some clients. This particular mapping was at the wrong place in your first commits then got removed but I found it's needed but at a different address so I've added it again with the fixed address.

+    /* PCI devices */
+    pci_create_simple(pci_bus, PCI_DEVFN(6, 0), "sm501");
+    /* SoC has a single SATA port but we don't emulate that yet
+     * However, firmware and usual clients have driver for SiI311x
+     * so add one for convenience by default */
+    pci_create_simple(pci_bus, -1, "sii3112");

You should probably not create this when started with -nodefaults.

We don't emulate the on-board SATA port of the SoC and without this
there's no other way to connect disks (maybe over USB, but firmware has
a bug which prevents that too even on real hardware AFAIK, I've
backported a fix which made booting from USB work but that broke
keyboard) so while this is an external card it's pretty much unusable
without this so it's added by default.

Maybe add a /*TODO*/ then?

I don't indent to implement that. Seems to be too complex and unnecessary when we have sii3112 (unless you want to test drivers for it on QEMU). I'd rather see an implementation of the network interfaces that seems to be more useful to spend time on than another SATA controller. So I chose to use the sii3112 by default.
reply via email to

[Prev in Thread] Current Thread [Next in Thread]