Re: [PATCH v7 edk-platforms 2/6] Platform/HiKey960: do basic initialization

[Leif Lindholm <leif.lindholm@linaro.org>]

This fixes the headers, but ignores the most important bit of feedback
on v6:

On Thu, May 31, 2018 at 11:10:06AM +0800, Haojian Zhuang wrote:
> Do some basic initliazation on peripherals, such as pins and
> regulators.
> 
> The hardcoding code is taken from non-open reference code.
> Can't fix it for lack of documents.
> 
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---

> diff --git a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
> new file mode 100644
> index 000000000000..e88bad2167c6
> --- /dev/null
> +++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
...
> +/**
> +  Notification function of the event defined as belonging to the
> +  EFI_END_OF_DXE_EVENT_GROUP_GUID event group that was created in
> +  the entry point of the driver.
> +
> +  This function is called when an event belonging to the
> +  EFI_END_OF_DXE_EVENT_GROUP_GUID event group is signalled. Such an
> +  event is signalled once at the end of the dispatching of all
> +  drivers (end of the so called DXE phase).
> +
> +  @param[in]  Event    Event declared in the entry point of the driver whose
> +                       notification function is being invoked.
> +  @param[in]  Context  NULL
> +**/
> +STATIC
> +VOID
> +OnEndOfDxe (
> +  IN EFI_EVENT  Event,
> +  IN VOID       *Context
> +  )
> +{
> +  UINT32        BootMode;
> +  CHAR8         Buf[64];
> +  UINTN         Count;
> +
> +  BootMode = MmioRead32 (SCTRL_BAK_DATA0) & BOOT_MODE_MASK;
> +  if (BootMode == BOOT_MODE_RECOVERY) {
> +    Count = AsciiSPrint (
> +              Buf,
> +              64,
> +              "WARNING: CAN NOT BOOT KERNEL IN RECOVERY MODE!\n"
> +              );
> +    SerialPortWrite ((UINT8 *)Buf, Count);
> +    Count = AsciiSPrint (
> +              Buf,
> +              64,
> +              "Switch to normal boot mode, then reboot to boot kernel.\n"
> +              );

I gave the following feedback on v6, which has not been addressed at all:
---
I asked for the hand-coded values to be removed, and instead they have
been replaced by a static buffer which may or may not be able to hold
updated versions of the stringsm and require changes in three places
if that happens. This is not an improvement.

The length or size of a compile time constant should always be
described through StrLen/AsciiStrLen, or where possible sizeof.

In this instance, I would suggest doing something like:

  VOID   *RecoveryString;
    VOID   *SwitchString;

  RecoveryString = "WARNING: CAN NOT BOOT KERNEL IN RECOVERY MODE!\n"
    SwitchString = "Switch to normal boot mode, then reboot to boot
    kernel.\n";

You can then use SerialPortWrite (RecoveryString,
AsciiStrLen(*RecoveryString)).

I think we should also in future look into how we can get the console
working before Dxe. Both HiKey boards are generally intended to be
used with displays, so these messages will be invisible to many users.
---

> +    SerialPortWrite ((UINT8 *)Buf, Count);
> +  }
> +}

/
    Leif