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

[Dmitry Osipenko]

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

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?