[PATCH 0/4] Improve CLKSRC_OF matching

[PATCH 0/4] Improve CLKSRC_OF matching

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

In the recently added support for OF based clocksource init, a device node
will be matched twice. We can fix this by passing the device node to the
init functions and removing the match functions within the init functions.

This is based on arm-soc for-next branch and commit "of: fix incorrect
return value of of_find_matching_node_and_match()" in my DT for-next
branch.

Rob

Rob Herring (4):
  clocksource: pass DT node pointer to init functions
  clocksource: bcm2835: use the device_node pointer passed to init
  clocksource: vt8500: use the device_node pointer passed to init
  clocksource: tegra20: use the device_node pointer passed to init

 drivers/clocksource/bcm2835_timer.c |   12 +-----
 drivers/clocksource/clksrc-of.c     |    4 +-
 drivers/clocksource/tegra20_timer.c |   70 ++++++++++++++---------------------
 drivers/clocksource/vt8500_timer.c  |   14 +------
 4 files changed, 31 insertions(+), 69 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] clocksource: pass DT node pointer to init functions

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

In cases where we have multiple nodes of the same type, we may need the
node pointer to know which node was matched. Passing the node pointer
also keeps the init function from having to match the node a 2nd time.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/clocksource/clksrc-of.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/clksrc-of.c b/drivers/clocksource/clksrc-of.c
index bdabdaa..3ef11fb 100644
--- a/drivers/clocksource/clksrc-of.c
+++ b/drivers/clocksource/clksrc-of.c
@@ -26,10 +26,10 @@ void __init clocksource_of_init(void)
 {
 	struct device_node *np;
 	const struct of_device_id *match;
-	void (*init_func)(void);
+	void (*init_func)(struct device_node *);
 
 	for_each_matching_node_and_match(np, __clksrc_of_table, &match) {
 		init_func = match->data;
-		init_func();
+		init_func(np);
 	}
 }
-- 
1.7.10.4

[PATCH 2/4] clocksource: bcm2835: use the device_node pointer passed to init

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

We've already matched the node, so use the node pointer passed in.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/clocksource/bcm2835_timer.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/clocksource/bcm2835_timer.c b/drivers/clocksource/bcm2835_timer.c
index 50c68fe..766611d 100644
--- a/drivers/clocksource/bcm2835_timer.c
+++ b/drivers/clocksource/bcm2835_timer.c
@@ -95,23 +95,13 @@ static irqreturn_t bcm2835_time_interrupt(int irq, void *dev_id)
 	}
 }
 
-static struct of_device_id bcm2835_time_match[] __initconst = {
-	{ .compatible = "brcm,bcm2835-system-timer" },
-	{}
-};
-
-static void __init bcm2835_timer_init(void)
+static void __init bcm2835_timer_init(struct device_node *node)
 {
-	struct device_node *node;
 	void __iomem *base;
 	u32 freq;
 	int irq;
 	struct bcm2835_timer *timer;
 
-	node = of_find_matching_node(NULL, bcm2835_time_match);
-	if (!node)
-		panic("No bcm2835 timer node");
-
 	base = of_iomap(node, 0);
 	if (!base)
 		panic("Can't remap registers");
-- 
1.7.10.4

[PATCH 3/4] clocksource: vt8500: use the device_node pointer passed to init

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

We've already matched the node, so use the node pointer passed in.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/clocksource/vt8500_timer.c |   14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index 8efc86b..2422552 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -129,22 +129,10 @@ static struct irqaction irq = {
 	.dev_id  = &clockevent,
 };
 
-static struct of_device_id vt8500_timer_ids[] = {
-	{ .compatible = "via,vt8500-timer" },
-	{ }
-};
-
-static void __init vt8500_timer_init(void)
+static void __init vt8500_timer_init(struct device_node *np)
 {
-	struct device_node *np;
 	int timer_irq;
 
-	np = of_find_matching_node(NULL, vt8500_timer_ids);
-	if (!np) {
-		pr_err("%s: Timer description missing from Device Tree\n",
-								__func__);
-		return;
-	}
 	regbase = of_iomap(np, 0);
 	if (!regbase) {
 		pr_err("%s: Missing iobase description in Device Tree\n",
-- 
1.7.10.4

[PATCH 4/4] clocksource: tegra20: use the device_node pointer passed to init

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

We've already matched the node, so use the node pointer passed in. The rtc
init was intermingled with the timer init, so split this out to a separate
init function.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
Stephen,

It doesn't seem like there are any init ordering issues to me, but please
test/comment.

Rob

 drivers/clocksource/tegra20_timer.c |   70 ++++++++++++++---------------------
 1 file changed, 27 insertions(+), 43 deletions(-)

diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
index 0bde03f..15cc723 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -154,29 +154,12 @@ static struct irqaction tegra_timer_irq = {
 	.dev_id		= &tegra_clockevent,
 };
 
-static const struct of_device_id timer_match[] __initconst = {
-	{ .compatible = "nvidia,tegra20-timer" },
-	{}
-};
-
-static const struct of_device_id rtc_match[] __initconst = {
-	{ .compatible = "nvidia,tegra20-rtc" },
-	{}
-};
-
-static void __init tegra20_init_timer(void)
+static void __init tegra20_init_timer(struct device_node *np)
 {
-	struct device_node *np;
 	struct clk *clk;
 	unsigned long rate;
 	int ret;
 
-	np = of_find_matching_node(NULL, timer_match);
-	if (!np) {
-		pr_err("Failed to find timer DT node\n");
-		BUG();
-	}
-
 	timer_reg_base = of_iomap(np, 0);
 	if (!timer_reg_base) {
 		pr_err("Can't map timer registers\n");
@@ -200,30 +183,6 @@ static void __init tegra20_init_timer(void)
 
 	of_node_put(np);
 
-	np = of_find_matching_node(NULL, rtc_match);
-	if (!np) {
-		pr_err("Failed to find RTC DT node\n");
-		BUG();
-	}
-
-	rtc_base = of_iomap(np, 0);
-	if (!rtc_base) {
-		pr_err("Can't map RTC registers");
-		BUG();
-	}
-
-	/*
-	 * rtc registers are used by read_persistent_clock, keep the rtc clock
-	 * enabled
-	 */
-	clk = clk_get_sys("rtc-tegra", NULL);
-	if (IS_ERR(clk))
-		pr_warn("Unable to get rtc-tegra clock\n");
-	else
-		clk_prepare_enable(clk);
-
-	of_node_put(np);
-
 	switch (rate) {
 	case 12000000:
 		timer_writel(0x000b, TIMERUS_USEC_CFG);
@@ -262,9 +221,34 @@ static void __init tegra20_init_timer(void)
 #ifdef CONFIG_HAVE_ARM_TWD
 	twd_local_timer_of_register();
 #endif
+}
+CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
+
+static void __init tegra20_init_rtc(struct device_node *np)
+{
+	struct clk *clk;
+
+	rtc_base = of_iomap(np, 0);
+	if (!rtc_base) {
+		pr_err("Can't map RTC registers");
+		BUG();
+	}
+
+	/*
+	 * rtc registers are used by read_persistent_clock, keep the rtc clock
+	 * enabled
+	 */
+	clk = clk_get_sys("rtc-tegra", NULL);
+	if (IS_ERR(clk))
+		pr_warn("Unable to get rtc-tegra clock\n");
+	else
+		clk_prepare_enable(clk);
+
+	of_node_put(np);
+
 	register_persistent_clock(NULL, tegra_read_persistent_clock);
 }
-CLOCKSOURCE_OF_DECLARE(tegra20, "nvidia,tegra20-timer", tegra20_init_timer);
+CLOCKSOURCE_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
 
 #ifdef CONFIG_PM
 static u32 usec_config;
-- 
1.7.10.4

Re: [PATCH 4/4] clocksource: tegra20: use the device_node pointer passed to init

[Stephen Warren]

On 02/07/2013 12:09 PM, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> We've already matched the node, so use the node pointer passed in. The rtc
> init was intermingled with the timer init, so split this out to a separate
> init function.

The series,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

Patches 1,4:
Tested-by: Stephen Warren <swarren@nvidia.com>

One thing I wonder re: patch 4 - I know someone (I think Hiroshi, now
CC'd) planned to refactor drivers/clocksource/tegra20_timer.c to enhance
it for Tegra114. I'd like to check with him that the refactoring in this
patch won't impede that at all.

Re: [PATCH 4/4] clocksource: tegra20: use the device_node pointer passed to init

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 7 Feb 2013 20:39:40 +0100:

> On 02/07/2013 12:09 PM, Rob Herring wrote:
> > From: Rob Herring <rob.herring@calxeda.com>
> > 
> > We've already matched the node, so use the node pointer passed in. The rtc
> > init was intermingled with the timer init, so split this out to a separate
> > init function.
> 
> The series,
> Reviewed-by: Stephen Warren <swarren@nvidia.com>
> 
> Patches 1,4:
> Tested-by: Stephen Warren <swarren@nvidia.com>
> 
> One thing I wonder re: patch 4 - I know someone (I think Hiroshi, now
> CC'd) planned to refactor drivers/clocksource/tegra20_timer.c to enhance
> it for Tegra114. I'd like to check with him that the refactoring in this
> patch won't impede that at all.

Actually this covers RTC part. I'll rework mine for the rest on the
top of this.

Re: [PATCH 0/4] Improve CLKSRC_OF matching

[Arnd Bergmann]

On Thursday 07 February 2013, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> In the recently added support for OF based clocksource init, a device node
> will be matched twice. We can fix this by passing the device node to the
> init functions and removing the match functions within the init functions.
> 
> This is based on arm-soc for-next branch and commit "of: fix incorrect
> return value of of_find_matching_node_and_match()" in my DT for-next
> branch.
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

Conceptually this is definitely the way to go, but I noticed that
you create build warnings for the bisection points after the
first patch. I would suggest actually merging the first three
patches into one and also changing the prototype for the tegra
function in that patch to avoid this.

	Arnd

Re: [PATCH 0/4] Improve CLKSRC_OF matching

[Rob Herring]

On 02/07/2013 04:44 PM, Arnd Bergmann wrote:
> On Thursday 07 February 2013, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> In the recently added support for OF based clocksource init, a device node
>> will be matched twice. We can fix this by passing the device node to the
>> init functions and removing the match functions within the init functions.
>>
>> This is based on arm-soc for-next branch and commit "of: fix incorrect
>> return value of of_find_matching_node_and_match()" in my DT for-next
>> branch.
>>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> Conceptually this is definitely the way to go, but I noticed that
> you create build warnings for the bisection points after the
> first patch. I would suggest actually merging the first three
> patches into one and also changing the prototype for the tegra
> function in that patch to avoid this.
> 

How so? I don't see a warning as there is no type checking on the init
function since of_device_id.data is just a void *. It would be good to
have type checking here if you know a way, but I don't. I agree it is a
bit abusive and can change it as you suggest.

Rob

Re: [PATCH 0/4] Improve CLKSRC_OF matching

[Arnd Bergmann]

On Thursday 07 February 2013, Rob Herring wrote:
> How so? I don't see a warning as there is no type checking on the init
> function since of_device_id.data is just a void *. It would be good to
> have type checking here if you know a way, but I don't.

Ah, that's right. So it silently builds find even though the
prototypes don't match, and given the C calling conventions
it even ends up working correctly, it just feels wrong.

	Arnd

Re: [PATCH 0/4] Improve CLKSRC_OF matching

[Tony Prisk]

On Thu, 2013-02-07 at 13:09 -0600, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> In the recently added support for OF based clocksource init, a device node
> will be matched twice. We can fix this by passing the device node to the
> init functions and removing the match functions within the init functions.
> 
> This is based on arm-soc for-next branch and commit "of: fix incorrect
> return value of of_find_matching_node_and_match()" in my DT for-next
> branch.
> 
> Rob
> 
> Rob Herring (4):
>   clocksource: pass DT node pointer to init functions
>   clocksource: bcm2835: use the device_node pointer passed to init
>   clocksource: vt8500: use the device_node pointer passed to init
>   clocksource: tegra20: use the device_node pointer passed to init
> 
>  drivers/clocksource/bcm2835_timer.c |   12 +-----
>  drivers/clocksource/clksrc-of.c     |    4 +-
>  drivers/clocksource/tegra20_timer.c |   70 ++++++++++++++---------------------
>  drivers/clocksource/vt8500_timer.c  |   14 +------
>  4 files changed, 31 insertions(+), 69 deletions(-)
> 

Looks fine, although I didn't get a CC for the vt8500 patch so I had to
go hunting for it :)

For patches 1 & 3:
Acked-by: Tony Prisk <linux@prisktech.co.nz>

Re: [PATCH 0/4] Improve CLKSRC_OF matching

[Rob Herring]

On 02/07/2013 10:51 PM, Tony Prisk wrote:
> On Thu, 2013-02-07 at 13:09 -0600, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> In the recently added support for OF based clocksource init, a device node
>> will be matched twice. We can fix this by passing the device node to the
>> init functions and removing the match functions within the init functions.
>>
>> This is based on arm-soc for-next branch and commit "of: fix incorrect
>> return value of of_find_matching_node_and_match()" in my DT for-next
>> branch.
>>
>> Rob
>>
>> Rob Herring (4):
>>   clocksource: pass DT node pointer to init functions
>>   clocksource: bcm2835: use the device_node pointer passed to init
>>   clocksource: vt8500: use the device_node pointer passed to init
>>   clocksource: tegra20: use the device_node pointer passed to init
>>
>>  drivers/clocksource/bcm2835_timer.c |   12 +-----
>>  drivers/clocksource/clksrc-of.c     |    4 +-
>>  drivers/clocksource/tegra20_timer.c |   70 ++++++++++++++---------------------
>>  drivers/clocksource/vt8500_timer.c  |   14 +------
>>  4 files changed, 31 insertions(+), 69 deletions(-)
>>
> 
> Looks fine, although I didn't get a CC for the vt8500 patch so I had to
> go hunting for it :)

Since it got moved to drivers/clocksource, did MAINTAINERS get updated?
I just ran get_maintainers.pl.

Stephen also needs to add himself to Tegra as I had to add him manually.

Rob

> 
> For patches 1 & 3:
> Acked-by: Tony Prisk <linux@prisktech.co.nz>
> 

Re: [PATCH 2/4] clocksource: bcm2835: use the device_node pointer passed to init

[Stephen Warren]

On 02/07/2013 12:09 PM, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> We've already matched the node, so use the node pointer passed in.

Patches 1 & 2,

Tested-by: Stephen Warren <swarren@wwwdotorg.org>

(i.e. with my bcm2835 hat on:-)

Re: [PATCH 1/4] clocksource: pass DT node pointer to init functions

[Michal Simek]

2013/2/7 Rob Herring <robherring2@gmail.com>:
> From: Rob Herring <rob.herring@calxeda.com>
>
> In cases where we have multiple nodes of the same type, we may need the
> node pointer to know which node was matched. Passing the node pointer
> also keeps the init function from having to match the node a 2nd time.
>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/clocksource/clksrc-of.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Tested-by: Michal Simek <michal.simek@xilinx.com>

The rest is just the same as I have done. Any option to add these
patches to v3.9?
Because I need these patches for zynq timer because we have two in the soc.
Is it OK to register several clock source and clockevent devices?

btw: there is still one issue because you can just setup only one
compatibility string.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH 1/4] clocksource: pass DT node pointer to init functions

[Rob Herring]

On 02/13/2013 10:21 AM, Michal Simek wrote:
> 2013/2/7 Rob Herring <robherring2@gmail.com>:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> In cases where we have multiple nodes of the same type, we may need the
>> node pointer to know which node was matched. Passing the node pointer
>> also keeps the init function from having to match the node a 2nd time.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> Cc: John Stultz <johnstul@us.ibm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  drivers/clocksource/clksrc-of.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Tested-by: Michal Simek <michal.simek@xilinx.com>
> 
> The rest is just the same as I have done. Any option to add these
> patches to v3.9?

I would like to before we have more users to fix, but it will have to be
post rc1. If not, Arnd/Olof should be be able to provide a stable branch
for 3.10.

> Because I need these patches for zynq timer because we have two in the soc.
> Is it OK to register several clock source and clockevent devices?

If it is 1 DT node, then that should be fine.

> btw: there is still one issue because you can just setup only one
> compatibility string.

You can have multiple CLOCKSOURCE_OF_DECLARE statements. The gic code
does this for irqchips.

Rob

Re: [PATCH 1/4] clocksource: pass DT node pointer to init functions

[Michal Simek]

2013/2/13 Rob Herring <robherring2@gmail.com>:
> On 02/13/2013 10:21 AM, Michal Simek wrote:
>> 2013/2/7 Rob Herring <robherring2@gmail.com>:
>>> From: Rob Herring <rob.herring@calxeda.com>
>>>
>>> In cases where we have multiple nodes of the same type, we may need the
>>> node pointer to know which node was matched. Passing the node pointer
>>> also keeps the init function from having to match the node a 2nd time.
>>>
>>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>>> Cc: John Stultz <johnstul@us.ibm.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> ---
>>>  drivers/clocksource/clksrc-of.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Tested-by: Michal Simek <michal.simek@xilinx.com>
>>
>> The rest is just the same as I have done. Any option to add these
>> patches to v3.9?
>
> I would like to before we have more users to fix, but it will have to be
> post rc1. If not, Arnd/Olof should be be able to provide a stable branch
> for 3.10.

ok

>> Because I need these patches for zynq timer because we have two in the soc.
>> Is it OK to register several clock source and clockevent devices?
>
> If it is 1 DT node, then that should be fine.

zynq is using two triple timer counter IP . There are also described by two
different DT nodes because there are separated and uses different baseaddresses.

Does it mean that if there are 2 DT nodes that it won't work?


One more thing. Is there any rule which should describe which timer should be
used for clockevent and for clocksource?


>> btw: there is still one issue because you can just setup only one
>> compatibility string.
>
> You can have multiple CLOCKSOURCE_OF_DECLARE statements. The gic code
> does this for irqchips.

Ok. Will look at it.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH 1/4] clocksource: pass DT node pointer to init functions

[Rob Herring]

On 02/13/2013 11:33 AM, Michal Simek wrote:
> 2013/2/13 Rob Herring <robherring2@gmail.com>:
>> On 02/13/2013 10:21 AM, Michal Simek wrote:
>>> 2013/2/7 Rob Herring <robherring2@gmail.com>:
>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>
>>>> In cases where we have multiple nodes of the same type, we may need the
>>>> node pointer to know which node was matched. Passing the node pointer
>>>> also keeps the init function from having to match the node a 2nd time.
>>>>
>>>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>>>> Cc: John Stultz <johnstul@us.ibm.com>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> ---
>>>>  drivers/clocksource/clksrc-of.c |    4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> Tested-by: Michal Simek <michal.simek@xilinx.com>
>>>
>>> The rest is just the same as I have done. Any option to add these
>>> patches to v3.9?
>>
>> I would like to before we have more users to fix, but it will have to be
>> post rc1. If not, Arnd/Olof should be be able to provide a stable branch
>> for 3.10.
> 
> ok

I now see you were trying to get zynq changes in for 3.9. You could add
this patch to your pull request. As is, it is not dependent on some DT
code changes, but the subsequent patches are. I can send the rest after
rc1. It's a bit of a hack with the function call prototype, but nothing
actually breaks. I was going to combine as Arnd suggested, but either
way is probably fine.

> 
>>> Because I need these patches for zynq timer because we have two in the soc.
>>> Is it OK to register several clock source and clockevent devices?
>>
>> If it is 1 DT node, then that should be fine.
> 
> zynq is using two triple timer counter IP . There are also described by two
> different DT nodes because there are separated and uses different baseaddresses.
> 
> Does it mean that if there are 2 DT nodes that it won't work?
> 
> 
> One more thing. Is there any rule which should describe which timer should be
> used for clockevent and for clocksource?

No. This is a common problem. A simple solution is a "linux,clockevent"
property, but I want to avoid that. Ultimately it is some feature of the
h/w that makes you choose. This could be it has an interrupt or not,
higher frequency, has timer compare pins, gets power gated, etc. So you
should describe enough of the h/w properties to make this decision. OMAP
is an example doing this with lots of timers with varying integration
level differences.

Rob

> 
> 
>>> btw: there is still one issue because you can just setup only one
>>> compatibility string.
>>
>> You can have multiple CLOCKSOURCE_OF_DECLARE statements. The gic code
>> does this for irqchips.
> 
> Ok. Will look at it.
> 
> Thanks,
> Michal
> 
> 

Re: [PATCH 1/4] clocksource: pass DT node pointer to init functions

[Michal Simek]

2013/2/14 Rob Herring <robherring2@gmail.com>:
> On 02/13/2013 11:33 AM, Michal Simek wrote:
>> 2013/2/13 Rob Herring <robherring2@gmail.com>:
>>> On 02/13/2013 10:21 AM, Michal Simek wrote:
>>>> 2013/2/7 Rob Herring <robherring2@gmail.com>:
>>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>>
>>>>> In cases where we have multiple nodes of the same type, we may need the
>>>>> node pointer to know which node was matched. Passing the node pointer
>>>>> also keeps the init function from having to match the node a 2nd time.
>>>>>
>>>>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>>>>> Cc: John Stultz <johnstul@us.ibm.com>
>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>> ---
>>>>>  drivers/clocksource/clksrc-of.c |    4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> Tested-by: Michal Simek <michal.simek@xilinx.com>
>>>>
>>>> The rest is just the same as I have done. Any option to add these
>>>> patches to v3.9?
>>>
>>> I would like to before we have more users to fix, but it will have to be
>>> post rc1. If not, Arnd/Olof should be be able to provide a stable branch
>>> for 3.10.
>>
>> ok
>
> I now see you were trying to get zynq changes in for 3.9. You could add
> this patch to your pull request. As is, it is not dependent on some DT
> code changes, but the subsequent patches are. I can send the rest after
> rc1. It's a bit of a hack with the function call prototype, but nothing
> actually breaks. I was going to combine as Arnd suggested, but either
> way is probably fine.

It is not big deal with that. I just want to use this patch. No problem to keep
it in my repo. I am not worried about. The main point for me is that
the patch exists
and I can use it.


>>>> Because I need these patches for zynq timer because we have two in the soc.
>>>> Is it OK to register several clock source and clockevent devices?
>>>
>>> If it is 1 DT node, then that should be fine.
>>
>> zynq is using two triple timer counter IP . There are also described by two
>> different DT nodes because there are separated and uses different baseaddresses.
>>
>> Does it mean that if there are 2 DT nodes that it won't work?
>>
>>
>> One more thing. Is there any rule which should describe which timer should be
>> used for clockevent and for clocksource?
>
> No. This is a common problem. A simple solution is a "linux,clockevent"
> property, but I want to avoid that.

Let me describe it a little bit more. I am going to change current
mainline implementation
for zynq timer which uses special compatible string to define
clocksource and clockevent
device. I don't think this is right way to go because compatible
string shouldn't point
to device usage. Which is exact case you wanted to avoid.

> Ultimately it is some feature of the
> h/w that makes you choose. This could be it has an interrupt or not,
> higher frequency, has timer compare pins, gets power gated, etc. So you
> should describe enough of the h/w properties to make this decision.

For different timer type, kernel should decide which timer should be
used. It should be easy to test
because I can add some timers to the programmable logic (as is done
for Microblaze) and check
how kernel decide which clocksource/clockevent device will use. I
believe there is any logic around.


For solution with two instances of the same triple timer counters it
is impossible
to specify additional h/w property because all of 6 timers are the same.
And also adding special parameter to IP goes against rule
that device-tree should describe hw not Linux usage.
Maybe enough to save information to driver that clocksource and
clockevent device is registered
and do not try to register another timer (and also another timer from
another instance).

Or maybe it can be done via chosen property or via aliases property where
timer0 alias is that one who should be used for clocksource and
clockevent device.
(for my case alias to one instance of triple timer counter).

I saw some dtses which have aliases timer.

How good/bad is this option?


> OMAP
> is an example doing this with lots of timers with varying integration
> level differences.

Can you point me that that omap drivers you are talking about?

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH 1/4] clocksource: pass DT node pointer to init functions

[Rob Herring]

On 02/14/2013 12:45 AM, Michal Simek wrote:
> 2013/2/14 Rob Herring <robherring2@gmail.com>:
>> On 02/13/2013 11:33 AM, Michal Simek wrote:
>>> 2013/2/13 Rob Herring <robherring2@gmail.com>:
>>>> On 02/13/2013 10:21 AM, Michal Simek wrote:
>>>>> 2013/2/7 Rob Herring <robherring2@gmail.com>:
>>>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>>>


>>> One more thing. Is there any rule which should describe which timer should be
>>> used for clockevent and for clocksource?
>>
>> No. This is a common problem. A simple solution is a "linux,clockevent"
>> property, but I want to avoid that.
> 
> Let me describe it a little bit more. I am going to change current
> mainline implementation
> for zynq timer which uses special compatible string to define
> clocksource and clockevent
> device. I don't think this is right way to go because compatible
> string shouldn't point
> to device usage. Which is exact case you wanted to avoid.
> 
>> Ultimately it is some feature of the
>> h/w that makes you choose. This could be it has an interrupt or not,
>> higher frequency, has timer compare pins, gets power gated, etc. So you
>> should describe enough of the h/w properties to make this decision.
> 
> For different timer type, kernel should decide which timer should be
> used. It should be easy to test
> because I can add some timers to the programmable logic (as is done
> for Microblaze) and check
> how kernel decide which clocksource/clockevent device will use. I
> believe there is any logic around.
> 
> 
> For solution with two instances of the same triple timer counters it
> is impossible
> to specify additional h/w property because all of 6 timers are the same.
> And also adding special parameter to IP goes against rule
> that device-tree should describe hw not Linux usage.
> Maybe enough to save information to driver that clocksource and
> clockevent device is registered
> and do not try to register another timer (and also another timer from
> another instance).

So if you don't care which one is used, then use the first one found and
be done with it. You will have to make your init function just return on
subsequent calls.

> 
> Or maybe it can be done via chosen property or via aliases property where
> timer0 alias is that one who should be used for clocksource and
> clockevent device.
> (for my case alias to one instance of triple timer counter).
> 
> I saw some dtses which have aliases timer.
> 
> How good/bad is this option?

It's best if you can avoid aliases IMO.

> 
> 
>> OMAP
>> is an example doing this with lots of timers with varying integration
>> level differences.
> 
> Can you point me that that omap drivers you are talking about?

arch/arm/mach-omap2/timer.c. They have properties such as always on and
secure mode.

Rob