diff mbox series

[v4,05/12] hw/arm: Add NPCM730 and NPCM750 SoC models

Message ID 20200707184730.3047754-6-hskinnemoen@google.com (mailing list archive)
State New, archived
Headers show
Series Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines | expand

Commit Message

Havard Skinnemoen July 7, 2020, 6:47 p.m. UTC
The Nuvoton NPCM7xx SoC family are used to implement Baseboard
Management Controllers in servers. While the family includes four SoCs,
this patch implements limited support for two of them: NPCM730 (targeted
for Data Center applications) and NPCM750 (targeted for Enterprise
applications).

This patch includes little more than the bare minimum needed to boot a
Linux kernel built with NPCM7xx support in direct-kernel mode:

  - Two Cortex-A9 CPU cores with built-in periperhals.
  - Global Configuration Registers.
  - Clock Management.
  - 3 Timer Modules with 5 timers each.
  - 4 serial ports.

The chips themselves have a lot more features, some of which will be
added to the model at a later stage.

Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
---
 hw/arm/Makefile.objs     |   1 +
 hw/arm/npcm7xx.c         | 328 +++++++++++++++++++++++++++++++++++++++
 include/hw/arm/npcm7xx.h |  81 ++++++++++
 3 files changed, 410 insertions(+)
 create mode 100644 hw/arm/npcm7xx.c
 create mode 100644 include/hw/arm/npcm7xx.h

Comments

Philippe Mathieu-Daudé July 8, 2020, 5:31 p.m. UTC | #1
Hi Havard,

On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> The Nuvoton NPCM7xx SoC family are used to implement Baseboard
> Management Controllers in servers. While the family includes four SoCs,
> this patch implements limited support for two of them: NPCM730 (targeted
> for Data Center applications) and NPCM750 (targeted for Enterprise
> applications).
> 
> This patch includes little more than the bare minimum needed to boot a
> Linux kernel built with NPCM7xx support in direct-kernel mode:
> 
>   - Two Cortex-A9 CPU cores with built-in periperhals.
>   - Global Configuration Registers.
>   - Clock Management.
>   - 3 Timer Modules with 5 timers each.
>   - 4 serial ports.
> 
> The chips themselves have a lot more features, some of which will be
> added to the model at a later stage.
> 
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>  hw/arm/Makefile.objs     |   1 +
>  hw/arm/npcm7xx.c         | 328 +++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/npcm7xx.h |  81 ++++++++++

Please have a look at the scripts/git.orderfile, using it would
ease our reviews.

>  3 files changed, 410 insertions(+)
>  create mode 100644 hw/arm/npcm7xx.c
>  create mode 100644 include/hw/arm/npcm7xx.h
> 
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 534a6a119e..13d163a599 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -41,6 +41,7 @@ obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
>  obj-$(CONFIG_STM32F405_SOC) += stm32f405_soc.o
>  obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o
>  obj-$(CONFIG_XLNX_VERSAL) += xlnx-versal.o xlnx-versal-virt.o
> +obj-$(CONFIG_NPCM7XX) += npcm7xx.o
>  obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
>  obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o
> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> new file mode 100644
> index 0000000000..0a9e30f66f
> --- /dev/null
> +++ b/hw/arm/npcm7xx.c
> @@ -0,0 +1,328 @@
> +/*
> + * Nuvoton NPCM7xx SoC family.
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "exec/address-spaces.h"
> +#include "hw/arm/npcm7xx.h"
> +#include "hw/char/serial.h"
> +#include "hw/loader.h"
> +#include "hw/misc/unimp.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/error.h"
> +#include "qemu/units.h"
> +#include "sysemu/sysemu.h"
> +
> +/* The first half of the address space is reserved for DDR4 DRAM. */
> +#define NPCM7XX_DRAM_BA         (0x00000000)
> +#define NPCM7XX_DRAM_SZ         (2 * GiB)
> +
> +/*
> + * This covers the whole MMIO space. We'll use this to catch any MMIO accesses
> + * that aren't handled by any device.
> + */
> +#define NPCM7XX_MMIO_BA         (0x80000000)
> +#define NPCM7XX_MMIO_SZ         (0x7FFD0000)
> +
> +/* Core system modules. */
> +#define NPCM7XX_L2C_BA          (0xF03FC000)
> +#define NPCM7XX_CPUP_BA         (0xF03FE000)
> +#define NPCM7XX_GCR_BA          (0xF0800000)
> +#define NPCM7XX_CLK_BA          (0xF0801000)
> +
> +/* Memory blocks at the end of the address space */
> +#define NPCM7XX_RAM2_BA         (0xFFFD0000)
> +#define NPCM7XX_RAM2_SZ         (128 * KiB)
> +#define NPCM7XX_ROM_BA          (0xFFFF0000)
> +#define NPCM7XX_ROM_SZ          (64 * KiB)
> +
> +/*
> + * Interrupt lines going into the GIC. This does not include internal Cortex-A9
> + * interrupts.
> + */
> +enum NPCM7xxInterrupt {
> +    NPCM7XX_UART0_IRQ           = 2,
> +    NPCM7XX_UART1_IRQ,
> +    NPCM7XX_UART2_IRQ,
> +    NPCM7XX_UART3_IRQ,
> +    NPCM7XX_TIMER0_IRQ          = 32,   /* Timer Module 0 */
> +    NPCM7XX_TIMER1_IRQ,
> +    NPCM7XX_TIMER2_IRQ,
> +    NPCM7XX_TIMER3_IRQ,
> +    NPCM7XX_TIMER4_IRQ,
> +    NPCM7XX_TIMER5_IRQ,                 /* Timer Module 1 */
> +    NPCM7XX_TIMER6_IRQ,
> +    NPCM7XX_TIMER7_IRQ,
> +    NPCM7XX_TIMER8_IRQ,
> +    NPCM7XX_TIMER9_IRQ,
> +    NPCM7XX_TIMER10_IRQ,                /* Timer Module 2 */
> +    NPCM7XX_TIMER11_IRQ,
> +    NPCM7XX_TIMER12_IRQ,
> +    NPCM7XX_TIMER13_IRQ,
> +    NPCM7XX_TIMER14_IRQ,
> +};
> +
> +/* Total number of GIC interrupts, including internal Cortex-A9 interrupts. */
> +#define NPCM7XX_NUM_IRQ         (160)
> +
> +/* Register base address for each Timer Module */
> +static const hwaddr npcm7xx_tim_addr[] = {
> +    0xF0008000,
> +    0xF0009000,
> +    0xF000A000,

Here caps hex ...

> +};
> +
> +/* Register base address for each 16550 UART */
> +static const hwaddr npcm7xx_uart_addr[] = {
> +    0xF0001000,
> +    0xF0002000,
> +    0xF0003000,
> +    0xF0004000,
> +};
> +
> +void npcm7xx_write_secondary_boot(ARMCPU *cpu, const struct arm_boot_info *info)
> +{
> +    /*
> +     * The default smpboot stub halts the secondary CPU with a 'wfi'
> +     * instruction, but the arch/arm/mach-npcm/platsmp.c in the Linux kernel
> +     * does not send an IPI to wake it up, so the second CPU fails to boot. So
> +     * we need to provide our own smpboot stub that can not use 'wfi', it has
> +     * to spin the secondary CPU until the first CPU writes to the SCRPAD reg.
> +     */
> +    uint32_t smpboot[] = {
> +        0xe59f2018,     /* ldr r2, bootreg_addr */
> +        0xe3a00000,     /* mov r0, #0 */
> +        0xe5820000,     /* str r0, [r2] */
> +        0xe320f002,     /* wfe */
> +        0xe5921000,     /* ldr r1, [r2] */
> +        0xe1110001,     /* tst r1, r1 */
> +        0x0afffffb,     /* beq <wfe> */
> +        0xe12fff11,     /* bx r1 */

... here small hex. Pick one style?

> +        NPCM7XX_SMP_BOOTREG_ADDR,
> +    };
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(smpboot); i++) {
> +        smpboot[i] = tswap32(smpboot[i]);
> +    }
> +
> +    rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
> +                       NPCM7XX_SMP_LOADER_START);
> +}
> +
> +static qemu_irq npcm7xx_irq(NPCM7xxState *s, int n)
> +{
> +    return qdev_get_gpio_in(DEVICE(&s->a9mpcore), n);
> +}
> +
> +static void npcm7xx_init(Object *obj)
> +{
> +    NPCM7xxState *s = NPCM7XX(obj);
> +    int i;
> +
> +    for (i = 0; i < NPCM7XX_MAX_NUM_CPUS; i++) {
> +        object_initialize_child(obj, "cpu[*]", &s->cpu[i],
> +                                ARM_CPU_TYPE_NAME("cortex-a9"));
> +    }
> +
> +    object_initialize_child(obj, "a9mpcore", &s->a9mpcore, TYPE_A9MPCORE_PRIV);
> +    object_initialize_child(obj, "gcr", &s->gcr, TYPE_NPCM7XX_GCR);
> +    object_property_add_alias(obj, "power-on-straps", OBJECT(&s->gcr),
> +                              "power-on-straps");
> +    object_initialize_child(obj, "clk", &s->clk, TYPE_NPCM7XX_CLK);
> +
> +    for (i = 0; i < ARRAY_SIZE(s->tim); i++) {
> +        object_initialize_child(obj, "tim[*]", &s->tim[i], TYPE_NPCM7XX_TIMER);
> +    }
> +}
> +
> +static void npcm7xx_realize(DeviceState *dev, Error **errp)
> +{
> +    NPCM7xxState *s = NPCM7XX(dev);
> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
> +    Error *err = NULL;
> +    int i;
> +
> +    /* I/O space -- unimplemented unless overridden below. */
> +    create_unimplemented_device("npcm7xx.io", NPCM7XX_MMIO_BA, NPCM7XX_MMIO_SZ);

I still insist this is not the best, but as "The data sheet for these
SoCs is not generally available" there is not much I can suggest to
improve.

> +
> +    /* CPUs */
> +    for (i = 0; i < nc->num_cpus; i++) {
> +        object_property_set_int(OBJECT(&s->cpu[i]),
> +                                arm_cpu_mp_affinity(i, NPCM7XX_MAX_NUM_CPUS),
> +                                "mp-affinity", &error_abort);
> +        object_property_set_int(OBJECT(&s->cpu[i]), NPCM7XX_GIC_CPU_IF_ADDR,
> +                                "reset-cbar", &error_abort);
> +        object_property_set_bool(OBJECT(&s->cpu[i]), true,
> +                                 "reset-hivecs", &error_abort);
> +
> +        /* Disable security extensions. */
> +        if (object_property_find(OBJECT(&s->cpu[i]), "has_el3", NULL)) {

You know these are cortex-a9 right? Why bother checking?

> +            object_property_set_bool(OBJECT(&s->cpu[i]), false, "has_el3",
> +                                     &error_abort);
> +        }
> +
> +        qdev_realize(DEVICE(&s->cpu[i]), NULL, &err);
> +        if (err) {
> +            error_propagate(errp, err);

Since you don't plan to create a SoC at runtime and hot-plug it, I'd
simplify by using &error_abort everywhere in this function, and forget
about propagating.

> +            return;
> +        }
> +    }
> +
> +    /* A9MPCORE peripherals */
> +    object_property_set_int(OBJECT(&s->a9mpcore), nc->num_cpus, "num-cpu",
> +                            &error_abort);

See, you already use &error_abort ;)

> +    object_property_set_int(OBJECT(&s->a9mpcore), NPCM7XX_NUM_IRQ, "num-irq",
> +                            &error_abort);
> +    sysbus_realize(SYS_BUS_DEVICE(&s->a9mpcore), &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->a9mpcore), 0, NPCM7XX_CPUP_BA);
> +
> +    for (i = 0; i < nc->num_cpus; i++) {
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->a9mpcore), i,
> +                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->a9mpcore), i + nc->num_cpus,
> +                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_FIQ));
> +    }
> +
> +    /* L2 cache controller */
> +    sysbus_create_simple("l2x0", NPCM7XX_L2C_BA, NULL);
> +
> +    /* System Global Control Registers (GCR) */
> +    object_property_set_int(OBJECT(&s->gcr), nc->disabled_modules,
> +                            "disabled-modules", &error_abort);
> +    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
> +                             &error_abort);

I guess you can simplify using in npcm7xx_init():

      object_property_add_const_link(obj, "dram-mr", OBJECT(&s->gcr));

And in npcm7xx_gcr_realize()

    obj = object_property_get_link(OBJECT(dev), "dram-mr", &err);
    if (obj == NULL) {
        error_setg(errp, "%s: required dram-mr link not found: %s",
                   __func__, error_get_pretty(err));
        return;
    }
    s->dram = MEMORY_REGION(obj);

> +    sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gcr), 0, NPCM7XX_GCR_BA);
> +
> +    /* Clock Control Registers (CLK) */
> +    sysbus_realize(SYS_BUS_DEVICE(&s->clk), &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->clk), 0, NPCM7XX_CLK_BA);
> +
> +    /* Timer Modules (TIM) */
> +    QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_tim_addr) != ARRAY_SIZE(s->tim));
> +    for (i = 0; i < ARRAY_SIZE(s->tim); i++) {
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->tim[i]);
> +        int first_irq;
> +        int j;
> +
> +        sysbus_realize(sbd, &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        sysbus_mmio_map(sbd, 0, npcm7xx_tim_addr[i]);
> +
> +        first_irq = NPCM7XX_TIMER0_IRQ + i * NPCM7XX_TIMERS_PER_CTRL;
> +        for (j = 0; j < NPCM7XX_TIMERS_PER_CTRL; j++) {
> +            qemu_irq irq = npcm7xx_irq(s, first_irq + j);
> +            sysbus_connect_irq(sbd, j, irq);
> +        }
> +    }
> +
> +    /* UART0..3 (16550 compatible) */
> +    for (i = 0; i < ARRAY_SIZE(npcm7xx_uart_addr); i++) {
> +        serial_mm_init(get_system_memory(), npcm7xx_uart_addr[i], 2,
> +                       npcm7xx_irq(s, NPCM7XX_UART0_IRQ + i), 115200,
> +                       serial_hd(i), DEVICE_LITTLE_ENDIAN);
> +    }
> +
> +    /* RAM2 (SRAM) */
> +    memory_region_init_ram(&s->sram, OBJECT(dev), "ram2",
> +                           NPCM7XX_RAM2_SZ, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    memory_region_add_subregion(get_system_memory(), NPCM7XX_RAM2_BA, &s->sram);
> +
> +    /* Internal ROM */
> +    memory_region_init_rom(&s->irom, OBJECT(dev), "irom", NPCM7XX_ROM_SZ, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    memory_region_add_subregion(get_system_memory(), NPCM7XX_ROM_BA, &s->irom);
> +
> +    /* External DDR4 SDRAM */
> +    memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, s->dram);

This doesn't look correct, the DRAM is an external component, not
embedded within the SoC. See the manual ;)
https://www.nuvoton.com/products/cloud-computing/ibmc/

You should call that in npcm7xx_create_soc() (and eventually rename it)
or add npcm7xx_create_dram(), maybe cleaner (and call it in each
mc->init).

> +}
> +
> +static Property npcm7xx_properties[] = {
> +    DEFINE_PROP_LINK("dram", NPCM7xxState, dram, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void npcm7xx_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = npcm7xx_realize;
> +    dc->user_creatable = false;

This confirms you can safely use &error_abort in realize().

> +    device_class_set_props(dc, npcm7xx_properties);
> +}
> +
> +static void npcm730_class_init(ObjectClass *oc, void *data)
> +{
> +    NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
> +
> +    /* NPCM730 is optimized for data center use, so no graphics, etc. */
> +    nc->disabled_modules = 0x00300395;
> +    nc->num_cpus = 2;
> +}
> +
> +static void npcm750_class_init(ObjectClass *oc, void *data)
> +{
> +    NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
> +
> +    /* NPCM750 has 2 cores and a full set of peripherals */
> +    nc->disabled_modules = 0x00000000;
> +    nc->num_cpus = 2;
> +}
> +
> +static const TypeInfo npcm7xx_soc_types[] = {
> +    {
> +        .name           = TYPE_NPCM7XX,
> +        .parent         = TYPE_DEVICE,
> +        .instance_size  = sizeof(NPCM7xxState),
> +        .instance_init  = npcm7xx_init,
> +        .class_size     = sizeof(NPCM7xxClass),
> +        .class_init     = npcm7xx_class_init,
> +        .abstract       = true,
> +    }, {
> +        .name           = TYPE_NPCM730,
> +        .parent         = TYPE_NPCM7XX,
> +        .class_init     = npcm730_class_init,
> +    }, {
> +        .name           = TYPE_NPCM750,
> +        .parent         = TYPE_NPCM7XX,
> +        .class_init     = npcm750_class_init,
> +    },
> +};
> +
> +DEFINE_TYPES(npcm7xx_soc_types);
> diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
> new file mode 100644
> index 0000000000..2ffa573b11
> --- /dev/null
> +++ b/include/hw/arm/npcm7xx.h
> @@ -0,0 +1,81 @@
> +/*
> + * Nuvoton NPCM7xx SoC family.
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +#ifndef NPCM7XX_H
> +#define NPCM7XX_H
> +
> +#include "hw/boards.h"
> +#include "hw/cpu/a9mpcore.h"
> +#include "hw/misc/npcm7xx_clk.h"
> +#include "hw/misc/npcm7xx_gcr.h"
> +#include "hw/timer/npcm7xx_timer.h"
> +#include "target/arm/cpu.h"
> +
> +#define NPCM7XX_MAX_NUM_CPUS    (2)
> +
> +/* Magic addresses for setting up direct kernel booting and SMP boot stubs. */
> +#define NPCM7XX_LOADER_START            (0x00000000)  /* Start of SDRAM */
> +#define NPCM7XX_SMP_LOADER_START        (0xFFFF0000)  /* Boot ROM */
> +#define NPCM7XX_SMP_BOOTREG_ADDR        (0xF080013C)  /* GCR.SCRPAD */
> +#define NPCM7XX_GIC_CPU_IF_ADDR         (0xF03FE100)  /* GIC within A9 */
> +
> +typedef struct NPCM7xxState {
> +    DeviceState         parent;
> +
> +    ARMCPU              cpu[NPCM7XX_MAX_NUM_CPUS];
> +    A9MPPrivState       a9mpcore;
> +
> +    MemoryRegion        sram;
> +    MemoryRegion        irom;
> +    MemoryRegion        *dram;
> +
> +    NPCM7xxGCRState     gcr;
> +    NPCM7xxCLKState     clk;
> +    NPCM7xxTimerCtrlState tim[3];
> +} NPCM7xxState;
> +
> +#define TYPE_NPCM7XX    "npcm7xx"
> +#define NPCM7XX(obj)    OBJECT_CHECK(NPCM7xxState, (obj), TYPE_NPCM7XX)
> +
> +#define TYPE_NPCM730    "npcm730"
> +#define TYPE_NPCM750    "npcm750"
> +
> +typedef struct NPCM7xxClass {
> +    DeviceClass         parent;

Similar comment that elsewhere on this series, if NPCM7xxClass not used
outside of npcm7xx.c, keep it local.

Very good patch, I wanted to R-b it but the DRAM part makes me think
it might be worth a v5...

Regards,

Phil.

> +
> +    /* Bitmask of modules that are permanently disabled on this chip. */
> +    uint32_t            disabled_modules;
> +    /* Number of CPU cores enabled in this SoC class (may be 1 or 2). */
> +    uint32_t            num_cpus;
> +} NPCM7xxClass;
> +
> +#define NPCM7XX_CLASS(klass)                                            \
> +    OBJECT_CLASS_CHECK(NPCM7xxClass, (klass), TYPE_NPCM7XX)
> +#define NPCM7XX_GET_CLASS(obj)                                          \
> +    OBJECT_GET_CLASS(NPCM7xxClass, (obj), TYPE_NPCM7XX)
> +
> +/**
> + * npcm7xx_write_secondary_boot - Write stub for booting secondary CPU.
> + * @cpu: The CPU to be booted.
> + * @info: Boot info structure for the board.
> + *
> + * This will write a short code stub to the internal ROM that will keep the
> + * secondary CPU spinning until the primary CPU writes an address to the SCRPAD
> + * register in the GCR, after which the secondary CPU will jump there.
> + */
> +extern void npcm7xx_write_secondary_boot(ARMCPU *cpu,
> +                                         const struct arm_boot_info *info);
> +
> +#endif /* NPCM7XX_H */
>
Philippe Mathieu-Daudé July 8, 2020, 5:56 p.m. UTC | #2
On 7/8/20 7:31 PM, Philippe Mathieu-Daudé wrote:
> Hi Havard,
> 
> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
>> The Nuvoton NPCM7xx SoC family are used to implement Baseboard
>> Management Controllers in servers. While the family includes four SoCs,
>> this patch implements limited support for two of them: NPCM730 (targeted
>> for Data Center applications) and NPCM750 (targeted for Enterprise
>> applications).
>>
>> This patch includes little more than the bare minimum needed to boot a
>> Linux kernel built with NPCM7xx support in direct-kernel mode:
>>
>>   - Two Cortex-A9 CPU cores with built-in periperhals.
>>   - Global Configuration Registers.
>>   - Clock Management.
>>   - 3 Timer Modules with 5 timers each.
>>   - 4 serial ports.
>>
>> The chips themselves have a lot more features, some of which will be
>> added to the model at a later stage.
[...]

>> +static void npcm7xx_realize(DeviceState *dev, Error **errp)
>> +{
>> +    NPCM7xxState *s = NPCM7XX(dev);
>> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
>> +    Error *err = NULL;
>> +    int i;
>> +
>> +    /* I/O space -- unimplemented unless overridden below. */
>> +    create_unimplemented_device("npcm7xx.io", NPCM7XX_MMIO_BA, NPCM7XX_MMIO_SZ);
> 
> I still insist this is not the best, but as "The data sheet for these
> SoCs is not generally available" there is not much I can suggest to
> improve.

From your other comment I found:

https://github.com/Nuvoton-Israel/bootblock/blob/master/SWC_HAL/Chips/npcm750/npcm750.h

In particular:

#define AHB1_BASE_ADDR                  0xF0000000      /* AHB1
allocation (Including APB allocations)  */
#define AHB18_BASE_ADDR                 0x80000000      /* AHB18
allocation  */
#define AHB3_BASE_ADDR                  0xA0000000      /* AHB3
allocation  */
#define XBUSR_BASE_ADDR                 0xC0002000      /* XBUS
registers  */
#define AHB14_BASE_ADDR                 0xE0000000      /* AHB14
Allocation  */
#define APB14_BASE_ADDR                 0xE0000000      /* APB14
Allocation  */
#define VDMX_BASE_ADDR                  0xE0800000      /* VDMX  */

XBUS doesn't seem important.

If SPI flashes aren't connected, returning bus transaction sounds
correct:

#define SPI0CS0_BASE_ADDR               0x80000000      /* SPI0 direct
access CS0  */
#define SPI0CS1_BASE_ADDR               0x88000000      /* SPI0 direct
access CS1  */
#define SPI0CS2_BASE_ADDR               0x90000000      /* SPI0 direct
access CS2  */
#define SPI0CS3_BASE_ADDR               0x98000000      /* SPI0 direct
access CS3  */

#define SPI3CS0_BASE_ADDR               0xA0000000      /* SPI3 direct
access CS0  */
#define SPI3CS1_BASE_ADDR               0xA8000000      /* SPI3 direct
access CS1  */
#define SPI3CS2_BASE_ADDR               0xB0000000      /* SPI3 direct
access CS2  */
#define SPI3CS3_BASE_ADDR               0xB8000000      /* SPI3 direct
access CS3  */

So I'd prefer you use:

  create_unimplemented_device("npcm7xx.AHB1",  0xf0000000, 256 * MiB);

Maybe for the PCI root complex:

  create_unimplemented_device("npcm7xx.AHB14", 0xe0000000, 256 * MiB);

What do you think?

Regards,

Phil.
Havard Skinnemoen July 8, 2020, 6:13 p.m. UTC | #3
On Wed, Jul 8, 2020 at 10:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Havard,
>
> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> > The Nuvoton NPCM7xx SoC family are used to implement Baseboard
> > Management Controllers in servers. While the family includes four SoCs,
> > this patch implements limited support for two of them: NPCM730 (targeted
> > for Data Center applications) and NPCM750 (targeted for Enterprise
> > applications).
> >
> > This patch includes little more than the bare minimum needed to boot a
> > Linux kernel built with NPCM7xx support in direct-kernel mode:
> >
> >   - Two Cortex-A9 CPU cores with built-in periperhals.
> >   - Global Configuration Registers.
> >   - Clock Management.
> >   - 3 Timer Modules with 5 timers each.
> >   - 4 serial ports.
> >
> > The chips themselves have a lot more features, some of which will be
> > added to the model at a later stage.
> >
> > Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> > Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> > ---
> >  hw/arm/Makefile.objs     |   1 +
> >  hw/arm/npcm7xx.c         | 328 +++++++++++++++++++++++++++++++++++++++
> >  include/hw/arm/npcm7xx.h |  81 ++++++++++
>
> Please have a look at the scripts/git.orderfile, using it would
> ease our reviews.

Oh cool.

git config diff.orderFile scripts/git.orderfile

>
> >  3 files changed, 410 insertions(+)
> >  create mode 100644 hw/arm/npcm7xx.c
> >  create mode 100644 include/hw/arm/npcm7xx.h
> >
> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> > index 534a6a119e..13d163a599 100644
> > --- a/hw/arm/Makefile.objs
> > +++ b/hw/arm/Makefile.objs
> > @@ -41,6 +41,7 @@ obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
> >  obj-$(CONFIG_STM32F405_SOC) += stm32f405_soc.o
> >  obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o
> >  obj-$(CONFIG_XLNX_VERSAL) += xlnx-versal.o xlnx-versal-virt.o
> > +obj-$(CONFIG_NPCM7XX) += npcm7xx.o
> >  obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
> >  obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
> >  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o
> > diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> > new file mode 100644
> > index 0000000000..0a9e30f66f
> > --- /dev/null
> > +++ b/hw/arm/npcm7xx.c
> > @@ -0,0 +1,328 @@
> > +/*
> > + * Nuvoton NPCM7xx SoC family.
> > + *
> > + * Copyright 2020 Google LLC
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> > + * for more details.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "exec/address-spaces.h"
> > +#include "hw/arm/npcm7xx.h"
> > +#include "hw/char/serial.h"
> > +#include "hw/loader.h"
> > +#include "hw/misc/unimp.h"
> > +#include "hw/qdev-properties.h"
> > +#include "qapi/error.h"
> > +#include "qemu/units.h"
> > +#include "sysemu/sysemu.h"
> > +
> > +/* The first half of the address space is reserved for DDR4 DRAM. */
> > +#define NPCM7XX_DRAM_BA         (0x00000000)
> > +#define NPCM7XX_DRAM_SZ         (2 * GiB)
> > +
> > +/*
> > + * This covers the whole MMIO space. We'll use this to catch any MMIO accesses
> > + * that aren't handled by any device.
> > + */
> > +#define NPCM7XX_MMIO_BA         (0x80000000)
> > +#define NPCM7XX_MMIO_SZ         (0x7FFD0000)
> > +
> > +/* Core system modules. */
> > +#define NPCM7XX_L2C_BA          (0xF03FC000)
> > +#define NPCM7XX_CPUP_BA         (0xF03FE000)
> > +#define NPCM7XX_GCR_BA          (0xF0800000)
> > +#define NPCM7XX_CLK_BA          (0xF0801000)
> > +
> > +/* Memory blocks at the end of the address space */
> > +#define NPCM7XX_RAM2_BA         (0xFFFD0000)
> > +#define NPCM7XX_RAM2_SZ         (128 * KiB)
> > +#define NPCM7XX_ROM_BA          (0xFFFF0000)
> > +#define NPCM7XX_ROM_SZ          (64 * KiB)
> > +
> > +/*
> > + * Interrupt lines going into the GIC. This does not include internal Cortex-A9
> > + * interrupts.
> > + */
> > +enum NPCM7xxInterrupt {
> > +    NPCM7XX_UART0_IRQ           = 2,
> > +    NPCM7XX_UART1_IRQ,
> > +    NPCM7XX_UART2_IRQ,
> > +    NPCM7XX_UART3_IRQ,
> > +    NPCM7XX_TIMER0_IRQ          = 32,   /* Timer Module 0 */
> > +    NPCM7XX_TIMER1_IRQ,
> > +    NPCM7XX_TIMER2_IRQ,
> > +    NPCM7XX_TIMER3_IRQ,
> > +    NPCM7XX_TIMER4_IRQ,
> > +    NPCM7XX_TIMER5_IRQ,                 /* Timer Module 1 */
> > +    NPCM7XX_TIMER6_IRQ,
> > +    NPCM7XX_TIMER7_IRQ,
> > +    NPCM7XX_TIMER8_IRQ,
> > +    NPCM7XX_TIMER9_IRQ,
> > +    NPCM7XX_TIMER10_IRQ,                /* Timer Module 2 */
> > +    NPCM7XX_TIMER11_IRQ,
> > +    NPCM7XX_TIMER12_IRQ,
> > +    NPCM7XX_TIMER13_IRQ,
> > +    NPCM7XX_TIMER14_IRQ,
> > +};
> > +
> > +/* Total number of GIC interrupts, including internal Cortex-A9 interrupts. */
> > +#define NPCM7XX_NUM_IRQ         (160)
> > +
> > +/* Register base address for each Timer Module */
> > +static const hwaddr npcm7xx_tim_addr[] = {
> > +    0xF0008000,
> > +    0xF0009000,
> > +    0xF000A000,
>
> Here caps hex ...
>
> > +};
> > +
> > +/* Register base address for each 16550 UART */
> > +static const hwaddr npcm7xx_uart_addr[] = {
> > +    0xF0001000,
> > +    0xF0002000,
> > +    0xF0003000,
> > +    0xF0004000,
> > +};
> > +
> > +void npcm7xx_write_secondary_boot(ARMCPU *cpu, const struct arm_boot_info *info)
> > +{
> > +    /*
> > +     * The default smpboot stub halts the secondary CPU with a 'wfi'
> > +     * instruction, but the arch/arm/mach-npcm/platsmp.c in the Linux kernel
> > +     * does not send an IPI to wake it up, so the second CPU fails to boot. So
> > +     * we need to provide our own smpboot stub that can not use 'wfi', it has
> > +     * to spin the secondary CPU until the first CPU writes to the SCRPAD reg.
> > +     */
> > +    uint32_t smpboot[] = {
> > +        0xe59f2018,     /* ldr r2, bootreg_addr */
> > +        0xe3a00000,     /* mov r0, #0 */
> > +        0xe5820000,     /* str r0, [r2] */
> > +        0xe320f002,     /* wfe */
> > +        0xe5921000,     /* ldr r1, [r2] */
> > +        0xe1110001,     /* tst r1, r1 */
> > +        0x0afffffb,     /* beq <wfe> */
> > +        0xe12fff11,     /* bx r1 */
>
> ... here small hex. Pick one style?

Oh wow, I'm being super inconsistent. I'll standardize on small hex throughout.

>
> > +        NPCM7XX_SMP_BOOTREG_ADDR,
> > +    };
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(smpboot); i++) {
> > +        smpboot[i] = tswap32(smpboot[i]);
> > +    }
> > +
> > +    rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
> > +                       NPCM7XX_SMP_LOADER_START);
> > +}
> > +
> > +static qemu_irq npcm7xx_irq(NPCM7xxState *s, int n)
> > +{
> > +    return qdev_get_gpio_in(DEVICE(&s->a9mpcore), n);
> > +}
> > +
> > +static void npcm7xx_init(Object *obj)
> > +{
> > +    NPCM7xxState *s = NPCM7XX(obj);
> > +    int i;
> > +
> > +    for (i = 0; i < NPCM7XX_MAX_NUM_CPUS; i++) {
> > +        object_initialize_child(obj, "cpu[*]", &s->cpu[i],
> > +                                ARM_CPU_TYPE_NAME("cortex-a9"));
> > +    }
> > +
> > +    object_initialize_child(obj, "a9mpcore", &s->a9mpcore, TYPE_A9MPCORE_PRIV);
> > +    object_initialize_child(obj, "gcr", &s->gcr, TYPE_NPCM7XX_GCR);
> > +    object_property_add_alias(obj, "power-on-straps", OBJECT(&s->gcr),
> > +                              "power-on-straps");
> > +    object_initialize_child(obj, "clk", &s->clk, TYPE_NPCM7XX_CLK);
> > +
> > +    for (i = 0; i < ARRAY_SIZE(s->tim); i++) {
> > +        object_initialize_child(obj, "tim[*]", &s->tim[i], TYPE_NPCM7XX_TIMER);
> > +    }
> > +}
> > +
> > +static void npcm7xx_realize(DeviceState *dev, Error **errp)
> > +{
> > +    NPCM7xxState *s = NPCM7XX(dev);
> > +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
> > +    Error *err = NULL;
> > +    int i;
> > +
> > +    /* I/O space -- unimplemented unless overridden below. */
> > +    create_unimplemented_device("npcm7xx.io", NPCM7XX_MMIO_BA, NPCM7XX_MMIO_SZ);
>
> I still insist this is not the best, but as "The data sheet for these
> SoCs is not generally available" there is not much I can suggest to
> improve.

OK, I'll try what you suggested in the next message.

> > +
> > +    /* CPUs */
> > +    for (i = 0; i < nc->num_cpus; i++) {
> > +        object_property_set_int(OBJECT(&s->cpu[i]),
> > +                                arm_cpu_mp_affinity(i, NPCM7XX_MAX_NUM_CPUS),
> > +                                "mp-affinity", &error_abort);
> > +        object_property_set_int(OBJECT(&s->cpu[i]), NPCM7XX_GIC_CPU_IF_ADDR,
> > +                                "reset-cbar", &error_abort);
> > +        object_property_set_bool(OBJECT(&s->cpu[i]), true,
> > +                                 "reset-hivecs", &error_abort);
> > +
> > +        /* Disable security extensions. */
> > +        if (object_property_find(OBJECT(&s->cpu[i]), "has_el3", NULL)) {
>
> You know these are cortex-a9 right? Why bother checking?

Most other SoCs do, but you're right, might be better to abort if the
CPU isn't what we expect.

>
> > +            object_property_set_bool(OBJECT(&s->cpu[i]), false, "has_el3",
> > +                                     &error_abort);
> > +        }
> > +
> > +        qdev_realize(DEVICE(&s->cpu[i]), NULL, &err);
> > +        if (err) {
> > +            error_propagate(errp, err);
>
> Since you don't plan to create a SoC at runtime and hot-plug it, I'd
> simplify by using &error_abort everywhere in this function, and forget
> about propagating.

Is it safe to assume we'll never hot-plug SoCs?

I can't think of a good use case for it, and removing error
propagation will make the code significantly shorter and easier to
follow, so I'll do what you suggest.

> > +            return;
> > +        }
> > +    }
> > +
> > +    /* A9MPCORE peripherals */
> > +    object_property_set_int(OBJECT(&s->a9mpcore), nc->num_cpus, "num-cpu",
> > +                            &error_abort);
>
> See, you already use &error_abort ;)

Right, I generally only propagate errors from realize(), but I guess
that's not necessary either.

> > +    object_property_set_int(OBJECT(&s->a9mpcore), NPCM7XX_NUM_IRQ, "num-irq",
> > +                            &error_abort);
> > +    sysbus_realize(SYS_BUS_DEVICE(&s->a9mpcore), &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +    }
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->a9mpcore), 0, NPCM7XX_CPUP_BA);
> > +
> > +    for (i = 0; i < nc->num_cpus; i++) {
> > +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->a9mpcore), i,
> > +                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
> > +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->a9mpcore), i + nc->num_cpus,
> > +                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_FIQ));
> > +    }
> > +
> > +    /* L2 cache controller */
> > +    sysbus_create_simple("l2x0", NPCM7XX_L2C_BA, NULL);
> > +
> > +    /* System Global Control Registers (GCR) */
> > +    object_property_set_int(OBJECT(&s->gcr), nc->disabled_modules,
> > +                            "disabled-modules", &error_abort);
> > +    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
> > +                             &error_abort);
>
> I guess you can simplify using in npcm7xx_init():
>
>       object_property_add_const_link(obj, "dram-mr", OBJECT(&s->gcr));
>
> And in npcm7xx_gcr_realize()
>
>     obj = object_property_get_link(OBJECT(dev), "dram-mr", &err);
>     if (obj == NULL) {
>         error_setg(errp, "%s: required dram-mr link not found: %s",
>                    __func__, error_get_pretty(err));
>         return;
>     }
>     s->dram = MEMORY_REGION(obj);

OK, I'll try that, thanks!

> > +    sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gcr), 0, NPCM7XX_GCR_BA);
> > +
> > +    /* Clock Control Registers (CLK) */
> > +    sysbus_realize(SYS_BUS_DEVICE(&s->clk), &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->clk), 0, NPCM7XX_CLK_BA);
> > +
> > +    /* Timer Modules (TIM) */
> > +    QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_tim_addr) != ARRAY_SIZE(s->tim));
> > +    for (i = 0; i < ARRAY_SIZE(s->tim); i++) {
> > +        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->tim[i]);
> > +        int first_irq;
> > +        int j;
> > +
> > +        sysbus_realize(sbd, &err);
> > +        if (err) {
> > +            error_propagate(errp, err);
> > +            return;
> > +        }
> > +        sysbus_mmio_map(sbd, 0, npcm7xx_tim_addr[i]);
> > +
> > +        first_irq = NPCM7XX_TIMER0_IRQ + i * NPCM7XX_TIMERS_PER_CTRL;
> > +        for (j = 0; j < NPCM7XX_TIMERS_PER_CTRL; j++) {
> > +            qemu_irq irq = npcm7xx_irq(s, first_irq + j);
> > +            sysbus_connect_irq(sbd, j, irq);
> > +        }
> > +    }
> > +
> > +    /* UART0..3 (16550 compatible) */
> > +    for (i = 0; i < ARRAY_SIZE(npcm7xx_uart_addr); i++) {
> > +        serial_mm_init(get_system_memory(), npcm7xx_uart_addr[i], 2,
> > +                       npcm7xx_irq(s, NPCM7XX_UART0_IRQ + i), 115200,
> > +                       serial_hd(i), DEVICE_LITTLE_ENDIAN);
> > +    }
> > +
> > +    /* RAM2 (SRAM) */
> > +    memory_region_init_ram(&s->sram, OBJECT(dev), "ram2",
> > +                           NPCM7XX_RAM2_SZ, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +    memory_region_add_subregion(get_system_memory(), NPCM7XX_RAM2_BA, &s->sram);
> > +
> > +    /* Internal ROM */
> > +    memory_region_init_rom(&s->irom, OBJECT(dev), "irom", NPCM7XX_ROM_SZ, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +    memory_region_add_subregion(get_system_memory(), NPCM7XX_ROM_BA, &s->irom);
> > +
> > +    /* External DDR4 SDRAM */
> > +    memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, s->dram);
>
> This doesn't look correct, the DRAM is an external component, not
> embedded within the SoC. See the manual ;)
> https://www.nuvoton.com/products/cloud-computing/ibmc/
>
> You should call that in npcm7xx_create_soc() (and eventually rename it)
> or add npcm7xx_create_dram(), maybe cleaner (and call it in each
> mc->init).

OK, will do.


> > +}
> > +
> > +static Property npcm7xx_properties[] = {
> > +    DEFINE_PROP_LINK("dram", NPCM7xxState, dram, TYPE_MEMORY_REGION,
> > +                     MemoryRegion *),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void npcm7xx_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +
> > +    dc->realize = npcm7xx_realize;
> > +    dc->user_creatable = false;
>
> This confirms you can safely use &error_abort in realize().

Got it.

> > +    device_class_set_props(dc, npcm7xx_properties);
> > +}
> > +
> > +static void npcm730_class_init(ObjectClass *oc, void *data)
> > +{
> > +    NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
> > +
> > +    /* NPCM730 is optimized for data center use, so no graphics, etc. */
> > +    nc->disabled_modules = 0x00300395;
> > +    nc->num_cpus = 2;
> > +}
> > +
> > +static void npcm750_class_init(ObjectClass *oc, void *data)
> > +{
> > +    NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
> > +
> > +    /* NPCM750 has 2 cores and a full set of peripherals */
> > +    nc->disabled_modules = 0x00000000;
> > +    nc->num_cpus = 2;
> > +}
> > +
> > +static const TypeInfo npcm7xx_soc_types[] = {
> > +    {
> > +        .name           = TYPE_NPCM7XX,
> > +        .parent         = TYPE_DEVICE,
> > +        .instance_size  = sizeof(NPCM7xxState),
> > +        .instance_init  = npcm7xx_init,
> > +        .class_size     = sizeof(NPCM7xxClass),
> > +        .class_init     = npcm7xx_class_init,
> > +        .abstract       = true,
> > +    }, {
> > +        .name           = TYPE_NPCM730,
> > +        .parent         = TYPE_NPCM7XX,
> > +        .class_init     = npcm730_class_init,
> > +    }, {
> > +        .name           = TYPE_NPCM750,
> > +        .parent         = TYPE_NPCM7XX,
> > +        .class_init     = npcm750_class_init,
> > +    },
> > +};
> > +
> > +DEFINE_TYPES(npcm7xx_soc_types);
> > diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
> > new file mode 100644
> > index 0000000000..2ffa573b11
> > --- /dev/null
> > +++ b/include/hw/arm/npcm7xx.h
> > @@ -0,0 +1,81 @@
> > +/*
> > + * Nuvoton NPCM7xx SoC family.
> > + *
> > + * Copyright 2020 Google LLC
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> > + * for more details.
> > + */
> > +#ifndef NPCM7XX_H
> > +#define NPCM7XX_H
> > +
> > +#include "hw/boards.h"
> > +#include "hw/cpu/a9mpcore.h"
> > +#include "hw/misc/npcm7xx_clk.h"
> > +#include "hw/misc/npcm7xx_gcr.h"
> > +#include "hw/timer/npcm7xx_timer.h"
> > +#include "target/arm/cpu.h"
> > +
> > +#define NPCM7XX_MAX_NUM_CPUS    (2)
> > +
> > +/* Magic addresses for setting up direct kernel booting and SMP boot stubs. */
> > +#define NPCM7XX_LOADER_START            (0x00000000)  /* Start of SDRAM */
> > +#define NPCM7XX_SMP_LOADER_START        (0xFFFF0000)  /* Boot ROM */
> > +#define NPCM7XX_SMP_BOOTREG_ADDR        (0xF080013C)  /* GCR.SCRPAD */
> > +#define NPCM7XX_GIC_CPU_IF_ADDR         (0xF03FE100)  /* GIC within A9 */
> > +
> > +typedef struct NPCM7xxState {
> > +    DeviceState         parent;
> > +
> > +    ARMCPU              cpu[NPCM7XX_MAX_NUM_CPUS];
> > +    A9MPPrivState       a9mpcore;
> > +
> > +    MemoryRegion        sram;
> > +    MemoryRegion        irom;
> > +    MemoryRegion        *dram;
> > +
> > +    NPCM7xxGCRState     gcr;
> > +    NPCM7xxCLKState     clk;
> > +    NPCM7xxTimerCtrlState tim[3];
> > +} NPCM7xxState;
> > +
> > +#define TYPE_NPCM7XX    "npcm7xx"
> > +#define NPCM7XX(obj)    OBJECT_CHECK(NPCM7xxState, (obj), TYPE_NPCM7XX)
> > +
> > +#define TYPE_NPCM730    "npcm730"
> > +#define TYPE_NPCM750    "npcm750"
> > +
> > +typedef struct NPCM7xxClass {
> > +    DeviceClass         parent;
>
> Similar comment that elsewhere on this series, if NPCM7xxClass not used
> outside of npcm7xx.c, keep it local.

OK, will do.

>
> Very good patch, I wanted to R-b it but the DRAM part makes me think
> it might be worth a v5...

Sure, I'll do a v5. Thanks a lot for reviewing!

Havard
Havard Skinnemoen July 8, 2020, 9:47 p.m. UTC | #4
On Wed, Jul 8, 2020 at 11:13 AM Havard Skinnemoen
<hskinnemoen@google.com> wrote:
>
> On Wed, Jul 8, 2020 at 10:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> > > +    /* System Global Control Registers (GCR) */
> > > +    object_property_set_int(OBJECT(&s->gcr), nc->disabled_modules,
> > > +                            "disabled-modules", &error_abort);
> > > +    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
> > > +                             &error_abort);
> >
> > I guess you can simplify using in npcm7xx_init():
> >
> >       object_property_add_const_link(obj, "dram-mr", OBJECT(&s->gcr));
> >
> > And in npcm7xx_gcr_realize()
> >
> >     obj = object_property_get_link(OBJECT(dev), "dram-mr", &err);
> >     if (obj == NULL) {
> >         error_setg(errp, "%s: required dram-mr link not found: %s",
> >                    __func__, error_get_pretty(err));
> >         return;
> >     }
> >     s->dram = MEMORY_REGION(obj);
>
> OK, I'll try that, thanks!

Hmm, I ended up doing

-    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
-                             &error_abort);
+    object_property_add_const_link(OBJECT(&s->gcr), "dram-mr",
OBJECT(s->dram));

in realize() because s->dram isn't initialized yet in npcm7xx_init().
Is this what you had in mind?

Here's the diff from all the dram-related changes:

diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
index 39a1f28d8e..30e00a514d 100644
--- a/include/hw/arm/npcm7xx.h
+++ b/include/hw/arm/npcm7xx.h
@@ -28,6 +28,10 @@

 #define NPCM7XX_MAX_NUM_CPUS    (2)

+/* The first half of the address space is reserved for DDR4 DRAM. */
+#define NPCM7XX_DRAM_BA         (0x00000000)
+#define NPCM7XX_DRAM_SZ         (2 * GiB)
+
 /* Magic addresses for setting up direct kernel booting and SMP boot stubs. */
 #define NPCM7XX_LOADER_START            (0x00000000)  /* Start of SDRAM */
 #define NPCM7XX_SMP_LOADER_START        (0xffff0000)  /* Boot ROM */
diff --git a/include/hw/misc/npcm7xx_gcr.h b/include/hw/misc/npcm7xx_gcr.h
index 49d699410f..4884676be2 100644
--- a/include/hw/misc/npcm7xx_gcr.h
+++ b/include/hw/misc/npcm7xx_gcr.h
@@ -68,7 +68,6 @@ typedef struct NPCM7xxGCRState {
     uint32_t reset_pwron;
     uint32_t reset_mdlr;
     uint32_t reset_intcr3;
-    MemoryRegion *dram;
 } NPCM7xxGCRState;

 #define TYPE_NPCM7XX_GCR "npcm7xx-gcr"
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index e64cf6a84c..a05a900197 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -26,10 +26,6 @@
 #include "qemu/units.h"
 #include "sysemu/sysemu.h"

-/* The first half of the address space is reserved for DDR4 DRAM. */
-#define NPCM7XX_DRAM_BA         (0x00000000)
-#define NPCM7XX_DRAM_SZ         (2 * GiB)
-
 /*
  * This covers the whole MMIO space. We'll use this to catch any MMIO accesses
  * that aren't handled by any device.
@@ -257,8 +253,7 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
     /* System Global Control Registers (GCR) */
     object_property_set_int(OBJECT(&s->gcr), nc->disabled_modules,
                             "disabled-modules", &error_abort);
-    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
-                             &error_abort);
+    object_property_add_const_link(OBJECT(&s->gcr), "dram-mr",
OBJECT(s->dram));
     sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &error_abort);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->gcr), 0, NPCM7XX_GCR_BA);

@@ -326,13 +321,10 @@ static void npcm7xx_realize(DeviceState *dev,
Error **errp)
     memory_region_init_rom(&s->irom, OBJECT(dev), "irom", NPCM7XX_ROM_SZ,
                            &error_abort);
     memory_region_add_subregion(get_system_memory(), NPCM7XX_ROM_BA, &s->irom);
-
-    /* External DDR4 SDRAM */
-    memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, s->dram);
 }

 static Property npcm7xx_properties[] = {
-    DEFINE_PROP_LINK("dram", NPCM7xxState, dram, TYPE_MEMORY_REGION,
+    DEFINE_PROP_LINK("dram-mr", NPCM7xxState, dram, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 2f66e699b1..cfb31ce6f5 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -83,21 +83,25 @@ static void npcm7xx_connect_flash(NPCM7xxFIUState
*fiu, int cs_no,
     sysbus_connect_irq(SYS_BUS_DEVICE(fiu), cs_no, flash_cs);
 }

+static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram)
+{
+    memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, dram);
+
+    object_property_set_link(OBJECT(soc), OBJECT(dram), "dram-mr",
+                             &error_abort);
+}
+
 static NPCM7xxState *npcm7xx_create_soc(MachineState *machine,
                                         uint32_t hw_straps)
 {
     NPCM7xxMachineClass *nmc = NPCM7XX_MACHINE_GET_CLASS(machine);
-    NPCM7xxState *soc;
+    Object *obj;

-    soc = NPCM7XX(object_new_with_props(nmc->soc_type, OBJECT(machine), "soc",
-                                        &error_abort, NULL));
-    object_property_set_link(OBJECT(soc), OBJECT(machine->ram), "dram",
-                             &error_abort);
-    object_property_set_uint(OBJECT(soc), hw_straps, "power-on-straps",
-                             &error_abort);
-    qdev_realize(DEVICE(soc), NULL, &error_abort);
+    obj = object_new_with_props(nmc->soc_type, OBJECT(machine), "soc",
+                                &error_abort, NULL);
+    object_property_set_uint(obj, hw_straps, "power-on-straps", &error_abort);

-    return soc;
+    return NPCM7XX(obj);
 }

 static void npcm750_evb_init(MachineState *machine)
@@ -105,6 +109,9 @@ static void npcm750_evb_init(MachineState *machine)
     NPCM7xxState *soc;

     soc = npcm7xx_create_soc(machine, NPCM750_EVB_POWER_ON_STRAPS);
+    npcm7xx_connect_dram(soc, machine->ram);
+    qdev_realize(DEVICE(soc), NULL, &error_abort);
+
     npcm7xx_load_bootrom(soc);
     npcm7xx_connect_flash(&soc->fiu[0], 0, "w25q256", drive_get(IF_MTD, 0, 0));
     npcm7xx_load_kernel(machine, soc);
@@ -115,6 +122,9 @@ static void quanta_gsj_init(MachineState *machine)
     NPCM7xxState *soc;

     soc = npcm7xx_create_soc(machine, QUANTA_GSJ_POWER_ON_STRAPS);
+    npcm7xx_connect_dram(soc, machine->ram);
+    qdev_realize(DEVICE(soc), NULL, &error_abort);
+
     npcm7xx_load_bootrom(soc);
     npcm7xx_connect_flash(&soc->fiu[0], 0, "mx25l25635e",
                           drive_get(IF_MTD, 0, 0));
diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c
index 78a885e265..9934cd238d 100644
--- a/hw/misc/npcm7xx_gcr.c
+++ b/hw/misc/npcm7xx_gcr.c
@@ -127,11 +127,16 @@ static void npcm7xx_gcr_realize(DeviceState
*dev, Error **errp)
 {
     NPCM7xxGCRState *s = NPCM7XX_GCR(dev);
     uint64_t dram_size;
+    Error *err = NULL;
+    Object *obj;

-    if (!s->dram) {
-        error_setg(errp, "npcm7xx_gcr: 'dram' link not set");
+    obj = object_property_get_link(OBJECT(dev), "dram-mr", &err);
+    if (!obj) {
+        error_setg(errp, "%s: required dram-mr link not found: %s",
+                   __func__, error_get_pretty(err));
         return;
     }
+    dram_size = memory_region_size(MEMORY_REGION(obj));

     /* Power-on reset value */
     s->reset_intcr3 = 0x00001002;
@@ -149,7 +154,6 @@ static void npcm7xx_gcr_realize(DeviceState *dev,
Error **errp)
      *
      * https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244
      */
-    dram_size = int128_get64(s->dram->size);
     if (dram_size >= 2 * GiB) {
         s->reset_intcr3 |= 4 << 8;
     } else if (dram_size >= 1 * GiB) {
@@ -191,8 +195,6 @@ static const VMStateDescription vmstate_npcm7xx_gcr = {
 static Property npcm7xx_gcr_properties[] = {
     DEFINE_PROP_UINT32("disabled-modules", NPCM7xxGCRState, reset_mdlr, 0),
     DEFINE_PROP_UINT32("power-on-straps", NPCM7xxGCRState, reset_pwron, 0),
-    DEFINE_PROP_LINK("dram", NPCM7xxGCRState, dram, TYPE_MEMORY_REGION,
-                     MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
 };
Havard Skinnemoen July 9, 2020, 12:06 a.m. UTC | #5
On Wed, Jul 8, 2020 at 11:13 AM Havard Skinnemoen
<hskinnemoen@google.com> wrote:
> On Wed, Jul 8, 2020 at 10:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> > > +typedef struct NPCM7xxClass {
> > > +    DeviceClass         parent;
> >
> > Similar comment that elsewhere on this series, if NPCM7xxClass not used
> > outside of npcm7xx.c, keep it local.
>
> OK, will do.

Turns out it is used in npcm7xx_boards.c, so it has to stay where it is.

Havard
Havard Skinnemoen July 9, 2020, 12:23 a.m. UTC | #6
On Wed, Jul 8, 2020 at 10:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/8/20 7:31 PM, Philippe Mathieu-Daudé wrote:
> > Hi Havard,
> >
> > On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> >> The Nuvoton NPCM7xx SoC family are used to implement Baseboard
> >> Management Controllers in servers. While the family includes four SoCs,
> >> this patch implements limited support for two of them: NPCM730 (targeted
> >> for Data Center applications) and NPCM750 (targeted for Enterprise
> >> applications).
> >>
> >> This patch includes little more than the bare minimum needed to boot a
> >> Linux kernel built with NPCM7xx support in direct-kernel mode:
> >>
> >>   - Two Cortex-A9 CPU cores with built-in periperhals.
> >>   - Global Configuration Registers.
> >>   - Clock Management.
> >>   - 3 Timer Modules with 5 timers each.
> >>   - 4 serial ports.
> >>
> >> The chips themselves have a lot more features, some of which will be
> >> added to the model at a later stage.
> [...]
>
> >> +static void npcm7xx_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    NPCM7xxState *s = NPCM7XX(dev);
> >> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
> >> +    Error *err = NULL;
> >> +    int i;
> >> +
> >> +    /* I/O space -- unimplemented unless overridden below. */
> >> +    create_unimplemented_device("npcm7xx.io", NPCM7XX_MMIO_BA, NPCM7XX_MMIO_SZ);
> >
> > I still insist this is not the best, but as "The data sheet for these
> > SoCs is not generally available" there is not much I can suggest to
> > improve.
>
> From your other comment I found:
>
> https://github.com/Nuvoton-Israel/bootblock/blob/master/SWC_HAL/Chips/npcm750/npcm750.h
>
> In particular:
>
> #define AHB1_BASE_ADDR                  0xF0000000      /* AHB1
> allocation (Including APB allocations)  */
> #define AHB18_BASE_ADDR                 0x80000000      /* AHB18
> allocation  */
> #define AHB3_BASE_ADDR                  0xA0000000      /* AHB3
> allocation  */
> #define XBUSR_BASE_ADDR                 0xC0002000      /* XBUS
> registers  */
> #define AHB14_BASE_ADDR                 0xE0000000      /* AHB14
> Allocation  */
> #define APB14_BASE_ADDR                 0xE0000000      /* APB14
> Allocation  */
> #define VDMX_BASE_ADDR                  0xE0800000      /* VDMX  */
>
> XBUS doesn't seem important.
>
> If SPI flashes aren't connected, returning bus transaction sounds
> correct:
>
> #define SPI0CS0_BASE_ADDR               0x80000000      /* SPI0 direct
> access CS0  */
> #define SPI0CS1_BASE_ADDR               0x88000000      /* SPI0 direct
> access CS1  */
> #define SPI0CS2_BASE_ADDR               0x90000000      /* SPI0 direct
> access CS2  */
> #define SPI0CS3_BASE_ADDR               0x98000000      /* SPI0 direct
> access CS3  */
>
> #define SPI3CS0_BASE_ADDR               0xA0000000      /* SPI3 direct
> access CS0  */
> #define SPI3CS1_BASE_ADDR               0xA8000000      /* SPI3 direct
> access CS1  */
> #define SPI3CS2_BASE_ADDR               0xB0000000      /* SPI3 direct
> access CS2  */
> #define SPI3CS3_BASE_ADDR               0xB8000000      /* SPI3 direct
> access CS3  */
>
> So I'd prefer you use:
>
>   create_unimplemented_device("npcm7xx.AHB1",  0xf0000000, 256 * MiB);
>
> Maybe for the PCI root complex:
>
>   create_unimplemented_device("npcm7xx.AHB14", 0xe0000000, 256 * MiB);
>
> What do you think?

I went ahead and added them all since they are all defined in that
public file. It does make the -d unimp output a lot more helpful.

I'll send v5 tonight. Not sure if I got the DRAM stuff 100% right.
Please let me know what you think.

Havard
Philippe Mathieu-Daudé July 9, 2020, 5:17 a.m. UTC | #7
On 7/9/20 2:23 AM, Havard Skinnemoen wrote:
> On Wed, Jul 8, 2020 at 10:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 7/8/20 7:31 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Havard,
>>>
>>> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
>>>> The Nuvoton NPCM7xx SoC family are used to implement Baseboard
>>>> Management Controllers in servers. While the family includes four SoCs,
>>>> this patch implements limited support for two of them: NPCM730 (targeted
>>>> for Data Center applications) and NPCM750 (targeted for Enterprise
>>>> applications).
>>>>
>>>> This patch includes little more than the bare minimum needed to boot a
>>>> Linux kernel built with NPCM7xx support in direct-kernel mode:
>>>>
>>>>   - Two Cortex-A9 CPU cores with built-in periperhals.
>>>>   - Global Configuration Registers.
>>>>   - Clock Management.
>>>>   - 3 Timer Modules with 5 timers each.
>>>>   - 4 serial ports.
>>>>
>>>> The chips themselves have a lot more features, some of which will be
>>>> added to the model at a later stage.
>> [...]
>>
>>>> +static void npcm7xx_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    NPCM7xxState *s = NPCM7XX(dev);
>>>> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
>>>> +    Error *err = NULL;
>>>> +    int i;
>>>> +
>>>> +    /* I/O space -- unimplemented unless overridden below. */
>>>> +    create_unimplemented_device("npcm7xx.io", NPCM7XX_MMIO_BA, NPCM7XX_MMIO_SZ);
>>>
>>> I still insist this is not the best, but as "The data sheet for these
>>> SoCs is not generally available" there is not much I can suggest to
>>> improve.
>>
>> From your other comment I found:
>>
>> https://github.com/Nuvoton-Israel/bootblock/blob/master/SWC_HAL/Chips/npcm750/npcm750.h
>>
>> In particular:
>>
>> #define AHB1_BASE_ADDR                  0xF0000000      /* AHB1
>> allocation (Including APB allocations)  */
>> #define AHB18_BASE_ADDR                 0x80000000      /* AHB18
>> allocation  */
>> #define AHB3_BASE_ADDR                  0xA0000000      /* AHB3
>> allocation  */
>> #define XBUSR_BASE_ADDR                 0xC0002000      /* XBUS
>> registers  */
>> #define AHB14_BASE_ADDR                 0xE0000000      /* AHB14
>> Allocation  */
>> #define APB14_BASE_ADDR                 0xE0000000      /* APB14
>> Allocation  */
>> #define VDMX_BASE_ADDR                  0xE0800000      /* VDMX  */
>>
>> XBUS doesn't seem important.
>>
>> If SPI flashes aren't connected, returning bus transaction sounds
>> correct:
>>
>> #define SPI0CS0_BASE_ADDR               0x80000000      /* SPI0 direct
>> access CS0  */
>> #define SPI0CS1_BASE_ADDR               0x88000000      /* SPI0 direct
>> access CS1  */
>> #define SPI0CS2_BASE_ADDR               0x90000000      /* SPI0 direct
>> access CS2  */
>> #define SPI0CS3_BASE_ADDR               0x98000000      /* SPI0 direct
>> access CS3  */
>>
>> #define SPI3CS0_BASE_ADDR               0xA0000000      /* SPI3 direct
>> access CS0  */
>> #define SPI3CS1_BASE_ADDR               0xA8000000      /* SPI3 direct
>> access CS1  */
>> #define SPI3CS2_BASE_ADDR               0xB0000000      /* SPI3 direct
>> access CS2  */
>> #define SPI3CS3_BASE_ADDR               0xB8000000      /* SPI3 direct
>> access CS3  */
>>
>> So I'd prefer you use:
>>
>>   create_unimplemented_device("npcm7xx.AHB1",  0xf0000000, 256 * MiB);
>>
>> Maybe for the PCI root complex:
>>
>>   create_unimplemented_device("npcm7xx.AHB14", 0xe0000000, 256 * MiB);
>>
>> What do you think?
> 
> I went ahead and added them all since they are all defined in that
> public file. It does make the -d unimp output a lot more helpful.

Great news!

> I'll send v5 tonight. Not sure if I got the DRAM stuff 100% right.
> Please let me know what you think.

I am seeing this now and v5 is already posted, so I'll review it
directly instead.

> 
> Havard
>
Philippe Mathieu-Daudé July 9, 2020, 5:34 a.m. UTC | #8
On 7/9/20 2:06 AM, Havard Skinnemoen wrote:
> On Wed, Jul 8, 2020 at 11:13 AM Havard Skinnemoen
> <hskinnemoen@google.com> wrote:
>> On Wed, Jul 8, 2020 at 10:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
>>>> +typedef struct NPCM7xxClass {
>>>> +    DeviceClass         parent;
>>>
>>> Similar comment that elsewhere on this series, if NPCM7xxClass not used
>>> outside of npcm7xx.c, keep it local.
>>
>> OK, will do.
> 
> Turns out it is used in npcm7xx_boards.c, so it has to stay where it is.

Indeed:

static void npcm7xx_load_kernel(MachineState *machine,
                                NPCM7xxState *soc)
{
    NPCM7xxClass *sc = NPCM7XX_GET_CLASS(soc);

    npcm7xx_binfo.ram_size = machine->ram_size;
    npcm7xx_binfo.nb_cpus = sc->num_cpus;

    arm_load_kernel(&soc->cpu[0], machine, &npcm7xx_binfo);
}

This is fine.

Just thinking loudly, we traditionally add the load_kernel() code
in the machine, because it is often specific to Linux guest, and
the SoC doesn't need to know about the guest OS.

hw/arm/boot.c contains helpers also useful for firmwares.

The SoC has a link to the DRAM so can get its size.
All the arm_boot_info fields are specific to this SoC.
So we could move a lot of code to npcm7xx.c, only declaring:

  void npcm7xx_load_kernel(MachineState *machine,
                           NPCM7xxState *soc);
Havard Skinnemoen July 9, 2020, 7:23 a.m. UTC | #9
On Wed, Jul 8, 2020 at 10:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/9/20 2:06 AM, Havard Skinnemoen wrote:
> > On Wed, Jul 8, 2020 at 11:13 AM Havard Skinnemoen
> > <hskinnemoen@google.com> wrote:
> >> On Wed, Jul 8, 2020 at 10:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> >>>> +typedef struct NPCM7xxClass {
> >>>> +    DeviceClass         parent;
> >>>
> >>> Similar comment that elsewhere on this series, if NPCM7xxClass not used
> >>> outside of npcm7xx.c, keep it local.
> >>
> >> OK, will do.
> >
> > Turns out it is used in npcm7xx_boards.c, so it has to stay where it is.
>
> Indeed:
>
> static void npcm7xx_load_kernel(MachineState *machine,
>                                 NPCM7xxState *soc)
> {
>     NPCM7xxClass *sc = NPCM7XX_GET_CLASS(soc);
>
>     npcm7xx_binfo.ram_size = machine->ram_size;
>     npcm7xx_binfo.nb_cpus = sc->num_cpus;
>
>     arm_load_kernel(&soc->cpu[0], machine, &npcm7xx_binfo);
> }
>
> This is fine.

It's also used here:

static void npcm7xx_set_soc_type(NPCM7xxMachineClass *nmc, const char *type)
{
    NPCM7xxClass *sc = NPCM7XX_CLASS(object_class_by_name(type));
    MachineClass *mc = MACHINE_CLASS(nmc);

    nmc->soc_type = type;
    mc->default_cpus = mc->min_cpus = mc->max_cpus = sc->num_cpus;
}

>
> Just thinking loudly, we traditionally add the load_kernel() code
> in the machine, because it is often specific to Linux guest, and
> the SoC doesn't need to know about the guest OS.
>
> hw/arm/boot.c contains helpers also useful for firmwares.
>
> The SoC has a link to the DRAM so can get its size.
> All the arm_boot_info fields are specific to this SoC.
> So we could move a lot of code to npcm7xx.c, only declaring:
>
>   void npcm7xx_load_kernel(MachineState *machine,
>                            NPCM7xxState *soc);

I can do that, but it doesn't completely get rid of all references to
NPCM7xxClass. I'm afraid there will always be some amount of coupling
between the machines and corresponding SoCs.

Havard
diff mbox series

Patch

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 534a6a119e..13d163a599 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -41,6 +41,7 @@  obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
 obj-$(CONFIG_STM32F405_SOC) += stm32f405_soc.o
 obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o
 obj-$(CONFIG_XLNX_VERSAL) += xlnx-versal.o xlnx-versal-virt.o
+obj-$(CONFIG_NPCM7XX) += npcm7xx.o
 obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
 obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
 obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
new file mode 100644
index 0000000000..0a9e30f66f
--- /dev/null
+++ b/hw/arm/npcm7xx.c
@@ -0,0 +1,328 @@ 
+/*
+ * Nuvoton NPCM7xx SoC family.
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+
+#include "exec/address-spaces.h"
+#include "hw/arm/npcm7xx.h"
+#include "hw/char/serial.h"
+#include "hw/loader.h"
+#include "hw/misc/unimp.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "qemu/units.h"
+#include "sysemu/sysemu.h"
+
+/* The first half of the address space is reserved for DDR4 DRAM. */
+#define NPCM7XX_DRAM_BA         (0x00000000)
+#define NPCM7XX_DRAM_SZ         (2 * GiB)
+
+/*
+ * This covers the whole MMIO space. We'll use this to catch any MMIO accesses
+ * that aren't handled by any device.
+ */
+#define NPCM7XX_MMIO_BA         (0x80000000)
+#define NPCM7XX_MMIO_SZ         (0x7FFD0000)
+
+/* Core system modules. */
+#define NPCM7XX_L2C_BA          (0xF03FC000)
+#define NPCM7XX_CPUP_BA         (0xF03FE000)
+#define NPCM7XX_GCR_BA          (0xF0800000)
+#define NPCM7XX_CLK_BA          (0xF0801000)
+
+/* Memory blocks at the end of the address space */
+#define NPCM7XX_RAM2_BA         (0xFFFD0000)
+#define NPCM7XX_RAM2_SZ         (128 * KiB)
+#define NPCM7XX_ROM_BA          (0xFFFF0000)
+#define NPCM7XX_ROM_SZ          (64 * KiB)
+
+/*
+ * Interrupt lines going into the GIC. This does not include internal Cortex-A9
+ * interrupts.
+ */
+enum NPCM7xxInterrupt {
+    NPCM7XX_UART0_IRQ           = 2,
+    NPCM7XX_UART1_IRQ,
+    NPCM7XX_UART2_IRQ,
+    NPCM7XX_UART3_IRQ,
+    NPCM7XX_TIMER0_IRQ          = 32,   /* Timer Module 0 */
+    NPCM7XX_TIMER1_IRQ,
+    NPCM7XX_TIMER2_IRQ,
+    NPCM7XX_TIMER3_IRQ,
+    NPCM7XX_TIMER4_IRQ,
+    NPCM7XX_TIMER5_IRQ,                 /* Timer Module 1 */
+    NPCM7XX_TIMER6_IRQ,
+    NPCM7XX_TIMER7_IRQ,
+    NPCM7XX_TIMER8_IRQ,
+    NPCM7XX_TIMER9_IRQ,
+    NPCM7XX_TIMER10_IRQ,                /* Timer Module 2 */
+    NPCM7XX_TIMER11_IRQ,
+    NPCM7XX_TIMER12_IRQ,
+    NPCM7XX_TIMER13_IRQ,
+    NPCM7XX_TIMER14_IRQ,
+};
+
+/* Total number of GIC interrupts, including internal Cortex-A9 interrupts. */
+#define NPCM7XX_NUM_IRQ         (160)
+
+/* Register base address for each Timer Module */
+static const hwaddr npcm7xx_tim_addr[] = {
+    0xF0008000,
+    0xF0009000,
+    0xF000A000,
+};
+
+/* Register base address for each 16550 UART */
+static const hwaddr npcm7xx_uart_addr[] = {
+    0xF0001000,
+    0xF0002000,
+    0xF0003000,
+    0xF0004000,
+};
+
+void npcm7xx_write_secondary_boot(ARMCPU *cpu, const struct arm_boot_info *info)
+{
+    /*
+     * The default smpboot stub halts the secondary CPU with a 'wfi'
+     * instruction, but the arch/arm/mach-npcm/platsmp.c in the Linux kernel
+     * does not send an IPI to wake it up, so the second CPU fails to boot. So
+     * we need to provide our own smpboot stub that can not use 'wfi', it has
+     * to spin the secondary CPU until the first CPU writes to the SCRPAD reg.
+     */
+    uint32_t smpboot[] = {
+        0xe59f2018,     /* ldr r2, bootreg_addr */
+        0xe3a00000,     /* mov r0, #0 */
+        0xe5820000,     /* str r0, [r2] */
+        0xe320f002,     /* wfe */
+        0xe5921000,     /* ldr r1, [r2] */
+        0xe1110001,     /* tst r1, r1 */
+        0x0afffffb,     /* beq <wfe> */
+        0xe12fff11,     /* bx r1 */
+        NPCM7XX_SMP_BOOTREG_ADDR,
+    };
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(smpboot); i++) {
+        smpboot[i] = tswap32(smpboot[i]);
+    }
+
+    rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
+                       NPCM7XX_SMP_LOADER_START);
+}
+
+static qemu_irq npcm7xx_irq(NPCM7xxState *s, int n)
+{
+    return qdev_get_gpio_in(DEVICE(&s->a9mpcore), n);
+}
+
+static void npcm7xx_init(Object *obj)
+{
+    NPCM7xxState *s = NPCM7XX(obj);
+    int i;
+
+    for (i = 0; i < NPCM7XX_MAX_NUM_CPUS; i++) {
+        object_initialize_child(obj, "cpu[*]", &s->cpu[i],
+                                ARM_CPU_TYPE_NAME("cortex-a9"));
+    }
+
+    object_initialize_child(obj, "a9mpcore", &s->a9mpcore, TYPE_A9MPCORE_PRIV);
+    object_initialize_child(obj, "gcr", &s->gcr, TYPE_NPCM7XX_GCR);
+    object_property_add_alias(obj, "power-on-straps", OBJECT(&s->gcr),
+                              "power-on-straps");
+    object_initialize_child(obj, "clk", &s->clk, TYPE_NPCM7XX_CLK);
+
+    for (i = 0; i < ARRAY_SIZE(s->tim); i++) {
+        object_initialize_child(obj, "tim[*]", &s->tim[i], TYPE_NPCM7XX_TIMER);
+    }
+}
+
+static void npcm7xx_realize(DeviceState *dev, Error **errp)
+{
+    NPCM7xxState *s = NPCM7XX(dev);
+    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
+    Error *err = NULL;
+    int i;
+
+    /* I/O space -- unimplemented unless overridden below. */
+    create_unimplemented_device("npcm7xx.io", NPCM7XX_MMIO_BA, NPCM7XX_MMIO_SZ);
+
+    /* CPUs */
+    for (i = 0; i < nc->num_cpus; i++) {
+        object_property_set_int(OBJECT(&s->cpu[i]),
+                                arm_cpu_mp_affinity(i, NPCM7XX_MAX_NUM_CPUS),
+                                "mp-affinity", &error_abort);
+        object_property_set_int(OBJECT(&s->cpu[i]), NPCM7XX_GIC_CPU_IF_ADDR,
+                                "reset-cbar", &error_abort);
+        object_property_set_bool(OBJECT(&s->cpu[i]), true,
+                                 "reset-hivecs", &error_abort);
+
+        /* Disable security extensions. */
+        if (object_property_find(OBJECT(&s->cpu[i]), "has_el3", NULL)) {
+            object_property_set_bool(OBJECT(&s->cpu[i]), false, "has_el3",
+                                     &error_abort);
+        }
+
+        qdev_realize(DEVICE(&s->cpu[i]), NULL, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+    }
+
+    /* A9MPCORE peripherals */
+    object_property_set_int(OBJECT(&s->a9mpcore), nc->num_cpus, "num-cpu",
+                            &error_abort);
+    object_property_set_int(OBJECT(&s->a9mpcore), NPCM7XX_NUM_IRQ, "num-irq",
+                            &error_abort);
+    sysbus_realize(SYS_BUS_DEVICE(&s->a9mpcore), &err);
+    if (err) {
+        error_propagate(errp, err);
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->a9mpcore), 0, NPCM7XX_CPUP_BA);
+
+    for (i = 0; i < nc->num_cpus; i++) {
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->a9mpcore), i,
+                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->a9mpcore), i + nc->num_cpus,
+                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_FIQ));
+    }
+
+    /* L2 cache controller */
+    sysbus_create_simple("l2x0", NPCM7XX_L2C_BA, NULL);
+
+    /* System Global Control Registers (GCR) */
+    object_property_set_int(OBJECT(&s->gcr), nc->disabled_modules,
+                            "disabled-modules", &error_abort);
+    object_property_set_link(OBJECT(&s->gcr), OBJECT(s->dram), "dram",
+                             &error_abort);
+    sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gcr), 0, NPCM7XX_GCR_BA);
+
+    /* Clock Control Registers (CLK) */
+    sysbus_realize(SYS_BUS_DEVICE(&s->clk), &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->clk), 0, NPCM7XX_CLK_BA);
+
+    /* Timer Modules (TIM) */
+    QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_tim_addr) != ARRAY_SIZE(s->tim));
+    for (i = 0; i < ARRAY_SIZE(s->tim); i++) {
+        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->tim[i]);
+        int first_irq;
+        int j;
+
+        sysbus_realize(sbd, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+        sysbus_mmio_map(sbd, 0, npcm7xx_tim_addr[i]);
+
+        first_irq = NPCM7XX_TIMER0_IRQ + i * NPCM7XX_TIMERS_PER_CTRL;
+        for (j = 0; j < NPCM7XX_TIMERS_PER_CTRL; j++) {
+            qemu_irq irq = npcm7xx_irq(s, first_irq + j);
+            sysbus_connect_irq(sbd, j, irq);
+        }
+    }
+
+    /* UART0..3 (16550 compatible) */
+    for (i = 0; i < ARRAY_SIZE(npcm7xx_uart_addr); i++) {
+        serial_mm_init(get_system_memory(), npcm7xx_uart_addr[i], 2,
+                       npcm7xx_irq(s, NPCM7XX_UART0_IRQ + i), 115200,
+                       serial_hd(i), DEVICE_LITTLE_ENDIAN);
+    }
+
+    /* RAM2 (SRAM) */
+    memory_region_init_ram(&s->sram, OBJECT(dev), "ram2",
+                           NPCM7XX_RAM2_SZ, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    memory_region_add_subregion(get_system_memory(), NPCM7XX_RAM2_BA, &s->sram);
+
+    /* Internal ROM */
+    memory_region_init_rom(&s->irom, OBJECT(dev), "irom", NPCM7XX_ROM_SZ, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    memory_region_add_subregion(get_system_memory(), NPCM7XX_ROM_BA, &s->irom);
+
+    /* External DDR4 SDRAM */
+    memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, s->dram);
+}
+
+static Property npcm7xx_properties[] = {
+    DEFINE_PROP_LINK("dram", NPCM7xxState, dram, TYPE_MEMORY_REGION,
+                     MemoryRegion *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void npcm7xx_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = npcm7xx_realize;
+    dc->user_creatable = false;
+    device_class_set_props(dc, npcm7xx_properties);
+}
+
+static void npcm730_class_init(ObjectClass *oc, void *data)
+{
+    NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
+
+    /* NPCM730 is optimized for data center use, so no graphics, etc. */
+    nc->disabled_modules = 0x00300395;
+    nc->num_cpus = 2;
+}
+
+static void npcm750_class_init(ObjectClass *oc, void *data)
+{
+    NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
+
+    /* NPCM750 has 2 cores and a full set of peripherals */
+    nc->disabled_modules = 0x00000000;
+    nc->num_cpus = 2;
+}
+
+static const TypeInfo npcm7xx_soc_types[] = {
+    {
+        .name           = TYPE_NPCM7XX,
+        .parent         = TYPE_DEVICE,
+        .instance_size  = sizeof(NPCM7xxState),
+        .instance_init  = npcm7xx_init,
+        .class_size     = sizeof(NPCM7xxClass),
+        .class_init     = npcm7xx_class_init,
+        .abstract       = true,
+    }, {
+        .name           = TYPE_NPCM730,
+        .parent         = TYPE_NPCM7XX,
+        .class_init     = npcm730_class_init,
+    }, {
+        .name           = TYPE_NPCM750,
+        .parent         = TYPE_NPCM7XX,
+        .class_init     = npcm750_class_init,
+    },
+};
+
+DEFINE_TYPES(npcm7xx_soc_types);
diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
new file mode 100644
index 0000000000..2ffa573b11
--- /dev/null
+++ b/include/hw/arm/npcm7xx.h
@@ -0,0 +1,81 @@ 
+/*
+ * Nuvoton NPCM7xx SoC family.
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+#ifndef NPCM7XX_H
+#define NPCM7XX_H
+
+#include "hw/boards.h"
+#include "hw/cpu/a9mpcore.h"
+#include "hw/misc/npcm7xx_clk.h"
+#include "hw/misc/npcm7xx_gcr.h"
+#include "hw/timer/npcm7xx_timer.h"
+#include "target/arm/cpu.h"
+
+#define NPCM7XX_MAX_NUM_CPUS    (2)
+
+/* Magic addresses for setting up direct kernel booting and SMP boot stubs. */
+#define NPCM7XX_LOADER_START            (0x00000000)  /* Start of SDRAM */
+#define NPCM7XX_SMP_LOADER_START        (0xFFFF0000)  /* Boot ROM */
+#define NPCM7XX_SMP_BOOTREG_ADDR        (0xF080013C)  /* GCR.SCRPAD */
+#define NPCM7XX_GIC_CPU_IF_ADDR         (0xF03FE100)  /* GIC within A9 */
+
+typedef struct NPCM7xxState {
+    DeviceState         parent;
+
+    ARMCPU              cpu[NPCM7XX_MAX_NUM_CPUS];
+    A9MPPrivState       a9mpcore;
+
+    MemoryRegion        sram;
+    MemoryRegion        irom;
+    MemoryRegion        *dram;
+
+    NPCM7xxGCRState     gcr;
+    NPCM7xxCLKState     clk;
+    NPCM7xxTimerCtrlState tim[3];
+} NPCM7xxState;
+
+#define TYPE_NPCM7XX    "npcm7xx"
+#define NPCM7XX(obj)    OBJECT_CHECK(NPCM7xxState, (obj), TYPE_NPCM7XX)
+
+#define TYPE_NPCM730    "npcm730"
+#define TYPE_NPCM750    "npcm750"
+
+typedef struct NPCM7xxClass {
+    DeviceClass         parent;
+
+    /* Bitmask of modules that are permanently disabled on this chip. */
+    uint32_t            disabled_modules;
+    /* Number of CPU cores enabled in this SoC class (may be 1 or 2). */
+    uint32_t            num_cpus;
+} NPCM7xxClass;
+
+#define NPCM7XX_CLASS(klass)                                            \
+    OBJECT_CLASS_CHECK(NPCM7xxClass, (klass), TYPE_NPCM7XX)
+#define NPCM7XX_GET_CLASS(obj)                                          \
+    OBJECT_GET_CLASS(NPCM7xxClass, (obj), TYPE_NPCM7XX)
+
+/**
+ * npcm7xx_write_secondary_boot - Write stub for booting secondary CPU.
+ * @cpu: The CPU to be booted.
+ * @info: Boot info structure for the board.
+ *
+ * This will write a short code stub to the internal ROM that will keep the
+ * secondary CPU spinning until the primary CPU writes an address to the SCRPAD
+ * register in the GCR, after which the secondary CPU will jump there.
+ */
+extern void npcm7xx_write_secondary_boot(ARMCPU *cpu,
+                                         const struct arm_boot_info *info);
+
+#endif /* NPCM7XX_H */