[PATCH 00/13] clk: qcom: cpu-8996: stability fixes

[PATCH 00/13] clk: qcom: cpu-8996: stability fixes

[Dmitry Baryshkov]

This series provides stability fixes for the MSM8996 boot process. It
changes the order of calls during the CPU PLL setup, makes it use GPLL0
(through sys_apcs_aux) during PLL init, finetunes the ACD, etc.

Dependencies: [1], [2]

[1] https://lore.kernel.org/linux-arm-msm/20230111191453.2509468-1-dmitry.baryshkov@linaro.org/
[2] https://lore.kernel.org/linux-arm-msm/20230111191634.2509616-1-dmitry.baryshkov@linaro.org/


Dmitry Baryshkov (13):
  clk: qcom: clk-alpha-pll: program PLL_TEST/PLL_TEST_U if required
  clk: qcom: cpu-8996: correct PLL programming
  clk: qcom: cpu-8996: fix the init clock rate
  clk: qcom: cpu-8996: support using GPLL0 as SMUX input
  clk: qcom: cpu-8996: skip ACD init if the setup is valid
  clk: qcom: cpu-8996: simplify the cpu_clk_notifier_cb
  clk: qcom: cpu-8996: setup PLLs before registering clocks
  clk: qcom: cpu-8996: move qcom_cpu_clk_msm8996_acd_init call
  clk: qcom: cpu-8996: fix PLL configuration sequence
  clk: qcom: cpu-8996: fix ACD initialization
  clk: qcom: cpu-8996: fix PLL clock ops
  clk: qcom: cpu-8996: change setup sequence to follow vendor kernel
  arm64: dts: qcom: msm8996: support using GPLL0 as kryocc input

 arch/arm64/boot/dts/qcom/msm8996.dtsi |   4 +-
 drivers/clk/qcom/clk-alpha-pll.c      |   5 +
 drivers/clk/qcom/clk-cpu-8996.c       | 145 ++++++++++++++++++--------
 3 files changed, 111 insertions(+), 43 deletions(-)

-- 
2.30.2

[PATCH 01/13] clk: qcom: clk-alpha-pll: program PLL_TEST/PLL_TEST_U if required

[Dmitry Baryshkov]

Program PLL_TEST and PLL_TEST_U registers if required by the pll
configuration.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index f9e4cfd7261c..e266379427f2 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -358,6 +358,11 @@ void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 
 	regmap_update_bits(regmap, PLL_USER_CTL(pll), mask, val);
 
+	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL(pll),
+						config->test_ctl_val);
+	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U(pll),
+						config->test_ctl_hi_val);
+
 	if (pll->flags & SUPPORTS_FSM_MODE)
 		qcom_pll_set_fsm_mode(regmap, PLL_MODE(pll), 6, 0);
 }
-- 
2.30.2

[PATCH 02/13] clk: qcom: cpu-8996: correct PLL programming

[Dmitry Baryshkov]

Change PLL programming to follow the downstream setup.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index ee76ef958d31..ed8cb558e1aa 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -93,12 +93,9 @@ static const u8 prim_pll_regs[PLL_OFF_MAX_REGS] = {
 static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
 	[PLL_OFF_L_VAL] = 0x04,
 	[PLL_OFF_ALPHA_VAL] = 0x08,
-	[PLL_OFF_ALPHA_VAL_U] = 0x0c,
 	[PLL_OFF_USER_CTL] = 0x10,
-	[PLL_OFF_USER_CTL_U] = 0x14,
 	[PLL_OFF_CONFIG_CTL] = 0x18,
 	[PLL_OFF_TEST_CTL] = 0x20,
-	[PLL_OFF_TEST_CTL_U] = 0x24,
 	[PLL_OFF_STATUS] = 0x28,
 };
 
@@ -106,8 +103,10 @@ static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
 
 static const struct alpha_pll_config hfpll_config = {
 	.l = 60,
-	.config_ctl_val = 0x200d4aa8,
+	.config_ctl_val = 0x200d4828,
 	.config_ctl_hi_val = 0x006,
+	.test_ctl_val = 0x1c000000,
+	.test_ctl_hi_val = 0x00004000,
 	.pre_div_mask = BIT(12),
 	.post_div_mask = 0x3 << 8,
 	.post_div_val = 0x1 << 8,
-- 
2.30.2

[PATCH 03/13] clk: qcom: cpu-8996: fix the init clock rate

[Dmitry Baryshkov]

Change PLL programming to let both power and performance cluster clocks
to start from the maximum common frequency.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index ed8cb558e1aa..d51965fda56d 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -102,7 +102,7 @@ static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
 /* PLLs */
 
 static const struct alpha_pll_config hfpll_config = {
-	.l = 60,
+	.l = 54,
 	.config_ctl_val = 0x200d4828,
 	.config_ctl_hi_val = 0x006,
 	.test_ctl_val = 0x1c000000,
-- 
2.30.2

[PATCH 04/13] clk: qcom: cpu-8996: support using GPLL0 as SMUX input

[Dmitry Baryshkov]

In some cases the driver might need using GPLL0 to drive CPU clocks.
Bring it in through the sys_apcs_aux clock.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index d51965fda56d..0e0c00d44c6f 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -12,6 +12,8 @@
  *                              +-------+
  *               XO             |       |
  *           +------------------>0      |
+ *               SYS_APCS_AUX   |       |
+ *           +------------------>3      |
  *                              |       |
  *                    PLL/2     | SMUX  +----+
  *                      +------->1      |    |
@@ -310,20 +312,29 @@ static const struct clk_ops clk_cpu_8996_pmux_ops = {
 	.determine_rate = clk_cpu_8996_pmux_determine_rate,
 };
 
+static const struct parent_map smux_parent_map[] = {
+	{ .cfg = 0, }, /* xo */
+	{ .cfg = 1, }, /* pll */
+	{ .cfg = 3, }, /* sys_apcs_aux */
+};
+
 static const struct clk_parent_data pwrcl_smux_parents[] = {
 	{ .fw_name = "xo" },
 	{ .hw = &pwrcl_pll_postdiv.hw },
+	{ .fw_name = "sys_apcs_aux" },
 };
 
 static const struct clk_parent_data perfcl_smux_parents[] = {
 	{ .fw_name = "xo" },
 	{ .hw = &perfcl_pll_postdiv.hw },
+	{ .fw_name = "sys_apcs_aux" },
 };
 
 static struct clk_regmap_mux pwrcl_smux = {
 	.reg = PWRCL_REG_OFFSET + MUX_OFFSET,
 	.shift = 2,
 	.width = 2,
+	.parent_map = smux_parent_map,
 	.clkr.hw.init = &(struct clk_init_data) {
 		.name = "pwrcl_smux",
 		.parent_data = pwrcl_smux_parents,
@@ -337,6 +348,7 @@ static struct clk_regmap_mux perfcl_smux = {
 	.reg = PERFCL_REG_OFFSET + MUX_OFFSET,
 	.shift = 2,
 	.width = 2,
+	.parent_map = smux_parent_map,
 	.clkr.hw.init = &(struct clk_init_data) {
 		.name = "perfcl_smux",
 		.parent_data = perfcl_smux_parents,
-- 
2.30.2

[PATCH 05/13] clk: qcom: cpu-8996: skip ACD init if the setup is valid

[Dmitry Baryshkov]

Check whether L2 registers contain correct values and skip programming
if they are valid. This follows the code present downstream.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index 0e0c00d44c6f..7e5246ca7e7f 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -472,10 +472,15 @@ static void __iomem *base;
 static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
 {
 	u64 hwid;
+	u32 val;
 	unsigned long flags;
 
 	spin_lock_irqsave(&qcom_clk_acd_lock, flags);
 
+	val = kryo_l2_get_indirect_reg(L2ACDTD_REG);
+	if (val == 0x00006a11)
+		goto out;
+
 	hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
 
 	kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
@@ -492,6 +497,7 @@ static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
 		writel(0xf, base + PERFCL_REG_OFFSET + SSSCTL_OFFSET);
 	}
 
+out:
 	spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);
 }
 
-- 
2.30.2

[PATCH 06/13] clk: qcom: cpu-8996: simplify the cpu_clk_notifier_cb

[Dmitry Baryshkov]

- Do not use the Alt PLL completely. Switch to smux when necessary to
  prevent overvolting
- Restore the parent in case the rate change aborts for some reason
- Do not duplicate resetting the parent in set_parent operation.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index 7e5246ca7e7f..ee7e18b37832 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -506,27 +506,34 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
 {
 	struct clk_cpu_8996_pmux *cpuclk = to_clk_cpu_8996_pmux_nb(nb);
 	struct clk_notifier_data *cnd = data;
-	int ret;
 
 	switch (event) {
 	case PRE_RATE_CHANGE:
-		ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ALT_INDEX);
 		qcom_cpu_clk_msm8996_acd_init(base);
+
+		/*
+		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
+		 * to bottom, so it will change the rate of the PLL before
+		 * chaging the parent of PMUX. This can result in pmux getting
+		 * clocked twice the expected rate.
+		 *
+		 * Manually switch to PLL/2 here.
+		 */
+		if (cnd->new_rate < DIV_2_THRESHOLD &&
+		    cnd->old_rate > DIV_2_THRESHOLD)
+			clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, SMUX_INDEX);
+
 		break;
-	case POST_RATE_CHANGE:
-		if (cnd->new_rate < DIV_2_THRESHOLD)
-			ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
-							   SMUX_INDEX);
-		else
-			ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
-							   ACD_INDEX);
-		break;
+	case ABORT_RATE_CHANGE:
+		/* Revert manual change */
+		if (cnd->new_rate < DIV_2_THRESHOLD &&
+		    cnd->old_rate > DIV_2_THRESHOLD)
+			clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ACD_INDEX);
 	default:
-		ret = 0;
 		break;
 	}
 
-	return notifier_from_errno(ret);
+	return NOTIFY_OK;
 };
 
 static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
-- 
2.30.2

[PATCH 07/13] clk: qcom: cpu-8996: setup PLLs before registering clocks

[Dmitry Baryshkov]

Setup all PLLs before registering clocks in the common clock framework.
This ensures that the clocks are not accessed before being setup in the
known way and that the CCF is in sync with the actual HW programming.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index ee7e18b37832..e390f4aadff1 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -430,6 +430,11 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 {
 	int i, ret;
 
+	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
+	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
+	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
+	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
+
 	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
 		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
 		if (ret)
@@ -442,11 +447,6 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 			return ret;
 	}
 
-	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
-	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
-	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
-	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
-
 	/* Enable alt PLLs */
 	clk_prepare_enable(pwrcl_alt_pll.clkr.hw.clk);
 	clk_prepare_enable(perfcl_alt_pll.clkr.hw.clk);
-- 
2.30.2

[PATCH 08/13] clk: qcom: cpu-8996: move qcom_cpu_clk_msm8996_acd_init call

[Dmitry Baryshkov]

Initialize ACD configuration from qcom_cpu_clk_msm8996_register_clks(),
before registering all clocks. This way we can be sure that the clock is
fully configured before letting CCF touch it.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index e390f4aadff1..571ed52b3026 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -425,6 +425,8 @@ static struct clk_regmap *cpu_msm8996_clks[] = {
 	&perfcl_pmux.clkr,
 };
 
+static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap);
+
 static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 					      struct regmap *regmap)
 {
@@ -435,6 +437,8 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
 	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
 
+	qcom_cpu_clk_msm8996_acd_init(regmap);
+
 	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
 		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
 		if (ret)
@@ -467,9 +471,8 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 #define L2ACDSSCR_REG 0x589ULL
 
 static DEFINE_SPINLOCK(qcom_clk_acd_lock);
-static void __iomem *base;
 
-static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
+static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap)
 {
 	u64 hwid;
 	u32 val;
@@ -488,13 +491,13 @@ static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
 	kryo_l2_set_indirect_reg(L2ACDSSCR_REG, 0x00000601);
 
 	if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
-		writel(0xf, base + PWRCL_REG_OFFSET + SSSCTL_OFFSET);
+		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
 		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
 	}
 
 	if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) {
 		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
-		writel(0xf, base + PERFCL_REG_OFFSET + SSSCTL_OFFSET);
+		regmap_write(regmap, PERFCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
 	}
 
 out:
@@ -509,7 +512,7 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
 
 	switch (event) {
 	case PRE_RATE_CHANGE:
-		qcom_cpu_clk_msm8996_acd_init(base);
+		qcom_cpu_clk_msm8996_acd_init(cpuclk->clkr.regmap);
 
 		/*
 		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
@@ -538,6 +541,7 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
 
 static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
 {
+	static void __iomem *base;
 	struct regmap *regmap;
 	struct clk_hw_onecell_data *data;
 	struct device *dev = &pdev->dev;
@@ -559,8 +563,6 @@ static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	qcom_cpu_clk_msm8996_acd_init(base);
-
 	data->hws[0] = &pwrcl_pmux.clkr.hw;
 	data->hws[1] = &perfcl_pmux.clkr.hw;
 	data->num = 2;
-- 
2.30.2

[PATCH 09/13] clk: qcom: cpu-8996: fix PLL configuration sequence

[Dmitry Baryshkov]

Switch both power and performance clocks to the GPLL0/2 (sys_apcs_aux)
before PLL configuration. Switch them to the ACD afterwards.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index 571ed52b3026..47c58bb5f21a 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -432,13 +432,27 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 {
 	int i, ret;
 
+	/* Select GPLL0 for 300MHz for the both clusters */
+	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0xc);
+	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0xc);
+
+	/* Ensure write goes through before PLLs are reconfigured */
+	udelay(5);
+
 	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
 	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
 	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
 	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
 
+	/* Wait for PLL(s) to lock */
+        udelay(50);
+
 	qcom_cpu_clk_msm8996_acd_init(regmap);
 
+	/* Switch clusters to use the ACD leg */
+	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
+	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
+
 	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
 		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
 		if (ret)
-- 
2.30.2

[PATCH 10/13] clk: qcom: cpu-8996: fix ACD initialization

[Dmitry Baryshkov]

The vendor kernel applies different order while programming SSSCTL and
L2ACDCR registers on power and performance clusters. However it was
demonstrated that doing this upstream results in the board reset. Make
both clusters use the same sequence, which fixes the reset.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index 47c58bb5f21a..1c00eb629b61 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -475,9 +475,9 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 	return ret;
 }
 
-#define CPU_AFINITY_MASK 0xFFF
-#define PWRCL_CPU_REG_MASK 0x3
-#define PERFCL_CPU_REG_MASK 0x103
+#define CPU_CLUSTER_AFFINITY_MASK 0xf00
+#define PWRCL_AFFINITY_MASK 0x000
+#define PERFCL_AFFINITY_MASK 0x100
 
 #define L2ACDCR_REG 0x580ULL
 #define L2ACDTD_REG 0x581ULL
@@ -498,21 +498,17 @@ static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap)
 	if (val == 0x00006a11)
 		goto out;
 
-	hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
-
 	kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
 	kryo_l2_set_indirect_reg(L2ACDDVMRC_REG, 0x000e0f0f);
 	kryo_l2_set_indirect_reg(L2ACDSSCR_REG, 0x00000601);
 
-	if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
-		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
-		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
-	}
+	kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
 
-	if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) {
-		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
+	hwid = read_cpuid_mpidr();
+	if ((hwid & CPU_CLUSTER_AFFINITY_MASK) == PWRCL_AFFINITY_MASK)
+		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
+	else
 		regmap_write(regmap, PERFCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
-	}
 
 out:
 	spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);
-- 
2.30.2

[PATCH 11/13] clk: qcom: cpu-8996: fix PLL clock ops

[Dmitry Baryshkov]

Switch CPU PLLs to use clk_alpha_pll_hwfsm_ops, it seems to suit
better.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index 1c00eb629b61..b53cddc4bca3 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -128,7 +128,7 @@ static struct clk_alpha_pll pwrcl_pll = {
 		.name = "pwrcl_pll",
 		.parent_data = pll_parent,
 		.num_parents = ARRAY_SIZE(pll_parent),
-		.ops = &clk_alpha_pll_huayra_ops,
+		.ops = &clk_alpha_pll_hwfsm_ops,
 	},
 };
 
@@ -140,7 +140,7 @@ static struct clk_alpha_pll perfcl_pll = {
 		.name = "perfcl_pll",
 		.parent_data = pll_parent,
 		.num_parents = ARRAY_SIZE(pll_parent),
-		.ops = &clk_alpha_pll_huayra_ops,
+		.ops = &clk_alpha_pll_hwfsm_ops,
 	},
 };
 
-- 
2.30.2

[PATCH 12/13] clk: qcom: cpu-8996: change setup sequence to follow vendor kernel

[Dmitry Baryshkov]

Add missing register writes to CPU clocks setup procedure. This makes it
follow the setup procedure used in msm-3.18 kernel.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cpu-8996.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
index b53cddc4bca3..78a18b95c48b 100644
--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c
@@ -76,10 +76,16 @@ enum _pmux_input {
 #define PWRCL_REG_OFFSET 0x0
 #define PERFCL_REG_OFFSET 0x80000
 #define MUX_OFFSET	0x40
+#define CLK_CTL_OFFSET 0x44
+#define CLK_CTL_AUTO_CLK_SEL BIT(8)
 #define ALT_PLL_OFFSET	0x100
 #define SSSCTL_OFFSET 0x160
+#define PSCTL_OFFSET 0x164
 
 #define PMUX_MASK	0x3
+#define MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK GENMASK(5, 4)
+#define MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL \
+	FIELD_PREP(MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK, 0x03)
 
 static const u8 prim_pll_regs[PLL_OFF_MAX_REGS] = {
 	[PLL_OFF_L_VAL] = 0x04,
@@ -439,6 +445,14 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 	/* Ensure write goes through before PLLs are reconfigured */
 	udelay(5);
 
+	/* Set the auto clock sel always-on source to GPLL0/2 (300MHz) */
+	regmap_update_bits(regmap, PWRCL_REG_OFFSET + MUX_OFFSET,
+			   MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
+			   MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
+	regmap_update_bits(regmap, PERFCL_REG_OFFSET + MUX_OFFSET,
+			   MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
+			   MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
+
 	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
 	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
 	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
@@ -447,11 +461,24 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
 	/* Wait for PLL(s) to lock */
         udelay(50);
 
+	/* Enable auto clock selection for both clusters */
+	regmap_update_bits(regmap, PWRCL_REG_OFFSET + CLK_CTL_OFFSET,
+			   CLK_CTL_AUTO_CLK_SEL, CLK_CTL_AUTO_CLK_SEL);
+	regmap_update_bits(regmap, PERFCL_REG_OFFSET + CLK_CTL_OFFSET,
+			   CLK_CTL_AUTO_CLK_SEL, CLK_CTL_AUTO_CLK_SEL);
+
+	/* Ensure write goes through before muxes are switched */
+	udelay(5);
+
 	qcom_cpu_clk_msm8996_acd_init(regmap);
 
+	/* Pulse swallower and soft-start settings */
+	regmap_write(regmap, PWRCL_REG_OFFSET + PSCTL_OFFSET, 0x00030005);
+	regmap_write(regmap, PERFCL_REG_OFFSET + PSCTL_OFFSET, 0x00030005);
+
 	/* Switch clusters to use the ACD leg */
-	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
-	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
+	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x32);
+	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x32);
 
 	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
 		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
-- 
2.30.2

[PATCH 13/13] arm64: dts: qcom: msm8996: support using GPLL0 as kryocc input

[Dmitry Baryshkov]

In some cases the driver might need using GPLL0 to drive CPU clocks.
Bring it in through the sys_apcs_aux clock.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index d52023c19682..4e3068bedf4c 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -2940,8 +2940,8 @@ kryocc: clock-controller@6400000 {
 			compatible = "qcom,msm8996-apcc";
 			reg = <0x06400000 0x90000>;
 
-			clock-names = "xo";
-			clocks = <&rpmcc RPM_SMD_BB_CLK1>;
+			clock-names = "xo", "sys_apcs_aux";
+			clocks = <&rpmcc RPM_SMD_BB_CLK1>, <&apcs_glb>;
 
 			#clock-cells = <1>;
 		};
-- 
2.30.2

Re: [PATCH 04/13] clk: qcom: cpu-8996: support using GPLL0 as SMUX input

[Stephen Boyd]

Quoting Dmitry Baryshkov (2023-01-11 11:19:55)
> In some cases the driver might need using GPLL0 to drive CPU clocks.
> Bring it in through the sys_apcs_aux clock.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/clk-cpu-8996.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index d51965fda56d..0e0c00d44c6f 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -12,6 +12,8 @@
>   *                              +-------+
>   *               XO             |       |
>   *           +------------------>0      |
> + *               SYS_APCS_AUX   |       |
> + *           +------------------>3      |
>   *                              |       |
>   *                    PLL/2     | SMUX  +----+
>   *                      +------->1      |    |
> @@ -310,20 +312,29 @@ static const struct clk_ops clk_cpu_8996_pmux_ops = {
>         .determine_rate = clk_cpu_8996_pmux_determine_rate,
>  };
>  
> +static const struct parent_map smux_parent_map[] = {
> +       { .cfg = 0, }, /* xo */
> +       { .cfg = 1, }, /* pll */
> +       { .cfg = 3, }, /* sys_apcs_aux */
> +};
> +
>  static const struct clk_parent_data pwrcl_smux_parents[] = {
>         { .fw_name = "xo" },
>         { .hw = &pwrcl_pll_postdiv.hw },
> +       { .fw_name = "sys_apcs_aux" },

Is there a binding update?

Re: [PATCH 04/13] clk: qcom: cpu-8996: support using GPLL0 as SMUX input

[Dmitry Baryshkov]

On 11/01/2023 21:26, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2023-01-11 11:19:55)
>> In some cases the driver might need using GPLL0 to drive CPU clocks.
>> Bring it in through the sys_apcs_aux clock.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/clk-cpu-8996.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> index d51965fda56d..0e0c00d44c6f 100644
>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -12,6 +12,8 @@
>>    *                              +-------+
>>    *               XO             |       |
>>    *           +------------------>0      |
>> + *               SYS_APCS_AUX   |       |
>> + *           +------------------>3      |
>>    *                              |       |
>>    *                    PLL/2     | SMUX  +----+
>>    *                      +------->1      |    |
>> @@ -310,20 +312,29 @@ static const struct clk_ops clk_cpu_8996_pmux_ops = {
>>          .determine_rate = clk_cpu_8996_pmux_determine_rate,
>>   };
>>   
>> +static const struct parent_map smux_parent_map[] = {
>> +       { .cfg = 0, }, /* xo */
>> +       { .cfg = 1, }, /* pll */
>> +       { .cfg = 3, }, /* sys_apcs_aux */
>> +};
>> +
>>   static const struct clk_parent_data pwrcl_smux_parents[] = {
>>          { .fw_name = "xo" },
>>          { .hw = &pwrcl_pll_postdiv.hw },
>> +       { .fw_name = "sys_apcs_aux" },
> 
> Is there a binding update?

Argh, missed the patch. I knew that 13 is not a right number for the 
patch count!

-- 
With best wishes
Dmitry

Re: [PATCH 02/13] clk: qcom: cpu-8996: correct PLL programming

[Konrad Dybcio]

On 11.01.2023 20:19, Dmitry Baryshkov wrote:
> Change PLL programming to follow the downstream setup.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index ee76ef958d31..ed8cb558e1aa 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -93,12 +93,9 @@ static const u8 prim_pll_regs[PLL_OFF_MAX_REGS] = {
>  static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
>  	[PLL_OFF_L_VAL] = 0x04,
>  	[PLL_OFF_ALPHA_VAL] = 0x08,
> -	[PLL_OFF_ALPHA_VAL_U] = 0x0c,
>  	[PLL_OFF_USER_CTL] = 0x10,
> -	[PLL_OFF_USER_CTL_U] = 0x14,
>  	[PLL_OFF_CONFIG_CTL] = 0x18,
>  	[PLL_OFF_TEST_CTL] = 0x20,
> -	[PLL_OFF_TEST_CTL_U] = 0x24,
>  	[PLL_OFF_STATUS] = 0x28,
>  };
>  
> @@ -106,8 +103,10 @@ static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
>  
>  static const struct alpha_pll_config hfpll_config = {
>  	.l = 60,
> -	.config_ctl_val = 0x200d4aa8,
> +	.config_ctl_val = 0x200d4828,
>  	.config_ctl_hi_val = 0x006,
> +	.test_ctl_val = 0x1c000000,
> +	.test_ctl_hi_val = 0x00004000,
>  	.pre_div_mask = BIT(12),
>  	.post_div_mask = 0x3 << 8,
>  	.post_div_val = 0x1 << 8,

Re: [PATCH 03/13] clk: qcom: cpu-8996: fix the init clock rate

[Konrad Dybcio]

On 11.01.2023 20:19, Dmitry Baryshkov wrote:
> Change PLL programming to let both power and performance cluster clocks
> to start from the maximum common frequency.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Can you point me to the source of this? My local random msm-3.18 has this at 60.

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index ed8cb558e1aa..d51965fda56d 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -102,7 +102,7 @@ static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
>  /* PLLs */
>  
>  static const struct alpha_pll_config hfpll_config = {
> -	.l = 60,
> +	.l = 54,
>  	.config_ctl_val = 0x200d4828,
>  	.config_ctl_hi_val = 0x006,
>  	.test_ctl_val = 0x1c000000,

Re: [PATCH 04/13] clk: qcom: cpu-8996: support using GPLL0 as SMUX input

[Konrad Dybcio]

On 11.01.2023 20:19, Dmitry Baryshkov wrote:
> In some cases the driver might need using GPLL0 to drive CPU clocks.
> Bring it in through the sys_apcs_aux clock.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Oh that's new.. downstream doesn't talk about this..

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index d51965fda56d..0e0c00d44c6f 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -12,6 +12,8 @@
>   *                              +-------+
>   *               XO             |       |
>   *           +------------------>0      |
> + *               SYS_APCS_AUX   |       |
> + *           +------------------>3      |
>   *                              |       |
>   *                    PLL/2     | SMUX  +----+
>   *                      +------->1      |    |
> @@ -310,20 +312,29 @@ static const struct clk_ops clk_cpu_8996_pmux_ops = {
>  	.determine_rate = clk_cpu_8996_pmux_determine_rate,
>  };
>  
> +static const struct parent_map smux_parent_map[] = {
> +	{ .cfg = 0, }, /* xo */
> +	{ .cfg = 1, }, /* pll */
> +	{ .cfg = 3, }, /* sys_apcs_aux */
> +};
> +
>  static const struct clk_parent_data pwrcl_smux_parents[] = {
>  	{ .fw_name = "xo" },
>  	{ .hw = &pwrcl_pll_postdiv.hw },
> +	{ .fw_name = "sys_apcs_aux" },
>  };
>  
>  static const struct clk_parent_data perfcl_smux_parents[] = {
>  	{ .fw_name = "xo" },
>  	{ .hw = &perfcl_pll_postdiv.hw },
> +	{ .fw_name = "sys_apcs_aux" },
>  };
>  
>  static struct clk_regmap_mux pwrcl_smux = {
>  	.reg = PWRCL_REG_OFFSET + MUX_OFFSET,
>  	.shift = 2,
>  	.width = 2,
> +	.parent_map = smux_parent_map,
>  	.clkr.hw.init = &(struct clk_init_data) {
>  		.name = "pwrcl_smux",
>  		.parent_data = pwrcl_smux_parents,
> @@ -337,6 +348,7 @@ static struct clk_regmap_mux perfcl_smux = {
>  	.reg = PERFCL_REG_OFFSET + MUX_OFFSET,
>  	.shift = 2,
>  	.width = 2,
> +	.parent_map = smux_parent_map,
>  	.clkr.hw.init = &(struct clk_init_data) {
>  		.name = "perfcl_smux",
>  		.parent_data = perfcl_smux_parents,

Re: [PATCH 05/13] clk: qcom: cpu-8996: skip ACD init if the setup is valid

[Konrad Dybcio]

On 11.01.2023 20:19, Dmitry Baryshkov wrote:
> Check whether L2 registers contain correct values and skip programming
> if they are valid. This follows the code present downstream.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Once again, my random local msm-3.18 doesn't do this, can you show
me the downstream source for this?

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index 0e0c00d44c6f..7e5246ca7e7f 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -472,10 +472,15 @@ static void __iomem *base;
>  static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
>  {
>  	u64 hwid;
> +	u32 val;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&qcom_clk_acd_lock, flags);
>  
> +	val = kryo_l2_get_indirect_reg(L2ACDTD_REG);
> +	if (val == 0x00006a11)
> +		goto out;
> +
>  	hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
>  
>  	kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
> @@ -492,6 +497,7 @@ static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
>  		writel(0xf, base + PERFCL_REG_OFFSET + SSSCTL_OFFSET);
>  	}
>  
> +out:
>  	spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);
>  }
>  

Re: [PATCH 06/13] clk: qcom: cpu-8996: simplify the cpu_clk_notifier_cb

[Konrad Dybcio]

On 11.01.2023 20:19, Dmitry Baryshkov wrote:
> - Do not use the Alt PLL completely. Switch to smux when necessary to
>   prevent overvolting
Is this empirical evidence, or did Qualcomm recommendations change since
msm-3.18 was released?


> - Restore the parent in case the rate change aborts for some reason
> - Do not duplicate resetting the parent in set_parent operation.
These sound good.

Konrad
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/clk-cpu-8996.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index 7e5246ca7e7f..ee7e18b37832 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -506,27 +506,34 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>  {
>  	struct clk_cpu_8996_pmux *cpuclk = to_clk_cpu_8996_pmux_nb(nb);
>  	struct clk_notifier_data *cnd = data;
> -	int ret;
>  
>  	switch (event) {
>  	case PRE_RATE_CHANGE:
> -		ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ALT_INDEX);
>  		qcom_cpu_clk_msm8996_acd_init(base);
> +
> +		/*
> +		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
> +		 * to bottom, so it will change the rate of the PLL before
> +		 * chaging the parent of PMUX. This can result in pmux getting
> +		 * clocked twice the expected rate.
> +		 *
> +		 * Manually switch to PLL/2 here.
> +		 */
> +		if (cnd->new_rate < DIV_2_THRESHOLD &&
> +		    cnd->old_rate > DIV_2_THRESHOLD)
> +			clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, SMUX_INDEX);
> +
>  		break;
> -	case POST_RATE_CHANGE:
> -		if (cnd->new_rate < DIV_2_THRESHOLD)
> -			ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
> -							   SMUX_INDEX);
> -		else
> -			ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
> -							   ACD_INDEX);
> -		break;
> +	case ABORT_RATE_CHANGE:
> +		/* Revert manual change */
> +		if (cnd->new_rate < DIV_2_THRESHOLD &&
> +		    cnd->old_rate > DIV_2_THRESHOLD)
> +			clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ACD_INDEX);
>  	default:
> -		ret = 0;
>  		break;
>  	}
>  
> -	return notifier_from_errno(ret);
> +	return NOTIFY_OK;
>  };
>  
>  static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)

Re: [PATCH 07/13] clk: qcom: cpu-8996: setup PLLs before registering clocks

[Konrad Dybcio]

On 11.01.2023 20:19, Dmitry Baryshkov wrote:
> Setup all PLLs before registering clocks in the common clock framework.
> This ensures that the clocks are not accessed before being setup in the
> known way and that the CCF is in sync with the actual HW programming.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index ee7e18b37832..e390f4aadff1 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -430,6 +430,11 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  {
>  	int i, ret;
>  
> +	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
> +	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
> +	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
> +	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
> +
>  	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>  		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
>  		if (ret)
> @@ -442,11 +447,6 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  			return ret;
>  	}
>  
> -	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
> -	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
> -	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
> -	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
> -
>  	/* Enable alt PLLs */
>  	clk_prepare_enable(pwrcl_alt_pll.clkr.hw.clk);
>  	clk_prepare_enable(perfcl_alt_pll.clkr.hw.clk);

Re: [PATCH 09/13] clk: qcom: cpu-8996: fix PLL configuration sequence

[Konrad Dybcio]

On 11.01.2023 20:20, Dmitry Baryshkov wrote:
> Switch both power and performance clocks to the GPLL0/2 (sys_apcs_aux)
> before PLL configuration. Switch them to the ACD afterwards.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/clk-cpu-8996.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index 571ed52b3026..47c58bb5f21a 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -432,13 +432,27 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  {
>  	int i, ret;
>  
> +	/* Select GPLL0 for 300MHz for the both clusters */
superfluous 'the'

> +	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0xc);
> +	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0xc);
> +
> +	/* Ensure write goes through before PLLs are reconfigured */
> +	udelay(5);
Is this value based on n clock cycles, or 'good enough'?

> +
>  	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
>  	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>  	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
>  	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
>  
> +	/* Wait for PLL(s) to lock */
> +        udelay(50);
Weird indentation

Maybe wait_for_pll_enable_lock() to be super sure?

> +
>  	qcom_cpu_clk_msm8996_acd_init(regmap);
>  
> +	/* Switch clusters to use the ACD leg */
> +	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
> +	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
> +
No delays here?

Konrad
>  	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>  		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
>  		if (ret)

Re: [PATCH 03/13] clk: qcom: cpu-8996: fix the init clock rate

[Dmitry Baryshkov]

On 11/01/2023 22:58, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>> Change PLL programming to let both power and performance cluster clocks
>> to start from the maximum common frequency.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> Can you point me to the source of this? My local random msm-3.18 has this at 60.

Yes, but with 60 cluster start at the unlisted frequency (60 * 19.2 = 
1152 MHz), which leads to cpufreq whining and immediately performing a 
switch.

I modified this to 54 * 19.2 =  1036.8 MHz which is supported by both 
power and performance clusters. Maybe we could have gone to 58 * 19.2 = 
1113. Mhz or to 62 * 19.2 = 1190.4 MHz, but as all the safety and power 
measures and not probed at this point, I preferred to rather be safe 
than sorry.

> 
> Konrad
>>   drivers/clk/qcom/clk-cpu-8996.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> index ed8cb558e1aa..d51965fda56d 100644
>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -102,7 +102,7 @@ static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
>>   /* PLLs */
>>   
>>   static const struct alpha_pll_config hfpll_config = {
>> -	.l = 60,
>> +	.l = 54,
>>   	.config_ctl_val = 0x200d4828,
>>   	.config_ctl_hi_val = 0x006,
>>   	.test_ctl_val = 0x1c000000,

-- 
With best wishes
Dmitry

Re: [PATCH 04/13] clk: qcom: cpu-8996: support using GPLL0 as SMUX input

[Dmitry Baryshkov]

On 11/01/2023 22:59, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>> In some cases the driver might need using GPLL0 to drive CPU clocks.
>> Bring it in through the sys_apcs_aux clock.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> Oh that's new.. downstream doesn't talk about this..

It does, but under the hood of the init procedure. See:

         /* Select GPLL0 for 300MHz for the perf cluster */
         writel_relaxed(0xC, vbases[APC1_BASE] + MUX_OFFSET);


> 
> Konrad
>>   drivers/clk/qcom/clk-cpu-8996.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> index d51965fda56d..0e0c00d44c6f 100644
>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -12,6 +12,8 @@
>>    *                              +-------+
>>    *               XO             |       |
>>    *           +------------------>0      |
>> + *               SYS_APCS_AUX   |       |
>> + *           +------------------>3      |
>>    *                              |       |
>>    *                    PLL/2     | SMUX  +----+
>>    *                      +------->1      |    |
>> @@ -310,20 +312,29 @@ static const struct clk_ops clk_cpu_8996_pmux_ops = {
>>   	.determine_rate = clk_cpu_8996_pmux_determine_rate,
>>   };
>>   
>> +static const struct parent_map smux_parent_map[] = {
>> +	{ .cfg = 0, }, /* xo */
>> +	{ .cfg = 1, }, /* pll */
>> +	{ .cfg = 3, }, /* sys_apcs_aux */
>> +};
>> +
>>   static const struct clk_parent_data pwrcl_smux_parents[] = {
>>   	{ .fw_name = "xo" },
>>   	{ .hw = &pwrcl_pll_postdiv.hw },
>> +	{ .fw_name = "sys_apcs_aux" },
>>   };
>>   
>>   static const struct clk_parent_data perfcl_smux_parents[] = {
>>   	{ .fw_name = "xo" },
>>   	{ .hw = &perfcl_pll_postdiv.hw },
>> +	{ .fw_name = "sys_apcs_aux" },
>>   };
>>   
>>   static struct clk_regmap_mux pwrcl_smux = {
>>   	.reg = PWRCL_REG_OFFSET + MUX_OFFSET,
>>   	.shift = 2,
>>   	.width = 2,
>> +	.parent_map = smux_parent_map,
>>   	.clkr.hw.init = &(struct clk_init_data) {
>>   		.name = "pwrcl_smux",
>>   		.parent_data = pwrcl_smux_parents,
>> @@ -337,6 +348,7 @@ static struct clk_regmap_mux perfcl_smux = {
>>   	.reg = PERFCL_REG_OFFSET + MUX_OFFSET,
>>   	.shift = 2,
>>   	.width = 2,
>> +	.parent_map = smux_parent_map,
>>   	.clkr.hw.init = &(struct clk_init_data) {
>>   		.name = "perfcl_smux",
>>   		.parent_data = perfcl_smux_parents,

-- 
With best wishes
Dmitry

Re: [PATCH 05/13] clk: qcom: cpu-8996: skip ACD init if the setup is valid

[Dmitry Baryshkov]

On 11/01/2023 23:00, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>> Check whether L2 registers contain correct values and skip programming
>> if they are valid. This follows the code present downstream.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> Once again, my random local msm-3.18 doesn't do this, can you show
> me the downstream source for this?

https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L856

> 
> Konrad
>>   drivers/clk/qcom/clk-cpu-8996.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> index 0e0c00d44c6f..7e5246ca7e7f 100644
>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -472,10 +472,15 @@ static void __iomem *base;
>>   static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
>>   {
>>   	u64 hwid;
>> +	u32 val;
>>   	unsigned long flags;
>>   
>>   	spin_lock_irqsave(&qcom_clk_acd_lock, flags);
>>   
>> +	val = kryo_l2_get_indirect_reg(L2ACDTD_REG);
>> +	if (val == 0x00006a11)
>> +		goto out;
>> +
>>   	hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
>>   
>>   	kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
>> @@ -492,6 +497,7 @@ static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
>>   		writel(0xf, base + PERFCL_REG_OFFSET + SSSCTL_OFFSET);
>>   	}
>>   
>> +out:
>>   	spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);
>>   }
>>   

-- 
With best wishes
Dmitry

Re: [PATCH 06/13] clk: qcom: cpu-8996: simplify the cpu_clk_notifier_cb

[Dmitry Baryshkov]

On 11/01/2023 23:03, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>> - Do not use the Alt PLL completely. Switch to smux when necessary to
>>    prevent overvolting
> Is this empirical evidence, or did Qualcomm recommendations change since
> msm-3.18 was released?

I think this is what they are doing, see 
https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L675

They switch altpll frequency for whatever reasons, then they do the 
dance of switching the parent rate to half rate if necessary and then 
they finally switch the parent's rate to the target rate. That's the 
only way I can interpret the cpu_clk_8996_set_rate().

https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L675

> 
> 
>> - Restore the parent in case the rate change aborts for some reason
>> - Do not duplicate resetting the parent in set_parent operation.
> These sound good.
> 
> Konrad
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/clk-cpu-8996.c | 31 +++++++++++++++++++------------
>>   1 file changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> index 7e5246ca7e7f..ee7e18b37832 100644
>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -506,27 +506,34 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>>   {
>>   	struct clk_cpu_8996_pmux *cpuclk = to_clk_cpu_8996_pmux_nb(nb);
>>   	struct clk_notifier_data *cnd = data;
>> -	int ret;
>>   
>>   	switch (event) {
>>   	case PRE_RATE_CHANGE:
>> -		ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ALT_INDEX);
>>   		qcom_cpu_clk_msm8996_acd_init(base);
>> +
>> +		/*
>> +		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
>> +		 * to bottom, so it will change the rate of the PLL before
>> +		 * chaging the parent of PMUX. This can result in pmux getting
>> +		 * clocked twice the expected rate.
>> +		 *
>> +		 * Manually switch to PLL/2 here.
>> +		 */
>> +		if (cnd->new_rate < DIV_2_THRESHOLD &&
>> +		    cnd->old_rate > DIV_2_THRESHOLD)
>> +			clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, SMUX_INDEX);
>> +
>>   		break;
>> -	case POST_RATE_CHANGE:
>> -		if (cnd->new_rate < DIV_2_THRESHOLD)
>> -			ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
>> -							   SMUX_INDEX);
>> -		else
>> -			ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
>> -							   ACD_INDEX);
>> -		break;
>> +	case ABORT_RATE_CHANGE:
>> +		/* Revert manual change */
>> +		if (cnd->new_rate < DIV_2_THRESHOLD &&
>> +		    cnd->old_rate > DIV_2_THRESHOLD)
>> +			clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ACD_INDEX);
>>   	default:
>> -		ret = 0;
>>   		break;
>>   	}
>>   
>> -	return notifier_from_errno(ret);
>> +	return NOTIFY_OK;
>>   };
>>   
>>   static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)

-- 
With best wishes
Dmitry

Re: [PATCH 09/13] clk: qcom: cpu-8996: fix PLL configuration sequence

[Dmitry Baryshkov]

On 11/01/2023 23:08, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>> Switch both power and performance clocks to the GPLL0/2 (sys_apcs_aux)
>> before PLL configuration. Switch them to the ACD afterwards.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/clk-cpu-8996.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> index 571ed52b3026..47c58bb5f21a 100644
>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -432,13 +432,27 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>>   {
>>   	int i, ret;
>>   
>> +	/* Select GPLL0 for 300MHz for the both clusters */
> superfluous 'the'
> 
>> +	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0xc);
>> +	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0xc);
>> +
>> +	/* Ensure write goes through before PLLs are reconfigured */
>> +	udelay(5);
> Is this value based on n clock cycles, or 'good enough'?

Don't know, this is based on downstream direclty.

> 
>> +
>>   	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
>>   	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>>   	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
>>   	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
>>   
>> +	/* Wait for PLL(s) to lock */
>> +        udelay(50);
> Weird indentation
> 
> Maybe wait_for_pll_enable_lock() to be super sure?

Does it work for HWFSM PLLs?

> 
>> +
>>   	qcom_cpu_clk_msm8996_acd_init(regmap);
>>   
>> +	/* Switch clusters to use the ACD leg */
>> +	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
>> +	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
>> +
> No delays here?

No. Probably it isn't required since there is no additional PLL locking, 
etc.

> 
> Konrad
>>   	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>>   		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
>>   		if (ret)

-- 
With best wishes
Dmitry

Re: [PATCH 03/13] clk: qcom: cpu-8996: fix the init clock rate

[Konrad Dybcio]

On 11.01.2023 22:51, Dmitry Baryshkov wrote:
> On 11/01/2023 22:58, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>>> Change PLL programming to let both power and performance cluster clocks
>>> to start from the maximum common frequency.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> Can you point me to the source of this? My local random msm-3.18 has this at 60.
> 
> Yes, but with 60 cluster start at the unlisted frequency (60 * 19.2 = 1152 MHz), which leads to cpufreq whining and immediately performing a switch.
> 
> I modified this to 54 * 19.2 =  1036.8 MHz which is supported by both power and performance clusters. Maybe we could have gone to 58 * 19.2 = 1113. Mhz or to 62 * 19.2 = 1190.4 MHz, but as all the safety and power measures and not probed at this point, I preferred to rather be safe than sorry.
Okay, please include this reasoning in the commit message, as nobody
would guess it fixes this issue..

Konrad
> 
>>
>> Konrad
>>>   drivers/clk/qcom/clk-cpu-8996.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index ed8cb558e1aa..d51965fda56d 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -102,7 +102,7 @@ static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
>>>   /* PLLs */
>>>     static const struct alpha_pll_config hfpll_config = {
>>> -    .l = 60,
>>> +    .l = 54,
>>>       .config_ctl_val = 0x200d4828,
>>>       .config_ctl_hi_val = 0x006,
>>>       .test_ctl_val = 0x1c000000,
> 

Re: [PATCH 04/13] clk: qcom: cpu-8996: support using GPLL0 as SMUX input

[Konrad Dybcio]

On 11.01.2023 22:52, Dmitry Baryshkov wrote:
> On 11/01/2023 22:59, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>>> In some cases the driver might need using GPLL0 to drive CPU clocks.
>>> Bring it in through the sys_apcs_aux clock.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> Oh that's new.. downstream doesn't talk about this..
> 
> It does, but under the hood of the init procedure. See:
> 
>         /* Select GPLL0 for 300MHz for the perf cluster */
>         writel_relaxed(0xC, vbases[APC1_BASE] + MUX_OFFSET);
> 
Okay I see it now!

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
> 
>>
>> Konrad
>>>   drivers/clk/qcom/clk-cpu-8996.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index d51965fda56d..0e0c00d44c6f 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -12,6 +12,8 @@
>>>    *                              +-------+
>>>    *               XO             |       |
>>>    *           +------------------>0      |
>>> + *               SYS_APCS_AUX   |       |
>>> + *           +------------------>3      |
>>>    *                              |       |
>>>    *                    PLL/2     | SMUX  +----+
>>>    *                      +------->1      |    |
>>> @@ -310,20 +312,29 @@ static const struct clk_ops clk_cpu_8996_pmux_ops = {
>>>       .determine_rate = clk_cpu_8996_pmux_determine_rate,
>>>   };
>>>   +static const struct parent_map smux_parent_map[] = {
>>> +    { .cfg = 0, }, /* xo */
>>> +    { .cfg = 1, }, /* pll */
>>> +    { .cfg = 3, }, /* sys_apcs_aux */
>>> +};
>>> +
>>>   static const struct clk_parent_data pwrcl_smux_parents[] = {
>>>       { .fw_name = "xo" },
>>>       { .hw = &pwrcl_pll_postdiv.hw },
>>> +    { .fw_name = "sys_apcs_aux" },
>>>   };
>>>     static const struct clk_parent_data perfcl_smux_parents[] = {
>>>       { .fw_name = "xo" },
>>>       { .hw = &perfcl_pll_postdiv.hw },
>>> +    { .fw_name = "sys_apcs_aux" },
>>>   };
>>>     static struct clk_regmap_mux pwrcl_smux = {
>>>       .reg = PWRCL_REG_OFFSET + MUX_OFFSET,
>>>       .shift = 2,
>>>       .width = 2,
>>> +    .parent_map = smux_parent_map,
>>>       .clkr.hw.init = &(struct clk_init_data) {
>>>           .name = "pwrcl_smux",
>>>           .parent_data = pwrcl_smux_parents,
>>> @@ -337,6 +348,7 @@ static struct clk_regmap_mux perfcl_smux = {
>>>       .reg = PERFCL_REG_OFFSET + MUX_OFFSET,
>>>       .shift = 2,
>>>       .width = 2,
>>> +    .parent_map = smux_parent_map,
>>>       .clkr.hw.init = &(struct clk_init_data) {
>>>           .name = "perfcl_smux",
>>>           .parent_data = perfcl_smux_parents,
> 

Re: [PATCH 05/13] clk: qcom: cpu-8996: skip ACD init if the setup is valid

[Konrad Dybcio]

On 11.01.2023 22:55, Dmitry Baryshkov wrote:
> On 11/01/2023 23:00, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>>> Check whether L2 registers contain correct values and skip programming
>>> if they are valid. This follows the code present downstream.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> Once again, my random local msm-3.18 doesn't do this, can you show
>> me the downstream source for this?
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L856
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
> 
>>
>> Konrad
>>>   drivers/clk/qcom/clk-cpu-8996.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index 0e0c00d44c6f..7e5246ca7e7f 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -472,10 +472,15 @@ static void __iomem *base;
>>>   static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
>>>   {
>>>       u64 hwid;
>>> +    u32 val;
>>>       unsigned long flags;
>>>         spin_lock_irqsave(&qcom_clk_acd_lock, flags);
>>>   +    val = kryo_l2_get_indirect_reg(L2ACDTD_REG);
>>> +    if (val == 0x00006a11)
>>> +        goto out;
>>> +
>>>       hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
>>>         kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
>>> @@ -492,6 +497,7 @@ static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
>>>           writel(0xf, base + PERFCL_REG_OFFSET + SSSCTL_OFFSET);
>>>       }
>>>   +out:
>>>       spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);
>>>   }
>>>   
> 

Re: [PATCH 06/13] clk: qcom: cpu-8996: simplify the cpu_clk_notifier_cb

[Konrad Dybcio]

On 11.01.2023 23:01, Dmitry Baryshkov wrote:
> On 11/01/2023 23:03, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>>> - Do not use the Alt PLL completely. Switch to smux when necessary to
>>>    prevent overvolting
>> Is this empirical evidence, or did Qualcomm recommendations change since
>> msm-3.18 was released?
> 
> I think this is what they are doing, see https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L675
> 
> They switch altpll frequency for whatever reasons, then they do the dance of switching the parent rate to half rate if necessary and then they finally switch the parent's rate to the target rate. That's the only way I can interpret the cpu_clk_8996_set_rate().
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L675

Okay, so on rate switch they:

PWR:
  - set (rate > 1190400 kHz ? 556800000 : 307200000) on ALT_PLL
  - use_alt_pll is 0 because it's an uninitialized global var
    - do_half_rate is TRUE on PWRCL_CLK
    - " /* Special handling needed " path is entered if the frequency
      is being requested to go from above 600 MHz to below 600 MHz
    - clk_set_rate sets OLD_RATE/2 on pwrcl_hf_mux which
      has 2 parents:
        * pwrcl_pll for 600 MHz - 3000 MHz
        * pwrcl_lf_mux for anything else fed by:
           * pwrcl_pll_main = pwrcl_pll/2 (300 MHz - 600 MHz???)
           * sys_apcsaux_clk (SMUX) (<300 MHz??)

PERF:
  - use_alt_pll is 0 because it's an uninitialized global var
    - do_half_rate is TRUE on PERFCL_CLK
    - " /* Special handling needed " path is entered if the frequency
      is being requested to go from above 600 MHz to below 600 MHz
    - clk_set_rate sets OLD_RATE/2 (/4 on Pro) on perfcl_hf_mux which
      has 2 parents:
        * perfcl_pll for 600 MHz - 3000 MHz
        * perfcl_lf_mux for anything else fed by:
           * perfcl_pll_main = perfcl_pll/2 (300 MHz - 600 MHz???)
           * sys_apcsaux_clk (SMUX) (<300 MHz??)

So I think the alt_plls are NOT used and this is correct..

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

> 
>>
>>
>>> - Restore the parent in case the rate change aborts for some reason
>>> - Do not duplicate resetting the parent in set_parent operation.
>> These sound good.
>>
>> Konrad
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/clk/qcom/clk-cpu-8996.c | 31 +++++++++++++++++++------------
>>>   1 file changed, 19 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index 7e5246ca7e7f..ee7e18b37832 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -506,27 +506,34 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>>>   {
>>>       struct clk_cpu_8996_pmux *cpuclk = to_clk_cpu_8996_pmux_nb(nb);
>>>       struct clk_notifier_data *cnd = data;
>>> -    int ret;
>>>         switch (event) {
>>>       case PRE_RATE_CHANGE:
>>> -        ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ALT_INDEX);
>>>           qcom_cpu_clk_msm8996_acd_init(base);
>>> +
>>> +        /*
>>> +         * Avoid overvolting. clk_core_set_rate_nolock() walks from top
>>> +         * to bottom, so it will change the rate of the PLL before
>>> +         * chaging the parent of PMUX. This can result in pmux getting
>>> +         * clocked twice the expected rate.
>>> +         *
>>> +         * Manually switch to PLL/2 here.
>>> +         */
>>> +        if (cnd->new_rate < DIV_2_THRESHOLD &&
>>> +            cnd->old_rate > DIV_2_THRESHOLD)
>>> +            clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, SMUX_INDEX);
>>> +
>>>           break;
>>> -    case POST_RATE_CHANGE:
>>> -        if (cnd->new_rate < DIV_2_THRESHOLD)
>>> -            ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
>>> -                               SMUX_INDEX);
>>> -        else
>>> -            ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
>>> -                               ACD_INDEX);
>>> -        break;
>>> +    case ABORT_RATE_CHANGE:
>>> +        /* Revert manual change */
>>> +        if (cnd->new_rate < DIV_2_THRESHOLD &&
>>> +            cnd->old_rate > DIV_2_THRESHOLD)
>>> +            clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ACD_INDEX);
>>>       default:
>>> -        ret = 0;
>>>           break;
>>>       }
>>>   -    return notifier_from_errno(ret);
>>> +    return NOTIFY_OK;
>>>   };
>>>     static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
> 

Re: [PATCH 08/13] clk: qcom: cpu-8996: move qcom_cpu_clk_msm8996_acd_init call

[Konrad Dybcio]

On 11.01.2023 20:19, Dmitry Baryshkov wrote:
> Initialize ACD configuration from qcom_cpu_clk_msm8996_register_clks(),
> before registering all clocks. This way we can be sure that the clock is
> fully configured before letting CCF touch it.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index e390f4aadff1..571ed52b3026 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -425,6 +425,8 @@ static struct clk_regmap *cpu_msm8996_clks[] = {
>  	&perfcl_pmux.clkr,
>  };
>  
> +static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap);
> +
>  static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  					      struct regmap *regmap)
>  {
> @@ -435,6 +437,8 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
>  	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
>  
> +	qcom_cpu_clk_msm8996_acd_init(regmap);
> +
>  	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>  		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
>  		if (ret)
> @@ -467,9 +471,8 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  #define L2ACDSSCR_REG 0x589ULL
>  
>  static DEFINE_SPINLOCK(qcom_clk_acd_lock);
> -static void __iomem *base;
>  
> -static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
> +static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap)
>  {
>  	u64 hwid;
>  	u32 val;
> @@ -488,13 +491,13 @@ static void qcom_cpu_clk_msm8996_acd_init(void __iomem *base)
>  	kryo_l2_set_indirect_reg(L2ACDSSCR_REG, 0x00000601);
>  
>  	if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
> -		writel(0xf, base + PWRCL_REG_OFFSET + SSSCTL_OFFSET);
> +		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
>  		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
>  	}
>  
>  	if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) {
>  		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
> -		writel(0xf, base + PERFCL_REG_OFFSET + SSSCTL_OFFSET);
> +		regmap_write(regmap, PERFCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
>  	}
>  
>  out:
> @@ -509,7 +512,7 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>  
>  	switch (event) {
>  	case PRE_RATE_CHANGE:
> -		qcom_cpu_clk_msm8996_acd_init(base);
> +		qcom_cpu_clk_msm8996_acd_init(cpuclk->clkr.regmap);
>  
>  		/*
>  		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
> @@ -538,6 +541,7 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>  
>  static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
>  {
> +	static void __iomem *base;
>  	struct regmap *regmap;
>  	struct clk_hw_onecell_data *data;
>  	struct device *dev = &pdev->dev;
> @@ -559,8 +563,6 @@ static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	qcom_cpu_clk_msm8996_acd_init(base);
> -
>  	data->hws[0] = &pwrcl_pmux.clkr.hw;
>  	data->hws[1] = &perfcl_pmux.clkr.hw;
>  	data->num = 2;

Re: [PATCH 09/13] clk: qcom: cpu-8996: fix PLL configuration sequence

[Konrad Dybcio]

On 11.01.2023 23:05, Dmitry Baryshkov wrote:
> On 11/01/2023 23:08, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>>> Switch both power and performance clocks to the GPLL0/2 (sys_apcs_aux)
>>> before PLL configuration. Switch them to the ACD afterwards.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/clk/qcom/clk-cpu-8996.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index 571ed52b3026..47c58bb5f21a 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -432,13 +432,27 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>>>   {
>>>       int i, ret;
>>>   +    /* Select GPLL0 for 300MHz for the both clusters */
>> superfluous 'the'
>>
>>> +    regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0xc);
>>> +    regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0xc);
>>> +
>>> +    /* Ensure write goes through before PLLs are reconfigured */
>>> +    udelay(5);
>> Is this value based on n clock cycles, or 'good enough'?
> 
> Don't know, this is based on downstream direclty.
Right, I see it now.

> 
>>
>>> +
>>>       clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
>>>       clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>>>       clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
>>>       clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
>>>   +    /* Wait for PLL(s) to lock */
>>> +        udelay(50);
>> Weird indentation
>>
>> Maybe wait_for_pll_enable_lock() to be super sure?
> 
> Does it work for HWFSM PLLs?
Not sure, but wait_for_pll_update_ack_clear() should, since it's
called by 

clk_alpha_pll_hwfsm_set_rate() ->
  __clk_alpha_pll_set_rate() ->
    clk_alpha_pll_update_latch() ->
      __clk_alpha_pll_update_latch()

Konrad
> 
>>
>>> +
>>>       qcom_cpu_clk_msm8996_acd_init(regmap);
>>>   +    /* Switch clusters to use the ACD leg */
>>> +    regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
>>> +    regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
>>> +
>> No delays here?
> 
> No. Probably it isn't required since there is no additional PLL locking, etc.
> 
>>
>> Konrad
>>>       for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>>>           ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
>>>           if (ret)
> 

Re: [PATCH 10/13] clk: qcom: cpu-8996: fix ACD initialization

[Konrad Dybcio]

On 11.01.2023 20:20, Dmitry Baryshkov wrote:
> The vendor kernel applies different order while programming SSSCTL and
> L2ACDCR registers on power and performance clusters. However it was
> demonstrated that doing this upstream results in the board reset. Make
> both clusters use the same sequence, which fixes the reset.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
I think we should look for the source of why this doesn't work,
e.g. does downstream program it earlier somewhere? Are we
missing something else that may bite later?

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index 47c58bb5f21a..1c00eb629b61 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -475,9 +475,9 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  	return ret;
>  }
>  
> -#define CPU_AFINITY_MASK 0xFFF
> -#define PWRCL_CPU_REG_MASK 0x3
> -#define PERFCL_CPU_REG_MASK 0x103
> +#define CPU_CLUSTER_AFFINITY_MASK 0xf00
> +#define PWRCL_AFFINITY_MASK 0x000
> +#define PERFCL_AFFINITY_MASK 0x100
>  
>  #define L2ACDCR_REG 0x580ULL
>  #define L2ACDTD_REG 0x581ULL
> @@ -498,21 +498,17 @@ static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap)
>  	if (val == 0x00006a11)
>  		goto out;
>  
> -	hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
> -
>  	kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
>  	kryo_l2_set_indirect_reg(L2ACDDVMRC_REG, 0x000e0f0f);
>  	kryo_l2_set_indirect_reg(L2ACDSSCR_REG, 0x00000601);
>  
> -	if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
> -		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
> -		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
> -	}
> +	kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
>  
> -	if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) {
> -		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
> +	hwid = read_cpuid_mpidr();
> +	if ((hwid & CPU_CLUSTER_AFFINITY_MASK) == PWRCL_AFFINITY_MASK)
> +		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
> +	else
>  		regmap_write(regmap, PERFCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
> -	}
>  
>  out:
>  	spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);

Re: [PATCH 12/13] clk: qcom: cpu-8996: change setup sequence to follow vendor kernel

[Konrad Dybcio]

On 11.01.2023 20:20, Dmitry Baryshkov wrote:
> Add missing register writes to CPU clocks setup procedure. This makes it
> follow the setup procedure used in msm-3.18 kernel.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index b53cddc4bca3..78a18b95c48b 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -76,10 +76,16 @@ enum _pmux_input {
>  #define PWRCL_REG_OFFSET 0x0
>  #define PERFCL_REG_OFFSET 0x80000
>  #define MUX_OFFSET	0x40
> +#define CLK_CTL_OFFSET 0x44
> +#define CLK_CTL_AUTO_CLK_SEL BIT(8)
>  #define ALT_PLL_OFFSET	0x100
>  #define SSSCTL_OFFSET 0x160
> +#define PSCTL_OFFSET 0x164
>  
>  #define PMUX_MASK	0x3
> +#define MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK GENMASK(5, 4)
> +#define MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL \
> +	FIELD_PREP(MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK, 0x03)
>  
>  static const u8 prim_pll_regs[PLL_OFF_MAX_REGS] = {
>  	[PLL_OFF_L_VAL] = 0x04,
> @@ -439,6 +445,14 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  	/* Ensure write goes through before PLLs are reconfigured */
>  	udelay(5);
>  
> +	/* Set the auto clock sel always-on source to GPLL0/2 (300MHz) */
> +	regmap_update_bits(regmap, PWRCL_REG_OFFSET + MUX_OFFSET,
> +			   MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
> +			   MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
> +	regmap_update_bits(regmap, PERFCL_REG_OFFSET + MUX_OFFSET,
> +			   MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
> +			   MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
> +
>  	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
>  	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>  	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
> @@ -447,11 +461,24 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>  	/* Wait for PLL(s) to lock */
>          udelay(50);
>  
> +	/* Enable auto clock selection for both clusters */
> +	regmap_update_bits(regmap, PWRCL_REG_OFFSET + CLK_CTL_OFFSET,
> +			   CLK_CTL_AUTO_CLK_SEL, CLK_CTL_AUTO_CLK_SEL);
> +	regmap_update_bits(regmap, PERFCL_REG_OFFSET + CLK_CTL_OFFSET,
> +			   CLK_CTL_AUTO_CLK_SEL, CLK_CTL_AUTO_CLK_SEL);
> +
> +	/* Ensure write goes through before muxes are switched */
> +	udelay(5);
> +
>  	qcom_cpu_clk_msm8996_acd_init(regmap);
>  
> +	/* Pulse swallower and soft-start settings */
> +	regmap_write(regmap, PWRCL_REG_OFFSET + PSCTL_OFFSET, 0x00030005);
> +	regmap_write(regmap, PERFCL_REG_OFFSET + PSCTL_OFFSET, 0x00030005);
> +
>  	/* Switch clusters to use the ACD leg */
> -	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
> -	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
> +	regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x32);
> +	regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x32);
>  
>  	for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>  		ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);

Re: [PATCH 11/13] clk: qcom: cpu-8996: fix PLL clock ops

[Konrad Dybcio]

On 11.01.2023 20:20, Dmitry Baryshkov wrote:
> Switch CPU PLLs to use clk_alpha_pll_hwfsm_ops, it seems to suit
> better.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
I *think* SUPPORTS_DYNAMIC_UPDATE should also be kicked from
non-alt PLLs.. Otherwise we might have been kicking ourselves
in the face all along, changing the frequency of a running
PLL that doesn't support it if we were using the main PLL
and not the altPLL/ACD..

Downstream sets it only for clk_ops_alpha_pll_hwfsm which is
used on alt PLLs only

This change seems sound, as Huayra supports dynamic update
even without setting any flags.

Konrad
>  drivers/clk/qcom/clk-cpu-8996.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> index 1c00eb629b61..b53cddc4bca3 100644
> --- a/drivers/clk/qcom/clk-cpu-8996.c
> +++ b/drivers/clk/qcom/clk-cpu-8996.c
> @@ -128,7 +128,7 @@ static struct clk_alpha_pll pwrcl_pll = {
>  		.name = "pwrcl_pll",
>  		.parent_data = pll_parent,
>  		.num_parents = ARRAY_SIZE(pll_parent),
> -		.ops = &clk_alpha_pll_huayra_ops,
> +		.ops = &clk_alpha_pll_hwfsm_ops,
>  	},
>  };
>  
> @@ -140,7 +140,7 @@ static struct clk_alpha_pll perfcl_pll = {
>  		.name = "perfcl_pll",
>  		.parent_data = pll_parent,
>  		.num_parents = ARRAY_SIZE(pll_parent),
> -		.ops = &clk_alpha_pll_huayra_ops,
> +		.ops = &clk_alpha_pll_hwfsm_ops,
>  	},
>  };
>  

Re: [PATCH 10/13] clk: qcom: cpu-8996: fix ACD initialization

[Dmitry Baryshkov]

On 12/01/2023 16:35, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>> The vendor kernel applies different order while programming SSSCTL and
>> L2ACDCR registers on power and performance clusters. However it was
>> demonstrated that doing this upstream results in the board reset. Make
>> both clusters use the same sequence, which fixes the reset.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> I think we should look for the source of why this doesn't work,
> e.g. does downstream program it earlier somewhere? Are we
> missing something else that may bite later?

I'm not sure what is the reason for downstream doing init in such 
sequence. Right now I'm sure that doing ACD init with the provided 
sequence fails the boot in some conditions. There might be the 
difference in the CPU init order. Or any other ordering issue. Or the 
lack of the CPR. Or Kryo LDO programming. There is a huge difference 
between vendor's 3.18 and the current 6.x.

I propose to take the patch in, as it fixes the boot and runtime issue 
and revisit it later if any of the problems occur. I don't fancy such 
approach usually, but without the documentation I don't see a way to 
find any particular reason for programming pwr and perf using the 
different order of operations.

> 
> Konrad
>>   drivers/clk/qcom/clk-cpu-8996.c | 20 ++++++++------------
>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> index 47c58bb5f21a..1c00eb629b61 100644
>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -475,9 +475,9 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>>   	return ret;
>>   }
>>   
>> -#define CPU_AFINITY_MASK 0xFFF
>> -#define PWRCL_CPU_REG_MASK 0x3
>> -#define PERFCL_CPU_REG_MASK 0x103
>> +#define CPU_CLUSTER_AFFINITY_MASK 0xf00
>> +#define PWRCL_AFFINITY_MASK 0x000
>> +#define PERFCL_AFFINITY_MASK 0x100
>>   
>>   #define L2ACDCR_REG 0x580ULL
>>   #define L2ACDTD_REG 0x581ULL
>> @@ -498,21 +498,17 @@ static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap)
>>   	if (val == 0x00006a11)
>>   		goto out;
>>   
>> -	hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
>> -
>>   	kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
>>   	kryo_l2_set_indirect_reg(L2ACDDVMRC_REG, 0x000e0f0f);
>>   	kryo_l2_set_indirect_reg(L2ACDSSCR_REG, 0x00000601);
>>   
>> -	if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
>> -		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
>> -		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
>> -	}
>> +	kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
>>   
>> -	if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) {
>> -		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
>> +	hwid = read_cpuid_mpidr();
>> +	if ((hwid & CPU_CLUSTER_AFFINITY_MASK) == PWRCL_AFFINITY_MASK)
>> +		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
>> +	else
>>   		regmap_write(regmap, PERFCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
>> -	}
>>   
>>   out:
>>   	spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);

-- 
With best wishes
Dmitry

Re: [PATCH 09/13] clk: qcom: cpu-8996: fix PLL configuration sequence

[Dmitry Baryshkov]

On 12/01/2023 16:32, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 23:05, Dmitry Baryshkov wrote:
>> On 11/01/2023 23:08, Konrad Dybcio wrote:
>>>
>>>
>>> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>>>> Switch both power and performance clocks to the GPLL0/2 (sys_apcs_aux)
>>>> before PLL configuration. Switch them to the ACD afterwards.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>    drivers/clk/qcom/clk-cpu-8996.c | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>>> index 571ed52b3026..47c58bb5f21a 100644
>>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>>> @@ -432,13 +432,27 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>>>>    {
>>>>        int i, ret;
>>>>    +    /* Select GPLL0 for 300MHz for the both clusters */
>>> superfluous 'the'
>>>
>>>> +    regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0xc);
>>>> +    regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0xc);
>>>> +
>>>> +    /* Ensure write goes through before PLLs are reconfigured */
>>>> +    udelay(5);
>>> Is this value based on n clock cycles, or 'good enough'?
>>
>> Don't know, this is based on downstream direclty.
> Right, I see it now.
> 
>>
>>>
>>>> +
>>>>        clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
>>>>        clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>>>>        clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
>>>>        clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
>>>>    +    /* Wait for PLL(s) to lock */
>>>> +        udelay(50);
>>> Weird indentation

Fixing for v2.

>>>
>>> Maybe wait_for_pll_enable_lock() to be super sure?
>>
>> Does it work for HWFSM PLLs?
> Not sure, but wait_for_pll_update_ack_clear() should, since it's
> called by

I'd prefer to keep it as is. First, this seems to be the difference 
between normal and hwfsm PLLs, see clk_alpha_pll_is_enabled() vs 
clk_alpha_pll_hwfsm_is_enabled(). And second, the wait_for_pll() 
function is not exported from the clk-alpha-pll.c. Note, that downstream 
also does sleep instead of waiting.

> 
> clk_alpha_pll_hwfsm_set_rate() ->
>    __clk_alpha_pll_set_rate() ->
>      clk_alpha_pll_update_latch() ->
>        __clk_alpha_pll_update_latch()
> 
> Konrad
>>
>>>
>>>> +
>>>>        qcom_cpu_clk_msm8996_acd_init(regmap);
>>>>    +    /* Switch clusters to use the ACD leg */
>>>> +    regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
>>>> +    regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
>>>> +
>>> No delays here?
>>
>> No. Probably it isn't required since there is no additional PLL locking, etc.
>>
>>>
>>> Konrad
>>>>        for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>>>>            ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
>>>>            if (ret)
>>

-- 
With best wishes
Dmitry

Re: [PATCH 11/13] clk: qcom: cpu-8996: fix PLL clock ops

[Dmitry Baryshkov]

On 12/01/2023 18:10, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>> Switch CPU PLLs to use clk_alpha_pll_hwfsm_ops, it seems to suit
>> better.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> I *think* SUPPORTS_DYNAMIC_UPDATE should also be kicked from
> non-alt PLLs.. Otherwise we might have been kicking ourselves
> in the face all along, changing the frequency of a running
> PLL that doesn't support it if we were using the main PLL
> and not the altPLL/ACD..
> 
> Downstream sets it only for clk_ops_alpha_pll_hwfsm which is
> used on alt PLLs only
> 
> This change seems sound, as Huayra supports dynamic update
> even without setting any flags.

I don't know where Huayra came from. Downstream uses plain hwfsm pll. 
Huayra uses different alpha register settings.

> 
> Konrad
>>   drivers/clk/qcom/clk-cpu-8996.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> index 1c00eb629b61..b53cddc4bca3 100644
>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -128,7 +128,7 @@ static struct clk_alpha_pll pwrcl_pll = {
>>   		.name = "pwrcl_pll",
>>   		.parent_data = pll_parent,
>>   		.num_parents = ARRAY_SIZE(pll_parent),
>> -		.ops = &clk_alpha_pll_huayra_ops,
>> +		.ops = &clk_alpha_pll_hwfsm_ops,
>>   	},
>>   };
>>   
>> @@ -140,7 +140,7 @@ static struct clk_alpha_pll perfcl_pll = {
>>   		.name = "perfcl_pll",
>>   		.parent_data = pll_parent,
>>   		.num_parents = ARRAY_SIZE(pll_parent),
>> -		.ops = &clk_alpha_pll_huayra_ops,
>> +		.ops = &clk_alpha_pll_hwfsm_ops,
>>   	},
>>   };
>>   

-- 
With best wishes
Dmitry

Re: [PATCH 09/13] clk: qcom: cpu-8996: fix PLL configuration sequence

[Konrad Dybcio]

On 13.01.2023 12:19, Dmitry Baryshkov wrote:
> On 12/01/2023 16:32, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 23:05, Dmitry Baryshkov wrote:
>>> On 11/01/2023 23:08, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>>>>> Switch both power and performance clocks to the GPLL0/2 (sys_apcs_aux)
>>>>> before PLL configuration. Switch them to the ACD afterwards.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>    drivers/clk/qcom/clk-cpu-8996.c | 14 ++++++++++++++
>>>>>    1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>>>> index 571ed52b3026..47c58bb5f21a 100644
>>>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>>>> @@ -432,13 +432,27 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>>>>>    {
>>>>>        int i, ret;
>>>>>    +    /* Select GPLL0 for 300MHz for the both clusters */
>>>> superfluous 'the'
>>>>
>>>>> +    regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0xc);
>>>>> +    regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0xc);
>>>>> +
>>>>> +    /* Ensure write goes through before PLLs are reconfigured */
>>>>> +    udelay(5);
>>>> Is this value based on n clock cycles, or 'good enough'?
>>>
>>> Don't know, this is based on downstream direclty.
>> Right, I see it now.
>>
>>>
>>>>
>>>>> +
>>>>>        clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
>>>>>        clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>>>>>        clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
>>>>>        clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
>>>>>    +    /* Wait for PLL(s) to lock */
>>>>> +        udelay(50);
>>>> Weird indentation
> 
> Fixing for v2.
> 
>>>>
>>>> Maybe wait_for_pll_enable_lock() to be super sure?
>>>
>>> Does it work for HWFSM PLLs?
>> Not sure, but wait_for_pll_update_ack_clear() should, since it's
>> called by
> 
> I'd prefer to keep it as is. First, this seems to be the difference between normal and hwfsm PLLs, see clk_alpha_pll_is_enabled() vs clk_alpha_pll_hwfsm_is_enabled(). And second, the wait_for_pll() function is not exported from the clk-alpha-pll.c. Note, that downstream also does sleep instead of waiting.
Okay let's settle on that.

Konrad
> 
>>
>> clk_alpha_pll_hwfsm_set_rate() ->
>>    __clk_alpha_pll_set_rate() ->
>>      clk_alpha_pll_update_latch() ->
>>        __clk_alpha_pll_update_latch()
>>
>> Konrad
>>>
>>>>
>>>>> +
>>>>>        qcom_cpu_clk_msm8996_acd_init(regmap);
>>>>>    +    /* Switch clusters to use the ACD leg */
>>>>> +    regmap_write(regmap, PWRCL_REG_OFFSET + MUX_OFFSET, 0x2);
>>>>> +    regmap_write(regmap, PERFCL_REG_OFFSET + MUX_OFFSET, 0x2);
>>>>> +
>>>> No delays here?
>>>
>>> No. Probably it isn't required since there is no additional PLL locking, etc.
>>>
>>>>
>>>> Konrad
>>>>>        for (i = 0; i < ARRAY_SIZE(cpu_msm8996_hw_clks); i++) {
>>>>>            ret = devm_clk_hw_register(dev, cpu_msm8996_hw_clks[i]);
>>>>>            if (ret)
>>>
> 

Re: [PATCH 10/13] clk: qcom: cpu-8996: fix ACD initialization

[Konrad Dybcio]

On 13.01.2023 11:44, Dmitry Baryshkov wrote:
> On 12/01/2023 16:35, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>>> The vendor kernel applies different order while programming SSSCTL and
>>> L2ACDCR registers on power and performance clusters. However it was
>>> demonstrated that doing this upstream results in the board reset. Make
>>> both clusters use the same sequence, which fixes the reset.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> I think we should look for the source of why this doesn't work,
>> e.g. does downstream program it earlier somewhere? Are we
>> missing something else that may bite later?
> 
> I'm not sure what is the reason for downstream doing init in such sequence. Right now I'm sure that doing ACD init with the provided sequence fails the boot in some conditions. There might be the difference in the CPU init order. Or any other ordering issue. Or the lack of the CPR. Or Kryo LDO programming. There is a huge difference between vendor's 3.18 and the current 6.x.
> 
> I propose to take the patch in, as it fixes the boot and runtime issue and revisit it later if any of the problems occur. I don't fancy such approach usually, but without the documentation I don't see a way to find any particular reason for programming pwr and perf using the different order of operations.
Ack, let's do that.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

One more thing, I noticed that downstream calls ACD init on
`CPU_STARTING` event instead of `PRE_RATE_CHANGE`, see [1].
Are we "over-programming" ACD too much, or is it intended/fine?

Konrad

[1] https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L1522-1540
> 
>>
>> Konrad
>>>   drivers/clk/qcom/clk-cpu-8996.c | 20 ++++++++------------
>>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index 47c58bb5f21a..1c00eb629b61 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -475,9 +475,9 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>>>       return ret;
>>>   }
>>>   -#define CPU_AFINITY_MASK 0xFFF
>>> -#define PWRCL_CPU_REG_MASK 0x3
>>> -#define PERFCL_CPU_REG_MASK 0x103
>>> +#define CPU_CLUSTER_AFFINITY_MASK 0xf00
>>> +#define PWRCL_AFFINITY_MASK 0x000
>>> +#define PERFCL_AFFINITY_MASK 0x100
>>>     #define L2ACDCR_REG 0x580ULL
>>>   #define L2ACDTD_REG 0x581ULL
>>> @@ -498,21 +498,17 @@ static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap)
>>>       if (val == 0x00006a11)
>>>           goto out;
>>>   -    hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
>>> -
>>>       kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
>>>       kryo_l2_set_indirect_reg(L2ACDDVMRC_REG, 0x000e0f0f);
>>>       kryo_l2_set_indirect_reg(L2ACDSSCR_REG, 0x00000601);
>>>   -    if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
>>> -        regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
>>> -        kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
>>> -    }
>>> +    kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
>>>   -    if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) {
>>> -        kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
>>> +    hwid = read_cpuid_mpidr();
>>> +    if ((hwid & CPU_CLUSTER_AFFINITY_MASK) == PWRCL_AFFINITY_MASK)
>>> +        regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
>>> +    else
>>>           regmap_write(regmap, PERFCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
>>> -    }
>>>     out:
>>>       spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);
> 

Re: [PATCH 11/13] clk: qcom: cpu-8996: fix PLL clock ops

[Konrad Dybcio]

On 13.01.2023 12:35, Dmitry Baryshkov wrote:
> On 12/01/2023 18:10, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
>>> Switch CPU PLLs to use clk_alpha_pll_hwfsm_ops, it seems to suit
>>> better.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> I *think* SUPPORTS_DYNAMIC_UPDATE should also be kicked from
>> non-alt PLLs.. Otherwise we might have been kicking ourselves
>> in the face all along, changing the frequency of a running
>> PLL that doesn't support it if we were using the main PLL
>> and not the altPLL/ACD..
>>
>> Downstream sets it only for clk_ops_alpha_pll_hwfsm which is
>> used on alt PLLs only
>>
>> This change seems sound, as Huayra supports dynamic update
>> even without setting any flags.
> 
> I don't know where Huayra came from. Downstream uses plain hwfsm pll. Huayra uses different alpha register settings.
Right, that too.. somewhat of a miracle things worked at all..

Konrad

P.S please revisit that SUPPORTS_DYNAMIC_UPDATE flag for main PLLs
> 
>>
>> Konrad
>>>   drivers/clk/qcom/clk-cpu-8996.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index 1c00eb629b61..b53cddc4bca3 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -128,7 +128,7 @@ static struct clk_alpha_pll pwrcl_pll = {
>>>           .name = "pwrcl_pll",
>>>           .parent_data = pll_parent,
>>>           .num_parents = ARRAY_SIZE(pll_parent),
>>> -        .ops = &clk_alpha_pll_huayra_ops,
>>> +        .ops = &clk_alpha_pll_hwfsm_ops,
>>>       },
>>>   };
>>>   @@ -140,7 +140,7 @@ static struct clk_alpha_pll perfcl_pll = {
>>>           .name = "perfcl_pll",
>>>           .parent_data = pll_parent,
>>>           .num_parents = ARRAY_SIZE(pll_parent),
>>> -        .ops = &clk_alpha_pll_huayra_ops,
>>> +        .ops = &clk_alpha_pll_hwfsm_ops,
>>>       },
>>>   };
>>>   
>