This patchset adds support for automatic selection of the best parent
for a clock mux, i.e. the one which can provide the closest clock rate
to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
so that it doesn't happen unless explicitly allowed.
This works by way of adding a parameter to the round_rate clock op which
allows the clock driver to optionally select a different parent index.
This is used in clk_calc_new_rates to decide whether to initiate a
set_parent operation. This would obviously require the argument to be
added to all users of round_rate, something this patchset doesn't do as
I'm not sure if it's really the preferred method (hence the RFC).
An alternative would be to add a new callback, but that would complicate
the code in clk.c somewhat. I suppose it would also be possible for the
round_rate callback to call a function to set a struct clk member to
mark that the parent should change (it's all within mutex protected
code after all). Comments anyone?
James Hogan (3):
clk: abstract parent cache
clk: add support for clock remuxing
clk: clk-mux: implement remuxing
drivers/clk/clk-mux.c | 47 ++++++++++++++++++
drivers/clk/clk.c | 115 ++++++++++++++++++++++++++++++-------------
include/linux/clk-provider.h | 7 ++-
3 files changed, 133 insertions(+), 36 deletions(-)
--
1.8.1.2
Abstract access to the clock parent cache by defining
__clk_get_parent_by_index(clk, index). This allows access to parent
clocks from clock drivers.
Signed-off-by: James Hogan <[email protected]>
---
drivers/clk/clk.c | 21 ++++++++++++++-------
include/linux/clk-provider.h | 1 +
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ed87b24..79d5deb 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -415,6 +415,19 @@ struct clk *__clk_get_parent(struct clk *clk)
return !clk ? NULL : clk->parent;
}
+struct clk *__clk_get_parent_by_index(struct clk *clk, u8 index)
+{
+ if (!clk || index >= clk->num_parents)
+ return NULL;
+ else if (!clk->parents)
+ return __clk_lookup(clk->parent_names[index]);
+ else if (!clk->parents[index])
+ return clk->parents[index] =
+ __clk_lookup(clk->parent_names[index]);
+ else
+ return clk->parents[index];
+}
+
unsigned int __clk_get_enable_count(struct clk *clk)
{
return !clk ? 0 : clk->enable_count;
@@ -1150,13 +1163,7 @@ static struct clk *__clk_init_parent(struct clk *clk)
kzalloc((sizeof(struct clk*) * clk->num_parents),
GFP_KERNEL);
- if (!clk->parents)
- ret = __clk_lookup(clk->parent_names[index]);
- else if (!clk->parents[index])
- ret = clk->parents[index] =
- __clk_lookup(clk->parent_names[index]);
- else
- ret = clk->parents[index];
+ ret = __clk_get_parent_by_index(clk, index);
out:
return ret;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 7f197d7..4e0b634 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -347,6 +347,7 @@ const char *__clk_get_name(struct clk *clk);
struct clk_hw *__clk_get_hw(struct clk *clk);
u8 __clk_get_num_parents(struct clk *clk);
struct clk *__clk_get_parent(struct clk *clk);
+struct clk *__clk_get_parent_by_index(struct clk *clk, u8 index);
unsigned int __clk_get_enable_count(struct clk *clk);
unsigned int __clk_get_prepare_count(struct clk *clk);
unsigned long __clk_get_rate(struct clk *clk);
--
1.8.1.2
Add support for automatic selection of the best parent for a mux, i.e.
the one which can provide the closest clock rate to that requested. This
is by way of adding a parameter to the round_rate clock op which allows
the clock to optionally select a different parent index. This is used in
clk_calc_new_rates to decide whether to initiate a set_parent operation.
This splits out the mutex protected portion of clk_set_parent() into a
separate __clk_set_parent_notify() which takes care of sending clock
change notifications.
A new clock flag is also added called CLK_SET_RATE_REMUX to indicate
that the clock can have it's parent changed automatically in response to
a set_rate. It isn't used yet, but will be used within clock mux
drivers.
Signed-off-by: James Hogan <[email protected]>
---
drivers/clk/clk.c | 94 +++++++++++++++++++++++++++++++-------------
include/linux/clk-provider.h | 6 ++-
2 files changed, 71 insertions(+), 29 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 79d5deb..3886ccd 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -36,6 +36,8 @@ static struct dentry *rootdir;
static struct dentry *orphandir;
static int inited = 0;
+static int __clk_set_parent_notify(struct clk *clk, struct clk *parent);
+
static void clk_summary_show_one(struct seq_file *s, struct clk *c, int level)
{
if (!c)
@@ -737,7 +739,7 @@ unsigned long __clk_round_rate(struct clk *clk, unsigned long rate)
if (clk->parent)
parent_rate = clk->parent->rate;
- return clk->ops->round_rate(clk->hw, rate, &parent_rate);
+ return clk->ops->round_rate(clk->hw, rate, &parent_rate, NULL);
}
/**
@@ -928,8 +930,10 @@ static void clk_calc_subtree(struct clk *clk, unsigned long new_rate)
static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
{
struct clk *top = clk;
+ struct clk *parent;
unsigned long best_parent_rate = 0;
unsigned long new_rate;
+ u8 old_parent = 0, best_parent;
/* sanity */
if (IS_ERR_OR_NULL(clk))
@@ -939,13 +943,21 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
if (clk->parent)
best_parent_rate = clk->parent->rate;
+ /* by default don't change the parent */
+ if (clk->ops->get_parent)
+ old_parent = clk->ops->get_parent(clk->hw);
+ best_parent = old_parent;
+ parent = clk->parent;
+
/* never propagate up to the parent */
if (!(clk->flags & CLK_SET_RATE_PARENT)) {
if (!clk->ops->round_rate) {
clk->new_rate = clk->rate;
return NULL;
}
- new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
+ new_rate = clk->ops->round_rate(clk->hw, rate,
+ &best_parent_rate,
+ &best_parent);
goto out;
}
@@ -962,15 +974,36 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
goto out;
}
- new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
+ new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate,
+ &best_parent);
- if (best_parent_rate != clk->parent->rate) {
- top = clk_calc_new_rates(clk->parent, best_parent_rate);
+ if (best_parent != old_parent) {
+ parent = __clk_get_parent_by_index(clk, best_parent);
+ /* couldn't find requested parent */
+ if (!parent)
+ return NULL;
+ }
+
+ if (best_parent_rate != parent->rate) {
+ top = clk_calc_new_rates(parent, best_parent_rate);
goto out;
}
out:
+ /* the parent may have changed */
+ if (best_parent != old_parent) {
+ parent = __clk_get_parent_by_index(clk, best_parent);
+ /* couldn't find requested parent */
+ if (!parent) {
+ parent = clk->parent;
+ best_parent = old_parent;
+ }
+ if (best_parent != old_parent)
+ if (__clk_set_parent_notify(clk, parent))
+ return NULL;
+ }
+
clk_calc_subtree(clk, new_rate);
return top;
@@ -1270,31 +1303,10 @@ out:
return ret;
}
-/**
- * clk_set_parent - switch the parent of a mux clk
- * @clk: the mux clk whose input we are switching
- * @parent: the new input to clk
- *
- * Re-parent clk to use parent as it's new input source. If clk has the
- * CLK_SET_PARENT_GATE flag set then clk must be gated for this
- * operation to succeed. After successfully changing clk's parent
- * clk_set_parent will update the clk topology, sysfs topology and
- * propagate rate recalculation via __clk_recalc_rates. Returns 0 on
- * success, -EERROR otherwise.
- */
-int clk_set_parent(struct clk *clk, struct clk *parent)
+static int __clk_set_parent_notify(struct clk *clk, struct clk *parent)
{
int ret = 0;
- if (!clk || !clk->ops)
- return -EINVAL;
-
- if (!clk->ops->set_parent)
- return -ENOSYS;
-
- /* prevent racing with updates to the clock topology */
- mutex_lock(&prepare_lock);
-
if (clk->parent == parent)
goto out;
@@ -1322,6 +1334,34 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
__clk_reparent(clk, parent);
out:
+ return ret;
+}
+
+/**
+ * clk_set_parent - switch the parent of a mux clk
+ * @clk: the mux clk whose input we are switching
+ * @parent: the new input to clk
+ *
+ * Re-parent clk to use parent as it's new input source. If clk has the
+ * CLK_SET_PARENT_GATE flag set then clk must be gated for this
+ * operation to succeed. After successfully changing clk's parent
+ * clk_set_parent will update the clk topology, sysfs topology and
+ * propagate rate recalculation via __clk_recalc_rates. Returns 0 on
+ * success, -EERROR otherwise.
+ */
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ int ret;
+
+ if (!clk || !clk->ops)
+ return -EINVAL;
+
+ if (!clk->ops->set_parent)
+ return -ENOSYS;
+
+ /* prevent racing with updates to the clock topology */
+ mutex_lock(&prepare_lock);
+ ret = __clk_set_parent_notify(clk, parent);
mutex_unlock(&prepare_lock);
return ret;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 4e0b634..cdad3ce 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -27,6 +27,7 @@
#define CLK_IS_ROOT BIT(4) /* root clk, has no parent */
#define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */
#define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */
+#define CLK_SET_RATE_REMUX BIT(7) /* find best parent for rate change */
struct clk_hw;
@@ -69,7 +70,8 @@ struct clk_hw;
* this op is not set then clock rate will be initialized to 0.
*
* @round_rate: Given a target rate as input, returns the closest rate actually
- * supported by the clock.
+ * supported by the clock, and optionally the index of the parent
+ * that should be used to provide the clock rate.
*
* @get_parent: Queries the hardware to determine the parent of a clock. The
* return value is a u8 which specifies the index corresponding to
@@ -115,7 +117,7 @@ struct clk_ops {
unsigned long (*recalc_rate)(struct clk_hw *hw,
unsigned long parent_rate);
long (*round_rate)(struct clk_hw *hw, unsigned long,
- unsigned long *);
+ unsigned long *, u8 *index);
int (*set_parent)(struct clk_hw *hw, u8 index);
u8 (*get_parent)(struct clk_hw *hw);
int (*set_rate)(struct clk_hw *hw, unsigned long,
--
1.8.1.2
Implement clk-mux remuxing if the CLK_SET_RATE_REMUX flag is set. This
implements round_rate for clk-mux to propagate the round_rate to each
parent and to choose the best one.
Signed-off-by: James Hogan <[email protected]>
---
drivers/clk/clk-mux.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 508c032..20b3f0b 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -82,9 +82,56 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
return 0;
}
+/* Return index of parent that provides best clock rate */
+static int clk_mux_bestparent(struct clk *clk, unsigned long rate,
+ unsigned long *best_parent_rate)
+{
+ int i, num_parents, bestparent = -1;
+ unsigned long parent_rate, best = 0;
+ struct clk *parent;
+
+ num_parents = __clk_get_num_parents(clk);
+ for (i = 0; i < num_parents; i++) {
+ parent = __clk_get_parent_by_index(clk, i);
+ if (!parent)
+ continue;
+ parent_rate = __clk_round_rate(parent, rate);
+ if (parent_rate <= rate && parent_rate > best) {
+ bestparent = i;
+ best = parent_rate;
+ *best_parent_rate = parent_rate;
+ }
+ }
+
+ return bestparent;
+}
+
+static long clk_mux_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *prate, u8 *best_parent)
+{
+ struct clk *clk = hw->clk;
+ unsigned long flags = __clk_get_flags(clk);
+ int parent;
+
+ /* never remux unless the flag is set */
+ if (!(flags & CLK_SET_RATE_REMUX)) {
+ if (flags & CLK_SET_RATE_PARENT)
+ return __clk_round_rate(__clk_get_parent(clk), rate);
+ else
+ return __clk_get_rate(clk);
+ }
+
+ parent = clk_mux_bestparent(clk, rate, prate);
+ if (best_parent && parent >= 0)
+ *best_parent = parent;
+
+ return *prate;
+}
+
const struct clk_ops clk_mux_ops = {
.get_parent = clk_mux_get_parent,
.set_parent = clk_mux_set_parent,
+ .round_rate = clk_mux_round_rate,
};
EXPORT_SYMBOL_GPL(clk_mux_ops);
--
1.8.1.2
On 03/22/13 08:43, James Hogan wrote:
> This patchset adds support for automatic selection of the best parent
> for a clock mux, i.e. the one which can provide the closest clock rate
> to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
> so that it doesn't happen unless explicitly allowed.
>
> This works by way of adding a parameter to the round_rate clock op which
> allows the clock driver to optionally select a different parent index.
> This is used in clk_calc_new_rates to decide whether to initiate a
> set_parent operation. This would obviously require the argument to be
> added to all users of round_rate, something this patchset doesn't do as
> I'm not sure if it's really the preferred method (hence the RFC).
>
> An alternative would be to add a new callback, but that would complicate
> the code in clk.c somewhat. I suppose it would also be possible for the
> round_rate callback to call a function to set a struct clk member to
> mark that the parent should change (it's all within mutex protected
> code after all). Comments anyone?
It seems like you want to be able to call clk_set_rate() on a mux?
Usually when this comes up people say you should use clk_set_parent()
but I have a real use case (see below) and maybe you do too.
This patch set caught my eye because we need to be able to switch
parents during clk_set_rate() on MSM. We usually have two or three PLLs
feed into a mux that might feed into a divider that feeds into one or
two gates. I want clk_set_rate() on these gates to propagate up through
the divider to the mux and then have the mux switch depending on the rate.
I would like to get away without writing any new code beyond the ops
that are already there though. Well, I may have to write one new op
because I have hardware that can change the mux and divider at the same
time.
Can we push the code into the core? Perhaps by doing what you do in
clk-mux.c directly in clk_calc_new_rates()? We could store a new_parent
pointer in struct clk along with the new_rate when we find the best rate
and then when we propagate rates we can propagate the parent switch.
This part would be a little tricky if a clock has both a set_parent and
a set_rate op; what do you call first? The set_rate op? The set_parent
op? It matters for my hardware. We probably need to introduce a new
set_rate_and_parent op in case both parts of the equation change so that
the driver can do it atomically if desired (for my hardware) or in the
correct order (this part could be two generic functions like
generic_clk_rate_parent() and generic_clk_parent_rate() for the two
different orders, or two flags and the logic in the core, whatever).
I like keeping it in the core because we wouldn't need to change the
round_rate() function signature, it would be called in a loop with
different parent rates, and we wouldn't have to touch the clk-mux code
at all because it would all be done generically. Plus I get almost
everything for free for my hardware, I just need to write a new op that
does that atomic mux and rate switch. What do you think?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 3 April 2013 03:06, Stephen Boyd <[email protected]> wrote:
>
> On 03/22/13 08:43, James Hogan wrote:
> > This patchset adds support for automatic selection of the best parent
> > for a clock mux, i.e. the one which can provide the closest clock rate
> > to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
> > so that it doesn't happen unless explicitly allowed.
> >
> > This works by way of adding a parameter to the round_rate clock op which
> > allows the clock driver to optionally select a different parent index.
> > This is used in clk_calc_new_rates to decide whether to initiate a
> > set_parent operation. This would obviously require the argument to be
> > added to all users of round_rate, something this patchset doesn't do as
> > I'm not sure if it's really the preferred method (hence the RFC).
> >
> > An alternative would be to add a new callback, but that would complicate
> > the code in clk.c somewhat. I suppose it would also be possible for the
> > round_rate callback to call a function to set a struct clk member to
> > mark that the parent should change (it's all within mutex protected
> > code after all). Comments anyone?
>
> It seems like you want to be able to call clk_set_rate() on a mux?
Yes, that's right.
> Usually when this comes up people say you should use clk_set_parent()
> but I have a real use case (see below) and maybe you do too.
Agreed, it shouldn't be up to generic drivers to know what clocks to
reparent to in order to get a given frequency.
>
> This patch set caught my eye because we need to be able to switch
> parents during clk_set_rate() on MSM. We usually have two or three PLLs
> feed into a mux that might feed into a divider that feeds into one or
> two gates. I want clk_set_rate() on these gates to propagate up through
> the divider to the mux and then have the mux switch depending on the rate.
We have a similar sort of thing here too. With the hardware I'm
working on, various clocks can be sourced from external oscillators,
the system clock (usually derived form a PLL), or in some cases from
other more complex clocks and PLLs, and are often fed into dividers
and gates which are the clocks that drivers are provided with.
>
> I would like to get away without writing any new code beyond the ops
> that are already there though. Well, I may have to write one new op
> because I have hardware that can change the mux and divider at the same
> time.
>
> Can we push the code into the core? Perhaps by doing what you do in
> clk-mux.c directly in clk_calc_new_rates()? We could store a new_parent
> pointer in struct clk along with the new_rate when we find the best rate
> and then when we propagate rates we can propagate the parent switch.
> This part would be a little tricky if a clock has both a set_parent and
> a set_rate op; what do you call first? The set_rate op? The set_parent
> op? It matters for my hardware. We probably need to introduce a new
> set_rate_and_parent op in case both parts of the equation change so that
> the driver can do it atomically if desired (for my hardware) or in the
> correct order (this part could be two generic functions like
> generic_clk_rate_parent() and generic_clk_parent_rate() for the two
> different orders, or two flags and the logic in the core, whatever).
>
> I like keeping it in the core because we wouldn't need to change the
> round_rate() function signature, it would be called in a loop with
> different parent rates, and we wouldn't have to touch the clk-mux code
> at all because it would all be done generically. Plus I get almost
> everything for free for my hardware, I just need to write a new op that
> does that atomic mux and rate switch. What do you think?
I like this idea a lot. It feels cleaner, and as far as I can tell at
a glance it should be possible. Thanks for the feedback, and comments
about atomic changes, I'll certainly bear those issues in mind so I
don't make it difficult to add that callback. I think it's probably
best in the non-atomic case to default to the existing behaviour (set
parent rate before child rate, which translates to setting parent
before setting rate), and if somebody comes along with a different use
case they can add the necessary flag or whatever to alter the
behaviour.
I won't get any time to try this out for a couple of weeks as I'm on
paternity leave at the moment.
Cheers
James
Quoting James Hogan (2013-04-03 14:08:17)
> On 3 April 2013 03:06, Stephen Boyd <[email protected]> wrote:
> >
> > On 03/22/13 08:43, James Hogan wrote:
> > > This patchset adds support for automatic selection of the best parent
> > > for a clock mux, i.e. the one which can provide the closest clock rate
> > > to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
> > > so that it doesn't happen unless explicitly allowed.
> > >
> > > This works by way of adding a parameter to the round_rate clock op which
> > > allows the clock driver to optionally select a different parent index.
> > > This is used in clk_calc_new_rates to decide whether to initiate a
> > > set_parent operation. This would obviously require the argument to be
> > > added to all users of round_rate, something this patchset doesn't do as
> > > I'm not sure if it's really the preferred method (hence the RFC).
> > >
> > > An alternative would be to add a new callback, but that would complicate
> > > the code in clk.c somewhat. I suppose it would also be possible for the
> > > round_rate callback to call a function to set a struct clk member to
> > > mark that the parent should change (it's all within mutex protected
> > > code after all). Comments anyone?
> >
> > It seems like you want to be able to call clk_set_rate() on a mux?
>
> Yes, that's right.
>
> > Usually when this comes up people say you should use clk_set_parent()
> > but I have a real use case (see below) and maybe you do too.
>
> Agreed, it shouldn't be up to generic drivers to know what clocks to
> reparent to in order to get a given frequency.
>
> >
> > This patch set caught my eye because we need to be able to switch
> > parents during clk_set_rate() on MSM. We usually have two or three PLLs
> > feed into a mux that might feed into a divider that feeds into one or
> > two gates. I want clk_set_rate() on these gates to propagate up through
> > the divider to the mux and then have the mux switch depending on the rate.
>
> We have a similar sort of thing here too. With the hardware I'm
> working on, various clocks can be sourced from external oscillators,
> the system clock (usually derived form a PLL), or in some cases from
> other more complex clocks and PLLs, and are often fed into dividers
> and gates which are the clocks that drivers are provided with.
>
> >
> > I would like to get away without writing any new code beyond the ops
> > that are already there though. Well, I may have to write one new op
> > because I have hardware that can change the mux and divider at the same
> > time.
> >
> > Can we push the code into the core? Perhaps by doing what you do in
> > clk-mux.c directly in clk_calc_new_rates()? We could store a new_parent
> > pointer in struct clk along with the new_rate when we find the best rate
> > and then when we propagate rates we can propagate the parent switch.
> > This part would be a little tricky if a clock has both a set_parent and
> > a set_rate op; what do you call first? The set_rate op? The set_parent
> > op? It matters for my hardware. We probably need to introduce a new
> > set_rate_and_parent op in case both parts of the equation change so that
> > the driver can do it atomically if desired (for my hardware) or in the
> > correct order (this part could be two generic functions like
> > generic_clk_rate_parent() and generic_clk_parent_rate() for the two
> > different orders, or two flags and the logic in the core, whatever).
> >
This has been discussed several times in the past and the core
definitely should handle this in a similar fashion to the way
clk->ops->recalc_rate can request a better parent rate.
One thought to not break compatibility might be something like:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0230c9d..0708fa3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1043,12 +1043,17 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
/* never propagate up to the parent */
if (!(clk->flags & CLK_SET_RATE_PARENT)) {
- if (!clk->ops->round_rate) {
+ if (clk->ops->determine_rate) {
+ new_rate = clk->ops->determine_rate(clk->hw, rate,
+ &best_parent_rate, &best_parent_clk);
+ goto out;
+ } else if (clk->ops->round_rate) {
+ new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
+ goto out;
+ } else {
clk->new_rate = clk->rate;
return NULL;
}
- new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
- goto out;
}
/* need clk->parent from here on out */
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 9fdfae7..1a19186 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -126,6 +126,9 @@ struct clk_ops {
unsigned long parent_rate);
long (*round_rate)(struct clk_hw *hw, unsigned long,
unsigned long *);
+ s64 (*determine_rate)(struct clk_hw *hw, unsigned long rate,
+ unsigned long *best_parent_rate,
+ struct clk *best_parent_clk);
int (*set_parent)(struct clk_hw *hw, u8 index);
u8 (*get_parent)(struct clk_hw *hw);
int (*set_rate)(struct clk_hw *hw, unsigned long,
Obviously lots of stuff is missing there, but it would not force every
user of .round_rate to add a new best_parent_clk parameter. Perhaps
.round_rate could be phased out in the future... or perhaps it would be
better to just do a mass update of all .round_rate implementations. It
is worth pointing out that the top-level clk_round_rate api would not be
changed; the idea above only affects clk drivers using the common struct
clk.
> > I like keeping it in the core because we wouldn't need to change the
> > round_rate() function signature, it would be called in a loop with
> > different parent rates, and we wouldn't have to touch the clk-mux code
> > at all because it would all be done generically. Plus I get almost
> > everything for free for my hardware, I just need to write a new op that
> > does that atomic mux and rate switch. What do you think?
>
Flags might be necessary to favor rate-change versus reparent, but that
is a small detail.
> I like this idea a lot. It feels cleaner, and as far as I can tell at
> a glance it should be possible. Thanks for the feedback, and comments
> about atomic changes, I'll certainly bear those issues in mind so I
> don't make it difficult to add that callback. I think it's probably
> best in the non-atomic case to default to the existing behaviour (set
> parent rate before child rate, which translates to setting parent
> before setting rate), and if somebody comes along with a different use
> case they can add the necessary flag or whatever to alter the
> behaviour.
>
> I won't get any time to try this out for a couple of weeks as I'm on
> paternity leave at the moment.
>
Congratulations! Enjoy your leave, or at least try to not become sleep
deprived.
I'll add this to my stack but it's pretty far and away compared to other
items. Feel free to beat me to it if you like.
Regards,
Mike
> Cheers
> James
Hi Mike,
On 03/04/13 22:34, Mike Turquette wrote:
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 9fdfae7..1a19186 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -126,6 +126,9 @@ struct clk_ops {
> unsigned long parent_rate);
> long (*round_rate)(struct clk_hw *hw, unsigned long,
> unsigned long *);
> + s64 (*determine_rate)(struct clk_hw *hw, unsigned long rate,
> + unsigned long *best_parent_rate,
> + struct clk *best_parent_clk);
Thanks for the comments. I've sent a new patchset based on this. I've
left the return value for determine_rate the same as round_rate though
(long rather than s64), figuring that introducing s64 would be a can of
worms, and if wanting to convert code to use s64, it should probably be
done together in a separate patchset.
Cheers
James