Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1406793 > unrolled thread

[PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

Started byCaesar Wang <wxt@rock-chips.com>
First post2016-05-25 12:00 +0200
Last post2016-06-01 08:20 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC Caesar Wang <wxt@rock-chips.com> - 2016-05-25 12:00 +0200
    Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-05-31 01:30 +0200
      Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC "Huang, Tao" <huangtao@rock-chips.com> - 2016-05-31 16:00 +0200
        Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-05-31 16:10 +0200
          Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC "Huang, Tao" <huangtao@rock-chips.com> - 2016-06-01 04:00 +0200
            Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-06-01 08:20 +0200

#1406793 — [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

FromCaesar Wang <wxt@rock-chips.com>
Date2016-05-25 12:00 +0200
Subject[PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC
Message-ID<rCDLA-7RB-11@gated-at.bofh.it>
From: Huang Tao <huangtao@rock-chips.com>

The CONTROL register offset is different from old SoCs.
For Linux driver, there are not functional changes at all.
Let's call it v2.

Signed-off-by: Huang Tao <huangtao@rock-chips.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 drivers/clocksource/rockchip_timer.c | 112 ++++++++++++++++++++++++++++-------
 1 file changed, 91 insertions(+), 21 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index f3dfb1a..7ce1d08 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -19,8 +19,9 @@
 
 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
-#define TIMER_CONTROL_REG	0x10
+#define TIMER_V1_CONTROL_REG	0x10
 #define TIMER_INT_STATUS	0x18
+#define TIMER_V2_CONTROL_REG	0x1c
 
 #define TIMER_DISABLE		0x0
 #define TIMER_ENABLE		0x1
@@ -46,15 +47,26 @@ static inline void __iomem *rk_base(struct clock_event_device *ce)
 	return rk_timer(ce)->base;
 }
 
-static inline void rk_timer_disable(struct clock_event_device *ce)
+static inline void rk_timer_v1_disable(struct clock_event_device *ce)
 {
-	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_V1_CONTROL_REG);
 }
 
-static inline void rk_timer_enable(struct clock_event_device *ce, u32 flags)
+static inline void rk_timer_v1_enable(struct clock_event_device *ce, u32 flags)
 {
 	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_base(ce) + TIMER_CONTROL_REG);
+		       rk_base(ce) + TIMER_V1_CONTROL_REG);
+}
+
+static inline void rk_timer_v2_disable(struct clock_event_device *ce)
+{
+	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_V2_CONTROL_REG);
+}
+
+static inline void rk_timer_v2_enable(struct clock_event_device *ce, u32 flags)
+{
+	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
+		       rk_base(ce) + TIMER_V2_CONTROL_REG);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
@@ -69,44 +81,82 @@ static void rk_timer_interrupt_clear(struct clock_event_device *ce)
 	writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS);
 }
 
-static inline int rk_timer_set_next_event(unsigned long cycles,
-					  struct clock_event_device *ce)
+static int rk_timer_v1_set_next_event(unsigned long cycles,
+				      struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	rk_timer_v1_disable(ce);
 	rk_timer_update_counter(cycles, ce);
-	rk_timer_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	rk_timer_v1_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
 	return 0;
 }
 
-static int rk_timer_shutdown(struct clock_event_device *ce)
+static int rk_timer_v1_shutdown(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	rk_timer_v1_disable(ce);
 	return 0;
 }
 
-static int rk_timer_set_periodic(struct clock_event_device *ce)
+static int rk_timer_v1_set_periodic(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	rk_timer_v1_disable(ce);
 	rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
-	rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
+	rk_timer_v1_enable(ce, TIMER_MODE_FREE_RUNNING);
 	return 0;
 }
 
-static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
+static irqreturn_t rk_timer_v1_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *ce = dev_id;
 
 	rk_timer_interrupt_clear(ce);
 
 	if (clockevent_state_oneshot(ce))
-		rk_timer_disable(ce);
+		rk_timer_v1_disable(ce);
 
 	ce->event_handler(ce);
 
 	return IRQ_HANDLED;
 }
 
-static void __init rk_timer_init(struct device_node *np)
+static int rk_timer_v2_set_next_event(unsigned long cycles,
+				      struct clock_event_device *ce)
+{
+	rk_timer_v2_disable(ce);
+	rk_timer_update_counter(cycles, ce);
+	rk_timer_v2_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	return 0;
+}
+
+static int rk_timer_v2_shutdown(struct clock_event_device *ce)
+{
+	rk_timer_v2_disable(ce);
+	return 0;
+}
+
+static int rk_timer_v2_set_periodic(struct clock_event_device *ce)
+{
+	rk_timer_v2_disable(ce);
+	rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
+	rk_timer_v2_enable(ce, TIMER_MODE_FREE_RUNNING);
+	return 0;
+}
+
+static irqreturn_t rk_timer_v2_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *ce = dev_id;
+
+	rk_timer_interrupt_clear(ce);
+
+	if (clockevent_state_oneshot(ce))
+		rk_timer_v2_disable(ce);
+
+	ce->event_handler(ce);
+
+	return IRQ_HANDLED;
+}
+
+static void __init rk_timer_init(struct device_node *np,
+				 irq_handler_t rk_timer_interrupt)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
 	struct clk *timer_clk;
@@ -152,9 +202,6 @@ static void __init rk_timer_init(struct device_node *np)
 	ce->name = TIMER_NAME;
 	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
 		       CLOCK_EVT_FEAT_DYNIRQ;
-	ce->set_next_event = rk_timer_set_next_event;
-	ce->set_state_shutdown = rk_timer_shutdown;
-	ce->set_state_periodic = rk_timer_set_periodic;
 	ce->irq = irq;
 	ce->cpumask = cpu_all_mask;
 	ce->rating = 250;
@@ -177,4 +224,27 @@ out_unmap:
 	iounmap(bc_timer.base);
 }
 
-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+static void __init rk_timer_v1_init(struct device_node *np)
+{
+	struct clock_event_device *ce = &bc_timer.ce;
+
+	ce->set_next_event = rk_timer_v1_set_next_event;
+	ce->set_state_shutdown = rk_timer_v1_shutdown;
+	ce->set_state_periodic = rk_timer_v1_set_periodic;
+
+	rk_timer_init(np, rk_timer_v1_interrupt);
+}
+
+static void __init rk_timer_v2_init(struct device_node *np)
+{
+	struct clock_event_device *ce = &bc_timer.ce;
+
+	ce->set_next_event = rk_timer_v2_set_next_event;
+	ce->set_state_shutdown = rk_timer_v2_shutdown;
+	ce->set_state_periodic = rk_timer_v2_set_periodic;
+
+	rk_timer_init(np, rk_timer_v2_interrupt);
+}
+
+CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer", rk_timer_v1_init);
+CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer", rk_timer_v2_init);
-- 
1.9.1

[toc] | [next] | [standalone]

#1409756

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2016-05-31 01:30 +0200
Message-ID<rEENc-2Hd-19@gated-at.bofh.it>
In reply to#1406793
On 05/25/2016 11:50 AM, Caesar Wang wrote:
> From: Huang Tao <huangtao@rock-chips.com>
>
> The CONTROL register offset is different from old SoCs.
> For Linux driver, there are not functional changes at all.
> Let's call it v2.
>
> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---

That's hackish.

Please consider something like:

diff --git a/drivers/clocksource/rockchip_timer.c 
b/drivers/clocksource/rockchip_timer.c
index b991b28..b6ba6f9 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -19,7 +19,8 @@

  #define TIMER_LOAD_COUNT0	0x00
  #define TIMER_LOAD_COUNT1	0x04
-#define TIMER_CONTROL_REG	0x10
+#define TIMER_CONTROL_REG3288	0x10
+#define TIMER_CONTROL_REG3399	0x1C
  #define TIMER_INT_STATUS	0x18

  #define TIMER_DISABLE		0x0
@@ -31,6 +32,7 @@
  struct bc_timer {
  	struct clock_event_device ce;
  	void __iomem *base;
+	void __iomem *ctrl;
  	u32 freq;
  };

@@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct 
clock_event_device *ce)
  	return rk_timer(ce)->base;
  }

+static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
+{
+        return rk_timer(ce)->ctrl;
+}
+
  static inline void rk_timer_disable(struct clock_event_device *ce)
  {
-	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
  }

  static inline void rk_timer_enable(struct clock_event_device *ce, u32 
flags)
  {
  	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_base(ce) + TIMER_CONTROL_REG);
+		       rk_ctrl(ce));
  }

  static void rk_timer_update_counter(unsigned long cycles,
@@ -179,4 +186,18 @@ out_unmap:
  	iounmap(bc_timer.base);
  }

-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+static void __init rk3288_timer_init(struct device_node *np)
+{
+	bc_timer.ctrl = TIMER_CONTROL_REG3288;
+	rk_timer_init(np);
+}
+
+static void __init rk3399_timer_init(struct device_node *np)
+{
+        bc_timer.ctrl = TIMER_CONTROL_REG3399;
+	rk_timer_init(np);
+}
+
+
+CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", 
rk3288_timer_init);
+CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3399-timer", 
rk3399_timer_init);




-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1410337

From"Huang, Tao" <huangtao@rock-chips.com>
Date2016-05-31 16:00 +0200
Message-ID<rESn8-3rp-27@gated-at.bofh.it>
In reply to#1409756
Hi Daniel:
On 2016年05月31日 07:28, Daniel Lezcano wrote:
> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>> From: Huang Tao <huangtao@rock-chips.com>
>>
>> The CONTROL register offset is different from old SoCs.
>> For Linux driver, there are not functional changes at all.
>> Let's call it v2.
>>
>> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Heiko Stuebner <heiko@sntech.de>
>> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> ---
> 
> That's hackish.
Yes:( I blamed our IC guy.
> 
> Please consider something like:
> 
> diff --git a/drivers/clocksource/rockchip_timer.c 
> b/drivers/clocksource/rockchip_timer.c
> index b991b28..b6ba6f9 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -19,7 +19,8 @@
> 
>   #define TIMER_LOAD_COUNT0	0x00
>   #define TIMER_LOAD_COUNT1	0x04
> -#define TIMER_CONTROL_REG	0x10
> +#define TIMER_CONTROL_REG3288	0x10
> +#define TIMER_CONTROL_REG3399	0x1C
>   #define TIMER_INT_STATUS	0x18
> 
>   #define TIMER_DISABLE		0x0
> @@ -31,6 +32,7 @@
>   struct bc_timer {
>   	struct clock_event_device ce;
>   	void __iomem *base;
> +	void __iomem *ctrl;
>   	u32 freq;
>   };
> 
> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct 
> clock_event_device *ce)
>   	return rk_timer(ce)->base;
>   }
> 
> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
> +{
> +        return rk_timer(ce)->ctrl;
> +}
> +
>   static inline void rk_timer_disable(struct clock_event_device *ce)
>   {
> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>   }
> 
>   static inline void rk_timer_enable(struct clock_event_device *ce, u32 
> flags)
>   {
>   	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
> -		       rk_base(ce) + TIMER_CONTROL_REG);
> +		       rk_ctrl(ce));
>   }
> 
>   static void rk_timer_update_counter(unsigned long cycles,
> @@ -179,4 +186,18 @@ out_unmap:
>   	iounmap(bc_timer.base);
>   }
> 
> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
> +static void __init rk3288_timer_init(struct device_node *np)
> +{
> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
> +	rk_timer_init(np);
> +}
> +
> +static void __init rk3399_timer_init(struct device_node *np)
> +{
> +        bc_timer.ctrl = TIMER_CONTROL_REG3399;
> +	rk_timer_init(np);
> +}
> +
> +
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", 
> rk3288_timer_init);
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3399-timer", 
> rk3399_timer_init);
> 
> 

I think you mean this patch otherwise compile will fail:
@@ -19,7 +19,8 @@

 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
-#define TIMER_CONTROL_REG	0x10
+#define TIMER_CONTROL_REG3288	0x10
+#define TIMER_CONTROL_REG3399	0x1C
 #define TIMER_INT_STATUS	0x18

 #define TIMER_DISABLE		0x0
@@ -31,6 +32,7 @@
 struct bc_timer {
 	struct clock_event_device ce;
 	void __iomem *base;
+	u32 ctrl;
 	u32 freq;
 };

@@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
clock_event_device *ce)
 	return rk_timer(ce)->base;
 }

+static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
+{
+	return rk_timer(ce)->base + rk_timer(ce)->ctrl;
+}
+
 static inline void rk_timer_disable(struct clock_event_device *ce)
 {
-	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
 }

 static inline void rk_timer_enable(struct clock_event_device *ce, u32
flags)
 {
 	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_base(ce) + TIMER_CONTROL_REG);
+		       rk_ctrl(ce));
 }

 static void rk_timer_update_counter(unsigned long cycles,
@@ -179,4 +186,19 @@ out_unmap:
 	iounmap(bc_timer.base);
 }

-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+static void __init rk3288_timer_init(struct device_node *np)
+{
+	bc_timer.ctrl = TIMER_CONTROL_REG3288;
+	rk_timer_init(np);
+}
+
+static void __init rk3399_timer_init(struct device_node *np)
+{
+	bc_timer.ctrl = TIMER_CONTROL_REG3399;
+	rk_timer_init(np);
+}
+
+CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
+		       rk3288_timer_init);
+CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer",
+		       rk3399_timer_init);

This patch will give us a little lager text size. If we do disassemble,
we can see additional LDR is called. I can accept this performance drop.
So we will send new patches.
BTW, the patch "clocksource: rockchip: remove unnecessary clear irq
before request_irq" can drop if we use this patch.

Thanks,
Huang, Tao

[toc] | [prev] | [next] | [standalone]

#1410340

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2016-05-31 16:10 +0200
Message-ID<rESwN-3JM-7@gated-at.bofh.it>
In reply to#1410337
On 05/31/2016 03:46 PM, Huang, Tao wrote:
> Hi Daniel:
> On 2016年05月31日 07:28, Daniel Lezcano wrote:
>> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>>> From: Huang Tao <huangtao@rock-chips.com>
>>>
>>> The CONTROL register offset is different from old SoCs.
>>> For Linux driver, there are not functional changes at all.
>>> Let's call it v2.
>>>
>>> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Heiko Stuebner <heiko@sntech.de>
>>> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
>>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>>> ---
>>
>> That's hackish.
> Yes:( I blamed our IC guy.
>>
>> Please consider something like:
>>
>> diff --git a/drivers/clocksource/rockchip_timer.c
>> b/drivers/clocksource/rockchip_timer.c
>> index b991b28..b6ba6f9 100644
>> --- a/drivers/clocksource/rockchip_timer.c
>> +++ b/drivers/clocksource/rockchip_timer.c
>> @@ -19,7 +19,8 @@
>>
>>    #define TIMER_LOAD_COUNT0	0x00
>>    #define TIMER_LOAD_COUNT1	0x04
>> -#define TIMER_CONTROL_REG	0x10
>> +#define TIMER_CONTROL_REG3288	0x10
>> +#define TIMER_CONTROL_REG3399	0x1C
>>    #define TIMER_INT_STATUS	0x18
>>
>>    #define TIMER_DISABLE		0x0
>> @@ -31,6 +32,7 @@
>>    struct bc_timer {
>>    	struct clock_event_device ce;
>>    	void __iomem *base;
>> +	void __iomem *ctrl;
>>    	u32 freq;
>>    };
>>
>> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
>> clock_event_device *ce)
>>    	return rk_timer(ce)->base;
>>    }
>>
>> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
>> +{
>> +        return rk_timer(ce)->ctrl;
>> +}
>> +
>>    static inline void rk_timer_disable(struct clock_event_device *ce)
>>    {
>> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
>> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>>    }
>>
>>    static inline void rk_timer_enable(struct clock_event_device *ce, u32
>> flags)
>>    {
>>    	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
>> -		       rk_base(ce) + TIMER_CONTROL_REG);
>> +		       rk_ctrl(ce));
>>    }
>>
>>    static void rk_timer_update_counter(unsigned long cycles,
>> @@ -179,4 +186,18 @@ out_unmap:
>>    	iounmap(bc_timer.base);
>>    }
>>
>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>> +static void __init rk3288_timer_init(struct device_node *np)
>> +{
>> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
>> +	rk_timer_init(np);
>> +}
>> +
>> +static void __init rk3399_timer_init(struct device_node *np)
>> +{
>> +        bc_timer.ctrl = TIMER_CONTROL_REG3399;
>> +	rk_timer_init(np);
>> +}
>> +
>> +
>> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer",
>> rk3288_timer_init);
>> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3399-timer",
>> rk3399_timer_init);
>>
>>
>
> I think you mean this patch otherwise compile will fail:
> @@ -19,7 +19,8 @@
>
>   #define TIMER_LOAD_COUNT0	0x00
>   #define TIMER_LOAD_COUNT1	0x04
> -#define TIMER_CONTROL_REG	0x10
> +#define TIMER_CONTROL_REG3288	0x10
> +#define TIMER_CONTROL_REG3399	0x1C
>   #define TIMER_INT_STATUS	0x18
>
>   #define TIMER_DISABLE		0x0
> @@ -31,6 +32,7 @@
>   struct bc_timer {
>   	struct clock_event_device ce;
>   	void __iomem *base;
> +	u32 ctrl;
>   	u32 freq;
>   };
>
> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
> clock_event_device *ce)
>   	return rk_timer(ce)->base;
>   }
>
> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
> +{
> +	return rk_timer(ce)->base + rk_timer(ce)->ctrl;

You can do a small optimization by pre-computing 'ctrl' at init time, so 
no need to do this addition each time.

> +}
> +
>   static inline void rk_timer_disable(struct clock_event_device *ce)
>   {
> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>   }
>
>   static inline void rk_timer_enable(struct clock_event_device *ce, u32
> flags)
>   {
>   	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
> -		       rk_base(ce) + TIMER_CONTROL_REG);
> +		       rk_ctrl(ce));
>   }
>
>   static void rk_timer_update_counter(unsigned long cycles,
> @@ -179,4 +186,19 @@ out_unmap:
>   	iounmap(bc_timer.base);
>   }
>
> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
> +static void __init rk3288_timer_init(struct device_node *np)
> +{
> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
> +	rk_timer_init(np);

	rk_timer_init(np);
	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;

> +}
> +
> +static void __init rk3399_timer_init(struct device_node *np)
> +{
> +	bc_timer.ctrl = TIMER_CONTROL_REG3399;
> +	rk_timer_init(np);

	rk_timer_init(np);
	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3399;

> +}
> +
> +CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
> +		       rk3288_timer_init);
> +CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer",
> +		       rk3399_timer_init);
>
> This patch will give us a little lager text size. If we do disassemble,
> we can see additional LDR is called. I can accept this performance drop.
> So we will send new patches.
> BTW, the patch "clocksource: rockchip: remove unnecessary clear irq
> before request_irq" can drop if we use this patch.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1410738

From"Huang, Tao" <huangtao@rock-chips.com>
Date2016-06-01 04:00 +0200
Message-ID<rF3BU-1SI-5@gated-at.bofh.it>
In reply to#1410340
Hi Daniel:
On 2016年05月31日 22:06, Daniel Lezcano wrote:
>>
>> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
>> clock_event_device *ce)
>>   	return rk_timer(ce)->base;
>>   }
>>
>> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
>> +{
>> +	return rk_timer(ce)->base + rk_timer(ce)->ctrl;
> 
> You can do a small optimization by pre-computing 'ctrl' at init time, so 
> no need to do this addition each time.

I understand what you mean, please see comment below.
And even we use ctrl as pointer, we still will get addition LDR other
then ADD.
This is disassemble code before:
   0:	f9408021 	ldr	x1, [x1,#256]
   4:	52800003 	mov	w3, #0x0
   8:	91004022 	add	x2, x1, #0x10
   c:	b9000043 	str	w3, [x2]
This is disassemble code after change:
   0:	52800003 	mov	w3, #0x0
   4:	f9408422 	ldr	x2, [x1,#264]
   8:	b9000043 	str	w3, [x2]
   c:	f9408021 	ldr	x1, [x1,#256]
Of course we can assume cache hit.

> 
>> +}
>> +
>>   static inline void rk_timer_disable(struct clock_event_device *ce)
>>   {
>> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
>> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>>   }
>>
>>   static inline void rk_timer_enable(struct clock_event_device *ce, u32
>> flags)
>>   {
>>   	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
>> -		       rk_base(ce) + TIMER_CONTROL_REG);
>> +		       rk_ctrl(ce));
>>   }
>>
>>   static void rk_timer_update_counter(unsigned long cycles,
>> @@ -179,4 +186,19 @@ out_unmap:
>>   	iounmap(bc_timer.base);
>>   }
>>
>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>> +static void __init rk3288_timer_init(struct device_node *np)
>> +{
>> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
>> +	rk_timer_init(np);
> 
> 	rk_timer_init(np);
> 	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;

No. It's not such simple. You will access null pointer when
rk_timer_init, if we keep rk_timer_disable call in init or after
request_irq/clockevents_config_and_register and interrupt happen
immediately.

So the code maybe:
static void __init rk3288_timer_init(struct device_node *np)
{
	bc_timer.base = of_iomap(np, 0);
	if (!bc_timer.base) {
		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
		return;
	}
	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
	rk_imter_init(np); // of course remove of_iomap from init.

Is this what you want?

[toc] | [prev] | [next] | [standalone]

#1410844

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2016-06-01 08:20 +0200
Message-ID<rF7Fv-4MU-5@gated-at.bofh.it>
In reply to#1410738
On 06/01/2016 03:58 AM, Huang, Tao wrote:
> Hi Daniel:
> On 2016年05月31日 22:06, Daniel Lezcano wrote:

[ ... ]

>>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>>> +static void __init rk3288_timer_init(struct device_node *np)
>>> +{
>>> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
>>> +	rk_timer_init(np);
>>
>> 	rk_timer_init(np);
>> 	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
>
> No. It's not such simple. You will access null pointer when
> rk_timer_init, if we keep rk_timer_disable call in init or after
> request_irq/clockevents_config_and_register and interrupt happen
> immediately.
>
> So the code maybe:
> static void __init rk3288_timer_init(struct device_node *np)
> {
> 	bc_timer.base = of_iomap(np, 0);
> 	if (!bc_timer.base) {
> 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
> 		return;
> 	}
> 	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
> 	rk_imter_init(np); // of course remove of_iomap from init.
>
> Is this what you want?

Not necessarily. There are plenty of variants.

eg. rk_timer_init(np, TIMER_CONTROL_REG3288);



-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web