Re: [PATCH v2 08/10] ARM: EXYNOS: Refactored code for using PMU address via DT
Hi Tomasz,
Thanks for review.
On Sat, Apr 26, 2014 at 7:19 AM, Tomasz Figa wrote:
> Hi,
>
>
> On 25.04.2014 14:32, Pankaj Dubey wrote:
>>
>> Under "arm/mach-exynos" many files are using PMU register offsets.
>> Since we have added support for accessing PMU base address via DT,
>> now we can remove PMU mapping from exynosX_iodesc.
>> Let's convert all these access using either of "get_exynos_pmuaddr"
>> or "get_exynos_regmap".
>> This will help us in removing static mapping of PMU base address
>> as well as help in reducing dependency over machine header files.
>> Thus helping for migration of PMU implementation from machine to driver
>> folder which can be reused for ARM64 bsed SoC.
>>
>> Signed-off-by: Pankaj Dubey
>> ---
>> arch/arm/mach-exynos/common.h |4 +-
>> arch/arm/mach-exynos/cpuidle.c | 37 ++-
>> arch/arm/mach-exynos/exynos.c | 19 +-
>> arch/arm/mach-exynos/hotplug.c |4 +-
>> arch/arm/mach-exynos/include/mach/map.h |3 -
>> arch/arm/mach-exynos/platsmp.c | 13 +-
>> arch/arm/mach-exynos/pm.c | 60 ++--
>> arch/arm/mach-exynos/pmu.c | 40 +--
>> arch/arm/mach-exynos/regs-pmu.h | 506
>> +++
>> 9 files changed, 348 insertions(+), 338 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
>> index ecfd0fc..ad5128e 100644
>> --- a/arch/arm/mach-exynos/common.h
>> +++ b/arch/arm/mach-exynos/common.h
>> @@ -40,7 +40,7 @@ extern void exynos_cpu_die(unsigned int cpu);
>>
>> /* PMU(Power Management Unit) support */
>>
>> -#define PMU_TABLE_END NULL
>> +#define PMU_TABLE_END 0x
>
>
> IMHO (-1U) would be more appropriate.
>
OK.
>
>>
>> enum sys_powerdown {
>> SYS_AFTR,
>> @@ -51,7 +51,7 @@ enum sys_powerdown {
>>
>> extern unsigned long l2x0_regs_phys;
>> struct exynos_pmu_conf {
>> - void __iomem *reg;
>> + unsigned int offset;
>> unsigned int val[NUM_SYS_POWERDOWN];
>> };
>>
>> diff --git a/arch/arm/mach-exynos/cpuidle.c
>> b/arch/arm/mach-exynos/cpuidle.c
>> index c57cae0..5dcdd46 100644
>> --- a/arch/arm/mach-exynos/cpuidle.c
>> +++ b/arch/arm/mach-exynos/cpuidle.c
>> @@ -17,6 +17,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>> #include
>> @@ -34,10 +35,10 @@
>>
>> #define REG_DIRECTGO_ADDR (samsung_rev() == EXYNOS4210_REV_1_1 ? \
>> S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0
>> ? \
>> - (S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
>> + 0x24 : S5P_INFORM0))
>> #define REG_DIRECTGO_FLAG (samsung_rev() == EXYNOS4210_REV_1_1 ? \
>> S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0
>> ? \
>> - (S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
>> + 0x20 : S5P_INFORM1))
>
>
> This patch seems to be based on old code, before Daniel Lezcano's Exynos
> cpuidle refactor [1] and it should be rebased on top of that series.
>
> [1] http://thread.gmane.org/gmane.linux.kernel.samsung-soc/29085
>
>
OK, I will rebase on top of it along with addressing all other review comments.
>>
>> #define S5P_CHECK_AFTR0xFCBA0D10
>>
>> @@ -60,6 +61,8 @@
>> #define PWR_CTRL2_CORE2_UP_RATIO (1 << 4)
>> #define PWR_CTRL2_CORE1_UP_RATIO (1 << 0)
>>
>> +static struct regmap *pmu_regmap;
>> +
>> static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>> struct cpuidle_driver *drv,
>> int index);
>> @@ -87,7 +90,7 @@ static struct cpuidle_driver exynos4_idle_driver = {
>> /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
>> static void exynos4_set_wakeupmask(void)
>> {
>> - __raw_writel(0xff3e, S5P_WAKEUP_MASK);
>> + regmap_write(pmu_regmap, S5P_WAKEUP_MASK, 0xff3e);
>> }
>>
>> static unsigned int g_pwr_ctrl, g_diag_reg;
>> @@ -120,22 +123,28 @@ static int exynos4_enter_core0_aftr(struct
>> cpuidle_device *dev,
>> struct cpuidle_driver *drv,
>> int index)
>> {
>> - unsigned long tmp;
>> + unsigned int tmp;
>>
>> exynos4_set_wakeupmask();
>>
>> /* Set value of power down register for aftr mode */
>> exynos_sys_powerdown_conf(SYS_AFTR);
>> -
>> - __raw_writel(virt_to_phys(exynos_cpu_resume), REG_DIRECTGO_ADDR);
>> - __raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
>> -
>> +
>> + if (samsung_rev() == EXYNOS4210_REV_1_0) {
>> + __raw_writel(virt_to_phys(exynos_cpu_resume),
>> + S5P_VA_SYSRAM + REG_DIRECTGO_ADDR);
>> + __raw_writel(S5P_CHECK_AFTR, S5P_VA_SYSRAM +
>> REG_DIRECTGO_FLAG);
>> + } else {
>> + regmap_write(pmu_regmap, REG_DIRECTGO_ADDR,
>> + vir
Re: [PATCH v2 08/10] ARM: EXYNOS: Refactored code for using PMU address via DT
Hi,
On 25.04.2014 14:32, Pankaj Dubey wrote:
Under "arm/mach-exynos" many files are using PMU register offsets.
Since we have added support for accessing PMU base address via DT,
now we can remove PMU mapping from exynosX_iodesc.
Let's convert all these access using either of "get_exynos_pmuaddr"
or "get_exynos_regmap".
This will help us in removing static mapping of PMU base address
as well as help in reducing dependency over machine header files.
Thus helping for migration of PMU implementation from machine to driver
folder which can be reused for ARM64 bsed SoC.
Signed-off-by: Pankaj Dubey
---
arch/arm/mach-exynos/common.h |4 +-
arch/arm/mach-exynos/cpuidle.c | 37 ++-
arch/arm/mach-exynos/exynos.c | 19 +-
arch/arm/mach-exynos/hotplug.c |4 +-
arch/arm/mach-exynos/include/mach/map.h |3 -
arch/arm/mach-exynos/platsmp.c | 13 +-
arch/arm/mach-exynos/pm.c | 60 ++--
arch/arm/mach-exynos/pmu.c | 40 +--
arch/arm/mach-exynos/regs-pmu.h | 506 +++
9 files changed, 348 insertions(+), 338 deletions(-)
diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index ecfd0fc..ad5128e 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -40,7 +40,7 @@ extern void exynos_cpu_die(unsigned int cpu);
/* PMU(Power Management Unit) support */
-#define PMU_TABLE_END NULL
+#define PMU_TABLE_END 0x
IMHO (-1U) would be more appropriate.
enum sys_powerdown {
SYS_AFTR,
@@ -51,7 +51,7 @@ enum sys_powerdown {
extern unsigned long l2x0_regs_phys;
struct exynos_pmu_conf {
- void __iomem *reg;
+ unsigned int offset;
unsigned int val[NUM_SYS_POWERDOWN];
};
diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index c57cae0..5dcdd46 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
@@ -34,10 +35,10 @@
#define REG_DIRECTGO_ADDR (samsung_rev() == EXYNOS4210_REV_1_1 ? \
S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
- (S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
+ 0x24 : S5P_INFORM0))
#define REG_DIRECTGO_FLAG (samsung_rev() == EXYNOS4210_REV_1_1 ? \
S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
- (S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
+ 0x20 : S5P_INFORM1))
This patch seems to be based on old code, before Daniel Lezcano's Exynos
cpuidle refactor [1] and it should be rebased on top of that series.
[1] http://thread.gmane.org/gmane.linux.kernel.samsung-soc/29085
#define S5P_CHECK_AFTR0xFCBA0D10
@@ -60,6 +61,8 @@
#define PWR_CTRL2_CORE2_UP_RATIO (1 << 4)
#define PWR_CTRL2_CORE1_UP_RATIO (1 << 0)
+static struct regmap *pmu_regmap;
+
static int exynos4_enter_lowpower(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index);
@@ -87,7 +90,7 @@ static struct cpuidle_driver exynos4_idle_driver = {
/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
static void exynos4_set_wakeupmask(void)
{
- __raw_writel(0xff3e, S5P_WAKEUP_MASK);
+ regmap_write(pmu_regmap, S5P_WAKEUP_MASK, 0xff3e);
}
static unsigned int g_pwr_ctrl, g_diag_reg;
@@ -120,22 +123,28 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device
*dev,
struct cpuidle_driver *drv,
int index)
{
- unsigned long tmp;
+ unsigned int tmp;
exynos4_set_wakeupmask();
/* Set value of power down register for aftr mode */
exynos_sys_powerdown_conf(SYS_AFTR);
-
- __raw_writel(virt_to_phys(exynos_cpu_resume), REG_DIRECTGO_ADDR);
- __raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
-
+
+ if (samsung_rev() == EXYNOS4210_REV_1_0) {
+ __raw_writel(virt_to_phys(exynos_cpu_resume),
+ S5P_VA_SYSRAM + REG_DIRECTGO_ADDR);
+ __raw_writel(S5P_CHECK_AFTR, S5P_VA_SYSRAM + REG_DIRECTGO_FLAG);
+ } else {
+ regmap_write(pmu_regmap, REG_DIRECTGO_ADDR,
+ virt_to_phys(exynos_cpu_resume));
+ regmap_write(pmu_regmap, REG_DIRECTGO_FLAG, S5P_CHECK_AFTR);
+ }
This is quite ugly. I'd refactor this into a function pointer set once
at initialization time depending on SoC type and create two functions
for both cases.
In general, please also see my comments about this kind of code checking
SoC type posted as a part of my review of Vikas' Exynos5260 PMU support
series [2].
[2] http://thread.gmane.org/gmane.linux.kernel.samsung-soc/28056/focus=29342
[PATCH v2 08/10] ARM: EXYNOS: Refactored code for using PMU address via DT
Under "arm/mach-exynos" many files are using PMU register offsets.
Since we have added support for accessing PMU base address via DT,
now we can remove PMU mapping from exynosX_iodesc.
Let's convert all these access using either of "get_exynos_pmuaddr"
or "get_exynos_regmap".
This will help us in removing static mapping of PMU base address
as well as help in reducing dependency over machine header files.
Thus helping for migration of PMU implementation from machine to driver
folder which can be reused for ARM64 bsed SoC.
Signed-off-by: Pankaj Dubey
---
arch/arm/mach-exynos/common.h |4 +-
arch/arm/mach-exynos/cpuidle.c | 37 ++-
arch/arm/mach-exynos/exynos.c | 19 +-
arch/arm/mach-exynos/hotplug.c |4 +-
arch/arm/mach-exynos/include/mach/map.h |3 -
arch/arm/mach-exynos/platsmp.c | 13 +-
arch/arm/mach-exynos/pm.c | 60 ++--
arch/arm/mach-exynos/pmu.c | 40 +--
arch/arm/mach-exynos/regs-pmu.h | 506 +++
9 files changed, 348 insertions(+), 338 deletions(-)
diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index ecfd0fc..ad5128e 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -40,7 +40,7 @@ extern void exynos_cpu_die(unsigned int cpu);
/* PMU(Power Management Unit) support */
-#define PMU_TABLE_END NULL
+#define PMU_TABLE_END 0x
enum sys_powerdown {
SYS_AFTR,
@@ -51,7 +51,7 @@ enum sys_powerdown {
extern unsigned long l2x0_regs_phys;
struct exynos_pmu_conf {
- void __iomem *reg;
+ unsigned int offset;
unsigned int val[NUM_SYS_POWERDOWN];
};
diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index c57cae0..5dcdd46 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
@@ -34,10 +35,10 @@
#define REG_DIRECTGO_ADDR (samsung_rev() == EXYNOS4210_REV_1_1 ? \
S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
- (S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
+ 0x24 : S5P_INFORM0))
#define REG_DIRECTGO_FLAG (samsung_rev() == EXYNOS4210_REV_1_1 ? \
S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
- (S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
+ 0x20 : S5P_INFORM1))
#define S5P_CHECK_AFTR 0xFCBA0D10
@@ -60,6 +61,8 @@
#define PWR_CTRL2_CORE2_UP_RATIO (1 << 4)
#define PWR_CTRL2_CORE1_UP_RATIO (1 << 0)
+static struct regmap *pmu_regmap;
+
static int exynos4_enter_lowpower(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index);
@@ -87,7 +90,7 @@ static struct cpuidle_driver exynos4_idle_driver = {
/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
static void exynos4_set_wakeupmask(void)
{
- __raw_writel(0xff3e, S5P_WAKEUP_MASK);
+ regmap_write(pmu_regmap, S5P_WAKEUP_MASK, 0xff3e);
}
static unsigned int g_pwr_ctrl, g_diag_reg;
@@ -120,22 +123,28 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device
*dev,
struct cpuidle_driver *drv,
int index)
{
- unsigned long tmp;
+ unsigned int tmp;
exynos4_set_wakeupmask();
/* Set value of power down register for aftr mode */
exynos_sys_powerdown_conf(SYS_AFTR);
-
- __raw_writel(virt_to_phys(exynos_cpu_resume), REG_DIRECTGO_ADDR);
- __raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
-
+
+ if (samsung_rev() == EXYNOS4210_REV_1_0) {
+ __raw_writel(virt_to_phys(exynos_cpu_resume),
+ S5P_VA_SYSRAM + REG_DIRECTGO_ADDR);
+ __raw_writel(S5P_CHECK_AFTR, S5P_VA_SYSRAM + REG_DIRECTGO_FLAG);
+ } else {
+ regmap_write(pmu_regmap, REG_DIRECTGO_ADDR,
+ virt_to_phys(exynos_cpu_resume));
+ regmap_write(pmu_regmap, REG_DIRECTGO_FLAG, S5P_CHECK_AFTR);
+ }
save_cpu_arch_register();
/* Setting Central Sequence Register for power down mode */
- tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
+ regmap_read(pmu_regmap, S5P_CENTRAL_SEQ_CONFIGURATION, &tmp);
tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
- __raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
+ regmap_write(pmu_regmap, S5P_CENTRAL_SEQ_CONFIGURATION, tmp);
cpu_pm_enter();
cpu_suspend(0, idle_finisher);
@@ -154,14 +163,14 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device
*dev,
* S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
* in this situation.
*/
- tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
+ regm
