diff mbox

clk: renesas: mstp: Support 8-bit registers for r7s72100

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

Commit Message

Chris Brandt Dec. 15, 2016, 2:11 a.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>
---
 drivers/clk/renesas/clk-mstp.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

Comments

Kuninori Morimoto Dec. 15, 2016, 2:32 a.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>
> ---
(snip)
> +/**
> + * Some devices only have 8-bit registers
> + */
> +bool reg_width_8bit;
(snip)
> +static void __init cpg_mstp_clocks_init8(struct device_node *np)
> +{
> +	reg_width_8bit = true;
> +	cpg_mstp_clocks_init(np);
> +}
> +CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
> +	       cpg_mstp_clocks_init8);

I don't think using global variable is a good idea.
For example, how about add reg_width_8bit into mstp_clock_group,
and cpg_mstp_read/write requests it, or something like that ?

Best regards
---
Kuninori Morimoto
--
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. 15, 2016, 2:58 a.m. UTC | #2
Hi Morimoto-san,

On Dec 14, 2016, Kuninori Morimoto wrote:
> I don't think using global variable is a good idea.
> For example, how about add reg_width_8bit into mstp_clock_group, and
> cpg_mstp_read/write requests it, or something like that ?

If I make a separate CLK_OF_DECLARE like this:

static void __init cpg_mstp_clocks_init8(struct device_node *np) {
	reg_width_8bit = true;
	cpg_mstp_clocks_init(np);
}
CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
	       cpg_mstp_clocks_init8);


The only way I can pass the 8-bit/32-bit choice into cpg_mstp_clocks_init()
is using a global variable.


Unless, I change the API to cpg_mstp_clocks_init(np, reg_width_8bit)

But, that means adding another function:

CLK_OF_DECLARE(cpg_mstp_clks32, "renesas,cpg-mstp-clocks",
               cpg_mstp_clocks_init32);

CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
               cpg_mstp_clocks_init8);

static void __init cpg_mstp_clocks_init32(struct device_node *np)
{
	cpg_mstp_clocks_init(np, false);
}
static void __init cpg_mstp_clocks_init8(struct device_node *np)
{
	cpg_mstp_clocks_init(np, true);
}


A global variable is much easier, but if no one likes it, I can change to
mstp_clock_group.reg_width_8bit instead.


Chris
--
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. 15, 2016, 8:42 a.m. UTC | #3
Hi Chris,

On Thu, Dec 15, 2016 at 3:58 AM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Dec 14, 2016, Kuninori Morimoto wrote:
>> I don't think using global variable is a good idea.
>> For example, how about add reg_width_8bit into mstp_clock_group, and
>> cpg_mstp_read/write requests it, or something like that ?
>
> If I make a separate CLK_OF_DECLARE like this:
>
> static void __init cpg_mstp_clocks_init8(struct device_node *np) {
>         reg_width_8bit = true;
>         cpg_mstp_clocks_init(np);
> }
> CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
>                cpg_mstp_clocks_init8);
>
>
> The only way I can pass the 8-bit/32-bit choice into cpg_mstp_clocks_init()
> is using a global variable.
>
>
> Unless, I change the API to cpg_mstp_clocks_init(np, reg_width_8bit)
>
> But, that means adding another function:
>
> CLK_OF_DECLARE(cpg_mstp_clks32, "renesas,cpg-mstp-clocks",
>                cpg_mstp_clocks_init32);
>
> CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
>                cpg_mstp_clocks_init8);
>
> static void __init cpg_mstp_clocks_init32(struct device_node *np)
> {
>         cpg_mstp_clocks_init(np, false);
> }
> static void __init cpg_mstp_clocks_init8(struct device_node *np)
> {
>         cpg_mstp_clocks_init(np, true);
> }

Or you can add an of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks")
check to cpg_mstp_clocks_init(), keeping the single CLK_OF_DECLARE().

I think that would also performs slightly better, as only nodes compatible
with "renesas,cpg-mstp-clocks" would be subject to the additional check.

> A global variable is much easier, but if no one likes it, I can change to
> mstp_clock_group.reg_width_8bit instead.

Personally, I have no problem with the global variable: either you're running
on RZ/A1, and all MSTP clocks need 8-bit accesses, or you're not.

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. 15, 2016, 2:09 p.m. UTC | #4
Hi Geert and Morimoto-san,

On December 15, 2016, Geert Uytterhoeven wrote:
> Or you can add an of_device_is_compatible(np, "renesas,r7s72100-mstp-

> clocks") check to cpg_mstp_clocks_init(), keeping the single

> CLK_OF_DECLARE().

> 

> I think that would also performs slightly better, as only nodes compatible

> with "renesas,cpg-mstp-clocks" would be subject to the additional check.

> 


I think now I am leaning more towards that idea:

 * use of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks")
 * add reg_width_8bit to group (not global)


The reason is that I HOPE that this one SOC will be the only Renesas device that will ever
have 8-bit MSTP registers. All the other MPU devices seem to have 32-bit.

Therefore, adding a simple 1-line of_device_is_compatible for this one case seems the best.

Thank you for the suggestions.

Chris
diff mbox

Patch

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 9375777..69c3604 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -59,6 +59,21 @@  struct mstp_clock {
 
 #define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw)
 
+/**
+ * Some devices only have 8-bit registers
+ */
+bool reg_width_8bit;
+
+static inline u32 cpg_mstp_read(u32 __iomem *reg)
+{
+	return reg_width_8bit ? readb(reg) : clk_readl(reg);
+}
+
+static inline void cpg_mstp_write(u32 val, u32 __iomem *reg)
+{
+	reg_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 +85,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->smstpcr);
 	if (enable)
 		value &= ~bitmask;
 	else
 		value |= bitmask;
-	clk_writel(value, group->smstpcr);
+	cpg_mstp_write(value, group->smstpcr);
 
 	spin_unlock_irqrestore(&group->lock, flags);
 
@@ -83,7 +98,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->mstpsr) & bitmask))
 			break;
 		cpu_relax();
 	}
@@ -114,9 +129,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->mstpsr);
 	else
-		value = clk_readl(group->smstpcr);
+		value = cpg_mstp_read(group->smstpcr);
 
 	return !(value & BIT(clock->bit_index));
 }
@@ -243,6 +258,14 @@  static void __init cpg_mstp_clocks_init(struct device_node *np)
 }
 CLK_OF_DECLARE(cpg_mstp_clks, "renesas,cpg-mstp-clocks", cpg_mstp_clocks_init);
 
+static void __init cpg_mstp_clocks_init8(struct device_node *np)
+{
+	reg_width_8bit = true;
+	cpg_mstp_clocks_init(np);
+}
+CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
+	       cpg_mstp_clocks_init8);
+
 int cpg_mstp_attach_dev(struct generic_pm_domain *unused, struct device *dev)
 {
 	struct device_node *np = dev->of_node;