All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Date: Wed, 25 May 2011 11:43:56 +0000	[thread overview]
Message-ID: <alpine.LFD.2.02.1105251324500.3078@ionos> (raw)
In-Reply-To: <BANLkTinmRt_fv1oJwFuJQ34aAky_5SB4Rw@mail.gmail.com>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3221 bytes --]

On Tue, 24 May 2011, Colin Cross wrote:
> On Tue, May 24, 2011 at 12:02 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > +       struct clk              *parent;
> >>
> >> Storing the list of possible parents at this level can help abstract
> >> some common code from mux ops if you pass the index into the list of
> >> the new parent into the op - most mux ops only need to know which of
> >> their mux inputs needs to be enabled.
> >
> > Please don't. Only muxes have more than one possible parent, so this
> > should be handled there.
> 
> The cost is one pointer per clock that is not actually a mux, and the
> benefit is that you can move a very common search loop out of every
> mux implementation into the framework.  It also lets you determine
> which clocks are connected, which becomes necessary if you try to do
> per-tree locking or sysfs controls for clocks.

We can be more clever than storing the world and some more in the
internal implementation.

struct clk should be just the basic data structure to hold the
information which is shared by all clk types. The way I see it is:

struct clk {
       struct clk_ops *ops;
       struct clk_data *data;
       void *hwdata;
       unsigned long flags;
       ....
};

Now have

struct clk_data {
       union {
       	     struct clk_divider_data;
	     struct clk_mux_data;
	     struct clk_pll_data;
	     .....
       };
};

Now on registration time you tell the core which type you are handing
in. struct clk_mux_data would contain the list of possible parent
clocks and clk_pll_data the possible multiplier values (linear or
table based). Same for dividers. Now the core can do the evaluation of
clock rate settings or parents and hand in the index or the linear
range value into ops->set_rate/set_parent. And with the information
available to the core you can do even complex settings which involve
selecting the proper pll for your pixelclock example below.

> > I hope we can avoid that. The decision of the best parent should be left
> > up to the user. Doing this in the mux/divider implementation would
> > torpedo attempts to implement generic building blocks.
> 
> I agree it would be nice, but it does push some knowledge of the clock
> tree into device drivers.  For example, on Tegra the display driver
> may need to change the source pll of the display clock to get the
> required pclk, which requires passing all the possible parents of the
> display clock into the display driver.  If this is a common usage
> pattern, there needs to be a hook in the ops to allow the clock or
> clock chip to make a more global decision.

No, we really dont want to have that knowledge at the driver level. A
driver should just say: Enable "pixelclk" at rate X.

So then a mapper converts pixelclk to the particular clock on the SoC
it's running on and the clk framework level needs to figure out how to
achieve that by looking at the full chain down from the clk gate which
feeds the pixelclk. We don't want drivers to be aware about that at
all. They simply do not care and that's a preliminary for making
drivers usable on different SoC incarnations.
 
Thanks,

	tglx

WARNING: multiple messages have this Message-ID (diff)
From: Thomas Gleixner <tglx@linutronix.de>
To: Colin Cross <ccross@google.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>,
	Jeremy Kerr <jeremy.kerr@canonical.com>,
	lkml <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	linux-sh@vger.kernel.org
Subject: Re: [PATCH 1/4] clk: Add a generic clock infrastructure
Date: Wed, 25 May 2011 13:43:56 +0200 (CEST)	[thread overview]
Message-ID: <alpine.LFD.2.02.1105251324500.3078@ionos> (raw)
In-Reply-To: <BANLkTinmRt_fv1oJwFuJQ34aAky_5SB4Rw@mail.gmail.com>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3145 bytes --]

On Tue, 24 May 2011, Colin Cross wrote:
> On Tue, May 24, 2011 at 12:02 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > +       struct clk              *parent;
> >>
> >> Storing the list of possible parents at this level can help abstract
> >> some common code from mux ops if you pass the index into the list of
> >> the new parent into the op - most mux ops only need to know which of
> >> their mux inputs needs to be enabled.
> >
> > Please don't. Only muxes have more than one possible parent, so this
> > should be handled there.
> 
> The cost is one pointer per clock that is not actually a mux, and the
> benefit is that you can move a very common search loop out of every
> mux implementation into the framework.  It also lets you determine
> which clocks are connected, which becomes necessary if you try to do
> per-tree locking or sysfs controls for clocks.

We can be more clever than storing the world and some more in the
internal implementation.

struct clk should be just the basic data structure to hold the
information which is shared by all clk types. The way I see it is:

struct clk {
       struct clk_ops *ops;
       struct clk_data *data;
       void *hwdata;
       unsigned long flags;
       ....
};

Now have

struct clk_data {
       union {
       	     struct clk_divider_data;
	     struct clk_mux_data;
	     struct clk_pll_data;
	     .....
       };
};

Now on registration time you tell the core which type you are handing
in. struct clk_mux_data would contain the list of possible parent
clocks and clk_pll_data the possible multiplier values (linear or
table based). Same for dividers. Now the core can do the evaluation of
clock rate settings or parents and hand in the index or the linear
range value into ops->set_rate/set_parent. And with the information
available to the core you can do even complex settings which involve
selecting the proper pll for your pixelclock example below.

> > I hope we can avoid that. The decision of the best parent should be left
> > up to the user. Doing this in the mux/divider implementation would
> > torpedo attempts to implement generic building blocks.
> 
> I agree it would be nice, but it does push some knowledge of the clock
> tree into device drivers.  For example, on Tegra the display driver
> may need to change the source pll of the display clock to get the
> required pclk, which requires passing all the possible parents of the
> display clock into the display driver.  If this is a common usage
> pattern, there needs to be a hook in the ops to allow the clock or
> clock chip to make a more global decision.

No, we really dont want to have that knowledge at the driver level. A
driver should just say: Enable "pixelclk" at rate X.

So then a mapper converts pixelclk to the particular clock on the SoC
it's running on and the clk framework level needs to figure out how to
achieve that by looking at the full chain down from the clk gate which
feeds the pixelclk. We don't want drivers to be aware about that at
all. They simply do not care and that's a preliminary for making
drivers usable on different SoC incarnations.
 
Thanks,

	tglx

WARNING: multiple messages have this Message-ID (diff)
From: tglx@linutronix.de (Thomas Gleixner)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/4] clk: Add a generic clock infrastructure
Date: Wed, 25 May 2011 13:43:56 +0200 (CEST)	[thread overview]
Message-ID: <alpine.LFD.2.02.1105251324500.3078@ionos> (raw)
In-Reply-To: <BANLkTinmRt_fv1oJwFuJQ34aAky_5SB4Rw@mail.gmail.com>

On Tue, 24 May 2011, Colin Cross wrote:
> On Tue, May 24, 2011 at 12:02 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > + ? ? ? struct clk ? ? ? ? ? ? ?*parent;
> >>
> >> Storing the list of possible parents at this level can help abstract
> >> some common code from mux ops if you pass the index into the list of
> >> the new parent into the op - most mux ops only need to know which of
> >> their mux inputs needs to be enabled.
> >
> > Please don't. Only muxes have more than one possible parent, so this
> > should be handled there.
> 
> The cost is one pointer per clock that is not actually a mux, and the
> benefit is that you can move a very common search loop out of every
> mux implementation into the framework.  It also lets you determine
> which clocks are connected, which becomes necessary if you try to do
> per-tree locking or sysfs controls for clocks.

We can be more clever than storing the world and some more in the
internal implementation.

struct clk should be just the basic data structure to hold the
information which is shared by all clk types. The way I see it is:

struct clk {
       struct clk_ops *ops;
       struct clk_data *data;
       void *hwdata;
       unsigned long flags;
       ....
};

Now have

struct clk_data {
       union {
       	     struct clk_divider_data;
	     struct clk_mux_data;
	     struct clk_pll_data;
	     .....
       };
};

Now on registration time you tell the core which type you are handing
in. struct clk_mux_data would contain the list of possible parent
clocks and clk_pll_data the possible multiplier values (linear or
table based). Same for dividers. Now the core can do the evaluation of
clock rate settings or parents and hand in the index or the linear
range value into ops->set_rate/set_parent. And with the information
available to the core you can do even complex settings which involve
selecting the proper pll for your pixelclock example below.

> > I hope we can avoid that. The decision of the best parent should be left
> > up to the user. Doing this in the mux/divider implementation would
> > torpedo attempts to implement generic building blocks.
> 
> I agree it would be nice, but it does push some knowledge of the clock
> tree into device drivers.  For example, on Tegra the display driver
> may need to change the source pll of the display clock to get the
> required pclk, which requires passing all the possible parents of the
> display clock into the display driver.  If this is a common usage
> pattern, there needs to be a hook in the ops to allow the clock or
> clock chip to make a more global decision.

No, we really dont want to have that knowledge at the driver level. A
driver should just say: Enable "pixelclk" at rate X.

So then a mapper converts pixelclk to the particular clock on the SoC
it's running on and the clk framework level needs to figure out how to
achieve that by looking at the full chain down from the clk gate which
feeds the pixelclk. We don't want drivers to be aware about that at
all. They simply do not care and that's a preliminary for making
drivers usable on different SoC incarnations.
 
Thanks,

	tglx

  parent reply	other threads:[~2011-05-25 11:43 UTC|newest]

Thread overview: 139+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-20  7:27 [PATCH 0/4] Add a generic struct clk Jeremy Kerr
2011-05-20  7:27 ` Jeremy Kerr
2011-05-20  7:27 ` Jeremy Kerr
2011-05-20  7:27 ` [PATCH 4/4] clk: Add simple gated clock Jeremy Kerr
2011-05-20  7:27   ` Jeremy Kerr
2011-05-20  7:27   ` Jeremy Kerr
2011-05-20 11:37   ` Arnd Bergmann
2011-05-20 11:37     ` Arnd Bergmann
2011-05-20 11:37     ` Arnd Bergmann
2011-05-20 22:19   ` Rob Herring
2011-05-20 22:19     ` Rob Herring
2011-05-20 22:19     ` Rob Herring
2011-05-20  7:27 ` [PATCH 2/4] clk: Implement clk_set_rate Jeremy Kerr
2011-05-20  7:27   ` Jeremy Kerr
2011-05-20  7:27   ` Jeremy Kerr
2011-05-20 12:25   ` Sascha Hauer
2011-05-20 12:25     ` Sascha Hauer
2011-05-20 12:25     ` Sascha Hauer
2011-05-24  7:59   ` Colin Cross
2011-05-24  7:59     ` Colin Cross
2011-05-24  7:59     ` Colin Cross
2011-05-25 19:03   ` Sascha Hauer
2011-05-25 19:03     ` Sascha Hauer
2011-05-25 19:03     ` Sascha Hauer
     [not found]     ` <1306373867.2875.162.camel@pororo>
2011-05-26  6:54       ` Sascha Hauer
2011-05-26  6:54         ` Sascha Hauer
2011-05-26  6:54         ` Sascha Hauer
2011-05-30  5:05   ` Mike Frysinger
2011-05-30  5:05     ` Mike Frysinger
2011-05-30  5:05     ` Mike Frysinger
2011-05-20  7:27 ` [PATCH 3/4] clk: Add fixed-rate clock Jeremy Kerr
2011-05-20  7:27   ` Jeremy Kerr
2011-05-20  7:27   ` Jeremy Kerr
2011-05-24  7:01   ` Francesco VIRLINZI
2011-05-30  5:01   ` Mike Frysinger
2011-05-30  5:01     ` Mike Frysinger
2011-05-30  5:01     ` Mike Frysinger
2011-05-30  5:02   ` Mike Frysinger
2011-05-30  5:02     ` Mike Frysinger
2011-05-30  5:02     ` Mike Frysinger
2011-05-20  7:27 ` [PATCH 1/4] clk: Add a generic clock infrastructure Jeremy Kerr
2011-05-20  7:27   ` Jeremy Kerr
2011-05-20  7:27   ` Jeremy Kerr
2011-05-20 11:59   ` Sascha Hauer
2011-05-20 11:59     ` Sascha Hauer
2011-05-20 11:59     ` Sascha Hauer
2011-05-20 13:25     ` Thomas Gleixner
2011-05-20 13:25       ` Thomas Gleixner
2011-05-20 13:25       ` Thomas Gleixner
2011-05-20 13:36       ` Sascha Hauer
2011-05-20 13:36         ` Sascha Hauer
2011-05-20 13:36         ` Sascha Hauer
2011-05-23 23:55   ` Colin Cross
2011-05-23 23:55     ` Colin Cross
2011-05-23 23:55     ` Colin Cross
2011-05-24  7:02     ` Sascha Hauer
2011-05-24  7:02       ` Sascha Hauer
2011-05-24  7:02       ` Sascha Hauer
2011-05-24  7:51       ` Colin Cross
2011-05-24  7:51         ` Colin Cross
2011-05-24  7:51         ` Colin Cross
2011-05-24  8:38         ` Sascha Hauer
2011-05-24  8:38           ` Sascha Hauer
2011-05-24  8:38           ` Sascha Hauer
2011-05-25 11:22           ` Richard Zhao
2011-05-25 11:22             ` Richard Zhao
2011-05-25 11:22             ` Richard Zhao
2011-05-25 11:43         ` Thomas Gleixner [this message]
2011-05-25 11:43           ` Thomas Gleixner
2011-05-25 11:43           ` Thomas Gleixner
2011-05-24  4:18   ` viresh kumar
2011-05-24  4:30     ` viresh kumar
2011-05-24  4:18     ` viresh kumar
2011-05-25 10:47   ` Richard Zhao
2011-05-25 10:47     ` Richard Zhao
2011-05-25 10:47     ` Richard Zhao
2011-05-30  5:00     ` Mike Frysinger
2011-05-30  5:00       ` Mike Frysinger
2011-05-30  5:00       ` Mike Frysinger
2011-05-23 23:12 ` [PATCH 0/4] Add a generic struct clk Colin Cross
2011-05-23 23:12   ` Colin Cross
2011-05-23 23:12   ` Colin Cross
2011-05-24  6:26   ` Sascha Hauer
2011-05-24  6:26     ` Sascha Hauer
2011-05-24  6:26     ` Sascha Hauer
2011-05-24  7:31     ` Colin Cross
2011-05-24  7:31       ` Colin Cross
2011-05-24  7:31       ` Colin Cross
2011-05-24  8:09       ` Sascha Hauer
2011-05-24  8:09         ` Sascha Hauer
2011-05-24  8:09         ` Sascha Hauer
2011-05-24 19:41         ` Colin Cross
2011-05-24 19:41           ` Colin Cross
2011-05-24 19:41           ` Colin Cross
2011-05-25  2:32           ` Richard Zhao
2011-05-25  2:32             ` Richard Zhao
2011-05-25  2:32             ` Richard Zhao
2011-05-25  6:23           ` Sascha Hauer
2011-05-25  6:23             ` Sascha Hauer
2011-05-25  6:23             ` Sascha Hauer
2011-05-25  7:51           ` Thomas Gleixner
2011-05-25  7:51             ` Thomas Gleixner
2011-05-25  7:51             ` Thomas Gleixner
2011-05-27 14:39           ` Mark Brown
2011-05-27 14:39             ` Mark Brown
2011-05-27 14:39             ` Mark Brown
2011-05-24 17:22   ` Richard Zhao
2011-05-24 17:22     ` Richard Zhao
2011-05-24 17:22     ` Richard Zhao
2011-05-24 17:52     ` Colin Cross
2011-05-24 17:52       ` Colin Cross
2011-05-24 17:52       ` Colin Cross
2011-05-25  2:08       ` Richard Zhao
2011-05-25  2:08         ` Richard Zhao
2011-05-25  2:08         ` Richard Zhao
2011-05-30  5:20 ` Mike Frysinger
2011-05-30  5:20   ` Mike Frysinger
2011-05-30  5:20   ` Mike Frysinger
2011-07-10  9:09 ` Mark Brown
2011-07-10  9:09   ` Mark Brown
2011-07-10  9:09   ` Mark Brown
2011-07-10  9:50   ` Russell King - ARM Linux
2011-07-10  9:50     ` Russell King - ARM Linux
2011-07-10  9:50     ` Russell King - ARM Linux
2011-07-10 10:00     ` Russell King - ARM Linux
2011-07-10 10:00       ` Russell King - ARM Linux
2011-07-10 10:00       ` Russell King - ARM Linux
2011-07-10 11:27     ` Mark Brown
2011-07-10 11:27       ` Mark Brown
2011-07-10 11:27       ` Mark Brown
2011-07-10 11:52       ` Russell King - ARM Linux
2011-07-10 11:52         ` Russell King - ARM Linux
2011-07-10 11:52         ` Russell King - ARM Linux
2011-07-11  2:49   ` Jeremy Kerr
2011-07-11  2:49     ` Jeremy Kerr
2011-07-11  2:49     ` Jeremy Kerr
2011-07-11  3:57     ` Mark Brown
2011-07-11  3:57       ` Mark Brown
2011-07-11  3:57       ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LFD.2.02.1105251324500.3078@ionos \
    --to=tglx@linutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.