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

[Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>]

Hi Chris

> > 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.

How about this ?
# I didn't test this, compile test only

------------------
diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 9375777..488ff56 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -43,6 +43,7 @@ struct mstp_clock_group {
 	void __iomem *smstpcr;
 	void __iomem *mstpsr;
 	spinlock_t lock;
+	bool reg_width_8bit;
 };
 
 /**
@@ -58,6 +59,14 @@ struct mstp_clock {
 };
 
 #define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw)
+#define cpg_mstp_read(group, reg)		\
+	(group)->reg_width_8bit ?		\
+	       readb((group)->reg) :		\
+	       clk_readl((group)->reg)
+#define cpg_mstp_write(group, val, reg)		\
+	(group)->reg_width_8bit ?		\
+	       writeb(val, (group)->reg) :	\
+	       clk_writel(val, (group)->reg)
 
 static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 {
@@ -70,12 +79,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(group, value, smstpcr);
 
 	spin_unlock_irqrestore(&group->lock, flags);
 
@@ -83,7 +92,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 +123,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));
 }
@@ -159,7 +168,8 @@ static int cpg_mstp_clock_is_enabled(struct clk_hw *hw)
 	return clk;
 }
 
-static void __init cpg_mstp_clocks_init(struct device_node *np)
+static struct mstp_clock_group* __init
+__cpg_mstp_clocks_init(struct device_node *np)
 {
 	struct mstp_clock_group *group;
 	const char *idxname;
@@ -172,7 +182,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 		kfree(group);
 		kfree(clks);
 		pr_err("%s: failed to allocate group\n", __func__);
-		return;
+		return NULL;
 	}
 
 	spin_lock_init(&group->lock);
@@ -185,7 +195,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 		pr_err("%s: failed to remap SMSTPCR\n", __func__);
 		kfree(group);
 		kfree(clks);
-		return;
+		return NULL;
 	}
 
 	for (i = 0; i < MSTP_MAX_CLOCKS; ++i)
@@ -240,9 +250,26 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 	}
 
 	of_clk_add_provider(np, of_clk_src_onecell_get, &group->data);
+
+	return group;
+}
+
+static void __init cpg_mstp_clocks_init(struct device_node *np)
+{
+	__cpg_mstp_clocks_init(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)
+{
+	struct mstp_clock_group *group = __cpg_mstp_clocks_init(np);
+
+	if (group)
+		group->reg_width_8bit = true;
+}
+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;