On 2019.10.10 17:43, Leif Lindholm wrote:
On Thu, Oct 10, 2019 at 01:41:06PM +0100, Pete Batard wrote:
Hi Leif,
On 2019.10.10 09:39, Leif Lindholm wrote:
On Tue, Oct 08, 2019 at 01:38:37PM +0100, Pete Batard wrote:
This patch introduces the capability to also query the Model Name/
Manufacturer Name/CPU Name/Firmware Revision using the RpiFirmware
protocol. This is aims at making the driver more suitable to cater
for platforms other than the Raspberry Pi 3 as well as simplifying
the population of entries in PlatformSmbiosDxe.
Signed-off-by: Pete Batard <p...@akeo.ie>
---
Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 155
+++++++++++++++++++-
Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58
++++++--
2 files changed, 196 insertions(+), 17 deletions(-)
diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index 9b5ee1946279..378c99bcba05 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -461,7 +461,7 @@ typedef struct {
RPI_FW_TAG_HEAD TagHead;
RPI_FW_MODEL_REVISION_TAG TagBody;
UINT32 EndTag;
-} RPI_FW_GET_MODEL_REVISION_CMD;
+} RPI_FW_GET_REVISION_CMD;
#pragma pack()
STATIC
@@ -471,7 +471,7 @@ RpiFirmwareGetModelRevision (
OUT UINT32 *Revision
)
{
- RPI_FW_GET_MODEL_REVISION_CMD *Cmd;
+ RPI_FW_GET_REVISION_CMD *Cmd;
EFI_STATUS Status;
UINT32 Result;
@@ -506,6 +506,153 @@ RpiFirmwareGetModelRevision (
return EFI_SUCCESS;
}
+STATIC
+EFI_STATUS
+EFIAPI
+RpiFirmwareGetFirmwareRevision (
+ OUT UINT32 *Revision
+ )
+{
+ RPI_FW_GET_REVISION_CMD *Cmd;
+ EFI_STATUS Status;
+ UINT32 Result;
+
+ if (!AcquireSpinLockOrFail (&mMailboxLock)) {
+ DEBUG ((DEBUG_ERROR, "%a: failed to acquire spinlock\n", __FUNCTION__));
+ return EFI_DEVICE_ERROR;
+ }
+
+ Cmd = mDmaBuffer;
+ ZeroMem (Cmd, sizeof (*Cmd));
+
+ Cmd->BufferHead.BufferSize = sizeof (*Cmd);
+ Cmd->BufferHead.Response = 0;
+ Cmd->TagHead.TagId = RPI_MBOX_GET_REVISION;
+ Cmd->TagHead.TagSize = sizeof (Cmd->TagBody);
+ Cmd->TagHead.TagValueSize = 0;
+ Cmd->EndTag = 0;
+
+ Status = MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_CHANNEL,
&Result);
+
+ ReleaseSpinLock (&mMailboxLock);
+
+ if (EFI_ERROR (Status) ||
+ Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) {
+ DEBUG ((DEBUG_ERROR,
+ "%a: mailbox transaction error: Status == %r, Response == 0x%x\n",
+ __FUNCTION__, Status, Cmd->BufferHead.Response));
+ return EFI_DEVICE_ERROR;
+ }
+
+ *Revision = Cmd->TagBody.Revision;
+ return EFI_SUCCESS;
+}
+
+STATIC
+CHAR8*
+EFIAPI
+RpiFirmwareGetModelName (
+ IN INTN ModelId
+ )
+{
+ UINT32 Revision;
+
+ // If a negative ModelId is passed, detect it.
+ if ((ModelId < 0) && (RpiFirmwareGetModelRevision (&Revision) ==
EFI_SUCCESS))
Style-wise, please always use {} with if-statements.
Will do.
Beyond that, this pattern repeats identcally in three functions in
this file. Meanwhile, there is never any error handling of
RpiFirmwareGetModelRevision other than if not successful, we leave it
as negative.
Yes, because then we'll get the default case (that returns "Unknown ..."),
which is precisely what we want if we can't access the model revision for
any reason.
Are there other intended uses of that function, or could
we move this logic there?
I'm not sure how that would work, since ModelId, ManufacturerId, and CpuId
are for different parts of the bit fields, so, if I understand what you're
suggesting correctly (but I'm not really sure I do) trying to move the check
for negative value into RpiFirmwareGetModelRevision () wouldn't work that
great. Plus then the function name would become a misnommer.
Are you really seeing a functional issue with the current code, or something
that would make a possible contributor wanting to modify this section
puzzled as to what we're trying to accomplish here (which I think is pretty
clear, but then again, I wrote the code so maybe I'm not the most impartial?
Because if not, I'd rather save my limited supply of time, and just go with
a v3 that only adds the {} suggested.
I see no functional issues with the code, but with my own limited
supply of time I try to ensure the code stays as simple as possible so
I don't need to stop and think where there isn't actually anything
complicated going on (like here). I am after all a bear of very little
brain.
I also will *always* trigger on seeing a near-identical pattern
repeated many times in the same file.
In this instance, I find myself spending way too much time untangling
what is actually going on. I see
- SysInfoUpdateSmbiosType1 () calling mFwProtocol->GetModelRevision (),
and then using the result from that to determine which parameters to
pass to mFwProtocol->GetModelName() and
mFwProtocol->GetManufacturerName().
- Both of which if the GetModelRevision () call was unsuccessful get
passed -1.
The whole point is to create new generic calls that may be used by any
driver that needs them, and not only PlatformSmbiosDxe. That's our
starting point. And the idea is that, unlike what currently happens in
SysInfoUpdateSmbiosType1 (), not all of these calls may also happen to
be calling GetModelRevision ().
As such, I believe it makes a lot of sense to have the new calls
designed as they are, else we find ourselves trying to cover the case of
minimizing calls to GetModelRevision (), either in PlatformSmbiosDxe or
RpiFirmwareDxe, which seems an unwarranted micro optimization goal to me.
- Now, when the parameter is -1, they both call straight into
GetModelRevision (). Why are we expecting the second call to succeed
when the first one fails?
We're not expecting anything. We're receiving a call from a driver, of
which we have no idea what actions it performed beforehand, that may
happens to ask us to replicate some of these actions. And, in a generic
manner, and regardless of a state we should not have any idea about due
to encapsulation, we do issue a call to GetModelRevision ().
Or, let me put it this way: From
PlatformSmbiosDxe:SysInfoUpdateSmbiosType1 ()'s perspective, the call to
RpiFirmwareDxe:GetModelName () is supposed to be a black box that
returns a result it can use. And from RpiFirmwareDxe:GetModelName ()'s
perspective, what potentially duplicated actions the caller did
beforehand is also a black box.
Ideally, this is intended to make both drivers, with their better
abstracted goals, easier to understand and maintain, so I can't help but
feel quite dismayed that it's the very opposite that appears to be
occurring here. Still, if anything, I would assert that any action that
aims at reducing calls to GetModelRevision () should be reported against
PlatformSmbiosDxe rather than the lower level (and therefore more
generic) RpiFirmwareDxe.
Regards,
/Pete
Am I completely misreading what this code does?
Does GetModelRevision() return different values depending on when you
call it?
Best Regards,
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48747): https://edk2.groups.io/g/devel/message/48747
Mute This Topic: https://groups.io/mt/34441816/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
