QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Niek Linnenbank <nieklinnenbank@gmail.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: b.galvani@gmail.com, Peter Maydell <peter.maydell@linaro.org>,
	qemu-arm@nongnu.org, qemu-devel@nongnu.org,
	Markus Armbruster <armbru@redhat.com>
Subject: Re: [PATCH 01/10] hw: arm: add Allwinner H3 System-on-Chip
Date: Wed, 4 Dec 2019 21:44:00 +0100
Message-ID: <CAPan3WpeYtA249jp2iF-baFXh2YihEYMiGVtov7DLpsOVb5ZhQ@mail.gmail.com> (raw)
In-Reply-To: <5d3961ca-8586-6d93-2525-fd2e29b233e1@redhat.com>

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

Hello Philippe,

On Wed, Dec 4, 2019 at 5:53 PM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Hi Niek,
>
> On 12/2/19 10:09 PM, Niek Linnenbank wrote:
> > The Allwinner H3 is a System on Chip containing four ARM Cortex A7
> > processor cores. Features and specifications include DDR2/DDR3 memory,
> > SD/MMC storage cards, 10/100/1000Mbit ethernet, USB 2.0, HDMI and
> > various I/O modules. This commit adds support for the Allwinner H3
> > System on Chip.
> >
> > Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> > ---
> >   MAINTAINERS                     |   7 ++
> >   default-configs/arm-softmmu.mak |   1 +
> >   hw/arm/Kconfig                  |   8 ++
> >   hw/arm/Makefile.objs            |   1 +
> >   hw/arm/allwinner-h3.c           | 215 ++++++++++++++++++++++++++++++++
> >   include/hw/arm/allwinner-h3.h   | 118 ++++++++++++++++++
> >   6 files changed, 350 insertions(+)
> >   create mode 100644 hw/arm/allwinner-h3.c
> >   create mode 100644 include/hw/arm/allwinner-h3.h
>
> Since your series changes various files, can you have a look at the
> scripts/git.orderfile file and setup it for your QEMU contributions?
>

OK, done! I didn't know such a script existed, thanks.
I ran this command in my local repository:
 $ git config diff.orderFile scripts/git.orderfile
It seems to work, when I re-generate the patches, the order of the diff is
different.



> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5e5e3e52d6..29c9936037 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -479,6 +479,13 @@ F: hw/*/allwinner*
> >   F: include/hw/*/allwinner*
> >   F: hw/arm/cubieboard.c
> >
> > +Allwinner-h3
> > +M: Niek Linnenbank <nieklinnenbank@gmail.com>
> > +L: qemu-arm@nongnu.org
> > +S: Maintained
> > +F: hw/*/allwinner-h3*
> > +F: include/hw/*/allwinner-h3*
> > +
> >   ARM PrimeCell and CMSDK devices
> >   M: Peter Maydell <peter.maydell@linaro.org>
> >   L: qemu-arm@nongnu.org
> > diff --git a/default-configs/arm-softmmu.mak
> b/default-configs/arm-softmmu.mak
> > index 1f2e0e7fde..d75a239c2c 100644
> > --- a/default-configs/arm-softmmu.mak
> > +++ b/default-configs/arm-softmmu.mak
> > @@ -40,3 +40,4 @@ CONFIG_FSL_IMX25=y
> >   CONFIG_FSL_IMX7=y
> >   CONFIG_FSL_IMX6UL=y
> >   CONFIG_SEMIHOSTING=y
> > +CONFIG_ALLWINNER_H3=y
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index c6e7782580..ebf8d2325f 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -291,6 +291,14 @@ config ALLWINNER_A10
> >       select SERIAL
> >       select UNIMP
> >
> > +config ALLWINNER_H3
> > +    bool
> > +    select ALLWINNER_A10_PIT
> > +    select SERIAL
> > +    select ARM_TIMER
> > +    select ARM_GIC
> > +    select UNIMP
> > +
> >   config RASPI
> >       bool
> >       select FRAMEBUFFER
> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> > index fe749f65fd..956e496052 100644
> > --- a/hw/arm/Makefile.objs
> > +++ b/hw/arm/Makefile.objs
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_DIGIC) += digic.o
> >   obj-$(CONFIG_OMAP) += omap1.o omap2.o
> >   obj-$(CONFIG_STRONGARM) += strongarm.o
> >   obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
> > +obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3.o
> >   obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
> >   obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
> >   obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o
> > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> > new file mode 100644
> > index 0000000000..470fdfebef
> > --- /dev/null
> > +++ b/hw/arm/allwinner-h3.c
> > @@ -0,0 +1,215 @@
> > +/*
> > + * Allwinner H3 System on Chip emulation
> > + *
> > + * Copyright (C) 2019 Niek Linnenbank <nieklinnenbank@gmail.com>
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/
> >.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "exec/address-spaces.h"
> > +#include "qapi/error.h"
> > +#include "qemu/module.h"
> > +#include "qemu/units.h"
> > +#include "cpu.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/arm/allwinner-h3.h"
> > +#include "hw/misc/unimp.h"
> > +#include "sysemu/sysemu.h"
> > +
> > +static void aw_h3_init(Object *obj)
> > +{
> > +    AwH3State *s = AW_H3(obj);
> > +
> > +    sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic),
> > +                          TYPE_ARM_GIC);
> > +
> > +    sysbus_init_child_obj(obj, "timer", &s->timer, sizeof(s->timer),
> > +                          TYPE_AW_A10_PIT);
> > +}
> > +
> > +static void aw_h3_realize(DeviceState *dev, Error **errp)
> > +{
> > +    AwH3State *s = AW_H3(dev);
> > +    SysBusDevice *sysbusdev = NULL;
> > +    Error *err = NULL;
> > +    unsigned i = 0;
> > +
> > +    /* CPUs */
> > +    for (i = 0; i < AW_H3_NUM_CPUS; i++) {
>
> In https://www.mail-archive.com/qemu-devel@nongnu.org/msg662942.html
> Markus noted some incorrect pattern, and apparently you inherited it.
> You should initialize 'err' in the loop.
>
> > +        Object *cpuobj = object_new(ARM_CPU_TYPE_NAME("cortex-a7"));
> > +        CPUState *cpustate = CPU(cpuobj);
>
> We loose access to the CPUs. Can you use an array of AW_H3_NUM_CPUS cpus
> in AwH3State?
>
> > +
> > +        /* Set the proper CPU index */
> > +        cpustate->cpu_index = i;
> > +
> > +        /* Provide Power State Coordination Interface */
> > +        object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_HVC,
> > +                                "psci-conduit", &error_abort);
>
> Here you use the error_abort shortcut.
>
> > +
> > +        /* Disable secondary CPUs */
> > +        object_property_set_bool(cpuobj, i > 0, "start-powered-off",
> &err);
> > +        if (err != NULL) {
> > +            error_propagate(errp, err);
> > +            return;
>
> Here you return.
>
> > +        }
> > +
> > +        /* All exception levels required */
> > +        object_property_set_bool(cpuobj,
> > +                                 true, "has_el3", NULL);
> > +        object_property_set_bool(cpuobj,
> > +                                 true, "has_el2", NULL);
>
> Here you don't use error.
>
> Cc'ing Markus who is the expert, since he might have better suggestions.
>
> This function is called before the machine starts, and we are not
> handling with user-provided configurations, so I'd say using
> &error_abort in all places is OK.
>
> > +
> > +        /* Mark realized */
> > +        object_property_set_bool(cpuobj, true, "realized", &err);
> > +        if (err != NULL) {
> > +            error_propagate(errp, err);
> > +            return;
> > +        }
> > +        object_unref(cpuobj);
> > +    }
> > +
> > +    /* Generic Interrupt Controller */
> > +    qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", AW_H3_GIC_NUM_SPI +
> > +                                                     GIC_INTERNAL);
> > +    qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
> > +    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", AW_H3_NUM_CPUS);
> > +    qdev_prop_set_bit(DEVICE(&s->gic), "has-security-extensions",
> false);
> > +    qdev_prop_set_bit(DEVICE(&s->gic), "has-virtualization-extensions",
> true);
> > +
> > +    object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
>
> Why change API? Can we use qdev_init_nofail() instead?
>

> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +
> > +    sysbusdev = SYS_BUS_DEVICE(&s->gic);
> > +    sysbus_mmio_map(sysbusdev, 0, AW_H3_GIC_DIST_BASE);
> > +    sysbus_mmio_map(sysbusdev, 1, AW_H3_GIC_CPU_BASE);
> > +    sysbus_mmio_map(sysbusdev, 2, AW_H3_GIC_HYP_BASE);
> > +    sysbus_mmio_map(sysbusdev, 3, AW_H3_GIC_VCPU_BASE);
> > +
> > +    /*
> > +     * Wire the outputs from each CPU's generic timer and the GICv3
> > +     * maintenance interrupt signal to the appropriate GIC PPI inputs,
> > +     * and the GIC's IRQ/FIQ/VIRQ/VFIQ interrupt outputs to the CPU's
> inputs.
> > +     */
> > +    for (i = 0; i < AW_H3_NUM_CPUS; i++) {
> > +        DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
> > +        int ppibase = AW_H3_GIC_NUM_SPI + i * GIC_INTERNAL +
> GIC_NR_SGIS;
> > +        int irq;
> > +        /*
> > +         * Mapping from the output timer irq lines from the CPU to the
> > +         * GIC PPI inputs used for this board.
> > +         */
> > +        const int timer_irq[] = {
> > +            [GTIMER_PHYS] = AW_H3_GIC_PPI_ARM_PHYSTIMER,
> > +            [GTIMER_VIRT] = AW_H3_GIC_PPI_ARM_VIRTTIMER,
> > +            [GTIMER_HYP]  = AW_H3_GIC_PPI_ARM_HYPTIMER,
> > +            [GTIMER_SEC]  = AW_H3_GIC_PPI_ARM_SECTIMER,
> > +        };
> > +
> > +        /* Connect CPU timer outputs to GIC PPI inputs */
> > +        for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
> > +            qdev_connect_gpio_out(cpudev, irq,
> > +                                  qdev_get_gpio_in(DEVICE(&s->gic),
> > +                                                   ppibase +
> timer_irq[irq]));
> > +        }
> > +
> > +        /* Connect GIC outputs to CPU interrupt inputs */
> > +        sysbus_connect_irq(sysbusdev, i,
> > +                           qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
> > +        sysbus_connect_irq(sysbusdev, i + AW_H3_NUM_CPUS,
> > +                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
> > +        sysbus_connect_irq(sysbusdev, i + (2 * AW_H3_NUM_CPUS),
> > +                           qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
> > +        sysbus_connect_irq(sysbusdev, i + (3 * AW_H3_NUM_CPUS),
> > +                           qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
> > +
> > +        /* GIC maintenance signal */
> > +        sysbus_connect_irq(sysbusdev, i + (4 * AW_H3_NUM_CPUS),
> > +                           qdev_get_gpio_in(DEVICE(&s->gic),
> > +                                            ppibase +
> AW_H3_GIC_PPI_MAINT));
> > +    }
> > +
> > +    for (i = 0; i < AW_H3_GIC_NUM_SPI; i++) {
> > +        s->irq[i] = qdev_get_gpio_in(DEVICE(&s->gic), i);
>
> Apparently we don't need the irq array in AwH3State, because ...
>
> > +    }
> > +
> > +    /* Timer */
> > +    object_property_set_bool(OBJECT(&s->timer), true, "realized", &err);
> > +    if (err != NULL) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +    sysbusdev = SYS_BUS_DEVICE(&s->timer);
> > +    sysbus_mmio_map(sysbusdev, 0, AW_H3_PIT_REG_BASE);
> > +    sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_TIMER0]);
> > +    sysbus_connect_irq(sysbusdev, 1, s->irq[AW_H3_GIC_SPI_TIMER1]);
>
> ... we can call qdev_get_gpio_in() here directly.
>
> > +
> > +    /* SRAM */
> > +    memory_region_init_ram(&s->sram_a1, OBJECT(dev), "sram A1",
> > +                            AW_H3_SRAM_A1_SIZE, &error_fatal);
> > +    memory_region_init_ram(&s->sram_a2, OBJECT(dev), "sram A2",
> > +                            AW_H3_SRAM_A2_SIZE, &error_fatal);
> > +    memory_region_init_ram(&s->sram_c, OBJECT(dev), "sram C",
> > +                            AW_H3_SRAM_C_SIZE, &error_fatal);
> > +    memory_region_add_subregion(get_system_memory(), AW_H3_SRAM_A1_BASE,
> > +                                &s->sram_a1);
> > +    memory_region_add_subregion(get_system_memory(), AW_H3_SRAM_A2_BASE,
> > +                                &s->sram_a2);
> > +    memory_region_add_subregion(get_system_memory(), AW_H3_SRAM_C_BASE,
> > +                                &s->sram_c);
> > +
> > +    /* UART */
> > +    if (serial_hd(0)) {
> > +        serial_mm_init(get_system_memory(), AW_H3_UART0_REG_BASE, 2,
> > +                       s->irq[AW_H3_GIC_SPI_UART0], 115200,
> serial_hd(0),
>
> qdev_get_gpio_in() here too.
>
> > +                       DEVICE_NATIVE_ENDIAN);
> > +    }
> > +
> > +    /* Unimplemented devices */
> > +    create_unimplemented_device("display-engine", AW_H3_DE_BASE,
> AW_H3_DE_SIZE);
> > +    create_unimplemented_device("dma", AW_H3_DMA_BASE, AW_H3_DMA_SIZE);
> > +    create_unimplemented_device("lcd0", AW_H3_LCD0_BASE,
> AW_H3_LCD0_SIZE);
> > +    create_unimplemented_device("lcd1", AW_H3_LCD1_BASE,
> AW_H3_LCD1_SIZE);
> > +    create_unimplemented_device("gpu", AW_H3_GPU_BASE, AW_H3_GPU_SIZE);
> > +    create_unimplemented_device("hdmi", AW_H3_HDMI_BASE,
> AW_H3_HDMI_SIZE);
> > +    create_unimplemented_device("rtc", AW_H3_RTC_BASE, AW_H3_RTC_SIZE);
> > +    create_unimplemented_device("audio-codec", AW_H3_AC_BASE,
> AW_H3_AC_SIZE);
> > +}
> > +
> > +static void aw_h3_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +
> > +    dc->realize = aw_h3_realize;
> > +    /* Reason: uses serial_hds and nd_table */
> > +    dc->user_creatable = false;
> > +}
> > +
> > +static const TypeInfo aw_h3_type_info = {
> > +    .name = TYPE_AW_H3,
> > +    .parent = TYPE_DEVICE,
> > +    .instance_size = sizeof(AwH3State),
> > +    .instance_init = aw_h3_init,
> > +    .class_init = aw_h3_class_init,
> > +};
> > +
> > +static void aw_h3_register_types(void)
> > +{
> > +    type_register_static(&aw_h3_type_info);
> > +}
> > +
> > +type_init(aw_h3_register_types)
> > diff --git a/include/hw/arm/allwinner-h3.h
> b/include/hw/arm/allwinner-h3.h
> > new file mode 100644
> > index 0000000000..af368c2254
> > --- /dev/null
> > +++ b/include/hw/arm/allwinner-h3.h
> > @@ -0,0 +1,118 @@
> > +/*
> > + * Allwinner H3 System on Chip emulation
> > + *
> > + * Copyright (C) 2019 Niek Linnenbank <nieklinnenbank@gmail.com>
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/
> >.
> > + */
> > +
> > +#ifndef HW_ARM_ALLWINNER_H3_H
> > +#define HW_ARM_ALLWINNER_H3_H
> > +
> > +#include "qemu/error-report.h"
> > +#include "qemu/units.h"
> > +#include "hw/char/serial.h"
> > +#include "hw/arm/boot.h"
> > +#include "hw/timer/allwinner-a10-pit.h"
> > +#include "hw/intc/arm_gic.h"
> > +#include "target/arm/cpu.h"
> > +
> > +#define AW_H3_SRAM_A1_BASE     (0x00000000)
> > +#define AW_H3_SRAM_A2_BASE     (0x00044000)
> > +#define AW_H3_SRAM_C_BASE      (0x00010000)
> > +#define AW_H3_DE_BASE          (0x01000000)
> > +#define AW_H3_SYSCON_BASE      (0x01c00000)
> > +#define AW_H3_DMA_BASE         (0x01c02000)
> > +#define AW_H3_LCD0_BASE        (0x01c0c000)
> > +#define AW_H3_LCD1_BASE        (0x01c0d000)
> > +#define AW_H3_SID_BASE         (0x01c14000)
> > +#define AW_H3_CCU_BASE         (0x01c20000)
> > +#define AW_H3_PIC_REG_BASE     (0x01c20400)
> > +#define AW_H3_PIT_REG_BASE     (0x01c20c00)
> > +#define AW_H3_AC_BASE          (0x01c22c00)
> > +#define AW_H3_UART0_REG_BASE   (0x01c28000)
> > +#define AW_H3_EMAC_BASE        (0x01c30000)
> > +#define AW_H3_MMC0_BASE        (0x01c0f000)
> > +#define AW_H3_EHCI0_BASE       (0x01c1a000)
> > +#define AW_H3_OHCI0_BASE       (0x01c1a400)
> > +#define AW_H3_EHCI1_BASE       (0x01c1b000)
> > +#define AW_H3_OHCI1_BASE       (0x01c1b400)
> > +#define AW_H3_EHCI2_BASE       (0x01c1c000)
> > +#define AW_H3_OHCI2_BASE       (0x01c1c400)
> > +#define AW_H3_EHCI3_BASE       (0x01c1d000)
> > +#define AW_H3_OHCI3_BASE       (0x01c1d400)
> > +#define AW_H3_GPU_BASE         (0x01c40000)
> > +#define AW_H3_GIC_DIST_BASE    (0x01c81000)
> > +#define AW_H3_GIC_CPU_BASE     (0x01c82000)
> > +#define AW_H3_GIC_HYP_BASE     (0x01c84000)
> > +#define AW_H3_GIC_VCPU_BASE    (0x01c86000)
> > +#define AW_H3_HDMI_BASE        (0x01ee0000)
> > +#define AW_H3_RTC_BASE         (0x01f00000)
> > +#define AW_H3_CPUCFG_BASE      (0x01f01c00)
> > +#define AW_H3_SDRAM_BASE       (0x40000000)
> > +
> > +#define AW_H3_SRAM_A1_SIZE     (64 * KiB)
> > +#define AW_H3_SRAM_A2_SIZE     (32 * KiB)
> > +#define AW_H3_SRAM_C_SIZE      (44 * KiB)
> > +#define AW_H3_DE_SIZE          (4 * MiB)
> > +#define AW_H3_DMA_SIZE         (4 * KiB)
> > +#define AW_H3_LCD0_SIZE        (4 * KiB)
> > +#define AW_H3_LCD1_SIZE        (4 * KiB)
> > +#define AW_H3_GPU_SIZE         (64 * KiB)
> > +#define AW_H3_HDMI_SIZE        (128 * KiB)
> > +#define AW_H3_RTC_SIZE         (1 * KiB)
> > +#define AW_H3_AC_SIZE          (2 * KiB)
> > +
> > +#define AW_H3_GIC_PPI_MAINT          (9)
> > +#define AW_H3_GIC_PPI_ARM_HYPTIMER  (10)
> > +#define AW_H3_GIC_PPI_ARM_VIRTTIMER (11)
> > +#define AW_H3_GIC_PPI_ARM_SECTIMER  (13)
> > +#define AW_H3_GIC_PPI_ARM_PHYSTIMER (14)
> > +
> > +#define AW_H3_GIC_SPI_UART0         (0)
> > +#define AW_H3_GIC_SPI_TIMER0        (18)
> > +#define AW_H3_GIC_SPI_TIMER1        (19)
> > +#define AW_H3_GIC_SPI_MMC0          (60)
> > +#define AW_H3_GIC_SPI_MMC1          (61)
> > +#define AW_H3_GIC_SPI_MMC2          (62)
> > +#define AW_H3_GIC_SPI_EHCI0         (72)
> > +#define AW_H3_GIC_SPI_OHCI0         (73)
> > +#define AW_H3_GIC_SPI_EHCI1         (74)
> > +#define AW_H3_GIC_SPI_OHCI1         (75)
> > +#define AW_H3_GIC_SPI_EHCI2         (76)
> > +#define AW_H3_GIC_SPI_OHCI2         (77)
> > +#define AW_H3_GIC_SPI_EHCI3         (78)
> > +#define AW_H3_GIC_SPI_OHCI3         (79)
> > +#define AW_H3_GIC_SPI_EMAC          (82)
>
> I'd move half of the previous definitions into allwinner-h3.c, since
> they are only used there.
>
> Indeed, you are right, I'll move them.

Also, I'd use an enum for the PPI/SPI.
>

Thanks, I will process all of your comments above for the next patch
version.


>
>
> +
> > +#define AW_H3_GIC_NUM_SPI           (128)
> > +#define AW_H3_NUM_CPUS              (4)
> > +
> > +#define TYPE_AW_H3 "allwinner-h3"
> > +#define AW_H3(obj) OBJECT_CHECK(AwH3State, (obj), TYPE_AW_H3)
> > +
> > +typedef struct AwH3State {
> > +    /*< private >*/
> > +    DeviceState parent_obj;
> > +    /*< public >*/
> > +
> > +    qemu_irq irq[AW_H3_GIC_NUM_SPI];
> > +    AwA10PITState timer;
> > +    GICState gic;
> > +    MemoryRegion sram_a1;
> > +    MemoryRegion sram_a2;
> > +    MemoryRegion sram_c;
> > +} AwH3State;
> > +
> > +#endif
> >
>
> Nice clean patch, for a first contribution :)
>

Thank you Philippe!

Regards,
Niek

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 25602 bytes --]

<div dir="ltr"><div>Hello Philippe,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Dec 4, 2019 at 5:53 PM Philippe Mathieu-Daudé &lt;<a href="mailto:philmd@redhat.com">philmd@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Niek,<br>
<br>
On 12/2/19 10:09 PM, Niek Linnenbank wrote:<br>
&gt; The Allwinner H3 is a System on Chip containing four ARM Cortex A7<br>
&gt; processor cores. Features and specifications include DDR2/DDR3 memory,<br>
&gt; SD/MMC storage cards, 10/100/1000Mbit ethernet, USB 2.0, HDMI and<br>
&gt; various I/O modules. This commit adds support for the Allwinner H3<br>
&gt; System on Chip.<br>
&gt; <br>
&gt; Signed-off-by: Niek Linnenbank &lt;<a href="mailto:nieklinnenbank@gmail.com" target="_blank">nieklinnenbank@gmail.com</a>&gt;<br>
&gt; ---<br>
&gt;   MAINTAINERS                     |   7 ++<br>
&gt;   default-configs/arm-softmmu.mak |   1 +<br>
&gt;   hw/arm/Kconfig                  |   8 ++<br>
&gt;   hw/arm/Makefile.objs            |   1 +<br>
&gt;   hw/arm/allwinner-h3.c           | 215 ++++++++++++++++++++++++++++++++<br>
&gt;   include/hw/arm/allwinner-h3.h   | 118 ++++++++++++++++++<br>
&gt;   6 files changed, 350 insertions(+)<br>
&gt;   create mode 100644 hw/arm/allwinner-h3.c<br>
&gt;   create mode 100644 include/hw/arm/allwinner-h3.h<br>
<br>
Since your series changes various files, can you have a look at the <br>
scripts/git.orderfile file and setup it for your QEMU contributions?<br></blockquote><div><br></div><div>OK, done! I didn&#39;t know such a script existed, thanks.</div><div>I ran this command in my local repository:</div><div> $ git config diff.orderFile scripts/git.orderfile<br></div><div>It seems to work, when I re-generate the patches, the order of the diff is different.<br></div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
&gt; <br>
&gt; diff --git a/MAINTAINERS b/MAINTAINERS<br>
&gt; index 5e5e3e52d6..29c9936037 100644<br>
&gt; --- a/MAINTAINERS<br>
&gt; +++ b/MAINTAINERS<br>
&gt; @@ -479,6 +479,13 @@ F: hw/*/allwinner*<br>
&gt;   F: include/hw/*/allwinner*<br>
&gt;   F: hw/arm/cubieboard.c<br>
&gt;   <br>
&gt; +Allwinner-h3<br>
&gt; +M: Niek Linnenbank &lt;<a href="mailto:nieklinnenbank@gmail.com" target="_blank">nieklinnenbank@gmail.com</a>&gt;<br>
&gt; +L: <a href="mailto:qemu-arm@nongnu.org" target="_blank">qemu-arm@nongnu.org</a><br>
&gt; +S: Maintained<br>
&gt; +F: hw/*/allwinner-h3*<br>
&gt; +F: include/hw/*/allwinner-h3*<br>
&gt; +<br>
&gt;   ARM PrimeCell and CMSDK devices<br>
&gt;   M: Peter Maydell &lt;<a href="mailto:peter.maydell@linaro.org" target="_blank">peter.maydell@linaro.org</a>&gt;<br>
&gt;   L: <a href="mailto:qemu-arm@nongnu.org" target="_blank">qemu-arm@nongnu.org</a><br>
&gt; diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak<br>
&gt; index 1f2e0e7fde..d75a239c2c 100644<br>
&gt; --- a/default-configs/arm-softmmu.mak<br>
&gt; +++ b/default-configs/arm-softmmu.mak<br>
&gt; @@ -40,3 +40,4 @@ CONFIG_FSL_IMX25=y<br>
&gt;   CONFIG_FSL_IMX7=y<br>
&gt;   CONFIG_FSL_IMX6UL=y<br>
&gt;   CONFIG_SEMIHOSTING=y<br>
&gt; +CONFIG_ALLWINNER_H3=y<br>
&gt; diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig<br>
&gt; index c6e7782580..ebf8d2325f 100644<br>
&gt; --- a/hw/arm/Kconfig<br>
&gt; +++ b/hw/arm/Kconfig<br>
&gt; @@ -291,6 +291,14 @@ config ALLWINNER_A10<br>
&gt;       select SERIAL<br>
&gt;       select UNIMP<br>
&gt;   <br>
&gt; +config ALLWINNER_H3<br>
&gt; +    bool<br>
&gt; +    select ALLWINNER_A10_PIT<br>
&gt; +    select SERIAL<br>
&gt; +    select ARM_TIMER<br>
&gt; +    select ARM_GIC<br>
&gt; +    select UNIMP<br>
&gt; +<br>
&gt;   config RASPI<br>
&gt;       bool<br>
&gt;       select FRAMEBUFFER<br>
&gt; diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs<br>
&gt; index fe749f65fd..956e496052 100644<br>
&gt; --- a/hw/arm/Makefile.objs<br>
&gt; +++ b/hw/arm/Makefile.objs<br>
&gt; @@ -34,6 +34,7 @@ obj-$(CONFIG_DIGIC) += digic.o<br>
&gt;   obj-$(CONFIG_OMAP) += omap1.o omap2.o<br>
&gt;   obj-$(CONFIG_STRONGARM) += strongarm.o<br>
&gt;   obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o<br>
&gt; +obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3.o<br>
&gt;   obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o<br>
&gt;   obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o<br>
&gt;   obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o<br>
&gt; diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c<br>
&gt; new file mode 100644<br>
&gt; index 0000000000..470fdfebef<br>
&gt; --- /dev/null<br>
&gt; +++ b/hw/arm/allwinner-h3.c<br>
&gt; @@ -0,0 +1,215 @@<br>
&gt; +/*<br>
&gt; + * Allwinner H3 System on Chip emulation<br>
&gt; + *<br>
&gt; + * Copyright (C) 2019 Niek Linnenbank &lt;<a href="mailto:nieklinnenbank@gmail.com" target="_blank">nieklinnenbank@gmail.com</a>&gt;<br>
&gt; + *<br>
&gt; + * This program is free software: you can redistribute it and/or modify<br>
&gt; + * it under the terms of the GNU General Public License as published by<br>
&gt; + * the Free Software Foundation, either version 2 of the License, or<br>
&gt; + * (at your option) any later version.<br>
&gt; + *<br>
&gt; + * This program is distributed in the hope that it will be useful,<br>
&gt; + * but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
&gt; + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the<br>
&gt; + * GNU General Public License for more details.<br>
&gt; + *<br>
&gt; + * You should have received a copy of the GNU General Public License<br>
&gt; + * along with this program.  If not, see &lt;<a href="http://www.gnu.org/licenses/" rel="noreferrer" target="_blank">http://www.gnu.org/licenses/</a>&gt;.<br>
&gt; + */<br>
&gt; +<br>
&gt; +#include &quot;qemu/osdep.h&quot;<br>
&gt; +#include &quot;exec/address-spaces.h&quot;<br>
&gt; +#include &quot;qapi/error.h&quot;<br>
&gt; +#include &quot;qemu/module.h&quot;<br>
&gt; +#include &quot;qemu/units.h&quot;<br>
&gt; +#include &quot;cpu.h&quot;<br>
&gt; +#include &quot;hw/sysbus.h&quot;<br>
&gt; +#include &quot;hw/arm/allwinner-h3.h&quot;<br>
&gt; +#include &quot;hw/misc/unimp.h&quot;<br>
&gt; +#include &quot;sysemu/sysemu.h&quot;<br>
&gt; +<br>
&gt; +static void aw_h3_init(Object *obj)<br>
&gt; +{<br>
&gt; +    AwH3State *s = AW_H3(obj);<br>
&gt; +<br>
&gt; +    sysbus_init_child_obj(obj, &quot;gic&quot;, &amp;s-&gt;gic, sizeof(s-&gt;gic),<br>
&gt; +                          TYPE_ARM_GIC);<br>
&gt; +<br>
&gt; +    sysbus_init_child_obj(obj, &quot;timer&quot;, &amp;s-&gt;timer, sizeof(s-&gt;timer),<br>
&gt; +                          TYPE_AW_A10_PIT);<br>
&gt; +}<br>
&gt; +<br>
&gt; +static void aw_h3_realize(DeviceState *dev, Error **errp)<br>
&gt; +{<br>
&gt; +    AwH3State *s = AW_H3(dev);<br>
&gt; +    SysBusDevice *sysbusdev = NULL;<br>
&gt; +    Error *err = NULL;<br>
&gt; +    unsigned i = 0;<br>
&gt; +<br>
&gt; +    /* CPUs */<br>
&gt; +    for (i = 0; i &lt; AW_H3_NUM_CPUS; i++) {<br>
<br>
In <a href="https://www.mail-archive.com/qemu-devel@nongnu.org/msg662942.html" rel="noreferrer" target="_blank">https://www.mail-archive.com/qemu-devel@nongnu.org/msg662942.html</a><br>
Markus noted some incorrect pattern, and apparently you inherited it.<br>
You should initialize &#39;err&#39; in the loop.<br>
<br>
&gt; +        Object *cpuobj = object_new(ARM_CPU_TYPE_NAME(&quot;cortex-a7&quot;));<br>
&gt; +        CPUState *cpustate = CPU(cpuobj);<br>
<br>
We loose access to the CPUs. Can you use an array of AW_H3_NUM_CPUS cpus <br>
in AwH3State?<br>
<br>
&gt; +<br>
&gt; +        /* Set the proper CPU index */<br>
&gt; +        cpustate-&gt;cpu_index = i;<br>
&gt; +<br>
&gt; +        /* Provide Power State Coordination Interface */<br>
&gt; +        object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_HVC,<br>
&gt; +                                &quot;psci-conduit&quot;, &amp;error_abort);<br>
<br>
Here you use the error_abort shortcut.<br>
<br>
&gt; +<br>
&gt; +        /* Disable secondary CPUs */<br>
&gt; +        object_property_set_bool(cpuobj, i &gt; 0, &quot;start-powered-off&quot;, &amp;err);<br>
&gt; +        if (err != NULL) {<br>
&gt; +            error_propagate(errp, err);<br>
&gt; +            return;<br>
<br>
Here you return.<br>
<br>
&gt; +        }<br>
&gt; +<br>
&gt; +        /* All exception levels required */<br>
&gt; +        object_property_set_bool(cpuobj,<br>
&gt; +                                 true, &quot;has_el3&quot;, NULL);<br>
&gt; +        object_property_set_bool(cpuobj,<br>
&gt; +                                 true, &quot;has_el2&quot;, NULL);<br>
<br>
Here you don&#39;t use error.<br>
<br>
Cc&#39;ing Markus who is the expert, since he might have better suggestions.<br>
<br>
This function is called before the machine starts, and we are not <br>
handling with user-provided configurations, so I&#39;d say using <br>
&amp;error_abort in all places is OK.<br>
<br>
&gt; +<br>
&gt; +        /* Mark realized */<br>
&gt; +        object_property_set_bool(cpuobj, true, &quot;realized&quot;, &amp;err);<br>
&gt; +        if (err != NULL) {<br>
&gt; +            error_propagate(errp, err);<br>
&gt; +            return;<br>
&gt; +        }<br>
&gt; +        object_unref(cpuobj);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    /* Generic Interrupt Controller */<br>
&gt; +    qdev_prop_set_uint32(DEVICE(&amp;s-&gt;gic), &quot;num-irq&quot;, AW_H3_GIC_NUM_SPI +<br>
&gt; +                                                     GIC_INTERNAL);<br>
&gt; +    qdev_prop_set_uint32(DEVICE(&amp;s-&gt;gic), &quot;revision&quot;, 2);<br>
&gt; +    qdev_prop_set_uint32(DEVICE(&amp;s-&gt;gic), &quot;num-cpu&quot;, AW_H3_NUM_CPUS);<br>
&gt; +    qdev_prop_set_bit(DEVICE(&amp;s-&gt;gic), &quot;has-security-extensions&quot;, false);<br>
&gt; +    qdev_prop_set_bit(DEVICE(&amp;s-&gt;gic), &quot;has-virtualization-extensions&quot;, true);<br>
&gt; +<br>
&gt; +    object_property_set_bool(OBJECT(&amp;s-&gt;gic), true, &quot;realized&quot;, &amp;err);<br>
<br>
Why change API? Can we use qdev_init_nofail() instead?<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
&gt; +    if (err) {<br>
&gt; +        error_propagate(errp, err);<br>
&gt; +        return;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    sysbusdev = SYS_BUS_DEVICE(&amp;s-&gt;gic);<br>
&gt; +    sysbus_mmio_map(sysbusdev, 0, AW_H3_GIC_DIST_BASE);<br>
&gt; +    sysbus_mmio_map(sysbusdev, 1, AW_H3_GIC_CPU_BASE);<br>
&gt; +    sysbus_mmio_map(sysbusdev, 2, AW_H3_GIC_HYP_BASE);<br>
&gt; +    sysbus_mmio_map(sysbusdev, 3, AW_H3_GIC_VCPU_BASE);<br>
&gt; +<br>
&gt; +    /*<br>
&gt; +     * Wire the outputs from each CPU&#39;s generic timer and the GICv3<br>
&gt; +     * maintenance interrupt signal to the appropriate GIC PPI inputs,<br>
&gt; +     * and the GIC&#39;s IRQ/FIQ/VIRQ/VFIQ interrupt outputs to the CPU&#39;s inputs.<br>
&gt; +     */<br>
&gt; +    for (i = 0; i &lt; AW_H3_NUM_CPUS; i++) {<br>
&gt; +        DeviceState *cpudev = DEVICE(qemu_get_cpu(i));<br>
&gt; +        int ppibase = AW_H3_GIC_NUM_SPI + i * GIC_INTERNAL + GIC_NR_SGIS;<br>
&gt; +        int irq;<br>
&gt; +        /*<br>
&gt; +         * Mapping from the output timer irq lines from the CPU to the<br>
&gt; +         * GIC PPI inputs used for this board.<br>
&gt; +         */<br>
&gt; +        const int timer_irq[] = {<br>
&gt; +            [GTIMER_PHYS] = AW_H3_GIC_PPI_ARM_PHYSTIMER,<br>
&gt; +            [GTIMER_VIRT] = AW_H3_GIC_PPI_ARM_VIRTTIMER,<br>
&gt; +            [GTIMER_HYP]  = AW_H3_GIC_PPI_ARM_HYPTIMER,<br>
&gt; +            [GTIMER_SEC]  = AW_H3_GIC_PPI_ARM_SECTIMER,<br>
&gt; +        };<br>
&gt; +<br>
&gt; +        /* Connect CPU timer outputs to GIC PPI inputs */<br>
&gt; +        for (irq = 0; irq &lt; ARRAY_SIZE(timer_irq); irq++) {<br>
&gt; +            qdev_connect_gpio_out(cpudev, irq,<br>
&gt; +                                  qdev_get_gpio_in(DEVICE(&amp;s-&gt;gic),<br>
&gt; +                                                   ppibase + timer_irq[irq]));<br>
&gt; +        }<br>
&gt; +<br>
&gt; +        /* Connect GIC outputs to CPU interrupt inputs */<br>
&gt; +        sysbus_connect_irq(sysbusdev, i,<br>
&gt; +                           qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));<br>
&gt; +        sysbus_connect_irq(sysbusdev, i + AW_H3_NUM_CPUS,<br>
&gt; +                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));<br>
&gt; +        sysbus_connect_irq(sysbusdev, i + (2 * AW_H3_NUM_CPUS),<br>
&gt; +                           qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));<br>
&gt; +        sysbus_connect_irq(sysbusdev, i + (3 * AW_H3_NUM_CPUS),<br>
&gt; +                           qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));<br>
&gt; +<br>
&gt; +        /* GIC maintenance signal */<br>
&gt; +        sysbus_connect_irq(sysbusdev, i + (4 * AW_H3_NUM_CPUS),<br>
&gt; +                           qdev_get_gpio_in(DEVICE(&amp;s-&gt;gic),<br>
&gt; +                                            ppibase + AW_H3_GIC_PPI_MAINT));<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    for (i = 0; i &lt; AW_H3_GIC_NUM_SPI; i++) {<br>
&gt; +        s-&gt;irq[i] = qdev_get_gpio_in(DEVICE(&amp;s-&gt;gic), i);<br>
<br>
Apparently we don&#39;t need the irq array in AwH3State, because ...<br>
<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    /* Timer */<br>
&gt; +    object_property_set_bool(OBJECT(&amp;s-&gt;timer), true, &quot;realized&quot;, &amp;err);<br>
&gt; +    if (err != NULL) {<br>
&gt; +        error_propagate(errp, err);<br>
&gt; +        return;<br>
&gt; +    }<br>
&gt; +    sysbusdev = SYS_BUS_DEVICE(&amp;s-&gt;timer);<br>
&gt; +    sysbus_mmio_map(sysbusdev, 0, AW_H3_PIT_REG_BASE);<br>
&gt; +    sysbus_connect_irq(sysbusdev, 0, s-&gt;irq[AW_H3_GIC_SPI_TIMER0]);<br>
&gt; +    sysbus_connect_irq(sysbusdev, 1, s-&gt;irq[AW_H3_GIC_SPI_TIMER1]);<br>
<br>
... we can call qdev_get_gpio_in() here directly.<br>
<br>
&gt; +<br>
&gt; +    /* SRAM */<br>
&gt; +    memory_region_init_ram(&amp;s-&gt;sram_a1, OBJECT(dev), &quot;sram A1&quot;,<br>
&gt; +                            AW_H3_SRAM_A1_SIZE, &amp;error_fatal);<br>
&gt; +    memory_region_init_ram(&amp;s-&gt;sram_a2, OBJECT(dev), &quot;sram A2&quot;,<br>
&gt; +                            AW_H3_SRAM_A2_SIZE, &amp;error_fatal);<br>
&gt; +    memory_region_init_ram(&amp;s-&gt;sram_c, OBJECT(dev), &quot;sram C&quot;,<br>
&gt; +                            AW_H3_SRAM_C_SIZE, &amp;error_fatal);<br>
&gt; +    memory_region_add_subregion(get_system_memory(), AW_H3_SRAM_A1_BASE,<br>
&gt; +                                &amp;s-&gt;sram_a1);<br>
&gt; +    memory_region_add_subregion(get_system_memory(), AW_H3_SRAM_A2_BASE,<br>
&gt; +                                &amp;s-&gt;sram_a2);<br>
&gt; +    memory_region_add_subregion(get_system_memory(), AW_H3_SRAM_C_BASE,<br>
&gt; +                                &amp;s-&gt;sram_c);<br>
&gt; +<br>
&gt; +    /* UART */<br>
&gt; +    if (serial_hd(0)) {<br>
&gt; +        serial_mm_init(get_system_memory(), AW_H3_UART0_REG_BASE, 2,<br>
&gt; +                       s-&gt;irq[AW_H3_GIC_SPI_UART0], 115200, serial_hd(0),<br>
<br>
qdev_get_gpio_in() here too.<br>
<br>
&gt; +                       DEVICE_NATIVE_ENDIAN);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    /* Unimplemented devices */<br>
&gt; +    create_unimplemented_device(&quot;display-engine&quot;, AW_H3_DE_BASE, AW_H3_DE_SIZE);<br>
&gt; +    create_unimplemented_device(&quot;dma&quot;, AW_H3_DMA_BASE, AW_H3_DMA_SIZE);<br>
&gt; +    create_unimplemented_device(&quot;lcd0&quot;, AW_H3_LCD0_BASE, AW_H3_LCD0_SIZE);<br>
&gt; +    create_unimplemented_device(&quot;lcd1&quot;, AW_H3_LCD1_BASE, AW_H3_LCD1_SIZE);<br>
&gt; +    create_unimplemented_device(&quot;gpu&quot;, AW_H3_GPU_BASE, AW_H3_GPU_SIZE);<br>
&gt; +    create_unimplemented_device(&quot;hdmi&quot;, AW_H3_HDMI_BASE, AW_H3_HDMI_SIZE);<br>
&gt; +    create_unimplemented_device(&quot;rtc&quot;, AW_H3_RTC_BASE, AW_H3_RTC_SIZE);<br>
&gt; +    create_unimplemented_device(&quot;audio-codec&quot;, AW_H3_AC_BASE, AW_H3_AC_SIZE);<br>
&gt; +}<br>
&gt; +<br>
&gt; +static void aw_h3_class_init(ObjectClass *oc, void *data)<br>
&gt; +{<br>
&gt; +    DeviceClass *dc = DEVICE_CLASS(oc);<br>
&gt; +<br>
&gt; +    dc-&gt;realize = aw_h3_realize;<br>
&gt; +    /* Reason: uses serial_hds and nd_table */<br>
&gt; +    dc-&gt;user_creatable = false;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static const TypeInfo aw_h3_type_info = {<br>
&gt; +    .name = TYPE_AW_H3,<br>
&gt; +    .parent = TYPE_DEVICE,<br>
&gt; +    .instance_size = sizeof(AwH3State),<br>
&gt; +    .instance_init = aw_h3_init,<br>
&gt; +    .class_init = aw_h3_class_init,<br>
&gt; +};<br>
&gt; +<br>
&gt; +static void aw_h3_register_types(void)<br>
&gt; +{<br>
&gt; +    type_register_static(&amp;aw_h3_type_info);<br>
&gt; +}<br>
&gt; +<br>
&gt; +type_init(aw_h3_register_types)<br>
&gt; diff --git a/include/hw/arm/allwinner-h3.h b/include/hw/arm/allwinner-h3.h<br>
&gt; new file mode 100644<br>
&gt; index 0000000000..af368c2254<br>
&gt; --- /dev/null<br>
&gt; +++ b/include/hw/arm/allwinner-h3.h<br>
&gt; @@ -0,0 +1,118 @@<br>
&gt; +/*<br>
&gt; + * Allwinner H3 System on Chip emulation<br>
&gt; + *<br>
&gt; + * Copyright (C) 2019 Niek Linnenbank &lt;<a href="mailto:nieklinnenbank@gmail.com" target="_blank">nieklinnenbank@gmail.com</a>&gt;<br>
&gt; + *<br>
&gt; + * This program is free software: you can redistribute it and/or modify<br>
&gt; + * it under the terms of the GNU General Public License as published by<br>
&gt; + * the Free Software Foundation, either version 2 of the License, or<br>
&gt; + * (at your option) any later version.<br>
&gt; + *<br>
&gt; + * This program is distributed in the hope that it will be useful,<br>
&gt; + * but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
&gt; + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the<br>
&gt; + * GNU General Public License for more details.<br>
&gt; + *<br>
&gt; + * You should have received a copy of the GNU General Public License<br>
&gt; + * along with this program.  If not, see &lt;<a href="http://www.gnu.org/licenses/" rel="noreferrer" target="_blank">http://www.gnu.org/licenses/</a>&gt;.<br>
&gt; + */<br>
&gt; +<br>
&gt; +#ifndef HW_ARM_ALLWINNER_H3_H<br>
&gt; +#define HW_ARM_ALLWINNER_H3_H<br>
&gt; +<br>
&gt; +#include &quot;qemu/error-report.h&quot;<br>
&gt; +#include &quot;qemu/units.h&quot;<br>
&gt; +#include &quot;hw/char/serial.h&quot;<br>
&gt; +#include &quot;hw/arm/boot.h&quot;<br>
&gt; +#include &quot;hw/timer/allwinner-a10-pit.h&quot;<br>
&gt; +#include &quot;hw/intc/arm_gic.h&quot;<br>
&gt; +#include &quot;target/arm/cpu.h&quot;<br>
&gt; +<br>
&gt; +#define AW_H3_SRAM_A1_BASE     (0x00000000)<br>
&gt; +#define AW_H3_SRAM_A2_BASE     (0x00044000)<br>
&gt; +#define AW_H3_SRAM_C_BASE      (0x00010000)<br>
&gt; +#define AW_H3_DE_BASE          (0x01000000)<br>
&gt; +#define AW_H3_SYSCON_BASE      (0x01c00000)<br>
&gt; +#define AW_H3_DMA_BASE         (0x01c02000)<br>
&gt; +#define AW_H3_LCD0_BASE        (0x01c0c000)<br>
&gt; +#define AW_H3_LCD1_BASE        (0x01c0d000)<br>
&gt; +#define AW_H3_SID_BASE         (0x01c14000)<br>
&gt; +#define AW_H3_CCU_BASE         (0x01c20000)<br>
&gt; +#define AW_H3_PIC_REG_BASE     (0x01c20400)<br>
&gt; +#define AW_H3_PIT_REG_BASE     (0x01c20c00)<br>
&gt; +#define AW_H3_AC_BASE          (0x01c22c00)<br>
&gt; +#define AW_H3_UART0_REG_BASE   (0x01c28000)<br>
&gt; +#define AW_H3_EMAC_BASE        (0x01c30000)<br>
&gt; +#define AW_H3_MMC0_BASE        (0x01c0f000)<br>
&gt; +#define AW_H3_EHCI0_BASE       (0x01c1a000)<br>
&gt; +#define AW_H3_OHCI0_BASE       (0x01c1a400)<br>
&gt; +#define AW_H3_EHCI1_BASE       (0x01c1b000)<br>
&gt; +#define AW_H3_OHCI1_BASE       (0x01c1b400)<br>
&gt; +#define AW_H3_EHCI2_BASE       (0x01c1c000)<br>
&gt; +#define AW_H3_OHCI2_BASE       (0x01c1c400)<br>
&gt; +#define AW_H3_EHCI3_BASE       (0x01c1d000)<br>
&gt; +#define AW_H3_OHCI3_BASE       (0x01c1d400)<br>
&gt; +#define AW_H3_GPU_BASE         (0x01c40000)<br>
&gt; +#define AW_H3_GIC_DIST_BASE    (0x01c81000)<br>
&gt; +#define AW_H3_GIC_CPU_BASE     (0x01c82000)<br>
&gt; +#define AW_H3_GIC_HYP_BASE     (0x01c84000)<br>
&gt; +#define AW_H3_GIC_VCPU_BASE    (0x01c86000)<br>
&gt; +#define AW_H3_HDMI_BASE        (0x01ee0000)<br>
&gt; +#define AW_H3_RTC_BASE         (0x01f00000)<br>
&gt; +#define AW_H3_CPUCFG_BASE      (0x01f01c00)<br>
&gt; +#define AW_H3_SDRAM_BASE       (0x40000000)<br>
&gt; +<br>
&gt; +#define AW_H3_SRAM_A1_SIZE     (64 * KiB)<br>
&gt; +#define AW_H3_SRAM_A2_SIZE     (32 * KiB)<br>
&gt; +#define AW_H3_SRAM_C_SIZE      (44 * KiB)<br>
&gt; +#define AW_H3_DE_SIZE          (4 * MiB)<br>
&gt; +#define AW_H3_DMA_SIZE         (4 * KiB)<br>
&gt; +#define AW_H3_LCD0_SIZE        (4 * KiB)<br>
&gt; +#define AW_H3_LCD1_SIZE        (4 * KiB)<br>
&gt; +#define AW_H3_GPU_SIZE         (64 * KiB)<br>
&gt; +#define AW_H3_HDMI_SIZE        (128 * KiB)<br>
&gt; +#define AW_H3_RTC_SIZE         (1 * KiB)<br>
&gt; +#define AW_H3_AC_SIZE          (2 * KiB)<br>
&gt; +<br>
&gt; +#define AW_H3_GIC_PPI_MAINT          (9)<br>
&gt; +#define AW_H3_GIC_PPI_ARM_HYPTIMER  (10)<br>
&gt; +#define AW_H3_GIC_PPI_ARM_VIRTTIMER (11)<br>
&gt; +#define AW_H3_GIC_PPI_ARM_SECTIMER  (13)<br>
&gt; +#define AW_H3_GIC_PPI_ARM_PHYSTIMER (14)<br>
&gt; +<br>
&gt; +#define AW_H3_GIC_SPI_UART0         (0)<br>
&gt; +#define AW_H3_GIC_SPI_TIMER0        (18)<br>
&gt; +#define AW_H3_GIC_SPI_TIMER1        (19)<br>
&gt; +#define AW_H3_GIC_SPI_MMC0          (60)<br>
&gt; +#define AW_H3_GIC_SPI_MMC1          (61)<br>
&gt; +#define AW_H3_GIC_SPI_MMC2          (62)<br>
&gt; +#define AW_H3_GIC_SPI_EHCI0         (72)<br>
&gt; +#define AW_H3_GIC_SPI_OHCI0         (73)<br>
&gt; +#define AW_H3_GIC_SPI_EHCI1         (74)<br>
&gt; +#define AW_H3_GIC_SPI_OHCI1         (75)<br>
&gt; +#define AW_H3_GIC_SPI_EHCI2         (76)<br>
&gt; +#define AW_H3_GIC_SPI_OHCI2         (77)<br>
&gt; +#define AW_H3_GIC_SPI_EHCI3         (78)<br>
&gt; +#define AW_H3_GIC_SPI_OHCI3         (79)<br>
&gt; +#define AW_H3_GIC_SPI_EMAC          (82)<br>
<br>
I&#39;d move half of the previous definitions into allwinner-h3.c, since <br>
they are only used there.<br>
<br></blockquote><div>Indeed, you are right, I&#39;ll move them.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, I&#39;d use an enum for the PPI/SPI. <br></blockquote><div><br></div><div></div><div> Thanks, I will process all of your comments above for the next patch version.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 
<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
&gt; +<br>
&gt; +#define AW_H3_GIC_NUM_SPI           (128)<br>
&gt; +#define AW_H3_NUM_CPUS              (4)<br>
&gt; +<br>
&gt; +#define TYPE_AW_H3 &quot;allwinner-h3&quot;<br>
&gt; +#define AW_H3(obj) OBJECT_CHECK(AwH3State, (obj), TYPE_AW_H3)<br>
&gt; +<br>
&gt; +typedef struct AwH3State {<br>
&gt; +    /*&lt; private &gt;*/<br>
&gt; +    DeviceState parent_obj;<br>
&gt; +    /*&lt; public &gt;*/<br>
&gt; +<br>
&gt; +    qemu_irq irq[AW_H3_GIC_NUM_SPI];<br>
&gt; +    AwA10PITState timer;<br>
&gt; +    GICState gic;<br>
&gt; +    MemoryRegion sram_a1;<br>
&gt; +    MemoryRegion sram_a2;<br>
&gt; +    MemoryRegion sram_c;<br>
&gt; +} AwH3State;<br>
&gt; +<br>
&gt; +#endif<br>
&gt; <br>
<br>
Nice clean patch, for a first contribution :)<br></blockquote><div><br></div><div>Thank you Philippe!  <br></div></div><div><br></div><div>Regards,</div><div>Niek<br></div><br>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Niek Linnenbank<br><br></div></div></div></div>

  reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-02 21:09 [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Niek Linnenbank
2019-12-02 21:09 ` [PATCH 01/10] hw: arm: add Allwinner H3 System-on-Chip Niek Linnenbank
2019-12-04 16:53   ` Philippe Mathieu-Daudé
2019-12-04 20:44     ` Niek Linnenbank [this message]
2019-12-10  9:02   ` Philippe Mathieu-Daudé
2019-12-10 19:17     ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine Niek Linnenbank
2019-12-03  9:17   ` Philippe Mathieu-Daudé
2019-12-03 19:33     ` Niek Linnenbank
2019-12-04  9:03       ` Philippe Mathieu-Daudé
2019-12-04 19:50         ` Niek Linnenbank
2019-12-05 22:15     ` Niek Linnenbank
2019-12-06  5:41       ` Philippe Mathieu-Daudé
2019-12-06 22:15         ` Niek Linnenbank
2019-12-10  8:59           ` Philippe Mathieu-Daudé
2019-12-10 19:14             ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 03/10] arm: allwinner-h3: add Clock Control Unit Niek Linnenbank
2019-12-02 21:09 ` [PATCH 04/10] arm: allwinner-h3: add USB host controller Niek Linnenbank
2019-12-04 16:11   ` Aleksandar Markovic
2019-12-04 20:20     ` Niek Linnenbank
2019-12-10  7:56   ` Philippe Mathieu-Daudé
2019-12-10  8:29     ` Gerd Hoffmann
2019-12-10 19:11       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 05/10] arm: allwinner-h3: add System Control module Niek Linnenbank
2019-12-02 21:09 ` [PATCH 06/10] arm/arm-powerctl: set NSACR.{CP11, CP10} bits in arm_set_cpu_on() Niek Linnenbank
2019-12-06 14:24   ` Peter Maydell
2019-12-06 20:01     ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 07/10] arm: allwinner-h3: add CPU Configuration module Niek Linnenbank
2019-12-02 21:09 ` [PATCH 08/10] arm: allwinner-h3: add Security Identifier device Niek Linnenbank
2019-12-06 14:27   ` Peter Maydell
2019-12-06 16:35     ` Philippe Mathieu-Daudé
2019-12-06 20:20       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller Niek Linnenbank
2019-12-02 21:09 ` [PATCH 10/10] arm: allwinner-h3: add EMAC ethernet device Niek Linnenbank
2019-12-03  9:33   ` KONRAD Frederic
2019-12-03 19:41     ` Niek Linnenbank
2019-12-04 15:14     ` Philippe Mathieu-Daudé
2019-12-04 15:22       ` KONRAD Frederic
2019-12-03  8:47 ` [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Philippe Mathieu-Daudé
2019-12-03 19:25   ` Niek Linnenbank
2019-12-10  8:40     ` Philippe Mathieu-Daudé
2019-12-09 21:37   ` Niek Linnenbank
2019-12-10  8:26     ` Philippe Mathieu-Daudé
2019-12-10 20:12       ` Niek Linnenbank
2019-12-03  9:02 ` Philippe Mathieu-Daudé
2019-12-03 19:32   ` Niek Linnenbank
2019-12-06 14:16     ` Peter Maydell
2019-12-09 22:24       ` Aleksandar Markovic
2019-12-10 10:34 ` KONRAD Frederic
2019-12-10 19:55   ` Niek Linnenbank

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPan3WpeYtA249jp2iF-baFXh2YihEYMiGVtov7DLpsOVb5ZhQ@mail.gmail.com \
    --to=nieklinnenbank@gmail.com \
    --cc=armbru@redhat.com \
    --cc=b.galvani@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=philmd@redhat.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git