Re: [Qemu-devel] [PATCH] raspi: Add Raspberry Pi 1 support

[Andrew Baumann]

> From: Omar Rizwan [mailto:address@hidden
> Sent: Friday, 7 April 2017 23:13
> On Fri, Apr 7, 2017 at 10:57 PM Andrew Baumann
> <address@hidden> wrote:
> > > From: Omar Rizwan [mailto:address@hidden
> > > Sent: Friday, 7 April 2017 22:43

> > Did you do any testing of this? One of the reasons I never got around to
> upstreaming this was that I couldn't get Raspbian working reliably (there was
> some problem with stalled DMA reads from the MMC controller), and I didn't
> have the cycles to debug it further. Also, IIRC Raspbian on pi1 depended on 
> the
> system timer devices, so it might not make sense to have this board-level
> support without adding that first.
> 
> I did test Raspbian on the raspi machine and maybe encountered the
> same issue you did (or, more likely, a systimer related issue);
> it would just hang in a loop with program counter at 0xC000A000.
> 
> But my use for this is bare-metal code, which works OK.
> (Is there some standard that the machine should run Linux before
> getting upstreamed?)

No, I don't know of such a standard, but it's probably worth noting the 
limitations in the commit message. And it might even be worth putting the 
systimers first...

> I'd like to upstream the system timer next, anyway.

Sounds good.

> > > --- a/hw/arm/bcm2835_peripherals.c
> > > +++ b/hw/arm/bcm2835_peripherals.c
> > > @@ -34,6 +34,7 @@ static void bcm2835_peripherals_init(Object *obj)
> > >      /* Internal memory region for peripheral bus addresses (not 
> > > exported) */
> > >      memory_region_init(&s->gpu_bus_mr, obj, "bcm2835-gpu", (uint64_t)1
> <<
> > > 32);
> > >      object_property_add_child(obj, "gpu-bus", OBJECT(&s->gpu_bus_mr),
> > > NULL);
> > > +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->gpu_bus_mr);
> > >
> > >      /* Internal memory region for request/response communication with
> > >       * mailbox-addressable peripherals (not exported)
> >
> > This line looks like it might have snuck in by accident?
> 
> 
> Do you recall why that line was removed? bcm2835_realize fails without
> it, because the second sysbus_mmio_map_overlap call
> 
> sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 1,
>                             0x40000000, 1);
> 
> fails this assertion in sysbus.c, in sysbus_mmio_map_common:
> 
> assert(n >= 0 && n < dev->num_mmio);
> 
> I can't easily find documentation for that 0x40000000 memory mapping,
> which I figured I'd keep in; should it just be removed from bcm2835?
> Am I misunderstanding something?

The various mapping aliases are intended to implement the bus addresses from 
S1.2.3 / figure on page 5 of this document:
https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

In ARM CPU physical addresses, the RAM is mapped at 0, and the peripherals at 
0x20000000

In VC CPU bus addresses (which get used for DMA), RAM is aliased four times, at 
0 0x40000000 0x80000000 and 0xC0000000. Peripherals are also aliased four times 
at a fixed offset of 0x3e000000 from the base of the RAM alias, which is how 
you get the 0x7e000000 peripheral base (0x40000000 + 0x3e000000).

This is implemented as follows: bcm2835_peripherals implements/exports one 
memory region "peri_mr" which contains just the peripherals. It also builds a 
private MR for the GPU ("VC CPU" in the spec) bus addresses, "gpu_mr", and 
aliases RAM into it four times along with the peripheral MR at 
BCM2835_VC_PERI_BASE (0x7e000000). Its parent, device, either bcm2835 or 
bcm2836, maps the peripheral MR into the default sysbus mmio bus (i.e., the 
CPU's physical address space) at the relevant CPU physical base address 
(0x20000000 on pi1, 0x3F000000 on pi2).

Now, as to why the mapping call fails: I'm confused where you saw that call 
with the 0x40000000 address. The code in my tree for bcm2835.c is:

/* Peripheral base address seen by the CPU */
#define BCM2835_PERI_BASE       0x20000000
...
    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
                            BCM2835_PERI_BASE, 1);

Also, note that this is mapping the peripherals MR, not the the gpu_bus_mr 
which is never used with any sysbus APIs, so I'm still not clear on why we need 
to call sysbus_init_mmio on it. It's entirely possible my code is broken, 
and/or that assert wasn't there when I tested it, but you might have to dig a 
bit deeper to convince me that what you're doing is also sane :)

BTW, I suspect nobody else will want to review this until after 2.9.

Andrew