Re: [PATCH 2/7] clocksource: Add Tegra186 timers support

[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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > ---
> >  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.

> > +static int __maybe_unused tegra186_timer_suspend(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused tegra186_timer_resume(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(tegra186_timer_pm_ops, tegra186_timer_suspend,
> > +			 tegra186_timer_resume);
> 
> Perhaps will be better to remove these OPS for now?

Yeah, I suppose I could remove those. Although... perhaps I should just
try and make this work properly.

Thierry

Attachment: signature.asc
Description: PGP signature