[PATCH 0/3] Add support for Tegra Activity Monitor

[PATCH 0/3] Add support for Tegra Activity Monitor

[Tomeu Vizoso]

Hello,

these patches implement support for setting the rate of the EMC clock based on
stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
memory accesses (among others).

It depends on the following in-flight patches:

* MC driver: http://thread.gmane.org/gmane.linux.ports.tegra/19623
* EMC driver: http://thread.gmane.org/gmane.linux.ports.arm.kernel/365125
* CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1812962

I have pushed a branch here for testing:

http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=actmon

Regards,

Tomeu

Tomeu Vizoso (3):
  of: Add binding for NVIDIA Tegra ACTMON node
  soc/tegra: add support for Tegra Activity Monitor
  ARM: tegra: Add Tegra124 ACTMON support

 .../devicetree/bindings/arm/tegra/actmon.txt       |  26 +
 arch/arm/boot/dts/tegra124.dtsi                    |  11 +
 drivers/soc/tegra/Makefile                         |   1 +
 drivers/soc/tegra/actmon.c                         | 523 +++++++++++++++++++++
 4 files changed, 561 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/actmon.txt
 create mode 100644 drivers/soc/tegra/actmon.c

-- 
1.9.3

[PATCH 3/3] ARM: tegra: Add Tegra124 ACTMON support

[Tomeu Vizoso]

Add device node for the ACTMON block to the Tegra124 device tree.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 arch/arm/boot/dts/tegra124.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index b1ec35e..8bce1b3 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -213,6 +213,17 @@
 		reg = <0x0 0x60007000 0x0 0x1000>;
 	};
 
+	actmon at 0,6000c800 {
+		compatible = "nvidia,tegra124-actmon";
+		reg = <0x0 0x6000c800 0x0 0x400>;
+		interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&tegra_car TEGRA124_CLK_ACTMON>,
+			 <&tegra_car TEGRA124_CLK_EMC>;
+		clock-names = "actmon", "emc";
+		resets = <&tegra_car 119>;
+		reset-names = "actmon";
+	};
+
 	gpio: gpio at 0,6000d000 {
 		compatible = "nvidia,tegra124-gpio", "nvidia,tegra30-gpio";
 		reg = <0x0 0x6000d000 0x0 0x1000>;
-- 
1.9.3

[PATCH 0/3] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On 10/29/2014 11:50 PM, Tomeu Vizoso wrote:
> Hello,
>
> these patches implement support for setting the rate of the EMC clock based on
> stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
> memory accesses (among others).
>
> It depends on the following in-flight patches:
>
> * MC driver: http://thread.gmane.org/gmane.linux.ports.tegra/19623
> * EMC driver: http://thread.gmane.org/gmane.linux.ports.arm.kernel/365125
> * CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1812962
>
> I have pushed a branch here for testing:

I am not too familiar with DVFS, but after going through this series it 
really seems to me that this could use devfreq. In its current form this 
driver mixes control and policy and lacks flexibility, preventing e.g. 
to switch to a performance or power-saving profile. Could you study the 
feasibility of using devfreq for this?

I also wonder if this driver could not be made more flexible generally 
speaking - right now it is hardcoded that you can only control EMC 
frequency with it. I can imagine that we could want to control several 
clocks using the same counter information, and that e.g. a notifier 
block might help with that. But let's keep that for later - whether to 
use devfreq or not seems to be the most important question for now.

[PATCH 0/3] Add support for Tegra Activity Monitor

[Tomeu Vizoso]

On 11/07/2014 10:07 AM, Alexandre Courbot wrote:
> On 10/29/2014 11:50 PM, Tomeu Vizoso wrote:
>> Hello,
>>
>> these patches implement support for setting the rate of the EMC clock based on
>> stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
>> memory accesses (among others).
>>
>> It depends on the following in-flight patches:
>>
>> * MC driver: http://thread.gmane.org/gmane.linux.ports.tegra/19623
>> * EMC driver: http://thread.gmane.org/gmane.linux.ports.arm.kernel/365125
>> * CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1812962
>>
>> I have pushed a branch here for testing:
> 
> I am not too familiar with DVFS, but after going through this series it 
> really seems to me that this could use devfreq. In its current form this 
> driver mixes control and policy and lacks flexibility, preventing e.g. 
> to switch to a performance or power-saving profile. Could you study the 
> feasibility of using devfreq for this?

Yeah, I started writing a devfreq driver, but then I looked in more
detail to the downstream driver and realized that most of the
functionality that devfreq provides overlaps with the hw.

The ACTMON can be configured to fire an interrupt when a set of
thresholds are crossed, similar to the simple-ondemand governor but a
bit more sophisticated. The only functionality of the governors that
isn't covered by the ACTMON hw is determining the new frequency after a
threshold has been crossed, but if we want to retain the flexibility of
the downstream solution, we would need to write a new governor anyway.

I realize that it would be cool to reuse the code in devfreq, but being
able to let the hw sample the counters, calculating averages and
checking if a threshold has been crossed without the cpu having to
intervene gives this SoC quite an edge when compared to its competitors.

Regards,

Tomeu

[PATCH 0/3] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On 11/07/2014 09:35 PM, Tomeu Vizoso wrote:
> On 11/07/2014 10:07 AM, Alexandre Courbot wrote:
>> On 10/29/2014 11:50 PM, Tomeu Vizoso wrote:
>>> Hello,
>>>
>>> these patches implement support for setting the rate of the EMC clock based on
>>> stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
>>> memory accesses (among others).
>>>
>>> It depends on the following in-flight patches:
>>>
>>> * MC driver: http://thread.gmane.org/gmane.linux.ports.tegra/19623
>>> * EMC driver: http://thread.gmane.org/gmane.linux.ports.arm.kernel/365125
>>> * CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1812962
>>>
>>> I have pushed a branch here for testing:
>>
>> I am not too familiar with DVFS, but after going through this series it
>> really seems to me that this could use devfreq. In its current form this
>> driver mixes control and policy and lacks flexibility, preventing e.g.
>> to switch to a performance or power-saving profile. Could you study the
>> feasibility of using devfreq for this?
>
> Yeah, I started writing a devfreq driver, but then I looked in more
> detail to the downstream driver and realized that most of the
> functionality that devfreq provides overlaps with the hw.
>
> The ACTMON can be configured to fire an interrupt when a set of
> thresholds are crossed, similar to the simple-ondemand governor but a
> bit more sophisticated.

I think (after a quick look at devfreq's source) that you can avoid 
polling altogether if you set polling_ms to 0 in your 
devfreq_dev_profile instance. Then it is up to you to call 
update_devfreq() from your custom governor whenever it sees fit.

ACTMON support seems to overlap between being a governor (which reacts 
to ACTMON interrupts and calls update_devfreq() when needed) and part of 
a devfreq_dev_profile (get_dev_status() needs to use the actmon 
counters). If we keep your current design where the driver simply 
controls a clock, you could have the ACTMON driver obtain that clock, 
register its governor, create a non-polling devfreq_dev_profile that 
controls that clock, and just call devfreq_add_device() with both. Then 
we will have the benefit of being able to use ACTMON as well as the 
performance and powersave governors on EMC, and switch policies dynamically.

Another benefit is that you will have a placeholder in the governor to 
implement suspend/resume for the ACTMON IP, in case this becomes 
necessary in the future.

Do you think that would work? Apart from the polling which doesn't seem 
to be an issue, have you seen any other redundancy between devfreq and 
ACTMON?

> The only functionality of the governors that
> isn't covered by the ACTMON hw is determining the new frequency after a
> threshold has been crossed, but if we want to retain the flexibility of
> the downstream solution, we would need to write a new governor anyway.

Yes, and that's fine. Actually if the parameters of the ACTMON governor 
could be fine-tuned via sysfs, that would just be perfect.

> I realize that it would be cool to reuse the code in devfreq, but being
> able to let the hw sample the counters, calculating averages and
> checking if a threshold has been crossed without the cpu having to
> intervene gives this SoC quite an edge when compared to its competitors.

AFAICT we can keep that edge while getting the benefit of using a common 
framework. Also I expect quite some resistance against the merge of this 
driver if it is not using devfreq. You probably can tell us better 
whether it is fit or not, but can you examine my points above and let us 
know what you think? After a quick look, it actually looks quite 
exploitable for our use-case.

Cheers,
Alex.

[PATCH 0/3] Add support for Tegra Activity Monitor

[Terje Bergström]

On 11.11.2014 06:29, Alexandre Courbot wrote:
> I think (after a quick look at devfreq's source) that you can avoid
> polling altogether if you set polling_ms to 0 in your
> devfreq_dev_profile instance. Then it is up to you to call
> update_devfreq() from your custom governor whenever it sees fit.
> 
> ACTMON support seems to overlap between being a governor (which reacts
> to ACTMON interrupts and calls update_devfreq() when needed) and part of
> a devfreq_dev_profile (get_dev_status() needs to use the actmon
> counters). If we keep your current design where the driver simply
> controls a clock, you could have the ACTMON driver obtain that clock,
> register its governor, create a non-polling devfreq_dev_profile that
> controls that clock, and just call devfreq_add_device() with both. Then
> we will have the benefit of being able to use ACTMON as well as the
> performance and powersave governors on EMC, and switch policies
> dynamically.

Another way to use it is that governor is just a governor. It takes in
load values and generates new target frequencies, and doesn't know
anything about HW.

Device profile is the one that enables threshold interrupts and disables
polling. Device profile receives the interrupt, retrieves new load
number from hardware, and calls update_devfreq().

This way we keep all HW specific code in device profile, and there's
potential to use a generic governor instead of writing your own.

[PATCH 0/3] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On 11/11/2014 03:57 PM, Terje Bergstr?m wrote:
> On 11.11.2014 06:29, Alexandre Courbot wrote:
>> I think (after a quick look at devfreq's source) that you can avoid
>> polling altogether if you set polling_ms to 0 in your
>> devfreq_dev_profile instance. Then it is up to you to call
>> update_devfreq() from your custom governor whenever it sees fit.
>>
>> ACTMON support seems to overlap between being a governor (which reacts
>> to ACTMON interrupts and calls update_devfreq() when needed) and part of
>> a devfreq_dev_profile (get_dev_status() needs to use the actmon
>> counters). If we keep your current design where the driver simply
>> controls a clock, you could have the ACTMON driver obtain that clock,
>> register its governor, create a non-polling devfreq_dev_profile that
>> controls that clock, and just call devfreq_add_device() with both. Then
>> we will have the benefit of being able to use ACTMON as well as the
>> performance and powersave governors on EMC, and switch policies
>> dynamically.
>
> Another way to use it is that governor is just a governor. It takes in
> load values and generates new target frequencies, and doesn't know
> anything about HW.
>
> Device profile is the one that enables threshold interrupts and disables
> polling. Device profile receives the interrupt, retrieves new load
> number from hardware, and calls update_devfreq().
>
> This way we keep all HW specific code in device profile, and there's
> potential to use a generic governor instead of writing your own.

I see several obstacles with this approach:

1) update_devfreq() is a governor private function (defined in 
drivers/devfreq/governor.h) and it would need to be called from the 
device profile.

2) devfreq events (start monitoring, stop monitoring, suspend, resume, 
...) are processed by the governor. So it would still need access to the 
actmon registers to honor these requests.

3) simple monitors like performance and powersave set the frequency (max 
or min) once and for all when they are started, and don't need to be 
called again afterwards. But this is probably what will happen if you 
let the devfreq_dev_profile handle the ACTMON registers, since it can 
make no assumption on when the governor expects to be invoked.

It would be good to have the feedback of devfreq's maintainers about 
this (added them). How could an IP capable of monitoring activity 
through counters and firing interrupts when these counters reach a 
certain threshold be integrated nicely into devfreq? The functions seem 
to overlap between the governor (reacting to specific events, in this 
case ACTMON interrupts) and dev_profile (querying current status through 
the counters), and it seems difficult to come with a design where the 
governor and dev_profile are not tightly intertwined.

[PATCH 0/3] Add support for Tegra Activity Monitor

[Tomeu Vizoso]

On 11 November 2014 05:29, Alexandre Courbot <acourbot@nvidia.com> wrote:
> On 11/07/2014 09:35 PM, Tomeu Vizoso wrote:
>>
>> On 11/07/2014 10:07 AM, Alexandre Courbot wrote:
>>>
>>> On 10/29/2014 11:50 PM, Tomeu Vizoso wrote:
>>>>
>>>> Hello,
>>>>
>>>> these patches implement support for setting the rate of the EMC clock
>>>> based on
>>>> stats collected from the ACTMON, a piece of hw in the Tegra124 that
>>>> counts
>>>> memory accesses (among others).
>>>>
>>>> It depends on the following in-flight patches:
>>>>
>>>> * MC driver: http://thread.gmane.org/gmane.linux.ports.tegra/19623
>>>> * EMC driver:
>>>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/365125
>>>> * CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1812962
>>>>
>>>> I have pushed a branch here for testing:
>>>
>>>
>>> I am not too familiar with DVFS, but after going through this series it
>>> really seems to me that this could use devfreq. In its current form this
>>> driver mixes control and policy and lacks flexibility, preventing e.g.
>>> to switch to a performance or power-saving profile. Could you study the
>>> feasibility of using devfreq for this?
>>
>>
>> Yeah, I started writing a devfreq driver, but then I looked in more
>> detail to the downstream driver and realized that most of the
>> functionality that devfreq provides overlaps with the hw.
>>
>> The ACTMON can be configured to fire an interrupt when a set of
>> thresholds are crossed, similar to the simple-ondemand governor but a
>> bit more sophisticated.
>
>
> I think (after a quick look at devfreq's source) that you can avoid polling
> altogether if you set polling_ms to 0 in your devfreq_dev_profile instance.
> Then it is up to you to call update_devfreq() from your custom governor
> whenever it sees fit.
>
> ACTMON support seems to overlap between being a governor (which reacts to
> ACTMON interrupts and calls update_devfreq() when needed) and part of a
> devfreq_dev_profile (get_dev_status() needs to use the actmon counters). If
> we keep your current design where the driver simply controls a clock, you
> could have the ACTMON driver obtain that clock, register its governor,
> create a non-polling devfreq_dev_profile that controls that clock, and just
> call devfreq_add_device() with both. Then we will have the benefit of being
> able to use ACTMON as well as the performance and powersave governors on
> EMC, and switch policies dynamically.
>
> Another benefit is that you will have a placeholder in the governor to
> implement suspend/resume for the ACTMON IP, in case this becomes necessary
> in the future.
>
> Do you think that would work? Apart from the polling which doesn't seem to
> be an issue, have you seen any other redundancy between devfreq and ACTMON?

I don't think there's any obstacle to having this as a devfreq driver,
it's just that it won't use much/any functionality of it so I don't
really see the point.

I think it will be better if I send a version of the driver that uses
the devfreq framework, so we can better compare (and maybe I will
change my opinion).

Regards,

Tomeu