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

[Kuninori Morimoto]

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;