Re: [PATCH 2/2] hppa: Fix serial port pass-through

[Mark Cave-Ayland]

On 26/05/2022 12:19, Helge Deller wrote:

> This fixes the serial ports in the emulation to behave as on original
> hardware.
>
> On the real hardware, the LASI UART is serial port #0 and the DINO UART
> is serial port #1. This is fixed in SEABIOS_HPPA_VERSION >= 6, which is
> why the latest firmware is required.
>
> The serial port addresses in hppa/hppa_hardware.h are swapped and the
> file is synced with the version in SeaBIOS-hppa. Please note that this
> file is shared between qemu and SeaBIOS-hppa, which is why a comment was
> added at the top of the file.
>
> When creating the virtual serial ports the correct port addresses are
> now used.
>
> Finally, this patch now allows the user to give:
>      -serial mon:stdio -serial /dev/ttyS4
> to use emulated ttyS0 in the guest for console output, and pass ttyS4
> from the host to ttyS1 in the guest.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
>   hw/hppa/hppa_hardware.h | 10 ++++++++--
>   hw/hppa/machine.c       | 17 +++++++----------
>   2 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/hw/hppa/hppa_hardware.h b/hw/hppa/hppa_hardware.h
> index 8b6b9222cb..c036d467f3 100644
> --- a/hw/hppa/hppa_hardware.h
> +++ b/hw/hppa/hppa_hardware.h
> @@ -1,4 +1,5 @@
>   /* HPPA cores and system support chips.  */
> +/* Be aware: This file is shared as-is with seabios-hppa. */

Ah thanks for the comment - I didn't realise that when I did the last set of changes, 
but presumably everything is in sync now? It would be nice for this to be done as a 
separate commit, since even with the PORT_SERIAL1 and PORT_SERIAL2 changes, there 
would be no regression if the serial ports are currently broken.

>   #ifndef HW_HPPA_HPPA_HARDWARE_H
>   #define HW_HPPA_HPPA_HARDWARE_H
> @@ -30,13 +31,18 @@
>   #define PCI_HPA         DINO_HPA        /* PCI bus */
>   #define IDE_HPA         0xf9000000      /* Boot disc controller */
>
> +/* offsets to DINO HPA: */
> +#define DINO_PCI_ADDR           0x064
> +#define DINO_CONFIG_DATA        0x068
> +#define DINO_IO_DATA            0x06c
> +
>   #define PORT_PCI_CMD    (PCI_HPA + DINO_PCI_ADDR)
>   #define PORT_PCI_DATA   (PCI_HPA + DINO_CONFIG_DATA)
>
>   #define FW_CFG_IO_BASE  0xfffa0000
>
> -#define PORT_SERIAL1    (DINO_UART_HPA + 0x800)
> -#define PORT_SERIAL2    (LASI_UART_HPA + 0x800)
> +#define PORT_SERIAL1    (LASI_UART_HPA + 0x800)
> +#define PORT_SERIAL2    (DINO_UART_HPA + 0x800)
>
>   #define HPPA_MAX_CPUS   16      /* max. number of SMP CPUs */
>   #define CPU_CLOCK_MHZ   250     /* emulate a 250 MHz CPU */
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index d1e174b1f4..5d23b9e528 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -32,7 +32,7 @@
>
>   #define MAX_IDE_BUS 2
>
> -#define MIN_SEABIOS_HPPA_VERSION 1 /* require at least this fw version */
> +#define MIN_SEABIOS_HPPA_VERSION 6 /* require at least this fw version */
>
>   #define HPA_POWER_BUTTON (FIRMWARE_END - 0x10)
>
> @@ -236,18 +236,15 @@ static void machine_hppa_init(MachineState *machine)
>       /* Realtime clock, used by firmware for PDC_TOD call. */
>       mc146818_rtc_init(isa_bus, 2000, NULL);
>
> -    /* Serial code setup.  */
> +    /* Serial ports - Lasi and Dino use a 7.272727 MHz clock. */
>       if (serial_hd(0)) {
> -        uint32_t addr = DINO_UART_HPA + 0x800;
> -        serial_mm_init(addr_space, addr, 0,
> -                       qdev_get_gpio_in(dino_dev, DINO_IRQ_RS232INT),
> -                       115200, serial_hd(0), DEVICE_BIG_ENDIAN);
> +        serial_mm_init(addr_space, LASI_UART_HPA + 0x800, 0,
> +                qdev_get_gpio_in(lasi_dev, LASI_IRQ_UART_HPA), 7272727 / 16,
> +                serial_hd(0), DEVICE_BIG_ENDIAN);
>       }
> -
>       if (serial_hd(1)) {
> -        /* Serial port */
> -        serial_mm_init(addr_space, LASI_UART_HPA + 0x800, 0,
> -                qdev_get_gpio_in(lasi_dev, LASI_IRQ_UART_HPA), 8000000 / 16,
> +        serial_mm_init(addr_space, DINO_UART_HPA + 0x800, 0,
> +                qdev_get_gpio_in(dino_dev, DINO_IRQ_RS232INT), 7272727 / 16,
>                   serial_hd(1), DEVICE_BIG_ENDIAN);
>       }

You can remove both of the "if (serial_hd(x)) { ... }" wrappers here: for some time 
now the chardev code assumes that NULL == not connected. This is also better since it 
means that even if you haven't connected anything to the guest serial port, the port 
is still mapped in the guest's address space which can be useful if the firmware 
unconditionally outputs debugging information on the serial port.


ATB,

Mark.