On 24.01.2014, at 01:49, Scott Wood <scottw...@freescale.com> wrote:
> 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). 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot