Re: [PATCH 4/5] iommu/mediatek: add support for mtk iommu generation one HW
hi, Robin, thanks very much for your comments.
On 5/10/2016 6:28 PM, Robin Murphy wrote:
> On 09/05/16 09:00, honghui.zh...@mediatek.com wrote:
> [...]
>> +static void *mtk_iommu_alloc_pgt(struct device *dev, size_t size, gfp_t gfp)
>> +{
>> +dma_addr_t dma;
>> +void *pages = alloc_pages_exact(size, gfp | __GFP_ZERO);
>> +
>> +if (!pages)
>> +return NULL;
>> +
>> +dma = dma_map_single(dev, pages, size, DMA_TO_DEVICE);
>> +if (dma_mapping_error(dev, dma))
>> +goto out_free;
>> +/*
>> + * We depend on the IOMMU being able to work with any physical
>> + * address directly, so if the DMA layer suggests otherwise by
>> + * translating or truncating them, that bodes very badly...
>> + */
>> +if (dma != virt_to_phys(pages))
>> +goto out_unmap;
>
> Given that you've only got a single table to allocate, and at 4MB it has a
> fair chance of failing beyond early boot time, just use dma_alloc_coherent()
> - you don't need to care about the dma <-> phys relationship because you
> don't have multi-level tables to walk. That way, you can get rid of all the
> awkward streaming DMA stuff, and also benefit from CMA to avoid allocation
> failures.
>
thanks, use dma_alloc_coherent is good for me.
>> +kmemleak_ignore(pages);
>> +return pages;
>> +
>> +out_unmap:
>> +dev_err(dev, "Cannot accommodate DMA translation for IOMMU page
>> tables\n");
>> +dma_unmap_single(dev, dma, size, DMA_TO_DEVICE);
>> +out_free:
>> +free_pages_exact(pages, size);
>> +return NULL;
>> +
>> +}
>> +
>> +static void mtk_iommu_free_pgt(struct device *dev, void *pages, size_t size)
>> +{
>> +dma_unmap_single(dev, (dma_addr_t)virt_to_phys(pages),
>> + size, DMA_TO_DEVICE);
>> +free_pages_exact(pages, size);
>> +}
>> +
>> +static int mtk_iommu_domain_finalise(struct mtk_iommu_data *data)
>> +{
>> +struct mtk_iommu_domain *dom = data->m4u_dom;
>> +
>> +spin_lock_init(&dom->pgtlock);
>> +
>> +dom->pgt_va = mtk_iommu_alloc_pgt(data->dev,
>> +dom->pgt_size, GFP_KERNEL);
>> +if (!dom->pgt_va)
>> +return -ENOMEM;
>> +
>> +dom->pgt_pa = virt_to_phys(dom->pgt_va);
>> +
>> +writel(dom->pgt_pa, data->base + REG_MMU_PT_BASE_ADDR);
>> +
>> +dom->cookie = (void *)data;
>> +
>> +return 0;
>> +}
>> +
>> +static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
>> +{
>> +struct mtk_iommu_domain *dom;
>> +
>> +if (type != IOMMU_DOMAIN_UNMANAGED)
>> +return NULL;
>> +
>> +dom = kzalloc(sizeof(*dom), GFP_KERNEL);
>> +if (!dom)
>> +return NULL;
>> +
>> +/*
>> + * MTK m4u support 4GB iova address space, and oly support 4K page
>> + * mapping. So the pagetable size should be exactly as 4M.
>> + */
>> +dom->pgt_size = SZ_4M;
>
> If the table size is fixed, then why bother having a variable at all?
>
>> +return &dom->domain;
>> +}
>> +
>> +static void mtk_iommu_domain_free(struct iommu_domain *domain)
>> +{
>> +kfree(to_mtk_domain(domain));
>> +}
>> +
>
> [...]
>
>> +static int mtk_iommu_map(struct iommu_domain *domain, unsigned long iova,
>> + phys_addr_t paddr, size_t size, int prot)
>> +{
>> +struct mtk_iommu_domain *dom = to_mtk_domain(domain);
>> +struct mtk_iommu_data *data = dom->cookie;
>> +unsigned int page_num = size >> MTK_IOMMU_PAGE_SHIFT;
>
> Since you only advertise a single page size, this will always be 1, so you
> could either get rid of the loop here...
>
I would prefer your following advise to change the pgsize_bitmap.
I would follow your advise in the next version.
Thanks very much.
>> +unsigned long flags;
>> +unsigned int i;
>> +u32 *pgt_base_iova;
>> +u32 pabase = (u32)paddr;
>> +int map_size = 0;
>> +
>> +spin_lock_irqsave(&dom->pgtlock, flags);
>> +pgt_base_iova = dom->pgt_va + (iova >> MTK_IOMMU_PAGE_SHIFT);
>> +for (i = 0; i < page_num; i++) {
>> +pgt_base_iova[i] = pabase | F_DESC_VALID | F_DESC_NONSEC;
>> +pabase += MTK_IOMMU_PAGE_SIZE;
>> +map_size += MTK_IOMMU_PAGE_SIZE;
>> +}
>> +dma_sync_single_for_device(data->dev,
>> +dom->pgt_pa + (iova >> MTK_IOMMU_PAGE_SHIFT),
>> +(size >> MTK_IOMMU_PAGE_SHIFT) * sizeof(u32),
>> +DMA_TO_DEVICE);
>> +spin_unlock_irqrestore(&dom->pgtlock, flags);
>> +
>> +mtk_iommu_tlb_flush_range(data, iova, size);
>> +
>> +return map_size;
>> +}
>
> [...]
>
>> +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,
>> +.remove_de
Re: [PATCH 1/5] dt-bindings: mediatek: add descriptions for mediatek mt2701 iommu and smi
On 5/10/2016 4:22 AM, Rob Herring wrote:
> On Mon, May 09, 2016 at 04:00:12PM +0800, honghui.zh...@mediatek.com wrote:
>> From: Honghui Zhang
>>
>> This patch defines the local arbitor port IDs for mediatek SoC MT2701 and
>> add descriptions of binding for mediatek generation one iommu and smi.
>>
>> Signed-off-by: Honghui Zhang
>> ---
>> .../devicetree/bindings/iommu/mediatek,iommu.txt | 13 +++-
>> .../memory-controllers/mediatek,smi-common.txt | 21 +-
>> .../memory-controllers/mediatek,smi-larb.txt | 4 +-
>> include/dt-bindings/memory/mt2701-larb-port.h | 85
>> ++
>> 4 files changed, 115 insertions(+), 8 deletions(-)
>> create mode 100644 include/dt-bindings/memory/mt2701-larb-port.h
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
>> b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
>> index cd1b1cd..9a4a5b5 100644
>> --- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
>> +++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
>> @@ -1,7 +1,9 @@
>> * Mediatek IOMMU Architecture Implementation
>>
>> - Some Mediatek SOCs contain a Multimedia Memory Management Unit (M4U) which
>> -uses the ARM Short-Descriptor translation table format for address
>> translation.
>> + Some Mediatek SOCs contain a Multimedia Memory Management Unit (M4U), and
>> +this M4U have two generations of HW architecture. Generation one use flat
>
> s/use/uses/
Hi, Rob,
Thank you very much, I will fix all of those in the next version.
>
>> +pagetable, and only support 4K size page mapping. Generation two uses the
>
> s/support/supports/
>
>> +ARM Short-Descriptor translation table format for address translation.
>>
>>About the M4U Hardware Block Diagram, please check below:
>>
>> @@ -36,7 +38,9 @@ in each larb. Take a example, There are many ports like
>> MC, PP, VLD in the
>> video decode local arbiter, all these ports are according to the video HW.
>>
>> Required properties:
>> -- compatible : must be "mediatek,mt8173-m4u".
>> +- compatible : must be one of the following string:
>> +"mediatek,mt2701-m4u" for mt2701 which use generation one m4u HW.
>> +"mediatek,mt8173-m4u" for mt8173 which use generation two m4u HW.
>> - reg : m4u register base and size.
>> - interrupts : the interrupt of m4u.
>> - clocks : must contain one entry for each clock-names.
>> @@ -46,7 +50,8 @@ Required properties:
>> according to the local arbiter index, like larb0, larb1, larb2...
>> - iommu-cells : must be 1. This is the mtk_m4u_id according to the HW.
>> Specifies the mtk_m4u_id as defined in
>> -dt-binding/memory/mt8173-larb-port.h.
>> +dt-binding/memory/mt2701-larb-port.h for mt2701 and
>> +dt-binding/memory/mt8173-larb-port.h for mt8173
>>
>> Example:
>> iommu: iommu@10205000 {
>> diff --git
>> a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
>>
>> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
>> index 06a83ce..80c0e22 100644
>> ---
>> a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
>> +++
>> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
>> @@ -2,16 +2,31 @@ SMI (Smart Multimedia Interface) Common
>>
>> The hardware block diagram please check bindings/iommu/mediatek,iommu.txt
>>
>> +Mediatek SMI have two generation HW architecture, mt8173 use the secondary
>
> s/generation/generations of/
> s/use/uses/
> s/secondary/second/
>
>> +generation of SMI HW while mt2701 use the first generation HW of SMI.
>
> s/use/uses/
>
>> +
>> +There's slight differences between the two SMI, for generation 2, the
>> +register which control the iommu port is at each larb's register base. But
>> +for generation 1, the register is at smi ao base(smi always on register
>> +base). Besides that, the smi async clock should be prepare and enabled for
>
> s/prepare/prepared/
>
>> +SMI generation 1 to transform the smi clock into emi clock domain, but no
>
> s/no/that is not/
>
>> +needed for SMI generation 2.
>> +
>> Required properties:
>> -- compatible : must be "mediatek,mt8173-smi-common"
>> +- compatible : must be one of :
>> +"mediatek,mt2701-smi-common"
>> +"mediatek,mt8173-smi-common"
>> - reg : the register and size of the SMI block.
>> - power-domains : a phandle to the power domain of this local arbiter.
>> - clocks : Must contain an entry for each entry in clock-names.
>> -- clock-names : must contain 2 entries, as follows:
>> +- clock-names : must contain 3 entries for generation 1 smi HW and 2 entries
>> + for generation 2 smi HW as follows:
>>- "apb" : Advanced Peripheral Bus clock, It's the clock for setting
>> the register.
>>- "smi" : It's the clock for transfer data and command.
>> - They may be the same if both source clocks are the same.
>> +They may be the same if both source clocks are the same.
>
