Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630078 > unrolled thread

[PATCH 0/2] DS1374 Watchdog fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] DS1374 Watchdog fixes Moritz Fischer <mdf@kernel.org> - 2017-04-25 00:10 +0200
    Re: [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck <linux@roeck-us.net> - 2017-04-25 07:10 +0200
      Re: [PATCH 0/2] DS1374 Watchdog fixes Moritz Fischer <moritz.fischer.private@gmail.com> - 2017-04-25 17:00 +0200
        Re: [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck <linux@roeck-us.net> - 2017-04-25 18:20 +0200
          Re: [PATCH 0/2] DS1374 Watchdog fixes Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-04-25 18:40 +0200
            Re: [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck <linux@roeck-us.net> - 2017-04-25 19:00 +0200
              Re: [PATCH 0/2] DS1374 Watchdog fixes Moritz Fischer <mdf@kernel.org> - 2017-04-25 22:00 +0200
                Re: [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck <linux@roeck-us.net> - 2017-04-25 22:30 +0200
                  Re: [PATCH 0/2] DS1374 Watchdog fixes Moritz Fischer <mdf@kernel.org> - 2017-04-25 22:40 +0200
                    Re: [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck <linux@roeck-us.net> - 2017-04-25 23:10 +0200

#1630078 — [PATCH 0/2] DS1374 Watchdog fixes

Hi all,

this series fixes two issues that I ran into when trying to use the
watchdog part of the DS1374 on of our products.

This series is basically a precursor to some further cleanup work,
that turns the DS1374 into a MFD device with an RTC and WDT in
separate drivers [1] each integrated in either the watchdog and
the rtc frameworks.

I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling
the watchdog behavior and currently I'm investigating how to make
that work via DT.

Watchdog maintainers, do you have an idea on how to do that in a
non breaking fashion?

Thanks,
Moritz

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux.git/log/?h=wip/mfd-ds1374-rfc

Moritz Fischer (2):
  rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt
    ticks
  rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL

 drivers/rtc/rtc-ds1374.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1630189

On 04/24/2017 03:05 PM, Moritz Fischer wrote:
> Hi all,
>
> this series fixes two issues that I ran into when trying to use the
> watchdog part of the DS1374 on of our products.
>
> This series is basically a precursor to some further cleanup work,
> that turns the DS1374 into a MFD device with an RTC and WDT in
> separate drivers [1] each integrated in either the watchdog and
> the rtc frameworks.
>
> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling
> the watchdog behavior and currently I'm investigating how to make
> that work via DT.
>
> Watchdog maintainers, do you have an idea on how to do that in a
> non breaking fashion?
>

Depends on what you mean with "non breaking". Just using the normal mfd
mechanisms, ie define an mfd cell for each client driver, should work.
Do you see any problems with that ? Either case, that doesn't seem
to be a watchdog driver problem, or am I missing something ?

> Thanks,
> Moritz
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux.git/log/?h=wip/mfd-ds1374-rfc
>
> Moritz Fischer (2):
>   rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt
>     ticks
>   rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL
>

I don't really see the point of doing that if you plan to move the watchdog
part of the driver into the watchdog directory. We for sure won't accept a
watchdog driver that does not use the watchdog infrastructure.

Regarding
+	/* WHY? */
+	ds1374->wdd.timeout = t;

Assuming you mean why the driver has to set the timeout value - not every
watchdog hardware supports timeouts in multiples of 1 second. The driver
is expected to set the value to the real timeout, not to the timeout asked
for by the infrastructure.

Guenter

[toc] | [prev] | [next] | [standalone]

#1630577

Hi Guenter,

On Mon, Apr 24, 2017 at 10:03 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 04/24/2017 03:05 PM, Moritz Fischer wrote:

>> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling
>> the watchdog behavior and currently I'm investigating how to make
>> that work via DT.
>>
>> Watchdog maintainers, do you have an idea on how to do that in a
>> non breaking fashion?
>>
>
> Depends on what you mean with "non breaking". Just using the normal mfd
> mechanisms, ie define an mfd cell for each client driver, should work.
> Do you see any problems with that ? Either case, that doesn't seem
> to be a watchdog driver problem, or am I missing something ?

Well so currently watchdog behavior is selected (out of the two options alarm,
or watchdog) by enabling the configuration option mentioned above.
If I change this over to use a dt-based approach like dallas,ds1374-mode = <2>;
to select the behavior in the mfd for example, won't that break people that
relied on the old behavior? If everyone involved is ok with that, I'm happy
to just add it to the binding.

> I don't really see the point of doing that if you plan to move the watchdog
> part of the driver into the watchdog directory. We for sure won't accept a
> watchdog driver that does not use the watchdog infrastructure.

The idea was to fix what's broken currently (this patchset) and then refactor.
But if you prefer I can do all in one go instead.

>
> Regarding
> +       /* WHY? */
> +       ds1374->wdd.timeout = t;
>
> Assuming you mean why the driver has to set the timeout value - not every
> watchdog hardware supports timeouts in multiples of 1 second. The driver
> is expected to set the value to the real timeout, not to the timeout asked
> for by the infrastructure.

Yeah that branch is work in progress and needs cleanup. Leftover from testing,
before I had understood why. Branch needs cleanup. It also doesn't really use
regmap_update_bits etc.

Thanks for the explanation,

Moritz

[toc] | [prev] | [next] | [standalone]

#1630741

On Tue, Apr 25, 2017 at 07:55:28AM -0700, Moritz Fischer wrote:
> Hi Guenter,
> 
> On Mon, Apr 24, 2017 at 10:03 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 04/24/2017 03:05 PM, Moritz Fischer wrote:
> 
> >> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling
> >> the watchdog behavior and currently I'm investigating how to make
> >> that work via DT.
> >>
> >> Watchdog maintainers, do you have an idea on how to do that in a
> >> non breaking fashion?
> >>
> >
> > Depends on what you mean with "non breaking". Just using the normal mfd
> > mechanisms, ie define an mfd cell for each client driver, should work.
> > Do you see any problems with that ? Either case, that doesn't seem
> > to be a watchdog driver problem, or am I missing something ?
> 
> Well so currently watchdog behavior is selected (out of the two options alarm,
> or watchdog) by enabling the configuration option mentioned above.
> If I change this over to use a dt-based approach like dallas,ds1374-mode = <2>;
> to select the behavior in the mfd for example, won't that break people that
> relied on the old behavior? If everyone involved is ok with that, I'm happy
> to just add it to the binding.
> 

Sorry, I must be missing something. Looking into the driver code, my
understanding is that CONFIG_RTC_DRV_DS1374_WDT enables the watchdog in
addition to rtc functionality, not one or the other. Sure you would need
a different configuration option if you were to move the watchdog code into
drivers/watchdog, but other than that I don't really understand the problem.
What is the issue with, for example,

config DS1374_WDT
        bool "Dallas/Maxim DS1374 watchdog timer"
	depends on MFD_DS1374
	help
	  If you say Y here you will get support for the
	  watchdog timer in the Dallas Semiconductor DS1374
	  real-time clock chips.

in drivers/watchdog/Kconfig, and the mfd driver instantiating it like
any other mfd client driver ?

Either case, limiting support to DT based systems seems to be the wrong
approach. There might be Intel platforms using this chip.

> > I don't really see the point of doing that if you plan to move the watchdog
> > part of the driver into the watchdog directory. We for sure won't accept a
> > watchdog driver that does not use the watchdog infrastructure.
> 
> The idea was to fix what's broken currently (this patchset) and then refactor.
> But if you prefer I can do all in one go instead.
> 

It just seemed a waste to me to change/fix a function which is going to
be removed in a subsequent patch (I seem to recall that there was a fix
to the ioctl function).

If/when you move the driver to drivers/watchdog, please make sure that
it doesn't use any instantiation related static variables (ie other than
module parameters).

> >
> > Regarding
> > +       /* WHY? */
> > +       ds1374->wdd.timeout = t;
> >
> > Assuming you mean why the driver has to set the timeout value - not every
> > watchdog hardware supports timeouts in multiples of 1 second. The driver
> > is expected to set the value to the real timeout, not to the timeout asked
> > for by the infrastructure.
> 
> Yeah that branch is work in progress and needs cleanup. Leftover from testing,
> before I had understood why. Branch needs cleanup. It also doesn't really use
> regmap_update_bits etc.
> 

Well, you did enhance the code to use regmap, which by itself is a significant
improvement ...

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1630770

On 25/04/2017 at 09:17:43 -0700, Guenter Roeck wrote:
> On Tue, Apr 25, 2017 at 07:55:28AM -0700, Moritz Fischer wrote:
> > Hi Guenter,
> > 
> > On Mon, Apr 24, 2017 at 10:03 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > > On 04/24/2017 03:05 PM, Moritz Fischer wrote:
> > 
> > >> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling
> > >> the watchdog behavior and currently I'm investigating how to make
> > >> that work via DT.
> > >>
> > >> Watchdog maintainers, do you have an idea on how to do that in a
> > >> non breaking fashion?
> > >>
> > >
> > > Depends on what you mean with "non breaking". Just using the normal mfd
> > > mechanisms, ie define an mfd cell for each client driver, should work.
> > > Do you see any problems with that ? Either case, that doesn't seem
> > > to be a watchdog driver problem, or am I missing something ?
> > 
> > Well so currently watchdog behavior is selected (out of the two options alarm,
> > or watchdog) by enabling the configuration option mentioned above.
> > If I change this over to use a dt-based approach like dallas,ds1374-mode = <2>;
> > to select the behavior in the mfd for example, won't that break people that
> > relied on the old behavior? If everyone involved is ok with that, I'm happy
> > to just add it to the binding.
> > 
> 
> Sorry, I must be missing something. Looking into the driver code, my
> understanding is that CONFIG_RTC_DRV_DS1374_WDT enables the watchdog in
> addition to rtc functionality, not one or the other. Sure you would need
> a different configuration option if you were to move the watchdog code into
> drivers/watchdog, but other than that I don't really understand the problem.
> What is the issue with, for example,
> 

The watchdog functionality and the rtc alarm are mutually exclusive.

> > The idea was to fix what's broken currently (this patchset) and then refactor.
> > But if you prefer I can do all in one go instead.
> > 
> 
> It just seemed a waste to me to change/fix a function which is going to
> be removed in a subsequent patch (I seem to recall that there was a fix
> to the ioctl function).
> 

I'd say that it depends on whether you want to backport the fixes to the
stable kernels. Backporting the full rework is probably riskier.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1630794

On Tue, Apr 25, 2017 at 06:32:04PM +0200, Alexandre Belloni wrote:
> On 25/04/2017 at 09:17:43 -0700, Guenter Roeck wrote:
> > On Tue, Apr 25, 2017 at 07:55:28AM -0700, Moritz Fischer wrote:
> > > Hi Guenter,
> > > 
> > > On Mon, Apr 24, 2017 at 10:03 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > > > On 04/24/2017 03:05 PM, Moritz Fischer wrote:
> > > 
> > > >> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling
> > > >> the watchdog behavior and currently I'm investigating how to make
> > > >> that work via DT.
> > > >>
> > > >> Watchdog maintainers, do you have an idea on how to do that in a
> > > >> non breaking fashion?
> > > >>
> > > >
> > > > Depends on what you mean with "non breaking". Just using the normal mfd
> > > > mechanisms, ie define an mfd cell for each client driver, should work.
> > > > Do you see any problems with that ? Either case, that doesn't seem
> > > > to be a watchdog driver problem, or am I missing something ?
> > > 
> > > Well so currently watchdog behavior is selected (out of the two options alarm,
> > > or watchdog) by enabling the configuration option mentioned above.
> > > If I change this over to use a dt-based approach like dallas,ds1374-mode = <2>;
> > > to select the behavior in the mfd for example, won't that break people that
> > > relied on the old behavior? If everyone involved is ok with that, I'm happy
> > > to just add it to the binding.
> > > 
> > 
> > Sorry, I must be missing something. Looking into the driver code, my
> > understanding is that CONFIG_RTC_DRV_DS1374_WDT enables the watchdog in
> > addition to rtc functionality, not one or the other. Sure you would need
> > a different configuration option if you were to move the watchdog code into
> > drivers/watchdog, but other than that I don't really understand the problem.
> > What is the issue with, for example,
> > 
> 
> The watchdog functionality and the rtc alarm are mutually exclusive.
> 
Ah, I missed the "n" in various #ifndef statements.

I can't really comment on how to solve that; I simply don't know.
Also, even with a dt property, it still would be necessary to have
a non-DT means to configure one or the other. Making whatever solution
backward compatible also seems tricky; I don't have a solution for that
problem either.

> > > The idea was to fix what's broken currently (this patchset) and then refactor.
> > > But if you prefer I can do all in one go instead.
> > > 
> > 
> > It just seemed a waste to me to change/fix a function which is going to
> > be removed in a subsequent patch (I seem to recall that there was a fix
> > to the ioctl function).
> > 
> 
> I'd say that it depends on whether you want to backport the fixes to the
> stable kernels. Backporting the full rework is probably riskier.
> 

Good point.

Guenter

[toc] | [prev] | [next] | [standalone]

#1630946

On Tue, Apr 25, 2017 at 09:58:24AM -0700, Guenter Roeck wrote:

> Ah, I missed the "n" in various #ifndef statements.
>
> I can't really comment on how to solve that; I simply don't know.
> Also, even with a dt property, it still would be necessary to have
> a non-DT means to configure one or the other. Making whatever solution
> backward compatible also seems tricky; I don't have a solution for that
> problem either.

How does one do these things in a non-dt context? Platform data? I'd let
the MFD select the 'mode'. Maybe being backwards compatible isn't
possible in any case. Is there a rule somewhere that we guarantee you'll
never have to change your CONFIG_FOO options?

>
> > > > The idea was to fix what's broken currently (this patchset) and then refactor.
> > > > But if you prefer I can do all in one go instead.
> > > >
> > >
> > > It just seemed a waste to me to change/fix a function which is going to
> > > be removed in a subsequent patch (I seem to recall that there was a fix
> > > to the ioctl function).
> > >
> >
> > I'd say that it depends on whether you want to backport the fixes to the
> > stable kernels. Backporting the full rework is probably riskier.

I suck at communicating these days. But yeah. That was basically my
concern when I split it up into 'Fixes' and 'Rework'.

Mostly since the rework might take a couple of rounds of review, while the
fix can unbrick stuff (might still need review of course)

Cheers,

Moritz

[toc] | [prev] | [next] | [standalone]

#1630972

On Tue, Apr 25, 2017 at 12:58:36PM -0700, Moritz Fischer wrote:
> On Tue, Apr 25, 2017 at 09:58:24AM -0700, Guenter Roeck wrote:
> 
> > Ah, I missed the "n" in various #ifndef statements.
> >
> > I can't really comment on how to solve that; I simply don't know.
> > Also, even with a dt property, it still would be necessary to have
> > a non-DT means to configure one or the other. Making whatever solution
> > backward compatible also seems tricky; I don't have a solution for that
> > problem either.
> 
> How does one do these things in a non-dt context? Platform data? I'd let

Platform data is out of favor. You'd probably want to use device properties
(see drivers/base/property.c). Question though is if this is considered
configuration, hardware description, or both. Presumably the watchdog
only makes sense if the reset signal is wired, and the alarm only makes
sense if the interrupt is wired, but what if both are wired ?

> the MFD select the 'mode'. Maybe being backwards compatible isn't
> possible in any case. Is there a rule somewhere that we guarantee you'll
> never have to change your CONFIG_FOO options?
> 

That would be nice, but no, there is no such rule. You can probably argue
that no published kernel configuration enables the watchdog flag,
so there is nothing to be concerned about.

Guenter

> >
> > > > > The idea was to fix what's broken currently (this patchset) and then refactor.
> > > > > But if you prefer I can do all in one go instead.
> > > > >
> > > >
> > > > It just seemed a waste to me to change/fix a function which is going to
> > > > be removed in a subsequent patch (I seem to recall that there was a fix
> > > > to the ioctl function).
> > > >
> > >
> > > I'd say that it depends on whether you want to backport the fixes to the
> > > stable kernels. Backporting the full rework is probably riskier.
> 
> I suck at communicating these days. But yeah. That was basically my
> concern when I split it up into 'Fixes' and 'Rework'.
> 
> Mostly since the rework might take a couple of rounds of review, while the
> fix can unbrick stuff (might still need review of course)
> 
> Cheers,
> 
> Moritz

[toc] | [prev] | [next] | [standalone]

#1630983

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 25, 2017 at 01:22:10PM -0700, Guenter Roeck wrote:
> On Tue, Apr 25, 2017 at 12:58:36PM -0700, Moritz Fischer wrote:
> > On Tue, Apr 25, 2017 at 09:58:24AM -0700, Guenter Roeck wrote:
> > 
> > > Ah, I missed the "n" in various #ifndef statements.
> > >
> > > I can't really comment on how to solve that; I simply don't know.
> > > Also, even with a dt property, it still would be necessary to have
> > > a non-DT means to configure one or the other. Making whatever solution
> > > backward compatible also seems tricky; I don't have a solution for that
> > > problem either.
> > 
> > How does one do these things in a non-dt context? Platform data? I'd let
> 
> Platform data is out of favor. You'd probably want to use device properties
> (see drivers/base/property.c). Question though is if this is considered
> configuration, hardware description, or both. Presumably the watchdog
> only makes sense if the reset signal is wired, and the alarm only makes
> sense if the interrupt is wired, but what if both are wired ?

To make things worse you can even remap the reset output to the INT pin
(which my platform does).

I'll look at device properties. Thanks for the pointer.

> 
> > the MFD select the 'mode'. Maybe being backwards compatible isn't
> > possible in any case. Is there a rule somewhere that we guarantee you'll
> > never have to change your CONFIG_FOO options?
> > 
> 
> That would be nice, but no, there is no such rule. You can probably argue
> that no published kernel configuration enables the watchdog flag,
> so there is nothing to be concerned about.

Alright, cool. Thanks

Moritz

PS: Haven't forgotten about the cros-ec-hwmon patch that I sent out ...

[toc] | [prev] | [next] | [standalone]

#1630995

On Tue, Apr 25, 2017 at 01:34:18PM -0700, Moritz Fischer wrote:
> On Tue, Apr 25, 2017 at 01:22:10PM -0700, Guenter Roeck wrote:
> > On Tue, Apr 25, 2017 at 12:58:36PM -0700, Moritz Fischer wrote:
> > > On Tue, Apr 25, 2017 at 09:58:24AM -0700, Guenter Roeck wrote:
> > > 
> > > > Ah, I missed the "n" in various #ifndef statements.
> > > >
> > > > I can't really comment on how to solve that; I simply don't know.
> > > > Also, even with a dt property, it still would be necessary to have
> > > > a non-DT means to configure one or the other. Making whatever solution
> > > > backward compatible also seems tricky; I don't have a solution for that
> > > > problem either.
> > > 
> > > How does one do these things in a non-dt context? Platform data? I'd let
> > 
> > Platform data is out of favor. You'd probably want to use device properties
> > (see drivers/base/property.c). Question though is if this is considered
> > configuration, hardware description, or both. Presumably the watchdog
> > only makes sense if the reset signal is wired, and the alarm only makes
> > sense if the interrupt is wired, but what if both are wired ?
> 
> To make things worse you can even remap the reset output to the INT pin
> (which my platform does).
> 

So that is what the weird 250ms "interrupt signal" is for. I had wondered
what that is supposed to be used for.

> I'll look at device properties. Thanks for the pointer.
> 
> > 
> > > the MFD select the 'mode'. Maybe being backwards compatible isn't
> > > possible in any case. Is there a rule somewhere that we guarantee you'll
> > > never have to change your CONFIG_FOO options?
> > > 
> > 
> > That would be nice, but no, there is no such rule. You can probably argue
> > that no published kernel configuration enables the watchdog flag,
> > so there is nothing to be concerned about.
> 
> Alright, cool. Thanks
> 
> Moritz
> 
> PS: Haven't forgotten about the cros-ec-hwmon patch that I sent out ...

No, still trying to get internal feedback. I'll have to ask again.

Guenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web