On Tue, 30 Apr 2019 at 22:17, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Thu, 18 Apr 2019 at 05:05, Hongbo Zhang <hongbo.zh...@linaro.org> wrote:
> >
> > Following the previous patch, this patch adds peripheral devices to the
> > newly introduced SBSA-ref machine.
> >
> > Signed-off-by: Hongbo Zhang <hongbo.zh...@linaro.org>
> > ---
> > hw/arm/sbsa-ref.c | 451
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 451 insertions(+)
>
> Some fairly minor comments on this one.
>
> > +static void create_flash(const SBSAMachineState *vms,
> > + MemoryRegion *sysmem,
> > + MemoryRegion *secure_sysmem)
> > +{
> > + /*
> > + * Create one secure and nonsecure flash devices to fill SBSA_FLASH
> > + * space in the memmap, file passed via -bios goes in the first one.
> > + */
> > + hwaddr flashsize = vms->memmap[SBSA_FLASH].size / 2;
> > + hwaddr flashbase = vms->memmap[SBSA_FLASH].base;
> > +
> > + create_one_flash("sbsa-ref.flash0", flashbase, flashsize,
> > + bios_name, secure_sysmem);
> > + create_one_flash("sbsa-ref.flash1", flashbase + flashsize, flashsize,
> > + NULL, sysmem);
> > +}
>
> I think Markus might have an opinion on the best way to create
> flash devices on a new board model. Is "just create two flash
> devices the way the virt board does" the right thing?
>
For the firmware part, we are using two flashes, one secure and
another non-secure.
> > +static void create_ahci(const SBSAMachineState *vms, qemu_irq *pic)
> > +{
> > + hwaddr base = vms->memmap[SBSA_AHCI].base;
> > + int irq = vms->irqmap[SBSA_AHCI];
> > + DeviceState *dev;
> > + DriveInfo *hd[NUM_SATA_PORTS];
> > + SysbusAHCIState *sysahci;
> > + AHCIState *ahci;
> > + int i;
> > +
> > + dev = qdev_create(NULL, "sysbus-ahci");
> > + qdev_prop_set_uint32(dev, "num-ports", NUM_SATA_PORTS);
> > + qdev_init_nofail(dev);
> > + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> > + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irq]);
> > +
> > + sysahci = SYSBUS_AHCI(dev);
> > + ahci = &sysahci->ahci;
> > + ide_drive_get(hd, ARRAY_SIZE(hd));
> > + for (i = 0; i < ahci->ports; i++) {
> > + if (hd[i] == NULL) {
> > + continue;
> > + }
> > + ide_create_drive(&ahci->dev[i].port, 0, hd[i]);
> > + }
> > +}
> > +
> > +static void create_ehci(const SBSAMachineState *vms, qemu_irq *pic)
> > +{
> > + hwaddr base = vms->memmap[SBSA_EHCI].base;
> > + int irq = vms->irqmap[SBSA_EHCI];
> > + USBBus *usb_bus;
> > +
> > + sysbus_create_simple("platform-ehci-usb", base, pic[irq]);
> > +
> > + usb_bus = usb_bus_find(-1);
> > + usb_create_simple(usb_bus, "usb-kbd");
> > + usb_create_simple(usb_bus, "usb-mouse");
>
> I don't think we should automatically create the usb keyboard
> and mouse devices. The user can do it on the command line if they
> want them.
>
OK.
> > static void sbsa_ref_init(MachineState *machine)
> > {
> > SBSAMachineState *vms = SBSA_MACHINE(machine);
> > @@ -125,6 +552,7 @@ static void sbsa_ref_init(MachineState *machine)
> > bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> > const CPUArchIdList *possible_cpus;
> > int n, sbsa_max_cpus;
> > + qemu_irq pic[NUM_IRQS];
> >
> > if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a57"))) {
> > error_report("sbsa-ref: CPU type other than the built-in "
> > @@ -209,11 +637,34 @@ static void sbsa_ref_init(MachineState *machine)
> > machine->ram_size);
> > memory_region_add_subregion(sysmem, vms->memmap[SBSA_MEM].base, ram);
> >
> > + create_fdt(vms);
> > +
> > + create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem);
> > +
> > + create_secure_ram(vms, secure_sysmem);
> > +
> > + create_gic(vms, pic);
> > +
> > + create_uart(vms, pic, SBSA_UART, sysmem, serial_hd(0));
> > + create_uart(vms, pic, SBSA_SECURE_UART, secure_sysmem, serial_hd(1));
> > + create_uart(vms, pic, SBSA_SECURE_UART_MM, secure_sysmem,
> > serial_hd(2));
>
> What's the third UART for (ie what is the name intended to mean)?
> Should we have more than one non-secure UART?
>
Yes, this is called " Standalone Management Mode", I will add comment
for it, this is needed by server RAS feature.
> thanks
> -- PMM
