[PATCH v6 5/8] clk: clock-wizard: Add support for fractional support

Shubhrajyoti Datta shubhraj at xilinx.com
Thu Sep 24 06:23:11 UTC 2020


Hi ,
Thanks for the review.

> -----Original Message-----
> From: Stephen Boyd <sboyd at kernel.org>
> Sent: Tuesday, September 22, 2020 2:48 AM
> To: Shubhrajyoti Datta <shubhraj at xilinx.com>; linux-clk at vger.kernel.org
> Cc: devicetree at vger.kernel.org; linux-kernel at vger.kernel.org;
> devel at driverdev.osuosl.org; robh+dt at kernel.org;
> gregkh at linuxfoundation.org; mturquette at baylibre.com; Shubhrajyoti
> Datta <shubhraj at xilinx.com>
> Subject: Re: [PATCH v6 5/8] clk: clock-wizard: Add support for fractional
> support
> 
> Quoting Shubhrajyoti Datta (2020-08-28 06:39:53)
> > Currently the set rate granularity is to integral divisors.
> > Add support for the fractional divisors.
> > Only the first output0 is fractional in the hardware.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta at xilinx.com>
> 
> Getting closer.
> 
> > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c
> > b/drivers/clk/clk-xlnx-clock-wizard.c
> > index 8dfcec8..1af59a4 100644
> > --- a/drivers/clk/clk-xlnx-clock-wizard.c
> > +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> > @@ -185,6 +191,134 @@ static const struct clk_ops
> clk_wzrd_clk_divider_ops = {
> >         .recalc_rate = clk_wzrd_recalc_rate,  };
> >
> > +static unsigned long clk_wzrd_recalc_ratef(struct clk_hw *hw,
> > +                                          unsigned long parent_rate)
> > +{
> > +       unsigned int val;
> > +       u32 div, frac;
> > +       struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
> > +       void __iomem *div_addr = divider->base + divider->offset;
> > +
> > +       val = readl(div_addr);
> > +       div = val & div_mask(divider->width);
> > +       frac = (val >> WZRD_CLKOUT_FRAC_SHIFT) &
> > + WZRD_CLKOUT_FRAC_MASK;
> > +
> > +       return ((parent_rate * 1000) / ((div * 1000) + frac));
> 
> Please remove extra parenthesis. And is this mult_frac()?
> 
Will fix
> > +}
> > +
> > +static int clk_wzrd_dynamic_reconfig_f(struct clk_hw *hw, unsigned long
> rate,
> > +                                      unsigned long parent_rate) {
> > +       int err;
> > +       u32 value, pre;
> > +       unsigned long rate_div, f, clockout0_div;
> > +       struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
> > +       void __iomem *div_addr = divider->base + divider->offset;
> > +
> > +       rate_div = ((parent_rate * 1000) / rate);
> > +       clockout0_div = rate_div / 1000;
> > +
> > +       pre = DIV_ROUND_CLOSEST((parent_rate * 1000), rate);
> > +       f = (u32)(pre - (clockout0_div * 1000));
> > +       f = f & WZRD_CLKOUT_FRAC_MASK;
> > +
> > +       value = ((f << WZRD_CLKOUT_DIVIDE_WIDTH) | (clockout0_div &
> > +                       WZRD_CLKOUT_DIVIDE_MASK));
> 
> Please split this to multiple lines.
Will fix
> 
> > +
> > +       /* Set divisor and clear phase offset */
> > +       writel(value, div_addr);
> > +       writel(0x0, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
> > +
> > +       /* Check status register */
> > +       err= readl_poll_timeout(divider->base +
> WZRD_DR_STATUS_REG_OFFSET, value,
> > +                               value & WZRD_DR_LOCK_BIT_MASK,
> > +                               WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
> > +       if (err)
> > +               return err;
> > +
> > +       /* Initiate reconfiguration */
> > +       writel(WZRD_DR_BEGIN_DYNA_RECONF,
> > +              divider->base + WZRD_DR_INIT_REG_OFFSET);
> > +
> > +       /* Check status register */
> > +       err= readl_poll_timeout(divider->base +
> WZRD_DR_STATUS_REG_OFFSET, value,
> > +                               value & WZRD_DR_LOCK_BIT_MASK,
> > +                               WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
> > +
> > +       return err;
> 
> Just return readl_poll_timeout() please.
Will fix
> 
> > +}
> > +
> > +static long clk_wzrd_round_rate_f(struct clk_hw *hw, unsigned long
> rate,
> > +                                 unsigned long *prate) {
> > +       return rate;
> 
> Can every rate be supported? This function is supposed to tell the clk
> framework what rate will be achieved if we call clk_set_rate() with 'rate'
> passed to this function. Almost always returning 'rate' is not the case.
> 

We can support rate upto 3 decimal places to prevent truncation here we are 
Returning rate.
> >
> > +
> > +static const struct clk_ops clk_wzrd_clk_divider_ops_f = {
> > +       .round_rate = clk_wzrd_round_rate_f,
> > +       .set_rate = clk_wzrd_dynamic_reconfig_f,
> > +       .recalc_rate = clk_wzrd_recalc_ratef, };
> > +
> > +static struct clk *clk_wzrd_register_divf(struct device *dev,
> > +                                         const char *name,
> > +                                         const char *parent_name,
> > +                                         unsigned long flags,
> > +                                         void __iomem *base, u16 offset,
> > +                                         u8 shift, u8 width,
> > +                                         u8 clk_divider_flags,
> > +                                         const struct clk_div_table *table,
> > +                                         spinlock_t *lock) {
> > +       struct clk_wzrd_divider *div;
> > +       struct clk_hw *hw;
> > +       struct clk_init_data init;
> > +       int ret;
> > +
> > +       if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {
> 
> Is this used? It's a rockchip specific flag mostly so probably not?
> 
> > +               if (width + shift > 16) {
> > +                       pr_warn("divider value exceeds LOWORD field\n");
> > +                       return ERR_PTR(-EINVAL);
> > +               }
> > +       }
> > +
> > +       /* allocate the divider */
> 
> Please remove useless comments like this.
Will fix
> 
> > +       div = kzalloc(sizeof(*div), GFP_KERNEL);
> > +       if (!div)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       init.name = name;
> > +
> > +       if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
> 
> Is this flag used?
Will fix
> 
> > +               init.ops = &clk_divider_ro_ops;
> > +       else
> > +               init.ops = &clk_wzrd_clk_divider_ops_f;
> > +
> > +       init.flags = flags;
> > +       init.parent_names = (parent_name ? &parent_name : NULL);
> > +       init.num_parents = (parent_name ? 1 : 0);
> 
> Do you have cases where there isn't a parent? Hopefully not, so this can be
> simplified.
> 
Will fix
> >
> > +       /* struct clk_divider assignments */
> 
> Drop this comment?
Will fix
> 
> > +       div->base = base;
> > +       div->offset = offset;
> > +       div->shift = shift;
> > +       div->width = width;
> > +       div->flags = clk_divider_flags;
> > +       div->lock = lock;
> > +       div->hw.init = &init;
> > +       div->table = table;
> > +
> > +       /* register the clock */
> 
> Drop this comment?
Will fix
> 
> > +       hw = &div->hw;
> > +       ret = clk_hw_register(dev, hw);
> 
> Any reason we can't use devm_clk_hw_register() here?
> 
Will do
> > +       if (ret) {
> > +               kfree(div);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       return hw->clk;
> > +}
> > +
> >  static struct clk *clk_wzrd_register_divider(struct device *dev,
> >                                              const char *name,
> >                                              const char *parent_name,
> > @@ -355,17 +489,13 @@ static int clk_wzrd_probe(struct
> platform_device *pdev)
> >                 goto err_disable_clk;
> >         }
> >
> > -       /* we don't support fractional div/mul yet */
> > -       reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
> > -                   WZRD_CLKFBOUT_FRAC_EN;
> > -       reg |= readl(clk_wzrd->base + WZRD_CLK_CFG_REG(2)) &
> > -                    WZRD_CLKOUT0_FRAC_EN;
> > -       if (reg)
> > -               dev_warn(&pdev->dev, "fractional div/mul not supported\n");
> > -
> >         /* register multiplier */
> >         reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
> >                      WZRD_CLKFBOUT_MULT_MASK) >>
> > WZRD_CLKFBOUT_MULT_SHIFT;
> > +       reg_f = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
> > +                    WZRD_CLKFBOUT_FRAC_MASK) >>
> > + WZRD_CLKFBOUT_FRAC_SHIFT;
> 
> Use two lines please. Read into variable on one line, shift on another.
> 
Will fix
> > +
> > +       mult = ((reg * 1000) + reg_f);
> 
> Please remove extra parenthesis.
> 
> >         clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(&pdev-
> >dev));
> >         if (!clk_name) {
> >                 ret = -ENOMEM;
> > @@ -413,8 +543,18 @@ static int clk_wzrd_probe(struct platform_device
> *pdev)
> >                         ret = -EINVAL;
> >                         goto err_rm_int_clks;
> >                 }
> > -               clk_wzrd->clkout[i] = clk_wzrd_register_divider(&pdev->dev,
> > -                                                               clkout_name,
> > +               if (!i)
> > +                       clk_wzrd->clkout[i] = clk_wzrd_register_divf
> > +                               (&pdev->dev, clkout_name,
> > +                               clk_name, 0,
> > +                               clk_wzrd->base, (WZRD_CLK_CFG_REG(2) + i * 12),
> > +                               WZRD_CLKOUT_DIVIDE_SHIFT,
> > +                               WZRD_CLKOUT_DIVIDE_WIDTH,
> > +                               CLK_DIVIDER_ONE_BASED |
> CLK_DIVIDER_ALLOW_ZERO,
> > +                               NULL, &clkwzrd_lock);
> > +               else
> > +                       clk_wzrd->clkout[i] = clk_wzrd_register_divider
> > +                               (&pdev->dev, clkout_name,
> >                                 clk_name, 0,
> >                                 clk_wzrd->base, (WZRD_CLK_CFG_REG(2) + i * 12),
> >                                 WZRD_CLKOUT_DIVIDE_SHIFT,
> >
> 
> I wonder if a do-while loop with flags set to ONE_BASED and ALLOW_ZERO
> could work and then flags gets overwritten to be just DIVIDE_SHIFT? Then
> we don't have to duplicate the registration line.

I did not understand this comment in one case I am registering  for the fractional operations
In another we are using the integral operations


More information about the devel mailing list