Message-ID: <20080326174433.GA13104@linux-sh.org>
Date:	Thu, 27 Mar 2008 02:44:33 +0900
From:	Paul Mundt <lethal@...ux-sh.org>
To:	Dmitry <dbaryshkov@...il.com>
Cc:	Haavard Skinnemoen <haavard.skinnemoen@...el.com>,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	hskinnemoen@...el.com, domen.puncer@...argo.com, tony@...mide.com,
	rmk+kernel@....linux.org.uk, paul@...an.com
Subject: Re: [PATCH 1/3] Clocklib: add generic framework for managing clocks.

On Wed, Mar 26, 2008 at 07:52:34PM +0300, Dmitry wrote:
> 2008/3/26, Haavard Skinnemoen <haavard.skinnemoen@...el.com>:
> > Hmm...this is exactly twice as big as the struct I'm currently using,
> >  it doesn't contain all the fields I need, and it's undocumented.
> 
> I've added a more sofisticated arch convertion patch (the clocklib for
> ARM PXA chips).
> 
What you've converted is still pretty tame in comparison to what this
sort of framework has to handle. Something like the OMAP struct clk is
more like a worst-case and is representative of how large this structure
will get for everyone if it is to be shared without dropping
functionality.

> >  I have quite a few clocks, so the increased memory consumption is quite
> >  significant. What are the advantages of this?
> 
> At maximum 55, IIUC. I counted 32 or so additional bytes in the struct
> (over avr32-specific one). That would count up to 1.5 K overhead. Is
> that really too much for current kernels?
> 
Your idea of maximum and my hardware's idea of maximum have very little
in common. Most of these platforms are dealing with hundreds of different
clocks, though they are not necessarily all interfaced in software
control (mostly because a lot of them auto-gate, and because writing up
struct clk definitions for 200+ clocks for 30 different CPUs isn't
exactly my idea of a good time). This does not mean that more clocks will
not be hooked up as the need arises.

On Wed, Mar 26, 2008 at 08:04:41PM +0300, Dmitry wrote:
> 2008/3/26, Paul Mundt <lethal@...ux-sh.org>:
> > Conversely it also has fields that I don't need. If struct clk could have
> >  been done generically without growing to insane sizes, it would have been
> >  done so in linux/clk.h a long time ago. The main thing there is API
> >  consistency for drivers, leaving the details up to the architecture.
> 
> In reality, as noted David Brownell on LAKML, there is no API consistency.
> The driver has no way to know whether clk API is implemented at all or whether
> the "optional" functions do exist.
> 
There is certainly API consistency. If there were not, we wouldn't have
linux/clk.h. The fact that many platforms add on top of this does not
detract from the fact that we have a common API that is currently shared.

There is indeed no way to know what optional functionality exists, but
your proposed structure and interface largely works around that problem
by ignoring all of the optional functionality. This works great for an
idealized set of platforms and clock configurations, but quickly runs in
to the same issue that linux/clk.h has today. It's not clear how your
proposed interface helps any of this other than providing a struct clk
that can be used more generically.

> Moreover clocks aren't limited only to platform-specific code. As an
> example of the in-tree driver that will benefit from clocklib you can
> check drivers/mfd/sm501.c which  contains  it's own set of functions
> to manage clocks.
> 
That's certainly true, it's definitely worthwhile to try and unify as
much of the interface as possible. Perhaps it makes more sense to try and
have a common struct clk with the bare essentials and then allow the
platforms to extend that functionality based on their own capabilities.
This could be done through the private data I suppose.

> That's one of the initial reasons of this patchserie: I have a device
> that can be installed on several platforms. And I want for this device
> to provide clocks to some other devices.
> 
I don't disagree with your intentions, or that something like this would
be a good idea. What needs to happen first is having a look at all of the
different versions of the clock framework that are out there and coming
up with a consolidated struct clk, then seeing if the resulting size is
practical enough to actually use for any significant number of clocks.

Your current definition doesn't meet the requirements of all of the
struct clk users in the kernel, and it's already getting quite big
compared to what people (avr32, sh, some ARM platforms, etc.) are using
today. This is a good indicator of why a common definition wasn't a good
fit in the first place.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists