[V4 PATCH 0/6] ACPI: Introduce support for _CCA object

[V4 PATCH 0/6] ACPI: Introduce support for _CCA object

[Suravee Suthikulpanit]

This patch series introduce support for _CCA object, which is currently
used mainly by ARM64 platform to specify DMA coherency attribute for
devices when booting with ACPI.

A copy of ACPIv6 can be found here:
    http://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf

This patch also introduces 2 new APIS:
    1. acpi_dma_is_coherent() as part of ACPI API.
    2. device_dma_is_coherent() as part of unified device property API.

This simplifies the logic in device drivers to determine device coherency
attribute regardless of booting with DT vs ACPI.

This has been tested on AMD-Seattle platform, which implements _CCA 
object as described in the AMD Opteron A1100 Series Processor ACPI Porting Guide:

http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2012/10/Seattle_ACPI_Guide.pdf

Changes from V3 (https://lkml.org/lkml/2015/5/7/1004):
    * Remove ARCH64_SUPPORT_ACPI_CCA_ZERO and just use CONFIG_ARM64.
      (per Catalin and Rafael)
    * Do not need to call arch_setup_dma_ops() for acpi_device->dev.
      (per Rafael)
    * [3/6] (New) We also need to call arch_setup_dma_ops() for pci
      devices and check the CCA of the host bridge. Similar logic
      exists for OF. So, I refactor of_pci_dma_configure() to
      the more generic version pci_dma_configure(), and add support
      for ACPI.

Changes from V2 (https://lkml.org/lkml/2015/5/5/510):
    * Reword ACPI_MUST_HAVE_CCA to ACPI_CCA_REQUIRED (per Rafael)
    * Reword ACPI_SUPPORT_CCA_ZERO to ARCH64_SUPPORT_ACPI_CCA_ZERO
      (per Rafael and Arnd)
    * Misc code styling clean up (per Rafael)
    * Only print missing _CCA warning message in debug mode.
    * Refactor logic in acpi_setup_device_dma() into
      if acpi_dma_is_supported() then call arch_setup_dma_ops().
    * Do not allocate device dma_mask if !acpi_dma_is_supported()
      (per Arnd).
    * Re-use the dummy functions with the same signature.

Changes from V1 (https://lkml.org/lkml/2015/4/29/290):
    * Remove supports for 32-bit ARM since doesn't currently
      supporting ACPI (Per Catalin suggestions.)
    * Do not call arch_setup_dma_ops() and when _CCA is missing.
      (per Arnd suggestion)
    * Add CONFIG_ACPI_SUPPORT_CCA_ZERO kernel config flag to
      allow architectures to specify the behavior when _CCA=0.
    * Add dummy_dma_ops for ARM64 (per Catalin suggestions).
    * Fixed build error when ACPI is not configured by defining
      acpi_dma_is_coherent() for when CONFIG_ACPI is not set.
    * Introduce device_dma_is_coherent().
    * Use device_dma_is_coherent in crypto/ccp and amd-xgbe driver.

Changes from RFC: (https://lkml.org/lkml/2015/4/1/389)
    * New logic for deriving and propagating coherent attribute from
      parent devices. (by Mark)
    * Introducing acpi_dma_is_coherent() API (Per Tom suggestion)
    * Introducing CONFIG_ACPI_MUST_HAVE_CCA kernel configuration.
    * Rebased to linux-4.1-rc1

Suravee Suthikulpanit (6):
  ACPI / scan: Parse _CCA and setup device coherency
  arm64 : Introduce support for ACPI _CCA object
  pci: Generic function for setting up PCI device DMA coherency
  device property: Introduces device_dma_is_coherent()
  crypto: ccp - Unify coherency checking logic with
    device_dma_is_coherent()
  amd-xgbe: Unify coherency checking logic with device_dma_is_coherent()

 arch/arm64/Kconfig                        |  1 +
 arch/arm64/include/asm/dma-mapping.h      | 18 +++++-
 arch/arm64/mm/dma-mapping.c               | 92 +++++++++++++++++++++++++++++++
 drivers/acpi/Kconfig                      |  3 +
 drivers/acpi/acpi_platform.c              | 10 +++-
 drivers/acpi/scan.c                       | 40 ++++++++++++++
 drivers/base/property.c                   | 12 ++++
 drivers/crypto/ccp/ccp-platform.c         | 60 +-------------------
 drivers/net/ethernet/amd/xgbe/xgbe-main.c | 27 +--------
 drivers/of/of_pci.c                       | 20 -------
 drivers/pci/probe.c                       | 35 +++++++++++-
 include/acpi/acpi_bus.h                   | 32 ++++++++++-
 include/linux/acpi.h                      | 10 ++++
 include/linux/of_pci.h                    |  3 -
 include/linux/property.h                  |  2 +
 15 files changed, 249 insertions(+), 116 deletions(-)

-- 
2.1.0

[V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Suravee Suthikulpanit]

This patch implements support for ACPI _CCA object, which is introduced in
ACPIv5.1, can be used for specifying device DMA coherency attribute.

The parsing logic traverses device namespace to parse coherency
information, and stores it in acpi_device_flags. Then uses it to call
arch_setup_dma_ops() when creating each device enumerated in DSDT
during ACPI scan.

This patch also introduces acpi_dma_is_coherent(), which provides
an interface for device drivers to check the coherency information
similarly to the of_dma_is_coherent().

Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Rafael J. Wysocki <rjw@rjwysocki.net>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Will Deacon <will.deacon@arm.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
---
 drivers/acpi/Kconfig         |  3 +++
 drivers/acpi/acpi_platform.c | 10 +++++++---
 drivers/acpi/scan.c          | 40 ++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h      | 32 +++++++++++++++++++++++++++++++-
 include/linux/acpi.h         | 10 ++++++++++
 5 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index ab2cbb5..212735f 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -54,6 +54,9 @@ config ACPI_GENERIC_GSI
 config ACPI_SYSTEM_POWER_STATES_SUPPORT
 	bool
 
+config ACPI_CCA_REQUIRED
+	bool
+
 config ACPI_SLEEP
 	bool
 	depends on SUSPEND || HIBERNATION
diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 4bf7559..f6bc438 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -103,14 +103,18 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 	pdevinfo.res = resources;
 	pdevinfo.num_res = count;
 	pdevinfo.fwnode = acpi_fwnode_handle(adev);
-	pdevinfo.dma_mask = DMA_BIT_MASK(32);
+	pdevinfo.dma_mask = acpi_dma_is_supported(adev) ? DMA_BIT_MASK(32) : 0;
 	pdev = platform_device_register_full(&pdevinfo);
-	if (IS_ERR(pdev))
+	if (IS_ERR(pdev)) {
 		dev_err(&adev->dev, "platform device creation failed: %ld\n",
 			PTR_ERR(pdev));
-	else
+	} else {
+		if (acpi_dma_is_supported(adev))
+			arch_setup_dma_ops(&pdev->dev, 0, 0, NULL,
+					   acpi_dma_is_coherent(adev));
 		dev_dbg(&adev->dev, "created platform device %s\n",
 			dev_name(&pdev->dev));
+	}
 
 	kfree(resources);
 	return pdev;
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 849b699..c56e66a 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -11,6 +11,7 @@
 #include <linux/kthread.h>
 #include <linux/dmi.h>
 #include <linux/nls.h>
+#include <linux/dma-mapping.h>
 
 #include <asm/pgtable.h>
 
@@ -2137,6 +2138,44 @@ void acpi_free_pnp_ids(struct acpi_device_pnp *pnp)
 	kfree(pnp->unique_id);
 }
 
+static void acpi_init_coherency(struct acpi_device *adev)
+{
+	unsigned long long cca = 0;
+	acpi_status status;
+	struct acpi_device *parent = adev->parent;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+	if (parent && parent->flags.cca_seen) {
+		/*
+		 * From ACPI spec, OSPM will ignore _CCA if an ancestor
+		 * already saw one.
+		 */
+		adev->flags.cca_seen = 1;
+		cca = acpi_dma_is_coherent(parent);
+	} else {
+		status = acpi_evaluate_integer(adev->handle, "_CCA",
+					       NULL, &cca);
+		if (ACPI_SUCCESS(status)) {
+			adev->flags.cca_seen = 1;
+		} else if (!IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED)) {
+			/*
+			 * If architecture does not specify that _CCA is
+			 * required for DMA-able devices (e.g. x86),
+			 * we default to _CCA=1.
+			 */
+			cca = 1;
+		} else {
+			acpi_get_name(adev->handle, ACPI_FULL_PATHNAME,
+				      &buffer);
+			pr_debug("ACPI device %s is missing _CCA.\n",
+				(char *) buffer.pointer);
+			kfree(buffer.pointer);
+		}
+	}
+
+	adev->flags.is_coherent = cca;
+}
+
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 			     int type, unsigned long long sta)
 {
@@ -2155,6 +2194,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	device->flags.visited = false;
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);
+	acpi_init_coherency(device);
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 8de4fa9..2a05ffb 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -208,7 +208,9 @@ struct acpi_device_flags {
 	u32 visited:1;
 	u32 hotplug_notify:1;
 	u32 is_dock_station:1;
-	u32 reserved:23;
+	u32 is_coherent:1;
+	u32 cca_seen:1;
+	u32 reserved:21;
 };
 
 /* File System */
@@ -380,6 +382,34 @@ struct acpi_device {
 	void (*remove)(struct acpi_device *);
 };
 
+static inline bool acpi_dma_is_supported(struct acpi_device *adev)
+{
+	/**
+	 * Currently, we mainly support _CCA=1 (i.e. is_coherent=1)
+	 * This should be equivalent to specifyig dma-coherent for
+	 * a device in OF.
+	 *
+	 * For the case when _CCA=0 (i.e. is_coherent=0 && cca_seen=1),
+	 * There are two approaches:
+	 * 1. Do not support and disable DMA.
+	 * 2. Support but rely on arch-specific cache maintenance for
+	 * non-coherence DMA operations. ARM64 is one example.
+	 *
+	 * For the case when _CCA is missing (i.e. cca_seen=0) but
+	 * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
+	 * and fallback to arch-specific default handling.
+	 *
+	 * See acpi_init_coherency() for more info.
+	 */
+	return adev && (adev->flags.is_coherent ||
+			(adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64)));
+}
+
+static inline bool acpi_dma_is_coherent(struct acpi_device *adev)
+{
+	return adev && adev->flags.is_coherent;
+}
+
 static inline bool is_acpi_node(struct fwnode_handle *fwnode)
 {
 	return fwnode && fwnode->type == FWNODE_ACPI;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index b10c4a6..baccf3b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -583,6 +583,16 @@ static inline int acpi_device_modalias(struct device *dev,
 	return -ENODEV;
 }
 
+static inline bool acpi_dma_is_supported(struct acpi_device *adev)
+{
+	return false;
+}
+
+static inline bool acpi_dma_is_coherent(struct acpi_device *adev)
+{
+	return false;
+}
+
 #define ACPI_PTR(_ptr)	(NULL)
 
 #endif	/* !CONFIG_ACPI */
-- 
2.1.0

Re: [V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Rafael J. Wysocki]

On Friday, May 15, 2015 04:23:09 PM Suravee Suthikulpanit wrote:
> This patch implements support for ACPI _CCA object, which is introduced in
> ACPIv5.1, can be used for specifying device DMA coherency attribute.
> 
> The parsing logic traverses device namespace to parse coherency
> information, and stores it in acpi_device_flags. Then uses it to call
> arch_setup_dma_ops() when creating each device enumerated in DSDT
> during ACPI scan.
> 
> This patch also introduces acpi_dma_is_coherent(), which provides
> an interface for device drivers to check the coherency information
> similarly to the of_dma_is_coherent().
> 
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  drivers/acpi/Kconfig         |  3 +++
>  drivers/acpi/acpi_platform.c | 10 +++++++---
>  drivers/acpi/scan.c          | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h      | 32 +++++++++++++++++++++++++++++++-
>  include/linux/acpi.h         | 10 ++++++++++
>  5 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index ab2cbb5..212735f 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -54,6 +54,9 @@ config ACPI_GENERIC_GSI
>  config ACPI_SYSTEM_POWER_STATES_SUPPORT
>  	bool
>  
> +config ACPI_CCA_REQUIRED
> +	bool
> +
>  config ACPI_SLEEP
>  	bool
>  	depends on SUSPEND || HIBERNATION
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index 4bf7559..f6bc438 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -103,14 +103,18 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
>  	pdevinfo.res = resources;
>  	pdevinfo.num_res = count;
>  	pdevinfo.fwnode = acpi_fwnode_handle(adev);
> -	pdevinfo.dma_mask = DMA_BIT_MASK(32);
> +	pdevinfo.dma_mask = acpi_dma_is_supported(adev) ? DMA_BIT_MASK(32) : 0;
>  	pdev = platform_device_register_full(&pdevinfo);
> -	if (IS_ERR(pdev))
> +	if (IS_ERR(pdev)) {
>  		dev_err(&adev->dev, "platform device creation failed: %ld\n",
>  			PTR_ERR(pdev));
> -	else
> +	} else {
> +		if (acpi_dma_is_supported(adev))
> +			arch_setup_dma_ops(&pdev->dev, 0, 0, NULL,
> +					   acpi_dma_is_coherent(adev));

Shouldn't we generally do that in acpi_bind_one() for all bus types
that don't have specific handling rather than here?

>  		dev_dbg(&adev->dev, "created platform device %s\n",
>  			dev_name(&pdev->dev));
> +	}
>  
>  	kfree(resources);
>  	return pdev;
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 849b699..c56e66a 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -11,6 +11,7 @@
>  #include <linux/kthread.h>
>  #include <linux/dmi.h>
>  #include <linux/nls.h>
> +#include <linux/dma-mapping.h>
>  
>  #include <asm/pgtable.h>
>  
> @@ -2137,6 +2138,44 @@ void acpi_free_pnp_ids(struct acpi_device_pnp *pnp)
>  	kfree(pnp->unique_id);
>  }
>  
> +static void acpi_init_coherency(struct acpi_device *adev)
> +{
> +	unsigned long long cca = 0;
> +	acpi_status status;
> +	struct acpi_device *parent = adev->parent;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +
> +	if (parent && parent->flags.cca_seen) {
> +		/*
> +		 * From ACPI spec, OSPM will ignore _CCA if an ancestor
> +		 * already saw one.
> +		 */
> +		adev->flags.cca_seen = 1;
> +		cca = acpi_dma_is_coherent(parent);

Shouldn't the device's own _CCA take precedence?

> +	} else {
> +		status = acpi_evaluate_integer(adev->handle, "_CCA",
> +					       NULL, &cca);
> +		if (ACPI_SUCCESS(status)) {
> +			adev->flags.cca_seen = 1;
> +		} else if (!IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED)) {
> +			/*
> +			 * If architecture does not specify that _CCA is
> +			 * required for DMA-able devices (e.g. x86),
> +			 * we default to _CCA=1.
> +			 */
> +			cca = 1;
> +		} else {

What about using acpi_handle_debug() here?

> +			acpi_get_name(adev->handle, ACPI_FULL_PATHNAME,
> +				      &buffer);
> +			pr_debug("ACPI device %s is missing _CCA.\n",
> +				(char *) buffer.pointer);
> +			kfree(buffer.pointer);
> +		}
> +	}
> +
> +	adev->flags.is_coherent = cca;
> +}
> +
>  void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
>  			     int type, unsigned long long sta)
>  {
> @@ -2155,6 +2194,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
>  	device->flags.visited = false;
>  	device_initialize(&device->dev);
>  	dev_set_uevent_suppress(&device->dev, true);
> +	acpi_init_coherency(device);
>  }
>  
>  void acpi_device_add_finalize(struct acpi_device *device)
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 8de4fa9..2a05ffb 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -208,7 +208,9 @@ struct acpi_device_flags {
>  	u32 visited:1;
>  	u32 hotplug_notify:1;
>  	u32 is_dock_station:1;
> -	u32 reserved:23;
> +	u32 is_coherent:1;

I'd prefer to call this 'coherent_dma'.

> +	u32 cca_seen:1;
> +	u32 reserved:21;
>  };
>  
>  /* File System */
> @@ -380,6 +382,34 @@ struct acpi_device {
>  	void (*remove)(struct acpi_device *);
>  };
>  
> +static inline bool acpi_dma_is_supported(struct acpi_device *adev)
> +{
> +	/**
> +	 * Currently, we mainly support _CCA=1 (i.e. is_coherent=1)
> +	 * This should be equivalent to specifyig dma-coherent for
> +	 * a device in OF.
> +	 *
> +	 * For the case when _CCA=0 (i.e. is_coherent=0 && cca_seen=1),
> +	 * There are two approaches:
> +	 * 1. Do not support and disable DMA.
> +	 * 2. Support but rely on arch-specific cache maintenance for
> +	 * non-coherence DMA operations. ARM64 is one example.
> +	 *
> +	 * For the case when _CCA is missing (i.e. cca_seen=0) but
> +	 * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
> +	 * and fallback to arch-specific default handling.
> +	 *
> +	 * See acpi_init_coherency() for more info.
> +	 */
> +	return adev && (adev->flags.is_coherent ||
> +			(adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64)));
> +}
> +
> +static inline bool acpi_dma_is_coherent(struct acpi_device *adev)
> +{
> +	return adev && adev->flags.is_coherent;
> +}
> +
>  static inline bool is_acpi_node(struct fwnode_handle *fwnode)
>  {
>  	return fwnode && fwnode->type == FWNODE_ACPI;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index b10c4a6..baccf3b 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -583,6 +583,16 @@ static inline int acpi_device_modalias(struct device *dev,
>  	return -ENODEV;
>  }
>  
> +static inline bool acpi_dma_is_supported(struct acpi_device *adev)
> +{
> +	return false;
> +}
> +
> +static inline bool acpi_dma_is_coherent(struct acpi_device *adev)
> +{
> +	return false;
> +}
> +
>  #define ACPI_PTR(_ptr)	(NULL)
>  
>  #endif	/* !CONFIG_ACPI */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Suravee Suthikulanit]

Hi Rafael,

On 5/15/2015 6:53 PM, Rafael J. Wysocki wrote:
> On Friday, May 15, 2015 04:23:09 PM Suravee Suthikulpanit wrote:
>> [...]
>> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
>> index 4bf7559..f6bc438 100644
>> --- a/drivers/acpi/acpi_platform.c
>> +++ b/drivers/acpi/acpi_platform.c
>> @@ -103,14 +103,18 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
>>   	pdevinfo.res = resources;
>>   	pdevinfo.num_res = count;
>>   	pdevinfo.fwnode = acpi_fwnode_handle(adev);
>> -	pdevinfo.dma_mask = DMA_BIT_MASK(32);
>> +	pdevinfo.dma_mask = acpi_dma_is_supported(adev) ? DMA_BIT_MASK(32) : 0;
>>   	pdev = platform_device_register_full(&pdevinfo);
>> -	if (IS_ERR(pdev))
>> +	if (IS_ERR(pdev)) {
>>   		dev_err(&adev->dev, "platform device creation failed: %ld\n",
>>   			PTR_ERR(pdev));
>> -	else
>> +	} else {
>> +		if (acpi_dma_is_supported(adev))
>> +			arch_setup_dma_ops(&pdev->dev, 0, 0, NULL,
>> +					   acpi_dma_is_coherent(adev));
>
> Shouldn't we generally do that in acpi_bind_one() for all bus types
> that don't have specific handling rather than here?

I think that would also work, and makes sense. However, I'm not sure if 
this would help in the case when we are creating PCI end-point devices, 
since the CCA is specified at the host bridge node, and there is no ACPI 
companion for the end-point devices. It seems that patch 3/6 of this 
series is still needed.


>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 849b699..c56e66a 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/kthread.h>
>>   #include <linux/dmi.h>
>>   #include <linux/nls.h>
>> +#include <linux/dma-mapping.h>
>>
>>   #include <asm/pgtable.h>
>>
>> @@ -2137,6 +2138,44 @@ void acpi_free_pnp_ids(struct acpi_device_pnp *pnp)
>>   	kfree(pnp->unique_id);
>>   }
>>
>> +static void acpi_init_coherency(struct acpi_device *adev)
>> +{
>> +	unsigned long long cca = 0;
>> +	acpi_status status;
>> +	struct acpi_device *parent = adev->parent;
>> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +
>> +	if (parent && parent->flags.cca_seen) {
>> +		/*
>> +		 * From ACPI spec, OSPM will ignore _CCA if an ancestor
>> +		 * already saw one.
>> +		 */
>> +		adev->flags.cca_seen = 1;
>> +		cca = acpi_dma_is_coherent(parent);
>
> Shouldn't the device's own _CCA take precedence?
According to the ACPI specification, the parent's _CCA take precedence.

>
>> +	} else {
>> +		status = acpi_evaluate_integer(adev->handle, "_CCA",
>> +					       NULL, &cca);
>> +		if (ACPI_SUCCESS(status)) {
>> +			adev->flags.cca_seen = 1;
>> +		} else if (!IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED)) {
>> +			/*
>> +			 * If architecture does not specify that _CCA is
>> +			 * required for DMA-able devices (e.g. x86),
>> +			 * we default to _CCA=1.
>> +			 */
>> +			cca = 1;
>> +		} else {
>
> What about using acpi_handle_debug() here?
Ok I can do that.

>> [...]
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index 8de4fa9..2a05ffb 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -208,7 +208,9 @@ struct acpi_device_flags {
>>   	u32 visited:1;
>>   	u32 hotplug_notify:1;
>>   	u32 is_dock_station:1;
>> -	u32 reserved:23;
>> +	u32 is_coherent:1;
>
> I'd prefer to call this 'coherent_dma'.

OK.

Thanks,

Suravee

Re: [V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Rafael J. Wysocki]

On Monday, May 18, 2015 05:38:17 PM Suravee Suthikulanit wrote:
> Hi Rafael,
> 
> On 5/15/2015 6:53 PM, Rafael J. Wysocki wrote:
> > On Friday, May 15, 2015 04:23:09 PM Suravee Suthikulpanit wrote:
> >> [...]
> >> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> >> index 4bf7559..f6bc438 100644
> >> --- a/drivers/acpi/acpi_platform.c
> >> +++ b/drivers/acpi/acpi_platform.c
> >> @@ -103,14 +103,18 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
> >>   	pdevinfo.res = resources;
> >>   	pdevinfo.num_res = count;
> >>   	pdevinfo.fwnode = acpi_fwnode_handle(adev);
> >> -	pdevinfo.dma_mask = DMA_BIT_MASK(32);
> >> +	pdevinfo.dma_mask = acpi_dma_is_supported(adev) ? DMA_BIT_MASK(32) : 0;
> >>   	pdev = platform_device_register_full(&pdevinfo);
> >> -	if (IS_ERR(pdev))
> >> +	if (IS_ERR(pdev)) {
> >>   		dev_err(&adev->dev, "platform device creation failed: %ld\n",
> >>   			PTR_ERR(pdev));
> >> -	else
> >> +	} else {
> >> +		if (acpi_dma_is_supported(adev))
> >> +			arch_setup_dma_ops(&pdev->dev, 0, 0, NULL,
> >> +					   acpi_dma_is_coherent(adev));
> >
> > Shouldn't we generally do that in acpi_bind_one() for all bus types
> > that don't have specific handling rather than here?
> 
> I think that would also work, and makes sense. However, I'm not sure if 
> this would help in the case when we are creating PCI end-point devices, 
> since the CCA is specified at the host bridge node, and there is no ACPI 
> companion for the end-point devices. It seems that patch 3/6 of this 
> series is still needed.

Yes, PCI needs its own handling, but there are multiple bus types that
don't (SPI, I2C etc) in addition to the platform bus type.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Catalin Marinas]

On Fri, May 15, 2015 at 04:23:09PM -0500, Suravee Suthikulpanit wrote:
> +static inline bool acpi_dma_is_supported(struct acpi_device *adev)
> +{
> +	/**
> +	 * Currently, we mainly support _CCA=1 (i.e. is_coherent=1)
> +	 * This should be equivalent to specifyig dma-coherent for
> +	 * a device in OF.
> +	 *
> +	 * For the case when _CCA=0 (i.e. is_coherent=0 && cca_seen=1),
> +	 * There are two approaches:
> +	 * 1. Do not support and disable DMA.
> +	 * 2. Support but rely on arch-specific cache maintenance for
> +	 * non-coherence DMA operations. ARM64 is one example.
> +	 *
> +	 * For the case when _CCA is missing (i.e. cca_seen=0) but
> +	 * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
> +	 * and fallback to arch-specific default handling.
> +	 *
> +	 * See acpi_init_coherency() for more info.
> +	 */
> +	return adev && (adev->flags.is_coherent ||
> +			(adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64)));
> +}

I don't particularly like the check for CONFIG_ARM64 here but I
understand why it was added (I had the wrong impression that x86 can
cope with _CCA = 0).

Alternatively, we could leave it out (together with cca_seen) until
someone comes forward with a real use-case for _CCA = 0 on arm64. One
platform I'm aware of is Juno but even though it boot with ACPI, I
wouldn't call it a server platform.

-- 
Catalin

Re: [V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Suravee Suthikulanit]

On 5/20/2015 5:01 AM, Catalin Marinas wrote:
> On Fri, May 15, 2015 at 04:23:09PM -0500, Suravee Suthikulpanit wrote:
>> +static inline bool acpi_dma_is_supported(struct acpi_device *adev)
>> +{
>> +	/**
>> +	 * Currently, we mainly support _CCA=1 (i.e. is_coherent=1)
>> +	 * This should be equivalent to specifyig dma-coherent for
>> +	 * a device in OF.
>> +	 *
>> +	 * For the case when _CCA=0 (i.e. is_coherent=0 && cca_seen=1),
>> +	 * There are two approaches:
>> +	 * 1. Do not support and disable DMA.
>> +	 * 2. Support but rely on arch-specific cache maintenance for
>> +	 * non-coherence DMA operations. ARM64 is one example.
>> +	 *
>> +	 * For the case when _CCA is missing (i.e. cca_seen=0) but
>> +	 * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
>> +	 * and fallback to arch-specific default handling.
>> +	 *
>> +	 * See acpi_init_coherency() for more info.
>> +	 */
>> +	return adev && (adev->flags.is_coherent ||
>> +			(adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64)));
>> +}
>
> I don't particularly like the check for CONFIG_ARM64 here but I
> understand why it was added (I had the wrong impression that x86 can
> cope with _CCA = 0).
>
> Alternatively, we could leave it out (together with cca_seen) until
> someone comes forward with a real use-case for _CCA = 0 on arm64. One
> platform I'm aware of is Juno but even though it boot with ACPI, I
> wouldn't call it a server platform.

Ok. That seems to be what Arnd would prefer as well.  Let's just leave 
the support for _CCA=0 out until it is needed then.

Thanks,
Suravee

Re: [Linaro-acpi] [V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Arnd Bergmann]

On Wednesday 20 May 2015 06:52:03 Suravee Suthikulanit wrote:
> On 5/20/2015 5:01 AM, Catalin Marinas wrote:
> > On Fri, May 15, 2015 at 04:23:09PM -0500, Suravee Suthikulpanit wrote:
> >> +static inline bool acpi_dma_is_supported(struct acpi_device *adev)
> >> +{
> >> +    /**
> >> +     * Currently, we mainly support _CCA=1 (i.e. is_coherent=1)
> >> +     * This should be equivalent to specifyig dma-coherent for
> >> +     * a device in OF.
> >> +     *
> >> +     * For the case when _CCA=0 (i.e. is_coherent=0 && cca_seen=1),
> >> +     * There are two approaches:
> >> +     * 1. Do not support and disable DMA.
> >> +     * 2. Support but rely on arch-specific cache maintenance for
> >> +     * non-coherence DMA operations. ARM64 is one example.
> >> +     *
> >> +     * For the case when _CCA is missing (i.e. cca_seen=0) but
> >> +     * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
> >> +     * and fallback to arch-specific default handling.
> >> +     *
> >> +     * See acpi_init_coherency() for more info.
> >> +     */
> >> +    return adev && (adev->flags.is_coherent ||
> >> +                    (adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64)));
> >> +}
> >
> > I don't particularly like the check for CONFIG_ARM64 here but I
> > understand why it was added (I had the wrong impression that x86 can
> > cope with _CCA = 0).
> >
> > Alternatively, we could leave it out (together with cca_seen) until
> > someone comes forward with a real use-case for _CCA = 0 on arm64. One
> > platform I'm aware of is Juno but even though it boot with ACPI, I
> > wouldn't call it a server platform.
> 
> Ok. That seems to be what Arnd would prefer as well.  Let's just leave 
> the support for _CCA=0 out until it is needed then.
> 

Yes, that would be best (as I said repeatedly ;-) )

	Arnd

Re: [Linaro-acpi] [V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Catalin Marinas]

On Wed, May 20, 2015 at 02:04:02PM +0200, Arnd Bergmann wrote:
> On Wednesday 20 May 2015 06:52:03 Suravee Suthikulanit wrote:
> > On 5/20/2015 5:01 AM, Catalin Marinas wrote:
> > > On Fri, May 15, 2015 at 04:23:09PM -0500, Suravee Suthikulpanit wrote:
> > >> +static inline bool acpi_dma_is_supported(struct acpi_device *adev)
> > >> +{
> > >> +    /**
> > >> +     * Currently, we mainly support _CCA=1 (i.e. is_coherent=1)
> > >> +     * This should be equivalent to specifyig dma-coherent for
> > >> +     * a device in OF.
> > >> +     *
> > >> +     * For the case when _CCA=0 (i.e. is_coherent=0 && cca_seen=1),
> > >> +     * There are two approaches:
> > >> +     * 1. Do not support and disable DMA.
> > >> +     * 2. Support but rely on arch-specific cache maintenance for
> > >> +     * non-coherence DMA operations. ARM64 is one example.
> > >> +     *
> > >> +     * For the case when _CCA is missing (i.e. cca_seen=0) but
> > >> +     * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
> > >> +     * and fallback to arch-specific default handling.
> > >> +     *
> > >> +     * See acpi_init_coherency() for more info.
> > >> +     */
> > >> +    return adev && (adev->flags.is_coherent ||
> > >> +                    (adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64)));
> > >> +}
> > >
> > > I don't particularly like the check for CONFIG_ARM64 here but I
> > > understand why it was added (I had the wrong impression that x86 can
> > > cope with _CCA = 0).
> > >
> > > Alternatively, we could leave it out (together with cca_seen) until
> > > someone comes forward with a real use-case for _CCA = 0 on arm64. One
> > > platform I'm aware of is Juno but even though it boot with ACPI, I
> > > wouldn't call it a server platform.
> > 
> > Ok. That seems to be what Arnd would prefer as well.  Let's just leave 
> > the support for _CCA=0 out until it is needed then.
> 
> Yes, that would be best (as I said repeatedly ;-) )

I'm sure it won't be long before someone asks for this feature ;).

-- 
Catalin

Re: [Linaro-acpi] [V4 PATCH 1/6] ACPI / scan: Parse _CCA and setup device coherency

[Arnd Bergmann]

On Thursday 21 May 2015 14:01:16 Catalin Marinas wrote:
> On Wed, May 20, 2015 at 02:04:02PM +0200, Arnd Bergmann wrote:
> > On Wednesday 20 May 2015 06:52:03 Suravee Suthikulanit wrote:
> > > 
> > > Ok. That seems to be what Arnd would prefer as well.  Let's just leave 
> > > the support for _CCA=0 out until it is needed then.
> > 
> > Yes, that would be best (as I said repeatedly  )
> 
> I'm sure it won't be long before someone asks for this feature ;).

If nothing else, we get to publically shame them for building crappy
hardware then ;-)

	Arnd

[V4 PATCH 2/6] arm64 : Introduce support for ACPI _CCA object

[Suravee Suthikulpanit]

>From http://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf,
section 6.2.17 _CCA states that ARM platforms require ACPI _CCA
object to be specified for DMA-cabpable devices. This patch introduces
ACPI_MUST_HAVE_CCA in arm64 Kconfig to specify such requirement.

In case _CCA is missing, arm64 would assign dummy_dma_ops
to disable DMA capability of the device.

Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/dma-mapping.h | 18 ++++++-
 arch/arm64/mm/dma-mapping.c          | 92 ++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 4269dba..95307b4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1,5 +1,6 @@
 config ARM64
 	def_bool y
+	select ACPI_CCA_REQUIRED if ACPI
 	select ACPI_GENERIC_GSI if ACPI
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index 9437e3d..f0d6d0b 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -18,6 +18,7 @@
 
 #ifdef __KERNEL__
 
+#include <linux/acpi.h>
 #include <linux/types.h>
 #include <linux/vmalloc.h>
 
@@ -28,13 +29,23 @@
 
 #define DMA_ERROR_CODE	(~(dma_addr_t)0)
 extern struct dma_map_ops *dma_ops;
+extern struct dma_map_ops dummy_dma_ops;
 
 static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
 {
-	if (unlikely(!dev) || !dev->archdata.dma_ops)
+	if (unlikely(!dev))
 		return dma_ops;
-	else
+	else if (dev->archdata.dma_ops)
 		return dev->archdata.dma_ops;
+	else if (acpi_disabled)
+		return dma_ops;
+
+	/*
+	 * When ACPI is enabled, if arch_set_dma_ops is not called,
+	 * we will disable device DMA capability by setting it
+	 * to dummy_dma_ops.
+	 */
+	return &dummy_dma_ops;
 }
 
 static inline struct dma_map_ops *get_dma_ops(struct device *dev)
@@ -48,6 +59,9 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 				      struct iommu_ops *iommu, bool coherent)
 {
+	if (!acpi_disabled && !dev->archdata.dma_ops)
+		dev->archdata.dma_ops = dma_ops;
+
 	dev->archdata.dma_coherent = coherent;
 }
 #define arch_setup_dma_ops	arch_setup_dma_ops
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index ef7d112..6e6d6ad 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -415,6 +415,98 @@ out:
 	return -ENOMEM;
 }
 
+/********************************************
+ * The following APIs are for dummy DMA ops *
+ ********************************************/
+
+static void *__dummy_alloc(struct device *dev, size_t size,
+			   dma_addr_t *dma_handle, gfp_t flags,
+			   struct dma_attrs *attrs)
+{
+	return NULL;
+}
+
+static void __dummy_free(struct device *dev, size_t size,
+			 void *vaddr, dma_addr_t dma_handle,
+			 struct dma_attrs *attrs)
+{
+}
+
+static int __dummy_mmap(struct device *dev,
+			struct vm_area_struct *vma,
+			void *cpu_addr, dma_addr_t dma_addr, size_t size,
+			struct dma_attrs *attrs)
+{
+	return -ENXIO;
+}
+
+static dma_addr_t __dummy_map_page(struct device *dev, struct page *page,
+				   unsigned long offset, size_t size,
+				   enum dma_data_direction dir,
+				   struct dma_attrs *attrs)
+{
+	return DMA_ERROR_CODE;
+}
+
+static void __dummy_unmap_page(struct device *dev, dma_addr_t dev_addr,
+			       size_t size, enum dma_data_direction dir,
+			       struct dma_attrs *attrs)
+{
+}
+
+static int __dummy_map_sg(struct device *dev, struct scatterlist *sgl,
+			  int nelems, enum dma_data_direction dir,
+			  struct dma_attrs *attrs)
+{
+	return 0;
+}
+
+static void __dummy_unmap_sg(struct device *dev,
+			     struct scatterlist *sgl, int nelems,
+			     enum dma_data_direction dir,
+			     struct dma_attrs *attrs)
+{
+}
+
+static void __dummy_sync_single(struct device *dev,
+				dma_addr_t dev_addr, size_t size,
+				enum dma_data_direction dir)
+{
+}
+
+static void __dummy_sync_sg(struct device *dev,
+			    struct scatterlist *sgl, int nelems,
+			    enum dma_data_direction dir)
+{
+}
+
+static int __dummy_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
+{
+	return 1;
+}
+
+static int __dummy_dma_supported(struct device *hwdev, u64 mask)
+{
+	return 0;
+}
+
+struct dma_map_ops dummy_dma_ops = {
+	.alloc                  = __dummy_alloc,
+	.free                   = __dummy_free,
+	.mmap                   = __dummy_mmap,
+	.map_page               = __dummy_map_page,
+	.unmap_page             = __dummy_unmap_page,
+	.map_sg                 = __dummy_map_sg,
+	.unmap_sg               = __dummy_unmap_sg,
+	.sync_single_for_cpu    = __dummy_sync_single,
+	.sync_single_for_device = __dummy_sync_single,
+	.sync_sg_for_cpu        = __dummy_sync_sg,
+	.sync_sg_for_device     = __dummy_sync_sg,
+	.mapping_error          = __dummy_mapping_error,
+	.dma_supported          = __dummy_dma_supported,
+};
+EXPORT_SYMBOL(dummy_dma_ops);
+
 static int __init arm64_dma_init(void)
 {
 	int ret;
-- 
2.1.0

Re: [V4 PATCH 2/6] arm64 : Introduce support for ACPI _CCA object

[Paul Bolle]

Just a nit regarding the commit explanation.

On Fri, 2015-05-15 at 16:23 -0500, Suravee Suthikulpanit wrote:
> From http://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf,
> section 6.2.17 _CCA states that ARM platforms require ACPI _CCA
> object to be specified for DMA-cabpable devices. This patch introduces
> ACPI_MUST_HAVE_CCA in arm64 Kconfig to specify such requirement.

s/ACPI_MUST_HAVE_CCA/ACPI_CCA_REQUIRED/.

Ie, update the commit explanation to match the change introduced in V2.

> In case _CCA is missing, arm64 would assign dummy_dma_ops
> to disable DMA capability of the device.

Thanks,


Paul Bolle

Re: [V4 PATCH 2/6] arm64 : Introduce support for ACPI _CCA object

[Suravee Suthikulpanit]

Hi Paul,

On 5/16/15 06:48, Paul Bolle wrote:
> Just a nit regarding the commit explanation.
>
> On Fri, 2015-05-15 at 16:23 -0500, Suravee Suthikulpanit wrote:
>>  From http://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf,
>> section 6.2.17 _CCA states that ARM platforms require ACPI _CCA
>> object to be specified for DMA-cabpable devices. This patch introduces
>> ACPI_MUST_HAVE_CCA in arm64 Kconfig to specify such requirement.
>
> s/ACPI_MUST_HAVE_CCA/ACPI_CCA_REQUIRED/.
>
> Ie, update the commit explanation to match the change introduced in V2.

Ahh ... thanks :)

Suravee

Re: [V4 PATCH 2/6] arm64 : Introduce support for ACPI _CCA object

[Catalin Marinas]

On Fri, May 15, 2015 at 04:23:10PM -0500, Suravee Suthikulpanit wrote:
> From http://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf,
> section 6.2.17 _CCA states that ARM platforms require ACPI _CCA
> object to be specified for DMA-cabpable devices. This patch introduces
> ACPI_MUST_HAVE_CCA in arm64 Kconfig to specify such requirement.
> 
> In case _CCA is missing, arm64 would assign dummy_dma_ops
> to disable DMA capability of the device.
> 
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Arnd Bergmann <arnd@arndb.de>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [V4 PATCH 2/6] arm64 : Introduce support for ACPI _CCA object

[Suravee Suthikulanit]

On 5/20/2015 5:03 AM, Catalin Marinas wrote:
> On Fri, May 15, 2015 at 04:23:10PM -0500, Suravee Suthikulpanit wrote:
>>  From http://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf,
>> section 6.2.17 _CCA states that ARM platforms require ACPI _CCA
>> object to be specified for DMA-cabpable devices. This patch introduces
>> ACPI_MUST_HAVE_CCA in arm64 Kconfig to specify such requirement.
>>
>> In case _CCA is missing, arm64 would assign dummy_dma_ops
>> to disable DMA capability of the device.
>>
>> Signed-off-by: Mark Salter <msalter@redhat.com>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> CC: Catalin Marinas <catalin.marinas@arm.com>
>> CC: Will Deacon <will.deacon@arm.com>
>> CC: Arnd Bergmann <arnd@arndb.de>
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>

Thanks,

Suravee

[V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Suravee Suthikulpanit]

This patch refactors of_pci_dma_configure() into a more generic
pci_dma_configure(), which can be reused by non-OF code.
Then, it adds support for setting up PCI device DMA coherency from
ACPI _CCA object that should normally be specified in the DSDT node
of its PCI host bridge..

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Rob Herring <robh+dt@kernel.org>
CC: Will Deacon <will.deacon@arm.com>
CC: Rafael J. Wysocki <rjw@rjwysocki.net>
CC: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/of/of_pci.c    | 20 --------------------
 drivers/pci/probe.c    | 35 +++++++++++++++++++++++++++++++++--
 include/linux/of_pci.h |  3 ---
 3 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 5751dc5..b66ee4e 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -117,26 +117,6 @@ int of_get_pci_domain_nr(struct device_node *node)
 }
 EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
 
-/**
- * of_pci_dma_configure - Setup DMA configuration
- * @dev: ptr to pci_dev struct of the PCI device
- *
- * Function to update PCI devices's DMA configuration using the same
- * info from the OF node of host bridge's parent (if any).
- */
-void of_pci_dma_configure(struct pci_dev *pci_dev)
-{
-	struct device *dev = &pci_dev->dev;
-	struct device *bridge = pci_get_host_bridge_device(pci_dev);
-
-	if (!bridge->parent)
-		return;
-
-	of_dma_configure(dev, bridge->parent->of_node);
-	pci_put_host_bridge_device(bridge);
-}
-EXPORT_SYMBOL_GPL(of_pci_dma_configure);
-
 #if defined(CONFIG_OF_ADDRESS)
 /**
  * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6675a7a..3c6f2e5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -6,12 +6,14 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/pci.h>
-#include <linux/of_pci.h>
+#include <linux/of_device.h>
 #include <linux/pci_hotplug.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/cpumask.h>
 #include <linux/pci-aspm.h>
+#include <linux/acpi.h>
+#include <linux/property.h>
 #include <asm-generic/pci-bridge.h>
 #include "pci.h"
 
@@ -1508,6 +1510,35 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_enable_acs(dev);
 }
 
+/**
+ * pci_dma_configure - Setup DMA configuration
+ * @pci_dev: ptr to pci_dev struct of the PCI device
+ *
+ * Function to update PCI devices's DMA configuration using the same
+ * info from the OF node or ACPI node of host bridge's parent (if any).
+ */
+static void pci_dma_configure(struct pci_dev *pci_dev)
+{
+	struct device *dev = &pci_dev->dev;
+	struct device *bridge = pci_get_host_bridge_device(pci_dev);
+	struct device *host = bridge->parent;
+	struct acpi_device *adev;
+
+	if (!host)
+		return;
+
+	if (acpi_disabled) {
+		of_dma_configure(dev, host->of_node);
+	} else if (has_acpi_companion(host)) {
+		adev = acpi_node(host->fwnode);
+		if (acpi_dma_is_supported(adev))
+			arch_setup_dma_ops(dev, 0, 0, NULL,
+					   acpi_dma_is_coherent(adev));
+	}
+
+	pci_put_host_bridge_device(bridge);
+}
+
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
 	int ret;
@@ -1521,7 +1552,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
 	dev->dev.coherent_dma_mask = 0xffffffffull;
-	of_pci_dma_configure(dev);
+	pci_dma_configure(dev);
 
 	pci_set_dma_max_seg_size(dev, 65536);
 	pci_set_dma_seg_boundary(dev, 0xffffffff);
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 29fd3fe..ce0e5ab 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -16,7 +16,6 @@ int of_pci_get_devfn(struct device_node *np);
 int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin);
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
-void of_pci_dma_configure(struct pci_dev *pci_dev);
 #else
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {
@@ -51,8 +50,6 @@ of_get_pci_domain_nr(struct device_node *node)
 {
 	return -1;
 }
-
-static inline void of_pci_dma_configure(struct pci_dev *pci_dev) { }
 #endif
 
 #if defined(CONFIG_OF_ADDRESS)
-- 
2.1.0

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Rafael J. Wysocki]

On Friday, May 15, 2015 04:23:11 PM Suravee Suthikulpanit wrote:
> This patch refactors of_pci_dma_configure() into a more generic
> pci_dma_configure(), which can be reused by non-OF code.
> Then, it adds support for setting up PCI device DMA coherency from
> ACPI _CCA object that should normally be specified in the DSDT node
> of its PCI host bridge..
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
> CC: Murali Karicheri <m-karicheri2@ti.com>
> ---
>  drivers/of/of_pci.c    | 20 --------------------
>  drivers/pci/probe.c    | 35 +++++++++++++++++++++++++++++++++--
>  include/linux/of_pci.h |  3 ---
>  3 files changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 5751dc5..b66ee4e 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -117,26 +117,6 @@ int of_get_pci_domain_nr(struct device_node *node)
>  }
>  EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
>  
> -/**
> - * of_pci_dma_configure - Setup DMA configuration
> - * @dev: ptr to pci_dev struct of the PCI device
> - *
> - * Function to update PCI devices's DMA configuration using the same
> - * info from the OF node of host bridge's parent (if any).
> - */
> -void of_pci_dma_configure(struct pci_dev *pci_dev)
> -{
> -	struct device *dev = &pci_dev->dev;
> -	struct device *bridge = pci_get_host_bridge_device(pci_dev);
> -
> -	if (!bridge->parent)
> -		return;
> -
> -	of_dma_configure(dev, bridge->parent->of_node);
> -	pci_put_host_bridge_device(bridge);
> -}
> -EXPORT_SYMBOL_GPL(of_pci_dma_configure);
> -
>  #if defined(CONFIG_OF_ADDRESS)
>  /**
>   * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6675a7a..3c6f2e5 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -6,12 +6,14 @@
>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/pci.h>
> -#include <linux/of_pci.h>
> +#include <linux/of_device.h>
>  #include <linux/pci_hotplug.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/cpumask.h>
>  #include <linux/pci-aspm.h>
> +#include <linux/acpi.h>
> +#include <linux/property.h>
>  #include <asm-generic/pci-bridge.h>
>  #include "pci.h"
>  
> @@ -1508,6 +1510,35 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	pci_enable_acs(dev);
>  }
>  
> +/**
> + * pci_dma_configure - Setup DMA configuration
> + * @pci_dev: ptr to pci_dev struct of the PCI device
> + *
> + * Function to update PCI devices's DMA configuration using the same
> + * info from the OF node or ACPI node of host bridge's parent (if any).
> + */
> +static void pci_dma_configure(struct pci_dev *pci_dev)
> +{
> +	struct device *dev = &pci_dev->dev;
> +	struct device *bridge = pci_get_host_bridge_device(pci_dev);
> +	struct device *host = bridge->parent;
> +	struct acpi_device *adev;
> +
> +	if (!host)
> +		return;
> +
> +	if (acpi_disabled) {
> +		of_dma_configure(dev, host->of_node);

I'd rather do

	if (IS_ENABLED(CONFIG_OF) && host->of_node) {
		of_dma_configure(dev, host->of_node);
	} else {
		struct acpi_device *adev = ACPI_COMPANION(host);

		if (adev && acpi_dma_is_supported(adev)) {


> +	} else if (has_acpi_companion(host)) {
> +		adev = acpi_node(host->fwnode);
> +		if (acpi_dma_is_supported(adev))
> +			arch_setup_dma_ops(dev, 0, 0, NULL,
> +					   acpi_dma_is_coherent(adev));
> +	}
> +
> +	pci_put_host_bridge_device(bridge);
> +}
> +
>  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  {
>  	int ret;
> @@ -1521,7 +1552,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  	dev->dev.dma_mask = &dev->dma_mask;
>  	dev->dev.dma_parms = &dev->dma_parms;
>  	dev->dev.coherent_dma_mask = 0xffffffffull;
> -	of_pci_dma_configure(dev);
> +	pci_dma_configure(dev);

Also I'm not quite sure if that's actually OK.

We should be handling the DMA setup on x86 in the ACPI case already without it.
Or is this a NOP on x86?

>  
>  	pci_set_dma_max_seg_size(dev, 65536);
>  	pci_set_dma_seg_boundary(dev, 0xffffffff);
> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index 29fd3fe..ce0e5ab 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -16,7 +16,6 @@ int of_pci_get_devfn(struct device_node *np);
>  int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin);
>  int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
>  int of_get_pci_domain_nr(struct device_node *node);
> -void of_pci_dma_configure(struct pci_dev *pci_dev);
>  #else
>  static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
>  {
> @@ -51,8 +50,6 @@ of_get_pci_domain_nr(struct device_node *node)
>  {
>  	return -1;
>  }
> -
> -static inline void of_pci_dma_configure(struct pci_dev *pci_dev) { }
>  #endif
>  
>  #if defined(CONFIG_OF_ADDRESS)
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Suthikulpanit, Suravee]

Hi Rafael,

On 5/15/15, 18:59, "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

>On Friday, May 15, 2015 04:23:11 PM Suravee Suthikulpanit wrote:
>> This patch refactors of_pci_dma_configure() into a more generic
>> pci_dma_configure(), which can be reused by non-OF code.
>> Then, it adds support for setting up PCI device DMA coherency from
>> ACPI _CCA object that should normally be specified in the DSDT node
>> of its PCI host bridge..
>> 
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> CC: Bjorn Helgaas <bhelgaas@google.com>
>> CC: Catalin Marinas <catalin.marinas@arm.com>
>> CC: Rob Herring <robh+dt@kernel.org>
>> CC: Will Deacon <will.deacon@arm.com>
>> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
>> CC: Murali Karicheri <m-karicheri2@ti.com>
>> ---
>>  drivers/of/of_pci.c    | 20 --------------------
>>  drivers/pci/probe.c    | 35 +++++++++++++++++++++++++++++++++--
>>  include/linux/of_pci.h |  3 ---
>>  3 files changed, 33 insertions(+), 25 deletions(-)
>> 
>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>> index 5751dc5..b66ee4e 100644
>> --- a/drivers/of/of_pci.c
>> +++ b/drivers/of/of_pci.c
>> @@ -117,26 +117,6 @@ int of_get_pci_domain_nr(struct device_node *node)
>>  }
>>  EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
>>  
>> -/**
>> - * of_pci_dma_configure - Setup DMA configuration
>> - * @dev: ptr to pci_dev struct of the PCI device
>> - *
>> - * Function to update PCI devices's DMA configuration using the same
>> - * info from the OF node of host bridge's parent (if any).
>> - */
>> -void of_pci_dma_configure(struct pci_dev *pci_dev)
>> -{
>> -	struct device *dev = &pci_dev->dev;
>> -	struct device *bridge = pci_get_host_bridge_device(pci_dev);
>> -
>> -	if (!bridge->parent)
>> -		return;
>> -
>> -	of_dma_configure(dev, bridge->parent->of_node);
>> -	pci_put_host_bridge_device(bridge);
>> -}
>> -EXPORT_SYMBOL_GPL(of_pci_dma_configure);
>> -
>>  #if defined(CONFIG_OF_ADDRESS)
>>  /**
>>   * of_pci_get_host_bridge_resources - Parse PCI host bridge resources
>>from DT
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 6675a7a..3c6f2e5 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -6,12 +6,14 @@
>>  #include <linux/delay.h>
>>  #include <linux/init.h>
>>  #include <linux/pci.h>
>> -#include <linux/of_pci.h>
>> +#include <linux/of_device.h>
>>  #include <linux/pci_hotplug.h>
>>  #include <linux/slab.h>
>>  #include <linux/module.h>
>>  #include <linux/cpumask.h>
>>  #include <linux/pci-aspm.h>
>> +#include <linux/acpi.h>
>> +#include <linux/property.h>
>>  #include <asm-generic/pci-bridge.h>
>>  #include "pci.h"
>>  
>> @@ -1508,6 +1510,35 @@ static void pci_init_capabilities(struct pci_dev
>>*dev)
>>  	pci_enable_acs(dev);
>>  }
>>  
>> +/**
>> + * pci_dma_configure - Setup DMA configuration
>> + * @pci_dev: ptr to pci_dev struct of the PCI device
>> + *
>> + * Function to update PCI devices's DMA configuration using the same
>> + * info from the OF node or ACPI node of host bridge's parent (if any).
>> + */
>> +static void pci_dma_configure(struct pci_dev *pci_dev)
>> +{
>> +	struct device *dev = &pci_dev->dev;
>> +	struct device *bridge = pci_get_host_bridge_device(pci_dev);
>> +	struct device *host = bridge->parent;
>> +	struct acpi_device *adev;
>> +
>> +	if (!host)
>> +		return;
>> +
>> +	if (acpi_disabled) {
>> +		of_dma_configure(dev, host->of_node);
>
>I'd rather do
>
>	if (IS_ENABLED(CONFIG_OF) && host->of_node) {
>		of_dma_configure(dev, host->of_node);
>	} else {
>		struct acpi_device *adev = ACPI_COMPANION(host);
>
>		if (adev && acpi_dma_is_supported(adev)) {

Ok, I¹ll update this.

>
>> +	} else if (has_acpi_companion(host)) {
>> +		adev = acpi_node(host->fwnode);
>> +		if (acpi_dma_is_supported(adev))
>> +			arch_setup_dma_ops(dev, 0, 0, NULL,
>> +					   acpi_dma_is_coherent(adev));
>> +	}
>> +
>> +	pci_put_host_bridge_device(bridge);
>> +}
>> +
>>  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>>  {
>>  	int ret;
>> @@ -1521,7 +1552,7 @@ void pci_device_add(struct pci_dev *dev, struct
>>pci_bus *bus)
>>  	dev->dev.dma_mask = &dev->dma_mask;
>>  	dev->dev.dma_parms = &dev->dma_parms;
>>  	dev->dev.coherent_dma_mask = 0xffffffffull;
>> -	of_pci_dma_configure(dev);
>> +	pci_dma_configure(dev);
>
>Also I'm not quite sure if that's actually OK.
>
>We should be handling the DMA setup on x86 in the ACPI case already
>without it.
>Or is this a NOP on x86?

You are correct. x86 doesn¹t implement the arch_setup_dma_ops().
Therefore, this is a NOP on x86.

Thanks,

Suravee

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Catalin Marinas]

On Sat, May 16, 2015 at 01:59:00AM +0200, Rafael J. Wysocki wrote:
> On Friday, May 15, 2015 04:23:11 PM Suravee Suthikulpanit wrote:
> > +/**
> > + * pci_dma_configure - Setup DMA configuration
> > + * @pci_dev: ptr to pci_dev struct of the PCI device
> > + *
> > + * Function to update PCI devices's DMA configuration using the same
> > + * info from the OF node or ACPI node of host bridge's parent (if any).
> > + */
> > +static void pci_dma_configure(struct pci_dev *pci_dev)
> > +{
> > +	struct device *dev = &pci_dev->dev;
> > +	struct device *bridge = pci_get_host_bridge_device(pci_dev);
> > +	struct device *host = bridge->parent;
> > +	struct acpi_device *adev;
> > +
> > +	if (!host)
> > +		return;
> > +
> > +	if (acpi_disabled) {
> > +		of_dma_configure(dev, host->of_node);
> 
> I'd rather do
> 
> 	if (IS_ENABLED(CONFIG_OF) && host->of_node) {
> 		of_dma_configure(dev, host->of_node);

Nitpick: do we need the CONFIG_OF check? If disabled, I don't think
anyone would set host->of_node.

-- 
Catalin

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Arnd Bergmann]

On Wednesday 20 May 2015 10:24:15 Catalin Marinas wrote:
> On Sat, May 16, 2015 at 01:59:00AM +0200, Rafael J. Wysocki wrote:
> > On Friday, May 15, 2015 04:23:11 PM Suravee Suthikulpanit wrote:
> > > +/**
> > > + * pci_dma_configure - Setup DMA configuration
> > > + * @pci_dev: ptr to pci_dev struct of the PCI device
> > > + *
> > > + * Function to update PCI devices's DMA configuration using the same
> > > + * info from the OF node or ACPI node of host bridge's parent (if any).
> > > + */
> > > +static void pci_dma_configure(struct pci_dev *pci_dev)
> > > +{
> > > +   struct device *dev = &pci_dev->dev;
> > > +   struct device *bridge = pci_get_host_bridge_device(pci_dev);
> > > +   struct device *host = bridge->parent;
> > > +   struct acpi_device *adev;
> > > +
> > > +   if (!host)
> > > +           return;
> > > +
> > > +   if (acpi_disabled) {
> > > +           of_dma_configure(dev, host->of_node);
> > 
> > I'd rather do
> > 
> >       if (IS_ENABLED(CONFIG_OF) && host->of_node) {
> >               of_dma_configure(dev, host->of_node);
> 
> Nitpick: do we need the CONFIG_OF check? If disabled, I don't think
> anyone would set host->of_node.
> 

If of_dma_configure() is defined in a file that is built conditionally
based on CONFIG_OF, you need it.

	Arnd

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Catalin Marinas]

On Wed, May 20, 2015 at 11:27:54AM +0200, Arnd Bergmann wrote:
> On Wednesday 20 May 2015 10:24:15 Catalin Marinas wrote:
> > On Sat, May 16, 2015 at 01:59:00AM +0200, Rafael J. Wysocki wrote:
> > > On Friday, May 15, 2015 04:23:11 PM Suravee Suthikulpanit wrote:
> > > > +/**
> > > > + * pci_dma_configure - Setup DMA configuration
> > > > + * @pci_dev: ptr to pci_dev struct of the PCI device
> > > > + *
> > > > + * Function to update PCI devices's DMA configuration using the same
> > > > + * info from the OF node or ACPI node of host bridge's parent (if any).
> > > > + */
> > > > +static void pci_dma_configure(struct pci_dev *pci_dev)
> > > > +{
> > > > +   struct device *dev = &pci_dev->dev;
> > > > +   struct device *bridge = pci_get_host_bridge_device(pci_dev);
> > > > +   struct device *host = bridge->parent;
> > > > +   struct acpi_device *adev;
> > > > +
> > > > +   if (!host)
> > > > +           return;
> > > > +
> > > > +   if (acpi_disabled) {
> > > > +           of_dma_configure(dev, host->of_node);
> > > 
> > > I'd rather do
> > > 
> > >       if (IS_ENABLED(CONFIG_OF) && host->of_node) {
> > >               of_dma_configure(dev, host->of_node);
> > 
> > Nitpick: do we need the CONFIG_OF check? If disabled, I don't think
> > anyone would set host->of_node.
> 
> If of_dma_configure() is defined in a file that is built conditionally
> based on CONFIG_OF, you need it.

We have a dummy of_dma_configure() already when !CONFIG_OF, otherwise
we would need #ifndef here. I already replied, I think for other
architectures we need this check to avoid a useless host->of_node test.

-- 
Catalin

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Suravee Suthikulanit]

On 5/20/2015 4:34 AM, Catalin Marinas wrote:
> On Wed, May 20, 2015 at 11:27:54AM +0200, Arnd Bergmann wrote:
>> On Wednesday 20 May 2015 10:24:15 Catalin Marinas wrote:
>>> On Sat, May 16, 2015 at 01:59:00AM +0200, Rafael J. Wysocki wrote:
>>>> On Friday, May 15, 2015 04:23:11 PM Suravee Suthikulpanit wrote:
>>>>> +/**
>>>>> + * pci_dma_configure - Setup DMA configuration
>>>>> + * @pci_dev: ptr to pci_dev struct of the PCI device
>>>>> + *
>>>>> + * Function to update PCI devices's DMA configuration using the same
>>>>> + * info from the OF node or ACPI node of host bridge's parent (if any).
>>>>> + */
>>>>> +static void pci_dma_configure(struct pci_dev *pci_dev)
>>>>> +{
>>>>> +   struct device *dev = &pci_dev->dev;
>>>>> +   struct device *bridge = pci_get_host_bridge_device(pci_dev);
>>>>> +   struct device *host = bridge->parent;
>>>>> +   struct acpi_device *adev;
>>>>> +
>>>>> +   if (!host)
>>>>> +           return;
>>>>> +
>>>>> +   if (acpi_disabled) {
>>>>> +           of_dma_configure(dev, host->of_node);
>>>>
>>>> I'd rather do
>>>>
>>>>        if (IS_ENABLED(CONFIG_OF) && host->of_node) {
>>>>                of_dma_configure(dev, host->of_node);
>>>
>>> Nitpick: do we need the CONFIG_OF check? If disabled, I don't think
>>> anyone would set host->of_node.
>>
>> If of_dma_configure() is defined in a file that is built conditionally
>> based on CONFIG_OF, you need it.
>
> We have a dummy of_dma_configure() already when !CONFIG_OF, otherwise
> we would need #ifndef here. I already replied, I think for other
> architectures we need this check to avoid a useless host->of_node test.
>

It seems that there are several places that have similar check. Would it 
be good to convert this into a macro? Something like:

#define OF_NODE_ENABLED(dev)	(IS_ENABLED(CONFIG_OF) && dev->of_node)

Thanks all for the review feedback.

Suravee

Re: [Linaro-acpi] [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Arnd Bergmann]

On Wednesday 20 May 2015 07:00:25 Suravee Suthikulanit wrote:
> It seems that there are several places that have similar check. Would it 
> be good to convert this into a macro? Something like:
> 
> #define OF_NODE_ENABLED(dev)    (IS_ENABLED(CONFIG_OF) && dev->of_node)
> 
> Thanks all for the review feedback.
> 

Better make that an inline function that returns the node:

struct device_node *dev_of_node(struct device *)
{
	if (IS_ENABLED(CONFIG_OF))
		return dev->of_node;

	return NULL;
}

	Arnd

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Russell King - ARM Linux]

On Wed, May 20, 2015 at 07:00:25AM -0500, Suravee Suthikulanit wrote:
> On 5/20/2015 4:34 AM, Catalin Marinas wrote:
> >We have a dummy of_dma_configure() already when !CONFIG_OF, otherwise
> >we would need #ifndef here. I already replied, I think for other
> >architectures we need this check to avoid a useless host->of_node test.
> 
> It seems that there are several places that have similar check. Would it be
> good to convert this into a macro? Something like:
> 
> #define OF_NODE_ENABLED(dev)	(IS_ENABLED(CONFIG_OF) && dev->of_node)

This /could/ be a useful compile-time optimisation: when CONFIG_OF is
disabled, dev->of_node exists but will always be NULL - but the
compiler doesn't know this.  Your suggestion above would tell the
compiler that when CONFIG_OF is disabled, OF_NODE_ENABLED() will
evaluate to a constant false, which means it can eliminate code.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Catalin Marinas]

On Wed, May 20, 2015 at 10:24:15AM +0100, Catalin Marinas wrote:
> On Sat, May 16, 2015 at 01:59:00AM +0200, Rafael J. Wysocki wrote:
> > On Friday, May 15, 2015 04:23:11 PM Suravee Suthikulpanit wrote:
> > > +/**
> > > + * pci_dma_configure - Setup DMA configuration
> > > + * @pci_dev: ptr to pci_dev struct of the PCI device
> > > + *
> > > + * Function to update PCI devices's DMA configuration using the same
> > > + * info from the OF node or ACPI node of host bridge's parent (if any).
> > > + */
> > > +static void pci_dma_configure(struct pci_dev *pci_dev)
> > > +{
> > > +	struct device *dev = &pci_dev->dev;
> > > +	struct device *bridge = pci_get_host_bridge_device(pci_dev);
> > > +	struct device *host = bridge->parent;
> > > +	struct acpi_device *adev;
> > > +
> > > +	if (!host)
> > > +		return;
> > > +
> > > +	if (acpi_disabled) {
> > > +		of_dma_configure(dev, host->of_node);
> > 
> > I'd rather do
> > 
> > 	if (IS_ENABLED(CONFIG_OF) && host->of_node) {
> > 		of_dma_configure(dev, host->of_node);
> 
> Nitpick: do we need the CONFIG_OF check? If disabled, I don't think
> anyone would set host->of_node.

Please ignore this, there is no point in checking host->of_node if
CONFIG_OF is disabled (I was just thinking from an arm64 perspective
where we always have CONFIG_OF enabled).

-- 
Catalin

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Bjorn Helgaas]

On Fri, May 15, 2015 at 4:23 PM, Suravee Suthikulpanit
<Suravee.Suthikulpanit@amd.com> wrote:
> This patch refactors of_pci_dma_configure() into a more generic
> pci_dma_configure(), which can be reused by non-OF code.
> Then, it adds support for setting up PCI device DMA coherency from
> ACPI _CCA object that should normally be specified in the DSDT node
> of its PCI host bridge..

>From the changelog, it sounds like you might be able to split this
into two patches:

1) Refactor of_pci_dma_configure(), with no functional change
2) Add support for using _CCA

If that's possible, please do so to make this easier to review.

Bjorn

Re: [V4 PATCH 3/6] pci: Generic function for setting up PCI device DMA coherency

[Suthikulpanit, Suravee]

Hi Bjorn,

On 5/16/15, 07:41, "Bjorn Helgaas" <bhelgaas@google.com> wrote:

>On Fri, May 15, 2015 at 4:23 PM, Suravee Suthikulpanit
><Suravee.Suthikulpanit@amd.com> wrote:
>> This patch refactors of_pci_dma_configure() into a more generic
>> pci_dma_configure(), which can be reused by non-OF code.
>> Then, it adds support for setting up PCI device DMA coherency from
>> ACPI _CCA object that should normally be specified in the DSDT node
>> of its PCI host bridge..
>
>From the changelog, it sounds like you might be able to split this
>into two patches:
>
>1) Refactor of_pci_dma_configure(), with no functional change
>2) Add support for using _CCA
>
>If that's possible, please do so to make this easier to review.
>
>Bjorn

Yes, I can separate the patch out. This would mainly affect ACPI+PCI
support for ARM64 (which is in progress).

Thanks,

Surafvee

[V4 PATCH 4/6] device property: Introduces device_dma_is_coherent()

[Suravee Suthikulpanit]

Currently, device drivers, which support both OF and ACPI,
need to call two separate APIs, of_dma_is_coherent() and
acpi_dma_is_coherent()) to determine device coherency attribute.

This patch simplifies this process by introducing a new device
property API, device_dma_is_coherent(), which calls the appropriate
interface based on the booting architecture.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Rafael J. Wysocki <rjw@rjwysocki.net>
---
 drivers/base/property.c  | 12 ++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 1d0b116..8123c6e 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -14,6 +14,7 @@
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/property.h>
 
 /**
@@ -519,3 +520,14 @@ unsigned int device_get_child_node_count(struct device *dev)
 	return count;
 }
 EXPORT_SYMBOL_GPL(device_get_child_node_count);
+
+bool device_dma_is_coherent(struct device *dev)
+{
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+		return of_dma_is_coherent(dev->of_node);
+	else if (has_acpi_companion(dev))
+		return acpi_dma_is_coherent(acpi_node(dev->fwnode));
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(device_dma_is_coherent);
diff --git a/include/linux/property.h b/include/linux/property.h
index de8bdf4..76ebde9 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -164,4 +164,6 @@ struct property_set {
 
 void device_add_property_set(struct device *dev, struct property_set *pset);
 
+bool device_dma_is_coherent(struct device *dev);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.1.0

Re: [V4 PATCH 4/6] device property: Introduces device_dma_is_coherent()

[Will Deacon]

On Fri, May 15, 2015 at 10:23:12PM +0100, Suravee Suthikulpanit wrote:
> Currently, device drivers, which support both OF and ACPI,
> need to call two separate APIs, of_dma_is_coherent() and
> acpi_dma_is_coherent()) to determine device coherency attribute.
> 
> This patch simplifies this process by introducing a new device
> property API, device_dma_is_coherent(), which calls the appropriate
> interface based on the booting architecture.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
> ---
>  drivers/base/property.c  | 12 ++++++++++++
>  include/linux/property.h |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 1d0b116..8123c6e 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -14,6 +14,7 @@
>  #include <linux/export.h>
>  #include <linux/kernel.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/property.h>
>  
>  /**
> @@ -519,3 +520,14 @@ unsigned int device_get_child_node_count(struct device *dev)
>  	return count;
>  }
>  EXPORT_SYMBOL_GPL(device_get_child_node_count);
> +
> +bool device_dma_is_coherent(struct device *dev)
> +{
> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> +		return of_dma_is_coherent(dev->of_node);
> +	else if (has_acpi_companion(dev))
> +		return acpi_dma_is_coherent(acpi_node(dev->fwnode));

I don't think you need the has_acpi_companion check, as acpi_node handles
this and acpi_dma_is_coherent(NULL) returns false.

Will

Re: [V4 PATCH 4/6] device property: Introduces device_dma_is_coherent()

[Suravee Suthikulanit]

On 5/20/2015 5:28 AM, Will Deacon wrote:
> On Fri, May 15, 2015 at 10:23:12PM +0100, Suravee Suthikulpanit wrote:
>> Currently, device drivers, which support both OF and ACPI,
>> need to call two separate APIs, of_dma_is_coherent() and
>> acpi_dma_is_coherent()) to determine device coherency attribute.
>>
>> This patch simplifies this process by introducing a new device
>> property API, device_dma_is_coherent(), which calls the appropriate
>> interface based on the booting architecture.
>>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
>> ---
>>   drivers/base/property.c  | 12 ++++++++++++
>>   include/linux/property.h |  2 ++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index 1d0b116..8123c6e 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/export.h>
>>   #include <linux/kernel.h>
>>   #include <linux/of.h>
>> +#include <linux/of_address.h>
>>   #include <linux/property.h>
>>
>>   /**
>> @@ -519,3 +520,14 @@ unsigned int device_get_child_node_count(struct device *dev)
>>   	return count;
>>   }
>>   EXPORT_SYMBOL_GPL(device_get_child_node_count);
>> +
>> +bool device_dma_is_coherent(struct device *dev)
>> +{
>> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>> +		return of_dma_is_coherent(dev->of_node);
>> +	else if (has_acpi_companion(dev))
>> +		return acpi_dma_is_coherent(acpi_node(dev->fwnode));
>
> I don't think you need the has_acpi_companion check, as acpi_node handles
> this and acpi_dma_is_coherent(NULL) returns false.
>
> Will
>
You are right.

Thanks,

Suravee

[V4 PATCH 5/6] crypto: ccp - Unify coherency checking logic with device_dma_is_coherent()

[Suravee Suthikulpanit]

Currently, the driver has separate logic to determine device coherency
for DT vs ACPI.  This patch simplifies the code with a call to
device_dma_is_coherent().

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
---
 drivers/crypto/ccp/ccp-platform.c | 60 +--------------------------------------
 1 file changed, 1 insertion(+), 59 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-platform.c b/drivers/crypto/ccp/ccp-platform.c
index b1c20b2..e446781 100644
--- a/drivers/crypto/ccp/ccp-platform.c
+++ b/drivers/crypto/ccp/ccp-platform.c
@@ -90,58 +90,6 @@ static struct resource *ccp_find_mmio_area(struct ccp_device *ccp)
 	return NULL;
 }
 
-#ifdef CONFIG_ACPI
-static int ccp_acpi_support(struct ccp_device *ccp)
-{
-	struct ccp_platform *ccp_platform = ccp->dev_specific;
-	struct acpi_device *adev = ACPI_COMPANION(ccp->dev);
-	acpi_handle handle;
-	acpi_status status;
-	unsigned long long data;
-	int cca;
-
-	/* Retrieve the device cache coherency value */
-	handle = adev->handle;
-	do {
-		status = acpi_evaluate_integer(handle, "_CCA", NULL, &data);
-		if (!ACPI_FAILURE(status)) {
-			cca = data;
-			break;
-		}
-	} while (!ACPI_FAILURE(status));
-
-	if (ACPI_FAILURE(status)) {
-		dev_err(ccp->dev, "error obtaining acpi coherency value\n");
-		return -EINVAL;
-	}
-
-	ccp_platform->coherent = !!cca;
-
-	return 0;
-}
-#else	/* CONFIG_ACPI */
-static int ccp_acpi_support(struct ccp_device *ccp)
-{
-	return -EINVAL;
-}
-#endif
-
-#ifdef CONFIG_OF
-static int ccp_of_support(struct ccp_device *ccp)
-{
-	struct ccp_platform *ccp_platform = ccp->dev_specific;
-
-	ccp_platform->coherent = of_dma_is_coherent(ccp->dev->of_node);
-
-	return 0;
-}
-#else
-static int ccp_of_support(struct ccp_device *ccp)
-{
-	return -EINVAL;
-}
-#endif
-
 static int ccp_platform_probe(struct platform_device *pdev)
 {
 	struct ccp_device *ccp;
@@ -182,13 +130,7 @@ static int ccp_platform_probe(struct platform_device *pdev)
 		goto e_err;
 	}
 
-	if (ccp_platform->use_acpi)
-		ret = ccp_acpi_support(ccp);
-	else
-		ret = ccp_of_support(ccp);
-	if (ret)
-		goto e_err;
-
+	ccp_platform->coherent = device_dma_is_coherent(ccp->dev);
 	if (ccp_platform->coherent)
 		ccp->axcache = CACHE_WB_NO_ALLOC;
 	else
-- 
2.1.0

[V4 PATCH 6/6] amd-xgbe: Unify coherency checking logic with device_dma_is_coherent()

[Suravee Suthikulpanit]

Currently, amd-xgbe driver has separate logic to determine device
coherency for DT vs. ACPI. This patch simplifies the code with
a call to device_dma_is_coherent().

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: David S. Miller <davem@davemloft.net>
---
 drivers/net/ethernet/amd/xgbe/xgbe-main.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-main.c b/drivers/net/ethernet/amd/xgbe/xgbe-main.c
index 7149053..6d2c702 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-main.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-main.c
@@ -168,13 +168,8 @@ static void xgbe_init_all_fptrs(struct xgbe_prv_data *pdata)
 #ifdef CONFIG_ACPI
 static int xgbe_acpi_support(struct xgbe_prv_data *pdata)
 {
-	struct acpi_device *adev = pdata->adev;
 	struct device *dev = pdata->dev;
 	u32 property;
-	acpi_handle handle;
-	acpi_status status;
-	unsigned long long data;
-	int cca;
 	int ret;
 
 	/* Obtain the system clock setting */
@@ -195,24 +190,6 @@ static int xgbe_acpi_support(struct xgbe_prv_data *pdata)
 	}
 	pdata->ptpclk_rate = property;
 
-	/* Retrieve the device cache coherency value */
-	handle = adev->handle;
-	do {
-		status = acpi_evaluate_integer(handle, "_CCA", NULL, &data);
-		if (!ACPI_FAILURE(status)) {
-			cca = data;
-			break;
-		}
-
-		status = acpi_get_parent(handle, &handle);
-	} while (!ACPI_FAILURE(status));
-
-	if (ACPI_FAILURE(status)) {
-		dev_err(dev, "error obtaining acpi coherency value\n");
-		return -EINVAL;
-	}
-	pdata->coherent = !!cca;
-
 	return 0;
 }
 #else   /* CONFIG_ACPI */
@@ -243,9 +220,6 @@ static int xgbe_of_support(struct xgbe_prv_data *pdata)
 	}
 	pdata->ptpclk_rate = clk_get_rate(pdata->ptpclk);
 
-	/* Retrieve the device cache coherency value */
-	pdata->coherent = of_dma_is_coherent(dev->of_node);
-
 	return 0;
 }
 #else   /* CONFIG_OF */
@@ -364,6 +338,7 @@ static int xgbe_probe(struct platform_device *pdev)
 		goto err_io;
 
 	/* Set the DMA coherency values */
+	pdata->coherent = device_dma_is_coherent(pdata->dev);
 	if (pdata->coherent) {
 		pdata->axdomain = XGBE_DMA_OS_AXDOMAIN;
 		pdata->arcache = XGBE_DMA_OS_ARCACHE;
-- 
2.1.0