On 24/05/16 10:57, Honghui Zhang wrote:
[...]
@@ -48,6 +48,9 @@ struct mtk_iommu_domain {
struct io_pgtable_ops *iop;
struct iommu_domain domain;
+ void *pgt_va;
+ dma_addr_t pgt_pa;
+ void *cookie;
These are going to be mutually exclusive with the cfg and iop members,
which implies it might be a good idea to use a union and not waste
space. Or better, just forward-declare struct mtk_iommu_domain here and
leave separate definitions private to each driver. The void *cookie is
also an unnecessary level of abstraction, I think.
Do you mean declare struct mtk_iommu_domain here, and implement a new
struct in mtk_iommu_v1.c like
struct mtk_iommu_domain_v1 {
struct mtk_iommu_domain domain;
u32 *pgt_va;
dma_addr_t pgt_pa;
mtk_iommu_data *data;
};
If this is acceptable I would implement it in the next version.
Pretty much, except they both want to be called struct mtk_iommu_domain,
so that a *declaration* for the sake of the m4u_dom member of struct
mtk_iommu_data in the header file can remain common to both drivers - it
then just picks up whichever private *definition* from the .c file being
compiled.
};
struct mtk_iommu_data {
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
new file mode 100644
index 0000000..55023e1
--- /dev/null
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -0,0 +1,742 @@
+/*
+ * Copyright (c) 2015-2016 MediaTek Inc.
+ * Author: Yong Wu <yong.wu@xxxxxxxxxxxx>
Nit: is that in the sense that this patch should also have Yong's
signed-off-by on it, or in that it's your work derived from his version
in mtk_iommu.c?
I write this driver based on Yong's version of mtk_iommu.c, should I add
his signed-off-by for this patch? Or should I put a comment about this?
Thanks.
OK, in that case I think the appropriate attribution would be along the
lines of "Author: Honghui Zhang, based on mtk_iommu.c by Yong Wu" (if in
doubt, grepping for "Based on" gives a feel for how this is commonly
done). If the work that comprises this patch itself (i.e. the copying
and modification of the existing code) is all yours then your sign-off
alone is fine.
[...]
+static int mtk_iommu_add_device(struct device *dev)
+{
+ struct iommu_group *group;
+ struct device_node *np;
+ struct of_phandle_args iommu_spec;
+ int idx = 0;
+
+ while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+ "#iommu-cells", idx,
+ &iommu_spec)) {
Hang on, this doesn't seem right - why do you need to reimplement all
this instead of using IOMMU_OF_DECLARE()?
All the clients of mtk generation one iommu share the same iommu domain,
as a matter of fact, mtk generation one iommu only support one iommu
domain. ALl the clients share the same iova address and use the same
pagetable. That means all iommu clients needed to be attached to the
same dma_iommu_mapping.
Ugh, right - I'd forgotten that the arch/arm DMA mapping code doesn't
respect IOMMU groups or default domains at all. That's the real root
cause of the issue here.
If use IOMMU_OF_DELCARE, we need to call of_iommu_set_ops to set the
iommu_ops, I do not want the iommu_ops be set since it would cause iommu
client device in different dma_iommu_mapping.
When an iommu client device has been created, the following sequence is
called.
of_platform_device_create
->of_dma_config
->arch_setup_dma_ops
->arch_setup_iommu_dma_ops
In this function of arch_setup_iommu_dma_ops would create a new
dma_iommu_mapping for each iommu client device and then attach the
device to this new dma_iommu_mapping. Since all the iommu clients share
the very same pagetable, this will not workable for our HW.
I could not release the dma_iommu_mapping in attach_device since the
to_dma_iommu_mapping was set after device_attached.
Any suggest for this?
On a second look, you're doing more or less the same thing that the
Renesas IPMMU driver currently does, so it's probably OK as a workaround
for now. Fixing the arch/arm code is part of the bigger ongoing problem
of sorting out IOMMU probing and DMA configuration, and it doesn't seem
fair to force that on you for the sake of one driver ;)
[...]
+static int __maybe_unused mtk_iommu_resume(struct device *dev)
+{
+ struct mtk_iommu_data *data = dev_get_drvdata(dev);
+ struct mtk_iommu_suspend_reg *reg = &data->reg;
+ void __iomem *base = data->base;
+
+ writel_relaxed(data->m4u_dom->pgt_pa, base + REG_MMU_PT_BASE_ADDR);
Hmm, this looks like the only case where m4u_dom actually seems
necessary - I'm pretty sure all the others could be fairly easily
reworked to not use it (I might try having a quick hack at the existing
M4U driver to see) - at which point we could just explicitly
save/restore the base register here and get rid of m4u_dom entirely.
Let me take a while to think about this.
That was true in the context of arm64, but you're right that the current
state of the 32-bit code does make m4u_dom more necessary, so I guess we
may as well leave it as-is for now.
Robin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
