Re: [platforms PATCH 04/25] Marvell/Drivers: MvBoardDescDxe: Introduce board description driver

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

On Fri, Jun 08, 2018 at 05:34:02PM +0200, Marcin Wojtas wrote:
> From: jinghua <jinghua@marvell.com>
> 
> This patch introduces a producer of MARVELL_BOARD_DESC_PROTOCOL, which
> gets SoC description from ArmadaSoCDescLib, then based on dsc file,
> provide only enabled hardware module controllers for the consumers,
> which are typically controllers' drivers. Thanks to that
> there is a separation between obtaining the platform description and
> the drivers. A first example of the board description callback
> is information about UTMI controllers and type.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: jinghua <jinghua@marvell.com>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c   | 174 ++++++++++++++++++++
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h   |  59 +++++++
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf |  65 ++++++++
>  3 files changed, 298 insertions(+)
> 
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> new file mode 100644
> index 0000000..c220e58
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> @@ -0,0 +1,174 @@
> +/*******************************************************************************
> +Copyright (C) 2018 Marvell International Ltd.
> +
> +Marvell BSD License Option
> +
> +If you received this File from Marvell, you may opt to use, redistribute and/or
> +modify this File under the following licensing terms.
> +Redistribution and use in source and binary forms, with or without modification,
> +are permitted provided that the following conditions are met:
> +
> +* Redistributions of source code must retain the above copyright notice,
> +  this list of conditions and the following disclaimer.
> +
> +* Redistributions in binary form must reproduce the above copyright
> +  notice, this list of conditions and the following disclaimer in the
> +  documentation and/or other materials provided with the distribution.
> +
> +* Neither the name of Marvell nor the names of its contributors may be
> +  used to endorse or promote products derived from this software without
> +  specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +*******************************************************************************/
> +#include "MvBoardDescDxe.h"
> +
> +MV_BOARD_DESC *mBoardDescInstance;
> +
> +STATIC
> +EFI_STATUS
> +MvBoardDescUtmiGet (
> +  IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> +  IN OUT MV_BOARD_UTMI_DESC      **UtmiDesc
> +  )
> +{
> +  UINT8 *UtmiDeviceTable, *XhciDeviceTable, *UtmiPortType, UtmiCount;

Neither of these pointers sound like they're pointing to
character/byte strings - why are they UINT8*?
(My guess is that these should all be VOID*.)
UtmiCount should preferably be UINTN.

> +  UINTN UtmiDeviceTableSize, UtmiIndex, Index;
> +  MV_BOARD_UTMI_DESC *BoardDesc;
> +  MV_SOC_UTMI_DESC *SoCDesc;
> +  EFI_STATUS Status;
> +
> +  /* Get SoC data about all available UTMI controllers */
> +  Status = ArmadaSoCDescUtmiGet (&SoCDesc, &UtmiCount);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  /* Obtain table with enabled Utmi PHY's */
> +  UtmiDeviceTable = (UINT8 *)PcdGetPtr (PcdUtmiControllersEnabled);

Will this (and subsequent casts) be required if the pointers are VOID*?

> +  if (UtmiDeviceTable == NULL) {
> +    /* No UTMI PHY on platform */
> +    return EFI_SUCCESS;
> +  }
> +
> +  /* Make sure XHCI controllers table is present */
> +  XhciDeviceTable = (UINT8 *)PcdGetPtr (PcdPciEXhci);
> +  if (XhciDeviceTable == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Missing PcdPciEXhci\n", __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  UtmiDeviceTableSize = PcdGetSize (PcdUtmiControllersEnabled);
> +
> +  /* Check if PCD with UTMI PHYs is correctly defined */
> +  if (UtmiDeviceTableSize > UtmiCount ||
> +      UtmiDeviceTableSize > PcdGetSize (PcdPciEXhci)) {

Please add some parentheses.

> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Wrong PcdUtmiControllersEnabled format\n",
> +      __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  /* Obtain port type table */
> +  UtmiPortType = (UINT8 *)PcdGetPtr (PcdUtmiPortType);
> +  if (UtmiPortType == NULL ||
> +      PcdGetSize (PcdUtmiPortType) != UtmiDeviceTableSize) {

Please add some parentheses.

> +    DEBUG ((DEBUG_ERROR, "%a: Wrong PcdUtmiPortType format\n", __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  /* Allocate and fill board description */
> +  BoardDesc = AllocateZeroPool (UtmiDeviceTableSize * sizeof (MV_BOARD_UTMI_DESC));
> +  if (BoardDesc == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  UtmiIndex = 0;
> +  for (Index = 0; Index < UtmiDeviceTableSize; Index++) {
> +    if (!MVHW_DEV_ENABLED (Utmi, Index)) {

    if (UtmiDeviceTable[Index] != MVHW_DEV_ENABLED)
would read a lot better.

> +      continue;
> +    }
> +
> +    /* UTMI PHY without enabled XHCI controller is useless */
> +    if (!MVHW_DEV_ENABLED (Xhci, Index)) {

Similarly,
  if (XhciDeviceTable[Index] != MVHW_DEV_ENABLED)

> +      DEBUG ((DEBUG_ERROR,
> +             "%a: Disabled Xhci controller %d\n",
> +             Index,
> +             __FUNCTION__));
> +      return EFI_INVALID_PARAMETER;
> +    }
> +
> +    BoardDesc[UtmiIndex].SoC = &SoCDesc[Index];
> +    BoardDesc[UtmiIndex].UtmiPortType = UtmiPortType[Index];
> +    UtmiIndex++;
> +  }
> +
> +  BoardDesc->UtmiDevCount = UtmiIndex;
> +
> +  *UtmiDesc = BoardDesc;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +VOID
> +MvBoardDescFree (
> +  IN VOID *BoardDesc
> +  )
> +{
> +  if (BoardDesc != NULL) {

Do we really want this test?
What it ends up doing is getting rid of an assert if the function is
called with a NULL pointer. And if so, why did we call it?

> +    FreePool (BoardDesc);
> +  }
> +}
> +
> +STATIC
> +EFI_STATUS
> +MvBoardDescInitProtocol (
> +  IN MARVELL_BOARD_DESC_PROTOCOL *BoardDescProtocol
> +  )
> +{
> +  BoardDescProtocol->BoardDescUtmiGet = MvBoardDescUtmiGet;
> +  BoardDescProtocol->BoardDescFree = MvBoardDescFree;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +MvBoardDescEntryPoint (
> +  IN EFI_HANDLE       ImageHandle,
> +  IN EFI_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  mBoardDescInstance = AllocateZeroPool (sizeof (MV_BOARD_DESC));
> +  if (mBoardDescInstance == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  MvBoardDescInitProtocol (&mBoardDescInstance->BoardDescProtocol);
> +
> +  mBoardDescInstance->Signature = BOARD_DESC_SIGNATURE;
> +
> +  Status = gBS->InstallMultipleProtocolInterfaces (&(mBoardDescInstance->Handle),
> +                  &gMarvellBoardDescProtocolGuid,
> +                  &(mBoardDescInstance->BoardDescProtocol));
> +  if (EFI_ERROR (Status)) {
> +    FreePool (mBoardDescInstance);
> +    return Status;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h
> new file mode 100644
> index 0000000..47d9a72
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h
> @@ -0,0 +1,59 @@
> +/*******************************************************************************
> +Copyright (C) 2018 Marvell International Ltd.
> +Marvell BSD License Option
> +
> +If you received this File from Marvell, you may opt to use, redistribute and/or
> +modify this File under the following licensing terms.
> +Redistribution and use in source and binary forms, with or without modification,
> +are permitted provided that the following conditions are met:
> +
> +* Redistributions of source code must retain the above copyright notice,
> +  this list of conditions and the following disclaimer.
> +
> +* Redistributions in binary form must reproduce the above copyright
> +  notice, this list of conditions and the following disclaimer in the
> +  documentation and/or other materials provided with the distribution.
> +
> +* Neither the name of Marvell nor the names of its contributors may be
> +  used to endorse or promote products derived from this software without
> +  specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +*******************************************************************************/
> +#ifndef __MV_BOARD_DESC_H__
> +#define __MV_BOARD_DESC_H__
> +
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +
> +#include <Protocol/BoardDesc.h>
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +#define BOARD_DESC_SIGNATURE                   SIGNATURE_64 ('B', 'O', 'A', 'R', 'D', 'D', 'S', 'C')

Very generic sig for something that is still very specific.
I'd be happier of the first two letters were 'M' and 'V'.

> +
> +typedef struct {
> +  MARVELL_BOARD_DESC_PROTOCOL   BoardDescProtocol;
> +  UINTN                   Signature;
> +  EFI_HANDLE              Handle;
> +  EFI_LOCK                Lock;
> +} MV_BOARD_DESC;
> +
> +#define MVHW_DEV_ENABLED(type, index) (type ## DeviceTable[index])

This macro obscures what's going on. Please get rid of it as per
earlier comments (added after I got here).

> +
> +#endif // __MV_BOARD_DESC_H__
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> new file mode 100644
> index 0000000..9367833
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> @@ -0,0 +1,65 @@
> +#
> +# Marvell BSD License Option
> +#
> +# If you received this File from Marvell, you may opt to use, redistribute
> +# and/or modify this File under the following licensing terms.
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are met:
> +#
> +# * Redistributions of source code must retain the above copyright notice,
> +# this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Marvell nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE
> +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
> +# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
> +# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010019

...001A (just apply throughout before reposting).

> +  BASE_NAME                      = BoardDescDxe
> +  FILE_GUID                      = 4ed385f9-5d2c-4774-95c5-d5d9d70b3c37
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = MvBoardDescEntryPoint
> +
> +[Sources]
> +  MvBoardDescDxe.c
> +  MvBoardDescDxe.h
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  ArmadaSoCDescLib
> +  DebugLib
> +  MemoryAllocationLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +
> +[Protocols]
> +  gMarvellBoardDescProtocolGuid
> +
> +[Pcd]
> +  gMarvellTokenSpaceGuid.PcdUtmiControllersEnabled
> +  gMarvellTokenSpaceGuid.PcdUtmiPortType
> +  gMarvellTokenSpaceGuid.PcdPciEXhci

PcdP before PcdU.

/
    Leif

> +
> +[Depex]
> +  TRUE
> -- 
> 2.7.4
>