[PATCH RESEND v2 0/5] Modify MxL's CGU clk driver to make it secure boot compatible

[PATCH RESEND v2 0/5] Modify MxL's CGU clk driver to make it secure boot compatible

[Rahul Tanwar]

* Rebase on latest 6.0 clk linux tree and resend patch series because of
  no response from anyone so far. Please let me know if anyone has any
  concerns about these changes. Thanks.

MxL's CGU driver was found to be lacking below required features. Add these
required lacking features:

1. Since it is a core driver, it has to conform to secure boot & secure
   access architecture. In order for the register accesses to be secure
   access compliant, it needs regmap support as per our security architecture.
   Hence, replace direct read/writel with regmap based IO. Also remove spinlocks
   because they are no longer necessary because regmap uses its own lock.

2. There are some gate clocks which are used by the power mgmt IP to gate when
   a certain power saving mode is activated. These gate clocks can also be 
   gated from CGU clk driver. This creates a conflict. To avoid the conflict,
   by default disable gating such gate registers from CGU clk driver. But keep
   a flag to do so for other older IP's which uses same CGU clk IP but does not
   use same power mgmt IP.

3. Fix two functional bugs found during testing.

This patch series is based on below git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git


Rahul Tanwar (5):
  clk: mxl: Switch from direct readl/writel based IO to regmap based IO
  clk: mxl: Remove unnecessary spinlocks
  clk: mxl: Avoid disabling gate clocks from clk driver
  clk: mxl: Add validation for register reads/writes
  clk: mxl: Add a missing flag to allow parent clock rate change

 drivers/clk/x86/Kconfig       |   5 +-
 drivers/clk/x86/clk-cgu-pll.c |  23 ++-----
 drivers/clk/x86/clk-cgu.c     | 117 +++++++++++-----------------------
 drivers/clk/x86/clk-cgu.h     |  71 +++++++++++++--------
 drivers/clk/x86/clk-lgm.c     |  16 +++--
 5 files changed, 101 insertions(+), 131 deletions(-)

-- 
2.17.1

[PATCH RESEND v2 1/5] clk: mxl: Switch from direct readl/writel based IO to regmap based IO

[Rahul Tanwar]

Earlier version of driver used direct io remapped register read
writes using readl/writel. But we need secure boot access which
is only possible when registers are read & written using regmap.
This is because the security bus/hook is written & coupled only
with regmap layer.

Switch the driver from direct readl/writel based register accesses
to regmap based register accesses.

Additionally, update the license headers to latest status.

Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
---
 drivers/clk/x86/Kconfig       |  5 +++--
 drivers/clk/x86/clk-cgu-pll.c | 10 +++++----
 drivers/clk/x86/clk-cgu.c     |  5 +++--
 drivers/clk/x86/clk-cgu.h     | 38 +++++++++++++++++++----------------
 drivers/clk/x86/clk-lgm.c     | 13 ++++++++----
 5 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/x86/Kconfig b/drivers/clk/x86/Kconfig
index 69642e15fcc1..ced99e082e3d 100644
--- a/drivers/clk/x86/Kconfig
+++ b/drivers/clk/x86/Kconfig
@@ -1,8 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config CLK_LGM_CGU
 	depends on OF && HAS_IOMEM && (X86 || COMPILE_TEST)
+	select MFD_SYSCON
 	select OF_EARLY_FLATTREE
 	bool "Clock driver for Lightning Mountain(LGM) platform"
 	help
-	  Clock Generation Unit(CGU) driver for Intel Lightning Mountain(LGM)
-	  network processor SoC.
+	  Clock Generation Unit(CGU) driver for MaxLinear's x86 based
+	  Lightning Mountain(LGM) network processor SoC.
diff --git a/drivers/clk/x86/clk-cgu-pll.c b/drivers/clk/x86/clk-cgu-pll.c
index 3179557b5f78..c83083affe88 100644
--- a/drivers/clk/x86/clk-cgu-pll.c
+++ b/drivers/clk/x86/clk-cgu-pll.c
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
+ * Copyright (C) 2020-2022 MaxLinear, Inc.
  * Copyright (C) 2020 Intel Corporation.
- * Zhu YiXin <yixin.zhu@intel.com>
- * Rahul Tanwar <rahul.tanwar@intel.com>
+ * Zhu Yixin <yzhu@maxlinear.com>
+ * Rahul Tanwar <rtanwar@maxlinear.com>
  */
 
 #include <linux/clk-provider.h>
@@ -76,8 +77,9 @@ static int lgm_pll_enable(struct clk_hw *hw)
 
 	spin_lock_irqsave(&pll->lock, flags);
 	lgm_set_clk_val(pll->membase, pll->reg, 0, 1, 1);
-	ret = readl_poll_timeout_atomic(pll->membase + pll->reg,
-					val, (val & 0x1), 1, 100);
+	ret = regmap_read_poll_timeout_atomic(pll->membase, pll->reg,
+					      val, (val & 0x1), 1, 100);
+
 	spin_unlock_irqrestore(&pll->lock, flags);
 
 	return ret;
diff --git a/drivers/clk/x86/clk-cgu.c b/drivers/clk/x86/clk-cgu.c
index 33de600e0c38..f5f30a18f486 100644
--- a/drivers/clk/x86/clk-cgu.c
+++ b/drivers/clk/x86/clk-cgu.c
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
+ * Copyright (C) 2020-2022 MaxLinear, Inc.
  * Copyright (C) 2020 Intel Corporation.
- * Zhu YiXin <yixin.zhu@intel.com>
- * Rahul Tanwar <rahul.tanwar@intel.com>
+ * Zhu Yixin <yzhu@maxlinear.com>
+ * Rahul Tanwar <rtanwar@maxlinear.com>
  */
 #include <linux/clk-provider.h>
 #include <linux/device.h>
diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
index 4e22bfb22312..dbcb66468797 100644
--- a/drivers/clk/x86/clk-cgu.h
+++ b/drivers/clk/x86/clk-cgu.h
@@ -1,18 +1,19 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
- * Copyright(c) 2020 Intel Corporation.
- * Zhu YiXin <yixin.zhu@intel.com>
- * Rahul Tanwar <rahul.tanwar@intel.com>
+ * Copyright (C) 2020-2022 MaxLinear, Inc.
+ * Copyright (C) 2020 Intel Corporation.
+ * Zhu Yixin <yzhu@maxlinear.com>
+ * Rahul Tanwar <rtanwar@maxlinear.com>
  */
 
 #ifndef __CLK_CGU_H
 #define __CLK_CGU_H
 
-#include <linux/io.h>
+#include <linux/regmap.h>
 
 struct lgm_clk_mux {
 	struct clk_hw hw;
-	void __iomem *membase;
+	struct regmap *membase;
 	unsigned int reg;
 	u8 shift;
 	u8 width;
@@ -22,7 +23,7 @@ struct lgm_clk_mux {
 
 struct lgm_clk_divider {
 	struct clk_hw hw;
-	void __iomem *membase;
+	struct regmap *membase;
 	unsigned int reg;
 	u8 shift;
 	u8 width;
@@ -35,7 +36,7 @@ struct lgm_clk_divider {
 
 struct lgm_clk_ddiv {
 	struct clk_hw hw;
-	void __iomem *membase;
+	struct regmap *membase;
 	unsigned int reg;
 	u8 shift0;
 	u8 width0;
@@ -53,7 +54,7 @@ struct lgm_clk_ddiv {
 
 struct lgm_clk_gate {
 	struct clk_hw hw;
-	void __iomem *membase;
+	struct regmap *membase;
 	unsigned int reg;
 	u8 shift;
 	unsigned long flags;
@@ -77,7 +78,7 @@ enum lgm_clk_type {
  * @clk_data: array of hw clocks and clk number.
  */
 struct lgm_clk_provider {
-	void __iomem *membase;
+	struct regmap *membase;
 	struct device_node *np;
 	struct device *dev;
 	struct clk_hw_onecell_data clk_data;
@@ -92,7 +93,7 @@ enum pll_type {
 
 struct lgm_clk_pll {
 	struct clk_hw hw;
-	void __iomem *membase;
+	struct regmap *membase;
 	unsigned int reg;
 	unsigned long flags;
 	enum pll_type type;
@@ -300,29 +301,32 @@ struct lgm_clk_branch {
 		.div = _d,					\
 	}
 
-static inline void lgm_set_clk_val(void __iomem *membase, u32 reg,
+static inline void lgm_set_clk_val(struct regmap *membase, u32 reg,
 				   u8 shift, u8 width, u32 set_val)
 {
 	u32 mask = (GENMASK(width - 1, 0) << shift);
-	u32 regval;
 
-	regval = readl(membase + reg);
-	regval = (regval & ~mask) | ((set_val << shift) & mask);
-	writel(regval, membase + reg);
+	regmap_update_bits(membase, reg, mask, set_val << shift);
 }
 
-static inline u32 lgm_get_clk_val(void __iomem *membase, u32 reg,
+static inline u32 lgm_get_clk_val(struct regmap *membase, u32 reg,
 				  u8 shift, u8 width)
 {
 	u32 mask = (GENMASK(width - 1, 0) << shift);
 	u32 val;
 
-	val = readl(membase + reg);
+	if (regmap_read(membase, reg, &val)) {
+		WARN_ONCE(1, "Failed to read clk reg: 0x%x\n", reg);
+		return 0;
+	}
+
 	val = (val & mask) >> shift;
 
 	return val;
 }
 
+
+
 int lgm_clk_register_branches(struct lgm_clk_provider *ctx,
 			      const struct lgm_clk_branch *list,
 			      unsigned int nr_clk);
diff --git a/drivers/clk/x86/clk-lgm.c b/drivers/clk/x86/clk-lgm.c
index 020f4e83a5cc..4fa2bcaf71c8 100644
--- a/drivers/clk/x86/clk-lgm.c
+++ b/drivers/clk/x86/clk-lgm.c
@@ -1,10 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
+ * Copyright (C) 2020-2022 MaxLinear, Inc.
  * Copyright (C) 2020 Intel Corporation.
- * Zhu YiXin <yixin.zhu@intel.com>
- * Rahul Tanwar <rahul.tanwar@intel.com>
+ * Zhu Yixin <yzhu@maxlinear.com>
+ * Rahul Tanwar <rtanwar@maxlinear.com>
  */
 #include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <dt-bindings/clock/intel,lgm-clk.h>
@@ -433,9 +435,12 @@ static int lgm_cgu_probe(struct platform_device *pdev)
 
 	ctx->clk_data.num = CLK_NR_CLKS;
 
-	ctx->membase = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(ctx->membase))
+	ctx->membase = syscon_node_to_regmap(np);
+	if (IS_ERR_OR_NULL(ctx->membase)) {
+		dev_err(dev, "Failed to get clk CGU iomem\n");
 		return PTR_ERR(ctx->membase);
+	}
+
 
 	ctx->np = np;
 	ctx->dev = dev;
-- 
2.17.1

Re: [PATCH RESEND v2 1/5] clk: mxl: Switch from direct readl/writel based IO to regmap based IO

[Stephen Boyd]

Quoting Rahul Tanwar (2022-09-21 23:24:24)
> diff --git a/drivers/clk/x86/clk-cgu-pll.c b/drivers/clk/x86/clk-cgu-pll.c
> index 3179557b5f78..c83083affe88 100644
> --- a/drivers/clk/x86/clk-cgu-pll.c
> +++ b/drivers/clk/x86/clk-cgu-pll.c
> @@ -1,8 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> + * Copyright (C) 2020-2022 MaxLinear, Inc.
>   * Copyright (C) 2020 Intel Corporation.
> - * Zhu YiXin <yixin.zhu@intel.com>
> - * Rahul Tanwar <rahul.tanwar@intel.com>
> + * Zhu Yixin <yzhu@maxlinear.com>

Does Zhu Yixin approve? They're not Cced on this patch.

> + * Rahul Tanwar <rtanwar@maxlinear.com>
>   */
>  
>  #include <linux/clk-provider.h>

Re: [PATCH RESEND v2 1/5] clk: mxl: Switch from direct readl/writel based IO to regmap based IO

[Rahul Tanwar]

Hi Stephen,

Thanks for review and response.


On 29/9/2022 8:14 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-21 23:24:24)
>> diff --git a/drivers/clk/x86/clk-cgu-pll.c b/drivers/clk/x86/clk-cgu-pll.c
>> index 3179557b5f78..c83083affe88 100644
>> --- a/drivers/clk/x86/clk-cgu-pll.c
>> +++ b/drivers/clk/x86/clk-cgu-pll.c
>> @@ -1,8 +1,9 @@
>>   // SPDX-License-Identifier: GPL-2.0
>>   /*
>> + * Copyright (C) 2020-2022 MaxLinear, Inc.
>>    * Copyright (C) 2020 Intel Corporation.
>> - * Zhu YiXin <yixin.zhu@intel.com>
>> - * Rahul Tanwar <rahul.tanwar@intel.com>
>> + * Zhu Yixin <yzhu@maxlinear.com>
> 
> Does Zhu Yixin approve? They're not Cced on this patch.


Zhu Yixin is part of linux-lgm-soc@maxlinear.com mail list which is CCed 
here. In v3, i will separately CC Yixin and request him to review again 
and provide a Reviewed-by tag if no concerns.

Thanks,
Rahul

> 
>> + * Rahul Tanwar <rtanwar@maxlinear.com>
>>    */
>>
>>   #include <linux/clk-provider.h>
> 
> 

[PATCH RESEND v2 2/5] clk: mxl: Remove unnecessary spinlocks

[Rahul Tanwar]

This driver is now switched from direct readl/writel based register
access to regmap based register acceess. Regmap already has its own
lock to serialize the register accesses across multiple cores.

Hence, there is no need for additional spinlocks in the driver.
Remove all spinlocks which are no longer required.

Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
---
 drivers/clk/x86/clk-cgu-pll.c | 13 ------
 drivers/clk/x86/clk-cgu.c     | 80 ++++-------------------------------
 drivers/clk/x86/clk-cgu.h     |  6 ---
 drivers/clk/x86/clk-lgm.c     |  1 -
 4 files changed, 9 insertions(+), 91 deletions(-)

diff --git a/drivers/clk/x86/clk-cgu-pll.c b/drivers/clk/x86/clk-cgu-pll.c
index c83083affe88..409dbf55f4ca 100644
--- a/drivers/clk/x86/clk-cgu-pll.c
+++ b/drivers/clk/x86/clk-cgu-pll.c
@@ -41,13 +41,10 @@ static unsigned long lgm_pll_recalc_rate(struct clk_hw *hw, unsigned long prate)
 {
 	struct lgm_clk_pll *pll = to_lgm_clk_pll(hw);
 	unsigned int div, mult, frac;
-	unsigned long flags;
 
-	spin_lock_irqsave(&pll->lock, flags);
 	mult = lgm_get_clk_val(pll->membase, PLL_REF_DIV(pll->reg), 0, 12);
 	div = lgm_get_clk_val(pll->membase, PLL_REF_DIV(pll->reg), 18, 6);
 	frac = lgm_get_clk_val(pll->membase, pll->reg, 2, 24);
-	spin_unlock_irqrestore(&pll->lock, flags);
 
 	if (pll->type == TYPE_LJPLL)
 		div *= 4;
@@ -58,12 +55,9 @@ static unsigned long lgm_pll_recalc_rate(struct clk_hw *hw, unsigned long prate)
 static int lgm_pll_is_enabled(struct clk_hw *hw)
 {
 	struct lgm_clk_pll *pll = to_lgm_clk_pll(hw);
-	unsigned long flags;
 	unsigned int ret;
 
-	spin_lock_irqsave(&pll->lock, flags);
 	ret = lgm_get_clk_val(pll->membase, pll->reg, 0, 1);
-	spin_unlock_irqrestore(&pll->lock, flags);
 
 	return ret;
 }
@@ -71,16 +65,13 @@ static int lgm_pll_is_enabled(struct clk_hw *hw)
 static int lgm_pll_enable(struct clk_hw *hw)
 {
 	struct lgm_clk_pll *pll = to_lgm_clk_pll(hw);
-	unsigned long flags;
 	u32 val;
 	int ret;
 
-	spin_lock_irqsave(&pll->lock, flags);
 	lgm_set_clk_val(pll->membase, pll->reg, 0, 1, 1);
 	ret = regmap_read_poll_timeout_atomic(pll->membase, pll->reg,
 					      val, (val & 0x1), 1, 100);
 
-	spin_unlock_irqrestore(&pll->lock, flags);
 
 	return ret;
 }
@@ -88,11 +79,8 @@ static int lgm_pll_enable(struct clk_hw *hw)
 static void lgm_pll_disable(struct clk_hw *hw)
 {
 	struct lgm_clk_pll *pll = to_lgm_clk_pll(hw);
-	unsigned long flags;
 
-	spin_lock_irqsave(&pll->lock, flags);
 	lgm_set_clk_val(pll->membase, pll->reg, 0, 1, 0);
-	spin_unlock_irqrestore(&pll->lock, flags);
 }
 
 static const struct clk_ops lgm_pll_ops = {
@@ -123,7 +111,6 @@ lgm_clk_register_pll(struct lgm_clk_provider *ctx,
 		return ERR_PTR(-ENOMEM);
 
 	pll->membase = ctx->membase;
-	pll->lock = ctx->lock;
 	pll->reg = list->reg;
 	pll->flags = list->flags;
 	pll->type = list->type;
diff --git a/drivers/clk/x86/clk-cgu.c b/drivers/clk/x86/clk-cgu.c
index f5f30a18f486..1f7e93de67bc 100644
--- a/drivers/clk/x86/clk-cgu.c
+++ b/drivers/clk/x86/clk-cgu.c
@@ -25,14 +25,10 @@
 static struct clk_hw *lgm_clk_register_fixed(struct lgm_clk_provider *ctx,
 					     const struct lgm_clk_branch *list)
 {
-	unsigned long flags;
 
-	if (list->div_flags & CLOCK_FLAG_VAL_INIT) {
-		spin_lock_irqsave(&ctx->lock, flags);
+	if (list->div_flags & CLOCK_FLAG_VAL_INIT)
 		lgm_set_clk_val(ctx->membase, list->div_off, list->div_shift,
 				list->div_width, list->div_val);
-		spin_unlock_irqrestore(&ctx->lock, flags);
-	}
 
 	return clk_hw_register_fixed_rate(NULL, list->name,
 					  list->parent_data[0].name,
@@ -42,33 +38,27 @@ static struct clk_hw *lgm_clk_register_fixed(struct lgm_clk_provider *ctx,
 static u8 lgm_clk_mux_get_parent(struct clk_hw *hw)
 {
 	struct lgm_clk_mux *mux = to_lgm_clk_mux(hw);
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&mux->lock, flags);
 	if (mux->flags & MUX_CLK_SW)
 		val = mux->reg;
 	else
 		val = lgm_get_clk_val(mux->membase, mux->reg, mux->shift,
 				      mux->width);
-	spin_unlock_irqrestore(&mux->lock, flags);
 	return clk_mux_val_to_index(hw, NULL, mux->flags, val);
 }
 
 static int lgm_clk_mux_set_parent(struct clk_hw *hw, u8 index)
 {
 	struct lgm_clk_mux *mux = to_lgm_clk_mux(hw);
-	unsigned long flags;
 	u32 val;
 
 	val = clk_mux_index_to_val(NULL, mux->flags, index);
-	spin_lock_irqsave(&mux->lock, flags);
 	if (mux->flags & MUX_CLK_SW)
 		mux->reg = val;
 	else
 		lgm_set_clk_val(mux->membase, mux->reg, mux->shift,
 				mux->width, val);
-	spin_unlock_irqrestore(&mux->lock, flags);
 
 	return 0;
 }
@@ -91,7 +81,7 @@ static struct clk_hw *
 lgm_clk_register_mux(struct lgm_clk_provider *ctx,
 		     const struct lgm_clk_branch *list)
 {
-	unsigned long flags, cflags = list->mux_flags;
+	unsigned long cflags = list->mux_flags;
 	struct device *dev = ctx->dev;
 	u8 shift = list->mux_shift;
 	u8 width = list->mux_width;
@@ -112,7 +102,6 @@ lgm_clk_register_mux(struct lgm_clk_provider *ctx,
 	init.num_parents = list->num_parents;
 
 	mux->membase = ctx->membase;
-	mux->lock = ctx->lock;
 	mux->reg = reg;
 	mux->shift = shift;
 	mux->width = width;
@@ -124,11 +113,8 @@ lgm_clk_register_mux(struct lgm_clk_provider *ctx,
 	if (ret)
 		return ERR_PTR(ret);
 
-	if (cflags & CLOCK_FLAG_VAL_INIT) {
-		spin_lock_irqsave(&mux->lock, flags);
+	if (cflags & CLOCK_FLAG_VAL_INIT)
 		lgm_set_clk_val(mux->membase, reg, shift, width, list->mux_val);
-		spin_unlock_irqrestore(&mux->lock, flags);
-	}
 
 	return hw;
 }
@@ -137,13 +123,10 @@ static unsigned long
 lgm_clk_divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 {
 	struct lgm_clk_divider *divider = to_lgm_clk_divider(hw);
-	unsigned long flags;
 	unsigned int val;
 
-	spin_lock_irqsave(&divider->lock, flags);
 	val = lgm_get_clk_val(divider->membase, divider->reg,
 			      divider->shift, divider->width);
-	spin_unlock_irqrestore(&divider->lock, flags);
 
 	return divider_recalc_rate(hw, parent_rate, val, divider->table,
 				   divider->flags, divider->width);
@@ -164,7 +147,6 @@ lgm_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 			 unsigned long prate)
 {
 	struct lgm_clk_divider *divider = to_lgm_clk_divider(hw);
-	unsigned long flags;
 	int value;
 
 	value = divider_get_val(rate, prate, divider->table,
@@ -172,10 +154,8 @@ lgm_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (value < 0)
 		return value;
 
-	spin_lock_irqsave(&divider->lock, flags);
 	lgm_set_clk_val(divider->membase, divider->reg,
 			divider->shift, divider->width, value);
-	spin_unlock_irqrestore(&divider->lock, flags);
 
 	return 0;
 }
@@ -183,12 +163,9 @@ lgm_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 static int lgm_clk_divider_enable_disable(struct clk_hw *hw, int enable)
 {
 	struct lgm_clk_divider *div = to_lgm_clk_divider(hw);
-	unsigned long flags;
 
-	spin_lock_irqsave(&div->lock, flags);
 	lgm_set_clk_val(div->membase, div->reg, div->shift_gate,
 			div->width_gate, enable);
-	spin_unlock_irqrestore(&div->lock, flags);
 	return 0;
 }
 
@@ -214,7 +191,7 @@ static struct clk_hw *
 lgm_clk_register_divider(struct lgm_clk_provider *ctx,
 			 const struct lgm_clk_branch *list)
 {
-	unsigned long flags, cflags = list->div_flags;
+	unsigned long cflags = list->div_flags;
 	struct device *dev = ctx->dev;
 	struct lgm_clk_divider *div;
 	struct clk_init_data init = {};
@@ -237,7 +214,6 @@ lgm_clk_register_divider(struct lgm_clk_provider *ctx,
 	init.num_parents = 1;
 
 	div->membase = ctx->membase;
-	div->lock = ctx->lock;
 	div->reg = reg;
 	div->shift = shift;
 	div->width = width;
@@ -252,11 +228,8 @@ lgm_clk_register_divider(struct lgm_clk_provider *ctx,
 	if (ret)
 		return ERR_PTR(ret);
 
-	if (cflags & CLOCK_FLAG_VAL_INIT) {
-		spin_lock_irqsave(&div->lock, flags);
+	if (cflags & CLOCK_FLAG_VAL_INIT)
 		lgm_set_clk_val(div->membase, reg, shift, width, list->div_val);
-		spin_unlock_irqrestore(&div->lock, flags);
-	}
 
 	return hw;
 }
@@ -265,7 +238,6 @@ static struct clk_hw *
 lgm_clk_register_fixed_factor(struct lgm_clk_provider *ctx,
 			      const struct lgm_clk_branch *list)
 {
-	unsigned long flags;
 	struct clk_hw *hw;
 
 	hw = clk_hw_register_fixed_factor(ctx->dev, list->name,
@@ -274,12 +246,9 @@ lgm_clk_register_fixed_factor(struct lgm_clk_provider *ctx,
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
 
-	if (list->div_flags & CLOCK_FLAG_VAL_INIT) {
-		spin_lock_irqsave(&ctx->lock, flags);
+	if (list->div_flags & CLOCK_FLAG_VAL_INIT)
 		lgm_set_clk_val(ctx->membase, list->div_off, list->div_shift,
 				list->div_width, list->div_val);
-		spin_unlock_irqrestore(&ctx->lock, flags);
-	}
 
 	return hw;
 }
@@ -287,13 +256,10 @@ lgm_clk_register_fixed_factor(struct lgm_clk_provider *ctx,
 static int lgm_clk_gate_enable(struct clk_hw *hw)
 {
 	struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
-	unsigned long flags;
 	unsigned int reg;
 
-	spin_lock_irqsave(&gate->lock, flags);
 	reg = GATE_HW_REG_EN(gate->reg);
 	lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
-	spin_unlock_irqrestore(&gate->lock, flags);
 
 	return 0;
 }
@@ -301,25 +267,19 @@ static int lgm_clk_gate_enable(struct clk_hw *hw)
 static void lgm_clk_gate_disable(struct clk_hw *hw)
 {
 	struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
-	unsigned long flags;
 	unsigned int reg;
 
-	spin_lock_irqsave(&gate->lock, flags);
 	reg = GATE_HW_REG_DIS(gate->reg);
 	lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
-	spin_unlock_irqrestore(&gate->lock, flags);
 }
 
 static int lgm_clk_gate_is_enabled(struct clk_hw *hw)
 {
 	struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
 	unsigned int reg, ret;
-	unsigned long flags;
 
-	spin_lock_irqsave(&gate->lock, flags);
 	reg = GATE_HW_REG_STAT(gate->reg);
 	ret = lgm_get_clk_val(gate->membase, reg, gate->shift, 1);
-	spin_unlock_irqrestore(&gate->lock, flags);
 
 	return ret;
 }
@@ -334,7 +294,7 @@ static struct clk_hw *
 lgm_clk_register_gate(struct lgm_clk_provider *ctx,
 		      const struct lgm_clk_branch *list)
 {
-	unsigned long flags, cflags = list->gate_flags;
+	unsigned long cflags = list->gate_flags;
 	const char *pname = list->parent_data[0].name;
 	struct device *dev = ctx->dev;
 	u8 shift = list->gate_shift;
@@ -355,7 +315,6 @@ lgm_clk_register_gate(struct lgm_clk_provider *ctx,
 	init.num_parents = pname ? 1 : 0;
 
 	gate->membase = ctx->membase;
-	gate->lock = ctx->lock;
 	gate->reg = reg;
 	gate->shift = shift;
 	gate->flags = cflags;
@@ -367,9 +326,7 @@ lgm_clk_register_gate(struct lgm_clk_provider *ctx,
 		return ERR_PTR(ret);
 
 	if (cflags & CLOCK_FLAG_VAL_INIT) {
-		spin_lock_irqsave(&gate->lock, flags);
 		lgm_set_clk_val(gate->membase, reg, shift, 1, list->gate_val);
-		spin_unlock_irqrestore(&gate->lock, flags);
 	}
 
 	return hw;
@@ -444,24 +401,18 @@ lgm_clk_ddiv_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 static int lgm_clk_ddiv_enable(struct clk_hw *hw)
 {
 	struct lgm_clk_ddiv *ddiv = to_lgm_clk_ddiv(hw);
-	unsigned long flags;
 
-	spin_lock_irqsave(&ddiv->lock, flags);
 	lgm_set_clk_val(ddiv->membase, ddiv->reg, ddiv->shift_gate,
 			ddiv->width_gate, 1);
-	spin_unlock_irqrestore(&ddiv->lock, flags);
 	return 0;
 }
 
 static void lgm_clk_ddiv_disable(struct clk_hw *hw)
 {
 	struct lgm_clk_ddiv *ddiv = to_lgm_clk_ddiv(hw);
-	unsigned long flags;
 
-	spin_lock_irqsave(&ddiv->lock, flags);
 	lgm_set_clk_val(ddiv->membase, ddiv->reg, ddiv->shift_gate,
 			ddiv->width_gate, 0);
-	spin_unlock_irqrestore(&ddiv->lock, flags);
 }
 
 static int
@@ -498,32 +449,25 @@ lgm_clk_ddiv_set_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct lgm_clk_ddiv *ddiv = to_lgm_clk_ddiv(hw);
 	u32 div, ddiv1, ddiv2;
-	unsigned long flags;
 
 	div = DIV_ROUND_CLOSEST_ULL((u64)prate, rate);
 
-	spin_lock_irqsave(&ddiv->lock, flags);
 	if (lgm_get_clk_val(ddiv->membase, ddiv->reg, ddiv->shift2, 1)) {
 		div = DIV_ROUND_CLOSEST_ULL((u64)div, 5);
 		div = div * 2;
 	}
 
-	if (div <= 0) {
-		spin_unlock_irqrestore(&ddiv->lock, flags);
+	if (div <= 0)
 		return -EINVAL;
-	}
 
-	if (lgm_clk_get_ddiv_val(div, &ddiv1, &ddiv2)) {
-		spin_unlock_irqrestore(&ddiv->lock, flags);
+	if (lgm_clk_get_ddiv_val(div, &ddiv1, &ddiv2))
 		return -EINVAL;
-	}
 
 	lgm_set_clk_val(ddiv->membase, ddiv->reg, ddiv->shift0, ddiv->width0,
 			ddiv1 - 1);
 
 	lgm_set_clk_val(ddiv->membase, ddiv->reg,  ddiv->shift1, ddiv->width1,
 			ddiv2 - 1);
-	spin_unlock_irqrestore(&ddiv->lock, flags);
 
 	return 0;
 }
@@ -534,18 +478,15 @@ lgm_clk_ddiv_round_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct lgm_clk_ddiv *ddiv = to_lgm_clk_ddiv(hw);
 	u32 div, ddiv1, ddiv2;
-	unsigned long flags;
 	u64 rate64;
 
 	div = DIV_ROUND_CLOSEST_ULL((u64)*prate, rate);
 
 	/* if predivide bit is enabled, modify div by factor of 2.5 */
-	spin_lock_irqsave(&ddiv->lock, flags);
 	if (lgm_get_clk_val(ddiv->membase, ddiv->reg, ddiv->shift2, 1)) {
 		div = div * 2;
 		div = DIV_ROUND_CLOSEST_ULL((u64)div, 5);
 	}
-	spin_unlock_irqrestore(&ddiv->lock, flags);
 
 	if (div <= 0)
 		return *prate;
@@ -559,12 +500,10 @@ lgm_clk_ddiv_round_rate(struct clk_hw *hw, unsigned long rate,
 	do_div(rate64, ddiv2);
 
 	/* if predivide bit is enabled, modify rounded rate by factor of 2.5 */
-	spin_lock_irqsave(&ddiv->lock, flags);
 	if (lgm_get_clk_val(ddiv->membase, ddiv->reg, ddiv->shift2, 1)) {
 		rate64 = rate64 * 2;
 		rate64 = DIV_ROUND_CLOSEST_ULL(rate64, 5);
 	}
-	spin_unlock_irqrestore(&ddiv->lock, flags);
 
 	return rate64;
 }
@@ -601,7 +540,6 @@ int lgm_clk_register_ddiv(struct lgm_clk_provider *ctx,
 		init.num_parents = 1;
 
 		ddiv->membase = ctx->membase;
-		ddiv->lock = ctx->lock;
 		ddiv->reg = list->reg;
 		ddiv->shift0 = list->shift0;
 		ddiv->width0 = list->width0;
diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
index dbcb66468797..0aa0f35d63a0 100644
--- a/drivers/clk/x86/clk-cgu.h
+++ b/drivers/clk/x86/clk-cgu.h
@@ -18,7 +18,6 @@ struct lgm_clk_mux {
 	u8 shift;
 	u8 width;
 	unsigned long flags;
-	spinlock_t lock;
 };
 
 struct lgm_clk_divider {
@@ -31,7 +30,6 @@ struct lgm_clk_divider {
 	u8 width_gate;
 	unsigned long flags;
 	const struct clk_div_table *table;
-	spinlock_t lock;
 };
 
 struct lgm_clk_ddiv {
@@ -49,7 +47,6 @@ struct lgm_clk_ddiv {
 	unsigned int mult;
 	unsigned int div;
 	unsigned long flags;
-	spinlock_t lock;
 };
 
 struct lgm_clk_gate {
@@ -58,7 +55,6 @@ struct lgm_clk_gate {
 	unsigned int reg;
 	u8 shift;
 	unsigned long flags;
-	spinlock_t lock;
 };
 
 enum lgm_clk_type {
@@ -82,7 +78,6 @@ struct lgm_clk_provider {
 	struct device_node *np;
 	struct device *dev;
 	struct clk_hw_onecell_data clk_data;
-	spinlock_t lock;
 };
 
 enum pll_type {
@@ -97,7 +92,6 @@ struct lgm_clk_pll {
 	unsigned int reg;
 	unsigned long flags;
 	enum pll_type type;
-	spinlock_t lock;
 };
 
 /**
diff --git a/drivers/clk/x86/clk-lgm.c b/drivers/clk/x86/clk-lgm.c
index 4fa2bcaf71c8..e312af42e97a 100644
--- a/drivers/clk/x86/clk-lgm.c
+++ b/drivers/clk/x86/clk-lgm.c
@@ -444,7 +444,6 @@ static int lgm_cgu_probe(struct platform_device *pdev)
 
 	ctx->np = np;
 	ctx->dev = dev;
-	spin_lock_init(&ctx->lock);
 
 	ret = lgm_clk_register_plls(ctx, lgm_pll_clks,
 				    ARRAY_SIZE(lgm_pll_clks));
-- 
2.17.1

Re: [PATCH RESEND v2 2/5] clk: mxl: Remove unnecessary spinlocks

[Stephen Boyd]

Quoting Rahul Tanwar (2022-09-21 23:24:25)
> This driver is now switched from direct readl/writel based register
> access to regmap based register acceess. Regmap already has its own
> lock to serialize the register accesses across multiple cores.
> 
> Hence, there is no need for additional spinlocks in the driver.
> Remove all spinlocks which are no longer required.

Can you confirm that the driver doesn't do a read, modify, write (rmw)
sequence where during the modification another CPU can rmw and corrupt
the previous "read"?

Re: [PATCH RESEND v2 2/5] clk: mxl: Remove unnecessary spinlocks

[Rahul Tanwar]

On 29/9/2022 8:16 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-21 23:24:25)
>> This driver is now switched from direct readl/writel based register
>> access to regmap based register acceess. Regmap already has its own
>> lock to serialize the register accesses across multiple cores.
>>
>> Hence, there is no need for additional spinlocks in the driver.
>> Remove all spinlocks which are no longer required.
> 
> Can you confirm that the driver doesn't do a read, modify, write (rmw)
> sequence where during the modification another CPU can rmw and corrupt
> the previous "read"?


For rmw, driver uses regmap_update_bits() API. All regmap API's use 
their own spinlocks before doing any read or write or rmw.

Patch 1/5 switches from direct readl/writel to regmap based API's for
register read/write/rmw in which case additional driver spinlock becomes
redundant. So this patch 2/5 removes redundant driver spin locks.

Thanks,
Rahul


> 
> 

[PATCH RESEND v2 3/5] clk: mxl: Avoid disabling gate clocks from clk driver

[Rahul Tanwar]

In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
from EPU (power management IP) in certain power saving modes. If gate
clocks are allowed to be enabled/disabled from CGU clk driver, then
there arises a conflict where in case clk driver disables a gate clk,
and then EPU tries to disable the same gate clk, then it will hang
polling for the clk gated successful status.

To avoid such a conflict, disable gate clocks enabling/disabling from
CGU clk driver. But add a GATE_CLK_HW flag to control this in order to
be backward compatible with other SoCs which share the same CGU IP but
not the same EPU IP.

Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
---
 drivers/clk/x86/clk-cgu.c | 32 ++++++++++++++++++++++++--------
 drivers/clk/x86/clk-cgu.h |  1 +
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/x86/clk-cgu.c b/drivers/clk/x86/clk-cgu.c
index 1f7e93de67bc..d24173cfe0b0 100644
--- a/drivers/clk/x86/clk-cgu.c
+++ b/drivers/clk/x86/clk-cgu.c
@@ -258,8 +258,12 @@ static int lgm_clk_gate_enable(struct clk_hw *hw)
 	struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
 	unsigned int reg;
 
-	reg = GATE_HW_REG_EN(gate->reg);
-	lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
+	if (gate->flags & GATE_CLK_HW) {
+		reg = GATE_HW_REG_EN(gate->reg);
+		lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
+	} else {
+		gate->reg = 1;
+	}
 
 	return 0;
 }
@@ -269,8 +273,12 @@ static void lgm_clk_gate_disable(struct clk_hw *hw)
 	struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
 	unsigned int reg;
 
-	reg = GATE_HW_REG_DIS(gate->reg);
-	lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
+	if (gate->flags & GATE_CLK_HW) {
+		reg = GATE_HW_REG_DIS(gate->reg);
+		lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
+	} else {
+		gate->reg = 0;
+	}
 }
 
 static int lgm_clk_gate_is_enabled(struct clk_hw *hw)
@@ -278,8 +286,12 @@ static int lgm_clk_gate_is_enabled(struct clk_hw *hw)
 	struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
 	unsigned int reg, ret;
 
-	reg = GATE_HW_REG_STAT(gate->reg);
-	ret = lgm_get_clk_val(gate->membase, reg, gate->shift, 1);
+	if (gate->flags & GATE_CLK_HW) {
+		reg = GATE_HW_REG_STAT(gate->reg);
+		ret = lgm_get_clk_val(gate->membase, reg, gate->shift, 1);
+	} else {
+		ret = gate->reg;
+	}
 
 	return ret;
 }
@@ -315,7 +327,8 @@ lgm_clk_register_gate(struct lgm_clk_provider *ctx,
 	init.num_parents = pname ? 1 : 0;
 
 	gate->membase = ctx->membase;
-	gate->reg = reg;
+	if (cflags & GATE_CLK_HW)
+		gate->reg = reg;
 	gate->shift = shift;
 	gate->flags = cflags;
 	gate->hw.init = &init;
@@ -326,7 +339,10 @@ lgm_clk_register_gate(struct lgm_clk_provider *ctx,
 		return ERR_PTR(ret);
 
 	if (cflags & CLOCK_FLAG_VAL_INIT) {
-		lgm_set_clk_val(gate->membase, reg, shift, 1, list->gate_val);
+		if (cflags & GATE_CLK_HW)
+			lgm_set_clk_val(gate->membase, reg, shift, 1, list->gate_val);
+		else
+			gate->reg = 1;
 	}
 
 	return hw;
diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
index 0aa0f35d63a0..73ce84345f81 100644
--- a/drivers/clk/x86/clk-cgu.h
+++ b/drivers/clk/x86/clk-cgu.h
@@ -197,6 +197,7 @@ struct lgm_clk_branch {
 /* clock flags definition */
 #define CLOCK_FLAG_VAL_INIT	BIT(16)
 #define MUX_CLK_SW		BIT(17)
+#define GATE_CLK_HW		BIT(18)
 
 #define LGM_MUX(_id, _name, _pdata, _f, _reg,		\
 		_shift, _width, _cf, _v)		\
-- 
2.17.1

Re: [PATCH RESEND v2 3/5] clk: mxl: Avoid disabling gate clocks from clk driver

[Stephen Boyd]

Quoting Rahul Tanwar (2022-09-21 23:24:26)
> In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
> from EPU (power management IP) in certain power saving modes. If gate
> clocks are allowed to be enabled/disabled from CGU clk driver, then
> there arises a conflict where in case clk driver disables a gate clk,
> and then EPU tries to disable the same gate clk, then it will hang
> polling for the clk gated successful status.

Is there any point in registering these clks when they're not supposed
to be controlled from Linux?

Re: [PATCH RESEND v2 3/5] clk: mxl: Avoid disabling gate clocks from clk driver

[Rahul Tanwar]

On 29/9/2022 8:17 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-21 23:24:26)
>> In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
>> from EPU (power management IP) in certain power saving modes. If gate
>> clocks are allowed to be enabled/disabled from CGU clk driver, then
>> there arises a conflict where in case clk driver disables a gate clk,
>> and then EPU tries to disable the same gate clk, then it will hang
>> polling for the clk gated successful status.
> 
> Is there any point in registering these clks when they're not supposed
> to be controlled from Linux?


As mentioned in the full commit log, only reason to register these clks 
is to be backward compatible with older versions of similar SoC's which 
reuse the same clk CGU IP but do not use same power management IP. Such 
older SoCs also use the same clk driver and for them these clks are 
required to be controlled by clk ops from Linux.

Thanks,
Rahul


> 
> 

Re: [PATCH RESEND v2 3/5] clk: mxl: Avoid disabling gate clocks from clk driver

[Rahul Tanwar]

Hi Stephen,

On 30/9/2022 9:01 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-28 22:45:59)
>> On 29/9/2022 8:17 am, Stephen Boyd wrote:
>>> This email was sent from outside of MaxLinear.
>>>
>>>
>>> Quoting Rahul Tanwar (2022-09-21 23:24:26)
>>>> In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
>>>> from EPU (power management IP) in certain power saving modes. If gate
>>>> clocks are allowed to be enabled/disabled from CGU clk driver, then
>>>> there arises a conflict where in case clk driver disables a gate clk,
>>>> and then EPU tries to disable the same gate clk, then it will hang
>>>> polling for the clk gated successful status.
>>>
>>> Is there any point in registering these clks when they're not supposed
>>> to be controlled from Linux?
>>
>>
>> As mentioned in the full commit log, only reason to register these clks
>> is to be backward compatible with older versions of similar SoC's which
>> reuse the same clk CGU IP but do not use same power management IP. Such
>> older SoCs also use the same clk driver and for them these clks are
>> required to be controlled by clk ops from Linux.
>>
> 
> Why is the clk driver probing on the new SoCs? Is it providing
> something? Can we detect that the power management IP exists and not
> register these clks?
> 

We discussed in the team about not registering gate clks at all as you 
mentioned. But if we do that, all peripheral drivers that use these clks 
would need modifications so their probe does not fail due to failure 
returns of clk related standard calls for e.g devm_clk_get(), 
clk_prepare_enable(). These are standard calls in probe for all the 
drivers and a lot of them use gate clks. So this would need a lot of 
changes with possibility of breaking working functionalities.

Also, i incorrectly mentioned about the reason being backward 
compatibility with older SoCs. Main reason is to support different power 
profiles use cases as per end product requirements some of which might 
control it from clk framework i.e. this driver. We keep a internal 
driver flag just for this purpose to provide this flexibility depending 
on the use case which is what we have used here.

I am sending v3 with more clear & correct description about it to 
justify the need for these changes.

Thanks,
Rahul


> 

Re: [PATCH RESEND v2 3/5] clk: mxl: Avoid disabling gate clocks from clk driver

[Rahul Tanwar]

Hi Stephen,

On 6/10/2022 4:20 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-10-05 02:36:00)
>>>>
>>>
>>> Why is the clk driver probing on the new SoCs? Is it providing
>>> something? Can we detect that the power management IP exists and not
>>> register these clks?
>>>
>>
>> We discussed in the team about not registering gate clks at all as you
>> mentioned. But if we do that, all peripheral drivers that use these clks
>> would need modifications so their probe does not fail due to failure
>> returns of clk related standard calls for e.g devm_clk_get(),
>> clk_prepare_enable(). These are standard calls in probe for all the
>> drivers and a lot of them use gate clks. So this would need a lot of
>> changes with possibility of breaking working functionalities.
> 
> Why? We can have clk_get() return NULL when these clks can't be used.
> This is how we implement "dummy" clks on systems where the clk provider
> can provide the clk but there's nothing to do for the clk operations.
> The clk API treats NULL as a valid clk but bails out early when calling
> any consumer APIs.
> 

I agree that what you are suggesting is the ideal way to handle such 
clks. If i understand you correctly, you are suggesting below (please 
correct me if i am mistaken):

1. Disable/remove such clocks from corresponding driver's devicetree 
nodes. This will make devm_clk_get() or clk_get() return failure.

2. Modify all drivers which use such clks

from:

    clk = devm_clk_get(...);
    if (IS_ERR(clk))
        return -ERROR;
    clk_prepare_enable(clk);
    clk_get/set_rate();
    ...

to:
    clk = devm_clk_get(...);
    if (!(IS_ERR(clk)) {
        clk_prepare_enable(clk);
        clk_get/set_rate();
        ...
    } else {
       // print error info - do nothing, no clk_ops calls
    }

But the problem that we have is that 80% of the clks that we use fall 
under this category (around 65 clks). And if we follow this approach, 
then we will have to make above changes in all of the drivers which use 
these clks & retest them again. That seems like a overhaul. We already 
keep a internal driver flag in each clk entry data structure and we 
already use it in the driver for other types of clks for e.g MUX_CLKs. 
So using the internal flag to handle this becomes a preferable & 
existing driver design aligned approach for us.

Thanks,
Rahul

>>
>> Also, i incorrectly mentioned about the reason being backward
>> compatibility with older SoCs. Main reason is to support different power
>> profiles use cases as per end product requirements some of which might
>> control it from clk framework i.e. this driver. We keep a internal
>> driver flag just for this purpose to provide this flexibility depending
>> on the use case which is what we have used here.
>>
>> I am sending v3 with more clear & correct description about it to
>> justify the need for these changes.
>>
> 
> Ok
> 
> 

Re: [PATCH RESEND v2 3/5] clk: mxl: Avoid disabling gate clocks from clk driver

[Rahul Tanwar]

Hi Stephen,

On 12/10/2022 1:43 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-10-11 00:28:56)
>>
>> I agree that what you are suggesting is the ideal way to handle such
>> clks. If i understand you correctly, you are suggesting below (please
>> correct me if i am mistaken):
>>
>> 1. Disable/remove such clocks from corresponding driver's devicetree
>> nodes. This will make devm_clk_get() or clk_get() return failure.
> 
> No. Don't remove anything from devicetree, simply make the clk provider
> return NULL when the clk_src_get() function pointer is asked for the clk
> index that represents the clks that have no abilities to do anything.
> 
>>
>> 2. Modify all drivers which use such clks
>>
>> from:
>>
>>      clk = devm_clk_get(...);
>>      if (IS_ERR(clk))
>>          return -ERROR;
>>      clk_prepare_enable(clk);
>>      clk_get/set_rate();
>>      ...
>>
>> to:
>>      clk = devm_clk_get(...);
>>      if (!(IS_ERR(clk)) {
>>          clk_prepare_enable(clk);
>>          clk_get/set_rate();
>>          ...
>>      } else {
>>         // print error info - do nothing, no clk_ops calls
>>      }
> 
> No. Nothing in the drivers changes. The 'clk' pointer will be NULL, that
> will not make the 'if (IS_ERR(clk))' condition be true, so return -ERROR
> won't happen. Similarly, clk_prepare_enable() will return 0 when called
> with a NULL pointer. Please try it out.
> 
>>
>> But the problem that we have is that 80% of the clks that we use fall
>> under this category (around 65 clks). And if we follow this approach,
>> then we will have to make above changes in all of the drivers which use
>> these clks & retest them again. That seems like a overhaul. We already
>> keep a internal driver flag in each clk entry data structure and we
>> already use it in the driver for other types of clks for e.g MUX_CLKs.
>> So using the internal flag to handle this becomes a preferable &
>> existing driver design aligned approach for us.
>>
> 
> There isn't an overhaul.
> 


After going through the code again together with your comments, now i 
understand exactly what you are suggesting. I will make the changes 
accordingly in v4. Thanks for your valuable inputs.

Rahul

> 

Re: [PATCH RESEND v2 3/5] clk: mxl: Avoid disabling gate clocks from clk driver

[Rahul Tanwar]

Hi Stephen,

On 6/10/2022 4:20 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-10-05 02:36:00)
>>>>
>>>
>>> Why is the clk driver probing on the new SoCs? Is it providing
>>> something? Can we detect that the power management IP exists and not
>>> register these clks?
>>>
>>
>> We discussed in the team about not registering gate clks at all as you
>> mentioned. But if we do that, all peripheral drivers that use these clks
>> would need modifications so their probe does not fail due to failure
>> returns of clk related standard calls for e.g devm_clk_get(),
>> clk_prepare_enable(). These are standard calls in probe for all the
>> drivers and a lot of them use gate clks. So this would need a lot of
>> changes with possibility of breaking working functionalities.
> 
> Why? We can have clk_get() return NULL when these clks can't be used.
> This is how we implement "dummy" clks on systems where the clk provider
> can provide the clk but there's nothing to do for the clk operations.
> The clk API treats NULL as a valid clk but bails out early when calling
> any consumer APIs.
> 


I agree that what you are suggesting is the ideal way to handle such
clks. If i understand you correctly, you are suggesting below (please
correct me if i am mistaken):

1. Disable/remove such clocks from corresponding driver's devicetree
nodes. This will make devm_clk_get() or clk_get() return failure.

2. Modify all drivers which use such clks

from:

     clk = devm_clk_get(...);
     if (IS_ERR(clk))
         return -ERROR;
     clk_prepare_enable(clk);
     clk_get/set_rate();
     ...

to:
     clk = devm_clk_get(...);
     if (!(IS_ERR(clk)) {
         clk_prepare_enable(clk);
         clk_get/set_rate();
         ...
     } else {
        // print error info - do nothing, no clk_ops calls
     }

But the problem that we have is that 80% of the clks that we use fall
under this category (around 65 clks). And if we follow this approach,
then we will have to make above changes in all of the drivers which use
these clks & retest them again. That seems like a overhaul. We already
keep a internal driver flag in each clk entry data structure and we
already use it in the driver for other types of clks for e.g MUX_CLKs.
So using the internal flag to handle this becomes a preferable &
existing driver design aligned approach for us.

Thanks,
Rahul


>>
>> Also, i incorrectly mentioned about the reason being backward
>> compatibility with older SoCs. Main reason is to support different power
>> profiles use cases as per end product requirements some of which might
>> control it from clk framework i.e. this driver. We keep a internal
>> driver flag just for this purpose to provide this flexibility depending
>> on the use case which is what we have used here.
>>
>> I am sending v3 with more clear & correct description about it to
>> justify the need for these changes.
>>
> 
> Ok
> 
> 

Re: [PATCH RESEND v2 3/5] clk: mxl: Avoid disabling gate clocks from clk driver

[Rahul Tanwar]

[Resend due to mail delivery failure in earlier reply - one email id got 
corrupted somehow in earlier reply]

Hi Stephen,


On 30/9/2022 9:01 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-28 22:45:59)
>> On 29/9/2022 8:17 am, Stephen Boyd wrote:
>>> This email was sent from outside of MaxLinear.
>>>
>>>
>>> Quoting Rahul Tanwar (2022-09-21 23:24:26)
>>>> In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
>>>> from EPU (power management IP) in certain power saving modes. If gate
>>>> clocks are allowed to be enabled/disabled from CGU clk driver, then
>>>> there arises a conflict where in case clk driver disables a gate clk,
>>>> and then EPU tries to disable the same gate clk, then it will hang
>>>> polling for the clk gated successful status.
>>>
>>> Is there any point in registering these clks when they're not supposed
>>> to be controlled from Linux?
>>
>>
>> As mentioned in the full commit log, only reason to register these clks
>> is to be backward compatible with older versions of similar SoC's which
>> reuse the same clk CGU IP but do not use same power management IP. Such
>> older SoCs also use the same clk driver and for them these clks are
>> required to be controlled by clk ops from Linux.
>>
> 
> Why is the clk driver probing on the new SoCs? Is it providing
> something? Can we detect that the power management IP exists and not
> register these clks?
> 

We discussed in the team about not registering gate clks at all as you
mentioned. But if we do that, all peripheral drivers that use these clks
would need modifications so their probe does not fail due to failure
returns of clk related standard calls for e.g devm_clk_get(),
clk_prepare_enable(). These are standard calls in probe for all the
drivers and a lot of them use gate clks. So this would need a lot of
changes with possibility of breaking working functionalities.

Also, i incorrectly mentioned about the reason being backward
compatibility with older SoCs. Main reason is to support different power
profiles use cases as per end product requirements some of which might
control it from clk framework i.e. this driver. We keep a internal
driver flag just for this purpose to provide this flexibility depending
on the use case which is what we have used here.

I am sending v3 with more clear & correct description about it to
justify the need for these changes.

Thanks,
Rahul


> 

[PATCH RESEND v2 4/5] clk: mxl: Add validation for register reads/writes

[Rahul Tanwar]

Some clocks support parent clock dividers but they do not
support clock gating (clk enable/disable). Such types of
clocks might call API's for get/set_reg_val routines with
width as 0 during clk_prepare_enable() call. Handle such
cases by first validating width during clk_prepare_enable()
while still supporting clk_set_rate() correctly.

Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
---
 drivers/clk/x86/clk-cgu.h | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
index 73ce84345f81..46daf9ebd6c9 100644
--- a/drivers/clk/x86/clk-cgu.h
+++ b/drivers/clk/x86/clk-cgu.h
@@ -299,29 +299,51 @@ struct lgm_clk_branch {
 static inline void lgm_set_clk_val(struct regmap *membase, u32 reg,
 				   u8 shift, u8 width, u32 set_val)
 {
-	u32 mask = (GENMASK(width - 1, 0) << shift);
+	u32 mask;
 
+	/*
+	 * Some clocks support parent clock dividers but they do not
+	 * support clock gating (clk enable/disable). Such types of
+	 * clocks might call this function with width as 0 during
+	 * clk_prepare_enable() call. Handle such cases by not doing
+	 * anything during clk_prepare_enable() but handle clk_set_rate()
+	 * correctly
+	 */
+	if (!width)
+		return;
+
+	mask = (GENMASK(width - 1, 0) << shift);
 	regmap_update_bits(membase, reg, mask, set_val << shift);
 }
 
 static inline u32 lgm_get_clk_val(struct regmap *membase, u32 reg,
 				  u8 shift, u8 width)
 {
-	u32 mask = (GENMASK(width - 1, 0) << shift);
+	u32 mask;
 	u32 val;
 
+	/*
+	 * Some clocks support parent clock dividers but they do not
+	 * support clock gating (clk enable/disable). Such types of
+	 * clocks might call this function with width as 0 during
+	 * clk_prepare_enable() call. Handle such cases by not doing
+	 * anything during clk_prepare_enable() but handle clk_set_rate()
+	 * correctly
+	 */
+	if (!width)
+		return 0;
+
 	if (regmap_read(membase, reg, &val)) {
 		WARN_ONCE(1, "Failed to read clk reg: 0x%x\n", reg);
 		return 0;
 	}
 
+	mask = (GENMASK(width - 1, 0) << shift);
 	val = (val & mask) >> shift;
 
 	return val;
 }
 
-
-
 int lgm_clk_register_branches(struct lgm_clk_provider *ctx,
 			      const struct lgm_clk_branch *list,
 			      unsigned int nr_clk);
-- 
2.17.1

Re: [PATCH RESEND v2 4/5] clk: mxl: Add validation for register reads/writes

[Stephen Boyd]

Quoting Rahul Tanwar (2022-09-21 23:24:27)
> Some clocks support parent clock dividers but they do not
> support clock gating (clk enable/disable). Such types of
> clocks might call API's for get/set_reg_val routines with
> width as 0 during clk_prepare_enable() call. Handle such
> cases by first validating width during clk_prepare_enable()
> while still supporting clk_set_rate() correctly.
> 
> Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
> ---
>  drivers/clk/x86/clk-cgu.h | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
> index 73ce84345f81..46daf9ebd6c9 100644
> --- a/drivers/clk/x86/clk-cgu.h
> +++ b/drivers/clk/x86/clk-cgu.h
> @@ -299,29 +299,51 @@ struct lgm_clk_branch {
>  static inline void lgm_set_clk_val(struct regmap *membase, u32 reg,
>                                    u8 shift, u8 width, u32 set_val)
>  {
> -       u32 mask = (GENMASK(width - 1, 0) << shift);
> +       u32 mask;
>  
> +       /*
> +        * Some clocks support parent clock dividers but they do not
> +        * support clock gating (clk enable/disable). Such types of
> +        * clocks might call this function with width as 0 during
> +        * clk_prepare_enable() call. Handle such cases by not doing
> +        * anything during clk_prepare_enable() but handle clk_set_rate()
> +        * correctly
> +        */
> +       if (!width)
> +               return;

Why are the clk_ops assigned in a way that makes the code get here? Why
can't we have different clk_ops, or not register the clks at all, when
the hardware can't be written?

> +
> +       mask = (GENMASK(width - 1, 0) << shift);
>         regmap_update_bits(membase, reg, mask, set_val << shift);

Re: [PATCH RESEND v2 4/5] clk: mxl: Add validation for register reads/writes

[Rahul Tanwar]

On 29/9/2022 8:20 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-21 23:24:27)
>> Some clocks support parent clock dividers but they do not
>> support clock gating (clk enable/disable). Such types of
>> clocks might call API's for get/set_reg_val routines with
>> width as 0 during clk_prepare_enable() call. Handle such
>> cases by first validating width during clk_prepare_enable()
>> while still supporting clk_set_rate() correctly.
>>
>> Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
>> ---
>>   drivers/clk/x86/clk-cgu.h | 30 ++++++++++++++++++++++++++----
>>   1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
>> index 73ce84345f81..46daf9ebd6c9 100644
>> --- a/drivers/clk/x86/clk-cgu.h
>> +++ b/drivers/clk/x86/clk-cgu.h
>> @@ -299,29 +299,51 @@ struct lgm_clk_branch {
>>   static inline void lgm_set_clk_val(struct regmap *membase, u32 reg,
>>                                     u8 shift, u8 width, u32 set_val)
>>   {
>> -       u32 mask = (GENMASK(width - 1, 0) << shift);
>> +       u32 mask;
>>
>> +       /*
>> +        * Some clocks support parent clock dividers but they do not
>> +        * support clock gating (clk enable/disable). Such types of
>> +        * clocks might call this function with width as 0 during
>> +        * clk_prepare_enable() call. Handle such cases by not doing
>> +        * anything during clk_prepare_enable() but handle clk_set_rate()
>> +        * correctly
>> +        */
>> +       if (!width)
>> +               return;
> 
> Why are the clk_ops assigned in a way that makes the code get here? Why
> can't we have different clk_ops, or not register the clks at all, when
> the hardware can't be written?


The hardware can actually be written for such clks but only for 
clk_set_rate() op for setting the clk rate. Just that hardware does not 
provide any way to enable/disable such clks.

Alternative way to handle such clks could be that the clk consumer does
not invoke clk_prepare_enable() before invoking clk_set_rate(). But we
want to avoid making changes in the clk consumer code to keep it 
standard. And handle it here by just validating the width parameter.

Thanks,
Rahul


> 
>> +
>> +       mask = (GENMASK(width - 1, 0) << shift);
>>          regmap_update_bits(membase, reg, mask, set_val << shift);
> 
> 

Re: [PATCH RESEND v2 4/5] clk: mxl: Add validation for register reads/writes

[Rahul Tanwar]

Hi Stephen,

On 30/9/2022 9:02 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-28 23:10:10)
>> On 29/9/2022 8:20 am, Stephen Boyd wrote:
>>>> +       u32 mask;
>>>>
>>>> +       /*
>>>> +        * Some clocks support parent clock dividers but they do not
>>>> +        * support clock gating (clk enable/disable). Such types of
>>>> +        * clocks might call this function with width as 0 during
>>>> +        * clk_prepare_enable() call. Handle such cases by not doing
>>>> +        * anything during clk_prepare_enable() but handle clk_set_rate()
>>>> +        * correctly
>>>> +        */
>>>> +       if (!width)
>>>> +               return;
>>>
>>> Why are the clk_ops assigned in a way that makes the code get here? Why
>>> can't we have different clk_ops, or not register the clks at all, when
>>> the hardware can't be written?
>>
>>
>> The hardware can actually be written for such clks but only for
>> clk_set_rate() op for setting the clk rate. Just that hardware does not
>> provide any way to enable/disable such clks.
>>
>> Alternative way to handle such clks could be that the clk consumer does
>> not invoke clk_prepare_enable() before invoking clk_set_rate(). But we
>> want to avoid making changes in the clk consumer code to keep it
>> standard. And handle it here by just validating the width parameter.
> 
> Why not have different clk_ops then that doesn't do anything for
> enable/disable and only does it for set_rate?
> 

There is only one clk entry which falls in this category. Adding a 
different clk_ops for just one clk would need many more lines of code 
addition which appears to be a overkill.

I have removed this change in v3 and used the driver internal flag to 
handle this particular clk. That requires minimal change and looks 
logical addition.

Thanks,
Rahul


> 

Re: [PATCH RESEND v2 4/5] clk: mxl: Add validation for register reads/writes

[Rahul Tanwar]

[Resend due to mail delivery failure in earlier reply - one email id got 
corrupted somehow in earlier reply]

Hi Stephen,


On 30/9/2022 9:02 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-28 23:10:10)
>> On 29/9/2022 8:20 am, Stephen Boyd wrote:
>>>> +       u32 mask;
>>>>
>>>> +       /*
>>>> +        * Some clocks support parent clock dividers but they do not
>>>> +        * support clock gating (clk enable/disable). Such types of
>>>> +        * clocks might call this function with width as 0 during
>>>> +        * clk_prepare_enable() call. Handle such cases by not doing
>>>> +        * anything during clk_prepare_enable() but handle clk_set_rate()
>>>> +        * correctly
>>>> +        */
>>>> +       if (!width)
>>>> +               return;
>>>
>>> Why are the clk_ops assigned in a way that makes the code get here? Why
>>> can't we have different clk_ops, or not register the clks at all, when
>>> the hardware can't be written?
>>
>>
>> The hardware can actually be written for such clks but only for
>> clk_set_rate() op for setting the clk rate. Just that hardware does not
>> provide any way to enable/disable such clks.
>>
>> Alternative way to handle such clks could be that the clk consumer does
>> not invoke clk_prepare_enable() before invoking clk_set_rate(). But we
>> want to avoid making changes in the clk consumer code to keep it
>> standard. And handle it here by just validating the width parameter.
> 
> Why not have different clk_ops then that doesn't do anything for
> enable/disable and only does it for set_rate?
> 


There is only one clk entry which falls in this category. Adding a
different clk_ops for just one clk would need many more lines of code
addition which appears to be a overkill.

I have removed this change in v3 and used the driver internal flag to
handle this particular clk. That requires minimal change and looks
logical addition.

Thanks,
Rahul


> 

[PATCH RESEND v2 5/5] clk: mxl: Add a missing flag to allow parent clock rate change

[Rahul Tanwar]

One of the clock entry "dcl" clk's rate can only be changed by
changing its parent's clock rate. But it was missing to have
CLK_SET_RATE_PARENT flag as enabled.

Add/enable CLK_SET_RATE_PARENT flag for dcl clk in order to
allow its clk rate to be changed via its parent's clk.

Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
---
 drivers/clk/x86/clk-lgm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/x86/clk-lgm.c b/drivers/clk/x86/clk-lgm.c
index e312af42e97a..34e16ea90596 100644
--- a/drivers/clk/x86/clk-lgm.c
+++ b/drivers/clk/x86/clk-lgm.c
@@ -255,7 +255,7 @@ static const struct lgm_clk_branch lgm_branch_clks[] = {
 	LGM_FIXED(LGM_CLK_SLIC, "slic", NULL, 0, CGU_IF_CLK1,
 		  8, 2, CLOCK_FLAG_VAL_INIT, 8192000, 2),
 	LGM_FIXED(LGM_CLK_DOCSIS, "v_docsis", NULL, 0, 0, 0, 0, 0, 16000000, 0),
-	LGM_DIV(LGM_CLK_DCL, "dcl", "v_ifclk", 0, CGU_PCMCR,
+	LGM_DIV(LGM_CLK_DCL, "dcl", "v_ifclk", CLK_SET_RATE_PARENT, CGU_PCMCR,
 		25, 3, 0, 0, 0, 0, dcl_div),
 	LGM_MUX(LGM_CLK_PCM, "pcm", pcm_p, 0, CGU_C55_PCMCR,
 		0, 1, CLK_MUX_ROUND_CLOSEST, 0),
-- 
2.17.1

Re: [PATCH RESEND v2 5/5] clk: mxl: Add a missing flag to allow parent clock rate change

[Stephen Boyd]

Quoting Rahul Tanwar (2022-09-21 23:24:28)
> One of the clock entry "dcl" clk's rate can only be changed by
> changing its parent's clock rate. But it was missing to have
> CLK_SET_RATE_PARENT flag as enabled.
> 
> Add/enable CLK_SET_RATE_PARENT flag for dcl clk in order to
> allow its clk rate to be changed via its parent's clk.
> 
> Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
> ---

Any Fixes tag?

Re: [PATCH RESEND v2 5/5] clk: mxl: Add a missing flag to allow parent clock rate change

[Rahul Tanwar]

On 29/9/2022 8:18 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> Quoting Rahul Tanwar (2022-09-21 23:24:28)
>> One of the clock entry "dcl" clk's rate can only be changed by
>> changing its parent's clock rate. But it was missing to have
>> CLK_SET_RATE_PARENT flag as enabled.
>>
>> Add/enable CLK_SET_RATE_PARENT flag for dcl clk in order to
>> allow its clk rate to be changed via its parent's clk.
>>
>> Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
>> ---
> 
> Any Fixes tag?
> 


Missed it, will add in v3.

Thanks,
Rahul


>