On 2020.01.31 17:53, Ard Biesheuvel wrote:
On Fri, 31 Jan 2020 at 18:48, Pete Batard <p...@akeo.ie> wrote:
Hi Ard,
On 2020.01.31 17:05, Ard Biesheuvel wrote:
On Fri, 24 Jan 2020 at 12:54, Pete Batard <p...@akeo.ie> 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 <p...@akeo.ie>
---
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 <p...@akeo.ie>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ **/
+
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PrintLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
+#include <Protocol/RpiFirmware.h>
+
+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).
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).
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...
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53621): https://edk2.groups.io/g/devel/message/53621
Mute This Topic: https://groups.io/mt/70068668/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
