| Message ID | 20161215170027.28411-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| 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> > --- 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 > >
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
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.
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
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
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.
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 --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);
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(-)