Hi Geert,
On Mon, Jul 29, 2024 at 9:14 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Sat, Jul 27, 2024 at 12:51 PM Lad, Prabhakar
> <prabhakar.csengg@xxxxxxxxx> wrote:
> > On Fri, Jul 26, 2024 at 3:53 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Mon, Jul 15, 2024 at 2:56 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>
> > > > --- /dev/null
> > > > +++ b/drivers/clk/renesas/rzv2h-cpg.h
> > >
> > > > +#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:
> > >
> > > $ nm -S drivers/clk/renesas/r9a09g057-cpg.o | grep r9a09g057_resets
> > > 0000000000000038 0000000000000258 r r9a09g057_resets
> > >
> > Agreed.
> >
> > > So please pack the array here, and either unpack it while making the
> > > priv->resets copy, or implement translation ("look-up") from ID to
> > > packed index in rzv2h_cpg_reset_xlate().
> > >
> > OK, I will implement the below:
> >
> > #define PACK_RESET(_resindex, _resbit, _monindex, _monbit) \
> > (((_resindex) << 24) | ((_resbit) << 16) | ((_monindex) << 8) | (_monbit))
> >
> > #define DEF_RST(_resindex, _resbit, _monindex, _monbit) \
> > PACK_RESET(_resindex, _resbit, _monindex, _monbit)
> >
> > #define GET_RESET_INDEX(x) (((x) >> 24) & 0xFF)
> > #define GET_RESET_BIT(x) (((x) >> 16) & 0xFF)
> > #define GET_MON_INDEX(x) (((x) >> 8) & 0xFF)
> > #define GET_MON_BIT(x) ((x) & 0xFF)
> >
> > 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];
> > u8 rst_index = id / 16;
> > u8 rst_bit = id % 16;
> > unsigned int i;
> >
> > for (i = 0; i < rcdev->nr_resets; i++) {
> > u8 cur_index = GET_RESET_INDEX(priv->resets[i]);
> > u8 cur_bit = GET_RESET_BIT(priv->resets[i]);
> >
> > if (rst_index == cur_index && rst_bit == cur_bit)
> > return i;
> > }
> >
> > return -EINVAL;
> > }
> >
> > Let me know if this is OK, or to avoid looping in xlate maybe we can
> > have a packed entry in the resets property of DT by this way we can
> > avoid having the resets array all together?
>
> Sorry for being unclear. I did not mean packing the fields in the struct
> into a single word, but packing the entries in the r9a09g057_resets[]
> array. Using the rzv2h_reset structure is fine.
>
> With:
>
> #define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit) \
> [_id] = { \
> .reset_index = (_resindex), \
> .reset_bit = (_resbit), \
> .mon_index = (_monindex), \
> .mon_bit = (_monbit), \
> }
>
> #define DEF_RST(_resindex, _resbit, _monindex, _monbit) \
> DEF_RST_BASE(RST_ID((_resindex), (_resbit)), _resindex,
> _resbit, _monindex, _monbit)
>
> static const struct rzv2h_reset r9a09g057_resets[] __initconst = {
> DEF_RST(9, 5, 4, 6), /* SCIF_0_RST_SYSTEM_N */
> };
>
> is expanded into an array of 150 entries (9 * 16 + 5 = 149 empty entries
> followed by the SCIF_0_RST_SYSTEM_N entry), which is wasteful.
> Over time the array will be filled more, but I expect there will still
> be lots of unused entries.
>
> Hence I suggest to drop the "[id]":
>
> - define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit) \
> - [_id] = { \
> +#define DEF_RST(_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(RST_ID((_resindex), (_resbit)), _resindex,
> _resbit, _monindex, _monbit)
>
> Then r9a09g057_resets[] will contain only non-empty entries, at the
> expense of no longer being able to index it directly by reset ID.
> To solve the indexing, there are two options.
>
> Option A: Translate from reset ID to real index during lookup, like
> you do in the rzv2h_cpg_reset_xlate() above:
>
> 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];
> u8 rst_index = id / 16;
> u8 rst_bit = id % 16;
> unsigned int i;
>
> for (i = 0; i < rcdev->nr_resets; i++) {
> if (rst_index == priv->resets[i].reset_index &&
> rst_bit == ->resets[i].reset_bit)
> return i;
> }
>
> return -EINVAL;
> }
>
> Option B: "Unpack" rzv2h_cpg_info.resets[] during copying in
> rzv2h_cpg_probe():
>
> priv->resets = devm_kcalloc(dev, max_num_reset_ids,
> sizeof(*priv->resets), GFP_KERNEL);
> for (i = 0; i < ARRAY_SIZE(info->resets); i++) {
> id = RST_ID(info->resets[i].reset_index, info->resets[i].reset_bit);
> priv->resets[id] = info->resets[i];
> }
>
> BTW, for option B (and for the current code in v4),
> rzv2h_cpg_reset_xlate() should validate that the entry is non-empty.
>
> I hope this is more clear?
>
Yes, thanks for the clarification. I will go with option A, so we
don't waste memory.
Cheers,
Prabhakar
