[RFC PATCH 2/2] ARM: CSR: add PM sleep entry for SiRFprimaII
Barry Song
21cnbao at gmail.com
Tue Aug 16 21:37:45 EDT 2011
Hi Arnd,
Thanks a lot!
2011/8/12 Arnd Bergmann <arnd at arndb.de>:
> On Friday 05 August 2011, Barry Song wrote:
>> From: Rongjun Ying <rongjun.ying at csr.com>
>>
>> Signed-off-by: Rongjun Ying <rongjun.ying at csr.com>
>> Signed-off-by: Barry Song <baohua.song at csr.com>
>
> Hi Barry and Rongjun,
>
> My impression of this code is that you just add hooks into
> a number of places, while this should really be a proper device
> driver.
>
>> diff --git a/arch/arm/mach-prima2/common.h b/arch/arm/mach-prima2/common.h
>> index 83e5d21..894e361 100644
>> --- a/arch/arm/mach-prima2/common.h
>> +++ b/arch/arm/mach-prima2/common.h
>> @@ -16,6 +16,7 @@ extern struct sys_timer sirfsoc_timer;
>>
>> extern void __init sirfsoc_of_irq_init(void);
>> extern void __init sirfsoc_of_clk_init(void);
>> +extern void sirfsoc_l2x_init(void);
>>
>> #ifndef CONFIG_DEBUG_LL
>> static inline void sirfsoc_map_lluart(void) {}
>
> You now call the sirfsoc_l2x_init function from two different places,
> which is very confusing. Better make sure that the idle code is
> only inialized after all the other subsystems it relies on have been
> activated.
anyway, after Rob Herring's patch "ARM: l2x0: Add OF based
initialization" is merged, sirfsoc l2 cache codes can be compelety
deleted.
i am thinking whether we can move l2 suspend/resume entries to
arch/arm/mm/cache-l2x0.c directly, maybe a syscore_ops:
static struct syscore_ops l2x_pm_syscore_ops = {
.suspend = l2x_pm_suspend,
.resume = l2x_pm_resume,
};
static void l2x_pm_init(void)
{
register_syscore_ops(&l2x_pm_syscore_ops);
}
or let cache l2x be a platform driver and add suspend/resume entries there?
if we move suspend/resume of L2 to arch/arm/mm/cache-l2x0.c, that can
eliminate my reference to l2 from other places.
>
>> diff --git a/arch/arm/mach-prima2/l2x0.c b/arch/arm/mach-prima2/l2x0.c
>> index 9cda205..adc9840 100644
>> --- a/arch/arm/mach-prima2/l2x0.c
>> +++ b/arch/arm/mach-prima2/l2x0.c
>> @@ -18,25 +18,14 @@
>> #define L2X0_ADDR_FILTERING_START 0xC00
>> #define L2X0_ADDR_FILTERING_END 0xC04
>>
>> +void __iomem *sirfsoc_l2x_base;
>> +
>> static struct of_device_id l2x_ids[] = {
>> { .compatible = "arm,pl310-cache" },
>> };
>
> Exporting the base address registers from one driver into another
> is a layering violation, don't do that. There are global functions
> that are used for managing the cache, so you should be calling those,
> or extend them to do what you need if necessary.
i can agree. i might test whether l2x_pm_syscore_ops can resolve our problem.
>
>> diff --git a/arch/arm/mach-prima2/pm.c b/arch/arm/mach-prima2/pm.c
>> new file mode 100644
>> index 0000000..bae7241
>> --- /dev/null
>> +++ b/arch/arm/mach-prima2/pm.c
>> @@ -0,0 +1,139 @@
>> +/*
>> + * 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/suspend.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/io.h>
>> +#include <linux/rtc/sirfsoc_rtciobrg.h>
>> +
>> +#include "common.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);
>> +}
>
> This again looks like a bad abstraction, and it gets the locking wrong,
> since you need a spinlock around both the read and write of the register
> to make the update atomic.
will there be two threads calling into pm_ops.enter() at the same
time? seems not, let me check....
>
> These functions therefore belong into the rtc_iobrg driver.
rtc_iobrg might only handle the read/write interfaces and let other
modules call these interfaces to read/write what they want.
>
>> +static struct of_device_id pwrc_ids[] = {
>> + { .compatible = "sirf,prima2-pwrc" },
>> +};
>> +
>> +static void __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");
>
> Don't use of_find_matching_node to look for stuff that is really more
> a device that you want to talk to. Instead, register a platform
> driver with that match_table and load a proper device driver for
> it.
agree. how do you think about cache l2, will it be a platform too?
>
>> +
>> + 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);
>> +}
>
> Why do you store the physical base address here, instead doing an of_iomap?
i should have failed to use of_iomap, register space behind rtciobrg
are not in memory space. it is accessed indirectly just like a NAND or
a harddisk.
>
>> +static struct of_device_id memc_ids[] = {
>> + { .compatible = "sirf,prima2-memc" },
>> +};
>>
>> +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.
ok.
>
>> +#include <linux/linkage.h>
>> +#include <asm/ptrace.h>
>> +#include <asm/assembler.h>
>> +#include <asm/hardware/cache-l2x0.h>
>> +
>> +#include "pm.h"
>> +
>> +#define DENALI_CTL_22_OFF 0x58
>> +#define DENALI_CTL_112_OFF 0x1c0
>> +
>> +ENTRY(sirfsoc_get_wakeup_pointer)
>> + stmfd sp!, {lr} @ save registers on stack
>> + adr r0, sirfsoc_wakeup
>> + ldmfd sp!, {pc} @ restore regs and return
>> +
>> +ENTRY(sirfsoc_sleep)
>> +sirfsoc_sleep:
>> + stmdb sp!, {r0-r12, lr} @ Push SVC state onto our stack
>> +
>> + mrc p15, 0, r2, c1, c0, 1 @ cp15 c1 auxiliary Control register
>> + mrc p15, 0, r3, c1, c0, 2 @ cp15 c1 coprocessor access control register
>> + mrc p15, 0, r4, c2, c0, 0 @ cp15 c2_r0 translation table base resigter 0
>> + mrc p15, 0, r5, c2, c0, 1 @ cp15 c2_r1 translation table base resigter 1
>> +
>> + mrc p15, 0, r6, c2, c0, 2 @ cp15 c2_r2 translation table base control resigter
>> +
>> + mrc p15, 0, r7, c3, c0, 0 @ load r2 with domain access control.
>> + mrc p15, 0, r8, c1, c0, 0 @ MMU Control
>> + mrc p15, 0, r9, c10,c2, 0 @ cp15 PRRR register
>> + mrc p15, 0, r10, c10,c2, 1 @ cp15 NMRR register
>> + mov r11, sp
>> + ldr r0, =save_context
>> + add r0, r0, #124
>> + stmdb r0!, {r2-r11} @ Save CP15 and the SP to stack
>
> It's not really clear why this code needs to be assembly. Would it be
> possible to write the same in C, with just a few inline assembly statements?
as the discussion with Russell, these will be replaced by generic cpu_suspend.
>
> Arnd
Thanks
Barry
More information about the linux-arm-kernel
mailing list