Re: [PATCH/RFC] ARM: shmobile: rcar-gen2: Remove CMA reservation code

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Geert,

Thanks for your work.

On 2025-01-07 12:26:55 +0100, Geert Uytterhoeven wrote:
> If CONFIG_HIGHMEM=y, two reserved blocks are allocated on R-Car Gen2:
> 
>     cma: Reserved 256 MiB at 0x70000000 on node -1
>     cma: Reserved 64 MiB at 0x6c000000 on node -1
> 
> The first block is reserved by the family-specific rcar_gen2_reserve(),
> the second by the common arm_memblock_init() (shmobile_defconfig sets
> CONFIG_CMA_SIZE_MBYTES=64).  As both blocks are reserved (eventually)
> using dma_contiguous_reserve_area(), they both have the same name
> ("reserved").  Hence if CONFIG_CMA_SYSFS=y:
> 
>   sysfs: cannot create duplicate filename '/kernel/mm/cma/reserved'
>    ...
>    cma_sysfs_init from do_one_initcall+0x84/0x178
>    ...
>   kobject: kobject_add_internal failed for reserved with -EEXIST, don't try to register things with the same name in the same directory.
> 
> This causes cma_sysfs_init() to fail completely, and not to create
> /sys/kernel/mm/cma/ at all.
> 
> Fix this by dropping the R-Car Gen2-specific reservation.  Compared to
> when it was introduced, now there exist more flexible mechanisms to
> control the size of memory reserved for CMA.  Users can reserve more
> memory by increasing CONFIG_CMA_SIZE_MBYTES, passing the cma=<N> kernel
> command line parameter, or adding a reserved-memory/linux,cma node to
> DT.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> Note that increasing CONFIG_CMA_SIZE_MBYTES in shmobile_defconfig is not
> a good idea, as it can also be used on other Renesas platforms that are
> more memory-constrained than R-Car Gen2.
> 
> Should we add reserved-memory/linux,cma nodes to DT on all affected
> boards?

For what it's worth I always set CONFIG_CMA_SIZE_MBYTES=128 to allow my 
capture tests enough CMA memory to operate correctly. I would not be 
against adding this to the upstream DTS files.

> ---
>  arch/arm/mach-shmobile/setup-rcar-gen2.c | 76 ------------------------
>  1 file changed, 76 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> index c38367a10c794162..3cd34a42e39bb1d7 100644
> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> @@ -8,19 +8,15 @@
>   */
>  
>  #include <linux/clocksource.h>
> -#include <linux/device.h>
> -#include <linux/dma-map-ops.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/memblock.h>
>  #include <linux/of.h>
>  #include <linux/of_clk.h>
> -#include <linux/of_fdt.h>
>  #include <linux/psci.h>
>  #include <asm/mach/arch.h>
>  #include <asm/secure_cntvoff.h>
>  #include "common.h"
> -#include "rcar-gen2.h"
>  
>  static const struct of_device_id cpg_matches[] __initconst = {
>  	{ .compatible = "renesas,r8a7742-cpg-mssr", .data = "extal" },
> @@ -122,76 +118,6 @@ static void __init rcar_gen2_timer_init(void)
>  	timer_probe();
>  }
>  
> -struct memory_reserve_config {
> -	u64 reserved;
> -	u64 base, size;
> -};
> -
> -static int __init rcar_gen2_scan_mem(unsigned long node, const char *uname,
> -				     int depth, void *data)
> -{
> -	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> -	const __be32 *reg, *endp;
> -	int l;
> -	struct memory_reserve_config *mrc = data;
> -	u64 lpae_start = 1ULL << 32;
> -
> -	/* We are scanning "memory" nodes only */
> -	if (type == NULL || strcmp(type, "memory"))
> -		return 0;
> -
> -	reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
> -	if (reg == NULL)
> -		reg = of_get_flat_dt_prop(node, "reg", &l);
> -	if (reg == NULL)
> -		return 0;
> -
> -	endp = reg + (l / sizeof(__be32));
> -	while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
> -		u64 base, size;
> -
> -		base = dt_mem_next_cell(dt_root_addr_cells, &reg);
> -		size = dt_mem_next_cell(dt_root_size_cells, &reg);
> -
> -		if (base >= lpae_start)
> -			continue;
> -
> -		if ((base + size) >= lpae_start)
> -			size = lpae_start - base;
> -
> -		if (size < mrc->reserved)
> -			continue;
> -
> -		if (base < mrc->base)
> -			continue;
> -
> -		/* keep the area at top near the 32-bit legacy limit */
> -		mrc->base = base + size - mrc->reserved;
> -		mrc->size = mrc->reserved;
> -	}
> -
> -	return 0;
> -}
> -
> -static void __init rcar_gen2_reserve(void)
> -{
> -	struct memory_reserve_config mrc;
> -
> -	/* reserve 256 MiB at the top of the physical legacy 32-bit space */
> -	memset(&mrc, 0, sizeof(mrc));
> -	mrc.reserved = SZ_256M;
> -
> -	of_scan_flat_dt(rcar_gen2_scan_mem, &mrc);
> -#ifdef CONFIG_DMA_CMA
> -	if (mrc.size && memblock_is_region_memory(mrc.base, mrc.size)) {
> -		static struct cma *rcar_gen2_dma_contiguous;
> -
> -		dma_contiguous_reserve_area(mrc.size, mrc.base, 0,
> -					    &rcar_gen2_dma_contiguous, true);
> -	}
> -#endif
> -}
> -
>  static const char * const rcar_gen2_boards_compat_dt[] __initconst = {
>  	"renesas,r8a7790",
>  	"renesas,r8a7791",
> @@ -204,7 +130,6 @@ static const char * const rcar_gen2_boards_compat_dt[] __initconst = {
>  DT_MACHINE_START(RCAR_GEN2_DT, "Generic R-Car Gen2 (Flattened Device Tree)")
>  	.init_late	= shmobile_init_late,
>  	.init_time	= rcar_gen2_timer_init,
> -	.reserve	= rcar_gen2_reserve,
>  	.dt_compat	= rcar_gen2_boards_compat_dt,
>  MACHINE_END
>  
> @@ -220,6 +145,5 @@ static const char * const rz_g1_boards_compat_dt[] __initconst = {
>  DT_MACHINE_START(RZ_G1_DT, "Generic RZ/G1 (Flattened Device Tree)")
>  	.init_late	= shmobile_init_late,
>  	.init_time	= rcar_gen2_timer_init,
> -	.reserve	= rcar_gen2_reserve,
>  	.dt_compat	= rz_g1_boards_compat_dt,
>  MACHINE_END
> -- 
> 2.43.0
> 
> 

-- 
Kind Regards,
Niklas Söderlund