On Sat, Feb 1, 2014 at 9:40 AM, Mike Turquette <mturquette@xxxxxxxxxx> wrote:
> Quoting Heiko Stübner (2014-01-30 07:09:04)
>> On Thursday, 30. January 2014 18:23:44 Thomas Abraham wrote:
>> > Hi Mike,
>> >
>> > On Wed, Jan 29, 2014 at 12:17 AM, Mike Turquette <mturquette@xxxxxxxxxx>
>> wrote:
>> > > On Mon, Jan 27, 2014 at 9:30 PM, Thomas Abraham <ta.omasab@xxxxxxxxx>
>> wrote:
>> > >> Hi Mike,
>> > >>
>> > >> On Tue, Jan 28, 2014 at 1:55 AM, Mike Turquette <mturquette@xxxxxxxxxx>
>> wrote:
>> > >>> Quoting Thomas Abraham (2014-01-18 04:10:51)
>> > >>>
>> > > As far as I can tell
>> > > the remux does not happen because it is necessary to generate the
>> > > required clock rate, but because we don't want to run the ARM core out
>> > > of spec for a short time while the PLL relocks. Assuming I have that
>> > > part of it right, I prefer for the parent mux operation to be a part
>> > > of the CPUfreq driver's .target callback instead of hidden away in the
>> > > clock driver.
>> >
>> > The re-parenting is mostly done to keep the ARM CPU clocked while the
>> > PLL is stopped, reprogrammed and restarted. One of the side effects of
>> > that is, the clock speed of the temporary parent could be higher then
>> > what is allowed due to the ARM voltage at the time of re-parenting.
>> > That is the reason to use the safe voltage.
>>
>> The Rockchip-SoCs use something similar, so I'm following quite closely what
>> Thomas is trying to do here, as similar solution would also solve this issue
>> for me.
>>
>> On some Rockchip-SoCs even stuff like pclk and hclk seems to be sourced from
>> the divided armclk, creating additional constraints.
>>
>> But on the RKs (at least in the upstream sources) the armclk is simply equal
>> to the pll output. A divider exists, but is only used to make sure that the
>> armclk stays below the original rate when sourced from the temp-parent, like
>>
>> if (clk_get_rate(temp_parent) > clk_get_rate(main_parent)
>> set_divider(something so that rate(temp) <= rate(main)
>> clk_set_parent(...)
>>
>> Isn't there a similar possiblity on your platform, as it would remove the need
>> for the safe-voltage?
>>
>>
>> In general I also like the approach of hiding the rate-change logic inside
>> this composite clock, as the depending clocks can be easily kept in sync. As
>> with the Rockchips the depending clocks are different for each of the three
>> Cortex-A9 SoCs I looked at, it would be "interesting" if all of this would
>> need to be done in a cpufreq driver.
>
> I wonder if hiding these details inside of the composite clock
> implementation indicates the lack of some needed feature in the clk
> core? I've discussed the idea of "coordinated rate changes" before. E.g:
>
> _________________________________________________________
> | clk | opp-low | opp-mid | opp-fast |
> | | | | |
> |pll | 300000 | 600000 | 600000 |
> | | | | |
> |div | 150000 | 300000 | 600000 |
> | | | | |
> |mpu_clk| 150000 | 300000 | 600000 |
> | | | | |
> |periph | 150000 | 150000 | 300000 |
> ---------------------------------------------------------
>
> A call to clk_set_rate() against any of those clocks will result in all
> of their dividers being updated. At the implementation level this might
> look something like this extremely simplified pseudocode:
>
> int clk_set_rate(struct clk* clk, unsigned long rate)
> {
> /* trap clks that support coordinated rate changes */
> if (clk->ops->coordinate_rate)
> return clk->ops->coordinate_rate(clk->hw, rate);
> ...
> }
>
> and,
>
> struct coord_rates {
> struct clk_hw *hw;
> struct clk *parent;
> struct clk *rate;
> };
>
> and in the clock driver,
>
> #define PLL 0
> #define DIV 1
> #define MPU 2
> #define PER 3
>
> #define NR_OPP 4
> #define NR_CLK 4
>
> struct coord_rates my_opps[NR_OPP][NR_CLK]; // populated from DT data
>
> int my_coordinate_rate_callback(struct clk_hw *hw, unsigned long rate)
> {
> struct coord_rate **selected_opp;
>
> for(i = 0; i < NR_OPP; i++) {
> for(j = 0; j < NR_CLK; j++) {
> if (my_opps[i][j]->hw == hw &&
> my_opps[i][j]->rate == rate)
> selected_opp = my_opps[i];
> break;
> }
> }
>
> /*
> * order of operations is specific to my hardware and should be
> * managed by my clock driver, not generic code
> */
>
> __clk_set_parent(selected_opp[DIV]->hw, temp_parent);
> __clk_set_rate(selected_opp[PLL]->hw, selected_opp[PLL]->rate);
> __clk_set_parent(selected_opp[DIV]->hw,
> selected_opp[PLL]->hw->clk);
> ...
>
> /*
> * note that the above could be handled by a switch-case or
> * something else
> */
> }
>
> Thoughts? Please forgive any gaps in my logic or abuse of C.
>
> I have long thought that something like the above would someday go into
> a generic dvfs layer instead of the clock framework, but maybe starting
> with the clk framework makes more sense?
Hi Mike,
Yes, this will be very helpful for atomically controlling the rates of
a group of clocks. This coordinated rate change method can be used
during the armclk rate changes on Samsung platforms.
Thanks,
Thomas.
>
> Regards,
> Mike
>
>>
>>
>> Heiko
>>
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
