[PATCH v4 0/4] drivers: clk: renesas: ignore all clocks which are assinged to non-Linux system

[PATCH v4 0/4] drivers: clk: renesas: ignore all clocks which are assinged to non-Linux system

[Kuninori Morimoto]

Hi Rob, Geert
Cc Aymeric, Goda-san

This is v4 of ignoring non Linux system assinged device.

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

v3 -> v4
	- add Reviewed-by from Geert
	- Tidyup many English
	- use of_for_each_phandle() instead of while(!of_parse_phandle_with_args())
	- move cpg_mssr_reserved_init() into cpg_mssr_common_init()

v2 -> v3
	- "__of_get_next_status_child()" -> "of_get_next_status_child()"
	- add Reviewed-by from Rob

v1 -> v2
	- remove "default_ret" from __of_device_is_status()
	- add new parameter explanation on cpg_mssr_priv


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: ignore all clocks which are assinged to non-Linux system

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

-- 
2.25.1

[PATCH v4 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>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/base.c | 57 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 21 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8d93cb6ea9cd..3cb467a7e747 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)
 {
 	const char *status;
 	int statlen;
@@ -433,16 +426,45 @@ static bool __of_device_is_available(const struct device_node *device)
 
 	status = __of_get_property(device, "status", &statlen);
 	if (status == NULL)
-		return true;
+		return false;
 
 	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};
+
+	if (!device)
+		return false;
+
+	return !__of_get_property(device, "status", NULL) ||
+		__of_device_is_status(device, ok);
+}
+
 /**
  *  of_device_is_available - check if a device is available for use
  *
@@ -474,16 +496,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);
 }
 
 /**
-- 
2.25.1

[PATCH v4 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>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 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 3cb467a7e747..25c841a3882f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -612,16 +612,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;
@@ -632,7 +625,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;
@@ -641,6 +634,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 v4 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>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 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 25c841a3882f..b1ecfcfbb90f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -465,6 +465,20 @@ static bool __of_device_is_available(const struct device_node *device)
 		__of_device_is_status(device, ok);
 }
 
+/**
+ *  __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);
+}
+
 /**
  *  of_device_is_available - check if a device is available for use
  *
@@ -650,6 +664,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 v4 4/4] drivers: clk: renesas: ignore all clocks which are assinged to non-Linux system

[Kuninori Morimoto]

Some boards might use Linux and another OS at the same time. In such
case, currently, during booting, Linux will stop necessary module clocks
which are not used on the Linux side, but are used by another OS.

To avoid such situation, renesas-cpg-mssr tries to find
status = "reserved" devices (A), and adds 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 | 101 +++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index cb80d1bf6c7c..0db2aec00d10 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -142,6 +142,8 @@ static const u16 srstclr_for_gen4[] = {
  * @reset_clear_regs:  Pointer to reset clearing registers array
  * @smstpcr_saved: [].mask: Mask of SMSTPCR[] bits under our control
  *                 [].val: Saved values of SMSTPCR[]
+ * @reserved_ids: Temporary used, reserved id list
+ * @num_reserved_ids: Temporary used, number of reserved id list
  * @clks: Array containing all Core and Module Clocks
  */
 struct cpg_mssr_priv {
@@ -168,6 +170,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 +458,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 +967,72 @@ 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 *soc = of_find_node_by_path("/soc");
+	struct device_node *node;
+	uint32_t args[MAX_PHANDLE_ARGS];
+	unsigned int *ids = NULL;
+	unsigned int num = 0;
+
+	/*
+	 * Because clk_disable_unused() will disable all unused clocks, the device which is assigned
+	 * to a non-Linux system will be disabled when Linux is booted.
+	 *
+	 * To avoid such situation, renesas-cpg-mssr assumes the device which has
+	 * status = "reserved" is assigned to a non-Linux system, and adds CLK_IGNORE_UNUSED flag
+	 * to its CPG_MOD clocks.
+	 * 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(soc, node) {
+		struct of_phandle_iterator it;
+		int rc;
+
+		of_for_each_phandle(&it, rc, node, "clocks", "#clock-cells", -1) {
+			int idx;
+
+			of_phandle_iterator_args(&it, args, MAX_PHANDLE_ARGS);
+
+			if (!(it.node == priv->np && args[0] == CPG_MOD))
+				continue;
+
+			ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
+			if (!ids)
+				return -ENOMEM;
+
+			if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A)
+				idx = MOD_CLK_PACK_10(args[1]);	/* for DEF_MOD_STB() */
+			else
+				idx = MOD_CLK_PACK(args[1]);	/* for DEF_MOD() */
+
+			ids[num] = info->num_total_core_clks + idx;
+
+			num++;
+		}
+	}
+
+	priv->num_reserved_ids	= num;
+	priv->reserved_ids	= ids;
+
+	return 0;
+}
+
 static int __init cpg_mssr_common_init(struct device *dev,
 				       struct device_node *np,
 				       const struct cpg_mssr_info *info)
@@ -1007,6 +1091,10 @@ static int __init cpg_mssr_common_init(struct device *dev,
 	if (error)
 		goto out_err;
 
+	error = cpg_mssr_reserved_init(priv, info);
+	if (error)
+		goto out_err;
+
 	cpg_mssr_priv = priv;
 
 	return 0;
@@ -1070,22 +1158,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 v4 4/4] drivers: clk: renesas: ignore all clocks which are assinged to non-Linux system

[Geert Uytterhoeven]

Hi Morimoto-san,

Thanks for the update!

s/assinged/assigned/

On Mon, Dec 11, 2023 at 4:03 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Some boards might use Linux and another OS at the same time. In such
> case, currently, during booting, Linux will stop necessary module clocks
> which are not used on the Linux side, but are used by another OS.
>
> To avoid such situation, renesas-cpg-mssr tries to find
> status = "reserved" devices (A), and adds 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>

> @@ -949,6 +967,72 @@ 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);
> +}

This function is called only once, so you might want to inline it manually.

> +
> +static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
> +                                        const struct cpg_mssr_info *info)
> +{
> +       struct device_node *soc = of_find_node_by_path("/soc");
> +       struct device_node *node;
> +       uint32_t args[MAX_PHANDLE_ARGS];
> +       unsigned int *ids = NULL;
> +       unsigned int num = 0;
> +
> +       /*
> +        * Because clk_disable_unused() will disable all unused clocks, the device which is assigned
> +        * to a non-Linux system will be disabled when Linux is booted.
> +        *
> +        * To avoid such situation, renesas-cpg-mssr assumes the device which has
> +        * status = "reserved" is assigned to a non-Linux system, and adds CLK_IGNORE_UNUSED flag
> +        * to its CPG_MOD clocks.
> +        * 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(soc, node) {
> +               struct of_phandle_iterator it;
> +               int rc;
> +
> +               of_for_each_phandle(&it, rc, node, "clocks", "#clock-cells", -1) {
> +                       int idx;
> +
> +                       of_phandle_iterator_args(&it, args, MAX_PHANDLE_ARGS);
> +
> +                       if (!(it.node == priv->np && args[0] == CPG_MOD))

I think "(it.node != priv->np || args[0] != CPG_MOD)" is easier to read ;-)

However, I think it would make sense to split this in two separate
checks, to avoid calling of_phandle_iterator_args() when it.node !=
priv->np, and to validate the number of arguments:

    if (it.node != priv->np)
            continue;

    if (of_phandle_iterator_args(&it, args, MAX_PHANDLE_ARGS) != 2)
            continue;

    if (args[0] != CPG_MOD)
            continue;

> +                               continue;
> +
> +                       ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
> +                       if (!ids)
> +                               return -ENOMEM;

Missing of_node_put(it.node) in the error path.

> +
> +                       if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A)
> +                               idx = MOD_CLK_PACK_10(args[1]); /* for DEF_MOD_STB() */
> +                       else
> +                               idx = MOD_CLK_PACK(args[1]);    /* for DEF_MOD() */
> +
> +                       ids[num] = info->num_total_core_clks + idx;
> +
> +                       num++;
> +               }
> +       }
> +
> +       priv->num_reserved_ids  = num;
> +       priv->reserved_ids      = ids;
> +
> +       return 0;
> +}
> +
>  static int __init cpg_mssr_common_init(struct device *dev,
>                                        struct device_node *np,
>                                        const struct cpg_mssr_info *info)
> @@ -1007,6 +1091,10 @@ static int __init cpg_mssr_common_init(struct device *dev,
>         if (error)
>                 goto out_err;
>
> +       error = cpg_mssr_reserved_init(priv, info);
> +       if (error)
> +               goto out_err;

Missing of_clk_del_provider() in the error path.

You may want to move the call to cpg_mssr_reserved_init() up, as
reverting that just needs an unconditional call to kfree() (kfree
works fine on NULL), while calling of_clk_del_provider() requires a
new label to jump to.

> +
>         cpg_mssr_priv = priv;
>
>         return 0;
> @@ -1070,22 +1158,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:

Perhaps rename the label to "reserve_exit", as this is called on
success, too?

> +       cpg_mssr_reserved_exit(priv);
> +
> +       return error;
>  }
>
>  static struct platform_driver cpg_mssr_driver = {

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 v4 4/4] drivers: clk: renesas: ignore all clocks which are assinged to non-Linux system

[Kuninori Morimoto]

Hi Geert

Thank you for your reveiw

> > +static void __init cpg_mssr_reserved_exit(struct cpg_mssr_priv *priv)
> > +{
> > +       kfree(priv->reserved_ids);
> > +}
> 
> This function is called only once, so you might want to inline it manually.

I want to keep init()/exit() pair because it is easy to understand.

I will consider other comment and post v5 patch next week


Thank you for your help !!

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