linux-kernel - Re: [RFC PATCH 4/4] clk: renesas: r9a07g043: Add support for RZ/Five SoC

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: <CAMuHMdVajeFy++etApUrZC4_sgFCPH76xOOb93rQWsz42i3EVg@mail.gmail.com>
Date:   Tue, 10 May 2022 17:14:18 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Prabhakar <prabhakar.csengg@...il.com>,
        Biju Das <biju.das.jz@...renesas.com>,
        Phil Edworthy <phil.edworthy@...esas.com>
Subject: Re: [RFC PATCH 4/4] clk: renesas: r9a07g043: Add support for RZ/Five SoC

Hi Prabhakar,

On Thu, May 5, 2022 at 9:32 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@...renesas.com> wrote:
> Add minimal clock and resets entries required to boot RZ/Five SoC.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>

Thanks for your patch!

I'm only commenting on the actual clock/reset definitions, as I expect
the structure of this file to change...

> --- a/drivers/clk/renesas/r9a07g043-cpg.c
> +++ b/drivers/clk/renesas/r9a07g043-cpg.c
> @@ -151,7 +151,7 @@ static const struct {
>  static const struct {
>         struct rzg2l_mod_clk common[54];
>  #ifdef CONFIG_RISCV
> -       struct rzg2l_mod_clk rzfive[0];
> +       struct rzg2l_mod_clk rzfive[2];
>  #else
>         struct rzg2l_mod_clk rzg2ul[3];
>  #endif
> @@ -268,6 +268,10 @@ static const struct {
>         },
>  #ifdef CONFIG_RISCV
>         .rzfive = {
> +               DEF_MOD("iax45_pclk",   R9A07G043_IAX45_PCLK, R9A07G043_CLK_P2,
> +                                       0x518, 0),
> +               DEF_MOD("iax45_clk",    R9A07G043_IAX45_CLK, R9A07G043_CLK_P1,
> +                                       0x518, 1),

OK.

Note that the register offset and bits for the IAX45 block on riscv
are exactly the same as for the IA55 block on arm64.  Hence it
would be an option to use the exact same values in the DT binding
definitions for R9A07G043_IAX45_(P)CLK and R9A07G043_IA55_(P)CLK,
and avoid the #ifdefs and duplicated DEF_MOD() entries here.

I'm undecided what's the best option...

>         },
>  #else
>         .rzg2ul = {
> @@ -284,7 +288,7 @@ static const struct {
>  static const struct {
>         struct rzg2l_reset common[42];
>  #ifdef CONFIG_RISCV
> -       struct rzg2l_reset rzfive[0];
> +       struct rzg2l_reset rzfive[1];
>  #else
>         struct rzg2l_reset rzg2ul[3];
>  #endif
> @@ -335,6 +339,7 @@ static const struct {
>         },
>  #ifdef CONFIG_RISCV
>         .rzfive = {
> +               DEF_RST(R9A07G043_IAX45_RESETN, 0x818, 0),

Same for R9A07G043_IAX45_RESETN and R9A07G043_IAA55_RESETN.

>         },
>  #else
>         .rzg2ul = {
> @@ -345,16 +350,27 @@ static const struct {
>  #endif
>  };
>
> +#ifdef CONFIG_RISCV
> +static const unsigned int r9a07g043_crit_mod_clks[] __initconst = {
> +       MOD_CLK_BASE + R9A07G043_IAX45_CLK,
> +       MOD_CLK_BASE + R9A07G043_DMAC_ACLK,
> +};
> +#else
>  static const unsigned int r9a07g043_crit_mod_clks[] __initconst = {
>         MOD_CLK_BASE + R9A07G043_GIC600_GICCLK,
>         MOD_CLK_BASE + R9A07G043_IA55_CLK,
>         MOD_CLK_BASE + R9A07G043_DMAC_ACLK,
>  };
> +#endif

Please keep a single r9a07g043_crit_mod_clks[] array, and protect the
entries inside by #ifdef CONFIG_ARM64 or CONFIG_RISCV.

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