Re: [PATCH v2 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure
Dear Mr. Park.
On 9 January 2012 05:57, Kyungmin Park wrote:
[...]
>> + * Exynos4 specific wrapper around the generic power domain
>> + */
>> +struct exynos4_pm_domain {
>> + void __iomem *base;
>> + char const *name;
>> + bool is_off;
>> + struct generic_pm_domain pd;
>> +};
> Even though you tested it at exynos4, we already know exynos5 will be used
> soon.
> so start to use the exynos prefix if possible.
Ok. I will change exynos4 to exynos.
[...]
>> +
>> +static __init int exynos4_pm_init_power_domain(void)
>> +{
>> + int idx;
>> + struct device_node *np;
> The np is only used at CONFIG_ON.
Ok. I will rework this.
>> +
>> +#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: failed to "
>> + "allocate memory for domain\n");
>> + return -ENOMEM;
>> + }
>> +
>> + 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 0;
>> +#endif /* CONFIG_OF */
>> +
>> + no_dt:
> You should add the "#endif /* CONFIG_OF */ since no_dt is used at
> CONFIG_OF case.
Ok.
>> + 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);
>> +
[...]
>> +#ifdef CONFIG_S5P_DEV_CSIS0
>> + if (pm_genpd_add_device(&exynos4_pd_cam.pd,
>> &s5p_device_mipi_csis0.dev))
>> + pr_info("error in adding csis0 to cam power domain\n");
>> +#endif
> Also need to add CSIS1 as Sylwester mentioned.
Ok. I will add CSIS1.
Thanks for your review.
Regards,
Thomas.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure
Hi Sylwester.
On 7 January 2012 20:14, Sylwester Nawrocki wrote:
[...]
>> diff --git a/arch/arm/mach-exynos/pm_domains.c
>> b/arch/arm/mach-exynos/pm_domains.c
>> new file mode 100644
>> index 000..95a7c55
>> --- /dev/null
>> +++ b/arch/arm/mach-exynos/pm_domains.c
>> @@ -0,0 +1,183 @@
>> +/*
>> + * Exynos4 Generic power domain support.
>> + *
>> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
>
> 2012 ?
Ok. I will change this.
>
>> + * http://www.samsung.com
[...]
>> +static int exynos4_pd_power(struct generic_pm_domain *domain, bool power_on)
>> +{
>> + struct exynos4_pm_domain *pd;
>> + void __iomem *base;
>> + u32 timeout, pwr;
>> + char *op;
>> +
>> + pd = container_of(domain, struct exynos4_pm_domain, pd);
>> + base = pd->base;
>> +
>> + pwr = (power_on) ? S5P_INT_LOCAL_PWR_EN : 0;
>
> Is there any value in parentheses around 'power_on' ?
No. I will remove the parentheses around 'power_on'.
>
>> + __raw_writel(pwr, base);
>> +
>> + /* Wait max 1ms */
>> + timeout = 10;
>> +
>> + while ((__raw_readl(base + 0x4)& S5P_INT_LOCAL_PWR_EN) != pwr) {
>> + if (!timeout) {
>> + op = (power_on) ? "enable" : "disable";
>> + pr_err("Power domain %s %s failed\n", op,
>> domain->name);
>
> How about just:
> pr_err("%s power domain state change (%d) failed\n",
> domain->name, power_on);
> ?
>From the message it will not be clear which state change failed. The
state (enable/disable) would be helpful.
>> + return -ETIMEDOUT;
>> + }
>> + timeout--;
>> + cpu_relax();
>
> Does cpu_relax() make any difference here ?
I need to check on this.
>
>> + usleep_range(80, 100);
>> + }
>> + return 0;
>> +}
>> +
[...]
>> + pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>> + if (!pd) {
>> + pr_err("exynos4_pm_init_power_domain: failed to "
>> + "allocate memory for domain\n");
>
> nit: what about:
> pr_err("%s: failed to allocate memory for domain\n",
> __func__);
> ?
Yes. This is better. I will change this.
>> + return -ENOMEM;
>> + }
>> +
[...]
>> +#ifdef CONFIG_S5P_DEV_CSIS0
>> + if (pm_genpd_add_device(&exynos4_pd_cam.pd,&s5p_device_mipi_csis0.dev))
>> + pr_info("error in adding csis0 to cam power domain\n");
>> +#endif
>
> Could you add CSIS1 as well ? Some boards will be using both MIPI-CSI
> receivers.
Ok. I will add CSIS1 here.
Thanks for your review.
Regards,
Thomas.
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure
On 1/7/12, Thomas Abraham wrote:
> Add support for generic power domain for Exynos4 platforms and remove the
> Samsung specific power domain control for Exynos4.
>
> 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: Rafael J. Wysocki
> Cc: Kukjin Kim
> Cc: Kyungmin Park
> Cc: Rob Herring
> Cc: Grant Likely
> Signed-off-by: Thomas Abraham
> ---
> This patch is mainly derived from Mark Brown's work on generic power domain
> support for s3c64xx platforms.
>
> arch/arm/mach-exynos/Kconfig | 10 +--
> arch/arm/mach-exynos/Makefile |2 +-
> arch/arm/mach-exynos/dev-pd.c | 139 -
> arch/arm/mach-exynos/mach-nuri.c | 11 --
> arch/arm/mach-exynos/mach-origen.c | 14 --
> arch/arm/mach-exynos/mach-smdkv310.c | 12 --
> arch/arm/mach-exynos/mach-universal_c210.c | 17 ---
> arch/arm/mach-exynos/pm_domains.c | 183
>
> 8 files changed, 185 insertions(+), 203 deletions(-)
> delete mode 100644 arch/arm/mach-exynos/dev-pd.c
> create mode 100644 arch/arm/mach-exynos/pm_domains.c
>
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index e931924..5dec134 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -32,6 +32,7 @@ config CPU_EXYNOS4210
> select ARM_CPU_SUSPEND if PM
> select S5P_PM if PM
> select S5P_SLEEP if PM
> + select PM_GENERIC_DOMAINS
> help
> Enable EXYNOS4210 CPU support
>
> @@ -72,11 +73,6 @@ config EXYNOS4_SETUP_FIMD0
> help
> Common setup code for FIMD0.
>
> -config EXYNOS4_DEV_PD
> - bool
> - help
> - Compile in platform device definitions for Power Domain
> -
> config EXYNOS4_DEV_SYSMMU
> bool
> help
> @@ -194,7 +190,6 @@ config MACH_SMDKV310
> select EXYNOS4_DEV_AHCI
> select SAMSUNG_DEV_KEYPAD
> select EXYNOS4_DEV_DMA
> - select EXYNOS4_DEV_PD
> select SAMSUNG_DEV_PWM
> select EXYNOS4_DEV_USB_OHCI
> select EXYNOS4_DEV_SYSMMU
> @@ -243,7 +238,6 @@ config MACH_UNIVERSAL_C210
> select S5P_DEV_ONENAND
> select S5P_DEV_TV
> select EXYNOS4_DEV_DMA
> - select EXYNOS4_DEV_PD
> select EXYNOS4_SETUP_FIMD0
> select EXYNOS4_SETUP_I2C1
> select EXYNOS4_SETUP_I2C3
> @@ -277,7 +271,6 @@ config MACH_NURI
> select S5P_DEV_USB_EHCI
> select S5P_SETUP_MIPIPHY
> select EXYNOS4_DEV_DMA
> - select EXYNOS4_DEV_PD
> select EXYNOS4_SETUP_FIMC
> select EXYNOS4_SETUP_FIMD0
> select EXYNOS4_SETUP_I2C1
> @@ -310,7 +303,6 @@ config MACH_ORIGEN
> select SAMSUNG_DEV_BACKLIGHT
> select SAMSUNG_DEV_PWM
> select EXYNOS4_DEV_DMA
> - select EXYNOS4_DEV_PD
> select EXYNOS4_DEV_USB_OHCI
> select EXYNOS4_SETUP_FIMD0
> select EXYNOS4_SETUP_SDHCI
> diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
> index db527ab..b7e4eca 100644
> --- a/arch/arm/mach-exynos/Makefile
> +++ b/arch/arm/mach-exynos/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_ARCH_EXYNOS4) += irq-eint.o pmu.o
> obj-$(CONFIG_CPU_EXYNOS4210) += clock-exynos4210.o
> obj-$(CONFIG_SOC_EXYNOS4212) += clock-exynos4212.o
> obj-$(CONFIG_PM) += pm.o
> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
> obj-$(CONFIG_CPU_IDLE) += cpuidle.o
>
> obj-$(CONFIG_SMP)+= platsmp.o headsmp.o
> @@ -43,7 +44,6 @@ obj-$(CONFIG_MACH_EXYNOS4_DT) +=
> mach-exynos4-dt.o
>
> obj-$(CONFIG_ARCH_EXYNOS4) += dev-audio.o
> obj-$(CONFIG_EXYNOS4_DEV_AHCI) += dev-ahci.o
> -obj-$(CONFIG_EXYNOS4_DEV_PD) += dev-pd.o
> obj-$(CONFIG_EXYNOS4_DEV_SYSMMU) += dev-sysmmu.o
> obj-$(CONFIG_EXYNOS4_DEV_DWMCI) += dev-dwmci.o
> obj-$(CONFIG_EXYNOS4_DEV_DMA)+= dma.o
> diff --git a/arch/arm/mach-exynos/dev-pd.c b/arch/arm/mach-exynos/dev-pd.c
> deleted file mode 100644
> index 3273f25..000
> --- a/arch/arm/mach-exynos/dev-pd.c
> +++ /dev/null
> @@ -1,139 +0,0 @@
> -/* linux/arch/arm/mach-exynos4/dev-pd.c
> - *
> - * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
> - * http://www.samsung.com
> - *
> - * EXYNOS4 - Power Domain support
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> -*/
> -
> -#include
> -#include
> -#include
> -#include
> -
> -#include
> -
> -#include
> -
> -static int exynos4_pd_enable(struct device *dev)
> -{
> - struct samsung_pd_info *pdata = dev->platform_data;
> - u32 timeout;
> -
> - __raw_writel(S5P_INT_LOCAL_PWR_EN,
Re: [PATCH v2 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure
Hi Thomas,
thank you for working on this. I have a few comments below..
On 01/07/2012 11:10 AM, Thomas Abraham wrote:
> Add support for generic power domain for Exynos4 platforms and remove the
> Samsung specific power domain control for Exynos4.
>
> 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: Rafael J. Wysocki
> Cc: Kukjin Kim
> Cc: Kyungmin Park
> Cc: Rob Herring
> Cc: Grant Likely
> Signed-off-by: Thomas Abraham
> ---
> This patch is mainly derived from Mark Brown's work on generic power domain
> support for s3c64xx platforms.
>
> arch/arm/mach-exynos/Kconfig | 10 +--
> arch/arm/mach-exynos/Makefile |2 +-
> arch/arm/mach-exynos/dev-pd.c | 139 -
> arch/arm/mach-exynos/mach-nuri.c | 11 --
> arch/arm/mach-exynos/mach-origen.c | 14 --
> arch/arm/mach-exynos/mach-smdkv310.c | 12 --
> arch/arm/mach-exynos/mach-universal_c210.c | 17 ---
> arch/arm/mach-exynos/pm_domains.c | 183
>
> 8 files changed, 185 insertions(+), 203 deletions(-)
> delete mode 100644 arch/arm/mach-exynos/dev-pd.c
> create mode 100644 arch/arm/mach-exynos/pm_domains.c
>
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index e931924..5dec134 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -32,6 +32,7 @@ config CPU_EXYNOS4210
> select ARM_CPU_SUSPEND if PM
> select S5P_PM if PM
> select S5P_SLEEP if PM
> + select PM_GENERIC_DOMAINS
> help
> Enable EXYNOS4210 CPU support
>
...
> diff --git a/arch/arm/mach-exynos/pm_domains.c
> b/arch/arm/mach-exynos/pm_domains.c
> new file mode 100644
> index 000..95a7c55
> --- /dev/null
> +++ b/arch/arm/mach-exynos/pm_domains.c
> @@ -0,0 +1,183 @@
> +/*
> + * Exynos4 Generic power domain support.
> + *
> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
2012 ?
> + * http://www.samsung.com
> + *
> + * Implementation of Exynos4 specific power domain control which is used in
> + * conjunction with runtime-pm. Support for both device-tree and
> non-device-tree
> + * based power domain support is included.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +/*
> + * Exynos4 specific wrapper around the generic power domain
> + */
> +struct exynos4_pm_domain {
> + void __iomem *base;
> + char const *name;
> + bool is_off;
> + struct generic_pm_domain pd;
> +};
> +
> +static int exynos4_pd_power(struct generic_pm_domain *domain, bool power_on)
> +{
> + struct exynos4_pm_domain *pd;
> + void __iomem *base;
> + u32 timeout, pwr;
> + char *op;
> +
> + pd = container_of(domain, struct exynos4_pm_domain, pd);
> + base = pd->base;
> +
> + pwr = (power_on) ? S5P_INT_LOCAL_PWR_EN : 0;
Is there any value in parentheses around 'power_on' ?
> + __raw_writel(pwr, base);
> +
> + /* Wait max 1ms */
> + timeout = 10;
> +
> + while ((__raw_readl(base + 0x4)& S5P_INT_LOCAL_PWR_EN) != pwr) {
> + if (!timeout) {
> + op = (power_on) ? "enable" : "disable";
> + pr_err("Power domain %s %s failed\n", op, domain->name);
How about just:
pr_err("%s power domain state change (%d) failed\n",
domain->name, power_on);
?
> + return -ETIMEDOUT;
> + }
> + timeout--;
> + cpu_relax();
Does cpu_relax() make any difference here ?
> + usleep_range(80, 100);
> + }
> + return 0;
> +}
> +
> +static int exynos4_pd_power_on(struct generic_pm_domain *domain)
> +{
> + return exynos4_pd_power(domain, true);
> +}
> +
> +static int exynos4_pd_power_off(struct generic_pm_domain *domain)
> +{
> + return exynos4_pd_power(domain, false);
> +}
> +
> +
> +#define EXYNOS4_GPD(PD, BASE, NAME) \
> +static struct exynos4_pm_domain PD = { \
> + .base = (void __iomem *)BASE, \
> + .name = NAME, \
> + .pd = { \
> + .power_off = exynos4_pd_power_off, \
> + .power_on = exynos4_pd_power_on,\
> + }, \
> +}
> +
> +EXYNOS4_GPD(exynos4_pd_mfc, S5P_PMU_MFC_CONF, "pd-mfc");
> +EXYNOS4_GPD(exynos4_pd_g3d, S5P_PMU_G3D_CONF, "pd-g3d");
> +EXYNOS4_GPD(exynos4_pd_lcd0, S5P_PMU_LCD0
