Hi Bjorn,

On 8/25/15 00:32, Bjorn Helgaas wrote:

Here it is again.

On Thu, Aug 13, 2015 at 6:50 PM, Bjorn Helgaas [off-list ref] wrote:

quoted

Hi Suravee,

On Thu, Aug 13, 2015 at 04:58:45PM +0700, Suravee Suthikulpanit wrote:

quoted

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

Since this does two things:
   1) Rename of_pci_dma_configure() and move it to PCI
   2) Add _CCA support,
maybe it should be split into two patches?

Sure, I can take care of that and separate them into two patches.

quoted

There are a couple more comments below.

While looking at this, I thought some of the existing code could be
made simpler and easier to follow.  I appended a couple possible patches;
you can incorporate them or ignore them, whatever seems best to you.

Bjorn

Please see my response below.

quoted

[...]

quoted

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..e2fcd3b 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"

@@ -1544,6 +1546,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)

Almost all pci_dev pointers in probe.c are named "dev", so I would use
that for this one, too.  I probably would just drop the "struct device
*dev" below and use "&dev->dev" the two places you need it.  That's a
common idiom in PCI.

I'll take care of this.

quoted

quoted

+{
+     struct device *dev = &pci_dev->dev;
+     struct device *bridge = pci_get_host_bridge_device(pci_dev);
+     struct acpi_device *adev;
+     bool coherent;
+
+     if (has_acpi_companion(bridge)) {
+             adev = to_acpi_node(bridge->fwnode);
+             if (acpi_check_dma(adev, &coherent))
+                     arch_setup_dma_ops(dev, 0, 0, NULL, coherent);
+     } else {
+             struct device *host = bridge->parent;
+             if (!host)
+                     return;
+
+             of_dma_configure(dev, host->of_node);
+     }

Why is this check reversed with respect to device_dma_is_coherent()?
In device_dma_is_coherent(), we first look for an OF property, then look
for ACPI _CCA.  But here we check for _CCA, then for OF.

I was trying to save some additional logic. But, think again I should 
not have done so. I'll fix this.

quoted

[...]
commit 18183957888f601807ca0e166516ae60f682eb62
Author: Bjorn Helgaas [off-list ref]
Date:   Thu Aug 13 20:01:21 2015 -0500

     ACPI / scan: Move _CCA comments to acpi_init_coherency()

     Move the comments about how we handle _CCA to the code that looks at _CCA,
     and fix a couple typos.

     No functional change.

     Signed-off-by: Bjorn Helgaas [off-list ref]

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index ec25635..a12e522 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c

@@ -2188,6 +2188,22 @@ static void acpi_init_coherency(struct acpi_device *adev)
         acpi_status status;
         struct acpi_device *parent = adev->parent;

+       /**
+        * Currently, we only support _CCA=1 (i.e. coherent_dma=1)
+        * This should be equivalent to specifying dma-coherent for
+        * a device in OF.
+        *
+        * For the case when _CCA=0 (i.e. coherent_dma=0 && cca_seen=1),
+        * we have two choices:
+        * case 1. Do not support and disable DMA.
+        * case 2. Support but rely on arch-specific cache maintenance for
+        *         non-coherent DMA operations.
+        * Currently, we implement case 1 above.
+        *
+        * For the case when _CCA is missing (i.e. cca_seen=0) and
+        * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
+        * and fallback to arch-specific default handling.
+        */
         if (parent && parent->flags.cca_seen) {
                 /*
                  * From ACPI spec, OSPM will ignore _CCA if an ancestor

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 83061ca..718942b 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h

@@ -389,24 +389,6 @@ static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
         if (!adev)
                 return ret;

-       /**
-        * Currently, we only support _CCA=1 (i.e. coherent_dma=1)
-        * This should be equivalent to specifyig dma-coherent for
-        * a device in OF.
-        *
-        * For the case when _CCA=0 (i.e. coherent_dma=0 && cca_seen=1),
-        * There are two cases:
-        * case 1. Do not support and disable DMA.
-        * case 2. Support but rely on arch-specific cache maintenance for
-        *         non-coherence DMA operations.
-        * Currently, we implement case 1 above.
-        *
-        * For the case when _CCA is missing (i.e. cca_seen=0) and
-        * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
-        * and fallback to arch-specific default handling.
-        *
-        * See acpi_init_coherency() for more info.
-        */
         if (adev->flags.coherent_dma) {
                 ret = true;
                 if (coherent)

Actually, the reason I put the comment in the acpi_check_dma() is 
because the logic for determining the coherency is in this function, 
which is separate from the CCA parsing/detection logic.

I can probably just get rid of the inline and move the whole function 
into /driver/acpi/scan.c to make it more readable, and fix the typos.

quoted

commit 84cfb2213cd400fef227ec0d7829ec4e12895da9
Author: Bjorn Helgaas [off-list ref]
Date:   Thu Aug 13 19:49:52 2015 -0500

     ACPI / scan: Rename acpi_check_dma() to acpi_dma_is_coherent()

     The name "acpi_check_dma()" doesn't give any much indication about what
     exactly it checks.  The function also returns information both as a normal
     return value and as the "bool *coherent" return parameter.  But "*coherent"
     doesn't actually give any extra information: it is unchanged when returning
     false and set to true when returning true.

     Rename acpi_check_dma() to acpi_dma_is_coherent() so the callers read more
     naturally.  Drop the return parameter and just use the function return
     value.

     Signed-off-by: Bjorn Helgaas [off-list ref]

This was because, at one point, we wanted to be able to differentiate 
between the case _CCA=0 and missing _CCA in ARM64, where we would 
support DMA (using arch-specific cache maintenance) if _CCA=0, and 
disable DMA when missing _CCA on ARM64.

It seems like the logic is now required (please see 
https://www.mail-archive.com/linux-usb at vger.kernel.org/msg62735.html). 
So, we would need the true/false return, and the coherent variable to be 
able to differentiate between the two cases.

Please let me know what you think.

Thanks,
Suravee