Re: [PATCH v4 09/10] clk: add clk_rate_exclusive api

[Michael Turquette]

Hi Jérôme,

On Tue, Oct 31, 2017 at 5:29 PM, Jerome Brunet  wrote:
> On Thu, 2017-10-26 at 07:26 +0200, Michael Turquette wrote:
>> Hi Jerome,
>>
>> Quoting Jerome Brunet (2017-09-24 22:00:29)
>> > @@ -1778,6 +1867,50 @@ int clk_set_rate(struct clk *clk, unsigned long 
>> > rate)
>> >  EXPORT_SYMBOL_GPL(clk_set_rate);
>> >
>> >  /**
>> > + * clk_set_rate_exclusive - specify a new rate get exclusive control
>> > + * @clk: the clk whose rate is being changed
>> > + * @rate: the new rate for clk
>> > + *
>> > + * This is a combination of clk_set_rate() and clk_rate_exclusive_get()
>> > + * within a critical section
>> > + *
>> > + * This can be used initially to ensure that at least 1 consumer is
>> > + * statisfied when several consumers are competing for exclusivity over 
>> > the
>> > + * same clock provider.
>>
>> Please add the following here:
>>
>>   Calls to clk_rate_exclusive_get() should be balanced with calls to
>>   clk_rate_exclusive_put().
>
> Oh indeed !
> I can do a resend with it or, if you prefer, you may directly amend the patch.
> As you prefer

Previously we agreed that these should go onto the next -rc1 so that
they have more soak time. Being a very lazy person, may I ask you to
rebase the patches on -rc1 when it comes out (with this small change)
and then I'll pull them? You can send a PR directly if you like.

Best regards,
Mike

>
> Thanks
>
>>
>> Otherwise looks good to me.
>>
>> Best regards,
>> Mike
>

Re: [PATCH v4 09/10] clk: add clk_rate_exclusive api

[Michael Turquette]

Hi Jérôme,

On Tue, Oct 31, 2017 at 5:29 PM, Jerome Brunet  wrote:
> On Thu, 2017-10-26 at 07:26 +0200, Michael Turquette wrote:
>> Hi Jerome,
>>
>> Quoting Jerome Brunet (2017-09-24 22:00:29)
>> > @@ -1778,6 +1867,50 @@ int clk_set_rate(struct clk *clk, unsigned long 
>> > rate)
>> >  EXPORT_SYMBOL_GPL(clk_set_rate);
>> >
>> >  /**
>> > + * clk_set_rate_exclusive - specify a new rate get exclusive control
>> > + * @clk: the clk whose rate is being changed
>> > + * @rate: the new rate for clk
>> > + *
>> > + * This is a combination of clk_set_rate() and clk_rate_exclusive_get()
>> > + * within a critical section
>> > + *
>> > + * This can be used initially to ensure that at least 1 consumer is
>> > + * statisfied when several consumers are competing for exclusivity over 
>> > the
>> > + * same clock provider.
>>
>> Please add the following here:
>>
>>   Calls to clk_rate_exclusive_get() should be balanced with calls to
>>   clk_rate_exclusive_put().
>
> Oh indeed !
> I can do a resend with it or, if you prefer, you may directly amend the patch.
> As you prefer

Previously we agreed that these should go onto the next -rc1 so that
they have more soak time. Being a very lazy person, may I ask you to
rebase the patches on -rc1 when it comes out (with this small change)
and then I'll pull them? You can send a PR directly if you like.

Best regards,
Mike

>
> Thanks
>
>>
>> Otherwise looks good to me.
>>
>> Best regards,
>> Mike
>

Re: [PATCH v4 09/10] clk: add clk_rate_exclusive api

[Jerome Brunet]

On Thu, 2017-10-26 at 07:26 +0200, Michael Turquette wrote:
> Hi Jerome,
> 
> Quoting Jerome Brunet (2017-09-24 22:00:29)
> > @@ -1778,6 +1867,50 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
> >  EXPORT_SYMBOL_GPL(clk_set_rate);
> >  
> >  /**
> > + * clk_set_rate_exclusive - specify a new rate get exclusive control
> > + * @clk: the clk whose rate is being changed
> > + * @rate: the new rate for clk
> > + *
> > + * This is a combination of clk_set_rate() and clk_rate_exclusive_get()
> > + * within a critical section
> > + *
> > + * This can be used initially to ensure that at least 1 consumer is
> > + * statisfied when several consumers are competing for exclusivity over the
> > + * same clock provider.
> 
> Please add the following here:
> 
>   Calls to clk_rate_exclusive_get() should be balanced with calls to
>   clk_rate_exclusive_put().

Oh indeed !
I can do a resend with it or, if you prefer, you may directly amend the patch.
As you prefer

Thanks

> 
> Otherwise looks good to me.
> 
> Best regards,
> Mike

Re: [PATCH v4 09/10] clk: add clk_rate_exclusive api

[Jerome Brunet]

On Thu, 2017-10-26 at 07:26 +0200, Michael Turquette wrote:
> Hi Jerome,
> 
> Quoting Jerome Brunet (2017-09-24 22:00:29)
> > @@ -1778,6 +1867,50 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
> >  EXPORT_SYMBOL_GPL(clk_set_rate);
> >  
> >  /**
> > + * clk_set_rate_exclusive - specify a new rate get exclusive control
> > + * @clk: the clk whose rate is being changed
> > + * @rate: the new rate for clk
> > + *
> > + * This is a combination of clk_set_rate() and clk_rate_exclusive_get()
> > + * within a critical section
> > + *
> > + * This can be used initially to ensure that at least 1 consumer is
> > + * statisfied when several consumers are competing for exclusivity over the
> > + * same clock provider.
> 
> Please add the following here:
> 
>   Calls to clk_rate_exclusive_get() should be balanced with calls to
>   clk_rate_exclusive_put().

Oh indeed !
I can do a resend with it or, if you prefer, you may directly amend the patch.
As you prefer

Thanks

> 
> Otherwise looks good to me.
> 
> Best regards,
> Mike

Re: [PATCH v4 09/10] clk: add clk_rate_exclusive api

[Michael Turquette]

Hi Jerome,

Quoting Jerome Brunet (2017-09-24 22:00:29)
> @@ -1778,6 +1867,50 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
>  EXPORT_SYMBOL_GPL(clk_set_rate);
>  
>  /**
> + * clk_set_rate_exclusive - specify a new rate get exclusive control
> + * @clk: the clk whose rate is being changed
> + * @rate: the new rate for clk
> + *
> + * This is a combination of clk_set_rate() and clk_rate_exclusive_get()
> + * within a critical section
> + *
> + * This can be used initially to ensure that at least 1 consumer is
> + * statisfied when several consumers are competing for exclusivity over the
> + * same clock provider.

Please add the following here:

  Calls to clk_rate_exclusive_get() should be balanced with calls to
  clk_rate_exclusive_put().

Otherwise looks good to me.

Best regards,
Mike

Re: [PATCH v4 09/10] clk: add clk_rate_exclusive api

[Michael Turquette]

Hi Jerome,

Quoting Jerome Brunet (2017-09-24 22:00:29)
> @@ -1778,6 +1867,50 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
>  EXPORT_SYMBOL_GPL(clk_set_rate);
>  
>  /**
> + * clk_set_rate_exclusive - specify a new rate get exclusive control
> + * @clk: the clk whose rate is being changed
> + * @rate: the new rate for clk
> + *
> + * This is a combination of clk_set_rate() and clk_rate_exclusive_get()
> + * within a critical section
> + *
> + * This can be used initially to ensure that at least 1 consumer is
> + * statisfied when several consumers are competing for exclusivity over the
> + * same clock provider.

Please add the following here:

  Calls to clk_rate_exclusive_get() should be balanced with calls to
  clk_rate_exclusive_put().

Otherwise looks good to me.

Best regards,
Mike

[PATCH v4 09/10] clk: add clk_rate_exclusive api

[Jerome Brunet]

Using clock rate protection, we can now provide a way for clock consumer
to claim exclusive control over the rate of a producer

So far, rate change operations have been a "last write wins" affair. This
changes allows drivers to explicitly protect against this behavior, if
required.

Of course, if exclusivity over a producer is claimed more than once, the
rate is effectively locked as exclusivity cannot be preempted

Signed-off-by: Jerome Brunet 
---
 drivers/clk/clk.c   | 167 
 include/linux/clk.h |  57 ++
 2 files changed, 224 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f990ef127a83..cbfff541ec8a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -85,6 +85,7 @@ struct clk {
const char *con_id;
unsigned long min_rate;
unsigned long max_rate;
+   unsigned int exclusive_count;
struct hlist_node clks_node;
 };
 
@@ -512,6 +513,45 @@ static int clk_core_rate_nuke_protect(struct clk_core 
*core)
return ret;
 }
 
+/**
+ * clk_rate_exclusive_put - release exclusivity over clock rate control
+ * @clk: the clk over which the exclusivity is released
+ *
+ * clk_rate_exclusive_put() completes a critical section during which a clock
+ * consumer cannot tolerate any other consumer making any operation on the
+ * clock which could result in a rate change or rate glitch. Exclusive clocks
+ * cannot have their rate changed, either directly or indirectly due to changes
+ * further up the parent chain of clocks. As a result, clocks up parent chain
+ * also get under exclusive control of the calling consumer.
+ *
+ * If exlusivity is claimed more than once on clock, even by the same consumer,
+ * the rate effectively gets locked as exclusivity can't be preempted.
+ *
+ * Calls to clk_rate_exclusive_put() must be balanced with calls to
+ * clk_rate_exclusive_get(). Calls to this function may sleep, and do not 
return
+ * error status.
+ */
+void clk_rate_exclusive_put(struct clk *clk)
+{
+   if (!clk)
+   return;
+
+   clk_prepare_lock();
+
+   /*
+* if there is something wrong with this consumer protect count, stop
+* here before messing with the provider
+*/
+   if (WARN_ON(clk->exclusive_count <= 0))
+   goto out;
+
+   clk_core_rate_unprotect(clk->core);
+   clk->exclusive_count--;
+out:
+   clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_exclusive_put);
+
 static void clk_core_rate_protect(struct clk_core *core)
 {
lockdep_assert_held(_lock);
@@ -539,6 +579,36 @@ static void clk_core_rate_restore_protect(struct clk_core 
*core, int count)
core->protect_count = count;
 }
 
+/**
+ * clk_rate_exclusive_get - get exclusivity over the clk rate control
+ * @clk: the clk over which the exclusity of rate control is requested
+ *
+ * clk_rate_exlusive_get() begins a critical section during which a clock
+ * consumer cannot tolerate any other consumer making any operation on the
+ * clock which could result in a rate change or rate glitch. Exclusive clocks
+ * cannot have their rate changed, either directly or indirectly due to changes
+ * further up the parent chain of clocks. As a result, clocks up parent chain
+ * also get under exclusive control of the calling consumer.
+ *
+ * If exlusivity is claimed more than once on clock, even by the same consumer,
+ * the rate effectively gets locked as exclusivity can't be preempted.
+ *
+ * Calls to clk_rate_exclusive_get() should be balanced with calls to
+ * clk_rate_exclusive_put(). Calls to this function may sleep, and do not
+ * return error status.
+ */
+void clk_rate_exclusive_get(struct clk *clk)
+{
+   if (!clk)
+   return;
+
+   clk_prepare_lock();
+   clk_core_rate_protect(clk->core);
+   clk->exclusive_count++;
+   clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
+
 static void clk_core_unprepare(struct clk_core *core)
 {
lockdep_assert_held(_lock);
@@ -929,6 +999,12 @@ static int clk_core_determine_round_nolock(struct clk_core 
*core,
if (!core)
return 0;
 
+   /*
+* At this point, core protection will be disabled if
+* - if the provider is not protected at all
+* - if the calling consumer is the only one which has exclusivity
+*   over the provider
+*/
if (clk_core_rate_is_protected(core)) {
req->rate = core->rate;
} else if (core->ops->determine_rate) {
@@ -1045,10 +1121,17 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 
clk_prepare_lock();
 
+   if (clk->exclusive_count)
+   clk_core_rate_unprotect(clk->core);
+
clk_core_get_boundaries(clk->core, _rate, _rate);
req.rate = rate;
 
ret = clk_core_round_rate_nolock(clk->core, );
+
+   if (clk->exclusive_count)
+   

[PATCH v4 09/10] clk: add clk_rate_exclusive api

[Jerome Brunet]

Using clock rate protection, we can now provide a way for clock consumer
to claim exclusive control over the rate of a producer

So far, rate change operations have been a "last write wins" affair. This
changes allows drivers to explicitly protect against this behavior, if
required.

Of course, if exclusivity over a producer is claimed more than once, the
rate is effectively locked as exclusivity cannot be preempted

Signed-off-by: Jerome Brunet 
---
 drivers/clk/clk.c   | 167 
 include/linux/clk.h |  57 ++
 2 files changed, 224 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f990ef127a83..cbfff541ec8a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -85,6 +85,7 @@ struct clk {
const char *con_id;
unsigned long min_rate;
unsigned long max_rate;
+   unsigned int exclusive_count;
struct hlist_node clks_node;
 };
 
@@ -512,6 +513,45 @@ static int clk_core_rate_nuke_protect(struct clk_core 
*core)
return ret;
 }
 
+/**
+ * clk_rate_exclusive_put - release exclusivity over clock rate control
+ * @clk: the clk over which the exclusivity is released
+ *
+ * clk_rate_exclusive_put() completes a critical section during which a clock
+ * consumer cannot tolerate any other consumer making any operation on the
+ * clock which could result in a rate change or rate glitch. Exclusive clocks
+ * cannot have their rate changed, either directly or indirectly due to changes
+ * further up the parent chain of clocks. As a result, clocks up parent chain
+ * also get under exclusive control of the calling consumer.
+ *
+ * If exlusivity is claimed more than once on clock, even by the same consumer,
+ * the rate effectively gets locked as exclusivity can't be preempted.
+ *
+ * Calls to clk_rate_exclusive_put() must be balanced with calls to
+ * clk_rate_exclusive_get(). Calls to this function may sleep, and do not 
return
+ * error status.
+ */
+void clk_rate_exclusive_put(struct clk *clk)
+{
+   if (!clk)
+   return;
+
+   clk_prepare_lock();
+
+   /*
+* if there is something wrong with this consumer protect count, stop
+* here before messing with the provider
+*/
+   if (WARN_ON(clk->exclusive_count <= 0))
+   goto out;
+
+   clk_core_rate_unprotect(clk->core);
+   clk->exclusive_count--;
+out:
+   clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_exclusive_put);
+
 static void clk_core_rate_protect(struct clk_core *core)
 {
lockdep_assert_held(_lock);
@@ -539,6 +579,36 @@ static void clk_core_rate_restore_protect(struct clk_core 
*core, int count)
core->protect_count = count;
 }
 
+/**
+ * clk_rate_exclusive_get - get exclusivity over the clk rate control
+ * @clk: the clk over which the exclusity of rate control is requested
+ *
+ * clk_rate_exlusive_get() begins a critical section during which a clock
+ * consumer cannot tolerate any other consumer making any operation on the
+ * clock which could result in a rate change or rate glitch. Exclusive clocks
+ * cannot have their rate changed, either directly or indirectly due to changes
+ * further up the parent chain of clocks. As a result, clocks up parent chain
+ * also get under exclusive control of the calling consumer.
+ *
+ * If exlusivity is claimed more than once on clock, even by the same consumer,
+ * the rate effectively gets locked as exclusivity can't be preempted.
+ *
+ * Calls to clk_rate_exclusive_get() should be balanced with calls to
+ * clk_rate_exclusive_put(). Calls to this function may sleep, and do not
+ * return error status.
+ */
+void clk_rate_exclusive_get(struct clk *clk)
+{
+   if (!clk)
+   return;
+
+   clk_prepare_lock();
+   clk_core_rate_protect(clk->core);
+   clk->exclusive_count++;
+   clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
+
 static void clk_core_unprepare(struct clk_core *core)
 {
lockdep_assert_held(_lock);
@@ -929,6 +999,12 @@ static int clk_core_determine_round_nolock(struct clk_core 
*core,
if (!core)
return 0;
 
+   /*
+* At this point, core protection will be disabled if
+* - if the provider is not protected at all
+* - if the calling consumer is the only one which has exclusivity
+*   over the provider
+*/
if (clk_core_rate_is_protected(core)) {
req->rate = core->rate;
} else if (core->ops->determine_rate) {
@@ -1045,10 +1121,17 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 
clk_prepare_lock();
 
+   if (clk->exclusive_count)
+   clk_core_rate_unprotect(clk->core);
+
clk_core_get_boundaries(clk->core, _rate, _rate);
req.rate = rate;
 
ret = clk_core_round_rate_nolock(clk->core, );
+
+   if (clk->exclusive_count)
+   clk_core_rate_protect(clk->core);