On the ARM virt machine, there is currently no way to programmatically
discover the frequency of the UART reference clock solely through the use of
UEFI/ACPI (without the DTB). The SPCR table can include this information
as of revision 3.

Bump the revision to 3 and add the clock frequency of 24 MHz to the table.

Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
---
 hw/arm/virt-acpi-build.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index b01fc4f8ef..029cbb37f7 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -559,12 +559,13 @@ spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         .pci_function = 0,
         .pci_flags = 0,
         .pci_segment = 0,
+        .uart_clk_freq = 24000000, /* 24MHz */
     };
     /*
-     * Passing NULL as the SPCR Table for Revision 2 doesn't support
+     * Passing NULL as the SPCR Table for Revision 3 doesn't support
      * NameSpaceString.
      */
-    build_spcr(table_data, linker, &serial, 2, vms->oem_id, vms->oem_table_id,
+    build_spcr(table_data, linker, &serial, 3, vms->oem_id, vms->oem_table_id,
                NULL);
 }
 
-- 
2.39.5 (Apple Git-154)

On Fri, 18 Jul 2025 19:20:44 +0300
Vadim Chichikalyuk <chichikalyuk@gmail.com> wrote:

> On the ARM virt machine, there is currently no way to programmatically
> discover the frequency of the UART reference clock solely through the use of
> UEFI/ACPI (without the DTB). The SPCR table can include this information
> as of revision 3.
> 
> Bump the revision to 3 and add the clock frequency of 24 MHz to the table.

Maybe add something on why you aren't just skipping forwards to 4 and filling
in the rest of the stuff?

> 
> Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  hw/arm/virt-acpi-build.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index b01fc4f8ef..029cbb37f7 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -559,12 +559,13 @@ spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          .pci_function = 0,
>          .pci_flags = 0,
>          .pci_segment = 0,
> +        .uart_clk_freq = 24000000, /* 24MHz */
>      };
>      /*
> -     * Passing NULL as the SPCR Table for Revision 2 doesn't support
> +     * Passing NULL as the SPCR Table for Revision 3 doesn't support
>       * NameSpaceString.
>       */
> -    build_spcr(table_data, linker, &serial, 2, vms->oem_id, vms->oem_table_id,
> +    build_spcr(table_data, linker, &serial, 3, vms->oem_id, vms->oem_table_id,
>                 NULL);
>  }
>

> On 21 Jul 2025, at 12:41, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> On Fri, 18 Jul 2025 19:20:44 +0300
> Vadim Chichikalyuk <chichikalyuk@gmail.com> wrote:
> 
>> On the ARM virt machine, there is currently no way to programmatically
>> discover the frequency of the UART reference clock solely through the use of
>> UEFI/ACPI (without the DTB). The SPCR table can include this information
>> as of revision 3.
>> 
>> Bump the revision to 3 and add the clock frequency of 24 MHz to the table.
> 
> Maybe add something on why you aren't just skipping forwards to 4 and filling
> in the rest of the stuff?

Haven’t really considered that – you’re right, might as well upgrade it to revision 4.

Based on build_dsdt() and acpi_dsdt_add_uart(), the NamespaceString would be 
“\_SB.COM0”, right? Although, for some reason, it’s just “.” for RISC-V virt (indicating
that there isn’t a corresponding device in the ACPI namespace), despite there being 
entries for the UART in the DSDT, just as for ARM?

The relevant lines, for reference (hw/arm/virt-acpi-build.c):

scope = aml_scope("\\_SB");
acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0], (irqmap[VIRT_UART0] + ARM_SPI_BASE), 0);

static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap, uint32_t uart_irq) {
    Aml *dev = aml_device("COM%d", uartidx);
    ...
}


Thanks for the review,
Vadim