Received-SPF: pass (google.com: domain of linux-kernel+bounces-208977-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240524082800.333991-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20240524082800.333991-4-prabhakar.mahadev-lad.rj@bp.renesas.com> <CAMuHMdVPZgxsM1OsFt-+802mzajKR6CO8B9ofFzaThKsBAdGTQ@mail.gmail.com>
In-Reply-To: <CAMuHMdVPZgxsM1OsFt-+802mzajKR6CO8B9ofFzaThKsBAdGTQ@mail.gmail.com>
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date: Mon, 10 Jun 2024 23:03:07 +0100
Message-ID: <CA+V-a8tQr8gXxAfRcffP9Bz2dL66+NOYUacKx0nmZd=TTVA9FA@mail.gmail.com>
Subject: Re: [PATCH 3/4] clk: renesas: Add RZ/V2H CPG core wrapper driver
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, 
	Philipp Zabel <p.zabel@pengutronix.de>, Magnus Damm <magnus.damm@gmail.com>, 
	linux-renesas-soc@vger.kernel.org, linux-clk@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Fabrizio Castro <fabrizio.castro.jz@renesas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Geert,

Thank you for the review.

On Tue, Jun 4, 2024 at 5:01=E2=80=AFPM Geert Uytterhoeven <geert@linux-m68k=
.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Fri, May 24, 2024 at 10:29=E2=80=AFAM Prabhakar <prabhakar.csengg@gmai=
l.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add CPG core helper wrapper driver for RZ/V2H SoC.
>
> What is a "core helper wrapper"? ;-)
>
As this file basically uses core API for clock and reset, I worded the
commit as such.

> Looking at the structure, this looks like a family-specific clock driver?
Yes, as the CPG on RZ/V2H varies quite a bit compared to RZ/G2L I have
introduced a family-specific clock driver

> Will there be more RZ/V2H-alike SoCs?
>
I'm not sure about it tbh!

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/clk/renesas/Kconfig     |   5 +
> >  drivers/clk/renesas/Makefile    |   1 +
> >  drivers/clk/renesas/rzv2h-cpg.c | 673 ++++++++++++++++++++++++++++++++
> >  drivers/clk/renesas/rzv2h-cpg.h | 149 +++++++
> >  4 files changed, 828 insertions(+)
> >  create mode 100644 drivers/clk/renesas/rzv2h-cpg.c
> >  create mode 100644 drivers/clk/renesas/rzv2h-cpg.h
> >
> > diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
> > index d252150402e8..254203c2cb2e 100644
> > --- a/drivers/clk/renesas/Kconfig
> > +++ b/drivers/clk/renesas/Kconfig
> > @@ -40,6 +40,7 @@ config CLK_RENESAS
> >         select CLK_R9A07G054 if ARCH_R9A07G054
> >         select CLK_R9A08G045 if ARCH_R9A08G045
> >         select CLK_R9A09G011 if ARCH_R9A09G011
> > +       select CLK_R9A09G057 if ARCH_R9A09G057
> >         select CLK_SH73A0 if ARCH_SH73A0
> >
> >  if CLK_RENESAS
> > @@ -193,6 +194,10 @@ config CLK_R9A09G011
> >         bool "RZ/V2M clock support" if COMPILE_TEST
> >         select CLK_RZG2L
> >
> > +config CLK_R9A09G057
> > +       bool "Renesas RZ/V2H(P) clock support" if COMPILE_TEST
>
> Please drop "Renesas "
> (few other symbols have it, I'll fix the remaining).
>
OK.

> > +       select RESET_CONTROLLER
> > +
> >  config CLK_SH73A0
> >         bool "SH-Mobile AG5 clock support" if COMPILE_TEST
> >         select CLK_RENESAS_CPG_MSTP
> > diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefil=
e
> > index f7e18679c3b8..79cc7c4d77c6 100644
> > --- a/drivers/clk/renesas/Makefile
> > +++ b/drivers/clk/renesas/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_CLK_R9A07G044)           +=3D r9a07g044-=
cpg.o
> >  obj-$(CONFIG_CLK_R9A07G054)            +=3D r9a07g044-cpg.o
> >  obj-$(CONFIG_CLK_R9A08G045)            +=3D r9a08g045-cpg.o
> >  obj-$(CONFIG_CLK_R9A09G011)            +=3D r9a09g011-cpg.o
> > +obj-$(CONFIG_CLK_R9A09G057)            +=3D rzv2h-cpg.o
>
> If this is a family-specific clock driver, please use a separate Kconfig
> symbol, like other families do, and move it ...
>
> >  obj-$(CONFIG_CLK_SH73A0)               +=3D clk-sh73a0.o
> >
> >  # Family
>
> ... here.
>
OK, I will move it to the family section.

> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > +/**
> > + * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
> > + *
> > + * @rcdev: Reset controller entity
> > + * @dev: CPG device
> > + * @base: CPG register block base address
> > + * @rmw_lock: protects register accesses
> > + * @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[]
> > + * @num_resets: Number of Module Resets in info->resets[]
> > + * @info: Pointer to platform data
> > + */
> > +struct rzv2h_cpg_priv {
> > +       struct reset_controller_dev rcdev;
> > +       struct device *dev;
> > +       void __iomem *base;
> > +       spinlock_t rmw_lock;
>
> Unused
>
I will drop it.

> > +
> > +       struct clk **clks;
> > +       unsigned int num_core_clks;
> > +       unsigned int num_mod_clks;
> > +       unsigned int num_resets;
> > +
> > +       const struct rzv2h_cpg_info *info;
> > +};
>
> > +static struct clk
> > +*rzv2h_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
> > +                              void *data)
> > +{
> > +       unsigned int clkidx =3D clkspec->args[1];
> > +       struct rzv2h_cpg_priv *priv =3D data;
> > +       struct device *dev =3D priv->dev;
> > +       const char *type;
> > +       struct clk *clk;
> > +
> > +       switch (clkspec->args[0]) {
> > +       case CPG_CORE:
> > +               type =3D "core";
> > +               clk =3D priv->clks[clkidx];
>
> No range checking?
>
I will add a check for it.

> > +               break;
> > +
> > +       case CPG_MOD:
> > +               type =3D "module";
> > +               if (clkidx >=3D priv->num_mod_clks) {
> > +                       dev_err(dev, "Invalid %s clock index %u\n", typ=
e,
> > +                               clkidx);
> > +                       return ERR_PTR(-EINVAL);
> > +               }
> > +               clk =3D priv->clks[priv->num_core_clks + clkidx];
> > +               break;
> > +
> > +       default:
> > +               dev_err(dev, "Invalid CPG clock type %u\n", clkspec->ar=
gs[0]);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       if (IS_ERR(clk))
> > +               dev_err(dev, "Cannot get %s clock %u: %ld", type, clkid=
x,
> > +                       PTR_ERR(clk));
> > +       else
> > +               dev_dbg(dev, "clock (%u, %u) is %pC at %lu Hz\n",
> > +                       clkspec->args[0], clkspec->args[1], clk,
> > +                       clk_get_rate(clk));
> > +       return clk;
> > +}
>
> > +static void __init
> > +rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
> > +                          const struct rzv2h_cpg_info *info,
> > +                          struct rzv2h_cpg_priv *priv)
> > +{
> > +       struct mstp_clock *clock =3D NULL;
> > +       struct device *dev =3D priv->dev;
> > +       unsigned int id =3D mod->id;
> > +       struct clk_init_data init;
> > +       struct clk *parent, *clk;
> > +       const char *parent_name;
> > +       unsigned int i;
> > +
> > +       WARN_DEBUG(id < priv->num_core_clks);
> > +       WARN_DEBUG(id >=3D priv->num_core_clks + priv->num_mod_clks);
> > +       WARN_DEBUG(mod->parent >=3D priv->num_core_clks + priv->num_mod=
_clks);
> > +       WARN_DEBUG(PTR_ERR(priv->clks[id]) !=3D -ENOENT);
> > +
> > +       if (!mod->name) {
> > +               /* Skip NULLified clock */
> > +               return;
> > +       }
>
> Do you have NULLified clocks?
>
Nope, I 'll drop this check.

>
> > new file mode 100644
> > index 000000000000..689c123d01c5
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzv2h-cpg.h
>
> > +/**
> > + * struct rzv2h_mod_clk - Module Clocks definitions
> > + *
> > + * @name: handle between common and hardware-specific interfaces
> > + * @id: clock index in array containing all Core and Module Clocks
> > + * @parent: id of parent clock
> > + * @off: register offset
>
> control register offset
>
> > + * @bit: ON/MON bit
>
> > + * @monoff: monitor register offset
> > + * @monbit: monitor bit
> > + */
> > +struct rzv2h_mod_clk {
> > +       const char *name;
> > +       unsigned int id;
> > +       unsigned int parent;
> > +       u16 off;
> > +       u8 bit;
>
> Perhaps name them ctrl{off,bit}?
>
> However, as all CPG_CLKONn registers are contiguous, storing
> the register index (u8) might be better than the register offset (u16)?
>
> > +       u16 monoff;
> > +       u8 monbit;
>
> Likewise for the CPG_CLKMONx registers...
>
> > +};
> > +
> > +#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _monoff, _monbit=
)        \
> > +       { \
> > +               .name =3D _name, \
> > +               .id =3D MOD_CLK_BASE + (_id), \
> > +               .parent =3D (_parent), \
> > +               .off =3D (_off), \
> > +               .bit =3D (_bit), \
> > +               .monoff =3D (_monoff), \
> > +               .monbit =3D (_monbit), \
> > +       }
> > +
> > +#define DEF_MOD(_name, _id, _parent, _off, _bit, _monoff, _monbit)    =
 \
> > +       DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _monoff, _monbit)
> > +
> > +/**
> > + * struct rzv2h_reset - Reset definitions
> > + *
> > + * @off: reset register offset
> > + * @bit: reset bit
> > + * @monoff: monitor register offset
> > + * @monbit: monitor bit
> > + */
> > +struct rzv2h_reset {
> > +       u16 resoff;
> > +       u8 resbit;
> > +       u16 monoff;
> > +       u8 monbit;
>
> ... and the CPG_RSTx and CPG_RSTMONx registers.
>
>
Ok, I will store the indexes for CLK/CLKMON/RST/RSTMON regs.

> > +};
> > +
> > +#define DEF_RST(_id, _resoff, _resbit, _monoff, _monbit)       \
> > +       [_id] =3D { \
> > +               .resoff =3D (_resoff), \
> > +               .resbit =3D (_resbit), \
> > +               .monoff =3D (_monoff), \
> > +               .monbit =3D (_monbit) \
> > +       }
> > +
> > +/**
> > + * struct rzv2h_cpg_info - SoC-specific CPG Description
> > + *
> > + * @core_clks: Array of Core Clock definitions
> > + * @num_core_clks: Number of entries in core_clks[]
> > + * @num_total_core_clks: Total number of Core Clocks (exported + inter=
nal)
> > + *
> > + * @mod_clks: Array of Module Clock definitions
> > + * @num_mod_clks: Number of entries in mod_clks[]
> > + * @num_hw_mod_clks: Number of Module Clocks supported by the hardware
> > + *
> > + * @resets: Array of Module Reset definitions
> > + * @num_resets: Number of entries in resets[]
> > + *
> > + * @crit_mod_clks: Array with Module Clock IDs of critical clocks that
> > + *                 should not be disabled without a knowledgeable driv=
er
> > + * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
> > + * @pll_get_clk1_offset: Function pointer to get PLL CLK1 offset
> > + * @pll_get_clk2_offset: Function pointer to get PLL CLK2 offset
> > + */
> > +struct rzv2h_cpg_info {
>
> > +       /* function pointers for PLL information */
> > +       int (*pll_get_clk1_offset)(int clk);
> > +       int (*pll_get_clk2_offset)(int clk);
>
> Why are these function pointers?
>
To get the offsets for PLL CLK1/2. But I plan to drop these and get
the offset from conf instead.

Cheers,
Prabhakar