[PATCH] hw/char/pl011: fix baud rate calculation

[PATCH] hw/char/pl011: fix baud rate calculation

[Baruch Siach]

The PL011 TRM says that "UARTIBRD = 0 is invalid and UARTFBRD is ignored
when this is the case". But the code looks at FBRD for the invalid case.
Fix this.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 hw/char/pl011.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 6e2d7f75095c..c076813423fc 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -176,7 +176,7 @@ static unsigned int pl011_get_baudrate(const PL011State *s)
 {
     uint64_t clk;
 
-    if (s->fbrd == 0) {
+    if (s->ibrd == 0) {
         return 0;
     }
 
-- 
2.35.1

Re: [PATCH] hw/char/pl011: fix baud rate calculation

[Peter Maydell]

On Thu, 6 Oct 2022 at 11:20, Baruch Siach <baruch@tkos.co.il> wrote:
>
> The PL011 TRM says that "UARTIBRD = 0 is invalid and UARTFBRD is ignored
> when this is the case". But the code looks at FBRD for the invalid case.
> Fix this.
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  hw/char/pl011.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks for this patch (I'll review it in a bit). Did you find this
because it caused problems for a guest, or just because you were
looking at the QEMU source code? (If it's the former, we like to
include some details of that in the commit message.)

-- PMM

Re: [PATCH] hw/char/pl011: fix baud rate calculation

[Baruch Siach]

Hi Peter,

On Thu, Oct 06 2022, Peter Maydell wrote:
> On Thu, 6 Oct 2022 at 11:20, Baruch Siach <baruch@tkos.co.il> wrote:
>>
>> The PL011 TRM says that "UARTIBRD = 0 is invalid and UARTFBRD is ignored
>> when this is the case". But the code looks at FBRD for the invalid case.
>> Fix this.
>>
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>>  hw/char/pl011.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Thanks for this patch (I'll review it in a bit). Did you find this
> because it caused problems for a guest, or just because you were
> looking at the QEMU source code? (If it's the former, we like to
> include some details of that in the commit message.)

Initially I only looked at the source code while trying to understand
how clock initialization and use is meant to work. Once I figured that
out I confirmed the issue with ATF code that first sets IBRD and then
FBRD.

Before patch:

pl011_baudrate_change new baudrate 0 (clk: 100000000hz, ibrd: 54, fbrd: 0)
pl011_baudrate_change new baudrate 115204 (clk: 100000000hz, ibrd: 54, fbrd: 16)

With patch:

pl011_baudrate_change new baudrate 115740 (clk: 100000000hz, ibrd: 54, fbrd: 0)
pl011_baudrate_change new baudrate 115204 (clk: 100000000hz, ibrd: 54, fbrd: 16)

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] hw/char/pl011: fix baud rate calculation

[Peter Maydell]

On Thu, 6 Oct 2022 at 11:20, Baruch Siach <baruch@tkos.co.il> wrote:
>
> The PL011 TRM says that "UARTIBRD = 0 is invalid and UARTFBRD is ignored
> when this is the case". But the code looks at FBRD for the invalid case.
> Fix this.
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  hw/char/pl011.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 6e2d7f75095c..c076813423fc 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -176,7 +176,7 @@ static unsigned int pl011_get_baudrate(const PL011State *s)
>  {
>      uint64_t clk;
>
> -    if (s->fbrd == 0) {
> +    if (s->ibrd == 0) {
>          return 0;
>      }

Applied to target-arm.next, thanks. (The only thing we do with the
calculated baud rate is trace it, so the bug wouldn't have had any
bad effects on the guest.)

-- PMM