[RFC PATCH 0/3] clk: attempt to keep requested rate on parent changes

[RFC PATCH 0/3] clk: attempt to keep requested rate on parent changes

[Heiko Stuebner]

I remember reading about people discussing that problem in the past, but
haven't been able to find another approach to it yet [or I'm just blind
as happens to often].

Our problem is the following clock structure:

[anotherPLL]
    |
    ------ [div] ----- dclk_vop
    |
[   vpll   ] --------- hdmi_phy


We need to set the vpll dynamically but still want to retain

The other option that comes to mind, would be to have a clock-notifier,
in the drm driver, but calling clk_set_rate from their looks like it
shouldn't work due to the prepare mutex already being held.


The whole thing is labeled RFC because while it works for us and solves
the problem, I'm not sure if I'm overlooking some important aspect or
am interferring with some other planned approach for that issue.


Heiko Stuebner (3):
  clk: fix inconsistent use of req_rate
  clk: adjust clocks to their requested rate after parent changes
  clk: rockchip: make rk3399 vop dclks keep their rate on parent rate changes

 drivers/clk/clk.c                 | 37 +++++++++++++++++++++++++++++--------
 drivers/clk/rockchip/clk-rk3399.c |  4 ++--
 include/linux/clk-provider.h      |  1 +
 3 files changed, 32 insertions(+), 10 deletions(-)

-- 
2.8.0.rc3

[RFC PATCH 1/3] clk: fix inconsistent use of req_rate

[Heiko Stuebner]

The req_rate property seems to be made to hold the rate requested through
clk_set_rate. Currently it gets initialized in clk_init to the clocks
current rate and then adapted in clk_set_rate calls. Orphan clocks and
their children get initialized to a rate of 0 and req_rate never gets
re-set when these lose their orphan-status.

Initializing req_rate to the clocks rate also is unintuitive as it just
copies the value that is already in the rate property and also looses the
information if a component actually requested a specific rate.

So separate the requested rate and only set it in clk_core_set_rate_nolock
when a real rate gets requested. The users of the req_rate __clk_put and
clk_set_rate_range that adjust a clock based on that value use req_rate
at first and fall back to rate if no rate had been requested now.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f079ea4..65e0aad 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1507,8 +1507,10 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		return 0;
 
 	/* bail early if nothing to do */
-	if (rate == clk_core_get_rate_nolock(core))
+	if (rate == clk_core_get_rate_nolock(core)) {
+		core->req_rate = req_rate;
 		return 0;
+	}
 
 	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
 		return -EBUSY;
@@ -1599,9 +1601,14 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 	clk_prepare_lock();
 
 	if (min != clk->min_rate || max != clk->max_rate) {
+		unsigned long rate = clk->core->req_rate;
+
+		if (!rate)
+			rate = clk->core->rate;
+
 		clk->min_rate = min;
 		clk->max_rate = max;
-		ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+		ret = clk_core_set_rate_nolock(clk->core, rate);
 	}
 
 	clk_prepare_unlock();
@@ -2379,7 +2386,7 @@ static int __clk_core_init(struct clk_core *core)
 		rate = core->parent->rate;
 	else
 		rate = 0;
-	core->rate = core->req_rate = rate;
+	core->rate = rate;
 
 	/*
 	 * walk the list of orphan clocks and reparent any that newly finds a
@@ -2809,6 +2816,7 @@ int __clk_get(struct clk *clk)
 
 void __clk_put(struct clk *clk)
 {
+	unsigned long rate;
 	struct module *owner;
 
 	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
@@ -2817,9 +2825,13 @@ void __clk_put(struct clk *clk)
 	clk_prepare_lock();
 
 	hlist_del(&clk->clks_node);
-	if (clk->min_rate > clk->core->req_rate ||
-	    clk->max_rate < clk->core->req_rate)
-		clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+
+	rate = clk->core->req_rate;
+	if (!rate)
+		rate = clk->core->rate;
+
+	if (clk->min_rate > rate || clk->max_rate < rate)
+		clk_core_set_rate_nolock(clk->core, rate);
 
 	owner = clk->core->owner;
 	kref_put(&clk->core->ref, __clk_release);
-- 
2.8.0.rc3

[RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Heiko Stuebner]

Given a hirarchy of clk1 -> [div] -> clk2, when the rate of clk1 gets
changed, clk2 changes as well as the divider stays the same. There may
be cases where a user of clk2 needs it at a specific rate, so clk2
needs to be readjusted for the changed rate of clk1.

So if a rate was requested for the clock, and its rate changed during
the underlying rate-change, with this change the clock framework now
tries to readjust the rate back to/near the requested one.

The whole process is protected by a new clock-flag to not force this
behaviour change onto every clock defined in the ccf.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk.c            | 13 +++++++++++--
 include/linux/clk-provider.h |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 65e0aad..22be369 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1410,6 +1410,9 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
 	return fail_clk;
 }
 
+static int clk_core_set_rate_nolock(struct clk_core *core,
+				    unsigned long req_rate);
+
 /*
  * walk down a subtree and set the new rates notifying the rate
  * change on the way
@@ -1494,6 +1497,12 @@ static void clk_change_rate(struct clk_core *core)
 	/* handle the new child who might not be in core->children yet */
 	if (core->new_child)
 		clk_change_rate(core->new_child);
+
+	/* handle a changed clock that needs to readjust its rate */
+	if (core->flags & CLK_KEEP_REQ_RATE && core->req_rate
+					    && core->new_rate != old_rate
+					    && core->new_rate != core->req_rate)
+		clk_core_set_rate_nolock(core, core->req_rate);
 }
 
 static int clk_core_set_rate_nolock(struct clk_core *core,
@@ -1529,11 +1538,11 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		return -EBUSY;
 	}
 
+	core->req_rate = req_rate;
+
 	/* change the rates */
 	clk_change_rate(top);
 
-	core->req_rate = req_rate;
-
 	return ret;
 }
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 0c72204..06f189e 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -33,6 +33,7 @@
 #define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
 #define CLK_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
 #define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
+#define CLK_KEEP_REQ_RATE	BIT(12) /* keep reqrate on parent rate change */
 
 struct clk;
 struct clk_hw;
-- 
2.8.0.rc3

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Doug Anderson]

Heiko,

On Mon, May 2, 2016 at 9:36 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> Given a hirarchy of clk1 -> [div] -> clk2, when the rate of clk1 gets
> changed, clk2 changes as well as the divider stays the same. There may
> be cases where a user of clk2 needs it at a specific rate, so clk2
> needs to be readjusted for the changed rate of clk1.
>
> So if a rate was requested for the clock, and its rate changed during
> the underlying rate-change, with this change the clock framework now
> tries to readjust the rate back to/near the requested one.
>
> The whole process is protected by a new clock-flag to not force this
> behaviour change onto every clock defined in the ccf.

I'm not an expert on CCF details, but presumably you need to be really
careful here.  Is there any way you could get an infinite loop here
where you ping-pong between two people trying to control their parent
clock?  Right now I see mutual recursion between
clk_core_set_rate_nolock() and clk_change_rate() and no base case.

Specifically if there's a path (because of CLK_SET_RATE_PARENT) where
setting a clock rate on "clk2" in your example can cause a rate change
of "clk1" I worry that we'll be in trouble.  Maybe a requirement of
your patch is that no such path exists?  ...or maybe something in the
code prevents this...


> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/clk.c            | 13 +++++++++++--
>  include/linux/clk-provider.h |  1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 65e0aad..22be369 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1410,6 +1410,9 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
>         return fail_clk;
>  }
>
> +static int clk_core_set_rate_nolock(struct clk_core *core,
> +                                   unsigned long req_rate);
> +
>  /*
>   * walk down a subtree and set the new rates notifying the rate
>   * change on the way
> @@ -1494,6 +1497,12 @@ static void clk_change_rate(struct clk_core *core)
>         /* handle the new child who might not be in core->children yet */
>         if (core->new_child)
>                 clk_change_rate(core->new_child);
> +
> +       /* handle a changed clock that needs to readjust its rate */
> +       if (core->flags & CLK_KEEP_REQ_RATE && core->req_rate
> +                                           && core->new_rate != old_rate
> +                                           && core->new_rate != core->req_rate)
> +               clk_core_set_rate_nolock(core, core->req_rate);

I guess we don't care about errors here?

...maybe (?) we could ignore errors if we validated this rate change
with PRE_RATE_CHANGE before attempting to change the parent clock, but
I don't see the code doing this unless I missed it.


>  }
>
>  static int clk_core_set_rate_nolock(struct clk_core *core,
> @@ -1529,11 +1538,11 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>                 return -EBUSY;
>         }
>
> +       core->req_rate = req_rate;
> +
>         /* change the rates */
>         clk_change_rate(top);
>
> -       core->req_rate = req_rate;
> -
>         return ret;
>  }
>
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 0c72204..06f189e 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -33,6 +33,7 @@
>  #define CLK_RECALC_NEW_RATES   BIT(9) /* recalc rates after notifications */
>  #define CLK_SET_RATE_UNGATE    BIT(10) /* clock needs to run to set rate */
>  #define CLK_IS_CRITICAL                BIT(11) /* do not gate, ever */
> +#define CLK_KEEP_REQ_RATE      BIT(12) /* keep reqrate on parent rate change */

s/reqrate/req_rate/

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Doug Anderson]

Hi,

On Thu, May 5, 2016 at 5:35 PM, Doug Anderson <dianders@chromium.org> wrote:
> I guess we don't care about errors here?
>
> ...maybe (?) we could ignore errors if we validated this rate change
> with PRE_RATE_CHANGE before attempting to change the parent clock, but
> I don't see the code doing this unless I missed it.

One other related thought: it seems like there should be code
_somewhere_ that decides whether to adjust the child dividers before
or after the parent clock changes.  Specifically if the parent clock
is increasing in speed we probably want to slow down the child clock
ahead of the parent's increase.  I don't see such code here, but again
I'm pretty good at missing things unless they are slapping me in the
face.  ;)

-Doug

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Heiko Stuebner]

Am Donnerstag, 5. Mai 2016, 17:49:39 schrieb Doug Anderson:
> Hi,
> 
> On Thu, May 5, 2016 at 5:35 PM, Doug Anderson <dianders@chromium.org> wrote:
> > I guess we don't care about errors here?
> > 
> > ...maybe (?) we could ignore errors if we validated this rate change
> > with PRE_RATE_CHANGE before attempting to change the parent clock, but
> > I don't see the code doing this unless I missed it.
> 
> One other related thought: it seems like there should be code
> _somewhere_ that decides whether to adjust the child dividers before
> or after the parent clock changes.  Specifically if the parent clock
> is increasing in speed we probably want to slow down the child clock
> ahead of the parent's increase.  I don't see such code here, but again
> I'm pretty good at missing things unless they are slapping me in the
> face.  ;)

yep, but that problem of a divider exceeding a requested rate temporarily
exist currently already, even before this change ;-)

Recently we did fix it on a small-scale for composite-clocks though [0].


Heiko


[0] https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/commit/drivers/clk/clk-composite.c?h=clk-next&id=9e52cec04fd3b9b686f9256151b47fe61f7c28ef

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Heiko Stuebner]

Am Donnerstag, 5. Mai 2016, 17:35:03 schrieb Doug Anderson:
> Heiko,
> 
> On Mon, May 2, 2016 at 9:36 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Given a hirarchy of clk1 -> [div] -> clk2, when the rate of clk1 gets
> > changed, clk2 changes as well as the divider stays the same. There may
> > be cases where a user of clk2 needs it at a specific rate, so clk2
> > needs to be readjusted for the changed rate of clk1.
> > 
> > So if a rate was requested for the clock, and its rate changed during
> > the underlying rate-change, with this change the clock framework now
> > tries to readjust the rate back to/near the requested one.
> > 
> > The whole process is protected by a new clock-flag to not force this
> > behaviour change onto every clock defined in the ccf.
> 
> I'm not an expert on CCF details, but presumably you need to be really
> careful here.  Is there any way you could get an infinite loop here
> where you ping-pong between two people trying to control their parent
> clock?  Right now I see mutual recursion between
> clk_core_set_rate_nolock() and clk_change_rate() and no base case.
> 
> Specifically if there's a path (because of CLK_SET_RATE_PARENT) where
> setting a clock rate on "clk2" in your example can cause a rate change
> of "clk1" I worry that we'll be in trouble.  Maybe a requirement of
> your patch is that no such path exists?  ...or maybe something in the
> code prevents this...

This was one of my worries as well, which is why the flag exists in the first 
place, right now offloading the requirement to check for such conflict cases to 
the clock-tree creator.


I think one possible test to add could be to check for conflicts between 
CLK_SET_RATE_PARENT and this new flag.

Aka in clk-init once encountering the CLK_KEEP_REQRATE, go up through parent 
clocks as long as CLK_SET_RATE_PARENT is set and if we encounter a parent 
with num_children > 1 (aka a shared base-clock, like a PLL on rockchip) 
unset CLK_KEEP_REQRATE, as it would like introduce that ping-pong game.

Hmm, although this test would also fire in cases like Rockchip's fractional 
dividers, where there is the common pll-select-mux+divider and after that 
the mux selecting between that or having the fractional-divider in between 
as well.

I guess it can get hairy to detect such cases sucessfully.


> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/clk/clk.c            | 13 +++++++++++--
> >  include/linux/clk-provider.h |  1 +
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 65e0aad..22be369 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1410,6 +1410,9 @@ static struct clk_core
> > *clk_propagate_rate_change(struct clk_core *core,> 
> >         return fail_clk;
> >  
> >  }
> > 
> > +static int clk_core_set_rate_nolock(struct clk_core *core,
> > +                                   unsigned long req_rate);
> > +
> > 
> >  /*
> >  
> >   * walk down a subtree and set the new rates notifying the rate
> >   * change on the way
> > 
> > @@ -1494,6 +1497,12 @@ static void clk_change_rate(struct clk_core
> > *core)
> > 
> >         /* handle the new child who might not be in core->children yet
> >         */
> >         if (core->new_child)
> >         
> >                 clk_change_rate(core->new_child);
> > 
> > +
> > +       /* handle a changed clock that needs to readjust its rate */
> > +       if (core->flags & CLK_KEEP_REQ_RATE && core->req_rate
> > +                                           && core->new_rate !=
> > old_rate
> > +                                           && core->new_rate !=
> > core->req_rate) +               clk_core_set_rate_nolock(core,
> > core->req_rate);
> 
> I guess we don't care about errors here?
> 
> ...maybe (?) we could ignore errors if we validated this rate change
> with PRE_RATE_CHANGE before attempting to change the parent clock, but
> I don't see the code doing this unless I missed it.

It's more like what would you want to do in the error/failure cases.

So far I was going by the assumption we're going to change the clock anyway 
and just try to pull marked clocks along, so in the error case it would just 
fall back to the current behaviour.


Heiko

> >  }
> >  
> >  static int clk_core_set_rate_nolock(struct clk_core *core,
> > 
> > @@ -1529,11 +1538,11 @@ static int clk_core_set_rate_nolock(struct
> > clk_core *core,> 
> >                 return -EBUSY;
> >         
> >         }
> > 
> > +       core->req_rate = req_rate;
> > +
> > 
> >         /* change the rates */
> >         clk_change_rate(top);
> > 
> > -       core->req_rate = req_rate;
> > -
> > 
> >         return ret;
> >  
> >  }
> > 
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 0c72204..06f189e 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -33,6 +33,7 @@
> > 
> >  #define CLK_RECALC_NEW_RATES   BIT(9) /* recalc rates after
> >  notifications */ #define CLK_SET_RATE_UNGATE    BIT(10) /* clock needs
> >  to run to set rate */ #define CLK_IS_CRITICAL                BIT(11)
> >  /* do not gate, ever */> 
> > +#define CLK_KEEP_REQ_RATE      BIT(12) /* keep reqrate on parent rate
> > change */
> s/reqrate/req_rate/

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Doug Anderson]

Hi,

On Mon, May 9, 2016 at 4:40 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> Am Donnerstag, 5. Mai 2016, 17:35:03 schrieb Doug Anderson:
>> Heiko,
>>
>> On Mon, May 2, 2016 at 9:36 AM, Heiko Stuebner <heiko@sntech.de> wrote:
>> > Given a hirarchy of clk1 -> [div] -> clk2, when the rate of clk1 gets
>> > changed, clk2 changes as well as the divider stays the same. There may
>> > be cases where a user of clk2 needs it at a specific rate, so clk2
>> > needs to be readjusted for the changed rate of clk1.
>> >
>> > So if a rate was requested for the clock, and its rate changed during
>> > the underlying rate-change, with this change the clock framework now
>> > tries to readjust the rate back to/near the requested one.
>> >
>> > The whole process is protected by a new clock-flag to not force this
>> > behaviour change onto every clock defined in the ccf.
>>
>> I'm not an expert on CCF details, but presumably you need to be really
>> careful here.  Is there any way you could get an infinite loop here
>> where you ping-pong between two people trying to control their parent
>> clock?  Right now I see mutual recursion between
>> clk_core_set_rate_nolock() and clk_change_rate() and no base case.
>>
>> Specifically if there's a path (because of CLK_SET_RATE_PARENT) where
>> setting a clock rate on "clk2" in your example can cause a rate change
>> of "clk1" I worry that we'll be in trouble.  Maybe a requirement of
>> your patch is that no such path exists?  ...or maybe something in the
>> code prevents this...
>
> This was one of my worries as well, which is why the flag exists in the first
> place, right now offloading the requirement to check for such conflict cases to
> the clock-tree creator.

If that's the case, it needs to be documented somewhere.  Without some
documentation this flag sounds like a flag that everyone would of
course want everywhere.


> I think one possible test to add could be to check for conflicts between
> CLK_SET_RATE_PARENT and this new flag.
>
> Aka in clk-init once encountering the CLK_KEEP_REQRATE, go up through parent
> clocks as long as CLK_SET_RATE_PARENT is set and if we encounter a parent
> with num_children > 1 (aka a shared base-clock, like a PLL on rockchip)
> unset CLK_KEEP_REQRATE, as it would like introduce that ping-pong game.
>
> Hmm, although this test would also fire in cases like Rockchip's fractional
> dividers, where there is the common pll-select-mux+divider and after that
> the mux selecting between that or having the fractional-divider in between
> as well.
>
> I guess it can get hairy to detect such cases sucessfully.

Wouldn't this logic work?

1. Walk up through parents as long as CLK_SET_RATE_PARENT is set.  Add
to "ancestors" list.

2. Walk down through children of "ancestors" as long as
CLK_SET_RATE_PARENT is set in the child.

3. If CLK_KEEP_REQRATE is found in one of those children and we're not
the original clock then it's an error.


Actually, though, it seems like there's a much easier rule.  For now,
just make CLK_SET_RATE_PARENT and CLK_KEEP_REQRATE mutually exclusive.
You can choose to react to your parent's rate or you can choose to
affect your parent's rate, but not both.

In your current patch 3/3 you actually set both, but I don't think you
really need to do you?


>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >
>> >  drivers/clk/clk.c            | 13 +++++++++++--
>> >  include/linux/clk-provider.h |  1 +
>> >  2 files changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> > index 65e0aad..22be369 100644
>> > --- a/drivers/clk/clk.c
>> > +++ b/drivers/clk/clk.c
>> > @@ -1410,6 +1410,9 @@ static struct clk_core
>> > *clk_propagate_rate_change(struct clk_core *core,>
>> >         return fail_clk;
>> >
>> >  }
>> >
>> > +static int clk_core_set_rate_nolock(struct clk_core *core,
>> > +                                   unsigned long req_rate);
>> > +
>> >
>> >  /*
>> >
>> >   * walk down a subtree and set the new rates notifying the rate
>> >   * change on the way
>> >
>> > @@ -1494,6 +1497,12 @@ static void clk_change_rate(struct clk_core
>> > *core)
>> >
>> >         /* handle the new child who might not be in core->children yet
>> >         */
>> >         if (core->new_child)
>> >
>> >                 clk_change_rate(core->new_child);
>> >
>> > +
>> > +       /* handle a changed clock that needs to readjust its rate */
>> > +       if (core->flags & CLK_KEEP_REQ_RATE && core->req_rate
>> > +                                           && core->new_rate !=
>> > old_rate
>> > +                                           && core->new_rate !=
>> > core->req_rate) +               clk_core_set_rate_nolock(core,
>> > core->req_rate);
>>
>> I guess we don't care about errors here?
>>
>> ...maybe (?) we could ignore errors if we validated this rate change
>> with PRE_RATE_CHANGE before attempting to change the parent clock, but
>> I don't see the code doing this unless I missed it.
>
> It's more like what would you want to do in the error/failure cases.
>
> So far I was going by the assumption we're going to change the clock anyway
> and just try to pull marked clocks along, so in the error case it would just
> fall back to the current behaviour.

In the very least it should cause a warning to be printed.  By
introducing a new flag you've changed the expectations and I don't
think a user would expect to fall back to the old behavior without at
least an warning.

I think it still makes sense to pre-validate your change in
PRE_RATE_CHANGE, of course.


-Doug

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Elaine Zhang]

On 05/03/2016 12:36 AM, Heiko Stuebner wrote:
> Given a hirarchy of clk1 -> [div] -> clk2, when the rate of clk1 gets
> changed, clk2 changes as well as the divider stays the same. There may
> be cases where a user of clk2 needs it at a specific rate, so clk2
> needs to be readjusted for the changed rate of clk1.
>
> So if a rate was requested for the clock, and its rate changed during
> the underlying rate-change, with this change the clock framework now
> tries to readjust the rate back to/near the requested one.
>
> The whole process is protected by a new clock-flag to not force this
> behaviour change onto every clock defined in the ccf.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>   drivers/clk/clk.c            | 13 +++++++++++--
>   include/linux/clk-provider.h |  1 +
>   2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 65e0aad..22be369 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1410,6 +1410,9 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
>   	return fail_clk;
>   }
>
> +static int clk_core_set_rate_nolock(struct clk_core *core,
> +				    unsigned long req_rate);
> +
>   /*
>    * walk down a subtree and set the new rates notifying the rate
>    * change on the way
> @@ -1494,6 +1497,12 @@ static void clk_change_rate(struct clk_core *core)
>   	/* handle the new child who might not be in core->children yet */
>   	if (core->new_child)
>   		clk_change_rate(core->new_child);
> +
> +	/* handle a changed clock that needs to readjust its rate */
> +	if (core->flags & CLK_KEEP_REQ_RATE && core->req_rate
> +					    && core->new_rate != old_rate
> +					    && core->new_rate != core->req_rate)
> +		clk_core_set_rate_nolock(core, core->req_rate);
>   }

I tests found a problem, about set the freq order.
e.p:
[VPLL]
     |
     ------ [div] ----- dclk_vop
If I change VPLL freq 148.5M to 594M, dclk_vop freq will changed as:
148.5M->24M->594M->1485.5M.
But we not hope the dclk_vop have a high freq,it will make the system 
crash or make vop not work well.

So if the VPLL is improve the freq, we need to set dclk_vop div first, 
and than set VPLL freq.
If VPLL is reduce the freq, we need to set vpll first,and set dclk_vop div.

This is just a example,for all change parent freq, we need follow this 
operation.
Do you have a better idea for this problem?

>
>   static int clk_core_set_rate_nolock(struct clk_core *core,
> @@ -1529,11 +1538,11 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>   		return -EBUSY;
>   	}
>
> +	core->req_rate = req_rate;
> +
>   	/* change the rates */
>   	clk_change_rate(top);
>
> -	core->req_rate = req_rate;
> -
>   	return ret;
>   }
>
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 0c72204..06f189e 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -33,6 +33,7 @@
>   #define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
>   #define CLK_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
>   #define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
> +#define CLK_KEEP_REQ_RATE	BIT(12) /* keep reqrate on parent rate change */
>
>   struct clk;
>   struct clk_hw;
>

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Heiko Stuebner]

Hi Elaine,

Am Dienstag, 5. Juli 2016, 15:27:30 schrieb Elaine Zhang:
> On 05/03/2016 12:36 AM, Heiko Stuebner wrote:
> > Given a hirarchy of clk1 -> [div] -> clk2, when the rate of clk1 gets
> > changed, clk2 changes as well as the divider stays the same. There may
> > be cases where a user of clk2 needs it at a specific rate, so clk2
> > needs to be readjusted for the changed rate of clk1.
> > 
> > So if a rate was requested for the clock, and its rate changed during
> > the underlying rate-change, with this change the clock framework now
> > tries to readjust the rate back to/near the requested one.
> > 
> > The whole process is protected by a new clock-flag to not force this
> > behaviour change onto every clock defined in the ccf.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >   drivers/clk/clk.c            | 13 +++++++++++--
> >   include/linux/clk-provider.h |  1 +
> >   2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 65e0aad..22be369 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1410,6 +1410,9 @@ static struct clk_core
> > *clk_propagate_rate_change(struct clk_core *core,> 
> >   	return fail_clk;
> >   
> >   }
> > 
> > +static int clk_core_set_rate_nolock(struct clk_core *core,
> > +				    unsigned long req_rate);
> > +
> > 
> >   /*
> >   
> >    * walk down a subtree and set the new rates notifying the rate
> >    * change on the way
> > 
> > @@ -1494,6 +1497,12 @@ static void clk_change_rate(struct clk_core
> > *core)
> > 
> >   	/* handle the new child who might not be in core->children yet */
> >   	if (core->new_child)
> >   	
> >   		clk_change_rate(core->new_child);
> > 
> > +
> > +	/* handle a changed clock that needs to readjust its rate */
> > +	if (core->flags & CLK_KEEP_REQ_RATE && core->req_rate
> > +					    && core->new_rate != old_rate
> > +					    && core->new_rate != core-
>req_rate)
> > +		clk_core_set_rate_nolock(core, core->req_rate);
> > 
> >   }
> 
> I tests found a problem, about set the freq order.
> e.p:
> [VPLL]
> 
>      ------ [div] ----- dclk_vop
> If I change VPLL freq 148.5M to 594M, dclk_vop freq will changed as:
> 148.5M->24M->594M->1485.5M.
> But we not hope the dclk_vop have a high freq,it will make the system
> crash or make vop not work well.
> 
> So if the VPLL is improve the freq, we need to set dclk_vop div first,
> and than set VPLL freq.
> If VPLL is reduce the freq, we need to set vpll first,and set dclk_vop
> div.
> 
> This is just a example,for all change parent freq, we need follow this
> operation.
> Do you have a better idea for this problem?

In general it seems my simplicistic approach only really works for really 
simple clock-setups and thus is likely not really usable for general things. 

For the VPLL on the rk3399 we were discussion a different approach in [0], 
as VPLL usage (aka which vop gets to control it) is likely to complicated to 
have this done in the clock-framework-

Doug wanted to take a look and add some thoughts and I guess he'll just do 
that after the 4th of july celebrations.


Heiko


[0] http://lists.infradead.org/pipermail/linux-rockchip/2016-June/010400.html

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Elaine Zhang]

On 07/06/2016 06:24 AM, Heiko Stuebner wrote:
> Hi Elaine,
>
> Am Dienstag, 5. Juli 2016, 15:27:30 schrieb Elaine Zhang:
>> On 05/03/2016 12:36 AM, Heiko Stuebner wrote:
>>> Given a hirarchy of clk1 -> [div] -> clk2, when the rate of clk1 gets
>>> changed, clk2 changes as well as the divider stays the same. There may
>>> be cases where a user of clk2 needs it at a specific rate, so clk2
>>> needs to be readjusted for the changed rate of clk1.
>>>
>>> So if a rate was requested for the clock, and its rate changed during
>>> the underlying rate-change, with this change the clock framework now
>>> tries to readjust the rate back to/near the requested one.
>>>
>>> The whole process is protected by a new clock-flag to not force this
>>> behaviour change onto every clock defined in the ccf.
>>>
>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>>> ---
>>>
>>>    drivers/clk/clk.c            | 13 +++++++++++--
>>>    include/linux/clk-provider.h |  1 +
>>>    2 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index 65e0aad..22be369 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -1410,6 +1410,9 @@ static struct clk_core
>>> *clk_propagate_rate_change(struct clk_core *core,>
>>>    	return fail_clk;
>>>
>>>    }
>>>
>>> +static int clk_core_set_rate_nolock(struct clk_core *core,
>>> +				    unsigned long req_rate);
>>> +
>>>
>>>    /*
>>>
>>>     * walk down a subtree and set the new rates notifying the rate
>>>     * change on the way
>>>
>>> @@ -1494,6 +1497,12 @@ static void clk_change_rate(struct clk_core
>>> *core)
>>>
>>>    	/* handle the new child who might not be in core->children yet */
>>>    	if (core->new_child)
>>>    	
>>>    		clk_change_rate(core->new_child);
>>>
>>> +
>>> +	/* handle a changed clock that needs to readjust its rate */
>>> +	if (core->flags & CLK_KEEP_REQ_RATE && core->req_rate
>>> +					    && core->new_rate != old_rate
>>> +					    && core->new_rate != core-
>> req_rate)
>>> +		clk_core_set_rate_nolock(core, core->req_rate);
>>>
>>>    }
>>
>> I tests found a problem, about set the freq order.
>> e.p:
>> [VPLL]
>>
>>       ------ [div] ----- dclk_vop
>> If I change VPLL freq 148.5M to 594M, dclk_vop freq will changed as:
>> 148.5M->24M->594M->1485.5M.
>> But we not hope the dclk_vop have a high freq,it will make the system
>> crash or make vop not work well.
>>
>> So if the VPLL is improve the freq, we need to set dclk_vop div first,
>> and than set VPLL freq.
>> If VPLL is reduce the freq, we need to set vpll first,and set dclk_vop
>> div.
>>
>> This is just a example,for all change parent freq, we need follow this
>> operation.
>> Do you have a better idea for this problem?
>
> In general it seems my simplicistic approach only really works for really
> simple clock-setups and thus is likely not really usable for general things.
>
> For the VPLL on the rk3399 we were discussion a different approach in [0],
> as VPLL usage (aka which vop gets to control it) is likely to complicated to
> have this done in the clock-framework-
>
> Doug wanted to take a look and add some thoughts and I guess he'll just do
> that after the 4th of july celebrations.
>
OK, Regardless of the VPLL and vop.
The current software of clock, there are some problems.
e.p:
[GPLL]
        ------ [div] ----- aclk_perilp
If I set GPLL 1200M use "assigned-clock-rate".My be the default div of 
aclk_perilp is 2, GPLL default freq is 600M.
If I set GPLL first,the aclk_perilp freq will changed as 
300M->600M->300M.But aclk_perilp 600M is a unsafety.It will make the 
system crash.
Aclk_perilp is sizeoff at 300M.It not allowed over frequency.

So if I init the PLL freq use "assigned-clock-rate", I need to list it's 
child clk, make sure it's child clk div is enough,make sure the child 
clk freq is below the  sizeoff freq.

Do you have a better idea for this problem?
>
> Heiko
>
>
> [0] http://lists.infradead.org/pipermail/linux-rockchip/2016-June/010400.html
>
>
>
>

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Doug Anderson]

Hi,

On Tue, Jul 5, 2016 at 6:39 PM, Elaine Zhang <zhangqing@rock-chips.com> wrote:
> OK, Regardless of the VPLL and vop.
> The current software of clock, there are some problems.
> e.p:
> [GPLL]
>        ------ [div] ----- aclk_perilp
> If I set GPLL 1200M use "assigned-clock-rate".My be the default div of
> aclk_perilp is 2, GPLL default freq is 600M.
> If I set GPLL first,the aclk_perilp freq will changed as
> 300M->600M->300M.But aclk_perilp 600M is a unsafety.It will make the system
> crash.
> Aclk_perilp is sizeoff at 300M.It not allowed over frequency.
>
> So if I init the PLL freq use "assigned-clock-rate", I need to list it's
> child clk, make sure it's child clk div is enough,make sure the child clk
> freq is below the  sizeoff freq.
>
> Do you have a better idea for this problem?

As I understand it, this is a problem that the CCF has had for a long
time.  I'll probably get shot if our firmware guy ever reads this, but
I could suggest that one sane way to "solve" this is to say that
firmware should prepare the clock tree in such a way that the kernel
doesn't happen to run into this issue.  Honestly I think that's why
we've never happened to run into this before.  ...but even I think
that's pretty ugly.

Another very ugly (and non-landable) way to solve this is to do this
in device tree:

* Set GPLL to something known slow, like 300M
* Set aclk_perilp to 300M / 4 = 75M
* Set GPLL to 1200M
* Poof, aclk_perilp is now magically the rate you want.


Presumably one nice way to "solve" this cleanly is to somehow add the
max clock rate into the code and adjust the divider (or parent) of a
clock to always make sure that it was under the max.  I'm not sure if
you could do this somehow using PRE_RATE_CHANGE notifiers or if you'd
need to touch the CCF itself.  I suspect the later.


-Doug

Re: [RFC PATCH 2/3] clk: adjust clocks to their requested rate after parent changes

[Doug Anderson]

Hi,

On Tue, Jul 5, 2016 at 3:24 PM, Heiko Stuebner <heiko@sntech.de> wrote:
>> I tests found a problem, about set the freq order.
>> e.p:
>> [VPLL]
>>
>>      ------ [div] ----- dclk_vop
>> If I change VPLL freq 148.5M to 594M, dclk_vop freq will changed as:
>> 148.5M->24M->594M->1485.5M.
>> But we not hope the dclk_vop have a high freq,it will make the system
>> crash or make vop not work well.
>>
>> So if the VPLL is improve the freq, we need to set dclk_vop div first,
>> and than set VPLL freq.
>> If VPLL is reduce the freq, we need to set vpll first,and set dclk_vop
>> div.
>>
>> This is just a example,for all change parent freq, we need follow this
>> operation.
>> Do you have a better idea for this problem?
>
> In general it seems my simplicistic approach only really works for really
> simple clock-setups and thus is likely not really usable for general things.
>
> For the VPLL on the rk3399 we were discussion a different approach in [0],
> as VPLL usage (aka which vop gets to control it) is likely to complicated to
> have this done in the clock-framework-
>
> Doug wanted to take a look and add some thoughts and I guess he'll just do
> that after the 4th of july celebrations.

Right.  IMHO you should do something to _really_ make sure that one of
the VOPs is parented on either GPLL or CPLL and then let the other VOP
be in control of VPLL.  How we actually do that is up for debate.  I
still have it on my list of things to do to look at Heiko's response
to my previous proposal, but given that I'm currently backlogged from
the two day holiday and that I'm only a few days away from a week long
vacation (and subsequent backlog), I'm not 100% sure I'll get to it
soon.  :(  I'll do my best, though...

In addition to the link Heiko gave, you could also look at
<https://chromium-review.googlesource.com/#/c/354165/>.  That's also
not fully baked but is another way to possibly tackle the problem.

In general if there is only one clock tree under VPLL then you
shouldn't need to worry too much about things getting out of whack.


-Doug

[RFC PATCH 3/3] clk: rockchip: make rk3399 vop dclks keep their rate on parent rate changes

[Heiko Stuebner]

The rk3399 hdmi phy is supplied by the vpll directly and needs to adapt
that frequency depending on the selected resolution on the hdmi output.
For the hdmi-phy the vpll frequency is supplied unchanged without
any dividers being present there.

The vpll also is one of the sources the general display clock of the
visual output processor (vop) and as it is somewhat special for
display operations possibly also the preferred pll source. Here a divider
is available between the pll-mux and the vop clock, so that this part
can adapt the resulting frequency if needed.

So to keep the vop clock in line with the target rate, set the newly
introduced CLK_KEEP_REQ_RATE flag for the dclk_vop clocks on rk3399.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-rk3399.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 5248726..4462a50 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -277,11 +277,11 @@ static struct rockchip_clk_branch rk3399_uart4_pmu_fracmux __initdata =
 			RK3399_PMU_CLKSEL_CON(5), 8, 2, MFLAGS);
 
 static struct rockchip_clk_branch rk3399_dclk_vop0_fracmux __initdata =
-	MUX(DCLK_VOP0, "dclk_vop0", mux_dclk_vop0_p, CLK_SET_RATE_PARENT,
+	MUX(DCLK_VOP0, "dclk_vop0", mux_dclk_vop0_p, CLK_SET_RATE_PARENT | CLK_KEEP_REQ_RATE,
 			RK3399_CLKSEL_CON(49), 11, 1, MFLAGS);
 
 static struct rockchip_clk_branch rk3399_dclk_vop1_fracmux __initdata =
-	MUX(DCLK_VOP1, "dclk_vop1", mux_dclk_vop1_p, CLK_SET_RATE_PARENT,
+	MUX(DCLK_VOP1, "dclk_vop1", mux_dclk_vop1_p, CLK_SET_RATE_PARENT | CLK_KEEP_REQ_RATE,
 			RK3399_CLKSEL_CON(50), 11, 1, MFLAGS);
 
 static struct rockchip_clk_branch rk3399_pmuclk_wifi_fracmux __initdata =
-- 
2.8.0.rc3

Re: [RFC PATCH 0/3] clk: attempt to keep requested rate on parent changes

[Tomeu Vizoso]

On 05/02/2016 06:36 PM, Heiko Stuebner wrote:
> I remember reading about people discussing that problem in the past, but
> haven't been able to find another approach to it yet [or I'm just blind
> as happens to often].
> 
> Our problem is the following clock structure:
> 
> [anotherPLL]
>     |
>     ------ [div] ----- dclk_vop
>     |
> [   vpll   ] --------- hdmi_phy
> 
> 
> We need to set the vpll dynamically but still want to retain

Guess something is missing here?

> The other option that comes to mind, would be to have a clock-notifier,
> in the drm driver, but calling clk_set_rate from their looks like it
> shouldn't work due to the prepare mutex already being held.
> 
> 
> The whole thing is labeled RFC because while it works for us and solves
> the problem, I'm not sure if I'm overlooking some important aspect or
> am interferring with some other planned approach for that issue.

I like this approach very much and wonder in what cases it wouldn't be
desirable to re-try to achieve the requested rate after a change in an
ancestor clock.

Besides that, do you know already if it would solve the conflicts
described by Doug in the thread below?

http://thread.gmane.org/gmane.linux.kernel/1820653/focus=3298

Thanks,

Tomeu




> Heiko Stuebner (3):
>   clk: fix inconsistent use of req_rate
>   clk: adjust clocks to their requested rate after parent changes
>   clk: rockchip: make rk3399 vop dclks keep their rate on parent rate changes
> 
>  drivers/clk/clk.c                 | 37 +++++++++++++++++++++++++++++--------
>  drivers/clk/rockchip/clk-rk3399.c |  4 ++--
>  include/linux/clk-provider.h      |  1 +
>  3 files changed, 32 insertions(+), 10 deletions(-)
> 

Re: [RFC PATCH 0/3] clk: attempt to keep requested rate on parent changes

[Heiko Stübner]

Hi Tomeu,

Am Donnerstag, 5. Mai 2016, 15:30:32 schrieb Tomeu Vizoso:
> On 05/02/2016 06:36 PM, Heiko Stuebner wrote:
> > I remember reading about people discussing that problem in the past, but
> > haven't been able to find another approach to it yet [or I'm just blind
> > as happens to often].
> > 
> > Our problem is the following clock structure:
> > 
> > [anotherPLL]
> > 
> >     ------ [div] ----- dclk_vop
> > 
> > [   vpll   ] --------- hdmi_phy
> > 
> > 
> > We need to set the vpll dynamically but still want to retain
> 
> Guess something is missing here?

looks like I forgot to finish my thought "... the vpll as regular dclk_vop 
source".


> > The other option that comes to mind, would be to have a clock-notifier,
> > in the drm driver, but calling clk_set_rate from their looks like it
> > shouldn't work due to the prepare mutex already being held.
> > 
> > 
> > The whole thing is labeled RFC because while it works for us and solves
> > the problem, I'm not sure if I'm overlooking some important aspect or
> > am interferring with some other planned approach for that issue.
> 
> I like this approach very much and wonder in what cases it wouldn't be
> desirable to re-try to achieve the requested rate after a change in an
> ancestor clock.

I guess that is mainly me being overly careful and not overlooking if the 
clock-framework can get into any troubled state if all clocks do this, but I 
guess it should just work ;-) .


> Besides that, do you know already if it would solve the conflicts
> described by Doug in the thread below?
> 
> http://thread.gmane.org/gmane.linux.kernel/1820653/focus=3298

I think the big issue on the rk3288 is, who is actually in charge of 
controlling the npll frequency.

On the rk3399 the vpll is supplied unmodified to the hdmiphy, while as a 
dclk_vop source it goes through a divider first. So it's pretty clear that the 
hdmiphy will control the core frequency while other users need to adapt to 
what they get.

On the rk3288 the npll is a possible source to both the hdmi- as well as the 
edp-clock (and some others as well), so when it gets adapted dynamically, wo 
is going to be in charge and who needs to adapt ... and how do we set this 
hierarchy.


Heiko

Re: [RFC PATCH 0/3] clk: attempt to keep requested rate on parent changes

[Doug Anderson]

Hi,

On Thu, May 5, 2016 at 8:07 AM, Heiko St=C3=BCbner <heiko@sntech.de> wrote:
>> Besides that, do you know already if it would solve the conflicts
>> described by Doug in the thread below?
>>
>> http://thread.gmane.org/gmane.linux.kernel/1820653/focus=3D3298
>
> I think the big issue on the rk3288 is, who is actually in charge of
> controlling the npll frequency.
>
> On the rk3399 the vpll is supplied unmodified to the hdmiphy, while as a
> dclk_vop source it goes through a divider first. So it's pretty clear tha=
t the
> hdmiphy will control the core frequency while other users need to adapt t=
o
> what they get.
>
> On the rk3288 the npll is a possible source to both the hdmi- as well as =
the
> edp-clock (and some others as well), so when it gets adapted dynamically,=
 wo
> is going to be in charge and who needs to adapt ... and how do we set thi=
s
> hierarchy.

Yeah, I don't think it fully solves the rk3288 problem, though
probably you'd end up in a better shape than today.

Specifically:

1. It assumes that the last requester of the rate is the one who gets
to specify it.  That's fine in the master-slave relationship that
Heiko describes for rk3399, but not so great for the peer-peer
relationship of NPLL uses on rk3288.  Said another way: on rk3399 it's
clear that HDMI decides the rate and the VOP has to deal with it, but
on rk3288 you've got two equals both wanting their own rate.

2. It's unclear exactly how well the different users of the VOP would
be able to cope with arbitrary rate changes since it really depends on
what's connected on the other end.


In general it seems like the rule for two peers should be that the
first requester should keep its rate and the 2nd requester should just
have to deal with it, unless the first requester added special code to
validate that the new rate was OK.

-Doug