[PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Chris Brandt]

The RZ/A1 is different than the other Renesas SOCs because the MSTP
registers are 8-bit instead of 32-bit and if you try writing values as
32-bit nothing happens...meaning this driver never worked for r7s72100.

Fixes: b6face404f38 ("ARM: shmobile: r7s72100: add essential clock nodes to dtsi")
Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v2:
* move r7s72100 detection inside cpg_mstp_clocks_init()
* change width_8bit flag from global to inside group struct
---
 drivers/clk/renesas/clk-mstp.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 9375777..b533f99 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -37,12 +37,14 @@
  * @smstpcr: module stop control register
  * @mstpsr: module stop status register (optional)
  * @lock: protects writes to SMSTPCR
+ * @width_8bit: registers are 8-bit, not 32-bit
  */
 struct mstp_clock_group {
 	struct clk_onecell_data data;
 	void __iomem *smstpcr;
 	void __iomem *mstpsr;
 	spinlock_t lock;
+	bool width_8bit;
 };
 
 /**
@@ -59,6 +61,18 @@ struct mstp_clock {
 
 #define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw)
 
+static inline u32 cpg_mstp_read(struct mstp_clock_group *group,
+				u32 __iomem *reg)
+{
+	return group->width_8bit ? readb(reg) : clk_readl(reg);
+}
+
+static inline void cpg_mstp_write(struct mstp_clock_group *group, u32 val,
+				  u32 __iomem *reg)
+{
+	group->width_8bit ? writeb(val, reg) : clk_writel(val, reg);
+}
+
 static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 {
 	struct mstp_clock *clock = to_mstp_clock(hw);
@@ -70,12 +84,12 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 
 	spin_lock_irqsave(&group->lock, flags);
 
-	value = clk_readl(group->smstpcr);
+	value = cpg_mstp_read(group, group->smstpcr);
 	if (enable)
 		value &= ~bitmask;
 	else
 		value |= bitmask;
-	clk_writel(value, group->smstpcr);
+	cpg_mstp_write(group, value, group->smstpcr);
 
 	spin_unlock_irqrestore(&group->lock, flags);
 
@@ -83,7 +97,7 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 		return 0;
 
 	for (i = 1000; i > 0; --i) {
-		if (!(clk_readl(group->mstpsr) & bitmask))
+		if (!(cpg_mstp_read(group, group->mstpsr) & bitmask))
 			break;
 		cpu_relax();
 	}
@@ -114,9 +128,9 @@ static int cpg_mstp_clock_is_enabled(struct clk_hw *hw)
 	u32 value;
 
 	if (group->mstpsr)
-		value = clk_readl(group->mstpsr);
+		value = cpg_mstp_read(group, group->mstpsr);
 	else
-		value = clk_readl(group->smstpcr);
+		value = cpg_mstp_read(group, group->smstpcr);
 
 	return !(value & BIT(clock->bit_index));
 }
@@ -188,6 +202,9 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 		return;
 	}
 
+	if (of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks"))
+		group->width_8bit = true;
+
 	for (i = 0; i < MSTP_MAX_CLOCKS; ++i)
 		clks[i] = ERR_PTR(-ENOENT);
 
-- 
2.10.1

Re: [PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Kuninori Morimoto]

Hi Chris

> The RZ/A1 is different than the other Renesas SOCs because the MSTP
> registers are 8-bit instead of 32-bit and if you try writing values as
> 32-bit nothing happens...meaning this driver never worked for r7s72100.
> 
> Fixes: b6face404f38 ("ARM: shmobile: r7s72100: add essential clock nodes to dtsi")
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---

Thanks. I looks nice for me

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

> v2:
> * move r7s72100 detection inside cpg_mstp_clocks_init()
> * change width_8bit flag from global to inside group struct
> ---
>  drivers/clk/renesas/clk-mstp.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
> index 9375777..b533f99 100644
> --- a/drivers/clk/renesas/clk-mstp.c
> +++ b/drivers/clk/renesas/clk-mstp.c
> @@ -37,12 +37,14 @@
>   * @smstpcr: module stop control register
>   * @mstpsr: module stop status register (optional)
>   * @lock: protects writes to SMSTPCR
> + * @width_8bit: registers are 8-bit, not 32-bit
>   */
>  struct mstp_clock_group {
>  	struct clk_onecell_data data;
>  	void __iomem *smstpcr;
>  	void __iomem *mstpsr;
>  	spinlock_t lock;
> +	bool width_8bit;
>  };
>  
>  /**
> @@ -59,6 +61,18 @@ struct mstp_clock {
>  
>  #define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw)
>  
> +static inline u32 cpg_mstp_read(struct mstp_clock_group *group,
> +				u32 __iomem *reg)
> +{
> +	return group->width_8bit ? readb(reg) : clk_readl(reg);
> +}
> +
> +static inline void cpg_mstp_write(struct mstp_clock_group *group, u32 val,
> +				  u32 __iomem *reg)
> +{
> +	group->width_8bit ? writeb(val, reg) : clk_writel(val, reg);
> +}
> +
>  static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
>  {
>  	struct mstp_clock *clock = to_mstp_clock(hw);
> @@ -70,12 +84,12 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
>  
>  	spin_lock_irqsave(&group->lock, flags);
>  
> -	value = clk_readl(group->smstpcr);
> +	value = cpg_mstp_read(group, group->smstpcr);
>  	if (enable)
>  		value &= ~bitmask;
>  	else
>  		value |= bitmask;
> -	clk_writel(value, group->smstpcr);
> +	cpg_mstp_write(group, value, group->smstpcr);
>  
>  	spin_unlock_irqrestore(&group->lock, flags);
>  
> @@ -83,7 +97,7 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
>  		return 0;
>  
>  	for (i = 1000; i > 0; --i) {
> -		if (!(clk_readl(group->mstpsr) & bitmask))
> +		if (!(cpg_mstp_read(group, group->mstpsr) & bitmask))
>  			break;
>  		cpu_relax();
>  	}
> @@ -114,9 +128,9 @@ static int cpg_mstp_clock_is_enabled(struct clk_hw *hw)
>  	u32 value;
>  
>  	if (group->mstpsr)
> -		value = clk_readl(group->mstpsr);
> +		value = cpg_mstp_read(group, group->mstpsr);
>  	else
> -		value = clk_readl(group->smstpcr);
> +		value = cpg_mstp_read(group, group->smstpcr);
>  
>  	return !(value & BIT(clock->bit_index));
>  }
> @@ -188,6 +202,9 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
>  		return;
>  	}
>  
> +	if (of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks"))
> +		group->width_8bit = true;
> +
>  	for (i = 0; i < MSTP_MAX_CLOCKS; ++i)
>  		clks[i] = ERR_PTR(-ENOENT);
>  
> -- 
> 2.10.1
> 
> 

Re: [PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Geert Uytterhoeven]

Hi Chris, Mike, Stephen,

On Thu, Dec 15, 2016 at 6:00 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> The RZ/A1 is different than the other Renesas SOCs because the MSTP
> registers are 8-bit instead of 32-bit and if you try writing values as
> 32-bit nothing happens...meaning this driver never worked for r7s72100.

Thanks for your patch!

The only reason it ever worked was that almost all module clocks are
enabled at boot time...

> Fixes: b6face404f38 ("ARM: shmobile: r7s72100: add essential clock nodes to dtsi")
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Mike/Stephen: as this is a fix for stable (v3.16+), can you please take it
directly?
Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Stephen Boyd]

On 12/19, Geert Uytterhoeven wrote:
> Hi Chris, Mike, Stephen,
> 
> On Thu, Dec 15, 2016 at 6:00 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> > The RZ/A1 is different than the other Renesas SOCs because the MSTP
> > registers are 8-bit instead of 32-bit and if you try writing values as
> > 32-bit nothing happens...meaning this driver never worked for r7s72100.
> 
> Thanks for your patch!
> 
> The only reason it ever worked was that almost all module clocks are
> enabled at boot time...
> 
> > Fixes: b6face404f38 ("ARM: shmobile: r7s72100: add essential clock nodes to dtsi")
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Mike/Stephen: as this is a fix for stable (v3.16+), can you please take it
> directly?

Sure, is it a fix for something that has been exposed as a
problem in this merge window? Just trying to gauge the urgency of
merging this.

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

Re: [PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Geert Uytterhoeven]

Hi Stephen,

On Tue, Dec 20, 2016 at 11:55 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/19, Geert Uytterhoeven wrote:
>> On Thu, Dec 15, 2016 at 6:00 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
>> > The RZ/A1 is different than the other Renesas SOCs because the MSTP
>> > registers are 8-bit instead of 32-bit and if you try writing values as
>> > 32-bit nothing happens...meaning this driver never worked for r7s72100.
>>
>> Thanks for your patch!
>>
>> The only reason it ever worked was that almost all module clocks are
>> enabled at boot time...
>>
>> > Fixes: b6face404f38 ("ARM: shmobile: r7s72100: add essential clock nodes to dtsi")
>> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
>>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>
>> Mike/Stephen: as this is a fix for stable (v3.16+), can you please take it
>> directly?
>
> Sure, is it a fix for something that has been exposed as a
> problem in this merge window? Just trying to gauge the urgency of
> merging this.

No, I don't think it has been exposed by changes in this merge window
(the only RZ/A1 changes were the enablement of SDHI and MMC).
Chris, can you please confirm?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Chris Brandt]

On December 21, 2016, Geert Uytterhoeven wrote:
> >> Mike/Stephen: as this is a fix for stable (v3.16+), can you please
> >> take it directly?
> >
> > Sure, is it a fix for something that has been exposed as a problem in
> > this merge window? Just trying to gauge the urgency of merging this.
> 
> No, I don't think it has been exposed by changes in this merge window (the
> only RZ/A1 changes were the enablement of SDHI and MMC).
> Chris, can you please confirm?

That's correct. In reality, none of the existing RZ/A1 drivers in the kernel
should have ever worked (ETH, SPI, I2C, MTU2, etc...) because on chip reset
all the peripheral clocks are turned off.

However...like Geert mentioned...the boot loader was turning everything on
shortly after reset so even though the clock driver never worked, it made
it seems so. Oops.

Of course now going forward, I use a modified boot loader to turn everything
back off (except the serial console) so I can confirm that clock gating is
really working.


Chris

RE: [PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Chris Brandt]

T24gRGVjZW1iZXIgMjEsIDIwMTYsIEdlZXJ0IFV5dHRlcmhvZXZlbiB3cm90ZToNCj4gPj4gTWlr
ZS9TdGVwaGVuOiBhcyB0aGlzIGlzIGEgZml4IGZvciBzdGFibGUgKHYzLjE2KyksIGNhbiB5b3Ug
cGxlYXNlDQo+ID4+IHRha2UgaXQgZGlyZWN0bHk/DQo+ID4NCj4gPiBTdXJlLCBpcyBpdCBhIGZp
eCBmb3Igc29tZXRoaW5nIHRoYXQgaGFzIGJlZW4gZXhwb3NlZCBhcyBhIHByb2JsZW0gaW4NCj4g
PiB0aGlzIG1lcmdlIHdpbmRvdz8gSnVzdCB0cnlpbmcgdG8gZ2F1Z2UgdGhlIHVyZ2VuY3kgb2Yg
bWVyZ2luZyB0aGlzLg0KPiANCj4gTm8sIEkgZG9uJ3QgdGhpbmsgaXQgaGFzIGJlZW4gZXhwb3Nl
ZCBieSBjaGFuZ2VzIGluIHRoaXMgbWVyZ2Ugd2luZG93ICh0aGUNCj4gb25seSBSWi9BMSBjaGFu
Z2VzIHdlcmUgdGhlIGVuYWJsZW1lbnQgb2YgU0RISSBhbmQgTU1DKS4NCj4gQ2hyaXMsIGNhbiB5
b3UgcGxlYXNlIGNvbmZpcm0/DQoNClRoYXQncyBjb3JyZWN0LiBJbiByZWFsaXR5LCBub25lIG9m
IHRoZSBleGlzdGluZyBSWi9BMSBkcml2ZXJzIGluIHRoZSBrZXJuZWwNCnNob3VsZCBoYXZlIGV2
ZXIgd29ya2VkIChFVEgsIFNQSSwgSTJDLCBNVFUyLCBldGMuLi4pIGJlY2F1c2Ugb24gY2hpcCBy
ZXNldA0KYWxsIHRoZSBwZXJpcGhlcmFsIGNsb2NrcyBhcmUgdHVybmVkIG9mZi4NCg0KSG93ZXZl
ci4uLmxpa2UgR2VlcnQgbWVudGlvbmVkLi4udGhlIGJvb3QgbG9hZGVyIHdhcyB0dXJuaW5nIGV2
ZXJ5dGhpbmcgb24NCnNob3J0bHkgYWZ0ZXIgcmVzZXQgc28gZXZlbiB0aG91Z2ggdGhlIGNsb2Nr
IGRyaXZlciBuZXZlciB3b3JrZWQsIGl0IG1hZGUNCml0IHNlZW1zIHNvLiBPb3BzLg0KDQpPZiBj
b3Vyc2Ugbm93IGdvaW5nIGZvcndhcmQsIEkgdXNlIGEgbW9kaWZpZWQgYm9vdCBsb2FkZXIgdG8g
dHVybiBldmVyeXRoaW5nDQpiYWNrIG9mZiAoZXhjZXB0IHRoZSBzZXJpYWwgY29uc29sZSkgc28g
SSBjYW4gY29uZmlybSB0aGF0IGNsb2NrIGdhdGluZyBpcw0KcmVhbGx5IHdvcmtpbmcuDQoNCg0K
Q2hyaXMNCg==

Re: [PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Stephen Boyd]

On 12/21, Chris Brandt wrote:
> On December 21, 2016, Geert Uytterhoeven wrote:
> > >> Mike/Stephen: as this is a fix for stable (v3.16+), can you please
> > >> take it directly?
> > >
> > > Sure, is it a fix for something that has been exposed as a problem in
> > > this merge window? Just trying to gauge the urgency of merging this.
> > 
> > No, I don't think it has been exposed by changes in this merge window (the
> > only RZ/A1 changes were the enablement of SDHI and MMC).
> > Chris, can you please confirm?
> 
> That's correct. In reality, none of the existing RZ/A1 drivers in the kernel
> should have ever worked (ETH, SPI, I2C, MTU2, etc...) because on chip reset
> all the peripheral clocks are turned off.
> 
> However...like Geert mentioned...the boot loader was turning everything on
> shortly after reset so even though the clock driver never worked, it made
> it seems so. Oops.
> 
> Of course now going forward, I use a modified boot loader to turn everything
> back off (except the serial console) so I can confirm that clock gating is
> really working.
> 

Ok, it seems like a more urgent fix due to the fact that the
bootloader update has caused the drivers to stop working. I'll
make sure to send it as a fix this release cycle.

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

Re: [PATCH v2] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Stephen Boyd]

On 12/15, Chris Brandt wrote:
> The RZ/A1 is different than the other Renesas SOCs because the MSTP
> registers are 8-bit instead of 32-bit and if you try writing values as
> 32-bit nothing happens...meaning this driver never worked for r7s72100.
> 
> Fixes: b6face404f38 ("ARM: shmobile: r7s72100: add essential clock nodes to dtsi")
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---

Applied to clk-fixes

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