From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web10.9873.1580545534519727718 for ; Sat, 01 Feb 2020 00:25:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=XmEnOgwS; spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f65.google.com with SMTP id j104so11364418wrj.7 for ; Sat, 01 Feb 2020 00:25:34 -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=2l1C+qyIdyZZl/BlsVH/3EElNw2Ygu/xECSNiMGz3SU=; b=XmEnOgwSdPqrdI7y9KhOVZlLwQgXoyS9cBJUiEzq3myldMPf8U6bqUZR9fm0+oDOs0 nESdgLN0blbSzMIRvP6T9mGMFyEnJmqw+WLxxXDMeqXgB3k+6r1AUn7ffQMw0ZklnV4u cr2Uz8yDut3pfNGXdaz/pRyXOfxEK2gTVYW6UboehS9G0vXendyoBE6MWkV+5qAMUYbo N5jcdjMDdbqNJPNtaWvnCcg1iAsyczEm9qnvp9LcGm2O+zKKplL/OdMdzHHrAaHBbrbg C9h8hN6OZLO70AZzCmjrCc+8GEqLq+xdk9PEmPNEeLxfTbG5AX6nL2eCbQSqzAkEi61h E1PQ== 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=2l1C+qyIdyZZl/BlsVH/3EElNw2Ygu/xECSNiMGz3SU=; b=pvu1pi5FU3jg4berK5oBoxms3OphNNHkkHVMJakwFNuR+OFTZJhx2eIPvAgqGHe6ME xZZfczO7WHyfk8/ozsyNihZmCY7w7m3yllCz7sCs1kK8j8FXiP59Gc/0VgmpR4gQCJ7+ S35q5ZOIx+e+86xEX081M6B1H8LI4CJwBOq0vmFvDzrjtlu0ebOY0eMGU4SJKze1zoE/ iLPDTwZH1yO7JXt3+tM5d9NBDE4cnf/J+ln6k7Mor2f/uWb8ipayh4j8cvG3lAU3Vu0y il0x7B6ehtNNxy2nuZc1WbtGKWULseY0YjhteSz364s07GY43lkLhjvO8raIIQtxV54g Pw/Q== X-Gm-Message-State: APjAAAWfcQ4FBA05Gdj693jDLVljaj+uzF/PBEeRUUeuZuf9Har4E90K LUBSerTvct89GcFkgifPBibvVl/yF764utxJXiEr3w== X-Google-Smtp-Source: APXvYqxGjLW+6gWmmHeLSRDCehfHHFLorp6PGuYg40qs/NV2UWaFbYkaFVs7D0zPmCeUVHDLIXxvT0Ga7zBDJtovJME= X-Received: by 2002:adf:fd8d:: with SMTP id d13mr3466766wrr.208.1580545532877; Sat, 01 Feb 2020 00:25:32 -0800 (PST) MIME-Version: 1.0 References: <20200124115411.9364-1-pete@akeo.ie> <20200124115411.9364-3-pete@akeo.ie> <3ff9a4fc-ee2a-1dcb-e8d2-d95a0c191a59@akeo.ie> <4b2dd95e-5ced-243a-73b4-a51e1f3040c2@akeo.ie> In-Reply-To: <4b2dd95e-5ced-243a-73b4-a51e1f3040c2@akeo.ie> From: "Ard Biesheuvel" Date: Sat, 1 Feb 2020 09:25:21 +0100 Message-ID: Subject: Re: [edk2-platforms][PATCH v2 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 Fri, 31 Jan 2020 at 19:45, Pete Batard wrote: > > On 2020.01.31 17:53, Ard Biesheuvel wrote: > > On Fri, 31 Jan 2020 at 18:48, Pete Batard wrote: > >> > >> Hi Ard, > >> > >> On 2020.01.31 17:05, Ard Biesheuvel wrote: > >>> On Fri, 24 Jan 2020 at 12:54, 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 | 51 ++++++++++++++++++++ > >>>> Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf | 43 +++++++++++++++++ > >>>> 2 files changed, 94 insertions(+) > >>>> > >>>> diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c > >>>> new file mode 100644 > >>>> index 000000000000..792073a903e9 > >>>> --- /dev/null > >>>> +++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c > >>>> @@ -0,0 +1,51 @@ > >>>> +/** @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) > >>> > >>> I still don't see why we need this conditional. This is a fixed PCD, > >>> so it must be set in the .DSC. If you are going to set it in the .DSC, > >>> why include this driver in the first place? > >> > >> Let's consider this: > >> > >> 1. We expand on PlatformPcdLib to do more than set the MAC Address PCD > >> for the Pi 4. For instance, we use it to set some other PCD, that is > >> specific to the Pi 3, or for an upcoming Pi. In this usage, we would be > >> using PlatformPcdLib with a .DSC where PcdBcmGenetRegistersAddress is > >> not set and therefore defaults to 0. > >> > > > > I don't think a single PlatformPcdLib is realistic, since you'd need > > to link it into different drivers via NULL resolution, all of which > > would inherited the conjunction of all the DEPEX clauses, potentially > > causing circular dependencies. > > > > I'd much rather have either a named library class in BcmGenet.dec > > scope that the platform can implement to provide the MAC, or one that > > can be used via NULL resolution (in cases where it is imaginable that > > the driver does not need the MAC to be provided at all) > > Yes, but the problem is, right now, I have no idea what the Genet driver > is going to look like, so we need a solution that is unlikely to > interfere with what's going to happen in terms of Genet driver > expansion. And this is the least intrusive solution I identified (which > means it will also be easiest to remove if we decide to supersede it). > If the genet stub driver morphs into something more elaborate, it will still require a platform specific way to discover the MAC the platforms wants it to use. > I personally don't think a named library is the better solution because, > ideally, we should have two independent drivers (not libraries), one for > UMAC and one for Genet, considering that we really want is have the > platform rely on a UMAC hardware service implementation since we are > dealing with 2 separate hardware functions, that pertain to networking > but that are not directly related. So if you want to go towards the > "There has to be a better solution that this" route, I'm would actually > push for a separate driver rather than a library. But then I expect that > we're going to create all kinds of problems for Jeremy when he produces > his Genet implementation, which is precisely why I steered away from > doing that in the first place. There are also other alternatives, such > as implementing Simple Network Driver protocol, since this appears to > provide means to set the MAC, but this too is going to interfere with > Jeremy's effort. > > So, and I hope this doesn't come as offensive because this is not my > intention here, I'm afraid that, unless someone else takes that matter > into their own hand, the current proposal is the best you're gonna get, > unless you want to produce that named library class yourself (which, > again, I still don't view as the better solution if that's what we are > looking for). > OK > What also bothers me is that you did state at the end of [1] that: > > > I don't mind this approach, but in general, I'd be happier with a > > specific library class to discover the MAC address. > > which I took as a tacit approval that, even if you didn't exactly like > it, you were still okay with applying this patchset as long as the other > issues were addressed. So I hope you can appreciate that I can't go > around with expecting approval on the general approach of a patchset, > only to see it suddenly rejected by the same person one week later... > That was before you refused to remove the #if PcdGet64() == 0 > But more pragmatically, we need a way to get networking in Linux, which > means UMAC setup in UEFI, now. Not in two weeks. Not in one month. And > short of someone coming up with an alternative tomorrow, I'm still > seeing PCDLib lib as the one means to achieve that, that is going to > create the least trouble for all the parties involved. > > Regards, > > /Pete > > [1] https://edk2.groups.io/g/devel/message/53447 > > >> 2. Since the whole PcdBcmGenetMacAddress setup section would not apply > >> then, we prefer ensuring that it cannot interfere or create issues, by > >> dropping it altogether at build time. Granted, the > >> if (PcdGet64 (PcdBcmGenetMacAddress) == 0) > >> condition could also be seen as enough of a guard, but I don't see much > >> of a reason not to go further, so that further alteration of the code > >> does not risk impacting the Pi 3 or other Pi platforms with future > >> developments. > >> > >> Does that make sense to you? Or do you see this as too far fetched a > >> scenario? > >> > > > > No, I don't think this is something we should care about. > > > >> I'm basically trying to make this whole library easier to maintain in > >> the long run, by adding some pre-emptive provisions to drop code that > >> may not apply (even if, in the current scenario, this library is only > >> used for the Pi 4 platform, where PcdBcmGenetRegistersAddress is always > >> set). > >> > > > > I don't see a single library that sets all kinds of unrelated PCDs as > > something highly useful tbh. > > >