On Tue, May 23, 2023 at 12:25 AM Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > > Dave Jiang wrote: > > > > On 5/22/23 14:31, Dan Williams wrote: > > > Dave Jiang wrote: > > >> Some of the routines in ACPI tables.c can be shared with parsing CDAT. > > > s,ACPI tables.c,driver/acpi/tables.c, > > > > > >> However, CDAT is used by CXL and can exist on platforms that do not use > > >> ACPI. > > > Clarify that CDAT is not an ACPI table: > > > > > > CDAT is a device-provided data structure that is formatted similar to a > > > platform-provided ACPI table. > > > > > >> Split out the common routine from ACPI to accomodate platforms that > > >> do not support ACPI. The common routines can be built outside of ACPI if > > >> ACPI_TABLES_LIB is selected. > > > Might be just me but I get confused where this is indicating "ACPI" the > > > platform vs "CONFIG_ACPI" the code. How about just: > > > > > > Refactor the table parsing routines in driver/acpi/tables.c into helpers > > > that can be shared with the CXL driver even in the CONFIG_ACPI=n case. > > > > > >> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx> > > >> --- > > >> drivers/Makefile | 2 > > >> drivers/acpi/Kconfig | 4 + > > >> drivers/acpi/Makefile | 3 + > > >> drivers/acpi/tables.c | 173 ---------------------------------------- > > >> drivers/acpi/tables_lib.c | 194 +++++++++++++++++++++++++++++++++++++++++++++ > > >> include/linux/acpi.h | 63 +++++++++------ > > >> 6 files changed, 241 insertions(+), 198 deletions(-) > > >> create mode 100644 drivers/acpi/tables_lib.c > > > Conversion looks ok to me. Even though the cover letter said "Hi Rafael, > > > Please consider these for 6.5 merge window" my expectation is to take > > > these through CXL with ACPI acks. > > > > I thought you wanted Rafael to take the ACPI patches. But going to the > > CXL tree works. > > Ultimately up to Rafael. Either need a stable ACPI tree baseline to base > the CDAT work upon, or take this all through CXL. AFAIAC, it can go in via the CXL tree, but see below. > > > > > > > > One question below: > > > > > >> diff --git a/drivers/Makefile b/drivers/Makefile > > >> index 20b118dca999..1824797f7dfe 100644 > > >> --- a/drivers/Makefile > > >> +++ b/drivers/Makefile > > >> @@ -31,7 +31,7 @@ obj-y += idle/ > > >> # IPMI must come before ACPI in order to provide IPMI opregion support > > >> obj-y += char/ipmi/ > > >> > > >> -obj-$(CONFIG_ACPI) += acpi/ > > >> +obj-y += acpi/ > > >> > > >> # PnP must come after ACPI since it will eventually need to check if acpi > > >> # was used and do nothing if so > > >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > > >> index ccbeab9500ec..ce74a20dc42f 100644 > > >> --- a/drivers/acpi/Kconfig > > >> +++ b/drivers/acpi/Kconfig > > >> @@ -6,12 +6,16 @@ > > >> config ARCH_SUPPORTS_ACPI > > >> bool > > >> > > >> +config ACPI_TABLES_LIB > > >> + bool > > >> + > > >> menuconfig ACPI > > >> bool "ACPI (Advanced Configuration and Power Interface) Support" > > >> depends on ARCH_SUPPORTS_ACPI > > >> select PNP > > >> select NLS > > >> select CRC32 > > >> + select ACPI_TABLES_LIB > > >> default y if X86 > > >> help > > >> Advanced Configuration and Power Interface (ACPI) support for > > >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > > >> index feb36c0b9446..4558e2876823 100644 > > >> --- a/drivers/acpi/Makefile > > >> +++ b/drivers/acpi/Makefile > > >> @@ -13,6 +13,9 @@ tables.o: $(src)/../../include/$(CONFIG_ACPI_CUSTOM_DSDT_FILE) ; > > >> > > >> endif > > >> > > >> +obj-$(CONFIG_ACPI_TABLES_LIB) += acpi_tables_lib.o > > >> +acpi_tables_lib-y := tables_lib.o > > > Why is a separate object name needed? > > > > Not all code in tables.c will be shared. There are ACPI table parsing > > specific code in tables.c that CXL does not care about. Or do you mean > > just do > > > > obj-$(CONFIG_ACPI_TABLES_LIB) += tables_lib.o > > Yes, this. First, this depends on what is there in tables_lib.c. IMV it should not contain stuff that is not needed for CDAT parsing. Second, I'm not sure if drivers/acpi/ is the most appropriate location for it, maybe lib/ would be less confusing?
