Hello Thomas On Thu, Oct 08, 2020 at 10:43:54AM +0200, Thomas Bogendoerfer wrote: > add_memory_region was the old interface for registering memory and > was already changed to used memblock internaly. Replace it by > directly calling memblock functions.
Thanks for suggesting this cleanup. It's great to see that the leftover of the old bootmem and MIPS-specific boot_mem_map framework time is going to be finally removed. A few comments are blow. > > Signed-off-by: Thomas Bogendoerfer <[email protected]> > --- > Changes in v2: > fixed missing memblock include in fw/sni/sniprom.c > tested on cobalt, IP22, IP28, IP30, IP32, JAZZ, SNI ... > diff --git a/arch/mips/kernel/prom.c b/arch/mips/kernel/prom.c > index 9e50dc8df2f6..fab532cb5a11 100644 > --- a/arch/mips/kernel/prom.c > +++ b/arch/mips/kernel/prom.c > @@ -50,14 +50,18 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 > size) > size = PHYS_ADDR_MAX - base; > } > > - add_memory_region(base, size, BOOT_MEM_RAM); > + memblock_add(base, size); > } > > int __init early_init_dt_reserve_memory_arch(phys_addr_t base, > phys_addr_t size, bool nomap) > { > - add_memory_region(base, size, > - nomap ? BOOT_MEM_NOMAP : BOOT_MEM_RESERVED); > + if (nomap) { > + memblock_remove(base, size); > + } else { > + memblock_add(base, size); > + memblock_reserve(base, size); > + } > > return 0; > } I guess originally the arch-specific early_init_dt_add_memory_arch() and early_init_dt_reserve_memory_arch() methods have been added since MIPS's got its own memory types declarations (BOOT_MEM_RAM, BOOT_MEM_RESERVED, etc) and had had a specific internal system memory regions mapping (add_memory_region(), boot_mem_map, etc). Since the leftover of that framework is going to be removed, we can freely use the standard early_init_dt_add_memory_arch() and early_init_dt_reserve_memory_arch() implementations from drivers/of/fdt.c:1102 drivers/of/fdt.c:1149 . At least I don't see a decent reason to preserve them. The memory registration method does nearly the same sanity checks. The memory reservation function defers a bit in adding the being reserved memory first. That seems redundant, since the reserved memory won't be available for the system anyway. Do I miss something? > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c > index 4c04a86f075b..fb05b66e111f 100644 > --- a/arch/mips/kernel/setup.c > +++ b/arch/mips/kernel/setup.c > @@ -91,45 +91,6 @@ unsigned long ARCH_PFN_OFFSET; > EXPORT_SYMBOL(ARCH_PFN_OFFSET); > #endif > > -void __init add_memory_region(phys_addr_t start, phys_addr_t size, long type) > -{ > - /* > - * Note: This function only exists for historical reason, > - * new code should use memblock_add or memblock_add_node instead. > - */ > - > - /* > - * If the region reaches the top of the physical address space, adjust > - * the size slightly so that (start + size) doesn't overflow > - */ > - if (start + size - 1 == PHYS_ADDR_MAX) > - --size; > - > - /* Sanity check */ > - if (start + size < start) { > - pr_warn("Trying to add an invalid memory region, skipped\n"); > - return; > - } > - > - if (start < PHYS_OFFSET) > - return; > - > - memblock_add(start, size); > - /* Reserve any memory except the ordinary RAM ranges. */ > - switch (type) { > - case BOOT_MEM_RAM: > - break; > - > - case BOOT_MEM_NOMAP: /* Discard the range from the system. */ > - memblock_remove(start, size); > - break; > - > - default: /* Reserve the rest of the memory types at boot time */ > - memblock_reserve(start, size); > - break; > - } > -} > - > void __init detect_memory_region(phys_addr_t start, phys_addr_t sz_min, > phys_addr_t sz_max) > { > void *dm = &detect_magic; > @@ -146,7 +107,7 @@ void __init detect_memory_region(phys_addr_t start, > phys_addr_t sz_min, phys_add > ((unsigned long long) sz_min) / SZ_1M, > ((unsigned long long) sz_max) / SZ_1M); > > - add_memory_region(start, size, BOOT_MEM_RAM); > + memblock_add(start, size); > } > > /* > @@ -400,7 +361,7 @@ static int __init early_parse_mem(char *p) > if (*p == '@') > start = memparse(p + 1, &p); > > - add_memory_region(start, size, BOOT_MEM_RAM); > + memblock_add(start, size); > > return 0; > } > @@ -426,13 +387,14 @@ static int __init early_parse_memmap(char *p) > > if (*p == '@') { > start_at = memparse(p+1, &p); > - add_memory_region(start_at, mem_size, BOOT_MEM_RAM); > + memblock_add(start_at, mem_size); > } else if (*p == '#') { > pr_err("\"memmap=nn#ss\" (force ACPI data) invalid on MIPS\n"); > return -EINVAL; > } else if (*p == '$') { > start_at = memparse(p+1, &p); > - add_memory_region(start_at, mem_size, BOOT_MEM_RESERVED); > + memblock_add(start_at, mem_size); > + memblock_reserve(start_at, mem_size); I suppose we could remove the memory addition from here too. What do you think? > } else { > pr_err("\"memmap\" invalid format!\n"); > return -EINVAL; > @@ -644,7 +606,7 @@ static void __init bootcmdline_init(void) > * arch_mem_init - initialize memory management subsystem > * > * o plat_mem_setup() detects the memory configuration and will record > detected > - * memory areas using add_memory_region. > + * memory areas using memblock_add. > * > * At this stage the memory configuration of the system is known to the > * kernel but generic memory management system is still entirely > uninitialized. > diff --git a/arch/mips/loongson2ef/common/mem.c > b/arch/mips/loongson2ef/common/mem.c > index ae21f1c62baa..057d58bb470e 100644 > --- a/arch/mips/loongson2ef/common/mem.c > +++ b/arch/mips/loongson2ef/common/mem.c > @@ -17,10 +17,7 @@ u32 memsize, highmemsize; > > void __init prom_init_memory(void) > { > - add_memory_region(0x0, (memsize << 20), BOOT_MEM_RAM); > - > - add_memory_region(memsize << 20, LOONGSON_PCI_MEM_START - (memsize << > - 20), BOOT_MEM_RESERVED); > + memblock_add(0x0, (memsize << 20)); Hm, am I missing something or the BOOT_MEM_RESERVED part has been discarded? > > #ifdef CONFIG_CPU_SUPPORTS_ADDRWINCFG > { > @@ -41,12 +38,7 @@ void __init prom_init_memory(void) > > #ifdef CONFIG_64BIT > if (highmemsize > 0) > - add_memory_region(LOONGSON_HIGHMEM_START, > - highmemsize << 20, BOOT_MEM_RAM); > - > - add_memory_region(LOONGSON_PCI_MEM_END + 1, LOONGSON_HIGHMEM_START - > - LOONGSON_PCI_MEM_END - 1, BOOT_MEM_RESERVED); > - > + memblock_add(LOONGSON_HIGHMEM_START, highmemsize << 20); The same question. Is it ok to discard the [LOONGSON_PCI_MEM_END+1:LOONGSON_HIGHMEM_START-LOONGSON_PCI_MEM_END-1] region removal operation? -Sergey > #endif /* !CONFIG_64BIT */ > } >
