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 <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Haojian Zhuang <[email protected]>
> > ---
>
> > 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
