clocksource: owl: Improve owl_timer_init fail messages
diff mbox series

Message ID 20200214064923.190035-1-matheus@castello.eng.br
State New
Headers show
Series
  • clocksource: owl: Improve owl_timer_init fail messages
Related show

Commit Message

Matheus Castello Feb. 14, 2020, 6:49 a.m. UTC
Adding error messages, in case of not having a defined clock property
and in case of an error in clocksource_mmio_init, which may not be
fatal, so just adding a pr_err to notify that it failed.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---

Tested on my Caninos Labrador s500 based board. If the clock property is not
set this message would help debug:

...
[    0.000000] Failed to get OF clock for clocksource
[    0.000000] Failed to initialize '/soc/timer@b0168000': -2
[    0.000000] timer_probe: no matching timers found
...

 drivers/clocksource/timer-owl.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

--
2.25.0

Comments

Manivannan Sadhasivam Feb. 14, 2020, 2:17 p.m. UTC | #1
Hi,

Thanks for the patch!

On Fri, Feb 14, 2020 at 03:49:23AM -0300, Matheus Castello wrote:
> Adding error messages, in case of not having a defined clock property
> and in case of an error in clocksource_mmio_init, which may not be
> fatal, so just adding a pr_err to notify that it failed.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
> 
> Tested on my Caninos Labrador s500 based board. If the clock property is not
> set this message would help debug:
> 
> ...
> [    0.000000] Failed to get OF clock for clocksource
> [    0.000000] Failed to initialize '/soc/timer@b0168000': -2
> [    0.000000] timer_probe: no matching timers found
> ...
> 
>  drivers/clocksource/timer-owl.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-owl.c b/drivers/clocksource/timer-owl.c
> index 900fe736145d..f53596f9e86c 100644
> --- a/drivers/clocksource/timer-owl.c
> +++ b/drivers/clocksource/timer-owl.c
> @@ -135,8 +135,10 @@ static int __init owl_timer_init(struct device_node *node)
>  	}
> 
>  	clk = of_clk_get(node, 0);
> -	if (IS_ERR(clk))
> +	if (IS_ERR(clk)) {
> +		pr_err("Failed to get OF clock for clocksource\n");

No need to mention OF here. Just, "Failed to get clock for clocksource"
is good enough.

>  		return PTR_ERR(clk);
> +	}
> 
>  	rate = clk_get_rate(clk);
> 
> @@ -144,8 +146,11 @@ static int __init owl_timer_init(struct device_node *node)
>  	owl_timer_set_enabled(owl_clksrc_base, true);
> 
>  	sched_clock_register(owl_timer_sched_read, 32, rate);
> -	clocksource_mmio_init(owl_clksrc_base + OWL_Tx_VAL, node->name,
> -			      rate, 200, 32, clocksource_mmio_readl_up);
> +	ret = clocksource_mmio_init(owl_clksrc_base + OWL_Tx_VAL, node->name,
> +				    rate, 200, 32, clocksource_mmio_readl_up);
> +
> +	if (ret)
> +		pr_err("Failed to register clocksource %d\n", ret);
> 

Do you want to continue if it fails? I'd bail out.

Thanks,
Mani

>  	owl_timer_reset(owl_clkevt_base);
> 
> --
> 2.25.0
>
Andreas Färber Feb. 14, 2020, 2:46 p.m. UTC | #2
Hi,

Am Freitag, den 14.02.2020, 19:47 +0530 schrieb Manivannan Sadhasivam:
> On Fri, Feb 14, 2020 at 03:49:23AM -0300, Matheus Castello wrote:
> > Adding error messages, in case of not having a defined clock
> > property
> > and in case of an error in clocksource_mmio_init, which may not be
> > fatal, so just adding a pr_err to notify that it failed.
> > 
> > Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> > ---
> > 
> > Tested on my Caninos Labrador s500 based board. If the clock
> > property is not
> > set this message would help debug:
> > 
> > ...
> > [    0.000000] Failed to get OF clock for clocksource
> > [    0.000000] Failed to initialize '/soc/timer@b0168000': -2
> > [    0.000000] timer_probe: no matching timers found
> > ...
> > 
> >  drivers/clocksource/timer-owl.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clocksource/timer-owl.c
> > b/drivers/clocksource/timer-owl.c
> > index 900fe736145d..f53596f9e86c 100644
> > --- a/drivers/clocksource/timer-owl.c
> > +++ b/drivers/clocksource/timer-owl.c
> > @@ -135,8 +135,10 @@ static int __init owl_timer_init(struct
> > device_node *node)
> >  	}
> > 
> >  	clk = of_clk_get(node, 0);
> > -	if (IS_ERR(clk))
> > +	if (IS_ERR(clk)) {
> > +		pr_err("Failed to get OF clock for clocksource\n");
> 
> No need to mention OF here. Just, "Failed to get clock for
> clocksource"
> is good enough.

We should be consistent then and output PTR_ERR(clk), too.

i.e., "Failed to get clock for clocksource (%d)\n"

> 
> >  		return PTR_ERR(clk);
> > +	}
> > 
> >  	rate = clk_get_rate(clk);
> > 
> > @@ -144,8 +146,11 @@ static int __init owl_timer_init(struct
> > device_node *node)
> >  	owl_timer_set_enabled(owl_clksrc_base, true);
> > 
> >  	sched_clock_register(owl_timer_sched_read, 32, rate);
> > -	clocksource_mmio_init(owl_clksrc_base + OWL_Tx_VAL, node->name,
> > -			      rate, 200, 32,
> > clocksource_mmio_readl_up);
> > +	ret = clocksource_mmio_init(owl_clksrc_base + OWL_Tx_VAL, node-
> > >name,
> > +				    rate, 200, 32,
> > clocksource_mmio_readl_up);
> > +
> > +	if (ret)
> > +		pr_err("Failed to register clocksource %d\n", ret);

It's not a numeric clocksource, so for humans please use "... (%d)\n".

> > 
> 
> Do you want to continue if it fails? I'd bail out.

Agreed, but I'd suggest to check under which circumstances this can
actually fail and how other drivers handle it, given that it was not
checked before. Was this missed during original review, or is the
return value new?

Cheers,
Andreas

Patch
diff mbox series

diff --git a/drivers/clocksource/timer-owl.c b/drivers/clocksource/timer-owl.c
index 900fe736145d..f53596f9e86c 100644
--- a/drivers/clocksource/timer-owl.c
+++ b/drivers/clocksource/timer-owl.c
@@ -135,8 +135,10 @@  static int __init owl_timer_init(struct device_node *node)
 	}

 	clk = of_clk_get(node, 0);
-	if (IS_ERR(clk))
+	if (IS_ERR(clk)) {
+		pr_err("Failed to get OF clock for clocksource\n");
 		return PTR_ERR(clk);
+	}

 	rate = clk_get_rate(clk);

@@ -144,8 +146,11 @@  static int __init owl_timer_init(struct device_node *node)
 	owl_timer_set_enabled(owl_clksrc_base, true);

 	sched_clock_register(owl_timer_sched_read, 32, rate);
-	clocksource_mmio_init(owl_clksrc_base + OWL_Tx_VAL, node->name,
-			      rate, 200, 32, clocksource_mmio_readl_up);
+	ret = clocksource_mmio_init(owl_clksrc_base + OWL_Tx_VAL, node->name,
+				    rate, 200, 32, clocksource_mmio_readl_up);
+
+	if (ret)
+		pr_err("Failed to register clocksource %d\n", ret);

 	owl_timer_reset(owl_clkevt_base);