Re: [PATCH 0/3] OMAP: control: bootaddr and bootmod APIs
Hello Omar, On Wed, 2 May 2012, Omar Ramirez Luna wrote: Recently a patch went in for tidspbridge code, to ioremap SCM registers and solve a build break[1]. However it has been pointed out before that this is a layer violation given that control module should handle its own registers, this series is an attempt to create APIs for the users of these registers. With some adaptations this patch might also make use of it: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg66491.html Patch: staging: tidspbridge: use scm functions to set boot address and mode, will be sent separately to staging tree. Tested on OMAP3 Beagleboard. [1] http://www.mail-archive.com/devel@linuxdriverproject.org/msg18762.html Omar Ramirez Luna (3): OMAP2+: control: new APIs to configure boot address and mode OMAP: dsp: interface to control module functions staging: tidspbridge: use scm functions to set boot address and mode Thanks, queued for 3.6. - Paul ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
Hello Arnd, On Sat, 17 Mar 2012, Arnd Bergmann wrote: I think it's rather pointless, because the option is not going to be user selectable but will get selected by the platform unless I'm mistaken. The platform maintainers that care already know the state of the framework. This is where we have differing views, I think. Clearly, Sascha, Saravana, Rob, and I have at least slightly different opinions on the durability of the existing API and code. So it seems reasonable to assume that others who have not followed the development of the common clock code might mistake the implementation or API as being stable and well-defined. It sounds like the primary objection is to the use of CONFIG_EXPERIMENTAL. So here is a patch to simply note the status of this code in its Kconfig text. - Paul From: Paul Walmsley p...@pwsan.com Date: Tue, 20 Mar 2012 17:19:06 -0600 Subject: [PATCH] clk: note that the common clk code is still subject to significant change Indicate that the common clk code and API is still likely to require significant change. The API is not well-defined and both it and the underlying mechanics are likely to need significant changes to support non-trivial uses of the rate changing code, such as DVFS with external I/O devices. So any platforms that switch their implementation over to this may need to revise much of their driver code and revalidate their implementations until the behavior of the code is better-defined. A good time for removing this help text would be after at least two platforms that do DVFS on groups of external I/O devices have ported their clock implementations over to the common clk code. Signed-off-by: Paul Walmsley p...@pwsan.com Cc: Mike Turquette mturque...@ti.com --- drivers/clk/Kconfig |5 + 1 files changed, 5 insertions(+), 0 deletions(-) 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 -- 1.7.9.1 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
Hello Sascha,
On Sat, 17 Mar 2012, Sascha Hauer wrote:
On Fri, Mar 16, 2012 at 04:21:17PM -0600, Paul Walmsley wrote:
If the common clock code is to go upstream now, it should be marked as
experimental.
No, please don't do this. This effectively marks the architectures using
the generic clock framework experimental. We can mark drivers as 'you
have been warned', but marking an architecture as experimental is the
wrong sign for both its users and people willing to adopt the framework.
Also we get this:
warning: (ARCH_MX1 MACH_MX21 ARCH_MX25 MACH_MX27) selects COMMON_CLK
which has unmet direct dependencies (EXPERIMENTAL)
(and no, I don't want to support to clock frameworks in parallel)
It sounds as if your objection is with CONFIG_EXPERIMENTAL. If that is
indeed your objection, I personally have no objection to simply marking
the code experimental in the Kconfig text. (Patch at the bottom of this
message.)
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.
This is because we know the API is not well-defined, and
that both the API and the underlying mechanics will almost certainly need
to change for non-trivial uses of the rate changing code (e.g., DVFS with
external I/O devices).
Please leave DVFS out of the game. DVFS will use the clock framework for
the F part and the regulator framework for the V part, but the clock
framework should not get extended with DVFS features. The notifiers we
currently have in the clock framework should give enough information
for DVFS implementations.
Sadly, that's not so.
Consider a CPUFreq driver as one common clock framework user. This driver
will attempt to repeatedly change the rate of a clock. Changing that
clock's rate may also involve changing the rate of several other clocks
used by active devices. So drivers for these devices will need to
register rate change notifiers. The notifier callbacks might involve
heavyweight work, such as asserting flow control to an
externally-connected device.
Suppose now that the last registered device in the notifier chain cannot
handle the frequency transition and must abort it. This in turn will
require notifier callbacks to all of the previously-notified devices to
abort the change. And then shortly after that, CPUFreq is likely to
request the same frequency change again: hammering a notifier chain that
can never succeed.
Bad enough. We know at least one way to solve this problem. We can use
something like the clk_{block,allow}_changes() functions that have been
discussed for some time now. But these quickly reveal another problem in
the existing API. By default, when there are multiple enabled users of a
clock, another entity is allowed to change the clock's rate, or the rate
of any parent of that clock (!).
This has several implications. One that is significant is that for any
non-trivial clock tree, any driver that cares about its clock's rate will
need to implement notifier callbacks. This is because the driver has no
way of knowing if or when any other code on the system will attempt to
change that clock's rate or source. Or any parent clock's rate or source
might change. Should we really force all current drivers using the clock
code to implement these callbacks? Or can we restrict the situations in
which the clock's rate or parent can change by placing restrictions on the
API? But then, driver code may need to be rewritten, and behavior
assumptions may change.
Even if they don't and we have to change something here this will have
no influence on the architectures implementing their clock tree with the
common clock framework.
That sounds pretty confident. Are you sure that the semantics are so well
understood?
For example, should we allow clk_set_rate() on disabled clocks? How about
prepared, but disabled clocks? If so, what exactly should the clock
framework do in these circumstances? Should notifier callbacks go out
immediately to registered callbacks? Or should those callbacks be delayed
until the clock is prepared or enabled? How should that work when
clk_enable() cannot block? And are you confident that any other user of
the clock framework will answer these undefined questions in the same way
you would?
The above questions are simply scratching the surface. (Just as
examples, there are still significant questions about locking, reentrancy,
and so on - [1] is just one example)
These questions have reasonable answers that I think can be mostly aligned
on. Thinking through the use-cases, and implications, and answering them,
should have been the first task in working on the common clock code. I am
sorry to say -- and perhaps this is partially my fault -- that it seems as
if most people are not even aware that these questions exist, despite
discussions at several conferences and on the mailing
Re: [PATCH v7 1/3] Documentation: common clk API
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 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
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 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
Hi On Fri, 16 Mar 2012, Arnd Bergmann wrote: On Friday 16 March 2012, Arnd Bergmann wrote: Can we shoe-horn this thing into 3.4 (it is a bit late, i know) so that platform ports can gather speed? Several people are waiting for a somewhat stable version before starting their ports. And what is the path into mainline - will Russell carry it or Arnd (with Russell's blessing)? I would suggest putting it into a late/* branch of arm-soc so it finally gets some linux-next exposure, and then sending it in the second week of the merge window. Russell, please let me know if you want to carry it instead or if you have found any last-minute show stoppers in the code. FWIW, it's in arm-soc now [...] If the common clock code is to go upstream now, it should be marked as experimental. This is because we know the API is not well-defined, and that both the API and the underlying mechanics will almost certainly need to change for non-trivial uses of the rate changing code (e.g., DVFS with external I/O devices). While I understand and accept the motivation to get the common clk code upstream now, if platforms start to use it, they need to be aware that the behavior of the code can change significantly. These platforms may need to revalidate their implementations or change the way that many of their drivers use the clock functions. It also seems important to recognize that significant changes are still coming into the common clk code (e.g., the clk_set_rate() changes that came in just a few days ago). So, the following is a patch to mark it as experimental. It applies on the current head of arm-soc/next/clk (9d9f78ed9af0e465d2fd15550471956e7f559b9f). This should be a good compromise: it allows simple platforms or platforms with maintainers with lots of time to start converting over to the common clk code, while still clearly indicating that the API and underlying code is likely to change in significant ways. Once at least two major SoC ports have DVFS with external I/O devices safely up and running on their mainline kernels with the common clk code, I'd suggest removing the experimental designation. ... None of this is meant to reflect on Mike, by the way, who is doing a good job in a difficult situation. - Paul From: Paul Walmsley p...@pwsan.com Date: Fri, 16 Mar 2012 16:06:30 -0600 Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now Mark the common clk code as depending on CONFIG_EXPERIMENTAL. The API is not well-defined and both it and the underlying mechanics are likely to need significant changes to support non-trivial uses of the rate changing code, such as DVFS with external I/O devices. So any platforms that switch their implementation over to this may need to revise much of their driver code and revalidate their implementations until the behavior of the code is better-defined. A good time for removing this EXPERIMENTAL designation would be after at least two platforms that do DVFS on groups of external I/O devices have ported their clock implementations over to the common clk code. Signed-off-by: Paul Walmsley p...@pwsan.com Cc: Mike Turquette mturque...@ti.com --- drivers/clk/Kconfig |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 2eaf17e..a0a83de 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,6 +12,7 @@ config HAVE_MACH_CLKDEV menuconfig COMMON_CLK bool Common Clock Framework select HAVE_CLK_PREPARE + depends on EXPERIMENTAL ---help--- The common clock framework is a single definition of struct clk, useful across many platforms, as well as an -- 1.7.9.1 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 4/4] arm/dts: OMAP3: Add mmc controller nodes and board data
On Thu, 8 Mar 2012, Grant Likely wrote: Yes, absolutely use separate compatible values. It is always important to be specific as to the silicon implementing the IP. In that case, it is probably best to use the full chip name in the compatible string, e.g., omap2420 or omap2430 rather than just omap2. Particularly in the case of OMAP3, which is a largely meaningless group these days with AM33xx, OMAP34xx, OMAP36xx, and AM3517, many of which are quite different from each other... - Paul ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH v4] ARM: OMAP2+: hwmod: Add a new flag to handle hwmods left enabled at init
From: Rajendra Nayak rna...@ti.com
An hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in
enabled state by the hwmod framework post the initial setup.
Once a real user of the device (a driver) tries to enable it
at a later point, the hwmod framework throws a WARN() about
the device being already in enabled state.
Fix this by introducing a new internal flag '_HWMOD_SKIP_ENABLE' to
identify such devices/hwmods. When the device/hwmod is requested to be
enabled (the first time) by its driver/user, nothing except the
mux-enable is needed. The mux data is board specific and is
unavailable during initial enable() of the device, done by the
framework as part of setup().
A good example of a such a device is an UART used as debug console.
The UART module needs to be kept enabled through the boot, until the
UART driver takes control of it, for debug prints to appear on
the console.
Acked-by: Kevin Hilman khil...@ti.com
Acked-by: Benoit Cousson b-cous...@ti.com
Signed-off-by: Rajendra Nayak rna...@ti.com
[p...@pwsan.com: use a flag rather than a state; updated commit message;
edited some documentation]
Signed-off-by: Paul Walmsley p...@pwsan.com
---
This patch needs to be applied along with Govindraj Kevin's UART
patch set. Otherwise lots of nasty warnings are generated to the console
during boot. Applies on top of v3.2-rc5.
arch/arm/mach-omap2/omap_hwmod.c | 23 ++-
arch/arm/plat-omap/include/plat/omap_hwmod.h |3 +++
2 files changed, 25 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 529142a..f673f80 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1441,6 +1441,25 @@ static int _enable(struct omap_hwmod *oh)
pr_debug(omap_hwmod: %s: enabling\n, oh-name);
+ /*
+* hwmods with HWMOD_INIT_NO_IDLE flag set are left
+* in enabled state at init.
+* Now that someone is really trying to enable them,
+* just ensure that the hwmod mux is set.
+*/
+ if (oh-_int_flags _HWMOD_SKIP_ENABLE) {
+ /*
+* If the caller has mux data populated, do the mux'ing
+* which wouldn't have been done as part of the _enable()
+* done during setup.
+*/
+ if (oh-mux)
+ omap_hwmod_mux(oh-mux, _HWMOD_STATE_ENABLED);
+
+ oh-_int_flags = ~_HWMOD_SKIP_ENABLE;
+ return 0;
+ }
+
if (oh-_state != _HWMOD_STATE_INITIALIZED
oh-_state != _HWMOD_STATE_IDLE
oh-_state != _HWMOD_STATE_DISABLED) {
@@ -1744,8 +1763,10 @@ static int _setup(struct omap_hwmod *oh, void *data)
* it should be set by the core code as a runtime flag during startup
*/
if ((oh-flags HWMOD_INIT_NO_IDLE)
- (postsetup_state == _HWMOD_STATE_IDLE))
+ (postsetup_state == _HWMOD_STATE_IDLE)) {
+ oh-_int_flags |= _HWMOD_SKIP_ENABLE;
postsetup_state = _HWMOD_STATE_ENABLED;
+ }
if (postsetup_state == _HWMOD_STATE_IDLE)
_idle(oh);
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h
b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 8b372ed..1a13c02 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -416,10 +416,13 @@ struct omap_hwmod_omap4_prcm {
* _HWMOD_NO_MPU_PORT: no path exists for the MPU to write to this module
* _HWMOD_WAKEUP_ENABLED: set when the omap_hwmod code has enabled ENAWAKEUP
* _HWMOD_SYSCONFIG_LOADED: set when the OCP_SYSCONFIG value has been cached
+ * _HWMOD_SKIP_ENABLE: set if hwmod enabled during init (HWMOD_INIT_NO_IDLE) -
+ * causes the first call to _enable() to only update the pinmux
*/
#define _HWMOD_NO_MPU_PORT (1 0)
#define _HWMOD_WAKEUP_ENABLED (1 1)
#define _HWMOD_SYSCONFIG_LOADED(1 2)
+#define _HWMOD_SKIP_ENABLE (1 3)
/*
* omap_hwmod._state definitions
--
1.7.7.3
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3] ARM: OMAP2+: hwmod: Add a new state to handle hwmods left enabled at init
Hi
On Mon, 21 Nov 2011, Rajendra Nayak wrote:
An hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in
enabled state by the hwmod framework post the initial setup.
Once a real user of the device (a driver) tries to enable it
at a later point, the hwmod framework throws a WARN() about
the device being already in enabled state.
Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT'
to identify such devices/hwmods. When the device/hwmod
is requested to be enabled (the first time) by its driver/user,
nothing except the mux-enable and a state change to '_HWMOD_STATE_ENABLED'
is needed. The mux data is board specific and is unavailable during
initial enable() of the device, done by the framework as part of
setup().
A good example of a such a device is an UART used as debug console.
The UART module needs to be kept enabled through the boot, until the
UART driver takes control of it, for debug prints to appear on
the console.
Acked-by: Kevin Hilman khil...@ti.com
Acked-by: Benoit Cousson b-cous...@ti.com
Signed-off-by: Rajendra Nayak rna...@ti.com
I've tweaked this patch a little bit, mostly to avoid adding a new state,
which increases the complexity of the rest of the code that handles the
hwmod state machine. The modified patch below just uses an internal flag.
Please let me know if you have any comments.
- Paul
From: Rajendra Nayak rna...@ti.com
Date: Mon, 21 Nov 2011 17:42:50 +0530
Subject: [PATCH] ARM: OMAP2+: hwmod: Add a new flag to handle hwmods left
enabled at init
An hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in
enabled state by the hwmod framework post the initial setup.
Once a real user of the device (a driver) tries to enable it
at a later point, the hwmod framework throws a WARN() about
the device being already in enabled state.
Fix this by introducing a new internal '_HWMOD_SKIP_ENABLE' to
identify such devices/hwmods. When the device/hwmod is requested to be
enabled (the first time) by its driver/user, nothing except the
mux-enable is needed. The mux data is board specific and is
unavailable during initial enable() of the device, done by the
framework as part of setup().
A good example of a such a device is an UART used as debug console.
The UART module needs to be kept enabled through the boot, until the
UART driver takes control of it, for debug prints to appear on
the console.
Acked-by: Kevin Hilman khil...@ti.com
Acked-by: Benoit Cousson b-cous...@ti.com
Signed-off-by: Rajendra Nayak rna...@ti.com
[p...@pwsan.com: use a flag rather than a state; updated commit message;
edited some documentation]
Signed-off-by: Paul Walmsley p...@pwsan.com
---
arch/arm/mach-omap2/omap_hwmod.c | 23 ++-
arch/arm/plat-omap/include/plat/omap_hwmod.h |3 +++
2 files changed, 25 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index ebace0f..0a89335 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1449,6 +1449,25 @@ static int _enable(struct omap_hwmod *oh)
pr_debug(omap_hwmod: %s: enabling\n, oh-name);
+ /*
+* hwmods with HWMOD_INIT_NO_IDLE flag set are left
+* in enabled state at init.
+* Now that someone is really trying to enable them,
+* just ensure that the hwmod mux is set.
+*/
+ if (oh-_int_flags _HWMOD_SKIP_ENABLE) {
+ /*
+* If the caller has mux data populated, do the mux'ing
+* which wouldn't have been done as part of the _enable()
+* done during setup.
+*/
+ if (oh-mux)
+ omap_hwmod_mux(oh-mux, _HWMOD_STATE_ENABLED);
+
+ oh-_int_flags = ~_HWMOD_SKIP_ENABLE;
+ return 0;
+ }
+
if (oh-_state != _HWMOD_STATE_INITIALIZED
oh-_state != _HWMOD_STATE_IDLE
oh-_state != _HWMOD_STATE_DISABLED) {
@@ -1744,8 +1763,10 @@ static int _setup(struct omap_hwmod *oh, void *data)
* it should be set by the core code as a runtime flag during startup
*/
if ((oh-flags HWMOD_INIT_NO_IDLE)
- (postsetup_state == _HWMOD_STATE_IDLE))
+ (postsetup_state == _HWMOD_STATE_IDLE)) {
+ oh-_int_flags |= _HWMOD_SKIP_ENABLE;
postsetup_state = _HWMOD_STATE_ENABLED;
+ }
if (postsetup_state == _HWMOD_STATE_IDLE)
_idle(oh);
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h
b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 8b372ed..1a13c02 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -416,10 +416,13 @@ struct omap_hwmod_omap4_prcm {
* _HWMOD_NO_MPU_PORT: no path exists for the MPU to write to this module
* _HWMOD_WAKEUP_ENABLED: set when the omap_hwmod code has enabled ENAWAKEUP
* _HWMOD_SYSCONFIG_LOADED: set
Re: [PATCH 3/7] HACK: omap: convert 44xx data to common struct clk
Hi On Tue, 13 Dec 2011, Mike Turquette wrote: omap_clk_get_by_name must die. You do realize that it exists for a reason? That hardware clock names don't have anything to do with the Linux device model? - Paul ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 3/7] HACK: omap: convert 44xx data to common struct clk
On Tue, 13 Dec 2011, Turquette, Mike wrote: On Tue, Dec 13, 2011 at 8:27 PM, Paul Walmsley p...@pwsan.com wrote: On Tue, 13 Dec 2011, Mike Turquette wrote: omap_clk_get_by_name must die. You do realize that it exists for a reason? That hardware clock names don't have anything to do with the Linux device model? We have a tree structure of clks in the new common clk code, and a list of clks in clkdev (which admittedly is meant to be a subset, but in reality we register every OMAP clk with it), and then the omap clk list which is only really used by omap_get_clk_by_name for hwmod and some initialization stuff. What I'd really like to do is get rid of the OMAP clk code keeping track of it's clks in a separate list, which seems quite wasteful. Clock lookups that only involve a hardware clock name, with no device, should be the province of the clock code itself, not clkdev. The clkdev code may handle this today in the OMAP code, but this is simply due to legacy reasons. In general, on OMAP, we've got much faster ways now to implement clk_get(). - Paul___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Mon, 5 Dec 2011, Paul Walmsley wrote:
For example, here's a trivial implementation for rate recalculation for a
integer divider clock node (that can't be handled with a right shift):
s64 div(struct clk *clk, u32 div) {
if (clk-flags CLK_PARENT_RATE_MAX_U32)
return ((u32)(clk-parent-rate 0x)) / div;
clk-rate = clk-parent-rate;
do_div(clk-rate, div);
return clk-rate;
}
(removing some useless cruft from the above function, for clarity's sake)
s64 div(struct clk *clk, u32 div) {
s64 r;
if (clk-flags CLK_PARENT_RATE_MAX_U32)
return ((u32)clk-parent-rate) / div;
r = clk-parent-rate;
do_div(r, div);
return r;
}
div:
0: e92d4010push{r4, lr}
4: e1a04001mov r4, r1
8: e5d03010ldrbr3, [r0, #16]
c: e3130001tst r3, #1
10: 1a05bne 2c div+0x2c
14: e5903000ldr r3, [r0]
18: e1c300d8ldrdr0, [r3, #8]
1c: ebfebl 0 __do_div64
20: e1a2mov r0, r2
24: e1a01003mov r1, r3
28: e8bd8010pop {r4, pc}
2c: e590ldr r0, [r0]
30: e598ldr r0, [r0, #8]
34: ebfebl 0 __aeabi_uidiv
38: e3a01000mov r1, #0
3c: e8bd8010pop {r4, pc}
- Paul
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
On Mon, 5 Dec 2011, Russell King - ARM Linux wrote:
On Mon, Dec 05, 2011 at 02:15:56PM -0700, Paul Walmsley wrote:
But I'd propose that we instead increase the size of struct clk.rate to be
s64:
s64 clk_round_rate(struct clk *clk, s64 desired_rate);
int clk_set_rate(struct clk *clk, s64 rate);
s64 clk_get_rate(struct clk *clk);
struct clk {
...
s64 rate;
...
};
That way the clock framework can accommodate current clock rates, as well
as any conceivable future clock rate. (Some production CPUs are already
running at clock rates greater than 4 GiHZ[1]. RF devices with 4 GiHz+
clock rates are also common, such as 802.11a devices running in the 5.8
GHz band, and drivers for those may eventually wish to use the clock
framework.)
Yuck. You are aware that 64-bit math on 32-bit CPUs sucks? So burdening
_everything_ with 64-bit rate quantities is absurd. As for making then
64-bit signed quantities, that's asking for horrid code from gcc for the
majority of cases.
64-bit divides would be the only real issue in the clock framework
context. And those are easily avoided on clock hardware where the parent
rate can never exceed 32 bits.
For example, here's a trivial implementation for rate recalculation for a
integer divider clock node (that can't be handled with a right shift):
s64 div(struct clk *clk, u32 div) {
if (clk-flags CLK_PARENT_RATE_MAX_U32)
return ((u32)(clk-parent-rate 0x)) / div;
clk-rate = clk-parent-rate;
do_div(clk-rate, div);
return clk-rate;
}
gcc generates this for ARMv6:
0010 div:
10: e92d4038push{r3, r4, r5, lr}
14: e1a05000mov r5, r0
18: e5d03010ldrbr3, [r0, #16] @ clk-flags
1c: e1a04001mov r4, r1
20: e3130001tst r3, #1 @ 32-bit parent rate?
24: 1a07bne 48 div+0x38 @ do 32-bit divide
/* 64-bit divide by 32-bit follows */
28: e5903000ldr r3, [r0]
2c: e1c300d8ldrdr0, [r3, #8]
30: ebfebl 0 __do_div64
34: e1a2mov r0, r2
38: e1a01003mov r1, r3
3c: e5852008str r2, [r5, #8]
40: e585300cstr r3, [r5, #12]
44: e8bd8038pop {r3, r4, r5, pc}
/* 32-bit divide follows */
48: e590ldr r0, [r0]
4c: e598ldr r0, [r0, #8]
50: ebfebl 0 __aeabi_uidiv
54: e3a01000mov r1, #0
58: e8bd8038pop {r3, r4, r5, pc}
Not bad at all, and this isn't even optimized. (The conditional could be
avoided completely with a little work during clock init.) What little
overhead there is seems quite trivial if it means that the clock framework
will be useful for present and future devices.
- Paul
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
Hi Russell, On Thu, 1 Dec 2011, Russell King - ARM Linux wrote: On Wed, Nov 30, 2011 at 06:20:50PM -0700, Paul Walmsley wrote: 1. When a clock user calls clk_enable() on a clock, the clock framework should prevent other users of the clock from changing the clock's rate. This should persist until the clock user calls clk_disable() (but see also #2 below). This will ensure that clock users can rely on the rate returned by clk_get_rate(), as long as it's called between clk_enable() and clk_disable(). And since the clock's rate is guaranteed to remain the same during this time, code that cannot tolerate clock rate changes without special handling (such as driver code for external I/O devices) will work safely without further modification. So, if you have a PLL whose parent clock is not used by anything else. You want to program it to a certain rate. You call clk_disable() on the PLL clock. The approach described wouldn't require the PLL to be disabled before changing its rate. If there are no other users of the PLL, or if the other users of the PLL have indicated that it's safe for others to change the PLL's rate, the clock framework would allow the PLL rate change, even if the PLL is enabled. (modulo any notifier activity, and assuming that the underlying PLL hardware allows its frequency to be reprogrammed while the PLL is enabled) This walks up the tree and disables the parent. You then try to set the rate using clk_set_rate(). clk_set_rate() in this circumstance can't wait for the PLL to lock because it can't - there's no reference clock for it. As an aside, this seems like a good time to mention that the behavior of clk_set_rate() on a disabled clock needs to be clarified. - Paul ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
Hi
a brief comment concerning clock rates:
On Mon, 21 Nov 2011, Mike Turquette wrote:
+unsigned long clk_get_rate(struct clk *clk)
...
+long clk_round_rate(struct clk *clk, unsigned long rate)
...
+int clk_set_rate(struct clk *clk, unsigned long rate)
...
+struct clk {
...
+ unsigned long rate;
...
+};
The types associated with clock rates in the clock interface
(include/linux/clk.h) are inconsistent, and we should fix this.
clk_round_rate() is the problematic case, returning a signed long rather
than an unsigned long. So clk_round_rate() won't work correctly with any
rates higher than 2 GiHz.
We could fix the immediate problem by changing the prototype of
clk_round_rate() to pass the rounded rate back to the caller via a pointer
in one of the arguments, and return an error code (if any) via the return
value:
int clk_round_rate(struct clk *clk, unsigned long rate, unsigned long
*rounded_rate);
But I'd propose that we instead increase the size of struct clk.rate to be
s64:
s64 clk_round_rate(struct clk *clk, s64 desired_rate);
int clk_set_rate(struct clk *clk, s64 rate);
s64 clk_get_rate(struct clk *clk);
struct clk {
...
s64 rate;
...
};
That way the clock framework can accommodate current clock rates, as well
as any conceivable future clock rate. (Some production CPUs are already
running at clock rates greater than 4 GiHZ[1]. RF devices with 4 GiHz+
clock rates are also common, such as 802.11a devices running in the 5.8
GHz band, and drivers for those may eventually wish to use the clock
framework.)
- Paul
1. www.cpu-world.com, Intel Xeon X5698 - AT80614007314AA
http://www.cpu-world.com/CPUs/Xeon/Intel-Xeon%20X5698%20-%20AT80614007314AA.html
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 3/5] clk: introduce the common clock framework
Hi Mark,
On Thu, 1 Dec 2011, Mark Brown wrote:
On Wed, Nov 30, 2011 at 11:39:59PM -0700, Paul Walmsley wrote:
Clock rate/parent-change notifiers are requirements for DVFS use-cases,
and they must be paired with something like the
clk_{allow,block}_rate_change() functions to work efficiently. I intend
to comment on this later; it's not a simple problem. It might be worth
noting that Tero and I implemented a simplified version of this for the
N900.
I'm thinking that if we're going to have clk_{allow,block}_rate_change()
we may as well make that the main interface to enable rate changes - if
a device wants to change the clock rate it allows rate changes using
that interface rather than by disabling the clocks. I've got devices
which can do glitch free updates of active clocks so having to disable
would be a real restriction, and cpufreq would have issues with actually
disabling the clock too I expect.
The intention behind the clk_{allow,block}_rate_change() proposal was to
allow the current user of the clock to change its rate without having to
call clk_{allow,block}_rate_change(), if that driver was the sole user of
the clock.
So for example, if you had a driver that did:
c = clk_get(dev, clk_name);
clk_enable(c);
clk_set_rate(c, clk_rate);
and c was currently not enabled by any other driver on the system, and
nothing else had called clk_block_rate_change(c), then the rate change
would be allowed to proceed. (modulo any notifier activity, etc.)
So clk_{allow,block}_rate_change() was simply intended to allow or
restrict other users of the same clock, not the current user.
- Paul
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Problem booting IGEP when using device tree
On Thu, 19 May 2011, Grant Likely wrote: Would either of you mind looking at this bug report? On the IGEP, when booting with DT is used, something appears to get messed up with initializing the clocks. In this case, the device tree isn't actually doing anything other than replacing ATAGs for passing memory size, command line and initrd address. There /shouldn't/ be any difference between ATAG and DT booting, but obviously something has gone wrong. I've attached to the bug a diff between the log output of a working boot (ATAGs) and a non working boot (DT). https://bugs.launchpad.net/linux-linaro/+bug/768680 Mainline kernels don't change anything with the clock configuration before the Clocking rate (Crystal/Core/MPU): is printed. No idea about Linaro kernels; one would hope they don't act any differently in this regard. So given that the submitter is using different X-Loader and U-Boot versions: -Texas Instruments X-Loader 1.4.2-1 (Mar 22 2010 - 08:57:12) +Texas Instruments X-Loader 1.5.0 (Apr 11 2011 - 09:47:30) -U-Boot 2009.11-1 (Mar 22 2010 - 10:41:15) +U-Boot 2011.03 (Apr 20 2011 - 07:02:48) the problem is unlikely to be in the kernel. I'd suggest booting a kernel with ATAGs with the same X-Loader/U-boot versions and making sure that works. - Paul ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
