Re: [PATCH v2 02/10] hw: arm: add Xunlong Orange Pi PC machine

[Philippe Mathieu-Daudé]

Hi Niek,

On 12/17/19 12:35 AM, 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 <address@hidden>
> Tested-by: KONRAD Frederic <address@hidden>
> ---
>   hw/arm/orangepi.c    | 101 +++++++++++++++++++++++++++++++++++++++++++
>   MAINTAINERS          |   1 +
>   hw/arm/Makefile.objs |   2 +-
>   3 files changed, 103 insertions(+), 1 deletion(-)
>   create mode 100644 hw/arm/orangepi.c
>
> diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> new file mode 100644
> index 0000000000..62cefc8c06
> --- /dev/null
> +++ b/hw/arm/orangepi.c
> @@ -0,0 +1,101 @@
> +/*
> + * Orange Pi emulation
> + *
> + * Copyright (C) 2019 Niek Linnenbank <address@hidden>
> + *
> + * 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 = {
> +    .board_id = -1,
> +};
> +
> +typedef struct OrangePiState {
> +    AwH3State *h3;
> +    MemoryRegion sdram;
> +} OrangePiState;
> +
> +static void orangepi_init(MachineState *machine)
> +{
> +    OrangePiState *s = g_new(OrangePiState, 1);
> +
> +    /* Only allow Cortex-A7 for this board */
> +    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",
> +                            &error_abort);

You access the timer object which is contained inside the soc object, 
but the soc isn't realized yet... I wonder if this is OK. Usually what 
we do is, either:
- add a 'xtal-freq-hz' property to the SoC, set it here in the board, 
then in soc::realize() set the property to the timer
- add an alias in the SoC to the timer 'freq-hz' property:
    object_property_add_alias(soc, "xtal-freq-hz", OBJECT(&s->timer),
                                   "freq-hz", &error_abort);

Also, if you use &error_abort, a failure in object_property_set_int() 
triggers abort(). See "qapi/error.h":

 * If @errp is &error_abort, print a suitable message and abort().
 * If @errp is &error_fatal, print a suitable message and exit(1).

> +    if (error_abort != NULL) {
> +        error_reportf_err(error_abort, "Couldn't set clk0 frequency: ");
> +        exit(1);
> +    }

So this if() block is useless.

> +
> +    object_property_set_int(OBJECT(&s->h3->timer), 24000000, "clk1-freq",
> +                            &error_abort);
> +    if (error_abort != NULL) {
> +        error_reportf_err(error_abort, "Couldn't set clk1 frequency: ");
> +        exit(1);
> +    }

Similarly, remove if() block.

> +
> +    /* Mark H3 object realized */
> +    object_property_set_bool(OBJECT(s->h3), true, "realized", &error_abort);
> +    if (error_abort != NULL) {
> +        error_reportf_err(error_abort, "Couldn't realize Allwinner H3: ");
> +        exit(1);
> +    }

Similarly, remove if() block.

> +
> +    /* RAM */
> +    if (machine->ram_size > 1 * GiB) {
> +        error_report("Requested ram size is too large for this machine: "
> +                     "maximum is 1GB");

Per http://www.orangepi.org/orangepipc/ this board comes with a specific 
amount of RAM. I'd enforce the default (1GiB) and refuse other cases.

I noticed this by testing your series, without specifying the memory 
size you suggested in the cover (512) it defaults to 128 MiB, and the 
Raspian userland fails:

[ ***  ] (2 of 4) A start job is running for…Persistent Storage (37s / 
2min 1s)
[  *** ] (2 of 4) A start job is running for…Persistent Storage (38s / 
2min 1s)
[  OK  ] Started Flush Journal to Persistent Storage.
Starting Create Volatile Files and Directories...
Starting Armbian ZRAM config...
[    **] (3 of 6) A start job is running for…s and Directories (55s / no 
limit)
[     *] (3 of 6) A start job is running for…s and Directories (55s / no 
limit)
[    **] (3 of 6) A start job is running for…s and Directories (56s / no 
limit)
[  OK  ] Started Create Volatile Files and Directories.
[***   ] (5 of 6) A start job is running for… ZRAM config (1min 10s / 
1min 19s)
[**    ] (5 of 6) A start job is running for… ZRAM config (1min 12s / 
1min 19s)
[*     ] (5 of 6) A start job is running for… ZRAM config (1min 13s / 
1min 19s)
[FAILED] Failed to start Armbian ZRAM config.
See 'systemctl status armbian-zram-config.service' for details.

> +        exit(1);
> +    }
> +    memory_region_allocate_system_memory(&s->sdram, NULL, "orangepi.ram",

There is only one type of ram on this machine, I'd simply name this "sdram".

> +                                         machine->ram_size);
> +    memory_region_add_subregion(get_system_memory(), 
> s->h3->memmap[AW_H3_SDRAM],
> +                                &s->sdram);
> +
> +    /* Load target kernel */
> +    orangepi_binfo.loader_start = s->h3->memmap[AW_H3_SDRAM];
> +    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);

I wonder if we should tell the user '-bios' is not supported on this 
machine.

> +}
> +
> +static void orangepi_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "Orange Pi PC";
> +    mc->init = orangepi_init;
> +    mc->units_per_default_bus = 1;

Maybe "units_per_default_bus = 1" belongs to patch 9 "add SD/MMC host 
controller".

> +    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");

Please add:

       mc->default_ram_size = 1 * GiB;

> +}
> +
> +DEFINE_MACHINE("orangepi-pc", orangepi_machine_init)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index aae1a049b4..db682e49ca 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -486,6 +486,7 @@ L: address@hidden
>   S: Maintained
>   F: hw/*/allwinner-h3*
>   F: include/hw/*/allwinner-h3*
> +F: hw/arm/orangepi.c
>   
>   ARM PrimeCell and CMSDK devices
>   M: Peter Maydell <address@hidden>
> 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