[edk2-platforms: PATCH 0/2] Armada 7k8k SPI flash driver improvements

[edk2-platforms: PATCH 0/2] Armada 7k8k SPI flash driver improvements

[Patryk Duda]

Hi,

This patchset contains progress bar implementation for MvSpiFlash driver and
small fix in calculating number of sectors to write.

Progress bar implementation was mostly borrowed from TFTP module.
During implementation it turned out that when size is multiplication of
sector size, then last sector was not copied because remainder was 0.
This was fixed by increasing number of sectors to write.

I'm looking forward to your comments or remarks.

Best regards,
Patryk

Patryk Duda (2):
  Marvell/Drivers: MvSpiFlashDxe: Fix sector number obtaining
  Marvell/Drivers: MvSpiFlashDxe: Implement progress bar

 Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf |  1 +
 Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h   |  1 +
 Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c   | 59 ++++++++++++++++----
 3 files changed, 51 insertions(+), 10 deletions(-)

-- 
2.21.0

[edk2-platforms: PATCH 1/2] Marvell/Drivers: MvSpiFlashDxe: Fix sector number obtaining

[Patryk Duda]

This commit fixes bug which was causing one sector of bytes to be
ommited. It was discovered when bytes to be written was 0 in last block,
but total count of bytes was multiplication of sector size. It turned
out that in every case one sector of data was missing.

This bug can be fixed in various ways, but this solution fixes hypotetical
situation in which total bytes count is smaller than sector size.

Signed-off-by: Patryk Duda <pdk@semihalf.com>
---
 Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
index 02bc281c8b..db12adb764 100755
--- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
+++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
@@ -388,7 +388,7 @@ MvSpiFlashUpdateWithProgress (
   UINT8 *TmpBuf;
 
   SectorSize = Slave->Info->SectorSize;
-  SectorNum = ByteCount / SectorSize;
+  SectorNum = (ByteCount / SectorSize) + 1;
   ToUpdate = SectorSize;
 
   TmpBuf = (UINT8 *)AllocateZeroPool (SectorSize);
-- 
2.21.0

Re: [edk2-platforms: PATCH 1/2] Marvell/Drivers: MvSpiFlashDxe: Fix sector number obtaining

[Leif Lindholm]

On Mon, Sep 09, 2019 at 05:52:11PM +0200, Patryk Duda wrote:
> This commit fixes bug which was causing one sector of bytes to be
> ommited. It was discovered when bytes to be written was 0 in last block,
> but total count of bytes was multiplication of sector size. It turned
> out that in every case one sector of data was missing.
> 
> This bug can be fixed in various ways, but this solution fixes hypotetical
> situation in which total bytes count is smaller than sector size.
> 
> Signed-off-by: Patryk Duda <pdk@semihalf.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Pushed as 6e58a613d9aa.

Thanks!

> ---
>  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> index 02bc281c8b..db12adb764 100755
> --- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> +++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> @@ -388,7 +388,7 @@ MvSpiFlashUpdateWithProgress (
>    UINT8 *TmpBuf;
>  
>    SectorSize = Slave->Info->SectorSize;
> -  SectorNum = ByteCount / SectorSize;
> +  SectorNum = (ByteCount / SectorSize) + 1;
>    ToUpdate = SectorSize;
>  
>    TmpBuf = (UINT8 *)AllocateZeroPool (SectorSize);
> -- 
> 2.21.0
> 

[edk2-platforms: PATCH 2/2] Marvell/Drivers: MvSpiFlashDxe: Implement progress bar

[Patryk Duda]

This patch implements TFTP-like progress bar in
MvSpiFlashUpdateWithProgress. This is necessary because
CapsuleRuntimeDxe uses DxeRuntimeCapsuleLib which uses
CapsuleProcessLibNull implementation of capsule process.

Signed-off-by: Patryk Duda <pdk@semihalf.com>
---
 Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf |  1 +
 Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h   |  1 +
 Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c   | 57 ++++++++++++++++----
 3 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf
index c6e93b82a1..088e9592e2 100644
--- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf
+++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf
@@ -23,6 +23,7 @@
   DebugLib
   MemoryAllocationLib
   NorFlashInfoLib
+  PrintLib
   TimerLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h
index 21877ba2a6..d51892c439 100755
--- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h
+++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h
@@ -9,6 +9,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
+#include <Library/PrintLib.h>
 #include <Library/UefiLib.h>
 #include <Library/DebugLib.h>
 #include <Library/MemoryAllocationLib.h>
diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
index db12adb764..91e195b97c 100755
--- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
+++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
@@ -6,6 +6,25 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 *******************************************************************************/
 #include "MvSpiFlashDxe.h"
 
+// Frame for the progression slider
+STATIC CONST CHAR16 mProgressFrame[] = L"[                                        ]";
+
+// Number of steps in the progression slider
+#define PROGRESS_SLIDER_STEPS  ((sizeof (mProgressFrame) / sizeof (CHAR16)) - 3)
+
+// Size in number of characters plus one (final zero) of the message to
+// indicate the progress of update. The format is "[(progress slider:
+// 40 characters)] (nb of KBytes written so far: 7 characters) Kb". There
+// are thus the number of characters in mProgressFrame[] plus 11 characters
+// (2 // spaces, "Kb" and seven characters for the number of KBytes).
+#define PROGRESS_MESSAGE_SIZE  ((sizeof (mProgressFrame) / sizeof (CHAR16)) + 12)
+
+// String to delete progress message to be able to update it :
+// (PROGRESS_MESSAGE_SIZE-1) '\b'
+STATIC CONST CHAR16 mProgressDelete[] = L"\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"
+                                         "\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"
+                                         "\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b";
+
 STATIC EFI_EVENT            mMvSpiFlashVirtualAddrChangeEvent;
 MARVELL_SPI_MASTER_PROTOCOL *SpiMasterProtocol;
 SPI_FLASH_INSTANCE  *mSpiFlashInstance;
@@ -380,12 +399,15 @@ MvSpiFlashUpdateWithProgress (
   IN UINTN                                          EndPercentage
   )
 {
+  CHAR16 ProgressBar[PROGRESS_MESSAGE_SIZE];
+  UINTN ProgressIndex;
   EFI_STATUS Status;
   UINTN SectorSize;
   UINTN SectorNum;
   UINTN ToUpdate;
-  UINTN Index;
   UINT8 *TmpBuf;
+  UINTN Index;
+  UINTN Step;
 
   SectorSize = Slave->Info->SectorSize;
   SectorNum = (ByteCount / SectorSize) + 1;
@@ -397,12 +419,9 @@ MvSpiFlashUpdateWithProgress (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  for (Index = 0; Index < SectorNum; Index++) {
-    if (Progress != NULL) {
-      Progress (StartPercentage +
-                ((Index * (EndPercentage - StartPercentage)) / SectorNum));
-    }
+  Print(L"%s       0 Kb", mProgressFrame);
 
+  for (Index = 0; Index < SectorNum; Index++) {
     // In the last chunk update only an actual number of remaining bytes.
     if (Index + 1 == SectorNum) {
       ToUpdate = ByteCount % SectorSize;
@@ -418,12 +437,32 @@ MvSpiFlashUpdateWithProgress (
       DEBUG ((DEBUG_ERROR, "%a: Error while updating\n", __FUNCTION__));
       return Status;
     }
+
+    ProgressBar[0] = L'\0';
+    Step = ((Index * SectorSize + ToUpdate) * PROGRESS_SLIDER_STEPS) / ByteCount;
+
+    Print (L"%s", mProgressDelete);
+
+    Status = StrCpyS (ProgressBar, PROGRESS_MESSAGE_SIZE, mProgressFrame);
+    if (EFI_ERROR(Status)) {
+      return Status;
+    }
+    for (ProgressIndex = 1; ProgressIndex < Step; ProgressIndex++) {
+      ProgressBar[ProgressIndex] = L'=';
+    }
+    ProgressBar[Step] = L'>';
+    UnicodeSPrint (
+      ProgressBar + (sizeof (mProgressFrame) / sizeof (CHAR16)) - 1,
+      sizeof (ProgressBar) - sizeof (mProgressFrame),
+      L" %7d Kb",
+      (Index * SectorSize + ToUpdate) / 1024
+      );
+
+    Print(L"%s", ProgressBar);
   }
   FreePool (TmpBuf);
 
-  if (Progress != NULL) {
-    Progress (EndPercentage);
-  }
+  Print (L"\n");
 
   return EFI_SUCCESS;
 }
-- 
2.21.0

Re: [edk2-platforms: PATCH 2/2] Marvell/Drivers: MvSpiFlashDxe: Implement progress bar

[Leif Lindholm]

On Mon, Sep 09, 2019 at 05:52:12PM +0200, Patryk Duda wrote:
> This patch implements TFTP-like progress bar in
> MvSpiFlashUpdateWithProgress. This is necessary because
> CapsuleRuntimeDxe uses DxeRuntimeCapsuleLib which uses
> CapsuleProcessLibNull implementation of capsule process.

I would prefer not adding more progress output code into individual
drivers. Could this driver reuse
MdeModulePkg/Library/DisplayUpdateProgressLibText/ instead?

/
    Leif

> Signed-off-by: Patryk Duda <pdk@semihalf.com>
> ---
>  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf |  1 +
>  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h   |  1 +
>  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c   | 57 ++++++++++++++++----
>  3 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf
> index c6e93b82a1..088e9592e2 100644
> --- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf
> +++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf
> @@ -23,6 +23,7 @@
>    DebugLib
>    MemoryAllocationLib
>    NorFlashInfoLib
> +  PrintLib
>    TimerLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h
> index 21877ba2a6..d51892c439 100755
> --- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h
> +++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h
> @@ -9,6 +9,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  #include <Library/IoLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
>  #include <Library/UefiLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>
> diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> index db12adb764..91e195b97c 100755
> --- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> +++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> @@ -6,6 +6,25 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  *******************************************************************************/
>  #include "MvSpiFlashDxe.h"
>  
> +// Frame for the progression slider
> +STATIC CONST CHAR16 mProgressFrame[] = L"[                                        ]";
> +
> +// Number of steps in the progression slider
> +#define PROGRESS_SLIDER_STEPS  ((sizeof (mProgressFrame) / sizeof (CHAR16)) - 3)
> +
> +// Size in number of characters plus one (final zero) of the message to
> +// indicate the progress of update. The format is "[(progress slider:
> +// 40 characters)] (nb of KBytes written so far: 7 characters) Kb". There
> +// are thus the number of characters in mProgressFrame[] plus 11 characters
> +// (2 // spaces, "Kb" and seven characters for the number of KBytes).
> +#define PROGRESS_MESSAGE_SIZE  ((sizeof (mProgressFrame) / sizeof (CHAR16)) + 12)
> +
> +// String to delete progress message to be able to update it :
> +// (PROGRESS_MESSAGE_SIZE-1) '\b'
> +STATIC CONST CHAR16 mProgressDelete[] = L"\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"
> +                                         "\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"
> +                                         "\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b";
> +
>  STATIC EFI_EVENT            mMvSpiFlashVirtualAddrChangeEvent;
>  MARVELL_SPI_MASTER_PROTOCOL *SpiMasterProtocol;
>  SPI_FLASH_INSTANCE  *mSpiFlashInstance;
> @@ -380,12 +399,15 @@ MvSpiFlashUpdateWithProgress (
>    IN UINTN                                          EndPercentage
>    )
>  {
> +  CHAR16 ProgressBar[PROGRESS_MESSAGE_SIZE];
> +  UINTN ProgressIndex;
>    EFI_STATUS Status;
>    UINTN SectorSize;
>    UINTN SectorNum;
>    UINTN ToUpdate;
> -  UINTN Index;
>    UINT8 *TmpBuf;
> +  UINTN Index;
> +  UINTN Step;
>  
>    SectorSize = Slave->Info->SectorSize;
>    SectorNum = (ByteCount / SectorSize) + 1;
> @@ -397,12 +419,9 @@ MvSpiFlashUpdateWithProgress (
>      return EFI_OUT_OF_RESOURCES;
>    }
>  
> -  for (Index = 0; Index < SectorNum; Index++) {
> -    if (Progress != NULL) {
> -      Progress (StartPercentage +
> -                ((Index * (EndPercentage - StartPercentage)) / SectorNum));
> -    }
> +  Print(L"%s       0 Kb", mProgressFrame);
>  
> +  for (Index = 0; Index < SectorNum; Index++) {
>      // In the last chunk update only an actual number of remaining bytes.
>      if (Index + 1 == SectorNum) {
>        ToUpdate = ByteCount % SectorSize;
> @@ -418,12 +437,32 @@ MvSpiFlashUpdateWithProgress (
>        DEBUG ((DEBUG_ERROR, "%a: Error while updating\n", __FUNCTION__));
>        return Status;
>      }
> +
> +    ProgressBar[0] = L'\0';
> +    Step = ((Index * SectorSize + ToUpdate) * PROGRESS_SLIDER_STEPS) / ByteCount;
> +
> +    Print (L"%s", mProgressDelete);
> +
> +    Status = StrCpyS (ProgressBar, PROGRESS_MESSAGE_SIZE, mProgressFrame);
> +    if (EFI_ERROR(Status)) {
> +      return Status;
> +    }
> +    for (ProgressIndex = 1; ProgressIndex < Step; ProgressIndex++) {
> +      ProgressBar[ProgressIndex] = L'=';
> +    }
> +    ProgressBar[Step] = L'>';
> +    UnicodeSPrint (
> +      ProgressBar + (sizeof (mProgressFrame) / sizeof (CHAR16)) - 1,
> +      sizeof (ProgressBar) - sizeof (mProgressFrame),
> +      L" %7d Kb",
> +      (Index * SectorSize + ToUpdate) / 1024
> +      );
> +
> +    Print(L"%s", ProgressBar);
>    }
>    FreePool (TmpBuf);
>  
> -  if (Progress != NULL) {
> -    Progress (EndPercentage);
> -  }
> +  Print (L"\n");
>  
>    return EFI_SUCCESS;
>  }
> -- 
> 2.21.0
> 

Re: [edk2-platforms: PATCH 0/2] Armada 7k8k SPI flash driver improvements

[Marcin Wojtas]

pon., 9 wrz 2019 o 17:52 Patryk Duda <pdk@semihalf.com> napisał(a):
>
> Hi,
>
> This patchset contains progress bar implementation for MvSpiFlash driver and
> small fix in calculating number of sectors to write.
>
> Progress bar implementation was mostly borrowed from TFTP module.
> During implementation it turned out that when size is multiplication of
> sector size, then last sector was not copied because remainder was 0.
> This was fixed by increasing number of sectors to write.
>
> I'm looking forward to your comments or remarks.
>
> Best regards,
> Patryk
>
> Patryk Duda (2):
>   Marvell/Drivers: MvSpiFlashDxe: Fix sector number obtaining
>   Marvell/Drivers: MvSpiFlashDxe: Implement progress bar
>
>  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf |  1 +
>  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h   |  1 +
>  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c   | 59 ++++++++++++++++----
>  3 files changed, 51 insertions(+), 10 deletions(-)
>

Tested-by: Marcin Wojtas <mw@semihalf.com>

Re: [edk2-platforms: PATCH 0/2] Armada 7k8k SPI flash driver improvements

[Patryk Duda]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

Hi,

Do you have any remarks to the patchset?

Best regards,
Patryk

wt., 10 wrz 2019 o 08:23 Marcin Wojtas <mw@semihalf.com> napisał(a):

> pon., 9 wrz 2019 o 17:52 Patryk Duda <pdk@semihalf.com> napisał(a):
> >
> > Hi,
> >
> > This patchset contains progress bar implementation for MvSpiFlash driver
> and
> > small fix in calculating number of sectors to write.
> >
> > Progress bar implementation was mostly borrowed from TFTP module.
> > During implementation it turned out that when size is multiplication of
> > sector size, then last sector was not copied because remainder was 0.
> > This was fixed by increasing number of sectors to write.
> >
> > I'm looking forward to your comments or remarks.
> >
> > Best regards,
> > Patryk
> >
> > Patryk Duda (2):
> >   Marvell/Drivers: MvSpiFlashDxe: Fix sector number obtaining
> >   Marvell/Drivers: MvSpiFlashDxe: Implement progress bar
> >
> >  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf |  1 +
> >  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.h   |  1 +
> >  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c   | 59
> ++++++++++++++++----
> >  3 files changed, 51 insertions(+), 10 deletions(-)
> >
>
> Tested-by: Marcin Wojtas <mw@semihalf.com>
>

[-- Attachment #2: Type: text/html, Size: 1884 bytes --]