On 01/31/2014 03:16 AM, Alexander Graf wrote:

With the qemu-ppce500 machine type we can run the same board with either an e500v2 or an e500mc core plugged in.

This means that the IVOR setup can't be based on compile time decisions, so instead we have to do a runtime check which CPU generation we're running on.

Signed-off-by: Alexander Graf agraf@suse.de

arch/powerpc/cpu/mpc85xx/fixed_ivor.S | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/fixed_ivor.S b/arch/powerpc/cpu/mpc85xx/fixed_ivor.S index ebbb8c0..635a97e 100644 --- a/arch/powerpc/cpu/mpc85xx/fixed_ivor.S +++ b/arch/powerpc/cpu/mpc85xx/fixed_ivor.S @@ -36,17 +36,25 @@ SET_IVOR(14, 0x1e0) /* Instruction TLB Error */ SET_IVOR(15, 0x040) /* Debug */

-/* e500v1 & e500v2 only */ -#ifndef CONFIG_E500MC

• /* Check for CPU */
• mfpvr r3
• srwi r3, r3, 16
• /* Compare with e500mc PVR major number */
• li r4, 0
• ori r4, r4, 0x8023
• cmpw r3, r4
• /* e500v1 & e500v2 only */
• bge 1f SET_IVOR(32, 0x200) /* SPE Unavailable */ SET_IVOR(33, 0x220) /* Embedded FP Data */ SET_IVOR(34, 0x240) /* Embedded FP Round */

-#endif +1:

SET_IVOR(35, 0x260) /* Performance monitor */

-/* e500mc only */ -#ifdef CONFIG_E500MC

• /* e500mc only */
• blt 2f

This "blt" has a risk. Please put a comment warning developers who will modify the code to be sure the flag has not been updated since last "cmpw".

York

On 04.02.2014, at 02:59, Scott Wood scottwood@freescale.com wrote:

On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:

...

With the qemu-ppce500 machine type we can run the same board with either an e500v2 or an e500mc core plugged in.

This means that the IVOR setup can't be based on compile time decisions, so instead we have to do a runtime check which CPU generation we're running on.

Signed-off-by: Alexander Graf agraf@suse.de

arch/powerpc/cpu/mpc85xx/fixed_ivor.S | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/fixed_ivor.S b/arch/powerpc/cpu/mpc85xx/fixed_ivor.S index ebbb8c0..635a97e 100644 --- a/arch/powerpc/cpu/mpc85xx/fixed_ivor.S +++ b/arch/powerpc/cpu/mpc85xx/fixed_ivor.S @@ -36,17 +36,25 @@ SET_IVOR(14, 0x1e0) /* Instruction TLB Error */ SET_IVOR(15, 0x040) /* Debug */

-/* e500v1 & e500v2 only */ -#ifndef CONFIG_E500MC

• /* Check for CPU */
• mfpvr r3
• srwi r3, r3, 16
• /* Compare with e500mc PVR major number */
• li r4, 0
• ori r4, r4, 0x8023
• cmpw r3, r4
• /* e500v1 & e500v2 only */
• bge 1f SET_IVOR(32, 0x200) /* SPE Unavailable */ SET_IVOR(33, 0x220) /* Embedded FP Data */ SET_IVOR(34, 0x240) /* Embedded FP Round */

-#endif +1:

SET_IVOR(35, 0x260) /* Performance monitor */

-/* e500mc only */ -#ifdef CONFIG_E500MC

• /* e500mc only */
• blt 2f SET_IVOR(36, 0x280) /* Processor doorbell */ SET_IVOR(37, 0x2a0) /* Processor doorbell critical */ SET_IVOR(38, 0x2c0) /* Guest Processor doorbell */

@@ -54,6 +62,8 @@ SET_IVOR(40, 0x300) /* Hypervisor system call */ SET_IVOR(41, 0x320) /* Hypervisor Priviledge */

+#ifndef CONFIG_QEMU_E500

• /* QEMU guests runs in guest mode and can't access GIVORs */ SET_GIVOR(2, 0x060) /* Guest Data Storage */ SET_GIVOR(3, 0x080) /* Guest Instruction Storage */ SET_GIVOR(4, 0x0a0) /* Guest External Input */

@@ -61,3 +71,4 @@ SET_GIVOR(13, 0x1c0) /* Guest Data TLB Error */ SET_GIVOR(14, 0x1e0) /* Guest Instruction TLB Error */ #endif +2:

Again, let's please just remove this entire file.

I can remove it, but it's a pretty drastic change from the original behavior that was introduced 4 1/2 years ago:

http://lists.denx.de/pipermail/u-boot/2009-August/058670.html

I assume the idea was to give OSs one thing less to worry about (IVOR setting). If any OS in between 2009 and now relied on that fact, we'll break it by removing this code.

Alex

On 07.02.2014, at 01:51, Scott Wood scottwood@freescale.com wrote:

On Thu, 2014-02-06 at 12:40 +0100, Alexander Graf wrote:

...

On 04.02.2014, at 02:59, Scott Wood scottwood@freescale.com wrote:

...

On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:

...

With the qemu-ppce500 machine type we can run the same board with either an e500v2 or an e500mc core plugged in.

This means that the IVOR setup can't be based on compile time decisions, so instead we have to do a runtime check which CPU generation we're running on.

Signed-off-by: Alexander Graf agraf@suse.de

arch/powerpc/cpu/mpc85xx/fixed_ivor.S | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/fixed_ivor.S b/arch/powerpc/cpu/mpc85xx/fixed_ivor.S index ebbb8c0..635a97e 100644 --- a/arch/powerpc/cpu/mpc85xx/fixed_ivor.S +++ b/arch/powerpc/cpu/mpc85xx/fixed_ivor.S @@ -36,17 +36,25 @@ SET_IVOR(14, 0x1e0) /* Instruction TLB Error */ SET_IVOR(15, 0x040) /* Debug */

-/* e500v1 & e500v2 only */ -#ifndef CONFIG_E500MC

• /* Check for CPU */
• mfpvr r3
• srwi r3, r3, 16
• /* Compare with e500mc PVR major number */
• li r4, 0
• ori r4, r4, 0x8023
• cmpw r3, r4
• /* e500v1 & e500v2 only */
• bge 1f SET_IVOR(32, 0x200) /* SPE Unavailable */ SET_IVOR(33, 0x220) /* Embedded FP Data */ SET_IVOR(34, 0x240) /* Embedded FP Round */

-#endif +1:

SET_IVOR(35, 0x260) /* Performance monitor */

-/* e500mc only */ -#ifdef CONFIG_E500MC

• /* e500mc only */
• blt 2f SET_IVOR(36, 0x280) /* Processor doorbell */ SET_IVOR(37, 0x2a0) /* Processor doorbell critical */ SET_IVOR(38, 0x2c0) /* Guest Processor doorbell */

@@ -54,6 +62,8 @@ SET_IVOR(40, 0x300) /* Hypervisor system call */ SET_IVOR(41, 0x320) /* Hypervisor Priviledge */

+#ifndef CONFIG_QEMU_E500

• /* QEMU guests runs in guest mode and can't access GIVORs */ SET_GIVOR(2, 0x060) /* Guest Data Storage */ SET_GIVOR(3, 0x080) /* Guest Instruction Storage */ SET_GIVOR(4, 0x0a0) /* Guest External Input */

@@ -61,3 +71,4 @@ SET_GIVOR(13, 0x1c0) /* Guest Data TLB Error */ SET_GIVOR(14, 0x1e0) /* Guest Instruction TLB Error */ #endif +2:

Again, let's please just remove this entire file.

I can remove it, but it's a pretty drastic change from the original behavior that was introduced 4 1/2 years ago:

http://lists.denx.de/pipermail/u-boot/2009-August/058670.html

I assume the idea was to give OSs one thing less to worry about (IVOR setting). If any OS in between 2009 and now relied on that fact, we'll break it by removing this code.

Any such OS would already be broken on pre-2009 U-Boot. Linux doesn't rely on it. Neither the code nor the commit message gives a sufficient rationale, and Kumar didn't answer when asked. It's incomplete (doesn't include e6500 IVORs). It doesn't work on e6500 secondary threads. The

I thought e6500 has hard coded IVORs which is the whole point of this exercise? Or am I wrong there?

Alex

reset value of these registers is documented as zero, so it's not like we're just putting things back the way they were. It only happens (except on secondary cores) when the bootm/bootz commands are used, not when using the ELF loader or the go command, nor when using bootm with IH_TYPE_STANDALONE.

Does anyone know of an OS that actually depends on this?

-Scott

On 01/31/2014 03:16 AM, 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

---

v1 -> v2:

• Write device tree offset directly into global variable
• use r4 rather than r2 for that
• access fdt directly from in-memory copy
• remove unneeded header includes
• clean up pci enumeration
• coding style fixes
• populate and only use fdt_addr_r
• remove unused exported functions
• remove unused TLB0 entries
• make TLB1 I/O maps non-executable
• remove unused defines in board header
• make -kernel boot variables more clear
• remove TLB0 invalidation
• use tlb1.14 for temporary as=1 map
• use CONFIG_SYS_MPC85XX_NO_RESETVEC
• store fdt pointer in gd through cpu_init_early_f()
• replace fixup_tlb1() with dynamic TLB creation hook

---

<snip>

diff --git a/arch/powerpc/cpu/mpc85xx/tlb.c b/arch/powerpc/cpu/mpc85xx/tlb.c index 8748ecd..c899d9e 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_USE_DYNAMIC_TLBS

• init_tlbs_dynamic();

+#endif

You are adding a new CONFIG to common files. Please document it.

return ; }

<snip>

diff --git a/include/configs/qemu-ppce500.h b/include/configs/qemu-ppce500.h new file mode 100644 index 0000000..1ebaf51 --- /dev/null +++ b/include/configs/qemu-ppce500.h @@ -0,0 +1,209 @@ +/*

• • 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 0xf01000 /* 15 MB */

+#define CONFIG_SYS_MPC85XX_NO_RESETVEC

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

+#define CONFIG_ADDR_MAP +#define CONFIG_SYS_NUM_ADDR_MAP 16 /* number of TLB1 entries */

+#define CONFIG_USE_DYNAMIC_TLBS

+#define CONFIG_SYS_MEMTEST_START 0x00200000 /* memtest works on */ +#define CONFIG_SYS_MEMTEST_END 0x00400000 +#define CONFIG_SYS_ALT_MEMTEST +#define CONFIG_PANIC_HANG /* do not reset board on panic */

+#define CONFIG_SYS_CCSRBAR 0xe0000000 +#define CONFIG_SYS_CCSRBAR_PHYS_LOW CONFIG_SYS_CCSRBAR

+/*

• • DDR Setup
• */

+#define CONFIG_VERY_BIG_RAM +#define CONFIG_SYS_DDR_SDRAM_BASE 0x00000000 +#define CONFIG_SYS_SDRAM_BASE CONFIG_SYS_DDR_SDRAM_BASE

+#define CONFIG_CHIP_SELECTS_PER_CTRL 0

I don't know if the qemu has PCI, DDR, etc. Setting the above line to 0 will actually disable DDR controllers. Is that what you want?

+/* Get RAM size from device tree */ +#define CONFIG_DDR_SPD

You enabled SPD but I don't see the I2C address. Did you miss something, or you don't really use SPD?

York

On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:

The definition of our ppce500 PV machine is that every address is dynamically determined through device tree bindings.

So don't hardcode where CCSR is in our physical memory layout but instead read it dynamically from the device tree we get passed on boot.

Signed-off-by: Alexander Graf agraf@suse.de

arch/powerpc/cpu/mpc85xx/start.S | 2 + board/freescale/qemu-ppce500/qemu-ppce500.c | 83 +++++++++++++++++++++++++++ include/configs/qemu-ppce500.h | 12 +++- 3 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S index 0e593d2..2672c20 100644 --- a/arch/powerpc/cpu/mpc85xx/start.S +++ b/arch/powerpc/cpu/mpc85xx/start.S @@ -519,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)

#if !defined(CONFIG_SYS_CCSRBAR_PHYS_HIGH) || !defined(CONFIG_SYS_CCSRBAR_PHYS_LOW) @@ -703,6 +704,7 @@ delete_temp_tlbs: delete_tlb0_entry 1, CONFIG_SYS_CCSRBAR + 0x1000, MAS2_I|MAS2_G, r3

#endif /* #if (CONFIG_SYS_CCSRBAR_DEFAULT != CONFIG_SYS_CCSRBAR_PHYS) */ +#endif /* #if !defined(CONFIG_DYNAMIC_CCSRBAR) */

#if defined(CONFIG_SYS_FSL_QORIQ_CHASSIS2) && defined(CONFIG_E6500) create_ccsr_l2_tlb: diff --git a/board/freescale/qemu-ppce500/qemu-ppce500.c b/board/freescale/qemu-ppce500/qemu-ppce500.c index b33b6b1..6491ae9 100644 --- a/board/freescale/qemu-ppce500/qemu-ppce500.c +++ b/board/freescale/qemu-ppce500/qemu-ppce500.c @@ -26,6 +26,89 @@ static const void *get_fdt(void) return gd->fdt_blob; }

+uint64_t get_phys_ccsrbar_addr(void) +{

• const void *fdt = get_fdt();
• int root_node = fdt_path_offset(fdt, "/");
• int soc_node = fdt_path_offset(fdt, "/soc");
• const uint32_t *prop;
• const uint64_t *prop64;
• int len;
• int root_address_cells = 1;
• int address_cells = 1;
• uint64_t r = 0;
• /* Read CCSRBAR address length and size from device tree */
• prop = fdt_getprop(fdt, root_node, "#address-cells", &len);
• if (prop && (len >= 4))

• root_address_cells = prop[0];
  

• prop = fdt_getprop(fdt, soc_node, "#address-cells", &len);
• if (prop && (len >= 4))

• address_cells = prop[0];
  

• /* Read CCSRBAR address from device tree */
• prop = fdt_getprop(fdt, soc_node, "ranges", &len);
• if (prop && (len >= ((address_cells + root_address_cells) * 4))) {

• /* The physical address starts after the child's offset */
  

• prop += address_cells;
  

• prop64 = (const uint64_t *)prop;
  

• if (root_address_cells == 1)
  

• 	r = prop[0];
  

• else if (root_address_cells == 2)
  

• 	r = prop64[0];
  

• }
• if (!r)

• panic("Couldn't find CCSR in FDT");
  

• return r;

+}

+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);
• write_tlb(mas0, mas1, mas2, mas3, mas7);
• r = get_phys_ccsrbar_addr();
• disable_tlb(10);
• return r;

+}

+int board_early_init_f(void) +{

• u32 mas0, mas1, mas2, mas3, mas7;
• uint64_t phys_ccsr_address = get_phys_ccsrbar_addr();
• /* Extend the CCSR map from cpu_init_early_f() */
• mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(13);
• mas1 = MAS1_VALID | MAS1_TID(0) | MAS1_TSIZE(BOOKE_PAGESZ_64M);
• mas2 = FSL_BOOKE_MAS2(CONFIG_SYS_CCSRBAR, MAS2_I|MAS2_G);
• mas3 = FSL_BOOKE_MAS3(phys_ccsr_address, 0, MAS3_SW|MAS3_SR);
• mas7 = FSL_BOOKE_MAS7(phys_ccsr_address);
• write_tlb(mas0, mas1, mas2, mas3, mas7);
• return 0;

+}

int checkboard(void) { return 0;

You're adding new code to map CCSR and FDT, but not removing the code to do the same thing you added in patch 3/6.

Why 64M?

diff --git a/include/configs/qemu-ppce500.h b/include/configs/qemu-ppce500.h index 1ebaf51..7ef7235 100644 --- a/include/configs/qemu-ppce500.h +++ b/include/configs/qemu-ppce500.h @@ -44,8 +44,18 @@ #define CONFIG_SYS_ALT_MEMTEST #define CONFIG_PANIC_HANG /* do not reset board on panic */

+/* Needed to fill the ccsrbar pointer */ +#define CONFIG_BOARD_EARLY_INIT_F

+/* Virtual address to CCSRBAR */ #define CONFIG_SYS_CCSRBAR 0xe0000000 -#define CONFIG_SYS_CCSRBAR_PHYS_LOW CONFIG_SYS_CCSRBAR +/* Physical address should be a function call */ +#ifndef __ASSEMBLY__ +extern unsigned long long get_phys_ccsrbar_addr_early(void); +#endif +#define CONFIG_SYS_CCSRBAR_PHYS_LOW ((uint32_t)get_phys_ccsrbar_addr_early()) +#define CONFIG_SYS_CCSRBAR_PHYS_HIGH (get_phys_ccsrbar_addr_early() >> 32) +#define CONFIG_DYNAMIC_CCSRBAR

What cares about CONFIG_SYS_CCSRBAR_PHYS* other than the code you skipped with an ifndef in start.S, and related header ifdeffery (which could be skipped by the existing CONFIG_SYS_CCSR_DO_NOT_RELOCATE instead)?

-Scott

On 04.02.2014, at 03:47, Scott Wood scottwood@freescale.com wrote:

On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:

...

The definition of our ppce500 PV machine is that every address is dynamically determined through device tree bindings.

So don't hardcode where PCI devices are in our physical memory layout but instead read them dynamically from the device tree we get passed on boot.

Would it be difficult to make the QEMU emulation properly implement access windows?

What are access windows? You mean BAR region offsets? Not too hard I suppose, but it adds complexity which we were trying to avoid, no?

...

Signed-off-by: Alexander Graf agraf@suse.de

board/freescale/qemu-ppce500/qemu-ppce500.c | 193 ++++++++++++++++++++++++--- board/freescale/qemu-ppce500/tlb.c | 13 -- include/configs/qemu-ppce500.h | 12 -- 3 files changed, 175 insertions(+), 43 deletions(-)

diff --git a/board/freescale/qemu-ppce500/qemu-ppce500.c b/board/freescale/qemu-ppce500/qemu-ppce500.c index 6491ae9..5d4dd64 100644 --- a/board/freescale/qemu-ppce500/qemu-ppce500.c +++ b/board/freescale/qemu-ppce500/qemu-ppce500.c @@ -19,7 +19,51 @@ #include <malloc.h>

DECLARE_GLOBAL_DATA_PTR; -static struct pci_controller pci1_hose;

+static uint64_t myfdt_readcells(const void *fdt, int node, const char *property,

• 		int num, int off, uint64_t defval)
  

"my" fdt?

Yeah, didn't want to clash with the fdt namespace.

...

+{

• int len;
• const uint32_t *prop;
• prop = fdt_getprop(fdt, node, property, &len);
• if (!prop)

• return defval;
  

• if (len < ((off + num) * sizeof(uint32_t)))

• panic("Invalid fdt");
  

• prop += off;
• switch (num) {
• case 1:

• return *prop;
  

• case 2:

• return *(const uint64_t *)prop;
  

• }

What about this function is specific to qemu-ppce500?

Nothing. But the less common code I touch the less I can break. There seems to be an fdt helper framework that's only targeted at a few ARM devices - not sure what to make of that.

• panic("Invalid cell size");

+}

s/cell size/cell count/

...

+static uint32_t myfdt_one_cell(const void *fdt, int node, const char *property,

• 	       uint32_t defval)
  

+{

• return myfdt_readcells(fdt, node, property, 1, 0, defval);

+}

This looks a lot like fdt_getprop_u32_default(), except for "int node" versus path.

Well, I use it inside of a loop where I don't have the path :).

...

+static void map_tlb1_io(ulong virt_addr, uint64_t phys_addr, uint64_t size) +{

• unsigned int max_cam, tsize_mask;
• int i;
• if ((mfspr(SPRN_MMUCFG) & MMUCFG_MAVN) == MMUCFG_MAVN_V1) {

• /* Convert (4^max) kB to (2^max) bytes */
  

• max_cam = ((mfspr(SPRN_TLB1CFG) >> 16) & 0xf) * 2 + 10;
  

• tsize_mask = ~1U;
  

• } else {

• /* Convert (2^max) kB to (2^max) bytes */
  

• max_cam = __ilog2(mfspr(SPRN_TLB1PS)) + 10;
  

• tsize_mask = ~0U;
  

• }
• for (i = 0; size && i < 8; i++) {

• int tlb_index = find_free_tlbcam();
  

• u32 camsize = __ilog2_u64(size) & tsize_mask;
  

• u32 align = __ilog2(virt_addr) & tsize_mask;
  

• unsigned int tlb_size;
  

• if (tlb_index == -1)
  

• 	break;
  

• if (align == -2) align = max_cam;
  

-2? Besides align being unsigned, if this is meant to handle the case where virt_addr is zero, that's undefined for __ilog2() (don't rely on it being implemented with cntlzw), and you're not handling the MMUv2 case.

I merely copied this from tlb.c's setup_ddr_tlbs_phys() and adjusted it slightly to let me choose the target virt address.

Would you prefer if I generalize setup_ddr_tlbs_phys() inside tlb.c and export that function there?

Plus, whitespace.

...

• if (camsize > align)
  

• 	camsize = align;
  

• if (camsize > max_cam)
  

• 	camsize = max_cam;
  

What about min_cam?

...

• }

+}

Whitespace.

...

void pci_init_board(void) {

• struct fsl_pci_info pci_info;

• struct pci_controller *pci_hoses; const void *fdt = get_fdt(); int pci_node;

• int pci_num = 0;

• int pci_count;

• const char *compat = "fsl,mpc8540-pci";

• ulong map_addr;

  puts("\n");

• pci_node = fdt_path_offset(fdt, "/pci");
• if (pci_node < 0) {

• /* Start MMIO and PIO range maps above RAM */
• map_addr = CONFIG_MAX_MEM_MAPPED;
• /* Count and allocate PCI buses */
• pci_count = myfdt_count_compatibles(fdt, compat);
• if (pci_count) {

• pci_hoses = malloc(sizeof(struct pci_controller) * pci_count);
  

• } else { printf("PCI: disabled\n\n"); return; }

• SET_STD_PCI_INFO(pci_info, 1);
• fsl_setup_hose(&pci1_hose, pci_info.regs);
• printf("PCI: 32 bit, 66 MHz, async, host, base address %lx\n",

• pci_info.regs);
  

• fsl_pci_init_port(&pci_info, &pci1_hose, 0);

• /* 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 = { };
  

• uint64_t phys_addr;
  

• int pnode = fdt_parent_offset(fdt, pci_node);
  

• int paddress_cells;
  

• int address_cells;
  

• int size_cells;
  

• int off = 0;
  

• paddress_cells = myfdt_one_cell(fdt, pnode, "#address-cells", 1);
  

• address_cells = myfdt_one_cell(fdt, pci_node, "#address-cells", 1);
  

• size_cells = myfdt_one_cell(fdt, pci_node, "#size-cells", 1);
  

• pci_info.regs = myfdt_readcells(fdt, pci_node, "reg",
  

• 				paddress_cells, 0, 0);
  

• /* MMIO range */
  

• off += address_cells;
  

• phys_addr = myfdt_readcells(fdt, pci_node, "ranges",
  

• 			    paddress_cells, off, 0);
  

• off += paddress_cells;
  

• pci_info.mem_size = myfdt_readcells(fdt, pci_node, "ranges",
  

• 	size_cells, off, 0);
  

• off += size_cells;
  

• /* Align virtual region */
  

• map_addr += pci_info.mem_size - 1;
  

• map_addr &= ~(pci_info.mem_size - 1);
  

• /* Map virtual memory for MMIO range */
  

• map_tlb1_io(map_addr, phys_addr, pci_info.mem_size);
  

• pci_info.mem_bus = phys_addr;
  

• pci_info.mem_phys = phys_addr;
  

• map_addr += pci_info.mem_size;
  

• /* PIO range */
  

• off += address_cells;
  

• pci_info.io_phys = myfdt_readcells(fdt, pci_node, "ranges",
  

• 	paddress_cells, off, 0);
  

• off += paddress_cells;
  

• pci_info.io_size = myfdt_readcells(fdt, pci_node, "ranges",
  

• 	size_cells, off, 0);
  

• /* Align virtual region */
  

• map_addr += pci_info.io_size - 1;
  

• map_addr &= ~(pci_info.io_size - 1);
  

• /* Map virtual memory for MMIO range */
  

• map_tlb1_io(map_addr, pci_info.io_phys, pci_info.io_size);
  

• pci_info.io_bus = map_addr;
  

• map_addr += pci_info.io_size;
  

• /* Instantiate */
  

• pci_info.pci_num = pci_num + 1;
  

• fsl_setup_hose(&pci_hoses[pci_num], pci_info.regs);
  

• printf("PCI: 32 bit, 66 MHz, async, host, base address %lx\n",
  

• 	pci_info.regs);
  

• fsl_pci_init_port(&pci_info, &pci_hoses[pci_num], pci_num);
  

• /* Jump to next PCI node */
  

• pci_node = fdt_node_offset_by_compatible(fdt, pci_node, compat);
  

• pci_num++;
  

• }

  puts("\n");

}

How about using fdt_translate_address() or other existing functionality?

Mind to show exactly how?

Alex

On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:

The only thing we know in our PV machine through device tree is the clock speed of the CPUs. Take that as CPU speed, system speed and ddr speed so that we have some meaningful values there at all.

The CPU speed is important because our timing loops get determined based on it.

Signed-off-by: Alexander Graf agraf@suse.de

arch/powerpc/cpu/mpc85xx/Makefile | 2 ++ board/freescale/qemu-ppce500/qemu-ppce500.c | 43 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)

diff --git a/arch/powerpc/cpu/mpc85xx/Makefile b/arch/powerpc/cpu/mpc85xx/Makefile index ef7637a..4094785 100644 --- a/arch/powerpc/cpu/mpc85xx/Makefile +++ b/arch/powerpc/cpu/mpc85xx/Makefile @@ -102,7 +102,9 @@ obj-y += cpu.o obj-y += cpu_init.o obj-y += cpu_init_early.o obj-y += interrupts.o +ifneq ($(CONFIG_QEMU_E500),y) obj-y += speed.o +endif obj-y += tlb.o obj-y += traps.o

diff --git a/board/freescale/qemu-ppce500/qemu-ppce500.c b/board/freescale/qemu-ppce500/qemu-ppce500.c index 5d4dd64..9e9d688 100644 --- a/board/freescale/qemu-ppce500/qemu-ppce500.c +++ b/board/freescale/qemu-ppce500/qemu-ppce500.c @@ -407,3 +407,46 @@ void init_laws(void) { /* We don't emulate LAWs yet */ }

+static uint32_t get_cpu_freq(void) +{

• const void *fdt = get_fdt();
• int cpus_node = fdt_path_offset(fdt, "/cpus");
• int cpu_node = fdt_first_subnode(fdt, cpus_node);
• return myfdt_one_cell(fdt, cpu_node, "clock-frequency", 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;

+}

+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;
• return 0;

+}

This probably decreases the accuracy of the timebase frequency, since you'll be basing it on the CPU frequency rather than the bus frequency.

If you're doing this, why not override get_tbclk() as well?

-Scott

On Thu, 2014-02-06 at 12:11 +0100, Alexander Graf wrote:

On 04.02.2014, at 03:52, Scott Wood scottwood@freescale.com wrote:

...

On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:

...

The only thing we know in our PV machine through device tree is the clock speed of the CPUs. Take that as CPU speed, system speed and ddr speed so that we have some meaningful values there at all.

The CPU speed is important because our timing loops get determined based on it.

Signed-off-by: Alexander Graf agraf@suse.de

arch/powerpc/cpu/mpc85xx/Makefile | 2 ++ board/freescale/qemu-ppce500/qemu-ppce500.c | 43 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)

diff --git a/arch/powerpc/cpu/mpc85xx/Makefile b/arch/powerpc/cpu/mpc85xx/Makefile index ef7637a..4094785 100644 --- a/arch/powerpc/cpu/mpc85xx/Makefile +++ b/arch/powerpc/cpu/mpc85xx/Makefile @@ -102,7 +102,9 @@ obj-y += cpu.o obj-y += cpu_init.o obj-y += cpu_init_early.o obj-y += interrupts.o +ifneq ($(CONFIG_QEMU_E500),y) obj-y += speed.o +endif obj-y += tlb.o obj-y += traps.o

diff --git a/board/freescale/qemu-ppce500/qemu-ppce500.c b/board/freescale/qemu-ppce500/qemu-ppce500.c index 5d4dd64..9e9d688 100644 --- a/board/freescale/qemu-ppce500/qemu-ppce500.c +++ b/board/freescale/qemu-ppce500/qemu-ppce500.c @@ -407,3 +407,46 @@ void init_laws(void) { /* We don't emulate LAWs yet */ }

+static uint32_t get_cpu_freq(void) +{

• const void *fdt = get_fdt();
• int cpus_node = fdt_path_offset(fdt, "/cpus");
• int cpu_node = fdt_first_subnode(fdt, cpus_node);
• return myfdt_one_cell(fdt, cpu_node, "clock-frequency", 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;

+}

+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;
• return 0;

+}

This probably decreases the accuracy of the timebase frequency, since you'll be basing it on the CPU frequency rather than the bus frequency.

If you're doing this, why not override get_tbclk() as well?

That one only uses variables I already control, no?

unsigned long get_tbclk (void) { unsigned long tbclk_div = CONFIG_SYS_FSL_TBCLK_DIV;

    return (gd->bus_clk + (tbclk_div >> 1)) / tbclk_div;

}

What did you set CONFIG_SYS_FSL_TBCLK_DIV to?

-Scott

On Fri, 2014-02-07 at 12:50 +0100, Alexander Graf wrote:

On 07.02.2014, at 02:33, Scott Wood scottwood@freescale.com wrote:

...

On Thu, 2014-02-06 at 12:11 +0100, Alexander Graf wrote:

...

On 04.02.2014, at 03:52, Scott Wood scottwood@freescale.com wrote:

...

On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:

...

The only thing we know in our PV machine through device tree is the clock speed of the CPUs. Take that as CPU speed, system speed and ddr speed so that we have some meaningful values there at all.

The CPU speed is important because our timing loops get determined based on it.

Signed-off-by: Alexander Graf agraf@suse.de

arch/powerpc/cpu/mpc85xx/Makefile | 2 ++ board/freescale/qemu-ppce500/qemu-ppce500.c | 43 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)

diff --git a/arch/powerpc/cpu/mpc85xx/Makefile b/arch/powerpc/cpu/mpc85xx/Makefile index ef7637a..4094785 100644 --- a/arch/powerpc/cpu/mpc85xx/Makefile +++ b/arch/powerpc/cpu/mpc85xx/Makefile @@ -102,7 +102,9 @@ obj-y += cpu.o obj-y += cpu_init.o obj-y += cpu_init_early.o obj-y += interrupts.o +ifneq ($(CONFIG_QEMU_E500),y) obj-y += speed.o +endif obj-y += tlb.o obj-y += traps.o

diff --git a/board/freescale/qemu-ppce500/qemu-ppce500.c b/board/freescale/qemu-ppce500/qemu-ppce500.c index 5d4dd64..9e9d688 100644 --- a/board/freescale/qemu-ppce500/qemu-ppce500.c +++ b/board/freescale/qemu-ppce500/qemu-ppce500.c @@ -407,3 +407,46 @@ void init_laws(void) { /* We don't emulate LAWs yet */ }

+static uint32_t get_cpu_freq(void) +{

• const void *fdt = get_fdt();
• int cpus_node = fdt_path_offset(fdt, "/cpus");
• int cpu_node = fdt_first_subnode(fdt, cpus_node);
• return myfdt_one_cell(fdt, cpu_node, "clock-frequency", 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;

+}

+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;
• return 0;

+}

This probably decreases the accuracy of the timebase frequency, since you'll be basing it on the CPU frequency rather than the bus frequency.

If you're doing this, why not override get_tbclk() as well?

That one only uses variables I already control, no?

unsigned long get_tbclk (void) { unsigned long tbclk_div = CONFIG_SYS_FSL_TBCLK_DIV;

   return (gd->bus_clk + (tbclk_div >> 1)) / tbclk_div;

}

What did you set CONFIG_SYS_FSL_TBCLK_DIV to?

Nothing which is why it's defaulting to 8.

Hence why I suggested overriding get_tbclk() rather than trying to fake up a divider (which could be difficult if the CPU clock is not a multiple of the bus clock).

-Scott