From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Zhao Date: Wed, 25 May 2011 02:32:09 +0000 Subject: Re: [PATCH 0/4] Add a generic struct clk Message-Id: <20110525023209.GA2535@b20223-02.ap.freescale.net> List-Id: References: <1305876469.325655.313573683829.0.gpush@pororo> <20110524062620.GA22096@pengutronix.de> <20110524080936.GI20715@pengutronix.de> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: linux-arm-kernel@lists.infradead.org On Tue, May 24, 2011 at 12:41:20PM -0700, Colin Cross wrote: > On Tue, May 24, 2011 at 1:09 AM, Sascha Hauer wr= ote: > > On Tue, May 24, 2011 at 12:31:13AM -0700, Colin Cross wrote: > >> On Mon, May 23, 2011 at 11:26 PM, Sascha Hauer wrote: > >> > On Mon, May 23, 2011 at 04:12:24PM -0700, Colin Cross wrote: > >> >> > > >> >> > =A0 tglx's plan is to create a separate struct clk_hwdata, which = contains a > >> >> > =A0 union of base data structures for common clocks: div, mux, ga= te, etc. The > >> >> > =A0 ops callbacks are passed a clk_hw, plus a clk_hwdata, and mos= t of the base > >> >> > =A0 hwdata fields are handled within the core clock code. This me= ans less > >> >> > =A0 encapsulation of clock implementation logic, but more coverag= e of > >> >> > =A0 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. =A0These 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 differ= ent > >> > clocks where you can't factor out the common stuff. > >> > >> That seems complicated. =A0You end up with lots of extra clocks (meani= ng > >> 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. =A0I'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. =A0Each 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. =A0The > >> 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. >=20 > Propagation is what I'm trying simplify, because it's impossible at > the framework level to determine the right time to propagate, and the > right time to return an error, and I don't like pushing it down to > each clock implementation either, because then you need a "propagating > clk gate" and a "non-propagating clk gate". >=20 > Your building blocks implement every op - clk_gate_ops implements > set_rate as clk_parent_set_rate (won't that cause a deadlock on > prepare_lock when it calls clk_set_rate?). I'm suggesting breaking > out the clock gate ops into a struct that only contains: > enable > disable > prepare > unprepare > And a rate ops struct that contains: > round_rate > set_rate > And a mux ops struct that contains > set_parent >=20 > For a single HW clock that has a mux, a gate, and a divider, the > existing implementation requires: > INIT_CLK(..., clk_mux_ops) > INIT_CLK(..., clk_div_ops) > INIT_CLK(..., clk_gate_ops) Not all clocks are like: pll -> [mux] -> [gate] -> [divider] -> clk A Some imx233 clocks are like: pll -> [divider] -> -----------\ [mux] -> clk B xtal -> [gate] -> [divider] -> / Thanks Richard > which creates 3 clocks, and requires lots of propagation logic to > figure out how to call clk_set_rate on the single clock that is > exposed to the device, but not propagate it past the device clock if > the device clock doesn't have a divider (propagating it up to an > active pll feeding other devices results in disaster, at least on > Tegra). This combination doesn't seem to be common in your MX code, > but _every_ Tegra device clock has these three parts. >=20 > With multiple smaller building blocks that can fit inside a clock, all > you need is: > INIT_CLK(..., clk_mux_ops, clk_div_ops, clk_gate_ops) > You have one struct clk, which is exposed to the device and matches > the datasheet. If the clock has rate ops, clk_set_rate works with no > propagation. If it doesn't have a rate, clk_rate_ops is NULL, and the > framework deal with propagation only in the case where it is really > propagation - a child clock that requires changing a parent clock. > The block abstraction is still in place, there are just 3 slots for > blocks within each clock. >=20 > Using a flag to mark clocks as "non-propagatable" is also not correct > - propagatability is a feature of the parent, not the child. >=20 > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >=20 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755943Ab1EYCcR (ORCPT ); Tue, 24 May 2011 22:32:17 -0400 Received: from [58.250.33.3] ([58.250.33.3]:50238 "EHLO b20223-02" rhost-flags-FAIL-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1755209Ab1EYCcP (ORCPT ); Tue, 24 May 2011 22:32:15 -0400 Date: Wed, 25 May 2011 10:32:09 +0800 From: Richard Zhao To: Colin Cross Cc: Sascha Hauer , Jeremy Kerr , linux-sh@vger.kernel.org, Thomas Gleixner , "linux-arm-kernel@lists.infradead.org" , lkml Subject: Re: [PATCH 0/4] Add a generic struct clk Message-ID: <20110525023209.GA2535@b20223-02.ap.freescale.net> References: <1305876469.325655.313573683829.0.gpush@pororo> <20110524062620.GA22096@pengutronix.de> <20110524080936.GI20715@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 24, 2011 at 12:41:20PM -0700, Colin Cross wrote: > On Tue, May 24, 2011 at 1:09 AM, Sascha Hauer wrote: > > On Tue, May 24, 2011 at 12:31:13AM -0700, Colin Cross wrote: > >> On Mon, May 23, 2011 at 11:26 PM, Sascha Hauer 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. > > Propagation is what I'm trying simplify, because it's impossible at > the framework level to determine the right time to propagate, and the > right time to return an error, and I don't like pushing it down to > each clock implementation either, because then you need a "propagating > clk gate" and a "non-propagating clk gate". > > Your building blocks implement every op - clk_gate_ops implements > set_rate as clk_parent_set_rate (won't that cause a deadlock on > prepare_lock when it calls clk_set_rate?). I'm suggesting breaking > out the clock gate ops into a struct that only contains: > enable > disable > prepare > unprepare > And a rate ops struct that contains: > round_rate > set_rate > And a mux ops struct that contains > set_parent > > For a single HW clock that has a mux, a gate, and a divider, the > existing implementation requires: > INIT_CLK(..., clk_mux_ops) > INIT_CLK(..., clk_div_ops) > INIT_CLK(..., clk_gate_ops) Not all clocks are like: pll -> [mux] -> [gate] -> [divider] -> clk A Some imx233 clocks are like: pll -> [divider] -> -----------\ [mux] -> clk B xtal -> [gate] -> [divider] -> / Thanks Richard > which creates 3 clocks, and requires lots of propagation logic to > figure out how to call clk_set_rate on the single clock that is > exposed to the device, but not propagate it past the device clock if > the device clock doesn't have a divider (propagating it up to an > active pll feeding other devices results in disaster, at least on > Tegra). This combination doesn't seem to be common in your MX code, > but _every_ Tegra device clock has these three parts. > > With multiple smaller building blocks that can fit inside a clock, all > you need is: > INIT_CLK(..., clk_mux_ops, clk_div_ops, clk_gate_ops) > You have one struct clk, which is exposed to the device and matches > the datasheet. If the clock has rate ops, clk_set_rate works with no > propagation. If it doesn't have a rate, clk_rate_ops is NULL, and the > framework deal with propagation only in the case where it is really > propagation - a child clock that requires changing a parent clock. > The block abstraction is still in place, there are just 3 slots for > blocks within each clock. > > Using a flag to mark clocks as "non-propagatable" is also not correct > - propagatability is a feature of the parent, not the child. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > From mboxrd@z Thu Jan 1 00:00:00 1970 From: linuxzsc@gmail.com (Richard Zhao) Date: Wed, 25 May 2011 10:32:09 +0800 Subject: [PATCH 0/4] Add a generic struct clk In-Reply-To: References: <1305876469.325655.313573683829.0.gpush@pororo> <20110524062620.GA22096@pengutronix.de> <20110524080936.GI20715@pengutronix.de> Message-ID: <20110525023209.GA2535@b20223-02.ap.freescale.net> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, May 24, 2011 at 12:41:20PM -0700, Colin Cross wrote: > On Tue, May 24, 2011 at 1:09 AM, Sascha Hauer wrote: > > On Tue, May 24, 2011 at 12:31:13AM -0700, Colin Cross wrote: > >> On Mon, May 23, 2011 at 11:26 PM, Sascha Hauer 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. > > Propagation is what I'm trying simplify, because it's impossible at > the framework level to determine the right time to propagate, and the > right time to return an error, and I don't like pushing it down to > each clock implementation either, because then you need a "propagating > clk gate" and a "non-propagating clk gate". > > Your building blocks implement every op - clk_gate_ops implements > set_rate as clk_parent_set_rate (won't that cause a deadlock on > prepare_lock when it calls clk_set_rate?). I'm suggesting breaking > out the clock gate ops into a struct that only contains: > enable > disable > prepare > unprepare > And a rate ops struct that contains: > round_rate > set_rate > And a mux ops struct that contains > set_parent > > For a single HW clock that has a mux, a gate, and a divider, the > existing implementation requires: > INIT_CLK(..., clk_mux_ops) > INIT_CLK(..., clk_div_ops) > INIT_CLK(..., clk_gate_ops) Not all clocks are like: pll -> [mux] -> [gate] -> [divider] -> clk A Some imx233 clocks are like: pll -> [divider] -> -----------\ [mux] -> clk B xtal -> [gate] -> [divider] -> / Thanks Richard > which creates 3 clocks, and requires lots of propagation logic to > figure out how to call clk_set_rate on the single clock that is > exposed to the device, but not propagate it past the device clock if > the device clock doesn't have a divider (propagating it up to an > active pll feeding other devices results in disaster, at least on > Tegra). This combination doesn't seem to be common in your MX code, > but _every_ Tegra device clock has these three parts. > > With multiple smaller building blocks that can fit inside a clock, all > you need is: > INIT_CLK(..., clk_mux_ops, clk_div_ops, clk_gate_ops) > You have one struct clk, which is exposed to the device and matches > the datasheet. If the clock has rate ops, clk_set_rate works with no > propagation. If it doesn't have a rate, clk_rate_ops is NULL, and the > framework deal with propagation only in the case where it is really > propagation - a child clock that requires changing a parent clock. > The block abstraction is still in place, there are just 3 slots for > blocks within each clock. > > Using a flag to mark clocks as "non-propagatable" is also not correct > - propagatability is a feature of the parent, not the child. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >