clk: sunxi: error checking on clock setup

PATCH New Andre Przywara 2 participants 5 messages Feb 16, 2016 – Feb 22, 2016

Subsystems (3)

Common clk Arm/Allwinner sunXi SoC support Arm/Allwinner SoC Clock Support

lore

Share & tools

lore

Andre Przywara

author

2016-02-16 10:46 UTC #0 lore raw

v2 cover clk: sunxi: error checking on clock setup

Setting up the clocks properly is quite critical to the system's operation, but currently our error handling is not very verbose. This series adds error handling and reporting to the sunxi clocks, so that any errors are correctly detected and reported. Also previous actions are rolled back in case something went wrong.
This proves to be helpful in debugging clock tree issues, especially when adding support for new SoCs.
I tested this on a BananaPi by deliberately misspelling "clock-output-names". The resulting kernel crash in sun4i_timer_interrupt is totally misleading, but now there is a line in the dmesg before saying:
=======
sunxi_divider_clk_setup: could not read clock-output-names for "apb0"
=======

This applies on top of Maxime's sunxi/for-next branch.

Changes v1 .. v2:
- in all three patches:
- use of_clock_full_name() instead of node->name
- don't report the uninitialized clk_name
- handle error in final check in if-clause

Andre Przywara (3):
clk: sunxi: improve mux_clk error handling and reporting
clk: sunxi: improve divider_clk error handling and reporting
clk: sunxi: Improve divs_clk error handling and reporting

 drivers/clk/sunxi/clk-sunxi.c | 75 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 63 insertions(+), 12 deletions(-)

Signature · 1 line

2.6.4

Andre Przywara

author

2016-02-16 10:46 UTC #1 lore raw

PATCH v2 1/3 clk: sunxi: improve mux_clk error handling and reporting

We now catch and report a failing ioremap, also a failure in the final step of the clock registration is now handled and reported. Also warnings are turned into errors.

Signed-off-by Andre Przywara <[email protected]>

---
 drivers/clk/sunxi/clk-sunxi.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

drivers/clk/sunxi/clk-sunxi.c +15 -6

index 99f60ef..775e137 100644
@@ -674,11 +674,16 @@ static struct clk * __init sunxi_mux_clk_setup(struct device_node *node,
 	int i;
 
 	reg = of_iomap(node, 0);
+	if (!reg) {
+		pr_err("Could not map registers for mux-clk: %s\n",
+		       of_node_full_name(node));
+		return NULL;
+	}
 
 	i = of_clk_parent_fill(node, parents, SUNXI_MAX_PARENTS);
 	if (of_property_read_string(node, "clock-output-names", &clk_name)) {
-		pr_warn("%s: could not read clock-output-names for \"%s\"\n",
-			__func__, clk_name);
+		pr_err("%s: could not read clock-output-names from \"%s\"\n",
+		       __func__, of_node_full_name(node));
 		goto out_unmap;
 	}

@@ -688,15 +693,19 @@ static struct clk * __init sunxi_mux_clk_setup(struct device_node *node,
 			       0, &clk_lock);
 
 	if (IS_ERR(clk)) {
-		pr_warn("%s: failed to register mux clock %s: %ld\n", __func__,
-			clk_name, PTR_ERR(clk));
+		pr_err("%s: failed to register mux clock %s: %ld\n", __func__,
+		       clk_name, PTR_ERR(clk));
 		goto out_unmap;
 	}
 
-	of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (of_clk_add_provider(node, of_clk_src_simple_get, clk)) {
+		pr_err("%s: failed to add clock provider for %s\n",
+		       __func__, clk_name);
+		clk_unregister_divider(clk);
+		goto out_unmap;
+	}
 
 	return clk;
-
 out_unmap:
 	iounmap(reg);
 	return NULL;

Signature · 1 line

2.6.4

Andre Przywara

author

2016-02-16 10:46 UTC #2 lore raw

PATCH v2 2/3 clk: sunxi: improve divider_clk error handling and reporting

In reply to Andre Przywara

We now report a failing ioremap, failing output names parsing, failures in table registration and in the final step. Also there was a bug where clk_register_divider_table() would return an ERR_PTR value instead of NULL, which we were checking for. We now implement proper rollback in case of an error.

Signed-off-by Andre Przywara <[email protected]>

---
 drivers/clk/sunxi/clk-sunxi.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

drivers/clk/sunxi/clk-sunxi.c +33 -3

index 775e137..4bd0917 100644
@@ -805,17 +805,47 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
 	void __iomem *reg;
 
 	reg = of_iomap(node, 0);
+	if (!reg) {
+		pr_err("Could not map registers for mux-clk: %s\n",
+		       of_node_full_name(node));
+		return;
+	}
 
 	clk_parent = of_clk_get_parent_name(node, 0);
 
-	of_property_read_string(node, "clock-output-names", &clk_name);
+	if (of_property_read_string(node, "clock-output-names", &clk_name)) {
+		pr_err("%s: could not read clock-output-names from \"%s\"\n",
+		       __func__, of_node_full_name(node));
+		goto out_unmap;
+	}
 
 	clk = clk_register_divider_table(NULL, clk_name, clk_parent, 0,
 					 reg, data->shift, data->width,
 					 data->pow ? CLK_DIVIDER_POWER_OF_TWO : 0,
 					 data->table, &clk_lock);
-	if (clk)
-		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (IS_ERR(clk)) {
+		pr_err("%s: failed to register divider clock %s: %ld\n",
+		       __func__, clk_name, PTR_ERR(clk));
+		goto out_unmap;
+	}
+
+	if (of_clk_add_provider(node, of_clk_src_simple_get, clk)) {
+		pr_err("%s: failed to add clock provider for %s\n",
+		       __func__, clk_name);
+		goto out_unregister;
+	}
+
+	if (clk_register_clkdev(clk, clk_name, NULL)) {
+		of_clk_del_provider(node);
+		goto out_unregister;
+	}
+
+	return;
+out_unregister:
+	clk_unregister_divider(clk);
+
+out_unmap:
+	iounmap(reg);
 }
 
 static void __init sun4i_ahb_clk_setup(struct device_node *node)

Signature · 1 line

2.6.4

Andre Przywara

author

2016-02-16 10:46 UTC #3 lore raw

PATCH v2 3/3 clk: sunxi: Improve divs_clk error handling and reporting

In reply to Andre Przywara

We catch errors in the base clock registration, failure to ioremap and failures in the final of_clk_add_provider() call. Also we unmap the registers when we need to rollback.

Signed-off-by Andre Przywara <[email protected]>

---
 drivers/clk/sunxi/clk-sunxi.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

drivers/clk/sunxi/clk-sunxi.c +15 -3

index 4bd0917..91de0a0 100644
@@ -985,13 +985,20 @@ static struct clk ** __init sunxi_divs_clk_setup(struct device_node *node,
 
 	/* Set up factor clock that we will be dividing */
 	pclk = sunxi_factors_clk_setup(node, data->factors);
+	if (!pclk)
+		return NULL;
 	parent = __clk_get_name(pclk);
 
 	reg = of_iomap(node, 0);
+	if (!reg) {
+		pr_err("Could not map registers for divs-clk: %s\n",
+		       of_node_full_name(node));
+		return NULL;
+	}
 
 	clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL);
 	if (!clk_data)
-		return NULL;
+		goto out_unmap;
 
 	clks = kcalloc(ndivs, sizeof(*clks), GFP_KERNEL);
 	if (!clks)

@@ -1074,16 +1081,21 @@ static struct clk ** __init sunxi_divs_clk_setup(struct device_node *node,
 	/* Adjust to the real max */
 	clk_data->clk_num = i;
 
-	of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+	if (of_clk_add_provider(node, of_clk_src_onecell_get, clk_data)) {
+		pr_err("%s: failed to add clock provider for %s\n",
+		       __func__, clk_name);
+		goto free_gate;
+	}
 
 	return clks;
-
 free_gate:
 	kfree(gate);
 free_clks:
 	kfree(clks);
 free_clkdata:
 	kfree(clk_data);
+out_unmap:
+	iounmap(reg);
 	return NULL;
 }

Signature · 1 line

2.6.4

Maxime Ripard

Applied

2016-02-22 03:45 UTC #4 lore raw

v2 cover clk: sunxi: error checking on clock setup

In reply to Andre Przywara

Hi,

Quoted 15 lines · Andre Przywara

Setting up the clocks properly is quite critical to the system's operation, but currently our error handling is not very verbose. This series adds error handling and reporting to the sunxi clocks, so that any errors are correctly detected and reported. Also previous actions are rolled back in case something went wrong.
This proves to be helpful in debugging clock tree issues, especially when adding support for new SoCs.
I tested this on a BananaPi by deliberately misspelling "clock-output-names". The resulting kernel crash in sun4i_timer_interrupt is totally misleading, but now there is a line in the dmesg before saying:
=======
sunxi_divider_clk_setup: could not read clock-output-names for "apb0"
=======

Quoted 1 line

This applies on top of Maxime's sunxi/for-next branch.

Applied all three, thanks!
Maxime

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160221/2d339957/attachment.sig>