* [PATCH v4 0/3] Add support for Tegra Activity Monitor
@ 2014-12-09 13:15 Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 1/3] of: Add binding for NVIDIA Tegra ACTMON node Tomeu Vizoso
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-12-09 13:15 UTC (permalink / raw)
To: linux-pm
Cc: Javier Martinez Canillas, Alexandre Courbot, Mikko Perttunen,
Arto Merilainen, Tomeu Vizoso, devicetree, linux-arm-kernel,
linux-kernel, linux-tegra
Hello,
this v4 just fixes a botched rename in v3. The original cover letter for v3 follows:
this v3 addresses the comments that the devfreq implementation got, namely:
* Address misc. style issues found by Thierry and Alexander
* Added helpers for register i/o
* Further documented the structs
* Enable the ACTMON after the IRQ handler has been installed
* Disable the ACTMON before removing the IRQ handler
* Add governor in a subsys initcall
There's an open question on whether some functionality currently in this
devfreq driver should be moved into the devfreq framework, but without knowing
of other SoC family that would benefit from it, I'm reticent. It would be
great to hear from the devfreq maintainers if they have any plans regarding
this, or if they have any suggestion.
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:
* EMC driver: http://thread.gmane.org/gmane.linux.drivers.devicetree/99304
* 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-v4
Regards,
Tomeu
Tomeu Vizoso (3):
of: Add binding for NVIDIA Tegra ACTMON node
PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor
ARM: tegra: Add Tegra124 ACTMON support
.../devicetree/bindings/arm/tegra/actmon.txt | 38 +
arch/arm/boot/dts/tegra124.dtsi | 23 +
drivers/devfreq/Kconfig | 9 +
drivers/devfreq/Makefile | 1 +
drivers/devfreq/tegra-actmon-devfreq.c | 777 +++++++++++++++++++++
5 files changed, 848 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/tegra/actmon.txt
create mode 100644 drivers/devfreq/tegra-actmon-devfreq.c
--
1.9.3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 1/3] of: Add binding for NVIDIA Tegra ACTMON node
2014-12-09 13:15 [PATCH v4 0/3] Add support for Tegra Activity Monitor Tomeu Vizoso
@ 2014-12-09 13:15 ` Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 3/3] ARM: tegra: Add Tegra124 ACTMON support Tomeu Vizoso
2 siblings, 0 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-12-09 13:15 UTC (permalink / raw)
To: linux-pm
Cc: Javier Martinez Canillas, Alexandre Courbot, Mikko Perttunen,
Arto Merilainen, Tomeu Vizoso, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Stephen Warren,
Thierry Reding, devicetree, linux-tegra, linux-kernel
This block gathers statistics about various counters and can be configured to
fire interrupts when thresholds are crossed.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
v2: * Add operating-points property
---
.../devicetree/bindings/arm/tegra/actmon.txt | 38 ++++++++++++++++++++++
1 file changed, 38 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..b4069df
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tegra/actmon.txt
@@ -0,0 +1,38 @@
+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
+- operating-points: Supported operating points. See ../power/opp.txt for details.
+
+Example:
+ actmon@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";
+ operating-points = <
+ /* kHz uV */
+ 102000 800000
+ 204000 800000
+ 300000 820000
+ 396000 850000
+ 528000 880000
+ 600000 910000
+ 792000 980000
+ 924000 1010000
+ >;
+ };
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor
2014-12-09 13:15 [PATCH v4 0/3] Add support for Tegra Activity Monitor Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 1/3] of: Add binding for NVIDIA Tegra ACTMON node Tomeu Vizoso
@ 2014-12-09 13:15 ` Tomeu Vizoso
2014-12-12 7:44 ` Alexandre Courbot
2014-12-14 18:26 ` Paul Walmsley
2014-12-09 13:15 ` [PATCH v4 3/3] ARM: tegra: Add Tegra124 ACTMON support Tomeu Vizoso
2 siblings, 2 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-12-09 13:15 UTC (permalink / raw)
To: linux-pm
Cc: Javier Martinez Canillas, Alexandre Courbot, Mikko Perttunen,
Arto Merilainen, Tomeu Vizoso, MyungJoo Ham, Kyungmin Park,
Stephen Warren, Thierry Reding, Grant Likely, Rob Herring,
linux-kernel, linux-tegra, devicetree
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 <afrid@nvidia.com> and Mikko
Perttunen <mikko.perttunen@kapsi.fi>.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
v3: * Address misc. style issues found by Thierry and Alexander
* Added helpers for register i/o
* Further documented the structs
* Enable the ACTMON after the IRQ handler has been installed
* Disable the ACTMON before removing the IRQ handler
* Add governor in a subsys initcall
* Rename tegra-devfreq.c to tegra-actmon-devfreq.c
v2: * Use devfreq
---
drivers/devfreq/Kconfig | 9 +
drivers/devfreq/Makefile | 1 +
drivers/devfreq/tegra-actmon-devfreq.c | 777 +++++++++++++++++++++++++++++++++
3 files changed, 787 insertions(+)
create mode 100644 drivers/devfreq/tegra-actmon-devfreq.c
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index faf4e70..76be33a 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -87,4 +87,13 @@ config ARM_EXYNOS5_BUS_DEVFREQ
It reads PPMU counters of memory controllers and adjusts the
operating frequencies and voltages with OPP support.
+config ARM_TEGRA_ACTMON_DEVFREQ
+ bool "Tegra ACTMON DEVFREQ Driver"
+ depends on ARCH_TEGRA
+ select PM_OPP
+ help
+ This adds the DEVFREQ driver for the Tegra family of SoCs.
+ It reads ACTMON counters of memory controllers and adjusts the
+ operating frequencies and voltages with OPP support.
+
endif # PM_DEVFREQ
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 16138c9..c9159c5 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
# DEVFREQ Drivers
obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ) += exynos/
obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ) += exynos/
+obj-$(CONFIG_ARM_TEGRA_ACTMON_DEVFREQ) += tegra-actmon-devfreq.o
diff --git a/drivers/devfreq/tegra-actmon-devfreq.c b/drivers/devfreq/tegra-actmon-devfreq.c
new file mode 100644
index 0000000..8d2ea22f
--- /dev/null
+++ b/drivers/devfreq/tegra-actmon-devfreq.c
@@ -0,0 +1,777 @@
+/*
+ * A devfreq driver for NVIDIA Tegra SoCs
+ *
+ * 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/devfreq.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/reset.h>
+
+#include "governor.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_INTR_STATUS_CLEAR 0xffffffff
+
+#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
+
+/* Assume that the bus is saturated if the utilization is 25% */
+#define BUS_SATURATION_RATIO 25
+
+/**
+ * struct tegra_devfreq_device_config - configuration specific to an ACTMON
+ * device
+ *
+ * Coefficients and thresholds are in %
+ */
+struct tegra_devfreq_device_config {
+ u32 offset;
+ u32 irq_mask;
+
+ /* Factors applied to boost_freq every consecutive watermark breach */
+ unsigned int boost_up_coeff;
+ unsigned int boost_down_coeff;
+
+ /* Define the watermark bounds when applied to the current avg */
+ unsigned int boost_up_threshold;
+ unsigned int boost_down_threshold;
+
+ /*
+ * Threshold of activity below which the CPU frequency isn't to be
+ * taken into account. This is to avoid increasing the EMC frequency
+ * when the CPU is very busy but not accessing the bus often.
+ */
+ u32 avg_dependency_threshold;
+};
+
+enum tegra_actmon_device {
+ MCALL = 0,
+ MCCPU,
+};
+
+static struct tegra_devfreq_device_config actmon_device_configs[] = {
+ {
+ /* MCALL: All memory accesses (including from the CPUs) */
+ .offset = 0x1c0,
+ .irq_mask = 1 << 26,
+ .boost_up_coeff = 200,
+ .boost_down_coeff = 50,
+ .boost_up_threshold = 60,
+ .boost_down_threshold = 40,
+ },
+ {
+ /* MCCPU: memory accesses from the CPUs */
+ .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_devfreq_device - state specific to an ACTMON device
+ *
+ * Frequencies are in kHz.
+ */
+struct tegra_devfreq_device {
+ const struct tegra_devfreq_device_config *config;
+
+ void __iomem *regs;
+
+ /* Average event count sampled in the last interrupt */
+ u32 avg_count;
+
+ /*
+ * Extra frequency to increase the target by due to consecutive
+ * watermark breaches.
+ */
+ unsigned long boost_freq;
+
+ /* Optimal frequency calculated from the stats for this device */
+ unsigned long target_freq;
+};
+
+struct tegra_devfreq {
+ struct devfreq *devfreq;
+
+ 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_devfreq_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 u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
+{
+ return readl(tegra->regs + offset);
+}
+
+static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
+{
+ writel(val, tegra->regs + offset);
+}
+
+static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
+{
+ return readl(dev->regs + offset);
+}
+
+static void device_writel(struct tegra_devfreq_device *dev, u32 val,
+ u32 offset)
+{
+ writel(val, dev->regs + offset);
+}
+
+static unsigned long do_percent(unsigned long val, unsigned int pct)
+{
+ return val * pct / 100;
+}
+
+static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
+ struct tegra_devfreq_device *dev)
+{
+ u32 avg = dev->avg_count;
+ u32 avg_band_freq = tegra->max_freq * ACTMON_DEFAULT_AVG_BAND / KHZ;
+ u32 band = avg_band_freq * ACTMON_SAMPLING_PERIOD;
+
+ device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);
+
+ avg = max(dev->avg_count, band);
+ device_writel(dev, avg - band, ACTMON_DEV_AVG_LOWER_WMARK);
+}
+
+static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
+ struct tegra_devfreq_device *dev)
+{
+ u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+
+ device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
+ ACTMON_DEV_UPPER_WMARK);
+
+ device_writel(dev, do_percent(val, dev->config->boost_down_threshold),
+ ACTMON_DEV_LOWER_WMARK);
+}
+
+static void actmon_write_barrier(struct tegra_devfreq *tegra)
+{
+ /* ensure the update has reached the ACTMON */
+ wmb();
+ actmon_readl(tegra, ACTMON_GLB_STATUS);
+}
+
+static irqreturn_t actmon_isr(int irq, void *data)
+{
+ struct tegra_devfreq *tegra = data;
+ struct tegra_devfreq_device *dev = NULL;
+ unsigned long flags;
+ u32 val, intr_status, dev_ctrl;
+ unsigned int i;
+
+ val = actmon_readl(tegra, 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 = device_readl(dev, ACTMON_DEV_AVG_COUNT);
+ tegra_devfreq_update_avg_wmark(tegra, dev);
+
+ intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
+ dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
+
+ if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
+ /*
+ * new_boost = min(old_boost * up_coef + step, max_freq)
+ */
+ dev->boost_freq = do_percent(dev->boost_freq,
+ dev->config->boost_up_coeff);
+ dev->boost_freq += ACTMON_BOOST_FREQ_STEP;
+
+ dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+
+ if (dev->boost_freq >= tegra->max_freq)
+ dev->boost_freq = tegra->max_freq;
+ else
+ dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+ } else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
+ /*
+ * 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);
+
+ dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+
+ if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
+ dev->boost_freq = 0;
+ else
+ dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ }
+
+ if (dev->config->avg_dependency_threshold) {
+ if (dev->avg_count >= dev->config->avg_dependency_threshold)
+ dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ else if (dev->boost_freq == 0)
+ dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ }
+
+ device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
+
+ device_writel(dev, ACTMON_INTR_STATUS_CLEAR, 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_devfreq *tegra,
+ unsigned long cpu_freq)
+{
+ unsigned 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_devfreq *tegra,
+ struct tegra_devfreq_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);
+
+ spin_unlock_irqrestore(&tegra->lock, flags);
+}
+
+static irqreturn_t actmon_thread_isr(int irq, void *data)
+{
+ struct tegra_devfreq *tegra = data;
+
+ mutex_lock(&tegra->devfreq->lock);
+ update_devfreq(tegra->devfreq);
+ mutex_unlock(&tegra->devfreq->lock);
+
+ 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_devfreq *tegra;
+ unsigned int i;
+ unsigned long flags;
+
+ if (action != POST_RATE_CHANGE)
+ return NOTIFY_OK;
+
+ tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
+
+ spin_lock_irqsave(&tegra->lock, flags);
+
+ tegra->cur_freq = data->new_rate / KHZ;
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
+ tegra_devfreq_update_wmark(tegra, tegra->devices + i);
+
+ actmon_write_barrier(tegra);
+
+ spin_unlock_irqrestore(&tegra->lock, flags);
+
+ return NOTIFY_OK;
+}
+
+static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
+ struct tegra_devfreq_device *dev)
+{
+ u32 val = 0;
+
+ dev->target_freq = tegra->cur_freq;
+
+ dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+ device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
+
+ tegra_devfreq_update_avg_wmark(tegra, dev);
+ tegra_devfreq_update_wmark(tegra, dev);
+
+ device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
+ device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
+
+ 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;
+
+ device_writel(dev, val, ACTMON_DEV_CTRL);
+
+ actmon_write_barrier(tegra);
+}
+
+static void tegra_actmon_enable_device(struct tegra_devfreq *tegra,
+ struct tegra_devfreq_device *dev)
+{
+ u32 val;
+
+ val = device_readl(dev, ACTMON_DEV_CTRL);
+ val |= ACTMON_DEV_CTRL_ENB;
+ device_writel(dev, val, ACTMON_DEV_CTRL);
+
+ actmon_write_barrier(tegra);
+}
+
+static void tegra_actmon_disable_device(struct tegra_devfreq *tegra,
+ struct tegra_devfreq_device *dev)
+{
+ u32 val;
+
+ val = device_readl(dev, ACTMON_DEV_CTRL);
+ val &= ~ACTMON_DEV_CTRL_ENB;
+ device_writel(dev, val, ACTMON_DEV_CTRL);
+
+ actmon_write_barrier(tegra);
+}
+
+static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
+ u32 flags)
+{
+ struct platform_device *pdev;
+ struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+ struct dev_pm_opp *opp;
+ unsigned long rate = *freq * KHZ;
+
+ pdev = container_of(dev, struct platform_device, dev);
+
+ rcu_read_lock();
+ opp = devfreq_recommended_opp(dev, &rate, flags);
+ if (IS_ERR(opp)) {
+ rcu_read_unlock();
+ dev_err(dev, "Failed to find opp for %lu KHz\n", *freq);
+ return PTR_ERR(opp);
+ }
+ rate = dev_pm_opp_get_freq(opp);
+ rcu_read_unlock();
+
+ /* TODO: Once we have per-user clk constraints, set a floor */
+ clk_set_rate(tegra->emc_clock, rate);
+
+ /* TODO: Set voltage as well */
+
+ return 0;
+}
+
+static int tegra_devfreq_get_dev_status(struct device *dev,
+ struct devfreq_dev_status *stat)
+{
+ struct platform_device *pdev;
+ struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+ struct tegra_devfreq_device *actmon_dev;
+
+ pdev = container_of(dev, struct platform_device, dev);
+
+ stat->current_frequency = tegra->cur_freq;
+
+ /* To be used by the tegra governor */
+ stat->private_data = tegra;
+
+ /* The below are to be used by the other governors */
+
+ actmon_dev = &tegra->devices[MCALL];
+
+ /* Number of cycles spent on memory access */
+ stat->busy_time = actmon_dev->avg_count;
+
+ /* The bus can be considered to be saturated way before 100% */
+ stat->busy_time *= 100 / BUS_SATURATION_RATIO;
+
+ /* Number of cycles in a sampling period */
+ stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
+
+ return 0;
+}
+
+static struct devfreq_dev_profile tegra_devfreq_profile = {
+ .polling_ms = 0,
+ .target = tegra_devfreq_target,
+ .get_dev_status = tegra_devfreq_get_dev_status,
+};
+
+static int tegra_governor_get_target(struct devfreq *devfreq,
+ unsigned long *freq)
+{
+ struct devfreq_dev_status stat;
+ struct tegra_devfreq *tegra;
+ struct tegra_devfreq_device *dev;
+ unsigned long target_freq = 0;
+ unsigned int i;
+ int err;
+
+ err = devfreq->profile->get_dev_status(devfreq->dev.parent, &stat);
+ if (err)
+ return err;
+
+ tegra = stat.private_data;
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
+ dev = &tegra->devices[i];
+
+ actmon_update_target(tegra, dev);
+
+ target_freq = max(target_freq, dev->target_freq);
+ }
+
+ *freq = target_freq;
+
+ return 0;
+}
+
+static int tegra_governor_event_handler(struct devfreq *devfreq,
+ unsigned int event, void *data)
+{
+ return 0;
+}
+
+static struct devfreq_governor tegra_devfreq_governor = {
+ .name = "tegra_actmon",
+ .get_target_freq = tegra_governor_get_target,
+ .event_handler = tegra_governor_event_handler,
+};
+
+static int __init tegra_governor_init(void)
+{
+ return devfreq_add_governor(&tegra_devfreq_governor);
+}
+subsys_initcall(tegra_governor_init);
+
+static int tegra_devfreq_probe(struct platform_device *pdev)
+{
+ struct tegra_devfreq *tegra;
+ struct tegra_devfreq_device *dev;
+ struct resource *res;
+ unsigned long max_freq;
+ unsigned int i;
+ int irq;
+ int err;
+
+ tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+ if (!tegra)
+ return -ENOMEM;
+
+ spin_lock_init(&tegra->lock);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ tegra->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(tegra->regs))
+ 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);
+ }
+
+ err = of_init_opp_table(&pdev->dev);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to init operating point table\n");
+ return err;
+ }
+
+ 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) {
+ dev_err(&pdev->dev,
+ "Failed to prepare and enable ACTMON clock\n");
+ 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;
+
+ actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
+ 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, dev);
+ }
+
+ tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
+ tegra->devfreq = devm_devfreq_add_device(&pdev->dev,
+ &tegra_devfreq_profile,
+ "tegra_actmon",
+ NULL);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0) {
+ dev_err(&pdev->dev, "Failed to get IRQ\n");
+ return -ENODEV;
+ }
+
+ platform_set_drvdata(pdev, tegra);
+
+ err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
+ actmon_thread_isr, IRQF_SHARED,
+ "tegra-devfreq", tegra);
+ if (err) {
+ dev_err(&pdev->dev, "Interrupt request failed\n");
+ return err;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++)
+ tegra_actmon_enable_device(tegra, tegra->devices + i);
+
+ return 0;
+}
+
+static int tegra_devfreq_remove(struct platform_device *pdev)
+{
+ struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
+ int irq = platform_get_irq(pdev, 0);
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++)
+ tegra_actmon_disable_device(tegra, tegra->devices + i);
+
+ devm_free_irq(&pdev->dev, irq, tegra);
+
+ clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
+
+ clk_disable_unprepare(tegra->clock);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+
+static int tegra_devfreq_suspend(struct device *dev)
+{
+ struct platform_device *pdev;
+ struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+ struct tegra_devfreq_device *actmon_dev;
+ unsigned int i;
+
+ pdev = container_of(dev, struct platform_device, dev);
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
+ actmon_dev = &tegra->devices[i];
+
+ tegra_actmon_disable_device(tegra, actmon_dev);
+
+ device_writel(actmon_dev, ACTMON_INTR_STATUS_CLEAR,
+ ACTMON_DEV_INTR_STATUS);
+
+ actmon_write_barrier(tegra);
+ }
+
+ return 0;
+}
+
+static int tegra_devfreq_resume(struct device *dev)
+{
+ struct platform_device *pdev;
+ struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+ struct tegra_devfreq_device *actmon_dev;
+ unsigned int i;
+
+ pdev = container_of(dev, struct platform_device, dev);
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
+ actmon_dev = &tegra->devices[i];
+
+ tegra_actmon_configure_device(tegra, actmon_dev);
+ tegra_actmon_enable_device(tegra, actmon_dev);
+ }
+
+ return 0;
+}
+
+#endif /* CONFIG_PM_SLEEP */
+
+static SIMPLE_DEV_PM_OPS(tegra_devfreq_pm_ops,
+ tegra_devfreq_suspend,
+ tegra_devfreq_resume);
+
+static struct of_device_id tegra_devfreq_of_match[] = {
+ { .compatible = "nvidia,tegra124-actmon" },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, tegra_devfreq_of_match);
+
+static struct platform_driver tegra_devfreq_driver = {
+ .probe = tegra_devfreq_probe,
+ .remove = tegra_devfreq_remove,
+ .driver = {
+ .name = "tegra-devfreq",
+ .of_match_table = tegra_devfreq_of_match,
+ .pm = &tegra_devfreq_pm_ops,
+ },
+};
+module_platform_driver(tegra_devfreq_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Tegra devfreq driver");
+MODULE_AUTHOR("Tomeu Vizoso <tomeu.vizoso@collabora.com>");
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 3/3] ARM: tegra: Add Tegra124 ACTMON support
2014-12-09 13:15 [PATCH v4 0/3] Add support for Tegra Activity Monitor Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 1/3] of: Add binding for NVIDIA Tegra ACTMON node Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor Tomeu Vizoso
@ 2014-12-09 13:15 ` Tomeu Vizoso
2 siblings, 0 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-12-09 13:15 UTC (permalink / raw)
To: linux-pm
Cc: Javier Martinez Canillas, Alexandre Courbot, Mikko Perttunen,
Arto Merilainen, Tomeu Vizoso, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, devicetree, linux-arm-kernel,
linux-tegra, linux-kernel
Add device node for the ACTMON block to the Tegra124 device tree.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
v2: * Add operating-points property
---
arch/arm/boot/dts/tegra124.dtsi | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index a49e7da..9d7baa8 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -215,6 +215,29 @@
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";
+ operating-points = <
+ /* KHz uV */
+ /* TODO: Add more points below 102 MHz */
+ 102000 800000
+ 204000 800000
+ 300000 820000
+ 396000 850000
+ 528000 880000
+ 600000 910000
+ 792000 980000
+ 924000 1010000
+ >;
+ };
+
gpio: gpio@0,6000d000 {
compatible = "nvidia,tegra124-gpio", "nvidia,tegra30-gpio";
reg = <0x0 0x6000d000 0x0 0x1000>;
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor
2014-12-09 13:15 ` [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor Tomeu Vizoso
@ 2014-12-12 7:44 ` Alexandre Courbot
2014-12-15 10:45 ` Tomeu Vizoso
2014-12-14 18:26 ` Paul Walmsley
1 sibling, 1 reply; 8+ messages in thread
From: Alexandre Courbot @ 2014-12-12 7:44 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-pm, Javier Martinez Canillas, Mikko Perttunen,
Arto Merilainen, MyungJoo Ham, Kyungmin Park, Stephen Warren,
Thierry Reding, Grant Likely, Rob Herring,
Linux Kernel Mailing List, linux-tegra, devicetree
Hi Tomeu,
On Tue, Dec 9, 2014 at 10:15 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> 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 <afrid@nvidia.com> and Mikko
> Perttunen <mikko.perttunen@kapsi.fi>.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> ---
>
> v3: * Address misc. style issues found by Thierry and Alexander
> * Added helpers for register i/o
> * Further documented the structs
> * Enable the ACTMON after the IRQ handler has been installed
> * Disable the ACTMON before removing the IRQ handler
> * Add governor in a subsys initcall
> * Rename tegra-devfreq.c to tegra-actmon-devfreq.c
>
> v2: * Use devfreq
This is taking shape, and although it will be desirable to port it to
the more suitable watermark model once it gets merged, I am for
merging this first version in the meantime as it provides a useful
feature. Then we could use it to test-drive watermarking and argue the
advantages of that model with impressive negative line count patches.
;)
A few comments below but I am close to giving my ack to this.
> ---
> drivers/devfreq/Kconfig | 9 +
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/tegra-actmon-devfreq.c | 777 +++++++++++++++++++++++++++++++++
> 3 files changed, 787 insertions(+)
> create mode 100644 drivers/devfreq/tegra-actmon-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index faf4e70..76be33a 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -87,4 +87,13 @@ config ARM_EXYNOS5_BUS_DEVFREQ
> It reads PPMU counters of memory controllers and adjusts the
> operating frequencies and voltages with OPP support.
>
> +config ARM_TEGRA_ACTMON_DEVFREQ
> + bool "Tegra ACTMON DEVFREQ Driver"
> + depends on ARCH_TEGRA
> + select PM_OPP
> + help
> + This adds the DEVFREQ driver for the Tegra family of SoCs.
> + It reads ACTMON counters of memory controllers and adjusts the
> + operating frequencies and voltages with OPP support.
> +
> endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 16138c9..c9159c5 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
> # DEVFREQ Drivers
> obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ) += exynos/
> obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ) += exynos/
> +obj-$(CONFIG_ARM_TEGRA_ACTMON_DEVFREQ) += tegra-actmon-devfreq.o
> diff --git a/drivers/devfreq/tegra-actmon-devfreq.c b/drivers/devfreq/tegra-actmon-devfreq.c
> new file mode 100644
> index 0000000..8d2ea22f
> --- /dev/null
> +++ b/drivers/devfreq/tegra-actmon-devfreq.c
> @@ -0,0 +1,777 @@
> +/*
> + * A devfreq driver for NVIDIA Tegra SoCs
> + *
> + * 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/devfreq.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/reset.h>
> +
> +#include "governor.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_INTR_STATUS_CLEAR 0xffffffff
> +
> +#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
> +
> +/* Assume that the bus is saturated if the utilization is 25% */
> +#define BUS_SATURATION_RATIO 25
> +
> +/**
> + * struct tegra_devfreq_device_config - configuration specific to an ACTMON
> + * device
> + *
> + * Coefficients and thresholds are in %
> + */
> +struct tegra_devfreq_device_config {
> + u32 offset;
> + u32 irq_mask;
> +
> + /* Factors applied to boost_freq every consecutive watermark breach */
> + unsigned int boost_up_coeff;
> + unsigned int boost_down_coeff;
> +
> + /* Define the watermark bounds when applied to the current avg */
> + unsigned int boost_up_threshold;
> + unsigned int boost_down_threshold;
> +
> + /*
> + * Threshold of activity below which the CPU frequency isn't to be
> + * taken into account. This is to avoid increasing the EMC frequency
> + * when the CPU is very busy but not accessing the bus often.
> + */
> + u32 avg_dependency_threshold;
> +};
> +
> +enum tegra_actmon_device {
> + MCALL = 0,
> + MCCPU,
> +};
> +
> +static struct tegra_devfreq_device_config actmon_device_configs[] = {
> + {
> + /* MCALL: All memory accesses (including from the CPUs) */
> + .offset = 0x1c0,
> + .irq_mask = 1 << 26,
> + .boost_up_coeff = 200,
> + .boost_down_coeff = 50,
> + .boost_up_threshold = 60,
> + .boost_down_threshold = 40,
> + },
> + {
> + /* MCCPU: memory accesses from the CPUs */
> + .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,
The comment above says that coefficients and thresholds are in % - but
does it apply to avg_dependency_threshold? The number looks like it is
something else.
> + },
> +};
> +
> +/**
> + * struct tegra_devfreq_device - state specific to an ACTMON device
> + *
> + * Frequencies are in kHz.
> + */
> +struct tegra_devfreq_device {
> + const struct tegra_devfreq_device_config *config;
> +
> + void __iomem *regs;
> +
> + /* Average event count sampled in the last interrupt */
> + u32 avg_count;
> +
> + /*
> + * Extra frequency to increase the target by due to consecutive
> + * watermark breaches.
> + */
> + unsigned long boost_freq;
> +
> + /* Optimal frequency calculated from the stats for this device */
> + unsigned long target_freq;
> +};
> +
> +struct tegra_devfreq {
> + struct devfreq *devfreq;
> +
> + struct platform_device *pdev;
It seems like pdev is not used anywhere in this file.
> + 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_devfreq_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 u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
> +{
> + return readl(tegra->regs + offset);
> +}
> +
> +static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
> +{
> + writel(val, tegra->regs + offset);
> +}
> +
> +static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
> +{
> + return readl(dev->regs + offset);
> +}
> +
> +static void device_writel(struct tegra_devfreq_device *dev, u32 val,
> + u32 offset)
> +{
> + writel(val, dev->regs + offset);
> +}
> +
> +static unsigned long do_percent(unsigned long val, unsigned int pct)
> +{
> + return val * pct / 100;
> +}
> +
> +static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev)
> +{
> + u32 avg = dev->avg_count;
> + u32 avg_band_freq = tegra->max_freq * ACTMON_DEFAULT_AVG_BAND / KHZ;
> + u32 band = avg_band_freq * ACTMON_SAMPLING_PERIOD;
> +
> + device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);
> +
> + avg = max(dev->avg_count, band);
> + device_writel(dev, avg - band, ACTMON_DEV_AVG_LOWER_WMARK);
> +}
> +
> +static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev)
> +{
> + u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
> +
> + device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
> + ACTMON_DEV_UPPER_WMARK);
> +
> + device_writel(dev, do_percent(val, dev->config->boost_down_threshold),
> + ACTMON_DEV_LOWER_WMARK);
> +}
> +
> +static void actmon_write_barrier(struct tegra_devfreq *tegra)
> +{
> + /* ensure the update has reached the ACTMON */
> + wmb();
> + actmon_readl(tegra, ACTMON_GLB_STATUS);
> +}
> +
> +static irqreturn_t actmon_isr(int irq, void *data)
> +{
> + struct tegra_devfreq *tegra = data;
> + struct tegra_devfreq_device *dev = NULL;
> + unsigned long flags;
> + u32 val, intr_status, dev_ctrl;
> + unsigned int i;
> +
> + val = actmon_readl(tegra, 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;
> + }
> + }
Just to be sure: could it happen that we have several IRQ bits set for
a single interrupt? If so, it seems like the current code is not
designed to handle this.
> +
> + if (!dev)
> + return IRQ_NONE;
> +
> + spin_lock_irqsave(&tegra->lock, flags);
> +
> + dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
> + tegra_devfreq_update_avg_wmark(tegra, dev);
> +
> + intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
> + dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
> +
> + if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
> + /*
> + * new_boost = min(old_boost * up_coef + step, max_freq)
> + */
> + dev->boost_freq = do_percent(dev->boost_freq,
> + dev->config->boost_up_coeff);
> + dev->boost_freq += ACTMON_BOOST_FREQ_STEP;
> +
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +
> + if (dev->boost_freq >= tegra->max_freq)
> + dev->boost_freq = tegra->max_freq;
> + else
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> + } else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
> + /*
> + * 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);
> +
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +
> + if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
> + dev->boost_freq = 0;
> + else
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> + }
> +
> + if (dev->config->avg_dependency_threshold) {
> + if (dev->avg_count >= dev->config->avg_dependency_threshold)
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> + else if (dev->boost_freq == 0)
> + dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> + }
> +
> + device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
> +
> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, 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_devfreq *tegra,
> + unsigned long cpu_freq)
> +{
> + unsigned 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_devfreq *tegra,
> + struct tegra_devfreq_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);
> +
> + spin_unlock_irqrestore(&tegra->lock, flags);
> +}
> +
> +static irqreturn_t actmon_thread_isr(int irq, void *data)
> +{
> + struct tegra_devfreq *tegra = data;
> +
> + mutex_lock(&tegra->devfreq->lock);
> + update_devfreq(tegra->devfreq);
> + mutex_unlock(&tegra->devfreq->lock);
> +
> + 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_devfreq *tegra;
> + unsigned int i;
> + unsigned long flags;
> +
> + if (action != POST_RATE_CHANGE)
> + return NOTIFY_OK;
> +
> + tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
> +
> + spin_lock_irqsave(&tegra->lock, flags);
> +
> + tegra->cur_freq = data->new_rate / KHZ;
> +
> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
> + tegra_devfreq_update_wmark(tegra, tegra->devices + i);
> +
> + actmon_write_barrier(tegra);
> +
> + spin_unlock_irqrestore(&tegra->lock, flags);
> +
> + return NOTIFY_OK;
> +}
> +
> +static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev)
> +{
> + u32 val = 0;
> +
> + dev->target_freq = tegra->cur_freq;
> +
> + dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
> + device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
> +
> + tegra_devfreq_update_avg_wmark(tegra, dev);
> + tegra_devfreq_update_wmark(tegra, dev);
> +
> + device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> +
> + 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;
> +
> + device_writel(dev, val, ACTMON_DEV_CTRL);
> +
> + actmon_write_barrier(tegra);
> +}
> +
> +static void tegra_actmon_enable_device(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev)
> +{
> + u32 val;
> +
> + val = device_readl(dev, ACTMON_DEV_CTRL);
> + val |= ACTMON_DEV_CTRL_ENB;
> + device_writel(dev, val, ACTMON_DEV_CTRL);
> +
> + actmon_write_barrier(tegra);
> +}
> +
> +static void tegra_actmon_disable_device(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev)
> +{
> + u32 val;
> +
> + val = device_readl(dev, ACTMON_DEV_CTRL);
> + val &= ~ACTMON_DEV_CTRL_ENB;
> + device_writel(dev, val, ACTMON_DEV_CTRL);
> +
> + actmon_write_barrier(tegra);
> +}
> +
> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> + u32 flags)
> +{
> + struct platform_device *pdev;
> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> + struct dev_pm_opp *opp;
> + unsigned long rate = *freq * KHZ;
> +
> + pdev = container_of(dev, struct platform_device, dev);
> +
> + rcu_read_lock();
> + opp = devfreq_recommended_opp(dev, &rate, flags);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + dev_err(dev, "Failed to find opp for %lu KHz\n", *freq);
> + return PTR_ERR(opp);
> + }
> + rate = dev_pm_opp_get_freq(opp);
> + rcu_read_unlock();
> +
> + /* TODO: Once we have per-user clk constraints, set a floor */
> + clk_set_rate(tegra->emc_clock, rate);
> +
> + /* TODO: Set voltage as well */
> +
> + return 0;
> +}
> +
> +static int tegra_devfreq_get_dev_status(struct device *dev,
> + struct devfreq_dev_status *stat)
> +{
> + struct platform_device *pdev;
> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> + struct tegra_devfreq_device *actmon_dev;
> +
> + pdev = container_of(dev, struct platform_device, dev);
> +
> + stat->current_frequency = tegra->cur_freq;
> +
> + /* To be used by the tegra governor */
> + stat->private_data = tegra;
> +
> + /* The below are to be used by the other governors */
> +
> + actmon_dev = &tegra->devices[MCALL];
> +
> + /* Number of cycles spent on memory access */
> + stat->busy_time = actmon_dev->avg_count;
> +
> + /* The bus can be considered to be saturated way before 100% */
> + stat->busy_time *= 100 / BUS_SATURATION_RATIO;
> +
> + /* Number of cycles in a sampling period */
> + stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
> +
> + return 0;
> +}
> +
> +static struct devfreq_dev_profile tegra_devfreq_profile = {
> + .polling_ms = 0,
> + .target = tegra_devfreq_target,
> + .get_dev_status = tegra_devfreq_get_dev_status,
> +};
> +
> +static int tegra_governor_get_target(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_dev_status stat;
> + struct tegra_devfreq *tegra;
> + struct tegra_devfreq_device *dev;
> + unsigned long target_freq = 0;
> + unsigned int i;
> + int err;
> +
> + err = devfreq->profile->get_dev_status(devfreq->dev.parent, &stat);
> + if (err)
> + return err;
> +
> + tegra = stat.private_data;
> +
> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> + dev = &tegra->devices[i];
> +
> + actmon_update_target(tegra, dev);
> +
> + target_freq = max(target_freq, dev->target_freq);
> + }
> +
> + *freq = target_freq;
> +
> + return 0;
> +}
> +
> +static int tegra_governor_event_handler(struct devfreq *devfreq,
> + unsigned int event, void *data)
> +{
> + return 0;
> +}
>From my v2 review, I don't think you replied to this point:
I think you will need an implementation of this.
DEVFREQ_GOV_START/DEVFREQ_GOV_RESUME, enable actmon interrupts. On
DEVFREQ_GOV_STOP/DEVFREQ_GOV_SUSPEND, disable interrupts. I know you
are doing it somewhere, but it should be done here as this is what
devfreq expects (e.g. to correctly stop interrupts if you switch from
the "tegra" to the "performance" governor).
Your suspend/resume callbacks can then call
devfreq_suspend_device()/devfreq_resume_device().
Or maybe you are keeping it this way to avoid having to store the
devfreq_device in the governor. That's something I can understand, but
I'm afraid it may cause issues when switching governors. Granted, the
possibility of this happening is very small, but there is also the
fact that we don't need/want the watermark interrupts to be fired if
we are using a polling or fixed governor such as "performance". This
will certainly result in an interrupt storm of some sort.
But looking at it twice it seems like avg_count will never be updated
unless the interrupts are fired, so maybe we should update that
counter from some other place, like tegra_devfreq_get_dev_status? Does
that sound feasible?
> +
> +static struct devfreq_governor tegra_devfreq_governor = {
> + .name = "tegra_actmon",
> + .get_target_freq = tegra_governor_get_target,
> + .event_handler = tegra_governor_event_handler,
> +};
> +
> +static int __init tegra_governor_init(void)
> +{
> + return devfreq_add_governor(&tegra_devfreq_governor);
> +}
> +subsys_initcall(tegra_governor_init);
> +
> +static int tegra_devfreq_probe(struct platform_device *pdev)
> +{
> + struct tegra_devfreq *tegra;
> + struct tegra_devfreq_device *dev;
> + struct resource *res;
> + unsigned long max_freq;
> + unsigned int i;
> + int irq;
> + int err;
> +
> + tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> + if (!tegra)
> + return -ENOMEM;
> +
> + spin_lock_init(&tegra->lock);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + tegra->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(tegra->regs))
> + 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);
> + }
> +
> + err = of_init_opp_table(&pdev->dev);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to init operating point table\n");
> + return err;
> + }
> +
> + 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) {
> + dev_err(&pdev->dev,
> + "Failed to prepare and enable ACTMON clock\n");
> + 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;
> +
> + actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
> + 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, dev);
> + }
> +
> + tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
> + tegra->devfreq = devm_devfreq_add_device(&pdev->dev,
> + &tegra_devfreq_profile,
> + "tegra_actmon",
> + NULL);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq <= 0) {
> + dev_err(&pdev->dev, "Failed to get IRQ\n");
> + return -ENODEV;
> + }
> +
> + platform_set_drvdata(pdev, tegra);
> +
> + err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
> + actmon_thread_isr, IRQF_SHARED,
> + "tegra-devfreq", tegra);
> + if (err) {
> + dev_err(&pdev->dev, "Interrupt request failed\n");
> + return err;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++)
> + tegra_actmon_enable_device(tegra, tegra->devices + i);
> +
> + return 0;
> +}
> +
> +static int tegra_devfreq_remove(struct platform_device *pdev)
> +{
> + struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
> + int irq = platform_get_irq(pdev, 0);
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++)
> + tegra_actmon_disable_device(tegra, tegra->devices + i);
> +
> + devm_free_irq(&pdev->dev, irq, tegra);
> +
> + clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
> +
> + clk_disable_unprepare(tegra->clock);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +
> +static int tegra_devfreq_suspend(struct device *dev)
> +{
> + struct platform_device *pdev;
> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> + struct tegra_devfreq_device *actmon_dev;
> + unsigned int i;
> +
> + pdev = container_of(dev, struct platform_device, dev);
> +
> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> + actmon_dev = &tegra->devices[i];
> +
> + tegra_actmon_disable_device(tegra, actmon_dev);
> +
> + device_writel(actmon_dev, ACTMON_INTR_STATUS_CLEAR,
> + ACTMON_DEV_INTR_STATUS);
> +
> + actmon_write_barrier(tegra);
> + }
> +
> + return 0;
> +}
> +
> +static int tegra_devfreq_resume(struct device *dev)
> +{
> + struct platform_device *pdev;
> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> + struct tegra_devfreq_device *actmon_dev;
> + unsigned int i;
> +
> + pdev = container_of(dev, struct platform_device, dev);
> +
> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> + actmon_dev = &tegra->devices[i];
> +
> + tegra_actmon_configure_device(tegra, actmon_dev);
> + tegra_actmon_enable_device(tegra, actmon_dev);
> + }
> +
> + return 0;
> +}
> +
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static SIMPLE_DEV_PM_OPS(tegra_devfreq_pm_ops,
> + tegra_devfreq_suspend,
> + tegra_devfreq_resume);
> +
> +static struct of_device_id tegra_devfreq_of_match[] = {
> + { .compatible = "nvidia,tegra124-actmon" },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, tegra_devfreq_of_match);
> +
> +static struct platform_driver tegra_devfreq_driver = {
> + .probe = tegra_devfreq_probe,
> + .remove = tegra_devfreq_remove,
> + .driver = {
> + .name = "tegra-devfreq",
> + .of_match_table = tegra_devfreq_of_match,
> + .pm = &tegra_devfreq_pm_ops,
> + },
> +};
> +module_platform_driver(tegra_devfreq_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Tegra devfreq driver");
> +MODULE_AUTHOR("Tomeu Vizoso <tomeu.vizoso@collabora.com>");
> --
> 1.9.3
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor
2014-12-09 13:15 ` [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor Tomeu Vizoso
2014-12-12 7:44 ` Alexandre Courbot
@ 2014-12-14 18:26 ` Paul Walmsley
2014-12-15 11:04 ` Tomeu Vizoso
1 sibling, 1 reply; 8+ messages in thread
From: Paul Walmsley @ 2014-12-14 18:26 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-pm, Javier Martinez Canillas, Alexandre Courbot,
Mikko Perttunen, Arto Merilainen, MyungJoo Ham, Kyungmin Park,
Stephen Warren, Thierry Reding, Grant Likely, Rob Herring,
linux-kernel, linux-tegra, devicetree, khilman, afrid
Hi
I have not reviewed this code closely, but a few items just caught my eye
at a brief glance.
On Tue, 9 Dec 2014, Tomeu Vizoso wrote:
> 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 <afrid@nvidia.com> and Mikko
> Perttunen <mikko.perttunen@kapsi.fi>.
It's important to put people in the Cc: section, either when you're basing
your code off of their work, or when you mention them in the patch
description. This means including specific Cc: lines in the
Signed-off-by: section at the bottom of the patch -- not just mentioning
them in the patch description. That way, any further followup from others
after the patch is committed is more likely to be appropriately copied to
those people.
For some reason this doesn't happen with many Tegra upstream-bound patches
-- from a variety of submitters, including NVIDIA submitters! -- but it
needs to start happening.
Also I see that Aleks Frid wasn't cc'ed at all in the E-mail headers;
fixing at least that point.
> +static struct tegra_devfreq_device_config actmon_device_configs[] = {
> + {
> + /* MCALL: All memory accesses (including from the CPUs) */
> + .offset = 0x1c0,
> + .irq_mask = 1 << 26,
> + .boost_up_coeff = 200,
> + .boost_down_coeff = 50,
> + .boost_up_threshold = 60,
> + .boost_down_threshold = 40,
> + },
> + {
> + /* MCCPU: memory accesses from the CPUs */
> + .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,
> + },
> +};
This data represents PM policy. In other words, it is use-case sensitive.
Different users may wish to change these values depending on their
application characteristics -- and it does not represent unchangeable
hardware fact.
On the other hand...
> +static u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
> +{
> + return readl(tegra->regs + offset);
> +}
> +
> +static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
> +{
> + writel(val, tegra->regs + offset);
> +}
> +
> +static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
> +{
> + return readl(dev->regs + offset);
> +}
> +
> +static void device_writel(struct tegra_devfreq_device *dev, u32 val,
> + u32 offset)
> +{
> + writel(val, dev->regs + offset);
> +}
> +
> +static unsigned long do_percent(unsigned long val, unsigned int pct)
> +{
> + return val * pct / 100;
> +}
> +
> +static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev)
> +{
> + u32 avg = dev->avg_count;
> + u32 avg_band_freq = tegra->max_freq * ACTMON_DEFAULT_AVG_BAND / KHZ;
> + u32 band = avg_band_freq * ACTMON_SAMPLING_PERIOD;
> +
> + device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);
> +
> + avg = max(dev->avg_count, band);
> + device_writel(dev, avg - band, ACTMON_DEV_AVG_LOWER_WMARK);
> +}
> +
> +static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev)
> +{
> + u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
> +
> + device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
> + ACTMON_DEV_UPPER_WMARK);
> +
> + device_writel(dev, do_percent(val, dev->config->boost_down_threshold),
> + ACTMON_DEV_LOWER_WMARK);
> +}
> +
> +static void actmon_write_barrier(struct tegra_devfreq *tegra)
> +{
> + /* ensure the update has reached the ACTMON */
> + wmb();
> + actmon_readl(tegra, ACTMON_GLB_STATUS);
> +}
> +
> +static irqreturn_t actmon_isr(int irq, void *data)
> +{
> + struct tegra_devfreq *tegra = data;
> + struct tegra_devfreq_device *dev = NULL;
> + unsigned long flags;
> + u32 val, intr_status, dev_ctrl;
> + unsigned int i;
> +
> + val = actmon_readl(tegra, 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 = device_readl(dev, ACTMON_DEV_AVG_COUNT);
> + tegra_devfreq_update_avg_wmark(tegra, dev);
> +
> + intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
> + dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
> +
> + if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
> + /*
> + * new_boost = min(old_boost * up_coef + step, max_freq)
> + */
> + dev->boost_freq = do_percent(dev->boost_freq,
> + dev->config->boost_up_coeff);
> + dev->boost_freq += ACTMON_BOOST_FREQ_STEP;
> +
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +
> + if (dev->boost_freq >= tegra->max_freq)
> + dev->boost_freq = tegra->max_freq;
> + else
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> + } else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
> + /*
> + * 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);
> +
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +
> + if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
> + dev->boost_freq = 0;
> + else
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> + }
> +
> + if (dev->config->avg_dependency_threshold) {
> + if (dev->avg_count >= dev->config->avg_dependency_threshold)
> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> + else if (dev->boost_freq == 0)
> + dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> + }
> +
> + device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
> +
> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> +
> + actmon_write_barrier(tegra);
> +
> + spin_unlock_irqrestore(&tegra->lock, flags);
> +
> + return IRQ_WAKE_THREAD;
> +}
... all this code is clearly low level device driver code and is intended
to represent immutable hardware fact. It is use-case independent, PM
policy-invariant, and in theory should be verifiable against the Tegra TRM
(or whatever).
The policy code and data should be separated into a separate file and/or
subsystem from the low-level ACTMON device driver. The policy code should
be easily swappable or tunable by end-users without touching the
underlying device driver.
So these entities should use some kind of Linux generic subsystem/function
call interface to loosely couple the policy with the low-level device
driver. I have not combed the tree to see what makes the most sense. But
the perf subsystem comes to mind as one strong candidate.
- Paul
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor
2014-12-12 7:44 ` Alexandre Courbot
@ 2014-12-15 10:45 ` Tomeu Vizoso
0 siblings, 0 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-12-15 10:45 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-pm, Javier Martinez Canillas, Mikko Perttunen,
Arto Merilainen, MyungJoo Ham, Kyungmin Park, Stephen Warren,
Thierry Reding, Grant Likely, Rob Herring,
Linux Kernel Mailing List, linux-tegra, devicetree
On 12 December 2014 at 08:44, Alexandre Courbot <gnurou@gmail.com> wrote:
> Hi Tomeu,
>
> On Tue, Dec 9, 2014 at 10:15 PM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> 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 <afrid@nvidia.com> and Mikko
>> Perttunen <mikko.perttunen@kapsi.fi>.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>
>> ---
>>
>> v3: * Address misc. style issues found by Thierry and Alexander
>> * Added helpers for register i/o
>> * Further documented the structs
>> * Enable the ACTMON after the IRQ handler has been installed
>> * Disable the ACTMON before removing the IRQ handler
>> * Add governor in a subsys initcall
>> * Rename tegra-devfreq.c to tegra-actmon-devfreq.c
>>
>> v2: * Use devfreq
>
> This is taking shape, and although it will be desirable to port it to
> the more suitable watermark model once it gets merged, I am for
> merging this first version in the meantime as it provides a useful
> feature. Then we could use it to test-drive watermarking and argue the
> advantages of that model with impressive negative line count patches.
> ;)
Yup, agreed.
> A few comments below but I am close to giving my ack to this.
>
>> ---
>> drivers/devfreq/Kconfig | 9 +
>> drivers/devfreq/Makefile | 1 +
>> drivers/devfreq/tegra-actmon-devfreq.c | 777 +++++++++++++++++++++++++++++++++
>> 3 files changed, 787 insertions(+)
>> create mode 100644 drivers/devfreq/tegra-actmon-devfreq.c
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index faf4e70..76be33a 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -87,4 +87,13 @@ config ARM_EXYNOS5_BUS_DEVFREQ
>> It reads PPMU counters of memory controllers and adjusts the
>> operating frequencies and voltages with OPP support.
>>
>> +config ARM_TEGRA_ACTMON_DEVFREQ
>> + bool "Tegra ACTMON DEVFREQ Driver"
>> + depends on ARCH_TEGRA
>> + select PM_OPP
>> + help
>> + This adds the DEVFREQ driver for the Tegra family of SoCs.
>> + It reads ACTMON counters of memory controllers and adjusts the
>> + operating frequencies and voltages with OPP support.
>> +
>> endif # PM_DEVFREQ
>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>> index 16138c9..c9159c5 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
>> @@ -7,3 +7,4 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
>> # DEVFREQ Drivers
>> obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ) += exynos/
>> obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ) += exynos/
>> +obj-$(CONFIG_ARM_TEGRA_ACTMON_DEVFREQ) += tegra-actmon-devfreq.o
>> diff --git a/drivers/devfreq/tegra-actmon-devfreq.c b/drivers/devfreq/tegra-actmon-devfreq.c
>> new file mode 100644
>> index 0000000..8d2ea22f
>> --- /dev/null
>> +++ b/drivers/devfreq/tegra-actmon-devfreq.c
>> @@ -0,0 +1,777 @@
>> +/*
>> + * A devfreq driver for NVIDIA Tegra SoCs
>> + *
>> + * 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/devfreq.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/reset.h>
>> +
>> +#include "governor.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_INTR_STATUS_CLEAR 0xffffffff
>> +
>> +#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
>> +
>> +/* Assume that the bus is saturated if the utilization is 25% */
>> +#define BUS_SATURATION_RATIO 25
>> +
>> +/**
>> + * struct tegra_devfreq_device_config - configuration specific to an ACTMON
>> + * device
>> + *
>> + * Coefficients and thresholds are in %
>> + */
>> +struct tegra_devfreq_device_config {
>> + u32 offset;
>> + u32 irq_mask;
>> +
>> + /* Factors applied to boost_freq every consecutive watermark breach */
>> + unsigned int boost_up_coeff;
>> + unsigned int boost_down_coeff;
>> +
>> + /* Define the watermark bounds when applied to the current avg */
>> + unsigned int boost_up_threshold;
>> + unsigned int boost_down_threshold;
>> +
>> + /*
>> + * Threshold of activity below which the CPU frequency isn't to be
>> + * taken into account. This is to avoid increasing the EMC frequency
>> + * when the CPU is very busy but not accessing the bus often.
>> + */
>> + u32 avg_dependency_threshold;
>> +};
>> +
>> +enum tegra_actmon_device {
>> + MCALL = 0,
>> + MCCPU,
>> +};
>> +
>> +static struct tegra_devfreq_device_config actmon_device_configs[] = {
>> + {
>> + /* MCALL: All memory accesses (including from the CPUs) */
>> + .offset = 0x1c0,
>> + .irq_mask = 1 << 26,
>> + .boost_up_coeff = 200,
>> + .boost_down_coeff = 50,
>> + .boost_up_threshold = 60,
>> + .boost_down_threshold = 40,
>> + },
>> + {
>> + /* MCCPU: memory accesses from the CPUs */
>> + .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,
>
> The comment above says that coefficients and thresholds are in % - but
> does it apply to avg_dependency_threshold? The number looks like it is
> something else.
True.
>> + },
>> +};
>> +
>> +/**
>> + * struct tegra_devfreq_device - state specific to an ACTMON device
>> + *
>> + * Frequencies are in kHz.
>> + */
>> +struct tegra_devfreq_device {
>> + const struct tegra_devfreq_device_config *config;
>> +
>> + void __iomem *regs;
>> +
>> + /* Average event count sampled in the last interrupt */
>> + u32 avg_count;
>> +
>> + /*
>> + * Extra frequency to increase the target by due to consecutive
>> + * watermark breaches.
>> + */
>> + unsigned long boost_freq;
>> +
>> + /* Optimal frequency calculated from the stats for this device */
>> + unsigned long target_freq;
>> +};
>> +
>> +struct tegra_devfreq {
>> + struct devfreq *devfreq;
>> +
>> + struct platform_device *pdev;
>
> It seems like pdev is not used anywhere in this file.
Oops :)
>> + 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_devfreq_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 u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
>> +{
>> + return readl(tegra->regs + offset);
>> +}
>> +
>> +static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
>> +{
>> + writel(val, tegra->regs + offset);
>> +}
>> +
>> +static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
>> +{
>> + return readl(dev->regs + offset);
>> +}
>> +
>> +static void device_writel(struct tegra_devfreq_device *dev, u32 val,
>> + u32 offset)
>> +{
>> + writel(val, dev->regs + offset);
>> +}
>> +
>> +static unsigned long do_percent(unsigned long val, unsigned int pct)
>> +{
>> + return val * pct / 100;
>> +}
>> +
>> +static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>> + struct tegra_devfreq_device *dev)
>> +{
>> + u32 avg = dev->avg_count;
>> + u32 avg_band_freq = tegra->max_freq * ACTMON_DEFAULT_AVG_BAND / KHZ;
>> + u32 band = avg_band_freq * ACTMON_SAMPLING_PERIOD;
>> +
>> + device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);
>> +
>> + avg = max(dev->avg_count, band);
>> + device_writel(dev, avg - band, ACTMON_DEV_AVG_LOWER_WMARK);
>> +}
>> +
>> +static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>> + struct tegra_devfreq_device *dev)
>> +{
>> + u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
>> +
>> + device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
>> + ACTMON_DEV_UPPER_WMARK);
>> +
>> + device_writel(dev, do_percent(val, dev->config->boost_down_threshold),
>> + ACTMON_DEV_LOWER_WMARK);
>> +}
>> +
>> +static void actmon_write_barrier(struct tegra_devfreq *tegra)
>> +{
>> + /* ensure the update has reached the ACTMON */
>> + wmb();
>> + actmon_readl(tegra, ACTMON_GLB_STATUS);
>> +}
>> +
>> +static irqreturn_t actmon_isr(int irq, void *data)
>> +{
>> + struct tegra_devfreq *tegra = data;
>> + struct tegra_devfreq_device *dev = NULL;
>> + unsigned long flags;
>> + u32 val, intr_status, dev_ctrl;
>> + unsigned int i;
>> +
>> + val = actmon_readl(tegra, 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;
>> + }
>> + }
>
> Just to be sure: could it happen that we have several IRQ bits set for
> a single interrupt? If so, it seems like the current code is not
> designed to handle this.
Yeah, the TRM isn't explicit about it so I'm going to register one irq
handler per device like downstream, just in case.
>> +
>> + if (!dev)
>> + return IRQ_NONE;
>> +
>> + spin_lock_irqsave(&tegra->lock, flags);
>> +
>> + dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>> + tegra_devfreq_update_avg_wmark(tegra, dev);
>> +
>> + intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
>> + dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
>> +
>> + if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
>> + /*
>> + * new_boost = min(old_boost * up_coef + step, max_freq)
>> + */
>> + dev->boost_freq = do_percent(dev->boost_freq,
>> + dev->config->boost_up_coeff);
>> + dev->boost_freq += ACTMON_BOOST_FREQ_STEP;
>> +
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> +
>> + if (dev->boost_freq >= tegra->max_freq)
>> + dev->boost_freq = tegra->max_freq;
>> + else
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>> + } else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
>> + /*
>> + * 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);
>> +
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>> +
>> + if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
>> + dev->boost_freq = 0;
>> + else
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> + }
>> +
>> + if (dev->config->avg_dependency_threshold) {
>> + if (dev->avg_count >= dev->config->avg_dependency_threshold)
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> + else if (dev->boost_freq == 0)
>> + dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> + }
>> +
>> + device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
>> +
>> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, 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_devfreq *tegra,
>> + unsigned long cpu_freq)
>> +{
>> + unsigned 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_devfreq *tegra,
>> + struct tegra_devfreq_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);
>> +
>> + spin_unlock_irqrestore(&tegra->lock, flags);
>> +}
>> +
>> +static irqreturn_t actmon_thread_isr(int irq, void *data)
>> +{
>> + struct tegra_devfreq *tegra = data;
>> +
>> + mutex_lock(&tegra->devfreq->lock);
>> + update_devfreq(tegra->devfreq);
>> + mutex_unlock(&tegra->devfreq->lock);
>> +
>> + 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_devfreq *tegra;
>> + unsigned int i;
>> + unsigned long flags;
>> +
>> + if (action != POST_RATE_CHANGE)
>> + return NOTIFY_OK;
>> +
>> + tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
>> +
>> + spin_lock_irqsave(&tegra->lock, flags);
>> +
>> + tegra->cur_freq = data->new_rate / KHZ;
>> +
>> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
>> + tegra_devfreq_update_wmark(tegra, tegra->devices + i);
>> +
>> + actmon_write_barrier(tegra);
>> +
>> + spin_unlock_irqrestore(&tegra->lock, flags);
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> +static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>> + struct tegra_devfreq_device *dev)
>> +{
>> + u32 val = 0;
>> +
>> + dev->target_freq = tegra->cur_freq;
>> +
>> + dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
>> + device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
>> +
>> + tegra_devfreq_update_avg_wmark(tegra, dev);
>> + tegra_devfreq_update_wmark(tegra, dev);
>> +
>> + device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
>> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>> +
>> + 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;
>> +
>> + device_writel(dev, val, ACTMON_DEV_CTRL);
>> +
>> + actmon_write_barrier(tegra);
>> +}
>> +
>> +static void tegra_actmon_enable_device(struct tegra_devfreq *tegra,
>> + struct tegra_devfreq_device *dev)
>> +{
>> + u32 val;
>> +
>> + val = device_readl(dev, ACTMON_DEV_CTRL);
>> + val |= ACTMON_DEV_CTRL_ENB;
>> + device_writel(dev, val, ACTMON_DEV_CTRL);
>> +
>> + actmon_write_barrier(tegra);
>> +}
>> +
>> +static void tegra_actmon_disable_device(struct tegra_devfreq *tegra,
>> + struct tegra_devfreq_device *dev)
>> +{
>> + u32 val;
>> +
>> + val = device_readl(dev, ACTMON_DEV_CTRL);
>> + val &= ~ACTMON_DEV_CTRL_ENB;
>> + device_writel(dev, val, ACTMON_DEV_CTRL);
>> +
>> + actmon_write_barrier(tegra);
>> +}
>> +
>> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>> + u32 flags)
>> +{
>> + struct platform_device *pdev;
>> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>> + struct dev_pm_opp *opp;
>> + unsigned long rate = *freq * KHZ;
>> +
>> + pdev = container_of(dev, struct platform_device, dev);
>> +
>> + rcu_read_lock();
>> + opp = devfreq_recommended_opp(dev, &rate, flags);
>> + if (IS_ERR(opp)) {
>> + rcu_read_unlock();
>> + dev_err(dev, "Failed to find opp for %lu KHz\n", *freq);
>> + return PTR_ERR(opp);
>> + }
>> + rate = dev_pm_opp_get_freq(opp);
>> + rcu_read_unlock();
>> +
>> + /* TODO: Once we have per-user clk constraints, set a floor */
>> + clk_set_rate(tegra->emc_clock, rate);
>> +
>> + /* TODO: Set voltage as well */
>> +
>> + return 0;
>> +}
>> +
>> +static int tegra_devfreq_get_dev_status(struct device *dev,
>> + struct devfreq_dev_status *stat)
>> +{
>> + struct platform_device *pdev;
>> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>> + struct tegra_devfreq_device *actmon_dev;
>> +
>> + pdev = container_of(dev, struct platform_device, dev);
>> +
>> + stat->current_frequency = tegra->cur_freq;
>> +
>> + /* To be used by the tegra governor */
>> + stat->private_data = tegra;
>> +
>> + /* The below are to be used by the other governors */
>> +
>> + actmon_dev = &tegra->devices[MCALL];
>> +
>> + /* Number of cycles spent on memory access */
>> + stat->busy_time = actmon_dev->avg_count;
>> +
>> + /* The bus can be considered to be saturated way before 100% */
>> + stat->busy_time *= 100 / BUS_SATURATION_RATIO;
>> +
>> + /* Number of cycles in a sampling period */
>> + stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
>> +
>> + return 0;
>> +}
>> +
>> +static struct devfreq_dev_profile tegra_devfreq_profile = {
>> + .polling_ms = 0,
>> + .target = tegra_devfreq_target,
>> + .get_dev_status = tegra_devfreq_get_dev_status,
>> +};
>> +
>> +static int tegra_governor_get_target(struct devfreq *devfreq,
>> + unsigned long *freq)
>> +{
>> + struct devfreq_dev_status stat;
>> + struct tegra_devfreq *tegra;
>> + struct tegra_devfreq_device *dev;
>> + unsigned long target_freq = 0;
>> + unsigned int i;
>> + int err;
>> +
>> + err = devfreq->profile->get_dev_status(devfreq->dev.parent, &stat);
>> + if (err)
>> + return err;
>> +
>> + tegra = stat.private_data;
>> +
>> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>> + dev = &tegra->devices[i];
>> +
>> + actmon_update_target(tegra, dev);
>> +
>> + target_freq = max(target_freq, dev->target_freq);
>> + }
>> +
>> + *freq = target_freq;
>> +
>> + return 0;
>> +}
>> +
>> +static int tegra_governor_event_handler(struct devfreq *devfreq,
>> + unsigned int event, void *data)
>> +{
>> + return 0;
>> +}
>
> From my v2 review, I don't think you replied to this point:
>
> I think you will need an implementation of this.
> DEVFREQ_GOV_START/DEVFREQ_GOV_RESUME, enable actmon interrupts. On
> DEVFREQ_GOV_STOP/DEVFREQ_GOV_SUSPEND, disable interrupts. I know you
> are doing it somewhere, but it should be done here as this is what
> devfreq expects (e.g. to correctly stop interrupts if you switch from
> the "tegra" to the "performance" governor).
>
> Your suspend/resume callbacks can then call
> devfreq_suspend_device()/devfreq_resume_device().
>
> Or maybe you are keeping it this way to avoid having to store the
> devfreq_device in the governor. That's something I can understand, but
> I'm afraid it may cause issues when switching governors. Granted, the
> possibility of this happening is very small, but there is also the
> fact that we don't need/want the watermark interrupts to be fired if
> we are using a polling or fixed governor such as "performance". This
> will certainly result in an interrupt storm of some sort.
>
> But looking at it twice it seems like avg_count will never be updated
> unless the interrupts are fired, so maybe we should update that
> counter from some other place, like tegra_devfreq_get_dev_status? Does
> that sound feasible?
Yeah, need to play a bit with it, but I'm positive about it.
Thanks!
Tomeu
>> +
>> +static struct devfreq_governor tegra_devfreq_governor = {
>> + .name = "tegra_actmon",
>> + .get_target_freq = tegra_governor_get_target,
>> + .event_handler = tegra_governor_event_handler,
>> +};
>> +
>> +static int __init tegra_governor_init(void)
>> +{
>> + return devfreq_add_governor(&tegra_devfreq_governor);
>> +}
>> +subsys_initcall(tegra_governor_init);
>> +
>> +static int tegra_devfreq_probe(struct platform_device *pdev)
>> +{
>> + struct tegra_devfreq *tegra;
>> + struct tegra_devfreq_device *dev;
>> + struct resource *res;
>> + unsigned long max_freq;
>> + unsigned int i;
>> + int irq;
>> + int err;
>> +
>> + tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>> + if (!tegra)
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&tegra->lock);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + tegra->regs = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(tegra->regs))
>> + 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);
>> + }
>> +
>> + err = of_init_opp_table(&pdev->dev);
>> + if (err) {
>> + dev_err(&pdev->dev, "Failed to init operating point table\n");
>> + return err;
>> + }
>> +
>> + 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) {
>> + dev_err(&pdev->dev,
>> + "Failed to prepare and enable ACTMON clock\n");
>> + 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;
>> +
>> + actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
>> + 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, dev);
>> + }
>> +
>> + tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
>> + tegra->devfreq = devm_devfreq_add_device(&pdev->dev,
>> + &tegra_devfreq_profile,
>> + "tegra_actmon",
>> + NULL);
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq <= 0) {
>> + dev_err(&pdev->dev, "Failed to get IRQ\n");
>> + return -ENODEV;
>> + }
>> +
>> + platform_set_drvdata(pdev, tegra);
>> +
>> + err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
>> + actmon_thread_isr, IRQF_SHARED,
>> + "tegra-devfreq", tegra);
>> + if (err) {
>> + dev_err(&pdev->dev, "Interrupt request failed\n");
>> + return err;
>> + }
>> +
>> + for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++)
>> + tegra_actmon_enable_device(tegra, tegra->devices + i);
>> +
>> + return 0;
>> +}
>> +
>> +static int tegra_devfreq_remove(struct platform_device *pdev)
>> +{
>> + struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
>> + int irq = platform_get_irq(pdev, 0);
>> + unsigned int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++)
>> + tegra_actmon_disable_device(tegra, tegra->devices + i);
>> +
>> + devm_free_irq(&pdev->dev, irq, tegra);
>> +
>> + clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
>> +
>> + clk_disable_unprepare(tegra->clock);
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +
>> +static int tegra_devfreq_suspend(struct device *dev)
>> +{
>> + struct platform_device *pdev;
>> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>> + struct tegra_devfreq_device *actmon_dev;
>> + unsigned int i;
>> +
>> + pdev = container_of(dev, struct platform_device, dev);
>> +
>> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>> + actmon_dev = &tegra->devices[i];
>> +
>> + tegra_actmon_disable_device(tegra, actmon_dev);
>> +
>> + device_writel(actmon_dev, ACTMON_INTR_STATUS_CLEAR,
>> + ACTMON_DEV_INTR_STATUS);
>> +
>> + actmon_write_barrier(tegra);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tegra_devfreq_resume(struct device *dev)
>> +{
>> + struct platform_device *pdev;
>> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>> + struct tegra_devfreq_device *actmon_dev;
>> + unsigned int i;
>> +
>> + pdev = container_of(dev, struct platform_device, dev);
>> +
>> + for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>> + actmon_dev = &tegra->devices[i];
>> +
>> + tegra_actmon_configure_device(tegra, actmon_dev);
>> + tegra_actmon_enable_device(tegra, actmon_dev);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>> +static SIMPLE_DEV_PM_OPS(tegra_devfreq_pm_ops,
>> + tegra_devfreq_suspend,
>> + tegra_devfreq_resume);
>> +
>> +static struct of_device_id tegra_devfreq_of_match[] = {
>> + { .compatible = "nvidia,tegra124-actmon" },
>> + { },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, tegra_devfreq_of_match);
>> +
>> +static struct platform_driver tegra_devfreq_driver = {
>> + .probe = tegra_devfreq_probe,
>> + .remove = tegra_devfreq_remove,
>> + .driver = {
>> + .name = "tegra-devfreq",
>> + .of_match_table = tegra_devfreq_of_match,
>> + .pm = &tegra_devfreq_pm_ops,
>> + },
>> +};
>> +module_platform_driver(tegra_devfreq_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Tegra devfreq driver");
>> +MODULE_AUTHOR("Tomeu Vizoso <tomeu.vizoso@collabora.com>");
>> --
>> 1.9.3
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor
2014-12-14 18:26 ` Paul Walmsley
@ 2014-12-15 11:04 ` Tomeu Vizoso
0 siblings, 0 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-12-15 11:04 UTC (permalink / raw)
To: Paul Walmsley
Cc: linux-pm, Javier Martinez Canillas, Alexandre Courbot,
Mikko Perttunen, Arto Merilainen, MyungJoo Ham, Kyungmin Park,
Stephen Warren, Thierry Reding, Grant Likely, Rob Herring,
linux-kernel, linux-tegra, devicetree, khilman, afrid
On 14 December 2014 at 19:26, Paul Walmsley <paul@pwsan.com> wrote:
> Hi
>
> I have not reviewed this code closely, but a few items just caught my eye
> at a brief glance.
>
> On Tue, 9 Dec 2014, Tomeu Vizoso wrote:
>
>> 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 <afrid@nvidia.com> and Mikko
>> Perttunen <mikko.perttunen@kapsi.fi>.
>
> It's important to put people in the Cc: section, either when you're basing
> your code off of their work, or when you mention them in the patch
> description. This means including specific Cc: lines in the
> Signed-off-by: section at the bottom of the patch -- not just mentioning
> them in the patch description. That way, any further followup from others
> after the patch is committed is more likely to be appropriately copied to
> those people.
>
> For some reason this doesn't happen with many Tegra upstream-bound patches
> -- from a variety of submitters, including NVIDIA submitters! -- but it
> needs to start happening.
>
> Also I see that Aleks Frid wasn't cc'ed at all in the E-mail headers;
> fixing at least that point.
Point taken.
>> +static struct tegra_devfreq_device_config actmon_device_configs[] = {
>> + {
>> + /* MCALL: All memory accesses (including from the CPUs) */
>> + .offset = 0x1c0,
>> + .irq_mask = 1 << 26,
>> + .boost_up_coeff = 200,
>> + .boost_down_coeff = 50,
>> + .boost_up_threshold = 60,
>> + .boost_down_threshold = 40,
>> + },
>> + {
>> + /* MCCPU: memory accesses from the CPUs */
>> + .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,
>> + },
>> +};
>
> This data represents PM policy. In other words, it is use-case sensitive.
> Different users may wish to change these values depending on their
> application characteristics -- and it does not represent unchangeable
> hardware fact.
>
> On the other hand...
>
>> +static u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
>> +{
>> + return readl(tegra->regs + offset);
>> +}
>> +
>> +static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
>> +{
>> + writel(val, tegra->regs + offset);
>> +}
>> +
>> +static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
>> +{
>> + return readl(dev->regs + offset);
>> +}
>> +
>> +static void device_writel(struct tegra_devfreq_device *dev, u32 val,
>> + u32 offset)
>> +{
>> + writel(val, dev->regs + offset);
>> +}
>> +
>> +static unsigned long do_percent(unsigned long val, unsigned int pct)
>> +{
>> + return val * pct / 100;
>> +}
>> +
>> +static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>> + struct tegra_devfreq_device *dev)
>> +{
>> + u32 avg = dev->avg_count;
>> + u32 avg_band_freq = tegra->max_freq * ACTMON_DEFAULT_AVG_BAND / KHZ;
>> + u32 band = avg_band_freq * ACTMON_SAMPLING_PERIOD;
>> +
>> + device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);
>> +
>> + avg = max(dev->avg_count, band);
>> + device_writel(dev, avg - band, ACTMON_DEV_AVG_LOWER_WMARK);
>> +}
>> +
>> +static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>> + struct tegra_devfreq_device *dev)
>> +{
>> + u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
>> +
>> + device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
>> + ACTMON_DEV_UPPER_WMARK);
>> +
>> + device_writel(dev, do_percent(val, dev->config->boost_down_threshold),
>> + ACTMON_DEV_LOWER_WMARK);
>> +}
>> +
>> +static void actmon_write_barrier(struct tegra_devfreq *tegra)
>> +{
>> + /* ensure the update has reached the ACTMON */
>> + wmb();
>> + actmon_readl(tegra, ACTMON_GLB_STATUS);
>> +}
>> +
>> +static irqreturn_t actmon_isr(int irq, void *data)
>> +{
>> + struct tegra_devfreq *tegra = data;
>> + struct tegra_devfreq_device *dev = NULL;
>> + unsigned long flags;
>> + u32 val, intr_status, dev_ctrl;
>> + unsigned int i;
>> +
>> + val = actmon_readl(tegra, 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 = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>> + tegra_devfreq_update_avg_wmark(tegra, dev);
>> +
>> + intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
>> + dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
>> +
>> + if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
>> + /*
>> + * new_boost = min(old_boost * up_coef + step, max_freq)
>> + */
>> + dev->boost_freq = do_percent(dev->boost_freq,
>> + dev->config->boost_up_coeff);
>> + dev->boost_freq += ACTMON_BOOST_FREQ_STEP;
>> +
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> +
>> + if (dev->boost_freq >= tegra->max_freq)
>> + dev->boost_freq = tegra->max_freq;
>> + else
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>> + } else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
>> + /*
>> + * 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);
>> +
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>> +
>> + if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
>> + dev->boost_freq = 0;
>> + else
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> + }
>> +
>> + if (dev->config->avg_dependency_threshold) {
>> + if (dev->avg_count >= dev->config->avg_dependency_threshold)
>> + dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> + else if (dev->boost_freq == 0)
>> + dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>> + }
>> +
>> + device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
>> +
>> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>> +
>> + actmon_write_barrier(tegra);
>> +
>> + spin_unlock_irqrestore(&tegra->lock, flags);
>> +
>> + return IRQ_WAKE_THREAD;
>> +}
>
> ... all this code is clearly low level device driver code and is intended
> to represent immutable hardware fact. It is use-case independent, PM
> policy-invariant, and in theory should be verifiable against the Tegra TRM
> (or whatever).
>
> The policy code and data should be separated into a separate file and/or
> subsystem from the low-level ACTMON device driver. The policy code should
> be easily swappable or tunable by end-users without touching the
> underlying device driver.
>
> So these entities should use some kind of Linux generic subsystem/function
> call interface to loosely couple the policy with the low-level device
> driver. I have not combed the tree to see what makes the most sense. But
> the perf subsystem comes to mind as one strong candidate.
I agree with your concern, but I'm wary to expose this to userspace at
this point in time because we hope for devfreq to eventually gain
better support for these kind of activity monitors and having to
maintain compatibility would limit us for little gain. Besides, we
don't have yet a good idea of how userspace would use these knobs
(both chromeos and l4t seem to be happy to just leave those defaults
as they are now).
We need to hardcode some sane defaults anyway, so I think that it
makes sense to leave things as they are until we have one more SoC
with similar functionality, and some idea of how userspace would want
to tweak them.
Regards,
Tomeu
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-12-15 11:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-09 13:15 [PATCH v4 0/3] Add support for Tegra Activity Monitor Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 1/3] of: Add binding for NVIDIA Tegra ACTMON node Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor Tomeu Vizoso
2014-12-12 7:44 ` Alexandre Courbot
2014-12-15 10:45 ` Tomeu Vizoso
2014-12-14 18:26 ` Paul Walmsley
2014-12-15 11:04 ` Tomeu Vizoso
2014-12-09 13:15 ` [PATCH v4 3/3] ARM: tegra: Add Tegra124 ACTMON support Tomeu Vizoso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).