[PATCH 0/3] PL011 updates.

[PATCH 0/3] PL011 updates.

[evan.lloyd]

From: Evan Lloyd <evan.lloyd@arm.com>

This patchset comprises two very minor fixes to the PL011 UART related
code, and some comment updates.  
The FIFO fix amounts to a performance improvement in limited circumstaces,
where some older platforms might not used the FIFO.
The UINTN cast fix preempts errors on future 32-bit platforms.

The code can be inspected at: https://github.com/EvanLloyd/tianocore/tree/pl011a_v1


Alexei (2):
  ArmPlatformPkg: Correct mendacious comments.
  ArmPlatformPkg: Remove UINTN cast when setting BaudRate.

Evan Lloyd (1):
  ArmPlatformPkg: Fix PL011 FIFO size test

 ArmPlatformPkg/Include/Drivers/PL011Uart.h                     |  5 ++---
 ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c                   | 16 +++++++--------
 ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 21 +++++++++-----------
 3 files changed, 19 insertions(+), 23 deletions(-)

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

[PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test

[evan.lloyd]

From: Evan Lloyd <evan.lloyd@arm.com>

This change updates PL011UartInitializePort to compare ReceiveFifoDepth
with the correct hardware FIFO size instead of the constant 32 used
previously.
This corrects a minor bug where a request for a fifo size > 15 and < 32
would not have been honoured on a system with a 16 byte FIFO.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
---
 ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
index 3748972acbfc80f1077b9b928756a72cc77b5c75..b3ea138bf60b93a1000dd29aedaad206f2d15f2b 100644
--- a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
+++ b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
@@ -79,17 +79,18 @@ PL011UartInitializePort (
   UINT32      Divisor;
   UINT32      Integer;
   UINT32      Fractional;
+  UINT32      HardwareFifoDepth;
 
+  HardwareFifoDepth = (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) \
+                       > PL011_VER_R1P4) \
+                      ? 32 : 16 ;
   // The PL011 supports a buffer of 1, 16 or 32 chars. Therefore we can accept
   // 1 char buffer as the minimum FIFO size. Because everything can be rounded
   // down, there is no maximum FIFO size.
-  if ((*ReceiveFifoDepth == 0) || (*ReceiveFifoDepth >= 32)) {
+  if ((*ReceiveFifoDepth == 0) || (*ReceiveFifoDepth >= HardwareFifoDepth)) {
     // Enable FIFO
     LineControl = PL011_UARTLCR_H_FEN;
-    if (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) > PL011_VER_R1P4)
-      *ReceiveFifoDepth = 32;
-    else
-      *ReceiveFifoDepth = 16;
+    *ReceiveFifoDepth = HardwareFifoDepth;
   } else {
     // Disable FIFO
     LineControl = 0;
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

[PATCH 2/3] ArmPlatformPkg: Correct mendacious comments.

[evan.lloyd]

From: Alexei <Alexei.Fedorov@arm.com>

Correct some obviously incorrect comments that have invalid details for
the returned values.  (Copy /Paste problem?)
There are no functional changes in this commit.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
---
 ArmPlatformPkg/Include/Drivers/PL011Uart.h                     |  5 ++---
 ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c                   |  5 ++---
 ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 17 +++++++----------
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/ArmPlatformPkg/Include/Drivers/PL011Uart.h b/ArmPlatformPkg/Include/Drivers/PL011Uart.h
index 36ea9d62696162460567d91f9c1ba245d830db71..d5e88e86c86814e708bc901cca2153f5b7e5f927 100644
--- a/ArmPlatformPkg/Include/Drivers/PL011Uart.h
+++ b/ArmPlatformPkg/Include/Drivers/PL011Uart.h
@@ -246,9 +246,8 @@ PL011UartRead (
 /**
   Check to see if any data is available to be read from the debug device.
 
-  @retval EFI_SUCCESS       At least one byte of data is available to be read
-  @retval EFI_NOT_READY     No data is available to be read
-  @retval EFI_DEVICE_ERROR  The serial device is not functioning properly
+  @retval TRUE       At least one byte of data is available to be read
+  @retval FALSE      No data is available to be read
 
 **/
 BOOLEAN
diff --git a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
index b3ea138bf60b93a1000dd29aedaad206f2d15f2b..77630237ae91e38d8579303023c111bb56fe159d 100644
--- a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
+++ b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
@@ -456,9 +456,8 @@ PL011UartRead (
 /**
   Check to see if any data is available to be read from the debug device.
 
-  @retval EFI_SUCCESS       At least one byte of data is available to be read
-  @retval EFI_NOT_READY     No data is available to be read
-  @retval EFI_DEVICE_ERROR  The serial device is not functioning properly
+  @retval TRUE       At least one byte of data is available to be read
+  @retval FALSE      No data is available to be read
 
 **/
 BOOLEAN
diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
index 5092a0a202fac18f8c1b7bdc6d4e904db0c0d585..5dce852d90f9cafb828d81dae39d03451ea608e2 100644
--- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
+++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
@@ -23,14 +23,12 @@
 
 #include <Drivers/PL011Uart.h>
 
+/** Initialise the serial device hardware with default settings.
 
-/**
-
-  Programmed hardware of Serial port.
-
-  @return    Always return RETURN_UNSUPPORTED.
-
-**/
+  @retval RETURN_SUCCESS            The serial device was initialised.
+  @retval RETURN_INVALID_PARAMETER  One or more of the default settings
+                                    has an unsupported value.
+ **/
 RETURN_STATUS
 EFIAPI
 SerialPortInitialize (
@@ -103,9 +101,8 @@ SerialPortRead (
 /**
   Check to see if any data is available to be read from the debug device.
 
-  @retval EFI_SUCCESS       At least one byte of data is available to be read
-  @retval EFI_NOT_READY     No data is available to be read
-  @retval EFI_DEVICE_ERROR  The serial device is not functioning properly
+  @retval TRUE       At least one byte of data is available to be read
+  @retval FALSE      No data is available to be read
 
 **/
 BOOLEAN
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

[PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate.

[evan.lloyd]

From: Alexei <Alexei.Fedorov@arm.com>

SerialPortInitialize() set the BaudRate variable (type UINT64) as:
BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);

This commit fixes a potential problem on ARM 32-bit builds, where the
UINTN type is defined as UINT32, by removing the cast:

BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);

Note - a minor whitespace correction is rolled into this commit.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
---
 ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
index 5dce852d90f9cafb828d81dae39d03451ea608e2..4a24eded0e7d0f91270bf778cf1d89b6c809d0b2 100644
--- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
+++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
@@ -41,11 +41,11 @@ SerialPortInitialize (
   UINT8               DataBits;
   EFI_STOP_BITS_TYPE  StopBits;
 
-  BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
+  BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
   ReceiveFifoDepth = 0;         // Use default FIFO depth
   Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
   DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
-  StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8 (PcdUartDefaultStopBits);
+  StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8 (PcdUartDefaultStopBits);
 
   return PL011UartInitializePort (
            (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

Re: [PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test

[Leif Lindholm]

Apologies for delay, now catching up post plugfest and Linaro Connect.

On Wed, Sep 21, 2016 at 09:33:13PM +0100, evan.lloyd@arm.com wrote:
> From: Evan Lloyd <evan.lloyd@arm.com>
> 
> This change updates PL011UartInitializePort to compare ReceiveFifoDepth
> with the correct hardware FIFO size instead of the constant 32 used
> previously.
> This corrects a minor bug where a request for a fifo size > 15 and < 32
> would not have been honoured on a system with a 16 byte FIFO.

Whoops.
Well, even without the bug, the below is an improvement in readability.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> ---
>  ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
> index 3748972acbfc80f1077b9b928756a72cc77b5c75..b3ea138bf60b93a1000dd29aedaad206f2d15f2b 100644
> --- a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
> +++ b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
> @@ -79,17 +79,18 @@ PL011UartInitializePort (
>    UINT32      Divisor;
>    UINT32      Integer;
>    UINT32      Fractional;
> +  UINT32      HardwareFifoDepth;
>  
> +  HardwareFifoDepth = (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) \
> +                       > PL011_VER_R1P4) \
> +                      ? 32 : 16 ;

I may drop that space before ';' before committing, but apart from
that:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>    // The PL011 supports a buffer of 1, 16 or 32 chars. Therefore we can accept
>    // 1 char buffer as the minimum FIFO size. Because everything can be rounded
>    // down, there is no maximum FIFO size.
> -  if ((*ReceiveFifoDepth == 0) || (*ReceiveFifoDepth >= 32)) {
> +  if ((*ReceiveFifoDepth == 0) || (*ReceiveFifoDepth >= HardwareFifoDepth)) {
>      // Enable FIFO
>      LineControl = PL011_UARTLCR_H_FEN;
> -    if (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) > PL011_VER_R1P4)
> -      *ReceiveFifoDepth = 32;
> -    else
> -      *ReceiveFifoDepth = 16;
> +    *ReceiveFifoDepth = HardwareFifoDepth;
>    } else {
>      // Disable FIFO
>      LineControl = 0;
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 

Re: [PATCH 2/3] ArmPlatformPkg: Correct mendacious comments.

[Leif Lindholm]

On Wed, Sep 21, 2016 at 09:33:14PM +0100, evan.lloyd@arm.com wrote:
> From: Alexei <Alexei.Fedorov@arm.com>
> 
> Correct some obviously incorrect comments that have invalid details for
> the returned values.  (Copy /Paste problem?)

Oh, definitely.

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

Thanks!

> There are no functional changes in this commit.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> ---
>  ArmPlatformPkg/Include/Drivers/PL011Uart.h                     |  5 ++---
>  ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c                   |  5 ++---
>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 17 +++++++----------
>  3 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Include/Drivers/PL011Uart.h b/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> index 36ea9d62696162460567d91f9c1ba245d830db71..d5e88e86c86814e708bc901cca2153f5b7e5f927 100644
> --- a/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> +++ b/ArmPlatformPkg/Include/Drivers/PL011Uart.h
> @@ -246,9 +246,8 @@ PL011UartRead (
>  /**
>    Check to see if any data is available to be read from the debug device.
>  
> -  @retval EFI_SUCCESS       At least one byte of data is available to be read
> -  @retval EFI_NOT_READY     No data is available to be read
> -  @retval EFI_DEVICE_ERROR  The serial device is not functioning properly
> +  @retval TRUE       At least one byte of data is available to be read
> +  @retval FALSE      No data is available to be read
>  
>  **/
>  BOOLEAN
> diff --git a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
> index b3ea138bf60b93a1000dd29aedaad206f2d15f2b..77630237ae91e38d8579303023c111bb56fe159d 100644
> --- a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
> +++ b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c
> @@ -456,9 +456,8 @@ PL011UartRead (
>  /**
>    Check to see if any data is available to be read from the debug device.
>  
> -  @retval EFI_SUCCESS       At least one byte of data is available to be read
> -  @retval EFI_NOT_READY     No data is available to be read
> -  @retval EFI_DEVICE_ERROR  The serial device is not functioning properly
> +  @retval TRUE       At least one byte of data is available to be read
> +  @retval FALSE      No data is available to be read
>  
>  **/
>  BOOLEAN
> diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> index 5092a0a202fac18f8c1b7bdc6d4e904db0c0d585..5dce852d90f9cafb828d81dae39d03451ea608e2 100644
> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> @@ -23,14 +23,12 @@
>  
>  #include <Drivers/PL011Uart.h>
>  
> +/** Initialise the serial device hardware with default settings.
>  
> -/**
> -
> -  Programmed hardware of Serial port.
> -
> -  @return    Always return RETURN_UNSUPPORTED.
> -
> -**/
> +  @retval RETURN_SUCCESS            The serial device was initialised.
> +  @retval RETURN_INVALID_PARAMETER  One or more of the default settings
> +                                    has an unsupported value.
> + **/
>  RETURN_STATUS
>  EFIAPI
>  SerialPortInitialize (
> @@ -103,9 +101,8 @@ SerialPortRead (
>  /**
>    Check to see if any data is available to be read from the debug device.
>  
> -  @retval EFI_SUCCESS       At least one byte of data is available to be read
> -  @retval EFI_NOT_READY     No data is available to be read
> -  @retval EFI_DEVICE_ERROR  The serial device is not functioning properly
> +  @retval TRUE       At least one byte of data is available to be read
> +  @retval FALSE      No data is available to be read
>  
>  **/
>  BOOLEAN
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 

Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate.

[Leif Lindholm]

On Wed, Sep 21, 2016 at 09:33:15PM +0100, evan.lloyd@arm.com wrote:
> From: Alexei <Alexei.Fedorov@arm.com>
> 
> SerialPortInitialize() set the BaudRate variable (type UINT64) as:
> BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
> 
> This commit fixes a potential problem on ARM 32-bit builds, where the
> UINTN type is defined as UINT32, by removing the cast:
> 
> BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
> 
> Note - a minor whitespace correction is rolled into this commit.

I can unroll it for you before committing, but I'm not going to leave
the history in a state where it looks like a FixedPcdGet8 was modified
by a commit with the title "Remove UINTN cast when setting BaudRate.".

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

Let me know how you want to deal with the whitespace change.

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> ---
>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> index 5dce852d90f9cafb828d81dae39d03451ea608e2..4a24eded0e7d0f91270bf778cf1d89b6c809d0b2 100644
> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> @@ -41,11 +41,11 @@ SerialPortInitialize (
>    UINT8               DataBits;
>    EFI_STOP_BITS_TYPE  StopBits;
>  
> -  BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
> +  BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
>    ReceiveFifoDepth = 0;         // Use default FIFO depth
>    Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
>    DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
> -  StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8 (PcdUartDefaultStopBits);
> +  StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8 (PcdUartDefaultStopBits);
>  
>    return PL011UartInitializePort (
>             (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 

Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate.

[Evan Lloyd]

Hi Leif.
Please feel free to fix the space change as you see fit and proper, as it was just incidental tidying up.

It would be good to have a discussion about the general position, though.
There are, I am sure, sound reasons for not rolling these things together (and I knew that, and shouldn't have, but...).
I understand some of those reasons, but I see some unfortunate consequences too, so I'd like to play devil's advocate here.

One unfortunate effect is to discourage the submission of trivial changes that would not in themselves justify the rigmarole of a patch.
I know we would hope to do it all properly, but I don't think that is how human nature works.  The cost/benefit comparison of adding or removing a space (or other cosmetic change) as a separate patch is not really worthwhile, so it is much easier to not "see" the improvement.  Please note: I am not thinking solely of myself here - I know others find the same thing.
Of course, if the intent is " to discourage the submission of trivial changes" that would make a sort of sense from the maintainer's perspective.
My position is that by making minor incidental improvement relatively expensive the actual effect is to discourage it.
Does the benefit derived from discrete patches really override this disadvantage?
One could rephrase that as "does a tidy git log outweigh good code quality?"

Regards,
Evan

>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: 10 October 2016 20:05
>To: Evan Lloyd
>Cc: edk2-devel@ml01.01.org; ard.biesheuvel@linaro.org;
>ryan.harkin@linaro.org
>Subject: Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when
>setting BaudRate.
>
>
>On Wed, Sep 21, 2016 at 09:33:15PM +0100, evan.lloyd@arm.com wrote:
>> From: Alexei <Alexei.Fedorov@arm.com>
>>
>> SerialPortInitialize() set the BaudRate variable (type UINT64) as:
>> BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
>>
>> This commit fixes a potential problem on ARM 32-bit builds, where the
>> UINTN type is defined as UINT32, by removing the cast:
>>
>> BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
>>
>> Note - a minor whitespace correction is rolled into this commit.
>
>I can unroll it for you before committing, but I'm not going to leave
>the history in a state where it looks like a FixedPcdGet8 was modified
>by a commit with the title "Remove UINTN cast when setting BaudRate.".
>
>For the fix:
>Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
>Let me know how you want to deal with the whitespace change.
>
>/
>    Leif
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
>> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
>> ---
>>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git
>a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
>b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
>> index
>5dce852d90f9cafb828d81dae39d03451ea608e2..4a24eded0e7d0f91270bf778
>cf1d89b6c809d0b2 100644
>> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
>> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
>> @@ -41,11 +41,11 @@ SerialPortInitialize (
>>    UINT8               DataBits;
>>    EFI_STOP_BITS_TYPE  StopBits;
>>
>> -  BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
>> +  BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
>>    ReceiveFifoDepth = 0;         // Use default FIFO depth
>>    Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
>>    DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
>> -  StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8
>(PcdUartDefaultStopBits);
>> +  StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8
>(PcdUartDefaultStopBits);
>>
>>    return PL011UartInitializePort (
>>             (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
>> --
>> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate.

[Leif Lindholm]

On Tue, Oct 11, 2016 at 10:23:12AM +0000, Evan Lloyd wrote:
> Please feel free to fix the space change as you see fit and proper,
> as it was just incidental tidying up.

Thanks.

> It would be good to have a discussion about the general position,
> though.
> There are, I am sure, sound reasons for not rolling these things
> together (and I knew that, and shouldn't have, but...).
> I understand some of those reasons, but I see some unfortunate
> consequences too, so I'd like to play devil's advocate here.
> One unfortunate effect is to discourage the submission of trivial
> changes that would not in themselves justify the rigmarole of a
> patch.

In summary: the mixing of functional and non-functional modifications
reduces the effectiveness of code review and increases maintainer
overhead.
  
> I know we would hope to do it all properly, but I don't think that
> is how human nature works.  The cost/benefit comparison of adding or
> removing a space (or other cosmetic change) as a separate patch is
> not really worthwhile, so it is much easier to not "see" the
> improvement.  Please note: I am not thinking solely of myself here -
> I know others find the same thing.
> Of course, if the intent is " to discourage the submission of
> trivial changes" that would make a sort of sense from the
> maintainer's perspective.

Certainly not. But maintainers are not mind readers. Sure, if it's a
trivial fix it won't take _that_ much longer to review the patch. But
where's the limit on that? How likely am I to miss a code error (or a
possible improvement) because I'm busy trying to find which bits are
functional vs. non-functional changes?

This is also why we sometimes ask for large functional patches to be
subdivided.
See also https://alexgaynor.net/2015/dec/29/shrinking-code-review/

On the flip-side, I am very much happy to take non-functional-only
patches to large swathes of files. So if you find that less tedious,
you're welcome to collect up a bunch of these, squash them together
into a single commit and submit every now and then.
(Although backpedalling again, semantic changes to comments are best
left separate.)

> My position is that by making minor incidental improvement
> relatively expensive the actual effect is to discourage it.
> Does the benefit derived from discrete patches really override this
> disadvantage?

Yes.

> One could rephrase that as "does a tidy git log outweigh good code
> quality?"

I would prefer to put it as "a tidy log and more easily reviewable
patches are required for good code quality".

Regards,

Leif

> Regards,
> Evan
> 
> >-----Original Message-----
> >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >Sent: 10 October 2016 20:05
> >To: Evan Lloyd
> >Cc: edk2-devel@ml01.01.org; ard.biesheuvel@linaro.org;
> >ryan.harkin@linaro.org
> >Subject: Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when
> >setting BaudRate.
> >
> >
> >On Wed, Sep 21, 2016 at 09:33:15PM +0100, evan.lloyd@arm.com wrote:
> >> From: Alexei <Alexei.Fedorov@arm.com>
> >>
> >> SerialPortInitialize() set the BaudRate variable (type UINT64) as:
> >> BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
> >>
> >> This commit fixes a potential problem on ARM 32-bit builds, where the
> >> UINTN type is defined as UINT32, by removing the cast:
> >>
> >> BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
> >>
> >> Note - a minor whitespace correction is rolled into this commit.
> >
> >I can unroll it for you before committing, but I'm not going to leave
> >the history in a state where it looks like a FixedPcdGet8 was modified
> >by a commit with the title "Remove UINTN cast when setting BaudRate.".
> >
> >For the fix:
> >Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> >Let me know how you want to deal with the whitespace change.
> >
> >/
> >    Leif
> >
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
> >> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> >> ---
> >>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git
> >a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> >b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> >> index
> >5dce852d90f9cafb828d81dae39d03451ea608e2..4a24eded0e7d0f91270bf778
> >cf1d89b6c809d0b2 100644
> >> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> >> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> >> @@ -41,11 +41,11 @@ SerialPortInitialize (
> >>    UINT8               DataBits;
> >>    EFI_STOP_BITS_TYPE  StopBits;
> >>
> >> -  BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
> >> +  BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
> >>    ReceiveFifoDepth = 0;         // Use default FIFO depth
> >>    Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
> >>    DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
> >> -  StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8
> >(PcdUartDefaultStopBits);
> >> +  StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8
> >(PcdUartDefaultStopBits);
> >>
> >>    return PL011UartInitializePort (
> >>             (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
> >> --
> >> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >>
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 

Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate.

[Laszlo Ersek]

On 10/11/16 12:23, Evan Lloyd wrote:
> Hi Leif.
> Please feel free to fix the space change as you see fit and proper,
> as it was just incidental tidying up.

I would simply drop that hunk for now. While I personally prefer the
no-space form, and stick with it consistently in all code I write, other
edk2 developers are consistent in their opposite use of the space.

The edk2 coding standards doc I'm looking at now (Version 2.1, 30
October 2015 -- it could be old) does not regulate casts in this aspect.
Looking for examples rather than for explicit rules, we find both flavors:

* In 5.7.2.3 "Comparison of unsigned integer types to be >=0 is
permitted", we have

  if ((INTN)foo >= 0) { ...

that is, no space.

* In 5.7.2.4 "The ordering of terms in predicate expressions may impact
performance significantly", there's

  if (( LogEntryArray[Index].Handle == (EFI_PHYSICAL_ADDRESS) (UINTN)
Handle)

with one space afte (EFI_PHYSICAL_ADDRESS) and (UINTN) each.

Again, I strongly prefer the no-space form -- the cast operator is one
of the most strongly binding operators in C, and the space, common to
edk2, has actually caused developers to mis-read code and add bugs --,
but in this specific case, removing just this one space looks arbitrary
to me.

> It would be good to have a discussion about the general position,
> though.
> There are, I am sure, sound reasons for not rolling these things
> together (and I knew that, and shouldn't have, but...).
> I understand some of those reasons, but I see some unfortunate
> consequences too, so I'd like to play devil's advocate here.
> 
> One unfortunate effect is to discourage the submission of trivial
> changes that would not in themselves justify the rigmarole of a
> patch.
> I know we would hope to do it all properly, but I don't think that is
> how human nature works.  The cost/benefit comparison of adding or
> removing a space (or other cosmetic change) as a separate patch is
> not really worthwhile, so it is much easier to not "see" the
> improvement.  Please note: I am not thinking solely of myself here -
> I know others find the same thing.

In this case, I agree with both of you -- squashing the space change is
not nice, and writing a one-liner patch for the space change is also
overkill. For that reason, I suggest to simply drop the change. The
current spacing -- while inferior, in my persional opinion -- is not a
bug, and it matches existent edk2 practice. I agree the spacing does not
match the direct neighborhood -- if you feel strongly about that, it
might justify a separate patch.

> Of course, if the intent is " to discourage the submission of trivial
> changes" that would make a sort of sense from the maintainer's
> perspective.

"Trivial" improvements are definitely welcome (as far as I'm concerned,
but I'm quite sure Leif agrees :)), as long as they are well-focused.

> My position is that by making minor incidental improvement relatively
> expensive the actual effect is to discourage it.

I agree with your assessment.

> Does the benefit derived from discrete patches really override this
> disadvantage?

In my opinion: yes, it does.

> One could rephrase that as "does a tidy git log outweigh good code quality?"

In my opinion: yes, it does.

I think you may have considered git-log only. Please consider git-blame
and git-bisect too:

- With git-blame, you go from source code to commit message (and other
source code) -- that is, you pick a line of code, and would like to see
the explanation for it (commit message) and any logically pertaining
changes (other hunks in the same patch). Unrelated code hunks disturb
this process.

- With git-bisect, you have a regression (or, in a reverse bisectection,
an unexpected / unidentified fix) that you'd like to pinpoint. Keyword
"pin". The larger the patch "git-bisect" identifies, the less useful
"git-bisect" is to you -- after the bisection completes, one still has
to figure out why the identified patch causes the regression (or the
unexpected fix). Unrelated hunks are distracting in this process.

So, generally speaking:
- if "good code quality" is attainable by an actual bugfix, then that
certainly deserves a separate patch,
- if we downgrade "good code quality" to "tidy-looking code", then a
tidy git log certainly outweighs that (IMO).

In this particular case, I'd suggest to simply drop the space change, or
if we feel strongly about it, to include it in a separate patch. The
latter might feel awkward now, but it'll feel better in future blamings
and bisections.

(Sorry for the unsolicited opinion...)

Thanks
Laszlo

>> -----Original Message-----
>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>> Sent: 10 October 2016 20:05
>> To: Evan Lloyd
>> Cc: edk2-devel@ml01.01.org; ard.biesheuvel@linaro.org;
>> ryan.harkin@linaro.org
>> Subject: Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when
>> setting BaudRate.
>>
>>
>> On Wed, Sep 21, 2016 at 09:33:15PM +0100, evan.lloyd@arm.com wrote:
>>> From: Alexei <Alexei.Fedorov@arm.com>
>>>
>>> SerialPortInitialize() set the BaudRate variable (type UINT64) as:
>>> BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
>>>
>>> This commit fixes a potential problem on ARM 32-bit builds, where the
>>> UINTN type is defined as UINT32, by removing the cast:
>>>
>>> BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
>>>
>>> Note - a minor whitespace correction is rolled into this commit.
>>
>> I can unroll it for you before committing, but I'm not going to leave
>> the history in a state where it looks like a FixedPcdGet8 was modified
>> by a commit with the title "Remove UINTN cast when setting BaudRate.".
>>
>> For the fix:
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> Let me know how you want to deal with the whitespace change.
>>
>> /
>>    Leif
>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
>>> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
>>> ---
>>>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git
>> a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
>> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
>>> index
>> 5dce852d90f9cafb828d81dae39d03451ea608e2..4a24eded0e7d0f91270bf778
>> cf1d89b6c809d0b2 100644
>>> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
>>> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
>>> @@ -41,11 +41,11 @@ SerialPortInitialize (
>>>    UINT8               DataBits;
>>>    EFI_STOP_BITS_TYPE  StopBits;
>>>
>>> -  BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
>>> +  BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
>>>    ReceiveFifoDepth = 0;         // Use default FIFO depth
>>>    Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
>>>    DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
>>> -  StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8
>> (PcdUartDefaultStopBits);
>>> +  StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8
>> (PcdUartDefaultStopBits);
>>>
>>>    return PL011UartInitializePort (
>>>             (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
>>> --
>>> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>>>
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate.

[Evan Lloyd]

Thanks, Leif.
Some interesting points, and I agree and will strive to comply.
I only point out that there MAY be a general pressure to not bother "tidying" where trivia are observed.
That is, of course, difficult to prove or quantify.

Regards,
Evan

>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: 11 October 2016 12:28
>To: Evan Lloyd
>Cc: edk2-devel@ml01.01.org; ard.biesheuvel@linaro.org;
>ryan.harkin@linaro.org
>Subject: Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when
>setting BaudRate.
>
>On Tue, Oct 11, 2016 at 10:23:12AM +0000, Evan Lloyd wrote:
>> Please feel free to fix the space change as you see fit and proper,
>> as it was just incidental tidying up.
>
>Thanks.
>
>> It would be good to have a discussion about the general position,
>> though.
>> There are, I am sure, sound reasons for not rolling these things
>> together (and I knew that, and shouldn't have, but...).
>> I understand some of those reasons, but I see some unfortunate
>> consequences too, so I'd like to play devil's advocate here.
>> One unfortunate effect is to discourage the submission of trivial
>> changes that would not in themselves justify the rigmarole of a
>> patch.
>
>In summary: the mixing of functional and non-functional modifications
>reduces the effectiveness of code review and increases maintainer
>overhead.
>
>> I know we would hope to do it all properly, but I don't think that
>> is how human nature works.  The cost/benefit comparison of adding or
>> removing a space (or other cosmetic change) as a separate patch is
>> not really worthwhile, so it is much easier to not "see" the
>> improvement.  Please note: I am not thinking solely of myself here -
>> I know others find the same thing.
>> Of course, if the intent is " to discourage the submission of
>> trivial changes" that would make a sort of sense from the
>> maintainer's perspective.
>
>Certainly not. But maintainers are not mind readers. Sure, if it's a
>trivial fix it won't take _that_ much longer to review the patch. But
>where's the limit on that? How likely am I to miss a code error (or a
>possible improvement) because I'm busy trying to find which bits are
>functional vs. non-functional changes?
>
>This is also why we sometimes ask for large functional patches to be
>subdivided.
>See also https://alexgaynor.net/2015/dec/29/shrinking-code-review/
>
>On the flip-side, I am very much happy to take non-functional-only
>patches to large swathes of files. So if you find that less tedious,
>you're welcome to collect up a bunch of these, squash them together
>into a single commit and submit every now and then.
>(Although backpedalling again, semantic changes to comments are best
>left separate.)
>
>> My position is that by making minor incidental improvement
>> relatively expensive the actual effect is to discourage it.
>> Does the benefit derived from discrete patches really override this
>> disadvantage?
>
>Yes.
>
>> One could rephrase that as "does a tidy git log outweigh good code
>> quality?"
>
>I would prefer to put it as "a tidy log and more easily reviewable
>patches are required for good code quality".
>
>Regards,
>
>Leif
>
>> Regards,
>> Evan
>>
...

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 0/3] PL011 updates.

[Ryan Harkin]

On 21 September 2016 at 21:33,  <evan.lloyd@arm.com> wrote:
> From: Evan Lloyd <evan.lloyd@arm.com>
>
> This patchset comprises two very minor fixes to the PL011 UART related
> code, and some comment updates.
> The FIFO fix amounts to a performance improvement in limited circumstaces,
> where some older platforms might not used the FIFO.
> The UINTN cast fix preempts errors on future 32-bit platforms.
>
> The code can be inspected at: https://github.com/EvanLloyd/tianocore/tree/pl011a_v1
>
>
> Alexei (2):
>   ArmPlatformPkg: Correct mendacious comments.
>   ArmPlatformPkg: Remove UINTN cast when setting BaudRate.
>
> Evan Lloyd (1):
>   ArmPlatformPkg: Fix PL011 FIFO size test
>
>  ArmPlatformPkg/Include/Drivers/PL011Uart.h                     |  5 ++---
>  ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c                   | 16 +++++++--------
>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 21 +++++++++-----------
>  3 files changed, 19 insertions(+), 23 deletions(-)
>
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>

For the whole series:

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

[Tested on Juno R0/1/2, TC2, FVP Foundation and AEMv8 models.]