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

[Alexander Graf]

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. And how do I get offsets into the gd structure from asm in the first place?

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 :).

> 
>>>> +		/*
>>>> +		 * We already know where the initrd is inside the dtb, so no
>>>> +		 * need to override it
>>>> +		 */
>>>> +			setenv("ramdisk_addr", "-");
>>> 
>>> Indentation.
>>> 
>>> What if the user wants to specify the initrd from within U-Boot?  This
>>> should be handled via the default environment, not by overriding the
>>> user's choice.
>> 
>> I'm not sure I see the difference. We have the following default boot script:
>> 
>> #define CONFIG_BOOTCOMMAND             \
>>       "test -n \"$kernel_addr\" && bootm $kernel_addr $ramdisk_addr $fdt_addr\0"
>> 
>> 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?

> 
>> Maybe I should rename kernel_addr to qemu_kernel_addr and drop
>> ramdisk_addr completely in favor for a - in the bootm command? Yeah,
>> I'll do that :).
> 
> OK.
> 
>>>> +	ret = fdt_open_into((void*)dt_base, fdt, dt_size);
>>>> +	if (ret)
>>>> +		panic("Couldn't open fdt");
>>>> +
>>>> +	memory = fdt_path_offset(fdt, "/memory");
>>>> +	prop = fdt_getprop(fdt, memory, "reg", &len);
>>>> +
>>>> +	if (prop && len >= 16)
>>>> +		return *(uint64_t*)(prop+8);
>>>> +
>>>> +	panic("Couldn't determine RAM size");
>>>> +}
>>> 
>>> Again, there's no need to create a temporary fdt copy every time you
>>> want to read from it.
>>> 
>>> What if memory doesn't start at zero (e.g. for e500v2 VFIO)?
>> 
>> In that case we're pretty broken regardless of determining the correct memory size. Can u-boot handle that case at all? How would this work?
> 
> U-Boot can handle this on other architectures.  Not sure about the PPC
> code.

It's handled through preprocessor configuration magic again. I'll leave that for later though.

> 
>>>> +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.

> 
>>>> +	/* Enter AS=1 */
>>>> +	asm volatile(
>>>> +		"mfmsr	%0			\n"
>>>> +		"ori	%0, %0, 0x30		\n"
>>>> +		"mtsrr1	%0			\n"
>>>> +		"bl	1f			\n"
>>>> +		"1:				\n"
>>>> +		"mflr	%0			\n"
>>>> +		"addi	%0, %0, 16		\n"
>>>> +		"mtsrr0	%0			\n"
>>>> +		"rfi				\n"
>>>> +	: "=r"(tmp) : : "lr");
>>>> +
>>>> +	/* Now we live in AS=1, so we can flush all old maps */
>>>> +	clear_ddr_tlbs(ram_size >> 20);
>>>> +	/* and create new ones */
>>>> +	setup_ddr_tlbs(ram_size >> 20);
>>>> +
>>>> +	/* Now we can safely go back to AS=0 */
>>>> +	asm volatile(
>>>> +		"mfmsr	%0			\n"
>>>> +		"subi	%0, %0, 0x30		\n"
>>>> +		"mtsrr1	%0			\n"
>>>> +		"bl	1f			\n"
>>>> +		"1:				\n"
>>>> +		"mflr	%0			\n"
>>>> +		"addi	%0, %0, 16		\n"
>>>> +		"mtsrr0	%0			\n"
>>>> +		"rfi				\n"
>>>> +	: "=r"(tmp) : : "lr");
>>>> +
>>>> +	/* And remove our AS=1 map */
>>>> +	disable_tlb(13);
>>>> +}
>>> 
>>> 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.

> 
>>>> +#undef CONFIG_RESET_VECTOR_ADDRESS
>>>> +#define CONFIG_RESET_VECTOR_ADDRESS	(0x1000000 - 4) /* 16 MB */
>>> 
>>> Does QEMU begin execution here, or at the ELF entry point?
>> 
>> At the ELF entry point. But I need to define this to something.
> 
> Set CONFIG_SYS_MPC85XX_NO_RESETVEC.

Sounds easy, but doesn't boot anymore now. I guess I'll need to figure out where in that fragile TLB setup mess it fails this time.

> 
>>>> +/* CONFIG_NUM_DDR_CONTROLLERS is defined in include/asm/config_mpc85xx.h */
>>>> +#define CONFIG_DIMM_SLOTS_PER_CTLR	0
>>>> +#define CONFIG_CHIP_SELECTS_PER_CTRL	0
>>> 
>>> Why are we including code that cares about this?
>> 
>> In file included from cpu.c:25:0:
>> /root/u-boot/include/fsl_ddr_sdram.h:183:7: error: 'CONFIG_CHIP_SELECTS_PER_CTRL' undeclared here (not in a function)
>> 
>> So we don't include code, but we include a header that cares.
> 
> I see.  Would be nice to clean that up at some point, but OK for now.
> 
>>>> +/* 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.


Alex