From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=104.47.2.80; helo=eur01-db5-obe.outbound.protection.outlook.com; envelope-from=udit.kumar@nxp.com; receiver=edk2-devel@lists.01.org Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0080.outbound.protection.outlook.com [104.47.2.80]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7225521B02839 for ; Thu, 7 Dec 2017 03:10:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=cnGhF5oQXTHAjBvDV8ThbDOIHX2zbKU575yzPhL1iLs=; b=J9EaD8IAmMLhDfePF5iHL5CIqxraN1Dng4S5eJjHFh+USDeGCQYD5T2ZY5sNSzZpqofSEOlgKpp4M80Ww94DVmHFunqHt9yi1/jzGl6ibm8uv9h3xSDxYGLnV8ucnNxXuv5DKa0fTOScwUwtgEUpIhlpEfoF6K6kqo1mA4SDWro= Received: from AM6PR0402MB3334.eurprd04.prod.outlook.com (52.133.18.151) by AM2PR04MB0996.eurprd04.prod.outlook.com (10.162.35.21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.282.5; Thu, 7 Dec 2017 11:15:24 +0000 Received: from AM6PR0402MB3334.eurprd04.prod.outlook.com ([fe80::51eb:bed3:cf67:f4be]) by AM6PR0402MB3334.eurprd04.prod.outlook.com ([fe80::51eb:bed3:cf67:f4be%13]) with mapi id 15.20.0282.012; Thu, 7 Dec 2017 11:15:24 +0000 From: Udit Kumar To: "Gao, Liming" , Leif Lindholm 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: AQHTbQ0qQaor6kmHaUekvON9HDYV2KMzTYcAgADatJCAAG/CAIAC4ycAgAAFHLA= Date: Thu, 7 Dec 2017 11:15:23 +0000 Message-ID: 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> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E18BC76@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=udit.kumar@nxp.com; x-originating-ip: [192.88.169.1] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM2PR04MB0996; 6:6jh3DyIjMQyX/xY+bbhvLlJIGVvIJENLAJdqG9iBhQL8Qtr/qpsTLQ8YtCh2kjgly/oI5nByjTaSBGYs+gFIvHK2uiBMIkmzK8W8bzUcCjbDxrGsRZGdtaqQuEj05+wBMX/8+UVtWkQWk/40F293iOEtv7/BYG4N/2b+LQ62nrH7eEdC5nFZt3dcPEJGn+6Qb+eZmJ4mUPTeEHVcrcSoGsq4cMyjdXLtLziwHoQ3PoDw8VMwa8HW6mH7PlkLZDcLMucazGdAS2LoWiQRRjj9VpG3/7i63rdpm4T0x/Ujr8rwAjTGCrOl2V41mkDYhNtXmTaGeZRet0uSVn/wKLkc8Ptp1M80OK/4fkfqnLbLI94=; 5:eSKSazJcFYna78yOgGDiA6EXLxoTZn7j/CJCZ1vBwATKuplBY7ruMmlTtRLGjs2tZYc6DfSYq8PzJP6blOAo2lSAbJ4Mkf82XUG33uX0yWpQmLi0bhc+3PY8sa30lDDFfxqQCowBl873lG7yU4GakrlPKEkqFNczpALS8kJq2AQ=; 24:Otr5demYMXKuRo1txTPuJNU9wZawR9FfWn0BkwSquFO2/4alnPpOiwTYcbyukXgFf5F0QuTlFrAXxQU0dmBRHys4lFX9iyJPEPHvTW5Tmlo=; 7:z38hjW4JFrAh5rqym8DvKbUsOW9D86j41mHB3A0iPGYaM0CWlWm+AakvaqHCUrI2TP4WUcLYp8ky4Urq85Q6iJ6o5/Ge1u0+aKd1X/9owxd7l4es67KEJvOLqUPCdKGYCryrDGkZV6zvks+fHn88zopNEpsaCoJXzh6h7AJ35e3UY7lp5xB/K6nmqFD3QoAG8627Nhwl/9NhWFn+EMSWzPOLUMSGrI/MG/atm7b0Zm9A8knARwXqHCactuXeevaX x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-forefront-antispam-report: SFV:SKI; SCL:-1; SFV:NSPM; SFS:(10009020)(39860400002)(346002)(376002)(366004)(24454002)(189003)(199004)(13464003)(93886005)(316002)(68736007)(86362001)(66066001)(2906002)(81156014)(25786009)(33656002)(81166006)(105586002)(97736004)(54906003)(3846002)(6116002)(106356001)(7696005)(110136005)(8676002)(102836003)(4326008)(76176011)(3660700001)(5250100002)(5660300001)(229853002)(6436002)(6506006)(7736002)(2950100002)(99286004)(305945005)(14454004)(53936002)(74316002)(8936002)(101416001)(3280700002)(6246003)(55016002)(53546010)(2900100001)(9686003)(478600001)(171213001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM2PR04MB0996; H:AM6PR0402MB3334.eurprd04.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; x-ms-office365-filtering-correlation-id: d64f7ffd-8cf2-4535-aeb9-08d53d63cfd3 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(5600026)(4604075)(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(48565401081)(2017052603286); SRVR:AM2PR04MB0996; x-ms-traffictypediagnostic: AM2PR04MB0996: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(185117386973197)(162533806227266)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040450)(2401047)(8121501046)(5005006)(3231022)(93006095)(93001095)(3002001)(10201501046)(6055026)(6041248)(20161123564025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(20161123558100)(20161123560025)(20161123562025)(6072148)(201708071742011); SRVR:AM2PR04MB0996; BCL:0; PCL:0; RULEID:(100000803101)(100110400095); SRVR:AM2PR04MB0996; x-forefront-prvs: 05143A8241 received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: d64f7ffd-8cf2-4535-aeb9-08d53d63cfd3 X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Dec 2017 11:15:23.9825 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM2PR04MB0996 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 11:10:56 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Liming,=20 > DxeCore uses WatchdogTimer->SetTimerPeriod(). This service is implemented= in > your driver. Callers of SetTimerPeriod are ignoring error reported.=20 Is they assume this call will be perfect or they are ok in case some error = on watchdog service.=20 =20 Regards Udit=20 > -----Original Message----- > From: Gao, Liming [mailto:liming.gao@intel.com] > Sent: Thursday, December 07, 2017 12:42 PM > To: Leif Lindholm ; Udit Kumar > > Cc: 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 sup= port > for Watchdog driver >=20 > 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. >=20 > Watchdog protocol is defined in PI spec. Spec describes that this proto= col > provides the services required to implement the Boot Service > SetWatchdogTimer(). It provides a service to set the amount of time to wa= it > before firing the watchdog timer, and it also provides a service to regis= ter 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 th= e > registered handler returns, then the system will be reset by calling the = Runtime > Service ResetSystem(). So, this protocol is required. >=20 > 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 : > >> > > 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 returne= d. > >> > > > + 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 in= terrupt > >fires. > >> > This > >> > > > + function executes at > >> > > > + TPL_HIGH_LEVEL. The DXE Core > >will > >> > > > + register a handler for the timer int= errupt, so it can know > >> > > > + how much time has passed. This infor= mation 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