[PATCH] clk-divider: make sure read-only dividers do not write to their register

[PATCH] clk-divider: make sure read-only dividers do not write to their register

[Heiko Stuebner]

Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
the special ops struct for read-only clocks and instead opted to handle
them regularly.

On the rk3368 this results in breakage as aclkm now gets set a value.
While it is the same divider, the A53 core still doesn't like it.
The reason being that "ACLKENMasserts one clock cycle before the rising
edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
is asserted.

So make sure, read-only clocks don't touch the clock-register at all
even if only writing the same value.

Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
Reported-by: Zhang Qing <zhangqing@rock-chips.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk-divider.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index ded3ff4..6dfe261 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -391,6 +391,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags = 0;
 	u32 val;
 
+	/* don't do writes in read-only case */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY)
+		return 0;
+
 	value = divider_get_val(rate, parent_rate, divider->table,
 				divider->width, divider->flags);
 
-- 
2.6.4

[PATCH] clk-divider: make sure read-only dividers do not write to their register

[Heiko Stuebner]

Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
the special ops struct for read-only clocks and instead opted to handle
them regularly.

On the rk3368 this results in breakage as aclkm now gets set a value.
While it is the same divider, the A53 core still doesn't like it.
The reason being that "ACLKENMasserts one clock cycle before the rising
edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
is asserted.

So make sure, read-only clocks don't touch the clock-register at all
even if only writing the same value.

Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
Reported-by: Zhang Qing <zhangqing@rock-chips.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk-divider.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index ded3ff4..6dfe261 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -391,6 +391,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags = 0;
 	u32 val;
 
+	/* don't do writes in read-only case */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY)
+		return 0;
+
 	value = divider_get_val(rate, parent_rate, divider->table,
 				divider->width, divider->flags);
 
-- 
2.6.4

Re: [PATCH] clk-divider: make sure read-only dividers do not write to their register

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

On Wed, Jan 20, 2016 at 03:42:54PM +0100, Heiko Stuebner wrote:
> Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
> the special ops struct for read-only clocks and instead opted to handle
> them regularly.
> 
> On the rk3368 this results in breakage as aclkm now gets set a value.
> While it is the same divider, the A53 core still doesn't like it.
> The reason being that "ACLKENMasserts one clock cycle before the rising
> edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
> is asserted.
> 
> So make sure, read-only clocks don't touch the clock-register at all
> even if only writing the same value.
> 
> Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> Reported-by: Zhang Qing <zhangqing@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

It wouldn't hurt to clarify in the comment that this isn't just an
optimisation, and that even writing the same value to the hardware may
not be safe.

In any case
Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> ---
>  drivers/clk/clk-divider.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index ded3ff4..6dfe261 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -391,6 +391,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
>  	unsigned long flags = 0;
>  	u32 val;
>  
> +	/* don't do writes in read-only case */
> +	if (divider->flags & CLK_DIVIDER_READ_ONLY)
> +		return 0;
> +
>  	value = divider_get_val(rate, parent_rate, divider->table,
>  				divider->width, divider->flags);
>  
> -- 
> 2.6.4
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] clk-divider: make sure read-only dividers do not write to their register

[James Hogan]

On Wed, Jan 20, 2016 at 03:42:54PM +0100, Heiko Stuebner wrote:
> Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
> the special ops struct for read-only clocks and instead opted to handle
> them regularly.
> 
> On the rk3368 this results in breakage as aclkm now gets set a value.
> While it is the same divider, the A53 core still doesn't like it.
> The reason being that "ACLKENMasserts one clock cycle before the rising
> edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
> is asserted.
> 
> So make sure, read-only clocks don't touch the clock-register at all
> even if only writing the same value.
> 
> Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> Reported-by: Zhang Qing <zhangqing@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

It wouldn't hurt to clarify in the comment that this isn't just an
optimisation, and that even writing the same value to the hardware may
not be safe.

In any case
Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> ---
>  drivers/clk/clk-divider.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index ded3ff4..6dfe261 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -391,6 +391,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
>  	unsigned long flags = 0;
>  	u32 val;
>  
> +	/* don't do writes in read-only case */
> +	if (divider->flags & CLK_DIVIDER_READ_ONLY)
> +		return 0;
> +
>  	value = divider_get_val(rate, parent_rate, divider->table,
>  				divider->width, divider->flags);
>  
> -- 
> 2.6.4
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160120/b6fd5394/attachment.sig>

Re: [PATCH] clk-divider: make sure read-only dividers do not write to their register

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

On Wed, Jan 20, 2016 at 03:42:54PM +0100, Heiko Stuebner wrote:
> Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
> the special ops struct for read-only clocks and instead opted to handle
> them regularly.
> 
> On the rk3368 this results in breakage as aclkm now gets set a value.
> While it is the same divider, the A53 core still doesn't like it.
> The reason being that "ACLKENMasserts one clock cycle before the rising
> edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
> is asserted.
> 
> So make sure, read-only clocks don't touch the clock-register at all
> even if only writing the same value.
> 
> Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> Reported-by: Zhang Qing <zhangqing@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

It wouldn't hurt to clarify in the comment that this isn't just an
optimisation, and that even writing the same value to the hardware may
not be safe.

In any case
Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> ---
>  drivers/clk/clk-divider.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index ded3ff4..6dfe261 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -391,6 +391,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
>  	unsigned long flags = 0;
>  	u32 val;
>  
> +	/* don't do writes in read-only case */
> +	if (divider->flags & CLK_DIVIDER_READ_ONLY)
> +		return 0;
> +
>  	value = divider_get_val(rate, parent_rate, divider->table,
>  				divider->width, divider->flags);
>  
> -- 
> 2.6.4
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2] clk-divider: make sure read-only dividers do not write to their register

[Heiko Stuebner]

Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
the special ops struct for read-only clocks and instead opted to handle
them regularly.

On the rk3368 this results in breakage as aclkm now gets set a value.
While it is the same divider value, the A53 core still doesn't like it,
which can result in the cpu ending up in a hang.
The reason being that "ACLKENMasserts one clock cycle before the rising
edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
is asserted.

So make sure read-only clocks don't touch the clock-register at all
even if only writing the same value, as even writing the same value
may not be safe in all cases.

Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
Reported-by: Zhang Qing <zhangqing@rock-chips.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
changes in v2:
- emphasize that even setting the same divider may be unsafe

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

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index ded3ff4..6dfe261 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -391,6 +391,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags = 0;
 	u32 val;
 
+	/* don't do writes in read-only case, as it may be unsafe */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY)
+		return 0;
+
 	value = divider_get_val(rate, parent_rate, divider->table,
 				divider->width, divider->flags);
 
-- 
2.6.4

[PATCH v2] clk-divider: make sure read-only dividers do not write to their register

[Heiko Stuebner]

Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
the special ops struct for read-only clocks and instead opted to handle
them regularly.

On the rk3368 this results in breakage as aclkm now gets set a value.
While it is the same divider value, the A53 core still doesn't like it,
which can result in the cpu ending up in a hang.
The reason being that "ACLKENMasserts one clock cycle before the rising
edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
is asserted.

So make sure read-only clocks don't touch the clock-register at all
even if only writing the same value, as even writing the same value
may not be safe in all cases.

Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
Reported-by: Zhang Qing <zhangqing@rock-chips.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
changes in v2:
- emphasize that even setting the same divider may be unsafe

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

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index ded3ff4..6dfe261 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -391,6 +391,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags = 0;
 	u32 val;
 
+	/* don't do writes in read-only case, as it may be unsafe */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY)
+		return 0;
+
 	value = divider_get_val(rate, parent_rate, divider->table,
 				divider->width, divider->flags);
 
-- 
2.6.4

[PATCH v2] clk-divider: make sure read-only dividers do not write to their register

[Heiko Stuebner]

Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
the special ops struct for read-only clocks and instead opted to handle
them regularly.

On the rk3368 this results in breakage as aclkm now gets set a value.
While it is the same divider value, the A53 core still doesn't like it,
which can result in the cpu ending up in a hang.
The reason being that "ACLKENMasserts one clock cycle before the rising
edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
is asserted.

So make sure read-only clocks don't touch the clock-register at all
even if only writing the same value, as even writing the same value
may not be safe in all cases.

Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
Reported-by: Zhang Qing <zhangqing-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Reviewed-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
changes in v2:
- emphasize that even setting the same divider may be unsafe

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

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index ded3ff4..6dfe261 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -391,6 +391,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags = 0;
 	u32 val;
 
+	/* don't do writes in read-only case, as it may be unsafe */
+	if (divider->flags & CLK_DIVIDER_READ_ONLY)
+		return 0;
+
 	value = divider_get_val(rate, parent_rate, divider->table,
 				divider->width, divider->flags);
 
-- 
2.6.4

Re: [PATCH v2] clk-divider: make sure read-only dividers do not write to their register

[Stephen Boyd]

On 01/20, Heiko Stuebner wrote:
> Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
> the special ops struct for read-only clocks and instead opted to handle
> them regularly.
> 
> On the rk3368 this results in breakage as aclkm now gets set a value.
> While it is the same divider value, the A53 core still doesn't like it,
> which can result in the cpu ending up in a hang.
> The reason being that "ACLKENMasserts one clock cycle before the rising
> edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
> is asserted.
> 
> So make sure read-only clocks don't touch the clock-register at all
> even if only writing the same value, as even writing the same value
> may not be safe in all cases.
> 
> Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> Reported-by: Zhang Qing <zhangqing@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: James Hogan <james.hogan@imgtec.com>
> ---

Hmph, the same patch[1] has been sitting in my inbox for months
now but I never got a response to my last question and then
forgot about it. I'll repeat the question here and hopefully we
can finish the discussion.

Maybe it would make more sense to have different ops for read
only dividers? The clk_set_rate op would be empty, while we would
have proper recalc_rate and round_rate ops. We can't just flat
out revert commit e6d5e7d90be9 (clk-divider: Fix READ_ONLY when
divider > 1, 2014-11-14) because that will introduce the problem
that it was fixing, but as long as we implement round_rate in
addition to recalc_rate it should work.

[1] http://lkml.kernel.org/r/1428392806-14538-1-git-send-email-jy0922.shim@samsung.com

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2] clk-divider: make sure read-only dividers do not write to their register

[Stephen Boyd]

On 01/20, Heiko Stuebner wrote:
> Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1") removed
> the special ops struct for read-only clocks and instead opted to handle
> them regularly.
> 
> On the rk3368 this results in breakage as aclkm now gets set a value.
> While it is the same divider value, the A53 core still doesn't like it,
> which can result in the cpu ending up in a hang.
> The reason being that "ACLKENMasserts one clock cycle before the rising
> edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
> is asserted.
> 
> So make sure read-only clocks don't touch the clock-register at all
> even if only writing the same value, as even writing the same value
> may not be safe in all cases.
> 
> Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> Reported-by: Zhang Qing <zhangqing@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: James Hogan <james.hogan@imgtec.com>
> ---

Hmph, the same patch[1] has been sitting in my inbox for months
now but I never got a response to my last question and then
forgot about it. I'll repeat the question here and hopefully we
can finish the discussion.

Maybe it would make more sense to have different ops for read
only dividers? The clk_set_rate op would be empty, while we would
have proper recalc_rate and round_rate ops. We can't just flat
out revert commit e6d5e7d90be9 (clk-divider: Fix READ_ONLY when
divider > 1, 2014-11-14) because that will introduce the problem
that it was fixing, but as long as we implement round_rate in
addition to recalc_rate it should work.

[1] http://lkml.kernel.org/r/1428392806-14538-1-git-send-email-jy0922.shim at samsung.com

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] clk-divider: make sure read-only dividers do not write to their register

[Heiko Stuebner]

Am Mittwoch, 20. Januar 2016, 14:16:56 schrieb Stephen Boyd:
> On 01/20, Heiko Stuebner wrote:
> > Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> > removed the special ops struct for read-only clocks and instead opted
> > to handle them regularly.
> > 
> > On the rk3368 this results in breakage as aclkm now gets set a value.
> > While it is the same divider value, the A53 core still doesn't like it,
> > which can result in the cpu ending up in a hang.
> > The reason being that "ACLKENMasserts one clock cycle before the rising
> > edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
> > is asserted.
> > 
> > So make sure read-only clocks don't touch the clock-register at all
> > even if only writing the same value, as even writing the same value
> > may not be safe in all cases.
> > 
> > Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> > Reported-by: Zhang Qing <zhangqing@rock-chips.com>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > Reviewed-by: James Hogan <james.hogan@imgtec.com>
> > ---
> 
> Hmph, the same patch[1] has been sitting in my inbox for months
> now but I never got a response to my last question and then
> forgot about it. I'll repeat the question here and hopefully we
> can finish the discussion.
> 
> Maybe it would make more sense to have different ops for read
> only dividers? The clk_set_rate op would be empty, while we would
> have proper recalc_rate and round_rate ops. We can't just flat
> out revert commit e6d5e7d90be9 (clk-divider: Fix READ_ONLY when
> divider > 1, 2014-11-14) because that will introduce the problem
> that it was fixing, but as long as we implement round_rate in
> addition to recalc_rate it should work.
> 
> [1]
> http://lkml.kernel.org/r/1428392806-14538-1-git-send-email-jy0922.shim@sa
> msung.com

Sorry for the second submission then. That really is from quite a while ago. 
Reintroducing a secondary ops-struct with a round-rate sounds fine for my 
side - I'll give that a shot tomorrow.


Heiko

[PATCH v2] clk-divider: make sure read-only dividers do not write to their register

[Heiko Stuebner]

Am Mittwoch, 20. Januar 2016, 14:16:56 schrieb Stephen Boyd:
> On 01/20, Heiko Stuebner wrote:
> > Commit e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> > removed the special ops struct for read-only clocks and instead opted
> > to handle them regularly.
> > 
> > On the rk3368 this results in breakage as aclkm now gets set a value.
> > While it is the same divider value, the A53 core still doesn't like it,
> > which can result in the cpu ending up in a hang.
> > The reason being that "ACLKENMasserts one clock cycle before the rising
> > edge of ACLKM" and the clock should only be touched when STANDBYWFIL2
> > is asserted.
> > 
> > So make sure read-only clocks don't touch the clock-register at all
> > even if only writing the same value, as even writing the same value
> > may not be safe in all cases.
> > 
> > Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> > Reported-by: Zhang Qing <zhangqing@rock-chips.com>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > Reviewed-by: James Hogan <james.hogan@imgtec.com>
> > ---
> 
> Hmph, the same patch[1] has been sitting in my inbox for months
> now but I never got a response to my last question and then
> forgot about it. I'll repeat the question here and hopefully we
> can finish the discussion.
> 
> Maybe it would make more sense to have different ops for read
> only dividers? The clk_set_rate op would be empty, while we would
> have proper recalc_rate and round_rate ops. We can't just flat
> out revert commit e6d5e7d90be9 (clk-divider: Fix READ_ONLY when
> divider > 1, 2014-11-14) because that will introduce the problem
> that it was fixing, but as long as we implement round_rate in
> addition to recalc_rate it should work.
> 
> [1]
> http://lkml.kernel.org/r/1428392806-14538-1-git-send-email-jy0922.shim at sa
> msung.com

Sorry for the second submission then. That really is from quite a while ago. 
Reintroducing a secondary ops-struct with a round-rate sounds fine for my 
side - I'll give that a shot tomorrow.


Heiko