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

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

[Marcin Wojtas]

Hi,

This patchset contains small fixes and improvements in various drivers
(I2C, Network and SdMmc) and MppLib. As for the heads-up,
it will be followed by two significant series for the SPI and SdMmc,
before finally the files structure will be ready for reshuffling.

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

I'm looking forward to your comments or remarks.

Best regards,
Marcin

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: Fix base clock frequency
  Marvell/Drivers: XenonDxe: Do not modify FIFO default values

 Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c      | 42 +++++++++++++++-----
 Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h      |  2 +-
 Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c        | 31 +++++++++++++++
 Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h        |  7 ++++
 Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c          |  6 ++-
 Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c  |  2 +
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c |  4 +-
 Platform/Marvell/Drivers/SdMmc/XenonDxe/XenonSdhci.c  | 16 --------
 Platform/Marvell/Library/MppLib/MppLib.c              | 33 ++++++++-------
 Platform/Marvell/Library/MppLib/MppLib.inf            |  3 ++
 10 files changed, 98 insertions(+), 48 deletions(-)

-- 
2.7.4

[platforms: PATCH 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.

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 | 34 +++++++++++++++++---
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
index d85ee0b..7faf1f7 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,35 +575,58 @@ MvI2cStartRequest (
     ReadMode = Operation->Flags & I2C_FLAG_READ;
 
     if (Count == 0) {
-      MvI2cStart ( I2cMasterContext,
+      Status = MvI2cStart (I2cMasterContext,
                    (SlaveAddress << 1) | ReadMode,
                    I2C_TRANSFER_TIMEOUT
                  );
     } else if (!(Operation->Flags & I2C_FLAG_NORESTART)) {
-      MvI2cRepeatedStart ( I2cMasterContext,
+      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,
+      Status = MvI2cRead (I2cMasterContext,
                   Operation->Buffer,
                   Operation->LengthInBytes,
                   &Transmitted,
                   Count == 1,
                   I2C_TRANSFER_TIMEOUT
                  );
+      Operation->LengthInBytes = Transmitted;
     } else {
-      MvI2cWrite ( I2cMasterContext,
+      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 );
+      MvI2cStop (I2cMasterContext);
     }
   }
 
-- 
2.7.4

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:28AM +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.
> 
> 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 | 34 +++++++++++++++++---
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> index d85ee0b..7faf1f7 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,35 +575,58 @@ MvI2cStartRequest (
>      ReadMode = Operation->Flags & I2C_FLAG_READ;
>  
>      if (Count == 0) {
> -      MvI2cStart ( I2cMasterContext,
> +      Status = MvI2cStart (I2cMasterContext,
>                     (SlaveAddress << 1) | ReadMode,
>                     I2C_TRANSFER_TIMEOUT

Much as I appreciate seeing this form of the code, since it simplifies
seeing the functional changes, this does cause those lines left
unchanges to no longer conform to coding style.
Can you please adjust throughout for a v2?

>                   );
>      } else if (!(Operation->Flags & I2C_FLAG_NORESTART)) {
> -      MvI2cRepeatedStart ( I2cMasterContext,
> +      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,
> +      Status = MvI2cRead (I2cMasterContext,
>                    Operation->Buffer,
>                    Operation->LengthInBytes,
>                    &Transmitted,
>                    Count == 1,
>                    I2C_TRANSFER_TIMEOUT
>                   );
> +      Operation->LengthInBytes = Transmitted;
>      } else {
> -      MvI2cWrite ( I2cMasterContext,
> +      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 );
> +      MvI2cStop (I2cMasterContext);

Can you simply drop this non-functional change?
I'd prefer the non-adherence to coding-style over a misleading
history.

No objection to functional aspects of this patch.

/
    Leif

>      }
>    }
>  
> -- 
> 2.7.4
> 

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

[Marcin Wojtas]

Hi Leif,

2017-10-26 14:51 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 03:19:28AM +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.
>>
>> 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 | 34 +++++++++++++++++---
>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
>> index d85ee0b..7faf1f7 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,35 +575,58 @@ MvI2cStartRequest (
>>      ReadMode = Operation->Flags & I2C_FLAG_READ;
>>
>>      if (Count == 0) {
>> -      MvI2cStart ( I2cMasterContext,
>> +      Status = MvI2cStart (I2cMasterContext,
>>                     (SlaveAddress << 1) | ReadMode,
>>                     I2C_TRANSFER_TIMEOUT
>
> Much as I appreciate seeing this form of the code, since it simplifies
> seeing the functional changes, this does cause those lines left
> unchanges to no longer conform to coding style.
> Can you please adjust throughout for a v2?
>

No problem. I of course saw style violations, but I gave up on them
for "no mix of functional improvements and style cleanups" contraint
:) I will correct the modified function calls.

>>                   );
>>      } else if (!(Operation->Flags & I2C_FLAG_NORESTART)) {
>> -      MvI2cRepeatedStart ( I2cMasterContext,
>> +      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,
>> +      Status = MvI2cRead (I2cMasterContext,
>>                    Operation->Buffer,
>>                    Operation->LengthInBytes,
>>                    &Transmitted,
>>                    Count == 1,
>>                    I2C_TRANSFER_TIMEOUT
>>                   );
>> +      Operation->LengthInBytes = Transmitted;
>>      } else {
>> -      MvI2cWrite ( I2cMasterContext,
>> +      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 );
>> +      MvI2cStop (I2cMasterContext);
>
> Can you simply drop this non-functional change?
> I'd prefer the non-adherence to coding-style over a misleading
> history.
>

Right, I saw it after sending - I was cleaning dirty patch and
splitting into 3, this line got here by mistake.

> No objection to functional aspects of this patch.

Ok, thanks!

Marcin

>
> /
>     Leif
>
>>      }
>>    }
>>
>> --
>> 2.7.4
>>

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:36PM +0200, Marcin Wojtas wrote:
> Hi Leif,
> 
> 2017-10-26 14:51 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Thu, Oct 26, 2017 at 03:19:28AM +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.
> >>
> >> 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 | 34 +++++++++++++++++---
> >>  1 file changed, 29 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> >> index d85ee0b..7faf1f7 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,35 +575,58 @@ MvI2cStartRequest (
> >>      ReadMode = Operation->Flags & I2C_FLAG_READ;
> >>
> >>      if (Count == 0) {
> >> -      MvI2cStart ( I2cMasterContext,
> >> +      Status = MvI2cStart (I2cMasterContext,
> >>                     (SlaveAddress << 1) | ReadMode,
> >>                     I2C_TRANSFER_TIMEOUT
> >
> > Much as I appreciate seeing this form of the code, since it simplifies
> > seeing the functional changes, this does cause those lines left
> > unchanges to no longer conform to coding style.
> > Can you please adjust throughout for a v2?
> >
> 
> No problem. I of course saw style violations, but I gave up on them
> for "no mix of functional improvements and style cleanups" contraint
> :) I will correct the modified function calls.

Clarification: this is and has always been _unrelated_ style cleanups.
Any statement that is actually being modified should be conformant
afterwards.

Thanks.

/
    Leif

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

[Marcin Wojtas]

2017-10-26 15:54 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 03:19:36PM +0200, Marcin Wojtas wrote:
>> Hi Leif,
>>
>> 2017-10-26 14:51 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> > On Thu, Oct 26, 2017 at 03:19:28AM +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.
>> >>
>> >> 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 | 34 +++++++++++++++++---
>> >>  1 file changed, 29 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
>> >> index d85ee0b..7faf1f7 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,35 +575,58 @@ MvI2cStartRequest (
>> >>      ReadMode = Operation->Flags & I2C_FLAG_READ;
>> >>
>> >>      if (Count == 0) {
>> >> -      MvI2cStart ( I2cMasterContext,
>> >> +      Status = MvI2cStart (I2cMasterContext,
>> >>                     (SlaveAddress << 1) | ReadMode,
>> >>                     I2C_TRANSFER_TIMEOUT
>> >
>> > Much as I appreciate seeing this form of the code, since it simplifies
>> > seeing the functional changes, this does cause those lines left
>> > unchanges to no longer conform to coding style.
>> > Can you please adjust throughout for a v2?
>> >
>>
>> No problem. I of course saw style violations, but I gave up on them
>> for "no mix of functional improvements and style cleanups" contraint
>> :) I will correct the modified function calls.
>
> Clarification: this is and has always been _unrelated_ style cleanups.
> Any statement that is actually being modified should be conformant
> afterwards.
>

Ok, thanks for clarification.

Marcin

[platforms: PATCH 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 7faf1f7..8ed96f0 100755
--- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
+++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
@@ -631,7 +631,7 @@ MvI2cStartRequest (
   }
 
   if (I2cStatus != NULL)
-    I2cStatus = EFI_SUCCESS;
+    *I2cStatus = Status;
   if (Event != NULL)
     gBS->SignalEvent(Event);
   return EFI_SUCCESS;
-- 
2.7.4

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:29AM +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>
> ---
>  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 7faf1f7..8ed96f0 100755
> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> @@ -631,7 +631,7 @@ MvI2cStartRequest (
>    }
>  
>    if (I2cStatus != NULL)
> -    I2cStatus = EFI_SUCCESS;
> +    *I2cStatus = Status;

Oh, that's horrible! And only did not generate warnings because
EFI_SUCCESS is 0.

However, you also change from setting *I2cStatus to Status instead of
EFI_SUCCESS. This should be mentioned in commit message.

/
    Leif

>    if (Event != NULL)
>      gBS->SignalEvent(Event);
>    return EFI_SUCCESS;
> -- 
> 2.7.4
> 

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

[Marcin Wojtas]

2017-10-26 15:11 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 03:19:29AM +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>
>> ---
>>  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 7faf1f7..8ed96f0 100755
>> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
>> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
>> @@ -631,7 +631,7 @@ MvI2cStartRequest (
>>    }
>>
>>    if (I2cStatus != NULL)
>> -    I2cStatus = EFI_SUCCESS;
>> +    *I2cStatus = Status;
>
> Oh, that's horrible! And only did not generate warnings because
> EFI_SUCCESS is 0.

This parameter is 'optional', so me must have been also lucky not to
use it in basic operation.

>
> However, you also change from setting *I2cStatus to Status instead of
> EFI_SUCCESS. This should be mentioned in commit message.
>
> /

We can leave EFI_SUCCESS, as it's the only option at this place. It
was changed to Status, due to first patch of the series. I will leave
the original and just modify *.

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:22:32PM +0200, Marcin Wojtas wrote:
> 2017-10-26 15:11 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Thu, Oct 26, 2017 at 03:19:29AM +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>
> >> ---
> >>  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 7faf1f7..8ed96f0 100755
> >> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> >> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> >> @@ -631,7 +631,7 @@ MvI2cStartRequest (
> >>    }
> >>
> >>    if (I2cStatus != NULL)
> >> -    I2cStatus = EFI_SUCCESS;
> >> +    *I2cStatus = Status;
> >
> > Oh, that's horrible! And only did not generate warnings because
> > EFI_SUCCESS is 0.
> 
> This parameter is 'optional', so me must have been also lucky not to
> use it in basic operation.
> 
> >
> > However, you also change from setting *I2cStatus to Status instead of
> > EFI_SUCCESS. This should be mentioned in commit message.
> >
> > /
> 
> We can leave EFI_SUCCESS, as it's the only option at this place. It
> was changed to Status, due to first patch of the series. I will leave
> the original and just modify *.

Yes, that works too - thanks!

/
    Leif

[platforms: PATCH 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>
---
 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 8ed96f0..3c26d18 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

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:30AM +0200, Marcin Wojtas wrote:
> 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.

See now why I keep nagging about explanations for delays? :)

> 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 8ed96f0..3c26d18 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 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>
---
 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

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:31AM +0200, Marcin Wojtas wrote:
> 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 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>
---
 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

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:32AM +0200, Marcin Wojtas wrote:
> 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.

It may. But it is also used by PlatInitDxe.
Can we use different build options for SEC and later phases?

/
    Leif

> 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.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
> 

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

[Ard Biesheuvel]

On 26 October 2017 at 14:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Oct 26, 2017 at 03:19:32AM +0200, Marcin Wojtas wrote:
>> 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.
>
> It may. But it is also used by PlatInitDxe.
> Can we use different build options for SEC and later phases?
>

No, libraries are only built a single time during the build, and
linked into every module that depends on them. This is the same issue
we had with -mstrict-align.


>
>> 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.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
>>

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 02:29:02PM +0100, Ard Biesheuvel wrote:
> On 26 October 2017 at 14:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Oct 26, 2017 at 03:19:32AM +0200, Marcin Wojtas wrote:
> >> 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.
> >
> > It may. But it is also used by PlatInitDxe.
> > Can we use different build options for SEC and later phases?
> 
> No, libraries are only built a single time during the build, and
> linked into every module that depends on them. This is the same issue
> we had with -mstrict-align.

Sure, but we could have duplicated the .inf and have a SEC version and
use that mapping for SEC...

Clearly that's tedious. I guess it comes down to how useful we think
the stack checking is in general. If the answer is "not very, to be
honest":
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> >> 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.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 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 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Platform/Marvell/Library/MppLib/MppLib.c b/Platform/Marvell/Library/MppLib/MppLib.c
index 383c820..9e42f08 100644
--- a/Platform/Marvell/Library/MppLib/MppLib.c
+++ b/Platform/Marvell/Library/MppLib/MppLib.c
@@ -79,18 +79,22 @@ SetRegisterValue (
   BOOLEAN ReverseFlag
   )
 {
-  UINT32 i, j, CtrlVal;
+  UINT32 i, j, CtrlVal, CtrlMask;
   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]);
+      if (MppRegPcd[i][7 * (UINTN) ReverseFlag + j * Sign] != 0xff) {
+        CtrlVal |= MPP_PIN_VAL(7 * (UINTN) ReverseFlag + j * Sign,
+          MppRegPcd[i][7 * (UINTN) ReverseFlag + j * Sign]);
+        CtrlMask |= MPP_PIN_VAL(7 * (UINTN) ReverseFlag + j * Sign, 0xf);
+      }
     }
-    MmioWrite32 (BaseAddr + 4 * i * Sign, CtrlVal);
+    MmioAndThenOr32 (BaseAddr + 4 * i * Sign, ~CtrlMask, CtrlVal);
   }
 }
 
-- 
2.7.4

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:33AM +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>
> ---
>  Platform/Marvell/Library/MppLib/MppLib.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Platform/Marvell/Library/MppLib/MppLib.c b/Platform/Marvell/Library/MppLib/MppLib.c
> index 383c820..9e42f08 100644
> --- a/Platform/Marvell/Library/MppLib/MppLib.c
> +++ b/Platform/Marvell/Library/MppLib/MppLib.c
> @@ -79,18 +79,22 @@ SetRegisterValue (
>    BOOLEAN ReverseFlag
>    )
>  {
> -  UINT32 i, j, CtrlVal;
> +  UINT32 i, j, CtrlVal, CtrlMask;
>    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]);
> +      if (MppRegPcd[i][7 * (UINTN) ReverseFlag + j * Sign] != 0xff) {

This addition makes an already messy situation worse.
Can we have a descriptively named temporary variable for
"7 * (UINTN) ReverseFlag + j * Sign"?

/
    Leif

> +        CtrlVal |= MPP_PIN_VAL(7 * (UINTN) ReverseFlag + j * Sign,
> +          MppRegPcd[i][7 * (UINTN) ReverseFlag + j * Sign]);
> +        CtrlMask |= MPP_PIN_VAL(7 * (UINTN) ReverseFlag + j * Sign, 0xf);
> +      }
>      }
> -    MmioWrite32 (BaseAddr + 4 * i * Sign, CtrlVal);
> +    MmioAndThenOr32 (BaseAddr + 4 * i * Sign, ~CtrlMask, CtrlVal);
>    }
>  }
>  
> -- 
> 2.7.4
> 

[platforms: PATCH 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 | 31 ++++++++++++++++++++
 Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h |  7 +++++
 Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   |  6 +++-
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
index 53154db..e3ddc58 100644
--- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
+++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
@@ -4804,6 +4804,37 @@ 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 ForceLinkUp,
+  IN BOOLEAN ForceLinkDown)
+{
+  UINT32 RegVal;
+
+  /* Can't force link pass and link fail at the same time */
+  if ((ForceLinkUp) && (ForceLinkDown))
+    return;
+
+  RegVal = MvGop110GmacRead (Port, MVPP2_PORT_AUTO_NEG_CFG_REG);
+
+  if (ForceLinkUp)
+    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
+  else
+    RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
+
+  if (ForceLinkDown)
+    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;
+  else
+    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..2938777 100644
--- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
+++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
@@ -504,6 +504,13 @@ MvGop110XlgPortLinkEventMask (
   IN PP2DXE_PORT *Port
   );
 
+VOID
+MvGop110GmacForceLinkModeSet (
+  IN PP2DXE_PORT *Port,
+  IN BOOLEAN ForceLinkUp,
+  IN BOOLEAN ForceLinkDown
+  );
+
 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..94a2988 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, FALSE);
+      MvGop110FlCfg (&Pp2Context->Port);
+    }
 
     Status = gBS->CreateEvent (
                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
-- 
2.7.4

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:34AM +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 | 31 ++++++++++++++++++++
>  Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h |  7 +++++
>  Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   |  6 +++-
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> index 53154db..e3ddc58 100644
> --- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> +++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.c
> @@ -4804,6 +4804,37 @@ 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 ForceLinkUp,
> +  IN BOOLEAN ForceLinkDown)
> +{
> +  UINT32 RegVal;
> +
> +  /* Can't force link pass and link fail at the same time */

Then why pass two parameters to the function?
The function is called ForceLink.
Have one ForceLinkUp BOOL parameter, and force up if TRUE and down if
FALSE.

> +  if ((ForceLinkUp) && (ForceLinkDown))
> +    return;
> +
> +  RegVal = MvGop110GmacRead (Port, MVPP2_PORT_AUTO_NEG_CFG_REG);
> +
> +  if (ForceLinkUp)

Always {} with if/else (throughout).

/
    Leif

> +    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
> +  else
> +    RegVal &= ~MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_UP_MASK;
> +
> +  if (ForceLinkDown)
> +    RegVal |= MVPP2_PORT_AUTO_NEG_CFG_FORCE_LINK_DOWN_MASK;
> +  else
> +    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..2938777 100644
> --- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
> +++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Mvpp2Lib.h
> @@ -504,6 +504,13 @@ MvGop110XlgPortLinkEventMask (
>    IN PP2DXE_PORT *Port
>    );
>  
> +VOID
> +MvGop110GmacForceLinkModeSet (
> +  IN PP2DXE_PORT *Port,
> +  IN BOOLEAN ForceLinkUp,
> +  IN BOOLEAN ForceLinkDown
> +  );
> +
>  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..94a2988 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, FALSE);
> +      MvGop110FlCfg (&Pp2Context->Port);
> +    }
>  
>      Status = gBS->CreateEvent (
>                   EVT_SIGNAL_EXIT_BOOT_SERVICES,
> -- 
> 2.7.4
> 

[platforms: PATCH 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. 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 08/10] Marvell/Drivers: XenonDxe: Fix UHS signalling mode setting

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:35AM +0200, Marcin Wojtas wrote:
> This patch fixes incorrect settings for UHS mode in
> SD_MMC_HC_HOST_CTRL2 register. 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) {

This patch adds previously missing handling of SDR25 (I guess).
Please reflect this in commit message.

/
    Leif

>      HostCtrl2 = BIT0;
>    } else {
>      HostCtrl2 = 0;
> -- 
> 2.7.4
> 

[platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Marcin Wojtas]

Incorrectly the clock divisor was calculated relatively
to 255MHz instead of actual 400MHz. Fix this.

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

diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
index ccbf355..0b9328b 100644
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
@@ -16,6 +16,7 @@
 **/
 
 #include "SdMmcPciHcDxe.h"
+#include "XenonSdhci.h"
 
 /**
   Dump the content of SD/MMC host controller's Capability Register.
@@ -703,9 +704,8 @@ SdMmcHcClockSupply (
   //
   // Calculate a divisor for SD clock frequency
   //
-  ASSERT (Capability.BaseClkFreq != 0);
 
-  BaseClkFreq = Capability.BaseClkFreq;
+  BaseClkFreq = XENON_MMC_MAX_CLK / 1000 / 1000;
   if (ClockFreq == 0) {
     return EFI_INVALID_PARAMETER;
   }
-- 
2.7.4

Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
> Incorrectly the clock divisor was calculated relatively
> to 255MHz instead of actual 400MHz.

This describes the specific symptom, not the problem with the existing
code.

> Fix this.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> index ccbf355..0b9328b 100644
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> @@ -16,6 +16,7 @@
>  **/
>  
>  #include "SdMmcPciHcDxe.h"
> +#include "XenonSdhci.h"
>  
>  /**
>    Dump the content of SD/MMC host controller's Capability Register.
> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>    //
>    // Calculate a divisor for SD clock frequency
>    //
> -  ASSERT (Capability.BaseClkFreq != 0);
>  
> -  BaseClkFreq = Capability.BaseClkFreq;

Why is Capability.BaseClkFreq the wrong frequency to use?

/
    Leif

> +  BaseClkFreq = XENON_MMC_MAX_CLK / 1000 / 1000;
>    if (ClockFreq == 0) {
>      return EFI_INVALID_PARAMETER;
>    }
> -- 
> 2.7.4
> 

Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Marcin Wojtas]

2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
>> Incorrectly the clock divisor was calculated relatively
>> to 255MHz instead of actual 400MHz.
>
> This describes the specific symptom, not the problem with the existing
> code.
>
>> Fix this.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> index ccbf355..0b9328b 100644
>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> @@ -16,6 +16,7 @@
>>  **/
>>
>>  #include "SdMmcPciHcDxe.h"
>> +#include "XenonSdhci.h"
>>
>>  /**
>>    Dump the content of SD/MMC host controller's Capability Register.
>> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>>    //
>>    // Calculate a divisor for SD clock frequency
>>    //
>> -  ASSERT (Capability.BaseClkFreq != 0);
>>
>> -  BaseClkFreq = Capability.BaseClkFreq;
>
> Why is Capability.BaseClkFreq the wrong frequency to use?
>

The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
An alternative would be change this generic type to UINT16 and update
field properly during initialization - do you prefer that?

Marcin

Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Ard Biesheuvel]

On 26 October 2017 at 14:54, Marcin Wojtas <mw@semihalf.com> wrote:
> 2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
>>> Incorrectly the clock divisor was calculated relatively
>>> to 255MHz instead of actual 400MHz.
>>
>> This describes the specific symptom, not the problem with the existing
>> code.
>>
>>> Fix this.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>> index ccbf355..0b9328b 100644
>>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>> @@ -16,6 +16,7 @@
>>>  **/
>>>
>>>  #include "SdMmcPciHcDxe.h"
>>> +#include "XenonSdhci.h"
>>>
>>>  /**
>>>    Dump the content of SD/MMC host controller's Capability Register.
>>> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>>>    //
>>>    // Calculate a divisor for SD clock frequency
>>>    //
>>> -  ASSERT (Capability.BaseClkFreq != 0);
>>>
>>> -  BaseClkFreq = Capability.BaseClkFreq;
>>
>> Why is Capability.BaseClkFreq the wrong frequency to use?
>>
>
> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
> An alternative would be change this generic type to UINT16 and update
> field properly during initialization - do you prefer that?
>

Isn't that value read from device registers?

Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Marcin Wojtas]

2017-10-26 15:55 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 26 October 2017 at 14:54, Marcin Wojtas <mw@semihalf.com> wrote:
>> 2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>>> On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
>>>> Incorrectly the clock divisor was calculated relatively
>>>> to 255MHz instead of actual 400MHz.
>>>
>>> This describes the specific symptom, not the problem with the existing
>>> code.
>>>
>>>> Fix this.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>>> ---
>>>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>>> index ccbf355..0b9328b 100644
>>>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>>> @@ -16,6 +16,7 @@
>>>>  **/
>>>>
>>>>  #include "SdMmcPciHcDxe.h"
>>>> +#include "XenonSdhci.h"
>>>>
>>>>  /**
>>>>    Dump the content of SD/MMC host controller's Capability Register.
>>>> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>>>>    //
>>>>    // Calculate a divisor for SD clock frequency
>>>>    //
>>>> -  ASSERT (Capability.BaseClkFreq != 0);
>>>>
>>>> -  BaseClkFreq = Capability.BaseClkFreq;
>>>
>>> Why is Capability.BaseClkFreq the wrong frequency to use?
>>>
>>
>> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
>> An alternative would be change this generic type to UINT16 and update
>> field properly during initialization - do you prefer that?
>>
>
> Isn't that value read from device registers?

This field in generic Capability1 register is only 8-bit wide, hence
the 255MHz limitation. Xenon controller however is fed with 400MHz and
it clearly cannot be obtained from there.

Marcin

Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:54:41PM +0200, Marcin Wojtas wrote:
> 2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
> >> Incorrectly the clock divisor was calculated relatively
> >> to 255MHz instead of actual 400MHz.
> >
> > This describes the specific symptom, not the problem with the existing
> > code.
> >
> >> Fix this.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> >> index ccbf355..0b9328b 100644
> >> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> >> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> >> @@ -16,6 +16,7 @@
> >>  **/
> >>
> >>  #include "SdMmcPciHcDxe.h"
> >> +#include "XenonSdhci.h"
> >>
> >>  /**
> >>    Dump the content of SD/MMC host controller's Capability Register.
> >> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
> >>    //
> >>    // Calculate a divisor for SD clock frequency
> >>    //
> >> -  ASSERT (Capability.BaseClkFreq != 0);
> >>
> >> -  BaseClkFreq = Capability.BaseClkFreq;
> >
> > Why is Capability.BaseClkFreq the wrong frequency to use?
> 
> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
> An alternative would be change this generic type to UINT16 and update
> field properly during initialization - do you prefer that?

No, I'm still dreaming we might be able to reintegrate this into the
MdeModulePkg driver in some glorious future.

So what you are basically saying is that this controller is running at
a higher frequency than is permitted (or even describable) by the
specification? If so, _that_ needs to be in the commit message (and
really, a comment by the code as well).

/
    Leif

Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Marcin Wojtas]

2017-10-26 16:02 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 03:54:41PM +0200, Marcin Wojtas wrote:
>> 2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> > On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
>> >> Incorrectly the clock divisor was calculated relatively
>> >> to 255MHz instead of actual 400MHz.
>> >
>> > This describes the specific symptom, not the problem with the existing
>> > code.
>> >
>> >> Fix this.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> >> ---
>> >>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> >> index ccbf355..0b9328b 100644
>> >> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> >> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> >> @@ -16,6 +16,7 @@
>> >>  **/
>> >>
>> >>  #include "SdMmcPciHcDxe.h"
>> >> +#include "XenonSdhci.h"
>> >>
>> >>  /**
>> >>    Dump the content of SD/MMC host controller's Capability Register.
>> >> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>> >>    //
>> >>    // Calculate a divisor for SD clock frequency
>> >>    //
>> >> -  ASSERT (Capability.BaseClkFreq != 0);
>> >>
>> >> -  BaseClkFreq = Capability.BaseClkFreq;
>> >
>> > Why is Capability.BaseClkFreq the wrong frequency to use?
>>
>> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
>> An alternative would be change this generic type to UINT16 and update
>> field properly during initialization - do you prefer that?
>
> No, I'm still dreaming we might be able to reintegrate this into the
> MdeModulePkg driver in some glorious future.

Yes, that would be great. I imagine some SDMMC_HOST_PROTOCOL exposing
callbacks to set UHS, custom clock handling, etc (like it's done in
the Linux).

>
> So what you are basically saying is that this controller is running at
> a higher frequency than is permitted (or even describable) by the
> specification? If so, _that_ needs to be in the commit message (and
> really, a comment by the code as well).

Yes, this clock value is Xenon controller's quirk. I will mention it
in commit log/comment, but please let know if you wish me to:
a. extend BaseClkFreq to UINT16 and configure it properly during init
b. leave as is with better description only
I lean towards a., it's a very low-cost quirk to be applied.

Best regards,
Marcin

Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Leif Lindholm]

On Thu, Oct 26, 2017 at 04:29:39PM +0200, Marcin Wojtas wrote:
> 2017-10-26 16:02 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> >> > Why is Capability.BaseClkFreq the wrong frequency to use?
> >>
> >> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
> >> An alternative would be change this generic type to UINT16 and update
> >> field properly during initialization - do you prefer that?
> >
> > No, I'm still dreaming we might be able to reintegrate this into the
> > MdeModulePkg driver in some glorious future.
> 
> Yes, that would be great. I imagine some SDMMC_HOST_PROTOCOL exposing
> callbacks to set UHS, custom clock handling, etc (like it's done in
> the Linux).

Yes, *waves hands*, something like that.

> > So what you are basically saying is that this controller is running at
> > a higher frequency than is permitted (or even describable) by the
> > specification? If so, _that_ needs to be in the commit message (and
> > really, a comment by the code as well).
> 
> Yes, this clock value is Xenon controller's quirk. I will mention it
> in commit log/comment, but please let know if you wish me to:
> a. extend BaseClkFreq to UINT16 and configure it properly during init
> b. leave as is with better description only
> I lean towards a., it's a very low-cost quirk to be applied.

I would actually lean towards b. That way it stands out and is less
likely to actually _confuse_ someone in the future.

/
    Leif

Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

[Marcin Wojtas]

2017-10-26 16:52 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 04:29:39PM +0200, Marcin Wojtas wrote:
>> 2017-10-26 16:02 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> >> > Why is Capability.BaseClkFreq the wrong frequency to use?
>> >>
>> >> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
>> >> An alternative would be change this generic type to UINT16 and update
>> >> field properly during initialization - do you prefer that?
>> >
>> > No, I'm still dreaming we might be able to reintegrate this into the
>> > MdeModulePkg driver in some glorious future.
>>
>> Yes, that would be great. I imagine some SDMMC_HOST_PROTOCOL exposing
>> callbacks to set UHS, custom clock handling, etc (like it's done in
>> the Linux).
>
> Yes, *waves hands*, something like that.

Just 'a bit' spare time needed.

>
>> > So what you are basically saying is that this controller is running at
>> > a higher frequency than is permitted (or even describable) by the
>> > specification? If so, _that_ needs to be in the commit message (and
>> > really, a comment by the code as well).
>>
>> Yes, this clock value is Xenon controller's quirk. I will mention it
>> in commit log/comment, but please let know if you wish me to:
>> a. extend BaseClkFreq to UINT16 and configure it properly during init
>> b. leave as is with better description only
>> I lean towards a., it's a very low-cost quirk to be applied.
>
> I would actually lean towards b. That way it stands out and is less
> likely to actually _confuse_ someone in the future.
>

If we dream about possible merge with edk2 original code, I'd really
suggest not to spoil (well, I'm criticizing my own patch:)) the
SdMmcHcClockSupply and continue to rely on BaseClkFreq field there.
This way the code can be common for various hosts.

Let's take look at linux - there is SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN
flag, thanks to which the host controller driver can override the
capability register value during initialization. Most common method
here ends up in clk_get_rate () call and this quirk is used by overall
10 different controllers. Modifying BaseClkFreq type and assignment
during driver initialization (if needed) will be IMO better for
maintenance than creating a custom version of SdMmcHcClockSupply
routine.

Is there any chance I might have convinced you?:)

Thanks,
Marcin

[platforms: PATCH 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>
---
 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

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

[Leif Lindholm]

On Thu, Oct 26, 2017 at 03:19:37AM +0200, Marcin Wojtas wrote:
> 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
>