Re: [edk2-devel] [PATCH 1/3] MdeModulePkg: Add ClockRate field to SerialPortInfo

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Mon, Oct 9, 2023 at 4:54 PM Matt DeVillier <matt.devillier@gmail.com> wrote:
>
> On Thu, Oct 5, 2023 at 12:45 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>>
>> On Wed, Oct 4, 2023 at 9:01 PM MrChromebox <matt.devillier@gmail.com> wrote:
>> >
>> > Add the ClockRate field to the UNIVERSAL_PAYLOAD_SERIAL_PORT_INFO
>> > struct, so that the field can be used by UefiPayloadPkg to properly
>> > set up the serial port on boards using a non-standard clock rate.
>> >
>> > Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
>> > Change-Id: I9bcaf03ab63f6a45d2cf25a580f7a2eba388cbbd
>>
>> Series-generic-feedback: Remove Change-Id lines and CC the proper
>> maintainers for each patch
>
>
> ack
>
>>
>> > ---
>> >  MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h b/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h
>> > index 3c4459e2c0..e3c9f93654 100644
>> > --- a/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h
>> > +++ b/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h
>> > @@ -19,6 +19,7 @@ typedef struct {
>> >    BOOLEAN                             UseMmio;
>> >    UINT8                               RegisterStride;
>> >    UINT32                              BaudRate;
>> > +  UINT32                              ClockRate;
>>
>> I don't think you can do this? UNIVERSAL_PAYLOAD_SERIAL_PORT_INFO is
>> part of a spec (https://universalscalablefirmware.github.io/documentation/2_universal_payload.html)
>> and it doesn't even seem to be versioned, so you'd just break ABI. Am
>> I missing something?
>
>
> The USF spec says that it is currently at version 0.7 and under development / not final. I don't see why adding a field to a hob is problematic provided it's properly documented. The alternatives are some really hacky workarounds to pass the necessary data, or the serial port not working.

My 2 cents, as someone that neither maintains nor works on this code:
1) This sounds like something that needs to be discussed with the spec
people (good luck! lol)
2) Changing the structure abruptly like this seems like it would break
many current users. HOWEVER:
The spec (https://universalscalablefirmware.github.io/documentation/2_universal_payload.html#common-payload-header)
says that the common header's Revision (indeed, it's versioned and I
missed that) should be incremented if current members are renamed or
reinterpreted, but not if you just append members. So it seems that if
you add ClockRate like this:

--- a/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h
+++ b/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h
@@ -19,6 +19,7 @@ typedef struct {
   BOOLEAN                             UseMmio;
   UINT8                               RegisterStride;
   UINT32                              BaudRate;
   EFI_PHYSICAL_ADDRESS                RegisterBase;
+  UINT32                              ClockRate;
} UNIVERSAL_PAYLOAD_SERIAL_PORT_INFO;

--

this should be acceptable for the spec people and
EDK2/coreboot/SlimBootloader/etc people, given all the current
consuming code takes the generic header's Length into account (as
specified by the spec).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109534): https://edk2.groups.io/g/devel/message/109534
Mute This Topic: https://groups.io/mt/101763374/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-