PM / Domains: Restrict "samsung, power-domain" checks to ARCH_EXYNOS

PATCH New Geert Uytterhoeven 6 participants 7 messages Oct 21, 2016

Subsystems (5)

Arm Samsung Exynos Driver core Suspend to RAM Hibernation +1 more subsystems

lore

Share & tools

lore

Geert Uytterhoeven

author

2016-10-21 11:34 UTC #0 lore raw

PATCH 1/1 PM / Domains: Restrict "samsung, power-domain" checks to ARCH_EXYNOS

Currently the generic PM Domain code code checks for the presence of both (generic) "power-domains" and (Samsung Exynos legacy) "samsung,power-domain" properties in all device tree nodes representing devices.

There are two issues with this:
1. This imposes a small boot-time penalty on all platforms using DT,
2. Platform-specific checks do not really belong in core framework code.

While moving the check from platform-agnostic code to Samsung-specific code is non-trivial, the runtime overhead can be restricted to kernels including support for 32-bit Samsung Exynos platforms.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
"samsung,power-domain" was only ever used in:
- arch/arm/boot/dts/exynos4415.dtsi: Unused?
- arch/arm/boot/dts/exynos3250.dtsi: CONFIG_ARCH_EXYNOS3
- arch/arm/boot/dts/exynos4.dtsi: CONFIG_ARCH_EXYNOS4
- arch/arm/boot/dts/exynos4x12.dtsi: CONFIG_ARCH_EXYNOS4 exynos4212.dtsi is unused?
- arch/arm/boot/dts/exynos5250.dtsi: CONFIG_ARCH_EXYNOS5
- arch/arm/boot/dts/exynos5420.dtsi: CONFIG_ARCH_EXYNOS5

---
 drivers/base/power/domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

index e023066e421547c5..d94d6a4b9b527108 100644
@@ -1853,7 +1853,8 @@ int genpd_dev_pm_attach(struct device *dev)
 	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
 					"#power-domain-cells", 0, &pd_args);
 	if (ret < 0) {
-		if (ret != -ENOENT)
+		if (ret != -ENOENT || !IS_ENABLED(CONFIG_ARCH_EXYNOS) ||
+		    IS_ENABLED(CONFIG_64BIT))
 			return ret;
 
 		/*

Signature · 1 line

1.9.1

Rafael J. Wysocki

maintainer

2016-10-21 12:29 UTC #1 lore raw

PATCH 1/1 PM / Domains: Restrict "samsung, power-domain" checks to ARCH_EXYNOS

Quoted 33 lines · Geert Uytterhoeven

Currently the generic PM Domain code code checks for the presence of both (generic) "power-domains" and (Samsung Exynos legacy) "samsung,power-domain" properties in all device tree nodes representing devices.
There are two issues with this:
1. This imposes a small boot-time penalty on all platforms using DT,
2. Platform-specific checks do not really belong in core framework code.
While moving the check from platform-agnostic code to Samsung-specific code is non-trivial, the runtime overhead can be restricted to kernels including support for 32-bit Samsung Exynos platforms.
Signed-off-by Geert Uytterhoeven <[email protected]>
---
"samsung,power-domain" was only ever used in:
- arch/arm/boot/dts/exynos4415.dtsi: Unused?
- arch/arm/boot/dts/exynos3250.dtsi: CONFIG_ARCH_EXYNOS3
- arch/arm/boot/dts/exynos4.dtsi: CONFIG_ARCH_EXYNOS4
- arch/arm/boot/dts/exynos4x12.dtsi: CONFIG_ARCH_EXYNOS4 exynos4212.dtsi is unused?
- arch/arm/boot/dts/exynos5250.dtsi: CONFIG_ARCH_EXYNOS5
- arch/arm/boot/dts/exynos5420.dtsi: CONFIG_ARCH_EXYNOS5
---
 drivers/base/power/domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/base/power/domain.c +0 -0
index e023066e421547c5..d94d6a4b9b527108 100644

drivers/base/power/domain.c int genpd_dev_pm_attach(struct device *dev) · line 1856

Quoted 5 lines

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

@@ -1853,7 +1853,8 @@ int genpd_dev_pm_attach(struct device *dev)
        ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
                                        "#power-domain-cells", 0, &pd_args);
        if (ret < 0) {
-               if (ret != -ENOENT)
+               if (ret != -ENOENT || !IS_ENABLED(CONFIG_ARCH_EXYNOS) ||

Please don't check things like CONFIG_ARCH_EXYNOS in the core.

If you need to put checks like that here, there is a design problem somewhere.

And imagine someone 5 years ahead from now looking at this code and wondering why on Earth the check is here.

drivers/base/power/domain.c int genpd_dev_pm_attach(struct device *dev)

Quoted 5 lines

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

+                   IS_ENABLED(CONFIG_64BIT))
                        return ret;
 
                /*
--

Thanks,
Rafael

Krzysztof Kozlowski

maintainer

2016-10-21 13:58 UTC #2 lore raw

PATCH 1/1 PM / Domains: Restrict "samsung,power-domain" checks to ARCH_EXYNOS

Quoted 43 lines · Rafael J. Wysocki

Quoted 33 lines · Geert Uytterhoeven
Currently the generic PM Domain code code checks for the presence of both (generic) "power-domains" and (Samsung Exynos legacy) "samsung,power-domain" properties in all device tree nodes representing devices.
There are two issues with this:
1. This imposes a small boot-time penalty on all platforms using DT,
2. Platform-specific checks do not really belong in core framework code.
While moving the check from platform-agnostic code to Samsung-specific code is non-trivial, the runtime overhead can be restricted to kernels including support for 32-bit Samsung Exynos platforms.
Signed-off-by Geert Uytterhoeven <[email protected]>
---
"samsung,power-domain" was only ever used in:
- arch/arm/boot/dts/exynos4415.dtsi: Unused?
- arch/arm/boot/dts/exynos3250.dtsi: CONFIG_ARCH_EXYNOS3
- arch/arm/boot/dts/exynos4.dtsi: CONFIG_ARCH_EXYNOS4
- arch/arm/boot/dts/exynos4x12.dtsi: CONFIG_ARCH_EXYNOS4 exynos4212.dtsi is unused?
- arch/arm/boot/dts/exynos5250.dtsi: CONFIG_ARCH_EXYNOS5
- arch/arm/boot/dts/exynos5420.dtsi: CONFIG_ARCH_EXYNOS5
---
 drivers/base/power/domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/base/power/domain.c +0 -0
index e023066e421547c5..d94d6a4b9b527108 100644
drivers/base/power/domain.c int genpd_dev_pm_attach(struct device *dev) · line 1856
Quoted 5 lines
drivers/base/power/domain.c +1 -1
@@ -1853,7 +1853,8 @@ int genpd_dev_pm_attach(struct device *dev)
        ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
                                        "#power-domain-cells", 0, &pd_args);
        if (ret < 0) {
-               if (ret != -ENOENT)
+               if (ret != -ENOENT || !IS_ENABLED(CONFIG_ARCH_EXYNOS) ||
Please don't check things like CONFIG_ARCH_EXYNOS in the core.

Quoted 4 lines

If you need to put checks like that here, there is a design problem somewhere.
And imagine someone 5 years ahead from now looking at this code and wondering why on Earth the check is here.

I don't find the argument of performance penalty such important but for the sake of design, the samsung-specific code could be moved to drivers/soc/samsung/pm_domains.c, called "legacy_pm_parse" and exported through a header. Thus with !ARCH_EXYNOS that would be 'static inline {}'. However that is not a nice solution - there will be still direct call to platform-specific code in the core. I am not sure if it is worth the effort.

The samsung,power-domain was made deprecated (although not explicitly) in January 2015 (0da658704136 ("ARM: dts: convert to generic power domain bindings for exynos DT")) so how about:
1. Printing a dev_warn() about usage of deprecated bindings.
2. Complete removal in January 2017?

Best regards,
Krzysztof

Sylwester Nawrocki

2016-10-21 14:14 UTC #3 lore raw

PATCH 1/1 PM / Domains: Restrict "samsung,power-domain" checks to ARCH_EXYNOS

Quoted 5 lines · Krzysztof Kozlowski

The samsung,power-domain was made deprecated (although not explicitly) in January 2015 (0da658704136 ("ARM: dts: convert to generic power domain bindings for exynos DT")) so how about:
1. Printing a dev_warn() about usage of deprecated bindings.
2. Complete removal in January 2017?

I doubt anyone will ever use new mainline kernel with older dts/dtb so IMHO it makes sense to queue a patch removing support for the deprecated compatible just now and don't bother with a warning.

Documentation/devicetree/bindings/display/exynos/exynos7-decon.txt arch/arm/boot/dts/exynos4415.dtsi
would just need to be updated in same release, I can prepare a patch for these files.

--
Thanks,
Sylwester

Javier Martinez Canillas

2016-10-21 14:18 UTC #4 lore raw

PATCH 1/1 PM / Domains: Restrict "samsung,power-domain" checks to ARCH_EXYNOS

Hello Sylwester,

Quoted 11 lines · Sylwester Nawrocki

Quoted 5 lines · Krzysztof Kozlowski
The samsung,power-domain was made deprecated (although not explicitly) in January 2015 (0da658704136 ("ARM: dts: convert to generic power domain bindings for exynos DT")) so how about:
1. Printing a dev_warn() about usage of deprecated bindings.
2. Complete removal in January 2017?
I doubt anyone will ever use new mainline kernel with older dts/dtb so IMHO it makes sense to queue a patch removing support for the deprecated compatible just now and don't bother with a warning.

FWIW I agree with you. I don't know of any Exynos machine that ships a DT as read-only. Even consumer devices like the Exynos Chromebooks use a FIT image (kernel + FDT bundled), so the DT can always be updated.

Removing the support for the deprecated property sound sensible to me.

Best regards,

Signature · 3 lines

Javier Martinez Canillas
Open Source Group
Samsung Research America

Geert Uytterhoeven

2016-10-21 14:39 UTC #5 lore raw

PATCH 1/1 PM / Domains: Restrict "samsung, power-domain" checks to ARCH_EXYNOS

Hi Javier,

Quoted 13 lines · Javier Martinez Canillas

Quoted 11 lines · Sylwester Nawrocki
Quoted 5 lines · Krzysztof Kozlowski
The samsung,power-domain was made deprecated (although not explicitly) in January 2015 (0da658704136 ("ARM: dts: convert to generic power domain bindings for exynos DT")) so how about:
1. Printing a dev_warn() about usage of deprecated bindings.
2. Complete removal in January 2017?
I doubt anyone will ever use new mainline kernel with older dts/dtb so IMHO it makes sense to queue a patch removing support for the deprecated compatible just now and don't bother with a warning.

Quoted 5 lines

FWIW I agree with you. I don't know of any Exynos machine that ships a DT as read-only. Even consumer devices like the Exynos Chromebooks use a FIT image (kernel + FDT bundled), so the DT can always be updated.
Removing the support for the deprecated property sound sensible to me.

I'm happy to hear that!

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Krzysztof Kozlowski

maintainer

2016-10-21 14:06 UTC #6 lore raw

PATCH 1/1 PM / Domains: Restrict "samsung,power-domain" checks to ARCH_EXYNOS

In reply to Rafael J. Wysocki

Quoted 43 lines · Rafael J. Wysocki

Quoted 33 lines · Geert Uytterhoeven
Currently the generic PM Domain code code checks for the presence of both (generic) "power-domains" and (Samsung Exynos legacy) "samsung,power-domain" properties in all device tree nodes representing devices.
There are two issues with this:
1. This imposes a small boot-time penalty on all platforms using DT,
2. Platform-specific checks do not really belong in core framework code.
While moving the check from platform-agnostic code to Samsung-specific code is non-trivial, the runtime overhead can be restricted to kernels including support for 32-bit Samsung Exynos platforms.
Signed-off-by Geert Uytterhoeven <[email protected]>
---
"samsung,power-domain" was only ever used in:
- arch/arm/boot/dts/exynos4415.dtsi: Unused?
- arch/arm/boot/dts/exynos3250.dtsi: CONFIG_ARCH_EXYNOS3
- arch/arm/boot/dts/exynos4.dtsi: CONFIG_ARCH_EXYNOS4
- arch/arm/boot/dts/exynos4x12.dtsi: CONFIG_ARCH_EXYNOS4 exynos4212.dtsi is unused?
- arch/arm/boot/dts/exynos5250.dtsi: CONFIG_ARCH_EXYNOS5
- arch/arm/boot/dts/exynos5420.dtsi: CONFIG_ARCH_EXYNOS5
---
 drivers/base/power/domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/base/power/domain.c +0 -0
index e023066e421547c5..d94d6a4b9b527108 100644
drivers/base/power/domain.c int genpd_dev_pm_attach(struct device *dev) · line 1856
Quoted 5 lines
drivers/base/power/domain.c +1 -1
@@ -1853,7 +1853,8 @@ int genpd_dev_pm_attach(struct device *dev)
        ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
                                        "#power-domain-cells", 0, &pd_args);
        if (ret < 0) {
-               if (ret != -ENOENT)
+               if (ret != -ENOENT || !IS_ENABLED(CONFIG_ARCH_EXYNOS) ||
Please don't check things like CONFIG_ARCH_EXYNOS in the core.

Quoted 4 lines

If you need to put checks like that here, there is a design problem somewhere.
And imagine someone 5 years ahead from now looking at this code and wondering why on Earth the check is here.

Sorry for the noise, sending once again without bogus recipient added by mistake:

Best regards,
Krzysztof