clk: qcom: CPU clock driver for msm8996

The driver provides kernel level API for other drivers to access the MSM8996 L2 cache registers. Separating the L2 access code from the PMU driver and making it public to allow other drivers use it. The accesses must be separated with a single spinlock, maintained in this driver.

drivers/perf/qcom_l2_pmu.c +2 -11

-static void set_l2_indirect_reg(u64 reg, u64 val)
-{
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&l2_access_lock, flags);
-	write_sysreg_s(reg, L2CPUSRSELR_EL1);
-	isb();
-	write_sysreg_s(val, L2CPUSRDR_EL1);
-	isb();
-	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
-}
 
+/**
+ * set_l2_indirect_reg: write value to an L2 register

drivers/soc/qcom/kryo-l2-accessors.c new file +8 -0

+	raw_spin_lock_irqsave(&l2_access_lock, flags);
+	write_sysreg_s(reg, L2CPUSRSELR_EL1);
+	isb();
+	write_sysreg_s(val, L2CPUSRDR_EL1);
+	isb();
+	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
+}
+EXPORT_SYMBOL(set_l2_indirect_reg);

drivers/soc/qcom/kryo-l2-accessors.c line 34

Quoted 6 lines · Ilia Lin

+ * accesses, and system registers with respect to device memory
+ */

drivers/soc/qcom/kryo-l2-accessors.c new file +4 -0

+void set_l2_indirect_reg(u64 reg, u64 val)
+{
+	unsigned long flags;
+	mb();

We didn't need this for the PMU driver, so it's unfortuante that it now has to pay the cost.

Can we please factor this mb() into the callers that need it?

Quoted 9 lines

drivers/soc/qcom/kryo-l2-accessors.c new file +8 -0

+	raw_spin_lock_irqsave(&l2_access_lock, flags);
+	write_sysreg_s(reg, L2CPUSRSELR_EL1);
+	isb();
+	write_sysreg_s(val, L2CPUSRDR_EL1);
+	isb();
+	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
+}
+EXPORT_SYMBOL(set_l2_indirect_reg);

[...]

include/soc/qcom/kryo-l2-accessors.h line 27

Quoted 11 lines

include/soc/qcom/kryo-l2-accessors.h new file +11 -0

+#ifdef CONFIG_ARCH_QCOM
+void set_l2_indirect_reg(u64 reg_addr, u64 val);
+u64 get_l2_indirect_reg(u64 reg_addr);
+#else
+static inline void set_l2_indirect_reg(u32 reg_addr, u32 val) {}
+static inline u32 get_l2_indirect_reg(u32 reg_addr)
+{
+	return 0;
+}
+#endif
+#endif

Are there any drivers that will bne built for !CONFIG_ARCH_QCOM that reference this?

It might be better to not have the stub versions, so that we get a build-error if they are erroneously used.

-----Original Message-----
From: Stephen Boyd [mailto:sboyd at codeaurora.org]
Sent: Friday, December 22, 2017 4:07 AM
To: Mark Rutland <[email protected]>
Cc: Ilia Lin <[email protected]>; linux-clk at vger.kernel.org;

kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; devicetree at vger.kernel.org; will.deacon at arm.com;
rnayak at codeaurora.org; qualcomm-lt at lists.linaro.org; celster at codeaurora.org; tfinkel at codeaurora.org
Subject: Re: [PATCH 01/10] soc: qcom: Separate kryo l2 accessors from PMU driver

Quoted 10 lines · Mark Rutland

drivers/soc/qcom/kryo-l2-accessors.c line 34

Quoted 4 lines · Ilia Lin

+ * accesses, and system registers with respect to device memory */

drivers/soc/qcom/kryo-l2-accessors.c new file +3 -0

+void set_l2_indirect_reg(u64 reg, u64 val) {
+	unsigned long flags;
+	mb();

We didn't need this for the PMU driver, so it's unfortuante that it now has to pay the cost.

Can we please factor this mb() into the callers that need it?

+1

Quoted 24 lines

Quoted 8 lines

drivers/soc/qcom/kryo-l2-accessors.c new file +7 -0

+	raw_spin_lock_irqsave(&l2_access_lock, flags);
+	write_sysreg_s(reg, L2CPUSRSELR_EL1);
+	isb();
+	write_sysreg_s(val, L2CPUSRDR_EL1);
+	isb();
+	raw_spin_unlock_irqrestore(&l2_access_lock, flags); }
+EXPORT_SYMBOL(set_l2_indirect_reg);

[...]

include/soc/qcom/kryo-l2-accessors.h line 27

Quoted 9 lines

include/soc/qcom/kryo-l2-accessors.h new file +9 -0

+#ifdef CONFIG_ARCH_QCOM
+void set_l2_indirect_reg(u64 reg_addr, u64 val);
+u64 get_l2_indirect_reg(u64 reg_addr); #else static inline void
+set_l2_indirect_reg(u32 reg_addr, u32 val) {} static inline u32
+get_l2_indirect_reg(u32 reg_addr) {
+	return 0;
+}
+#endif
+#endif

Are there any drivers that will bne built for !CONFIG_ARCH_QCOM that reference this?

Quoted 3 lines

It might be better to not have the stub versions, so that we get a build-error if they are erroneously used.

Does that approach make COMPILE_TEST break?

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

-----Original Message-----
From: Mark Rutland [mailto:mark.rutland at arm.com]
Sent: Tuesday, December 12, 2017 4:03 PM
To: Ilia Lin <[email protected]>
Cc: linux-clk at vger.kernel.org; linux-arm-kernel at lists.infradead.org;

arm-msm at vger.kernel.org; sboyd at codeaurora.org; devicetree at vger.kernel.org; will.deacon at arm.com;
rnayak at codeaurora.org; qualcomm-lt at lists.linaro.org; celster at codeaurora.org; tfinkel at codeaurora.org
Subject: Re: [PATCH 01/10] soc: qcom: Separate kryo l2 accessors from PMU driver

Hi,

Quoted 17 lines · Ilia Lin

The driver provides kernel level API for other drivers to access the MSM8996 L2 cache registers.
Separating the L2 access code from the PMU driver and making it public to allow other drivers use it.
The accesses must be separated with a single spinlock, maintained in this driver.

drivers/perf/qcom_l2_pmu.c +0 -10

-static void set_l2_indirect_reg(u64 reg, u64 val) -{
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&l2_access_lock, flags);
-	write_sysreg_s(reg, L2CPUSRSELR_EL1);
-	isb();
-	write_sysreg_s(val, L2CPUSRDR_EL1);
-	isb();
-	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
-}

drivers/soc/qcom/kryo-l2-accessors.c line 34

Quoted 11 lines

drivers/soc/qcom/kryo-l2-accessors.c new file +11 -0

+/**
+ * set_l2_indirect_reg: write value to an L2 register
+ * @reg: Address of L2 register.
+ * @value: Value to be written to register.
+ *
+ * Use architecturally required barriers for ordering between system
+register
+ * accesses, and system registers with respect to device memory  */
+void set_l2_indirect_reg(u64 reg, u64 val) {
+	unsigned long flags;
+	mb();

We didn't need this for the PMU driver, so it's unfortuante that it now

pay the cost.

Can we please factor this mb() into the callers that need it?

Quoted 7 lines

drivers/soc/qcom/kryo-l2-accessors.c new file +7 -0

+	raw_spin_lock_irqsave(&l2_access_lock, flags);
+	write_sysreg_s(reg, L2CPUSRSELR_EL1);
+	isb();
+	write_sysreg_s(val, L2CPUSRDR_EL1);
+	isb();
+	raw_spin_unlock_irqrestore(&l2_access_lock, flags); }
+EXPORT_SYMBOL(set_l2_indirect_reg);

[...]

include/soc/qcom/kryo-l2-accessors.h line 27

Quoted 9 lines

include/soc/qcom/kryo-l2-accessors.h new file +9 -0

+#ifdef CONFIG_ARCH_QCOM
+void set_l2_indirect_reg(u64 reg_addr, u64 val);
+u64 get_l2_indirect_reg(u64 reg_addr); #else static inline void
+set_l2_indirect_reg(u32 reg_addr, u32 val) {} static inline u32
+get_l2_indirect_reg(u32 reg_addr) {
+	return 0;
+}
+#endif
+#endif

Are there any drivers that will bne built for !CONFIG_ARCH_QCOM that reference this?

It might be better to not have the stub versions, so that we get a

if they are erroneously used.

Thannks,
Mark.

From: Taniya Das <[email protected]>

From: Taniya Das <[email protected]>

Alpha PLLs which do not support dynamic update feature need to be explicitly disabled before a rate change. The ones which do support dynamic update do so within a single vco range, so add a min/max freq check for such PLLs so they fall in the vco range.

Signed-off-by Taniya Das <[email protected]>

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
  drivers/clk/qcom/clk-alpha-pll.c | 71 +++++++++++++++++++++++++++++++++-------
  drivers/clk/qcom/clk-alpha-pll.h |  5 +++
  2 files changed, 65 insertions(+), 11 deletions(-)

drivers/clk/qcom/clk-alpha-pll.c +0 -0

index 47a1da3..ecb9e7f 100644
@@ -376,19 +376,46 @@ static unsigned long alpha_pll_calc_rate(u64 prate, u32 l, u32 a)
  	return alpha_pll_calc_rate(prate, l, a);

drivers/clk/qcom/clk-alpha-pll.c +27 -0

+
+	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE) {
+		/*
+		 * PLLs which support dynamic updates support one single
+		 * vco range, between min_rate and max_rate supported
+		 */
+		if (rate < pll->min_rate || rate > pll->max_rate) {
+			pr_err("alpha pll rate outside supported min/max range\n");
+			return -EINVAL;
+		}
+	} else {
+		/*
+		 * All alpha PLLs which do not support dynamic update,
+		 * should be disabled before a vco update.
+		 */
+		if (enabled)
+			disable(hw);
+
+		vco = alpha_pll_find_vco(pll, rate);
+		if (!vco) {
+			pr_err("alpha pll not in a valid vco range\n");
+			return -EINVAL;
+		}
+
+		regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,
+				   PLL_VCO_MASK << PLL_VCO_SHIFT,
+				   vco->val << PLL_VCO_SHIFT);
  	}
 
  	regmap_write(pll->clkr.regmap, off + PLL_L_VAL, l);

@@ -401,16 +428,29 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
  		regmap_write(pll->clkr.regmap, off + PLL_ALPHA_VAL_U, a >> 32);
  	}
 

-----Original Message-----
From: Julien Thierry [mailto:julien.thierry at arm.com]
Sent: Tuesday, December 12, 2017 5:06 PM
To: Ilia Lin <[email protected]>; linux-clk at vger.kernel.org; linux-arm- kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; sboyd at codeaurora.org
Cc: mark.rutland at arm.com; devicetree at vger.kernel.org;
rnayak at codeaurora.org; will.deacon at arm.com; tfinkel at codeaurora.org; qualcomm-lt at lists.linaro.org; celster at codeaurora.org; Taniya Das <[email protected]>
Subject: Re: [PATCH 02/10] clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update

Hi,

Quoted 10 lines · Ilia Lin

From: Taniya Das <[email protected]>

From: Taniya Das <[email protected]>

Alpha PLLs which do not support dynamic update feature need to be explicitly disabled before a rate change.
The ones which do support dynamic update do so within a single vco range, so add a min/max freq check for such PLLs so they fall in the vco range.

Quoted 5 lines

Signed-off-by Taniya Das <[email protected]>

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
  drivers/clk/qcom/clk-alpha-pll.c | 71

+++++++++++++++++++++++++++++++++-------

drivers/clk/qcom/clk-alpha-pll.c static unsigned long alpha_pll_calc_rate(u64

Quoted 9 lines

  drivers/clk/qcom/clk-alpha-pll.h |  5 +++
  2 files changed, 65 insertions(+), 11 deletions(-)

drivers/clk/qcom/clk-alpha-pll.c +0 -0

index 47a1da3..ecb9e7f 100644
@@ -376,19 +376,46 @@ static unsigned long alpha_pll_calc_rate(u64

prate, u32 l, u32 a)

drivers/clk/qcom/clk-alpha-pll.c static unsigned long alpha_pll_calc_rate(u64 prate, u32 l, u32 a) · line 384

Quoted 11 lines

drivers/clk/qcom/clk-alpha-pll.c +5 -2

  	return alpha_pll_calc_rate(prate, l, a);
  }
 
-static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
-				  unsigned long prate)
+static int alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long prate,
+			      int (*enable)(struct clk_hw *hw),
+			      void (*disable)(struct clk_hw *hw))
  {
+	bool enabled;

Some remarks about this.

drivers/clk/qcom/clk-alpha-pll.c static unsigned long alpha_pll_calc_rate(u64 prate, u32 l, u32 a) · line 391

Quoted 11 lines

drivers/clk/qcom/clk-alpha-pll.c +1 -4

  	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
  	const struct pll_vco *vco;
  	u32 l, off = pll->offset;
  	u64 a;
 
  	rate = alpha_pll_round_rate(rate, prate, &l, &a);
-	vco = alpha_pll_find_vco(pll, rate);
-	if (!vco) {
-		pr_err("alpha pll not in a valid vco range\n");
-		return -EINVAL;
+	enabled = clk_hw_is_enabled(hw);

This is not needed unless we go through the 'else' branch.

drivers/clk/qcom/clk-alpha-pll.c static unsigned long alpha_pll_calc_rate(u64 prate, u32 l, u32 a)

Quoted 8 lines

drivers/clk/qcom/clk-alpha-pll.c +8 -0

+
+	if (pll->flags & SUPPORTS_DYNAMIC_UPDATE) {
+		/*
+		 * PLLs which support dynamic updates support one single
+		 * vco range, between min_rate and max_rate supported
+		 */
+		if (rate < pll->min_rate || rate > pll->max_rate) {
+			pr_err("alpha pll rate outside supported min/max

range\n");

Quoted 13 lines

drivers/clk/qcom/clk-alpha-pll.c +13 -0

+			return -EINVAL;
+		}
+	} else {
+		/*
+		 * All alpha PLLs which do not support dynamic update,
+		 * should be disabled before a vco update.
+		 */
+		if (enabled)
+			disable(hw);
+
+		vco = alpha_pll_find_vco(pll, rate);
+		if (!vco) {
+			pr_err("alpha pll not in a valid vco range\n");

drivers/clk/qcom/clk-alpha-pll.c static unsigned long alpha_pll_calc_rate(u64 prate, u32 l, u32 a)

Quoted 10 lines

drivers/clk/qcom/clk-alpha-pll.c +7 -0

+			return -EINVAL;
+		}
+
+		regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,
+				   PLL_VCO_MASK << PLL_VCO_SHIFT,
+				   vco->val << PLL_VCO_SHIFT);
  	}
 
  	regmap_write(pll->clkr.regmap, off + PLL_L_VAL, l); @@ -401,16
+428,29 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned

long rate,

drivers/clk/qcom/clk-alpha-pll.c static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,

Quoted 9 lines

regmap_write(pll->clkr.regmap, off + PLL_ALPHA_VAL_U, a

Quoted 1 line

32);

}

drivers/clk/qcom/clk-alpha-pll.c +0 -4

-	regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,
-			   PLL_VCO_MASK << PLL_VCO_SHIFT,
-			   vco->val << PLL_VCO_SHIFT);
-
  	regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,

PLL_ALPHA_EN,

drivers/clk/qcom/clk-alpha-pll.c static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate, · line 436

Quoted 5 lines

PLL_ALPHA_EN);

drivers/clk/qcom/clk-alpha-pll.c +3 -0

+	if (!(pll->flags & SUPPORTS_DYNAMIC_UPDATE) && enabled)
+		enable(hw);
+

This condition is only "did we disable the clock and need to reenable it?".

To make it clearer, I'd suggest renaming 'enabled' to something like 'need_reenabling' and the code look like this:

static int alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,

			      unsigned long prate,
			      int (*enable)(struct clk_hw *hw),
			      void (*disable)(struct clk_hw *hw)) {
	bool need_reenabling = false;

	[...]

	if(pll->flags & SUPPORTS_DYNAMIC_UPDATE) {
		[...]
	} else {
		if (clk_hw_is_enabled(hw)) {
			disable(hw);
			need_reenabling = true;
		}
		[...]
	}

	[...]

	if (need_reenabling)
		enable(hw);

}

Cheers,

--
Julien Thierry

From: Rajendra Nayak <[email protected]>

Each of the CPU clusters (Power and Perf) on msm8996 are clocked via 2 PLLs, a primary and alternate. There are also 2 Mux'es, a primary and secondary all connected together as shown below

                             +-------+
              XO             |       |
          +------------------>0      |
                             |       |
                   PLL/2     | SMUX  +----+
                     +------->1      |    |
                     |       |       |    |
                     |       +-------+    |    +-------+
                     |                    +---->0      |
                     |                         |       |
+---------------+    |             +----------->1      | CPU clk
|Primary PLL    +----+ PLL_EARLY   |           |       +------>
|               +------+-----------+    +------>2 PMUX |
+---------------+      |                |      |       |
                       |   +------+     |   +-->3      |
                       +--^+  ACD +-----+   |  +-------+
+---------------+          +------+         |
|Alt PLL        |                           |
|               +---------------------------+
+---------------+         PLL_EARLY

The primary PLL is what drives the CPU clk, except for times when we are reprogramming the PLL itself (for rate changes) when we temporarily switch to an alternate PLL. A subsequent patch adds support to switch between primary and alternate PLL during rate changes.

The primary PLL operates on a single VCO range, between 600Mhz and 3Ghz. However the CPUs do support OPPs with frequencies between 300Mhz and 600Mhz. In order to support running the CPUs at those frequencies we end up having to lock the PLL at twice the rate and drive the CPU clk via the PLL/2 output and SMUX.

So for frequencies above 600Mhz we follow the following path
Primary PLL --> PLL_EARLY --> PMUX(1) --> CPU clk
and for frequencies between 300Mhz and 600Mhz we follow
Primary PLL --> PLL/2 --> SMUX(1) --> PMUX(0) --> CPU clk
Support for this is added in a subsequent patch as well.

ACD stands for Adaptive Clock Distribution and is used to detect voltage droops. We do not add support for ACD as yet. This can be added at a later point as needed.

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
 .../devicetree/bindings/clock/qcom,kryocc.txt      |  17 +

 drivers/clk/qcom/Kconfig                           |   8 +
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/clk-cpu-8996.c                    | 388 +++++++++++++++++++++
 4 files changed, 414 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,kryocc.txt
 create mode 100644 drivers/clk/qcom/clk-cpu-8996.c

-----Original Message-----
From: Rob Herring [mailto:robh at kernel.org]
Sent: Saturday, December 16, 2017 12:35 AM
To: Ilia Lin <[email protected]>
Cc: linux-clk at vger.kernel.org; linux-arm-kernel at lists.infradead.org;

arm-msm at vger.kernel.org; sboyd at codeaurora.org;
devicetree at vger.kernel.org; mark.rutland at arm.com; will.deacon at arm.com; rnayak at codeaurora.org; qualcomm-
lt at lists.linaro.org; celster at codeaurora.org; tfinkel at codeaurora.org
Subject: Re: [PATCH 04/10] clk: qcom: Add CPU clock driver for msm8996

Quoted 28 lines · Ilia Lin

From: Rajendra Nayak <[email protected]>

Each of the CPU clusters (Power and Perf) on msm8996 are clocked via 2 PLLs, a primary and alternate. There are also
2 Mux'es, a primary and secondary all connected together as shown below

                             +-------+
              XO             |       |
          +------------------>0      |
                             |       |
                   PLL/2     | SMUX  +----+
                     +------->1      |    |
                     |       |       |    |
                     |       +-------+    |    +-------+
                     |                    +---->0      |
                     |                         |       |
+---------------+    |             +----------->1      | CPU clk
|Primary PLL    +----+ PLL_EARLY   |           |       +------>
|               +------+-----------+    +------>2 PMUX |
+---------------+      |                |      |       |
                       |   +------+     |   +-->3      |
                       +--^+  ACD +-----+   |  +-------+
+---------------+          +------+         |
|Alt PLL        |                           |
|               +---------------------------+
+---------------+         PLL_EARLY

Quoted 8 lines

The primary PLL is what drives the CPU clk, except for times when we are reprogramming the PLL itself (for rate changes) when we temporarily switch to an alternate PLL. A subsequent patch adds support to switch between primary and alternate PLL during rate changes.

The primary PLL operates on a single VCO range, between 600Mhz and 3Ghz. However the CPUs do support OPPs with frequencies between

300Mhz

Quoted 10 lines

and 600Mhz. In order to support running the CPUs at those frequencies we end up having to lock the PLL at twice the rate and drive the CPU clk via the PLL/2 output and SMUX.

So for frequencies above 600Mhz we follow the following path Primary PLL --> PLL_EARLY --> PMUX(1) --> CPU clk and for frequencies between 300Mhz and 600Mhz we follow Primary PLL --> PLL/2 --> SMUX(1) --> PMUX(0) --> CPU clk Support for this is added in a subsequent patch as well.

Quoted 8 lines

ACD stands for Adaptive Clock Distribution and is used to detect voltage droops. We do not add support for ACD as yet. This can be added at a later point as needed.

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
 .../devicetree/bindings/clock/qcom,kryocc.txt      |  17 +

If you respin, please make bindings a separate patch. In any case,

Reviewed-by Rob Herring <[email protected]>

Quoted 3 lines

 drivers/clk/qcom/Kconfig                           |   8 +
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/clk-cpu-8996.c                    | 388

+++++++++++++++++++++

Quoted 4 lines

 4 files changed, 414 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/clock/qcom,kryocc.txt
 create mode 100644 drivers/clk/qcom/clk-cpu-8996.c