On 21.01.2014, at 03:25, Scott Wood scottwood@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@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.

...

diff --git a/arch/powerpc/include/asm/config_mpc85xx.h b/arch/powerpc/include/asm/config_mpc85xx.h index 54ce2f0..a0ab453 100644 --- a/arch/powerpc/include/asm/config_mpc85xx.h +++ b/arch/powerpc/include/asm/config_mpc85xx.h @@ -776,6 +776,10 @@ defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022) #define CONFIG_SYS_CCSRBAR_DEFAULT 0xff700000 #define CONFIG_SYS_FSL_ERRATUM_A005125

+#elif defined(CONFIG_QEMU_E500) +#define CONFIG_MAX_CPUS 1 +#define CONFIG_SYS_CCSRBAR_DEFAULT 0xe0000000

This is supposed to come from the device tree. We will want to change that address eventually to support larger guest memory.

That's a lot easier said than done. There is a lot of code in u-boot that puts the CCSRBAR or addresses derived from CCSRBAR into arrays which fails miserably when you make it a variable.

It's certainly a reasonably big task.

...

diff --git a/board/freescale/qemu-ppce500/qemu-ppce500.c b/board/freescale/qemu-ppce500/qemu-ppce500.c new file mode 100644 index 0000000..c6c4b4a --- /dev/null +++ b/board/freescale/qemu-ppce500/qemu-ppce500.c @@ -0,0 +1,260 @@ +/*

• • Copyright 2007,2009-2014 Freescale Semiconductor, Inc.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <command.h> +#include <pci.h> +#include <asm/processor.h> +#include <asm/mmu.h> +#include <asm/immap_85xx.h> +#include <asm/fsl_pci.h> +#include <asm/io.h> +#include <miiphy.h> +#include <libfdt.h> +#include <fdt_support.h> +#include <netdev.h> +#include <fdtdec.h> +#include <errno.h> +#include <malloc.h>

Do you really need all these headers? miiphy?

Ah, I missed that one during my header cleaning :). Reved that and immap_85xx.h.

...

+int checkboard(void) +{

• return 0;

+}

+static struct pci_controller pci1_hose;

+void pci_init_board(void) +{

• volatile ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
• struct fsl_pci_info pci_info;
• u32 devdisr, pordevsr;
• u32 porpllsr, pci_agent, pci_speed, pci_32, pci_arb, pci_clk_sel;
• int first_free_busno = 0;
• devdisr = in_be32(&gur->devdisr);
• pordevsr = in_be32(&gur->pordevsr);
• porpllsr = in_be32(&gur->porpllsr);

Why are you reading registers that don't exist in QEMU?

Dropped

...

+int last_stage_init(void) +{

• int ret;
• int len = 0;
• const void *prop;
• int chosen;

•    uint32_t *dt_base_ptr = (void*)CONFIG_QEMU_DT_ADDR;
  

Whitespace

...

• uint32_t dt_base = *dt_base_ptr;
• uint32_t dt_size;
• void *fdt;
• dt_size = fdt_totalsize((void*)dt_base);

Please put a space before the * in casts.

...

• /* Reserve 4k for dtb tweaks */
• dt_size += 4096;
• fdt = malloc(dt_size);
• /* Open device tree */
• ret = fdt_open_into((void*)dt_base, fdt, dt_size);
• if (ret) {

• printf("Couldn't open fdt at %x\n", dt_base);
  

• return -EIO;
  

• }
• chosen = fdt_path_offset(fdt, "/chosen");
• if (chosen < 0) {

• printf("Couldn't find /chosen node in fdt\n");
  

• return -EIO;
  

• }
• prop = fdt_getprop(fdt, chosen, "qemu,boot-kernel", &len);
• if (prop && (len >= 8)) {

• /* -kernel boot */
  

• setenv_hex("kernel_addr", *(uint64_t*)prop);
  

Don't cast away the const. This looks like the only place you use prop; why not make it uint64_t to begin with?

Good idea. Fixed.

...

• /*
  

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

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

...

• }
• /* Give the user a variable for the host fdt */
• setenv_hex("fdt_addr", (int)dt_base);

Unnecessary cast.

...

• free(fdt);
• return 0;

+}

Why is the device tree handling here different from anything else we already have? In particular, why do you create a temporary fdt (with space for tweaks that never happen) just to read from it?

Because I had no idea :). Changed to directly use the in-memory fdt.

...

+static uint64_t get_linear_ram_size(void) +{

• const void *prop;
• int memory;
• int len;

•    uint32_t *dt_base_ptr = (void*)CONFIG_QEMU_DT_ADDR;
  

Whitespace.

...

• void *fdt = &dt_base_ptr[1];

• int ret;

• uint32_t dt_base = *dt_base_ptr;

• uint32_t dt_size = 1024 * 1024;

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

...

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

...

+int board_phy_config(struct phy_device *phydev) +{

• return 0;

+}

Why?

mpc8544ds is the only board in boards/freescale that has this, so it's clearly optional, and you clearly don't need it...

Just because QEMU started with mpc8544ds doesn't mean this needs to be a copy-and-paste of the U-Boot mpc8544ds code. In fact it's better if it isn't to reduce the likelihood of accidentally depending on hardcoded addresses and such.

Yup. I just had to start from somewhere :). Removed.

...

+int board_eth_init(bd_t *bis) +{

• return pci_eth_init(bis);

+}

+#if defined(CONFIG_OF_BOARD_SETUP) +void ft_board_setup(void *blob, bd_t *bd) +{

• FT_FSL_PCI_SETUP;

+} +#endif

+void print_laws(void) +{

• /* We don't emulate LAWs yet */

+}

+static void fixup_tlb1(uint64_t ram_size) +{

• u32 mas0, mas1, mas2, mas3, mas7;
• long tmp;
• /* Flush TLB0 - it only contains stale maps by now */
• invalidate_tlb(0);
• /* Create a temporary AS=1 map for ourselves */

•    mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(13);
  

•    mas1 = MAS1_VALID | MAS1_TID(0) | MAS1_TS | MAS1_TSIZE(BOOKE_PAGESZ_64M);
  

•    mas2 = FSL_BOOKE_MAS2(0, 0);
  

•    mas3 = FSL_BOOKE_MAS3(CONFIG_SYS_DDR_SDRAM_BASE, 0,
  

• 	      MAS3_SW|MAS3_SR|MAS3_SX);
  

•    mas7 = FSL_BOOKE_MAS7(CONFIG_SYS_DDR_SDRAM_BASE);
  

•    write_tlb(mas0, mas1, mas2, mas3, mas7);
  

Whitespace. Please run scripts/checkpatch.pl.

I shouldn't copy&paste I suppose :).

...

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

...

+phys_size_t fixed_sdram(void) +{

• uint64_t ram_size;
• ram_size = get_linear_ram_size();
• /*

• * At this point we know that we're running in RAM, but within the
  

• * first 64MB because we don't have a mapping for anything else.
  

• *
  

• * Replace that mapping with real maps for our full RAM range.
  

• */
  

• fixup_tlb1(ram_size);
• return ram_size;

+}

+phys_size_t fsl_ddr_sdram_size(void) +{

• return fixed_sdram();

+}

+void init_laws(void) +{

• /* We don't emulate LAWs yet */

+}

+int set_next_law(phys_addr_t addr, enum law_size sz, enum law_trgt_if id) +{

• /* We don't emulate LAWs yet */
• return 0;

+}

What is calling set_next_law()?

Nothing anymore apparently. Removed.

...

diff --git a/board/freescale/qemu-ppce500/tlb.c b/board/freescale/qemu-ppce500/tlb.c new file mode 100644 index 0000000..cdc7013 --- /dev/null +++ b/board/freescale/qemu-ppce500/tlb.c @@ -0,0 +1,59 @@ +/*

• • Copyright 2008 Freescale Semiconductor, Inc.
• • (C) Copyright 2000
• • Wolfgang Denk, DENX Software Engineering, wd@denx.de.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <asm/mmu.h>

+struct fsl_e_tlb_entry tlb_table[] = {

• /* TLB 0 - for temp stack in cache */
• SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR, CONFIG_SYS_INIT_RAM_ADDR,

•       MAS3_SX|MAS3_SW|MAS3_SR, 0,
  

•       0, 0, BOOKE_PAGESZ_4K, 0),
  

• SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 4 * 1024 , CONFIG_SYS_INIT_RAM_ADDR + 4 * 1024,

•       MAS3_SX|MAS3_SW|MAS3_SR, 0,
  

•       0, 0, BOOKE_PAGESZ_4K, 0),
  

• SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 8 * 1024 , CONFIG_SYS_INIT_RAM_ADDR + 8 * 1024,

•       MAS3_SX|MAS3_SW|MAS3_SR, 0,
  

•       0, 0, BOOKE_PAGESZ_4K, 0),
  

• SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 12 * 1024 , CONFIG_SYS_INIT_RAM_ADDR + 12 * 1024,

•       MAS3_SX|MAS3_SW|MAS3_SR, 0,
  

•       0, 0, BOOKE_PAGESZ_4K, 0),
  

Isn't this just normal RAM? Thus, aren't you creating overlapping TLB entries here?

Indeed. Removed.

...

• /*

• * TLB 0:	64M	Cacheable
  

• * 0x00000000	64M	Covers RAM at 0x00000000
  

• */
  

• SET_TLB_ENTRY(1, CONFIG_SYS_BOOT_BLOCK, CONFIG_SYS_BOOT_BLOCK,

•       MAS3_SX|MAS3_SW|MAS3_SR, 0,
  

•       0, 0, BOOKE_PAGESZ_64M, 1),
  

• /*

• * TLB 1:	256M	Non-cacheable, guarded
  

• */
  

• SET_TLB_ENTRY(1, CONFIG_SYS_PCI_VIRT, CONFIG_SYS_PCI_PHYS,

•       MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
  

•       0, 1, BOOKE_PAGESZ_256M, 1),
  

• /*

• * TLB 2:	256M	Non-cacheable, guarded
  

• */
  

• SET_TLB_ENTRY(1, CONFIG_SYS_PCI_VIRT + 0x10000000, CONFIG_SYS_PCI_PHYS + 0x10000000,

•       MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
  

•       0, 2, BOOKE_PAGESZ_256M, 1),
  

• /*

• * TLB 3:	64M	Non-cacheable, guarded
  

• * 0xe000_0000	1M	CCSRBAR
  

• * 0xe100_0000	255M	PCI IO range
  

• */
  

• SET_TLB_ENTRY(1, CONFIG_SYS_CCSRBAR, CONFIG_SYS_CCSRBAR_PHYS,

•       MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
  

•       0, 3, BOOKE_PAGESZ_64M, 1),
  

+};

These should not be executable.

Aww :). Fixed.

...

+int num_tlb_entries = ARRAY_SIZE(tlb_table); diff --git a/boards.cfg b/boards.cfg index d177f82..ab50982 100644 --- a/boards.cfg +++ b/boards.cfg @@ -986,6 +986,7 @@ Active powerpc mpc85xx - freescale t2080qds Active powerpc mpc85xx - freescale t2080qds T2080QDS_SPIFLASH T2080QDS:PPC_T2080,RAMBOOT_PBL,SPIFLASH,SYS_TEXT_BASE=0xFFF80000 Active powerpc mpc85xx - freescale t2080qds T2080QDS_NAND T2080QDS:PPC_T2080,RAMBOOT_PBL,NAND,SYS_TEXT_BASE=0xFFF80000 Active powerpc mpc85xx - freescale t2080qds T2080QDS_SRIO_PCIE_BOOT T2080QDS:PPC_T2080,SRIO_PCIE_BOOT_SLAVE,SYS_TEXT_BASE=0xFFF80000 +Active powerpc mpc85xx - freescale qemu-ppce500 qemu-ppce500 - Alexander Graf agraf@suse.de Active powerpc mpc85xx - gdsys p1022 controlcenterd_36BIT_SDCARD controlcenterd:36BIT,SDCARD Dirk Eibach eibach@gdsys.de Active powerpc mpc85xx - gdsys p1022 controlcenterd_36BIT_SDCARD_DEVELOP controlcenterd:36BIT,SDCARD,DEVELOP Dirk Eibach eibach@gdsys.de Active powerpc mpc85xx - gdsys p1022 controlcenterd_TRAILBLAZER controlcenterd:TRAILBLAZER,SPIFLASH Dirk Eibach eibach@gdsys.de diff --git a/include/configs/qemu-ppce500.h b/include/configs/qemu-ppce500.h new file mode 100644 index 0000000..981b016 --- /dev/null +++ b/include/configs/qemu-ppce500.h @@ -0,0 +1,235 @@ +/*

• • Copyright 2011-2014 Freescale Semiconductor, Inc.
• • SPDX-License-Identifier: GPL-2.0+
• */

+/*

• • Corenet DS style board configuration file
• */

+#ifndef __QEMU_PPCE500_H +#define __QEMU_PPCE500_H

+#define CONFIG_CMD_REGINFO

+/* High Level Configuration Options */ +#define CONFIG_BOOKE +#define CONFIG_E500 /* BOOKE e500 family */ +#define CONFIG_MPC85xx /* MPC85xx/PQ3 platform */ +#define CONFIG_QEMU_E500

+#undef CONFIG_SYS_TEXT_BASE +#define CONFIG_SYS_TEXT_BASE 0xf00000 /* 15 MB */

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

...

+#define CONFIG_SYS_RAMBOOT

+#define CONFIG_PCI /* Enable PCI/PCIE */ +#define CONFIG_PCI1 1 /* PCI controller 1 */ +#define CONFIG_FSL_PCI_INIT /* Use common FSL init code */ +#define CONFIG_SYS_PCI_64BIT /* enable 64-bit PCI resources */

+#define CONFIG_ENV_OVERWRITE +#define CONFIG_INTERRUPTS /* enable pci, srio, ddr interrupts */

SRIO and DDR interrupts?

Why do we need CONFIG_INTERRUPTS at all?

We don't. Removed.

...

+#define CONFIG_SYS_CCSRBAR 0xe0000000 +#define CONFIG_SYS_CCSRBAR_PHYS_LOW CONFIG_SYS_CCSRBAR

Again, shouldn't be hardcoded (and must equal the default since QEMU doesn't currently support relocating CCSR)

Yes.

...

+#define CONFIG_FW_CFG_ADDR 0xFF7FFFFC

Where did this come from? If this is a new paravirt device please advertise it through the device tree rather than a hardcoded address. And if it must be a hardcoded address, please put it well above 4G.

It's a leftover from a time when I used fw_cfg to fetch the dt pointer. Removed.

...

+#define CONFIG_QEMU_DT_ADDR ((2 * 1024 * 1024) - 4) /* below 2MB */

This is U-Boot-internal and not something hardcoded in QEMU, right?

Yes, and it's gone now :).

...

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

...

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

...

+/*

• • IFC Definitions
• */

There is no IFC.

Removed.

...

+#define CONFIG_SYS_NO_FLASH

+#define CONFIG_SYS_BOOT_BLOCK 0x00000000 /* boot TLB */ +#define CONFIG_SYS_FLASH_BASE 0xff800000 /* start of FLASH 8M */ +#define CONFIG_SYS_FLASH_BASE_PHYS (0xf00000000ull | CONFIG_SYS_FLASH_BASE) +#define CONFIG_SYS_MAX_FLASH_BANKS 1 /* number of banks */ +#define CONFIG_SYS_MAX_FLASH_SECT 128 /* sectors per device */

NO_FLASH, but flash starts at 0xff800000?

Yeah, I was experimenting with putting up a flash there. Removed.

...

+#define CONFIG_SYS_MONITOR_BASE (CONFIG_RESET_VECTOR_ADDRESS - 0x100000 + 4) +#define CONFIG_SYS_SDRAM_SIZE 64

Why hardcoded to 64 MiB?

The define wasn't needed anymore. Removed.

...

+/*

• • General PCI
• • Memory space is mapped 1-1, but I/O space must start from 0.
• */

+#define CONFIG_SYS_PCI_VIRT 0xc0000000 /* 512M PCI TLB */ +#define CONFIG_SYS_PCI_PHYS 0xc0000000 /* 512M PCI TLB */

+#define CONFIG_SYS_PCI1_MEM_VIRT 0xc0000000 +#define CONFIG_SYS_PCI1_MEM_BUS 0xc0000000 +#define CONFIG_SYS_PCI1_MEM_PHYS 0xc0000000 +#define CONFIG_SYS_PCI1_MEM_SIZE 0x20000000 /* 512M */ +#define CONFIG_SYS_PCI1_IO_VIRT 0xe1000000 +#define CONFIG_SYS_PCI1_IO_BUS 0x00000000 +#define CONFIG_SYS_PCI1_IO_PHYS 0xe1000000 +#define CONFIG_SYS_PCI1_IO_SIZE 0x00010000 /* 64k */

I assume U-Boot will reprogram PCI based on this, but does QEMU support that for I/O (for memory IIRC it completely ignores the ATMU)? Don't rely on the QEMU address not changing.

It populated its PCI accessor struct based on this. This ideally should be dt based. But for now we have something that works at all :).

...

+/*

• • Environment
• */

+#define CONFIG_ENV_SIZE 0x2000 +#define CONFIG_ENV_SECT_SIZE 0x10000 /* 64K (one sector) */

Even though ENV_IS_NOWHERE. :-)

Removed :)

...

+#define CONFIG_LOADS_ECHO /* echo on for serial download */ +#define CONFIG_SYS_LOADS_BAUD_CHANGE /* allow baudrate change */

Baud rate?

Removed.

...

+#define CONFIG_CMD_DHCP +#define CONFIG_CMD_ELF +#define CONFIG_CMD_BOOTZ +#define CONFIG_CMD_ERRATA +#define CONFIG_CMD_GREPENV +#define CONFIG_CMD_IRQ +#define CONFIG_CMD_PING +#define CONFIG_CMD_SETEXPR

Errata?

Eh - removed :)

...

+#ifdef CONFIG_CMD_KGDB +#define CONFIG_SYS_CBSIZE 1024 /* Console I/O Buffer Size */ +#else +#define CONFIG_SYS_CBSIZE 256 /* Console I/O Buffer Size */ +#endif

KGDB?

*shrug* I just copied this from somewhere.

...

+#ifdef CONFIG_CMD_KGDB +#define CONFIG_KGDB_BAUDRATE 230400 /* speed to run kgdb serial port */ +#endif

This is copy-and-paste cruft even on the non-QEMU targets.

Ok, I'll remove it.

Alex

On 24.01.2014, at 01:49, Scott Wood scottwood@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@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@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

On 24.01.2014, at 02:39, Scott Wood scottwood@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@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@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@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.

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