[platforms: PATCH v2 00/10] Armada 7k/8k - misc improvements pt.2

[platforms: PATCH v2 00/10] Armada 7k/8k - misc improvements pt.2

[Marcin Wojtas]

Hi,

I present you a second version of the patchset with post review
fixes and improvements. 9/10 patch was completely changed -
the diff is bigger, but such generic clock handling gives some chances
to benefit if we want to merge Xenon support with original EDK2
SdMmc driver in future. More details can be found in the commit logs and
a changelog below.

The patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/misc-upstream-r20171027

I'm looking forward to your comments or remarks.

Best regards,
Marcin

Changelog:
v1 -> v2:
1/10
  - remove unrelated style fix
  - fix style around modified functions calls

2/10
  - leave original EFI_SUCCESS assignment

6/10
  - use descriptively named temporary variable for pin index in a loop

7/10
  - use single flag for link up/down
  - simplify logic
  - correct style

8/10
  - mention missing SDR25 in a commit message

9/10
  - use new member of SD_MMC_HC_PRIVATE_DATA
    to set actual input clock speed and use it for the output
    clock configuration
  - rewrite commit message

3/10, 4/10, 5/10, 10/10
  - add RB's

Ard Biesheuvel (2):
  Marvell/Library: MppLib: Disable the stack protector
  Marvell/Library: MppLib: Take 0xFF placeholders into account

David Greeson (2):
  Marvell/Drivers: MvI2cDxe: Abort transaction immediately upon fail
  Marvell/Drivers: MvI2cDxe: Reduce bus occupation time

Joe Zhou (1):
  Marvell/Library: MppLib: Prevent overwriting PCD values

Marcin Wojtas (5):
  Marvell/Drivers: MvI2cDxe: Fix returning status in MvI2cStartRequest
  Marvell/Drivers: Pp2Dxe: Change settings for the always-up link
  Marvell/Drivers: XenonDxe: Fix UHS signalling mode setting
  Marvell/Drivers: XenonDxe: Allow overriding base clock frequency
  Marvell/Drivers: XenonDxe: Do not modify FIFO default values

 Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c        | 70 ++++++++++++--------
 Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h        |  2 +-
 Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c          | 25 +++++++
 Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h          |  6 ++
 Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c            |  6 +-
 Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c    |  6 +-
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c      |  4 +-
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c | 13 ++--
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h |  6 ++
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c   | 22 +++---
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h   | 12 ++--
 Platform/Marvell/Drivers/SdMmc/XenonDxe/XenonSdhci.c    | 16 -----
 Platform/Marvell/Library/MppLib/MppLib.c                | 35 +++++-----
 Platform/Marvell/Library/MppLib/MppLib.inf              |  3 +
 14 files changed, 140 insertions(+), 86 deletions(-)

-- 
2.7.4

[platforms: PATCH v2 01/10] Marvell/Drivers: MvI2cDxe: Abort transaction immediately upon fail

[Marcin Wojtas]

From: David Greeson <dgreeson@cisco.com>

Although the I2C transaction routines were prepared to
return their status, they were never used. This could
cause bus lock-up e.g. in case of failing to send a
slave address, the data transfer was attempted to be
continued anyway.

This patch fixes faulty behavior by checking transaction
status and stopping it immediately, once the fail
is detected. On the occasion fix style around modified
functions calls.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: David Greeson <dgreeson@cisco.com>
[Style adjustment and cleanup]
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 62 +++++++++++++-------
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
index d85ee0b..b4599d2 100755
--- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
+++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
@@ -565,6 +565,7 @@ MvI2cStartRequest (
   UINTN Transmitted;
   I2C_MASTER_CONTEXT *I2cMasterContext = I2C_SC_FROM_MASTER(This);
   EFI_I2C_OPERATION *Operation;
+  EFI_STATUS Status = EFI_SUCCESS;
 
   ASSERT (RequestPacket != NULL);
   ASSERT (I2cMasterContext != NULL);
@@ -574,33 +575,52 @@ MvI2cStartRequest (
     ReadMode = Operation->Flags & I2C_FLAG_READ;
 
     if (Count == 0) {
-      MvI2cStart ( I2cMasterContext,
-                   (SlaveAddress << 1) | ReadMode,
-                   I2C_TRANSFER_TIMEOUT
-                 );
+      Status = MvI2cStart (I2cMasterContext,
+                 (SlaveAddress << 1) | ReadMode,
+                 I2C_TRANSFER_TIMEOUT);
     } else if (!(Operation->Flags & I2C_FLAG_NORESTART)) {
-      MvI2cRepeatedStart ( I2cMasterContext,
-                           (SlaveAddress << 1) | ReadMode,
-                           I2C_TRANSFER_TIMEOUT
-                         );
+      Status = MvI2cRepeatedStart (I2cMasterContext,
+                 (SlaveAddress << 1) | ReadMode,
+                 I2C_TRANSFER_TIMEOUT);
     }
 
+    /* I2C transaction was aborted, so stop further transactions */
+    if (EFI_ERROR (Status)) {
+      MvI2cStop (I2cMasterContext);
+      break;
+    }
+
+    /*
+     * If sending the slave address was successful,
+     * proceed to read or write section.
+     */
     if (ReadMode) {
-      MvI2cRead ( I2cMasterContext,
-                  Operation->Buffer,
-                  Operation->LengthInBytes,
-                  &Transmitted,
-                  Count == 1,
-                  I2C_TRANSFER_TIMEOUT
-                 );
+      Status = MvI2cRead (I2cMasterContext,
+                 Operation->Buffer,
+                 Operation->LengthInBytes,
+                 &Transmitted,
+                 Count == 1,
+                 I2C_TRANSFER_TIMEOUT);
+      Operation->LengthInBytes = Transmitted;
     } else {
-      MvI2cWrite ( I2cMasterContext,
-                   Operation->Buffer,
-                   Operation->LengthInBytes,
-                   &Transmitted,
-                   I2C_TRANSFER_TIMEOUT
-                  );
+      Status = MvI2cWrite (I2cMasterContext,
+                 Operation->Buffer,
+                 Operation->LengthInBytes,
+                 &Transmitted,
+                 I2C_TRANSFER_TIMEOUT);
+      Operation->LengthInBytes = Transmitted;
     }
+
+    /*
+     * The I2C read or write transaction failed.
+     * Stop the I2C transaction.
+     */
+    if (EFI_ERROR (Status)) {
+      MvI2cStop (I2cMasterContext);
+      break;
+    }
+
+    /* Check if there is any more data to be sent */
     if (Count == RequestPacket->OperationCount - 1) {
       MvI2cStop ( I2cMasterContext );
     }
-- 
2.7.4

Re: [platforms: PATCH v2 01/10] Marvell/Drivers: MvI2cDxe: Abort transaction immediately upon fail

[Leif Lindholm]

On Fri, Oct 27, 2017 at 03:13:43AM +0200, Marcin Wojtas wrote:
> From: David Greeson <dgreeson@cisco.com>
> 
> Although the I2C transaction routines were prepared to
> return their status, they were never used. This could
> cause bus lock-up e.g. in case of failing to send a
> slave address, the data transfer was attempted to be
> continued anyway.
> 
> This patch fixes faulty behavior by checking transaction
> status and stopping it immediately, once the fail
> is detected. On the occasion fix style around modified
> functions calls.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: David Greeson <dgreeson@cisco.com>
> [Style adjustment and cleanup]
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 62 +++++++++++++-------
>  1 file changed, 41 insertions(+), 21 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> index d85ee0b..b4599d2 100755
> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> @@ -565,6 +565,7 @@ MvI2cStartRequest (
>    UINTN Transmitted;
>    I2C_MASTER_CONTEXT *I2cMasterContext = I2C_SC_FROM_MASTER(This);
>    EFI_I2C_OPERATION *Operation;
> +  EFI_STATUS Status = EFI_SUCCESS;
>  
>    ASSERT (RequestPacket != NULL);
>    ASSERT (I2cMasterContext != NULL);
> @@ -574,33 +575,52 @@ MvI2cStartRequest (
>      ReadMode = Operation->Flags & I2C_FLAG_READ;
>  
>      if (Count == 0) {
> -      MvI2cStart ( I2cMasterContext,
> -                   (SlaveAddress << 1) | ReadMode,
> -                   I2C_TRANSFER_TIMEOUT
> -                 );
> +      Status = MvI2cStart (I2cMasterContext,
> +                 (SlaveAddress << 1) | ReadMode,
> +                 I2C_TRANSFER_TIMEOUT);
>      } else if (!(Operation->Flags & I2C_FLAG_NORESTART)) {
> -      MvI2cRepeatedStart ( I2cMasterContext,
> -                           (SlaveAddress << 1) | ReadMode,
> -                           I2C_TRANSFER_TIMEOUT
> -                         );
> +      Status = MvI2cRepeatedStart (I2cMasterContext,
> +                 (SlaveAddress << 1) | ReadMode,
> +                 I2C_TRANSFER_TIMEOUT);
>      }
>  
> +    /* I2C transaction was aborted, so stop further transactions */
> +    if (EFI_ERROR (Status)) {
> +      MvI2cStop (I2cMasterContext);
> +      break;
> +    }
> +
> +    /*
> +     * If sending the slave address was successful,
> +     * proceed to read or write section.
> +     */
>      if (ReadMode) {
> -      MvI2cRead ( I2cMasterContext,
> -                  Operation->Buffer,
> -                  Operation->LengthInBytes,
> -                  &Transmitted,
> -                  Count == 1,
> -                  I2C_TRANSFER_TIMEOUT
> -                 );
> +      Status = MvI2cRead (I2cMasterContext,
> +                 Operation->Buffer,
> +                 Operation->LengthInBytes,
> +                 &Transmitted,
> +                 Count == 1,
> +                 I2C_TRANSFER_TIMEOUT);
> +      Operation->LengthInBytes = Transmitted;
>      } else {
> -      MvI2cWrite ( I2cMasterContext,
> -                   Operation->Buffer,
> -                   Operation->LengthInBytes,
> -                   &Transmitted,
> -                   I2C_TRANSFER_TIMEOUT
> -                  );
> +      Status = MvI2cWrite (I2cMasterContext,
> +                 Operation->Buffer,
> +                 Operation->LengthInBytes,
> +                 &Transmitted,
> +                 I2C_TRANSFER_TIMEOUT);
> +      Operation->LengthInBytes = Transmitted;
>      }
> +
> +    /*
> +     * The I2C read or write transaction failed.
> +     * Stop the I2C transaction.
> +     */
> +    if (EFI_ERROR (Status)) {
> +      MvI2cStop (I2cMasterContext);
> +      break;
> +    }
> +
> +    /* Check if there is any more data to be sent */
>      if (Count == RequestPacket->OperationCount - 1) {
>        MvI2cStop ( I2cMasterContext );
>      }
> -- 
> 2.7.4
> 

[platforms: PATCH v2 02/10] Marvell/Drivers: MvI2cDxe: Fix returning status in MvI2cStartRequest

[Marcin Wojtas]

In MvI2cStartRequest the status was assigned to the variable
without dereferencing a pointer. Fix it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
index b4599d2..a62383f 100755
--- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
+++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
@@ -627,7 +627,7 @@ MvI2cStartRequest (
   }
 
   if (I2cStatus != NULL)
-    I2cStatus = EFI_SUCCESS;
+    *I2cStatus = EFI_SUCCESS;
   if (Event != NULL)
     gBS->SignalEvent(Event);
   return EFI_SUCCESS;
-- 
2.7.4

Re: [platforms: PATCH v2 02/10] Marvell/Drivers: MvI2cDxe: Fix returning status in MvI2cStartRequest

[Leif Lindholm]

On Fri, Oct 27, 2017 at 03:13:44AM +0200, Marcin Wojtas wrote:
> In MvI2cStartRequest the status was assigned to the variable
> without dereferencing a pointer. Fix it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> index b4599d2..a62383f 100755
> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> @@ -627,7 +627,7 @@ MvI2cStartRequest (
>    }
>  
>    if (I2cStatus != NULL)
> -    I2cStatus = EFI_SUCCESS;
> +    *I2cStatus = EFI_SUCCESS;
>    if (Event != NULL)
>      gBS->SignalEvent(Event);
>    return EFI_SUCCESS;
> -- 
> 2.7.4
> 

[platforms: PATCH v2 03/10] Marvell/Drivers: MvI2cDxe: Reduce bus occupation time

[Marcin Wojtas]

From: David Greeson <dgreeson@cisco.com>

During each transaction start, clearing the I2C_CONTROL_FLAG
was surrounded by 3 uncoditional stalls. This was not necessary,
so replace them with one busy-wait loop, whose polling
count could be also safely reduced.

Above improvements result in faster transfer initialization
and allow to reduce the I2C bus occupation.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: David Greeson <dgreeson@cisco.com>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 6 +-----
 Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h | 2 +-
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
index a62383f..d6f590d 100755
--- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
+++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
@@ -243,9 +243,8 @@ MvI2cClearIflg (
  IN I2C_MASTER_CONTEXT *I2cMasterContext
  )
 {
-  gBS->Stall(I2C_OPERATION_TIMEOUT);
+  MvI2cPollCtrl (I2cMasterContext, I2C_OPERATION_TIMEOUT, I2C_CONTROL_IFLG);
   MvI2cControlClear(I2cMasterContext, I2C_CONTROL_IFLG);
-  gBS->Stall(I2C_OPERATION_TIMEOUT);
 }
 
 /* Timeout is given in us */
@@ -295,9 +294,6 @@ MvI2cLockedStart (
     MvI2cClearIflg(I2cMasterContext);
   }
 
-  /* Without this delay we Timeout checking IFLG if the Timeout is 0 */
-  gBS->Stall(I2C_OPERATION_TIMEOUT);
-
   if (MvI2cPollCtrl(I2cMasterContext, Timeout, I2C_CONTROL_IFLG)) {
     DEBUG((DEBUG_ERROR, "MvI2cDxe: Timeout sending %sSTART condition\n",
         Mask == I2C_STATUS_START ? "" : "repeated "));
diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
index 028fd54..3c9beaf 100644
--- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
+++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
@@ -68,7 +68,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #define I2C_SOFT_RESET    0x1c
 #define I2C_TRANSFER_TIMEOUT 10000
-#define I2C_OPERATION_TIMEOUT 1000
+#define I2C_OPERATION_TIMEOUT 100
 
 #define I2C_UNKNOWN        0x0
 #define I2C_SLOW           0x1
-- 
2.7.4

[platforms: PATCH v2 04/10] Marvell/Library: MppLib: Prevent overwriting PCD values

[Marcin Wojtas]

From: Joe Zhou <shjzhou@marvell.com>

After enabling dynamic PCDs, it is possible to reconfigure
MPP during platform initialization. It occurred that due to
a faulty way of passing temporary values, information obtained
from PCDs was overwritten. This patch fixes the issue, which
on the occasion simplifies PcdToMppRegs function.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Joe Zhou <shjzhou@marvell.com>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/Marvell/Library/MppLib/MppLib.c | 21 ++++++++------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/Platform/Marvell/Library/MppLib/MppLib.c b/Platform/Marvell/Library/MppLib/MppLib.c
index c09acf9..383c820 100644
--- a/Platform/Marvell/Library/MppLib/MppLib.c
+++ b/Platform/Marvell/Library/MppLib/MppLib.c
@@ -74,7 +74,7 @@ STATIC
 VOID
 SetRegisterValue (
   UINT8 RegCount,
-  UINT8 **MppRegPcd,
+  UINT8 MppRegPcd[][MPP_PINS_PER_REG],
   UINTN BaseAddr,
   BOOLEAN ReverseFlag
   )
@@ -99,10 +99,10 @@ STATIC
 UINT8
 PcdToMppRegs (
   UINTN PinCount,
-  UINT8 **MppRegPcd
+  UINT8 **MppRegPcd,
+  UINT8 MppRegPcdTmp[][MPP_PINS_PER_REG]
   )
 {
-  UINT8 MppRegPcdTmp[MPP_MAX_REGS][MPP_PINS_PER_REG];
   UINT8 PcdGroupCount, MppRegCount;
   UINTN i, j, k, l;
 
@@ -125,14 +125,7 @@ PcdToMppRegs (
     for (j = 0; j < PCD_PINS_PER_GROUP; j++) {
       k = (PCD_PINS_PER_GROUP * i + j) / MPP_PINS_PER_REG;
       l = (PCD_PINS_PER_GROUP * i + j) % MPP_PINS_PER_REG;
-      MppRegPcdTmp[k][l] = MppRegPcd[i][j];
-    }
-  }
-
-  /* Update input table */
-  for (i = 0; i < MppRegCount; i++) {
-    for (j = 0; j < MPP_PINS_PER_REG; j++) {
-      MppRegPcd[i][j] = MppRegPcdTmp[i][j];
+      MppRegPcdTmp[k][l] = (UINT8)MppRegPcd[i][j];
     }
   }
 
@@ -191,6 +184,7 @@ MppInitialize (
   BOOLEAN ReverseFlag[MAX_CHIPS];
   UINT8 *MppRegPcd[MAX_CHIPS][MPP_MAX_REGS];
   UINT32 i, ChipCount;
+  UINT8 TmpMppValue[MPP_MAX_REGS][MPP_PINS_PER_REG];
 
   ChipCount = PcdGet32 (PcdMppChipCount);
 
@@ -203,8 +197,9 @@ MppInitialize (
   for (i = 0; i < MAX_CHIPS; i++) {
     if (i == ChipCount)
       break;
-    RegCount = PcdToMppRegs (PinCount[i], MppRegPcd[i]);
-    SetRegisterValue (RegCount, MppRegPcd[i], BaseAddr[i], ReverseFlag[i]);
+
+    RegCount = PcdToMppRegs (PinCount[i], MppRegPcd[i], TmpMppValue);
+    SetRegisterValue (RegCount, TmpMppValue, BaseAddr[i], ReverseFlag[i]);
 
     /*
      * eMMC PHY IP has its own MPP configuration.
-- 
2.7.4

[platforms: PATCH v2 05/10] Marvell/Library: MppLib: Disable the stack protector

[Marcin Wojtas]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

MppLib may be used very early (in SEC), at which point stack protection
measures are more likely to cause harm than help, given that not even
the UART has been configured to the point where we can complain usefully.
So just disable it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/Marvell/Library/MppLib/MppLib.inf | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Platform/Marvell/Library/MppLib/MppLib.inf b/Platform/Marvell/Library/MppLib/MppLib.inf
index 2de9cd0..1268542 100644
--- a/Platform/Marvell/Library/MppLib/MppLib.inf
+++ b/Platform/Marvell/Library/MppLib/MppLib.inf
@@ -106,3 +106,6 @@
   gMarvellTokenSpaceGuid.PcdChip3MppSel7
 
   gMarvellTokenSpaceGuid.PcdPciESdhci
+
+[BuildOptions]
+  *_*_*_CC_FLAGS = -fno-stack-protector
-- 
2.7.4

[platforms: PATCH v2 06/10] Marvell/Library: MppLib: Take 0xFF placeholders into account

[Marcin Wojtas]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

The MppSel definition PCDs contain 0xFF placeholders for values that
should be left untouched. MppLib needs to be taught how to take those
into account.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Library/MppLib/MppLib.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Platform/Marvell/Library/MppLib/MppLib.c b/Platform/Marvell/Library/MppLib/MppLib.c
index 383c820..297725f 100644
--- a/Platform/Marvell/Library/MppLib/MppLib.c
+++ b/Platform/Marvell/Library/MppLib/MppLib.c
@@ -79,18 +79,24 @@ SetRegisterValue (
   BOOLEAN ReverseFlag
   )
 {
-  UINT32 i, j, CtrlVal;
+  UINT32 i, j, CtrlVal, CtrlMask, PinIndex;
   INTN Sign;
 
   Sign = ReverseFlag ? -1 : 1;
 
   for (i = 0; i < RegCount; i++) {
     CtrlVal = 0;
+    CtrlMask = 0;
     for (j = 0; j < MPP_PINS_PER_REG; j++) {
-      CtrlVal |= MPP_PIN_VAL(7 * (UINTN) ReverseFlag + j * Sign,
-        MppRegPcd[i][7 * (UINTN) ReverseFlag + j * Sign]);
+
+      PinIndex = 7 * (UINTN)ReverseFlag + j * Sign;
+
+      if (MppRegPcd[i][PinIndex] != 0xff) {
+        CtrlVal |= MPP_PIN_VAL(PinIndex, MppRegPcd[i][PinIndex]);
+        CtrlMask |= MPP_PIN_VAL(PinIndex, 0xf);
+      }
     }
-    MmioWrite32 (BaseAddr + 4 * i * Sign, CtrlVal);
+    MmioAndThenOr32 (BaseAddr + 4 * i * Sign, ~CtrlMask, CtrlVal);
   }
 }
 
-- 
2.7.4

Re: [platforms: PATCH v2 06/10] Marvell/Library: MppLib: Take 0xFF placeholders into account

[Leif Lindholm]

On Fri, Oct 27, 2017 at 03:13:48AM +0200, Marcin Wojtas wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> The MppSel definition PCDs contain 0xFF placeholders for values that
> should be left untouched. MppLib needs to be taught how to take those
> into account.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

That is so much nicer than the v1, thanks.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Platform/Marvell/Library/MppLib/MppLib.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/Platform/Marvell/Library/MppLib/MppLib.c b/Platform/Marvell/Library/MppLib/MppLib.c
> index 383c820..297725f 100644
> --- a/Platform/Marvell/Library/MppLib/MppLib.c
> +++ b/Platform/Marvell/Library/MppLib/MppLib.c
> @@ -79,18 +79,24 @@ SetRegisterValue (
>    BOOLEAN ReverseFlag
>    )
>  {
> -  UINT32 i, j, CtrlVal;
> +  UINT32 i, j, CtrlVal, CtrlMask, PinIndex;
>    INTN Sign;
>  
>    Sign = ReverseFlag ? -1 : 1;
>  
>    for (i = 0; i < RegCount; i++) {
>      CtrlVal = 0;
> +    CtrlMask = 0;
>      for (j = 0; j < MPP_PINS_PER_REG; j++) {
> -      CtrlVal |= MPP_PIN_VAL(7 * (UINTN) ReverseFlag + j * Sign,
> -        MppRegPcd[i][7 * (UINTN) ReverseFlag + j * Sign]);
> +
> +      PinIndex = 7 * (UINTN)ReverseFlag + j * Sign;
> +
> +      if (MppRegPcd[i][PinIndex] != 0xff) {
> +        CtrlVal |= MPP_PIN_VAL(PinIndex, MppRegPcd[i][PinIndex]);
> +        CtrlMask |= MPP_PIN_VAL(PinIndex, 0xf);
> +      }
>      }
> -    MmioWrite32 (BaseAddr + 4 * i * Sign, CtrlVal);
> +    MmioAndThenOr32 (BaseAddr + 4 * i * Sign, ~CtrlMask, CtrlVal);
>    }
>  }
>  
> -- 
> 2.7.4
> 

[platforms: PATCH v2 07/10] Marvell/Drivers: Pp2Dxe: Change settings for the always-up link

[Marcin Wojtas]

Currently initial forcing link status happened for all ports, not only
marked as 'always-up'. Although this didn't actually matter for the MAC
settings, because MAC is automatically updated with PHY HW polling
feature of the controller, perform mv_gop110_fl_cfg only when
the appropriate flag is true. Also in such case, force the link as up,
using a new library routine.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c | 25 ++++++++++++++++++++
 Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h |  6 +++++
 Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   |  6 ++++-
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
index 53154db..c2d0199 100644
--- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
+++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
@@ -4804,6 +4804,31 @@ MvGop110PortEventsMask (
   return 0;
 }
 
+/*
+ * Sets "Force Link Pass" and "Do Not Force Link Fail" bits.
+ * This function should only be called when the port is disabled.
+ */
+VOID
+MvGop110GmacForceLinkModeSet(
+  IN PP2DXE_PORT *Port,
+  IN BOOLEAN LinkUp
+  )
+{
+  UINT32 RegVal;
+
+  RegVal = MvGop110GmacRead (Port, MVPP2_PORT_AUTO_NEG_CFG_REG);
+
+  if (LinkUp) {
+    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
+    RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;
+  } else {
+    RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
+    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;
+  }
+
+  MvGop110GmacWrite (Port, MVPP2_PORT_AUTO_NEG_CFG_REG, RegVal);
+}
+
 INT32
 MvGop110FlCfg (
   IN PP2DXE_PORT *Port
diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
index a7011f7..3ebe294 100644
--- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
+++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
@@ -504,6 +504,12 @@ MvGop110XlgPortLinkEventMask (
   IN PP2DXE_PORT *Port
   );
 
+VOID
+MvGop110GmacForceLinkModeSet (
+  IN PP2DXE_PORT *Port,
+  IN BOOLEAN LinkUp
+  );
+
 INT32
 MvGop110FlCfg (
   IN PP2DXE_PORT *Port
diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
index 2827976..4a1b9d5 100644
--- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
+++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
@@ -1310,7 +1310,11 @@ Pp2DxeInitialiseController (
     NetCompConfig |= MvpPp2xGop110NetcCfgCreate(&Pp2Context->Port);
 
     MvGop110PortInit(&Pp2Context->Port);
-    MvGop110FlCfg(&Pp2Context->Port);
+
+    if (Pp2Context->Port.AlwaysUp == TRUE) {
+      MvGop110GmacForceLinkModeSet (&Pp2Context->Port, TRUE);
+      MvGop110FlCfg (&Pp2Context->Port);
+    }
 
     Status = gBS->CreateEvent (
                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
-- 
2.7.4

Re: [platforms: PATCH v2 07/10] Marvell/Drivers: Pp2Dxe: Change settings for the always-up link

[Leif Lindholm]

On Fri, Oct 27, 2017 at 03:13:49AM +0200, Marcin Wojtas wrote:
> Currently initial forcing link status happened for all ports, not only
> marked as 'always-up'. Although this didn't actually matter for the MAC
> settings, because MAC is automatically updated with PHY HW polling
> feature of the controller, perform mv_gop110_fl_cfg only when
> the appropriate flag is true. Also in such case, force the link as up,
> using a new library routine.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c | 25 ++++++++++++++++++++
>  Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h |  6 +++++
>  Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   |  6 ++++-
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> index 53154db..c2d0199 100644
> --- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> +++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> @@ -4804,6 +4804,31 @@ MvGop110PortEventsMask (
>    return 0;
>  }
>  
> +/*
> + * Sets "Force Link Pass" and "Do Not Force Link Fail" bits.
> + * This function should only be called when the port is disabled.
> + */
> +VOID
> +MvGop110GmacForceLinkModeSet(
> +  IN PP2DXE_PORT *Port,
> +  IN BOOLEAN LinkUp
> +  )
> +{
> +  UINT32 RegVal;
> +
> +  RegVal = MvGop110GmacRead (Port, MVPP2_PORT_AUTO_NEG_CFG_REG);
> +
> +  if (LinkUp) {
> +    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
> +    RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;

So, I know UP and DOWN are separate bits (what's that about!?), but
unless you know that, the above two lines look like they're in the
wrong order.

If you flip those around (and make it like the 'else' clause):
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> +  } else {
> +    RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
> +    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;
> +  }
> +
> +  MvGop110GmacWrite (Port, MVPP2_PORT_AUTO_NEG_CFG_REG, RegVal);
> +}
> +
>  INT32
>  MvGop110FlCfg (
>    IN PP2DXE_PORT *Port
> diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
> index a7011f7..3ebe294 100644
> --- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
> +++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
> @@ -504,6 +504,12 @@ MvGop110XlgPortLinkEventMask (
>    IN PP2DXE_PORT *Port
>    );
>  
> +VOID
> +MvGop110GmacForceLinkModeSet (
> +  IN PP2DXE_PORT *Port,
> +  IN BOOLEAN LinkUp
> +  );
> +
>  INT32
>  MvGop110FlCfg (
>    IN PP2DXE_PORT *Port
> diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> index 2827976..4a1b9d5 100644
> --- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> +++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> @@ -1310,7 +1310,11 @@ Pp2DxeInitialiseController (
>      NetCompConfig |= MvpPp2xGop110NetcCfgCreate(&Pp2Context->Port);
>  
>      MvGop110PortInit(&Pp2Context->Port);
> -    MvGop110FlCfg(&Pp2Context->Port);
> +
> +    if (Pp2Context->Port.AlwaysUp == TRUE) {
> +      MvGop110GmacForceLinkModeSet (&Pp2Context->Port, TRUE);
> +      MvGop110FlCfg (&Pp2Context->Port);
> +    }
>  
>      Status = gBS->CreateEvent (
>                   EVT_SIGNAL_EXIT_BOOT_SERVICES,
> -- 
> 2.7.4
> 

Re: [platforms: PATCH v2 07/10] Marvell/Drivers: Pp2Dxe: Change settings for the always-up link

[Marcin Wojtas]

2017-10-27 16:36 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>
> On Fri, Oct 27, 2017 at 03:13:49AM +0200, Marcin Wojtas wrote:
> > Currently initial forcing link status happened for all ports, not only
> > marked as 'always-up'. Although this didn't actually matter for the MAC
> > settings, because MAC is automatically updated with PHY HW polling
> > feature of the controller, perform mv_gop110_fl_cfg only when
> > the appropriate flag is true. Also in such case, force the link as up,
> > using a new library routine.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c | 25 ++++++++++++++++++++
> >  Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h |  6 +++++
> >  Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   |  6 ++++-
> >  3 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> > index 53154db..c2d0199 100644
> > --- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> > +++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> > @@ -4804,6 +4804,31 @@ MvGop110PortEventsMask (
> >    return 0;
> >  }
> >
> > +/*
> > + * Sets "Force Link Pass" and "Do Not Force Link Fail" bits.
> > + * This function should only be called when the port is disabled.
> > + */
> > +VOID
> > +MvGop110GmacForceLinkModeSet(
> > +  IN PP2DXE_PORT *Port,
> > +  IN BOOLEAN LinkUp
> > +  )
> > +{
> > +  UINT32 RegVal;
> > +
> > +  RegVal = MvGop110GmacRead (Port, MVPP2_PORT_AUTO_NEG_CFG_REG);
> > +
> > +  if (LinkUp) {
> > +    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
> > +    RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;
>
> So, I know UP and DOWN are separate bits (what's that about!?), but
> unless you know that, the above two lines look like they're in the
> wrong order.
>

There is a 3rd case that if both are set to '1', autonegotiation is
forced, but we don't use it. I can even more simplify according to the
usecase (only 1 call in Pp2Dxe driver):

VOID
MvGop110GmacForceLinkUp(
  IN PP2DXE_PORT *Port
  )
{
  UINT32 RegVal;

  RegVal = MvGop110GmacRead (Port, MVPP2_PORT_AUTO_NEG_CFG_REG);

  RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
  RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;

  MvGop110GmacWrite (Port, MVPP2_PORT_AUTO_NEG_CFG_REG, RegVal);
}

What do you think?

> If you flip those around (and make it like the 'else' clause):

Ok.

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> > +  } else {
> > +    RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
> > +    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;
> > +  }
> > +
> > +  MvGop110GmacWrite (Port, MVPP2_PORT_AUTO_NEG_CFG_REG, RegVal);
> > +}
> > +
> >  INT32
> >  MvGop110FlCfg (
> >    IN PP2DXE_PORT *Port
> > diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
> > index a7011f7..3ebe294 100644
> > --- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
> > +++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
> > @@ -504,6 +504,12 @@ MvGop110XlgPortLinkEventMask (
> >    IN PP2DXE_PORT *Port
> >    );
> >
> > +VOID
> > +MvGop110GmacForceLinkModeSet (
> > +  IN PP2DXE_PORT *Port,
> > +  IN BOOLEAN LinkUp
> > +  );
> > +
> >  INT32
> >  MvGop110FlCfg (
> >    IN PP2DXE_PORT *Port
> > diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> > index 2827976..4a1b9d5 100644
> > --- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> > +++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> > @@ -1310,7 +1310,11 @@ Pp2DxeInitialiseController (
> >      NetCompConfig |= MvpPp2xGop110NetcCfgCreate(&Pp2Context->Port);
> >
> >      MvGop110PortInit(&Pp2Context->Port);
> > -    MvGop110FlCfg(&Pp2Context->Port);
> > +
> > +    if (Pp2Context->Port.AlwaysUp == TRUE) {
> > +      MvGop110GmacForceLinkModeSet (&Pp2Context->Port, TRUE);
> > +      MvGop110FlCfg (&Pp2Context->Port);
> > +    }
> >
> >      Status = gBS->CreateEvent (
> >                   EVT_SIGNAL_EXIT_BOOT_SERVICES,
> > --
> > 2.7.4
> >

[platforms: PATCH v2 08/10] Marvell/Drivers: XenonDxe: Fix UHS signalling mode setting

[Marcin Wojtas]

This patch fixes incorrect settings for UHS mode in
SD_MMC_HC_HOST_CTRL2 register for SDR50 and SDR25, of which
the latter was missing. This field should be set to:

0x4 for DDR52
0x2 for SDR50
0x1 for SDR25
0x0 for others.

This way EmmcSwitchToHighSpeed function is on par with Linux
set_uhs_signaling routine in the Xenon driver.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
index 3f73194..4d4833f 100755
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
@@ -772,6 +772,8 @@ EmmcSwitchToHighSpeed (
   if (IsDdr) {
     HostCtrl2 = BIT2;
   } else if (ClockFreq == 52) {
+    HostCtrl2 = BIT1;
+  } else if (ClockFreq == 26) {
     HostCtrl2 = BIT0;
   } else {
     HostCtrl2 = 0;
-- 
2.7.4

Re: [platforms: PATCH v2 08/10] Marvell/Drivers: XenonDxe: Fix UHS signalling mode setting

[Leif Lindholm]

On Fri, Oct 27, 2017 at 03:13:50AM +0200, Marcin Wojtas wrote:
> This patch fixes incorrect settings for UHS mode in
> SD_MMC_HC_HOST_CTRL2 register for SDR50 and SDR25, of which
> the latter was missing. This field should be set to:
> 
> 0x4 for DDR52
> 0x2 for SDR50
> 0x1 for SDR25
> 0x0 for others.
> 
> This way EmmcSwitchToHighSpeed function is on par with Linux
> set_uhs_signaling routine in the Xenon driver.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
> index 3f73194..4d4833f 100755
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
> @@ -772,6 +772,8 @@ EmmcSwitchToHighSpeed (
>    if (IsDdr) {
>      HostCtrl2 = BIT2;
>    } else if (ClockFreq == 52) {
> +    HostCtrl2 = BIT1;
> +  } else if (ClockFreq == 26) {
>      HostCtrl2 = BIT0;
>    } else {
>      HostCtrl2 = 0;
> -- 
> 2.7.4
> 

[platforms: PATCH v2 09/10] Marvell/Drivers: XenonDxe: Allow overriding base clock frequency

[Marcin Wojtas]

Some SdMmc host controllers are run by clocks with different
frequency than it is reflected in Capabilities Register 1.
Because the bitfield is only 8 bits wide, a maximum value
that could be obtained from hardware is 255(MHz).

In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq
member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient
to be used for setting the clock speed in SdMmcHcClockSupply
function.

This patch adds new UINT32 array ('BaseClkFreq[]') to
SD_MMC_HC_PRIVATE_DATA structure for specifying
the input clock speed for each slot of the host controller.
All routines that are used for clock configuration are
updated accordingly.

Thanks to above the Xenon host controller driver
could be modified to configure clock speed relatively
to actual 400MHz input.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c    |  4 ++--
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c      |  4 ++--
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c | 13 ++++++++----
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h |  6 ++++++
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c   | 22 ++++++++++----------
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h   | 12 ++++++-----
 6 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
index 4d4833f..530a01c 100755
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
@@ -705,7 +705,7 @@ EmmcSwitchClockFreq (
   //
   // Convert the clock freq unit from MHz to KHz.
   //
-  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]);
+  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
 
   return Status;
 }
@@ -1007,7 +1007,7 @@ EmmcSetBusMode (
     return Status;
   }
 
-  ASSERT (Private->Capability[Slot].BaseClkFreq != 0);
+  ASSERT (Private->BaseClkFreq[Slot] != 0);
   //
   // Check if the Host Controller support 8bits bus width.
   //
diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
index 9122848..ea7eed7 100644
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
@@ -972,7 +972,7 @@ SdCardSetBusMode (
     return Status;
   }
 
-  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
+  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -1144,7 +1144,7 @@ SdCardIdentification (
         goto Error;
       }
 
-      SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]);
+      SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
 
       gBS->Stall (1000);
 
diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
index 981eab5..10e15c5 100644
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
@@ -291,7 +291,10 @@ SdMmcPciHcEnumerateDevice (
         //
         // Reinitialize slot and restart identification process for the new attached device
         //
-        Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
+        Status = SdMmcHcInitHost (Private->PciIo,
+                   Slot,
+                   Private->Capability[Slot],
+                   Private->BaseClkFreq[Slot]);
         if (EFI_ERROR (Status)) {
           continue;
         }
@@ -617,9 +620,11 @@ SdMmcPciHcDriverBindingStart (
   Private->Capability[Slot].Sdr50 = 0;
   Private->Capability[Slot].BusWidth8 = 0;
 
-  if (Private->Capability[Slot].BaseClkFreq == 0) {
-    Private->Capability[Slot].BaseClkFreq = 0xff;
-  }
+  //
+  // Override inappropriate base clock frequency from Capabilities Register 1.
+  // Actual clock speed of Xenon controller is 400MHz.
+  //
+  Private->BaseClkFreq[Slot] = XENON_MMC_MAX_CLK / 1000 / 1000;
 
   DumpCapabilityReg (Slot, &Private->Capability[Slot]);
 
diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
index 6a2a279..067b9ac 100644
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
@@ -115,6 +115,12 @@ typedef struct {
   UINT64                              MaxCurrent[SD_MMC_HC_MAX_SLOT];
 
   UINT32                              ControllerVersion;
+
+  //
+  // Some controllers may require to override base clock frequency
+  // value stored in Capabilities Register 1.
+  //
+  UINT32                              BaseClkFreq[SD_MMC_HC_MAX_SLOT];
 } SD_MMC_HC_PRIVATE_DATA;
 
 #define SD_MMC_HC_TRB_SIG             SIGNATURE_32 ('T', 'R', 'B', 'T')
diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
index ccbf355..706618d 100644
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
@@ -678,7 +678,7 @@ SdMmcHcStopClock (
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -689,11 +689,10 @@ SdMmcHcClockSupply (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
   IN UINT64                 ClockFreq,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   )
 {
   EFI_STATUS                Status;
-  UINT32                    BaseClkFreq;
   UINT32                    SettingFreq;
   UINT32                    Divisor;
   UINT32                    Remainder;
@@ -703,9 +702,8 @@ SdMmcHcClockSupply (
   //
   // Calculate a divisor for SD clock frequency
   //
-  ASSERT (Capability.BaseClkFreq != 0);
+  ASSERT (BaseClkFreq != 0);
 
-  BaseClkFreq = Capability.BaseClkFreq;
   if (ClockFreq == 0) {
     return EFI_INVALID_PARAMETER;
   }
@@ -896,7 +894,7 @@ SdMmcHcSetBusWidth (
 
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -906,7 +904,7 @@ EFI_STATUS
 SdMmcHcInitClockFreq (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   )
 {
   EFI_STATUS                Status;
@@ -915,7 +913,7 @@ SdMmcHcInitClockFreq (
   //
   // Calculate a divisor for SD clock frequency
   //
-  if (Capability.BaseClkFreq == 0) {
+  if (BaseClkFreq == 0) {
     //
     // Don't support get Base Clock Frequency information via another method
     //
@@ -925,7 +923,7 @@ SdMmcHcInitClockFreq (
   // Supply 400KHz clock frequency at initialization phase.
   //
   InitFreq = 400;
-  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability);
+  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq);
   return Status;
 }
 
@@ -1024,6 +1022,7 @@ SdMmcHcInitTimeoutCtrl (
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The host controller is initialized successfully.
   @retval Others            The host controller isn't initialized successfully.
@@ -1033,12 +1032,13 @@ EFI_STATUS
 SdMmcHcInitHost (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN SD_MMC_HC_SLOT_CAP     Capability,
+  IN UINT32                 BaseClkFreq
   )
 {
   EFI_STATUS       Status;
 
-  Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
+  Status = SdMmcHcInitClockFreq (PciIo, Slot, BaseClkFreq);
   if (EFI_ERROR (Status)) {
     return Status;
   }
diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
index fb62758..a4ec4fe 100644
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
@@ -414,7 +414,7 @@ SdMmcHcStopClock (
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -425,7 +425,7 @@ SdMmcHcClockSupply (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
   IN UINT64                 ClockFreq,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClockFreq
   );
 
 /**
@@ -473,7 +473,7 @@ SdMmcHcSetBusWidth (
 
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -483,7 +483,7 @@ EFI_STATUS
 SdMmcHcInitClockFreq (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClockFreq
   );
 
 /**
@@ -531,6 +531,7 @@ SdMmcHcInitTimeoutCtrl (
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The host controller is initialized successfully.
   @retval Others            The host controller isn't initialized successfully.
@@ -540,7 +541,8 @@ EFI_STATUS
 SdMmcHcInitHost (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN SD_MMC_HC_SLOT_CAP     Capability,
+  IN UINT32                 BaseClockFreq
   );
 
 #endif
-- 
2.7.4

Re: [platforms: PATCH v2 09/10] Marvell/Drivers: XenonDxe: Allow overriding base clock frequency

[Leif Lindholm]

On Fri, Oct 27, 2017 at 03:13:51AM +0200, Marcin Wojtas wrote:
> Some SdMmc host controllers are run by clocks with different
> frequency than it is reflected in Capabilities Register 1.
> Because the bitfield is only 8 bits wide, a maximum value
> that could be obtained from hardware is 255(MHz).
> 
> In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq
> member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient
> to be used for setting the clock speed in SdMmcHcClockSupply
> function.
> 
> This patch adds new UINT32 array ('BaseClkFreq[]') to
> SD_MMC_HC_PRIVATE_DATA structure for specifying
> the input clock speed for each slot of the host controller.
> All routines that are used for clock configuration are
> updated accordingly.
> 
> Thanks to above the Xenon host controller driver
> could be modified to configure clock speed relatively
> to actual 400MHz input.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c    |  4 ++--
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c      |  4 ++--
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c | 13 ++++++++----
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h |  6 ++++++
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c   | 22 ++++++++++----------
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h   | 12 ++++++-----
>  6 files changed, 37 insertions(+), 24 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
> index 4d4833f..530a01c 100755
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
> @@ -705,7 +705,7 @@ EmmcSwitchClockFreq (
>    //
>    // Convert the clock freq unit from MHz to KHz.
>    //
> -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]);
> +  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
>  
>    return Status;
>  }
> @@ -1007,7 +1007,7 @@ EmmcSetBusMode (
>      return Status;
>    }
>  
> -  ASSERT (Private->Capability[Slot].BaseClkFreq != 0);
> +  ASSERT (Private->BaseClkFreq[Slot] != 0);
>    //
>    // Check if the Host Controller support 8bits bus width.
>    //
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
> index 9122848..ea7eed7 100644
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
> @@ -972,7 +972,7 @@ SdCardSetBusMode (
>      return Status;
>    }
>  
> -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
> +  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -1144,7 +1144,7 @@ SdCardIdentification (
>          goto Error;
>        }
>  
> -      SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]);
> +      SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
>  
>        gBS->Stall (1000);
>  
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
> index 981eab5..10e15c5 100644
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
> @@ -291,7 +291,10 @@ SdMmcPciHcEnumerateDevice (
>          //
>          // Reinitialize slot and restart identification process for the new attached device
>          //
> -        Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
> +        Status = SdMmcHcInitHost (Private->PciIo,
> +                   Slot,
> +                   Private->Capability[Slot],
> +                   Private->BaseClkFreq[Slot]);
>          if (EFI_ERROR (Status)) {
>            continue;
>          }
> @@ -617,9 +620,11 @@ SdMmcPciHcDriverBindingStart (
>    Private->Capability[Slot].Sdr50 = 0;
>    Private->Capability[Slot].BusWidth8 = 0;
>  
> -  if (Private->Capability[Slot].BaseClkFreq == 0) {
> -    Private->Capability[Slot].BaseClkFreq = 0xff;
> -  }
> +  //
> +  // Override inappropriate base clock frequency from Capabilities Register 1.
> +  // Actual clock speed of Xenon controller is 400MHz.
> +  //
> +  Private->BaseClkFreq[Slot] = XENON_MMC_MAX_CLK / 1000 / 1000;
>  
>    DumpCapabilityReg (Slot, &Private->Capability[Slot]);
>  
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
> index 6a2a279..067b9ac 100644
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
> @@ -115,6 +115,12 @@ typedef struct {
>    UINT64                              MaxCurrent[SD_MMC_HC_MAX_SLOT];
>  
>    UINT32                              ControllerVersion;
> +
> +  //
> +  // Some controllers may require to override base clock frequency
> +  // value stored in Capabilities Register 1.
> +  //
> +  UINT32                              BaseClkFreq[SD_MMC_HC_MAX_SLOT];
>  } SD_MMC_HC_PRIVATE_DATA;
>  
>  #define SD_MMC_HC_TRB_SIG             SIGNATURE_32 ('T', 'R', 'B', 'T')
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> index ccbf355..706618d 100644
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c

If doing this rework, there should probably be an addition to
DumpCapabilityReg to at least point out that Capability->BaseClkFreq
is ignored, rather than just printing the (now unused) value.

Otherwise, this looks sort of OK.

/
    Leif

> @@ -678,7 +678,7 @@ SdMmcHcStopClock (
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
>    @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
> -  @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>  
>    @retval EFI_SUCCESS       The clock is supplied successfully.
>    @retval Others            The clock isn't supplied successfully.
> @@ -689,11 +689,10 @@ SdMmcHcClockSupply (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
>    IN UINT64                 ClockFreq,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN UINT32                 BaseClkFreq
>    )
>  {
>    EFI_STATUS                Status;
> -  UINT32                    BaseClkFreq;
>    UINT32                    SettingFreq;
>    UINT32                    Divisor;
>    UINT32                    Remainder;
> @@ -703,9 +702,8 @@ SdMmcHcClockSupply (
>    //
>    // Calculate a divisor for SD clock frequency
>    //
> -  ASSERT (Capability.BaseClkFreq != 0);
> +  ASSERT (BaseClkFreq != 0);
>  
> -  BaseClkFreq = Capability.BaseClkFreq;
>    if (ClockFreq == 0) {
>      return EFI_INVALID_PARAMETER;
>    }
> @@ -896,7 +894,7 @@ SdMmcHcSetBusWidth (
>  
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
> -  @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>  
>    @retval EFI_SUCCESS       The clock is supplied successfully.
>    @retval Others            The clock isn't supplied successfully.
> @@ -906,7 +904,7 @@ EFI_STATUS
>  SdMmcHcInitClockFreq (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN UINT32                 BaseClkFreq
>    )
>  {
>    EFI_STATUS                Status;
> @@ -915,7 +913,7 @@ SdMmcHcInitClockFreq (
>    //
>    // Calculate a divisor for SD clock frequency
>    //
> -  if (Capability.BaseClkFreq == 0) {
> +  if (BaseClkFreq == 0) {
>      //
>      // Don't support get Base Clock Frequency information via another method
>      //
> @@ -925,7 +923,7 @@ SdMmcHcInitClockFreq (
>    // Supply 400KHz clock frequency at initialization phase.
>    //
>    InitFreq = 400;
> -  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability);
> +  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq);
>    return Status;
>  }
>  
> @@ -1024,6 +1022,7 @@ SdMmcHcInitTimeoutCtrl (
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
>    @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>  
>    @retval EFI_SUCCESS       The host controller is initialized successfully.
>    @retval Others            The host controller isn't initialized successfully.
> @@ -1033,12 +1032,13 @@ EFI_STATUS
>  SdMmcHcInitHost (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN SD_MMC_HC_SLOT_CAP     Capability,
> +  IN UINT32                 BaseClkFreq
>    )
>  {
>    EFI_STATUS       Status;
>  
> -  Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
> +  Status = SdMmcHcInitClockFreq (PciIo, Slot, BaseClkFreq);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
> index fb62758..a4ec4fe 100644
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
> @@ -414,7 +414,7 @@ SdMmcHcStopClock (
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
>    @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
> -  @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>  
>    @retval EFI_SUCCESS       The clock is supplied successfully.
>    @retval Others            The clock isn't supplied successfully.
> @@ -425,7 +425,7 @@ SdMmcHcClockSupply (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
>    IN UINT64                 ClockFreq,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN UINT32                 BaseClockFreq
>    );
>  
>  /**
> @@ -473,7 +473,7 @@ SdMmcHcSetBusWidth (
>  
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
> -  @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>  
>    @retval EFI_SUCCESS       The clock is supplied successfully.
>    @retval Others            The clock isn't supplied successfully.
> @@ -483,7 +483,7 @@ EFI_STATUS
>  SdMmcHcInitClockFreq (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN UINT32                 BaseClockFreq
>    );
>  
>  /**
> @@ -531,6 +531,7 @@ SdMmcHcInitTimeoutCtrl (
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
>    @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>  
>    @retval EFI_SUCCESS       The host controller is initialized successfully.
>    @retval Others            The host controller isn't initialized successfully.
> @@ -540,7 +541,8 @@ EFI_STATUS
>  SdMmcHcInitHost (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN SD_MMC_HC_SLOT_CAP     Capability,
> +  IN UINT32                 BaseClockFreq
>    );
>  
>  #endif
> -- 
> 2.7.4
> 

Re: [platforms: PATCH v2 09/10] Marvell/Drivers: XenonDxe: Allow overriding base clock frequency

[Marcin Wojtas]

2017-10-27 16:51 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Oct 27, 2017 at 03:13:51AM +0200, Marcin Wojtas wrote:
>> Some SdMmc host controllers are run by clocks with different
>> frequency than it is reflected in Capabilities Register 1.
>> Because the bitfield is only 8 bits wide, a maximum value
>> that could be obtained from hardware is 255(MHz).
>>
>> In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq
>> member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient
>> to be used for setting the clock speed in SdMmcHcClockSupply
>> function.
>>
>> This patch adds new UINT32 array ('BaseClkFreq[]') to
>> SD_MMC_HC_PRIVATE_DATA structure for specifying
>> the input clock speed for each slot of the host controller.
>> All routines that are used for clock configuration are
>> updated accordingly.
>>
>> Thanks to above the Xenon host controller driver
>> could be modified to configure clock speed relatively
>> to actual 400MHz input.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c    |  4 ++--
>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c      |  4 ++--
>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c | 13 ++++++++----
>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h |  6 ++++++
>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c   | 22 ++++++++++----------
>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h   | 12 ++++++-----
>>  6 files changed, 37 insertions(+), 24 deletions(-)
>>
>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
>> index 4d4833f..530a01c 100755
>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c
>> @@ -705,7 +705,7 @@ EmmcSwitchClockFreq (
>>    //
>>    // Convert the clock freq unit from MHz to KHz.
>>    //
>> -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]);
>> +  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
>>
>>    return Status;
>>  }
>> @@ -1007,7 +1007,7 @@ EmmcSetBusMode (
>>      return Status;
>>    }
>>
>> -  ASSERT (Private->Capability[Slot].BaseClkFreq != 0);
>> +  ASSERT (Private->BaseClkFreq[Slot] != 0);
>>    //
>>    // Check if the Host Controller support 8bits bus width.
>>    //
>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
>> index 9122848..ea7eed7 100644
>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c
>> @@ -972,7 +972,7 @@ SdCardSetBusMode (
>>      return Status;
>>    }
>>
>> -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
>> +  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
>>    if (EFI_ERROR (Status)) {
>>      return Status;
>>    }
>> @@ -1144,7 +1144,7 @@ SdCardIdentification (
>>          goto Error;
>>        }
>>
>> -      SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]);
>> +      SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
>>
>>        gBS->Stall (1000);
>>
>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
>> index 981eab5..10e15c5 100644
>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c
>> @@ -291,7 +291,10 @@ SdMmcPciHcEnumerateDevice (
>>          //
>>          // Reinitialize slot and restart identification process for the new attached device
>>          //
>> -        Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
>> +        Status = SdMmcHcInitHost (Private->PciIo,
>> +                   Slot,
>> +                   Private->Capability[Slot],
>> +                   Private->BaseClkFreq[Slot]);
>>          if (EFI_ERROR (Status)) {
>>            continue;
>>          }
>> @@ -617,9 +620,11 @@ SdMmcPciHcDriverBindingStart (
>>    Private->Capability[Slot].Sdr50 = 0;
>>    Private->Capability[Slot].BusWidth8 = 0;
>>
>> -  if (Private->Capability[Slot].BaseClkFreq == 0) {
>> -    Private->Capability[Slot].BaseClkFreq = 0xff;
>> -  }
>> +  //
>> +  // Override inappropriate base clock frequency from Capabilities Register 1.
>> +  // Actual clock speed of Xenon controller is 400MHz.
>> +  //
>> +  Private->BaseClkFreq[Slot] = XENON_MMC_MAX_CLK / 1000 / 1000;
>>
>>    DumpCapabilityReg (Slot, &Private->Capability[Slot]);
>>
>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
>> index 6a2a279..067b9ac 100644
>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h
>> @@ -115,6 +115,12 @@ typedef struct {
>>    UINT64                              MaxCurrent[SD_MMC_HC_MAX_SLOT];
>>
>>    UINT32                              ControllerVersion;
>> +
>> +  //
>> +  // Some controllers may require to override base clock frequency
>> +  // value stored in Capabilities Register 1.
>> +  //
>> +  UINT32                              BaseClkFreq[SD_MMC_HC_MAX_SLOT];
>>  } SD_MMC_HC_PRIVATE_DATA;
>>
>>  #define SD_MMC_HC_TRB_SIG             SIGNATURE_32 ('T', 'R', 'B', 'T')
>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> index ccbf355..706618d 100644
>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>
> If doing this rework, there should probably be an addition to
> DumpCapabilityReg to at least point out that Capability->BaseClkFreq
> is ignored, rather than just printing the (now unused) value.
>

Ok, I'll sort that out.

> Otherwise, this looks sort of OK.
>

Great, thanks.
Marcin

> /
>     Leif
>
>> @@ -678,7 +678,7 @@ SdMmcHcStopClock (
>>    @param[in] PciIo          The PCI IO protocol instance.
>>    @param[in] Slot           The slot number of the SD card to send the command to.
>>    @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
>> -  @param[in] Capability     The capability of the slot.
>> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>>
>>    @retval EFI_SUCCESS       The clock is supplied successfully.
>>    @retval Others            The clock isn't supplied successfully.
>> @@ -689,11 +689,10 @@ SdMmcHcClockSupply (
>>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>>    IN UINT8                  Slot,
>>    IN UINT64                 ClockFreq,
>> -  IN SD_MMC_HC_SLOT_CAP     Capability
>> +  IN UINT32                 BaseClkFreq
>>    )
>>  {
>>    EFI_STATUS                Status;
>> -  UINT32                    BaseClkFreq;
>>    UINT32                    SettingFreq;
>>    UINT32                    Divisor;
>>    UINT32                    Remainder;
>> @@ -703,9 +702,8 @@ SdMmcHcClockSupply (
>>    //
>>    // Calculate a divisor for SD clock frequency
>>    //
>> -  ASSERT (Capability.BaseClkFreq != 0);
>> +  ASSERT (BaseClkFreq != 0);
>>
>> -  BaseClkFreq = Capability.BaseClkFreq;
>>    if (ClockFreq == 0) {
>>      return EFI_INVALID_PARAMETER;
>>    }
>> @@ -896,7 +894,7 @@ SdMmcHcSetBusWidth (
>>
>>    @param[in] PciIo          The PCI IO protocol instance.
>>    @param[in] Slot           The slot number of the SD card to send the command to.
>> -  @param[in] Capability     The capability of the slot.
>> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>>
>>    @retval EFI_SUCCESS       The clock is supplied successfully.
>>    @retval Others            The clock isn't supplied successfully.
>> @@ -906,7 +904,7 @@ EFI_STATUS
>>  SdMmcHcInitClockFreq (
>>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>>    IN UINT8                  Slot,
>> -  IN SD_MMC_HC_SLOT_CAP     Capability
>> +  IN UINT32                 BaseClkFreq
>>    )
>>  {
>>    EFI_STATUS                Status;
>> @@ -915,7 +913,7 @@ SdMmcHcInitClockFreq (
>>    //
>>    // Calculate a divisor for SD clock frequency
>>    //
>> -  if (Capability.BaseClkFreq == 0) {
>> +  if (BaseClkFreq == 0) {
>>      //
>>      // Don't support get Base Clock Frequency information via another method
>>      //
>> @@ -925,7 +923,7 @@ SdMmcHcInitClockFreq (
>>    // Supply 400KHz clock frequency at initialization phase.
>>    //
>>    InitFreq = 400;
>> -  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability);
>> +  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq);
>>    return Status;
>>  }
>>
>> @@ -1024,6 +1022,7 @@ SdMmcHcInitTimeoutCtrl (
>>    @param[in] PciIo          The PCI IO protocol instance.
>>    @param[in] Slot           The slot number of the SD card to send the command to.
>>    @param[in] Capability     The capability of the slot.
>> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>>
>>    @retval EFI_SUCCESS       The host controller is initialized successfully.
>>    @retval Others            The host controller isn't initialized successfully.
>> @@ -1033,12 +1032,13 @@ EFI_STATUS
>>  SdMmcHcInitHost (
>>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>>    IN UINT8                  Slot,
>> -  IN SD_MMC_HC_SLOT_CAP     Capability
>> +  IN SD_MMC_HC_SLOT_CAP     Capability,
>> +  IN UINT32                 BaseClkFreq
>>    )
>>  {
>>    EFI_STATUS       Status;
>>
>> -  Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
>> +  Status = SdMmcHcInitClockFreq (PciIo, Slot, BaseClkFreq);
>>    if (EFI_ERROR (Status)) {
>>      return Status;
>>    }
>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
>> index fb62758..a4ec4fe 100644
>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h
>> @@ -414,7 +414,7 @@ SdMmcHcStopClock (
>>    @param[in] PciIo          The PCI IO protocol instance.
>>    @param[in] Slot           The slot number of the SD card to send the command to.
>>    @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
>> -  @param[in] Capability     The capability of the slot.
>> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>>
>>    @retval EFI_SUCCESS       The clock is supplied successfully.
>>    @retval Others            The clock isn't supplied successfully.
>> @@ -425,7 +425,7 @@ SdMmcHcClockSupply (
>>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>>    IN UINT8                  Slot,
>>    IN UINT64                 ClockFreq,
>> -  IN SD_MMC_HC_SLOT_CAP     Capability
>> +  IN UINT32                 BaseClockFreq
>>    );
>>
>>  /**
>> @@ -473,7 +473,7 @@ SdMmcHcSetBusWidth (
>>
>>    @param[in] PciIo          The PCI IO protocol instance.
>>    @param[in] Slot           The slot number of the SD card to send the command to.
>> -  @param[in] Capability     The capability of the slot.
>> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>>
>>    @retval EFI_SUCCESS       The clock is supplied successfully.
>>    @retval Others            The clock isn't supplied successfully.
>> @@ -483,7 +483,7 @@ EFI_STATUS
>>  SdMmcHcInitClockFreq (
>>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>>    IN UINT8                  Slot,
>> -  IN SD_MMC_HC_SLOT_CAP     Capability
>> +  IN UINT32                 BaseClockFreq
>>    );
>>
>>  /**
>> @@ -531,6 +531,7 @@ SdMmcHcInitTimeoutCtrl (
>>    @param[in] PciIo          The PCI IO protocol instance.
>>    @param[in] Slot           The slot number of the SD card to send the command to.
>>    @param[in] Capability     The capability of the slot.
>> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>>
>>    @retval EFI_SUCCESS       The host controller is initialized successfully.
>>    @retval Others            The host controller isn't initialized successfully.
>> @@ -540,7 +541,8 @@ EFI_STATUS
>>  SdMmcHcInitHost (
>>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>>    IN UINT8                  Slot,
>> -  IN SD_MMC_HC_SLOT_CAP     Capability
>> +  IN SD_MMC_HC_SLOT_CAP     Capability,
>> +  IN UINT32                 BaseClockFreq
>>    );
>>
>>  #endif
>> --
>> 2.7.4
>>

[platforms: PATCH v2 10/10] Marvell/Drivers: XenonDxe: Do not modify FIFO default values

[Marcin Wojtas]

Changing controller's FIFO default values is not necessary and
possibly can cause instabilities, when using some devices.
Disable the modification and rely on initial settings.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/Marvell/Drivers/SdMmc/XenonDxe/XenonSdhci.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/XenonSdhci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/XenonSdhci.c
index 31f207e..6bbe5bc 100755
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/XenonSdhci.c
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/XenonSdhci.c
@@ -44,20 +44,6 @@ XenonReadVersion (
   SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CTRL_VER, TRUE, SDHC_REG_SIZE_2B, ControllerVersion);
 }
 
-STATIC
-VOID
-XenonSetFifo (
-  IN EFI_PCI_IO_PROTOCOL   *PciIo
-  )
-{
-  UINTN Data;
-
-  // Set FIFO_RTC, FIFO_WTC, FIFO_CS and FIFO_PDLVMC
-  Data = SDHC_SLOT_FIFO_DEFAULT_CONFIG;
-
-  SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SDHC_SLOT_FIFO_CTRL, FALSE, SDHC_REG_SIZE_4B, &Data);
-}
-
 // Auto Clock Gating
 STATIC
 VOID
@@ -634,8 +620,6 @@ XenonInit (
   // Read XENON version
   XenonReadVersion (PciIo, &Private->ControllerVersion);
 
-  XenonSetFifo (PciIo);
-
   // Disable auto clock generator
   XenonSetAcg (PciIo, FALSE);
 
-- 
2.7.4