clk: Provide support for always-on clocks

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -	Provides 'play' over critical clocks.  A clock can be
+ *			marked as critical, meaning that it should not be
+ *			disabled.  However, if a driver which is aware of the
+ *			critical behaviour wants to control it, it can do so
+ *			using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled	Is clock critical?  Once set, doesn't change
+ * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+	bool enabled;
+	bool leave_on;
+};
+
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;

@@ -75,6 +90,7 @@ struct clk_core {
 	struct dentry		*dentry;
 #endif
 	struct kref		ref;
+	struct critical		critical;
 };
 
 struct clk {

Quoted 47 lines · Lee Jones

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -	Provides 'play' over critical clocks.  A clock can be
+ *			marked as critical, meaning that it should not be
+ *			disabled.  However, if a driver which is aware of the
+ *			critical behaviour wants to control it, it can do so
+ *			using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled	Is clock critical?  Once set, doesn't change
+ * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+	bool enabled;
+	bool leave_on;
+};
+
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;

@@ -75,6 +90,7 @@ struct clk_core {
 	struct dentry		*dentry;
 #endif
 	struct kref		ref;
+	struct critical		critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
 	if (WARN_ON(clk->enable_count == 0))
 		return;
 
+	/* Refuse to turn off a critical clock */
+	if (clk->enable_count == 1 && clk->critical.leave_on)
+		return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

 	if (--clk->enable_count > 0)
 		return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+	clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Quoted 67 lines · Maxime Ripard

Quoted 47 lines · Lee Jones

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -	Provides 'play' over critical clocks.  A clock can be
+ *			marked as critical, meaning that it should not be
+ *			disabled.  However, if a driver which is aware of the
+ *			critical behaviour wants to control it, it can do so
+ *			using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled	Is clock critical?  Once set, doesn't change
+ * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+	bool enabled;
+	bool leave_on;
+};
+
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;

@@ -75,6 +90,7 @@ struct clk_core {
 	struct dentry		*dentry;
 #endif
 	struct kref		ref;
+	struct critical		critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
 	if (WARN_ON(clk->enable_count == 0))
 		return;
 
+	/* Refuse to turn off a critical clock */
+	if (clk->enable_count == 1 && clk->critical.leave_on)
+		return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

 	if (--clk->enable_count > 0)
 		return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+	clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

I don't think we can use reference counting, because we'd need as many critical clock owners as there are critical clocks.

Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

Quoted 75 lines · Lee Jones

Quoted 67 lines · Maxime Ripard

Quoted 47 lines · Lee Jones

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -	Provides 'play' over critical clocks.  A clock can be
+ *			marked as critical, meaning that it should not be
+ *			disabled.  However, if a driver which is aware of the
+ *			critical behaviour wants to control it, it can do so
+ *			using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled	Is clock critical?  Once set, doesn't change
+ * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+	bool enabled;
+	bool leave_on;
+};
+
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;

@@ -75,6 +90,7 @@ struct clk_core {
 	struct dentry		*dentry;
 #endif
 	struct kref		ref;
+	struct critical		critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
 	if (WARN_ON(clk->enable_count == 0))
 		return;
 
+	/* Refuse to turn off a critical clock */
+	if (clk->enable_count == 1 && clk->critical.leave_on)
+		return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

 	if (--clk->enable_count > 0)
 		return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+	clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Quoted 6 lines

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

Indeed.

Quoted 2 lines

I don't think we can use reference counting, because we'd need as many critical clock owners as there are critical clocks.

Which we can have if we replace the call to clk_prepare_enable you add in your fourth patch in __set_critical_clocks.

Quoted 5 lines

Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

Quoted 2 lines

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

I don't think this addresses the issue, if you just throw more customers at it, the issue remain with your implementation.

If you have three customers that used the critical API, and if on of these calls clk_disable_critical, you're losing leave_on.

Which means that if there's one of the two users left that calls clk_disable on it, the clock will actually be disabled, which is clearly not what we want to do, as we have still a user that want the clock to be enabled.

It would be much more robust to have another count for the critical stuff, initialised to one by the __set_critical_clocks function.

Quoted 84 lines · Lee Jones

Quoted 75 lines · Maxime Ripard

Quoted 68 lines · Lee Jones

Quoted 47 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -   Provides 'play' over critical clocks.  A clock can be
+ *                     marked as critical, meaning that it should not be
+ *                     disabled.  However, if a driver which is aware of the
+ *                     critical behaviour wants to control it, it can do so
+ *                     using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled    Is clock critical?  Once set, doesn't change
+ * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
        if (WARN_ON(clk->enable_count == 0))
                return;
 
+       /* Refuse to turn off a critical clock */
+       if (clk->enable_count == 1 && clk->critical.leave_on)
+               return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

        if (--clk->enable_count > 0)
                return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+       clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Quoted 6 lines

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

Quoted 7 lines

Indeed.

Quoted 2 lines

I don't think we can use reference counting, because we'd need as many critical clock owners as there are critical clocks.

Which we can have if we replace the call to clk_prepare_enable you add in your fourth patch in __set_critical_clocks.

What should it be replaced with?

Quoted 5 lines

Quoted 5 lines

Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

Quoted 8 lines

Quoted 2 lines

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

I don't think this addresses the issue, if you just throw more customers at it, the issue remain with your implementation.

If you have three customers that used the critical API, and if on of these calls clk_disable_critical, you're losing leave_on.

That's the idea. See my point above, the one you replied "Indeed" to. So when a driver uses clk_disable_critical() it's saying, "I know why this clock is a critical clock, and I know that nothing terrible will happen if I disable it, as I have that covered". So then if it's not the last user to call clk_disable(), the last one out the door will be allowed to finally gate the clock, regardless whether it's critical aware or not.

Then, when we come to enable the clock again, the critical aware user then re-marks the clock as leave_on, so not critical un-aware user can take the final reference and disable the clock.

Quoted 4 lines

Which means that if there's one of the two users left that calls clk_disable on it, the clock will actually be disabled, which is clearly not what we want to do, as we have still a user that want the clock to be enabled.

That's not what happens (at least it shouldn't if I've coded it up right). The API _still_ requires all of the users to give-up their reference.

Quoted 2 lines

It would be much more robust to have another count for the critical stuff, initialised to one by the __set_critical_clocks function.

If I understand you correctly, we already have a count. We use the original reference count. No need for one of our own.

Using your RAM Clock (Clock 4) as an example
--------------------------------------------

Early start-up:
Clock 4 is marked as critical and a reference is taken (ref == 1)

Driver probe:
SPI enables Clock 4 (ref == 2)
I2C enables Clock 4 (ref == 3)

Suspend (without RAM driver's permission):
SPI disables Clock 4 (ref == 2)
I2C disables Clock 4 (ref == 1)
/*
* Clock won't be gated because:
* .leave_on is True - can't dec final reference

   */

Suspend (with RAM driver's permission):
/* Order is unimportant */
SPI disables Clock 4 (ref == 2)
RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0) I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */ /*
* Clock will be gated because:
* .leave_on is False, so (ref == 0)

   */

Resume:
/* Order is unimportant */
SPI enables Clock 4 (ref == 1)
RAM enables Clock 4 and re-enables .leave_on (ref == 2) I2C enables Clock 4 (ref == 3)

Hopefully that clears things up.

Please tell me if the code doesn't reflect this strategy, or if you can see anything wrong with how it operates.

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Quoting Lee Jones (2015-07-28 06:00:55)

Quoted 146 lines · Maxime Ripard

Quoted 84 lines · Lee Jones

Quoted 75 lines · Maxime Ripard

Quoted 68 lines · Lee Jones

Quoted 47 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -   Provides 'play' over critical clocks.  A clock can be
+ *                     marked as critical, meaning that it should not be
+ *                     disabled.  However, if a driver which is aware of the
+ *                     critical behaviour wants to control it, it can do so
+ *                     using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled    Is clock critical?  Once set, doesn't change
+ * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
        if (WARN_ON(clk->enable_count == 0))
                return;
 
+       /* Refuse to turn off a critical clock */
+       if (clk->enable_count == 1 && clk->critical.leave_on)
+               return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

        if (--clk->enable_count > 0)
                return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+       clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Quoted 6 lines

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

Quoted 7 lines

Indeed.

Quoted 2 lines

I don't think we can use reference counting, because we'd need as many critical clock owners as there are critical clocks.

Which we can have if we replace the call to clk_prepare_enable you add in your fourth patch in __set_critical_clocks.

What should it be replaced with?

Quoted 5 lines

Quoted 5 lines

Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

Quoted 8 lines

Quoted 2 lines

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

I don't think this addresses the issue, if you just throw more customers at it, the issue remain with your implementation.

If you have three customers that used the critical API, and if on of these calls clk_disable_critical, you're losing leave_on.

That's the idea. See my point above, the one you replied "Indeed" to. So when a driver uses clk_disable_critical() it's saying, "I know why this clock is a critical clock, and I know that nothing terrible will happen if I disable it, as I have that covered". So then if it's not the last user to call clk_disable(), the last one out the door will be allowed to finally gate the clock, regardless whether it's critical aware or not.

Then, when we come to enable the clock again, the critical aware user then re-marks the clock as leave_on, so not critical un-aware user can take the final reference and disable the clock.

Quoted 4 lines

Which means that if there's one of the two users left that calls clk_disable on it, the clock will actually be disabled, which is clearly not what we want to do, as we have still a user that want the clock to be enabled.

That's not what happens (at least it shouldn't if I've coded it up right). The API _still_ requires all of the users to give-up their reference.

Quoted 2 lines

It would be much more robust to have another count for the critical stuff, initialised to one by the __set_critical_clocks function.

If I understand you correctly, we already have a count. We use the original reference count. No need for one of our own.

Using your RAM Clock (Clock 4) as an example
--------------------------------------------

Early start-up:
Clock 4 is marked as critical and a reference is taken (ref == 1)

Driver probe:
SPI enables Clock 4 (ref == 2)
I2C enables Clock 4 (ref == 3)

Quoted 6 lines

Suspend (without RAM driver's permission):
SPI disables Clock 4 (ref == 2)
I2C disables Clock 4 (ref == 1)
/*
* Clock won't be gated because:
* .leave_on is True - can't dec final reference

I am clearly missing the point. The clock won't be gated because the enable_count is still 1! What does .leave_on do here?

Quoted 10 lines

   */

Suspend (with RAM driver's permission):
/* Order is unimportant */
SPI disables Clock 4 (ref == 2)
RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0) I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */ /*
* Clock will be gated because:
* .leave_on is False, so (ref == 0)

Again, .leave_on does nothing new here. We gate the clock because the reference count is 0.

Quoted 7 lines

   */

Resume:
/* Order is unimportant */
SPI enables Clock 4 (ref == 1)
RAM enables Clock 4 and re-enables .leave_on (ref == 2) I2C enables Clock 4 (ref == 3)

Same again. As soon as RAM calls clk_enable_critical the ref count goes up. .leave_on does nothing as far as I can tell. The all works because of the reference counting, which already exists before this patch series.

Quoted 154 lines

Quoting Lee Jones (2015-07-28 06:00:55)

Quoted 146 lines · Maxime Ripard

Quoted 84 lines · Lee Jones

Quoted 75 lines · Maxime Ripard

Quoted 68 lines · Lee Jones

Quoted 47 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -   Provides 'play' over critical clocks.  A clock can be
+ *                     marked as critical, meaning that it should not be
+ *                     disabled.  However, if a driver which is aware of the
+ *                     critical behaviour wants to control it, it can do so
+ *                     using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled    Is clock critical?  Once set, doesn't change
+ * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
        if (WARN_ON(clk->enable_count == 0))
                return;
 
+       /* Refuse to turn off a critical clock */
+       if (clk->enable_count == 1 && clk->critical.leave_on)
+               return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

        if (--clk->enable_count > 0)
                return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+       clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Quoted 6 lines

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

Quoted 7 lines

Indeed.

Quoted 2 lines

I don't think we can use reference counting, because we'd need as many critical clock owners as there are critical clocks.

Which we can have if we replace the call to clk_prepare_enable you add in your fourth patch in __set_critical_clocks.

What should it be replaced with?

Quoted 5 lines

Quoted 5 lines

Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

Quoted 8 lines

Quoted 2 lines

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

I don't think this addresses the issue, if you just throw more customers at it, the issue remain with your implementation.

If you have three customers that used the critical API, and if on of these calls clk_disable_critical, you're losing leave_on.

That's the idea. See my point above, the one you replied "Indeed" to. So when a driver uses clk_disable_critical() it's saying, "I know why this clock is a critical clock, and I know that nothing terrible will happen if I disable it, as I have that covered". So then if it's not the last user to call clk_disable(), the last one out the door will be allowed to finally gate the clock, regardless whether it's critical aware or not.

Then, when we come to enable the clock again, the critical aware user then re-marks the clock as leave_on, so not critical un-aware user can take the final reference and disable the clock.

Quoted 4 lines

Which means that if there's one of the two users left that calls clk_disable on it, the clock will actually be disabled, which is clearly not what we want to do, as we have still a user that want the clock to be enabled.

That's not what happens (at least it shouldn't if I've coded it up right). The API _still_ requires all of the users to give-up their reference.

Quoted 2 lines

It would be much more robust to have another count for the critical stuff, initialised to one by the __set_critical_clocks function.

If I understand you correctly, we already have a count. We use the original reference count. No need for one of our own.

Using your RAM Clock (Clock 4) as an example
--------------------------------------------

Early start-up:
Clock 4 is marked as critical and a reference is taken (ref == 1)

Driver probe:
SPI enables Clock 4 (ref == 2)
I2C enables Clock 4 (ref == 3)

Quoted 6 lines

Suspend (without RAM driver's permission):
SPI disables Clock 4 (ref == 2)
I2C disables Clock 4 (ref == 1)
/*
* Clock won't be gated because:
* .leave_on is True - can't dec final reference

Quoted 2 lines

I am clearly missing the point. The clock won't be gated because the enable_count is still 1! What does .leave_on do here?

The point of _this_ (the extended) part of the API is so that the clock _can_ be turned off. Without the possibility to disable .leave_on and the logic with accompanies it (i.e. clk_disable_critical()) the clock will _never_ be gated.

Quoted 11 lines

Quoted 10 lines

   */

Suspend (with RAM driver's permission):
/* Order is unimportant */
SPI disables Clock 4 (ref == 2)
RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0) I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */ /*
* Clock will be gated because:
* .leave_on is False, so (ref == 0)

Quoted 2 lines

Again, .leave_on does nothing new here. We gate the clock because the reference count is 0.

It's the fact that .leave_on has been disabled in clk_disable_critical() that allows the final reference to be taken.

Quoted 8 lines

Quoted 7 lines

   */

Resume:
/* Order is unimportant */
SPI enables Clock 4 (ref == 1)
RAM enables Clock 4 and re-enables .leave_on (ref == 2) I2C enables Clock 4 (ref == 3)

Quoted 4 lines

Same again. As soon as RAM calls clk_enable_critical the ref count goes up. .leave_on does nothing as far as I can tell. The all works because of the reference counting, which already exists before this patch series.

So fundamentally you're right in what you say. All you really need to disable a critical clock is write a knowledgeable driver, which is intentionally unbalanced i.e. just calls clk_disable(). All this

extended API really does is makes the process more official and ensures that an unintentionally unbalanced driver doesn't bugger up the running platform. We could also add a new WARN() to say that said driver is unbalanced, as it just tried to turn off a critical clock.

What do you think is best?

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

   */

Resume:
/* Order is unimportant */
SPI enables Clock 4 (ref == 1)
RAM enables Clock 4 and re-enables .leave_on (ref == 2) I2C enables Clock 4 (ref == 3)

Quoted 8 lines

Quoted 4 lines

Same again. As soon as RAM calls clk_enable_critical the ref count goes up. .leave_on does nothing as far as I can tell. The all works because of the reference counting, which already exists before this patch series.

So fundamentally you're right in what you say. All you really need to disable a critical clock is write a knowledgeable driver, which is intentionally unbalanced i.e. just calls clk_disable(). All this

OK, the line above is helpful. What you really want is a formalized hand-off mechanism, whereby the clock is enabled at registration-time and it cannot be turned off until the right driver claims it and decides turning it off is OK (with a priori knowledge that the clock is already enabled).

Note that I don't think this implementation can really work in the near future. Today we do not catch unbalanced calls to clk_enable and clk_disable, but I have a patch that catches this and WARNs loudly in my private tree. More on that in the next stanza.

What I don't understand is if there is ever a case for a clock consumer driver to ever call clk_enable_critical... I do not think there is. What you're trying to protect against is having the clock disabled BEFORE that "knowledgeable driver" has a chance to enable it.

Quoted 17 lines · Michael Turquette

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

   */

Resume:
/* Order is unimportant */
SPI enables Clock 4 (ref == 1)
RAM enables Clock 4 and re-enables .leave_on (ref == 2) I2C enables Clock 4 (ref == 3)

Quoted 8 lines

Quoted 4 lines

Same again. As soon as RAM calls clk_enable_critical the ref count goes up. .leave_on does nothing as far as I can tell. The all works because of the reference counting, which already exists before this patch series.

So fundamentally you're right in what you say. All you really need to disable a critical clock is write a knowledgeable driver, which is intentionally unbalanced i.e. just calls clk_disable(). All this

Quoted 5 lines

OK, the line above is helpful. What you really want is a formalized hand-off mechanism, whereby the clock is enabled at registration-time and it cannot be turned off until the right driver claims it and decides turning it off is OK (with a priori knowledge that the clock is already enabled).

There's two things that should be covered, and are related, yet can be done in two steps:

- Have a way to, no matter what (which configuration we have, if we have multiple users or not that might reparent or disable their clocks, etc.), make sure that a clock will always be running by default. This is done through the call in clk-conf, and we identify such clocks using critical-clocks in the DT.

- Now, even though that information is true, some driver who are aware of that fact might want to disable those critical clocks. This is what the clk_disable_critical and clk_enable_critical functions are here for.

Quoted 9 lines

Note that I don't think this implementation can really work in the near future. Today we do not catch unbalanced calls to clk_enable and clk_disable, but I have a patch that catches this and WARNs loudly in my private tree. More on that in the next stanza.

What I don't understand is if there is ever a case for a clock consumer driver to ever call clk_enable_critical... I do not think there is. What you're trying to protect against is having the clock disabled BEFORE that "knowledgeable driver" has a chance to enable it.

It's really about what we want the API to look like in the second case.

Do we want such drivers to still call clk_prepare_enable? Some other function? Should they assume that the clock has already been enabled, or do we want a handover, or a force disable, or whatever.

I guess we should discuss those questions, before starting to think about how to implement it.

IMHO, I think that the existing way of doing should be used, or at least with the same mindset to avoid confusion, errors, and misinformed reviews.

So I'd expect the drivers to do something like:

probe:
clk_get
clk_critical_enable

remove / probe error path:
clk_critical_disable
clk_put

and use the clk_critical_enable and clk_critical_disable whenever needed, and thing would just work as intended.

Quoted 5 lines

Quoted 2 lines

I don't think we can use reference counting, because we'd need as many critical clock owners as there are critical clocks.

Which we can have if we replace the call to clk_prepare_enable you add in your fourth patch in __set_critical_clocks.

What should it be replaced with?

Quoted 12 lines

Quoted 8 lines

Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

I don't think this addresses the issue, if you just throw more customers at it, the issue remain with your implementation.

Quoted 2 lines

If you have three customers that used the critical API, and if on of these calls clk_disable_critical, you're losing leave_on.

That's the idea. See my point above, the one you replied "Indeed" to. So when a driver uses clk_disable_critical() it's saying, "I know why this clock is a critical clock, and I know that nothing terrible will happen if I disable it, as I have that covered".

So then if it's not the last user to call clk_disable(), the last one out the door will be allowed to finally gate the clock, regardless whether it's critical aware or not.

Then, when we come to enable the clock again, the critical aware user then re-marks the clock as leave_on, so not critical un-aware user can take the final reference and disable the clock.

Quoted 4 lines

Which means that if there's one of the two users left that calls clk_disable on it, the clock will actually be disabled, which is clearly not what we want to do, as we have still a user that want the clock to be enabled.

That's not what happens (at least it shouldn't if I've coded it up right). The API _still_ requires all of the users to give-up their reference.

Quoted 3 lines

It would be much more robust to have another count for the critical stuff, initialised to one by the __set_critical_clocks function.

If I understand you correctly, we already have a count. We use the original reference count. No need for one of our own.

Using your RAM Clock (Clock 4) as an example
--------------------------------------------

Early start-up:
Clock 4 is marked as critical and a reference is taken (ref == 1)

Driver probe:
SPI enables Clock 4 (ref == 2)
I2C enables Clock 4 (ref == 3)

Suspend (without RAM driver's permission):
SPI disables Clock 4 (ref == 2)
I2C disables Clock 4 (ref == 1)
/*
* Clock won't be gated because:
* .leave_on is True - can't dec final reference
*/

Suspend (with RAM driver's permission):
/* Order is unimportant */
SPI disables Clock 4 (ref == 2)
RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0) I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */ /*
* Clock will be gated because:
* .leave_on is False, so (ref == 0)
*/

Resume:
/* Order is unimportant */
SPI enables Clock 4 (ref == 1)
RAM enables Clock 4 and re-enables .leave_on (ref == 2) I2C enables Clock 4 (ref == 3)

Hopefully that clears things up.

Quoted 6 lines · Lee Jones

Quoted 5 lines

Quoted 2 lines

I don't think we can use reference counting, because we'd need as many critical clock owners as there are critical clocks.

Which we can have if we replace the call to clk_prepare_enable you add in your fourth patch in __set_critical_clocks.

Quoted 1 line

What should it be replaced with?

clk->critical_count++
clk_prepare_enable

?

Quoted 11 lines

Quoted 11 lines

Quoted 7 lines

Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

I don't think this addresses the issue, if you just throw more customers at it, the issue remain with your implementation.

Quoted 7 lines

Quoted 2 lines

If you have three customers that used the critical API, and if on of these calls clk_disable_critical, you're losing leave_on.

That's the idea. See my point above, the one you replied "Indeed" to. So when a driver uses clk_disable_critical() it's saying, "I know why this clock is a critical clock, and I know that nothing terrible will happen if I disable it, as I have that covered".

We do agree on the semantic of clk_disable_critical :)

Quoted 3 lines

So then if it's not the last user to call clk_disable(), the last one out the door will be allowed to finally gate the clock, regardless whether it's critical aware or not.

That's right, but what I mean would be a case where you have two users that are aware that it is a critical clock (A and B), and one which is not (C).

If I understood correctly your code, if A calls clk_disable_critical, leave_on is set to false. That means that now, if C calls clk_disable on that clock, it will actually be shut down, while B still considers it a critical clock.

Quoted 9 lines

Then, when we come to enable the clock again, the critical aware user then re-marks the clock as leave_on, so not critical un-aware user can take the final reference and disable the clock.

Quoted 4 lines

Which means that if there's one of the two users left that calls clk_disable on it, the clock will actually be disabled, which is clearly not what we want to do, as we have still a user that want the clock to be enabled.

Quoted 3 lines

That's not what happens (at least it shouldn't if I've coded it up right). The API _still_ requires all of the users to give-up their reference.

Ah, right. So I guess it all boils down to the discussion you're having with Mike regarding whether critical users should expect to already have a reference taken or calling clk_prepare / clk_enable themselves.

Quoted 34 lines

Quoted 2 lines

It would be much more robust to have another count for the critical stuff, initialised to one by the __set_critical_clocks function.

If I understand you correctly, we already have a count. We use the original reference count. No need for one of our own.

Using your RAM Clock (Clock 4) as an example
--------------------------------------------

Early start-up:
Clock 4 is marked as critical and a reference is taken (ref == 1)

Driver probe:
SPI enables Clock 4 (ref == 2)
I2C enables Clock 4 (ref == 3)

Suspend (without RAM driver's permission):
SPI disables Clock 4 (ref == 2)
I2C disables Clock 4 (ref == 1)
/*
* Clock won't be gated because:
* .leave_on is True - can't dec final reference
*/

Suspend (with RAM driver's permission):
/* Order is unimportant */
SPI disables Clock 4 (ref == 2)
RAM disables Clock 4 (ref == 1) /* Won't turn off here (ref > 0) I2C disables Clock 4 (ref == 0) /* (.leave_on == False) last ref can be taken */ /*
* Clock will be gated because:
* .leave_on is False, so (ref == 0)
*/

Quoted 7 lines

Resume:
/* Order is unimportant */
SPI enables Clock 4 (ref == 1)
RAM enables Clock 4 and re-enables .leave_on (ref == 2) I2C enables Clock 4 (ref == 3)

Hopefully that clears things up.

It does indeed. I simply forgot to take into account the fact that it would still need the reference to be set to 0. My bad.

Still, If we take the same kind of scenario:

Early start-up:
Clock 4 is marked as critical and a reference is taken (ref == 1, leave_on = true)

Driver probe:
SPI enables Clock 4 (ref == 2)
I2C enables Clock 4 (ref == 3)
RAM enables Clock 4 (ref == 4, leave_on = true ) CPUIdle enables Clock 4 (ref == 5, leave_on = true )

Suspend (with CPUIdle and RAM permissions):
/* Order is unimportant */
SPI disables Clock 4 (ref == 4)
CPUIdle disables Clock 4 (ref == 3, leave_on = false ) RAM disables Clock 4 (ref == 2, leave_on = false ) I2C disables Clock 4 (ref == 1)

And even though the clock will still be running when CPUIdle calls clk_disable_critical because of the enable_count, the status of leave_on is off, as the RAM driver still considers it to be left on (ie, hasn't called clk_disable_critical yet).

Or at least, that's what I understood of it.

Quoted 68 lines · Lee Jones

Quoted 47 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -       Provides 'play' over critical clocks.  A clock can be
+ *                 marked as critical, meaning that it should not be
+ *                 disabled.  However, if a driver which is aware of the
+ *                 critical behaviour wants to control it, it can do so
+ *                 using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled        Is clock critical?  Once set, doesn't change
+ * @leave_on       Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+   bool enabled;
+   bool leave_on;
+};
+
 struct clk_core {
    const char              *name;
    const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
    struct dentry           *dentry;
 #endif
    struct kref             ref;
+   struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
    if (WARN_ON(clk->enable_count == 0))
            return;
 
+   /* Refuse to turn off a critical clock */
+   if (clk->enable_count == 1 && clk->critical.leave_on)
+           return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

    if (--clk->enable_count > 0)
            return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+   clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

I don't think we can use reference
counting, because we'd need as many critical clock owners as there are critical clocks. Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Quoting Lee Jones (2015-07-27 01:53:38)

Quoted 75 lines · Maxime Ripard

Quoted 68 lines · Lee Jones

Quoted 47 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -       Provides 'play' over critical clocks.  A clock can be
+ *                 marked as critical, meaning that it should not be
+ *                 disabled.  However, if a driver which is aware of the
+ *                 critical behaviour wants to control it, it can do so
+ *                 using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled        Is clock critical?  Once set, doesn't change
+ * @leave_on       Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+   bool enabled;
+   bool leave_on;
+};
+
 struct clk_core {
    const char              *name;
    const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
    struct dentry           *dentry;
 #endif
    struct kref             ref;
+   struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
    if (WARN_ON(clk->enable_count == 0))
            return;
 
+   /* Refuse to turn off a critical clock */
+   if (clk->enable_count == 1 && clk->critical.leave_on)
+           return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

    if (--clk->enable_count > 0)
            return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+   clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Quoted 6 lines

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

If this statement above is true then I fail to see the need for a new api. A driver which has a really great idea of when it is safe or unsafe to gate a clock should call clk_prepare_enable at probe and then only call clk_disable_unprepare once it is safe to do so.

The existing bookkeeping in the clock framework will do the rest.

Quoted 10 lines

I don't think we can use reference
counting, because we'd need as many critical clock owners as there are critical clocks. Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

Quoted 88 lines

Quoting Lee Jones (2015-07-27 01:53:38)

Quoted 75 lines · Maxime Ripard

Quoted 68 lines · Lee Jones

Quoted 47 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -       Provides 'play' over critical clocks.  A clock can be
+ *                 marked as critical, meaning that it should not be
+ *                 disabled.  However, if a driver which is aware of the
+ *                 critical behaviour wants to control it, it can do so
+ *                 using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled        Is clock critical?  Once set, doesn't change
+ * @leave_on       Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+   bool enabled;
+   bool leave_on;
+};
+
 struct clk_core {
    const char              *name;
    const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
    struct dentry           *dentry;
 #endif
    struct kref             ref;
+   struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
    if (WARN_ON(clk->enable_count == 0))
            return;
 
+   /* Refuse to turn off a critical clock */
+   if (clk->enable_count == 1 && clk->critical.leave_on)
+           return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

    if (--clk->enable_count > 0)
            return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+   clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Quoted 6 lines

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

If this statement above is true then I fail to see the need for a new api. A driver which has a really great idea of when it is safe or unsafe to gate a clock should call clk_prepare_enable at probe and then only call clk_disable_unprepare once it is safe to do so.

Quoted 1 line

The existing bookkeeping in the clock framework will do the rest.

I think you are viewing this particular API back-to-front. The idea is to mark all of the critical clocks at start-up by taking a reference. Then, if there are no knowledgable drivers who wish to turn the clock off, the CCF will leave the clock ungated becuase the reference count will always be >0.

The clk_{disable,enable}_critical() calls are to be used by knowledgable drivers to say "[disable] I know what I'm doing and it's okay for this clock to be turned off" and "[enable] right I'm done with this clock now, let's turn it back on and mark it back as critical, so no one else can turn it off".

To put things simply, the knowledgable driver will _not_ be enabling the clock in the first place. The first interaction it has with it is to gate it. Then, once it's done, it will enable it again and mark it back up as critical.a

Still confused ... let's go on another Q in one of your other emails for another way of putting it.

Quoted 10 lines

Quoted 10 lines

I don't think we can use reference
counting, because we'd need as many critical clock owners as there are critical clocks. Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Quoting Lee Jones (2015-07-30 02:21:39)

Quoted 89 lines · Michael Turquette

Quoted 88 lines

Quoting Lee Jones (2015-07-27 01:53:38)

Quoted 75 lines · Maxime Ripard

Quoted 68 lines · Lee Jones

Quoted 47 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +16 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/
 
+/**
+ * struct critical -       Provides 'play' over critical clocks.  A clock can be
+ *                 marked as critical, meaning that it should not be
+ *                 disabled.  However, if a driver which is aware of the
+ *                 critical behaviour wants to control it, it can do so
+ *                 using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled        Is clock critical?  Once set, doesn't change
+ * @leave_on       Self explanatory.  Can be disabled by knowledgeable drivers
+ */
+struct critical {
+   bool enabled;
+   bool leave_on;
+};
+
 struct clk_core {
    const char              *name;
    const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
    struct dentry           *dentry;
 #endif
    struct kref             ref;
+   struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1017

Quoted 7 lines

drivers/clk/clk.c +4 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
    if (WARN_ON(clk->enable_count == 0))
            return;
 
+   /* Refuse to turn off a critical clock */
+   if (clk->enable_count == 1 && clk->critical.leave_on)
+           return;
+

I think it should be handled by a separate counting. Otherwise, if you have two users that marked the clock as critical, and then one of them disable it...

Quoted 7 lines

drivers/clk/clk.c +0 -0

    if (--clk->enable_count > 0)
            return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 

Quoted 5 lines

drivers/clk/clk.c void clk_disable(struct clk *clk) · line 1062

Quoted 3 lines

drivers/clk/clk.c +3 -0

+void clk_disable_critical(struct clk *clk)
+{
+   clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Quoted 6 lines

I thought about both of these points, which is why I came up with this strategy.

Any device which uses the *_critical() API should a) have knowledge of what happens when a particular critical clock is gated and b) have thought about the consequences.

If this statement above is true then I fail to see the need for a new api. A driver which has a really great idea of when it is safe or unsafe to gate a clock should call clk_prepare_enable at probe and then only call clk_disable_unprepare once it is safe to do so.

Quoted 7 lines

Quoted 1 line

The existing bookkeeping in the clock framework will do the rest.

I think you are viewing this particular API back-to-front. The idea is to mark all of the critical clocks at start-up by taking a reference. Then, if there are no knowledgable drivers who wish to turn the clock off, the CCF will leave the clock ungated becuase the reference count will always be >0.

Right. So I'll ask the same question here that I asked in the other
patch: is there ever a case where a clock consumer driver would want to call clk_enable_critical? It seems to me that in you usage of it, that call would only ever be made by the core framework code (e.g. clk-conf.c or perhaps somewhere in drivers/clk/clk.c).

Quoted 5 lines

The clk_{disable,enable}_critical() calls are to be used by knowledgable drivers to say "[disable] I know what I'm doing and it's okay for this clock to be turned off" and "[enable] right I'm done with this clock now, let's turn it back on and mark it back as critical, so no one else can turn it off".

OK, so this almost answers my question above. You have a driver that may finish using a clock for a while (ie, rmmod knowledgeable_driver), and you want it (critically) enabled again. Is this a real use case? Who would come along and disable this clock later on? If the driver is to be re-loaded then I would suggest never unloading it in the first place.

(Oh and bear in mind when answering my question above that imbalanced clk_enable/clk_disable calls will not succeed thanks to the vaporware patch that I have in my local tree)

If you have a second knowledgeable_driver_2 that shares that clock and can be trusted to manage it (critically) then I would need to see that example, as it doesn't feel like a real use to me.

Quoted 4 lines

To put things simply, the knowledgable driver will _not_ be enabling the clock in the first place. The first interaction it has with it is to gate it. Then, once it's done, it will enable it again and mark it back up as critical.a

I like the first part. Makes sense and fills a real need. I am fully on-board with a provider-handoff-to-consumer solution. It is all the weird stuff that comes after it that I'm unsure of.

Quoted 14 lines

Still confused ... let's go on another Q in one of your other emails for another way of putting it.

Quoted 10 lines

Quoted 10 lines

I don't think we can use reference
counting, because we'd need as many critical clock owners as there are critical clocks. Cast your mind back to the reasons for this critical clock API. One of the most important intentions of this API is the requirement mitigation for each of the critical clocks to have an owner (driver).

With regards to your second point, that's what 'critical.enabled' is for. Take a look at clk_enable_critical().

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +0 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/

drivers/clk/clk.c +7 -0

+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c +26 -0

+
        if (--clk->enable_count > 0)
                return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+void clk_disable_critical(struct clk *clk)
+{
+       clk->core->critical.leave_on = false;
+       clk_disable(clk);
+}
+EXPORT_SYMBOL_GPL(clk_disable_critical);
+
 static int clk_core_enable(struct clk_core *clk)
 {
        int ret = 0;

@@ -1100,6 +1127,15 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+int clk_enable_critical(struct clk *clk)
+{
+       if (clk->core->critical.enabled)
+               clk->core->critical.leave_on = true;
+
+       return clk_enable(clk);
+}
+EXPORT_SYMBOL_GPL(clk_enable_critical);
+
 static unsigned long clk_core_round_rate_nolock(struct clk_core *clk,
                                                unsigned long rate,
                                                unsigned long min_rate,

@@ -2482,6 +2518,15 @@ fail_out:
 }
 EXPORT_SYMBOL_GPL(clk_register);
 
+void clk_init_critical(struct clk *clk)
+{
+       struct critical *critical = &clk->core->critical;
+
+       critical->enabled = true;
+       critical->leave_on = true;
+}
+EXPORT_SYMBOL_GPL(clk_init_critical);
+
 /*
  * Free memory allocated for a clock.
  * Caller must hold prepare_lock.

include/linux/clk-provider.h +2 -0

index 5591ea7..15ef8c9 100644
@@ -563,6 +563,8 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
 void clk_unregister(struct clk *clk);
 void devm_clk_unregister(struct device *dev, struct clk *clk);
 
+void clk_init_critical(struct clk *clk);
+
 /* helper functions */
 const char *__clk_get_name(struct clk *clk);
 struct clk_hw *__clk_get_hw(struct clk *clk);

include/linux/clk.h +30 -0

index 8381bbf..9807f3b 100644
@@ -231,6 +231,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
 int clk_enable(struct clk *clk);
 
 /**
+ * clk_enable_critical - inform the system when the clock source should be
+ *                      running, even if clock is critical.
+ * @clk: clock source
+ *
+ * If the clock can not be enabled/disabled, this should return success.
+ *
+ * May be called from atomic contexts.
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_enable_critical(struct clk *clk);
+
+/**
  * clk_disable - inform the system when the clock source is no longer required.
  * @clk: clock source
  *

@@ -247,6 +260,23 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_disable_critical - inform the system when the clock source is no
+ *                       longer required, even if clock is critical.
+ * @clk: clock source
+ *
+ * Inform the system that a clock source is no longer required by
+ * a driver and may be shut down.
+ *
+ * May be called from atomic contexts.
+ *
+ * Implementation detail: if the clock source is shared between
+ * multiple drivers, clk_enable_critical() calls must be balanced
+ * by the same number of clk_disable_critical() calls for the clock
+ * source to be disabled.
+ */
+void clk_disable_critical(struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *               This is only valid once the clock source has been enabled.
  * @clk: clock source

Signature · 1 line

1.9.1

Hi Lee,

+ linux-clk ml

Quoting Lee Jones (2015-07-22 06:04:13)

Quoted 19 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +0 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/

drivers/clk/clk.c static struct clk_core *clk_core_lookup(const char *name); · line 57

Quoted 10 lines

drivers/clk/clk.c +9 -0

 
+/**
+ * struct critical -   Provides 'play' over critical clocks.  A clock can be
+ *                     marked as critical, meaning that it should not be
+ *                     disabled.  However, if a driver which is aware of the
+ *                     critical behaviour wants to control it, it can do so
+ *                     using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled    Is clock critical?  Once set, doesn't change
+ * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers

Not self explanatory. I need this explained to me. What does leave_on do? Better yet, what would happen if leave_on did not exist?

Quoted 18 lines

drivers/clk/clk.c +7 -0

+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1016

Quoted 6 lines

drivers/clk/clk.c +3 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
        if (WARN_ON(clk->enable_count == 0))
                return;
 
+       /* Refuse to turn off a critical clock */
+       if (clk->enable_count == 1 && clk->critical.leave_on)
+               return;

How do we get to this point? clk_enable_critical actually calls clk_enable, thus incrementing the enable_count. The only time that we could hit the above case is if,

a) there is an imbalance in clk_enable and clk_disable calls. If this is the case then the drivers need to be fixed. Or better yet some infrastructure to catch that, now that we have per-user struct clk cookies.

b) a driver knowingly calls clk_enable_critical(foo) and then regular, old clk_disable(foo). But why would a driver do that?

It might be that I am missing the point here, so please feel free to clue me in.

Quoted 111 lines

drivers/clk/clk.c +26 -0

+
        if (--clk->enable_count > 0)
                return;
 

@@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+void clk_disable_critical(struct clk *clk)
+{
+       clk->core->critical.leave_on = false;
+       clk_disable(clk);
+}
+EXPORT_SYMBOL_GPL(clk_disable_critical);
+
 static int clk_core_enable(struct clk_core *clk)
 {
        int ret = 0;

@@ -1100,6 +1127,15 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+int clk_enable_critical(struct clk *clk)
+{
+       if (clk->core->critical.enabled)
+               clk->core->critical.leave_on = true;
+
+       return clk_enable(clk);
+}
+EXPORT_SYMBOL_GPL(clk_enable_critical);
+
 static unsigned long clk_core_round_rate_nolock(struct clk_core *clk,
                                                unsigned long rate,
                                                unsigned long min_rate,

@@ -2482,6 +2518,15 @@ fail_out:
 }
 EXPORT_SYMBOL_GPL(clk_register);
 
+void clk_init_critical(struct clk *clk)
+{
+       struct critical *critical = &clk->core->critical;
+
+       critical->enabled = true;
+       critical->leave_on = true;
+}
+EXPORT_SYMBOL_GPL(clk_init_critical);
+
 /*
  * Free memory allocated for a clock.
  * Caller must hold prepare_lock.

include/linux/clk-provider.h +2 -0

index 5591ea7..15ef8c9 100644
@@ -563,6 +563,8 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
 void clk_unregister(struct clk *clk);
 void devm_clk_unregister(struct device *dev, struct clk *clk);
 
+void clk_init_critical(struct clk *clk);
+
 /* helper functions */
 const char *__clk_get_name(struct clk *clk);
 struct clk_hw *__clk_get_hw(struct clk *clk);

include/linux/clk.h +30 -0

index 8381bbf..9807f3b 100644
@@ -231,6 +231,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
 int clk_enable(struct clk *clk);
 
 /**
+ * clk_enable_critical - inform the system when the clock source should be
+ *                      running, even if clock is critical.
+ * @clk: clock source
+ *
+ * If the clock can not be enabled/disabled, this should return success.
+ *
+ * May be called from atomic contexts.
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_enable_critical(struct clk *clk);
+
+/**
  * clk_disable - inform the system when the clock source is no longer required.
  * @clk: clock source
  *

@@ -247,6 +260,23 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_disable_critical - inform the system when the clock source is no
+ *                       longer required, even if clock is critical.
+ * @clk: clock source
+ *
+ * Inform the system that a clock source is no longer required by
+ * a driver and may be shut down.
+ *
+ * May be called from atomic contexts.
+ *
+ * Implementation detail: if the clock source is shared between
+ * multiple drivers, clk_enable_critical() calls must be balanced
+ * by the same number of clk_disable_critical() calls for the clock
+ * source to be disabled.
+ */
+void clk_disable_critical(struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *               This is only valid once the clock source has been enabled.
  * @clk: clock source

Quoted 76 lines

Hi Lee,

+ linux-clk ml

Quoting Lee Jones (2015-07-22 06:04:13)

Quoted 19 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +0 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/

drivers/clk/clk.c static struct clk_core *clk_core_lookup(const char *name); · line 57

Quoted 10 lines

drivers/clk/clk.c +9 -0

 
+/**
+ * struct critical -   Provides 'play' over critical clocks.  A clock can be
+ *                     marked as critical, meaning that it should not be
+ *                     disabled.  However, if a driver which is aware of the
+ *                     critical behaviour wants to control it, it can do so
+ *                     using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled    Is clock critical?  Once set, doesn't change
+ * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers

Not self explanatory. I need this explained to me. What does leave_on do? Better yet, what would happen if leave_on did not exist?

Quoted 18 lines

drivers/clk/clk.c +7 -0

+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1016

Quoted 6 lines

drivers/clk/clk.c +3 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
        if (WARN_ON(clk->enable_count == 0))
                return;
 
+       /* Refuse to turn off a critical clock */
+       if (clk->enable_count == 1 && clk->critical.leave_on)
+               return;

How do we get to this point? clk_enable_critical actually calls clk_enable, thus incrementing the enable_count. The only time that we could hit the above case is if,

a) there is an imbalance in clk_enable and clk_disable calls. If this is the case then the drivers need to be fixed. Or better yet some infrastructure to catch that, now that we have per-user struct clk cookies.

b) a driver knowingly calls clk_enable_critical(foo) and then regular, old clk_disable(foo). But why would a driver do that?

Quoted 2 lines

It might be that I am missing the point here, so please feel free to clue me in.

This check behaves in a very similar to the WARN() above. It's more of a fail-safe. If all drivers are behaving properly, then it shouldn't ever be true. If they're not, it prevents an incorrectly written driver from irrecoverably crippling the system.

As I said in the other mail. We can do without these 3 new wrappers. We _could_ just write a driver which only calls clk_enable() _after_ it calls clk_disable(), a kind of intentional unbalance and it would do that same thing.

However, what we're trying to do here is provide
a proper API, so we can see at first glance what the 'knowledgeable' driver is trying to do and not have someone attempt to submit a 'fix' which calls clk_enable() or something.

From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001

From ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001

From the clk_put kerneldoc in include/linux/clk.h:

Quoting Lee Jones (2015-07-30 04:17:47)

Quoted 80 lines · Michael Turquette

Quoted 78 lines

Hi Lee,

+ linux-clk ml

Quoting Lee Jones (2015-07-22 06:04:13)

Quoted 19 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +0 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/

drivers/clk/clk.c static struct clk_core *clk_core_lookup(const char *name); · line 57

Quoted 10 lines

drivers/clk/clk.c +9 -0

 
+/**
+ * struct critical -   Provides 'play' over critical clocks.  A clock can be
+ *                     marked as critical, meaning that it should not be
+ *                     disabled.  However, if a driver which is aware of the
+ *                     critical behaviour wants to control it, it can do so
+ *                     using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled    Is clock critical?  Once set, doesn't change
+ * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers

Not self explanatory. I need this explained to me. What does leave_on do? Better yet, what would happen if leave_on did not exist?

Quoted 18 lines

drivers/clk/clk.c +7 -0

+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1016

Quoted 6 lines

drivers/clk/clk.c +3 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
        if (WARN_ON(clk->enable_count == 0))
                return;
 
+       /* Refuse to turn off a critical clock */
+       if (clk->enable_count == 1 && clk->critical.leave_on)
+               return;

How do we get to this point? clk_enable_critical actually calls clk_enable, thus incrementing the enable_count. The only time that we could hit the above case is if,

a) there is an imbalance in clk_enable and clk_disable calls. If this is the case then the drivers need to be fixed. Or better yet some infrastructure to catch that, now that we have per-user struct clk cookies.

b) a driver knowingly calls clk_enable_critical(foo) and then regular, old clk_disable(foo). But why would a driver do that?

It might be that I am missing the point here, so please feel free to clue me in.

Quoted 4 lines

This check behaves in a very similar to the WARN() above. It's more of a fail-safe. If all drivers are behaving properly, then it shouldn't ever be true. If they're not, it prevents an incorrectly written driver from irrecoverably crippling the system.

Then this check should be replaced with a generic approach that refuses to honor imbalances anyways. Below are two patches that probably resolve the issue of badly behaving drivers that cause enable imbalances.

Quoted 4 lines

As I said in the other mail. We can do without these 3 new wrappers. We _could_ just write a driver which only calls clk_enable() _after_ it calls clk_disable(), a kind of intentional unbalance and it would do that same thing.

This naive approach will not work with per-user imbalance tracking.

Quoted 4 lines

However, what we're trying to do here is provide
a proper API, so we can see at first glance what the 'knowledgeable' driver is trying to do and not have someone attempt to submit a 'fix' which calls clk_enable() or something.

We'll need some type of api for sure for the handoff.

From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001
From: Michael Turquette <[email protected]>
Date: Wed, 29 Jul 2015 18:22:45 -0700
Subject: [PATCH 1/2] clk: per-user clk prepare & enable ref counts

This patch adds prepare and enable reference counts for the per-user handles that clock consumers have for a clock node. This patch warns if an imbalance occurs while trying to disable or unprepare a clock and aborts, leaving the hardware unaffected.

Signed-off-by Michael Turquette <[email protected]>

---
 drivers/clk/clk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/clk/clk.c +10 -0

index 898052e..72feee9 100644
@@ -84,6 +84,8 @@ struct clk {
 	unsigned long min_rate;
 	unsigned long max_rate;
 	struct hlist_node clks_node;
+	unsigned int enable_count;
+	unsigned int prepare_count;
 };
 
 /***           locking             ***/

@@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk)
 		return;
 
 	clk_prepare_lock();
+	if (WARN_ON(clk->prepare_count == 0))
+		return;
+	clk->prepare_count--;
 	clk_core_unprepare(clk->core);
 	clk_prepare_unlock();
 }

@@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk)
 		return 0;
 
 	clk_prepare_lock();
+	clk->prepare_count++;
 	ret = clk_core_prepare(clk->core);
 	clk_prepare_unlock();
 

@@ -707,6 +713,9 @@ void clk_disable(struct clk *clk)
 		return;
 
 	flags = clk_enable_lock();
+	if (WARN_ON(clk->enable_count == 0))
+		return;
+	clk->enable_count--;
 	clk_core_disable(clk->core);
 	clk_enable_unlock(flags);
 }

@@ -769,6 +778,7 @@ int clk_enable(struct clk *clk)
 		return 0;
 
 	flags = clk_enable_lock();
+	clk->enable_count++;
 	ret = clk_core_enable(clk->core);
 	clk_enable_unlock(flags);
 

Quoted 82 lines

Quoting Lee Jones (2015-07-30 04:17:47)

Quoted 80 lines · Michael Turquette

Quoted 78 lines

Hi Lee,

+ linux-clk ml

Quoting Lee Jones (2015-07-22 06:04:13)

Quoted 19 lines

These new API calls will firstly provide a mechanisms to tag a clock as critical and secondly allow any knowledgeable driver to (un)gate clocks, even if they are marked as critical.

Suggested-by Maxime Ripard <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

drivers/clk/clk.c +0 -0

index 61c3fc5..486b1da 100644
@@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
 
 /***    private data structures    ***/

drivers/clk/clk.c static struct clk_core *clk_core_lookup(const char *name); · line 57

Quoted 10 lines

drivers/clk/clk.c +9 -0

 
+/**
+ * struct critical -   Provides 'play' over critical clocks.  A clock can be
+ *                     marked as critical, meaning that it should not be
+ *                     disabled.  However, if a driver which is aware of the
+ *                     critical behaviour wants to control it, it can do so
+ *                     using clk_enable_critical() and clk_disable_critical().
+ *
+ * @enabled    Is clock critical?  Once set, doesn't change
+ * @leave_on   Self explanatory.  Can be disabled by knowledgeable drivers

Not self explanatory. I need this explained to me. What does leave_on do? Better yet, what would happen if leave_on did not exist?

Quoted 18 lines

drivers/clk/clk.c +7 -0

+ */
+struct critical {
+       bool enabled;
+       bool leave_on;
+};
+
 struct clk_core {
        const char              *name;
        const struct clk_ops    *ops;

@@ -75,6 +90,7 @@ struct clk_core {
        struct dentry           *dentry;
 #endif
        struct kref             ref;
+       struct critical         critical;
 };
 
 struct clk {

drivers/clk/clk.c static void clk_core_disable(struct clk_core *clk) · line 1016

Quoted 6 lines

drivers/clk/clk.c +3 -0

@@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
        if (WARN_ON(clk->enable_count == 0))
                return;
 
+       /* Refuse to turn off a critical clock */
+       if (clk->enable_count == 1 && clk->critical.leave_on)
+               return;

How do we get to this point? clk_enable_critical actually calls clk_enable, thus incrementing the enable_count. The only time that we could hit the above case is if,

a) there is an imbalance in clk_enable and clk_disable calls. If this is the case then the drivers need to be fixed. Or better yet some infrastructure to catch that, now that we have per-user struct clk cookies.

b) a driver knowingly calls clk_enable_critical(foo) and then regular, old clk_disable(foo). But why would a driver do that?

It might be that I am missing the point here, so please feel free to clue me in.

Quoted 8 lines

Quoted 4 lines

This check behaves in a very similar to the WARN() above. It's more of a fail-safe. If all drivers are behaving properly, then it shouldn't ever be true. If they're not, it prevents an incorrectly written driver from irrecoverably crippling the system.

Then this check should be replaced with a generic approach that refuses to honor imbalances anyways. Below are two patches that probably resolve the issue of badly behaving drivers that cause enable imbalances.

Your patch should make the requirement for this check moot, so it can probably be removed.

Quoted 6 lines

Quoted 4 lines

As I said in the other mail. We can do without these 3 new wrappers. We _could_ just write a driver which only calls clk_enable() _after_ it calls clk_disable(), a kind of intentional unbalance and it would do that same thing.

This naive approach will not work with per-user imbalance tracking.

Steady on. I said we "_could_", that that I think it's a good idea.

I think it's a bad idea, which is why I wrote this set. ;)

Quoted 6 lines

Quoted 4 lines

However, what we're trying to do here is provide
a proper API, so we can see at first glance what the 'knowledgeable' driver is trying to do and not have someone attempt to submit a 'fix' which calls clk_enable() or something.

We'll need some type of api for sure for the handoff.

This set will not trigger your new checks. The clocks will be in perfect ballance becuase a reference will be taken at start-up.

Again:

start-up:
clk_prepare_enable()

knowlegable_driver_probe:
clk_get()

knowlegable_driver_gate_clk:
clk_disable_critical()

knowlegable_driver_ungate_clk:
clk_enable_critical()

knowlegable_driver_remove:
clk_put()

Quoted 64 lines

From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001
From: Michael Turquette <[email protected]>
Date: Wed, 29 Jul 2015 18:22:45 -0700
Subject: [PATCH 1/2] clk: per-user clk prepare & enable ref counts

This patch adds prepare and enable reference counts for the per-user handles that clock consumers have for a clock node. This patch warns if an imbalance occurs while trying to disable or unprepare a clock and aborts, leaving the hardware unaffected.

Signed-off-by Michael Turquette <[email protected]>

---
 drivers/clk/clk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/clk/clk.c +10 -0

index 898052e..72feee9 100644
@@ -84,6 +84,8 @@ struct clk {
      unsigned long min_rate;
      unsigned long max_rate;
      struct hlist_node clks_node;
+     unsigned int enable_count;
+     unsigned int prepare_count;
 };
 
 /***           locking             ***/

@@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk)
              return;
 
      clk_prepare_lock();
+     if (WARN_ON(clk->prepare_count == 0))
+             return;
+     clk->prepare_count--;
      clk_core_unprepare(clk->core);
      clk_prepare_unlock();
 }

@@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk)
              return 0;
 
      clk_prepare_lock();
+     clk->prepare_count++;
      ret = clk_core_prepare(clk->core);
      clk_prepare_unlock();
 

@@ -707,6 +713,9 @@ void clk_disable(struct clk *clk)
              return;
 
      flags = clk_enable_lock();
+     if (WARN_ON(clk->enable_count == 0))
+             return;
+     clk->enable_count--;
      clk_core_disable(clk->core);
      clk_enable_unlock(flags);
 }

@@ -769,6 +778,7 @@ int clk_enable(struct clk *clk)
              return 0;
 
      flags = clk_enable_lock();
+     clk->enable_count++;
      ret = clk_core_enable(clk->core);
      clk_enable_unlock(flags);
 

Signature · 8 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lots of platforms contain clocks which if turned off would prove fatal. The only way to recover from these catastrophic failures is to restart the board(s). Now, when a clock provider is registered with the framework it is possible for a list of critical clocks to be supplied which must be kept ungated. Each clock mentioned in the newly introduced 'critical-clock' property will be clk_prepare_enable()d where the normal references will be taken. This will prevent the common clk framework from attempting to gate them during the normal clk_disable_unused() and disable_clock() procedures.

Note that it will still be possible for knowledgeable drivers to turn these clocks off using clk_{enable,disable}_critical() calls.

Signed-off-by Lee Jones <[email protected]>

---
 drivers/clk/clk-conf.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

drivers/clk/clk-conf.c +44 -2

index aad4796..f83c1c2 100644
@@ -116,6 +116,45 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
        return 0;
 }
 
+static int __set_critical_clocks(struct device_node *node, bool clk_supplier)
+{
+       struct of_phandle_args clkspec;
+       struct clk *clk;
+       struct property *prop;
+       const __be32 *cur;
+       uint32_t index;
+       int ret;
+
+       if (!clk_supplier)
+               return 0;
+
+       of_property_for_each_u32(node, "critical-clock", prop, cur, index) {
+               clkspec.np = node;
+               clkspec.args_count = 1;
+               clkspec.args[0] = index;
+
+               clk = of_clk_get_from_provider(&clkspec);
+               if (IS_ERR(clk)) {
+                       pr_err("clk: couldn't get clock %u for %s\n",
+                               index, node->full_name);
+                       return PTR_ERR(clk);
+               }
+
+               clk_init_critical(clk);
+
+               ret = clk_prepare_enable(clk);
+               if (ret) {
+                       pr_err("Failed to enable clock %u for %s: %d\n",
+                              index, node->full_name, ret);
+                       return ret;
+               }
+
+               pr_debug("Setting clock as critical %pC\n", clk);
+       }
+
+       return 0;
+}
+
 /**
  * of_clk_set_defaults() - parse and set assigned clocks configuration
  * @node: device node to apply clock settings for

@@ -139,6 +178,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
        if (rc < 0)
                return rc;
 
-       return __set_clk_rates(node, clk_supplier);
+       rc = __set_clk_rates(node, clk_supplier);
+       if (rc < 0)
+               return rc;
+
+       return __set_critical_clocks(node, clk_supplier);
 }
 EXPORT_SYMBOL_GPL(of_clk_set_defaults);
--

1.9.1

2015-07-22 21:04 GMT+08:00 Lee Jones <[email protected]>:

Quoted 10 lines

Lots of platforms contain clocks which if turned off would prove fatal. The only way to recover from these catastrophic failures is to restart the board(s). Now, when a clock provider is registered with the framework it is possible for a list of critical clocks to be supplied which must be kept ungated. Each clock mentioned in the newly introduced 'critical-clock' property will be clk_prepare_enable()d where the normal references will be taken. This will prevent the common clk framework from attempting to gate them during the normal clk_disable_unused() and disable_clock() procedures.

Quoted 4 lines

Note that it will still be possible for knowledgeable drivers to turn these clocks off using clk_{enable,disable}_critical() calls.

Signed-off-by Lee Jones <[email protected]>

hi Lee,
i have another email about this. i am wondering whether CLK_IGNORE_UNUSE is still useful after your patch. another solution for your patch might be extending the current CLK_IGNORE_UNUSE to runtime?

copy the mail here:
currently we can set a CLK_IGNORE_UNUSE flag to a clock to stop clk_disable_unused() from disabling it at the boot stage:

static void clk_disable_unused_subtree(struct clk_core *core) {
...

if (core->flags & CLK_IGNORE_UNUSED)
goto unlock_out;
}

static int clk_disable_unused(void)
{
...

clk_unprepare_unused_subtree(core);
...
return 0;
}

late_initcall_sync(clk_disable_unused);

so if there are two clocks A and B, A is the parent of B, and A is marked as CLK_IGNORE_UNUSED.

in boot stage if there is nobody using A and B, Linux will disable B due to clk_disable_unused() , but keep A being enabled since A has CLK_IGNORE_UNUSED.

but in Linux runtime, we might frequently disable /enable B in runtime power management, this will cause A disabled since Linux will not check CLK_IGNORE_UNUSED for runtime disabling clk .

so this makes CLK_IGNORE_UNUSE only work if we don't disable its
sub-clock at runtime. this looks making no sense.

i am thinking whether we should do some changes to make it have side
affect for runtime clk disable. otherwise, why do we want to stop it from being disabled during boot stage?

I am not sure whether i missed something in clk core level support.

-barry

Quoted 66 lines

---
 drivers/clk/clk-conf.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

drivers/clk/clk-conf.c +44 -1

index aad4796..f83c1c2 100644
@@ -116,6 +116,45 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
        return 0;
 }
 
+static int __set_critical_clocks(struct device_node *node, bool clk_supplier)
+{
+       struct of_phandle_args clkspec;
+       struct clk *clk;
+       struct property *prop;
+       const __be32 *cur;
+       uint32_t index;
+       int ret;
+
+       if (!clk_supplier)
+               return 0;
+
+       of_property_for_each_u32(node, "critical-clock", prop, cur, index) {
+               clkspec.np = node;
+               clkspec.args_count = 1;
+               clkspec.args[0] = index;
+
+               clk = of_clk_get_from_provider(&clkspec);
+               if (IS_ERR(clk)) {
+                       pr_err("clk: couldn't get clock %u for %s\n",
+                               index, node->full_name);
+                       return PTR_ERR(clk);
+               }
+
+               clk_init_critical(clk);
+
+               ret = clk_prepare_enable(clk);
+               if (ret) {
+                       pr_err("Failed to enable clock %u for %s: %d\n",
+                              index, node->full_name, ret);
+                       return ret;
+               }
+
+               pr_debug("Setting clock as critical %pC\n", clk);
+       }
+
+       return 0;
+}
+
 /**
  * of_clk_set_defaults() - parse and set assigned clocks configuration
  * @node: device node to apply clock settings for

@@ -139,6 +178,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
        if (rc < 0)
                return rc;
 
-       return __set_clk_rates(node, clk_supplier);
+       rc = __set_clk_rates(node, clk_supplier);
+       if (rc < 0)
+               return rc;
+
+       return __set_critical_clocks(node, clk_supplier);
 }
 EXPORT_SYMBOL_GPL(of_clk_set_defaults);

Quoted 58 lines

2015-07-22 21:04 GMT+08:00 Lee Jones <[email protected]>:

Quoted 10 lines

Lots of platforms contain clocks which if turned off would prove fatal. The only way to recover from these catastrophic failures is to restart the board(s). Now, when a clock provider is registered with the framework it is possible for a list of critical clocks to be supplied which must be kept ungated. Each clock mentioned in the newly introduced 'critical-clock' property will be clk_prepare_enable()d where the normal references will be taken. This will prevent the common clk framework from attempting to gate them during the normal clk_disable_unused() and disable_clock() procedures.

Quoted 4 lines

Note that it will still be possible for knowledgeable drivers to turn these clocks off using clk_{enable,disable}_critical() calls.

Signed-off-by Lee Jones <[email protected]>

hi Lee,
i have another email about this. i am wondering whether CLK_IGNORE_UNUSE is still useful after your patch. another solution for your patch might be extending the current CLK_IGNORE_UNUSE to runtime?

copy the mail here:
currently we can set a CLK_IGNORE_UNUSE flag to a clock to stop clk_disable_unused() from disabling it at the boot stage:

static void clk_disable_unused_subtree(struct clk_core *core) {
...

if (core->flags & CLK_IGNORE_UNUSED)
goto unlock_out;
}

static int clk_disable_unused(void)
{
...

clk_unprepare_unused_subtree(core);
...
return 0;
}

late_initcall_sync(clk_disable_unused);

so if there are two clocks A and B, A is the parent of B, and A is marked as CLK_IGNORE_UNUSED.

in boot stage if there is nobody using A and B, Linux will disable B due to clk_disable_unused() , but keep A being enabled since A has CLK_IGNORE_UNUSED.

but in Linux runtime, we might frequently disable /enable B in runtime power management, this will cause A disabled since Linux will not check CLK_IGNORE_UNUSED for runtime disabling clk .

Quoted 6 lines

so this makes CLK_IGNORE_UNUSE only work if we don't disable its
sub-clock at runtime. this looks making no sense.

i am thinking whether we should do some changes to make it have side
affect for runtime clk disable. otherwise, why do we want to stop it from being disabled during boot stage?

This is one of this problems, along with some others that this set aims to solve.

Extending CLK_IGNORE_UNUSED is not a good idea. In fact, if we can phase it out completely, that will be a good thing.

Since this set Mike has submitted an alternitive solution.

Please see: https://groups.google.com/forum/#!msg/linux.kernel/kX_nWSsWRxU/IZSjhG5Ed4oJ

Quoted 62 lines

I am not sure whether i missed something in clk core level support.

-barry

Quoted 58 lines

---
 drivers/clk/clk-conf.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

drivers/clk/clk-conf.c +39 -0

index aad4796..f83c1c2 100644
@@ -116,6 +116,45 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
        return 0;
 }
 
+static int __set_critical_clocks(struct device_node *node, bool clk_supplier)
+{
+       struct of_phandle_args clkspec;
+       struct clk *clk;
+       struct property *prop;
+       const __be32 *cur;
+       uint32_t index;
+       int ret;
+
+       if (!clk_supplier)
+               return 0;
+
+       of_property_for_each_u32(node, "critical-clock", prop, cur, index) {
+               clkspec.np = node;
+               clkspec.args_count = 1;
+               clkspec.args[0] = index;
+
+               clk = of_clk_get_from_provider(&clkspec);
+               if (IS_ERR(clk)) {
+                       pr_err("clk: couldn't get clock %u for %s\n",
+                               index, node->full_name);
+                       return PTR_ERR(clk);
+               }
+
+               clk_init_critical(clk);
+
+               ret = clk_prepare_enable(clk);
+               if (ret) {
+                       pr_err("Failed to enable clock %u for %s: %d\n",
+                              index, node->full_name, ret);
+                       return ret;
+               }
+
+               pr_debug("Setting clock as critical %pC\n", clk);
+       }
+
+       return 0;
+}
+
 /**
  * of_clk_set_defaults() - parse and set assigned clocks configuration
  * @node: device node to apply clock settings for

@@ -139,6 +178,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
        if (rc < 0)
                return rc;
 

Quoted 8 lines

drivers/clk/clk-conf.c int of_clk_set_defaults(struct device_node *node, bool clk_supplier) · line 187

Quoted 8 lines

drivers/clk/clk-conf.c +5 -1

-       return __set_clk_rates(node, clk_supplier);
+       rc = __set_clk_rates(node, clk_supplier);
+       if (rc < 0)
+               return rc;
+
+       return __set_critical_clocks(node, clk_supplier);
 }
 EXPORT_SYMBOL_GPL(of_clk_set_defaults);

--

Signed-off-by Lee Jones <[email protected]>

---
 .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/clock/clock-bindings.txt +15 -0

index 06fc6d5..4137034 100644
@@ -44,6 +44,45 @@ For example:
   clocks by index. The names should reflect the clock output signal
   names for the device.
 
+critical-clock:	Some hardware contains bunches of clocks which, in normal
+		circumstances, must never be turned off.  If drivers a) fail to
+		obtain a reference to any of these or b) give up a previously
+		obtained reference during suspend, it is possible that some
+		Operating Systems might attempt to disable them to save power.
+		If this happens a platform can fail irrecoverably as a result.
+		Usually the only way to recover from these failures is to
+		reboot.
+
+		To avoid either of these two scenarios from catastrophically
+		disabling an otherwise perfectly healthy running system,
+		clocks can be identified as 'critical' using this property from
+		inside a clocksource's node.
+
+		This property is not to be abused.  It is only to be used to

Hi Lee,

Quoted 34 lines · Lee Jones

Signed-off-by Lee Jones <[email protected]>

---
 .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/clock/clock-bindings.txt +21 -0

index 06fc6d5..4137034 100644
@@ -44,6 +44,45 @@ For example:
   clocks by index. The names should reflect the clock output signal
   names for the device.
 
+critical-clock:	Some hardware contains bunches of clocks which, in normal
+		circumstances, must never be turned off.  If drivers a) fail to
+		obtain a reference to any of these or b) give up a previously
+		obtained reference during suspend, it is possible that some
+		Operating Systems might attempt to disable them to save power.
+		If this happens a platform can fail irrecoverably as a result.
+		Usually the only way to recover from these failures is to
+		reboot.
+
+		To avoid either of these two scenarios from catastrophically
+		disabling an otherwise perfectly healthy running system,
+		clocks can be identified as 'critical' using this property from
+		inside a clocksource's node.
+
+		This property is not to be abused.  It is only to be used to
+		protect platforms from being crippled by gated clocks, NOT as a
+		convenience function to avoid using the framework correctly
+		inside device drivers.
+
+		Expected values are hardware clock indices.  If the
+		clock-indices property (see below) is used, then supplied

Documentation/devicetree/bindings/clock/clock-bindings.txt For example: · line 71

Quoted 4 lines

Documentation/devicetree/bindings/clock/clock-bindings.txt +4 -0

+		values must correspond to one of the listed identifiers.
+		Using the clock-indices example below, hardware clock <2>
+		is missing, therefore it is considered invalid to then
+		list clock <2> as a critical clock.

I think we should also consider having it simply as a boolean. Using indices for clocks that don't have any (for example because it only provides a single clock) seem to not really make much sense.

Also, since you can have a bunch of them, using critical-clocks seem more appropriate.

Quoted 37 lines · Maxime Ripard

Hi Lee,

Quoted 34 lines · Lee Jones

Signed-off-by Lee Jones <[email protected]>

---
 .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/clock/clock-bindings.txt +21 -0

index 06fc6d5..4137034 100644
@@ -44,6 +44,45 @@ For example:
   clocks by index. The names should reflect the clock output signal
   names for the device.
 
+critical-clock:	Some hardware contains bunches of clocks which, in normal
+		circumstances, must never be turned off.  If drivers a) fail to
+		obtain a reference to any of these or b) give up a previously
+		obtained reference during suspend, it is possible that some
+		Operating Systems might attempt to disable them to save power.
+		If this happens a platform can fail irrecoverably as a result.
+		Usually the only way to recover from these failures is to
+		reboot.
+
+		To avoid either of these two scenarios from catastrophically
+		disabling an otherwise perfectly healthy running system,
+		clocks can be identified as 'critical' using this property from
+		inside a clocksource's node.
+
+		This property is not to be abused.  It is only to be used to
+		protect platforms from being crippled by gated clocks, NOT as a
+		convenience function to avoid using the framework correctly
+		inside device drivers.
+
+		Expected values are hardware clock indices.  If the
+		clock-indices property (see below) is used, then supplied

Quoted 8 lines

Documentation/devicetree/bindings/clock/clock-bindings.txt For example: · line 71

Quoted 4 lines

Documentation/devicetree/bindings/clock/clock-bindings.txt +4 -0

+		values must correspond to one of the listed identifiers.
+		Using the clock-indices example below, hardware clock <2>
+		is missing, therefore it is considered invalid to then
+		list clock <2> as a critical clock.

I think we should also consider having it simply as a boolean. Using indices for clocks that don't have any (for example because it only provides a single clock) seem to not really make much sense.

Then how would you distinguish between the clocks if the provider provides more than a single clock?

Quoted 48 lines · Lee Jones

Quoted 37 lines · Maxime Ripard

Hi Lee,

Quoted 34 lines · Lee Jones

Signed-off-by Lee Jones <[email protected]>

---
 .../devicetree/bindings/clock/clock-bindings.txt   | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/clock/clock-bindings.txt +21 -0

index 06fc6d5..4137034 100644
@@ -44,6 +44,45 @@ For example:
   clocks by index. The names should reflect the clock output signal
   names for the device.
 
+critical-clock:	Some hardware contains bunches of clocks which, in normal
+		circumstances, must never be turned off.  If drivers a) fail to
+		obtain a reference to any of these or b) give up a previously
+		obtained reference during suspend, it is possible that some
+		Operating Systems might attempt to disable them to save power.
+		If this happens a platform can fail irrecoverably as a result.
+		Usually the only way to recover from these failures is to
+		reboot.
+
+		To avoid either of these two scenarios from catastrophically
+		disabling an otherwise perfectly healthy running system,
+		clocks can be identified as 'critical' using this property from
+		inside a clocksource's node.
+
+		This property is not to be abused.  It is only to be used to
+		protect platforms from being crippled by gated clocks, NOT as a
+		convenience function to avoid using the framework correctly
+		inside device drivers.
+
+		Expected values are hardware clock indices.  If the
+		clock-indices property (see below) is used, then supplied

Quoted 8 lines

Documentation/devicetree/bindings/clock/clock-bindings.txt For example: · line 71

Quoted 4 lines

Documentation/devicetree/bindings/clock/clock-bindings.txt +4 -0

+		values must correspond to one of the listed identifiers.
+		Using the clock-indices example below, hardware clock <2>
+		is missing, therefore it is considered invalid to then
+		list clock <2> as a critical clock.

I think we should also consider having it simply as a boolean. Using indices for clocks that don't have any (for example because it only provides a single clock) seem to not really make much sense.

Quoted 2 lines

Then how would you distinguish between the clocks if the provider provides more than a single clock?

What I had in mind was that, you would have three cases:

- critical-clocks is not there: no clocks are made critical

- critical-clocks is there, but doesn't have any values: all the clocks provided are marked critical

- critical-clocks is there and it has a list of values: only the clocks listed are marked critical.

Does that make sense to you?

Lots of platforms contain clocks which if turned off would prove fatal. The only way to recover from these catastrophic failures is to restart the board(s). Now, when a clock provider is registered with the framework it is possible for a list of critical clocks to be supplied which must be kept ungated. Each clock mentioned in the newly introduced 'critical-clock' property will be clk_prepare_enable()d where the normal references will be taken. This will prevent the common clk framework from attempting to gate them during the normal clk_disable_unused() and disable_clock() procedures.

Note that it will still be possible for knowledgeable drivers to turn these clocks off using clk_{enable,disable}_critical() calls.

Changelog:
v6 => v7:
- Introduce API to enable and disable critical clocks
- Rename 'always-on-clock' to 'critical-clock'

v5 => v6:
- Use of_clk_get_from_provider() instead of of_clk_get_by_clkspec()
- Explicitly describe expected DT values
- Be pedantic with regards to printk() format specifiers

vX => v5:
Implementations have changed drastically between versions, thus I would like for this set to be thought of independently from its predecessors. The only reason for identifying as 'v5' is ease of differentiation on the list, which stems from the confusion caused by submitting 'v4' as a separate entity.

Lee Jones (5):
ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
ARM: sti: stih410-clocks: Identify critical clocks
clk: Supply the critical clock {init, enable, disable} framework
clk: Provide critical clock support
clk: dt: Introduce binding for critical clock support

 .../devicetree/bindings/clock/clock-bindings.txt   | 39 +++++++++++++++++++
 arch/arm/boot/dts/stih410-clock.dtsi               | 10 +++++
 drivers/clk/clk-conf.c                             | 45 +++++++++++++++++++++-
 drivers/clk/clk.c                                  | 45 ++++++++++++++++++++++
 include/dt-bindings/clock/stih407-clks.h           |  4 ++
 include/linux/clk-provider.h                       |  2 +
 include/linux/clk.h                                | 30 +++++++++++++++
 7 files changed, 174 insertions(+), 1 deletion(-)

Signature · 1 line

1.9.1

Quoting Lee Jones (2015-07-22 06:04:10)

Quoted 10 lines

Lots of platforms contain clocks which if turned off would prove fatal. The only way to recover from these catastrophic failures is to restart the board(s). Now, when a clock provider is registered with the framework it is possible for a list of critical clocks to be supplied which must be kept ungated. Each clock mentioned in the newly introduced 'critical-clock' property will be clk_prepare_enable()d where the normal references will be taken. This will prevent the common clk framework from attempting to gate them during the normal clk_disable_unused() and disable_clock() procedures.

Quoted 2 lines

Note that it will still be possible for knowledgeable drivers to turn these clocks off using clk_{enable,disable}_critical() calls.

Hi Lee,

Thanks for submitting the series.

It has been a little while since v6. It would be helpful to remind me of why a new api is necessary, versus using the existing clk_prepare_unused call at the time that a clock is enabled.

I'm looking over the patches in detail now, but one question stands out:
besides the DT use case, would a Linux device driver ever call clk_enable_critical instead of using clk_enable? If so, why?

Quoted 33 lines

Changelog:
v6 => v7:
- Introduce API to enable and disable critical clocks
- Rename 'always-on-clock' to 'critical-clock'

v5 => v6:
- Use of_clk_get_from_provider() instead of of_clk_get_by_clkspec()
- Explicitly describe expected DT values
- Be pedantic with regards to printk() format specifiers

vX => v5:
Implementations have changed drastically between versions, thus I would like for this set to be thought of independently from its predecessors. The only reason for identifying as 'v5' is ease of differentiation on the list, which stems from the confusion caused by submitting 'v4' as a separate entity.

Lee Jones (5):
ARM: sti: stih407-family: Supply defines for CLOCKGEN A0
ARM: sti: stih410-clocks: Identify critical clocks
clk: Supply the critical clock {init, enable, disable} framework
clk: Provide critical clock support
clk: dt: Introduce binding for critical clock support

 .../devicetree/bindings/clock/clock-bindings.txt   | 39 +++++++++++++++++++
 arch/arm/boot/dts/stih410-clock.dtsi               | 10 +++++
 drivers/clk/clk-conf.c                             | 45 +++++++++++++++++++++-
 drivers/clk/clk.c                                  | 45 ++++++++++++++++++++++
 include/dt-bindings/clock/stih407-clks.h           |  4 ++
 include/linux/clk-provider.h                       |  2 +
 include/linux/clk.h                                | 30 +++++++++++++++
 7 files changed, 174 insertions(+), 1 deletion(-)