From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Permerror (SPF Permanent Error: More than 10 MX records returned) identity=mailfrom; client-ip=192.55.52.120; helo=mga04.intel.com; envelope-from=liming.gao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1027E21B02829 for ; Wed, 6 Dec 2017 23:07:09 -0800 (PST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Dec 2017 23:11:41 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,371,1508828400"; d="scan'208";a="184857493" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga006.fm.intel.com with ESMTP; 06 Dec 2017 23:11:41 -0800 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 6 Dec 2017 23:11:41 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 6 Dec 2017 23:11:41 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.152]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.218]) with mapi id 14.03.0319.002; Thu, 7 Dec 2017 15:11:39 +0800 From: "Gao, Liming" To: Leif Lindholm , Udit Kumar CC: "Kinney, Michael D" , Meenakshi Aggarwal , "ard.biesheuvel@linaro.org" , "edk2-devel@lists.01.org" , Varun Sethi Thread-Topic: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver Thread-Index: AQHTbQ0vgZNINfv/j0CGQcZcSf6avaMzTR3AgABgbwCAAGRVAIADZB8w Date: Thu, 7 Dec 2017 07:11:38 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E18BC76@SHSMSX104.ccr.corp.intel.com> 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> In-Reply-To: <20171205110606.374td5x3rhdnhh5g@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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 07:07:09 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Leif: I don't see the core driver uses WatchdogTimer->RegisterHandler(). When i= t returns unsupported, it means the additional handler can't be registered.= DxeCore uses WatchdogTimer->SetTimerPeriod(). This service is implemented = in your driver.=20 Watchdog protocol is defined in PI spec. Spec describes that this protoco= l provides the services required to implement the Boot Service SetWatchdogT= imer(). It provides a service to set the amount of time to wait before firi= ng 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 m= ake use of custom hardware. If no handler has been registered, or the regis= tered handler returns, then the system will be reset by calling the Runtime= Service ResetSystem(). So, this protocol is required. 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 =3D 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 : Ad= d >> > > 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 inte= rrupt >fires. >> > This >> > > > + function executes at TPL_HIGH_LEVEL. T= he DXE Core >will >> > > > + register a handler for the timer inter= rupt, so it can know >> > > > + how much time has passed. This informa= tion is used to >> > > > + signal timer based events. NULL will u= nregister 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