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

[Geert Uytterhoeven]

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?

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