lkml.org 
[lkml]   [2011]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] ARM: EXYNOS4: Support for generic I/O power domains on EXYNOS4210
    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.

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

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

    > +{
    > + return 0;
    > +}
    > +extern int exynos4_add_device_to_domain(struct exynos4210_pm_domain *domain,
    > + struct platform_device *pdev)

    Ditto.

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

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

    > +}

    --
    Regards,
    Sylwester


    \
     
     \ /
      Last update: 2011-06-26 19:39    [W:0.039 / U:0.056 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site