| Message ID | 20200713162601.6829-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 6cfcd5563b4fadbf49ba8fa481978e5e86d30322 |
| Headers | show |
| Series | clocksource/drivers/timer-ti-dm: Fix suspend and resume for am3 and am4 | expand |
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?
* Grygorii Strashko <grygorii.strashko@ti.com> [200715 10:17]: > On 13/07/2020 19:26, Tony Lindgren wrote: > > @@ -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? Well we have time_init() run very early, and some SoCs have only external timers. So timer-ti-dm-systimer must be initialized early. And I think soc_device_match() depends on soc_bus_register() that runs later at core_initcall(). If you are thinking of something else, or we can use soc_device_match() early, sure :) But sounds like that change should be a separate patch then. The reason for why we need to check for the SoC, we have the compatible for ti,am335x-timer already in use on am3/4 and dm814x, so we can't use TIMER_OF_DECLARE alone here. Regards, Tony
On 15/07/2020 20:32, Tony Lindgren wrote: > * Grygorii Strashko <grygorii.strashko@ti.com> [200715 10:17]: >> On 13/07/2020 19:26, Tony Lindgren wrote: >>> @@ -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? > > Well we have time_init() run very early, and some SoCs have only external > timers. So timer-ti-dm-systimer must be initialized early. And I think > soc_device_match() depends on soc_bus_register() that runs later at > core_initcall(). Ah. right it's early init. I revoke my proposal then. > > If you are thinking of something else, or we can use soc_device_match() > early, sure :) But sounds like that change should be a separate patch > then. > > The reason for why we need to check for the SoC, we have the compatible > for ti,am335x-timer already in use on am3/4 and dm814x, so we can't use > TIMER_OF_DECLARE alone here.
On 13/07/2020 18: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> > --- Carlos, were you able to test this patch ?
On 7/17/20 5:34 PM, Carlos Hernandez wrote: > > > On 7/17/20 6:29 AM, Daniel Lezcano wrote: >> On 13/07/2020 18: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> >>> --- >> Carlos, were you able to test this patch ? > > Tested the patch on top of 5.8-rc5. > > cbdb2617290d (HEAD) clocksource/drivers/timer-ti-dm: Fix suspend and > resume for am3 and am4 > 11ba468877bb (tag: v5.8-rc5) Linux 5.8-rc5 > > It works on am335x-evm but fails on am437x-evm > > am3: > > [ 122.541423] PM: suspend entry (deep) > [ 122.545498] Filesystems sync: 0.000 seconds > [ 122.549711] PM: Preparing system for sleep (deep) > [ 122.564217] Freezing user space processes ... (elapsed 0.003 > seconds) done. > [ 122.575110] OOM killer disabled. > [ 122.578370] Freezing remaining freezable tasks ... (elapsed 0.001 > seconds) done. > [ 122.587604] PM: Suspending system (deep) > [ 122.591572] printk: Suspending console(s) (use no_console_suspend to > debug) > [ 122.735877] cpsw 4a100000.ethernet eth0: Link is Down > [ 122.742365] PM: suspend of devices complete after 142.546 msecs > [ 122.742397] PM: start suspend of devices complete after 143.777 msecs > [ 122.748722] PM: late suspend of devices complete after 6.257 msecs > [ 122.754662] PM: noirq suspend of devices complete after 5.632 msecs > [ 122.754689] Disabling non-boot CPUs ... > [ 122.754715] pm33xx pm33xx: PM: Successfully put all powerdomains to > target state > [ 122.754715] PM: Wakeup source RTC Alarm > [ 122.766169] ti-sysc 4a101200.target-module: OCP softreset timed out > [ 122.769222] PM: noirq resume of devices complete after 14.367 msecs > [ 122.772956] PM: early resume of devices complete after 3.428 msecs > [ 122.775749] cpsw 4a100000.ethernet: initializing cpsw version 1.12 (0) > [ 122.857132] Qualcomm Atheros AR8031/AR8033 4a101000.mdio:00: > attached PHY driver [Qualcomm Atheros AR8031/AR8033] > (mii_bus:phy_addr=4a101000.mdio:00, irq=POLL) > [ 122.874236] ti-sysc 4802a000.target-module: OCP softreset timed out > [ 122.879559] PM: Timekeeping suspended for 18.212 seconds > [ 122.994120] PM: resume of devices complete after 221.091 msecs > [ 123.101133] PM: Finishing wakeup. > [ 123.104493] OOM killer enabled. > [ 123.107657] Restarting tasks ... done. > [ 123.168294] PM: suspend exit > [ 126.005262] cpsw 4a100000.ethernet eth0: Link is Up - 1Gbps/Full - > flow control off > [ 122.742365] PM: suspend of devices complete after 142.546 msecs > > > am4: > > |TRACE LOG|Inside do_cmd:CMD=echo 1 > /sys/kernel/debug/pm_debug/enable_off_mode| > |TRACE LOG|suspend function: power_state: mem| > |TRACE LOG|suspend function: max_stime: 10| > |TRACE LOG|suspend function: max_atime: 5| > |TRACE LOG|suspend function: iterations: 2| > |TRACE LOG|suspend function: usb_remove: 0| > |TRACE LOG|suspend function: usb_module: | > |TRACE LOG|===suspend iteration 0===| > sh1577:1590898691->1590898692(1):: wakeup - 9 sec 0 msec > sh1577:1590898691->1590898692(1):: Use rtc to suspend resume, adding 10 secs to suspend time > rtcwake: wakeup from "mem" using /dev/rtc0 at Sun May 31 04:18:33 2020 > [ 106.401004] PM: suspend entry (deep) > [ 106.420151] Filesystems sync: 0.015 seconds > [ 106.424598] PM: Preparing system for sleep (deep) > [ 106.434312] Freezing user space processes ... (elapsed 0.001 seconds) done. > [ 106.442808] OOM killer disabled. > [ 106.446152] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > [ 106.454894] PM: Suspending system (deep) > [ 106.458893] printk: Suspending console(s) (use no_console_suspend to debug) > CCCCCCCC** 1196 printk messages dropped ** > [ 107.379605] [<c0573bd8>] (gic_handle_irq) from [<c0100b6c>] (__irq_svc+0x6c/0x90) > ** 41 printk messages dropped ** > [ 107.412635] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 5.8.0-rc5-00001-gcbdb2617290d #1 > ** 37 printk messages dropped ** > [ 107.429822] [<c0198358>] (handle_irq_event) from [<c019cdf8>] (handle_fasteoi_irq+0xc4/0x188) > ** 37 printk messages dropped ** > [ 107.446915] [<c09573f8>] (cpu_idle_poll) from [<c016d0e4>] (do_idle+0x7c/0x2b4) > [ 107.446946] [<c016d0e4>] (do_idle) from [<c016d5d4>] (cpu_startup_entry+0x18/0x1c) > ** 34 printk messages dropped ** > [ 107.479851] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 5.8.0-rc5-00001-gcbdb2617290d #1 > ** 42 printk messages dropped ** > [ 107.497182] Exception stack(0xc0e01f10 to 0xc0e01f58) > [ 107.497202] 1f00: 000429e8 00000000 c0e00000 00000001 > ** 34 printk messages dropped ** > [ 107.514208] ---[ end trace c18ea2591ccbc697 ]--- > [ 107.524884] usb 1-1: reset high-speed USB device number 2 using xhci-hcd > ** 34 printk messages dropped ** > [ 107.547126] [<c0111b08>] (unwind_backtrace) from [<c010ba54>] (show_stack+0x10/0x14) > [ 107.547164] [<c010ba54>] (show_stack) from [<c055ad98>] (dump_stack+0xc4/0xe0) > ** 42 printk messages dropped ** > [ 107.564441] 1e60: c0e00000 00000002 ffffb4d5 c0e01e88 c013f95c c01012fc 60000153 ffffffff > [ 107.564473] [<c0100b6c>] (__irq_svc) from [<c01012fc>] (__do_softirq+0x7c/0x390) > ** 43 printk messages dropped ** > [ 107.597112] 44000000.ocp:L3 Custom Error: MASTER DSS TARGET GPMC (Read): Data Access in User mode during Functional access > ** 34 printk messages dropped ** > [ 107.614456] [<c0138a40>] (__warn) from [<c0138e08>] (warn_slowpath_fmt+0x90/0xc0) > [ 107.614491] [<c0138e08>] (warn_slowpath_fmt) from [<c057581c>] (l3_interrupt_handler+0x250/0x370) > ** 42 printk messages dropped ** > [ 107.631781] [<c016d0e4>] (do_idle) from [<c016d5d4>] (cpu_startup_entry+0x18/0x1c) > ** 35 printk messages dropped ** > [ 107.664649] Hardware name: Generic AM43 (Flattened Device Tree) > [ 107.664711] [<c0111b08>] (unwind_backtrace) from [<c010ba54>] (show_stack+0x10/0x14) > ** 42 printk messages dropped ** > [ 107.682068] 1f20: c0e00000 c0e05234 c0ec3b9c c0e05234 00000000 c0d48a40 c0d766b0 c0e055e0 > [ 107.682089] 1f40: 00000000 c0e01f60 c0957380 c09573d0 20000153 ffffffff > ** 38 printk messages dropped ** > [ 107.720215] CPU: 0 PID: 1604 Comm: rtcwake Tainted: G W 5.8.0-rc5-00001-gcbdb2617290d #1 > ** 44 printk messages dropped ** > [ 107.734583] CPU: 0 PID: 1604 Comm: rtcwake Tainted: G W 5.8.0-rc5-00001-gcbdb2617290d #1 > [ 107.734594] Hardware name: Generic AM43 (Flattened Device Tree) > ** 39 printk messages dropped ** > [ 107.747964] ti-sysc 44e0b000.target-module: OCP softreset timed out > > -- > Carlos
* Carlos Hernandez <ceh@ti.com> [200717 21:35]: > On 7/17/20 6:29 AM, Daniel Lezcano wrote: > > On 13/07/2020 18:26, Tony Lindgren wrote: > > > 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> > > > --- > > Carlos, were you able to test this patch ? > > Tested the patch on top of 5.8-rc5. > > cbdb2617290d (HEAD) clocksource/drivers/timer-ti-dm: Fix suspend and resume > for am3 and am4 > 11ba468877bb (tag: v5.8-rc5) Linux 5.8-rc5 > > It works on am335x-evm but fails on am437x-evm Thanks for testing. > am4: > > CCCCCCCC** 1196 printk messages dropped ** The above does not look normal.. > 44000000.ocp:L3 Custom Error: MASTER DSS TARGET GPMC (Read) > ** 34 printk messages dropped ** ..but the above points to the GPMC module failing to suspend. This seems to be some other GPMC specific issue not related to the system timers. FYI, I have not seen the error above with am437x-sk-evm. But then again, the sk-evm is probably not using GPMC. Regards, Tony
On 7/20/20 8:00 PM, Tony Lindgren wrote: > * Carlos Hernandez <ceh@ti.com> [200717 21:35]: >> On 7/17/20 6:29 AM, Daniel Lezcano wrote: >>> On 13/07/2020 18:26, Tony Lindgren wrote: >>>> 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> >>>> --- >>> Carlos, were you able to test this patch ? >> >> Tested the patch on top of 5.8-rc5. >> >> cbdb2617290d (HEAD) clocksource/drivers/timer-ti-dm: Fix suspend and resume >> for am3 and am4 >> 11ba468877bb (tag: v5.8-rc5) Linux 5.8-rc5 >> >> It works on am335x-evm but fails on am437x-evm > > Thanks for testing. > >> am4: >> >> CCCCCCCC** 1196 printk messages dropped ** > > The above does not look normal.. > >> 44000000.ocp:L3 Custom Error: MASTER DSS TARGET GPMC (Read) >> ** 34 printk messages dropped ** > > ..but the above points to the GPMC module failing to suspend. > This seems to be some other GPMC specific issue not related > to the system timers. So, I guess this patch can still go into v5.8 while the AM437x GP EVM failures are root caused. Carlos, Daniel is looking for your tested by. Can you send it because it fixes the original problem report with dmtimer? Thanks, Sekhar
On 7/17/20 6:29 AM, Daniel Lezcano wrote: > On 13/07/2020 18: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> >> --- Tested-by: Carlos Hernandez <ceh@ti.com> > Carlos, were you able to test this patch ? >
On 21/07/2020 15:11, Carlos Hernandez wrote: > > On 7/17/20 6:29 AM, Daniel Lezcano wrote: >> On 13/07/2020 18: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> >>> --- > > Tested-by: Carlos Hernandez <ceh@ti.com> Thanks >> Carlos, were you able to test this patch ? >>
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; }
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(-)