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
> fase. (ie. calloc)
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).
> > 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]));
and the uclass gets this right.
