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

[François Revol]

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

>>> +/* 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:

https://github.com/torvalds/linux/blob/master/arch/powerpc/boot/epapr.c

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

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.

> 
>>> +    /* 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?

François.