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.web09.11235.1580560363543921952 for ; Sat, 01 Feb 2020 04:32:44 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=QUkluNPY; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.65, mailfrom: pete@akeo.ie) Received: by mail-wr1-f65.google.com with SMTP id w12so937218wrt.2 for ; Sat, 01 Feb 2020 04:32:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=L6DT5j1VuLziu/4fRWQPhwhzUYEaNmo336dV6OYMjh0=; b=QUkluNPY7+VDqJTOqXersNcFTZZocpn1+7q5HiRPC73pjDVFkXkvODggpqRyiPB8Es nZMWUMFmtnf4MpCw8fS6TQyajj6ce4zkY96vIu0aV1Zb6WUagItvwPIN1HTKGyjCImvO IYr9bGeyouKma5hP3cvsyfpDXlYO+mgDj/EwfV7tnwNiR1aOix+4DWMGMReF0pNigPGG uLQmw3/jWxs42Key6pA06H5kY5fXinsem85rB3W+wrhz7wP/0ONZXsTtGU2hBaq8B95x 3j+96D0uF5RpqooqXJgqQ5NhV+W/CD3wFn5Elozf4uxImwbMJR6FB3bJMmHh239OIHvN GGEA== 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-language :content-transfer-encoding; bh=L6DT5j1VuLziu/4fRWQPhwhzUYEaNmo336dV6OYMjh0=; b=cFMV8KFuGFBfdk927F1mLOMp8gR/D0+dqXhzITwTbacmKQB6NR4ntOiabILJU7v8u2 nCjBWIxfb/R3cZAbZ+bZt5tTPKbTm6NAwY4vupO9+ZTxkqF4RksCRbAg6F/t3bSG7mIj u6Kf4df5P7GF3TkbWuVqt3PJeSNfCw7IfWiAUBVUNys3I3D4FtfKqkYOJtqiTIuLCn5Z OsSkFEf1aurT9VXj9oKbFPXCwtS+g2B+NJUY8+IXk+JepF+ohB90KvHLDKNpv28CjRk0 IbhU8awHELfU+ONlTKxgCO+GRiJypc5B4cZcAW2V6PEt32Nhhlt9qHdLTlZOor9owZdm miMg== X-Gm-Message-State: APjAAAWt9uLaPv17bVx6YAaIMjBuRy4791+aeDHgjRX6rvUHsqSNIceU +z1pMdeLsAqxFDsLVlVNt0Rh9g== X-Google-Smtp-Source: APXvYqzz/PtAlKULOHzeytcrfo6e200zkoK8Ja945ux8xVv2MqpzgPj/VnwCwSPkkFrhCaB283kf+A== X-Received: by 2002:a5d:4b47:: with SMTP id w7mr4800185wrs.276.1580560362036; Sat, 01 Feb 2020 04:32:42 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.49.247]) by smtp.googlemail.com with ESMTPSA id 124sm15945121wmc.29.2020.02.01.04.32.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 01 Feb 2020 04:32:41 -0800 (PST) Subject: Re: [edk2-platforms][PATCH v2 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address To: Ard Biesheuvel Cc: edk2-devel-groups-io , Leif Lindholm , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Jeremy Linton 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> From: "Pete Batard" Message-ID: Date: Sat, 1 Feb 2020 12:32:39 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit On 2020.02.01 09:48, Ard Biesheuvel wrote: > On Sat, 1 Feb 2020 at 09:25, Ard Biesheuvel wrote: >> >> 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 I actually have no problem at all removing that line. My take away until just now was that you did not understand why the line was there, not that you really wanted to see it gone. For the current use of this driver (RPi4 platform only), I'm fine with not having the line, and only adding it at a later stage if/as needed. > Apologies if that came out grumpy. Same for me. I'm not that intractable about PlatformPcdLib, but if there's something you really don't want to see in it, please don't be afraid to explicitly state so. Therefore, unless you are fine with removing it yourself during integration, I can send a v2 that removes the #ifdef condition. > > The reality is that it is highly unlikely that the genet driver will > ever be usable in another platform without substantial rework, Well, I genuinely have no idea what the genet driver is going to look like, so I'm not willing to make any predictions one way or another. All I know is that Broadcom appear to have released 5 versions of the genet hardware, so they don't seem intent on retiring it, which means we might see it being used on other platforms. > so the > kind of future proofing that we're doing here is both too much and too > little. If you are concerned about timelines, just keep it simple and > inflexible, and we can refactor it later. By that time, I will also > hopefully have more bandwidth to look at this stuff. > > Unrelated: you don't happen to be at FOSDEM do you? I wish I was, but I'm afraid not. Maybe next year... Regards, /Pete