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@xxxxxxxxxxx>
> > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> 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
