Re: [PATCH] ArmPlatformPkg: Support different reg offset to PL011

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 3 July 2017 at 03:48, Jun Nie <jun.nie@linaro.org> wrote:
> ZTE SoC has different offset for some registers and bits.
> Add a macro flag to undef/redef those value. The macro
> flag can be enabled in BuildOptions section of platform.dsc.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>

Please use a single UINT8 type fixed PCD to configure whether your
alternative register mapping should be used. You can test its value
using preprocessor conditionals, e.g.,

#if !FixedPcdget8 (xxxx)
#else
#endif

> ---
>  ArmPlatformPkg/Include/Drivers/PL011Uart.h | 40 ++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/ArmPlatformPkg/Include/Drivers/PL011Uart.h b/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> index d5e88e8..87fab60 100644
> --- a/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> +++ b/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> @@ -40,6 +40,34 @@
>  #define UARTPID2                  0xFE8
>  #define UARTPID3                  0xFEC
>
> +#ifdef ZX_PL011_FLAG
> +#undef UARTDR
> +#undef UARTFR
> +#undef UARTIBRD
> +#undef UARTFBRD
> +#undef UARTLCR_H
> +#undef UARTCR
> +#undef UARTIFLS
> +#undef UARTIMSC
> +#undef UARTRIS
> +#undef UARTMIS
> +#undef UARTICR
> +#undef UARTDMACR
> +

Please don't use #undef and redefine. Create two equivalent blocks
instead, one with the default values and one with the alternative
values.

> +#define UARTDR                    0x004
> +#define UARTFR                    0x014
> +#define UARTIBRD                  0x024
> +#define UARTFBRD                  0x028
> +#define UARTLCR_H                 0x030
> +#define UARTCR                    0x034
> +#define UARTIFLS                  0x038
> +#define UARTIMSC                  0x040
> +#define UARTRIS                   0x044
> +#define UARTMIS                   0x048
> +#define UARTICR                   0x04c
> +#define UARTDMACR                 0x050
> +#endif
> +
>  // Data status bits
>  #define UART_DATA_ERROR_MASK      0x0F00
>
> @@ -57,6 +85,18 @@
>  #define PL011_UARTFR_DSR          (1 << 1)  // Data set ready
>  #define PL011_UARTFR_CTS          (1 << 0)  // Clear to send
>
> +#ifdef ZX_PL011_FLAG
> +#undef PL011_UARTFR_RI
> +#undef PL011_UARTFR_BUSY
> +#undef PL011_UARTFR_DSR
> +#undef PL011_UARTFR_CTS
> +
> +#define PL011_UARTFR_RI           0x001  // Ring indicator
> +#define PL011_UARTFR_BUSY         0x100 // UART busy
> +#define PL011_UARTFR_DSR          0x008  // Data set ready
> +#define PL011_UARTFR_CTS          0x002  // Clear to send

Please use the same style to define the constants (1 << n)