* [PATCH 0/3] PL011 updates.
@ 2016-09-21 20:33 evan.lloyd
2016-09-21 20:33 ` [PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test evan.lloyd
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: evan.lloyd @ 2016-09-21 20:33 UTC (permalink / raw)
To: edk2-devel; +Cc: Ard Biesheuvel, Leif Lindholm, Ryan Harkin
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")
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test 2016-09-21 20:33 [PATCH 0/3] PL011 updates evan.lloyd @ 2016-09-21 20:33 ` evan.lloyd 2016-10-10 18:58 ` Leif Lindholm 2016-09-21 20:33 ` [PATCH 2/3] ArmPlatformPkg: Correct mendacious comments evan.lloyd ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: evan.lloyd @ 2016-09-21 20:33 UTC (permalink / raw) To: edk2-devel; +Cc: Ard Biesheuvel, Leif Lindholm, Ryan Harkin 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") ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test 2016-09-21 20:33 ` [PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test evan.lloyd @ 2016-10-10 18:58 ` Leif Lindholm 0 siblings, 0 replies; 12+ messages in thread From: Leif Lindholm @ 2016-10-10 18:58 UTC (permalink / raw) To: evan.lloyd; +Cc: edk2-devel, Ard Biesheuvel, Ryan Harkin 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") > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] ArmPlatformPkg: Correct mendacious comments. 2016-09-21 20:33 [PATCH 0/3] PL011 updates evan.lloyd 2016-09-21 20:33 ` [PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test evan.lloyd @ 2016-09-21 20:33 ` evan.lloyd 2016-10-10 19:00 ` Leif Lindholm 2016-09-21 20:33 ` [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate evan.lloyd 2016-10-12 7:30 ` [PATCH 0/3] PL011 updates Ryan Harkin 3 siblings, 1 reply; 12+ messages in thread From: evan.lloyd @ 2016-09-21 20:33 UTC (permalink / raw) To: edk2-devel; +Cc: Ard Biesheuvel, Leif Lindholm, Ryan Harkin 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") ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] ArmPlatformPkg: Correct mendacious comments. 2016-09-21 20:33 ` [PATCH 2/3] ArmPlatformPkg: Correct mendacious comments evan.lloyd @ 2016-10-10 19:00 ` Leif Lindholm 0 siblings, 0 replies; 12+ messages in thread From: Leif Lindholm @ 2016-10-10 19:00 UTC (permalink / raw) To: evan.lloyd; +Cc: edk2-devel, Ard Biesheuvel, Ryan Harkin 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") > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate. 2016-09-21 20:33 [PATCH 0/3] PL011 updates evan.lloyd 2016-09-21 20:33 ` [PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test evan.lloyd 2016-09-21 20:33 ` [PATCH 2/3] ArmPlatformPkg: Correct mendacious comments evan.lloyd @ 2016-09-21 20:33 ` evan.lloyd 2016-10-10 19:04 ` Leif Lindholm 2016-10-12 7:30 ` [PATCH 0/3] PL011 updates Ryan Harkin 3 siblings, 1 reply; 12+ messages in thread From: evan.lloyd @ 2016-09-21 20:33 UTC (permalink / raw) To: edk2-devel; +Cc: Ard Biesheuvel, Leif Lindholm, Ryan Harkin 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") ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate. 2016-09-21 20:33 ` [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate evan.lloyd @ 2016-10-10 19:04 ` Leif Lindholm 2016-10-11 10:23 ` Evan Lloyd 0 siblings, 1 reply; 12+ messages in thread From: Leif Lindholm @ 2016-10-10 19:04 UTC (permalink / raw) To: evan.lloyd; +Cc: edk2-devel, Ard Biesheuvel, Ryan Harkin 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") > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate. 2016-10-10 19:04 ` Leif Lindholm @ 2016-10-11 10:23 ` Evan Lloyd 2016-10-11 11:27 ` Leif Lindholm 2016-10-11 11:34 ` Laszlo Ersek 0 siblings, 2 replies; 12+ messages in thread From: Evan Lloyd @ 2016-10-11 10:23 UTC (permalink / raw) To: Leif Lindholm Cc: edk2-devel@ml01.01.org, ard.biesheuvel@linaro.org, ryan.harkin@linaro.org 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate. 2016-10-11 10:23 ` Evan Lloyd @ 2016-10-11 11:27 ` Leif Lindholm 2016-10-11 12:29 ` Evan Lloyd 2016-10-11 11:34 ` Laszlo Ersek 1 sibling, 1 reply; 12+ messages in thread From: Leif Lindholm @ 2016-10-11 11:27 UTC (permalink / raw) To: Evan Lloyd Cc: edk2-devel@ml01.01.org, ard.biesheuvel@linaro.org, ryan.harkin@linaro.org 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. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate. 2016-10-11 11:27 ` Leif Lindholm @ 2016-10-11 12:29 ` Evan Lloyd 0 siblings, 0 replies; 12+ messages in thread From: Evan Lloyd @ 2016-10-11 12:29 UTC (permalink / raw) To: Leif Lindholm Cc: edk2-devel@ml01.01.org, ard.biesheuvel@linaro.org, ryan.harkin@linaro.org 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate. 2016-10-11 10:23 ` Evan Lloyd 2016-10-11 11:27 ` Leif Lindholm @ 2016-10-11 11:34 ` Laszlo Ersek 1 sibling, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2016-10-11 11:34 UTC (permalink / raw) To: Evan Lloyd, Leif Lindholm Cc: edk2-devel@ml01.01.org, ryan.harkin@linaro.org, ard.biesheuvel@linaro.org 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] PL011 updates. 2016-09-21 20:33 [PATCH 0/3] PL011 updates evan.lloyd ` (2 preceding siblings ...) 2016-09-21 20:33 ` [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate evan.lloyd @ 2016-10-12 7:30 ` Ryan Harkin 3 siblings, 0 replies; 12+ messages in thread From: Ryan Harkin @ 2016-10-12 7:30 UTC (permalink / raw) To: Evan Lloyd; +Cc: edk2-devel@lists.01.org, Ard Biesheuvel, Leif Lindholm 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.] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-10-12 7:30 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-21 20:33 [PATCH 0/3] PL011 updates evan.lloyd 2016-09-21 20:33 ` [PATCH 1/3] ArmPlatformPkg: Fix PL011 FIFO size test evan.lloyd 2016-10-10 18:58 ` Leif Lindholm 2016-09-21 20:33 ` [PATCH 2/3] ArmPlatformPkg: Correct mendacious comments evan.lloyd 2016-10-10 19:00 ` Leif Lindholm 2016-09-21 20:33 ` [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate evan.lloyd 2016-10-10 19:04 ` Leif Lindholm 2016-10-11 10:23 ` Evan Lloyd 2016-10-11 11:27 ` Leif Lindholm 2016-10-11 12:29 ` Evan Lloyd 2016-10-11 11:34 ` Laszlo Ersek 2016-10-12 7:30 ` [PATCH 0/3] PL011 updates Ryan Harkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox