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

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:

quoted

On Tue, Jun 18, 2013 at 9:30 PM, Laurent Pinchart wrote:

quoted

On Tuesday 18 June 2013 20:54:47 Magnus Damm wrote:

quoted

On Tue, Jun 18, 2013 at 7:35 PM, Laurent Pinchart wrote:

quoted

On Tuesday 18 June 2013 14:39:38 Magnus Damm wrote:

quoted

On Tue, Jun 18, 2013 at 3:37 AM, Laurent Pinchart wrote:

quoted

On Monday 17 June 2013 15:40:52 Magnus Damm wrote:

quoted

From: Magnus Damm <redacted>

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.

quoted

quoted

quoted

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. =)

quoted

quoted

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