> On 21 Jul 2025, at 12:39, Jonathan Cameron via <qemu-devel@nongnu.org> wrote:
>
> On Fri, 18 Jul 2025 19:20:42 +0300
> Vadim Chichikalyuk <chichikal...@gmail.com> wrote:
>
>> The UART clock frequency field of the SPCR table was added in revision 3.
>> Currently, build_spcr() treats revision 3 tables the same as revision 2 and
>> only includes this field in revision 4 tables.
>
> Given this isn't in the ACPI spec, I'd make sure you have a reference to the
> MS
> documentation for this. I think it is this one:
> https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/serial-port-console-redirection-table
I’m going off of the following line in the revision history table on that MS
page,
suggesting there was a revision 3 with just the clock frequency added:
"Changed Table Revision to 3 and created field for UART Clock Frequency. Edited
formatting."
>
>> ---
>> hw/acpi/aml-build.c | 20 +++++++++++---------
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 1e685f982f..9855d5f053 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2123,20 +2123,22 @@ void build_spcr(GArray *table_data, BIOSLinker
>> *linker,
>> build_append_int_noprefix(table_data, f->pci_flags, 4);
>> /* PCI Segment */
>> build_append_int_noprefix(table_data, f->pci_segment, 1);
>> - if (rev < 4) {
>> + if (rev < 3) {
>> /* Reserved */
>> build_append_int_noprefix(table_data, 0, 4);
>> } else {
>> /* UartClkFreq */
>> build_append_int_noprefix(table_data, f->uart_clk_freq, 4);
>> - /* PreciseBaudrate */
>> - build_append_int_noprefix(table_data, f->precise_baudrate, 4);
>> - /* NameSpaceStringLength */
>> - build_append_int_noprefix(table_data, f->namespace_string_length,
>> 2);
>> - /* NameSpaceStringOffset */
>> - build_append_int_noprefix(table_data, f->namespace_string_offset,
>> 2);
>> - /* NamespaceString[] */
>> - g_array_append_vals(table_data, name, f->namespace_string_length);
>> + if (rev >= 4) {
>> + /* PreciseBaudrate */
>
> Obviously historical, but this does seem like a lot of unnecessary comments
> given the clear naming of the input parameters!
>
Absolutely, I’ll remove those that don’t add any useful information in v2.
