[PATCH V10 03/12] of: dma: Move range size workaround to of_dma_get_range()

[Sricharan R]

From: Laurent Pinchart 

Invalid dma-ranges values should be worked around when retrieving the
DMA range in of_dma_get_range(), not by all callers of the function.
This isn't much of a problem now that we have a single caller, but that
situation will change when moving DMA configuration to device probe
time.

Tested-by: Marek Szyprowski 
Signed-off-by: Laurent Pinchart 
---
 drivers/of/address.c | 20 ++--
 drivers/of/device.c  | 15 ---
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903..6aeb816 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -819,8 +819,8 @@ void __iomem *of_io_request_and_map(struct device_node *np, 
int index,
  * CPU addr (phys_addr_t)  : pna cells
  * size: nsize cells
  *
- * It returns -ENODEV if "dma-ranges" property was not found
- * for this device in DT.
+ * Return 0 on success, -ENODEV if the "dma-ranges" property was not found for
+ * this device in DT, or -EINVAL if the CPU address or size is invalid.
  */
 int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 
*size)
 {
@@ -880,6 +880,22 @@ int of_dma_get_range(struct device_node *np, u64 
*dma_addr, u64 *paddr, u64 *siz
*dma_addr = dmaaddr;
 
*size = of_read_number(ranges + naddr + pna, nsize);
+   /*
+* DT nodes sometimes incorrectly set the size as a mask. Work around
+* those incorrect DT by computing the size as mask + 1.
+*/
+   if (*size & 1) {
+   pr_warn("%s: size 0x%llx for dma-range in node(%s) set as 
mask\n",
+   __func__, *size, np->full_name);
+   *size = *size + 1;
+   }
+
+   if (!*size) {
+   pr_err("%s: invalid size zero for dma-range in node(%s)\n",
+  __func__, np->full_name);
+   ret = -EINVAL;
+   goto out;
+   }
 
pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
 *dma_addr, *paddr, *size);
diff --git a/drivers/of/device.c b/drivers/of/device.c
index b1e6beb..09dedd0 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -110,21 +110,6 @@ void of_dma_configure(struct device *dev, struct 
device_node *np)
size = dev->coherent_dma_mask + 1;
} else {
offset = PFN_DOWN(paddr - dma_addr);
-
-   /*
-* Add a work around to treat the size as mask + 1 in case
-* it is defined in DT as a mask.
-*/
-   if (size & 1) {
-   dev_warn(dev, "Invalid size 0x%llx for dma-range\n",
-size);
-   size = size + 1;
-   }
-
-   if (!size) {
-   dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
-   return;
-   }
dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
}
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V10 02/12] iommu/of: Prepare for deferred IOMMU configuration

[Sricharan R]

From: Robin Murphy 

IOMMU configuration represents unchanging properties of the hardware,
and as such should only need happen once in a device's lifetime, but
the necessary interaction with the IOMMU device and driver complicates
exactly when that point should be.

Since the only reasonable tool available for handling the inter-device
dependency is probe deferral, we need to prepare of_iommu_configure()
to run later than it is currently called (i.e. at driver probe rather
than device creation), to handle being retried, and to tell whether a
not-yet present IOMMU should be waited for or skipped (by virtue of
having declared a built-in driver or not).

Tested-by: Marek Szyprowski 
Signed-off-by: Robin Murphy 
---
 drivers/iommu/of_iommu.c | 43 ++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 8f4e599..c8be889 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -96,6 +96,19 @@ int of_get_dma_window(struct device_node *dn, const char 
*prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+static bool of_iommu_driver_present(struct device_node *np)
+{
+   /*
+* If the IOMMU still isn't ready by the time we reach init, assume
+* it never will be. We don't want to defer indefinitely, nor attempt
+* to dereference __iommu_of_table after it's been freed.
+*/
+   if (system_state > SYSTEM_BOOTING)
+   return false;
+
+   return of_match_node(&__iommu_of_table, np);
+}
+
 static const struct iommu_ops
 *of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec)
 {
@@ -104,12 +117,20 @@ int of_get_dma_window(struct device_node *dn, const char 
*prefix, int index,
int err;
 
ops = iommu_ops_from_fwnode(fwnode);
-   if (!ops || !ops->of_xlate)
+   if ((ops && !ops->of_xlate) ||
+   (!ops && !of_iommu_driver_present(iommu_spec->np)))
return NULL;
 
err = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops);
if (err)
return ERR_PTR(err);
+   /*
+* The otherwise-empty fwspec handily serves to indicate the specific
+* IOMMU device we're waiting for, which will be useful if we ever get
+* a proper probe-ordering dependency mechanism in future.
+*/
+   if (!ops)
+   return ERR_PTR(-EPROBE_DEFER);
 
err = ops->of_xlate(dev, iommu_spec);
if (err)
@@ -186,14 +207,34 @@ const struct iommu_ops *of_iommu_configure(struct device 
*dev,
   struct device_node *master_np)
 {
const struct iommu_ops *ops;
+   struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 
if (!master_np)
return NULL;
 
+   if (fwspec) {
+   if (fwspec->ops)
+   return fwspec->ops;
+
+   /* In the deferred case, start again from scratch */
+   iommu_fwspec_free(dev);
+   }
+
if (dev_is_pci(dev))
ops = of_pci_iommu_init(to_pci_dev(dev), master_np);
else
ops = of_platform_iommu_init(dev, master_np);
+   /*
+* If we have reason to believe the IOMMU driver missed the initial
+* add_device callback for dev, replay it to get things in order.
+*/
+   if (!IS_ERR_OR_NULL(ops) && ops->add_device &&
+   dev->bus && !dev->iommu_group) {
+   int err = ops->add_device(dev);
+
+   if (err)
+   ops = ERR_PTR(err);
+   }
 
return IS_ERR(ops) ? NULL : ops;
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V10 00/12] IOMMU probe deferral support

[Sricharan R]

with deferred probing or error

Lorenzo Pieralisi (2):
  ACPI/IORT: Add function to check SMMUs drivers presence
  ACPI/IORT: Remove linker section for IORT entries probing

Robin Murphy (3):
  iommu/of: Refactor of_iommu_configure() for error handling
  iommu/of: Prepare for deferred IOMMU configuration
  iommu/arm-smmu: Clean up early-probing workarounds

Sricharan R (4):
  of: device: Fix overflow of coherent_dma_mask
  of/acpi: Configure dma operations at probe time for platform/amba/pci
bus devices
  drivers: acpi: Handle IOMMU lookup failure with deferred probing or
error
  arm64: dma-mapping: Remove the notifier trick to handle early setting
of dma_ops

 arch/arm64/mm/dma-mapping.c   | 142 +-
 drivers/acpi/arm64/iort.c |  48 -
 drivers/acpi/glue.c   |   5 --
 drivers/acpi/scan.c   |  11 ++-
 drivers/base/dd.c |   9 +++
 drivers/base/dma-mapping.c|  41 +++
 drivers/iommu/arm-smmu-v3.c   |  46 +---
 drivers/iommu/arm-smmu.c  | 110 +
 drivers/iommu/of_iommu.c  | 126 -
 drivers/of/address.c  |  20 +-
 drivers/of/device.c   |  36 +-
 drivers/of/platform.c |  10 +--
 drivers/pci/probe.c   |  28 
 include/acpi/acpi_bus.h   |   2 +-
 include/asm-generic/vmlinux.lds.h |   1 -
 include/linux/acpi.h  |   7 +-
 include/linux/acpi_iort.h |   3 -
 include/linux/dma-mapping.h   |  12 
 include/linux/of_device.h |  10 ++-
 19 files changed, 329 insertions(+), 338 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V10 08/12] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Sricharan R]

From: Laurent Pinchart 

Failures to look up an IOMMU when parsing the DT iommus property need to
be handled separately from the .of_xlate() failures to support deferred
probing.

The lack of a registered IOMMU can be caused by the lack of a driver for
the IOMMU, the IOMMU device probe not having been performed yet, having
been deferred, or having failed.

The first case occurs when the device tree describes the bus master and
IOMMU topology correctly but no device driver exists for the IOMMU yet
or the device driver has not been compiled in. Return NULL, the caller
will configure the device without an IOMMU.

The second and third cases are handled by deferring the probe of the bus
master device which will eventually get reprobed after the IOMMU.

The last case is currently handled by deferring the probe of the bus
master device as well. A mechanism to either configure the bus master
device without an IOMMU or to fail the bus master device probe depending
on whether the IOMMU is optional or mandatory would be a good
enhancement.

Tested-by: Marek Szyprowski 
Signed-off-by: Laurent Pichart 
Signed-off-by: Sricharan R 
---
 drivers/base/dma-mapping.c | 5 +++--
 drivers/iommu/of_iommu.c   | 4 ++--
 drivers/of/device.c| 7 ++-
 include/linux/of_device.h  | 9 ++---
 4 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index 449b948..82bd45c 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -353,6 +353,7 @@ int dma_configure(struct device *dev)
 {
struct device *bridge = NULL, *dma_dev = dev;
enum dev_dma_attr attr;
+   int ret = 0;
 
if (dev_is_pci(dev)) {
bridge = pci_get_host_bridge_device(to_pci_dev(dev));
@@ -363,7 +364,7 @@ int dma_configure(struct device *dev)
}
 
if (dma_dev->of_node) {
-   of_dma_configure(dev, dma_dev->of_node);
+   ret = of_dma_configure(dev, dma_dev->of_node);
} else if (has_acpi_companion(dma_dev)) {
attr = acpi_get_dma_attr(to_acpi_device_node(dma_dev->fwnode));
if (attr != DEV_DMA_NOT_SUPPORTED)
@@ -373,7 +374,7 @@ int dma_configure(struct device *dev)
if (bridge)
pci_put_host_bridge_device(bridge);
 
-   return 0;
+   return ret;
 }
 
 void dma_deconfigure(struct device *dev)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index c8be889..9f44ee8 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -236,7 +236,7 @@ const struct iommu_ops *of_iommu_configure(struct device 
*dev,
ops = ERR_PTR(err);
}
 
-   return IS_ERR(ops) ? NULL : ops;
+   return ops;
 }
 
 static int __init of_iommu_init(void)
@@ -247,7 +247,7 @@ static int __init of_iommu_init(void)
for_each_matching_node_and_match(np, matches, &match) {
const of_iommu_init_fn init_fn = match->data;
 
-   if (init_fn(np))
+   if (init_fn && init_fn(np))
pr_err("Failed to initialise IOMMU %s\n",
of_node_full_name(np));
}
diff --git a/drivers/of/device.c b/drivers/of/device.c
index c2ae6bb..bea8aec 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -82,7 +82,7 @@ int of_device_add(struct platform_device *ofdev)
  * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events
  * to fix up DMA configuration.
  */
-void of_dma_configure(struct device *dev, struct device_node *np)
+int of_dma_configure(struct device *dev, struct device_node *np)
 {
u64 dma_addr, paddr, size;
int ret;
@@ -129,10 +129,15 @@ void of_dma_configure(struct device *dev, struct 
device_node *np)
coherent ? " " : " not ");
 
iommu = of_iommu_configure(dev, np);
+   if (IS_ERR(iommu))
+   return PTR_ERR(iommu);
+
dev_dbg(dev, "device is%sbehind an iommu\n",
iommu ? " " : " not ");
 
arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+
+   return 0;
 }
 EXPORT_SYMBOL_GPL(of_dma_configure);
 
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index af98455..2cacdd8 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -55,7 +55,7 @@ static inline struct device_node *of_cpu_device_node_get(int 
cpu)
return of_node_get(cpu_dev->of_node);
 }
 
-void of_dma_configure(struct device *dev, struct device_node *np);
+int of_dma_configure(struct device *dev, struct device_node *np);
 void of_dma_deconfigure(struct device *dev);
 #else /* CONFIG_OF */
 
@@ -104,8 +104,11 @@ static inline struct device_node 
*of_cpu_device_node_get(int cpu)
 {
return NULL;
 }
-static inline void of_dma_configure(struct device *dev, struct device_node *np)
-{}
+
+static inline int of

[PATCH V10 01/12] iommu/of: Refactor of_iommu_configure() for error handling

[Sricharan R]

From: Robin Murphy 

In preparation for some upcoming cleverness, rework the control flow in
of_iommu_configure() to minimise duplication and improve the propogation
of errors. It's also as good a time as any to switch over from the
now-just-a-compatibility-wrapper of_iommu_get_ops() to using the generic
IOMMU instance interface directly.

Tested-by: Marek Szyprowski 
Signed-off-by: Robin Murphy 
---
 drivers/iommu/of_iommu.c | 83 +++-
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 2683e9f..8f4e599 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -96,6 +96,28 @@ int of_get_dma_window(struct device_node *dn, const char 
*prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+static const struct iommu_ops
+*of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec)
+{
+   const struct iommu_ops *ops;
+   struct fwnode_handle *fwnode = &iommu_spec->np->fwnode;
+   int err;
+
+   ops = iommu_ops_from_fwnode(fwnode);
+   if (!ops || !ops->of_xlate)
+   return NULL;
+
+   err = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops);
+   if (err)
+   return ERR_PTR(err);
+
+   err = ops->of_xlate(dev, iommu_spec);
+   if (err)
+   return ERR_PTR(err);
+
+   return ops;
+}
+
 static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
 {
struct of_phandle_args *iommu_spec = data;
@@ -105,10 +127,11 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 alias, 
void *data)
 }
 
 static const struct iommu_ops
-*of_pci_iommu_configure(struct pci_dev *pdev, struct device_node *bridge_np)
+*of_pci_iommu_init(struct pci_dev *pdev, struct device_node *bridge_np)
 {
const struct iommu_ops *ops;
struct of_phandle_args iommu_spec;
+   int err;
 
/*
 * Start by tracing the RID alias down the PCI topology as
@@ -123,56 +146,56 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 alias, 
void *data)
 * bus into the system beyond, and which IOMMU it ends up at.
 */
iommu_spec.np = NULL;
-   if (of_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
-  "iommu-map-mask", &iommu_spec.np, iommu_spec.args))
-   return NULL;
+   err = of_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
+"iommu-map-mask", &iommu_spec.np,
+iommu_spec.args);
+   if (err)
+   return err == -ENODEV ? NULL : ERR_PTR(err);
 
-   ops = iommu_ops_from_fwnode(&iommu_spec.np->fwnode);
-   if (!ops || !ops->of_xlate ||
-   iommu_fwspec_init(&pdev->dev, &iommu_spec.np->fwnode, ops) ||
-   ops->of_xlate(&pdev->dev, &iommu_spec))
-   ops = NULL;
+   ops = of_iommu_xlate(&pdev->dev, &iommu_spec);
 
of_node_put(iommu_spec.np);
return ops;
 }
 
-const struct iommu_ops *of_iommu_configure(struct device *dev,
-  struct device_node *master_np)
+static const struct iommu_ops
+*of_platform_iommu_init(struct device *dev, struct device_node *np)
 {
struct of_phandle_args iommu_spec;
-   struct device_node *np;
const struct iommu_ops *ops = NULL;
int idx = 0;
 
-   if (dev_is_pci(dev))
-   return of_pci_iommu_configure(to_pci_dev(dev), master_np);
-
/*
 * We don't currently walk up the tree looking for a parent IOMMU.
 * See the `Notes:' section of
 * Documentation/devicetree/bindings/iommu/iommu.txt
 */
-   while (!of_parse_phandle_with_args(master_np, "iommus",
-  "#iommu-cells", idx,
-  &iommu_spec)) {
-   np = iommu_spec.np;
-   ops = iommu_ops_from_fwnode(&np->fwnode);
-
-   if (!ops || !ops->of_xlate ||
-   iommu_fwspec_init(dev, &np->fwnode, ops) ||
-   ops->of_xlate(dev, &iommu_spec))
-   goto err_put_node;
-
-   of_node_put(np);
+   while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells",
+  idx, &iommu_spec)) {
+   ops = of_iommu_xlate(dev, &iommu_spec);
+   of_node_put(iommu_spec.np);
idx++;
+   if (IS_ERR_OR_NULL(ops))
+   break;
}
 
return ops;
+}
+
+const struct iommu_ops *of_iommu_configure(struct device *dev,
+  struct device_node *master_np)
+{
+   const struct iommu_ops *ops;
+
+   if (!master_np)
+   return NULL;
+
+   if (dev_is_pci(dev))
+   ops = of_pci_iommu_init(to_pci_dev(dev), master_np);
+   else
+   ops = of_platform_iommu_init(d

[PATCH V10 10/12] arm64: dma-mapping: Remove the notifier trick to handle early setting of dma_ops

[Sricharan R]

With arch_setup_dma_ops now being called late during device's probe after
the device's iommu is probed, the notifier trick required to handle the
early setup of dma_ops before the iommu group gets created is not
required. So removing the notifier's here.

Tested-by: Marek Szyprowski 
Tested-by: Hanjun Guo 
Acked-by: Will Deacon 
Signed-off-by: Sricharan R 
[rm: clean up even more]
Signed-off-by: Robin Murphy 
---
 arch/arm64/mm/dma-mapping.c | 142 ++--
 1 file changed, 18 insertions(+), 124 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 81cdb2e..b465759 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -813,34 +813,26 @@ static void __iommu_unmap_sg_attrs(struct device *dev,
.mapping_error = iommu_dma_mapping_error,
 };
 
-/*
- * TODO: Right now __iommu_setup_dma_ops() gets called too early to do
- * everything it needs to - the device is only partially created and the
- * IOMMU driver hasn't seen it yet, so it can't have a group. Thus we
- * need this delayed attachment dance. Once IOMMU probe ordering is sorted
- * to move the arch_setup_dma_ops() call later, all the notifier bits below
- * become unnecessary, and will go away.
- */
-struct iommu_dma_notifier_data {
-   struct list_head list;
-   struct device *dev;
-   const struct iommu_ops *ops;
-   u64 dma_base;
-   u64 size;
-};
-static LIST_HEAD(iommu_dma_masters);
-static DEFINE_MUTEX(iommu_dma_notifier_lock);
+static int __init __iommu_dma_init(void)
+{
+   return iommu_dma_init();
+}
+arch_initcall(__iommu_dma_init);
 
-static bool do_iommu_attach(struct device *dev, const struct iommu_ops *ops,
-  u64 dma_base, u64 size)
+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+ const struct iommu_ops *ops)
 {
-   struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+   struct iommu_domain *domain;
+
+   if (!ops)
+   return;
 
/*
-* If the IOMMU driver has the DMA domain support that we require,
-* then the IOMMU core will have already configured a group for this
-* device, and allocated the default domain for that group.
+* The IOMMU core code allocates the default DMA domain, which the
+* underlying IOMMU driver needs to support via the dma-iommu layer.
 */
+   domain = iommu_get_domain_for_dev(dev);
+
if (!domain)
goto out_err;
 
@@ -851,109 +843,11 @@ static bool do_iommu_attach(struct device *dev, const 
struct iommu_ops *ops,
dev->dma_ops = &iommu_dma_ops;
}
 
-   return true;
+   return;
+
 out_err:
-   pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA 
ops\n",
+pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA 
ops\n",
 dev_name(dev));
-   return false;
-}
-
-static void queue_iommu_attach(struct device *dev, const struct iommu_ops *ops,
- u64 dma_base, u64 size)
-{
-   struct iommu_dma_notifier_data *iommudata;
-
-   iommudata = kzalloc(sizeof(*iommudata), GFP_KERNEL);
-   if (!iommudata)
-   return;
-
-   iommudata->dev = dev;
-   iommudata->ops = ops;
-   iommudata->dma_base = dma_base;
-   iommudata->size = size;
-
-   mutex_lock(&iommu_dma_notifier_lock);
-   list_add(&iommudata->list, &iommu_dma_masters);
-   mutex_unlock(&iommu_dma_notifier_lock);
-}
-
-static int __iommu_attach_notifier(struct notifier_block *nb,
-  unsigned long action, void *data)
-{
-   struct iommu_dma_notifier_data *master, *tmp;
-
-   if (action != BUS_NOTIFY_BIND_DRIVER)
-   return 0;
-
-   mutex_lock(&iommu_dma_notifier_lock);
-   list_for_each_entry_safe(master, tmp, &iommu_dma_masters, list) {
-   if (data == master->dev && do_iommu_attach(master->dev,
-   master->ops, master->dma_base, master->size)) {
-   list_del(&master->list);
-   kfree(master);
-   break;
-   }
-   }
-   mutex_unlock(&iommu_dma_notifier_lock);
-   return 0;
-}
-
-static int __init register_iommu_dma_ops_notifier(struct bus_type *bus)
-{
-   struct notifier_block *nb = kzalloc(sizeof(*nb), GFP_KERNEL);
-   int ret;
-
-   if (!nb)
-   return -ENOMEM;
-
-   nb->notifier_call = __iommu_attach_notifier;
-
-   ret = bus_register_notifier(bus, nb);
-   if (ret) {
-   pr_warn("Failed to register DMA domain notifier; IOMMU DMA ops 
unavailable on bus '%s'\n",
-   bus->name);
-  

[PATCH V10 11/12] iommu/arm-smmu: Clean up early-probing workarounds

[Sricharan R]

From: Robin Murphy 

Now that the appropriate ordering is enforced via probe-deferral of
masters in core code, rip it all out and bask in the simplicity.

Tested-by: Hanjun Guo 
Acked-by: Will Deacon 
Signed-off-by: Robin Murphy 
[Sricharan: Rebased on top of ACPI IORT SMMU series]
Signed-off-by: Sricharan R 
---
 drivers/iommu/arm-smmu-v3.c |  46 +-
 drivers/iommu/arm-smmu.c| 110 +++-
 2 files changed, 49 insertions(+), 107 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 591bb96..4362139 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2761,51 +2761,9 @@ static int arm_smmu_device_remove(struct platform_device 
*pdev)
.probe  = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
 };
+module_platform_driver(arm_smmu_driver);
 
-static int __init arm_smmu_init(void)
-{
-   static bool registered;
-   int ret = 0;
-
-   if (!registered) {
-   ret = platform_driver_register(&arm_smmu_driver);
-   registered = !ret;
-   }
-   return ret;
-}
-
-static void __exit arm_smmu_exit(void)
-{
-   return platform_driver_unregister(&arm_smmu_driver);
-}
-
-subsys_initcall(arm_smmu_init);
-module_exit(arm_smmu_exit);
-
-static int __init arm_smmu_of_init(struct device_node *np)
-{
-   int ret = arm_smmu_init();
-
-   if (ret)
-   return ret;
-
-   if (!of_platform_device_create(np, NULL, platform_bus_type.dev_root))
-   return -ENODEV;
-
-   return 0;
-}
-IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", arm_smmu_of_init);
-
-#ifdef CONFIG_ACPI
-static int __init acpi_smmu_v3_init(struct acpi_table_header *table)
-{
-   if (iort_node_match(ACPI_IORT_NODE_SMMU_V3))
-   return arm_smmu_init();
-
-   return 0;
-}
-IORT_ACPI_DECLARE(arm_smmu_v3, ACPI_SIG_IORT, acpi_smmu_v3_init);
-#endif
+IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", NULL);
 
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
 MODULE_AUTHOR("Will Deacon ");
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index b493c99..792fe7d 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2075,6 +2075,23 @@ static int arm_smmu_device_dt_probe(struct 
platform_device *pdev,
return 0;
 }
 
+static void arm_smmu_bus_init(void)
+{
+   /* Oh, for a proper bus abstraction */
+   if (!iommu_present(&platform_bus_type))
+   bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
+#ifdef CONFIG_ARM_AMBA
+   if (!iommu_present(&amba_bustype))
+   bus_set_iommu(&amba_bustype, &arm_smmu_ops);
+#endif
+#ifdef CONFIG_PCI
+   if (!iommu_present(&pci_bus_type)) {
+   pci_request_acs();
+   bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
+   }
+#endif
+}
+
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
struct resource *res;
@@ -2180,21 +2197,30 @@ static int arm_smmu_device_probe(struct platform_device 
*pdev)
arm_smmu_device_reset(smmu);
arm_smmu_test_smr_masks(smmu);
 
-   /* Oh, for a proper bus abstraction */
-   if (!iommu_present(&platform_bus_type))
-   bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
-#ifdef CONFIG_ARM_AMBA
-   if (!iommu_present(&amba_bustype))
-   bus_set_iommu(&amba_bustype, &arm_smmu_ops);
-#endif
-#ifdef CONFIG_PCI
-   if (!iommu_present(&pci_bus_type)) {
-   pci_request_acs();
-   bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
-   }
-#endif
+   /*
+* For ACPI and generic DT bindings, an SMMU will be probed before
+* any device which might need it, so we want the bus ops in place
+* ready to handle default domain setup as soon as any SMMU exists.
+*/
+   if (!using_legacy_binding)
+   arm_smmu_bus_init();
+
+   return 0;
+}
+
+/*
+ * With the legacy DT binding in play, though, we have no guarantees about
+ * probe order, but then we're also not doing default domains, so we can
+ * delay setting bus ops until we're sure every possible SMMU is ready,
+ * and that way ensure that no add_device() calls get missed.
+ */
+static int arm_smmu_legacy_bus_init(void)
+{
+   if (using_legacy_binding)
+   arm_smmu_bus_init();
return 0;
 }
+device_initcall_sync(arm_smmu_legacy_bus_init);
 
 static int arm_smmu_device_remove(struct platform_device *pdev)
 {
@@ -2219,56 +2245,14 @@ static int arm_smmu_device_remove(struct 
platform_device *pdev)
.probe  = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
 };
-
-static int __init arm_smmu_init(void)
-{
-   static bool registered;
-   int ret = 0;
-
-   if (!registered) {
-   re

[PATCH V10 12/12] ACPI/IORT: Remove linker section for IORT entries probing

[Sricharan R]

From: Lorenzo Pieralisi 

The IORT linker section introduced by commit 34ceea275f62
("ACPI/IORT: Introduce linker section for IORT entries probing")
was needed to make sure SMMU drivers are registered (and therefore
probed) in the kernel before devices using the SMMU have a chance
to probe in turn.

Through the introduction of deferred IOMMU configuration the linker
section based IORT probing infrastructure is not needed any longer, in
that device/SMMU probe dependencies are managed through the probe
deferral mechanism, making the IORT linker section infrastructure
unused, so that it can be removed.

Remove the unused IORT linker section probing infrastructure
from the kernel to complete the ACPI IORT IOMMU configure probe
deferral mechanism implementation.

Tested-by: Hanjun Guo 
Signed-off-by: Lorenzo Pieralisi 
Cc: Robin Murphy 
Cc: Sricharan R 
---
 drivers/acpi/arm64/iort.c | 2 --
 include/asm-generic/vmlinux.lds.h | 1 -
 include/linux/acpi_iort.h | 3 ---
 3 files changed, 6 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index e323ece..e7b1940 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1000,6 +1000,4 @@ void __init acpi_iort_init(void)
}
 
iort_init_platform_devices();
-
-   acpi_probe_device_table(iort);
 }
diff --git a/include/asm-generic/vmlinux.lds.h 
b/include/asm-generic/vmlinux.lds.h
index 0968d13..9faa26c 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -566,7 +566,6 @@
IRQCHIP_OF_MATCH_TABLE()\
ACPI_PROBE_TABLE(irqchip)   \
ACPI_PROBE_TABLE(clksrc)\
-   ACPI_PROBE_TABLE(iort)  \
EARLYCON_TABLE()
 
 #define INIT_TEXT  \
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 77e0809..f167e1d04 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -52,7 +52,4 @@ const struct iommu_ops *iort_iommu_configure(struct device 
*dev)
 { return NULL; }
 #endif
 
-#define IORT_ACPI_DECLARE(name, table_id, fn)  \
-   ACPI_DECLARE_PROBE_ENTRY(iort, name, table_id, 0, NULL, 0, fn)
-
 #endif /* __ACPI_IORT_H__ */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V10 09/12] drivers: acpi: Handle IOMMU lookup failure with deferred probing or error

[Sricharan R]

This is an equivalent to the DT's handling of the iommu master's probe
with deferred probing when the corrsponding iommu is not probed yet.
The lack of a registered IOMMU can be caused by the lack of a driver for
the IOMMU, the IOMMU device probe not having been performed yet, having
been deferred, or having failed.

The first case occurs when the firmware describes the bus master and
IOMMU topology correctly but no device driver exists for the IOMMU yet
or the device driver has not been compiled in. Return NULL, the caller
will configure the device without an IOMMU.

The second and third cases are handled by deferring the probe of the bus
master device which will eventually get reprobed after the IOMMU.

The last case is currently handled by deferring the probe of the bus
master device as well. A mechanism to either configure the bus master
device without an IOMMU or to fail the bus master device probe depending
on whether the IOMMU is optional or mandatory would be a good
enhancement.

Tested-by: Hanjun Guo 
[Lorenzo: Added fixes for dma_coherent_mask overflow, acpi_dma_configure
  called multiple times for same device]
Signed-off-by: Lorenzo Pieralisi 
Signed-off-by: Sricharan R 
---

 [V10] Added Lorenzo's ACPI fix for coherent_dma_mask overflow
   and the fix for dma_configure getting called more than
   once for the same device.

 drivers/acpi/arm64/iort.c  | 33 -
 drivers/acpi/scan.c| 11 ---
 drivers/base/dma-mapping.c |  2 +-
 include/acpi/acpi_bus.h|  2 +-
 include/linux/acpi.h   |  7 +--
 5 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 3dd9ec3..e323ece 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -543,6 +543,14 @@ static const struct iommu_ops *iort_iommu_xlate(struct 
device *dev,
const struct iommu_ops *ops = NULL;
int ret = -ENODEV;
struct fwnode_handle *iort_fwnode;
+   struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+   /*
+* If we already translated the fwspec there
+* is nothing left to do, return the iommu_ops.
+*/
+   if (fwspec && fwspec->ops)
+   return fwspec->ops;
 
if (node) {
iort_fwnode = iort_get_fwnode(node);
@@ -550,8 +558,17 @@ static const struct iommu_ops *iort_iommu_xlate(struct 
device *dev,
return NULL;
 
ops = iommu_ops_from_fwnode(iort_fwnode);
+   /*
+* If the ops look-up fails, this means that either
+* the SMMU drivers have not been probed yet or that
+* the SMMU drivers are not built in the kernel;
+* Depending on whether the SMMU drivers are built-in
+* in the kernel or not, defer the IOMMU configuration
+* or just abort it.
+*/
if (!ops)
-   return NULL;
+   return iort_iommu_driver_enabled(node->type) ?
+  ERR_PTR(-EPROBE_DEFER) : NULL;
 
ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
}
@@ -625,12 +642,26 @@ const struct iommu_ops *iort_iommu_configure(struct 
device *dev)
 
while (parent) {
ops = iort_iommu_xlate(dev, parent, streamid);
+   if (IS_ERR_OR_NULL(ops))
+   return ops;
 
parent = iort_node_get_id(node, &streamid,
  IORT_IOMMU_TYPE, i++);
}
}
 
+   /*
+* If we have reason to believe the IOMMU driver missed the initial
+* add_device callback for dev, replay it to get things in order.
+*/
+   if (!IS_ERR_OR_NULL(ops) && ops->add_device &&
+   dev->bus && !dev->iommu_group) {
+   int err = ops->add_device(dev);
+
+   if (err)
+   ops = ERR_PTR(err);
+   }
+
return ops;
 }
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1926918..2a513cc 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1373,20 +1373,25 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device 
*adev)
  * @dev: The pointer to the device
  * @attr: device dma attributes
  */
-void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
+int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
 {
const struct iommu_ops *iommu;
+   u64 size;
 
iort_set_dma_mask(dev);
 
iommu = iort_iommu_configure(dev);
+   if (IS_ERR(iommu))
+   return PTR_ERR(iommu);
 
+   size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
/*
 * Assume dma valid range starts at 0 and covers the 

[PATCH V10 07/12] of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices

[Sricharan R]

Configuring DMA ops at probe time will allow deferring device probe when
the IOMMU isn't available yet. The dma_configure for the device is
now called from the generic device_attach callback just before the
bus/driver probe is called. This way, configuring the DMA ops for the
device would be called at the same place for all bus_types, hence the
deferred probing mechanism should work for all buses as well.

pci_bus_add_devices(platform/amba)(_device_create/driver_register)
   | |
pci_bus_add_device (device_add/driver_register)
   | |
device_attach   device_initial_probe
   | |
__device_attach_driver__device_attach_driver
   |
driver_probe_device
   |
really_probe
   |
dma_configure

Similarly on the device/driver_unregister path __device_release_driver is
called which inturn calls dma_deconfigure.

This patch changes the dma ops configuration to probe time for
both OF and ACPI based platform/amba/pci bus devices.

Tested-by: Marek Szyprowski 
Tested-by: Hanjun Guo 
Acked-by: Bjorn Helgaas  (drivers/pci part)
Acked-by: Rafael J. Wysocki 
Signed-off-by: Sricharan R 
---

 [V10] Added dummy dma_(de)configure functions in case
   of !CONFIG_HAS_DMA to avoid build breaks.

 drivers/acpi/glue.c |  5 -
 drivers/base/dd.c   |  9 +
 drivers/base/dma-mapping.c  | 40 
 drivers/of/platform.c   |  5 +
 drivers/pci/probe.c | 28 
 include/linux/dma-mapping.h | 12 
 6 files changed, 62 insertions(+), 37 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index fb19e1c..c05f241 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -176,7 +176,6 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
*acpi_dev)
struct list_head *physnode_list;
unsigned int node_id;
int retval = -EINVAL;
-   enum dev_dma_attr attr;
 
if (has_acpi_companion(dev)) {
if (acpi_dev) {
@@ -233,10 +232,6 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
*acpi_dev)
if (!has_acpi_companion(dev))
ACPI_COMPANION_SET(dev, acpi_dev);
 
-   attr = acpi_get_dma_attr(acpi_dev);
-   if (attr != DEV_DMA_NOT_SUPPORTED)
-   acpi_dma_configure(dev, attr);
-
acpi_physnode_link_name(physical_node_name, node_id);
retval = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
   physical_node_name);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a1fbf55..4882f06 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -19,6 +19,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -356,6 +357,10 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)
if (ret)
goto pinctrl_bind_failed;
 
+   ret = dma_configure(dev);
+   if (ret)
+   goto dma_failed;
+
if (driver_sysfs_add(dev)) {
printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
__func__, dev_name(dev));
@@ -417,6 +422,8 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)
goto done;
 
 probe_failed:
+   dma_deconfigure(dev);
+dma_failed:
if (dev->bus)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
@@ -826,6 +833,8 @@ static void __device_release_driver(struct device *dev, 
struct device *parent)
drv->remove(dev);
 
device_links_driver_cleanup(dev);
+   dma_deconfigure(dev);
+
devres_release_all(dev);
dev->driver = NULL;
dev_set_drvdata(dev, NULL);
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index efd71cf..449b948 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -7,9 +7,11 @@
  * This file is released under the GPLv2.
  */
 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -341,3 +343,41 @@ void dma_common_free_remap(void *cpu_addr, size_t size, 
unsigned long vm_flags)
vunmap(cpu_addr);
 }
 #endif
+
+/*
+ * Common configuration to enable DMA API use for a device
+ */
+#include 
+
+int dma_configure(struct device *dev)
+{
+   struct device *bridge = NULL, *dma_dev = dev;
+   enum dev_dma_attr attr;
+
+   if (dev_is_pci(dev)) {
+   bridge = pci_get_host_bridge_device(to_pci_dev(dev));
+   dma_dev = bridge;
+   if (IS_ENABLED(CONFIG_OF) && dma_dev->parent &&
+   dma_dev->parent->of_node)
+   dma_dev = dma_dev->parent;
+   }
+
+   if (dma_dev->of_

RE: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Sricharan]

Hi Stan,

>-Original Message-
>From: Stanimir Varbanov [mailto:stanimir.varba...@linaro.org]
>Sent: Wednesday, January 11, 2017 2:25 PM
>To: Sricharan ; 'Stanimir Varbanov' 
>; 'Rajendra Nayak'
>; sb...@codeaurora.org; mturque...@baylibre.com
>Cc: linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org; 
>linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable
>
>Hi Sricharan,
>
>On 01/10/2017 09:29 PM, Sricharan wrote:
>> Hi stan,
>>
>>> -Original Message-
>>> From: linux-arm-msm-ow...@vger.kernel.org 
>>> [mailto:linux-arm-msm-ow...@vger.kernel.org] On Behalf Of Stanimir Varbanov
>>> Sent: Tuesday, January 10, 2017 10:14 PM
>>> To: Rajendra Nayak ; sb...@codeaurora.org; 
>>> mturque...@baylibre.com
>>> Cc: linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org; 
>>> linux-kernel@vger.kernel.org; sricha...@codeaurora.org
>>> Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control 
>>> enable/disable
>>>
>>> Hi Rajendra,
>>>
>>> On 01/10/2017 07:54 AM, Rajendra Nayak wrote:
>>>> Once a gdsc is brought in and out of HW control, there is a
>>>> power down and up cycle which can take upto 1us. Polling on
>>>> the gdsc status immediately after the hw control enable/disable
>>>> can mislead software/firmware to belive the gdsc is already either on
>>>> or off, while its yet to complete the power cycle.
>>>> To avoid this add a 1us delay post a enable/disable of HW control
>>>> mode.
>>>>
>>>> Also after the HW control mode is disabled, poll on the status to
>>>> check gdsc status reflects its 'on' before force disabling it
>>>> in software.
>>>>
>>>> Reported-by: Stanimir Varbanov 
>>>> Signed-off-by: Rajendra Nayak 
>>>> ---
>>>>
>>>> Stan,
>>>> If there was a specific issue you saw with venus because of the missing
>>>> delay and poll, can you check if this fixes any of that.
>>>
>>> Something more about the issue.
>>>
>>> I had re-designed venus driver on three platform drivers venus-core,
>>> venus-dec and venus-enc in order to be able to control those three
>>> power-domains (VENUS_GDSC, VENUS_CORE0_GDSC and VENUS_CORE1_GDSC).
>>>
>>> After that I abstracted MMAGIC hw on a separate mmagic driver. This
>>> driver just controls mmagic clocks and GDSC in its runtime_suspend and
>>> runtime_resume methods.
>>>
>>> The DT nodes looks like:
>>>
>>> mmagic_video {
>>> compatible = "qcom,msm8996-mmagic";
>>> clocks = <&rpmcc MSM8996_RPM_SMD_MMAXI_CLK>,
>>>  <&mmcc MMSS_MMAGIC_AHB_CLK>,
>>>  <&mmcc MMSS_MMAGIC_CFG_AHB_CLK>,
>>>  <&mmcc MMAGIC_VIDEO_NOC_CFG_AHB_CLK>,
>>>  <&mmcc MMSS_MMAGIC_MAXI_CLK>,
>>>  <&mmcc MMAGIC_VIDEO_AXI_CLK>,
>>>  <&mmcc SMMU_VIDEO_AHB_CLK>,
>>>  <&mmcc SMMU_VIDEO_AXI_CLK>;
>>> power-domains = <&mmcc MMAGIC_VIDEO_GDSC>;
>>>
>>> video-codec {
>>> compatible = "qcom,msm8996-venus";
>>> clocks = <&mmcc VIDEO_CORE_CLK>,
>>>  <&mmcc VIDEO_AHB_CLK>,
>>>  <&mmcc VIDEO_AXI_CLK>,
>>>  <&mmcc VIDEO_MAXI_CLK>;
>>> power-domains = <&mmcc VENUS_GDSC>;
>>> ...
>>>
>>> video-decoder {
>>> compatible = "venus-decoder";
>>> clocks = "subcore0";
>>> clock-names = <&mmcc VIDEO_SUBCORE0_CLK>;
>>> power-domains = <&mmcc VENUS_CORE0_GDSC>;
>>> };
>>>
>>> video-encoder {
>>> compatible = "venus-encoder";
>>> clocks = "subcore1";
>>> clock-names = <&mmcc VIDEO_SUBCORE1_CLK>;
>>> power-domains = <&mmcc VENUS_CORE1_GDSC>;
>>> };
>>> };
>>> };
>>>
>>> Note that mmagic_video is a parent dt node for smmu_video

RE: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Sricharan]

Hi stan,

>-Original Message-
>From: linux-arm-msm-ow...@vger.kernel.org 
>[mailto:linux-arm-msm-ow...@vger.kernel.org] On Behalf Of Stanimir Varbanov
>Sent: Tuesday, January 10, 2017 10:14 PM
>To: Rajendra Nayak ; sb...@codeaurora.org; 
>mturque...@baylibre.com
>Cc: linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org; 
>linux-kernel@vger.kernel.org; sricha...@codeaurora.org
>Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable
>
>Hi Rajendra,
>
>On 01/10/2017 07:54 AM, Rajendra Nayak wrote:
>> Once a gdsc is brought in and out of HW control, there is a
>> power down and up cycle which can take upto 1us. Polling on
>> the gdsc status immediately after the hw control enable/disable
>> can mislead software/firmware to belive the gdsc is already either on
>> or off, while its yet to complete the power cycle.
>> To avoid this add a 1us delay post a enable/disable of HW control
>> mode.
>>
>> Also after the HW control mode is disabled, poll on the status to
>> check gdsc status reflects its 'on' before force disabling it
>> in software.
>>
>> Reported-by: Stanimir Varbanov 
>> Signed-off-by: Rajendra Nayak 
>> ---
>>
>> Stan,
>> If there was a specific issue you saw with venus because of the missing
>> delay and poll, can you check if this fixes any of that.
>
>Something more about the issue.
>
>I had re-designed venus driver on three platform drivers venus-core,
>venus-dec and venus-enc in order to be able to control those three
>power-domains (VENUS_GDSC, VENUS_CORE0_GDSC and VENUS_CORE1_GDSC).
>
>After that I abstracted MMAGIC hw on a separate mmagic driver. This
>driver just controls mmagic clocks and GDSC in its runtime_suspend and
>runtime_resume methods.
>
>The DT nodes looks like:
>
>mmagic_video {
>   compatible = "qcom,msm8996-mmagic";
>   clocks = <&rpmcc MSM8996_RPM_SMD_MMAXI_CLK>,
><&mmcc MMSS_MMAGIC_AHB_CLK>,
><&mmcc MMSS_MMAGIC_CFG_AHB_CLK>,
><&mmcc MMAGIC_VIDEO_NOC_CFG_AHB_CLK>,
><&mmcc MMSS_MMAGIC_MAXI_CLK>,
><&mmcc MMAGIC_VIDEO_AXI_CLK>,
><&mmcc SMMU_VIDEO_AHB_CLK>,
><&mmcc SMMU_VIDEO_AXI_CLK>;
>   power-domains = <&mmcc MMAGIC_VIDEO_GDSC>;
>
>   video-codec {
>   compatible = "qcom,msm8996-venus";
>   clocks = <&mmcc VIDEO_CORE_CLK>,
><&mmcc VIDEO_AHB_CLK>,
><&mmcc VIDEO_AXI_CLK>,
><&mmcc VIDEO_MAXI_CLK>;
>   power-domains = <&mmcc VENUS_GDSC>;
>   ...
>
>   video-decoder {
>   compatible = "venus-decoder";
>   clocks = "subcore0";
>   clock-names = <&mmcc VIDEO_SUBCORE0_CLK>;
>   power-domains = <&mmcc VENUS_CORE0_GDSC>;
>   };
>
>   video-encoder {
>   compatible = "venus-encoder";
>   clocks = "subcore1";
>   clock-names = <&mmcc VIDEO_SUBCORE1_CLK>;
>   power-domains = <&mmcc VENUS_CORE1_GDSC>;
>   };
>   };
>};
>
>Note that mmagic_video is a parent dt node for smmu_video DT node so
>that clocks and mmagic_video gdsc will be enabled once smmu driver is
>instantiated by venus-core diriver.
>

mmagic_video is a parent DT for smmu_video ? , so there are no clocks
populated for the smmu node as such ?

>Now when video-dec driver calls pm_runtime_get_sync() the sequence of
>enabling is:
>
>MMAGIC_VIDEO_GDSC -> MMAGIC clocks and SMMU clocks -> VENUS_GDSC ->
>VIDEO clocks -> VENUS_CORE0_GDSC -> VIDEO subcore0 clock
>
>When video-dec platform driver calls pm_runtime_put_sync() we should
>disabling of GDSC and clocks in the reversed oder.
>
>The issue comes when I have ran video decoder, the decoder hw finish
>stream decoding and we want to suspend venus core. The issue is that
>when I start disabling SMMU_VIDEO_AXI_CLK and/or MMAGIC_VIDEO_AXI_CLK
>the system reboots.
>
>I have added a delay (200ms) before disabling mmagic clocks and then
>everything is fine again.
>
>Any idea?
>

Can you share me a branch, i can have a quick check with a t32
if there is any crash logged in the TZ buffer when the system reboots.

Regards,
 Sricharan

[PATCH V9 0/9] Add support for privileged mappings

[Sricharan R]

This series is a resend of the V5 that Mitch sent sometime back [2]
All the patches are the same and i have just rebased. Redid patch [3],
as it does not apply in this code base. Added a couple of more patches
[4], [5] from Robin for adding the privileged attributes to armv7s format
and arm-smmuv3 revert. Added a patch for passing in the privileged
attributes from arm32 dma-mapping apis as well.

The following patch to the ARM SMMU driver:

commit d346180e70b91b3d5a1ae7e5603e65593d4622bc
Author: Robin Murphy 
Date:   Tue Jan 26 18:06:34 2016 +

iommu/arm-smmu: Treat all device transactions as unprivileged

started forcing all SMMU transactions to come through as "unprivileged".
The rationale given was that:

  (1) There is no way in the IOMMU API to even request privileged
  mappings.

  (2) It's difficult to implement a DMA mapper that correctly models the
  ARM VMSAv8 behavior of unprivileged-writeable =>
  privileged-execute-never.

This series rectifies (1) by introducing an IOMMU API for privileged
mappings and implements it in io-pgtable-arm.

This series rectifies (2) by introducing a new dma attribute
(DMA_ATTR_PRIVILEGED) for users of the DMA API that need privileged
mappings which are inaccessible to lesser-privileged execution levels, and
implements it in the arm64 IOMMU DMA mapper.  The one known user (pl330.c)
is converted over to the new attribute.

Jordan and Jeremy can provide more info on the use case if needed, but the
high level is that it's a security feature to prevent attacks such as [1].

Note that, i tested this on arm64 with arm-smmuv2, short descriptor changes,
tested this on arm32 platform as well and do not have an platform to test
this with arm-smmuv3.

[1] https://github.com/robclark/kilroy
[2] https://lkml.org/lkml/2016/7/27/590
[3] https://patchwork.kernel.org/patch/9250493/
[4] 
http://www.linux-arm.org/git?p=linux-rm.git;a=commit;h=1291bd74f05d31da1dab3df02987cba5bd25849b
[5] 
http://www.linux-arm.org/git?p=linux-rm.git;a=commit;h=a79c1c6333f26849dba418cd92de26b60f5954f3

Changelog:
 v8..v9
- Added additional comment in patch 1 and added tag for patch 6.

 v7..v8
- Added a patch for passing in the privileged attributes from arm32
  dma-mapping apis as well.

 v6..v7
- Added couple of more patches, picked up acks, updated commit log

 v5..v6
- Rebased all the patches and redid 6/6 as it does not apply in
  this code base. 

 v4..v5
- Simplified patch 4/6 (suggested by Robin Murphy).

 v3..v4
- Rebased and reworked on linux next due to the dma attrs rework going
  on over there.  Patches changed: 3/6, 4/6, and 5/6.

 v2..v3
- Incorporated feedback from Robin:
  * Various comments and re-wordings.
  * Use existing bit definitions for IOMMU_PRIV implementation
in io-pgtable-arm.
  * Renamed and redocumented dma_direction_to_prot.
  * Don't worry about executability in new DMA attr.

 v1..v2
- Added a new DMA attribute to make executable privileged mappings
  work, and use that in the pl330 driver (suggested by Will).

Jeremy Gebben (1):
  iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

Mitchel Humpherys (4):
  iommu: add IOMMU_PRIV attribute
  common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute
  arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
  dmaengine: pl330: Make sure microcode is privileged

Robin Murphy (2):
  iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag
  Revert "iommu/arm-smmu: Set PRIVCFG in stage 1 STEs"

Sricharan R (2):
  arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED
  iommu/arm-smmu: Set privileged attribute to 'default' instead of
'unprivileged'

 Documentation/DMA-attributes.txt   | 10 +++
 arch/arm/mm/dma-mapping.c  | 60 +++---
 arch/arm64/mm/dma-mapping.c|  6 ++--
 drivers/dma/pl330.c|  5 ++--
 drivers/iommu/arm-smmu-v3.c|  7 +
 drivers/iommu/arm-smmu.c   |  2 +-
 drivers/iommu/dma-iommu.c  | 12 ++--
 drivers/iommu/io-pgtable-arm-v7s.c |  6 +++-
 drivers/iommu/io-pgtable-arm.c |  5 +++-
 include/linux/dma-iommu.h  |  3 +-
 include/linux/dma-mapping.h|  7 +
 include/linux/iommu.h  |  7 +
 12 files changed, 82 insertions(+), 48 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 4/9] common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute

[Sricharan R]

From: Mitchel Humpherys 

This patch adds the DMA_ATTR_PRIVILEGED attribute to the DMA-mapping
subsystem.

Some advanced peripherals such as remote processors and GPUs perform
accesses to DMA buffers in both privileged "supervisor" and unprivileged
"user" modes.  This attribute is used to indicate to the DMA-mapping
subsystem that the buffer is fully accessible at the elevated privilege
level (and ideally inaccessible or at least read-only at the
lesser-privileged levels).

Cc: linux-...@vger.kernel.org
Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---
 Documentation/DMA-attributes.txt | 10 ++
 include/linux/dma-mapping.h  |  7 +++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/DMA-attributes.txt b/Documentation/DMA-attributes.txt
index 98bf7ac..44c6bc4 100644
--- a/Documentation/DMA-attributes.txt
+++ b/Documentation/DMA-attributes.txt
@@ -143,3 +143,13 @@ So, this provides a way for drivers to avoid those error 
messages on calls
 where allocation failures are not a problem, and shouldn't bother the logs.
 
 NOTE: At the moment DMA_ATTR_NO_WARN is only implemented on PowerPC.
+
+DMA_ATTR_PRIVILEGED
+--
+
+Some advanced peripherals such as remote processors and GPUs perform
+accesses to DMA buffers in both privileged "supervisor" and unprivileged
+"user" modes.  This attribute is used to indicate to the DMA-mapping
+subsystem that the buffer is fully accessible at the elevated privilege
+level (and ideally inaccessible or at least read-only at the
+lesser-privileged levels).
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 10c5a17b1..c24721a 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -63,6 +63,13 @@
 #define DMA_ATTR_NO_WARN   (1UL << 8)
 
 /*
+ * DMA_ATTR_PRIVILEGED: used to indicate that the buffer is fully
+ * accessible at an elevated privilege level (and ideally inaccessible or
+ * at least read-only at lesser-privileged levels).
+ */
+#define DMA_ATTR_PRIVILEGED(1UL << 9)
+
+/*
  * A dma_addr_t can hold any valid DMA or bus address for the platform.
  * It can be given to a device to use as a DMA source or target.  A CPU cannot
  * reference a dma_addr_t directly because there may be translation between
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 2/9] iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

[Sricharan R]

From: Jeremy Gebben 

Allow the creation of privileged mode mappings, for stage 1 only.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Jeremy Gebben 
---
 drivers/iommu/io-pgtable-arm.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index a40ce34..feacc54 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -350,11 +350,14 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct 
arm_lpae_io_pgtable *data,
 
if (data->iop.fmt == ARM_64_LPAE_S1 ||
data->iop.fmt == ARM_32_LPAE_S1) {
-   pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
+   pte = ARM_LPAE_PTE_nG;
 
if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
pte |= ARM_LPAE_PTE_AP_RDONLY;
 
+   if (!(prot & IOMMU_PRIV))
+   pte |= ARM_LPAE_PTE_AP_UNPRIV;
+
if (prot & IOMMU_MMIO)
pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 3/9] iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag

[Sricharan R]

From: Robin Murphy 

The short-descriptor format also allows privileged-only mappings, so
let's wire it up.

Signed-off-by: Robin Murphy 
Tested-by: Sricharan R 
---
 drivers/iommu/io-pgtable-arm-v7s.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c 
b/drivers/iommu/io-pgtable-arm-v7s.c
index 0769276..1c049e2 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -265,7 +265,9 @@ static arm_v7s_iopte arm_v7s_prot_to_pte(int prot, int lvl,
if (!(prot & IOMMU_MMIO))
pte |= ARM_V7S_ATTR_TEX(1);
if (ap) {
-   pte |= ARM_V7S_PTE_AF | ARM_V7S_PTE_AP_UNPRIV;
+   pte |= ARM_V7S_PTE_AF;
+   if (!(prot & IOMMU_PRIV))
+   pte |= ARM_V7S_PTE_AP_UNPRIV;
if (!(prot & IOMMU_WRITE))
pte |= ARM_V7S_PTE_AP_RDONLY;
}
@@ -288,6 +290,8 @@ static int arm_v7s_pte_to_prot(arm_v7s_iopte pte, int lvl)
 
if (!(attr & ARM_V7S_PTE_AP_RDONLY))
prot |= IOMMU_WRITE;
+   if (!(attr & ARM_V7S_PTE_AP_UNPRIV))
+   prot |= IOMMU_PRIV;
if ((attr & (ARM_V7S_TEX_MASK << ARM_V7S_TEX_SHIFT)) == 0)
prot |= IOMMU_MMIO;
else if (pte & ARM_V7S_ATTR_C)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 1/9] iommu: add IOMMU_PRIV attribute

[Sricharan R]

From: Mitchel Humpherys 

Add the IOMMU_PRIV attribute, which is used to indicate privileged
mappings.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Signed-off-by: Mitchel Humpherys 
Acked-by: Will Deacon 
---
 include/linux/iommu.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0ff5111..69e2417 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -31,6 +31,13 @@
 #define IOMMU_CACHE(1 << 2) /* DMA cache coherency */
 #define IOMMU_NOEXEC   (1 << 3)
 #define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
+/*
+ * This is to make the IOMMU API setup privileged
+ * mapppings accessible by the master only at higher
+ * privileged execution level and inaccessible at
+ * less privileged levels.
+ */
+#define IOMMU_PRIV (1 << 5)
 
 struct iommu_ops;
 struct iommu_group;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 7/9] dmaengine: pl330: Make sure microcode is privileged

[Sricharan R]

From: Mitchel Humpherys 

The PL330 is hard-wired such that instruction fetches on both the
manager and channel threads go out onto the bus with the "privileged"
bit set. This can become troublesome once there is an IOMMU or other
form of memory protection downstream, since those will typically be
programmed by the DMA mapping subsystem in the expectation of normal
unprivileged transactions (such as the PL330 channel threads' own data
accesses as currently configured by this driver).

To avoid the case of, say, an IOMMU blocking an unexpected privileged
transaction with a permission fault, use the newly-introduced
DMA_ATTR_PRIVILEGED attribute for the mapping of our microcode buffer.
That way the DMA layer can do whatever it needs to do to make things
continue to work as expected on more complex systems.

Cc: Dan Williams 
Cc: Vinod Koul 
Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Acked-by: Vinod Koul 
Signed-off-by: Mitchel Humpherys 
[rm: remove now-redundant local variable, clarify commit message]
Signed-off-by: Robin Murphy 
---
 drivers/dma/pl330.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 87fd015..5a90d0c 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1864,9 +1864,10 @@ static int dmac_alloc_resources(struct pl330_dmac *pl330)
 * Alloc MicroCode buffer for 'chans' Channel threads.
 * A channel's buffer offset is (Channel_Id * MCODE_BUFF_PERCHAN)
 */
-   pl330->mcode_cpu = dma_alloc_coherent(pl330->ddma.dev,
+   pl330->mcode_cpu = dma_alloc_attrs(pl330->ddma.dev,
chans * pl330->mcbufsz,
-   &pl330->mcode_bus, GFP_KERNEL);
+   &pl330->mcode_bus, GFP_KERNEL,
+   DMA_ATTR_PRIVILEGED);
if (!pl330->mcode_cpu) {
dev_err(pl330->ddma.dev, "%s:%d Can't allocate memory!\n",
__func__, __LINE__);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 5/9] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan R]

From: Mitchel Humpherys 

The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines.  Implement it in
dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---
 arch/arm64/mm/dma-mapping.c |  6 +++---
 drivers/iommu/dma-iommu.c   | 12 +---
 include/linux/dma-iommu.h   |  3 ++-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 290a84f..49c6f75 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -557,7 +557,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t 
size,
 unsigned long attrs)
 {
bool coherent = is_device_dma_coherent(dev);
-   int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+   int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
size_t iosize = size;
void *addr;
 
@@ -711,7 +711,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, 
struct page *page,
   unsigned long attrs)
 {
bool coherent = is_device_dma_coherent(dev);
-   int prot = dma_direction_to_prot(dir, coherent);
+   int prot = dma_info_to_prot(dir, coherent, attrs);
dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
 
if (!iommu_dma_mapping_error(dev, dev_addr) &&
@@ -769,7 +769,7 @@ static int __iommu_map_sg_attrs(struct device *dev, struct 
scatterlist *sgl,
__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
 
return iommu_dma_map_sg(dev, sgl, nelems,
-   dma_direction_to_prot(dir, coherent));
+   dma_info_to_prot(dir, coherent, attrs));
 }
 
 static void __iommu_unmap_sg_attrs(struct device *dev,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 2db0d64..3006eee 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -181,16 +181,22 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
dma_addr_t base,
 EXPORT_SYMBOL(iommu_dma_init_domain);
 
 /**
- * dma_direction_to_prot - Translate DMA API directions to IOMMU API page flags
+ * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU API
+ *page flags.
  * @dir: Direction of DMA transfer
  * @coherent: Is the DMA master cache-coherent?
+ * @attrs: DMA attributes for the mapping
  *
  * Return: corresponding IOMMU API page protection flags
  */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+unsigned long attrs)
 {
int prot = coherent ? IOMMU_CACHE : 0;
 
+   if (attrs & DMA_ATTR_PRIVILEGED)
+   prot |= IOMMU_PRIV;
+
switch (dir) {
case DMA_BIDIRECTIONAL:
return prot | IOMMU_READ | IOMMU_WRITE;
@@ -633,7 +639,7 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, 
phys_addr_t phys,
size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
return __iommu_dma_map(dev, phys, size,
-   dma_direction_to_prot(dir, false) | IOMMU_MMIO);
+   dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
 }
 
 void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 7f7e9a7..c5511e1 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -34,7 +34,8 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
dma_addr_t base,
u64 size, struct device *dev);
 
 /* General helpers for DMA-API <-> IOMMU-API interaction */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent);
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+unsigned long attrs);
 
 /*
  * These implement the bulk of the relevant DMA mapping callbacks, but require
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 9/9] Revert "iommu/arm-smmu: Set PRIVCFG in stage 1 STEs"

[Sricharan R]

From: Robin Murphy 

This reverts commit df5e1a0f2a2d779ad467a691203bcbc74d75690e.

Now that proper privileged mappings can be requested via IOMMU_PRIV,
unconditionally overriding the incoming PRIVCFG becomes the wrong thing
to do, so stop it.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/arm-smmu-v3.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4d6ec44..7d45d8b 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -269,9 +269,6 @@
 #define STRTAB_STE_1_SHCFG_INCOMING1UL
 #define STRTAB_STE_1_SHCFG_SHIFT   44
 
-#define STRTAB_STE_1_PRIVCFG_UNPRIV2UL
-#define STRTAB_STE_1_PRIVCFG_SHIFT 48
-
 #define STRTAB_STE_2_S2VMID_SHIFT  0
 #define STRTAB_STE_2_S2VMID_MASK   0xUL
 #define STRTAB_STE_2_VTCR_SHIFT32
@@ -1073,9 +1070,7 @@ static void arm_smmu_write_strtab_ent(struct 
arm_smmu_device *smmu, u32 sid,
 #ifdef CONFIG_PCI_ATS
 STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT |
 #endif
-STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT |
-STRTAB_STE_1_PRIVCFG_UNPRIV <<
-STRTAB_STE_1_PRIVCFG_SHIFT);
+STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT);
 
if (smmu->features & ARM_SMMU_FEAT_STALLS)
dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 8/9] iommu/arm-smmu: Set privileged attribute to 'default' instead of 'unprivileged'

[Sricharan R]

Currently the driver sets all the device transactions privileges
to UNPRIVILEGED, but there are cases where the iommu masters wants
to isolate privileged supervisor and unprivileged user.
So don't override the privileged setting to unprivileged, instead
set it to default as incoming and let it be controlled by the pagetable
settings.

Acked-by: Will Deacon 
Signed-off-by: Sricharan R 
---
 drivers/iommu/arm-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a60cded..73a0a25 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1214,7 +1214,7 @@ static int arm_smmu_domain_add_master(struct 
arm_smmu_domain *smmu_domain,
continue;
 
s2cr[idx].type = type;
-   s2cr[idx].privcfg = S2CR_PRIVCFG_UNPRIV;
+   s2cr[idx].privcfg = S2CR_PRIVCFG_DEFAULT;
s2cr[idx].cbndx = cbndx;
arm_smmu_write_s2cr(smmu, idx);
}
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V9 6/9] arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan R]

The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines.  Adding it to the
arm dma-mapping.c so that the ARM32 DMA IOMMU mapper can make use of it.

Signed-off-by: Sricharan R 
Reviewed-by: Will Deacon 
---
 arch/arm/mm/dma-mapping.c | 60 +++
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ab77100..82d3e79 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1171,6 +1171,25 @@ static int __init dma_debug_do_init(void)
 
 #ifdef CONFIG_ARM_DMA_USE_IOMMU
 
+static int __dma_info_to_prot(enum dma_data_direction dir, unsigned long attrs)
+{
+   int prot = 0;
+
+   if (attrs & DMA_ATTR_PRIVILEGED)
+   prot |= IOMMU_PRIV;
+
+   switch (dir) {
+   case DMA_BIDIRECTIONAL:
+   return prot | IOMMU_READ | IOMMU_WRITE;
+   case DMA_TO_DEVICE:
+   return prot | IOMMU_READ;
+   case DMA_FROM_DEVICE:
+   return prot | IOMMU_WRITE;
+   default:
+   return prot;
+   }
+}
+
 /* IOMMU */
 
 static int extend_iommu_mapping(struct dma_iommu_mapping *mapping);
@@ -1394,7 +1413,8 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
  * Create a mapping in device IO address space for specified pages
  */
 static dma_addr_t
-__iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
+__iommu_create_mapping(struct device *dev, struct page **pages, size_t size,
+  unsigned long attrs)
 {
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
@@ -1419,7 +1439,7 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
 
len = (j - i) << PAGE_SHIFT;
ret = iommu_map(mapping->domain, iova, phys, len,
-   IOMMU_READ|IOMMU_WRITE);
+   __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs));
if (ret < 0)
goto fail;
iova += len;
@@ -1476,7 +1496,8 @@ static struct page **__iommu_get_pages(void *cpu_addr, 
unsigned long attrs)
 }
 
 static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
- dma_addr_t *handle, int coherent_flag)
+ dma_addr_t *handle, int coherent_flag,
+ unsigned long attrs)
 {
struct page *page;
void *addr;
@@ -1488,7 +1509,7 @@ static void *__iommu_alloc_simple(struct device *dev, 
size_t size, gfp_t gfp,
if (!addr)
return NULL;
 
-   *handle = __iommu_create_mapping(dev, &page, size);
+   *handle = __iommu_create_mapping(dev, &page, size, attrs);
if (*handle == DMA_ERROR_CODE)
goto err_mapping;
 
@@ -1522,7 +1543,7 @@ static void *__arm_iommu_alloc_attrs(struct device *dev, 
size_t size,
 
if (coherent_flag  == COHERENT || !gfpflags_allow_blocking(gfp))
return __iommu_alloc_simple(dev, size, gfp, handle,
-   coherent_flag);
+   coherent_flag, attrs);
 
/*
 * Following is a work-around (a.k.a. hack) to prevent pages
@@ -1537,7 +1558,7 @@ static void *__arm_iommu_alloc_attrs(struct device *dev, 
size_t size,
if (!pages)
return NULL;
 
-   *handle = __iommu_create_mapping(dev, pages, size);
+   *handle = __iommu_create_mapping(dev, pages, size, attrs);
if (*handle == DMA_ERROR_CODE)
goto err_buffer;
 
@@ -1672,27 +1693,6 @@ static int arm_iommu_get_sgtable(struct device *dev, 
struct sg_table *sgt,
 GFP_KERNEL);
 }
 
-static int __dma_direction_to_prot(enum dma_data_direction dir)
-{
-   int prot;
-
-   switch (dir) {
-   case DMA_BIDIRECTIONAL:
-   prot = IOMMU_READ | IOMMU_WRITE;
-   break;
-   case DMA_TO_DEVICE:
-   prot = IOMMU_READ;
-   break;
-   case DMA_FROM_DEVICE:
-   prot = IOMMU_WRITE;
-   break;
-   default:
-   prot = 0;
-   }
-
-   return prot;
-}
-
 /*
  * Map a part of the scatter-gather list into contiguous io address space
  */
@@ -1722,7 +1722,7 @@ static int __map_sg_chunk(struct device *dev, struct 
scatterlist *sg,
if (!is_coherent && (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
__dma_page_cpu_to_dev(sg_page(s), s->offset, s->length, 
dir);
 
-   prot = __dma_direction_to_prot(dir);
+   prot = __dma_info_to_prot(dir, attrs);
 
ret = iommu_map(mapping->domain, iova, phys, len, prot);
  

RE: [PATCH V8 1/9] iommu: add IOMMU_PRIV attribute

[Sricharan]

Hi Joerg,

>-Original Message-
>From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
>On Behalf Of Joerg Roedel
>Sent: Friday, January 06, 2017 4:36 PM
>To: Sricharan R 
>Cc: mitch...@codeaurora.org; pd...@codeaurora.org; vinod.k...@intel.com; 
>jgeb...@codeaurora.org; will.dea...@arm.com;
>linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org; 
>li...@armlinux.org.uk; jcro...@codeaurora.org;
>dan.j.willi...@intel.com; prat...@codeaurora.org; tz...@codeaurora.org; 
>linux-arm-ker...@lists.infradead.org;
>robin.mur...@arm.com
>Subject: Re: [PATCH V8 1/9] iommu: add IOMMU_PRIV attribute
>
>On Mon, Jan 02, 2017 at 06:42:36PM +0530, Sricharan R wrote:
>> From: Mitchel Humpherys 
>>
>> Add the IOMMU_PRIV attribute, which is used to indicate privileged
>> mappings.
>>
>> Reviewed-by: Robin Murphy 
>> Tested-by: Robin Murphy 
>> Signed-off-by: Mitchel Humpherys 
>> Acked-by: Will Deacon 
>> ---
>>  include/linux/iommu.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 0ff5111..8c15ada 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -31,6 +31,7 @@
>>  #define IOMMU_CACHE (1 << 2) /* DMA cache coherency */
>>  #define IOMMU_NOEXEC(1 << 3)
>>  #define IOMMU_MMIO  (1 << 4) /* e.g. things like MSI doorbells */
>> +#define IOMMU_PRIV  (1 << 5) /* privileged */
>
>Since this is a new generic global flag, can you please add a more
>verbose comment telling what a 'privileged' mapping is about in the
>iommu-case? We should have a clear defined semantic for this flag or we
>might end up with different meanings for it with different
>iommu-hardware.

This flag is to inform the iommu api to setup privileged mappings
for the caller which can be accessed only by the master running with
supervisor/privileged permissions and inaccessible to less privileged
execution levels.

i will repost this with comment added.

Regards,
 Sricharan

RE: [PATCH] iommu: Drop the of_iommu_{set/get}_ops() interface

[Sricharan]

Hi Lorenzo,

>With the introduction of the new iommu_{register/get}_instance()
>interface in commit e4f10ffe4c9b ("iommu: Make of_iommu_set/get_ops() DT
>agnostic") (based on struct fwnode_handle as look-up token, so firmware
>agnostic) to register IOMMU instances with the core IOMMU layer there is
>no reason to keep the old OF based interface around any longer.
>
>Convert all the IOMMU drivers (and OF IOMMU core code) that rely on the
>of_iommu_{set/get}_ops() to the new kernel interface to register/retrieve
>IOMMU instances and remove the of_iommu_{set/get}_ops() remaining glue
>code in order to complete the interface rework.
>
>Signed-off-by: Lorenzo Pieralisi 
>Cc: Matthias Brugger 
>Cc: Will Deacon 
>Cc: Robin Murphy 
>Cc: Joerg Roedel 
>Cc: Marek Szyprowski 
>---
>Exynos, msm and mtk code compile tested only owing to lack of
>test platforms, I would appreciate some help in testing this
>patch on those platforms before merging it even if it is just
>a simple interface conversion.
>

For msm,
Tested-by: Sricharan R 

Regards,
 Sricharan

RE: [PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan]

Hi Robin,

>>
>> 
>>
>>>>>>> return prot | IOMMU_READ | IOMMU_WRITE;
>>>>>>
>>>>>> ...and applying against -next now also needs this hunk:
>>>>>>
>>>>>> @@ -639,7 +639,7 @@ dma_addr_t iommu_dma_map_resource(struct device
>>>>>> *dev, phys_addr_t phys,
>>>>>>  size_t size, enum dma_data_direction dir, unsigned long attrs)
>>>>>> {
>>>>>>  return __iommu_dma_map(dev, phys, size,
>>>>>> -dma_direction_to_prot(dir, false) | IOMMU_MMIO);
>>>>>> +dma_info_to_prot(dir, false, attrs) | 
>>>>>> IOMMU_MMIO);
>>>>>> }
>>>>>>
>>>>>> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>>>>>>
>>>>>> With those two issues fixed up, I've given the series (applied to
>>>>>> next-20161213) a spin on a SMMUv3/PL330 fast model and it still checks 
>>>>>> out.
>>>>>>
>>>>>
>>>>> oops, sorry that i missed this in rebase. I can repost now with this 
>>>>> fixed,
>>>>> 'checks out' you mean something is not working correct ?
>>>>
>>>> No, I mean it _is_ still correct - I guess that's more of an idiom than
>>>> I thought :)
>>>>
>>>
>>> ha ok, thanks for the testing as well. I will just send a v8 with those two 
>>> fixed now.
>>
>> Just while checking that i have not missed anything else, realized that the
>> dma-mapping apis in arm as to be modified to pass the PRIVILIGED attributes
>> as well. While my testing path was using the iommu_map directly i was not
>> seeing this, but then i did a patch like below. I will just figure out 
>> another
>> other codebase where the master uses the dma apis, test and add it in the
>> V8 that i would send.
>
>True, adding support to 32-bit as well can't hurt, and I guess it's
>equally relevant to QC's GPU use-case. I haven't considered it myself
>because AArch32 is immune to the specific PL330 problem which caught me
>out - that subtle corner of VMSAv8 is unique to AArch64.
>
>> From: Sricharan R 
>> Date: Tue, 13 Dec 2016 23:25:01 +0530
>> Subject: [PATCH V8 6/9] arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED
>>
>> The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
>> are only accessible to privileged DMA engines.  Implementing it in 
>> dma-mapping
>> for it to get used from the dma mappings apis.
>>
>> Signed-off-by: Sricharan R 
>> ---
>>  arch/arm/mm/dma-mapping.c | 24 +++-
>>  1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index ab77100..e0d9923 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -1394,7 +1394,8 @@ static int __iommu_free_buffer(struct device *dev, 
>> struct page **pages,
>>   * Create a mapping in device IO address space for specified pages
>>   */
>>  static dma_addr_t
>> -__iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
>> +__iommu_create_mapping(struct device *dev, struct page **pages, size_t size,
>> +   unsigned long attrs)
>>  {
>>  struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
>>  unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> @@ -1419,7 +1420,7 @@ static int __iommu_free_buffer(struct device *dev, 
>> struct page **pages,
>>
>>  len = (j - i) << PAGE_SHIFT;
>>  ret = iommu_map(mapping->domain, iova, phys, len,
>> -IOMMU_READ|IOMMU_WRITE);
>> +__dma_info_to_prot(DMA_BIRECTIONAL, attrs));
>>  if (ret < 0)
>>  goto fail;
>>  iova += len;
>> @@ -1476,7 +1477,8 @@ static struct page **__iommu_get_pages(void *cpu_addr, 
>> unsigned long attrs)
>>  }
>>
>>  static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t 
>> gfp,
>> -  dma_addr_t *handle, int coherent_flag)
>> +  dma_addr_t *handle, int coherent_flag,
>> +  unsigned long attrs)
>>  {
>>  struct page *page;
>>  void *addr;
>> @@ -1488,

[PATCH V8 3/9] iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag

[Sricharan R]

From: Robin Murphy 

The short-descriptor format also allows privileged-only mappings, so
let's wire it up.

Signed-off-by: Robin Murphy 
Tested-by: Sricharan R 
---
 drivers/iommu/io-pgtable-arm-v7s.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c 
b/drivers/iommu/io-pgtable-arm-v7s.c
index 0769276..1c049e2 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -265,7 +265,9 @@ static arm_v7s_iopte arm_v7s_prot_to_pte(int prot, int lvl,
if (!(prot & IOMMU_MMIO))
pte |= ARM_V7S_ATTR_TEX(1);
if (ap) {
-   pte |= ARM_V7S_PTE_AF | ARM_V7S_PTE_AP_UNPRIV;
+   pte |= ARM_V7S_PTE_AF;
+   if (!(prot & IOMMU_PRIV))
+   pte |= ARM_V7S_PTE_AP_UNPRIV;
if (!(prot & IOMMU_WRITE))
pte |= ARM_V7S_PTE_AP_RDONLY;
}
@@ -288,6 +290,8 @@ static int arm_v7s_pte_to_prot(arm_v7s_iopte pte, int lvl)
 
if (!(attr & ARM_V7S_PTE_AP_RDONLY))
prot |= IOMMU_WRITE;
+   if (!(attr & ARM_V7S_PTE_AP_UNPRIV))
+   prot |= IOMMU_PRIV;
if ((attr & (ARM_V7S_TEX_MASK << ARM_V7S_TEX_SHIFT)) == 0)
prot |= IOMMU_MMIO;
else if (pte & ARM_V7S_ATTR_C)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V8 1/9] iommu: add IOMMU_PRIV attribute

[Sricharan R]

From: Mitchel Humpherys 

Add the IOMMU_PRIV attribute, which is used to indicate privileged
mappings.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Signed-off-by: Mitchel Humpherys 
Acked-by: Will Deacon 
---
 include/linux/iommu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0ff5111..8c15ada 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -31,6 +31,7 @@
 #define IOMMU_CACHE(1 << 2) /* DMA cache coherency */
 #define IOMMU_NOEXEC   (1 << 3)
 #define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
+#define IOMMU_PRIV (1 << 5) /* privileged */
 
 struct iommu_ops;
 struct iommu_group;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V8 8/9] iommu/arm-smmu: Set privileged attribute to 'default' instead of 'unprivileged'

[Sricharan R]

Currently the driver sets all the device transactions privileges
to UNPRIVILEGED, but there are cases where the iommu masters wants
to isolate privileged supervisor and unprivileged user.
So don't override the privileged setting to unprivileged, instead
set it to default as incoming and let it be controlled by the pagetable
settings.

Acked-by: Will Deacon 
Signed-off-by: Sricharan R 
---
 drivers/iommu/arm-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a60cded..73a0a25 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1214,7 +1214,7 @@ static int arm_smmu_domain_add_master(struct 
arm_smmu_domain *smmu_domain,
continue;
 
s2cr[idx].type = type;
-   s2cr[idx].privcfg = S2CR_PRIVCFG_UNPRIV;
+   s2cr[idx].privcfg = S2CR_PRIVCFG_DEFAULT;
s2cr[idx].cbndx = cbndx;
arm_smmu_write_s2cr(smmu, idx);
}
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V8 9/9] Revert "iommu/arm-smmu: Set PRIVCFG in stage 1 STEs"

[Sricharan R]

From: Robin Murphy 

This reverts commit df5e1a0f2a2d779ad467a691203bcbc74d75690e.

Now that proper privileged mappings can be requested via IOMMU_PRIV,
unconditionally overriding the incoming PRIVCFG becomes the wrong thing
to do, so stop it.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/arm-smmu-v3.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4d6ec44..7d45d8b 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -269,9 +269,6 @@
 #define STRTAB_STE_1_SHCFG_INCOMING1UL
 #define STRTAB_STE_1_SHCFG_SHIFT   44
 
-#define STRTAB_STE_1_PRIVCFG_UNPRIV2UL
-#define STRTAB_STE_1_PRIVCFG_SHIFT 48
-
 #define STRTAB_STE_2_S2VMID_SHIFT  0
 #define STRTAB_STE_2_S2VMID_MASK   0xUL
 #define STRTAB_STE_2_VTCR_SHIFT32
@@ -1073,9 +1070,7 @@ static void arm_smmu_write_strtab_ent(struct 
arm_smmu_device *smmu, u32 sid,
 #ifdef CONFIG_PCI_ATS
 STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT |
 #endif
-STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT |
-STRTAB_STE_1_PRIVCFG_UNPRIV <<
-STRTAB_STE_1_PRIVCFG_SHIFT);
+STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT);
 
if (smmu->features & ARM_SMMU_FEAT_STALLS)
dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V8 4/9] common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute

[Sricharan R]

From: Mitchel Humpherys 

This patch adds the DMA_ATTR_PRIVILEGED attribute to the DMA-mapping
subsystem.

Some advanced peripherals such as remote processors and GPUs perform
accesses to DMA buffers in both privileged "supervisor" and unprivileged
"user" modes.  This attribute is used to indicate to the DMA-mapping
subsystem that the buffer is fully accessible at the elevated privilege
level (and ideally inaccessible or at least read-only at the
lesser-privileged levels).

Cc: linux-...@vger.kernel.org
Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---
 Documentation/DMA-attributes.txt | 10 ++
 include/linux/dma-mapping.h  |  7 +++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/DMA-attributes.txt b/Documentation/DMA-attributes.txt
index 98bf7ac..44c6bc4 100644
--- a/Documentation/DMA-attributes.txt
+++ b/Documentation/DMA-attributes.txt
@@ -143,3 +143,13 @@ So, this provides a way for drivers to avoid those error 
messages on calls
 where allocation failures are not a problem, and shouldn't bother the logs.
 
 NOTE: At the moment DMA_ATTR_NO_WARN is only implemented on PowerPC.
+
+DMA_ATTR_PRIVILEGED
+--
+
+Some advanced peripherals such as remote processors and GPUs perform
+accesses to DMA buffers in both privileged "supervisor" and unprivileged
+"user" modes.  This attribute is used to indicate to the DMA-mapping
+subsystem that the buffer is fully accessible at the elevated privilege
+level (and ideally inaccessible or at least read-only at the
+lesser-privileged levels).
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 10c5a17b1..c24721a 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -63,6 +63,13 @@
 #define DMA_ATTR_NO_WARN   (1UL << 8)
 
 /*
+ * DMA_ATTR_PRIVILEGED: used to indicate that the buffer is fully
+ * accessible at an elevated privilege level (and ideally inaccessible or
+ * at least read-only at lesser-privileged levels).
+ */
+#define DMA_ATTR_PRIVILEGED(1UL << 9)
+
+/*
  * A dma_addr_t can hold any valid DMA or bus address for the platform.
  * It can be given to a device to use as a DMA source or target.  A CPU cannot
  * reference a dma_addr_t directly because there may be translation between
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V8 6/9] arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan R]

The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines.  Adding it to the
arm dma-mapping.c so that the ARM32 DMA IOMMU mapper can make use of it.

Signed-off-by: Sricharan R 
---
 arch/arm/mm/dma-mapping.c | 60 +++
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ab77100..82d3e79 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1171,6 +1171,25 @@ static int __init dma_debug_do_init(void)
 
 #ifdef CONFIG_ARM_DMA_USE_IOMMU
 
+static int __dma_info_to_prot(enum dma_data_direction dir, unsigned long attrs)
+{
+   int prot = 0;
+
+   if (attrs & DMA_ATTR_PRIVILEGED)
+   prot |= IOMMU_PRIV;
+
+   switch (dir) {
+   case DMA_BIDIRECTIONAL:
+   return prot | IOMMU_READ | IOMMU_WRITE;
+   case DMA_TO_DEVICE:
+   return prot | IOMMU_READ;
+   case DMA_FROM_DEVICE:
+   return prot | IOMMU_WRITE;
+   default:
+   return prot;
+   }
+}
+
 /* IOMMU */
 
 static int extend_iommu_mapping(struct dma_iommu_mapping *mapping);
@@ -1394,7 +1413,8 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
  * Create a mapping in device IO address space for specified pages
  */
 static dma_addr_t
-__iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
+__iommu_create_mapping(struct device *dev, struct page **pages, size_t size,
+  unsigned long attrs)
 {
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
@@ -1419,7 +1439,7 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
 
len = (j - i) << PAGE_SHIFT;
ret = iommu_map(mapping->domain, iova, phys, len,
-   IOMMU_READ|IOMMU_WRITE);
+   __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs));
if (ret < 0)
goto fail;
iova += len;
@@ -1476,7 +1496,8 @@ static struct page **__iommu_get_pages(void *cpu_addr, 
unsigned long attrs)
 }
 
 static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
- dma_addr_t *handle, int coherent_flag)
+ dma_addr_t *handle, int coherent_flag,
+ unsigned long attrs)
 {
struct page *page;
void *addr;
@@ -1488,7 +1509,7 @@ static void *__iommu_alloc_simple(struct device *dev, 
size_t size, gfp_t gfp,
if (!addr)
return NULL;
 
-   *handle = __iommu_create_mapping(dev, &page, size);
+   *handle = __iommu_create_mapping(dev, &page, size, attrs);
if (*handle == DMA_ERROR_CODE)
goto err_mapping;
 
@@ -1522,7 +1543,7 @@ static void *__arm_iommu_alloc_attrs(struct device *dev, 
size_t size,
 
if (coherent_flag  == COHERENT || !gfpflags_allow_blocking(gfp))
return __iommu_alloc_simple(dev, size, gfp, handle,
-   coherent_flag);
+   coherent_flag, attrs);
 
/*
 * Following is a work-around (a.k.a. hack) to prevent pages
@@ -1537,7 +1558,7 @@ static void *__arm_iommu_alloc_attrs(struct device *dev, 
size_t size,
if (!pages)
return NULL;
 
-   *handle = __iommu_create_mapping(dev, pages, size);
+   *handle = __iommu_create_mapping(dev, pages, size, attrs);
if (*handle == DMA_ERROR_CODE)
goto err_buffer;
 
@@ -1672,27 +1693,6 @@ static int arm_iommu_get_sgtable(struct device *dev, 
struct sg_table *sgt,
 GFP_KERNEL);
 }
 
-static int __dma_direction_to_prot(enum dma_data_direction dir)
-{
-   int prot;
-
-   switch (dir) {
-   case DMA_BIDIRECTIONAL:
-   prot = IOMMU_READ | IOMMU_WRITE;
-   break;
-   case DMA_TO_DEVICE:
-   prot = IOMMU_READ;
-   break;
-   case DMA_FROM_DEVICE:
-   prot = IOMMU_WRITE;
-   break;
-   default:
-   prot = 0;
-   }
-
-   return prot;
-}
-
 /*
  * Map a part of the scatter-gather list into contiguous io address space
  */
@@ -1722,7 +1722,7 @@ static int __map_sg_chunk(struct device *dev, struct 
scatterlist *sg,
if (!is_coherent && (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
__dma_page_cpu_to_dev(sg_page(s), s->offset, s->length, 
dir);
 
-   prot = __dma_direction_to_prot(dir);
+   prot = __dma_info_to_prot(dir, attrs);
 
ret = iommu_map(mapping->domain, iova, phys, len, prot);
i

[PATCH V8 5/9] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan R]

From: Mitchel Humpherys 

The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines.  Implement it in
dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---
 arch/arm64/mm/dma-mapping.c |  6 +++---
 drivers/iommu/dma-iommu.c   | 12 +---
 include/linux/dma-iommu.h   |  3 ++-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 290a84f..49c6f75 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -557,7 +557,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t 
size,
 unsigned long attrs)
 {
bool coherent = is_device_dma_coherent(dev);
-   int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+   int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
size_t iosize = size;
void *addr;
 
@@ -711,7 +711,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, 
struct page *page,
   unsigned long attrs)
 {
bool coherent = is_device_dma_coherent(dev);
-   int prot = dma_direction_to_prot(dir, coherent);
+   int prot = dma_info_to_prot(dir, coherent, attrs);
dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
 
if (!iommu_dma_mapping_error(dev, dev_addr) &&
@@ -769,7 +769,7 @@ static int __iommu_map_sg_attrs(struct device *dev, struct 
scatterlist *sgl,
__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
 
return iommu_dma_map_sg(dev, sgl, nelems,
-   dma_direction_to_prot(dir, coherent));
+   dma_info_to_prot(dir, coherent, attrs));
 }
 
 static void __iommu_unmap_sg_attrs(struct device *dev,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 2db0d64..3006eee 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -181,16 +181,22 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
dma_addr_t base,
 EXPORT_SYMBOL(iommu_dma_init_domain);
 
 /**
- * dma_direction_to_prot - Translate DMA API directions to IOMMU API page flags
+ * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU API
+ *page flags.
  * @dir: Direction of DMA transfer
  * @coherent: Is the DMA master cache-coherent?
+ * @attrs: DMA attributes for the mapping
  *
  * Return: corresponding IOMMU API page protection flags
  */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+unsigned long attrs)
 {
int prot = coherent ? IOMMU_CACHE : 0;
 
+   if (attrs & DMA_ATTR_PRIVILEGED)
+   prot |= IOMMU_PRIV;
+
switch (dir) {
case DMA_BIDIRECTIONAL:
return prot | IOMMU_READ | IOMMU_WRITE;
@@ -633,7 +639,7 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, 
phys_addr_t phys,
size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
return __iommu_dma_map(dev, phys, size,
-   dma_direction_to_prot(dir, false) | IOMMU_MMIO);
+   dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
 }
 
 void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 7f7e9a7..c5511e1 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -34,7 +34,8 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
dma_addr_t base,
u64 size, struct device *dev);
 
 /* General helpers for DMA-API <-> IOMMU-API interaction */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent);
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+unsigned long attrs);
 
 /*
  * These implement the bulk of the relevant DMA mapping callbacks, but require
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V8 7/9] dmaengine: pl330: Make sure microcode is privileged

[Sricharan R]

From: Mitchel Humpherys 

The PL330 is hard-wired such that instruction fetches on both the
manager and channel threads go out onto the bus with the "privileged"
bit set. This can become troublesome once there is an IOMMU or other
form of memory protection downstream, since those will typically be
programmed by the DMA mapping subsystem in the expectation of normal
unprivileged transactions (such as the PL330 channel threads' own data
accesses as currently configured by this driver).

To avoid the case of, say, an IOMMU blocking an unexpected privileged
transaction with a permission fault, use the newly-introduced
DMA_ATTR_PRIVILEGED attribute for the mapping of our microcode buffer.
That way the DMA layer can do whatever it needs to do to make things
continue to work as expected on more complex systems.

Cc: Dan Williams 
Cc: Vinod Koul 
Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Acked-by: Vinod Koul 
Signed-off-by: Mitchel Humpherys 
[rm: remove now-redundant local variable, clarify commit message]
Signed-off-by: Robin Murphy 
---
 drivers/dma/pl330.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 87fd015..5a90d0c 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1864,9 +1864,10 @@ static int dmac_alloc_resources(struct pl330_dmac *pl330)
 * Alloc MicroCode buffer for 'chans' Channel threads.
 * A channel's buffer offset is (Channel_Id * MCODE_BUFF_PERCHAN)
 */
-   pl330->mcode_cpu = dma_alloc_coherent(pl330->ddma.dev,
+   pl330->mcode_cpu = dma_alloc_attrs(pl330->ddma.dev,
chans * pl330->mcbufsz,
-   &pl330->mcode_bus, GFP_KERNEL);
+   &pl330->mcode_bus, GFP_KERNEL,
+   DMA_ATTR_PRIVILEGED);
if (!pl330->mcode_cpu) {
dev_err(pl330->ddma.dev, "%s:%d Can't allocate memory!\n",
__func__, __LINE__);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V8 2/9] iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

[Sricharan R]

From: Jeremy Gebben 

Allow the creation of privileged mode mappings, for stage 1 only.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Jeremy Gebben 
---
 drivers/iommu/io-pgtable-arm.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index a40ce34..feacc54 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -350,11 +350,14 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct 
arm_lpae_io_pgtable *data,
 
if (data->iop.fmt == ARM_64_LPAE_S1 ||
data->iop.fmt == ARM_32_LPAE_S1) {
-   pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
+   pte = ARM_LPAE_PTE_nG;
 
if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
pte |= ARM_LPAE_PTE_AP_RDONLY;
 
+   if (!(prot & IOMMU_PRIV))
+   pte |= ARM_LPAE_PTE_AP_UNPRIV;
+
if (prot & IOMMU_MMIO)
pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V8 0/9] Add support for privileged mappings

[Sricharan R]

This series is a resend of the V5 that Mitch sent sometime back [2]
All the patches are the same and i have just rebased. Redid patch [3],
as it does not apply in this code base. Added a couple of more patches
[4], [5] from Robin for adding the privileged attributes to armv7s format
and arm-smmuv3 revert. Added a patch for passing in the privileged
attributes from arm32 dma-mapping apis as well.

The following patch to the ARM SMMU driver:

commit d346180e70b91b3d5a1ae7e5603e65593d4622bc
Author: Robin Murphy 
Date:   Tue Jan 26 18:06:34 2016 +

iommu/arm-smmu: Treat all device transactions as unprivileged

started forcing all SMMU transactions to come through as "unprivileged".
The rationale given was that:

  (1) There is no way in the IOMMU API to even request privileged
  mappings.

  (2) It's difficult to implement a DMA mapper that correctly models the
  ARM VMSAv8 behavior of unprivileged-writeable =>
  privileged-execute-never.

This series rectifies (1) by introducing an IOMMU API for privileged
mappings and implements it in io-pgtable-arm.

This series rectifies (2) by introducing a new dma attribute
(DMA_ATTR_PRIVILEGED) for users of the DMA API that need privileged
mappings which are inaccessible to lesser-privileged execution levels, and
implements it in the arm64 IOMMU DMA mapper.  The one known user (pl330.c)
is converted over to the new attribute.

Jordan and Jeremy can provide more info on the use case if needed, but the
high level is that it's a security feature to prevent attacks such as [1].

Note that, i tested this on arm64 with arm-smmuv2, short descriptor changes,
tested this on arm32 platform as well and do not have an platform to test
this with arm-smmuv3.

[1] https://github.com/robclark/kilroy
[2] https://lkml.org/lkml/2016/7/27/590
[3] https://patchwork.kernel.org/patch/9250493/
[4] 
http://www.linux-arm.org/git?p=linux-rm.git;a=commit;h=1291bd74f05d31da1dab3df02987cba5bd25849b
[5] 
http://www.linux-arm.org/git?p=linux-rm.git;a=commit;h=a79c1c6333f26849dba418cd92de26b60f5954f3

Changelog:
 v7..v8
- Added a patch for passing in the privileged attributes from arm32
  dma-mapping apis as well.

 v6..v7
- Added couple of more patches, picked up acks, updated commit log

 v5..v6
- Rebased all the patches and redid 6/6 as it does not apply in
  this code base. 

 v4..v5
- Simplified patch 4/6 (suggested by Robin Murphy).

 v3..v4
- Rebased and reworked on linux next due to the dma attrs rework going
  on over there.  Patches changed: 3/6, 4/6, and 5/6.

 v2..v3
- Incorporated feedback from Robin:
  * Various comments and re-wordings.
  * Use existing bit definitions for IOMMU_PRIV implementation
in io-pgtable-arm.
  * Renamed and redocumented dma_direction_to_prot.
  * Don't worry about executability in new DMA attr.

 v1..v2
- Added a new DMA attribute to make executable privileged mappings
  work, and use that in the pl330 driver (suggested by Will).

Jeremy Gebben (1):
  iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

Mitchel Humpherys (4):
  iommu: add IOMMU_PRIV attribute
  common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute
  arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
  dmaengine: pl330: Make sure microcode is privileged

Robin Murphy (2):
  iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag
  Revert "iommu/arm-smmu: Set PRIVCFG in stage 1 STEs"

Sricharan R (2):
  arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED
  iommu/arm-smmu: Set privileged attribute to 'default' instead of
'unprivileged'

 Documentation/DMA-attributes.txt   | 10 +++
 arch/arm/mm/dma-mapping.c  | 60 +++---
 arch/arm64/mm/dma-mapping.c|  6 ++--
 drivers/dma/pl330.c|  5 ++--
 drivers/iommu/arm-smmu-v3.c|  7 +
 drivers/iommu/arm-smmu.c   |  2 +-
 drivers/iommu/dma-iommu.c  | 12 ++--
 drivers/iommu/io-pgtable-arm-v7s.c |  6 +++-
 drivers/iommu/io-pgtable-arm.c |  5 +++-
 include/linux/dma-iommu.h  |  3 +-
 include/linux/dma-mapping.h|  7 +
 include/linux/iommu.h  |  1 +
 12 files changed, 76 insertions(+), 48 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan]

Hi Robin,



>>>>>   return prot | IOMMU_READ | IOMMU_WRITE;
>>>>
>>>> ...and applying against -next now also needs this hunk:
>>>>
>>>> @@ -639,7 +639,7 @@ dma_addr_t iommu_dma_map_resource(struct device
>>>> *dev, phys_addr_t phys,
>>>>size_t size, enum dma_data_direction dir, unsigned long attrs)
>>>> {
>>>>return __iommu_dma_map(dev, phys, size,
>>>> -  dma_direction_to_prot(dir, false) | IOMMU_MMIO);
>>>> +  dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
>>>> }
>>>>
>>>> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>>>>
>>>> With those two issues fixed up, I've given the series (applied to
>>>> next-20161213) a spin on a SMMUv3/PL330 fast model and it still checks out.
>>>>
>>>
>>> oops, sorry that i missed this in rebase. I can repost now with this fixed,
>>> 'checks out' you mean something is not working correct ?
>>
>>No, I mean it _is_ still correct - I guess that's more of an idiom than
>>I thought :)
>>
>
>ha ok, thanks for the testing as well. I will just send a v8 with those two 
>fixed now.

Just while checking that i have not missed anything else, realized that the
dma-mapping apis in arm as to be modified to pass the PRIVILIGED attributes
as well. While my testing path was using the iommu_map directly i was not
seeing this, but then i did a patch like below. I will just figure out another
other codebase where the master uses the dma apis, test and add it in the
V8 that i would send.


From: Sricharan R 
Date: Tue, 13 Dec 2016 23:25:01 +0530
Subject: [PATCH V8 6/9] arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED

The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines.  Implementing it in dma-mapping
for it to get used from the dma mappings apis.

Signed-off-by: Sricharan R 
---
 arch/arm/mm/dma-mapping.c | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ab77100..e0d9923 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1394,7 +1394,8 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
  * Create a mapping in device IO address space for specified pages
  */
 static dma_addr_t
-__iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
+__iommu_create_mapping(struct device *dev, struct page **pages, size_t size,
+  unsigned long attrs)
 {
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
@@ -1419,7 +1420,7 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
 
len = (j - i) << PAGE_SHIFT;
ret = iommu_map(mapping->domain, iova, phys, len,
-   IOMMU_READ|IOMMU_WRITE);
+   __dma_info_to_prot(DMA_BIRECTIONAL, attrs));
if (ret < 0)
goto fail;
iova += len;
@@ -1476,7 +1477,8 @@ static struct page **__iommu_get_pages(void *cpu_addr, 
unsigned long attrs)
 }
 
 static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
- dma_addr_t *handle, int coherent_flag)
+ dma_addr_t *handle, int coherent_flag,
+ unsigned long attrs)
 {
struct page *page;
void *addr;
@@ -1488,7 +1490,7 @@ static void *__iommu_alloc_simple(struct device *dev, 
size_t size, gfp_t gfp,
if (!addr)
return NULL;
 
-   *handle = __iommu_create_mapping(dev, &page, size);
+   *handle = __iommu_create_mapping(dev, &page, size, attrs);
if (*handle == DMA_ERROR_CODE)
goto err_mapping;
 
@@ -1522,7 +1524,8 @@ static void *__arm_iommu_alloc_attrs(struct device *dev, 
size_t size,
 
if (coherent_flag  == COHERENT || !gfpflags_allow_blocking(gfp))
return __iommu_alloc_simple(dev, size, gfp, handle,
-   coherent_flag);
+   coherent_flag,
+   attrs);
 
/*
 * Following is a work-around (a.k.a. hack) to prevent pages
@@ -1672,10 +1675,13 @@ static int arm_iommu_get_sgtable(struct device *dev, 
struct sg_table *sgt,
 GFP_KERNEL);
 }
 
-static int __dma_direction_to_prot(enum dma_data_direction dir)
+static int __dma_info_to_prot(enum dma_data_direction dir, unsigned long

RE: [PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan]

Hi,

>
>On 13/12/16 14:38, Sricharan wrote:
>> Hi Robin,
>>
>>> -Original Message-
>>> From: linux-arm-kernel 
>>> [mailto:linux-arm-kernel-boun...@lists.infradead.org] On Behalf Of Robin 
>>> Murphy
>>> Sent: Tuesday, December 13, 2016 7:33 PM
>>> To: Sricharan R ; jcro...@codeaurora.org; 
>>> pd...@codeaurora.org; jgeb...@codeaurora.org;
>>> j...@8bytes.org; linux-kernel@vger.kernel.org; prat...@codeaurora.org; 
>>> io...@lists.linux-foundation.org;
>tz...@codeaurora.org;
>>> linux-arm-ker...@lists.infradead.org; will.dea...@arm.com; 
>>> mitch...@codeaurora.org; vinod.k...@intel.com
>>> Cc: dan.j.willi...@intel.com
>>> Subject: Re: [PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
>>>
>>> On 12/12/16 18:38, Sricharan R wrote:
>>>> From: Mitchel Humpherys 
>>>>
>>>> The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
>>>> are only accessible to privileged DMA engines.  Implement it in
>>>> dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.
>>>>
>>>> Reviewed-by: Robin Murphy 
>>>> Tested-by: Robin Murphy 
>>>> Acked-by: Will Deacon 
>>>> Signed-off-by: Mitchel Humpherys 
>>>> ---
>>>>  arch/arm64/mm/dma-mapping.c |  6 +++---
>>>>  drivers/iommu/dma-iommu.c   | 10 --
>>>>  include/linux/dma-iommu.h   |  3 ++-
>>>>  3 files changed, 13 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>>>> index 401f79a..ae76ead 100644
>>>> --- a/arch/arm64/mm/dma-mapping.c
>>>> +++ b/arch/arm64/mm/dma-mapping.c
>>>> @@ -557,7 +557,7 @@ static void *__iommu_alloc_attrs(struct device *dev, 
>>>> size_t size,
>>>> unsigned long attrs)
>>>>  {
>>>>bool coherent = is_device_dma_coherent(dev);
>>>> -  int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
>>>> +  int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
>>>>size_t iosize = size;
>>>>void *addr;
>>>>
>>>> @@ -711,7 +711,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, 
>>>> struct page *page,
>>>>   unsigned long attrs)
>>>>  {
>>>>bool coherent = is_device_dma_coherent(dev);
>>>> -  int prot = dma_direction_to_prot(dir, coherent);
>>>> +  int prot = dma_info_to_prot(dir, coherent, attrs);
>>>>dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
>>>>
>>>>if (!iommu_dma_mapping_error(dev, dev_addr) &&
>>>> @@ -769,7 +769,7 @@ static int __iommu_map_sg_attrs(struct device *dev, 
>>>> struct scatterlist *sgl,
>>>>__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
>>>>
>>>>return iommu_dma_map_sg(dev, sgl, nelems,
>>>> -  dma_direction_to_prot(dir, coherent));
>>>> +  dma_info_to_prot(dir, coherent, attrs));
>>>>  }
>>>>
>>>>  static void __iommu_unmap_sg_attrs(struct device *dev,
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index d2a7a46..756d5e0 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -182,16 +182,22 @@ int iommu_dma_init_domain(struct iommu_domain 
>>>> *domain, dma_addr_t base,
>>>>  EXPORT_SYMBOL(iommu_dma_init_domain);
>>>>
>>>>  /**
>>>> - * dma_direction_to_prot - Translate DMA API directions to IOMMU API page 
>>>> flags
>>>> + * dma_info_to_prot - Translate DMA API directions and attributes to 
>>>> IOMMU API
>>>> + *page flags.
>>>>   * @dir: Direction of DMA transfer
>>>>   * @coherent: Is the DMA master cache-coherent?
>>>> + * @attrs: DMA attributes for the mapping
>>>>   *
>>>>   * Return: corresponding IOMMU API page protection flags
>>>>   */
>>>> -int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
>>>> +int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
>>>> +   unsigned long attrs)
>>>>  {
>>>>int prot = coherent ? IOMMU_CACHE : 0;
>>>&

RE: [PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan]

Hi Robin,

>-Original Message-
>From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
>On Behalf Of Robin Murphy
>Sent: Tuesday, December 13, 2016 7:33 PM
>To: Sricharan R ; jcro...@codeaurora.org; 
>pd...@codeaurora.org; jgeb...@codeaurora.org;
>j...@8bytes.org; linux-kernel@vger.kernel.org; prat...@codeaurora.org; 
>io...@lists.linux-foundation.org; tz...@codeaurora.org;
>linux-arm-ker...@lists.infradead.org; will.dea...@arm.com; 
>mitch...@codeaurora.org; vinod.k...@intel.com
>Cc: dan.j.willi...@intel.com
>Subject: Re: [PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
>
>On 12/12/16 18:38, Sricharan R wrote:
>> From: Mitchel Humpherys 
>>
>> The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
>> are only accessible to privileged DMA engines.  Implement it in
>> dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.
>>
>> Reviewed-by: Robin Murphy 
>> Tested-by: Robin Murphy 
>> Acked-by: Will Deacon 
>> Signed-off-by: Mitchel Humpherys 
>> ---
>>  arch/arm64/mm/dma-mapping.c |  6 +++---
>>  drivers/iommu/dma-iommu.c   | 10 --
>>  include/linux/dma-iommu.h   |  3 ++-
>>  3 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index 401f79a..ae76ead 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -557,7 +557,7 @@ static void *__iommu_alloc_attrs(struct device *dev, 
>> size_t size,
>>   unsigned long attrs)
>>  {
>>  bool coherent = is_device_dma_coherent(dev);
>> -int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
>> +int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
>>  size_t iosize = size;
>>  void *addr;
>>
>> @@ -711,7 +711,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, 
>> struct page *page,
>> unsigned long attrs)
>>  {
>>  bool coherent = is_device_dma_coherent(dev);
>> -int prot = dma_direction_to_prot(dir, coherent);
>> +int prot = dma_info_to_prot(dir, coherent, attrs);
>>  dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
>>
>>  if (!iommu_dma_mapping_error(dev, dev_addr) &&
>> @@ -769,7 +769,7 @@ static int __iommu_map_sg_attrs(struct device *dev, 
>> struct scatterlist *sgl,
>>  __iommu_sync_sg_for_device(dev, sgl, nelems, dir);
>>
>>  return iommu_dma_map_sg(dev, sgl, nelems,
>> -dma_direction_to_prot(dir, coherent));
>> +dma_info_to_prot(dir, coherent, attrs));
>>  }
>>
>>  static void __iommu_unmap_sg_attrs(struct device *dev,
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index d2a7a46..756d5e0 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -182,16 +182,22 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
>> dma_addr_t base,
>>  EXPORT_SYMBOL(iommu_dma_init_domain);
>>
>>  /**
>> - * dma_direction_to_prot - Translate DMA API directions to IOMMU API page 
>> flags
>> + * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU 
>> API
>> + *page flags.
>>   * @dir: Direction of DMA transfer
>>   * @coherent: Is the DMA master cache-coherent?
>> + * @attrs: DMA attributes for the mapping
>>   *
>>   * Return: corresponding IOMMU API page protection flags
>>   */
>> -int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
>> +int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
>> + unsigned long attrs)
>>  {
>>  int prot = coherent ? IOMMU_CACHE : 0;
>>
>> +if (attrs & DMA_ATTR_PRIVILEGED)
>> +prot |= IOMMU_PRIV;
>> +
>>  switch (dir) {
>>  case DMA_BIDIRECTIONAL:
>>  return prot | IOMMU_READ | IOMMU_WRITE;
>
>...and applying against -next now also needs this hunk:
>
>@@ -639,7 +639,7 @@ dma_addr_t iommu_dma_map_resource(struct device
>*dev, phys_addr_t phys,
>   size_t size, enum dma_data_direction dir, unsigned long attrs)
> {
>   return __iommu_dma_map(dev, phys, size,
>-  dma_direction_to_prot(dir, false) | IOMMU_MMIO);
>+  dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
> }
>
> void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>
>With those two issues fixed up, I've given the series (applied to
>next-20161213) a spin on a SMMUv3/PL330 fast model and it still checks out.
>

oops, sorry that i missed this in rebase. I can repost now with this fixed,
'checks out' you mean something is not working correct ?

Regards,
 Sricharan  

RE: [RESEND PATCH V6 0/6] Add support for privileged mappings

[Sricharan]

Hi Robin,


>Hi Robin,
>
>>>> Hi Sricharan,
>>>>
>>>> On 02/12/16 14:55, Sricharan R wrote:
>>>>> This series is a resend of the V5 that Mitch sent sometime back [2]
>>>>> All the patches are the same and i have just rebased. Not sure why this
>>>>> finally did not make it last time. The last patch in the previous
>>>>> series does not apply now [3], so just redid that. Also Copied the tags
>>>>> that he had from last time as well.
>>>>
>>>> Heh, I was assuming this would be down to me to pick up. Vinod did have
>>>> some complaints last time about the commit message on the PL330 patch -
>>>> I did get as far as rewriting that and reworking onto my SMMU
>>>> changes[1], I just hadn't got round to sending it, so it fell onto the
>>>> "after the next merge window" pile.
>>>>
>>>> I'd give some review comments, but they'd essentially be a diff against
>>>> that branch :)
>>>>
>>>
>>> Sure, i did not knew that you were on this already. I can repost with the 
>>> diff
>>> from your branch taken in or wait for you as well. I am fine with either 
>>> ways
>>> that you suggest.
>>>
>>> I checked the patches against your branch, i see that the changes are,
>>>
>>> 1) one patch for implementing it for armv7s descriptor
>>> 2) Changes on pl330 patch commit logs and
>>> 3) One patch for doing the revert on arm-smmuv3 as well.
>>
>>If you want to pick up my short-descriptor and SMMUv3 patches and run
>>with them you're more than welcome - the rest is just cosmetic stuff
>>which doesn't really matter, especially as it's picking up acks as-is.
>>
>
>Sure, i will repost with additional stuff picked up from your branch and
>the acks as well.

Posted V7 [1], while i tested the additional short descriptor changes,
but was not able to get hold of board with arm-smmuv3 in it.

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1291140.html

Regards,
 Sricharan

[PATCH V7 3/8] iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag

[Sricharan R]

From: Robin Murphy 

The short-descriptor format also allows privileged-only mappings, so
let's wire it up.

Signed-off-by: Robin Murphy 
Tested-by: Sricharan R 
---
 drivers/iommu/io-pgtable-arm-v7s.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c 
b/drivers/iommu/io-pgtable-arm-v7s.c
index f50e51c..1177782 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -265,7 +265,9 @@ static arm_v7s_iopte arm_v7s_prot_to_pte(int prot, int lvl,
if (!(prot & IOMMU_MMIO))
pte |= ARM_V7S_ATTR_TEX(1);
if (ap) {
-   pte |= ARM_V7S_PTE_AF | ARM_V7S_PTE_AP_UNPRIV;
+   pte |= ARM_V7S_PTE_AF;
+   if (!(prot & IOMMU_PRIV))
+   pte |= ARM_V7S_PTE_AP_UNPRIV;
if (!(prot & IOMMU_WRITE))
pte |= ARM_V7S_PTE_AP_RDONLY;
}
@@ -288,6 +290,8 @@ static int arm_v7s_pte_to_prot(arm_v7s_iopte pte, int lvl)
 
if (!(attr & ARM_V7S_PTE_AP_RDONLY))
prot |= IOMMU_WRITE;
+   if (!(attr & ARM_V7S_PTE_AP_UNPRIV))
+   prot |= IOMMU_PRIV;
if ((attr & (ARM_V7S_TEX_MASK << ARM_V7S_TEX_SHIFT)) == 0)
prot |= IOMMU_MMIO;
else if (pte & ARM_V7S_ATTR_C)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V7 8/8] iommu/arm-smmu: Revert "iommu/arm-smmu: Set PRIVCFG in stage 1 STEs"

[Sricharan R]

From: Robin Murphy 

Now that proper privileged mappings can be requested via IOMMU_PRIV,
unconditionally overriding the incoming PRIVCFG becomes the wrong thing
to do, so stop it.

This reverts commit df5e1a0f2a2d779ad467a691203bcbc74d75690e.

Signed-off-by: Robin Murphy 
---
 drivers/iommu/arm-smmu-v3.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 257a6a3..0eca0553 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -269,9 +269,6 @@
 #define STRTAB_STE_1_SHCFG_INCOMING1UL
 #define STRTAB_STE_1_SHCFG_SHIFT   44
 
-#define STRTAB_STE_1_PRIVCFG_UNPRIV2UL
-#define STRTAB_STE_1_PRIVCFG_SHIFT 48
-
 #define STRTAB_STE_2_S2VMID_SHIFT  0
 #define STRTAB_STE_2_S2VMID_MASK   0xUL
 #define STRTAB_STE_2_VTCR_SHIFT32
@@ -1073,9 +1070,7 @@ static void arm_smmu_write_strtab_ent(struct 
arm_smmu_device *smmu, u32 sid,
 #ifdef CONFIG_PCI_ATS
 STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT |
 #endif
-STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT |
-STRTAB_STE_1_PRIVCFG_UNPRIV <<
-STRTAB_STE_1_PRIVCFG_SHIFT);
+STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT);
 
if (smmu->features & ARM_SMMU_FEAT_STALLS)
dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V7 7/8] iommu/arm-smmu: Set privileged attribute to 'default' instead of 'unprivileged'

[Sricharan R]

Currently the driver sets all the device transactions privileges
to UNPRIVILEGED, but there are cases where the iommu masters wants
to isolate privileged supervisor and unprivileged user.
So don't override the privileged setting to unprivileged, instead
set it to default as incoming and let it be controlled by the pagetable
settings.

Acked-by: Will Deacon 
Signed-off-by: Sricharan R 
---
 drivers/iommu/arm-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index eaa8f44..8bb0eea 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1213,7 +1213,7 @@ static int arm_smmu_domain_add_master(struct 
arm_smmu_domain *smmu_domain,
continue;
 
s2cr[idx].type = type;
-   s2cr[idx].privcfg = S2CR_PRIVCFG_UNPRIV;
+   s2cr[idx].privcfg = S2CR_PRIVCFG_DEFAULT;
s2cr[idx].cbndx = cbndx;
arm_smmu_write_s2cr(smmu, idx);
}
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V7 6/8] dmaengine: pl330: Make sure microcode is privileged

[Sricharan R]

From: Mitchel Humpherys 

The PL330 is hard-wired such that instruction fetches on both the
manager and channel threads go out onto the bus with the "privileged"
bit set. This can become troublesome once there is an IOMMU or other
form of memory protection downstream, since those will typically be
programmed by the DMA mapping subsystem in the expectation of normal
unprivileged transactions (such as the PL330 channel threads' own data
accesses as currently configured by this driver).

To avoid the case of, say, an IOMMU blocking an unexpected privileged
transaction with a permission fault, use the newly-introduced
DMA_ATTR_PRIVILEGED attribute for the mapping of our microcode buffer.
That way the DMA layer can do whatever it needs to do to make things
continue to work as expected on more complex systems.

Cc: Dan Williams 
Cc: Vinod Koul 
Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Acked-by: Vinod Koul 
Signed-off-by: Mitchel Humpherys 
[rm: remove now-redundant local variable, clarify commit message]
Signed-off-by: Robin Murphy 
---
 drivers/dma/pl330.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 030fe05..1e5ae0c 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1859,9 +1859,10 @@ static int dmac_alloc_resources(struct pl330_dmac *pl330)
 * Alloc MicroCode buffer for 'chans' Channel threads.
 * A channel's buffer offset is (Channel_Id * MCODE_BUFF_PERCHAN)
 */
-   pl330->mcode_cpu = dma_alloc_coherent(pl330->ddma.dev,
+   pl330->mcode_cpu = dma_alloc_attrs(pl330->ddma.dev,
chans * pl330->mcbufsz,
-   &pl330->mcode_bus, GFP_KERNEL);
+   &pl330->mcode_bus, GFP_KERNEL,
+   DMA_ATTR_PRIVILEGED);
if (!pl330->mcode_cpu) {
dev_err(pl330->ddma.dev, "%s:%d Can't allocate memory!\n",
__func__, __LINE__);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V7 5/8] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan R]

From: Mitchel Humpherys 

The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines.  Implement it in
dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---
 arch/arm64/mm/dma-mapping.c |  6 +++---
 drivers/iommu/dma-iommu.c   | 10 --
 include/linux/dma-iommu.h   |  3 ++-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 401f79a..ae76ead 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -557,7 +557,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t 
size,
 unsigned long attrs)
 {
bool coherent = is_device_dma_coherent(dev);
-   int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+   int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
size_t iosize = size;
void *addr;
 
@@ -711,7 +711,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, 
struct page *page,
   unsigned long attrs)
 {
bool coherent = is_device_dma_coherent(dev);
-   int prot = dma_direction_to_prot(dir, coherent);
+   int prot = dma_info_to_prot(dir, coherent, attrs);
dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
 
if (!iommu_dma_mapping_error(dev, dev_addr) &&
@@ -769,7 +769,7 @@ static int __iommu_map_sg_attrs(struct device *dev, struct 
scatterlist *sgl,
__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
 
return iommu_dma_map_sg(dev, sgl, nelems,
-   dma_direction_to_prot(dir, coherent));
+   dma_info_to_prot(dir, coherent, attrs));
 }
 
 static void __iommu_unmap_sg_attrs(struct device *dev,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index d2a7a46..756d5e0 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -182,16 +182,22 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
dma_addr_t base,
 EXPORT_SYMBOL(iommu_dma_init_domain);
 
 /**
- * dma_direction_to_prot - Translate DMA API directions to IOMMU API page flags
+ * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU API
+ *page flags.
  * @dir: Direction of DMA transfer
  * @coherent: Is the DMA master cache-coherent?
+ * @attrs: DMA attributes for the mapping
  *
  * Return: corresponding IOMMU API page protection flags
  */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+unsigned long attrs)
 {
int prot = coherent ? IOMMU_CACHE : 0;
 
+   if (attrs & DMA_ATTR_PRIVILEGED)
+   prot |= IOMMU_PRIV;
+
switch (dir) {
case DMA_BIDIRECTIONAL:
return prot | IOMMU_READ | IOMMU_WRITE;
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 32c5890..a203181 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -34,7 +34,8 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
dma_addr_t base,
u64 size, struct device *dev);
 
 /* General helpers for DMA-API <-> IOMMU-API interaction */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent);
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+unsigned long attrs);
 
 /*
  * These implement the bulk of the relevant DMA mapping callbacks, but require
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V7 4/8] common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute

[Sricharan R]

From: Mitchel Humpherys 

This patch adds the DMA_ATTR_PRIVILEGED attribute to the DMA-mapping
subsystem.

Some advanced peripherals such as remote processors and GPUs perform
accesses to DMA buffers in both privileged "supervisor" and unprivileged
"user" modes.  This attribute is used to indicate to the DMA-mapping
subsystem that the buffer is fully accessible at the elevated privilege
level (and ideally inaccessible or at least read-only at the
lesser-privileged levels).

Cc: linux-...@vger.kernel.org
Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---
 Documentation/DMA-attributes.txt | 10 ++
 include/linux/dma-mapping.h  |  7 +++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/DMA-attributes.txt b/Documentation/DMA-attributes.txt
index 98bf7ac..44c6bc4 100644
--- a/Documentation/DMA-attributes.txt
+++ b/Documentation/DMA-attributes.txt
@@ -143,3 +143,13 @@ So, this provides a way for drivers to avoid those error 
messages on calls
 where allocation failures are not a problem, and shouldn't bother the logs.
 
 NOTE: At the moment DMA_ATTR_NO_WARN is only implemented on PowerPC.
+
+DMA_ATTR_PRIVILEGED
+--
+
+Some advanced peripherals such as remote processors and GPUs perform
+accesses to DMA buffers in both privileged "supervisor" and unprivileged
+"user" modes.  This attribute is used to indicate to the DMA-mapping
+subsystem that the buffer is fully accessible at the elevated privilege
+level (and ideally inaccessible or at least read-only at the
+lesser-privileged levels).
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 6f3e6ca..ee31ea1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -63,6 +63,13 @@
 #define DMA_ATTR_NO_WARN   (1UL << 8)
 
 /*
+ * DMA_ATTR_PRIVILEGED: used to indicate that the buffer is fully
+ * accessible at an elevated privilege level (and ideally inaccessible or
+ * at least read-only at lesser-privileged levels).
+ */
+#define DMA_ATTR_PRIVILEGED(1UL << 8)
+
+/*
  * A dma_addr_t can hold any valid DMA or bus address for the platform.
  * It can be given to a device to use as a DMA source or target.  A CPU cannot
  * reference a dma_addr_t directly because there may be translation between
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V7 2/8] iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

[Sricharan R]

From: Jeremy Gebben 

Allow the creation of privileged mode mappings, for stage 1 only.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Jeremy Gebben 
---
 drivers/iommu/io-pgtable-arm.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index f5c90e1..69ba83a 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -350,11 +350,14 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct 
arm_lpae_io_pgtable *data,
 
if (data->iop.fmt == ARM_64_LPAE_S1 ||
data->iop.fmt == ARM_32_LPAE_S1) {
-   pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
+   pte = ARM_LPAE_PTE_nG;
 
if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
pte |= ARM_LPAE_PTE_AP_RDONLY;
 
+   if (!(prot & IOMMU_PRIV))
+   pte |= ARM_LPAE_PTE_AP_UNPRIV;
+
if (prot & IOMMU_MMIO)
pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V7 0/8] Add support for privileged mappings

[Sricharan R]

This series is a resend of the V5 that Mitch sent sometime back [2]
All the patches are the same and i have just rebased. Redid patch [3],
as it does not apply in this code base. Added a couple of more patches
[4], [5] from Robin for adding the privileged attributes to armv7s format
and arm-smmuv3 revert.

The following patch to the ARM SMMU driver:

commit d346180e70b91b3d5a1ae7e5603e65593d4622bc
Author: Robin Murphy 
Date:   Tue Jan 26 18:06:34 2016 +

iommu/arm-smmu: Treat all device transactions as unprivileged

started forcing all SMMU transactions to come through as "unprivileged".
The rationale given was that:

  (1) There is no way in the IOMMU API to even request privileged
  mappings.

  (2) It's difficult to implement a DMA mapper that correctly models the
  ARM VMSAv8 behavior of unprivileged-writeable =>
  privileged-execute-never.

This series rectifies (1) by introducing an IOMMU API for privileged
mappings and implements it in io-pgtable-arm.

This series rectifies (2) by introducing a new dma attribute
(DMA_ATTR_PRIVILEGED) for users of the DMA API that need privileged
mappings which are inaccessible to lesser-privileged execution levels, and
implements it in the arm64 IOMMU DMA mapper.  The one known user (pl330.c)
is converted over to the new attribute.

Jordan and Jeremy can provide more info on the use case if needed, but the
high level is that it's a security feature to prevent attacks such as [1].

Note that, i tested this on arm64 with arm-smmuv2, short descriptor changes,
and do not have an platform to test this with arm-smmuv3.

[1] https://github.com/robclark/kilroy
[2] https://lkml.org/lkml/2016/7/27/590
[3] https://patchwork.kernel.org/patch/9250493/
[4] 
http://www.linux-arm.org/git?p=linux-rm.git;a=commit;h=1291bd74f05d31da1dab3df02987cba5bd25849b
[5] 
http://www.linux-arm.org/git?p=linux-rm.git;a=commit;h=a79c1c6333f26849dba418cd92de26b60f5954f3

Changelog:
 v6..v7
- Added couple of more patches, picked up acks, updated commit log

 v5..v6
- Rebased all the patches and redid 6/6 as it does not apply in
  this code base. 

 v4..v5

- Simplified patch 4/6 (suggested by Robin Murphy).

  v3..v4

- Rebased and reworked on linux next due to the dma attrs rework going
  on over there.  Patches changed: 3/6, 4/6, and 5/6.

  v2..v3

- Incorporated feedback from Robin:
  * Various comments and re-wordings.
  * Use existing bit definitions for IOMMU_PRIV implementation
in io-pgtable-arm.
  * Renamed and redocumented dma_direction_to_prot.
  * Don't worry about executability in new DMA attr.

  v1..v2

- Added a new DMA attribute to make executable privileged mappings
  work, and use that in the pl330 driver (suggested by Will).

Jeremy Gebben (1):
  iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

Mitchel Humpherys (4):
  iommu: add IOMMU_PRIV attribute
  common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute
  arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
  dmaengine: pl330: Make sure microcode is privileged

Robin Murphy (2):
  iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag
  iommu/arm-smmu: Revert "iommu/arm-smmu: Set PRIVCFG in stage 1 STEs"

Sricharan R (1):
  iommu/arm-smmu: Set privileged attribute to 'default' instead of
'unprivileged'

 Documentation/DMA-attributes.txt   | 10 ++
 arch/arm64/mm/dma-mapping.c|  6 +++---
 drivers/dma/pl330.c|  5 +++--
 drivers/iommu/arm-smmu-v3.c|  7 +--
 drivers/iommu/arm-smmu.c   |  2 +-
 drivers/iommu/dma-iommu.c  | 10 --
 drivers/iommu/io-pgtable-arm-v7s.c |  6 +-
 drivers/iommu/io-pgtable-arm.c |  5 -
 include/linux/dma-iommu.h  |  3 ++-
 include/linux/dma-mapping.h|  7 +++
 include/linux/iommu.h  |  1 +
 11 files changed, 45 insertions(+), 17 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V7 1/8] iommu: add IOMMU_PRIV attribute

[Sricharan R]

From: Mitchel Humpherys 

Add the IOMMU_PRIV attribute, which is used to indicate privileged
mappings.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Signed-off-by: Mitchel Humpherys 
Acked-by: Will Deacon 
---
 include/linux/iommu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f2960e4..bf22131 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -31,6 +31,7 @@
 #define IOMMU_CACHE(1 << 2) /* DMA cache coherency */
 #define IOMMU_NOEXEC   (1 << 3)
 #define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
+#define IOMMU_PRIV (1 << 5) /* privileged */
 
 struct iommu_ops;
 struct iommu_group;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

RE: [RESEND PATCH V6 0/6] Add support for privileged mappings

[Sricharan]

Hi Robin,

>>> Hi Sricharan,
>>>
>>> On 02/12/16 14:55, Sricharan R wrote:
>>>> This series is a resend of the V5 that Mitch sent sometime back [2]
>>>> All the patches are the same and i have just rebased. Not sure why this
>>>> finally did not make it last time. The last patch in the previous
>>>> series does not apply now [3], so just redid that. Also Copied the tags
>>>> that he had from last time as well.
>>>
>>> Heh, I was assuming this would be down to me to pick up. Vinod did have
>>> some complaints last time about the commit message on the PL330 patch -
>>> I did get as far as rewriting that and reworking onto my SMMU
>>> changes[1], I just hadn't got round to sending it, so it fell onto the
>>> "after the next merge window" pile.
>>>
>>> I'd give some review comments, but they'd essentially be a diff against
>>> that branch :)
>>>
>>
>> Sure, i did not knew that you were on this already. I can repost with the 
>> diff
>> from your branch taken in or wait for you as well. I am fine with either ways
>> that you suggest.
>>
>> I checked the patches against your branch, i see that the changes are,
>>
>> 1) one patch for implementing it for armv7s descriptor
>> 2) Changes on pl330 patch commit logs and
>> 3) One patch for doing the revert on arm-smmuv3 as well.
>
>If you want to pick up my short-descriptor and SMMUv3 patches and run
>with them you're more than welcome - the rest is just cosmetic stuff
>which doesn't really matter, especially as it's picking up acks as-is.
>

Sure, i will repost with additional stuff picked up from your branch and
the acks as well.

Regards,
 Sricharan

RE: [PATCH v9 07/16] drivers: acpi: implement acpi_dma_configure

[Sricharan]

igure()
>> interface on ACPI for obvious reasons (on ARM systems), I stopped
>> short of adding ACPI code to mirror of_dma_get_range() equivalent
>> (through the _DMA object) but I am really really nervous about changing
>> the code path on x86 because in theory all is fine, in practice even
>> just setting the masks to sane values can have unexpected consequences,
>> I just can't know (that's why I wasn't doing it in the first iterations
>> of this series).
>>
>> Side note: DT with of_dma_configure() and ACPI with
>> acpi_create_platform_device() set the default dma mask for all
>> platform devices already _regardless_ of what they really are, though
>> arguably acpi_bind_one() touches ways more devices.
>>
>> I really think that removing the default dma masks settings from
>> acpi_dma_configure() is the safer thing to do for the time being (or
>> moving acpi_dma_configure() to acpi_create_platform_device(), where the
>> DMA masks are set-up by default by core ACPI. Mark, Suravee, what was
>> the rationale behind calling arch_setup_dma_ops() in acpi_bind_one() ?)
>
>Alternatively, you can add one more arch wrapper that will be a no-op
>on x86 and that will set up the default masks and call
>arch_setup_dma_ops() on ARM.  Then, you can invoke that from
>acpi_dma_configure().
>
>Or make the definition of acpi_dma_configure() itself depend on the
>architecture.
>

So is it better that either removing the masks from acpi_dma_configure (or)
creating the wrapper as Rafael mentioned, than moving
acpi_dma_configure itself , because with something like iommu probe
deferral that is tried, acpi_dma_configure is getting invoked from a device's 
really_probe, a different path again ?

Regards,
 Sricharan

RE: [RESEND PATCH V6 0/6] Add support for privileged mappings

[Sricharan]

Hi Robin,

>Hi Sricharan,
>
>On 02/12/16 14:55, Sricharan R wrote:
>> This series is a resend of the V5 that Mitch sent sometime back [2]
>> All the patches are the same and i have just rebased. Not sure why this
>> finally did not make it last time. The last patch in the previous
>> series does not apply now [3], so just redid that. Also Copied the tags
>> that he had from last time as well.
>
>Heh, I was assuming this would be down to me to pick up. Vinod did have
>some complaints last time about the commit message on the PL330 patch -
>I did get as far as rewriting that and reworking onto my SMMU
>changes[1], I just hadn't got round to sending it, so it fell onto the
>"after the next merge window" pile.
>
>I'd give some review comments, but they'd essentially be a diff against
>that branch :)
>

Sure, i did not knew that you were on this already. I can repost with the diff
from your branch taken in or wait for you as well. I am fine with either ways
that you suggest.

I checked the patches against your branch, i see that the changes are,

1) one patch for implementing it for armv7s descriptor
2) Changes on pl330 patch commit logs and
3) One patch for doing the revert on arm-smmuv3 as well.

Regards,
 Sricharan

[PATCH V6 6/6] iommu/arm-smmu: Set privileged attribute to 'default' instead of 'unprivileged'

[Sricharan R]

Currently the driver sets all the device transactions privileges
to UNPRIVILEGED, but there are cases where the iommu masters wants
to isolate privileged supervisor and unprivileged user.
So don't override the privileged setting to unprivileged, instead
set it to default as incoming and let it be controlled by the pagetable
settings.

Signed-off-by: Sricharan R 
---

[V6] V5 was doing this with a 'revert'[1] patch, which no more
 applies on this code base. So changed the same like this.
 [1] https://patchwork.kernel.org/patch/9250493/

 drivers/iommu/arm-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index eaa8f44..8bb0eea 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1213,7 +1213,7 @@ static int arm_smmu_domain_add_master(struct 
arm_smmu_domain *smmu_domain,
continue;
 
s2cr[idx].type = type;
-   s2cr[idx].privcfg = S2CR_PRIVCFG_UNPRIV;
+   s2cr[idx].privcfg = S2CR_PRIVCFG_DEFAULT;
s2cr[idx].cbndx = cbndx;
arm_smmu_write_s2cr(smmu, idx);
}
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[RESEND PATCH V6 5/6] dmaengine: pl330: Make sure microcode is privileged

[Sricharan R]

From: Mitchel Humpherys 

The PL330 performs privileged instruction fetches.  This can result in
SMMU permission faults on SMMUs that implement the ARMv8 VMSA, which
specifies that mappings that are writeable at one execution level shall
not be executable at any higher-privileged level.  Fix this by using the
DMA_ATTR_PRIVILEGED attribute, which will ensure that the microcode
IOMMU mapping is only accessible to the privileged level.

Cc: Dan Williams 
Cc: Vinod Koul 
Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---

[V6] No change

 drivers/dma/pl330.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 030fe05..1a8bac2 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1854,14 +1854,16 @@ static int dmac_alloc_resources(struct pl330_dmac 
*pl330)
 {
int chans = pl330->pcfg.num_chan;
int ret;
+   unsigned long dma_attrs = DMA_ATTR_PRIVILEGED;
 
/*
 * Alloc MicroCode buffer for 'chans' Channel threads.
 * A channel's buffer offset is (Channel_Id * MCODE_BUFF_PERCHAN)
 */
-   pl330->mcode_cpu = dma_alloc_coherent(pl330->ddma.dev,
+   pl330->mcode_cpu = dma_alloc_attrs(pl330->ddma.dev,
chans * pl330->mcbufsz,
-   &pl330->mcode_bus, GFP_KERNEL);
+   &pl330->mcode_bus, GFP_KERNEL,
+   dma_attrs);
if (!pl330->mcode_cpu) {
dev_err(pl330->ddma.dev, "%s:%d Can't allocate memory!\n",
__func__, __LINE__);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[RESEND PATCH V6 3/6] common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute

[Sricharan R]

From: Mitchel Humpherys 

This patch adds the DMA_ATTR_PRIVILEGED attribute to the DMA-mapping
subsystem.

Some advanced peripherals such as remote processors and GPUs perform
accesses to DMA buffers in both privileged "supervisor" and unprivileged
"user" modes.  This attribute is used to indicate to the DMA-mapping
subsystem that the buffer is fully accessible at the elevated privilege
level (and ideally inaccessible or at least read-only at the
lesser-privileged levels).

Cc: linux-...@vger.kernel.org
Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---

[V6] No change

 Documentation/DMA-attributes.txt | 10 ++
 include/linux/dma-mapping.h  |  7 +++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/DMA-attributes.txt b/Documentation/DMA-attributes.txt
index 98bf7ac..44c6bc4 100644
--- a/Documentation/DMA-attributes.txt
+++ b/Documentation/DMA-attributes.txt
@@ -143,3 +143,13 @@ So, this provides a way for drivers to avoid those error 
messages on calls
 where allocation failures are not a problem, and shouldn't bother the logs.
 
 NOTE: At the moment DMA_ATTR_NO_WARN is only implemented on PowerPC.
+
+DMA_ATTR_PRIVILEGED
+--
+
+Some advanced peripherals such as remote processors and GPUs perform
+accesses to DMA buffers in both privileged "supervisor" and unprivileged
+"user" modes.  This attribute is used to indicate to the DMA-mapping
+subsystem that the buffer is fully accessible at the elevated privilege
+level (and ideally inaccessible or at least read-only at the
+lesser-privileged levels).
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 6f3e6ca..ee31ea1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -63,6 +63,13 @@
 #define DMA_ATTR_NO_WARN   (1UL << 8)
 
 /*
+ * DMA_ATTR_PRIVILEGED: used to indicate that the buffer is fully
+ * accessible at an elevated privilege level (and ideally inaccessible or
+ * at least read-only at lesser-privileged levels).
+ */
+#define DMA_ATTR_PRIVILEGED(1UL << 8)
+
+/*
  * A dma_addr_t can hold any valid DMA or bus address for the platform.
  * It can be given to a device to use as a DMA source or target.  A CPU cannot
  * reference a dma_addr_t directly because there may be translation between
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[RESEND PATCH V6 1/6] iommu: add IOMMU_PRIV attribute

[Sricharan R]

From: Mitchel Humpherys 

Add the IOMMU_PRIV attribute, which is used to indicate privileged
mappings.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---

[V6] No change

 include/linux/iommu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f2960e4..bf22131 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -31,6 +31,7 @@
 #define IOMMU_CACHE(1 << 2) /* DMA cache coherency */
 #define IOMMU_NOEXEC   (1 << 3)
 #define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
+#define IOMMU_PRIV (1 << 5) /* privileged */
 
 struct iommu_ops;
 struct iommu_group;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[RESEND PATCH V6 4/6] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED

[Sricharan R]

From: Mitchel Humpherys 

The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines.  Implement it in
dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Mitchel Humpherys 
---

[V6] No change

 arch/arm64/mm/dma-mapping.c |  6 +++---
 drivers/iommu/dma-iommu.c   | 10 --
 include/linux/dma-iommu.h   |  3 ++-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 401f79a..ae76ead 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -557,7 +557,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t 
size,
 unsigned long attrs)
 {
bool coherent = is_device_dma_coherent(dev);
-   int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+   int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
size_t iosize = size;
void *addr;
 
@@ -711,7 +711,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, 
struct page *page,
   unsigned long attrs)
 {
bool coherent = is_device_dma_coherent(dev);
-   int prot = dma_direction_to_prot(dir, coherent);
+   int prot = dma_info_to_prot(dir, coherent, attrs);
dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
 
if (!iommu_dma_mapping_error(dev, dev_addr) &&
@@ -769,7 +769,7 @@ static int __iommu_map_sg_attrs(struct device *dev, struct 
scatterlist *sgl,
__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
 
return iommu_dma_map_sg(dev, sgl, nelems,
-   dma_direction_to_prot(dir, coherent));
+   dma_info_to_prot(dir, coherent, attrs));
 }
 
 static void __iommu_unmap_sg_attrs(struct device *dev,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index d2a7a46..756d5e0 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -182,16 +182,22 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
dma_addr_t base,
 EXPORT_SYMBOL(iommu_dma_init_domain);
 
 /**
- * dma_direction_to_prot - Translate DMA API directions to IOMMU API page flags
+ * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU API
+ *page flags.
  * @dir: Direction of DMA transfer
  * @coherent: Is the DMA master cache-coherent?
+ * @attrs: DMA attributes for the mapping
  *
  * Return: corresponding IOMMU API page protection flags
  */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+unsigned long attrs)
 {
int prot = coherent ? IOMMU_CACHE : 0;
 
+   if (attrs & DMA_ATTR_PRIVILEGED)
+   prot |= IOMMU_PRIV;
+
switch (dir) {
case DMA_BIDIRECTIONAL:
return prot | IOMMU_READ | IOMMU_WRITE;
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 32c5890..a203181 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -34,7 +34,8 @@ int iommu_dma_init_domain(struct iommu_domain *domain, 
dma_addr_t base,
u64 size, struct device *dev);
 
 /* General helpers for DMA-API <-> IOMMU-API interaction */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent);
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+unsigned long attrs);
 
 /*
  * These implement the bulk of the relevant DMA mapping callbacks, but require
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[RESEND PATCH V6 0/6] Add support for privileged mappings

[Sricharan R]

This series is a resend of the V5 that Mitch sent sometime back [2]
All the patches are the same and i have just rebased. Not sure why this
finally did not make it last time. The last patch in the previous
series does not apply now [3], so just redid that. Also Copied the tags
that he had from last time as well.

The following patch to the ARM SMMU driver:

commit d346180e70b91b3d5a1ae7e5603e65593d4622bc
Author: Robin Murphy 
Date:   Tue Jan 26 18:06:34 2016 +

iommu/arm-smmu: Treat all device transactions as unprivileged

started forcing all SMMU transactions to come through as "unprivileged".
The rationale given was that:

  (1) There is no way in the IOMMU API to even request privileged
  mappings.

  (2) It's difficult to implement a DMA mapper that correctly models the
  ARM VMSAv8 behavior of unprivileged-writeable =>
  privileged-execute-never.

This series rectifies (1) by introducing an IOMMU API for privileged
mappings and implements it in io-pgtable-arm.

This series rectifies (2) by introducing a new dma attribute
(DMA_ATTR_PRIVILEGED) for users of the DMA API that need privileged
mappings which are inaccessible to lesser-privileged execution levels, and
implements it in the arm64 IOMMU DMA mapper.  The one known user (pl330.c)
is converted over to the new attribute.

Jordan and Jeremy can provide more info on the use case if needed, but the
high level is that it's a security feature to prevent attacks such as [1].

[1] https://github.com/robclark/kilroy
[2] https://lkml.org/lkml/2016/7/27/590
[3] https://patchwork.kernel.org/patch/9250493/

Changelog:

 v5..v6
- Rebased all the patches and redid 6/6 as it does not apply in
  this code base. 

 v4..v5

- Simplified patch 4/6 (suggested by Robin Murphy).

  v3..v4

- Rebased and reworked on linux next due to the dma attrs rework going
  on over there.  Patches changed: 3/6, 4/6, and 5/6.

  v2..v3

- Incorporated feedback from Robin:
  * Various comments and re-wordings.
  * Use existing bit definitions for IOMMU_PRIV implementation
in io-pgtable-arm.
  * Renamed and redocumented dma_direction_to_prot.
  * Don't worry about executability in new DMA attr.

  v1..v2

- Added a new DMA attribute to make executable privileged mappings
  work, and use that in the pl330 driver (suggested by Will).

Jeremy Gebben (1):
  iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

Mitchel Humpherys (4):
  iommu: add IOMMU_PRIV attribute
  common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute
  arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
  dmaengine: pl330: Make sure microcode is privileged

Sricharan R (1):
  iommu/arm-smmu: Set privileged attribute to 'default' instead of
'unprivileged'

 Documentation/DMA-attributes.txt | 10 ++
 arch/arm64/mm/dma-mapping.c  |  6 +++---
 drivers/dma/pl330.c  |  6 --
 drivers/iommu/arm-smmu.c |  2 +-
 drivers/iommu/dma-iommu.c| 10 --
 drivers/iommu/io-pgtable-arm.c   |  5 -
 include/linux/dma-iommu.h|  3 ++-
 include/linux/dma-mapping.h  |  7 +++
 include/linux/iommu.h|  1 +
 9 files changed, 40 insertions(+), 10 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[RESEND PATCH V6 2/6] iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

[Sricharan R]

From: Jeremy Gebben 

Allow the creation of privileged mode mappings, for stage 1 only.

Reviewed-by: Robin Murphy 
Tested-by: Robin Murphy 
Acked-by: Will Deacon 
Signed-off-by: Jeremy Gebben 
---

[V6] No change

 drivers/iommu/io-pgtable-arm.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index f5c90e1..69ba83a 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -350,11 +350,14 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct 
arm_lpae_io_pgtable *data,
 
if (data->iop.fmt == ARM_64_LPAE_S1 ||
data->iop.fmt == ARM_32_LPAE_S1) {
-   pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
+   pte = ARM_LPAE_PTE_nG;
 
if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
pte |= ARM_LPAE_PTE_AP_RDONLY;
 
+   if (!(prot & IOMMU_PRIV))
+   pte |= ARM_LPAE_PTE_AP_UNPRIV;
+
if (prot & IOMMU_MMIO)
pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH V2 0/2] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi Stan,

>Hi,
>
>On 11/18/2016 02:28 PM, Sricharan R wrote:
>> This series adds support for gdscs(powerdomains) that can be configured
>> in hw controlled mode. So they are turned 'ON' based on needs dynamically,
>> helping to save power. Also updated the venus video ip's gdsc/clock
>> data to put them in hw control.
>>
>> V2:
>>  Dropped patch#3 [1] as it was concluded that the patch was effectively
>>  masking the fact the clocks were not getting turned on when the gdsc
>>  is put in hwctrl. With some change in sequence from venus core, masking
>>  is not needed and so patch needs to handled in venus driver.
>
>Which sequence should be changed in venus driver?
>
Ya wanted to discuss this with you on the venus thread, but let me put it here.

So while enabling the hw control bit for the venus subcores 0/1 gdscs and 
turning
on the subcore 0/1 clks, we saw that unless the
VENUS_WRAPPER_VENUS0_MMCC_VDEC_VCODEC_POWER_CONTROL
register is programmed to '0'(reset value is 1), the subcores domain/
clocks do not turn on. So this means that the,

1) venus driver should turn on all clocks except the subcore clocks.
2) Program VENUS_WRAPPER_VENUS0_MMCC_VDEC_VCODEC_POWER_CONTROL to
'0' to turn on sub domains.
3) Turn on subcore clocks (cbc) and verify their running status using clk_enable
4) Program VENUS_WRAPPER_VENUS0_MMCC_VDEC_VCODEC_POWER_CONTROL to
 '1' again to turn off subdomain/clocks and let the firmware turn it on 
when required.

Note that in my previous patch set, i was skipping the check to verify the 
subcore clocks
'running status' previously, assuming that it can't be done while the gdsc is 
in hwctrl, but
that was not right.

Regards,
 Sricharan

[PATCH V2 2/2] clk: qcom: Put venus core0/1 gdscs to hw control mode

[Sricharan R]

The venus video ip's internal core blocks are under the
control of the firmware and their powerdomains needs to be
'ON' only when used by the firmware. So putting it into
hw controlled mode lets this to happen, otherwise the firmware
hangs checking for this.

Signed-off-by: Sricharan R 
---
 drivers/clk/qcom/mmcc-msm8996.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index ca97e11..41aabe3 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -2945,6 +2945,7 @@ enum {
.name = "venus_core0",
},
.pwrsts = PWRSTS_OFF_ON,
+   .flags = HW_CTRL,
 };
 
 static struct gdsc venus_core1_gdsc = {
@@ -2955,6 +2956,7 @@ enum {
.name = "venus_core1",
},
.pwrsts = PWRSTS_OFF_ON,
+   .flags = HW_CTRL,
 };
 
 static struct gdsc camss_gdsc = {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V2 0/2] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan R]

This series adds support for gdscs(powerdomains) that can be configured
in hw controlled mode. So they are turned 'ON' based on needs dynamically,
helping to save power. Also updated the venus video ip's gdsc/clock
data to put them in hw control.

V2:
 Dropped patch#3 [1] as it was concluded that the patch was effectively
 masking the fact the clocks were not getting turned on when the gdsc
 is put in hwctrl. With some change in sequence from venus core, masking
 is not needed and so patch needs to handled in venus driver.

 Fixed a comment in patch#1 to check the return value for gdsc_hwctrl

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1270922.html

Rajendra Nayak (1):
  clk: qcom: gdsc: Add support for gdscs with HW control

Sricharan R (1):
  clk: qcom: Put venus core0/1 gdscs to hw control mode

 drivers/clk/qcom/gdsc.c | 19 +++
 drivers/clk/qcom/gdsc.h |  1 +
 drivers/clk/qcom/mmcc-msm8996.c |  2 ++
 3 files changed, 22 insertions(+)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V2 1/2] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan R]

From: Rajendra Nayak 

Some GDSCs might support a HW control mode, where in the power
domain (gdsc) is brought in and out of low power state (while
unsued) without any SW assistance, saving power.
Such GDSCs can be configured in a HW control mode when powered on
until they are explicitly requested to be powered off by software.

Signed-off-by: Rajendra Nayak 
Signed-off-by: Sricharan R 
---
[V2] Fixed to take care of the return value of gdsc_hwctrl

 drivers/clk/qcom/gdsc.c | 19 +++
 drivers/clk/qcom/gdsc.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index f12d7b2..57c7c1b 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg)
return !!(val & PWR_ON_MASK);
 }
 
+static int gdsc_hwctrl(struct gdsc *sc, bool en)
+{
+   u32 val = en ? HW_CONTROL_MASK : 0;
+
+   return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 {
int ret;
@@ -164,16 +171,28 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 */
udelay(1);
 
+   /* Turn on HW trigger mode if supported */
+   if (sc->flags & HW_CTRL)
+   return gdsc_hwctrl(sc, true);
+
return 0;
 }
 
 static int gdsc_disable(struct generic_pm_domain *domain)
 {
struct gdsc *sc = domain_to_gdsc(domain);
+   int ret;
 
if (sc->pwrsts == PWRSTS_ON)
return gdsc_assert_reset(sc);
 
+   /* Turn off HW trigger mode if supported */
+   if (sc->flags & HW_CTRL) {
+   ret = gdsc_hwctrl(sc, false);
+   if (ret < 0)
+   return ret;
+   }
+
if (sc->pwrsts & PWRSTS_OFF)
gdsc_clear_mem_on(sc);
 
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 3bf497c..b1f30f8 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -50,6 +50,7 @@ struct gdsc {
 #define PWRSTS_RET_ON  (PWRSTS_RET | PWRSTS_ON)
const u8flags;
 #define VOTABLEBIT(0)
+#define HW_CTRLBIT(1)
struct reset_controller_dev *rcdev;
unsigned int*resets;
unsigned intreset_count;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH 2/2] clk: qcom: Put venus core0/1 gdscs to hw control mode

[Sricharan R]

The venus video ip's internal core blocks are under the
control of the firmware and their powerdomains needs to be
'ON' only when used by the firmware. So putting it into
hw controlled mode lets this to happen, otherwise the firmware
hangs checking for this.

Signed-off-by: Sricharan R 
---
 drivers/clk/qcom/mmcc-msm8996.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index ca97e11..41aabe3 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -2945,6 +2945,7 @@ enum {
.name = "venus_core0",
},
.pwrsts = PWRSTS_OFF_ON,
+   .flags = HW_CTRL,
 };
 
 static struct gdsc venus_core1_gdsc = {
@@ -2955,6 +2956,7 @@ enum {
.name = "venus_core1",
},
.pwrsts = PWRSTS_OFF_ON,
+   .flags = HW_CTRL,
 };
 
 static struct gdsc camss_gdsc = {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH 1/2] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan R]

From: Rajendra Nayak 

Some GDSCs might support a HW control mode, where in the power
domain (gdsc) is brought in and out of low power state (while
unsued) without any SW assistance, saving power.
Such GDSCs can be configured in a HW control mode when powered on
until they are explicitly requested to be powered off by software.

Signed-off-by: Rajendra Nayak 
Signed-off-by: Sricharan R 
---
[V2] Fixed to take care of the return value of gdsc_hwctrl

 drivers/clk/qcom/gdsc.c | 19 +++
 drivers/clk/qcom/gdsc.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index f12d7b2..57c7c1b 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg)
return !!(val & PWR_ON_MASK);
 }
 
+static int gdsc_hwctrl(struct gdsc *sc, bool en)
+{
+   u32 val = en ? HW_CONTROL_MASK : 0;
+
+   return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 {
int ret;
@@ -164,16 +171,28 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 */
udelay(1);
 
+   /* Turn on HW trigger mode if supported */
+   if (sc->flags & HW_CTRL)
+   return gdsc_hwctrl(sc, true);
+
return 0;
 }
 
 static int gdsc_disable(struct generic_pm_domain *domain)
 {
struct gdsc *sc = domain_to_gdsc(domain);
+   int ret;
 
if (sc->pwrsts == PWRSTS_ON)
return gdsc_assert_reset(sc);
 
+   /* Turn off HW trigger mode if supported */
+   if (sc->flags & HW_CTRL) {
+   ret = gdsc_hwctrl(sc, false);
+   if (ret < 0)
+   return ret;
+   }
+
if (sc->pwrsts & PWRSTS_OFF)
gdsc_clear_mem_on(sc);
 
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 3bf497c..b1f30f8 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -50,6 +50,7 @@ struct gdsc {
 #define PWRSTS_RET_ON  (PWRSTS_RET | PWRSTS_ON)
const u8flags;
 #define VOTABLEBIT(0)
+#define HW_CTRLBIT(1)
struct reset_controller_dev *rcdev;
unsigned int*resets;
unsigned intreset_count;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH 0/2] clk: qcom: Add support for hw controlled gdscs/clocks

[Sricharan R]

This series adds support for gdscs(powerdomains) that can be configured
in hw controlled mode. So they are turned 'ON' based on needs dynamically,
helping to save power. Also updated the venus video ip's gdsc/clock
data to put them in hw control.

V2:
 Dropped patch#3 [1] as it was concluded that the patch was effectively
 masking the fact the clocks were not getting turned on when the gdsc
 is put in hwctrl. With some change in sequence from venus core, masking
 is not needed and so patch needs to handled in venus driver.

 Fixed a comment in patch#1 to check the return value for gdsc_hwctrl

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1270922.html

Rajendra Nayak (1):
  clk: qcom: gdsc: Add support for gdscs with HW control

Sricharan R (1):
  clk: qcom: Put venus core0/1 gdscs to hw control mode

 drivers/clk/qcom/gdsc.c | 19 +++
 drivers/clk/qcom/gdsc.h |  1 +
 drivers/clk/qcom/mmcc-msm8996.c |  2 ++
 3 files changed, 22 insertions(+)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi Stephen,

>>
>> So the above is the sequence which is actually carried out on the
>> firmware side. The same can be done in host as well.
>> The clocks stuck issue indeed is not there with this.
>
>Great! We've finally connected on what the actual problem is.
>
>> But with the above sequence we need to add a step to do inverse
>> of STEP3 above (ie write the registers to de-assert hw_signal),
>> to keep the subdomains in off, till firmware uses it. So the
>> above sequence helps to avoid masking the halt check, although
>> the host really does not wants to use these clocks, except
>> setting it up for the firmware.
>>
>
>Right, but knowing that the clocks failed to turn on in the first
>place is much safer than silently ignoring the failure.
>Otherwise, we could hand over control to the firmware, and the
>firmware would fail to operate the hardware, and we're stuck with
>debugging the firmware now. That sounds quite painful to figure
>out.
>

Right, i already debugged this sort of a scenario which was quite
paintful sometime back :)

>If we properly toggle the video hw bits in coordination with
>firmware and turn on/off the clocks with the GDSC ON, then
>debugging is made simpler. The point is, we don't want to lose
>robustness by silencing halt checks. The semantics of
>clk_enable() means the clock is running, and that won't be true
>here unless we ensure the GDSC is enabled.
>

ok, which means with this approach, this patch can be dropped and
the other bits needs to be added to the video driver. I will follow that
up with Stanimir in his video driver patches.

Regards,
 Sricharan

RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi Rajendra,
>
>>>
>>> The proper sequence sounds like it should be:
>>>
>>> 1. Enable GDSC for main domain
>>> 2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>>> 3. Write the two registers to assert hw signal for subdomains
>>> 4. Enable GDSCs for two subdomains
>>> 5. Enable clocks for subdomains (video_subcore{0,1}_clk)
>>>
>[]..
>
>>
>> So the above is the sequence which is actually carried out on the
>> firmware side. The same can be done in host as well.
>
>By the 'above sequence is done on firmware side', I hope you don;t mean *all* 
>5 steps.
>I guess you mean only step 3 is done by firmware?
>

Yes, only step 3.

Regards,
 Sricharan

RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi Stephen,

>>
>> On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote:
>> > Well I'm also curious which case is failing. Does turning on the
>> > clocks work after the gdsc is enabled? Does turning off the
>> > clocks fail because we don't know when the gdsc has turned off? I
>> > would hope that the firmware keeps the gdsc on when it's done
>> > processing things, goes idle, and hands back control to software.
>> > Right now I'm failing to see how the halt bits fail to toggle
>> > assuming that firmware isn't misbehaving and the kernel driver is
>> > power controlling in a coordinated manner with the firmware.
>>
>> What fails is turning ON the clocks after the gdsc is put under
>> hardware control (by fails I mean the halt checks fail to indicate
>> the clock is running, but register accesses etc thereafter suggest
>> the clocks are actually running)
>> The halt checks seem to work only while the gdsc is put in SW enabled
>> state.
>>
>
>Um... that is bad. I don't see how that is possible. It sounds
>like the clocks are not turning on when we're asking for them to
>turn on. The register accesses are always working because these
>subcore clks aren't required for register accesses. Most likely
>the GDSC for the subdomains is off (even after we thought we
>enabled it).
>
>Let's take a step back. The video hardware has three GDSCs. One
>for the main logic, and two for each subdomain. We're adding hw
>control for the two subdomains here. From what I can tell there
>isn't any hw control for the main domain. I see that there are
>two registers in the video hardware to control those subdomain hw
>enable signals that go to the GDSC. The reset value is OFF (not
>ON like was stated earlier), so it seems that if we switch the
>subdomain GDSCs on in these patches it will turn on for a short
>time, and then turn off when we switch into hw mode (by virtue of
>the way we enable the GDSC). Presumably we can assert these hw
>signal bits regardless of the subdomain power states, because
>otherwise we're in a chicken-egg situation for the firmware
>controlling this stuff.
>
>The proper sequence sounds like it should be:
>
>   1. Enable GDSC for main domain
>   2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>   3. Write the two registers to assert hw signal for subdomains
>   4. Enable GDSCs for two subdomains
>   5. Enable clocks for subdomains (video_subcore{0,1}_clk)
>
>I can only guess that we're not doing this. Probably the sequence
>right now is:
>
>   1. Enable GDSC for main domain and both sub-domains
>   2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>   3. Enable clocks for subdomains (video_subcore{0,1}_clk)
>   
>
>Sound right? If so, please fix the sequence in the video driver.
>

So the above is the sequence which is actually carried out on the
firmware side. The same can be done in host as well.
The clocks stuck issue indeed is not there with this. But with the
above sequence we need to add a step to do inverse of STEP3
above (ie write the registers to de-assert hw_signal), to keep
the subdomains in off, till firmware uses it. So the above sequence
helps to avoid masking the halt check, although the host really
does not wants to use these clocks, except setting it up for the
firmware.

Regards,
 Sricharan

RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi,
>
>A better design would be to check if the associated GDSC is in hw
>control mode and then skip the checks because the clocks are no
>longer under the control of the registers. I presume we only
>enable the clocks here to turn on parent clocks which don't turn
>on/off automatically, i.e. the PLL.
>
I was thinking clocks in the powerdomain still needs to be turned
on explicitly, these are branch clocks, irrespective of the PLLs.
Putting the gdsc in hw_ctrl, only makes the polling on their status
invalid. Anyways would be good to be aligned on this.

>Given that hw control is a static decision I guess that is an
>over-engineered solution though? The problem is that this seems
>brittle because we have to keep two things in sync, the branches
>and the gdsc. So I guess this is ok, but it deserves a comment
>like "GDSC is in HW control" so we know what's going on. Also the
>commit text could be more explicit that clocks within the gdsc
>power domain don't work when the gdsc is off, and with hw control
>of a gdsc we can't tell when the gdsc may be off or on.
>

ok, i will reword the commit log better as above.

So i understand its ok to continue with this way of checking ?
since we are always having a static association which never changes,
than introducing additional fields in the clk_branch which can
get the status of the gdsc.

Regards,
 Sricharan

RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi Stephen,

>> >On 10/24, Sricharan R wrote:
>> >> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain 
>> >> *domain)
>> >>*/
>> >>   udelay(1);
>> >>
>> >> + /* Turn on HW trigger mode if supported */
>> >> + if (sc->flags & HW_CTRL)
>> >> + gdsc_hwctrl(sc, true);
>> >> +
>> >
>> >It sounds like this will cause glitches if the hardware isn't
>> >asserting their hw control bit by default? This has me concerned
>> >that we can't just throw the hw control enable part into here,
>> >because that bit doesn't live in the clock controller, instead it
>> >lives in the hw block that is powered by the power domain?
>> >
>> >Or does the power on reset value of that hw control signal
>> >asserted? If that's true then we should be ok to force it into hw
>> >control mode by default.
>> >
>>
>> The hw control bit is set by default. Instead its turned 'off'
>> with the reset value. So it has to not
>> be turned 'on' at some point
>> to put the gdsc in hw control if required. This bit is part of the
>> gdscr register. So i did not quite understand the reason for the
>> glitch here ?
>
>I mean the reset value of the hw control signal inside the device
>that is inside the GDSC power domain. For example, the hw control
>bit inside the video core.
>
 Ok, so the video ip core, has a hw control signal/bit.
 I checked this by dumping this out that,  the moment the
gdsc is put to hw control, the video ip's hw control bit also
gets asserted/set. so this means that video ip's bit get
aligned with the gdsc setting.  so this should avoid the
glitches, right ?

Regards,
 Sricharan

RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi,

>-Original Message-
>From: linux-arm-msm-ow...@vger.kernel.org 
>[mailto:linux-arm-msm-ow...@vger.kernel.org] On Behalf Of Sricharan
>Sent: Wednesday, November 02, 2016 12:21 PM
>To: 'Stephen Boyd' 
>Cc: mturque...@baylibre.com; linux-...@vger.kernel.org; 
>linux-arm-...@vger.kernel.org; linux-kernel@vger.kernel.org;
>rna...@codeaurora.org; stanimir.varba...@linaro.org
>Subject: RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control
>
>Hi Stephen,
>
>>On 10/24, Sricharan R wrote:
>>> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain 
>>> *domain)
>>>  */
>>> udelay(1);
>>>
>>> +   /* Turn on HW trigger mode if supported */
>>> +   if (sc->flags & HW_CTRL)
>>> +   gdsc_hwctrl(sc, true);
>>> +
>>
>>It sounds like this will cause glitches if the hardware isn't
>>asserting their hw control bit by default? This has me concerned
>>that we can't just throw the hw control enable part into here,
>>because that bit doesn't live in the clock controller, instead it
>>lives in the hw block that is powered by the power domain?
>>
>>Or does the power on reset value of that hw control signal
>>asserted? If that's true then we should be ok to force it into hw
>>control mode by default.
>>
>
>The hw control bit is set by default. Instead its turned 'off'
>with the reset value. So it has to not
>be turned 'on' at some point
>to put the gdsc in hw control if required. This bit is part of the
>gdscr register. So i did not quite understand the reason for the
>glitch here ?
>

typo above, i meant it has to be turned 'on' at some point
if required.

Regards,
 Sricharan

RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi Stephen,

>On 10/24, Sricharan R wrote:
>> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>   */
>>  udelay(1);
>>
>> +/* Turn on HW trigger mode if supported */
>> +if (sc->flags & HW_CTRL)
>> +gdsc_hwctrl(sc, true);
>> +
>
>It sounds like this will cause glitches if the hardware isn't
>asserting their hw control bit by default? This has me concerned
>that we can't just throw the hw control enable part into here,
>because that bit doesn't live in the clock controller, instead it
>lives in the hw block that is powered by the power domain?
>
>Or does the power on reset value of that hw control signal
>asserted? If that's true then we should be ok to force it into hw
>control mode by default.
>

The hw control bit is set by default. Instead its turned 'off'
with the reset value. So it has to not 
be turned 'on' at some point
to put the gdsc in hw control if required. This bit is part of the
gdscr register. So i did not quite understand the reason for the
glitch here ?

Regards,
 Sricharan

RE: [PATCH V1]iio: adc: spmi-vadc: Changes to support different scaling

[Sricharan]

Hi Ramakrishna,

[snip..]

>>> +   u32 i = 0;
>>> +
>>> +   if (!pts)
>>> +   return -EINVAL;
>>> +
>>> +   /* Check if table is descending or ascending */
>>> +   if (tablesize > 1) {
>>> +   if (pts[0].x < pts[1].x)
>>> +   descending = 0;
>>> +   }
>>> +
>>> +   while (i < tablesize) {
>>> +   if ((descending == 1) && (pts[i].x < input)) {
>>
>>  Just if (descending) instead of (descending == 1) and so on for the 
>> below as well
>
>   Will change in next patch.
>
>>
>>> +   /* table entry is less than measured*/
>>> +/* value and table is descending, stop */
>>> +   break;
>>> +   } else if ((descending == 0) &&
>>> +   (pts[i].x > input)) {
>>> +   /* table entry is greater than measured*/
>>> +   /*value and table is ascending, stop */
>>> +   break;
>>> +   }
>>> +   i++;
>>> +   }
>>> +
>>> +   if (i == 0) {
>>> +   *output = pts[0].y;
>>> +   } else if (i == tablesize) {
>>> +   *output = pts[tablesize - 1].y;
>>> +   } else {
>>> +   /* result is between search_index and search_index-1 */
>>> +   /* interpolate linearly */
>>> +   *output = (((s32)((pts[i].y - pts[i - 1].y) *
>>> +   (input - pts[i - 1].x)) /
>>> +   (pts[i].x - pts[i - 1].x)) +
>>> +   pts[i - 1].y);
>>> +   }
>>
>>hmm, so for descending, input - pts[i -1].x is negative and
>>we are adding that to pts[i-1].y, is that correct ?
>
>   The formula used is to interpolate between two points   using 
> linear
>interpolation.

 Right, agree. my question can be ignored.

[snip..]

>>> #define VADC_CHAN_TEMP(_dname, _pre)
>>> \
>>> -   VADC_CHAN(_dname, IIO_TEMP, BIT(IIO_CHAN_INFO_PROCESSED), _pre) \
>>> +   VADC_CHAN(_dname, IIO_TEMP, \
>>> +   BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_PROCESSED), \
>>> +   _pre)   \
>>>
>>> #define VADC_CHAN_VOLT(_dname, _pre)
>>> \
>>> -   VADC_CHAN(_dname, IIO_VOLTAGE,  \
>>> - BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>>> +   VADC_CHAN(_dname, IIO_VOLTAGE,  \
>>> + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_PROCESSED),\
>>>   _pre) \
>>>
>>   For this and the below changes to VADC_CHAN_VOLT to TEMP, why is that done 
>> ?
>>Now both macros are setting the same flags.
>
>   For Voltage channels IIO_VOLTAGE is needed where as for Temperature
>channels IIO_TEMP is needed.
>
>>
>>> /*
>>> @@ -637,12 +811,11 @@ struct vadc_channels {
>>> VADC_CHAN_TEMP(DIE_TEMP, 0)
>>> VADC_CHAN_VOLT(REF_625MV, 0)
>>> VADC_CHAN_VOLT(REF_1250MV, 0)
>>> -   VADC_CHAN_VOLT(CHG_TEMP, 0)
>>> +   VADC_CHAN_TEMP(CHG_TEMP, 0)
>>> VADC_CHAN_VOLT(SPARE1, 0)
>>> VADC_CHAN_VOLT(SPARE2, 0)
>>> VADC_CHAN_VOLT(GND_REF, 0)
>>> VADC_CHAN_VOLT(VDD_VADC, 0)
>>> -

And also looks like the deletion of these and below 
new lines are unnecessary ?

Regards,
 Sricharan

RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi Stan,

>Hi Sricharan,
>
>On 10/24/2016 01:18 PM, Sricharan R wrote:
>> From: Rajendra Nayak 
>>
>> Some GDSCs might support a HW control mode, where in the power
>> domain (gdsc) is brought in and out of low power state (while
>> unsued) without any SW assistance, saving power.
>> Such GDSCs can be configured in a HW control mode when powered on
>> until they are explicitly requested to be powered off by software.
>>
>> Signed-off-by: Rajendra Nayak 
>> Signed-off-by: Sricharan R 
>> ---
>>  drivers/clk/qcom/gdsc.c | 15 +++
>>  drivers/clk/qcom/gdsc.h |  1 +
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index f12d7b2..a5e1c8c 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int 
>> reg)
>>  return !!(val & PWR_ON_MASK);
>>  }
>>
>> +static int gdsc_hwctrl(struct gdsc *sc, bool en)
>> +{
>> +u32 val = en ? HW_CONTROL_MASK : 0;
>> +
>> +return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
>> +}
>> +
>>  static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>>  {
>>  int ret;
>> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>   */
>>  udelay(1);
>>
>> +/* Turn on HW trigger mode if supported */
>> +if (sc->flags & HW_CTRL)
>> +gdsc_hwctrl(sc, true);
>
   Sure, will add the check.

Regards,
 Sricharan

RE: [PATCH V1]iio: adc: spmi-vadc: Changes to support different scaling

[Sricharan]

1_BAT_THERM, 0)
>   VADC_CHAN_VOLT(LR_MUX2_BAT_ID, 0)
>   VADC_CHAN_VOLT(LR_MUX3_XO_THERM, 0)
>@@ -690,42 +861,40 @@ struct vadc_channels {
>   VADC_CHAN_VOLT(AMUX_PU1, 0)
>   VADC_CHAN_VOLT(AMUX_PU2, 0)
>   VADC_CHAN_VOLT(LR_MUX3_BUF_XO_THERM, 0)
>-
>-  VADC_CHAN_VOLT(LR_MUX1_PU1_BAT_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX1_PU1_BAT_THERM, 0)
>   VADC_CHAN_VOLT(LR_MUX2_PU1_BAT_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX3_PU1_XO_THERM, 0)
>-  VADC_CHAN_VOLT(LR_MUX4_PU1_AMUX_THM1, 0)
>-  VADC_CHAN_VOLT(LR_MUX5_PU1_AMUX_THM2, 0)
>-  VADC_CHAN_VOLT(LR_MUX6_PU1_AMUX_THM3, 0)
>+  VADC_CHAN_TEMP(LR_MUX3_PU1_XO_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX4_PU1_AMUX_THM1, 0)
>+  VADC_CHAN_TEMP(LR_MUX5_PU1_AMUX_THM2, 0)
>+  VADC_CHAN_TEMP(LR_MUX6_PU1_AMUX_THM3, 0)
>   VADC_CHAN_VOLT(LR_MUX7_PU1_AMUX_HW_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX8_PU1_AMUX_THM4, 0)
>-  VADC_CHAN_VOLT(LR_MUX9_PU1_AMUX_THM5, 0)
>+  VADC_CHAN_TEMP(LR_MUX8_PU1_AMUX_THM4, 0)
>+  VADC_CHAN_TEMP(LR_MUX9_PU1_AMUX_THM5, 0)
>   VADC_CHAN_VOLT(LR_MUX10_PU1_AMUX_USB_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX3_BUF_PU1_XO_THERM, 0)
>-
>-  VADC_CHAN_VOLT(LR_MUX1_PU2_BAT_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX3_BUF_PU1_XO_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX1_PU2_BAT_THERM, 0)
>   VADC_CHAN_VOLT(LR_MUX2_PU2_BAT_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX3_PU2_XO_THERM, 0)
>-  VADC_CHAN_VOLT(LR_MUX4_PU2_AMUX_THM1, 0)
>-  VADC_CHAN_VOLT(LR_MUX5_PU2_AMUX_THM2, 0)
>-  VADC_CHAN_VOLT(LR_MUX6_PU2_AMUX_THM3, 0)
>+  VADC_CHAN_TEMP(LR_MUX3_PU2_XO_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX4_PU2_AMUX_THM1, 0)
>+  VADC_CHAN_TEMP(LR_MUX5_PU2_AMUX_THM2, 0)
>+  VADC_CHAN_TEMP(LR_MUX6_PU2_AMUX_THM3, 0)
>   VADC_CHAN_VOLT(LR_MUX7_PU2_AMUX_HW_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX8_PU2_AMUX_THM4, 0)
>-  VADC_CHAN_VOLT(LR_MUX9_PU2_AMUX_THM5, 0)
>+  VADC_CHAN_TEMP(LR_MUX8_PU2_AMUX_THM4, 0)
>+  VADC_CHAN_TEMP(LR_MUX9_PU2_AMUX_THM5, 0)
>   VADC_CHAN_VOLT(LR_MUX10_PU2_AMUX_USB_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX3_BUF_PU2_XO_THERM, 0)
>-
>-  VADC_CHAN_VOLT(LR_MUX1_PU1_PU2_BAT_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX3_BUF_PU2_XO_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX1_PU1_PU2_BAT_THERM, 0)
>   VADC_CHAN_VOLT(LR_MUX2_PU1_PU2_BAT_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX3_PU1_PU2_XO_THERM, 0)
>-  VADC_CHAN_VOLT(LR_MUX4_PU1_PU2_AMUX_THM1, 0)
>-  VADC_CHAN_VOLT(LR_MUX5_PU1_PU2_AMUX_THM2, 0)
>-  VADC_CHAN_VOLT(LR_MUX6_PU1_PU2_AMUX_THM3, 0)
>+  VADC_CHAN_TEMP(LR_MUX3_PU1_PU2_XO_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX4_PU1_PU2_AMUX_THM1, 0)
>+  VADC_CHAN_TEMP(LR_MUX5_PU1_PU2_AMUX_THM2, 0)
>+  VADC_CHAN_TEMP(LR_MUX6_PU1_PU2_AMUX_THM3, 0)
>   VADC_CHAN_VOLT(LR_MUX7_PU1_PU2_AMUX_HW_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX8_PU1_PU2_AMUX_THM4, 0)
>-  VADC_CHAN_VOLT(LR_MUX9_PU1_PU2_AMUX_THM5, 0)
>+  VADC_CHAN_TEMP(LR_MUX8_PU1_PU2_AMUX_THM4, 0)
>+  VADC_CHAN_TEMP(LR_MUX9_PU1_PU2_AMUX_THM5, 0)
>   VADC_CHAN_VOLT(LR_MUX10_PU1_PU2_AMUX_USB_ID, 0)
>-  VADC_CHAN_VOLT(LR_MUX3_BUF_PU1_PU2_XO_THERM, 0)
>+  VADC_CHAN_TEMP(LR_MUX3_BUF_PU1_PU2_XO_THERM, 0)
>+
> };
>
> static int vadc_get_dt_channel_data(struct device *dev,
>@@ -802,6 +971,11 @@ static int vadc_get_dt_channel_data(struct device *dev,
>   prop->avg_samples = VADC_DEF_AVG_SAMPLES;
>   }
>
>+  ret = of_property_read_u32(node, "qcom,scale-function",
>+ &prop->scale_function);
>+  if (ret)
>+  prop->scale_function = SCALE_DEFAULT;
>+


 Is this a new binding, in that case the documentation has to be updated 
for this and
 probably introduce this in one first patch and more patches for the rest 
of the changes.

>   if (of_property_read_bool(node, "qcom,ratiometric"))
>   prop->calibration = VADC_CALIB_RATIOMETRIC;
>   else
>@@ -850,9 +1024,9 @@ static int vadc_get_dt_data(struct vadc_priv *vadc, 
>struct device_node *node)
>
>   iio_chan->channel = prop.channel;
>   iio_chan->datasheet_name = vadc_chan->datasheet_name;
>+  iio_chan->extend_name = child->name;
>   iio_chan->info_mask_separate = vadc_chan->info_mask;
>   iio_chan->type = vadc_chan->type;
>-  iio_chan->indexed = 1;
>   iio_chan->address = index++;
>
>   iio_chan++;
>@@ -964,16 +1138,21 @@ static int vadc_probe(struct platform_device *pdev)
>   if (ret)
>   return ret;
>
>-  irq_eoc = platform_get_irq(pdev, 0);
>-  if (irq_eoc < 0) {
>-  if (irq_eoc == -EPROBE_DEFER || irq_eoc == -EINVAL)
>-  return irq_eoc;
>-  vadc->poll_eoc = true;
>-  } else {
>-  ret = devm_request_irq(dev, irq_eoc, vadc_isr, 0,
>- "spmi-vadc", vadc);
>-  if (ret)
>-  return ret;
>+  vadc->poll_eoc = of_property_read_bool(node,
>+  "qcom,vadc-poll-eoc");
>+

 Same comment as above for introducing the new binding and the 
reason 
  for that.

>+  if (!vadc->poll_eoc) {
>+  irq_eoc = platform_get_irq(pdev, 0);
>+  if (irq_eoc < 0) {
>+  if (irq_eoc == -EPROBE_DEFER || irq_eoc == -EINVAL)
>+  return irq_eoc;
>+  vadc->poll_eoc = true;
>+  } else {
>+  ret = devm_request_irq(dev, irq_eoc, vadc_isr, 0,
>+ "spmi-vadc", vadc);
>+  if (ret)
>+  return ret;
>+  }
>   }
>
>   ret = vadc_reset(vadc);

Regards,
 Sricharan

RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Sricharan]

Hi Marek,

>>>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>>>> index 5e6d7bbf9b70..59b4f2ce4f5f 100644
>>>>> --- a/drivers/iommu/exynos-iommu.c
>>>>> +++ b/drivers/iommu/exynos-iommu.c
>>>>> @@ -781,10 +781,6 @@ static void exynos_iommu_detach_device(struct 
>>>>> iommu_domain *iommu_domain,
>>>>>   if (!has_sysmmu(dev) || owner->domain != iommu_domain)
>>>>>   return;
>>>>>
>>>>> - list_for_each_entry(data, &owner->controllers, owner_node) {
>>>>> - pm_runtime_put_sync(data->sysmmu);
>>>>> - }
>>>>> -
>>>>>   mutex_lock(&owner->rpm_lock);
>>>>>
>>>>>   list_for_each_entry(data, &owner->controllers, owner_node) {
>>>>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct 
>>>>> iommu_domain *iommu_domain,
>>>>>
>>>>>   mutex_unlock(&owner->rpm_lock);
>>>>>
>>>>> - list_for_each_entry(data, &owner->controllers, owner_node) {
>>>>> - pm_runtime_get_sync(data->sysmmu);
>>>>> - }
>>>>> -
>>>>>   dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>>>>>   &pagetable);
>>>>>
>>>>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device 
>>>>> *dev,
>>>>>
>>>>>   list_add_tail(&data->owner_node, &owner->controllers);
>>>>>   data->master = dev;
>>>>> +
>>>>> + /*
>>>>> +  * SYSMMU will be runtime activated via device link (dependency) to its
>>>>> +  * master device, so there are no direct calls to pm_runtime_get/put
>>>>> +  * in this driver.
>>>>> +  */
>>>>> + device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>>>>> +
>>>> So in the case of master with multiple sids, this would be called 
>>>> multiple times
>>>> for the same master ?
>>> I don't know what is "multiple sids" case, but if given SYSMMU master
>>> device (supplier)
>>> has multiple SYSMMU controllers (consumers), then links will be created
>>> for each SYSMMU
>>> controller. Please note that this code is based on vanilla v4.9-rc1,
>>> which calls
>>> of_xlate() callback only once for every iommu for given master device.
>>> Your IOMMU
>>> deferred probe patches change this, but I already posted a fix for
>>> Exynos IOMMU driver
>>> to handle such case.
>>   By multiple sids, i meant iommus = <&phandle sid1 sid2 .. sidn> case,
>>   so xlate would be called multiples for the same master without deferred
>>   probing also. But the fix that you showed on the other thread would work
>>   here as well or maybe if you dont have masters with multiple sids you wont
>>   have any issues as well.
>
>Exynos SYSMMU driver always use "#iommu-cells = <0>", so it doesn't support
>multiple sids. However there is a case with 2 SYSMMU controllers attached
>to the same master device: "iommus = <&sysmmu_mfc_l>, <&sysmmu_mfc_r>;".
>
   with iommu-cells = <0> always, its ok. The case of 2 SYSMMU controllers that
   is shown above is fine, as anyways both the suppliers (symmu) needs to linked
seperately. So it looks all fine.

Regards,
  Sricharan

RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Sricharan]

Hi Marek,

>>> This patch uses recently introduced device dependency links to track the
>>> runtime pm state of the master's device. This way each SYSMMU controller
>>> is set to runtime active only when its master's device is active and can
>>> restore or save its state instead of being activated all the time when
>>> attached to the given master device. This way SYSMMU controllers no longer
>>> prevents respective power domains to be turned off when master's device
>>> is not being used.
>>>
>>> Signed-off-by: Marek Szyprowski 
>>> ---
>>> drivers/iommu/exynos-iommu.c | 16 
>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>> index 5e6d7bbf9b70..59b4f2ce4f5f 100644
>>> --- a/drivers/iommu/exynos-iommu.c
>>> +++ b/drivers/iommu/exynos-iommu.c
>>> @@ -781,10 +781,6 @@ static void exynos_iommu_detach_device(struct 
>>> iommu_domain *iommu_domain,
>>> if (!has_sysmmu(dev) || owner->domain != iommu_domain)
>>> return;
>>>
>>> -   list_for_each_entry(data, &owner->controllers, owner_node) {
>>> -   pm_runtime_put_sync(data->sysmmu);
>>> -   }
>>> -
>>> mutex_lock(&owner->rpm_lock);
>>>
>>> list_for_each_entry(data, &owner->controllers, owner_node) {
>>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct 
>>> iommu_domain *iommu_domain,
>>>
>>> mutex_unlock(&owner->rpm_lock);
>>>
>>> -   list_for_each_entry(data, &owner->controllers, owner_node) {
>>> -   pm_runtime_get_sync(data->sysmmu);
>>> -   }
>>> -
>>> dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>>> &pagetable);
>>>
>>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>>>
>>> list_add_tail(&data->owner_node, &owner->controllers);
>>> data->master = dev;
>>> +
>>> +   /*
>>> +* SYSMMU will be runtime activated via device link (dependency) to its
>>> +* master device, so there are no direct calls to pm_runtime_get/put
>>> +* in this driver.
>>> +*/
>>> +   device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>>> +
>>So in the case of master with multiple sids, this would be called 
>> multiple times
>>for the same master ?
>
>I don't know what is "multiple sids" case, but if given SYSMMU master
>device (supplier)
>has multiple SYSMMU controllers (consumers), then links will be created
>for each SYSMMU
>controller. Please note that this code is based on vanilla v4.9-rc1,
>which calls
>of_xlate() callback only once for every iommu for given master device.
>Your IOMMU
>deferred probe patches change this, but I already posted a fix for
>Exynos IOMMU driver
>to handle such case.
  By multiple sids, i meant iommus = <&phandle sid1 sid2 .. sidn> case,
  so xlate would be called multiples for the same master without deferred
   probing also. But the fix that you showed on the other thread would work
   here as well or maybe if you dont have masters with multiple sids you 
wont
  have any issues as well.

Regards,
 Sricharan

RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support

[Sricharan]

Hi Marek,

>Hi Sricharan
>
>
>On 2016-10-22 07:50, Sricharan wrote:
>>
>>> This patch adds runtime pm implementation, which is based on previous
>>> suspend/resume code. SYSMMU controller is now being enabled/disabled mainly
>> > from the runtime pm callbacks. System sleep callbacks relies on generic
>>> pm_runtime_force_suspend/pm_runtime_force_resume helpers. To ensure
>>> internal state consistency, additional lock for runtime pm transitions
>>> was introduced.
>>>
>>> Signed-off-by: Marek Szyprowski 
>>> ---
>>> drivers/iommu/exynos-iommu.c | 45 
>>> +++-
>>> 1 file changed, 36 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>> index a959443e6f33..5e6d7bbf9b70 100644
>>> --- a/drivers/iommu/exynos-iommu.c
>>> +++ b/drivers/iommu/exynos-iommu.c
>>> @@ -206,6 +206,7 @@ struct sysmmu_fault_info {
>>> struct exynos_iommu_owner {
>>> struct list_head controllers;   /* list of sysmmu_drvdata.owner_node */
>>> struct iommu_domain *domain;/* domain this device is attached */
>>> +   struct mutex rpm_lock;  /* for runtime pm of all sysmmus */
>>> };
>>>
>>> /*
>>> @@ -594,40 +595,46 @@ static int __init exynos_sysmmu_probe(struct 
>>> platform_device *pdev)
>>> return 0;
>>> }
>>>
>>> -#ifdef CONFIG_PM_SLEEP
>>> -static int exynos_sysmmu_suspend(struct device *dev)
>>> +static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
>>> {
>>> struct sysmmu_drvdata *data = dev_get_drvdata(dev);
>>> struct device *master = data->master;
>>>
>>> if (master) {
>>> -   pm_runtime_put(dev);
>>> +   struct exynos_iommu_owner *owner = master->archdata.iommu;
>>> +
>>> +   mutex_lock(&owner->rpm_lock);
>> More of a device link question,
>> To understand, i see that with device link + runtime, the supplier
>> callbacks are not called for irqsafe clients, even if supplier is irqsafe.
>> Why so ?
>
>Frankly I didn't care about irqsafe runtime pm, because there is no such
>need
>for Exynos platform and its drivers. Exynos power domain driver also doesn't
>support irqsafe mode.
  ok, i asked this because, i was doing the same thing for arm-smmu driver
   and thought that when we depend on device-link for doing the runtime pm,
   then it might not work for irqsafe master. Probably i can ask this on device 
link
series post.

Regards,
 Sricharan

[PATCH 2/3] clk: qcom: Put venus core0/1 gdscs to hw control mode

[Sricharan R]

The venus video ip's internal core blocks are under the
control of the firmware and their powerdomains needs to be
'ON' only when used by the firmware. So putting it into
hw controlled mode lets this to happen, otherwise the firmware
hangs checking for this.

Signed-off-by: Sricharan R 
---
 drivers/clk/qcom/mmcc-msm8996.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index ca97e11..41aabe3 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -2945,6 +2945,7 @@ enum {
.name = "venus_core0",
},
.pwrsts = PWRSTS_OFF_ON,
+   .flags = HW_CTRL,
 };
 
 static struct gdsc venus_core1_gdsc = {
@@ -2955,6 +2956,7 @@ enum {
.name = "venus_core1",
},
.pwrsts = PWRSTS_OFF_ON,
+   .flags = HW_CTRL,
 };
 
 static struct gdsc camss_gdsc = {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan R]

From: Rajendra Nayak 

Some GDSCs might support a HW control mode, where in the power
domain (gdsc) is brought in and out of low power state (while
unsued) without any SW assistance, saving power.
Such GDSCs can be configured in a HW control mode when powered on
until they are explicitly requested to be powered off by software.

Signed-off-by: Rajendra Nayak 
Signed-off-by: Sricharan R 
---
 drivers/clk/qcom/gdsc.c | 15 +++
 drivers/clk/qcom/gdsc.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index f12d7b2..a5e1c8c 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg)
return !!(val & PWR_ON_MASK);
 }
 
+static int gdsc_hwctrl(struct gdsc *sc, bool en)
+{
+   u32 val = en ? HW_CONTROL_MASK : 0;
+
+   return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 {
int ret;
@@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 */
udelay(1);
 
+   /* Turn on HW trigger mode if supported */
+   if (sc->flags & HW_CTRL)
+   gdsc_hwctrl(sc, true);
+
return 0;
 }
 
@@ -174,6 +185,10 @@ static int gdsc_disable(struct generic_pm_domain *domain)
if (sc->pwrsts == PWRSTS_ON)
return gdsc_assert_reset(sc);
 
+   /* Turn off HW trigger mode if supported */
+   if (sc->flags & HW_CTRL)
+   gdsc_hwctrl(sc, false);
+
if (sc->pwrsts & PWRSTS_OFF)
gdsc_clear_mem_on(sc);
 
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 3bf497c..b1f30f8 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -50,6 +50,7 @@ struct gdsc {
 #define PWRSTS_RET_ON  (PWRSTS_RET | PWRSTS_ON)
const u8flags;
 #define VOTABLEBIT(0)
+#define HW_CTRLBIT(1)
struct reset_controller_dev *rcdev;
unsigned int*resets;
unsigned intreset_count;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH 0/3] clk: qcom: Add support for hw controlled gdscs/clocks

[Sricharan R]

This series adds support for gdscs(powerdomains) that can be configured
in hw controlled mode. So they are turned 'ON' based on needs dynamically,
helping to save power. Also updated the venus video ip's gdsc/clock
data to put them in hw control.

Rajendra Nayak (1):
  clk: qcom: gdsc: Add support for gdscs with HW control

Sricharan R (2):
  clk: qcom: Put venus core0/1 gdscs to hw control mode
  clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

 drivers/clk/qcom/gdsc.c | 15 +++
 drivers/clk/qcom/gdsc.h |  1 +
 drivers/clk/qcom/mmcc-msm8996.c |  4 
 3 files changed, 20 insertions(+)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan R]

With the venus subcore0/1 gdscs(powerdomains) in
hw controlled mode, the clock controller does not handle
the status bit for the clocks in that domain. So avoid
checking for the status bit of those clocks by setting the
BRANCH_HALT_DELAY flag. This avoids the WARN_ONs which
otherwise occurs when enabling/disabling those clocks.

Signed-off-by: Sricharan R 
---
 drivers/clk/qcom/mmcc-msm8996.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index 41aabe3..8f3f480 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -1760,6 +1760,7 @@ enum {
 };
 
 static struct clk_branch video_subcore0_clk = {
+   .halt_check = BRANCH_HALT_DELAY,
.halt_reg = 0x1048,
.clkr = {
.enable_reg = 0x1048,
@@ -1775,6 +1776,7 @@ enum {
 };
 
 static struct clk_branch video_subcore1_clk = {
+   .halt_check = BRANCH_HALT_DELAY,
.halt_reg = 0x104c,
.clkr = {
.enable_reg = 0x104c,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Sricharan]

Hi Marek,
>This patch uses recently introduced device dependency links to track the
>runtime pm state of the master's device. This way each SYSMMU controller
>is set to runtime active only when its master's device is active and can
>restore or save its state instead of being activated all the time when
>attached to the given master device. This way SYSMMU controllers no longer
>prevents respective power domains to be turned off when master's device
>is not being used.
>
>Signed-off-by: Marek Szyprowski 
>---
> drivers/iommu/exynos-iommu.c | 16 
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>index 5e6d7bbf9b70..59b4f2ce4f5f 100644
>--- a/drivers/iommu/exynos-iommu.c
>+++ b/drivers/iommu/exynos-iommu.c
>@@ -781,10 +781,6 @@ static void exynos_iommu_detach_device(struct 
>iommu_domain *iommu_domain,
>   if (!has_sysmmu(dev) || owner->domain != iommu_domain)
>   return;
>
>-  list_for_each_entry(data, &owner->controllers, owner_node) {
>-  pm_runtime_put_sync(data->sysmmu);
>-  }
>-
>   mutex_lock(&owner->rpm_lock);
>
>   list_for_each_entry(data, &owner->controllers, owner_node) {
>@@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain 
>*iommu_domain,
>
>   mutex_unlock(&owner->rpm_lock);
>
>-  list_for_each_entry(data, &owner->controllers, owner_node) {
>-  pm_runtime_get_sync(data->sysmmu);
>-  }
>-
>   dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>   &pagetable);
>
>@@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>
>   list_add_tail(&data->owner_node, &owner->controllers);
>   data->master = dev;
>+
>+  /*
>+   * SYSMMU will be runtime activated via device link (dependency) to its
>+   * master device, so there are no direct calls to pm_runtime_get/put
>+   * in this driver.
>+   */
>+  device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>+
  So in the case of master with multiple sids, this would be called multiple 
times
  for the same master ?

Regards,
 Sricharan

RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support

[Sricharan]

Hi Marek,

>This patch adds runtime pm implementation, which is based on previous
>suspend/resume code. SYSMMU controller is now being enabled/disabled mainly
>from the runtime pm callbacks. System sleep callbacks relies on generic
>pm_runtime_force_suspend/pm_runtime_force_resume helpers. To ensure
>internal state consistency, additional lock for runtime pm transitions
>was introduced.
>
>Signed-off-by: Marek Szyprowski 
>---
> drivers/iommu/exynos-iommu.c | 45 +++-
> 1 file changed, 36 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>index a959443e6f33..5e6d7bbf9b70 100644
>--- a/drivers/iommu/exynos-iommu.c
>+++ b/drivers/iommu/exynos-iommu.c
>@@ -206,6 +206,7 @@ struct sysmmu_fault_info {
> struct exynos_iommu_owner {
>   struct list_head controllers;   /* list of sysmmu_drvdata.owner_node */
>   struct iommu_domain *domain;/* domain this device is attached */
>+  struct mutex rpm_lock;  /* for runtime pm of all sysmmus */
> };
>
> /*
>@@ -594,40 +595,46 @@ static int __init exynos_sysmmu_probe(struct 
>platform_device *pdev)
>   return 0;
> }
>
>-#ifdef CONFIG_PM_SLEEP
>-static int exynos_sysmmu_suspend(struct device *dev)
>+static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
> {
>   struct sysmmu_drvdata *data = dev_get_drvdata(dev);
>   struct device *master = data->master;
>
>   if (master) {
>-  pm_runtime_put(dev);
>+  struct exynos_iommu_owner *owner = master->archdata.iommu;
>+
>+  mutex_lock(&owner->rpm_lock);
More of a device link question,
To understand, i see that with device link + runtime, the supplier
callbacks are not called for irqsafe clients, even if supplier is irqsafe.
Why so ?

>   if (data->domain) {
>   dev_dbg(data->sysmmu, "saving state\n");
>   __sysmmu_disable(data);
>   }
>+  mutex_unlock(&owner->rpm_lock);
>   }
>   return 0;
> }
>
>-static int exynos_sysmmu_resume(struct device *dev)
>+static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
> {
>   struct sysmmu_drvdata *data = dev_get_drvdata(dev);
>   struct device *master = data->master;
>
>   if (master) {
>-  pm_runtime_get_sync(dev);
>+  struct exynos_iommu_owner *owner = master->archdata.iommu;
>+
>+  mutex_lock(&owner->rpm_lock);
>   if (data->domain) {
>   dev_dbg(data->sysmmu, "restoring state\n");
>   __sysmmu_enable(data);
>   }
>+  mutex_unlock(&owner->rpm_lock);
>   }
>   return 0;
> }
>-#endif
>
> static const struct dev_pm_ops sysmmu_pm_ops = {
>-  SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend, 
>exynos_sysmmu_resume)
>+  SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
>+  SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>+   pm_runtime_force_resume)
> };
 Is this needed to be LATE_SYSTEM_SLEEP_PM_OPS with device links to take care
  of the order ?

Regards,
 Sricharan

[PATCH] firmware: qcom: scm: Fix interrupted SCM calls fully

[Sricharan R]

Patch [1] fixes the issues with interrupted SCM64 calls
by returning the register r6 (Session ID) on the next try, but register
r0 also needs to be preserved for the retry. r0 contains the
result of the previous try. Without this i see that the SCM calls
hang when retried.

[1] https://patchwork.kernel.org/patch/9291589/

Signed-off-by: Sricharan R 
---
 drivers/firmware/qcom_scm-64.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 7ecd0e7..28b604e 100755
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -134,6 +134,9 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, 
u32 cmd_id,
res->a6 = 0;
 
do {
+   if (res->a0 == QCOM_SCM_INTERRUPTED)
+   cmd = res->a0;
+
arm_smccc_smc(cmd, desc->arginfo, desc->args[0],
  desc->args[1], desc->args[2], x5, 
res->a6, 0,
  res);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH V4 2/3] i2c: qup: Fix wrong value of index variable

[Sricharan]

+Wolfram Sang

>-Original Message-
>From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
>On Behalf Of Sricharan R
>Sent: Friday, June 10, 2016 11:38 PM
>To: linux-arm-...@vger.kernel.org; ntel...@codeaurora.org; 
>ga...@codeaurora.org; linux-kernel@vger.kernel.org;
>andy.gr...@linaro.org; linux-...@vger.kernel.org; agr...@codeaurora.org; 
>linux-arm-ker...@lists.infradead.org;
>nk...@codeaurora.org; abs...@codeaurora.org
>Cc: sricha...@codeaurora.org
>Subject: [PATCH V4 2/3] i2c: qup: Fix wrong value of index variable
>
>index gets incremented during check to determine if the
>messages can be transferred with dma. But not reset after
>that, resulting in wrong start value in subsequent loop,
>causing failure. Fix it.
>
>Signed-off-by: Sricharan R 
>---
> drivers/i2c/busses/i2c-qup.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>index 43adc2d..b8116e5 100644
>--- a/drivers/i2c/busses/i2c-qup.c
>+++ b/drivers/i2c/busses/i2c-qup.c
>@@ -1260,6 +1260,8 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
>   }
>   }
>
>+  idx = 0;
>+
>   do {
>   if (msgs[idx].len == 0) {
>   ret = -EINVAL;
>--
>QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
>Code Aurora Forum, hosted by The Linux
>Foundation
>
>
>___
>linux-arm-kernel mailing list
>linux-arm-ker...@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V4 3/3] i2c: qup: Fix error handling

[Sricharan]

+Wolfram Sang

>-Original Message-
>From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
>On Behalf Of Sricharan R
>Sent: Friday, June 10, 2016 11:38 PM
>To: linux-arm-...@vger.kernel.org; ntel...@codeaurora.org; 
>ga...@codeaurora.org; linux-kernel@vger.kernel.org;
>andy.gr...@linaro.org; linux-...@vger.kernel.org; agr...@codeaurora.org; 
>linux-arm-ker...@lists.infradead.org;
>nk...@codeaurora.org; abs...@codeaurora.org
>Cc: sricha...@codeaurora.org
>Subject: [PATCH V4 3/3] i2c: qup: Fix error handling
>
>Among the bus errors reported from the QUP_MASTER_STATUS register
>only NACK is considered and transfer gets suspended, while
>other errors are ignored. Correct this and suspend the transfer
>for other errors as well. This avoids unnecessary 'timeouts' which
>happens when waiting for events that would never happen when there
>is already an error condition on the bus. Also the error handling
>procedure should be the same for both NACK and other bus errors in
>case of dma mode. So correct that as well.
>
>Signed-off-by: Sricharan R 
>---
> drivers/i2c/busses/i2c-qup.c | 76 
> 1 file changed, 41 insertions(+), 35 deletions(-)
>
>diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>index b8116e5..44c80ae 100644
>--- a/drivers/i2c/busses/i2c-qup.c
>+++ b/drivers/i2c/busses/i2c-qup.c
>@@ -310,6 +310,7 @@ static int qup_i2c_wait_ready(struct qup_i2c_dev *qup, int 
>op, bool val,
>   u32 opflags;
>   u32 status;
>   u32 shift = __ffs(op);
>+  int ret = 0;
>
>   len *= qup->one_byte_t;
>   /* timeout after a wait of twice the max time */
>@@ -321,18 +322,28 @@ static int qup_i2c_wait_ready(struct qup_i2c_dev *qup, 
>int op, bool val,
>
>   if (((opflags & op) >> shift) == val) {
>   if ((op == QUP_OUT_NOT_EMPTY) && qup->is_last) {
>-  if (!(status & I2C_STATUS_BUS_ACTIVE))
>-  return 0;
>+  if (!(status & I2C_STATUS_BUS_ACTIVE)) {
>+  ret = 0;
>+  goto done;
>+  }
>   } else {
>-  return 0;
>+  ret = 0;
>+  goto done;
>   }
>   }
>
>-  if (time_after(jiffies, timeout))
>-  return -ETIMEDOUT;
>-
>+  if (time_after(jiffies, timeout)) {
>+  ret = -ETIMEDOUT;
>+  goto done;
>+  }
>   usleep_range(len, len * 2);
>   }
>+
>+done:
>+  if (qup->bus_err || qup->qup_err)
>+  ret =  (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
>+
>+  return ret;
> }
>
> static void qup_i2c_set_write_mode_v2(struct qup_i2c_dev *qup,
>@@ -793,39 +804,35 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
>struct i2c_msg *msg,
>   }
>
>   if (ret || qup->bus_err || qup->qup_err) {
>-  if (qup->bus_err & QUP_I2C_NACK_FLAG) {
>-  msg--;
>-  dev_err(qup->dev, "NACK from %x\n", msg->addr);
>-  ret = -EIO;
>+  if (qup_i2c_change_state(qup, QUP_RUN_STATE)) {
>+  dev_err(qup->dev, "change to run state timed out");
>+  goto desc_err;
>+  }
>
>-  if (qup_i2c_change_state(qup, QUP_RUN_STATE)) {
>-  dev_err(qup->dev, "change to run state timed 
>out");
>-  return ret;
>-  }
>+  if (rx_nents)
>+  writel(QUP_BAM_INPUT_EOT,
>+ qup->base + QUP_OUT_FIFO_BASE);
>
>-  if (rx_nents)
>-  writel(QUP_BAM_INPUT_EOT,
>- qup->base + QUP_OUT_FIFO_BASE);
>+  writel(QUP_BAM_FLUSH_STOP, qup->base + QUP_OUT_FIFO_BASE);
>
>-  writel(QUP_BAM_FLUSH_STOP,
>- qup->base + QUP_OUT_FIFO_BASE);
>+  qup_i2c_flush(qup);
>
>-  qup_i2c_flush(qup);
>+  /* wait for remaining interrupts to occur */
>+  if (!wait_for_completion_timeout(&qup->xfer, HZ))
>+  dev_err(qup->dev, "flush timed out\n"

RE: [PATCH V4 1/3] i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled

[Sricharan]

+Wolfram Sang

>-Original Message-
>From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
>On Behalf Of Sricharan R
>Sent: Friday, June 10, 2016 11:38 PM
>To: linux-arm-...@vger.kernel.org; ntel...@codeaurora.org; 
>ga...@codeaurora.org; linux-kernel@vger.kernel.org;
>andy.gr...@linaro.org; linux-...@vger.kernel.org; agr...@codeaurora.org; 
>linux-arm-ker...@lists.infradead.org;
>nk...@codeaurora.org; abs...@codeaurora.org
>Cc: sricha...@codeaurora.org
>Subject: [PATCH V4 1/3] i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is 
>enabled
>
>With CONFIG_DEBUG_SG is enabled and when dma mode is used, below dump is seen,
>
>[ cut here ]
>kernel BUG at include/linux/scatterlist.h:140!
>Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>Modules linked in:
>CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-00459-g9f087b9-dirty #7
>Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
>task: ffc036868000 ti: ffc03687 task.ti: ffc03687
>PC is at qup_sg_set_buf.isra.13+0x138/0x154
>LR is at qup_sg_set_buf.isra.13+0x50/0x154
>pc : [] lr : [] pstate: 6145
>sp : ffc0368735c0
>x29: ffc0368735c0 x28: ffc036873752
>x27: ffc035233018 x26: ffc000c4e000
>x25:  x24: 0004
>x23:  x22: ffc035233668
>x21: ff80004e3000 x20: ffc0352e0018
>x19: 0040 x18: 0028
>x17: 0004 x16: ffc0017a39c8
>x15: 1cdf x14: ffc0019929d8
>x13: ffc0352e0018 x12: 
>x11: 0001 x10: 0001
>x9 : ffc0012b2d70 x8 : ff80004e3000
>x7 : 0018 x6 : 3000
>x5 : ffc00199f018 x4 : ffc035233018
>x3 : 0004 x2 : c000
>x1 : 0003 x0 : 
>
>Process swapper/0 (pid: 1, stack limit = 0xffc036870020)
>Stack: (0xffc0368735c0 to 0xffc036874000)
>
>sg_set_bug expects that the buf parameter passed in should be from
>lowmem and a valid pageframe. This is not true for pages from
>dma_alloc_coherent which can be carveouts, hence the check fails.
>Change allocation of sg buffers from dma_coherent memory to kzalloc
>to fix the issue. Note that now dma_map/unmap is used to make the
>kzalloc'ed buffers coherent before passing it to the dmaengine.
>
>Signed-off-by: Sricharan R 
>Reviewed-by: Andy Gross 
>---
> drivers/i2c/busses/i2c-qup.c | 51 +---
> 1 file changed, 15 insertions(+), 36 deletions(-)
>
>diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>index cc6439ab..43adc2d 100644
>--- a/drivers/i2c/busses/i2c-qup.c
>+++ b/drivers/i2c/busses/i2c-qup.c
>@@ -585,8 +585,8 @@ static void qup_i2c_bam_cb(void *data)
> }
>
> static int qup_sg_set_buf(struct scatterlist *sg, void *buf,
>-struct qup_i2c_tag *tg, unsigned int buflen,
>-struct qup_i2c_dev *qup, int map, int dir)
>+unsigned int buflen, struct qup_i2c_dev *qup,
>+int dir)
> {
>   int ret;
>
>@@ -595,9 +595,6 @@ static int qup_sg_set_buf(struct scatterlist *sg, void 
>*buf,
>   if (!ret)
>   return -EINVAL;
>
>-  if (!map)
>-  sg_dma_address(sg) = tg->addr + ((u8 *)buf - tg->start);
>-
>   return 0;
> }
>
>@@ -670,16 +667,15 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
>struct i2c_msg *msg,
>   /* scratch buf to read the start and len tags */
>   ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
>&qup->brx.tag.start[0],
>-   &qup->brx.tag,
>-   2, qup, 0, 0);
>+   2, qup, DMA_FROM_DEVICE);
>
>   if (ret)
>   return ret;
>
>   ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
>&msg->buf[limit * i],
>-   NULL, tlen, qup,
>-   1, DMA_FROM_DEVICE);
>+   tlen, qup,
>+   DMA_FROM_DEVICE);
>   if (ret)
>   return ret;
>
>@@ -688,7 +684,7 @@ static int qup_i2c_bam_do_xfer

RE: [PATCH V4 0/3] i2c: qup: Some misc fixes

[Sricharan]

+Wolfram. Sorry Missed.

>-Original Message-
>From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
>On Behalf Of Sricharan R
>Sent: Friday, June 10, 2016 11:38 PM
>To: linux-arm-...@vger.kernel.org; ntel...@codeaurora.org; 
>ga...@codeaurora.org; linux-kernel@vger.kernel.org;
>andy.gr...@linaro.org; linux-...@vger.kernel.org; agr...@codeaurora.org; 
>linux-arm-ker...@lists.infradead.org;
>nk...@codeaurora.org; abs...@codeaurora.org
>Cc: sricha...@codeaurora.org
>Subject: [PATCH V4 0/3] i2c: qup: Some misc fixes
>
>One for fixing the bug with CONFIG_DEBUG_SG enabled, another
>to suspend the transfer for all errors instead of just for NACK
>and one for correcting the wrong initial value for index variable.
>
>[V4] Added little more description to patch#1 and split a
> change in to a new patch.
>
>[V3] Added more commit description. Return more appropriate
> error codes for NACK and other bus errors. Corrected
> other bus errors handling procedure for dma mode as well.
> Removed the dev_err log for NACKs.
>
>[V2] Removed the use of unnecessary variable assignment.
>
>Kept the reviewed and Tested by tag for patch#1,
>as there was no code change.
>
>Depends on patch[1] for the error handling to be complete.
>
>[1] https://lkml.org/lkml/2016/5/9/447
>
>Sricharan R (3):
>  i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled
>  i2c: qup: Fix wrong value of index variable
>  i2c: qup: Fix error handling
>
> drivers/i2c/busses/i2c-qup.c | 128 ---
> 1 file changed, 58 insertions(+), 70 deletions(-)
>
>--
>QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
>Code Aurora Forum, hosted by The Linux
>Foundation
>
>
>___
>linux-arm-kernel mailing list
>linux-arm-ker...@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V4 0/3] i2c: qup: Some misc fixes

[Sricharan R]

One for fixing the bug with CONFIG_DEBUG_SG enabled, another
to suspend the transfer for all errors instead of just for NACK
and one for correcting the wrong initial value for index variable.

[V4] Added little more description to patch#1 and split a
 change in to a new patch.
 
[V3] Added more commit description. Return more appropriate
 error codes for NACK and other bus errors. Corrected
 other bus errors handling procedure for dma mode as well.
 Removed the dev_err log for NACKs.

[V2] Removed the use of unnecessary variable assignment.

Kept the reviewed and Tested by tag for patch#1,
as there was no code change.

Depends on patch[1] for the error handling to be complete.

[1] https://lkml.org/lkml/2016/5/9/447

Sricharan R (3):
  i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled
  i2c: qup: Fix wrong value of index variable
  i2c: qup: Fix error handling

 drivers/i2c/busses/i2c-qup.c | 128 ---
 1 file changed, 58 insertions(+), 70 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V4 2/3] i2c: qup: Fix wrong value of index variable

[Sricharan R]

index gets incremented during check to determine if the
messages can be transferred with dma. But not reset after
that, resulting in wrong start value in subsequent loop,
causing failure. Fix it.

Signed-off-by: Sricharan R 
---
 drivers/i2c/busses/i2c-qup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 43adc2d..b8116e5 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -1260,6 +1260,8 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
}
}
 
+   idx = 0;
+
do {
if (msgs[idx].len == 0) {
ret = -EINVAL;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH V4 3/3] i2c: qup: Fix error handling

[Sricharan R]

Among the bus errors reported from the QUP_MASTER_STATUS register
only NACK is considered and transfer gets suspended, while
other errors are ignored. Correct this and suspend the transfer
for other errors as well. This avoids unnecessary 'timeouts' which
happens when waiting for events that would never happen when there
is already an error condition on the bus. Also the error handling
procedure should be the same for both NACK and other bus errors in
case of dma mode. So correct that as well.

Signed-off-by: Sricharan R 
---
 drivers/i2c/busses/i2c-qup.c | 76 
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index b8116e5..44c80ae 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -310,6 +310,7 @@ static int qup_i2c_wait_ready(struct qup_i2c_dev *qup, int 
op, bool val,
u32 opflags;
u32 status;
u32 shift = __ffs(op);
+   int ret = 0;
 
len *= qup->one_byte_t;
/* timeout after a wait of twice the max time */
@@ -321,18 +322,28 @@ static int qup_i2c_wait_ready(struct qup_i2c_dev *qup, 
int op, bool val,
 
if (((opflags & op) >> shift) == val) {
if ((op == QUP_OUT_NOT_EMPTY) && qup->is_last) {
-   if (!(status & I2C_STATUS_BUS_ACTIVE))
-   return 0;
+   if (!(status & I2C_STATUS_BUS_ACTIVE)) {
+   ret = 0;
+   goto done;
+   }
} else {
-   return 0;
+   ret = 0;
+   goto done;
}
}
 
-   if (time_after(jiffies, timeout))
-   return -ETIMEDOUT;
-
+   if (time_after(jiffies, timeout)) {
+   ret = -ETIMEDOUT;
+   goto done;
+   }
usleep_range(len, len * 2);
}
+
+done:
+   if (qup->bus_err || qup->qup_err)
+   ret =  (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
+
+   return ret;
 }
 
 static void qup_i2c_set_write_mode_v2(struct qup_i2c_dev *qup,
@@ -793,39 +804,35 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
struct i2c_msg *msg,
}
 
if (ret || qup->bus_err || qup->qup_err) {
-   if (qup->bus_err & QUP_I2C_NACK_FLAG) {
-   msg--;
-   dev_err(qup->dev, "NACK from %x\n", msg->addr);
-   ret = -EIO;
+   if (qup_i2c_change_state(qup, QUP_RUN_STATE)) {
+   dev_err(qup->dev, "change to run state timed out");
+   goto desc_err;
+   }
 
-   if (qup_i2c_change_state(qup, QUP_RUN_STATE)) {
-   dev_err(qup->dev, "change to run state timed 
out");
-   return ret;
-   }
+   if (rx_nents)
+   writel(QUP_BAM_INPUT_EOT,
+  qup->base + QUP_OUT_FIFO_BASE);
 
-   if (rx_nents)
-   writel(QUP_BAM_INPUT_EOT,
-  qup->base + QUP_OUT_FIFO_BASE);
+   writel(QUP_BAM_FLUSH_STOP, qup->base + QUP_OUT_FIFO_BASE);
 
-   writel(QUP_BAM_FLUSH_STOP,
-  qup->base + QUP_OUT_FIFO_BASE);
+   qup_i2c_flush(qup);
 
-   qup_i2c_flush(qup);
+   /* wait for remaining interrupts to occur */
+   if (!wait_for_completion_timeout(&qup->xfer, HZ))
+   dev_err(qup->dev, "flush timed out\n");
 
-   /* wait for remaining interrupts to occur */
-   if (!wait_for_completion_timeout(&qup->xfer, HZ))
-   dev_err(qup->dev, "flush timed out\n");
+   qup_i2c_rel_dma(qup);
 
-   qup_i2c_rel_dma(qup);
-   }
+   ret =  (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
}
 
+desc_err:
dma_unmap_sg(qup->dev, qup->btx.sg, tx_nents, DMA_TO_DEVICE);
 
if (rx_nents)
dma_unmap_sg(qup->dev, qup->brx.sg, rx_nents,
 DMA_FROM_DEVICE);
-desc_err:
+
return ret;
 }
 
@@ -841,9 +848,6 @@ static int qup_i2c_bam_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msg,
if (ret)
goto out;
 
-   qup->bus_err = 0;
-   qup->qup_err = 0

[PATCH V4 1/3] i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled

[Sricharan R]

With CONFIG_DEBUG_SG is enabled and when dma mode is used, below dump is seen,

[ cut here ]
kernel BUG at include/linux/scatterlist.h:140!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-00459-g9f087b9-dirty #7
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
task: ffc036868000 ti: ffc03687 task.ti: ffc03687
PC is at qup_sg_set_buf.isra.13+0x138/0x154
LR is at qup_sg_set_buf.isra.13+0x50/0x154
pc : [] lr : [] pstate: 6145
sp : ffc0368735c0
x29: ffc0368735c0 x28: ffc036873752
x27: ffc035233018 x26: ffc000c4e000
x25:  x24: 0004
x23:  x22: ffc035233668
x21: ff80004e3000 x20: ffc0352e0018
x19: 0040 x18: 0028
x17: 0004 x16: ffc0017a39c8
x15: 1cdf x14: ffc0019929d8
x13: ffc0352e0018 x12: 
x11: 0001 x10: 0001
x9 : ffc0012b2d70 x8 : ff80004e3000
x7 : 0018 x6 : 3000
x5 : ffc00199f018 x4 : ffc035233018
x3 : 0004 x2 : c000
x1 : 0003 x0 : 

Process swapper/0 (pid: 1, stack limit = 0xffc036870020)
Stack: (0xffc0368735c0 to 0xffc036874000)

sg_set_bug expects that the buf parameter passed in should be from
lowmem and a valid pageframe. This is not true for pages from
dma_alloc_coherent which can be carveouts, hence the check fails.
Change allocation of sg buffers from dma_coherent memory to kzalloc
to fix the issue. Note that now dma_map/unmap is used to make the
kzalloc'ed buffers coherent before passing it to the dmaengine.

Signed-off-by: Sricharan R 
Reviewed-by: Andy Gross 
---
 drivers/i2c/busses/i2c-qup.c | 51 +---
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index cc6439ab..43adc2d 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -585,8 +585,8 @@ static void qup_i2c_bam_cb(void *data)
 }
 
 static int qup_sg_set_buf(struct scatterlist *sg, void *buf,
- struct qup_i2c_tag *tg, unsigned int buflen,
- struct qup_i2c_dev *qup, int map, int dir)
+ unsigned int buflen, struct qup_i2c_dev *qup,
+ int dir)
 {
int ret;
 
@@ -595,9 +595,6 @@ static int qup_sg_set_buf(struct scatterlist *sg, void *buf,
if (!ret)
return -EINVAL;
 
-   if (!map)
-   sg_dma_address(sg) = tg->addr + ((u8 *)buf - tg->start);
-
return 0;
 }
 
@@ -670,16 +667,15 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
struct i2c_msg *msg,
/* scratch buf to read the start and len tags */
ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
 &qup->brx.tag.start[0],
-&qup->brx.tag,
-2, qup, 0, 0);
+2, qup, DMA_FROM_DEVICE);
 
if (ret)
return ret;
 
ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
 &msg->buf[limit * i],
-NULL, tlen, qup,
-1, DMA_FROM_DEVICE);
+tlen, qup,
+DMA_FROM_DEVICE);
if (ret)
return ret;
 
@@ -688,7 +684,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
struct i2c_msg *msg,
}
ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
 &qup->start_tag.start[off],
-&qup->start_tag, len, qup, 0, 0);
+len, qup, DMA_TO_DEVICE);
if (ret)
return ret;
 
@@ -696,8 +692,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
struct i2c_msg *msg,
/* scratch buf to read the BAM EOT and FLUSH tags */
ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
 &qup->brx.tag.start[0],
-&qup->brx.tag, 2,
-qup, 0, 0);
+2, qup, DMA_F

RE: [PATCH V3 1/2] i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled

[Sricharan]

Hi,

>> sg_set_buf expects that the buf parameter passed in should be from
>> lowmem and a valid pageframe. This is not true for pages from
>> dma_alloc_coherent which can be carveouts, hence the check fails.
>
>OK, given you mean dma_pool_alloc here, the check fails for the
>pageframe because of the pool? Is my understanding correct?
>
Yes right. Since those are carveouts, there is no valid pageframe,
 so the check fails.

>> Change allocation of sg buffers from dma_coherent memory to kzalloc
>> to fix the issue.
>
>But why can you drop the coherency?
>
 The coherency is not dropped here. dma_map/unmap used
 makes the buffer coherent before passing it to
 dmaengine. Previously it was not required.
 I can add this in description if its not clear.

 >> @@ -1268,6 +1260,8 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
>>  }
>>  }
>>
>> +idx = 0;
>> +
>
>This looks like an unrelated change.
Ha, wrong. This should have been in a separate patch. This was to fix a
initialization issue in dma mode. Sorry, should not have been here, will
move it to out.

Regards,
 Sricharan

RE: [PATCH v3 2/2] i2c: qup: support SMBus block read

[Sricharan]

msg);
>+  }
>   } while (qup->blk.pos < qup->blk.count);
>
> err:
>@@ -1210,6 +1267,11 @@ static int qup_i2c_xfer(struct i2c_adapter *adap,
>   goto out;
>   }
>
>+  if (qup_i2c_check_msg_len(&msgs[idx])) {
>+  ret = -EOPNOTSUPP;
>+  goto out;
>+  }
>+
>   if (msgs[idx].flags & I2C_M_RD)
>   ret = qup_i2c_read_one(qup, &msgs[idx]);
>   else

Reviewed-by: sricha...@codeaurora.org

Regards,
 Sricharan

[PATCH V3 0/2] i2c: qup: Some misc fixes

[Sricharan R]

One for fixing the bug with CONFIG_DEBUG_SG enabled and another
to suspend the transfer for all errors instead of just for NACK.

[V3] Added more commit description. Return more appropriate
 error codes for NACK and other bus errors. Corrected
 other bus errors handling procedure for dma mode as well.
 Removed the dev_err log for NACKs.

[V2] Removed the use of unnecessary variable assignment.

Kept the reviewed and Tested by tag for patch#1,
as there was no code change.

Depends on patch[1] for the error handling to be complete.

[1] https://lkml.org/lkml/2016/5/9/447

Sricharan R (2):
  i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled
  i2c: qup: Fix error handling

 drivers/i2c/busses/i2c-qup.c | 129 +++
 1 file changed, 58 insertions(+), 71 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH v3 1/2] i2c: qup: add ACPI support

[Sricharan]

lt; 8) | (fs_div & 0xff);
>@@ -1639,6 +1658,14 @@ static const struct of_device_id qup_i2c_dt_match[] = {
> };
> MODULE_DEVICE_TABLE(of, qup_i2c_dt_match);
>
>+#if IS_ENABLED(CONFIG_ACPI)
>+static const struct acpi_device_id qup_i2c_acpi_match[] = {
>+  { "QCOM8010"},
>+  { },
>+};
>+MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_ids);
>+#endif
>+
> static struct platform_driver qup_i2c_driver = {
>   .probe  = qup_i2c_probe,
>   .remove = qup_i2c_remove,
>@@ -1646,6 +1673,7 @@ static struct platform_driver qup_i2c_driver = {
>   .name = "i2c_qup",
>   .pm = &qup_i2c_qup_pm_ops,
>   .of_match_table = qup_i2c_dt_match,
>+  .acpi_match_table = ACPI_PTR(qup_i2c_acpi_match),
>   },
> };

Reviewed-by: sricha...@codeaurora.org

Regards,
 Sricharan

[PATCH V3 1/2] i2c: qup: Fix broken dma when CONFIG_DEBUG_SG is enabled

[Sricharan R]

With CONFIG_DEBUG_SG is enabled and when dma mode is used, below dump is seen,

[ cut here ]
kernel BUG at include/linux/scatterlist.h:140!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-00459-g9f087b9-dirty #7
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
task: ffc036868000 ti: ffc03687 task.ti: ffc03687
PC is at qup_sg_set_buf.isra.13+0x138/0x154
LR is at qup_sg_set_buf.isra.13+0x50/0x154
pc : [] lr : [] pstate: 6145
sp : ffc0368735c0
x29: ffc0368735c0 x28: ffc036873752
x27: ffc035233018 x26: ffc000c4e000
x25:  x24: 0004
x23:  x22: ffc035233668
x21: ff80004e3000 x20: ffc0352e0018
x19: 0040 x18: 0028
x17: 0004 x16: ffc0017a39c8
x15: 1cdf x14: ffc0019929d8
x13: ffc0352e0018 x12: 
x11: 0001 x10: 0001
x9 : ffc0012b2d70 x8 : ff80004e3000
x7 : 0018 x6 : 3000
x5 : ffc00199f018 x4 : ffc035233018
x3 : 0004 x2 : c000
x1 : 0003 x0 : 

Process swapper/0 (pid: 1, stack limit = 0xffc036870020)
Stack: (0xffc0368735c0 to 0xffc036874000)

sg_set_buf expects that the buf parameter passed in should be from
lowmem and a valid pageframe. This is not true for pages from
dma_alloc_coherent which can be carveouts, hence the check fails.
Change allocation of sg buffers from dma_coherent memory to kzalloc
to fix the issue.

Signed-off-by: Sricharan R 
Reviewed-by: Andy Gross 
Tested-by: Naveen Kaje 
---
[V3] Added more commit description.

 drivers/i2c/busses/i2c-qup.c | 53 ++--
 1 file changed, 17 insertions(+), 36 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 23eaabb..8620e99 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -585,8 +585,8 @@ static void qup_i2c_bam_cb(void *data)
 }
 
 static int qup_sg_set_buf(struct scatterlist *sg, void *buf,
- struct qup_i2c_tag *tg, unsigned int buflen,
- struct qup_i2c_dev *qup, int map, int dir)
+ unsigned int buflen, struct qup_i2c_dev *qup,
+ int dir)
 {
int ret;
 
@@ -595,9 +595,6 @@ static int qup_sg_set_buf(struct scatterlist *sg, void *buf,
if (!ret)
return -EINVAL;
 
-   if (!map)
-   sg_dma_address(sg) = tg->addr + ((u8 *)buf - tg->start);
-
return 0;
 }
 
@@ -670,16 +667,15 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
struct i2c_msg *msg,
/* scratch buf to read the start and len tags */
ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
 &qup->brx.tag.start[0],
-&qup->brx.tag,
-2, qup, 0, 0);
+2, qup, DMA_FROM_DEVICE);
 
if (ret)
return ret;
 
ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
 &msg->buf[limit * i],
-NULL, tlen, qup,
-1, DMA_FROM_DEVICE);
+tlen, qup,
+DMA_FROM_DEVICE);
if (ret)
return ret;
 
@@ -688,7 +684,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
struct i2c_msg *msg,
}
ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
 &qup->start_tag.start[off],
-&qup->start_tag, len, qup, 0, 0);
+len, qup, DMA_TO_DEVICE);
if (ret)
return ret;
 
@@ -696,8 +692,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
struct i2c_msg *msg,
/* scratch buf to read the BAM EOT and FLUSH tags */
ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
 &qup->brx.tag.start[0],
-&qup->brx.tag, 2,
-qup, 0, 0);
+2, qup, DMA_FROM_DEVICE);
if (ret)

[PATCH V3 2/2] i2c: qup: Fix error handling

[Sricharan R]

Among the bus errors reported from the QUP_MASTER_STATUS register
only NACK is considered and transfer gets suspended, while
other errors are ignored. Correct this and suspend the transfer
for other errors as well. This avoids unnecessary 'timeouts' which
happens when waiting for events that would never happen when there
is already an error condition on the bus. Also the error handling
procedure should be the same for both NACK and other bus errors in
case of dma mode. So correct that as well.

Signed-off-by: Sricharan R 
---
[V3] Change the return error code for NACK and other bus errors.
 Added the error handling procedure for other bus errors in
 dma mode. Removed the dev_err print in case of NACK.

 drivers/i2c/busses/i2c-qup.c | 76 
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 8620e99..edced86 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -310,6 +310,7 @@ static int qup_i2c_wait_ready(struct qup_i2c_dev *qup, int 
op, bool val,
u32 opflags;
u32 status;
u32 shift = __ffs(op);
+   int ret = 0;
 
len *= qup->one_byte_t;
/* timeout after a wait of twice the max time */
@@ -321,18 +322,28 @@ static int qup_i2c_wait_ready(struct qup_i2c_dev *qup, 
int op, bool val,
 
if (((opflags & op) >> shift) == val) {
if ((op == QUP_OUT_NOT_EMPTY) && qup->is_last) {
-   if (!(status & I2C_STATUS_BUS_ACTIVE))
-   return 0;
+   if (!(status & I2C_STATUS_BUS_ACTIVE)) {
+   ret = 0;
+   goto done;
+   }
} else {
-   return 0;
+   ret = 0;
+   goto done;
}
}
 
-   if (time_after(jiffies, timeout))
-   return -ETIMEDOUT;
-
+   if (time_after(jiffies, timeout)) {
+   ret = -ETIMEDOUT;
+   goto done;
+   }
usleep_range(len, len * 2);
}
+
+done:
+   if (qup->bus_err || qup->qup_err)
+   ret =  (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
+
+   return ret;
 }
 
 static void qup_i2c_set_write_mode_v2(struct qup_i2c_dev *qup,
@@ -793,39 +804,35 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, 
struct i2c_msg *msg,
}
 
if (ret || qup->bus_err || qup->qup_err) {
-   if (qup->bus_err & QUP_I2C_NACK_FLAG) {
-   msg--;
-   dev_err(qup->dev, "NACK from %x\n", msg->addr);
-   ret = -EIO;
+   if (qup_i2c_change_state(qup, QUP_RUN_STATE)) {
+   dev_err(qup->dev, "change to run state timed out");
+   goto desc_err;
+   }
 
-   if (qup_i2c_change_state(qup, QUP_RUN_STATE)) {
-   dev_err(qup->dev, "change to run state timed 
out");
-   return ret;
-   }
+   if (rx_nents)
+   writel(QUP_BAM_INPUT_EOT,
+  qup->base + QUP_OUT_FIFO_BASE);
 
-   if (rx_nents)
-   writel(QUP_BAM_INPUT_EOT,
-  qup->base + QUP_OUT_FIFO_BASE);
+   writel(QUP_BAM_FLUSH_STOP, qup->base + QUP_OUT_FIFO_BASE);
 
-   writel(QUP_BAM_FLUSH_STOP,
-  qup->base + QUP_OUT_FIFO_BASE);
+   qup_i2c_flush(qup);
 
-   qup_i2c_flush(qup);
+   /* wait for remaining interrupts to occur */
+   if (!wait_for_completion_timeout(&qup->xfer, HZ))
+   dev_err(qup->dev, "flush timed out\n");
 
-   /* wait for remaining interrupts to occur */
-   if (!wait_for_completion_timeout(&qup->xfer, HZ))
-   dev_err(qup->dev, "flush timed out\n");
+   qup_i2c_rel_dma(qup);
 
-   qup_i2c_rel_dma(qup);
-   }
+   ret =  (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
}
 
+desc_err:
dma_unmap_sg(qup->dev, qup->btx.sg, tx_nents, DMA_TO_DEVICE);
 
if (rx_nents)
dma_unmap_sg(qup->dev, qup->brx.sg, rx_nents,
 DMA_FROM_DEVICE);
-desc_err:
+
return ret;
 }
 
@@ -841,9 

RE: [PATCH v2 2/2] i2c: qup: support SMBus block read

[Sricharan]

Hi,


>>> @@ -1128,6 +1173,22 @@ static int qup_i2c_read_one_v2(struct
>>> qup_i2c_dev *qup, struct i2c_msg *msg)
>>> goto err;
>>>
>>> qup->blk.pos++;
>>> +
>>> +   /* Handle SMBus block read length */
>>> +   if (qup_i2c_check_msg_len(msg) && (msg->len == 1)) {
>>> +   if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) {
>>> +   ret = -EPROTO;
>>> +   goto err;
>>> +   }
>>> +   msg->len += msg->buf[0];
>>> +   qup->pos = 0;
>>> +   qup_i2c_set_read_mode_v2(qup, msg->len);
>>> +   qup_i2c_issue_xfer_v2(qup, msg);
>>> +   ret = qup_i2c_wait_for_complete(qup, msg);
>>> +   if (ret)
>>> +   goto err;
>>  Is the issue_xfer_v2 needed inside this here ?
>No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data
>that is indicated by the length we read earlier.

So qup_i2c_issue_xfer_v2 writes the tags and there is one already in the loop 
above this
 check. So if you just do qup_i2c_set_read_mode_v2 and qup_i2c_set_blk_data 
inside this
 check, will not be enough ?

Regards,
 Sricharan