Re: [PATCH 1/2] clk: sunxi-ng: Fix CPUX clock for the A23/A33

Re: [PATCH 1/2] clk: sunxi-ng: Fix CPUX clock for the A23/A33

[Icenowy Zheng]

CjIwMTblubQxMeaciDfml6UgMTU6NTTkuo4gTWF4aW1lIFJpcGFyZCA8bWF4aW1lLnJpcGFyZEBm
cmVlLWVsZWN0cm9ucy5jb20+5YaZ6YGT77yaCj4KPiBIaSwgCj4KPiBPbiBNb24sIE5vdiAwNywg
MjAxNiBhdCAwMToyOTozMUFNICswODAwLCBJY2Vub3d5IFpoZW5nIHdyb3RlOiAKPiA+IFRoZSBD
UFVYIGNsb2NrIG9mIEEyMy8zMyBDQ1UgZHJpdmVyIGZvcmdvdCB0byBhZGQgQ0xLX1NFVF9SQVRF
X1BBUkVOVCAKPiA+IGZsYWcsIHdoaWNoIG1ha2VzIGl0cyBmcmVxdWVuY3kgZmFpbCB0byBjaGFu
Z2UgKGFzIHBhcnQgb2YgY3B1IHRoZXJtYWwgCj4gPiB0aHJvdHRsZSkuIAo+ID4gCj4gPiBGaXgg
aXQgYnkgYWRkIHRoaXMgZmxhZy4gCj4KPiBZb3VyIGNvbW1pdCB0aXRsZSBhbmQgbG9nIGNvdWxk
IGJlIGJldHRlci4gCj4KPiBUaGUgdGl0bGUgdXN1YWxseSBpcyBtb3JlIGFib3V0IHdoYXQgaXMg
ZG9uZSBpbiB0aGUgcGF0Y2ggaXRzZWxmLCAKPiBzb21ldGhpbmcgbGlrZSAiY2xrOiBzdW54aS1u
ZzogQWxsb3cgdG8gY2hhbmdlIHRoZSBwYXJlbnQgcmF0ZSBmb3IgdGhlIAo+IENQVSBjbG9ja3Mi
IGluIHRoaXMgY2FzZS4gCgpJJ20gYSBjbG9jayBuZXdiaWUgYW5kIGhhdmUganVzdCBzdGFydGVk
IHRvIGV4cGxvcmUgdGhlIHN1YnN5c3RlbS4uLiBTb3JyeSBidXQgSSB3aWxsIGRvIGJldHRlciBp
biB2MiA6LSkKCj4KPiBBbmQgeW91ciBjb21taXQgbG9nIHNob3VsZCBleHBsYWluIHdoeSBpdCBp
cyBhbiBpc3N1ZS4gWW91cnMgaXMgZXZlbiAKPiB3cm9uZyBoZXJlLCB3ZSBjb3VsZCBkZWZpbml0
ZWx5IGNoYW5nZSB0aGUgcmF0ZSBvZiB0aGVzZSBjbG9ja3MgCj4gYWxyZWFkeS4gVGhlIG9ubHkg
dGhpbmcgdGhhdCB3YXMgbm90IGFsbG93ZWQgd2FzIHRvIGNoYW5nZSB0aGUgcmF0ZSBvZiAKPiBp
dHMgcGFyZW50cywgd2hpY2ggaXMgd2hhdCB0aGlzIHBhdGNoIGZpeGVzLiAKPgo+ID4gU2lnbmVk
LW9mZi1ieTogSWNlbm93eSBaaGVuZyA8aWNlbm93eUBhb3NjLnh5ej4gCj4gPiAtLS0gCj4gPiBQ
YXRjaCA0LjktcmMgdG9vLiAKPgo+IEkgZG9uJ3Qgc2VlIHdoeSBpdCBzaG91bGQgYmUgcGFydCBv
ZiA0LjkuIE5vIG9uZSB1c2VzIHRoYXQgY29kZSBpbiA0LjkuIAoKSXQgY2FuIGJlIHRyaWdnZXJl
ZCBieSBhIG1vZGlmaWVkIGR0IGluIDQuOSwgYnkgYWRkIG9wZXJhdGluZyBwb2ludHMuCgo+Cj4g
VGhhbmtzISAKPiBNYXhpbWUgCj4KPiAtLSAKPiBNYXhpbWUgUmlwYXJkLCBGcmVlIEVsZWN0cm9u
cyAKPiBFbWJlZGRlZCBMaW51eCBhbmQgS2VybmVsIGVuZ2luZWVyaW5nIAo+IGh0dHA6Ly9mcmVl
LWVsZWN0cm9ucy5jb20gCg==

Re: [PATCH 1/2] clk: sunxi-ng: Fix CPUX clock for the A23/A33

[Maxime Ripard]

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

On Mon, Nov 07, 2016 at 05:36:03PM +0800, Icenowy Zheng wrote:
> > And your commit log should explain why it is an issue. Yours is even 
> > wrong here, we could definitely change the rate of these clocks 
> > already. The only thing that was not allowed was to change the rate of 
> > its parents, which is what this patch fixes. 
> >
> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> 
> > > --- 
> > > Patch 4.9-rc too. 
> >
> > I don't see why it should be part of 4.9. No one uses that code in 4.9. 
> 
> It can be triggered by a modified dt in 4.9, by add operating points.

Still, there's no reason to fix something that is not used by anyone,
it can definitely wait until the next release. Linus also said that he
didn't want patches like that last week.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 1/2] clk: sunxi-ng: Fix CPUX clock for the A23/A33

[Maxime Ripard]

On Mon, Nov 07, 2016 at 05:36:03PM +0800, Icenowy Zheng wrote:
> > And your commit log should explain why it is an issue. Yours is even 
> > wrong here, we could definitely change the rate of these clocks 
> > already. The only thing that was not allowed was to change the rate of 
> > its parents, which is what this patch fixes. 
> >
> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> 
> > > --- 
> > > Patch 4.9-rc too. 
> >
> > I don't see why it should be part of 4.9. No one uses that code in 4.9. 
> 
> It can be triggered by a modified dt in 4.9, by add operating points.

Still, there's no reason to fix something that is not used by anyone,
it can definitely wait until the next release. Linus also said that he
didn't want patches like that last week.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161108/fed056dc/attachment.sig>

Re: [PATCH 1/2] clk: sunxi-ng: Fix CPUX clock for the A23/A33

[Maxime Ripard]

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

Hi,

On Mon, Nov 07, 2016 at 01:29:31AM +0800, Icenowy Zheng wrote:
> The CPUX clock of A23/33 CCU driver forgot to add CLK_SET_RATE_PARENT
> flag, which makes its frequency fail to change (as part of cpu thermal
> throttle).
> 
> Fix it by add this flag.

Your commit title and log could be better.

The title usually is more about what is done in the patch itself,
something like "clk: sunxi-ng: Allow to change the parent rate for the
CPU clocks" in this case.

And your commit log should explain why it is an issue. Yours is even
wrong here, we could definitely change the rate of these clocks
already. The only thing that was not allowed was to change the rate of
its parents, which is what this patch fixes.

> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> Patch 4.9-rc too.

I don't see why it should be part of 4.9. No one uses that code in 4.9.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 1/2] clk: sunxi-ng: Fix CPUX clock for the A23/A33

[Maxime Ripard]

Hi,

On Mon, Nov 07, 2016 at 01:29:31AM +0800, Icenowy Zheng wrote:
> The CPUX clock of A23/33 CCU driver forgot to add CLK_SET_RATE_PARENT
> flag, which makes its frequency fail to change (as part of cpu thermal
> throttle).
> 
> Fix it by add this flag.

Your commit title and log could be better.

The title usually is more about what is done in the patch itself,
something like "clk: sunxi-ng: Allow to change the parent rate for the
CPU clocks" in this case.

And your commit log should explain why it is an issue. Yours is even
wrong here, we could definitely change the rate of these clocks
already. The only thing that was not allowed was to change the rate of
its parents, which is what this patch fixes.

> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> Patch 4.9-rc too.

I don't see why it should be part of 4.9. No one uses that code in 4.9.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161107/46d235a8/attachment.sig>

[PATCH 1/2] clk: sunxi-ng: Fix CPUX clock for the A23/A33

[Icenowy Zheng]

The CPUX clock of A23/33 CCU driver forgot to add CLK_SET_RATE_PARENT
flag, which makes its frequency fail to change (as part of cpu thermal
throttle).

Fix it by add this flag.

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
Patch 4.9-rc too.
 drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 2 +-
 drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
index 2646d98..44c4775 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
@@ -163,7 +163,7 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_de_clk, "pll-de",
 static const char * const cpux_parents[] = { "osc32k", "osc24M",
 					     "pll-cpux" , "pll-cpux" };
 static SUNXI_CCU_MUX(cpux_clk, "cpux", cpux_parents,
-		     0x050, 16, 2, CLK_IS_CRITICAL);
+		     0x050, 16, 2, CLK_IS_CRITICAL | CLK_SET_RATE_PARENT);
 
 static SUNXI_CCU_M(axi_clk, "axi", "cpux", 0x050, 0, 2, 0);
 
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
index 96b40ca..59cfdc8 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
@@ -170,7 +170,7 @@ static SUNXI_CCU_N_WITH_GATE_LOCK(pll_ddr1_clk, "pll-ddr1",
 static const char * const cpux_parents[] = { "osc32k", "osc24M",
 					     "pll-cpux" , "pll-cpux" };
 static SUNXI_CCU_MUX(cpux_clk, "cpux", cpux_parents,
-		     0x050, 16, 2, CLK_IS_CRITICAL);
+		     0x050, 16, 2, CLK_IS_CRITICAL | CLK_SET_RATE_PARENT);
 
 static SUNXI_CCU_M(axi_clk, "axi", "cpux", 0x050, 0, 2, 0);
 
-- 
2.10.1

[PATCH 1/2] clk: sunxi-ng: Fix CPUX clock for the A23/A33

[Icenowy Zheng]

The CPUX clock of A23/33 CCU driver forgot to add CLK_SET_RATE_PARENT
flag, which makes its frequency fail to change (as part of cpu thermal
throttle).

Fix it by add this flag.

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
Patch 4.9-rc too.
 drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 2 +-
 drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
index 2646d98..44c4775 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
@@ -163,7 +163,7 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_de_clk, "pll-de",
 static const char * const cpux_parents[] = { "osc32k", "osc24M",
 					     "pll-cpux" , "pll-cpux" };
 static SUNXI_CCU_MUX(cpux_clk, "cpux", cpux_parents,
-		     0x050, 16, 2, CLK_IS_CRITICAL);
+		     0x050, 16, 2, CLK_IS_CRITICAL | CLK_SET_RATE_PARENT);
 
 static SUNXI_CCU_M(axi_clk, "axi", "cpux", 0x050, 0, 2, 0);
 
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
index 96b40ca..59cfdc8 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
@@ -170,7 +170,7 @@ static SUNXI_CCU_N_WITH_GATE_LOCK(pll_ddr1_clk, "pll-ddr1",
 static const char * const cpux_parents[] = { "osc32k", "osc24M",
 					     "pll-cpux" , "pll-cpux" };
 static SUNXI_CCU_MUX(cpux_clk, "cpux", cpux_parents,
-		     0x050, 16, 2, CLK_IS_CRITICAL);
+		     0x050, 16, 2, CLK_IS_CRITICAL | CLK_SET_RATE_PARENT);
 
 static SUNXI_CCU_M(axi_clk, "axi", "cpux", 0x050, 0, 2, 0);
 
-- 
2.10.1