[v4,2/6] clk: renesas: rcar-gen3: Add Z2 clock divider support

Commit Message

Simon Horman Jan. 3, 2018, 12:18 p.m. UTC

From: Takeshi Kihara <[e-mail]>

This patch adds Z2 clock divider support for R-Car Gen3 SoC.

Signed-off-by: Takeshi Kihara <[e-mail]>
Signed-off-by: Simon Horman <[e-mail]>
---
v4 [Simon Horman]
* Rebase
* Use __ffs as FIELD_{GET,PREP} don't not work with non-constant masks
* Use correct mask in cpg_z_clk_recalc_rate()

v3 [Simon Horman]
* Consolidate Z and Z2 clock ops
* Allow setting of Z2 clock

v1 [Simon Horman]
* Provide __cpg_z_clk_recalc_rate() helper
* Use GENMASK

v0 [Takeshi Kihara]
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 22 ++++++++++++++++------
 drivers/clk/renesas/rcar-gen3-cpg.h |  1 +
 2 files changed, 17 insertions(+), 6 deletions(-)

Comments

Geert Uytterhoeven Jan. 3, 2018, 12:47 p.m. UTC | #1

Hi Simon,

On Wed, Jan 3, 2018 at 1:18 PM, Simon Horman <[e-mail]> wrote:
> From: Takeshi Kihara <[e-mail]>
>
> This patch adds Z2 clock divider support for R-Car Gen3 SoC.
>
> Signed-off-by: Takeshi Kihara <[e-mail]>
> Signed-off-by: Simon Horman <[e-mail]>
> ---
> v4 [Simon Horman]
> * Rebase
> * Use __ffs as FIELD_{GET,PREP} don't not work with non-constant masks
> * Use correct mask in cpg_z_clk_recalc_rate()

Thanks for the update!

> --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -63,7 +63,7 @@ static void cpg_simple_notifier_register(struct raw_notifier_head *notifiers,
>  }
>
>  /*
> - * Z Clock
> + * Z Clock & Z2 Clock
>   *
>   * Traits of this clock:
>   * prepare - clk_prepare only ensures that parents are prepared
> @@ -75,11 +75,13 @@ static void cpg_simple_notifier_register(struct raw_notifier_head *notifiers,
>  #define CPG_FRQCRB_KICK                        BIT(31)
>  #define CPG_FRQCRC                     0x000000e0
>  #define CPG_FRQCRC_ZFC_MASK            GENMASK(12, 8)
> +#define CPG_FRQCRC_Z2FC_MASK           GENMASK(4, 0)
>
>  struct cpg_z_clk {
>         struct clk_hw hw;
>         void __iomem *reg;
>         void __iomem *kick_reg;
> +       unsigned long mask;
>  };
>
>  #define to_z_clk(_hw)  container_of(_hw, struct cpg_z_clk, hw)
> @@ -89,8 +91,10 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
>  {
>         struct cpg_z_clk *zclk = to_z_clk(hw);
>         unsigned int mult;
> +       u32 val;
>
> -       mult = 32 - FIELD_GET(CPG_FRQCRC_ZFC_MASK, clk_readl(zclk->reg));
> +       val = clk_readl(zclk->reg) & zclk->mask;
> +       mult = 32 - (val >> (__ffs(zclk->mask) - 1));

Shouldn't that be

        mult = 32 - (val >> __ffs(zclk->mask));

(same below)?

__ffs() returns 0..31, so you will shift right by 7 (Z) or -1 (Z2)?

As the CPG/MSSR driver now has suspend/resume support, do we need
a notifier to restore the Z or Z2 registers? Or is that handled automatically
by cpufreq during system resume, for both the primary and the secondary
CPU cores?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Simon Horman Jan. 5, 2018, 2:04 p.m. UTC | #2

On Wed, Jan 03, 2018 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Wed, Jan 3, 2018 at 1:18 PM, Simon Horman <[e-mail]> wrote:
> > From: Takeshi Kihara <[e-mail]>
> >
> > This patch adds Z2 clock divider support for R-Car Gen3 SoC.
> >
> > Signed-off-by: Takeshi Kihara <[e-mail]>
> > Signed-off-by: Simon Horman <[e-mail]>
> > ---
> > v4 [Simon Horman]
> > * Rebase
> > * Use __ffs as FIELD_{GET,PREP} don't not work with non-constant masks
> > * Use correct mask in cpg_z_clk_recalc_rate()
> 
> Thanks for the update!
> 
> > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> > @@ -63,7 +63,7 @@ static void cpg_simple_notifier_register(struct raw_notifier_head *notifiers,
> >  }
> >
> >  /*
> > - * Z Clock
> > + * Z Clock & Z2 Clock
> >   *
> >   * Traits of this clock:
> >   * prepare - clk_prepare only ensures that parents are prepared
> > @@ -75,11 +75,13 @@ static void cpg_simple_notifier_register(struct raw_notifier_head *notifiers,
> >  #define CPG_FRQCRB_KICK                        BIT(31)
> >  #define CPG_FRQCRC                     0x000000e0
> >  #define CPG_FRQCRC_ZFC_MASK            GENMASK(12, 8)
> > +#define CPG_FRQCRC_Z2FC_MASK           GENMASK(4, 0)
> >
> >  struct cpg_z_clk {
> >         struct clk_hw hw;
> >         void __iomem *reg;
> >         void __iomem *kick_reg;
> > +       unsigned long mask;
> >  };
> >
> >  #define to_z_clk(_hw)  container_of(_hw, struct cpg_z_clk, hw)
> > @@ -89,8 +91,10 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
> >  {
> >         struct cpg_z_clk *zclk = to_z_clk(hw);
> >         unsigned int mult;
> > +       u32 val;
> >
> > -       mult = 32 - FIELD_GET(CPG_FRQCRC_ZFC_MASK, clk_readl(zclk->reg));
> > +       val = clk_readl(zclk->reg) & zclk->mask;
> > +       mult = 32 - (val >> (__ffs(zclk->mask) - 1));
> 
> Shouldn't that be
> 
>         mult = 32 - (val >> __ffs(zclk->mask));
> 
> (same below)?
> 
> __ffs() returns 0..31, so you will shift right by 7 (Z) or -1 (Z2)?

Thanks, I'll look at fixing that.

> As the CPG/MSSR driver now has suspend/resume support, do we need
> a notifier to restore the Z or Z2 registers? Or is that handled automatically
> by cpufreq during system resume, for both the primary and the secondary
> CPU cores?

I am a bit unsure.

When using the A57 cores, which is the default case, the Z clk is queried
by CPUFreq on resume. It appears that on my system its already set to the
correct value but I assume if it was not then it would be reset. However,
this does not cover Z2 clk. So perhaps to be safe we need to register
notifiers and make sure they they play nicely with CPUFreq?

Geert Uytterhoeven Jan. 5, 2018, 2:35 p.m. UTC | #3

Hi Simon,

On Fri, Jan 5, 2018 at 3:04 PM, Simon Horman <[e-mail]> wrote:
> On Wed, Jan 03, 2018 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
>> On Wed, Jan 3, 2018 at 1:18 PM, Simon Horman <[e-mail]> wrote:
>> > From: Takeshi Kihara <[e-mail]>
>> >
>> > This patch adds Z2 clock divider support for R-Car Gen3 SoC.
>> >
>> > Signed-off-by: Takeshi Kihara <[e-mail]>
>> > Signed-off-by: Simon Horman <[e-mail]>

>> As the CPG/MSSR driver now has suspend/resume support, do we need
>> a notifier to restore the Z or Z2 registers? Or is that handled automatically
>> by cpufreq during system resume, for both the primary and the secondary
>> CPU cores?
>
> I am a bit unsure.
>
> When using the A57 cores, which is the default case, the Z clk is queried
> by CPUFreq on resume. It appears that on my system its already set to the
> correct value but I assume if it was not then it would be reset. However,
> this does not cover Z2 clk. So perhaps to be safe we need to register
> notifiers and make sure they they play nicely with CPUFreq?

Of course the CPU is special: unlike many other devices, it must be running
when the kernel is reentered upon system resume.
It may be running using a different frequency setting, though.
However, following "opp-suspend", the system will always suspend with the
Z clock running at 1.5GHz, which is the default?
So Z is probably OK.

It's more interesting to check what happens when the little cores are
enabled as well (unfortunately that requires different firmware).
1. Does cpufreq handle them correctly when they are onlined again during
   system resume?
2. What happens if you offline all big cores, and enter system suspend
   running with little cores only?
   Perhaps that's prevented by the "opp-suspend" property as well?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Simon Horman Jan. 8, 2018, 8:02 a.m. UTC | #4

On Fri, Jan 05, 2018 at 03:35:13PM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Fri, Jan 5, 2018 at 3:04 PM, Simon Horman <[e-mail]> wrote:
> > On Wed, Jan 03, 2018 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
> >> On Wed, Jan 3, 2018 at 1:18 PM, Simon Horman <[e-mail]> wrote:
> >> > From: Takeshi Kihara <[e-mail]>
> >> >
> >> > This patch adds Z2 clock divider support for R-Car Gen3 SoC.
> >> >
> >> > Signed-off-by: Takeshi Kihara <[e-mail]>
> >> > Signed-off-by: Simon Horman <[e-mail]>
> 
> >> As the CPG/MSSR driver now has suspend/resume support, do we need
> >> a notifier to restore the Z or Z2 registers? Or is that handled automatically
> >> by cpufreq during system resume, for both the primary and the secondary
> >> CPU cores?
> >
> > I am a bit unsure.
> >
> > When using the A57 cores, which is the default case, the Z clk is queried
> > by CPUFreq on resume. It appears that on my system its already set to the
> > correct value but I assume if it was not then it would be reset. However,
> > this does not cover Z2 clk. So perhaps to be safe we need to register
> > notifiers and make sure they they play nicely with CPUFreq?
> 
> Of course the CPU is special: unlike many other devices, it must be running
> when the kernel is reentered upon system resume.
> It may be running using a different frequency setting, though.
> However, following "opp-suspend", the system will always suspend with the
> Z clock running at 1.5GHz, which is the default?
> So Z is probably OK.
> 
> It's more interesting to check what happens when the little cores are
> enabled as well (unfortunately that requires different firmware).
> 1. Does cpufreq handle them correctly when they are onlined again during
>    system resume?
> 2. What happens if you offline all big cores, and enter system suspend
>    running with little cores only?
>    Perhaps that's prevented by the "opp-suspend" property as well?

Thanks for clarifying the problem.

I should be able to install updated firmware on:
i* r8a7796 (M3-W) ES1.0 / Salvator-X; and
* r8a7795 (H3) ES2.0 / Salvator-XS.
* But not easily r8a7795 (H3) / Salvator-X

Do you know what the minimal / desired firmware version is to exercise the
scenarios you describe.

Geert Uytterhoeven Jan. 8, 2018, 8:06 a.m. UTC | #5

Hi Simon,

On Mon, Jan 8, 2018 at 9:02 AM, Simon Horman <[e-mail]> wrote:
> On Fri, Jan 05, 2018 at 03:35:13PM +0100, Geert Uytterhoeven wrote:
>> On Fri, Jan 5, 2018 at 3:04 PM, Simon Horman <[e-mail]> wrote:
>> > On Wed, Jan 03, 2018 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
>> >> On Wed, Jan 3, 2018 at 1:18 PM, Simon Horman <[e-mail]> wrote:
>> >> > From: Takeshi Kihara <[e-mail]>
>> >> >
>> >> > This patch adds Z2 clock divider support for R-Car Gen3 SoC.
>> >> >
>> >> > Signed-off-by: Takeshi Kihara <[e-mail]>
>> >> > Signed-off-by: Simon Horman <[e-mail]>
>>
>> >> As the CPG/MSSR driver now has suspend/resume support, do we need
>> >> a notifier to restore the Z or Z2 registers? Or is that handled automatically
>> >> by cpufreq during system resume, for both the primary and the secondary
>> >> CPU cores?
>> >
>> > I am a bit unsure.
>> >
>> > When using the A57 cores, which is the default case, the Z clk is queried
>> > by CPUFreq on resume. It appears that on my system its already set to the
>> > correct value but I assume if it was not then it would be reset. However,
>> > this does not cover Z2 clk. So perhaps to be safe we need to register
>> > notifiers and make sure they they play nicely with CPUFreq?
>>
>> Of course the CPU is special: unlike many other devices, it must be running
>> when the kernel is reentered upon system resume.
>> It may be running using a different frequency setting, though.
>> However, following "opp-suspend", the system will always suspend with the
>> Z clock running at 1.5GHz, which is the default?
>> So Z is probably OK.
>>
>> It's more interesting to check what happens when the little cores are
>> enabled as well (unfortunately that requires different firmware).
>> 1. Does cpufreq handle them correctly when they are onlined again during
>>    system resume?
>> 2. What happens if you offline all big cores, and enter system suspend
>>    running with little cores only?
>>    Perhaps that's prevented by the "opp-suspend" property as well?
>
> Thanks for clarifying the problem.
>
> I should be able to install updated firmware on:
> i* r8a7796 (M3-W) ES1.0 / Salvator-X; and
> * r8a7795 (H3) ES2.0 / Salvator-XS.
> * But not easily r8a7795 (H3) / Salvator-X
>
> Do you know what the minimal / desired firmware version is to exercise the
> scenarios you describe.

There are binaries available for "v2.7.0 8core". Unfortunately I
believe it supports
your "not easily r8a7795 (H3) / Salvator-X" only...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Patch

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index b85918fa62c6..a7d68cefda9c 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -63,7 +63,7 @@  static void cpg_simple_notifier_register(struct raw_notifier_head *notifiers,
 }
 
 /*
- * Z Clock
+ * Z Clock & Z2 Clock
  *
  * Traits of this clock:
  * prepare - clk_prepare only ensures that parents are prepared
@@ -75,11 +75,13 @@  static void cpg_simple_notifier_register(struct raw_notifier_head *notifiers,
 #define CPG_FRQCRB_KICK			BIT(31)
 #define CPG_FRQCRC			0x000000e0
 #define CPG_FRQCRC_ZFC_MASK		GENMASK(12, 8)
+#define CPG_FRQCRC_Z2FC_MASK		GENMASK(4, 0)
 
 struct cpg_z_clk {
 	struct clk_hw hw;
 	void __iomem *reg;
 	void __iomem *kick_reg;
+	unsigned long mask;
 };
 
 #define to_z_clk(_hw)	container_of(_hw, struct cpg_z_clk, hw)
@@ -89,8 +91,10 @@  static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
 {
 	struct cpg_z_clk *zclk = to_z_clk(hw);
 	unsigned int mult;
+	u32 val;
 
-	mult = 32 - FIELD_GET(CPG_FRQCRC_ZFC_MASK, clk_readl(zclk->reg));
+	val = clk_readl(zclk->reg) & zclk->mask;
+	mult = 32 - (val >> (__ffs(zclk->mask) - 1));
 
 	/* Factor of 2 is for fixed divider */
 	return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, 32 * 2);
@@ -124,8 +128,8 @@  static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
 		return -EBUSY;
 
-	val = clk_readl(zclk->reg) & ~CPG_FRQCRC_ZFC_MASK;
-	val |= FIELD_PREP(CPG_FRQCRC_ZFC_MASK, 32 - mult);
+	val = clk_readl(zclk->reg) & ~zclk->mask;
+	val |= ((32 - mult) << (__ffs(zclk->mask) - 1)) & zclk->mask;
 	clk_writel(val, zclk->reg);
 
 	/*
@@ -163,7 +167,8 @@  static const struct clk_ops cpg_z_clk_ops = {
 
 static struct clk * __init cpg_z_clk_register(const char *name,
 					      const char *parent_name,
-					      void __iomem *reg)
+					      void __iomem *reg,
+					      unsigned long mask)
 {
 	struct clk_init_data init;
 	struct cpg_z_clk *zclk;
@@ -182,6 +187,7 @@  static struct clk * __init cpg_z_clk_register(const char *name,
 	zclk->reg = reg + CPG_FRQCRC;
 	zclk->kick_reg = reg + CPG_FRQCRB;
 	zclk->hw.init = &init;
+	zclk->mask = mask;
 
 	clk = clk_register(NULL, &zclk->hw);
 	if (IS_ERR(clk))
@@ -551,7 +557,11 @@  struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
 
 	case CLK_TYPE_GEN3_Z:
 		return cpg_z_clk_register(core->name, __clk_get_name(parent),
-					  base);
+					  base, CPG_FRQCRC_ZFC_MASK);
+
+	case CLK_TYPE_GEN3_Z2:
+		return cpg_z_clk_register(core->name, __clk_get_name(parent),
+					  base, CPG_FRQCRC_Z2FC_MASK);
 
 	default:
 		return ERR_PTR(-EINVAL);
diff --git a/drivers/clk/renesas/rcar-gen3-cpg.h b/drivers/clk/renesas/rcar-gen3-cpg.h
index c73d4d6fdc85..ea4f8fc3c4c9 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.h
+++ b/drivers/clk/renesas/rcar-gen3-cpg.h
@@ -22,6 +22,7 @@  enum rcar_gen3_clk_types {
 	CLK_TYPE_GEN3_R,
 	CLK_TYPE_GEN3_PE,
 	CLK_TYPE_GEN3_Z,
+	CLK_TYPE_GEN3_Z2,
 };
 
 #define DEF_GEN3_SD(_name, _id, _parent, _offset)	\