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

[Alexander Graf]

On 24.01.2014, at 02:39, Scott Wood <scottwood at freescale.com> wrote:

> 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?

Ok, I'll change it :)

Saving it from asm only doesn't work though because we end up accessing the fdt pointer inside of cpu_early_init_f() which clears the gd. I've made the fdt pointer a function argument now.

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

Bah - I'll just set all of them.

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

We don't modify the DT when passing it to Linux. That would mess up quite a number of things (SMP among others).

As for internal use, it's definitely close enough on everything I tested it on.

> 
>>>>> 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?

#ifndef CONFIG_FSL_CORENET
#if (defined(CONFIG_SYS_RAMBOOT) || defined(CONFIG_SPL)) && \
        !defined(CONFIG_SYS_INIT_L2_ADDR)
phys_size_t initdram(int board_type)
{
#if defined(CONFIG_SPD_EEPROM) || defined(CONFIG_DDR_SPD)
        return fsl_ddr_sdram_size();
#else
        return (phys_size_t)CONFIG_SYS_SDRAM_SIZE * 1024 * 1024;
#endif
}
#else /* CONFIG_SYS_RAMBOOT */
phys_size_t initdram(int board_type)
{
[...]
        dram_size = setup_ddr_tlbs(dram_size / 0x100000);
[...]

So because we set CONFIG_SYS_RAMBOOT we never call setup_ddr_tlbs() but instead have to do it ourselves in fsl_ddr_sdram_size(). I'll move the TLB fixup there.

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

Hrm, let me try that.


Alex