| Message ID | 1462206982-10444-3-git-send-email-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/
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
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
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/
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
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; >
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
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 > > > >
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
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
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;
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(-)