From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c01::243; helo=mail-pl0-x243.google.com; envelope-from=ming.huang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl0-x243.google.com (mail-pl0-x243.google.com [IPv6:2607:f8b0:400e:c01::243]) (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 D19EE210E8D5D for ; Thu, 9 Aug 2018 18:44:44 -0700 (PDT) Received: by mail-pl0-x243.google.com with SMTP id s17-v6so3321894plp.7 for ; Thu, 09 Aug 2018 18:44:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=Ew3gTwnHDTV4xaWXOOnVi9siMLCJBkggn5xlM5tdBRQ=; b=BlcJs1zcAxQmxvd+RVcmfUpCaaI7wXI7EzIytyTsfZ5W49A42shSTG/FaENKeGAPhS J6BrIMmtynGkdflAgzb7EsE41th52KoQJWxP1Jq6atZDeJa9GbRCWkgaboaRVzU0zhYq KFbWTaJgNjhVfO7Nz3AJUjImIEGWEnyounVg4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=Ew3gTwnHDTV4xaWXOOnVi9siMLCJBkggn5xlM5tdBRQ=; b=UrsHGXGe13mYaTo1aPfO16+O1pQXUTKy1k19k3Gn2V/WcVdcsyYMmboNfddshGCc05 OP5pX61e5qXCyyZ2AG+hWVZE1xl3cD+8qJiJ1tLxcsqZBEYjsenP0YXEHoRtxsguqX4o zTPXwtKZZuIiyZ2sRBb+pATuXIqi5cVoA5R4egJ7iwBOOg+l0m0DXTUqjKMk+lAQNSna uZ4b6+57/+FFq/mOpfPCyTytdRGwhj6ebUTfEUDoiao7c+lTKDjDBSbVJLzZJB7vD1vM z9Xv8QpDzhimMzcyVlABixroIipATvXyo6NhEGgfAY0M4bpir84nHBBgfpN6p2cN+i6c fXhQ== X-Gm-Message-State: AOUpUlFSu9yrX4vZQMU5AWFXKHCDTY04pOUX1Svp74It/ibajHjJFIrp cLICJiYPI78xsahRbn/sgGkSdg== X-Google-Smtp-Source: AA+uWPxqU42O/maHAQAhDTzsAIwEmBvOzKLEMQK/gwmvxN9CpivEEd3kVGol2mgVHBFT6ff0h+qrqQ== X-Received: by 2002:a17:902:bb85:: with SMTP id m5-v6mr4085182pls.46.1533865484403; Thu, 09 Aug 2018 18:44:44 -0700 (PDT) Received: from [10.199.0.182] ([64.64.108.224]) by smtp.gmail.com with ESMTPSA id w192-v6sm9298144pfd.74.2018.08.09.18.44.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 Aug 2018 18:44:43 -0700 (PDT) To: Leif Lindholm 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 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> <20180808095909.tavnft3vmycftxc7@bivouac.eciton.net> <12d71306-5d19-837c-3827-8be1883b693a@linaro.org> <20180808125306.nllzdcfbnbc57wt7@bivouac.eciton.net> From: Ming Message-ID: <5b310aff-f2f0-5b89-00fc-a778fe15a1a1@linaro.org> Date: Fri, 10 Aug 2018 09:44:29 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180808125306.nllzdcfbnbc57wt7@bivouac.eciton.net> 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: Fri, 10 Aug 2018 01:44:45 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 在 8/8/2018 8:53 PM, Leif Lindholm 写道: > On Wed, Aug 08, 2018 at 07:44:36PM +0800, Ming wrote: >> >> >> 在 8/8/2018 5:59 PM, Leif Lindholm 写道: >>> 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? >> >> HisiOem and Oem are the same scope, just need a guid for SetVariable. > > The only purpose for the GUID is so that you can have variables of the > same name in different modules without risking clashes. Sharing the > same variable GUID across multiple modules is perfectly fine (and > often preferable) when those modules are closely related. > > But as I think this was only used for the communication with the SAS > controller driver, I think maybe this one can just go. OK. > >>>>>> 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. >> >> In this case, it don't want to wait exactly time, the time depand on >> SAS driver and which backboard is used. After optimizing SAS driver, >> wait time is reduced. Here,30 just for avoiding dead loop. > > Which is exactly what an event is for. > At the point where the SAS controller driver is currently updating the > variable, it can instead call SignalEvent (). > > The WaitForDiskReady () function can be changed to wait/check for that event. > As current solution(using variables) have run stable, I prefer don't change SAS driver and this wait function. Is it OK? >>>>> 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. >> >> Yes, it's always present. >> I intend to optimize SAS driver in future. It's not enouth time to do >> this before 18.08. >> Thanks. > > OK. > > / > Leif >