| Message ID | 1477049656-27535-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Fri, Oct 21, 2016 at 1:34 PM, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > 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 <geert+renesas@glider.be> > --- > "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(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index e023066e421547c5..d94d6a4b9b527108 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -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. > + IS_ENABLED(CONFIG_64BIT)) > return ret; > > /* > -- Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 21, 2016 at 02:29:05PM +0200, Rafael J. Wysocki wrote: > On Fri, Oct 21, 2016 at 1:34 PM, Geert Uytterhoeven > <geert+renesas@glider.be> wrote: > > 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 <geert+renesas@glider.be> > > --- > > "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(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index e023066e421547c5..d94d6a4b9b527108 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 21, 2016 at 02:29:05PM +0200, Rafael J. Wysocki wrote: > On Fri, Oct 21, 2016 at 1:34 PM, Geert Uytterhoeven > <geert+renesas@glider.be> wrote: > > 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 <geert+renesas@glider.be> > > --- > > "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(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index e023066e421547c5..d94d6a4b9b527108 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -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. Sorry for the noise, sending once again without bogus recipient added by mistake: 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2016 03:58 PM, Krzysztof Kozlowski wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Sylwester, On 10/21/2016 11:14 AM, Sylwester Nawrocki wrote: > On 10/21/2016 03:58 PM, Krzysztof Kozlowski wrote: >> 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,
Hi Javier, On Fri, Oct 21, 2016 at 4:18 PM, Javier Martinez Canillas <javier@osg.samsung.com> wrote: > On 10/21/2016 11:14 AM, Sylwester Nawrocki wrote: >> On 10/21/2016 03:58 PM, Krzysztof Kozlowski wrote: >>> 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. I'm happy to hear that! Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index e023066e421547c5..d94d6a4b9b527108 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -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; /*
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 <geert+renesas@glider.be> --- "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(-)