Hello Philippe,

Thanks for your quick review comments!
I'll start working on a v2 of the patches and include the changes you
suggested.

Regards,
Niek

On Tue, Dec 3, 2019 at 10:18 AM Philippe Mathieu-Daudé <phi...@redhat.com>
wrote:

> On 12/2/19 10:09 PM, Niek Linnenbank wrote:
> > The Xunlong Orange Pi PC is an Allwinner H3 System on Chip
> > based embedded computer with mainline support in both U-Boot
> > and Linux. The board comes with a Quad Core Cortex A7 @ 1.3GHz,
> > 512MB RAM, 100Mbit ethernet, USB, SD/MMC, USB, HDMI and
> > various other I/O. This commit add support for the Xunlong
> > Orange Pi PC machine.
> >
> > Signed-off-by: Niek Linnenbank <nieklinnenb...@gmail.com>
> > ---
> >   MAINTAINERS          |  1 +
> >   hw/arm/Makefile.objs |  2 +-
> >   hw/arm/orangepi.c    | 90 ++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 92 insertions(+), 1 deletion(-)
> >   create mode 100644 hw/arm/orangepi.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 29c9936037..42c913d6cb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -485,6 +485,7 @@ L: qemu-...@nongnu.org
> >   S: Maintained
> >   F: hw/*/allwinner-h3*
> >   F: include/hw/*/allwinner-h3*
> > +F: hw/arm/orangepi.c
> >
> >   ARM PrimeCell and CMSDK devices
> >   M: Peter Maydell <peter.mayd...@linaro.org>
> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> > index 956e496052..8d5ea453d5 100644
> > --- a/hw/arm/Makefile.objs
> > +++ b/hw/arm/Makefile.objs
> > @@ -34,7 +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_ALLWINNER_H3) += allwinner-h3.o orangepi.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/orangepi.c b/hw/arm/orangepi.c
> > new file mode 100644
> > index 0000000000..5ef2735f81
> > --- /dev/null
> > +++ b/hw/arm/orangepi.c
> > @@ -0,0 +1,90 @@
> > +/*
> > + * Orange Pi emulation
> > + *
> > + * Copyright (C) 2019 Niek Linnenbank <nieklinnenb...@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 "cpu.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/boards.h"
> > +#include "hw/qdev-properties.h"
> > +#include "hw/arm/allwinner-h3.h"
> > +
> > +static struct arm_boot_info orangepi_binfo = {
> > +    .loader_start = AW_H3_SDRAM_BASE,
> > +    .board_id = -1,
> > +};
> > +
> > +typedef struct OrangePiState {
> > +    AwH3State *h3;
> > +    MemoryRegion sdram;
> > +} OrangePiState;
> > +
> > +static void orangepi_init(MachineState *machine)
> > +{
> > +    OrangePiState *s = g_new(OrangePiState, 1);
> > +    Error *err = NULL;
> > +
>
> Here I'd add:
>
>        if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a7")) != 0)
> {
>            error_report("This board can only be used with cortex-a7 CPU");
>            exit(1);
>        }
>
> > +    s->h3 = AW_H3(object_new(TYPE_AW_H3));
> > +
> > +    /* Setup timer properties */
> > +    object_property_set_int(OBJECT(&s->h3->timer), 32768, "clk0-freq",
> &err);
> > +    if (err != NULL) {
> > +        error_reportf_err(err, "Couldn't set clk0 frequency: ");
> > +        exit(1);
> > +    }
> > +
> > +    object_property_set_int(OBJECT(&s->h3->timer), 24000000,
> "clk1-freq",
> > +                            &err);
> > +    if (err != NULL) {
> > +        error_reportf_err(err, "Couldn't set clk1 frequency: ");
> > +        exit(1);
> > +    }
> > +
> > +    /* Mark H3 object realized */
> > +    object_property_set_bool(OBJECT(s->h3), true, "realized", &err);
>
> I'm not sure if that's correct but I'd simply use &error_abort here.
>
> > +    if (err != NULL) {
> > +        error_reportf_err(err, "Couldn't realize Allwinner H3: ");
> > +        exit(1);
> > +    }
> > +
> > +    /* RAM */
> > +    memory_region_allocate_system_memory(&s->sdram, NULL,
> "orangepi.ram",
> > +                                         machine->ram_size);
>
> I'd only allow machine->ram_size == 1 * GiB here, since the onboard DRAM
> is not upgradable.
>
> > +    memory_region_add_subregion(get_system_memory(), AW_H3_SDRAM_BASE,
> > +                                &s->sdram);
> > +
> > +    /* Load target kernel */
> > +    orangepi_binfo.ram_size = machine->ram_size;
> > +    orangepi_binfo.nb_cpus  = AW_H3_NUM_CPUS;
> > +    arm_load_kernel(ARM_CPU(first_cpu), machine, &orangepi_binfo);
> > +}
> > +
> > +static void orangepi_machine_init(MachineClass *mc)
> > +{
> > +    mc->desc = "Orange Pi PC";
> > +    mc->init = orangepi_init;
> > +    mc->units_per_default_bus = 1;
> > +    mc->min_cpus = AW_H3_NUM_CPUS;
> > +    mc->max_cpus = AW_H3_NUM_CPUS;
> > +    mc->default_cpus = AW_H3_NUM_CPUS;
>
>         mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
>
> > +    mc->ignore_memory_transaction_failures = true;
>
> You should not use this flag in new design. See the documentation in
> include/hw/boards.h:
>
>   * @ignore_memory_transaction_failures:
>   *    [...] New board models
>   *    should instead use "unimplemented-device" for all memory ranges
> where
>   *    the guest will attempt to probe for a device that QEMU doesn't
>   *    implement and a stub device is required.
>
> You already use the "unimplemented-device".
>
> > +}
> > +
> > +DEFINE_MACHINE("orangepi", orangepi_machine_init)
>
> Can you name it 'orangepi-pc'? So we can add other orangepi models.
>
> Thanks,
>
> Phil.
>
>

-- 
Niek Linnenbank

WWW: http://www.nieklinnenbank.nl/
BLOG: http://nieklinnenbank.wordpress.com/
FUN:    http://www.FreeNOS.org/

Reply via email to