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

[Scott Wood]

On Tue, 2014-02-11 at 01:10 +0100, Alexander Graf wrote:
> diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S
> index bb0025c..8982c78 100644
> --- a/arch/powerpc/cpu/mpc85xx/start.S
> +++ b/arch/powerpc/cpu/mpc85xx/start.S
> @@ -80,6 +80,11 @@ _start_e500:
>  	li	r1,MSR_DE
>  	mtmsr 	r1
>  
> +#ifdef CONFIG_QEMU_E500
> +	/* Save our ePAPR device tree pointer before we clobber it */
> +	mr	r24, r3
> +#endif

FWIW, it should be harmless to do this unconditionally (though in that
case I'd insert "(if we have one)" in the comment.

>  #ifdef CONFIG_SYS_FSL_ERRATUM_A004510
>  	mfspr	r3,SPRN_SVR
>  	rlwinm	r3,r3,0,0xff
> @@ -514,6 +519,7 @@ nexti:	mflr	r1		/* R1 = our PC */
>   * As a general rule, TLB0 is used for short-term TLBs, and TLB1 is used for
>   * long-term TLBs, so we use TLB0 here.
>   */
> +#if !defined(CONFIG_DYNAMIC_CCSRBAR)
>  #if (CONFIG_SYS_CCSRBAR_DEFAULT != CONFIG_SYS_CCSRBAR_PHYS)

This shouldn't be necessary, if you have
CONFIG_SYS_CCSR_DO_NOT_RELOCATE.

> diff --git a/arch/powerpc/cpu/mpc85xx/tlb.c b/arch/powerpc/cpu/mpc85xx/tlb.c
> index 2011fb8..0e0b483 100644
> --- a/arch/powerpc/cpu/mpc85xx/tlb.c
> +++ b/arch/powerpc/cpu/mpc85xx/tlb.c
> @@ -36,6 +36,10 @@ void init_tlbs(void)
>  			  tlb_table[i].mas7);
>  	}
>  
> +#ifdef CONFIG_SYS_USE_DYNAMIC_TLBS
> +	init_tlbs_dynamic();
> +#endif

You could avoid the ifdef by moving a stub implementation into the
header -- or possibly better, avoid the config symbol entirely by using
a weak symbol.

Better still, make init_tlbs() weak.  Then you don't need a config
symbol, a new function name, or a dummy tlb_table[] -- you just redefine
init_tlbs() in board code.

> diff --git a/board/freescale/qemu-ppce500/qemu-ppce500.c b/board/freescale/qemu-ppce500/qemu-ppce500.c
> new file mode 100644
> index 0000000..fc546b9
> --- /dev/null
> +++ b/board/freescale/qemu-ppce500/qemu-ppce500.c
> @@ -0,0 +1,334 @@
> +/*
> + * 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/fsl_pci.h>
> +#include <asm/io.h>
> +#include <libfdt.h>
> +#include <fdt_support.h>
> +#include <netdev.h>
> +#include <fdtdec.h>
> +#include <errno.h>
> +#include <malloc.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static void *get_fdt(void)
> +{
> +	return (void *)gd->fdt_blob;
> +}

Does this return virtual or physical?

> +
> +uint64_t get_phys_ccsrbar_addr_early(void)
> +{
> +	u32 mas0, mas1, mas2, mas3, mas7;
> +	ulong fdt = (ulong)get_fdt();
> +	uint64_t r;
> +
> +	/*
> +	 * To be able to read the FDT we need to create a temporary TLB
> +	 * map for it.
> +	 */
> +
> +	mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(10);
> +	mas1 = MAS1_VALID | MAS1_TID(0) | MAS1_TS | MAS1_TSIZE(BOOKE_PAGESZ_1M);
> +	mas2 = FSL_BOOKE_MAS2(fdt, 0);
> +	mas3 = FSL_BOOKE_MAS3(fdt, 0, MAS3_SW|MAS3_SR);
> +	mas7 = FSL_BOOKE_MAS7(fdt);

Don't use the physical address as a virtual address.  Use a known-good
virtual address.  Using unknown physical addresses as virtual addresses
is generally a bad idea (it's different for stuff whose physical address
is hardcoded in U-Boot), but it's particularly bad here because you'll
create an overlapping TLB entry if the fdt happens to live within your
initial memory mapping.

OK, I see you use CONFIG_SYS_TMPVIRT for this elsewhere, but I guess
forgot to use it here (why is fdt mapping code duplicated?).

> +void pci_init_board(void)
> +{
> +	struct pci_controller *pci_hoses;
> +	void *fdt = get_fdt();
> +	int pci_node;
> +	int pci_num = 0;
> +	int pci_count = 0;
> +	const char *compat = "fsl,mpc8540-pci";
> +	ulong map_addr;

May want to look for arbitrary PCI host controllers (device_type would
be simplest), in case the QEMU machine ever gets an upgrade to PCIe.

> +
> +	puts("\n");
> +
> +	/* Start MMIO and PIO range maps above RAM */
> +	map_addr = CONFIG_MAX_MEM_MAPPED;

It'd be better to hardcode virtual addresses for this (as other boards
do), and limit the size you map to the smaller of the hardcoded size or
the device tree size.

> +	/* Count and allocate PCI buses */
> +	pci_node = fdt_node_offset_by_compatible(fdt, -1, compat);
> +	while (pci_node != -FDT_ERR_NOTFOUND) {
> +		pci_node = fdt_node_offset_by_compatible(fdt, pci_node, compat);
> +		pci_count++;
> +	}
> +
> +	if (pci_count) {
> +		pci_hoses = malloc(sizeof(struct pci_controller) * pci_count);
> +	} else {
> +		printf("PCI: disabled\n\n");
> +		return;
> +	}
> +
> +	/* Spawn PCI buses based on device tree */
> +	pci_node = fdt_node_offset_by_compatible(fdt, -1, compat);
> +	while (pci_node != -FDT_ERR_NOTFOUND) {
> +		struct fsl_pci_info pci_info = { };
> +		const fdt32_t *reg;
> +		int r;
> +
> +		reg = fdt_getprop(fdt, pci_node, "reg", NULL);
> +		pci_info.regs = fdt_translate_address((void *)fdt, pci_node, reg);

Unnecessary cast.

> +void init_tlbs_dynamic(void)
> +{
> +	unsigned long fdt_phys = (unsigned long)get_fdt();
> +	unsigned long fdt_phys_tlb = fdt_phys & ~0xffffful;
> +	unsigned long fdt_virt_tlb = CONFIG_SYS_TMPVIRT;
> +	unsigned long fdt_virt = fdt_virt_tlb + (fdt_phys & 0xffffful);
> +	u32 mas0, mas1, mas2, mas3, mas7;
> +	phys_size_t ram_size;
> +
> +	/*
> +	 * Create a temporary AS=1 map for the fdt
> +	 *
> +	 * We use ESEL=0 here to overwrite the previous AS=0 map for ourselves
> +	 * which was only 4k big. This way we don't have to clear any other maps.
> +	 */

I don't think it's generally safe to assume that this entry is ESEL 0 --
though I'm wondering if the current TLB code is assuming that (or at
least, assuming that whatever entry is used gets overwritten by the TLB
table).

> +	mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(0);
> +	mas1 = MAS1_VALID | MAS1_TID(0) | MAS1_TS | MAS1_TSIZE(BOOKE_PAGESZ_1M);
> +	mas2 = FSL_BOOKE_MAS2(fdt_virt_tlb, 0);
> +	mas3 = FSL_BOOKE_MAS3(fdt_phys_tlb, 0, MAS3_SW|MAS3_SR);
> +	mas7 = FSL_BOOKE_MAS7(fdt_phys_tlb);

What if the fdt straddles a 1M boundary?

> +	write_tlb(mas0, mas1, mas2, mas3, mas7);
> +	gd->fdt_blob = (void *)fdt_virt;
> +
> +	/* Fetch RAM size from the fdt */
> +	ram_size = fixed_sdram();

Why not just call get_linear_ram_size() directly?

> +	/* And remove our fdt map again */
> +	gd->fdt_blob = (void *)fdt_phys;
> +	disable_tlb(0);
> +
> +	/* Create a dynamic AS=0 CCSRBAR mapping */
> +	mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(13);
> +	mas1 = MAS1_VALID | MAS1_TID(0) | MAS1_TSIZE(BOOKE_PAGESZ_1M);
> +	mas2 = FSL_BOOKE_MAS2(CONFIG_SYS_CCSRBAR, MAS2_I|MAS2_G);
> +	mas3 = FSL_BOOKE_MAS3(CONFIG_SYS_CCSRBAR_PHYS, 0, MAS3_SW|MAS3_SR);
> +	mas7 = FSL_BOOKE_MAS7(CONFIG_SYS_CCSRBAR_PHYS);
> +
> +	write_tlb(mas0, mas1, mas2, mas3, mas7);

Why is this not done via tlb_map_range (after calling
init_used_tlb_cams, of course)?

> +
> +	/* Create a RAM map that spans all accessible RAM */
> +	init_used_tlb_cams();
> +	setup_ddr_tlbs(ram_size >> 20);
> +}
> +
> +void init_laws(void)
> +{
> +	/* We don't emulate LAWs yet */
> +}
> +
> +static uint32_t get_cpu_freq(void)
> +{
> +	void *fdt = get_fdt();
> +	int cpus_node = fdt_path_offset(fdt, "/cpus");
> +	int cpu_node = fdt_first_subnode(fdt, cpus_node);
> +	const char *prop = "clock-frequency";
> +	return fdt_getprop_u32_default_node(fdt, cpu_node, 0, prop, 0);
> +}
> +
> +void get_sys_info(sys_info_t *sys_info)
> +{
> +	int freq = get_cpu_freq();
> +
> +	memset(sys_info, 0, sizeof(sys_info_t));
> +	sys_info->freq_systembus = freq;
> +	sys_info->freq_ddrbus = freq;
> +	sys_info->freq_processor[0] = freq;
> +}
> +

Again, if you're doing this you really should override get_tbclk()
instead of letting it use the much faster cpufreq/8.

> +int get_clocks (void)
> +{
> +	sys_info_t sys_info;
> +
> +	get_sys_info(&sys_info);
> +
> +	gd->cpu_clk = sys_info.freq_processor[0];
> +	gd->bus_clk = sys_info.freq_systembus;
> +	gd->mem_clk = sys_info.freq_ddrbus;
> +	gd->arch.lbc_clk = sys_info.freq_ddrbus;

I wonder if with higher frequencies we'll trigger overflows in some
drivers. :-)

> +/* Physical address should be a function call */
> +#ifndef __ASSEMBLY__
> +extern unsigned long long get_phys_ccsrbar_addr_early(void);
> +#endif

Where does this get called?

-Scott