From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: tien.hock.loh@intel.com) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by groups.io with SMTP; Thu, 25 Apr 2019 23:02:00 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Apr 2019 23:01:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,396,1549958400"; d="scan'208";a="226852076" Received: from kmsmsx156.gar.corp.intel.com ([172.21.138.133]) by orsmga001.jf.intel.com with ESMTP; 25 Apr 2019 23:01:52 -0700 Received: from pgsmsx110.gar.corp.intel.com ([169.254.13.159]) by KMSMSX156.gar.corp.intel.com ([169.254.1.187]) with mapi id 14.03.0415.000; Fri, 26 Apr 2019 14:01:51 +0800 From: "Loh, Tien Hock" To: "Wu, Hao A" , "thloh85@gmail.com" , "devel@edk2.groups.io" , "Kinney, Michael D" CC: "Wang, Jian J" Subject: Re: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support Thread-Topic: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support Thread-Index: AQHU+y4DePhCbzBBs02Goi9OhwNWD6ZNZ4IAgACMGDA= Date: Fri, 26 Apr 2019 06:01:50 +0000 Message-ID: References: <1556172774-134132-1-git-send-email-tien.hock.loh@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDY5NDdhNjQtOTdjNC00MDcwLThjZTMtNTdiZGFmYjY0MWZiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiS210UkhqS2tKT3R2MWpJcTlxb1pUU01NeHVwWnpyYmdJR0hmbVR2ZUJKR1NDUXQ0QThsZnVvQmNLWkRLaklVWSJ9 x-originating-ip: [172.30.20.206] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Wu, Hao A > Sent: Friday, April 26, 2019 1:36 PM > To: thloh85@gmail.com; devel@edk2.groups.io; Loh, Tien Hock > ; Kinney, Michael D > > Cc: Wang, Jian J > Subject: RE: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add > Mmio32 support >=20 > > -----Original Message----- > > From: Loh, Tien Hock > > Sent: Thursday, April 25, 2019 2:13 PM > > To: thloh85@gmail.com devel@edk2.groups.io; Kinney, Michael D > > Cc: Loh, Tien Hock; Wang, Jian J; Wu, Hao A > > Subject: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 > > support > > > > From: "Tien Hock, Loh" > > > > Some busses doesn't allow 8 bit MMIO read/write, this adds support for > > 32 bits read/write > > > > Signed-off-by: "Tien Hock, Loh" > > Cc: Jian J Wang > > Cc: Hao Wu > > > > --- > > v4 > > - Updates Pcd name to a better name: PcdSerialRegisterAccessWidth > > v3 > > - Updates the Pcd to be UINT8 to allow more options such as 16 bits > > access in the future > > - Updated copyright date > > v2 > > - Updates the Pcd name to PcdSerialMmio32BitAccess and access 32 bits > > register if PcdSerialUseMmio and PcdSerialMmio32BitAccess is set > > --- > > .../Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c | 12 > > +++++++++++- > > .../BaseSerialPortLib16550/BaseSerialPortLib16550.inf | 3 ++- > > MdeModulePkg/MdeModulePkg.dec | 7 ++++= +++ > > 3 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git > > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c > > index 34df34d..5910606 100644 > > --- > > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c > > +++ > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c > > @@ -2,7 +2,7 @@ > > 16550 UART Serial Port library functions > > > > (C) Copyright 2014 Hewlett-Packard Development Company, L.P.
> > - Copyright (c) 2006 - 2018, Intel Corporation. All rights > > reserved.
> > + Copyright (c) 2006 - 2019, Intel Corporation. All rights > > + reserved.
> > Copyright (c) 2018, AMD Incorporated. All rights reserved.
> > > > SPDX-License-Identifier: BSD-2-Clause-Patent @@ -77,6 +77,11 @@ > > SerialPortReadRegister ( > > ) > > { > > if (PcdGetBool (PcdSerialUseMmio)) { > > + if (PcdGet8 (PcdSerialRegisterAccessWidth) =3D=3D 8) { > > + return MmioRead8 (Base + Offset * PcdGet32 > (PcdSerialRegisterStride)); > > + } else if (PcdGet8 (PcdSerialRegisterAccessWidth) =3D=3D 32) { > > + return (UINT8) MmioRead32 (Base + Offset * PcdGet32 > > (PcdSerialRegisterStride)); > > + } > > return MmioRead8 (Base + Offset * PcdGet32 > > (PcdSerialRegisterStride)); >=20 > Hello Tien Hock, >=20 > Sorry for the delayed response. No problem, thanks for reviewing the patch. >=20 > The above codes deal with 3 cases (also true for SerialPortWriteRegister)= : >=20 > 1. PcdSerialRegisterAccessWidth is set to 8 2. PcdSerialRegisterAccessWid= th is > set to 32 3. PcdSerialRegisterAccessWidth is set to values other than 8 o= r 32 > (For 3, the behavior falls back to default, which is okay to me) >=20 > For case 1 and 3, they end up calling MmioRead8(). So IMO, maybe we can > simplifies the logic to: >=20 > if (PcdGet8 (PcdSerialRegisterAccessWidth) =3D=3D 32) { > MmioRead32 (...); > } else { > MmioRead8 (...); > } >=20 > What do you think? >=20 I'm OK either way, I'll make the changes. >=20 > Also, could you help to update the function description comments for >=20 > * SerialPortReadRegister() > * SerialPortWriteRegister() >=20 > to reflect their behavior with regard to the new PCD? OK noted. >=20 > Lastly, when sending a new version of the patch, could you help to append > option '--subject-prefix=3D"PATCH vN"' to the 'git-format-patch' command.= So > for this v4 patch, it will be '--subject-prefix=3D"PATCH v4"'. OK noted. >=20 >=20 > Best Regards, > Hao Wu >=20 > > } else { > > return IoRead8 (Base + Offset * PcdGet32 > > (PcdSerialRegisterStride)); @@ -104,6 +109,11 @@ SerialPortWriteRegiste= r > ( > > ) > > { > > if (PcdGetBool (PcdSerialUseMmio)) { > > + if (PcdGet8 (PcdSerialRegisterAccessWidth) =3D=3D 8) { > > + return MmioWrite8 (Base + Offset * PcdGet32 > > + (PcdSerialRegisterStride), > > Value); > > + } else if (PcdGet8 (PcdSerialRegisterAccessWidth) =3D=3D 32) { > > + return (UINT8) MmioWrite32 (Base + Offset * PcdGet32 > > (PcdSerialRegisterStride), (UINT8)Value); > > + } > > return MmioWrite8 (Base + Offset * PcdGet32 > > (PcdSerialRegisterStride), Value); > > } else { > > return IoWrite8 (Base + Offset * PcdGet32 > > (PcdSerialRegisterStride), Value); diff --git > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i > > nf > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i > > nf > > index b60779c..8b4ae3f 100644 > > --- > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i > > nf > > +++ > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i > > nf > > @@ -1,7 +1,7 @@ > > ## @file > > # SerialPortLib instance for 16550 UART. > > # > > -# Copyright (c) 2006 - 2018, Intel Corporation. All rights > > reserved.
> > +# Copyright (c) 2006 - 2019, Intel Corporation. All rights > > +reserved.
> > # SPDX-License-Identifier: BSD-2-Clause-Patent # ## @@ -29,6 +29,7 > > @@ > > BaseSerialPortLib16550.c > > > > [Pcd] > > + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth ## > > SOMETIMES_CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio ## > > CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl > ## > > CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable ## > > SOMETIMES_CONSUMES > > diff --git a/MdeModulePkg/MdeModulePkg.dec > > b/MdeModulePkg/MdeModulePkg.dec index be84916..2ef48f2 100644 > > --- a/MdeModulePkg/MdeModulePkg.dec > > +++ b/MdeModulePkg/MdeModulePkg.dec > > @@ -1170,6 +1170,13 @@ > > # @Prompt Serial port registers use MMIO. > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|FALSE|BOOLEAN|0x > 00 > > 020000 > > > > + ## Indicates the access width for 16550 serial port registers. > > + # Default is 8-bit access mode.

> > + # 8 - 16550 serial port registers are accessed in 8-bit width. > > + # 32 - 16550 serial port registers are accessed in 32-bit width. > > + # @Prompt Serial port register access width. > > + > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth|8|UINT8| > 0x > > 00020007 > > + > > ## Indicates if the 16550 serial port hardware flow control will be > enabled. > > Default is FALSE.

> > # TRUE - 16550 serial port hardware flow control will be enabled.=
> > # FALSE - 16550 serial port hardware flow control will be disabled= .
> > -- > > 2.2.2