Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver
Dear Mitchel,
Thanks very much for your review.
On Fri, 2015-03-06 at 09:15 -0800, Mitchel Humpherys wrote:
On Fri, Mar 06 2015 at 02:48:17 AM, yong...@mediatek.com wrote:
From: Yong Wu yong...@mediatek.com
This patch adds support for mediatek m4u (MultiMedia Memory Management
Unit).
Currently this only supports m4u gen 2 with 2 levels of page table on
mt8173.
[...]
+static int mtk_iommu_invalidate_tlb(const struct mtk_iommu_info *piommu,
+ int isinvall, unsigned int iova_start,
+ unsigned int iova_end)
+{
+ void __iomem *m4u_base = piommu-m4u_base;
+ u32 val;
+ u64 start, end;
+
+ start = sched_clock();
+
+ if (!isinvall) {
+ iova_start = round_down(iova_start, SZ_4K);
+ iova_end = round_up(iova_end, SZ_4K);
+ }
+
+ val = F_MMU_INV_EN_L2 | F_MMU_INV_EN_L1;
+
+ writel(val, m4u_base + REG_INVLID_SEL);
+
+ if (isinvall) {
+ writel(F_MMU_INV_ALL, m4u_base + REG_MMU_INVLD);
+ } else {
+ writel(iova_start, m4u_base + REG_MMU_INVLD_SA);
+ writel(iova_end, m4u_base + REG_MMU_INVLD_EA);
+ writel(F_MMU_INV_RANGE, m4u_base + REG_MMU_INVLD);
+
+ while (!readl(m4u_base + REG_MMU_CPE_DONE)) {
+ end = sched_clock();
+ if (end - start = 1ULL) {
+ dev_warn(piommu-dev, invalid don't done\n);
+ writel(F_MMU_INV_ALL, m4u_base + REG_MMU_INVLD);
+ }
+ };
Superfluous `;'. Also, maybe you should be using readl_poll_timeout?
Thanks.
For the readl_poll_timeout, My base is 3.19-rc7 and robin's patch.
it don't have this interface. I will try to add it in the next version.
+ writel(0, m4u_base + REG_MMU_CPE_DONE);
+ }
+
+ return 0;
+}
-Mitch
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver
Hi,
You can find part 2 of my comments inline.
On Fri, Mar 6, 2015 at 7:48 PM, yong...@mediatek.com wrote:
[snip]
+static irqreturn_t mtk_iommu_isr(int irq, void *dev_id)
+{
+ struct iommu_domain *domain = dev_id;
+ struct mtk_iommu_domain *mtkdomain = domain-priv;
+ struct mtk_iommu_info *piommu = mtkdomain-piommuinfo;
+
+ if (irq == piommu-irq)
+ report_iommu_fault(domain, piommu-dev, 0, 0);
In addition to my previous comment about how this gets called from
this handler, you need to keep in mind that the function called by
report_iommu_fault() might actually be a different function than
mtk_iommu_fault_handler(), because upper layers can provide their own
handlers. This means that you need to perform any operations on
hardware from this handler and only use the iommu fault handler as a
way to tell an upper layer about the fault (including notifying the
user through kernel log if there is no special handler installed and
the generic fallback is used).
+ else
+ dev_err(piommu-dev, irq number:%d\n, irq);
+
+ return IRQ_HANDLED;
+}
[snip]
+static int mtk_iommu_fault_handler(struct iommu_domain *imudomain,
+ struct device *dev, unsigned long iova,
+ int m4uindex, void *pimu)
+{
+ void __iomem *m4u_base;
+ u32 int_state, regval;
+ int m4u_slave_id = 0;
+ unsigned int layer, write, m4u_port;
+ unsigned int fault_mva, fault_pa;
+ struct mtk_iommu_info *piommu = pimu;
+ struct mtk_iommu_domain *mtkdomain = imudomain-priv;
+
+ m4u_base = piommu-m4u_base;
+ int_state = readl(m4u_base + REG_MMU_MAIN_FAULT_ST);
+
+ /* read error info from registers */
+ fault_mva = readl(m4u_base + REG_MMU_FAULT_VA(m4u_slave_id));
+ layer = !!(fault_mva F_MMU_FAULT_VA_LAYER_BIT);
+ write = !!(fault_mva F_MMU_FAULT_VA_WRITE_BIT);
+ fault_mva = F_MMU_FAULT_VA_MSK;
+ fault_pa = readl(m4u_base + REG_MMU_INVLD_PA(m4u_slave_id));
+ regval = readl(m4u_base + REG_MMU_INT_ID(m4u_slave_id));
+ regval = F_MMU0_INT_ID_TF_MSK;
+ m4u_port = mtk_iommu_get_port_by_tfid(piommu, regval);
+
+ if (int_state F_INT_TRANSLATION_FAULT(m4u_slave_id)) {
+ struct m4u_pte_info_t pte;
+ unsigned long flags;
+
+ spin_lock_irqsave(mtkdomain-pgtlock, flags);
+ m4u_get_pte_info(mtkdomain, fault_mva, pte);
+ spin_unlock_irqrestore(mtkdomain-pgtlock, flags);
+
+ if (pte.size == MMU_SMALL_PAGE_SIZE ||
+ pte.size == MMU_LARGE_PAGE_SIZE) {
+ dev_err_ratelimited(
+ dev,
+ fault:port=%s iova=0x%x pa=0x%x layer=%d %s;
+
pgd(0x%x)-pte(0x%x)-pa(%pad)sz(0x%x)Valid(%d)\n,
+ mtk_iommu_get_port_name(piommu, m4u_port),
+ fault_mva, fault_pa, layer,
+ write ? write : read,
+ imu_pgd_val(*pte.pgd), imu_pte_val(*pte.pte),
+ pte.pa, pte.size, pte.valid);
+ } else {
+ dev_err_ratelimited(
+ dev,
+ fault:port=%s iova=0x%x pa=0x%x layer=%d %s;
+ pgd(0x%x)-pa(%pad)sz(0x%x)Valid(%d)\n,
+ mtk_iommu_get_port_name(piommu, m4u_port),
+ fault_mva, fault_pa, layer,
+ write ? write : read,
+ imu_pgd_val(*pte.pgd),
+ pte.pa, pte.size, pte.valid);
+ }
+ }
+
+ if (int_state F_INT_MAIN_MULTI_HIT_FAULT(m4u_slave_id))
+ dev_err_ratelimited(dev, multi-hit!port=%s iova=0x%x\n,
+ mtk_iommu_get_port_name(piommu, m4u_port),
+ fault_mva);
+
+ if (int_state F_INT_INVALID_PA_FAULT(m4u_slave_id)) {
+ if (!(int_state F_INT_TRANSLATION_FAULT(m4u_slave_id)))
+ dev_err_ratelimited(dev, invalid pa!port=%s
iova=0x%x\n,
+ mtk_iommu_get_port_name(piommu,
+ m4u_port),
+ fault_mva);
+ }
+ if (int_state F_INT_ENTRY_REPLACEMENT_FAULT(m4u_slave_id))
+ dev_err_ratelimited(dev, replace-fault!port=%s iova=0x%x\n,
+ mtk_iommu_get_port_name(piommu, m4u_port),
+ fault_mva);
+
+ if (int_state F_INT_TLB_MISS_FAULT(m4u_slave_id))
+
Re: [PATCH 4/5] dt-bindings: iommu: Add binding for mediatek IOMMU
Dear Mark,
Thanks very much for your review.
I will fix them in the next verion.
On Fri, 2015-03-06 at 11:21 +, Mark Rutland wrote:
On Fri, Mar 06, 2015 at 10:48:19AM +, yong...@mediatek.com wrote:
From: Yong Wu yong...@mediatek.com
This patch add mediatek iommu dts binding document.
Signed-off-by: Yong Wu yong...@mediatek.com
---
.../devicetree/bindings/iommu/mediatek,iommu.txt | 41
++
1 file changed, 41 insertions(+)
create mode 100644
Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
new file mode 100644
index 000..db01c92
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
@@ -0,0 +1,41 @@
+/**/
+/*Mediatek IOMMU Hardware block diagram */
+/**/
+ EMI (External Memory Interface)
+ |
+ m4u (Multimedia Memory Management Unit)
+ |
+ smi (Smart Multimedia Interface)
+ |
++---+---
+| |
+| |
+vdec larb disp larb ... SoCs have different local
arbiter(larb).
+| |
+| |
+ ++++-+-+
+ |||| | |...
+ |||| | |...
+ |||| | |...
+ MC PP VLD OVL0 RDMA0 WDMA0 ... There are different ports in each
larb.
+
+Required properties:
+- compatible : must be mediatek,mt8173-iommu
s/iommu/m4u/ -- the name should match what the hardware is called.
+- reg : m4u register base
... and size
+- interrupts : must contain the interrupts from each internal translation
unit
How many internal translation units are there?
How should the interrupts be ordered?
How do these relate to the larbs?
There is only one internal translation units in MT8173.(2 units in
MT8135)
Because this patch is only for mt8173, so I will change it like this:
interrupts : the interrupt of the m4u.
is it ok?
+- clocks : must contain one entry for each clock-name
+- clock-name: m4u clock
This does not match the example.
s/clock-name/clock-names/
Please format this like a list, with names quoted, e.g.
- clock-names: must contain:
* m4u - The functional clock for the m4u
+- larb : must contain the larbes of current platform
s/larbes/local arbiters/
How should these be ordered? Surely there's a relationship with
registers/interrupts/etc in this unit?
+- iommu-cells : must be 1. Specifies the client PortID as defined in
+dt-binding/iommu/mt**-iommu-port.h
Give the absolute filename.
The include file should be added in this patch (it's part of the
binding). The whole patch should be moved earlier in the series.
Thanks,
Mark.
+
+Example:
+ iommu: mmsys_iommu@10205000 {
+ compatible = mediatek,mt8173-iommu;
+ reg = 0 0x10205000 0 0x1000;
+ interrupts = GIC_SPI 139 IRQ_TYPE_LEVEL_LOW;
+ clocks = infrasys INFRA_M4U;
+ clock-names = infra_m4u;
+ larb = larb0 larb1 larb2 larb3 larb4 larb5;
+ #iommu-cells = 1;
+ };
\ No newline at end of file
--
1.8.1.1.dirty
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver
On Mon, Mar 9, 2015 at 11:46 PM, Yingjoe Chen yingjoe.c...@mediatek.com wrote: On Mon, 2015-03-09 at 20:11 +0900, Tomasz Figa wrote: ... +/* + * pimudev is a global var for dma_alloc_coherent. + * It is not accepatable, we will delete it if domain_alloc is enabled + */ +static struct device *pimudev; This is indeed not acceptable. Could you replace dma_alloc_coherent() with something that doesn't require device pointer, e.g. alloc_pages()? (Although that would require you to handle cache maintenance in the driver, due to cached memory allocated.) I need to think about a better solution for this. Hi, For 2nd level page table, we use cached memory now. Currently we are using __dma_flush_range to flush the cache, which is also unacceptable. For proper cache management, we'll need to use dma_map_single or dma_sync_*, which still need a deivce*. Looking at how already mainlined drivers do this, they either use dmac_flush_range() (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/msm_iommu.c?id=refs/tags/v4.0-rc3#n80) or directly __cpuc_flush_dcache_area() and outer_flush_range() (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/rockchip-iommu.c?id=refs/tags/v4.0-rc3#n93). Joerg, what's your opinion on this? Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver
On Tue, 2015-03-10 at 02:00 +0900, Tomasz Figa wrote: On Mon, Mar 9, 2015 at 11:46 PM, Yingjoe Chen yingjoe.c...@mediatek.com wrote: On Mon, 2015-03-09 at 20:11 +0900, Tomasz Figa wrote: ... +/* + * pimudev is a global var for dma_alloc_coherent. + * It is not accepatable, we will delete it if domain_alloc is enabled + */ +static struct device *pimudev; This is indeed not acceptable. Could you replace dma_alloc_coherent() with something that doesn't require device pointer, e.g. alloc_pages()? (Although that would require you to handle cache maintenance in the driver, due to cached memory allocated.) I need to think about a better solution for this. Hi, For 2nd level page table, we use cached memory now. Currently we are using __dma_flush_range to flush the cache, which is also unacceptable. For proper cache management, we'll need to use dma_map_single or dma_sync_*, which still need a deivce*. Looking at how already mainlined drivers do this, they either use dmac_flush_range() (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/msm_iommu.c?id=refs/tags/v4.0-rc3#n80) or directly __cpuc_flush_dcache_area() and outer_flush_range() (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/rockchip-iommu.c?id=refs/tags/v4.0-rc3#n93). Hi, These only exist in arch/arm, not arm64. I think we should avoid using API start with __ in drivers. This driver might be used in both arm/arm64, I think the only option for us is DMA APIs. Actually, I'm thinking that we should change to use coherent memory for 2nd level page table as well and totally skip the cache flush. It seems dma_pool_create is suitable to replace kmem_cache we are using right now. However it still need a device*, which we have to fix anyway. Joe.C ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
Hi Yong,
On Fri, Mar 6, 2015 at 6:37 PM, yong...@mediatek.com wrote:
From: Yong Wu yong...@mediatek.com
This patch add SMI(Smart Multimedia Interface) driver. This driver
is responsible to enable/disable iommu and control the clocks of each
local arbiter.
High-level:
Is there more to the smi (or smi-larb) driver, or is it always just a
1:1 wrapper for a particular m4u consumer?
In other words, instead of a separate driver, is it possible to move
this functionality into the m4u driver and/or the m4u consumers
directly?
Signed-off-by: Yong Wu yong...@mediatek.com
---
drivers/soc/mediatek/Kconfig | 7 ++
drivers/soc/mediatek/Makefile | 1 +
drivers/soc/mediatek/mt8173-smi.c | 143
++
include/linux/mtk-smi.h | 40 +++
4 files changed, 191 insertions(+)
create mode 100644 drivers/soc/mediatek/mt8173-smi.c
create mode 100644 include/linux/mtk-smi.h
diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index 729f93e..27fb26c 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -20,3 +20,10 @@ config MT8173_PMIC_WRAP
PMIC wrapper is a proprietary hardware in MT8173 to make
communication protocols to access PMIC device.
This driver implement access protocols for MT8173.
+
+config MTK_SMI
+bool
+ help
+ Smi help enable/disable iommu in mt8173 and control the
+ clock of each local arbiter.
+ It should be true while MTK_IOMMU enable.
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index 9b5709b..cdfe95c 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_MT8135_PMIC_WRAP) += mt8135-pmic-wrap.o
obj-$(CONFIG_MT8173_PMIC_WRAP) += mt8173-pmic-wrap.o
+obj-$(CONFIG_MTK_SMI) += mt8173-smi.o
diff --git a/drivers/soc/mediatek/mt8173-smi.c
b/drivers/soc/mediatek/mt8173-smi.c
new file mode 100644
index 000..4e3fab9
--- /dev/null
+++ b/drivers/soc/mediatek/mt8173-smi.c
@@ -0,0 +1,143 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu yong...@mediatek.com
+ *
+ * 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.
+ */
+#include linux/io.h
+#include linux/interrupt.h
+#include linux/platform_device.h
+#include linux/clk.h
+#include linux/err.h
+#include linux/mm.h
+
+#define SMI_LARB_MMU_EN (0xf00)
+#define F_SMI_MMU_EN(port) (1 (port))
+
+struct mtk_smi_larb {
+ void __iomem *larb_base;
+ struct clk *larb_clk[3];/* each larb has 3 clk at most */
+};
+
+static const char * const mtk_smi_clk_name[] = {
+ larb_sub0, larb_sub1, larb_sub2
+};
The order and meaning of these clocks do not seem particularly important.
It seems a bit awkward to use these arbitrary names just so we can use
devm_clk_get() to get a variably sized array of clocks from .dts.
Can we eliminate the clock-names property, and just use a single
.dts proprety that lists an array of clocks?
Then you would also get an explicit clock count, and can remove the
NULL checking when iterating.
An example of a clock list without names is:
Clock list in .dts:
https://lkml.org/lkml/2014/11/17/115
Filling in the clocks from .dts:
https://lkml.org/lkml/2014/11/17/114
Unfortunately, those patches never made it out of list discussion into
a maintainer tree.
+
+static const struct of_device_id mtk_smi_of_ids[] = {
+ { .compatible = mediatek,mt8173-smi-larb,
+ },
+ {}
+};
I find it a bit redundant to call the struct mtk_smi_larb, and then
to prepend larb_ to all of the fields.
In fact this whole driver is a bit confusing because it isn't clear if
this is an smi driver (of which only larb control has been
implemented) or is this an smi_larb driver (and potentially there
are other smi drivers).
Perhaps we can just call this an smi_larb driver, rename this file
to mt8173-smi-larb.c, and then doing something like:
struct mtk_smi_larb {
void __iomem *base;
struct clk *clk[3];/* each smi_larb has at most 3 clocks */
};
static const struct of_device_id mtk_smi_larb_of_ids[] = {
{ .compatible = mediatek,mt8173-smi-larb },
{}
};
+
+int mtk_smi_larb_get(struct platform_device *plarbdev)
Is there any reason to use struct platform_device here instead of
just struct device?
+{
+ struct mtk_smi_larb *larbpriv = dev_get_drvdata(plarbdev-dev);
+ int i, ret = 0;
+
+ for (i = 0; i
