Re: [Qemu-arm] [PATCH v2 4/7] bcm2835_peripherals: add rollup device for bcm2835 peripherals

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Wednesday, 30 December 2015 18:54
> On Wed, Dec 23, 2015 at 4:25 PM, Andrew Baumann
> <address@hidden> wrote:
> > This device maintains all the non-CPU peripherals on bcm2835 (Pi1)
> > which are also present on bcm2836 (Pi2). It also implements the
> > private address space used for DMA.
[...]
> > +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->peri_mr);
> > +
> > +    /* Internal memory region for peripheral bus addresses (not exported)
> */
> > +    memory_region_init_io(&s->gpu_bus_mr, OBJECT(s), NULL, s,
> "bcm2835_gpu_bus",
> > +                          (uint64_t)1 << 32);
> 
> You already have obj for the OBJECT casted version of s.
> 
> Why do you us memory_region_init_io though instead of
> memory_region_init? This looks like a container MR which is what
> unqualified memory_region_init provides.

Hmm, probably just copy-and-paste. If memory_region_init works, I'll use that.

> > +    object_property_add_child(obj, "gpu_bus", OBJECT(&s->gpu_bus_mr),
> NULL);
> > +
> > +    /* Internal memory region for communication of mailbox channel data
> */
> > +    memory_region_init_io(&s->mbox_mr, OBJECT(s), NULL, s,
> "bcm2835_mbox",
> > +                          MBOX_CHAN_COUNT << 4);
> > +
> 
> Same here. Although this container seems to have only one subregion at
> offset 0 (the property device). Long term, will this MR have more
> stuff in it or will always be just the property?

Yes, there are a couple more such devices to come. See:
https://github.com/0xabu/qemu/blob/raspi/hw/arm/bcm2835_peripherals.c#L263

[...]
> > +    /* XXX: assume that RAM is contiguous and mapped at system address
> zero */
> > +    ram = memory_region_find(get_system_memory(), 0, 1).mr;
> 
> Not sure about this. Alistair (from Xilinx) has a similar problem
> where the SoC needed to do a complex split of the single DDR into
> multiple system address regions. I cc'd you on a patch. Following
> Alistair's patch in the Xiinx ep108/zynqmp work, the system_memory
> region is now fully owned by the SoC level and disused by the
> machine/board level. I would suggest something similar here. The board
> is responsible for creating (and mainly sizing) the RAM, and links (or
> const-link's) the RAM to the SoC where it can be subregioned as-is
> without this search. As you already have SoC level memory regions for
> your multiple address spaces, you may actually end up with
> system_memory being completely unused (and that is OK).

Ok. There are different Pi boards with different ram sizes, so it seems the 
right thing to do is allocate the ram as I already do at the board level, but 
then pass it via a link property to the SoC (as Alistair does) and not bother 
with system_memory.

> > +    assert(ram != NULL && memory_region_size(ram) >= 128 * 1024 *
> 1024);
> 
> What is is the significance of the 128MB lower clamp? This should
> probably be a weaker error than assert. Depending on the reason for
> the error, it could be located in a number of different places. If the
> SoC cannot function without 128MB for some reason this is the right
> place.

This was just a paranoid check to make sure that my fragile search for the 
system memory found the right thing -- 128MB was just a lower bound on a sane 
memory size. It can go away.

[...]
> > +    /* Property channel */
> 
> These are all self-documented by the already-well-named s-> fields, so
> you can drop these comments indicating what you are realizing.

*shrug* I find them quite helpful when navigating the code. Plus I never really 
see the point of deleting (accurate, non-distracting) comments.

[...]
> > --- /dev/null
> > +++ b/include/hw/arm/raspi_platform.h
> > @@ -0,0 +1,161 @@
> > +/*
> > + * bcm2708 aka bcm2835/2836 aka Raspberry Pi/Pi2 SoC platform defines
> > + *
> > + * These definitions are derived from those in Linux at
> > + * arch/arm/mach-{bcm2708,bcm2709}/include/mach/platform.h
> 
> I think this is in the same basket as the header from P1. We should
> cherry pick out the defs that we care about (or are likely to care
> about in near term follow up). I'm guessing this is sourced from the
> same Linux fork as before?

Yes. I can drop some of the defs (e.g. redundant interrupts); however, assuming 
that we'll eventually have a full-fledged emulation, most of them will end up 
being needed, so I'd prefer not to have to throw them all away and then re-add 
them in later patches.

[...]
> > +#define ARM_IRQ2_BASE                  32
> 
> Can you just keep it as one big 64-long list? I don't think we need
> the _BASE defs at all anymore.

Yes, we could, but this helps diff-ability against the original file, and I 
really don't think it's that hard to parse.

[...]
> > +#define ARM_IRQ0_BASE                  64
> 
> This 64 shouldn't be needed anymore with the arm_irqs now in their own
> enumeration space. It should just count from 0 again (and I think the
> only usages you have of these subtract off ARM_IRQ0_BASE).

Agreed. I was planning to address this by setting ARM_IRQ0_BASE to 0 (again to 
keep the diff easily readable).

> > +#define INTERRUPT_ARM_TIMER            (ARM_IRQ0_BASE + 0)
> > +#define INTERRUPT_ARM_MAILBOX          (ARM_IRQ0_BASE + 1)
> > +#define INTERRUPT_ARM_DOORBELL_0       (ARM_IRQ0_BASE + 2)
> > +#define INTERRUPT_ARM_DOORBELL_1       (ARM_IRQ0_BASE + 3)
> > +#define INTERRUPT_VPU0_HALTED          (ARM_IRQ0_BASE + 4)
> > +#define INTERRUPT_VPU1_HALTED          (ARM_IRQ0_BASE + 5)
> > +#define INTERRUPT_ILLEGAL_TYPE0        (ARM_IRQ0_BASE + 6)
> > +#define INTERRUPT_ILLEGAL_TYPE1        (ARM_IRQ0_BASE + 7)
> 
> The defs after here are confusing. Are they for a different SoC? I
> thought the SoC was limited to 8 ARM_ irqs?

This seems to be an artifact of the way Linux manages the interrupt controller. 
Some of the GPU IRQs on the interrupt controller are aliased into another set 
of status bits where they can be checked in the same read as the ARM IRQs. It 
looks like Linux treats those as totally different interrupts. I'll just delete 
them from this file.

Andrew