Re: [PATCH v4 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Geert,

On Mon, Jul 29, 2024 at 9:14 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Sat, Jul 27, 2024 at 12:51 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Fri, Jul 26, 2024 at 3:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, Jul 15, 2024 at 2:56 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Add family-specific clock driver for RZ/V2H(P) SoCs.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > --- /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