Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Tero Kristo t-kri...@ti.com [150202 11:35]:
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock rates
(the parent rate may change also), so simple index value is not sufficient.
We need a handle of some sort to the bypass/ref clocks. The DPLL code
generally requires knowledge of the bypass + reference clock rates to work
properly, as it calculates the M/N values based on these.
Shall I change the DPLL code to check against clk_hw pointers or what is the
preferred approach here? The patch at the end does this and fixes the dpll
related warnings.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Thanks Tero, looks like your fix fixes all the issues I'm seeing with
commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
on 4430sdp, and off-idle not working for omap3.
I could not get the patch to apply, below is what I applied manually.
Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
some fuzz on that too. And inn that case, please feel also to add the
following for Tomeu's patch:
Tested-by: Tony Lindgren t...@atomide.com
8
From: Tero Kristo t-kri...@ti.com
Date: Mon, 2 Feb 2015 12:17:00 -0800
Subject: [PATCH] ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances)
Signed-off-by: Tero Kristo t-kri...@ti.com
[t...@atomide.com: updated to apply]
Signed-off-by: Tony Lindgren t...@atomide.com
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk-dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw-clk);
+ parent = __clk_get_hw(__clk_get_parent(hw-clk));
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
- WARN_ON(parent != dd-clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd-clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned
long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw-clk) != dd-clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw-clk)) !=
+ __clk_get_hw(dd-clk_ref))
return -EINVAL;
if (dd-last_rounded_rate == 0)
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock
On 02/01/2015 11:55 AM, Bjorn Andersson wrote: On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen o...@wizery.com wrote: On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson bj...@kryo.se wrote: In a system where you have two hwlock blocks lckA and lckB, each consisting of 8 locks and you have dspB that can only access lckB This is a good example - thanks. To be able to cope with such cases we will have to pass a hwlock block reference and its relative lock id. Correct, so the #hwlock-cells and hwlock part from the proposal are the important one. Having an optional hwlock-names will make things easier to read as well, but is not necessary. Right, if anything, it would be useful only for the clients, but the hwspinlock core itself would not need it. So, I would forgo adding the hwlock-names for now. The DT binding should definitely be prepared for such cases (just kill the base-id field?), but let's see what it means about the Linux implementation. From the dt binding PoV, we should be able to skip num-locks as well. It seems most hwlock blocks have a fixed amount of locks provided and the drivers are reporting this to the core when registering. I added this originally based on the initial MSM HW Mutex block bindings. So I think we can reduce the binding to: Providers: #hwlock-cells Consumers: hwlocks hwlock-names For the hardware where number of locks is actually variable (e.g. different variants of same block) there can be driver specific entries for this. Right, we should be able to drop this and use the driver match data. As it is, the field is used during registration of the block with the hwspinlock core. regards Suman -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: v3.19: Nokia N900 - usb nokia gadget crash
On Mon, Feb 02, 2015 at 04:17:13PM -0600, Felipe Balbi wrote: On Sat, Jan 31, 2015 at 10:25:09AM +0100, Pali Rohár wrote: On Saturday 31 January 2015 10:06:11 Pali Rohár wrote: Hello, when I try to modprobe g_nokia.ko gadget module on n900 device, it produce tons on lines to display and then crash and reboot device. So its not working and I (without Nokia (TM) serial console I cannot debug it). When I compile g_nokia into zImage (not as external module) then it kernel loads correctly and usb is working. Any idea why g_nokia as external kernel module cause problems, but compiled into zImage is working?? what I got here is a WARNING from f_phonet only, I'll look into it. Keep in mind I don't have N900 available, testing with beagleboneblack and AM437x SK. looks like there's a bug with MUSB too which needs fixing for that. Did you get inconsistent lock stae WARN ? -- balbi signature.asc Description: Digital signature
Re: v3.19: Nokia N900 - usb nokia gadget crash
On Sat, Jan 31, 2015 at 10:25:09AM +0100, Pali Rohár wrote: On Saturday 31 January 2015 10:06:11 Pali Rohár wrote: Hello, when I try to modprobe g_nokia.ko gadget module on n900 device, it produce tons on lines to display and then crash and reboot device. So its not working and I (without Nokia (TM) serial console I cannot debug it). When I compile g_nokia into zImage (not as external module) then it kernel loads correctly and usb is working. Any idea why g_nokia as external kernel module cause problems, but compiled into zImage is working?? what I got here is a WARNING from f_phonet only, I'll look into it. Keep in mind I don't have N900 available, testing with beagleboneblack and AM437x SK. -- balbi signature.asc Description: Digital signature
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/01/15 13:24, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little API
as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but
by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers.
Julia,
Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.
-Stephen
Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
dd-clk_ref = of_clk_get(node, 0);
dd-clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Mon, 2 Feb 2015, Stephen Boyd wrote:
On 02/01/15 13:24, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller,
but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers.
Julia,
Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.
Comparing or dereferencing pointers of a particular type should be
straightforward to check for. Is there an example of how to use the
parent_index value to fix the problem?
julia
-Stephen
Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
dd-clk_ref = of_clk_get(node, 0);
dd-clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock
On 02/01/2015 05:00 AM, Ohad Ben-Cohen wrote: On Sat, Jan 31, 2015 at 7:41 AM, Ohad Ben-Cohen o...@wizery.com wrote: On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson bj...@kryo.se wrote: In a system where you have two hwlock blocks lckA and lckB, each consisting of 8 locks and you have dspB that can only access lckB This is a good example - thanks. To be able to cope with such cases we will have to pass a hwlock block reference and its relative lock id. So, you mean lckB is only between the host and dspB. Obviously, if it were shared between dspA and dspB only, then the allocation and management would be completely outside the host Linux driver's scope. Additionally, to support such a scenario, we can no longer retain the simple dynamic allocation API we have today, because it might end up allocating dspB an hwlock from IckA. We will have to make sure hwlocks are allocated only from pools visible to the user, something that will change not only the hwspinlock API but also the way it maintains the hwlocks. Right, the current API definitely will not scale for that. It was designed around the concept that it's easier to exchange a single global id, rather than a lcbB:id or some other similar semantics that needs to be interpreted. I suspect we want to wait for such hardware to show up first, and only then add framework support for it. Agreed. regards Suman Regardless, we obviously do want to make sure our DT binding is prepared for the worse, so we'll drop the base-id field. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/1] extcon: usb-gpio: Introduce gpio usb extcon driver
Hi Roger,
Looks good to me. Applied it on v3.21 queue.
Thanks,
Chanwoo Choi
On 02/02/2015 07:21 PM, Roger Quadros wrote:
This driver observes the USB ID pin connected over a GPIO and
updates the USB cable extcon states accordingly.
The existing GPIO extcon driver is not suitable for this purpose
as it needs to be taught to understand USB cable states and it
can't handle more than one cable per instance.
For the USB case we need to handle 2 cable states.
1) USB (attach/detach)
2) USB-HOST (attach/detach)
This driver can be easily updated in the future to handle VBUS
events in case it happens to be available on GPIO for any platform.
Signed-off-by: Roger Quadros rog...@ti.com
---
v4:
- got rid of id_irqwake flag. Fail if enable/disable_irq_wake() fails
- changed host cable name to USB-HOST
- use 'depends on' instead of 'select' GPIOLIB
.../devicetree/bindings/extcon/extcon-usb-gpio.txt | 18 ++
drivers/extcon/Kconfig | 7 +
drivers/extcon/Makefile| 1 +
drivers/extcon/extcon-usb-gpio.c | 237
+
4 files changed, 263 insertions(+)
create mode 100644
Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
create mode 100644 drivers/extcon/extcon-usb-gpio.c
diff --git a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
new file mode 100644
index 000..85fe6b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
@@ -0,0 +1,18 @@
+USB GPIO Extcon device
+
+This is a virtual device used to generate USB cable states from the USB ID
pin
+connected to a GPIO pin.
+
+Required properties:
+- compatible: Should be linux,extcon-usb-gpio
+- id-gpio: gpio for USB ID pin. See gpio binding.
+
+Example:
I add some description for example as following:
+Example: Examples of extcon-usb-gpio node in dra7-evm.dts as listed below:
+ extcon_usb1 {
+ compatible = linux,extcon-usb-gpio;
+ id-gpio = gpio6 1 GPIO_ACTIVE_HIGH;
+ }
+
+ omap_dwc3_1 {
+ extcon = extcon_usb1;
+ };
[snip]
Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Mike Turquette mturque...@linaro.org [150202 14:51]: Quoting Tony Lindgren (2015-02-02 12:44:02) Thanks Tero, looks like your fix fixes all the issues I'm seeing with commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking on 4430sdp, and off-idle not working for omap3. I could not get the patch to apply, below is what I applied manually. Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with some fuzz on that too. And inn that case, please feel also to add the following for Tomeu's patch: Tested-by: Tony Lindgren t...@atomide.com Done and done. Things look good in my testing. I've pushed another branch out to the mirrors and hopefully the autobuild/autoboot testing will give us the green light. Thanks I just checked that your updated branch works for me now. This implementation can be revisited probably after 3.19 comes out if Tero doesn't like using clk_hw directly, or if we provide a better interface. Sounds like what Tero is saying also relates to knowing if the parent clock is in bypass mode or not in addition to the parent rate. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: v3.19: Nokia N900 - usb nokia gadget crash
On Mon, Feb 02, 2015 at 11:47:26PM +0100, Pali Rohár wrote: On Monday 02 February 2015 23:27:27 Felipe Balbi wrote: On Mon, Feb 02, 2015 at 04:17:13PM -0600, Felipe Balbi wrote: On Sat, Jan 31, 2015 at 10:25:09AM +0100, Pali Rohár wrote: On Saturday 31 January 2015 10:06:11 Pali Rohár wrote: Hello, when I try to modprobe g_nokia.ko gadget module on n900 device, it produce tons on lines to display and then crash and reboot device. So its not working and I (without Nokia (TM) serial console I cannot debug it). When I compile g_nokia into zImage (not as external module) then it kernel loads correctly and usb is working. Any idea why g_nokia as external kernel module cause problems, but compiled into zImage is working?? what I got here is a WARNING from f_phonet only, I'll look into it. Keep in mind I don't have N900 available, testing with beagleboneblack and AM437x SK. looks like there's a bug with MUSB too which needs fixing for that. Did you get inconsistent lock stae WARN ? I cannot read any message after I modprobe g_nokia.ko I just see that kernel try to overwrite couple of lines on n900 display and after 1-3 seconds device reboot. alright, I guess I found both bugs. I'll send patches soon. -- balbi signature.asc Description: Digital signature
Re: [PATCH] watchdog: Fix omap watchdogs to enable the magic close bit
* Wim Van Sebroeck w...@iguana.be [150202 15:44]: Hi Tony, * Tony Lindgren t...@atomide.com [141014 12:27]: This allows testing the watchdog easily with distros just by doing pkill -9 watchdog. Reported-by: Thomas Dziedzic gos...@gmail.com Signed-off-by: Tony Lindgren t...@atomide.com Wim, still not seeing this applied, did you forget about this one? I just applied it to linux-watchdog-next. Didn't forgot about it, just didn't had the time yet. OK thanks! Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/15 14:41, Mike Turquette wrote: Quoting Tero Kristo (2015-02-02 11:32:01) On 02/01/2015 11:24 PM, Mike Turquette wrote: AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. We can maybe introduce something like of_clk_get_parent_rate, as we have analogous stuff for getting parent names and indexes. Without introducing a new helper you could probably just do: clk_ref = clk_get_parent_by_index(dpll_clk, 0); ref_rate = clk_get_rate(clk_ref); clk_bypass = clk_get_parent_by_index(dpll_clk, 1); bypass_rate = clk_get_rate(clk_bypass); Currently the semantics around this call are weird. It seems like it would create a new struct clk pointer but it does not. So don't call clk_put on clk_ref and clk_bypass yet. That might change in the future as we iron out this brave new world that we all live in. Probably best to leave a FIXME in there. Stephen Tomeu, let me know if I got any of that wrong. The plan is to make clk_get_parent_by_index() return a clk_hw pointer instead of a clk pointer (probably with a new clk_get_parent_hw_by_index() API). Then drivers that are clk providers can deal in struct clk_hw and clk consumers can deal in struct clk, nicely splitting the API between consumers and providers on the structures they use to interact with the framework. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: v3.19: Nokia N900 - usb nokia gadget crash
On Monday 02 February 2015 23:27:27 Felipe Balbi wrote: On Mon, Feb 02, 2015 at 04:17:13PM -0600, Felipe Balbi wrote: On Sat, Jan 31, 2015 at 10:25:09AM +0100, Pali Rohár wrote: On Saturday 31 January 2015 10:06:11 Pali Rohár wrote: Hello, when I try to modprobe g_nokia.ko gadget module on n900 device, it produce tons on lines to display and then crash and reboot device. So its not working and I (without Nokia (TM) serial console I cannot debug it). When I compile g_nokia into zImage (not as external module) then it kernel loads correctly and usb is working. Any idea why g_nokia as external kernel module cause problems, but compiled into zImage is working?? what I got here is a WARNING from f_phonet only, I'll look into it. Keep in mind I don't have N900 available, testing with beagleboneblack and AM437x SK. looks like there's a bug with MUSB too which needs fixing for that. Did you get inconsistent lock stae WARN ? I cannot read any message after I modprobe g_nokia.ko I just see that kernel try to overwrite couple of lines on n900 display and after 1-3 seconds device reboot. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tero Kristo (2015-02-02 11:32:01)
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller,
but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
We can maybe introduce something like of_clk_get_parent_rate, as we have
analogous stuff for getting parent names and indexes. Without
introducing a new helper you could probably just do:
clk_ref = clk_get_parent_by_index(dpll_clk, 0);
ref_rate = clk_get_rate(clk_ref);
clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
bypass_rate = clk_get_rate(clk_bypass);
Currently the semantics around this call are weird. It seems like it
would create a new struct clk pointer but it does not. So don't call
clk_put on clk_ref and clk_bypass yet. That might change in the future
as we iron out this brave new world that we all live in. Probably best
to leave a FIXME in there.
Stephen Tomeu, let me know if I got any of that wrong.
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Yes, for now that is fine, but feels a bit hacky to me. I don't know
honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
but we might want to switch to something like the scheme above.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Fixed with Stephen's patch from last week. Thanks for dealing with all
the breakage so promptly. It has helped a lot!
Regards,
Mike
-Tero
Author: Tero Kristo t-kri...@ti.com
Date: Mon Feb 2 17:19:17 2015 +0200
ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining
runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Signed-off-by: Tero Kristo t-kri...@ti.com
Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk
instances)
diff --git
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tony Lindgren (2015-02-02 12:44:02)
* Tero Kristo t-kri...@ti.com [150202 11:35]:
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock rates
(the parent rate may change also), so simple index value is not sufficient.
We need a handle of some sort to the bypass/ref clocks. The DPLL code
generally requires knowledge of the bypass + reference clock rates to work
properly, as it calculates the M/N values based on these.
Shall I change the DPLL code to check against clk_hw pointers or what is the
preferred approach here? The patch at the end does this and fixes the dpll
related warnings.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Thanks Tero, looks like your fix fixes all the issues I'm seeing with
commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
on 4430sdp, and off-idle not working for omap3.
I could not get the patch to apply, below is what I applied manually.
Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
some fuzz on that too. And inn that case, please feel also to add the
following for Tomeu's patch:
Tested-by: Tony Lindgren t...@atomide.com
Done and done. Things look good in my testing. I've pushed another
branch out to the mirrors and hopefully the autobuild/autoboot testing
will give us the green light.
This implementation can be revisited probably after 3.19 comes out if
Tero doesn't like using clk_hw directly, or if we provide a better
interface.
Thanks,
Mike
8
From: Tero Kristo t-kri...@ti.com
Date: Mon, 2 Feb 2015 12:17:00 -0800
Subject: [PATCH] ARM: OMAP3+: clock: dpll: fix logic for comparing parent
clocks
DPLL code uses reference and bypass clock pointers for determining runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances)
Signed-off-by: Tero Kristo t-kri...@ti.com
[t...@atomide.com: updated to apply]
Signed-off-by: Tony Lindgren t...@atomide.com
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk-dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw-clk);
+ parent = __clk_get_hw(__clk_get_parent(hw-clk));
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
- WARN_ON(parent != dd-clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd-clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw,
unsigned long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw-clk) != dd-clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw-clk)) !=
+ __clk_get_hw(dd-clk_ref))
return -EINVAL;
if (dd-last_rounded_rate == 0)
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] watchdog: Fix omap watchdogs to enable the magic close bit
Hi Tony, * Tony Lindgren t...@atomide.com [141014 12:27]: This allows testing the watchdog easily with distros just by doing pkill -9 watchdog. Reported-by: Thomas Dziedzic gos...@gmail.com Signed-off-by: Tony Lindgren t...@atomide.com Wim, still not seeing this applied, did you forget about this one? I just applied it to linux-watchdog-next. Didn't forgot about it, just didn't had the time yet. Kind regards, Wim. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/15 13:31, Julia Lawall wrote: On Mon, 2 Feb 2015, Stephen Boyd wrote: Julia, Is there a way we can write a coccinelle script to check for this? The goal being to find all drivers that are comparing struct clk pointers or attempting to dereference them. There are probably other frameworks that could use the same type of check (regulator, gpiod, reset, pwm, etc.). Probably anything that has a get/put API. Comparing or dereferencing pointers of a particular type should be straightforward to check for. Is there an example of how to use the parent_index value to fix the problem? I'm not sure how to fix this case with parent_index values generically. I imagine it would be highly specific to the surrounding code to the point where we couldn't fix it automatically. For example, if it's a clk consumer (not provider like in this case) using an index wouldn't be the right fix. We would need some sort of clk API that we don't currently have and I would wonder why clock consumers even care to compare such pointers in the first place. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Stephen Boyd (2015-02-02 14:35:59) On 02/02/15 13:31, Julia Lawall wrote: On Mon, 2 Feb 2015, Stephen Boyd wrote: Julia, Is there a way we can write a coccinelle script to check for this? The goal being to find all drivers that are comparing struct clk pointers or attempting to dereference them. There are probably other frameworks that could use the same type of check (regulator, gpiod, reset, pwm, etc.). Probably anything that has a get/put API. Comparing or dereferencing pointers of a particular type should be straightforward to check for. Is there an example of how to use the parent_index value to fix the problem? I'm not sure how to fix this case with parent_index values generically. I imagine it would be highly specific to the surrounding code to the point where we couldn't fix it automatically. For example, if it's a clk consumer (not provider like in this case) using an index wouldn't be the right fix. We would need some sort of clk API that we don't currently have and I would wonder why clock consumers even care to compare such pointers in the first place. Ack. Is there precedent for a Don't do that kind of response? Regards, Mike -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: N900 v3.19-rc5 arm atags_to_fdt.c is broken
On Mon, 2 Feb 2015, Pavel Machek wrote: On Tue 2015-01-27 10:16:24, Nicolas Pitre wrote: On Tue, 27 Jan 2015, Pavel Machek wrote: I would say, problem is because omap3-n900 binary DT is too large I agree. OK if that's the case, then your patch makes sense to me. It also seems we can have the temporary stack be larger than the initial stack just for atags_to_fdt. The stack size isn't the issue, but rather its location. We need to position it away from the DT data. The DT size is known and we could use that, plus some room for the insertion of new data coming from the ATAG conversion. Something like the following would be a more robust solution: Tested-by: Pavel Machek pa...@ucw.cz (Note, that in 3.19 dts for n900 got too big, so we are actually triggering old bugs. That means that this is a regression fix, and should go in ASAP). It is queued here: http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8294/1 Hmm, but it should be on kernel.org, not in private arm trees, as it fixes a regression. Russell, you are the ARM maintainer, can you push it to Linus? The patch is included in RMK's fixes branch already, along with other fixes. I suppose it'll make its way to Linus before v3.19 final. Nicolas -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/7] usb: extcon: Fix USB-Host cable name
Hi Roger, On 02/02/2015 06:09 PM, Roger Quadros wrote: Chanwoo, On 02/02/15 07:04, Chanwoo Choi wrote: Hi Roger, On 01/30/2015 11:05 PM, Roger Quadros wrote: Hi, On 30/01/15 13:04, Roger Quadros wrote: Felipe Chanwoo, On 26/01/15 14:15, Roger Quadros wrote: The recommended name for USB-Host cable state is USB-Host and not USB-HOST as per drivers/extcon/extcon-class.c extcon_cable_name. Change all instances of USB-HOST to USB-Host. Signed-off-by: Roger Quadros rog...@ti.com Reviewed-by: Felipe Balbi ba...@ti.com Acked-by: Felipe Balbi ba...@ti.com This patch has no dependency to the rest so can be picked up as soon as possible. Do you think it is better to go via the USB tree? If yes then Chanwoo, can you please Ack this one? Thanks. This would mean that only the first patch needs to go through extcon tree as Tony will pick the rest. Hold on. Let's first decide what we really want to go ahead with USB-Host or USB-HOST. Currently, extcon driver have used the specific cable name(USB-Host or USB-HOST) without any standard way. So, I have plan to define common cable name in extcon header file by using capital letter. OK. In that case, this patch is not required. I will resend patch 1 with cable name corrected to USB-HOST. If you possbile, I want to use 'USB-HOST' cable name in drivers related to extcon. If we use different cable name, this cause the confusion to control cable. Thanks, Chanwoo -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: N900 v3.19-rc5 arm atags_to_fdt.c is broken
On Tue 2015-01-27 10:16:24, Nicolas Pitre wrote: On Tue, 27 Jan 2015, Pavel Machek wrote: I would say, problem is because omap3-n900 binary DT is too large I agree. OK if that's the case, then your patch makes sense to me. It also seems we can have the temporary stack be larger than the initial stack just for atags_to_fdt. The stack size isn't the issue, but rather its location. We need to position it away from the DT data. The DT size is known and we could use that, plus some room for the insertion of new data coming from the ATAG conversion. Something like the following would be a more robust solution: Tested-by: Pavel Machek pa...@ucw.cz (Note, that in 3.19 dts for n900 got too big, so we are actually triggering old bugs. That means that this is a regression fix, and should go in ASAP). It is queued here: http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8294/1 Hmm, but it should be on kernel.org, not in private arm trees, as it fixes a regression. Russell, you are the ARM maintainer, can you push it to Linus? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/7] usb: extcon: Fix USB-Host cable name
On 02/02/2015 07:01 PM, Roger Quadros wrote: On 02/02/15 11:55, Chanwoo Choi wrote: Hi Roger, On 02/02/2015 06:09 PM, Roger Quadros wrote: Chanwoo, On 02/02/15 07:04, Chanwoo Choi wrote: Hi Roger, On 01/30/2015 11:05 PM, Roger Quadros wrote: Hi, On 30/01/15 13:04, Roger Quadros wrote: Felipe Chanwoo, On 26/01/15 14:15, Roger Quadros wrote: The recommended name for USB-Host cable state is USB-Host and not USB-HOST as per drivers/extcon/extcon-class.c extcon_cable_name. Change all instances of USB-HOST to USB-Host. Signed-off-by: Roger Quadros rog...@ti.com Reviewed-by: Felipe Balbi ba...@ti.com Acked-by: Felipe Balbi ba...@ti.com This patch has no dependency to the rest so can be picked up as soon as possible. Do you think it is better to go via the USB tree? If yes then Chanwoo, can you please Ack this one? Thanks. This would mean that only the first patch needs to go through extcon tree as Tony will pick the rest. Hold on. Let's first decide what we really want to go ahead with USB-Host or USB-HOST. Currently, extcon driver have used the specific cable name(USB-Host or USB-HOST) without any standard way. So, I have plan to define common cable name in extcon header file by using capital letter. OK. In that case, this patch is not required. I will resend patch 1 with cable name corrected to USB-HOST. If you possbile, I want to use 'USB-HOST' cable name in drivers related to extcon. If we use different cable name, this cause the confusion to control cable. Kernel tree shows following users of USB-Host that will have to be changed to USB-HOST. You're right. I'll modify all cable name of 'USB-HOST'. Also, I have plan to use only capital letter for cable name. extcon-class.c: [EXTCON_USB_HOST] = USB-Host, extcon-max77693.c:[EXTCON_CABLE_USB_HOST] = USB-Host, extcon-max77693.c:extcon_set_cable_state(info-edev, USB-Host, attached); extcon-max8997.c: [EXTCON_CABLE_USB_HOST] = USB-Host, extcon-max8997.c: extcon_set_cable_state(info-edev, USB-Host, attached); extcon-rt8973a.c: [EXTCON_CABLE_USB_HOST] = USB-Host, extcon-sm5502.c: [EXTCON_CABLE_USB_HOST] = USB-Host, I'm not aware if any user space programs depend on this name. Do you know of any? As I knew, released samsung smart-phone used the cable name to detect the cable state becaues extcon send the uevent with both cable name and cable state. Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/1] extcon: usb-gpio: Introduce gpio usb extcon driver
This driver observes the USB ID pin connected over a GPIO and
updates the USB cable extcon states accordingly.
The existing GPIO extcon driver is not suitable for this purpose
as it needs to be taught to understand USB cable states and it
can't handle more than one cable per instance.
For the USB case we need to handle 2 cable states.
1) USB (attach/detach)
2) USB-HOST (attach/detach)
This driver can be easily updated in the future to handle VBUS
events in case it happens to be available on GPIO for any platform.
Signed-off-by: Roger Quadros rog...@ti.com
---
v4:
- got rid of id_irqwake flag. Fail if enable/disable_irq_wake() fails
- changed host cable name to USB-HOST
- use 'depends on' instead of 'select' GPIOLIB
.../devicetree/bindings/extcon/extcon-usb-gpio.txt | 18 ++
drivers/extcon/Kconfig | 7 +
drivers/extcon/Makefile| 1 +
drivers/extcon/extcon-usb-gpio.c | 237 +
4 files changed, 263 insertions(+)
create mode 100644 Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
create mode 100644 drivers/extcon/extcon-usb-gpio.c
diff --git a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
new file mode 100644
index 000..85fe6b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
@@ -0,0 +1,18 @@
+USB GPIO Extcon device
+
+This is a virtual device used to generate USB cable states from the USB ID pin
+connected to a GPIO pin.
+
+Required properties:
+- compatible: Should be linux,extcon-usb-gpio
+- id-gpio: gpio for USB ID pin. See gpio binding.
+
+Example:
+ extcon_usb1 {
+ compatible = linux,extcon-usb-gpio;
+ id-gpio = gpio6 1 GPIO_ACTIVE_HIGH;
+ }
+
+ omap_dwc3_1 {
+ extcon = extcon_usb1;
+ };
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 6a1f7de..e4c01ab 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -93,4 +93,11 @@ config EXTCON_SM5502
Silicon Mitus SM5502. The SM5502 is a USB port accessory
detector and switch.
+config EXTCON_USB_GPIO
+ tristate USB GPIO extcon support
+ depends on GPIOLIB
+ help
+ Say Y here to enable GPIO based USB cable detection extcon support.
+ Used typically if GPIO is used for USB ID pin detection.
+
endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0370b42..6a08a98 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o
obj-$(CONFIG_EXTCON_PALMAS)+= extcon-palmas.o
obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
obj-$(CONFIG_EXTCON_SM5502)+= extcon-sm5502.o
+obj-$(CONFIG_EXTCON_USB_GPIO) += extcon-usb-gpio.o
diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
new file mode 100644
index 000..3f0bad3
--- /dev/null
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -0,0 +1,237 @@
+/**
+ * drivers/extcon/extcon-usb-gpio.c - USB GPIO extcon driver
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
+ * Author: Roger Quadros rog...@ti.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/extcon.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/irq.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/of_gpio.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/workqueue.h
+
+#define USB_GPIO_DEBOUNCE_MS 20 /* ms */
+
+struct usb_extcon_info {
+ struct device *dev;
+ struct extcon_dev *edev;
+
+ struct gpio_desc *id_gpiod;
+ int id_irq;
+
+ unsigned long debounce_jiffies;
+ struct delayed_work wq_detcable;
+};
+
+/* List of detectable cables */
+enum {
+ EXTCON_CABLE_USB = 0,
+ EXTCON_CABLE_USB_HOST,
+
+ EXTCON_CABLE_END,
+};
+
+static const char *usb_extcon_cable[] = {
+ [EXTCON_CABLE_USB] = USB,
+ [EXTCON_CABLE_USB_HOST] = USB-HOST,
+ NULL,
+};
+
+static void usb_extcon_detect_cable(struct work_struct *work)
+{
+ int id;
+ struct usb_extcon_info *info = container_of(to_delayed_work(work),
+ struct usb_extcon_info,
+ wq_detcable);
+
+ /* check ID and update cable state */
+ id = gpiod_get_value_cansleep(info-id_gpiod);
+ if (id) {
+
Re: [PATCH v2 2/7] usb: extcon: Fix USB-Host cable name
On 02/02/15 11:55, Chanwoo Choi wrote: Hi Roger, On 02/02/2015 06:09 PM, Roger Quadros wrote: Chanwoo, On 02/02/15 07:04, Chanwoo Choi wrote: Hi Roger, On 01/30/2015 11:05 PM, Roger Quadros wrote: Hi, On 30/01/15 13:04, Roger Quadros wrote: Felipe Chanwoo, On 26/01/15 14:15, Roger Quadros wrote: The recommended name for USB-Host cable state is USB-Host and not USB-HOST as per drivers/extcon/extcon-class.c extcon_cable_name. Change all instances of USB-HOST to USB-Host. Signed-off-by: Roger Quadros rog...@ti.com Reviewed-by: Felipe Balbi ba...@ti.com Acked-by: Felipe Balbi ba...@ti.com This patch has no dependency to the rest so can be picked up as soon as possible. Do you think it is better to go via the USB tree? If yes then Chanwoo, can you please Ack this one? Thanks. This would mean that only the first patch needs to go through extcon tree as Tony will pick the rest. Hold on. Let's first decide what we really want to go ahead with USB-Host or USB-HOST. Currently, extcon driver have used the specific cable name(USB-Host or USB-HOST) without any standard way. So, I have plan to define common cable name in extcon header file by using capital letter. OK. In that case, this patch is not required. I will resend patch 1 with cable name corrected to USB-HOST. If you possbile, I want to use 'USB-HOST' cable name in drivers related to extcon. If we use different cable name, this cause the confusion to control cable. Kernel tree shows following users of USB-Host that will have to be changed to USB-HOST. extcon-class.c: [EXTCON_USB_HOST] = USB-Host, extcon-max77693.c: [EXTCON_CABLE_USB_HOST] = USB-Host, extcon-max77693.c: extcon_set_cable_state(info-edev, USB-Host, attached); extcon-max8997.c: [EXTCON_CABLE_USB_HOST] = USB-Host, extcon-max8997.c: extcon_set_cable_state(info-edev, USB-Host, attached); extcon-rt8973a.c: [EXTCON_CABLE_USB_HOST] = USB-Host, extcon-sm5502.c:[EXTCON_CABLE_USB_HOST] = USB-Host, I'm not aware if any user space programs depend on this name. Do you know of any? cheers, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] ARM: dts: Add minimal support for dm8168-evm
On 2 February 2015 at 18:44, Tony Lindgren t...@atomide.com wrote:
* Matthijs van Duin matthijsvand...@gmail.com [150128 13:46]:
On 26 January 2015 at 16:58, Tony Lindgren t...@atomide.com wrote:
I'm pretty sure I verified the that the audio_pll_clk1 is hardwwired
to 32KiHz by looking at it with a scope on the clkout pin.
Yeah it comes straight from the rtcdivider so it'll produce
0.0016384 * devosc.
Not much about rtcdivider in the dm816x TRM..
[..]
That seems to be in the dm814x TRM. OK so it looks like in
mainline kernel drivers/clk/ti/fapll.c we can then fix the hardcoded
value in ti_fapll_synth_recalc_rate() to return the rate of fd-clk_ref
multiplied 0.0016384 instead of 32768.
Ah sorry, yes, for some reason I thought you meant the dm814x one...
(which makes no sense in retrospect)
It seems however the situation is actually similar on the dm816x since
the diagram of the audio FAPLL also shows clock 1 deriving directly
from devosc, and the associated audio frequency/divider 1 registers
read as zero. Presumably it was produced by the audio PLL in some
early design but replaced by a fixed synthesizer later on, later named
rtcdivider in the dm814x. Note that the multiplier 0.0016384 = 32768
/ 20e6 is for the dm814x, it'll be 32768 / 27e6 on the dm816x.
I think audio 1 should be kept out of the fapll code entirely given
that it doesn't seem to be related to the audio FAPLL anymore in any
way other than in name.
I've also been looking at the rest of Netra's 32KiHz clock tree, but
the TRM is a real mess there, often contradicting both itself and the
TI kernel. As far as I can tell they agree on:
audio_1 = devosc * (32768 / 27e6);
audio_a = audio_1 / (CM_DPLL_APA_CLKSEL + 1);
rtc_fck = { clkin32, audio_a }[CM_DPLL_SYSCLK18_CLKSEL];
but the TRM seems to suggest the mux output is the sec_clk32k
specifically for the RTC while the signal I called audio_a here is
sysclk18 provided for debouncing and as timer option. In contrast the
TI kernel considers the mux the only user of audio_a and its output
sysclk18. It does also define a secure 32k but without parent or any
apparent users.
(The audio FAPLL description (1.10.3.1.4) also claims mux option 0
selects the synthesized 32k clock, but that contradicts all other
evidence so I will ignore that.)
I'm leaning more towards believing the TI kernel since on Centaurus
(all flavors) sysclk18 definitely comes from the mux. It adds a twist
however: PRCM's clkin32 doesn't come straight from the pin but PLLSS
inserts another mux which by default *also* feeds it the synthesized
32k:
rtcdivider = devosc * (32768 / 20e6); // former audio 1
prcm_clkin32 = { rtcdivider, clkin32 }[PLLSS_SYSCLK18_CLKSRC];
sysclk18_a = rtcdivider / (CM_DPLL_RTCDIVA_CLKSEL + 1);
sysclk18 = { prcm_clkin32, sysclk18_a }[CM_DPLL_SYSCLK18_CLKSEL];
I confirmed the above on our board where clkin32 is not connected, so
I could manually toggle it using the internal pull up/down. This
also revealed that mux in PLLSS, unlike the PRCM one, is not
glitch-free but takes immediate effect. It also revealed the TRM's
statement that If newly selected clock is idle, a switchover never
occurs (previously selected clock continues to pass through the mux)
is false. Switching sysclk18 to the idle clkin32 stopped timers fed by
it (including the synctimer32k) and switching back also proved
impossible without toggling clkin32 or having PLLSS feed it the
rtcdivider clock. This may be the reason this (otherwise redundant)
mux exists in the first place.
Finally, Centaurus also has an internal ~32kHz RC osc as option for
the watchdog and on the 811x (also known as Jacinto 5 Eco) the clkin32
is upgraded from clock input to oscillator.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] ASoC: tlv320aic3x: Add support for tlv320aic3104
Disables GPIO support and LINE2 input and renames Mic3 input to Mic2,
if tlv320aic3104 mode is seleced. Devicetree binding document is
updated accordingly.
Signed-off-by: Jyri Sarha jsa...@ti.com
---
Changes since the first version of the patch
- Added ti,tlv320aic3104 to tlv320aic3x_of_match table
What surprises me is how come the code worked even before this change?
.../devicetree/bindings/sound/tlv320aic3x.txt | 10 +-
sound/soc/codecs/tlv320aic3x.c | 345 +++--
2 files changed, 253 insertions(+), 102 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/tlv320aic3x.txt
b/Documentation/devicetree/bindings/sound/tlv320aic3x.txt
index 5e6040c..47a213c 100644
--- a/Documentation/devicetree/bindings/sound/tlv320aic3x.txt
+++ b/Documentation/devicetree/bindings/sound/tlv320aic3x.txt
@@ -9,6 +9,7 @@ Required properties:
ti,tlv320aic33 - TLV320AIC33
ti,tlv320aic3007 - TLV320AIC3007
ti,tlv320aic3106 - TLV320AIC3106
+ti,tlv320aic3104 - TLV320AIC3104
- reg - int - I2C slave address
@@ -18,6 +19,7 @@ Optional properties:
- gpio-reset - gpio pin number used for codec reset
- ai3x-gpio-func - array of 2 int - AIC3X_GPIO1 AIC3X_GPIO2 Functionality
+ - Not supported on tlv320aic3104
- ai3x-micbias-vg - MicBias Voltage required.
1 - MICBIAS output is powered to 2.0V,
2 - MICBIAS output is powered to 2.5V,
@@ -36,7 +38,13 @@ CODEC output pins:
* HPLCOM
* HPRCOM
-CODEC input pins:
+CODEC input pins for TLV320AIC3104:
+ * MIC2L
+ * MIC2R
+ * LINE1L
+ * LINE1R
+
+CODEC input pins for other compatible codecs:
* MIC3L
* MIC3R
* LINE1L
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index b7ebce0..cb92cdb 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -87,6 +87,7 @@ struct aic3x_priv {
#define AIC3X_MODEL_3X 0
#define AIC3X_MODEL_33 1
#define AIC3X_MODEL_3007 2
+#define AIC3X_MODEL_3104 3
u16 model;
/* Selects the micbias voltage */
@@ -316,52 +317,37 @@ static const struct snd_kcontrol_new aic3x_snd_controls[]
= {
* only for swapped L-to-R and R-to-L routes. See below stereo controls
* for direct L-to-L and R-to-R routes.
*/
- SOC_SINGLE_TLV(Left Line Mixer Line2R Bypass Volume,
- LINE2R_2_LLOPM_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Left Line Mixer PGAR Bypass Volume,
PGAR_2_LLOPM_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Left Line Mixer DACR1 Playback Volume,
DACR1_2_LLOPM_VOL, 0, 118, 1, output_stage_tlv),
- SOC_SINGLE_TLV(Right Line Mixer Line2L Bypass Volume,
- LINE2L_2_RLOPM_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Right Line Mixer PGAL Bypass Volume,
PGAL_2_RLOPM_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Right Line Mixer DACL1 Playback Volume,
DACL1_2_RLOPM_VOL, 0, 118, 1, output_stage_tlv),
- SOC_SINGLE_TLV(Left HP Mixer Line2R Bypass Volume,
- LINE2R_2_HPLOUT_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Left HP Mixer PGAR Bypass Volume,
PGAR_2_HPLOUT_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Left HP Mixer DACR1 Playback Volume,
DACR1_2_HPLOUT_VOL, 0, 118, 1, output_stage_tlv),
- SOC_SINGLE_TLV(Right HP Mixer Line2L Bypass Volume,
- LINE2L_2_HPROUT_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Right HP Mixer PGAL Bypass Volume,
PGAL_2_HPROUT_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Right HP Mixer DACL1 Playback Volume,
DACL1_2_HPROUT_VOL, 0, 118, 1, output_stage_tlv),
- SOC_SINGLE_TLV(Left HPCOM Mixer Line2R Bypass Volume,
- LINE2R_2_HPLCOM_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Left HPCOM Mixer PGAR Bypass Volume,
PGAR_2_HPLCOM_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Left HPCOM Mixer DACR1 Playback Volume,
DACR1_2_HPLCOM_VOL, 0, 118, 1, output_stage_tlv),
- SOC_SINGLE_TLV(Right HPCOM Mixer Line2L Bypass Volume,
- LINE2L_2_HPRCOM_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Right HPCOM Mixer PGAL Bypass Volume,
PGAL_2_HPRCOM_VOL, 0, 118, 1, output_stage_tlv),
SOC_SINGLE_TLV(Right HPCOM Mixer DACL1 Playback Volume,
DACL1_2_HPRCOM_VOL, 0, 118, 1, output_stage_tlv),
/* Stereo output controls for direct L-to-L and R-to-R routes */
- SOC_DOUBLE_R_TLV(Line Line2 Bypass Volume,
-LINE2L_2_LLOPM_VOL, LINE2R_2_RLOPM_VOL,
-0, 118, 1, output_stage_tlv),
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/2015 11:41 PM, Mike Turquette wrote:
Quoting Tero Kristo (2015-02-02 11:32:01)
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller,
but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
We can maybe introduce something like of_clk_get_parent_rate, as we have
analogous stuff for getting parent names and indexes. Without
introducing a new helper you could probably just do:
clk_ref = clk_get_parent_by_index(dpll_clk, 0);
ref_rate = clk_get_rate(clk_ref);
clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
bypass_rate = clk_get_rate(clk_bypass);
Currently the semantics around this call are weird. It seems like it
would create a new struct clk pointer but it does not. So don't call
clk_put on clk_ref and clk_bypass yet. That might change in the future
as we iron out this brave new world that we all live in. Probably best
to leave a FIXME in there.
Stephen Tomeu, let me know if I got any of that wrong.
I think you got it right, just wanted to mention that we can and
probably should make the clk_get_parent_* calls in the consumer API to
return per-user clk instances but that we need to make sure first that
callers call clk_put afterwards.
This should also allow us to remove the reference to struct clk from
clk_hw, which is at best awkward.
Regards,
Tomeu
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Yes, for now that is fine, but feels a bit hacky to me. I don't know
honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
but we might want to switch to something like the scheme above.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Fixed with Stephen's patch from last week. Thanks for dealing with all
the breakage so promptly. It has helped a lot!
Regards,
Mike
-Tero
Author: Tero Kristo t-kri...@ti.com
Date: Mon Feb 2 17:19:17 2015 +0200
ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining
runtime
properties for these clocks, like parent clock rates.
As clock API now returns
Re: [PATCH v13 4/6] clk: Add rate constraints to clocks
* Geert Uytterhoeven ge...@linux-m68k.org [150202 00:03]:
On Sun, Feb 1, 2015 at 11:18 PM, Mike Turquette mturque...@linaro.org wrote:
Quoting Tomeu Vizoso (2015-01-31 10:36:22)
On 31 January 2015 at 02:31, Stephen Boyd sb...@codeaurora.org wrote:
On 01/29, Stephen Boyd wrote:
On 01/29/15 05:31, Geert Uytterhoeven wrote:
Hi Tomeu, Mike,
On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
tomeu.viz...@collabora.com wrote:
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
return 1;
}
-static void clk_core_put(struct clk_core *core)
+void __clk_put(struct clk *clk)
{
struct module *owner;
- owner = core-owner;
+ if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+ return;
clk_prepare_lock();
- kref_put(core-ref, __clk_release);
+
+ hlist_del(clk-child_node);
+ clk_core_set_rate_nolock(clk-core, clk-core-req_rate);
At this point, clk-core-req_rate is still zero, causing
cpg_div6_clock_round_rate() to be called with a zero rate parameter,
e.g. on r8a7791:
Hmm.. I wonder if we should assign core-req_rate to be the same as
core-rate during __clk_init()? That would make this call to
clk_core_set_rate_nolock() a nop in this case.
Here's a patch to do this
---8
From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] clk: Assign a requested rate by default
We need to assign a requested rate here so that we avoid
requesting a rate of 0 on clocks when we remove clock consumers.
Hi, this looks good to me.
Reviewed-by: Tomeu Vizoso tomeu.viz...@collabora.com
It seems to fix the total boot failure on OMAPs, and hopefully does the
same for SH Mobile and others. I've squashed this into Tomeu's rate
constraints patch to maintain bisect.
Yes, it fixes shmobile. .round_rate() is now called with a sane value of rate.
Looks like next-20150202 now produces tons of the following errors,
these from omap4:
[ 10.568206] [ cut here ]
[ 10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925
clk_disable+0x28/0x34()
[ 10.568237] Modules linked in:
[ 10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW
3.19.0-rc6-next-20150202 #2037
[ 10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree)
[ 10.568267] [c0015bdc] (unwind_backtrace) from [c001222c]
(show_stack+0x10/0x14)
[ 10.568267] [c001222c] (show_stack) from [c05d2014]
(dump_stack+0x84/0x9c)
[ 10.568267] [c05d2014] (dump_stack) from [c003ea90]
(warn_slowpath_common+0x7c/0xb8)
[ 10.568298] [c003ea90] (warn_slowpath_common) from [c003eb68]
(warn_slowpath_null+0x1c/0x24)
[ 10.568298] [c003eb68] (warn_slowpath_null) from [c04c1ffc]
(clk_disable+0x28/0x34)
[ 10.568328] [c04c1ffc] (clk_disable) from [c0025b3c]
(_disable_clocks+0x18/0x68)
[ 10.568328] [c0025b3c] (_disable_clocks) from [c0026f14]
(_idle+0x10c/0x214)
[ 10.568328] [c0026f14] (_idle) from [c0855fac] (_setup+0x338/0x410)
[ 10.568359] [c0855fac] (_setup) from [c0027360]
(omap_hwmod_for_each+0x34/0x60)
[ 10.568359] [c0027360] (omap_hwmod_for_each) from [c08563c4]
(__omap_hwmod_setup_all+0x30/0x40)
[ 10.568389] [c08563c4] (__omap_hwmod_setup_all) from [c0008a04]
(do_one_initcall+0x80/0x1dc)
[ 10.568389] [c0008a04] (do_one_initcall) from [c0848ea0]
(kernel_init_freeable+0x204/0x2d0)
[ 10.568420] [c0848ea0] (kernel_init_freeable) from [c05cdab8]
(kernel_init+0x8/0xec)
[ 10.568420] [c05cdab8] (kernel_init) from [c000e790]
(ret_from_fork+0x14/0x24)
[ 10.568420] ---[ end trace cb88537fdc8fa211 ]---
[ 10.568450] [ cut here ]
[ 10.568450] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436
omap3_noncore_dpll_enable+0xdc/0
x10c()
[ 10.568450] Modules linked in:
[ 10.568481] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW
3.19.0-rc6-next-20150202 #2037
[ 10.568481] Hardware name: Generic OMAP4 (Flattened Device Tree)
[ 10.568481] [c0015bdc] (unwind_backtrace) from [c001222c]
(show_stack+0x10/0x14)
[ 10.568511] [c001222c] (show_stack) from [c05d2014]
(dump_stack+0x84/0x9c)
[ 10.568511] [c05d2014] (dump_stack) from [c003ea90]
(warn_slowpath_common+0x7c/0xb8)
[ 10.568511] [c003ea90] (warn_slowpath_common) from [c003eb68]
(warn_slowpath_null+0x1c/0x24)
[ 10.568542] [c003eb68] (warn_slowpath_null) from [c0035800]
(omap3_noncore_dpll_enable+0xdc/0x10c)
[ 10.568542] [c0035800] (omap3_noncore_dpll_enable) from [c04c0a10]
(clk_core_enable+0x60/0x9c)
[ 10.568572] [c04c0a10] (clk_core_enable) from [c04c09f0]
(clk_core_enable+0x40/0x9c)
[ 10.568572] ---[ end trace cb88537fdc8fa212 ]---
...
Regards,
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo
Re: Clock Regression in next-20150130 caused by cb75a8fcd14e
* Mike Turquette mturque...@linaro.org [150201 15:29]: Quoting Tony Lindgren (2015-01-30 17:04:44) Hi all, Looks like commit cb75a8fcd14e (clk: Add rate constraints to clocks) causes a regression on at least omaps where the serial console either does not show anything, or just prints garbage. Reverting cb75a8fcd14e makes things work again on next-20150130. Any ideas? Stephen posted a patch[0] to fix this. I've squashed that into Tomeu's commit that you reference above and my Panda board is booting fine once again. [0] http://lkml.kernel.org/r/20150131013158.ga4...@codeaurora.org Please let me know if any other issues pop up. There are new WARNs for OMAP3+ boards introduced by a different patch from Tomeu, but this is really because the OMAP DPLL and FAPLL code are dereferencing struct clk pointers when they should not be and is a separate issue from the constraints patch (with a separate email thread). Seems Linux next is broken again. Now we omaps get tons of: clock: dpll_abe_ck failed transition to 'locked' WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925 clk_disable+0x28/0x34() and WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436 omap3_noncore_dpll_enable+0xdc/0x10c() Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] ARM: dts: Add minimal support for dm8168-evm
* Matthijs van Duin matthijsvand...@gmail.com [150131 17:54]:
I just noticed the dm816x.dtsi says:
ocp {
compatible = ti,omap3-l3-smx, simple-bus;
This is incorrect: the DM81xx (and siblings like the AM335x) use
Arteris FlexNOC for the L3 interconnect, same as omap4/5 and vayu, not
SonicsMX. (In general everything on the DM81xx seems to be
omap4-generation)
Good catch, yes it seems it's mostly omap4. Looks like we
should also add it for am33xx.dtsi while at it.
I'll also check the devices one more time if I have some as
compatible with omap3 instead of omap4.
Regards,
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static
* Roger Quadros rog...@ti.com [150126 01:23]: On 24/01/15 22:28, Semen Protsenko wrote: Fix sparse warning: warning: symbol 'gpmc_cs_get_name' was not declared. Should it be static? Signed-off-by: Semen Protsenko semen.protse...@globallogic.com Acked-by: Roger Quadros rog...@ti.com Roger is going to queue this, so: Acked-by: Tony Lindgren t...@atomide.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 4/6] clk: Add rate constraints to clocks
* Tony Lindgren t...@atomide.com [150202 11:28]: * Mike Turquette mturque...@linaro.org [150202 09:50]: Quoting Tony Lindgren (2015-02-02 08:12:37) [ 10.568206] [ cut here ] [ 10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925 clk_disable+0x28/0x34() [ 10.568237] Modules linked in: [ 10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW 3.19.0-rc6-next-20150202 #2037 [ 10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree) [ 10.568267] [c0015bdc] (unwind_backtrace) from [c001222c] (show_stack+0x10/0x14) [ 10.568267] [c001222c] (show_stack) from [c05d2014] (dump_stack+0x84/0x9c) [ 10.568267] [c05d2014] (dump_stack) from [c003ea90] (warn_slowpath_common+0x7c/0xb8) [ 10.568298] [c003ea90] (warn_slowpath_common) from [c003eb68] (warn_slowpath_null+0x1c/0x24) [ 10.568298] [c003eb68] (warn_slowpath_null) from [c04c1ffc] (clk_disable+0x28/0x34) [ 10.568328] [c04c1ffc] (clk_disable) from [c0025b3c] (_disable_clocks+0x18/0x68) [ 10.568328] [c0025b3c] (_disable_clocks) from [c0026f14] (_idle+0x10c/0x214) [ 10.568328] [c0026f14] (_idle) from [c0855fac] (_setup+0x338/0x410) [ 10.568359] [c0855fac] (_setup) from [c0027360] (omap_hwmod_for_each+0x34/0x60) [ 10.568359] [c0027360] (omap_hwmod_for_each) from [c08563c4] (__omap_hwmod_setup_all+0x30/0x40) [ 10.568389] [c08563c4] (__omap_hwmod_setup_all) from [c0008a04] (do_one_initcall+0x80/0x1dc) [ 10.568389] [c0008a04] (do_one_initcall) from [c0848ea0] (kernel_init_freeable+0x204/0x2d0) [ 10.568420] [c0848ea0] (kernel_init_freeable) from [c05cdab8] (kernel_init+0x8/0xec) [ 10.568420] [c05cdab8] (kernel_init) from [c000e790] (ret_from_fork+0x14/0x24) [ 10.568420] ---[ end trace cb88537fdc8fa211 ]--- For reference, the above is line 992 in clk-next. ... Just booting 4430-sdp with omap2plus_defconfig. And git bisect points to 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances) as you already guessed. [ 10.568450] [ cut here ] [ 10.568450] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436 omap3_noncore_dpll_enable+0xdc/0 x10c() [ 10.568450] Modules linked in: [ 10.568481] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW 3.19.0-rc6-next-20150202 #2037 [ 10.568481] Hardware name: Generic OMAP4 (Flattened Device Tree) [ 10.568481] [c0015bdc] (unwind_backtrace) from [c001222c] (show_stack+0x10/0x14) [ 10.568511] [c001222c] (show_stack) from [c05d2014] (dump_stack+0x84/0x9c) [ 10.568511] [c05d2014] (dump_stack) from [c003ea90] (warn_slowpath_common+0x7c/0xb8) [ 10.568511] [c003ea90] (warn_slowpath_common) from [c003eb68] (warn_slowpath_null+0x1c/0x24) [ 10.568542] [c003eb68] (warn_slowpath_null) from [c0035800] (omap3_noncore_dpll_enable+0xdc/0x10c) [ 10.568542] [c0035800] (omap3_noncore_dpll_enable) from [c04c0a10] (clk_core_enable+0x60/0x9c) [ 10.568572] [c04c0a10] (clk_core_enable) from [c04c09f0] (clk_core_enable+0x40/0x9c) [ 10.568572] ---[ end trace cb88537fdc8fa212 ]--- ... This is the same issue discussed already in this thread[0]. Feedback from Tero Paul on how to handle it would be nice. Yes that one is a separate issue. Please let me know if anything else breaks for you. Also off-idle on omap3 seems to be broken by commit 59cf3fcf9baf. Actually the two warnigns above are all caused by the same issue. And the patch Tero just posted fixes both of the issue. It's the thread [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances for reference. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Mike Turquette mturque...@linaro.org [150201 13:27]:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but
by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
Also on at least omap4 like I posted. So sounds like the check for
WARN is wrong but harmless. Paul Tero, what do you want to do
about that?
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
dd-clk_ref = of_clk_get(node, 0);
dd-clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
Not seeing these warnings with dm186x as fapll.c does not use
dpll3xxx.c. This is because of the way the PLL's child synthesizers
need to also access the PLL registers for power and bypass mode.
Not related to the $subject bug, but to me it seems that we could
possibly have Linux generic PLL code if we add support for
parent_in_bypass_mode in addition to the parent_rate. This is because
the PLL can in theory generate the same rate both in bypass mode and
regular mode so parent_rate is not enough to tell it to the child
synthesizers. Not sure how the PLL registers enabling and disabling
it's children should be handled, maybe regmap would work there.
Regards,
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf
* Roger Quadros rog...@ti.com [150126 01:38]: On 24/01/15 22:28, Semen Protsenko wrote: Some GPMC_CONFIG7 register bits marked as RESERVED, means they shouldn't be overwritten. A typical approach to handle such bits called Read-Modify-Write. Writing procedure used in gpmc_cs_set_memconf() utilizes RMW technique, but implemented incorrectly. Due to obvious typo in code read register value is being rewritten by another value, which leads to loss of read RESERVED bits. This patch fixes this. While at it, replace magic numbers with named constants to improve code readability. Signed-off-by: Semen Protsenko semen.protse...@globallogic.com This is much nicer. Acked-by: Roger Quadros rog...@ti.com Roger will queue this so: Acked-by: Tony Lindgren t...@atomide.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 4/6] clk: Add rate constraints to clocks
Quoting Tony Lindgren (2015-02-02 08:12:37)
* Geert Uytterhoeven ge...@linux-m68k.org [150202 00:03]:
On Sun, Feb 1, 2015 at 11:18 PM, Mike Turquette mturque...@linaro.org
wrote:
Quoting Tomeu Vizoso (2015-01-31 10:36:22)
On 31 January 2015 at 02:31, Stephen Boyd sb...@codeaurora.org wrote:
On 01/29, Stephen Boyd wrote:
On 01/29/15 05:31, Geert Uytterhoeven wrote:
Hi Tomeu, Mike,
On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
tomeu.viz...@collabora.com wrote:
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
return 1;
}
-static void clk_core_put(struct clk_core *core)
+void __clk_put(struct clk *clk)
{
struct module *owner;
- owner = core-owner;
+ if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+ return;
clk_prepare_lock();
- kref_put(core-ref, __clk_release);
+
+ hlist_del(clk-child_node);
+ clk_core_set_rate_nolock(clk-core, clk-core-req_rate);
At this point, clk-core-req_rate is still zero, causing
cpg_div6_clock_round_rate() to be called with a zero rate
parameter,
e.g. on r8a7791:
Hmm.. I wonder if we should assign core-req_rate to be the same as
core-rate during __clk_init()? That would make this call to
clk_core_set_rate_nolock() a nop in this case.
Here's a patch to do this
---8
From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] clk: Assign a requested rate by default
We need to assign a requested rate here so that we avoid
requesting a rate of 0 on clocks when we remove clock consumers.
Hi, this looks good to me.
Reviewed-by: Tomeu Vizoso tomeu.viz...@collabora.com
It seems to fix the total boot failure on OMAPs, and hopefully does the
same for SH Mobile and others. I've squashed this into Tomeu's rate
constraints patch to maintain bisect.
Yes, it fixes shmobile. .round_rate() is now called with a sane value of
rate.
Looks like next-20150202 now produces tons of the following errors,
these from omap4:
next-20150202 is the rolled-back changes from last Friday. I removed the
clock constraints patch and in doing so also rolled back the TI clock
driver migration and clk-private.h removal patches. Those are all back
in clk-next as of last night and it looks as though they missed being
pulled into todays linux-next by a matter of minutes :-/
[ 10.568206] [ cut here ]
[ 10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925
clk_disable+0x28/0x34()
[ 10.568237] Modules linked in:
[ 10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW
3.19.0-rc6-next-20150202 #2037
[ 10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree)
[ 10.568267] [c0015bdc] (unwind_backtrace) from [c001222c]
(show_stack+0x10/0x14)
[ 10.568267] [c001222c] (show_stack) from [c05d2014]
(dump_stack+0x84/0x9c)
[ 10.568267] [c05d2014] (dump_stack) from [c003ea90]
(warn_slowpath_common+0x7c/0xb8)
[ 10.568298] [c003ea90] (warn_slowpath_common) from [c003eb68]
(warn_slowpath_null+0x1c/0x24)
[ 10.568298] [c003eb68] (warn_slowpath_null) from [c04c1ffc]
(clk_disable+0x28/0x34)
[ 10.568328] [c04c1ffc] (clk_disable) from [c0025b3c]
(_disable_clocks+0x18/0x68)
[ 10.568328] [c0025b3c] (_disable_clocks) from [c0026f14]
(_idle+0x10c/0x214)
[ 10.568328] [c0026f14] (_idle) from [c0855fac] (_setup+0x338/0x410)
[ 10.568359] [c0855fac] (_setup) from [c0027360]
(omap_hwmod_for_each+0x34/0x60)
[ 10.568359] [c0027360] (omap_hwmod_for_each) from [c08563c4]
(__omap_hwmod_setup_all+0x30/0x40)
[ 10.568389] [c08563c4] (__omap_hwmod_setup_all) from [c0008a04]
(do_one_initcall+0x80/0x1dc)
[ 10.568389] [c0008a04] (do_one_initcall) from [c0848ea0]
(kernel_init_freeable+0x204/0x2d0)
[ 10.568420] [c0848ea0] (kernel_init_freeable) from [c05cdab8]
(kernel_init+0x8/0xec)
[ 10.568420] [c05cdab8] (kernel_init) from [c000e790]
(ret_from_fork+0x14/0x24)
[ 10.568420] ---[ end trace cb88537fdc8fa211 ]---
This looks like mis-matched enable/disable calls. We now have unique
struct clk pointers for every call to clk_get. I haven't yet looked
through the hwmod code but I have a feeling that we're doing something
like this:
/* enable clock */
my_clk = clk_get(...);
clk_prepare_enable(my_clk);
clk_put(my_clk);
/* do some work */
do_work();
/* disable clock */
my_clk = clk_get(...);
clk_disable_unprepare(my_clk);
clk_put(my_clk);
The above pattern no longer works since my_clk will be two different
unique pointers, but it really should be one stable pointer across the
whole usage of the clk. E.g:
/* enable clock */
my_clk = clk_get(...);
clk_prepare_enable(my_clk
Re: [PATCH 4/4] ARM: dts: Add minimal support for dm8168-evm
* Matthijs van Duin matthijsvand...@gmail.com [150128 13:46]: On 26 January 2015 at 16:58, Tony Lindgren t...@atomide.com wrote: See earlier I was assuming copy paste issues from dm814x to dm816x Ahh, you thought the 816x was 814x-derived... yes I can imagine that will have led to some confusion. I'm pretty sure I verified the that the audio_pll_clk1 is hardwwired to 32KiHz by looking at it with a scope on the clkout pin. Yeah it comes straight from the rtcdivider so it'll produce 0.0016384 * devosc. Not much about rtcdivider in the dm816x TRM.. The global clock structure overview in the TRM (figure 2-6) actually correctly marks the former-audio-fapll clocks (except audio5 := osc0, yielding sysclk22 after divider) but you already need to know what the hell is going on to recognize that. That seems to be in the dm814x TRM. OK so it looks like in mainline kernel drivers/clk/ti/fapll.c we can then fix the hardcoded value in ti_fapll_synth_recalc_rate() to return the rate of fd-clk_ref multiplied 0.0016384 instead of 32768. And we probably should add what you're explaining to the comments also to make some sense out of it :) Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 4/6] clk: Add rate constraints to clocks
On Mon, Feb 02, 2015 at 09:46:46AM -0800, Mike Turquette wrote: This looks like mis-matched enable/disable calls. We now have unique struct clk pointers for every call to clk_get. I haven't yet looked through the hwmod code but I have a feeling that we're doing something like this: /* enable clock */ my_clk = clk_get(...); clk_prepare_enable(my_clk); clk_put(my_clk); /* do some work */ do_work(); /* disable clock */ my_clk = clk_get(...); clk_disable_unprepare(my_clk); clk_put(my_clk); The above pattern no longer works since my_clk will be two different unique pointers, but it really should be one stable pointer across the whole usage of the clk. E.g: Yes, it has always been documented that shall be the case. Anyone doing the above is basically broken. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 4/6] clk: Add rate constraints to clocks
* Mike Turquette mturque...@linaro.org [150202 09:50]: Quoting Tony Lindgren (2015-02-02 08:12:37) [ 10.568206] [ cut here ] [ 10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925 clk_disable+0x28/0x34() [ 10.568237] Modules linked in: [ 10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW 3.19.0-rc6-next-20150202 #2037 [ 10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree) [ 10.568267] [c0015bdc] (unwind_backtrace) from [c001222c] (show_stack+0x10/0x14) [ 10.568267] [c001222c] (show_stack) from [c05d2014] (dump_stack+0x84/0x9c) [ 10.568267] [c05d2014] (dump_stack) from [c003ea90] (warn_slowpath_common+0x7c/0xb8) [ 10.568298] [c003ea90] (warn_slowpath_common) from [c003eb68] (warn_slowpath_null+0x1c/0x24) [ 10.568298] [c003eb68] (warn_slowpath_null) from [c04c1ffc] (clk_disable+0x28/0x34) [ 10.568328] [c04c1ffc] (clk_disable) from [c0025b3c] (_disable_clocks+0x18/0x68) [ 10.568328] [c0025b3c] (_disable_clocks) from [c0026f14] (_idle+0x10c/0x214) [ 10.568328] [c0026f14] (_idle) from [c0855fac] (_setup+0x338/0x410) [ 10.568359] [c0855fac] (_setup) from [c0027360] (omap_hwmod_for_each+0x34/0x60) [ 10.568359] [c0027360] (omap_hwmod_for_each) from [c08563c4] (__omap_hwmod_setup_all+0x30/0x40) [ 10.568389] [c08563c4] (__omap_hwmod_setup_all) from [c0008a04] (do_one_initcall+0x80/0x1dc) [ 10.568389] [c0008a04] (do_one_initcall) from [c0848ea0] (kernel_init_freeable+0x204/0x2d0) [ 10.568420] [c0848ea0] (kernel_init_freeable) from [c05cdab8] (kernel_init+0x8/0xec) [ 10.568420] [c05cdab8] (kernel_init) from [c000e790] (ret_from_fork+0x14/0x24) [ 10.568420] ---[ end trace cb88537fdc8fa211 ]--- For reference, the above is line 992 in clk-next. This looks like mis-matched enable/disable calls. We now have unique struct clk pointers for every call to clk_get. I haven't yet looked through the hwmod code but I have a feeling that we're doing something like this: /* enable clock */ my_clk = clk_get(...); clk_prepare_enable(my_clk); clk_put(my_clk); /* do some work */ do_work(); /* disable clock */ my_clk = clk_get(...); clk_disable_unprepare(my_clk); clk_put(my_clk); The above pattern no longer works since my_clk will be two different unique pointers, but it really should be one stable pointer across the whole usage of the clk. E.g: /* enable clock */ my_clk = clk_get(...); clk_prepare_enable(my_clk); /* do some work */ do_work(); /* disable clock */ clk_disable_unprepare(my_clk); clk_put(my_clk); Again, I haven't looked through the code, so the above is just an educated guess. Anyways I am testing with an OMAP4460 Panda ES and I didn't see the above. Is there a test you are running to get this? Just booting 4430-sdp with omap2plus_defconfig. And git bisect points to 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances) as you already guessed. [ 10.568450] [ cut here ] [ 10.568450] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436 omap3_noncore_dpll_enable+0xdc/0 x10c() [ 10.568450] Modules linked in: [ 10.568481] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW 3.19.0-rc6-next-20150202 #2037 [ 10.568481] Hardware name: Generic OMAP4 (Flattened Device Tree) [ 10.568481] [c0015bdc] (unwind_backtrace) from [c001222c] (show_stack+0x10/0x14) [ 10.568511] [c001222c] (show_stack) from [c05d2014] (dump_stack+0x84/0x9c) [ 10.568511] [c05d2014] (dump_stack) from [c003ea90] (warn_slowpath_common+0x7c/0xb8) [ 10.568511] [c003ea90] (warn_slowpath_common) from [c003eb68] (warn_slowpath_null+0x1c/0x24) [ 10.568542] [c003eb68] (warn_slowpath_null) from [c0035800] (omap3_noncore_dpll_enable+0xdc/0x10c) [ 10.568542] [c0035800] (omap3_noncore_dpll_enable) from [c04c0a10] (clk_core_enable+0x60/0x9c) [ 10.568572] [c04c0a10] (clk_core_enable) from [c04c09f0] (clk_core_enable+0x40/0x9c) [ 10.568572] ---[ end trace cb88537fdc8fa212 ]--- ... This is the same issue discussed already in this thread[0]. Feedback from Tero Paul on how to handle it would be nice. Yes that one is a separate issue. Please let me know if anything else breaks for you. Also off-idle on omap3 seems to be broken by commit 59cf3fcf9baf. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their clock
registration had to be updated to take into account that the clock information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
-Tero
Author: Tero Kristo t-kri...@ti.com
Date: Mon Feb 2 17:19:17 2015 +0200
ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining
runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Signed-off-by: Tero Kristo t-kri...@ti.com
Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk
instances)
diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index c2da2a0..49752d7 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk-dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw-clk);
+ parent = __clk_get_hw(__clk_get_parent(hw-clk));
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
- WARN_ON(parent != dd-clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd-clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw,
unsigned long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw-clk) != dd-clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw-clk))
Re: [PATCH v13 4/6] clk: Add rate constraints to clocks
On Sun, Feb 1, 2015 at 11:18 PM, Mike Turquette mturque...@linaro.org wrote:
Quoting Tomeu Vizoso (2015-01-31 10:36:22)
On 31 January 2015 at 02:31, Stephen Boyd sb...@codeaurora.org wrote:
On 01/29, Stephen Boyd wrote:
On 01/29/15 05:31, Geert Uytterhoeven wrote:
Hi Tomeu, Mike,
On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
tomeu.viz...@collabora.com wrote:
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
return 1;
}
-static void clk_core_put(struct clk_core *core)
+void __clk_put(struct clk *clk)
{
struct module *owner;
- owner = core-owner;
+ if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+ return;
clk_prepare_lock();
- kref_put(core-ref, __clk_release);
+
+ hlist_del(clk-child_node);
+ clk_core_set_rate_nolock(clk-core, clk-core-req_rate);
At this point, clk-core-req_rate is still zero, causing
cpg_div6_clock_round_rate() to be called with a zero rate parameter,
e.g. on r8a7791:
Hmm.. I wonder if we should assign core-req_rate to be the same as
core-rate during __clk_init()? That would make this call to
clk_core_set_rate_nolock() a nop in this case.
Here's a patch to do this
---8
From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] clk: Assign a requested rate by default
We need to assign a requested rate here so that we avoid
requesting a rate of 0 on clocks when we remove clock consumers.
Hi, this looks good to me.
Reviewed-by: Tomeu Vizoso tomeu.viz...@collabora.com
It seems to fix the total boot failure on OMAPs, and hopefully does the
same for SH Mobile and others. I've squashed this into Tomeu's rate
constraints patch to maintain bisect.
Yes, it fixes shmobile. .round_rate() is now called with a sane value of rate.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/7] usb: extcon: Fix USB-Host cable name
Chanwoo, On 02/02/15 07:04, Chanwoo Choi wrote: Hi Roger, On 01/30/2015 11:05 PM, Roger Quadros wrote: Hi, On 30/01/15 13:04, Roger Quadros wrote: Felipe Chanwoo, On 26/01/15 14:15, Roger Quadros wrote: The recommended name for USB-Host cable state is USB-Host and not USB-HOST as per drivers/extcon/extcon-class.c extcon_cable_name. Change all instances of USB-HOST to USB-Host. Signed-off-by: Roger Quadros rog...@ti.com Reviewed-by: Felipe Balbi ba...@ti.com Acked-by: Felipe Balbi ba...@ti.com This patch has no dependency to the rest so can be picked up as soon as possible. Do you think it is better to go via the USB tree? If yes then Chanwoo, can you please Ack this one? Thanks. This would mean that only the first patch needs to go through extcon tree as Tony will pick the rest. Hold on. Let's first decide what we really want to go ahead with USB-Host or USB-HOST. Currently, extcon driver have used the specific cable name(USB-Host or USB-HOST) without any standard way. So, I have plan to define common cable name in extcon header file by using capital letter. OK. In that case, this patch is not required. I will resend patch 1 with cable name corrected to USB-HOST. cheers, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
