RE: [PATCH v2 09/13] clk: renesas: rzg2l: Add support for RZ/V2M reset monitor reg

From: Phil Edworthy <phil.edworthy@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: RE: [PATCH v2 09/13] clk: renesas: rzg2l: Add support for RZ/V2M reset monitor reg
Date: Wed, 27 Apr 2022 18:00:24 +0000	[thread overview]
Message-ID: <OS3PR01MB708084236C44425A51C48B4CF5FA9@OS3PR01MB7080.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdXSRx5XxS0a0OkJd+tiyXmXGxprQkwgWDT9FODLz5=msw@mail.gmail.com>

Hi Geert,

On  26 April 2022 16:37 Geert Uytterhoeven wrote:
> On Wed, Mar 30, 2022 at 5:42 PM Phil Edworthy wrote:
> > The RZ/V2M doesn't have a matching set of reset monitor regs for each
> reset
> > reg like the RZ/G2L. Instead, it has a single CPG_RST_MON reg which has
> a
> > single bit per module.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -748,8 +748,12 @@ static int rzg2l_cpg_status(struct
> reset_controller_dev *rcdev,
> >         const struct rzg2l_cpg_info *info = priv->info;
> >         unsigned int reg = info->resets[id].off;
> >         u32 bitmask = BIT(info->resets[id].bit);
> > +       u32 monbitmask = BIT(info->resets[id].monbit);
> 
> BIT(-1) is not defined...
> 
> >
> > -       return !(readl(priv->base + CLK_MRST_R(reg)) & bitmask);
> > +       if (info->has_clk_mon_regs)
> > +               return !(readl(priv->base + CLK_MRST_R(reg)) & bitmask);
> > +       else
> > +               return !!(readl(priv->base + CPG_RST_MON) & monbitmask);
> 
> ... hence the above may behave badly when the reset has no bit in
> CPG_RST_MON (69 resets do not have a bit in CPG_RST_MON).

Ah, right. The SoCs other than RZ/V2M have monbit = -1, but they all
have info->has_clk_mon_regs = 1.
Still, I take you point that it's not very good code.

> > --- a/drivers/clk/renesas/rzg2l-cpg.h
> > +++ b/drivers/clk/renesas/rzg2l-cpg.h
> > @@ -18,6 +18,7 @@
> >  #define CPG_PL3_SSEL           (0x408)
> >  #define CPG_PL6_SSEL           (0x414)
> >  #define CPG_PL6_ETH_SSEL       (0x418)
> > +#define CPG_RST_MON            (0x680)
> >
> >  #define CPG_CLKSTATUS_SELSDHI0_STS     BIT(28)
> >  #define CPG_CLKSTATUS_SELSDHI1_STS     BIT(29)
> > @@ -151,17 +152,22 @@ struct rzg2l_mod_clk {
> >   *
> >   * @off: register offset
> >   * @bit: reset bit
> > + * @monbit: monitor bit in CPG_RST_MON register, -1 if none
> >   */
> >  struct rzg2l_reset {
> >         u16 off;
> >         u8 bit;
> > +       s8 monbit;
> >  };
> >
> > -#define DEF_RST(_id, _off, _bit)       \
> > +#define DEF_RST_MON(_id, _off, _bit, _monbit)  \
> >         [_id] = { \
> >                 .off = (_off), \
> > -               .bit = (_bit) \
> > +               .bit = (_bit), \
> > +               .monbit = (_monbit) \
> >         }
> > +#define DEF_RST(_id, _off, _bit)       \
> > +       DEF_RST_MON(_id, _off, _bit, -1)
> >
> >  /**
> >   * struct rzg2l_cpg_info - SoC-specific CPG Description

Thanks
Phil