Re: [PATCH] clocksource/drivers/timer-ti-dm: Fix suspend and resume for am3 and am4

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 13/07/2020 19:26, Tony Lindgren wrote:
> Carlos Hernandez <ceh@ti.com> reported that we now have a suspend and
> resume regresssion on am3 and am4 compared to the earlier kernels. While
> suspend and resume works with v5.8-rc3, we now get errors with rtcwake:
> 
> pm33xx pm33xx: PM: Could not transition all powerdomains to target state
> ...
> rtcwake: write error
> 
> This is because we now fail to idle the system timer clocks that the
> idle code checks and the error gets propagated to the rtcwake.
> 
> Turns out there are several issues that need to be fixed:
> 
> 1. Ignore no-idle and no-reset configured timers for the ti-sysc
>     interconnect target driver as otherwise it will keep the system timer
>     clocks enabled
> 
> 2. Toggle the system timer functional clock for suspend for am3 and am4
>     (but not for clocksource on am3)
> 
> 3. Only reconfigure type1 timers in dmtimer_systimer_disable()
> 
> 4. Use of_machine_is_compatible() instead of of_device_is_compatible()
>     for checking the SoC type
> 
> Fixes: 52762fbd1c47 ("clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support")
> Reported-by: Carlos Hernandez <ceh@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/bus/ti-sysc.c                      | 22 +++++++++++
>   drivers/clocksource/timer-ti-dm-systimer.c | 46 +++++++++++++++++-----
>   2 files changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
> --- a/drivers/bus/ti-sysc.c
> +++ b/drivers/bus/ti-sysc.c
> @@ -2877,6 +2877,24 @@ static int sysc_check_disabled_devices(struct sysc *ddata)
>   	return error;
>   }
>   
> +/*
> + * Ignore timers tagged with no-reset and no-idle. These are likely in use,
> + * for example by drivers/clocksource/timer-ti-dm-systimer.c. If more checks
> + * are needed, we could also look at the timer register configuration.
> + */
> +static int sysc_check_active_timer(struct sysc *ddata)
> +{
> +	if (ddata->cap->type != TI_SYSC_OMAP2_TIMER &&
> +	    ddata->cap->type != TI_SYSC_OMAP4_TIMER)
> +		return 0;
> +
> +	if ((ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) &&
> +	    (ddata->cfg.quirks & SYSC_QUIRK_NO_IDLE))
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
>   static const struct of_device_id sysc_match_table[] = {
>   	{ .compatible = "simple-bus", },
>   	{ /* sentinel */ },
> @@ -2933,6 +2951,10 @@ static int sysc_probe(struct platform_device *pdev)
>   	if (error)
>   		return error;
>   
> +	error = sysc_check_active_timer(ddata);
> +	if (error)
> +		return error;
> +
>   	error = sysc_get_clocks(ddata);
>   	if (error)
>   		return error;
> diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
> --- a/drivers/clocksource/timer-ti-dm-systimer.c
> +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> @@ -19,7 +19,7 @@
>   /* For type1, set SYSC_OMAP2_CLOCKACTIVITY for fck off on idle, l4 clock on */
>   #define DMTIMER_TYPE1_ENABLE	((1 << 9) | (SYSC_IDLE_SMART << 3) | \
>   				 SYSC_OMAP2_ENAWAKEUP | SYSC_OMAP2_AUTOIDLE)
> -
> +#define DMTIMER_TYPE1_DISABLE	(SYSC_OMAP2_SOFTRESET | SYSC_OMAP2_AUTOIDLE)
>   #define DMTIMER_TYPE2_ENABLE	(SYSC_IDLE_SMART_WKUP << 2)
>   #define DMTIMER_RESET_WAIT	100000
>   
> @@ -44,6 +44,8 @@ struct dmtimer_systimer {
>   	u8 ctrl;
>   	u8 wakeup;
>   	u8 ifctrl;
> +	struct clk *fck;
> +	struct clk *ick;
>   	unsigned long rate;
>   };
>   
> @@ -298,16 +300,20 @@ static void __init dmtimer_systimer_select_best(void)
>   }
>   
>   /* Interface clocks are only available on some SoCs variants */
> -static int __init dmtimer_systimer_init_clock(struct device_node *np,
> +static int __init dmtimer_systimer_init_clock(struct dmtimer_systimer *t,
> +					      struct device_node *np,
>   					      const char *name,
>   					      unsigned long *rate)
>   {
>   	struct clk *clock;
>   	unsigned long r;
> +	bool is_ick = false;
>   	int error;
>   
> +	is_ick = !strncmp(name, "ick", 3);
> +
>   	clock = of_clk_get_by_name(np, name);
> -	if ((PTR_ERR(clock) == -EINVAL) && !strncmp(name, "ick", 3))
> +	if ((PTR_ERR(clock) == -EINVAL) && is_ick)
>   		return 0;
>   	else if (IS_ERR(clock))
>   		return PTR_ERR(clock);
> @@ -320,6 +326,11 @@ static int __init dmtimer_systimer_init_clock(struct device_node *np,
>   	if (!r)
>   		return -ENODEV;
>   
> +	if (is_ick)
> +		t->ick = clock;
> +	else
> +		t->fck = clock;
> +
>   	*rate = r;
>   
>   	return 0;
> @@ -339,7 +350,10 @@ static void dmtimer_systimer_enable(struct dmtimer_systimer *t)
>   
>   static void dmtimer_systimer_disable(struct dmtimer_systimer *t)
>   {
> -	writel_relaxed(0, t->base + t->sysc);
> +	if (!dmtimer_systimer_revision1(t))
> +		return;
> +
> +	writel_relaxed(DMTIMER_TYPE1_DISABLE, t->base + t->sysc);
>   }
>   
>   static int __init dmtimer_systimer_setup(struct device_node *np,
> @@ -366,13 +380,13 @@ static int __init dmtimer_systimer_setup(struct device_node *np,
>   		pr_err("%s: clock source init failed: %i\n", __func__, error);
>   
>   	/* For ti-sysc, we have timer clocks at the parent module level */
> -	error = dmtimer_systimer_init_clock(np->parent, "fck", &rate);
> +	error = dmtimer_systimer_init_clock(t, np->parent, "fck", &rate);
>   	if (error)
>   		goto err_unmap;
>   
>   	t->rate = rate;
>   
> -	error = dmtimer_systimer_init_clock(np->parent, "ick", &rate);
> +	error = dmtimer_systimer_init_clock(t, np->parent, "ick", &rate);
>   	if (error)
>   		goto err_unmap;
>   
> @@ -496,12 +510,18 @@ static void omap_clockevent_idle(struct clock_event_device *evt)
>   	struct dmtimer_systimer *t = &clkevt->t;
>   
>   	dmtimer_systimer_disable(t);
> +	clk_disable(t->fck);
>   }
>   
>   static void omap_clockevent_unidle(struct clock_event_device *evt)
>   {
>   	struct dmtimer_clockevent *clkevt = to_dmtimer_clockevent(evt);
>   	struct dmtimer_systimer *t = &clkevt->t;
> +	int error;
> +
> +	error = clk_enable(t->fck);
> +	if (error)
> +		pr_err("could not enable timer fck on resume: %i\n", error);
>   
>   	dmtimer_systimer_enable(t);
>   	writel_relaxed(OMAP_TIMER_INT_OVERFLOW, t->base + t->irq_ena);
> @@ -570,8 +590,8 @@ static int __init dmtimer_clockevent_init(struct device_node *np)
>   					3, /* Timer internal resynch latency */
>   					0xffffffff);
>   
> -	if (of_device_is_compatible(np, "ti,am33xx") ||
> -	    of_device_is_compatible(np, "ti,am43")) {
> +	if (of_machine_is_compatible("ti,am33xx") ||
> +	    of_machine_is_compatible("ti,am43")) {
>   		dev->suspend = omap_clockevent_idle;
>   		dev->resume = omap_clockevent_unidle;
>   	}
> @@ -616,12 +636,18 @@ static void dmtimer_clocksource_suspend(struct clocksource *cs)
>   
>   	clksrc->loadval = readl_relaxed(t->base + t->counter);
>   	dmtimer_systimer_disable(t);
> +	clk_disable(t->fck);
>   }
>   
>   static void dmtimer_clocksource_resume(struct clocksource *cs)
>   {
>   	struct dmtimer_clocksource *clksrc = to_dmtimer_clocksource(cs);
>   	struct dmtimer_systimer *t = &clksrc->t;
> +	int error;
> +
> +	error = clk_enable(t->fck);
> +	if (error)
> +		pr_err("could not enable timer fck on resume: %i\n", error);
>   
>   	dmtimer_systimer_enable(t);
>   	writel_relaxed(clksrc->loadval, t->base + t->counter);
> @@ -653,8 +679,8 @@ static int __init dmtimer_clocksource_init(struct device_node *np)
>   	dev->mask = CLOCKSOURCE_MASK(32);
>   	dev->flags = CLOCK_SOURCE_IS_CONTINUOUS;
>   
> -	if (of_device_is_compatible(np, "ti,am33xx") ||
> -	    of_device_is_compatible(np, "ti,am43")) {
> +	/* Unlike for clockevent, legacy code sets suspend only for am4 */
> +	if (of_machine_is_compatible("ti,am43")) {
>   		dev->suspend = dmtimer_clocksource_suspend;
>   		dev->resume = dmtimer_clocksource_resume;
>   	}
> 

It might be better to use SOC_BUS infra here, which is available on OMAP platforms by default,
instead if DT. What do you think?

-- 
Best regards,
grygorii