From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::544; helo=mail-ed1-x544.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-ed1-x544.google.com (mail-ed1-x544.google.com [IPv6:2a00:1450:4864:20::544]) (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 94A22210DF774 for ; Wed, 8 Aug 2018 02:59:14 -0700 (PDT) Received: by mail-ed1-x544.google.com with SMTP id f23-v6so941916edr.11 for ; Wed, 08 Aug 2018 02:59:14 -0700 (PDT) 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:content-transfer-encoding:in-reply-to :user-agent; bh=Bcl/pJiqU8eCsNyA7mr64MJXi5QMmpsRf2vVGRKaR64=; b=ARZDjMcqAK/+rc7UPUP1ACDbR8wFd0qBqXG3EzXc09OfQOOFnXC7xC4YoztY9qVaW0 TLk2zAnnwLWj8DWZns/O+BghisYgjOQKKGl8J7IDeC1GEOfX0cJxRP3IcuP7z1DnuwjW l0IKeQislMgN39dHC5SZ0NyIkZhkzOb75a4qM= 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:content-transfer-encoding :in-reply-to:user-agent; bh=Bcl/pJiqU8eCsNyA7mr64MJXi5QMmpsRf2vVGRKaR64=; b=ez02+pjQh7uSh4i0vvvYV4z665Kfru/mtvbj0x5eWYvPc5Zly/pEUWNqeBaDLj+vqp fQMpUduAdgj8AvrXb1ocJf1f3bj0Mq/hSFZNEDiaZLkwd/USaV5ibDktvz7vHYinF2I2 HDMvYZeB2IwR/1WzB49G8WQDH2glTVGdcFbuBvMKz7duUjOcfSi4saWN8AucXMy3skQ9 xuYiMho71KpXvt8xEAy5YKHnKww16+PlXkKzu5o0+19tfGbrA45ZIhMGA9RX1N+Xspah kIXH+uP0V9gdNi7GqC9j0Cd4nVFMQJO91/2Btveq6UV17+HiskheLvIfVB093ziEEv1z A9rg== X-Gm-Message-State: AOUpUlGbBVfyoRaFE249zWrKwsvzfuiYMWL7rjKsBUm0kfDaFP5xeO7+ GAGecOag3FRsOxUCq0WxQ6eCrg== X-Google-Smtp-Source: AA+uWPzMgb+MQqzMLqu1URzoVap8vFz5gD/4tnSn8HZuLrwHdlwA7PH2lXZk/j5ZFYAoGEhmFTq6/A== X-Received: by 2002:aa7:d786:: with SMTP id s6-v6mr2419696edq.228.1533722352656; Wed, 08 Aug 2018 02:59:12 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id g28-v6sm3976012edg.67.2018.08.08.02.59.11 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 08 Aug 2018 02:59:11 -0700 (PDT) Date: Wed, 8 Aug 2018 10:59:09 +0100 From: Leif Lindholm To: Ming Cc: linaro-uefi@lists.linaro.org, edk2-devel@lists.01.org, graeme.gregory@linaro.org, ard.biesheuvel@linaro.org, guoheyi@huawei.com, wanghuiqiang@huawei.com, huangming23@huawei.com, zhangjinsong2@huawei.com, huangdaode@hisilicon.com, john.garry@huawei.com, xinliang.liu@linaro.org, Heyi Guo Message-ID: <20180808095909.tavnft3vmycftxc7@bivouac.eciton.net> References: <20180724070922.63362-1-ming.huang@linaro.org> <20180724070922.63362-8-ming.huang@linaro.org> <20180802173647.paepialtuopbe6y5@bivouac.eciton.net> <22f0c26b-0666-82b5-2b1c-f71af63526a4@linaro.org> MIME-Version: 1.0 In-Reply-To: <22f0c26b-0666-82b5-2b1c-f71af63526a4@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms v1 07/38] Silicon/Hisilicon/D06: Wait for all disk ready X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Aug 2018 09:59:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Wed, Aug 08, 2018 at 05:02:15PM +0800, Ming wrote: > 在 8/3/2018 1:36 AM, Leif Lindholm 写道: > > On Tue, Jul 24, 2018 at 03:08:51PM +0800, Ming Huang wrote: > >> This patch is relative to D06 SasDxe driver. The SasDxe set a > >> variable to notice this libray. Here Wait for all disk ready > >> for 30S at most. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Ming Huang > >> Signed-off-by: Heyi Guo > >> --- > >> Silicon/Hisilicon/HisiPkg.dec | 1 + > >> Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c | 43 ++++++++++++++++++++ > >> Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 2 + > >> 3 files changed, 46 insertions(+) > >> > >> diff --git a/Silicon/Hisilicon/HisiPkg.dec b/Silicon/Hisilicon/HisiPkg.dec > >> index 35bea970ec..b56a6a6af7 100644 > >> --- a/Silicon/Hisilicon/HisiPkg.dec > >> +++ b/Silicon/Hisilicon/HisiPkg.dec > >> @@ -45,6 +45,7 @@ > >> > >> gHisiEfiMemoryMapGuid = {0xf8870015, 0x6994, 0x4b98, {0x95, 0xa2, 0xbd, 0x56, 0xda, 0x91, 0xc0, 0x7f}} > >> gVersionInfoHobGuid = {0xe13a14c, 0x859c, 0x4f22, {0x82, 0xbd, 0x18, 0xe, 0xe1, 0x42, 0x12, 0xbf}} > >> + gHisiOemVariableGuid = {0xac62b9a5, 0x9939, 0x41d3, {0xff, 0x5c, 0xc5, 0x80, 0x32, 0x7d, 0x9b, 0x29}} > >> gOemBootVariableGuid = {0xb7784577, 0x5aaf, 0x4557, {0xa1, 0x99, 0xd4, 0xa4, 0x2f, 0x45, 0x06, 0xf8}} > > > > What is the difference between gHisiOemVariableGuid and gOemBootVariableGuid? > > > > Just a guid using to a variable. > Do you suggest to use the old one? Well, I am unsure about the intended scope for either. So I don't know. How is HisiOem different from Oem? Can you explain the structure? > >> gEfiHisiSocControllerGuid = {0xee369cc3, 0xa743, 0x5382, {0x75, 0x64, 0x53, 0xe4, 0x31, 0x19, 0x38, 0x35}} > >> > >> diff --git a/Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c b/Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c > >> index 7dd5ba615c..f7536bfea3 100644 > >> --- a/Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c > >> +++ b/Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c > >> @@ -20,6 +20,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> #include > >> #include > >> #include > >> @@ -554,6 +555,47 @@ PlatformBootManagerBeforeConsole ( > >> PlatformRegisterOptionsAndKeys (); > >> } > >> > >> +STATIC > >> +VOID > >> +WaitForDiskReady ( > >> + ) > >> +{ > >> + EFI_STATUS Status; > >> + UINT32 Index; > >> + UINTN DataSize; > >> + UINT32 DiskInfo; > >> + UINT8 IsFinished; > >> + > >> + Status = EFI_NOT_FOUND; > >> + DataSize = sizeof (UINT32); > >> + // Wait for 30 seconds at most. > >> + for (Index=0; Index<30; Index++) { > > > > Spaces around '=' and '<'. > > > >> + Status = gRT->GetVariable ( > >> + L"SASDiskInfo", > >> + &gHisiOemVariableGuid, > >> + NULL, > >> + &DataSize, > >> + &DiskInfo > >> + ); > > > > Wait... > > Are we synchronizing against the storage controller driver using an > > environment variable and looping over it for 30 seconds? > > > D06: > For using hard disk backboard, some disk need 15 seconds to ready. > Actually, wait less 15 seconds here(minus the time from end of SAS > driver to here). > For using expander, wait less 6 seconds here(minus the time from end of SAS > driver to here). > > D03/D05: > no waiting here. > > Maybe I should change 30 to 15, it will never loop over for 30 seconds. > > > That can't go in. > > Please look into implementing an event in the SAS driver which you can > > wait for here. > > I don't really understand what differece between implementing with variable > and implementing with event. One is using the mechanism explicitly designed for this sort of thing. (And hence making the code a lot more clear and avoiding unintended side effects.) It also mean you would wait exactly as long as you needed, and not need to worry about how long. The other is using variables. > >> + if (EFI_ERROR(Status)) { > >> + DEBUG ((DEBUG_ERROR, "Get DiskInfo:%r\n", Status)); > >> + break; > >> + } > >> + > >> + IsFinished = (UINT8)(DiskInfo >> 24); > >> + if (IsFinished) { > >> + break; > >> + } > >> + DEBUG ((DEBUG_ERROR, "%a", Index == 0 ? "Wait for disk." : ".")); > >> + MicroSecondDelay(1000*1000); // 1S > > > > Spaces around '*'. > > The code already says to sleep a million microseconds, comment superfluous. > > > >> + } > >> + > >> + if (!EFI_ERROR(Status)) { > >> + DEBUG ((DEBUG_ERROR, "DiskInfo:%x\n", DiskInfo)); > >> + EfiBootManagerConnectAll (); > > > > Why not call WaitForDiskReady() before EfiBootManagerConnectAll () in > > PlatformBootManagerAfterConsole ()? > > > > The Sas controller of D06 is a pci device, SAS driver (Start functio) run after > pci enumeration. WaitForDiskReady should be called after SAS driver and before > creating boot options. OK, but EfiBootManagerConnectAll () is a bit of a sledge hammer, and will affect boot times. The SAS controller is onboard (so always present), right? If so, you could connect it manually. / Leif