Re: [PATCH edk2-platforms v2 6/7] Platform/NinetySixBoards: add core driver for LS connector and config

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

On 22 February 2018 at 15:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Feb 20, 2018 at 05:49:43PM +0000, Ard Biesheuvel wrote:
>> This adds a driver that manages the 96boards LS connector, i.e, it
>> installs a HII page to configure the type of mezzanine that is installed
>> in the slot, and it exposes this information via the LS connector protocol.
>> It is also in charge of applying the overlay to the platform device tree
>> at end of DXE.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Platform/NinetySixBoards/Include/Guid/FormSet.h                    |  23 ++
>>  Platform/NinetySixBoards/NinetySixBoards.dec                       |   3 +
>>  Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.c   | 221 ++++++++++++++++++++
>>  Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.h   |  32 +++
>>  Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.inf |  57 +++++
>>  Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsHii.uni |  27 +++
>>  Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsHii.vfr |  51 +++++
>>  7 files changed, 414 insertions(+)
>>
>> diff --git a/Platform/NinetySixBoards/Include/Guid/FormSet.h b/Platform/NinetySixBoards/Include/Guid/FormSet.h
>> new file mode 100644
>> index 000000000000..db16657f0848
>> --- /dev/null
>> +++ b/Platform/NinetySixBoards/Include/Guid/FormSet.h
>> @@ -0,0 +1,23 @@
>> +/** @file
>> +
>> +  Copyright (c) 2018, Linaro Limited. All rights reserved.
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __NINETY_SIX_BOARDS_FORMSET_H__
>> +#define __NINETY_SIX_BOARDS_FORMSET_H__
>> +
>> +#define NINETY_SIX_BOARDS_FORMSET_GUID  \
>> +  { 0x7500c9d2, 0x9203, 0x4a37, { 0x84, 0xbb, 0x92, 0xa9, 0xce, 0x34, 0x38, 0xbd } }
>> +
>> +extern EFI_GUID gNinetySixBoardsFormsetGuid;
>> +
>> +#endif // __NINETY_SIX_BOARDS_FORMSET_H__
>> diff --git a/Platform/NinetySixBoards/NinetySixBoards.dec b/Platform/NinetySixBoards/NinetySixBoards.dec
>> index 5c3fe43dbb24..e7d1705d47ff 100644
>> --- a/Platform/NinetySixBoards/NinetySixBoards.dec
>> +++ b/Platform/NinetySixBoards/NinetySixBoards.dec
>> @@ -37,6 +37,9 @@ [Guids]
>>    gNinetySixBoardsI2c1MasterGuid = { 0xcf64ac46, 0xd0be, 0x4a69, { 0x90, 0xa2, 0xf2, 0x82, 0x5b, 0x92, 0x25, 0x61 } }
>>    gNinetySixBoardsSpiMasterGuid = { 0x9703fd99, 0xe638, 0x42b8, { 0xab, 0x81, 0x52, 0x61, 0x1b, 0xf7, 0xf7, 0x5d } }
>>
>> +  # GUID for the HII configuration form
>> +  gNinetySixBoardsFormsetGuid = { 0x7500c9d2, 0x9203, 0x4a37, { 0x84, 0xbb, 0x92, 0xa9, 0xce, 0x34, 0x38, 0xbd } }
>> +
>>  [PcdsFixedAtBuild]
>>    # ASCII DT paths to the I2C parent nodes of the 96boards LS connector
>>    gNinetySixBoardsTokenSpaceGuid.PcdI2c0Parent|""|VOID*|0x00000001
...
>> diff --git a/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.h b/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.h
>> new file mode 100644
>> index 000000000000..e94a115a5929
>> --- /dev/null
>> +++ b/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.h
>> @@ -0,0 +1,32 @@
>> +/** @file
>> +
>> +  Copyright (c) 2018, Linaro Limited. All rights reserved.
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __NINETY_SIX_BOARDS_DXE_H__
>> +#define __NINETY_SIX_BOARDS_DXE_H__
>> +
>> +#include <Guid/HiiPlatformSetupFormset.h>
>> +#include <Guid/FormSet.h>
>> +
>> +#define MEZZANINE_NONE                            0x0
>> +#define MEZZANINE_SECURE96                        0x1
>> +#define MEZZANINE_MAX                             0x1
>
> Would this be simpler as an enum with a permanent final element called
> MaxMezzanineType? This seems like the common EDK2 (and even UEFI)
> pattern.
>

Does C guarantee a certain size for enums? Because the struct below is
projected onto the EFI variable directly.

>> +
>> +#define NINETY_SIX_BOARDS_CONFIG_VARIABLE_NAME    L"NinetySixBoardsConfig"
>> +
>> +typedef struct {
>> +  UINT8         MezzanineType;
>> +  UINT8         Reserved[7];
>> +} NINETY_SIX_BOARDS_CONFIG_DATA;
>> +
>> +#endif // __NINETY_SIX_BOARDS_DXE_H__
>> diff --git a/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.inf b/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.inf
>> new file mode 100644
>> index 000000000000..5c6f863e8db4
>> --- /dev/null
>> +++ b/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsDxe.inf
>> @@ -0,0 +1,57 @@
>> +## @file
>> +#
>> +#  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
>> +#
>> +#  This program and the accompanying materials are licensed and made
>> +#  available under the terms and conditions of the BSD License which
>> +#  accompanies this distribution.  The full text of the license may be
>> +#  found at http://opensource.org/licenses/bsd-license.php
>> +#
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> +#  IMPLIED.
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION               = 0x0001001A
>> +  BASE_NAME                 = NinetySixBoardsDxe
>> +  FILE_GUID                 = 3f68e889-cb77-4efc-bc84-afa0a64ad26e
>> +  MODULE_TYPE               = DXE_DRIVER
>> +  VERSION_STRING            = 1.0
>> +  ENTRY_POINT               = EntryPoint
>> +
>> +[Sources]
>> +  NinetySixBoardsDxe.c
>> +  NinetySixBoardsDxe.h
>> +  NinetySixBoardsHii.vfr
>> +  NinetySixBoardsHii.uni
>> +
>> +[Packages]
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  Platform/NinetySixBoards/NinetySixBoards.dec
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +  BaseMemoryLib
>> +  DebugLib
>> +  HiiLib
>> +  UefiBootServicesTableLib
>> +  UefiDriverEntryPoint
>> +  UefiLib
>> +  UefiRuntimeServicesTableLib
>> +
>> +[Protocols]
>> +  gNinetySixBoardsLsConnectorProtocolGuid     ## PRODUCES
>> +  gNinetySixBoardsMezzanineProtocolGuid       ## CONSUMES
>> +
>> +[Guids]
>> +  gEfiEndOfDxeEventGroupGuid
>> +  gFdtTableGuid
>> +  gNinetySixBoardsFormsetGuid
>> +
>> +[Depex]
>> +  gEfiVariableArchProtocolGuid        AND
>> +  gEfiVariableWriteArchProtocolGuid
>> diff --git a/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsHii.uni b/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsHii.uni
>> new file mode 100644
>> index 000000000000..23f4c5859207
>> --- /dev/null
>> +++ b/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsHii.uni
>> @@ -0,0 +1,27 @@
>> +/** @file
>> +
>> +  Copyright (c) 2018, Linaro, Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#langdef en-US  "English"
>> +
>> +#string STR_FORM_SET_TITLE             #language en-US "96boards Mezzanine options"
>> +#string STR_FORM_SET_TITLE_HELP        #language en-US "Configure the installed 96boards mezzanine"
>> +
>> +#string STR_MAIN_FORM_TITLE            #language en-US "96boards Mezzanine options"
>> +#string STR_NULL_STRING                #language en-US ""
>> +
>> +#string STR_MEZZANINE_SELECT_PROMPT    #language en-US "96boards mezzanine"
>> +#string STR_MEZZANINE_SELECT_HELP      #language en-US "The type of mezzanine board plugged into the 96boards LS connector"
>> +
>> +#string STR_MEZZANINE_NONE             #language en-US "None/Unknown"
>> +#string STR_MEZZANINE_SECURE96         #language en-US "Secure96"
>> diff --git a/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsHii.vfr b/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsHii.vfr
>> new file mode 100644
>> index 000000000000..7e78c9cfa22f
>> --- /dev/null
>> +++ b/Platform/NinetySixBoards/NinetySixBoardsDxe/NinetySixBoardsHii.vfr
>> @@ -0,0 +1,51 @@
>> +/** @file
>> +
>> +  Copyright (c) 2018, Linaro, Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include "NinetySixBoardsDxe.h"
>> +
>> +//
>> +// EFI Variable attributes
>> +//
>> +#define EFI_VARIABLE_NON_VOLATILE       0x00000001
>> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
>> +#define EFI_VARIABLE_RUNTIME_ACCESS     0x00000004
>> +#define EFI_VARIABLE_READ_ONLY          0x00000008
>
> Hmm, that jogged my memory - I need to follow up on
> https://www.mail-archive.com/edk2-devel@lists.01.org/msg34156.html
>
> ... and I would appreciate an R-b on
> https://www.mail-archive.com/edk2-devel@lists.01.org/msg34154.html
>
> :)
>
> I would prefer if we could get this one in with a
> #include <Uefi/UefiMultiPhase.h>
> instead.
>

OK, but please chase this with the other maintainers then.