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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 12/18/19 9:14 PM, Niek Linnenbank wrote:
> Hi Philippe,
> 
> Thanks again for your quick and helpful feedback! :-)
> 
> On Tue, Dec 17, 2019 at 8:31 AM Philippe Mathieu-Daudé 
> <philmd@redhat.com <mailto:philmd@redhat.com>> 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 <nieklinnenbank@gmail.com
>     <mailto:nieklinnenbank@gmail.com>>
>      > Tested-by: KONRAD Frederic <frederic.konrad@adacore.com
>     <mailto:frederic.konrad@adacore.com>>
>      > ---
>      >   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 <nieklinnenbank@gmail.com
>     <mailto: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 "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?

See my answer about generalize/reuse here:
http://mid.mail-archive.com/20191219185127.24388-1-f4bug@amsat.org