Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] clocksource: dw_apb_timer_of: return EPROBE_DEFER if no
 clock available
To:     Dinh Nguyen <dinguyen@kernel.org>, linux-kernel@vger.kernel.org
Cc:     tglx@linutronix.de, p.zabel@pengutronix.de,
        Jisheng.Zhang@synaptics.com, arnd@arndb.de
References: <20201119121225.26536-1-dinguyen@kernel.org>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
Message-ID: <0b648a7d-98f7-dc54-99b0-4331379a65ba@linaro.org>
Date:   Thu, 3 Dec 2020 11:09:48 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201119121225.26536-1-dinguyen@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


Hi Dinh,

On 19/11/2020 13:12, Dinh Nguyen wrote:
> commit ("b0fc70ce1f02 arm64: berlin: Select DW_APB_TIMER_OF") added the
> support for the dw_apb_timer into the arm64 defconfig. However, for some
> platforms like the Intel Stratix10 and Agilex, the clock manager doesn't
> get probed until after the timer driver is probed. Thus, the driver hits
> the panic "No clock nor clock-frequency property for %" because it cannot
> properly get the clock.
> 
> This patch adds support for EPROBE_DEFER so the kernel can come back to
> finish probing this timer driver after the clock driver is probed.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>

A few comments below.

> ---
>  drivers/clocksource/dw_apb_timer_of.c | 86 ++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
> index ab3ddebe8344..a8ce980c5146 100644
> --- a/drivers/clocksource/dw_apb_timer_of.c
> +++ b/drivers/clocksource/dw_apb_timer_of.c
> @@ -14,7 +14,7 @@
>  #include <linux/reset.h>
>  #include <linux/sched_clock.h>
>  
> -static void __init timer_get_base_and_rate(struct device_node *np,
> +static int __init timer_get_base_and_rate(struct device_node *np,
>  				    void __iomem **base, u32 *rate)
>  {
>  	struct clk *timer_clk;
> @@ -47,65 +47,77 @@ static void __init timer_get_base_and_rate(struct device_node *np,
>  				np);
>  
>  	timer_clk = of_clk_get_by_name(np, "timer");
> -	if (IS_ERR(timer_clk))
> -		goto try_clock_freq;
> +	if (IS_ERR(timer_clk)) {
> +		if (PTR_ERR(timer_clk) != -EPROBE_DEFER) {
> +			pr_err("Failed to get clock for %pOF\n", np);
> +			goto try_clock_freq;
> +		}
> +		return PTR_ERR(timer_clk);
> +	}

May be massage the changes by moving the of-rate check first

 	if (!of_property_read_u32(np, "clock-freq", rate) &&
  	    !of_property_read_u32(np, "clock-frequency", rate))
		return 0;

	timer_clk = of_clk_get_by_name(np, "timer");

	/*
	 * Whatever the result, we return
	 */
	ret = PTR_ERR(timer_clk);
	if (ret)
		return ret;

	ret = clk_prepare_enable(timer_clk);
	if (ret)
		return ret;

	*rate = clk_get_rate(timer_clk);
	if (!(*rate))
		return -EINVAL;

	return 0;

>  
>  	if (!clk_prepare_enable(timer_clk)) {
>  		*rate = clk_get_rate(timer_clk);
> -		return;
> +		return 0;
>  	}
>  
>  try_clock_freq:
>  	if (of_property_read_u32(np, "clock-freq", rate) &&
>  	    of_property_read_u32(np, "clock-frequency", rate))
>  		panic("No clock nor clock-frequency property for %pOFn", np);
>
> +	return 0;
>  }
>  
> -static void __init add_clockevent(struct device_node *event_timer)
> +static int __init add_clockevent(struct device_node *event_timer)
>  {
>  	void __iomem *iobase;
>  	struct dw_apb_clock_event_device *ced;
>  	u32 irq, rate;
> +	int ret = 0;
>  
>  	irq = irq_of_parse_and_map(event_timer, 0);
>  	if (irq == 0)
>  		panic("No IRQ for clock event timer");
>  
> -	timer_get_base_and_rate(event_timer, &iobase, &rate);
> -
> -	ced = dw_apb_clockevent_init(-1, event_timer->name, 300, iobase, irq,
> +	ret = timer_get_base_and_rate(event_timer, &iobase, &rate);
> +	if (ret == 0) {
> +		ced = dw_apb_clockevent_init(-1, event_timer->name, 300, iobase, irq,
>  				     rate);
> -	if (!ced)
> -		panic("Unable to initialise clockevent device");
> +		if (!ced)
> +			panic("Unable to initialise clockevent device");

	ret = timer_get_base_and_rate(event_timer, &iobase, &rate);
	if (ret)
		return ret;

	ced = dw_apb_clockevent_init(-1, event_timer->name, 300,
				iobase, irq, rate);
	if (!ced)
		return -EINVAL;

	dw_apb_clockevent_register(ced);

	return 0;

> -	dw_apb_clockevent_register(ced);
> +		dw_apb_clockevent_register(ced);
> +	}
> +	return ret;
>  }
>  
>  static void __iomem *sched_io_base;
>  static u32 sched_rate;
>  
> -static void __init add_clocksource(struct device_node *source_timer)
> +static int __init add_clocksource(struct device_node *source_timer)
>  {
>  	void __iomem *iobase;
>  	struct dw_apb_clocksource *cs;
>  	u32 rate;
> -
> -	timer_get_base_and_rate(source_timer, &iobase, &rate);
> -
> -	cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate);
> -	if (!cs)
> -		panic("Unable to initialise clocksource device");
> -
> -	dw_apb_clocksource_start(cs);
> -	dw_apb_clocksource_register(cs);
> -
> -	/*
> -	 * Fallback to use the clocksource as sched_clock if no separate
> -	 * timer is found. sched_io_base then points to the current_value
> -	 * register of the clocksource timer.
> -	 */
> -	sched_io_base = iobase + 0x04;
> -	sched_rate = rate;
> +	int ret;
> +
> +	ret = timer_get_base_and_rate(source_timer, &iobase, &rate);
> +	if (ret == 0) {
> +		cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate);
> +		if (!cs)
> +			panic("Unable to initialise clocksource device");
> +
> +		dw_apb_clocksource_start(cs);
> +		dw_apb_clocksource_register(cs);
> +
> +		/*
> +		 * Fallback to use the clocksource as sched_clock if no separate
> +		 * timer is found. sched_io_base then points to the current_value
> +		 * register of the clocksource timer.
> +		 */
> +		sched_io_base = iobase + 0x04;
> +		sched_rate = rate;
> +	}
> +	return ret;

Same suggestion as above

>  }
>  
>  static u64 notrace read_sched_clock(void)
> @@ -146,25 +158,29 @@ static struct delay_timer dw_apb_delay_timer = {
>  static int num_called;
>  static int __init dw_apb_timer_init(struct device_node *timer)
>  {
> +	int ret = 0;
> +
>  	switch (num_called) {
>  	case 1:
>  		pr_debug("%s: found clocksource timer\n", __func__);
> -		add_clocksource(timer);
> -		init_sched_clock();
> +		ret = add_clocksource(timer);
> +		if (ret == 0) {
> +			init_sched_clock();

		ret = add_clocksource(timer);
		if (ret)
			return ret;

		init_sched_clock();
		dw_apb_delay_timer.freq = sched_rate;
		register_current_timer_delay(&dw_apb_delay_timer);

>  #ifdef CONFIG_ARM
> -		dw_apb_delay_timer.freq = sched_rate;
> -		register_current_timer_delay(&dw_apb_delay_timer);
> +			dw_apb_delay_timer.freq = sched_rate;
> +			register_current_timer_delay(&dw_apb_delay_timer);
>  #endif
> +		}
>  		break;
>  	default:
>  		pr_debug("%s: found clockevent timer\n", __func__);
> -		add_clockevent(timer);
> +		ret = add_clockevent(timer);

		ret = add_clockevent(timer);
		if (ret)
			return ret;

>  		break;
>  	}
>  
>  	num_called++;
>  
> -	return 0;
> +	return ret;

	return 0;

>  }
>  TIMER_OF_DECLARE(pc3x2_timer, "picochip,pc3x2-timer", dw_apb_timer_init);
>  TIMER_OF_DECLARE(apb_timer_osc, "snps,dw-apb-timer-osc", dw_apb_timer_init);
> 


-- 
<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