qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Niek Linnenbank
Subject: Re: [PATCH v2 02/10] hw: arm: add Xunlong Orange Pi PC machine
Date: Wed, 18 Dec 2019 21:14:02 +0100

Hi Philippe,

Thanks again for your quick and helpful feedback! :-)

On Tue, Dec 17, 2019 at 8:31 AM Philippe Mathieu-Daudé <address@hidden> wrote:
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);
Good point. I shall rework that part using your suggestion.
Actually, I copied the timer support code from the existing cubieboard.c that has
the Allwinner A10, so potentially the same problem is there.

While looking more closer at this part, I now also discovered that the timer module from the Allwinner H3 is
mostly a stripped down version of the timer module in the Allwinner A10:

  Allwinner A10, 10.2 Timer Register List, page 85:
  https://linux-sunxi.org/images/1/1e/Allwinner_A10_User_manual_V1.5.pdf

The A10 version has six timers, where the H3 has only two. That should be fine I would say, the guest would simply
use those available on H3 and ignore the rest. There is however one conflicting difference: the WDOG0 registers in the Allwinner H3 start
at a different offset and are also different. The current A10 timer does not currently implement the watchdog part.

The watchdog part of this timer is relevant for the 'reset' command in U-Boot: that does not work right now, because
U-Boot implements the reset for the Allwinner H3 boards by letting this watchdog expire (and we dont emulate it).
Also, this timer module is required for booting Linux, without it the kernel crashes using the sunxi_defconfig:

[    0.000000] PC is at sun4i_timer_init+0x34/0x168
[    0.000000] LR is at sun4i_timer_init+0x2c/0x168
[    0.000000] pc : [<c07fa634>]    lr : [<c07fa62c>]    psr: 600000d3
[    0.000000] sp : c0a03f70  ip : eec00188  fp : ef7ed040
...
[ 0.000000] [<c07fa634>] (sun4i_timer_init) from [<c07fa4e8>] (timer_probe+0x74/0xe4) [ 0.000000] [<c07fa4e8>] (timer_probe) from [<c07d9c10>] (start_kernel+0x2e0/0x440) [ 0.000000] [<c07d9c10>] (start_kernel) from [<00000000>] (0x0)

So in my opinion its a bit of a trade off here: we can keep it like this and re-use the A10 timer for now, and perhaps
attempt to generalize that module for proper use in both SoCs. Or we can introduce a new H3 specific timer module.
What do you think?
 

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.

Ah ok, I'll remove them.
 
> +
> +    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.

OK sure, I'll change it to a enforcing 1GiB. I do recall we briefly discussed this
in v1. Then we agreed to make it an upper limit for use cases where resources are
limited which is why I changed it like this.


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:

Indeed! By the way, this is also the case for U-Boot: it freezes when using 128MiB.
Actually when working on the initial code I searched a bit for a way
to set a default ram size, but could not find it at that time. But now I see in your comment below,
it can be done simply with mc->default_ram_size. Thanks a lot, I will surely add that!
 

[ ***  ] (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".

OK!


> +                                         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.

Good suggestion, its not handled, at least not anywhere in the code I added for H3 support.
Shall I make it an error_report followed by exit(1), similar to the 1GiB check?
 

> +}
> +
> +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".
True, it should be in patch 9 indeed. I overlooked this when separating the work in individual patches.
Now I am also wondering if I actually need this setting. Without it, the SD device still works fine.
I did some greps in the code to discover what it is used for, but its not very clear to me yet. Is this ment to
restrict machines to only one harddisk (or SD card)? If I try to supply multiple SD cards with multiple -sd arguments,
this error is printed, regardless of having units_per_default_bus=1 or not:
   qemu-system-arm: -sd test3.ext2: machine type does not support if=sd,bus=1,unit=0
 

> +    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;
Yes, thanks!
 

> +}
> +
> +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
>


Regards,
Niek

--
Niek Linnenbank


reply via email to

[Prev in Thread] Current Thread [Next in Thread]