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

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

Oh, and please resubmit only this patch.
The rest are good to go, but I'll bring them in together.

/
    Leif

On Thu, May 31, 2018 at 10:01:51PM +0100, Leif Lindholm wrote:
> 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