[PATCH v7 11/23] core: remap: fix regmap_init_mem_plat() reg size handeling
Johan Jonker
jbx6244 at gmail.com
Sun Mar 12 13:21:25 CET 2023
On 3/11/23 02:37, Simon Glass wrote:
> Hi Johan,
>
> On Fri, 10 Mar 2023 at 08:42, Johan Jonker <jbx6244 at 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 at 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 at 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 at 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(-)
>>
>
> [..]
More information about the U-Boot
mailing list