From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::244; helo=mail-wm0-x244.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x244.google.com (mail-wm0-x244.google.com [IPv6:2a00:1450:400c:c09::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 63C7921F3B3FF for ; Sat, 28 Oct 2017 14:27:16 -0700 (PDT) Received: by mail-wm0-x244.google.com with SMTP id p75so8997422wmg.3 for ; Sat, 28 Oct 2017 14:31:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=0OyEkyoJ+EbijTtkjKO6kdaUGmyWD1UePzSIb/4Ar9I=; b=c2SN6EgnTtfMylCUzYocA3Wa9NQlvxBF7Ab449YZFUEqt385XYe1dsZhzt6xVp+0ZV pITGuO7Qa0jvATSPJvSuHC3CD7GWWo8rfcRcH0BedRskF0sgn2nDtnytsEzKSno/tJFf ZRtgadxwRqS18bdOx8u38wp6Z/6trvSGZ6+7U= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=0OyEkyoJ+EbijTtkjKO6kdaUGmyWD1UePzSIb/4Ar9I=; b=mV0yjcYspcq5ALPfYkyfrs9sXE7SwA4N3S1UM9Qv0GEbygl4rZYIMc5bkhji/dd8Av QuMVz1bi9dE+eH3lmgQww/7/JC5uv44bSqMQdS061E8nuxD76/cybsJIpPIMCHA0o564 RaXxFpY/eH1ZdWyIqb7Bk/IJIZMvaq9TpaPS/vBNUjdIcVWRxvnrZ19zVdct66+Bh4fi B40prwlC+pq0PV0VMjdKDSDL+dlgKFej2kfBlFIcOSokVhw5W5rXprMy+1Tru++UY3av +sKcYJQbke3k8hSNreq1z4Ut0LCuMMdDeBzWSHS5Xsl1ZVliOqgloAb1l33ikLi51JQB v50w== X-Gm-Message-State: AMCzsaXupG0MaKR0gwqG1SFYvTqWsy9epnhynLcWYpVLBD+oHF5/CNB7 jEcSfQVKeZWxeyGM65JPU2GyZg== X-Google-Smtp-Source: ABhQp+RWG1sIoT7WBIMwHiSD1iNtmuxcRzmPuZnQj10NChTgrGFAG7Fh5iruPDb6qv88gztayoPUvw== X-Received: by 10.28.95.10 with SMTP id t10mr199205wmb.72.1509226263538; Sat, 28 Oct 2017 14:31:03 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id f10sm9711425wrg.20.2017.10.28.14.31.02 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 28 Oct 2017 14:31:02 -0700 (PDT) Date: Sat, 28 Oct 2017 22:31:00 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Daniel Thompson , Masami Hiramatsu , =?utf-8?B?UGlwYXQv44Oh44K/44Ov44OL44OD44OI44Od44OzIOODlOODkeODg+ODiA==?= Message-ID: <20171028213100.ctexodlfehapsbt2@bivouac.eciton.net> References: <20171025175947.22798-1-ard.biesheuvel@linaro.org> <20171025175947.22798-14-ard.biesheuvel@linaro.org> <20171026211939.7qace3rpz4ffnbrz@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms v2 13/23] Silicon/Socionext: add driver for SPI NOR flash X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Oct 2017 21:27:16 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Oct 28, 2017 at 03:16:50PM +0100, Ard Biesheuvel wrote: > On 26 October 2017 at 22:19, Leif Lindholm wrote: > > On Wed, Oct 25, 2017 at 06:59:37PM +0100, Ard Biesheuvel wrote: > >> From: Pipat Methavanitpong > >> > >> 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 > >> > >> [various tweaks and bugfixes] > >> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Ard Biesheuvel > > > > 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.
> >> + Copyright (c) 2017, Socionext Inc. All rights reserved.
> >> + Copyright (c) 2017, 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 > >> +#include > >> +#include > >> +#include > >> +#include > > > > 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