Re: omap dmtimer driver bug and a silicon issue on TI AM3358

[Matthias Welwarsky <linux-omap@welwarsky.de>]

On Dienstag, 16. Juni 2020 17:27:17 CEST Tony Lindgren wrote:
> Hi,
> 
> * Matthias Welwarsky <linux-omap@welwarsky.de> [200614 09:57]:
> > Hi,
> > 
> > while doing some timekeeping experiments with the Beaglebone Black I ran
> > across two issues I'd like advice on before submitting patches.
> > 
> > I'm feeding one of the dmtimers with a external clock (10MHz from a GPSDO)
> > to improve drift behaviour of CLOCK_REALTIME. For this, I need to set the
> > input clock multiplexer of one of the dmtimers to "tclkin_ck". I also
> > need to set up the pin multiplexing so that the external clock is
> > actually available.
> OK, so presumably you want to use this for Linux system timers then.

Actually, have to use one of the "normal" timers because I need the capture 
input. The driver implements a clocksource and a kpps interface, which makes 
getting exact 1PPS event timestamps very easy.

> > The first issue is the framework function omap_dm_timer_set_source(). Of
> > the available clock sources, none is a possible parent. But even when
> > fixing them, the clk_set_parent() will fail because timer->fclk points to
> > the "wrong" clock in the clock topology. You can only set the parent
> > clock of the "timerN_fclk" nodes, but timer->fclk points to the actual
> > hardware clock one level "down" in the topology. This clock only has one
> > possible parent, which is "timerN_fck". The work-around I use in my
> > clocksource driver is to use the clock framework directly and manually
> > retrieve the parent clock of timer->fclk, then reparent that clock to
> > "tclkin_ck". That works, but I'd prefer to fix the driver framework. But
> > I'd need a hint what would be an appropriate approach for that.

> You can specify the source clocks in the dts with assigned-clocks and
> assigned-clock-parents like commit e20ef23dd693 ("ARM: dts: Configure system
> timers for am335x") is doing for system timers starting with v5.8-rc1. That
> should just work, if not there are bugs somewhere :)

You mean, for setting the source clock of the timer to tclkin_ck through the 
dts? That would be applied during boot, right? That will probably not work for 
me as I need to switch on the external clock manually and then it'll take a 
little additional time to become stable. So I'm pretty much bound to setting 
up the clock multiplexing at runtime.

> > The second issue is more of a silicon "bug". It seems that the clock
> > multiplexer is not warm-reset sensitive but the pinmux is. In consequence,
> > when the chip is reset (watchdog or "reboot" command), the pinmux defaults
> > back to GPIO but the timer functional clock mux still points to
> > "tclkin_ck"
> > and when the kernel boots up and the dmtimer framework tries to initialize
> > the timer, it accesses a hwmod that has no functional clock and the
> > kernel receives an async external abort and dies.
> > 
> > Two possible places for a fix come to mind: u-boot could reset the clock
> > mux, or the kernel needs to do it when it boots, either in the dmtimer
> > framework or in the clock framework, maybe based on a device tree
> > attribute that specifies a default state of the clock mux.
> > 
> > I'd like to hear your take on this.
> 
> For system timers, we do not have struct device available as at least one
> usable system timer is needed very early for a clockevent. You should add
> necessary initialization based on the assigned-clocks and assigned-clock
> parents to drivers/clocksource/timer-ti-dm-systimer.c.
> 
> For non-systimer usage with normal device drivers, just configuring the
> pictrl entries for the device in the dts file can be used.

That would be too late, no? The kernel apparently dies in timer-ti-dm.c during 
probing, in particular in __omap_dm_timer_init_regs() while reading the TIDR. 
A device driver (especially when it's a module) would be probed way later.

Regards,
Matthias

> 
> Regards,
> 
> Tony