All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sascha Hauer <s.hauer@pengutronix.de>
To: linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 0/4] Add a generic struct clk
Date: Tue, 24 May 2011 08:09:36 +0000	[thread overview]
Message-ID: <20110524080936.GI20715@pengutronix.de> (raw)
In-Reply-To: <BANLkTimEoCaKwJtpbQTDSEDydNkZ1BkKog@mail.gmail.com>

On Tue, May 24, 2011 at 12:31:13AM -0700, Colin Cross wrote:
> On Mon, May 23, 2011 at 11:26 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, May 23, 2011 at 04:12:24PM -0700, Colin Cross wrote:
> >> >
> >> >   tglx's plan is to create a separate struct clk_hwdata, which contains a
> >> >   union of base data structures for common clocks: div, mux, gate, etc. The
> >> >   ops callbacks are passed a clk_hw, plus a clk_hwdata, and most of the base
> >> >   hwdata fields are handled within the core clock code. This means less
> >> >   encapsulation of clock implementation logic, but more coverage of
> >> >   clock basics through the core code.
> >>
> >> I don't think they should be a union, I think there should be 3
> >> separate private datas, and three sets of clock ops, for the three
> >> different types of clock blocks: rate changers (dividers and plls),
> >> muxes, and gates.  These blocks are very often combined - a device
> >> clock often has a mux and a divider, and clk_set_parent and
> >> clk_set_rate on the same struct clk both need to work.
> >
> > The idea is to being able to propagate functions to the parent. It's
> > very convenient for the implementation of clocks when they only
> > implement either a divider, a mux or a gate. Combining all of these
> > into a single clock leads to complicated clock trees and many different
> > clocks where you can't factor out the common stuff.
> 
> That seems complicated.  You end up with lots of extra clocks (meaning
> more boilerplate in the platform files) that have no meaning in the
> system (what is the clock between the mux and the divider in Tegra's
> i2c1 clock, it can never feed any devices), and you have to figure out
> at the framework level when to propagate and when to error out.  I'm
> not even sure you can always find the right place to stop propagating
> - do you just keep going up until the set_parent callback succeeds, or
> exists, or what?

For the set_parent there would be no propagating at all. For set_rate I
can imagine a flag in the generic clock which tells whether to propagate
set_rate or not.

> 
> I think you can still factor out all the common code if you treat each
> clock as a possible mux, divider, and gate combination.  Each part of
> the clock is still just as abstractable as before - you can set the
> rate_ops to be the generic single register divider implementation, and
> the gate ops to be the generic single bit enable implementation.  The
> idea of what a clock node is matches the HW design,

The hardware design consists of only discrete rate changers (plls,
dividers), muxes and gates. These are the only building blocks
*every* hardware design has. I believe that many of the problems
the current implementations have are due to the multiple building
blocks stuffed into one clock. If you haven't already take a look
at my i.MX5 clock patches:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/113631

They need changes to fit onto the current patches and the rate
propagation problem is not solved there, but the resulting clock
data files are really short and nice to read. Furthermore it's easy
to implement. Just look at the diagrams in the datasheet and go through
them.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

WARNING: multiple messages have this Message-ID
From: Sascha Hauer <s.hauer@pengutronix.de>
To: Colin Cross <ccross@google.com>
Cc: Jeremy Kerr <jeremy.kerr@canonical.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	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 0/4] Add a generic struct clk
Date: Tue, 24 May 2011 10:09:36 +0200	[thread overview]
Message-ID: <20110524080936.GI20715@pengutronix.de> (raw)
In-Reply-To: <BANLkTimEoCaKwJtpbQTDSEDydNkZ1BkKog@mail.gmail.com>

On Tue, May 24, 2011 at 12:31:13AM -0700, Colin Cross wrote:
> On Mon, May 23, 2011 at 11:26 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, May 23, 2011 at 04:12:24PM -0700, Colin Cross wrote:
> >> >
> >> >   tglx's plan is to create a separate struct clk_hwdata, which contains a
> >> >   union of base data structures for common clocks: div, mux, gate, etc. The
> >> >   ops callbacks are passed a clk_hw, plus a clk_hwdata, and most of the base
> >> >   hwdata fields are handled within the core clock code. This means less
> >> >   encapsulation of clock implementation logic, but more coverage of
> >> >   clock basics through the core code.
> >>
> >> I don't think they should be a union, I think there should be 3
> >> separate private datas, and three sets of clock ops, for the three
> >> different types of clock blocks: rate changers (dividers and plls),
> >> muxes, and gates.  These blocks are very often combined - a device
> >> clock often has a mux and a divider, and clk_set_parent and
> >> clk_set_rate on the same struct clk both need to work.
> >
> > The idea is to being able to propagate functions to the parent. It's
> > very convenient for the implementation of clocks when they only
> > implement either a divider, a mux or a gate. Combining all of these
> > into a single clock leads to complicated clock trees and many different
> > clocks where you can't factor out the common stuff.
> 
> That seems complicated.  You end up with lots of extra clocks (meaning
> more boilerplate in the platform files) that have no meaning in the
> system (what is the clock between the mux and the divider in Tegra's
> i2c1 clock, it can never feed any devices), and you have to figure out
> at the framework level when to propagate and when to error out.  I'm
> not even sure you can always find the right place to stop propagating
> - do you just keep going up until the set_parent callback succeeds, or
> exists, or what?

For the set_parent there would be no propagating at all. For set_rate I
can imagine a flag in the generic clock which tells whether to propagate
set_rate or not.

> 
> I think you can still factor out all the common code if you treat each
> clock as a possible mux, divider, and gate combination.  Each part of
> the clock is still just as abstractable as before - you can set the
> rate_ops to be the generic single register divider implementation, and
> the gate ops to be the generic single bit enable implementation.  The
> idea of what a clock node is matches the HW design,

The hardware design consists of only discrete rate changers (plls,
dividers), muxes and gates. These are the only building blocks
*every* hardware design has. I believe that many of the problems
the current implementations have are due to the multiple building
blocks stuffed into one clock. If you haven't already take a look
at my i.MX5 clock patches:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/113631

They need changes to fit onto the current patches and the rate
propagation problem is not solved there, but the resulting clock
data files are really short and nice to read. Furthermore it's easy
to implement. Just look at the diagrams in the datasheet and go through
them.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

WARNING: multiple messages have this Message-ID
From: s.hauer@pengutronix.de (Sascha Hauer)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 0/4] Add a generic struct clk
Date: Tue, 24 May 2011 10:09:36 +0200	[thread overview]
Message-ID: <20110524080936.GI20715@pengutronix.de> (raw)
In-Reply-To: <BANLkTimEoCaKwJtpbQTDSEDydNkZ1BkKog@mail.gmail.com>

On Tue, May 24, 2011 at 12:31:13AM -0700, Colin Cross wrote:
> On Mon, May 23, 2011 at 11:26 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, May 23, 2011 at 04:12:24PM -0700, Colin Cross wrote:
> >> >
> >> > ? tglx's plan is to create a separate struct clk_hwdata, which contains a
> >> > ? union of base data structures for common clocks: div, mux, gate, etc. The
> >> > ? ops callbacks are passed a clk_hw, plus a clk_hwdata, and most of the base
> >> > ? hwdata fields are handled within the core clock code. This means less
> >> > ? encapsulation of clock implementation logic, but more coverage of
> >> > ? clock basics through the core code.
> >>
> >> I don't think they should be a union, I think there should be 3
> >> separate private datas, and three sets of clock ops, for the three
> >> different types of clock blocks: rate changers (dividers and plls),
> >> muxes, and gates. ?These blocks are very often combined - a device
> >> clock often has a mux and a divider, and clk_set_parent and
> >> clk_set_rate on the same struct clk both need to work.
> >
> > The idea is to being able to propagate functions to the parent. It's
> > very convenient for the implementation of clocks when they only
> > implement either a divider, a mux or a gate. Combining all of these
> > into a single clock leads to complicated clock trees and many different
> > clocks where you can't factor out the common stuff.
> 
> That seems complicated.  You end up with lots of extra clocks (meaning
> more boilerplate in the platform files) that have no meaning in the
> system (what is the clock between the mux and the divider in Tegra's
> i2c1 clock, it can never feed any devices), and you have to figure out
> at the framework level when to propagate and when to error out.  I'm
> not even sure you can always find the right place to stop propagating
> - do you just keep going up until the set_parent callback succeeds, or
> exists, or what?

For the set_parent there would be no propagating at all. For set_rate I
can imagine a flag in the generic clock which tells whether to propagate
set_rate or not.

> 
> I think you can still factor out all the common code if you treat each
> clock as a possible mux, divider, and gate combination.  Each part of
> the clock is still just as abstractable as before - you can set the
> rate_ops to be the generic single register divider implementation, and
> the gate ops to be the generic single bit enable implementation.  The
> idea of what a clock node is matches the HW design,

The hardware design consists of only discrete rate changers (plls,
dividers), muxes and gates. These are the only building blocks
*every* hardware design has. I believe that many of the problems
the current implementations have are due to the multiple building
blocks stuffed into one clock. If you haven't already take a look
at my i.MX5 clock patches:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/113631

They need changes to fit onto the current patches and the rate
propagation problem is not solved there, but the resulting clock
data files are really short and nice to read. Furthermore it's easy
to implement. Just look at the diagrams in the datasheet and go through
them.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

  reply	other threads:[~2011-05-24  8:09 UTC|newest]

Thread overview: 139+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-20  7:27 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
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 [this message]
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=20110524080936.GI20715@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --subject='Re: [PATCH 0/4] Add a generic struct clk' \
    /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

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.