Message ID | alpine.DEB.2.00.1203201611330.22974@utopia.booyaka.com |
---|---|
State | New |
Headers | show |
On Tue, 20 Mar 2012, Paul Walmsley wrote: > We need to indicate in some way that the existing code and API is very > likely to change in ways that could involve quite a bit of work for > adopters. [...] > Anyway. It is okay if we want to have some starter common clock framework > in mainline; this is why deferring the merge hasn't been proposed. But > the point is that anyone who bases their code or platform on the common > clock framework needs to be aware that, to satisfy one of its major > use-cases, the behavior and/or API of the common clock code may need to > change significantly. Paul, While I understand your concern, please don't forget that the perfect is the enemy of the good. This common clk API has been under development for over *two* years already, with several attempts to merge it. And each previous merge attempt aborted because someone came along at the last minute to do exactly what you are doing i.e. underline all the flaws and call for a redesign. This is becoming a bad joke. We finally have something that the majority of reviewers are happy with and which should cover the majority of existing cases out there. Let's give it a chance, shall we? Otherwise one might ask where were you during those development years if you claim that the behavior and/or API of the common clock code still need to change significantly? > Explicitly stating this is not only simple courtesy to its users, many of > whom won't have been privy to its development. It also is intended to > make the code easier to change once it reaches mainline. The code will be easier to change once it is in mainline, simply due to the fact that you can also change all its users at once. And it is well possible that most users won't have to deal with the same magnitude of complexity as yours, again reducing the scope for resistance to changes. Let's see how things go with the current code and improve it incrementally. Otherwise no one will get involved with this API which is almost just as bad as not having the code merged at all. > So, until the API is well-defined and does all that it needs to do for its > major users, [...] No, the API simply can't ever be well defined if people don't actually start using it to eventually refine it further. Major users were converted to it, and in most cases The API does all that it needs to do already. Otherwise you'll be stuck in a catch22 situation forever. And APIs in the Linux kernel do change all the time. There is no stable API in the kernel. Extensions will come about eventually, and existing users (simple ones by definition if the current API already meets their needs) should be converted over easily. Nicolas
On 03/20/2012 08:15 PM, Nicolas Pitre wrote: > On Tue, 20 Mar 2012, Paul Walmsley wrote: > >> We need to indicate in some way that the existing code and API is very >> likely to change in ways that could involve quite a bit of work for >> adopters. > > [...] > >> Anyway. It is okay if we want to have some starter common clock framework >> in mainline; this is why deferring the merge hasn't been proposed. But >> the point is that anyone who bases their code or platform on the common >> clock framework needs to be aware that, to satisfy one of its major >> use-cases, the behavior and/or API of the common clock code may need to >> change significantly. > > Paul, > > While I understand your concern, please don't forget that the perfect is > the enemy of the good. > > This common clk API has been under development for over *two* years > already, with several attempts to merge it. And each previous merge > attempt aborted because someone came along at the last minute to do > exactly what you are doing i.e. underline all the flaws and call for a > redesign. This is becoming a bad joke. > > We finally have something that the majority of reviewers are happy with > and which should cover the majority of existing cases out there. Let's > give it a chance, shall we? Otherwise one might ask where were you > during those development years if you claim that the behavior and/or API > of the common clock code still need to change significantly? > >> Explicitly stating this is not only simple courtesy to its users, many of >> whom won't have been privy to its development. It also is intended to >> make the code easier to change once it reaches mainline. > > The code will be easier to change once it is in mainline, simply due to > the fact that you can also change all its users at once. And it is well > possible that most users won't have to deal with the same magnitude of > complexity as yours, again reducing the scope for resistance to changes. > > Let's see how things go with the current code and improve it > incrementally. Otherwise no one will get involved with this API which > is almost just as bad as not having the code merged at all. > >> So, until the API is well-defined and does all that it needs to do for its >> major users, [...] > > No, the API simply can't ever be well defined if people don't actually > start using it to eventually refine it further. Major users were > converted to it, and in most cases The API does all that it needs to do > already. Otherwise you'll be stuck in a catch22 situation forever. > > And APIs in the Linux kernel do change all the time. There is no stable > API in the kernel. Extensions will come about eventually, and existing > users (simple ones by definition if the current API already meets their > needs) should be converted over easily. +1 to the general idea in Nicolas's email. To add a few more thoughts, while I agree with Paul that there is room for improvement in the APIs, I think the difference in opinion comes when we ask the question: "When we eventually refine the APIs in the future to be more expressive, do we think that the current APIs can or cannot be made as wrappers around the new more expressive APIs?" Absolutes are almost never right, so I can't give an absolute answer. But I'm strongly leaning towards "we can" as the answer to the question. That combined with the reasons Nicholas mentioned is why I think the APIs should not be marked as experimental in anyway. -Saravana
Hello Nico, On Tue, 20 Mar 2012, Nicolas Pitre wrote: > This common clk API has been under development for over *two* years > already, with several attempts to merge it. And each previous merge > attempt aborted because someone came along at the last minute to do > exactly what you are doing i.e. underline all the flaws and call for a > redesign. This is becoming a bad joke. There is a misunderstanding. I am not calling for a redesign. I am simply stating that the current maturity level of the API and code should be documented in the Kconfig dependencies or description text before the code goes upstream. The objectives are to make future changes easier, set expectations, and clearly disclose the extent to which the API and code will need to change. > The code will be easier to change once it is in mainline, simply due to > the fact that you can also change all its users at once. And it is well > possible that most users won't have to deal with the same magnitude of > complexity as yours, again reducing the scope for resistance to changes. I hope you are right. To me, these conclusions seem unlikely. It seems equally likely that these rationales will make it much more difficult to change the code once it's upstream and platforms are depending on it. Particularly given the ongoing sensitivity to reducing "churn" of mainline code, so publicly discussed. So it seems like a good idea to attempt to address these potential roadblocks and criticisms to major clock framework changes early. > And APIs in the Linux kernel do change all the time. There is no stable > API in the kernel. Extensions will come about eventually, and existing > users (simple ones by definition if the current API already meets their > needs) should be converted over easily. Yes, simple extensions should be straightforward. Of greater concern are changes to the existing interface that will probably be required. These could involve significant changes to driver or platform code. It seems likely that there will be strong inertia to making these changes after platforms and drivers are converted. However, if we clearly state that these API changes are likely until the API is well-defined, we will hopefully reduce some angst by disclosing what some of us know. ... One last comment to address which is orthogonal to the technical content of this discussion. > Otherwise one might ask where were you during those development years if > you claim that the behavior and/or API of the common clock code still > need to change significantly? One might ask this. If one were to ask this, another might briefly outline the participation in public and private clock discussions at Linaro Connect in Budapest and Redwood Shores, at LPC in Santa Rosa, at ELCE/KS in Prague, at ELC in Redwood Shores, in conference calls, IRC sessions, and private E-mails with many of the people included in the header of this message, not to mention the list posts. None of the concerns that have been described are new. There has been an endeavour to discuss them with anyone who seemed even remotely interested. Of course it is a personal source of regret that the participation could not have been greater, but this regret is hardly limited to the common clock project. regards, - Paul
Hello Saravana, On Tue, 20 Mar 2012, Saravana Kannan wrote: > To add a few more thoughts, while I agree with Paul that there is room for > improvement in the APIs, I think the difference in opinion comes when we ask > the question: > > "When we eventually refine the APIs in the future to be more expressive, do we > think that the current APIs can or cannot be made as wrappers around the new > more expressive APIs?" > > Absolutes are almost never right, so I can't give an absolute answer. > But I'm strongly leaning towards "we can" as the answer to the question. > That combined with the reasons Nicholas mentioned is why I think the > APIs should not be marked as experimental in anyway. The resistance to documenting that the clock interface is not well-defined, and that therefore it is likely to change, and that users should not expect it to be stable, is perplexing to me. Certainly a Kconfig help text change seems trivial enough. But even the resistance to CONFIG_EXPERIMENTAL has been quite surprising to me, given that every single defconfig in arch/arm/defconfig sets it: $ find arch/arm/configs -type f | wc -l 122 $ fgrep -r CONFIG_EXPERIMENTAL=y arch/arm/configs | wc -l 122 $ (that includes iMX, by the way...) Certainly, neither Kconfig change is going to prevent us on OMAP from figuring out what else is needed to convert our platform to the common clock code. And given the level of enthusiasm on the lists, I don't think it's going to prevent many of the other ARM platforms from experimenting with the conversion, either. So it would be interesting to know more about why you (or anyone else) perceive that the Kconfig changes would be harmful. - Paul
On Wed, Mar 21, 2012 at 01:44:01AM -0600, Paul Walmsley wrote: > Hello Saravana, > > Certainly a Kconfig help text change seems trivial enough. But even the > resistance to CONFIG_EXPERIMENTAL has been quite surprising to me, given > that every single defconfig in arch/arm/defconfig sets it: > > $ find arch/arm/configs -type f | wc -l > 122 > $ fgrep -r CONFIG_EXPERIMENTAL=y arch/arm/configs | wc -l > 122 > $ > > (that includes iMX, by the way...) > > Certainly, neither Kconfig change is going to prevent us on OMAP from > figuring out what else is needed to convert our platform to the common > clock code. And given the level of enthusiasm on the lists, I don't think > it's going to prevent many of the other ARM platforms from experimenting > with the conversion, either. > > So it would be interesting to know more about why you (or anyone else) > perceive that the Kconfig changes would be harmful. Mainly because COMMON_CLK is an invisible option which has to be selected by architectures. So with the Kconfig change we either have to: config ARCH_MXC depends on EXPERIMENTAL or: config ARCH_MXC select EXPERIMENTAL select COMMON_CLK Neither of both seems very appealing to me. You can add a warning to the Kconfig help text if you like, I have no problem with that. As you said it will prevent noone from using it anyway. Sascha
On Wed, 21 Mar 2012, Paul Walmsley wrote: > Hello Nico, > > On Tue, 20 Mar 2012, Nicolas Pitre wrote: > > > This common clk API has been under development for over *two* years > > already, with several attempts to merge it. And each previous merge > > attempt aborted because someone came along at the last minute to do > > exactly what you are doing i.e. underline all the flaws and call for a > > redesign. This is becoming a bad joke. > > There is a misunderstanding. I am not calling for a redesign. I am > simply stating that the current maturity level of the API and code should > be documented in the Kconfig dependencies or description text before the > code goes upstream. The objectives are to make future changes easier, set > expectations, and clearly disclose the extent to which the API and code > will need to change. Maybe there is no actual consensus on that extent. > > The code will be easier to change once it is in mainline, simply due to > > the fact that you can also change all its users at once. And it is well > > possible that most users won't have to deal with the same magnitude of > > complexity as yours, again reducing the scope for resistance to changes. > > I hope you are right. To me, these conclusions seem unlikely. It seems > equally likely that these rationales will make it much more difficult to > change the code once it's upstream and platforms are depending on it. No code should go upstream if no one depends on it. Therefore we change code that many platforms depend on all the time. What is killing us is the need to synchronize with multiple external code bases scattered around. > Particularly given the ongoing sensitivity to reducing "churn" of mainline > code, so publicly discussed. Please stop buying into that crap. We congratulate ourselves every merge cycles with the current process being able to deal with around half a million of lines changed every 3 months or so. It's pointless churn that is frowned upon, not useful and incremental API changes. I don't think that "pointless" would apply here. > So it seems like a good idea to attempt to address these potential > roadblocks and criticisms to major clock framework changes early. I understand your concern, however I don't think it is as serious as you are making it. > One last comment to address which is orthogonal to the technical content > of this discussion. > > > Otherwise one might ask where were you during those development years if > > you claim that the behavior and/or API of the common clock code still > > need to change significantly? > > One might ask this. If one were to ask this, another might briefly > outline the participation in public and private clock discussions at > Linaro Connect in Budapest and Redwood Shores, at LPC in Santa Rosa, at > ELCE/KS in Prague, at ELC in Redwood Shores, in conference calls, IRC > sessions, and private E-mails with many of the people included in the > header of this message, not to mention the list posts. Sure. I was there too and came across you many times. I just don't understand why some apparent consensus was built around the current submission, that people involved appeared to be highly satisfied at last, given the emphasis you are giving to your present concern which doesn't seem to be widely shared. This is a bit surprising. > None of the concerns that have been described are new. There has been an > endeavour to discuss them with anyone who seemed even remotely interested. Let's change tactics then. We've been discussing this code for over two years and no one could really benefit from it during that time. Maybe future discussion could become more productive and concrete when some code is merged _and_ used at last. Nicolas
On 03/21/2012 12:44 AM, Paul Walmsley wrote: > Hello Saravana, > > On Tue, 20 Mar 2012, Saravana Kannan wrote: > >> To add a few more thoughts, while I agree with Paul that there is room for >> improvement in the APIs, I think the difference in opinion comes when we ask >> the question: >> >> "When we eventually refine the APIs in the future to be more expressive, do we >> think that the current APIs can or cannot be made as wrappers around the new >> more expressive APIs?" >> >> Absolutes are almost never right, so I can't give an absolute answer. >> But I'm strongly leaning towards "we can" as the answer to the question. >> That combined with the reasons Nicholas mentioned is why I think the >> APIs should not be marked as experimental in anyway. > > The resistance to documenting that the clock interface is not > well-defined, and that therefore it is likely to change, and that users > should not expect it to be stable, is perplexing to me. > > Certainly a Kconfig help text change seems trivial enough. But even the > resistance to CONFIG_EXPERIMENTAL has been quite surprising to me, given > that every single defconfig in arch/arm/defconfig sets it: > > $ find arch/arm/configs -type f | wc -l > 122 > $ fgrep -r CONFIG_EXPERIMENTAL=y arch/arm/configs | wc -l > 122 > $ > > (that includes iMX, by the way...) > > Certainly, neither Kconfig change is going to prevent us on OMAP from > figuring out what else is needed to convert our platform to the common > clock code. And given the level of enthusiasm on the lists, I think the enthusiasm we are seeing are from the clock driver developers. And for good reasons. Everyone is tired of having to write or rewrite their own implementation. > I don't think > it's going to prevent many of the other ARM platforms from experimenting > with the conversion, either. > > So it would be interesting to know more about why you (or anyone else) > perceive that the Kconfig changes would be harmful. But the enthusiasm of the clock driver developers doesn't necessarily translate to users of the clock APIs (other driver devs). My worry with marking it as experimental in Kconfig and to a certain extent in the documentation is that it will discourage the driver devs from switching to the new APIs. If no one is using the new APIs, then platforms can't switch to the common clock framework either. If a lot of platform don't convert to the common clock framework, the effort to get it merged in now is not also very meaningful. Also, at the rate at which we come to an agreement on new APIs, I think these new APIs should be considered quite stable :-) It's certainly better than what we have today. We should encourage driver devs to move to these new APIs and get the benefits while we go on another 2 yr cycle to agree on the next set of APIs improvements. And as I said before, I think it's much less likely that we will break backward source compatibility when we do the next improvement. We could have mostly avoid this large scale change by calling the APIs prepare/unprepare/gate/ungate (or whatever) and make clk_enable/disable similar to clk_prepare_enable/clk_disable_unprepare(). That would have avoid a lot of drivers from having to mess with their clock calls. But we didn't think about that in the excitement for improved APIs. I think this will still be seared in our minds enough to make sure we don't repeat that in the future. That covers all I have to say on this topic. -Saravana
On Wed, Mar 21, 2012 at 11:38:58AM -0700, Saravana Kannan wrote: > >So it would be interesting to know more about why you (or anyone else) > >perceive that the Kconfig changes would be harmful. > But the enthusiasm of the clock driver developers doesn't > necessarily translate to users of the clock APIs (other driver > devs). My worry with marking it as experimental in Kconfig and to a > certain extent in the documentation is that it will discourage the > driver devs from switching to the new APIs. If no one is using the > new APIs, then platforms can't switch to the common clock framework These aren't new APIs, the clock API has been around since forever. For driver authors working on anything that isn't platform specific the issue has been that it's not implemented at all on the overwhelming majority of architectures and those that do all have their own random implementations with their own random quirks and with no ability for anything except the platform to even try to do incredibly basic stuff like register a new clock. Simply having something, anything, in place even if it's going to churn is a massive step forward here for people working with clocks.
* Mark Brown <broonie@opensource.wolfsonmicro.com> [120321 12:11]: > On Wed, Mar 21, 2012 at 11:38:58AM -0700, Saravana Kannan wrote: > > > >So it would be interesting to know more about why you (or anyone else) > > >perceive that the Kconfig changes would be harmful. > > > But the enthusiasm of the clock driver developers doesn't > > necessarily translate to users of the clock APIs (other driver > > devs). My worry with marking it as experimental in Kconfig and to a > > certain extent in the documentation is that it will discourage the > > driver devs from switching to the new APIs. If no one is using the > > new APIs, then platforms can't switch to the common clock framework > > These aren't new APIs, the clock API has been around since forever. > For driver authors working on anything that isn't platform specific the > issue has been that it's not implemented at all on the overwhelming > majority of architectures and those that do all have their own random > implementations with their own random quirks and with no ability for > anything except the platform to even try to do incredibly basic stuff > like register a new clock. > > Simply having something, anything, in place even if it's going to churn > is a massive step forward here for people working with clocks. Right, and now at least the people reading this thread are pretty aware of the yet unsolved issues with clock fwk api :) Regards, Tony
On 03/21/2012 12:33 PM, Tony Lindgren wrote: > * Mark Brown<broonie@opensource.wolfsonmicro.com> [120321 12:11]: >> On Wed, Mar 21, 2012 at 11:38:58AM -0700, Saravana Kannan wrote: >> >>>> So it would be interesting to know more about why you (or anyone else) >>>> perceive that the Kconfig changes would be harmful. >> >>> But the enthusiasm of the clock driver developers doesn't >>> necessarily translate to users of the clock APIs (other driver >>> devs). My worry with marking it as experimental in Kconfig and to a >>> certain extent in the documentation is that it will discourage the >>> driver devs from switching to the new APIs. If no one is using the >>> new APIs, then platforms can't switch to the common clock framework >> >> These aren't new APIs, the clock API has been around since forever. I disagree. When I say clock API, I mean the actual functions and their semantics. Not the existence of a header file. The meaning of clk_enable/disable has been changed and they won't work without calling clk_prepare/unprepare. So, these are definitely new APIs. If it weren't new APIs, then none of the general drivers would need to change. >> For driver authors working on anything that isn't platform specific the >> issue has been that it's not implemented at all on the overwhelming >> majority of architectures and those that do all have their own random >> implementations with their own random quirks and with no ability for >> anything except the platform to even try to do incredibly basic stuff >> like register a new clock. >> >> Simply having something, anything, in place even if it's going to churn >> is a massive step forward here for people working with clocks. > > Right, and now at least the people reading this thread are pretty > aware of the yet unsolved issues with clock fwk api :) :-) Shhh... not so loud! -Saravana
On Wed, Mar 21, 2012 at 12:41:41PM -0700, Saravana Kannan wrote: > On 03/21/2012 12:33 PM, Tony Lindgren wrote: > >* Mark Brown<broonie@opensource.wolfsonmicro.com> [120321 12:11]: > >>These aren't new APIs, the clock API has been around since forever. > I disagree. When I say clock API, I mean the actual functions and > their semantics. Not the existence of a header file. > The meaning of clk_enable/disable has been changed and they won't > work without calling clk_prepare/unprepare. So, these are definitely > new APIs. If it weren't new APIs, then none of the general drivers > would need to change. clk_prepare() and clk_unprepare() are already there for any clk.h user and are stubbed in no matter what, they're really not a meaningful barrier here.
On 03/21/2012 12:56 PM, Mark Brown wrote: > On Wed, Mar 21, 2012 at 12:41:41PM -0700, Saravana Kannan wrote: >> On 03/21/2012 12:33 PM, Tony Lindgren wrote: >>> * Mark Brown<broonie@opensource.wolfsonmicro.com> [120321 12:11]: > >>>> These aren't new APIs, the clock API has been around since forever. > >> I disagree. When I say clock API, I mean the actual functions and >> their semantics. Not the existence of a header file. > >> The meaning of clk_enable/disable has been changed and they won't >> work without calling clk_prepare/unprepare. So, these are definitely >> new APIs. If it weren't new APIs, then none of the general drivers >> would need to change. > > clk_prepare() and clk_unprepare() are already there for any clk.h user > and are stubbed in no matter what, they're really not a meaningful > barrier here. Isn't this whole experimental vs non-experimental discussion about the actual external facing clock APIs and not the platform driver facing APIs? Sure, prepare/unprepare are already there in the .h file. But they are stubs and have no impact till we move to the common clock framework or platforms move to them with their own implementation (certainly not happening in upstream, so let's leave that part out of this discussion). So. IMO, for all practical purposes, common clk fwk forces the move to these new APIs and hence IMO forces the new APIs. -Saravana
On Wed, Mar 21, 2012 at 01:04:22PM -0700, Saravana Kannan wrote: > Sure, prepare/unprepare are already there in the .h file. But they > are stubs and have no impact till we move to the common clock > framework or platforms move to them with their own implementation > (certainly not happening in upstream, so let's leave that part out > of this discussion). > So. IMO, for all practical purposes, common clk fwk forces the move > to these new APIs and hence IMO forces the new APIs. Sure, if you want to look at it from that point of view - anything wanting to run on a platform which uses the generic API needs to use them, but there's no blocker on the user from this (it can convert with or without the platform it runs on) - but it's hardly a tough sell.
On Wed, Mar 21, 2012 at 12:41:41PM -0700, Saravana Kannan wrote: > The meaning of clk_enable/disable has been changed and they won't work > without calling clk_prepare/unprepare. So, these are definitely new > APIs. If it weren't new APIs, then none of the general drivers would > need to change. Yes and no. I disagree that the meaning of clk_enable/disable() has changed. It hasn't. What has changed is the preconditions for calling those functions, and necessarily so in the interest of being able to unify the different implementations.
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 2eaf17e..dd2d528 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -21,6 +21,11 @@ menuconfig COMMON_CLK this option for loadable modules requiring the common clock framework. + The API and implementation enabled by this option is still + incomplete. The API and implementation is expected to be + fluid and subject to change at any time, potentially + breaking existing users. + If in doubt, say "N". if COMMON_CLK