From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4001:c06::242; helo=mail-io0-x242.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (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 DC9EB202E5C97 for ; Wed, 1 Nov 2017 09:24:31 -0700 (PDT) Received: by mail-io0-x242.google.com with SMTP id n137so7272244iod.6 for ; Wed, 01 Nov 2017 09:28:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=KXsaSc7ecdmyD07b02WAkOaffhAvHqp58zknUAbHDCo=; b=unJRjTe9WeCCaXD8pibW2n1zFtkq8YtMSraw76r3aXyJdWJchGR1qZ9jMget5OyNGT hJnocKSYfvDe9wNXoIdNTXJzzaRmjCpcOIjYju7QHvJrANjhb4ET5DnCTDxldBzgyH0Y bMgUqnmbM+VFokafxVKKkg0LDeRL1BmfM4QeWJ+oItS842ynlYg3v/9H+x9K1HCSJw6p kLBMd6yCpr52azrQaX0C+mih4k+FZ6Qe5TWQfVpdGL17giO2J4GpKqLNhitd7ylm4lRy HKAv8bMf7Mm68v9o/iNJrPpOc3FGJu3PRHM6wuqNg9mp4nDfBwF3EMRX9K1573YXmwCn fUww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=KXsaSc7ecdmyD07b02WAkOaffhAvHqp58zknUAbHDCo=; b=oOsmrhPgWPFRGuFoXReKo1J6FdtaXjBHYLy0OpovqHKE7UQbxUv7B7eN8/hDwvsWIh YL05EpdqYhxeYAF++ZM6BUxKcDR5sHOYqwbNVxqjIGl4bDnKRiTEsMLTnsXqyg1QtCkx p+grnQleqrvqn1WoHm937SimplUELZUA/Yeg2ZITVdx+iH9cBMTEQtnOLrJyTnb9zhrd iJSjygUCYBtWJp/68V84YfWLKSO63/NycwQ2VmjRxiJliIGSd5KnpKyCU1+2Ef0lj4+E gRDi62bKDnvhB4yM7q9gvQLcXaHfVYDMZdDkl6z84QT5jPGP5Q4f82L2LazkjcJZzWeH jfCg== X-Gm-Message-State: AMCzsaUGbpwDq69upQVHwf/nndaj3q4AuRCKQJTiRBdnQ7PT+SV7hwgp tJMM/rIUqJDNE2XDm6Sgmdre3Qhvu6ROjAbxfx3loA== X-Google-Smtp-Source: ABhQp+R+1GDm3MnC5uv03eyPzVIhHowOON0egAfgFM93rkDNzOQAdXVFSeJ6OLsmqBW5MLj7++qIFxrEE+GRz2P/ZBQ= X-Received: by 10.36.105.65 with SMTP id e62mr1276536itc.16.1509553704191; Wed, 01 Nov 2017 09:28:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.167.208 with HTTP; Wed, 1 Nov 2017 09:28:23 -0700 (PDT) In-Reply-To: <20171101031413.kwvnyuzfyl4mlo7z@bivouac.eciton.net> References: <1509422375-20198-1-git-send-email-mw@semihalf.com> <1509422375-20198-2-git-send-email-mw@semihalf.com> <20171031090717.xwbjaitebmcfdiue@bivouac.eciton.net> <20171101031413.kwvnyuzfyl4mlo7z@bivouac.eciton.net> From: Marcin Wojtas Date: Wed, 1 Nov 2017 17:28:23 +0100 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan Subject: Re: [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId 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: Wed, 01 Nov 2017 16:24:32 -0000 Content-Type: text/plain; charset="UTF-8" 2017-11-01 4:14 GMT+01:00 Leif Lindholm : > On Tue, Oct 31, 2017 at 10:22:50AM +0100, Marcin Wojtas wrote: >> 2017-10-31 10:07 GMT+01:00 Leif Lindholm : >> > On Tue, Oct 31, 2017 at 04:59:30AM +0100, Marcin Wojtas wrote: >> >> Fix the ReadId routine by using master's ReadWrite callback >> >> instead of the raw Transfer - no longer swapping and byte >> >> shifting is needed. Simplify code by using local array >> >> instead of dynamic allocation. Moreover store the FlashId >> >> in an UINT8 array PCD instead of the concatenated UINT32 >> >> format - this way less overhead in the driver is needed >> >> for comparing the buffers. >> >> >> >> The new handling allowed for cleaning Fupdate and Sf >> >> shell commands FlashProbe routines. >> >> >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> >> Signed-off-by: Marcin Wojtas >> >> --- >> >> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 22 +++-------- >> >> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 37 ++++++------------ >> >> Platform/Marvell/Armada/Armada70x0.dsc | 2 +- >> >> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 41 ++++++++++++-------- >> >> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h | 2 + >> >> Platform/Marvell/Include/Protocol/SpiFlash.h | 3 ++ >> >> Platform/Marvell/Marvell.dec | 2 +- >> >> 7 files changed, 48 insertions(+), 61 deletions(-) >> >> >> >> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c >> >> index 664411a..d70645d 100644 >> >> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c >> >> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c >> >> @@ -94,28 +94,16 @@ SpiFlashProbe ( >> >> ) >> >> { >> >> EFI_STATUS Status; >> >> - UINT32 IdBuffer, Id, RefId; >> >> + UINT8 *FlashId; >> >> >> >> - Id = PcdGet32 (PcdSpiFlashId); >> >> - >> >> - IdBuffer = CMD_READ_ID & 0xff; >> >> + FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId); >> >> >> >> // Read SPI flash ID >> >> - SpiFlashProtocol->ReadId (Slave, sizeof (UINT32), (UINT8 *)&IdBuffer); >> >> - >> >> - // Swap and extract 3 bytes of the ID >> >> - RefId = SwapBytes32 (IdBuffer) >> 8; >> >> - >> >> - if (RefId == 0) { >> >> - Print (L"%s: No SPI flash detected"); >> >> - return EFI_DEVICE_ERROR; >> >> - } else if (RefId != Id) { >> >> - Print (L"%s: Unsupported SPI flash detected with ID=%2x\n", CMD_NAME_STRING, RefId); >> >> - return EFI_DEVICE_ERROR; >> >> + Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId); >> > >> > Is the length not possible to calculate somehow? >> > Having a MAX_LEN defined and then using a DEFAULT_LEN or explicitly >> > extracting 3 bytes from somewhere feels suboptimal. >> > >> >> I know. It is however a change that was somewhat artificially >> extracted, so that to make the next patch more readable >> (NOR_FLASH_ID_DEFAULT_LEN is removed there). I will substitute it with >> PcdGetSize (PcdSpiFlashId). > > Right, OK, that sort of thing is useful to mention in the cover > letter. > > But there's also MAX_ID_LEN, which is added here only to allocate > buffers that are hard-coded to always contain value of ID_DEFAULT_LEN. > Surely this could just be left out? > The id can be either 3 or 5 bytes in the extended variant, which is used by some flash devices. In theory we can define such pcd and PcdGetSize ensures flexibility. However with the next commit and NorFlashInfoLib table, MAX_ID_LEN is suitable and used with new implementation. In the new version of commit I already removed ID_DEFAULT_LEN. Marcin