[U-Boot] [PATCH 3/3] PPC 85xx: Add qemu-ppce500 machine

[Scott Wood]

On Fri, 2014-01-24 at 02:25 +0100, Alexander Graf wrote:
> On 24.01.2014, at 01:49, Scott Wood <scottwood at freescale.com> wrote:
> 
> > On Thu, 2014-01-23 at 14:08 +0100, Alexander Graf wrote:
> >> On 21.01.2014, at 03:25, Scott Wood <scottwood at freescale.com> wrote:
> >> 
> >>> On Sun, 2014-01-19 at 16:19 +0100, Alexander Graf wrote:
> >>>> For KVM we have a special PV machine type called "ppce500". This machine
> >>>> is inspired by the MPC8544DS board, but implements a lot less features
> >>>> than that one.
> >>>> 
> >>>> It also provides more PCI slots and is supposed to be enumerated by
> >>>> device tree only.
> >>>> 
> >>>> This patch adds support for the current generation ppce500 machine as
> >>>> it is implemented today.
> >>>> 
> >>>> Signed-off-by: Alexander Graf <agraf at suse.de>
> >>>> ---
> >>>> arch/powerpc/cpu/mpc85xx/start.S            |    7 +
> >>>> arch/powerpc/include/asm/config_mpc85xx.h   |    4 +
> >>>> board/freescale/qemu-ppce500/Makefile       |   10 ++
> >>>> board/freescale/qemu-ppce500/qemu-ppce500.c |  260 +++++++++++++++++++++++++++
> >>>> board/freescale/qemu-ppce500/tlb.c          |   59 ++++++
> >>>> boards.cfg                                  |    1 +
> >>>> include/configs/qemu-ppce500.h              |  235 ++++++++++++++++++++++++
> >>>> 7 files changed, 576 insertions(+)
> >>>> create mode 100644 board/freescale/qemu-ppce500/Makefile
> >>>> create mode 100644 board/freescale/qemu-ppce500/qemu-ppce500.c
> >>>> create mode 100644 board/freescale/qemu-ppce500/tlb.c
> >>>> create mode 100644 include/configs/qemu-ppce500.h
> >>>> 
> >>>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S
> >>>> index db84d10..ccbcc03 100644
> >>>> --- a/arch/powerpc/cpu/mpc85xx/start.S
> >>>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
> >>>> @@ -80,6 +80,13 @@ _start_e500:
> >>>> 	li	r1,MSR_DE
> >>>> 	mtmsr 	r1
> >>>> 
> >>>> +#ifdef CONFIG_QEMU_E500
> >>>> +	/* Save our ePAPR device tree off before we clobber it */
> >>>> +	lis	r2, CONFIG_QEMU_DT_ADDR at h
> >>>> +	ori	r2, r2, CONFIG_QEMU_DT_ADDR at l
> >>>> +	stw	r3, 0(r2)
> >>>> +#endif
> >>> 
> >>> r2 has a special purpose -- please don't use it for other things, even
> >>> if it's too early to be a problem and other code is similarly bad.
> >> 
> >> Heh, ok. I'll use r4 instead.
> >> 
> >>> Instead of saving the DT pointer in some random hardcoded address, how
> >>> about sticking it in a callee-saved register until you're able to store
> >>> it in the global data struct?
> >> 
> >> I did that at first but that didn't survive relocation. However, I just
> >> remembered that I had my global variable in bss, so maybe relocation
> >> doesn't work properly there. Instead I put it in .data now and that
> >> seems to work.
> >> 
> >> It's certainly the nicer solution, I agree.
> > 
> > I don't mean a global variable, but a field in the global data struct
> > (gd_t).  BSS should not be accessed, and global variables should not be
> > written, before relocation (although you may get away with the latter
> > pre-relocation in ramboot cases, you still shouldn't).
> 
> But the global data gets cleared in cpu_init_early_f(). So I'd have to
> shove it to some non-volatile, make sure no other code uses that
> particular register and then move it into gd.

Right.

> And how do I get offsets into the gd structure from asm in the first place?

See lib/asm-offsets.c

> Or are you suggesting I move r3 into r2x, leave it there until after
> cpu_init_early_f(), copy it back to r3 and then call a C helper that
> puts it into gd? Sounds quite excessive for something that works quite
> well with a global variable in .data :).

It only works because you are booting directly from RAM, and the
variable you're writing to isn't subject to relocation, and isn't in the
BSS (unlike most uninitialized/zero-initialized data).  It's not good
practice.  What if you later want to simulate NOR flash and boot from
that?

> >> which is the only place where we actually use $ramdisk_addr. If a user
> >> wants to specify the initrd within U-Boot he can do that easily because
> >> he would specify his own boot command anyway, right?
> > 
> > Oh.  I was confusing it with the existing $ramdiskaddr.  So I'm changing
> > my objection to the fact that it's confusing. :-)
> > 
> > Likewise $fdt_addr versus $fdtaddr.
> 
> Mind to explain what the difference is between $fdtaddr, $fdt_addr (pxe boot?) and $fdt_addr_r? Which one should I set?

I don't know what the "_r" is supposed to mean, but the underscore seems
to just be something that varies from board to board.  Our boards use
$fdtaddr and until now I didn't realize other boards do it differently.
 
> >>>> +unsigned long
> >>>> +get_board_sys_clk(ulong dummy)
> >>>> +{
> >>>> +	/* The actual clock doesn't matter in a PV machine */
> >>>> +	return 33333333;
> >>>> +}
> >>> 
> >>> s/doesn't matter/doesn't exist/
> >>> 
> >>> Where is this used from?  Can it be skipped for this target?  Is the CPU
> >>> timebase handled properly?
> >> 
> >> Turns out it's not required at all. Must have been a dependency of something I threw away in between.
> > 
> > OK.  I'm still curious about how the timebase frequency is handled.
> 
> Incorrectly. The question is whether it matters.

Incorrectly for U-Boot's own use?  Incorrectly for passing to Linux?

The latter case definitely matters.  The former case would affect the
boot delay, so it needs to be at least reasonably close.
 
> >>> Why aren't we already in AS1?  What code are you replacing with this?
> >> 
> >> I honestly don't know how it's supposed to work at all usually.
> > 
> > Usually we enter AS1 from start.S during early boot (see switch_as), and
> > return to AS0 later in start.S (see _start_cont) after calling
> > cpu_init_early_f().
> 
> Right, but the only function that gets called in AS=1 is
> cpu_init_early_f() which is CPU, not board specific. I really don't
> want to mess with common code too much unless I'm 100% sure I don't
> break it and it doesn't break me next time.
> 
> So I need to run this from AS=0 context, which means we need to quickly
> go out of AS=0, modify the AS=0 map and go back into AS=0.

What sort of messing would you need to do to cpu_init_early_f()?  What
part of the normal workflow breaks under QEMU?

> >>>> +/* Get RAM size from device tree */
> >>>> +#define CONFIG_DDR_SPD
> >>>> +
> >>>> +#define CONFIG_SYS_CLK_FREQ        33000000
> >>> 
> >>> Likewise.  BTW, this made up sysclock frequency doesn't match the one
> >>> you made up elsewhere.
> >> 
> >> speed.c: In function 'get_sys_info':
> >> speed.c:347:42: error: 'CONFIG_SYS_CLK_FREQ' undeclared (first use in this function)
> > 
> > What do we need from speed.c?
> 
> Nothing really, but it gets included unconditionally in arch/powerpc/cpu/mpc85xx/Makefile so I can't override it with my own stubs.

You could stick an ifndef CONFIG_QEMU_E500 in there I guess, or define
it to be zero and make sure you don't call any of those functions that
depend on it.

-Scott