Re: [PATCH edk2-platforms v2 13/23] Silicon/Socionext: add driver for SPI NOR flash

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

On Sat, Oct 28, 2017 at 03:16:50PM +0100, Ard Biesheuvel wrote:
> On 26 October 2017 at 22:19, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Wed, Oct 25, 2017 at 06:59:37PM +0100, Ard Biesheuvel wrote:
> >> From: Pipat Methavanitpong <methavanitpong.pipat@socionext.com>
> >>
> >> This imports the driver sources provided by Socionext for the FIP006
> >> SPI NOR flash device found on SynQuacer SoCs. It has been slightly
> >> tweaked to bring it up to date with the changes made on the EDK2 side
> >> since it was forked.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Pipat Methavanitpong <methavanitpong.pipat@socionext.com>
> >>
> >> [various tweaks and bugfixes]
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > Do we need Contributed-under twice?
> > I'm not sure that carries any legal significane anyway.
> >
> > Sorry, I would love to trim the below, but there are minor comments
> > spread throughout.
> >
> >> ---
> >>  Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/Fip006Dxe.dec        |   31 +
> >>  Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/Fip006Dxe.inf        |   79 ++
> >>  Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/Fip006Reg.h          |  244 ++++
> >>  Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashBlockIoDxe.c |  138 ++
> >>  Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c        | 1376 ++++++++++++++++++++
> >>  Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.h        |  314 +++++
> >>  Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashFvbDxe.c     |  859 ++++++++++++
> >>  7 files changed, 3041 insertions(+)
> >>

> >> diff --git a/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c b/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c
> >> new file mode 100644
> >> index 000000000000..d9cf11dd5be5
> >> --- /dev/null
> >> +++ b/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c
> >> @@ -0,0 +1,1376 @@
> >> +/** @file  NorFlashDxe.c
> >> +
> >> +  Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
> >> +  Copyright (c) 2017, Socionext Inc. All rights reserved.<BR>
> >> +  Copyright (c) 2017, 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.
> >> +
> >> +**/
> >> +
> >> +#include <Library/UefiLib.h>
> >> +#include <Library/BaseMemoryLib.h>
> >> +#include <Library/MemoryAllocationLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> +#include <Library/PcdLib.h>
> >
> > Move up one row?
> >
> >> +
> >> +#include "NorFlashDxe.h"
> >> +
> >> +STATIC EFI_EVENT mNorFlashVirtualAddrChangeEvent;
> >> +
> >> +//
> >> +// Global variable declarations
> >> +//
> >> +STATIC NOR_FLASH_INSTANCE   **mNorFlashInstances;
> >> +STATIC UINT32               mNorFlashDeviceCount;
> >> +
> >> +STATIC CONST UINT16 mFip006NullCmdSeq[] = {
> >> +  CSDC (0x07, 0, CSDC_TRP_MBM, 1),
> >> +  CSDC (0x07, 0, CSDC_TRP_MBM, 1),
> >> +  CSDC (0x07, 0, CSDC_TRP_MBM, 1),
> >> +  CSDC (0x07, 0, CSDC_TRP_MBM, 1),
> >> +  CSDC (0x07, 0, CSDC_TRP_MBM, 1),
> >> +  CSDC (0x07, 0, CSDC_TRP_MBM, 1),
> >> +  CSDC (0x07, 0, CSDC_TRP_MBM, 1),
> >> +  CSDC (0x07, 0, CSDC_TRP_MBM, 1)
> >
> > Can we get some helpful #defines for these live-coded values please?
> 
> I can fix up the other cosmetic values, but unfortunately, only
> Socionext can address the comments regarding the use of symbolic
> constants, because I don't have a clue what they mean.

Right. That's sort of the problem.
That basically reduces code review to looking for canaries.

> Pipat?
> 
> I will note that some of the comments below apply to
> ArmPlatformPkg/Drivers/NorFlashDxe equally.

Fair enough.
Will need to revisit that one at some point then.

/
    Leif