Re: [PATCH v7 11/23] core: remap: fix regmap_init_mem_plat() reg size handeling

[Johan Jonker <jbx6244@gmail.com>]

On 3/11/23 02:37, Simon Glass wrote:
> Hi Johan,
> 
> On Fri, 10 Mar 2023 at 08:42, Johan Jonker <jbx6244@gmail.com> wrote:
>>
>> The fdt_addr_t and phys_addr_t size have been decoupled.
>> A 32bit CPU can expect 64-bit data from the device tree parser,
> 

> Sorry if you already responded and I missed it.
> 
> I don't understand this line. It looks like sizeof(fdt_addr_t) is
> still 4 on 32-bit machines. So what does this actually mean?

The original response on WHY we need it for partitions:
https://lore.kernel.org/u-boot/7256f237-4b7b-f7d7-834f-f7c3fb8984d7@gmail.com/T/#m81afcb203e2309c05ca97d36c63ef758cf3cef89

Below an explanation of the consequences.
Does this text below help?

Johan 

===

Current (coupled):
   phys_addr_t       fdt_addr_t
1: 32bit             32bit (problem: not enough bits to describe NAND partitions)
2: 64bit             64bit

New (decoupled)
   phys_addr_t       fdt_addr_t
1: 32bit             32bit (problem: not enough bits to describe NAND partitions)
2: 64bit             64bit
3: 32bit             64bit (Current U-boot DT with rk3288 32bit reg size DT)
4: 32bit             64bit (Synced rk3288 Linux 64bit reg size DT)

===

For situation 3 and 4:

In sandbox a 64bit addr is cast back to phys_addr_t and then to pointer of a memory area.
On real hardware a 64bit addr is cast to uintptr_t and then to a pointer.

In this serie we fix the functions that have a wrong cast on the same line and are easy to find:
-	data->base = (void *)dev_read_addr(dev);
+	data->base = dev_read_addr_ptr(dev);

This makes it pass the test.

Not fixed are drivers that cast later on:

	addr_base = dev_read_addr(dev);
	if (addr_base == FDT_ADDR_T_NONE)
		return -EINVAL;

	priv->regs = (void __iomem *)addr_base;

These drivers must be fixed by there MAINTAINERS if they like to enable decoupling.
This passes the test as long as they don't use it.

===

From io.h:

/* For sandbox, we want addresses to point into our RAM buffer */
static inline void *map_sysmem(phys_addr_t paddr, unsigned long len)
{
	return map_physmem(paddr, len, MAP_WRBACK);
}

===

From compiler.h:
/* Type for `void *' pointers. */
typedef unsigned long int uintptr_t;

From mapmem.h

/* Define a null map_sysmem() if the architecture doesn't use it */
# ifdef CONFIG_ARCH_MAP_SYSMEM
#include <asm/io.h>
# else
static inline void *map_sysmem(phys_addr_t paddr, unsigned long len)
{
	return (void *)(uintptr_t)paddr;
}

===

U-boot 32bit size:

	noc: syscon@ffac0000 {
		compatible = "rockchip,rk3288-noc", "syscon";
		reg = <0xffac0000 0x2000>;
		u-boot,dm-pre-reloc;
	};

struct dtd_rockchip_rk3288_noc {
	fdt32_t		reg[2];
};
static struct dtd_rockchip_rk3288_noc dtv_syscon_at_ffac0000 = {
	.reg			= {0xffac0000, 0x2000},
};

===

Linux 64bit reg size:

	noc: syscon@ffac0000 {
		compatible = "rockchip,rk3288-noc", "syscon";
		reg = <0x0 0xffac0000 0x0 0x2000>;
		u-boot,dm-pre-reloc;
	};

struct dtd_rockchip_rk3288_noc {
	fdt64_t		reg[2];
};
static struct dtd_rockchip_rk3288_noc dtv_syscon_at_ffac0000 = {
	.reg			= {0xffac0000, 0x2000},
};

===

Program output on why we must specify the pointer reg size or else things mess up.

/*
gcc c_size_test.c -o c_size_test
./c_size_test
sizeof char  1 8
sizeof short 2 16
sizeof int   4 32
sizeof long  8 64
016llx ffffffffffffffff
x bbaa9988
llx ffeeddccbbaa9988
x ffeeddcc
x bbaa9988
x 77665544
x 33221100
x2 bbaa9988
x2 ffeeddcc
x2 33221100
x2 77665544
llx ffeeddccbbaa9988
llx 7766554433221100
llx2 bbaa9988ffeeddcc
llx2 3322110077665544
*/

#include <stdio.h>

int main(void) {
	printf("sizeof char  %d %d\n", sizeof(char), sizeof(char)*8);
	printf("sizeof short %d %d\n", sizeof(short), sizeof(short)*8);
	printf("sizeof int   %d %d\n", sizeof(int), sizeof(int)*8);
	printf("sizeof long  %d %d\n", sizeof(long), sizeof(long)*8);
	unsigned long long val = -1;
	printf("016llx %016llx\n", (unsigned long long)val);


	unsigned long long val2[] = {0xffeeddccbbaa9988, 0x7766554433221100};
	unsigned int val3[] = {0xffeeddcc, 0xbbaa9988, 0x77665544, 0x33221100};
	printf("x %x\n", (unsigned int)val2[0]);
	printf("llx %llx\n", (unsigned long long)val2[0]);

	unsigned int *ptr = (unsigned int *)val3;
	int count;
	for (count = 2; count > 0;
		ptr += 2, count--) {
		printf("x %x\n", *ptr);
		printf("x %x\n", ptr[1]);
	}

	ptr = (unsigned int *)val2;
	for (count = 2; count > 0;
		ptr += 2, count--) {
		printf("x2 %x\n", *ptr);
		printf("x2 %x\n", ptr[1]);
	}

	unsigned long long *ptr2 = (unsigned long long *)val2;
	for (count = 1; count > 0;
		ptr += 2, count--) {
		printf("llx %llx\n", (unsigned long long)*ptr2);
		printf("llx %llx\n", (unsigned long long)ptr2[1]);
	}

	ptr2 = (unsigned long long *)val3;
	for (count = 1; count > 0;
		ptr += 2, count--) {
		printf("llx2 %llx\n", (unsigned long long)*ptr2);
		printf("llx2 %llx\n", (unsigned long long)ptr2[1]);
	}

	return 0;
}

> 
>> so convert regmap_init_mem_plat() input to handel both. The
>> syscon class driver also makes use of the regmap_init_mem_plat()
>> function, but has no way of knowing the format of the
>> device-specific platform data. In case of odd reg structures other
>> then that the syscon class driver assumes the regmap must be
>> filled in the individual syscon driver before pre-probe.
>> Also fix the ARRAY_SIZE divider in the syscon class driver.
> 
> 
> Regards,
> Simon
> 
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>
>> Changed V7:
>>   changed title
>>   add reg size input
>>   rework function calls
>> ---
>>  drivers/core/regmap.c               | 23 +++++++++++++++++++----
>>  drivers/core/syscon-uclass.c        | 21 ++++++++++++++++-----
>>  drivers/ram/rockchip/sdram_rk3066.c |  2 +-
>>  drivers/ram/rockchip/sdram_rk3188.c |  2 +-
>>  drivers/ram/rockchip/sdram_rk322x.c |  2 +-
>>  drivers/ram/rockchip/sdram_rk3288.c |  2 +-
>>  drivers/ram/rockchip/sdram_rk3328.c |  2 +-
>>  drivers/ram/rockchip/sdram_rk3399.c |  2 +-
>>  include/regmap.h                    |  5 +++--
>>  include/syscon.h                    | 13 -------------
>>  10 files changed, 44 insertions(+), 30 deletions(-)
>>
> 
> [..]