[PATCH] clocksource: sh_cmt: 32-bit control register access prototype

[PATCH] clocksource: sh_cmt: 32-bit control register access prototype

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

This is a 32-bit control register access prototype
for the CMT driver. It is not ready for merge but
it intends to show the reasoning behind the
register access patches included in this series:
"[PATCH 00/08] clocksource: sh_cmt: CMT driver update"

Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/clocksource/sh_cmt.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

--- 0006/drivers/clocksource/sh_cmt.c
+++ work/drivers/clocksource/sh_cmt.c	2012-12-14 11:53:23.000000000 +0900
@@ -79,6 +79,12 @@ struct sh_cmt_priv {
  * CMCSR 0xffca0060 16-bit
  * CMCNT 0xffca0064 32-bit
  * CMCOR 0xffca0068 32-bit
+ *
+ * "32-bit counter and 32-bit control"
+ * CMSTR 0xe6130000 32-bit
+ * CMCSR 0xe6130010 32-bit
+ * CMCNT 0xe6130014 32-bit
+ * CMCOR 0xe6130018 32-bit
  */
 
 static unsigned long sh_cmt_read16(void __iomem *base, unsigned long offs)
@@ -734,8 +740,13 @@ static int sh_cmt_setup(struct sh_cmt_pr
 		goto err1;
 	}
 
+#if 1 /* XXX: filthy prototype hack! */
+	p->read_control = sh_cmt_read32;
+	p->write_control = sh_cmt_write32;
+#else
 	p->read_control = sh_cmt_read16;
 	p->write_control = sh_cmt_write16;
+#endif
 
 	if (resource_size(res) = 6) {
 		p->width = 16;

[PATCH] clocksource: sh_cmt: 32-bit control register support

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add support for CMT hardware with 32-bit control and counter
registers, as found on r8a73a4 and r8a7790. To use the CMT
with 32-bit hardware a second I/O memory resource needs to
point out the CMSTR register and it needs to be 32 bit wide.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 Tested on r8a73a4 used on APE6EVM.

 drivers/clocksource/sh_cmt.c |   50 ++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 14 deletions(-)

--- 0001/drivers/clocksource/sh_cmt.c
+++ work/drivers/clocksource/sh_cmt.c	2013-06-17 13:47:34.000000000 +0900
@@ -37,6 +37,7 @@
 
 struct sh_cmt_priv {
 	void __iomem *mapbase;
+	void __iomem *mapbase_str;
 	struct clk *clk;
 	unsigned long width; /* 16 or 32 bit version of hardware block */
 	unsigned long overflow_bit;
@@ -79,6 +80,12 @@ struct sh_cmt_priv {
  * CMCSR 0xffca0060 16-bit
  * CMCNT 0xffca0064 32-bit
  * CMCOR 0xffca0068 32-bit
+ *
+ * "32-bit counter and 32-bit control" as found on r8a73a4 and r8a7790:
+ * CMSTR 0xffca0500 32-bit
+ * CMCSR 0xffca0510 32-bit
+ * CMCNT 0xffca0514 32-bit
+ * CMCOR 0xffca0518 32-bit
  */
 
 static unsigned long sh_cmt_read16(void __iomem *base, unsigned long offs)
@@ -109,9 +116,7 @@ static void sh_cmt_write32(void __iomem
 
 static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_priv *p)
 {
-	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
-
-	return p->read_control(p->mapbase - cfg->channel_offset, 0);
+	return p->read_control(p->mapbase_str, 0);
 }
 
 static inline unsigned long sh_cmt_read_cmcsr(struct sh_cmt_priv *p)
@@ -127,9 +132,7 @@ static inline unsigned long sh_cmt_read_
 static inline void sh_cmt_write_cmstr(struct sh_cmt_priv *p,
 				      unsigned long value)
 {
-	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
-
-	p->write_control(p->mapbase - cfg->channel_offset, 0, value);
+	p->write_control(p->mapbase_str, 0, value);
 }
 
 static inline void sh_cmt_write_cmcsr(struct sh_cmt_priv *p,
@@ -676,7 +679,7 @@ static int sh_cmt_register(struct sh_cmt
 static int sh_cmt_setup(struct sh_cmt_priv *p, struct platform_device *pdev)
 {
 	struct sh_timer_config *cfg = pdev->dev.platform_data;
-	struct resource *res;
+	struct resource *res, *res2;
 	int irq, ret;
 	ret = -ENXIO;
 
@@ -694,6 +697,9 @@ static int sh_cmt_setup(struct sh_cmt_pr
 		goto err0;
 	}
 
+	/* optional resource for the shared timer start/stop register */
+	res2 = platform_get_resource(p->pdev, IORESOURCE_MEM, 1);
+
 	irq = platform_get_irq(p->pdev, 0);
 	if (irq < 0) {
 		dev_err(&p->pdev->dev, "failed to get irq\n");
@@ -707,6 +713,15 @@ static int sh_cmt_setup(struct sh_cmt_pr
 		goto err0;
 	}
 
+	/* map second resource for CMSTR */
+	p->mapbase_str = ioremap_nocache(res2 ? res2->start :
+					 res->start - cfg->channel_offset,
+					 res2 ? resource_size(res2) : 2);
+	if (p->mapbase_str = NULL) {
+		dev_err(&p->pdev->dev, "failed to remap I/O second memory\n");
+		goto err1;
+	}
+
 	/* request irq using setup_irq() (too early for request_irq()) */
 	p->irqaction.name = dev_name(&p->pdev->dev);
 	p->irqaction.handler = sh_cmt_interrupt;
@@ -719,11 +734,17 @@ static int sh_cmt_setup(struct sh_cmt_pr
 	if (IS_ERR(p->clk)) {
 		dev_err(&p->pdev->dev, "cannot get clock\n");
 		ret = PTR_ERR(p->clk);
-		goto err1;
+		goto err2;
 	}
 
-	p->read_control = sh_cmt_read16;
-	p->write_control = sh_cmt_write16;
+	if (res2 && (resource_size(res2) = 4)) {
+		/* assume both CMSTR and CMCSR to be 32-bit */
+		p->read_control = sh_cmt_read32;
+		p->write_control = sh_cmt_write32;
+	} else {
+		p->read_control = sh_cmt_read16;
+		p->write_control = sh_cmt_write16;
+	}
 
 	if (resource_size(res) = 6) {
 		p->width = 16;
@@ -752,22 +773,23 @@ static int sh_cmt_setup(struct sh_cmt_pr
 			      cfg->clocksource_rating);
 	if (ret) {
 		dev_err(&p->pdev->dev, "registration failed\n");
-		goto err2;
+		goto err3;
 	}
 	p->cs_enabled = false;
 
 	ret = setup_irq(irq, &p->irqaction);
 	if (ret) {
 		dev_err(&p->pdev->dev, "failed to request irq %d\n", irq);
-		goto err2;
+		goto err3;
 	}
 
 	platform_set_drvdata(pdev, p);
 
 	return 0;
-err2:
+err3:
 	clk_put(p->clk);
-
+err2:
+	iounmap(p->mapbase_str);
 err1:
 	iounmap(p->mapbase);
 err0:

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Laurent Pinchart]

Hi Magnus,

Thanks for the patch.

On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Add support for CMT hardware with 32-bit control and counter
> registers, as found on r8a73a4 and r8a7790. To use the CMT
> with 32-bit hardware a second I/O memory resource needs to
> point out the CMSTR register and it needs to be 32 bit wide.

Is a memory second resource required ? Can't we use a single resource that 
will contain all the registers ?

(and one more comment below)

> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  Tested on r8a73a4 used on APE6EVM.
> 
>  drivers/clocksource/sh_cmt.c |   50 +++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 14 deletions(-)
> 
> --- 0001/drivers/clocksource/sh_cmt.c
> +++ work/drivers/clocksource/sh_cmt.c	2013-06-17 13:47:34.000000000 +0900
> @@ -37,6 +37,7 @@
> 
>  struct sh_cmt_priv {
>  	void __iomem *mapbase;
> +	void __iomem *mapbase_str;
>  	struct clk *clk;
>  	unsigned long width; /* 16 or 32 bit version of hardware block */
>  	unsigned long overflow_bit;
> @@ -79,6 +80,12 @@ struct sh_cmt_priv {
>   * CMCSR 0xffca0060 16-bit
>   * CMCNT 0xffca0064 32-bit
>   * CMCOR 0xffca0068 32-bit
> + *
> + * "32-bit counter and 32-bit control" as found on r8a73a4 and r8a7790:
> + * CMSTR 0xffca0500 32-bit
> + * CMCSR 0xffca0510 32-bit
> + * CMCNT 0xffca0514 32-bit
> + * CMCOR 0xffca0518 32-bit
>   */
> 
>  static unsigned long sh_cmt_read16(void __iomem *base, unsigned long offs)
> @@ -109,9 +116,7 @@ static void sh_cmt_write32(void __iomem
> 
>  static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_priv *p)
>  {
> -	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
> -
> -	return p->read_control(p->mapbase - cfg->channel_offset, 0);
> +	return p->read_control(p->mapbase_str, 0);
>  }
> 
>  static inline unsigned long sh_cmt_read_cmcsr(struct sh_cmt_priv *p)
> @@ -127,9 +132,7 @@ static inline unsigned long sh_cmt_read_
>  static inline void sh_cmt_write_cmstr(struct sh_cmt_priv *p,
>  				      unsigned long value)
>  {
> -	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
> -
> -	p->write_control(p->mapbase - cfg->channel_offset, 0, value);
> +	p->write_control(p->mapbase_str, 0, value);
>  }
> 
>  static inline void sh_cmt_write_cmcsr(struct sh_cmt_priv *p,
> @@ -676,7 +679,7 @@ static int sh_cmt_register(struct sh_cmt
>  static int sh_cmt_setup(struct sh_cmt_priv *p, struct platform_device
> *pdev) {
>  	struct sh_timer_config *cfg = pdev->dev.platform_data;
> -	struct resource *res;
> +	struct resource *res, *res2;
>  	int irq, ret;
>  	ret = -ENXIO;
> 
> @@ -694,6 +697,9 @@ static int sh_cmt_setup(struct sh_cmt_pr
>  		goto err0;
>  	}
> 
> +	/* optional resource for the shared timer start/stop register */
> +	res2 = platform_get_resource(p->pdev, IORESOURCE_MEM, 1);
> +
>  	irq = platform_get_irq(p->pdev, 0);
>  	if (irq < 0) {
>  		dev_err(&p->pdev->dev, "failed to get irq\n");
> @@ -707,6 +713,15 @@ static int sh_cmt_setup(struct sh_cmt_pr
>  		goto err0;
>  	}
> 
> +	/* map second resource for CMSTR */
> +	p->mapbase_str = ioremap_nocache(res2 ? res2->start :
> +					 res->start - cfg->channel_offset,
> +					 res2 ? resource_size(res2) : 2);
> +	if (p->mapbase_str = NULL) {
> +		dev_err(&p->pdev->dev, "failed to remap I/O second memory\n");
> +		goto err1;
> +	}
> +
>  	/* request irq using setup_irq() (too early for request_irq()) */
>  	p->irqaction.name = dev_name(&p->pdev->dev);
>  	p->irqaction.handler = sh_cmt_interrupt;
> @@ -719,11 +734,17 @@ static int sh_cmt_setup(struct sh_cmt_pr
>  	if (IS_ERR(p->clk)) {
>  		dev_err(&p->pdev->dev, "cannot get clock\n");
>  		ret = PTR_ERR(p->clk);
> -		goto err1;
> +		goto err2;
>  	}
> 
> -	p->read_control = sh_cmt_read16;
> -	p->write_control = sh_cmt_write16;
> +	if (res2 && (resource_size(res2) = 4)) {
> +		/* assume both CMSTR and CMCSR to be 32-bit */
> +		p->read_control = sh_cmt_read32;
> +		p->write_control = sh_cmt_write32;
> +	} else {
> +		p->read_control = sh_cmt_read16;
> +		p->write_control = sh_cmt_write16;
> +	}
> 
>  	if (resource_size(res) = 6) {
>  		p->width = 16;
> @@ -752,22 +773,23 @@ static int sh_cmt_setup(struct sh_cmt_pr
>  			      cfg->clocksource_rating);
>  	if (ret) {
>  		dev_err(&p->pdev->dev, "registration failed\n");
> -		goto err2;
> +		goto err3;
>  	}
>  	p->cs_enabled = false;
> 
>  	ret = setup_irq(irq, &p->irqaction);
>  	if (ret) {
>  		dev_err(&p->pdev->dev, "failed to request irq %d\n", irq);
> -		goto err2;
> +		goto err3;
>  	}
> 
>  	platform_set_drvdata(pdev, p);
> 
>  	return 0;
> -err2:
> +err3:
>  	clk_put(p->clk);
> -
> +err2:
> +	iounmap(p->mapbase_str);
>  err1:
>  	iounmap(p->mapbase);

Time to switch to devm_* managed functions ? :-)

>  err0:

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Magnus Damm]

Hi Laurent,

On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thanks for the patch.
>
> On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> Add support for CMT hardware with 32-bit control and counter
>> registers, as found on r8a73a4 and r8a7790. To use the CMT
>> with 32-bit hardware a second I/O memory resource needs to
>> point out the CMSTR register and it needs to be 32 bit wide.
>
> Is a memory second resource required ? Can't we use a single resource that
> will contain all the registers ?

The CMT hardware block comes with a shared timer start stop register
that historically has been left out of the resource. The location of
this register has so far been pointed out by the "channel offset"
platform data member, together with information about which bit that
happens to be assigned to the timer channel. This start stop register
has happened to be kept in the same page of I/O memory as the main
timer channel resource, so at this point we're sort of "lucky" that a
single ioremap() has covered all cases.

With this patch it becomes optional to instead of platform data use a
second resource to point out the timer start/stop register. While we
do that we can also use the size of that resource to determine the I/O
access width, which happens to be something that is needed to enable
the driver on certain SoCs.

> Time to switch to devm_* managed functions ? :-)

Yes, indeed. That among other things, like converting the driver to in
a more optimal way support clock source only or clock event only
configurations. Also, some more modern CMT hardware versions have
extended registers with 48-bit counters, and we can also often use
more high frequency clocks to improve timer resolution.

As you can tell, in general there are many things that can be improved
with this driver. I thought a first shot could be to make it actually
work on more recent CMT hardware with 32-bit only registers. So that's
what this patch does!

Cheers,

/ magnus

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Laurent Pinchart]

Hi Magnus,

On Tuesday 18 June 2013 14:39:38 Magnus Damm wrote:
> On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart wrote:
> > On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> Add support for CMT hardware with 32-bit control and counter
> >> registers, as found on r8a73a4 and r8a7790. To use the CMT
> >> with 32-bit hardware a second I/O memory resource needs to
> >> point out the CMSTR register and it needs to be 32 bit wide.
> > 
> > Is a memory second resource required ? Can't we use a single resource that
> > will contain all the registers ?
> 
> The CMT hardware block comes with a shared timer start stop register
> that historically has been left out of the resource. The location of
> this register has so far been pointed out by the "channel offset"
> platform data member, together with information about which bit that
> happens to be assigned to the timer channel. This start stop register
> has happened to be kept in the same page of I/O memory as the main
> timer channel resource, so at this point we're sort of "lucky" that a
> single ioremap() has covered all cases.
> 
> With this patch it becomes optional to instead of platform data use a
> second resource to point out the timer start/stop register. While we
> do that we can also use the size of that resource to determine the I/O
> access width, which happens to be something that is needed to enable
> the driver on certain SoCs.

OK, I get it now. I've had a quick look at the documentation, and I'm 
wondering whether we shouldn't register a single platform device that span all 
the channels contained in the CMT, instead of registering one platform device 
per channel.

> > Time to switch to devm_* managed functions ? :-)
> 
> Yes, indeed. That among other things, like converting the driver to in
> a more optimal way support clock source only or clock event only
> configurations. Also, some more modern CMT hardware versions have
> extended registers with 48-bit counters, and we can also often use
> more high frequency clocks to improve timer resolution.
> 
> As you can tell, in general there are many things that can be improved
> with this driver. I thought a first shot could be to make it actually
> work on more recent CMT hardware with 32-bit only registers. So that's
> what this patch does!

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Magnus Damm]

Hi Laurent,

On Tue, Jun 18, 2013 at 7:35 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Tuesday 18 June 2013 14:39:38 Magnus Damm wrote:
>> On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart wrote:
>> > On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
>> >> From: Magnus Damm <damm@opensource.se>
>> >>
>> >> Add support for CMT hardware with 32-bit control and counter
>> >> registers, as found on r8a73a4 and r8a7790. To use the CMT
>> >> with 32-bit hardware a second I/O memory resource needs to
>> >> point out the CMSTR register and it needs to be 32 bit wide.
>> >
>> > Is a memory second resource required ? Can't we use a single resource that
>> > will contain all the registers ?
>>
>> The CMT hardware block comes with a shared timer start stop register
>> that historically has been left out of the resource. The location of
>> this register has so far been pointed out by the "channel offset"
>> platform data member, together with information about which bit that
>> happens to be assigned to the timer channel. This start stop register
>> has happened to be kept in the same page of I/O memory as the main
>> timer channel resource, so at this point we're sort of "lucky" that a
>> single ioremap() has covered all cases.
>>
>> With this patch it becomes optional to instead of platform data use a
>> second resource to point out the timer start/stop register. While we
>> do that we can also use the size of that resource to determine the I/O
>> access width, which happens to be something that is needed to enable
>> the driver on certain SoCs.
>
> OK, I get it now. I've had a quick look at the documentation, and I'm
> wondering whether we shouldn't register a single platform device that span all
> the channels contained in the CMT, instead of registering one platform device
> per channel.

I both agree with you and disagree because of the current state of
timers in the linux kernel. I would have liked a single platform
device with all channles if this would be a generic timer driver that
from user space could be configured to associate channels with various
subsystems like PWM, clocksource, clockevent.

At this point the driver is doing clockevent and clocksource only, and
no sane user wants 84 channels of equivalent hardware blocks for those
two. So based on that I'd rather do it like today and let people write
custom drivers for whatever applications they may use the other
channels for.

So if you're in hacking mode, why don't you figure out some way timers
can be configured from user space? =) If so then we can use DT to
describe the actual hardware and let the software policy be decided
via some configuration mechanism.

Cheers,

/ magnus

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Laurent Pinchart]

Hi Magnus,

On Tuesday 18 June 2013 20:54:47 Magnus Damm wrote:
> On Tue, Jun 18, 2013 at 7:35 PM, Laurent Pinchart wrote:
> > On Tuesday 18 June 2013 14:39:38 Magnus Damm wrote:
> >> On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart wrote:
> >> > On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
> >> >> From: Magnus Damm <damm@opensource.se>
> >> >> 
> >> >> Add support for CMT hardware with 32-bit control and counter
> >> >> registers, as found on r8a73a4 and r8a7790. To use the CMT
> >> >> with 32-bit hardware a second I/O memory resource needs to
> >> >> point out the CMSTR register and it needs to be 32 bit wide.
> >> > 
> >> > Is a memory second resource required ? Can't we use a single resource
> >> > that will contain all the registers ?
> >> 
> >> The CMT hardware block comes with a shared timer start stop register
> >> that historically has been left out of the resource. The location of
> >> this register has so far been pointed out by the "channel offset"
> >> platform data member, together with information about which bit that
> >> happens to be assigned to the timer channel. This start stop register
> >> has happened to be kept in the same page of I/O memory as the main
> >> timer channel resource, so at this point we're sort of "lucky" that a
> >> single ioremap() has covered all cases.
> >> 
> >> With this patch it becomes optional to instead of platform data use a
> >> second resource to point out the timer start/stop register. While we
> >> do that we can also use the size of that resource to determine the I/O
> >> access width, which happens to be something that is needed to enable
> >> the driver on certain SoCs.
> > 
> > OK, I get it now. I've had a quick look at the documentation, and I'm
> > wondering whether we shouldn't register a single platform device that span
> > all the channels contained in the CMT, instead of registering one
> > platform device per channel.
> 
> I both agree with you and disagree because of the current state of timers in
> the linux kernel. I would have liked a single platform device with all
> channles if this would be a generic timer driver that from user space could
> be configured to associate channels with various subsystems like PWM,
> clocksource, clockevent.
> 
> At this point the driver is doing clockevent and clocksource only, and no
> sane user wants 84 channels of equivalent hardware blocks for those two.

Of course, but we could always select which channels to register clockevents 
and clocksources for in platform data. That won't fix the overall problem, but 
it's one step forward.

> So based on that I'd rather do it like today and let people write custom
> drivers for whatever applications they may use the other channels for.
> 
> So if you're in hacking mode, why don't you figure out some way timers can
> be configured from user space? =)

I don't have *that* much free time at the moment I'm afraid, and I'm sure you 
know why :-)

> If so then we can use DT to describe the actual hardware and let the
> software policy be decided via some configuration mechanism.

Don't we also need timers during early boot, when userspace isn't available 
yet ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Magnus Damm]

Hi Laurent,

On Tue, Jun 18, 2013 at 9:30 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Tuesday 18 June 2013 20:54:47 Magnus Damm wrote:
>> On Tue, Jun 18, 2013 at 7:35 PM, Laurent Pinchart wrote:
>> > On Tuesday 18 June 2013 14:39:38 Magnus Damm wrote:
>> >> On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart wrote:
>> >> > On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
>> >> >> From: Magnus Damm <damm@opensource.se>
>> >> >>
>> >> >> Add support for CMT hardware with 32-bit control and counter
>> >> >> registers, as found on r8a73a4 and r8a7790. To use the CMT
>> >> >> with 32-bit hardware a second I/O memory resource needs to
>> >> >> point out the CMSTR register and it needs to be 32 bit wide.
>> >> >
>> >> > Is a memory second resource required ? Can't we use a single resource
>> >> > that will contain all the registers ?
>> >>
>> >> The CMT hardware block comes with a shared timer start stop register
>> >> that historically has been left out of the resource. The location of
>> >> this register has so far been pointed out by the "channel offset"
>> >> platform data member, together with information about which bit that
>> >> happens to be assigned to the timer channel. This start stop register
>> >> has happened to be kept in the same page of I/O memory as the main
>> >> timer channel resource, so at this point we're sort of "lucky" that a
>> >> single ioremap() has covered all cases.
>> >>
>> >> With this patch it becomes optional to instead of platform data use a
>> >> second resource to point out the timer start/stop register. While we
>> >> do that we can also use the size of that resource to determine the I/O
>> >> access width, which happens to be something that is needed to enable
>> >> the driver on certain SoCs.
>> >
>> > OK, I get it now. I've had a quick look at the documentation, and I'm
>> > wondering whether we shouldn't register a single platform device that span
>> > all the channels contained in the CMT, instead of registering one
>> > platform device per channel.
>>
>> I both agree with you and disagree because of the current state of timers in
>> the linux kernel. I would have liked a single platform device with all
>> channles if this would be a generic timer driver that from user space could
>> be configured to associate channels with various subsystems like PWM,
>> clocksource, clockevent.
>>
>> At this point the driver is doing clockevent and clocksource only, and no
>> sane user wants 84 channels of equivalent hardware blocks for those two.
>
> Of course, but we could always select which channels to register clockevents
> and clocksources for in platform data. That won't fix the overall problem, but
> it's one step forward.

But that's pretty much what we're doing, but only listing timer
channels that will be used. Of course, moving around things can be
done but I can't see why we want to do that if we have no selection of
drivers for the actual timer channels. Also, each timer channel may
have it's own unique set of possible parent clocks. That's something
we want to tie in to DT together with CCF. Solving those things
together makes sense IMO.

>> So based on that I'd rather do it like today and let people write custom
>> drivers for whatever applications they may use the other channels for.
>>
>> So if you're in hacking mode, why don't you figure out some way timers can
>> be configured from user space? =)
>
> I don't have *that* much free time at the moment I'm afraid, and I'm sure you
> know why :-)

Yes I do, and that's why I asked. =)

>> If so then we can use DT to describe the actual hardware and let the
>> software policy be decided via some configuration mechanism.
>
> Don't we also need timers during early boot, when userspace isn't available
> yet ?

It depends on the rest of the system. It is possible to boot to user
space without a timer, but I don't recommend it. =)

Cheers,

/ magnus

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Simon Horman]

On Tue, Jun 18, 2013 at 10:27:44PM +0900, Magnus Damm wrote:
> Hi Laurent,
> 
> On Tue, Jun 18, 2013 at 9:30 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Magnus,
> >
> > On Tuesday 18 June 2013 20:54:47 Magnus Damm wrote:
> >> On Tue, Jun 18, 2013 at 7:35 PM, Laurent Pinchart wrote:
> >> > On Tuesday 18 June 2013 14:39:38 Magnus Damm wrote:
> >> >> On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart wrote:
> >> >> > On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
> >> >> >> From: Magnus Damm <damm@opensource.se>
> >> >> >>
> >> >> >> Add support for CMT hardware with 32-bit control and counter
> >> >> >> registers, as found on r8a73a4 and r8a7790. To use the CMT
> >> >> >> with 32-bit hardware a second I/O memory resource needs to
> >> >> >> point out the CMSTR register and it needs to be 32 bit wide.
> >> >> >
> >> >> > Is a memory second resource required ? Can't we use a single resource
> >> >> > that will contain all the registers ?
> >> >>
> >> >> The CMT hardware block comes with a shared timer start stop register
> >> >> that historically has been left out of the resource. The location of
> >> >> this register has so far been pointed out by the "channel offset"
> >> >> platform data member, together with information about which bit that
> >> >> happens to be assigned to the timer channel. This start stop register
> >> >> has happened to be kept in the same page of I/O memory as the main
> >> >> timer channel resource, so at this point we're sort of "lucky" that a
> >> >> single ioremap() has covered all cases.
> >> >>
> >> >> With this patch it becomes optional to instead of platform data use a
> >> >> second resource to point out the timer start/stop register. While we
> >> >> do that we can also use the size of that resource to determine the I/O
> >> >> access width, which happens to be something that is needed to enable
> >> >> the driver on certain SoCs.
> >> >
> >> > OK, I get it now. I've had a quick look at the documentation, and I'm
> >> > wondering whether we shouldn't register a single platform device that span
> >> > all the channels contained in the CMT, instead of registering one
> >> > platform device per channel.
> >>
> >> I both agree with you and disagree because of the current state of timers in
> >> the linux kernel. I would have liked a single platform device with all
> >> channles if this would be a generic timer driver that from user space could
> >> be configured to associate channels with various subsystems like PWM,
> >> clocksource, clockevent.
> >>
> >> At this point the driver is doing clockevent and clocksource only, and no
> >> sane user wants 84 channels of equivalent hardware blocks for those two.
> >
> > Of course, but we could always select which channels to register clockevents
> > and clocksources for in platform data. That won't fix the overall problem, but
> > it's one step forward.
> 
> But that's pretty much what we're doing, but only listing timer
> channels that will be used. Of course, moving around things can be
> done but I can't see why we want to do that if we have no selection of
> drivers for the actual timer channels. Also, each timer channel may
> have it's own unique set of possible parent clocks. That's something
> we want to tie in to DT together with CCF. Solving those things
> together makes sense IMO.
> 
> >> So based on that I'd rather do it like today and let people write custom
> >> drivers for whatever applications they may use the other channels for.
> >>
> >> So if you're in hacking mode, why don't you figure out some way timers can
> >> be configured from user space? =)
> >
> > I don't have *that* much free time at the moment I'm afraid, and I'm sure you
> > know why :-)
> 
> Yes I do, and that's why I asked. =)
> 
> >> If so then we can use DT to describe the actual hardware and let the
> >> software policy be decided via some configuration mechanism.
> >
> > Don't we also need timers during early boot, when userspace isn't available
> > yet ?
> 
> It depends on the rest of the system. It is possible to boot to user
> space without a timer, but I don't recommend it. =)

Hi,

I am holding off on this patch until some consensus is reached.

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Laurent Pinchart]

Hi Simon,

On Wednesday 19 June 2013 21:31:23 Simon Horman wrote:
> On Tue, Jun 18, 2013 at 10:27:44PM +0900, Magnus Damm wrote:
> > On Tue, Jun 18, 2013 at 9:30 PM, Laurent Pinchart wrote:
> > > On Tuesday 18 June 2013 20:54:47 Magnus Damm wrote:
> > >> On Tue, Jun 18, 2013 at 7:35 PM, Laurent Pinchart wrote:
> > >> > On Tuesday 18 June 2013 14:39:38 Magnus Damm wrote:
> > >> >> On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart wrote:
> > >> >> > On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
> > >> >> >> From: Magnus Damm <damm@opensource.se>
> > >> >> >> 
> > >> >> >> Add support for CMT hardware with 32-bit control and counter
> > >> >> >> registers, as found on r8a73a4 and r8a7790. To use the CMT
> > >> >> >> with 32-bit hardware a second I/O memory resource needs to
> > >> >> >> point out the CMSTR register and it needs to be 32 bit wide.
> > >> >> > 
> > >> >> > Is a memory second resource required ? Can't we use a single
> > >> >> > resource that will contain all the registers ?
> > >> >> 
> > >> >> The CMT hardware block comes with a shared timer start stop register
> > >> >> that historically has been left out of the resource. The location of
> > >> >> this register has so far been pointed out by the "channel offset"
> > >> >> platform data member, together with information about which bit that
> > >> >> happens to be assigned to the timer channel. This start stop
> > >> >> register has happened to be kept in the same page of I/O memory as
> > >> >> the main timer channel resource, so at this point we're sort of
> > >> >> "lucky" that a single ioremap() has covered all cases.
> > >> >> 
> > >> >> With this patch it becomes optional to instead of platform data use
> > >> >> a second resource to point out the timer start/stop register. While
> > >> >> we do that we can also use the size of that resource to determine
> > >> >> the I/O access width, which happens to be something that is needed
> > >> >> to enable the driver on certain SoCs.
> > >> > 
> > >> > OK, I get it now. I've had a quick look at the documentation, and I'm
> > >> > wondering whether we shouldn't register a single platform device that
> > >> > span all the channels contained in the CMT, instead of registering
> > >> > one platform device per channel.
> > >> 
> > >> I both agree with you and disagree because of the current state of
> > >> timers in the linux kernel. I would have liked a single platform
> > >> device with all channles if this would be a generic timer driver that
> > >> from user space could be configured to associate channels with various
> > >> subsystems like PWM, clocksource, clockevent.
> > >> 
> > >> At this point the driver is doing clockevent and clocksource only, and
> > >> no sane user wants 84 channels of equivalent hardware blocks for those
> > >> two.
> > > 
> > > Of course, but we could always select which channels to register
> > > clockevents and clocksources for in platform data. That won't fix the
> > > overall problem, but it's one step forward.
> > 
> > But that's pretty much what we're doing, but only listing timer
> > channels that will be used. Of course, moving around things can be
> > done but I can't see why we want to do that if we have no selection of
> > drivers for the actual timer channels. Also, each timer channel may
> > have it's own unique set of possible parent clocks. That's something
> > we want to tie in to DT together with CCF. Solving those things
> > together makes sense IMO.

If you want to solve this along with the CCF implementation, please go ahead 
:-) I'm not too familiar with timers so I don't know what the best approach 
would be API-wise, but from a DT point of view we should have one node per 
timer. If we can't get there in a single step moving first to one platform 
device per CMT and then adding an API to select timers would be acceptable to 
me.

> > >> So based on that I'd rather do it like today and let people write
> > >> custom drivers for whatever applications they may use the other
> > >> channels for.
> > >> 
> > >> So if you're in hacking mode, why don't you figure out some way timers
> > >> can be configured from user space? =)
> > > 
> > > I don't have *that* much free time at the moment I'm afraid, and I'm
> > > sure you know why :-)
> > 
> > Yes I do, and that's why I asked. =)
> > 
> > >> If so then we can use DT to describe the actual hardware and let the
> > >> software policy be decided via some configuration mechanism.
> > > 
> > > Don't we also need timers during early boot, when userspace isn't
> > > available yet ?
> > 
> > It depends on the rest of the system. It is possible to boot to user
> > space without a timer, but I don't recommend it. =)
> 
> Hi,
> 
> I am holding off on this patch until some consensus is reached.

I don't think there's a need to hold off, this patch doesn't worsen the 
situation, cleanups would go on top.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clocksource: sh_cmt: 32-bit control register support

[Simon Horman]

On Wed, Jun 19, 2013 at 02:58:01PM +0200, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Wednesday 19 June 2013 21:31:23 Simon Horman wrote:
> > On Tue, Jun 18, 2013 at 10:27:44PM +0900, Magnus Damm wrote:
> > > On Tue, Jun 18, 2013 at 9:30 PM, Laurent Pinchart wrote:
> > > > On Tuesday 18 June 2013 20:54:47 Magnus Damm wrote:
> > > >> On Tue, Jun 18, 2013 at 7:35 PM, Laurent Pinchart wrote:
> > > >> > On Tuesday 18 June 2013 14:39:38 Magnus Damm wrote:
> > > >> >> On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart wrote:
> > > >> >> > On Monday 17 June 2013 15:40:52 Magnus Damm wrote:
> > > >> >> >> From: Magnus Damm <damm@opensource.se>
> > > >> >> >> 
> > > >> >> >> Add support for CMT hardware with 32-bit control and counter
> > > >> >> >> registers, as found on r8a73a4 and r8a7790. To use the CMT
> > > >> >> >> with 32-bit hardware a second I/O memory resource needs to
> > > >> >> >> point out the CMSTR register and it needs to be 32 bit wide.
> > > >> >> > 
> > > >> >> > Is a memory second resource required ? Can't we use a single
> > > >> >> > resource that will contain all the registers ?
> > > >> >> 
> > > >> >> The CMT hardware block comes with a shared timer start stop register
> > > >> >> that historically has been left out of the resource. The location of
> > > >> >> this register has so far been pointed out by the "channel offset"
> > > >> >> platform data member, together with information about which bit that
> > > >> >> happens to be assigned to the timer channel. This start stop
> > > >> >> register has happened to be kept in the same page of I/O memory as
> > > >> >> the main timer channel resource, so at this point we're sort of
> > > >> >> "lucky" that a single ioremap() has covered all cases.
> > > >> >> 
> > > >> >> With this patch it becomes optional to instead of platform data use
> > > >> >> a second resource to point out the timer start/stop register. While
> > > >> >> we do that we can also use the size of that resource to determine
> > > >> >> the I/O access width, which happens to be something that is needed
> > > >> >> to enable the driver on certain SoCs.
> > > >> > 
> > > >> > OK, I get it now. I've had a quick look at the documentation, and I'm
> > > >> > wondering whether we shouldn't register a single platform device that
> > > >> > span all the channels contained in the CMT, instead of registering
> > > >> > one platform device per channel.
> > > >> 
> > > >> I both agree with you and disagree because of the current state of
> > > >> timers in the linux kernel. I would have liked a single platform
> > > >> device with all channles if this would be a generic timer driver that
> > > >> from user space could be configured to associate channels with various
> > > >> subsystems like PWM, clocksource, clockevent.
> > > >> 
> > > >> At this point the driver is doing clockevent and clocksource only, and
> > > >> no sane user wants 84 channels of equivalent hardware blocks for those
> > > >> two.
> > > > 
> > > > Of course, but we could always select which channels to register
> > > > clockevents and clocksources for in platform data. That won't fix the
> > > > overall problem, but it's one step forward.
> > > 
> > > But that's pretty much what we're doing, but only listing timer
> > > channels that will be used. Of course, moving around things can be
> > > done but I can't see why we want to do that if we have no selection of
> > > drivers for the actual timer channels. Also, each timer channel may
> > > have it's own unique set of possible parent clocks. That's something
> > > we want to tie in to DT together with CCF. Solving those things
> > > together makes sense IMO.
> 
> If you want to solve this along with the CCF implementation, please go ahead 
> :-) I'm not too familiar with timers so I don't know what the best approach 
> would be API-wise, but from a DT point of view we should have one node per 
> timer. If we can't get there in a single step moving first to one platform 
> device per CMT and then adding an API to select timers would be acceptable to 
> me.
> 
> > > >> So based on that I'd rather do it like today and let people write
> > > >> custom drivers for whatever applications they may use the other
> > > >> channels for.
> > > >> 
> > > >> So if you're in hacking mode, why don't you figure out some way timers
> > > >> can be configured from user space? =)
> > > > 
> > > > I don't have *that* much free time at the moment I'm afraid, and I'm
> > > > sure you know why :-)
> > > 
> > > Yes I do, and that's why I asked. =)
> > > 
> > > >> If so then we can use DT to describe the actual hardware and let the
> > > >> software policy be decided via some configuration mechanism.
> > > > 
> > > > Don't we also need timers during early boot, when userspace isn't
> > > > available yet ?
> > > 
> > > It depends on the rest of the system. It is possible to boot to user
> > > space without a timer, but I don't recommend it. =)
> > 
> > Hi,
> > 
> > I am holding off on this patch until some consensus is reached.
> 
> I don't think there's a need to hold off, this patch doesn't worsen the 
> situation, cleanups would go on top.

Thanks, I will queue this up in the clocksource branch.