| Message ID | 1225742121.24019.9.camel@localhost.localdomain (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Josh Boyer |
| Headers | show |
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); > >
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?
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.
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);