LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Doug Anderson <dianders@chromium.org>
Cc: Jisheng Zhang <jszhang@marvell.com>,
	Wim Van Sebroeck <wim@iguana.be>,
	Heiko Stuebner <heiko@sntech.de>,
	Lunxue Dai <lunxue.dai@rock-chips.com>,
	Dinh Nguyen <dinguyen@altera.com>,
	"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
Date: Fri, 23 Jan 2015 09:02:51 -0800	[thread overview]
Message-ID: <54C27EBB.9000200@roeck-us.net> (raw)
In-Reply-To: <CAD=FV=U1dg8w+_J0A3Q_O6-9qMmiTm=qO-uH9vo3gQCyM1fM4Q@mail.gmail.com>

On 01/23/2015 08:20 AM, Doug Anderson wrote:
> Guenter,
>
> On Fri, Jan 23, 2015 at 8:03 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/22/2015 09:09 AM, Doug Anderson wrote:
>>>
>>> Jisheng,
>>>
>>> On Wed, Jan 21, 2015 at 9:22 PM, Jisheng Zhang <jszhang@marvell.com>
>>> wrote:
>>>>
>>>> Dear Doug,
>>>>
>>>> On Wed, 21 Jan 2015 15:17:22 -0800
>>>> Doug Anderson <dianders@chromium.org> wrote:
>>>>
>>>>> On some dw_wdt implementations the "top" register may be initted to 0
>>>>> at bootup.  In such a case, each "pat" of the watchdog will reset the
>>>>> timer to 0xffff.  That's pretty short.
>>>>
>>>>
>>>> + Guenter Roeck
>>>>
>>>> This should have been fixed by dfa07141e7a792("watchdog: dw_wdt:
>>>> initialise
>>>> TOP_INIT in dw_wdt_set_top()")
>>>
>>>
>>> I will admit that I'm testing on a tree that doesn't have your patch
>>> (I'm on a 3.14 kernel with lots of backports).  ...but I did try
>>> cherry-picking your patch before I wrote up mine and it didn't fix my
>>> problem.  I believe that the watchdog that's in Rockchip rk3288 must
>>> be a slightly different version of the IP block than you're working
>>> with.
>>>
>>> Specifically I see the register WDT_TORR that has an offset of 0x4.
>>> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
>>> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
>>> "reserved".
>>>
>> Not sure where that leaves us. Does that mean the driver supports different
>> hardware with different register sets ?
>
> Apparently so.  I've only seen the documentation from rk3288, but it's
> clearly different than what you saw.
>
>>   Should that be documented in the
>> driver,
>
> Probably not a terrible idea.
>
>> and should we have (or do we need) different compatible statements for those
>> variants, and conditional code in the driver ?
>
> I'm not sure we actually need any conditional code.  I've put the
> other patch on rk3288 and it didn't hurt to write those reserved bits.
> I also can't quite believe that the extra pat will hurt on other
> hardware.
>
>
>> And does it mean we need both patches, at least for some of the hardware
>> variants ? If so, what happens if those patches are applied and the
>> resulting
>> driver runs on the other hardware ?
>
> I think it should be fine.
>
>
> Do you want me to spin my patch and add some extra comments (but
> otherwise keep it roughly unchanged?).  We can get Jisheng to add his
> Tested-by...
>

Yes, that would be great.

Thanks,
Guenter


  reply	other threads:[~2015-01-23 17:02 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-21 23:17 Doug Anderson
2015-01-21 23:17 ` [PATCH 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default Doug Anderson
2015-01-21 23:36   ` Guenter Roeck
2015-01-21 23:35 ` [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it Guenter Roeck
2015-01-22  5:22 ` Jisheng Zhang
2015-01-22  5:31   ` Guenter Roeck
2015-01-22 17:09   ` Doug Anderson
2015-01-23 16:03     ` Guenter Roeck
2015-01-23 16:20       ` Doug Anderson
2015-01-23 17:02         ` Guenter Roeck [this message]
2015-01-26  6:22     ` Jisheng Zhang
2015-01-26 17:01       ` Doug Anderson
2015-01-27  3:44         ` Jisheng Zhang
2015-01-27  4:07           ` Guenter Roeck
2015-01-27  4:36             ` Jisheng Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54C27EBB.9000200@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=dianders@chromium.org \
    --cc=dinguyen@altera.com \
    --cc=heiko@sntech.de \
    --cc=jszhang@marvell.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=lunxue.dai@rock-chips.com \
    --cc=wim@iguana.be \
    --subject='Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).