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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9101 bytes --]

On Wed, Aug 23, 2017 at 01:12:06PM +0200, BALATON Zoltan wrote:
> On Wed, 23 Aug 2017, David Gibson wrote:
> > On Sun, Aug 20, 2017 at 07:23:05PM +0200, BALATON Zoltan wrote:
> > > Add emulation of aCube Sam460ex board based on AMCC 460EX embedded SoC.
> > > This is not a full implementation yet with a lot of components still
> > > missing but enough to start a Linux kernel and the U-Boot firmware.
> > > 
> > > Signed-off-by: François Revol <revol@free.fr>
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > 
> > As usual, only fairly superficial review here.
> > 
> > > ---
> > >  default-configs/ppcemb-softmmu.mak |   3 +
> > >  hw/ppc/Makefile.objs               |   2 +
> > >  hw/ppc/sam460ex.c                  | 611 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 616 insertions(+)
> > >  create mode 100644 hw/ppc/sam460ex.c
> > > 
> > > diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> > > index 635923a..90b42f0 100644
> > > --- a/default-configs/ppcemb-softmmu.mak
> > > +++ b/default-configs/ppcemb-softmmu.mak
> [...]
> > > +/*****************************************************************************/
> > > +/* SPD eeprom content from mips_malta.c */
> > 
> > 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.

Ok.

[snip]
> > > +        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.

Have you attempted it on both BE and LE hosts though?

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

Fair enough.

[snip]
> > > +        generate_eeprom_spd(&smbus_eeprom_buf[i * 256], ram_sizes[i]);
> > > +    }
> > > +    generate_eeprom_serial(&smbus_eeprom_buf[4 * 256]);
> > > +    generate_eeprom_serial(&smbus_eeprom_buf[6 * 256]);
> > > +
> > > +    /* IIC controllers */
> > > +    dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]);
> > > +    i2c[0] = PPC4xx_I2C(dev);
> > > +    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
> > > +    smbus_eeprom_init(i2c[0]->bus, 8, smbus_eeprom_buf, smbus_eeprom_size);
> > > +    g_free(smbus_eeprom_buf);
> > > +
> > > +    dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600800, uic[0][3]);
> > > +    i2c[1] = PPC4xx_I2C(dev);
> > > +
> > > +    /* External bus controller */
> > > +    ppc405_ebc_init(env);
> > > +
> > > +    /* CPR */
> > > +    ppc4xx_cpr_init(env);
> > > +
> > > +    /* PLB to AHB bridge */
> > > +    ppc4xx_ahb_init(env);
> > > +
> > > +    /* System DCRs */
> > > +    ppc4xx_sdr_init(env);
> > > +
> > > +    /* MAL */
> > > +    ppc4xx_mal_init(env, 4, 16, &uic[2][3]);
> > > +
> > > +    /* 256K of L2 cache as memory */
> > > +    ppc4xx_l2sram_init(env);
> > 
> > Seems like a lot of this peripheral setup should be handled by a SoC
> > helper function, since it's not really board specific (I'm guessing).
> > I'm ok with that being a later clean up though.
> 
> I know, the ppc405 has an init function and I considered moving some of this
> to a similar function but I've left it here for now for simplicity until
> it's decided what's the best way to clean this up.

Ok, seems reasonable.

> > > +    /* FIXME: remove this after fixing l2sram mapping in ppc440_uc.c? */
> > > +    memory_region_init_ram(l2cache_ram, NULL, "ppc440.l2cache_ram", 256 << 10,
> > > +                           &error_abort);
> > > +    memory_region_add_subregion(address_space_mem, 0x400000000LL, l2cache_ram);
> > > +
> > > +    /* USB */
> > > +    sysbus_create_simple(TYPE_PPC4xx_EHCI, 0x4bffd0400, uic[2][29]);
> > > +    dev = qdev_create(NULL, "sysbus-ohci");
> > > +    qdev_prop_set_string(dev, "masterbus", "usb-bus.0");
> > > +    qdev_prop_set_uint32(dev, "num-ports", 6);
> > > +    qdev_init_nofail(dev);
> > > +    sbdev = SYS_BUS_DEVICE(dev);
> > > +    sysbus_mmio_map(sbdev, 0, 0x4bffd0000);
> > > +    sysbus_connect_irq(sbdev, 0, uic[2][30]);
> > > +    usb_create_simple(usb_bus_find(-1), "usb-kbd");
> > > +    usb_create_simple(usb_bus_find(-1), "usb-mouse");
> > > +
> > > +    /* PCI bus */
> > > +    ppc460ex_pcie_init(env);
> > > +    /*XXX: FIXME: is this correct? */
> > > +    dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
> > > +                                uic[1][0], uic[1][20], uic[1][21], uic[1][22],
> > > +                                NULL);
> > > +    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
> > > +    if (!pci_bus) {
> > > +        fprintf(stderr, "couldn't create PCI controller!\n");
> > > +        exit(1);
> > > +    }
> > > +    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.

Ah, ok.  I think the confusion here is that you can have PCI I/O space
without ISA or a system IO space.  In fact that's pretty standard on
things without a CPU level IO space (which means just about everything
except x86).

But in that case I'd expect the PCI host bridge to map its IO memory
regions directly into address_space_memory rather than involving the
global address_space_io (what get_system_io()) returns.  The only
reason I can see that you'd need to involve get_system_io() is if you
have devices registering themselves directly into the global IO space,
which should only happen for legacy ISA devices, which it sounds like
you don't have.

Possibly this is an error in the PCI bridge implementation that your
code here is essentially a workaround for, though.

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

Yes, but you're not just adding it by default, you're adding it
*always*.  -nodefaults should turn that off (and the user will need to
manually instantiate it - or another disk controller, I guess).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]