On 04/11/2015 12:11 AM, Michael Turquette wrote:
> Quoting Thierry Reding (2015-03-11 03:07:43)
>> Hi Mike,
>>
>> Have you had a chance to look at these changes to the Tegra clock
>> driver? If you're fine with it, I'd like to take these patches through
>> the Tegra tree because the rest of the series depends on them. I can
>> provide a stable branch in case we need to base other Tegra clock
>> changes on top of this.
>
> Hi Thierry,
>
> Clock patches (and corresponding DT binding descriptions and changes to
> DTS) look fine to me. Please add:
>
> Acked-by: Michael Turquette <[email protected]>
>
> I did have a question about the beahvior of clk_put in one of Mikko's
> patches but it should not gate this series. I'm just trying to find out
> if we have a bug in the framework or if the Tegra driver is a special
> case.
>
> Also I do not think a stable branch is necessary.
>
> Regards,
> Mike
>
Looks like in the meantime, this has been partially broken by
03bc10ab5b0f "clk: check ->determine/round_rate() return value in
clk_calc_new_rates". The highest rates supported by the DFLL clock have
1 in the MSB, so those cannot be entered after the aforementioned patch,
as the return value of round_rate is interpreted as an error. Avenues
that I can see: 1) revert the above patch 2) restrict the cpu clock rate
to those with 0 in the MSB 3) move to 64-bit clock rates.
Cheers,
Mikko.
>>
>> Thierry
>>
>> On Sun, Mar 01, 2015 at 02:44:23PM +0200, Mikko Perttunen wrote:
>>> Hi, this is v8 of the Tegra124 cpufreq series. Aside rebasing on latest -next,
>>> the following changes have been done:
>>>
>>> clk: tegra: Add binding for the Tegra124 DFLL clocksource
>>> - Changed dfll@ -> clock@
>>> - Changed compatibility string to "nvidia,tegra124-dfll"
>>> - Clarified how the vdd-cpu-supply property is used
>>> - Marked nvidia,cg-scale as optional since it is a boolean property
>>> - Expanded the 'FS mode' term to 'full speed mode'
>>> - Added dvco reset control
>>>
>>> ARM: tegra: Add the DFLL to Tegra124 device tree
>>> - Changed dfll@ -> clock@
>>> - Added dvco reset control
>>>
>>> ARM: tegra: Enable the DFLL on the Jetson TK1
>>> - Changed dfll@ -> clock@
>>>
>>> clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent
>>> - Added
>>>
>>> clk: tegra: Introduce ability for SoC-specific reset control callbacks
>>> - Added
>>>
>>> clk: tegra: Add DFLL DVCO reset control for Tegra124
>>> - Changed to use SoC-specific reset control callback
>>>
>>> clk: tegra: Add Tegra124 DFLL clocksource platform driver
>>> - Don't set DVCO reset handlers
>>>
>>> clk: tegra: Add library for the DFLL clock source (open-loop mode)
>>> - Use reset control instead of function pointers
>>>
>>> also added acks from v7.
>>>
>>> The series is available in a git repository at
>>> git://github.com/cyndis/linux.git cldvfs-v8
>>>
>>> Tested by me on Jetson-TK1 (rev. D).
>>>
>>> Original cover letter:
>>>
>>> This series implements the DFLL/CL-DVFS clock source for the fast CPU
>>> cluster on Tegra124, and a cpufreq driver that uses the DFLL for
>>> clocking the CPU. Most of this is based on Paul Walmsley's public patch
>>> set from December 2013, which is available at
>>> http://comments.gmane.org/gmane.linux.ports.tegra/15273
>>>
>>> The DFLL clock hardware is a voltage-controlled oscillator plus
>>> control logic that compares the generated output clock with a
>>> 51 MHz reference clock, and can make decisions to either lower
>>> or raise the DFLL voltage to keep the output rate close to the
>>> software-requested rate. The voltage changes are done by
>>> communicating with an off-chip PMIC via either I2C or PWM.
>>> As the DFLL oscillator is powered via the CPU rail, using
>>> the DFLL as the CPU clocksource also gives us dynamic CPU
>>> voltage scaling.
>>>
>>> This series has been tested on the Jetson TK1 (Rev C). Porting this to
>>> the Venice2 should be simple, though do note that it does not have
>>> active cooling.
>>>
>>> Thanks,
>>> Tuomas
>>>
>>> Mikko Perttunen (3):
>>> clk: tegra: Introduce ability for SoC-specific reset control callbacks
>>> clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent
>>> ARM: tegra: Add CPU regulator to the Jetson TK1 device tree
>>>
>>> Paul Walmsley (1):
>>> clk: tegra: Add DFLL DVCO reset control for Tegra124
>>>
>>> Tuomas Tynkkynen (14):
>>> clk: tegra: Add binding for the Tegra124 DFLL clocksource
>>> clk: tegra: Add library for the DFLL clock source (open-loop mode)
>>> clk: tegra: Add closed loop support for the DFLL
>>> clk: tegra: Add functions for parsing CVB tables
>>> clk: tegra: Add Tegra124 DFLL clocksource platform driver
>>> clk: tegra: Save/restore CCLKG_BURST_POLICY on suspend
>>> clk: tegra: Add the DFLL as a possible parent of the cclk_g clock
>>> ARM: tegra: Add the DFLL to Tegra124 device tree
>>> ARM: tegra: Enable the DFLL on the Jetson TK1
>>> cpufreq: tegra124: Add device tree bindings
>>> cpufreq: tegra: Rename tegra-cpufreq to tegra20-cpufreq
>>> cpufreq: Add cpufreq driver for Tegra124
>>> ARM: tegra: Add entries for cpufreq on Tegra124
>>> ARM: tegra: enable Tegra124 cpufreq driver by default
>>>
>>> .../bindings/clock/nvidia,tegra124-dfll.txt | 79 +
>>> .../bindings/cpufreq/tegra124-cpufreq.txt | 44 +
>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts | 15 +-
>>> arch/arm/boot/dts/tegra124.dtsi | 34 +
>>> arch/arm/configs/tegra_defconfig | 1 +
>>> arch/arm/mach-tegra/Kconfig | 1 +
>>> drivers/clk/tegra/Makefile | 3 +
>>> drivers/clk/tegra/clk-dfll.c | 1755 ++++++++++++++++++++
>>> drivers/clk/tegra/clk-dfll.h | 54 +
>>> drivers/clk/tegra/clk-tegra-super-gen4.c | 50 +-
>>> drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 163 ++
>>> drivers/clk/tegra/clk-tegra124.c | 82 +
>>> drivers/clk/tegra/clk.c | 36 +-
>>> drivers/clk/tegra/clk.h | 3 +
>>> drivers/clk/tegra/cvb.c | 133 ++
>>> drivers/clk/tegra/cvb.h | 67 +
>>> drivers/cpufreq/Kconfig.arm | 13 +-
>>> drivers/cpufreq/Makefile | 3 +-
>>> drivers/cpufreq/tegra124-cpufreq.c | 217 +++
>>> .../cpufreq/{tegra-cpufreq.c => tegra20-cpufreq.c} | 0
>>> include/dt-bindings/reset/tegra124-car.h | 11 +
>>> 21 files changed, 2730 insertions(+), 34 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
>>> create mode 100644 Documentation/devicetree/bindings/cpufreq/tegra124-cpufreq.txt
>>> create mode 100644 drivers/clk/tegra/clk-dfll.c
>>> create mode 100644 drivers/clk/tegra/clk-dfll.h
>>> create mode 100644 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
>>> create mode 100644 drivers/clk/tegra/cvb.c
>>> create mode 100644 drivers/clk/tegra/cvb.h
>>> create mode 100644 drivers/cpufreq/tegra124-cpufreq.c
>>> rename drivers/cpufreq/{tegra-cpufreq.c => tegra20-cpufreq.c} (100%)
>>> create mode 100644 include/dt-bindings/reset/tegra124-car.h
>>>
>>> --
>>> 2.3.0
>>>
Hi Mikko,
On Tue, 14 Apr 2015 14:25:59 +0300
Mikko Perttunen <[email protected]> wrote:
> On 04/11/2015 12:11 AM, Michael Turquette wrote:
> > Quoting Thierry Reding (2015-03-11 03:07:43)
> >> Hi Mike,
> >>
> >> Have you had a chance to look at these changes to the Tegra clock
> >> driver? If you're fine with it, I'd like to take these patches through
> >> the Tegra tree because the rest of the series depends on them. I can
> >> provide a stable branch in case we need to base other Tegra clock
> >> changes on top of this.
> >
> > Hi Thierry,
> >
> > Clock patches (and corresponding DT binding descriptions and changes to
> > DTS) look fine to me. Please add:
> >
> > Acked-by: Michael Turquette <[email protected]>
> >
> > I did have a question about the beahvior of clk_put in one of Mikko's
> > patches but it should not gate this series. I'm just trying to find out
> > if we have a bug in the framework or if the Tegra driver is a special
> > case.
> >
> > Also I do not think a stable branch is necessary.
> >
> > Regards,
> > Mike
> >
>
> Looks like in the meantime, this has been partially broken by
> 03bc10ab5b0f "clk: check ->determine/round_rate() return value in
> clk_calc_new_rates". The highest rates supported by the DFLL clock have
> 1 in the MSB, so those cannot be entered after the aforementioned patch,
> as the return value of round_rate is interpreted as an error. Avenues
> that I can see: 1) revert the above patch 2) restrict the cpu clock rate
> to those with 0 in the MSB 3) move to 64-bit clock rates.
How about changing ->determine_rate() and ->round_rate() prototypes so
that they always return 0 or an error code and passing the adjusted_rate
as an argument ?
Something like that:
int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long *parent_rate);
int (*determine_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long min_rate,
unsigned long max_rate,
unsigned long *best_parent_rate,
struct clk_hw **best_parent_hw);
I know this implies a lot of changes (in all clock drivers and in the
core infrastructure), but I really think we should not mix error codes
and clock frequencies (even if we decide to move to a 64 bits rate
approach).
Anyway, IMHO the only alternative to this solution is solution #3,
because #1 implies re-introducing another bug where
->round_rate()/->determine_rate() are silently ignored, and #2 implies
lying about your DFLL capabilities.
Mike, what's your opinion ?
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On 04/14/2015 08:21 PM, Boris Brezillon wrote:
> Hi Mikko,
>
> On Tue, 14 Apr 2015 14:25:59 +0300
> Mikko Perttunen <[email protected]> wrote:
>
>> On 04/11/2015 12:11 AM, Michael Turquette wrote:
>>> Quoting Thierry Reding (2015-03-11 03:07:43)
>>>> Hi Mike,
>>>>
>>>> Have you had a chance to look at these changes to the Tegra clock
>>>> driver? If you're fine with it, I'd like to take these patches through
>>>> the Tegra tree because the rest of the series depends on them. I can
>>>> provide a stable branch in case we need to base other Tegra clock
>>>> changes on top of this.
>>>
>>> Hi Thierry,
>>>
>>> Clock patches (and corresponding DT binding descriptions and changes to
>>> DTS) look fine to me. Please add:
>>>
>>> Acked-by: Michael Turquette <[email protected]>
>>>
>>> I did have a question about the beahvior of clk_put in one of Mikko's
>>> patches but it should not gate this series. I'm just trying to find out
>>> if we have a bug in the framework or if the Tegra driver is a special
>>> case.
>>>
>>> Also I do not think a stable branch is necessary.
>>>
>>> Regards,
>>> Mike
>>>
>>
>> Looks like in the meantime, this has been partially broken by
>> 03bc10ab5b0f "clk: check ->determine/round_rate() return value in
>> clk_calc_new_rates". The highest rates supported by the DFLL clock have
>> 1 in the MSB, so those cannot be entered after the aforementioned patch,
>> as the return value of round_rate is interpreted as an error. Avenues
>> that I can see: 1) revert the above patch 2) restrict the cpu clock rate
>> to those with 0 in the MSB 3) move to 64-bit clock rates.
>
> How about changing ->determine_rate() and ->round_rate() prototypes so
> that they always return 0 or an error code and passing the adjusted_rate
> as an argument ?
>
> Something like that:
>
> int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
> unsigned long *parent_rate);
> int (*determine_rate)(struct clk_hw *hw, unsigned long *rate,
> unsigned long min_rate,
> unsigned long max_rate,
> unsigned long *best_parent_rate,
> struct clk_hw **best_parent_hw);
>
> I know this implies a lot of changes (in all clock drivers and in the
> core infrastructure), but I really think we should not mix error codes
> and clock frequencies (even if we decide to move to a 64 bits rate
> approach).
This sounds like a good idea, too.
>
> Anyway, IMHO the only alternative to this solution is solution #3,
> because #1 implies re-introducing another bug where
> ->round_rate()/->determine_rate() are silently ignored, and #2 implies
> lying about your DFLL capabilities.
>
Yeah, #1 and #2 weren't really meant as realistic options to end up with :)
> Mike, what's your opinion ?
>
> Best Regards,
>
> Boris
>
Cheers,
Mikko.
Quoting Mikko Perttunen (2015-04-14 12:40:36)
> On 04/14/2015 08:21 PM, Boris Brezillon wrote:
> > Hi Mikko,
> >
> > On Tue, 14 Apr 2015 14:25:59 +0300
> > Mikko Perttunen <[email protected]> wrote:
> >
> >> On 04/11/2015 12:11 AM, Michael Turquette wrote:
> >>> Quoting Thierry Reding (2015-03-11 03:07:43)
> >>>> Hi Mike,
> >>>>
> >>>> Have you had a chance to look at these changes to the Tegra clock
> >>>> driver? If you're fine with it, I'd like to take these patches through
> >>>> the Tegra tree because the rest of the series depends on them. I can
> >>>> provide a stable branch in case we need to base other Tegra clock
> >>>> changes on top of this.
> >>>
> >>> Hi Thierry,
> >>>
> >>> Clock patches (and corresponding DT binding descriptions and changes to
> >>> DTS) look fine to me. Please add:
> >>>
> >>> Acked-by: Michael Turquette <[email protected]>
> >>>
> >>> I did have a question about the beahvior of clk_put in one of Mikko's
> >>> patches but it should not gate this series. I'm just trying to find out
> >>> if we have a bug in the framework or if the Tegra driver is a special
> >>> case.
> >>>
> >>> Also I do not think a stable branch is necessary.
> >>>
> >>> Regards,
> >>> Mike
> >>>
> >>
> >> Looks like in the meantime, this has been partially broken by
> >> 03bc10ab5b0f "clk: check ->determine/round_rate() return value in
> >> clk_calc_new_rates". The highest rates supported by the DFLL clock have
> >> 1 in the MSB, so those cannot be entered after the aforementioned patch,
> >> as the return value of round_rate is interpreted as an error. Avenues
> >> that I can see: 1) revert the above patch 2) restrict the cpu clock rate
> >> to those with 0 in the MSB 3) move to 64-bit clock rates.
> >
> > How about changing ->determine_rate() and ->round_rate() prototypes so
> > that they always return 0 or an error code and passing the adjusted_rate
> > as an argument ?
> >
> > Something like that:
> >
> > int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
> > unsigned long *parent_rate);
> > int (*determine_rate)(struct clk_hw *hw, unsigned long *rate,
> > unsigned long min_rate,
> > unsigned long max_rate,
> > unsigned long *best_parent_rate,
> > struct clk_hw **best_parent_hw);
> >
> > I know this implies a lot of changes (in all clock drivers and in the
> > core infrastructure), but I really think we should not mix error codes
> > and clock frequencies (even if we decide to move to a 64 bits rate
> > approach).
>
> This sounds like a good idea, too.
I've had this idea as well, which is to never return rates but only
error codes, and rates are passed by reference like in your example
above. Clearly the *best_parent_rate stuff already functions this way.
Would be cool to use a programming language that supported complex
return types ;-)
>
> >
> > Anyway, IMHO the only alternative to this solution is solution #3,
> > because #1 implies re-introducing another bug where
> > ->round_rate()/->determine_rate() are silently ignored, and #2 implies
> > lying about your DFLL capabilities.
> >
>
> Yeah, #1 and #2 weren't really meant as realistic options to end up with :)
Yes, seems that we're heading towards #3. In the mean time option #1.5
(the one where we change the round_rate/determine_rate semantics) is
probably a good idea and can resolve this issue in the shorter term
compared to signed 64-bit rates (and will be necessary anyhow if we use
unsigned 64-bit rates).
I'll add this to the high priority todo list since the Tegra EMC stuff
won't go for 4.1 but will very likely go for 4.2.
Regards,
Mike
>
> > Mike, what's your opinion ?
> >
> > Best Regards,
> >
> > Boris
> >
>
> Cheers,
> Mikko.
>
On 04/15/2015 12:06 AM, Michael Turquette wrote:
> Quoting Mikko Perttunen (2015-04-14 12:40:36)
>> On 04/14/2015 08:21 PM, Boris Brezillon wrote:
>>> Hi Mikko,
>>>
>>> On Tue, 14 Apr 2015 14:25:59 +0300
>>> Mikko Perttunen <[email protected]> wrote:
>>>
>>>> On 04/11/2015 12:11 AM, Michael Turquette wrote:
>>>>> Quoting Thierry Reding (2015-03-11 03:07:43)
>>>>>> Hi Mike,
>>>>>>
>>>>>> Have you had a chance to look at these changes to the Tegra clock
>>>>>> driver? If you're fine with it, I'd like to take these patches through
>>>>>> the Tegra tree because the rest of the series depends on them. I can
>>>>>> provide a stable branch in case we need to base other Tegra clock
>>>>>> changes on top of this.
>>>>>
>>>>> Hi Thierry,
>>>>>
>>>>> Clock patches (and corresponding DT binding descriptions and changes to
>>>>> DTS) look fine to me. Please add:
>>>>>
>>>>> Acked-by: Michael Turquette <[email protected]>
>>>>>
>>>>> I did have a question about the beahvior of clk_put in one of Mikko's
>>>>> patches but it should not gate this series. I'm just trying to find out
>>>>> if we have a bug in the framework or if the Tegra driver is a special
>>>>> case.
>>>>>
>>>>> Also I do not think a stable branch is necessary.
>>>>>
>>>>> Regards,
>>>>> Mike
>>>>>
>>>>
>>>> Looks like in the meantime, this has been partially broken by
>>>> 03bc10ab5b0f "clk: check ->determine/round_rate() return value in
>>>> clk_calc_new_rates". The highest rates supported by the DFLL clock have
>>>> 1 in the MSB, so those cannot be entered after the aforementioned patch,
>>>> as the return value of round_rate is interpreted as an error. Avenues
>>>> that I can see: 1) revert the above patch 2) restrict the cpu clock rate
>>>> to those with 0 in the MSB 3) move to 64-bit clock rates.
>>>
>>> How about changing ->determine_rate() and ->round_rate() prototypes so
>>> that they always return 0 or an error code and passing the adjusted_rate
>>> as an argument ?
>>>
>>> Something like that:
>>>
>>> int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
>>> unsigned long *parent_rate);
>>> int (*determine_rate)(struct clk_hw *hw, unsigned long *rate,
>>> unsigned long min_rate,
>>> unsigned long max_rate,
>>> unsigned long *best_parent_rate,
>>> struct clk_hw **best_parent_hw);
>>>
>>> I know this implies a lot of changes (in all clock drivers and in the
>>> core infrastructure), but I really think we should not mix error codes
>>> and clock frequencies (even if we decide to move to a 64 bits rate
>>> approach).
>>
>> This sounds like a good idea, too.
>
> I've had this idea as well, which is to never return rates but only
> error codes, and rates are passed by reference like in your example
> above. Clearly the *best_parent_rate stuff already functions this way.
> Would be cool to use a programming language that supported complex
> return types ;-)
Algebraic data type pipe dreams.. :)
>
>>
>>>
>>> Anyway, IMHO the only alternative to this solution is solution #3,
>>> because #1 implies re-introducing another bug where
>>> ->round_rate()/->determine_rate() are silently ignored, and #2 implies
>>> lying about your DFLL capabilities.
>>>
>>
>> Yeah, #1 and #2 weren't really meant as realistic options to end up with :)
>
> Yes, seems that we're heading towards #3. In the mean time option #1.5
> (the one where we change the round_rate/determine_rate semantics) is
> probably a good idea and can resolve this issue in the shorter term
> compared to signed 64-bit rates (and will be necessary anyhow if we use
> unsigned 64-bit rates).
>
> I'll add this to the high priority todo list since the Tegra EMC stuff
> won't go for 4.1 but will very likely go for 4.2.
Wonderful :)
Thanks,
Mikko
>
> Regards,
> Mike
>
>>
>>> Mike, what's your opinion ?
>>>
>>> Best Regards,
>>>
>>> Boris
>>>
>>
>> Cheers,
>> Mikko.
>>