| Message ID | 1379372965-22359-19-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17 Sep 2013, at 00:09, Rob Herring <robherring2@gmail.com> wrote: > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -147,24 +147,6 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys) > pr_info("Machine: %s\n", machine_name); > } > > -void __init early_init_dt_add_memory_arch(u64 base, u64 size) > -{ > - base &= PAGE_MASK; > - size &= PAGE_MASK; > - if (base + size < PHYS_OFFSET) { > - pr_warning("Ignoring memory block 0x%llx - 0x%llx\n", > - base, base + size); > - return; > - } > - if (base < PHYS_OFFSET) { > - pr_warning("Ignoring memory range 0x%llx - 0x%llx\n", > - base, PHYS_OFFSET); > - size -= PHYS_OFFSET - base; > - base = PHYS_OFFSET; > - } > - memblock_add(base, size); > -} > - > /* > * Limit the memory size that was specified via FDT. > */ ... > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -688,6 +688,17 @@ u64 __init dt_mem_next_cell(int s, __be32 **cellp) > return of_read_number(p, s); > } > > +void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) > +{ > +#ifdef CONFIG_HAVE_MEMBLOCK > + base &= PAGE_MASK; > + size &= PAGE_MASK; > + memblock_add(base, size); > +#else > + pr_err("%s: ignoring memory (%llx, %llx)\n", __func__, base, size); > +#endif > +} Are the arm64 changes equivalent here? There are some safety checks to cope with the kernel being loaded at a higher offset than the recommended one (PHYS_OFFSET calculated automatically). Catalin
On Tue, Sep 17, 2013 at 3:46 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On 17 Sep 2013, at 00:09, Rob Herring <robherring2@gmail.com> wrote: >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -147,24 +147,6 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys) >> pr_info("Machine: %s\n", machine_name); >> } >> >> -void __init early_init_dt_add_memory_arch(u64 base, u64 size) >> -{ >> - base &= PAGE_MASK; >> - size &= PAGE_MASK; >> - if (base + size < PHYS_OFFSET) { >> - pr_warning("Ignoring memory block 0x%llx - 0x%llx\n", >> - base, base + size); >> - return; >> - } >> - if (base < PHYS_OFFSET) { >> - pr_warning("Ignoring memory range 0x%llx - 0x%llx\n", >> - base, PHYS_OFFSET); >> - size -= PHYS_OFFSET - base; >> - base = PHYS_OFFSET; >> - } >> - memblock_add(base, size); >> -} >> - >> /* >> * Limit the memory size that was specified via FDT. >> */ > > ... > >> --- a/drivers/of/fdt.c >> +++ b/drivers/of/fdt.c >> @@ -688,6 +688,17 @@ u64 __init dt_mem_next_cell(int s, __be32 **cellp) >> return of_read_number(p, s); >> } >> >> +void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) >> +{ >> +#ifdef CONFIG_HAVE_MEMBLOCK >> + base &= PAGE_MASK; >> + size &= PAGE_MASK; >> + memblock_add(base, size); >> +#else >> + pr_err("%s: ignoring memory (%llx, %llx)\n", __func__, base, size); >> +#endif >> +} > > Are the arm64 changes equivalent here? There are some safety checks to > cope with the kernel being loaded at a higher offset than the > recommended one (PHYS_OFFSET calculated automatically). I tried to keep that, but PHYS_OFFSET is not universally defined. My reasoning is this range checking is hardly specific to an architecture. Perhaps if memory always starts at 0 you don't need it. If arm64 really needs these checks, then all architectures do. Perhaps "__virt_to_phys(PAGE_OFFSET)" instead of PHYS_OFFSET would work for all? Rob
On Tue, Sep 17, 2013 at 02:01:36PM +0100, Rob Herring wrote: > On Tue, Sep 17, 2013 at 3:46 AM, Catalin Marinas > <catalin.marinas@arm.com> wrote: > > On 17 Sep 2013, at 00:09, Rob Herring <robherring2@gmail.com> wrote: > >> --- a/arch/arm64/kernel/setup.c > >> +++ b/arch/arm64/kernel/setup.c > >> @@ -147,24 +147,6 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys) > >> pr_info("Machine: %s\n", machine_name); > >> } > >> > >> -void __init early_init_dt_add_memory_arch(u64 base, u64 size) > >> -{ > >> - base &= PAGE_MASK; > >> - size &= PAGE_MASK; > >> - if (base + size < PHYS_OFFSET) { > >> - pr_warning("Ignoring memory block 0x%llx - 0x%llx\n", > >> - base, base + size); > >> - return; > >> - } > >> - if (base < PHYS_OFFSET) { > >> - pr_warning("Ignoring memory range 0x%llx - 0x%llx\n", > >> - base, PHYS_OFFSET); > >> - size -= PHYS_OFFSET - base; > >> - base = PHYS_OFFSET; > >> - } > >> - memblock_add(base, size); > >> -} > >> - > >> /* > >> * Limit the memory size that was specified via FDT. > >> */ > > > > ... > > > >> --- a/drivers/of/fdt.c > >> +++ b/drivers/of/fdt.c > >> @@ -688,6 +688,17 @@ u64 __init dt_mem_next_cell(int s, __be32 **cellp) > >> return of_read_number(p, s); > >> } > >> > >> +void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) > >> +{ > >> +#ifdef CONFIG_HAVE_MEMBLOCK > >> + base &= PAGE_MASK; > >> + size &= PAGE_MASK; > >> + memblock_add(base, size); > >> +#else > >> + pr_err("%s: ignoring memory (%llx, %llx)\n", __func__, base, size); > >> +#endif > >> +} > > > > Are the arm64 changes equivalent here? There are some safety checks to > > cope with the kernel being loaded at a higher offset than the > > recommended one (PHYS_OFFSET calculated automatically). > > I tried to keep that, but PHYS_OFFSET is not universally defined. My > reasoning is this range checking is hardly specific to an > architecture. Perhaps if memory always starts at 0 you don't need it. > If arm64 really needs these checks, then all architectures do. > > Perhaps "__virt_to_phys(PAGE_OFFSET)" instead of PHYS_OFFSET would work for all? I think virt_to_phys() or __pa() should work, the __virt_to_phys() is only defined by a few architectures.
On Mon, 16 Sep 2013 18:09:14 -0500, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <rob.herring@calxeda.com> > > Create a weak version of early_init_dt_add_memory_arch which uses > memblock or is an empty function when memblock is not enabled. This > will unify all architectures except ones with custom memory bank > structs. Two comments below, but otherwise: Acked-by: Grant Likely <grant.likely@linaro.org> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index 0714dd4..a9dce7a 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -688,6 +688,17 @@ u64 __init dt_mem_next_cell(int s, __be32 **cellp) > return of_read_number(p, s); > } > > +void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) > +{ > +#ifdef CONFIG_HAVE_MEMBLOCK > + base &= PAGE_MASK; > + size &= PAGE_MASK; > + memblock_add(base, size); > +#else > + pr_err("%s: ignoring memory (%llx, %llx)\n", __func__, base, size); > +#endif > +} > + Can you do it this way instead: #ifdef CONFIG_HAVE_MEMBLOCK void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) { base &= PAGE_MASK; size &= PAGE_MASK; memblock_add(base, size); } #endif If the platform doesn't provide an early_init_dt_add_memory_arch() function and it doesn't have a memblock implementation, then the build should outright fail. I don't see a scenario where we would want to successfully build the kernel without a working add memory function. Also, can you group this function with the common __weak early_init_dt_alloc_memory_arch() implementation? It would be good to group all the memblock specific functions together. g.
On Tue, Sep 17, 2013 at 10:33 PM, Grant Likely <grant.likely@linaro.org> wrote: > On Mon, 16 Sep 2013 18:09:14 -0500, Rob Herring <robherring2@gmail.com> wrote: >> From: Rob Herring <rob.herring@calxeda.com> >> >> Create a weak version of early_init_dt_add_memory_arch which uses >> memblock or is an empty function when memblock is not enabled. This >> will unify all architectures except ones with custom memory bank >> structs. > > Two comments below, but otherwise: > > Acked-by: Grant Likely <grant.likely@linaro.org> > >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> index 0714dd4..a9dce7a 100644 >> --- a/drivers/of/fdt.c >> +++ b/drivers/of/fdt.c >> @@ -688,6 +688,17 @@ u64 __init dt_mem_next_cell(int s, __be32 **cellp) >> return of_read_number(p, s); >> } >> >> +void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) >> +{ >> +#ifdef CONFIG_HAVE_MEMBLOCK >> + base &= PAGE_MASK; >> + size &= PAGE_MASK; >> + memblock_add(base, size); >> +#else >> + pr_err("%s: ignoring memory (%llx, %llx)\n", __func__, base, size); >> +#endif >> +} >> + > > Can you do it this way instead: > > #ifdef CONFIG_HAVE_MEMBLOCK > void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) > { > base &= PAGE_MASK; > size &= PAGE_MASK; > memblock_add(base, size); > } > #endif > > If the platform doesn't provide an early_init_dt_add_memory_arch() > function and it doesn't have a memblock implementation, then the build > should outright fail. I don't see a scenario where we would want to > successfully build the kernel without a working add memory function. metag and x86 both have empty functions. I guess they get memory from a different boot interface. Rob > > Also, can you group this function with the common __weak > early_init_dt_alloc_memory_arch() implementation? It would be good to > group all the memblock specific functions together. > > g. >
On Wed, 18 Sep 2013 10:09:40 -0500, Rob Herring <robherring2@gmail.com> wrote: > On Tue, Sep 17, 2013 at 10:33 PM, Grant Likely <grant.likely@linaro.org> wrote: > > On Mon, 16 Sep 2013 18:09:14 -0500, Rob Herring <robherring2@gmail.com> wrote: > >> From: Rob Herring <rob.herring@calxeda.com> > >> > >> Create a weak version of early_init_dt_add_memory_arch which uses > >> memblock or is an empty function when memblock is not enabled. This > >> will unify all architectures except ones with custom memory bank > >> structs. > > > > Two comments below, but otherwise: > > > > Acked-by: Grant Likely <grant.likely@linaro.org> > > > >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > >> index 0714dd4..a9dce7a 100644 > >> --- a/drivers/of/fdt.c > >> +++ b/drivers/of/fdt.c > >> @@ -688,6 +688,17 @@ u64 __init dt_mem_next_cell(int s, __be32 **cellp) > >> return of_read_number(p, s); > >> } > >> > >> +void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) > >> +{ > >> +#ifdef CONFIG_HAVE_MEMBLOCK > >> + base &= PAGE_MASK; > >> + size &= PAGE_MASK; > >> + memblock_add(base, size); > >> +#else > >> + pr_err("%s: ignoring memory (%llx, %llx)\n", __func__, base, size); > >> +#endif > >> +} > >> + > > > > Can you do it this way instead: > > > > #ifdef CONFIG_HAVE_MEMBLOCK > > void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) > > { > > base &= PAGE_MASK; > > size &= PAGE_MASK; > > memblock_add(base, size); > > } > > #endif > > > > If the platform doesn't provide an early_init_dt_add_memory_arch() > > function and it doesn't have a memblock implementation, then the build > > should outright fail. I don't see a scenario where we would want to > > successfully build the kernel without a working add memory function. > > metag and x86 both have empty functions. I guess they get memory from > a different boot interface. I would put the exceptions into arch/x86 and arch/metag then. The default answer should be that early_init_dt_add_memory_arch() works, and the build will fail if they aren't implemented. If it really is valid to have an empty implementation, then the architecture should have to do something special to get that. g.
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index b4461e1..3790004 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -147,24 +147,6 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys) pr_info("Machine: %s\n", machine_name); } -void __init early_init_dt_add_memory_arch(u64 base, u64 size) -{ - base &= PAGE_MASK; - size &= PAGE_MASK; - if (base + size < PHYS_OFFSET) { - pr_warning("Ignoring memory block 0x%llx - 0x%llx\n", - base, base + size); - return; - } - if (base < PHYS_OFFSET) { - pr_warning("Ignoring memory range 0x%llx - 0x%llx\n", - base, PHYS_OFFSET); - size -= PHYS_OFFSET - base; - base = PHYS_OFFSET; - } - memblock_add(base, size); -} - /* * Limit the memory size that was specified via FDT. */ diff --git a/arch/metag/kernel/devtree.c b/arch/metag/kernel/devtree.c index 049af56..2c6ee6d 100644 --- a/arch/metag/kernel/devtree.c +++ b/arch/metag/kernel/devtree.c @@ -23,12 +23,6 @@ #include <asm/page.h> #include <asm/mach/arch.h> -void __init early_init_dt_add_memory_arch(u64 base, u64 size) -{ - pr_err("%s(%llx, %llx)\n", - __func__, base, size); -} - void * __init early_init_dt_alloc_memory_arch(u64 size, u64 align) { return alloc_bootmem_align(size, align); diff --git a/arch/microblaze/kernel/prom.c b/arch/microblaze/kernel/prom.c index e13686e..951e4d6 100644 --- a/arch/microblaze/kernel/prom.c +++ b/arch/microblaze/kernel/prom.c @@ -41,11 +41,6 @@ #include <asm/sections.h> #include <asm/pci-bridge.h> -void __init early_init_dt_add_memory_arch(u64 base, u64 size) -{ - memblock_add(base, size); -} - #ifdef CONFIG_EARLY_PRINTK static char *stdout; diff --git a/arch/openrisc/kernel/prom.c b/arch/openrisc/kernel/prom.c index fbed459..6dbcaa8 100644 --- a/arch/openrisc/kernel/prom.c +++ b/arch/openrisc/kernel/prom.c @@ -47,12 +47,6 @@ #include <asm/sections.h> #include <asm/setup.h> -void __init early_init_dt_add_memory_arch(u64 base, u64 size) -{ - size &= PAGE_MASK; - memblock_add(base, size); -} - void __init early_init_devtree(void *params) { early_init_dt_scan(params); diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c index 0db805c..2f5cb37 100644 --- a/arch/x86/kernel/devicetree.c +++ b/arch/x86/kernel/devicetree.c @@ -36,16 +36,6 @@ unsigned long pci_address_to_pio(phys_addr_t address) } EXPORT_SYMBOL_GPL(pci_address_to_pio); -void __init early_init_dt_scan_chosen_arch(unsigned long node) -{ - BUG(); -} - -void __init early_init_dt_add_memory_arch(u64 base, u64 size) -{ - BUG(); -} - void * __init early_init_dt_alloc_memory_arch(u64 size, u64 align) { return __alloc_bootmem(size, align, __pa(MAX_DMA_ADDRESS)); diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 0714dd4..a9dce7a 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -688,6 +688,17 @@ u64 __init dt_mem_next_cell(int s, __be32 **cellp) return of_read_number(p, s); } +void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) +{ +#ifdef CONFIG_HAVE_MEMBLOCK + base &= PAGE_MASK; + size &= PAGE_MASK; + memblock_add(base, size); +#else + pr_err("%s: ignoring memory (%llx, %llx)\n", __func__, base, size); +#endif +} + /** * early_init_dt_scan_memory - Look for an parse memory nodes */