Hi Prabhakar, On Mon, Jul 15, 2024 at 10:44 AM Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > On Fri, Jul 12, 2024 at 6:11 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > On Fri, Jul 12, 2024 at 5:29 PM Lad, Prabhakar > > <prabhakar.csengg@xxxxxxxxx> wrote: > > > On Fri, Jul 12, 2024 at 4:23 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > > > On Fri, Jul 12, 2024 at 5:14 PM Lad, Prabhakar > > > > <prabhakar.csengg@xxxxxxxxx> wrote: > > > > > On Fri, Jul 12, 2024 at 12:59 PM Geert Uytterhoeven > > > > > > On Thu, Jun 27, 2024 at 6:14 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > > > > > > > > > > Add family-specific clock driver for RZ/V2H(P) SoCs. > > > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > > > --- > > > > > > > v2->v3 > > > > > > > - Dropped num_hw_resets from struct rzv2h_cpg_priv > > > > > > > - Dropped range_check for module clocks > > > > > > > - Made mon_index to s8 instead of u8 in struct rzv2h_mod_clk > > > > > > > - Added support for critical module clocks with DEF_MOD_CRITICAL > > > > > > > - Added check for mon_index in rzv2h_mod_clock_endisable and > > > > > > > rzv2h_mod_clock_is_enabled() > > > > > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/clk/renesas/rzv2h-cpg.h > > > > > > > > > > > +/** > > > > > > > + * struct rzv2h_reset - Reset definitions > > > > > > > + * > > > > > > > + * @reset_index: reset register index > > > > > > > + * @reset_bit: reset bit > > > > > > > + * @mon_index: monitor register index > > > > > > > + * @mon_bit: monitor bit > > > > > > > + */ > > > > > > > +struct rzv2h_reset { > > > > > > > + u8 reset_index; > > > > > > > + u8 reset_bit; > > > > > > > + u8 mon_index; > > > > > > > + u8 mon_bit; > > > > > > > +}; > > > > > > > + > > > > > > > +#define RST_ID(x, y) ((((x) * 16)) + (y)) > > > > > > > + > > > > > > > +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit) \ > > > > > > > + [_id] = { \ > > > > > > > > > > > > Indexing by _id means the reset array will be very sparse. E.g. the > > > > > > innocent-looking r9a09g057_resets[] with only a single entry takes > > > > > > 600 bytes. > > > > > > > > > > > > If you do need the full array for indexing, please allocate and > > > > > > populate it at runtime. > > > > > > > > > > > OK, I will use the radix tree for resets (is that OK)? > > > > > > > > You mean XArray? include/linux/radix-tree.h has: > > > > > > > > /* Keep unconverted code working */ > > > > #define radix_tree_root xarray > > > > #define radix_tree_node xa_node > > > > > > > Yes, I meant the above. > > > > > > > Given a single xa_node is already 576 bytes, just allocating the full > > > > linear reset array at runtime is probably better. > > > > > > > Agreed, I will create a linear reset array and loop through the array > > > based on reset index and reset bit to match with id whenever required. > > > > With a full allocated linear reset array you do not need to loop, > > but you can just index it by the reset ID?? > > > Instead of having a sparse array, to save memory I was thinking > something like below: > > /** > * struct rzv2h_reset - Reset definitions > * > * @reset_index: reset register index > * @reset_bit: reset bit > * @mon_index: monitor register index > * @mon_bit: monitor bit > */ > struct rzv2h_reset { > u8 reset_index; > u8 reset_bit; > u8 mon_index; > u8 mon_bit; > }; > > #define DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit) \ > { \ > .reset_index = (_resindex), \ > .reset_bit = (_resbit), \ > .mon_index = (_monindex), \ > .mon_bit = (_monbit), \ > } > > #define DEF_RST(_resindex, _resbit, _monindex, _monbit) \ > DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit) > > > in rzv2h_cpg_probe() (.num_resets = ARRAY_SIZE(r9a09g057_resets)) > > resets = devm_kmalloc_array(dev, info->num_resets, sizeof(struct > rzv2h_reset), GFP_KERNEL); > if (!resets) > return -ENOMEM; > > for (i = 0; i < priv->num_resets; i++) > memcpy(&resets[i], &info->resets[i], sizeof(struct rzv2h_reset)); You can combine both using devm_kmemdup(). > And have the below xlate function that will convert id into index ie > index into rests array. > > static int rzv2h_get_reset_index(struct rzv2h_cpg_priv *priv, > unsigned long id) > { > u8 reset_index = id / 16; > u8 reset_bit = id % 16; > unsigned int i; > > for (i = 0; i < priv->num_resets; i++) { > if (priv->resets[i].reset_index == reset_index && > priv->resets[i].reset_bit == reset_bit) > return i; > } > > return -EINVAL; > } > > static int rzv2h_cpg_reset_xlate(struct reset_controller_dev *rcdev, > const struct of_phandle_args *reset_spec) > { > struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev); > unsigned int id = reset_spec->args[0]; > int index = rzv2h_get_reset_index(priv, id); > > if (index < 0) { > dev_err(rcdev->dev, "Invalid reset index %u\n", id); > return -EINVAL; > } > > return index; > } > > > rzv2h_cpg_assert() and rzv2h_cpg_deassert() which will use an id that > can directly index into resets[] array. > > Please let me know if this is OK. That would work, too, at the expense of needing a loop for look-up (traditional trade-off between memory and time ;-) But look-up is only done once (per device), so that should be fine. It all depends on how many resets you will end up using... Memory allocation also has a granularity, so once you have more than a specific number of resets, you better use a sparse array, and simple indexing. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds