Re: [PATCH v6 3/3] clk: basic clock hardware types

[Rob Herring]

On 03/14/2012 01:23 PM, Sascha Hauer wrote:
> On Fri, Mar 09, 2012 at 11:54:24PM -0800, Mike Turquette wrote:
>> Many platforms support simple gateable clocks, fixed-rate clocks,
>> adjustable divider clocks and multi-parent multiplexer clocks.
>>
>> This patch introduces basic clock types for the above-mentioned hardware
>> which share some common characteristics.
>>
>> Based on original work by Jeremy Kerr and contribution by Jamie Iles.
>> Dividers and multiplexor clocks originally contributed by Richard Zhao &
>> Sascha Hauer.
>>
>> Signed-off-by: Mike Turquette <mturquette@xxxxxxxxxx>
>> Signed-off-by: Mike Turquette <mturquette@xxxxxx>
>> Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
>> Cc: Jeremy Kerr <jeremy.kerr@xxxxxxxxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Arnd Bergman <arnd.bergmann@xxxxxxxxxx>
>> Cc: Paul Walmsley <paul@xxxxxxxxx>
>> Cc: Shawn Guo <shawn.guo@xxxxxxxxxxxxx>
>> Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>> Cc: Jamie Iles <jamie@xxxxxxxxxxxxx>
>> Cc: Richard Zhao <richard.zhao@xxxxxxxxxx>
>> Cc: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
>> Cc: Magnus Damm <magnus.damm@xxxxxxxxx>
>> Cc: Rob Herring <rob.herring@xxxxxxxxxxx>
>> Cc: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>
>> Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
>> Cc: Amit Kucheria <amit.kucheria@xxxxxxxxxx>
>> Cc: Deepak Saxena <dsaxena@xxxxxxxxxx>
>> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
>> Cc: Andrew Lunn <andrew@xxxxxxx>
>> ---
>> Changes since v5:
>>  * standardized the hw-specific locking in the basic clock types
>>  * export the individual ops for each basic clock type
>>  * improve registration for single-parent basic clocks (thanks Sascha)
>>  * fixed bugs in gate clock's static initializers (thanks Andrew)
>>
>>  drivers/clk/Makefile         |    3 +-
>>  drivers/clk/clk-divider.c    |  204 ++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/clk-fixed-rate.c |   82 +++++++++++++++++
>>  drivers/clk/clk-gate.c       |  157 ++++++++++++++++++++++++++++++++
>>  drivers/clk/clk-mux.c        |  123 +++++++++++++++++++++++++
>>  include/linux/clk-private.h  |  124 +++++++++++++++++++++++++
>>  include/linux/clk-provider.h |  127 ++++++++++++++++++++++++++
>>  7 files changed, 819 insertions(+), 1 deletions(-)
>>  create mode 100644 drivers/clk/clk-divider.c
>>  create mode 100644 drivers/clk/clk-fixed-rate.c
>>  create mode 100644 drivers/clk/clk-gate.c
>>  create mode 100644 drivers/clk/clk-mux.c
>>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index ff362c4..1f736bc 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -1,3 +1,4 @@
>>  
>>  obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
>> -obj-$(CONFIG_COMMON_CLK)	+= clk.o
>> +obj-$(CONFIG_COMMON_CLK)	+= clk.o clk-fixed-rate.o clk-gate.o \
>> +				   clk-mux.o clk-divider.o
>> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
>> new file mode 100644
>> index 0000000..c0c4e0b
>> --- /dev/null
>> +++ b/drivers/clk/clk-divider.c
>> @@ -0,0 +1,204 @@
>> +/*
>> + * Copyright (C) 2011 Sascha Hauer, Pengutronix <s.hauer@xxxxxxxxxxxxxx>
>> + * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@xxxxxxxxxx>
>> + * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@xxxxxxxxxx>
>> + *
>> + * 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.
>> + *
>> + * Adjustable divider clock implementation
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/io.h>
>> +#include <linux/err.h>
>> +#include <linux/string.h>
>> +
>> +/*
>> + * DOC: basic adjustable divider clock that cannot gate
>> + *
>> + * Traits of this clock:
>> + * prepare - clk_prepare only ensures that parents are prepared
>> + * enable - clk_enable only ensures that parents are enabled
>> + * rate - rate is adjustable.  clk->rate = parent->rate / divisor
>> + * parent - fixed parent.  No clk_set_parent support
>> + */
>> +
>> +#define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
>> +
>> +static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
>> +		unsigned long parent_rate)
>> +{
>> +	struct clk_divider *divider = to_clk_divider(hw);
>> +	unsigned int div;
>> +	unsigned long flags = 0;
>> +
>> +	if (divider->lock)
>> +		spin_lock_irqsave(divider->lock, flags);
>> +
>> +	div = readl(divider->reg) >> divider->shift;
>> +	div &= (1 << divider->width) - 1;
>> +
>> +	if (divider->lock)
>> +		spin_unlock_irqrestore(divider->lock, flags);
>> +
>> +	if (!(divider->flags & CLK_DIVIDER_ONE_BASED))
>> +		div++;
>> +
>> +	return parent_rate / div;
>> +}
>> +EXPORT_SYMBOL_GPL(clk_divider_recalc_rate);
>> +
>> +static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
>> +		unsigned long *best_parent_rate)
>> +{
>> +	struct clk_divider *divider = to_clk_divider(hw);
>> +	int i, bestdiv = 0;
>> +	unsigned long parent_rate, best = 0, now, maxdiv;
>> +
>> +	maxdiv = (1 << divider->width);
> 
> We need a check for rate == 0 here:
> 
> 	if (rate == 0)
> 		rate = 1;
> 
> Otherwise we divide by zero below.
> 
>> +
>> +	if (divider->flags & CLK_DIVIDER_ONE_BASED)
>> +		maxdiv--;
>> +
>> +	if (!(__clk_get_flags(hw->clk) & CLK_SET_RATE_PARENT)) {
>> +		parent_rate = __clk_get_rate(__clk_get_parent(hw->clk));
>> +		bestdiv = parent_rate / rate;
> 
> It's a bit strange but due to integer maths you must use DIV_ROUND_UP
> in round_rate and div = parent_rate / rate in set_rate.
> 
> Example:
> 
> parent_rate = 100
> rate = 34
> 
> Now the best divider would be 3 (100/3 = 33), but the above
> results in 2. With DIV_ROUND_UP round_rate correctly returns
> 33. Now when you use DIV_ROUND_UP in set_rate the resulting
> divider would be 4 which is wrong whereas 100/33 returns the correct
> result.

This gets into the problem with round_rate. You can't specify whether to
round up or down. For something like an SD card you would want to pass
in the max freq for the card and get back something less than or equal
to it. For something like an LCD pixel clock, you need a frequency
greater than or equal to the requested freq to maintain a minimum
refresh rate. There's been some discussion about this some time back.

But for now we shouldn't fix round_rate, and your solution is right (or
at least safer).

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/