Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On Tue, Apr 29, 2025 at 04:23:22PM +0200, Thierry Reding wrote:
> On Tue, Apr 29, 2025 at 03:19:25PM +0200, Daniel Lezcano wrote:
> > On 29/04/2025 11:15, Jon Hunter wrote:
> > > Hi Daniel,
> > > 
> > > On 29/04/2025 09:59, Daniel Lezcano wrote:
> > > > On Mon, Apr 21, 2025 at 06:08:19PM +0800, Robert Lin wrote:
> > > > > From: Pohsun Su <pohsuns@nvidia.com>
> > > > > 
> > > > > This change adds support for WDIOC_GETTIMELEFT so userspace
> > > > > programs can get the number of seconds before system reset by
> > > > > the watchdog timer via ioctl.
> > > > > 
> > > > > Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
> > > > > Signed-off-by: Robert Lin <robelin@nvidia.com>
> > > > > ---
> > > > 
> > > > Hi Robert,
> > > > 
> > > > I realize that this driver should be split in two and the watchdog
> > > > part go
> > > > under drivers/watchdog.
> > > 
> > > Are there any other examples you know of where the timer is split in
> > > this way? It is not clear to me how you propose we do this?
> > 
> > Just keep the clocksource and move the watchdog code (everything related to
> > the watchdog_ops) to a new driver under drivers/watchdog
> 
> That's a bad idea. This is all a single register space, so we can't have
> "proper" drivers (i.e. ones that exclusively request I/O memory regions)
> if we split them up.
> 
> I understand that it's nice and easy to have things split up along
> subsystem boundaries, but sometimes hardware designs just aren't that
> cleanly separated.

Yes, that's true.

The driver has a lot of watchdog code inside and I think it is
possible to move most part of it under drivers/watchdog. Perhaps by
exporting tegra186_wdt_disable() / tegra186_wdt_enable().

Anyway, I understand that is an important change and I don't want to
block this series for this reason. At the first glance, these changes
seem to be fine for me, I'll just do a last review and comment/pick
them.

> > BTW, there are three clocksources with the same rating, what is the point of
> > having them supported ?
> > 
> > Is it not the architected clocksource enough ?
> 
> The TSC clock source that this driver exposes is different from the
> architected timer. It's a SoC-wide clock that is routed to various IP
> blocks and used for timestamping events. This clocksource allows these
> events to be properly compared, etc.

I see, thanks for clarifying

  -- Daniel


-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog