From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (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 8EE4F21A07A80 for ; Mon, 19 Nov 2018 11:09:44 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id m15so9152991itl.4 for ; Mon, 19 Nov 2018 11:09:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=IC5+QoSOyNsuuAizmDURp5ON5kG6Qfrq6M9G+772FBk=; b=kISBe1QT4C8NPip+Jz8O6QxvDmqbMdvIrhIFTKGGPAE81U0bhXnGrKPM4fX/KF0KYG Psw66Np1ZX3MR93dDsO0uV0AhmPlbB03cPjP9vKCgx5m1M42yB/9NEFIzeTDzSIhIh2c RVSUckC9K1ph6Ko/L/jrurYLoq8h1RJykkUEw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=IC5+QoSOyNsuuAizmDURp5ON5kG6Qfrq6M9G+772FBk=; b=ESW95yxBKVGPCfCdDVpcImbZhBE3MCrZFssegTSGTyIVw0o+jl3ykfVkHwZmuV0KNC wDzrLRTIyJjpWTA1vJxcWg3e/alHQYN9hUtoEXZGY2J8Iej4BY4QwSLTbMF5n7qcWyui vT5dyBcUdtwjdBYKf10Ck2p3qYBXaeEf6e4mInzISxqtohETJEKnW6uoC1zihD1EDvUU NQdX8z9fV+y33x8XmVX7Z38kuHI8iHfuIsWIwYNOqkEwh84xb1Q+nKP5ObKnRNksRTnP zam5k8pT28gOdwuPu/MueO3oK7njYcj4/USrMhuZIL/baMwUkRK7T6XXHpAdO0yT1Hm6 pbVg== X-Gm-Message-State: AA+aEWYDL98nEiayIdwstfToMf+PUPJlrTspodPMRZ41SoPCRYv7nE9r CJEH3mUgP28Jxdm33n3JOcamUr3jni0cz3ycVUK5JA== X-Google-Smtp-Source: AFSGD/VPMX8859Mu7WsZ34t11LDfI0x2yoNJ7naEQni1PPMqsowSptlEqh/mXLPG1PJbEPeicE2dFKgfY/N4oI/79ao= X-Received: by 2002:a24:710:: with SMTP id f16mr1700806itf.121.1542654583717; Mon, 19 Nov 2018 11:09:43 -0800 (PST) MIME-Version: 1.0 References: <20181117004524.31851-1-ard.biesheuvel@linaro.org> <20181117004524.31851-2-ard.biesheuvel@linaro.org> <20181119190555.3hlnyds46waiited@bivouac.eciton.net> In-Reply-To: <20181119190555.3hlnyds46waiited@bivouac.eciton.net> From: Ard Biesheuvel Date: Mon, 19 Nov 2018 11:09:32 -0800 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Laszlo Ersek , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , Hongbo Zhang , Thomas Panakamattam Abraham , Nariman Poushin Subject: Re: [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Nov 2018 19:09:44 -0000 Content-Type: text/plain; charset="UTF-8" On Mon, 19 Nov 2018 at 11:05, Leif Lindholm wrote: > > On Fri, Nov 16, 2018 at 04:45:23PM -0800, Ard Biesheuvel wrote: > > Currently, each flash bank controlled by ArmPlatformPkg/NorFlashDxe > > has its own VendorHw GUID, and instances of NorFlashPlatformLib > > describe each bank to the driver, along with the GUID for each. > > > > This works ok for bare metal platforms, but it would be useful for > > virtual platforms if we could obtain this information from a > > device tree, which would require us to invent GUIDs on the fly, > > given that the 'cfi-flash' binding does not include a GUID. > > > > So instead, let's switch to a single GUID for all flash banks, > > and update the driver's device path handling to include an index > > to identify each bank uniquely. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > > --- > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 15 +++++++++------ > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 3 +++ > > 2 files changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > > index 46e815beb343..60a06e4a5058 100644 > > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > > @@ -82,10 +82,14 @@ NOR_FLASH_INSTANCE mNorFlashInstanceTemplate = { > > { > > HARDWARE_DEVICE_PATH, > > HW_VENDOR_DP, > > - { (UINT8)sizeof(VENDOR_DEVICE_PATH), (UINT8)((sizeof(VENDOR_DEVICE_PATH)) >> 8) } > > + { > > + (UINT8)(OFFSET_OF(NOR_FLASH_DEVICE_PATH, End)), > > + (UINT8)(OFFSET_OF(NOR_FLASH_DEVICE_PATH, End) >> 8) > > + } > > }, > > { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }, // GUID ... NEED TO BE FILLED > > }, > > + 0, // Index > > { > > END_DEVICE_PATH_TYPE, > > END_ENTIRE_DEVICE_PATH_SUBTYPE, > > @@ -99,10 +103,9 @@ NorFlashCreateInstance ( > > IN UINTN NorFlashDeviceBase, > > IN UINTN NorFlashRegionBase, > > IN UINTN NorFlashSize, > > - IN UINT32 MediaId, > > + IN UINT32 Index, > > IN UINT32 BlockSize, > > IN BOOLEAN SupportFvb, > > - IN CONST GUID *NorFlashGuid, > > OUT NOR_FLASH_INSTANCE** NorFlashInstance > > ) > > { > > @@ -121,11 +124,12 @@ NorFlashCreateInstance ( > > Instance->Size = NorFlashSize; > > > > Instance->BlockIoProtocol.Media = &Instance->Media; > > - Instance->Media.MediaId = MediaId; > > + Instance->Media.MediaId = Index; > > Instance->Media.BlockSize = BlockSize; > > Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1; > > > > - CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid); > > + CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid); > > Just sanity checking: this sets the VendorGuid to the NorFlashDxe > GUID? (93E34C7E-B50E-11DF-9223-2443DFD72085) > Yes. Before: Mapping table BLK1: Alias(s): VenHw(F9B94AE2-8BA6-409B-9D56-B9B417F53CB3) BLK0: Alias(s): VenHw(8047DB4B-7E9C-4C0C-8EBC-DFBBAACACE8F) After: Mapping table BLK0: Alias(s): VenHw(93E34C7E-B50E-11DF-9223-2443DFD72085,00) BLK1: Alias(s): VenHw(93E34C7E-B50E-11DF-9223-2443DFD72085,01) > If not, can you explain it to me slowly? :) > > Thomas, Nariman: would this change cause any transient issues for > anything that has set Boot#### options in any of your configurations? > And if it would, is that a big deal? > (Ard has a separate patch that fixes up any default values.) > > / > Leif > > > + Instance->DevicePath.Index = Index; > > > > Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);; > > if (Instance->ShadowBuffer == NULL) { > > @@ -1311,7 +1315,6 @@ NorFlashInitialise ( > > Index, > > NorFlashDevices[Index].BlockSize, > > ContainVariableStorage, > > - &NorFlashDevices[Index].Guid, > > &mNorFlashInstances[Index] > > ); > > if (EFI_ERROR(Status)) { > > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h > > index 5c07694fbfaa..8886aa43d9f3 100644 > > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h > > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h > > @@ -122,10 +122,13 @@ > > > > typedef struct _NOR_FLASH_INSTANCE NOR_FLASH_INSTANCE; > > > > +#pragma pack(1) > > typedef struct { > > VENDOR_DEVICE_PATH Vendor; > > + UINT8 Index; > > EFI_DEVICE_PATH_PROTOCOL End; > > } NOR_FLASH_DEVICE_PATH; > > +#pragma pack() > > > > struct _NOR_FLASH_INSTANCE { > > UINT32 Signature; > > -- > > 2.17.1 > >