From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 292C778003C for ; Wed, 11 Oct 2023 23:22:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=jqpx/b3Q35IjUzDgcncdYLVH3fADOubLiT0hk8HFN1k=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1697066527; v=1; b=CsuiqEioz93NHwp8uN10wS29wIT9JHlAS1cap6xSYbhCgSC6/6/tYAwlGJpw7T+PfEywCyPh o417n16sj5aDG/0JWsqGto8cc35Qk++xQu7J6fC+jbGLS0L8kA84ZYk3lBMP3vI7CBgaJxgLnw6 Qa7reBRhupYnYsJxyDVFlAB8= X-Received: by 127.0.0.2 with SMTP id R7UrYY7687511xNzn0EDPpx6; Wed, 11 Oct 2023 16:22:07 -0700 X-Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) by mx.groups.io with SMTP id smtpd.web10.1345.1697066526766348727 for ; Wed, 11 Oct 2023 16:22:07 -0700 X-Received: by mail-lf1-f52.google.com with SMTP id 2adb3069b0e04-504a7f9204eso522457e87.3 for ; Wed, 11 Oct 2023 16:22:06 -0700 (PDT) X-Gm-Message-State: VwBWqbnCVmsQfP4wLWUmHCvdx7686176AA= X-Google-Smtp-Source: AGHT+IGId94iVU4IZ0ywIqfBnaTbnujrfpvd2a5fE8HMNv0XPSq2NlzHWzcVWqujm3qxgEKjxjpk9yxEzdnXCJupfBM= X-Received: by 2002:a05:6512:1586:b0:500:daf6:3898 with SMTP id bp6-20020a056512158600b00500daf63898mr23650393lfb.26.1697066524478; Wed, 11 Oct 2023 16:22:04 -0700 (PDT) MIME-Version: 1.0 References: <9143994e6a5f37650930abfc4141c6061e5bef98.1696448945.git.matt.devillier@gmail.com> In-Reply-To: From: "MrChromebox" Date: Wed, 11 Oct 2023 16:21:52 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/3] MdeModulePkg: Add ClockRate field to SerialPortInfo To: Pedro Falcato Cc: devel@edk2.groups.io, Zhiguang Liu , "Ni, Ray" , gua.guo@intel.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,matt.devillier@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="0000000000005a13d506077917d5" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=CsuiqEio; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --0000000000005a13d506077917d5 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable hi Pedro, thanks for the input, I will adjust the patch to append for v2 On Wed, Oct 11, 2023 at 2:16=E2=80=AFPM Pedro Falcato wrote: > On Mon, Oct 9, 2023 at 4:54=E2=80=AFPM Matt DeVillier > wrote: > > > > On Thu, Oct 5, 2023 at 12:45=E2=80=AFPM Pedro Falcato > wrote: > >> > >> On Wed, Oct 4, 2023 at 9:01=E2=80=AFPM MrChromebox > 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 > >> > 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_pay= load.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 n= ot > 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_pay= load.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 > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --0000000000005a13d506077917d5 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
hi Pedro,

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

On Wed, Oct 11, 2023 at 2:16= =E2=80=AFPM Pedro Falcato <pe= dro.falcato@gmail.com> wrote:
On Mon, Oct 9, 2023 at 4:54=E2=80=AFPM Matt DeVillier = <matt.devi= llier@gmail.com> wrote:
>
> On Thu, Oct 5, 2023 at 12:45=E2=80=AFPM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>>
>> On Wed, Oct 4, 2023 at 9:01=E2=80=AFPM 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 pr= operly
>> > set up the serial port on boards using a non-standard clock r= ate.
>> >
>> > Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
>> > Change-Id: I9bcaf03ab63f6a45d2cf25a580f7a2eba388cbbd
>>
>> Series-generic-feedback: Remove Change-Id lines and CC the proper<= br> >> maintainers for each patch
>
>
> ack
>
>>
>> > ---
>> >=C2=A0 MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h = | 1 +
>> >=C2=A0 1 file changed, 1 insertion(+)
>> >
>> > diff --git a/MdeModulePkg/Include/UniversalPayload/SerialPort= Info.h b/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h
>> > index 3c4459e2c0..e3c9f93654 100644
>> > --- a/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h<= br> >> > +++ b/MdeModulePkg/Include/UniversalPayload/SerialPortInfo.h<= br> >> > @@ -19,6 +19,7 @@ typedef struct {
>> >=C2=A0 =C2=A0 BOOLEAN=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0UseMmio;
>> >=C2=A0 =C2=A0 UINT8=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Regist= erStride;
>> >=C2=A0 =C2=A0 UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BaudRate; >> > +=C2=A0 UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ClockRate;
>>
>> I don't think you can do this? UNIVERSAL_PAYLOAD_SERIAL_PORT_I= NFO is
>> part of a spec (https://universalscalablefirmware.github.io/documentation/2_universa= l_payload.html)
>> and it doesn't even seem to be versioned, so you'd just br= eak ABI. Am
>> I missing something?
>
>
> The USF spec says that it is currently at version 0.7 and under develo= pment / not final. I don't see why adding a field to a hob is problemat= ic provided it's properly documented. The alternatives are some really = hacky workarounds to pass the necessary data, or the serial port not workin= g.

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_u= niversal_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 {
=C2=A0 =C2=A0BOOLEAN=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0UseMmio;
=C2=A0 =C2=A0UINT8=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0RegisterStride;
=C2=A0 =C2=A0UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BaudRate;
=C2=A0 =C2=A0EFI_PHYSICAL_ADDRESS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 RegisterBase;
+=C2=A0 UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#109535) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--0000000000005a13d506077917d5--