Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2014-10-21 19:59:04
Also in:
linux-arm-kernel, lkml
Hi Jinkun, On Tue, Oct 21, 2014 at 02:13:26AM -0700, jinkun.hong wrote:
quoted hunk ↗ jump to hunk
From: "jinkun.hong" <redacted> Add power domain drivers based on generic power domain for Rockchip platform, and support RK3288. Signed-off-by: Jack Dai <redacted> Signed-off-by: jinkun.hong <redacted> --- Changes in v5: - delete idle_lock - add timeout in rockchip_pmu_set_idle_request() Changes in v4: - use list storage dev Changes in v3: - change use pm_clk_resume() and pm_clk_suspend() Changes in v2: - remove the "pd->pd.of_node = np" arch/arm/mach-rockchip/Kconfig | 1 + arch/arm/mach-rockchip/Makefile | 1 + arch/arm/mach-rockchip/pm_domains.c | 368 +++++++++++++++++++++++++++++++++++ 3 files changed, 370 insertions(+) create mode 100644 arch/arm/mach-rockchip/pm_domains.cdiff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig index d168669..4920a88 100644 --- a/arch/arm/mach-rockchip/Kconfig +++ b/arch/arm/mach-rockchip/Kconfig@@ -12,6 +12,7 @@ config ARCH_ROCKCHIP select DW_APB_TIMER_OF select ARM_GLOBAL_TIMER select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK + select PM_GENERIC_DOMAINS if PM help Support for Rockchip's Cortex-A9 Single-to-Quad-Core-SoCs containing the RK2928, RK30xx and RK31xx series.diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index b29d8ea..805268d 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile@@ -2,3 +2,4 @@ CFLAGS_platsmp.o := -march=armv7-a obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o obj-$(CONFIG_SMP) += headsmp.o platsmp.o +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.odiff --git a/arch/arm/mach-rockchip/pm_domains.c b/arch/arm/mach-rockchip/pm_domains.c new file mode 100644 index 0000000..8f13445 --- /dev/null +++ b/arch/arm/mach-rockchip/pm_domains.c@@ -0,0 +1,368 @@ +/* + * Rockchip Generic power domain support. + * + * Copyright (c) 2014 ROCKCHIP, Co. Ltd. + * Author: Hong Jinkun <jinkun.hong@rock-chips.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#include <linux/module.h> +#include <linux/io.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/pm_domain.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/sched.h> +#include <linux/clk.h> +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> +#include <linux/spinlock.h> +#include <linux/pm_clock.h> +#include <linux/delay.h> + +#define PWR_OFFSET 0x08 +#define STATUS_OFFSET 0x0c +#define REQ_OFFSET 0x10 +#define IDLE_OFFSET 0x14 +#define ACK_OFFSET 0x14 + +struct rockchip_dev_entry { + struct list_head node; + struct device *dev; +}; + +struct rockchip_domain { + struct generic_pm_domain base; + struct device *dev; + struct regmap *regmap_pmu; + struct list_head dev_list; + /* spin lock for read/write pmu reg */ + spinlock_t pmu_lock; + /* spin lock for dev_list */ + spinlock_t dev_lock; + u32 pwr_shift; + u32 status_shift; + u32 req_shift; + u32 idle_shift; + u32 ack_shift; +}; + +#define to_rockchip_pd(_gpd) container_of(_gpd, struct rockchip_domain, base) + +static int rockchip_pmu_set_idle_request(struct rockchip_domain *pd, + bool idle) +{ + u32 idle_mask = BIT(pd->idle_shift); + u32 idle_target = idle << (pd->idle_shift); + u32 ack_mask = BIT(pd->ack_shift); + u32 ack_target = idle << (pd->ack_shift); + unsigned int mask = BIT(pd->req_shift); + unsigned int val; + unsigned long flags; + int timeout = 0; + + val = (idle) ? mask : 0; + regmap_update_bits(pd->regmap_pmu, REQ_OFFSET, mask, val); + dsb(); + + do { + regmap_read(pd->regmap_pmu, ACK_OFFSET, &val); + udelay(1); + if (timeout > 10) { + pr_err("%s wait pmu ack timeout!\n", __func__); + break; + } + timeout += 1; + } while ((val & ack_mask) != ack_target); + + timeout = 0; + do { + regmap_read(pd->regmap_pmu, IDLE_OFFSET, &val); + udelay(1); + if (timeout > 10) { + pr_err("%s wait pmu idle timeout!\n", __func__); + break; + } + timeout += 1; + } while ((val & idle_mask) != idle_target); + return 0; +} + +static bool rockchip_pmu_power_domain_is_on(struct rockchip_domain *pd) +{ + unsigned int val; + + regmap_read(pd->regmap_pmu, STATUS_OFFSET, &val); + + /* 1'b0: power on, 1'b1: power off */ + return !(val & BIT(pd->status_shift)); +} + +static void rockchip_do_pmu_set_power_domain( + struct rockchip_domain *pd, bool on) +{ + unsigned int mask = BIT(pd->pwr_shift); + unsigned int val; + + val = (on) ? 0 : mask; + regmap_update_bits(pd->regmap_pmu, PWR_OFFSET, mask, val); + dsb(); + + do { + regmap_read(pd->regmap_pmu, STATUS_OFFSET, &val); + } while ((val & BIT(pd->status_shift)) == on); +} + +static int rockchip_pmu_set_power_domain(struct rockchip_domain *pd, + bool on) +{ + unsigned long flags; + + spin_lock_irqsave(&pd->pmu_lock, flags);
You only call rockchip_pmu_set_power_domain() with pd->dev_lock held, why do you need this spinlock?
+ if (rockchip_pmu_power_domain_is_on(pd) == on)
+ goto out;
+
+ if (!on) {
+ /* FIXME: add code to save AXI_QOS */
+ /* if power down, idle request to NIU first */
+ rockchip_pmu_set_idle_request(pd, true);
+ }
+
+ rockchip_do_pmu_set_power_domain(pd, on);
+
+ if (on) {
+ /* if power up, idle request release to NIU */
+ rockchip_pmu_set_idle_request(pd, false);
+ /* FIXME: add code to restore AXI_QOS */
+ }
+out:
+ spin_unlock_irqrestore(&pd->pmu_lock, flags);
+ return 0;
+}
+
+static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
+{
+ int i = 0;
+ int ret = 0;
+ struct rockchip_dev_entry *de;
+
+ spin_lock_irq(&pd->dev_lock);Do you really need to run here with interrupts off? Maybe a mutex would be better here?
+
+ list_for_each_entry(de, &pd->dev_list, node) {
+ i += 1;
+ pm_clk_resume(pd->dev);Do you really need to call pm_clk_resume() number of times that there are devices in power domain? Did you want it to be pm_clk_resume(de->dev); by any chance?
+ }
+
+ /* no clk, set power domain will fail */
+ if (i == 0) {
+ pr_err("%s: failed to on/off power domain!", __func__);
+ spin_unlock_irq(&pd->dev_lock);
+ return ret;
+ }
Instead of counting I'd do
if (list_empty(&pd->dev_list)) {
pr_waen("%s: no devices in power domain\n", __func__);
goto out;
}
in the beginning of the function.
+
+ ret = rockchip_pmu_set_power_domain(pd, power_on);
+
+ list_for_each_entry(de, &pd->dev_list, node) {
+ pm_clk_suspend(pd->dev);Same here?
+ }
+
+ spin_unlock_irq(&pd->dev_lock);
+
+ return ret;
+}
+
+static int rockchip_pd_power_on(struct generic_pm_domain *domain)
+{
+ struct rockchip_domain *pd = to_rockchip_pd(domain);
+
+ return rockchip_pd_power(pd, true);
+}
+
+static int rockchip_pd_power_off(struct generic_pm_domain *domain)
+{
+ struct rockchip_domain *pd = to_rockchip_pd(domain);
+
+ return rockchip_pd_power(pd, false);
+}
+
+void rockchip_pm_domain_attach_dev(struct device *dev)
+{
+ int ret;
+ int i = 0;
+ struct clk *clk;
+ struct rockchip_domain *pd;
+ struct rockchip_dev_entry *de;
+
+ pd = (struct rockchip_domain *)dev->pm_domain;
+ ret = pm_clk_create(dev);
+ if (ret) {
+ dev_err(dev, "pm_clk_create failed %d\n", ret);
+ return;
+ };Stray semicolon.
+
+ while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
+ ret = pm_clk_add_clk(dev, clk);
+ if (ret) {
+ dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
+ goto clk_err;
+ };
+ }
+
+ de = devm_kcalloc(pd->dev, 1,
+ sizeof(struct rockchip_dev_entry *), GFP_KERNEL);Why devm_calloc for a single element and not devm_kzalloc? Also, I am a bit concerned about using devm_* API here. They are better reserved fir driver's ->probe() paths whereas we are called from dev_pm_domain_attach() which is more general API (yes, currently it is used by buses probing code, but that might change in the future). Also, where is OOM error handling?
+ spin_lock_irq(&pd->dev_lock);
+ list_add_tail(&de->node, &pd->dev_list);
+ spin_unlock_irq(&pd->dev_lock);
+
+ return;
+clk_err:
+ pm_clk_destroy(dev);
+}
+
+void rockchip_pm_domain_detach_dev(struct device *dev)
+{
+ struct rockchip_domain *pd;
+ struct rockchip_dev_entry *de;
+
+ pd = (struct rockchip_domain *)dev->pm_domain;
+ spin_lock_irq(&pd->dev_lock);
+
+ list_for_each_entry(de, &pd->dev_list, node) {
+ if (de->dev == dev)
+ goto remove;If you use mutex instead of spinlock I think you'll be able to do list_del/pm_clk_destroy() right here. I'd free memory here as well. Thanks. -- Dmitry