Re: [PATCH] ARM: EXYNOS4: Support for generic I/O power domains on EXYNOS4210

[Chanwoo Choi <cw00.choi@samsung.com>]

Hello,

Sylwester Nawrocki wrote:
> Hello,
> 
> On 06/24/2011 12:06 PM, Chanwoo Choi wrote:
>> Use the generic power domains support to implement support
>> for power domain on EXYNOS4210.
>>
>> This patch is based on your "pm-domains" branch.(v7)
>>
>> Signed-off-by: Chanwoo Choi<cw00.choi@samsung.com>
>> Signed-off-by: Myungjoo Ham<myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin.park<kyungmin.park@samsung.com>
>> ---
>>   arch/arm/mach-exynos4/Kconfig                      |    1 +
>>   arch/arm/mach-exynos4/Makefile                     |    1 +
>>   arch/arm/mach-exynos4/include/mach/pm-exynos4210.h |   78 ++++++++++++
>>   arch/arm/mach-exynos4/mach-nuri.c                  |   20 +++
>>   arch/arm/mach-exynos4/pm-exynos4210.c              |  131 ++++++++++++++++++++
>>   5 files changed, 231 insertions(+), 0 deletions(-)
>>   create mode 100644 arch/arm/mach-exynos4/include/mach/pm-exynos4210.h
>>   create mode 100644 arch/arm/mach-exynos4/pm-exynos4210.c
>>
> ...
> 
>> diff --git a/arch/arm/mach-exynos4/include/mach/pm-exynos4210.h b/arch/arm/mach-exynos4/include/mach/pm-exynos4210.h
>> new file mode 100644
>> index 0000000..fa1c7a0
>> --- /dev/null
>> +++ b/arch/arm/mach-exynos4/include/mach/pm-exynos4210.h
>> @@ -0,0 +1,78 @@
>> +/* linux/arch/arm/mach-exynos4/include/mach/pm-exynos4.h
>> + *
>> + * Exynos4 Power management support
>> + *
>> + * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
>> + *		http://www.samsung.com
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef __PM_EXYNOS4210_H__
>> +#define __PM_EXYNOS4210_H__
>> +
>> +#include<linux/pm_domain.h>
>> +
>> +struct platform_device;
>> +
>> +struct exynos4210_pm_domain {
>> +	struct generic_pm_domain genpd;
>> +
>> +	const char *name;
>> +	void __iomem *base;
>> +	int boot_on;
>> +};
>> +
>> +static inline struct exynos4210_pm_domain *to_exynos4210_pd(
>> +		struct generic_pm_domain *pd)
>> +{
>> +	return container_of(pd, struct exynos4210_pm_domain, genpd);
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +enum exynos4210_pd_data {
>> +	EXYNOS4210_PD_MFC,
>> +	EXYNOS4210_PD_G3D,
>> +	EXYNOS4210_PD_LCD0,
>> +	EXYNOS4210_PD_LCD1,
>> +	EXYNOS4210_PD_TV,
>> +	EXYNOS4210_PD_CAM,
>> +	EXYNOS4210_PD_GPS,
>> +	EXYNOS4210_PD_MAX_NUM
>> +};
>> +
>> +extern struct exynos4210_pm_domain exynos4210_pd_list[EXYNOS4210_PD_MAX_NUM];
> 
> Could this array be made static in ../pm-exynos4210.c ?
> It looks like all functions using it are there. Plus you have already
> enum exynos4210_pd_data to identify each power domain.

It is poosible to define static arrary in ../pm-exynos4210.c,
but it could be defined as non-static to keep the same form 
with the patch using generic I/O power domain support :
http://git.kernel.org/?p=linux/kernel/git/rafael/suspend-2.6.git;a=commit;h=9b3c17cd4555612ff852b7c1ce9583d48d9b0c31

And I will modify that use previous definition of enum exynos4210_pd_data
to identify each power domain.

> 
>> +#define EXYNOS4210_MFC		(&exynos4210_pd_list[EXYNOS4210_PD_MFC])
>> +#define EXYNOS4210_G3D		(&exynos4210_pd_list[EXYNOS4210_PD_G3D])
>> +#define EXYNOS4210_LCD0		(&exynos4210_pd_list[EXYNOS4210_PD_LCD0])
>> +#define EXYNOS4210_LCD1		(&exynos4210_pd_list[EXYNOS4210_PD_LCD1])
>> +#define EXYNOS4210_TV		(&exynos4210_pd_list[EXYNOS4210_PD_TV])
>> +#define EXYNOS4210_CAM		(&exynos4210_pd_list[EXYNOS4210_PD_CAM])
>> +#define EXYNOS4210_GPS		(&exynos4210_pd_list[EXYNOS4210_PD_GPS])
>> +
>> +extern int exynos4210_pd_init(struct exynos4210_pm_domain *domain);
>> +extern int exynos4210_add_device_to_domain(struct exynos4210_pm_domain *domain,
>> +				struct platform_device *pdev);
> 
> Is there any advantage of having these functions marked as "extern" ?
> 
I will remove "extern" keyword.

>> +#else
>> +#define EXYNOS4210_MFC		NULL
>> +#define EXYNOS4210_G3D		NULL
>> +#define EXYNOS4210_LCD0		NULL
>> +#define EXYNOS4210_LCD1		NULL
>> +#define EXYNOS4210_TV		NULL
>> +#define EXYNOS4210_CAM		NULL
>> +#define EXYNOS4210_GPS		NULL
>> +
>> +extern int exynos4210_pd_init(struct exynos4210_pm_domain *domain)
> 
> Shouldn't it be "static inline" instead ?

My mistake. I will fix it.

>> +{
>> +	return 0;
>> +}
>> +extern int exynos4_add_device_to_domain(struct exynos4210_pm_domain *domain,
>> +				struct platform_device *pdev)
> 
> Ditto.
> 
I will fix it.
> ...
>> diff --git a/arch/arm/mach-exynos4/mach-nuri.c b/arch/arm/mach-exynos4/mach-nuri.c
>> index 642702b..3e5c42c 100644
>> --- a/arch/arm/mach-exynos4/mach-nuri.c
>> +++ b/arch/arm/mach-exynos4/mach-nuri.c
>> @@ -37,6 +37,7 @@
>>   #include<plat/iic.h>
>>
>>   #include<mach/map.h>
>> +#include<mach/pm-exynos4210.h>
>>
>>   /* Following are default values for UCON, ULCON and UFCON UART registers */
>>   #define NURI_UCON_DEFAULT	(S3C2410_UCON_TXILEVEL |	\
>> @@ -376,6 +377,23 @@ static struct platform_device *nuri_devices[] __initdata = {
>>   	&nuri_backlight_device,
>>   };
>>
>> +static void __init nuri_power_domain_init(void)
>> +{
>> +	int i;
>> +
>> +	for (i = 0 ; i<  EXYNOS4210_PD_MAX_NUM ; i++)
>> +		exynos4210_pd_init(&exynos4210_pd_list[i]);
>> +
>> +	/* Add device to power-domain */
>> +	exynos4210_add_device_to_domain(EXYNOS4210_MFC, NULL);
>> +	exynos4210_add_device_to_domain(EXYNOS4210_G3D, NULL);
>> +	exynos4210_add_device_to_domain(EXYNOS4210_LCD0,&nuri_lcd_device);
>> +	exynos4210_add_device_to_domain(EXYNOS4210_LCD1, NULL);
>> +	exynos4210_add_device_to_domain(EXYNOS4210_TV, NULL);
>> +	exynos4210_add_device_to_domain(EXYNOS4210_CAM, NULL);
>> +	exynos4210_add_device_to_domain(EXYNOS4210_GPS, NULL);
>> +}
>> +
>>   static void __init nuri_map_io(void)
>>   {
>>   	s5p_init_io(NULL, 0, S5P_VA_CHIPID);
>> @@ -398,6 +416,8 @@ static void __init nuri_machine_init(void)
>>
>>   	/* Last */
>>   	platform_add_devices(nuri_devices, ARRAY_SIZE(nuri_devices));
>> +
>> +	nuri_power_domain_init();
>>   }
>>
>>   MACHINE_START(NURI, "NURI")
>> diff --git a/arch/arm/mach-exynos4/pm-exynos4210.c b/arch/arm/mach-exynos4/pm-exynos4210.c
>> new file mode 100644
>> index 0000000..8e95d49
>> --- /dev/null
>> +++ b/arch/arm/mach-exynos4/pm-exynos4210.c
>> @@ -0,0 +1,131 @@
>> +/* linux/arch/arm/mach-exynos4/pm-exynos4210.c
>> + *
> ...
>> +
>> +struct exynos4210_pm_domain exynos4210_pd_list[] = {
>> +	{
>> +		.name		= "PD_MFC",
>> +		.base		= S5P_PMU_MFC_CONF,
>> +		.boot_on	= 0,
>> +	}, {
>> +		.name		= "PD_G3D",
>> +		.base		= S5P_PMU_G3D_CONF,
>> +		.boot_on	= 0,
>> +	}, {
> ...
>> +
>> +int exynos4210_pd_init(struct exynos4210_pm_domain *domain)
>> +{
>> +	struct generic_pm_domain *genpd =&domain->genpd;
>> +
>> +	pm_genpd_init(genpd, NULL, false);
>> +
>> +	genpd->stop_device = pm_runtime_clk_suspend;
>> +	genpd->start_device = pm_runtime_clk_resume;
> 
> Have you considered handling the S5P_CLKGATE_BLOCK register to make
> sure the clock domains are enabled ? AFAICS it is not properly handled
> by current clock API on mach-exynos4. Thus if bootloader clears
> S5P_CLKGATE_BLOCK register clocks remain globally masked.
> 
> This issue have been attempted to be resolved in this patch:
> http://git.infradead.org/users/kmpark/linux-2.6-samsung/commit/39a81876d034dcbdc2a4c4c4b847b3b49e38870c
> 
> While at here, it might be good to resolve this either on power domains
> or the clock API level.
> 

I know this issue about your commnt. This patch didn't consider the clock domain
because refer to the previous function which enable/disable only the power state
of power domain. As your comment, I will adapt that handling S5P_CLKGATE_BLOCK to 
control the clock domains.

>> +	genpd->power_off = pd_power_down;
>> +	genpd->power_on = pd_power_up;
>> +
>> +	if (domain->boot_on)
>> +		pd_power_up(genpd);
>> +
>> +	return 0;
>> +}
>> +
>> +int exynos4210_add_device_to_domain(struct exynos4210_pm_domain *domain,
>> +				struct platform_device *pdev)
>> +{
>> +	struct device *dev =&pdev->dev;
>> +	struct generic_pm_domain *genpd =&domain->genpd;
>> +
>> +	if (pdev != NULL)
>> +		return pm_genpd_add_device(genpd, dev);
>> +	else
>> +		return -EINVAL;
> 
> You could do instead:
> 
> +	if (pdev)
> +		return pm_genpd_add_device(genpd, dev);
> +
> +	return -EINVAL;
> 
Ok, I will modify it.
Thanks for your comment.

Regards,
Chanwoo Choi