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.
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?
> 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.
> 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?
> +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?
> +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?
> + /*
> + * 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.
> + }
> +
> + /* 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?
> +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)?
> +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?
> +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.
> +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.
> +
> + /* 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?
> +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()?
> 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, w...@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?
> + /*
> + * 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.
> +
> +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 <ag...@suse.de>
> Active powerpc mpc85xx - gdsys p1022
> controlcenterd_36BIT_SDCARD controlcenterd:36BIT,SDCARD
>
> Dirk Eibach <eib...@gdsys.de>
> Active powerpc mpc85xx - gdsys p1022
> controlcenterd_36BIT_SDCARD_DEVELOP
> controlcenterd:36BIT,SDCARD,DEVELOP
> Dirk Eibach
> <eib...@gdsys.de>
> Active powerpc mpc85xx - gdsys p1022
> controlcenterd_TRAILBLAZER
> controlcenterd:TRAILBLAZER,SPIFLASH
> Dirk Eibach
> <eib...@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?
> +#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?
> +#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)
> +#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.
> +#define CONFIG_QEMU_DT_ADDR ((2 * 1024 * 1024) - 4) /* below 2MB */
This is U-Boot-internal and not something hardcoded in QEMU, right?
> +/* 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?
> +/* 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.
> +
> +
> +/*
> + * IFC Definitions
> + */
There is no IFC.
> +#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?
> +#define CONFIG_SYS_MONITOR_BASE (CONFIG_RESET_VECTOR_ADDRESS -
> 0x100000 + 4)
> +#define CONFIG_SYS_SDRAM_SIZE 64
Why hardcoded to 64 MiB?
> +/*
> + * 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.
> +/*
> + * Environment
> + */
> +#define CONFIG_ENV_SIZE 0x2000
> +#define CONFIG_ENV_SECT_SIZE 0x10000 /* 64K (one sector) */
Even though ENV_IS_NOWHERE. :-)
> +#define CONFIG_LOADS_ECHO /* echo on for serial download */
> +#define CONFIG_SYS_LOADS_BAUD_CHANGE /* allow baudrate change */
Baud rate?
> +#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?
> +#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?
> +#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.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
