[PATCH 0/2] ARM: Exynos: Adapt to generic power domain

[PATCH 0/2] ARM: Exynos: Adapt to generic power domain

[Thomas Abraham]

The power domains available on Exynos4 are made controllable using the generic
power domain infrastructure. The existing power domain code is not removed by
this patchset. Support for both DT and non-DT Exynos platforms is added.

The first patch adds device tree support for generic power domain. A device
node is added to the generic power domain strucure that represents a power
domain node in the device tree. Other device nodes in the device tree can
include a property with a phandle of the power domain node which will enable
that device to be associated with a power domain.

The second patch registers available power domains to generic power domain
framework with support for both dt and non-dt platforms.

The patchset is based on the following tree
http://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git for-next

with all patches merged from
http://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next

Thomas Abraham (2):
  PM / Domains: Add OF support
  ARM: Exynos: Hook up power domains to generic power domain infrastructure

 arch/arm/mach-exynos/Kconfig |    1 +
 arch/arm/mach-exynos/pm.c    |  179 ++++++++++++++++++++++++++++++++++++++++++
 drivers/base/power/domain.c  |   14 +++-
 include/linux/pm_domain.h    |   13 +++-
 4 files changed, 204 insertions(+), 3 deletions(-)

[PATCH 1/2] PM / Domains: Add OF support

[Thomas Abraham]

A device node pointer is added to generic pm domain structure to associate
the domain with a node in the device tree. The platform code parses the
device tree to find available nodes representing the generic power domain,
instantiates the available domains and initializes them by calling
pm_genpd_init().

Nodes representing the devices include a phandle of the power domain to
which it belongs. As these devices get instantiated, the driver code
checkes for availability of a power domain phandle, converts the phandle
to a device node and uses the new pm_genpd_of_add_device() api to
associate the device with a power domain.

pm_genpd_of_add_device() runs through its list of registered power domains
and matches the OF node of the domain with the one specified as the
parameter. If a match is found, the device is associated with the matched
domain.

Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/base/power/domain.c |   14 +++++++++++++-
 include/linux/pm_domain.h   |   13 +++++++++++--
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 92e6a90..e895dc9 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1118,14 +1118,26 @@ static void pm_genpd_complete(struct device *dev)
  * @td: Set of PM QoS timing parameters to attach to the device.
  */
 int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
-			  struct gpd_timing_data *td)
+			  struct gpd_timing_data *td, struct device_node *node)
 {
 	struct generic_pm_domain_data *gpd_data;
+	struct generic_pm_domain *gpd = NULL;
 	struct pm_domain_data *pdd;
 	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
+	mutex_lock(&gpd_list_lock);
+	if (node) {
+		list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+			if (gpd->of_node == node) {
+				genpd = gpd;
+				break;
+			}
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a03a0ad..2ce7cc4 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -11,6 +11,7 @@
 
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/of.h>
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
@@ -70,6 +71,7 @@ struct generic_pm_domain {
 	s64 break_even_ns;	/* Power break even for the entire domain. */
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	ktime_t power_off_time;
+	struct device_node *of_node; /* Node in device tree */
 };
 
 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
@@ -115,12 +117,19 @@ extern struct dev_power_governor simple_qos_governor;
 extern struct generic_pm_domain *dev_to_genpd(struct device *dev);
 extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
 				 struct device *dev,
-				 struct gpd_timing_data *td);
+				 struct gpd_timing_data *td,
+				 struct device_node *gpd_node);
 
 static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
 				      struct device *dev)
 {
-	return __pm_genpd_add_device(genpd, dev, NULL);
+	return __pm_genpd_add_device(genpd, dev, NULL, NULL);
+}
+
+static inline int pm_genpd_of_add_device(struct device_node *gpd_node,
+					 struct device *dev)
+{
+	return __pm_genpd_add_device(NULL, dev, NULL, gpd_node);
 }
 
 extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
-- 
1.6.6.rc2

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Thomas Abraham]

The generic power domain infrastructure is used to control the power domains
available on Exynos4. For non-dt platforms, the power domains are statically
instantiated. For dt platforms, the power domain nodes found in the device
tree are instantiated.

Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
This patch is mainly derived from Mark Brown's work on generic power domain
support for s3c64xx platforms. The existing exynos4 power domain implementation
is not removed in this patch. The devices are not yet registered with the power
domains for non-dt platforms.

 arch/arm/mach-exynos/Kconfig |    1 +
 arch/arm/mach-exynos/pm.c    |  179 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 0afcc3b..c1f77c7 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -29,6 +29,7 @@ config CPU_EXYNOS4210
 	default y
 	depends on ARCH_EXYNOS4
 	select SAMSUNG_DMADEV
+	select PM_GENERIC_DOMAINS
 	select ARM_CPU_SUSPEND if PM
 	select S5P_PM if PM
 	select S5P_SLEEP if PM
diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index 4093fea..a471ea6 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -20,6 +20,10 @@
 #include <linux/io.h>
 #include <linux/err.h>
 #include <linux/clk.h>
+#include <linux/slab.h>
+#include <linux/pm_domain.h>
+#include <linux/delay.h>
+#include <linux/of_address.h>
 
 #include <asm/cacheflush.h>
 #include <asm/hardware/cache-l2x0.h>
@@ -37,6 +41,13 @@
 #include <mach/pm-core.h>
 #include <mach/pmu.h>
 
+struct exynos4_pm_domain {
+	void __iomem *base;
+	char const *name;
+	bool is_off;
+	struct generic_pm_domain pd;
+};
+
 static struct sleep_save exynos4_set_clksrc[] = {
 	{ .reg = S5P_CLKSRC_MASK_TOP			, .val = 0x00000001, },
 	{ .reg = S5P_CLKSRC_MASK_CAM			, .val = 0x11111111, },
@@ -285,12 +296,172 @@ static struct sysdev_driver exynos4_pm_driver = {
 	.add		= exynos4_pm_add,
 };
 
+static int exynos4_pd_power_on(struct generic_pm_domain *domain)
+{
+	struct exynos4_pm_domain *pd;
+	void __iomem *base;
+	u32 timeout;
+
+	pd = container_of(domain, struct exynos4_pm_domain, pd);
+	base = pd->base;
+
+	__raw_writel(S5P_INT_LOCAL_PWR_EN, base);
+
+	/* Wait max 1ms */
+	timeout = 10;
+	while ((__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN)
+					!= S5P_INT_LOCAL_PWR_EN) {
+		if (!timeout) {
+			pr_err("Power domain %s enable failed\n", domain->name);
+			return -ETIMEDOUT;
+		}
+		timeout--;
+		udelay(100);
+	}
+	return 0;
+}
+
+static int exynos4_pd_power_off(struct generic_pm_domain *domain)
+{
+	struct exynos4_pm_domain *pd;
+	void __iomem *base;
+	u32 timeout;
+
+	pd = container_of(domain, struct exynos4_pm_domain, pd);
+	base = pd->base;
+
+	__raw_writel(0, base);
+
+	/* Wait max 1ms */
+	timeout = 10;
+	while (__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN) {
+		if (!timeout) {
+			pr_err("Power domain %s disable failed\n", domain->name);
+			return -ETIMEDOUT;
+		}
+		timeout--;
+		udelay(100);
+	}
+	return 0;
+}
+
+static struct exynos4_pm_domain exynos4_pd_mfc = {
+	.base = (void __iomem *)S5P_PMU_MFC_CONF,
+	.name = "pd-mfc",
+	.pd = {
+		.power_off = exynos4_pd_power_off,
+		.power_on = exynos4_pd_power_on,
+	},
+};
+
+static struct exynos4_pm_domain exynos4_pd_g3d = {
+	.base = (void __iomem *)S5P_PMU_G3D_CONF,
+	.name = "pd-g3d",
+	.pd = {
+		.power_off = exynos4_pd_power_off,
+		.power_on = exynos4_pd_power_on,
+	},
+};
+
+static struct exynos4_pm_domain exynos4_pd_lcd0 = {
+	.base = (void __iomem *)S5P_PMU_LCD0_CONF,
+	.name = "pd-lcd0",
+	.pd = {
+		.power_off = exynos4_pd_power_off,
+		.power_on = exynos4_pd_power_on,
+	},
+};
+
+static struct exynos4_pm_domain exynos4_pd_lcd1 = {
+	.base = (void __iomem *)S5P_PMU_LCD1_CONF,
+	.name = "pd-lcd1",
+	.pd = {
+		.power_off = exynos4_pd_power_off,
+		.power_on = exynos4_pd_power_on,
+	},
+};
+
+static struct exynos4_pm_domain exynos4_pd_tv = {
+	.base = (void __iomem *)S5P_PMU_TV_CONF,
+	.name = "pd-tv",
+	.pd = {
+		.power_off = exynos4_pd_power_off,
+		.power_on = exynos4_pd_power_on,
+	},
+};
+
+static struct exynos4_pm_domain exynos4_pd_cam = {
+	.base = (void __iomem *)S5P_PMU_CAM_CONF,
+	.name = "pd-cam",
+	.pd = {
+		.power_off = exynos4_pd_power_off,
+		.power_on = exynos4_pd_power_on,
+	},
+};
+
+static struct exynos4_pm_domain exynos4_pd_gps = {
+	.base = (void __iomem *)S5P_PMU_GPS_CONF,
+	.name = "pd-gps",
+	.pd = {
+		.power_off = exynos4_pd_power_off,
+		.power_on = exynos4_pd_power_on,
+	},
+};
+
+static struct exynos4_pm_domain *exynos4_pm_domains[] = {
+	&exynos4_pd_mfc,
+	&exynos4_pd_g3d,
+	&exynos4_pd_lcd0,
+	&exynos4_pd_lcd1,
+	&exynos4_pd_tv,
+	&exynos4_pd_cam,
+	&exynos4_pd_gps,
+};
+
+static __init void exynos4_pm_init_power_domain(void)
+{
+	int idx;
+	struct device_node *np;
+
+#ifdef CONFIG_OF
+	if (!of_have_populated_dt())
+		goto no_dt;
+
+	for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
+		struct exynos4_pm_domain *pd;
+
+		pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+		if (!pd) {
+			pr_err("exynos4_pm_init_power_domain: memalloc "
+					"failed\n");
+			return;
+		}
+
+		if (of_get_property(np, "samsung,exynos4210-pd-off", NULL))
+			pd->is_off = true;
+		pd->name = np->name;
+		pd->base = of_iomap(np, 0);
+		pd->pd.power_off = exynos4_pd_power_off;
+		pd->pd.power_on = exynos4_pd_power_on;
+		pd->pd.of_node = np;
+		pm_genpd_init(&pd->pd, NULL, false);
+	}
+	return;
+#endif /* CONFIG_OF */
+
+no_dt:
+	for (idx = 0; idx < ARRAY_SIZE(exynos4_pm_domains); idx++)
+		pm_genpd_init(&exynos4_pm_domains[idx]->pd, NULL,
+				exynos4_pm_domains[idx]->is_off);
+}
+
 static __init int exynos4_pm_drvinit(void)
 {
 	struct clk *pll_base;
 	unsigned int tmp;
 
 	s3c_pm_init();
+	exynos4_pm_init_power_domain();
 
 	/* All wakeup disable */
 
@@ -406,3 +577,11 @@ static __init int exynos4_pm_syscore_init(void)
 	return 0;
 }
 arch_initcall(exynos4_pm_syscore_init);
+
+
+static __init int exynos4_pm_late_initcall(void)
+{
+	pm_genpd_poweroff_unused();
+	return 0;
+}
+late_initcall(exynos4_pm_late_initcall);
-- 
1.6.6.rc2

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Mark Brown]

On Mon, Dec 12, 2011 at 09:16:29PM +0530, Thomas Abraham wrote:

> +	/* Wait max 1ms */
> +	timeout = 10;
> +	while ((__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN)
> +					!= S5P_INT_LOCAL_PWR_EN) {
> +		if (!timeout) {
> +			pr_err("Power domain %s enable failed\n", domain->name);
> +			return -ETIMEDOUT;
> +		}
> +		timeout--;
> +		udelay(100);
> +	}

Might be worth putting a cpu_relax() in there just to be a bit nicer?

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Sylwester Nawrocki]

On 12/26/2011 08:06 PM, Mark Brown wrote:
> On Mon, Dec 12, 2011 at 09:16:29PM +0530, Thomas Abraham wrote:
> 
>> +	/* Wait max 1ms */
>> +	timeout = 10;
>> +	while ((__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN)
>> +					!= S5P_INT_LOCAL_PWR_EN) {
>> +		if (!timeout) {
>> +			pr_err("Power domain %s enable failed\n", domain->name);
>> +			return -ETIMEDOUT;
>> +		}
>> +		timeout--;
>> +		udelay(100);
>> +	}
> 
> Might be worth putting a cpu_relax() in there just to be a bit nicer?

There is a quite significant delay within the loop, thus it might make more
sense to replace udelay() with usleep_range() instead.

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Sylwester Nawrocki]

Hi Thomas,

On 12/12/2011 04:46 PM, Thomas Abraham wrote:
> The generic power domain infrastructure is used to control the power domains
> available on Exynos4. For non-dt platforms, the power domains are statically
> instantiated. For dt platforms, the power domain nodes found in the device
> tree are instantiated.
> 
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
> This patch is mainly derived from Mark Brown's work on generic power domain
> support for s3c64xx platforms. The existing exynos4 power domain implementation
> is not removed in this patch. The devices are not yet registered with the power
> domains for non-dt platforms.
> 
>  arch/arm/mach-exynos/Kconfig |    1 +
>  arch/arm/mach-exynos/pm.c    |  179 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 180 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index 0afcc3b..c1f77c7 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -29,6 +29,7 @@ config CPU_EXYNOS4210
>  	default y
>  	depends on ARCH_EXYNOS4
>  	select SAMSUNG_DMADEV
> +	select PM_GENERIC_DOMAINS
>  	select ARM_CPU_SUSPEND if PM
>  	select S5P_PM if PM
>  	select S5P_SLEEP if PM
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 4093fea..a471ea6 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -20,6 +20,10 @@
>  #include <linux/io.h>
>  #include <linux/err.h>
>  #include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <linux/pm_domain.h>
> +#include <linux/delay.h>
> +#include <linux/of_address.h>
>  
>  #include <asm/cacheflush.h>
>  #include <asm/hardware/cache-l2x0.h>
> @@ -37,6 +41,13 @@
>  #include <mach/pm-core.h>
>  #include <mach/pmu.h>
>  
> +struct exynos4_pm_domain {
> +	void __iomem *base;
> +	char const *name;
> +	bool is_off;
> +	struct generic_pm_domain pd;
> +};
> +
>  static struct sleep_save exynos4_set_clksrc[] = {
>  	{ .reg = S5P_CLKSRC_MASK_TOP			, .val = 0x00000001, },
>  	{ .reg = S5P_CLKSRC_MASK_CAM			, .val = 0x11111111, },
> @@ -285,12 +296,172 @@ static struct sysdev_driver exynos4_pm_driver = {
>  	.add		= exynos4_pm_add,
>  };
>  
> +static int exynos4_pd_power_on(struct generic_pm_domain *domain)
> +{
> +	struct exynos4_pm_domain *pd;
> +	void __iomem *base;
> +	u32 timeout;
> +
> +	pd = container_of(domain, struct exynos4_pm_domain, pd);
> +	base = pd->base;
> +
> +	__raw_writel(S5P_INT_LOCAL_PWR_EN, base);
> +
> +	/* Wait max 1ms */
> +	timeout = 10;
> +	while ((__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN)
> +					!= S5P_INT_LOCAL_PWR_EN) {
> +		if (!timeout) {
> +			pr_err("Power domain %s enable failed\n", domain->name);
> +			return -ETIMEDOUT;
> +		}
> +		timeout--;
> +		udelay(100);
> +	}
> +	return 0;
> +}
> +
> +static int exynos4_pd_power_off(struct generic_pm_domain *domain)
> +{
> +	struct exynos4_pm_domain *pd;
> +	void __iomem *base;
> +	u32 timeout;
> +
> +	pd = container_of(domain, struct exynos4_pm_domain, pd);
> +	base = pd->base;
> +
> +	__raw_writel(0, base);
> +
> +	/* Wait max 1ms */
> +	timeout = 10;
> +	while (__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN) {
> +		if (!timeout) {
> +			pr_err("Power domain %s disable failed\n", domain->name);
> +			return -ETIMEDOUT;
> +		}
> +		timeout--;
> +		udelay(100);
> +	}
> +	return 0;
> +}
> +
> +static struct exynos4_pm_domain exynos4_pd_mfc = {
> +	.base = (void __iomem *)S5P_PMU_MFC_CONF,
> +	.name = "pd-mfc",
> +	.pd = {
> +		.power_off = exynos4_pd_power_off,
> +		.power_on = exynos4_pd_power_on,
> +	},
> +};
> +
> +static struct exynos4_pm_domain exynos4_pd_g3d = {
> +	.base = (void __iomem *)S5P_PMU_G3D_CONF,
> +	.name = "pd-g3d",
> +	.pd = {
> +		.power_off = exynos4_pd_power_off,
> +		.power_on = exynos4_pd_power_on,
> +	},
> +};
> +
> +static struct exynos4_pm_domain exynos4_pd_lcd0 = {
> +	.base = (void __iomem *)S5P_PMU_LCD0_CONF,
> +	.name = "pd-lcd0",
> +	.pd = {
> +		.power_off = exynos4_pd_power_off,
> +		.power_on = exynos4_pd_power_on,
> +	},
> +};
> +
> +static struct exynos4_pm_domain exynos4_pd_lcd1 = {
> +	.base = (void __iomem *)S5P_PMU_LCD1_CONF,
> +	.name = "pd-lcd1",
> +	.pd = {
> +		.power_off = exynos4_pd_power_off,
> +		.power_on = exynos4_pd_power_on,
> +	},
> +};
> +
> +static struct exynos4_pm_domain exynos4_pd_tv = {
> +	.base = (void __iomem *)S5P_PMU_TV_CONF,
> +	.name = "pd-tv",
> +	.pd = {
> +		.power_off = exynos4_pd_power_off,
> +		.power_on = exynos4_pd_power_on,
> +	},
> +};
> +
> +static struct exynos4_pm_domain exynos4_pd_cam = {
> +	.base = (void __iomem *)S5P_PMU_CAM_CONF,
> +	.name = "pd-cam",
> +	.pd = {
> +		.power_off = exynos4_pd_power_off,
> +		.power_on = exynos4_pd_power_on,
> +	},
> +};
> +
> +static struct exynos4_pm_domain exynos4_pd_gps = {
> +	.base = (void __iomem *)S5P_PMU_GPS_CONF,
> +	.name = "pd-gps",
> +	.pd = {
> +		.power_off = exynos4_pd_power_off,
> +		.power_on = exynos4_pd_power_on,
> +	},
> +};

I'm not sure if arch/arm/mach-exynos/pm.c is the right place to add this.
IMHO it would be much better to put PD in separate file, pm-runtime.c
or something like this.

Can we assume various exynos versions will have same S5P_PMU_*_CONF
addresses? For old s3c64xx SoCs such assumption could be valid but I'm
a bit uncertain about Exynos series.

IMHO the proper way to proceed would be to convert Samsung power domains
to generic power domains and then add device tree support. Rather than
doing a sort of work around. Much of the conversion to genpd work have
been already done [1]. We would perhaps just need to drop the clock gating
as there seem to be no agreement about this.

[1]. http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg07431.html

--

Thanks,
Sylwester

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Thomas Abraham]

Hi Sylwester,

On 28 December 2011 04:44, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> Hi Thomas,
>
> On 12/12/2011 04:46 PM, Thomas Abraham wrote:
>> The generic power domain infrastructure is used to control the power domains
>> available on Exynos4. For non-dt platforms, the power domains are statically
>> instantiated. For dt platforms, the power domain nodes found in the device
>> tree are instantiated.
>>
>> Cc: Kukjin Kim <kgene.kim@samsung.com>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> This patch is mainly derived from Mark Brown's work on generic power domain
>> support for s3c64xx platforms. The existing exynos4 power domain implementation
>> is not removed in this patch. The devices are not yet registered with the power
>> domains for non-dt platforms.
>>
>> ?arch/arm/mach-exynos/Kconfig | ? ?1 +
>> ?arch/arm/mach-exynos/pm.c ? ?| ?179 ++++++++++++++++++++++++++++++++++++++++++
>> ?2 files changed, 180 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
>> index 0afcc3b..c1f77c7 100644
>> --- a/arch/arm/mach-exynos/Kconfig
>> +++ b/arch/arm/mach-exynos/Kconfig
>> @@ -29,6 +29,7 @@ config CPU_EXYNOS4210
>> ? ? ? default y
>> ? ? ? depends on ARCH_EXYNOS4
>> ? ? ? select SAMSUNG_DMADEV
>> + ? ? select PM_GENERIC_DOMAINS
>> ? ? ? select ARM_CPU_SUSPEND if PM
>> ? ? ? select S5P_PM if PM
>> ? ? ? select S5P_SLEEP if PM
>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>> index 4093fea..a471ea6 100644
>> --- a/arch/arm/mach-exynos/pm.c
>> +++ b/arch/arm/mach-exynos/pm.c
>> @@ -20,6 +20,10 @@
>> ?#include <linux/io.h>
>> ?#include <linux/err.h>
>> ?#include <linux/clk.h>
>> +#include <linux/slab.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/delay.h>
>> +#include <linux/of_address.h>
>>
>> ?#include <asm/cacheflush.h>
>> ?#include <asm/hardware/cache-l2x0.h>
>> @@ -37,6 +41,13 @@
>> ?#include <mach/pm-core.h>
>> ?#include <mach/pmu.h>
>>
>> +struct exynos4_pm_domain {
>> + ? ? void __iomem *base;
>> + ? ? char const *name;
>> + ? ? bool is_off;
>> + ? ? struct generic_pm_domain pd;
>> +};
>> +
>> ?static struct sleep_save exynos4_set_clksrc[] = {
>> ? ? ? { .reg = S5P_CLKSRC_MASK_TOP ? ? ? ? ? ? ? ? ? ?, .val = 0x00000001, },
>> ? ? ? { .reg = S5P_CLKSRC_MASK_CAM ? ? ? ? ? ? ? ? ? ?, .val = 0x11111111, },
>> @@ -285,12 +296,172 @@ static struct sysdev_driver exynos4_pm_driver = {
>> ? ? ? .add ? ? ? ? ? ?= exynos4_pm_add,
>> ?};
>>
>> +static int exynos4_pd_power_on(struct generic_pm_domain *domain)
>> +{
>> + ? ? struct exynos4_pm_domain *pd;
>> + ? ? void __iomem *base;
>> + ? ? u32 timeout;
>> +
>> + ? ? pd = container_of(domain, struct exynos4_pm_domain, pd);
>> + ? ? base = pd->base;
>> +
>> + ? ? __raw_writel(S5P_INT_LOCAL_PWR_EN, base);
>> +
>> + ? ? /* Wait max 1ms */
>> + ? ? timeout = 10;
>> + ? ? while ((__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? != S5P_INT_LOCAL_PWR_EN) {
>> + ? ? ? ? ? ? if (!timeout) {
>> + ? ? ? ? ? ? ? ? ? ? pr_err("Power domain %s enable failed\n", domain->name);
>> + ? ? ? ? ? ? ? ? ? ? return -ETIMEDOUT;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? ? timeout--;
>> + ? ? ? ? ? ? udelay(100);
>> + ? ? }
>> + ? ? return 0;
>> +}
>> +
>> +static int exynos4_pd_power_off(struct generic_pm_domain *domain)
>> +{
>> + ? ? struct exynos4_pm_domain *pd;
>> + ? ? void __iomem *base;
>> + ? ? u32 timeout;
>> +
>> + ? ? pd = container_of(domain, struct exynos4_pm_domain, pd);
>> + ? ? base = pd->base;
>> +
>> + ? ? __raw_writel(0, base);
>> +
>> + ? ? /* Wait max 1ms */
>> + ? ? timeout = 10;
>> + ? ? while (__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN) {
>> + ? ? ? ? ? ? if (!timeout) {
>> + ? ? ? ? ? ? ? ? ? ? pr_err("Power domain %s disable failed\n", domain->name);
>> + ? ? ? ? ? ? ? ? ? ? return -ETIMEDOUT;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? ? timeout--;
>> + ? ? ? ? ? ? udelay(100);
>> + ? ? }
>> + ? ? return 0;
>> +}
>> +
>> +static struct exynos4_pm_domain exynos4_pd_mfc = {
>> + ? ? .base = (void __iomem *)S5P_PMU_MFC_CONF,
>> + ? ? .name = "pd-mfc",
>> + ? ? .pd = {
>> + ? ? ? ? ? ? .power_off = exynos4_pd_power_off,
>> + ? ? ? ? ? ? .power_on = exynos4_pd_power_on,
>> + ? ? },
>> +};
>> +
>> +static struct exynos4_pm_domain exynos4_pd_g3d = {
>> + ? ? .base = (void __iomem *)S5P_PMU_G3D_CONF,
>> + ? ? .name = "pd-g3d",
>> + ? ? .pd = {
>> + ? ? ? ? ? ? .power_off = exynos4_pd_power_off,
>> + ? ? ? ? ? ? .power_on = exynos4_pd_power_on,
>> + ? ? },
>> +};
>> +
>> +static struct exynos4_pm_domain exynos4_pd_lcd0 = {
>> + ? ? .base = (void __iomem *)S5P_PMU_LCD0_CONF,
>> + ? ? .name = "pd-lcd0",
>> + ? ? .pd = {
>> + ? ? ? ? ? ? .power_off = exynos4_pd_power_off,
>> + ? ? ? ? ? ? .power_on = exynos4_pd_power_on,
>> + ? ? },
>> +};
>> +
>> +static struct exynos4_pm_domain exynos4_pd_lcd1 = {
>> + ? ? .base = (void __iomem *)S5P_PMU_LCD1_CONF,
>> + ? ? .name = "pd-lcd1",
>> + ? ? .pd = {
>> + ? ? ? ? ? ? .power_off = exynos4_pd_power_off,
>> + ? ? ? ? ? ? .power_on = exynos4_pd_power_on,
>> + ? ? },
>> +};
>> +
>> +static struct exynos4_pm_domain exynos4_pd_tv = {
>> + ? ? .base = (void __iomem *)S5P_PMU_TV_CONF,
>> + ? ? .name = "pd-tv",
>> + ? ? .pd = {
>> + ? ? ? ? ? ? .power_off = exynos4_pd_power_off,
>> + ? ? ? ? ? ? .power_on = exynos4_pd_power_on,
>> + ? ? },
>> +};
>> +
>> +static struct exynos4_pm_domain exynos4_pd_cam = {
>> + ? ? .base = (void __iomem *)S5P_PMU_CAM_CONF,
>> + ? ? .name = "pd-cam",
>> + ? ? .pd = {
>> + ? ? ? ? ? ? .power_off = exynos4_pd_power_off,
>> + ? ? ? ? ? ? .power_on = exynos4_pd_power_on,
>> + ? ? },
>> +};
>> +
>> +static struct exynos4_pm_domain exynos4_pd_gps = {
>> + ? ? .base = (void __iomem *)S5P_PMU_GPS_CONF,
>> + ? ? .name = "pd-gps",
>> + ? ? .pd = {
>> + ? ? ? ? ? ? .power_off = exynos4_pd_power_off,
>> + ? ? ? ? ? ? .power_on = exynos4_pd_power_on,
>> + ? ? },
>> +};
>
> I'm not sure if arch/arm/mach-exynos/pm.c is the right place to add this.
> IMHO it would be much better to put PD in separate file, pm-runtime.c
> or something like this.

Ok. I will move this to a new file pm-runtime.c.

>
> Can we assume various exynos versions will have same S5P_PMU_*_CONF
> addresses? For old s3c64xx SoCs such assumption could be valid but I'm
> a bit uncertain about Exynos series.

The addresses could be different, but it can be mapped to a
S5P_PMU_*_CONF. Maybe I did not understand your question here.

>
> IMHO the proper way to proceed would be to convert Samsung power domains
> to generic power domains and then add device tree support. Rather than

Yes, this patchset is intended to replace Samsung specific power
domains with generic power domains.

> doing a sort of work around. Much of the conversion to genpd work have
> been already done [1]. We would perhaps just need to drop the clock gating
> as there seem to be no agreement about this.

Ok. Thanks for the link. I think I missed looking at this patch.

Thanks,
Thomas.

>
> [1]. http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg07431.html
>
> --
>
> Thanks,
> Sylwester

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Sylwester Nawrocki]

Hi Thomas,

On 12/28/2011 06:25 AM, Thomas Abraham wrote:
>>> +
>>> +static struct exynos4_pm_domain exynos4_pd_gps = {
>>> +     .base = (void __iomem *)S5P_PMU_GPS_CONF,
>>> +     .name = "pd-gps",
>>> +     .pd = {
>>> +             .power_off = exynos4_pd_power_off,
>>> +             .power_on = exynos4_pd_power_on,
>>> +     },
>>> +};
>>
>> I'm not sure if arch/arm/mach-exynos/pm.c is the right place to add this.
>> IMHO it would be much better to put PD in separate file, pm-runtime.c
>> or something like this.
> 
> Ok. I will move this to a new file pm-runtime.c.

Thanks.

>> Can we assume various exynos versions will have same S5P_PMU_*_CONF
>> addresses? For old s3c64xx SoCs such assumption could be valid but I'm
>> a bit uncertain about Exynos series.
> 
> The addresses could be different, but it can be mapped to a
> S5P_PMU_*_CONF. Maybe I did not understand your question here.

I meant that for handling the newer SoCs (e.g. exynos5) at runtime we might need
to create new power domain definitions. That's why I suggested new file
for runtime PM. mach-shmobile has even separate compilation unit per SoC,
with register definitions put directly in C file. I'm not sure we need separate
pm-exynos4.c, pm-exynos5.c... For now your patch looks good.

--

Regards,
Sylwester

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Sylwester Nawrocki]

Hi Thomas,

On 12/12/2011 04:46 PM, Thomas Abraham wrote:
> The generic power domain infrastructure is used to control the power domains
> available on Exynos4. For non-dt platforms, the power domains are statically
> instantiated. For dt platforms, the power domain nodes found in the device
> tree are instantiated.
> 
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
> This patch is mainly derived from Mark Brown's work on generic power domain
> support for s3c64xx platforms. The existing exynos4 power domain implementation
> is not removed in this patch. The devices are not yet registered with the power
> domains for non-dt platforms.
> 
>  arch/arm/mach-exynos/Kconfig |    1 +
>  arch/arm/mach-exynos/pm.c    |  179 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 180 insertions(+), 0 deletions(-)
> 
[...]
> +
> +static struct exynos4_pm_domain exynos4_pd_gps = {
> +	.base = (void __iomem *)S5P_PMU_GPS_CONF,
> +	.name = "pd-gps",
> +	.pd = {
> +		.power_off = exynos4_pd_power_off,
> +		.power_on = exynos4_pd_power_on,
> +	},
> +};
> +
> +static struct exynos4_pm_domain *exynos4_pm_domains[] = {
> +	&exynos4_pd_mfc,
> +	&exynos4_pd_g3d,
> +	&exynos4_pd_lcd0,
> +	&exynos4_pd_lcd1,
> +	&exynos4_pd_tv,
> +	&exynos4_pd_cam,
> +	&exynos4_pd_gps,
> +};
> +
> +static __init void exynos4_pm_init_power_domain(void)
> +{
> +	int idx;
> +	struct device_node *np;
> +
> +#ifdef CONFIG_OF
> +	if (!of_have_populated_dt())
> +		goto no_dt;
> +
> +	for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
> +		struct exynos4_pm_domain *pd;
> +
> +		pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> +		if (!pd) {
> +			pr_err("exynos4_pm_init_power_domain: memalloc "
> +					"failed\n");
> +			return;
> +		}
> +
> +		if (of_get_property(np, "samsung,exynos4210-pd-off", NULL))
> +			pd->is_off = true;
> +		pd->name = np->name;
> +		pd->base = of_iomap(np, 0);

Sorry, I haven't reviewed your patch carefully enough. So for dt platforms
pd->base is initialized from "reg" property, directly from each power domain's
DT node. Only the static power domain instantiation for non-dt platforms would
possibly need some code modifications when new SoCs are added.

Would be nice to have a relevant patch for *.dts files in this series too. :)

> +		pd->pd.power_off = exynos4_pd_power_off;
> +		pd->pd.power_on = exynos4_pd_power_on;
> +		pd->pd.of_node = np;
> +		pm_genpd_init(&pd->pd, NULL, false);
> +	}
> +	return;
> +#endif /* CONFIG_OF */
> +
> +no_dt:
> +	for (idx = 0; idx < ARRAY_SIZE(exynos4_pm_domains); idx++)
> +		pm_genpd_init(&exynos4_pm_domains[idx]->pd, NULL,
> +				exynos4_pm_domains[idx]->is_off);
> +}

--
Thanks,
Sylwester

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Thomas Abraham]

Hi Sylwester,

On 29 December 2011 00:28, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> Hi Thomas,
>
> On 12/12/2011 04:46 PM, Thomas Abraham wrote:
>> The generic power domain infrastructure is used to control the power domains
>> available on Exynos4. For non-dt platforms, the power domains are statically
>> instantiated. For dt platforms, the power domain nodes found in the device
>> tree are instantiated.
>>
>> Cc: Kukjin Kim <kgene.kim@samsung.com>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> This patch is mainly derived from Mark Brown's work on generic power domain
>> support for s3c64xx platforms. The existing exynos4 power domain implementation
>> is not removed in this patch. The devices are not yet registered with the power
>> domains for non-dt platforms.
>>
>> ?arch/arm/mach-exynos/Kconfig | ? ?1 +
>> ?arch/arm/mach-exynos/pm.c ? ?| ?179 ++++++++++++++++++++++++++++++++++++++++++
>> ?2 files changed, 180 insertions(+), 0 deletions(-)

[...]

> Sorry, I haven't reviewed your patch carefully enough. So for dt platforms
> pd->base is initialized from "reg" property, directly from each power domain's
> DT node. Only the static power domain instantiation for non-dt platforms would
> possibly need some code modifications when new SoCs are added.
>
> Would be nice to have a relevant patch for *.dts files in this series too. :)

The following is a snippet from the dts file used for testing.

   [...]

   lcd0:power-domain-lcd0 {
            compatible = "samsung,exynos4210-pd";
            reg = <0x10023C00 0x10>;
   };

   [...]

   fimd0:display-controller {
            compatible = "samsung,exynos4-fimd";
            [...]
            pd = <&lcd0>;
   };

The fimd (display controller) driver would then do the following.

parp = of_get_property(pdev->dev.of_node, "pd", NULL);
pd_np = of_find_node_by_phandle(be32_to_cpup(parp));
pm_genpd_of_add_device(pd_np, &pdev->dev);

The lookup is based on the node pointer of the power domain.

Thanks,
Thomas.

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Sylwester Nawrocki]

Hi Thomas,

thank you for clarifying.

On 01/02/2012 03:14 AM, Thomas Abraham wrote:
> 
> The following is a snippet from the dts file used for testing.
> 
>    [...]
> 
>    lcd0:power-domain-lcd0 {
>             compatible = "samsung,exynos4210-pd";
>             reg = <0x10023C00 0x10>;
>    };
> 
>    [...]
> 
>    fimd0:display-controller {
>             compatible = "samsung,exynos4-fimd";
>             [...]
>             pd = <&lcd0>;
>    };
> 
> The fimd (display controller) driver would then do the following.
> 
> parp = of_get_property(pdev->dev.of_node, "pd", NULL);
> pd_np = of_find_node_by_phandle(be32_to_cpup(parp));
> pm_genpd_of_add_device(pd_np, &pdev->dev);

Sounds interesting. Currently it's platform code that adds devices to
a corresponding power domain. But doing it at drivers might be more
convenient for avoiding device/driver/power domain registration
synchronization issues, especially that knowledge about power domain
existence may be contained directly in DT description, not needing
drivers to carry platform specific data.

BTW, I have a feeling that "samsung" is a bit longish prefix for the bindings.
Didn't you initially consider "sec" for instance ? Probably it is already
too late for changing that though.

> The lookup is based on the node pointer of the power domain.

Thanks,
Sylwester

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Thomas Abraham]

Hi Sylwester,

On 3 January 2012 03:49, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> Hi Thomas,
>
> thank you for clarifying.
>
> On 01/02/2012 03:14 AM, Thomas Abraham wrote:
>>
>> The following is a snippet from the dts file used for testing.
>>
>> ? ?[...]
>>
>> ? ?lcd0:power-domain-lcd0 {
>> ? ? ? ? ? ? compatible = "samsung,exynos4210-pd";
>> ? ? ? ? ? ? reg = <0x10023C00 0x10>;
>> ? ?};
>>
>> ? ?[...]
>>
>> ? ?fimd0:display-controller {
>> ? ? ? ? ? ? compatible = "samsung,exynos4-fimd";
>> ? ? ? ? ? ? [...]
>> ? ? ? ? ? ? pd = <&lcd0>;
>> ? ?};
>>
>> The fimd (display controller) driver would then do the following.
>>
>> parp = of_get_property(pdev->dev.of_node, "pd", NULL);
>> pd_np = of_find_node_by_phandle(be32_to_cpup(parp));
>> pm_genpd_of_add_device(pd_np, &pdev->dev);
>
> Sounds interesting. Currently it's platform code that adds devices to
> a corresponding power domain. But doing it at drivers might be more
> convenient for avoiding device/driver/power domain registration
> synchronization issues, especially that knowledge about power domain
> existence may be contained directly in DT description, not needing
> drivers to carry platform specific data.
>
> BTW, I have a feeling that "samsung" is a bit longish prefix for the bindings.
> Didn't you initially consider "sec" for instance ? Probably it is already
> too late for changing that though.

I had not thought of "sec". I agree that "sec" would have been better
as it is shorter and represents bindings specific to Samsung
Electronics. But it is not intuitive at the same time. If there is
greater consensus on using "sec", we could try and request for a
change but looks difficult to get through.

Thanks,
Thomas.

>
>> The lookup is based on the node pointer of the power domain.
>
> Thanks,
> Sylwester

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Grant Likely]

On Tue, Jan 03, 2012 at 01:53:28PM +0530, Thomas Abraham wrote:
> Hi Sylwester,
> 
> On 3 January 2012 03:49, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> > Hi Thomas,
> >
> > thank you for clarifying.
> >
> > On 01/02/2012 03:14 AM, Thomas Abraham wrote:
> >>
> >> The following is a snippet from the dts file used for testing.
> >>
> >> ? ?[...]
> >>
> >> ? ?lcd0:power-domain-lcd0 {
> >> ? ? ? ? ? ? compatible = "samsung,exynos4210-pd";
> >> ? ? ? ? ? ? reg = <0x10023C00 0x10>;
> >> ? ?};
> >>
> >> ? ?[...]
> >>
> >> ? ?fimd0:display-controller {
> >> ? ? ? ? ? ? compatible = "samsung,exynos4-fimd";
> >> ? ? ? ? ? ? [...]
> >> ? ? ? ? ? ? pd = <&lcd0>;
> >> ? ?};
> >>
> >> The fimd (display controller) driver would then do the following.
> >>
> >> parp = of_get_property(pdev->dev.of_node, "pd", NULL);
> >> pd_np = of_find_node_by_phandle(be32_to_cpup(parp));
> >> pm_genpd_of_add_device(pd_np, &pdev->dev);
> >
> > Sounds interesting. Currently it's platform code that adds devices to
> > a corresponding power domain. But doing it at drivers might be more
> > convenient for avoiding device/driver/power domain registration
> > synchronization issues, especially that knowledge about power domain
> > existence may be contained directly in DT description, not needing
> > drivers to carry platform specific data.
> >
> > BTW, I have a feeling that "samsung" is a bit longish prefix for the bindings.
> > Didn't you initially consider "sec" for instance ? Probably it is already
> > too late for changing that though.
> 
> I had not thought of "sec". I agree that "sec" would have been better
> as it is shorter and represents bindings specific to Samsung
> Electronics. But it is not intuitive at the same time. If there is
> greater consensus on using "sec", we could try and request for a
> change but looks difficult to get through.

Don't bother.  'samsung,' is fine.

g.

[PATCH 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Kukjin Kim]

Grant Likely wrote:
> 
> On Tue, Jan 03, 2012 at 01:53:28PM +0530, Thomas Abraham wrote:
> > Hi Sylwester,
> >
> > On 3 January 2012 03:49, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> > > Hi Thomas,
> > >
> > > thank you for clarifying.
> > >
> > > On 01/02/2012 03:14 AM, Thomas Abraham wrote:
> > >>
> > >> The following is a snippet from the dts file used for testing.
> > >>
> > >> ? ?[...]
> > >>
> > >> ? ?lcd0:power-domain-lcd0 {
> > >> ? ? ? ? ? ? compatible = "samsung,exynos4210-pd";
> > >> ? ? ? ? ? ? reg = <0x10023C00 0x10>;
> > >> ? ?};
> > >>
> > >> ? ?[...]
> > >>
> > >> ? ?fimd0:display-controller {
> > >> ? ? ? ? ? ? compatible = "samsung,exynos4-fimd";
> > >> ? ? ? ? ? ? [...]
> > >> ? ? ? ? ? ? pd = <&lcd0>;
> > >> ? ?};
> > >>
> > >> The fimd (display controller) driver would then do the following.
> > >>
> > >> parp = of_get_property(pdev->dev.of_node, "pd", NULL);
> > >> pd_np = of_find_node_by_phandle(be32_to_cpup(parp));
> > >> pm_genpd_of_add_device(pd_np, &pdev->dev);
> > >
> > > Sounds interesting. Currently it's platform code that adds devices to
> > > a corresponding power domain. But doing it at drivers might be more
> > > convenient for avoiding device/driver/power domain registration
> > > synchronization issues, especially that knowledge about power domain
> > > existence may be contained directly in DT description, not needing
> > > drivers to carry platform specific data.
> > >
> > > BTW, I have a feeling that "samsung" is a bit longish prefix for the
> bindings.
> > > Didn't you initially consider "sec" for instance ? Probably it is
> already
> > > too late for changing that though.
> >
> > I had not thought of "sec". I agree that "sec" would have been better
> > as it is shorter and represents bindings specific to Samsung
> > Electronics. But it is not intuitive at the same time. If there is
> > greater consensus on using "sec", we could try and request for a
> > change but looks difficult to get through.
> 
> Don't bother.  'samsung,' is fine.
> 
Same here :)

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH 1/2] PM / Domains: Add OF support

[Mark Brown]

On Mon, Dec 12, 2011 at 09:16:28PM +0530, Thomas Abraham wrote:
> A device node pointer is added to generic pm domain structure to associate
> the domain with a node in the device tree. The platform code parses the
> device tree to find available nodes representing the generic power domain,
> instantiates the available domains and initializes them by calling
> pm_genpd_init().

This sounds useful but the binding needs to be documented properly.

[PATCH 1/2] PM / Domains: Add OF support

[Rafael J. Wysocki]

Hi,

On Monday, December 12, 2011, Thomas Abraham wrote:
> A device node pointer is added to generic pm domain structure to associate
> the domain with a node in the device tree.

That sounds fine except for one thing: PM domains are not devices, so adding
"device node" pointers to them is kind of confusing.  Perhaps there should be
something like struct dt_node, representing a more general device tree node?

> The platform code parses the
> device tree to find available nodes representing the generic power domain,
> instantiates the available domains and initializes them by calling
> pm_genpd_init().
> 
> Nodes representing the devices include a phandle of the power domain to
> which it belongs. As these devices get instantiated, the driver code
> checkes for availability of a power domain phandle, converts the phandle
> to a device node and uses the new pm_genpd_of_add_device() api to
> associate the device with a power domain.
> 
> pm_genpd_of_add_device() runs through its list of registered power domains
> and matches the OF node of the domain with the one specified as the
> parameter. If a match is found, the device is associated with the matched
> domain.
> 
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/base/power/domain.c |   14 +++++++++++++-
>  include/linux/pm_domain.h   |   13 +++++++++++--
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 92e6a90..e895dc9 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1118,14 +1118,26 @@ static void pm_genpd_complete(struct device *dev)
>   * @td: Set of PM QoS timing parameters to attach to the device.
>   */
>  int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> -			  struct gpd_timing_data *td)
> +			  struct gpd_timing_data *td, struct device_node *node)
>  {
>  	struct generic_pm_domain_data *gpd_data;
> +	struct generic_pm_domain *gpd = NULL;
>  	struct pm_domain_data *pdd;
>  	int ret = 0;
>  
>  	dev_dbg(dev, "%s()\n", __func__);
>  
> +	mutex_lock(&gpd_list_lock);
> +	if (node) {
> +		list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> +			if (gpd->of_node == node) {
> +				genpd = gpd;
> +				break;
> +			}
> +		}
> +	}
> +	mutex_unlock(&gpd_list_lock);

It looks like you might add a wrapper around the original
__pm_genpd_add_device() instead of modifying __pm_genpd_add_device()
itself.  That would be a bit cleaner, as your new function is always
called with at least one NULL arg.

> +
>  	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
>  		return -EINVAL;
>  
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a03a0ad..2ce7cc4 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -11,6 +11,7 @@
>  
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/of.h>
>  
>  enum gpd_status {
>  	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
> @@ -70,6 +71,7 @@ struct generic_pm_domain {
>  	s64 break_even_ns;	/* Power break even for the entire domain. */
>  	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
>  	ktime_t power_off_time;
> +	struct device_node *of_node; /* Node in device tree */

That should be initialized by the caller of pm_genpd_init(), right?

>  };
>  
>  static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
> @@ -115,12 +117,19 @@ extern struct dev_power_governor simple_qos_governor;
>  extern struct generic_pm_domain *dev_to_genpd(struct device *dev);
>  extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
>  				 struct device *dev,
> -				 struct gpd_timing_data *td);
> +				 struct gpd_timing_data *td,
> +				 struct device_node *gpd_node);
>  
>  static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
>  				      struct device *dev)
>  {
> -	return __pm_genpd_add_device(genpd, dev, NULL);
> +	return __pm_genpd_add_device(genpd, dev, NULL, NULL);
> +}
> +
> +static inline int pm_genpd_of_add_device(struct device_node *gpd_node,
> +					 struct device *dev)
> +{
> +	return __pm_genpd_add_device(NULL, dev, NULL, gpd_node);
>  }
>  
>  extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> 

Thanks,
Rafael

[PATCH 1/2] PM / Domains: Add OF support

[Mark Brown]

On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote:
> On Monday, December 12, 2011, Thomas Abraham wrote:

> > A device node pointer is added to generic pm domain structure to associate
> > the domain with a node in the device tree.

> That sounds fine except for one thing: PM domains are not devices, so adding
> "device node" pointers to them is kind of confusing.  Perhaps there should be
> something like struct dt_node, representing a more general device tree node?

There's struct of_node which is exactly that, though practically
speaking you need a device if you're going to bind automatically to
something from the device tree in a sensible fashion and there is actual
hardware under there so a device does make some sense.

This is in part compatibility with the existing Exynos code which uses
devices to probe the domains for non-DT systems.

[PATCH 1/2] PM / Domains: Add OF support

[Rafael J. Wysocki]

On Monday, December 26, 2011, Mark Brown wrote:
> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote:
> > On Monday, December 12, 2011, Thomas Abraham wrote:
> 
> > > A device node pointer is added to generic pm domain structure to associate
> > > the domain with a node in the device tree.
> 
> > That sounds fine except for one thing: PM domains are not devices, so adding
> > "device node" pointers to them is kind of confusing.  Perhaps there should be
> > something like struct dt_node, representing a more general device tree node?
> 
> There's struct of_node which is exactly that, though practically
> speaking you need a device if you're going to bind automatically to
> something from the device tree in a sensible fashion and there is actual
> hardware under there so a device does make some sense.
> 
> This is in part compatibility with the existing Exynos code which uses
> devices to probe the domains for non-DT systems.

Well, that's not a general case, though.

It doesn't feel approporiate to use a "device node" pointer for something
that's not based on struct device, at least not a generic level, so I wonder
if there's a different way.

Thanks,
Rafael

[PATCH 1/2] PM / Domains: Add OF support

[Thomas Abraham]

Hi Mark, Rafael,

On 27 December 2011 02:14, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, December 26, 2011, Mark Brown wrote:
>> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote:
>> > On Monday, December 12, 2011, Thomas Abraham wrote:
>>
>> > > A device node pointer is added to generic pm domain structure to associate
>> > > the domain with a node in the device tree.
>>
>> > That sounds fine except for one thing: PM domains are not devices, so adding
>> > "device node" pointers to them is kind of confusing. ?Perhaps there should be
>> > something like struct dt_node, representing a more general device tree node?
>>
>> There's struct of_node which is exactly that, though practically
>> speaking you need a device if you're going to bind automatically to
>> something from the device tree in a sensible fashion and there is actual
>> hardware under there so a device does make some sense.

In patch 2/2 of this series, the platform code looks for nodes in
device tree that represent a power domain. When a power domain node is
found, a generic power domain is instantiated with pm_genpd_init()
using the information available from the node in device tree. There is
no automatic binding required in this case. The power domain node does
represent a hardware that manages the power domain.

>>
>> This is in part compatibility with the existing Exynos code which uses
>> devices to probe the domains for non-DT systems.
>
> Well, that's not a general case, though.
>
> It doesn't feel approporiate to use a "device node" pointer for something
> that's not based on struct device, at least not a generic level, so I wonder
> if there's a different way.

A device node pointer or of_node pointer is a simple pointer to a
instance of a node in device tree. All nodes in a device tree need not
represent a corresponding 'struct device'. A node in device tree can
described a hardware feature such as a power domain supported in the
hardware.

The addition of device tree support for generic power domains in this
patchset is generic for all platforms. The platform code instantiates
generic power domains from device tree with the of_node pointer
assigned to 'struct generic_pm_domain'. Then, in
__pm_genpd_add_device(), given a of_node pointer (to gen_pd), it is
possible to find a matching power domain to select.

Thanks,
Thomas.

>
> Thanks,
> Rafael

[PATCH 1/2] PM / Domains: Add OF support

[Rafael J. Wysocki]

On Wednesday, December 28, 2011, Thomas Abraham wrote:
> Hi Mark, Rafael,

Hi,

> On 27 December 2011 02:14, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Monday, December 26, 2011, Mark Brown wrote:
> >> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote:
> >> > On Monday, December 12, 2011, Thomas Abraham wrote:
> >>
> >> > > A device node pointer is added to generic pm domain structure to associate
> >> > > the domain with a node in the device tree.
> >>
> >> > That sounds fine except for one thing: PM domains are not devices, so adding
> >> > "device node" pointers to them is kind of confusing.  Perhaps there should be
> >> > something like struct dt_node, representing a more general device tree node?
> >>
> >> There's struct of_node which is exactly that, though practically
> >> speaking you need a device if you're going to bind automatically to
> >> something from the device tree in a sensible fashion and there is actual
> >> hardware under there so a device does make some sense.
> 
> In patch 2/2 of this series, the platform code looks for nodes in
> device tree that represent a power domain. When a power domain node is
> found, a generic power domain is instantiated with pm_genpd_init()
> using the information available from the node in device tree. There is
> no automatic binding required in this case. The power domain node does
> represent a hardware that manages the power domain.

Good.  So would it be possible to use struct of_node instead of
struct device_node in struct generic_pm_domain?

> >>
> >> This is in part compatibility with the existing Exynos code which uses
> >> devices to probe the domains for non-DT systems.
> >
> > Well, that's not a general case, though.
> >
> > It doesn't feel approporiate to use a "device node" pointer for something
> > that's not based on struct device, at least not a generic level, so I wonder
> > if there's a different way.
> 
> A device node pointer or of_node pointer is a simple pointer to a
> instance of a node in device tree. All nodes in a device tree need not
> represent a corresponding 'struct device'. A node in device tree can
> described a hardware feature such as a power domain supported in the
> hardware.

Sure.

> The addition of device tree support for generic power domains in this
> patchset is generic for all platforms. The platform code instantiates
> generic power domains from device tree with the of_node pointer
> assigned to 'struct generic_pm_domain'. Then, in
> __pm_genpd_add_device(), given a of_node pointer (to gen_pd), it is
> possible to find a matching power domain to select.

My point was that adding the struct device_node pointer to
struct generic_pm_domain didn't look good, because that structure didn't
represent a device in general.  While I understand that it may be regarded
as a "device object" on some platforms, there are platforms that don't
regard PM domains as devices.  For this reason (and only for this reason)
it appears to be more appropriate to use a more generic device tree node
type for struct generic_pm_domain.

Thanks,
Rafael

[PATCH 1/2] PM / Domains: Add OF support

[Thomas Abraham]

Hi Rafael,

On 29 December 2011 03:47, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, December 28, 2011, Thomas Abraham wrote:
>> Hi Mark, Rafael,
>
> Hi,
>
>> On 27 December 2011 02:14, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Monday, December 26, 2011, Mark Brown wrote:
>> >> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote:
>> >> > On Monday, December 12, 2011, Thomas Abraham wrote:
>> >>
>> >> > > A device node pointer is added to generic pm domain structure to associate
>> >> > > the domain with a node in the device tree.
>> >>
>> >> > That sounds fine except for one thing: PM domains are not devices, so adding
>> >> > "device node" pointers to them is kind of confusing. ?Perhaps there should be
>> >> > something like struct dt_node, representing a more general device tree node?
>> >>
>> >> There's struct of_node which is exactly that, though practically
>> >> speaking you need a device if you're going to bind automatically to
>> >> something from the device tree in a sensible fashion and there is actual
>> >> hardware under there so a device does make some sense.
>>
>> In patch 2/2 of this series, the platform code looks for nodes in
>> device tree that represent a power domain. When a power domain node is
>> found, a generic power domain is instantiated with pm_genpd_init()
>> using the information available from the node in device tree. There is
>> no automatic binding required in this case. The power domain node does
>> represent a hardware that manages the power domain.
>
> Good. ?So would it be possible to use struct of_node instead of
> struct device_node in struct generic_pm_domain?

Sorry, I used 'struct of_node' and 'struct device_node'
interchangeably in my reply. All the nodes in a device tree are
represented by 'struct device_node'.

>
>> >>
>> >> This is in part compatibility with the existing Exynos code which uses
>> >> devices to probe the domains for non-DT systems.
>> >
>> > Well, that's not a general case, though.
>> >
>> > It doesn't feel approporiate to use a "device node" pointer for something
>> > that's not based on struct device, at least not a generic level, so I wonder
>> > if there's a different way.
>>
>> A device node pointer or of_node pointer is a simple pointer to a
>> instance of a node in device tree. All nodes in a device tree need not
>> represent a corresponding 'struct device'. A node in device tree can
>> described a hardware feature such as a power domain supported in the
>> hardware.
>
> Sure.
>
>> The addition of device tree support for generic power domains in this
>> patchset is generic for all platforms. The platform code instantiates
>> generic power domains from device tree with the of_node pointer
>> assigned to 'struct generic_pm_domain'. Then, in
>> __pm_genpd_add_device(), given a of_node pointer (to gen_pd), it is
>> possible to find a matching power domain to select.
>
> My point was that adding the struct device_node pointer to
> struct generic_pm_domain didn't look good, because that structure didn't
> represent a device in general. ?While I understand that it may be regarded
> as a "device object" on some platforms, there are platforms that don't
> regard PM domains as devices. ?For this reason (and only for this reason)
> it appears to be more appropriate to use a more generic device tree node
> type for struct generic_pm_domain.

If a platform uses some hardware controls (register read/writes) to
enable/disable/control power domain, then it can be represented in the
device tree. But such a node need not represent a 'struct device'.
Other nodes in the device tree can then reference the power domain
node.

If a platform does not have any hardware knobs for controlling power
domains, then probably it will not be represented in the device tree.
But I am not sure of this case.

The 'struct device_node' is a representation of a instance of a device
tree node. It can represent nodes that are not devices. Hence, it can
be used to represent a power domain in a device tree and also included
in the 'struct generic_pm_domain'.

Thanks,
Thomas.


>
> Thanks,
> Rafael

[PATCH 1/2] PM / Domains: Add OF support

[Rafael J. Wysocki]

On Monday, January 02, 2012, Thomas Abraham wrote:
> Hi Rafael,
> 
> On 29 December 2011 03:47, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, December 28, 2011, Thomas Abraham wrote:
> >> Hi Mark, Rafael,
> >
> > Hi,
> >
> >> On 27 December 2011 02:14, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Monday, December 26, 2011, Mark Brown wrote:
> >> >> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote:
> >> >> > On Monday, December 12, 2011, Thomas Abraham wrote:
> >> >>
> >> >> > > A device node pointer is added to generic pm domain structure to associate
> >> >> > > the domain with a node in the device tree.
> >> >>
> >> >> > That sounds fine except for one thing: PM domains are not devices, so adding
> >> >> > "device node" pointers to them is kind of confusing.  Perhaps there should be
> >> >> > something like struct dt_node, representing a more general device tree node?
> >> >>
> >> >> There's struct of_node which is exactly that, though practically
> >> >> speaking you need a device if you're going to bind automatically to
> >> >> something from the device tree in a sensible fashion and there is actual
> >> >> hardware under there so a device does make some sense.
> >>
> >> In patch 2/2 of this series, the platform code looks for nodes in
> >> device tree that represent a power domain. When a power domain node is
> >> found, a generic power domain is instantiated with pm_genpd_init()
> >> using the information available from the node in device tree. There is
> >> no automatic binding required in this case. The power domain node does
> >> represent a hardware that manages the power domain.
> >
> > Good.  So would it be possible to use struct of_node instead of
> > struct device_node in struct generic_pm_domain?
> 
> Sorry, I used 'struct of_node' and 'struct device_node'
> interchangeably in my reply. All the nodes in a device tree are
> represented by 'struct device_node'.
> 
> >
> >> >>
> >> >> This is in part compatibility with the existing Exynos code which uses
> >> >> devices to probe the domains for non-DT systems.
> >> >
> >> > Well, that's not a general case, though.
> >> >
> >> > It doesn't feel approporiate to use a "device node" pointer for something
> >> > that's not based on struct device, at least not a generic level, so I wonder
> >> > if there's a different way.
> >>
> >> A device node pointer or of_node pointer is a simple pointer to a
> >> instance of a node in device tree. All nodes in a device tree need not
> >> represent a corresponding 'struct device'. A node in device tree can
> >> described a hardware feature such as a power domain supported in the
> >> hardware.
> >
> > Sure.
> >
> >> The addition of device tree support for generic power domains in this
> >> patchset is generic for all platforms. The platform code instantiates
> >> generic power domains from device tree with the of_node pointer
> >> assigned to 'struct generic_pm_domain'. Then, in
> >> __pm_genpd_add_device(), given a of_node pointer (to gen_pd), it is
> >> possible to find a matching power domain to select.
> >
> > My point was that adding the struct device_node pointer to
> > struct generic_pm_domain didn't look good, because that structure didn't
> > represent a device in general.  While I understand that it may be regarded
> > as a "device object" on some platforms, there are platforms that don't
> > regard PM domains as devices.  For this reason (and only for this reason)
> > it appears to be more appropriate to use a more generic device tree node
> > type for struct generic_pm_domain.
> 
> If a platform uses some hardware controls (register read/writes) to
> enable/disable/control power domain, then it can be represented in the
> device tree. But such a node need not represent a 'struct device'.
> Other nodes in the device tree can then reference the power domain
> node.
> 
> If a platform does not have any hardware knobs for controlling power
> domains, then probably it will not be represented in the device tree.
> But I am not sure of this case.
> 
> The 'struct device_node' is a representation of a instance of a device
> tree node. It can represent nodes that are not devices. Hence, it can
> be used to represent a power domain in a device tree and also included
> in the 'struct generic_pm_domain'.

OK, so please address my second comment regarding the $subject patch,
i.e. that you might add a wrapper around the original
__pm_genpd_add_device() instead of modifying __pm_genpd_add_device()
itself.

Thanks,
Rafael

[PATCH 1/2] PM / Domains: Add OF support

[Thomas Abraham]

Hi Rafael,

On 4 January 2012 04:00, Rafael J. Wysocki <rjw@sisk.pl> wrote:
[...]

>> The 'struct device_node' is a representation of a instance of a device
>> tree node. It can represent nodes that are not devices. Hence, it can
>> be used to represent a power domain in a device tree and also included
>> in the 'struct generic_pm_domain'.
>
> OK, so please address my second comment regarding the $subject patch,
> i.e. that you might add a wrapper around the original
> __pm_genpd_add_device() instead of modifying __pm_genpd_add_device()
> itself.

Thanks. I will prepare and submit the updated patch.

Regards,
Thomas.

>
> Thanks,
> Rafael

[PATCH 1/2] PM / Domains: Add OF support

[Grant Likely]

On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote:
> Hi,
> 
> On Monday, December 12, 2011, Thomas Abraham wrote:
> > A device node pointer is added to generic pm domain structure to associate
> > the domain with a node in the device tree.
> 
> That sounds fine except for one thing: PM domains are not devices, so adding
> "device node" pointers to them is kind of confusing.  Perhaps there should be
> something like struct dt_node, representing a more general device tree node?

struct device_node has nothing to do with struct device.  device_node is a
generic device tree node.  Thomas' approach is fine.

g.

[PATCH 1/2] PM / Domains: Add OF support

[Rafael J. Wysocki]

On Monday, January 02, 2012, Grant Likely wrote:
> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Monday, December 12, 2011, Thomas Abraham wrote:
> > > A device node pointer is added to generic pm domain structure to associate
> > > the domain with a node in the device tree.
> > 
> > That sounds fine except for one thing: PM domains are not devices, so adding
> > "device node" pointers to them is kind of confusing.  Perhaps there should be
> > something like struct dt_node, representing a more general device tree node?
> 
> struct device_node has nothing to do with struct device.  device_node is a
> generic device tree node.

So the name is kind of confusing. :-)

> Thomas' approach is fine.

Well, I guess so.

Thanks,
Rafael