| Message ID | 20161215021134.14902-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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
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
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
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 --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;
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(-)