[PATCH 0/9] clocksource/drivers/clksrc-of: Improve error handling

[PATCH 0/9] clocksource/drivers/clksrc-of: Improve error handling

[Daniel Lezcano]

The macro CLOCKSOURCE_OF_DECLARE is widely used in the timer drivers.

Basically, this macro is defined to insert in a table a tuple name,function.
This function is an init function called when the name matches the DT node and
its signature is:

        typedef void (*of_init_fn_1)(struct device_node *);

It does not return an error code.

That results in the clocksource-probe not being able to figure out if the driver
was correctly initialized or not, the different drivers to act as they were the
only ones on the system (panic, instead of failing gracefully), and duplicated
code for error reporting.

This series initiates the logic change and centralize the error handling in the
clocksource probe code.

In order to do the changes little by little, a new macro was introduced:

 CLOCKSOURCE_OF_DECLARE_RET()

It expect an init function signature:

	typedef int (*of_init_ret_fn_1)(struct device_node *);

As soon as all drivers will have their init functions signature changed to
return a value, CLOCKSOURCE_OF_DECLARE_RET will change its name back to
CLOCKSOURCE_OF_DECLARE.

Daniel Lezcano (9):
  of: Add a new macro to declare_of for one parameter function returning
    a value
  clocksource/drivers/clksrc-probe: Introduce init functions with return
    code
  clocksource/drivers/rockchip_timer: Convert init function to return
    error
  clocksource/drivers/mkt_timer: Convert init function to return error
  clocksource/drivers/exynos_mct: Convert init function to return error
  clocksource/drivers/asm9260: Convert init function to return error
  clocksource/drivers/cadence_ttc: Convert init function to return error
  clocksource/drivers/st_lpc: Convert init function to return error
  clocksource/drivers/dw_apb_timer: Convert init function to return
    error

 drivers/clocksource/asm9260_timer.c     | 24 ++++++----
 drivers/clocksource/cadence_ttc_timer.c | 82 +++++++++++++++++++--------------
 drivers/clocksource/clksrc-probe.c      | 22 +++++++++
 drivers/clocksource/clksrc_st_lpc.c     | 22 +++++----
 drivers/clocksource/dw_apb_timer_of.c   | 12 +++--
 drivers/clocksource/exynos_mct.c        | 36 ++++++++++-----
 drivers/clocksource/mtk_timer.c         | 10 ++--
 drivers/clocksource/rockchip_timer.c    | 12 +++--
 include/asm-generic/vmlinux.lds.h       |  2 +
 include/linux/clocksource.h             |  3 ++
 include/linux/of.h                      |  3 ++
 11 files changed, 151 insertions(+), 77 deletions(-)

-- 
1.9.1

[PATCH 1/9] of: Add a new macro to declare_of for one parameter function returning a value

[Daniel Lezcano]

The macro OF_DECLARE_1 expect a void (*func)(struct device_node *) while the
OF_DECLARE_2 expect a int (*func)(struct device_node *, struct device_node *).

The second one allows to pass an init function returning a value, which make
possible to call the functions in the table and check the return value in order
to catch at a higher level the errors and handle them from there instead of
doing a panic in each driver (well at least this is the case for the clkevt).

Unfortunately the OF_DECLARE_1 does not allow that and that lead to some code
duplication and crappyness in the drivers.

The OF_DECLARE_1 is used by all the clk drivers and the clocksource/clockevent
drivers. It is not possible to do the change in one shot as we have to change
all the init functions.

The OF_DECLARE_2 specifies an init function prototype with two parameters with
the node and its parent. The latter won't be used, ever, in the timer drivers.

Introduce a OF_DECLARE_1_RET macro to be used, and hopefully we can smoothly
and iteratively change the users of OF_DECLARE_1 to use the new macro instead.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 include/linux/of.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 7fcb681..5b396d7 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -944,10 +944,13 @@ static inline int of_get_available_child_count(const struct device_node *np)
 #endif
 
 typedef int (*of_init_fn_2)(struct device_node *, struct device_node *);
+typedef int (*of_init_fn_1_ret)(struct device_node *);
 typedef void (*of_init_fn_1)(struct device_node *);
 
 #define OF_DECLARE_1(table, name, compat, fn) \
 		_OF_DECLARE(table, name, compat, fn, of_init_fn_1)
+#define OF_DECLARE_1_RET(table, name, compat, fn) \
+		_OF_DECLARE(table, name, compat, fn, of_init_fn_1_ret)
 #define OF_DECLARE_2(table, name, compat, fn) \
 		_OF_DECLARE(table, name, compat, fn, of_init_fn_2)
 
-- 
1.9.1

Re: [PATCH 1/9] of: Add a new macro to declare_of for one parameter function returning a value

[Daniel Lezcano]

On 06/01/2016 10:34 AM, Daniel Lezcano wrote:
> The macro OF_DECLARE_1 expect a void (*func)(struct device_node *) while the
> OF_DECLARE_2 expect a int (*func)(struct device_node *, struct device_node *).
>
> The second one allows to pass an init function returning a value, which make
> possible to call the functions in the table and check the return value in order
> to catch at a higher level the errors and handle them from there instead of
> doing a panic in each driver (well at least this is the case for the clkevt).
>
> Unfortunately the OF_DECLARE_1 does not allow that and that lead to some code
> duplication and crappyness in the drivers.
>
> The OF_DECLARE_1 is used by all the clk drivers and the clocksource/clockevent
> drivers. It is not possible to do the change in one shot as we have to change
> all the init functions.
>
> The OF_DECLARE_2 specifies an init function prototype with two parameters with
> the node and its parent. The latter won't be used, ever, in the timer drivers.
>
> Introduce a OF_DECLARE_1_RET macro to be used, and hopefully we can smoothly
> and iteratively change the users of OF_DECLARE_1 to use the new macro instead.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---

Rob, Grant,

do you agree with this change ?

Thanks.

   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 2/9] clocksource/drivers/clksrc-probe: Introduce init functions with return code

[Daniel Lezcano]

Currently, the clksrc-probe is not able to handle any error from the init
functions. There are different issues with the current code:
 - the code is duplicated in the init functions by writing error
 - every driver tends to panic in its own init function
 - counting the number of clocksources is not reliable

This patch adds another table to store the functions returning an error.
The table is temporary while we convert all the drivers to return an error
and will disappear.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/clksrc-probe.c | 22 ++++++++++++++++++++++
 include/asm-generic/vmlinux.lds.h  |  2 ++
 include/linux/clocksource.h        |  3 +++
 3 files changed, 27 insertions(+)

diff --git a/drivers/clocksource/clksrc-probe.c b/drivers/clocksource/clksrc-probe.c
index 7cb6c92..5fa6a55 100644
--- a/drivers/clocksource/clksrc-probe.c
+++ b/drivers/clocksource/clksrc-probe.c
@@ -20,16 +20,22 @@
 #include <linux/clocksource.h>
 
 extern struct of_device_id __clksrc_of_table[];
+extern struct of_device_id __clksrc_ret_of_table[];
 
 static const struct of_device_id __clksrc_of_table_sentinel
 	__used __section(__clksrc_of_table_end);
 
+static const struct of_device_id __clksrc_ret_of_table_sentinel
+	__used __section(__clksrc_ret_of_table_end);
+
 void __init clocksource_probe(void)
 {
 	struct device_node *np;
 	const struct of_device_id *match;
 	of_init_fn_1 init_func;
+	of_init_fn_1_ret init_func_ret;
 	unsigned clocksources = 0;
+	int ret;
 
 	for_each_matching_node_and_match(np, __clksrc_of_table, &match) {
 		if (!of_device_is_available(np))
@@ -40,6 +46,22 @@ void __init clocksource_probe(void)
 		clocksources++;
 	}
 
+	for_each_matching_node_and_match(np, __clksrc_ret_of_table, &match) {
+		if (!of_device_is_available(np))
+			continue;
+
+		init_func_ret = match->data;
+
+		ret = init_func_ret(np);
+		if (ret) {
+			pr_err("Failed to initialize '%s': %d",
+			       of_node_full_name(np), ret);
+			continue;
+		}
+
+		clocksources++;
+	}
+
 	clocksources += acpi_probe_device_table(clksrc);
 
 	if (!clocksources)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 339125b..1c655c4 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -173,6 +173,7 @@
 	*(__##name##_of_table_end)
 
 #define CLKSRC_OF_TABLES()	OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
+#define CLKSRC_RET_OF_TABLES()	OF_TABLE(CONFIG_CLKSRC_OF, clksrc_ret)
 #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
 #define CLK_OF_TABLES()		OF_TABLE(CONFIG_COMMON_CLK, clk)
 #define IOMMU_OF_TABLES()	OF_TABLE(CONFIG_OF_IOMMU, iommu)
@@ -529,6 +530,7 @@
 	CLK_OF_TABLES()							\
 	RESERVEDMEM_OF_TABLES()						\
 	CLKSRC_OF_TABLES()						\
+	CLKSRC_RET_OF_TABLES()						\
 	IOMMU_OF_TABLES()						\
 	CPU_METHOD_OF_TABLES()						\
 	CPUIDLE_METHOD_OF_TABLES()					\
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 44a1aff..15c3839 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -246,6 +246,9 @@ extern int clocksource_i8253_init(void);
 #define CLOCKSOURCE_OF_DECLARE(name, compat, fn) \
 	OF_DECLARE_1(clksrc, name, compat, fn)
 
+#define CLOCKSOURCE_OF_DECLARE_RET(name, compat, fn) \
+	OF_DECLARE_1_RET(clksrc_ret, name, compat, fn)
+
 #ifdef CONFIG_CLKSRC_PROBE
 extern void clocksource_probe(void);
 #else
-- 
1.9.1

[PATCH 3/9] clocksource/drivers/rockchip_timer: Convert init function to return error

[Daniel Lezcano]

The init functions do not return any error. They behave as the following:

 - panic, thus leading to a kernel crash while another timer may work and
   make the system boot up correctly

 or

 - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/rockchip_timer.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index b991b28..36b0209 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -106,17 +106,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void __init rk_timer_init(struct device_node *np)
+static int __init rk_timer_init(struct device_node *np)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
 	struct clk *timer_clk;
 	struct clk *pclk;
-	int ret, irq;
+	int ret = -EINVAL, irq;
 
 	bc_timer.base = of_iomap(np, 0);
 	if (!bc_timer.base) {
 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
-		return;
+		return -ENXIO;
 	}
 
 	pclk = of_clk_get_by_name(np, "pclk");
@@ -169,7 +169,7 @@ static void __init rk_timer_init(struct device_node *np)
 
 	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
 
-	return;
+	return 0;
 
 out_irq:
 	clk_disable_unprepare(timer_clk);
@@ -177,6 +177,8 @@ out_timer_clk:
 	clk_disable_unprepare(pclk);
 out_unmap:
 	iounmap(bc_timer.base);
+
+	return ret;
 }
 
-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+CLOCKSOURCE_OF_DECLARE_RET(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
-- 
1.9.1

[PATCH 4/9] clocksource/drivers/mkt_timer: Convert init function to return error

[Daniel Lezcano]

The init functions do not return any error. They behave as the following:

 - panic, thus leading to a kernel crash while another timer may work and
   make the system boot up correctly

 or

 - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/mtk_timer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
index 7e583f8..432a2c0 100644
--- a/drivers/clocksource/mtk_timer.c
+++ b/drivers/clocksource/mtk_timer.c
@@ -181,7 +181,7 @@ static void mtk_timer_enable_irq(struct mtk_clock_event_device *evt, u8 timer)
 			evt->gpt_base + GPT_IRQ_EN_REG);
 }
 
-static void __init mtk_timer_init(struct device_node *node)
+static int __init mtk_timer_init(struct device_node *node)
 {
 	struct mtk_clock_event_device *evt;
 	struct resource res;
@@ -190,7 +190,7 @@ static void __init mtk_timer_init(struct device_node *node)
 
 	evt = kzalloc(sizeof(*evt), GFP_KERNEL);
 	if (!evt)
-		return;
+		return -ENOMEM;
 
 	evt->dev.name = "mtk_tick";
 	evt->dev.rating = 300;
@@ -248,7 +248,7 @@ static void __init mtk_timer_init(struct device_node *node)
 
 	mtk_timer_enable_irq(evt, GPT_CLK_EVT);
 
-	return;
+	return 0;
 
 err_clk_disable:
 	clk_disable_unprepare(clk);
@@ -262,5 +262,7 @@ err_mem:
 	release_mem_region(res.start, resource_size(&res));
 err_kzalloc:
 	kfree(evt);
+
+	return -EINVAL;
 }
-CLOCKSOURCE_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_timer_init);
+CLOCKSOURCE_OF_DECLARE_RET(mtk_mt6577, "mediatek,mt6577-timer", mtk_timer_init);
-- 
1.9.1

[PATCH 5/9] clocksource/drivers/exynos_mct: Convert init function to return error

[Daniel Lezcano]

The init functions do not return any error. They behave as the following:

 - panic, thus leading to a kernel crash while another timer may work and
   make the system boot up correctly

 or

 - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/exynos_mct.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index be09bc0..f6caed0 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -232,7 +232,7 @@ static cycles_t exynos4_read_current_timer(void)
 	return exynos4_read_count_32();
 }
 
-static void __init exynos4_clocksource_init(void)
+static int __init exynos4_clocksource_init(void)
 {
 	exynos4_mct_frc_start();
 
@@ -244,6 +244,8 @@ static void __init exynos4_clocksource_init(void)
 		panic("%s: can't register clocksource\n", mct_frc.name);
 
 	sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
+
+	return 0;
 }
 
 static void exynos4_mct_comp0_stop(void)
@@ -335,12 +337,14 @@ static struct irqaction mct_comp_event_irq = {
 	.dev_id		= &mct_comp_device,
 };
 
-static void exynos4_clockevent_init(void)
+static int exynos4_clockevent_init(void)
 {
 	mct_comp_device.cpumask = cpumask_of(0);
 	clockevents_config_and_register(&mct_comp_device, clk_rate,
 					0xf, 0xffffffff);
 	setup_irq(mct_irqs[MCT_G0_IRQ], &mct_comp_event_irq);
+
+	return 0;
 }
 
 static DEFINE_PER_CPU(struct mct_clock_event_device, percpu_mct_tick);
@@ -516,7 +520,7 @@ static struct notifier_block exynos4_mct_cpu_nb = {
 	.notifier_call = exynos4_mct_cpu_notify,
 };
 
-static void __init exynos4_timer_resources(struct device_node *np, void __iomem *base)
+static int __init exynos4_timer_resources(struct device_node *np, void __iomem *base)
 {
 	int err, cpu;
 	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
@@ -572,15 +576,17 @@ static void __init exynos4_timer_resources(struct device_node *np, void __iomem
 
 	/* Immediately configure the timer on the boot CPU */
 	exynos4_local_timer_setup(mevt);
-	return;
+	return 0;
 
 out_irq:
 	free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
+	return err;
 }
 
-static void __init mct_init_dt(struct device_node *np, unsigned int int_type)
+static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
 {
 	u32 nr_irqs, i;
+	int ret;
 
 	mct_int_type = int_type;
 
@@ -600,20 +606,26 @@ static void __init mct_init_dt(struct device_node *np, unsigned int int_type)
 	for (i = MCT_L0_IRQ; i < nr_irqs; i++)
 		mct_irqs[i] = irq_of_parse_and_map(np, i);
 
-	exynos4_timer_resources(np, of_iomap(np, 0));
-	exynos4_clocksource_init();
-	exynos4_clockevent_init();
+	ret = exynos4_timer_resources(np, of_iomap(np, 0));
+	if (ret)
+		return ret;
+
+	ret = exynos4_clocksource_init();
+	if (ret)
+		return ret;
+
+	return exynos4_clockevent_init();
 }
 
 
-static void __init mct_init_spi(struct device_node *np)
+static int __init mct_init_spi(struct device_node *np)
 {
 	return mct_init_dt(np, MCT_INT_SPI);
 }
 
-static void __init mct_init_ppi(struct device_node *np)
+static int __init mct_init_ppi(struct device_node *np)
 {
 	return mct_init_dt(np, MCT_INT_PPI);
 }
-CLOCKSOURCE_OF_DECLARE(exynos4210, "samsung,exynos4210-mct", mct_init_spi);
-CLOCKSOURCE_OF_DECLARE(exynos4412, "samsung,exynos4412-mct", mct_init_ppi);
+CLOCKSOURCE_OF_DECLARE_RET(exynos4210, "samsung,exynos4210-mct", mct_init_spi);
+CLOCKSOURCE_OF_DECLARE_RET(exynos4412, "samsung,exynos4412-mct", mct_init_ppi);
-- 
1.9.1

Re: [PATCH 5/9] clocksource/drivers/exynos_mct: Convert init function to return error

[Krzysztof Kozlowski]

On 06/01/2016 10:34 AM, Daniel Lezcano wrote:
> The init functions do not return any error. They behave as the following:
> 
>  - panic, thus leading to a kernel crash while another timer may work and
>    make the system boot up correctly
> 
>  or
> 
>  - print an error and let the caller unaware if the state of the system
> 
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
> 
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.

I don't see the benefits of this change alone. You are replacing one
non-return code to another always-return-success code. The effect is
exactly the same. It would make sense to change it to CLOCKSOURCE_OF_RET
along with proper error handling.

Best regards,
Krzysztof

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/clocksource/exynos_mct.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index be09bc0..f6caed0 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -232,7 +232,7 @@ static cycles_t exynos4_read_current_timer(void)
>  	return exynos4_read_count_32();
>  }
>  
> -static void __init exynos4_clocksource_init(void)
> +static int __init exynos4_clocksource_init(void)
>  {
>  	exynos4_mct_frc_start();
>  
> @@ -244,6 +244,8 @@ static void __init exynos4_clocksource_init(void)
>  		panic("%s: can't register clocksource\n", mct_frc.name);
>  
>  	sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
> +
> +	return 0;
>  }
>  
>  static void exynos4_mct_comp0_stop(void)
> @@ -335,12 +337,14 @@ static struct irqaction mct_comp_event_irq = {
>  	.dev_id		= &mct_comp_device,
>  };
>  
> -static void exynos4_clockevent_init(void)
> +static int exynos4_clockevent_init(void)
>  {
>  	mct_comp_device.cpumask = cpumask_of(0);
>  	clockevents_config_and_register(&mct_comp_device, clk_rate,
>  					0xf, 0xffffffff);
>  	setup_irq(mct_irqs[MCT_G0_IRQ], &mct_comp_event_irq);
> +
> +	return 0;
>  }
>  
>  static DEFINE_PER_CPU(struct mct_clock_event_device, percpu_mct_tick);
> @@ -516,7 +520,7 @@ static struct notifier_block exynos4_mct_cpu_nb = {
>  	.notifier_call = exynos4_mct_cpu_notify,
>  };
>  
> -static void __init exynos4_timer_resources(struct device_node *np, void __iomem *base)
> +static int __init exynos4_timer_resources(struct device_node *np, void __iomem *base)
>  {
>  	int err, cpu;
>  	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
> @@ -572,15 +576,17 @@ static void __init exynos4_timer_resources(struct device_node *np, void __iomem
>  
>  	/* Immediately configure the timer on the boot CPU */
>  	exynos4_local_timer_setup(mevt);
> -	return;
> +	return 0;
>  
>  out_irq:
>  	free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
> +	return err;
>  }
>  
> -static void __init mct_init_dt(struct device_node *np, unsigned int int_type)
> +static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
>  {
>  	u32 nr_irqs, i;
> +	int ret;
>  
>  	mct_int_type = int_type;
>  
> @@ -600,20 +606,26 @@ static void __init mct_init_dt(struct device_node *np, unsigned int int_type)
>  	for (i = MCT_L0_IRQ; i < nr_irqs; i++)
>  		mct_irqs[i] = irq_of_parse_and_map(np, i);
>  
> -	exynos4_timer_resources(np, of_iomap(np, 0));
> -	exynos4_clocksource_init();
> -	exynos4_clockevent_init();
> +	ret = exynos4_timer_resources(np, of_iomap(np, 0));
> +	if (ret)
> +		return ret;
> +
> +	ret = exynos4_clocksource_init();
> +	if (ret)
> +		return ret;
> +
> +	return exynos4_clockevent_init();
>  }
>  
>  
> -static void __init mct_init_spi(struct device_node *np)
> +static int __init mct_init_spi(struct device_node *np)
>  {
>  	return mct_init_dt(np, MCT_INT_SPI);
>  }
>  
> -static void __init mct_init_ppi(struct device_node *np)
> +static int __init mct_init_ppi(struct device_node *np)
>  {
>  	return mct_init_dt(np, MCT_INT_PPI);
>  }
> -CLOCKSOURCE_OF_DECLARE(exynos4210, "samsung,exynos4210-mct", mct_init_spi);
> -CLOCKSOURCE_OF_DECLARE(exynos4412, "samsung,exynos4412-mct", mct_init_ppi);
> +CLOCKSOURCE_OF_DECLARE_RET(exynos4210, "samsung,exynos4210-mct", mct_init_spi);
> +CLOCKSOURCE_OF_DECLARE_RET(exynos4412, "samsung,exynos4412-mct", mct_init_ppi);
> 

Re: [PATCH 5/9] clocksource/drivers/exynos_mct: Convert init function to return error

[Daniel Lezcano]

On 06/06/2016 12:51 PM, Krzysztof Kozlowski wrote:
> On 06/01/2016 10:34 AM, Daniel Lezcano wrote:
>> The init functions do not return any error. They behave as the following:
>>
>>   - panic, thus leading to a kernel crash while another timer may work and
>>     make the system boot up correctly
>>
>>   or
>>
>>   - print an error and let the caller unaware if the state of the system
>>
>> Change that by converting the init functions to return an error conforming
>> to the CLOCKSOURCE_OF_RET prototype.
>>
>> Proper error handling (rollback, errno value) will be changed later case
>> by case, thus this change just return back an error or success in the init
>> function.
>
> I don't see the benefits of this change alone. You are replacing one
> non-return code to another always-return-success code. The effect is
> exactly the same. It would make sense to change it to CLOCKSOURCE_OF_RET
> along with proper error handling.
>

Hi Krzysztof,

I can understand you expect an error handling but, as stated in the 
changelog, proper error handling will be done later. Currently, there 
are roughly 80 drivers to changes the init function to return a value in 
order change the CLOCKSOURCE_OF_DECLARE macro. That is a quite important 
number and changing also the internals will introduce bugs.

So the series (and there is a huge one coming right after this one), 
will focus only on changing the init function to return a value, so we 
can swap the clocksource table and rename CLOCKSOURCE_OF_DECLARE_RET 
without the '_RET'. I want this to be done before the next PR in order 
to prevent to have a double clksrc table.

If error handling is already taken into account and is trivial, I try to 
do the change, otherwise I let the function to return a success value.

Regarding the important number of drivers, if you have time to change 
the init function in the exynos_mct, that will be more than welcome :)

   -- Daniel




-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 5/9] clocksource/drivers/exynos_mct: Convert init function to return error

[Krzysztof Kozlowski]

On 06/06/2016 01:23 PM, Daniel Lezcano wrote:
> On 06/06/2016 12:51 PM, Krzysztof Kozlowski wrote:
>> On 06/01/2016 10:34 AM, Daniel Lezcano wrote:
>>> The init functions do not return any error. They behave as the
>>> following:
>>>
>>>   - panic, thus leading to a kernel crash while another timer may
>>> work and
>>>     make the system boot up correctly
>>>
>>>   or
>>>
>>>   - print an error and let the caller unaware if the state of the system
>>>
>>> Change that by converting the init functions to return an error
>>> conforming
>>> to the CLOCKSOURCE_OF_RET prototype.
>>>
>>> Proper error handling (rollback, errno value) will be changed later case
>>> by case, thus this change just return back an error or success in the
>>> init
>>> function.
>>
>> I don't see the benefits of this change alone. You are replacing one
>> non-return code to another always-return-success code. The effect is
>> exactly the same. It would make sense to change it to CLOCKSOURCE_OF_RET
>> along with proper error handling.
>>
> 
> Hi Krzysztof,
> 
> I can understand you expect an error handling but, as stated in the
> changelog, proper error handling will be done later. Currently, there
> are roughly 80 drivers to changes the init function to return a value in
> order change the CLOCKSOURCE_OF_DECLARE macro. That is a quite important
> number and changing also the internals will introduce bugs.
> 
> So the series (and there is a huge one coming right after this one),
> will focus only on changing the init function to return a value, so we
> can swap the clocksource table and rename CLOCKSOURCE_OF_DECLARE_RET
> without the '_RET'. I want this to be done before the next PR in order
> to prevent to have a double clksrc table.
> 
> If error handling is already taken into account and is trivial, I try to
> do the change, otherwise I let the function to return a success value.
> 
> Regarding the important number of drivers, if you have time to change
> the init function in the exynos_mct, that will be more than welcome :)

Ah, okay, you are planning to get rid of CLOCKSOURCE_OF_DECLARE. I
didn't receive the cover letter so the big picture remained hidden. Now
it looks good:

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

[PATCH 6/9] clocksource/drivers/asm9260: Convert init function to return error

[Daniel Lezcano]

The init functions do not return any error. They behave as the following:

 - panic, thus leading to a kernel crash while another timer may work and
   make the system boot up correctly

 or

 - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/asm9260_timer.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/asm9260_timer.c b/drivers/clocksource/asm9260_timer.c
index 217438d..d113c02 100644
--- a/drivers/clocksource/asm9260_timer.c
+++ b/drivers/clocksource/asm9260_timer.c
@@ -184,7 +184,7 @@ static irqreturn_t asm9260_timer_interrupt(int irq, void *dev_id)
  * Timer initialization
  * ---------------------------------------------------------------------------
  */
-static void __init asm9260_timer_init(struct device_node *np)
+static int __init asm9260_timer_init(struct device_node *np)
 {
 	int irq;
 	struct clk *clk;
@@ -192,20 +192,26 @@ static void __init asm9260_timer_init(struct device_node *np)
 	unsigned long rate;
 
 	priv.base = of_io_request_and_map(np, 0, np->name);
-	if (IS_ERR(priv.base))
-		panic("%s: unable to map resource", np->name);
+	if (IS_ERR(priv.base)) {
+		pr_err("%s: unable to map resource", np->name);
+		return PTR_ERR(priv.base);
+	}
 
 	clk = of_clk_get(np, 0);
 
 	ret = clk_prepare_enable(clk);
-	if (ret)
-		panic("Failed to enable clk!\n");
+	if (ret) {
+		pr_err("Failed to enable clk!\n");
+		return ret;
+	}
 
 	irq = irq_of_parse_and_map(np, 0);
 	ret = request_irq(irq, asm9260_timer_interrupt, IRQF_TIMER,
 			DRIVER_NAME, &event_dev);
-	if (ret)
-		panic("Failed to setup irq!\n");
+	if (ret) {
+		pr_err("Failed to setup irq!\n");
+		return ret;
+	}
 
 	/* set all timers for count-up */
 	writel_relaxed(BM_DIR_DEFAULT, priv.base + HW_DIR);
@@ -229,6 +235,8 @@ static void __init asm9260_timer_init(struct device_node *np)
 	priv.ticks_per_jiffy = DIV_ROUND_CLOSEST(rate, HZ);
 	event_dev.cpumask = cpumask_of(0);
 	clockevents_config_and_register(&event_dev, rate, 0x2c00, 0xfffffffe);
+
+	return 0;
 }
-CLOCKSOURCE_OF_DECLARE(asm9260_timer, "alphascale,asm9260-timer",
+CLOCKSOURCE_OF_DECLARE_RET(asm9260_timer, "alphascale,asm9260-timer",
 		asm9260_timer_init);
-- 
1.9.1

[PATCH 7/9] clocksource/drivers/cadence_ttc: Convert init function to return error

[Daniel Lezcano]

The init functions do not return any error. They behave as the following:

 - panic, thus leading to a kernel crash while another timer may work and
   make the system boot up correctly

 or

 - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/cadence_ttc_timer.c | 82 +++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 34 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index 9be6018..aa36cbf 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -322,22 +322,22 @@ static int ttc_rate_change_clocksource_cb(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
+static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
 					 u32 timer_width)
 {
 	struct ttc_timer_clocksource *ttccs;
 	int err;
 
 	ttccs = kzalloc(sizeof(*ttccs), GFP_KERNEL);
-	if (WARN_ON(!ttccs))
-		return;
+	if (!ttccs)
+		return -ENOMEM;
 
 	ttccs->ttc.clk = clk;
 
 	err = clk_prepare_enable(ttccs->ttc.clk);
-	if (WARN_ON(err)) {
+	if (err) {
 		kfree(ttccs);
-		return;
+		return err;
 	}
 
 	ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
@@ -345,9 +345,13 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
 	ttccs->ttc.clk_rate_change_nb.notifier_call =
 		ttc_rate_change_clocksource_cb;
 	ttccs->ttc.clk_rate_change_nb.next = NULL;
-	if (clk_notifier_register(ttccs->ttc.clk,
-				&ttccs->ttc.clk_rate_change_nb))
+
+	err = clk_notifier_register(ttccs->ttc.clk,
+				    &ttccs->ttc.clk_rate_change_nb);
+	if (err) {
 		pr_warn("Unable to register clock notifier.\n");
+		return err;
+	}
 
 	ttccs->ttc.base_addr = base;
 	ttccs->cs.name = "ttc_clocksource";
@@ -368,14 +372,16 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
 		     ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
 
 	err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
-	if (WARN_ON(err)) {
+	if (err) {
 		kfree(ttccs);
-		return;
+		return err;
 	}
 
 	ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
 	sched_clock_register(ttc_sched_clock_read, timer_width,
 			     ttccs->ttc.freq / PRESCALE);
+
+	return 0;
 }
 
 static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
@@ -401,30 +407,35 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
 	}
 }
 
-static void __init ttc_setup_clockevent(struct clk *clk,
-						void __iomem *base, u32 irq)
+static int __init ttc_setup_clockevent(struct clk *clk,
+				       void __iomem *base, u32 irq)
 {
 	struct ttc_timer_clockevent *ttcce;
 	int err;
 
 	ttcce = kzalloc(sizeof(*ttcce), GFP_KERNEL);
-	if (WARN_ON(!ttcce))
-		return;
+	if (!ttcce)
+		return -ENOMEM;
 
 	ttcce->ttc.clk = clk;
 
 	err = clk_prepare_enable(ttcce->ttc.clk);
-	if (WARN_ON(err)) {
+	if (err) {
 		kfree(ttcce);
-		return;
+		return err;
 	}
 
 	ttcce->ttc.clk_rate_change_nb.notifier_call =
 		ttc_rate_change_clockevent_cb;
 	ttcce->ttc.clk_rate_change_nb.next = NULL;
-	if (clk_notifier_register(ttcce->ttc.clk,
-				&ttcce->ttc.clk_rate_change_nb))
+
+	err = clk_notifier_register(ttcce->ttc.clk,
+				    &ttcce->ttc.clk_rate_change_nb);
+	if (err) {
 		pr_warn("Unable to register clock notifier.\n");
+		return err;
+	}
+
 	ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
 
 	ttcce->ttc.base_addr = base;
@@ -451,13 +462,15 @@ static void __init ttc_setup_clockevent(struct clk *clk,
 
 	err = request_irq(irq, ttc_clock_event_interrupt,
 			  IRQF_TIMER, ttcce->ce.name, ttcce);
-	if (WARN_ON(err)) {
+	if (err) {
 		kfree(ttcce);
-		return;
+		return err;
 	}
 
 	clockevents_config_and_register(&ttcce->ce,
 			ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
+
+	return 0;
 }
 
 /**
@@ -466,20 +479,14 @@ static void __init ttc_setup_clockevent(struct clk *clk,
  * Initializes the timer hardware and register the clock source and clock event
  * timers with Linux kernal timer framework
  */
-static void __init ttc_timer_init(struct device_node *timer)
+static int __init ttc_timer_init(struct device_node *timer)
 {
 	unsigned int irq;
 	void __iomem *timer_baseaddr;
 	struct clk *clk_cs, *clk_ce;
-	static int initialized;
-	int clksel;
+	int clksel, ret;
 	u32 timer_width = 16;
 
-	if (initialized)
-		return;
-
-	initialized = 1;
-
 	/*
 	 * Get the 1st Triple Timer Counter (TTC) block from the device tree
 	 * and use it. Note that the event timer uses the interrupt and it's the
@@ -488,13 +495,13 @@ static void __init ttc_timer_init(struct device_node *timer)
 	timer_baseaddr = of_iomap(timer, 0);
 	if (!timer_baseaddr) {
 		pr_err("ERROR: invalid timer base address\n");
-		BUG();
+		return -ENXIO;
 	}
 
 	irq = irq_of_parse_and_map(timer, 1);
 	if (irq <= 0) {
 		pr_err("ERROR: invalid interrupt number\n");
-		BUG();
+		return -EINVAL;
 	}
 
 	of_property_read_u32(timer, "timer-width", &timer_width);
@@ -504,7 +511,7 @@ static void __init ttc_timer_init(struct device_node *timer)
 	clk_cs = of_clk_get(timer, clksel);
 	if (IS_ERR(clk_cs)) {
 		pr_err("ERROR: timer input clock not found\n");
-		BUG();
+		return PTR_ERR(clk_cs);
 	}
 
 	clksel = readl_relaxed(timer_baseaddr + 4 + TTC_CLK_CNTRL_OFFSET);
@@ -512,13 +519,20 @@ static void __init ttc_timer_init(struct device_node *timer)
 	clk_ce = of_clk_get(timer, clksel);
 	if (IS_ERR(clk_ce)) {
 		pr_err("ERROR: timer input clock not found\n");
-		BUG();
+		return PTR_ERR(clk_cs);
 	}
 
-	ttc_setup_clocksource(clk_cs, timer_baseaddr, timer_width);
-	ttc_setup_clockevent(clk_ce, timer_baseaddr + 4, irq);
+	ret = ttc_setup_clocksource(clk_cs, timer_baseaddr, timer_width);
+	if (ret)
+		return ret;
+
+	ret = ttc_setup_clockevent(clk_ce, timer_baseaddr + 4, irq);
+	if (ret)
+		return ret;
 
 	pr_info("%s #0 at %p, irq=%d\n", timer->name, timer_baseaddr, irq);
+
+	return 0;
 }
 
-CLOCKSOURCE_OF_DECLARE(ttc, "cdns,ttc", ttc_timer_init);
+CLOCKSOURCE_OF_DECLARE_RET(ttc, "cdns,ttc", ttc_timer_init);
-- 
1.9.1

Re: [PATCH 7/9] clocksource/drivers/cadence_ttc: Convert init function to return error

[Sören Brinkmann]

Hi Daniel,

On Wed, 2016-06-01 at 10:34:50 +0200, Daniel Lezcano wrote:
> The init functions do not return any error. They behave as the following:
> 
>  - panic, thus leading to a kernel crash while another timer may work and
>    make the system boot up correctly
> 
>  or
> 
>  - print an error and let the caller unaware if the state of the system
> 
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
> 
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/clocksource/cadence_ttc_timer.c | 82 +++++++++++++++++++--------------
>  1 file changed, 48 insertions(+), 34 deletions(-)
> 
[...]
>  	ttcce->ttc.clk_rate_change_nb.notifier_call =
>  		ttc_rate_change_clockevent_cb;
>  	ttcce->ttc.clk_rate_change_nb.next = NULL;
> -	if (clk_notifier_register(ttcce->ttc.clk,
> -				&ttcce->ttc.clk_rate_change_nb))
> +
> +	err = clk_notifier_register(ttcce->ttc.clk,
> +				    &ttcce->ttc.clk_rate_change_nb);
> +	if (err) {
>  		pr_warn("Unable to register clock notifier.\n");
> +		return err;

So far we handle this as warning only and move on, as the notifier is
only needed when frequency scaling is enabled. And even then, the effect
is usually just that timing is off.

> +	}
> +
>  	ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
>  
>  	ttcce->ttc.base_addr = base;
> @@ -451,13 +462,15 @@ static void __init ttc_setup_clockevent(struct clk *clk,
>  
>  	err = request_irq(irq, ttc_clock_event_interrupt,
>  			  IRQF_TIMER, ttcce->ce.name, ttcce);
> -	if (WARN_ON(err)) {
> +	if (err) {
>  		kfree(ttcce);
> -		return;
> +		return err;
>  	}
>  
>  	clockevents_config_and_register(&ttcce->ce,
>  			ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -466,20 +479,14 @@ static void __init ttc_setup_clockevent(struct clk *clk,
>   * Initializes the timer hardware and register the clock source and clock event
>   * timers with Linux kernal timer framework
>   */
> -static void __init ttc_timer_init(struct device_node *timer)
> +static int __init ttc_timer_init(struct device_node *timer)
>  {
>  	unsigned int irq;
>  	void __iomem *timer_baseaddr;
>  	struct clk *clk_cs, *clk_ce;
> -	static int initialized;
> -	int clksel;
> +	int clksel, ret;
>  	u32 timer_width = 16;
>  
> -	if (initialized)
> -		return;
> -
> -	initialized = 1;
> -

This also changes behavior. We have multiple of these timer modules in
our HW and we don't want them all to be used for time keeping. This
construct made sure that we only use the first timer for which init is
called leaving the others for non-OS purposes.

	Sören

Re: [PATCH 7/9] clocksource/drivers/cadence_ttc: Convert init function to return error

[Daniel Lezcano]

On 06/01/2016 04:36 PM, Sören Brinkmann wrote:
> Hi Daniel,

Hi Soren,

[ ... ]

>> +	err = clk_notifier_register(ttcce->ttc.clk,
>> +				    &ttcce->ttc.clk_rate_change_nb);
>> +	if (err) {
>>   		pr_warn("Unable to register clock notifier.\n");
>> +		return err;
>
> So far we handle this as warning only and move on, as the notifier is
> only needed when frequency scaling is enabled. And even then, the effect
> is usually just that timing is off.

Ok, I will fix it.

[ ... ]

>> -	static int initialized;
>> -	int clksel;
>> +	int clksel, ret;
>>   	u32 timer_width = 16;
>>
>> -	if (initialized)
>> -		return;
>> -
>> -	initialized = 1;
>> -
>
> This also changes behavior. We have multiple of these timer modules in
> our HW and we don't want them all to be used for time keeping. This
> construct made sure that we only use the first timer for which init is
> called leaving the others for non-OS purposes.

Ha, yes. My bad, this change was not supposed to be here.

Thanks for spotting this.

   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 8/9] clocksource/drivers/st_lpc: Convert init function to return error

[Daniel Lezcano]

The init functions do not return any error. They behave as the following:

 - panic, thus leading to a kernel crash while another timer may work and
   make the system boot up correctly

 or

 - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/clksrc_st_lpc.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/clocksource/clksrc_st_lpc.c b/drivers/clocksource/clksrc_st_lpc.c
index 65ec467..c9022a9 100644
--- a/drivers/clocksource/clksrc_st_lpc.c
+++ b/drivers/clocksource/clksrc_st_lpc.c
@@ -92,7 +92,7 @@ static int __init st_clksrc_setup_clk(struct device_node *np)
 	return 0;
 }
 
-static void __init st_clksrc_of_register(struct device_node *np)
+static int __init st_clksrc_of_register(struct device_node *np)
 {
 	int ret;
 	uint32_t mode;
@@ -100,32 +100,36 @@ static void __init st_clksrc_of_register(struct device_node *np)
 	ret = of_property_read_u32(np, "st,lpc-mode", &mode);
 	if (ret) {
 		pr_err("clksrc-st-lpc: An LPC mode must be provided\n");
-		return;
+		return ret;
 	}
 
 	/* LPC can either run as a Clocksource or in RTC or WDT mode */
 	if (mode != ST_LPC_MODE_CLKSRC)
-		return;
+		return 0;
 
 	ddata.base = of_iomap(np, 0);
 	if (!ddata.base) {
 		pr_err("clksrc-st-lpc: Unable to map iomem\n");
-		return;
+		return -ENXIO;
 	}
 
-	if (st_clksrc_setup_clk(np)) {
+	ret = st_clksrc_setup_clk(np);
+	if (ret) {
 		iounmap(ddata.base);
-		return;
+		return ret;
 	}
 
-	if (st_clksrc_init()) {
+	ret = st_clksrc_init();
+	if (ret) {
 		clk_disable_unprepare(ddata.clk);
 		clk_put(ddata.clk);
 		iounmap(ddata.base);
-		return;
+		return ret;
 	}
 
 	pr_info("clksrc-st-lpc: clocksource initialised - running @ %luHz\n",
 		clk_get_rate(ddata.clk));
+
+	return ret;
 }
-CLOCKSOURCE_OF_DECLARE(ddata, "st,stih407-lpc", st_clksrc_of_register);
+CLOCKSOURCE_OF_DECLARE_RET(ddata, "st,stih407-lpc", st_clksrc_of_register);
-- 
1.9.1

[PATCH 9/9] clocksource/drivers/dw_apb_timer: Convert init function to return error

[Daniel Lezcano]

The init functions do not return any error. They behave as the following:

 - panic, thus leading to a kernel crash while another timer may work and
   make the system boot up correctly

 or

 - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/dw_apb_timer_of.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index 860843c..4985a2c 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -143,7 +143,7 @@ static struct delay_timer dw_apb_delay_timer = {
 #endif
 
 static int num_called;
-static void __init dw_apb_timer_init(struct device_node *timer)
+static int __init dw_apb_timer_init(struct device_node *timer)
 {
 	switch (num_called) {
 	case 0:
@@ -164,8 +164,10 @@ static void __init dw_apb_timer_init(struct device_node *timer)
 	}
 
 	num_called++;
+
+	return 0;
 }
-CLOCKSOURCE_OF_DECLARE(pc3x2_timer, "picochip,pc3x2-timer", dw_apb_timer_init);
-CLOCKSOURCE_OF_DECLARE(apb_timer_osc, "snps,dw-apb-timer-osc", dw_apb_timer_init);
-CLOCKSOURCE_OF_DECLARE(apb_timer_sp, "snps,dw-apb-timer-sp", dw_apb_timer_init);
-CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer", dw_apb_timer_init);
+CLOCKSOURCE_OF_DECLARE_RET(pc3x2_timer, "picochip,pc3x2-timer", dw_apb_timer_init);
+CLOCKSOURCE_OF_DECLARE_RET(apb_timer_osc, "snps,dw-apb-timer-osc", dw_apb_timer_init);
+CLOCKSOURCE_OF_DECLARE_RET(apb_timer_sp, "snps,dw-apb-timer-sp", dw_apb_timer_init);
+CLOCKSOURCE_OF_DECLARE_RET(apb_timer, "snps,dw-apb-timer", dw_apb_timer_init);
-- 
1.9.1

Re: [PATCH 0/9] clocksource/drivers/clksrc-of: Improve error handling

[Geert Uytterhoeven]

Hi Daniel,

On Wed, Jun 1, 2016 at 10:34 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> The macro CLOCKSOURCE_OF_DECLARE is widely used in the timer drivers.
>
> Basically, this macro is defined to insert in a table a tuple name,function.
> This function is an init function called when the name matches the DT node and
> its signature is:
>
>         typedef void (*of_init_fn_1)(struct device_node *);
>
> It does not return an error code.
>
> That results in the clocksource-probe not being able to figure out if the driver
> was correctly initialized or not, the different drivers to act as they were the
> only ones on the system (panic, instead of failing gracefully), and duplicated
> code for error reporting.
>
> This series initiates the logic change and centralize the error handling in the
> clocksource probe code.
>
> In order to do the changes little by little, a new macro was introduced:
>
>  CLOCKSOURCE_OF_DECLARE_RET()

As I have no other thread to reply to, I'm using this related one.

commit bcbe219f9306da478b77e705a7273843c2660d7b
Author: Daniel Lezcano <daniel.lezcano@linaro.org>
Date:   Tue Jun 7 00:27:44 2016 +0200

    clocksources: Switch back to the clksrc table

    Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

in clockevents/clockevents/next breaks the boot on e.g. r8a7791/koelsch
(arm32) and r8a7795/salvator-x (arm64).

Using "earlycon keep_bootcon" on koelsch (this doesn't help on arm64)
reveals it's stuck at:

    clocksource_probe: no matching clocksources found
    sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every
21474836475000000ns
    Calibrating delay loop...

With the above commit reverted, it works again:

    Architected cp15 timer(s) running at 10.00MHz (virt).
    clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles:
0x24e6a1710, max_idle_ns: 440795202120 ns
    sched_clock: 56 bits at 10MHz, resolution 100ns, wraps every 4398046511100ns
    Switching to timer-based delay loop, resolution 100ns
    Calibrating delay loop (skipped), value calculated using timer
frequency.. 20.00 BogoMIPS (lpj=100000)

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 0/9] clocksource/drivers/clksrc-of: Improve error handling

[Daniel Lezcano]

On 06/07/2016 11:54 AM, Geert Uytterhoeven wrote:
> Hi Daniel,
>
> On Wed, Jun 1, 2016 at 10:34 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> The macro CLOCKSOURCE_OF_DECLARE is widely used in the timer drivers.
>>
>> Basically, this macro is defined to insert in a table a tuple name,function.
>> This function is an init function called when the name matches the DT node and
>> its signature is:
>>
>>          typedef void (*of_init_fn_1)(struct device_node *);
>>
>> It does not return an error code.
>>
>> That results in the clocksource-probe not being able to figure out if the driver
>> was correctly initialized or not, the different drivers to act as they were the
>> only ones on the system (panic, instead of failing gracefully), and duplicated
>> code for error reporting.
>>
>> This series initiates the logic change and centralize the error handling in the
>> clocksource probe code.
>>
>> In order to do the changes little by little, a new macro was introduced:
>>
>>   CLOCKSOURCE_OF_DECLARE_RET()
>
> As I have no other thread to reply to, I'm using this related one.
>
> commit bcbe219f9306da478b77e705a7273843c2660d7b
> Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> Date:   Tue Jun 7 00:27:44 2016 +0200
>
>      clocksources: Switch back to the clksrc table
>
>      Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
> in clockevents/clockevents/next breaks the boot on e.g. r8a7791/koelsch
> (arm32) and r8a7795/salvator-x (arm64).
>
> Using "earlycon keep_bootcon" on koelsch (this doesn't help on arm64)
> reveals it's stuck at:
>
>      clocksource_probe: no matching clocksources found
>      sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every
> 21474836475000000ns
>      Calibrating delay loop...
>
> With the above commit reverted, it works again:

Ah, thanks for reporting.

I will fix it.

   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/9] clocksource/drivers/clksrc-of: Improve error handling

[Daniel Lezcano]

On 06/07/2016 11:54 AM, Geert Uytterhoeven wrote:
> Hi Daniel,

Hi Geert,

[ ... ]

> Using "earlycon keep_bootcon" on koelsch (this doesn't help on arm64)
> reveals it's stuck at:
>
>      clocksource_probe: no matching clocksources found
>      sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every
> 21474836475000000ns
>      Calibrating delay loop...
>

Ok, so the "renesas,cmt-48-gen2" is not impacted by the changes because 
it uses the platform approach. Very likely, the issue is coming from the 
arch_arm_timer failing somewhere.

Or I added a regression and the timer is wrongly returning an error, or 
the error is always there and now it is caught.

Can you give the traces before 'clocksource_probe: no matching 
clocksources found' ?

   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/9] clocksource/drivers/clksrc-of: Improve error handling

[Daniel Lezcano]

On 06/07/2016 11:54 AM, Geert Uytterhoeven wrote:

[ ... ]

> in clockevents/clockevents/next breaks the boot on e.g. r8a7791/koelsch
> (arm32) and r8a7795/salvator-x (arm64).
>
> Using "earlycon keep_bootcon" on koelsch (this doesn't help on arm64)
> reveals it's stuck at:
>
>      clocksource_probe: no matching clocksources found
>      sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every
> 21474836475000000ns
>      Calibrating delay loop...
>
> With the above commit reverted, it works again:
>
>      Architected cp15 timer(s) running at 10.00MHz (virt).
>      clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles:
> 0x24e6a1710, max_idle_ns: 440795202120 ns
>      sched_clock: 56 bits at 10MHz, resolution 100ns, wraps every 4398046511100ns
>      Switching to timer-based delay loop, resolution 100ns
>      Calibrating delay loop (skipped), value calculated using timer
> frequency.. 20.00 BogoMIPS (lpj=100000)

I think it is fixed now and pushed on my tree. Is it possible to confirm 
your boards are working correctly again after the linux-next is updated 
with my latest changes ?

Thanks for the report.

   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/9] clocksource/drivers/clksrc-of: Improve error handling

[Geert Uytterhoeven]

Hi Daniel,

On Wed, Jun 8, 2016 at 4:10 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 06/07/2016 11:54 AM, Geert Uytterhoeven wrote:
>
> [ ... ]
>
>> in clockevents/clockevents/next breaks the boot on e.g. r8a7791/koelsch
>> (arm32) and r8a7795/salvator-x (arm64).
>>
>> Using "earlycon keep_bootcon" on koelsch (this doesn't help on arm64)
>> reveals it's stuck at:
>>
>>      clocksource_probe: no matching clocksources found
>>      sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every
>> 21474836475000000ns
>>      Calibrating delay loop...
>>
>> With the above commit reverted, it works again:
>>
>>      Architected cp15 timer(s) running at 10.00MHz (virt).
>>      clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles:
>> 0x24e6a1710, max_idle_ns: 440795202120 ns
>>      sched_clock: 56 bits at 10MHz, resolution 100ns, wraps every
>> 4398046511100ns
>>      Switching to timer-based delay loop, resolution 100ns
>>      Calibrating delay loop (skipped), value calculated using timer
>> frequency.. 20.00 BogoMIPS (lpj=100000)
>
> I think it is fixed now and pushed on my tree. Is it possible to confirm
> your boards are working correctly again after the linux-next is updated with
> my latest changes ?

I can confirm the issue is fixed in today's clockevents/next
(52be039599e1339e).

Thanks!

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 0/9] clocksource/drivers/clksrc-of: Improve error handling

[Daniel Lezcano]

On 06/09/2016 09:46 AM, Geert Uytterhoeven wrote:
> Hi Daniel,
>
> On Wed, Jun 8, 2016 at 4:10 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 06/07/2016 11:54 AM, Geert Uytterhoeven wrote:
>>
>> [ ... ]
>>
>>> in clockevents/clockevents/next breaks the boot on e.g. r8a7791/koelsch
>>> (arm32) and r8a7795/salvator-x (arm64).
>>>
>>> Using "earlycon keep_bootcon" on koelsch (this doesn't help on arm64)
>>> reveals it's stuck at:
>>>
>>>       clocksource_probe: no matching clocksources found
>>>       sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every
>>> 21474836475000000ns
>>>       Calibrating delay loop...
>>>
>>> With the above commit reverted, it works again:
>>>
>>>       Architected cp15 timer(s) running at 10.00MHz (virt).
>>>       clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles:
>>> 0x24e6a1710, max_idle_ns: 440795202120 ns
>>>       sched_clock: 56 bits at 10MHz, resolution 100ns, wraps every
>>> 4398046511100ns
>>>       Switching to timer-based delay loop, resolution 100ns
>>>       Calibrating delay loop (skipped), value calculated using timer
>>> frequency.. 20.00 BogoMIPS (lpj=100000)
>>
>> I think it is fixed now and pushed on my tree. Is it possible to confirm
>> your boards are working correctly again after the linux-next is updated with
>> my latest changes ?
>
> I can confirm the issue is fixed in today's clockevents/next
> (52be039599e1339e).

Great ! Thanks for testing.

   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog