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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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