On 7 February 2016 at 02:57, Guenter Roeck [off-list ref] wrote:

On 02/06/2016 10:02 AM, Fu Wei wrote:

quoted

Hi Guenter,

On 6 February 2016 at 07:54, Guenter Roeck [off-list ref] wrote:

quoted

On 02/05/2016 10:21 AM, Fu Wei wrote:

quoted

On 5 February 2016 at 22:42, Guenter Roeck [off-list ref] wrote:

quoted

On 02/05/2016 01:51 AM, Fu Wei wrote:

quoted

Hi Guenter,

On 4 February 2016 at 13:17, Guenter Roeck [off-list ref] wrote:

quoted

On 02/03/2016 03:00 PM, Fu Wei wrote:

quoted

On 4 February 2016 at 02:45, Timur Tabi [off-list ref]
wrote:

quoted

Fu Wei wrote:

quoted

As you know I have made the pre-timeout support patch, If people
like
it, i am happy to go on upstream it separately.

If we want to use pre-timeout here, user only can use
get_pretimeout
and disable panic by setting pretimeout to 0
but user can not really set pretimeout, because "pre-timeout  ==
timeout / 2 (always)".
if user want to change pretimeout, he/she has to set_time instead.

Ok, I think patches 4 and 5 should be combined, and I think the
Kconfig
entry should be removed and just use panic_enabled.

Agreed.

np, will do

quoted

quoted

NP, will update this patchset like that ,  thanks :-)

Also, if panic is enabled, the timeout needs to be adjusted
accordingly
(to only panic after the entire timeout period has expired, not after
half of it). We can not panic the system after timeout / 2.

OK, my thought is

if panic is enabled :
|--------WOR-------WS0--------WOR-------WS1
|------timeout------(panic)------timeout-----reset

if panic is disabled .
|--------WOR-------WS0--------WOR-------WS1
|---------------------timeout---------------------reset

    panic_enabled only can be configured when module is loaded by
module
parameter

But user should know that max_timeout(panic_enable) =
max_timeout(panic_disable) / 2

That means you'll have to update max_timeout accordingly.

panic_enabled only can be configured when module is loaded, so we
don't need to update it.

max_timeout will only be set up in the init stage.

Does it make sense ? :-)

Not sure I understand your problem or question.

max_timeout will have to reflect the correct maximum timeout, under
all circumstances. It will have to be set to the correct value before
the watchdog driver is registered.

yes, understood,  my thought is :

in  static int sbsa_gwdt_probe(struct platform_device *pdev)

if (action) {
         wdd->min_timeout = 1;
         wdd->max_timeout = U32_MAX / gwdt->clk;
} else {
         wdd->min_timeout = 2;
         wdd->max_timeout = U32_MAX / gwdt->clk * 2;

Pretty much, though you would also have to adjust all calculations
using gwdt->clk, in both set_timeout() and get_timeout(). Wonder if
you could adjust gwdt->clk instead.

Does min_timeout really have to be 2 if panic is disabled ?
The only reason seems to be the calculation in sbsa_gwdt_set_timeout().

        writel(timeout / 2 * gwdt->clk, gwdt->control_base + SBSA_GWDT_WOR);

Maybe you could use something like

        writel(timeout * (gwdt->clk / 2), ...);

instead. Or, as mentioned above, adjust the value of gwdt->clk
to include the factor.

yes,  Thanks for pointing it out.
I will fix that following your suggestion.

Thanks,
Guenter

-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021