| Message ID | 1393451575-23758-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 26, 2014 at 10:52:55PM +0100, Laurent Pinchart wrote: > Briefly documentation the common clock framework locking scheme from a > clock driver point of view. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > Documentation/clk.txt | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/Documentation/clk.txt b/Documentation/clk.txt > index 699ef2a..4bd6fd7 100644 > --- a/Documentation/clk.txt > +++ b/Documentation/clk.txt > @@ -255,3 +255,29 @@ are sorted out. > > To bypass this disabling, include "clk_ignore_unused" in the bootargs to the > kernel. > + > + Part 7 - Locking > + > +The common clock framework uses two global locks. One of them (the enable > +lock) is held across calls to the .enable, .disable and .is_enabled > +operations, while the other (the prepare lock) is held across calls to all other > +operations. This effectively divides operations in two groups from a locking > +perspective. It would be a good idea to mention the types of these locks - the fact that the enable lock is a spinlock, and the prepare lock is a mutex. That's a very important distinction as there's limitations on the contexts that the prepare-lock can be called from.
Quoting Russell King - ARM Linux (2014-02-26 14:29:26) > On Wed, Feb 26, 2014 at 10:52:55PM +0100, Laurent Pinchart wrote: > > Briefly documentation the common clock framework locking scheme from a > > clock driver point of view. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > Documentation/clk.txt | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/Documentation/clk.txt b/Documentation/clk.txt > > index 699ef2a..4bd6fd7 100644 > > --- a/Documentation/clk.txt > > +++ b/Documentation/clk.txt > > @@ -255,3 +255,29 @@ are sorted out. > > > > To bypass this disabling, include "clk_ignore_unused" in the bootargs to the > > kernel. > > + > > + Part 7 - Locking > > + > > +The common clock framework uses two global locks. One of them (the enable > > +lock) is held across calls to the .enable, .disable and .is_enabled > > +operations, while the other (the prepare lock) is held across calls to all other > > +operations. This effectively divides operations in two groups from a locking > > +perspective. > > It would be a good idea to mention the types of these locks - the fact > that the enable lock is a spinlock, and the prepare lock is a mutex. > That's a very important distinction as there's limitations on the > contexts that the prepare-lock can be called from. I agree with Russell and would take it a step further: this documentation on locking should reiterate the relationship between clk_prepare and clk_enable, namely that clk_prepare MUST be called and MUST complete before calling clk_enable. This contract helps us deal with the difference in lock types and keep things sane despite the lack of lock synchronization. Regards, Mike > > -- > FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly > improving, and getting towards what was expected from it.
Hi Mike and Russell, On Wednesday 26 February 2014 15:30:53 Mike Turquette wrote: > Quoting Russell King - ARM Linux (2014-02-26 14:29:26) > > On Wed, Feb 26, 2014 at 10:52:55PM +0100, Laurent Pinchart wrote: > > > Briefly documentation the common clock framework locking scheme from a > > > clock driver point of view. > > > > > > Signed-off-by: Laurent Pinchart > > > <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > > > > Documentation/clk.txt | 26 ++++++++++++++++++++++++++ > > > 1 file changed, 26 insertions(+) > > > > > > diff --git a/Documentation/clk.txt b/Documentation/clk.txt > > > index 699ef2a..4bd6fd7 100644 > > > --- a/Documentation/clk.txt > > > +++ b/Documentation/clk.txt > > > @@ -255,3 +255,29 @@ are sorted out. > > > > > > To bypass this disabling, include "clk_ignore_unused" in the bootargs > > > to the kernel. > > > > > > + > > > + Part 7 - Locking > > > + > > > +The common clock framework uses two global locks. One of them (the > > > enable > > > +lock) is held across calls to the .enable, .disable and .is_enabled > > > +operations, while the other (the prepare lock) is held across calls to > > > all other +operations. This effectively divides operations in two > > > groups from a locking +perspective. > > > > It would be a good idea to mention the types of these locks - the fact > > that the enable lock is a spinlock, and the prepare lock is a mutex. > > That's a very important distinction as there's limitations on the > > contexts that the prepare-lock can be called from. > > I agree with Russell and would take it a step further: this > documentation on locking should reiterate the relationship between > clk_prepare and clk_enable, namely that clk_prepare MUST be called and > MUST complete before calling clk_enable. > > This contract helps us deal with the difference in lock types and keep > things sane despite the lack of lock synchronization. If I'm not mistaken that document explains CCF from a driver point of view, not from a user point of view. The clk_enable() and clk_prepare() functions are only mentioned in one example to describe the internal call stack. While I totally agree that the enable/prepare relationship and the spinlock/mutex pair is a very important part of CCF, I believe they would rather belong to a CCF user API documentation. What's your opinion on this ?
On Fri, Feb 28, 2014 at 12:07:55AM +0100, Laurent Pinchart wrote: > Hi Mike and Russell, > > If I'm not mistaken that document explains CCF from a driver point of view, > not from a user point of view. The clk_enable() and clk_prepare() functions > are only mentioned in one example to describe the internal call stack. > While I totally agree that the enable/prepare relationship and the > spinlock/mutex pair is a very important part of CCF, I believe they would > rather belong to a CCF user API documentation. The type of lock is relevant on both sides of the API. Users need to know that the prepare/unprepare may sleep during those calls, and implementations need to know that they can sleep during those calls. Conversely, with enable/disable, users can see they will never sleep (because sleeping with a spinlock held is illegal) and implementations need to know that too. So, I think there's value in adding that information here.
diff --git a/Documentation/clk.txt b/Documentation/clk.txt index 699ef2a..4bd6fd7 100644 --- a/Documentation/clk.txt +++ b/Documentation/clk.txt @@ -255,3 +255,29 @@ are sorted out. To bypass this disabling, include "clk_ignore_unused" in the bootargs to the kernel. + + Part 7 - Locking + +The common clock framework uses two global locks. One of them (the enable +lock) is held across calls to the .enable, .disable and .is_enabled +operations, while the other (the prepare lock) is held across calls to all other +operations. This effectively divides operations in two groups from a locking +perspective. + +Drivers don't need to manually protect resources shared between the operations +of one group, regardless of whether those resources are shared by multiple +clocks or not. However, access to resources that are shared between operations +of the two groups needs to be protected by the drivers. An example of such a +resource would be a register that controls both the clock rate and the clock +enable/disable state. + +The clock framework is reentrant, in that a driver is allowed to call clock +framework functions from within its implementation of clock operations. This +can for instance cause a .set_rate operation of one clock being called from +within the .set_rate operation of another clock. This case must be considered +in the driver implementations, but the code flow is usually controlled by the +driver in that case. + +Note that locking must also be considered when code outside of the common +clock framework needs to access resources used by the clock operations. This +is considered out of scope of this document.
Briefly documentation the common clock framework locking scheme from a clock driver point of view. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- Documentation/clk.txt | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)