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

[Geert Uytterhoeven]

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.

Hint: W=1 would have told you.

> + * @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

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().

> +               .reset_index = (_resindex), \
> +               .reset_bit = (_resbit), \
> +               .mon_index = (_monindex), \
> +               .mon_bit = (_monbit), \
> +       }

The rest LGTM.

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