[19/19] Documentation: ACPI for ARM64

Commit Message

Hanjun Guo July 24, 2014, 1 p.m. UTC

From: Graeme Gregory <[e-mail]>

Add documentation for the guidelines of how to use ACPI
on ARM64.

Signed-off-by: Graeme Gregory <[e-mail]>
Signed-off-by: Hanjun Guo <[e-mail]>
---
 Documentation/arm64/arm-acpi.txt |  240 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 240 insertions(+)
 create mode 100644 Documentation/arm64/arm-acpi.txt

Comments

Randy Dunlap July 24, 2014, 8:42 p.m. UTC | #1

On 07/24/2014 06:00 AM, Hanjun Guo wrote:
> From: Graeme Gregory <[e-mail]>
> 
> Add documentation for the guidelines of how to use ACPI
> on ARM64.
> 
> Signed-off-by: Graeme Gregory <[e-mail]>
> Signed-off-by: Hanjun Guo <[e-mail]>
> ---
>  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.
>

Randy Dunlap July 24, 2014, 9:19 p.m. UTC | #2

On 07/24/2014 02:16 PM, Naresh Bhat wrote:
> 
> On 24 July 2014 18:30, Hanjun Guo <[e-mail] <mailto:[e-mail]>> wrote:
> 
>     From: Graeme Gregory <[e-mail] <mailto:[e-mail]>>
> 
>     Add documentation for the guidelines of how to use ACPI
>     on ARM64.
> 
>     Signed-off-by: Graeme Gregory <[e-mail] <mailto:[e-mail]>>
>     Signed-off-by: Hanjun Guo <[e-mail] <mailto:[e-mail]>>
>     ---
>      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
> 
>

Hanjun Guo July 25, 2014, 10:55 a.m. UTC | #3

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 <[e-mail]>
>>
>> Add documentation for the guidelines of how to use ACPI
>> on ARM64.
>>
>> Signed-off-by: Graeme Gregory <[e-mail]>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> ---
>>  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.
>>
> 
>

Olof Johansson July 27, 2014, 2:34 a.m. UTC | #4

On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <[e-mail]> wrote:
> From: Graeme Gregory <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Hanjun Guo <[e-mail]>
> ---
>  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

Graeme Gregory July 28, 2014, 8:42 a.m. UTC | #5

On 27/07/2014 03:34, Olof Johansson wrote:
> On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <[e-mail]> wrote:
>> From: Graeme Gregory <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> ---
>>   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

Arnd Bergmann July 28, 2014, 9:07 a.m. UTC | #6

On Saturday 26 July 2014 19:34:48 Olof Johansson wrote:
> On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <[e-mail]> 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.

It's possible that I'm misremembering it though, and it should be
documented better.

> > +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.

Right.

> > +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},
> 
> 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?

Exactly. I think what is going on here is a conflict of interests between
Intel's embedded ACPI uses and the ARM64 server requirements. The above
closely resembles what we do in DT, and that makes perfect sense for
Intel's machines so they can reuse a lot of the infrastructure we put
in place for DT. I also suspect it will take a few more years before
this actually gets accepted into both an ACPI specification and the
common operating systems (no point doing it if only Linux is going to
adopt it).

For the servers, I don't see how it makes any sense at all, independent
of the architecture, and relying on a feature like this would only serve
to delay the adoption of ACPI (whether that is a good or bad thing
may be a matter of perspective).

Maybe Graeme or others can comment on where this is coming from. What kind
of driver would actually need to find out the clock rate of a device on
an arm64 server? The examples above list "UARTs, SoC driven LCD displays,
etc.". For all I know, the UART is required to be PL01x (without DMA)
compatible, which should be fully described in ACPI, and I don't see why
a server would come with an LCD.

	Arnd

Graeme Gregory July 28, 2014, 9:23 a.m. UTC | #7

On 28/07/2014 10:07, 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 <[e-mail]> 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.
>
> It's possible that I'm misremembering it though, and it should be
> documented better.
>
>>> +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.
> Right.
>
>>> +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},
>> 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?
> Exactly. I think what is going on here is a conflict of interests between
> Intel's embedded ACPI uses and the ARM64 server requirements. The above
> closely resembles what we do in DT, and that makes perfect sense for
> Intel's machines so they can reuse a lot of the infrastructure we put
> in place for DT. I also suspect it will take a few more years before
> this actually gets accepted into both an ACPI specification and the
> common operating systems (no point doing it if only Linux is going to
> adopt it).
>
> For the servers, I don't see how it makes any sense at all, independent
> of the architecture, and relying on a feature like this would only serve
> to delay the adoption of ACPI (whether that is a good or bad thing
> may be a matter of perspective).
>
> Maybe Graeme or others can comment on where this is coming from. What kind
> of driver would actually need to find out the clock rate of a device on
> an arm64 server? The examples above list "UARTs, SoC driven LCD displays,
> etc.". For all I know, the UART is required to be PL01x (without DMA)
> compatible, which should be fully described in ACPI, and I don't see why
> a server would come with an LCD.
>
>
The PL011 UART is the use-case I keep hitting, that IP block has a 
variable input clock on pretty much everything I have seen in the wild. 
I really hope that this use does not spread beyond a few essential 
devices like the UART. IMO all real hardware should be the other side of 
a PCIe bridge.

Graeme

Mark Rutland July 28, 2014, 10:06 a.m. UTC | #8

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 <[e-mail]> wrote:
> > From: Graeme Gregory <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Hanjun Guo <[e-mail]>
> > ---
> >  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.

Mark Rutland July 28, 2014, 10:12 a.m. UTC | #9

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 <[e-mail]> 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.

Arnd Bergmann July 28, 2014, 10:46 a.m. UTC | #10

On Monday 28 July 2014 10:23:57 Graeme Gregory wrote:
> The PL011 UART is the use-case I keep hitting, that IP block has a 
> variable input clock on pretty much everything I have seen in the wild. 

Ok, I see. What does ACPI-5.1 say about pl011?

Interestingly, the subset of pl011 that is specified by SBSA does not
contain the IBRD/FBRD registers, effectively making it a fixed-rated
UART (I guess that would be a ART, without the U then), and you
consequently don't even need to know the clock rate.

However, my guess is that most hardware in the real world contains
an actual pl011 and it does make a lot of sense to allow setting
the baud rate on it, which then requires knowing the input clock.

If there is any hardware that implements just the SBSA-mandated subset
rather than the full pl011, we should probably implement both
in the kernel: a dumb driver that can only send and receive, and the
more complex one that can set the bit rates and flow-control but that
requires a standardized ACPI table with the input clock rate.

Whether the two would belong into one file or two separate driver
modules is something I can't tell, it would be up to the serial
maintainers to decide.

> I really hope that this use does not spread beyond a few essential 
> devices like the UART. IMO all real hardware should be the other side of 
> a PCIe bridge.

I would definitely agree with that.

	Arnd

Andre Przywara July 28, 2014, 2:20 p.m. UTC | #11

On 28/07/14 11:46, Arnd Bergmann wrote:
> On Monday 28 July 2014 10:23:57 Graeme Gregory wrote:
>> The PL011 UART is the use-case I keep hitting, that IP block has a 
>> variable input clock on pretty much everything I have seen in the wild. 
> 
> Ok, I see. What does ACPI-5.1 say about pl011?
> 
> Interestingly, the subset of pl011 that is specified by SBSA does not
> contain the IBRD/FBRD registers, effectively making it a fixed-rated
> UART (I guess that would be a ART, without the U then), and you
> consequently don't even need to know the clock rate.

The idea of this was probably to let the baudrate set by some firmware
code to the "right" value and the spec just didn't want to expose the
details for the generic UART:
"This specification does not cover registers needed to configure the
UART as these are considered hardware-specific and will be set up by
hardware-specific software."
To me that reads like the SBSA UART is just for debugging, and you are
expected just to access the data register.

> However, my guess is that most hardware in the real world contains
> an actual pl011 and it does make a lot of sense to allow setting
> the baud rate on it, which then requires knowing the input clock.
> 
> If there is any hardware that implements just the SBSA-mandated subset
> rather than the full pl011, we should probably implement both
> in the kernel: a dumb driver that can only send and receive, and the
> more complex one that can set the bit rates and flow-control but that
> requires a standardized ACPI table with the input clock rate.

The fast model I use can be switched to use the SBSA restricted PL011,
and as expected the Linux kernel crashes at the device doesn't support
DMA (and a lot more stuff) - but the current code requires it.
So I am about to implement a new driver for that SBSA subset. So far
this will be a separate driver, starting from a copy of amba-pl011.c,
but removing most of the code ;-)

> Whether the two would belong into one file or two separate driver
> modules is something I can't tell, it would be up to the serial
> maintainers to decide.

Sharing support for both devices in the same file doesn't seem to make
too much sense to me so far (but I may amend this later).
Will post a version as soon as I have it finished.

Cheers,
Andre.

Arnd Bergmann July 28, 2014, 3:23 p.m. UTC | #12

On Monday 28 July 2014 15:20:06 Andre Przywara wrote:
> On 28/07/14 11:46, Arnd Bergmann wrote:
> > On Monday 28 July 2014 10:23:57 Graeme Gregory wrote:
> >> The PL011 UART is the use-case I keep hitting, that IP block has a 
> >> variable input clock on pretty much everything I have seen in the wild. 
> > 
> > Ok, I see. What does ACPI-5.1 say about pl011?
> > 
> > Interestingly, the subset of pl011 that is specified by SBSA does not
> > contain the IBRD/FBRD registers, effectively making it a fixed-rated
> > UART (I guess that would be a ART, without the U then), and you
> > consequently don't even need to know the clock rate.
> 
> The idea of this was probably to let the baudrate set by some firmware
> code to the "right" value and the spec just didn't want to expose the
> details for the generic UART:
> "This specification does not cover registers needed to configure the
> UART as these are considered hardware-specific and will be set up by
> hardware-specific software."
> To me that reads like the SBSA UART is just for debugging, and you are
> expected just to access the data register.

Right, makes sense. It also avoids the case where Linux for some reason
ends up using a different line rate than the firmware, which can
cause a lot of unnecessary pain.

> > However, my guess is that most hardware in the real world contains
> > an actual pl011 and it does make a lot of sense to allow setting
> > the baud rate on it, which then requires knowing the input clock.
> > 
> > If there is any hardware that implements just the SBSA-mandated subset
> > rather than the full pl011, we should probably implement both
> > in the kernel: a dumb driver that can only send and receive, and the
> > more complex one that can set the bit rates and flow-control but that
> > requires a standardized ACPI table with the input clock rate.
> 
> The fast model I use can be switched to use the SBSA restricted PL011,
> and as expected the Linux kernel crashes at the device doesn't support
> DMA (and a lot more stuff) - but the current code requires it.

It does? We have a lot of platforms that don't have DMA support for
pl011.

> So I am about to implement a new driver for that SBSA subset. So far
> this will be a separate driver, starting from a copy of amba-pl011.c,
> but removing most of the code ;-)

Ok. You might want to consider starting from a different base though.
IIRC, pl011 uses uart_port as the basic abstraction, while the
new driver should probably use the raw tty_port instead.
drivers/tty/goldfish.c is probably a good example to look at for
that.

You could also make it a hvc_driver like drivers/tty/hvc/hvc_vio.c,
but I'm not sure if that model seen favorable by the tty maintainers.
It would probably be the shortest driver though.

	Arnd

Andre Przywara July 28, 2014, 4:14 p.m. UTC | #13

On 28/07/14 16:23, Arnd Bergmann wrote:
> On Monday 28 July 2014 15:20:06 Andre Przywara wrote:
>> On 28/07/14 11:46, Arnd Bergmann wrote:
>>> On Monday 28 July 2014 10:23:57 Graeme Gregory wrote:
>>>> The PL011 UART is the use-case I keep hitting, that IP block has a 
>>>> variable input clock on pretty much everything I have seen in the wild. 
>>>
>>> Ok, I see. What does ACPI-5.1 say about pl011?
>>>
>>> Interestingly, the subset of pl011 that is specified by SBSA does not
>>> contain the IBRD/FBRD registers, effectively making it a fixed-rated
>>> UART (I guess that would be a ART, without the U then), and you
>>> consequently don't even need to know the clock rate.
>>
>> The idea of this was probably to let the baudrate set by some firmware
>> code to the "right" value and the spec just didn't want to expose the
>> details for the generic UART:
>> "This specification does not cover registers needed to configure the
>> UART as these are considered hardware-specific and will be set up by
>> hardware-specific software."
>> To me that reads like the SBSA UART is just for debugging, and you are
>> expected just to access the data register.
> 
> Right, makes sense. It also avoids the case where Linux for some reason
> ends up using a different line rate than the firmware, which can
> cause a lot of unnecessary pain.
> 
>>> However, my guess is that most hardware in the real world contains
>>> an actual pl011 and it does make a lot of sense to allow setting
>>> the baud rate on it, which then requires knowing the input clock.
>>>
>>> If there is any hardware that implements just the SBSA-mandated subset
>>> rather than the full pl011, we should probably implement both
>>> in the kernel: a dumb driver that can only send and receive, and the
>>> more complex one that can set the bit rates and flow-control but that
>>> requires a standardized ACPI table with the input clock rate.
>>
>> The fast model I use can be switched to use the SBSA restricted PL011,
>> and as expected the Linux kernel crashes at the device doesn't support
>> DMA (and a lot more stuff) - but the current code requires it.
> 
> It does? We have a lot of platforms that don't have DMA support for
> pl011.

Well, to be honest I just booted the full featured kernel with the
changed fast model config, so the platform and the DT claimed DMA
support, just the emulated hardware doesn't implement it ;-)
And beside that a whole lot of other PL011 registers are not available,
so the crash could be caused by anything. I didn't look to closely why
it broke.

>> So I am about to implement a new driver for that SBSA subset. So far
>> this will be a separate driver, starting from a copy of amba-pl011.c,
>> but removing most of the code ;-)
> 
> Ok. You might want to consider starting from a different base though.
> IIRC, pl011 uses uart_port as the basic abstraction, while the
> new driver should probably use the raw tty_port instead.
> drivers/tty/goldfish.c is probably a good example to look at for
> that.

Good hint, will look at this.

Cheers,
Andre.

> 
> You could also make it a hvc_driver like drivers/tty/hvc/hvc_vio.c,
> but I'm not sure if that model seen favorable by the tty maintainers.
> It would probably be the shortest driver though.
> 
> 	Arnd
> 
>

Olof Johansson July 28, 2014, 4:23 p.m. UTC | #14

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 <[e-mail]> wrote:
> >>From: Graeme Gregory <[e-mail]>
> >>
> >>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 <[e-mail]>
> >>Signed-off-by: Hanjun Guo <[e-mail]>
> >>---
> >>  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

Olof Johansson July 28, 2014, 4:27 p.m. UTC | #15

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 <[e-mail]> 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.

> 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? 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.

> 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.


-Olof

Olof Johansson July 28, 2014, 4:33 p.m. UTC | #16

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 <[e-mail]> 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

Olof Johansson July 28, 2014, 4:44 p.m. UTC | #17

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 <[e-mail]> wrote:
> > > From: Graeme Gregory <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Signed-off-by: Hanjun Guo <[e-mail]>
> > > ---
> > >  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

Mark Rutland July 28, 2014, 5 p.m. UTC | #18

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 <[e-mail]> 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.

Mark Rutland July 28, 2014, 5:36 p.m. UTC | #19

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 <[e-mail]> wrote:
> > > > From: Graeme Gregory <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Hanjun Guo <[e-mail]>
> > > > ---
> > > >  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
>

Mark Brown July 28, 2014, 5:44 p.m. UTC | #20

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.

Olof Johansson July 28, 2014, 6:27 p.m. UTC | #21

On Mon, Jul 28, 2014 at 10:00 AM, Mark Rutland <[e-mail]> 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 <[e-mail]> 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

Olof Johansson July 28, 2014, 6:34 p.m. UTC | #22

On Mon, Jul 28, 2014 at 10:36 AM, Mark Rutland <[e-mail]> 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 <[e-mail]> wrote:
>> > > > From: Graeme Gregory <[e-mail]>
>> > > >
>> > > > 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 <[e-mail]>
>> > > > Signed-off-by: Hanjun Guo <[e-mail]>
>> > > > ---
>> > > >  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
>>

Mark Rutland July 28, 2014, 6:37 p.m. UTC | #23

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 <[e-mail]> 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.

Olof Johansson July 28, 2014, 6:44 p.m. UTC | #24

On Mon, Jul 28, 2014 at 11:37 AM, Mark Rutland <[e-mail]> 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 <[e-mail]> 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

Hanjun Guo July 29, 2014, 7:58 a.m. UTC | #25

On 2014-7-27 10:34, Olof Johansson wrote:
> On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <[e-mail]> wrote:
>> From: Graeme Gregory <[e-mail]>
>>
>> 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

Hanjun Guo July 29, 2014, 9:01 a.m. UTC | #26

On 2014-7-28 17:07, 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 <[e-mail]> 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.
> 
> It's possible that I'm misremembering it though, and it should be
> documented better.
> 
>>> +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.
> 
> Right.

Actually, if platforms don't implement ACPI, acpi_disabled will always be set to
1 at early boot stage which before the device tree is unflattened, so device
tree will work as expected even if CONFIG_ACPI=y on such platforms.

Thanks
Hanjun

Graeme Gregory July 29, 2014, 9:17 a.m. UTC | #27

On 28/07/2014 17:14, Andre Przywara wrote:
>
> On 28/07/14 16:23, Arnd Bergmann wrote:
>> On Monday 28 July 2014 15:20:06 Andre Przywara wrote:
>>> On 28/07/14 11:46, Arnd Bergmann wrote:
>>>> On Monday 28 July 2014 10:23:57 Graeme Gregory wrote:
>>>>> The PL011 UART is the use-case I keep hitting, that IP block has a
>>>>> variable input clock on pretty much everything I have seen in the wild.
>>>> Ok, I see. What does ACPI-5.1 say about pl011?
>>>>
>>>> Interestingly, the subset of pl011 that is specified by SBSA does not
>>>> contain the IBRD/FBRD registers, effectively making it a fixed-rated
>>>> UART (I guess that would be a ART, without the U then), and you
>>>> consequently don't even need to know the clock rate.
>>> The idea of this was probably to let the baudrate set by some firmware
>>> code to the "right" value and the spec just didn't want to expose the
>>> details for the generic UART:
>>> "This specification does not cover registers needed to configure the
>>> UART as these are considered hardware-specific and will be set up by
>>> hardware-specific software."
>>> To me that reads like the SBSA UART is just for debugging, and you are
>>> expected just to access the data register.
>> Right, makes sense. It also avoids the case where Linux for some reason
>> ends up using a different line rate than the firmware, which can
>> cause a lot of unnecessary pain.
I must be suffering snow blindness reading specs, I totally missed that 
the pl011 subset does not allow baud setting. This means that my current 
test hardware "Juno" does not actually need any clocks defined in DSDT 
at this stage (given that this new driver is created).

I may then return to my original opinion of not defining clocks in the 
DSDT at all.

Graeme

>>>> However, my guess is that most hardware in the real world contains
>>>> an actual pl011 and it does make a lot of sense to allow setting
>>>> the baud rate on it, which then requires knowing the input clock.
>>>>
>>>> If there is any hardware that implements just the SBSA-mandated subset
>>>> rather than the full pl011, we should probably implement both
>>>> in the kernel: a dumb driver that can only send and receive, and the
>>>> more complex one that can set the bit rates and flow-control but that
>>>> requires a standardized ACPI table with the input clock rate.
>>> The fast model I use can be switched to use the SBSA restricted PL011,
>>> and as expected the Linux kernel crashes at the device doesn't support
>>> DMA (and a lot more stuff) - but the current code requires it.
>> It does? We have a lot of platforms that don't have DMA support for
>> pl011.
> Well, to be honest I just booted the full featured kernel with the
> changed fast model config, so the platform and the DT claimed DMA
> support, just the emulated hardware doesn't implement it ;-)
> And beside that a whole lot of other PL011 registers are not available,
> so the crash could be caused by anything. I didn't look to closely why
> it broke.
>
>>> So I am about to implement a new driver for that SBSA subset. So far
>>> this will be a separate driver, starting from a copy of amba-pl011.c,
>>> but removing most of the code ;-)
>> Ok. You might want to consider starting from a different base though.
>> IIRC, pl011 uses uart_port as the basic abstraction, while the
>> new driver should probably use the raw tty_port instead.
>> drivers/tty/goldfish.c is probably a good example to look at for
>> that.
> Good hint, will look at this.
>
> Cheers,
> Andre.
>
>> You could also make it a hvc_driver like drivers/tty/hvc/hvc_vio.c,
>> but I'm not sure if that model seen favorable by the tty maintainers.
>> It would probably be the shortest driver though.
>>
>> 	Arnd
>>
>>

Arnd Bergmann July 29, 2014, 10:07 a.m. UTC | #28

On Tuesday 29 July 2014 10:17:22 Graeme Gregory wrote:
> On 28/07/2014 17:14, Andre Przywara wrote:
> >
> > On 28/07/14 16:23, Arnd Bergmann wrote:
> >> On Monday 28 July 2014 15:20:06 Andre Przywara wrote:
> >>> On 28/07/14 11:46, Arnd Bergmann wrote:
> >>>> On Monday 28 July 2014 10:23:57 Graeme Gregory wrote:
> >>>>> The PL011 UART is the use-case I keep hitting, that IP block has a
> >>>>> variable input clock on pretty much everything I have seen in the wild.
> >>>> Ok, I see. What does ACPI-5.1 say about pl011?
> >>>>
> >>>> Interestingly, the subset of pl011 that is specified by SBSA does not
> >>>> contain the IBRD/FBRD registers, effectively making it a fixed-rated
> >>>> UART (I guess that would be a ART, without the U then), and you
> >>>> consequently don't even need to know the clock rate.
> >>> The idea of this was probably to let the baudrate set by some firmware
> >>> code to the "right" value and the spec just didn't want to expose the
> >>> details for the generic UART:
> >>> "This specification does not cover registers needed to configure the
> >>> UART as these are considered hardware-specific and will be set up by
> >>> hardware-specific software."
> >>> To me that reads like the SBSA UART is just for debugging, and you are
> >>> expected just to access the data register.
> >> Right, makes sense. It also avoids the case where Linux for some reason
> >> ends up using a different line rate than the firmware, which can
> >> cause a lot of unnecessary pain.
> I must be suffering snow blindness reading specs, I totally missed that 
> the pl011 subset does not allow baud setting. This means that my current 
> test hardware "Juno" does not actually need any clocks defined in DSDT 
> at this stage (given that this new driver is created).
> 
> I may then return to my original opinion of not defining clocks in the 
> DSDT at all.

Ok, that certainly simplifies it a lot, thanks!

	Arnd

Christoffer Dall July 29, 2014, 10:07 a.m. UTC | #29

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 <[e-mail] <mailto:[e-mail]>> wrote:
> > 
> >     From: Graeme Gregory <[e-mail] <mailto:[e-mail]>>
> > 
> >     Add documentation for the guidelines of how to use ACPI
> >     on ARM64.
> > 
> >     Signed-off-by: Graeme Gregory <[e-mail] <mailto:[e-mail]>>
> >     Signed-off-by: Hanjun Guo <[e-mail] <mailto:[e-mail]>>
> >     ---
> >      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

Christoffer Dall July 29, 2014, 10:29 a.m. UTC | #30

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

Christoffer Dall July 29, 2014, 10:30 a.m. UTC | #31

On Thu, Jul 24, 2014 at 09:00:25PM +0800, Hanjun Guo wrote:
> From: Graeme Gregory <[e-mail]>
> 
> Add documentation for the guidelines of how to use ACPI
> on ARM64.
> 
> Signed-off-by: Graeme Gregory <[e-mail]>
> Signed-off-by: Hanjun Guo <[e-mail]>
> ---
>  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

Arnd Bergmann July 29, 2014, 10:41 a.m. UTC | #32

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

Mark Rutland July 29, 2014, 10:55 a.m. UTC | #33

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.

Mark Rutland July 29, 2014, 11:28 a.m. UTC | #34

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

Christoffer Dall July 29, 2014, 12:37 p.m. UTC | #35

On 29 July 2014 13:28, Mark Rutland <[e-mail]> 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

Arnd Bergmann July 29, 2014, 12:52 p.m. UTC | #36

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

Mark Rutland July 29, 2014, 1:08 p.m. UTC | #37

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.

Christoffer Dall July 29, 2014, 1:31 p.m. UTC | #38

On 29 July 2014 15:08, Mark Rutland <[e-mail]> 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

Christoffer Dall July 29, 2014, 1:33 p.m. UTC | #39

On 29 July 2014 14:52, Arnd Bergmann <[e-mail]> 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

Mark Rutland July 29, 2014, 2:04 p.m. UTC | #40

On Tue, Jul 29, 2014 at 02:31:16PM +0100, Christoffer Dall wrote:
> On 29 July 2014 15:08, Mark Rutland <[e-mail]> 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.

Arnd Bergmann July 29, 2014, 2:41 p.m. UTC | #41

On Tuesday 29 July 2014 15:31:16 Christoffer Dall wrote:
> On 29 July 2014 15:08, Mark Rutland <[e-mail]> 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.

	Arnd

Christoffer Dall July 29, 2014, 3:01 p.m. UTC | #42

On 29 July 2014 16:41, Arnd Bergmann <[e-mail]> wrote:
> On Tuesday 29 July 2014 15:31:16 Christoffer Dall wrote:
>> On 29 July 2014 15:08, Mark Rutland <[e-mail]> 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

Hanjun Guo July 30, 2014, 6:47 a.m. UTC | #43

On 2014-7-29 21:31, Christoffer Dall wrote:
> On 29 July 2014 15:08, Mark Rutland <[e-mail]> 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

Christoffer Dall July 30, 2014, 7:14 a.m. UTC | #44

On 30 July 2014 08:47, Hanjun Guo <[e-mail]> wrote:
> On 2014-7-29 21:31, Christoffer Dall wrote:
>> On 29 July 2014 15:08, Mark Rutland <[e-mail]> 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

Hanjun Guo July 30, 2014, 9:36 a.m. UTC | #45

On 2014-7-30 15:14, Christoffer Dall wrote:
> On 30 July 2014 08:47, Hanjun Guo <[e-mail]> wrote:
>> On 2014-7-29 21:31, Christoffer Dall wrote:
>>> On 29 July 2014 15:08, Mark Rutland <[e-mail]> 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

Catalin Marinas Aug. 12, 2014, 6:23 p.m. UTC | #46

On Mon, Jul 28, 2014 at 07:27:52PM +0100, Olof Johansson wrote:
> On Mon, Jul 28, 2014 at 10:00 AM, Mark Rutland <[e-mail]> 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 <[e-mail]> 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.

Rafael J. Wysocki Aug. 13, 2014, 11:41 p.m. UTC | #47

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 <[e-mail]> 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 <[e-mail]> 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.

Hanjun Guo Aug. 14, 2014, 3:21 a.m. UTC | #48

On 2014-8-14 7:41, Rafael J. Wysocki wrote:
> 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 <[e-mail]> 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 <[e-mail]> 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.

Real users will coming soon, we already tested this patch set on real hardware
(ARM64 Juno platform), and I think ARM64 server chips and platforms will show
up before 3.18 is released.

For this patch set, DT is the first class citizen at now:

a) We can always set CONFIG_ACPI as off in Kconfig, and use DT only;

b) Even if we set CONFIG_ACPI=Y, we also can use DT as normal:

  - Pass DT blob without (valid) ACPI tables (just as we boot the kernel now),
    ACPI will disabled in the very early stage and FDT will still to be
    unflattened, so will not break DT booting.

  - We can pass ACPI=off to disable ACPI and use DT even if we got valid
    ACPI tables (in the v1 patch set);

So it is safe for people who want to use DT, and didn't change any behavior
of DT booting except some extra test of if(acpi_disabled).

Thanks
Hanjun

Catalin Marinas Aug. 14, 2014, 10:27 a.m. UTC | #49

On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
> On 2014-8-14 7:41, Rafael J. Wysocki wrote:
> > On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
> >> 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.
> 
> Real users will coming soon, we already tested this patch set on real hardware
> (ARM64 Juno platform),

I don't consider Juno a server platform ;) (but it's good enough for
development).

> and I think ARM64 server chips and platforms will show up before 3.18
> is released.

That's what I've heard/seen. The questions I have are (a) whether
current ACPI patchset is enough to successfully run Linux on such
_hardware_ platform (maybe not fully optimised, for example just WFI
cpuidle) and (b) whether we still want to mandate a DT in the kernel for
such platforms.

Given the answer to (a) and what other features are needed, we may or
may not mandate (b). We were pretty clear few months ago that (b) is
still required but at the time we were only openly talking about ACPI
5.0 which was lacking many features. I think we need to revisit that
position based on how usable ACPI 5.1 for ARM (and current kernel
implementation) is. Would you mind elaborating what an ACPI-only
platform miss?

I would expect a new server platform designed with ACPI in mind to
require minimal SoC specific code, so we may only see a few patches
under drivers/ for such platforms adding ACPI-specific probing (possibly
new drivers as well if it's a new component).

> For this patch set, DT is the first class citizen at now:
> 
> a) We can always set CONFIG_ACPI as off in Kconfig, and use DT only;

Not just off but, based on maturity, depend on EXPERT.

> b) Even if we set CONFIG_ACPI=Y, we also can use DT as normal:
> 
>   - Pass DT blob without (valid) ACPI tables (just as we boot the kernel now),
>     ACPI will disabled in the very early stage and FDT will still to be
>     unflattened, so will not break DT booting.
> 
>   - We can pass ACPI=off to disable ACPI and use DT even if we got valid
>     ACPI tables (in the v1 patch set);
> 
> So it is safe for people who want to use DT, and didn't change any behavior
> of DT booting except some extra test of if(acpi_disabled).

But should we require SoC firmware to provide both valid DT and ACPI
tables so that the user can decide which one to select at boot-time?

Arnd Bergmann Aug. 14, 2014, 8:53 p.m. UTC | #50

On Thursday 14 August 2014 11:27:23 Catalin Marinas wrote:
> On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
> > On 2014-8-14 7:41, Rafael J. Wysocki wrote:
> > > On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
> > >> 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.
> > 
> > Real users will coming soon, we already tested this patch set on real hardware
> > (ARM64 Juno platform),
> 
> I don't consider Juno a server platform  (but it's good enough for
> development).

I would expect that Juno is a superset of what servers need. If this
ACPI patch set is sufficient to support every device present on Juno
with an ACPI firmware, what would be a strong indication that server
platforms work as well.

OTOH, if ACPI on Juno only supports half the features that the hardware
has, that doesn't tell us much about the suitability of ACPI for any
real-world system, server or not.

> > and I think ARM64 server chips and platforms will show up before 3.18
> > is released.
> 
> That's what I've heard/seen. The questions I have are (a) whether
> current ACPI patchset is enough to successfully run Linux on such
> _hardware_ platform (maybe not fully optimised, for example just WFI
> cpuidle) and (b) whether we still want to mandate a DT in the kernel for
> such platforms.
> 
> Given the answer to (a) and what other features are needed, we may or
> may not mandate (b). We were pretty clear few months ago that (b) is
> still required but at the time we were only openly talking about ACPI
> 5.0 which was lacking many features. I think we need to revisit that
> position based on how usable ACPI 5.1 for ARM (and current kernel
> implementation) is. Would you mind elaborating what an ACPI-only
> platform miss?
> 
> I would expect a new server platform designed with ACPI in mind to
> require minimal SoC specific code, so we may only see a few patches
> under drivers/ for such platforms adding ACPI-specific probing (possibly
> new drivers as well if it's a new component).

That is at least the hope, but I have no way of knowing whether it
works that way or not, other than seeing the actual patches.

It is rather annoying that we first have to wait for hardware
to become available to actually see that code, but I guess that
means that those hardware vendors no longer see ACPI as something
on their critical path, so we definitely shouldn't hurry things
until we understand the reason for the endless delay of platform
support patches.

	Arnd

Olof Johansson Aug. 15, 2014, 1:02 a.m. UTC | #51

On Thu, Aug 14, 2014 at 1:53 PM, Arnd Bergmann <[e-mail]> wrote:
> On Thursday 14 August 2014 11:27:23 Catalin Marinas wrote:
>> On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
>> > On 2014-8-14 7:41, Rafael J. Wysocki wrote:
>> > > On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
>> > >> 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.
>> >
>> > Real users will coming soon, we already tested this patch set on real hardware
>> > (ARM64 Juno platform),
>>
>> I don't consider Juno a server platform  (but it's good enough for
>> development).
>
> I would expect that Juno is a superset of what servers need. If this
> ACPI patch set is sufficient to support every device present on Juno
> with an ACPI firmware, what would be a strong indication that server
> platforms work as well.
>
> OTOH, if ACPI on Juno only supports half the features that the hardware
> has, that doesn't tell us much about the suitability of ACPI for any
> real-world system, server or not.

Juno is lacking in several components compared to a server platform,
it doesn't have PCIe, SATA, or real ethernet. So it's in many ways
just a few cores with RAM and a few slow interfaces.

The scary parts from the ACPI 5.1 docs (the peripheral ones in
particular) are around the modelling of clocks and other things that
we thought ACPI was going to stay away from. Until we see how steep
that slope is, we're better off taking it easy with merging the
support. It could get quite messy very quickly, and we'd be stuck
supporting whatever solutions are tried in the first ACPI generations
forever if we do enable them now. That's the main reason for holding
off and being conservative (and seeing several real platforms and how
they end up modelling these things).

As I've also commented on some of the documents, I am not excited
about how the ACPI platform code is integrated: Add some ACPI
functions in an acpi-only file that exports some variables. Then, in a
shared file, create a completely separate code path that uses said
variables to do whatever it needs. I'm not crazy at all at the split
of code paths between the platforms, and I want to see it more data
driven (filling in shared structures and sharing code paths). There
are some challenges to make that happen but I also don't think there's
been a whole lot of effort to make it happen -- the ACPI developers
don't seem to like touching the DT code paths to make them fit better.
:-)

>> > and I think ARM64 server chips and platforms will show up before 3.18
>> > is released.
>>
>> That's what I've heard/seen. The questions I have are (a) whether
>> current ACPI patchset is enough to successfully run Linux on such
>> _hardware_ platform (maybe not fully optimised, for example just WFI
>> cpuidle) and (b) whether we still want to mandate a DT in the kernel for
>> such platforms.
>>
>> Given the answer to (a) and what other features are needed, we may or
>> may not mandate (b). We were pretty clear few months ago that (b) is
>> still required but at the time we were only openly talking about ACPI
>> 5.0 which was lacking many features. I think we need to revisit that
>> position based on how usable ACPI 5.1 for ARM (and current kernel
>> implementation) is. Would you mind elaborating what an ACPI-only
>> platform miss?
>>
>> I would expect a new server platform designed with ACPI in mind to
>> require minimal SoC specific code, so we may only see a few patches
>> under drivers/ for such platforms adding ACPI-specific probing (possibly
>> new drivers as well if it's a new component).
>
> That is at least the hope, but I have no way of knowing whether it
> works that way or not, other than seeing the actual patches.
>
> It is rather annoying that we first have to wait for hardware
> to become available to actually see that code, but I guess that
> means that those hardware vendors no longer see ACPI as something
> on their critical path, so we definitely shouldn't hurry things
> until we understand the reason for the endless delay of platform
> support patches.

I've tried reaching out to several vendors but I've heard nothing back
from any of them. Very disappointing.

It is quite concerning that they are not willing to talk to the
community, but it also makes it easy to decide what kind of priority
we should put on reviewing and merging their patches when they do get
posted -- their lack of planning does not constitute an emergency on
our behalf.


-Olof

Hanjun Guo Aug. 15, 2014, 9:09 a.m. UTC | #52

On 2014-8-14 18:27, Catalin Marinas wrote:
> On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
>> On 2014-8-14 7:41, Rafael J. Wysocki wrote:
>>> On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
>>>> 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.
>>
>> Real users will coming soon, we already tested this patch set on real hardware
>> (ARM64 Juno platform),
> 
> I don't consider Juno a server platform ;) (but it's good enough for
> development).
> 
>> and I think ARM64 server chips and platforms will show up before 3.18
>> is released.
> 
> That's what I've heard/seen. The questions I have are (a) whether
> current ACPI patchset is enough to successfully run Linux on such
> _hardware_ platform (maybe not fully optimised, for example just WFI
> cpuidle) and (b) whether we still want to mandate a DT in the kernel for
> such platforms.

For (a), this patch set is only for ARM64 core, not including platform
specific device drivers, it will be covered by the binding of _DSD or
explicit definition of PNP ID/ACPI ID(s).

> 
> Given the answer to (a) and what other features are needed, we may or
> may not mandate (b). We were pretty clear few months ago that (b) is
> still required but at the time we were only openly talking about ACPI
> 5.0 which was lacking many features. I think we need to revisit that
> position based on how usable ACPI 5.1 for ARM (and current kernel
> implementation) is. Would you mind elaborating what an ACPI-only
> platform miss?

Do you mean something still missing? We still miss some features for
ARM in ACPI, but I think they are not critical, here is the list I can
remember:
- ITS for GICv3/4;
- SMMU support;
- CPU idle control.

For ACPI 5.1, it fixes many problems for ARM:
- weak definition for GIC, so we introduce visualization, v2m and
  part of GICv3/4 (redistributors) support.
- No support for PSCI. Fix it to support PSCI 0.2+;
- Not support for Always-on timer and SBSA-L1 watchdog.
- How to describe device properties, so _DSD is introduced for
  device probe.

> 
> I would expect a new server platform designed with ACPI in mind to
> require minimal SoC specific code, so we may only see a few patches
> under drivers/ for such platforms adding ACPI-specific probing (possibly
> new drivers as well if it's a new component).
> 
>> For this patch set, DT is the first class citizen at now:
>>
>> a) We can always set CONFIG_ACPI as off in Kconfig, and use DT only;
> 
> Not just off but, based on maturity, depend on EXPERT.

Ok. And don't set ACPI default off (pass acpi=on to enable it)?

> 
>> b) Even if we set CONFIG_ACPI=Y, we also can use DT as normal:
>>
>>   - Pass DT blob without (valid) ACPI tables (just as we boot the kernel now),
>>     ACPI will disabled in the very early stage and FDT will still to be
>>     unflattened, so will not break DT booting.
>>
>>   - We can pass ACPI=off to disable ACPI and use DT even if we got valid
>>     ACPI tables (in the v1 patch set);
>>
>> So it is safe for people who want to use DT, and didn't change any behavior
>> of DT booting except some extra test of if(acpi_disabled).
> 
> But should we require SoC firmware to provide both valid DT and ACPI
> tables so that the user can decide which one to select at boot-time?

No, I think only one of them should be provided on real platforms.

Thanks
Hanjun

Catalin Marinas Aug. 15, 2014, 10:01 a.m. UTC | #53

Hanjun,

On Fri, Aug 15, 2014 at 10:09:42AM +0100, Hanjun Guo wrote:
> On 2014-8-14 18:27, Catalin Marinas wrote:
> > On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
> >> On 2014-8-14 7:41, Rafael J. Wysocki wrote:
> >>> On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
> >>>> 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.
> >>
> >> Real users will coming soon, we already tested this patch set on real hardware
> >> (ARM64 Juno platform),
> > 
> > I don't consider Juno a server platform ;) (but it's good enough for
> > development).
> > 
> >> and I think ARM64 server chips and platforms will show up before 3.18
> >> is released.
> > 
> > That's what I've heard/seen. The questions I have are (a) whether
> > current ACPI patchset is enough to successfully run Linux on such
> > _hardware_ platform (maybe not fully optimised, for example just WFI
> > cpuidle) and (b) whether we still want to mandate a DT in the kernel for
> > such platforms.
> 
> For (a), this patch set is only for ARM64 core, not including platform
> specific device drivers, it will be covered by the binding of _DSD or
> explicit definition of PNP ID/ACPI ID(s).

So we go back to the discussions we had few months ago in Macau. I'm not
concerned about the core ARM and architected peripherals covered by ACPI
5.1 (as long as the current patches get positive technical review). But
I'm concerned about the additional bits needed for a real SoC like _DSD
definitions, how they get reviewed/accepted (or is it just the vendor's
problem?).

I think SBSA is too vague to guarantee a kernel image running on a
compliant platform without additional (vendor-specific) tweaks. So what
I asked for is (1) a document (guide) to define the strict set of ACPI
features and bindings needed for a real SoC and (2) proof that the
guidelines are enough for real hardware. I think we have (1) under
review with some good feedback so far. As for (2), we can probably only
discuss Juno openly. I think you could share the additional Juno patches
on this list so that reviewers can assess the suitability. If we deem
ACPI not (yet) suitable for Juno, is there other platform we could see
patches for?

> > Given the answer to (a) and what other features are needed, we may or
> > may not mandate (b). We were pretty clear few months ago that (b) is
> > still required but at the time we were only openly talking about ACPI
> > 5.0 which was lacking many features. I think we need to revisit that
> > position based on how usable ACPI 5.1 for ARM (and current kernel
> > implementation) is. Would you mind elaborating what an ACPI-only
> > platform miss?
> 
> Do you mean something still missing? We still miss some features for
> ARM in ACPI, but I think they are not critical, here is the list I can
> remember:
> - ITS for GICv3/4;
> - SMMU support;
> - CPU idle control.

I agree, these are not critical at this stage. But they only refer to
architected peripherals. Is there anything else missing for an SoC? Do
we need to define clocks?

> For ACPI 5.1, it fixes many problems for ARM:
> - weak definition for GIC, so we introduce visualization, v2m and
>   part of GICv3/4 (redistributors) support.
> - No support for PSCI. Fix it to support PSCI 0.2+;
> - Not support for Always-on timer and SBSA-L1 watchdog.

These are all good, that's why we shouldn't even talk about ACPI 5.0 in
the ARM context.

> - How to describe device properties, so _DSD is introduced for
>   device probe.

For the last bullet, is there any review process (at least like what we
have for DT bindings)? On top of such process, do we have guidelines and
example code on how the Linux support should be implemented. As Olof
mentioned, should we see how the DT and ACPI probing paths work
together? I really think we should be very clear here and not let
vendors invent their own independent methods.

> > I would expect a new server platform designed with ACPI in mind to
> > require minimal SoC specific code, so we may only see a few patches
> > under drivers/ for such platforms adding ACPI-specific probing (possibly
> > new drivers as well if it's a new component).
> > 
> >> For this patch set, DT is the first class citizen at now:
> >>
> >> a) We can always set CONFIG_ACPI as off in Kconfig, and use DT only;
> > 
> > Not just off but, based on maturity, depend on EXPERT.
> 
> Ok. And don't set ACPI default off (pass acpi=on to enable it)?

That's my view, just make it clear ACPI is experimental at the Kconfig
level because longer term we won't mandate SoCs to provide both DT and
ACPI tables.

> >> b) Even if we set CONFIG_ACPI=Y, we also can use DT as normal:
> >>
> >>   - Pass DT blob without (valid) ACPI tables (just as we boot the kernel now),
> >>     ACPI will disabled in the very early stage and FDT will still to be
> >>     unflattened, so will not break DT booting.
> >>
> >>   - We can pass ACPI=off to disable ACPI and use DT even if we got valid
> >>     ACPI tables (in the v1 patch set);
> >>
> >> So it is safe for people who want to use DT, and didn't change any behavior
> >> of DT booting except some extra test of if(acpi_disabled).
> > 
> > But should we require SoC firmware to provide both valid DT and ACPI
> > tables so that the user can decide which one to select at boot-time?
> 
> No, I think only one of them should be provided on real platforms.

That's what I thought. And such platforms would end up always passing
acpi=on anyway.

Arnd Bergmann Aug. 15, 2014, 7:49 p.m. UTC | #54

On Friday 15 August 2014, Olof Johansson wrote:
> On Thu, Aug 14, 2014 at 1:53 PM, Arnd Bergmann <[e-mail]> wrote:
> > On Thursday 14 August 2014 11:27:23 Catalin Marinas wrote:
> >
> > I would expect that Juno is a superset of what servers need. If this
> > ACPI patch set is sufficient to support every device present on Juno
> > with an ACPI firmware, what would be a strong indication that server
> > platforms work as well.
> >
> > OTOH, if ACPI on Juno only supports half the features that the hardware
> > has, that doesn't tell us much about the suitability of ACPI for any
> > real-world system, server or not.
> 
> Juno is lacking in several components compared to a server platform,
> it doesn't have PCIe, SATA, or real ethernet. So it's in many ways
> just a few cores with RAM and a few slow interfaces.

I see. I wouldn't really expect SATA in a server, but the lack of
PCIe of course also implies that there is no SAS/NVMe/FCoE storage
either.

Some of the slow interfaces may actually be more interesting,
since PCI and anything attached to it should (at least in theory)
be fully discoverable and not need much ACPI support at all.

The parts that I'm particularly interested in are the interaction
with the BMC and embedded controller, and how the power management
for nondiscoverable devices is implemented through AML.

> The scary parts from the ACPI 5.1 docs (the peripheral ones in
> particular) are around the modelling of clocks and other things that
> we thought ACPI was going to stay away from. Until we see how steep
> that slope is, we're better off taking it easy with merging the
> support. It could get quite messy very quickly, and we'd be stuck
> supporting whatever solutions are tried in the first ACPI generations
> forever if we do enable them now. That's the main reason for holding
> off and being conservative (and seeing several real platforms and how
> they end up modelling these things).

Agreed. I think we had already concluded previously when discussing this
patch that the clock management in APCI-5.1 should not be used on ARM64,
but I think there is a problem one level deeper: The 5.0 and 5.1 versions
apparently add a lot of new features that are meant for either ARM64
servers or embedded x86 machines. These two typically have conflicting
requirements, and it would be better for the specification itself to
provide clearer statements to which parts apply in what use case rather
than us (the Linux people) making claims about what parts of the spec
are acceptable or not.

There are already two specified classes of systems, the legacy x86
and itanium machines, and the "reduced hardware" profile, which
apparently covers both of the new types of machines mentioned above.

What would be the process to get a clarification into the next version
of ACPI that makes them more distinct?

	Arnd

Len Brown Aug. 15, 2014, 10:43 p.m. UTC | #55

> 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

Mark Brown Aug. 15, 2014, 11:19 p.m. UTC | #56

On Fri, Aug 15, 2014 at 09:49:44PM +0200, Arnd Bergmann wrote:

> Agreed. I think we had already concluded previously when discussing this
> patch that the clock management in APCI-5.1 should not be used on ARM64,
> but I think there is a problem one level deeper: The 5.0 and 5.1 versions
> apparently add a lot of new features that are meant for either ARM64
> servers or embedded x86 machines. These two typically have conflicting
> requirements, and it would be better for the specification itself to
> provide clearer statements to which parts apply in what use case rather
> than us (the Linux people) making claims about what parts of the spec
> are acceptable or not.

> There are already two specified classes of systems, the legacy x86
> and itanium machines, and the "reduced hardware" profile, which
> apparently covers both of the new types of machines mentioned above.

We also have legacy machines taking bits of the new stuff to contend
with.

> What would be the process to get a clarification into the next version
> of ACPI that makes them more distinct?

I agree with this, I actually had some patches come in this week which
seem to be worse than anything we've talked about and were intended just
key off DMI data instead of any form of platform data AFAICT.  The
pushback was that the Windows driver was doing this and the BIOS wasn't
about to change for us.  I'm not really convinced that waving some Linux
internal document about ARM servers at them is going to be terribly
persuasive to get them to improve future systems.

Graeme Gregory Aug. 16, 2014, 12:45 p.m. UTC | #57

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

Graeme Gregory Aug. 16, 2014, 12:51 p.m. UTC | #58

On Fri, Aug 15, 2014 at 09:49:44PM +0200, Arnd Bergmann wrote:
> What would be the process to get a clarification into the next version
> of ACPI that makes them more distinct?
> 
If you work for a UEFI member you can sign up to become a ASWG member on
the UEFI webpage. You can then submit at ECR (Engineering Change Request)
with the clarifications.

If you do not work for a UEFI member I guess you would have to do this
by proxy.

Graeme

Hanjun Guo Aug. 18, 2014, 9:29 a.m. UTC | #59

On 2014-8-15 18:01, Catalin Marinas wrote:
> Hanjun,

Hi Catalin,

> 
> On Fri, Aug 15, 2014 at 10:09:42AM +0100, Hanjun Guo wrote:
>> On 2014-8-14 18:27, Catalin Marinas wrote:
>>> On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
>>>> On 2014-8-14 7:41, Rafael J. Wysocki wrote:
>>>>> On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
>>>>>> 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.
>>>>
>>>> Real users will coming soon, we already tested this patch set on real hardware
>>>> (ARM64 Juno platform),
>>>
>>> I don't consider Juno a server platform ;) (but it's good enough for
>>> development).
>>>
>>>> and I think ARM64 server chips and platforms will show up before 3.18
>>>> is released.
>>>
>>> That's what I've heard/seen. The questions I have are (a) whether
>>> current ACPI patchset is enough to successfully run Linux on such
>>> _hardware_ platform (maybe not fully optimised, for example just WFI
>>> cpuidle) and (b) whether we still want to mandate a DT in the kernel for
>>> such platforms.
>>
>> For (a), this patch set is only for ARM64 core, not including platform
>> specific device drivers, it will be covered by the binding of _DSD or
>> explicit definition of PNP ID/ACPI ID(s).
> 
> So we go back to the discussions we had few months ago in Macau. I'm not
> concerned about the core ARM and architected peripherals covered by ACPI
> 5.1 (as long as the current patches get positive technical review). But
> I'm concerned about the additional bits needed for a real SoC like _DSD
> definitions, how they get reviewed/accepted (or is it just the vendor's
> problem?).

As the _DSD patch set sent out by Intel folks, _DSD definitions are just
DT definitions. To use _DSD or not, I think it depends on OEM use cases,
we can bring up Juno without _DSD (Graeme is working on that, still need
some time to clean up the code).

> 
> I think SBSA is too vague to guarantee a kernel image running on a
> compliant platform without additional (vendor-specific) tweaks. So what
> I asked for is (1) a document (guide) to define the strict set of ACPI
> features and bindings needed for a real SoC and (2) proof that the
> guidelines are enough for real hardware. I think we have (1) under
> review with some good feedback so far. As for (2), we can probably only
> discuss Juno openly. I think you could share the additional Juno patches
> on this list so that reviewers can assess the suitability. If we deem
> ACPI not (yet) suitable for Juno, is there other platform we could see
> patches for?

Ok, we will send out all the patches for Juno in next version for review,
as mentioned above, we still need more time to clean up the code.

> 
>>> Given the answer to (a) and what other features are needed, we may or
>>> may not mandate (b). We were pretty clear few months ago that (b) is
>>> still required but at the time we were only openly talking about ACPI
>>> 5.0 which was lacking many features. I think we need to revisit that
>>> position based on how usable ACPI 5.1 for ARM (and current kernel
>>> implementation) is. Would you mind elaborating what an ACPI-only
>>> platform miss?
>>
>> Do you mean something still missing? We still miss some features for
>> ARM in ACPI, but I think they are not critical, here is the list I can
>> remember:
>> - ITS for GICv3/4;
>> - SMMU support;
>> - CPU idle control.
> 
> I agree, these are not critical at this stage. But they only refer to
> architected peripherals. Is there anything else missing for an SoC? Do
> we need to define clocks?

No, I prefer not. As we discussed in this thread before, we don't need
clock definition if we use SBSA compatible UART on Juno.

> 
>> For ACPI 5.1, it fixes many problems for ARM:
>> - weak definition for GIC, so we introduce visualization, v2m and
>>   part of GICv3/4 (redistributors) support.
>> - No support for PSCI. Fix it to support PSCI 0.2+;
>> - Not support for Always-on timer and SBSA-L1 watchdog.
> 
> These are all good, that's why we shouldn't even talk about ACPI 5.0 in
> the ARM context.
> 
>> - How to describe device properties, so _DSD is introduced for
>>   device probe.
> 
> For the last bullet, is there any review process (at least like what we
> have for DT bindings)? On top of such process, do we have guidelines and
> example code on how the Linux support should be implemented. As Olof
> mentioned, should we see how the DT and ACPI probing paths work
> together? I really think we should be very clear here and not let
> vendors invent their own independent methods.

As said above, Intel folks provided some good examples for that, and
clarified a lot of things:

https://lkml.org/lkml/2014/8/17/10

> 
>>> I would expect a new server platform designed with ACPI in mind to
>>> require minimal SoC specific code, so we may only see a few patches
>>> under drivers/ for such platforms adding ACPI-specific probing (possibly
>>> new drivers as well if it's a new component).
>>>
>>>> For this patch set, DT is the first class citizen at now:
>>>>
>>>> a) We can always set CONFIG_ACPI as off in Kconfig, and use DT only;
>>>
>>> Not just off but, based on maturity, depend on EXPERT.
>>
>> Ok. And don't set ACPI default off (pass acpi=on to enable it)?
> 
> That's my view, just make it clear ACPI is experimental at the Kconfig
> level because longer term we won't mandate SoCs to provide both DT and
> ACPI tables.

I agree with you that if we set ACPI default off, firmware will always
pass acpi=on if they want to use ACPI, so I think it would be better
to depend on EXPERT instead.

Olof, is it ok to you too?

Thanks
Hanjun

Mark Rutland Aug. 18, 2014, 12:49 p.m. UTC | #60

On Mon, Aug 18, 2014 at 10:29:26AM +0100, Hanjun Guo wrote:
> On 2014-8-15 18:01, Catalin Marinas wrote:
> > Hanjun,
> 
> Hi Catalin,
> 
> > 
> > On Fri, Aug 15, 2014 at 10:09:42AM +0100, Hanjun Guo wrote:
> >> On 2014-8-14 18:27, Catalin Marinas wrote:
> >>> On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
> >>>> On 2014-8-14 7:41, Rafael J. Wysocki wrote:
> >>>>> On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
> >>>>>> 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.
> >>>>
> >>>> Real users will coming soon, we already tested this patch set on real hardware
> >>>> (ARM64 Juno platform),
> >>>
> >>> I don't consider Juno a server platform ;) (but it's good enough for
> >>> development).
> >>>
> >>>> and I think ARM64 server chips and platforms will show up before 3.18
> >>>> is released.
> >>>
> >>> That's what I've heard/seen. The questions I have are (a) whether
> >>> current ACPI patchset is enough to successfully run Linux on such
> >>> _hardware_ platform (maybe not fully optimised, for example just WFI
> >>> cpuidle) and (b) whether we still want to mandate a DT in the kernel for
> >>> such platforms.
> >>
> >> For (a), this patch set is only for ARM64 core, not including platform
> >> specific device drivers, it will be covered by the binding of _DSD or
> >> explicit definition of PNP ID/ACPI ID(s).
> > 
> > So we go back to the discussions we had few months ago in Macau. I'm not
> > concerned about the core ARM and architected peripherals covered by ACPI
> > 5.1 (as long as the current patches get positive technical review). But
> > I'm concerned about the additional bits needed for a real SoC like _DSD
> > definitions, how they get reviewed/accepted (or is it just the vendor's
> > problem?).
> 
> As the _DSD patch set sent out by Intel folks, _DSD definitions are just
> DT definitions. To use _DSD or not, I think it depends on OEM use cases,
> we can bring up Juno without _DSD (Graeme is working on that, still need
> some time to clean up the code).

Let's not confuse matters by saying that _DSD is DT. DSD allows for
key-value pairs, and has a reference mechanism roughly equivalent to
phandles. That does not make them the same thing.

Not having any guidelines for vendors is an extremely bad idea. The DT
bindings we get a chance to review often have major issues. I do not
believe that vendors will do things sanely without good guidance and
strong incentives.

[...]

> >> For ACPI 5.1, it fixes many problems for ARM:
> >> - weak definition for GIC, so we introduce visualization, v2m and
> >>   part of GICv3/4 (redistributors) support.
> >> - No support for PSCI. Fix it to support PSCI 0.2+;
> >> - Not support for Always-on timer and SBSA-L1 watchdog.
> > 
> > These are all good, that's why we shouldn't even talk about ACPI 5.0 in
> > the ARM context.
> > 
> >> - How to describe device properties, so _DSD is introduced for
> >>   device probe.
> > 
> > For the last bullet, is there any review process (at least like what we
> > have for DT bindings)? On top of such process, do we have guidelines and
> > example code on how the Linux support should be implemented. As Olof
> > mentioned, should we see how the DT and ACPI probing paths work
> > together? I really think we should be very clear here and not let
> > vendors invent their own independent methods.
> 
> As said above, Intel folks provided some good examples for that, and
> clarified a lot of things:
> 
> https://lkml.org/lkml/2014/8/17/10

Quite frankly, the examples provided in the _DSD series are atrocious.
They constitute a trivial mapping of some existing DT bindings to ACPI
which do not appear to have gone through any sort of review w.r.t.
remaining idiomatic.

Thanks,
Mark.

Grant Likely Aug. 20, 2014, 4:42 p.m. UTC | #61

On Thu, 24 Jul 2014 21:00:25 +0800, Hanjun Guo <[e-mail]> 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.

Olof Johansson Aug. 20, 2014, 10:17 p.m. UTC | #62

On Mon, Aug 18, 2014 at 05:29:26PM +0800, Hanjun Guo wrote:
> On 2014-8-15 18:01, Catalin Marinas wrote:
> > Hanjun,
> 
> Hi Catalin,
> 
> > 
> > On Fri, Aug 15, 2014 at 10:09:42AM +0100, Hanjun Guo wrote:
> >> On 2014-8-14 18:27, Catalin Marinas wrote:
> >>> On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
> >>>> On 2014-8-14 7:41, Rafael J. Wysocki wrote:
> >>>>> On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
> >>>>>> 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.
> >>>>
> >>>> Real users will coming soon, we already tested this patch set on real hardware
> >>>> (ARM64 Juno platform),
> >>>
> >>> I don't consider Juno a server platform ;) (but it's good enough for
> >>> development).
> >>>
> >>>> and I think ARM64 server chips and platforms will show up before 3.18
> >>>> is released.
> >>>
> >>> That's what I've heard/seen. The questions I have are (a) whether
> >>> current ACPI patchset is enough to successfully run Linux on such
> >>> _hardware_ platform (maybe not fully optimised, for example just WFI
> >>> cpuidle) and (b) whether we still want to mandate a DT in the kernel for
> >>> such platforms.
> >>
> >> For (a), this patch set is only for ARM64 core, not including platform
> >> specific device drivers, it will be covered by the binding of _DSD or
> >> explicit definition of PNP ID/ACPI ID(s).
> > 
> > So we go back to the discussions we had few months ago in Macau. I'm not
> > concerned about the core ARM and architected peripherals covered by ACPI
> > 5.1 (as long as the current patches get positive technical review). But
> > I'm concerned about the additional bits needed for a real SoC like _DSD
> > definitions, how they get reviewed/accepted (or is it just the vendor's
> > problem?).
> 
> As the _DSD patch set sent out by Intel folks, _DSD definitions are just
> DT definitions. To use _DSD or not, I think it depends on OEM use cases,
> we can bring up Juno without _DSD (Graeme is working on that, still need
> some time to clean up the code).
> 
> > 
> > I think SBSA is too vague to guarantee a kernel image running on a
> > compliant platform without additional (vendor-specific) tweaks. So what
> > I asked for is (1) a document (guide) to define the strict set of ACPI
> > features and bindings needed for a real SoC and (2) proof that the
> > guidelines are enough for real hardware. I think we have (1) under
> > review with some good feedback so far. As for (2), we can probably only
> > discuss Juno openly. I think you could share the additional Juno patches
> > on this list so that reviewers can assess the suitability. If we deem
> > ACPI not (yet) suitable for Juno, is there other platform we could see
> > patches for?
> 
> Ok, we will send out all the patches for Juno in next version for review,
> as mentioned above, we still need more time to clean up the code.
> 
> > 
> >>> Given the answer to (a) and what other features are needed, we may or
> >>> may not mandate (b). We were pretty clear few months ago that (b) is
> >>> still required but at the time we were only openly talking about ACPI
> >>> 5.0 which was lacking many features. I think we need to revisit that
> >>> position based on how usable ACPI 5.1 for ARM (and current kernel
> >>> implementation) is. Would you mind elaborating what an ACPI-only
> >>> platform miss?
> >>
> >> Do you mean something still missing? We still miss some features for
> >> ARM in ACPI, but I think they are not critical, here is the list I can
> >> remember:
> >> - ITS for GICv3/4;
> >> - SMMU support;
> >> - CPU idle control.
> > 
> > I agree, these are not critical at this stage. But they only refer to
> > architected peripherals. Is there anything else missing for an SoC? Do
> > we need to define clocks?
> 
> No, I prefer not. As we discussed in this thread before, we don't need
> clock definition if we use SBSA compatible UART on Juno.
> 
> > 
> >> For ACPI 5.1, it fixes many problems for ARM:
> >> - weak definition for GIC, so we introduce visualization, v2m and
> >>   part of GICv3/4 (redistributors) support.
> >> - No support for PSCI. Fix it to support PSCI 0.2+;
> >> - Not support for Always-on timer and SBSA-L1 watchdog.
> > 
> > These are all good, that's why we shouldn't even talk about ACPI 5.0 in
> > the ARM context.
> > 
> >> - How to describe device properties, so _DSD is introduced for
> >>   device probe.
> > 
> > For the last bullet, is there any review process (at least like what we
> > have for DT bindings)? On top of such process, do we have guidelines and
> > example code on how the Linux support should be implemented. As Olof
> > mentioned, should we see how the DT and ACPI probing paths work
> > together? I really think we should be very clear here and not let
> > vendors invent their own independent methods.
> 
> As said above, Intel folks provided some good examples for that, and
> clarified a lot of things:
> 
> https://lkml.org/lkml/2014/8/17/10
> 
> > 
> >>> I would expect a new server platform designed with ACPI in mind to
> >>> require minimal SoC specific code, so we may only see a few patches
> >>> under drivers/ for such platforms adding ACPI-specific probing (possibly
> >>> new drivers as well if it's a new component).
> >>>
> >>>> For this patch set, DT is the first class citizen at now:
> >>>>
> >>>> a) We can always set CONFIG_ACPI as off in Kconfig, and use DT only;
> >>>
> >>> Not just off but, based on maturity, depend on EXPERT.
> >>
> >> Ok. And don't set ACPI default off (pass acpi=on to enable it)?
> > 
> > That's my view, just make it clear ACPI is experimental at the Kconfig
> > level because longer term we won't mandate SoCs to provide both DT and
> > ACPI tables.
> 
> I agree with you that if we set ACPI default off, firmware will always
> pass acpi=on if they want to use ACPI, so I think it would be better
> to depend on EXPERT instead.
> 
> Olof, is it ok to you too?

Given that we're going through all these complex schemes to merge code
that isn't ready and keeping it off by default, the answer is really
to continue holding off merging it.

We already had agreement from earlier this year that we needed to see
several systems in the _market_ that uses ACPI before we have an idea of
how messy they will be in reality. Not eval boards, development systems
or reference designs. None of the current discussion has changed that.

There's also the concern that there are still significant portions missing
from 5.1 that won't be there until 5.2 or later. Having experimental
5.1 support for a few systems (out of tree) is likely going to result in
finding out things that don't work well and should be revised -- if we
don't merge this now then we can avoid having to keep the 5.1 backwards
compatibility forever. Compare this to how we've been regretting some
of the early-defined bindings on DT and wish we didn't have to keep
them around. Please learn from our mistakes. :-)

On the patches themselves:

It's great to see the patch sets posted, and they're looking pretty good
-- things are very much heading in the right direction. My main remaining
objection is around how it is integrated with the arch code. I don't
like seeing all the dual code paths that ACPI introduces. It's obvious
that two completely different entities have written the ACPI and the DT
portions of the kernel, and we can't really have that situation for a
brand new architecture like this.

So, I'd like to see closer integration between the two before code
goes in. More shared code path and driving the differences through data
(or possibly function pointers in places, etc), and fewer completely
separate implementations.

Until then, please keep posting patches for review -- it's useful
to see where it's going. I think it's also useful to get the generic
ACPI integration merged as it has been already (with pieces going in
over time).

If someone already needs to climb the hurdle of turning on an EXPORT
config option, then it's really not a significant additional hurdle to
merge in an external branch that includes the ACPI support (and that
branch can enable it by default!). So, I think that's the solution that
makes sense for the time being.


-Olof

Hanjun Guo Aug. 21, 2014, 4 a.m. UTC | #63

On 2014-8-21 6:17, Olof Johansson wrote:
> On Mon, Aug 18, 2014 at 05:29:26PM +0800, Hanjun Guo wrote:
>> On 2014-8-15 18:01, Catalin Marinas wrote:
>>> Hanjun,
>>
>> Hi Catalin,
>>
>>>
>>> On Fri, Aug 15, 2014 at 10:09:42AM +0100, Hanjun Guo wrote:
>>>> On 2014-8-14 18:27, Catalin Marinas wrote:
>>>>> On Thu, Aug 14, 2014 at 04:21:25AM +0100, Hanjun Guo wrote:
>>>>>> On 2014-8-14 7:41, Rafael J. Wysocki wrote:
>>>>>>> On Tuesday, August 12, 2014 07:23:47 PM Catalin Marinas wrote:
>>>>>>>> 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.
>>>>>>
>>>>>> Real users will coming soon, we already tested this patch set on real hardware
>>>>>> (ARM64 Juno platform),
>>>>>
>>>>> I don't consider Juno a server platform ;) (but it's good enough for
>>>>> development).
>>>>>
>>>>>> and I think ARM64 server chips and platforms will show up before 3.18
>>>>>> is released.
>>>>>
>>>>> That's what I've heard/seen. The questions I have are (a) whether
>>>>> current ACPI patchset is enough to successfully run Linux on such
>>>>> _hardware_ platform (maybe not fully optimised, for example just WFI
>>>>> cpuidle) and (b) whether we still want to mandate a DT in the kernel for
>>>>> such platforms.
>>>>
>>>> For (a), this patch set is only for ARM64 core, not including platform
>>>> specific device drivers, it will be covered by the binding of _DSD or
>>>> explicit definition of PNP ID/ACPI ID(s).
>>>
>>> So we go back to the discussions we had few months ago in Macau. I'm not
>>> concerned about the core ARM and architected peripherals covered by ACPI
>>> 5.1 (as long as the current patches get positive technical review). But
>>> I'm concerned about the additional bits needed for a real SoC like _DSD
>>> definitions, how they get reviewed/accepted (or is it just the vendor's
>>> problem?).
>>
>> As the _DSD patch set sent out by Intel folks, _DSD definitions are just
>> DT definitions. To use _DSD or not, I think it depends on OEM use cases,
>> we can bring up Juno without _DSD (Graeme is working on that, still need
>> some time to clean up the code).
>>
>>>
>>> I think SBSA is too vague to guarantee a kernel image running on a
>>> compliant platform without additional (vendor-specific) tweaks. So what
>>> I asked for is (1) a document (guide) to define the strict set of ACPI
>>> features and bindings needed for a real SoC and (2) proof that the
>>> guidelines are enough for real hardware. I think we have (1) under
>>> review with some good feedback so far. As for (2), we can probably only
>>> discuss Juno openly. I think you could share the additional Juno patches
>>> on this list so that reviewers can assess the suitability. If we deem
>>> ACPI not (yet) suitable for Juno, is there other platform we could see
>>> patches for?
>>
>> Ok, we will send out all the patches for Juno in next version for review,
>> as mentioned above, we still need more time to clean up the code.
>>
>>>
>>>>> Given the answer to (a) and what other features are needed, we may or
>>>>> may not mandate (b). We were pretty clear few months ago that (b) is
>>>>> still required but at the time we were only openly talking about ACPI
>>>>> 5.0 which was lacking many features. I think we need to revisit that
>>>>> position based on how usable ACPI 5.1 for ARM (and current kernel
>>>>> implementation) is. Would you mind elaborating what an ACPI-only
>>>>> platform miss?
>>>>
>>>> Do you mean something still missing? We still miss some features for
>>>> ARM in ACPI, but I think they are not critical, here is the list I can
>>>> remember:
>>>> - ITS for GICv3/4;
>>>> - SMMU support;
>>>> - CPU idle control.
>>>
>>> I agree, these are not critical at this stage. But they only refer to
>>> architected peripherals. Is there anything else missing for an SoC? Do
>>> we need to define clocks?
>>
>> No, I prefer not. As we discussed in this thread before, we don't need
>> clock definition if we use SBSA compatible UART on Juno.
>>
>>>
>>>> For ACPI 5.1, it fixes many problems for ARM:
>>>> - weak definition for GIC, so we introduce visualization, v2m and
>>>>   part of GICv3/4 (redistributors) support.
>>>> - No support for PSCI. Fix it to support PSCI 0.2+;
>>>> - Not support for Always-on timer and SBSA-L1 watchdog.
>>>
>>> These are all good, that's why we shouldn't even talk about ACPI 5.0 in
>>> the ARM context.
>>>
>>>> - How to describe device properties, so _DSD is introduced for
>>>>   device probe.
>>>
>>> For the last bullet, is there any review process (at least like what we
>>> have for DT bindings)? On top of such process, do we have guidelines and
>>> example code on how the Linux support should be implemented. As Olof
>>> mentioned, should we see how the DT and ACPI probing paths work
>>> together? I really think we should be very clear here and not let
>>> vendors invent their own independent methods.
>>
>> As said above, Intel folks provided some good examples for that, and
>> clarified a lot of things:
>>
>> https://lkml.org/lkml/2014/8/17/10
>>
>>>
>>>>> I would expect a new server platform designed with ACPI in mind to
>>>>> require minimal SoC specific code, so we may only see a few patches
>>>>> under drivers/ for such platforms adding ACPI-specific probing (possibly
>>>>> new drivers as well if it's a new component).
>>>>>
>>>>>> For this patch set, DT is the first class citizen at now:
>>>>>>
>>>>>> a) We can always set CONFIG_ACPI as off in Kconfig, and use DT only;
>>>>>
>>>>> Not just off but, based on maturity, depend on EXPERT.
>>>>
>>>> Ok. And don't set ACPI default off (pass acpi=on to enable it)?
>>>
>>> That's my view, just make it clear ACPI is experimental at the Kconfig
>>> level because longer term we won't mandate SoCs to provide both DT and
>>> ACPI tables.
>>
>> I agree with you that if we set ACPI default off, firmware will always
>> pass acpi=on if they want to use ACPI, so I think it would be better
>> to depend on EXPERT instead.
>>
>> Olof, is it ok to you too?
> 
> Given that we're going through all these complex schemes to merge code
> that isn't ready and keeping it off by default, the answer is really
> to continue holding off merging it.
> 
> We already had agreement from earlier this year that we needed to see
> several systems in the _market_ that uses ACPI before we have an idea of
> how messy they will be in reality. Not eval boards, development systems
> or reference designs. None of the current discussion has changed that.

I think some SBSA compatible ARM64 server boards would work, such as Seattle
board, right?

> 
> There's also the concern that there are still significant portions missing
> from 5.1 that won't be there until 5.2 or later. Having experimental
> 5.1 support for a few systems (out of tree) is likely going to result in
> finding out things that don't work well and should be revised -- if we
> don't merge this now then we can avoid having to keep the 5.1 backwards
> compatibility forever. Compare this to how we've been regretting some
> of the early-defined bindings on DT and wish we didn't have to keep
> them around. Please learn from our mistakes. :-)

Yes, there are some features missing in ACPI 5.1, and I list them out
in previous email replying Catalin, as I said, they are not critical
for ARM64 for now.

> 
> On the patches themselves:
> 
> It's great to see the patch sets posted, and they're looking pretty good
> -- things are very much heading in the right direction. My main remaining
> objection is around how it is integrated with the arch code. I don't
> like seeing all the dual code paths that ACPI introduces. It's obvious
> that two completely different entities have written the ACPI and the DT
> portions of the kernel, and we can't really have that situation for a
> brand new architecture like this.
> 
> So, I'd like to see closer integration between the two before code
> goes in. More shared code path and driving the differences through data
> (or possibly function pointers in places, etc), and fewer completely
> separate implementations.

I'm working on it, I hope I can achieve that, you know, it needs
more efforts to do it :)

> 
> Until then, please keep posting patches for review -- it's useful
> to see where it's going. I think it's also useful to get the generic
> ACPI integration merged as it has been already (with pieces going in
> over time).

Sure I will :)

Thanks
Hanjun

Patch

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.