Hi Geert, On Thu, Jun 27, 2024 at 8:01 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > Hi Prabhakar, > > On Wed, Jun 26, 2024 at 7:36 PM Lad, Prabhakar > <prabhakar.csengg@xxxxxxxxx> wrote: > > On Wed, Jun 26, 2024 at 11:07 AM Geert Uytterhoeven > > <geert@xxxxxxxxxxxxxx> wrote: > > > On Tue, Jun 11, 2024 at 1:32 AM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > > > > Add family-specific clock driver for RZ/V2H(P) SoCs. > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > +/** > > > > + * struct mod_clock - Module clock > > > > + * > > > > + * @hw: handle between common and hardware-specific interfaces > > > > + * @off: register offset > > > > + * @bit: ON/MON bit > > > > + * @monoff: monitor register offset > > > > + * @monbit: montor bit > > > > + * @priv: CPG private data > > > > + */ > > > > +struct mod_clock { > > > > + struct clk_hw hw; > > > > + u8 on_index; > > > > + u8 on_bit; > > > > + u16 mon_index; > > BTW, why is this u16? The corresponding member in rzv2h_mod_clk is u8. > Oops, I will fix this. > > > > + u8 mon_bit; > > > > > +static int rzv2h_mod_clock_endisable(struct clk_hw *hw, bool enable) > > > > +{ > > > > + struct mod_clock *clock = to_mod_clock(hw); > > > > + unsigned int reg = GET_CLK_ON_OFFSET(clock->on_index); > > > > + struct rzv2h_cpg_priv *priv = clock->priv; > > > > + u32 bitmask = BIT(clock->on_bit); > > > > + struct device *dev = priv->dev; > > > > + u32 value; > > > > + int error; > > > > + > > > > + dev_dbg(dev, "CLK_ON 0x%x/%pC %s\n", reg, hw->clk, > > > > + enable ? "ON" : "OFF"); > > > > + > > > > + value = bitmask << 16; > > > > + if (enable) > > > > + value |= bitmask; > > > > + > > > > + writel(value, priv->base + reg); > > > > + > > > > + if (!enable) > > > > + return 0; > > > > + > > > > + reg = GET_CLK_MON_OFFSET(clock->mon_index); > > > > > > What if a clock does not have a clock monitor bit? > > > Clock bits in registers CPG_CLKON_22 and later do not have corresponding > > > clock monitor bits. > > > > > Oops I had missed this case. > > > > I'll introduce a macro (NO_MON_REG_INDEX) for clocks which do not have > > monitor support and add a check above to skip clk monitor operation if > > clock->mon_index == NO_MON_REG_INDEX. > > > > /* monitor index for clocks which do not have CLKMON support */ > > #define NO_MON_REG_INDEX 0xff > > > > Does this sound OK? > > Either that, or make mon_index signed (which would reduce its > effective range by one bit). > Ok I'll make it to s8 instead and add a negative check monitor index. Cheers, Prabhakar