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.
> > > + 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).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
