Re: [resend] Timer broadcast question
On Tue, 19 Feb 2013, Andy Lutomirski wrote: On 02/19/2013 10:21 AM, Daniel Lezcano wrote: On 02/19/2013 07:10 PM, Thomas Gleixner wrote: On Tue, 19 Feb 2013, Daniel Lezcano wrote: I am working on identifying the different wakeup sources from the interrupts and I have a question regarding the timer broadcast. The broadcast timer is setup to the next event and that will wake up any idle cpu belonging to the broadcast cpumask, right ? The cpu which has been woken up will look for each cpu the next-event and send an IPI to wake it up. Although, it is possible the sender of this IPI may not be concerned by the timer expiration and has been woken up just for sending the IPI, right ? Correct. If this is correct, is it possible to setup the timer irq affinity to a cpu which will be concerned by the timer expiration ? so we prevent an unnecessary wake up for a cpu. It is possible, but we never implemented it. If we go there, we want to make that conditional on a property flag, because some interrupt controllers especially on x86 only allow to move the affinity from interrupt context, which is pointless. Thanks Thomas for your quick answer. I will write a RFC patchset. I'm curious what the use case is. I played with this code awhile ago, and AFAICT it's not used on sensible (i.e. modern) systems. Is there anything other than old x86 machines that needs it? If the local apic timer is not affected by C-States, it's irrelevant, but there are enough machines out there which do not have that. The point is that we want a flag on the broadcast device which tells us whether we should use dynamic affinity settings or not. On x86 we would not set that flag ever. Thanks, tglx ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On Fri, 16 Mar 2012, Linus Walleij wrote: On Fri, Mar 16, 2012 at 7:11 AM, Mike Turquette mturque...@linaro.org wrote: Provide documentation for the common clk structures and APIs. This code can be found in drivers/clk/ and include/linux/clk*.h. Acked-by: Linus Wallej linus.wall...@linaro.org For this three-piece v7 patchset. It already does magnitudes more advanced stuff than what I need so I'm pretty pleased, any remaining details can surely be ironed out in-tree. Ack. We really need to get that in now and sort out the details in tree otherwise we will never finish that thing. Thanks, tglx___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Wed, 14 Mar 2012, Turquette, Mike wrote: Could you folks please trim your replies? It's annoying to page down a gazillion of lines to find the gist. On Wed, Mar 14, 2012 at 1:48 AM, Sascha Hauer s.ha...@pengutronix.de wrote: Also, do you forsee needing hole in parent_names for any reason other than described above? I need it only for the case where a some values in the mux are marked as reserved in the datasheet or we simply do not have the corresponding clock in our tree (yet). We could also say that NULL pointers are not allowed in parent arrays, but instead orphan or dummy should be used. Then __clk_init should check for NULL pointers to make this clear. I've added a WARN in __clk_init if any entries in .parent_names are NULL. I think it best to populate it with dummy, or maybe a platform-specific name if it helps you during development. There is no guarantee that the selection of a parent can be mapped linear to register values. So the right way to deal with it is to have an array of valid names with no holes and NULL pointers allowed and have a mapping from the array index to the register value. That makes the core code robust and allows to handle all corner cases including reserved bits, not implemented clocks and weird register layouts. Thanks, tglx ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Wed, 14 Mar 2012, Turquette, Mike wrote: On Wed, Mar 14, 2012 at 2:28 PM, Thomas Gleixner t...@linutronix.de wrote: So the right way to deal with it is to have an array of valid names with no holes and NULL pointers allowed and have a mapping from the array index to the register value. This is essentially what the .set_rate callback does. It takes as input u8 index and peforms the hardware specific magic to select the correct parent clock. This might be a register write using that exact same index, or it might be a single-bit register write using that index as the shift value, or it might translate that index into the data sent to an i2c device (where the address would be stored in struct clk_foo), etc etc. We both agree that .parent_names must contain valid names and should not have holes. What I don't understand is if you are saying that we should allow NULL ptrs as names; that seems contradictory but I want to make sure I'm reading you correctly. I should have said: no holes and no NULL pointers, just an array of valid names. Thanks, tglx ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Fri, 9 Mar 2012, Mike Turquette wrote:
+inline unsigned long __clk_get_enable_count(struct clk *clk)
+{
+ return !clk ? -EINVAL : clk-enable_count;
Returning negative error codes in a function with a return value
unsigned long is a bit strange at least. Shouldn't that be long ?
+#ifndef __LINUX_CLK_PRIVATE_H
+#define __LINUX_CLK_PRIVATE_H
+
+#include linux/clk-provider.h
+#include linux/list.h
+
+/*
+ * WARNING: Do not include clk-private.h from any file that implements struct
+ * clk_ops. Doing so is a layering violation!
+ *
+ * This header exists only to allow for statically initialized clock data.
Any
+ * static clock data must be defined in a separate file from the logic that
+ * implements the clock operations for that same data.
Now the question is whether you should provide a data structure which
is explicitely used for static initialization and instead of having
struct clk static you register the static initializer structure, which
would be initdata. I don't think that anything needs clocks before the
memory allocators are up and running. The clocks which are necessary
to get that far have to be enabled in the boot loader anyway.
The static initialization question should not hold off this set from
being merged, though settling it before growing users would be nice.
Otherwise this is a very well done infrastructure implementation!
Thanks a lot Mike!
Reviewed-by: Thomas Gleixner t...@linutronix.de
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/4] clk: introduce the common clock framework
On Wed, 7 Mar 2012, Turquette, Mike wrote: Assuming that some day OMAP code can be refactored to allow for lazy (or at least initcall-based) registration of clocks then perhaps your suggestion can take root. Which leads me to this question: are there any other platforms out there that require the level of expose to struct clk present in this patchset? OMAP does, for now, but if that changes then I need to know if others require this as well. I can't see the problem, really. Other than existing code doing stuff before the memory allocator is up and running. We allocate interrupt data structures in the early boot process today and I don't see a reason why you want clocks, which have not been configured by the boot loader, accesible before that point. Thanks, tglx ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: clock_getres() and real resolution
On Wed, 8 Feb 2012, Dmitry Antipov wrote:
IIUC, an idea behind clock_getres() is to give a hint about the resolution of
specified clock. This hint may be used by an application programmer to check
whether
this clock is suitable for a some purpose. So why clock_getres() always
returns
something like {0, 1} (if hrtimers are enabled) regardless of the underlying
platform's
real numbers?
For example, OMAP4's real resolution of CLOCK_REALTIME is 30.5us for 32K timer
and 26ns
for MPU timer. Such a difference definitely makes sense - but
clock_getres(CLOCK_REALTIME,..)
always returns {0, KTIME_HIGH_RES}. Since this behavior causes a confusion
like
http://lists.linaro.org/pipermail/linaro-dev/2012-February/010112.html, I'm
considering
this as a stupid misfeature.
We had this discussion before. The point is that the accuracy of the
internal kernel timer handling is 1nsec in case of high resolution
timers. The fact that the underlying clock event device has a coarser
resolution does not change that.
It would be possible to return the real resolution of the clock event
device, but we have systems, where the clockevent device is
dynamically changing. So which resolution do we expose to an
application? The one of the current active device or some magic number
of a device which might not even be initialized? That's more confusing
than telling user space that high resolution timers are active and the
kernel is trying to achieve the 1ns accuracy.
Thanks,
tglx
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 3/6] clk: introduce the common clock framework
On Tue, 13 Dec 2011, Mike Turquette wrote:
+void __clk_unprepare(struct clk *clk)
+{
+ if (!clk)
+ return;
+
+ if (WARN_ON(clk-prepare_count == 0))
+ return;
+
+ if (--clk-prepare_count 0)
+ return;
+
+ WARN_ON(clk-enable_count 0);
So this leaves the clock enable count set. I'm a bit wary about
that. Shouldn't it either return (including bumping the prepare_count
again) or call clk_disable() ?
+/**
+ * clk_get_parent - return the parent of a clk
+ * @clk: the clk whose parent gets returned
+ *
+ * Simply returns clk-parent. It is up to the caller to hold the
+ * prepare_lock, if desired. Returns NULL if clk is NULL.
Holding the prepare lock in the caller will deadlock. So it's not a
real good idea to document it as an option :)
+ */
+struct clk *clk_get_parent(struct clk *clk)
+{
+ struct clk *parent;
+
+ if (!clk)
+ return NULL;
+
+ mutex_lock(prepare_lock);
+/**
+ * clk_init - initialize the data structures in a struct clk
+ * @dev: device initializing this clk, placeholder for now
+ * @clk: clk being initialized
+ *
+ * Initializes the lists in struct clk, queries the hardware for the
+ * parent and rate and sets them both. Adds the clk to the sysfs tree
+ * topology.
+ *
+ * Caller must populate clk-name and clk-flags before calling
I'm not too happy about this construct. That leaves struct clk and its
members exposed to the world instead of making it a real opaque
cookie. I know from my own painful experience, that this will lead to
random fiddling in that data structure in drivers and arch code just
because the core code has a shortcoming.
Why can't we make struct clk a real cookie and confine the data
structure to the core code ?
That would change the init call to something like:
struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
struct clk *parent)
And have:
struct clk_hw {
struct clk_hw_ops *ops;
const char*name;
unsigned long flags;
};
Implementers can do:
struct my_clk_hw {
struct clk_hwhw;
mydata;
};
And then change the clk ops callbacks to take struct clk_hw * as an
argument.
So the core code can allocate the clk data structure and you return a
real opaque cookie. You do the same thing for the basic gated clock in
one of the next patches, so doing it for struct clk itself is just
consequent.
+ * clk_init. A key assumption in the call to .get_parent is that clks
+ * are being initialized in-order.
We should not require that, see below.
+ */
+void clk_init(struct device *dev, struct clk *clk)
+{
+ if (!clk)
+ return;
+
+ INIT_HLIST_HEAD(clk-children);
+ INIT_HLIST_NODE(clk-child_node);
+
+ mutex_lock(prepare_lock);
+
+ /*
+ * .get_parent is mandatory for populating .parent for clks with
+ * multiple possible parents. For clks with a fixed parent
+ * .get_parent is not mandatory and .parent can be statically
+ * initialized
+ */
+ if (clk-ops-get_parent)
+ clk-parent = clk-ops-get_parent(clk);
+
+ /* clks without a parent are considered root clks */
I'd rather prefer that root clocks are explicitely marked with a flag
instead of relying on clk-parent being NULL.
+ if (clk-parent)
+ hlist_add_head(clk-child_node,
+ clk-parent-children);
+ else
+ hlist_add_head(clk-child_node, clk_root_list);
You could put clocks which have no parent and are not marked as root
clock onto a separate list clk_orphaned or such. You might need this
for handling clocks which are disconnected from any parent runtime as
well.
And to avoid the whole in-order initialization issue, you could change
clk_init to:
struct clk *clk_init(struct device *dev, const struct clk_hw *hw,
unsigned long flags, const char *parent_name)
Then you can lookup the requested parent by name. If that fails, you
put the clock into the orphaned list. When a new clock is initialized,
then you walk the orphaned list and check whether something is waiting
for that clock.
To handle multi-parent clocks you need to go one step further and add:
struct clk_hw {
struct clk_hw_ops *ops;
const char*name;
const char**possible_parents;
};
You also require a get_parent_idx() function in clk_hw_ops. So when
clk_init() is called with parent_name = NULL and get_parent_idx() is
implemented, then you call it and the clock returns you the index of
the possible_parents array. If that parent clock is not yet
registered, you store the requested name and do the lookup when new
clocks are registered.
That has also the advantage, that you can validate parent settings
upfront and for setting the parent during runtime, you don't have to
acquire a pointer to the parent clock. It's enough to call
clk_set_named_parent().
