[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]

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