From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web10.7787.1579787432880172883 for ; Thu, 23 Jan 2020 05:50:33 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=FwS47PIg; spf=pass (domain: linaro.org, ip: 209.85.128.65, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f65.google.com with SMTP id f129so2617202wmf.2 for ; Thu, 23 Jan 2020 05:50:32 -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=fYY6GwExktU5nbZi1bcyoEJ/6EiaXzD7z9YIfQA9BMc=; b=FwS47PIgkhyb4LKCGC930mS+2paBvZO1uHSepsooHunDftdk7h74b1J5Tlky5c1xjT hxJXumWnuEslOqc9G6W4WPVXY2e7AqIum8GGNCz6pU16tdfxhTyPJRGIeTJgNCqlPd7/ TSU8Xa7BB9G0Mgz3t47cNm9Ontvm1HZ2idMZ++vC7y/t33moCq5wNxsqRDhbe9jwztpH Qli/AjKvxod7TX7OfhTZ7nbpvu1nJLquCNb4ziFlieQLAax+szyynL8gt7F4N1bOljRK 5NfXT16qe4sj5POgxv+2IfoSKnFO5lRJLfrpSYz3tnF4FKOtzWsILFDzKPsQ5Pr+yzO9 Sp6A== 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=fYY6GwExktU5nbZi1bcyoEJ/6EiaXzD7z9YIfQA9BMc=; b=h3OyEYEnOUivLCxnartLoBmL5gej4mb2Bf2oMm0gdVAIwNMkxsi3VUqBbVmyVGDJBh /FNbZN5NBG5OQlwd5kTQXjlVsZFf7AK/h7Qw5+ocVdSBpvDf7av8d0+qtwwB50+4jPBG X+X5EyZRTVlm28tcdfT0wksBuXsJH7TRYWMcsles0ShGNFOx3qhw+5p5csNV0q0+G0jv tQKcfj8/0I2s3WGNPNGlJIiiqxAqHa+phW+btHDRzQdLNkxHzCki+teIhFAAVVnuX4+w gdmMLccOv4xn6Iw2NxXwtrY/kZu46v1VU06PNnyl+B3wQv5XaaLE7UdAuGXjgqDHGxvM UFDw== X-Gm-Message-State: APjAAAUnmNZRB4lA9AnX38l5Tgnltdt5+n4MEzDnxcz6bu5ufajrxikI y81jOrFwWOFUjACeR70+1ACDpCRMluHHe8Ij/btTgA== X-Google-Smtp-Source: APXvYqy+XqcLBCaijCzPIhKOy54Pnj5uNzwxlJD7m4k1BrMcbVRGuaK+B+/URLlX5MhlXd8Dgyd2p8b4Hzs7hEvTfds= X-Received: by 2002:a1c:a406:: with SMTP id n6mr4275339wme.40.1579787431401; Thu, 23 Jan 2020 05:50:31 -0800 (PST) MIME-Version: 1.0 References: <20200123120007.4784-1-pete@akeo.ie> <20200123120007.4784-3-pete@akeo.ie> In-Reply-To: <20200123120007.4784-3-pete@akeo.ie> From: "Ard Biesheuvel" Date: Thu, 23 Jan 2020 14:50:20 +0100 Message-ID: Subject: Re: [edk2-platforms][PATCH 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address To: Pete Batard Cc: edk2-devel-groups-io , Leif Lindholm , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , Jeremy Linton Content-Type: text/plain; charset="UTF-8" On Thu, 23 Jan 2020 at 13:00, Pete Batard wrote: > > The Genet driver stub used by the Raspberry Pi 4 platform is > designed to set the MAC address according to a PCD. > > To be able to set that PCD at runtime, by using the Raspberry > Pi firmware interface, that has a dedicated call to retrieve > the MAC address, and satisfy driver dependencies in a generic > manner, we create a new PlatformPcdLib that can be referenced > by the Genet driver, to set the MAC PCD before use there. > > While it is currently only tailored around MAC PCD population > for Genet, we do expect this PCD library to be extended in the > future, to provide additional PCD facilities for other drivers. > > Signed-off-by: Pete Batard > --- > Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c | 61 ++++++++++++++++++++ > Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf | 44 ++++++++++++++ > 2 files changed, 105 insertions(+) > > diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c > new file mode 100644 > index 000000000000..78f3679e2e48 > --- /dev/null > +++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c > @@ -0,0 +1,61 @@ > +/** @file > + * > + * Copyright (c) 2020, Pete Batard > + * > + * SPDX-License-Identifier: BSD-2-Clause-Patent > + * > + **/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +EFI_STATUS > +EFIAPI > +PlatformPcdLibConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + > + // > + // If Genet is in use and a MAC address PCD has not been set, do it here. > + // > +#if (FixedPcdGet64 (PcdBcmGenetRegistersAddress) != 0) Can we drop this check? Why would you include this library in your .DSC and set the fixed PCD to 0? > + EFI_STATUS Status; > + UINT64 MacAddr; > + RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; > + > + if (PcdGet64 (PcdBcmGenetMacAddress) == 0) { This is policy, which we don't need today, and may well be needed elsewhere once we do need it. So I'd prefer it if we just drop this check. > + Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL, > + (VOID**)&mFwProtocol); > + ASSERT_EFI_ERROR(Status); > + > + // > + // Get the MAC address from the firmware > + // > + Status = mFwProtocol->GetMacAddress ((UINT8*) &MacAddr); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "%a: failed to retrieve MAC address\n", __FUNCTION__)); > + } else { > + PcdSet64 (PcdBcmGenetMacAddress, MacAddr); Please use PcdSet64S () instead - the unsafe ones are deprecated AFAIR > + } > + } > +#endif > + > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +EFIAPI > +PlatformPcdLibDestructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + return EFI_SUCCESS; > +} No need for a destructor > diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf > new file mode 100644 > index 000000000000..a564ddfbb2a3 > --- /dev/null > +++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf > @@ -0,0 +1,44 @@ > +#/** @file > +# > +# Copyright (c) 2020, Pete Batard > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x0001001A > + BASE_NAME = PlatformPcdLib > + FILE_GUID = 3B8409D7-D3C7-4006-823B-BFB184435363 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = NULL|DXE_DRIVER UEFI_APPLICATION > + CONSTRUCTOR = PlatformPcdLibConstructor > + DESTRUCTOR = PlatformPcdLibDestructor > + > +[Sources] > + PlatformPcdLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + Platform/RaspberryPi/RaspberryPi.dec > + Silicon/Broadcom/Drivers/Net/BcmNet.dec > + > +[LibraryClasses] > + UefiLib > + DebugLib > + PrintLib > + > +[Protocols] > + gRaspberryPiFirmwareProtocolGuid ## CONSUMES > + > +[Pcd] > + gBcmNetTokenSpaceGuid.PcdBcmGenetMacAddress ## SOMETIMES_PRODUCES > + > +[FixedPcd] > + gBcmNetTokenSpaceGuid.PcdBcmGenetRegistersAddress > + > +[Depex] > + gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid > + Use the PcdLib library class instead - this will depex on the gPcdProtocolGuid if needed. I don't mind this approach, but in general, I'd be happier with a specific library class to discover the MAC address. I.e., a library class in BcmNet.dec called BcmGenetPlatformLib, which has [for now] a single function called BcmGenetGetMacFromPlatform(). The RPi4 implementation would provide that routine, and depex on the firmware protocol, ensuring the ordering is correct. That way, we can get rid of the PCDs entirely.