From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752866AbbKVCcd (ORCPT ); Sat, 21 Nov 2015 21:32:33 -0500 Received: from bh-25.webhostbox.net ([208.91.199.152]:33626 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751954AbbKVCcb (ORCPT ); Sat, 21 Nov 2015 21:32:31 -0500 Subject: Re: [PATCH 4/4] MIPS: bmips: Convert bcm63xx_wdt to use WATCHDOG_CORE To: Simon Arlott , "devicetree@vger.kernel.org" , Ralf Baechle , Thomas Gleixner , Jason Cooper , Marc Zyngier , Kevin Cernekee , Florian Fainelli , Wim Van Sebroeck , Miguel Gaio , Maxime Bizon , Linux Kernel Mailing List , linux-mips@linux-mips.org, linux-watchdog@vger.kernel.org References: <5650BFD6.5030700@simon.arlott.org.uk> <5650C08C.6090300@simon.arlott.org.uk> <5650E2FA.6090408@roeck-us.net> <5650E5BB.6020404@simon.arlott.org.uk> Cc: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala From: Guenter Roeck Message-ID: <56512937.6030903@roeck-us.net> Date: Sat, 21 Nov 2015 18:32:23 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <5650E5BB.6020404@simon.arlott.org.uk> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/21/2015 01:44 PM, Simon Arlott wrote: > On 21/11/15 21:32, Guenter Roeck wrote: >> On 11/21/2015 11:05 AM, Simon Arlott wrote: >>> Convert bcm63xx_wdt to use WATCHDOG_CORE and add a device tree binding. >>> >>> Adds support for the time left value and provides a more effective >>> interrupt handler based on the watchdog warning interrupt behaviour. >>> >>> This removes the unnecessary software countdown timer and replaces the >>> use of bcm63xx_timer with a normal interrupt when not using mach-bcm63xx. >>> >> >> Hi Simon, >> >> this is really doing a bit too much in a single patch. >> Conversion to the watchdog infrastructure should probably be >> the first step, followed by further optimizations and improvements. > > I'll split it into two patches, but that won't remove the need for #ifdefs. > >> In general, it would be great if we can avoid #ifdef in the code. >> Maybe there is some other means to determine if one code path >> needs to be taken or another. The driver may be part of a >> multi-platform image, and #ifdefs in the code make that all >> but impossible. Besides, it makes the code really hard to read >> and understand. > > It's impossible to avoid the #ifdefs because the driver needs to support > mach-bmips while still supporting mach-bcm63xx. I don't think they make > it too difficult to understand. Until there are device tree supporting > drivers for everything mach-bcm63xx needs, it can't be removed. > Even if ifdefs are needed, they don't need to be as extensive as they are. #ifdef around function names can be handled with shim functions, different clock names can be handled by defining the clock name per platform. The interrupt handler registration may not require an #ifdef if it is just made optional. Conditional include files are typically not needed at all. >> We have some infrastructure changes in the works which will move >> the need for soft-timers from individual drivers into the watchdog core. >> Would this possibly be helpful here ? The timer-driven watchdog ping >> seems to accomplish pretty much the same. > > There is no need for a software timer. This is not a timer-driven > watchdog ping, there is an unmaskable timer interrupt when the watchdog > timer has less than 50% remaining. > Ok. Maybe I got confused by the interrupt-triggered watchdog ping. I'll have to look into that much more closely; it is quite unusual and complex. The explanation is also not easy to understand. What does "The only way to stop this interrupt" mean ? Repeatedly triggering the interrupt in 1/2, 1/4, 1/8 of the remaining time is really odd. On side note, the subject tag should be "watchdog:", not "MIPS:". Thanks, Guenter