Intel MID vs. Intel SCU watchdog

Intel MID vs. Intel SCU watchdog

[Guenter Roeck]

Hi,

with the recent submission of an Intel MID watchdog driver, we now have

config INTEL_SCU_WATCHDOG
        bool "Intel SCU Watchdog for Mobile Platforms"
	depends on X86_INTEL_MID
	---help---
	  Hardware driver for the watchdog time built into the Intel SCU
	  for Intel Mobile Platforms.


config INTEL_MID_WATCHDOG
        tristate "Intel MID Watchdog Timer"
	depends on X86_INTEL_MID
	--help--
	  Watchdog timer driver built into the Intel SCU for Intel MID Platforms.

	  This driver currently supports only the watchdog evolution
	  implementation in SCU, available for Merrifield generation.

Furthermore, in the SCU watchdog driver initialization code, we have

	...
	 * If it isn't an intel MID device then it doesn't have this watchdog
         */
         if (!intel_mid_identify_cpu())
                  return -ENODEV;

I must admit I find this very confusing. Both watchdogs have the same
dependencies. The MID watchdog driver only instantiates for Tangier,
while the SCU watchdog driver instantiates for all MID CPUs (specfically
including Tangier, but also Penwell and Cloverview and, from earlier
commit logs, Moorsetown).

I understand I was told earlier that the supported devices would be
"sufficiently different" to warrant separate drivers, but this doesn't
really make sense to me, at least not without further explanation.

Can someone please clarify which driver is supposed to work on which device,
and why there are now two watchdog drivers for Tangier (or at least two
watchdog drivers which instantiate on Tangier) ? Does the SCU driver work
on Tangier, or does it not ? If not, why is there no code in the SCU driver
which would cause it to fail to instantiate on Tangier, and why is this
limitation not mentioned in Kconfig ? If yes, why do we need separate drivers
in the first place ?

Thanks,
Guenter

Re: Intel MID vs. Intel SCU watchdog

[David Cohen]

On Mon, Apr 21, 2014 at 11:16:53AM -0700, Guenter Roeck wrote:
> Hi,
> 
> with the recent submission of an Intel MID watchdog driver, we now have
> 
> config INTEL_SCU_WATCHDOG
>         bool "Intel SCU Watchdog for Mobile Platforms"
> 	depends on X86_INTEL_MID
> 	---help---
> 	  Hardware driver for the watchdog time built into the Intel SCU
> 	  for Intel Mobile Platforms.
> 
> 
> config INTEL_MID_WATCHDOG
>         tristate "Intel MID Watchdog Timer"
> 	depends on X86_INTEL_MID
> 	--help--
> 	  Watchdog timer driver built into the Intel SCU for Intel MID Platforms.
> 
> 	  This driver currently supports only the watchdog evolution
> 	  implementation in SCU, available for Merrifield generation.
> 
> Furthermore, in the SCU watchdog driver initialization code, we have
> 
> 	...
> 	 * If it isn't an intel MID device then it doesn't have this watchdog
>          */
>          if (!intel_mid_identify_cpu())
>                   return -ENODEV;
> 
> I must admit I find this very confusing. Both watchdogs have the same
> dependencies. The MID watchdog driver only instantiates for Tangier,
> while the SCU watchdog driver instantiates for all MID CPUs (specfically
> including Tangier, but also Penwell and Cloverview and, from earlier
> commit logs, Moorsetown).
> 
> I understand I was told earlier that the supported devices would be
> "sufficiently different" to warrant separate drivers, but this doesn't
> really make sense to me, at least not without further explanation.
> 
> Can someone please clarify which driver is supposed to work on which device,
> and why there are now two watchdog drivers for Tangier (or at least two
> watchdog drivers which instantiate on Tangier) ? Does the SCU driver work
> on Tangier, or does it not ? If not, why is there no code in the SCU driver
> which would cause it to fail to instantiate on Tangier, and why is this
> limitation not mentioned in Kconfig ? If yes, why do we need separate drivers
> in the first place ?

Let me try to give extra info:

INTEL_SCU_WATCHDOG supports Penwell and Cloverview. But it is using an
obsolete interface and would need too much if's to support Tangier (and
newer ones). And it is mostly unsupported nowadays.

Internally we had a new watchdog for Tangier (and newer platforms)
called INTEL_SCU_WATCHDOG_EVO. Somebody else tried to upstream it but
got nacked due to obsolete interface too. I started to rewrite this
watchdog to comply with community comments [1].

While rewriting the _EVO one, I felt it would be better to not have 2
wdt drivers for Intel MID, but only one. This new INTEL_MID_WATCHDOG
supports only Tangier now but I am trying to make it generic enough to
bring the legacy platforms too. Then we can get rid of INTEL_SCU_WATCHDOG.

Does that sound feasible? Maybe I need to write comments to
INTEL_SCU_WATCHDOG's Kconfig entry about its limitation.

BR, David

[1] https://lkml.org/lkml/2014/1/2/505

> 
> Thanks,
> Guenter

Re: Intel MID vs. Intel SCU watchdog

[Guenter Roeck]

On Mon, Apr 21, 2014 at 12:51:27PM -0700, David Cohen wrote:
> On Mon, Apr 21, 2014 at 11:16:53AM -0700, Guenter Roeck wrote:
> > Hi,
> > 
> > with the recent submission of an Intel MID watchdog driver, we now have
> > 
> > config INTEL_SCU_WATCHDOG
> >         bool "Intel SCU Watchdog for Mobile Platforms"
> > 	depends on X86_INTEL_MID
> > 	---help---
> > 	  Hardware driver for the watchdog time built into the Intel SCU
> > 	  for Intel Mobile Platforms.
> > 
> > 
> > config INTEL_MID_WATCHDOG
> >         tristate "Intel MID Watchdog Timer"
> > 	depends on X86_INTEL_MID
> > 	--help--
> > 	  Watchdog timer driver built into the Intel SCU for Intel MID Platforms.
> > 
> > 	  This driver currently supports only the watchdog evolution
> > 	  implementation in SCU, available for Merrifield generation.
> > 
> > Furthermore, in the SCU watchdog driver initialization code, we have
> > 
> > 	...
> > 	 * If it isn't an intel MID device then it doesn't have this watchdog
> >          */
> >          if (!intel_mid_identify_cpu())
> >                   return -ENODEV;
> > 
> > I must admit I find this very confusing. Both watchdogs have the same
> > dependencies. The MID watchdog driver only instantiates for Tangier,
> > while the SCU watchdog driver instantiates for all MID CPUs (specfically
> > including Tangier, but also Penwell and Cloverview and, from earlier
> > commit logs, Moorsetown).
> > 
> > I understand I was told earlier that the supported devices would be
> > "sufficiently different" to warrant separate drivers, but this doesn't
> > really make sense to me, at least not without further explanation.
> > 
> > Can someone please clarify which driver is supposed to work on which device,
> > and why there are now two watchdog drivers for Tangier (or at least two
> > watchdog drivers which instantiate on Tangier) ? Does the SCU driver work
> > on Tangier, or does it not ? If not, why is there no code in the SCU driver
> > which would cause it to fail to instantiate on Tangier, and why is this
> > limitation not mentioned in Kconfig ? If yes, why do we need separate drivers
> > in the first place ?
> 
> Let me try to give extra info:
> 
> INTEL_SCU_WATCHDOG supports Penwell and Cloverview. But it is using an
> obsolete interface and would need too much if's to support Tangier (and
> newer ones). And it is mostly unsupported nowadays.
> 
> Internally we had a new watchdog for Tangier (and newer platforms)
> called INTEL_SCU_WATCHDOG_EVO. Somebody else tried to upstream it but
> got nacked due to obsolete interface too. I started to rewrite this
> watchdog to comply with community comments [1].
> 
> While rewriting the _EVO one, I felt it would be better to not have 2
> wdt drivers for Intel MID, but only one. This new INTEL_MID_WATCHDOG
> supports only Tangier now but I am trying to make it generic enough to
> bring the legacy platforms too. Then we can get rid of INTEL_SCU_WATCHDOG.
> 
> Does that sound feasible? Maybe I need to write comments to
> INTEL_SCU_WATCHDOG's Kconfig entry about its limitation.
> 
Yes, I think that would make sense. I also think the SCU watchdog
driver should fail to load if it is instantiated on Tangier,
if that is not supported. Or, rather, it should only instantiate on 
Penwell and Cloverview if those are the only supported platforms
(but what about Moorestown ?).

Can you submit a patch to do that ?

Thanks,
Guenter

Re: Intel MID vs. Intel SCU watchdog

[David Cohen]

On Mon, Apr 21, 2014 at 01:05:08PM -0700, Guenter Roeck wrote:
> On Mon, Apr 21, 2014 at 12:51:27PM -0700, David Cohen wrote:
> > On Mon, Apr 21, 2014 at 11:16:53AM -0700, Guenter Roeck wrote:
> > > Hi,
> > > 
> > > with the recent submission of an Intel MID watchdog driver, we now have
> > > 
> > > config INTEL_SCU_WATCHDOG
> > >         bool "Intel SCU Watchdog for Mobile Platforms"
> > > 	depends on X86_INTEL_MID
> > > 	---help---
> > > 	  Hardware driver for the watchdog time built into the Intel SCU
> > > 	  for Intel Mobile Platforms.
> > > 
> > > 
> > > config INTEL_MID_WATCHDOG
> > >         tristate "Intel MID Watchdog Timer"
> > > 	depends on X86_INTEL_MID
> > > 	--help--
> > > 	  Watchdog timer driver built into the Intel SCU for Intel MID Platforms.
> > > 
> > > 	  This driver currently supports only the watchdog evolution
> > > 	  implementation in SCU, available for Merrifield generation.
> > > 
> > > Furthermore, in the SCU watchdog driver initialization code, we have
> > > 
> > > 	...
> > > 	 * If it isn't an intel MID device then it doesn't have this watchdog
> > >          */
> > >          if (!intel_mid_identify_cpu())
> > >                   return -ENODEV;
> > > 
> > > I must admit I find this very confusing. Both watchdogs have the same
> > > dependencies. The MID watchdog driver only instantiates for Tangier,
> > > while the SCU watchdog driver instantiates for all MID CPUs (specfically
> > > including Tangier, but also Penwell and Cloverview and, from earlier
> > > commit logs, Moorsetown).
> > > 
> > > I understand I was told earlier that the supported devices would be
> > > "sufficiently different" to warrant separate drivers, but this doesn't
> > > really make sense to me, at least not without further explanation.
> > > 
> > > Can someone please clarify which driver is supposed to work on which device,
> > > and why there are now two watchdog drivers for Tangier (or at least two
> > > watchdog drivers which instantiate on Tangier) ? Does the SCU driver work
> > > on Tangier, or does it not ? If not, why is there no code in the SCU driver
> > > which would cause it to fail to instantiate on Tangier, and why is this
> > > limitation not mentioned in Kconfig ? If yes, why do we need separate drivers
> > > in the first place ?
> > 
> > Let me try to give extra info:
> > 
> > INTEL_SCU_WATCHDOG supports Penwell and Cloverview. But it is using an
> > obsolete interface and would need too much if's to support Tangier (and
> > newer ones). And it is mostly unsupported nowadays.
> > 
> > Internally we had a new watchdog for Tangier (and newer platforms)
> > called INTEL_SCU_WATCHDOG_EVO. Somebody else tried to upstream it but
> > got nacked due to obsolete interface too. I started to rewrite this
> > watchdog to comply with community comments [1].
> > 
> > While rewriting the _EVO one, I felt it would be better to not have 2
> > wdt drivers for Intel MID, but only one. This new INTEL_MID_WATCHDOG
> > supports only Tangier now but I am trying to make it generic enough to
> > bring the legacy platforms too. Then we can get rid of INTEL_SCU_WATCHDOG.
> > 
> > Does that sound feasible? Maybe I need to write comments to
> > INTEL_SCU_WATCHDOG's Kconfig entry about its limitation.
> > 
> Yes, I think that would make sense. I also think the SCU watchdog
> driver should fail to load if it is instantiated on Tangier,
> if that is not supported. Or, rather, it should only instantiate on 
> Penwell and Cloverview if those are the only supported platforms
> (but what about Moorestown ?).
> 
> Can you submit a patch to do that ?

I am about to submit a patch a bit different.
I'm extending the arch/x86/platform/intel-mid/device_libs/platform_wdt.c
code to penwell and cloverview. INTEL_SCU_WATCHDOG will no longer probe
itself, but register a platform driver instead. Then the platform code
will register the correct device (SCU or MID).

Since watchdog is not part of SFI tables, this is the option I found to
avoid the self-probe approach from Intel MID wdt drivers.

BR, David

> 
> Thanks,
> Guenter