Message ID | 1334065553-7565-1-git-send-email-s.hauer@pengutronix.de |
---|---|
State | New |
Headers | show |
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
arch/arm/mach-imx/clock-imx1.c | 636 -----------
arch/arm/mach-imx/clock-imx21.c | 1239 --------------------
arch/arm/mach-imx/clock-imx25.c | 346 ------
arch/arm/mach-imx/clock-imx27.c | 785 -------------
arch/arm/mach-imx/clock-imx31.c | 630 -----------
arch/arm/mach-imx/clock-imx35.c | 536 ---------
arch/arm/mach-imx/clock-imx6q.c | 2111 -----------------------------------
arch/arm/mach-imx/clock-mx51-mx53.c | 1675 ---------------------------
8 files changed, 7958 deletions(-)
delete mode 100644 arch/arm/mach-imx/clock-imx1.c
delete mode 100644 arch/arm/mach-imx/clock-imx21.c
delete mode 100644 arch/arm/mach-imx/clock-imx25.c
delete mode 100644 arch/arm/mach-imx/clock-imx27.c
delete mode 100644 arch/arm/mach-imx/clock-imx31.c
delete mode 100644 arch/arm/mach-imx/clock-imx35.c
delete mode 100644 arch/arm/mach-imx/clock-imx6q.c
delete mode 100644 arch/arm/mach-imx/clock-mx51-mx53.c
diff --git a/arch/arm/mach-imx/clock-imx1.c b/arch/arm/mach-imx/clock-imx1.c
deleted file mode 100644
index 4aabeb2..0000000
diff --git a/arch/arm/mach-imx/clock-imx21.c b/arch/arm/mach-imx/clock-imx21.c
deleted file mode 100644
index ee15d8c..0000000
diff --git a/arch/arm/mach-imx/clock-imx25.c b/arch/arm/mach-imx/clock-imx25.c
deleted file mode 100644
index b0fec74c..0000000
diff --git a/arch/arm/mach-imx/clock-imx27.c b/arch/arm/mach-imx/clock-imx27.c
deleted file mode 100644
index 98e04f5..0000000
diff --git a/arch/arm/mach-imx/clock-imx31.c b/arch/arm/mach-imx/clock-imx31.c
deleted file mode 100644
index 3a943cd..0000000
diff --git a/arch/arm/mach-imx/clock-imx35.c b/arch/arm/mach-imx/clock-imx35.c
deleted file mode 100644
index e56c1a8..0000000
diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
deleted file mode 100644
index 111c328..0000000
diff --git a/arch/arm/mach-imx/clock-mx51-mx53.c b/arch/arm/mach-imx/clock-mx51-mx53.c
deleted file mode 100644
index 0847050..0000000
On Tue, Apr 10, 2012 at 03:45:14PM +0200, Sascha Hauer wrote: > With the generic clock framework we do not necessarily have > a pointer to the struct clk we want to register a lookup > for, so add a const char *clkname field to struct clk_lookup > so that a lookup can be registered with a clock name. All this silly names all over the place is getting to be utterly rediculous. Why do we need to name clocks and look them up by name when we have a perfectly good way (clkdev) to do this already? Yes, it takes a struct clk pointer but that's exactly because that's what it has to return. Why do we want to have another idiotic string to look up a clock which is named using an idiotic scheme to find out its pointer to only then register that with clkdev? I think this clk stuff has gone totally insane wrt names recently.
On Tue, Apr 10, 2012 at 03:30:55PM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 10, 2012 at 03:45:14PM +0200, Sascha Hauer wrote: > > With the generic clock framework we do not necessarily have > > a pointer to the struct clk we want to register a lookup > > for, so add a const char *clkname field to struct clk_lookup > > so that a lookup can be registered with a clock name. > > All this silly names all over the place is getting to be utterly > rediculous. Why do we need to name clocks and look them up by name > when we have a perfectly good way (clkdev) to do this already? Yes, > it takes a struct clk pointer but that's exactly because that's what > it has to return. > > Why do we want to have another idiotic string to look up a clock which > is named using an idiotic scheme to find out its pointer to only then > register that with clkdev? > > I think this clk stuff has gone totally insane wrt names recently. clk names and clk lookups are two different things. The former is a unique identifier for a clock whereas the latter associates a clock to a device. Multiple lookups can point to the same clock, so they can't be used to identify a particular clock. I know I'm not telling anything new to you here, so I think your question goes down to why we need a unique identifier string for clocks. For me the big advantage for clocks having unique names is that I do not have to have a struct clk * to pass the (possible) parents of a clock to the clock framework during registration. Also it makes us independent of the registration order of the clock tree (no need to register the parents before the children). The debugfs representation of the clock tree also makes use of the names. Sascha
On Tue, Apr 10, 2012 at 06:11:42PM +0200, Sascha Hauer wrote: > On Tue, Apr 10, 2012 at 03:30:55PM +0100, Russell King - ARM Linux wrote: > > On Tue, Apr 10, 2012 at 03:45:14PM +0200, Sascha Hauer wrote: > > > With the generic clock framework we do not necessarily have > > > a pointer to the struct clk we want to register a lookup > > > for, so add a const char *clkname field to struct clk_lookup > > > so that a lookup can be registered with a clock name. > > > > All this silly names all over the place is getting to be utterly > > rediculous. Why do we need to name clocks and look them up by name > > when we have a perfectly good way (clkdev) to do this already? Yes, > > it takes a struct clk pointer but that's exactly because that's what > > it has to return. > > > > Why do we want to have another idiotic string to look up a clock which > > is named using an idiotic scheme to find out its pointer to only then > > register that with clkdev? > > > > I think this clk stuff has gone totally insane wrt names recently. > > clk names and clk lookups are two different things. The former is a > unique identifier for a clock whereas the latter associates a clock > to a device. Multiple lookups can point to the same clock, so they can't > be used to identify a particular clock. I know I'm not telling anything > new to you here, so I think your question goes down to why we need a > unique identifier string for clocks. > > For me the big advantage for clocks having unique names is that I do not > have to have a struct clk * to pass the (possible) parents of a clock to > the clock framework during registration. Also it makes us independent of > the registration order of the clock tree (no need to register the > parents before the children). The debugfs representation of the clock > tree also makes use of the names. I don't like string loopup either. And after DT binding, we can use phandler to refer clk. BRs Richard
On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote: > I don't like string loopup either. And after DT binding, we can use > phandler to refer clk. No, this is only useful on platforms that use DT. This is a generic Linux API so it needs to support architectures and platforms that don't use DT as it's vanishingly unlikely that DT will ever be adopted by all platforms.
On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote: > On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote: > > > I don't like string loopup either. And after DT binding, we can use > > phandler to refer clk. > > No, this is only useful on platforms that use DT. This is a generic > Linux API so it needs to support architectures and platforms that don't > use DT as it's vanishingly unlikely that DT will ever be adopted by all > platforms. My point is using string lookup as less as possible. When one register a clk, one already got the struct clk* pointer and could use it in struct lookup. I'm worried about the performance as I saw string lookup is used more and more often. In fast boot case, for example, even 5ms is important. Thanks Richard
On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote: > On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote: > > No, this is only useful on platforms that use DT. This is a generic > > Linux API so it needs to support architectures and platforms that don't > > use DT as it's vanishingly unlikely that DT will ever be adopted by all > > platforms. > My point is using string lookup as less as possible. When one register That's not really relevant to what I'm saying here - I'm not commenting on your goal, I'm commenting on the solution. Obviously we want to be able to use things with DT but if we rely on it then we still need to solve the same problem for non-DT platforms. > a clk, one already got the struct clk* pointer and could use it in > struct lookup. How will you handle cases where one clock supplies another?
On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote: > On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote: > > On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote: > > > > > I don't like string loopup either. And after DT binding, we can use > > > phandler to refer clk. > > > > No, this is only useful on platforms that use DT. This is a generic > > Linux API so it needs to support architectures and platforms that don't > > use DT as it's vanishingly unlikely that DT will ever be adopted by all > > platforms. > My point is using string lookup as less as possible. When one register > a clk, one already got the struct clk* pointer and could use it in > struct lookup. > > I'm worried about the performance as I saw string lookup is used more > and more often. In fast boot case, for example, even 5ms is important. Yes, and you're not the only one who has that concern. What this patch does is turn a pair of string compares through a table into that plus another set of string compares across all struct clk, which will make the lookup yet more expensive. I see no reason why you'd register the cl_lookup structures before you know about which clks actually exist - and if they already exist, then you can already find the struct clk pointer and use that to register the proper return value for clk_get() via the clkdev APIs. As I've said in the past, naming individual clock structures is the stupidest thing that anyone ever did and causes nothing but problems. We've seen that in the drivers with the abuse of clk_get(NULL, name) where people wanted to pass the name in via platform data. We're starting to see the same thing here as well, except the problem people will see will be boot performance instead caused by multiple levels of string compares.
On Wed, Apr 11, 2012 at 10:15:49AM +0100, Mark Brown wrote: > On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote: > > a clk, one already got the struct clk* pointer and could use it in > > struct lookup. > > How will you handle cases where one clock supplies another? What has that got to do with clkdev (which is the topic in this thread)?
On Wed, Apr 11, 2012 at 10:21:47AM +0100, Russell King - ARM Linux wrote: > On Wed, Apr 11, 2012 at 10:15:49AM +0100, Mark Brown wrote: > > On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote: > > > a clk, one already got the struct clk* pointer and could use it in > > > struct lookup. > > How will you handle cases where one clock supplies another? > What has that got to do with clkdev (which is the topic in this thread)? I'm expecting (well, hoping) that at some point we'll want to connect clock trees between devices and it seems likely that clkdev will be pressed into service for that since it's how we're binding clocks at the minute. It may be that we come up with some other scheme for making those connections but as we appear to be defining a new scheme for binding clocks together anyway it seems sensible that we should at least be considering that case.
On Wed, Apr 11, 2012 at 10:32:54AM +0100, Mark Brown wrote: > On Wed, Apr 11, 2012 at 10:21:47AM +0100, Russell King - ARM Linux wrote: > > On Wed, Apr 11, 2012 at 10:15:49AM +0100, Mark Brown wrote: > > > On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote: > > > > a clk, one already got the struct clk* pointer and could use it in > > > > struct lookup. > > > > How will you handle cases where one clock supplies another? > > > What has that got to do with clkdev (which is the topic in this thread)? > > I'm expecting (well, hoping) that at some point we'll want to connect > clock trees between devices and it seems likely that clkdev will be > pressed into service for that since it's how we're binding clocks at the > minute. It may be that we come up with some other scheme for making > those connections but as we appear to be defining a new scheme for > binding clocks together anyway it seems sensible that we should at least > be considering that case. clkdev is just a mechanism to obtain a struct clk for a particular input on a particular device. Nothing more, nothing less. So how does this detail about how the struct clk ultimate gets used concern clkdev?
On Wed, Apr 11, 2012 at 10:20:34AM +0100, Russell King - ARM Linux wrote: > On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote: > > On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote: > > > On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote: > > > > > > > I don't like string loopup either. And after DT binding, we can use > > > > phandler to refer clk. > > > > > > No, this is only useful on platforms that use DT. This is a generic > > > Linux API so it needs to support architectures and platforms that don't > > > use DT as it's vanishingly unlikely that DT will ever be adopted by all > > > platforms. > > My point is using string lookup as less as possible. When one register > > a clk, one already got the struct clk* pointer and could use it in > > struct lookup. > > > > I'm worried about the performance as I saw string lookup is used more > > and more often. In fast boot case, for example, even 5ms is important. > > Yes, and you're not the only one who has that concern. What this patch > does is turn a pair of string compares through a table into that plus > another set of string compares across all struct clk, which will make > the lookup yet more expensive. > > I see no reason why you'd register the cl_lookup structures before you > know about which clks actually exist - and if they already exist, then > you can already find the struct clk pointer and use that to register > the proper return value for clk_get() via the clkdev APIs. Indeed I have struct clk pointers, so I can instead use a to-be-written combination of clkdev_alloc/clkdev_add. Would that be ok? Sascha
On Wed, Apr 11, 2012 at 11:42:31AM +0200, Sascha Hauer wrote: > On Wed, Apr 11, 2012 at 10:20:34AM +0100, Russell King - ARM Linux wrote: > > Yes, and you're not the only one who has that concern. What this patch > > does is turn a pair of string compares through a table into that plus > > another set of string compares across all struct clk, which will make > > the lookup yet more expensive. > > > > I see no reason why you'd register the cl_lookup structures before you > > know about which clks actually exist - and if they already exist, then > > you can already find the struct clk pointer and use that to register > > the proper return value for clk_get() via the clkdev APIs. > > Indeed I have struct clk pointers, so I can instead use a to-be-written > combination of clkdev_alloc/clkdev_add. Would that be ok? As we already have clkdev_alloc and clkdev_add, then I don't see that as a problem - except we may have to change __clkdev_alloc() so that it can be used before kmalloc() is up and running.
On Wed, Apr 11, 2012 at 10:41:36AM +0100, Russell King - ARM Linux wrote: > clkdev is just a mechanism to obtain a struct clk for a particular input > on a particular device. Nothing more, nothing less. So how does this > detail about how the struct clk ultimate gets used concern clkdev? It shouldn't. The main point I'm trying to make is that if we're adding an alternative mechanism to the direct pointers for specifying the bindings to clkdev (which is broadly the point of the original patch, AFAICT based on a desire to split the mapping out from the definitions) then having it rely on device tree in the first instance means we'll just need to solve the same problem again for non-DT systems. Richard was saying that the problem Sascha was trying to solve is irrelevant because we can rely on device tree but device tree is not widely available enough for that. The bit about clock to clock mappings was mostly an aside since it seems like it's roughly the same as what Sascha was trying to do and if we're going to add something new it'd be better if it handled that case also; that said re-reading Richard's message I'm not sure if this is actually a suggestion of a new mechanism.
On Wed, Apr 11, 2012 at 11:31:07AM +0100, Mark Brown wrote: > On Wed, Apr 11, 2012 at 10:41:36AM +0100, Russell King - ARM Linux wrote: > > clkdev is just a mechanism to obtain a struct clk for a particular input > > on a particular device. Nothing more, nothing less. So how does this > > detail about how the struct clk ultimate gets used concern clkdev? > > It shouldn't. > > The main point I'm trying to make is that if we're adding an alternative > mechanism to the direct pointers for specifying the bindings to clkdev > (which is broadly the point of the original patch, AFAICT based on a > desire to split the mapping out from the definitions) then having it rely > on device tree in the first instance means we'll just need to solve the > same problem again for non-DT systems. Richard was saying that the > problem Sascha was trying to solve is irrelevant because we can rely on > device tree but device tree is not widely available enough for that. Err what? Look, it's very very simple. clkdev is all about translating a device + connection ID to a struct clk pointer. That's all, nothing more, nothing less. What I see going on is that people want to translate a device + connection ID to some other name, and use this other name to look up a struct clk. Utterly idiotic and pointless. Whether it be OF or not OF. It's a crazy absurd idea. At some point, you have to do some kind of lookup and return a struct clk. Why the hell have this insane double mapping? Look, solving the DT problem with clkdev is really simple. You have the DT node for the struct device, so you can look up DT properties associated with that device. Properties have a name, and you can use the connection ID to translate into a DT property name to be looked up - eg, clk_%s. This can return a phandle or whatever else which could then be translated into a struct clk if you're representing each clk as a node in DT. Nice and simple, no need for any additional crappy translation what so ever - and, oh my god, it reduces the amount of list based searching required to do the translation. And it also allows non-DT to work just fine provided you register your clk lookups _after_ you know the struct clk pointers which is exactly what happens today. Now, if getting the struct clk pointers is insanely difficult because of the design of the common clk stuff, then that's where the problem lies, and that problem needs to be fixed, and clkdev does not need to be bodged around to fix a problem which is not relevant to it.
On Wed, Apr 11, 2012 at 12:43:11PM +0100, Russell King - ARM Linux wrote: > What I see going on is that people want to translate a device + connection > ID to some other name, and use this other name to look up a struct clk. > Utterly idiotic and pointless. Whether it be OF or not OF. > It's a crazy absurd idea. At some point, you have to do some kind of > lookup and return a struct clk. Why the hell have this insane double > mapping? I agree, that is silly. I had understood from Sascha's comments that the use case was building up the SoC clock tree with some indirect references. I can understand someone wanting to do that for a use case like grafting chunks of tree built up of static data together, it might be a bit more nicely data driven. > Look, solving the DT problem with clkdev is really simple. You have the > DT node for the struct device, so you can look up DT properties associated Yes, DT is completely straightforward here. > And it also allows non-DT to work just fine provided you register your > clk lookups _after_ you know the struct clk pointers which is exactly what > happens today. > Now, if getting the struct clk pointers is insanely difficult because of > the design of the common clk stuff, then that's where the problem lies, > and that problem needs to be fixed, and clkdev does not need to be bodged > around to fix a problem which is not relevant to it. I don't think it's anything particularly to do with the common clk stuff really (at least I share your opinion that it shouldn't be) but it does seem like it might be a real pain once we start deploying clock trees that go off SoC.
On 4/11/2012 3:17 PM, Russell King - ARM Linux wrote: >> > Indeed I have struct clk pointers, so I can instead use a to-be-written >> > combination of clkdev_alloc/clkdev_add. Would that be ok? > As we already have clkdev_alloc and clkdev_add, then I don't see that as > a problem - except we may have to change __clkdev_alloc() so that it can > be used before kmalloc() is up and running. Instead of platforms calling these routines, can we have these calls from clk_register_*() routines directly? So, for every clock registered with common clk framework this gets done automatically. We just need to pass dev & con id strings to these routines.
On 13 April 2012 11:33, Viresh Kumar <viresh.kumar@st.com> wrote: > On 4/11/2012 3:17 PM, Russell King - ARM Linux wrote: >>> > Indeed I have struct clk pointers, so I can instead use a to-be-written >>> > combination of clkdev_alloc/clkdev_add. Would that be ok? >> As we already have clkdev_alloc and clkdev_add, then I don't see that as >> a problem - except we may have to change __clkdev_alloc() so that it can >> be used before kmalloc() is up and running. > > Instead of platforms calling these routines, can we have these calls from > clk_register_*() routines directly? So, for every clock registered with > common clk framework this gets done automatically. We just need to pass > dev & con id strings to these routines. > No. Only small portion of the entire clock tree need to be in the lookup, usually the leaf clocks that device drivers need to access/manage. Regards, Shawn
On 4/13/2012 9:47 AM, Shawn Guo wrote: >> > >> > Instead of platforms calling these routines, can we have these calls from >> > clk_register_*() routines directly? So, for every clock registered with >> > common clk framework this gets done automatically. We just need to pass >> > dev & con id strings to these routines. >> > > No. > > Only small portion of the entire clock tree need to be in the lookup, > usually the leaf clocks that device drivers need to access/manage. I am not asking to enforce this for all clocks and create too many clk lookups. But do it for all clk_register() calls, that have valid dev_id or con_id. Apart from device clocks, there are other clocks, parent selection, etc that need it, so that we can do clk_get_sys().
On 13 April 2012 13:06, Viresh Kumar <viresh.kumar@st.com> wrote: > On 4/13/2012 9:47 AM, Shawn Guo wrote: >>> > >>> > Instead of platforms calling these routines, can we have these calls from >>> > clk_register_*() routines directly? So, for every clock registered with >>> > common clk framework this gets done automatically. We just need to pass >>> > dev & con id strings to these routines. >>> > >> No. >> >> Only small portion of the entire clock tree need to be in the lookup, >> usually the leaf clocks that device drivers need to access/manage. > > I am not asking to enforce this for all clocks and create too many clk lookups. > But do it for all clk_register() calls, that have valid dev_id or con_id. > It makes sense then. Only concern I have is the long argument list of clk_register() becomes even longer. Regards, Shawn > Apart from device clocks, there are other clocks, parent selection, etc > that need it, so that we can do clk_get_sys().
On Fri, Apr 13, 2012 at 01:22:24PM +0800, Shawn Guo wrote: > It makes sense then. Only concern I have is the long argument list of > clk_register() becomes even longer. Or we need to dump the arguments into a struct.
On 4/13/2012 2:29 PM, Mark Brown wrote:
> Or we need to dump the arguments into a struct.
Then we would end up creating too many structures in our mach/clock.c
files. That will look bad. May perform better. :)
On Fri, Apr 13, 2012 at 02:40:12PM +0530, Viresh Kumar wrote: > On 4/13/2012 2:29 PM, Mark Brown wrote: > > Or we need to dump the arguments into a struct. > > Then we would end up creating too many structures in our mach/clock.c > files. That will look bad. May perform better. :) or pass struct clk_lookup* ? For non-leaf clks, it's NULL. > > -- > viresh >
On Fri, Apr 13, 2012 at 05:17:13PM +0800, Richard Zhao wrote: > On Fri, Apr 13, 2012 at 02:40:12PM +0530, Viresh Kumar wrote: > > On 4/13/2012 2:29 PM, Mark Brown wrote: > > > Or we need to dump the arguments into a struct. > > > > Then we would end up creating too many structures in our mach/clock.c > > files. That will look bad. May perform better. :) > or pass struct clk_lookup* ? For non-leaf clks, it's NULL. What's wrong with the struct clk returned from clk_register() - why not do this as a two-step process? Especially as you may want to associate a single clock with more than one clk_lookup.
On 4/13/2012 2:56 PM, Russell King - ARM Linux wrote: > What's wrong with the struct clk returned from clk_register() - why > not do this as a two-step process? Code duplication on all platforms for creating these lookups. > Especially as you may want to > associate a single clock with more than one clk_lookup. Can still be done, as we are still getting clk pointer back.
On Fri, Apr 13, 2012 at 02:57:36PM +0530, Viresh Kumar wrote: > On 4/13/2012 2:56 PM, Russell King - ARM Linux wrote: > > What's wrong with the struct clk returned from clk_register() - why > > not do this as a two-step process? > > Code duplication on all platforms for creating these lookups. > > > Especially as you may want to > > associate a single clock with more than one clk_lookup. > > Can still be done, as we are still getting clk pointer back. You're not convincing me that you're approach here is correct, and I really doubt that this will have any effect at saving lines of code - your argument list is soo long that you'll have to wrap it onto several lines. So I don't buy the 'code duplication' argument here - I think that's a red herring, and I think you're better off making interfaces which are simpler to use.
On 4/13/2012 3:06 PM, Russell King - ARM Linux wrote: > You're not convincing me that you're approach here is correct, and :( > I really doubt that this will have any effect at saving lines of code - > your argument list is soo long that you'll have to wrap it onto several > lines. There are two ways here: 1. Leave clk_register() untouched and create lookups in machine code. 2. change clk_register(): A. pass all arguments as i mentioned in my patch B. pass structure instead of all so many args. Option 2 will have much lesser code than option 1, with both option A & B. In option 2.A.: we can create separate registration routines to save extra arguments passed, like: struct clk *clk_register_gate(struct device *dev, const char *name, const char *parent_name, unsigned long flags, void __iomem *reg, u8 bit_idx, u8 clk_gate_flags, spinlock_t *lock, struct clk_lookup **cl, const char *dev_id, const char *con_id); static inline struct clk *clk_register_gate_nolookup(struct device *dev, const char *name, const char *parent_name, unsigned long flags, void __iomem *reg, u8 bit_idx, u8 clk_gate_flags, spinlock_t *lock); Nodes, that don't need lookup stuff, can go ahead as earlier, just need to add _nolookup in existing routine name.
On Fri, Apr 13, 2012 at 03:32:43PM +0530, Viresh Kumar wrote: > Nodes, that don't need lookup stuff, can go ahead as earlier, just need > to add _nolookup in existing routine name. Or just have the core function handle NULL as a lookup array gracefully.
On Fri, Apr 13, 2012 at 03:32:43PM +0530, Viresh Kumar wrote: > On 4/13/2012 3:06 PM, Russell King - ARM Linux wrote: > > You're not convincing me that you're approach here is correct, and > > :( > > > I really doubt that this will have any effect at saving lines of code - > > your argument list is soo long that you'll have to wrap it onto several > > lines. > > There are two ways here: > 1. Leave clk_register() untouched and create lookups in machine code. > 2. change clk_register(): > A. pass all arguments as i mentioned in my patch > B. pass structure instead of all so many args. > > Option 2 will have much lesser code than option 1, with both option A & B. > In option 2.A.: we can create separate registration routines to save extra > arguments passed, like: > > struct clk *clk_register_gate(struct device *dev, const char *name, > const char *parent_name, unsigned long flags, > void __iomem *reg, u8 bit_idx, > u8 clk_gate_flags, spinlock_t *lock, struct clk_lookup **cl, > const char *dev_id, const char *con_id); So you're going to have something around 4-5 lines of arguments per function call to use this function. That's not elegant, that's not easy to use, that's not nice. And I doubt that it really solves any code space concern because the compiler will have to store pointers and values somewhere, and shuffle those into registers and onto the stack for every function call using additional code to do that. I really don't get why you think this is an improvement.
On 4/13/2012 3:50 PM, Russell King - ARM Linux wrote: > So you're going to have something around 4-5 lines of arguments per > function call to use this function. That's not elegant, that's not > easy to use, that's not nice. And I doubt that it really solves any > code space concern because the compiler will have to store pointers > and values somewhere, and shuffle those into registers and onto the > stack for every function call using additional code to do that. But it is still the same with the current implementation. Right now also we have too many arguments. Even if we do clkdev_alloc() in platform files. > I really don't get why you think this is an improvement. Only improvement is simplicity at the end of machine clock.c files, that don't need to do clkdev_alloc() for every clock they clk_lookups. I didn't liked this long of argument list, but couldn't think of something better, that makes machine clock code easy. :(
On Fri, Apr 13, 2012 at 3:20 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Apr 13, 2012 at 03:32:43PM +0530, Viresh Kumar wrote: >> On 4/13/2012 3:06 PM, Russell King - ARM Linux wrote: >> > You're not convincing me that you're approach here is correct, and >> >> :( >> >> > I really doubt that this will have any effect at saving lines of code - >> > your argument list is soo long that you'll have to wrap it onto several >> > lines. >> >> There are two ways here: >> 1. Leave clk_register() untouched and create lookups in machine code. >> 2. change clk_register(): >> A. pass all arguments as i mentioned in my patch >> B. pass structure instead of all so many args. >> >> Option 2 will have much lesser code than option 1, with both option A & B. >> In option 2.A.: we can create separate registration routines to save extra >> arguments passed, like: >> >> struct clk *clk_register_gate(struct device *dev, const char *name, >> const char *parent_name, unsigned long flags, >> void __iomem *reg, u8 bit_idx, >> u8 clk_gate_flags, spinlock_t *lock, struct clk_lookup **cl, >> const char *dev_id, const char *con_id); > > So you're going to have something around 4-5 lines of arguments per > function call to use this function. That's not elegant, that's not > easy to use, that's not nice. And I doubt that it really solves any > code space concern because the compiler will have to store pointers > and values somewhere, and shuffle those into registers and onto the > stack for every function call using additional code to do that. > > I really don't get why you think this is an improvement. My hope is that forthcoming struct clk_hw/static initializer patch will reduce the number of arguments to clk_register considerably. These interfaces are not frozen and everyone concerned with using the common framework is still figuring out exactly how this stuff should look. http://article.gmane.org/gmane.linux.ports.arm.msm/2587 Regards, Mike
On Thu, Apr 19, 2012 at 09:18:01AM +0530, Viresh Kumar wrote: > On 4/10/2012 7:15 PM, Sascha Hauer wrote: > > Having fixed factors/dividers in hardware is a common pattern, so > > add a basic clock type doing this. It basically describes a fixed > > factor clock using a nominator and a denominator. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/clk/Makefile | 2 +- > > drivers/clk/clk-fixed-factor.c | 97 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/clk-provider.h | 4 ++ > > 3 files changed, 102 insertions(+), 1 deletion(-) > > create mode 100644 drivers/clk/clk-fixed-factor.c > > @Mike: It would be better if you can take this patch atleast ASAP in your next, > so that Arnd can pull in SPEAr clk patches. > > Hi Sascha/Mike, > > Please see if following can be squashed with this patch. This clock is > used for SPEAr too :) > > --- > drivers/clk/clk-fixed-factor.c | 12 ++---------- > 1 files changed, 2 insertions(+), 10 deletions(-) > > diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c > index 7c5e1fc..aaf5a88 100644 > --- a/drivers/clk/clk-fixed-factor.c > +++ b/drivers/clk/clk-fixed-factor.c > @@ -16,7 +16,6 @@ struct clk_fixed_factor { > struct clk_hw hw; > unsigned int mult; > unsigned int div; > - char *parent[1]; add const here > }; > > #define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw) > @@ -75,22 +74,15 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name, > fix->mult = mult; > fix->div = div; > > - if (parent_name) { char *tmp = kstrdup(parent_name, GFP_KERNEL); if (!tmp) goto out; fix->parent[0] = tmp; > - } > - > clk = clk_register(dev, name, > &clk_fixed_factor_ops, &fix->hw, > - fix->parent, > + &parent_name, &parent_name is a pointer to a stack variable, right? clk_register saves it in the clk struct for later use, when the stack is already gone. > (parent_name ? 1 : 0), > flags); > + > if (clk) > return clk; > > -out: > - kfree(fix->parent[0]); > kfree(fix); > > return NULL; BRs, Domenico
On 4/19/2012 11:46 AM, Domenico Andreoli wrote: > On Thu, Apr 19, 2012 at 09:18:01AM +0530, Viresh Kumar wrote: >> On 4/10/2012 7:15 PM, Sascha Hauer wrote: >> diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c >> @@ -16,7 +16,6 @@ struct clk_fixed_factor { >> struct clk_hw hw; >> unsigned int mult; >> unsigned int div; >> - char *parent[1]; > > add const here > I removed it. :) >> clk = clk_register(dev, name, >> &clk_fixed_factor_ops, &fix->hw, >> - fix->parent, >> + &parent_name, > > &parent_name is a pointer to a stack variable, right? clk_register > saves it in the clk struct for later use, when the stack is already gone. Yes. It is on stack, but clk_register doesn't save it any more. Following is part of clk_register that saves /* copy each string name in case parent_names is __initdata */ for (i = 0; i < num_parents; i++) { clk->parent_names[i] = kstrdup(parent_names[i], GFP_KERNEL); if (!clk->parent_names[i]) { pr_err("%s: could not copy parent_names\n", __func__); goto fail_parent_names_copy; } } This happened with following patch sent by Mike. This isn't pushed till now. Author: Mike Turquette <mturquette@linaro.org> Date: Thu Apr 12 09:02:50 2012 +0800 clk: core: copy parent_names & return error codes This patch cleans up clk_register and solves a few bugs by teaching clk_register and __clk_init to return error codes (instead of just NULL) to better align with the existing clk.h api. Along with that change this patch also introduces a new behavior whereby clk_register copies the parent_names array, thus allowing platforms to declare their parent_names arrays as __initdata. -- Viresh
On Thu, Apr 19, 2012 at 09:18:01AM +0530, Viresh Kumar wrote: > On 4/10/2012 7:15 PM, Sascha Hauer wrote: > > Having fixed factors/dividers in hardware is a common pattern, so > > add a basic clock type doing this. It basically describes a fixed > > factor clock using a nominator and a denominator. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/clk/Makefile | 2 +- > > drivers/clk/clk-fixed-factor.c | 97 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/clk-provider.h | 4 ++ > > 3 files changed, 102 insertions(+), 1 deletion(-) > > create mode 100644 drivers/clk/clk-fixed-factor.c > > @Mike: It would be better if you can take this patch atleast ASAP in your next, > so that Arnd can pull in SPEAr clk patches. > > Hi Sascha/Mike, > > Please see if following can be squashed with this patch. This clock is > used for SPEAr too :) Generally yes, but I want to delay this until we have an official branch for clk-common-next (In which probably clk_register copies the parent names) Sascha > > --- > drivers/clk/clk-fixed-factor.c | 12 ++---------- > 1 files changed, 2 insertions(+), 10 deletions(-) > > diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c > index 7c5e1fc..aaf5a88 100644 > --- a/drivers/clk/clk-fixed-factor.c > +++ b/drivers/clk/clk-fixed-factor.c > @@ -16,7 +16,6 @@ struct clk_fixed_factor { > struct clk_hw hw; > unsigned int mult; > unsigned int div; > - char *parent[1]; > }; > > #define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw) > @@ -75,22 +74,15 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name, > fix->mult = mult; > fix->div = div; > > - if (parent_name) { > - fix->parent[0] = kstrdup(parent_name, GFP_KERNEL); > - if (!fix->parent[0]) > - goto out; > - } > - > clk = clk_register(dev, name, > &clk_fixed_factor_ops, &fix->hw, > - fix->parent, > + &parent_name, > (parent_name ? 1 : 0), > flags); > + > if (clk) > return clk; > > -out: > - kfree(fix->parent[0]); > kfree(fix); > > return NULL; > -- > 1.7.9 >
On Tue, Apr 10, 2012 at 03:45:22PM +0200, Sascha Hauer wrote: > the current i.MX clock support groups together unrelated clocks > to a single clock which is then used by the driver. This can't > be accomplished with the generic clock framework so we instead > request the individual clocks in the driver. For i.MX there are > generally three different clocks: > > ipg: bus clock (needed to access registers) > ahb: dma relevant clock, sometimes referred to as hclk in the datasheet > per: bit clock, pixel clock > > This patch changes the driver to request the individual clocks. > Currently all clk_get will get the same clock until the SoCs > are converted to the generic clock framework > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 42 ++++++++++++++++++++++++++---------- > 1 file changed, 31 insertions(+), 11 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index 6193a0d..938095a 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -69,6 +69,9 @@ struct pltfm_imx_data { > u32 scratchpad; > enum imx_esdhc_type devtype; > struct esdhc_platform_data boarddata; > + struct clk *clk_ipg; > + struct clk *clk_ahb; > + struct clk *clk_per; > }; > > static struct platform_device_id imx_esdhc_devtype[] = { > @@ -437,7 +440,6 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_host *host; > struct esdhc_platform_data *boarddata; > - struct clk *clk; > int err; > struct pltfm_imx_data *imx_data; > > @@ -458,14 +460,29 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev) > imx_data->devtype = pdev->id_entry->driver_data; > pltfm_host->priv = imx_data; > > - clk = clk_get(mmc_dev(host->mmc), NULL); > - if (IS_ERR(clk)) { > - dev_err(mmc_dev(host->mmc), "clk err\n"); > - err = PTR_ERR(clk); > + imx_data->clk_ipg = devm_clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(imx_data->clk_ipg)) { > + err = PTR_ERR(imx_data->clk_ipg); > goto err_clk_get; > } > - clk_prepare_enable(clk); > - pltfm_host->clk = clk; > + > + imx_data->clk_ahb = devm_clk_get(&pdev->dev, "ahb"); > + if (IS_ERR(imx_data->clk_ahb)) { > + err = PTR_ERR(imx_data->clk_ahb); > + goto err_clk_get; > + } > + > + imx_data->clk_per = devm_clk_get(&pdev->dev, "per"); > + if (IS_ERR(imx_data->clk_per)) { > + err = PTR_ERR(imx_data->clk_per); > + goto err_clk_get; > + } > + > + pltfm_host->clk = imx_data->clk_per; > + > + clk_prepare_enable(imx_data->clk_per); > + clk_prepare_enable(imx_data->clk_ipg); > + clk_prepare_enable(imx_data->clk_ahb); > > if (!is_imx25_esdhc(imx_data)) > host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; > @@ -559,8 +576,9 @@ no_card_detect_irq: > gpio_free(boarddata->wp_gpio); > no_card_detect_pin: > no_board_data: > - clk_disable_unprepare(pltfm_host->clk); > - clk_put(pltfm_host->clk); > + clk_disable_unprepare(imx_data->clk_per); > + clk_disable_unprepare(imx_data->clk_ipg); > + clk_disable_unprepare(imx_data->clk_ahb); You forgot to clk_put them. > err_clk_get: > kfree(imx_data); > err_imx_data: > @@ -586,8 +604,10 @@ static int __devexit sdhci_esdhc_imx_remove(struct platform_device *pdev) > gpio_free(boarddata->cd_gpio); > } > > - clk_disable_unprepare(pltfm_host->clk); > - clk_put(pltfm_host->clk); > + clk_disable_unprepare(imx_data->clk_per); > + clk_disable_unprepare(imx_data->clk_ipg); > + clk_disable_unprepare(imx_data->clk_ahb); ditto Thanks Richard > + > kfree(imx_data); > > sdhci_pltfm_free(pdev); > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
> > - clk_disable_unprepare(pltfm_host->clk); > > - clk_put(pltfm_host->clk); > > + clk_disable_unprepare(imx_data->clk_per); > > + clk_disable_unprepare(imx_data->clk_ipg); > > + clk_disable_unprepare(imx_data->clk_ahb); > You forgot to clk_put them. Sorry, I just realized you're using the new devm_clk_get. Richard
On Wed, Apr 11, 2012 at 10:20:34AM +0100, Russell King - ARM Linux wrote: > On Wed, Apr 11, 2012 at 04:45:29PM +0800, Richard Zhao wrote: > > On Wed, Apr 11, 2012 at 09:24:02AM +0100, Mark Brown wrote: > > > On Wed, Apr 11, 2012 at 09:11:50AM +0800, Richard Zhao wrote: > > > > > > > I don't like string loopup either. And after DT binding, we can use > > > > phandler to refer clk. > > > > > > No, this is only useful on platforms that use DT. This is a generic > > > Linux API so it needs to support architectures and platforms that don't > > > use DT as it's vanishingly unlikely that DT will ever be adopted by all > > > platforms. > > My point is using string lookup as less as possible. When one register > > a clk, one already got the struct clk* pointer and could use it in > > struct lookup. > > > > I'm worried about the performance as I saw string lookup is used more > > and more often. In fast boot case, for example, even 5ms is important. > > Yes, and you're not the only one who has that concern. What this patch > does is turn a pair of string compares through a table into that plus > another set of string compares across all struct clk, which will make > the lookup yet more expensive. > > I see no reason why you'd register the cl_lookup structures before you > know about which clks actually exist - and if they already exist, then > you can already find the struct clk pointer and use that to register > the proper return value for clk_get() via the clkdev APIs. > > As I've said in the past, naming individual clock structures is the > stupidest thing that anyone ever did and causes nothing but problems. > We've seen that in the drivers with the abuse of clk_get(NULL, name) > where people wanted to pass the name in via platform data. We're > starting to see the same thing here as well, except the problem people > will see will be boot performance instead caused by multiple levels of > string compares. I just realized an important benefit of string lookup is we can define machine specific lookups at its machine file. Without it, we have to expose machine specific clk twice. For example, cko1 is SoC internal clock name and is connected to sgtl5000 audio codec. {"sgtl5000-dev-name", "function name"} lookup is board specific, and {NULL, "cko1"} is SoC specific. We have to expose both of them. Thanks Richard >