[PATCH 05/10] clk: Add support for simple dividers

Sascha Hauer
2011-04-18T10:07:16Z

On Mon, Apr 18, 2011 at 11:49:09AM +0200, Uwe Kleine-K?nig wrote:
> On Fri, Apr 15, 2011 at 09:08:10PM +0200, Sascha Hauer wrote:
> > This patch adds support for the most common type of divider,
> > which expects a register, width and shift values to desacribe
> > the location of the divider. The divider can be zero based
> > or one based (div = reg_val + 1 vs. div = reg_val).
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
> > ---
> > drivers/clk/Kconfig | 3 +
> > drivers/clk/Makefile | 1 +
> > drivers/clk/clk-divider.c | 132 +++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/clk.h | 31 +++++++++++
> > 4 files changed, 167 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/clk/clk-divider.c
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 6e3ae54..76bb4c9 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -5,3 +5,6 @@ config CLKDEV_LOOKUP
> >
> > config USE_COMMON_STRUCT_CLK
> > bool
> > +
> > +config USE_COMMON_CLK_DIVIDER
> > + bool
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index a1a06d3..723d884 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -1,3 +1,4 @@
> >
> > obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o
> > obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o
> > +obj-$(CONFIG_USE_COMMON_CLK_DIVIDER) += clk-divider.o
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > new file mode 100644
> > index 0000000..2de94df
> > --- /dev/null
> > +++ b/drivers/clk/clk-divider.c
> > @@ -0,0 +1,132 @@
> > +/*
> > + * Copyright (C) 2011 Sascha Hauer <s.hauer@pengutronix.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * Standard functionality for the common clock API.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +
> > +#define to_clk_divider(clk) (container_of(clk, struct clk_divider, clk))
> > +
> > +static unsigned long clk_divider_get_rate(struct clk *clk)
> > +{
> > + struct clk_divider *divider = to_clk_divider(clk);
> > + unsigned long rate = clk_get_rate(divider->parent);
> > + unsigned int div;
> > +
> > + div = readl(divider->reg) >> divider->shift;
> > + div &= (1 << divider->width) - 1;
> > +
> > + if (!(divider->flags & CLK_DIVIDER_FLAG_ONE_BASED))
> > + div++;
> > +
> > + return rate / div;
> DIV_ROUND_UP(rate, div)? (There is DIV_ROUND_CLOSEST, too, but I think
> you need to round up, see below.)
>
> > +}
> > +
> > +static int clk_divider_bestdiv(struct clk *clk, unsigned long rate,
> > + unsigned long *best_parent_rate)
> conceptually this returns an unsigned value
>
> > +{
> > + struct clk_divider *divider = to_clk_divider(clk);
> > + int i, bestdiv = 0;
> > + unsigned long parent_rate, best = 0, now, maxdiv;
> > +
> > + maxdiv = (1 << divider->width);
> Is divider->width == 32 a valid scenario? If so, this overflows.
>
> > +
> > + if (divider->flags & CLK_DIVIDER_FLAG_ONE_BASED)
> > + maxdiv--;
> > +
> > + /*
> > + * The maximum divider we can use without overflowing
> > + * unsigned long in rate * i below
> > + */
> > + maxdiv = min(ULONG_MAX / rate, maxdiv);
> > +
> > + for (i = 1; i <= maxdiv; i++) {
> > + parent_rate = clk_round_rate(divider->parent, (rate + 1) * i);
> > + now = parent_rate / i;
> This is buggy, take rate = 25, i = 4. You request the parent to set
> 104. If you get that, the resulting rate is 26. (And even if
> clk_round_rate only return 101 this might be too much.)
>
> I think you need:
>
> parent_rate = clk_round_rate(divider->parent, rate * i)
> now = DIV_ROUND_UP(parent_rate, i);
>
> I think you really need DIV_ROUND_UP here, DIV_ROUND_CLOSEST won't work.
> Consider that a rate of 25 is requested, again with i=4. So the parent
> is asked for 100 and might return 97. With DIV_ROUND_CLOSEST you report
> to be able to set 24 now, but in fact it's more (24.25). If now further
> up in the recursion another divider considers 24 to be a great value to
> make 12Hz the value actually provided is too big.

I will give it a try using the DIV_ROUND_ macros. I had problems with
two cascaded dividers where due to rounding errors while dividing I
got got the next smaller divider value when passing in a value which
I could exactly get.

> > +static int clk_divider_set_rate(struct clk *clk, unsigned long rate)
> > +{
> > + unsigned long best_parent_rate;
> > + struct clk_divider *divider = to_clk_divider(clk);
> > + unsigned int div;
> > + int ret;
> > + unsigned long flags = 0;
> > + u32 val;
> > +
> > + div = clk_divider_bestdiv(clk, rate, &best_parent_rate);
> > +
> > + if (rate != best_parent_rate / div)
> > + return -EINVAL;
> This is too harsh, isn't it. Or can you expect to only get values that
> are returned by round_rate? Again you need DIV_ROUND_UP.

AFAIK there are two different implementation types in the tree. Some
implementations only allow to set to the exact rate round_rate returns
while others round down in set_rate.

Has this been specified what behaviour is expected?

> >
> > +/**
> > + * clock divider
> > + *
> > + * @clk clock source
> > + * @reg register containing the divider
> > + * @shift shift to the divider
> > + * @width width of the divider
> > + * @lock register lock
> "optional register lock"?

ok.

Sascha

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |