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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdUvfAqJR6=4MG3hXC6cn1AnKz7+RAp4GG1jvdwOctgNzQ@mail.gmail.com>
Date: Fri, 26 Jul 2024 16:53:40 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Magnus Damm <magnus.damm@...il.com>, 
	devicetree@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v4 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

Hi Prabhakar,

On Mon, Jul 15, 2024 at 2:56 PM Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Add family-specific clock driver for RZ/V2H(P) SoCs.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
> 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@...ux-m68k.org

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