| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <503BBF24.2010107@codeaurora.org>
Date: Mon, 27 Aug 2012 11:40:36 -0700
From: Rohit Vaswani <rvaswani@...eaurora.org>
To: Cyril Chemparathy <cyril@...com>
CC: Russell King <linux@....linux.org.uk>, linux-doc@...r.kernel.org,
marc.zyngier@....com, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org,
Rob Herring <rob.herring@...xeda.com>,
Grant Likely <grant.likely@...retlab.ca>,
Rob Landley <rob@...dley.net>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] ARM: local timers: add timer support using IO mapped
register
On 8/10/2012 7:40 PM, Cyril Chemparathy wrote:
> On 8/10/2012 5:58 PM, Rohit Vaswani wrote:
>> The current arch_timer only support accessing through CP15 interface.
>> Add support for ARM processors that only support IO mapped register
>> interface
>>
>
> It looks like this patch attempts to address both (a) non-percpu arch
> timers, and (b) memory mapped arch timers in one go. These should
> probably be broken out into two distinct logical changes.
>
Will split them.
> More below...
>
>> Signed-off-by: Rohit Vaswani <rvaswani@...eaurora.org>
>> ---
>> .../devicetree/bindings/arm/arch_timer.txt | 7 +
>> arch/arm/kernel/arch_timer.c | 259
>> ++++++++++++++++----
>> 2 files changed, 223 insertions(+), 43 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt
>> b/Documentation/devicetree/bindings/arm/arch_timer.txt
>> index 52478c8..1c71799 100644
>> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
>> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
>> @@ -14,6 +14,13 @@ The timer is attached to a GIC to deliver its
>> per-processor interrupts.
>>
>> - clock-frequency : The frequency of the main counter, in Hz.
>> Optional.
>>
>> +- irq-is-not-percpu: Specify is the timer irq is *NOT* a percpu
>> (PPI) interrupt
>> + In the default case i.e without this property, the timer irq is
>> treated as a
>> + PPI interrupt. Optional.
>> +
>
> The handling of non-percpu IRQs looks broken. The code does
> (enable/disable)_percpu_irq() on IRQs that may no longer be percpu.
I am thinking of adding a wrapper function (function pointer in the
timer_ops) that will call (enable/disable)_percpu_irq() based on if the
said property is defined or not.
>
>> +- If the node address and reg is specified, the arch_timer will try
>> to use the memory
>> + mapped timer. Optional.
>> +
>> Example:
>>
>> timer {
>> diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
>> index 1d0d9df..09604b7 100644
>> --- a/arch/arm/kernel/arch_timer.c
>> +++ b/arch/arm/kernel/arch_timer.c
>> @@ -18,6 +18,7 @@
>> #include <linux/clockchips.h>
>> #include <linux/interrupt.h>
>> #include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> #include <linux/io.h>
>>
>> #include <asm/cputype.h>
>> @@ -29,8 +30,17 @@
>> static unsigned long arch_timer_rate;
>> static int arch_timer_ppi;
>> static int arch_timer_ppi2;
>> +static int is_irq_percpu;
>>
>> static struct clock_event_device __percpu **arch_timer_evt;
>> +static void __iomem *timer_base;
>> +
>
> Are percpu memory mapped arch timers an impossibility?
I will make sure to keep this open , as in add memory mapped timers as
well as the possibility of percpu timer interrupts for both memory
mapped and the cp15 interface.
>
>> +struct arch_timer_operations {
>> + void (*reg_write)(int, u32);
>> + u32 (*reg_read)(int);
>> + cycle_t (*get_cntpct)(void);
>> + cycle_t (*get_cntvct)(void);
>> +};
>>
>> /*
>> * Architected system timer support.
>> @@ -44,7 +54,29 @@ static struct clock_event_device __percpu
>> **arch_timer_evt;
>> #define ARCH_TIMER_REG_FREQ 1
>> #define ARCH_TIMER_REG_TVAL 2
>>
>> -static void arch_timer_reg_write(int reg, u32 val)
>> +/* Iomapped Register Offsets */
>> +#define ARCH_TIMER_CNTP_LOW_REG 0x000
>> +#define ARCH_TIMER_CNTP_HIGH_REG 0x004
>> +#define ARCH_TIMER_CNTV_LOW_REG 0x008
>> +#define ARCH_TIMER_CNTV_HIGH_REG 0x00C
>> +#define ARCH_TIMER_CTRL_REG 0x02C
>> +#define ARCH_TIMER_FREQ_REG 0x010
>> +#define ARCH_TIMER_CNTP_TVAL_REG 0x028
>> +#define ARCH_TIMER_CNTV_TVAL_REG 0x038
>> +
>
> ARCH_TIMER_CNTV_TVAL_REG appears to be unused here.
>
>> +static void timer_reg_write_mem(int reg, u32 val)
>> +{
>> + switch (reg) {
>> + case ARCH_TIMER_REG_CTRL:
>> + __raw_writel(val, timer_base + ARCH_TIMER_CTRL_REG);
>> + break;
>> + case ARCH_TIMER_REG_TVAL:
>> + __raw_writel(val, timer_base + ARCH_TIMER_CNTP_TVAL_REG);
>> + break;
>> + }
>> +}
>> +
>
> Wouldn't an array of offsets to map from ARCH_TIMER_REG_* to these
> memory mapped registers eliminate the need to switch-case your way
> through each register?
Done.
>
>> +static void timer_reg_write_cp15(int reg, u32 val)
>> {
>> switch (reg) {
>> case ARCH_TIMER_REG_CTRL:
>> @@ -58,7 +90,28 @@ static void arch_timer_reg_write(int reg, u32 val)
>> isb();
>> }
>>
>> -static u32 arch_timer_reg_read(int reg)
>> +static u32 timer_reg_read_mem(int reg)
>> +{
>> + u32 val;
>> +
>> + switch (reg) {
>> + case ARCH_TIMER_REG_CTRL:
>> + val = __raw_readl(timer_base + ARCH_TIMER_CTRL_REG);
>> + break;
>> + case ARCH_TIMER_REG_FREQ:
>> + val = __raw_readl(timer_base + ARCH_TIMER_FREQ_REG);
>> + break;
>> + case ARCH_TIMER_REG_TVAL:
>> + val = __raw_readl(timer_base + ARCH_TIMER_CNTP_TVAL_REG);
>> + break;
>> + default:
>> + BUG();
>> + }
>> +
>> + return val;
>> +}
>> +
>
> Same as above.
>
>> +static u32 timer_reg_read_cp15(int reg)
>> {
>> u32 val;
>>
>> @@ -79,6 +132,103 @@ static u32 arch_timer_reg_read(int reg)
>> return val;
>> }
>>
>> +static cycle_t arch_counter_get_cntpct_mem(void)
>> +{
>> + u32 cvall, cvalh, thigh;
>> +
>> + do {
>> + cvalh = __raw_readl(timer_base + ARCH_TIMER_CNTP_HIGH_REG);
>> + cvall = __raw_readl(timer_base + ARCH_TIMER_CNTP_LOW_REG);
>> + thigh = __raw_readl(timer_base + ARCH_TIMER_CNTP_HIGH_REG);
>> + } while (cvalh != thigh);
>> +
>> + return ((cycle_t) cvalh << 32) | cvall;
>> +}
>> +
>> +static cycle_t arch_counter_get_cntpct_cp15(void)
>> +{
>> + u32 cvall, cvalh;
>> +
>> + asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r"
>> (cvalh));
>> +
>> + return ((cycle_t) cvalh << 32) | cvall;
>> +}
>> +
>> +static cycle_t arch_counter_get_cntvct_mem(void)
>> +{
>> + u32 cvall, cvalh, thigh;
>> +
>> + do {
>> + cvalh = __raw_readl(timer_base + ARCH_TIMER_CNTV_HIGH_REG);
>> + cvall = __raw_readl(timer_base + ARCH_TIMER_CNTV_LOW_REG);
>> + thigh = __raw_readl(timer_base + ARCH_TIMER_CNTV_HIGH_REG);
>> + } while (cvalh != thigh);
>> +
>> + return ((cycle_t) cvalh << 32) | cvall;
>> +}
>> +
>
> Repetitive - the logic is identical to arch_counter_get_cntpct_mem()
> above.
>
>> +static cycle_t arch_counter_get_cntvct_cp15(void)
>> +{
>> + u32 cvall, cvalh;
>> +
>> + asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r"
>> (cvalh));
>> +
>> + return ((cycle_t) cvalh << 32) | cvall;
>> +}
>> +
>> +static struct arch_timer_operations arch_timer_ops_cp15 = {
>> + .reg_read = &timer_reg_read_cp15,
>> + .reg_write = &timer_reg_write_cp15,
>> + .get_cntpct = &arch_counter_get_cntpct_cp15,
>> + .get_cntvct = &arch_counter_get_cntvct_cp15,
>> +};
>> +
>> +static struct arch_timer_operations arch_timer_ops_mem = {
>> + .reg_read = &timer_reg_read_mem,
>> + .reg_write = &timer_reg_write_mem,
>> + .get_cntpct = &arch_counter_get_cntpct_mem,
>> + .get_cntvct = &arch_counter_get_cntvct_mem,
>> +};
>> +
>> +static struct arch_timer_operations *arch_specific_timer =
>> &arch_timer_ops_cp15;
>> +
>> +static inline void arch_timer_reg_write(int reg, u32 val)
>> +{
>> + arch_specific_timer->reg_write(reg, val);
>> +}
>> +
>> +static inline u32 arch_timer_reg_read(int reg)
>> +{
>> + return arch_specific_timer->reg_read(reg);
>> +}
>> +
>> +static inline cycle_t arch_counter_get_cntpct(void)
>> +{
>> + return arch_specific_timer->get_cntpct();
>> +}
>> +
>> +static inline cycle_t arch_counter_get_cntvct(void)
>> +{
>> + return arch_specific_timer->get_cntvct();
>> +}
>> +
>
> The 4 pointer chasers above could lose some verbosity by being #defines.
I am rebasing this patch on top of Marc's similar patch to add support
for virtual timers and will fix up the problems you have mentioned.
Thanks for your feedback.
>
> <snip>
Thanks,
Rohit Vaswani
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists