RE: [PATCH RESEND V4 1/9] clk: clk-divider: add CLK_DIVIDER_ZERO_GATE clk support

["A.s. Dong" <aisheng.dong@nxp.com>]

Hi Michael,

> -----Original Message-----
> From: Michael Turquette [mailto:mturquette@baylibre.com]
[...]
> Hi Dong,
> 
> Quoting A.s. Dong (2018-10-21 06:10:48)
> > For dividers with zero indicating clock is disabled, instead of giving
> > a warning each time like "clkx: Zero divisor and
> > CLK_DIVIDER_ALLOW_ZERO not set" in exist code, we'd like to introduce
> enable/disable function for it.
> > e.g.
> > 000b - Clock disabled
> > 001b - Divide by 1
> > 010b - Divide by 2
> > ...
> 
> I feel bad to NAK this patch after it's been on the list for so long, 

Never mind, I feel better than no response about it. :-)

> but this
> implementation really should belong in your hardware specific clock provider
> driver.
> 

Got your point.

> This patch expands clk-divider to also be a gate, which is a non-starter.  Basic
> clock types were meant to remain basic. I'm already imagining how this
> precedent would cause other submissions: "why should I use composite clock
> when we can just add new clk_ops to the basic clock types!" :-(
> 
> Also the implementation becomes cleaner when you don't have to make it
> coexist with clk-divider.c. You can drop the flags and just implement a machine
> specific clock type that combines gates and dividers.

Sound good to me. The original purpose to put it in framework is in order to
save possible duplicated codes for a similar SoC as the implementation actually
is HW independent. But I think we could start from putting it in machine code first.

Thanks for the suggestion.
Will update soon and resend.

Regards
Dong Aisheng
> 
> Best regards,
> Mike
> 
> >
> > Set rate when the clk is disabled will cache the rate request and only
> > when the clk is enabled will the driver actually program the hardware
> > to have the requested divider value. Similarly, when the clk is
> > disabled we'll write a 0 there, but when the clk is enabled we'll
> > restore whatever rate
> > (divider) was chosen last.
> >
> > It does mean that recalc rate will be sort of odd, because when the
> > clk is off it will return 0, and when the clk is on it will return the right rate.
> > So to make things work, we'll need to return the cached rate in recalc
> > rate when the clk is off and read the hardware when the clk is on.
> >
> > NOTE for the default off divider, the recalc rate will still return 0
> > as there's still no proper preset rate. Enable such divider will give
> > user a reminder error message.
> >
> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > ---
> > ChangeLog:
> > v3->v4:
> >  * no changes
> > v2->v3:
> >  * split normal and gate ops
> >  * fix the possible racy
> > v1->v2:
> >  * add enable/disable for the type of CLK_DIVIDER_ZERO_GATE dividers
> > ---
> >  drivers/clk/clk-divider.c    | 152
> +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk-provider.h |   9 +++
> >  2 files changed, 161 insertions(+)
> >
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > index b6234a5..b3566fd 100644
> > --- a/drivers/clk/clk-divider.c
> > +++ b/drivers/clk/clk-divider.c
> > @@ -122,6 +122,9 @@ unsigned long divider_recalc_rate(struct clk_hw
> > *hw, unsigned long parent_rate,
> >
> >         div = _get_div(table, val, flags, width);
> >         if (!div) {
> > +               if (flags & CLK_DIVIDER_ZERO_GATE)
> > +                       return 0;
> > +
> >                 WARN(!(flags & CLK_DIVIDER_ALLOW_ZERO),
> >                         "%s: Zero divisor and
> CLK_DIVIDER_ALLOW_ZERO not set\n",
> >                         clk_hw_get_name(hw)); @@ -145,6 +148,34
> @@
> > static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
> >                                    divider->flags, divider->width);  }
> >
> > +static unsigned long clk_divider_gate_recalc_rate(struct clk_hw *hw,
> > +                                                 unsigned long
> > +parent_rate) {
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       unsigned long flags = 0;
> > +       unsigned int val;
> > +
> > +       if (divider->lock)
> > +               spin_lock_irqsave(divider->lock, flags);
> > +       else
> > +               __acquire(divider->lock);
> > +
> > +       if (!clk_hw_is_enabled(hw)) {
> > +               val = divider->cached_val;
> > +       } else {
> > +               val = clk_readl(divider->reg) >> divider->shift;
> > +               val &= clk_div_mask(divider->width);
> > +       }
> > +
> > +       if (divider->lock)
> > +               spin_unlock_irqrestore(divider->lock, flags);
> > +       else
> > +               __release(divider->lock);
> > +
> > +       return divider_recalc_rate(hw, parent_rate, val, divider->table,
> > +                                  divider->flags, divider->width); }
> > +
> >  static bool _is_valid_table_div(const struct clk_div_table *table,
> >                                                          unsigned
> int
> > div)  { @@ -437,6 +468,108 @@ static int clk_divider_set_rate(struct
> > clk_hw *hw, unsigned long rate,
> >         return 0;
> >  }
> >
> > +static int clk_divider_gate_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                               unsigned long parent_rate) {
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       unsigned long flags = 0;
> > +       int value;
> > +       u32 val;
> > +
> > +       value = divider_get_val(rate, parent_rate, divider->table,
> > +                               divider->width, divider->flags);
> > +       if (value < 0)
> > +               return value;
> > +
> > +       if (divider->lock)
> > +               spin_lock_irqsave(divider->lock, flags);
> > +       else
> > +               __acquire(divider->lock);
> > +
> > +       if (clk_hw_is_enabled(hw)) {
> > +               if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
> > +                       val = clk_div_mask(divider->width) <<
> (divider->shift + 16);
> > +               } else {
> > +                       val = clk_readl(divider->reg);
> > +                       val &= ~(clk_div_mask(divider->width) <<
> divider->shift);
> > +               }
> > +               val |= (u32)value << divider->shift;
> > +               clk_writel(val, divider->reg);
> > +       } else {
> > +               divider->cached_val = value;
> > +       }
> > +
> > +       if (divider->lock)
> > +               spin_unlock_irqrestore(divider->lock, flags);
> > +       else
> > +               __release(divider->lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static int clk_divider_enable(struct clk_hw *hw) {
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       unsigned long flags = 0;
> > +       u32 val;
> > +
> > +       if (!divider->cached_val) {
> > +               pr_err("%s: no valid preset rate\n",
> clk_hw_get_name(hw));
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (divider->lock)
> > +               spin_lock_irqsave(divider->lock, flags);
> > +       else
> > +               __acquire(divider->lock);
> > +
> > +       /* restore div val */
> > +       val = clk_readl(divider->reg);
> > +       val |= divider->cached_val << divider->shift;
> > +       clk_writel(val, divider->reg);
> > +
> > +       if (divider->lock)
> > +               spin_unlock_irqrestore(divider->lock, flags);
> > +       else
> > +               __release(divider->lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static void clk_divider_disable(struct clk_hw *hw) {
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       unsigned long flags = 0;
> > +       u32 val;
> > +
> > +       if (divider->lock)
> > +               spin_lock_irqsave(divider->lock, flags);
> > +       else
> > +               __acquire(divider->lock);
> > +
> > +       /* store the current div val */
> > +       val = clk_readl(divider->reg) >> divider->shift;
> > +       val &= clk_div_mask(divider->width);
> > +       divider->cached_val = val;
> > +       clk_writel(0, divider->reg);
> > +
> > +       if (divider->lock)
> > +               spin_unlock_irqrestore(divider->lock, flags);
> > +       else
> > +               __release(divider->lock); }
> > +
> > +static int clk_divider_is_enabled(struct clk_hw *hw) {
> > +       struct clk_divider *divider = to_clk_divider(hw);
> > +       u32 val;
> > +
> > +       val = clk_readl(divider->reg) >> divider->shift;
> > +       val &= clk_div_mask(divider->width);
> > +
> > +       return val ? 1 : 0;
> > +}
> > +
> >  const struct clk_ops clk_divider_ops = {
> >         .recalc_rate = clk_divider_recalc_rate,
> >         .round_rate = clk_divider_round_rate, @@ -444,6 +577,16 @@
> > const struct clk_ops clk_divider_ops = {  };
> > EXPORT_SYMBOL_GPL(clk_divider_ops);
> >
> > +const struct clk_ops clk_divider_gate_ops = {
> > +       .recalc_rate = clk_divider_gate_recalc_rate,
> > +       .round_rate = clk_divider_round_rate,
> > +       .set_rate = clk_divider_gate_set_rate,
> > +       .enable = clk_divider_enable,
> > +       .disable = clk_divider_disable,
> > +       .is_enabled = clk_divider_is_enabled, };
> > +EXPORT_SYMBOL_GPL(clk_divider_gate_ops);
> > +
> >  const struct clk_ops clk_divider_ro_ops = {
> >         .recalc_rate = clk_divider_recalc_rate,
> >         .round_rate = clk_divider_round_rate, @@ -459,6 +602,7 @@
> > static struct clk_hw *_register_divider(struct device *dev, const char *name,
> >         struct clk_divider *div;
> >         struct clk_hw *hw;
> >         struct clk_init_data init;
> > +       u32 val;
> >         int ret;
> >
> >         if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) { @@ -476,6
> > +620,8 @@ static struct clk_hw *_register_divider(struct device *dev, const
> char *name,
> >         init.name = name;
> >         if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
> >                 init.ops = &clk_divider_ro_ops;
> > +       else if (clk_divider_flags & CLK_DIVIDER_ZERO_GATE)
> > +               init.ops = &clk_divider_gate_ops;
> >         else
> >                 init.ops = &clk_divider_ops;
> >         init.flags = flags | CLK_IS_BASIC; @@ -491,6 +637,12 @@ static
> > struct clk_hw *_register_divider(struct device *dev, const char *name,
> >         div->hw.init = &init;
> >         div->table = table;
> >
> > +       if (div->flags & CLK_DIVIDER_ZERO_GATE) {
> > +               val = clk_readl(reg) >> shift;
> > +               val &= clk_div_mask(width);
> > +               div->cached_val = val;
> > +       }
> > +
> >         /* register the clock */
> >         hw = &div->hw;
> >         ret = clk_hw_register(dev, hw); diff --git
> > a/include/linux/clk-provider.h b/include/linux/clk-provider.h index
> > 08b1aa7..08f135a 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -387,6 +387,7 @@ struct clk_div_table {
> >   * @shift:     shift to the divider bit field
> >   * @width:     width of the divider bit field
> >   * @table:     array of value/divider pairs, last entry should have div = 0
> > + * @cached_val: cached div hw value used for CLK_DIVIDER_ZERO_GATE
> >   * @lock:      register lock
> >   *
> >   * Clock with an adjustable divider affecting its output frequency.
> > Implements @@ -415,6 +416,12 @@ struct clk_div_table {
> >   * CLK_DIVIDER_MAX_AT_ZERO - For dividers which are like
> CLK_DIVIDER_ONE_BASED
> >   *     except when the value read from the register is zero, the divisor is
> >   *     2^width of the field.
> > + * CLK_DIVIDER_ZERO_GATE - For dividers which are like
> CLK_DIVIDER_ONE_BASED
> > + *     when the value read from the register is zero, it means the divisor
> > + *     is gated. For this case, the cached_val will be used to store the
> > + *     intermediate div for the normal rate operation, like
> set_rate/get_rate/
> > + *     recalc_rate. When the divider is ungated, the driver will actually
> > + *     program the hardware to have the requested divider value.
> >   */
> >  struct clk_divider {
> >         struct clk_hw   hw;
> > @@ -423,6 +430,7 @@ struct clk_divider {
> >         u8              width;
> >         u8              flags;
> >         const struct clk_div_table      *table;
> > +       u32             cached_val;
> >         spinlock_t      *lock;
> >  };
> >
> > @@ -436,6 +444,7 @@ struct clk_divider {
> >  #define CLK_DIVIDER_ROUND_CLOSEST      BIT(4)
> >  #define CLK_DIVIDER_READ_ONLY          BIT(5)
> >  #define CLK_DIVIDER_MAX_AT_ZERO                BIT(6)
> > +#define CLK_DIVIDER_ZERO_GATE          BIT(7)
> >
> >  extern const struct clk_ops clk_divider_ops;  extern const struct
> > clk_ops clk_divider_ro_ops;
> > --
> > 2.7.4
> >