RE: [PATCH 2/2] iommu: add qcom_iommu
Hi Rob,
<..>
>>>+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>>>@@ -0,0 +1,45 @@
>>>+* QCOM IOMMU Implementation
>>>+
>>>+Qualcomm "B" family devices which are not compatible with arm-smmu have
>>>+a similar looking IOMMU but without access to the global register space.
>>>+This is modelled as separate IOMMU devices which have just a single
>>>+master.
>>>+
>>>+** Required properties:
>>>+
>>>+- compatible: Should be one of:
>>>+
>>>+"qcom,msm8916-iommu-context-bank"
>>>+
>>>+ depending on the particular implementation and/or the
>>>+ version of the architecture implemented.
>>>+
>>>+- reg : Base address and size of the SMMU. And optionally,
>>>+ if present, the "smmu_local_base"
>>>+
>>>+- interrupts: The context fault irq.
>>>+
>>>+- #iommu-cells : Must be 0
>>>+
>>>+- qcom,iommu-ctx-asid : context ASID
>>>+
>>>+- qcom,iommu-secure-id : secure-id
>>>+
>>>+- clocks: The interface clock (iface_clk) and bus clock (bus_clk)
>>>+
>>>+** Examples:
>>>+
>>>+ mdp_iommu: iommu-context-bank@1e24000 {
>>>+ compatible = "qcom,msm8916-iommu-context-bank";
>>>+ reg = <0x1e24000 0x1000
>>>+ 0x1ef 0x3000>;
>>>+ reg-names = "iommu_base", "smmu_local_base";
>>>+ interrupts = ;
>>>+ qcom,iommu-ctx-asid = <4>;
>>>+ qcom,iommu-secure-id = <17>;
>>
>> This is not an per context bank property and can be programmed for an
>> given iommu only once. So we call qcom_iommu_sec_init for
>> each context bank once, which does not look correct. Similarly for
>> smmu_local_base as well. So should this be handled using an global
>> once for all contexts ?
>
>yeah, smmu_local_base and secure-id would be duplicate for all context
>banks that are part of the same actual iommu. (But it was Robin's
>suggestion to just model this as separate context-bank devices, since
>we cannot touch the global space).
>
>Did I misunderstand the downstream driver code? It looked like
>qcom_scm_restore_sec_cfg() was called once on first attach per
>context-bank, not globally for the entire iommu, which is what I'm
>doing with this driver. But I haven't yet tried to enable other
>context-banks in the apps iommu yet.
>
The downstream driver seems to be calling the sec_cfg once
for an iommu when a context is attached for the first time and not for
the subsequent's contexts that are attached. So, means programmed
only once and not for every context. I see it that way. Anyways when you
add more than context-banks, we can see if that causes trouble..
Regards,
Sricharan
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] iommu: add qcom_iommu
On Wed, Feb 22, 2017 at 4:31 AM, Sricharan wrote:
> Hi Rob,
>
>>diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>>b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>>new file mode 100644
>>index 000..78a8d65
>>--- /dev/null
>>+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>>@@ -0,0 +1,45 @@
>>+* QCOM IOMMU Implementation
>>+
>>+Qualcomm "B" family devices which are not compatible with arm-smmu have
>>+a similar looking IOMMU but without access to the global register space.
>>+This is modelled as separate IOMMU devices which have just a single
>>+master.
>>+
>>+** Required properties:
>>+
>>+- compatible: Should be one of:
>>+
>>+"qcom,msm8916-iommu-context-bank"
>>+
>>+ depending on the particular implementation and/or the
>>+ version of the architecture implemented.
>>+
>>+- reg : Base address and size of the SMMU. And optionally,
>>+ if present, the "smmu_local_base"
>>+
>>+- interrupts: The context fault irq.
>>+
>>+- #iommu-cells : Must be 0
>>+
>>+- qcom,iommu-ctx-asid : context ASID
>>+
>>+- qcom,iommu-secure-id : secure-id
>>+
>>+- clocks: The interface clock (iface_clk) and bus clock (bus_clk)
>>+
>>+** Examples:
>>+
>>+ mdp_iommu: iommu-context-bank@1e24000 {
>>+ compatible = "qcom,msm8916-iommu-context-bank";
>>+ reg = <0x1e24000 0x1000
>>+ 0x1ef 0x3000>;
>>+ reg-names = "iommu_base", "smmu_local_base";
>>+ interrupts = ;
>>+ qcom,iommu-ctx-asid = <4>;
>>+ qcom,iommu-secure-id = <17>;
>
> This is not an per context bank property and can be programmed for an
> given iommu only once. So we call qcom_iommu_sec_init for
> each context bank once, which does not look correct. Similarly for
> smmu_local_base as well. So should this be handled using an global
> once for all contexts ?
yeah, smmu_local_base and secure-id would be duplicate for all context
banks that are part of the same actual iommu. (But it was Robin's
suggestion to just model this as separate context-bank devices, since
we cannot touch the global space).
Did I misunderstand the downstream driver code? It looked like
qcom_scm_restore_sec_cfg() was called once on first attach per
context-bank, not globally for the entire iommu, which is what I'm
doing with this driver. But I haven't yet tried to enable other
context-banks in the apps iommu yet.
>>+ #iommu-cells = <0>;
>>+ clocks = <&gcc GCC_SMMU_CFG_CLK>,
>>+ <&gcc GCC_APSS_TCU_CLK>;
>>+ clock-names = "iface_clk", "bus_clk";
>
> I am trying to generalize the clock bindings for MMU-500 and one more
> qcom specific. Anyways this can follow that.
no problem to adapt to what you come up with for arm-smmu, it is
basically the same requirements.
>>+ status = "okay";
>
> <..>
>
>>+#define pr_fmt(fmt) "qcom-iommu: " fmt
>>+
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+#include
>>+
>>+#include "io-pgtable.h"
>>+#include "arm-smmu-regs.h"
>>+
>>+// TODO are these qcom specific, or just something no one bothered to add to
>>arm-smmu
>>+#define SMMU_CB_TLBSYNC 0x7f0
>>+#define SMMU_CB_TLBSTATUS0x7f4
>
> I think the reason was in arm-smmu, we are using the global TLBSYNC/STATUS
> bits, as its
> used in both global device reset and flush path. Otherwise here, its correct
> to add this.
ok, that is what I suspected.. in next version I'll add these two to
the shared header instead
>>+#define SMMU_INTR_SEL_NS 0x2000
>>+
>>+
>>+struct qcom_iommu_device {
>>+ struct device *dev;
>>+
>>+ void __iomem*base;
>>+ void __iomem*local_base;
>>+ unsigned int irq;
>>+ struct clk *iface_clk;
>>+ struct clk *bus_clk;
>>+
>>+ bool secure_init;
>>+ u32 asid; /* asid and ctx bank # are 1:1 */
>>+ u32 sec_id;
>>+
>>+ /* single group per device: */
>>+ struct iommu_group *group;
>>+};
>>+
>>+struct qcom_iommu_domain {
>>+ struct qcom_iommu_device*iommu;
>>+ struct io_pgtable_ops *pgtbl_ops;
>>+ spinlock_t pgtbl_lock;
>>+ struct mutex init_mutex; /* Protects iommu pointer
>>*/
>>+ struct iommu_domain domain;
>>+};
>>+
>>+static struct qcom_iommu_domain *to_qcom_iommu_domain(struct iommu_domain
>>*dom)
>>+{
>>+ return container_of(dom, struct qcom_iommu_domain, domain);
>>+}
>>+
>>+static const struct iommu_ops qcom_iommu_ops;
>>+static struct platform_driver
RE: [PATCH 2/2] iommu: add qcom_iommu
Hi Rob,
>diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>new file mode 100644
>index 000..78a8d65
>--- /dev/null
>+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>@@ -0,0 +1,45 @@
>+* QCOM IOMMU Implementation
>+
>+Qualcomm "B" family devices which are not compatible with arm-smmu have
>+a similar looking IOMMU but without access to the global register space.
>+This is modelled as separate IOMMU devices which have just a single
>+master.
>+
>+** Required properties:
>+
>+- compatible: Should be one of:
>+
>+"qcom,msm8916-iommu-context-bank"
>+
>+ depending on the particular implementation and/or the
>+ version of the architecture implemented.
>+
>+- reg : Base address and size of the SMMU. And optionally,
>+ if present, the "smmu_local_base"
>+
>+- interrupts: The context fault irq.
>+
>+- #iommu-cells : Must be 0
>+
>+- qcom,iommu-ctx-asid : context ASID
>+
>+- qcom,iommu-secure-id : secure-id
>+
>+- clocks: The interface clock (iface_clk) and bus clock (bus_clk)
>+
>+** Examples:
>+
>+ mdp_iommu: iommu-context-bank@1e24000 {
>+ compatible = "qcom,msm8916-iommu-context-bank";
>+ reg = <0x1e24000 0x1000
>+ 0x1ef 0x3000>;
>+ reg-names = "iommu_base", "smmu_local_base";
>+ interrupts = ;
>+ qcom,iommu-ctx-asid = <4>;
>+ qcom,iommu-secure-id = <17>;
This is not an per context bank property and can be programmed for an
given iommu only once. So we call qcom_iommu_sec_init for
each context bank once, which does not look correct. Similarly for
smmu_local_base as well. So should this be handled using an global
once for all contexts ?
>+ #iommu-cells = <0>;
>+ clocks = <&gcc GCC_SMMU_CFG_CLK>,
>+ <&gcc GCC_APSS_TCU_CLK>;
>+ clock-names = "iface_clk", "bus_clk";
I am trying to generalize the clock bindings for MMU-500 and one more
qcom specific. Anyways this can follow that.
>+ status = "okay";
<..>
>+#define pr_fmt(fmt) "qcom-iommu: " fmt
>+
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+
>+#include "io-pgtable.h"
>+#include "arm-smmu-regs.h"
>+
>+// TODO are these qcom specific, or just something no one bothered to add to
>arm-smmu
>+#define SMMU_CB_TLBSYNC 0x7f0
>+#define SMMU_CB_TLBSTATUS0x7f4
I think the reason was in arm-smmu, we are using the global TLBSYNC/STATUS
bits, as its
used in both global device reset and flush path. Otherwise here, its correct to
add this.
>+#define SMMU_INTR_SEL_NS 0x2000
>+
>+
>+struct qcom_iommu_device {
>+ struct device *dev;
>+
>+ void __iomem*base;
>+ void __iomem*local_base;
>+ unsigned int irq;
>+ struct clk *iface_clk;
>+ struct clk *bus_clk;
>+
>+ bool secure_init;
>+ u32 asid; /* asid and ctx bank # are 1:1 */
>+ u32 sec_id;
>+
>+ /* single group per device: */
>+ struct iommu_group *group;
>+};
>+
>+struct qcom_iommu_domain {
>+ struct qcom_iommu_device*iommu;
>+ struct io_pgtable_ops *pgtbl_ops;
>+ spinlock_t pgtbl_lock;
>+ struct mutex init_mutex; /* Protects iommu pointer
>*/
>+ struct iommu_domain domain;
>+};
>+
>+static struct qcom_iommu_domain *to_qcom_iommu_domain(struct iommu_domain
>*dom)
>+{
>+ return container_of(dom, struct qcom_iommu_domain, domain);
>+}
>+
>+static const struct iommu_ops qcom_iommu_ops;
>+static struct platform_driver qcom_iommu_driver;
>+
>+static struct qcom_iommu_device * dev_to_iommu(struct device *dev)
>+{
>+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>+ if (WARN_ON(!fwspec || fwspec->ops != &qcom_iommu_ops))
>+ return NULL;
>+ return fwspec->iommu_priv;
>+}
>+
>+static inline void
>+iommu_writel(struct qcom_iommu_device *qcom_iommu, unsigned reg, u32 val)
>+{
>+ writel_relaxed(val, qcom_iommu->base + reg);
>+}
>+
>+static inline void
>+iommu_writeq(struct qcom_iommu_device *qcom_iommu, unsigned reg, u64 val)
>+{
>+ writeq_relaxed(val, qcom_iommu->base + reg);
>+}
>+
>+static inline u32
>+iommu_readl(struct qcom_iommu_device *qcom_iommu, unsigned reg)
>+{
>+ return readl_relaxed(qcom_iommu->base + reg);
>+}
>+
>+static inline u32
>+iommu_readq(struct qcom_iommu_device *qcom_iommu, unsigned reg)
>+{
>+ return readq_relaxed(qcom_iommu->base + reg);
>+}
>+
>+static void __sync_tlb(s
[PATCH 2/2] iommu: add qcom_iommu
An iommu driver for Qualcomm "B" family devices which do not completely
implement the ARM SMMU spec. These devices have context-bank register
layout that is similar to ARM SMMU, but no global register space (or at
least not one that is accessible).
Signed-off-by: Rob Clark
---
Feel free to bikeshed the name, I just had to pick something. Maybe
we should go back to what downstream calls it (ie. msm-iommu-v1)
Also unsure about the compatible string. Possibly it should be more
generic, since 8x74 and 8x84 and probably a bunch of others want to
use this same driver. Although I'm not entirely sure whether they
use the same pagetable format configuration, so we might just want
to keep the SoC name in the compat string for making those sorts of
decisions.
.../devicetree/bindings/iommu/qcom,iommu.txt | 45 ++
drivers/iommu/Kconfig | 10 +
drivers/iommu/Makefile | 1 +
drivers/iommu/qcom_iommu.c | 699 +
4 files changed, 755 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iommu/qcom,iommu.txt
create mode 100644 drivers/iommu/qcom_iommu.c
diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
new file mode 100644
index 000..78a8d65
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
@@ -0,0 +1,45 @@
+* QCOM IOMMU Implementation
+
+Qualcomm "B" family devices which are not compatible with arm-smmu have
+a similar looking IOMMU but without access to the global register space.
+This is modelled as separate IOMMU devices which have just a single
+master.
+
+** Required properties:
+
+- compatible: Should be one of:
+
+"qcom,msm8916-iommu-context-bank"
+
+ depending on the particular implementation and/or the
+ version of the architecture implemented.
+
+- reg : Base address and size of the SMMU. And optionally,
+ if present, the "smmu_local_base"
+
+- interrupts: The context fault irq.
+
+- #iommu-cells : Must be 0
+
+- qcom,iommu-ctx-asid : context ASID
+
+- qcom,iommu-secure-id : secure-id
+
+- clocks: The interface clock (iface_clk) and bus clock (bus_clk)
+
+** Examples:
+
+ mdp_iommu: iommu-context-bank@1e24000 {
+ compatible = "qcom,msm8916-iommu-context-bank";
+ reg = <0x1e24000 0x1000
+ 0x1ef 0x3000>;
+ reg-names = "iommu_base", "smmu_local_base";
+ interrupts = ;
+ qcom,iommu-ctx-asid = <4>;
+ qcom,iommu-secure-id = <17>;
+ #iommu-cells = <0>;
+ clocks = <&gcc GCC_SMMU_CFG_CLK>,
+<&gcc GCC_APSS_TCU_CLK>;
+ clock-names = "iface_clk", "bus_clk";
+ status = "okay";
+ };
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 8ee54d7..631e1cd 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -362,4 +362,14 @@ config MTK_IOMMU_V1
if unsure, say N here.
+config QCOM_IOMMU
+ bool "Qualcomm IOMMU Support"
+ depends on ARM || ARM64
+ depends on ARCH_QCOM || COMPILE_TEST
+ select IOMMU_API
+ select IOMMU_IO_PGTABLE_LPAE
+ select ARM_DMA_USE_IOMMU
+ help
+ Support for IOMMU on certain Qualcomm SoCs.
+
endif # IOMMU_SUPPORT
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 195f7b9..b910aea 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -27,3 +27,4 @@ obj-$(CONFIG_TEGRA_IOMMU_SMMU) += tegra-smmu.o
obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
+obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
new file mode 100644
index 000..eb92d60
--- /dev/null
+++ b/drivers/iommu/qcom_iommu.c
@@ -0,0 +1,699 @@
+/*
+ * IOMMU API for QCOM secure IOMMUs. Somewhat based on arm-smmu.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * Copyright (C) 2013 ARM Limited
+ * Copyright (C) 2017 Red Hat
+ */
+
+#define pr_fmt(fmt) "qcom-iommu: " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#
