Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver
Dear Tomasz,
About a hardcode your comment, please help check below.
Dear Mark,
I would like to add a item in the dtsi of mtk-iommu. Please also
help have a look.
+static const struct mtk_iommu_port mtk_iommu_mt8173_port[] = {
+ /* port namem4uid slaveid larbid portid
tfid */
+ /* larb0 */
+ {M4U_PORT_DISP_OVL0, 0, 0,0, 0,
MTK_TFID(0, 0)},
+ {M4U_PORT_DISP_RDMA0, 0, 0,0, 1,
MTK_TFID(0, 1)},
+ {M4U_PORT_DISP_WDMA0, 0, 0,0, 2,
MTK_TFID(0, 2)},
+ {M4U_PORT_DISP_OD_R, 0, 0,0, 3,
MTK_TFID(0, 3)},
+ {M4U_PORT_DISP_OD_W, 0, 0,0, 4,
MTK_TFID(0, 4)},
+ {M4U_PORT_MDP_RDMA0, 0, 0,0, 5,
MTK_TFID(0, 5)},
+ {M4U_PORT_MDP_WDMA, 0, 0,0, 6,
MTK_TFID(0, 6)},
+ {M4U_PORT_MDP_WROT0, 0, 0,0, 7,
MTK_TFID(0, 7)},
[...]
+};
+
Anyway, is it really necessary to hardcode the SoC specific topology
data in this driver? Is there really any use besides of printing
port
name? If not, you could just print the values in a way letting you
quickly look up in the datasheet, without hardcoding this. Or even
better, you could print which devices are attached to the port.
a) Printing the port name is for debug. We could not request every
iommu
user to understand smilocal arbiter. When there is irq, they have to
look up the iommu's datasheet to find out which port error. if we
print
it directly, It may be more easily to debug.
b) In mtk_iommu_config_port, according to this hardcode we can be
easily
to get out which local arbiter and which port we prepare to config.
c) If we support different SOCs, we could change this arrays easily.
There is no similar code in the others iommu, so I prepare to
delete it, But we really need know which local arbiter and which port we
are going to config(which port will enable iommu)
so we prepare add a item in the dtsi like this:
iommu: mmsys_iommu@10205000 {
compatible = mediatek,mt8173-iommu;
...
+ larb-portes-nr = M4U_LARB0_PORT_NR
+M4U_LARB1_PORT_NR
+M4U_LARB2_PORT_NR
+M4U_LARB3_PORT_NR
+M4U_LARB4_PORT_NR
+M4U_LARB5_PORT_NR;
larb = larb0 larb1 larb2 larb3 larb4 larb5;
#iommu-cells = 1;
};
larb-portes-nr : the number of the portes in each local arbiter.
If we have this item, we can get which larb and which port from the
portid in the dtsi of the iommu user.
And while there is isr, I will print the larb-id and the port-id
instead of the string of the port name.
The M4U_LARB0_PORT_NR/... will be added in
dt-bindings/iommu/mt8173-iommu-port.h[0]
Dear Mark,
As above, if I add this item in the dtsi of iommu, is it ok?
[0]:http://lists.linuxfoundation.org/pipermail/iommu/2015-March/012450.html
___
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 Wed, 2015-04-15 at 11:20 +0900, Tomasz Figa wrote: On Tue, Apr 14, 2015 at 3:31 PM, Yong Wu yong...@mediatek.com wrote: + + piommu-protect_va = devm_kmalloc(piommu-dev, MTK_PROTECT_PA_ALIGN*2, style: Operators like * should have space on both sides. + GFP_KERNEL); Shouldn't dma_alloc_coherent() be used for this? We don't care the data in it. I think they are the same. Could you help tell me why dma_alloc_coherent may be better. Can you guarantee that at the time you allocate the memory using devm_kmalloc() the memory is not dirty (i.e. some write back data are stored in CPU cache) and is not going to be written back in some time, overwriting data put there by IOMMU hardware? As I noted in the function mtk_iommu_hw_init: /* protect memory,HW will write here while translation fault */ protectpa = __virt_to_phys(piommu-protect_va); We don’t care the content of this buffer, It is ok even though its data is dirty. It seem to be a the protect memory. While a translation fault happened, The iommu HW will overwrite here instead of writing to the fault physical address which may be 0 or some random address. Do you mean that it's just a dummy page for hardware behind the IOMMU to access when the mapping is not available? How would that work with potential on demand paging when the hardware needs to be blocked until the mapping is created? Best regards, Tomasz 1. YES 2. Sorry. Our iommu HW can not support this right now. The HW can not be blocked until the mapping is created. If the page is not ready, we can not get the physical address, then How to fill the pagetable for that memory. I think the dmaiommu may guaranty it? ___ 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 Wed, Apr 15, 2015 at 4:06 PM, Yong Wu yong...@mediatek.com wrote: On Wed, 2015-04-15 at 11:20 +0900, Tomasz Figa wrote: On Tue, Apr 14, 2015 at 3:31 PM, Yong Wu yong...@mediatek.com wrote: + + piommu-protect_va = devm_kmalloc(piommu-dev, MTK_PROTECT_PA_ALIGN*2, style: Operators like * should have space on both sides. + GFP_KERNEL); Shouldn't dma_alloc_coherent() be used for this? We don't care the data in it. I think they are the same. Could you help tell me why dma_alloc_coherent may be better. Can you guarantee that at the time you allocate the memory using devm_kmalloc() the memory is not dirty (i.e. some write back data are stored in CPU cache) and is not going to be written back in some time, overwriting data put there by IOMMU hardware? As I noted in the function mtk_iommu_hw_init: /* protect memory,HW will write here while translation fault */ protectpa = __virt_to_phys(piommu-protect_va); We don’t care the content of this buffer, It is ok even though its data is dirty. It seem to be a the protect memory. While a translation fault happened, The iommu HW will overwrite here instead of writing to the fault physical address which may be 0 or some random address. Do you mean that it's just a dummy page for hardware behind the IOMMU to access when the mapping is not available? How would that work with potential on demand paging when the hardware needs to be blocked until the mapping is created? Best regards, Tomasz 1. YES 2. Sorry. Our iommu HW can not support this right now. The HW can not be blocked until the mapping is created. OK, that explains it. Well, then I guess this is necessary and contents of that memory don't matter that much. (Although, this might be a minor security issue, because the faulting hardware would get access to some data previously stored by kernel code. Not sure how much of a threat would that be, though.) If the page is not ready, we can not get the physical address, then How to fill the pagetable for that memory. I think the dmaiommu may guaranty it? If your hardware can't block until the mapping is created then what you do currently seems to be the only option. (+/- the missing cache maintenance at initialization) 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
Hi Tomasz,
Thanks very much for you suggestion and explain so detail.
please help check below.
On Fri, 2015-03-27 at 18:41 +0900, Tomasz Figa wrote:
Hi Yong Wu,
Sorry for long delay, I had to figure out some time to look at this again.
On Wed, Mar 18, 2015 at 8:22 PM, Yong Wu yong...@mediatek.com wrote:
+ imudev = piommu-dev;
+
+ spin_lock_irqsave(priv-portlock, flags);
What is protected by this spinlock?
We will write a register of the local arbiter while config port. If
some modules are in the same local arbiter, it may be overwrite. so I
add it here.
OK. Maybe it could be called larb_lock then? It would be good to have
structures or code that should be running under this spinlock
annotated with proper comments. And purpose of the lock documented in
a comment as well (probably in a kerneldoc-style documentation of
priv).
Thanks. I have move the spinlock into the smi driver, it will lock
for writing the local arbiter regsiter only.
+static void mtk_iommu_detach_device(struct iommu_domain *domain,
+ struct device *dev)
+{
No hardware (de)configuration or clean-up necessary?
I will add it. Actually we design like this:If a device have attached to
iommu domain, it won't detach from it.
Isn't proper clean-up required for module removal? Some drivers might
be required to be loadable modules, which should be unloadable.
+
+ piommu-protect_va = devm_kmalloc(piommu-dev,
MTK_PROTECT_PA_ALIGN*2,
style: Operators like * should have space on both sides.
+ GFP_KERNEL);
Shouldn't dma_alloc_coherent() be used for this?
We don't care the data in it. I think they are the same. Could you
help tell me why dma_alloc_coherent may be better.
Can you guarantee that at the time you allocate the memory using
devm_kmalloc() the memory is not dirty (i.e. some write back data are
stored in CPU cache) and is not going to be written back in some time,
overwriting data put there by IOMMU hardware?
As I noted in the function mtk_iommu_hw_init:
/* protect memory,HW will write here while translation fault */
protectpa = __virt_to_phys(piommu-protect_va);
We don’t care the content of this buffer, It is ok even though its
data is dirty.
It seem to be a the protect memory. While a translation fault
happened, The iommu HW will overwrite here instead of writing to the
fault physical address which may be 0 or some random address.
+
+ iommu_set_fault_handler(domain, mtk_iommu_fault_handler, piommu);
I don't see any other drivers doing this. Isn't this for upper layers,
so that they can set their own generic fault handlers?
I think that this function is related with the iommu domain, we
have only one multimedia iommu domain. so I add it after the iommu
domain are created.
No, this function is for drivers of IOMMU clients (i.e. master IP
blocks) which want to subscribe to page fault to do things like paging
on demand and so on. It shouldn't be called by IOMMU driver. Please
see other IOMMU drivers, for example rockchip-iommmu.c.
Thanks. I have read it. I will delete it and print the error info
in the ISR. Also call the report_iommu_fault in the ISR.
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, Apr 14, 2015 at 3:31 PM, Yong Wu yong...@mediatek.com wrote: + + piommu-protect_va = devm_kmalloc(piommu-dev, MTK_PROTECT_PA_ALIGN*2, style: Operators like * should have space on both sides. + GFP_KERNEL); Shouldn't dma_alloc_coherent() be used for this? We don't care the data in it. I think they are the same. Could you help tell me why dma_alloc_coherent may be better. Can you guarantee that at the time you allocate the memory using devm_kmalloc() the memory is not dirty (i.e. some write back data are stored in CPU cache) and is not going to be written back in some time, overwriting data put there by IOMMU hardware? As I noted in the function mtk_iommu_hw_init: /* protect memory,HW will write here while translation fault */ protectpa = __virt_to_phys(piommu-protect_va); We don’t care the content of this buffer, It is ok even though its data is dirty. It seem to be a the protect memory. While a translation fault happened, The iommu HW will overwrite here instead of writing to the fault physical address which may be 0 or some random address. Do you mean that it's just a dummy page for hardware behind the IOMMU to access when the mapping is not available? How would that work with potential on demand paging when the hardware needs to be blocked until the mapping is created? 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 Mon, Mar 09, 2015 at 12:11:43PM +, Yong Wu wrote: On Fri, 2015-03-06 at 10:58 +, Will Deacon wrote: On Fri, Mar 06, 2015 at 10:48:17AM +, 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. [...] +/* 2 level pagetable: pgd - pte */ +#define F_PTE_TYPE_GET(regval) (regval 0x3) +#define F_PTE_TYPE_LARGE BIT(0) +#define F_PTE_TYPE_SMALL BIT(1) +#define F_PTE_B_BIT BIT(2) +#define F_PTE_C_BIT BIT(3) +#define F_PTE_BIT32_BIT BIT(9) +#define F_PTE_S_BIT BIT(10) +#define F_PTE_NG_BIT BIT(11) +#define F_PTE_PA_LARGE_MSK(~0UL 16) +#define F_PTE_PA_LARGE_GET(regval)((regval 16) 0x) +#define F_PTE_PA_SMALL_MSK(~0UL 12) +#define F_PTE_PA_SMALL_GET(regval)((regval 12) (~0)) +#define F_PTE_TYPE_IS_LARGE_PAGE(pte) ((imu_pte_val(pte) 0x3) == \ + F_PTE_TYPE_LARGE) +#define F_PTE_TYPE_IS_SMALL_PAGE(pte) ((imu_pte_val(pte) 0x3) == \ + F_PTE_TYPE_SMALL) This looks like the ARM short-descriptor format to me. Could you please add a new page table format to the io-pgtable code, so that other IOMMU drivers can make use of this? I know there was some interest in using short descriptor for the ARM SMMU, for example. Currently I not familiar with the io-pgtable,I may need some time for it and the ARM short-descriptor. Well, you can read the LPAE version I wrote in io-pgtable-arm.c for some inspiration (it's used by arm-smmu.c and ipmmu-vmsa.c). And there are some difference between mediatek's pagetable with the standard short-descriptor, like bit 9. we use it for the dram over 4GB. Then how should we do if there are some difference. That can easily be handled using a quirk (see, for example, IO_PGTABLE_QUIRK_ARM_NS). Will ___ 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
Dear Tomasz,
Thanks very much for review so detail!
Please check my reply below. Others I will fix it in the next
version.
And I have got your comment of [2/5]. Do you have plan for the
other patch?
On Sun, 2015-03-08 at 13:12 +0900, Tomasz Figa wrote:
Hi Yong Wu,
Thanks for this series. Please see my comments inline.
On Fri, Mar 6, 2015 at 7:48 PM, 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.
[snip]
+static const struct mtk_iommu_port mtk_iommu_mt8173_port[] = {
+ /* port namem4uid slaveid larbid portid tfid */
+ /* larb0 */
+ {M4U_PORT_DISP_OVL0, 0, 0,0, 0, MTK_TFID(0, 0)},
+ {M4U_PORT_DISP_RDMA0, 0, 0,0, 1, MTK_TFID(0, 1)},
+ {M4U_PORT_DISP_WDMA0, 0, 0,0, 2, MTK_TFID(0, 2)},
+ {M4U_PORT_DISP_OD_R, 0, 0,0, 3, MTK_TFID(0, 3)},
+ {M4U_PORT_DISP_OD_W, 0, 0,0, 4, MTK_TFID(0, 4)},
+ {M4U_PORT_MDP_RDMA0, 0, 0,0, 5, MTK_TFID(0, 5)},
+ {M4U_PORT_MDP_WDMA, 0, 0,0, 6, MTK_TFID(0, 6)},
+ {M4U_PORT_MDP_WROT0, 0, 0,0, 7, MTK_TFID(0, 7)},
[snip]
+ {M4U_PORT_HSIC_MAS, 1, 0,6, 12, 0x11},
+ {M4U_PORT_HSIC_DEV, 1, 0,6, 13, 0x19},
+ {M4U_PORT_AP_DMA,1, 0,6, 14, 0x18},
+ {M4U_PORT_HSIC_DMA, 1, 0,6, 15, 0xc8},
+ {M4U_PORT_MSDC0, 1, 0,6, 16, 0x0},
+ {M4U_PORT_MSDC3, 1, 0,6, 17, 0x20},
+ {M4U_PORT_UNKNOWN, 1, 0,6, 18, 0xf},
Why the MTK_TFID() macro is not used for perisys iommu?
The perisys iommu don't connected with SMI and Local arbiter.
it's translation fault id is not MTK_TFID(x, y).it is special.
For this perisys iommu , it is different with multimedia iommu,
we don't support it in this version, We have plan to delete perisys
iommu port next time.
+};
+
Anyway, is it really necessary to hardcode the SoC specific topology
data in this driver? Is there really any use besides of printing port
name? If not, you could just print the values in a way letting you
quickly look up in the datasheet, without hardcoding this. Or even
better, you could print which devices are attached to the port.
a) Printing the port name is for debug. We could not request every iommu
user to understand smilocal arbiter. When there is irq, they have to
look up the iommu's datasheet to find out which port error. if we print
it directly, It may be more easily to debug.
b) In mtk_iommu_config_port, according to this hardcode we can be easily
to get out which local arbiter and which port we prepare to config.
c) If we support different SOCs, we could change this arrays easily.
+static const struct mtk_iommu_cfg mtk_iommu_mt8173_cfg = {
+ .larb_nr = 6,
+ .m4u_port_nr = ARRAY_SIZE(mtk_iommu_mt8173_port),
+ .pport = mtk_iommu_mt8173_port,
+};
+
+static const char *mtk_iommu_get_port_name(const struct mtk_iommu_info
*piommu,
+ unsigned int portid)
+{
+ const struct mtk_iommu_port *pcurport = NULL;
+
+ pcurport = piommu-imucfg-pport + portid;
+ if (portid piommu-imucfg-m4u_port_nr pcurport)
+ return pcurport-port_name;
+ else
+ return UNKNOWN_PORT;
+}
This function seems to be used just for printing the hardcoded port names.
+
+static int mtk_iommu_get_port_by_tfid(const struct mtk_iommu_info *pimu,
+ int tf_id)
+{
+ const struct mtk_iommu_cfg *pimucfg = pimu-imucfg;
+ int i;
+ unsigned int portid = pimucfg-m4u_port_nr;
+
+ for (i = 0; i pimucfg-m4u_port_nr; i++) {
+ if (pimucfg-pport[i].tf_id == tf_id) {
+ portid = i;
+ break;
+ }
+ }
+ if (i == pimucfg-m4u_port_nr)
+ dev_err(pimu-dev, tf_id find fail, tfid %d\n, tf_id);
+ return portid;
+}
This function seems to be used just for finding an index into the
array of hardcoded port names for printing purposes.
Yes. mtk_iommu_get_port_name and mtk_iommu_get_port_by_tfid is
only for find out the right port to print for improve debug.
[snip]
+
+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 =
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 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 2/5] iommu/mediatek: Add mt8173 IOMMU driver
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?
+ writel(0, m4u_base + REG_MMU_CPE_DONE);
+ }
+
+ return 0;
+}
-Mitch
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
