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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Prabhakar,

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