Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Tomasz Figa]

Hi,

You can find part 2 of my comments inline.

On Fri, Mar 6, 2015 at 7:48 PM,  <yong.wu@xxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/