Message-ID: <20200331200417.GB2950334@ulmo>
Date:   Tue, 31 Mar 2020 22:04:17 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Jon Hunter <jonathanh@...dia.com>, linux-tegra@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] clocksource: Add Tegra186 timers support

On Fri, Mar 20, 2020 at 06:23:35PM +0300, Dmitry Osipenko wrote:
> 20.03.2020 18:04, Thierry Reding пишет:
> > On Fri, Mar 20, 2020 at 05:39:01PM +0300, Dmitry Osipenko wrote:
> >> 20.03.2020 16:34, Thierry Reding пишет:
> >>> From: Thierry Reding <treding@...dia.com>
> >>>
> >>> Currently this only supports a single watchdog, which uses a timer in
> >>> the background for countdown. Eventually the timers could be used for
> >>> various time-keeping tasks, but by default the architected timer will
> >>> already provide that functionality.
> >>>
> >>> Signed-off-by: Thierry Reding <treding@...dia.com>
> >>> ---
> >>>  drivers/clocksource/Kconfig          |   8 +
> >>>  drivers/clocksource/Makefile         |   1 +
> >>>  drivers/clocksource/timer-tegra186.c | 377 +++++++++++++++++++++++++++
> >>>  3 files changed, 386 insertions(+)
> >>>  create mode 100644 drivers/clocksource/timer-tegra186.c
> >> Hello Thierry,
> >>
> >> Shouldn't this driver reside in drivers/watchdog/? Like it's done in a
> >> case of the T30+ driver.
> > 
> > The hardware block that this binds to is primarily a time-keeping block
> > that just so happens to also implement a watchdog. Moving this to
> > drivers/watchdog would put us into an odd situation if we ever added
> > code to also implement the time-keeping bits for this hardware.
> > 
> > I also think that the way this is done on Tegra30 was a bad choice. The
> > problem is that we now have two drivers (tegra_wdt.c and tegra-timer.c)
> > that both access the same region of memory. This seems to be relatively
> > safe to do on those chips because there's no overlap between the timer
> > and the watchdog interfaces, but on Tegra186 and later the watchdog is
> > actually using one of the timers, so we'd have to be extra careful how
> > to coordinate between the two. It seems much easier to do that by having
> > everything in the same driver and have that register multiple devices in
> > the system.
> 
> Sounds like a watchdog on Tegra20, where one of the timer is shared with
> a watchdog function and there are no other free timers. Well, yes, it's
> not nice.
> 
> But, will you really ever need an additional clocksource on T186?

Actually there are a couple of interesting clocksources that this IP
block exposes. It contains both a microsecond clock that might come in
useful because it is used as a reference by some other blocks that work
with microsecond counters (some hardware sequencers have this). Another
one is the OSC, which is the system's main oscillator that most clocks
are derived from.

Perhaps the most useful source from a software point of view is the TSC.
It's a timestamp counter that can also be used as a reference for HW
timestamping of certain system events, which is something that we want
to upstream eventually. Having the TSC exposed as a clocksource can be
interesting because it allows us to correlate these hardware timestamps
with code path execution.

I've implemented the three clocksources above for v2, which makes this a
bit more of an actual clocksource driver that additionally provides a
watchdog.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists