On Fri, Dec 6, 2019 at 6:41 AM Philippe Mathieu-Daudé <
address@hidden> 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é
> <address@hidden <mailto:address@hidden>> 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 <address@hidden
> <mailto:address@hidden>>
> > ---
> > 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: address@hidden
> <mailto: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
> <mailto: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
> > 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 <address@hidden
> <mailto: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 = {
> > + .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
>