[PATCH] Documentation: clk: Add locking documentation

[PATCH] Documentation: clk: Add locking documentation

[Laurent Pinchart]

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.
+
+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.
-- 
Regards,

Laurent Pinchart

[PATCH] Documentation: clk: Add locking documentation

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH] Documentation: clk: Add locking documentation

[Mike Turquette]

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.

[PATCH] Documentation: clk: Add locking documentation

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

[PATCH] Documentation: clk: Add locking documentation

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.