From 88763a5cf80ca59a7c3bea32681ce8f697d9995f Mon Sep 17 00:00:00 2001 From: Daniel Lezcano Date: Tue, 26 Jun 2018 12:53:29 +0200 Subject: [PATCH 01/11] powercap / idle_inject: Add an idle injection framework Initially, the cpu_cooling device for ARM was changed by adding a new policy inserting idle cycles. The intel_powerclamp driver does a similar action. Instead of implementing idle injections privately in the cpu_cooling device, move the idle injection code in a dedicated framework and give the opportunity to other frameworks to make use of it. The framework relies on the smpboot kthreads which handles via its main loop the common code for hotplugging and [un]parking. This code was previously tested with the cpu cooling device and went through several iterations. It results now in split code and API exported in the header file. It was tested with the cpu cooling device with success. Signed-off-by: Daniel Lezcano Reviewed-by: Viresh Kumar [ rjw: Rewrite of all comments ] Signed-off-by: Rafael J. Wysocki --- drivers/powercap/Kconfig | 10 + drivers/powercap/Makefile | 1 + drivers/powercap/idle_inject.c | 356 +++++++++++++++++++++++++++++++++ include/linux/idle_inject.h | 29 +++ 4 files changed, 396 insertions(+) create mode 100644 drivers/powercap/idle_inject.c create mode 100644 include/linux/idle_inject.h diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig index 85727ef6ce8e..6ac27e5908f5 100644 --- a/drivers/powercap/Kconfig +++ b/drivers/powercap/Kconfig @@ -29,4 +29,14 @@ config INTEL_RAPL controller, CPU core (Power Plance 0), graphics uncore (Power Plane 1), etc. +config IDLE_INJECT + bool "Idle injection framework" + depends on CPU_IDLE + default n + help + This enables support for the idle injection framework. It + provides a way to force idle periods on a set of specified + CPUs for power capping. Idle period can be injected + synchronously on a set of specified CPUs or alternatively + on a per CPU basis. endif diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile index 0a21ef31372b..1b328854b36e 100644 --- a/drivers/powercap/Makefile +++ b/drivers/powercap/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_POWERCAP) += powercap_sys.o obj-$(CONFIG_INTEL_RAPL) += intel_rapl.o +obj-$(CONFIG_IDLE_INJECT) += idle_inject.o diff --git a/drivers/powercap/idle_inject.c b/drivers/powercap/idle_inject.c new file mode 100644 index 000000000000..24ff2a068978 --- /dev/null +++ b/drivers/powercap/idle_inject.c @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2018 Linaro Limited + * + * Author: Daniel Lezcano + * + * The idle injection framework provides a way to force CPUs to enter idle + * states for a specified fraction of time over a specified period. + * + * It relies on the smpboot kthreads feature providing common code for CPU + * hotplug and thread [un]parking. + * + * All of the kthreads used for idle injection are created at init time. + * + * Next, the users of the the idle injection framework provide a cpumask via + * its register function. The kthreads will be synchronized with respect to + * this cpumask. + * + * The idle + run duration is specified via separate helpers and that allows + * idle injection to be started. + * + * The idle injection kthreads will call play_idle() with the idle duration + * specified as per the above. + * + * After all of them have been woken up, a timer is set to start the next idle + * injection cycle. + * + * The timer interrupt handler will wake up the idle injection kthreads for + * all of the CPUs in the cpumask provided by the user. + * + * Idle injection is stopped synchronously and no leftover idle injection + * kthread activity after its completion is guaranteed. + * + * It is up to the user of this framework to provide a lock for higher-level + * synchronization to prevent race conditions like starting idle injection + * while unregistering from the framework. + */ +#define pr_fmt(fmt) "ii_dev: " fmt + +#include +#include +#include +#include +#include +#include + +#include + +/** + * struct idle_inject_thread - task on/off switch structure + * @tsk: task injecting the idle cycles + * @should_run: whether or not to run the task (for the smpboot kthread API) + */ +struct idle_inject_thread { + struct task_struct *tsk; + int should_run; +}; + +/** + * struct idle_inject_device - idle injection data + * @timer: idle injection period timer + * @idle_duration_ms: duration of CPU idle time to inject + * @run_duration_ms: duration of CPU run time to allow + * @cpumask: mask of CPUs affected by idle injection + */ +struct idle_inject_device { + struct hrtimer timer; + unsigned int idle_duration_ms; + unsigned int run_duration_ms; + unsigned long int cpumask[0]; +}; + +static DEFINE_PER_CPU(struct idle_inject_thread, idle_inject_thread); +static DEFINE_PER_CPU(struct idle_inject_device *, idle_inject_device); + +/** + * idle_inject_wakeup - Wake up idle injection threads + * @ii_dev: target idle injection device + * + * Every idle injection task associated with the given idle injection device + * and running on an online CPU will be woken up. + */ +static void idle_inject_wakeup(struct idle_inject_device *ii_dev) +{ + struct idle_inject_thread *iit; + unsigned int cpu; + + for_each_cpu_and(cpu, to_cpumask(ii_dev->cpumask), cpu_online_mask) { + iit = per_cpu_ptr(&idle_inject_thread, cpu); + iit->should_run = 1; + wake_up_process(iit->tsk); + } +} + +/** + * idle_inject_timer_fn - idle injection timer function + * @timer: idle injection hrtimer + * + * This function is called when the idle injection timer expires. It wakes up + * idle injection tasks associated with the timer and they, in turn, invoke + * play_idle() to inject a specified amount of CPU idle time. + * + * Return: HRTIMER_RESTART. + */ +static enum hrtimer_restart idle_inject_timer_fn(struct hrtimer *timer) +{ + unsigned int duration_ms; + struct idle_inject_device *ii_dev = + container_of(timer, struct idle_inject_device, timer); + + duration_ms = READ_ONCE(ii_dev->run_duration_ms); + duration_ms += READ_ONCE(ii_dev->idle_duration_ms); + + idle_inject_wakeup(ii_dev); + + hrtimer_forward_now(timer, ms_to_ktime(duration_ms)); + + return HRTIMER_RESTART; +} + +/** + * idle_inject_fn - idle injection work function + * @cpu: the CPU owning the task + * + * This function calls play_idle() to inject a specified amount of CPU idle + * time. + */ +static void idle_inject_fn(unsigned int cpu) +{ + struct idle_inject_device *ii_dev; + struct idle_inject_thread *iit; + + ii_dev = per_cpu(idle_inject_device, cpu); + iit = per_cpu_ptr(&idle_inject_thread, cpu); + + /* + * Let the smpboot main loop know that the task should not run again. + */ + iit->should_run = 0; + + play_idle(READ_ONCE(ii_dev->idle_duration_ms)); +} + +/** + * idle_inject_set_duration - idle and run duration update helper + * @run_duration_ms: CPU run time to allow in milliseconds + * @idle_duration_ms: CPU idle time to inject in milliseconds + */ +void idle_inject_set_duration(struct idle_inject_device *ii_dev, + unsigned int run_duration_ms, + unsigned int idle_duration_ms) +{ + if (run_duration_ms && idle_duration_ms) { + WRITE_ONCE(ii_dev->run_duration_ms, run_duration_ms); + WRITE_ONCE(ii_dev->idle_duration_ms, idle_duration_ms); + } +} + +/** + * idle_inject_get_duration - idle and run duration retrieval helper + * @run_duration_ms: memory location to store the current CPU run time + * @idle_duration_ms: memory location to store the current CPU idle time + */ +void idle_inject_get_duration(struct idle_inject_device *ii_dev, + unsigned int *run_duration_ms, + unsigned int *idle_duration_ms) +{ + *run_duration_ms = READ_ONCE(ii_dev->run_duration_ms); + *idle_duration_ms = READ_ONCE(ii_dev->idle_duration_ms); +} + +/** + * idle_inject_start - start idle injections + * @ii_dev: idle injection control device structure + * + * The function starts idle injection by first waking up all of the idle + * injection kthreads associated with @ii_dev to let them inject CPU idle time + * sets up a timer to start the next idle injection period. + * + * Return: -EINVAL if the CPU idle or CPU run time is not set or 0 on success. + */ +int idle_inject_start(struct idle_inject_device *ii_dev) +{ + unsigned int idle_duration_ms = READ_ONCE(ii_dev->idle_duration_ms); + unsigned int run_duration_ms = READ_ONCE(ii_dev->run_duration_ms); + + if (!idle_duration_ms || !run_duration_ms) + return -EINVAL; + + pr_debug("Starting injecting idle cycles on CPUs '%*pbl'\n", + cpumask_pr_args(to_cpumask(ii_dev->cpumask))); + + idle_inject_wakeup(ii_dev); + + hrtimer_start(&ii_dev->timer, + ms_to_ktime(idle_duration_ms + run_duration_ms), + HRTIMER_MODE_REL); + + return 0; +} + +/** + * idle_inject_stop - stops idle injections + * @ii_dev: idle injection control device structure + * + * The function stops idle injection and waits for the threads to finish work. + * If CPU idle time is being injected when this function runs, then it will + * wait until the end of the cycle. + * + * When it returns, there is no more idle injection kthread activity. The + * kthreads are scheduled out and the periodic timer is off. + */ +void idle_inject_stop(struct idle_inject_device *ii_dev) +{ + struct idle_inject_thread *iit; + unsigned int cpu; + + pr_debug("Stopping idle injection on CPUs '%*pbl'\n", + cpumask_pr_args(to_cpumask(ii_dev->cpumask))); + + hrtimer_cancel(&ii_dev->timer); + + /* + * Stopping idle injection requires all of the idle injection kthreads + * associated with the given cpumask to be parked and stay that way, so + * prevent CPUs from going online at this point. Any CPUs going online + * after the loop below will be covered by clearing the should_run flag + * that will cause the smpboot main loop to schedule them out. + */ + cpu_hotplug_disable(); + + /* + * Iterate over all (online + offline) CPUs here in case one of them + * goes offline with the should_run flag set so as to prevent its idle + * injection kthread from running when the CPU goes online again after + * the ii_dev has been freed. + */ + for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) { + iit = per_cpu_ptr(&idle_inject_thread, cpu); + iit->should_run = 0; + + wait_task_inactive(iit->tsk, 0); + } + + cpu_hotplug_enable(); +} + +/** + * idle_inject_setup - prepare the current task for idle injection + * @cpu: not used + * + * Called once, this function is in charge of setting the current task's + * scheduler parameters to make it an RT task. + */ +static void idle_inject_setup(unsigned int cpu) +{ + struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO / 2 }; + + sched_setscheduler(current, SCHED_FIFO, ¶m); +} + +/** + * idle_inject_should_run - function helper for the smpboot API + * @cpu: CPU the kthread is running on + * + * Return: whether or not the thread can run. + */ +static int idle_inject_should_run(unsigned int cpu) +{ + struct idle_inject_thread *iit = + per_cpu_ptr(&idle_inject_thread, cpu); + + return iit->should_run; +} + +/** + * idle_inject_register - initialize idle injection on a set of CPUs + * @cpumask: CPUs to be affected by idle injection + * + * This function creates an idle injection control device structure for the + * given set of CPUs and initializes the timer associated with it. It does not + * start any injection cycles. + * + * Return: NULL if memory allocation fails, idle injection control device + * pointer on success. + */ +struct idle_inject_device *idle_inject_register(struct cpumask *cpumask) +{ + struct idle_inject_device *ii_dev; + int cpu, cpu_rb; + + ii_dev = kzalloc(sizeof(*ii_dev) + cpumask_size(), GFP_KERNEL); + if (!ii_dev) + return NULL; + + cpumask_copy(to_cpumask(ii_dev->cpumask), cpumask); + hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + ii_dev->timer.function = idle_inject_timer_fn; + + for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) { + + if (per_cpu(idle_inject_device, cpu)) { + pr_err("cpu%d is already registered\n", cpu); + goto out_rollback; + } + + per_cpu(idle_inject_device, cpu) = ii_dev; + } + + return ii_dev; + +out_rollback: + for_each_cpu(cpu_rb, to_cpumask(ii_dev->cpumask)) { + if (cpu == cpu_rb) + break; + per_cpu(idle_inject_device, cpu_rb) = NULL; + } + + kfree(ii_dev); + + return NULL; +} + +/** + * idle_inject_unregister - unregister idle injection control device + * @ii_dev: idle injection control device to unregister + * + * The function stops idle injection for the given control device, + * unregisters its kthreads and frees memory allocated when that device was + * created. + */ +void idle_inject_unregister(struct idle_inject_device *ii_dev) +{ + unsigned int cpu; + + idle_inject_stop(ii_dev); + + for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) + per_cpu(idle_inject_device, cpu) = NULL; + + kfree(ii_dev); +} + +static struct smp_hotplug_thread idle_inject_threads = { + .store = &idle_inject_thread.tsk, + .setup = idle_inject_setup, + .thread_fn = idle_inject_fn, + .thread_comm = "idle_inject/%u", + .thread_should_run = idle_inject_should_run, +}; + +static int __init idle_inject_init(void) +{ + return smpboot_register_percpu_thread(&idle_inject_threads); +} +early_initcall(idle_inject_init); diff --git a/include/linux/idle_inject.h b/include/linux/idle_inject.h new file mode 100644 index 000000000000..bdc0293fb6cb --- /dev/null +++ b/include/linux/idle_inject.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2018 Linaro Ltd + * + * Author: Daniel Lezcano + * + */ +#ifndef __IDLE_INJECT_H__ +#define __IDLE_INJECT_H__ + +/* private idle injection device structure */ +struct idle_inject_device; + +struct idle_inject_device *idle_inject_register(struct cpumask *cpumask); + +void idle_inject_unregister(struct idle_inject_device *ii_dev); + +int idle_inject_start(struct idle_inject_device *ii_dev); + +void idle_inject_stop(struct idle_inject_device *ii_dev); + +void idle_inject_set_duration(struct idle_inject_device *ii_dev, + unsigned int run_duration_ms, + unsigned int idle_duration_ms); + +void idle_inject_get_duration(struct idle_inject_device *ii_dev, + unsigned int *run_duration_ms, + unsigned int *idle_duration_ms); +#endif /* __IDLE_INJECT_H__ */ From 12ba2c65f9d3d6996d1e909921151487b009dd30 Mon Sep 17 00:00:00 2001 From: Markus Elfring Date: Tue, 13 Feb 2018 22:10:42 +0100 Subject: [PATCH 02/11] PM / devfreq: exynos-ppmu: Delete an error message for a failed memory allocation in exynos_ppmu_probe() Omit an extra message for a memory allocation failure in this function. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring Acked-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/event/exynos-ppmu.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c index 3cd6a184fe7c..a9c64f0d3284 100644 --- a/drivers/devfreq/event/exynos-ppmu.c +++ b/drivers/devfreq/event/exynos-ppmu.c @@ -627,11 +627,9 @@ static int exynos_ppmu_probe(struct platform_device *pdev) size = sizeof(struct devfreq_event_dev *) * info->num_events; info->edev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL); - if (!info->edev) { - dev_err(&pdev->dev, - "failed to allocate memory devfreq-event devices\n"); + if (!info->edev) return -ENOMEM; - } + edev = info->edev; platform_set_drvdata(pdev, info); From 2d803dc8f7a5f622ac47c3b650834ada3a2659b9 Mon Sep 17 00:00:00 2001 From: Arvind Yadav Date: Fri, 30 Mar 2018 17:14:03 +0530 Subject: [PATCH 03/11] PM / devfreq: use put_device() instead of kfree() Never directly free @dev after calling device_register() or device_unregister(), even if device_register() returned an error. Always use put_device() to give up the reference initialized. Signed-off-by: Arvind Yadav Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/devfreq.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 0b5b3abe054e..e26adf67e218 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -625,7 +625,8 @@ struct devfreq *devfreq_add_device(struct device *dev, err = device_register(&devfreq->dev); if (err) { mutex_unlock(&devfreq->lock); - goto err_dev; + put_device(&devfreq->dev); + goto err_out; } devfreq->trans_table = @@ -672,6 +673,7 @@ err_init: mutex_unlock(&devfreq_list_lock); device_unregister(&devfreq->dev); + devfreq = NULL; err_dev: if (devfreq) kfree(devfreq); From 2d2894c94a61930615cd1411cd8e0bf5b804e3a1 Mon Sep 17 00:00:00 2001 From: Nick Milner Date: Wed, 9 May 2018 14:57:43 +0200 Subject: [PATCH 04/11] dt-bindings: devfreq: rk3399_dmc: improve binding documentation. There are several typos, references to non existent files, grammar and punctuation mistakes in the rk3399_dmc.txt binding. This patch tries to improve the binding documentation and fix these mistakes. Signed-off-by: Nick Milner Signed-off-by: Enric Balletbo i Serra Reviewed-by: Rob Herring - [1/5] Fix some attributes to match with the code s/_disb/_dis/ Signed-off-by: MyungJoo Ham --- .../bindings/devfreq/rk3399_dmc.txt | 189 +++++++++--------- 1 file changed, 96 insertions(+), 93 deletions(-) diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt index fc2bcbe26b1e..1b93b33c7d38 100644 --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt @@ -1,14 +1,14 @@ -* Rockchip rk3399 DMC(Dynamic Memory Controller) device +* Rockchip rk3399 DMC (Dynamic Memory Controller) device Required properties: - compatible: Must be "rockchip,rk3399-dmc". - devfreq-events: Node to get DDR loading, Refer to - Documentation/devicetree/bindings/devfreq/ + Documentation/devicetree/bindings/devfreq/event/ rockchip-dfi.txt -- interrupts: The interrupt number to the CPU. The interrupt - specifier format depends on the interrupt controller. - It should be DCF interrupts, when DDR dvfs finish, - it will happen. +- interrupts: The CPU interrupt number. The interrupt specifier + format depends on the interrupt controller. + It should be a DCF interrupt. When DDR DVFS finishes + a DCF interrupt is triggered. - clocks: Phandles for clock specified in "clock-names" property - clock-names : The name of clock used by the DFI, must be "pclk_ddr_mon"; @@ -17,139 +17,142 @@ Required properties: - center-supply: DMC supply node. - status: Marks the node enabled/disabled. -Following properties are ddr timing: +Following properties relate to DDR timing: - rockchip,dram_speed_bin : Value reference include/dt-bindings/clock/ddr.h, - it select ddr3 cl-trp-trcd type, default value - "DDR3_DEFAULT".it must selected according to - "Speed Bin" in ddr3 datasheet, DO NOT use - smaller "Speed Bin" than ddr3 exactly is. + it selects the DDR3 cl-trp-trcd type. It must be + set according to "Speed Bin" in DDR3 datasheet, + DO NOT use a smaller "Speed Bin" than specified + for the DDR3 being used. -- rockchip,pd_idle : Config the PD_IDLE value, defined the power-down - idle period, memories are places into power-down - mode if bus is idle for PD_IDLE DFI clocks. +- rockchip,pd_idle : Configure the PD_IDLE value. Defines the + power-down idle period in which memories are + placed into power-down mode if bus is idle + for PD_IDLE DFI clock cycles. -- rockchip,sr_idle : Configure the SR_IDLE value, defined the - selfrefresh idle period, memories are places - into self-refresh mode if bus is idle for - SR_IDLE*1024 DFI clocks (DFI clocks freq is - half of dram's clocks), defaule value is "0". +- rockchip,sr_idle : Configure the SR_IDLE value. Defines the + self-refresh idle period in which memories are + placed into self-refresh mode if bus is idle + for SR_IDLE * 1024 DFI clock cycles (DFI + clocks freq is half of DRAM clock), default + value is "0". -- rockchip,sr_mc_gate_idle : Defined the self-refresh with memory and - controller clock gating idle period, memories - are places into self-refresh mode and memory - controller clock arg gating if bus is idle for - sr_mc_gate_idle*1024 DFI clocks. +- rockchip,sr_mc_gate_idle : Defines the memory self-refresh and controller + clock gating idle period. Memories are placed + into self-refresh mode and memory controller + clock arg gating started if bus is idle for + sr_mc_gate_idle*1024 DFI clock cycles. -- rockchip,srpd_lite_idle : Defined the self-refresh power down idle - period, memories are places into self-refresh - power down mode if bus is idle for - srpd_lite_idle*1024 DFI clocks. This parameter - is for LPDDR4 only. +- rockchip,srpd_lite_idle : Defines the self-refresh power down idle + period in which memories are placed into + self-refresh power down mode if bus is idle + for srpd_lite_idle * 1024 DFI clock cycles. + This parameter is for LPDDR4 only. -- rockchip,standby_idle : Defined the standby idle period, memories are - places into self-refresh than controller, pi, - phy and dram clock will gating if bus is idle - for standby_idle * DFI clocks. +- rockchip,standby_idle : Defines the standby idle period in which + memories are placed into self-refresh mode. + The controller, pi, PHY and DRAM clock will + be gated if bus is idle for standby_idle * DFI + clock cycles. -- rockchip,dram_dll_disb_freq : It's defined the DDR3 dll bypass frequency in - MHz, when ddr freq less than DRAM_DLL_DISB_FREQ, - ddr3 dll will bypssed note: if dll was bypassed, - the odt also stop working. +- rockchip,dram_dll_dis_freq : Defines the DDR3 DLL bypass frequency in MHz. + When DDR frequency is less than DRAM_DLL_DISB_FREQ, + DDR3 DLL will be bypassed. Note: if DLL was bypassed, + the odt will also stop working. -- rockchip,phy_dll_disb_freq : Defined the PHY dll bypass frequency in - MHz (Mega Hz), when ddr freq less than - DRAM_DLL_DISB_FREQ, phy dll will bypssed. - note: phy dll and phy odt are independent. +- rockchip,phy_dll_dis_freq : Defines the PHY dll bypass frequency in + MHz (Mega Hz). When DDR frequency is less than + DRAM_DLL_DISB_FREQ, PHY DLL will be bypassed. + Note: PHY DLL and PHY ODT are independent. -- rockchip,ddr3_odt_disb_freq : When dram type is DDR3, this parameter defined - the odt disable frequency in MHz (Mega Hz), - when ddr frequency less then ddr3_odt_disb_freq, - the odt on dram side and controller side are +- rockchip,ddr3_odt_dis_freq : When the DRAM type is DDR3, this parameter defines + the ODT disable frequency in MHz (Mega Hz). + when the DDR frequency is less then ddr3_odt_dis_freq, + the ODT on the DRAM side and controller side are both disabled. -- rockchip,ddr3_drv : When dram type is DDR3, this parameter define - the dram side driver stength in ohm, default +- rockchip,ddr3_drv : When the DRAM type is DDR3, this parameter defines + the DRAM side driver strength in ohms. Default value is DDR3_DS_40ohm. -- rockchip,ddr3_odt : When dram type is DDR3, this parameter define - the dram side ODT stength in ohm, default value +- rockchip,ddr3_odt : When the DRAM type is DDR3, this parameter defines + the DRAM side ODT strength in ohms. Default value is DDR3_ODT_120ohm. -- rockchip,phy_ddr3_ca_drv : When dram type is DDR3, this parameter define - the phy side CA line(incluing command line, +- rockchip,phy_ddr3_ca_drv : When the DRAM type is DDR3, this parameter defines + the phy side CA line (incluing command line, address line and clock line) driver strength. Default value is PHY_DRV_ODT_40. -- rockchip,phy_ddr3_dq_drv : When dram type is DDR3, this parameter define - the phy side DQ line(incluing DQS/DQ/DM line) - driver strength. default value is PHY_DRV_ODT_40. +- rockchip,phy_ddr3_dq_drv : When the DRAM type is DDR3, this parameter defines + the PHY side DQ line (including DQS/DQ/DM line) + driver strength. Default value is PHY_DRV_ODT_40. -- rockchip,phy_ddr3_odt : When dram type is DDR3, this parameter define the - phy side odt strength, default value is +- rockchip,phy_ddr3_odt : When the DRAM type is DDR3, this parameter defines + the PHY side ODT strength. Default value is PHY_DRV_ODT_240. -- rockchip,lpddr3_odt_disb_freq : When dram type is LPDDR3, this parameter defined - then odt disable frequency in MHz (Mega Hz), - when ddr frequency less then ddr3_odt_disb_freq, - the odt on dram side and controller side are +- rockchip,lpddr3_odt_dis_freq : When the DRAM type is LPDDR3, this parameter defines + then ODT disable frequency in MHz (Mega Hz). + When DDR frequency is less then ddr3_odt_dis_freq, + the ODT on the DRAM side and controller side are both disabled. -- rockchip,lpddr3_drv : When dram type is LPDDR3, this parameter define - the dram side driver stength in ohm, default +- rockchip,lpddr3_drv : When the DRAM type is LPDDR3, this parameter defines + the DRAM side driver strength in ohms. Default value is LP3_DS_34ohm. -- rockchip,lpddr3_odt : When dram type is LPDDR3, this parameter define - the dram side ODT stength in ohm, default value +- rockchip,lpddr3_odt : When the DRAM type is LPDDR3, this parameter defines + the DRAM side ODT strength in ohms. Default value is LP3_ODT_240ohm. -- rockchip,phy_lpddr3_ca_drv : When dram type is LPDDR3, this parameter define - the phy side CA line(incluing command line, +- rockchip,phy_lpddr3_ca_drv : When the DRAM type is LPDDR3, this parameter defines + the PHY side CA line (including command line, address line and clock line) driver strength. - default value is PHY_DRV_ODT_40. + Default value is PHY_DRV_ODT_40. -- rockchip,phy_lpddr3_dq_drv : When dram type is LPDDR3, this parameter define - the phy side DQ line(incluing DQS/DQ/DM line) - driver strength. default value is +- rockchip,phy_lpddr3_dq_drv : When the DRAM type is LPDDR3, this parameter defines + the PHY side DQ line (including DQS/DQ/DM line) + driver strength. Default value is PHY_DRV_ODT_40. - rockchip,phy_lpddr3_odt : When dram type is LPDDR3, this parameter define the phy side odt strength, default value is PHY_DRV_ODT_240. -- rockchip,lpddr4_odt_disb_freq : When dram type is LPDDR4, this parameter - defined the odt disable frequency in - MHz (Mega Hz), when ddr frequency less then - ddr3_odt_disb_freq, the odt on dram side and +- rockchip,lpddr4_odt_dis_freq : When the DRAM type is LPDDR4, this parameter + defines the ODT disable frequency in + MHz (Mega Hz). When the DDR frequency is less then + ddr3_odt_dis_freq, the ODT on the DRAM side and controller side are both disabled. -- rockchip,lpddr4_drv : When dram type is LPDDR4, this parameter define - the dram side driver stength in ohm, default +- rockchip,lpddr4_drv : When the DRAM type is LPDDR4, this parameter defines + the DRAM side driver strength in ohms. Default value is LP4_PDDS_60ohm. -- rockchip,lpddr4_dq_odt : When dram type is LPDDR4, this parameter define - the dram side ODT on dqs/dq line stength in ohm, - default value is LP4_DQ_ODT_40ohm. +- rockchip,lpddr4_dq_odt : When the DRAM type is LPDDR4, this parameter defines + the DRAM side ODT on DQS/DQ line strength in ohms. + Default value is LP4_DQ_ODT_40ohm. -- rockchip,lpddr4_ca_odt : When dram type is LPDDR4, this parameter define - the dram side ODT on ca line stength in ohm, - default value is LP4_CA_ODT_40ohm. +- rockchip,lpddr4_ca_odt : When the DRAM type is LPDDR4, this parameter defines + the DRAM side ODT on CA line strength in ohms. + Default value is LP4_CA_ODT_40ohm. -- rockchip,phy_lpddr4_ca_drv : When dram type is LPDDR4, this parameter define - the phy side CA line(incluing command address - line) driver strength. default value is +- rockchip,phy_lpddr4_ca_drv : When the DRAM type is LPDDR4, this parameter defines + the PHY side CA line (including command address + line) driver strength. Default value is PHY_DRV_ODT_40. -- rockchip,phy_lpddr4_ck_cs_drv : When dram type is LPDDR4, this parameter define - the phy side clock line and cs line driver - strength. default value is PHY_DRV_ODT_80. +- rockchip,phy_lpddr4_ck_cs_drv : When the DRAM type is LPDDR4, this parameter defines + the PHY side clock line and CS line driver + strength. Default value is PHY_DRV_ODT_80. -- rockchip,phy_lpddr4_dq_drv : When dram type is LPDDR4, this parameter define - the phy side DQ line(incluing DQS/DQ/DM line) - driver strength. default value is PHY_DRV_ODT_80. +- rockchip,phy_lpddr4_dq_drv : When the DRAM type is LPDDR4, this parameter defines + the PHY side DQ line (including DQS/DQ/DM line) + driver strength. Default value is PHY_DRV_ODT_80. -- rockchip,phy_lpddr4_odt : When dram type is LPDDR4, this parameter define - the phy side odt strength, default value is +- rockchip,phy_lpddr4_odt : When the DRAM type is LPDDR4, this parameter defines + the PHY side ODT strength. Default value is PHY_DRV_ODT_60. Example: From 49502b23e0e45b57b776490cfdfd95ea7fecd7c7 Mon Sep 17 00:00:00 2001 From: Enric Balletbo i Serra Date: Wed, 9 May 2018 14:57:44 +0200 Subject: [PATCH 05/11] dt-bindings: clock: add rk3399 DDR3 standard speed bins. DDR3 SDRAM Standard (JESD79-3F) defines some standard speed bins for DDR3 memories. The rk3399_dmc driver allows you to pass these values via the device tree. For that purpose the devfreq/rk3399_dmc.txt binding refers to a ddr.h file which does not exist. This patch adds the missing defines in a include file called rk3399-ddr.h with the definition of standard speed bins according to the ARM Trusted Firmware (ATF). Fixes: c1ceb8f7c167 (Documentation: bindings: add dt documentation for rk3399 dmc) Signed-off-by: Enric Balletbo i Serra Reviewed-by: Rob Herring Signed-off-by: MyungJoo Ham --- .../bindings/devfreq/rk3399_dmc.txt | 2 +- include/dt-bindings/clock/rk3399-ddr.h | 56 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 include/dt-bindings/clock/rk3399-ddr.h diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt index 1b93b33c7d38..e0030a139383 100644 --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt @@ -19,7 +19,7 @@ Required properties: Following properties relate to DDR timing: -- rockchip,dram_speed_bin : Value reference include/dt-bindings/clock/ddr.h, +- rockchip,dram_speed_bin : Value reference include/dt-bindings/clock/rk3399-ddr.h, it selects the DDR3 cl-trp-trcd type. It must be set according to "Speed Bin" in DDR3 datasheet, DO NOT use a smaller "Speed Bin" than specified diff --git a/include/dt-bindings/clock/rk3399-ddr.h b/include/dt-bindings/clock/rk3399-ddr.h new file mode 100644 index 000000000000..ed2280844963 --- /dev/null +++ b/include/dt-bindings/clock/rk3399-ddr.h @@ -0,0 +1,56 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ + +#ifndef DT_BINDINGS_DDR_H +#define DT_BINDINGS_DDR_H + +/* + * DDR3 SDRAM Standard Speed Bins include tCK, tRCD, tRP, tRAS and tRC for + * each corresponding bin. + */ + +/* DDR3-800 (5-5-5) */ +#define DDR3_800D 0 +/* DDR3-800 (6-6-6) */ +#define DDR3_800E 1 +/* DDR3-1066 (6-6-6) */ +#define DDR3_1066E 2 +/* DDR3-1066 (7-7-7) */ +#define DDR3_1066F 3 +/* DDR3-1066 (8-8-8) */ +#define DDR3_1066G 4 +/* DDR3-1333 (7-7-7) */ +#define DDR3_1333F 5 +/* DDR3-1333 (8-8-8) */ +#define DDR3_1333G 6 +/* DDR3-1333 (9-9-9) */ +#define DDR3_1333H 7 +/* DDR3-1333 (10-10-10) */ +#define DDR3_1333J 8 +/* DDR3-1600 (8-8-8) */ +#define DDR3_1600G 9 +/* DDR3-1600 (9-9-9) */ +#define DDR3_1600H 10 +/* DDR3-1600 (10-10-10) */ +#define DDR3_1600J 11 +/* DDR3-1600 (11-11-11) */ +#define DDR3_1600K 12 +/* DDR3-1600 (10-10-10) */ +#define DDR3_1866J 13 +/* DDR3-1866 (11-11-11) */ +#define DDR3_1866K 14 +/* DDR3-1866 (12-12-12) */ +#define DDR3_1866L 15 +/* DDR3-1866 (13-13-13) */ +#define DDR3_1866M 16 +/* DDR3-2133 (11-11-11) */ +#define DDR3_2133K 17 +/* DDR3-2133 (12-12-12) */ +#define DDR3_2133L 18 +/* DDR3-2133 (13-13-13) */ +#define DDR3_2133M 19 +/* DDR3-2133 (14-14-14) */ +#define DDR3_2133N 20 +/* DDR3 ATF default */ +#define DDR3_DEFAULT 21 + +#endif From 90dd72e1290dd86c4b6e5c421fcd13e60e625782 Mon Sep 17 00:00:00 2001 From: Enric Balletbo i Serra Date: Wed, 9 May 2018 14:57:45 +0200 Subject: [PATCH 06/11] PM / devfreq: rk3399_dmc: remove wait for dcf irq event. We have already wait dcf done in ATF, so don't need wait dcf irq in kernel, besides, clear dcf irq in kernel will import competiton between kernel and ATF, only handle dcf irq in ATF is a better way. Signed-off-by: Lin Huang Signed-off-by: Enric Balletbo i Serra Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/rk3399_dmc.c | 53 +----------------------------------- 1 file changed, 1 insertion(+), 52 deletions(-) diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 5dfbfa3cc878..44a379657cd5 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -68,15 +68,6 @@ struct rk3399_dmcfreq { struct devfreq_event_dev *edev; struct mutex lock; struct dram_timing timing; - - /* - * DDR Converser of Frequency (DCF) is used to implement DDR frequency - * conversion without the participation of CPU, we will implement and - * control it in arm trust firmware. - */ - wait_queue_head_t wait_dcf_queue; - int irq; - int wait_dcf_flag; struct regulator *vdd_center; unsigned long rate, target_rate; unsigned long volt, target_volt; @@ -117,7 +108,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, goto out; } } - dmcfreq->wait_dcf_flag = 1; err = clk_set_rate(dmcfreq->dmc_clk, target_rate); if (err) { @@ -128,14 +118,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, goto out; } - /* - * Wait until bcf irq happen, it means freq scaling finish in - * arm trust firmware, use 100ms as timeout time. - */ - if (!wait_event_timeout(dmcfreq->wait_dcf_queue, - !dmcfreq->wait_dcf_flag, HZ / 10)) - dev_warn(dev, "Timeout waiting for dcf interrupt\n"); - /* * Check the dpll rate, * There only two result we will get, @@ -241,22 +223,6 @@ static __maybe_unused int rk3399_dmcfreq_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(rk3399_dmcfreq_pm, rk3399_dmcfreq_suspend, rk3399_dmcfreq_resume); -static irqreturn_t rk3399_dmc_irq(int irq, void *dev_id) -{ - struct rk3399_dmcfreq *dmcfreq = dev_id; - struct arm_smccc_res res; - - dmcfreq->wait_dcf_flag = 0; - wake_up(&dmcfreq->wait_dcf_queue); - - /* Clear the DCF interrupt */ - arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, 0, 0, - ROCKCHIP_SIP_CONFIG_DRAM_CLR_IRQ, - 0, 0, 0, 0, &res); - - return IRQ_HANDLED; -} - static int of_get_ddr_timings(struct dram_timing *timing, struct device_node *np) { @@ -330,16 +296,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct device_node *np = pdev->dev.of_node; struct rk3399_dmcfreq *data; - int ret, irq, index, size; + int ret, index, size; uint32_t *timing; struct dev_pm_opp *opp; - irq = platform_get_irq(pdev, 0); - if (irq < 0) { - dev_err(&pdev->dev, - "Cannot get the dmc interrupt resource: %d\n", irq); - return irq; - } data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL); if (!data) return -ENOMEM; @@ -358,17 +318,6 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) return PTR_ERR(data->dmc_clk); }; - data->irq = irq; - ret = devm_request_irq(dev, irq, rk3399_dmc_irq, 0, - dev_name(dev), data); - if (ret) { - dev_err(dev, "Failed to request dmc irq: %d\n", ret); - return ret; - } - - init_waitqueue_head(&data->wait_dcf_queue); - data->wait_dcf_flag = 0; - data->edev = devfreq_event_get_edev_by_phandle(dev, 0); if (IS_ERR(data->edev)) return -EPROBE_DEFER; From 77f2c2305decda26885f8b2307577267b78c468f Mon Sep 17 00:00:00 2001 From: Enric Balletbo i Serra Date: Wed, 9 May 2018 14:57:46 +0200 Subject: [PATCH 07/11] dt-bindings: devfreq: rk3399_dmc: move interrupts to be optional. In ATF we already wait for DDR DVFS finish, so move the interrupts properties to be optional. Signed-off-by: Enric Balletbo i Serra Reviewed-by: Chanwoo Choi Reviewed-by: Rob Herring Signed-off-by: MyungJoo Ham --- .../devicetree/bindings/devfreq/rk3399_dmc.txt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt index e0030a139383..0ec68141f85a 100644 --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt @@ -5,10 +5,6 @@ Required properties: - devfreq-events: Node to get DDR loading, Refer to Documentation/devicetree/bindings/devfreq/event/ rockchip-dfi.txt -- interrupts: The CPU interrupt number. The interrupt specifier - format depends on the interrupt controller. - It should be a DCF interrupt. When DDR DVFS finishes - a DCF interrupt is triggered. - clocks: Phandles for clock specified in "clock-names" property - clock-names : The name of clock used by the DFI, must be "pclk_ddr_mon"; @@ -17,6 +13,12 @@ Required properties: - center-supply: DMC supply node. - status: Marks the node enabled/disabled. +Optional properties: +- interrupts: The CPU interrupt number. The interrupt specifier + format depends on the interrupt controller. + It should be a DCF interrupt. When DDR DVFS finishes + a DCF interrupt is triggered. + Following properties relate to DDR timing: - rockchip,dram_speed_bin : Value reference include/dt-bindings/clock/rk3399-ddr.h, From 49edc52312c34c981722833b0d9344c2aa83892d Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Wed, 9 May 2018 14:57:47 +0200 Subject: [PATCH 08/11] PM / devfreq: rk3399_dmc: do not print error when get supply and clk defer. We just return -EPROBE_DEFER error code to caller and do not print error message when try to get center logic regulator and DMC clock defer. Signed-off-by: Lin Huang Signed-off-by: Enric Balletbo i Serra Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/rk3399_dmc.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 44a379657cd5..5bfca028eaaf 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -308,12 +308,18 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) data->vdd_center = devm_regulator_get(dev, "center"); if (IS_ERR(data->vdd_center)) { + if (PTR_ERR(data->vdd_center) == -EPROBE_DEFER) + return -EPROBE_DEFER; + dev_err(dev, "Cannot get the regulator \"center\"\n"); return PTR_ERR(data->vdd_center); } data->dmc_clk = devm_clk_get(dev, "dmc_clk"); if (IS_ERR(data->dmc_clk)) { + if (PTR_ERR(data->dmc_clk) == -EPROBE_DEFER) + return -EPROBE_DEFER; + dev_err(dev, "Cannot get the clk dmc_clk\n"); return PTR_ERR(data->dmc_clk); }; From dfa7d764caf00b12da276ea473d7f1fd7fd40200 Mon Sep 17 00:00:00 2001 From: Enric Balletbo i Serra Date: Wed, 9 May 2018 14:57:48 +0200 Subject: [PATCH 09/11] PM / devfreq: rk3399_dmc: fix spelling mistakes. Fix some spelling mistakes in error and debug messages. Signed-off-by: Enric Balletbo i Serra Signed-off-by: MyungJoo Ham --- drivers/devfreq/rk3399_dmc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 5bfca028eaaf..d5c03e5abe13 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -103,7 +103,7 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, err = regulator_set_voltage(dmcfreq->vdd_center, target_volt, target_volt); if (err) { - dev_err(dev, "Cannot to set voltage %lu uV\n", + dev_err(dev, "Cannot set voltage %lu uV\n", target_volt); goto out; } @@ -111,8 +111,8 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, err = clk_set_rate(dmcfreq->dmc_clk, target_rate); if (err) { - dev_err(dev, "Cannot to set frequency %lu (%d)\n", - target_rate, err); + dev_err(dev, "Cannot set frequency %lu (%d)\n", target_rate, + err); regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt, dmcfreq->volt); goto out; @@ -128,8 +128,8 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, /* If get the incorrect rate, set voltage to old value. */ if (dmcfreq->rate != target_rate) { - dev_err(dev, "Get wrong ddr frequency, Request frequency %lu,\ - Current frequency %lu\n", target_rate, dmcfreq->rate); + dev_err(dev, "Got wrong frequency, Request %lu, Current %lu\n", + target_rate, dmcfreq->rate); regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt, dmcfreq->volt); goto out; @@ -137,7 +137,7 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, err = regulator_set_voltage(dmcfreq->vdd_center, target_volt, target_volt); if (err) - dev_err(dev, "Cannot to set vol %lu uV\n", target_volt); + dev_err(dev, "Cannot set voltage %lu uV\n", target_volt); dmcfreq->rate = target_rate; dmcfreq->volt = target_volt; From 2c2cb1e6b05b90d55b4b943646faa3cfbdf78f6e Mon Sep 17 00:00:00 2001 From: Matthias Kaehlcke Date: Fri, 25 May 2018 13:30:33 -0700 Subject: [PATCH 10/11] PM / devfreq: Init user limits from OPP limits, not viceversa Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding the devfreq device") introduced the initialization of the user limits min/max_freq from the lowest/highest available OPPs. Later commit f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency") added scaling_min/max_freq, which actually represent the frequencies of the lowest/highest available OPP. scaling_min/ max_freq are initialized with the values from min/max_freq, which is totally correct in the context, but a bit awkward to read. Swap the initialization and assign scaling_min/max_freq with the OPP freqs and then the user limts min/max_freq with scaling_min/ max_freq. Needless to say that this change is a NOP, intended to improve readability. Signed-off-by: Matthias Kaehlcke Reviewed-by: Chanwoo Choi Reviewed-by: Brian Norris Signed-off-by: MyungJoo Ham --- drivers/devfreq/devfreq.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index e26adf67e218..4c49bb1330b5 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -604,21 +604,21 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_lock(&devfreq->lock); } - devfreq->min_freq = find_available_min_freq(devfreq); - if (!devfreq->min_freq) { + devfreq->scaling_min_freq = find_available_min_freq(devfreq); + if (!devfreq->scaling_min_freq) { mutex_unlock(&devfreq->lock); err = -EINVAL; goto err_dev; } - devfreq->scaling_min_freq = devfreq->min_freq; + devfreq->min_freq = devfreq->scaling_min_freq; - devfreq->max_freq = find_available_max_freq(devfreq); - if (!devfreq->max_freq) { + devfreq->scaling_max_freq = find_available_max_freq(devfreq); + if (!devfreq->scaling_max_freq) { mutex_unlock(&devfreq->lock); err = -EINVAL; goto err_dev; } - devfreq->scaling_max_freq = devfreq->max_freq; + devfreq->max_freq = devfreq->scaling_max_freq; dev_set_name(&devfreq->dev, "devfreq%d", atomic_inc_return(&devfreq_no)); From d6e98f3e6d825380b566dc59359917a116090154 Mon Sep 17 00:00:00 2001 From: Enric Balletbo i Serra Date: Fri, 15 Jun 2018 17:12:17 +0200 Subject: [PATCH 11/11] PM / devfreq: rk3399_dmc: Fix duplicated opp table on reload. The opp table is not removed when the driver is unloaded neither when there is an error within probe, so if the driver is reloaded the opp core shows the following warning: rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq: 200000000, volt: 900000, enabled: 1. New: freq: 200000000, volt: 900000, enabled: 1 rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq: 400000000, volt: 900000, enabled: 1. New: freq: 400000000, volt: 900000, enabled: 1 rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq: 666000000, volt: 900000, enabled: 1. New: freq: 666000000, volt: 900000, enabled: 1 rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq: 800000000, volt: 900000, enabled: 1. New: freq: 800000000, volt: 900000, enabled: 1 rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq: 928000000, volt: 900000, enabled: 1. New: freq: 928000000, volt: 900000, enabled: 1 This patch fixes the error path in the probe function and adds a .remove function to properly cleanup the opp table on unloading. Fixes: 5a893e31a636c (PM / devfreq: rockchip: add devfreq driver for rk3399 dmc) Signed-off-by: Enric Balletbo i Serra Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/rk3399_dmc.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index d5c03e5abe13..e795ad2b3f6b 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -375,8 +375,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) data->rate = clk_get_rate(data->dmc_clk); opp = devfreq_recommended_opp(dev, &data->rate, 0); - if (IS_ERR(opp)) - return PTR_ERR(opp); + if (IS_ERR(opp)) { + ret = PTR_ERR(opp); + goto err_free_opp; + } data->rate = dev_pm_opp_get_freq(opp); data->volt = dev_pm_opp_get_voltage(opp); @@ -388,13 +390,33 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) &rk3399_devfreq_dmc_profile, DEVFREQ_GOV_SIMPLE_ONDEMAND, &data->ondemand_data); - if (IS_ERR(data->devfreq)) - return PTR_ERR(data->devfreq); + if (IS_ERR(data->devfreq)) { + ret = PTR_ERR(data->devfreq); + goto err_free_opp; + } + devm_devfreq_register_opp_notifier(dev, data->devfreq); data->dev = dev; platform_set_drvdata(pdev, data); + return 0; + +err_free_opp: + dev_pm_opp_of_remove_table(&pdev->dev); + return ret; +} + +static int rk3399_dmcfreq_remove(struct platform_device *pdev) +{ + struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(&pdev->dev); + + /* + * Before remove the opp table we need to unregister the opp notifier. + */ + devm_devfreq_unregister_opp_notifier(dmcfreq->dev, dmcfreq->devfreq); + dev_pm_opp_of_remove_table(dmcfreq->dev); + return 0; } @@ -406,6 +428,7 @@ MODULE_DEVICE_TABLE(of, rk3399dmc_devfreq_of_match); static struct platform_driver rk3399_dmcfreq_driver = { .probe = rk3399_dmcfreq_probe, + .remove = rk3399_dmcfreq_remove, .driver = { .name = "rk3399-dmc-freq", .pm = &rk3399_dmcfreq_pm,