QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Niek Linnenbank <nieklinnenbank@gmail.com>
Cc: b.galvani@gmail.com, Peter Maydell <peter.maydell@linaro.org>,
	qemu-arm@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine
Date: Fri, 6 Dec 2019 06:41:02 +0100
Message-ID: <ce2125dd-004c-a5a2-81cf-b8aaae76444e@redhat.com> (raw)
In-Reply-To: <CAPan3WohBP_47h1ZPP12ZuyiY43B3B6tGFMH-sK=LAAjMbwpaA@mail.gmail.com>

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
> 



  reply index

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-02 21:09 [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Niek Linnenbank
2019-12-02 21:09 ` [PATCH 01/10] hw: arm: add Allwinner H3 System-on-Chip Niek Linnenbank
2019-12-04 16:53   ` Philippe Mathieu-Daudé
2019-12-04 20:44     ` Niek Linnenbank
2019-12-10  9:02   ` Philippe Mathieu-Daudé
2019-12-10 19:17     ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine Niek Linnenbank
2019-12-03  9:17   ` Philippe Mathieu-Daudé
2019-12-03 19:33     ` Niek Linnenbank
2019-12-04  9:03       ` Philippe Mathieu-Daudé
2019-12-04 19:50         ` Niek Linnenbank
2019-12-05 22:15     ` Niek Linnenbank
2019-12-06  5:41       ` Philippe Mathieu-Daudé [this message]
2019-12-06 22:15         ` Niek Linnenbank
2019-12-10  8:59           ` Philippe Mathieu-Daudé
2019-12-10 19:14             ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 03/10] arm: allwinner-h3: add Clock Control Unit Niek Linnenbank
2019-12-13  0:03   ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 04/10] arm: allwinner-h3: add USB host controller Niek Linnenbank
2019-12-04 16:11   ` Aleksandar Markovic
2019-12-04 20:20     ` Niek Linnenbank
2019-12-10  7:56   ` Philippe Mathieu-Daudé
2019-12-10  8:29     ` Gerd Hoffmann
2019-12-10 19:11       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 05/10] arm: allwinner-h3: add System Control module Niek Linnenbank
2019-12-13  0:09   ` Philippe Mathieu-Daudé
2019-12-15 23:27     ` Niek Linnenbank
2019-12-16  0:17       ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 06/10] arm/arm-powerctl: set NSACR.{CP11, CP10} bits in arm_set_cpu_on() Niek Linnenbank
2019-12-06 14:24   ` Peter Maydell
2019-12-06 20:01     ` Niek Linnenbank
2019-12-13 20:52       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 07/10] arm: allwinner-h3: add CPU Configuration module Niek Linnenbank
2019-12-02 21:09 ` [PATCH 08/10] arm: allwinner-h3: add Security Identifier device Niek Linnenbank
2019-12-06 14:27   ` Peter Maydell
2019-12-06 16:35     ` Philippe Mathieu-Daudé
2019-12-06 20:20       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller Niek Linnenbank
2019-12-11 22:34   ` Niek Linnenbank
2019-12-12 23:56     ` Philippe Mathieu-Daudé
2019-12-13 21:00       ` Niek Linnenbank
2019-12-14 13:59         ` Philippe Mathieu-Daudé
2019-12-14 20:32           ` Niek Linnenbank
2019-12-15 23:07       ` Niek Linnenbank
2019-12-16  0:14         ` Philippe Mathieu-Daudé
2019-12-16 19:46           ` Niek Linnenbank
2019-12-16 21:28             ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 10/10] arm: allwinner-h3: add EMAC ethernet device Niek Linnenbank
2019-12-03  9:33   ` KONRAD Frederic
2019-12-03 19:41     ` Niek Linnenbank
2019-12-04 15:14     ` Philippe Mathieu-Daudé
2019-12-04 15:22       ` KONRAD Frederic
2019-12-03  8:47 ` [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Philippe Mathieu-Daudé
2019-12-03 19:25   ` Niek Linnenbank
2019-12-10  8:40     ` Philippe Mathieu-Daudé
2019-12-09 21:37   ` Niek Linnenbank
2019-12-10  8:26     ` Philippe Mathieu-Daudé
2019-12-10 20:12       ` Niek Linnenbank
2019-12-12 23:07         ` Niek Linnenbank
2019-12-12 23:25           ` Philippe Mathieu-Daudé
2019-12-13 20:45             ` Niek Linnenbank
2019-12-03  9:02 ` Philippe Mathieu-Daudé
2019-12-03 19:32   ` Niek Linnenbank
2019-12-06 14:16     ` Peter Maydell
2019-12-09 22:24       ` Aleksandar Markovic
2019-12-10 10:34 ` KONRAD Frederic
2019-12-10 19:55   ` Niek Linnenbank

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ce2125dd-004c-a5a2-81cf-b8aaae76444e@redhat.com \
    --to=philmd@redhat.com \
    --cc=b.galvani@gmail.com \
    --cc=nieklinnenbank@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git