| Message ID | 20240719181041.49545-10-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand |
On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > We weren't enabling the PL011 TX UART before using it > on the raspi and virt machines. Update the ASM code > prefixing: > > *UART_CTRL = UART_ENABLE | TX_ENABLE; > > to: > > while (true) { > *UART_DATA = 'T'; > } > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > tests/qtest/boot-serial-test.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c > index 3b92fa5d50..5cb309ccf0 100644 > --- a/tests/qtest/boot-serial-test.c > +++ b/tests/qtest/boot-serial-test.c > @@ -70,18 +70,23 @@ static const uint8_t kernel_plml605[] = { > }; > > static const uint8_t bios_raspi2[] = { > - 0x08, 0x30, 0x9f, 0xe5, /* ldr r3,[pc,#8] Get base */ > + 0x10, 0x30, 0x9f, 0xe5, /* ldr r3,[pc,#8] Get base */ The instruction bytes have changed but the disassembly comment has not... > + 0x10, 0x20, 0x9f, 0xe5, /* ldr r2,[pc,#8] Get CR */ > + 0xb0, 0x23, 0xc3, 0xe1, /* strh r2,[r3, #48] Set CR */ > 0x54, 0x20, 0xa0, 0xe3, /* mov r2,#'T' */ > - 0x00, 0x20, 0xc3, 0xe5, /* strb r2,[r3] */ > - 0xfb, 0xff, 0xff, 0xea, /* b loop */ > + 0x00, 0x20, 0xc3, 0xe5, /* strb r2,[r3] loop: */ This placement of the "loop:" label is odd -- usually the label goes before the instruction that a branch to the label will start executing at. > + 0xfd, 0xff, 0xff, 0xea, /* b loop */ Here also the bytes changed but not the disassembly. Since 'b' is a relative branch, why does the offset change? > 0x00, 0x10, 0x20, 0x3f, /* 0x3f201000 = UART0 base addr */ > + 0x01, 0x01, 0x00, 0x00, /* 0x101 = CR UARTEN|TXE */ > }; > > static const uint8_t kernel_aarch64[] = { > - 0x81, 0x0a, 0x80, 0x52, /* mov w1, #0x54 */ > 0x02, 0x20, 0xa1, 0xd2, /* mov x2, #0x9000000 */ > + 0x21, 0x20, 0x80, 0x52, /* mov w1, #0x101 */ > + 0x41, 0x60, 0x00, 0x79, /* strh w1, [x2, #48] */ > + 0x81, 0x0a, 0x80, 0x52, /* mov w1, #0x54 */ > 0x41, 0x00, 0x00, 0x39, /* strb w1, [x2] */ > - 0xfd, 0xff, 0xff, 0x17, /* b -12 (loop) */ > + 0xff, 0xff, 0xff, 0x17, /* b -4 (loop) */ Another unexplained offset change here. > }; > > static const uint8_t kernel_nrf51[] = { > -- > 2.41.0 thanks -- PMM
On 29/7/24 17:47, Peter Maydell wrote: > On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> We weren't enabling the PL011 TX UART before using it >> on the raspi and virt machines. Update the ASM code >> prefixing: >> >> *UART_CTRL = UART_ENABLE | TX_ENABLE; >> >> to: >> >> while (true) { >> *UART_DATA = 'T'; >> } >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> tests/qtest/boot-serial-test.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c >> index 3b92fa5d50..5cb309ccf0 100644 >> --- a/tests/qtest/boot-serial-test.c >> +++ b/tests/qtest/boot-serial-test.c >> @@ -70,18 +70,23 @@ static const uint8_t kernel_plml605[] = { >> }; >> >> static const uint8_t bios_raspi2[] = { >> - 0x08, 0x30, 0x9f, 0xe5, /* ldr r3,[pc,#8] Get base */ >> + 0x10, 0x30, 0x9f, 0xe5, /* ldr r3,[pc,#8] Get base */ > > The instruction bytes have changed but the disassembly comment has not... > >> + 0x10, 0x20, 0x9f, 0xe5, /* ldr r2,[pc,#8] Get CR */ >> + 0xb0, 0x23, 0xc3, 0xe1, /* strh r2,[r3, #48] Set CR */ >> 0x54, 0x20, 0xa0, 0xe3, /* mov r2,#'T' */ >> - 0x00, 0x20, 0xc3, 0xe5, /* strb r2,[r3] */ >> - 0xfb, 0xff, 0xff, 0xea, /* b loop */ >> + 0x00, 0x20, 0xc3, 0xe5, /* strb r2,[r3] loop: */ > > This placement of the "loop:" label is odd -- usually the label > goes before the instruction that a branch to the label will > start executing at. > >> + 0xfd, 0xff, 0xff, 0xea, /* b loop */ > > Here also the bytes changed but not the disassembly. Since > 'b' is a relative branch, why does the offset change? It felt simpler while single-stepping to just fill the TXDATA register with the byte to send, and not reset the other registers with unchanged values. Anyway you are right, I'll split the changes for clarity. > >> 0x00, 0x10, 0x20, 0x3f, /* 0x3f201000 = UART0 base addr */ >> + 0x01, 0x01, 0x00, 0x00, /* 0x101 = CR UARTEN|TXE */ >> }; >> >> static const uint8_t kernel_aarch64[] = { >> - 0x81, 0x0a, 0x80, 0x52, /* mov w1, #0x54 */ >> 0x02, 0x20, 0xa1, 0xd2, /* mov x2, #0x9000000 */ >> + 0x21, 0x20, 0x80, 0x52, /* mov w1, #0x101 */ >> + 0x41, 0x60, 0x00, 0x79, /* strh w1, [x2, #48] */ >> + 0x81, 0x0a, 0x80, 0x52, /* mov w1, #0x54 */ >> 0x41, 0x00, 0x00, 0x39, /* strb w1, [x2] */ >> - 0xfd, 0xff, 0xff, 0x17, /* b -12 (loop) */ >> + 0xff, 0xff, 0xff, 0x17, /* b -4 (loop) */ > > Another unexplained offset change here. > >> }; >> >> static const uint8_t kernel_nrf51[] = { >> -- >> 2.41.0 > > thanks > -- PMM
diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c index 3b92fa5d50..5cb309ccf0 100644 --- a/tests/qtest/boot-serial-test.c +++ b/tests/qtest/boot-serial-test.c @@ -70,18 +70,23 @@ static const uint8_t kernel_plml605[] = { }; static const uint8_t bios_raspi2[] = { - 0x08, 0x30, 0x9f, 0xe5, /* ldr r3,[pc,#8] Get base */ + 0x10, 0x30, 0x9f, 0xe5, /* ldr r3,[pc,#8] Get base */ + 0x10, 0x20, 0x9f, 0xe5, /* ldr r2,[pc,#8] Get CR */ + 0xb0, 0x23, 0xc3, 0xe1, /* strh r2,[r3, #48] Set CR */ 0x54, 0x20, 0xa0, 0xe3, /* mov r2,#'T' */ - 0x00, 0x20, 0xc3, 0xe5, /* strb r2,[r3] */ - 0xfb, 0xff, 0xff, 0xea, /* b loop */ + 0x00, 0x20, 0xc3, 0xe5, /* strb r2,[r3] loop: */ + 0xfd, 0xff, 0xff, 0xea, /* b loop */ 0x00, 0x10, 0x20, 0x3f, /* 0x3f201000 = UART0 base addr */ + 0x01, 0x01, 0x00, 0x00, /* 0x101 = CR UARTEN|TXE */ }; static const uint8_t kernel_aarch64[] = { - 0x81, 0x0a, 0x80, 0x52, /* mov w1, #0x54 */ 0x02, 0x20, 0xa1, 0xd2, /* mov x2, #0x9000000 */ + 0x21, 0x20, 0x80, 0x52, /* mov w1, #0x101 */ + 0x41, 0x60, 0x00, 0x79, /* strh w1, [x2, #48] */ + 0x81, 0x0a, 0x80, 0x52, /* mov w1, #0x54 */ 0x41, 0x00, 0x00, 0x39, /* strb w1, [x2] */ - 0xfd, 0xff, 0xff, 0x17, /* b -12 (loop) */ + 0xff, 0xff, 0xff, 0x17, /* b -4 (loop) */ }; static const uint8_t kernel_nrf51[] = {
We weren't enabling the PL011 TX UART before using it on the raspi and virt machines. Update the ASM code prefixing: *UART_CTRL = UART_ENABLE | TX_ENABLE; to: while (true) { *UART_DATA = 'T'; } Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- tests/qtest/boot-serial-test.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)