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

[Johan Jonker <jbx6244@gmail.com>]

On 3/13/23 18:46, John Keeping wrote:
> On Mon, Mar 13, 2023 at 05:53:20PM +0100, Johan Jonker wrote:
>> 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 phase. (ie. calloc)

Currently syscon_uc_info is allocated/set after bind on in the probe phase.

device_probe()->device_of_to_plat()->device_alloc_priv()->dev_set_uclass_priv()

Not aware how to hookup to "struct uclass_driver",so no other option to do that then here.

> 
> What about non-Rockchip SoCs using the syscon uclass?
> 
>>> 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.
> 

> Right - but the generic syscon-uclass code assumes a single range and
> that reg is the first property (at least I think it should be assuming a
> single range, but it looks like there might be a bug there now).

I'm not aware that syscon nodes can have multiple reg ranges, but don't assume that in the future there won't be a binding that does.

> 
>>> 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.
> 
> But the syscon uclass doesn't need any of that extra info - for
> OF_PLATDATA is makes assumptions and I don't see why that needs to be
> any different for 32-bit platforms with #memory-cells = <2>.  From
> syscon-uclass.c:
> 
> 	/*
> 	 * With OF_PLATDATA we really have no way of knowing the format of
> 	 * the device-specific platform data. So we assume that it starts with
> 	 * a 'reg' member, and this holds a single address and size. Drivers
> 	 * using OF_PLATDATA will need to ensure that this is true.
> 	 */
> 
> In fact, for RK3288 we can just do:
> 

> 	static_assert(sizeof(fdt_val_t) == FIELD_SIZEOF(struct dtd_rockchip_rk3288_grf, reg[0]));

fdt_val_t describes the parser capability and is not the same as the size in the dtd structure.

> 
> and the uclass gets this right.

There's no guaranty that the dtd structure is going to be like syscon_base_plat and that the reg property is first.

struct syscon_base_plat {
	phys_addr_t reg[2];
};

From syscon.yaml and other bindings we learn that there can be other properties then "reg" in syscon nodes.
Properties pop-up where ever they like in the dtd structure after combining various dtsi and dts files.

struct dtd_rockchip_rk3066_grf {
	bool		dummy_property;
	fdt64_t		reg[2];
};

Only passing a size flag is not enough.