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

[Niek Linnenbank <nieklinnenbank@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 12552 bytes --]

Hi Philippe,

On Fri, Dec 6, 2019 at 6:41 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 12/5/19 11:15 PM, Niek Linnenbank wrote:
> > Hello Philippe,
> >
> > On Tue, Dec 3, 2019 at 10:18 AM Philippe Mathieu-Daudé
> > <philmd@redhat.com <mailto:philmd@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 <nieklinnenbank@gmail.com
> >     <mailto:nieklinnenbank@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-arm@nongnu.org
> >     <mailto:qemu-arm@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.maydell@linaro.org
> >     <mailto:peter.maydell@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 <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 = {
> >      > +    .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);
> >             }
> >
> > Done!
> >
> >      > +    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.
> >
> > Done, I applied it to all the functions and removed the err variable.
> >
> >      > +    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.
> >
> >
> > Agree, we should add something for that. Would it be acceptable if we
> > make the 1GB an upper limit?
> > I see that the Raspberry Pi is doing that too in hw/arm/raspi.c, like so:
> >
> >      if (machine->ram_size > 1 * GiB) {
> >          error_report("Requested ram size is too large for this machine:
> "
> >                       "maximum is 1GB");
> >          exit(1);
> >      }
> >
> > I think it would be helpful to allow the flexibility to the user of
> > reducing the RAM to less than 1GB,
> > in case resources of the host OS are limited. What do you think?
>
> Sure, good idea.
>
> FIY (in case you add more models) we recently noticed there is a problem
> when using 2GiB default on 32-bit hosts, so the workaround is to use <=
> 1GiB default.
>
> >      > +    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".
> >
> > Thanks, I'm working on this now. I think that at least I'll need to add
> > all of the devices mentioned in the 4.1 Memory Mapping chapter of the
> > datasheet
> > as an unimplemented device. Previously I only added some that I thought
> > were relevant.
> >
> > I added all the missing devices as unimplemented and removed the
> > ignore_memory_transaction_failures flag
>
> I was going to say, "instead of adding *all* the devices regions you can
> add the likely bus decoding regions", probably:
>
> 0x01c0.0000   128KiB   AMBA AXI
> 0x01c2.0000   64KiB    AMBA APB
>
> But too late.
>

Hehe its okey, I can change it to whichever is preferable: the minimum set
with unimplemented device entries to get a working linux kernel / u-boot or
just cover the full memory space from the datasheet. My initial thought was
that if
we only provide the minimum set, and the linux kernel later adds a new
driver for a device
which is not marked unimplemented, it will trigger the data abort and
potentially resulting in a non-booting kernel.

But I'm not sure what is normally done here. I do see other board files
using the create_unimplemented_device() function,
but I dont know if they are covering the whole memory space or not.

Any thoughts? :-)


>
> > from the machine. Now it seems Linux gets a data abort while probing the
> > uart1 serial device at 0x01c28400,
>
> Did you add the UART1 as UNIMP or 16550?
>
>
I discovered what goes wrong here. See this kernel oops message:

[    1.084985] [f08600f8] *pgd=6f00a811, *pte=01c28653, *ppte=01c28453
[    1.085564] Internal error: : 8 [#1] SMP ARM
[    1.085698] Modules linked in:
[    1.085940] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
5.4.0-11747-g2f13437b8917 #4
[    1.085968] Hardware name: Allwinner sun8i Family
[    1.086447] PC is at dw8250_setup_port+0x10/0x10c
[    1.086478] LR is at dw8250_probe+0x500/0x56c

It tries to access the UART0 at base address 0x01c28400, which I did
provide. The strange
thing is that is accesses offset 0xf8, thus address 0x01c284f8. The
datasheet does not mention this register
but if we provide the 1KiB (0x400) I/O space it should at least read as
zero and writes ignored. Unfortunately the emulated
serial driver only maps a small portion until 0x1f:

(qemu) info mtree
...
    0000000001c28000-0000000001c2801f (prio 0, i/o): serial
    0000000001c28400-0000000001c2841f (prio 0, i/o): serial
    0000000001c28800-0000000001c2881f (prio 0, i/o): serial


Apparently, the register that the mainline linux kernel is using is
DesignWare specific:

drivers/tty/serial/8250/8250_dwlib.c:13:

/* Offsets for the DesignWare specific registers */
#define DW_UART_DLF<--->0xc0 /* Divisor Latch Fraction Register */
#define DW_UART_CPR<--->0xf4 /* Component Parameter Register */
#define DW_UART_UCV<--->0xf8 /* UART Component Version */

I tried to find a way to increase the memory mapped size of the serial
device I created with serial_mm_init(),
but I don't think its possible with that interface.

I did manage to get it working by overlaying the UART0 with another
unimplemented device
that does have an I/O size of 0x400, but I guess that is probably not the
solution we are looking for?

I wonder, did any of the other SoC / boards have this problem when removing
mc->ignore_memory_transaction_failures?

Regards,
Niek

> so I'll need to debug it further. I'll post back when I have more results.
> >
> > Regards,
> > Niek
> >
> >      > +}
> >      > +
> >      > +DEFINE_MACHINE("orangepi", orangepi_machine_init)
> >
> >     Can you name it 'orangepi-pc'? So we can add other orangepi models.
> >
> >     Thanks,
> >
> >     Phil.
> >
> >
> >
> > --
> > Niek Linnenbank
> >
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 17219 bytes --]