Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Guenter and Olof,

On Mon, Dec 2, 2013 at 1:36 PM, Guenter Roeck  wrote:
> On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
>> On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck  wrote:
>> > On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
>> >> A good watchdog driver is supposed to report when it was responsible
>> >> for resetting the system.  Implement this for the s3c2410, at least on
>> >> exynos5250 and exynos5420 where we already have a pointer to the PMU
>> >> registers to read the information.
>> >>
>> >> Signed-off-by: Doug Anderson 
>> >> ---
>> >> This patch is based atop Leela Krishna's recent series that ends with
>> >> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
>> >> AKA .
>> >>
>> >>  drivers/watchdog/s3c2410_wdt.c | 42 
>> >> +++---
>> >>  1 file changed, 39 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/watchdog/s3c2410_wdt.c 
>> >> b/drivers/watchdog/s3c2410_wdt.c
>> >> index 47f4dcf..2c87d37 100644
>> >> --- a/drivers/watchdog/s3c2410_wdt.c
>> >> +++ b/drivers/watchdog/s3c2410_wdt.c
>> >> @@ -62,9 +62,13 @@
>> >>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
>> >>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
>> >>
>> >> +#define RST_STAT_REG_OFFSET  0x0404
>> >>  #define WDT_DISABLE_REG_OFFSET   0x0408
>> >>  #define WDT_MASK_RESET_REG_OFFSET0x040c
>> >>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
>> >> +#define QUIRK_HAS_RST_STAT   (1 << 1)
>> >> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
>> >> +  QUIRK_HAS_RST_STAT)
>> >>
>> > I am not really happy about the NEED (both of them, really) here.
>> > How about HAS instead ?
>>
>> Ah, I just commented on these things on our internal review site too
>> on this patch. I don't even think a quirk is needed -- just use the
>> presence of a non-0 rst_stat_reg to tell if you need to use regmap.
>>
> Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
> as well.

I have changed all instances of NEED / NEEDS and HAS / HAVE.  Please
take a look.  Since some of those changes applied to Leela Krishna's
patch I'm sending up a "fixup" patch which hopefully he can
incorporate into a v12.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Tomasz,

On Thu, Dec 5, 2013 at 8:21 AM, Tomasz Figa  wrote:
> Hi Doug,
>
> Please see my comments inline.
>
> On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
>> A good watchdog driver is supposed to report when it was responsible
>> for resetting the system.  Implement this for the s3c2410, at least on
>> exynos5250 and exynos5420 where we already have a pointer to the PMU
>> registers to read the information.
>>
>> Signed-off-by: Doug Anderson 
>> ---
>> This patch is based atop Leela Krishna's recent series that ends with
>> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
>> AKA .
>>
>>  drivers/watchdog/s3c2410_wdt.c | 42 
>> +++---
>>  1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>> index 47f4dcf..2c87d37 100644
>> --- a/drivers/watchdog/s3c2410_wdt.c
>> +++ b/drivers/watchdog/s3c2410_wdt.c
>> @@ -62,9 +62,13 @@
>>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
>>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
>>
>> +#define RST_STAT_REG_OFFSET  0x0404
>
> IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
> registers below should be as well, but I missed this in the patch adding
> them.

Done in v2 (plus fixup of Leela Krishna's).


>>  #define WDT_DISABLE_REG_OFFSET   0x0408
>>  #define WDT_MASK_RESET_REG_OFFSET0x040c
>>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
>> +#define QUIRK_HAS_RST_STAT   (1 << 1)
>> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
>> +  QUIRK_HAS_RST_STAT)
>>
>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>>  static int tmr_margin;
>> @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for 
>> debug (default 0)");
>>   * timer reset functionality.
>>   * @mask_bit: Bit number for the watchdog timer in the disable register and 
>> the
>>   * mask reset register.
>> + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
>> status.
>> + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
>> + * reset.
>>   * @quirks: A bitfield of quirks.
>>   */
>>
>> @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
>>   int disable_reg;
>>   int mask_reset_reg;
>>   int mask_bit;
>> + int rst_stat_reg;
>> + int rst_stat_bit;
>>   u32 quirks;
>>  };
>>
>> @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
>> drv_data_exynos5250  = {
>>   .disable_reg = WDT_DISABLE_REG_OFFSET,
>>   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>>   .mask_bit = 20,
>> - .quirks = QUIRK_NEEDS_PMU_CONFIG
>> + .rst_stat_reg = RST_STAT_REG_OFFSET,
>> + .rst_stat_bit = 20,
>> + .quirks = QUIRK_NEEDS_PMU_CONFIG |
>> + QUIRK_HAS_RST_STAT,
>>  };
>>
>>  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>>   .disable_reg = WDT_DISABLE_REG_OFFSET,
>>   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>>   .mask_bit = 0,
>> - .quirks = QUIRK_NEEDS_PMU_CONFIG
>> + .rst_stat_reg = RST_STAT_REG_OFFSET,
>> + .rst_stat_bit = 9,
>> + .quirks = QUIRK_NEEDS_PMU_CONFIG |
>> + QUIRK_HAS_RST_STAT,
>>  };
>>
>>  static const struct of_device_id s3c2410_wdt_match[] = {
>> @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
>> s3c2410_wdt *wdt)
>>  }
>>  #endif
>>
>> +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
>> *wdt)
>> +{
>> + unsigned int bootstatus = 0;
>> + int ret;
>> +
>> + if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {
>
> nit: I guess it's just a matter of taste, but to reduce code indentation
> you could inverse the check and simply return 0 here.

Done in v2.  This is the same suggestion Olof made.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Tomasz

On Thu, Dec 5, 2013 at 8:18 AM, Tomasz Figa  wrote:
> On Thursday 05 of December 2013 08:00:27 Guenter Roeck wrote:
>> On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote:
>> > Hi Guenter Roeck,
>> >
>> > On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck  wrote:
>> > >
>> > > On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
>> > > > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck  
>> > > > wrote:
>> > > > > On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
>> > > > >> A good watchdog driver is supposed to report when it was responsible
>> > > > >> for resetting the system.  Implement this for the s3c2410, at least 
>> > > > >> on
>> > > > >> exynos5250 and exynos5420 where we already have a pointer to the PMU
>> > > > >> registers to read the information.
>> > > > >>
>> > > > >> Signed-off-by: Doug Anderson 
>> > > > >> ---
>> > > > >> This patch is based atop Leela Krishna's recent series that ends 
>> > > > >> with
>> > > > >> (ARM: dts: update watchdog device nodes for Exynos5250 and 
>> > > > >> Exynos5420)
>> > > > >> AKA .
>> > > > >>
>> > > > >>  drivers/watchdog/s3c2410_wdt.c | 42 
>> > > > >> +++---
>> > > > >>  1 file changed, 39 insertions(+), 3 deletions(-)
>> > > > >>
>> > > > >> diff --git a/drivers/watchdog/s3c2410_wdt.c 
>> > > > >> b/drivers/watchdog/s3c2410_wdt.c
>> > > > >> index 47f4dcf..2c87d37 100644
>> > > > >> --- a/drivers/watchdog/s3c2410_wdt.c
>> > > > >> +++ b/drivers/watchdog/s3c2410_wdt.c
>> > > > >> @@ -62,9 +62,13 @@
>> > > > >>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
>> > > > >>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
>> > > > >>
>> > > > >> +#define RST_STAT_REG_OFFSET  0x0404
>> > > > >>  #define WDT_DISABLE_REG_OFFSET   0x0408
>> > > > >>  #define WDT_MASK_RESET_REG_OFFSET0x040c
>> > > > >>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
>> > > > >> +#define QUIRK_HAS_RST_STAT   (1 << 1)
>> > > > >> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
>> > > > >> +  QUIRK_HAS_RST_STAT)
>> > > > >>
>> > > > > I am not really happy about the NEED (both of them, really) here.
>> > > > > How about HAS instead ?
>> > > >
>> > > > Ah, I just commented on these things on our internal review site too
>> > > > on this patch. I don't even think a quirk is needed -- just use the
>> > > > presence of a non-0 rst_stat_reg to tell if you need to use regmap.
>> > > >
>> > > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
>> > > as well.
>> > >
>> >
>> > As Tomasz Figa suggested I introduced quirks,
>> >
>> 'quirk' implies a workaround for non-standard or broken hardware,
>> not a flag indicating device specific functionality.
>
> I wouldn't limit meaning of "quirk" to only this. The word has been widely
> used around the kernel as a name for differences between hardware
> variants.
>
> As for the original issue itself, IMHO Doug's original solution is the
> most practical, as 0 can be a valid register offset and RST_STAT register
> could be used for other purposes as well in future, depending on other
> quirk flag.

OK, I'm keeping my concept but adjusting the names.  Version 2 coming
up shortly.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Guenter,

On Thu, Dec 5, 2013 at 8:40 AM, Guenter Roeck  wrote:
> On Thu, Dec 05, 2013 at 05:21:47PM +0100, Tomasz Figa wrote:
>> Hi Doug,
>>
>> Please see my comments inline.
>>
>> On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
>> > A good watchdog driver is supposed to report when it was responsible
>> > for resetting the system.  Implement this for the s3c2410, at least on
>> > exynos5250 and exynos5420 where we already have a pointer to the PMU
>> > registers to read the information.
>> >
>> > Signed-off-by: Doug Anderson 
>> > ---
>> > This patch is based atop Leela Krishna's recent series that ends with
>> > (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
>> > AKA .
>> >
>> >  drivers/watchdog/s3c2410_wdt.c | 42 
>> > +++---
>> >  1 file changed, 39 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/watchdog/s3c2410_wdt.c 
>> > b/drivers/watchdog/s3c2410_wdt.c
>> > index 47f4dcf..2c87d37 100644
>> > --- a/drivers/watchdog/s3c2410_wdt.c
>> > +++ b/drivers/watchdog/s3c2410_wdt.c
>> > @@ -62,9 +62,13 @@
>> >  #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
>> >  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME   (15)
>> >
>> > +#define RST_STAT_REG_OFFSET0x0404
>>
>> IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
>> registers below should be as well, but I missed this in the patch adding
>> them.
>>
>> >  #define WDT_DISABLE_REG_OFFSET 0x0408
>> >  #define WDT_MASK_RESET_REG_OFFSET  0x040c
>> >  #define QUIRK_NEEDS_PMU_CONFIG (1 << 0)
>> > +#define QUIRK_HAS_RST_STAT (1 << 1)
>> > +#define QUIRKS_NEED_PMUREG (QUIRK_NEEDS_PMU_CONFIG | \
>> > +QUIRK_HAS_RST_STAT)
>> >
>> >  static bool nowayout   = WATCHDOG_NOWAYOUT;
>> >  static int tmr_margin;
>> > @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for 
>> > debug (default 0)");
>> >   * timer reset functionality.
>> >   * @mask_bit: Bit number for the watchdog timer in the disable register 
>> > and the
>> >   * mask reset register.
>> > + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
>> > status.
>> > + * @rst_stat_bit: Bit number in the rst_stat register indicating a 
>> > watchdog
>> > + * reset.
>> >   * @quirks: A bitfield of quirks.
>> >   */
>> >
>> > @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
>> > int disable_reg;
>> > int mask_reset_reg;
>> > int mask_bit;
>> > +   int rst_stat_reg;
>> > +   int rst_stat_bit;
>> > u32 quirks;
>> >  };
>> >
>> > @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
>> > drv_data_exynos5250  = {
>> > .disable_reg = WDT_DISABLE_REG_OFFSET,
>> > .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>> > .mask_bit = 20,
>> > -   .quirks = QUIRK_NEEDS_PMU_CONFIG
>> > +   .rst_stat_reg = RST_STAT_REG_OFFSET,
>> > +   .rst_stat_bit = 20,
>> > +   .quirks = QUIRK_NEEDS_PMU_CONFIG |
>> > +   QUIRK_HAS_RST_STAT,
>> >  };
>> >
>> >  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>> > .disable_reg = WDT_DISABLE_REG_OFFSET,
>> > .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>> > .mask_bit = 0,
>> > -   .quirks = QUIRK_NEEDS_PMU_CONFIG
>> > +   .rst_stat_reg = RST_STAT_REG_OFFSET,
>> > +   .rst_stat_bit = 9,
>> > +   .quirks = QUIRK_NEEDS_PMU_CONFIG |
>> > +   QUIRK_HAS_RST_STAT,
>> >  };
>> >
>> >  static const struct of_device_id s3c2410_wdt_match[] = {
>> > @@ -423,6 +438,25 @@ static inline void 
>> > s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
>> >  }
>> >  #endif
>> >
>> > +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
>> > *wdt)
>> > +{
>> > +   unsigned int bootstatus = 0;
>> > +   int ret;
>> > +
>> > +   if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {
>>
>> nit: I guess it's just a matter of taste, but to reduce code indentation
>> you could inverse the check and simply return 0 here.
>>
>
> nit: If so, it would make sense to to the same for the other 'quirk'
> for consistency.
>
> static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool 
> mask)
> {
> ...
> if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG))
> return 0;
> ...
> }

Done in my proposed fixup for Leela Krishna's patch.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Tomasz Figa]

On Thursday 05 of December 2013 08:40:38 Guenter Roeck wrote:
> On Thu, Dec 05, 2013 at 05:21:47PM +0100, Tomasz Figa wrote:
> > Hi Doug,
> > 
> > Please see my comments inline.
> > 
> > On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
> > > A good watchdog driver is supposed to report when it was responsible
> > > for resetting the system.  Implement this for the s3c2410, at least on
> > > exynos5250 and exynos5420 where we already have a pointer to the PMU
> > > registers to read the information.
> > > 
> > > Signed-off-by: Doug Anderson 
> > > ---
> > > This patch is based atop Leela Krishna's recent series that ends with
> > > (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> > > AKA .
> > > 
> > >  drivers/watchdog/s3c2410_wdt.c | 42 
> > > +++---
> > >  1 file changed, 39 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/s3c2410_wdt.c 
> > > b/drivers/watchdog/s3c2410_wdt.c
> > > index 47f4dcf..2c87d37 100644
> > > --- a/drivers/watchdog/s3c2410_wdt.c
> > > +++ b/drivers/watchdog/s3c2410_wdt.c
> > > @@ -62,9 +62,13 @@
> > >  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
> > >  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
> > >  
> > > +#define RST_STAT_REG_OFFSET  0x0404
> > 
> > IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
> > registers below should be as well, but I missed this in the patch adding
> > them.
> > 
> > >  #define WDT_DISABLE_REG_OFFSET   0x0408
> > >  #define WDT_MASK_RESET_REG_OFFSET0x040c
> > >  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
> > > +#define QUIRK_HAS_RST_STAT   (1 << 1)
> > > +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
> > > +  QUIRK_HAS_RST_STAT)
> > >  
> > >  static bool nowayout = WATCHDOG_NOWAYOUT;
> > >  static int tmr_margin;
> > > @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for 
> > > debug (default 0)");
> > >   * timer reset functionality.
> > >   * @mask_bit: Bit number for the watchdog timer in the disable register 
> > > and the
> > >   * mask reset register.
> > > + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
> > > status.
> > > + * @rst_stat_bit: Bit number in the rst_stat register indicating a 
> > > watchdog
> > > + * reset.
> > >   * @quirks: A bitfield of quirks.
> > >   */
> > >  
> > > @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
> > >   int disable_reg;
> > >   int mask_reset_reg;
> > >   int mask_bit;
> > > + int rst_stat_reg;
> > > + int rst_stat_bit;
> > >   u32 quirks;
> > >  };
> > >  
> > > @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
> > > drv_data_exynos5250  = {
> > >   .disable_reg = WDT_DISABLE_REG_OFFSET,
> > >   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> > >   .mask_bit = 20,
> > > - .quirks = QUIRK_NEEDS_PMU_CONFIG
> > > + .rst_stat_reg = RST_STAT_REG_OFFSET,
> > > + .rst_stat_bit = 20,
> > > + .quirks = QUIRK_NEEDS_PMU_CONFIG |
> > > + QUIRK_HAS_RST_STAT,
> > >  };
> > >  
> > >  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> > >   .disable_reg = WDT_DISABLE_REG_OFFSET,
> > >   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> > >   .mask_bit = 0,
> > > - .quirks = QUIRK_NEEDS_PMU_CONFIG
> > > + .rst_stat_reg = RST_STAT_REG_OFFSET,
> > > + .rst_stat_bit = 9,
> > > + .quirks = QUIRK_NEEDS_PMU_CONFIG |
> > > + QUIRK_HAS_RST_STAT,
> > >  };
> > >  
> > >  static const struct of_device_id s3c2410_wdt_match[] = {
> > > @@ -423,6 +438,25 @@ static inline void 
> > > s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
> > >  }
> > >  #endif
> > >  
> > > +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
> > > *wdt)
> > > +{
> > > + unsigned int bootstatus = 0;
> > > + int ret;
> > > +
> > > + if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {
> > 
> > nit: I guess it's just a matter of taste, but to reduce code indentation
> > you could inverse the check and simply return 0 here.
> > 
> 
> nit: If so, it would make sense to to the same for the other 'quirk'
> for consistency.
> 
> static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool 
> mask)
> {
>   ...
>   if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG))
>   return 0;
>   ...
> }

That's quite a bit different story, as currently the check is outside of
this function, but I agree, it would look better.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Thu, Dec 05, 2013 at 05:21:47PM +0100, Tomasz Figa wrote:
> Hi Doug,
> 
> Please see my comments inline.
> 
> On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
> > A good watchdog driver is supposed to report when it was responsible
> > for resetting the system.  Implement this for the s3c2410, at least on
> > exynos5250 and exynos5420 where we already have a pointer to the PMU
> > registers to read the information.
> > 
> > Signed-off-by: Doug Anderson 
> > ---
> > This patch is based atop Leela Krishna's recent series that ends with
> > (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> > AKA .
> > 
> >  drivers/watchdog/s3c2410_wdt.c | 42 
> > +++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 47f4dcf..2c87d37 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -62,9 +62,13 @@
> >  #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
> >  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME   (15)
> >  
> > +#define RST_STAT_REG_OFFSET0x0404
> 
> IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
> registers below should be as well, but I missed this in the patch adding
> them.
> 
> >  #define WDT_DISABLE_REG_OFFSET 0x0408
> >  #define WDT_MASK_RESET_REG_OFFSET  0x040c
> >  #define QUIRK_NEEDS_PMU_CONFIG (1 << 0)
> > +#define QUIRK_HAS_RST_STAT (1 << 1)
> > +#define QUIRKS_NEED_PMUREG (QUIRK_NEEDS_PMU_CONFIG | \
> > +QUIRK_HAS_RST_STAT)
> >  
> >  static bool nowayout   = WATCHDOG_NOWAYOUT;
> >  static int tmr_margin;
> > @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for 
> > debug (default 0)");
> >   * timer reset functionality.
> >   * @mask_bit: Bit number for the watchdog timer in the disable register 
> > and the
> >   * mask reset register.
> > + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
> > status.
> > + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
> > + * reset.
> >   * @quirks: A bitfield of quirks.
> >   */
> >  
> > @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
> > int disable_reg;
> > int mask_reset_reg;
> > int mask_bit;
> > +   int rst_stat_reg;
> > +   int rst_stat_bit;
> > u32 quirks;
> >  };
> >  
> > @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
> > drv_data_exynos5250  = {
> > .disable_reg = WDT_DISABLE_REG_OFFSET,
> > .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> > .mask_bit = 20,
> > -   .quirks = QUIRK_NEEDS_PMU_CONFIG
> > +   .rst_stat_reg = RST_STAT_REG_OFFSET,
> > +   .rst_stat_bit = 20,
> > +   .quirks = QUIRK_NEEDS_PMU_CONFIG |
> > +   QUIRK_HAS_RST_STAT,
> >  };
> >  
> >  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> > .disable_reg = WDT_DISABLE_REG_OFFSET,
> > .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> > .mask_bit = 0,
> > -   .quirks = QUIRK_NEEDS_PMU_CONFIG
> > +   .rst_stat_reg = RST_STAT_REG_OFFSET,
> > +   .rst_stat_bit = 9,
> > +   .quirks = QUIRK_NEEDS_PMU_CONFIG |
> > +   QUIRK_HAS_RST_STAT,
> >  };
> >  
> >  static const struct of_device_id s3c2410_wdt_match[] = {
> > @@ -423,6 +438,25 @@ static inline void 
> > s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
> >  }
> >  #endif
> >  
> > +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
> > *wdt)
> > +{
> > +   unsigned int bootstatus = 0;
> > +   int ret;
> > +
> > +   if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {
> 
> nit: I guess it's just a matter of taste, but to reduce code indentation
> you could inverse the check and simply return 0 here.
> 

nit: If so, it would make sense to to the same for the other 'quirk'
for consistency.

static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
{
...
if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG))
return 0;
...
}

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Tomasz Figa]

Hi Doug,

Please see my comments inline.

On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
> A good watchdog driver is supposed to report when it was responsible
> for resetting the system.  Implement this for the s3c2410, at least on
> exynos5250 and exynos5420 where we already have a pointer to the PMU
> registers to read the information.
> 
> Signed-off-by: Doug Anderson 
> ---
> This patch is based atop Leela Krishna's recent series that ends with
> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> AKA .
> 
>  drivers/watchdog/s3c2410_wdt.c | 42 
> +++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 47f4dcf..2c87d37 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -62,9 +62,13 @@
>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
>  
> +#define RST_STAT_REG_OFFSET  0x0404

IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
registers below should be as well, but I missed this in the patch adding
them.

>  #define WDT_DISABLE_REG_OFFSET   0x0408
>  #define WDT_MASK_RESET_REG_OFFSET0x040c
>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
> +#define QUIRK_HAS_RST_STAT   (1 << 1)
> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
> +  QUIRK_HAS_RST_STAT)
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  static int tmr_margin;
> @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for 
> debug (default 0)");
>   * timer reset functionality.
>   * @mask_bit: Bit number for the watchdog timer in the disable register and 
> the
>   * mask reset register.
> + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
> status.
> + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
> + * reset.
>   * @quirks: A bitfield of quirks.
>   */
>  
> @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
>   int disable_reg;
>   int mask_reset_reg;
>   int mask_bit;
> + int rst_stat_reg;
> + int rst_stat_bit;
>   u32 quirks;
>  };
>  
> @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
> drv_data_exynos5250  = {
>   .disable_reg = WDT_DISABLE_REG_OFFSET,
>   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>   .mask_bit = 20,
> - .quirks = QUIRK_NEEDS_PMU_CONFIG
> + .rst_stat_reg = RST_STAT_REG_OFFSET,
> + .rst_stat_bit = 20,
> + .quirks = QUIRK_NEEDS_PMU_CONFIG |
> + QUIRK_HAS_RST_STAT,
>  };
>  
>  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>   .disable_reg = WDT_DISABLE_REG_OFFSET,
>   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>   .mask_bit = 0,
> - .quirks = QUIRK_NEEDS_PMU_CONFIG
> + .rst_stat_reg = RST_STAT_REG_OFFSET,
> + .rst_stat_bit = 9,
> + .quirks = QUIRK_NEEDS_PMU_CONFIG |
> + QUIRK_HAS_RST_STAT,
>  };
>  
>  static const struct of_device_id s3c2410_wdt_match[] = {
> @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
> s3c2410_wdt *wdt)
>  }
>  #endif
>  
> +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
> +{
> + unsigned int bootstatus = 0;
> + int ret;
> +
> + if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {

nit: I guess it's just a matter of taste, but to reduce code indentation
you could inverse the check and simply return 0 here.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Tomasz Figa]

On Thursday 05 of December 2013 08:00:27 Guenter Roeck wrote:
> On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote:
> > Hi Guenter Roeck,
> > 
> > On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck  wrote:
> > >
> > > On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
> > > > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck  
> > > > wrote:
> > > > > On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
> > > > >> A good watchdog driver is supposed to report when it was responsible
> > > > >> for resetting the system.  Implement this for the s3c2410, at least 
> > > > >> on
> > > > >> exynos5250 and exynos5420 where we already have a pointer to the PMU
> > > > >> registers to read the information.
> > > > >>
> > > > >> Signed-off-by: Doug Anderson 
> > > > >> ---
> > > > >> This patch is based atop Leela Krishna's recent series that ends with
> > > > >> (ARM: dts: update watchdog device nodes for Exynos5250 and 
> > > > >> Exynos5420)
> > > > >> AKA .
> > > > >>
> > > > >>  drivers/watchdog/s3c2410_wdt.c | 42 
> > > > >> +++---
> > > > >>  1 file changed, 39 insertions(+), 3 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/watchdog/s3c2410_wdt.c 
> > > > >> b/drivers/watchdog/s3c2410_wdt.c
> > > > >> index 47f4dcf..2c87d37 100644
> > > > >> --- a/drivers/watchdog/s3c2410_wdt.c
> > > > >> +++ b/drivers/watchdog/s3c2410_wdt.c
> > > > >> @@ -62,9 +62,13 @@
> > > > >>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
> > > > >>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
> > > > >>
> > > > >> +#define RST_STAT_REG_OFFSET  0x0404
> > > > >>  #define WDT_DISABLE_REG_OFFSET   0x0408
> > > > >>  #define WDT_MASK_RESET_REG_OFFSET0x040c
> > > > >>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
> > > > >> +#define QUIRK_HAS_RST_STAT   (1 << 1)
> > > > >> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
> > > > >> +  QUIRK_HAS_RST_STAT)
> > > > >>
> > > > > I am not really happy about the NEED (both of them, really) here.
> > > > > How about HAS instead ?
> > > >
> > > > Ah, I just commented on these things on our internal review site too
> > > > on this patch. I don't even think a quirk is needed -- just use the
> > > > presence of a non-0 rst_stat_reg to tell if you need to use regmap.
> > > >
> > > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
> > > as well.
> > >
> > 
> > As Tomasz Figa suggested I introduced quirks,
> > 
> 'quirk' implies a workaround for non-standard or broken hardware,
> not a flag indicating device specific functionality.

I wouldn't limit meaning of "quirk" to only this. The word has been widely
used around the kernel as a name for differences between hardware
variants.

As for the original issue itself, IMHO Doug's original solution is the
most practical, as 0 can be a valid register offset and RST_STAT register
could be used for other purposes as well in future, depending on other
quirk flag.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote:
> Hi Guenter Roeck,
> 
> On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck  wrote:
> >
> > On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
> > > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck  wrote:
> > > > On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
> > > >> A good watchdog driver is supposed to report when it was responsible
> > > >> for resetting the system.  Implement this for the s3c2410, at least on
> > > >> exynos5250 and exynos5420 where we already have a pointer to the PMU
> > > >> registers to read the information.
> > > >>
> > > >> Signed-off-by: Doug Anderson 
> > > >> ---
> > > >> This patch is based atop Leela Krishna's recent series that ends with
> > > >> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> > > >> AKA .
> > > >>
> > > >>  drivers/watchdog/s3c2410_wdt.c | 42 
> > > >> +++---
> > > >>  1 file changed, 39 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/drivers/watchdog/s3c2410_wdt.c 
> > > >> b/drivers/watchdog/s3c2410_wdt.c
> > > >> index 47f4dcf..2c87d37 100644
> > > >> --- a/drivers/watchdog/s3c2410_wdt.c
> > > >> +++ b/drivers/watchdog/s3c2410_wdt.c
> > > >> @@ -62,9 +62,13 @@
> > > >>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
> > > >>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
> > > >>
> > > >> +#define RST_STAT_REG_OFFSET  0x0404
> > > >>  #define WDT_DISABLE_REG_OFFSET   0x0408
> > > >>  #define WDT_MASK_RESET_REG_OFFSET0x040c
> > > >>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
> > > >> +#define QUIRK_HAS_RST_STAT   (1 << 1)
> > > >> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
> > > >> +  QUIRK_HAS_RST_STAT)
> > > >>
> > > > I am not really happy about the NEED (both of them, really) here.
> > > > How about HAS instead ?
> > >
> > > Ah, I just commented on these things on our internal review site too
> > > on this patch. I don't even think a quirk is needed -- just use the
> > > presence of a non-0 rst_stat_reg to tell if you need to use regmap.
> > >
> > Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
> > as well.
> >
> 
> As Tomasz Figa suggested I introduced quirks,
> 
'quirk' implies a workaround for non-standard or broken hardware,
not a flag indicating device specific functionality.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote:
 Hi Guenter Roeck,
 
 On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck li...@roeck-us.net wrote:
 
  On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
   On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net wrote:
On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
A good watchdog driver is supposed to report when it was responsible
for resetting the system.  Implement this for the s3c2410, at least on
exynos5250 and exynos5420 where we already have a pointer to the PMU
registers to read the information.
   
Signed-off-by: Doug Anderson diand...@chromium.org
---
This patch is based atop Leela Krishna's recent series that ends with
(ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
AKA https://patchwork.kernel.org/patch/3251861/.
   
 drivers/watchdog/s3c2410_wdt.c | 42 
+++---
 1 file changed, 39 insertions(+), 3 deletions(-)
   
diff --git a/drivers/watchdog/s3c2410_wdt.c 
b/drivers/watchdog/s3c2410_wdt.c
index 47f4dcf..2c87d37 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -62,9 +62,13 @@
 #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
 #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
   
+#define RST_STAT_REG_OFFSET  0x0404
 #define WDT_DISABLE_REG_OFFSET   0x0408
 #define WDT_MASK_RESET_REG_OFFSET0x040c
 #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
+#define QUIRK_HAS_RST_STAT   (1  1)
+#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
+  QUIRK_HAS_RST_STAT)
   
I am not really happy about the NEED (both of them, really) here.
How about HAS instead ?
  
   Ah, I just commented on these things on our internal review site too
   on this patch. I don't even think a quirk is needed -- just use the
   presence of a non-0 rst_stat_reg to tell if you need to use regmap.
  
  Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
  as well.
 
 
 As Tomasz Figa suggested I introduced quirks,
 
'quirk' implies a workaround for non-standard or broken hardware,
not a flag indicating device specific functionality.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Tomasz Figa]

On Thursday 05 of December 2013 08:00:27 Guenter Roeck wrote:
 On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote:
  Hi Guenter Roeck,
  
  On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck li...@roeck-us.net wrote:
  
   On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net 
wrote:
 On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least 
 on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.

 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 This patch is based atop Leela Krishna's recent series that ends with
 (ARM: dts: update watchdog device nodes for Exynos5250 and 
 Exynos5420)
 AKA https://patchwork.kernel.org/patch/3251861/.

  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)

 diff --git a/drivers/watchdog/s3c2410_wdt.c 
 b/drivers/watchdog/s3c2410_wdt.c
 index 47f4dcf..2c87d37 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,13 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

 +#define RST_STAT_REG_OFFSET  0x0404
  #define WDT_DISABLE_REG_OFFSET   0x0408
  #define WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)

 I am not really happy about the NEED (both of them, really) here.
 How about HAS instead ?
   
Ah, I just commented on these things on our internal review site too
on this patch. I don't even think a quirk is needed -- just use the
presence of a non-0 rst_stat_reg to tell if you need to use regmap.
   
   Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
   as well.
  
  
  As Tomasz Figa suggested I introduced quirks,
  
 'quirk' implies a workaround for non-standard or broken hardware,
 not a flag indicating device specific functionality.

I wouldn't limit meaning of quirk to only this. The word has been widely
used around the kernel as a name for differences between hardware
variants.

As for the original issue itself, IMHO Doug's original solution is the
most practical, as 0 can be a valid register offset and RST_STAT register
could be used for other purposes as well in future, depending on other
quirk flag.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Tomasz Figa]

Hi Doug,

Please see my comments inline.

On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.
 
 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 This patch is based atop Leela Krishna's recent series that ends with
 (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
 AKA https://patchwork.kernel.org/patch/3251861/.
 
  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 47f4dcf..2c87d37 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,13 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
  
 +#define RST_STAT_REG_OFFSET  0x0404

IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
registers below should be as well, but I missed this in the patch adding
them.

  #define WDT_DISABLE_REG_OFFSET   0x0408
  #define WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)
  
  static bool nowayout = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
 @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
 debug (default 0));
   * timer reset functionality.
   * @mask_bit: Bit number for the watchdog timer in the disable register and 
 the
   * mask reset register.
 + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
 status.
 + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
 + * reset.
   * @quirks: A bitfield of quirks.
   */
  
 @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
   int disable_reg;
   int mask_reset_reg;
   int mask_bit;
 + int rst_stat_reg;
 + int rst_stat_bit;
   u32 quirks;
  };
  
 @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
 drv_data_exynos5250  = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 20,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 20,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,
  };
  
  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 0,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 9,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,
  };
  
  static const struct of_device_id s3c2410_wdt_match[] = {
 @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
 s3c2410_wdt *wdt)
  }
  #endif
  
 +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 +{
 + unsigned int bootstatus = 0;
 + int ret;
 +
 + if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {

nit: I guess it's just a matter of taste, but to reduce code indentation
you could inverse the check and simply return 0 here.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Thu, Dec 05, 2013 at 05:21:47PM +0100, Tomasz Figa wrote:
 Hi Doug,
 
 Please see my comments inline.
 
 On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
  A good watchdog driver is supposed to report when it was responsible
  for resetting the system.  Implement this for the s3c2410, at least on
  exynos5250 and exynos5420 where we already have a pointer to the PMU
  registers to read the information.
  
  Signed-off-by: Doug Anderson diand...@chromium.org
  ---
  This patch is based atop Leela Krishna's recent series that ends with
  (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
  AKA https://patchwork.kernel.org/patch/3251861/.
  
   drivers/watchdog/s3c2410_wdt.c | 42 
  +++---
   1 file changed, 39 insertions(+), 3 deletions(-)
  
  diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
  index 47f4dcf..2c87d37 100644
  --- a/drivers/watchdog/s3c2410_wdt.c
  +++ b/drivers/watchdog/s3c2410_wdt.c
  @@ -62,9 +62,13 @@
   #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
   #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME   (15)
   
  +#define RST_STAT_REG_OFFSET0x0404
 
 IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
 registers below should be as well, but I missed this in the patch adding
 them.
 
   #define WDT_DISABLE_REG_OFFSET 0x0408
   #define WDT_MASK_RESET_REG_OFFSET  0x040c
   #define QUIRK_NEEDS_PMU_CONFIG (1  0)
  +#define QUIRK_HAS_RST_STAT (1  1)
  +#define QUIRKS_NEED_PMUREG (QUIRK_NEEDS_PMU_CONFIG | \
  +QUIRK_HAS_RST_STAT)
   
   static bool nowayout   = WATCHDOG_NOWAYOUT;
   static int tmr_margin;
  @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
  debug (default 0));
* timer reset functionality.
* @mask_bit: Bit number for the watchdog timer in the disable register 
  and the
* mask reset register.
  + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
  status.
  + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
  + * reset.
* @quirks: A bitfield of quirks.
*/
   
  @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
  int disable_reg;
  int mask_reset_reg;
  int mask_bit;
  +   int rst_stat_reg;
  +   int rst_stat_bit;
  u32 quirks;
   };
   
  @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
  drv_data_exynos5250  = {
  .disable_reg = WDT_DISABLE_REG_OFFSET,
  .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
  .mask_bit = 20,
  -   .quirks = QUIRK_NEEDS_PMU_CONFIG
  +   .rst_stat_reg = RST_STAT_REG_OFFSET,
  +   .rst_stat_bit = 20,
  +   .quirks = QUIRK_NEEDS_PMU_CONFIG |
  +   QUIRK_HAS_RST_STAT,
   };
   
   static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
  .disable_reg = WDT_DISABLE_REG_OFFSET,
  .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
  .mask_bit = 0,
  -   .quirks = QUIRK_NEEDS_PMU_CONFIG
  +   .rst_stat_reg = RST_STAT_REG_OFFSET,
  +   .rst_stat_bit = 9,
  +   .quirks = QUIRK_NEEDS_PMU_CONFIG |
  +   QUIRK_HAS_RST_STAT,
   };
   
   static const struct of_device_id s3c2410_wdt_match[] = {
  @@ -423,6 +438,25 @@ static inline void 
  s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
   }
   #endif
   
  +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
  *wdt)
  +{
  +   unsigned int bootstatus = 0;
  +   int ret;
  +
  +   if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {
 
 nit: I guess it's just a matter of taste, but to reduce code indentation
 you could inverse the check and simply return 0 here.
 

nit: If so, it would make sense to to the same for the other 'quirk'
for consistency.

static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
{
...
if (!(wdt-drv_data-quirks  QUIRK_NEEDS_PMU_CONFIG))
return 0;
...
}

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Tomasz Figa]

On Thursday 05 of December 2013 08:40:38 Guenter Roeck wrote:
 On Thu, Dec 05, 2013 at 05:21:47PM +0100, Tomasz Figa wrote:
  Hi Doug,
  
  Please see my comments inline.
  
  On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
   A good watchdog driver is supposed to report when it was responsible
   for resetting the system.  Implement this for the s3c2410, at least on
   exynos5250 and exynos5420 where we already have a pointer to the PMU
   registers to read the information.
   
   Signed-off-by: Doug Anderson diand...@chromium.org
   ---
   This patch is based atop Leela Krishna's recent series that ends with
   (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
   AKA https://patchwork.kernel.org/patch/3251861/.
   
drivers/watchdog/s3c2410_wdt.c | 42 
   +++---
1 file changed, 39 insertions(+), 3 deletions(-)
   
   diff --git a/drivers/watchdog/s3c2410_wdt.c 
   b/drivers/watchdog/s3c2410_wdt.c
   index 47f4dcf..2c87d37 100644
   --- a/drivers/watchdog/s3c2410_wdt.c
   +++ b/drivers/watchdog/s3c2410_wdt.c
   @@ -62,9 +62,13 @@
#define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
#define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

   +#define RST_STAT_REG_OFFSET  0x0404
  
  IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
  registers below should be as well, but I missed this in the patch adding
  them.
  
#define WDT_DISABLE_REG_OFFSET   0x0408
#define WDT_MASK_RESET_REG_OFFSET0x040c
#define QUIRK_NEEDS_PMU_CONFIG   (1  0)
   +#define QUIRK_HAS_RST_STAT   (1  1)
   +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
   +  QUIRK_HAS_RST_STAT)

static bool nowayout = WATCHDOG_NOWAYOUT;
static int tmr_margin;
   @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
   debug (default 0));
 * timer reset functionality.
 * @mask_bit: Bit number for the watchdog timer in the disable register 
   and the
 * mask reset register.
   + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
   status.
   + * @rst_stat_bit: Bit number in the rst_stat register indicating a 
   watchdog
   + * reset.
 * @quirks: A bitfield of quirks.
 */

   @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
 int disable_reg;
 int mask_reset_reg;
 int mask_bit;
   + int rst_stat_reg;
   + int rst_stat_bit;
 u32 quirks;
};

   @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
   drv_data_exynos5250  = {
 .disable_reg = WDT_DISABLE_REG_OFFSET,
 .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
 .mask_bit = 20,
   - .quirks = QUIRK_NEEDS_PMU_CONFIG
   + .rst_stat_reg = RST_STAT_REG_OFFSET,
   + .rst_stat_bit = 20,
   + .quirks = QUIRK_NEEDS_PMU_CONFIG |
   + QUIRK_HAS_RST_STAT,
};

static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
 .disable_reg = WDT_DISABLE_REG_OFFSET,
 .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
 .mask_bit = 0,
   - .quirks = QUIRK_NEEDS_PMU_CONFIG
   + .rst_stat_reg = RST_STAT_REG_OFFSET,
   + .rst_stat_bit = 9,
   + .quirks = QUIRK_NEEDS_PMU_CONFIG |
   + QUIRK_HAS_RST_STAT,
};

static const struct of_device_id s3c2410_wdt_match[] = {
   @@ -423,6 +438,25 @@ static inline void 
   s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
}
#endif

   +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
   *wdt)
   +{
   + unsigned int bootstatus = 0;
   + int ret;
   +
   + if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {
  
  nit: I guess it's just a matter of taste, but to reduce code indentation
  you could inverse the check and simply return 0 here.
  
 
 nit: If so, it would make sense to to the same for the other 'quirk'
 for consistency.
 
 static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool 
 mask)
 {
   ...
   if (!(wdt-drv_data-quirks  QUIRK_NEEDS_PMU_CONFIG))
   return 0;
   ...
 }

That's quite a bit different story, as currently the check is outside of
this function, but I agree, it would look better.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Guenter,

On Thu, Dec 5, 2013 at 8:40 AM, Guenter Roeck li...@roeck-us.net wrote:
 On Thu, Dec 05, 2013 at 05:21:47PM +0100, Tomasz Figa wrote:
 Hi Doug,

 Please see my comments inline.

 On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
  A good watchdog driver is supposed to report when it was responsible
  for resetting the system.  Implement this for the s3c2410, at least on
  exynos5250 and exynos5420 where we already have a pointer to the PMU
  registers to read the information.
 
  Signed-off-by: Doug Anderson diand...@chromium.org
  ---
  This patch is based atop Leela Krishna's recent series that ends with
  (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
  AKA https://patchwork.kernel.org/patch/3251861/.
 
   drivers/watchdog/s3c2410_wdt.c | 42 
  +++---
   1 file changed, 39 insertions(+), 3 deletions(-)
 
  diff --git a/drivers/watchdog/s3c2410_wdt.c 
  b/drivers/watchdog/s3c2410_wdt.c
  index 47f4dcf..2c87d37 100644
  --- a/drivers/watchdog/s3c2410_wdt.c
  +++ b/drivers/watchdog/s3c2410_wdt.c
  @@ -62,9 +62,13 @@
   #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
   #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME   (15)
 
  +#define RST_STAT_REG_OFFSET0x0404

 IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
 registers below should be as well, but I missed this in the patch adding
 them.

   #define WDT_DISABLE_REG_OFFSET 0x0408
   #define WDT_MASK_RESET_REG_OFFSET  0x040c
   #define QUIRK_NEEDS_PMU_CONFIG (1  0)
  +#define QUIRK_HAS_RST_STAT (1  1)
  +#define QUIRKS_NEED_PMUREG (QUIRK_NEEDS_PMU_CONFIG | \
  +QUIRK_HAS_RST_STAT)
 
   static bool nowayout   = WATCHDOG_NOWAYOUT;
   static int tmr_margin;
  @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
  debug (default 0));
* timer reset functionality.
* @mask_bit: Bit number for the watchdog timer in the disable register 
  and the
* mask reset register.
  + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
  status.
  + * @rst_stat_bit: Bit number in the rst_stat register indicating a 
  watchdog
  + * reset.
* @quirks: A bitfield of quirks.
*/
 
  @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
  int disable_reg;
  int mask_reset_reg;
  int mask_bit;
  +   int rst_stat_reg;
  +   int rst_stat_bit;
  u32 quirks;
   };
 
  @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
  drv_data_exynos5250  = {
  .disable_reg = WDT_DISABLE_REG_OFFSET,
  .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
  .mask_bit = 20,
  -   .quirks = QUIRK_NEEDS_PMU_CONFIG
  +   .rst_stat_reg = RST_STAT_REG_OFFSET,
  +   .rst_stat_bit = 20,
  +   .quirks = QUIRK_NEEDS_PMU_CONFIG |
  +   QUIRK_HAS_RST_STAT,
   };
 
   static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
  .disable_reg = WDT_DISABLE_REG_OFFSET,
  .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
  .mask_bit = 0,
  -   .quirks = QUIRK_NEEDS_PMU_CONFIG
  +   .rst_stat_reg = RST_STAT_REG_OFFSET,
  +   .rst_stat_bit = 9,
  +   .quirks = QUIRK_NEEDS_PMU_CONFIG |
  +   QUIRK_HAS_RST_STAT,
   };
 
   static const struct of_device_id s3c2410_wdt_match[] = {
  @@ -423,6 +438,25 @@ static inline void 
  s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
   }
   #endif
 
  +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
  *wdt)
  +{
  +   unsigned int bootstatus = 0;
  +   int ret;
  +
  +   if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {

 nit: I guess it's just a matter of taste, but to reduce code indentation
 you could inverse the check and simply return 0 here.


 nit: If so, it would make sense to to the same for the other 'quirk'
 for consistency.

 static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool 
 mask)
 {
 ...
 if (!(wdt-drv_data-quirks  QUIRK_NEEDS_PMU_CONFIG))
 return 0;
 ...
 }

Done in my proposed fixup for Leela Krishna's patch.

-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Tomasz

On Thu, Dec 5, 2013 at 8:18 AM, Tomasz Figa t.f...@samsung.com wrote:
 On Thursday 05 of December 2013 08:00:27 Guenter Roeck wrote:
 On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote:
  Hi Guenter Roeck,
 
  On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck li...@roeck-us.net wrote:
  
   On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net 
wrote:
 On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least 
 on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.

 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 This patch is based atop Leela Krishna's recent series that ends 
 with
 (ARM: dts: update watchdog device nodes for Exynos5250 and 
 Exynos5420)
 AKA https://patchwork.kernel.org/patch/3251861/.

  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)

 diff --git a/drivers/watchdog/s3c2410_wdt.c 
 b/drivers/watchdog/s3c2410_wdt.c
 index 47f4dcf..2c87d37 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,13 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

 +#define RST_STAT_REG_OFFSET  0x0404
  #define WDT_DISABLE_REG_OFFSET   0x0408
  #define WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)

 I am not really happy about the NEED (both of them, really) here.
 How about HAS instead ?
   
Ah, I just commented on these things on our internal review site too
on this patch. I don't even think a quirk is needed -- just use the
presence of a non-0 rst_stat_reg to tell if you need to use regmap.
   
   Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
   as well.
  
 
  As Tomasz Figa suggested I introduced quirks,
 
 'quirk' implies a workaround for non-standard or broken hardware,
 not a flag indicating device specific functionality.

 I wouldn't limit meaning of quirk to only this. The word has been widely
 used around the kernel as a name for differences between hardware
 variants.

 As for the original issue itself, IMHO Doug's original solution is the
 most practical, as 0 can be a valid register offset and RST_STAT register
 could be used for other purposes as well in future, depending on other
 quirk flag.

OK, I'm keeping my concept but adjusting the names.  Version 2 coming
up shortly.

-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Tomasz,

On Thu, Dec 5, 2013 at 8:21 AM, Tomasz Figa t.f...@samsung.com wrote:
 Hi Doug,

 Please see my comments inline.

 On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.

 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 This patch is based atop Leela Krishna's recent series that ends with
 (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
 AKA https://patchwork.kernel.org/patch/3251861/.

  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)

 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 47f4dcf..2c87d37 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,13 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

 +#define RST_STAT_REG_OFFSET  0x0404

 IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
 registers below should be as well, but I missed this in the patch adding
 them.

Done in v2 (plus fixup of Leela Krishna's).


  #define WDT_DISABLE_REG_OFFSET   0x0408
  #define WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)

  static bool nowayout = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
 @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
 debug (default 0));
   * timer reset functionality.
   * @mask_bit: Bit number for the watchdog timer in the disable register and 
 the
   * mask reset register.
 + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
 status.
 + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
 + * reset.
   * @quirks: A bitfield of quirks.
   */

 @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
   int disable_reg;
   int mask_reset_reg;
   int mask_bit;
 + int rst_stat_reg;
 + int rst_stat_bit;
   u32 quirks;
  };

 @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
 drv_data_exynos5250  = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 20,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 20,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,
  };

  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 0,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 9,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,
  };

  static const struct of_device_id s3c2410_wdt_match[] = {
 @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
 s3c2410_wdt *wdt)
  }
  #endif

 +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
 *wdt)
 +{
 + unsigned int bootstatus = 0;
 + int ret;
 +
 + if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {

 nit: I guess it's just a matter of taste, but to reduce code indentation
 you could inverse the check and simply return 0 here.

Done in v2.  This is the same suggestion Olof made.

-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Guenter and Olof,

On Mon, Dec 2, 2013 at 1:36 PM, Guenter Roeck li...@roeck-us.net wrote:
 On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
 On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net wrote:
  On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
  A good watchdog driver is supposed to report when it was responsible
  for resetting the system.  Implement this for the s3c2410, at least on
  exynos5250 and exynos5420 where we already have a pointer to the PMU
  registers to read the information.
 
  Signed-off-by: Doug Anderson diand...@chromium.org
  ---
  This patch is based atop Leela Krishna's recent series that ends with
  (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
  AKA https://patchwork.kernel.org/patch/3251861/.
 
   drivers/watchdog/s3c2410_wdt.c | 42 
  +++---
   1 file changed, 39 insertions(+), 3 deletions(-)
 
  diff --git a/drivers/watchdog/s3c2410_wdt.c 
  b/drivers/watchdog/s3c2410_wdt.c
  index 47f4dcf..2c87d37 100644
  --- a/drivers/watchdog/s3c2410_wdt.c
  +++ b/drivers/watchdog/s3c2410_wdt.c
  @@ -62,9 +62,13 @@
   #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
   #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
 
  +#define RST_STAT_REG_OFFSET  0x0404
   #define WDT_DISABLE_REG_OFFSET   0x0408
   #define WDT_MASK_RESET_REG_OFFSET0x040c
   #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
  +#define QUIRK_HAS_RST_STAT   (1  1)
  +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
  +  QUIRK_HAS_RST_STAT)
 
  I am not really happy about the NEED (both of them, really) here.
  How about HAS instead ?

 Ah, I just commented on these things on our internal review site too
 on this patch. I don't even think a quirk is needed -- just use the
 presence of a non-0 rst_stat_reg to tell if you need to use regmap.

 Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
 as well.

I have changed all instances of NEED / NEEDS and HAS / HAVE.  Please
take a look.  Since some of those changes applied to Leela Krishna's
patch I'm sending up a fixup patch which hopefully he can
incorporate into a v12.

-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Leela Krishna Amudala]

Hi Guenter Roeck,

On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck  wrote:
>
> On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
> > On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck  wrote:
> > > On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
> > >> A good watchdog driver is supposed to report when it was responsible
> > >> for resetting the system.  Implement this for the s3c2410, at least on
> > >> exynos5250 and exynos5420 where we already have a pointer to the PMU
> > >> registers to read the information.
> > >>
> > >> Signed-off-by: Doug Anderson 
> > >> ---
> > >> This patch is based atop Leela Krishna's recent series that ends with
> > >> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> > >> AKA .
> > >>
> > >>  drivers/watchdog/s3c2410_wdt.c | 42 
> > >> +++---
> > >>  1 file changed, 39 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/watchdog/s3c2410_wdt.c 
> > >> b/drivers/watchdog/s3c2410_wdt.c
> > >> index 47f4dcf..2c87d37 100644
> > >> --- a/drivers/watchdog/s3c2410_wdt.c
> > >> +++ b/drivers/watchdog/s3c2410_wdt.c
> > >> @@ -62,9 +62,13 @@
> > >>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
> > >>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
> > >>
> > >> +#define RST_STAT_REG_OFFSET  0x0404
> > >>  #define WDT_DISABLE_REG_OFFSET   0x0408
> > >>  #define WDT_MASK_RESET_REG_OFFSET0x040c
> > >>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
> > >> +#define QUIRK_HAS_RST_STAT   (1 << 1)
> > >> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
> > >> +  QUIRK_HAS_RST_STAT)
> > >>
> > > I am not really happy about the NEED (both of them, really) here.
> > > How about HAS instead ?
> >
> > Ah, I just commented on these things on our internal review site too
> > on this patch. I don't even think a quirk is needed -- just use the
> > presence of a non-0 rst_stat_reg to tell if you need to use regmap.
> >
> Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
> as well.
>

As Tomasz Figa suggested I introduced quirks,

Tomasz,
Any comments from you here...??


Best wishes,
Leela Krishna


> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Leela Krishna Amudala]

Hi Guenter Roeck,

On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck li...@roeck-us.net wrote:

 On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
  On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net wrote:
   On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
   A good watchdog driver is supposed to report when it was responsible
   for resetting the system.  Implement this for the s3c2410, at least on
   exynos5250 and exynos5420 where we already have a pointer to the PMU
   registers to read the information.
  
   Signed-off-by: Doug Anderson diand...@chromium.org
   ---
   This patch is based atop Leela Krishna's recent series that ends with
   (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
   AKA https://patchwork.kernel.org/patch/3251861/.
  
drivers/watchdog/s3c2410_wdt.c | 42 
   +++---
1 file changed, 39 insertions(+), 3 deletions(-)
  
   diff --git a/drivers/watchdog/s3c2410_wdt.c 
   b/drivers/watchdog/s3c2410_wdt.c
   index 47f4dcf..2c87d37 100644
   --- a/drivers/watchdog/s3c2410_wdt.c
   +++ b/drivers/watchdog/s3c2410_wdt.c
   @@ -62,9 +62,13 @@
#define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
#define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
  
   +#define RST_STAT_REG_OFFSET  0x0404
#define WDT_DISABLE_REG_OFFSET   0x0408
#define WDT_MASK_RESET_REG_OFFSET0x040c
#define QUIRK_NEEDS_PMU_CONFIG   (1  0)
   +#define QUIRK_HAS_RST_STAT   (1  1)
   +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
   +  QUIRK_HAS_RST_STAT)
  
   I am not really happy about the NEED (both of them, really) here.
   How about HAS instead ?
 
  Ah, I just commented on these things on our internal review site too
  on this patch. I don't even think a quirk is needed -- just use the
  presence of a non-0 rst_stat_reg to tell if you need to use regmap.
 
 Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
 as well.


As Tomasz Figa suggested I introduced quirks,

Tomasz,
Any comments from you here...??


Best wishes,
Leela Krishna


 Guenter
 --
 To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 
 in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
> On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck  wrote:
> > On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
> >> A good watchdog driver is supposed to report when it was responsible
> >> for resetting the system.  Implement this for the s3c2410, at least on
> >> exynos5250 and exynos5420 where we already have a pointer to the PMU
> >> registers to read the information.
> >>
> >> Signed-off-by: Doug Anderson 
> >> ---
> >> This patch is based atop Leela Krishna's recent series that ends with
> >> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> >> AKA .
> >>
> >>  drivers/watchdog/s3c2410_wdt.c | 42 
> >> +++---
> >>  1 file changed, 39 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/s3c2410_wdt.c 
> >> b/drivers/watchdog/s3c2410_wdt.c
> >> index 47f4dcf..2c87d37 100644
> >> --- a/drivers/watchdog/s3c2410_wdt.c
> >> +++ b/drivers/watchdog/s3c2410_wdt.c
> >> @@ -62,9 +62,13 @@
> >>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
> >>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
> >>
> >> +#define RST_STAT_REG_OFFSET  0x0404
> >>  #define WDT_DISABLE_REG_OFFSET   0x0408
> >>  #define WDT_MASK_RESET_REG_OFFSET0x040c
> >>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
> >> +#define QUIRK_HAS_RST_STAT   (1 << 1)
> >> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
> >> +  QUIRK_HAS_RST_STAT)
> >>
> > I am not really happy about the NEED (both of them, really) here.
> > How about HAS instead ?
> 
> Ah, I just commented on these things on our internal review site too
> on this patch. I don't even think a quirk is needed -- just use the
> presence of a non-0 rst_stat_reg to tell if you need to use regmap.
> 
Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
as well.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Guenter and Olof,

On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck  wrote:
> On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
>> A good watchdog driver is supposed to report when it was responsible
>> for resetting the system.  Implement this for the s3c2410, at least on
>> exynos5250 and exynos5420 where we already have a pointer to the PMU
>> registers to read the information.
>>
>> Signed-off-by: Doug Anderson 
>> ---
>> This patch is based atop Leela Krishna's recent series that ends with
>> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
>> AKA .
>>
>>  drivers/watchdog/s3c2410_wdt.c | 42 
>> +++---
>>  1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>> index 47f4dcf..2c87d37 100644
>> --- a/drivers/watchdog/s3c2410_wdt.c
>> +++ b/drivers/watchdog/s3c2410_wdt.c
>> @@ -62,9 +62,13 @@
>>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
>>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
>>
>> +#define RST_STAT_REG_OFFSET  0x0404
>>  #define WDT_DISABLE_REG_OFFSET   0x0408
>>  #define WDT_MASK_RESET_REG_OFFSET0x040c
>>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
>> +#define QUIRK_HAS_RST_STAT   (1 << 1)
>> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
>> +  QUIRK_HAS_RST_STAT)
>>
> I am not really happy about the NEED (both of them, really) here.
> How about HAS instead ?

Are you suggesting also changing QUIRK_NEEDS_PMU_CONFIG, then?  That
would be a request for Leela Krishna on his patch?

...see below for more...

>
>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>>  static int tmr_margin;
>> @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for 
>> debug (default 0)");
>>   * timer reset functionality.
>>   * @mask_bit: Bit number for the watchdog timer in the disable register and 
>> the
>>   * mask reset register.
>> + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
>> status.
>> + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
>> + * reset.
>>   * @quirks: A bitfield of quirks.
>>   */
>>
>> @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
>>   int disable_reg;
>>   int mask_reset_reg;
>>   int mask_bit;
>> + int rst_stat_reg;
>> + int rst_stat_bit;
>>   u32 quirks;
>>  };
>>
>> @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
>> drv_data_exynos5250  = {
>>   .disable_reg = WDT_DISABLE_REG_OFFSET,
>>   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>>   .mask_bit = 20,
>> - .quirks = QUIRK_NEEDS_PMU_CONFIG
>> + .rst_stat_reg = RST_STAT_REG_OFFSET,
>> + .rst_stat_bit = 20,
>> + .quirks = QUIRK_NEEDS_PMU_CONFIG |
>> + QUIRK_HAS_RST_STAT,
>
> Why not use QUIRKS_NEED_PMUREG ?
>
> Also, is there ever a chance that the two bits don't come together ?
> If not a single bit might be sufficient.

Here's my logic:

According to our 3.4 sources (exynos_get_bootstatus) there is a
RST_STAT register on exynos4. See
.

According to Tomasz at
 the extra
PMU_CONFIG was needed on exynos5250/5420 and not needed on exynos4.

My patch doesn't actually add RST_STAT support for exynos4 but it
seems wise to pave the way for someone else to add it.


...so basically I was saying:

* On exynos5250 and exynos5420 we _need_ to configure the PMU to get
proper functioning

* On various devices we _have_ a reset status that register that we can query.

* If we need to use the PMU or we want to query the reset status we
need to have PMU registers specified.


Does any of that change your mind(s)?


>>  static const struct of_device_id s3c2410_wdt_match[] = {
>> @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
>> s3c2410_wdt *wdt)
>>  }
>>  #endif
>>
>> +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
>> *wdt)
>> +{
>> + unsigned int bootstatus = 0;
>> + int ret;
>> +
>> + if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {

Internally Olof requested to reverse this "if" and return 0 early
(avoid indentation).  I'll fix that up for the next patch.


>> + unsigned int rst_stat;
>> +
>> + ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg,
>> +   _stat);
>> + if (ret)
>> + dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>> + else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
>> + bootstatus |= WDIOF_CARDRESET;
>> + }
>> +
>> + return bootstatus;
>> +}
>> +
--
To unsubscribe from this list: send the line "unsubscribe 

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Olof Johansson]

On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck  wrote:
> On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
>> A good watchdog driver is supposed to report when it was responsible
>> for resetting the system.  Implement this for the s3c2410, at least on
>> exynos5250 and exynos5420 where we already have a pointer to the PMU
>> registers to read the information.
>>
>> Signed-off-by: Doug Anderson 
>> ---
>> This patch is based atop Leela Krishna's recent series that ends with
>> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
>> AKA .
>>
>>  drivers/watchdog/s3c2410_wdt.c | 42 
>> +++---
>>  1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>> index 47f4dcf..2c87d37 100644
>> --- a/drivers/watchdog/s3c2410_wdt.c
>> +++ b/drivers/watchdog/s3c2410_wdt.c
>> @@ -62,9 +62,13 @@
>>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
>>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
>>
>> +#define RST_STAT_REG_OFFSET  0x0404
>>  #define WDT_DISABLE_REG_OFFSET   0x0408
>>  #define WDT_MASK_RESET_REG_OFFSET0x040c
>>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
>> +#define QUIRK_HAS_RST_STAT   (1 << 1)
>> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
>> +  QUIRK_HAS_RST_STAT)
>>
> I am not really happy about the NEED (both of them, really) here.
> How about HAS instead ?

Ah, I just commented on these things on our internal review site too
on this patch. I don't even think a quirk is needed -- just use the
presence of a non-0 rst_stat_reg to tell if you need to use regmap.


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
> A good watchdog driver is supposed to report when it was responsible
> for resetting the system.  Implement this for the s3c2410, at least on
> exynos5250 and exynos5420 where we already have a pointer to the PMU
> registers to read the information.
> 
> Signed-off-by: Doug Anderson 
> ---
> This patch is based atop Leela Krishna's recent series that ends with
> (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> AKA .
> 
>  drivers/watchdog/s3c2410_wdt.c | 42 
> +++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 47f4dcf..2c87d37 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -62,9 +62,13 @@
>  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
>  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
>  
> +#define RST_STAT_REG_OFFSET  0x0404
>  #define WDT_DISABLE_REG_OFFSET   0x0408
>  #define WDT_MASK_RESET_REG_OFFSET0x040c
>  #define QUIRK_NEEDS_PMU_CONFIG   (1 << 0)
> +#define QUIRK_HAS_RST_STAT   (1 << 1)
> +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
> +  QUIRK_HAS_RST_STAT)
>  
I am not really happy about the NEED (both of them, really) here.
How about HAS instead ?

>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  static int tmr_margin;
> @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for 
> debug (default 0)");
>   * timer reset functionality.
>   * @mask_bit: Bit number for the watchdog timer in the disable register and 
> the
>   * mask reset register.
> + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
> status.
> + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
> + * reset.
>   * @quirks: A bitfield of quirks.
>   */
>  
> @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
>   int disable_reg;
>   int mask_reset_reg;
>   int mask_bit;
> + int rst_stat_reg;
> + int rst_stat_bit;
>   u32 quirks;
>  };
>  
> @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
> drv_data_exynos5250  = {
>   .disable_reg = WDT_DISABLE_REG_OFFSET,
>   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>   .mask_bit = 20,
> - .quirks = QUIRK_NEEDS_PMU_CONFIG
> + .rst_stat_reg = RST_STAT_REG_OFFSET,
> + .rst_stat_bit = 20,
> + .quirks = QUIRK_NEEDS_PMU_CONFIG |
> + QUIRK_HAS_RST_STAT,

Why not use QUIRKS_NEED_PMUREG ?

Also, is there ever a chance that the two bits don't come together ?
If not a single bit might be sufficient.

Thanks,
Guenter

>  };
>  
>  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>   .disable_reg = WDT_DISABLE_REG_OFFSET,
>   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>   .mask_bit = 0,
> - .quirks = QUIRK_NEEDS_PMU_CONFIG
> + .rst_stat_reg = RST_STAT_REG_OFFSET,
> + .rst_stat_bit = 9,
> + .quirks = QUIRK_NEEDS_PMU_CONFIG |
> + QUIRK_HAS_RST_STAT,
>  };
>  
>  static const struct of_device_id s3c2410_wdt_match[] = {
> @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
> s3c2410_wdt *wdt)
>  }
>  #endif
>  
> +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
> +{
> + unsigned int bootstatus = 0;
> + int ret;
> +
> + if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {
> + unsigned int rst_stat;
> +
> + ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg,
> +   _stat);
> + if (ret)
> + dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> + else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> + bootstatus |= WDIOF_CARDRESET;
> + }
> +
> + return bootstatus;
> +}
> +
>  /* s3c2410_get_wdt_driver_data */
>  static inline struct s3c2410_wdt_variant *
>  get_wdt_drv_data(struct platform_device *pdev)
> @@ -460,7 +494,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   wdt->wdt_device = s3c2410_wdd;
>  
>   wdt->drv_data = get_wdt_drv_data(pdev);
> - if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
> + if (wdt->drv_data->quirks & QUIRKS_NEED_PMUREG) {
>   wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>   
> "samsung,syscon-phandle");
>   if (IS_ERR(wdt->pmureg)) {
> @@ -531,6 +565,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>  
>   watchdog_set_nowayout(>wdt_device, nowayout);
>  
> + wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
> +
>   ret = watchdog_register_device(>wdt_device);
>   if (ret) {
>   dev_err(dev, 

[PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

A good watchdog driver is supposed to report when it was responsible
for resetting the system.  Implement this for the s3c2410, at least on
exynos5250 and exynos5420 where we already have a pointer to the PMU
registers to read the information.

Signed-off-by: Doug Anderson 
---
This patch is based atop Leela Krishna's recent series that ends with
(ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
AKA .

 drivers/watchdog/s3c2410_wdt.c | 42 +++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 47f4dcf..2c87d37 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -62,9 +62,13 @@
 #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
 #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME   (15)
 
+#define RST_STAT_REG_OFFSET0x0404
 #define WDT_DISABLE_REG_OFFSET 0x0408
 #define WDT_MASK_RESET_REG_OFFSET  0x040c
 #define QUIRK_NEEDS_PMU_CONFIG (1 << 0)
+#define QUIRK_HAS_RST_STAT (1 << 1)
+#define QUIRKS_NEED_PMUREG (QUIRK_NEEDS_PMU_CONFIG | \
+QUIRK_HAS_RST_STAT)
 
 static bool nowayout   = WATCHDOG_NOWAYOUT;
 static int tmr_margin;
@@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug 
(default 0)");
  * timer reset functionality.
  * @mask_bit: Bit number for the watchdog timer in the disable register and the
  * mask reset register.
+ * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
+ * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
+ * reset.
  * @quirks: A bitfield of quirks.
  */
 
@@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
int disable_reg;
int mask_reset_reg;
int mask_bit;
+   int rst_stat_reg;
+   int rst_stat_bit;
u32 quirks;
 };
 
@@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
drv_data_exynos5250  = {
.disable_reg = WDT_DISABLE_REG_OFFSET,
.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 20,
-   .quirks = QUIRK_NEEDS_PMU_CONFIG
+   .rst_stat_reg = RST_STAT_REG_OFFSET,
+   .rst_stat_bit = 20,
+   .quirks = QUIRK_NEEDS_PMU_CONFIG |
+   QUIRK_HAS_RST_STAT,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
.disable_reg = WDT_DISABLE_REG_OFFSET,
.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 0,
-   .quirks = QUIRK_NEEDS_PMU_CONFIG
+   .rst_stat_reg = RST_STAT_REG_OFFSET,
+   .rst_stat_bit = 9,
+   .quirks = QUIRK_NEEDS_PMU_CONFIG |
+   QUIRK_HAS_RST_STAT,
 };
 
 static const struct of_device_id s3c2410_wdt_match[] = {
@@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
s3c2410_wdt *wdt)
 }
 #endif
 
+static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
+{
+   unsigned int bootstatus = 0;
+   int ret;
+
+   if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {
+   unsigned int rst_stat;
+
+   ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg,
+ _stat);
+   if (ret)
+   dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
+   else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
+   bootstatus |= WDIOF_CARDRESET;
+   }
+
+   return bootstatus;
+}
+
 /* s3c2410_get_wdt_driver_data */
 static inline struct s3c2410_wdt_variant *
 get_wdt_drv_data(struct platform_device *pdev)
@@ -460,7 +494,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
wdt->wdt_device = s3c2410_wdd;
 
wdt->drv_data = get_wdt_drv_data(pdev);
-   if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
+   if (wdt->drv_data->quirks & QUIRKS_NEED_PMUREG) {
wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,

"samsung,syscon-phandle");
if (IS_ERR(wdt->pmureg)) {
@@ -531,6 +565,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 
watchdog_set_nowayout(>wdt_device, nowayout);
 
+   wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
+
ret = watchdog_register_device(>wdt_device);
if (ret) {
dev_err(dev, "cannot register watchdog (%d)\n", ret);
-- 
1.8.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

A good watchdog driver is supposed to report when it was responsible
for resetting the system.  Implement this for the s3c2410, at least on
exynos5250 and exynos5420 where we already have a pointer to the PMU
registers to read the information.

Signed-off-by: Doug Anderson diand...@chromium.org
---
This patch is based atop Leela Krishna's recent series that ends with
(ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
AKA https://patchwork.kernel.org/patch/3251861/.

 drivers/watchdog/s3c2410_wdt.c | 42 +++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 47f4dcf..2c87d37 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -62,9 +62,13 @@
 #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
 #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME   (15)
 
+#define RST_STAT_REG_OFFSET0x0404
 #define WDT_DISABLE_REG_OFFSET 0x0408
 #define WDT_MASK_RESET_REG_OFFSET  0x040c
 #define QUIRK_NEEDS_PMU_CONFIG (1  0)
+#define QUIRK_HAS_RST_STAT (1  1)
+#define QUIRKS_NEED_PMUREG (QUIRK_NEEDS_PMU_CONFIG | \
+QUIRK_HAS_RST_STAT)
 
 static bool nowayout   = WATCHDOG_NOWAYOUT;
 static int tmr_margin;
@@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for debug 
(default 0));
  * timer reset functionality.
  * @mask_bit: Bit number for the watchdog timer in the disable register and the
  * mask reset register.
+ * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
+ * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
+ * reset.
  * @quirks: A bitfield of quirks.
  */
 
@@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
int disable_reg;
int mask_reset_reg;
int mask_bit;
+   int rst_stat_reg;
+   int rst_stat_bit;
u32 quirks;
 };
 
@@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
drv_data_exynos5250  = {
.disable_reg = WDT_DISABLE_REG_OFFSET,
.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 20,
-   .quirks = QUIRK_NEEDS_PMU_CONFIG
+   .rst_stat_reg = RST_STAT_REG_OFFSET,
+   .rst_stat_bit = 20,
+   .quirks = QUIRK_NEEDS_PMU_CONFIG |
+   QUIRK_HAS_RST_STAT,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
.disable_reg = WDT_DISABLE_REG_OFFSET,
.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
.mask_bit = 0,
-   .quirks = QUIRK_NEEDS_PMU_CONFIG
+   .rst_stat_reg = RST_STAT_REG_OFFSET,
+   .rst_stat_bit = 9,
+   .quirks = QUIRK_NEEDS_PMU_CONFIG |
+   QUIRK_HAS_RST_STAT,
 };
 
 static const struct of_device_id s3c2410_wdt_match[] = {
@@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
s3c2410_wdt *wdt)
 }
 #endif
 
+static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
+{
+   unsigned int bootstatus = 0;
+   int ret;
+
+   if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {
+   unsigned int rst_stat;
+
+   ret = regmap_read(wdt-pmureg, wdt-drv_data-rst_stat_reg,
+ rst_stat);
+   if (ret)
+   dev_warn(wdt-dev, Couldn't get RST_STAT register\n);
+   else if (rst_stat  BIT(wdt-drv_data-rst_stat_bit))
+   bootstatus |= WDIOF_CARDRESET;
+   }
+
+   return bootstatus;
+}
+
 /* s3c2410_get_wdt_driver_data */
 static inline struct s3c2410_wdt_variant *
 get_wdt_drv_data(struct platform_device *pdev)
@@ -460,7 +494,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
wdt-wdt_device = s3c2410_wdd;
 
wdt-drv_data = get_wdt_drv_data(pdev);
-   if (wdt-drv_data-quirks  QUIRK_NEEDS_PMU_CONFIG) {
+   if (wdt-drv_data-quirks  QUIRKS_NEED_PMUREG) {
wdt-pmureg = syscon_regmap_lookup_by_phandle(dev-of_node,

samsung,syscon-phandle);
if (IS_ERR(wdt-pmureg)) {
@@ -531,6 +565,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 
watchdog_set_nowayout(wdt-wdt_device, nowayout);
 
+   wdt-wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
+
ret = watchdog_register_device(wdt-wdt_device);
if (ret) {
dev_err(dev, cannot register watchdog (%d)\n, ret);
-- 
1.8.4.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.
 
 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 This patch is based atop Leela Krishna's recent series that ends with
 (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
 AKA https://patchwork.kernel.org/patch/3251861/.
 
  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 47f4dcf..2c87d37 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,13 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
  
 +#define RST_STAT_REG_OFFSET  0x0404
  #define WDT_DISABLE_REG_OFFSET   0x0408
  #define WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)
  
I am not really happy about the NEED (both of them, really) here.
How about HAS instead ?

  static bool nowayout = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
 @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
 debug (default 0));
   * timer reset functionality.
   * @mask_bit: Bit number for the watchdog timer in the disable register and 
 the
   * mask reset register.
 + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
 status.
 + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
 + * reset.
   * @quirks: A bitfield of quirks.
   */
  
 @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
   int disable_reg;
   int mask_reset_reg;
   int mask_bit;
 + int rst_stat_reg;
 + int rst_stat_bit;
   u32 quirks;
  };
  
 @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
 drv_data_exynos5250  = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 20,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 20,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,

Why not use QUIRKS_NEED_PMUREG ?

Also, is there ever a chance that the two bits don't come together ?
If not a single bit might be sufficient.

Thanks,
Guenter

  };
  
  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 0,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 9,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,
  };
  
  static const struct of_device_id s3c2410_wdt_match[] = {
 @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
 s3c2410_wdt *wdt)
  }
  #endif
  
 +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 +{
 + unsigned int bootstatus = 0;
 + int ret;
 +
 + if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {
 + unsigned int rst_stat;
 +
 + ret = regmap_read(wdt-pmureg, wdt-drv_data-rst_stat_reg,
 +   rst_stat);
 + if (ret)
 + dev_warn(wdt-dev, Couldn't get RST_STAT register\n);
 + else if (rst_stat  BIT(wdt-drv_data-rst_stat_bit))
 + bootstatus |= WDIOF_CARDRESET;
 + }
 +
 + return bootstatus;
 +}
 +
  /* s3c2410_get_wdt_driver_data */
  static inline struct s3c2410_wdt_variant *
  get_wdt_drv_data(struct platform_device *pdev)
 @@ -460,7 +494,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
   wdt-wdt_device = s3c2410_wdd;
  
   wdt-drv_data = get_wdt_drv_data(pdev);
 - if (wdt-drv_data-quirks  QUIRK_NEEDS_PMU_CONFIG) {
 + if (wdt-drv_data-quirks  QUIRKS_NEED_PMUREG) {
   wdt-pmureg = syscon_regmap_lookup_by_phandle(dev-of_node,
   
 samsung,syscon-phandle);
   if (IS_ERR(wdt-pmureg)) {
 @@ -531,6 +565,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
  
   watchdog_set_nowayout(wdt-wdt_device, nowayout);
  
 + wdt-wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
 +
   ret = watchdog_register_device(wdt-wdt_device);
   if (ret) {
   dev_err(dev, cannot register watchdog (%d)\n, ret);
 -- 
 1.8.4.1
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Olof Johansson]

On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net wrote:
 On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.

 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 This patch is based atop Leela Krishna's recent series that ends with
 (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
 AKA https://patchwork.kernel.org/patch/3251861/.

  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)

 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 47f4dcf..2c87d37 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,13 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

 +#define RST_STAT_REG_OFFSET  0x0404
  #define WDT_DISABLE_REG_OFFSET   0x0408
  #define WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)

 I am not really happy about the NEED (both of them, really) here.
 How about HAS instead ?

Ah, I just commented on these things on our internal review site too
on this patch. I don't even think a quirk is needed -- just use the
presence of a non-0 rst_stat_reg to tell if you need to use regmap.


-Olof
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Doug Anderson]

Guenter and Olof,

On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net wrote:
 On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.

 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 This patch is based atop Leela Krishna's recent series that ends with
 (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
 AKA https://patchwork.kernel.org/patch/3251861/.

  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)

 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 47f4dcf..2c87d37 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,13 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

 +#define RST_STAT_REG_OFFSET  0x0404
  #define WDT_DISABLE_REG_OFFSET   0x0408
  #define WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)

 I am not really happy about the NEED (both of them, really) here.
 How about HAS instead ?

Are you suggesting also changing QUIRK_NEEDS_PMU_CONFIG, then?  That
would be a request for Leela Krishna on his patch?

...see below for more...


  static bool nowayout = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
 @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
 debug (default 0));
   * timer reset functionality.
   * @mask_bit: Bit number for the watchdog timer in the disable register and 
 the
   * mask reset register.
 + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
 status.
 + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
 + * reset.
   * @quirks: A bitfield of quirks.
   */

 @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
   int disable_reg;
   int mask_reset_reg;
   int mask_bit;
 + int rst_stat_reg;
 + int rst_stat_bit;
   u32 quirks;
  };

 @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
 drv_data_exynos5250  = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 20,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 20,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,

 Why not use QUIRKS_NEED_PMUREG ?

 Also, is there ever a chance that the two bits don't come together ?
 If not a single bit might be sufficient.

Here's my logic:

According to our 3.4 sources (exynos_get_bootstatus) there is a
RST_STAT register on exynos4. See
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.4/arch/arm/mach-exynos/pmu.c.

According to Tomasz at
http://www.spinics.net/lists/linux-watchdog/msg02942.html the extra
PMU_CONFIG was needed on exynos5250/5420 and not needed on exynos4.

My patch doesn't actually add RST_STAT support for exynos4 but it
seems wise to pave the way for someone else to add it.


...so basically I was saying:

* On exynos5250 and exynos5420 we _need_ to configure the PMU to get
proper functioning

* On various devices we _have_ a reset status that register that we can query.

* If we need to use the PMU or we want to query the reset status we
need to have PMU registers specified.


Does any of that change your mind(s)?


  static const struct of_device_id s3c2410_wdt_match[] = {
 @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
 s3c2410_wdt *wdt)
  }
  #endif

 +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt 
 *wdt)
 +{
 + unsigned int bootstatus = 0;
 + int ret;
 +
 + if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {

Internally Olof requested to reverse this if and return 0 early
(avoid indentation).  I'll fix that up for the next patch.


 + unsigned int rst_stat;
 +
 + ret = regmap_read(wdt-pmureg, wdt-drv_data-rst_stat_reg,
 +   rst_stat);
 + if (ret)
 + dev_warn(wdt-dev, Couldn't get RST_STAT register\n);
 + else if (rst_stat  BIT(wdt-drv_data-rst_stat_bit))
 + bootstatus |= WDIOF_CARDRESET;
 + }
 +
 + return bootstatus;
 +}
 +
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
 On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net wrote:
  On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
  A good watchdog driver is supposed to report when it was responsible
  for resetting the system.  Implement this for the s3c2410, at least on
  exynos5250 and exynos5420 where we already have a pointer to the PMU
  registers to read the information.
 
  Signed-off-by: Doug Anderson diand...@chromium.org
  ---
  This patch is based atop Leela Krishna's recent series that ends with
  (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
  AKA https://patchwork.kernel.org/patch/3251861/.
 
   drivers/watchdog/s3c2410_wdt.c | 42 
  +++---
   1 file changed, 39 insertions(+), 3 deletions(-)
 
  diff --git a/drivers/watchdog/s3c2410_wdt.c 
  b/drivers/watchdog/s3c2410_wdt.c
  index 47f4dcf..2c87d37 100644
  --- a/drivers/watchdog/s3c2410_wdt.c
  +++ b/drivers/watchdog/s3c2410_wdt.c
  @@ -62,9 +62,13 @@
   #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
   #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
 
  +#define RST_STAT_REG_OFFSET  0x0404
   #define WDT_DISABLE_REG_OFFSET   0x0408
   #define WDT_MASK_RESET_REG_OFFSET0x040c
   #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
  +#define QUIRK_HAS_RST_STAT   (1  1)
  +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
  +  QUIRK_HAS_RST_STAT)
 
  I am not really happy about the NEED (both of them, really) here.
  How about HAS instead ?
 
 Ah, I just commented on these things on our internal review site too
 on this patch. I don't even think a quirk is needed -- just use the
 presence of a non-0 rst_stat_reg to tell if you need to use regmap.
 
Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
as well.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/