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 > > Fix build_spcr() to include the clock frequency field in revision 3 tables. > Per the specification, this is the only change between revisions 2 and 3. Maybe say why this has never mattered - I think because no code actually uses revision 3. Prior to this series, arm-virt and loongarch were 2 and riscv-virt was 4. > > Signed-off-by: Vadim Chichikalyuk <chichikal...@gmail.com> Code looks fine so with those additions to the description Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com> > --- > 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! > + 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); > + } > } > acpi_table_end(linker, &table); > }
