mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/6] memory: fuse overlapping memory banks
@ 2021-05-31  7:12 Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 1/6] common: memory: allocate all memory devices at once Ahmad Fatoum
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-05-31  7:12 UTC (permalink / raw)
  To: barebox; +Cc: mol

The request region code makes sense to ensure drivers don't try to claim
(I/O) memory region claimed by another driver or by the memory
allocator. It doesn't make as much sense for registering available
memory banks, because there is often overlap:

 - device tree often defines the (minimally) available /memory across
   variants
 - subarchitectures like i.MX, STM32MP, OMAP or SAMA5 query the main
   RAM size from controller registers

So far, overlap went mostly unnoticed, because the RAM controller
drivers didn't check for errors. However, if barebox is already running
from RAM outside that described by the device tree, there will be errors
and /delete-node/ annotations that need to be added into device trees.

Upstream device tree can be extended later on with a /memory node, which
in turn will break barebox. This series fixes all of that. Overlapping
memory banks are combined into one. Errors are propagated everywhere.
/delete-node/ memory@X shouldn't be needed for new boards.

Ahmad Fatoum (6):
  common: memory: allocate all memory devices at once
  memory: fuse overlapping memory banks
  of: propagate errors inside barebox_register_{of,fdt} into initcalls
  of: warn about of_add_memory_bank errors
  ARM: <asm/memory.h>: propagate error codes from arm_add_mem_device()
  ARM: report probe error at arm_add_mem_device() callsites on failure

 arch/arm/boards/qemu-virt/board.c |  3 +-
 arch/arm/cpu/dtb.c                |  4 +-
 arch/arm/include/asm/memory.h     |  6 +--
 arch/arm/mach-at91/ddramc.c       |  4 +-
 arch/arm/mach-imx/esdctl.c        | 56 +++++++++++++--------------
 arch/arm/mach-omap/am33xx_scrm.c  |  4 +-
 arch/arm/mach-stm32mp/ddrctrl.c   |  4 +-
 arch/kvx/lib/dtb.c                |  4 +-
 arch/mips/boot/dtb.c              | 13 ++++---
 arch/openrisc/lib/dtb.c           |  4 +-
 arch/riscv/lib/dtb.c              |  5 ++-
 arch/sandbox/board/dtb.c          |  4 +-
 common/memory.c                   | 64 +++++++++++++++++++++++++++----
 common/resource.c                 | 22 +++++++++++
 drivers/of/base.c                 | 40 ++++++++++++-------
 drivers/of/mem_generic.c          |  5 ++-
 include/linux/ioport.h            | 20 ++++++++++
 include/memory.h                  |  1 -
 include/of.h                      |  6 +--
 19 files changed, 181 insertions(+), 88 deletions(-)

-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 1/6] common: memory: allocate all memory devices at once
  2021-05-31  7:12 [PATCH 0/6] memory: fuse overlapping memory banks Ahmad Fatoum
@ 2021-05-31  7:12 ` Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 2/6] memory: fuse overlapping memory banks Ahmad Fatoum
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-05-31  7:12 UTC (permalink / raw)
  To: barebox; +Cc: mol, Ahmad Fatoum

Follow-up commit will fuse overlapping RAM banks. As all memory is
supposed to be registered during mem_initcall or before, we can postpone
device creation to mmu_initcall, so we can directly allocate devices
spanning the correct region.

The mem driver and the devinfo command are the only consumers of these
devices, so it's ok to register the devices at mmu_initcall.

While at it, drop the struct memory_bank::dev member.
It's unused anywhere.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/memory.c  | 17 +++++++++++++----
 include/memory.h |  1 -
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/common/memory.c b/common/memory.c
index 392522bfc3db..612ed87168b5 100644
--- a/common/memory.c
+++ b/common/memory.c
@@ -115,15 +115,11 @@ int barebox_add_memory_bank(const char *name, resource_size_t start,
 				    resource_size_t size)
 {
 	struct memory_bank *bank = xzalloc(sizeof(*bank));
-	struct device_d *dev;
 
 	bank->res = request_iomem_region(name, start, start + size - 1);
 	if (IS_ERR(bank->res))
 		return PTR_ERR(bank->res);
 
-	dev = add_mem_device(name, start, size, IORESOURCE_MEM_WRITEABLE);
-
-	bank->dev = dev;
 	bank->start = start;
 	bank->size = size;
 
@@ -132,6 +128,19 @@ int barebox_add_memory_bank(const char *name, resource_size_t start,
 	return 0;
 }
 
+static int add_mem_devices(void)
+{
+	struct memory_bank *bank;
+
+	for_each_memory_bank(bank) {
+		add_mem_device(bank->res->name, bank->start, bank->size,
+			       IORESOURCE_MEM_WRITEABLE);
+	}
+
+	return 0;
+}
+mmu_initcall(add_mem_devices);
+
 /*
  * Request a region from the registered sdram
  */
diff --git a/include/memory.h b/include/memory.h
index 906d9f7b2689..c793bb51ed77 100644
--- a/include/memory.h
+++ b/include/memory.h
@@ -11,7 +11,6 @@ ulong mem_malloc_end(void);
 
 struct memory_bank {
 	struct list_head list;
-	struct device_d *dev;
 	unsigned long start;
 	unsigned long size;
 	struct resource *res;
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 2/6] memory: fuse overlapping memory banks
  2021-05-31  7:12 [PATCH 0/6] memory: fuse overlapping memory banks Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 1/6] common: memory: allocate all memory devices at once Ahmad Fatoum
@ 2021-05-31  7:12 ` Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 3/6] of: propagate errors inside barebox_register_{of, fdt} into initcalls Ahmad Fatoum
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-05-31  7:12 UTC (permalink / raw)
  To: barebox; +Cc: mol, Ahmad Fatoum

Some ARM subarchitectures read back RAM size from the SDRAM
controller and use the info to register memory banks.

If an overlapping memory region is already registered, e.g. via device
tree /memory, this second registration will fail.

This is especially annoying as it can regress after a device tree sync:

 - Kind soul updates upstream device tree to describe minimal available
   RAM across hardware variants
 - barebox PBL has enough info about the board to set up larger RAM size
   and relocates barebox to the end of the RAM
 - barebox proper starts with new device tree and is upset to
   find itself outside of registered memory

Account for this by growing the existing bank if a bank to be added
happens to overlap it. As a special case, if the existing bank
completely contains the new memory bank, the function is a no-op.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/memory.c        | 49 ++++++++++++++++++++++++++++++++++++++----
 common/resource.c      | 22 +++++++++++++++++++
 include/linux/ioport.h | 20 +++++++++++++++++
 3 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/common/memory.c b/common/memory.c
index 612ed87168b5..95995bb6e310 100644
--- a/common/memory.c
+++ b/common/memory.c
@@ -111,15 +111,56 @@ void *sbrk(ptrdiff_t increment)
 
 LIST_HEAD(memory_banks);
 
+static int barebox_grow_memory_bank(struct memory_bank *bank, const char *name,
+				    const struct resource *newres)
+{
+	struct resource *res;
+	resource_size_t bank_end = bank->res->end;
+
+	if (newres->start < bank->start) {
+		res = request_iomem_region(name, newres->start, bank->start - 1);
+		if (IS_ERR(res))
+			return PTR_ERR(res);
+		__merge_regions(name, bank->res, res);
+	}
+
+	if (bank_end < newres->end) {
+		res = request_iomem_region(name, bank_end + 1, newres->end);
+		if (IS_ERR(res))
+			return PTR_ERR(res);
+		__merge_regions(name, bank->res, res);
+	}
+
+	bank->start = newres->start;
+	bank->size = resource_size(bank->res);
+
+	return 0;
+}
+
 int barebox_add_memory_bank(const char *name, resource_size_t start,
 				    resource_size_t size)
 {
-	struct memory_bank *bank = xzalloc(sizeof(*bank));
+	struct memory_bank *bank;
+	struct resource *res;
+	struct resource newres = {
+		.start = start,
+		.end = start + size - 1,
+	};
+
+	for_each_memory_bank(bank) {
+		if (resource_contains(bank->res, &newres))
+			return 0;
+		if (resource_contains(&newres, bank->res))
+			return barebox_grow_memory_bank(bank, name, &newres);
+	}
+
+	res = request_iomem_region(name, start, start + size - 1);
+	if (IS_ERR(res))
+		return PTR_ERR(res);
 
-	bank->res = request_iomem_region(name, start, start + size - 1);
-	if (IS_ERR(bank->res))
-		return PTR_ERR(bank->res);
+	bank = xzalloc(sizeof(*bank));
 
+	bank->res = res;
 	bank->start = start;
 	bank->size = size;
 
diff --git a/common/resource.c b/common/resource.c
index ff4318a0d77f..f96cb94b5074 100644
--- a/common/resource.c
+++ b/common/resource.c
@@ -102,6 +102,28 @@ int release_region(struct resource *res)
 	return 0;
 }
 
+
+/*
+ * merge two adjacent sibling regions.
+ */
+int __merge_regions(const char *name,
+		struct resource *resa, struct resource *resb)
+{
+	if (!resource_adjacent(resa, resb))
+		return -EINVAL;
+
+	if (resa->start < resb->start)
+		resa->end = resb->end;
+	else
+		resa->start = resb->start;
+
+	free((char *)resa->name);
+	resa->name = xstrdup(name);
+	release_region(resb);
+
+	return 0;
+}
+
 /* The root resource for the whole memory-mapped io space */
 struct resource iomem_resource = {
 	.start = 0,
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 3d375a87400c..295ab4a49dff 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -130,6 +130,23 @@ static inline unsigned long resource_type(const struct resource *res)
 	return res->flags & IORESOURCE_TYPE_BITS;
 }
 
+/* True iff r1 completely contains r2 */
+static inline bool resource_contains(struct resource *r1, struct resource *r2)
+{
+	if (resource_type(r1) != resource_type(r2))
+		return false;
+	if (r1->flags & IORESOURCE_UNSET || r2->flags & IORESOURCE_UNSET)
+		return false;
+	return r1->start <= r2->start && r1->end >= r2->end;
+}
+
+/* True if r1 and r2 are adjacent to each other */
+static inline bool resource_adjacent(struct resource *r1, struct resource *r2)
+{
+	return (r1->end != ~(resource_size_t)0 && r1->end + 1 == r2->start) ||
+		(r2->end != ~(resource_size_t)0 && r2->end + 1 == r1->start);
+}
+
 struct resource *request_iomem_region(const char *name,
 		resource_size_t start, resource_size_t end);
 struct resource *request_ioport_region(const char *name,
@@ -139,6 +156,9 @@ struct resource *__request_region(struct resource *parent,
 		const char *name, resource_size_t end,
 		resource_size_t size);
 
+int __merge_regions(const char *name,
+		struct resource *resa, struct resource *resb);
+
 int release_region(struct resource *res);
 
 extern struct resource iomem_resource;
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 3/6] of: propagate errors inside barebox_register_{of, fdt} into initcalls
  2021-05-31  7:12 [PATCH 0/6] memory: fuse overlapping memory banks Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 1/6] common: memory: allocate all memory devices at once Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 2/6] memory: fuse overlapping memory banks Ahmad Fatoum
@ 2021-05-31  7:12 ` Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 4/6] of: warn about of_add_memory_bank errors Ahmad Fatoum
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-05-31  7:12 UTC (permalink / raw)
  To: barebox; +Cc: mol, Ahmad Fatoum

Errors during device tree registration, while uncommon, are really
annoying, because the system may limp along and it's not clear where
the misbehavior originates from.

Failing the initcall of the device tree would improve user experience in
that error case. There is intentionally no early exit on error cases
to give barebox a chance to probe the serial driver to actually report
errors when DEBUG_LL is disabled.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/boards/qemu-virt/board.c |  3 +--
 arch/arm/cpu/dtb.c                |  4 +---
 arch/kvx/lib/dtb.c                |  4 +---
 arch/mips/boot/dtb.c              |  4 +---
 arch/openrisc/lib/dtb.c           |  4 +---
 arch/riscv/lib/dtb.c              |  5 +++--
 arch/sandbox/board/dtb.c          |  4 +---
 drivers/of/base.c                 | 16 +++++++++-------
 include/of.h                      |  4 ++--
 9 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/arch/arm/boards/qemu-virt/board.c b/arch/arm/boards/qemu-virt/board.c
index d0a7e3da4ff3..5ce1ecfc2461 100644
--- a/arch/arm/boards/qemu-virt/board.c
+++ b/arch/arm/boards/qemu-virt/board.c
@@ -40,9 +40,8 @@ static int replace_dtb(void) {
 
 	overlay = of_unflatten_dtb(__dtb_overlay_of_flash_start);
 	of_overlay_apply_tree(root, overlay);
-	barebox_register_of(root);
 
-	return 0;
+	return barebox_register_of(root);
 }
 
 pure_initcall(replace_dtb);
diff --git a/arch/arm/cpu/dtb.c b/arch/arm/cpu/dtb.c
index 35f251d99a35..9aa979ca081c 100644
--- a/arch/arm/cpu/dtb.c
+++ b/arch/arm/cpu/dtb.c
@@ -26,8 +26,6 @@ static int of_arm_init(void)
 		return 0;
 	}
 
-	barebox_register_fdt(fdt);
-
-	return 0;
+	return barebox_register_fdt(fdt);
 }
 core_initcall(of_arm_init);
diff --git a/arch/kvx/lib/dtb.c b/arch/kvx/lib/dtb.c
index 09898017c9b5..3d65bd7bd45f 100644
--- a/arch/kvx/lib/dtb.c
+++ b/arch/kvx/lib/dtb.c
@@ -9,8 +9,6 @@
 
 static int of_kvx_init(void)
 {
-	barebox_register_fdt(boot_dtb);
-
-	return 0;
+	return barebox_register_fdt(boot_dtb);
 }
 core_initcall(of_kvx_init);
diff --git a/arch/mips/boot/dtb.c b/arch/mips/boot/dtb.c
index 6fce4700cc9e..dbb6315d1f05 100644
--- a/arch/mips/boot/dtb.c
+++ b/arch/mips/boot/dtb.c
@@ -41,8 +41,6 @@ static int of_mips_init(void)
 	if (!fdt)
 		fdt = __dtb_start;
 
-	barebox_register_fdt(fdt);
-
-	return 0;
+	return barebox_register_fdt(fdt);
 }
 core_initcall(of_mips_init);
diff --git a/arch/openrisc/lib/dtb.c b/arch/openrisc/lib/dtb.c
index 61cf35ddf362..0507eed1d75b 100644
--- a/arch/openrisc/lib/dtb.c
+++ b/arch/openrisc/lib/dtb.c
@@ -28,8 +28,6 @@ static int of_openrisc_init(void)
 	if (root)
 		return 0;
 
-	barebox_register_fdt(__dtb_start);
-
-	return 0;
+	return barebox_register_fdt(__dtb_start);
 }
 core_initcall(of_openrisc_init);
diff --git a/arch/riscv/lib/dtb.c b/arch/riscv/lib/dtb.c
index 8c2f5b251883..aa287953870a 100644
--- a/arch/riscv/lib/dtb.c
+++ b/arch/riscv/lib/dtb.c
@@ -9,6 +9,7 @@
 static int of_riscv_init(void)
 {
 	void *fdt;
+	int ret;
 
 	/* See if we are provided a dtb in boarddata */
 	fdt = barebox_riscv_boot_dtb();
@@ -20,11 +21,11 @@ static int of_riscv_init(void)
 	pr_debug("using boarddata provided DTB\n");
 
 
-	barebox_register_fdt(fdt);
+	ret = barebox_register_fdt(fdt);
 
 	/* do it now, before clocksource drivers run postcore */
 	timer_init();
 
-	return 0;
+	return ret;
 }
 core_initcall(of_riscv_init);
diff --git a/arch/sandbox/board/dtb.c b/arch/sandbox/board/dtb.c
index 4a8cbfb26f32..98d2e774b89a 100644
--- a/arch/sandbox/board/dtb.c
+++ b/arch/sandbox/board/dtb.c
@@ -39,8 +39,6 @@ static int of_sandbox_init(void)
 	if (!dtb)
 		dtb = __dtb_sandbox_start;
 
-	barebox_register_fdt(dtb);
-
-	return 0;
+	return barebox_register_fdt(dtb);
 }
 core_initcall(of_sandbox_init);
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6fe02649ee53..b99201a50fd5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1699,32 +1699,34 @@ int of_set_root_node(struct device_node *node)
 	return 0;
 }
 
-void barebox_register_of(struct device_node *root)
+int barebox_register_of(struct device_node *root)
 {
 	if (root_node)
-		return;
+		return -EBUSY;
 
 	of_set_root_node(root);
 	of_fix_tree(root);
 
 	if (IS_ENABLED(CONFIG_OFDEVICE))
-		of_probe();
+		return of_probe();
+
+	return 0;
 }
 
-void barebox_register_fdt(const void *dtb)
+int barebox_register_fdt(const void *dtb)
 {
 	struct device_node *root;
 
 	if (root_node)
-		return;
+		return -EBUSY;
 
 	root = of_unflatten_dtb(dtb);
 	if (IS_ERR(root)) {
 		pr_err("Cannot unflatten dtb: %pe\n", root);
-		return;
+		return PTR_ERR(root);
 	}
 
-	barebox_register_of(root);
+	return barebox_register_of(root);
 }
 
 /**
diff --git a/include/of.h b/include/of.h
index 645f429bdeed..66d1edcc5c0d 100644
--- a/include/of.h
+++ b/include/of.h
@@ -261,8 +261,8 @@ extern int of_modalias_node(struct device_node *node, char *modalias, int len);
 
 extern struct device_node *of_get_root_node(void);
 extern int of_set_root_node(struct device_node *node);
-extern void barebox_register_of(struct device_node *root);
-extern void barebox_register_fdt(const void *dtb);
+extern int barebox_register_of(struct device_node *root);
+extern int barebox_register_fdt(const void *dtb);
 
 extern struct device_d *of_platform_device_create(struct device_node *np,
 						struct device_d *parent);
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 4/6] of: warn about of_add_memory_bank errors
  2021-05-31  7:12 [PATCH 0/6] memory: fuse overlapping memory banks Ahmad Fatoum
                   ` (2 preceding siblings ...)
  2021-05-31  7:12 ` [PATCH 3/6] of: propagate errors inside barebox_register_{of, fdt} into initcalls Ahmad Fatoum
@ 2021-05-31  7:12 ` Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 5/6] ARM: <asm/memory.h>: propagate error codes from arm_add_mem_device() Ahmad Fatoum
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-05-31  7:12 UTC (permalink / raw)
  To: barebox; +Cc: mol, Ahmad Fatoum

Now that errors from of_probe are propagated to the respective initcalls
registering the device tree, propagate of_add_memory_bank errors as
well. This ensures that clashes of device-tree added regions with
previous ones don't go unnoticed. This can e.g. be the case if a device
tree happens to have both /memory@X { }; and /memory { }; nodes.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/mips/boot/dtb.c     |  9 ++++++---
 drivers/of/base.c        | 24 ++++++++++++++++++------
 drivers/of/mem_generic.c |  5 +++--
 include/of.h             |  2 +-
 4 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/arch/mips/boot/dtb.c b/arch/mips/boot/dtb.c
index dbb6315d1f05..ece1494e5fdf 100644
--- a/arch/mips/boot/dtb.c
+++ b/arch/mips/boot/dtb.c
@@ -14,21 +14,24 @@
 void *glob_fdt;
 u32 glob_fdt_size;
 
-void of_add_memory_bank(struct device_node *node, bool dump, int r,
+int of_add_memory_bank(struct device_node *node, bool dump, int r,
 		u64 base, u64 size)
 {
 	static char str[12];
+	int ret;
 
 	if (IS_ENABLED(CONFIG_MMU)) {
 		sprintf(str, "kseg0_ram%d", r);
-		barebox_add_memory_bank(str, CKSEG0 | base, size);
+		ret = barebox_add_memory_bank(str, CKSEG0 | base, size);
 	} else {
 		sprintf(str, "kseg1_ram%d", r);
-		barebox_add_memory_bank(str, CKSEG1 | base, size);
+		ret = barebox_add_memory_bank(str, CKSEG1 | base, size);
 	}
 
 	if (dump)
 		pr_info("%s: %s: 0x%llx@0x%llx\n", node->name, str, size, base);
+
+	return ret;
 }
 
 extern char __dtb_start[];
diff --git a/drivers/of/base.c b/drivers/of/base.c
index b99201a50fd5..17f58dba233e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2248,6 +2248,7 @@ int of_add_memory(struct device_node *node, bool dump)
 		return -ENXIO;
 
 	while (!of_address_to_resource(node, n, &res)) {
+		int err;
 		n++;
 		if (!resource_size(&res))
 			continue;
@@ -2255,12 +2256,15 @@ int of_add_memory(struct device_node *node, bool dump)
 		if (!of_device_is_available(node))
 			continue;
 
-		of_add_memory_bank(node, dump, mem_bank_num,
+		err = of_add_memory_bank(node, dump, mem_bank_num,
 				res.start, resource_size(&res));
+		if (err)
+			ret = err;
+
 		mem_bank_num++;
 	}
 
-	return 0;
+	return ret;
 }
 
 static struct device_node *of_chosen;
@@ -2283,18 +2287,25 @@ const struct of_device_id of_default_bus_match_table[] = {
 	}
 };
 
-static void of_probe_memory(void)
+static int of_probe_memory(void)
 {
 	struct device_node *memory = root_node;
+	int ret = 0;
 
 	/* Parse all available node with "memory" device_type */
 	while (1) {
+		int err;
+
 		memory = of_find_node_by_type(memory, "memory");
 		if (!memory)
 			break;
 
-		of_add_memory(memory, false);
+		err = of_add_memory(memory, false);
+		if (err)
+			ret = err;
 	}
+
+	return ret;
 }
 
 static void of_platform_device_create_root(struct device_node *np)
@@ -2315,6 +2326,7 @@ static void of_platform_device_create_root(struct device_node *np)
 int of_probe(void)
 {
 	struct device_node *firmware;
+	int ret;
 
 	if(!root_node)
 		return -ENODEV;
@@ -2325,7 +2337,7 @@ int of_probe(void)
 	if (of_model)
 		barebox_set_model(of_model);
 
-	of_probe_memory();
+	ret = of_probe_memory();
 
 	firmware = of_find_node_by_path("/firmware");
 	if (firmware)
@@ -2336,7 +2348,7 @@ int of_probe(void)
 	of_clk_init(root_node, NULL);
 	of_platform_populate(root_node, of_default_bus_match_table, NULL);
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/drivers/of/mem_generic.c b/drivers/of/mem_generic.c
index 9094243c0400..55d93bcb06a8 100644
--- a/drivers/of/mem_generic.c
+++ b/drivers/of/mem_generic.c
@@ -2,14 +2,15 @@
 #include <of.h>
 #include <memory.h>
 
-void of_add_memory_bank(struct device_node *node, bool dump, int r,
+int of_add_memory_bank(struct device_node *node, bool dump, int r,
 		u64 base, u64 size)
 {
 	static char str[6];
 
 	sprintf(str, "ram%d", r);
-	barebox_add_memory_bank(str, base, size);
 
 	if (dump)
 		pr_info("%s: %s: 0x%llx@0x%llx\n", node->name, str, size, base);
+
+	return barebox_add_memory_bank(str, base, size);
 }
diff --git a/include/of.h b/include/of.h
index 66d1edcc5c0d..c8ebf4009921 100644
--- a/include/of.h
+++ b/include/of.h
@@ -283,7 +283,7 @@ int of_device_is_stdout_path(struct device_d *dev);
 const char *of_get_model(void);
 void *of_flatten_dtb(struct device_node *node);
 int of_add_memory(struct device_node *node, bool dump);
-void of_add_memory_bank(struct device_node *node, bool dump, int r,
+int of_add_memory_bank(struct device_node *node, bool dump, int r,
 		u64 base, u64 size);
 struct device_d *of_find_device_by_node_path(const char *path);
 #define OF_FIND_PATH_FLAGS_BB 1		/* return .bb device if available */
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 5/6] ARM: <asm/memory.h>: propagate error codes from arm_add_mem_device()
  2021-05-31  7:12 [PATCH 0/6] memory: fuse overlapping memory banks Ahmad Fatoum
                   ` (3 preceding siblings ...)
  2021-05-31  7:12 ` [PATCH 4/6] of: warn about of_add_memory_bank errors Ahmad Fatoum
@ 2021-05-31  7:12 ` Ahmad Fatoum
  2021-05-31  7:12 ` [PATCH 6/6] ARM: report probe error at arm_add_mem_device() callsites on failure Ahmad Fatoum
  2021-06-02  6:38 ` [PATCH 0/6] memory: fuse overlapping memory banks Sascha Hauer
  6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-05-31  7:12 UTC (permalink / raw)
  To: barebox; +Cc: mol, Ahmad Fatoum

barebox_add_memory_bank() can fail if the to-be-added memory region
has been requested before. This can happen most easily on i.MX and
STM32MP1 boards:

 - The /memory node in the device tree requests a region
 - The DDR controller driver requests an overlapping region after
   reading back RAM configuration.

This most often leads to error messages down the road, but it can be
difficult to pinpoint the cause. Propagate the error code from
arm_add_mem_device(), so DDR controller drivers can fail their probe
on error.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/include/asm/memory.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 52114d0c4ef5..2279306179af 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -10,10 +10,10 @@
  */
 #define UL(x) _AC(x, UL)
 
-static inline void arm_add_mem_device(const char* name, resource_size_t start,
-				    resource_size_t size)
+static inline int arm_add_mem_device(const char* name, resource_size_t start,
+				     resource_size_t size)
 {
-	barebox_add_memory_bank(name, start, size);
+	return barebox_add_memory_bank(name, start, size);
 }
 
 #endif	/* __ASM_ARM_MEMORY_H */
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 6/6] ARM: report probe error at arm_add_mem_device() callsites on failure
  2021-05-31  7:12 [PATCH 0/6] memory: fuse overlapping memory banks Ahmad Fatoum
                   ` (4 preceding siblings ...)
  2021-05-31  7:12 ` [PATCH 5/6] ARM: <asm/memory.h>: propagate error codes from arm_add_mem_device() Ahmad Fatoum
@ 2021-05-31  7:12 ` Ahmad Fatoum
  2021-06-02  6:38 ` [PATCH 0/6] memory: fuse overlapping memory banks Sascha Hauer
  6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2021-05-31  7:12 UTC (permalink / raw)
  To: barebox; +Cc: mol, Ahmad Fatoum

Failure to add one memory bank shouldn't prevent the driver from trying
to add other memory banks, but the user should be informed as this
points at a misconfiguration. Have the probe functions eventually fail
with -EBUSY in such a case.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/mach-at91/ddramc.c      |  4 +--
 arch/arm/mach-imx/esdctl.c       | 56 ++++++++++++++++----------------
 arch/arm/mach-omap/am33xx_scrm.c |  4 +--
 arch/arm/mach-stm32mp/ddrctrl.c  |  4 +--
 4 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/arch/arm/mach-at91/ddramc.c b/arch/arm/mach-at91/ddramc.c
index 0aece5345f72..8f70b4ada2a1 100644
--- a/arch/arm/mach-at91/ddramc.c
+++ b/arch/arm/mach-at91/ddramc.c
@@ -44,9 +44,7 @@ static int sama5_ddr_probe(struct device_d *dev)
 		return PTR_ERR(iores);
 	base = IOMEM(iores->start);
 
-	arm_add_mem_device("ram0", SAMA5_DDRCS, sama5_ramsize(base));
-
-	return 0;
+	return arm_add_mem_device("ram0", SAMA5_DDRCS, sama5_ramsize(base));
 }
 
 static struct of_device_id sama5_ddr_dt_ids[] = {
diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
index f297cd35d540..5355f55c6f33 100644
--- a/arch/arm/mach-imx/esdctl.c
+++ b/arch/arm/mach-imx/esdctl.c
@@ -32,7 +32,7 @@
 struct imx_esdctl_data {
 	unsigned long base0;
 	unsigned long base1;
-	void (*add_mem)(void *esdctlbase, struct imx_esdctl_data *);
+	int (*add_mem)(void *esdctlbase, struct imx_esdctl_data *);
 };
 
 static int imx_esdctl_disabled;
@@ -182,9 +182,11 @@ static inline u64 __imx6_mmdc_sdram_size(void __iomem *mmdcbase, int cs)
 	return memory_sdram_size(cols, rows, banks, width);
 }
 
-static void add_mem(unsigned long base0, unsigned long size0,
+static int add_mem(unsigned long base0, unsigned long size0,
 		unsigned long base1, unsigned long size1)
 {
+	int ret0 = 0, ret1 = 0;
+
 	debug("%s: cs0 base: 0x%08lx cs0 size: 0x%08lx\n", __func__, base0, size0);
 	debug("%s: cs1 base: 0x%08lx cs1 size: 0x%08lx\n", __func__, base1, size1);
 
@@ -192,16 +194,16 @@ static void add_mem(unsigned long base0, unsigned long size0,
 		/*
 		 * concatenate both chip selects to a single bank
 		 */
-		arm_add_mem_device("ram0", base0, size0 + size1);
-
-		return;
+		return arm_add_mem_device("ram0", base0, size0 + size1);
 	}
 
 	if (size0)
-		arm_add_mem_device("ram0", base0, size0);
+		ret0 = arm_add_mem_device("ram0", base0, size0);
 
 	if (size1)
-		arm_add_mem_device(size0 ? "ram1" : "ram0", base1, size1);
+		ret1 = arm_add_mem_device(size0 ? "ram1" : "ram0", base1, size1);
+
+	return ret0 ? ret0 : ret1;
 }
 
 /*
@@ -224,35 +226,35 @@ static inline void imx_esdctl_v2_disable_default(void __iomem *esdctlbase)
 	}
 }
 
-static void imx_esdctl_v1_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
+static int imx_esdctl_v1_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
-	add_mem(data->base0, imx_v1_sdram_size(esdctlbase, 0),
+	return add_mem(data->base0, imx_v1_sdram_size(esdctlbase, 0),
 			data->base1, imx_v1_sdram_size(esdctlbase, 1));
 }
 
-static void imx_esdctl_v2_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
+static int imx_esdctl_v2_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
-	add_mem(data->base0, imx_v2_sdram_size(esdctlbase, 0),
+	return add_mem(data->base0, imx_v2_sdram_size(esdctlbase, 0),
 			data->base1, imx_v2_sdram_size(esdctlbase, 1));
 }
 
-static void imx_esdctl_v2_bug_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
+static int imx_esdctl_v2_bug_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
 	imx_esdctl_v2_disable_default(esdctlbase);
 
-	add_mem(data->base0, imx_v2_sdram_size(esdctlbase, 0),
+	return add_mem(data->base0, imx_v2_sdram_size(esdctlbase, 0),
 			data->base1, imx_v2_sdram_size(esdctlbase, 1));
 }
 
-static void imx_esdctl_v3_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
+static int imx_esdctl_v3_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
-	add_mem(data->base0, imx_v3_sdram_size(esdctlbase, 0),
+	return add_mem(data->base0, imx_v3_sdram_size(esdctlbase, 0),
 			data->base1, imx_v3_sdram_size(esdctlbase, 1));
 }
 
-static void imx_esdctl_v4_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
+static int imx_esdctl_v4_add_mem(void *esdctlbase, struct imx_esdctl_data *data)
 {
-	add_mem(data->base0, imx_v4_sdram_size(esdctlbase, 0),
+	return add_mem(data->base0, imx_v4_sdram_size(esdctlbase, 0),
 			data->base1, imx_v4_sdram_size(esdctlbase, 1));
 }
 
@@ -281,9 +283,9 @@ static inline resource_size_t imx6_mmdc_sdram_size(void __iomem *mmdcbase)
 	return size;
 }
 
-static void imx6_mmdc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
+static int imx6_mmdc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
 {
-	arm_add_mem_device("ram0", data->base0,
+	return arm_add_mem_device("ram0", data->base0,
 			   imx6_mmdc_sdram_size(mmdcbase));
 }
 
@@ -303,9 +305,9 @@ static inline resource_size_t vf610_ddrmc_sdram_size(void __iomem *ddrmc)
 	return memory_sdram_size(cols, rows, banks, width);
 }
 
-static void vf610_ddrmc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
+static int vf610_ddrmc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
 {
-	arm_add_mem_device("ram0", data->base0,
+	return arm_add_mem_device("ram0", data->base0,
 			   vf610_ddrmc_sdram_size(mmdcbase));
 }
 
@@ -467,9 +469,9 @@ static resource_size_t imx8m_ddrc_sdram_size(void __iomem *ddrc)
 				   reduced_adress_space);
 }
 
-static void imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
+static int imx8m_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
 {
-	arm_add_mem_device("ram0", data->base0,
+	return arm_add_mem_device("ram0", data->base0,
 			   imx8m_ddrc_sdram_size(mmdcbase));
 }
 
@@ -509,9 +511,9 @@ static resource_size_t imx7d_ddrc_sdram_size(void __iomem *ddrc)
 				   reduced_adress_space);
 }
 
-static void imx7d_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
+static int imx7d_ddrc_add_mem(void *mmdcbase, struct imx_esdctl_data *data)
 {
-	arm_add_mem_device("ram0", data->base0,
+	return arm_add_mem_device("ram0", data->base0,
 			   imx7d_ddrc_sdram_size(mmdcbase));
 }
 
@@ -534,9 +536,7 @@ static int imx_esdctl_probe(struct device_d *dev)
 	if (imx_esdctl_disabled)
 		return 0;
 
-	data->add_mem(base, data);
-
-	return 0;
+	return data->add_mem(base, data);
 }
 
 static __maybe_unused struct imx_esdctl_data imx1_data = {
diff --git a/arch/arm/mach-omap/am33xx_scrm.c b/arch/arm/mach-omap/am33xx_scrm.c
index 80510cf5b423..0f13a9deb6af 100644
--- a/arch/arm/mach-omap/am33xx_scrm.c
+++ b/arch/arm/mach-omap/am33xx_scrm.c
@@ -24,9 +24,7 @@
 
 static int am33xx_scrm_probe(struct device_d *dev)
 {
-	arm_add_mem_device("ram0", 0x80000000, am335x_sdram_size());
-
-	return 0;
+	return arm_add_mem_device("ram0", 0x80000000, am335x_sdram_size());
 }
 
 static __maybe_unused struct of_device_id am33xx_scrm_dt_ids[] = {
diff --git a/arch/arm/mach-stm32mp/ddrctrl.c b/arch/arm/mach-stm32mp/ddrctrl.c
index 646fe4401acf..43486c57c56a 100644
--- a/arch/arm/mach-stm32mp/ddrctrl.c
+++ b/arch/arm/mach-stm32mp/ddrctrl.c
@@ -132,9 +132,7 @@ static int stm32mp1_ddr_probe(struct device_d *dev)
 		return PTR_ERR(iores);
 	base = IOMEM(iores->start);
 
-	arm_add_mem_device("ram0", STM32_DDR_BASE, ddrctrl_ramsize(base));
-
-	return 0;
+	return arm_add_mem_device("ram0", STM32_DDR_BASE, ddrctrl_ramsize(base));
 }
 
 static __maybe_unused struct of_device_id stm32mp1_ddr_dt_ids[] = {
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 0/6] memory: fuse overlapping memory banks
  2021-05-31  7:12 [PATCH 0/6] memory: fuse overlapping memory banks Ahmad Fatoum
                   ` (5 preceding siblings ...)
  2021-05-31  7:12 ` [PATCH 6/6] ARM: report probe error at arm_add_mem_device() callsites on failure Ahmad Fatoum
@ 2021-06-02  6:38 ` Sascha Hauer
  6 siblings, 0 replies; 8+ messages in thread
From: Sascha Hauer @ 2021-06-02  6:38 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox, mol

On Mon, May 31, 2021 at 09:12:33AM +0200, Ahmad Fatoum wrote:
> The request region code makes sense to ensure drivers don't try to claim
> (I/O) memory region claimed by another driver or by the memory
> allocator. It doesn't make as much sense for registering available
> memory banks, because there is often overlap:
> 
>  - device tree often defines the (minimally) available /memory across
>    variants
>  - subarchitectures like i.MX, STM32MP, OMAP or SAMA5 query the main
>    RAM size from controller registers
> 
> So far, overlap went mostly unnoticed, because the RAM controller
> drivers didn't check for errors. However, if barebox is already running
> from RAM outside that described by the device tree, there will be errors
> and /delete-node/ annotations that need to be added into device trees.
> 
> Upstream device tree can be extended later on with a /memory node, which
> in turn will break barebox. This series fixes all of that. Overlapping
> memory banks are combined into one. Errors are propagated everywhere.
> /delete-node/ memory@X shouldn't be needed for new boards.
> 
> Ahmad Fatoum (6):
>   common: memory: allocate all memory devices at once
>   memory: fuse overlapping memory banks
>   of: propagate errors inside barebox_register_{of,fdt} into initcalls
>   of: warn about of_add_memory_bank errors
>   ARM: <asm/memory.h>: propagate error codes from arm_add_mem_device()
>   ARM: report probe error at arm_add_mem_device() callsites on failure

Applied, thanks

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

end of thread, other threads:[~2021-06-02  6:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-31  7:12 [PATCH 0/6] memory: fuse overlapping memory banks Ahmad Fatoum
2021-05-31  7:12 ` [PATCH 1/6] common: memory: allocate all memory devices at once Ahmad Fatoum
2021-05-31  7:12 ` [PATCH 2/6] memory: fuse overlapping memory banks Ahmad Fatoum
2021-05-31  7:12 ` [PATCH 3/6] of: propagate errors inside barebox_register_{of, fdt} into initcalls Ahmad Fatoum
2021-05-31  7:12 ` [PATCH 4/6] of: warn about of_add_memory_bank errors Ahmad Fatoum
2021-05-31  7:12 ` [PATCH 5/6] ARM: <asm/memory.h>: propagate error codes from arm_add_mem_device() Ahmad Fatoum
2021-05-31  7:12 ` [PATCH 6/6] ARM: report probe error at arm_add_mem_device() callsites on failure Ahmad Fatoum
2021-06-02  6:38 ` [PATCH 0/6] memory: fuse overlapping memory banks Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox