diff mbox

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

Message ID 20161215170027.28411-1-chris.brandt@renesas.com (mailing list archive)
State Accepted, archived
Delegated to: Stephen Boyd
Headers show

Commit Message

Chris Brandt Dec. 15, 2016, 5 p.m. UTC
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(-)

Comments

Kuninori Morimoto Dec. 15, 2016, 11:53 p.m. UTC | #1
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
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Dec. 19, 2016, 10:47 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Dec. 20, 2016, 10:55 p.m. UTC | #3
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.
Geert Uytterhoeven Dec. 21, 2016, 7:03 a.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Dec. 21, 2016, 3:32 p.m. UTC | #5
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
Stephen Boyd Dec. 21, 2016, 10:53 p.m. UTC | #6
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.
Stephen Boyd Dec. 21, 2016, 10:56 p.m. UTC | #7
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
diff mbox

Patch

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);