On 6 September 2013 00:02, Mike Turquette <mturquette@xxxxxxxxxx> wrote: > On Wed, Sep 4, 2013 at 11:01 AM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote: >> On Wednesday 04 of September 2013 10:43:28 Mike Turquette wrote: >>> Quoting Tomasz Figa (2013-09-03 15:36:50) >>> >>> > Hi Chander, >>> > >>> > On Tuesday 03 of September 2013 17:04:28 Chander Kashyap wrote: >>> > > Some platform has provision to change cpu parent clock during >>> > > cpu frequency scaling. This patch series provides a mechanism to >>> > > implement the same using CCF. >>> > > >>> > > Patch1 provides mechanism to migrate to new parent temporarily. >>> > > >>> > > Patch2 updates the user of clk_register_mux and DEFINE_CLK_MUX which >>> > > are modified to add support for clk migration. >>> > > >>> > > Patch3 adds support to Exynos5250 to use the clock parent migration >>> > > feature implemented in CCF. >>> > >>> > I don't really like this approach. A need to change mux setting >>> > temporarily is heavily platform-specific and I don't think it should >>> > be >>> > handled by generic code. >>> >>> I agree with Tomasz. >>> >>> > First of all there are many factor that you would >>> > >>> > have to account for to make this solution generic, such as: >>> > - board specific alternative parents, >>> > - exact moment of parent change, >>> > - some other platform specific conditions, like CPU voltage that must >>> > be> >>> > changed when mux is changed, because it changes CPU frequency, >>> > >>> > - and probably a lot of more factors that only people working with >>> > all >>> > >>> > the platforms supported (and unsupported yet) by Linux. >>> > >>> > I can see at least two solutions for this problem that don't require >>> > changing core code of common clock framework: >>> > >>> > 1) Implementing a special clock type using normal mux ops, but also >>> > registering a notifier for its PRE_RATE_CHANGE and POST_RATE_CHANGE >>> > events to perform parent switching. >>> >>> Creating a custom clock type is the way to go here. It is possible to >>> wrap the mux clk_ops to re-use that code, or just write a custom clock >>> type from scratch. >>> >>> I do not like using the clock rate-change notifiers for this purpose. >>> The notifiers provide hooks to drivers that need to take care around >>> clock transitions. Using the notifiers from within the clock framework >>> indicates poor design. >> >> I was not sure how a .set_parent() from inside a .set_rate() would >> interact with rate setting, so I mentioned notifiers here, but now as I >> think of it, CCF is supposed to be re-entrant, so things should be fine. >> >>> > 2) Using normal mux clock, but registering such notifiers in clock >>> > controller or cpufreq driver. >>> >>> This depends on what the data sheet or reference manual states. If using >>> a temporary parent is a property of the clock programming sequence >>> (e.g. to have a glitch-less transition) then that logic belongs in the >>> clock provider driver (i.e. a custom clock type needs to be created >>> with this logic). >>> >>> However if using a temporary parent is not required for programming the >>> clock, but is instead a requirement of the clock consumer (e.g. a CPU, >>> or some I/O controller) then perhaps putting this logic in that driver >>> is the right way to go. In that case the logic could be explicit: >>> >>> clk_set_parent(clk, temp_parent); >>> clk_set_rate(clk, target_rate); >>> clk_set_parent(clk, target_parent); >>> >>> Or it could implicit with the use of rate-change notifiers. Again the >>> rate-change notifiers exist for clock consumer drivers to use, so this >>> is OK. >>> >>> I have a hunch that the right way to do this is for a custom clock type >>> to be created which simply calls clk_set_parent from within the clock's >>> .set_rate callback, but I'll wait on feedback from Chander on the needs >>> of his platform. >> >> I believe Chander has exactly the same use case for this as we have for >> Exynos 4210 and 4x12. >> >> On these SoCs, CPU frequency is being scaled by reconfiguring PLL, which >> needs to be masked for locking time. To let the CPU operate normally, a >> mux that allows switching CPU clock domain between two PLLs can be >> switched to the other PLL (MPLL) until the main ARM PLL (APLL) starts >> operating again. > > Right, this is the "glitchless" operation I mentioned earlier and it > is not uncommon in PLLs. > >> >> However this issue is not limited to clocks, because there must be also a >> voltage transition involved if the utility PLL has frequency higher than >> the one currently being reconfigured. > > Right, that is an altogether different issue. What I would like to see > is the temporary parent managed by calls to clk_set_parent from within > a custom .set_rate callback. > > As for the voltage scaling, it would be cool to see this working with > the voltage notifier series I posted recently. Both parents of the > PLLs can have their own operating point tables and each transition > would fire off notifiers that scale voltage. Something like this: > > clk_set_rate(pll, rate) > -> enter .set_rate > -> clk_set_parent(pll, temp_parent) > -> temp_parent PRE_RATE_CHANGE notifier is triggered, maybe scale voltage > -> .set_parent callback > -> temp_parent POST_RATE_CHANGE notifier is triggered, maybe scale voltage > -> pll PRE_RATE_CHANGE notifier is triggered, maybe scale voltage > -> .set_rate callback > -> pll POST_RATE_CHANGE notifier is triggered, maybe scale voltage > -> clk_set_parent(pll, original_parent) > -> original_parent PRE_RATE_CHANGE notifier is triggered, maybe > scale voltage > -> .set_parent callback > -> original_parent POST_RATE_CHANGE notifier is triggered, maybe > scale voltage > > There is some inefficiency in calling the notifiers three times, but > the solution is generic. > > I'm sending this from the gmail web interface so hopefully the spacing > is preserved... > Thanks Mike. Looking at the feedback it seems doing this in generic way is not clean enough. I will redesign it within platform and resend. > Regards, > Mike > >> >> Best regards, >> Tomasz >> -- with warm regards, Chander Kashyap -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |