From: Tomasz Figa <tfiga@chromium.org>
To: "Yong Wu (吴勇)" <yong.wu@mediatek.com>
Cc: Rob Herring <robh+dt@kernel.org>, Joerg Roedel <joro@8bytes.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Robin Murphy <robin.murphy@arm.com>,
Will Deacon <will.deacon@arm.com>,
Daniel Kurtz <djkurtz@google.com>,
Lucas Stach <l.stach@pengutronix.de>,
Mark Rutland <mark.rutland@arm.com>,
Catalin Marinas <catalin.marinas@arm.com>,
linux-mediatek@lists.infradead.org,
Sasha Hauer <kernel@pengutronix.de>,
srv_heupstream@mediatek.com, devicetree@vger.kernel.org,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
iommu@lists.linux-foundation.org
Subject: Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver
Date: Mon, 9 Mar 2015 20:11:40 +0900 [thread overview]
Message-ID: <CAAFQd5AhTe=d9tXN8kaRah_GiZYYxG0cD0up=cbHkSfbBnXmvQ@mail.gmail.com> (raw)
In-Reply-To: <1425638900-24989-3-git-send-email-yong.wu@mediatek.com>
Hi,
You can find part 2 of my comments inline.
On Fri, Mar 6, 2015 at 7:48 PM, <yong.wu@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))
> + dev_err_ratelimited(dev, "tlb miss-fault!port=%s iova=0x%x\n",
> + mtk_iommu_get_port_name(piommu, m4u_port),
> + fault_mva);
> +
> + mtk_iommu_invalidate_tlb(piommu, 1, 0, 0);
> +
> + mtk_iommu_clear_intr(m4u_base);
As per my comment above, most of what is happening in this function
should be actually done in interrupt handler, excluding printing of
course.
> +
> + return 0;
> +}
> +
> +static int mtk_iommu_parse_dt(struct platform_device *pdev,
> + struct mtk_iommu_info *piommu)
> +{
> + struct device *piommudev = &pdev->dev;
> + struct device_node *ofnode;
> + struct resource *res;
> + unsigned int mtk_iommu_cell = 0;
> + unsigned int i;
> +
> + ofnode = piommudev->of_node;
You could do this assignment on the same line as declaration.
nit: The usual naming convention for such variables are "dev" for
struct device * and "np" for struct device_node *.
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + piommu->m4u_base = devm_ioremap_resource(&pdev->dev, res);
nit: There doesn't seem to be any other "bases" involved in this
driver. Could you simply name the field "base", without the obvious
prefix "m4u_"?
> + if (IS_ERR(piommu->m4u_base)) {
> + dev_err(piommudev, "m4u_base %p err\n", piommu->m4u_base);
> + goto iommu_dts_err;
> + }
> +
> + piommu->irq = platform_get_irq(pdev, 0);
> + if (piommu->irq < 0) {
> + dev_err(piommudev, "irq err %d\n", piommu->irq);
Please keep the messages human readable, e.g. "Failed to get IRQ (%d)\n"
> + goto iommu_dts_err;
> + }
> +
> + piommu->m4u_infra_clk = devm_clk_get(piommudev, "infra_m4u");
> + if (IS_ERR(piommu->m4u_infra_clk)) {
> + dev_err(piommudev, "clk err %p\n", piommu->m4u_infra_clk);
"Failed to get clock 'infra_m4u' (%d)\n" (and extract the error code
using PTR_ERR() helper.
> + goto iommu_dts_err;
> + }
> +
> + of_property_read_u32(ofnode, "#iommu-cells", &mtk_iommu_cell);
> + if (mtk_iommu_cell != 1) {
> + dev_err(piommudev, "iommu-cell fail:%d\n", mtk_iommu_cell);
> + goto iommu_dts_err;
> + }
I don't think you should be checking this here. The function
performing translation from phandle and specifier should check whether
the correct cell count was provided.
> +
> + for (i = 0; i < piommu->imucfg->larb_nr; i++) {
> + struct device_node *larbnode;
> +
> + larbnode = of_parse_phandle(ofnode, "larb", i);
> + piommu->larbpdev[i] = of_find_device_by_node(larbnode);
> + of_node_put(larbnode);
I need to take a look at further changes, but this looks like syscon
should be used here instead of using the device directly.
> + if (!piommu->larbpdev[i]) {
> + dev_err(piommudev, "larb pdev fail@larb%d\n", i);
> + goto iommu_dts_err;
> + }
> + }
> +
> + return 0;
> +
> +iommu_dts_err:
> + return -EINVAL;
Please return the return value that actually brought us here.
> +}
> +
> +static int mtk_iommu_hw_init(const struct mtk_iommu_domain *mtkdomain)
> +{
> + struct mtk_iommu_info *piommu = mtkdomain->piommuinfo;
> + void __iomem *gm4ubaseaddr = piommu->m4u_base;
Hmm, gm4ubaseaddr is exactly as long as piommu->base (if you follow my
comment to rename this field). In general,
> + phys_addr_t protectpa;
> + u32 regval, protectreg;
> + int ret = 0;
> +
> + ret = clk_prepare_enable(piommu->m4u_infra_clk);
> + if (ret) {
> + dev_err(piommu->dev, "m4u clk enable error\n");
> + return -ENODEV;
> + }
> +
> + writel((u32)mtkdomain->pgd_pa, gm4ubaseaddr + REG_MMUG_PT_BASE);
Why this has to be casted? Is the type of pgd_pa field correct?
> +
> + regval = F_MMU_CTRL_REROUTE_PFQ_TO_MQ_EN |
> + F_MMU_CTRL_TF_PROT_VAL(2) |
> + F_MMU_CTRL_COHERE_EN;
> + writel(regval, gm4ubaseaddr + REG_MMU_CTRL_REG);
> +
> + writel(0x6f, gm4ubaseaddr + REG_MMU_INT_L2_CONTROL);
> + writel(0xffffffff, gm4ubaseaddr + REG_MMU_INT_MAIN_CONTROL);
Please define all the bitfields and use the definitions here instead
of magic numbers.
> +
> + /* protect memory,HW will write here while translation fault */
> + protectpa = __virt_to_phys(piommu->protect_va);
Why the underscore variant? virt_to_phys() should be just fine.
> + protectpa = ALIGN(protectpa, MTK_PROTECT_PA_ALIGN);
Shouldn't protect_va be already aligned?
> + protectreg = (u32)F_MMU_IVRP_PA_SET(protectpa);
Again, why is this cast needed?
> + writel(protectreg, gm4ubaseaddr + REG_MMU_IVRP_PADDR);
> +
> + writel(0, gm4ubaseaddr + REG_MMU_DCM_DIS);
> + writel(0, gm4ubaseaddr + REG_MMU_STANDARD_AXI_MODE);
> +
> + return 0;
> +}
> +
> +static inline void mtk_iommu_config_port(struct mtk_iommu_info *piommu,
> + int portid)
Please let the compiler decide whether to inline this function or not.
> +{
> + int larb, larb_port;
> +
> + larb = piommu->imucfg->pport[portid].larb_id;
> + larb_port = piommu->imucfg->pport[portid].port_id;
> +
> + mtk_smi_config_port(piommu->larbpdev[larb], larb_port);
> +}
> +
> +/*
> + * 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.
OK. Enough for part 2. Stay tuned for part 3.
Best regards,
Tomasz
next prev parent reply other threads:[~2015-03-09 11:12 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-06 10:48 [RFC PATCH 0/5] MT8173 IOMMU support yong.wu
2015-03-06 10:48 ` [PATCH 1/5] soc: mediatek: Add SMI driver yong.wu
2015-03-06 11:30 ` Paul Bolle
2015-03-09 11:57 ` Yong Wu
2015-03-09 17:59 ` Paul Bolle
2015-03-09 21:54 ` Arnd Bergmann
2015-03-10 6:17 ` Yingjoe Chen
2015-03-09 3:26 ` Yingjoe Chen
2015-03-09 21:56 ` Arnd Bergmann
2015-03-10 6:27 ` Yingjoe Chen
2015-03-10 9:05 ` Arnd Bergmann
2015-03-10 9:24 ` Lucas Stach
2015-03-09 11:03 ` Sascha Hauer
2015-03-06 10:48 ` [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver yong.wu
2015-03-06 10:58 ` Will Deacon
2015-03-09 12:11 ` Yong Wu
2015-03-17 15:14 ` Will Deacon
2015-03-06 17:15 ` Mitchel Humpherys
2015-03-09 12:16 ` Yong Wu
2015-03-09 16:57 ` Mitchel Humpherys
2015-03-08 4:12 ` Tomasz Figa
2015-03-12 14:16 ` Yong Wu
2015-03-09 8:24 ` Daniel Kurtz
2015-03-09 11:11 ` Tomasz Figa [this message]
2015-03-09 14:46 ` Yingjoe Chen
2015-03-09 17:00 ` Tomasz Figa
2015-03-10 3:41 ` Yingjoe Chen
2015-03-10 4:06 ` Tomasz Figa
2015-03-11 10:53 ` Tomasz Figa
2015-03-18 11:22 ` Yong Wu
2015-03-20 19:14 ` Robin Murphy
2015-04-14 6:50 ` Yong Wu
2015-03-27 9:41 ` Tomasz Figa
2015-04-14 6:31 ` Yong Wu
2015-04-15 2:20 ` Tomasz Figa
2015-04-15 7:06 ` Yong Wu
2015-04-15 7:41 ` Tomasz Figa
2015-04-29 6:23 ` Yong Wu
2015-03-06 10:48 ` [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding yong.wu
2015-03-06 11:13 ` Mark Rutland
2015-03-09 12:55 ` Yong Wu
2015-04-14 9:07 ` Yong Wu
2015-04-14 10:06 ` Mark Rutland
2015-04-14 13:49 ` Yong Wu
2015-04-14 13:55 ` Yong Wu
2015-04-14 13:56 ` Mark Rutland
2015-03-06 14:48 ` Sergei Shtylyov
2015-03-09 12:32 ` Yong Wu
2015-03-06 10:48 ` [PATCH 4/5] dt-bindings: iommu: Add binding for mediatek IOMMU yong.wu
2015-03-06 11:21 ` Mark Rutland
2015-03-09 11:30 ` Yong Wu
2015-03-06 10:48 ` [PATCH 5/5] dts: mt8173: Add iommu/smi nodes for mt8173 yong.wu
2015-03-07 15:20 ` Daniel Kurtz
2015-03-09 12:18 ` Yong Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAFQd5AhTe=d9tXN8kaRah_GiZYYxG0cD0up=cbHkSfbBnXmvQ@mail.gmail.com' \
--to=tfiga@chromium.org \
--cc=catalin.marinas@arm.com \
--cc=devicetree@vger.kernel.org \
--cc=djkurtz@google.com \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.org \
--cc=kernel@pengutronix.de \
--cc=l.stach@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=matthias.bgg@gmail.com \
--cc=robh+dt@kernel.org \
--cc=robin.murphy@arm.com \
--cc=srv_heupstream@mediatek.com \
--cc=will.deacon@arm.com \
--cc=yong.wu@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).