Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1193088 > unrolled thread
| Started by | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| First post | 2015-07-27 15:30 +0200 |
| Last post | 2015-07-29 07:50 +0200 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver Robin Murphy <robin.murphy@arm.com> - 2015-07-27 15:30 +0200
Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu <yong.wu@mediatek.com> - 2015-07-29 07:50 +0200
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2015-07-27 15:30 +0200 |
| Subject | Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver |
| Message-ID | <pQQDE-37q-21@gated-at.bofh.it> |
On 16/07/15 10:04, Yong Wu wrote:
> This patch adds support for mediatek m4u (MultiMedia Memory Management
> Unit).
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
[...]
> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> +{
> + struct mtk_iommu_domain *domain = cookie;
> + unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> +
> + dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> + size, DMA_TO_DEVICE);
Nit: this looks like it may as well be dma_map_single.
It would probably be worth following it with a matching unmap too, just
to avoid any possible leakage bugs (especially if this M4U ever appears
in a SoC supporting RAM above the 32-bit boundary).
> +}
> +
[...]
> +static int mtk_iommu_parse_dt(struct platform_device *pdev,
> + struct mtk_iommu_data *data)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *ofnode;
> + struct resource *res;
> + int i;
> +
> + ofnode = dev->of_node;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->base))
> + return PTR_ERR(data->base);
> +
> + data->irq = platform_get_irq(pdev, 0);
> + if (data->irq < 0)
> + return data->irq;
> +
> + data->bclk = devm_clk_get(dev, "bclk");
> + if (IS_ERR(data->bclk))
> + return PTR_ERR(data->bclk);
> +
> + data->larb_nr = of_count_phandle_with_args(
> + ofnode, "mediatek,larb", NULL);
> + if (data->larb_nr < 0)
> + return data->larb_nr;
> +
> + for (i = 0; i < data->larb_nr; i++) {
> + struct device_node *larbnode;
> + struct platform_device *plarbdev;
> +
> + larbnode = of_parse_phandle(ofnode, "mediatek,larb", i);
> + if (!larbnode)
> + return -EINVAL;
> +
> + plarbdev = of_find_device_by_node(larbnode);
> + of_node_put(larbnode);
> + if (!plarbdev)
> + return -EPROBE_DEFER;
> + data->larbdev[i] = &plarbdev->dev;
> + }
At a glance this seems like a job for of_parse_phandle_with_args, but I
may be missing something subtle.
> + return 0;
> +}
> +
[...]
> +static int mtk_iommu_init_domain_context(struct mtk_iommu_domain *dom)
> +{
> + int ret;
> +
> + if (dom->iop)
> + return 0;
> +
> + spin_lock_init(&dom->pgtlock);
> + dom->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS |
> + IO_PGTABLE_QUIRK_SHORT_SUPERSECTION |
> + IO_PGTABLE_QUIRK_SHORT_MTK;
> + dom->cfg.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
> + dom->cfg.ias = 32;
> + dom->cfg.oas = 32;
> + dom->cfg.tlb = &mtk_iommu_gather_ops;
> +
> + dom->iop = alloc_io_pgtable_ops(ARM_SHORT_DESC, &dom->cfg, dom);
> + if (!dom->iop) {
> + pr_err("Failed to alloc io pgtable\n");
> + return -EINVAL;
> + }
> +
> + /* Update our support page sizes bitmap */
> + mtk_iommu_ops.pgsize_bitmap = dom->cfg.pgsize_bitmap;
> +
> + ret = mtk_iommu_hw_init(dom);
> + if (ret)
> + free_io_pgtable_ops(dom->iop);
> +
> + return ret;
> +}
I don't see that you need the above function at all - since your
pagetable config is fixed and doesn't have any depency on which IOMMU
you're attaching to, can't you just do all of that straight away in
domain_alloc?
> +static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
> +{
> + struct mtk_iommu_domain *priv;
> +
> + /* We only support unmanaged domains for now */
> + if (type != IOMMU_DOMAIN_UNMANAGED)
> + return NULL;
Have you had a chance to play with the latest DMA mapping series now
that I've integrated the default domain changes? I think if you handled
IOMMU_DOMAIN_DMA requests here and made them always return the
(preallocated) private domain, you should be able to get rid of the
tricks in attach_device completely.
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return NULL;
> +
> + priv->domain.geometry.aperture_start = 0;
> + priv->domain.geometry.aperture_end = (1ULL << 32) - 1;
DMA_BIT_MASK(32) ?
> + priv->domain.geometry.force_aperture = true;
> +
> + return &priv->domain;
> +}
> +
[...]
> +static int mtk_iommu_add_device(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom;
> + struct iommu_group *group;
> + struct mtk_iommu_client_priv *devhead;
> + int ret;
> +
> + if (!dev->archdata.dma_ops)/* Not a iommu client device */
> + return -ENODEV;
I think archdata.dma_ops is the wrong thing to test here. It would be
better to use archdata.iommu, since you go on to dereference that
unconditionally anyway, plus then you only depend on your own of_xlate
behaviour, rather than some quirk of the arch code (which could quietly
change in future).
> + group = iommu_group_get(dev);
> + if (!group) {
> + group = iommu_group_alloc();
> + if (IS_ERR(group)) {
> + dev_err(dev, "Failed to allocate IOMMU group\n");
> + return PTR_ERR(group);
> + }
> + }
> +
> + ret = iommu_group_add_device(group, dev);
> + if (ret) {
> + dev_err(dev, "Failed to add IOMMU group\n");
> + goto err_group_put;
> + }
> +
> + /* get the mtk_iommu_domain from the iommu device */
> + devhead = dev->archdata.iommu;
> + mtkdom = devhead->imudev->archdata.iommu;
> + ret = iommu_attach_group(&mtkdom->domain, group);
> + if (ret)
> + dev_err(dev, "Failed to attach IOMMU group\n");
> +
> +err_group_put:
> + iommu_group_put(group);
> + return ret;
> +}
> +
[...]
> +static struct iommu_ops mtk_iommu_ops = {
> + .domain_alloc = mtk_iommu_domain_alloc,
> + .domain_free = mtk_iommu_domain_free,
> + .attach_dev = mtk_iommu_attach_device,
> + .detach_dev = mtk_iommu_detach_device,
> + .map = mtk_iommu_map,
> + .unmap = mtk_iommu_unmap,
> + .map_sg = default_iommu_map_sg,
> + .iova_to_phys = mtk_iommu_iova_to_phys,
> + .add_device = mtk_iommu_add_device,
> + .of_xlate = mtk_iommu_of_xlate,
> + .pgsize_bitmap = -1UL,
As above, if you know the hardware only supports a single descriptor
format with a fixed set of page sizes, why not just represent that directly?
> +};
> +
[...]
> +static int mtk_iommu_suspend(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
> + struct mtk_iommu_data *data = mtkdom->data;
> + void __iomem *base = data->base;
> + unsigned int i = 0;
> +
> + data->reg[i++] = readl(base + REG_MMU_PT_BASE_ADDR);
Redundancy nit: any particular reason for saving this here rather than
simply restoring it from mtkdom->cfg.arm_short_cfg.ttbr[0] on resume?
> + data->reg[i++] = readl(base + REG_MMU_STANDARD_AXI_MODE);
> + data->reg[i++] = readl(base + REG_MMU_DCM_DIS);
> + data->reg[i++] = readl(base + REG_MMU_CTRL_REG);
> + data->reg[i++] = readl(base + REG_MMU_IVRP_PADDR);
> + data->reg[i++] = readl(base + REG_MMU_INT_CONTROL0);
> + data->reg[i++] = readl(base + REG_MMU_INT_MAIN_CONTROL);
Nit: given that these are fairly arbitrary discontiguous registers so
you can't have a simple loop over the array, it might be clearer to
replace the array with an equivalent struct, e.g.:
struct mtk_iommu_suspend_regs {
u32 standard_axi_mode;
u32 dcm_dis;
...
}
...
data->reg.dcm_dis = readl(base + REG_MMU_DCM_DIS);
...
then since you refer to everything by name you don't have to worry about
the length and order of array elements if anything ever changes.
> + return 0;
> +}
> +
> +static int mtk_iommu_resume(struct device *dev)
> +{
> + struct mtk_iommu_domain *mtkdom = dev_get_drvdata(dev);
> + struct mtk_iommu_data *data = mtkdom->data;
> + void __iomem *base = data->base;
> + unsigned int i = 0;
> +
> + writel(data->reg[i++], base + REG_MMU_PT_BASE_ADDR);
> + writel(data->reg[i++], base + REG_MMU_STANDARD_AXI_MODE);
> + writel(data->reg[i++], base + REG_MMU_DCM_DIS);
> + writel(data->reg[i++], base + REG_MMU_CTRL_REG);
> + writel(data->reg[i++], base + REG_MMU_IVRP_PADDR);
> + writel(data->reg[i++], base + REG_MMU_INT_CONTROL0);
> + writel(data->reg[i++], base + REG_MMU_INT_MAIN_CONTROL);
> + return 0;
> +}
Other than that though, modulo the issues with the pagetable code I
think this part of the driver is shaping up quite nicely.
Robin.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Yong Wu <yong.wu@mediatek.com> |
|---|---|
| Date | 2015-07-29 07:50 +0200 |
| Message-ID | <pRspB-7rV-15@gated-at.bofh.it> |
| In reply to | #1193088 |
On Mon, 2015-07-27 at 16:49 +0100, Robin Murphy wrote:
> On 27/07/15 16:31, Russell King - ARM Linux wrote:
> > On Mon, Jul 27, 2015 at 02:23:26PM +0100, Robin Murphy wrote:
> >> On 16/07/15 10:04, Yong Wu wrote:
> >>> This patch adds support for mediatek m4u (MultiMedia Memory Management
> >>> Unit).
> >>>
> >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> >> [...]
> >>> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> >>> +{
> >>> + struct mtk_iommu_domain *domain = cookie;
> >>> + unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> >>> +
> >>> + dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> >>> + size, DMA_TO_DEVICE);
> >>
> >> Nit: this looks like it may as well be dma_map_single.
> >>
> >> It would probably be worth following it with a matching unmap too, just to
> >> avoid any possible leakage bugs (especially if this M4U ever appears in a
> >> SoC supporting RAM above the 32-bit boundary).
> >
> > Why not do the job properly? Take a look at how I implemented the
> > streaming DMA API on Tegra SMMU (patch set recently sent out earlier
> > today).
> >
> > There's no need for hacks like dma_map_page() (and discarding it's
> > return value) or dma_map_page() followed by dma_unmap_page().
>
> Indeed, as it happens I do have a branch where I prototyped that for the
> long-descriptor io-pgtable-arm code a while ago; this discussion has
> prompted me to dig it up again. Stay tuned, folks...
Hi Russell, Robin,
From I see in arm-smmu-v3.c in v4.2-rc1,
The flush_pgtable seems like this:
//==========
dma_addr_t dma_addr;
dma_addr = dma_map_page(dev, ptr, size, DMA_TO_DEVICE);
if (dma_mapping_error(dev, dma_addr))
dev_err(dev, "failed to flush pgtable at %p\n", ptr);
else
dma_unmap_page(dev, dma_addr, size, DMA_TO_DEVICE);
//==========
I will change map like this and use dma_map_single instead.
Is this also seems to be not proper?
Then how to do it? add this before unmap? :
dma_sync_single_for_device(dev, dma_addr, size, DMA_TO_DEVICE);
>
> Robin.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web