Patchwork [1/2] powerpc: add 16K/64K pages support for the 44x PPC32 architectures.

login
register
mail settings
Submitter Hollis Blanchard
Date Nov. 3, 2008, 7:55 p.m.
Message ID <1225742121.24019.9.camel@localhost.localdomain>
Download mbox | patch
Permalink /patch/6960/
State Superseded
Delegated to: Josh Boyer
Headers show

Comments

Hollis Blanchard - Nov. 3, 2008, 7:55 p.m.
On Mon, 2008-11-03 at 11:43 +1100, Benjamin Herrenschmidt wrote:
> > Cropping the size of the memory node.  That was simplest to do from the
> > cuboot wrapper at the time.  If marking it reserved via a reserve map
> > is more elegant and correct, we could do that.
> > 
> > But I will still like to know what about the other way is hairy please.
> 
> I don't like it :-) Bad feeling ... don't like having a memory
> node entry that isn't aligned to some large power of two typically.

More specifically, mm/bootmem.c seems to be making the implicit
assumption that memory size is an even multiple of PAGE_SIZE. With 4K
pages, 0xffff000 bytes of RAM fits; with 64K pages it does not.

Using the device tree reserve map stuff does indeed seem to solve the
problem. However, I really don't understand the layering in
arch/powerpc/boot at all, so I'll just put this patch out here and
people can play with wrappers and prototypes all they want:



powerpc/4xx: work around CHIP11 errata in a more PAGE_SIZE-friendly way

The current CHIP11 errata truncates the device tree memory node, and assumes a
4K page size. This breaks kernels with non-4K PAGE_SIZE.

Instead, use a device tree memory reservation to reserve only the 256 bytes
actually affected by the errata, leaving the total memory size unaltered.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
Josh Boyer - Nov. 3, 2008, 8 p.m.
On Mon, 03 Nov 2008 13:55:21 -0600
Hollis Blanchard <hollisb@us.ibm.com> wrote:

> On Mon, 2008-11-03 at 11:43 +1100, Benjamin Herrenschmidt wrote:
> > > Cropping the size of the memory node.  That was simplest to do from the
> > > cuboot wrapper at the time.  If marking it reserved via a reserve map
> > > is more elegant and correct, we could do that.
> > > 
> > > But I will still like to know what about the other way is hairy please.
> > 
> > I don't like it :-) Bad feeling ... don't like having a memory
> > node entry that isn't aligned to some large power of two typically.
> 
> More specifically, mm/bootmem.c seems to be making the implicit
> assumption that memory size is an even multiple of PAGE_SIZE. With 4K
> pages, 0xffff000 bytes of RAM fits; with 64K pages it does not.

Hmm..  I dunno what to think about that.  Again, how does mem= play
into this?  (I will look myself in a bit, but if someone knows offhand
that would be nice..)

> Using the device tree reserve map stuff does indeed seem to solve the
> problem. However, I really don't understand the layering in
> arch/powerpc/boot at all, so I'll just put this patch out here and
> people can play with wrappers and prototypes all they want:

This actually looks pretty nice.  I'll wait for David to Ack the fdt
parts.

josh

> powerpc/4xx: work around CHIP11 errata in a more PAGE_SIZE-friendly way
> 
> The current CHIP11 errata truncates the device tree memory node, and assumes a
> 4K page size. This breaks kernels with non-4K PAGE_SIZE.
> 
> Instead, use a device tree memory reservation to reserve only the 256 bytes
> actually affected by the errata, leaving the total memory size unaltered.
> 
> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
> 
> diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c
> --- a/arch/powerpc/boot/4xx.c
> +++ b/arch/powerpc/boot/4xx.c
> @@ -21,7 +21,7 @@
>  #include "reg.h"
>  #include "dcr.h"
> 
> -static unsigned long chip_11_errata(unsigned long memsize)
> +static void chip_11_errata(unsigned long memsize)
>  {
>  	unsigned long pvr;
> 
> @@ -31,13 +31,11 @@ static unsigned long chip_11_errata(unsi
>  		case 0x40000850:
>  		case 0x400008d0:
>  		case 0x200008d0:
> -			memsize -= 4096;
> +			fdt_reserve_mem(memsize - 256, 256);
>  			break;
>  		default:
>  			break;
>  	}
> -
> -	return memsize;
>  }
> 
>  /* Read the 4xx SDRAM controller to get size of system memory. */
> @@ -53,7 +51,7 @@ void ibm4xx_sdram_fixup_memsize(void)
>  			memsize += SDRAM_CONFIG_BANK_SIZE(bank_config);
>  	}
> 
> -	memsize = chip_11_errata(memsize);
> +	chip_11_errata(memsize);
>  	dt_fixup_memory(0, memsize);
>  }
> 
> @@ -219,7 +217,7 @@ void ibm4xx_denali_fixup_memsize(void)
>  		bank = 4; /* 4 banks */
> 
>  	memsize = cs * (1 << (col+row)) * bank * dpath;
> -	memsize = chip_11_errata(memsize);
> +	chip_11_errata(memsize);
>  	dt_fixup_memory(0, memsize);
>  }
> 
> diff --git a/arch/powerpc/boot/libfdt-wrapper.c b/arch/powerpc/boot/libfdt-wrapper.c
> --- a/arch/powerpc/boot/libfdt-wrapper.c
> +++ b/arch/powerpc/boot/libfdt-wrapper.c
> @@ -167,6 +167,11 @@ static unsigned long fdt_wrapper_finaliz
>  	return (unsigned long)fdt;
>  }
> 
> +int fdt_reserve_mem(unsigned long addr, unsigned long bytes)
> +{
> +	return fdt_add_mem_rsv(fdt, addr, bytes);
> +}
> +
>  void fdt_init(void *blob)
>  {
>  	int err;
> diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
> --- a/arch/powerpc/boot/ops.h
> +++ b/arch/powerpc/boot/ops.h
> @@ -83,6 +83,7 @@ extern struct loader_info loader_info;
> 
>  void start(void);
>  void fdt_init(void *blob);
> +int fdt_reserve_mem(unsigned long addr, unsigned long bytes);
>  int serial_console_init(void);
>  int ns16550_console_init(void *devp, struct serial_console_data *scdp);
>  int mpsc_console_init(void *devp, struct serial_console_data *scdp);
> 
>
Hollis Blanchard - Nov. 5, 2008, 5:33 p.m.
On Mon, 2008-11-03 at 15:00 -0500, Josh Boyer wrote:
> On Mon, 03 Nov 2008 13:55:21 -0600
> Hollis Blanchard <hollisb@us.ibm.com> wrote:
> 
> > On Mon, 2008-11-03 at 11:43 +1100, Benjamin Herrenschmidt wrote:
> > > > Cropping the size of the memory node.  That was simplest to do from the
> > > > cuboot wrapper at the time.  If marking it reserved via a reserve map
> > > > is more elegant and correct, we could do that.
> > > > 
> > > > But I will still like to know what about the other way is hairy please.
> > > 
> > > I don't like it :-) Bad feeling ... don't like having a memory
> > > node entry that isn't aligned to some large power of two typically.
> > 
> > More specifically, mm/bootmem.c seems to be making the implicit
> > assumption that memory size is an even multiple of PAGE_SIZE. With 4K
> > pages, 0xffff000 bytes of RAM fits; with 64K pages it does not.
> 
> Hmm..  I dunno what to think about that.  Again, how does mem= play
> into this?  (I will look myself in a bit, but if someone knows offhand
> that would be nice..)
> 
> > Using the device tree reserve map stuff does indeed seem to solve the
> > problem. However, I really don't understand the layering in
> > arch/powerpc/boot at all, so I'll just put this patch out here and
> > people can play with wrappers and prototypes all they want:
> 
> This actually looks pretty nice.  I'll wait for David to Ack the fdt
> parts.

David?
David Gibson - Nov. 6, 2008, 1:48 a.m.
On Wed, Nov 05, 2008 at 11:33:28AM -0600, Hollis Blanchard wrote:
> On Mon, 2008-11-03 at 15:00 -0500, Josh Boyer wrote:
> > On Mon, 03 Nov 2008 13:55:21 -0600
> > Hollis Blanchard <hollisb@us.ibm.com> wrote:
> > 
> > > On Mon, 2008-11-03 at 11:43 +1100, Benjamin Herrenschmidt wrote:
> > > > > Cropping the size of the memory node.  That was simplest to do from the
> > > > > cuboot wrapper at the time.  If marking it reserved via a reserve map
> > > > > is more elegant and correct, we could do that.
> > > > > 
> > > > > But I will still like to know what about the other way is hairy please.
> > > > 
> > > > I don't like it :-) Bad feeling ... don't like having a memory
> > > > node entry that isn't aligned to some large power of two typically.
> > > 
> > > More specifically, mm/bootmem.c seems to be making the implicit
> > > assumption that memory size is an even multiple of PAGE_SIZE. With 4K
> > > pages, 0xffff000 bytes of RAM fits; with 64K pages it does not.
> > 
> > Hmm..  I dunno what to think about that.  Again, how does mem= play
> > into this?  (I will look myself in a bit, but if someone knows offhand
> > that would be nice..)
> > 
> > > Using the device tree reserve map stuff does indeed seem to solve the
> > > problem. However, I really don't understand the layering in
> > > arch/powerpc/boot at all, so I'll just put this patch out here and
> > > people can play with wrappers and prototypes all they want:
> > 
> > This actually looks pretty nice.  I'll wait for David to Ack the fdt
> > parts.
> 
> David?

Sorry, I've been on leave for a few days.

I assume you mean the new call through to fdt_add_mem_rsv().

Hrm.. currently all the things in fdt_wrapper are hooks called through
dt_ops.  Adding such a trivial wrapper seems a little silly.  There
have been other people wanting to use other libfdt features directly,
knowing that they have a flat tree on their system.  I think it would
be more sensible, I think, to just expose the global fdt pointer, so
that people can use the libfdt functions directly, without having to
go through the wrapper code.

Unless of course there is occasion to use this "add reserve" callback
on real OF systems, in which case it should be a new dt_ops hook.

Patch

diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c
--- a/arch/powerpc/boot/4xx.c
+++ b/arch/powerpc/boot/4xx.c
@@ -21,7 +21,7 @@ 
 #include "reg.h"
 #include "dcr.h"
 
-static unsigned long chip_11_errata(unsigned long memsize)
+static void chip_11_errata(unsigned long memsize)
 {
 	unsigned long pvr;
 
@@ -31,13 +31,11 @@  static unsigned long chip_11_errata(unsi
 		case 0x40000850:
 		case 0x400008d0:
 		case 0x200008d0:
-			memsize -= 4096;
+			fdt_reserve_mem(memsize - 256, 256);
 			break;
 		default:
 			break;
 	}
-
-	return memsize;
 }
 
 /* Read the 4xx SDRAM controller to get size of system memory. */
@@ -53,7 +51,7 @@  void ibm4xx_sdram_fixup_memsize(void)
 			memsize += SDRAM_CONFIG_BANK_SIZE(bank_config);
 	}
 
-	memsize = chip_11_errata(memsize);
+	chip_11_errata(memsize);
 	dt_fixup_memory(0, memsize);
 }
 
@@ -219,7 +217,7 @@  void ibm4xx_denali_fixup_memsize(void)
 		bank = 4; /* 4 banks */
 
 	memsize = cs * (1 << (col+row)) * bank * dpath;
-	memsize = chip_11_errata(memsize);
+	chip_11_errata(memsize);
 	dt_fixup_memory(0, memsize);
 }
 
diff --git a/arch/powerpc/boot/libfdt-wrapper.c b/arch/powerpc/boot/libfdt-wrapper.c
--- a/arch/powerpc/boot/libfdt-wrapper.c
+++ b/arch/powerpc/boot/libfdt-wrapper.c
@@ -167,6 +167,11 @@  static unsigned long fdt_wrapper_finaliz
 	return (unsigned long)fdt;
 }
 
+int fdt_reserve_mem(unsigned long addr, unsigned long bytes)
+{
+	return fdt_add_mem_rsv(fdt, addr, bytes);
+}
+
 void fdt_init(void *blob)
 {
 	int err;
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -83,6 +83,7 @@  extern struct loader_info loader_info;
 
 void start(void);
 void fdt_init(void *blob);
+int fdt_reserve_mem(unsigned long addr, unsigned long bytes);
 int serial_console_init(void);
 int ns16550_console_init(void *devp, struct serial_console_data *scdp);
 int mpsc_console_init(void *devp, struct serial_console_data *scdp);