On Mon, Mar 13, 2023 at 10:09:23PM +0100, Johan Jonker wrote:
> 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 <jbx6...@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.
What I still don't understand is how any of that is different from the
current state - so why does this need to change to support 64-bit
addresses?
All of the constraints you discuss above are mentioned in the existing
uclass-syscon.c implementation, but that implementation is working today
for all of these platforms with 32-bit addresses. So is there any need
to support all those other use cases as part of allowing 64-bit
addresses in device trees?
