[02/10] hw: arm: add Xunlong Orange Pi PC machine
diff mbox series

Message ID 20191202210947.3603-3-nieklinnenbank@gmail.com
State New
Headers show
Series
  • Add Allwinner H3 SoC and Orange Pi PC Machine
Related show

Commit Message

Niek Linnenbank Dec. 2, 2019, 9:09 p.m. UTC
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>
---
 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

Comments

Philippe Mathieu-Daudé Dec. 3, 2019, 9:17 a.m. UTC | #1
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>
> ---
>   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
>   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>
> 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>
> + *
> + * 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);
       }

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

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

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

> +}
> +
> +DEFINE_MACHINE("orangepi", orangepi_machine_init)

Can you name it 'orangepi-pc'? So we can add other orangepi models.

Thanks,

Phil.
Niek Linnenbank Dec. 3, 2019, 7:33 p.m. UTC | #2
Hello Philippe,

Thanks for your quick review comments!
I'll start working on a v2 of the patches and include the changes you
suggested.

Regards,
Niek

On Tue, Dec 3, 2019 at 10:18 AM Philippe Mathieu-Daudé <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>
> > ---
> >   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
> >   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>
> > 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>
> > + *
> > + * 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);
>        }
>
> > +    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.
>
> > +    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.
>
> > +    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".
>
> > +}
> > +
> > +DEFINE_MACHINE("orangepi", orangepi_machine_init)
>
> Can you name it 'orangepi-pc'? So we can add other orangepi models.
>
> Thanks,
>
> Phil.
>
>
Philippe Mathieu-Daudé Dec. 4, 2019, 9:03 a.m. UTC | #3
On 12/3/19 8:33 PM, Niek Linnenbank wrote:
> Hello Philippe,
> 
> Thanks for your quick review comments!
> I'll start working on a v2 of the patches and include the changes you 
> suggested.

Thanks, but I'd suggest to wait few more days to give time to others 
reviewers. Else having multiple versions of a big series reviewed at the 
same time is very confusing.
I have other minor comments on others patches, but need to find the time 
to continue reviewing.
Niek Linnenbank Dec. 4, 2019, 7:50 p.m. UTC | #4
On Wed, Dec 4, 2019 at 10:03 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 12/3/19 8:33 PM, Niek Linnenbank wrote:
> > Hello Philippe,
> >
> > Thanks for your quick review comments!
> > I'll start working on a v2 of the patches and include the changes you
> > suggested.
>
> Thanks, but I'd suggest to wait few more days to give time to others
> reviewers. Else having multiple versions of a big series reviewed at the
> same time is very confusing.
> I have other minor comments on others patches, but need to find the time
> to continue reviewing.
>
>
OK Philippe, I will follow your advise and wait a few more days before
submitting a new version.
I'll wait at least until you had a chance to review all the patches. I'm
new to the QEMU
community, so I will need to learn the process along the way.

Regards,
Niek
Niek Linnenbank Dec. 5, 2019, 10:15 p.m. UTC | #5
Hello Philippe,

On Tue, Dec 3, 2019 at 10:18 AM Philippe Mathieu-Daudé <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>
> > ---
> >   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
> >   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>
> > 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>
> > + *
> > + * 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?



> > +    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
from the machine. Now it seems Linux gets a data abort while probing the
uart1 serial device at 0x01c28400,
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.
>
>
Philippe Mathieu-Daudé Dec. 6, 2019, 5:41 a.m. UTC | #6
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.

> 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?

> 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 Dec. 6, 2019, 10:15 p.m. UTC | #7
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
> >
>
>
Philippe Mathieu-Daudé Dec. 10, 2019, 8:59 a.m. UTC | #8
On 12/6/19 11:15 PM, Niek Linnenbank wrote:
[...]
>      >      > +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.

If they don't cover all memory regions, the guest code can trigger a 
data abort indeed. Since we don't know the memory layout of old board, 
they are still supported with ignore_memory_transaction_failures=true, 
so guest still run unaffected.
We expect new boards to implement the minimum layout.
As long as your guest is happy and usable, UNIMP devices are fine, 
either as big region or individual device (this requires someone with 
access to the datasheet to verify). If you can add a UNIMP for each 
device - which is what you did - it is even better because the the unimp 
access log will be more useful, having finer granularity.

 > I added all the missing devices as unimplemented and removed the
 > ignore_memory_transaction_failures flag

IOW, you already did the best you could do :)

>      > 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?

IMHO this is the correct solution :)

Memory regions have priority. By default a device has priority 0, and 
UNIMP device has priority -1000.

So you can use:

    serial_mm_init(get_system_memory(), AW_H3_UART0_REG_BASE, 2,
                   s->irq[AW_H3_GIC_SPI_UART0], 115200,
                   serial_hd(0), DEVICE_NATIVE_ENDIAN);
    create_unimplemented_device("DesignWare-uart",\
                                AW_H3_UART0_REG_BASE,
                                0x400);

(Or cleaner, add a create_designware_uart(...) function that does both).

(qemu) info mtree
...
    0000000001c28000-0000000001c2801f (prio 0, i/o): serial
    0000000001c28000-0000000001c283ff (prio -1000, i/o): DesignWare-uart

You could create an UNIMP region of 0x400 - 0x20 = 0x3e0, but that would 
appear this is a different device, so I don't recommend that.

 > I wonder, did any of the other SoC / boards have this problem when
 > removing mc->ignore_memory_transaction_failures?
Niek Linnenbank Dec. 10, 2019, 7:14 p.m. UTC | #9
Hi Philippe,

On Tue, Dec 10, 2019 at 9:59 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 12/6/19 11:15 PM, Niek Linnenbank wrote:
> [...]
> >      >      > +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.
>
> If they don't cover all memory regions, the guest code can trigger a
> data abort indeed. Since we don't know the memory layout of old board,
> they are still supported with ignore_memory_transaction_failures=true,
> so guest still run unaffected.
> We expect new boards to implement the minimum layout.
> As long as your guest is happy and usable, UNIMP devices are fine,
> either as big region or individual device (this requires someone with
> access to the datasheet to verify). If you can add a UNIMP for each
> device - which is what you did - it is even better because the the unimp
> access log will be more useful, having finer granularity.
>
>  > I added all the missing devices as unimplemented and removed the
>  > ignore_memory_transaction_failures flag
>
> IOW, you already did the best you could do :)
>
> >      > 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?
>
> IMHO this is the correct solution :)
>
> Memory regions have priority. By default a device has priority 0, and
> UNIMP device has priority -1000.
>
> So you can use:
>
>     serial_mm_init(get_system_memory(), AW_H3_UART0_REG_BASE, 2,
>                    s->irq[AW_H3_GIC_SPI_UART0], 115200,
>                    serial_hd(0), DEVICE_NATIVE_ENDIAN);
>     create_unimplemented_device("DesignWare-uart",\
>                                 AW_H3_UART0_REG_BASE,
>                                 0x400);
>
>
Now it makes much more sense to me, thanks a lot for explaining this!

Allright, I'll use this approach to finish the work for removing
mc->ignore_memory_transaction_failures.

Regards,
Niek


> (Or cleaner, add a create_designware_uart(...) function that does both).
>
> (qemu) info mtree
> ...
>     0000000001c28000-0000000001c2801f (prio 0, i/o): serial
>     0000000001c28000-0000000001c283ff (prio -1000, i/o): DesignWare-uart
>
> You could create an UNIMP region of 0x400 - 0x20 = 0x3e0, but that would
> appear this is a different device, so I don't recommend that.
>
>  > I wonder, did any of the other SoC / boards have this problem when
>  > removing mc->ignore_memory_transaction_failures?
>
>

Patch
diff mbox series

diff --git a/MAINTAINERS b/MAINTAINERS
index 29c9936037..42c913d6cb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -485,6 +485,7 @@  L: 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>
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>
+ *
+ * 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;
+
+    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);
+    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);
+    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->ignore_memory_transaction_failures = true;
+}
+
+DEFINE_MACHINE("orangepi", orangepi_machine_init)