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