qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 7/7] raspi: add raspberry pi 2 machine


From: Andrew Baumann
Subject: Re: [Qemu-devel] [PATCH v3 7/7] raspi: add raspberry pi 2 machine
Date: Tue, 12 Jan 2016 23:53:05 +0000

> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Monday, 11 January 2016 19:58
[...]
> > +static void write_board_setup(ARMCPU *cpu, const struct arm_boot_info
> *info)
> 
> This is almost identical to Highbank, I'm guessing you are stubbing monitor
> firmware where you get away with nopping all the SMCs? Maybe we should
> split
> Highbanks version off to common code, and parameterise the few
> differences.
> 
> write_board_setup_dummy_monitior(ARMCPU *cpu ..., uint32_t scr_flags);
> 
> or something like it. Makes sense to be in arm/boot.c .

Actually, I added this only to make Linux happy (and yes, it was derived from 
highbank). Without it, I was seeing complaints about:
Ignoring attempt to switch CPSR_A flag from non-secure world with SCR.AW bit 
clear
Ignoring attempt to switch CPSR_F flag from non-secure world with SCR.FW bit 
clear

I don't believe anything actually uses the SMC handler after boot. I think it's 
just an architectural requirement to flush the change to non-secure mode.

I would prefer not to touch highbank, because I don't know how to test it. Is 
it better to submit this patch without the board setup?

> 
> > +{
> > +    static const uint32_t board_setup[] = {
> > +        /* MVBAR_ADDR: secure monitor vectors */
> > +        0xEAFFFFFE, /* (spin) */
> > +        0xEAFFFFFE, /* (spin) */
> > +        0xE1B0F00E, /* movs pc, lr ;SMC exception return */
> > +        0xEAFFFFFE, /* (spin) */
> > +        0xEAFFFFFE, /* (spin) */
> > +        0xEAFFFFFE, /* (spin) */
> > +        0xEAFFFFFE, /* (spin) */
> > +        0xEAFFFFFE, /* (spin) */
> > +        /* BOARDSETUP_ADDR */
> > +        0xE3A00B01, /* mov     r0, #0x400             ;MVBAR_ADDR */
> > +        0xEE0C0F30, /* mcr     p15, 0, r0, c12, c0, 1 ;set MVBAR */
> > +        0xE3A00031, /* mov     r0, #0x31              ;enable AW, FW, NS */
> > +        0xEE010F11, /* mcr     p15, 0, r0, c1, c1, 0  ;write SCR */
> 
> If combining with HB, could you do this as read-modify-write?
> 
> > +        0xE1A0100E, /* mov     r1, lr                 ;save LR across SMC 
> > */
> > +        0xE1600070, /* smc     #0                     ;monitor call */
> > +        0xE1A0F001, /* mov     pc, r1                 ;return */
> 
> I'm looking at the Highbank version which doesn't save lr across the SMC and
> wondering why it doesn't. Is this banked by CPU mode and do you get from
> already-in-monitor-mode? Or simply, that Highbank code may have a bug?

I think it's needed because I call the board setup blob on each core (from the 
smpboot blob), but highbank doesn't. I found that I needed to do this to avoid 
the above warnings on an SMP boot; I don't know why highbank doesn't.

[...]
> > +    /* Allocate and map RAM */
> > +    memory_region_allocate_system_memory(&s->ram,
> OBJECT(machine), "ram",
> > +                                         machine->ram_size);
> > +    memory_region_add_subregion_overlap(get_system_memory(), 0,
> &s->ram, 0);
> 
> I thought the SoC handled this now? Why do you need to add to
> system_memory?

If I don't map it here, how do RAM accesses from the CPUs work?

Or are you saying that I should still do this, but do it in the SoC not the 
board level?

[...]
> > +static void raspi2_machine_init(MachineClass *mc)
> > +{
> > +    mc->desc = "Raspberry Pi 2";
> > +    mc->init = raspi2_init;
> > +    mc->block_default_type = IF_SD;
> 
> > +    mc->no_parallel = 1;
> > +    mc->no_floppy = 1;
> > +    mc->no_cdrom = 1;
> 
> Curious, what do these do from a user-visible point of view? Maybe we
> should
> add them to more ARM boards as they certainly make sense.

I think they turn off some redundant stuff in the UI (e.g., there's no 
View->Parallel menu option). I'm guessing they also disable the -cdrom and  
-fd* options, but didn't test that.

Cheers,
Andrew



reply via email to

[Prev in Thread] Current Thread [Next in Thread]