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

2014-04-28 Thread York Sun
On 02/20/2014 04:52 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 generic ppce500 machine and tries to
 rely solely on device tree for device enumeration.
 
 Signed-off-by: Alexander Graf ag...@suse.de
 
 ---
 
 v3 - v4:
 
   - use new tlb map syntax
   - check that tlb map mapped everything
   - always save/restore r3 for cpu_init_early_f
   - remove CONFIG_DYNAMIC_CCSRBAR
   - make init_tlbs weak and remove all init_tlbs_dynamic() cruft
   - always map fdt to fixed virtual address
   - fold together AS=1 fdt map functions
   - use tlb_map_range for CCSR map
   - search for PCI based on device type rather than compatible
   - remove unnecessary cast
   - call get_linear_ram_size directly
   - override get_tbclk with timebase-frequency fetching code
 ---

snip

 diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c 
 b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
 index 993b8b8..998781b 100644
 --- a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
 +++ b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
 @@ -79,7 +79,7 @@ void setup_ifc(void)
  #endif
  
  /* We run cpu_init_early_f in AS = 1 */
 -void cpu_init_early_f(void)
 +void cpu_init_early_f(void *fdt)
  {
   u32 mas0, mas1, mas2, mas3, mas7;
   int i;
 @@ -102,6 +102,12 @@ void cpu_init_early_f(void)
   for (i = 0; i  sizeof(gd_t); i++)
   ((char *)gd)[i] = 0;
  
 + /*
 +  * CONFIG_SYS_CCSRBAR_PHYS below may use gd-fdt_blob on ePAPR systems,
 +  * so we need to populate it before it accesses it.
 +  */
 + gd-fdt_blob = fdt;
 +

Alex,

I am thinking to add back #ifdef CONFIG_QEMU_E500 you had for your v3 and
earlier version for the above change. Do you have any concerns?

The reason I am proposing this is to use generic board architecture.
gd-fdt_blob is used by some other functions to detect if device tree exists.
Your change takes r3 from entry and treat it as device tree. It is not the case
for a normal metal SoC. I need to clear this variable.

York

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2014-04-28 Thread Alexander Graf


 Am 28.04.2014 um 20:40 schrieb York Sun york...@freescale.com:
 
 On 02/20/2014 04:52 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 generic ppce500 machine and tries to
 rely solely on device tree for device enumeration.
 
 Signed-off-by: Alexander Graf ag...@suse.de
 
 ---
 
 v3 - v4:
 
  - use new tlb map syntax
  - check that tlb map mapped everything
  - always save/restore r3 for cpu_init_early_f
  - remove CONFIG_DYNAMIC_CCSRBAR
  - make init_tlbs weak and remove all init_tlbs_dynamic() cruft
  - always map fdt to fixed virtual address
  - fold together AS=1 fdt map functions
  - use tlb_map_range for CCSR map
  - search for PCI based on device type rather than compatible
  - remove unnecessary cast
  - call get_linear_ram_size directly
  - override get_tbclk with timebase-frequency fetching code
 ---
 
 snip
 
 diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c 
 b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
 index 993b8b8..998781b 100644
 --- a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
 +++ b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
 @@ -79,7 +79,7 @@ void setup_ifc(void)
 #endif
 
 /* We run cpu_init_early_f in AS = 1 */
 -void cpu_init_early_f(void)
 +void cpu_init_early_f(void *fdt)
 {
u32 mas0, mas1, mas2, mas3, mas7;
int i;
 @@ -102,6 +102,12 @@ void cpu_init_early_f(void)
for (i = 0; i  sizeof(gd_t); i++)
((char *)gd)[i] = 0;
 
 +/*
 + * CONFIG_SYS_CCSRBAR_PHYS below may use gd-fdt_blob on ePAPR systems,
 + * so we need to populate it before it accesses it.
 + */
 +gd-fdt_blob = fdt;
 +
 
 Alex,
 
 I am thinking to add back #ifdef CONFIG_QEMU_E500 you had for your v3 and
 earlier version for the above change. Do you have any concerns?
 
 The reason I am proposing this is to use generic board architecture.
 gd-fdt_blob is used by some other functions to detect if device tree exists.
 Your change takes r3 from entry and treat it as device tree. It is not the 
 case
 for a normal metal SoC. I need to clear this variable.

No complaints at all from me here :)

Alex

 
 York
 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2014-04-28 Thread York Sun
On 04/28/2014 02:27 PM, Alexander Graf wrote:
 I am thinking to add back #ifdef CONFIG_QEMU_E500 you had for your v3 and
 earlier version for the above change. Do you have any concerns?

 The reason I am proposing this is to use generic board architecture.
 gd-fdt_blob is used by some other functions to detect if device tree exists.
 Your change takes r3 from entry and treat it as device tree. It is not the 
 case
 for a normal metal SoC. I need to clear this variable.
 
 No complaints at all from me here :)
 

Thanks. I will submit a patch to fix it.

York


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2014-04-11 Thread Alexander Graf


On 19.03.14 19:49, York Sun wrote:

On 02/20/2014 07:59 AM, Scott Wood wrote:

On Thu, 2014-02-20 at 13:52 +0100, Alexander Graf wrote:

diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
index 3e99b07..51f2085 100644
--- a/arch/powerpc/cpu/mpc85xx/cpu.c
+++ b/arch/powerpc/cpu/mpc85xx/cpu.c
@@ -272,6 +272,7 @@ int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char * 
const argv[])
  #ifndef CONFIG_SYS_FSL_TBCLK_DIV
  #define CONFIG_SYS_FSL_TBCLK_DIV 8
  #endif
+__attribute__((weak))
  unsigned long get_tbclk (void)
  {
unsigned long tbclk_div = CONFIG_SYS_FSL_TBCLK_DIV;

It's not a big deal, but we do have __weak available.


+static void map_fdt_as(int esel)
+{
+   u32 mas0, mas1, mas2, mas3, mas7;
+   uint64_t fdt_phys = get_fdt_phys();
+   unsigned long fdt_phys_tlb = fdt_phys  ~0xful;
+   unsigned long fdt_virt_tlb = (ulong)get_fdt_virt()  ~0xful;
+
+   mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(10);

Shouldn't you use the esel parameter here?


Are you going to address this comment with a new version?


Yes :).


Alex

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2014-03-19 Thread York Sun
On 02/20/2014 07:59 AM, Scott Wood wrote:
 On Thu, 2014-02-20 at 13:52 +0100, Alexander Graf wrote:
 diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
 index 3e99b07..51f2085 100644
 --- a/arch/powerpc/cpu/mpc85xx/cpu.c
 +++ b/arch/powerpc/cpu/mpc85xx/cpu.c
 @@ -272,6 +272,7 @@ int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char 
 * const argv[])
  #ifndef CONFIG_SYS_FSL_TBCLK_DIV
  #define CONFIG_SYS_FSL_TBCLK_DIV 8
  #endif
 +__attribute__((weak))
  unsigned long get_tbclk (void)
  {
  unsigned long tbclk_div = CONFIG_SYS_FSL_TBCLK_DIV;
 
 It's not a big deal, but we do have __weak available.
 
 +static void map_fdt_as(int esel)
 +{
 +u32 mas0, mas1, mas2, mas3, mas7;
 +uint64_t fdt_phys = get_fdt_phys();
 +unsigned long fdt_phys_tlb = fdt_phys  ~0xful;
 +unsigned long fdt_virt_tlb = (ulong)get_fdt_virt()  ~0xful;
 +
 +mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(10);
 
 Shouldn't you use the esel parameter here?
 

Are you going to address this comment with a new version?

York


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2014-02-20 Thread Scott Wood
On Thu, 2014-02-20 at 13:52 +0100, Alexander Graf wrote:
 diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
 index 3e99b07..51f2085 100644
 --- a/arch/powerpc/cpu/mpc85xx/cpu.c
 +++ b/arch/powerpc/cpu/mpc85xx/cpu.c
 @@ -272,6 +272,7 @@ int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char 
 * const argv[])
  #ifndef CONFIG_SYS_FSL_TBCLK_DIV
  #define CONFIG_SYS_FSL_TBCLK_DIV 8
  #endif
 +__attribute__((weak))
  unsigned long get_tbclk (void)
  {
   unsigned long tbclk_div = CONFIG_SYS_FSL_TBCLK_DIV;

It's not a big deal, but we do have __weak available.

 +static void map_fdt_as(int esel)
 +{
 + u32 mas0, mas1, mas2, mas3, mas7;
 + uint64_t fdt_phys = get_fdt_phys();
 + unsigned long fdt_phys_tlb = fdt_phys  ~0xful;
 + unsigned long fdt_virt_tlb = (ulong)get_fdt_virt()  ~0xful;
 +
 + mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(10);

Shouldn't you use the esel parameter here?

-Scott


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot