Hi Lee,

I like the idea of setting the BAUD rate in one place.

Do we really need the extra defines for GEN1_BAUD_RATE and GEN2_BAUD_RATE?  
With BAUD_RATE define, I can set the GALILEO board type and the BAUD_RATE from 
the command line using /D flags for just those two if I want to override.  I 
just don't see a use case to use /D with these 2 extra define names.

Maybe just change to the following and remove the extra defines.  Can can add 
comments to explain why those baud rates are selected in this same set of lines 
in define section.

> +!if $(GALILEO) == GEN1
> +  BAUD_RATE = 460800
> +!endif
> +!if $(GALILEO) == GEN2
> +  BAUD_RATE = 921600
> +!endif

Thanks,

Mike

> -----Original Message-----
> From: Leahy, Leroy P
> Sent: Friday, March 4, 2016 9:03 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; Steele, Kelly
> <kelly.ste...@intel.com>; edk2-devel@lists.01.org; Bjorge, Erik C
> <erik.c.bjo...@intel.com>
> Cc: Leahy, Leroy P <leroy.p.le...@intel.com>
> Subject: [PATCH 8/9] QuarkPkg: Move baud rate setting to top of .dsc file
> 
> Move the baud rate setting to the top of the .dsc file.  Use a single
> setting for each board.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Lee Leahy <leroy.p.le...@intel.com>
> ---
>  QuarkPlatformPkg/Quark.dsc | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/QuarkPlatformPkg/Quark.dsc b/QuarkPlatformPkg/Quark.dsc
> index c87bb17..c8952d8 100644
> --- a/QuarkPlatformPkg/Quark.dsc
> +++ b/QuarkPlatformPkg/Quark.dsc
> @@ -46,6 +46,19 @@
>    DEFINE GALILEO              = GEN2
> 
>    #
> +  # Specify the maximum baud rate for the board
> +  #
> +  DEFINE GEN1_BAUD_RATE       = 460800
> +  DEFINE GEN2_BAUD_RATE       = 921600
> +
> +!if $(GALILEO) == GEN1
> +  BAUD_RATE = $(GEN1_BAUD_RATE)
> +!endif
> +!if $(GALILEO) == GEN2
> +  BAUD_RATE = $(GEN2_BAUD_RATE)
> +!endif> +
> +  #
>    # TPM 1.2 Hardware.  Options are [NONE, LPC, ATMEL_I2C, INFINEON_I2C]
>    #
>    DEFINE TPM_12_HARDWARE      = NONE
> @@ -342,12 +355,7 @@
>  !endif
>    gEfiMdePkgTokenSpaceGuid.PcdPostCodePropertyMask|0x18
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
> -!if $(GALILEO) == GEN1
> -  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|460800
> -!endif
> -!if $(GALILEO) == GEN2
> -  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|921600
> -!endif
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|$(BAUD_RATE)
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits|8
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity|1
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits|1
> @@ -382,14 +390,8 @@
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0x9000B000
> -!if $(GALILEO) == GEN1
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate|460800
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate|$(BAUD_RATE)
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl|FALSE
> -!endif
> -!if $(GALILEO) == GEN2
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate|921600
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl|FALSE
> -!endif
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl|0x03
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x07
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable|FALSE
> --
> 1.9.1

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

Reply via email to