diff mbox

[edk2,4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

Message ID 20161201175633.2538-5-lersek@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek Dec. 1, 2016, 5:56 p.m. UTC 
where "QemuFwCfgDma.h" was added in the previous patch.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>

---
 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-----------------
 1 file changed, 3 insertions(+), 21 deletions(-)

-- 
2.9.2


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Dec. 2, 2016, 11:03 a.m. UTC | #1 
On Thu, Dec 01, 2016 at 06:56:32PM +0100, Laszlo Ersek wrote:
> where "QemuFwCfgDma.h" was added in the previous patch.

> 

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Laszlo Ersek <lersek@redhat.com>


This is nice cleanup.
One bit of bikeshedding below, address or ignore - regardless:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> ---

>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-----------------

>  1 file changed, 3 insertions(+), 21 deletions(-)

> 

> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c

> index 2fd8d9050566..62a85dff328e 100644

> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c

> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c

> @@ -25,6 +25,8 @@

>  

>  #include <Protocol/FdtClient.h>

>  

> +#include <IndustryStandard/QemuFwCfgDma.h>

> +


So, I forget if we have official guidelines on this, but instinctively
I would put IndustryStandard before Library (alphabetically).

Regards,

Leif

>  STATIC UINTN mFwCfgSelectorAddress;

>  STATIC UINTN mFwCfgDataAddress;

>  STATIC UINTN mFwCfgDmaAddress;

> @@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;

>  //

>  STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;

>  

> -//

> -// Communication structure for DmaReadBytes(). All fields are encoded in big

> -// endian.

> -//

> -#pragma pack (1)

> -typedef struct {

> -  UINT32 Control;

> -  UINT32 Length;

> -  UINT64 Address;

> -} FW_CFG_DMA_ACCESS;

> -#pragma pack ()

> -

> -//

> -// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).

> -//

> -#define FW_CFG_DMA_CTL_ERROR  BIT0

> -#define FW_CFG_DMA_CTL_READ   BIT1

> -#define FW_CFG_DMA_CTL_SKIP   BIT2

> -#define FW_CFG_DMA_CTL_SELECT BIT3

> -

>  

>  /**

>    Returns a boolean indicating if the firmware configuration interface

> @@ -183,7 +165,7 @@ QemuFwCfgInitialize (

>  

>          QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);

>          Features = QemuFwCfgRead32 ();

> -        if ((Features & BIT1) != 0) {

> +        if ((Features & FW_CFG_F_DMA) != 0) {

>            mFwCfgDmaAddress = FwCfgDmaAddress;

>            InternalQemuFwCfgReadBytes = DmaReadBytes;

>          }

> -- 

> 2.9.2

> 

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Dec. 2, 2016, 11:35 a.m. UTC | #2 
On 12/02/16 12:03, Leif Lindholm wrote:
> On Thu, Dec 01, 2016 at 06:56:32PM +0100, Laszlo Ersek wrote:

>> where "QemuFwCfgDma.h" was added in the previous patch.

>>

>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

> 

> This is nice cleanup.

> One bit of bikeshedding below, address or ignore - regardless:

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> 

>> ---

>>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-----------------

>>  1 file changed, 3 insertions(+), 21 deletions(-)

>>

>> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c

>> index 2fd8d9050566..62a85dff328e 100644

>> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c

>> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c

>> @@ -25,6 +25,8 @@

>>  

>>  #include <Protocol/FdtClient.h>

>>  

>> +#include <IndustryStandard/QemuFwCfgDma.h>

>> +

> 

> So, I forget if we have official guidelines on this, but instinctively

> I would put IndustryStandard before Library (alphabetically).


Good point. But, in the next version, this #include directive will go
away anyway, because this file already includes
<Library/QemuFwCfgLib.h>, and that header file will get the new macros
in v2 (based on Jordan's feedback).

Thanks!
Laszlo

> 

> Regards,

> 

> Leif

> 

>>  STATIC UINTN mFwCfgSelectorAddress;

>>  STATIC UINTN mFwCfgDataAddress;

>>  STATIC UINTN mFwCfgDmaAddress;

>> @@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;

>>  //

>>  STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;

>>  

>> -//

>> -// Communication structure for DmaReadBytes(). All fields are encoded in big

>> -// endian.

>> -//

>> -#pragma pack (1)

>> -typedef struct {

>> -  UINT32 Control;

>> -  UINT32 Length;

>> -  UINT64 Address;

>> -} FW_CFG_DMA_ACCESS;

>> -#pragma pack ()

>> -

>> -//

>> -// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).

>> -//

>> -#define FW_CFG_DMA_CTL_ERROR  BIT0

>> -#define FW_CFG_DMA_CTL_READ   BIT1

>> -#define FW_CFG_DMA_CTL_SKIP   BIT2

>> -#define FW_CFG_DMA_CTL_SELECT BIT3

>> -

>>  

>>  /**

>>    Returns a boolean indicating if the firmware configuration interface

>> @@ -183,7 +165,7 @@ QemuFwCfgInitialize (

>>  

>>          QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);

>>          Features = QemuFwCfgRead32 ();

>> -        if ((Features & BIT1) != 0) {

>> +        if ((Features & FW_CFG_F_DMA) != 0) {

>>            mFwCfgDmaAddress = FwCfgDmaAddress;

>>            InternalQemuFwCfgReadBytes = DmaReadBytes;

>>          }

>> -- 

>> 2.9.2

>>

>>

>> _______________________________________________

>> edk2-devel mailing list

>> edk2-devel@lists.01.org

>> https://lists.01.org/mailman/listinfo/edk2-devel

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 2fd8d9050566..62a85dff328e 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -25,6 +25,8 @@ 
 
 #include <Protocol/FdtClient.h>
 
+#include <IndustryStandard/QemuFwCfgDma.h>
+
 STATIC UINTN mFwCfgSelectorAddress;
 STATIC UINTN mFwCfgDataAddress;
 STATIC UINTN mFwCfgDmaAddress;
@@ -53,26 +55,6 @@  STATIC READ_BYTES_FUNCTION DmaReadBytes;
 //
 STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
 
-//
-// Communication structure for DmaReadBytes(). All fields are encoded in big
-// endian.
-//
-#pragma pack (1)
-typedef struct {
-  UINT32 Control;
-  UINT32 Length;
-  UINT64 Address;
-} FW_CFG_DMA_ACCESS;
-#pragma pack ()
-
-//
-// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
-//
-#define FW_CFG_DMA_CTL_ERROR  BIT0
-#define FW_CFG_DMA_CTL_READ   BIT1
-#define FW_CFG_DMA_CTL_SKIP   BIT2
-#define FW_CFG_DMA_CTL_SELECT BIT3
-
 
 /**
   Returns a boolean indicating if the firmware configuration interface
@@ -183,7 +165,7 @@  QemuFwCfgInitialize (
 
         QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
         Features = QemuFwCfgRead32 ();
-        if ((Features & BIT1) != 0) {
+        if ((Features & FW_CFG_F_DMA) != 0) {
           mFwCfgDmaAddress = FwCfgDmaAddress;
           InternalQemuFwCfgReadBytes = DmaReadBytes;
         }