Changelog:
v4: Addressed all review comments that were made by Chanwoo Choi to v3:
- changed the driver removal order to match the probe exactly
- added clarifying comment for 1/8 ratio to the Tegra20 driver
Chanwoo, please also note that the clk patch that should fix
compilation problem that was reported the kbuild-test-robot is already
applied and available in the recent linux-next.
v3: Addressed all review comments that were made by Chanwoo Choi to v2.
Patch "Synchronize IRQ after masking it in hardware" morphed into
"Properly disable interrupts", which disables interrupts more solidly.
Added new minor patch: "Rename tegra-devfreq.c to tegra30-devfreq.c".
Added missed error handlings for dev_pm_opp_add().
v2: The patchset was quite heavily reworked since v1, few patches we
dropped or squashed into the new ones and more patches we added.
In a result more bugs and potential problems are fixed now, driver's
code got more clean up.
The Tegra20 driver-addition patch is now a part of this series, it has
no changes since v1.
Dmitry Osipenko (16):
PM / devfreq: tegra: Fix kHz to Hz conversion
PM / devfreq: tegra: Replace readl-writel with relaxed versions
PM / devfreq: tegra: Replace write memory barrier with the read
barrier
PM / devfreq: tegra: Don't ignore clk errors
PM / devfreq: tegra: Don't set EMC clock rate to maximum on probe
PM / devfreq: tegra: Drop primary interrupt handler
PM / devfreq: tegra: Properly disable interrupts
PM / devfreq: tegra: Clean up driver's probe / remove
PM / devfreq: tegra: Avoid inconsistency of current frequency value
PM / devfreq: tegra: Mark ACTMON's governor as immutable
PM / devfreq: tegra: Move governor registration to driver's probe
PM / devfreq: tegra: Reconfigure hardware on governor's restart
PM / devfreq: tegra: Support Tegra30
PM / devfreq: tegra: Enable COMPILE_TEST for the driver
PM / devfreq: tegra: Rename tegra-devfreq.c to tegra30-devfreq.c
PM / devfreq: Introduce driver for NVIDIA Tegra20
MAINTAINERS | 8 +
drivers/devfreq/Kconfig | 15 +-
drivers/devfreq/Makefile | 3 +-
drivers/devfreq/tegra20-devfreq.c | 212 ++++++++++++
.../{tegra-devfreq.c => tegra30-devfreq.c} | 315 ++++++++----------
5 files changed, 379 insertions(+), 174 deletions(-)
create mode 100644 drivers/devfreq/tegra20-devfreq.c
rename drivers/devfreq/{tegra-devfreq.c => tegra30-devfreq.c} (81%)
--
2.21.0
The kHz to Hz is incorrectly converted in a few places in the code,
this results in a wrong frequency being calculated because devfreq core
uses OPP frequencies that are given in Hz to clamp the rate, while
tegra-devfreq gives to the core value in kHz and then it also expects to
receive value in kHz from the core. In a result memory freq is always set
to a value which is close to ULONG_MAX because of the bug. Hence the EMC
frequency is always capped to the maximum and the driver doesn't do
anything useful. This patch was tested on Tegra30 and Tegra124 SoC's, EMC
frequency scaling works properly now.
Cc: <[email protected]> # 4.14+
Tested-by: Steev Klimaszewski <[email protected]>
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index c89ba7b834ff..43cd1233f87b 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -486,11 +486,11 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
{
struct tegra_devfreq *tegra = dev_get_drvdata(dev);
struct dev_pm_opp *opp;
- unsigned long rate = *freq * KHZ;
+ unsigned long rate;
- opp = devfreq_recommended_opp(dev, &rate, flags);
+ opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(opp)) {
- dev_err(dev, "Failed to find opp for %lu KHz\n", *freq);
+ dev_err(dev, "Failed to find opp for %lu Hz\n", *freq);
return PTR_ERR(opp);
}
rate = dev_pm_opp_get_freq(opp);
@@ -499,8 +499,6 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
clk_set_min_rate(tegra->emc_clock, rate);
clk_set_rate(tegra->emc_clock, 0);
- *freq = rate;
-
return 0;
}
@@ -510,7 +508,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
struct tegra_devfreq *tegra = dev_get_drvdata(dev);
struct tegra_devfreq_device *actmon_dev;
- stat->current_frequency = tegra->cur_freq;
+ stat->current_frequency = tegra->cur_freq * KHZ;
/* To be used by the tegra governor */
stat->private_data = tegra;
@@ -565,7 +563,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
target_freq = max(target_freq, dev->target_freq);
}
- *freq = target_freq;
+ *freq = target_freq * KHZ;
return 0;
}
--
2.21.0
There is no need to insert memory barrier on each readl/writel
invocation, hence use the relaxed versions.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 43cd1233f87b..f7378a42d184 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -191,23 +191,23 @@ static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
static u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
{
- return readl(tegra->regs + offset);
+ return readl_relaxed(tegra->regs + offset);
}
static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
{
- writel(val, tegra->regs + offset);
+ writel_relaxed(val, tegra->regs + offset);
}
static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
{
- return readl(dev->regs + offset);
+ return readl_relaxed(dev->regs + offset);
}
static void device_writel(struct tegra_devfreq_device *dev, u32 val,
u32 offset)
{
- writel(val, dev->regs + offset);
+ writel_relaxed(val, dev->regs + offset);
}
static unsigned long do_percent(unsigned long val, unsigned int pct)
--
2.21.0
The clk_set_min_rate() could fail and in this case clk_set_rate() sets
rate to 0, which may drop EMC rate to minimum and make machine very
difficult to use.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 7d7b9a5895b2..c7428c5eee23 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -484,8 +484,10 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
u32 flags)
{
struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+ struct devfreq *devfreq = tegra->devfreq;
struct dev_pm_opp *opp;
unsigned long rate;
+ int err;
opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(opp)) {
@@ -495,10 +497,20 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
rate = dev_pm_opp_get_freq(opp);
dev_pm_opp_put(opp);
- clk_set_min_rate(tegra->emc_clock, rate);
- clk_set_rate(tegra->emc_clock, 0);
+ err = clk_set_min_rate(tegra->emc_clock, rate);
+ if (err)
+ return err;
+
+ err = clk_set_rate(tegra->emc_clock, 0);
+ if (err)
+ goto restore_min_rate;
return 0;
+
+restore_min_rate:
+ clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq);
+
+ return err;
}
static int tegra_devfreq_get_dev_status(struct device *dev,
--
2.21.0
There is no real benefit from doing so, hence let's drop that rate setting
for consistency.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index c7428c5eee23..24ec65556c39 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -653,8 +653,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
return PTR_ERR(tegra->emc_clock);
}
- clk_set_rate(tegra->emc_clock, ULONG_MAX);
-
tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
if (err) {
--
2.21.0
There is no real need in the primary interrupt handler, hence move
everything to the secondary (threaded) handler. In a result locking
is consistent now and there are no potential races with the interrupt
handler because it is protected with the devfreq's mutex.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 55 +++++++++++----------------------
1 file changed, 18 insertions(+), 37 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 24ec65556c39..b65313fe3c2e 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -144,7 +144,6 @@ static struct tegra_devfreq_device_config actmon_device_configs[] = {
struct tegra_devfreq_device {
const struct tegra_devfreq_device_config *config;
void __iomem *regs;
- spinlock_t lock;
/* Average event count sampled in the last interrupt */
u32 avg_count;
@@ -249,11 +248,8 @@ static void actmon_write_barrier(struct tegra_devfreq *tegra)
static void actmon_isr_device(struct tegra_devfreq *tegra,
struct tegra_devfreq_device *dev)
{
- unsigned long flags;
u32 intr_status, dev_ctrl;
- spin_lock_irqsave(&dev->lock, flags);
-
dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
tegra_devfreq_update_avg_wmark(tegra, dev);
@@ -302,26 +298,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
actmon_write_barrier(tegra);
-
- spin_unlock_irqrestore(&dev->lock, flags);
-}
-
-static irqreturn_t actmon_isr(int irq, void *data)
-{
- struct tegra_devfreq *tegra = data;
- bool handled = false;
- unsigned int i;
- u32 val;
-
- val = actmon_readl(tegra, ACTMON_GLB_STATUS);
- for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
- if (val & tegra->devices[i].config->irq_mask) {
- actmon_isr_device(tegra, tegra->devices + i);
- handled = true;
- }
- }
-
- return handled ? IRQ_WAKE_THREAD : IRQ_NONE;
}
static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
@@ -348,15 +324,12 @@ static void actmon_update_target(struct tegra_devfreq *tegra,
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(&dev->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);
@@ -364,19 +337,31 @@ static void actmon_update_target(struct tegra_devfreq *tegra,
if (dev->avg_count >= dev->config->avg_dependency_threshold)
dev->target_freq = max(dev->target_freq, static_cpu_emc_freq);
-
- spin_unlock_irqrestore(&dev->lock, flags);
}
static irqreturn_t actmon_thread_isr(int irq, void *data)
{
struct tegra_devfreq *tegra = data;
+ bool handled = false;
+ unsigned int i;
+ u32 val;
mutex_lock(&tegra->devfreq->lock);
- update_devfreq(tegra->devfreq);
+
+ val = actmon_readl(tegra, ACTMON_GLB_STATUS);
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
+ if (val & tegra->devices[i].config->irq_mask) {
+ actmon_isr_device(tegra, tegra->devices + i);
+ handled = true;
+ }
+ }
+
+ if (handled)
+ update_devfreq(tegra->devfreq);
+
mutex_unlock(&tegra->devfreq->lock);
- return IRQ_HANDLED;
+ return handled ? IRQ_HANDLED : IRQ_NONE;
}
static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
@@ -386,7 +371,6 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
struct tegra_devfreq *tegra;
struct tegra_devfreq_device *dev;
unsigned int i;
- unsigned long flags;
if (action != POST_RATE_CHANGE)
return NOTIFY_OK;
@@ -398,9 +382,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
dev = &tegra->devices[i];
- spin_lock_irqsave(&dev->lock, flags);
tegra_devfreq_update_wmark(tegra, dev);
- spin_unlock_irqrestore(&dev->lock, flags);
}
actmon_write_barrier(tegra);
@@ -682,7 +664,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
dev = tegra->devices + i;
dev->config = actmon_device_configs + i;
dev->regs = tegra->regs + dev->config->offset;
- spin_lock_init(&dev->lock);
tegra_actmon_configure_device(tegra, dev);
}
@@ -700,8 +681,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, tegra);
- err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
- actmon_thread_isr, IRQF_SHARED,
+ err = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ actmon_thread_isr, IRQF_ONESHOT,
"tegra-devfreq", tegra);
if (err) {
dev_err(&pdev->dev, "Interrupt request failed\n");
--
2.21.0
There is no guarantee that interrupt handling isn't running in parallel
with tegra_actmon_disable_interrupts(), hence it is necessary to protect
DEV_CTRL register accesses and clear IRQ status with ACTMON's IRQ being
disabled in the Interrupt Controller in order to ensure that device
interrupt is indeed being disabled.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index b65313fe3c2e..ce1eb97a2090 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -171,6 +171,8 @@ struct tegra_devfreq {
struct notifier_block rate_change_nb;
struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
+
+ int irq;
};
struct tegra_actmon_emc_ratio {
@@ -417,6 +419,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
u32 val;
unsigned int i;
+ disable_irq(tegra->irq);
+
for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
dev = &tegra->devices[i];
@@ -427,9 +431,14 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
device_writel(dev, val, ACTMON_DEV_CTRL);
+
+ device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
+ ACTMON_DEV_INTR_STATUS);
}
actmon_write_barrier(tegra);
+
+ enable_irq(tegra->irq);
}
static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
@@ -604,7 +613,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
struct resource *res;
unsigned int i;
unsigned long rate;
- int irq;
int err;
tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
@@ -673,15 +681,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
dev_pm_opp_add(&pdev->dev, rate, 0);
}
- irq = platform_get_irq(pdev, 0);
- if (irq < 0) {
- dev_err(&pdev->dev, "Failed to get IRQ: %d\n", irq);
- return irq;
+ tegra->irq = platform_get_irq(pdev, 0);
+ if (tegra->irq < 0) {
+ err = tegra->irq;
+ dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
+ return err;
}
platform_set_drvdata(pdev, tegra);
- err = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
actmon_thread_isr, IRQF_ONESHOT,
"tegra-devfreq", tegra);
if (err) {
--
2.21.0
Reset hardware, disable ACTMON clock, release OPP's and handle all
possible error cases correctly, maintaining the correct tear down
order. Also use devm_platform_ioremap_resource() which is now available
in the kernel.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 83 +++++++++++++++++++--------------
1 file changed, 47 insertions(+), 36 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index ce1eb97a2090..ea0da05cd7f2 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -610,7 +610,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
{
struct tegra_devfreq *tegra;
struct tegra_devfreq_device *dev;
- struct resource *res;
unsigned int i;
unsigned long rate;
int err;
@@ -619,9 +618,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
if (!tegra)
return -ENOMEM;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
- tegra->regs = devm_ioremap_resource(&pdev->dev, res);
+ tegra->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(tegra->regs))
return PTR_ERR(tegra->regs);
@@ -643,11 +640,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
return PTR_ERR(tegra->emc_clock);
}
- tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
- err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
- if (err) {
- dev_err(&pdev->dev,
- "Failed to register rate change notifier\n");
+ tegra->irq = platform_get_irq(pdev, 0);
+ if (tegra->irq < 0) {
+ err = tegra->irq;
+ dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
return err;
}
@@ -678,54 +674,69 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
rate = clk_round_rate(tegra->emc_clock, rate);
- dev_pm_opp_add(&pdev->dev, rate, 0);
- }
- tegra->irq = platform_get_irq(pdev, 0);
- if (tegra->irq < 0) {
- err = tegra->irq;
- dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
- return err;
+ err = dev_pm_opp_add(&pdev->dev, rate, 0);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
+ goto remove_opps;
+ }
}
platform_set_drvdata(pdev, tegra);
+ 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");
+ goto remove_opps;
+ }
+
+ tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
+ tegra->devfreq = devfreq_add_device(&pdev->dev,
+ &tegra_devfreq_profile,
+ "tegra_actmon",
+ NULL);
+ if (IS_ERR(tegra->devfreq)) {
+ err = PTR_ERR(tegra->devfreq);
+ goto unreg_notifier;
+ }
+
err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
actmon_thread_isr, IRQF_ONESHOT,
"tegra-devfreq", tegra);
if (err) {
- dev_err(&pdev->dev, "Interrupt request failed\n");
- return err;
+ dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
+ goto remove_devfreq;
}
- 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);
-
return 0;
+
+remove_devfreq:
+ devfreq_remove_device(tegra->devfreq);
+
+unreg_notifier:
+ clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
+
+remove_opps:
+ dev_pm_opp_remove_all_dynamic(&pdev->dev);
+
+ reset_control_reset(tegra->reset);
+ clk_disable_unprepare(tegra->clock);
+
+ return err;
}
static int tegra_devfreq_remove(struct platform_device *pdev)
{
struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
- int irq = platform_get_irq(pdev, 0);
- u32 val;
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
- val = device_readl(&tegra->devices[i], ACTMON_DEV_CTRL);
- val &= ~ACTMON_DEV_CTRL_ENB;
- device_writel(&tegra->devices[i], val, ACTMON_DEV_CTRL);
- }
-
- actmon_write_barrier(tegra);
- devm_free_irq(&pdev->dev, irq, tegra);
+ devfreq_remove_device(tegra->devfreq);
clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
+ dev_pm_opp_remove_all_dynamic(&pdev->dev);
+ reset_control_reset(tegra->reset);
clk_disable_unprepare(tegra->clock);
return 0;
--
2.21.0
There is no need to register the ACTMON's governor separately from
the driver, hence let's move the registration into the driver's probe
function for consistency and to make code cleaner a tad.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 43 +++++++++------------------------
1 file changed, 12 insertions(+), 31 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 65b3a7c72293..40ce9881f144 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -695,6 +695,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
goto remove_opps;
}
+ err = devfreq_add_governor(&tegra_devfreq_governor);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to add governor: %d\n", err);
+ goto unreg_notifier;
+ }
+
tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
tegra->devfreq = devfreq_add_device(&pdev->dev,
&tegra_devfreq_profile,
@@ -702,7 +708,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
NULL);
if (IS_ERR(tegra->devfreq)) {
err = PTR_ERR(tegra->devfreq);
- goto unreg_notifier;
+ goto remove_governor;
}
err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
@@ -718,6 +724,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
remove_devfreq:
devfreq_remove_device(tegra->devfreq);
+remove_governor:
+ devfreq_remove_governor(&tegra_devfreq_governor);
+
unreg_notifier:
clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
@@ -735,6 +744,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
devfreq_remove_device(tegra->devfreq);
+ devfreq_remove_governor(&tegra_devfreq_governor);
clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
dev_pm_opp_remove_all_dynamic(&pdev->dev);
@@ -760,36 +770,7 @@ static struct platform_driver tegra_devfreq_driver = {
.of_match_table = tegra_devfreq_of_match,
},
};
-
-static int __init tegra_devfreq_init(void)
-{
- int ret = 0;
-
- ret = devfreq_add_governor(&tegra_devfreq_governor);
- if (ret) {
- pr_err("%s: failed to add governor: %d\n", __func__, ret);
- return ret;
- }
-
- ret = platform_driver_register(&tegra_devfreq_driver);
- if (ret)
- devfreq_remove_governor(&tegra_devfreq_governor);
-
- return ret;
-}
-module_init(tegra_devfreq_init)
-
-static void __exit tegra_devfreq_exit(void)
-{
- int ret = 0;
-
- platform_driver_unregister(&tegra_devfreq_driver);
-
- ret = devfreq_remove_governor(&tegra_devfreq_governor);
- if (ret)
- pr_err("%s: failed to remove governor: %d\n", __func__, ret);
-}
-module_exit(tegra_devfreq_exit)
+module_platform_driver(tegra_devfreq_driver);
MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("Tegra devfreq driver");
--
2.21.0
Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
reads out Memory Controller counters and adjusts memory frequency based
on the memory clients activity.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
MAINTAINERS | 8 ++
drivers/devfreq/Kconfig | 10 ++
drivers/devfreq/Makefile | 1 +
drivers/devfreq/tegra20-devfreq.c | 212 ++++++++++++++++++++++++++++++
4 files changed, 231 insertions(+)
create mode 100644 drivers/devfreq/tegra20-devfreq.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 98edc38bfd7b..e7e434f74038 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10098,6 +10098,14 @@ F: include/linux/memblock.h
F: mm/memblock.c
F: Documentation/core-api/boot-time-mm.rst
+MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
+M: Dmitry Osipenko <[email protected]>
+L: [email protected]
+L: [email protected]
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
+S: Maintained
+F: drivers/devfreq/tegra20-devfreq.c
+
MEMORY MANAGEMENT
L: [email protected]
W: http://www.linux-mm.org
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index a6bba6e1e7d9..1530dbefa31f 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ
It reads ACTMON counters of memory controllers and adjusts the
operating frequencies and voltages with OPP support.
+config ARM_TEGRA20_DEVFREQ
+ tristate "NVIDIA Tegra20 DEVFREQ Driver"
+ depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
+ select DEVFREQ_GOV_SIMPLE_ONDEMAND
+ select PM_OPP
+ help
+ This adds the DEVFREQ driver for the Tegra20 family of SoCs.
+ It reads Memory Controller counters and adjusts the operating
+ frequencies and voltages with OPP support.
+
config ARM_RK3399_DMC_DEVFREQ
tristate "ARM RK3399 DMC DEVFREQ Driver"
depends on ARCH_ROCKCHIP
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 47e5aeeebfd1..338ae8440db6 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
+obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o
# DEVFREQ Event Drivers
obj-$(CONFIG_PM_DEVFREQ_EVENT) += event/
diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
new file mode 100644
index 000000000000..ff82bac9ee4e
--- /dev/null
+++ b/drivers/devfreq/tegra20-devfreq.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NVIDIA Tegra20 devfreq driver
+ *
+ * Copyright (C) 2019 GRATE-DRIVER project
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/slab.h>
+
+#include <soc/tegra/mc.h>
+
+#include "governor.h"
+
+#define MC_STAT_CONTROL 0x90
+#define MC_STAT_EMC_CLOCK_LIMIT 0xa0
+#define MC_STAT_EMC_CLOCKS 0xa4
+#define MC_STAT_EMC_CONTROL 0xa8
+#define MC_STAT_EMC_COUNT 0xb8
+
+#define EMC_GATHER_CLEAR (1 << 8)
+#define EMC_GATHER_ENABLE (3 << 8)
+
+struct tegra_devfreq {
+ struct devfreq *devfreq;
+ struct clk *emc_clock;
+ void __iomem *regs;
+};
+
+static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
+ u32 flags)
+{
+ struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+ struct devfreq *devfreq = tegra->devfreq;
+ struct dev_pm_opp *opp;
+ unsigned long rate;
+ int err;
+
+ opp = devfreq_recommended_opp(dev, freq, flags);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ rate = dev_pm_opp_get_freq(opp);
+ dev_pm_opp_put(opp);
+
+ err = clk_set_min_rate(tegra->emc_clock, rate);
+ if (err)
+ return err;
+
+ err = clk_set_rate(tegra->emc_clock, 0);
+ if (err)
+ goto restore_min_rate;
+
+ return 0;
+
+restore_min_rate:
+ clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq);
+
+ return err;
+}
+
+static int tegra_devfreq_get_dev_status(struct device *dev,
+ struct devfreq_dev_status *stat)
+{
+ struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+
+ /*
+ * EMC_COUNT returns number of memory events, that number is lower
+ * than the number of clocks. Conversion ratio of 1/8 results in a
+ * bit higher bandwidth than actually needed, it is good enough for
+ * the time being because drivers don't support requesting minimum
+ * needed memory bandwidth yet.
+ *
+ * TODO: adjust the ratio value once relevant drivers will support
+ * memory bandwidth management.
+ */
+ stat->busy_time = readl_relaxed(tegra->regs + MC_STAT_EMC_COUNT);
+ stat->total_time = readl_relaxed(tegra->regs + MC_STAT_EMC_CLOCKS) / 8;
+ stat->current_frequency = clk_get_rate(tegra->emc_clock);
+
+ writel_relaxed(EMC_GATHER_CLEAR, tegra->regs + MC_STAT_CONTROL);
+ writel_relaxed(EMC_GATHER_ENABLE, tegra->regs + MC_STAT_CONTROL);
+
+ return 0;
+}
+
+static struct devfreq_dev_profile tegra_devfreq_profile = {
+ .polling_ms = 500,
+ .target = tegra_devfreq_target,
+ .get_dev_status = tegra_devfreq_get_dev_status,
+};
+
+static struct tegra_mc *tegra_get_memory_controller(void)
+{
+ struct platform_device *pdev;
+ struct device_node *np;
+ struct tegra_mc *mc;
+
+ np = of_find_compatible_node(NULL, NULL, "nvidia,tegra20-mc-gart");
+ if (!np)
+ return ERR_PTR(-ENOENT);
+
+ pdev = of_find_device_by_node(np);
+ of_node_put(np);
+ if (!pdev)
+ return ERR_PTR(-ENODEV);
+
+ mc = platform_get_drvdata(pdev);
+ if (!mc)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ return mc;
+}
+
+static int tegra_devfreq_probe(struct platform_device *pdev)
+{
+ struct tegra_devfreq *tegra;
+ struct tegra_mc *mc;
+ unsigned long max_rate;
+ unsigned long rate;
+ int err;
+
+ mc = tegra_get_memory_controller();
+ if (IS_ERR(mc)) {
+ err = PTR_ERR(mc);
+ dev_err(&pdev->dev, "failed to get memory controller: %d\n",
+ err);
+ return err;
+ }
+
+ tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+ if (!tegra)
+ return -ENOMEM;
+
+ /* EMC is a system-critical clock that is always enabled */
+ tegra->emc_clock = devm_clk_get(&pdev->dev, "emc");
+ if (IS_ERR(tegra->emc_clock)) {
+ err = PTR_ERR(tegra->emc_clock);
+ dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
+ return err;
+ }
+
+ tegra->regs = mc->regs;
+
+ max_rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
+
+ for (rate = 0; rate <= max_rate; rate++) {
+ rate = clk_round_rate(tegra->emc_clock, rate);
+
+ err = dev_pm_opp_add(&pdev->dev, rate, 0);
+ if (err) {
+ dev_err(&pdev->dev, "failed to add opp: %d\n", err);
+ goto remove_opps;
+ }
+ }
+
+ /*
+ * Reset statistic gathers state, select global bandwidth for the
+ * statistics collection mode and set clocks counter saturation
+ * limit to maximum.
+ */
+ writel_relaxed(0x00000000, tegra->regs + MC_STAT_CONTROL);
+ writel_relaxed(0x00000000, tegra->regs + MC_STAT_EMC_CONTROL);
+ writel_relaxed(0xffffffff, tegra->regs + MC_STAT_EMC_CLOCK_LIMIT);
+
+ platform_set_drvdata(pdev, tegra);
+
+ tegra->devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
+ DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
+ if (IS_ERR(tegra->devfreq)) {
+ err = PTR_ERR(tegra->devfreq);
+ goto remove_opps;
+ }
+
+ return 0;
+
+remove_opps:
+ dev_pm_opp_remove_all_dynamic(&pdev->dev);
+
+ return err;
+}
+
+static int tegra_devfreq_remove(struct platform_device *pdev)
+{
+ struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
+
+ devfreq_remove_device(tegra->devfreq);
+ dev_pm_opp_remove_all_dynamic(&pdev->dev);
+
+ return 0;
+}
+
+static struct platform_driver tegra_devfreq_driver = {
+ .probe = tegra_devfreq_probe,
+ .remove = tegra_devfreq_remove,
+ .driver = {
+ .name = "tegra20-devfreq",
+ },
+};
+module_platform_driver(tegra_devfreq_driver);
+
+MODULE_ALIAS("platform:tegra20-devfreq");
+MODULE_AUTHOR("Dmitry Osipenko <[email protected]>");
+MODULE_DESCRIPTION("NVIDIA Tegra20 devfreq driver");
+MODULE_LICENSE("GPL v2");
--
2.21.0
The devfreq driver can be used on Tegra30 without any code change and
it works perfectly fine, the default Tegra124 parameters are good enough
for Tegra30.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/Kconfig | 4 ++--
drivers/devfreq/tegra-devfreq.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index a78dffe603c1..56db9dc05edb 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -92,8 +92,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
This does not yet operate with optimal voltages.
config ARM_TEGRA_DEVFREQ
- tristate "Tegra DEVFREQ Driver"
- depends on ARCH_TEGRA_124_SOC
+ tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
+ depends on ARCH_TEGRA
select PM_OPP
help
This adds the DEVFREQ driver for the Tegra family of SoCs.
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 2d9d53daedd8..dd0fbd2c8e04 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -738,6 +738,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
}
static const struct of_device_id tegra_devfreq_of_match[] = {
+ { .compatible = "nvidia,tegra30-actmon" },
{ .compatible = "nvidia,tegra124-actmon" },
{ },
};
--
2.21.0
The driver's compilation doesn't have any specific dependencies, hence
the COMPILE_TEST option can be supported in Kconfig.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 56db9dc05edb..a6bba6e1e7d9 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
config ARM_TEGRA_DEVFREQ
tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
- depends on ARCH_TEGRA
+ depends on ARCH_TEGRA || COMPILE_TEST
select PM_OPP
help
This adds the DEVFREQ driver for the Tegra family of SoCs.
--
2.21.0
The frequency value potentially could change in-between. It doesn't
cause any real problem at all right now, but that could change in the
future. Hence let's avoid the inconsistency.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index ea0da05cd7f2..5265d735419f 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -509,13 +509,15 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
{
struct tegra_devfreq *tegra = dev_get_drvdata(dev);
struct tegra_devfreq_device *actmon_dev;
+ unsigned long cur_freq;
- stat->current_frequency = tegra->cur_freq * KHZ;
+ cur_freq = READ_ONCE(tegra->cur_freq);
/* To be used by the tegra governor */
stat->private_data = tegra;
/* The below are to be used by the other governors */
+ stat->current_frequency = cur_freq * KHZ;
actmon_dev = &tegra->devices[MCALL];
@@ -526,7 +528,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
stat->busy_time *= 100 / BUS_SATURATION_RATIO;
/* Number of cycles in a sampling period */
- stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
+ stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
stat->busy_time = min(stat->busy_time, stat->total_time);
--
2.21.0
In order to reflect that driver serves NVIDIA Tegra30 and later SoC
generations, let's rename the driver's source file to "tegra30-devfreq.c".
This will make driver files to look more consistent after addition of a
driver for Tegra20.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/Makefile | 2 +-
drivers/devfreq/{tegra-devfreq.c => tegra30-devfreq.c} | 0
2 files changed, 1 insertion(+), 1 deletion(-)
rename drivers/devfreq/{tegra-devfreq.c => tegra30-devfreq.c} (100%)
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 32b8d4d3f12c..47e5aeeebfd1 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -10,7 +10,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
# DEVFREQ Drivers
obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
-obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra-devfreq.o
+obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
# DEVFREQ Event Drivers
obj-$(CONFIG_PM_DEVFREQ_EVENT) += event/
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
similarity index 100%
rename from drivers/devfreq/tegra-devfreq.c
rename to drivers/devfreq/tegra30-devfreq.c
--
2.21.0
Move hardware configuration to governor's start/resume methods.
This allows to re-initialize hardware counters and reconfigure
cleanly if governor was stopped/paused. That is needed because we
are not aware of all hardware changes that happened while governor
was stopped and the paused state may get out of sync with reality,
hence it's better to start with a clean slate after the pause. In
a result there is no memory bandwidth starvation after resume from
suspend-to-ram that results in display controller underflowing that
happens on resume because of improper decision made by devfreq about
the required memory frequency. This change also cleans up code a tad
by moving hardware-configuration code into a single location.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 98 ++++++++++++++-------------------
1 file changed, 40 insertions(+), 58 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 40ce9881f144..2d9d53daedd8 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -392,55 +392,6 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
return NOTIFY_OK;
}
-static void tegra_actmon_enable_interrupts(struct tegra_devfreq *tegra)
-{
- struct tegra_devfreq_device *dev;
- u32 val;
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
- dev = &tegra->devices[i];
-
- val = device_readl(dev, ACTMON_DEV_CTRL);
- val |= ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
- val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
- val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
- val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
-
- device_writel(dev, val, ACTMON_DEV_CTRL);
- }
-
- actmon_write_barrier(tegra);
-}
-
-static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
-{
- struct tegra_devfreq_device *dev;
- u32 val;
- unsigned int i;
-
- disable_irq(tegra->irq);
-
- for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
- dev = &tegra->devices[i];
-
- val = device_readl(dev, ACTMON_DEV_CTRL);
- val &= ~ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
- val &= ~ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
- val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
- val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
-
- device_writel(dev, val, ACTMON_DEV_CTRL);
-
- device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
- ACTMON_DEV_INTR_STATUS);
- }
-
- actmon_write_barrier(tegra);
-
- enable_irq(tegra->irq);
-}
-
static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
struct tegra_devfreq_device *dev)
{
@@ -464,11 +415,47 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
<< 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_AVG_ABOVE_WMARK_EN;
+ val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
+ val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+ val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
val |= ACTMON_DEV_CTRL_ENB;
device_writel(dev, val, ACTMON_DEV_CTRL);
+}
+
+static void tegra_actmon_start(struct tegra_devfreq *tegra)
+{
+ unsigned int i;
+
+ disable_irq(tegra->irq);
+
+ actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
+ ACTMON_GLB_PERIOD_CTRL);
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
+ tegra_actmon_configure_device(tegra, &tegra->devices[i]);
+
+ actmon_write_barrier(tegra);
+
+ enable_irq(tegra->irq);
+}
+
+static void tegra_actmon_stop(struct tegra_devfreq *tegra)
+{
+ unsigned int i;
+
+ disable_irq(tegra->irq);
+
+ for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
+ device_writel(&tegra->devices[i], 0x00000000, ACTMON_DEV_CTRL);
+ device_writel(&tegra->devices[i], ACTMON_INTR_STATUS_CLEAR,
+ ACTMON_DEV_INTR_STATUS);
+ }
actmon_write_barrier(tegra);
+
+ enable_irq(tegra->irq);
}
static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
@@ -580,22 +567,22 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
switch (event) {
case DEVFREQ_GOV_START:
devfreq_monitor_start(devfreq);
- tegra_actmon_enable_interrupts(tegra);
+ tegra_actmon_start(tegra);
break;
case DEVFREQ_GOV_STOP:
- tegra_actmon_disable_interrupts(tegra);
+ tegra_actmon_stop(tegra);
devfreq_monitor_stop(devfreq);
break;
case DEVFREQ_GOV_SUSPEND:
- tegra_actmon_disable_interrupts(tegra);
+ tegra_actmon_stop(tegra);
devfreq_monitor_suspend(devfreq);
break;
case DEVFREQ_GOV_RESUME:
devfreq_monitor_resume(devfreq);
- tegra_actmon_enable_interrupts(tegra);
+ tegra_actmon_start(tegra);
break;
}
@@ -664,15 +651,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
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);
}
for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
--
2.21.0
The ACTMON's governor supports only the Tegra's devfreq device and there
is no need to use any other governor, hence let's mark Tegra governor as
immutable to permanently stick it with Tegra's devfreq device.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/Kconfig | 1 -
drivers/devfreq/tegra-devfreq.c | 1 +
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 6a172d338f6d..a78dffe603c1 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -94,7 +94,6 @@ config ARM_EXYNOS_BUS_DEVFREQ
config ARM_TEGRA_DEVFREQ
tristate "Tegra DEVFREQ Driver"
depends on ARCH_TEGRA_124_SOC
- select DEVFREQ_GOV_SIMPLE_ONDEMAND
select PM_OPP
help
This adds the DEVFREQ driver for the Tegra family of SoCs.
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 5265d735419f..65b3a7c72293 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -606,6 +606,7 @@ static struct devfreq_governor tegra_devfreq_governor = {
.name = "tegra_actmon",
.get_target_freq = tegra_governor_get_target,
.event_handler = tegra_governor_event_handler,
+ .immutable = true,
};
static int tegra_devfreq_probe(struct platform_device *pdev)
--
2.21.0
The write memory barrier isn't needed because the BUS buffer is flushed
by read after write that happens after the removed wmb(), we will also
use readl() instead of the relaxed version to ensure that read is indeed
completed.
Reviewed-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra-devfreq.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index f7378a42d184..7d7b9a5895b2 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -243,8 +243,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
static void actmon_write_barrier(struct tegra_devfreq *tegra)
{
/* ensure the update has reached the ACTMON */
- wmb();
- actmon_readl(tegra, ACTMON_GLB_STATUS);
+ readl(tegra->regs + ACTMON_GLB_STATUS);
}
static void actmon_isr_device(struct tegra_devfreq *tegra,
--
2.21.0
03.05.2019 3:31, Chanwoo Choi пишет:
> Hi Dmitry,
>
> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>> Changelog:
>>
>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>
>> - changed the driver removal order to match the probe exactly
>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>
>> Chanwoo, please also note that the clk patch that should fix
>> compilation problem that was reported the kbuild-test-robot is already
>> applied and available in the recent linux-next.
>
> I knew that Stephen picked up your path about clock.
Hi Chanwoo,
Okay, good. Thank you very much for reviewing this series! I assume it's
too late now for v5.2, but it should be good to go for v5.3.
Hi Dmitry,
On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
> Changelog:
>
> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>
> - changed the driver removal order to match the probe exactly
> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>
> Chanwoo, please also note that the clk patch that should fix
> compilation problem that was reported the kbuild-test-robot is already
> applied and available in the recent linux-next.
I knew that Stephen picked up your path about clock.
>
> v3: Addressed all review comments that were made by Chanwoo Choi to v2.
>
> Patch "Synchronize IRQ after masking it in hardware" morphed into
> "Properly disable interrupts", which disables interrupts more solidly.
>
> Added new minor patch: "Rename tegra-devfreq.c to tegra30-devfreq.c".
>
> Added missed error handlings for dev_pm_opp_add().
>
> v2: The patchset was quite heavily reworked since v1, few patches we
> dropped or squashed into the new ones and more patches we added.
> In a result more bugs and potential problems are fixed now, driver's
> code got more clean up.
>
> The Tegra20 driver-addition patch is now a part of this series, it has
> no changes since v1.
>
> Dmitry Osipenko (16):
> PM / devfreq: tegra: Fix kHz to Hz conversion
> PM / devfreq: tegra: Replace readl-writel with relaxed versions
> PM / devfreq: tegra: Replace write memory barrier with the read
> barrier
> PM / devfreq: tegra: Don't ignore clk errors
> PM / devfreq: tegra: Don't set EMC clock rate to maximum on probe
> PM / devfreq: tegra: Drop primary interrupt handler
> PM / devfreq: tegra: Properly disable interrupts
> PM / devfreq: tegra: Clean up driver's probe / remove
> PM / devfreq: tegra: Avoid inconsistency of current frequency value
> PM / devfreq: tegra: Mark ACTMON's governor as immutable
> PM / devfreq: tegra: Move governor registration to driver's probe
> PM / devfreq: tegra: Reconfigure hardware on governor's restart
> PM / devfreq: tegra: Support Tegra30
> PM / devfreq: tegra: Enable COMPILE_TEST for the driver
> PM / devfreq: tegra: Rename tegra-devfreq.c to tegra30-devfreq.c
> PM / devfreq: Introduce driver for NVIDIA Tegra20
>
> MAINTAINERS | 8 +
> drivers/devfreq/Kconfig | 15 +-
> drivers/devfreq/Makefile | 3 +-
> drivers/devfreq/tegra20-devfreq.c | 212 ++++++++++++
> .../{tegra-devfreq.c => tegra30-devfreq.c} | 315 ++++++++----------
> 5 files changed, 379 insertions(+), 174 deletions(-)
> create mode 100644 drivers/devfreq/tegra20-devfreq.c
> rename drivers/devfreq/{tegra-devfreq.c => tegra30-devfreq.c} (81%)
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
03.05.2019 3:52, Dmitry Osipenko пишет:
> 03.05.2019 3:31, Chanwoo Choi пишет:
>> Hi Dmitry,
>>
>> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>>> Changelog:
>>>
>>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>>
>>> - changed the driver removal order to match the probe exactly
>>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>>
>>> Chanwoo, please also note that the clk patch that should fix
>>> compilation problem that was reported the kbuild-test-robot is already
>>> applied and available in the recent linux-next.
>>
>> I knew that Stephen picked up your path about clock.
>
> Hi Chanwoo,
>
> Okay, good. Thank you very much for reviewing this series! I assume it's
> too late now for v5.2, but it should be good to go for v5.3.
>
Hello Chanwoo,
Will be nice to see the patches in the linux-next before they'll hit mainline. We have tested that
everything works fine on a selective devices, but won't hurt to get some extra testing beforehand.
AFAIK, at least NVIDIA people are regularly testing -next on theirs dev boards. Please note that
this not very important, so don't bother if there is some hurdle with pushing to the tracking branch
for now. Also please let me know if you're expecting to see some ACK's on the patches, I'm sure
we'll be able to work out that with Thierry and Jon if necessary.
On 19. 6. 4. 오전 1:52, Dmitry Osipenko wrote:
> 03.05.2019 3:52, Dmitry Osipenko пишет:
>> 03.05.2019 3:31, Chanwoo Choi пишет:
>>> Hi Dmitry,
>>>
>>> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>>>> Changelog:
>>>>
>>>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>>>
>>>> - changed the driver removal order to match the probe exactly
>>>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>>>
>>>> Chanwoo, please also note that the clk patch that should fix
>>>> compilation problem that was reported the kbuild-test-robot is already
>>>> applied and available in the recent linux-next.
>>>
>>> I knew that Stephen picked up your path about clock.
>>
>> Hi Chanwoo,
>>
>> Okay, good. Thank you very much for reviewing this series! I assume it's
>> too late now for v5.2, but it should be good to go for v5.3.
>>
>
> Hello Chanwoo,
>
> Will be nice to see the patches in the linux-next before they'll hit mainline. We have tested that
> everything works fine on a selective devices, but won't hurt to get some extra testing beforehand.
> AFAIK, at least NVIDIA people are regularly testing -next on theirs dev boards. Please note that
> this not very important, so don't bother if there is some hurdle with pushing to the tracking branch
> for now. Also please let me know if you're expecting to see some ACK's on the patches, I'm sure
> we'll be able to work out that with Thierry and Jon if necessary.
>
>
Hi Dmitry,
I think that it is enough for applying to mainline branch.
The devfreq.git is maintained by Myungjoo. He will be merged or
reviewed if there are th remained review point.
Hi Myungjoo,
I reviewed the Dmitry's patches from v1 to v4 patches.
And then I tested them on my testing branch[1] for catching
the build warning and error. In result, it is clean.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
Please review or apply these patches for v5.3.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On Thu, May 02, 2019 at 02:38:00AM +0300, Dmitry Osipenko wrote:
> The kHz to Hz is incorrectly converted in a few places in the code,
> this results in a wrong frequency being calculated because devfreq core
> uses OPP frequencies that are given in Hz to clamp the rate, while
> tegra-devfreq gives to the core value in kHz and then it also expects to
> receive value in kHz from the core. In a result memory freq is always set
> to a value which is close to ULONG_MAX because of the bug. Hence the EMC
> frequency is always capped to the maximum and the driver doesn't do
> anything useful. This patch was tested on Tegra30 and Tegra124 SoC's, EMC
> frequency scaling works properly now.
>
> Cc: <[email protected]> # 4.14+
> Tested-by: Steev Klimaszewski <[email protected]>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:01AM +0300, Dmitry Osipenko wrote:
> There is no need to insert memory barrier on each readl/writel
> invocation, hence use the relaxed versions.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:02AM +0300, Dmitry Osipenko wrote:
> The write memory barrier isn't needed because the BUS buffer is flushed
> by read after write that happens after the removed wmb(), we will also
> use readl() instead of the relaxed version to ensure that read is indeed
> completed.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:03AM +0300, Dmitry Osipenko wrote:
> The clk_set_min_rate() could fail and in this case clk_set_rate() sets
> rate to 0, which may drop EMC rate to minimum and make machine very
> difficult to use.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:04AM +0300, Dmitry Osipenko wrote:
> There is no real benefit from doing so, hence let's drop that rate setting
> for consistency.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 2 --
> 1 file changed, 2 deletions(-)
Do you have any numbers to tell how long it would take for the EMC rate
to get incremented? My understanding is that ACTMON basically reacts to
system load, so I could imagine that not setting to the maximum
frequency after this is loaded might make the system slow in the short
term. Only after ACTMON has collected enough data to determine that it
needs to clock the EMC higher would system speed resume normal.
I guess technically this patch doesn't change anything if the system
already boots at the highest EMC frequency anyway, which I think it does
on many (although not all) devices.
Anyway, you said you've tested this and are satisfied with the
performance, so it can't be that bad:
Acked-by: Thierry Reding <[email protected]>
>
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index c7428c5eee23..24ec65556c39 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -653,8 +653,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> return PTR_ERR(tegra->emc_clock);
> }
>
> - clk_set_rate(tegra->emc_clock, ULONG_MAX);
> -
> tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
> err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
> if (err) {
> --
> 2.21.0
>
On Thu, May 02, 2019 at 02:38:05AM +0300, Dmitry Osipenko wrote:
> There is no real need in the primary interrupt handler, hence move
> everything to the secondary (threaded) handler. In a result locking
> is consistent now and there are no potential races with the interrupt
> handler because it is protected with the devfreq's mutex.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 55 +++++++++++----------------------
> 1 file changed, 18 insertions(+), 37 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:06AM +0300, Dmitry Osipenko wrote:
> There is no guarantee that interrupt handling isn't running in parallel
> with tegra_actmon_disable_interrupts(), hence it is necessary to protect
> DEV_CTRL register accesses and clear IRQ status with ACTMON's IRQ being
> disabled in the Interrupt Controller in order to ensure that device
> interrupt is indeed being disabled.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index b65313fe3c2e..ce1eb97a2090 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -171,6 +171,8 @@ struct tegra_devfreq {
> struct notifier_block rate_change_nb;
>
> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
> +
> + int irq;
Interrupts are typically unsigned int.
> };
>
> struct tegra_actmon_emc_ratio {
> @@ -417,6 +419,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
> u32 val;
> unsigned int i;
>
> + disable_irq(tegra->irq);
> +
> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> dev = &tegra->devices[i];
>
> @@ -427,9 +431,14 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
> val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>
> device_writel(dev, val, ACTMON_DEV_CTRL);
> +
> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
> + ACTMON_DEV_INTR_STATUS);
> }
>
> actmon_write_barrier(tegra);
> +
> + enable_irq(tegra->irq);
Why do we enable interrupts after this? Is there any use in having the
top-level interrupt enabled if nothing's going to generate an interrupt
anyway?
> }
>
> static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> @@ -604,7 +613,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> struct resource *res;
> unsigned int i;
> unsigned long rate;
> - int irq;
> int err;
>
> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> @@ -673,15 +681,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> dev_pm_opp_add(&pdev->dev, rate, 0);
> }
>
> - irq = platform_get_irq(pdev, 0);
> - if (irq < 0) {
> - dev_err(&pdev->dev, "Failed to get IRQ: %d\n", irq);
> - return irq;
> + tegra->irq = platform_get_irq(pdev, 0);
> + if (tegra->irq < 0) {
> + err = tegra->irq;
> + dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
> + return err;
> }
This is very oddly written. tegra->irq should really be an unsigned int
since that's the standard type for interrupt numbers. But since you need
to be able to detect errors from platform_get_irq() it now becomes
natural to write this as:
err = platform_get_irq(pdev, 0);
if (err < 0) {
dev_err(...);
return err;
}
tegra->irq = err;
Two birds with one stone. I suppose this could be done in a follow-up
patch since it isn't practically wrong in your version, so either way:
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:07AM +0300, Dmitry Osipenko wrote:
> Reset hardware, disable ACTMON clock, release OPP's and handle all
> possible error cases correctly, maintaining the correct tear down
> order. Also use devm_platform_ioremap_resource() which is now available
> in the kernel.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 83 +++++++++++++++++++--------------
> 1 file changed, 47 insertions(+), 36 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:08AM +0300, Dmitry Osipenko wrote:
> The frequency value potentially could change in-between. It doesn't
> cause any real problem at all right now, but that could change in the
> future. Hence let's avoid the inconsistency.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:09AM +0300, Dmitry Osipenko wrote:
> The ACTMON's governor supports only the Tegra's devfreq device and there
> is no need to use any other governor, hence let's mark Tegra governor as
> immutable to permanently stick it with Tegra's devfreq device.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/Kconfig | 1 -
> drivers/devfreq/tegra-devfreq.c | 1 +
> 2 files changed, 1 insertion(+), 1 deletion(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:10AM +0300, Dmitry Osipenko wrote:
> There is no need to register the ACTMON's governor separately from
> the driver, hence let's move the registration into the driver's probe
> function for consistency and to make code cleaner a tad.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 43 +++++++++------------------------
> 1 file changed, 12 insertions(+), 31 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:12AM +0300, Dmitry Osipenko wrote:
> The devfreq driver can be used on Tegra30 without any code change and
> it works perfectly fine, the default Tegra124 parameters are good enough
> for Tegra30.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/Kconfig | 4 ++--
> drivers/devfreq/tegra-devfreq.c | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:11AM +0300, Dmitry Osipenko wrote:
> Move hardware configuration to governor's start/resume methods.
> This allows to re-initialize hardware counters and reconfigure
> cleanly if governor was stopped/paused. That is needed because we
> are not aware of all hardware changes that happened while governor
> was stopped and the paused state may get out of sync with reality,
> hence it's better to start with a clean slate after the pause. In
> a result there is no memory bandwidth starvation after resume from
> suspend-to-ram that results in display controller underflowing that
> happens on resume because of improper decision made by devfreq about
> the required memory frequency. This change also cleans up code a tad
> by moving hardware-configuration code into a single location.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra-devfreq.c | 98 ++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 58 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
> The driver's compilation doesn't have any specific dependencies, hence
> the COMPILE_TEST option can be supported in Kconfig.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 56db9dc05edb..a6bba6e1e7d9 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>
> config ARM_TEGRA_DEVFREQ
> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> - depends on ARCH_TEGRA
> + depends on ARCH_TEGRA || COMPILE_TEST
> select PM_OPP
> help
> This adds the DEVFREQ driver for the Tegra family of SoCs.
You need to be careful with these. You're using I/O register accessors,
which are not supported on the UM architecture, for example.
This may end up getting flagged during build testing.
Thierry
On Thu, May 02, 2019 at 02:38:14AM +0300, Dmitry Osipenko wrote:
> In order to reflect that driver serves NVIDIA Tegra30 and later SoC
> generations, let's rename the driver's source file to "tegra30-devfreq.c".
> This will make driver files to look more consistent after addition of a
> driver for Tegra20.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/Makefile | 2 +-
> drivers/devfreq/{tegra-devfreq.c => tegra30-devfreq.c} | 0
> 2 files changed, 1 insertion(+), 1 deletion(-)
> rename drivers/devfreq/{tegra-devfreq.c => tegra30-devfreq.c} (100%)
>
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d3f12c..47e5aeeebfd1 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -10,7 +10,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
> # DEVFREQ Drivers
> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
> -obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra-devfreq.o
> +obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
Technically this changes the name of the driver. Sometimes boot or other
scripts rely on those names. Perhaps a better way of keeping backwards-
compatibility would be to do:
obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra-devfreq.o
tegra-devfreq-y += tegra30-devfreq.o
That way you can later on just add the tegra20-devfreq.o to that driver
as well and have them both ship in one .ko.
Thierry
>
> # DEVFREQ Event Drivers
> obj-$(CONFIG_PM_DEVFREQ_EVENT) += event/
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> similarity index 100%
> rename from drivers/devfreq/tegra-devfreq.c
> rename to drivers/devfreq/tegra30-devfreq.c
> --
> 2.21.0
>
On Thu, May 02, 2019 at 02:38:15AM +0300, Dmitry Osipenko wrote:
> Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
> reads out Memory Controller counters and adjusts memory frequency based
> on the memory clients activity.
>
> Reviewed-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> MAINTAINERS | 8 ++
> drivers/devfreq/Kconfig | 10 ++
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/tegra20-devfreq.c | 212 ++++++++++++++++++++++++++++++
> 4 files changed, 231 insertions(+)
> create mode 100644 drivers/devfreq/tegra20-devfreq.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 98edc38bfd7b..e7e434f74038 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10098,6 +10098,14 @@ F: include/linux/memblock.h
> F: mm/memblock.c
> F: Documentation/core-api/boot-time-mm.rst
>
> +MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
> +M: Dmitry Osipenko <[email protected]>
> +L: [email protected]
> +L: [email protected]
> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
> +S: Maintained
> +F: drivers/devfreq/tegra20-devfreq.c
> +
> MEMORY MANAGEMENT
> L: [email protected]
> W: http://www.linux-mm.org
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index a6bba6e1e7d9..1530dbefa31f 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ
> It reads ACTMON counters of memory controllers and adjusts the
> operating frequencies and voltages with OPP support.
>
> +config ARM_TEGRA20_DEVFREQ
> + tristate "NVIDIA Tegra20 DEVFREQ Driver"
> + depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
> + select DEVFREQ_GOV_SIMPLE_ONDEMAND
> + select PM_OPP
Again, I'm not sure the COMPILE_TEST will work here unless you add a few
more dependencies.
Thierry
> + help
> + This adds the DEVFREQ driver for the Tegra20 family of SoCs.
> + It reads Memory Controller counters and adjusts the operating
> + frequencies and voltages with OPP support.
> +
> config ARM_RK3399_DMC_DEVFREQ
> tristate "ARM RK3399 DMC DEVFREQ Driver"
> depends on ARCH_ROCKCHIP
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 47e5aeeebfd1..338ae8440db6 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
> +obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o
>
> # DEVFREQ Event Drivers
> obj-$(CONFIG_PM_DEVFREQ_EVENT) += event/
> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
> new file mode 100644
> index 000000000000..ff82bac9ee4e
> --- /dev/null
> +++ b/drivers/devfreq/tegra20-devfreq.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * NVIDIA Tegra20 devfreq driver
> + *
> + * Copyright (C) 2019 GRATE-DRIVER project
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +
> +#include <soc/tegra/mc.h>
> +
> +#include "governor.h"
> +
> +#define MC_STAT_CONTROL 0x90
> +#define MC_STAT_EMC_CLOCK_LIMIT 0xa0
> +#define MC_STAT_EMC_CLOCKS 0xa4
> +#define MC_STAT_EMC_CONTROL 0xa8
> +#define MC_STAT_EMC_COUNT 0xb8
> +
> +#define EMC_GATHER_CLEAR (1 << 8)
> +#define EMC_GATHER_ENABLE (3 << 8)
> +
> +struct tegra_devfreq {
> + struct devfreq *devfreq;
> + struct clk *emc_clock;
> + void __iomem *regs;
> +};
> +
> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> + u32 flags)
> +{
> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> + struct devfreq *devfreq = tegra->devfreq;
> + struct dev_pm_opp *opp;
> + unsigned long rate;
> + int err;
> +
> + opp = devfreq_recommended_opp(dev, freq, flags);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + rate = dev_pm_opp_get_freq(opp);
> + dev_pm_opp_put(opp);
> +
> + err = clk_set_min_rate(tegra->emc_clock, rate);
> + if (err)
> + return err;
> +
> + err = clk_set_rate(tegra->emc_clock, 0);
> + if (err)
> + goto restore_min_rate;
> +
> + return 0;
> +
> +restore_min_rate:
> + clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq);
> +
> + return err;
> +}
> +
> +static int tegra_devfreq_get_dev_status(struct device *dev,
> + struct devfreq_dev_status *stat)
> +{
> + struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> +
> + /*
> + * EMC_COUNT returns number of memory events, that number is lower
> + * than the number of clocks. Conversion ratio of 1/8 results in a
> + * bit higher bandwidth than actually needed, it is good enough for
> + * the time being because drivers don't support requesting minimum
> + * needed memory bandwidth yet.
> + *
> + * TODO: adjust the ratio value once relevant drivers will support
> + * memory bandwidth management.
> + */
> + stat->busy_time = readl_relaxed(tegra->regs + MC_STAT_EMC_COUNT);
> + stat->total_time = readl_relaxed(tegra->regs + MC_STAT_EMC_CLOCKS) / 8;
> + stat->current_frequency = clk_get_rate(tegra->emc_clock);
> +
> + writel_relaxed(EMC_GATHER_CLEAR, tegra->regs + MC_STAT_CONTROL);
> + writel_relaxed(EMC_GATHER_ENABLE, tegra->regs + MC_STAT_CONTROL);
> +
> + return 0;
> +}
> +
> +static struct devfreq_dev_profile tegra_devfreq_profile = {
> + .polling_ms = 500,
> + .target = tegra_devfreq_target,
> + .get_dev_status = tegra_devfreq_get_dev_status,
> +};
> +
> +static struct tegra_mc *tegra_get_memory_controller(void)
> +{
> + struct platform_device *pdev;
> + struct device_node *np;
> + struct tegra_mc *mc;
> +
> + np = of_find_compatible_node(NULL, NULL, "nvidia,tegra20-mc-gart");
> + if (!np)
> + return ERR_PTR(-ENOENT);
> +
> + pdev = of_find_device_by_node(np);
> + of_node_put(np);
> + if (!pdev)
> + return ERR_PTR(-ENODEV);
> +
> + mc = platform_get_drvdata(pdev);
> + if (!mc)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + return mc;
> +}
> +
> +static int tegra_devfreq_probe(struct platform_device *pdev)
> +{
> + struct tegra_devfreq *tegra;
> + struct tegra_mc *mc;
> + unsigned long max_rate;
> + unsigned long rate;
> + int err;
> +
> + mc = tegra_get_memory_controller();
> + if (IS_ERR(mc)) {
> + err = PTR_ERR(mc);
> + dev_err(&pdev->dev, "failed to get memory controller: %d\n",
> + err);
> + return err;
> + }
> +
> + tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> + if (!tegra)
> + return -ENOMEM;
> +
> + /* EMC is a system-critical clock that is always enabled */
> + tegra->emc_clock = devm_clk_get(&pdev->dev, "emc");
> + if (IS_ERR(tegra->emc_clock)) {
> + err = PTR_ERR(tegra->emc_clock);
> + dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
> + return err;
> + }
> +
> + tegra->regs = mc->regs;
> +
> + max_rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
> +
> + for (rate = 0; rate <= max_rate; rate++) {
> + rate = clk_round_rate(tegra->emc_clock, rate);
> +
> + err = dev_pm_opp_add(&pdev->dev, rate, 0);
> + if (err) {
> + dev_err(&pdev->dev, "failed to add opp: %d\n", err);
> + goto remove_opps;
> + }
> + }
> +
> + /*
> + * Reset statistic gathers state, select global bandwidth for the
> + * statistics collection mode and set clocks counter saturation
> + * limit to maximum.
> + */
> + writel_relaxed(0x00000000, tegra->regs + MC_STAT_CONTROL);
> + writel_relaxed(0x00000000, tegra->regs + MC_STAT_EMC_CONTROL);
> + writel_relaxed(0xffffffff, tegra->regs + MC_STAT_EMC_CLOCK_LIMIT);
> +
> + platform_set_drvdata(pdev, tegra);
> +
> + tegra->devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
> + DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
> + if (IS_ERR(tegra->devfreq)) {
> + err = PTR_ERR(tegra->devfreq);
> + goto remove_opps;
> + }
> +
> + return 0;
> +
> +remove_opps:
> + dev_pm_opp_remove_all_dynamic(&pdev->dev);
> +
> + return err;
> +}
> +
> +static int tegra_devfreq_remove(struct platform_device *pdev)
> +{
> + struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
> +
> + devfreq_remove_device(tegra->devfreq);
> + dev_pm_opp_remove_all_dynamic(&pdev->dev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver tegra_devfreq_driver = {
> + .probe = tegra_devfreq_probe,
> + .remove = tegra_devfreq_remove,
> + .driver = {
> + .name = "tegra20-devfreq",
> + },
> +};
> +module_platform_driver(tegra_devfreq_driver);
> +
> +MODULE_ALIAS("platform:tegra20-devfreq");
> +MODULE_AUTHOR("Dmitry Osipenko <[email protected]>");
> +MODULE_DESCRIPTION("NVIDIA Tegra20 devfreq driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.21.0
>
04.06.2019 14:00, Thierry Reding пишет:
> On Thu, May 02, 2019 at 02:38:04AM +0300, Dmitry Osipenko wrote:
>> There is no real benefit from doing so, hence let's drop that rate setting
>> for consistency.
>>
>> Reviewed-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/devfreq/tegra-devfreq.c | 2 --
>> 1 file changed, 2 deletions(-)
>
> Do you have any numbers to tell how long it would take for the EMC rate
> to get incremented? My understanding is that ACTMON basically reacts to
> system load, so I could imagine that not setting to the maximum
> frequency after this is loaded might make the system slow in the short
> term. Only after ACTMON has collected enough data to determine that it
> needs to clock the EMC higher would system speed resume normal.
>
> I guess technically this patch doesn't change anything if the system
> already boots at the highest EMC frequency anyway, which I think it does
> on many (although not all) devices.
>
> Anyway, you said you've tested this and are satisfied with the
> performance, so it can't be that bad:
>
> Acked-by: Thierry Reding <[email protected]>
It takes 12ms for ACTMON to react and then about (couple) hundred
microseconds to change memory freq. This is a very short period of time
that human being can't notice.
AFAIK, in practice there are no devices in the wild that boot up with
DRAM clocked at lowest rate. Most devices have a video output and thus
require significant memory bandwidth at a boot time already. Secondly,
higher memory bandwidth is only really needed for a cases like
multimedia, in most generic cases CPU is hitting cache and not utilizing
DRAM a lot.
04.06.2019 14:07, Thierry Reding пишет:
> On Thu, May 02, 2019 at 02:38:06AM +0300, Dmitry Osipenko wrote:
>> There is no guarantee that interrupt handling isn't running in parallel
>> with tegra_actmon_disable_interrupts(), hence it is necessary to protect
>> DEV_CTRL register accesses and clear IRQ status with ACTMON's IRQ being
>> disabled in the Interrupt Controller in order to ensure that device
>> interrupt is indeed being disabled.
>>
>> Reviewed-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/devfreq/tegra-devfreq.c | 21 +++++++++++++++------
>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index b65313fe3c2e..ce1eb97a2090 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -171,6 +171,8 @@ struct tegra_devfreq {
>> struct notifier_block rate_change_nb;
>>
>> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>> +
>> + int irq;
>
> Interrupts are typically unsigned int.
>
>> };
>>
>> struct tegra_actmon_emc_ratio {
>> @@ -417,6 +419,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>> u32 val;
>> unsigned int i;
>>
>> + disable_irq(tegra->irq);
>> +
>> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>> dev = &tegra->devices[i];
>>
>> @@ -427,9 +431,14 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>> val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>>
>> device_writel(dev, val, ACTMON_DEV_CTRL);
>> +
>> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
>> + ACTMON_DEV_INTR_STATUS);
>> }
>>
>> actmon_write_barrier(tegra);
>> +
>> + enable_irq(tegra->irq);
>
> Why do we enable interrupts after this? Is there any use in having the
> top-level interrupt enabled if nothing's going to generate an interrupt
> anyway?
There is no real point in having the interrupt enabled other than to
keep the enable count balanced.
IIUC, we will need to disable IRQ at the driver's probe time (after
requesting the IRQ) if we want to avoid that (not really necessary)
balancing. This is probably something that could be improved in a
follow-up patches, if desired.
>> }
>>
>> static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>> @@ -604,7 +613,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>> struct resource *res;
>> unsigned int i;
>> unsigned long rate;
>> - int irq;
>> int err;
>>
>> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>> @@ -673,15 +681,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>> dev_pm_opp_add(&pdev->dev, rate, 0);
>> }
>>
>> - irq = platform_get_irq(pdev, 0);
>> - if (irq < 0) {
>> - dev_err(&pdev->dev, "Failed to get IRQ: %d\n", irq);
>> - return irq;
>> + tegra->irq = platform_get_irq(pdev, 0);
>> + if (tegra->irq < 0) {
>> + err = tegra->irq;
>> + dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
>> + return err;
>> }
>
> This is very oddly written. tegra->irq should really be an unsigned int
> since that's the standard type for interrupt numbers. But since you need
> to be able to detect errors from platform_get_irq() it now becomes
> natural to write this as:
>
> err = platform_get_irq(pdev, 0);
> if (err < 0) {
> dev_err(...);
> return err;
> }
>
> tegra->irq = err;
>
> Two birds with one stone. I suppose this could be done in a follow-up
> patch since it isn't practically wrong in your version, so either way:
>
> Acked-by: Thierry Reding <[email protected]>
>
Thank you for the ACK! Although, I disagree with yours suggestion, to me
that makes code a bit less straightforward and it's not really
worthwhile to bloat the code just because technically IRQ's are unsigned
numbers (we don't care about that). It also makes me a bit uncomfortable
to see "err" assigned to a variable, I don't think it's a good practice.
04.06.2019 14:20, Thierry Reding пишет:
> On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
>> The driver's compilation doesn't have any specific dependencies, hence
>> the COMPILE_TEST option can be supported in Kconfig.
>>
>> Reviewed-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/devfreq/Kconfig | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 56db9dc05edb..a6bba6e1e7d9 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>
>> config ARM_TEGRA_DEVFREQ
>> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>> - depends on ARCH_TEGRA
>> + depends on ARCH_TEGRA || COMPILE_TEST
>> select PM_OPP
>> help
>> This adds the DEVFREQ driver for the Tegra family of SoCs.
>
> You need to be careful with these. You're using I/O register accessors,
> which are not supported on the UM architecture, for example.
>
> This may end up getting flagged during build testing.
We have similar cases in other drivers and it doesn't cause any known
problems because (I think) build-bots are aware of this detail. Hence
there is no real need to be overreactive here and in this particular
case it's better to react to real problems once they show up (we already
did that by fixing build breakage caused by a CLK API problem found by
bot in v3). Does it sound like a good argument to you? ACK?
04.06.2019 14:25, Thierry Reding пишет:
> On Thu, May 02, 2019 at 02:38:15AM +0300, Dmitry Osipenko wrote:
>> Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
>> reads out Memory Controller counters and adjusts memory frequency based
>> on the memory clients activity.
>>
>> Reviewed-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> MAINTAINERS | 8 ++
>> drivers/devfreq/Kconfig | 10 ++
>> drivers/devfreq/Makefile | 1 +
>> drivers/devfreq/tegra20-devfreq.c | 212 ++++++++++++++++++++++++++++++
>> 4 files changed, 231 insertions(+)
>> create mode 100644 drivers/devfreq/tegra20-devfreq.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 98edc38bfd7b..e7e434f74038 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10098,6 +10098,14 @@ F: include/linux/memblock.h
>> F: mm/memblock.c
>> F: Documentation/core-api/boot-time-mm.rst
>>
>> +MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
>> +M: Dmitry Osipenko <[email protected]>
>> +L: [email protected]
>> +L: [email protected]
>> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
>> +S: Maintained
>> +F: drivers/devfreq/tegra20-devfreq.c
>> +
>> MEMORY MANAGEMENT
>> L: [email protected]
>> W: http://www.linux-mm.org
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index a6bba6e1e7d9..1530dbefa31f 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ
>> It reads ACTMON counters of memory controllers and adjusts the
>> operating frequencies and voltages with OPP support.
>>
>> +config ARM_TEGRA20_DEVFREQ
>> + tristate "NVIDIA Tegra20 DEVFREQ Driver"
>> + depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
>> + select DEVFREQ_GOV_SIMPLE_ONDEMAND
>> + select PM_OPP
>
> Again, I'm not sure the COMPILE_TEST will work here unless you add a few
> more dependencies.
I have the same answer as I made in the comment to "Enable COMPILE_TEST
for the driver" patch. I think there is no real need to be overreactive
here as well, ACK?
On Tue, Jun 04, 2019 at 04:40:18PM +0300, Dmitry Osipenko wrote:
> 04.06.2019 14:07, Thierry Reding пишет:
> > On Thu, May 02, 2019 at 02:38:06AM +0300, Dmitry Osipenko wrote:
> >> There is no guarantee that interrupt handling isn't running in parallel
> >> with tegra_actmon_disable_interrupts(), hence it is necessary to protect
> >> DEV_CTRL register accesses and clear IRQ status with ACTMON's IRQ being
> >> disabled in the Interrupt Controller in order to ensure that device
> >> interrupt is indeed being disabled.
> >>
> >> Reviewed-by: Chanwoo Choi <[email protected]>
> >> Signed-off-by: Dmitry Osipenko <[email protected]>
> >> ---
> >> drivers/devfreq/tegra-devfreq.c | 21 +++++++++++++++------
> >> 1 file changed, 15 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> >> index b65313fe3c2e..ce1eb97a2090 100644
> >> --- a/drivers/devfreq/tegra-devfreq.c
> >> +++ b/drivers/devfreq/tegra-devfreq.c
> >> @@ -171,6 +171,8 @@ struct tegra_devfreq {
> >> struct notifier_block rate_change_nb;
> >>
> >> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
> >> +
> >> + int irq;
> >
> > Interrupts are typically unsigned int.
> >
> >> };
> >>
> >> struct tegra_actmon_emc_ratio {
> >> @@ -417,6 +419,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
> >> u32 val;
> >> unsigned int i;
> >>
> >> + disable_irq(tegra->irq);
> >> +
> >> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> >> dev = &tegra->devices[i];
> >>
> >> @@ -427,9 +431,14 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
> >> val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> >>
> >> device_writel(dev, val, ACTMON_DEV_CTRL);
> >> +
> >> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
> >> + ACTMON_DEV_INTR_STATUS);
> >> }
> >>
> >> actmon_write_barrier(tegra);
> >> +
> >> + enable_irq(tegra->irq);
> >
> > Why do we enable interrupts after this? Is there any use in having the
> > top-level interrupt enabled if nothing's going to generate an interrupt
> > anyway?
>
> There is no real point in having the interrupt enabled other than to
> keep the enable count balanced.
>
> IIUC, we will need to disable IRQ at the driver's probe time (after
> requesting the IRQ) if we want to avoid that (not really necessary)
> balancing. This is probably something that could be improved in a
> follow-up patches, if desired.
>
> >> }
> >>
> >> static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> >> @@ -604,7 +613,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> >> struct resource *res;
> >> unsigned int i;
> >> unsigned long rate;
> >> - int irq;
> >> int err;
> >>
> >> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> >> @@ -673,15 +681,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> >> dev_pm_opp_add(&pdev->dev, rate, 0);
> >> }
> >>
> >> - irq = platform_get_irq(pdev, 0);
> >> - if (irq < 0) {
> >> - dev_err(&pdev->dev, "Failed to get IRQ: %d\n", irq);
> >> - return irq;
> >> + tegra->irq = platform_get_irq(pdev, 0);
> >> + if (tegra->irq < 0) {
> >> + err = tegra->irq;
> >> + dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
> >> + return err;
> >> }
> >
> > This is very oddly written. tegra->irq should really be an unsigned int
> > since that's the standard type for interrupt numbers. But since you need
> > to be able to detect errors from platform_get_irq() it now becomes
> > natural to write this as:
> >
> > err = platform_get_irq(pdev, 0);
> > if (err < 0) {
> > dev_err(...);
> > return err;
> > }
> >
> > tegra->irq = err;
> >
> > Two birds with one stone. I suppose this could be done in a follow-up
> > patch since it isn't practically wrong in your version, so either way:
> >
> > Acked-by: Thierry Reding <[email protected]>
> >
>
> Thank you for the ACK! Although, I disagree with yours suggestion, to me
> that makes code a bit less straightforward and it's not really
> worthwhile to bloat the code just because technically IRQ's are unsigned
> numbers (we don't care about that). It also makes me a bit uncomfortable
> to see "err" assigned to a variable, I don't think it's a good practice.
Actually you should care that IRQs are unsigned. Implicit casting from
a potentially negative value can hide bugs. That is, once you've passed
that negative value into the IRQ API you loose the context that it could
be an error code. Hence I think it makes sense to always store values in
the native type, and only store them if they are actually valid.
In the above you have an error value in tegra->irq. In this particular
case it's pretty harmless because you don't do anything with it, but if
the circumstances were slightly different that could lead to problems
down the road.
On the other hand what I was proposing makes it pretty clear from the
context that err contains a valid interrupt number when it is assigned
to tegra->irq. There's plenty of similar constructs in the kernel if you
want to grep for it.
Also, it's not bloating the code at all. It's the exact same number of
lines of code as your variant.
Thierry
On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
> 04.06.2019 14:20, Thierry Reding пишет:
> > On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
> >> The driver's compilation doesn't have any specific dependencies, hence
> >> the COMPILE_TEST option can be supported in Kconfig.
> >>
> >> Reviewed-by: Chanwoo Choi <[email protected]>
> >> Signed-off-by: Dmitry Osipenko <[email protected]>
> >> ---
> >> drivers/devfreq/Kconfig | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> >> index 56db9dc05edb..a6bba6e1e7d9 100644
> >> --- a/drivers/devfreq/Kconfig
> >> +++ b/drivers/devfreq/Kconfig
> >> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
> >>
> >> config ARM_TEGRA_DEVFREQ
> >> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> >> - depends on ARCH_TEGRA
> >> + depends on ARCH_TEGRA || COMPILE_TEST
> >> select PM_OPP
> >> help
> >> This adds the DEVFREQ driver for the Tegra family of SoCs.
> >
> > You need to be careful with these. You're using I/O register accessors,
> > which are not supported on the UM architecture, for example.
> >
> > This may end up getting flagged during build testing.
>
> We have similar cases in other drivers and it doesn't cause any known
> problems because (I think) build-bots are aware of this detail. Hence
I don't understand how the build-bots would be aware of this detail.
Unless you explicitly state what the dependencies are, how would the
build-bots know? Perhaps there's some logic built-in somewhere that I
don't know about?
> there is no real need to be overreactive here and in this particular
> case it's better to react to real problems once they show up (we already
> did that by fixing build breakage caused by a CLK API problem found by
> bot in v3). Does it sound like a good argument to you? ACK?
No. This strikes me as a strange argument. I'm pointing out a potential
source of problems and you just brush it aside claiming that it's not
true, or even if it was true that we'll see eventually and we can fix it
up when there's an actual problem.
Why would you want to have to fix things up if you can avoid breakage in
the first place by being a little proactive?
Thierry
On Tue, Jun 04, 2019 at 04:10:31PM +0200, Thierry Reding wrote:
> On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
> > 04.06.2019 14:20, Thierry Reding пишет:
> > > On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
> > >> The driver's compilation doesn't have any specific dependencies, hence
> > >> the COMPILE_TEST option can be supported in Kconfig.
> > >>
> > >> Reviewed-by: Chanwoo Choi <[email protected]>
> > >> Signed-off-by: Dmitry Osipenko <[email protected]>
> > >> ---
> > >> drivers/devfreq/Kconfig | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > >> index 56db9dc05edb..a6bba6e1e7d9 100644
> > >> --- a/drivers/devfreq/Kconfig
> > >> +++ b/drivers/devfreq/Kconfig
> > >> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
> > >>
> > >> config ARM_TEGRA_DEVFREQ
> > >> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> > >> - depends on ARCH_TEGRA
> > >> + depends on ARCH_TEGRA || COMPILE_TEST
> > >> select PM_OPP
> > >> help
> > >> This adds the DEVFREQ driver for the Tegra family of SoCs.
> > >
> > > You need to be careful with these. You're using I/O register accessors,
> > > which are not supported on the UM architecture, for example.
> > >
> > > This may end up getting flagged during build testing.
> >
> > We have similar cases in other drivers and it doesn't cause any known
> > problems because (I think) build-bots are aware of this detail. Hence
>
> I don't understand how the build-bots would be aware of this detail.
> Unless you explicitly state what the dependencies are, how would the
> build-bots know? Perhaps there's some logic built-in somewhere that I
> don't know about?
So looks like COMPILE_TEST has a !UML dependency, so this might just
work.
Acked-by: Thierry Reding <[email protected]>
04.06.2019 14:23, Thierry Reding пишет:
> On Thu, May 02, 2019 at 02:38:14AM +0300, Dmitry Osipenko wrote:
>> In order to reflect that driver serves NVIDIA Tegra30 and later SoC
>> generations, let's rename the driver's source file to "tegra30-devfreq.c".
>> This will make driver files to look more consistent after addition of a
>> driver for Tegra20.
>>
>> Reviewed-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/devfreq/Makefile | 2 +-
>> drivers/devfreq/{tegra-devfreq.c => tegra30-devfreq.c} | 0
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>> rename drivers/devfreq/{tegra-devfreq.c => tegra30-devfreq.c} (100%)
>>
>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>> index 32b8d4d3f12c..47e5aeeebfd1 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
>> @@ -10,7 +10,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
>> # DEVFREQ Drivers
>> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
>> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
>> -obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra-devfreq.o
>> +obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
>
> Technically this changes the name of the driver. Sometimes boot or other
> scripts rely on those names. Perhaps a better way of keeping backwards-
> compatibility would be to do:
>
> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra-devfreq.o
> tegra-devfreq-y += tegra30-devfreq.o
>
> That way you can later on just add the tegra20-devfreq.o to that driver
> as well and have them both ship in one .ko.
Combining two drivers into a single kernel object certainly doesn't work
("multiple definition of `init_module'" error, etc).
Indeed, this changes the name of the driver. It should be fine as long
as it doesn't hurt anybody, so what about to keep this change as-is for
now and wait for complains? I promise to make a revert if this will
cause real problems for anyone. Let's be realistic, there should be a
very little chance that somebody will notice this change. ACK?
04.06.2019 17:18, Thierry Reding пишет:
> On Tue, Jun 04, 2019 at 04:10:31PM +0200, Thierry Reding wrote:
>> On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
>>> 04.06.2019 14:20, Thierry Reding пишет:
>>>> On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
>>>>> The driver's compilation doesn't have any specific dependencies, hence
>>>>> the COMPILE_TEST option can be supported in Kconfig.
>>>>>
>>>>> Reviewed-by: Chanwoo Choi <[email protected]>
>>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>>> ---
>>>>> drivers/devfreq/Kconfig | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>> index 56db9dc05edb..a6bba6e1e7d9 100644
>>>>> --- a/drivers/devfreq/Kconfig
>>>>> +++ b/drivers/devfreq/Kconfig
>>>>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>>>>
>>>>> config ARM_TEGRA_DEVFREQ
>>>>> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>>>>> - depends on ARCH_TEGRA
>>>>> + depends on ARCH_TEGRA || COMPILE_TEST
>>>>> select PM_OPP
>>>>> help
>>>>> This adds the DEVFREQ driver for the Tegra family of SoCs.
>>>>
>>>> You need to be careful with these. You're using I/O register accessors,
>>>> which are not supported on the UM architecture, for example.
>>>>
>>>> This may end up getting flagged during build testing.
>>>
>>> We have similar cases in other drivers and it doesn't cause any known
>>> problems because (I think) build-bots are aware of this detail. Hence
>>
>> I don't understand how the build-bots would be aware of this detail.
>> Unless you explicitly state what the dependencies are, how would the
>> build-bots know? Perhaps there's some logic built-in somewhere that I
>> don't know about?
>
> So looks like COMPILE_TEST has a !UML dependency, so this might just
> work.
>
> Acked-by: Thierry Reding <[email protected]>
>
Thank you very much for the clarification! Certainly that would caused
problems already since there are such cases all over the kernel,
including Tegra drivers.
On Tue, Jun 04, 2019 at 05:41:53PM +0300, Dmitry Osipenko wrote:
> 04.06.2019 17:18, Thierry Reding пишет:
> > On Tue, Jun 04, 2019 at 04:10:31PM +0200, Thierry Reding wrote:
> >> On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
> >>> 04.06.2019 14:20, Thierry Reding пишет:
> >>>> On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
> >>>>> The driver's compilation doesn't have any specific dependencies, hence
> >>>>> the COMPILE_TEST option can be supported in Kconfig.
> >>>>>
> >>>>> Reviewed-by: Chanwoo Choi <[email protected]>
> >>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
> >>>>> ---
> >>>>> drivers/devfreq/Kconfig | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> >>>>> index 56db9dc05edb..a6bba6e1e7d9 100644
> >>>>> --- a/drivers/devfreq/Kconfig
> >>>>> +++ b/drivers/devfreq/Kconfig
> >>>>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
> >>>>>
> >>>>> config ARM_TEGRA_DEVFREQ
> >>>>> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> >>>>> - depends on ARCH_TEGRA
> >>>>> + depends on ARCH_TEGRA || COMPILE_TEST
> >>>>> select PM_OPP
> >>>>> help
> >>>>> This adds the DEVFREQ driver for the Tegra family of SoCs.
> >>>>
> >>>> You need to be careful with these. You're using I/O register accessors,
> >>>> which are not supported on the UM architecture, for example.
> >>>>
> >>>> This may end up getting flagged during build testing.
> >>>
> >>> We have similar cases in other drivers and it doesn't cause any known
> >>> problems because (I think) build-bots are aware of this detail. Hence
> >>
> >> I don't understand how the build-bots would be aware of this detail.
> >> Unless you explicitly state what the dependencies are, how would the
> >> build-bots know? Perhaps there's some logic built-in somewhere that I
> >> don't know about?
> >
> > So looks like COMPILE_TEST has a !UML dependency, so this might just
> > work.
> >
> > Acked-by: Thierry Reding <[email protected]>
> >
>
> Thank you very much for the clarification! Certainly that would caused
> problems already since there are such cases all over the kernel,
> including Tegra drivers.
In the cases that I'm aware of we used to explicitly list all the
dependencies that would've otherwise been pulled in by the ARCH_TEGRA
dependency to make sure there were no issues.
Now that we've been discussing this I vaguely recall a discussion about
the only real issue nowadays being HAS_IOMEM and since that's only
missing on UML, that may have been the reason for why the !UML
dependency was added.
Yes, looks like that was it:
commit bc083a64b6c035135c0f80718f9e9192cc0867c6
Author: Richard Weinberger <[email protected]>
Date: Tue Aug 2 14:03:27 2016 -0700
init/Kconfig: make COMPILE_TEST depend on !UML
UML is a bit special since it does not have iomem nor dma. That means a
lot of drivers will not build if they miss a dependency on HAS_IOMEM.
s390 used to have the same issues but since it gained PCI support UML is
the only stranger.
We are tired of patching dozens of new drivers after every merge window
just to un-break allmod/yesconfig UML builds. One could argue that a
decent driver has to know on what it depends and therefore a missing
HAS_IOMEM dependency is a clear driver bug. But the dependency not
obvious and not everyone does UML builds with COMPILE_TEST enabled when
developing a device driver.
A possible solution to make these builds succeed on UML would be
providing stub functions for ioremap() and friends which fail upon
runtime. Another one is simply disabling COMPILE_TEST for UML. Since
it is the least hassle and does not force use to fake iomem support
let's do the latter.
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Richard Weinberger <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Cc: Al Viro <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
Oh wow... almost three years now.
Thierry
04.06.2019 17:06, Thierry Reding пишет:
> On Tue, Jun 04, 2019 at 04:40:18PM +0300, Dmitry Osipenko wrote:
>> 04.06.2019 14:07, Thierry Reding пишет:
>>> On Thu, May 02, 2019 at 02:38:06AM +0300, Dmitry Osipenko wrote:
>>>> There is no guarantee that interrupt handling isn't running in parallel
>>>> with tegra_actmon_disable_interrupts(), hence it is necessary to protect
>>>> DEV_CTRL register accesses and clear IRQ status with ACTMON's IRQ being
>>>> disabled in the Interrupt Controller in order to ensure that device
>>>> interrupt is indeed being disabled.
>>>>
>>>> Reviewed-by: Chanwoo Choi <[email protected]>
>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>> ---
>>>> drivers/devfreq/tegra-devfreq.c | 21 +++++++++++++++------
>>>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>>>> index b65313fe3c2e..ce1eb97a2090 100644
>>>> --- a/drivers/devfreq/tegra-devfreq.c
>>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>>> @@ -171,6 +171,8 @@ struct tegra_devfreq {
>>>> struct notifier_block rate_change_nb;
>>>>
>>>> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>>>> +
>>>> + int irq;
>>>
>>> Interrupts are typically unsigned int.
>>>
>>>> };
>>>>
>>>> struct tegra_actmon_emc_ratio {
>>>> @@ -417,6 +419,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>>>> u32 val;
>>>> unsigned int i;
>>>>
>>>> + disable_irq(tegra->irq);
>>>> +
>>>> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>>>> dev = &tegra->devices[i];
>>>>
>>>> @@ -427,9 +431,14 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>>>> val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>>>>
>>>> device_writel(dev, val, ACTMON_DEV_CTRL);
>>>> +
>>>> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
>>>> + ACTMON_DEV_INTR_STATUS);
>>>> }
>>>>
>>>> actmon_write_barrier(tegra);
>>>> +
>>>> + enable_irq(tegra->irq);
>>>
>>> Why do we enable interrupts after this? Is there any use in having the
>>> top-level interrupt enabled if nothing's going to generate an interrupt
>>> anyway?
>>
>> There is no real point in having the interrupt enabled other than to
>> keep the enable count balanced.
>>
>> IIUC, we will need to disable IRQ at the driver's probe time (after
>> requesting the IRQ) if we want to avoid that (not really necessary)
>> balancing. This is probably something that could be improved in a
>> follow-up patches, if desired.
>>
>>>> }
>>>>
>>>> static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>>>> @@ -604,7 +613,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>> struct resource *res;
>>>> unsigned int i;
>>>> unsigned long rate;
>>>> - int irq;
>>>> int err;
>>>>
>>>> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>>>> @@ -673,15 +681,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>> dev_pm_opp_add(&pdev->dev, rate, 0);
>>>> }
>>>>
>>>> - irq = platform_get_irq(pdev, 0);
>>>> - if (irq < 0) {
>>>> - dev_err(&pdev->dev, "Failed to get IRQ: %d\n", irq);
>>>> - return irq;
>>>> + tegra->irq = platform_get_irq(pdev, 0);
>>>> + if (tegra->irq < 0) {
>>>> + err = tegra->irq;
>>>> + dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
>>>> + return err;
>>>> }
>>>
>>> This is very oddly written. tegra->irq should really be an unsigned int
>>> since that's the standard type for interrupt numbers. But since you need
>>> to be able to detect errors from platform_get_irq() it now becomes
>>> natural to write this as:
>>>
>>> err = platform_get_irq(pdev, 0);
>>> if (err < 0) {
>>> dev_err(...);
>>> return err;
>>> }
>>>
>>> tegra->irq = err;
>>>
>>> Two birds with one stone. I suppose this could be done in a follow-up
>>> patch since it isn't practically wrong in your version, so either way:
>>>
>>> Acked-by: Thierry Reding <[email protected]>
>>>
>>
>> Thank you for the ACK! Although, I disagree with yours suggestion, to me
>> that makes code a bit less straightforward and it's not really
>> worthwhile to bloat the code just because technically IRQ's are unsigned
>> numbers (we don't care about that). It also makes me a bit uncomfortable
>> to see "err" assigned to a variable, I don't think it's a good practice.
>
> Actually you should care that IRQs are unsigned. Implicit casting from
> a potentially negative value can hide bugs. That is, once you've passed
> that negative value into the IRQ API you loose the context that it could
> be an error code. Hence I think it makes sense to always store values in
> the native type, and only store them if they are actually valid.
>
> In the above you have an error value in tegra->irq. In this particular
> case it's pretty harmless because you don't do anything with it, but if
> the circumstances were slightly different that could lead to problems
> down the road.
>
> On the other hand what I was proposing makes it pretty clear from the
> context that err contains a valid interrupt number when it is assigned
> to tegra->irq. There's plenty of similar constructs in the kernel if you
> want to grep for it.
>
> Also, it's not bloating the code at all. It's the exact same number of
> lines of code as your variant.
I agree that it is better to maintain proper typing everywhere in
general, I have been bitten so many times by typecasting bugs..
Opentegra's Bool (unsigned) -> BOOL (signed) casting horror was the most
recent one. Well, I guess indeed it won't hurt to apply your suggestion
in a follow-up patch to keep things a bit more consistent.
04.06.2019 17:50, Thierry Reding пишет:
> On Tue, Jun 04, 2019 at 05:41:53PM +0300, Dmitry Osipenko wrote:
>> 04.06.2019 17:18, Thierry Reding пишет:
>>> On Tue, Jun 04, 2019 at 04:10:31PM +0200, Thierry Reding wrote:
>>>> On Tue, Jun 04, 2019 at 04:53:17PM +0300, Dmitry Osipenko wrote:
>>>>> 04.06.2019 14:20, Thierry Reding пишет:
>>>>>> On Thu, May 02, 2019 at 02:38:13AM +0300, Dmitry Osipenko wrote:
>>>>>>> The driver's compilation doesn't have any specific dependencies, hence
>>>>>>> the COMPILE_TEST option can be supported in Kconfig.
>>>>>>>
>>>>>>> Reviewed-by: Chanwoo Choi <[email protected]>
>>>>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>>>>> ---
>>>>>>> drivers/devfreq/Kconfig | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>>>> index 56db9dc05edb..a6bba6e1e7d9 100644
>>>>>>> --- a/drivers/devfreq/Kconfig
>>>>>>> +++ b/drivers/devfreq/Kconfig
>>>>>>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>>>>>>
>>>>>>> config ARM_TEGRA_DEVFREQ
>>>>>>> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>>>>>>> - depends on ARCH_TEGRA
>>>>>>> + depends on ARCH_TEGRA || COMPILE_TEST
>>>>>>> select PM_OPP
>>>>>>> help
>>>>>>> This adds the DEVFREQ driver for the Tegra family of SoCs.
>>>>>>
>>>>>> You need to be careful with these. You're using I/O register accessors,
>>>>>> which are not supported on the UM architecture, for example.
>>>>>>
>>>>>> This may end up getting flagged during build testing.
>>>>>
>>>>> We have similar cases in other drivers and it doesn't cause any known
>>>>> problems because (I think) build-bots are aware of this detail. Hence
>>>>
>>>> I don't understand how the build-bots would be aware of this detail.
>>>> Unless you explicitly state what the dependencies are, how would the
>>>> build-bots know? Perhaps there's some logic built-in somewhere that I
>>>> don't know about?
>>>
>>> So looks like COMPILE_TEST has a !UML dependency, so this might just
>>> work.
>>>
>>> Acked-by: Thierry Reding <[email protected]>
>>>
>>
>> Thank you very much for the clarification! Certainly that would caused
>> problems already since there are such cases all over the kernel,
>> including Tegra drivers.
>
> In the cases that I'm aware of we used to explicitly list all the
> dependencies that would've otherwise been pulled in by the ARCH_TEGRA
> dependency to make sure there were no issues.
>
> Now that we've been discussing this I vaguely recall a discussion about
> the only real issue nowadays being HAS_IOMEM and since that's only
> missing on UML, that may have been the reason for why the !UML
> dependency was added.
>
> Yes, looks like that was it:
>
> commit bc083a64b6c035135c0f80718f9e9192cc0867c6
> Author: Richard Weinberger <[email protected]>
> Date: Tue Aug 2 14:03:27 2016 -0700
>
> init/Kconfig: make COMPILE_TEST depend on !UML
>
> UML is a bit special since it does not have iomem nor dma. That means a
> lot of drivers will not build if they miss a dependency on HAS_IOMEM.
> s390 used to have the same issues but since it gained PCI support UML is
> the only stranger.
>
> We are tired of patching dozens of new drivers after every merge window
> just to un-break allmod/yesconfig UML builds. One could argue that a
> decent driver has to know on what it depends and therefore a missing
> HAS_IOMEM dependency is a clear driver bug. But the dependency not
> obvious and not everyone does UML builds with COMPILE_TEST enabled when
> developing a device driver.
>
> A possible solution to make these builds succeed on UML would be
> providing stub functions for ioremap() and friends which fail upon
> runtime. Another one is simply disabling COMPILE_TEST for UML. Since
> it is the least hassle and does not force use to fake iomem support
> let's do the latter.
>
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: Richard Weinberger <[email protected]>
> Acked-by: Arnd Bergmann <[email protected]>
> Cc: Al Viro <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> Signed-off-by: Linus Torvalds <[email protected]>
That was a wise solution. Thank you very much again for the detailed
comment! Very appreciate that!
> Oh wow... almost three years now.
Please don't pay much attention to the time, it never stops. Live in the
moment :)
04.06.2019 16:40, Dmitry Osipenko пишет:
> 04.06.2019 14:07, Thierry Reding пишет:
>> On Thu, May 02, 2019 at 02:38:06AM +0300, Dmitry Osipenko wrote:
>>> There is no guarantee that interrupt handling isn't running in parallel
>>> with tegra_actmon_disable_interrupts(), hence it is necessary to protect
>>> DEV_CTRL register accesses and clear IRQ status with ACTMON's IRQ being
>>> disabled in the Interrupt Controller in order to ensure that device
>>> interrupt is indeed being disabled.
>>>
>>> Reviewed-by: Chanwoo Choi <[email protected]>
>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>> ---
>>> drivers/devfreq/tegra-devfreq.c | 21 +++++++++++++++------
>>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>>> index b65313fe3c2e..ce1eb97a2090 100644
>>> --- a/drivers/devfreq/tegra-devfreq.c
>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>> @@ -171,6 +171,8 @@ struct tegra_devfreq {
>>> struct notifier_block rate_change_nb;
>>>
>>> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>>> +
>>> + int irq;
>>
>> Interrupts are typically unsigned int.
>>
>>> };
>>>
>>> struct tegra_actmon_emc_ratio {
>>> @@ -417,6 +419,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>>> u32 val;
>>> unsigned int i;
>>>
>>> + disable_irq(tegra->irq);
>>> +
>>> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>>> dev = &tegra->devices[i];
>>>
>>> @@ -427,9 +431,14 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>>> val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>>>
>>> device_writel(dev, val, ACTMON_DEV_CTRL);
>>> +
>>> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
>>> + ACTMON_DEV_INTR_STATUS);
>>> }
>>>
>>> actmon_write_barrier(tegra);
>>> +
>>> + enable_irq(tegra->irq);
>>
>> Why do we enable interrupts after this? Is there any use in having the
>> top-level interrupt enabled if nothing's going to generate an interrupt
>> anyway?
>
> There is no real point in having the interrupt enabled other than to
> keep the enable count balanced.
>
> IIUC, we will need to disable IRQ at the driver's probe time (after
> requesting the IRQ) if we want to avoid that (not really necessary)
> balancing. This is probably something that could be improved in a
> follow-up patches, if desired.
Nah, it's not worth the effort. It is quite problematic that we can't
keep interrupt disabled during of devfreq_add_device() execution because
it asks governor to enable the interrupt and the interrupt shall be
disabled because we're using device's lock in the governor interrupt
handler.. device is getting assigned only after completion of the
devfreq_add_device() and hence ISR gets a NULL deref if it is fired
before device is assigned. So I'll leave this part as-is.
Thierry, please answer to all of the remaining patches where you had
some concerns. I'll send out another series on top of this, addressing
yours comments and fixing another bug that I spotted today.
04.06.2019 3:49, Chanwoo Choi пишет:
> On 19. 6. 4. 오전 1:52, Dmitry Osipenko wrote:
>> 03.05.2019 3:52, Dmitry Osipenko пишет:
>>> 03.05.2019 3:31, Chanwoo Choi пишет:
>>>> Hi Dmitry,
>>>>
>>>> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>>>>> Changelog:
>>>>>
>>>>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>>>>
>>>>> - changed the driver removal order to match the probe exactly
>>>>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>>>>
>>>>> Chanwoo, please also note that the clk patch that should fix
>>>>> compilation problem that was reported the kbuild-test-robot is already
>>>>> applied and available in the recent linux-next.
>>>>
>>>> I knew that Stephen picked up your path about clock.
>>>
>>> Hi Chanwoo,
>>>
>>> Okay, good. Thank you very much for reviewing this series! I assume it's
>>> too late now for v5.2, but it should be good to go for v5.3.
>>>
>>
>> Hello Chanwoo,
>>
>> Will be nice to see the patches in the linux-next before they'll hit mainline. We have tested that
>> everything works fine on a selective devices, but won't hurt to get some extra testing beforehand.
>> AFAIK, at least NVIDIA people are regularly testing -next on theirs dev boards. Please note that
>> this not very important, so don't bother if there is some hurdle with pushing to the tracking branch
>> for now. Also please let me know if you're expecting to see some ACK's on the patches, I'm sure
>> we'll be able to work out that with Thierry and Jon if necessary.
>>
>>
>
> Hi Dmitry,
> I think that it is enough for applying to mainline branch.
> The devfreq.git is maintained by Myungjoo. He will be merged or
> reviewed if there are th remained review point.
Thank you very much!
>
> Hi Myungjoo,
> I reviewed the Dmitry's patches from v1 to v4 patches.
> And then I tested them on my testing branch[1] for catching
> the build warning and error. In result, it is clean.
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
>
> Please review or apply these patches for v5.3.
>
Hello Myungjoo,
I think this patchset should be completed now. Thierry has some extra
comments to the patches, but seems nothing critical so far and all the
concerns could be addressed in a follow-up series. Please let me know if
you're fine with this, I can re-spin v5 as well if necessary.
05.06.2019 1:55, Dmitry Osipenko пишет:
> 04.06.2019 16:40, Dmitry Osipenko пишет:
>> 04.06.2019 14:07, Thierry Reding пишет:
>>> On Thu, May 02, 2019 at 02:38:06AM +0300, Dmitry Osipenko wrote:
>>>> There is no guarantee that interrupt handling isn't running in parallel
>>>> with tegra_actmon_disable_interrupts(), hence it is necessary to protect
>>>> DEV_CTRL register accesses and clear IRQ status with ACTMON's IRQ being
>>>> disabled in the Interrupt Controller in order to ensure that device
>>>> interrupt is indeed being disabled.
>>>>
>>>> Reviewed-by: Chanwoo Choi <[email protected]>
>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>> ---
>>>> drivers/devfreq/tegra-devfreq.c | 21 +++++++++++++++------
>>>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>>>> index b65313fe3c2e..ce1eb97a2090 100644
>>>> --- a/drivers/devfreq/tegra-devfreq.c
>>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>>> @@ -171,6 +171,8 @@ struct tegra_devfreq {
>>>> struct notifier_block rate_change_nb;
>>>>
>>>> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>>>> +
>>>> + int irq;
>>>
>>> Interrupts are typically unsigned int.
>>>
>>>> };
>>>>
>>>> struct tegra_actmon_emc_ratio {
>>>> @@ -417,6 +419,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>>>> u32 val;
>>>> unsigned int i;
>>>>
>>>> + disable_irq(tegra->irq);
>>>> +
>>>> for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>>>> dev = &tegra->devices[i];
>>>>
>>>> @@ -427,9 +431,14 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>>>> val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>>>>
>>>> device_writel(dev, val, ACTMON_DEV_CTRL);
>>>> +
>>>> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
>>>> + ACTMON_DEV_INTR_STATUS);
>>>> }
>>>>
>>>> actmon_write_barrier(tegra);
>>>> +
>>>> + enable_irq(tegra->irq);
>>>
>>> Why do we enable interrupts after this? Is there any use in having the
>>> top-level interrupt enabled if nothing's going to generate an interrupt
>>> anyway?
>>
>> There is no real point in having the interrupt enabled other than to
>> keep the enable count balanced.
>>
>> IIUC, we will need to disable IRQ at the driver's probe time (after
>> requesting the IRQ) if we want to avoid that (not really necessary)
>> balancing. This is probably something that could be improved in a
>> follow-up patches, if desired.
>
> Nah, it's not worth the effort. It is quite problematic that we can't
> keep interrupt disabled during of devfreq_add_device() execution because
> it asks governor to enable the interrupt and the interrupt shall be
> disabled because we're using device's lock in the governor interrupt
> handler.. device is getting assigned only after completion of the
> devfreq_add_device() and hence ISR gets a NULL deref if it is fired
> before device is assigned. So I'll leave this part as-is.
>
> Thierry, please answer to all of the remaining patches where you had
> some concerns. I'll send out another series on top of this, addressing
> yours comments and fixing another bug that I spotted today.
>
I looked at this once again and found that the interrupt could be kept
disabled on request using the IRQ_NOAUTOEN flag and then the device
could be assigned within the governor's event handler, so everything is
resolved very nicely! :)
I'll send patches addressing this comment and the rest after getting
relies from you guys. Please try to not postpone the responses too much
as more interactivity in a review/apply process usually help quite a
lot, thanks in advance!
05.06.2019 2:09, Dmitry Osipenko пишет:
> 04.06.2019 3:49, Chanwoo Choi пишет:
>> On 19. 6. 4. 오전 1:52, Dmitry Osipenko wrote:
>>> 03.05.2019 3:52, Dmitry Osipenko пишет:
>>>> 03.05.2019 3:31, Chanwoo Choi пишет:
>>>>> Hi Dmitry,
>>>>>
>>>>> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>>>>>> Changelog:
>>>>>>
>>>>>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>>>>>
>>>>>> - changed the driver removal order to match the probe exactly
>>>>>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>>>>>
>>>>>> Chanwoo, please also note that the clk patch that should fix
>>>>>> compilation problem that was reported the kbuild-test-robot is already
>>>>>> applied and available in the recent linux-next.
>>>>>
>>>>> I knew that Stephen picked up your path about clock.
>>>>
>>>> Hi Chanwoo,
>>>>
>>>> Okay, good. Thank you very much for reviewing this series! I assume it's
>>>> too late now for v5.2, but it should be good to go for v5.3.
>>>>
>>>
>>> Hello Chanwoo,
>>>
>>> Will be nice to see the patches in the linux-next before they'll hit mainline. We have tested that
>>> everything works fine on a selective devices, but won't hurt to get some extra testing beforehand.
>>> AFAIK, at least NVIDIA people are regularly testing -next on theirs dev boards. Please note that
>>> this not very important, so don't bother if there is some hurdle with pushing to the tracking branch
>>> for now. Also please let me know if you're expecting to see some ACK's on the patches, I'm sure
>>> we'll be able to work out that with Thierry and Jon if necessary.
>>>
>>>
>>
>> Hi Dmitry,
>> I think that it is enough for applying to mainline branch.
>> The devfreq.git is maintained by Myungjoo. He will be merged or
>> reviewed if there are th remained review point.
>
> Thank you very much!
>
>>
>> Hi Myungjoo,
>> I reviewed the Dmitry's patches from v1 to v4 patches.
>> And then I tested them on my testing branch[1] for catching
>> the build warning and error. In result, it is clean.
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
>>
>> Please review or apply these patches for v5.3.
>>
>
> Hello Myungjoo,
>
> I think this patchset should be completed now. Thierry has some extra
> comments to the patches, but seems nothing critical so far and all the
> concerns could be addressed in a follow-up series. Please let me know if
> you're fine with this, I can re-spin v5 as well if necessary.
>
Hello Chanwoo,
It looks like Myungjoo is inactive at the moment. Do you know if he'll
be back to the time of the merge window opening or you'll be curating
the pull request for 5.3 this time?
Secondly, I'll send a few more patches on top of this series, addressing
Thierry's comments and making more improvements. Please let me know if
this causes any problems and I should re-spin the whole series.
Hi Dmitry,
On 19. 6. 24. 오전 2:17, Dmitry Osipenko wrote:
> 05.06.2019 2:09, Dmitry Osipenko пишет:
>> 04.06.2019 3:49, Chanwoo Choi пишет:
>>> On 19. 6. 4. 오전 1:52, Dmitry Osipenko wrote:
>>>> 03.05.2019 3:52, Dmitry Osipenko пишет:
>>>>> 03.05.2019 3:31, Chanwoo Choi пишет:
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>>>>>>> Changelog:
>>>>>>>
>>>>>>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>>>>>>
>>>>>>> - changed the driver removal order to match the probe exactly
>>>>>>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>>>>>>
>>>>>>> Chanwoo, please also note that the clk patch that should fix
>>>>>>> compilation problem that was reported the kbuild-test-robot is already
>>>>>>> applied and available in the recent linux-next.
>>>>>>
>>>>>> I knew that Stephen picked up your path about clock.
>>>>>
>>>>> Hi Chanwoo,
>>>>>
>>>>> Okay, good. Thank you very much for reviewing this series! I assume it's
>>>>> too late now for v5.2, but it should be good to go for v5.3.
>>>>>
>>>>
>>>> Hello Chanwoo,
>>>>
>>>> Will be nice to see the patches in the linux-next before they'll hit mainline. We have tested that
>>>> everything works fine on a selective devices, but won't hurt to get some extra testing beforehand.
>>>> AFAIK, at least NVIDIA people are regularly testing -next on theirs dev boards. Please note that
>>>> this not very important, so don't bother if there is some hurdle with pushing to the tracking branch
>>>> for now. Also please let me know if you're expecting to see some ACK's on the patches, I'm sure
>>>> we'll be able to work out that with Thierry and Jon if necessary.
>>>>
>>>>
>>>
>>> Hi Dmitry,
>>> I think that it is enough for applying to mainline branch.
>>> The devfreq.git is maintained by Myungjoo. He will be merged or
>>> reviewed if there are th remained review point.
>>
>> Thank you very much!
>>
>>>
>>> Hi Myungjoo,
>>> I reviewed the Dmitry's patches from v1 to v4 patches.
>>> And then I tested them on my testing branch[1] for catching
>>> the build warning and error. In result, it is clean.
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
>>>
>>> Please review or apply these patches for v5.3.
>>>
>>
>> Hello Myungjoo,
>>
>> I think this patchset should be completed now. Thierry has some extra
>> comments to the patches, but seems nothing critical so far and all the
>> concerns could be addressed in a follow-up series. Please let me know if
>> you're fine with this, I can re-spin v5 as well if necessary.
>>
>
> Hello Chanwoo,
>
> It looks like Myungjoo is inactive at the moment. Do you know if he'll
> be back to the time of the merge window opening or you'll be curating
> the pull request for 5.3 this time?
Myungoo works in the same place. I'll talk with him.
>
> Secondly, I'll send a few more patches on top of this series, addressing
> Thierry's comments and making more improvements. Please let me know if
> this causes any problems and I should re-spin the whole series.
OK. I'll review them.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
24.06.2019 2:50, Chanwoo Choi пишет:
> Hi Dmitry,
>
> On 19. 6. 24. 오전 2:17, Dmitry Osipenko wrote:
>> 05.06.2019 2:09, Dmitry Osipenko пишет:
>>> 04.06.2019 3:49, Chanwoo Choi пишет:
>>>> On 19. 6. 4. 오전 1:52, Dmitry Osipenko wrote:
>>>>> 03.05.2019 3:52, Dmitry Osipenko пишет:
>>>>>> 03.05.2019 3:31, Chanwoo Choi пишет:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> On 19. 5. 2. 오전 8:37, Dmitry Osipenko wrote:
>>>>>>>> Changelog:
>>>>>>>>
>>>>>>>> v4: Addressed all review comments that were made by Chanwoo Choi to v3:
>>>>>>>>
>>>>>>>> - changed the driver removal order to match the probe exactly
>>>>>>>> - added clarifying comment for 1/8 ratio to the Tegra20 driver
>>>>>>>>
>>>>>>>> Chanwoo, please also note that the clk patch that should fix
>>>>>>>> compilation problem that was reported the kbuild-test-robot is already
>>>>>>>> applied and available in the recent linux-next.
>>>>>>>
>>>>>>> I knew that Stephen picked up your path about clock.
>>>>>>
>>>>>> Hi Chanwoo,
>>>>>>
>>>>>> Okay, good. Thank you very much for reviewing this series! I assume it's
>>>>>> too late now for v5.2, but it should be good to go for v5.3.
>>>>>>
>>>>>
>>>>> Hello Chanwoo,
>>>>>
>>>>> Will be nice to see the patches in the linux-next before they'll hit mainline. We have tested that
>>>>> everything works fine on a selective devices, but won't hurt to get some extra testing beforehand.
>>>>> AFAIK, at least NVIDIA people are regularly testing -next on theirs dev boards. Please note that
>>>>> this not very important, so don't bother if there is some hurdle with pushing to the tracking branch
>>>>> for now. Also please let me know if you're expecting to see some ACK's on the patches, I'm sure
>>>>> we'll be able to work out that with Thierry and Jon if necessary.
>>>>>
>>>>>
>>>>
>>>> Hi Dmitry,
>>>> I think that it is enough for applying to mainline branch.
>>>> The devfreq.git is maintained by Myungjoo. He will be merged or
>>>> reviewed if there are th remained review point.
>>>
>>> Thank you very much!
>>>
>>>>
>>>> Hi Myungjoo,
>>>> I reviewed the Dmitry's patches from v1 to v4 patches.
>>>> And then I tested them on my testing branch[1] for catching
>>>> the build warning and error. In result, it is clean.
>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
>>>>
>>>> Please review or apply these patches for v5.3.
>>>>
>>>
>>> Hello Myungjoo,
>>>
>>> I think this patchset should be completed now. Thierry has some extra
>>> comments to the patches, but seems nothing critical so far and all the
>>> concerns could be addressed in a follow-up series. Please let me know if
>>> you're fine with this, I can re-spin v5 as well if necessary.
>>>
>>
>> Hello Chanwoo,
>>
>> It looks like Myungjoo is inactive at the moment. Do you know if he'll
>> be back to the time of the merge window opening or you'll be curating
>> the pull request for 5.3 this time?
>
> Myungoo works in the same place. I'll talk with him.
>
>>
>> Secondly, I'll send a few more patches on top of this series, addressing
>> Thierry's comments and making more improvements. Please let me know if
>> this causes any problems and I should re-spin the whole series.
>
> OK. I'll review them.
Thank you!
> On Thu, May 02, 2019 at 02:38:12AM +0300, Dmitry Osipenko wrote:
> > The devfreq driver can be used on Tegra30 without any code change and
> > it works perfectly fine, the default Tegra124 parameters are good enough
> > for Tegra30.
> >
> > Reviewed-by: Chanwoo Choi
> > Signed-off-by: Dmitry Osipenko
> > ---
> > drivers/devfreq/Kconfig | 4 ++--
> > drivers/devfreq/tegra-devfreq.c | 1 +
> > 2 files changed, 3 insertions(+), 2 deletions(-)
>
> Acked-by: Thierry Reding
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 7dd46d4..b291803 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -93,8 +93,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
> This does not yet operate with optimal voltages.
>
> config ARM_TEGRA_DEVFREQ
> - tristate "Tegra DEVFREQ Driver"
> - depends on ARCH_TEGRA_124_SOC
> + tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> + depends on ARCH_TEGRA
> select PM_OPP
> help
> This adds the DEVFREQ driver for the Tegra family of SoCs.
A question:
Does this driver support Tegra20 as well?
I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
according to /drivers/soc/tegra/Kconfig.
Cheers,
MyungJoo.
>
>A question:
>
>Does this driver support Tegra20 as well?
>I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>according to /drivers/soc/tegra/Kconfig.
>
For this matter, how about updating your 13/16 patch as follows?
---
drivers/devfreq/Kconfig | 4 ++--
drivers/devfreq/tegra-devfreq.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 7dd46d44579d..78c4b436aad8 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -93,8 +93,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
This does not yet operate with optimal voltages.
config ARM_TEGRA_DEVFREQ
- tristate "Tegra DEVFREQ Driver"
- depends on ARCH_TEGRA_124_SOC
+ tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
+ depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || ARCH_TEGRA_124_SOC || ARCH_TEGRA_210_SOC
select PM_OPP
help
This adds the DEVFREQ driver for the Tegra family of SoCs.
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 5cddf2199c4e..a6ba75f4106d 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -726,6 +726,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
}
static const struct of_device_id tegra_devfreq_of_match[] = {
+ { .compatible = "nvidia,tegra30-actmon" },
{ .compatible = "nvidia,tegra124-actmon" },
{ },
};
--
2.17.1
24.06.2019 10:34, MyungJoo Ham пишет:
>>
>> A question:
>>
>> Does this driver support Tegra20 as well?
>> I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>> according to /drivers/soc/tegra/Kconfig.
>>
>
> For this matter, how about updating your 13/16 patch as follows?
>
> ---
> drivers/devfreq/Kconfig | 4 ++--
> drivers/devfreq/tegra-devfreq.c | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 7dd46d44579d..78c4b436aad8 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -93,8 +93,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
> This does not yet operate with optimal voltages.
>
> config ARM_TEGRA_DEVFREQ
> - tristate "Tegra DEVFREQ Driver"
> - depends on ARCH_TEGRA_124_SOC
> + tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> + depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || ARCH_TEGRA_124_SOC || ARCH_TEGRA_210_SOC
> select PM_OPP
> help
> This adds the DEVFREQ driver for the Tegra family of SoCs.
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 5cddf2199c4e..a6ba75f4106d 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -726,6 +726,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
> }
>
> static const struct of_device_id tegra_devfreq_of_match[] = {
> + { .compatible = "nvidia,tegra30-actmon" },
> { .compatible = "nvidia,tegra124-actmon" },
> { },
> };
>
Good call! I'll update this patch following yours suggestion, thanks.
>
>--------- Original Message ---------
>Sender : Dmitry Osipenko <[email protected]>
>
>24.06.2019 10:34, MyungJoo Ham пишет:
>>>
>>> A question:
>>>
>>> Does this driver support Tegra20 as well?
>>> I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>>> according to /drivers/soc/tegra/Kconfig.
>>>
>>
>> For this matter, how about updating your 13/16 patch as follows?
>>
[]
>
>Good call! I'll update this patch following yours suggestion, thanks.
Or, you may approve the modified commits here:
https://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git/log/?h=for-next
Cheers,
MyungJoo
24.06.2019 14:11, MyungJoo Ham пишет:
>>
>> --------- Original Message ---------
>> Sender : Dmitry Osipenko <[email protected]>
>>
>> 24.06.2019 10:34, MyungJoo Ham пишет:
>>>>
>>>> A question:
>>>>
>>>> Does this driver support Tegra20 as well?
>>>> I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>>>> according to /drivers/soc/tegra/Kconfig.
>>>>
>>>
>>> For this matter, how about updating your 13/16 patch as follows?
>>>
> []
>>
>> Good call! I'll update this patch following yours suggestion, thanks.
>
> Or, you may approve the modified commits here:
> https://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git/log/?h=for-next
Looks almost good to me!
I just recalled that there is also a 64bit variant of Tegra124, the Tegra132. Hence
the Tegra30+ Kconfig entry should look like this (it's also worthy to break the lines
for readability):
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index ccb1a68c4b51..bd2efbc27725 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -94,7 +94,10 @@ config ARM_EXYNOS_BUS_DEVFREQ
config ARM_TEGRA_DEVFREQ
tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
- depends on ARCH_TEGRA || COMPILE_TEST
+ depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
+ ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
+ ARCH_TEGRA_210_SOC || \
+ COMPILE_TEST
select PM_OPP
help
This adds the DEVFREQ driver for the Tegra family of SoCs.
Could you please adjust the patches like I'm suggesting? I'll approve yours change
then and won't re-spin the first batch of the patches.
Sender : Dmitry Osipenko <[email protected]>
>24.06.2019 14:11, MyungJoo Ham пишет:
>>>
>>> --------- Original Message ---------
>>> Sender : Dmitry Osipenko <[email protected]>
>>>
>>> 24.06.2019 10:34, MyungJoo Ham пишет:
>>>>>
>>>>> A question:
>>>>>
>>>>> Does this driver support Tegra20 as well?
>>>>> I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>>>>> according to /drivers/soc/tegra/Kconfig.
>>>>>
>>>>
>>>> For this matter, how about updating your 13/16 patch as follows?
>>>>
>> []
>>>
>>> Good call! I'll update this patch following yours suggestion, thanks.
>>
>> Or, you may approve the modified commits here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git/log/?h=for-next
>
>Looks almost good to me!
>
>I just recalled that there is also a 64bit variant of Tegra124, the Tegra132. Hence
>the Tegra30+ Kconfig entry should look like this (it's also worthy to break the lines
>for readability):
>
>diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>index ccb1a68c4b51..bd2efbc27725 100644
>--- a/drivers/devfreq/Kconfig
>+++ b/drivers/devfreq/Kconfig
>@@ -94,7 +94,10 @@ config ARM_EXYNOS_BUS_DEVFREQ
>
> config ARM_TEGRA_DEVFREQ
> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>- depends on ARCH_TEGRA || COMPILE_TEST
>+ depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
>+ ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
>+ ARCH_TEGRA_210_SOC || \
>+ COMPILE_TEST
> select PM_OPP
> help
> This adds the DEVFREQ driver for the Tegra family of SoCs.
>
>Could you please adjust the patches like I'm suggesting? I'll approve yours change
>then and won't re-spin the first batch of the patches.
I've adjusted as you suggested. It's pushed to the git repo as well.
Cheers,
MyungJoo.
25.06.2019 4:42, MyungJoo Ham пишет:
> Sender : Dmitry Osipenko <[email protected]>
>> 24.06.2019 14:11, MyungJoo Ham пишет:
>>>>
>>>> --------- Original Message ---------
>>>> Sender : Dmitry Osipenko <[email protected]>
>>>>
>>>> 24.06.2019 10:34, MyungJoo Ham пишет:
>>>>>>
>>>>>> A question:
>>>>>>
>>>>>> Does this driver support Tegra20 as well?
>>>>>> I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>>>>>> according to /drivers/soc/tegra/Kconfig.
>>>>>>
>>>>>
>>>>> For this matter, how about updating your 13/16 patch as follows?
>>>>>
>>> []
>>>>
>>>> Good call! I'll update this patch following yours suggestion, thanks.
>>>
>>> Or, you may approve the modified commits here:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git/log/?h=for-next
>>
>> Looks almost good to me!
>>
>> I just recalled that there is also a 64bit variant of Tegra124, the Tegra132. Hence
>> the Tegra30+ Kconfig entry should look like this (it's also worthy to break the lines
>> for readability):
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index ccb1a68c4b51..bd2efbc27725 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -94,7 +94,10 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>
>> config ARM_TEGRA_DEVFREQ
>> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>> - depends on ARCH_TEGRA || COMPILE_TEST
>> + depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
>> + ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
>> + ARCH_TEGRA_210_SOC || \
>> + COMPILE_TEST
>> select PM_OPP
>> help
>> This adds the DEVFREQ driver for the Tegra family of SoCs.
>>
>> Could you please adjust the patches like I'm suggesting? I'll approve yours change
>> then and won't re-spin the first batch of the patches.
>
> I've adjusted as you suggested. It's pushed to the git repo as well.
Thank you very much, looking good now!