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

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

[MrChromebox]

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
---
 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;
   EFI_PHYSICAL_ADDRESS                RegisterBase;
 } UNIVERSAL_PAYLOAD_SERIAL_PORT_INFO;
 #pragma pack()
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109335): https://edk2.groups.io/g/devel/message/109335
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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/3] UefiPayloadEntry: Set serial ClockRate from parsed SerialInfo

[MrChromebox]

Extract and use the serial port clock rate provided by coreboot via
the InputHertz field.

Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Change-Id: If764bd7c0b691cf887205471d0343fdf62372141
---
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
index 030a5baed9..91dfac0d76 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
@@ -431,6 +431,7 @@ _ModuleEntryPoint (
     UniversalSerialPort->UseMmio         = (SerialPortInfo.Type == 1) ? FALSE : TRUE;
     UniversalSerialPort->RegisterBase    = SerialPortInfo.BaseAddr;
     UniversalSerialPort->BaudRate        = SerialPortInfo.Baud;
+    UniversalSerialPort->ClockRate       = SerialPortInfo.InputHertz;
     UniversalSerialPort->RegisterStride  = (UINT8)SerialPortInfo.RegWidth;
   }
 
-- 
2.34.1



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

[edk2-devel] [PATCH 3/3] PlatformHookLib: Set PcdSerialClockRate using HOB data

[MrChromebox]

Fixes serial output on platforms using coreboot and a non-default
clock rate such as AMD Picasso and newer Zen-based platforms.

Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Change-Id: I91290397852176754e9a34ec6e5829044f41d15a
---
 UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c   | 5 +++++
 UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf | 1 +
 2 files changed, 6 insertions(+)

diff --git a/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c b/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
index 60a17b8fc2..e3d47ac2fa 100644
--- a/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
+++ b/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
@@ -90,6 +90,11 @@ PlatformHookSerialPortInitialize (
       return Status;
     }
 
+    Status = PcdSet32S (PcdSerialClockRate, SerialPortInfo->ClockRate);
+    if (RETURN_ERROR (Status)) {
+      return Status;
+    }
+
     return RETURN_SUCCESS;
   }
 
diff --git a/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf b/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
index 7ac6bfa1b1..e2908cfbca 100644
--- a/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
+++ b/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
@@ -38,3 +38,4 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase    ## PRODUCES
   gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate        ## PRODUCES
   gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride  ## PRODUCES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate       ## PRODUCES
-- 
2.34.1



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

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

[Pedro Falcato]

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

> ---
>  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?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109361): https://edk2.groups.io/g/devel/message/109361
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[MrChromebox]

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

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.


>
> --
> Pedro
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109461): https://edk2.groups.io/g/devel/message/109461
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: 3751 bytes --]

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

[Pedro Falcato]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[MrChromebox]

[-- 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 --]

Re: [edk2-devel] [PATCH 3/3] PlatformHookLib: Set PcdSerialClockRate using HOB data

[Sean Rhodes]

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

PR created - https://github.com/tianocore/edk2/pull/5513

LGTM but I can't check it against UPL.

@Dong, Guo <guo.dong@intel.com> Would you be able to take a look please'?

On Wed, 4 Oct 2023 at 21:02, MrChromebox <matt.devillier@gmail.com> wrote:

> Fixes serial output on platforms using coreboot and a non-default
> clock rate such as AMD Picasso and newer Zen-based platforms.
>
> Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
> Change-Id: I91290397852176754e9a34ec6e5829044f41d15a
> ---
>  UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c   | 5 +++++
>  UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf | 1 +
>  2 files changed, 6 insertions(+)
>
> diff --git a/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
> b/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
> index 60a17b8fc2..e3d47ac2fa 100644
> --- a/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
> +++ b/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
> @@ -90,6 +90,11 @@ PlatformHookSerialPortInitialize (
>        return Status;
>      }
>
> +    Status = PcdSet32S (PcdSerialClockRate, SerialPortInfo->ClockRate);
> +    if (RETURN_ERROR (Status)) {
> +      return Status;
> +    }
> +
>      return RETURN_SUCCESS;
>    }
>
> diff --git a/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> b/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> index 7ac6bfa1b1..e2908cfbca 100644
> --- a/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> +++ b/UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> @@ -38,3 +38,4 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase    ## PRODUCES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate        ## PRODUCES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride  ## PRODUCES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate       ## PRODUCES
> --
> 2.34.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#109337): https://edk2.groups.io/g/devel/message/109337
> Mute This Topic: https://groups.io/mt/101763377/6718866
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [sean@starlabs.systems]
> ------------
>
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117355): https://edk2.groups.io/g/devel/message/117355
Mute This Topic: https://groups.io/mt/101763377/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: 4451 bytes --]