Re: [platforms: PATCH v2 01/25] Marvell/Library: Introduce ArmadaSoCDescLib class

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

On Mon, Jun 18, 2018 at 05:58:42PM +0200, Marcin Wojtas wrote:
> Hi Leif,
> 
> 2018-06-18 17:44 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Sun, Jun 17, 2018 at 10:11:41PM +0200, Marcin Wojtas wrote:
> >> From: jinghua <jinghua@marvell.com>
> >>
> >> ArmadaSoCDescLib is a per SoC family library, which provides SoC
> >> description, like register base of some hardware module controller,
> >> COMPHY/I2C/NETWORK etc., which right now is hardcoded in MvHwDescLib.h.
> >> There will be a new protocol, which gets SoC description from this
> >> library, and provides board description based on enable/disable
> >> values of each hardware module controller in dsc file.
> >>
> >> As a first example implement obtaining UTMI controllers information.
> >> Remaining interfaces will be added in follow-up commits.
> >> This patch introduces new library callback (ArmadaSoCDescUtmiGet ()),
> >> which dynamically allocates and fills MV_SOC_UTMI_DESC structure,
> >> SoC description of UTMI PHYs. A new PCD is introduced (PcdMaxCpCount)
> >> which stores maximal amount of CP110 blocks in the SoC family.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: jinghua <jinghua@marvell.com>
> >
> > Several of these pre-existing Signed-off-by remain in this set. I did
> > only comment it on 2/25, but it remains there as well.
> 
> As I understood, the problem was 'Reviewed-by' from internal review -
> all those were dropped. So far when the commit was authored by someone
> else (at least in OpenPlatformPkg times) we used to leave the
> authorship and authors 'Signed-off-by' - in such case I had to add my
> own Signed-off-by, as I was sending this to the lists.

We may have been poor at communicating (and if so I apologise) and
potentially on spotting this before,  but we did mean both. You are
the one contributing this to the project, so we need your sign-off
that the code is actually under the license you say it is and that you
are permitted to contribute it.

Including Jinghua's Signed-off-by on initial submission is effectively
you saying "Jinghua says this is fine to distribute under this
license", which is nice and all but untraceable and hence of no legal
value.

Jinghua's effort get recognised by the Author tag because you've kept
them in the From: field.

Now, if you and Jinghua were playing tag team and sending different
sets that got merged together, then sure. We treat Signed-off-by
(nearly) exactly like Linux. We may not be as stringent on explicitly
mentioning ourselves as signing off on changes we may make on pushing.

I see a few patches in the tree where you and Ard have been working
together in public, and then the multiple signed-off-by make sense.

You will also note that the bulk import I did to edk2-platforms does
not come with any other Signed-off-by than my own. It does come with a
commit message pointing to where I got everything from.

Best Regards,

Leif

> > Please send a v3 with this addressed across all patches.
> > But please wait until I've had a chance to go through this set.
> > When you do so, this one has
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> > I will comment explicitly on all such patches in this set, apart from
> > the one where I already gave Reviewed-by (but please delete all other
> > signed-off-bys than your own from those as well).
> 
> I will update  according to your wish and the new policy.
>
> Best regards,
> Marcin
> 
> > /
> >     Leif
> >
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >>  Silicon/Marvell/Marvell.dec                                                      |  4 ++
> >>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.inf | 37 +++++++++++
> >>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h   | 35 +++++++++++
> >>  Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                               | 33 ++++++++++
> >>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c   | 65 ++++++++++++++++++++
> >>  5 files changed, 174 insertions(+)
> >>  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.inf
> >>  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> >>  create mode 100644 Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> >>  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> >>
> >> diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
> >> index be74b4e..2a92eff 100644
> >> --- a/Silicon/Marvell/Marvell.dec
> >> +++ b/Silicon/Marvell/Marvell.dec
> >> @@ -60,6 +60,7 @@
> >>    gMarvellSpiFlashDxeGuid = { 0x49d7fb74, 0x306d, 0x42bd, { 0x94, 0xc8, 0xc0, 0xc5, 0x4b, 0x18, 0x1d, 0xd7 } }
> >>
> >>  [LibraryClasses]
> >> +  ArmadaSoCDescLib|Include/Library/ArmadaSoCDescLib.h
> >>    SampleAtResetLib|Include/Library/SampleAtResetLib.h
> >>
> >>  [Protocols]
> >> @@ -68,6 +69,9 @@
> >>    gMarvellPlatformInitCompleteProtocolGuid = { 0x465b8cf7, 0x016f, 0x4ba6, { 0xbe, 0x6b, 0x28, 0x0e, 0x3a, 0x7d, 0x38, 0x6f } }
> >>
> >>  [PcdsFixedAtBuild.common]
> >> +#Board description
> >> +  gMarvellTokenSpaceGuid.PcdMaxCpCount|0x2|UINT8|0x30000072
> >> +
> >>  #MPP
> >>    gMarvellTokenSpaceGuid.PcdMppChipCount|0|UINT32|0x30000001
> >>
> >> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.inf b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.inf
> >> new file mode 100644
> >> index 0000000..2b73b73
> >> --- /dev/null
> >> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.inf
> >> @@ -0,0 +1,37 @@
> >> +## @file
> >> +#
> >> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates<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                      = Armada7k8kDescLib
> >> +  FILE_GUID                      = c64f0048-4ca3-4573-b0a6-c2e9e6457285
> >> +  MODULE_TYPE                    = BASE
> >> +  VERSION_STRING                 = 1.0
> >> +  LIBRARY_CLASS                  = ArmadaSoCDescLib
> >> +
> >> +[Sources]
> >> +  Armada7k8kSoCDescLib.c
> >> +
> >> +[Packages]
> >> +  MdeModulePkg/MdeModulePkg.dec
> >> +  MdePkg/MdePkg.dec
> >> +  Silicon/Marvell/Marvell.dec
> >> +
> >> +[LibraryClasses]
> >> +  DebugLib
> >> +  IoLib
> >> +  PcdLib
> >> +
> >> +[FixedPcd]
> >> +  gMarvellTokenSpaceGuid.PcdMaxCpCount
> >> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> >> new file mode 100644
> >> index 0000000..c5711b0
> >> --- /dev/null
> >> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> >> @@ -0,0 +1,35 @@
> >> +/**
> >> +*
> >> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> >> +*
> >> +*  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.
> >> +*
> >> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> >> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> >> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> >> +**/
> >> +
> >> +#ifndef __ARMADA7K8K_SOC_DESC_LIB_H__
> >> +#define __ARMADA7K8K_SOC_DESC_LIB_H__
> >> +
> >> +//
> >> +// Common macros
> >> +//
> >> +#define MV_SOC_CP_BASE(Cp)               (0xF2000000 + ((Cp) * 0x2000000))
> >> +
> >> +//
> >> +// Platform description of UTMI PHY's
> >> +//
> >> +#define MV_SOC_UTMI_PER_CP_COUNT         2
> >> +#define MV_SOC_UTMI_ID(Utmi)             (Utmi)
> >> +#define MV_SOC_UTMI_BASE(Utmi)           (0x580000 + ((Utmi) * 0x1000))
> >> +#define MV_SOC_UTMI_CFG_BASE             0x440440
> >> +#define MV_SOC_UTMI_USB_CFG_BASE         0x440420
> >> +
> >> +#endif
> >> diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> >> new file mode 100644
> >> index 0000000..0d45684
> >> --- /dev/null
> >> +++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> >> @@ -0,0 +1,33 @@
> >> +/**
> >> +*
> >> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates
> >> +*
> >> +*  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 __ARMADA_SOC_DESC_LIB_H__
> >> +#define __ARMADA_SOC_DESC_LIB_H__
> >> +
> >> +//
> >> +// UTMI PHY devices SoC description
> >> +//
> >> +typedef struct {
> >> +  UINT8 UtmiPhyId;
> >> +  UINTN UtmiBaseAddress;
> >> +  UINTN UtmiConfigAddress;
> >> +  UINTN UsbConfigAddress;
> >> +} MV_SOC_UTMI_DESC;
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +ArmadaSoCDescUtmiGet (
> >> +  IN OUT MV_SOC_UTMI_DESC  **UtmiDesc,
> >> +  IN OUT UINTN              *DescCount
> >> +  );
> >> +#endif /* __ARMADA_SOC_DESC_LIB_H__ */
> >> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> >> new file mode 100644
> >> index 0000000..63fb224
> >> --- /dev/null
> >> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> >> @@ -0,0 +1,65 @@
> >> +/**
> >> +*
> >> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> >> +*
> >> +*  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.
> >> +*
> >> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> >> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> >> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> >> +**/
> >> +
> >> +#include <Uefi.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 <Protocol/BoardDesc.h>
> >> +
> >> +#include "Armada7k8kSoCDescLib.h"
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +ArmadaSoCDescUtmiGet (
> >> +  IN OUT MV_SOC_UTMI_DESC  **UtmiDesc,
> >> +  IN OUT UINTN              *DescCount
> >> +  )
> >> +{
> >> +  MV_SOC_UTMI_DESC *Desc;
> >> +  UINTN CpCount, CpIndex, Index, UtmiIndex;
> >> +
> >> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> >> +
> >> +  *DescCount = CpCount * MV_SOC_UTMI_PER_CP_COUNT;
> >> +  Desc = AllocateZeroPool (*DescCount * sizeof (MV_SOC_UTMI_DESC));
> >> +  if (Desc == NULL) {
> >> +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> >> +    return EFI_OUT_OF_RESOURCES;
> >> +  }
> >> +
> >> +  *UtmiDesc = Desc;
> >> +
> >> +  UtmiIndex = 0;
> >> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> >> +    for (Index = 0; Index < MV_SOC_UTMI_PER_CP_COUNT; Index++) {
> >> +      Desc->UtmiPhyId = MV_SOC_UTMI_ID (UtmiIndex);
> >> +      Desc->UtmiBaseAddress = MV_SOC_CP_BASE (CpIndex) + MV_SOC_UTMI_BASE (Index);
> >> +      Desc->UtmiConfigAddress = MV_SOC_CP_BASE (CpIndex) + MV_SOC_UTMI_CFG_BASE;
> >> +      Desc->UsbConfigAddress = MV_SOC_CP_BASE (CpIndex) + MV_SOC_UTMI_USB_CFG_BASE;
> >> +      Desc++;
> >> +      UtmiIndex++;
> >> +    }
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> --
> >> 2.7.4
> >>