From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: hao.a.wu@intel.com) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by groups.io with SMTP; Thu, 25 Apr 2019 22:36:30 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Apr 2019 22:36:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,396,1549958400"; d="scan'208";a="138959475" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga006.jf.intel.com with ESMTP; 25 Apr 2019 22:36:29 -0700 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 25 Apr 2019 22:36:29 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 25 Apr 2019 22:36:28 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.39]) with mapi id 14.03.0415.000; Fri, 26 Apr 2019 13:36:27 +0800 From: "Wu, Hao A" 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 Thread-Topic: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support Thread-Index: AQHU+y4DV3DBXxGVo0SGyZj+U4Bn46ZNwoPA Date: Fri, 26 Apr 2019 05:36:26 +0000 Message-ID: References: <1556172774-134132-1-git-send-email-tien.hock.loh@intel.com> In-Reply-To: <1556172774-134132-1-git-send-email-tien.hock.loh@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 >=20 > From: "Tien Hock, Loh" >=20 > Some busses doesn't allow 8 bit MMIO read/write, this adds support for > 32 bits read/write >=20 > Signed-off-by: "Tien Hock, Loh" > Cc: Jian J Wang > Cc: Hao Wu >=20 > --- > v4 > - Updates Pcd name to a better name: PcdSerialRegisterAccessWidth > v3 > - Updates the Pcd to be UINT8 to allow more options such as 16 bits acces= s > 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(-) >=20 > 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 >=20 > (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.
>=20 > 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 (PcdSerialRegisterStrid= e)); > + } else if (PcdGet8 (PcdSerialRegisterAccessWidth) =3D=3D 32) { > + return (UINT8) MmioRead32 (Base + Offset * PcdGet32 > (PcdSerialRegisterStride)); > + } > return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride)= ); Hello Tien Hock, Sorry for the delayed response. The above codes deal with 3 cases (also true for SerialPortWriteRegister): 1. PcdSerialRegisterAccessWidth is set to 8 2. PcdSerialRegisterAccessWidth is set to 32 3. PcdSerialRegisterAccessWidth is set to values other than 8 or 32 (For 3, the behavior falls back to default, which is okay to me) For case 1 and 3, they end up calling MmioRead8(). So IMO, maybe we can simplifies the logic to: if (PcdGet8 (PcdSerialRegisterAccessWidth) =3D=3D 32) { MmioRead32 (...); } else { MmioRead8 (...); } What do you think? Also, could you help to update the function description comments for * SerialPortReadRegister() * SerialPortWriteRegister() to reflect their behavior with regard to the new PCD? 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. S= o for this v4 patch, it will be '--subject-prefix=3D"PATCH v4"'. Best Regards, Hao Wu > } else { > return IoRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride)); > @@ -104,6 +109,11 @@ SerialPortWriteRegister ( > ) > { > if (PcdGetBool (PcdSerialUseMmio)) { > + if (PcdGet8 (PcdSerialRegisterAccessWidth) =3D=3D 8) { > + return MmioWrite8 (Base + Offset * PcdGet32 (PcdSerialRegisterStri= de), > 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.inf > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf > index b60779c..8b4ae3f 100644 > --- > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf > +++ > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf > @@ -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 >=20 > [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. >=20 > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|FALSE|BOOLEAN|0x00 > 020000 >=20 > + ## 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 en= abled. > Default is FALSE.

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