Re: [PATCH v8 13/24] rockchip: rk3288: syscon_rk3288: store syscon platdata in regmap

[Johan Jonker <jbx6244@gmail.com>]

On 3/13/23 14:26, John Keeping wrote:
> On Mon, Mar 13, 2023 at 01:30:57AM +0100, Johan Jonker wrote:
>> The Rockchip SoC rk3288 has 2 types of device trees floating around.
>> A 64bit reg size when synced from Linux and a 32bit for U-boot.
>> A pre-probe function in the syscon class driver assumes only 32bit.
>> For other odd reg structures the regmap must be defined in the individual
>> syscon driver. Store rk3288 platdata in a regmap before pre-probe
>> during bind.
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
> 
> What is special about the rk3288 syscon that means the driver needs this
> handling?  Isn't this a general problem for DTs with 64-bit addresses on
> 32-bit systems that could be solved in syscon-uclass.c?

The dtd structure is only know to the driver with the SoC orientated compatible string.
I see guessing the "reg" size more as a legacy that we keep using for existing drivers
and should be deprecated.

> 
> I suspect it's difficult to handle the general case since #memory-cells
> may be difference for difference syscons so a global constant doesn't
> work, but the approach in this patch seems incredibly verbose for

You are right here, but other then rk3288 I don't see that happen for other 32bit Rockchip SoCs.
It's more verbose, because struct syscon_uc_info is not there yet in the bind fase. (ie. calloc)

> something that is likely to be needed for many platforms.
> 

> Could we use driver flags with something like:
> 
> 	.flags = of_platdata_reg_size(struct rockchip_rk3288_noc_plat),

Driver flags might solve only the "reg" size part, but not the ARRAY_SIZE and the unknown "reg" property location part.

> 
> and this untested macro:
> 
> 	#define of_platdata_reg_size(s) \
> 		((sizeof(((struct rockchip_rk3288_noc_plat *) 0)->reg) == 64) ? \
> 			DM_FLAGS_PLATDATA_REG_64BIT : 0)

This would create a parallel data flow of a "size flag and ARRAY_SIZE variable + data" in
a structure to the syscon class driver that also must be stored somewhere,
while we could do the thing correct in the regmap structure right away.

Johan