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::233; helo=mail-io0-x233.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-io0-x233.google.com (mail-io0-x233.google.com [IPv6:2607:f8b0:4001:c06::233]) (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 4BC1021FCA298 for ; Wed, 1 Nov 2017 09:31:44 -0700 (PDT) Received: by mail-io0-x233.google.com with SMTP id 97so7311924iok.7 for ; Wed, 01 Nov 2017 09:35:37 -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=d8mSBXlkYshOZjm0SEy0ANOKvq+VNxbT+QRjl2fSesk=; b=vBGRWiukVH/o9BEijw+uYKk3DzmOJXDkdfOJ7WVK68Jc5jmufd+HjZATl5zbRCGdUR u3cc6dVwfOkejw/iPJRb/jK7xqiIsjFEaY6KUwY23twmebt84Xr6KZfFIyGGOehGO/6l M+rqFfizOz2DDokNOkZu3DQhaFyjhlhuFzNE1wmyyjqKqMkKz+KjCnoI7b4SFE1CHl4V 5z8N9ecMxGEuYvf3eeHCaalRkXJcgGJO9K7tj7+eyl9XMkyEpvZqFXVd16KI7AolaKhv Jv3ulStc/s72Er3dWjgVqOyesvsy0EfDLZWwoyOmXSz+hfFAj/mQGHrnRFC8VKCuc5fP Pnpg== 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=d8mSBXlkYshOZjm0SEy0ANOKvq+VNxbT+QRjl2fSesk=; b=nb06jXWa0RxdtMUfYnlNFDqy9Cu7LPzTCRyb8qRHVvISN1qX03JWo5Nnpdv150geTW WRhibfkHBH0LGnDQj0/jjA16xyPIPmbK0BbC7PJ8eEd40EAdvO+cwitLIRgB4u9pGAOg 7/zuyfEr3af8Y/vozE7+Kp3aoz3+sZxdFeIG3j9QfnX5bnLO5ytUUfMf/58nHxUTas21 84DeE26kZuI1eK4wmjJHQYRZPjVZCexZxsnIZ+DJg7pjeyvTimqgZ+b3UEONDSE63mX1 JoqJ1DuFB17gC81oXarWf3ygqxvDqVUg0VwB/tNL4h1nPDdzJ2pkrcKQeb1I++jFAx00 BpYA== X-Gm-Message-State: AMCzsaUYoF1JMvbAL26gS6qGz0eZLY3oh1cjlQGZk0Ba4v8VzSrEIVbi /bqWPWwhom0JVH9cWCEaiJVxxp1dfh+6SKHqx47Lpw== X-Google-Smtp-Source: ABhQp+RmfRn9CU7I69nhHEH2SfR2kIyGw68lDcbbcyEsVMiZffP4MqtjvRNfLIdWrZIqLToVB0z5nxcV5wHmtobJ+5k= X-Received: by 10.36.110.71 with SMTP id w68mr1321241itc.113.1509554136539; Wed, 01 Nov 2017 09:35:36 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.167.208 with HTTP; Wed, 1 Nov 2017 09:35:36 -0700 (PDT) In-Reply-To: <20171101032524.t56cnqir5sdfiqtl@bivouac.eciton.net> References: <1509422375-20198-1-git-send-email-mw@semihalf.com> <1509422375-20198-3-git-send-email-mw@semihalf.com> <20171101032524.t56cnqir5sdfiqtl@bivouac.eciton.net> From: Marcin Wojtas Date: Wed, 1 Nov 2017 17:35:36 +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 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection 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:31:44 -0000 Content-Type: text/plain; charset="UTF-8" Leif, 2017-11-01 4:25 GMT+01:00 Leif Lindholm : > On Tue, Oct 31, 2017 at 04:59:31AM +0100, Marcin Wojtas wrote: >> Hitherto mechanism of fixing SPI flash model in the PCDs, >> occured to be very inefficient and problematic. Enable >> dynamic detection by reworking MvSpiFlashReadId() command, >> which now uses newly added NorFlashInfoLib, that helps to >> obtain description of the JEDEC compliant devices. >> >> This patch updates the MvSpiFlashProtocol ReadId() protocol >> callback on both producer's (MvFlashDxe) and consumers' sides >> (FirmwareUpdate and SpiTool applications). Because all >> information about detected SPI NOR flash is now stored in >> the obtained NorFlashInfo structure fields, use them instead >> of the PCDs. >> >> Enable compilation of the NorFlashInfoLib and update >> PortingGuide documentation accordingly. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Marcin Wojtas >> --- >> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 5 +- >> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf | 4 +- >> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 5 +- >> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf | 2 +- >> Platform/Marvell/Armada/Armada.dsc.inc | 1 + >> Platform/Marvell/Armada/Armada70x0.dsc | 5 -- >> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 68 +++++++++++--------- >> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf | 9 +-- >> Platform/Marvell/Drivers/Spi/MvSpiDxe.inf | 2 + >> Platform/Marvell/Include/Protocol/Spi.h | 3 + >> Platform/Marvell/Include/Protocol/SpiFlash.h | 6 +- >> Platform/Marvell/Marvell.dec | 6 -- >> Silicon/Marvell/Documentation/PortingGuide.txt | 18 ------ >> 13 files changed, 51 insertions(+), 83 deletions(-) >> >> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c >> index d70645d..750e52a 100644 >> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c >> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c >> @@ -94,12 +94,9 @@ SpiFlashProbe ( >> ) >> { >> EFI_STATUS Status; >> - UINT8 *FlashId; >> - >> - FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId); >> >> // Read SPI flash ID >> - Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId); >> + Status = SpiFlashProtocol->ReadId (Slave, FALSE); >> if (EFI_ERROR (Status)) { >> return SHELL_ABORTED; >> } >> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf >> index 92c3333..53ea491 100644 >> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf >> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf >> @@ -44,6 +44,7 @@ >> FUpdate.uni >> >> [Packages] >> + EmbeddedPkg/EmbeddedPkg.dec >> MdeModulePkg/MdeModulePkg.dec >> MdePkg/MdePkg.dec >> Platform/Marvell/Marvell.dec >> @@ -64,9 +65,6 @@ >> UefiLib >> UefiRuntimeServicesTableLib >> >> -[Pcd] >> - gMarvellTokenSpaceGuid.PcdSpiFlashId >> - >> [Protocols] >> gMarvellSpiFlashProtocolGuid >> gMarvellSpiMasterProtocolGuid >> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c >> index a12f2ec..68a6cf7 100644 >> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c >> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c >> @@ -166,11 +166,8 @@ FlashProbe ( >> ) >> { >> EFI_STATUS Status; >> - UINT8 *FlashId; >> >> - FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId); >> - >> - Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId); >> + Status = SpiFlashProtocol->ReadId (Slave, FALSE); >> if (EFI_ERROR (Status)) { >> return SHELL_ABORTED; >> } >> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf >> index 887b9a5..a52906b 100644 >> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf >> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf >> @@ -44,6 +44,7 @@ >> SpiFlashCmd.uni >> >> [Packages] >> + EmbeddedPkg/EmbeddedPkg.dec >> MdePkg/MdePkg.dec >> ShellPkg/ShellPkg.dec >> MdeModulePkg/MdeModulePkg.dec >> @@ -66,7 +67,6 @@ >> >> [Pcd] >> gMarvellTokenSpaceGuid.PcdSpiFlashCs >> - gMarvellTokenSpaceGuid.PcdSpiFlashId >> gMarvellTokenSpaceGuid.PcdSpiFlashMode >> >> [Protocols] >> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc >> index b9fc384..2cd96e6 100644 >> --- a/Platform/Marvell/Armada/Armada.dsc.inc >> +++ b/Platform/Marvell/Armada/Armada.dsc.inc >> @@ -33,6 +33,7 @@ >> ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf >> ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf >> MppLib|Platform/Marvell/Library/MppLib/MppLib.inf >> + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf >> UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf >> >> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf >> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc >> index 4d5f55f..8e4cdb2 100644 >> --- a/Platform/Marvell/Armada/Armada70x0.dsc >> +++ b/Platform/Marvell/Armada/Armada70x0.dsc >> @@ -90,11 +90,6 @@ >> gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|10000000 >> gMarvellTokenSpaceGuid.PcdSpiClockFrequency|200000000 >> >> - gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0x70 >> - gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3 >> - gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536 >> - gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256 >> - gMarvellTokenSpaceGuid.PcdSpiFlashId|{ 0x20, 0xBA, 0x18 } >> gMarvellTokenSpaceGuid.PcdSpiFlashMode|3 >> gMarvellTokenSpaceGuid.PcdSpiFlashCs|0 >> >> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c >> index ab3ed6a..703994c 100755 >> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c >> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c >> @@ -107,10 +107,10 @@ MvSpiFlashWriteCommon ( >> UINT8 PollBit = STATUS_REG_POLL_WIP; >> UINT8 CheckStatus = 0x0; >> >> - CmdStatus = (UINT8)PcdGet32 (PcdSpiFlashPollCmd); >> - if (CmdStatus == CMD_FLAG_STATUS) { >> + if (Slave->Info->Flags & NF_WRITE_FSR) { >> + CmdStatus = CMD_FLAG_STATUS; >> PollBit = STATUS_REG_POLL_PEC; >> - CheckStatus = PollBit; >> + CheckStatus = STATUS_REG_POLL_PEC; >> } >> >> // Send command >> @@ -181,8 +181,19 @@ MvSpiFlashErase ( >> UINTN EraseSize; >> UINT8 Cmd[5]; >> >> - AddrSize = PcdGet32 (PcdSpiFlashAddressCycles); >> - EraseSize = PcdGet64 (PcdSpiFlashEraseSize); >> + if (Slave->Info->Flags & NF_4B_ADDR) { >> + AddrSize = 4; >> + } else { >> + AddrSize = 3; >> + } >> + >> + if (Slave->Info->Flags & NF_ERASE_4K) { >> + Cmd[0] = CMD_ERASE_4K; >> + EraseSize = SIZE_4KB; >> + } else { > > Are 4 and 64K the only legal sizes? > This patch deletes a detection of 32K size and the error message (and > return) if an unknown size is encountered. I didn't find 32K flash in Linux and U-Boot, maybe there some exotic models, which use it. Anyway, there is no size check, as there are two valid options now - 64KB and 4KB size, which is determined by Info->Flag. Previously it was explicit EraseSize value, so the check for wrong value was needed. Although unused, I can add NF_ERASE_32K flag to NorFlashInfoLib and extend a check here. > >> + Cmd[0] = CMD_ERASE_64K; >> + EraseSize = Slave->Info->SectorSize; >> + } >> >> // Check input parameters >> if (Offset % EraseSize || Length % EraseSize) { >> @@ -191,21 +202,6 @@ MvSpiFlashErase ( >> return EFI_DEVICE_ERROR; >> } >> >> - switch (EraseSize) { >> - case SIZE_4KB: >> - Cmd[0] = CMD_ERASE_4K; >> - break; >> - case SIZE_32KB: >> - Cmd[0] = CMD_ERASE_32K; >> - break; >> - case SIZE_64KB: >> - Cmd[0] = CMD_ERASE_64K; >> - break; >> - default: >> - DEBUG ((DEBUG_ERROR, "MvSpiFlash: Invalid EraseSize parameter\n")); >> - return EFI_INVALID_PARAMETER; >> - } >> - >> while (Length) { >> EraseAddr = Offset; >> >> @@ -239,7 +235,11 @@ MvSpiFlashRead ( >> UINT32 AddrSize, ReadAddr, ReadLength, RemainLength; >> UINTN BankSel = 0; >> >> - AddrSize = PcdGet32 (PcdSpiFlashAddressCycles); >> + if (Slave->Info->Flags & NF_4B_ADDR) { >> + AddrSize = 4; >> + } else { >> + AddrSize = 3; >> + } > > I see this sequence repeated many times. > Could it be replaced by a macro or static helper function? Or cached > in a device structure? > Ok. Thanks, Marcin