Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] MIPS: replace add_memory_region with memblock
@ 2020-10-08  8:43 Thomas Bogendoerfer
  2020-10-08 15:20 ` Serge Semin
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Bogendoerfer @ 2020-10-08  8:43 UTC (permalink / raw)
  To: Hauke Mehrtens, Rafał Miłecki, Florian Fainelli,
	bcm-kernel-feedback-list, Maciej W. Rozycki, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

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.

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
Changes in v2:
	fixed missing memblock include in fw/sni/sniprom.c
	tested on cobalt, IP22, IP28, IP30, IP32, JAZZ, SNI

 arch/mips/alchemy/common/prom.c               |  3 +-
 arch/mips/ar7/memory.c                        |  2 +-
 arch/mips/ath25/ar2315.c                      |  3 +-
 arch/mips/ath25/ar5312.c                      |  3 +-
 arch/mips/bcm47xx/prom.c                      |  3 +-
 arch/mips/bcm47xx/setup.c                     |  2 +-
 arch/mips/bcm63xx/setup.c                     |  2 +-
 arch/mips/cavium-octeon/setup.c               | 24 ++++++-------
 arch/mips/cobalt/setup.c                      |  3 +-
 arch/mips/dec/prom/memory.c                   |  8 ++---
 arch/mips/fw/arc/memory.c                     | 28 ++++++++++-----
 arch/mips/fw/sni/sniprom.c                    |  4 +--
 arch/mips/include/asm/bootinfo.h              |  7 ----
 arch/mips/include/asm/netlogic/psb-bootinfo.h |  1 +
 arch/mips/kernel/prom.c                       | 10 ++++--
 arch/mips/kernel/setup.c                      | 50 ++++-----------------------
 arch/mips/loongson2ef/common/mem.c            | 12 ++-----
 arch/mips/loongson32/common/prom.c            |  4 +--
 arch/mips/netlogic/xlp/setup.c                |  2 +-
 arch/mips/netlogic/xlr/setup.c                |  5 +--
 arch/mips/ralink/of.c                         |  3 +-
 arch/mips/rb532/prom.c                        |  2 +-
 arch/mips/sgi-ip32/ip32-memory.c              |  3 +-
 arch/mips/sibyte/common/cfe.c                 | 16 ++++-----
 arch/mips/txx9/jmr3927/prom.c                 |  4 +--
 arch/mips/txx9/rbtx4927/prom.c                |  5 +--
 arch/mips/txx9/rbtx4938/prom.c                |  3 +-
 arch/mips/txx9/rbtx4939/prom.c                |  4 +--
 28 files changed, 89 insertions(+), 127 deletions(-)

diff --git a/arch/mips/alchemy/common/prom.c b/arch/mips/alchemy/common/prom.c
index cfa203064d3c..d910c0a64de9 100644
--- a/arch/mips/alchemy/common/prom.c
+++ b/arch/mips/alchemy/common/prom.c
@@ -35,6 +35,7 @@
 
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/memblock.h>
 #include <linux/sizes.h>
 #include <linux/string.h>
 
@@ -93,7 +94,7 @@ void __init prom_init(void)
 	if (!memsize_str || kstrtoul(memsize_str, 0, &memsize))
 		memsize = SZ_64M; /* minimum memsize is 64MB RAM */
 
-	add_memory_region(0, memsize, BOOT_MEM_RAM);
+	memblock_add(0, memsize);
 }
 
 static inline unsigned char str2hexnum(unsigned char c)
diff --git a/arch/mips/ar7/memory.c b/arch/mips/ar7/memory.c
index ad6efb36ebfe..787716c5e946 100644
--- a/arch/mips/ar7/memory.c
+++ b/arch/mips/ar7/memory.c
@@ -47,7 +47,7 @@ void __init prom_meminit(void)
 	unsigned long pages;
 
 	pages = memsize() >> PAGE_SHIFT;
-	add_memory_region(PHYS_OFFSET, pages << PAGE_SHIFT, BOOT_MEM_RAM);
+	memblock_add(PHYS_OFFSET, pages << PAGE_SHIFT);
 }
 
 void __init prom_free_prom_memory(void)
diff --git a/arch/mips/ath25/ar2315.c b/arch/mips/ath25/ar2315.c
index e7b53e3960c8..9dbed7b5ea76 100644
--- a/arch/mips/ath25/ar2315.c
+++ b/arch/mips/ath25/ar2315.c
@@ -19,6 +19,7 @@
 #include <linux/bitops.h>
 #include <linux/irqdomain.h>
 #include <linux/interrupt.h>
+#include <linux/memblock.h>
 #include <linux/platform_device.h>
 #include <linux/reboot.h>
 #include <asm/bootinfo.h>
@@ -266,7 +267,7 @@ void __init ar2315_plat_mem_setup(void)
 	memsize <<= 1 + ATH25_REG_MS(memcfg, AR2315_MEM_CFG_COL_WIDTH);
 	memsize <<= 1 + ATH25_REG_MS(memcfg, AR2315_MEM_CFG_ROW_WIDTH);
 	memsize <<= 3;
-	add_memory_region(0, memsize, BOOT_MEM_RAM);
+	memblock_add(0, memsize);
 	iounmap(sdram_base);
 
 	ar2315_rst_base = ioremap(AR2315_RST_BASE, AR2315_RST_SIZE);
diff --git a/arch/mips/ath25/ar5312.c b/arch/mips/ath25/ar5312.c
index 42bf2afb4765..23c879f4b734 100644
--- a/arch/mips/ath25/ar5312.c
+++ b/arch/mips/ath25/ar5312.c
@@ -19,6 +19,7 @@
 #include <linux/bitops.h>
 #include <linux/irqdomain.h>
 #include <linux/interrupt.h>
+#include <linux/memblock.h>
 #include <linux/platform_device.h>
 #include <linux/mtd/physmap.h>
 #include <linux/reboot.h>
@@ -363,7 +364,7 @@ void __init ar5312_plat_mem_setup(void)
 	memsize = (bank0_ac ? (1 << (bank0_ac + 1)) : 0) +
 		  (bank1_ac ? (1 << (bank1_ac + 1)) : 0);
 	memsize <<= 20;
-	add_memory_region(0, memsize, BOOT_MEM_RAM);
+	memblock_add(0, memsize);
 	iounmap(sdram_base);
 
 	ar5312_rst_base = ioremap(AR5312_RST_BASE, AR5312_RST_SIZE);
diff --git a/arch/mips/bcm47xx/prom.c b/arch/mips/bcm47xx/prom.c
index 135a5407f015..3e2a8166377f 100644
--- a/arch/mips/bcm47xx/prom.c
+++ b/arch/mips/bcm47xx/prom.c
@@ -27,6 +27,7 @@
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
+#include <linux/memblock.h>
 #include <linux/spinlock.h>
 #include <linux/ssb/ssb_driver_chipcommon.h>
 #include <linux/ssb/ssb_regs.h>
@@ -97,7 +98,7 @@ static __init void prom_init_mem(void)
 	 */
 	if (c->cputype == CPU_74K && (mem == (128  << 20)))
 		mem -= 0x1000;
-	add_memory_region(0, mem, BOOT_MEM_RAM);
+	memblock_add(0, mem);
 }
 
 /*
diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
index 82627c264964..751997eb1552 100644
--- a/arch/mips/bcm47xx/setup.c
+++ b/arch/mips/bcm47xx/setup.c
@@ -141,7 +141,7 @@ static void __init bcm47xx_register_bcma(void)
 
 /*
  * Memory setup is done in the early part of MIPS's arch_mem_init. It's supposed
- * to detect memory and record it with add_memory_region.
+ * to detect memory and record it with memblock_add.
  * Any extra initializaion performed here must not use kmalloc or bootmem.
  */
 void __init plat_mem_setup(void)
diff --git a/arch/mips/bcm63xx/setup.c b/arch/mips/bcm63xx/setup.c
index e28ee9a7cc7e..d811e3e03f81 100644
--- a/arch/mips/bcm63xx/setup.c
+++ b/arch/mips/bcm63xx/setup.c
@@ -146,7 +146,7 @@ void __init plat_time_init(void)
 
 void __init plat_mem_setup(void)
 {
-	add_memory_region(0, bcm63xx_get_memory_size(), BOOT_MEM_RAM);
+	memblock_add(0, bcm63xx_get_memory_size());
 
 	_machine_halt = bcm63xx_machine_halt;
 	_machine_restart = __bcm63xx_machine_reboot;
diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
index 8a357cb068c2..561389d3fadb 100644
--- a/arch/mips/cavium-octeon/setup.c
+++ b/arch/mips/cavium-octeon/setup.c
@@ -16,6 +16,7 @@
 #include <linux/export.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/memblock.h>
 #include <linux/serial.h>
 #include <linux/smp.h>
 #include <linux/types.h>
@@ -930,7 +931,7 @@ static __init void memory_exclude_page(u64 addr, u64 *mem, u64 *size)
 {
 	if (addr > *mem && addr < *mem + *size) {
 		u64 inc = addr - *mem;
-		add_memory_region(*mem, inc, BOOT_MEM_RAM);
+		memblock_add(*mem, inc);
 		*mem += inc;
 		*size -= inc;
 	}
@@ -992,19 +993,18 @@ void __init plat_mem_setup(void)
 
 /* Crashkernel ignores bootmem list. It relies on mem=X@Y option */
 #ifdef CONFIG_CRASH_DUMP
-	add_memory_region(reserve_low_mem, max_memory, BOOT_MEM_RAM);
+	memblock_add(reserve_low_mem, max_memory);
 	total += max_memory;
 #else
 #ifdef CONFIG_KEXEC
 	if (crashk_size > 0) {
-		add_memory_region(crashk_base, crashk_size, BOOT_MEM_RAM);
+		memblock_add(crashk_base, crashk_size);
 		crashk_end = crashk_base + crashk_size;
 	}
 #endif
 	/*
-	 * When allocating memory, we want incrementing addresses from
-	 * bootmem_alloc so the code in add_memory_region can merge
-	 * regions next to each other.
+	 * When allocating memory, we want incrementing addresses,
+	 * which is handled by memblock
 	 */
 	cvmx_bootmem_lock();
 	while (total < max_memory) {
@@ -1039,13 +1039,9 @@ void __init plat_mem_setup(void)
 			 */
 			if (memory < crashk_base && end >  crashk_end) {
 				/* region is fully in */
-				add_memory_region(memory,
-						  crashk_base - memory,
-						  BOOT_MEM_RAM);
+				memblock_add(memory, crashk_base - memory);
 				total += crashk_base - memory;
-				add_memory_region(crashk_end,
-						  end - crashk_end,
-						  BOOT_MEM_RAM);
+				memblock_add(crashk_end, end - crashk_end);
 				total += end - crashk_end;
 				continue;
 			}
@@ -1073,7 +1069,7 @@ void __init plat_mem_setup(void)
 				 */
 				mem_alloc_size -= end - crashk_base;
 #endif
-			add_memory_region(memory, mem_alloc_size, BOOT_MEM_RAM);
+			memblock_add(memory, mem_alloc_size);
 			total += mem_alloc_size;
 			/* Recovering mem_alloc_size */
 			mem_alloc_size = 4 << 20;
@@ -1088,7 +1084,7 @@ void __init plat_mem_setup(void)
 
 	/* Adjust for physical offset. */
 	kernel_start &= ~0xffffffff80000000ULL;
-	add_memory_region(kernel_start, kernel_size, BOOT_MEM_RAM);
+	memblock_add(kernel_start, kernel_size);
 #endif /* CONFIG_CRASH_DUMP */
 
 #ifdef CONFIG_CAVIUM_RESERVE32
diff --git a/arch/mips/cobalt/setup.c b/arch/mips/cobalt/setup.c
index c136a18c7221..46581e686882 100644
--- a/arch/mips/cobalt/setup.c
+++ b/arch/mips/cobalt/setup.c
@@ -13,6 +13,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/memblock.h>
 #include <linux/pm.h>
 
 #include <asm/bootinfo.h>
@@ -112,7 +113,7 @@ void __init prom_init(void)
 			strlcat(arcs_cmdline, " ", COMMAND_LINE_SIZE);
 	}
 
-	add_memory_region(0x0, memsz, BOOT_MEM_RAM);
+	memblock_add(0, memsz);
 
 	setup_8250_early_printk_port(CKSEG1ADDR(0x1c800000), 0, 0);
 }
diff --git a/arch/mips/dec/prom/memory.c b/arch/mips/dec/prom/memory.c
index df8e1af20eb7..44490c30d63b 100644
--- a/arch/mips/dec/prom/memory.c
+++ b/arch/mips/dec/prom/memory.c
@@ -12,7 +12,6 @@
 #include <linux/types.h>
 
 #include <asm/addrspace.h>
-#include <asm/bootinfo.h>
 #include <asm/dec/machtype.h>
 #include <asm/dec/prom.h>
 #include <asm/page.h>
@@ -50,8 +49,7 @@ static __init void pmax_setup_memory_region(void)
 	}
 	memcpy((void *)(CKSEG0 + 0x80), &old_handler, 0x80);
 
-	add_memory_region(0, (unsigned long)memory_page - CKSEG1 - CHUNK_SIZE,
-			  BOOT_MEM_RAM);
+	memblock_add(0, (unsigned long)memory_page - CKSEG1 - CHUNK_SIZE);
 }
 
 /*
@@ -76,13 +74,13 @@ static __init void rex_setup_memory_region(void)
 		else if (!mem_size)
 			mem_start += (8 * bm->pagesize);
 		else {
-			add_memory_region(mem_start, mem_size, BOOT_MEM_RAM);
+			memblock_add(mem_start, mem_size);
 			mem_start += mem_size + (8 * bm->pagesize);
 			mem_size = 0;
 		}
 	}
 	if (mem_size)
-		add_memory_region(mem_start, mem_size, BOOT_MEM_RAM);
+		memblock_add(mem_start, mem_size);
 }
 
 void __init prom_meminit(u32 magic)
diff --git a/arch/mips/fw/arc/memory.c b/arch/mips/fw/arc/memory.c
index da0712ad85f5..37625ae5e35d 100644
--- a/arch/mips/fw/arc/memory.c
+++ b/arch/mips/fw/arc/memory.c
@@ -68,20 +68,24 @@ static char *arc_mtypes[8] = {
 						: arc_mtypes[a.arc]
 #endif
 
+enum {
+	mem_free, mem_prom_used, mem_reserved
+};
+
 static inline int memtype_classify_arcs(union linux_memtypes type)
 {
 	switch (type.arcs) {
 	case arcs_fcontig:
 	case arcs_free:
-		return BOOT_MEM_RAM;
+		return mem_free;
 	case arcs_atmp:
-		return BOOT_MEM_ROM_DATA;
+		return mem_prom_used;
 	case arcs_eblock:
 	case arcs_rvpage:
 	case arcs_bmem:
 	case arcs_prog:
 	case arcs_aperm:
-		return BOOT_MEM_RESERVED;
+		return mem_reserved;
 	default:
 		BUG();
 	}
@@ -93,15 +97,15 @@ static inline int memtype_classify_arc(union linux_memtypes type)
 	switch (type.arc) {
 	case arc_free:
 	case arc_fcontig:
-		return BOOT_MEM_RAM;
+		return mem_free;
 	case arc_atmp:
-		return BOOT_MEM_ROM_DATA;
+		return mem_prom_used;
 	case arc_eblock:
 	case arc_rvpage:
 	case arc_bmem:
 	case arc_prog:
 	case arc_aperm:
-		return BOOT_MEM_RESERVED;
+		return mem_reserved;
 	default:
 		BUG();
 	}
@@ -143,9 +147,17 @@ void __weak __init prom_meminit(void)
 		size = p->pages << ARC_PAGE_SHIFT;
 		type = prom_memtype_classify(p->type);
 
-		add_memory_region(base, size, type);
+		/* ignore mirrored RAM on IP28/IP30 */
+		if (base < PHYS_OFFSET)
+			continue;
+
+		memblock_add(base, size);
+
+		if (type == mem_reserved)
+			memblock_reserve(base, size);
 
-		if (type == BOOT_MEM_ROM_DATA) {
+		if (type == mem_prom_used) {
+			memblock_reserve(base, size);
 			if (nr_prom_mem >= 5) {
 				pr_err("Too many ROM DATA regions");
 				continue;
diff --git a/arch/mips/fw/sni/sniprom.c b/arch/mips/fw/sni/sniprom.c
index 80112f2298b6..8f6730376a42 100644
--- a/arch/mips/fw/sni/sniprom.c
+++ b/arch/mips/fw/sni/sniprom.c
@@ -11,6 +11,7 @@
 
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/memblock.h>
 #include <linux/string.h>
 #include <linux/console.h>
 
@@ -131,8 +132,7 @@ static void __init sni_mem_init(void)
 		}
 		pr_debug("Bank%d: %08x @ %08x\n", i,
 			memconf[i].size, memconf[i].base);
-		add_memory_region(memconf[i].base, memconf[i].size,
-				  BOOT_MEM_RAM);
+		memblock_add(memconf[i].base, memconf[i].size);
 	}
 }
 
diff --git a/arch/mips/include/asm/bootinfo.h b/arch/mips/include/asm/bootinfo.h
index 6dd173a22aeb..aa03b1237155 100644
--- a/arch/mips/include/asm/bootinfo.h
+++ b/arch/mips/include/asm/bootinfo.h
@@ -90,13 +90,6 @@ const char *get_system_type(void);
 
 extern unsigned long mips_machtype;
 
-#define BOOT_MEM_RAM		1
-#define BOOT_MEM_ROM_DATA	2
-#define BOOT_MEM_RESERVED	3
-#define BOOT_MEM_INIT_RAM	4
-#define BOOT_MEM_NOMAP		5
-
-extern void add_memory_region(phys_addr_t start, phys_addr_t size, long type);
 extern void detect_memory_region(phys_addr_t start, phys_addr_t sz_min,  phys_addr_t sz_max);
 
 extern void prom_init(void);
diff --git a/arch/mips/include/asm/netlogic/psb-bootinfo.h b/arch/mips/include/asm/netlogic/psb-bootinfo.h
index 272544b55ceb..c716e9397113 100644
--- a/arch/mips/include/asm/netlogic/psb-bootinfo.h
+++ b/arch/mips/include/asm/netlogic/psb-bootinfo.h
@@ -87,6 +87,7 @@ struct nlm_boot_mem_map {
 		uint32_t type;		/* type of memory segment */
 	} map[NLM_BOOT_MEM_MAP_MAX];
 };
+#define NLM_BOOT_MEM_RAM	1
 
 /* Pointer to saved boot loader info */
 extern struct psb_info nlm_prom_info;
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;
 }
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);
 	} 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));
 
 #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);
 #endif /* !CONFIG_64BIT */
 }
 
diff --git a/arch/mips/loongson32/common/prom.c b/arch/mips/loongson32/common/prom.c
index fd76114fa3b0..c133b5adf34e 100644
--- a/arch/mips/loongson32/common/prom.c
+++ b/arch/mips/loongson32/common/prom.c
@@ -7,8 +7,8 @@
 
 #include <linux/io.h>
 #include <linux/init.h>
+#include <linux/memblock.h>
 #include <linux/serial_reg.h>
-#include <asm/bootinfo.h>
 #include <asm/fw/fw.h>
 
 #include <loongson1.h>
@@ -42,5 +42,5 @@ void __init prom_free_prom_memory(void)
 
 void __init plat_mem_setup(void)
 {
-	add_memory_region(0x0, (memsize << 20), BOOT_MEM_RAM);
+	memblock_add(0x0, (memsize << 20));
 }
diff --git a/arch/mips/netlogic/xlp/setup.c b/arch/mips/netlogic/xlp/setup.c
index 1a0fc5b62ba4..230adaf93e11 100644
--- a/arch/mips/netlogic/xlp/setup.c
+++ b/arch/mips/netlogic/xlp/setup.c
@@ -89,7 +89,7 @@ static void __init xlp_init_mem_from_bars(void)
 		if (map[i] > 0x10000000 && map[i] < 0x20000000)
 			map[i] = 0x20000000;
 
-		add_memory_region(map[i], map[i+1] - map[i], BOOT_MEM_RAM);
+		memblock_add(map[i], map[i+1] - map[i]);
 	}
 }
 
diff --git a/arch/mips/netlogic/xlr/setup.c b/arch/mips/netlogic/xlr/setup.c
index 72ceddc9a03f..627e88101316 100644
--- a/arch/mips/netlogic/xlr/setup.c
+++ b/arch/mips/netlogic/xlr/setup.c
@@ -34,6 +34,7 @@
 
 #include <linux/kernel.h>
 #include <linux/serial_8250.h>
+#include <linux/memblock.h>
 #include <linux/pm.h>
 
 #include <asm/idle.h>
@@ -149,7 +150,7 @@ static void prom_add_memory(void)
 
 	bootm = (void *)(long)nlm_prom_info.psb_mem_map;
 	for (i = 0; i < bootm->nr_map; i++) {
-		if (bootm->map[i].type != BOOT_MEM_RAM)
+		if (bootm->map[i].type != NLM_BOOT_MEM_RAM)
 			continue;
 		start = bootm->map[i].addr;
 		size   = bootm->map[i].size;
@@ -158,7 +159,7 @@ static void prom_add_memory(void)
 		if (i == 0 && start == 0 && size == 0x0c000000)
 			size = 0x0ff00000;
 
-		add_memory_region(start, size - pref_backup, BOOT_MEM_RAM);
+		memblock_add(start, size - pref_backup);
 	}
 }
 
diff --git a/arch/mips/ralink/of.c b/arch/mips/ralink/of.c
index 90c6d4a11c5d..cbae9d23ab7f 100644
--- a/arch/mips/ralink/of.c
+++ b/arch/mips/ralink/of.c
@@ -84,8 +84,7 @@ void __init plat_mem_setup(void)
 	if (memory_dtb)
 		of_scan_flat_dt(early_init_dt_scan_memory, NULL);
 	else if (soc_info.mem_size)
-		add_memory_region(soc_info.mem_base, soc_info.mem_size * SZ_1M,
-				  BOOT_MEM_RAM);
+		memblock_add(soc_info.mem_base, soc_info.mem_size * SZ_1M);
 	else
 		detect_memory_region(soc_info.mem_base,
 				     soc_info.mem_size_min * SZ_1M,
diff --git a/arch/mips/rb532/prom.c b/arch/mips/rb532/prom.c
index 303cc3dc1749..a9d1f2019dc3 100644
--- a/arch/mips/rb532/prom.c
+++ b/arch/mips/rb532/prom.c
@@ -126,5 +126,5 @@ void __init prom_init(void)
 
 	/* give all RAM to boot allocator,
 	 * except for the first 0x400 and the last 0x200 bytes */
-	add_memory_region(ddrbase + 0x400, memsize - 0x600, BOOT_MEM_RAM);
+	memblock_add(ddrbase + 0x400, memsize - 0x600);
 }
diff --git a/arch/mips/sgi-ip32/ip32-memory.c b/arch/mips/sgi-ip32/ip32-memory.c
index 62b956cc2d1d..0f53fed39da6 100644
--- a/arch/mips/sgi-ip32/ip32-memory.c
+++ b/arch/mips/sgi-ip32/ip32-memory.c
@@ -9,6 +9,7 @@
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/memblock.h>
 #include <linux/mm.h>
 
 #include <asm/ip32/crime.h>
@@ -36,7 +37,7 @@ void __init prom_meminit(void)
 
 		printk("CRIME MC: bank %u base 0x%016Lx size %LuMiB\n",
 			bank, base, size >> 20);
-		add_memory_region(base, size, BOOT_MEM_RAM);
+		memblock_add(base, size);
 	}
 }
 
diff --git a/arch/mips/sibyte/common/cfe.c b/arch/mips/sibyte/common/cfe.c
index cbf5939ed53a..89f7fca45152 100644
--- a/arch/mips/sibyte/common/cfe.c
+++ b/arch/mips/sibyte/common/cfe.c
@@ -114,16 +114,14 @@ static __init void prom_meminit(void)
 			if (initrd_start) {
 				if ((initrd_pstart > addr) &&
 				    (initrd_pstart < (addr + size))) {
-					add_memory_region(addr,
-							  initrd_pstart - addr,
-							  BOOT_MEM_RAM);
+					memblock_add(addr,
+						     initrd_pstart - addr);
 					rd_flag = 1;
 				}
 				if ((initrd_pend > addr) &&
 				    (initrd_pend < (addr + size))) {
-					add_memory_region(initrd_pend,
-						(addr + size) - initrd_pend,
-						 BOOT_MEM_RAM);
+					memblock_add(initrd_pend,
+						(addr + size) - initrd_pend);
 					rd_flag = 1;
 				}
 			}
@@ -142,7 +140,7 @@ static __init void prom_meminit(void)
 				 */
 				if (size > 512)
 					size -= 512;
-				add_memory_region(addr, size, BOOT_MEM_RAM);
+				memblock_add(addr, size);
 			}
 			board_mem_region_addrs[board_mem_region_count] = addr;
 			board_mem_region_sizes[board_mem_region_count] = size;
@@ -158,8 +156,8 @@ static __init void prom_meminit(void)
 	}
 #ifdef CONFIG_BLK_DEV_INITRD
 	if (initrd_start) {
-		add_memory_region(initrd_pstart, initrd_pend - initrd_pstart,
-				  BOOT_MEM_RESERVED);
+		memblock_add(initrd_pstart, initrd_pend - initrd_pstart);
+		memblock_reserve(initrd_pstart, initrd_pend - initrd_pstart);
 	}
 #endif
 }
diff --git a/arch/mips/txx9/jmr3927/prom.c b/arch/mips/txx9/jmr3927/prom.c
index 68a96473c134..53c68de54d30 100644
--- a/arch/mips/txx9/jmr3927/prom.c
+++ b/arch/mips/txx9/jmr3927/prom.c
@@ -37,7 +37,7 @@
  */
 #include <linux/init.h>
 #include <linux/kernel.h>
-#include <asm/bootinfo.h>
+#include <linux/memblock.h>
 #include <asm/txx9/generic.h>
 #include <asm/txx9/jmr3927.h>
 
@@ -47,6 +47,6 @@ void __init jmr3927_prom_init(void)
 	if ((tx3927_ccfgptr->ccfg & TX3927_CCFG_TLBOFF) == 0)
 		pr_err("TX3927 TLB off\n");
 
-	add_memory_region(0, JMR3927_SDRAM_SIZE, BOOT_MEM_RAM);
+	memblock_add(0, JMR3927_SDRAM_SIZE);
 	txx9_sio_putchar_init(TX3927_SIO_REG(1));
 }
diff --git a/arch/mips/txx9/rbtx4927/prom.c b/arch/mips/txx9/rbtx4927/prom.c
index fe6d0b54763f..9b4acff826eb 100644
--- a/arch/mips/txx9/rbtx4927/prom.c
+++ b/arch/mips/txx9/rbtx4927/prom.c
@@ -29,13 +29,14 @@
  *  with this program; if not, write to the Free Software Foundation, Inc.,
  *  675 Mass Ave, Cambridge, MA 02139, USA.
  */
+
 #include <linux/init.h>
-#include <asm/bootinfo.h>
+#include <linux/memblock.h>
 #include <asm/txx9/generic.h>
 #include <asm/txx9/rbtx4927.h>
 
 void __init rbtx4927_prom_init(void)
 {
-	add_memory_region(0, tx4927_get_mem_size(), BOOT_MEM_RAM);
+	memblock_add(0, tx4927_get_mem_size());
 	txx9_sio_putchar_init(TX4927_SIO_REG(0) & 0xfffffffffULL);
 }
diff --git a/arch/mips/txx9/rbtx4938/prom.c b/arch/mips/txx9/rbtx4938/prom.c
index 2b36a2ee744c..0de84716a428 100644
--- a/arch/mips/txx9/rbtx4938/prom.c
+++ b/arch/mips/txx9/rbtx4938/prom.c
@@ -12,12 +12,11 @@
 
 #include <linux/init.h>
 #include <linux/memblock.h>
-#include <asm/bootinfo.h>
 #include <asm/txx9/generic.h>
 #include <asm/txx9/rbtx4938.h>
 
 void __init rbtx4938_prom_init(void)
 {
-	add_memory_region(0, tx4938_get_mem_size(), BOOT_MEM_RAM);
+	memblock_add(0, tx4938_get_mem_size());
 	txx9_sio_putchar_init(TX4938_SIO_REG(0) & 0xfffffffffULL);
 }
diff --git a/arch/mips/txx9/rbtx4939/prom.c b/arch/mips/txx9/rbtx4939/prom.c
index 1dc47ce81c92..ba25ba1bd2ec 100644
--- a/arch/mips/txx9/rbtx4939/prom.c
+++ b/arch/mips/txx9/rbtx4939/prom.c
@@ -7,7 +7,7 @@
  */
 
 #include <linux/init.h>
-#include <asm/bootinfo.h>
+#include <linux/memblock.h>
 #include <asm/txx9/generic.h>
 #include <asm/txx9/rbtx4939.h>
 
@@ -23,7 +23,7 @@ void __init rbtx4939_prom_init(void)
 		win = ____raw_readq(&tx4939_ddrcptr->win[i]);
 		start = (unsigned long)(win >> 48);
 		size = (((unsigned long)(win >> 32) & 0xffff) + 1) - start;
-		add_memory_region(start << 20, size << 20, BOOT_MEM_RAM);
+		memblock_add(start << 20, size << 20);
 	}
 	txx9_sio_putchar_init(TX4939_SIO_REG(0) & 0xfffffffffULL);
 }
-- 
2.16.4


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08  8:43 [PATCH v2] MIPS: replace add_memory_region with memblock Thomas Bogendoerfer
@ 2020-10-08 15:20 ` Serge Semin
  2020-10-08 15:30   ` Maciej W. Rozycki
                     ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Serge Semin @ 2020-10-08 15:20 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Hauke Mehrtens, Rafał Miłecki, Florian Fainelli,
	bcm-kernel-feedback-list, Maciej W. Rozycki, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

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 <tsbogend@alpha.franken.de>
> ---
> 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 */
>  }
>  

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08 15:20 ` Serge Semin
@ 2020-10-08 15:30   ` Maciej W. Rozycki
  2020-10-08 15:54     ` Serge Semin
  2020-10-09  3:00   ` Jiaxun Yang
  2020-10-09 12:07   ` Thomas Bogendoerfer
  2 siblings, 1 reply; 13+ messages in thread
From: Maciej W. Rozycki @ 2020-10-08 15:30 UTC (permalink / raw)
  To: Serge Semin
  Cc: Thomas Bogendoerfer, Hauke Mehrtens, Rafał Miłecki,
	Florian Fainelli, bcm-kernel-feedback-list, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Thu, 8 Oct 2020, Serge Semin wrote:

> 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?

 At the very least it serves informational purposes as it shows up in 
/proc/iomem.

  Maciej

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08 15:30   ` Maciej W. Rozycki
@ 2020-10-08 15:54     ` Serge Semin
  2020-10-08 16:49       ` Florian Fainelli
  2020-10-08 16:56       ` Maciej W. Rozycki
  0 siblings, 2 replies; 13+ messages in thread
From: Serge Semin @ 2020-10-08 15:54 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Thomas Bogendoerfer, Hauke Mehrtens, Rafał Miłecki,
	Florian Fainelli, bcm-kernel-feedback-list, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Thu, Oct 08, 2020 at 04:30:35PM +0100, Maciej W. Rozycki wrote:
> On Thu, 8 Oct 2020, Serge Semin wrote:
> 
> > 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?
> 

>  At the very least it serves informational purposes as it shows up in 
> /proc/iomem.

I thought about that, but /proc/iomem prints the System RAM up. Adding the reserved
memory regions to be just memory region first still seem redundant, since
reserving a non-reflected in memory region most likely indicates an erroneous
dts. I failed to find that, but do the kernel or DTC make sure that the reserved
memory regions has actual memory behind? (At least in the framework of the
memblock.memory vs memblock.reserved arrays or in the DT source file)

I also don't see the other platforms doing that, since the MIPS arch only
redefines these methods. So if a problem of adding a reserved memory with
possible no real memory behind exist, it should be fixed in the cross-platform
basis, don't you think?

-Sergey

> 
>   Maciej

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08 15:54     ` Serge Semin
@ 2020-10-08 16:49       ` Florian Fainelli
  2020-10-08 21:20         ` Serge Semin
  2020-10-08 16:56       ` Maciej W. Rozycki
  1 sibling, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2020-10-08 16:49 UTC (permalink / raw)
  To: Serge Semin, Maciej W. Rozycki
  Cc: Thomas Bogendoerfer, Hauke Mehrtens, Rafał Miłecki,
	Florian Fainelli, bcm-kernel-feedback-list, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel



On 10/8/2020 8:54 AM, Serge Semin wrote:
> On Thu, Oct 08, 2020 at 04:30:35PM +0100, Maciej W. Rozycki wrote:
>> On Thu, 8 Oct 2020, Serge Semin wrote:
>>
>>> 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?
>>
> 
>>   At the very least it serves informational purposes as it shows up in
>> /proc/iomem.
> 
> I thought about that, but /proc/iomem prints the System RAM up. Adding the reserved
> memory regions to be just memory region first still seem redundant, since
> reserving a non-reflected in memory region most likely indicates an erroneous
> dts. I failed to find that, but do the kernel or DTC make sure that the reserved
> memory regions has actual memory behind? (At least in the framework of the
> memblock.memory vs memblock.reserved arrays or in the DT source file)

AFAICT DTC does not do any validation that regions you declare in 
/memreserve or /reserved-memory are within the 'reg' property defined 
for the /memory node. Not that it could not but that goes a little 
beyond is compiler job.

The kernel ought to be able to do that validation through memblock but 
there could be valid use cases behind declaring a reserved memory region 
that is not backed by a corresponding DRAM region. For instance if you 
hotplugged memory through the sysfs probe interface, and that memory was 
not initially declared in the Device Tree, but there were reserved 
regions within that hot-plugged range that you would have to be aware 
of, then this would break.

> 
> I also don't see the other platforms doing that, since the MIPS arch only
> redefines these methods. So if a problem of adding a reserved memory with
> possible no real memory behind exist, it should be fixed in the cross-platform
> basis, don't you think?

Would we be breaking any use case if we stopped allowing reserved region 
that are not part of DRAM being declared?
-- 
Florian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08 15:54     ` Serge Semin
  2020-10-08 16:49       ` Florian Fainelli
@ 2020-10-08 16:56       ` Maciej W. Rozycki
  2020-10-08 22:51         ` Serge Semin
  1 sibling, 1 reply; 13+ messages in thread
From: Maciej W. Rozycki @ 2020-10-08 16:56 UTC (permalink / raw)
  To: Serge Semin
  Cc: Thomas Bogendoerfer, Hauke Mehrtens, Rafał Miłecki,
	Florian Fainelli, bcm-kernel-feedback-list, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Thu, 8 Oct 2020, Serge Semin wrote:

> > > 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?
> > 
> 
> >  At the very least it serves informational purposes as it shows up in 
> > /proc/iomem.
> 
> I thought about that, but /proc/iomem prints the System RAM up. Adding the reserved
> memory regions to be just memory region first still seem redundant, since
> reserving a non-reflected in memory region most likely indicates an erroneous
> dts. I failed to find that, but do the kernel or DTC make sure that the reserved
> memory regions has actual memory behind? (At least in the framework of the
> memblock.memory vs memblock.reserved arrays or in the DT source file)

 Reserved regions need not be RAM or a memory-like device.  They could be 
floating bus even.  Here's an example from my x86 laptop where all kinds 
of stuff is marked reserved:

00000000-00000fff : Reserved
00001000-0009cfff : System RAM
0009d000-0009ffff : Reserved
000a0000-000bffff : PCI Bus 0000:00
000c0000-000cebff : Video ROM
000d0000-000d3fff : pnp 00:0b
000d8000-000dbfff : pnp 00:0b
000e0000-000fffff : Reserved
  000f0000-000fffff : System ROM
00100000-5f24afff : System RAM
5f24b000-5f24cfff : Reserved
5f24d000-5fee5fff : System RAM
5fee6000-5fee6fff : Reserved
5fee7000-6daecfff : System RAM
6daed000-729bafff : Reserved
729bb000-729bbfff : ACPI Non-volatile Storage
729bc000-7fefefff : Reserved
7feff000-7ff99fff : ACPI Non-volatile Storage
7ff9a000-7fffefff : ACPI Tables
7ffff000-87ffffff : Reserved
  80200000-85f7ffff : INT0E0C:00
88600000-89ffffff : Reserved
8a000000-efffffff : PCI Bus 0000:00
  c0000000-d1ffffff : PCI Bus 0000:01
    c0000000-cfffffff : 0000:01:00.0
    d0000000-d1ffffff : 0000:01:00.0
  d4000000-ea0fffff : PCI Bus 0000:05
  eb000000-ec0fffff : PCI Bus 0000:01
    eb000000-ebffffff : 0000:01:00.0
  ec100000-ecafffff : PCI Bus 0000:03
  ecb00000-ecbfffff : PCI Bus 0000:3f
    ecb00000-ecb00fff : 0000:3f:00.0
      ecb00000-ecb00fff : rtsx_pci
  ecc00000-eccfffff : PCI Bus 0000:3e
    ecc00000-ecc03fff : 0000:3e:00.0
      ecc00000-ecc03fff : nvme
  ecd00000-ecdfffff : PCI Bus 0000:04
    ecd00000-ecd01fff : 0000:04:00.0
      ecd00000-ecd01fff : iwlwifi
  ece00000-ed7fffff : PCI Bus 0000:03
  ed800000-ed8fffff : PCI Bus 0000:02
    ed800000-ed803fff : 0000:02:00.0
      ed800000-ed803fff : nvme
  ed900000-ed91ffff : 0000:00:1f.6
    ed900000-ed91ffff : e1000e
  ed920000-ed92ffff : 0000:00:14.0
    ed920000-ed92ffff : xhci-hcd
  ed930000-ed933fff : 0000:00:1f.2
  effe0000-efffffff : pnp 00:08
    effe0000-efffffff : pnp 00:0a
f0000000-f7ffffff : PCI MMCONFIG 0000 [bus 00-7f]
  f0000000-f7ffffff : Reserved
    f0000000-f7ffffff : pnp 00:08
      f0000000-f7ffffff : pnp 00:0a
fd000000-fe7fffff : Reserved
  fd000000-fe7fffff : PCI Bus 0000:00
    fd000000-fdabffff : pnp 00:00
    fdac0000-fdacffff : INT345D:00
      fdac0000-fdacffff : INT345D:00
    fdad0000-fdadffff : pnp 00:00
    fdae0000-fdaeffff : INT345D:00
      fdae0000-fdaeffff : INT345D:00
    fdaf0000-fdafffff : INT345D:00
      fdaf0000-fdafffff : INT345D:00
    fdb00000-fdffffff : pnp 00:00
      fdc6000c-fdc6000f : iTCO_wdt
        fdc6000c-fdc6000f : iTCO_wdt
    fe000000-fe01ffff : pnp 00:00
    fe036000-fe03bfff : pnp 00:00
    fe03d000-fe3fffff : pnp 00:00
    fe410000-fe7fffff : pnp 00:00
feb00000-febfffff : pnp 00:08
fec00000-fec00fff : Reserved
  fec00000-fec003ff : IOAPIC 0
fed00000-fed00fff : Reserved
  fed00000-fed003ff : HPET 0
    fed00000-fed003ff : PNP0103:00
fed10000-fed19fff : Reserved
  fed10000-fed13fff : pnp 00:08
  fed18000-fed18fff : pnp 00:08
    fed18000-fed18fff : pnp 00:0a
  fed19000-fed19fff : pnp 00:08
    fed19000-fed19fff : pnp 00:0a
fed20000-fed3ffff : pnp 00:08
  fed20000-fed3ffff : pnp 00:0a
fed84000-fed84fff : Reserved
fed90000-fed93fff : pnp 00:08
  fed90000-fed93fff : pnp 00:0a
fee00000-fee00fff : Local APIC
  fee00000-fee00fff : Reserved
ff000000-ffffffff : INT0800:00
  ff800000-ffffffff : Reserved
100000000-873ffffff : System RAM
  4bb200000-4bbc031d0 : Kernel code
  4bbc031d1-4bc334e7f : Kernel data
  4bc7b7000-4bc9fffff : Kernel bss
2000000000-2fffffffff : PCI Bus 0000:00
  2fd0000000-2ff1ffffff : PCI Bus 0000:05
  2ff2000000-2ff200ffff : 0000:00:1f.3
    2ff2000000-2ff200ffff : ICH HD audio
  2ff2010000-2ff2013fff : 0000:00:1f.3
    2ff2010000-2ff2013fff : ICH HD audio
  2ff2014000-2ff20140ff : 0000:00:1f.4
  2ff2015000-2ff2015fff : 0000:00:16.0
    2ff2015000-2ff2015fff : mei_me
  2ff2016000-2ff2016fff : 0000:00:15.0
    2ff2016000-2ff20161ff : lpss_dev
      2ff2016000-2ff20161ff : lpss_dev
    2ff2016200-2ff20162ff : lpss_priv
    2ff2016800-2ff2016fff : idma64.0
      2ff2016800-2ff2016fff : idma64.0
  2ff2017000-2ff2017fff : 0000:00:14.2
    2ff2017000-2ff2017fff : Intel PCH thermal driver
  2ff2018000-2ff2018fff : 0000:00:08.0

Actually another reason to mark regions reserved is to prevent them from 
being claimed by the wrong driver or, perhaps more importantly, used for 
assigning bus address ranges to hardware resources (BAR programming).

> I also don't see the other platforms doing that, since the MIPS arch only
> redefines these methods. So if a problem of adding a reserved memory with
> possible no real memory behind exist, it should be fixed in the cross-platform
> basis, don't you think?

 I think doing things in a generic way where possible is surely desired, 
however platforms have different ways to discover resources and I can't 
see offhand how this could be unified.  I haven't look at that code for a 
while now, so I can't be more specific offhand.

  Maciej

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08 16:49       ` Florian Fainelli
@ 2020-10-08 21:20         ` Serge Semin
  0 siblings, 0 replies; 13+ messages in thread
From: Serge Semin @ 2020-10-08 21:20 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Maciej W. Rozycki, Thomas Bogendoerfer, Hauke Mehrtens,
	Rafał Miłecki, bcm-kernel-feedback-list, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Thu, Oct 08, 2020 at 09:49:46AM -0700, Florian Fainelli wrote:
> 
> 
> On 10/8/2020 8:54 AM, Serge Semin wrote:
> > On Thu, Oct 08, 2020 at 04:30:35PM +0100, Maciej W. Rozycki wrote:
> > > On Thu, 8 Oct 2020, Serge Semin wrote:
> > > 
> > > > 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?
> > > 
> > 
> > >   At the very least it serves informational purposes as it shows up in
> > > /proc/iomem.
> > 
> > I thought about that, but /proc/iomem prints the System RAM up. Adding the reserved
> > memory regions to be just memory region first still seem redundant, since
> > reserving a non-reflected in memory region most likely indicates an erroneous
> > dts. I failed to find that, but do the kernel or DTC make sure that the reserved
> > memory regions has actual memory behind? (At least in the framework of the
> > memblock.memory vs memblock.reserved arrays or in the DT source file)
> 
> AFAICT DTC does not do any validation that regions you declare in
> /memreserve or /reserved-memory are within the 'reg' property defined for
> the /memory node.

> Not that it could not but that goes a little beyond is
> compiler job.

You are probably right, but it does the #{address,size}-cells validations
against the subnode' "reg" properties. It even does I2C controllers subnodes
"reg" validation so they wouldn't be greater than 7 or 10 bits wide. It also
does some magic checks of SPI controllers subnodes. See scripts/dtc/checks.c for
details. So I thought DTC might do some memory/reserved-memory validations too.
Apparently it doesn't. On the other hand it would probably be pointless, since a
system bootloader may change the "memory" node' "reg" value anyway if a platform
has a variable memory layout. So any validation being passed at the DTS compile
time wouldn't surely mean the memory/reserved-memory nodes coherency at the
system boot time.

> 
> The kernel ought to be able to do that validation through memblock but there
> could be valid use cases behind declaring a reserved memory region that is
> not backed by a corresponding DRAM region. For instance if you hotplugged
> memory through the sysfs probe interface, and that memory was not initially
> declared in the Device Tree, but there were reserved regions within that
> hot-plugged range that you would have to be aware of, then this would break.

Yeah, it seems to me all hot-pluggable regions are also marked as reserved. So
hot-plugging a memory device behind the manually reserved regions (reserved by
means of the DT reserved-memory/memreserve nodes) will unreserve them, which
most likely isn't what has been originally wanted.

Alas I don't have any hardware with hot-pluggable memory device to check out the
problem availability.

> 
> > 
> > I also don't see the other platforms doing that, since the MIPS arch only
> > redefines these methods. So if a problem of adding a reserved memory with
> > possible no real memory behind exist, it should be fixed in the cross-platform
> > basis, don't you think?
> 

> Would we be breaking any use case if we stopped allowing reserved region
> that are not part of DRAM being declared?

Hm, good question. I don't really know. But AFAICS from the reserved-memory node
DT bindings the reserved regions can be used to declare both normal RAM and some
vendor-specific regions. The later one theoretically can be some resource, bus or
memory-mapped device thing especially if it's marked with "no-map" boolean
property.

---

Getting back to the topic. In the MIPS-specific early_init_dt_reserve_memory_arch()
method (which is called for every region declared in reserved-memory/memreserve nodes)
we currently consider the reserved region as DRAM if 'no-map' property isn't specified.
The main question here is whether it is correct...

-Sergey

> -- 
> Florian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08 16:56       ` Maciej W. Rozycki
@ 2020-10-08 22:51         ` Serge Semin
  0 siblings, 0 replies; 13+ messages in thread
From: Serge Semin @ 2020-10-08 22:51 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Thomas Bogendoerfer, Hauke Mehrtens, Rafał Miłecki,
	Florian Fainelli, bcm-kernel-feedback-list, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Thu, Oct 08, 2020 at 05:56:17PM +0100, Maciej W. Rozycki wrote:
> On Thu, 8 Oct 2020, Serge Semin wrote:
> 
> > > > 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?
> > > 
> > 
> > >  At the very least it serves informational purposes as it shows up in 
> > > /proc/iomem.
> > 
> > I thought about that, but /proc/iomem prints the System RAM up. Adding the reserved
> > memory regions to be just memory region first still seem redundant, since
> > reserving a non-reflected in memory region most likely indicates an erroneous
> > dts. I failed to find that, but do the kernel or DTC make sure that the reserved
> > memory regions has actual memory behind? (At least in the framework of the
> > memblock.memory vs memblock.reserved arrays or in the DT source file)
> 
>  Reserved regions need not be RAM or a memory-like device.  

Agree. My statement about considering as error the situation when the
reserved-memory is declared with no backed DRAM region was most likely
wrong. But in this case what we currently do and what Thomas suggest to keep
doing in the framework of the MIPS-specific early_init_dt_reserve_memory_arch()
is probably incorrect, since the method makes sure that any added reserved
memory region is actually backed with DRAM (called both memblock_add() and
memblock_reserve() for such regions). None of the other platforms is noticed to
execute the same pattern.

> They could be 
> floating bus even.  Here's an example from my x86 laptop where all kinds 
> of stuff is marked reserved:
> 
> 00000000-00000fff : Reserved
> 00001000-0009cfff : System RAM
> 0009d000-0009ffff : Reserved

AFAICS from the ./arch/x86/kernel/e820.c code, the regions marked as "Reserved"
aren't mem-mapped by the kernel, since they are skipped in the method
e820__memblock_setup(), which is responsible for the x86-specific memory mapping
data conversion to the memblock. So the system doesn't consider them as a RAM
resource. Note each normal memory is supposed to be added to the memblock, so to
be then used by the buddy allocator for normal memory allocations.

Basically the x86-specific "Reserved" regions are similar to the regions added
by means of the "reserved-memory" DT sub-node with "no-map" property specified.
The problem in the topic is whether we must or must not make sure that each
reserved-memory region with no "no-map" property considered as normal memory
region backed with DRAM. The rest of the platforms currently just register the
"reserved-memory" regions in memblock.reserved with no checking whether they are
backed with corresponding DRAM region and with no adding them to the
memblock.memory pool. So most likely we ought to do the same in MIPS.

> ...
>
> 

> Actually another reason to mark regions reserved is to prevent them from 
> being claimed by the wrong driver or, perhaps more importantly, used for 
> assigning bus address ranges to hardware resources (BAR programming).

Right, AFAICS that's what the "reserved-memory" DT node has been introduced for.
By using it we can create a DMA/CMA memory pool fully dedicated for a particular
device, or just completely remove a memory region from the kernel virtual
mapping framework if instead of providing an access to the System RAM a region
is remapped to IO to/from some hardware resource.

-Sergey

> 
> > I also don't see the other platforms doing that, since the MIPS arch only
> > redefines these methods. So if a problem of adding a reserved memory with
> > possible no real memory behind exist, it should be fixed in the cross-platform
> > basis, don't you think?
> 
>  I think doing things in a generic way where possible is surely desired, 
> however platforms have different ways to discover resources and I can't 
> see offhand how this could be unified.  I haven't look at that code for a 
> while now, so I can't be more specific offhand.
> 
>   Maciej
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08 15:20 ` Serge Semin
  2020-10-08 15:30   ` Maciej W. Rozycki
@ 2020-10-09  3:00   ` Jiaxun Yang
  2020-10-09 12:09     ` Thomas Bogendoerfer
  2020-10-09 12:07   ` Thomas Bogendoerfer
  2 siblings, 1 reply; 13+ messages in thread
From: Jiaxun Yang @ 2020-10-09  3:00 UTC (permalink / raw)
  To: Serge Semin, Thomas Bogendoerfer
  Cc: Hauke Mehrtens, Rafał Miłecki, Florian Fainelli,
	bcm-kernel-feedback-list, Maciej W. Rozycki, Keguang Zhang,
	John Crispin, linux-mips, linux-kernel, linux-arm-kernel



在 2020/10/8 23:20, Serge Semin 写道:

[...]
>
>> -		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?
Hi all,

I can confirm this reservation can be removed.
It was used to ensure HIGHMEM won't overlap PCI MMIO region when
firmware was unreliable.
But I do think we shouldn't care this case as an unreliable firmware 
shouldn't
ship with any devices for end users.

Also it won't affect display of /proc/iomem, now we're on age of 
DeviceTree :-)

Thanks.

- Jiaxun

>
> -Sergey
>
>>   #endif /* !CONFIG_64BIT */
>>   }
>>   

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-08 15:20 ` Serge Semin
  2020-10-08 15:30   ` Maciej W. Rozycki
  2020-10-09  3:00   ` Jiaxun Yang
@ 2020-10-09 12:07   ` Thomas Bogendoerfer
  2020-10-09 14:15     ` Serge Semin
  2 siblings, 1 reply; 13+ messages in thread
From: Thomas Bogendoerfer @ 2020-10-09 12:07 UTC (permalink / raw)
  To: Serge Semin
  Cc: Hauke Mehrtens, Rafał Miłecki, Florian Fainelli,
	bcm-kernel-feedback-list, Maciej W. Rozycki, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Thu, Oct 08, 2020 at 06:20:06PM +0300, Serge Semin wrote:
> > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > ---
> > 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 .

perfect, I'll remove it in my next version.

> > @@ -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?

I'm not sure about that, add_memory_region() did a memblock_add
and then memblock_reserve for BOOT_MEM_RESERVED, that's why I changed
it that way.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-09  3:00   ` Jiaxun Yang
@ 2020-10-09 12:09     ` Thomas Bogendoerfer
  0 siblings, 0 replies; 13+ messages in thread
From: Thomas Bogendoerfer @ 2020-10-09 12:09 UTC (permalink / raw)
  To: Jiaxun Yang
  Cc: Serge Semin, Hauke Mehrtens, Rafał Miłecki,
	Florian Fainelli, bcm-kernel-feedback-list, Maciej W. Rozycki,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Fri, Oct 09, 2020 at 11:00:36AM +0800, Jiaxun Yang wrote:
> 
> 
> 在 2020/10/8 23:20, Serge Semin 写道:
> 
> [...]
> >
> >>-		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?
> Hi all,
> 
> I can confirm this reservation can be removed.

thank you for confirming.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-09 12:07   ` Thomas Bogendoerfer
@ 2020-10-09 14:15     ` Serge Semin
  2020-10-12 10:01       ` Thomas Bogendoerfer
  0 siblings, 1 reply; 13+ messages in thread
From: Serge Semin @ 2020-10-09 14:15 UTC (permalink / raw)
  To: Thomas Bogendoerfer
  Cc: Hauke Mehrtens, Rafał Miłecki, Florian Fainelli,
	bcm-kernel-feedback-list, Maciej W. Rozycki, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Fri, Oct 09, 2020 at 02:07:52PM +0200, Thomas Bogendoerfer wrote:
> On Thu, Oct 08, 2020 at 06:20:06PM +0300, Serge Semin wrote:
> > > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > > ---
> > > 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 .
> 
> perfect, I'll remove it in my next version.
> 
> > > @@ -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?
> 

> I'm not sure about that, add_memory_region() did a memblock_add
> and then memblock_reserve for BOOT_MEM_RESERVED, that's why I changed
> it that way.

The main question here whether we need to preserve the MIPS-specific semantics
of the kernel 'memmap' parameter. Currently the memmap parameter passed with
'$' specifier will be perceived as a reserved RAM region, while, for instance, 
the same parameter on x86 will be converted to a region, which won't be
registered in memblock at all, so the system won't be able to reuse it if it's
needed to be (see parse_memmap_one() and e820__memblock_setup() for details).

I don't really know what approach is correct... 
Documentation/admin-guide/kernel-parameters.txt isn't certain about that. It
says that the region must be reserved, but no words whether it is supposed to be
mappable or non-mappable.

-Sergey

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] MIPS: replace add_memory_region with memblock
  2020-10-09 14:15     ` Serge Semin
@ 2020-10-12 10:01       ` Thomas Bogendoerfer
  0 siblings, 0 replies; 13+ messages in thread
From: Thomas Bogendoerfer @ 2020-10-12 10:01 UTC (permalink / raw)
  To: Serge Semin
  Cc: Hauke Mehrtens, Rafał Miłecki, Florian Fainelli,
	bcm-kernel-feedback-list, Maciej W. Rozycki, Jiaxun Yang,
	Keguang Zhang, John Crispin, linux-mips, linux-kernel,
	linux-arm-kernel

On Fri, Oct 09, 2020 at 05:15:37PM +0300, Serge Semin wrote:
> On Fri, Oct 09, 2020 at 02:07:52PM +0200, Thomas Bogendoerfer wrote:
> > On Thu, Oct 08, 2020 at 06:20:06PM +0300, Serge Semin wrote:
> > > > @@ -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?
> > 
> 
> > I'm not sure about that, add_memory_region() did a memblock_add
> > and then memblock_reserve for BOOT_MEM_RESERVED, that's why I changed
> > it that way.
> 
> The main question here whether we need to preserve the MIPS-specific semantics
> of the kernel 'memmap' parameter. Currently the memmap parameter passed with
> '$' specifier will be perceived as a reserved RAM region, while, for instance, 
> the same parameter on x86 will be converted to a region, which won't be
> registered in memblock at all, so the system won't be able to reuse it if it's
> needed to be (see parse_memmap_one() and e820__memblock_setup() for details).
> 
> I don't really know what approach is correct... 
> Documentation/admin-guide/kernel-parameters.txt isn't certain about that. It
> says that the region must be reserved, but no words whether it is supposed to be
> mappable or non-mappable.

I leave it as in v3 of the patch for now. If we come to the point what
the correct semantic should be, we can change it.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-08  8:43 [PATCH v2] MIPS: replace add_memory_region with memblock Thomas Bogendoerfer
2020-10-08 15:20 ` Serge Semin
2020-10-08 15:30   ` Maciej W. Rozycki
2020-10-08 15:54     ` Serge Semin
2020-10-08 16:49       ` Florian Fainelli
2020-10-08 21:20         ` Serge Semin
2020-10-08 16:56       ` Maciej W. Rozycki
2020-10-08 22:51         ` Serge Semin
2020-10-09  3:00   ` Jiaxun Yang
2020-10-09 12:09     ` Thomas Bogendoerfer
2020-10-09 12:07   ` Thomas Bogendoerfer
2020-10-09 14:15     ` Serge Semin
2020-10-12 10:01       ` Thomas Bogendoerfer

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git