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

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Fri, Jul 26, 2024 at 3:53 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> 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>
> > ---
> > v3->v4
> > - Dropped masking of parent clks with 0xffff
> > - Dropped storing mod clk id and now calculating it
> >   based on index and bit.
> > - Made parent to u16 in struct rzv2h_mod_clk
> > - Made a copy of resets array in struct rzv2h_cpg_priv
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > +/**
> > + * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
> > + *
> > + * @info: Pointer to platform data
>
> There is no longer an info member.
>
Agreed, I will drop it.

> Hint: W=1 would have told you.
>
Thanks for the pointer.

> > + * @dev: CPG device
> > + * @base: CPG register block base address
> > + * @clks: Array containing all Core and Module Clocks
> > + * @num_core_clks: Number of Core Clocks in clks[]
> > + * @num_mod_clks: Number of Module Clocks in clks[]
> > + * @resets: Array of resets
> > + * @num_resets: Number of Module Resets in info->resets[]
> > + * @last_dt_core_clk: ID of the last Core Clock exported to DT
> > + * @rcdev: Reset controller entity
> > + */
> > +struct rzv2h_cpg_priv {
> > +       struct device *dev;
> > +       void __iomem *base;
> > +
> > +       struct clk **clks;
> > +       unsigned int num_core_clks;
> > +       unsigned int num_mod_clks;
> > +       struct rzv2h_reset *resets;
> > +       unsigned int num_resets;
> > +       unsigned int last_dt_core_clk;
> > +
> > +       struct reset_controller_dev rcdev;
> > +};
>
> > index 000000000000..33631c101541
> > --- /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?

Cheers,
Prabhakar