2014-10-29 14:51:24

by Tomeu Vizoso

[permalink] [raw]
Subject: [PATCH 0/3] Add support for Tegra Activity Monitor

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


2014-10-29 14:51:31

by Tomeu Vizoso

[permalink] [raw]
Subject: [PATCH 1/3] of: Add binding for NVIDIA Tegra ACTMON node

This block gathers statistics about various counters and can be configured to
fire interrupts when thresholds are crossed.

Signed-off-by: Tomeu Vizoso <[email protected]>
---
.../devicetree/bindings/arm/tegra/actmon.txt | 26 ++++++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/tegra/actmon.txt

diff --git a/Documentation/devicetree/bindings/arm/tegra/actmon.txt b/Documentation/devicetree/bindings/arm/tegra/actmon.txt
new file mode 100644
index 0000000..593683f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tegra/actmon.txt
@@ -0,0 +1,26 @@
+Tegra124 Activity Monitor driver
+
+Required properties:
+
+- compatible: should be "nvidia,tegra124-actmon"
+- reg: offset and length of the register set for the device
+- interrupts: standard interrupt property
+- clocks: Must contain a phandle and clock specifier pair for each entry in clock-names. See ../clock/clock-bindings.txt for details.
+- clock-names: Must include the following entries:
+ - actmon
+ - emc
+- resets: Must contain an entry for each entry in reset-names. See ../reset/reset.txt for details.
+- reset-names: Must include the following entries:
+ - actmon
+
+Example:
+ actmon@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";
+ };
--
1.9.3

2014-10-29 14:51:44

by Tomeu Vizoso

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

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

Signed-off-by: Tomeu Vizoso <[email protected]>
---
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@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@0,6000d000 {
compatible = "nvidia,tegra124-gpio", "nvidia,tegra30-gpio";
reg = <0x0 0x6000d000 0x0 0x1000>;
--
1.9.3

2014-10-29 14:51:42

by Tomeu Vizoso

[permalink] [raw]
Subject: [PATCH 2/3] soc/tegra: add support for Tegra Activity Monitor

The ACTMON block can monitor several counters, providing averaging and firing
interrupts based on watermarking configuration. This implementation monitors
the MCALL and MCCPU counters to choose an appropriate frequency for the
external memory clock.

This patch is based on work by Alex Frid <[email protected]> and Mikko
Perttunen <[email protected]>.

Signed-off-by: Tomeu Vizoso <[email protected]>
---
drivers/soc/tegra/Makefile | 1 +
drivers/soc/tegra/actmon.c | 523 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 524 insertions(+)
create mode 100644 drivers/soc/tegra/actmon.c

diff --git a/drivers/soc/tegra/Makefile b/drivers/soc/tegra/Makefile
index cdaad9d..00b08a0 100644
--- a/drivers/soc/tegra/Makefile
+++ b/drivers/soc/tegra/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_ARCH_TEGRA) += fuse/

obj-$(CONFIG_ARCH_TEGRA) += common.o
obj-$(CONFIG_ARCH_TEGRA) += pmc.o
+obj-$(CONFIG_ARCH_TEGRA) += actmon.o
diff --git a/drivers/soc/tegra/actmon.c b/drivers/soc/tegra/actmon.c
new file mode 100644
index 0000000..031c601
--- /dev/null
+++ b/drivers/soc/tegra/actmon.c
@@ -0,0 +1,523 @@
+/*
+ * A driver for the ACTMON block
+ *
+ * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.
+ * Copyright (C) 2014 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/cpufreq.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+#define ACTMON_GLB_STATUS 0x0
+#define ACTMON_GLB_PERIOD_CTRL 0x4
+
+#define ACTMON_DEV_CTRL 0x0
+#define ACTMON_DEV_CTRL_K_VAL_SHIFT 10
+#define ACTMON_DEV_CTRL_ENB_PERIODIC BIT(18)
+#define ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN BIT(20)
+#define ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN BIT(21)
+#define ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_NUM_SHIFT 23
+#define ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT 26
+#define ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN BIT(29)
+#define ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN BIT(30)
+#define ACTMON_DEV_CTRL_ENB BIT(31)
+
+#define ACTMON_DEV_UPPER_WMARK 0x4
+#define ACTMON_DEV_LOWER_WMARK 0x8
+#define ACTMON_DEV_INIT_AVG 0xc
+#define ACTMON_DEV_AVG_UPPER_WMARK 0x10
+#define ACTMON_DEV_AVG_LOWER_WMARK 0x14
+#define ACTMON_DEV_COUNT_WEIGHT 0x18
+#define ACTMON_DEV_AVG_COUNT 0x20
+#define ACTMON_DEV_INTR_STATUS 0x24
+
+#define ACTMON_DEV_INTR_CONSECUTIVE_UPPER BIT(31)
+#define ACTMON_DEV_INTR_CONSECUTIVE_LOWER BIT(30)
+
+#define ACTMON_ABOVE_WMARK_WINDOW 1
+#define ACTMON_BELOW_WMARK_WINDOW 3
+#define ACTMON_BOOST_FREQ_STEP 16000
+
+/* activity counter is incremented every 256 memory transactions, and each
+ * transaction takes 4 EMC clocks for Tegra124; So the COUNT_WEIGHT is
+ * 4 * 256 = 1024.
+ */
+#define ACTMON_COUNT_WEIGHT 0x400
+
+/*
+ * ACTMON_AVERAGE_WINDOW_LOG2: default value for @DEV_CTRL_K_VAL, which translates to
+ * 2 ^ (K_VAL + 1). ex: 2 ^ (6 + 1) = 128
+ */
+#define ACTMON_AVERAGE_WINDOW_LOG2 6
+#define ACTMON_SAMPLING_PERIOD 12 /* ms */
+#define ACTMON_DEFAULT_AVG_BAND 6 /* 1/10 of % */
+
+#define KHZ 1000
+
+/**
+ * struct tegra_actmon_device_config - configuration specific to an ACTMON device
+ *
+ * Coefficients and thresholds are in %
+ */
+struct tegra_actmon_device_config {
+ u32 offset;
+ u32 irq_mask;
+
+ unsigned int boost_up_coeff;
+ unsigned int boost_down_coeff;
+ unsigned int boost_up_threshold;
+ unsigned int boost_down_threshold;
+ u32 avg_dependency_threshold;
+};
+
+static struct tegra_actmon_device_config actmon_device_configs[] = {
+ {
+ /* MCALL */
+ .offset = 0x1c0,
+ .irq_mask = 1 << 26,
+ .boost_up_coeff = 200,
+ .boost_down_coeff = 50,
+ .boost_up_threshold = 60,
+ .boost_down_threshold = 40,
+ },
+ {
+ /* MCCPU */
+ .offset = 0x200,
+ .irq_mask = 1 << 25,
+ .boost_up_coeff = 800,
+ .boost_down_coeff = 90,
+ .boost_up_threshold = 27,
+ .boost_down_threshold = 10,
+ .avg_dependency_threshold = 50000,
+ },
+};
+
+/**
+ * struct tegra_actmon_device - state specific to an ACTMON device
+ *
+ * Frequencies are in kHz.
+ */
+struct tegra_actmon_device {
+ const struct tegra_actmon_device_config *config;
+
+ void __iomem *regs;
+ u32 avg_band_freq;
+ u32 avg_count;
+
+ unsigned long target_freq;
+ unsigned long boost_freq;
+};
+
+struct tegra_actmon {
+ struct platform_device *pdev;
+ struct reset_control *reset;
+ struct clk *clock;
+ void __iomem *regs;
+ spinlock_t lock;
+
+ struct clk *emc_clock;
+ unsigned long max_freq;
+ unsigned long cur_freq;
+ struct notifier_block rate_change_nb;
+
+ struct tegra_actmon_device devices[ARRAY_SIZE(actmon_device_configs)];
+};
+
+struct tegra_actmon_emc_ratio {
+ unsigned long cpu_freq;
+ unsigned long emc_freq;
+};
+
+static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
+ { 1400000, ULONG_MAX },
+ { 1200000, 750000 },
+ { 1100000, 600000 },
+ { 1000000, 500000 },
+ { 800000, 375000 },
+ { 500000, 200000 },
+ { 250000, 100000 },
+};
+
+static unsigned long do_percent(unsigned long val, unsigned int pct)
+{
+ return val * pct / 100;
+}
+
+static void tegra_actmon_update_avg_wmark(struct tegra_actmon_device *dev)
+{
+ u32 avg = dev->avg_count;
+ u32 band = dev->avg_band_freq * ACTMON_SAMPLING_PERIOD;
+
+ writel(avg + band, dev->regs + ACTMON_DEV_AVG_UPPER_WMARK);
+ avg = max(avg, band);
+ writel(avg - band, dev->regs + ACTMON_DEV_AVG_LOWER_WMARK);
+}
+
+static void tegra_actmon_update_wmark(struct tegra_actmon *tegra, struct tegra_actmon_device *dev)
+{
+ u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+
+ writel(do_percent(val, dev->config->boost_up_threshold),
+ dev->regs + ACTMON_DEV_UPPER_WMARK);
+
+ writel(do_percent(val, dev->config->boost_down_threshold),
+ dev->regs + ACTMON_DEV_LOWER_WMARK);
+}
+
+static void actmon_write_barrier(struct tegra_actmon *tegra)
+{
+ /* ensure the update has reached the ACTMON */
+ wmb();
+ readl(tegra->regs + ACTMON_GLB_STATUS);
+}
+
+irqreturn_t actmon_isr(int irq, void *data)
+{
+ struct tegra_actmon *tegra = data;
+ struct tegra_actmon_device *dev = NULL;
+ unsigned long flags;
+ u32 val;
+ int i;
+
+ val = readl(tegra->regs + ACTMON_GLB_STATUS);
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); ++i) {
+ if (val & tegra->devices[i].config->irq_mask) {
+ dev = tegra->devices + i;
+ break;
+ }
+ }
+
+ if (!dev)
+ return IRQ_NONE;
+
+ spin_lock_irqsave(&tegra->lock, flags);
+
+ dev->avg_count = readl(dev->regs + ACTMON_DEV_AVG_COUNT);
+ tegra_actmon_update_avg_wmark(dev);
+
+ val = readl(dev->regs + ACTMON_DEV_INTR_STATUS);
+ if (val & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
+ val = readl(dev->regs + ACTMON_DEV_CTRL) |
+ ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN |
+ ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+
+ /*
+ * new_boost = min(old_boost * up_coef + step, max_freq)
+ */
+ dev->boost_freq = ACTMON_BOOST_FREQ_STEP +
+ do_percent(dev->boost_freq, dev->config->boost_up_coeff);
+ if (dev->boost_freq >= tegra->max_freq) {
+ dev->boost_freq = tegra->max_freq;
+ val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+ }
+ writel(val, dev->regs + ACTMON_DEV_CTRL);
+ } else if (val & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
+ val = readl(dev->regs + ACTMON_DEV_CTRL) |
+ ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN |
+ ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+
+ /*
+ * new_boost = old_boost * down_coef
+ * or 0 if (old_boost * down_coef < step / 2)
+ */
+ dev->boost_freq =
+ do_percent(dev->boost_freq, dev->config->boost_down_coeff);
+ if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1)) {
+ dev->boost_freq = 0;
+ val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ }
+ writel(val, dev->regs + ACTMON_DEV_CTRL);
+ }
+
+ if (dev->config->avg_dependency_threshold) {
+ val = readl(dev->regs + ACTMON_DEV_CTRL);
+ if (dev->avg_count >= dev->config->avg_dependency_threshold)
+ val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ else if (dev->boost_freq == 0)
+ val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ writel(val, dev->regs + ACTMON_DEV_CTRL);
+ }
+
+ writel(0xffffffff, dev->regs + ACTMON_DEV_INTR_STATUS);
+
+ actmon_write_barrier(tegra);
+
+ spin_unlock_irqrestore(&tegra->lock, flags);
+
+ return IRQ_WAKE_THREAD;
+}
+
+static unsigned long actmon_cpu_to_emc_rate(struct tegra_actmon *tegra, unsigned long cpu_freq)
+{
+ int i;
+ struct tegra_actmon_emc_ratio *ratio = actmon_emc_ratios;
+
+ for (i = 0; i < ARRAY_SIZE(actmon_emc_ratios); i++, ratio++) {
+ if (cpu_freq >= ratio->cpu_freq) {
+ if (ratio->emc_freq >= tegra->max_freq)
+ return tegra->max_freq;
+ else
+ return ratio->emc_freq;
+ }
+ }
+
+ return 0;
+}
+
+static void actmon_update_target(struct tegra_actmon *tegra, struct tegra_actmon_device *dev)
+{
+ unsigned long cpu_freq = 0;
+ unsigned long static_cpu_emc_freq = 0;
+ unsigned int avg_sustain_coef;
+ unsigned long flags;
+
+ if (dev->config->avg_dependency_threshold) {
+ cpu_freq = cpufreq_get(0);
+ static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
+ }
+
+ spin_lock_irqsave(&tegra->lock, flags);
+
+ dev->target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+ avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
+ dev->target_freq = do_percent(dev->target_freq, avg_sustain_coef);
+ dev->target_freq += dev->boost_freq;
+
+ if (dev->avg_count >= dev->config->avg_dependency_threshold)
+ dev->target_freq = max(dev->target_freq, static_cpu_emc_freq);
+
+ dev->target_freq = dev->target_freq < 102000 ? 102000 : dev->target_freq;
+
+ spin_unlock_irqrestore(&tegra->lock, flags);
+}
+
+irqreturn_t actmon_thread_isr(int irq, void *data)
+{
+ struct tegra_actmon *tegra = data;
+ struct tegra_actmon_device *dev = NULL;
+ unsigned long target_freq = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); ++i) {
+ dev = tegra->devices + i;
+
+ actmon_update_target(tegra, dev);
+
+ /* TODO: Once we have per-user clks, set one floor freq for each device */
+ target_freq = max(target_freq, dev->target_freq);
+ }
+
+ clk_set_rate(tegra->emc_clock, target_freq * KHZ);
+
+ return IRQ_HANDLED;
+}
+
+static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
+ unsigned long action, void *ptr)
+{
+ struct clk_notifier_data *data = ptr;
+ struct tegra_actmon *tegra = container_of(nb, struct tegra_actmon,
+ rate_change_nb);
+ int i;
+ unsigned long flags;
+
+ spin_lock_irqsave(&tegra->lock, flags);
+
+ switch (action) {
+ case POST_RATE_CHANGE:
+ tegra->cur_freq = data->new_rate / KHZ;
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); ++i)
+ tegra_actmon_update_wmark(tegra, tegra->devices + i);
+
+ actmon_write_barrier(tegra);
+ break;
+ case PRE_RATE_CHANGE:
+ /* fall through */
+ case ABORT_RATE_CHANGE:
+ break;
+ };
+
+ spin_unlock_irqrestore(&tegra->lock, flags);
+ return NOTIFY_OK;
+}
+
+static void tegra_actmon_configure_device(struct tegra_actmon *tegra,
+ struct tegra_actmon_device *dev)
+{
+ u32 val;
+
+ dev->avg_band_freq = tegra->max_freq * ACTMON_DEFAULT_AVG_BAND / KHZ;
+ dev->target_freq = tegra->cur_freq;
+
+ dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+ writel(dev->avg_count, dev->regs + ACTMON_DEV_INIT_AVG);
+
+ tegra_actmon_update_avg_wmark(dev);
+ tegra_actmon_update_wmark(tegra, dev);
+
+ writel(ACTMON_COUNT_WEIGHT, dev->regs + ACTMON_DEV_COUNT_WEIGHT);
+
+ writel(0xffffffff, dev->regs + ACTMON_DEV_INTR_STATUS);
+
+ val = 0;
+ val |= ACTMON_DEV_CTRL_ENB_PERIODIC |
+ ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN |
+ ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
+ val |= (ACTMON_AVERAGE_WINDOW_LOG2 - 1)
+ << ACTMON_DEV_CTRL_K_VAL_SHIFT;
+ val |= (ACTMON_BELOW_WMARK_WINDOW - 1)
+ << ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_NUM_SHIFT;
+ val |= (ACTMON_ABOVE_WMARK_WINDOW - 1)
+ << ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT;
+ val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN |
+ ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+
+ writel(val, dev->regs + ACTMON_DEV_CTRL);
+
+ actmon_write_barrier(tegra);
+
+ val = readl(dev->regs + ACTMON_DEV_CTRL);
+ val |= ACTMON_DEV_CTRL_ENB;
+ writel(val, dev->regs + ACTMON_DEV_CTRL);
+
+ actmon_write_barrier(tegra);
+}
+
+static int tegra_actmon_probe(struct platform_device *pdev)
+{
+ struct tegra_actmon *tegra;
+ struct tegra_actmon_device *dev;
+ unsigned long max_freq;
+ int i;
+ int irq;
+ int err;
+
+ tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+ if (!tegra)
+ return -ENOMEM;
+
+ spin_lock_init(&tegra->lock);
+
+ tegra->regs = devm_ioremap_resource(&pdev->dev, platform_get_resource(pdev, IORESOURCE_MEM, 0));
+ if (IS_ERR(tegra->regs)) {
+ dev_err(&pdev->dev, "Failed to get IO memory\n");
+ return PTR_ERR(tegra->regs);
+ }
+
+ tegra->reset = devm_reset_control_get(&pdev->dev, "actmon");
+ if (IS_ERR(tegra->reset)) {
+ dev_err(&pdev->dev, "Failed to get reset\n");
+ return PTR_ERR(tegra->reset);
+ }
+
+ tegra->clock = devm_clk_get(&pdev->dev, "actmon");
+ if (IS_ERR(tegra->clock)) {
+ dev_err(&pdev->dev, "Failed to get actmon clock\n");
+ return PTR_ERR(tegra->clock);
+ }
+
+ tegra->emc_clock = devm_clk_get(&pdev->dev, "emc");
+ if (IS_ERR(tegra->emc_clock)) {
+ dev_err(&pdev->dev, "Failed to get emc clock\n");
+ return PTR_ERR(tegra->emc_clock);
+ }
+
+ tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
+ err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
+ if (err) {
+ dev_err(&pdev->dev,
+ "Failed to register rate change notifier\n");
+ return err;
+ }
+
+ reset_control_assert(tegra->reset);
+
+ err = clk_prepare_enable(tegra->clock);
+ if (err) {
+ reset_control_deassert(tegra->reset);
+ return err;
+ }
+
+ reset_control_deassert(tegra->reset);
+
+ max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX);
+ tegra->max_freq = max_freq / KHZ;
+
+ clk_set_rate(tegra->emc_clock, max_freq);
+
+ tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+
+ writel(ACTMON_SAMPLING_PERIOD - 1, tegra->regs + ACTMON_GLB_PERIOD_CTRL);
+
+ for (i = 0; i < ARRAY_SIZE(actmon_device_configs); ++i) {
+ dev = tegra->devices + i;
+ dev->config = actmon_device_configs + i;
+ dev->regs = tegra->regs + dev->config->offset;
+
+ tegra_actmon_configure_device(tegra, tegra->devices + i);
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
+ actmon_thread_isr, IRQF_SHARED,
+ "tegra-actmon", tegra);
+ if (err) {
+ dev_err(&pdev->dev, "Interrupt request failed\n");
+ return err;
+ }
+
+ platform_set_drvdata(pdev, tegra);
+
+ dev_info(&pdev->dev, "probed\n");
+
+ return 0;
+}
+
+static int tegra_actmon_remove(struct platform_device *pdev)
+{
+ struct tegra_actmon *tegra = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(tegra->clock);
+
+ return 0;
+}
+
+static struct of_device_id tegra_actmon_of_match[] = {
+ { .compatible = "nvidia,tegra124-actmon" },
+ { },
+};
+
+static struct platform_driver tegra_actmon_driver = {
+ .probe = tegra_actmon_probe,
+ .remove = tegra_actmon_remove,
+ .driver = {
+ .name = "tegra-actmon",
+ .owner = THIS_MODULE,
+ .of_match_table = tegra_actmon_of_match,
+ },
+};
+module_platform_driver(tegra_actmon_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Tegra ACTMON driver");
+MODULE_DEVICE_TABLE(of, tegra_actmon_of_match);
--
1.9.3

2014-11-07 09:07:39

by Alexandre Courbot

[permalink] [raw]
Subject: Re: [PATCH 0/3] Add support for Tegra Activity Monitor

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.

2014-11-07 12:35:29

by Tomeu Vizoso

[permalink] [raw]
Subject: Re: [PATCH 0/3] Add support for Tegra Activity Monitor

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

2014-11-11 04:30:27

by Alexandre Courbot

[permalink] [raw]
Subject: Re: [PATCH 0/3] Add support for Tegra Activity Monitor

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.

2014-11-11 06:56:26

by Terje Bergstrom

[permalink] [raw]
Subject: Re: [PATCH 0/3] Add support for Tegra Activity Monitor

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.

2014-11-11 07:33:05

by Alexandre Courbot

[permalink] [raw]
Subject: Re: [PATCH 0/3] Add support for Tegra Activity Monitor

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.

2014-11-11 08:41:14

by Tomeu Vizoso

[permalink] [raw]
Subject: Re: [PATCH 0/3] Add support for Tegra Activity Monitor

On 11 November 2014 05:29, Alexandre Courbot <[email protected]> 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