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

["MrChromebox" <matt.devillier@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4041 bytes --]

hi Pedro,

thanks for the input, I will adjust the patch to append for v2

On Wed, Oct 11, 2023 at 2:16 PM Pedro Falcato <pedro.falcato@gmail.com>
wrote:

> 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 (#109535): https://edk2.groups.io/g/devel/message/109535
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 5868 bytes --]