From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::22f; helo=mail-wr0-x22f.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x22f.google.com (mail-wr0-x22f.google.com [IPv6:2a00:1450:400c:c0c::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 37CE921B02834 for ; Thu, 7 Dec 2017 02:59:09 -0800 (PST) Received: by mail-wr0-x22f.google.com with SMTP id y21so7037046wrc.1 for ; Thu, 07 Dec 2017 03:03:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=/10GSyX0xmYX2F9WUA/45B3k+R2daMGo2hDRAylV+i4=; b=RH134Ynm3o2WfnQ7zAWkPgXpT1v9WS16+BGIdfCWXvmjVlZ7FIDRHwdn6HS3ccn+wX oi5FqhGHkt0EIWCM0wJVUCXNEAYGPlGCRs07R2zuh+P5Z82usaVRZY27h1uQ+baRvC4B M4rpCNn5kj/xOcNw32BgsqHqexlB/Pfo44yRs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=/10GSyX0xmYX2F9WUA/45B3k+R2daMGo2hDRAylV+i4=; b=uE9NHuIKv4UKjWn5HB3C8hMhbuyKsnduihEcu3ERKdQT/yFVFtAcG1Ea+M5Hk1D9Ez txeV9B1MUeg5guA/anvoHV9vOgGrBFGDkfkE83Tb6EfoEq8oHnWiXYYBRiE1UiuJdTNT vtL1fTIM0Y34Tb2Og5rYcB05h22g2ryjS+pFAgSBLqA+KiyDT6UjwPObmger35dQwvn0 gHjMfhs9nqP9TBWfPjH2OyPyxA/tWzp5vXCN9XxUZukunZaxsD9F/efl+dNscU/czWV2 +bT2z1iVyvPoZOein0zzCfvzQTssaDPMOpF8B5GZw7M8gVf0xZUSIqz/xdxbJVbtMZEc wJaw== X-Gm-Message-State: AJaThX58tbPqyqD0oGfp/Ch5FQZBzrA2iyev0n9Z5SCcuUmdnB+MKZST FE2eBTpk4PJnx8xitMbiMcFqT0YzcwE= X-Google-Smtp-Source: AGs4zMbU8LcWrqz3953I4/7VJghk7kmYPWMWNxR+1rBy+KYJ1/OQSeJOJRzjipqznAbhyEP1vZwquw== X-Received: by 10.223.135.243 with SMTP id c48mr21122101wrc.140.1512644621506; Thu, 07 Dec 2017 03:03:41 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id d10sm5497208wmh.15.2017.12.07.03.03.40 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 07 Dec 2017 03:03:40 -0800 (PST) Date: Thu, 7 Dec 2017 11:03:38 +0000 From: Leif Lindholm To: "Gao, Liming" Cc: Udit Kumar , "Kinney, Michael D" , Meenakshi Aggarwal , "ard.biesheuvel@linaro.org" , "edk2-devel@lists.01.org" , Varun Sethi Message-ID: <20171207110338.do5i3kqrseb24edx@bivouac.eciton.net> References: <1511779917-9255-1-git-send-email-meenakshi.aggarwal@nxp.com> <1511779917-9255-3-git-send-email-meenakshi.aggarwal@nxp.com> <20171204143537.abxvjztldex2bjde@bivouac.eciton.net> <4A89E2EF3DFEDB4C8BFDE51014F606A14E189CAA@SHSMSX104.ccr.corp.intel.com> <20171205110606.374td5x3rhdnhh5g@bivouac.eciton.net> <4A89E2EF3DFEDB4C8BFDE51014F606A14E18BC76@SHSMSX104.ccr.corp.intel.com> MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E18BC76@SHSMSX104.ccr.corp.intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Dec 2017 10:59:10 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Liming, On Thu, Dec 07, 2017 at 07:11:38AM +0000, Gao, Liming wrote: > Leif: > I don't see the core driver uses > WatchdogTimer->RegisterHandler(). When it returns unsupported, it > means the additional handler can't be registered. DxeCore uses > WatchdogTimer->SetTimerPeriod(). This service is implemented in > your driver. > > Watchdog protocol is defined in PI spec. Spec describes that this > protocol provides the services required to implement the Boot > Service SetWatchdogTimer(). It provides a service to set the > amount of time to wait before firing the watchdog timer, and it > also provides a service to register a handler that is invoked when > the watchdog timer fires. This protocol can implement the watchdog > timer by using the event and timer Boot Services, or it can make > use of custom hardware. If no handler has been registered, or the > registered handler returns, then the system will be reset by > calling the Runtime Service ResetSystem(). So, this protocol is > required. I am not disputing that the protocol is not required. I am suggesting that this hardware watchdog _cannot_ be used to register a handler. If this hardware watchdog does not get updated in time, that causes an immediate hardware reset of the processor. Because of this, I believe EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is not the appropriate way to make use of it. Please let me know whether you agree. Regards, Leif > Thanks > Liming > >-----Original Message----- > >From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > >Sent: Tuesday, December 05, 2017 7:06 PM > >To: Udit Kumar > >Cc: Gao, Liming ; Kinney, Michael D > >; Meenakshi Aggarwal > >; ard.biesheuvel@linaro.org; edk2- > >devel@lists.01.org; Varun Sethi > >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add > >support for Watchdog driver > > > >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote: > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol > >implementation > >> > could return its status besides spec defined status. > >> > >> Thanks to help me , how core will treat this error > >> 1/ Wdt not available > >> 2/ ignoring this error > >> 3/ core is not registering handler > >> I guess 3 is valid, > > > >Looking at Core/Dxe/Misc/SetWatchdogTimer.c: > > // > > // Attempt to set the timeout > > // > > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer, > > MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND)); > > > > // > > // Check for errors > > // > > if (EFI_ERROR (Status)) { > > return EFI_DEVICE_ERROR; > > } > > > >The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR. > > > >> On side track, looks wdt is not used by core services then do we > >> really need this as part of arch protocol ? > > > >Yes, that was ultimately what I was implying with my question > >regarding whether this protocol is relevant for a watchdog that can > >only ever reset the system on timeout. > > > >The protocol looks to me to be designed to use a dedicated generic > >timer as backing for a software watchdog. > > > >Liming, Mike? > > > >If that is the case, then I agree this driver should probably not > >implement this protocol, but rather set up a timer event (or a > >dedicated timer) to stroke the watchdog. > > > >Regards, > > > >Leif > > > >> regards > >> Udit > >> > >> > -----Original Message----- > >> > From: Gao, Liming [mailto:liming.gao@intel.com] > >> > Sent: Monday, December 04, 2017 8:53 PM > >> > To: Leif Lindholm ; Kinney, Michael D > >> > > >> > Cc: Meenakshi Aggarwal ; > >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar > >> > ; Varun Sethi > >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add > >support > >> > for Watchdog driver > >> > > >> > Leif: > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol > >implementation > >> > could return its status besides spec defined status. > >> > > >> > Thanks > >> > Liming > >> > > -----Original Message----- > >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > >> > > Sent: Monday, December 4, 2017 10:36 PM > >> > > To: Kinney, Michael D ; Gao, Liming > >> > > > >> > > Cc: Meenakshi Aggarwal ; > >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; > >> > > udit.kumar@nxp.com; v.sethi@nxp.com > >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add > >> > > support for Watchdog driver > >> > > > >> > > Mike, Liming, as MdePkg mainteiners - one question below: > >> > > > >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote: > >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c > >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c > >> > > > new file mode 100644 > >> > > > index 0000000..a9c70ef > >> > > > --- /dev/null > >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c > >> > > > @@ -0,0 +1,421 @@ > >> > > > >> > > ... > >> > > > >> > > > +/** > >> > > > + This function registers the handler NotifyFunction so it is > >> > > > +called every time > >> > > > + the watchdog timer expires. It also passes the amount of time > >> > > > +since the last > >> > > > + handler call to the NotifyFunction. > >> > > > + If NotifyFunction is not NULL and a handler is not already > >> > > > +registered, > >> > > > + then the new handler is registered and EFI_SUCCESS is returned. > >> > > > + If NotifyFunction is NULL, and a handler is already registered, > >> > > > + then that handler is unregistered. > >> > > > + If an attempt is made to register a handler when a handler is > >> > > > +already registered, > >> > > > + then EFI_ALREADY_STARTED is returned. > >> > > > + If an attempt is made to unregister a handler when a handler is > >> > > > +not registered, > >> > > > + then EFI_INVALID_PARAMETER is returned. > >> > > > + > >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance. > >> > > > + @param NotifyFunction The function to call when a timer interrupt > >fires. > >> > This > >> > > > + function executes at TPL_HIGH_LEVEL. The DXE Core > >will > >> > > > + register a handler for the timer interrupt, so it can know > >> > > > + how much time has passed. This information is used to > >> > > > + signal timer based events. NULL will unregister the > >handler. > >> > > > + > >> > > > + @retval EFI_SUCCESS The watchdog timer handler was > >registered. > >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a > >> > handler is already > >> > > > + registered. > >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a > >handler > >> > was not > >> > > > + previously registered. > >> > > > + > >> > > > +**/ > >> > > > +STATIC > >> > > > +EFI_STATUS > >> > > > +EFIAPI > >> > > > +WdogRegisterHandler ( > >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This, > >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction > >> > > > + ) > >> > > > +{ > >> > > > + // ERROR: This function is not supported. > >> > > > + // The hardware watchdog will reset the board > >> > > > + return EFI_INVALID_PARAMETER; > >> > > > >> > > Michael, Liming - what's your take on this? > >> > > > >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a > >pure-hw > >> > > watchdog such as this? > >> > > > >> > > If so, what would be a suitable return code here? > >> > > EFI_INVALID_PARAMETER does not look ideal. > >> > > > >> > > / > >> > > Leif