ARM: Exynos: Adapt to generic power domain

arch/arm/mach-exynos/pm.c static struct sysdev_driver exynos4_pm_driver = { · line 320

Quoted 11 lines · Thomas Abraham

arch/arm/mach-exynos/pm.c +11 -0

+	/* 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?

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 <[email protected]>

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
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(-)

arch/arm/mach-exynos/Kconfig +1 -0

index 0afcc3b..c1f77c7 100644
@@ -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

arch/arm/mach-exynos/pm.c +112 -0

index 4093fea..a471ea6 100644
@@ -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,
+	},

Hi Thomas,

Quoted 171 lines · 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 <[email protected]>

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
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(-)

arch/arm/mach-exynos/Kconfig +0 -0

index 0afcc3b..c1f77c7 100644
@@ -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

arch/arm/mach-exynos/pm.c +0 -0

index 4093fea..a471ea6 100644
@@ -20,6 +20,10 @@

?#include <linux/io.h>
?#include <linux/err.h>
?#include <linux/clk.h>

arch/arm/mach-exynos/pm.c +4 -0

+#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>

arch/arm/mach-exynos/pm.c +0 -0

@@ -37,6 +41,13 @@

?#include <mach/pm-core.h>
?#include <mach/pmu.h>

arch/arm/mach-exynos/pm.c +7 -0

+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, },

arch/arm/mach-exynos/pm.c +107 -0

@@ -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 = {

arch/arm/mach-exynos/pm.c static struct sysdev_driver exynos4_pm_driver = { · line 409

Quoted 4 lines

arch/arm/mach-exynos/pm.c +4 -0

+ ? ? ? ? ? ? .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

Quoted 5 lines

Quoted 5 lines

arch/arm/mach-exynos/pm.c +5 -0

+
+static struct exynos4_pm_domain exynos4_pd_gps = {
+     .base = (void __iomem *)S5P_PMU_GPS_CONF,
+     .name = "pd-gps",
+     .pd = {

Quoted 8 lines

arch/arm/mach-exynos/pm.c static struct sysdev_driver exynos4_pm_driver = { · line 355

Quoted 4 lines

arch/arm/mach-exynos/pm.c +4 -0

+             .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.

Quoted 3 lines

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.

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 <[email protected]>

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
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(-)

arch/arm/mach-exynos/pm.c +33 -0

+
+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);

Hi Thomas,

Quoted 18 lines · 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 <[email protected]>

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
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.

Hi Thomas,

thank you for clarifying.

Quoted 18 lines · Thomas Abraham

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>;
? ?};

Quoted 5 lines

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.

Quoted 2 lines

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

Thanks,
Sylwester

Hi Sylwester,

Quoted 36 lines · Sylwester Nawrocki

Hi Thomas,

thank you for clarifying.

Quoted 18 lines · Thomas Abraham

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>;
? ?};

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 40 lines · Thomas Abraham

Hi Sylwester,

Quoted 37 lines · Sylwester Nawrocki

Hi Thomas,

thank you for clarifying.

Quoted 18 lines · Thomas Abraham

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>;
? ?};

Quoted 5 lines

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.

Quoted 1 line

Quoted 1 line

Didn't you initially consider "sec" for instance ? Probably it is

already

Quoted 2 lines

Quoted 1 line

too late for changing that though.

Quoted 5 lines

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.

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().

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 <[email protected]>

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
 drivers/base/power/domain.c |   14 +++++++++++++-
 include/linux/pm_domain.h   |   13 +++++++++++--
 2 files changed, 24 insertions(+), 3 deletions(-)

drivers/base/power/domain.c +2 -1

index 92e6a90..e895dc9 100644
@@ -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__);
 

drivers/base/power/domain.c +1 -0

+
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 

include/linux/pm_domain.h +1 -0

index a03a0ad..2ce7cc4 100644
@@ -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 */

include/linux/pm_domain.h +2 -2

 };
 
 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);

Quoted 2 lines · Thomas Abraham

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?

Quoted 8 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Abraham

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.

Quoted 6 lines · Mark Brown

Quoted 4 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Abraham

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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

Hi Mark, Rafael,

Quoted 7 lines · Mark Brown

Quoted 6 lines · Rafael J. Wysocki

Quoted 4 lines · Thomas Abraham

Quoted 3 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 3 lines

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.

Quoted 1 line · Thomas Abraham

Hi Mark, Rafael,

Hi,

Quoted 17 lines · Rafael J. Wysocki

Quoted 7 lines · Mark Brown

Quoted 6 lines · Rafael J. Wysocki

Quoted 4 lines · Thomas Abraham

Quoted 3 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines

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?

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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

Hi Rafael,

Quoted 29 lines · Rafael J. Wysocki

Quoted 1 line · Thomas Abraham

Hi Mark, Rafael,

Hi,

Quoted 17 lines · Rafael J. Wysocki

Quoted 7 lines · Mark Brown

Quoted 6 lines · Rafael J. Wysocki

Quoted 4 lines · Thomas Abraham

Quoted 3 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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'.

Quoted 26 lines

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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'.

Quoted 4 lines

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

Hi,

Quoted 2 lines · Thomas Abraham

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?

Quoted 6 lines · Rafael J. Wysocki

Hi,

Quoted 2 lines · Thomas Abraham

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

Quoted 3 lines

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.