On Thu, 2014-01-23 at 14:08 +0100, Alexander Graf wrote:
> On 21.01.2014, at 03:25, Scott Wood <scottw...@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 <ag...@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@h
> >> + ori r2, r2, CONFIG_QEMU_DT_ADDR@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).
> >> + /*
> >> + * 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.
> 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.
> >> +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.
> >> + /* 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().
> >> +#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.
> >> +/* 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?
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
