[PATCH 0/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[PATCH 0/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Kuninori Morimoto]

Hi Rob, Geert
Cc Aymeric, Goda-san

Some board might use Linux and another OS in the same time. In such
case, current Linux will stop necessary module clock when booting
which is not used on Linux side, but is used on another OS side.

To avoid such situation, this patch-set try to find status = "reserved"
devices, and add CLK_IGNORE_UNUSED flag to its clock.

Table 2.4: Values for status property
https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf

"reserved"
	Indicates that the device is operational, but should not be
	used. Typically this is used for devices that are controlled
	by another software component, such as platform firmware.


[1/4] - [3/4] : expand existing function for "reserved"
[4/4]         : adjust to "reserved" device on Renesas CPG

Kuninori Morimoto (4):
  of: add __of_device_is_status() and makes more generic status check
  of: add __of_get_next_status_child() and makes more generic of_get_next
  of: add for_each_reserved_child_of_node()
  drivers: clk: renesas: enable all clocks which is assinged to non Linux system

 drivers/clk/renesas/renesas-cpg-mssr.c | 116 +++++++++++++++++++++++--
 drivers/of/base.c                      | 111 ++++++++++++++++-------
 include/linux/of.h                     |  11 +++
 3 files changed, 197 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH 1/4] of: add __of_device_is_status() and makes more generic status check

[Kuninori Morimoto]

Linux Kernel has __of_device_is_available() / __of_device_is_fail(),
these are checking if the status was "okay" / "ok" / "fail" / "fail-".

Add more generic __of_device_is_status() function for these.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
---
 drivers/of/base.c | 53 ++++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8d93cb6ea9cd..d67cb650dcd6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -415,15 +415,8 @@ int of_machine_is_compatible(const char *compat)
 }
 EXPORT_SYMBOL(of_machine_is_compatible);
 
-/**
- *  __of_device_is_available - check if a device is available for use
- *
- *  @device: Node to check for availability, with locks already held
- *
- *  Return: True if the status property is absent or set to "okay" or "ok",
- *  false otherwise
- */
-static bool __of_device_is_available(const struct device_node *device)
+static bool __of_device_is_status(const struct device_node *device,
+				  const char * const*strings, bool default_ret)
 {
 	const char *status;
 	int statlen;
@@ -433,16 +426,41 @@ static bool __of_device_is_available(const struct device_node *device)
 
 	status = __of_get_property(device, "status", &statlen);
 	if (status == NULL)
-		return true;
+		return default_ret;
 
 	if (statlen > 0) {
-		if (!strcmp(status, "okay") || !strcmp(status, "ok"))
-			return true;
+		while (*strings) {
+			unsigned int len = strlen(*strings);
+
+			if ((*strings)[len - 1] == '-') {
+				if (!strncmp(status, *strings, len))
+					return true;
+			} else {
+				if (!strcmp(status, *strings))
+					return true;
+			}
+			strings++;
+		}
 	}
 
 	return false;
 }
 
+/**
+ *  __of_device_is_available - check if a device is available for use
+ *
+ *  @device: Node to check for availability, with locks already held
+ *
+ *  Return: True if the status property is absent or set to "okay" or "ok",
+ *  false otherwise
+ */
+static bool __of_device_is_available(const struct device_node *device)
+{
+	static const char * const ok[] = {"okay", "ok", NULL};
+
+	return __of_device_is_status(device, ok, true);
+}
+
 /**
  *  of_device_is_available - check if a device is available for use
  *
@@ -474,16 +492,9 @@ EXPORT_SYMBOL(of_device_is_available);
  */
 static bool __of_device_is_fail(const struct device_node *device)
 {
-	const char *status;
-
-	if (!device)
-		return false;
-
-	status = __of_get_property(device, "status", NULL);
-	if (status == NULL)
-		return false;
+	static const char * const fail[] = {"fail", "fail-", NULL};
 
-	return !strcmp(status, "fail") || !strncmp(status, "fail-", 5);
+	return __of_device_is_status(device, fail, false);
 }
 
 /**
-- 
2.25.1

Re: [PATCH 1/4] of: add __of_device_is_status() and makes more generic status check

[Rob Herring]

On Tue, Nov 14, 2023 at 12:00:49AM +0000, Kuninori Morimoto wrote:
> Linux Kernel has __of_device_is_available() / __of_device_is_fail(),
> these are checking if the status was "okay" / "ok" / "fail" / "fail-".
> 
> Add more generic __of_device_is_status() function for these.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Tested-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
> ---
>  drivers/of/base.c | 53 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8d93cb6ea9cd..d67cb650dcd6 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -415,15 +415,8 @@ int of_machine_is_compatible(const char *compat)
>  }
>  EXPORT_SYMBOL(of_machine_is_compatible);
>  
> -/**
> - *  __of_device_is_available - check if a device is available for use
> - *
> - *  @device: Node to check for availability, with locks already held
> - *
> - *  Return: True if the status property is absent or set to "okay" or "ok",
> - *  false otherwise
> - */
> -static bool __of_device_is_available(const struct device_node *device)
> +static bool __of_device_is_status(const struct device_node *device,
> +				  const char * const*strings, bool default_ret)
>  {
>  	const char *status;
>  	int statlen;
> @@ -433,16 +426,41 @@ static bool __of_device_is_available(const struct device_node *device)
>  
>  	status = __of_get_property(device, "status", &statlen);
>  	if (status == NULL)
> -		return true;
> +		return default_ret;
>  
>  	if (statlen > 0) {
> -		if (!strcmp(status, "okay") || !strcmp(status, "ok"))
> -			return true;
> +		while (*strings) {
> +			unsigned int len = strlen(*strings);
> +
> +			if ((*strings)[len - 1] == '-') {
> +				if (!strncmp(status, *strings, len))
> +					return true;
> +			} else {
> +				if (!strcmp(status, *strings))
> +					return true;
> +			}
> +			strings++;
> +		}
>  	}
>  
>  	return false;
>  }
>  
> +/**
> + *  __of_device_is_available - check if a device is available for use
> + *
> + *  @device: Node to check for availability, with locks already held
> + *
> + *  Return: True if the status property is absent or set to "okay" or "ok",
> + *  false otherwise
> + */
> +static bool __of_device_is_available(const struct device_node *device)
> +{
> +	static const char * const ok[] = {"okay", "ok", NULL};
> +
> +	return __of_device_is_status(device, ok, true);

Available is special compared to any other status check. Rather than 
passing a value to return, I would make this:

return __of_device_is_status(device, ok) || !__of_get_property(device, "status", NULL);

Rob

[PATCH 2/4] of: add __of_get_next_status_child() and makes more generic of_get_next

[Kuninori Morimoto]

Linux Kernel has of_get_next_available_child().
Add more generic __of_get_next_status_child() to enable to use same
logic for other status.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
---
 drivers/of/base.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d67cb650dcd6..f0b63e195f66 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -608,16 +608,9 @@ struct device_node *of_get_next_child(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_get_next_child);
 
-/**
- * of_get_next_available_child - Find the next available child node
- * @node:	parent node
- * @prev:	previous child of the parent node, or NULL to get first
- *
- * This function is like of_get_next_child(), except that it
- * automatically skips any disabled nodes (i.e. status = "disabled").
- */
-struct device_node *of_get_next_available_child(const struct device_node *node,
-	struct device_node *prev)
+static struct device_node *__of_get_next_status_child(const struct device_node *node,
+						      struct device_node *prev,
+						      bool (*checker)(const struct device_node *))
 {
 	struct device_node *next;
 	unsigned long flags;
@@ -628,7 +621,7 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	next = prev ? prev->sibling : node->child;
 	for (; next; next = next->sibling) {
-		if (!__of_device_is_available(next))
+		if (!checker(next))
 			continue;
 		if (of_node_get(next))
 			break;
@@ -637,6 +630,20 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return next;
 }
+
+/**
+ * of_get_next_available_child - Find the next available child node
+ * @node:	parent node
+ * @prev:	previous child of the parent node, or NULL to get first
+ *
+ * This function is like of_get_next_child(), except that it
+ * automatically skips any disabled nodes (i.e. status = "disabled").
+ */
+struct device_node *of_get_next_available_child(const struct device_node *node,
+	struct device_node *prev)
+{
+	return __of_get_next_status_child(node, prev, __of_device_is_available);
+}
 EXPORT_SYMBOL(of_get_next_available_child);
 
 /**
-- 
2.25.1

[PATCH 3/4] of: add for_each_reserved_child_of_node()

[Kuninori Morimoto]

We would like to use for_each loop for status = "reserved" nodes.
Add for_each_reserved_child_of_node() for it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
---
 drivers/of/base.c  | 29 +++++++++++++++++++++++++++++
 include/linux/of.h | 11 +++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f0b63e195f66..341bc45bef05 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -461,6 +461,20 @@ static bool __of_device_is_available(const struct device_node *device)
 	return __of_device_is_status(device, ok, true);
 }
 
+/**
+ *  __of_device_is_reserved - check if a device is reserved
+ *
+ *  @device: Node to check for availability, with locks already held
+ *
+ *  Return: True if the status property is set to "reserved", false otherwise
+ */
+static bool __of_device_is_reserved(const struct device_node *device)
+{
+	static const char * const reserved[] = {"reserved", NULL};
+
+	return __of_device_is_status(device, reserved, false);
+}
+
 /**
  *  of_device_is_available - check if a device is available for use
  *
@@ -646,6 +660,21 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_get_next_available_child);
 
+/**
+ * of_get_next_reserved_child - Find the next reserved child node
+ * @node:	parent node
+ * @prev:	previous child of the parent node, or NULL to get first
+ *
+ * This function is like of_get_next_child(), except that it
+ * automatically skips any disabled nodes (i.e. status = "disabled").
+ */
+struct device_node *of_get_next_reserved_child(const struct device_node *node,
+						struct device_node *prev)
+{
+	return __of_get_next_status_child(node, prev, __of_device_is_reserved);
+}
+EXPORT_SYMBOL(of_get_next_reserved_child);
+
 /**
  * of_get_next_cpu_node - Iterate on cpu nodes
  * @prev:	previous child of the /cpus node, or NULL to get first
diff --git a/include/linux/of.h b/include/linux/of.h
index 6a9ddf20e79a..331e05918f11 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -294,6 +294,8 @@ extern struct device_node *of_get_next_child(const struct device_node *node,
 					     struct device_node *prev);
 extern struct device_node *of_get_next_available_child(
 	const struct device_node *node, struct device_node *prev);
+extern struct device_node *of_get_next_reserved_child(
+	const struct device_node *node, struct device_node *prev);
 
 extern struct device_node *of_get_compatible_child(const struct device_node *parent,
 					const char *compatible);
@@ -541,6 +543,12 @@ static inline struct device_node *of_get_next_available_child(
 	return NULL;
 }
 
+static inline struct device_node *of_get_next_reserved_child(
+	const struct device_node *node, struct device_node *prev)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_find_node_with_property(
 	struct device_node *from, const char *prop_name)
 {
@@ -1431,6 +1439,9 @@ static inline int of_property_read_s32(const struct device_node *np,
 #define for_each_available_child_of_node(parent, child) \
 	for (child = of_get_next_available_child(parent, NULL); child != NULL; \
 	     child = of_get_next_available_child(parent, child))
+#define for_each_reserved_child_of_node(parent, child)			\
+	for (child = of_get_next_reserved_child(parent, NULL); child != NULL; \
+	     child = of_get_next_reserved_child(parent, child))
 
 #define for_each_of_cpu_node(cpu) \
 	for (cpu = of_get_next_cpu_node(NULL); cpu != NULL; \
-- 
2.25.1

[PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Kuninori Morimoto]

Some board might use Linux and another OS in the same time. In such
case, current Linux will stop necessary module clock when booting
which is not used on Linux side, but is used on another OS side.

To avoid such situation, renesas-cpg-mssr try to find
status = "reserved" devices (A), and add CLK_IGNORE_UNUSED flag to its
<&cgp CPG_MOD xxx> clock (B).

Table 2.4: Values for status property
https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf

"reserved"
	Indicates that the device is operational, but should not be
	used. Typically this is used for devices that are controlled
	by another software component, such as platform firmware.

ex)
	scif5: serial@e6f30000 {
		...
(B)		clocks = <&cpg CPG_MOD 202>,
			 <&cpg CPG_CORE R8A7795_CLK_S3D1>,
			 <&scif_clk>;
		...
(A)		status = "reserved";
	};

Cc: Aymeric Aillet <aymeric.aillet@iot.bzh>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
---
 drivers/clk/renesas/renesas-cpg-mssr.c | 116 +++++++++++++++++++++++--
 1 file changed, 107 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index cb80d1bf6c7c..3781861bdfa0 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -168,6 +168,9 @@ struct cpg_mssr_priv {
 		u32 val;
 	} smstpcr_saved[ARRAY_SIZE(mstpsr_for_gen4)];
 
+	unsigned int *reserved_ids;
+	unsigned int num_reserved_ids;
+
 	struct clk *clks[];
 };
 
@@ -453,6 +456,19 @@ static void __init cpg_mssr_register_mod_clk(const struct mssr_mod_clk *mod,
 			break;
 		}
 
+	/*
+	 * Ignore reserved device.
+	 * see
+	 *	cpg_mssr_reserved_init()
+	 */
+	for (i = 0; i < priv->num_reserved_ids; i++) {
+		if (id == priv->reserved_ids[i]) {
+			dev_info(dev, "Ignore Linux non-assigned mod (%s)\n", mod->name);
+			init.flags |= CLK_IGNORE_UNUSED;
+			break;
+		}
+	}
+
 	clk = clk_register(NULL, &clock->hw);
 	if (IS_ERR(clk))
 		goto fail;
@@ -949,6 +965,75 @@ static const struct dev_pm_ops cpg_mssr_pm = {
 #define DEV_PM_OPS	NULL
 #endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
 
+static void __init cpg_mssr_reserved_exit(struct cpg_mssr_priv *priv)
+{
+	kfree(priv->reserved_ids);
+}
+
+static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
+					 const struct cpg_mssr_info *info)
+{
+	struct device_node *root = of_find_node_by_path("/soc");
+	struct device_node *node = NULL;
+	struct of_phandle_args clkspec;
+	unsigned int *ids = NULL;
+	unsigned int num = 0;
+
+	/*
+	 * Because cpg_mssr_info has .num_hw_mod_clks which indicates number of all Module Clocks,
+	 * and clk_disable_unused() will disable all unused clocks, the device which is assigned to
+	 * non-Linux system will be disabled when Linux was booted.
+	 *
+	 * To avoid such situation, renesas-cpg-mssr assumes the device which has
+	 * status = "reserved" is assigned to non-Linux system, and add CLK_IGNORE_UNUSED flag
+	 * to its clocks if it was CPG_MOD.
+	 * see also
+	 *	cpg_mssr_register_mod_clk()
+	 *
+	 *	scif5: serial@e6f30000 {
+	 *		...
+	 * =>		clocks = <&cpg CPG_MOD 202>,
+	 *			 <&cpg CPG_CORE R8A7795_CLK_S3D1>,
+	 *			 <&scif_clk>;
+	 *			 ...
+	 *		 status = "reserved";
+	 *	};
+	 */
+	for_each_reserved_child_of_node(root, node) {
+		unsigned int i = 0;
+
+		while (!of_parse_phandle_with_args(node, "clocks", "#clock-cells", i++, &clkspec)) {
+
+			of_node_put(clkspec.np);
+
+			if (clkspec.np == priv->dev->of_node &&
+			    clkspec.args[0] == CPG_MOD) {
+
+				ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
+				if (!ids)
+					return -ENOMEM;
+
+				ids[num] = info->num_total_core_clks +
+						MOD_CLK_PACK(clkspec.args[1]);
+
+				num++;
+			}
+		}
+	}
+
+	priv->num_reserved_ids	= num;
+	priv->reserved_ids	= ids;
+
+	return 0;
+}
+
+static void __init cpg_mssr_common_exit(struct cpg_mssr_priv *priv)
+{
+	if (priv->base)
+		iounmap(priv->base);
+	kfree(priv);
+}
+
 static int __init cpg_mssr_common_init(struct device *dev,
 				       struct device_node *np,
 				       const struct cpg_mssr_info *info)
@@ -1012,9 +1097,7 @@ static int __init cpg_mssr_common_init(struct device *dev,
 	return 0;
 
 out_err:
-	if (priv->base)
-		iounmap(priv->base);
-	kfree(priv);
+	cpg_mssr_common_exit(priv);
 
 	return error;
 }
@@ -1029,6 +1112,10 @@ void __init cpg_mssr_early_init(struct device_node *np,
 	if (error)
 		return;
 
+	error = cpg_mssr_reserved_init(cpg_mssr_priv, info);
+	if (error)
+		goto err;
+
 	for (i = 0; i < info->num_early_core_clks; i++)
 		cpg_mssr_register_core_clk(&info->early_core_clks[i], info,
 					   cpg_mssr_priv);
@@ -1037,6 +1124,12 @@ void __init cpg_mssr_early_init(struct device_node *np,
 		cpg_mssr_register_mod_clk(&info->early_mod_clks[i], info,
 					  cpg_mssr_priv);
 
+	cpg_mssr_reserved_exit(cpg_mssr_priv);
+
+	return;
+
+err:
+	cpg_mssr_common_exit(cpg_mssr_priv);
 }
 
 static int __init cpg_mssr_probe(struct platform_device *pdev)
@@ -1060,6 +1153,10 @@ static int __init cpg_mssr_probe(struct platform_device *pdev)
 	priv->dev = dev;
 	dev_set_drvdata(dev, priv);
 
+	error = cpg_mssr_reserved_init(priv, info);
+	if (error)
+		return error;
+
 	for (i = 0; i < info->num_core_clks; i++)
 		cpg_mssr_register_core_clk(&info->core_clks[i], info, priv);
 
@@ -1070,22 +1167,23 @@ static int __init cpg_mssr_probe(struct platform_device *pdev)
 					 cpg_mssr_del_clk_provider,
 					 np);
 	if (error)
-		return error;
+		goto reserve_err;
 
 	error = cpg_mssr_add_clk_domain(dev, info->core_pm_clks,
 					info->num_core_pm_clks);
 	if (error)
-		return error;
+		goto reserve_err;
 
 	/* Reset Controller not supported for Standby Control SoCs */
 	if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A)
-		return 0;
+		goto reserve_err;
 
 	error = cpg_mssr_reset_controller_register(priv);
-	if (error)
-		return error;
 
-	return 0;
+reserve_err:
+	cpg_mssr_reserved_exit(priv);
+
+	return error;
 }
 
 static struct platform_driver cpg_mssr_driver = {
-- 
2.25.1

Re: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[kernel test robot]

Hi Kuninori,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on geert-renesas-drivers/renesas-clk clk/clk-next linus/master v6.7-rc1 next-20231114]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuninori-Morimoto/of-add-__of_device_is_status-and-makes-more-generic-status-check/20231114-081044
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/87wmulrynq.wl-kuninori.morimoto.gx%40renesas.com
patch subject: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20231114/202311142059.XrPUseGq-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231114/202311142059.XrPUseGq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311142059.XrPUseGq-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/clk/renesas/renesas-cpg-mssr.c:175: warning: Function parameter or member 'reserved_ids' not described in 'cpg_mssr_priv'
>> drivers/clk/renesas/renesas-cpg-mssr.c:175: warning: Function parameter or member 'num_reserved_ids' not described in 'cpg_mssr_priv'


vim +175 drivers/clk/renesas/renesas-cpg-mssr.c

17bcc8035d2d19 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  124  
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  125  /**
b5fb3b8859a491 drivers/clk/renesas/renesas-cpg-mssr.c  Krzysztof Kozlowski 2020-11-03  126   * struct cpg_mssr_priv - Clock Pulse Generator / Module Standby
b5fb3b8859a491 drivers/clk/renesas/renesas-cpg-mssr.c  Krzysztof Kozlowski 2020-11-03  127   *                        and Software Reset Private Data
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  128   *
6197aa65c49055 drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-01-20  129   * @rcdev: Optional reset controller entity
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  130   * @dev: CPG/MSSR device
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  131   * @base: CPG/MSSR register block base address
ffbf9cf3f9460e drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  132   * @reg_layout: CPG/MSSR register layout
a4ea6a0f83073f drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-01-20  133   * @rmw_lock: protects RMW register accesses
d2e4cb45af8fac drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2019-06-12  134   * @np: Device node in DT for this CPG/MSSR module
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  135   * @num_core_clks: Number of Core Clocks in clks[]
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  136   * @num_mod_clks: Number of Module Clocks in clks[]
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  137   * @last_dt_core_clk: ID of the last Core Clock exported to DT
1f4023cdd1bdbe drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-06-21  138   * @notifiers: Notifier chain to save/restore clock state for system resume
8b652aa8a1fb02 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  139   * @status_regs: Pointer to status registers array
8b652aa8a1fb02 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  140   * @control_regs: Pointer to control registers array
8b652aa8a1fb02 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  141   * @reset_regs: Pointer to reset registers array
8b652aa8a1fb02 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  142   * @reset_clear_regs:  Pointer to reset clearing registers array
24ece96554a963 drivers/clk/renesas/renesas-cpg-mssr.c  Lee Jones           2021-01-26  143   * @smstpcr_saved: [].mask: Mask of SMSTPCR[] bits under our control
24ece96554a963 drivers/clk/renesas/renesas-cpg-mssr.c  Lee Jones           2021-01-26  144   *                 [].val: Saved values of SMSTPCR[]
8f5e20b6b8848b drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2019-06-12  145   * @clks: Array containing all Core and Module Clocks
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  146   */
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  147  struct cpg_mssr_priv {
6197aa65c49055 drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-01-20  148  #ifdef CONFIG_RESET_CONTROLLER
6197aa65c49055 drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-01-20  149  	struct reset_controller_dev rcdev;
6197aa65c49055 drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-01-20  150  #endif
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  151  	struct device *dev;
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  152  	void __iomem *base;
ffbf9cf3f9460e drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  153  	enum clk_reg_layout reg_layout;
a4ea6a0f83073f drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-01-20  154  	spinlock_t rmw_lock;
1f7db7bbf03182 drivers/clk/renesas/renesas-cpg-mssr.c  Chris Brandt        2018-09-24  155  	struct device_node *np;
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  156  
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  157  	unsigned int num_core_clks;
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  158  	unsigned int num_mod_clks;
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  159  	unsigned int last_dt_core_clk;
560869100b99a3 drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-06-07  160  
1f4023cdd1bdbe drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-06-21  161  	struct raw_notifier_head notifiers;
8b652aa8a1fb02 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  162  	const u16 *status_regs;
8b652aa8a1fb02 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  163  	const u16 *control_regs;
8b652aa8a1fb02 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  164  	const u16 *reset_regs;
8b652aa8a1fb02 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2020-09-11  165  	const u16 *reset_clear_regs;
560869100b99a3 drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-06-07  166  	struct {
560869100b99a3 drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-06-07  167  		u32 mask;
560869100b99a3 drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2017-06-07  168  		u32 val;
470e3f0d0b1529 drivers/clk/renesas/renesas-cpg-mssr.c  Yoshihiro Shimoda   2021-12-01  169  	} smstpcr_saved[ARRAY_SIZE(mstpsr_for_gen4)];
8f5e20b6b8848b drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2019-06-12  170  
b357c19f075f23 drivers/clk/renesas/renesas-cpg-mssr.c  Kuninori Morimoto   2023-11-14  171  	unsigned int *reserved_ids;
b357c19f075f23 drivers/clk/renesas/renesas-cpg-mssr.c  Kuninori Morimoto   2023-11-14  172  	unsigned int num_reserved_ids;
b357c19f075f23 drivers/clk/renesas/renesas-cpg-mssr.c  Kuninori Morimoto   2023-11-14  173  
8f5e20b6b8848b drivers/clk/renesas/renesas-cpg-mssr.c  Geert Uytterhoeven  2019-06-12  174  	struct clk *clks[];
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16 @175  };
f793d1e51705b2 drivers/clk/shmobile/renesas-cpg-mssr.c Geert Uytterhoeven  2015-10-16  176  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Kuninori Morimoto]

Hi kernel test

> kernel test robot noticed the following build warnings:
(snip)
> >> drivers/clk/renesas/renesas-cpg-mssr.c:175: warning: Function parameter or member 'reserved_ids' not described in 'cpg_mssr_priv'
> >> drivers/clk/renesas/renesas-cpg-mssr.c:175: warning: Function parameter or member 'num_reserved_ids' not described in 'cpg_mssr_priv'

Thank you for the report.
I will fixup it on v2.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Rob Herring]

On Tue, Nov 14, 2023 at 12:01:14AM +0000, Kuninori Morimoto wrote:
> Some board might use Linux and another OS in the same time. In such
> case, current Linux will stop necessary module clock when booting
> which is not used on Linux side, but is used on another OS side.
> 
> To avoid such situation, renesas-cpg-mssr try to find
> status = "reserved" devices (A), and add CLK_IGNORE_UNUSED flag to its
> <&cgp CPG_MOD xxx> clock (B).

See Stephen's presentation from Plumbers this week. The default behavior 
for unused clocks may be changing soon.

> 
> Table 2.4: Values for status property
> https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf
> 
> "reserved"
> 	Indicates that the device is operational, but should not be
> 	used. Typically this is used for devices that are controlled
> 	by another software component, such as platform firmware.
> 
> ex)
> 	scif5: serial@e6f30000 {
> 		...
> (B)		clocks = <&cpg CPG_MOD 202>,
> 			 <&cpg CPG_CORE R8A7795_CLK_S3D1>,
> 			 <&scif_clk>;
> 		...
> (A)		status = "reserved";
> 	};

I have some reservations about whether a reserved node should be touched 
at all by Linux. I suppose since it is platform specific, it's okay. I 
don't think we could apply such behavior globally.

Rob

Re: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Geert Uytterhoeven]

Hi Rob,

On Thu, Nov 16, 2023 at 8:23 PM Rob Herring <robh@kernel.org> wrote:
> On Tue, Nov 14, 2023 at 12:01:14AM +0000, Kuninori Morimoto wrote:
> > Some board might use Linux and another OS in the same time. In such
> > case, current Linux will stop necessary module clock when booting
> > which is not used on Linux side, but is used on another OS side.
> >
> > To avoid such situation, renesas-cpg-mssr try to find
> > status = "reserved" devices (A), and add CLK_IGNORE_UNUSED flag to its
> > <&cgp CPG_MOD xxx> clock (B).
>
> See Stephen's presentation from Plumbers this week. The default behavior
> for unused clocks may be changing soon.

Thank you!

ou mean "Make sync_state()/handoff work for the common clk
framework"[1]? IIUIC, that presentation didn't cover the problem we are
facing, except for the big "Kconfig for clk_ignore_unused=true" hammer.

> > Table 2.4: Values for status property
> > https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf
> >
> > "reserved"
> >       Indicates that the device is operational, but should not be
> >       used. Typically this is used for devices that are controlled
> >       by another software component, such as platform firmware.
> >
> > ex)
> >       scif5: serial@e6f30000 {
> >               ...
> > (B)           clocks = <&cpg CPG_MOD 202>,
> >                        <&cpg CPG_CORE R8A7795_CLK_S3D1>,
> >                        <&scif_clk>;
> >               ...
> > (A)           status = "reserved";
> >       };
>
> I have some reservations about whether a reserved node should be touched
> at all by Linux. I suppose since it is platform specific, it's okay. I
> don't think we could apply such behavior globally.

That's an interesting comment, as the issue is that currently Linux
does touch (resources belonging to) reserved nodes, and this patch
would prevent doing that for module clock resources;-)

The core issue is that Linux distinguishes only between two cases:
  1. "device is used by Linux" (if a driver is available),
     as indicated by 'status = "okay"' in DT, or
  2. "device is unused by Linux".
On a heterogenous system, the latter actually comprises two cases:
  2a. "device is unused", or
  2b. "device is used by another OS running on another CPU core".

Looking for 'status = "reserved"' allows us to distinguish between 2a
and 2b, and can prevent disabling clocks that are used by another OS.
Probably we need a similar solution for power domains.

Do you have a better or alternative suggestion?
Thanks!

[1] https://lpc.events/event/17/contributions/1432/
    https://www.youtube.com/watch?v=NSSSIVQgsIk?t=164m

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Stephen Boyd]

Quoting Geert Uytterhoeven (2023-11-16 13:08:46)
> On Thu, Nov 16, 2023 at 8:23 PM Rob Herring <robh@kernel.org> wrote:
> > On Tue, Nov 14, 2023 at 12:01:14AM +0000, Kuninori Morimoto wrote:
> > > Some board might use Linux and another OS in the same time. In such
> > > case, current Linux will stop necessary module clock when booting
> > > which is not used on Linux side, but is used on another OS side.
> > >
> > > To avoid such situation, renesas-cpg-mssr try to find
> > > status = "reserved" devices (A), and add CLK_IGNORE_UNUSED flag to its
> > > <&cgp CPG_MOD xxx> clock (B).
> >
> > See Stephen's presentation from Plumbers this week. The default behavior
> > for unused clocks may be changing soon.
> 
> Thank you!
> 
> ou mean "Make sync_state()/handoff work for the common clk
> framework"[1]? IIUIC, that presentation didn't cover the problem we are
> facing, except for the big "Kconfig for clk_ignore_unused=true" hammer.

:)

> 
> > > Table 2.4: Values for status property
> > > https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf
> > >
> > > "reserved"
> > >       Indicates that the device is operational, but should not be
> > >       used. Typically this is used for devices that are controlled
> > >       by another software component, such as platform firmware.
> > >
> > > ex)
> > >       scif5: serial@e6f30000 {
> > >               ...
> > > (B)           clocks = <&cpg CPG_MOD 202>,
> > >                        <&cpg CPG_CORE R8A7795_CLK_S3D1>,
> > >                        <&scif_clk>;
> > >               ...
> > > (A)           status = "reserved";
> > >       };
> >
> > I have some reservations about whether a reserved node should be touched
> > at all by Linux. I suppose since it is platform specific, it's okay. I
> > don't think we could apply such behavior globally.
> 
> That's an interesting comment, as the issue is that currently Linux
> does touch (resources belonging to) reserved nodes, and this patch
> would prevent doing that for module clock resources;-)

I think I get it.

> 
> The core issue is that Linux distinguishes only between two cases:
>   1. "device is used by Linux" (if a driver is available),
>      as indicated by 'status = "okay"' in DT, or
>   2. "device is unused by Linux".
> On a heterogenous system, the latter actually comprises two cases:
>   2a. "device is unused", or
>   2b. "device is used by another OS running on another CPU core".
> 
> Looking for 'status = "reserved"' allows us to distinguish between 2a
> and 2b, and can prevent disabling clocks that are used by another OS.
> Probably we need a similar solution for power domains.
> 
> Do you have a better or alternative suggestion?

Does the protected-clocks property work? That basically says "don't use
these clks in the OS". The driver implementation would not register
those clks and then the framework would be unaware of their existence,
leading to them never being disabled during late init.

This approach also looks OK to me, basically programmatically creating
the protected-clocks list by parsing DT for reserved consumer nodes and
then figuring out that no consumer exists so we can skip registering the
clk entirely, or add the flag. I'm not sure we want to implement that
policy globally, because maybe someone really wants to disable the clk
still to clean up bootloader state and then let a remoteproc use the clk
later.

Do you want to keep those clks registered with the framework? Is there
any benefit to keeping clks around if linux can't do anything with them?

Re: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Kuninori Morimoto]

Hi Stephen

Thank you for your feedback

> Does the protected-clocks property work? That basically says "don't use
> these clks in the OS". The driver implementation would not register
> those clks and then the framework would be unaware of their existence,
> leading to them never being disabled during late init.
> 
> This approach also looks OK to me, basically programmatically creating
> the protected-clocks list by parsing DT for reserved consumer nodes and
> then figuring out that no consumer exists so we can skip registering the
> clk entirely, or add the flag. I'm not sure we want to implement that
> policy globally, because maybe someone really wants to disable the clk
> still to clean up bootloader state and then let a remoteproc use the clk
> later.
> 
> Do you want to keep those clks registered with the framework? Is there
> any benefit to keeping clks around if linux can't do anything with them?

Actually, this idea (= using "protected-clocks" property style) was our 1st
idea, but I noticed that it is not enough. Because clock driver is possible
to know which module was not used on Linux, but other driver can't, in this
idea. For example, power-domain control driver might want to know about it.

In case of using "protected-clocks" property, we need to have same/similar
settings on each driver, but in case of using "status = reserved", all
driver is possible to know it. It has consistent, and no contradict like
some module was listed as "protected-clocks" on clock / power driver,
but has "status = ok" on its driver, etc.

It seems we have different opinions around here ?

Our other idea was having "unassigned" node instead of
"status = reserved", like below.
clock driver checks "unassigned" node's "devices" property, and
ignore/disable listed device's "clocks".

	unassigned {
		devices = <&scif1>, <&hsusb>;
	};

	scif1: serial@xxxx {
		status = "disabled";
		clocks = <&cpg CPG_MOD 206>,
			 <&cpg CPG_CORE R8A7795_CLK_S3D1>,
			 <&scif_clk>;
		...
	};

	hsusb: usb@xxxx {
		status = "disabled";
		clocks = <&cpg CPG_MOD 704>, <&cpg CPG_MOD 703>;
		...
	};



Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Stephen Boyd]

Quoting Kuninori Morimoto (2023-11-27 23:48:04)
> 
> Hi Stephen
> 
> Thank you for your feedback
> 
> > Does the protected-clocks property work? That basically says "don't use
> > these clks in the OS". The driver implementation would not register
> > those clks and then the framework would be unaware of their existence,
> > leading to them never being disabled during late init.
> > 
> > This approach also looks OK to me, basically programmatically creating
> > the protected-clocks list by parsing DT for reserved consumer nodes and
> > then figuring out that no consumer exists so we can skip registering the
> > clk entirely, or add the flag. I'm not sure we want to implement that
> > policy globally, because maybe someone really wants to disable the clk
> > still to clean up bootloader state and then let a remoteproc use the clk
> > later.
> > 
> > Do you want to keep those clks registered with the framework? Is there
> > any benefit to keeping clks around if linux can't do anything with them?
> 
> Actually, this idea (= using "protected-clocks" property style) was our 1st
> idea, but I noticed that it is not enough. Because clock driver is possible
> to know which module was not used on Linux, but other driver can't, in this
> idea. For example, power-domain control driver might want to know about it.
> 
> In case of using "protected-clocks" property, we need to have same/similar
> settings on each driver, but in case of using "status = reserved", all
> driver is possible to know it. It has consistent, and no contradict like
> some module was listed as "protected-clocks" on clock / power driver,
> but has "status = ok" on its driver, etc.
> 
> It seems we have different opinions around here ?

I don't really have any opinion here.

> 
> Our other idea was having "unassigned" node instead of
> "status = reserved", like below.
> clock driver checks "unassigned" node's "devices" property, and
> ignore/disable listed device's "clocks".
> 
>         unassigned {
>                 devices = <&scif1>, <&hsusb>;
>         };

This approach looks like the chosen node. I'd say what you have done in
this patch series is better. The protected-clocks property is really
about clks that shouldn't be used by the OS on some board. In your case
it sounds like you want to still be able to read the clk hardware? Does
anything go wrong if you let some consumer get the clk and change
settings? Do you want to prevent that from happening? I'm mostly
thinking it may be useful to have this logic be common in the clk
framework by having the framework search through DT and figure out that
the only consumers are reserved and thus we should treat those clks as
read-only in the framework.

Re: [PATCH 4/4] drivers: clk: renesas: enable all clocks which is assinged to non Linux system

[Kuninori Morimoto]

Hi Stephen

Thank you for your feedback

> In your case
> it sounds like you want to still be able to read the clk hardware? Does
> anything go wrong if you let some consumer get the clk and change
> settings? Do you want to prevent that from happening?

We want to do by this patch-set is to ignoring specific device/clk from Linux.
If Linux side change the specific clk settings, other OS side will get damage.

> I'm mostly
> thinking it may be useful to have this logic be common in the clk
> framework by having the framework search through DT and figure out that
> the only consumers are reserved and thus we should treat those clks as
> read-only in the framework.

"this logic" = "this patch-set idea" = "check status=reserved" ?

If so, now I quick checked the code. I think it is difficult or makes the code
complex if we try implement it on framework or common code.
Because the idea itself is very easy (= just adding the CLK_IGNORE_UNUSED),
but how to judge the clks is very vender/driver specific.
In our case, we need to think about total clock number, conver Linux
number to HW number, etc, etc.


# I will goto OSS-Japan conference after tomorrow, thus no response
# from me until next week.

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto