| Message ID | 20231121152740.24783-2-heinrich.schuchardt@canonical.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andes |
| Headers | show |
| Series | risc-v: add ACPI support on QEMU | expand |
On Tue, Nov 21, 2023 at 04:27:34PM +0100, Heinrich Schuchardt wrote: > We have two implementations of write_acpi_tables(). One for writing ACPI > tables based on ACPI_WRITER() entries another based on copying tables from > QEMU. > > Create a symbol CONFIG_QFW_ACPI that signifies copying ACPI tables from > QEMU and use it consistently. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > v2: > new patch > --- > drivers/misc/Kconfig | 7 +++++++ > drivers/misc/qfw.c | 4 ++-- > lib/acpi/Makefile | 2 +- > lib/acpi/acpi_writer.c | 4 ++-- > 4 files changed, 12 insertions(+), 5 deletions(-) I still feel like this is a weird direction to go in and that: > diff --git a/lib/acpi/acpi_writer.c b/lib/acpi/acpi_writer.c > index 946f90e8e7..9b9fdc190b 100644 > --- a/lib/acpi/acpi_writer.c > +++ b/lib/acpi/acpi_writer.c > @@ -48,7 +48,7 @@ int acpi_write_one(struct acpi_ctx *ctx, const struct acpi_writer *entry) > return 0; > } > > -#ifndef CONFIG_QEMU > +#ifndef CONFIG_QFW_ACPI > static int acpi_write_all(struct acpi_ctx *ctx) > { > const struct acpi_writer *writer = > @@ -115,7 +115,7 @@ ulong acpi_get_rsdp_addr(void) > > return map_to_sysmem(gd->acpi_ctx->rsdp); > } > -#endif /* QEMU */ > +#endif /* QFW_ACPI */ > > void acpi_setup_ctx(struct acpi_ctx *ctx, ulong start) > { Will need to be tweaked later on still with some other symbol to denote "ACPI tables were passed along on real hardware by $mechanism". But we can cross that when we come to it.
On 11/21/23 20:00, Tom Rini wrote: > On Tue, Nov 21, 2023 at 04:27:34PM +0100, Heinrich Schuchardt wrote: > >> We have two implementations of write_acpi_tables(). One for writing ACPI >> tables based on ACPI_WRITER() entries another based on copying tables from >> QEMU. >> >> Create a symbol CONFIG_QFW_ACPI that signifies copying ACPI tables from >> QEMU and use it consistently. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> v2: >> new patch >> --- >> drivers/misc/Kconfig | 7 +++++++ >> drivers/misc/qfw.c | 4 ++-- >> lib/acpi/Makefile | 2 +- >> lib/acpi/acpi_writer.c | 4 ++-- >> 4 files changed, 12 insertions(+), 5 deletions(-) > > I still feel like this is a weird direction to go in and that: >> diff --git a/lib/acpi/acpi_writer.c b/lib/acpi/acpi_writer.c >> index 946f90e8e7..9b9fdc190b 100644 >> --- a/lib/acpi/acpi_writer.c >> +++ b/lib/acpi/acpi_writer.c >> @@ -48,7 +48,7 @@ int acpi_write_one(struct acpi_ctx *ctx, const struct acpi_writer *entry) >> return 0; >> } >> >> -#ifndef CONFIG_QEMU >> +#ifndef CONFIG_QFW_ACPI >> static int acpi_write_all(struct acpi_ctx *ctx) >> { >> const struct acpi_writer *writer = >> @@ -115,7 +115,7 @@ ulong acpi_get_rsdp_addr(void) >> >> return map_to_sysmem(gd->acpi_ctx->rsdp); >> } >> -#endif /* QEMU */ >> +#endif /* QFW_ACPI */ >> >> void acpi_setup_ctx(struct acpi_ctx *ctx, ulong start) >> { > > Will need to be tweaked later on still with some other symbol to denote > "ACPI tables were passed along on real hardware by $mechanism". But we > can cross that when we come to it. > QFW is only about QEMU. What I did here is separating the transfer of ACPI tables from QEMU from the code co-used by the sandbox. Do you think this is wrong? Probably when booting U-Boot via Coreboot we should be able to copy ACPI tables. But this will not involve QFW. Is there any code, yet, where U-Boot is picking up ACPI tables from Coreboot? In this case you would not set symbol CONFIG_GENERATE_ACPI_TABLES. Do we really need yet another symbol? Best regards Heinrich
On Tue, Nov 21, 2023 at 08:30:34PM +0100, Heinrich Schuchardt wrote: > On 11/21/23 20:00, Tom Rini wrote: > > On Tue, Nov 21, 2023 at 04:27:34PM +0100, Heinrich Schuchardt wrote: > > > > > We have two implementations of write_acpi_tables(). One for writing ACPI > > > tables based on ACPI_WRITER() entries another based on copying tables from > > > QEMU. > > > > > > Create a symbol CONFIG_QFW_ACPI that signifies copying ACPI tables from > > > QEMU and use it consistently. > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > > --- > > > v2: > > > new patch > > > --- > > > drivers/misc/Kconfig | 7 +++++++ > > > drivers/misc/qfw.c | 4 ++-- > > > lib/acpi/Makefile | 2 +- > > > lib/acpi/acpi_writer.c | 4 ++-- > > > 4 files changed, 12 insertions(+), 5 deletions(-) > > > > I still feel like this is a weird direction to go in and that: > > > diff --git a/lib/acpi/acpi_writer.c b/lib/acpi/acpi_writer.c > > > index 946f90e8e7..9b9fdc190b 100644 > > > --- a/lib/acpi/acpi_writer.c > > > +++ b/lib/acpi/acpi_writer.c > > > @@ -48,7 +48,7 @@ int acpi_write_one(struct acpi_ctx *ctx, const struct acpi_writer *entry) > > > return 0; > > > } > > > -#ifndef CONFIG_QEMU > > > +#ifndef CONFIG_QFW_ACPI > > > static int acpi_write_all(struct acpi_ctx *ctx) > > > { > > > const struct acpi_writer *writer = > > > @@ -115,7 +115,7 @@ ulong acpi_get_rsdp_addr(void) > > > return map_to_sysmem(gd->acpi_ctx->rsdp); > > > } > > > -#endif /* QEMU */ > > > +#endif /* QFW_ACPI */ > > > void acpi_setup_ctx(struct acpi_ctx *ctx, ulong start) > > > { > > > > Will need to be tweaked later on still with some other symbol to denote > > "ACPI tables were passed along on real hardware by $mechanism". But we > > can cross that when we come to it. > > > > QFW is only about QEMU. What I did here is separating the transfer of ACPI > tables from QEMU from the code co-used by the sandbox. Do you think this is > wrong? This part is right, yes. But I quoted the generic code here and not the QEMU part of the code. > Probably when booting U-Boot via Coreboot we should be able to copy ACPI > tables. But this will not involve QFW. Is there any code, yet, where U-Boot > is picking up ACPI tables from Coreboot? Is there, or should there, or could there be? I don't know the answer there, and is what I'm driving at. And that is.. > In this case you would not set symbol CONFIG_GENERATE_ACPI_TABLES. Do we > really need yet another symbol? Yes, I think a number of our symbols are oddly named, and confusing and we probably do need a new symbol or two. We have platforms which use CONFIG_GENERATE_ACPI_TABLES to mean "allocate and populate ACPI structures" and other platforms to mean "copy ACPI tables someone generated for us and pass them along to the OS".
On Tue, 21 Nov 2023 at 08:28, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > We have two implementations of write_acpi_tables(). One for writing ACPI > tables based on ACPI_WRITER() entries another based on copying tables from > QEMU. > > Create a symbol CONFIG_QFW_ACPI that signifies copying ACPI tables from > QEMU and use it consistently. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > v2: > new patch > --- > drivers/misc/Kconfig | 7 +++++++ > drivers/misc/qfw.c | 4 ++-- > lib/acpi/Makefile | 2 +- > lib/acpi/acpi_writer.c | 4 ++-- > 4 files changed, 12 insertions(+), 5 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 97057de8bf..86529efb2e 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -540,6 +540,13 @@ config QFW Hidden option to enable QEMU fw_cfg interface and uclass. This will be selected by either CONFIG_CMD_QFW or CONFIG_GENERATE_ACPI_TABLE. +config QFW_ACPI + bool + default y + depends on QFW && GENERATE_ACPI_TABLE && !SANDBOX + help + Hidden option to read ACPI tables from QEMU. + config QFW_PIO bool depends on QFW diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c index e3b6b4cd74..307334faf4 100644 --- a/drivers/misc/qfw.c +++ b/drivers/misc/qfw.c @@ -21,7 +21,7 @@ #include <tables_csum.h> #include <asm/acpi_table.h> -#if defined(CONFIG_GENERATE_ACPI_TABLE) && !defined(CONFIG_SANDBOX) +#ifdef QFW_ACPI /* * This function allocates memory for ACPI tables * @@ -259,7 +259,7 @@ ulong acpi_get_rsdp_addr(void) file = qfw_find_file(dev, "etc/acpi/rsdp"); return file->addr; } -#endif +#endif /* QFW_ACPI */ static void qfw_read_entry_io(struct qfw_dev *qdev, u16 entry, u32 size, void *address) diff --git a/lib/acpi/Makefile b/lib/acpi/Makefile index c1c9675b5d..cc2868488a 100644 --- a/lib/acpi/Makefile +++ b/lib/acpi/Makefile @@ -12,7 +12,7 @@ obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi_table.o obj-y += acpi_writer.o # With QEMU the ACPI tables come from there, not from U-Boot -ifndef CONFIG_QEMU +ifndef CONFIG_QFW_ACPI obj-y += base.o obj-y += csrt.o obj-y += mcfg.o diff --git a/lib/acpi/acpi_writer.c b/lib/acpi/acpi_writer.c index 946f90e8e7..9b9fdc190b 100644 --- a/lib/acpi/acpi_writer.c +++ b/lib/acpi/acpi_writer.c @@ -48,7 +48,7 @@ int acpi_write_one(struct acpi_ctx *ctx, const struct acpi_writer *entry) return 0; } -#ifndef CONFIG_QEMU +#ifndef CONFIG_QFW_ACPI static int acpi_write_all(struct acpi_ctx *ctx) { const struct acpi_writer *writer = @@ -115,7 +115,7 @@ ulong acpi_get_rsdp_addr(void) return map_to_sysmem(gd->acpi_ctx->rsdp); } -#endif /* QEMU */ +#endif /* QFW_ACPI */ void acpi_setup_ctx(struct acpi_ctx *ctx, ulong start) {
We have two implementations of write_acpi_tables(). One for writing ACPI tables based on ACPI_WRITER() entries another based on copying tables from QEMU. Create a symbol CONFIG_QFW_ACPI that signifies copying ACPI tables from QEMU and use it consistently. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- v2: new patch --- drivers/misc/Kconfig | 7 +++++++ drivers/misc/qfw.c | 4 ++-- lib/acpi/Makefile | 2 +- lib/acpi/acpi_writer.c | 4 ++-- 4 files changed, 12 insertions(+), 5 deletions(-)