[PATCH v10 02/37] acpi: x86: Write FADT in common code

[Simon Glass]

Hi Heinrich,

On Fri, 8 Nov 2024 at 04:19, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 8. November 2024 08:10:05 MEZ schrieb Patrick Rudolph <patrick.rudolph at 9elements.com>:
> >Hi Heinrich,
> >On Thu, Nov 7, 2024 at 10:19 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> On 10/23/24 15:19, Patrick Rudolph wrote:
> >> > From: Maximilian Brune <maximilian.brune at 9elements.com>
> >> >
> >> > Write the FADT in common code since it's used on all architectures.
> >> > Since the FADT is mandatory all SoCs or mainboards must implement the
> >> > introduced function acpi_fill_fadt() and properly update the FADT.
> >> >
> >> > Signed-off-by: Patrick Rudolph <patrick.rudolph at 9elements.com>
> >> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >> > Cc: Simon Glass <sjg at chromium.org>
> >> > Cc: Bin Meng <bmeng.cn at gmail.com>
> >>
> >> This merged patch breaks
> >>
> >> make qemu_riscv64_defconfig acpi.config
> >> make
> >I'm sorry, since the CI doesn't build that target, I was not aware of
> >build failures.
> >
> >>
> >> You are implementing function acpi_fill_fadt() only for x86 and call it
> >> in code that is used by all architectures.
> >>
> >acpi_fill_fadt() is called on all architectures that are ACPI enabled
> >and build by the CI.
> >There might additional ACPI enabled platforms, but since they are not
> >build tested,
> >they aren't officially supported, are they?
>
> I worked on getting ACPI enabled on arm64 and riscv64  QEMU but could have made better by adding it to CI.
>
> For some features it was decided that we prefer config overlays over deconfigs. But the test concept is kind of missing.

I am not a fan either. But I just sent a patch[1] proposing what we
might do about it, assuming we are going to continue down this 'config
fragment' path. It isn't a good solution, since it will potentially
explode the number of builds, unfortunately. But it does have some
advantages. For example we could build all boards without
CONFIG_CMDLINE or filesystems, to make sure all is well.

>
> Best regards
>
> Heinrich
>
>
> >
> >> ACPI_WRITER(5fadt, "FADT", acpi_write_fadt, 0);
> >> makes no sense for CONFIG_QFW=y because QEMU is providing the table.
> >Good catch. I'll send a follow up series.
> >
> >>
> >> @Tom:
> >> After getting this fixed we should add a build with acpi.config into the CI.
> >>
> >> further comments below:
> >>
> >>
> >>
> >> >
> >> > ---
> >> > Changelog v4:
> >> > - Drop __weak attribute
> >> > ---
> >> >   arch/sandbox/lib/acpi_table.c     |  6 +++++
> >> >   arch/x86/cpu/apollolake/acpi.c    | 20 ++++------------
> >> >   arch/x86/cpu/baytrail/acpi.c      | 17 +-------------
> >> >   arch/x86/cpu/quark/acpi.c         | 19 +---------------
> >> >   arch/x86/cpu/tangier/acpi.c       | 25 ++------------------
> >> >   arch/x86/include/asm/acpi_table.h | 12 ----------
> >> >   arch/x86/lib/acpi_table.c         | 23 -------------------
> >> >   include/acpi/acpi_table.h         |  9 ++++++++
> >> >   lib/acpi/acpi_table.c             | 38 +++++++++++++++++++++++++++++++
> >> >   9 files changed, 61 insertions(+), 108 deletions(-)
> >> >   create mode 100644 arch/sandbox/lib/acpi_table.c
> >> >
> >> > diff --git a/arch/sandbox/lib/acpi_table.c b/arch/sandbox/lib/acpi_table.c
> >> > new file mode 100644
> >> > index 0000000000..10b7ce441a
> >> > --- /dev/null
> >> > +++ b/arch/sandbox/lib/acpi_table.c
> >> > @@ -0,0 +1,6 @@
> >> > +// SPDX-License-Identifier: GPL-2.0+
> >> > +#include <acpi/acpi_table.h>
> >> > +
> >> > +void acpi_fill_fadt(struct acpi_fadt *fadt)
> >> > +{
> >> > +}

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20241108152350.3686274-9-sjg@chromium.org/