QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Niek Linnenbank <nieklinnenbank@gmail.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Beniamino Galvani <b.galvani@gmail.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine
Date: Tue, 10 Dec 2019 20:14:46 +0100
Message-ID: <CAPan3Wru7AnjmeB5DWwgD0sDA2L1emZdMJLD2-d1g0cQH5fEOA@mail.gmail.com> (raw)
In-Reply-To: <f5983dbb-28f6-206f-a180-83633a049325@redhat.com>


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

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

-- 
Niek Linnenbank

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

  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é
2019-12-06 22:15         ` Niek Linnenbank
2019-12-10  8:59           ` Philippe Mathieu-Daudé
2019-12-10 19:14             ` Niek Linnenbank [this message]
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=CAPan3Wru7AnjmeB5DWwgD0sDA2L1emZdMJLD2-d1g0cQH5fEOA@mail.gmail.com \
    --to=nieklinnenbank@gmail.com \
    --cc=b.galvani@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=philmd@redhat.com \
    --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