Message ID | 1314080152-30503-3-git-send-email-Baohua.Song@csr.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Barry, Rongjun, A couple of minor nits inline. Jamie On Mon, Aug 22, 2011 at 11:15:50PM -0700, Barry Song wrote: > From: Rongjun Ying <rongjun.ying@csr.com> > > Signed-off-by: Rongjun Ying <rongjun.ying@csr.com> > Signed-off-by: Barry Song <Baohua.Song@csr.com> > --- > -v2: > delete redundant ARM mode registers save/restore > use generic cpu suspend > move l2 cache suspend/resume codes out > make memc become a device driver > > arch/arm/mach-prima2/pm.c | 158 ++++++++++++++++++++++++++++++++++++++++++ > arch/arm/mach-prima2/pm.h | 31 ++++++++ > arch/arm/mach-prima2/sleep.S | 61 ++++++++++++++++ > 3 files changed, 250 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/mach-prima2/pm.c > create mode 100644 arch/arm/mach-prima2/pm.h > create mode 100644 arch/arm/mach-prima2/sleep.S > > diff --git a/arch/arm/mach-prima2/pm.c b/arch/arm/mach-prima2/pm.c > new file mode 100644 > index 0000000..75167df > --- /dev/null > +++ b/arch/arm/mach-prima2/pm.c > @@ -0,0 +1,158 @@ > +/* > + * power management entry for CSR SiRFprimaII > + * > + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. > + * > + * Licensed under GPLv2 or later. > + */ > + > +#include <linux/kernel.h> > +#include <linux/suspend.h> > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/of_platform.h> > +#include <linux/io.h> > +#include <linux/rtc/sirfsoc_rtciobrg.h> > +#include <asm/suspend.h> > + > +#include "pm.h" > + > +static u32 sirfsoc_pwrc_base; > +void __iomem *sirfsoc_memc_base; > + > +static void sirfsoc_set_wakeup_source(void) > +{ > + u32 pwr_trigger_en_reg; > + pwr_trigger_en_reg = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + > + SIRFSOC_PWRC_TRIGGER_EN); > +#define X_ON_KEY_B (1 << 0) > + sirfsoc_rtc_iobrg_writel(pwr_trigger_en_reg | X_ON_KEY_B, > + sirfsoc_pwrc_base + SIRFSOC_PWRC_TRIGGER_EN); > +} > + > +static void sirfsoc_set_sleep_mode(u32 mode) > +{ > + u32 sleep_mode = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + > + SIRFSOC_PWRC_PDN_CTRL); > + sleep_mode &= ~(SIRFSOC_SLEEP_MODE_MASK << 1); > + sleep_mode |= ((mode << 1)); > + sirfsoc_rtc_iobrg_writel(sleep_mode, sirfsoc_pwrc_base + > + SIRFSOC_PWRC_PDN_CTRL); > +} > + > +int sirfsoc_pre_suspend_power_off(void) > +{ > + u32 wakeup_entry = virt_to_phys(cpu_resume); > + > + sirfsoc_rtc_iobrg_writel(wakeup_entry, > + SIRFSOC_PWRC_SCRATCH_PAD1); > + > + sirfsoc_set_wakeup_source(); > + > + sirfsoc_set_sleep_mode(SIRFSOC_DEEP_SLEEP_MODE); > + > + return 0; > +} > + > +static void sirfsoc_save_register(u32 *ptr) > +{ > + /* todo: save necessary system registers here */ > +} > + > +static void sirfsoc_restore_regs(u32 *ptr) > +{ > + /* todo: restore saved system registers here */ > +} > + > +static int sirfsoc_pm_enter(suspend_state_t state) > +{ > + u32 *saved_regs; > + > + switch (state) { > + case PM_SUSPEND_MEM: > + saved_regs = kmalloc(1024, GFP_ATOMIC); > + if (!saved_regs) > + return -ENOMEM; > + > + sirfsoc_save_register(saved_regs); > + > + /* go zzz */ > + cpu_suspend(0, sirfsoc_finish_suspend); > + > + sirfsoc_restore_regs(saved_regs); > + kfree(saved_regs); > + break; > + default: > + return -EINVAL; > + } > + return 0; > +} > + > +static const struct platform_suspend_ops sirfsoc_pm_ops = { > + .enter = sirfsoc_pm_enter, > + .valid = suspend_valid_only_mem, > +}; > + > +static int __init sirfsoc_pm_init(void) > +{ > + suspend_set_ops(&sirfsoc_pm_ops); > + return 0; > +} > +late_initcall(sirfsoc_pm_init); > + > +static struct of_device_id pwrc_ids[] = { > + { .compatible = "sirf,prima2-pwrc" }, Missing a sentinel terminator and const? > +}; > + > +static int __init sirfsoc_of_pwrc_init(void) > +{ > + struct device_node *np; > + const __be32 *addrp; > + > + np = of_find_matching_node(NULL, pwrc_ids); > + if (!np) > + panic("unable to find compatible pwrc node in dtb\n"); > + > + addrp = of_get_property(np, "reg", NULL); > + if (!addrp) > + panic("unable to find base address of pwrc node in dtb\n"); > + > + sirfsoc_pwrc_base = be32_to_cpup(addrp); > + > + of_node_put(np); > + > + return 0; > +} > +postcore_initcall(sirfsoc_of_pwrc_init); > + > +static struct of_device_id memc_ids[] = { > + { .compatible = "sirf,prima2-memc" }, and here? > +}; > + > +static int __devinit sirfsoc_memc_probe(struct platform_device *op) > +{ > + struct device_node *np = op->dev.of_node; > + > + sirfsoc_memc_base = of_iomap(np, 0); > + if (!sirfsoc_memc_base) > + panic("unable to map memc registers\n"); > + > + return 0; > +} > + > +static struct platform_driver sirfsoc_memc_driver = { > + .probe = sirfsoc_memc_probe, > + .driver = { > + .name = "sirfsoc-memc", > + .owner = THIS_MODULE, > + .of_match_table = memc_ids, > + }, > +}; > + > +static int __init sirfsoc_memc_init(void) > +{ > + return platform_driver_register(&sirfsoc_memc_driver); > +} > +postcore_initcall(sirfsoc_memc_init); > diff --git a/arch/arm/mach-prima2/pm.h b/arch/arm/mach-prima2/pm.h > new file mode 100644 > index 0000000..fe2028b > --- /dev/null > +++ b/arch/arm/mach-prima2/pm.h > @@ -0,0 +1,31 @@ > +/* > + * arch/arm/mach-prima2/pm.h > + * > + * Copyright (C) 2011 CSR > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#ifndef _MACH_PRIMA2_PM_H_ > +#define _MACH_PRIMA2_PM_H_ > + > +#define SIRFSOC_PWR_SLEEPFORCE 0x01 > + > +#define SIRFSOC_SLEEP_MODE_MASK 0x3 > +#define SIRFSOC_DEEP_SLEEP_MODE 0x1 > + > +#define SIRFSOC_PWRC_PDN_CTRL 0x0 > +#define SIRFSOC_PWRC_PON_OFF 0x4 > +#define SIRFSOC_PWRC_TRIGGER_EN 0x8 > +#define SIRFSOC_PWRC_PIN_STATUS 0x14 > +#define SIRFSOC_PWRC_SCRATCH_PAD1 0x18 > +#define SIRFSOC_PWRC_SCRATCH_PAD2 0x1C > + > +#ifndef __ASSEMBLY__ > +extern int sirfsoc_finish_suspend(unsigned long); > +#endif > + > +#endif > + > diff --git a/arch/arm/mach-prima2/sleep.S b/arch/arm/mach-prima2/sleep.S > new file mode 100644 > index 0000000..07f8067 > --- /dev/null > +++ b/arch/arm/mach-prima2/sleep.S > @@ -0,0 +1,61 @@ > +/* > + * sleep mode for CSR SiRFprimaII > + * > + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. > + * > + * Licensed under GPLv2 or later. > + */ > + > +#include <linux/linkage.h> > +#include <asm/ptrace.h> > +#include <asm/assembler.h> > + > +#include "pm.h" > + > +#define DENALI_CTL_22_OFF 0x58 > +#define DENALI_CTL_112_OFF 0x1c0 > + > + .text > + > +ENTRY(sirfsoc_finish_suspend) > + @ r5: mem controller > + ldr r0, =sirfsoc_memc_base > + ldr r5, [r0] > + @ r7: rtc iobrg controller > + ldr r0, =sirfsoc_rtciobrg_base > + ldr r7, [r0] > + > + @ Read the power control register and set the > + @ sleep force bit. > + ldr r0, =SIRFSOC_PWRC_PDN_CTRL > + bl __sirfsoc_rtc_iobrg_readl > + orr r0,r0,#SIRFSOC_PWR_SLEEPFORCE > + ldr r1, =SIRFSOC_PWRC_PDN_CTRL > + bl sirfsoc_rtc_iobrg_pre_writel > + mov r1, #0x1 > + > + @ read the MEM ctl register and set the self > + @ refresh bit > + > + ldr r2, [r5, #DENALI_CTL_22_OFF] > + orr r2, r2, #0x1 > + > + @ Following code has to run from cache since > + @ the RAM is going to self refresh mode > + .align 5 > + str r2, [r5, #DENALI_CTL_22_OFF] > + > +1: > + ldr r4, [r5, #DENALI_CTL_112_OFF] > + tst r4, #0x1 > + bne 1b > + > + @ write SLEEPFORCE through rtc iobridge > + > + str r1, [r7] > + @ wait rtc io bridge sync > +1: > + ldr r3, [r7] > + tst r3, #0x01 > + bne 1b > + b . > -- > 1.7.1 > > > > Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom > More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> + >> +static struct of_device_id pwrc_ids[] = { >> + { .compatible = "sirf,prima2-pwrc" }, > > Missing a sentinel terminator and const? i have agreed with you about that before, but forget to fix that yet.... Thanks for pointing out again. -barry
On Mon, Aug 22, 2011 at 11:15:50PM -0700, Barry Song wrote: > From: Rongjun Ying <rongjun.ying@csr.com> > > Signed-off-by: Rongjun Ying <rongjun.ying@csr.com> > Signed-off-by: Barry Song <Baohua.Song@csr.com> > --- > -v2: > delete redundant ARM mode registers save/restore > use generic cpu suspend > move l2 cache suspend/resume codes out > make memc become a device driver This looks much better, thanks.
On Mon, Aug 22, 2011 at 11:15:50PM -0700, Barry Song wrote: > From: Rongjun Ying <rongjun.ying@csr.com> > > Signed-off-by: Rongjun Ying <rongjun.ying@csr.com> > Signed-off-by: Barry Song <Baohua.Song@csr.com> > --- > -v2: > delete redundant ARM mode registers save/restore > use generic cpu suspend > move l2 cache suspend/resume codes out > make memc become a device driver > > arch/arm/mach-prima2/pm.c | 158 ++++++++++++++++++++++++++++++++++++++++++ > arch/arm/mach-prima2/pm.h | 31 ++++++++ > arch/arm/mach-prima2/sleep.S | 61 ++++++++++++++++ > 3 files changed, 250 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/mach-prima2/pm.c > create mode 100644 arch/arm/mach-prima2/pm.h > create mode 100644 arch/arm/mach-prima2/sleep.S > > diff --git a/arch/arm/mach-prima2/pm.c b/arch/arm/mach-prima2/pm.c > new file mode 100644 > index 0000000..75167df > --- /dev/null > +++ b/arch/arm/mach-prima2/pm.c > @@ -0,0 +1,158 @@ > +/* > + * power management entry for CSR SiRFprimaII > + * > + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. > + * > + * Licensed under GPLv2 or later. > + */ > + > +#include <linux/kernel.h> > +#include <linux/suspend.h> > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/of_platform.h> > +#include <linux/io.h> > +#include <linux/rtc/sirfsoc_rtciobrg.h> > +#include <asm/suspend.h> > + > +#include "pm.h" > + > +static u32 sirfsoc_pwrc_base; > +void __iomem *sirfsoc_memc_base; > + > +static void sirfsoc_set_wakeup_source(void) > +{ > + u32 pwr_trigger_en_reg; > + pwr_trigger_en_reg = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + > + SIRFSOC_PWRC_TRIGGER_EN); > +#define X_ON_KEY_B (1 << 0) > + sirfsoc_rtc_iobrg_writel(pwr_trigger_en_reg | X_ON_KEY_B, > + sirfsoc_pwrc_base + SIRFSOC_PWRC_TRIGGER_EN); > +} > + > +static void sirfsoc_set_sleep_mode(u32 mode) > +{ > + u32 sleep_mode = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + > + SIRFSOC_PWRC_PDN_CTRL); > + sleep_mode &= ~(SIRFSOC_SLEEP_MODE_MASK << 1); > + sleep_mode |= ((mode << 1)); Redundant parentheses. > + sirfsoc_rtc_iobrg_writel(sleep_mode, sirfsoc_pwrc_base + > + SIRFSOC_PWRC_PDN_CTRL); > +} > + > +int sirfsoc_pre_suspend_power_off(void) > +{ > + u32 wakeup_entry = virt_to_phys(cpu_resume); > + > + sirfsoc_rtc_iobrg_writel(wakeup_entry, > + SIRFSOC_PWRC_SCRATCH_PAD1); > + > + sirfsoc_set_wakeup_source(); > + > + sirfsoc_set_sleep_mode(SIRFSOC_DEEP_SLEEP_MODE); > + > + return 0; > +} > + If I read it correctly, this function sets up resume entry, but I failed to see it being called anywhere? Am I missing anything? > +static void sirfsoc_save_register(u32 *ptr) > +{ > + /* todo: save necessary system registers here */ > +} > + > +static void sirfsoc_restore_regs(u32 *ptr) > +{ > + /* todo: restore saved system registers here */ > +} > + > +static int sirfsoc_pm_enter(suspend_state_t state) > +{ > + u32 *saved_regs; > + > + switch (state) { > + case PM_SUSPEND_MEM: > + saved_regs = kmalloc(1024, GFP_ATOMIC); > + if (!saved_regs) > + return -ENOMEM; > + > + sirfsoc_save_register(saved_regs); > + > + /* go zzz */ > + cpu_suspend(0, sirfsoc_finish_suspend); > + > + sirfsoc_restore_regs(saved_regs); > + kfree(saved_regs); > + break; > + default: > + return -EINVAL; > + } > + return 0; > +} > + > +static const struct platform_suspend_ops sirfsoc_pm_ops = { > + .enter = sirfsoc_pm_enter, > + .valid = suspend_valid_only_mem, > +}; > + > +static int __init sirfsoc_pm_init(void) > +{ > + suspend_set_ops(&sirfsoc_pm_ops); > + return 0; > +} > +late_initcall(sirfsoc_pm_init); > + 8<--- > +static struct of_device_id pwrc_ids[] = { > + { .compatible = "sirf,prima2-pwrc" }, > +}; > + > +static int __init sirfsoc_of_pwrc_init(void) > +{ > + struct device_node *np; > + const __be32 *addrp; > + > + np = of_find_matching_node(NULL, pwrc_ids); > + if (!np) > + panic("unable to find compatible pwrc node in dtb\n"); > + > + addrp = of_get_property(np, "reg", NULL); > + if (!addrp) > + panic("unable to find base address of pwrc node in dtb\n"); > + > + sirfsoc_pwrc_base = be32_to_cpup(addrp); > + > + of_node_put(np); --->8 To me, it seems that the above code snippet can be as simply as the following. np = of_find_compatible_node(NULL, NULL, "sirf,prima2-pwrc"); sirfsoc_pwrc_base = of_iomap(np, 0); if (!sirfsoc_pwrc_base) return -ENOMEM; > + > + return 0; > +} > +postcore_initcall(sirfsoc_of_pwrc_init); > + > +static struct of_device_id memc_ids[] = { > + { .compatible = "sirf,prima2-memc" }, > +}; > + > +static int __devinit sirfsoc_memc_probe(struct platform_device *op) > +{ > + struct device_node *np = op->dev.of_node; > + > + sirfsoc_memc_base = of_iomap(np, 0); > + if (!sirfsoc_memc_base) > + panic("unable to map memc registers\n"); > + > + return 0; > +} > + > +static struct platform_driver sirfsoc_memc_driver = { > + .probe = sirfsoc_memc_probe, > + .driver = { > + .name = "sirfsoc-memc", > + .owner = THIS_MODULE, > + .of_match_table = memc_ids, > + }, > +}; > + > +static int __init sirfsoc_memc_init(void) > +{ > + return platform_driver_register(&sirfsoc_memc_driver); > +} > +postcore_initcall(sirfsoc_memc_init); Doing the same thing - mapping address, why sirfsoc_pwrc_base uses a function, while sirfsoc_memc_base needs a platform_driver? You will have more stuff about memc to add there?
Hi Shawn, 2011/8/25 Shawn Guo <shawn.guo@freescale.com>: > On Mon, Aug 22, 2011 at 11:15:50PM -0700, Barry Song wrote: >> From: Rongjun Ying <rongjun.ying@csr.com> >> >> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com> >> Signed-off-by: Barry Song <Baohua.Song@csr.com> >> --- >> -v2: >> delete redundant ARM mode registers save/restore >> use generic cpu suspend >> move l2 cache suspend/resume codes out >> make memc become a device driver >> >> arch/arm/mach-prima2/pm.c | 158 ++++++++++++++++++++++++++++++++++++++++++ >> arch/arm/mach-prima2/pm.h | 31 ++++++++ >> arch/arm/mach-prima2/sleep.S | 61 ++++++++++++++++ >> 3 files changed, 250 insertions(+), 0 deletions(-) >> create mode 100644 arch/arm/mach-prima2/pm.c >> create mode 100644 arch/arm/mach-prima2/pm.h >> create mode 100644 arch/arm/mach-prima2/sleep.S >> >> diff --git a/arch/arm/mach-prima2/pm.c b/arch/arm/mach-prima2/pm.c >> new file mode 100644 >> index 0000000..75167df >> --- /dev/null >> +++ b/arch/arm/mach-prima2/pm.c >> @@ -0,0 +1,158 @@ >> +/* >> + * power management entry for CSR SiRFprimaII >> + * >> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. >> + * >> + * Licensed under GPLv2 or later. >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/suspend.h> >> +#include <linux/slab.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/of_device.h> >> +#include <linux/of_platform.h> >> +#include <linux/io.h> >> +#include <linux/rtc/sirfsoc_rtciobrg.h> >> +#include <asm/suspend.h> >> + >> +#include "pm.h" >> + >> +static u32 sirfsoc_pwrc_base; >> +void __iomem *sirfsoc_memc_base; >> + >> +static void sirfsoc_set_wakeup_source(void) >> +{ >> + u32 pwr_trigger_en_reg; >> + pwr_trigger_en_reg = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + >> + SIRFSOC_PWRC_TRIGGER_EN); >> +#define X_ON_KEY_B (1 << 0) >> + sirfsoc_rtc_iobrg_writel(pwr_trigger_en_reg | X_ON_KEY_B, >> + sirfsoc_pwrc_base + SIRFSOC_PWRC_TRIGGER_EN); >> +} >> + >> +static void sirfsoc_set_sleep_mode(u32 mode) >> +{ >> + u32 sleep_mode = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + >> + SIRFSOC_PWRC_PDN_CTRL); >> + sleep_mode &= ~(SIRFSOC_SLEEP_MODE_MASK << 1); >> + sleep_mode |= ((mode << 1)); > > Redundant parentheses. > >> + sirfsoc_rtc_iobrg_writel(sleep_mode, sirfsoc_pwrc_base + >> + SIRFSOC_PWRC_PDN_CTRL); >> +} >> + >> +int sirfsoc_pre_suspend_power_off(void) >> +{ >> + u32 wakeup_entry = virt_to_phys(cpu_resume); >> + >> + sirfsoc_rtc_iobrg_writel(wakeup_entry, >> + SIRFSOC_PWRC_SCRATCH_PAD1); >> + >> + sirfsoc_set_wakeup_source(); >> + >> + sirfsoc_set_sleep_mode(SIRFSOC_DEEP_SLEEP_MODE); >> + >> + return 0; >> +} >> + > If I read it correctly, this function sets up resume entry, but I > failed to see it being called anywhere? Am I missing anything? lose this while sending patch, it is before cpu_suspend() called. Thanks for review. > >> +static void sirfsoc_save_register(u32 *ptr) >> +{ >> + /* todo: save necessary system registers here */ >> +} >> + >> +static void sirfsoc_restore_regs(u32 *ptr) >> +{ >> + /* todo: restore saved system registers here */ >> +} >> + >> +static int sirfsoc_pm_enter(suspend_state_t state) >> +{ >> + u32 *saved_regs; >> + >> + switch (state) { >> + case PM_SUSPEND_MEM: >> + saved_regs = kmalloc(1024, GFP_ATOMIC); >> + if (!saved_regs) >> + return -ENOMEM; >> + >> + sirfsoc_save_register(saved_regs); >> + >> + /* go zzz */ >> + cpu_suspend(0, sirfsoc_finish_suspend); >> + >> + sirfsoc_restore_regs(saved_regs); >> + kfree(saved_regs); >> + break; >> + default: >> + return -EINVAL; >> + } >> + return 0; >> +} >> + >> +static const struct platform_suspend_ops sirfsoc_pm_ops = { >> + .enter = sirfsoc_pm_enter, >> + .valid = suspend_valid_only_mem, >> +}; >> + >> +static int __init sirfsoc_pm_init(void) >> +{ >> + suspend_set_ops(&sirfsoc_pm_ops); >> + return 0; >> +} >> +late_initcall(sirfsoc_pm_init); >> + > > 8<--- >> +static struct of_device_id pwrc_ids[] = { >> + { .compatible = "sirf,prima2-pwrc" }, >> +}; >> + >> +static int __init sirfsoc_of_pwrc_init(void) >> +{ >> + struct device_node *np; >> + const __be32 *addrp; >> + >> + np = of_find_matching_node(NULL, pwrc_ids); >> + if (!np) >> + panic("unable to find compatible pwrc node in dtb\n"); >> + >> + addrp = of_get_property(np, "reg", NULL); >> + if (!addrp) >> + panic("unable to find base address of pwrc node in dtb\n"); >> + >> + sirfsoc_pwrc_base = be32_to_cpup(addrp); >> + >> + of_node_put(np); > --->8 > > To me, it seems that the above code snippet can be as simply as the > following. > > np = of_find_compatible_node(NULL, NULL, "sirf,prima2-pwrc"); > sirfsoc_pwrc_base = of_iomap(np, 0); > if (!sirfsoc_pwrc_base) > return -ENOMEM; you might want to read earlier thread in v1. PWRC is not in memory space. > >> + >> + return 0; >> +} >> +postcore_initcall(sirfsoc_of_pwrc_init); >> + >> +static struct of_device_id memc_ids[] = { >> + { .compatible = "sirf,prima2-memc" }, >> +}; >> + >> +static int __devinit sirfsoc_memc_probe(struct platform_device *op) >> +{ >> + struct device_node *np = op->dev.of_node; >> + >> + sirfsoc_memc_base = of_iomap(np, 0); >> + if (!sirfsoc_memc_base) >> + panic("unable to map memc registers\n"); >> + >> + return 0; >> +} >> + >> +static struct platform_driver sirfsoc_memc_driver = { >> + .probe = sirfsoc_memc_probe, >> + .driver = { >> + .name = "sirfsoc-memc", >> + .owner = THIS_MODULE, >> + .of_match_table = memc_ids, >> + }, >> +}; >> + >> +static int __init sirfsoc_memc_init(void) >> +{ >> + return platform_driver_register(&sirfsoc_memc_driver); >> +} >> +postcore_initcall(sirfsoc_memc_init); > > Doing the same thing - mapping address, why sirfsoc_pwrc_base uses > a function, while sirfsoc_memc_base needs a platform_driver? You > will have more stuff about memc to add there? memc is in memory space, actually simple_bus, then a platform device has existed for it. pwrc is now not compitable with simple_bus. it looks like not worth a platform for the moment too. > > -- > Regards, > Shawn Thanks Barry
On Thu, Aug 25, 2011 at 03:30:27PM +0800, Barry Song wrote: > Hi Shawn, > 2011/8/25 Shawn Guo <shawn.guo@freescale.com>: > > On Mon, Aug 22, 2011 at 11:15:50PM -0700, Barry Song wrote: > >> From: Rongjun Ying <rongjun.ying@csr.com> > >> > >> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com> > >> Signed-off-by: Barry Song <Baohua.Song@csr.com> > >> --- > >> -v2: > >> delete redundant ARM mode registers save/restore > >> use generic cpu suspend > >> move l2 cache suspend/resume codes out > >> make memc become a device driver > >> > >> arch/arm/mach-prima2/pm.c | 158 ++++++++++++++++++++++++++++++++++++++++++ > >> arch/arm/mach-prima2/pm.h | 31 ++++++++ > >> arch/arm/mach-prima2/sleep.S | 61 ++++++++++++++++ > >> 3 files changed, 250 insertions(+), 0 deletions(-) > >> create mode 100644 arch/arm/mach-prima2/pm.c > >> create mode 100644 arch/arm/mach-prima2/pm.h > >> create mode 100644 arch/arm/mach-prima2/sleep.S > >> > >> diff --git a/arch/arm/mach-prima2/pm.c b/arch/arm/mach-prima2/pm.c > >> new file mode 100644 > >> index 0000000..75167df > >> --- /dev/null > >> +++ b/arch/arm/mach-prima2/pm.c > >> @@ -0,0 +1,158 @@ > >> +/* > >> + * power management entry for CSR SiRFprimaII > >> + * > >> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. > >> + * > >> + * Licensed under GPLv2 or later. > >> + */ > >> + > >> +#include <linux/kernel.h> > >> +#include <linux/suspend.h> > >> +#include <linux/slab.h> > >> +#include <linux/of.h> > >> +#include <linux/of_address.h> > >> +#include <linux/of_device.h> > >> +#include <linux/of_platform.h> > >> +#include <linux/io.h> > >> +#include <linux/rtc/sirfsoc_rtciobrg.h> > >> +#include <asm/suspend.h> > >> + > >> +#include "pm.h" > >> + > >> +static u32 sirfsoc_pwrc_base; > >> +void __iomem *sirfsoc_memc_base; > >> + > >> +static void sirfsoc_set_wakeup_source(void) > >> +{ > >> + u32 pwr_trigger_en_reg; > >> + pwr_trigger_en_reg = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + > >> + SIRFSOC_PWRC_TRIGGER_EN); > >> +#define X_ON_KEY_B (1 << 0) > >> + sirfsoc_rtc_iobrg_writel(pwr_trigger_en_reg | X_ON_KEY_B, > >> + sirfsoc_pwrc_base + SIRFSOC_PWRC_TRIGGER_EN); > >> +} > >> + > >> +static void sirfsoc_set_sleep_mode(u32 mode) > >> +{ > >> + u32 sleep_mode = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + > >> + SIRFSOC_PWRC_PDN_CTRL); > >> + sleep_mode &= ~(SIRFSOC_SLEEP_MODE_MASK << 1); > >> + sleep_mode |= ((mode << 1)); > > > > Redundant parentheses. > > > >> + sirfsoc_rtc_iobrg_writel(sleep_mode, sirfsoc_pwrc_base + > >> + SIRFSOC_PWRC_PDN_CTRL); > >> +} > >> + > >> +int sirfsoc_pre_suspend_power_off(void) > >> +{ > >> + u32 wakeup_entry = virt_to_phys(cpu_resume); > >> + > >> + sirfsoc_rtc_iobrg_writel(wakeup_entry, > >> + SIRFSOC_PWRC_SCRATCH_PAD1); > >> + > >> + sirfsoc_set_wakeup_source(); > >> + > >> + sirfsoc_set_sleep_mode(SIRFSOC_DEEP_SLEEP_MODE); > >> + > >> + return 0; > >> +} > >> + > > If I read it correctly, this function sets up resume entry, but I > > failed to see it being called anywhere? Am I missing anything? > > lose this while sending patch, it is before cpu_suspend() called. > Thanks for review. > > > > >> +static void sirfsoc_save_register(u32 *ptr) > >> +{ > >> + /* todo: save necessary system registers here */ > >> +} > >> + > >> +static void sirfsoc_restore_regs(u32 *ptr) > >> +{ > >> + /* todo: restore saved system registers here */ > >> +} > >> + > >> +static int sirfsoc_pm_enter(suspend_state_t state) > >> +{ > >> + u32 *saved_regs; > >> + > >> + switch (state) { > >> + case PM_SUSPEND_MEM: > >> + saved_regs = kmalloc(1024, GFP_ATOMIC); > >> + if (!saved_regs) > >> + return -ENOMEM; > >> + > >> + sirfsoc_save_register(saved_regs); > >> + > >> + /* go zzz */ > >> + cpu_suspend(0, sirfsoc_finish_suspend); > >> + > >> + sirfsoc_restore_regs(saved_regs); > >> + kfree(saved_regs); > >> + break; > >> + default: > >> + return -EINVAL; > >> + } > >> + return 0; > >> +} > >> + > >> +static const struct platform_suspend_ops sirfsoc_pm_ops = { > >> + .enter = sirfsoc_pm_enter, > >> + .valid = suspend_valid_only_mem, > >> +}; > >> + > >> +static int __init sirfsoc_pm_init(void) > >> +{ > >> + suspend_set_ops(&sirfsoc_pm_ops); > >> + return 0; > >> +} > >> +late_initcall(sirfsoc_pm_init); > >> + > > > > 8<--- > >> +static struct of_device_id pwrc_ids[] = { > >> + { .compatible = "sirf,prima2-pwrc" }, > >> +}; > >> + > >> +static int __init sirfsoc_of_pwrc_init(void) > >> +{ > >> + struct device_node *np; > >> + const __be32 *addrp; > >> + > >> + np = of_find_matching_node(NULL, pwrc_ids); > >> + if (!np) > >> + panic("unable to find compatible pwrc node in dtb\n"); > >> + > >> + addrp = of_get_property(np, "reg", NULL); > >> + if (!addrp) > >> + panic("unable to find base address of pwrc node in dtb\n"); > >> + > >> + sirfsoc_pwrc_base = be32_to_cpup(addrp); > >> + > >> + of_node_put(np); > > --->8 > > > > To me, it seems that the above code snippet can be as simply as the > > following. > > > > np = of_find_compatible_node(NULL, NULL, "sirf,prima2-pwrc"); > > sirfsoc_pwrc_base = of_iomap(np, 0); > > if (!sirfsoc_pwrc_base) > > return -ENOMEM; > > you might want to read earlier thread in v1. PWRC is not in memory space. > Ah, just read. Then you at least should be able to use of_property_read_u32(). > > > >> + > >> + return 0; > >> +} > >> +postcore_initcall(sirfsoc_of_pwrc_init); > >> + > >> +static struct of_device_id memc_ids[] = { > >> + { .compatible = "sirf,prima2-memc" }, > >> +}; > >> + > >> +static int __devinit sirfsoc_memc_probe(struct platform_device *op) > >> +{ > >> + struct device_node *np = op->dev.of_node; > >> + > >> + sirfsoc_memc_base = of_iomap(np, 0); > >> + if (!sirfsoc_memc_base) > >> + panic("unable to map memc registers\n"); > >> + > >> + return 0; > >> +} > >> + > >> +static struct platform_driver sirfsoc_memc_driver = { > >> + .probe = sirfsoc_memc_probe, > >> + .driver = { > >> + .name = "sirfsoc-memc", > >> + .owner = THIS_MODULE, > >> + .of_match_table = memc_ids, > >> + }, > >> +}; > >> + > >> +static int __init sirfsoc_memc_init(void) > >> +{ > >> + return platform_driver_register(&sirfsoc_memc_driver); > >> +} > >> +postcore_initcall(sirfsoc_memc_init); > > > > Doing the same thing - mapping address, why sirfsoc_pwrc_base uses > > a function, while sirfsoc_memc_base needs a platform_driver? You > > will have more stuff about memc to add there? > > memc is in memory space, actually simple_bus, then a platform device > has existed for it. > pwrc is now not compitable with simple_bus. it looks like not worth a > platform for the moment too. > It seems a little complicated to register a platform_driver just for getting an address. I'm not sure how hard Arnd is on this position. I'm going to send a patch to test it :)
2011/8/25 Shawn Guo <shawn.guo@freescale.com>: > On Thu, Aug 25, 2011 at 03:30:27PM +0800, Barry Song wrote: >> Hi Shawn, >> 2011/8/25 Shawn Guo <shawn.guo@freescale.com>: >> > On Mon, Aug 22, 2011 at 11:15:50PM -0700, Barry Song wrote: >> >> From: Rongjun Ying <rongjun.ying@csr.com> >> >> >> >> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com> >> >> Signed-off-by: Barry Song <Baohua.Song@csr.com> >> >> --- >> >> -v2: >> >> delete redundant ARM mode registers save/restore >> >> use generic cpu suspend >> >> move l2 cache suspend/resume codes out >> >> make memc become a device driver >> >> >> >> arch/arm/mach-prima2/pm.c | 158 ++++++++++++++++++++++++++++++++++++++++++ >> >> arch/arm/mach-prima2/pm.h | 31 ++++++++ >> >> arch/arm/mach-prima2/sleep.S | 61 ++++++++++++++++ >> >> 3 files changed, 250 insertions(+), 0 deletions(-) >> >> create mode 100644 arch/arm/mach-prima2/pm.c >> >> create mode 100644 arch/arm/mach-prima2/pm.h >> >> create mode 100644 arch/arm/mach-prima2/sleep.S >> >> >> >> diff --git a/arch/arm/mach-prima2/pm.c b/arch/arm/mach-prima2/pm.c >> >> new file mode 100644 >> >> index 0000000..75167df >> >> --- /dev/null >> >> +++ b/arch/arm/mach-prima2/pm.c >> >> @@ -0,0 +1,158 @@ >> >> +/* >> >> + * power management entry for CSR SiRFprimaII >> >> + * >> >> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. >> >> + * >> >> + * Licensed under GPLv2 or later. >> >> + */ >> >> + >> >> +#include <linux/kernel.h> >> >> +#include <linux/suspend.h> >> >> +#include <linux/slab.h> >> >> +#include <linux/of.h> >> >> +#include <linux/of_address.h> >> >> +#include <linux/of_device.h> >> >> +#include <linux/of_platform.h> >> >> +#include <linux/io.h> >> >> +#include <linux/rtc/sirfsoc_rtciobrg.h> >> >> +#include <asm/suspend.h> >> >> + >> >> +#include "pm.h" >> >> + >> >> +static u32 sirfsoc_pwrc_base; >> >> +void __iomem *sirfsoc_memc_base; >> >> + >> >> +static void sirfsoc_set_wakeup_source(void) >> >> +{ >> >> + u32 pwr_trigger_en_reg; >> >> + pwr_trigger_en_reg = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + >> >> + SIRFSOC_PWRC_TRIGGER_EN); >> >> +#define X_ON_KEY_B (1 << 0) >> >> + sirfsoc_rtc_iobrg_writel(pwr_trigger_en_reg | X_ON_KEY_B, >> >> + sirfsoc_pwrc_base + SIRFSOC_PWRC_TRIGGER_EN); >> >> +} >> >> + >> >> +static void sirfsoc_set_sleep_mode(u32 mode) >> >> +{ >> >> + u32 sleep_mode = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + >> >> + SIRFSOC_PWRC_PDN_CTRL); >> >> + sleep_mode &= ~(SIRFSOC_SLEEP_MODE_MASK << 1); >> >> + sleep_mode |= ((mode << 1)); >> > >> > Redundant parentheses. >> > >> >> + sirfsoc_rtc_iobrg_writel(sleep_mode, sirfsoc_pwrc_base + >> >> + SIRFSOC_PWRC_PDN_CTRL); >> >> +} >> >> + >> >> +int sirfsoc_pre_suspend_power_off(void) >> >> +{ >> >> + u32 wakeup_entry = virt_to_phys(cpu_resume); >> >> + >> >> + sirfsoc_rtc_iobrg_writel(wakeup_entry, >> >> + SIRFSOC_PWRC_SCRATCH_PAD1); >> >> + >> >> + sirfsoc_set_wakeup_source(); >> >> + >> >> + sirfsoc_set_sleep_mode(SIRFSOC_DEEP_SLEEP_MODE); >> >> + >> >> + return 0; >> >> +} >> >> + >> > If I read it correctly, this function sets up resume entry, but I >> > failed to see it being called anywhere? Am I missing anything? >> >> lose this while sending patch, it is before cpu_suspend() called. >> Thanks for review. >> >> > >> >> +static void sirfsoc_save_register(u32 *ptr) >> >> +{ >> >> + /* todo: save necessary system registers here */ >> >> +} >> >> + >> >> +static void sirfsoc_restore_regs(u32 *ptr) >> >> +{ >> >> + /* todo: restore saved system registers here */ >> >> +} >> >> + >> >> +static int sirfsoc_pm_enter(suspend_state_t state) >> >> +{ >> >> + u32 *saved_regs; >> >> + >> >> + switch (state) { >> >> + case PM_SUSPEND_MEM: >> >> + saved_regs = kmalloc(1024, GFP_ATOMIC); >> >> + if (!saved_regs) >> >> + return -ENOMEM; >> >> + >> >> + sirfsoc_save_register(saved_regs); >> >> + >> >> + /* go zzz */ >> >> + cpu_suspend(0, sirfsoc_finish_suspend); >> >> + >> >> + sirfsoc_restore_regs(saved_regs); >> >> + kfree(saved_regs); >> >> + break; >> >> + default: >> >> + return -EINVAL; >> >> + } >> >> + return 0; >> >> +} >> >> + >> >> +static const struct platform_suspend_ops sirfsoc_pm_ops = { >> >> + .enter = sirfsoc_pm_enter, >> >> + .valid = suspend_valid_only_mem, >> >> +}; >> >> + >> >> +static int __init sirfsoc_pm_init(void) >> >> +{ >> >> + suspend_set_ops(&sirfsoc_pm_ops); >> >> + return 0; >> >> +} >> >> +late_initcall(sirfsoc_pm_init); >> >> + >> > >> > 8<--- >> >> +static struct of_device_id pwrc_ids[] = { >> >> + { .compatible = "sirf,prima2-pwrc" }, >> >> +}; >> >> + >> >> +static int __init sirfsoc_of_pwrc_init(void) >> >> +{ >> >> + struct device_node *np; >> >> + const __be32 *addrp; >> >> + >> >> + np = of_find_matching_node(NULL, pwrc_ids); >> >> + if (!np) >> >> + panic("unable to find compatible pwrc node in dtb\n"); >> >> + >> >> + addrp = of_get_property(np, "reg", NULL); >> >> + if (!addrp) >> >> + panic("unable to find base address of pwrc node in dtb\n"); >> >> + >> >> + sirfsoc_pwrc_base = be32_to_cpup(addrp); >> >> + >> >> + of_node_put(np); >> > --->8 >> > >> > To me, it seems that the above code snippet can be as simply as the >> > following. >> > >> > np = of_find_compatible_node(NULL, NULL, "sirf,prima2-pwrc"); >> > sirfsoc_pwrc_base = of_iomap(np, 0); >> > if (!sirfsoc_pwrc_base) >> > return -ENOMEM; >> >> you might want to read earlier thread in v1. PWRC is not in memory space. >> > Ah, just read. Then you at least should be able to use > of_property_read_u32(). > >> > >> >> + >> >> + return 0; >> >> +} >> >> +postcore_initcall(sirfsoc_of_pwrc_init); >> >> + >> >> +static struct of_device_id memc_ids[] = { >> >> + { .compatible = "sirf,prima2-memc" }, >> >> +}; >> >> + >> >> +static int __devinit sirfsoc_memc_probe(struct platform_device *op) >> >> +{ >> >> + struct device_node *np = op->dev.of_node; >> >> + >> >> + sirfsoc_memc_base = of_iomap(np, 0); >> >> + if (!sirfsoc_memc_base) >> >> + panic("unable to map memc registers\n"); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static struct platform_driver sirfsoc_memc_driver = { >> >> + .probe = sirfsoc_memc_probe, >> >> + .driver = { >> >> + .name = "sirfsoc-memc", >> >> + .owner = THIS_MODULE, >> >> + .of_match_table = memc_ids, >> >> + }, >> >> +}; >> >> + >> >> +static int __init sirfsoc_memc_init(void) >> >> +{ >> >> + return platform_driver_register(&sirfsoc_memc_driver); >> >> +} >> >> +postcore_initcall(sirfsoc_memc_init); >> > >> > Doing the same thing - mapping address, why sirfsoc_pwrc_base uses >> > a function, while sirfsoc_memc_base needs a platform_driver? You >> > will have more stuff about memc to add there? >> >> memc is in memory space, actually simple_bus, then a platform device >> has existed for it. >> pwrc is now not compitable with simple_bus. it looks like not worth a >> platform for the moment too. >> > It seems a little complicated to register a platform_driver just for > getting an address. I'm not sure how hard Arnd is on this position. > I'm going to send a patch to test it :) i think Arnd preferred it to be driver: " > +static void __init sirfsoc_of_memc_map(void) > +{ > + struct device_node *np; > + > + np = of_find_matching_node(NULL, memc_ids); > + if (!np) > + panic("unable to find compatible memc node in dtb\n"); > + > + sirfsoc_memc_base = of_iomap(np, 0); > + if (!np) > + panic("unable to map compatible memc node in dtb\n"); > + > + of_node_put(np); > +} Same as for the other one, I think this should be another device driver. " > > -- > Regards, > Shawn
>>> >> +static int __devinit sirfsoc_memc_probe(struct platform_device *op) >>> >> +{ >>> >> + struct device_node *np = op->dev.of_node; >>> >> + >>> >> + sirfsoc_memc_base = of_iomap(np, 0); >>> >> + if (!sirfsoc_memc_base) >>> >> + panic("unable to map memc registers\n"); >>> >> + >>> >> + return 0; >>> >> +} >>> >> + >>> >> +static struct platform_driver sirfsoc_memc_driver = { >>> >> + .probe = sirfsoc_memc_probe, >>> >> + .driver = { >>> >> + .name = "sirfsoc-memc", >>> >> + .owner = THIS_MODULE, >>> >> + .of_match_table = memc_ids, >>> >> + }, >>> >> +}; >>> >> + >>> >> +static int __init sirfsoc_memc_init(void) >>> >> +{ >>> >> + return platform_driver_register(&sirfsoc_memc_driver); >>> >> +} >>> >> +postcore_initcall(sirfsoc_memc_init); >>> > >>> > Doing the same thing - mapping address, why sirfsoc_pwrc_base uses >>> > a function, while sirfsoc_memc_base needs a platform_driver? You >>> > will have more stuff about memc to add there? >>> >>> memc is in memory space, actually simple_bus, then a platform device >>> has existed for it. >>> pwrc is now not compitable with simple_bus. it looks like not worth a >>> platform for the moment too. >>> >> It seems a little complicated to register a platform_driver just for >> getting an address. I'm not sure how hard Arnd is on this position. >> I'm going to send a patch to test it :) > > i think Arnd preferred it to be driver: > > " >> +static void __init sirfsoc_of_memc_map(void) >> +{ >> + struct device_node *np; >> + >> + np = of_find_matching_node(NULL, memc_ids); >> + if (!np) >> + panic("unable to find compatible memc node in dtb\n"); >> + >> + sirfsoc_memc_base = of_iomap(np, 0); >> + if (!np) >> + panic("unable to map compatible memc node in dtb\n"); >> + >> + of_node_put(np); >> +} > > Same as for the other one, I think this should be another device > driver. > " will we have a common wrapper as below if we only need some virtual address and have no complicated driver? void __iomem* of_iomap_from_id(struct of_device_id id[], int index) { struct device_node *np; void iomem *addr; np = of_find_matching_node(NULL, ids); if (!np) return NULL; addr = of_iomap(ids, 0); of_node_put(np); return addr; } -barry
On Thursday 25 August 2011, Shawn Guo wrote: > > > To me, it seems that the above code snippet can be as simply as the > > > following. > > > > > > np = of_find_compatible_node(NULL, NULL, "sirf,prima2-pwrc"); > > > sirfsoc_pwrc_base = of_iomap(np, 0); > > > if (!sirfsoc_pwrc_base) > > > return -ENOMEM; > > > > you might want to read earlier thread in v1. PWRC is not in memory space. > > > Ah, just read. Then you at least should be able to use > of_property_read_u32(). I think of_get_address would be even more appropriate, but I could be misreading that function. > > >> + > > >> +static int __init sirfsoc_memc_init(void) > > >> +{ > > >> + return platform_driver_register(&sirfsoc_memc_driver); > > >> +} > > >> +postcore_initcall(sirfsoc_memc_init); > > > > > > Doing the same thing - mapping address, why sirfsoc_pwrc_base uses > > > a function, while sirfsoc_memc_base needs a platform_driver? You > > > will have more stuff about memc to add there? > > > > memc is in memory space, actually simple_bus, then a platform device > > has existed for it. > > pwrc is now not compitable with simple_bus. it looks like not worth a > > platform for the moment too. > > > It seems a little complicated to register a platform_driver just for > getting an address. I'm not sure how hard Arnd is on this position. > I'm going to send a patch to test it :) I think it's a grey area. In general, I always recommend a device driver unless the mapping is absolutely needed at boot time before we bring up the driver subsystem. This enforces an object-oriented mental model about the building blocks: Everything you want to talk to has to export its own functions and we can stack modules on top of other modules. Clearly there are a few cases where this is not possible or only adds complexity without any benefit, but I would like to see the model of having a device driver for each device be the rule, with few exceptions. One argument that I can accept for this specific case is that the power management code has to be written in assembly and doesn't really understand the device object model anyway, so we just end up exporting the base address, which is not something that proper drivers are doing. However, as soon as more functionality gets added that uses the memc register space, my preference would increasingly lean towards doing a device driver here. Arnd
2011/8/25 Arnd Bergmann <arnd@arndb.de>: > On Thursday 25 August 2011, Shawn Guo wrote: >> > > To me, it seems that the above code snippet can be as simply as the >> > > following. >> > > >> > > np = of_find_compatible_node(NULL, NULL, "sirf,prima2-pwrc"); >> > > sirfsoc_pwrc_base = of_iomap(np, 0); >> > > if (!sirfsoc_pwrc_base) >> > > return -ENOMEM; >> > >> > you might want to read earlier thread in v1. PWRC is not in memory space. >> > >> Ah, just read. Then you at least should be able to use >> of_property_read_u32(). > > I think of_get_address would be even more appropriate, but I could > be misreading that function. might use of_get_address and add some comments to clarify. > >> > >> + >> > >> +static int __init sirfsoc_memc_init(void) >> > >> +{ >> > >> + return platform_driver_register(&sirfsoc_memc_driver); >> > >> +} >> > >> +postcore_initcall(sirfsoc_memc_init); >> > > >> > > Doing the same thing - mapping address, why sirfsoc_pwrc_base uses >> > > a function, while sirfsoc_memc_base needs a platform_driver? You >> > > will have more stuff about memc to add there? >> > >> > memc is in memory space, actually simple_bus, then a platform device >> > has existed for it. >> > pwrc is now not compitable with simple_bus. it looks like not worth a >> > platform for the moment too. >> > >> It seems a little complicated to register a platform_driver just for >> getting an address. I'm not sure how hard Arnd is on this position. >> I'm going to send a patch to test it :) > > I think it's a grey area. In general, I always recommend a device > driver unless the mapping is absolutely needed at boot time before > we bring up the driver subsystem. > This enforces an object-oriented mental model about the building > blocks: Everything you want to talk to has to export its own functions > and we can stack modules on top of other modules. Clearly there are > a few cases where this is not possible or only adds complexity without > any benefit, but I would like to see the model of having a device > driver for each device be the rule, with few exceptions. > > One argument that I can accept for this specific case is that the > power management code has to be written in assembly and doesn't > really understand the device object model anyway, so we just end > up exporting the base address, which is not something that proper > drivers are doing. However, as soon as more functionality gets added > that uses the memc register space, my preference would increasingly > lean towards doing a device driver here. > > Arnd > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel Thanks barry
2011/8/23 Russell King - ARM Linux <linux@arm.linux.org.uk>: > On Mon, Aug 22, 2011 at 11:15:50PM -0700, Barry Song wrote: >> From: Rongjun Ying <rongjun.ying@csr.com> >> >> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com> >> Signed-off-by: Barry Song <Baohua.Song@csr.com> >> --- >> -v2: >> delete redundant ARM mode registers save/restore >> use generic cpu suspend >> move l2 cache suspend/resume codes out >> make memc become a device driver Arnd, are you ok with this or you have more comments? if you have been ok, i'd like to send v3 with fixing comments from Jamie, Shawn and you. -barry
diff --git a/arch/arm/mach-prima2/pm.c b/arch/arm/mach-prima2/pm.c new file mode 100644 index 0000000..75167df --- /dev/null +++ b/arch/arm/mach-prima2/pm.c @@ -0,0 +1,158 @@ +/* + * power management entry for CSR SiRFprimaII + * + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. + * + * Licensed under GPLv2 or later. + */ + +#include <linux/kernel.h> +#include <linux/suspend.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +#include <linux/io.h> +#include <linux/rtc/sirfsoc_rtciobrg.h> +#include <asm/suspend.h> + +#include "pm.h" + +static u32 sirfsoc_pwrc_base; +void __iomem *sirfsoc_memc_base; + +static void sirfsoc_set_wakeup_source(void) +{ + u32 pwr_trigger_en_reg; + pwr_trigger_en_reg = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + + SIRFSOC_PWRC_TRIGGER_EN); +#define X_ON_KEY_B (1 << 0) + sirfsoc_rtc_iobrg_writel(pwr_trigger_en_reg | X_ON_KEY_B, + sirfsoc_pwrc_base + SIRFSOC_PWRC_TRIGGER_EN); +} + +static void sirfsoc_set_sleep_mode(u32 mode) +{ + u32 sleep_mode = sirfsoc_rtc_iobrg_readl(sirfsoc_pwrc_base + + SIRFSOC_PWRC_PDN_CTRL); + sleep_mode &= ~(SIRFSOC_SLEEP_MODE_MASK << 1); + sleep_mode |= ((mode << 1)); + sirfsoc_rtc_iobrg_writel(sleep_mode, sirfsoc_pwrc_base + + SIRFSOC_PWRC_PDN_CTRL); +} + +int sirfsoc_pre_suspend_power_off(void) +{ + u32 wakeup_entry = virt_to_phys(cpu_resume); + + sirfsoc_rtc_iobrg_writel(wakeup_entry, + SIRFSOC_PWRC_SCRATCH_PAD1); + + sirfsoc_set_wakeup_source(); + + sirfsoc_set_sleep_mode(SIRFSOC_DEEP_SLEEP_MODE); + + return 0; +} + +static void sirfsoc_save_register(u32 *ptr) +{ + /* todo: save necessary system registers here */ +} + +static void sirfsoc_restore_regs(u32 *ptr) +{ + /* todo: restore saved system registers here */ +} + +static int sirfsoc_pm_enter(suspend_state_t state) +{ + u32 *saved_regs; + + switch (state) { + case PM_SUSPEND_MEM: + saved_regs = kmalloc(1024, GFP_ATOMIC); + if (!saved_regs) + return -ENOMEM; + + sirfsoc_save_register(saved_regs); + + /* go zzz */ + cpu_suspend(0, sirfsoc_finish_suspend); + + sirfsoc_restore_regs(saved_regs); + kfree(saved_regs); + break; + default: + return -EINVAL; + } + return 0; +} + +static const struct platform_suspend_ops sirfsoc_pm_ops = { + .enter = sirfsoc_pm_enter, + .valid = suspend_valid_only_mem, +}; + +static int __init sirfsoc_pm_init(void) +{ + suspend_set_ops(&sirfsoc_pm_ops); + return 0; +} +late_initcall(sirfsoc_pm_init); + +static struct of_device_id pwrc_ids[] = { + { .compatible = "sirf,prima2-pwrc" }, +}; + +static int __init sirfsoc_of_pwrc_init(void) +{ + struct device_node *np; + const __be32 *addrp; + + np = of_find_matching_node(NULL, pwrc_ids); + if (!np) + panic("unable to find compatible pwrc node in dtb\n"); + + addrp = of_get_property(np, "reg", NULL); + if (!addrp) + panic("unable to find base address of pwrc node in dtb\n"); + + sirfsoc_pwrc_base = be32_to_cpup(addrp); + + of_node_put(np); + + return 0; +} +postcore_initcall(sirfsoc_of_pwrc_init); + +static struct of_device_id memc_ids[] = { + { .compatible = "sirf,prima2-memc" }, +}; + +static int __devinit sirfsoc_memc_probe(struct platform_device *op) +{ + struct device_node *np = op->dev.of_node; + + sirfsoc_memc_base = of_iomap(np, 0); + if (!sirfsoc_memc_base) + panic("unable to map memc registers\n"); + + return 0; +} + +static struct platform_driver sirfsoc_memc_driver = { + .probe = sirfsoc_memc_probe, + .driver = { + .name = "sirfsoc-memc", + .owner = THIS_MODULE, + .of_match_table = memc_ids, + }, +}; + +static int __init sirfsoc_memc_init(void) +{ + return platform_driver_register(&sirfsoc_memc_driver); +} +postcore_initcall(sirfsoc_memc_init); diff --git a/arch/arm/mach-prima2/pm.h b/arch/arm/mach-prima2/pm.h new file mode 100644 index 0000000..fe2028b --- /dev/null +++ b/arch/arm/mach-prima2/pm.h @@ -0,0 +1,31 @@ +/* + * arch/arm/mach-prima2/pm.h + * + * Copyright (C) 2011 CSR + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#ifndef _MACH_PRIMA2_PM_H_ +#define _MACH_PRIMA2_PM_H_ + +#define SIRFSOC_PWR_SLEEPFORCE 0x01 + +#define SIRFSOC_SLEEP_MODE_MASK 0x3 +#define SIRFSOC_DEEP_SLEEP_MODE 0x1 + +#define SIRFSOC_PWRC_PDN_CTRL 0x0 +#define SIRFSOC_PWRC_PON_OFF 0x4 +#define SIRFSOC_PWRC_TRIGGER_EN 0x8 +#define SIRFSOC_PWRC_PIN_STATUS 0x14 +#define SIRFSOC_PWRC_SCRATCH_PAD1 0x18 +#define SIRFSOC_PWRC_SCRATCH_PAD2 0x1C + +#ifndef __ASSEMBLY__ +extern int sirfsoc_finish_suspend(unsigned long); +#endif + +#endif + diff --git a/arch/arm/mach-prima2/sleep.S b/arch/arm/mach-prima2/sleep.S new file mode 100644 index 0000000..07f8067 --- /dev/null +++ b/arch/arm/mach-prima2/sleep.S @@ -0,0 +1,61 @@ +/* + * sleep mode for CSR SiRFprimaII + * + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. + * + * Licensed under GPLv2 or later. + */ + +#include <linux/linkage.h> +#include <asm/ptrace.h> +#include <asm/assembler.h> + +#include "pm.h" + +#define DENALI_CTL_22_OFF 0x58 +#define DENALI_CTL_112_OFF 0x1c0 + + .text + +ENTRY(sirfsoc_finish_suspend) + @ r5: mem controller + ldr r0, =sirfsoc_memc_base + ldr r5, [r0] + @ r7: rtc iobrg controller + ldr r0, =sirfsoc_rtciobrg_base + ldr r7, [r0] + + @ Read the power control register and set the + @ sleep force bit. + ldr r0, =SIRFSOC_PWRC_PDN_CTRL + bl __sirfsoc_rtc_iobrg_readl + orr r0,r0,#SIRFSOC_PWR_SLEEPFORCE + ldr r1, =SIRFSOC_PWRC_PDN_CTRL + bl sirfsoc_rtc_iobrg_pre_writel + mov r1, #0x1 + + @ read the MEM ctl register and set the self + @ refresh bit + + ldr r2, [r5, #DENALI_CTL_22_OFF] + orr r2, r2, #0x1 + + @ Following code has to run from cache since + @ the RAM is going to self refresh mode + .align 5 + str r2, [r5, #DENALI_CTL_22_OFF] + +1: + ldr r4, [r5, #DENALI_CTL_112_OFF] + tst r4, #0x1 + bne 1b + + @ write SLEEPFORCE through rtc iobridge + + str r1, [r7] + @ wait rtc io bridge sync +1: + ldr r3, [r7] + tst r3, #0x01 + bne 1b + b .