This series fixes some issues in rockchip iommu driver, and add of_iommu
support in it.
Changes in v5:
Use out labels to save the duplication between the error and success paths.
Use RK_MMU_POLL_PERIOD_US instead of 100.
Remove clk names.
Use clk_bulk APIs.
Avoid race about pm_runtime_get_if_in_use() and pm_runtime_enabled().
Changes in v4:
Rewrite commit message.
Changes in v3:
Also remove remove() and module_exit() as Tomasz suggested.
Loop platform_get_irq() as Robin suggested.
Add struct rk_iommudata.
Squash iommu/rockchip: Use iommu_group_get_for_dev() for add_device
Only call startup() and shutdown() when iommu attached.
Remove pm_mutex.
Check runtime PM disabled.
Check pm_runtime in rk_iommu_irq().
Remove rk_iommudata->domain.
Changes in v2:
Move irq request to probe(in patch[0])
Move bus_set_iommu() to rk_iommu_probe().
Jeffy Chen (9):
iommu/rockchip: Prohibit unbind and remove
iommu/rockchip: Fix error handling in probe
iommu/rockchip: Request irqs in rk_iommu_probe()
ARM: dts: rockchip: add clocks in vop iommu nodes
iommu/rockchip: Use IOMMU device for dma mapping operations
iommu/rockchip: Use OF_IOMMU to attach devices automatically
iommu/rockchip: Fix error handling in init
iommu/rockchip: Add runtime PM support
iommu/rockchip: Support sharing IOMMU between masters
Tomasz Figa (4):
iommu/rockchip: Fix error handling in attach
iommu/rockchip: Use iopoll helpers to wait for hardware
iommu/rockchip: Fix TLB flush of secondary IOMMUs
iommu/rockchip: Control clocks needed to access the IOMMU
.../devicetree/bindings/iommu/rockchip,iommu.txt | 8 +
arch/arm/boot/dts/rk3036.dtsi | 1 +
arch/arm/boot/dts/rk3288.dtsi | 2 +
drivers/iommu/rockchip-iommu.c | 618 +++++++++++----------
4 files changed, 343 insertions(+), 286 deletions(-)
--
2.11.0
Move request_irq to the end of rk_iommu_probe().
Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Jeffy Chen <redacted>
---
Changes in v5: None
Changes in v4: None
Changes in v3:
Loop platform_get_irq() as Robin suggested.
Changes in v2: None
drivers/iommu/rockchip-iommu.c | 38 +++++++++-----------------------------
1 file changed, 9 insertions(+), 29 deletions(-)
@@ -1138,7 +1126,7 @@ static int rk_iommu_probe(struct platform_device *pdev)structrk_iommu*iommu;structresource*res;intnum_res=pdev->num_resources;-interr,i;+interr,i,irq;iommu=devm_kzalloc(dev,sizeof(*iommu),GFP_KERNEL);if(!iommu)
@@ -1165,23 +1153,15 @@ static int rk_iommu_probe(struct platform_device *pdev)if(iommu->num_mmu==0)returnPTR_ERR(iommu->bases[0]);-iommu->num_irq=platform_irq_count(pdev);-if(iommu->num_irq<0)-returniommu->num_irq;-if(iommu->num_irq==0)-return-ENXIO;+i=0;+while((irq=platform_get_irq(pdev,i++))!=-ENXIO){+if(irq<0)+returnirq;-iommu->irq=devm_kcalloc(dev,iommu->num_irq,sizeof(*iommu->irq),-GFP_KERNEL);-if(!iommu->irq)-return-ENOMEM;--for(i=0;i<iommu->num_irq;i++){-iommu->irq[i]=platform_get_irq(pdev,i);-if(iommu->irq[i]<0){-dev_err(dev,"Failed to get IRQ, %d\n",iommu->irq[i]);-return-ENXIO;-}+err=devm_request_irq(iommu->dev,irq,rk_iommu_irq,+IRQF_SHARED,dev_name(dev),iommu);+if(err)+returnerr;}iommu->reset_disabled=device_property_read_bool(dev,
From: Tomasz Figa <tfiga@chromium.org>
Currently if the driver encounters an error while attaching device, it
will leave the IOMMU in an inconsistent state. Even though it shouldn't
really happen in reality, let's just add proper error path to keep
things consistent.
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Jeffy Chen <redacted>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
Changes in v5:
Use out labels to save the duplication between the error and success paths.
Changes in v4: None
Changes in v3: None
Changes in v2:
Move irq request to probe(in patch[0])
drivers/iommu/rockchip-iommu.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Tomasz Figa <tfiga@chromium.org>
This patch converts the rockchip-iommu driver to use the in-kernel
iopoll helpers to wait for certain status bits to change in registers
instead of an open-coded custom macro.
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Jeffy Chen <redacted>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
Changes in v5:
Use RK_MMU_POLL_PERIOD_US instead of 100.
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/iommu/rockchip-iommu.c | 75 ++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 36 deletions(-)
From: Tomasz Figa <tfiga@chromium.org>
Due to the bug in current code, only first IOMMU has the TLB lines
flushed in rk_iommu_zap_lines. This patch fixes the inner loop to
execute for all IOMMUs and properly flush the TLB.
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Jeffy Chen <redacted>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/iommu/rockchip-iommu.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
Add clocks in vop iommu nodes, since we are going to control clocks in
rockchip iommu driver.
Signed-off-by: Jeffy Chen <redacted>
---
Changes in v5:
Remove clk names.
Changes in v4: None
Changes in v3: None
Changes in v2: None
arch/arm/boot/dts/rk3036.dtsi | 1 +
arch/arm/boot/dts/rk3288.dtsi | 2 ++
2 files changed, 3 insertions(+)
Use the first registered IOMMU device for dma mapping operations, and
drop the domain platform device.
This is similar to exynos iommu driver.
Signed-off-by: Jeffy Chen <redacted>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/iommu/rockchip-iommu.c | 85 ++++++++++++------------------------------
1 file changed, 24 insertions(+), 61 deletions(-)
@@ -79,7 +79,6 @@structrk_iommu_domain{structlist_headiommus;-structplatform_device*pdev;u32*dt;/* page directory table */dma_addr_tdt_dma;spinlock_tiommus_lock;/* lock for iommus list */
@@ -100,12 +99,14 @@ struct rk_iommu {structiommu_domain*domain;/* domain to which iommu is attached */};+staticstructdevice*dma_dev;+staticinlinevoidrk_table_flush(structrk_iommu_domain*dom,dma_addr_tdma,unsignedintcount){size_tsize=count*sizeof(u32);/* count of u32 entry */-dma_sync_single_for_device(&dom->pdev->dev,dma,size,DMA_TO_DEVICE);+dma_sync_single_for_device(dma_dev,dma,size,DMA_TO_DEVICE);}staticstructrk_iommu_domain*to_rk_domain(structiommu_domain*dom)
@@ -938,29 +938,20 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,staticstructiommu_domain*rk_iommu_domain_alloc(unsignedtype){structrk_iommu_domain*rk_domain;-structplatform_device*pdev;-structdevice*iommu_dev;if(type!=IOMMU_DOMAIN_UNMANAGED&&type!=IOMMU_DOMAIN_DMA)returnNULL;-/* Register a pdev per domain, so DMA API can base on this *dev-*evensomevirtualmasterdoesn'thaveaniommuslave-*/-pdev=platform_device_register_simple("rk_iommu_domain",-PLATFORM_DEVID_AUTO,NULL,0);-if(IS_ERR(pdev))+if(!dma_dev)returnNULL;-rk_domain=devm_kzalloc(&pdev->dev,sizeof(*rk_domain),GFP_KERNEL);+rk_domain=devm_kzalloc(dma_dev,sizeof(*rk_domain),GFP_KERNEL);if(!rk_domain)-gotoerr_unreg_pdev;--rk_domain->pdev=pdev;+returnNULL;if(type==IOMMU_DOMAIN_DMA&&iommu_get_dma_cookie(&rk_domain->domain))-gotoerr_unreg_pdev;+returnNULL;/**rk32xxiommususea2levelpagetable.
@@ -971,11 +962,10 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)if(!rk_domain->dt)gotoerr_put_cookie;-iommu_dev=&pdev->dev;-rk_domain->dt_dma=dma_map_single(iommu_dev,rk_domain->dt,+rk_domain->dt_dma=dma_map_single(dma_dev,rk_domain->dt,SPAGE_SIZE,DMA_TO_DEVICE);-if(dma_mapping_error(iommu_dev,rk_domain->dt_dma)){-dev_err(iommu_dev,"DMA map error for DT\n");+if(dma_mapping_error(dma_dev,rk_domain->dt_dma)){+dev_err(dma_dev,"DMA map error for DT\n");gotoerr_free_dt;}
@@ -1150,30 +1136,6 @@ static const struct iommu_ops rk_iommu_ops = {.pgsize_bitmap=RK_IOMMU_PGSIZE_BITMAP,};-staticintrk_iommu_domain_probe(structplatform_device*pdev)-{-structdevice*dev=&pdev->dev;--dev->dma_parms=devm_kzalloc(dev,sizeof(*dev->dma_parms),GFP_KERNEL);-if(!dev->dma_parms)-return-ENOMEM;--/* Set dma_ops for dev, otherwise it would be dummy_dma_ops */-arch_setup_dma_ops(dev,0,DMA_BIT_MASK(32),NULL,false);--dma_set_max_seg_size(dev,DMA_BIT_MASK(32));-dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK(32));--return0;-}--staticstructplatform_driverrk_iommu_domain_driver={-.probe=rk_iommu_domain_probe,-.driver={-.name="rk_iommu_domain",-},-};-staticintrk_iommu_probe(structplatform_device*pdev){structdevice*dev=&pdev->dev;
@@ -1238,6 +1200,14 @@ static int rk_iommu_probe(struct platform_device *pdev)if(err)gotoerr_remove_sysfs;+/*+*UsethefirstregisteredIOMMUdevicefordomaintousewithDMA+*API,sinceadomainmightnotphysicallycorrespondtoasingle+*IOMMUdevice..+*/+if(!dma_dev)+dma_dev=&pdev->dev;+return0;err_remove_sysfs:iommu_device_sysfs_remove(&iommu->iommu);
@@ -1278,14 +1248,7 @@ static int __init rk_iommu_init(void)if(ret)returnret;-ret=platform_driver_register(&rk_iommu_domain_driver);-if(ret)-returnret;--ret=platform_driver_register(&rk_iommu_driver);-if(ret)-platform_driver_unregister(&rk_iommu_domain_driver);-returnret;+returnplatform_driver_register(&rk_iommu_driver);}subsys_initcall(rk_iommu_init);
Converts the rockchip-iommu driver to use the OF_IOMMU infrastructure,
which allows attaching master devices to their IOMMUs automatically
according to DT properties.
Signed-off-by: Jeffy Chen <redacted>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3:
Add struct rk_iommudata.
Squash iommu/rockchip: Use iommu_group_get_for_dev() for add_device
Changes in v2: None
drivers/iommu/rockchip-iommu.c | 135 ++++++++++++-----------------------------
1 file changed, 40 insertions(+), 95 deletions(-)
@@ -99,6 +100,10 @@ struct rk_iommu {structiommu_domain*domain;/* domain to which iommu is attached */};+structrk_iommudata{+structrk_iommu*iommu;+};+staticstructdevice*dma_dev;staticinlinevoidrk_table_flush(structrk_iommu_domain*dom,dma_addr_tdma,
@@ -1016,110 +1012,53 @@ static void rk_iommu_domain_free(struct iommu_domain *domain)iommu_put_dma_cookie(&rk_domain->domain);}-staticboolrk_iommu_is_dev_iommu_master(structdevice*dev)-{-structdevice_node*np=dev->of_node;-intret;--/*-*Aniommumasterhasaniommuspropertycontainingalistofphandles-*toiommunodes,eachwithan#iommu-cellspropertywithvalue0.-*/-ret=of_count_phandle_with_args(np,"iommus","#iommu-cells");-return(ret>0);-}--staticintrk_iommu_group_set_iommudata(structiommu_group*group,-structdevice*dev)+staticintrk_iommu_add_device(structdevice*dev){-structdevice_node*np=dev->of_node;-structplatform_device*pd;-intret;-structof_phandle_argsargs;+structiommu_group*group;+structrk_iommu*iommu;-/*-*Aniommumasterhasaniommuspropertycontainingalistofphandles-*toiommunodes,eachwithan#iommu-cellspropertywithvalue0.-*/-ret=of_parse_phandle_with_args(np,"iommus","#iommu-cells",0,-&args);-if(ret){-dev_err(dev,"of_parse_phandle_with_args(%pOF) => %d\n",-np,ret);-returnret;-}-if(args.args_count!=0){-dev_err(dev,"incorrect number of iommu params found for %pOF (found %d, expected 0)\n",-args.np,args.args_count);-return-EINVAL;-}+iommu=rk_iommu_from_dev(dev);+if(!iommu)+return-ENODEV;-pd=of_find_device_by_node(args.np);-of_node_put(args.np);-if(!pd){-dev_err(dev,"iommu %pOF not found\n",args.np);-return-EPROBE_DEFER;-}+group=iommu_group_get_for_dev(dev);+if(IS_ERR(group))+returnPTR_ERR(group);+iommu_group_put(group);-/* TODO(djkurtz): handle multiple slave iommus for a single master */-iommu_group_set_iommudata(group,&pd->dev,NULL);+iommu_device_link(&iommu->iommu,dev);return0;}-staticintrk_iommu_add_device(structdevice*dev)+staticvoidrk_iommu_remove_device(structdevice*dev){-structiommu_group*group;structrk_iommu*iommu;-intret;--if(!rk_iommu_is_dev_iommu_master(dev))-return-ENODEV;--group=iommu_group_get(dev);-if(!group){-group=iommu_group_alloc();-if(IS_ERR(group)){-dev_err(dev,"Failed to allocate IOMMU group\n");-returnPTR_ERR(group);-}-}--ret=iommu_group_add_device(group,dev);-if(ret)-gotoerr_put_group;--ret=rk_iommu_group_set_iommudata(group,dev);-if(ret)-gotoerr_remove_device;iommu=rk_iommu_from_dev(dev);-if(iommu)-iommu_device_link(&iommu->iommu,dev);-iommu_group_put(group);--return0;--err_remove_device:+iommu_device_unlink(&iommu->iommu,dev);iommu_group_remove_device(dev);-err_put_group:-iommu_group_put(group);-returnret;}-staticvoidrk_iommu_remove_device(structdevice*dev)+staticintrk_iommu_of_xlate(structdevice*dev,+structof_phandle_args*args){-structrk_iommu*iommu;+structplatform_device*iommu_dev;+structrk_iommudata*data;-if(!rk_iommu_is_dev_iommu_master(dev))-return;+data=devm_kzalloc(dma_dev,sizeof(*data),GFP_KERNEL);+if(!data)+return-ENOMEM;-iommu=rk_iommu_from_dev(dev);-if(iommu)-iommu_device_unlink(&iommu->iommu,dev);+iommu_dev=of_find_device_by_node(args->np);-iommu_group_remove_device(dev);+data->iommu=platform_get_drvdata(iommu_dev);+dev->archdata.iommu=data;++of_dev_put(iommu_dev);++return0;}staticconststructiommu_opsrk_iommu_ops={
@@ -1196,6 +1137,8 @@ static int rk_iommu_probe(struct platform_device *pdev)gotoerr_unprepare_clocks;iommu_device_set_ops(&iommu->iommu,&rk_iommu_ops);+iommu_device_set_fwnode(&iommu->iommu,&dev->of_node->fwnode);+err=iommu_device_register(&iommu->iommu);if(err)gotoerr_remove_sysfs;
@@ -1252,6 +1195,8 @@ static int __init rk_iommu_init(void)}subsys_initcall(rk_iommu_init);+IOMMU_OF_DECLARE(rk_iommu_of,"rockchip,iommu");+MODULE_DESCRIPTION("IOMMU API for Rockchip");MODULE_AUTHOR("Simon Xue <xxm@rock-chips.com> and Daniel Kurtz <djkurtz@chromium.org>");MODULE_ALIAS("platform:rockchip-iommu");
It's hard to undo bus_set_iommu() in the error path, so move it to the
end of rk_iommu_probe().
Signed-off-by: Jeffy Chen <redacted>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
Move bus_set_iommu() to rk_iommu_probe().
drivers/iommu/rockchip-iommu.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
When the power domain is powered off, the IOMMU cannot be accessed and
register programming must be deferred until the power domain becomes
enabled.
Add runtime PM support, and use runtime PM device link from IOMMU to
master to startup and shutdown IOMMU.
Signed-off-by: Jeffy Chen <redacted>
---
Changes in v5:
Avoid race about pm_runtime_get_if_in_use() and pm_runtime_enabled().
Changes in v4: None
Changes in v3:
Only call startup() and shutdown() when iommu attached.
Remove pm_mutex.
Check runtime PM disabled.
Check pm_runtime in rk_iommu_irq().
Changes in v2: None
drivers/iommu/rockchip-iommu.c | 181 +++++++++++++++++++++++++++++++----------
1 file changed, 140 insertions(+), 41 deletions(-)
@@ -638,10 +648,20 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain,spin_lock_irqsave(&rk_domain->iommus_lock,flags);list_for_each(pos,&rk_domain->iommus){structrk_iommu*iommu;+intret;+iommu=list_entry(pos,structrk_iommu,node);-WARN_ON(clk_bulk_enable(iommu->num_clocks,iommu->clocks));-rk_iommu_zap_lines(iommu,iova,size);-clk_bulk_disable(iommu->num_clocks,iommu->clocks);++/* Only zap TLBs of IOMMUs that are powered on. */+ret=pm_runtime_get_if_in_use(iommu->dev);+if(ret>0||ret==-EINVAL){+WARN_ON(clk_bulk_enable(iommu->num_clocks,+iommu->clocks));+rk_iommu_zap_lines(iommu,iova,size);+clk_bulk_disable(iommu->num_clocks,iommu->clocks);+}+if(ret>0)+pm_runtime_put(iommu->dev);}spin_unlock_irqrestore(&rk_domain->iommus_lock,flags);}
@@ -844,22 +864,30 @@ static struct rk_iommu *rk_iommu_from_dev(struct device *dev)returndata?data->iommu:NULL;}-staticintrk_iommu_attach_device(structiommu_domain*domain,-structdevice*dev)+/* Must be called with iommu powered on and attached */+staticvoidrk_iommu_shutdown(structrk_iommu*iommu){-structrk_iommu*iommu;+inti;++/* Ignore error while disabling, just keep going */+WARN_ON(clk_bulk_enable(iommu->num_clocks,iommu->clocks));+rk_iommu_enable_stall(iommu);+rk_iommu_disable_paging(iommu);+for(i=0;i<iommu->num_mmu;i++){+rk_iommu_write(iommu->bases[i],RK_MMU_INT_MASK,0);+rk_iommu_write(iommu->bases[i],RK_MMU_DTE_ADDR,0);+}+rk_iommu_disable_stall(iommu);+clk_bulk_disable(iommu->num_clocks,iommu->clocks);+}++/* Must be called with iommu powered on and attached */+staticintrk_iommu_startup(structrk_iommu*iommu)+{+structiommu_domain*domain=iommu->domain;structrk_iommu_domain*rk_domain=to_rk_domain(domain);-unsignedlongflags;intret,i;-/*-*Allow'virtualdevices'(e.g.,drm)toattachtodomain.-*Suchadevicedoesnotbelongtoaniommugroup.-*/-iommu=rk_iommu_from_dev(dev);-if(!iommu)-return0;-ret=clk_bulk_enable(iommu->num_clocks,iommu->clocks);if(ret)returnret;
@@ -872,8 +900,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,if(ret)gotoout_disable_stall;-iommu->domain=domain;-for(i=0;i<iommu->num_mmu;i++){rk_iommu_write(iommu->bases[i],RK_MMU_DTE_ADDR,rk_domain->dt_dma);
@@ -882,14 +908,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,}ret=rk_iommu_enable_paging(iommu);-if(ret)-gotoout_disable_stall;--spin_lock_irqsave(&rk_domain->iommus_lock,flags);-list_add_tail(&iommu->node,&rk_domain->iommus);-spin_unlock_irqrestore(&rk_domain->iommus_lock,flags);--dev_dbg(dev,"Attached to iommu domain\n");out_disable_stall:rk_iommu_disable_stall(iommu);
@@ -904,31 +922,76 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,structrk_iommu*iommu;structrk_iommu_domain*rk_domain=to_rk_domain(domain);unsignedlongflags;-inti;+intret;/* Allow 'virtual devices' (eg drm) to detach from domain */iommu=rk_iommu_from_dev(dev);if(!iommu)return;+dev_dbg(dev,"Detaching from iommu domain\n");++/* iommu already detached */+if(iommu->domain!=domain)+return;++iommu->domain=NULL;+spin_lock_irqsave(&rk_domain->iommus_lock,flags);list_del_init(&iommu->node);spin_unlock_irqrestore(&rk_domain->iommus_lock,flags);-/* Ignore error while disabling, just keep going */-WARN_ON(clk_bulk_enable(iommu->num_clocks,iommu->clocks));-rk_iommu_enable_stall(iommu);-rk_iommu_disable_paging(iommu);-for(i=0;i<iommu->num_mmu;i++){-rk_iommu_write(iommu->bases[i],RK_MMU_INT_MASK,0);-rk_iommu_write(iommu->bases[i],RK_MMU_DTE_ADDR,0);-}-rk_iommu_disable_stall(iommu);-clk_bulk_disable(iommu->num_clocks,iommu->clocks);+ret=pm_runtime_get_if_in_use(iommu->dev);+if(ret>0||ret==-EINVAL)+rk_iommu_shutdown(iommu);+if(ret>0)+pm_runtime_put(iommu->dev);+}-iommu->domain=NULL;+staticintrk_iommu_attach_device(structiommu_domain*domain,+structdevice*dev)+{+structrk_iommu*iommu;+structrk_iommu_domain*rk_domain=to_rk_domain(domain);+unsignedlongflags;+intret,need_runtime_put;++/*+*Allow'virtualdevices'(e.g.,drm)toattachtodomain.+*Suchadevicedoesnotbelongtoaniommugroup.+*/+iommu=rk_iommu_from_dev(dev);+if(!iommu)+return0;++dev_dbg(dev,"Attaching to iommu domain\n");++/* iommu already attached */+if(iommu->domain==domain)+return0;++if(iommu->domain)+rk_iommu_detach_device(iommu->domain,dev);++iommu->domain=domain;++spin_lock_irqsave(&rk_domain->iommus_lock,flags);+list_add_tail(&iommu->node,&rk_domain->iommus);+spin_unlock_irqrestore(&rk_domain->iommus_lock,flags);++ret=pm_runtime_get_if_in_use(iommu->dev);+if(ret<=0&&ret!=-EINVAL)+return0;+need_runtime_put=ret>0;++ret=rk_iommu_startup(iommu);+if(ret)+rk_iommu_detach_device(iommu->domain,dev);++if(need_runtime_put)+pm_runtime_put(iommu->dev);-dev_dbg(dev,"Detached from iommu domain\n");+returnret;}staticstructiommu_domain*rk_iommu_domain_alloc(unsignedtype)
@@ -1016,17 +1079,21 @@ static int rk_iommu_add_device(struct device *dev){structiommu_group*group;structrk_iommu*iommu;+structrk_iommudata*data;-iommu=rk_iommu_from_dev(dev);-if(!iommu)+data=dev->archdata.iommu;+if(!data)return-ENODEV;+iommu=rk_iommu_from_dev(dev);+group=iommu_group_get_for_dev(dev);if(IS_ERR(group))returnPTR_ERR(group);iommu_group_put(group);iommu_device_link(&iommu->iommu,dev);+data->link=device_link_add(dev,iommu->dev,DL_FLAG_PM_RUNTIME);return0;}
@@ -1034,9 +1101,11 @@ static int rk_iommu_add_device(struct device *dev)staticvoidrk_iommu_remove_device(structdevice*dev){structrk_iommu*iommu;+structrk_iommudata*data=dev->archdata.iommu;iommu=rk_iommu_from_dev(dev);+device_link_del(data->link);iommu_device_unlink(&iommu->iommu,dev);iommu_group_remove_device(dev);}
@@ -1153,6 +1222,8 @@ static int rk_iommu_probe(struct platform_device *pdev)bus_set_iommu(&platform_bus_type,&rk_iommu_ops);+pm_runtime_enable(dev);+return0;err_remove_sysfs:iommu_device_sysfs_remove(&iommu->iommu);
There would be some masters sharing the same IOMMU device. Put them in
the same iommu group and share the same iommu domain.
Signed-off-by: Jeffy Chen <redacted>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3:
Remove rk_iommudata->domain.
Changes in v2: None
drivers/iommu/rockchip-iommu.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
@@ -99,6 +99,7 @@ struct rk_iommu {structiommu_deviceiommu;structlist_headnode;/* entry in rk_iommu_domain.iommus */structiommu_domain*domain;/* domain to which iommu is attached */+structiommu_group*group;};structrk_iommudata{
From: Tomasz Figa <tfiga@chromium.org>
Current code relies on master driver enabling necessary clocks before
IOMMU is accessed, however there are cases when the IOMMU should be
accessed while the master is not running yet, for example allocating
V4L2 videobuf2 buffers, which is done by the VB2 framework using DMA
mapping API and doesn't engage the master driver at all.
This patch fixes the problem by letting clocks needed for IOMMU
operation to be listed in Device Tree and making the driver enable them
for the time of accessing the hardware.
Signed-off-by: Jeffy Chen <redacted>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
Changes in v5:
Use clk_bulk APIs.
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../devicetree/bindings/iommu/rockchip,iommu.txt | 8 +++
drivers/iommu/rockchip-iommu.c | 74 ++++++++++++++++++++--
2 files changed, 76 insertions(+), 6 deletions(-)
@@ -14,6 +14,13 @@ Required properties: "single-master" device, and needs no additional information to associate with its master device. See: Documentation/devicetree/bindings/iommu/iommu.txt+Optional properties:+- clocks : A list of master clocks requires for the IOMMU to be accessible+ by the host CPU. The number of clocks depends on the master+ block and might as well be zero. See [1] for generic clock+ bindings description.++[1] Documentation/devicetree/bindings/clock/clock-bindings.txt Optional properties: - rockchip,disable-mmu-reset : Don't use the mmu reset operation.
Removal of IOMMUs cannot be done reliably.
This is similar to exynos iommu driver.
Signed-off-by: Jeffy Chen <redacted>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
Changes in v5: None
Changes in v4:
Rewrite commit message.
Changes in v3:
Also remove remove() and module_exit() as Tomasz suggested.
Changes in v2: None
drivers/iommu/rockchip-iommu.c | 21 +--------------------
1 file changed, 1 insertion(+), 20 deletions(-)
@@ -1248,14 +1236,7 @@ static int __init rk_iommu_init(void)platform_driver_unregister(&rk_iommu_domain_driver);returnret;}-staticvoid__exitrk_iommu_exit(void)-{-platform_driver_unregister(&rk_iommu_driver);-platform_driver_unregister(&rk_iommu_domain_driver);-}-subsys_initcall(rk_iommu_init);-module_exit(rk_iommu_exit);MODULE_DESCRIPTION("IOMMU API for Rockchip");MODULE_AUTHOR("Simon Xue <xxm@rock-chips.com> and Daniel Kurtz <djkurtz@chromium.org>");
From: Robin Murphy <robin.murphy@arm.com> Date: 2018-01-24 13:49:44
On 24/01/18 10:35, Jeffy Chen wrote:
quoted hunk
From: Tomasz Figa <tfiga@chromium.org>
Current code relies on master driver enabling necessary clocks before
IOMMU is accessed, however there are cases when the IOMMU should be
accessed while the master is not running yet, for example allocating
V4L2 videobuf2 buffers, which is done by the VB2 framework using DMA
mapping API and doesn't engage the master driver at all.
This patch fixes the problem by letting clocks needed for IOMMU
operation to be listed in Device Tree and making the driver enable them
for the time of accessing the hardware.
Signed-off-by: Jeffy Chen <redacted>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
Changes in v5:
Use clk_bulk APIs.
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../devicetree/bindings/iommu/rockchip,iommu.txt | 8 +++
drivers/iommu/rockchip-iommu.c | 74 ++++++++++++++++++++--
2 files changed, 76 insertions(+), 6 deletions(-)
@@ -14,6 +14,13 @@ Required properties: "single-master" device, and needs no additional information to associate with its master device. See: Documentation/devicetree/bindings/iommu/iommu.txt+Optional properties:+- clocks : A list of master clocks requires for the IOMMU to be accessible
s/requires/required/
+ by the host CPU. The number of clocks depends on the master
+ block and might as well be zero. See [1] for generic clock
Oops, some subtleties of English here :)
To say "the number of clocks ... might as well be zero" effectively
implies "there's no point ever specifying any clocks". I guess what you
really mean here is "...might well be...", i.e. it is both valid and
reasonably likely to require zero clocks.
quoted hunk
+ bindings description.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
Optional properties:
- rockchip,disable-mmu-reset : Don't use the mmu reset operation.
@@ -450,6 +453,38 @@ static int rk_iommu_force_reset(struct rk_iommu *iommu)return0;}+staticintrk_iommu_of_get_clocks(structrk_iommu*iommu)+{+structdevice_node*np=iommu->dev->of_node;+intret;+inti;++ret=of_count_phandle_with_args(np,"clocks","#clock-cells");+if(ret==-ENOENT)+return0;+elseif(ret<0)+returnret;++iommu->num_clocks=ret;+iommu->clocks=devm_kcalloc(iommu->dev,iommu->num_clocks,+sizeof(*iommu->clocks),GFP_KERNEL);+if(!iommu->clocks)+return-ENOMEM;++for(i=0;i<iommu->num_clocks;++i){+iommu->clocks[i].clk=of_clk_get(np,i);+if(IS_ERR(iommu->clocks[i].clk)){+ret=PTR_ERR(iommu->clocks[i].clk);+gotoerr_clk_put;+}+}
Just to confirm my understanding from a quick scan through the code, the
reason we can't use clk_bulk_get() here is that currently, clocks[i].id
being NULL means we'd end up just getting the first clock multiple
times, right?
I guess there could be other users who also want "just get whatever
clocks I have" functionality, so it might be worth proposing that for
the core API as a separate/follow-up patch, but it definitely doesn't
need to be part of this series.
I really don't know enough about correct clk API usage, but modulo the
binding comments it certainly looks nice and tidy now;
Acked-by: Robin Murphy <robin.murphy@arm.com>
Thanks,
Robin.
@@ -823,10 +864,14 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, if (!iommu) return 0;- ret = rk_iommu_enable_stall(iommu);+ ret = clk_bulk_enable(iommu->num_clocks, iommu->clocks); if (ret) return ret;+ ret = rk_iommu_enable_stall(iommu);+ if (ret)+ goto out_disable_clocks;+ ret = rk_iommu_force_reset(iommu); if (ret) goto out_disable_stall;
Hi Robin,
Thanks for your reply.
On 01/24/2018 09:49 PM, Robin Murphy wrote:
quoted
+Optional properties:
+- clocks : A list of master clocks requires for the IOMMU to be
accessible
s/requires/required/
ok
quoted
+ by the host CPU. The number of clocks depends on the master
+ block and might as well be zero. See [1] for generic clock
Oops, some subtleties of English here :)
To say "the number of clocks ... might as well be zero" effectively
implies "there's no point ever specifying any clocks". I guess what you
really mean here is "...might well be...", i.e. it is both valid and
reasonably likely to require zero clocks.
ok
quoted
+ bindings description.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
Optional properties:
- rockchip,disable-mmu-reset : Don't use the mmu reset operation.
@@ -450,6 +453,38 @@ static int rk_iommu_force_reset(struct rk_iommu
*iommu)
return 0;
}
+static int rk_iommu_of_get_clocks(struct rk_iommu *iommu)
+{
+ struct device_node *np = iommu->dev->of_node;
+ int ret;
+ int i;
+
+ ret = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+ if (ret == -ENOENT)
+ return 0;
+ else if (ret < 0)
+ return ret;
+
+ iommu->num_clocks = ret;
+ iommu->clocks = devm_kcalloc(iommu->dev, iommu->num_clocks,
+ sizeof(*iommu->clocks), GFP_KERNEL);
+ if (!iommu->clocks)
+ return -ENOMEM;
+
+ for (i = 0; i < iommu->num_clocks; ++i) {
+ iommu->clocks[i].clk = of_clk_get(np, i);
+ if (IS_ERR(iommu->clocks[i].clk)) {
+ ret = PTR_ERR(iommu->clocks[i].clk);
+ goto err_clk_put;
+ }
+ }
Just to confirm my understanding from a quick scan through the code, the
reason we can't use clk_bulk_get() here is that currently, clocks[i].id
being NULL means we'd end up just getting the first clock multiple
times, right?
right, without a valid name, it would return the first clock.
/* Walk up the tree of devices looking for a clock that matches */
while (np) {
int index = 0;
/*
* For named clocks, first look up the name in the
* "clock-names" property. If it cannot be found, then
* index will be an error code, and of_clk_get() will fail.
*/
if (name)
index = of_property_match_string(np, "clock-names", name);
clk = __of_clk_get(np, index, dev_id, name);
I guess there could be other users who also want "just get whatever
clocks I have" functionality, so it might be worth proposing that for
the core API as a separate/follow-up patch, but it definitely doesn't
need to be part of this series.
right, i can try to do it later :)
I really don't know enough about correct clk API usage, but modulo the
binding comments it certainly looks nice and tidy now;
Acked-by: Robin Murphy <robin.murphy@arm.com>
From: Rob Herring <robh@kernel.org> Date: 2018-01-30 17:05:19
On Wed, Jan 24, 2018 at 06:35:11PM +0800, Jeffy Chen wrote:
From: Tomasz Figa <tfiga@chromium.org>
Current code relies on master driver enabling necessary clocks before
IOMMU is accessed, however there are cases when the IOMMU should be
accessed while the master is not running yet, for example allocating
V4L2 videobuf2 buffers, which is done by the VB2 framework using DMA
mapping API and doesn't engage the master driver at all.
This patch fixes the problem by letting clocks needed for IOMMU
operation to be listed in Device Tree and making the driver enable them
for the time of accessing the hardware.
Signed-off-by: Jeffy Chen <redacted>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
Changes in v5:
Use clk_bulk APIs.
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../devicetree/bindings/iommu/rockchip,iommu.txt | 8 +++
@@ -14,6 +14,13 @@ Required properties: "single-master" device, and needs no additional information to associate with its master device. See: Documentation/devicetree/bindings/iommu/iommu.txt+Optional properties:+- clocks : A list of master clocks requires for the IOMMU to be accessible+ by the host CPU. The number of clocks depends on the master+ block and might as well be zero. See [1] for generic clock+ bindings description.
Hardware blocks don't have a variable number of clock connections. This
needs to be a defined number of clocks (per compatible string if there
are different implementations with different # of clocks).
quoted hunk
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
Optional properties:
- rockchip,disable-mmu-reset : Don't use the mmu reset operation.
From: Tomasz Figa <tfiga@chromium.org> Date: 2018-01-31 07:53:11
Hi Rob,
On Wed, Jan 31, 2018 at 2:05 AM, Rob Herring [off-list ref] wrote:
On Wed, Jan 24, 2018 at 06:35:11PM +0800, Jeffy Chen wrote:
quoted
From: Tomasz Figa <tfiga@chromium.org>
Current code relies on master driver enabling necessary clocks before
IOMMU is accessed, however there are cases when the IOMMU should be
accessed while the master is not running yet, for example allocating
V4L2 videobuf2 buffers, which is done by the VB2 framework using DMA
mapping API and doesn't engage the master driver at all.
This patch fixes the problem by letting clocks needed for IOMMU
operation to be listed in Device Tree and making the driver enable them
for the time of accessing the hardware.
Signed-off-by: Jeffy Chen <redacted>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
Changes in v5:
Use clk_bulk APIs.
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../devicetree/bindings/iommu/rockchip,iommu.txt | 8 +++
@@ -14,6 +14,13 @@ Required properties: "single-master" device, and needs no additional information to associate with its master device. See: Documentation/devicetree/bindings/iommu/iommu.txt+Optional properties:+- clocks : A list of master clocks requires for the IOMMU to be accessible+ by the host CPU. The number of clocks depends on the master+ block and might as well be zero. See [1] for generic clock+ bindings description.
Hardware blocks don't have a variable number of clock connections.
I think you underestimate the imagination of hardware designers. :)
For Rockchip IOMMU, there is a set of clocks, which all need to be
enabled for IOMMU register access to succeed. The clocks are not
directly fed to the IOMMU, but they are needed for the various buses
and intermediate blocks on the way to the IOMMU to work.
And the set varies based on next to which master block the IOMMU block
is located, because the hierarchy of buses and intermediate blocks is
different.
Best regards,
Tomasz
From: Rob Herring <robh@kernel.org> Date: 2018-01-31 13:51:23
On Wed, Jan 31, 2018 at 1:52 AM, Tomasz Figa [off-list ref] wrote:
Hi Rob,
On Wed, Jan 31, 2018 at 2:05 AM, Rob Herring [off-list ref] wrote:
quoted
On Wed, Jan 24, 2018 at 06:35:11PM +0800, Jeffy Chen wrote:
quoted
From: Tomasz Figa <tfiga@chromium.org>
Current code relies on master driver enabling necessary clocks before
IOMMU is accessed, however there are cases when the IOMMU should be
accessed while the master is not running yet, for example allocating
V4L2 videobuf2 buffers, which is done by the VB2 framework using DMA
mapping API and doesn't engage the master driver at all.
This patch fixes the problem by letting clocks needed for IOMMU
operation to be listed in Device Tree and making the driver enable them
for the time of accessing the hardware.
Signed-off-by: Jeffy Chen <redacted>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
Changes in v5:
Use clk_bulk APIs.
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../devicetree/bindings/iommu/rockchip,iommu.txt | 8 +++
@@ -14,6 +14,13 @@ Required properties: "single-master" device, and needs no additional information to associate with its master device. See: Documentation/devicetree/bindings/iommu/iommu.txt+Optional properties:+- clocks : A list of master clocks requires for the IOMMU to be accessible+ by the host CPU. The number of clocks depends on the master+ block and might as well be zero. See [1] for generic clock+ bindings description.
Hardware blocks don't have a variable number of clock connections.
I think you underestimate the imagination of hardware designers. :)
Learned long ago to never do that. If there are 2 ways to do
something, they will find a 3rd way.
For Rockchip IOMMU, there is a set of clocks, which all need to be
enabled for IOMMU register access to succeed. The clocks are not
directly fed to the IOMMU, but they are needed for the various buses
and intermediate blocks on the way to the IOMMU to work.
The binding should describe the clock connections, not what clocks a
driver needs (currently). It sounds like a lack of managing bus clocks
to me.
In any case, the binding must be written so it can be verified. If you
can have any number of clocks with any names, there's no point in
documenting.
Rob
From: Tomasz Figa <tfiga@chromium.org>
Current code relies on master driver enabling necessary clocks before
IOMMU is accessed, however there are cases when the IOMMU should be
accessed while the master is not running yet, for example allocating
V4L2 videobuf2 buffers, which is done by the VB2 framework using DMA
mapping API and doesn't engage the master driver at all.
This patch fixes the problem by letting clocks needed for IOMMU
operation to be listed in Device Tree and making the driver enable them
for the time of accessing the hardware.
Signed-off-by: Jeffy Chen <redacted>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
Changes in v5:
Use clk_bulk APIs.
Changes in v4: None
Changes in v3: None
Changes in v2: None
[snip]
quoted
? +static int rk_iommu_of_get_clocks(struct rk_iommu *iommu)
+{
+??? struct device_node *np = iommu->dev->of_node;
+??? int ret;
+??? int i;
+
+??? ret = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+??? if (ret == -ENOENT)
+??????? return 0;
+??? else if (ret < 0)
+??????? return ret;
+
+??? iommu->num_clocks = ret;
+??? iommu->clocks = devm_kcalloc(iommu->dev, iommu->num_clocks,
+???????????????????? sizeof(*iommu->clocks), GFP_KERNEL);
+??? if (!iommu->clocks)
+??????? return -ENOMEM;
+
+??? for (i = 0; i < iommu->num_clocks; ++i) {
+??????? iommu->clocks[i].clk = of_clk_get(np, i);
+??????? if (IS_ERR(iommu->clocks[i].clk)) {
+??????????? ret = PTR_ERR(iommu->clocks[i].clk);
+??????????? goto err_clk_put;
+??????? }
+??? }
Just to confirm my understanding from a quick scan through the code,
the reason we can't use clk_bulk_get() here is that currently,
clocks[i].id being NULL means we'd end up just getting the first clock
multiple times, right?
I guess there could be other users who also want "just get whatever
clocks I have" functionality, so it might be worth proposing that for
the core API as a separate/follow-up patch, but it definitely doesn't
need to be part of this series.
Just to understand. Is it okay to make the driver "just get whatever
clocks device node gives"?
Doesn't the driver need to be aware of which all clocks are supposed to
be obtained and enabled
?It's should good for debug to let the world know which clock we failed
to get.
regards
Vivek
I really don't know enough about correct clk API usage, but modulo the
binding comments it certainly looks nice and tidy now;
Acked-by: Robin Murphy <robin.murphy@arm.com>
Thanks,
Robin.
From: Tomasz Figa <hidden> Date: 2018-02-14 11:27:53
On Wed, Feb 14, 2018 at 7:03 PM, Vivek Gautam
[off-list ref] wrote:
On 1/24/2018 7:19 PM, Robin Murphy wrote:
quoted
On 24/01/18 10:35, Jeffy Chen wrote:
quoted
From: Tomasz Figa <tfiga@chromium.org>
Current code relies on master driver enabling necessary clocks before
IOMMU is accessed, however there are cases when the IOMMU should be
accessed while the master is not running yet, for example allocating
V4L2 videobuf2 buffers, which is done by the VB2 framework using DMA
mapping API and doesn't engage the master driver at all.
This patch fixes the problem by letting clocks needed for IOMMU
operation to be listed in Device Tree and making the driver enable them
for the time of accessing the hardware.
Signed-off-by: Jeffy Chen <redacted>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
Changes in v5:
Use clk_bulk APIs.
Changes in v4: None
Changes in v3: None
Changes in v2: None
[snip]
quoted
quoted
+static int rk_iommu_of_get_clocks(struct rk_iommu *iommu)
+{
+ struct device_node *np = iommu->dev->of_node;
+ int ret;
+ int i;
+
+ ret = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+ if (ret == -ENOENT)
+ return 0;
+ else if (ret < 0)
+ return ret;
+
+ iommu->num_clocks = ret;
+ iommu->clocks = devm_kcalloc(iommu->dev, iommu->num_clocks,
+ sizeof(*iommu->clocks), GFP_KERNEL);
+ if (!iommu->clocks)
+ return -ENOMEM;
+
+ for (i = 0; i < iommu->num_clocks; ++i) {
+ iommu->clocks[i].clk = of_clk_get(np, i);
+ if (IS_ERR(iommu->clocks[i].clk)) {
+ ret = PTR_ERR(iommu->clocks[i].clk);
+ goto err_clk_put;
+ }
+ }
Just to confirm my understanding from a quick scan through the code, the
reason we can't use clk_bulk_get() here is that currently, clocks[i].id
being NULL means we'd end up just getting the first clock multiple times,
right?
I guess there could be other users who also want "just get whatever clocks
I have" functionality, so it might be worth proposing that for the core API
as a separate/follow-up patch, but it definitely doesn't need to be part of
this series.
Just to understand. Is it okay to make the driver "just get whatever clocks
device node gives"?
Doesn't the driver need to be aware of which all clocks are supposed to be
obtained and enabled
It's should good for debug to let the world know which clock we failed to
get.
Yeah, in general that's desired. However, it is at least impractical
to specify all the clocks in Rockchip case, because it's different for
each block and depends on the master next to which it is located.
Best regards,
Tomasz