Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure

[Saravana Kannan]

On 09/22/2011 03:26 PM, Mike Turquette wrote:
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..d6ae10b 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> +#ifdef CONFIG_GENERIC_CLK
> +
> +struct clk_hw {
> +	struct clk *clk;
> +};
> +
> +/**
> + * struct clk_hw_ops -  Callback operations for hardware clocks; these are to
> + * be provided by the clock implementation, and will be called by drivers
> + * through the clk_* API.
> + *
> + * @prepare:	Prepare the clock for enabling. This must not return until
> + *		the clock is fully prepared, and it's safe to call clk_enable.
> + *		This callback is intended to allow clock implementations to
> + *		do any initialisation that may sleep. Called with
> + *		prepare_lock held.
> + *
> + * @unprepare:	Release the clock from its prepared state. This will typically
> + *		undo any work done in the @prepare callback. Called with
> + *		prepare_lock held.
> + *
> + * @enable:	Enable the clock atomically. This must not return until the
> + *		clock is generating a valid clock signal, usable by consumer
> + *		devices. Called with enable_lock held. This function must not
> + *		sleep.
> + *
> + * @disable:	Disable the clock atomically. Called with enable_lock held.
> + *		This function must not sleep.
> + *
> + * @recalc_rate	Recalculate the rate of this clock, by quering hardware
> + *		and/or the clock's parent. Called with the global clock mutex
> + *		held. Optional, but recommended - if this op is not set,
> + *		clk_get_rate will return 0.
> + *
> + * @get_parent	Query the parent of this clock; for clocks with multiple
> + *		possible parents, query the hardware for the current
> + *		parent. Currently only called when the clock is first
> + *		registered.
> + *
> + * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
> + * implementations to split any work between atomic (enable) and sleepable
> + * (prepare) contexts.  If a clock requires sleeping code to be turned on, this
> + * should be done in clk_prepare. Switching that will not sleep should be done
> + * in clk_enable.
> + *
> + * Typically, drivers will call clk_prepare when a clock may be needed later
> + * (eg. when a device is opened), and clk_enable when the clock is actually
> + * required (eg. from an interrupt). Note that clk_prepare *must* have been
> + * called before clk_enable.
>    */
> +struct clk_hw_ops {
> +	int		(*prepare)(struct clk_hw *);
> +	void		(*unprepare)(struct clk_hw *);
> +	int		(*enable)(struct clk_hw *);
> +	void		(*disable)(struct clk_hw *);
> +	unsigned long	(*recalc_rate)(struct clk_hw *);
> +	long		(*round_rate)(struct clk_hw *, unsigned long);
> +	struct clk *	(*get_parent)(struct clk_hw *);
> +};

I would like to understand the need for recalc rate if that's something 
that we want to go into the common framework (even if it's optional). I 
have mostly heard only second hand explanations of the need for 
recalc_rate(), so I might not have the full picture. But for all the 
cases that I can think of, recalc_rate seems like a paradox.

If recalc_rate() is used to make sure the "current rate" of a "clock A" 
is always known even if it's parent "clock B"'s rate is changed, then it 
also means that the rate of "clock A" can change without 
clk_set_rate(clock A, new rate). That in turn means that the 
clk_get_rate() just gives the instantaneous snapshot of the rate. So, 
any use of clk_get_rate(clock A) for anything other than 
printing/logging the return value is broken code. In which case, do we 
really care for recalc_rate()? We could just return the rate that it was 
set to when clk_set_rate() was called and call it a day or return 0 for 
such clocks to indicate that the clock rate is "unknown".

The whole concept of trying to recalculate the rate for a clock makes me 
feel uneasy since it promotes misunderstanding the behavior of the clock 
and writing bad code based on that misunderstanding.

I would like to hear to real usecases before I propose some alternatives 
that I have in mind.

Thanks,
Saravana

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
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/