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

[Heiko Stuebner <heiko@sntech.de>]

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/