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.
