| Message ID | 1406206825-15590-20-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 07/24/2014 06:00 AM, Hanjun Guo wrote: > From: Graeme Gregory <graeme.gregory@linaro.org> > > Add documentation for the guidelines of how to use ACPI > on ARM64. > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 240 insertions(+) > create mode 100644 Documentation/arm64/arm-acpi.txt > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > new file mode 100644 > index 0000000..12cd550 > --- /dev/null > +++ b/Documentation/arm64/arm-acpi.txt > @@ -0,0 +1,240 @@ > +ACPI on ARMv8 Servers > +--------------------- > + > +ACPI will be used for ARMv8 general purpose servers designed to follow > +the SBSA specification (currently available to people with an ARM login at > +http://silver.arm.com) .com). > + > +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, > +which is available at <http://www.uefi.org/acpi/specs>. > + > +If the machine does not meet these requirements then it is likely that Device > +Tree (DT) is more suitable for the hardware. > + > +Relationship with Device Tree > +----------------------------- > + > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > +exclusive with DT support at compile time. > + > +At boot time the kernel will only use one description method depending on > +parameters passed from the bootloader. > + > +Regardless of whether DT or ACPI is used, the kernel must always be capable > +of booting with either scheme. > + > +When booting using ACPI tables the /chosen node in DT will still be parsed > +to extract the kernel command line and initrd path. No other section of > +the DT will be used. > + > +Booting using ACPI tables > +------------------------- > + > +Currently, the only defined method to pass ACPI tables to the kernel on ARMv8 > +is via the UEFI system configuration table. > + > +The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the > +RSDP table (the table with the ACPI signature "RSD PTR "). > + > +The pointer to the RSDP table will be retrieved from EFI by the ACPI core. > + > +Processing of ACPI tables may be disabled by passing acpi=off on the kernel > +command line. > + > +DO use an XSDT, RSDTs are deprecated and should not be used on arm64. They XSDT; > +only allow for 32bit addresses. 32-bit > + > +DO NOT use the 32-bit address fields in the FADT, they are deprecated, the FADT; they are deprecated. The > +64-bit alternatives MUST be used. > + > +The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT > +and GTDT. If PCI is used the MCFG table MUST also be present. > + > +ACPI Detection > +-------------- > + > +Drivers should determine their probe() type by checking for ACPI_HANDLE, > +or .of_node, or other information in the device structure. This is > +detailed further in the "Driver Recomendations" section. Recommendations > + > +If the presence of ACPI needs to be detected at runtime, then check the value > +of acpi_disabled. If CONFIG_ACPI not being set acpi_disabled will always be 1. If CONFIG_ACPI is not set, acpi_disabled will always be 1. > + > +Device Enumeration > +------------------ > + > +Device descriptions in ACPI should use standard recognised ACPI interfaces. > +These are far simpler than the information provided via Device Tree. Drivers > +should take into account this simplicity and work with sensible defaults. > + > +On no account should a Device Tree attempt to be replicated in ASL using such > +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. > + > +Common _DSD bindings should be submitted to ASWG to be included in the > +document :- > + > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm > + > +TODO: Clarification and examples from Juno implementation. > + > +Programmable Power Control Resources > +------------------------------------ > + > +Programmable power control resources include such resources as voltage/current > +providers (regulators) and clock sources. > + > +For power control of these resources they should be represented with Power > +Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly > +enabling/disabling of resources as they are needed. > + > +There exists in the ACPI 5.1 specification no standard binding for these objects > +to enable programmable levels or rates so this should be avoid if possible and avoided > +the resources set to appropriate level by the firmware. If this is not possible levels > +then any manipulation should be abstracted in ASL. > + > +Each device in ACPI has D-states and these can be controlled through > +the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off. > + > +If either _PS0 or _PS3 is implemented, then the other method must also be > +implemented. > + > +If a device requires usage or setup of a power resource when on, the ASL > +should organise that it is allocated/enabled using the _PS0 method. > + > +Resources allocated/enabled in the _PS0 method should be disabled/de-allocated > +in the _PS3 method. > + > +Such code in _PS? methods will of course be very platform specific but > +should allow the driver to operate the device without special non standard non-standard > +values being read from ASL. Further, abstracting the use of these resources > +allows hardware revisions without requiring updates to the kernel. > + > +TODO: Clarification and examples from Juno implementation. > + > +Clocks > +------ > + > +Like clocks that are part of the power resources there is no standard way > +to represent a clock tree in ACPI 5.1 in a similar manner to how it is > +described in DT. > + > +Devices affected by this include things like UARTs, SoC driven LCD displays, > +etc. > + > +The firmware for example UEFI should initialise these clocks to fixed working (for example, UEFI) > +values before the kernel is executed. If a driver requires to know rates of > +clocks set by firmware then they can be passed to kernel using _DSD. > + > +example :- > + > +Device (CLK0) { > + ... > + > + Name (_DSD, Package() { > + ToUUID("XXXXX"), > + Package() { > + Package(2) {"#clock-cells", 0}, > + Package(2) {"clock-frequency", "10000"} > + } > + }) > + > + ... > +} > + > +Device (USR1) { > + ... > + > + Name (_DSD, Package() { > + ToUUID("XXXXX"), > + Package() { > + Package(2) {"clocks", Package() {1, ^CLK0}}}, > + } > + }) > + > + ... > +} > + > +Driver Recommendations > +---------------------- > + > +DO NOT remove any FDT handling when adding ACPI support for a driver, different driver. Different > +systems may use the same device. > + > +DO try and keep complex sections of ACPI and DT functionality seperate. This separate. > +may mean a patch to break out some complex DT to another function before > +the patch to add ACPI. This may happen in other functions but is most likely > +in probe function. This gives a clearer flow of data for reviewing driver > +source. > + > +probe() :- > + > +TODO: replace this with a specific real example from Juno? > + > +static int device_probe_dt(struct platform_device *pdev) > +{ > + /* DT specific functionality */ > + ... > +} > + > +static int device_probe_acpi(struct platform_device *pdev) > +{ > + /* ACPI specific functionality */ > + ... > +} > + > +static int device_probe(stuct platform_device *pdev) > +{ > + ... > + acpi_handle handle = ACPI_HANDLE(&pdev->dev); > + struct device_node node = pdev->dev.of_node; > + ... > + > + if (node) > + ret = device_probe_dt(pdev); > + else if (handle) > + ret = device_probe_acpi(pdev); > + else > + /* other initialisation */ > + ... > + /* Continue with any generic probe operations */ > + ... > +} > + > +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear > +the different names the driver is probed for, both from DT and from ACPI. > + > +module device tables :- > + > +static struct of_device_id virtio_mmio_match[] = { > + { .compatible = "virtio,mmio", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, virtio_mmio_match); > + > +static const struct acpi_device_id virtio_mmio_acpi_match[] = { > + { "LNRO0005", }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match); > + > +TODO: Add any other helpful rules that develop from Juno ACPI work. > + > +ASWG > +---- > + > +The following areas are not yet well defined for ARM in the current ACPI > +specification and are expected to be worked through in the UEFI ACPI > +Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>. > +Participation in this group is open to all UEFI members. > + > + - ACPI based CPU topology > + - ACPI based Power management > + - CPU idle control based on PSCI > + - CPU performance control (CPPC) > + > +No code shall be accepted into the kernel unless it complies with the released > +standards from UEFI ASWG. If there are features missing from ACPI to make it > +function on a platform ECRs should be submitted to ASWG and go through the on a platform, ECRs > +approval process. >
On 07/24/2014 02:16 PM, Naresh Bhat wrote: > > On 24 July 2014 18:30, Hanjun Guo <hanjun.guo@linaro.org <mailto:hanjun.guo@linaro.org>> wrote: > > From: Graeme Gregory <graeme.gregory@linaro.org <mailto:graeme.gregory@linaro.org>> > > Add documentation for the guidelines of how to use ACPI > on ARM64. > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org <mailto:graeme.gregory@linaro.org>> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org <mailto:hanjun.guo@linaro.org>> > --- > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 240 insertions(+) > create mode 100644 Documentation/arm64/arm-acpi.txt > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > new file mode 100644 > index 0000000..12cd550 > --- /dev/null > +++ b/Documentation/arm64/arm-acpi.txt > @@ -0,0 +1,240 @@ > +ACPI on ARMv8 Servers > +--------------------- > + > +ACPI will be used for ARMv8 general purpose servers designed to follow > +the SBSA specification (currently available to people with an ARM login at > +http://silver.arm.com) > + > +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, > +which is available at <http://www.uefi.org/acpi/specs>. > + > +If the machine does not meet these requirements then it is likely that Device > +Tree (DT) is more suitable for the hardware. > + > +Relationship with Device Tree > +----------------------------- > + > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > +exclusive with DT support at compile time. > + > +At boot time the kernel will only use one description method depending on > +parameters passed from the bootloader. > + > +Regardless of whether DT or ACPI is used, the kernel must always be capable > +of booting with either scheme. > + > +When booting using ACPI tables the /chosen node in DT will still be parsed > +to extract the kernel command line and initrd path. No other section of > +the DT will be used. > + > +Booting using ACPI tables > +------------------------- > + > +Currently, the only defined method to pass ACPI tables to the kernel on ARMv8 > +is via the UEFI system configuration table. > + > +The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the > +RSDP table (the table with the ACPI signature "RSD PTR "). > + > +The pointer to the RSDP table will be retrieved from EFI by the ACPI core. > + > +Processing of ACPI tables may be disabled by passing acpi=off on the kernel > +command line. > + > +DO use an XSDT, RSDTs are deprecated and should not be used on arm64. They > +only allow for 32bit addresses. > + > +DO NOT use the 32-bit address fields in the FADT, they are deprecated, the > +64-bit alternatives MUST be used. > + > +The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT > +and GTDT. If PCI is used the MCFG table MUST also be present. > + > +ACPI Detection > +-------------- > + > +Drivers should determine their probe() type by checking for ACPI_HANDLE, > +or .of_node, or other information in the device structure. This is > +detailed further in the "Driver Recomendations" section. > + > +If the presence of ACPI needs to be detected at runtime, then check the value > +of acpi_disabled. If CONFIG_ACPI not being set acpi_disabled will always be 1. > + > +Device Enumeration > +------------------ > + > +Device descriptions in ACPI should use standard recognised ACPI interfaces. > > > recognized Yeah, I saw all of these also, but we accept British or American spelling of these words. > > > +These are far simpler than the information provided via Device Tree. Drivers > +should take into account this simplicity and work with sensible defaults. > + > +On no account should a Device Tree attempt to be replicated in ASL using such > +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. > + > +Common _DSD bindings should be submitted to ASWG to be included in the > +document :- > + > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm > + > +TODO: Clarification and examples from Juno implementation. > + > +Programmable Power Control Resources > +------------------------------------ > + > +Programmable power control resources include such resources as voltage/current > +providers (regulators) and clock sources. > + > +For power control of these resources they should be represented with Power > +Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly > +enabling/disabling of resources as they are needed. > + > +There exists in the ACPI 5.1 specification no standard binding for these objects > +to enable programmable levels or rates so this should be avoid if possible and > +the resources set to appropriate level by the firmware. If this is not possible > +then any manipulation should be abstracted in ASL. > + > +Each device in ACPI has D-states and these can be controlled through > +the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off. > + > +If either _PS0 or _PS3 is implemented, then the other method must also be > +implemented. > + > +If a device requires usage or setup of a power resource when on, the ASL > +should organise that it is allocated/enabled using the _PS0 method. > > > organize > > + > +Resources allocated/enabled in the _PS0 method should be disabled/de-allocated > +in the _PS3 method. > + > +Such code in _PS? methods will of course be very platform specific but > +should allow the driver to operate the device without special non standard > +values being read from ASL. Further, abstracting the use of these resources > +allows hardware revisions without requiring updates to the kernel. > + > +TODO: Clarification and examples from Juno implementation. > + > +Clocks > +------ > + > +Like clocks that are part of the power resources there is no standard way > +to represent a clock tree in ACPI 5.1 in a similar manner to how it is > +described in DT. > + > +Devices affected by this include things like UARTs, SoC driven LCD displays, > +etc. > + > +The firmware for example UEFI should initialise these clocks to fixed working > > > initialize > > > +values before the kernel is executed. If a driver requires to know rates of > +clocks set by firmware then they can be passed to kernel using _DSD. > + > +example :- > + > +Device (CLK0) { > + ... > + > + Name (_DSD, Package() { > + ToUUID("XXXXX"), > + Package() { > + Package(2) {"#clock-cells", 0}, > + Package(2) {"clock-frequency", "10000"} > + } > + }) > + > + ... > +} > + > +Device (USR1) { > + ... > + > + Name (_DSD, Package() { > + ToUUID("XXXXX"), > + Package() { > + Package(2) {"clocks", Package() {1, ^CLK0}}}, > + } > + }) > + > + ... > +} > + > +Driver Recommendations > +---------------------- > + > +DO NOT remove any FDT handling when adding ACPI support for a driver, different > +systems may use the same device. > + > +DO try and keep complex sections of ACPI and DT functionality seperate. This > > > separate > > > +may mean a patch to break out some complex DT to another function before > +the patch to add ACPI. This may happen in other functions but is most likely > +in probe function. This gives a clearer flow of data for reviewing driver > +source. > + > +probe() :- > + > +TODO: replace this with a specific real example from Juno? > + > +static int device_probe_dt(struct platform_device *pdev) > +{ > + /* DT specific functionality */ > + ... > +} > + > +static int device_probe_acpi(struct platform_device *pdev) > +{ > + /* ACPI specific functionality */ > + ... > +} > + > +static int device_probe(stuct platform_device *pdev) > +{ > + ... > + acpi_handle handle = ACPI_HANDLE(&pdev->dev); > + struct device_node node = pdev->dev.of_node; > + ... > + > + if (node) > + ret = device_probe_dt(pdev); > + else if (handle) > + ret = device_probe_acpi(pdev); > + else > + /* other initialisation */ > > > initialization > > > + ... > + /* Continue with any generic probe operations */ > + ... > +} > + > +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear > +the different names the driver is probed for, both from DT and from ACPI. > + > +module device tables :- > + > +static struct of_device_id virtio_mmio_match[] = { > + { .compatible = "virtio,mmio", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, virtio_mmio_match); > + > +static const struct acpi_device_id virtio_mmio_acpi_match[] = { > + { "LNRO0005", }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match); > + > +TODO: Add any other helpful rules that develop from Juno ACPI work. > + > +ASWG > +---- > + > +The following areas are not yet well defined for ARM in the current ACPI > +specification and are expected to be worked through in the UEFI ACPI > +Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>. > +Participation in this group is open to all UEFI members. > + > + - ACPI based CPU topology > + - ACPI based Power management > + - CPU idle control based on PSCI > + - CPU performance control (CPPC) > + > +No code shall be accepted into the kernel unless it complies with the released > +standards from UEFI ASWG. If there are features missing from ACPI to make it > +function on a platform ECRs should be submitted to ASWG and go through the > +approval process. > -- > 1.7.9.5 > >
Hi Randy, Thank you for your careful review comments, I will update it in next version :) Best Regards Hanjun On 2014-7-25 4:42, Randy Dunlap wrote: > On 07/24/2014 06:00 AM, Hanjun Guo wrote: >> From: Graeme Gregory <graeme.gregory@linaro.org> >> >> Add documentation for the guidelines of how to use ACPI >> on ARM64. >> >> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 240 insertions(+) >> create mode 100644 Documentation/arm64/arm-acpi.txt >> >> diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt >> new file mode 100644 >> index 0000000..12cd550 >> --- /dev/null >> +++ b/Documentation/arm64/arm-acpi.txt >> @@ -0,0 +1,240 @@ >> +ACPI on ARMv8 Servers >> +--------------------- >> + >> +ACPI will be used for ARMv8 general purpose servers designed to follow >> +the SBSA specification (currently available to people with an ARM login at >> +http://silver.arm.com) > > .com). > >> + >> +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, >> +which is available at <http://www.uefi.org/acpi/specs>. >> + >> +If the machine does not meet these requirements then it is likely that Device >> +Tree (DT) is more suitable for the hardware. >> + >> +Relationship with Device Tree >> +----------------------------- >> + >> +ACPI support in drivers and subsystems for ARMv8 should never be mutually >> +exclusive with DT support at compile time. >> + >> +At boot time the kernel will only use one description method depending on >> +parameters passed from the bootloader. >> + >> +Regardless of whether DT or ACPI is used, the kernel must always be capable >> +of booting with either scheme. >> + >> +When booting using ACPI tables the /chosen node in DT will still be parsed >> +to extract the kernel command line and initrd path. No other section of >> +the DT will be used. >> + >> +Booting using ACPI tables >> +------------------------- >> + >> +Currently, the only defined method to pass ACPI tables to the kernel on ARMv8 >> +is via the UEFI system configuration table. >> + >> +The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the >> +RSDP table (the table with the ACPI signature "RSD PTR "). >> + >> +The pointer to the RSDP table will be retrieved from EFI by the ACPI core. >> + >> +Processing of ACPI tables may be disabled by passing acpi=off on the kernel >> +command line. >> + >> +DO use an XSDT, RSDTs are deprecated and should not be used on arm64. They > > XSDT; > >> +only allow for 32bit addresses. > > 32-bit > >> + >> +DO NOT use the 32-bit address fields in the FADT, they are deprecated, the > > FADT; they are deprecated. The > >> +64-bit alternatives MUST be used. >> + >> +The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT >> +and GTDT. If PCI is used the MCFG table MUST also be present. >> + >> +ACPI Detection >> +-------------- >> + >> +Drivers should determine their probe() type by checking for ACPI_HANDLE, >> +or .of_node, or other information in the device structure. This is >> +detailed further in the "Driver Recomendations" section. > > Recommendations > >> + >> +If the presence of ACPI needs to be detected at runtime, then check the value >> +of acpi_disabled. If CONFIG_ACPI not being set acpi_disabled will always be 1. > > If CONFIG_ACPI is not set, acpi_disabled will always be 1. > >> + >> +Device Enumeration >> +------------------ >> + >> +Device descriptions in ACPI should use standard recognised ACPI interfaces. >> +These are far simpler than the information provided via Device Tree. Drivers >> +should take into account this simplicity and work with sensible defaults. >> + >> +On no account should a Device Tree attempt to be replicated in ASL using such >> +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific >> +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or >> +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. >> + >> +Common _DSD bindings should be submitted to ASWG to be included in the >> +document :- >> + >> +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm >> + >> +TODO: Clarification and examples from Juno implementation. >> + >> +Programmable Power Control Resources >> +------------------------------------ >> + >> +Programmable power control resources include such resources as voltage/current >> +providers (regulators) and clock sources. >> + >> +For power control of these resources they should be represented with Power >> +Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly >> +enabling/disabling of resources as they are needed. >> + >> +There exists in the ACPI 5.1 specification no standard binding for these objects >> +to enable programmable levels or rates so this should be avoid if possible and > > avoided > >> +the resources set to appropriate level by the firmware. If this is not possible > > levels > >> +then any manipulation should be abstracted in ASL. >> + >> +Each device in ACPI has D-states and these can be controlled through >> +the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off. >> + >> +If either _PS0 or _PS3 is implemented, then the other method must also be >> +implemented. >> + >> +If a device requires usage or setup of a power resource when on, the ASL >> +should organise that it is allocated/enabled using the _PS0 method. >> + >> +Resources allocated/enabled in the _PS0 method should be disabled/de-allocated >> +in the _PS3 method. >> + >> +Such code in _PS? methods will of course be very platform specific but >> +should allow the driver to operate the device without special non standard > > non-standard > >> +values being read from ASL. Further, abstracting the use of these resources >> +allows hardware revisions without requiring updates to the kernel. >> + >> +TODO: Clarification and examples from Juno implementation. >> + >> +Clocks >> +------ >> + >> +Like clocks that are part of the power resources there is no standard way >> +to represent a clock tree in ACPI 5.1 in a similar manner to how it is >> +described in DT. >> + >> +Devices affected by this include things like UARTs, SoC driven LCD displays, >> +etc. >> + >> +The firmware for example UEFI should initialise these clocks to fixed working > > (for example, UEFI) > >> +values before the kernel is executed. If a driver requires to know rates of >> +clocks set by firmware then they can be passed to kernel using _DSD. >> + >> +example :- >> + >> +Device (CLK0) { >> + ... >> + >> + Name (_DSD, Package() { >> + ToUUID("XXXXX"), >> + Package() { >> + Package(2) {"#clock-cells", 0}, >> + Package(2) {"clock-frequency", "10000"} >> + } >> + }) >> + >> + ... >> +} >> + >> +Device (USR1) { >> + ... >> + >> + Name (_DSD, Package() { >> + ToUUID("XXXXX"), >> + Package() { >> + Package(2) {"clocks", Package() {1, ^CLK0}}}, >> + } >> + }) >> + >> + ... >> +} >> + >> +Driver Recommendations >> +---------------------- >> + >> +DO NOT remove any FDT handling when adding ACPI support for a driver, different > > driver. Different > >> +systems may use the same device. >> + >> +DO try and keep complex sections of ACPI and DT functionality seperate. This > > separate. > >> +may mean a patch to break out some complex DT to another function before >> +the patch to add ACPI. This may happen in other functions but is most likely >> +in probe function. This gives a clearer flow of data for reviewing driver >> +source. >> + >> +probe() :- >> + >> +TODO: replace this with a specific real example from Juno? >> + >> +static int device_probe_dt(struct platform_device *pdev) >> +{ >> + /* DT specific functionality */ >> + ... >> +} >> + >> +static int device_probe_acpi(struct platform_device *pdev) >> +{ >> + /* ACPI specific functionality */ >> + ... >> +} >> + >> +static int device_probe(stuct platform_device *pdev) >> +{ >> + ... >> + acpi_handle handle = ACPI_HANDLE(&pdev->dev); >> + struct device_node node = pdev->dev.of_node; >> + ... >> + >> + if (node) >> + ret = device_probe_dt(pdev); >> + else if (handle) >> + ret = device_probe_acpi(pdev); >> + else >> + /* other initialisation */ >> + ... >> + /* Continue with any generic probe operations */ >> + ... >> +} >> + >> +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear >> +the different names the driver is probed for, both from DT and from ACPI. >> + >> +module device tables :- >> + >> +static struct of_device_id virtio_mmio_match[] = { >> + { .compatible = "virtio,mmio", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, virtio_mmio_match); >> + >> +static const struct acpi_device_id virtio_mmio_acpi_match[] = { >> + { "LNRO0005", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match); >> + >> +TODO: Add any other helpful rules that develop from Juno ACPI work. >> + >> +ASWG >> +---- >> + >> +The following areas are not yet well defined for ARM in the current ACPI >> +specification and are expected to be worked through in the UEFI ACPI >> +Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>. >> +Participation in this group is open to all UEFI members. >> + >> + - ACPI based CPU topology >> + - ACPI based Power management >> + - CPU idle control based on PSCI >> + - CPU performance control (CPPC) >> + >> +No code shall be accepted into the kernel unless it complies with the released >> +standards from UEFI ASWG. If there are features missing from ACPI to make it >> +function on a platform ECRs should be submitted to ASWG and go through the > > on a platform, ECRs > >> +approval process. >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > From: Graeme Gregory <graeme.gregory@linaro.org> > > Add documentation for the guidelines of how to use ACPI > on ARM64. As the most vocal participant against ACPI being adopted, I would have appreciated a cc on this patch set -- it's not like you were going for a minimal set of cc recipients already. It makes it seem like you're trying to sneak it past me for comments. Not cool. I know that's probably not your intent, but still. Some comments below. Overall the doc looks pretty good, but the details about _DSD and clocks are somewhat worrisome. > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 240 insertions(+) > create mode 100644 Documentation/arm64/arm-acpi.txt > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > new file mode 100644 > index 0000000..12cd550 > --- /dev/null > +++ b/Documentation/arm64/arm-acpi.txt > @@ -0,0 +1,240 @@ > +ACPI on ARMv8 Servers > +--------------------- > + > +ACPI will be used for ARMv8 general purpose servers designed to follow "ACPI might be used" or "can be used" > +the SBSA specification (currently available to people with an ARM login at > +http://silver.arm.com) > + > +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, > +which is available at <http://www.uefi.org/acpi/specs>. The implemented version where? The kernel implements that version? Worth clarifying. > +If the machine does not meet these requirements then it is likely that Device > +Tree (DT) is more suitable for the hardware. This is should be a very clear statement that is currently vague w.r.t. which requirements are met or not, especially based on the sentence above. > +Relationship with Device Tree > +----------------------------- > + > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > +exclusive with DT support at compile time. > + > +At boot time the kernel will only use one description method depending on > +parameters passed from the bootloader. Possibly overriden by kernel bootargs. And as debated for quite a while earlier this year, acpi should still default to off -- if a DT and ACPI are both passed in, DT should at this time be given priority. (Where can I learn more about how the boot loaders currently handle this? Do some of them pass in both DT and ACPI on some platforms, for example?) > +Regardless of whether DT or ACPI is used, the kernel must always be capable > +of booting with either scheme. It should always be possible to compile out ACPI. There will be plenty of platforms that will not implement it, so disabling CONFIG_ACPI needs to be possible. > +When booting using ACPI tables the /chosen node in DT will still be parsed > +to extract the kernel command line and initrd path. No other section of > +the DT will be used. > + > +Booting using ACPI tables > +------------------------- > + > +Currently, the only defined method to pass ACPI tables to the kernel on ARMv8 > +is via the UEFI system configuration table. > + > +The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the > +RSDP table (the table with the ACPI signature "RSD PTR "). > + > +The pointer to the RSDP table will be retrieved from EFI by the ACPI core. > + > +Processing of ACPI tables may be disabled by passing acpi=off on the kernel > +command line. > + > +DO use an XSDT, RSDTs are deprecated and should not be used on arm64. They > +only allow for 32bit addresses. > + > +DO NOT use the 32-bit address fields in the FADT, they are deprecated, the > +64-bit alternatives MUST be used. > + > +The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT > +and GTDT. If PCI is used the MCFG table MUST also be present. > + > +ACPI Detection > +-------------- > + > +Drivers should determine their probe() type by checking for ACPI_HANDLE, > +or .of_node, or other information in the device structure. This is > +detailed further in the "Driver Recomendations" section. > + > +If the presence of ACPI needs to be detected at runtime, then check the value > +of acpi_disabled. If CONFIG_ACPI not being set acpi_disabled will always be 1. Just to make sure, if acpi_disabled is 0, then there will be no acpi handle associated with the device, right? I.e. there should be no need to have every single driver check for whether ACPI is disabled, the handle check should just fail instead. > +Device Enumeration > +------------------ > + > +Device descriptions in ACPI should use standard recognised ACPI interfaces. > +These are far simpler than the information provided via Device Tree. Drivers > +should take into account this simplicity and work with sensible defaults. > + > +On no account should a Device Tree attempt to be replicated in ASL using such > +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. I see these two sentences as contradictory, given that the _DSD doc linked below contains examples that mirror over several properties, such as "linux,default-trigger" and other LED-specific properties. (section 2.4.2 in the below doc). "default-state" seems to come from DT too? Care to elaborate and explain what the intention here is? This could worst case turn into quite a mess. Given that ACPI can present completely different data based on what OS is running, it's quite common to indeed have OS specific data in there. How does that relate to this document and these practices? > +Common _DSD bindings should be submitted to ASWG to be included in the > +document :- > + > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm So, for these that are a mirror of the device tree bindings, there needs to be a wording here around reviewing the DT binding first so we don't get diverging bindings. > +TODO: Clarification and examples from Juno implementation. > + > +Programmable Power Control Resources > +------------------------------------ > + > +Programmable power control resources include such resources as voltage/current > +providers (regulators) and clock sources. > + > +For power control of these resources they should be represented with Power > +Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly > +enabling/disabling of resources as they are needed. > + > +There exists in the ACPI 5.1 specification no standard binding for these objects > +to enable programmable levels or rates so this should be avoid if possible and > +the resources set to appropriate level by the firmware. If this is not possible > +then any manipulation should be abstracted in ASL. > + > +Each device in ACPI has D-states and these can be controlled through > +the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off. > + > +If either _PS0 or _PS3 is implemented, then the other method must also be > +implemented. > + > +If a device requires usage or setup of a power resource when on, the ASL > +should organise that it is allocated/enabled using the _PS0 method. > + > +Resources allocated/enabled in the _PS0 method should be disabled/de-allocated > +in the _PS3 method. > + > +Such code in _PS? methods will of course be very platform specific but > +should allow the driver to operate the device without special non standard > +values being read from ASL. Further, abstracting the use of these resources > +allows hardware revisions without requiring updates to the kernel. > + > +TODO: Clarification and examples from Juno implementation. > + > +Clocks > +------ > + > +Like clocks that are part of the power resources there is no standard way > +to represent a clock tree in ACPI 5.1 in a similar manner to how it is > +described in DT. > + > +Devices affected by this include things like UARTs, SoC driven LCD displays, > +etc. > + > +The firmware for example UEFI should initialise these clocks to fixed working Odd wording. Maube "The firmware (for example UEFI) should..." > +values before the kernel is executed. If a driver requires to know rates of > +clocks set by firmware then they can be passed to kernel using _DSD. > + > +example :- > + > +Device (CLK0) { > + ... > + > + Name (_DSD, Package() { > + ToUUID("XXXXX"), > + Package() { > + Package(2) {"#clock-cells", 0}, Clock-cells? What do they mean here? Is this specified in the ACPI standards? I had to register to get access to it, and didn't feel like doing that right now. I guess it's not _all_ that open a spec. :( > + Package(2) {"clock-frequency", "10000"} > + } > + }) > + > + ... > +} > + > +Device (USR1) { > + ... > + > + Name (_DSD, Package() { > + ToUUID("XXXXX"), > + Package() { > + Package(2) {"clocks", Package() {1, ^CLK0}}}, A clock is a device in the ACPI model? Why not just provide the rate as data into the device here? You said you're not trying to model the clock tree, so why reference an external node for it? > + } > + }) > + > + ... > +} > + > +Driver Recommendations > +---------------------- > + > +DO NOT remove any FDT handling when adding ACPI support for a driver, different > +systems may use the same device. > + > +DO try and keep complex sections of ACPI and DT functionality seperate. This > +may mean a patch to break out some complex DT to another function before > +the patch to add ACPI. This may happen in other functions but is most likely > +in probe function. This gives a clearer flow of data for reviewing driver > +source. > + > +probe() :- > + > +TODO: replace this with a specific real example from Juno? > + > +static int device_probe_dt(struct platform_device *pdev) > +{ > + /* DT specific functionality */ > + ... > +} > + > +static int device_probe_acpi(struct platform_device *pdev) > +{ > + /* ACPI specific functionality */ > + ... > +} > + > +static int device_probe(stuct platform_device *pdev) > +{ > + ... > + acpi_handle handle = ACPI_HANDLE(&pdev->dev); > + struct device_node node = pdev->dev.of_node; > + ... > + > + if (node) > + ret = device_probe_dt(pdev); > + else if (handle) > + ret = device_probe_acpi(pdev); > + else > + /* other initialisation */ > + ... > + /* Continue with any generic probe operations */ > + ... > +} This looks good to me, and it's also my preferred way of ACPI-enabling drivers. I guess we might discuss this at KS since it was a proposed topic there, and others will object. :) > + > +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear > +the different names the driver is probed for, both from DT and from ACPI. > + > +module device tables :- > + > +static struct of_device_id virtio_mmio_match[] = { > + { .compatible = "virtio,mmio", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, virtio_mmio_match); > + > +static const struct acpi_device_id virtio_mmio_acpi_match[] = { > + { "LNRO0005", }, > + { } No comma here, but a comma on DT. Probably make them equivalent for consistency (including space between the braces). > +}; > +MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match); > + > +TODO: Add any other helpful rules that develop from Juno ACPI work. Looks like this should be fixed before the patch is merged, or this TODO removed. > + > +ASWG > +---- > + > +The following areas are not yet well defined for ARM in the current ACPI > +specification and are expected to be worked through in the UEFI ACPI > +Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>. > +Participation in this group is open to all UEFI members. > + > + - ACPI based CPU topology > + - ACPI based Power management > + - CPU idle control based on PSCI > + - CPU performance control (CPPC) > + > +No code shall be accepted into the kernel unless it complies with the released > +standards from UEFI ASWG. If there are features missing from ACPI to make it > +function on a platform ECRs should be submitted to ASWG and go through the > +approval process. Thanks for listing the things that are not in place yet. Please keep this doc up to date as new areas are discovered. -Olof
On 27/07/2014 03:34, Olof Johansson wrote: > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> From: Graeme Gregory <graeme.gregory@linaro.org> >> >> Add documentation for the guidelines of how to use ACPI >> on ARM64. > As the most vocal participant against ACPI being adopted, I would have > appreciated a cc on this patch set -- it's not like you were going for > a minimal set of cc recipients already. It makes it seem like you're > trying to sneak it past me for comments. Not cool. I know that's > probably not your intent, but still. > > Some comments below. Overall the doc looks pretty good, but the > details about _DSD and clocks are somewhat worrisome. > >> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 240 insertions(+) >> create mode 100644 Documentation/arm64/arm-acpi.txt >> >> diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt >> new file mode 100644 >> index 0000000..12cd550 >> --- /dev/null >> +++ b/Documentation/arm64/arm-acpi.txt >> @@ -0,0 +1,240 @@ >> +ACPI on ARMv8 Servers >> +--------------------- >> + >> +ACPI will be used for ARMv8 general purpose servers designed to follow > "ACPI might be used" or "can be used" > >> +the SBSA specification (currently available to people with an ARM login at >> +http://silver.arm.com) >> + >> +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, >> +which is available at <http://www.uefi.org/acpi/specs>. > The implemented version where? The kernel implements that version? > Worth clarifying. ok the tables passed must be acpi 5.1+, the kernel must then obviously implement the 5.1 features, will clarify. >> +If the machine does not meet these requirements then it is likely that Device >> +Tree (DT) is more suitable for the hardware. > This is should be a very clear statement that is currently vague > w.r.t. which requirements are met or not, especially based on the > sentence above. The SBSA is the set of requirements, will clarify. >> +Relationship with Device Tree >> +----------------------------- >> + >> +ACPI support in drivers and subsystems for ARMv8 should never be mutually >> +exclusive with DT support at compile time. >> + >> +At boot time the kernel will only use one description method depending on >> +parameters passed from the bootloader. > Possibly overriden by kernel bootargs. And as debated for quite a > while earlier this year, acpi should still default to off -- if a DT > and ACPI are both passed in, DT should at this time be given priority. This does not work due to DT being misused as the kernel/bootloader communication layer as well. So a DT is always passed to the kernel. We cannot tell whether this is a useful DT without unpacking it and trying to boot platform from it. There is an acpi=off parameter that can be passed to always disable acpi runtime. > (Where can I learn more about how the boot loaders currently handle > this? Do some of them pass in both DT and ACPI on some platforms, for > example?) Currently only one bootloader protocol is supported for ACPI and thats UEFI. As noted above due to abuse of DT in the /chosen/ node a DT is always passed to the kernel. >> +Regardless of whether DT or ACPI is used, the kernel must always be capable >> +of booting with either scheme. > It should always be possible to compile out ACPI. There will be plenty > of platforms that will not implement it, so disabling CONFIG_ACPI > needs to be possible. This will always be possible! >> +When booting using ACPI tables the /chosen node in DT will still be parsed >> +to extract the kernel command line and initrd path. No other section of >> +the DT will be used. >> + >> +Booting using ACPI tables >> +------------------------- >> + >> +Currently, the only defined method to pass ACPI tables to the kernel on ARMv8 >> +is via the UEFI system configuration table. >> + >> +The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the >> +RSDP table (the table with the ACPI signature "RSD PTR "). >> + >> +The pointer to the RSDP table will be retrieved from EFI by the ACPI core. >> + >> +Processing of ACPI tables may be disabled by passing acpi=off on the kernel >> +command line. >> + >> +DO use an XSDT, RSDTs are deprecated and should not be used on arm64. They >> +only allow for 32bit addresses. >> + >> +DO NOT use the 32-bit address fields in the FADT, they are deprecated, the >> +64-bit alternatives MUST be used. >> + >> +The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT >> +and GTDT. If PCI is used the MCFG table MUST also be present. >> + >> +ACPI Detection >> +-------------- >> + >> +Drivers should determine their probe() type by checking for ACPI_HANDLE, >> +or .of_node, or other information in the device structure. This is >> +detailed further in the "Driver Recomendations" section. >> + >> +If the presence of ACPI needs to be detected at runtime, then check the value >> +of acpi_disabled. If CONFIG_ACPI not being set acpi_disabled will always be 1. > Just to make sure, if acpi_disabled is 0, then there will be no acpi > handle associated with the device, right? I.e. there should be no need > to have every single driver check for whether ACPI is disabled, the > handle check should just fail instead. I need to clarify this obviously, I meant for the second paragraph for that to be for code outside driver probing. Inside drivers they should only check for ACPI_HANDLE presence. But other bits of code espcially bits parsing tables for information in early boot should check for acpi_disabled before hunting for the tables etc. >> +Device Enumeration >> +------------------ >> + >> +Device descriptions in ACPI should use standard recognised ACPI interfaces. >> +These are far simpler than the information provided via Device Tree. Drivers >> +should take into account this simplicity and work with sensible defaults. >> + >> +On no account should a Device Tree attempt to be replicated in ASL using such >> +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific >> +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or >> +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. > I see these two sentences as contradictory, given that the _DSD doc > linked below contains examples that mirror over several properties, > such as "linux,default-trigger" and other LED-specific properties. > (section 2.4.2 in the below doc). "default-state" seems to come from > DT too? > > Care to elaborate and explain what the intention here is? This could > worst case turn into quite a mess. > > Given that ACPI can present completely different data based on what OS > is running, it's quite common to indeed have OS specific data in > there. How does that relate to this document and these practices? OS specific data has traditionally not worked out well for ACPI, I would like to "persuade" people not to use it on ARM. The _DSD was quite late to the standards process and the supporting documentation is still catching up. We are working with ARM to bring these issues up and to define proper OS agnostic bindings for ARM. >> +Common _DSD bindings should be submitted to ASWG to be included in the >> +document :- >> + >> +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm > So, for these that are a mirror of the device tree bindings, there > needs to be a wording here around reviewing the DT binding first so we > don't get diverging bindings. > >> +TODO: Clarification and examples from Juno implementation. >> + >> +Programmable Power Control Resources >> +------------------------------------ >> + >> +Programmable power control resources include such resources as voltage/current >> +providers (regulators) and clock sources. >> + >> +For power control of these resources they should be represented with Power >> +Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly >> +enabling/disabling of resources as they are needed. >> + >> +There exists in the ACPI 5.1 specification no standard binding for these objects >> +to enable programmable levels or rates so this should be avoid if possible and >> +the resources set to appropriate level by the firmware. If this is not possible >> +then any manipulation should be abstracted in ASL. >> + >> +Each device in ACPI has D-states and these can be controlled through >> +the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off. >> + >> +If either _PS0 or _PS3 is implemented, then the other method must also be >> +implemented. >> + >> +If a device requires usage or setup of a power resource when on, the ASL >> +should organise that it is allocated/enabled using the _PS0 method. >> + >> +Resources allocated/enabled in the _PS0 method should be disabled/de-allocated >> +in the _PS3 method. >> + >> +Such code in _PS? methods will of course be very platform specific but >> +should allow the driver to operate the device without special non standard >> +values being read from ASL. Further, abstracting the use of these resources >> +allows hardware revisions without requiring updates to the kernel. >> + >> +TODO: Clarification and examples from Juno implementation. >> + >> +Clocks >> +------ >> + >> +Like clocks that are part of the power resources there is no standard way >> +to represent a clock tree in ACPI 5.1 in a similar manner to how it is >> +described in DT. >> + >> +Devices affected by this include things like UARTs, SoC driven LCD displays, >> +etc. >> + >> +The firmware for example UEFI should initialise these clocks to fixed working > Odd wording. Maube "The firmware (for example UEFI) should..." agreed! >> +values before the kernel is executed. If a driver requires to know rates of >> +clocks set by firmware then they can be passed to kernel using _DSD. >> + >> +example :- >> + >> +Device (CLK0) { >> + ... >> + >> + Name (_DSD, Package() { >> + ToUUID("XXXXX"), >> + Package() { >> + Package(2) {"#clock-cells", 0}, > Clock-cells? What do they mean here? Is this specified in the ACPI > standards? I had to register to get access to it, and didn't feel like > doing that right now. I guess it's not _all_ that open a spec. :( > >> + Package(2) {"clock-frequency", "10000"} >> + } >> + }) >> + >> + ... >> +} >> + >> +Device (USR1) { >> + ... >> + >> + Name (_DSD, Package() { >> + ToUUID("XXXXX"), >> + Package() { >> + Package(2) {"clocks", Package() {1, ^CLK0}}}, > A clock is a device in the ACPI model? Why not just provide the rate > as data into the device here? You said you're not trying to model the > clock tree, so why reference an external node for it? This section is still a bit WIP due to the above noted issues with _DSD documentation catching up with the standards process. I will need to work with the clock maintainers to see if we can agree a proper set of bindings for this. #blah-cells always was my least favorite DT feature. >> + } >> + }) >> + >> + ... >> +} >> + >> +Driver Recommendations >> +---------------------- >> + >> +DO NOT remove any FDT handling when adding ACPI support for a driver, different >> +systems may use the same device. >> + >> +DO try and keep complex sections of ACPI and DT functionality seperate. This >> +may mean a patch to break out some complex DT to another function before >> +the patch to add ACPI. This may happen in other functions but is most likely >> +in probe function. This gives a clearer flow of data for reviewing driver >> +source. >> + >> +probe() :- >> + >> +TODO: replace this with a specific real example from Juno? >> + >> +static int device_probe_dt(struct platform_device *pdev) >> +{ >> + /* DT specific functionality */ >> + ... >> +} >> + >> +static int device_probe_acpi(struct platform_device *pdev) >> +{ >> + /* ACPI specific functionality */ >> + ... >> +} >> + >> +static int device_probe(stuct platform_device *pdev) >> +{ >> + ... >> + acpi_handle handle = ACPI_HANDLE(&pdev->dev); >> + struct device_node node = pdev->dev.of_node; >> + ... >> + >> + if (node) >> + ret = device_probe_dt(pdev); >> + else if (handle) >> + ret = device_probe_acpi(pdev); >> + else >> + /* other initialisation */ >> + ... >> + /* Continue with any generic probe operations */ >> + ... >> +} > This looks good to me, and it's also my preferred way of ACPI-enabling > drivers. I guess we might discuss this at KS since it was a proposed > topic there, and others will object. :) Hopefully someone can summarise the discussion at KS for me, I will not be there. >> + >> +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear >> +the different names the driver is probed for, both from DT and from ACPI. >> + >> +module device tables :- >> + >> +static struct of_device_id virtio_mmio_match[] = { >> + { .compatible = "virtio,mmio", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, virtio_mmio_match); >> + >> +static const struct acpi_device_id virtio_mmio_acpi_match[] = { >> + { "LNRO0005", }, >> + { } > No comma here, but a comma on DT. Probably make them equivalent for > consistency (including space between the braces). ok >> +}; >> +MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match); >> + >> +TODO: Add any other helpful rules that develop from Juno ACPI work. > Looks like this should be fixed before the patch is merged, or this > TODO removed. The plan is to fix these TODOs with actual data from Juno as that will be in the UEFI for Juno. >> + >> +ASWG >> +---- >> + >> +The following areas are not yet well defined for ARM in the current ACPI >> +specification and are expected to be worked through in the UEFI ACPI >> +Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>. >> +Participation in this group is open to all UEFI members. >> + >> + - ACPI based CPU topology >> + - ACPI based Power management >> + - CPU idle control based on PSCI >> + - CPU performance control (CPPC) >> + >> +No code shall be accepted into the kernel unless it complies with the released >> +standards from UEFI ASWG. If there are features missing from ACPI to make it >> +function on a platform ECRs should be submitted to ASWG and go through the >> +approval process. > Thanks for listing the things that are not in place yet. Please keep > this doc up to date as new areas are discovered. > > > -Olof Thanks for the feedback, we shall work to incorporate it into the document. Graeme -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Olof, On Sun, Jul 27, 2014 at 03:34:48AM +0100, Olof Johansson wrote: > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > From: Graeme Gregory <graeme.gregory@linaro.org> > > > > Add documentation for the guidelines of how to use ACPI > > on ARM64. > > As the most vocal participant against ACPI being adopted, I would have > appreciated a cc on this patch set -- it's not like you were going for > a minimal set of cc recipients already. It makes it seem like you're > trying to sneak it past me for comments. Not cool. I know that's > probably not your intent, but still. > > Some comments below. Overall the doc looks pretty good, but the > details about _DSD and clocks are somewhat worrisome. > > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > --- > > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 240 insertions(+) > > create mode 100644 Documentation/arm64/arm-acpi.txt > > > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > > new file mode 100644 > > index 0000000..12cd550 > > --- /dev/null > > +++ b/Documentation/arm64/arm-acpi.txt > > @@ -0,0 +1,240 @@ > > +ACPI on ARMv8 Servers > > +--------------------- > > + > > +ACPI will be used for ARMv8 general purpose servers designed to follow > > "ACPI might be used" or "can be used" > > > +the SBSA specification (currently available to people with an ARM login at > > +http://silver.arm.com) > > + > > +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, > > +which is available at <http://www.uefi.org/acpi/specs>. > > The implemented version where? The kernel implements that version? > Worth clarifying. > > > +If the machine does not meet these requirements then it is likely that Device > > +Tree (DT) is more suitable for the hardware. > > This is should be a very clear statement that is currently vague > w.r.t. which requirements are met or not, especially based on the > sentence above. > > > +Relationship with Device Tree > > +----------------------------- > > + > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > +exclusive with DT support at compile time. > > + > > +At boot time the kernel will only use one description method depending on > > +parameters passed from the bootloader. > > Possibly overriden by kernel bootargs. And as debated for quite a > while earlier this year, acpi should still default to off -- if a DT > and ACPI are both passed in, DT should at this time be given priority. Why? I really don't see the logic in doing that. Currently the kernel can only boot using DT, so DT will be used even if ACPI is present. In the presence of ACPI support in the kernel, ACPI would be used on said systems. From the discussions at the last Linaro Connect, this was seen as important for virtual machines which want to provide ACPI services to guests while still being able to boot DT-only kernels. I'll leave it to Grant, Rob, and Christoffer to cover that. > (Where can I learn more about how the boot loaders currently handle > this? Do some of them pass in both DT and ACPI on some platforms, for > example?) All will pass in some form of DT. If booted through UEFI, the DT will be augmented with data about the UEFI memory map (and if no DT is provided, a /chosen only DT will eb created by the EFI stub). A kernel with ACPI support will query EFI for ACPI tables, and if found use them. > > +Regardless of whether DT or ACPI is used, the kernel must always be capable > > +of booting with either scheme. > > It should always be possible to compile out ACPI. There will be plenty > of platforms that will not implement it, so disabling CONFIG_ACPI > needs to be possible. A better description would be that the two must never be mutually exclusive. It must always be possible to have a kernel which supports both, and code must never assume the absence of the other. At run time, the kernel will decide to use one for system description and use that. [...] > > +Device Enumeration > > +------------------ > > + > > +Device descriptions in ACPI should use standard recognised ACPI interfaces. > > +These are far simpler than the information provided via Device Tree. Drivers > > +should take into account this simplicity and work with sensible defaults. > > + > > +On no account should a Device Tree attempt to be replicated in ASL using such > > +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific > > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > > +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. > > I see these two sentences as contradictory, given that the _DSD doc > linked below contains examples that mirror over several properties, > such as "linux,default-trigger" and other LED-specific properties. > (section 2.4.2 in the below doc). "default-state" seems to come from > DT too? Urrgh. Those should never been placed in the ACPI spec. It's bad enough in DT. > Care to elaborate and explain what the intention here is? This could > worst case turn into quite a mess. > > Given that ACPI can present completely different data based on what OS > is running, it's quite common to indeed have OS specific data in > there. How does that relate to this document and these practices? > > > +Common _DSD bindings should be submitted to ASWG to be included in the > > +document :- > > + > > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm > > So, for these that are a mirror of the device tree bindings, there > needs to be a wording here around reviewing the DT binding first so we > don't get diverging bindings. We shouldn't work on the assumption that the two should be identical. ACPI already has standard mechanisms for describing certain linkages that are divergent from DT. This really needs higher-level thought, and I'd hoped that things wouldn't blindly be copied one way or the other. [...] > > +values before the kernel is executed. If a driver requires to know rates of > > +clocks set by firmware then they can be passed to kernel using _DSD. > > + > > +example :- > > + > > +Device (CLK0) { > > + ... > > + > > + Name (_DSD, Package() { > > + ToUUID("XXXXX"), > > + Package() { > > + Package(2) {"#clock-cells", 0}, > > Clock-cells? What do they mean here? Is this specified in the ACPI > standards? I had to register to get access to it, and didn't feel like > doing that right now. I guess it's not _all_ that open a spec. :( Yeah, this is complete nonsense. If people are going to blindly copy elements from DT into ACPI without actually understanding them, then ACPI is clearly no better than DT. [...] > > +static int device_probe(stuct platform_device *pdev) > > +{ > > + ... > > + acpi_handle handle = ACPI_HANDLE(&pdev->dev); > > + struct device_node node = pdev->dev.of_node; > > + ... > > + > > + if (node) > > + ret = device_probe_dt(pdev); > > + else if (handle) > > + ret = device_probe_acpi(pdev); > > + else > > + /* other initialisation */ > > + ... > > + /* Continue with any generic probe operations */ > > + ... > > +} > > This looks good to me, and it's also my preferred way of ACPI-enabling > drivers. I guess we might discuss this at KS since it was a proposed > topic there, and others will object. :) This is almost precisely the form of probe I want to see (it would be nicer IMO to have separate entry points for ACPI / DT / platform data that can all call a common probe core, but this isn't too far off). > > > + > > +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear > > +the different names the driver is probed for, both from DT and from ACPI. > > + > > +module device tables :- > > + > > +static struct of_device_id virtio_mmio_match[] = { > > + { .compatible = "virtio,mmio", }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, virtio_mmio_match); > > + > > +static const struct acpi_device_id virtio_mmio_acpi_match[] = { > > + { "LNRO0005", }, > > + { } > > No comma here, but a comma on DT. Probably make them equivalent for > consistency (including space between the braces). The comma on the DT form should probably be dropped. The NULL sentinel's only job is to delimit the list, so it never makes sense to place something after it. Cheers, Mark.
On Mon, Jul 28, 2014 at 10:07:50AM +0100, Arnd Bergmann wrote: > On Saturday 26 July 2014 19:34:48 Olof Johansson wrote: > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > > +Relationship with Device Tree > > > +----------------------------- > > > + > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > +exclusive with DT support at compile time. > > > + > > > +At boot time the kernel will only use one description method depending on > > > +parameters passed from the bootloader. > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > while earlier this year, acpi should still default to off -- if a DT > > and ACPI are both passed in, DT should at this time be given priority. > > I think this would be harder to do with the way that ACPI is passed in > to the kernel. IIRC, you always have a minimal DT information based on > the ARM64 boot protocol, but in the case of ACPI, this contains pointers > to the ACPI tables, which are then used for populating the Linux platform > devices (unless acpi=disabled is set), while the other contents of the > DTB may be present but we skip the of_platform_populate state. > > If this is correct, then replacing the firmware-generated dtb with a > user-provided on would implicitly remove the ACPI tables from visibility, > which is exactly what we want. That's not quite true. There might not be any DTB, or the user might have provided a DTB with only /chosen/bootargs. Trying to distinguish the many cases of possible DTB is broken as a concept. The EFI stub will attempt to get a DTB from somewhere (provided by filename or as a system table with a magic UUID), and if it can't find one will generate an empty stub DTB. Then it will go and perform some EFI memory setup, placing some properties in the DTB (which might be a stub) describing the EFI memory map. Then we boot the kernel proper, which sees the EFI pointers and looks for an ACPI table. If they are found, ACPI is used. Otherwise we attempt to use the DTB (which might be a stub). Unless ACPI is explcitly disabled, ACPI will be used if present. Thanks, Mark.
On Mon, Jul 28, 2014 at 09:42:57AM +0100, Graeme Gregory wrote: > > On 27/07/2014 03:34, Olof Johansson wrote: > >On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > >>From: Graeme Gregory <graeme.gregory@linaro.org> > >> > >>Add documentation for the guidelines of how to use ACPI > >>on ARM64. > >As the most vocal participant against ACPI being adopted, I would have > >appreciated a cc on this patch set -- it's not like you were going for > >a minimal set of cc recipients already. It makes it seem like you're > >trying to sneak it past me for comments. Not cool. I know that's > >probably not your intent, but still. > > > >Some comments below. Overall the doc looks pretty good, but the > >details about _DSD and clocks are somewhat worrisome. > > > >>Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > >>Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > >>--- > >> Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 240 insertions(+) > >> create mode 100644 Documentation/arm64/arm-acpi.txt > >> > >>diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > >>new file mode 100644 > >>index 0000000..12cd550 > >>--- /dev/null > >>+++ b/Documentation/arm64/arm-acpi.txt > >>@@ -0,0 +1,240 @@ > >>+ACPI on ARMv8 Servers > >>+--------------------- > >>+ > >>+ACPI will be used for ARMv8 general purpose servers designed to follow > >"ACPI might be used" or "can be used" > > > >>+the SBSA specification (currently available to people with an ARM login at > >>+http://silver.arm.com) > >>+ > >>+The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, > >>+which is available at <http://www.uefi.org/acpi/specs>. > >The implemented version where? The kernel implements that version? > >Worth clarifying. > ok > > the tables passed must be acpi 5.1+, the kernel must then obviously > implement the 5.1 features, will clarify. > >>+If the machine does not meet these requirements then it is likely that Device > >>+Tree (DT) is more suitable for the hardware. > >This is should be a very clear statement that is currently vague > >w.r.t. which requirements are met or not, especially based on the > >sentence above. > The SBSA is the set of requirements, will clarify. > > >>+Relationship with Device Tree > >>+----------------------------- > >>+ > >>+ACPI support in drivers and subsystems for ARMv8 should never be mutually > >>+exclusive with DT support at compile time. > >>+ > >>+At boot time the kernel will only use one description method depending on > >>+parameters passed from the bootloader. > >Possibly overriden by kernel bootargs. And as debated for quite a > >while earlier this year, acpi should still default to off -- if a DT > >and ACPI are both passed in, DT should at this time be given priority. > This does not work due to DT being misused as the kernel/bootloader > communication layer as well. So a DT is always passed to the kernel. > We cannot tell whether this is a useful DT without unpacking it and > trying to boot platform from it. > > There is an acpi=off parameter that can be passed to always disable > acpi runtime. Right, but the agreement we had from earlier this year was to keep ACPI default off until we've seen at least the first generation of real hardware, since we have no confidence that the system vendors will do sane things with ACPI yet. If they completely mess it up, we at least will retain basic functionality without accuring huge technical debt dealing with the messed up ACPI tables forever. In other words, to boot with ACPI enabled, the requrement is that you have to pass 'acpi' or 'acpi=on' on the command line (or equivalent). Not the other way around -- to have it on by default and give an option to turn it off. If you need a refresher: http://www.secretlab.ca/archives/27 > >(Where can I learn more about how the boot loaders currently handle > >this? Do some of them pass in both DT and ACPI on some platforms, for > >example?) > Currently only one bootloader protocol is supported for ACPI and > thats UEFI. As noted above due to abuse of DT in the /chosen/ node a > DT is always passed to the kernel. The above is hard to understand. I suppose I should restate my question: I presume actual end users will use something like Grub2 to load a kernel/ramdisk/dtb. Where can I learn more about how it handles ACPI? > >>+Regardless of whether DT or ACPI is used, the kernel must always be capable > >>+of booting with either scheme. > >It should always be possible to compile out ACPI. There will be plenty > >of platforms that will not implement it, so disabling CONFIG_ACPI > >needs to be possible. > This will always be possible! Yes, I commented because it should also be clear in the doc. > >>+When booting using ACPI tables the /chosen node in DT will still be parsed > >>+to extract the kernel command line and initrd path. No other section of > >>+the DT will be used. > >>+ > >>+Booting using ACPI tables > >>+------------------------- > >>+ > >>+Currently, the only defined method to pass ACPI tables to the kernel on ARMv8 > >>+is via the UEFI system configuration table. > >>+ > >>+The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the > >>+RSDP table (the table with the ACPI signature "RSD PTR "). > >>+ > >>+The pointer to the RSDP table will be retrieved from EFI by the ACPI core. > >>+ > >>+Processing of ACPI tables may be disabled by passing acpi=off on the kernel > >>+command line. > >>+ > >>+DO use an XSDT, RSDTs are deprecated and should not be used on arm64. They > >>+only allow for 32bit addresses. > >>+ > >>+DO NOT use the 32-bit address fields in the FADT, they are deprecated, the > >>+64-bit alternatives MUST be used. > >>+ > >>+The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT > >>+and GTDT. If PCI is used the MCFG table MUST also be present. > >>+ > >>+ACPI Detection > >>+-------------- > >>+ > >>+Drivers should determine their probe() type by checking for ACPI_HANDLE, > >>+or .of_node, or other information in the device structure. This is > >>+detailed further in the "Driver Recomendations" section. > >>+ > >>+If the presence of ACPI needs to be detected at runtime, then check the value > >>+of acpi_disabled. If CONFIG_ACPI not being set acpi_disabled will always be 1. > >Just to make sure, if acpi_disabled is 0, then there will be no acpi > >handle associated with the device, right? I.e. there should be no need > >to have every single driver check for whether ACPI is disabled, the > >handle check should just fail instead. > I need to clarify this obviously, I meant for the second paragraph > for that to be for code outside driver probing. Inside drivers they > should only check for ACPI_HANDLE presence. But other bits of code > espcially bits parsing tables for information in early boot should > check for acpi_disabled before hunting for the tables etc. Yep, that sounds sane. > >>+Device Enumeration > >>+------------------ > >>+ > >>+Device descriptions in ACPI should use standard recognised ACPI interfaces. > >>+These are far simpler than the information provided via Device Tree. Drivers > >>+should take into account this simplicity and work with sensible defaults. > >>+ > >>+On no account should a Device Tree attempt to be replicated in ASL using such > >>+constructs as Name(KEY0, "Value1") type constructs. Additional driver specific > >>+data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > >>+_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. > >I see these two sentences as contradictory, given that the _DSD doc > >linked below contains examples that mirror over several properties, > >such as "linux,default-trigger" and other LED-specific properties. > >(section 2.4.2 in the below doc). "default-state" seems to come from > >DT too? > > > >Care to elaborate and explain what the intention here is? This could > >worst case turn into quite a mess. > > > >Given that ACPI can present completely different data based on what OS > >is running, it's quite common to indeed have OS specific data in > >there. How does that relate to this document and these practices? > OS specific data has traditionally not worked out well for ACPI, I > would like to "persuade" people not to use it on ARM. It hasn't? I think Microsoft disagrees. It's also how vendors have been able to present an older machine description to keep their newer hardware compatible with older software, isn't it? How do you expect to handle that if you can only present one table? It's the same challenge that DT has. > The _DSD was quite late to the standards process and the supporting > documentation is still catching up. We are working with ARM to bring > these issues up and to define proper OS agnostic bindings for ARM. I'm guessing that the first ARM should be ACPI? Or is ARM Ltd on critical path on this?! > >>+Common _DSD bindings should be submitted to ASWG to be included in the > >>+document :- > >>+ > >>+http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm > >So, for these that are a mirror of the device tree bindings, there > >needs to be a wording here around reviewing the DT binding first so we > >don't get diverging bindings. > > > >>+TODO: Clarification and examples from Juno implementation. > >>+ > >>+Programmable Power Control Resources > >>+------------------------------------ > >>+ > >>+Programmable power control resources include such resources as voltage/current > >>+providers (regulators) and clock sources. > >>+ > >>+For power control of these resources they should be represented with Power > >>+Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly > >>+enabling/disabling of resources as they are needed. > >>+ > >>+There exists in the ACPI 5.1 specification no standard binding for these objects > >>+to enable programmable levels or rates so this should be avoid if possible and > >>+the resources set to appropriate level by the firmware. If this is not possible > >>+then any manipulation should be abstracted in ASL. > >>+ > >>+Each device in ACPI has D-states and these can be controlled through > >>+the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off. > >>+ > >>+If either _PS0 or _PS3 is implemented, then the other method must also be > >>+implemented. > >>+ > >>+If a device requires usage or setup of a power resource when on, the ASL > >>+should organise that it is allocated/enabled using the _PS0 method. > >>+ > >>+Resources allocated/enabled in the _PS0 method should be disabled/de-allocated > >>+in the _PS3 method. > >>+ > >>+Such code in _PS? methods will of course be very platform specific but > >>+should allow the driver to operate the device without special non standard > >>+values being read from ASL. Further, abstracting the use of these resources > >>+allows hardware revisions without requiring updates to the kernel. > >>+ > >>+TODO: Clarification and examples from Juno implementation. > >>+ > >>+Clocks > >>+------ > >>+ > >>+Like clocks that are part of the power resources there is no standard way > >>+to represent a clock tree in ACPI 5.1 in a similar manner to how it is > >>+described in DT. > >>+ > >>+Devices affected by this include things like UARTs, SoC driven LCD displays, > >>+etc. > >>+ > >>+The firmware for example UEFI should initialise these clocks to fixed working > >Odd wording. Maube "The firmware (for example UEFI) should..." > agreed! > >>+values before the kernel is executed. If a driver requires to know rates of > >>+clocks set by firmware then they can be passed to kernel using _DSD. > >>+ > >>+example :- > >>+ > >>+Device (CLK0) { > >>+ ... > >>+ > >>+ Name (_DSD, Package() { > >>+ ToUUID("XXXXX"), > >>+ Package() { > >>+ Package(2) {"#clock-cells", 0}, > >Clock-cells? What do they mean here? Is this specified in the ACPI > >standards? I had to register to get access to it, and didn't feel like > >doing that right now. I guess it's not _all_ that open a spec. :( > > > >>+ Package(2) {"clock-frequency", "10000"} > >>+ } > >>+ }) > >>+ > >>+ ... > >>+} > >>+ > >>+Device (USR1) { > >>+ ... > >>+ > >>+ Name (_DSD, Package() { > >>+ ToUUID("XXXXX"), > >>+ Package() { > >>+ Package(2) {"clocks", Package() {1, ^CLK0}}}, > >A clock is a device in the ACPI model? Why not just provide the rate > >as data into the device here? You said you're not trying to model the > >clock tree, so why reference an external node for it? > This section is still a bit WIP due to the above noted issues with > _DSD documentation catching up with the standards process. I will > need to work with the clock maintainers to see if we can agree a > proper set of bindings for this. #blah-cells always was my least > favorite DT feature. Ok, work in progress is fine with me. But it is the reason for why we need to continue defaulting to ACPI off for now -- just because we don't want to have to support it in case some vendor picks up and use something half-baked while it's still being worked on. > >>+ } > >>+ }) > >>+ > >>+ ... > >>+} > >>+ > >>+Driver Recommendations > >>+---------------------- > >>+ > >>+DO NOT remove any FDT handling when adding ACPI support for a driver, different > >>+systems may use the same device. > >>+ > >>+DO try and keep complex sections of ACPI and DT functionality seperate. This > >>+may mean a patch to break out some complex DT to another function before > >>+the patch to add ACPI. This may happen in other functions but is most likely > >>+in probe function. This gives a clearer flow of data for reviewing driver > >>+source. > >>+ > >>+probe() :- > >>+ > >>+TODO: replace this with a specific real example from Juno? > >>+ > >>+static int device_probe_dt(struct platform_device *pdev) > >>+{ > >>+ /* DT specific functionality */ > >>+ ... > >>+} > >>+ > >>+static int device_probe_acpi(struct platform_device *pdev) > >>+{ > >>+ /* ACPI specific functionality */ > >>+ ... > >>+} > >>+ > >>+static int device_probe(stuct platform_device *pdev) > >>+{ > >>+ ... > >>+ acpi_handle handle = ACPI_HANDLE(&pdev->dev); > >>+ struct device_node node = pdev->dev.of_node; > >>+ ... > >>+ > >>+ if (node) > >>+ ret = device_probe_dt(pdev); > >>+ else if (handle) > >>+ ret = device_probe_acpi(pdev); > >>+ else > >>+ /* other initialisation */ > >>+ ... > >>+ /* Continue with any generic probe operations */ > >>+ ... > >>+} > >This looks good to me, and it's also my preferred way of ACPI-enabling > >drivers. I guess we might discuss this at KS since it was a proposed > >topic there, and others will object. :) > Hopefully someone can summarise the discussion at KS for me, I will > not be there. LWN.net usually has good summaries. I hope they'll be in the room this time too. If not, we'll send out a summary separately. > >>+ > >>+DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear > >>+the different names the driver is probed for, both from DT and from ACPI. > >>+ > >>+module device tables :- > >>+ > >>+static struct of_device_id virtio_mmio_match[] = { > >>+ { .compatible = "virtio,mmio", }, > >>+ {}, > >>+}; > >>+MODULE_DEVICE_TABLE(of, virtio_mmio_match); > >>+ > >>+static const struct acpi_device_id virtio_mmio_acpi_match[] = { > >>+ { "LNRO0005", }, > >>+ { } > >No comma here, but a comma on DT. Probably make them equivalent for > >consistency (including space between the braces). > ok > >>+}; > >>+MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match); > >>+ > >>+TODO: Add any other helpful rules that develop from Juno ACPI work. > >Looks like this should be fixed before the patch is merged, or this > >TODO removed. > The plan is to fix these TODOs with actual data from Juno as that > will be in the UEFI for Juno. > >>+ > >>+ASWG > >>+---- > >>+ > >>+The following areas are not yet well defined for ARM in the current ACPI > >>+specification and are expected to be worked through in the UEFI ACPI > >>+Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>. > >>+Participation in this group is open to all UEFI members. > >>+ > >>+ - ACPI based CPU topology > >>+ - ACPI based Power management > >>+ - CPU idle control based on PSCI > >>+ - CPU performance control (CPPC) > >>+ > >>+No code shall be accepted into the kernel unless it complies with the released > >>+standards from UEFI ASWG. If there are features missing from ACPI to make it > >>+function on a platform ECRs should be submitted to ASWG and go through the > >>+approval process. > >Thanks for listing the things that are not in place yet. Please keep > >this doc up to date as new areas are discovered. > > > > > >-Olof > Thanks for the feedback, we shall work to incorporate it into the document. Happy to help. Besides readiness to flip ACPI on by default, I don't think we're in substantial technical disagreements here. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 28, 2014 at 11:12:29AM +0100, Mark Rutland wrote: > On Mon, Jul 28, 2014 at 10:07:50AM +0100, Arnd Bergmann wrote: > > On Saturday 26 July 2014 19:34:48 Olof Johansson wrote: > > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > > > +Relationship with Device Tree > > > > +----------------------------- > > > > + > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > > +exclusive with DT support at compile time. > > > > + > > > > +At boot time the kernel will only use one description method depending on > > > > +parameters passed from the bootloader. > > > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > > while earlier this year, acpi should still default to off -- if a DT > > > and ACPI are both passed in, DT should at this time be given priority. > > > > I think this would be harder to do with the way that ACPI is passed in > > to the kernel. IIRC, you always have a minimal DT information based on > > the ARM64 boot protocol, but in the case of ACPI, this contains pointers > > to the ACPI tables, which are then used for populating the Linux platform > > devices (unless acpi=disabled is set), while the other contents of the > > DTB may be present but we skip the of_platform_populate state. > > > > If this is correct, then replacing the firmware-generated dtb with a > > user-provided on would implicitly remove the ACPI tables from visibility, > > which is exactly what we want. > > That's not quite true. > > There might not be any DTB, or the user might have provided a DTB with > only /chosen/bootargs. Trying to distinguish the many cases of possible > DTB is broken as a concept. > > The EFI stub will attempt to get a DTB from somewhere (provided by > filename or as a system table with a magic UUID), and if it can't find > one will generate an empty stub DTB. > > Then it will go and perform some EFI memory setup, placing some > properties in the DTB (which might be a stub) describing the EFI memory > map. > > Then we boot the kernel proper, which sees the EFI pointers and looks > for an ACPI table. If they are found, ACPI is used. Otherwise we attempt > to use the DTB (which might be a stub). > > Unless ACPI is explcitly disabled, ACPI will be used if present. Ah, I saw this after I replied to Arnd's email. The above sounds more like how I envisioned things working, so based on that, the default just needs to be flipped and we should be fine (i.e. ACPI needs to be explicitly enabled). -Olof
On Mon, Jul 28, 2014 at 11:06:54AM +0100, Mark Rutland wrote: > Hi Olof, > > On Sun, Jul 27, 2014 at 03:34:48AM +0100, Olof Johansson wrote: > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > > From: Graeme Gregory <graeme.gregory@linaro.org> > > > > > > Add documentation for the guidelines of how to use ACPI > > > on ARM64. > > > > As the most vocal participant against ACPI being adopted, I would have > > appreciated a cc on this patch set -- it's not like you were going for > > a minimal set of cc recipients already. It makes it seem like you're > > trying to sneak it past me for comments. Not cool. I know that's > > probably not your intent, but still. > > > > Some comments below. Overall the doc looks pretty good, but the > > details about _DSD and clocks are somewhat worrisome. > > > > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > --- > > > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 240 insertions(+) > > > create mode 100644 Documentation/arm64/arm-acpi.txt > > > > > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > > > new file mode 100644 > > > index 0000000..12cd550 > > > --- /dev/null > > > +++ b/Documentation/arm64/arm-acpi.txt > > > @@ -0,0 +1,240 @@ > > > +ACPI on ARMv8 Servers > > > +--------------------- > > > + > > > +ACPI will be used for ARMv8 general purpose servers designed to follow > > > > "ACPI might be used" or "can be used" > > > > > +the SBSA specification (currently available to people with an ARM login at > > > +http://silver.arm.com) > > > + > > > +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, > > > +which is available at <http://www.uefi.org/acpi/specs>. > > > > The implemented version where? The kernel implements that version? > > Worth clarifying. > > > > > +If the machine does not meet these requirements then it is likely that Device > > > +Tree (DT) is more suitable for the hardware. > > > > This is should be a very clear statement that is currently vague > > w.r.t. which requirements are met or not, especially based on the > > sentence above. > > > > > +Relationship with Device Tree > > > +----------------------------- > > > + > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > +exclusive with DT support at compile time. > > > + > > > +At boot time the kernel will only use one description method depending on > > > +parameters passed from the bootloader. > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > while earlier this year, acpi should still default to off -- if a DT > > and ACPI are both passed in, DT should at this time be given priority. > > Why? I really don't see the logic in doing that. Seems like I am replying to your emails in reverse order. > Currently the kernel can only boot using DT, so DT will be used even if > ACPI is present. In the presence of ACPI support in the kernel, ACPI > would be used on said systems. For all the reasons we hashed out earlier this year: We can't trust that vendors will know how to do ACPI from day one, so instead of loading up the kernel with lots of quirks and workarounds, we'll default to not using it until we're at a point where things look mature enough. The alternative is to keep this patch set out of the kernel. We can do that too, but I don't think that really helps anyone. > From the discussions at the last Linaro Connect, this was seen as > important for virtual machines which want to provide ACPI services to > guests while still being able to boot DT-only kernels. I'll leave it to > Grant, Rob, and Christoffer to cover that. Ok, waiting to see what they have to say then. > > (Where can I learn more about how the boot loaders currently handle > > this? Do some of them pass in both DT and ACPI on some platforms, for > > example?) > > All will pass in some form of DT. If booted through UEFI, the DT will be > augmented with data about the UEFI memory map (and if no DT is provided, > a /chosen only DT will eb created by the EFI stub). The in-kernel EFI stub? > A kernel with ACPI support will query EFI for ACPI tables, and if found > use them. > > > > +Regardless of whether DT or ACPI is used, the kernel must always be capable > > > +of booting with either scheme. > > > > It should always be possible to compile out ACPI. There will be plenty > > of platforms that will not implement it, so disabling CONFIG_ACPI > > needs to be possible. > > A better description would be that the two must never be mutually > exclusive. It must always be possible to have a kernel which supports > both, and code must never assume the absence of the other. No. "Not mutually exclusive" and "possible to turn off one of them" is not the same thing. > At run time, the kernel will decide to use one for system description > and use that. > > [...] > > > > +Device Enumeration > > > +------------------ > > > + > > > +Device descriptions in ACPI should use standard recognised ACPI interfaces. > > > +These are far simpler than the information provided via Device Tree. Drivers > > > +should take into account this simplicity and work with sensible defaults. > > > + > > > +On no account should a Device Tree attempt to be replicated in ASL using such > > > +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific > > > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > > > +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. > > > > I see these two sentences as contradictory, given that the _DSD doc > > linked below contains examples that mirror over several properties, > > such as "linux,default-trigger" and other LED-specific properties. > > (section 2.4.2 in the below doc). "default-state" seems to come from > > DT too? > > Urrgh. Those should never been placed in the ACPI spec. It's bad enough > in DT. +1 > > Care to elaborate and explain what the intention here is? This could > > worst case turn into quite a mess. > > > > Given that ACPI can present completely different data based on what OS > > is running, it's quite common to indeed have OS specific data in > > there. How does that relate to this document and these practices? > > > > > +Common _DSD bindings should be submitted to ASWG to be included in the > > > +document :- > > > + > > > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm > > > > So, for these that are a mirror of the device tree bindings, there > > needs to be a wording here around reviewing the DT binding first so we > > don't get diverging bindings. > > We shouldn't work on the assumption that the two should be identical. > ACPI already has standard mechanisms for describing certain linkages > that are divergent from DT. I can't tell what is worse, identical mirroring over into DT of FDT (and the lack of being able to fix it up in case of description bugs) or _DSD doing things subtly different and now the kernel needs to handle both variants. > This really needs higher-level thought, and I'd hoped that things > wouldn't blindly be copied one way or the other. Agreed. Besides, the whole idea of ACPI is to not have to model clocks. Sigh. > [...] > > > > +values before the kernel is executed. If a driver requires to know rates of > > > +clocks set by firmware then they can be passed to kernel using _DSD. > > > + > > > +example :- > > > + > > > +Device (CLK0) { > > > + ... > > > + > > > + Name (_DSD, Package() { > > > + ToUUID("XXXXX"), > > > + Package() { > > > + Package(2) {"#clock-cells", 0}, > > > > Clock-cells? What do they mean here? Is this specified in the ACPI > > standards? I had to register to get access to it, and didn't feel like > > doing that right now. I guess it's not _all_ that open a spec. :( > > Yeah, this is complete nonsense. > > If people are going to blindly copy elements from DT into ACPI without > actually understanding them, then ACPI is clearly no better than DT. > > [...] > > > > +static int device_probe(stuct platform_device *pdev) > > > +{ > > > + ... > > > + acpi_handle handle = ACPI_HANDLE(&pdev->dev); > > > + struct device_node node = pdev->dev.of_node; > > > + ... > > > + > > > + if (node) > > > + ret = device_probe_dt(pdev); > > > + else if (handle) > > > + ret = device_probe_acpi(pdev); > > > + else > > > + /* other initialisation */ > > > + ... > > > + /* Continue with any generic probe operations */ > > > + ... > > > +} > > > > This looks good to me, and it's also my preferred way of ACPI-enabling > > drivers. I guess we might discuss this at KS since it was a proposed > > topic there, and others will object. :) > > This is almost precisely the form of probe I want to see (it would be > nicer IMO to have separate entry points for ACPI / DT / platform data > that can all call a common probe core, but this isn't too far off). I disagree, but it's also not something we're looking to change at this time. We'll take that debate when you post the patches changing how device probing works. :-) > > > + > > > +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear > > > +the different names the driver is probed for, both from DT and from ACPI. > > > + > > > +module device tables :- > > > + > > > +static struct of_device_id virtio_mmio_match[] = { > > > + { .compatible = "virtio,mmio", }, > > > + {}, > > > +}; > > > +MODULE_DEVICE_TABLE(of, virtio_mmio_match); > > > + > > > +static const struct acpi_device_id virtio_mmio_acpi_match[] = { > > > + { "LNRO0005", }, > > > + { } > > > > No comma here, but a comma on DT. Probably make them equivalent for > > consistency (including space between the braces). > > The comma on the DT form should probably be dropped. The NULL sentinel's > only job is to delimit the list, so it never makes sense to place > something after it. Yep. -Olof
On Mon, Jul 28, 2014 at 05:27:50PM +0100, Olof Johansson wrote: > On Mon, Jul 28, 2014 at 11:07:50AM +0200, Arnd Bergmann wrote: > > On Saturday 26 July 2014 19:34:48 Olof Johansson wrote: > > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > > > +Relationship with Device Tree > > > > +----------------------------- > > > > + > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > > +exclusive with DT support at compile time. > > > > + > > > > +At boot time the kernel will only use one description method depending on > > > > +parameters passed from the bootloader. > > > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > > while earlier this year, acpi should still default to off -- if a DT > > > and ACPI are both passed in, DT should at this time be given priority. > > > > I think this would be harder to do with the way that ACPI is passed in > > to the kernel. IIRC, you always have a minimal DT information based on > > the ARM64 boot protocol, but in the case of ACPI, this contains pointers > > to the ACPI tables, which are then used for populating the Linux platform > > devices (unless acpi=disabled is set), while the other contents of the > > DTB may be present but we skip the of_platform_populate state. > > How can it be harder to do? If you support acpi=off, then you should support > acpi=on. > > Another alternative would be to have an early fixup that stubs out > the acpi properties from the DTB unless there's an 'acpi' or 'acpi=on' > argument on the cmdline. Not quite as tidy a solution, though. I don't follow: If you want to disable ACPI, you can pass acpi=off. This is your workaround for broken ACPI (and only if you happen to have wrirten your own DTB, because many/most ACPI systems WILL NOT have a DTB to begin with). If you have ACPI, for what technical reason would you not attempt to use it by default? There will be systems which _DO NOT_ have any sort of DTB to begin with. For VMs, both may be provided. By the necessity of a DTB being present for bootargs, ACPI _MUST_ take precedence. If you don't want it, you pass acpi=off, or configure your VM software to not pass an ACPI configuration table. Why avoid ACPI by default? So that we can later enable it and discover it's completely broken? Then we need short-sighted hacks to work around short-sighted decisions. The best thing to do is to try and use things, be as strict as possible, stress the implementation, and blow up early and loudly so that said systems must be fixed. People are using Linux for bringup; it is the closest thing to a validation suite. Being lazy and hacking around things for now will only make things worse in the long run. We _CANNOT_ place our fingers in our ears and blind ourselves with the DT banner. ACPI is ugly, sure, but so is DT. Neither is fundamentally better than the other. The best thing we can do is make it as easy as possible to highlight bugs in ACPI implementations and barf such that people are forced to fix their ACPI implementations. No other OS supports ACPI on 64-bit arm systems. Being strict should force implementors to try harder. > > If this is correct, then replacing the firmware-generated dtb with a > > user-provided on would implicitly remove the ACPI tables from visibility, > > which is exactly what we want. > > I was of the impression that firmware patches in the ACPI entries into either > device-tree before launching the kernel. Is that not the case? 1) The ACPI tables will be registered as UEFI configuration tables, within platform-specific UEFI code. Nothing outside of UEFI is involved. 1a) A loader (e.g. GRUB, the EFI stub) COULD override the loaded tables. This is a workaround, and should never be the standard way of doing things. It defeats the point, much like appended DTB. 2) The EFI stub will patch UEFI memory map properties into the DTB. The firmware is not involved. 3) The kernel will detect whether EFI is present by the presence of the memory map, and try to use it if so. The memory map comes from UEFI, and memory nodes (and memreserves) are ignored. Only select properties under /chosen (e.g. bootargs) will be used. 4) If booted via EFI, the kernel will look for known UEFI configuration tables by (G|U)UID, and set up some pointers. 5) The ACPI code will go and look to see if the ACPI table pointers have been initialised. If so, the kernel will attempt to use the ACPI tables unlesss instructed otherwise. If using ACPI, the DTB will be ignored outside of /chosen. The ACPI tables or pointers to them are not directly patched into the DTB at any stage. The choice of using ACPI is left with the kernel. > And what if some bootloader chooses to do it that way in the future? > It's better to not assume that they get it right. For the firmware/loader to do so it would have to work around the kernel version parameter the stub places in the DTB and the kernel verifies. If it does so, I would argue said firmware is actively malicious. > > It's possible that I'm misremembering it though, and it should be > > documented better. > > Yes, definitely needs to be documented to not leave room for random > interpretation later on. We should definitely make the documentation as strict as possible on what's allowed, and have the kernel barf early on if requirements are not met. Thanks, Mark.
On Mon, Jul 28, 2014 at 05:44:59PM +0100, Olof Johansson wrote: > On Mon, Jul 28, 2014 at 11:06:54AM +0100, Mark Rutland wrote: > > Hi Olof, > > > > On Sun, Jul 27, 2014 at 03:34:48AM +0100, Olof Johansson wrote: > > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > > > From: Graeme Gregory <graeme.gregory@linaro.org> > > > > > > > > Add documentation for the guidelines of how to use ACPI > > > > on ARM64. > > > > > > As the most vocal participant against ACPI being adopted, I would have > > > appreciated a cc on this patch set -- it's not like you were going for > > > a minimal set of cc recipients already. It makes it seem like you're > > > trying to sneak it past me for comments. Not cool. I know that's > > > probably not your intent, but still. > > > > > > Some comments below. Overall the doc looks pretty good, but the > > > details about _DSD and clocks are somewhat worrisome. > > > > > > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > > --- > > > > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 240 insertions(+) > > > > create mode 100644 Documentation/arm64/arm-acpi.txt > > > > > > > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > > > > new file mode 100644 > > > > index 0000000..12cd550 > > > > --- /dev/null > > > > +++ b/Documentation/arm64/arm-acpi.txt > > > > @@ -0,0 +1,240 @@ > > > > +ACPI on ARMv8 Servers > > > > +--------------------- > > > > + > > > > +ACPI will be used for ARMv8 general purpose servers designed to follow > > > > > > "ACPI might be used" or "can be used" > > > > > > > +the SBSA specification (currently available to people with an ARM login at > > > > +http://silver.arm.com) > > > > + > > > > +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, > > > > +which is available at <http://www.uefi.org/acpi/specs>. > > > > > > The implemented version where? The kernel implements that version? > > > Worth clarifying. > > > > > > > +If the machine does not meet these requirements then it is likely that Device > > > > +Tree (DT) is more suitable for the hardware. > > > > > > This is should be a very clear statement that is currently vague > > > w.r.t. which requirements are met or not, especially based on the > > > sentence above. > > > > > > > +Relationship with Device Tree > > > > +----------------------------- > > > > + > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > > +exclusive with DT support at compile time. > > > > + > > > > +At boot time the kernel will only use one description method depending on > > > > +parameters passed from the bootloader. > > > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > > while earlier this year, acpi should still default to off -- if a DT > > > and ACPI are both passed in, DT should at this time be given priority. > > > > Why? I really don't see the logic in doing that. > > Seems like I am replying to your emails in reverse order. Oh well; it makes the discussion more fun to read :) > > Currently the kernel can only boot using DT, so DT will be used even if > > ACPI is present. In the presence of ACPI support in the kernel, ACPI > > would be used on said systems. > > For all the reasons we hashed out earlier this year: We can't trust that > vendors will know how to do ACPI from day one, so instead of loading up the > kernel with lots of quirks and workarounds, we'll default to not using it until > we're at a point where things look mature enough. I am not arguing for quirks and workarounds. What I would like to see is for the kernel barf horrifically right now on even slightly dodgy ACPI implementations before they are out in the wild. Otherwise the later switching on of ACPI support is what will necessitate workarounds for said systems. > The alternative is to keep this patch set out of the kernel. We can do that > too, but I don't think that really helps anyone. That's just another way of disabling it by default. IMO that's worse because only a subset of people will discover how broken their ACPI implementation is. > > From the discussions at the last Linaro Connect, this was seen as > > important for virtual machines which want to provide ACPI services to > > guests while still being able to boot DT-only kernels. I'll leave it to > > Grant, Rob, and Christoffer to cover that. > > Ok, waiting to see what they have to say then. > > > > (Where can I learn more about how the boot loaders currently handle > > > this? Do some of them pass in both DT and ACPI on some platforms, for > > > example?) > > > > All will pass in some form of DT. If booted through UEFI, the DT will be > > augmented with data about the UEFI memory map (and if no DT is provided, > > a /chosen only DT will eb created by the EFI stub). > > The in-kernel EFI stub? Yes. If a kernel with the stub is booted as an EFI application, the stub code will be executed and augment / create a DTB with handles for the UEFI memory map. There is a tight coupling between the stub and the kernel. Only the stub is able to do this currently. > > A kernel with ACPI support will query EFI for ACPI tables, and if found > > use them. > > > > > > +Regardless of whether DT or ACPI is used, the kernel must always be capable > > > > +of booting with either scheme. > > > > > > It should always be possible to compile out ACPI. There will be plenty > > > of platforms that will not implement it, so disabling CONFIG_ACPI > > > needs to be possible. > > > > A better description would be that the two must never be mutually > > exclusive. It must always be possible to have a kernel which supports > > both, and code must never assume the absence of the other. > > No. "Not mutually exclusive" and "possible to turn off one of them" is not the > same thing. I think we're talking cross purposes. Those sentences are indeed not equivalent. We want two things: 1) A single kernel image must be able to support both ACPI and DT. 2) It must be possible to build a kernel supporting only DT. Of the two (1) is the most important, or single image is dead. That does not mean that (2) is not important, but it is an optimisation to remove unused code and save on size. Any wording we have here must express both these points. > > At run time, the kernel will decide to use one for system description > > and use that. > > > > [...] > > > > > > +Device Enumeration > > > > +------------------ > > > > + > > > > +Device descriptions in ACPI should use standard recognised ACPI interfaces. > > > > +These are far simpler than the information provided via Device Tree. Drivers > > > > +should take into account this simplicity and work with sensible defaults. > > > > + > > > > +On no account should a Device Tree attempt to be replicated in ASL using such > > > > +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific > > > > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > > > > +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. > > > > > > I see these two sentences as contradictory, given that the _DSD doc > > > linked below contains examples that mirror over several properties, > > > such as "linux,default-trigger" and other LED-specific properties. > > > (section 2.4.2 in the below doc). "default-state" seems to come from > > > DT too? > > > > Urrgh. Those should never been placed in the ACPI spec. It's bad enough > > in DT. > > +1 > > > > Care to elaborate and explain what the intention here is? This could > > > worst case turn into quite a mess. > > > > > > Given that ACPI can present completely different data based on what OS > > > is running, it's quite common to indeed have OS specific data in > > > there. How does that relate to this document and these practices? > > > > > > > +Common _DSD bindings should be submitted to ASWG to be included in the > > > > +document :- > > > > + > > > > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm > > > > > > So, for these that are a mirror of the device tree bindings, there > > > needs to be a wording here around reviewing the DT binding first so we > > > don't get diverging bindings. > > > > We shouldn't work on the assumption that the two should be identical. > > ACPI already has standard mechanisms for describing certain linkages > > that are divergent from DT. > > I can't tell what is worse, identical mirroring over into DT of FDT (and the > lack of being able to fix it up in case of description bugs) or _DSD doing > things subtly different and now the kernel needs to handle both variants. ACPI and DT are already doing different things. It's fine to do the same thing where it makes sense to, but we shouldn't go for a false equivalence here. > > This really needs higher-level thought, and I'd hoped that things > > wouldn't blindly be copied one way or the other. > > Agreed. Besides, the whole idea of ACPI is to not have to model clocks. Sigh. Well, maybe not the _whole_ idea. I hear ACPI actually tries to model other devices... > > [...] > > > > > > +values before the kernel is executed. If a driver requires to know rates of > > > > +clocks set by firmware then they can be passed to kernel using _DSD. > > > > + > > > > +example :- > > > > + > > > > +Device (CLK0) { > > > > + ... > > > > + > > > > + Name (_DSD, Package() { > > > > + ToUUID("XXXXX"), > > > > + Package() { > > > > + Package(2) {"#clock-cells", 0}, > > > > > > Clock-cells? What do they mean here? Is this specified in the ACPI > > > standards? I had to register to get access to it, and didn't feel like > > > doing that right now. I guess it's not _all_ that open a spec. :( > > > > Yeah, this is complete nonsense. > > > > If people are going to blindly copy elements from DT into ACPI without > > actually understanding them, then ACPI is clearly no better than DT. > > > > [...] > > > > > > +static int device_probe(stuct platform_device *pdev) > > > > +{ > > > > + ... > > > > + acpi_handle handle = ACPI_HANDLE(&pdev->dev); > > > > + struct device_node node = pdev->dev.of_node; > > > > + ... > > > > + > > > > + if (node) > > > > + ret = device_probe_dt(pdev); > > > > + else if (handle) > > > > + ret = device_probe_acpi(pdev); > > > > + else > > > > + /* other initialisation */ > > > > + ... > > > > + /* Continue with any generic probe operations */ > > > > + ... > > > > +} > > > > > > This looks good to me, and it's also my preferred way of ACPI-enabling > > > drivers. I guess we might discuss this at KS since it was a proposed > > > topic there, and others will object. :) > > > > This is almost precisely the form of probe I want to see (it would be > > nicer IMO to have separate entry points for ACPI / DT / platform data > > that can all call a common probe core, but this isn't too far off). > > I disagree, but it's also not something we're looking to change at this time. > We'll take that debate when you post the patches changing how device probing > works. :-) Sure :) Cheers, Mark. > > > > + > > > > +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear > > > > +the different names the driver is probed for, both from DT and from ACPI. > > > > + > > > > +module device tables :- > > > > + > > > > +static struct of_device_id virtio_mmio_match[] = { > > > > + { .compatible = "virtio,mmio", }, > > > > + {}, > > > > +}; > > > > +MODULE_DEVICE_TABLE(of, virtio_mmio_match); > > > > + > > > > +static const struct acpi_device_id virtio_mmio_acpi_match[] = { > > > > + { "LNRO0005", }, > > > > + { } > > > > > > No comma here, but a comma on DT. Probably make them equivalent for > > > consistency (including space between the braces). > > > > The comma on the DT form should probably be dropped. The NULL sentinel's > > only job is to delimit the list, so it never makes sense to place > > something after it. > > Yep. > > > -Olof >
On Mon, Jul 28, 2014 at 09:23:40AM -0700, Olof Johansson wrote: > On Mon, Jul 28, 2014 at 09:42:57AM +0100, Graeme Gregory wrote: > > >>+On no account should a Device Tree attempt to be replicated in ASL using such > > >>+constructs as Name(KEY0, "Value1") type constructs. Additional driver specific > > >>+data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > > >>+_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. ... > > >I see these two sentences as contradictory, given that the _DSD doc > > >worst case turn into quite a mess. > > >Given that ACPI can present completely different data based on what OS > > >is running, it's quite common to indeed have OS specific data in > > >there. How does that relate to this document and these practices? > > OS specific data has traditionally not worked out well for ACPI, I > > would like to "persuade" people not to use it on ARM. > It hasn't? I think Microsoft disagrees. It's also how vendors have been able to > present an older machine description to keep their newer hardware compatible > with older software, isn't it? How do you expect to handle that if you can > only present one table? It's the same challenge that DT has. It seems sensible to recommend against using OS specifics if possible if only from the point of view of improving the robustness of the system - the less paths there are to test in the BIOS the more likely it is that the active path is one that's been well tested. It's legal in the spec and you can do it but encouraging people not to do it will hopefully make life easier down the line. Similarly encouraging people to put as little as possible in there should reduce the opportunities they have to get things wrong. The best use case for OS testing is to enable a non-default workaround for older versions of the OS but in the case of Linux that's a bit tricky since we don't have clear versions to test against - even with the kernel version number it's never clear if it's been patched by a distro or something. Windows is a much more fixed target here.
On Mon, Jul 28, 2014 at 10:00 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Jul 28, 2014 at 05:27:50PM +0100, Olof Johansson wrote: >> On Mon, Jul 28, 2014 at 11:07:50AM +0200, Arnd Bergmann wrote: >> > On Saturday 26 July 2014 19:34:48 Olof Johansson wrote: >> > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> > > > +Relationship with Device Tree >> > > > +----------------------------- >> > > > + >> > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually >> > > > +exclusive with DT support at compile time. >> > > > + >> > > > +At boot time the kernel will only use one description method depending on >> > > > +parameters passed from the bootloader. >> > > >> > > Possibly overriden by kernel bootargs. And as debated for quite a >> > > while earlier this year, acpi should still default to off -- if a DT >> > > and ACPI are both passed in, DT should at this time be given priority. >> > >> > I think this would be harder to do with the way that ACPI is passed in >> > to the kernel. IIRC, you always have a minimal DT information based on >> > the ARM64 boot protocol, but in the case of ACPI, this contains pointers >> > to the ACPI tables, which are then used for populating the Linux platform >> > devices (unless acpi=disabled is set), while the other contents of the >> > DTB may be present but we skip the of_platform_populate state. >> >> How can it be harder to do? If you support acpi=off, then you should support >> acpi=on. >> >> Another alternative would be to have an early fixup that stubs out >> the acpi properties from the DTB unless there's an 'acpi' or 'acpi=on' >> argument on the cmdline. Not quite as tidy a solution, though. > > I don't follow: > > If you want to disable ACPI, you can pass acpi=off. This is your > workaround for broken ACPI (and only if you happen to have wrirten your > own DTB, because many/most ACPI systems WILL NOT have a DTB to begin > with). All ACPI should be assumed broken at this time, so acpi=off _must_ be the default. > If you have ACPI, for what technical reason would you not attempt to use > it by default? Because it will be broken, or the company you bought the machine from implemented it wrong because they're still learning how to do this too. The original doc even mentioned that there are parts of the binding that aren't even sorted out. The _DSD stuff, too. Overall, it's not yet ready to be the default boot method. > There will be systems which _DO NOT_ have any sort of DTB to begin with. Listen, we've been really clear about this: DT is the default that everybody has to use for mainline kernel for the near term. If you have an ACPI-only system, then you either have to write a DT for it, or you won't be booting mainline on it. If people have not been listening to the advice we've been giving, and that sucks for them. Even worse, I suspect there are players out there who have taken bad advice from certain players. At the end of the day, it is not our problem. We were quite clear, and Grant even wrote a long and good blog post about this. If they really want to, they can boot with acpi=on instead. It's not like it's hard to add. > For VMs, both may be provided. By the necessity of a DTB being present > for bootargs, ACPI _MUST_ take precedence. If you don't want it, you > pass acpi=off, or configure your VM software to not pass an ACPI > configuration table. > > Why avoid ACPI by default? So that we can later enable it and discover > it's completely broken? Then we need short-sighted hacks to work around > short-sighted decisions. We have answered this multiple times in the past already. Please go read the archives. It is highly unlikely that we will retroactively enable it in the future for the first-generation devices either. Chances are there'll be a rev or two more of ACPI before then, so we can do something such as "only automatically accept ACPI 5.4 or higher" or whatever. That can all be sorted when the time comes. > The best thing to do is to try and use things, be as strict as possible, > stress the implementation, and blow up early and loudly so that said > systems must be fixed. "Try", sure. Make it the default and blow up and make a system unbootable when it's wrong? No, absolutely not. Not while bindings are still being hashed out and vendors are still figuring things out. > People are using Linux for bringup; it is the closest thing to a > validation suite. Being lazy and hacking around things for now will only > make things worse in the long run. Who's hacking around what? And holy shit, there is no validation suite? Then ACPI is doomed. Literally. Linux can't be a validation suite for it. It means that potentially every single code change to anything related to ACPI (drivers or core) on Linux means that it's now using completely untested pieces of firmware. Fodder for nightmares. > We _CANNOT_ place our fingers in our ears and blind ourselves with the > DT banner. ACPI is ugly, sure, but so is DT. Neither is fundamentally > better than the other. The best thing we can do is make it as easy as > possible to highlight bugs in ACPI implementations and barf such that > people are forced to fix their ACPI implementations. It's not about what's better than the other. They both suck. But one is already here and we've learned by now most of the ways in which it sucks and we have a good idea of how to avoid the worst of it. On the other, we have all of that in front of us still. Guess which one it makes sense to stick to? > No other OS supports ACPI on 64-bit arm systems. Being strict should > force implementors to try harder. Thanks for _finally_ stating what we've all known for a very long time but everybody's been pretending isn't the case. >> > If this is correct, then replacing the firmware-generated dtb with a >> > user-provided on would implicitly remove the ACPI tables from visibility, >> > which is exactly what we want. >> >> I was of the impression that firmware patches in the ACPI entries into either >> device-tree before launching the kernel. Is that not the case? > > 1) The ACPI tables will be registered as UEFI configuration tables, > within platform-specific UEFI code. Nothing outside of UEFI is > involved. > > 1a) A loader (e.g. GRUB, the EFI stub) COULD override the loaded tables. > This is a workaround, and should never be the standard way of doing > things. It defeats the point, much like appended DTB. > > 2) The EFI stub will patch UEFI memory map properties into the DTB. The > firmware is not involved. > > 3) The kernel will detect whether EFI is present by the presence of the > memory map, and try to use it if so. The memory map comes from UEFI, > and memory nodes (and memreserves) are ignored. Only select > properties under /chosen (e.g. bootargs) will be used. > > 4) If booted via EFI, the kernel will look for known UEFI configuration > tables by (G|U)UID, and set up some pointers. > > 5) The ACPI code will go and look to see if the ACPI table pointers have > been initialised. If so, the kernel will attempt to use the ACPI > tables unlesss instructed otherwise. If using ACPI, the DTB will be > ignored outside of /chosen. > > The ACPI tables or pointers to them are not directly patched into the > DTB at any stage. The choice of using ACPI is left with the kernel. Thanks, excellent summary. >> And what if some bootloader chooses to do it that way in the future? >> It's better to not assume that they get it right. > > For the firmware/loader to do so it would have to work around the kernel > version parameter the stub places in the DTB and the kernel verifies. If > it does so, I would argue said firmware is actively malicious. Ok. >> > It's possible that I'm misremembering it though, and it should be >> > documented better. >> >> Yes, definitely needs to be documented to not leave room for random >> interpretation later on. > > We should definitely make the documentation as strict as possible on > what's allowed, and have the kernel barf early on if requirements are > not met. It should definitely complain when it finds bad entries, but making the kernel unusable for end-users because someone is still under development is the wrong answer. While the ACPI bindings are sorted out, we'll ship platforms using DT support in the kernel. This must not change just because some piece of the ACPI work is starting to land in-tree while other pieces are still being worked out. And again: The kernel is not a compliance suite for ACPI, and if the vendors doing bringup is treating it that way then I'm quite scared of the whole endeavor. -Olof
On Mon, Jul 28, 2014 at 10:36 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Jul 28, 2014 at 05:44:59PM +0100, Olof Johansson wrote: >> On Mon, Jul 28, 2014 at 11:06:54AM +0100, Mark Rutland wrote: >> > Hi Olof, >> > >> > On Sun, Jul 27, 2014 at 03:34:48AM +0100, Olof Johansson wrote: >> > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> > > > From: Graeme Gregory <graeme.gregory@linaro.org> >> > > > >> > > > Add documentation for the guidelines of how to use ACPI >> > > > on ARM64. >> > > >> > > As the most vocal participant against ACPI being adopted, I would have >> > > appreciated a cc on this patch set -- it's not like you were going for >> > > a minimal set of cc recipients already. It makes it seem like you're >> > > trying to sneak it past me for comments. Not cool. I know that's >> > > probably not your intent, but still. >> > > >> > > Some comments below. Overall the doc looks pretty good, but the >> > > details about _DSD and clocks are somewhat worrisome. >> > > >> > > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> >> > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> > > > --- >> > > > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ >> > > > 1 file changed, 240 insertions(+) >> > > > create mode 100644 Documentation/arm64/arm-acpi.txt >> > > > >> > > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt >> > > > new file mode 100644 >> > > > index 0000000..12cd550 >> > > > --- /dev/null >> > > > +++ b/Documentation/arm64/arm-acpi.txt >> > > > @@ -0,0 +1,240 @@ >> > > > +ACPI on ARMv8 Servers >> > > > +--------------------- >> > > > + >> > > > +ACPI will be used for ARMv8 general purpose servers designed to follow >> > > >> > > "ACPI might be used" or "can be used" >> > > >> > > > +the SBSA specification (currently available to people with an ARM login at >> > > > +http://silver.arm.com) >> > > > + >> > > > +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, >> > > > +which is available at <http://www.uefi.org/acpi/specs>. >> > > >> > > The implemented version where? The kernel implements that version? >> > > Worth clarifying. >> > > >> > > > +If the machine does not meet these requirements then it is likely that Device >> > > > +Tree (DT) is more suitable for the hardware. >> > > >> > > This is should be a very clear statement that is currently vague >> > > w.r.t. which requirements are met or not, especially based on the >> > > sentence above. >> > > >> > > > +Relationship with Device Tree >> > > > +----------------------------- >> > > > + >> > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually >> > > > +exclusive with DT support at compile time. >> > > > + >> > > > +At boot time the kernel will only use one description method depending on >> > > > +parameters passed from the bootloader. >> > > >> > > Possibly overriden by kernel bootargs. And as debated for quite a >> > > while earlier this year, acpi should still default to off -- if a DT >> > > and ACPI are both passed in, DT should at this time be given priority. >> > >> > Why? I really don't see the logic in doing that. >> >> Seems like I am replying to your emails in reverse order. > > Oh well; it makes the discussion more fun to read :) > >> > Currently the kernel can only boot using DT, so DT will be used even if >> > ACPI is present. In the presence of ACPI support in the kernel, ACPI >> > would be used on said systems. >> >> For all the reasons we hashed out earlier this year: We can't trust that >> vendors will know how to do ACPI from day one, so instead of loading up the >> kernel with lots of quirks and workarounds, we'll default to not using it until >> we're at a point where things look mature enough. > > I am not arguing for quirks and workarounds. > > What I would like to see is for the kernel barf horrifically right now > on even slightly dodgy ACPI implementations before they are out in the > wild. Otherwise the later switching on of ACPI support is what will > necessitate workarounds for said systems. That's the job of a compliance test suite, not of the Linux kernel. We're here to make systems usable and ship a computing environment that people can use to get work done. We're not a bringup tool and a test suite. Turning it on by option means that those who want to test it can do so. I'd be happy to do both acpi and non-acpi boots of my boot farm, for example, if anyone ever sends me 64-bit hardware for it. >> The alternative is to keep this patch set out of the kernel. We can do that >> too, but I don't think that really helps anyone. > > That's just another way of disabling it by default. IMO that's worse > because only a subset of people will discover how broken their ACPI > implementation is. Yes, I think that's worse too. I don't want to do that, but if people aren't listening to reason on why it can be merged as long as it's disabled by default, then not merging it is the answer. >> > From the discussions at the last Linaro Connect, this was seen as >> > important for virtual machines which want to provide ACPI services to >> > guests while still being able to boot DT-only kernels. I'll leave it to >> > Grant, Rob, and Christoffer to cover that. >> >> Ok, waiting to see what they have to say then. >> >> > > (Where can I learn more about how the boot loaders currently handle >> > > this? Do some of them pass in both DT and ACPI on some platforms, for >> > > example?) >> > >> > All will pass in some form of DT. If booted through UEFI, the DT will be >> > augmented with data about the UEFI memory map (and if no DT is provided, >> > a /chosen only DT will eb created by the EFI stub). >> >> The in-kernel EFI stub? > > Yes. If a kernel with the stub is booted as an EFI application, the stub > code will be executed and augment / create a DTB with handles for the > UEFI memory map. > > There is a tight coupling between the stub and the kernel. Only the stub > is able to do this currently. Ok, just making clear in case the common bootloaders had their own functionality for the same. >> > A kernel with ACPI support will query EFI for ACPI tables, and if found >> > use them. >> > >> > > > +Regardless of whether DT or ACPI is used, the kernel must always be capable >> > > > +of booting with either scheme. >> > > >> > > It should always be possible to compile out ACPI. There will be plenty >> > > of platforms that will not implement it, so disabling CONFIG_ACPI >> > > needs to be possible. >> > >> > A better description would be that the two must never be mutually >> > exclusive. It must always be possible to have a kernel which supports >> > both, and code must never assume the absence of the other. >> >> No. "Not mutually exclusive" and "possible to turn off one of them" is not the >> same thing. > > I think we're talking cross purposes. Those sentences are indeed not > equivalent. We want two things: > > 1) A single kernel image must be able to support both ACPI and DT. > > 2) It must be possible to build a kernel supporting only DT. > > Of the two (1) is the most important, or single image is dead. That does > not mean that (2) is not important, but it is an optimisation to remove > unused code and save on size. > > Any wording we have here must express both these points. Yes, agreed (1) is clearly stated elsewhere, it's the (2) portion that needs to be reflected in the docs, which was what my initial comment was as well. Then you took it for a drive around the block with your "not mutually exclusive" comment. Now we're back where we started. >> > At run time, the kernel will decide to use one for system description >> > and use that. >> > >> > [...] >> > >> > > > +Device Enumeration >> > > > +------------------ >> > > > + >> > > > +Device descriptions in ACPI should use standard recognised ACPI interfaces. >> > > > +These are far simpler than the information provided via Device Tree. Drivers >> > > > +should take into account this simplicity and work with sensible defaults. >> > > > + >> > > > +On no account should a Device Tree attempt to be replicated in ASL using such >> > > > +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific >> > > > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or >> > > > +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. >> > > >> > > I see these two sentences as contradictory, given that the _DSD doc >> > > linked below contains examples that mirror over several properties, >> > > such as "linux,default-trigger" and other LED-specific properties. >> > > (section 2.4.2 in the below doc). "default-state" seems to come from >> > > DT too? >> > >> > Urrgh. Those should never been placed in the ACPI spec. It's bad enough >> > in DT. >> >> +1 >> >> > > Care to elaborate and explain what the intention here is? This could >> > > worst case turn into quite a mess. >> > > >> > > Given that ACPI can present completely different data based on what OS >> > > is running, it's quite common to indeed have OS specific data in >> > > there. How does that relate to this document and these practices? >> > > >> > > > +Common _DSD bindings should be submitted to ASWG to be included in the >> > > > +document :- >> > > > + >> > > > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm >> > > >> > > So, for these that are a mirror of the device tree bindings, there >> > > needs to be a wording here around reviewing the DT binding first so we >> > > don't get diverging bindings. >> > >> > We shouldn't work on the assumption that the two should be identical. >> > ACPI already has standard mechanisms for describing certain linkages >> > that are divergent from DT. >> >> I can't tell what is worse, identical mirroring over into DT of FDT (and the >> lack of being able to fix it up in case of description bugs) or _DSD doing >> things subtly different and now the kernel needs to handle both variants. > > ACPI and DT are already doing different things. It's fine to do the same > thing where it makes sense to, but we shouldn't go for a false > equivalence here. > >> > This really needs higher-level thought, and I'd hoped that things >> > wouldn't blindly be copied one way or the other. >> >> Agreed. Besides, the whole idea of ACPI is to not have to model clocks. Sigh. > > Well, maybe not the _whole_ idea. I hear ACPI actually tries to model > other devices... One of the most significant reasons for ACPI is to abstract away the hardware such that the kernel doesn't have to bother about knowing intimate details about it, such as clock trees. We can discuss this some more if you want, but I've got other work to do. >> > [...] >> > >> > > > +values before the kernel is executed. If a driver requires to know rates of >> > > > +clocks set by firmware then they can be passed to kernel using _DSD. >> > > > + >> > > > +example :- >> > > > + >> > > > +Device (CLK0) { >> > > > + ... >> > > > + >> > > > + Name (_DSD, Package() { >> > > > + ToUUID("XXXXX"), >> > > > + Package() { >> > > > + Package(2) {"#clock-cells", 0}, >> > > >> > > Clock-cells? What do they mean here? Is this specified in the ACPI >> > > standards? I had to register to get access to it, and didn't feel like >> > > doing that right now. I guess it's not _all_ that open a spec. :( >> > >> > Yeah, this is complete nonsense. >> > >> > If people are going to blindly copy elements from DT into ACPI without >> > actually understanding them, then ACPI is clearly no better than DT. >> > >> > [...] >> > >> > > > +static int device_probe(stuct platform_device *pdev) >> > > > +{ >> > > > + ... >> > > > + acpi_handle handle = ACPI_HANDLE(&pdev->dev); >> > > > + struct device_node node = pdev->dev.of_node; >> > > > + ... >> > > > + >> > > > + if (node) >> > > > + ret = device_probe_dt(pdev); >> > > > + else if (handle) >> > > > + ret = device_probe_acpi(pdev); >> > > > + else >> > > > + /* other initialisation */ >> > > > + ... >> > > > + /* Continue with any generic probe operations */ >> > > > + ... >> > > > +} >> > > >> > > This looks good to me, and it's also my preferred way of ACPI-enabling >> > > drivers. I guess we might discuss this at KS since it was a proposed >> > > topic there, and others will object. :) >> > >> > This is almost precisely the form of probe I want to see (it would be >> > nicer IMO to have separate entry points for ACPI / DT / platform data >> > that can all call a common probe core, but this isn't too far off). >> >> I disagree, but it's also not something we're looking to change at this time. >> We'll take that debate when you post the patches changing how device probing >> works. :-) > > Sure :) > > Cheers, > Mark. > >> > > > + >> > > > +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear >> > > > +the different names the driver is probed for, both from DT and from ACPI. >> > > > + >> > > > +module device tables :- >> > > > + >> > > > +static struct of_device_id virtio_mmio_match[] = { >> > > > + { .compatible = "virtio,mmio", }, >> > > > + {}, >> > > > +}; >> > > > +MODULE_DEVICE_TABLE(of, virtio_mmio_match); >> > > > + >> > > > +static const struct acpi_device_id virtio_mmio_acpi_match[] = { >> > > > + { "LNRO0005", }, >> > > > + { } >> > > >> > > No comma here, but a comma on DT. Probably make them equivalent for >> > > consistency (including space between the braces). >> > >> > The comma on the DT form should probably be dropped. The NULL sentinel's >> > only job is to delimit the list, so it never makes sense to place >> > something after it. >> >> Yep. >> >> >> -Olof >>
On Mon, Jul 28, 2014 at 05:33:13PM +0100, Olof Johansson wrote: > On Mon, Jul 28, 2014 at 11:12:29AM +0100, Mark Rutland wrote: > > On Mon, Jul 28, 2014 at 10:07:50AM +0100, Arnd Bergmann wrote: > > > On Saturday 26 July 2014 19:34:48 Olof Johansson wrote: > > > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > > > > +Relationship with Device Tree > > > > > +----------------------------- > > > > > + > > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > > > +exclusive with DT support at compile time. > > > > > + > > > > > +At boot time the kernel will only use one description method depending on > > > > > +parameters passed from the bootloader. > > > > > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > > > while earlier this year, acpi should still default to off -- if a DT > > > > and ACPI are both passed in, DT should at this time be given priority. > > > > > > I think this would be harder to do with the way that ACPI is passed in > > > to the kernel. IIRC, you always have a minimal DT information based on > > > the ARM64 boot protocol, but in the case of ACPI, this contains pointers > > > to the ACPI tables, which are then used for populating the Linux platform > > > devices (unless acpi=disabled is set), while the other contents of the > > > DTB may be present but we skip the of_platform_populate state. > > > > > > If this is correct, then replacing the firmware-generated dtb with a > > > user-provided on would implicitly remove the ACPI tables from visibility, > > > which is exactly what we want. > > > > That's not quite true. > > > > There might not be any DTB, or the user might have provided a DTB with > > only /chosen/bootargs. Trying to distinguish the many cases of possible > > DTB is broken as a concept. > > > > The EFI stub will attempt to get a DTB from somewhere (provided by > > filename or as a system table with a magic UUID), and if it can't find > > one will generate an empty stub DTB. > > > > Then it will go and perform some EFI memory setup, placing some > > properties in the DTB (which might be a stub) describing the EFI memory > > map. > > > > Then we boot the kernel proper, which sees the EFI pointers and looks > > for an ACPI table. If they are found, ACPI is used. Otherwise we attempt > > to use the DTB (which might be a stub). > > > > Unless ACPI is explcitly disabled, ACPI will be used if present. > > Ah, I saw this after I replied to Arnd's email. > > The above sounds more like how I envisioned things working, so based on that, > the default just needs to be flipped and we should be fine (i.e. ACPI needs to > be explicitly enabled). Sorry, but I don't follow your reasoning. What is the rationale for disabling ACPI by default? That's not going to work for ACPI-only systems which may not have any HW described in a DTB. I fear it will result in more pain on any systems which try to ship both ACPI and DT, where ACPI will not get the testing it requires, leading to a greater possibility of quirks/workarounds being required later. Thanks, Mark.
On Mon, Jul 28, 2014 at 11:37 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Jul 28, 2014 at 05:33:13PM +0100, Olof Johansson wrote: >> On Mon, Jul 28, 2014 at 11:12:29AM +0100, Mark Rutland wrote: >> > On Mon, Jul 28, 2014 at 10:07:50AM +0100, Arnd Bergmann wrote: >> > > On Saturday 26 July 2014 19:34:48 Olof Johansson wrote: >> > > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> > > > > +Relationship with Device Tree >> > > > > +----------------------------- >> > > > > + >> > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually >> > > > > +exclusive with DT support at compile time. >> > > > > + >> > > > > +At boot time the kernel will only use one description method depending on >> > > > > +parameters passed from the bootloader. >> > > > >> > > > Possibly overriden by kernel bootargs. And as debated for quite a >> > > > while earlier this year, acpi should still default to off -- if a DT >> > > > and ACPI are both passed in, DT should at this time be given priority. >> > > >> > > I think this would be harder to do with the way that ACPI is passed in >> > > to the kernel. IIRC, you always have a minimal DT information based on >> > > the ARM64 boot protocol, but in the case of ACPI, this contains pointers >> > > to the ACPI tables, which are then used for populating the Linux platform >> > > devices (unless acpi=disabled is set), while the other contents of the >> > > DTB may be present but we skip the of_platform_populate state. >> > > >> > > If this is correct, then replacing the firmware-generated dtb with a >> > > user-provided on would implicitly remove the ACPI tables from visibility, >> > > which is exactly what we want. >> > >> > That's not quite true. >> > >> > There might not be any DTB, or the user might have provided a DTB with >> > only /chosen/bootargs. Trying to distinguish the many cases of possible >> > DTB is broken as a concept. >> > >> > The EFI stub will attempt to get a DTB from somewhere (provided by >> > filename or as a system table with a magic UUID), and if it can't find >> > one will generate an empty stub DTB. >> > >> > Then it will go and perform some EFI memory setup, placing some >> > properties in the DTB (which might be a stub) describing the EFI memory >> > map. >> > >> > Then we boot the kernel proper, which sees the EFI pointers and looks >> > for an ACPI table. If they are found, ACPI is used. Otherwise we attempt >> > to use the DTB (which might be a stub). >> > >> > Unless ACPI is explcitly disabled, ACPI will be used if present. >> >> Ah, I saw this after I replied to Arnd's email. >> >> The above sounds more like how I envisioned things working, so based on that, >> the default just needs to be flipped and we should be fine (i.e. ACPI needs to >> be explicitly enabled). > > Sorry, but I don't follow your reasoning. What is the rationale for > disabling ACPI by default? > > That's not going to work for ACPI-only systems which may not have any HW > described in a DTB. I fear it will result in more pain on any systems > which try to ship both ACPI and DT, where ACPI will not get the testing > it requires, leading to a greater possibility of quirks/workarounds > being required later. We have said that we are not going to support ACPI-only systems at this time. If vendors choose to use ACPI only then we can choose to write a DT for them. We've been *very* clear on this. There's no difference between quirks "now or later". Once we need a quirk, it's around forever. So by giving them a chance to avoid some now, we can reduce the need to ever see them. The alternative is to pick them up now and forever carry them. For ACPI test coverage, I suggest you get your validation test suite work going if it hasn't been started. Beyond that, there's always testing with acpi=on. -Olof
On 2014-7-27 10:34, Olof Johansson wrote: > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> From: Graeme Gregory <graeme.gregory@linaro.org> >> >> Add documentation for the guidelines of how to use ACPI >> on ARM64. > > As the most vocal participant against ACPI being adopted, I would have > appreciated a cc on this patch set -- it's not like you were going for > a minimal set of cc recipients already. It makes it seem like you're > trying to sneak it past me for comments. Not cool. I know that's > probably not your intent, but still. My bad, I'm sorry for that. Actually it was not my intention, I was not playing with a full deck when I sent this patch set out, I missed someone else (such as Rob) in the CC list and I even added private mailing list in CC. I'm sure you will in the CC list with next version. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, Jul 24, 2014 at 02:19:14PM -0700, Randy Dunlap wrote: > On 07/24/2014 02:16 PM, Naresh Bhat wrote: > > > > On 24 July 2014 18:30, Hanjun Guo <hanjun.guo@linaro.org <mailto:hanjun.guo@linaro.org>> wrote: > > > > From: Graeme Gregory <graeme.gregory@linaro.org <mailto:graeme.gregory@linaro.org>> > > > > Add documentation for the guidelines of how to use ACPI > > on ARM64. > > > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org <mailto:graeme.gregory@linaro.org>> > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org <mailto:hanjun.guo@linaro.org>> > > --- > > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 240 insertions(+) > > create mode 100644 Documentation/arm64/arm-acpi.txt > > > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > > new file mode 100644 > > index 0000000..12cd550 > > --- /dev/null > > +++ b/Documentation/arm64/arm-acpi.txt > > @@ -0,0 +1,240 @@ > > +ACPI on ARMv8 Servers > > +--------------------- > > + > > +ACPI will be used for ARMv8 general purpose servers designed to follow > > +the SBSA specification (currently available to people with an ARM login at > > +http://silver.arm.com) > > + > > +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, > > +which is available at <http://www.uefi.org/acpi/specs>. > > + > > +If the machine does not meet these requirements then it is likely that Device > > +Tree (DT) is more suitable for the hardware. > > + > > +Relationship with Device Tree > > +----------------------------- > > + > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > +exclusive with DT support at compile time. > > + > > +At boot time the kernel will only use one description method depending on > > +parameters passed from the bootloader. > > + > > +Regardless of whether DT or ACPI is used, the kernel must always be capable > > +of booting with either scheme. > > + > > +When booting using ACPI tables the /chosen node in DT will still be parsed > > +to extract the kernel command line and initrd path. No other section of > > +the DT will be used. > > + > > +Booting using ACPI tables > > +------------------------- > > + > > +Currently, the only defined method to pass ACPI tables to the kernel on ARMv8 > > +is via the UEFI system configuration table. > > + > > +The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the > > +RSDP table (the table with the ACPI signature "RSD PTR "). > > + > > +The pointer to the RSDP table will be retrieved from EFI by the ACPI core. > > + > > +Processing of ACPI tables may be disabled by passing acpi=off on the kernel > > +command line. > > + > > +DO use an XSDT, RSDTs are deprecated and should not be used on arm64. They > > +only allow for 32bit addresses. > > + > > +DO NOT use the 32-bit address fields in the FADT, they are deprecated, the > > +64-bit alternatives MUST be used. > > + > > +The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT > > +and GTDT. If PCI is used the MCFG table MUST also be present. > > + > > +ACPI Detection > > +-------------- > > + > > +Drivers should determine their probe() type by checking for ACPI_HANDLE, > > +or .of_node, or other information in the device structure. This is > > +detailed further in the "Driver Recomendations" section. > > + > > +If the presence of ACPI needs to be detected at runtime, then check the value > > +of acpi_disabled. If CONFIG_ACPI not being set acpi_disabled will always be 1. > > + > > +Device Enumeration > > +------------------ > > + > > +Device descriptions in ACPI should use standard recognised ACPI interfaces. > > > > > > recognized > > Yeah, I saw all of these also, but we accept British or American spelling of these words. > Would be good to check for a consistent spelling in this doc though. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 28, 2014 at 09:44:59AM -0700, Olof Johansson wrote: > On Mon, Jul 28, 2014 at 11:06:54AM +0100, Mark Rutland wrote: [...] > > > > > > > +Relationship with Device Tree > > > > +----------------------------- > > > > + > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > > +exclusive with DT support at compile time. > > > > + > > > > +At boot time the kernel will only use one description method depending on > > > > +parameters passed from the bootloader. > > > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > > while earlier this year, acpi should still default to off -- if a DT > > > and ACPI are both passed in, DT should at this time be given priority. > > > > Why? I really don't see the logic in doing that. > > Seems like I am replying to your emails in reverse order. > > > Currently the kernel can only boot using DT, so DT will be used even if > > ACPI is present. In the presence of ACPI support in the kernel, ACPI > > would be used on said systems. > > For all the reasons we hashed out earlier this year: We can't trust that > vendors will know how to do ACPI from day one, so instead of loading up the > kernel with lots of quirks and workarounds, we'll default to not using it until > we're at a point where things look mature enough. > > The alternative is to keep this patch set out of the kernel. We can do that > too, but I don't think that really helps anyone. > > > From the discussions at the last Linaro Connect, this was seen as > > important for virtual machines which want to provide ACPI services to > > guests while still being able to boot DT-only kernels. I'll leave it to > > Grant, Rob, and Christoffer to cover that. > > Ok, waiting to see what they have to say then. > Hmm, a virtual machine implementaion may provide either a DT or ACPI (or both). I think the point at Linaro Connect was that for a first cut, there is no obvious need to provide ACPI to VMs and to be spec compliant, server kernels must be able to boot with DT-only. In most cases such systems will only access a few limited emulated devices (UART, GIC, RTC, flash controller) and VirtIO devices, which should just work using DT only. People are talking about adding ACPI for VM guests as well for various reasons (device passthrough for example) in which case I would always expect people to run UEFI inside their guests too (perhaps this is not 100% true in the case of Xen fast-boot scenarios though) and I would expect Linux to only see the little stub DT and ACPI. Does this clarify anything or add futher to the confusion? -Christoffer
On Thu, Jul 24, 2014 at 09:00:25PM +0800, Hanjun Guo wrote: > From: Graeme Gregory <graeme.gregory@linaro.org> > > Add documentation for the guidelines of how to use ACPI > on ARM64. > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > Documentation/arm64/arm-acpi.txt | 240 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 240 insertions(+) > create mode 100644 Documentation/arm64/arm-acpi.txt > > diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt > new file mode 100644 > index 0000000..12cd550 > --- /dev/null > +++ b/Documentation/arm64/arm-acpi.txt > @@ -0,0 +1,240 @@ > +ACPI on ARMv8 Servers > +--------------------- [...] > + > + > +There exists in the ACPI 5.1 specification no standard binding for these objects > +to enable programmable levels or rates so this should be avoid if possible and > +the resources set to appropriate level by the firmware. If this is not possible > +then any manipulation should be abstracted in ASL. > + I'm not a native English speaker, but this wording sounds strange to me. I would suggest "There exists no specification in the the..." or more simply "The ACPI 5.1 specification does not contain any standard...". -Christoffer -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tuesday 29 July 2014 12:29:40 Christoffer Dall wrote: > On Mon, Jul 28, 2014 at 09:44:59AM -0700, Olof Johansson wrote: > > On Mon, Jul 28, 2014 at 11:06:54AM +0100, Mark Rutland wrote: > > > > > From the discussions at the last Linaro Connect, this was seen as > > > important for virtual machines which want to provide ACPI services to > > > guests while still being able to boot DT-only kernels. I'll leave it to > > > Grant, Rob, and Christoffer to cover that. > > > > Ok, waiting to see what they have to say then. > > > > Hmm, a virtual machine implementaion may provide either a DT or ACPI (or > both). I think the point at Linaro Connect was that for a first cut, > there is no obvious need to provide ACPI to VMs and to be spec > compliant, server kernels must be able to boot with DT-only. In most > cases such systems will only access a few limited emulated devices > (UART, GIC, RTC, flash controller) and VirtIO devices, which should just > work using DT only. Right > People are talking about adding ACPI for VM guests as well for various > reasons (device passthrough for example) in which case I would always > expect people to run UEFI inside their guests too (perhaps this is not > 100% true in the case of Xen fast-boot scenarios though) and I would > expect Linux to only see the little stub DT and ACPI. > > Does this clarify anything or add futher to the confusion? I think the only reason that was given for ACPI in a virtual machine was Red Hat's insistence on intentionally breaking DT support in their enterprise distro. I believe both the cases of device pass-through and running something other than UEFI are outside of the scope of the standard virtual machine model, for different reasons. Outside of that model, anybody is of course free to run whatever they like in their guests. Arnd
On Tue, Jul 29, 2014 at 11:29:40AM +0100, Christoffer Dall wrote: > On Mon, Jul 28, 2014 at 09:44:59AM -0700, Olof Johansson wrote: > > On Mon, Jul 28, 2014 at 11:06:54AM +0100, Mark Rutland wrote: > > [...] > > > > > > > > > > +Relationship with Device Tree > > > > > +----------------------------- > > > > > + > > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > > > +exclusive with DT support at compile time. > > > > > + > > > > > +At boot time the kernel will only use one description method depending on > > > > > +parameters passed from the bootloader. > > > > > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > > > while earlier this year, acpi should still default to off -- if a DT > > > > and ACPI are both passed in, DT should at this time be given priority. > > > > > > Why? I really don't see the logic in doing that. > > > > Seems like I am replying to your emails in reverse order. > > > > > Currently the kernel can only boot using DT, so DT will be used even if > > > ACPI is present. In the presence of ACPI support in the kernel, ACPI > > > would be used on said systems. > > > > For all the reasons we hashed out earlier this year: We can't trust that > > vendors will know how to do ACPI from day one, so instead of loading up the > > kernel with lots of quirks and workarounds, we'll default to not using it until > > we're at a point where things look mature enough. > > > > The alternative is to keep this patch set out of the kernel. We can do that > > too, but I don't think that really helps anyone. > > > > > From the discussions at the last Linaro Connect, this was seen as > > > important for virtual machines which want to provide ACPI services to > > > guests while still being able to boot DT-only kernels. I'll leave it to > > > Grant, Rob, and Christoffer to cover that. > > > > Ok, waiting to see what they have to say then. > > > > Hmm, a virtual machine implementaion may provide either a DT or ACPI (or > both). I think the point at Linaro Connect was that for a first cut, > there is no obvious need to provide ACPI to VMs and to be spec > compliant, server kernels must be able to boot with DT-only. In most > cases such systems will only access a few limited emulated devices > (UART, GIC, RTC, flash controller) and VirtIO devices, which should just > work using DT only. > > People are talking about adding ACPI for VM guests as well for various > reasons (device passthrough for example) in which case I would always > expect people to run UEFI inside their guests too (perhaps this is not > 100% true in the case of Xen fast-boot scenarios though) and I would > expect Linux to only see the little stub DT and ACPI. > > Does this clarify anything or add futher to the confusion? I was under the impression that there was a case where we'd have a DT with HW description in a VM which also had ACPI tables, to enable a kernel without ACPI support to boot in said VM (albeit with reduced functionality). I may have got confused since the VM standards discussion at LCA-14. If we we expect a hypervisor to provide DT only by default (for compatibility), and ACPI only when the user has explicitly enabled it for an ACPI-specific feature, then that sounds OK. Thanks, Mark.
On Tue, Jul 29, 2014 at 11:55:43AM +0100, Mark Rutland wrote: > On Tue, Jul 29, 2014 at 11:29:40AM +0100, Christoffer Dall wrote: > > On Mon, Jul 28, 2014 at 09:44:59AM -0700, Olof Johansson wrote: > > > On Mon, Jul 28, 2014 at 11:06:54AM +0100, Mark Rutland wrote: > > > > [...] > > > > > > > > > > > > > +Relationship with Device Tree > > > > > > +----------------------------- > > > > > > + > > > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > > > > > +exclusive with DT support at compile time. > > > > > > + > > > > > > +At boot time the kernel will only use one description method depending on > > > > > > +parameters passed from the bootloader. > > > > > > > > > > Possibly overriden by kernel bootargs. And as debated for quite a > > > > > while earlier this year, acpi should still default to off -- if a DT > > > > > and ACPI are both passed in, DT should at this time be given priority. > > > > > > > > Why? I really don't see the logic in doing that. > > > > > > Seems like I am replying to your emails in reverse order. > > > > > > > Currently the kernel can only boot using DT, so DT will be used even if > > > > ACPI is present. In the presence of ACPI support in the kernel, ACPI > > > > would be used on said systems. > > > > > > For all the reasons we hashed out earlier this year: We can't trust that > > > vendors will know how to do ACPI from day one, so instead of loading up the > > > kernel with lots of quirks and workarounds, we'll default to not using it until > > > we're at a point where things look mature enough. > > > > > > The alternative is to keep this patch set out of the kernel. We can do that > > > too, but I don't think that really helps anyone. > > > > > > > From the discussions at the last Linaro Connect, this was seen as > > > > important for virtual machines which want to provide ACPI services to > > > > guests while still being able to boot DT-only kernels. I'll leave it to > > > > Grant, Rob, and Christoffer to cover that. > > > > > > Ok, waiting to see what they have to say then. > > > > > > > Hmm, a virtual machine implementaion may provide either a DT or ACPI (or > > both). I think the point at Linaro Connect was that for a first cut, > > there is no obvious need to provide ACPI to VMs and to be spec > > compliant, server kernels must be able to boot with DT-only. In most > > cases such systems will only access a few limited emulated devices > > (UART, GIC, RTC, flash controller) and VirtIO devices, which should just > > work using DT only. > > > > People are talking about adding ACPI for VM guests as well for various > > reasons (device passthrough for example) in which case I would always > > expect people to run UEFI inside their guests too (perhaps this is not > > 100% true in the case of Xen fast-boot scenarios though) and I would > > expect Linux to only see the little stub DT and ACPI. > > > > Does this clarify anything or add futher to the confusion? > > I was under the impression that there was a case where we'd have a DT > with HW description in a VM which also had ACPI tables, to enable a > kernel without ACPI support to boot in said VM (albeit with reduced > functionality). > > I may have got confused since the VM standards discussion at LCA-14. I took a look at the video [1,2] of said session [3], and it looks like I was the one arguing for the case of full description in both DT and ACPI, so either I was confused or I've forgotten some hallway discussion. I think my reasoning was somewhat flawed; if the hypervisor defaults to providing DT, with an option to use ACPI in certain cases (where not all guests would be expected to work), then that would cover the "grab an iso, it just works™" use case for Linux images while allowing for the cases where people want ACPI in the VM. The only issue with that would be catering for OSs which support ACPI but not DT or whose DT support is not Linux-compatible. Thanks, Mark. [1] http://www.youtube.com/watch?v=Qh3SX3p3B74 [2] http://people.linaro.org/linaro-connect/lca14/videos/03-03-Monday/LCA14-101-%20ARM%20VM%20Standards.mp4 [3] http://lca-14.zerista.com/event/member/102395
On 29 July 2014 13:28, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Jul 29, 2014 at 11:55:43AM +0100, Mark Rutland wrote: >> On Tue, Jul 29, 2014 at 11:29:40AM +0100, Christoffer Dall wrote: >> > On Mon, Jul 28, 2014 at 09:44:59AM -0700, Olof Johansson wrote: >> > > On Mon, Jul 28, 2014 at 11:06:54AM +0100, Mark Rutland wrote: >> > >> > [...] >> > >> > > > > >> > > > > > +Relationship with Device Tree >> > > > > > +----------------------------- >> > > > > > + >> > > > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually >> > > > > > +exclusive with DT support at compile time. >> > > > > > + >> > > > > > +At boot time the kernel will only use one description method depending on >> > > > > > +parameters passed from the bootloader. >> > > > > >> > > > > Possibly overriden by kernel bootargs. And as debated for quite a >> > > > > while earlier this year, acpi should still default to off -- if a DT >> > > > > and ACPI are both passed in, DT should at this time be given priority. >> > > > >> > > > Why? I really don't see the logic in doing that. >> > > >> > > Seems like I am replying to your emails in reverse order. >> > > >> > > > Currently the kernel can only boot using DT, so DT will be used even if >> > > > ACPI is present. In the presence of ACPI support in the kernel, ACPI >> > > > would be used on said systems. >> > > >> > > For all the reasons we hashed out earlier this year: We can't trust that >> > > vendors will know how to do ACPI from day one, so instead of loading up the >> > > kernel with lots of quirks and workarounds, we'll default to not using it until >> > > we're at a point where things look mature enough. >> > > >> > > The alternative is to keep this patch set out of the kernel. We can do that >> > > too, but I don't think that really helps anyone. >> > > >> > > > From the discussions at the last Linaro Connect, this was seen as >> > > > important for virtual machines which want to provide ACPI services to >> > > > guests while still being able to boot DT-only kernels. I'll leave it to >> > > > Grant, Rob, and Christoffer to cover that. >> > > >> > > Ok, waiting to see what they have to say then. >> > > >> > >> > Hmm, a virtual machine implementaion may provide either a DT or ACPI (or >> > both). I think the point at Linaro Connect was that for a first cut, >> > there is no obvious need to provide ACPI to VMs and to be spec >> > compliant, server kernels must be able to boot with DT-only. In most >> > cases such systems will only access a few limited emulated devices >> > (UART, GIC, RTC, flash controller) and VirtIO devices, which should just >> > work using DT only. >> > >> > People are talking about adding ACPI for VM guests as well for various >> > reasons (device passthrough for example) in which case I would always >> > expect people to run UEFI inside their guests too (perhaps this is not >> > 100% true in the case of Xen fast-boot scenarios though) and I would >> > expect Linux to only see the little stub DT and ACPI. >> > >> > Does this clarify anything or add futher to the confusion? >> >> I was under the impression that there was a case where we'd have a DT >> with HW description in a VM which also had ACPI tables, to enable a >> kernel without ACPI support to boot in said VM (albeit with reduced >> functionality). >> >> I may have got confused since the VM standards discussion at LCA-14. > > I took a look at the video [1,2] of said session [3], and it looks like > I was the one arguing for the case of full description in both DT and > ACPI, so either I was confused or I've forgotten some hallway > discussion. > > I think my reasoning was somewhat flawed; if the hypervisor defaults to > providing DT, with an option to use ACPI in certain cases (where not all > guests would be expected to work), then that would cover the "grab an > iso, it just works™" use case for Linux images while allowing for > the cases where people want ACPI in the VM. > > The only issue with that would be catering for OSs which support ACPI > but not DT or whose DT support is not Linux-compatible. > So I think this is where the spec is really useful. Either you're spec compliant or you're not. Distro images will need to support DT if they're spec compliant to v1 of the spec (yes, it's on my todo list to publish this properly, but the hold up is not all my fault). A v2 of the spec may mention ACPI, and it may not, and we can deal with that problem then. For reference, Red Hat's current arguing point for ACPI in VMs is hotplug of things like CPUs and memory for very large VMs, but I haven't thought too carefully about this just yet, as I don't have a 100+ core ARM 64-bit hardware lying around... -Christoffer
On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: > > For reference, Red Hat's current arguing point for ACPI in VMs is > hotplug of things like CPUs and memory for very large VMs, but I > haven't thought too carefully about this just yet, as I don't have a > 100+ core ARM 64-bit hardware lying around... I thought you could run guests with more virtual CPUs that you have physical CPUs on the host. Regarding CPU and memory hotplug, don't we already have PSCI and xen-balloon/virtio-balloon for that? Arnd
On Tue, Jul 29, 2014 at 01:52:40PM +0100, Arnd Bergmann wrote: > On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: > > > > For reference, Red Hat's current arguing point for ACPI in VMs is > > hotplug of things like CPUs and memory for very large VMs, but I > > haven't thought too carefully about this just yet, as I don't have a > > 100+ core ARM 64-bit hardware lying around... > > I thought you could run guests with more virtual CPUs that you have > physical CPUs on the host. > > Regarding CPU and memory hotplug, don't we already have PSCI and > xen-balloon/virtio-balloon for that? PSCI (0.1) was there for guests from the start, and ACPI doesn't do anything different w.r.t. PSCI other than requiring PSCI 0.2 (which can be used by guests supporting only PSCI 0.1). So there's no magic for CPU hotplug provided by ACPI. Do either of the balloon drivers allow for memory to be hot-added to a system initially provisioned with less? Thanks, Mark.
On 29 July 2014 15:08, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Jul 29, 2014 at 01:52:40PM +0100, Arnd Bergmann wrote: >> On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: >> > >> > For reference, Red Hat's current arguing point for ACPI in VMs is >> > hotplug of things like CPUs and memory for very large VMs, but I >> > haven't thought too carefully about this just yet, as I don't have a >> > 100+ core ARM 64-bit hardware lying around... >> >> I thought you could run guests with more virtual CPUs that you have >> physical CPUs on the host. >> >> Regarding CPU and memory hotplug, don't we already have PSCI and >> xen-balloon/virtio-balloon for that? > > PSCI (0.1) was there for guests from the start, and ACPI doesn't do > anything different w.r.t. PSCI other than requiring PSCI 0.2 (which can > be used by guests supporting only PSCI 0.1). So there's no magic for > CPU hotplug provided by ACPI. With PSCI you can only provide your VM a bunch of CPUs and say that they're all turned off, and then turn some of them on later. I honestly don't know if you can do proper CPU hotplug with ACPI, but the RH guys seem to argue that you can. Again, I didn't think too carefully about this. > > Do either of the balloon drivers allow for memory to be hot-added to a > system initially provisioned with less? > No, it's just about reclaiming memory. Same argument as above. -Christoffer
On 29 July 2014 14:52, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: >> >> For reference, Red Hat's current arguing point for ACPI in VMs is >> hotplug of things like CPUs and memory for very large VMs, but I >> haven't thought too carefully about this just yet, as I don't have a >> 100+ core ARM 64-bit hardware lying around... > > I thought you could run guests with more virtual CPUs that you have > physical CPUs on the host. > you can, my sentence was meant to be a bit tongue-in-cheek, running 100 VCPUs on the Foundation Model (which is the only thing I have on my desk as of now) is not very useful, so I just don't care at this point. Let me see or give me access to something with at least 8 physical cores before I start caring (with my community hat on, as a Linaro employee I may be told to care). > Regarding CPU and memory hotplug, don't we already have PSCI and > xen-balloon/virtio-balloon for that? > Virtio-balloon don't do anything for you wrt. CPUs, wrt. memory you have to provision it beforehand. -Christoffer
On Tue, Jul 29, 2014 at 02:31:16PM +0100, Christoffer Dall wrote: > On 29 July 2014 15:08, Mark Rutland <mark.rutland@arm.com> wrote: > > On Tue, Jul 29, 2014 at 01:52:40PM +0100, Arnd Bergmann wrote: > >> On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: > >> > > >> > For reference, Red Hat's current arguing point for ACPI in VMs is > >> > hotplug of things like CPUs and memory for very large VMs, but I > >> > haven't thought too carefully about this just yet, as I don't have a > >> > 100+ core ARM 64-bit hardware lying around... > >> > >> I thought you could run guests with more virtual CPUs that you have > >> physical CPUs on the host. > >> > >> Regarding CPU and memory hotplug, don't we already have PSCI and > >> xen-balloon/virtio-balloon for that? > > > > PSCI (0.1) was there for guests from the start, and ACPI doesn't do > > anything different w.r.t. PSCI other than requiring PSCI 0.2 (which can > > be used by guests supporting only PSCI 0.1). So there's no magic for > > CPU hotplug provided by ACPI. > > With PSCI you can only provide your VM a bunch of CPUs and say that > they're all turned off, and then turn some of them on later. I > honestly don't know if you can do proper CPU hotplug with ACPI, but > the RH guys seem to argue that you can. Again, I didn't think too > carefully about this. Ah, I see. That would make some sense. > > > > Do either of the balloon drivers allow for memory to be hot-added to a > > system initially provisioned with less? > > > No, it's just about reclaiming memory. Same argument as above. Ok. Thanks for the clarifications. Cheers, Mark.
On 29 July 2014 16:41, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 29 July 2014 15:31:16 Christoffer Dall wrote: >> On 29 July 2014 15:08, Mark Rutland <mark.rutland@arm.com> wrote: >> > On Tue, Jul 29, 2014 at 01:52:40PM +0100, Arnd Bergmann wrote: >> >> On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: >> >> > >> >> > For reference, Red Hat's current arguing point for ACPI in VMs is >> >> > hotplug of things like CPUs and memory for very large VMs, but I >> >> > haven't thought too carefully about this just yet, as I don't have a >> >> > 100+ core ARM 64-bit hardware lying around... >> >> >> >> I thought you could run guests with more virtual CPUs that you have >> >> physical CPUs on the host. >> >> >> >> Regarding CPU and memory hotplug, don't we already have PSCI and >> >> xen-balloon/virtio-balloon for that? >> > >> > PSCI (0.1) was there for guests from the start, and ACPI doesn't do >> > anything different w.r.t. PSCI other than requiring PSCI 0.2 (which can >> > be used by guests supporting only PSCI 0.1). So there's no magic for >> > CPU hotplug provided by ACPI. >> >> With PSCI you can only provide your VM a bunch of CPUs and say that >> they're all turned off, and then turn some of them on later. I >> honestly don't know if you can do proper CPU hotplug with ACPI, but >> the RH guys seem to argue that you can. Again, I didn't think too >> carefully about this. > > Xen does this in drivers/xen/cpu_hotplug.c, acpi does it in > drivers/acpi/acpi_processor.c. > >> > Do either of the balloon drivers allow for memory to be hot-added to a >> > system initially provisioned with less? >> > >> No, it's just about reclaiming memory. Same argument as above. > > Again, Xen seems to be able to add more memory: > > config XEN_BALLOON_MEMORY_HOTPLUG > bool "Memory hotplug support for Xen balloon driver" > default n > depends on XEN_BALLOON && MEMORY_HOTPLUG > help > Memory hotplug support for Xen balloon driver allows expanding memory > available for the system above limit declared at system startup. > It is very useful on critical systems which require long > run without rebooting. > ... > > The same goes for hyperv, s390 and vmware. It should not be hard to add it > for KVM. > Absolutely, and I would prefer doing that over adding ACPI in guests as things stand right now. We can have another fun discussion about this at LCU... -Christoffer
On 2014-7-29 21:31, Christoffer Dall wrote: > On 29 July 2014 15:08, Mark Rutland <mark.rutland@arm.com> wrote: >> On Tue, Jul 29, 2014 at 01:52:40PM +0100, Arnd Bergmann wrote: >>> On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: >>>> >>>> For reference, Red Hat's current arguing point for ACPI in VMs is >>>> hotplug of things like CPUs and memory for very large VMs, but I >>>> haven't thought too carefully about this just yet, as I don't have a >>>> 100+ core ARM 64-bit hardware lying around... >>> >>> I thought you could run guests with more virtual CPUs that you have >>> physical CPUs on the host. >>> >>> Regarding CPU and memory hotplug, don't we already have PSCI and >>> xen-balloon/virtio-balloon for that? >> >> PSCI (0.1) was there for guests from the start, and ACPI doesn't do >> anything different w.r.t. PSCI other than requiring PSCI 0.2 (which can >> be used by guests supporting only PSCI 0.1). So there's no magic for >> CPU hotplug provided by ACPI. > > With PSCI you can only provide your VM a bunch of CPUs and say that > they're all turned off, and then turn some of them on later. Yes, agreed. > I honestly don't know if you can do proper CPU hotplug with ACPI, but > the RH guys seem to argue that you can. Again, I didn't think too > carefully about this. Yes, we can do proper CPU hotplug with ACPI based physical hotplug (named dynamic device configuration in ACPI spec), you can refer to section 6.3 "Device Insertion, Removal, and Status Objects" in ACPI 5.1. There are mechanisms for handling dynamic insertion and removal of devices and for determining device and notification processing status. When removing a processor, a) we will call PSCI or similar interface to offline a CPU from OS, then OS will not use it any more; b) call ACPI API to trim the resources that allocated during device enumeration; c) Call ACPI method _EJ0 and then will notify firmware or hypervisor device will be removed, jump to firmware or hypervisor to remove the device from that level; CPU hotplug with ACPI is pretty mature on x86 and it works on VM too. I prepared some patches to support CPU hotplug on ARM64 and simulated the hot add/remove notify to test those patches, it worked as expected, I will send them out when this patchset is ready for mainline. Thanks Hanjun
On 30 July 2014 08:47, Hanjun Guo <hanjun.guo@linaro.org> wrote: > On 2014-7-29 21:31, Christoffer Dall wrote: >> On 29 July 2014 15:08, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Tue, Jul 29, 2014 at 01:52:40PM +0100, Arnd Bergmann wrote: >>>> On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: >>>>> >>>>> For reference, Red Hat's current arguing point for ACPI in VMs is >>>>> hotplug of things like CPUs and memory for very large VMs, but I >>>>> haven't thought too carefully about this just yet, as I don't have a >>>>> 100+ core ARM 64-bit hardware lying around... >>>> >>>> I thought you could run guests with more virtual CPUs that you have >>>> physical CPUs on the host. >>>> >>>> Regarding CPU and memory hotplug, don't we already have PSCI and >>>> xen-balloon/virtio-balloon for that? >>> >>> PSCI (0.1) was there for guests from the start, and ACPI doesn't do >>> anything different w.r.t. PSCI other than requiring PSCI 0.2 (which can >>> be used by guests supporting only PSCI 0.1). So there's no magic for >>> CPU hotplug provided by ACPI. >> >> With PSCI you can only provide your VM a bunch of CPUs and say that >> they're all turned off, and then turn some of them on later. > > Yes, agreed. > >> I honestly don't know if you can do proper CPU hotplug with ACPI, but >> the RH guys seem to argue that you can. Again, I didn't think too >> carefully about this. > > Yes, we can do proper CPU hotplug with ACPI based physical hotplug (named > dynamic device configuration in ACPI spec), you can refer to section 6.3 > "Device Insertion, Removal, and Status Objects" in ACPI 5.1. > > There are mechanisms for handling dynamic insertion and removal of devices > and for determining device and notification processing status. When removing > a processor, > > a) we will call PSCI or similar interface to offline a CPU from OS, then > OS will not use it any more; > > b) call ACPI API to trim the resources that allocated during device > enumeration; > > c) Call ACPI method _EJ0 and then will notify firmware or hypervisor device > will be removed, jump to firmware or hypervisor to remove the device > from that level; When you say notify hypervisor, would we really need a hypervisor-specific interface if you're running UEFI as your firmware? Can you not just call whatever UEFI service to remove a CPU and let that UEFI implementation deal with the hypervisor/hardware interface? -Christoffer
On 2014-7-30 15:14, Christoffer Dall wrote: > On 30 July 2014 08:47, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> On 2014-7-29 21:31, Christoffer Dall wrote: >>> On 29 July 2014 15:08, Mark Rutland <mark.rutland@arm.com> wrote: >>>> On Tue, Jul 29, 2014 at 01:52:40PM +0100, Arnd Bergmann wrote: >>>>> On Tuesday 29 July 2014 14:37:38 Christoffer Dall wrote: >>>>>> >>>>>> For reference, Red Hat's current arguing point for ACPI in VMs is >>>>>> hotplug of things like CPUs and memory for very large VMs, but I >>>>>> haven't thought too carefully about this just yet, as I don't have a >>>>>> 100+ core ARM 64-bit hardware lying around... >>>>> >>>>> I thought you could run guests with more virtual CPUs that you have >>>>> physical CPUs on the host. >>>>> >>>>> Regarding CPU and memory hotplug, don't we already have PSCI and >>>>> xen-balloon/virtio-balloon for that? >>>> >>>> PSCI (0.1) was there for guests from the start, and ACPI doesn't do >>>> anything different w.r.t. PSCI other than requiring PSCI 0.2 (which can >>>> be used by guests supporting only PSCI 0.1). So there's no magic for >>>> CPU hotplug provided by ACPI. >>> >>> With PSCI you can only provide your VM a bunch of CPUs and say that >>> they're all turned off, and then turn some of them on later. >> >> Yes, agreed. >> >>> I honestly don't know if you can do proper CPU hotplug with ACPI, but >>> the RH guys seem to argue that you can. Again, I didn't think too >>> carefully about this. >> >> Yes, we can do proper CPU hotplug with ACPI based physical hotplug (named >> dynamic device configuration in ACPI spec), you can refer to section 6.3 >> "Device Insertion, Removal, and Status Objects" in ACPI 5.1. >> >> There are mechanisms for handling dynamic insertion and removal of devices >> and for determining device and notification processing status. When removing >> a processor, >> >> a) we will call PSCI or similar interface to offline a CPU from OS, then >> OS will not use it any more; >> >> b) call ACPI API to trim the resources that allocated during device >> enumeration; >> >> c) Call ACPI method _EJ0 and then will notify firmware or hypervisor device >> will be removed, jump to firmware or hypervisor to remove the device >> from that level; > > When you say notify hypervisor, would we really need a > hypervisor-specific interface if you're running UEFI as your firmware? > Can you not just call whatever UEFI service to remove a CPU and let > that UEFI implementation deal with the hypervisor/hardware interface? Yes, it should work as you said, OS will notfy UEFI and then UEFI deal with hypervisor interface, sorry, I didn't describe it clearly. Thanks Hanjun
On Mon, Jul 28, 2014 at 07:27:52PM +0100, Olof Johansson wrote: > On Mon, Jul 28, 2014 at 10:00 AM, Mark Rutland <mark.rutland@arm.com> wrote: > > On Mon, Jul 28, 2014 at 05:27:50PM +0100, Olof Johansson wrote: > >> On Mon, Jul 28, 2014 at 11:07:50AM +0200, Arnd Bergmann wrote: > >> > On Saturday 26 July 2014 19:34:48 Olof Johansson wrote: > >> > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > >> > > > +Relationship with Device Tree > >> > > > +----------------------------- > >> > > > + > >> > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > >> > > > +exclusive with DT support at compile time. > >> > > > + > >> > > > +At boot time the kernel will only use one description method depending on > >> > > > +parameters passed from the bootloader. > >> > > > >> > > Possibly overriden by kernel bootargs. And as debated for quite a > >> > > while earlier this year, acpi should still default to off -- if a DT > >> > > and ACPI are both passed in, DT should at this time be given priority. > >> > > >> > I think this would be harder to do with the way that ACPI is passed in > >> > to the kernel. IIRC, you always have a minimal DT information based on > >> > the ARM64 boot protocol, but in the case of ACPI, this contains pointers > >> > to the ACPI tables, which are then used for populating the Linux platform > >> > devices (unless acpi=disabled is set), while the other contents of the > >> > DTB may be present but we skip the of_platform_populate state. > >> > >> How can it be harder to do? If you support acpi=off, then you should support > >> acpi=on. > >> > >> Another alternative would be to have an early fixup that stubs out > >> the acpi properties from the DTB unless there's an 'acpi' or 'acpi=on' > >> argument on the cmdline. Not quite as tidy a solution, though. > > > > I don't follow: > > > > If you want to disable ACPI, you can pass acpi=off. This is your > > workaround for broken ACPI (and only if you happen to have wrirten your > > own DTB, because many/most ACPI systems WILL NOT have a DTB to begin > > with). > > All ACPI should be assumed broken at this time, so acpi=off _must_ be > the default. (catching up with emails after holiday and I may have missed some of your arguments) If we consider ACPI unusable on ARM but we still want to start merging patches, we should rather make the config option depend on BROKEN (though if it is that unusable that no real platform can use it, I would rather not merge it at all at this stage). I don't really see what requiring acpi=on explicitly would buy us as vendors firmware providing only ACPI tables would simply always set this command line option. Defaulting to off is not really a way for mandating valid DT to be provided by firmware (rather a statement we try to make that ACPI is not a first class citizen yet). I would also expect platforms to be ACPI only and handle for example D-states via AML. If they work fine, do we really need to bother adding DT support for them, potentially with additional drivers? IOW, do we want to mandate all ARM(v8) platforms to work with "acpi=off" out of firmware? > > If you have ACPI, for what technical reason would you not attempt to use > > it by default? > > Because it will be broken, or the company you bought the machine from > implemented it wrong because they're still learning how to do this > too. The original doc even mentioned that there are parts of the > binding that aren't even sorted out. The _DSD stuff, too. I assume with the current patches, a (sane) platform could still boot and run without the missing features listed in the document, though it may not be optimal. We should definitely be strict and not accept quirks in the kernel to work around missing features until the UEFI forum accepted the specification. > Overall, it's not yet ready to be the default boot method. Then make CONFIG_ACPI default off and depend on EXPERT as a better statement (but as with acpi=on, it's still just a statement we try to make). Once the concerns are addressed, we make it an equal citizen. > > There will be systems which _DO NOT_ have any sort of DTB to begin with. > > Listen, we've been really clear about this: DT is the default that > everybody has to use for mainline kernel for the near term. If you > have an ACPI-only system, then you either have to write a DT for it, > or you won't be booting mainline on it. OK, this answers one of my questions above (I may have forgotten the old threads but at the time we were primarily talking about ACPI 5.0 which wasn't of much use on ARM). So you want to mandate DT for all platforms, even if some choose ACPI only longer term. I'm not arguing against this (especially for the first generation of devices), but how would default acpi=off enforce it? The firmware would simply pass acpi=on by default and run mainline just fine (as well as it can based on the implemented features). We should also not accepted any ACPI features in drivers without a DT counterpart. > If they really want to, they can boot with acpi=on instead. It's not > like it's hard to add. And this would be the default argument passed by their firmware. As I said, it's rather just a statement the kernel people want to make, nothing more. If you want to make the DT requirement stricter, what about adding code for checking the validity of the DT passed even though it's booting with ACPI? IMO, it's not worth it, we should rather be very strict with what ACPI code we accept in the kernel (primarily drivers, avoid random clock descriptors etc). If a platform cannot work (optimally) with the strict ACPI set, the vendor either provides DT or shouldn't bother us with upstreaming at all.
On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote: > On Mon, Jul 28, 2014 at 07:27:52PM +0100, Olof Johansson wrote: > > On Mon, Jul 28, 2014 at 10:00 AM, Mark Rutland <mark.rutland@arm.com> wrote: > > > On Mon, Jul 28, 2014 at 05:27:50PM +0100, Olof Johansson wrote: > > >> On Mon, Jul 28, 2014 at 11:07:50AM +0200, Arnd Bergmann wrote: > > >> > On Saturday 26 July 2014 19:34:48 Olof Johansson wrote: > > >> > > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > >> > > > +Relationship with Device Tree > > >> > > > +----------------------------- > > >> > > > + > > >> > > > +ACPI support in drivers and subsystems for ARMv8 should never be mutually > > >> > > > +exclusive with DT support at compile time. > > >> > > > + > > >> > > > +At boot time the kernel will only use one description method depending on > > >> > > > +parameters passed from the bootloader. > > >> > > > > >> > > Possibly overriden by kernel bootargs. And as debated for quite a > > >> > > while earlier this year, acpi should still default to off -- if a DT > > >> > > and ACPI are both passed in, DT should at this time be given priority. > > >> > > > >> > I think this would be harder to do with the way that ACPI is passed in > > >> > to the kernel. IIRC, you always have a minimal DT information based on > > >> > the ARM64 boot protocol, but in the case of ACPI, this contains pointers > > >> > to the ACPI tables, which are then used for populating the Linux platform > > >> > devices (unless acpi=disabled is set), while the other contents of the > > >> > DTB may be present but we skip the of_platform_populate state. > > >> > > >> How can it be harder to do? If you support acpi=off, then you should support > > >> acpi=on. > > >> > > >> Another alternative would be to have an early fixup that stubs out > > >> the acpi properties from the DTB unless there's an 'acpi' or 'acpi=on' > > >> argument on the cmdline. Not quite as tidy a solution, though. > > > > > > I don't follow: > > > > > > If you want to disable ACPI, you can pass acpi=off. This is your > > > workaround for broken ACPI (and only if you happen to have wrirten your > > > own DTB, because many/most ACPI systems WILL NOT have a DTB to begin > > > with). > > > > All ACPI should be assumed broken at this time, so acpi=off _must_ be > > the default. > > (catching up with emails after holiday and I may have missed some of > your arguments) > > If we consider ACPI unusable on ARM but we still want to start merging > patches, we should rather make the config option depend on BROKEN > (though if it is that unusable that no real platform can use it, I would > rather not merge it at all at this stage). I agree here. I would recommend creating a separate branch for that living outside of the mainline kernel and merging it when there are real users.
> Additional driver specific > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > +_DSD (ACPI Section 6.2.5). Re: _DSD Yes. Re: _DSM No, not if it can be handled by _DSD. cheers, Len Brown, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 15, 2014 at 06:43:10PM -0400, Len Brown wrote: > > Additional driver specific > > +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or > > +_DSD (ACPI Section 6.2.5). > > Re: _DSD > > Yes. > > Re: _DSM > > No, not if it can be handled by _DSD. > > cheers, > Len Brown, Intel Open Source Technology Center Thanks good point, Ill update that paragraph to indicate preference on _DSD. Graeme
On Thu, 24 Jul 2014 21:00:25 +0800, Hanjun Guo <hanjun.guo@linaro.org> wrote: > +Clocks > +------ > + > +Like clocks that are part of the power resources there is no standard way > +to represent a clock tree in ACPI 5.1 in a similar manner to how it is > +described in DT. > + > +Devices affected by this include things like UARTs, SoC driven LCD displays, > +etc. > + > +The firmware for example UEFI should initialise these clocks to fixed working > +values before the kernel is executed. If a driver requires to know rates of > +clocks set by firmware then they can be passed to kernel using _DSD. > + > +example :- > + > +Device (CLK0) { > + ... > + > + Name (_DSD, Package() { > + ToUUID("XXXXX"), > + Package() { > + Package(2) {"#clock-cells", 0}, > + Package(2) {"clock-frequency", "10000"} > + } > + }) > + > + ... > +} > + > +Device (USR1) { > + ... > + > + Name (_DSD, Package() { > + ToUUID("XXXXX"), > + Package() { > + Package(2) {"clocks", Package() {1, ^CLK0}}}, > + } > + }) > + > + ... > +} Really? This looks wrong. The above example goes right back to conceptually putting the clock tree into ACPI. I would expect the ACPI way to expose current clock rate to an individual device driver is to expose a clock rate method that internally knows how to return the currently set rate. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt new file mode 100644 index 0000000..12cd550 --- /dev/null +++ b/Documentation/arm64/arm-acpi.txt @@ -0,0 +1,240 @@ +ACPI on ARMv8 Servers +--------------------- + +ACPI will be used for ARMv8 general purpose servers designed to follow +the SBSA specification (currently available to people with an ARM login at +http://silver.arm.com) + +The implemented ACPI version is 5.1 + errata as released by the UEFI Forum, +which is available at <http://www.uefi.org/acpi/specs>. + +If the machine does not meet these requirements then it is likely that Device +Tree (DT) is more suitable for the hardware. + +Relationship with Device Tree +----------------------------- + +ACPI support in drivers and subsystems for ARMv8 should never be mutually +exclusive with DT support at compile time. + +At boot time the kernel will only use one description method depending on +parameters passed from the bootloader. + +Regardless of whether DT or ACPI is used, the kernel must always be capable +of booting with either scheme. + +When booting using ACPI tables the /chosen node in DT will still be parsed +to extract the kernel command line and initrd path. No other section of +the DT will be used. + +Booting using ACPI tables +------------------------- + +Currently, the only defined method to pass ACPI tables to the kernel on ARMv8 +is via the UEFI system configuration table. + +The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the +RSDP table (the table with the ACPI signature "RSD PTR "). + +The pointer to the RSDP table will be retrieved from EFI by the ACPI core. + +Processing of ACPI tables may be disabled by passing acpi=off on the kernel +command line. + +DO use an XSDT, RSDTs are deprecated and should not be used on arm64. They +only allow for 32bit addresses. + +DO NOT use the 32-bit address fields in the FADT, they are deprecated, the +64-bit alternatives MUST be used. + +The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT +and GTDT. If PCI is used the MCFG table MUST also be present. + +ACPI Detection +-------------- + +Drivers should determine their probe() type by checking for ACPI_HANDLE, +or .of_node, or other information in the device structure. This is +detailed further in the "Driver Recomendations" section. + +If the presence of ACPI needs to be detected at runtime, then check the value +of acpi_disabled. If CONFIG_ACPI not being set acpi_disabled will always be 1. + +Device Enumeration +------------------ + +Device descriptions in ACPI should use standard recognised ACPI interfaces. +These are far simpler than the information provided via Device Tree. Drivers +should take into account this simplicity and work with sensible defaults. + +On no account should a Device Tree attempt to be replicated in ASL using such +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific +data should be passed in the appropriate _DSM (ACPI Section 9.14.1) method or +_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific. + +Common _DSD bindings should be submitted to ASWG to be included in the +document :- + +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm + +TODO: Clarification and examples from Juno implementation. + +Programmable Power Control Resources +------------------------------------ + +Programmable power control resources include such resources as voltage/current +providers (regulators) and clock sources. + +For power control of these resources they should be represented with Power +Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly +enabling/disabling of resources as they are needed. + +There exists in the ACPI 5.1 specification no standard binding for these objects +to enable programmable levels or rates so this should be avoid if possible and +the resources set to appropriate level by the firmware. If this is not possible +then any manipulation should be abstracted in ASL. + +Each device in ACPI has D-states and these can be controlled through +the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off. + +If either _PS0 or _PS3 is implemented, then the other method must also be +implemented. + +If a device requires usage or setup of a power resource when on, the ASL +should organise that it is allocated/enabled using the _PS0 method. + +Resources allocated/enabled in the _PS0 method should be disabled/de-allocated +in the _PS3 method. + +Such code in _PS? methods will of course be very platform specific but +should allow the driver to operate the device without special non standard +values being read from ASL. Further, abstracting the use of these resources +allows hardware revisions without requiring updates to the kernel. + +TODO: Clarification and examples from Juno implementation. + +Clocks +------ + +Like clocks that are part of the power resources there is no standard way +to represent a clock tree in ACPI 5.1 in a similar manner to how it is +described in DT. + +Devices affected by this include things like UARTs, SoC driven LCD displays, +etc. + +The firmware for example UEFI should initialise these clocks to fixed working +values before the kernel is executed. If a driver requires to know rates of +clocks set by firmware then they can be passed to kernel using _DSD. + +example :- + +Device (CLK0) { + ... + + Name (_DSD, Package() { + ToUUID("XXXXX"), + Package() { + Package(2) {"#clock-cells", 0}, + Package(2) {"clock-frequency", "10000"} + } + }) + + ... +} + +Device (USR1) { + ... + + Name (_DSD, Package() { + ToUUID("XXXXX"), + Package() { + Package(2) {"clocks", Package() {1, ^CLK0}}}, + } + }) + + ... +} + +Driver Recommendations +---------------------- + +DO NOT remove any FDT handling when adding ACPI support for a driver, different +systems may use the same device. + +DO try and keep complex sections of ACPI and DT functionality seperate. This +may mean a patch to break out some complex DT to another function before +the patch to add ACPI. This may happen in other functions but is most likely +in probe function. This gives a clearer flow of data for reviewing driver +source. + +probe() :- + +TODO: replace this with a specific real example from Juno? + +static int device_probe_dt(struct platform_device *pdev) +{ + /* DT specific functionality */ + ... +} + +static int device_probe_acpi(struct platform_device *pdev) +{ + /* ACPI specific functionality */ + ... +} + +static int device_probe(stuct platform_device *pdev) +{ + ... + acpi_handle handle = ACPI_HANDLE(&pdev->dev); + struct device_node node = pdev->dev.of_node; + ... + + if (node) + ret = device_probe_dt(pdev); + else if (handle) + ret = device_probe_acpi(pdev); + else + /* other initialisation */ + ... + /* Continue with any generic probe operations */ + ... +} + +DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear +the different names the driver is probed for, both from DT and from ACPI. + +module device tables :- + +static struct of_device_id virtio_mmio_match[] = { + { .compatible = "virtio,mmio", }, + {}, +}; +MODULE_DEVICE_TABLE(of, virtio_mmio_match); + +static const struct acpi_device_id virtio_mmio_acpi_match[] = { + { "LNRO0005", }, + { } +}; +MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match); + +TODO: Add any other helpful rules that develop from Juno ACPI work. + +ASWG +---- + +The following areas are not yet well defined for ARM in the current ACPI +specification and are expected to be worked through in the UEFI ACPI +Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>. +Participation in this group is open to all UEFI members. + + - ACPI based CPU topology + - ACPI based Power management + - CPU idle control based on PSCI + - CPU performance control (CPPC) + +No code shall be accepted into the kernel unless it complies with the released +standards from UEFI ASWG. If there are features missing from ACPI to make it +function on a platform ECRs should be submitted to ASWG and go through the +approval process.