Hello Everyone,
This is another attempt to finally make Exynos SYSMMU driver fully integrated with DMA-mapping subsystem. The main change from previous version is a rebase onto latest "automatic DMA configuration for IOMMU masters" patches from Will Deacon.
This patchset demonstrates that Will's proposal works fine and significantly simplifies the driver code.
Best regards Marek Szyprowski Samsung R&D Institute Poland
Changelog: v3: - rebased onto "[RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters" - added some minor fixes for iommu and dma-mapping frameworks
v2: http://thread.gmane.org/gmane.linux.kernel.iommu/6472/ - rebased onto "[RFC PATCH v3 0/7] Introduce automatic DMA configuration for IOMMU masters" patches: http://www.spinics.net/lists/arm-kernel/msg362076.html - changed initialization from bus notifiers to DT related callbacks - removed support for separate IO address spaces - this will be discussed separately after the basic support gets merged - removed support for power domain notifier-based runtime power management - this also will be discussed separately later
v1: https://lkml.org/lkml/2014/8/5/183 - initial version, feature complete, completely rewrote integration approach
Patch summary:
Marek Szyprowski (19): iommu: fix const qualifier in of_iommu_set_ops iommu: fix initialization without 'add_device' callback arm: dma-mapping: add missing check for iommu drm: exynos: detach from default dma-mapping domain on init arm: exynos: pm_domains: add support for devices registered before arch_initcall ARM: dts: exynos4: add sysmmu nodes iommu: exynos: don't read version register on every tlb operation iommu: exynos: remove unused functions iommu: exynos: remove useless spinlock iommu: exynos: refactor function parameters to simplify code iommu: exynos: remove unused functions, part 2 iommu: exynos: remove useless device_add/remove callbacks iommu: exynos: add support for binding more than one sysmmu to master device iommu: exynos: add support for runtime_pm iommu: exynos: rename variables to reflect their purpose iommu: exynos: document internal structures iommu: exynos: remove excessive includes and sort others alphabetically iommu: exynos: init from dt-specific callback instead of initcall iommu: exynos: add callback for initializing devices from device tree
arch/arm/boot/dts/exynos4.dtsi | 117 +++++++ arch/arm/boot/dts/exynos4210.dtsi | 23 ++ arch/arm/boot/dts/exynos4x12.dtsi | 82 +++++ arch/arm/mach-exynos/pm_domains.c | 9 +- arch/arm/mm/dma-mapping.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_iommu.c | 3 + drivers/iommu/exynos-iommu.c | 490 ++++++++++++++---------------- drivers/iommu/iommu.c | 2 +- include/linux/of_iommu.h | 4 +- 9 files changed, 459 insertions(+), 273 deletions(-)
Make of_iommu_set_ops() parameter attribute consistent with bus_set_iommu().
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- include/linux/of_iommu.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index 7ded21c0ccdf..d03abbb11c34 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -32,9 +32,9 @@ static inline struct iommu_ops *of_iommu_configure(struct device *dev) #endif /* CONFIG_OF_IOMMU */
static inline void of_iommu_set_ops(struct device_node *np, - struct iommu_ops *ops) + const struct iommu_ops *ops) { - np->data = ops; + np->data = (struct iommu_ops *)ops; }
static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
IOMMU drivers can be initialized from of_iommu helpers. Such drivers don't need to provide device_add callbacks to operate properly, so there is no need to fail initialization if the callback is missing.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index ed8b04867b1f..02f798b7e295 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -737,7 +737,7 @@ static int add_iommu_group(struct device *dev, void *data) const struct iommu_ops *ops = cb->ops;
if (!ops->add_device) - return -ENODEV; + return 0;
WARN_ON(dev->iommu_group);
There is no point trying to initialize dma_ops with arm_get_iommu_dma_map_ops() if no iommu has been provided.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index f15eae5e0513..68040fd62166 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -2050,7 +2050,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, { struct dma_map_ops *dma_ops;
- if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu)) + if (iommu && arm_setup_iommu_dma_ops(dev, dma_base, size, iommu)) dma_ops = arm_get_iommu_dma_map_ops(coherent); else dma_ops = arm_get_dma_map_ops(coherent);
This patch adds code, which detach sub-device nodes from default iommu domain if such has been configured.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_iommu.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_iommu.c b/drivers/gpu/drm/exynos/exynos_drm_iommu.c index b32b291f88ff..323601a52a25 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_iommu.c +++ b/drivers/gpu/drm/exynos/exynos_drm_iommu.c @@ -100,6 +100,9 @@ int drm_iommu_attach_device(struct drm_device *drm_dev,
dma_set_max_seg_size(subdrv_dev, 0xffffffffu);
+ if (subdrv_dev->archdata.mapping) + arm_iommu_detach_device(subdrv_dev); + ret = arm_iommu_attach_device(subdrv_dev, dev->archdata.mapping); if (ret < 0) { DRM_DEBUG_KMS("failed iommu attach.\n");
SYSMMU devices will be registered early before any other devices and before calling arch_initcall. To add them to respective power domains, additional scan of all platform devices is needed.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- arch/arm/mach-exynos/pm_domains.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c index 20f267121b3e..8f1f239ee424 100644 --- a/arch/arm/mach-exynos/pm_domains.c +++ b/arch/arm/mach-exynos/pm_domains.c @@ -105,6 +105,12 @@ static int exynos_pd_power_off(struct generic_pm_domain *domain) return exynos_pd_power(domain, false); }
+static __init int exynos_pd_init_platform_dev(struct device *dev, void *data) +{ + dev_pm_domain_attach(dev, true); + return 0; +} + static __init int exynos4_pm_init_power_domain(void) { struct platform_device *pdev; @@ -161,6 +167,7 @@ no_clk: of_genpd_add_provider_simple(np, &pd->pd); }
- return 0; + return bus_for_each_dev(&platform_bus_type, NULL, NULL, + exynos_pd_init_platform_dev); } arch_initcall(exynos4_pm_init_power_domain);
This patch adds System MMU nodes that are specific to Exynos4210/4x12 series.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- arch/arm/boot/dts/exynos4.dtsi | 117 ++++++++++++++++++++++++++++++++++++++ arch/arm/boot/dts/exynos4210.dtsi | 23 ++++++++ arch/arm/boot/dts/exynos4x12.dtsi | 82 ++++++++++++++++++++++++++ 3 files changed, 222 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi index e0278ecbc816..52a7745c43c3 100644 --- a/arch/arm/boot/dts/exynos4.dtsi +++ b/arch/arm/boot/dts/exynos4.dtsi @@ -174,6 +174,7 @@ clock-names = "fimc", "sclk_fimc"; samsung,power-domain = <&pd_cam>; samsung,sysreg = <&sys_reg>; + iommus = <&sysmmu_fimc0>; status = "disabled"; };
@@ -185,6 +186,7 @@ clock-names = "fimc", "sclk_fimc"; samsung,power-domain = <&pd_cam>; samsung,sysreg = <&sys_reg>; + iommus = <&sysmmu_fimc1>; status = "disabled"; };
@@ -196,6 +198,7 @@ clock-names = "fimc", "sclk_fimc"; samsung,power-domain = <&pd_cam>; samsung,sysreg = <&sys_reg>; + iommus = <&sysmmu_fimc2>; status = "disabled"; };
@@ -207,6 +210,7 @@ clock-names = "fimc", "sclk_fimc"; samsung,power-domain = <&pd_cam>; samsung,sysreg = <&sys_reg>; + iommus = <&sysmmu_fimc3>; status = "disabled"; };
@@ -395,6 +399,8 @@ clocks = <&clock CLK_MFC>; clock-names = "mfc"; status = "disabled"; + iommus = <&sysmmu_mfc_l>, <&sysmmu_mfc_r>; + iommu-names = "left", "right"; };
serial_0: serial@13800000 { @@ -644,5 +650,116 @@ samsung,power-domain = <&pd_lcd0>; samsung,sysreg = <&sys_reg>; status = "disabled"; + iommus = <&sysmmu_fimd0>; + }; + + sysmmu_mfc_l: sysmmu@13620000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x13620000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <5 5>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_MFCL>, <&clock CLK_MFC>; + samsung,power-domain = <&pd_mfc>; + #iommu-cells = <0>; + }; + + sysmmu_mfc_r: sysmmu@13630000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x13630000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <5 6>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_MFCR>, <&clock CLK_MFC>; + samsung,power-domain = <&pd_mfc>; + #iommu-cells = <0>; + }; + + sysmmu_tv: sysmmu@12E20000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x12E20000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <5 4>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_TV>, <&clock CLK_MIXER>; + samsung,power-domain = <&pd_tv>; + #iommu-cells = <0>; + }; + + sysmmu_fimc0: sysmmu@11A20000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x11A20000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <4 2>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_FIMC0>, <&clock CLK_FIMC0>; + samsung,power-domain = <&pd_cam>; + #iommu-cells = <0>; + }; + + sysmmu_fimc1: sysmmu@11A30000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x11A30000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <4 3>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_FIMC1>, <&clock CLK_FIMC1>; + samsung,power-domain = <&pd_cam>; + #iommu-cells = <0>; + }; + + sysmmu_fimc2: sysmmu@11A40000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x11A40000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <4 4>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_FIMC2>, <&clock CLK_FIMC2>; + samsung,power-domain = <&pd_cam>; + #iommu-cells = <0>; + }; + + sysmmu_fimc3: sysmmu@11A50000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x11A50000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <4 5>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_FIMC3>, <&clock CLK_FIMC3>; + samsung,power-domain = <&pd_cam>; + #iommu-cells = <0>; + }; + + sysmmu_jpeg: sysmmu@11A60000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x11A60000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <4 6>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_JPEG>, <&clock CLK_JPEG>; + samsung,power-domain = <&pd_cam>; + #iommu-cells = <0>; + }; + + sysmmu_rotator: sysmmu@12A30000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x12A30000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <5 0>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_ROTATOR>, <&clock CLK_ROTATOR>; + samsung,power-domain = <&pd_lcd0>; + #iommu-cells = <0>; + }; + + sysmmu_fimd0: sysmmu@11E20000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x11E20000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <5 2>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_FIMD0>, <&clock CLK_FIMD0>; + samsung,power-domain = <&pd_lcd0>; + #iommu-cells = <0>; }; }; diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi index 807bb5bf91fc..9ae48c236a6b 100644 --- a/arch/arm/boot/dts/exynos4210.dtsi +++ b/arch/arm/boot/dts/exynos4210.dtsi @@ -142,6 +142,7 @@ interrupts = <0 89 0>; clocks = <&clock CLK_SCLK_FIMG2D>, <&clock CLK_G2D>; clock-names = "sclk_fimg2d", "fimg2d"; + iommus = <&sysmmu_g2d>; status = "disabled"; };
@@ -175,4 +176,26 @@ samsung,lcd-wb; }; }; + + sysmmu_g2d: sysmmu@12A20000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x12A20000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <4 7>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_G2D>, <&clock CLK_G2D>; + samsung,power-domain = <&pd_lcd0>; + #iommu-cells = <0>; + }; + + sysmmu_fimd1: sysmmu@12220000 { + compatible = "samsung,exynos-sysmmu"; + interrupt-parent = <&combiner>; + reg = <0x12220000 0x1000>; + interrupts = <5 3>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_FIMD1>, <&clock CLK_FIMD1>; + samsung,power-domain = <&pd_lcd1>; + #iommu-cells = <0>; + }; }; diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi index 861bb919f6d3..cc97e18cebb8 100644 --- a/arch/arm/boot/dts/exynos4x12.dtsi +++ b/arch/arm/boot/dts/exynos4x12.dtsi @@ -148,6 +148,7 @@ interrupts = <0 89 0>; clocks = <&clock CLK_SCLK_FIMG2D>, <&clock CLK_G2D>; clock-names = "sclk_fimg2d", "fimg2d"; + iommus = <&sysmmu_g2d>; status = "disabled"; };
@@ -197,6 +198,7 @@ samsung,power-domain = <&pd_isp>; clocks = <&clock CLK_FIMC_LITE0>; clock-names = "flite"; + iommus = <&sysmmu_fimc_lite0>; status = "disabled"; };
@@ -207,6 +209,7 @@ samsung,power-domain = <&pd_isp>; clocks = <&clock CLK_FIMC_LITE1>; clock-names = "flite"; + iommus = <&sysmmu_fimc_lite1>; status = "disabled"; };
@@ -235,6 +238,9 @@ "mcuispdiv1", "uart", "aclk200", "div_aclk200", "aclk400mcuisp", "div_aclk400mcuisp"; + iommus = <&sysmmu_fimc_isp>, <&sysmmu_fimc_drc>, + <&sysmmu_fimc_fd>, <&sysmmu_fimc_mcuctl>; + iommu-names = "isp", "drc", "fd", "mcuctl"; #address-cells = <1>; #size-cells = <1>; ranges; @@ -271,4 +277,80 @@ compatible = "samsung,exynos4x12-usb2-phy"; samsung,sysreg-phandle = <&sys_reg>; }; + + sysmmu_g2d: sysmmu@10A40000{ + compatible = "samsung,exynos-sysmmu"; + reg = <0x10A40000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <4 7>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_G2D>, <&clock CLK_G2D>; + #iommu-cells = <0>; + }; + + sysmmu_fimc_isp: sysmmu@12260000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x12260000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <16 2>; + samsung,power-domain = <&pd_isp>; + clock-names = "sysmmu"; + clocks = <&clock CLK_SMMU_ISP>; + #iommu-cells = <0>; + }; + + sysmmu_fimc_drc: sysmmu@12270000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x12270000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <16 3>; + samsung,power-domain = <&pd_isp>; + clock-names = "sysmmu"; + clocks = <&clock CLK_SMMU_DRC>; + #iommu-cells = <0>; + }; + + sysmmu_fimc_fd: sysmmu@122A0000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x122A0000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <16 4>; + samsung,power-domain = <&pd_isp>; + clock-names = "sysmmu"; + clocks = <&clock CLK_SMMU_FD>; + #iommu-cells = <0>; + }; + + sysmmu_fimc_mcuctl: sysmmu@122B0000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x122B0000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <16 5>; + samsung,power-domain = <&pd_isp>; + clock-names = "sysmmu"; + clocks = <&clock CLK_SMMU_ISPCX>; + #iommu-cells = <0>; + }; + + sysmmu_fimc_lite0: sysmmu@123B0000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x123B0000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <16 0>; + samsung,power-domain = <&pd_isp>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_LITE0>, <&clock CLK_FIMC_LITE0>; + #iommu-cells = <0>; + }; + + sysmmu_fimc_lite1: sysmmu@123C0000 { + compatible = "samsung,exynos-sysmmu"; + reg = <0x123C0000 0x1000>; + interrupt-parent = <&combiner>; + interrupts = <16 1>; + samsung,power-domain = <&pd_isp>; + clock-names = "sysmmu", "master"; + clocks = <&clock CLK_SMMU_LITE1>, <&clock CLK_FIMC_LITE1>; + #iommu-cells = <0>; + }; };
This patch removes reading of REG_MMU_VERSION register on every tlb operation and caches SYSMMU version in driver's internal data.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 74233186f6f7..2912c4f9a8ad 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -212,6 +212,7 @@ struct sysmmu_drvdata { spinlock_t lock; struct iommu_domain *domain; phys_addr_t pgtable; + int version; };
static bool set_sysmmu_active(struct sysmmu_drvdata *data) @@ -238,11 +239,6 @@ static void sysmmu_unblock(void __iomem *sfrbase) __raw_writel(CTRL_ENABLE, sfrbase + REG_MMU_CTRL); }
-static unsigned int __raw_sysmmu_version(struct sysmmu_drvdata *data) -{ - return MMU_RAW_VER(__raw_readl(data->sfrbase + REG_MMU_VERSION)); -} - static bool sysmmu_block(void __iomem *sfrbase) { int i = 120; @@ -402,7 +398,7 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data) unsigned int cfg = CFG_LRU | CFG_QOS(15); unsigned int ver;
- ver = __raw_sysmmu_version(data); + ver = MMU_RAW_VER(__raw_readl(data->sfrbase + REG_MMU_VERSION)); if (MMU_MAJ_VER(ver) == 3) { if (MMU_MIN_VER(ver) >= 2) { cfg |= CFG_FLPDCACHE; @@ -416,6 +412,7 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data) }
__raw_writel(cfg, data->sfrbase + REG_MMU_CFG); + data->version = ver; }
static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data) @@ -525,7 +522,7 @@ static bool exynos_sysmmu_disable(struct device *dev) static void __sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data, sysmmu_iova_t iova) { - if (__raw_sysmmu_version(data) == MAKE_MMU_VER(3, 3)) + if (data->version == MAKE_MMU_VER(3, 3)) __raw_writel(iova | 0x1, data->sfrbase + REG_MMU_FLUSH_ENTRY); }
@@ -574,7 +571,7 @@ static void sysmmu_tlb_invalidate_entry(struct device *dev, sysmmu_iova_t iova, * 1MB page can be cached in one of all sets. * 64KB page can be one of 16 consecutive sets. */ - if (MMU_MAJ_VER(__raw_sysmmu_version(data)) == 2) + if (MMU_MAJ_VER(data->version) == 2) num_inv = min_t(unsigned int, size / PAGE_SIZE, 64);
if (sysmmu_block(data->sfrbase)) {
This patch removes two unneeded functions, which are not a part of generic IOMMU API and were never used by any other driver.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 31 ------------------------------- 1 file changed, 31 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 2912c4f9a8ad..c0262ed1a31e 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -490,13 +490,6 @@ static int __exynos_sysmmu_enable(struct device *dev, phys_addr_t pgtable, return ret; }
-int exynos_sysmmu_enable(struct device *dev, phys_addr_t pgtable) -{ - BUG_ON(!memblock_is_memory(pgtable)); - - return __exynos_sysmmu_enable(dev, pgtable, NULL); -} - static bool exynos_sysmmu_disable(struct device *dev) { unsigned long flags; @@ -588,30 +581,6 @@ static void sysmmu_tlb_invalidate_entry(struct device *dev, sysmmu_iova_t iova, spin_unlock_irqrestore(&data->lock, flags); }
-void exynos_sysmmu_tlb_invalidate(struct device *dev) -{ - struct exynos_iommu_owner *owner = dev->archdata.iommu; - unsigned long flags; - struct sysmmu_drvdata *data; - - data = dev_get_drvdata(owner->sysmmu); - - spin_lock_irqsave(&data->lock, flags); - if (is_sysmmu_active(data)) { - if (!IS_ERR(data->clk_master)) - clk_enable(data->clk_master); - if (sysmmu_block(data->sfrbase)) { - __sysmmu_tlb_invalidate(data->sfrbase); - sysmmu_unblock(data->sfrbase); - } - if (!IS_ERR(data->clk_master)) - clk_disable(data->clk_master); - } else { - dev_dbg(dev, "disabled. Skipping TLB invalidation\n"); - } - spin_unlock_irqrestore(&data->lock, flags); -} - static int __init exynos_sysmmu_probe(struct platform_device *pdev) { int irq, ret;
This patch removes useless spinlocks and other unused members from struct exynos_iommu_owner. There is no point is protecting this structure by spinlock because content of this structure doesn't change and other structures have their own spinlocks.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index c0262ed1a31e..8a952a573477 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -189,9 +189,6 @@ struct exynos_iommu_owner { struct list_head client; /* entry of exynos_iommu_domain.clients */ struct device *dev; struct device *sysmmu; - struct iommu_domain *domain; - void *vmm_data; /* IO virtual memory manager's data */ - spinlock_t lock; /* Lock to preserve consistency of System MMU */ };
struct exynos_iommu_domain { @@ -477,16 +474,12 @@ static int __exynos_sysmmu_enable(struct device *dev, phys_addr_t pgtable,
BUG_ON(!has_sysmmu(dev));
- spin_lock_irqsave(&owner->lock, flags); - data = dev_get_drvdata(owner->sysmmu);
ret = __sysmmu_enable(data, pgtable, domain); if (ret >= 0) data->master = dev;
- spin_unlock_irqrestore(&owner->lock, flags); - return ret; }
@@ -499,16 +492,12 @@ static bool exynos_sysmmu_disable(struct device *dev)
BUG_ON(!has_sysmmu(dev));
- spin_lock_irqsave(&owner->lock, flags); - data = dev_get_drvdata(owner->sysmmu);
disabled = __sysmmu_disable(data); if (disabled) data->master = NULL;
- spin_unlock_irqrestore(&owner->lock, flags); - return disabled; }
This patch simplifies the code by: - refactoring function parameters from struct device pointer to direct pointer to struct sysmmu drvdata - moving list_head enteries from struct exynos_iommu_owner directly to struct sysmmu_drvdata
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 93 ++++++++++++++++++++++---------------------- 1 file changed, 46 insertions(+), 47 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 8a952a573477..1e7c55191d98 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -186,8 +186,6 @@ static char *sysmmu_fault_name[SYSMMU_FAULTS_NUM] = {
/* attached to dev.archdata.iommu of the master device */ struct exynos_iommu_owner { - struct list_head client; /* entry of exynos_iommu_domain.clients */ - struct device *dev; struct device *sysmmu; };
@@ -208,6 +206,7 @@ struct sysmmu_drvdata { int activations; spinlock_t lock; struct iommu_domain *domain; + struct list_head domain_node; phys_addr_t pgtable; int version; }; @@ -508,12 +507,10 @@ static void __sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data, __raw_writel(iova | 0x1, data->sfrbase + REG_MMU_FLUSH_ENTRY); }
-static void sysmmu_tlb_invalidate_flpdcache(struct device *dev, +static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data, sysmmu_iova_t iova) { unsigned long flags; - struct exynos_iommu_owner *owner = dev->archdata.iommu; - struct sysmmu_drvdata *data = dev_get_drvdata(owner->sysmmu);
if (!IS_ERR(data->clk_master)) clk_enable(data->clk_master); @@ -527,14 +524,10 @@ static void sysmmu_tlb_invalidate_flpdcache(struct device *dev, clk_disable(data->clk_master); }
-static void sysmmu_tlb_invalidate_entry(struct device *dev, sysmmu_iova_t iova, - size_t size) +static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data, + sysmmu_iova_t iova, size_t size) { - struct exynos_iommu_owner *owner = dev->archdata.iommu; unsigned long flags; - struct sysmmu_drvdata *data; - - data = dev_get_drvdata(owner->sysmmu);
spin_lock_irqsave(&data->lock, flags); if (is_sysmmu_active(data)) { @@ -564,8 +557,8 @@ static void sysmmu_tlb_invalidate_entry(struct device *dev, sysmmu_iova_t iova, if (!IS_ERR(data->clk_master)) clk_disable(data->clk_master); } else { - dev_dbg(dev, "disabled. Skipping TLB invalidation @ %#x\n", - iova); + dev_dbg(data->master, + "disabled. Skipping TLB invalidation @ %#x\n", iova); } spin_unlock_irqrestore(&data->lock, flags); } @@ -704,7 +697,7 @@ err_pgtable: static void exynos_iommu_domain_destroy(struct iommu_domain *domain) { struct exynos_iommu_domain *priv = domain->priv; - struct exynos_iommu_owner *owner; + struct sysmmu_drvdata *data; unsigned long flags; int i;
@@ -712,14 +705,12 @@ static void exynos_iommu_domain_destroy(struct iommu_domain *domain)
spin_lock_irqsave(&priv->lock, flags);
- list_for_each_entry(owner, &priv->clients, client) { - while (!exynos_sysmmu_disable(owner->dev)) - ; /* until System MMU is actually disabled */ + list_for_each_entry(data, &priv->clients, domain_node) { + if (__sysmmu_disable(data)) + data->master = NULL; + list_del_init(&data->domain_node); }
- while (!list_empty(&priv->clients)) - list_del_init(priv->clients.next); - spin_unlock_irqrestore(&priv->lock, flags);
for (i = 0; i < NUM_LV1ENTRIES; i++) @@ -738,20 +729,26 @@ static int exynos_iommu_attach_device(struct iommu_domain *domain, { struct exynos_iommu_owner *owner = dev->archdata.iommu; struct exynos_iommu_domain *priv = domain->priv; + struct sysmmu_drvdata *data; phys_addr_t pagetable = virt_to_phys(priv->pgtable); unsigned long flags; - int ret; + int ret = -ENODEV;
- spin_lock_irqsave(&priv->lock, flags); + if (!has_sysmmu(dev)) + return -ENODEV;
- ret = __exynos_sysmmu_enable(dev, pagetable, domain); - if (ret == 0) { - list_add_tail(&owner->client, &priv->clients); - owner->domain = domain; + data = dev_get_drvdata(owner->sysmmu); + if (data) { + ret = __sysmmu_enable(data, pagetable, domain); + if (ret >= 0) { + data->master = dev; + + spin_lock_irqsave(&priv->lock, flags); + list_add_tail(&data->domain_node, &priv->clients); + spin_unlock_irqrestore(&priv->lock, flags); + } }
- spin_unlock_irqrestore(&priv->lock, flags); - if (ret < 0) { dev_err(dev, "%s: Failed to attach IOMMU with pgtable %pa\n", __func__, &pagetable); @@ -767,26 +764,29 @@ static int exynos_iommu_attach_device(struct iommu_domain *domain, static void exynos_iommu_detach_device(struct iommu_domain *domain, struct device *dev) { - struct exynos_iommu_owner *owner; struct exynos_iommu_domain *priv = domain->priv; phys_addr_t pagetable = virt_to_phys(priv->pgtable); + struct sysmmu_drvdata *data; unsigned long flags; + int found = 0;
- spin_lock_irqsave(&priv->lock, flags); + if (!has_sysmmu(dev)) + return;
- list_for_each_entry(owner, &priv->clients, client) { - if (owner == dev->archdata.iommu) { - if (exynos_sysmmu_disable(dev)) { - list_del_init(&owner->client); - owner->domain = NULL; + spin_lock_irqsave(&priv->lock, flags); + list_for_each_entry(data, &priv->clients, domain_node) { + if (data->master == dev) { + if (__sysmmu_disable(data)) { + data->master = NULL; + list_del_init(&data->domain_node); } + found = true; break; } } - spin_unlock_irqrestore(&priv->lock, flags);
- if (owner == dev->archdata.iommu) + if (found) dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__, &pagetable); else @@ -833,12 +833,11 @@ static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *priv, * not currently mapped. */ if (need_flush_flpd_cache) { - struct exynos_iommu_owner *owner; + struct sysmmu_drvdata *data;
spin_lock(&priv->lock); - list_for_each_entry(owner, &priv->clients, client) - sysmmu_tlb_invalidate_flpdcache( - owner->dev, iova); + list_for_each_entry(data, &priv->clients, domain_node) + sysmmu_tlb_invalidate_flpdcache(data, iova); spin_unlock(&priv->lock); } } @@ -873,13 +872,13 @@ static int lv1set_section(struct exynos_iommu_domain *priv,
spin_lock(&priv->lock); if (lv1ent_page_zero(sent)) { - struct exynos_iommu_owner *owner; + struct sysmmu_drvdata *data; /* * Flushing FLPD cache in System MMU v3.3 that may cache a FLPD * entry by speculative prefetch of SLPD which has no mapping. */ - list_for_each_entry(owner, &priv->clients, client) - sysmmu_tlb_invalidate_flpdcache(owner->dev, iova); + list_for_each_entry(data, &priv->clients, domain_node) + sysmmu_tlb_invalidate_flpdcache(data, iova); } spin_unlock(&priv->lock);
@@ -984,13 +983,13 @@ static int exynos_iommu_map(struct iommu_domain *domain, unsigned long l_iova, static void exynos_iommu_tlb_invalidate_entry(struct exynos_iommu_domain *priv, sysmmu_iova_t iova, size_t size) { - struct exynos_iommu_owner *owner; + struct sysmmu_drvdata *data; unsigned long flags;
spin_lock_irqsave(&priv->lock, flags);
- list_for_each_entry(owner, &priv->clients, client) - sysmmu_tlb_invalidate_entry(owner->dev, iova, size); + list_for_each_entry(data, &priv->clients, domain_node) + sysmmu_tlb_invalidate_entry(data, iova, size);
spin_unlock_irqrestore(&priv->lock, flags); }
After refactoring functions to use pointer to struct sysmmu_drvdata directly, some functions became useless and thus never used, so remove them completely.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 43 ------------------------------------------- 1 file changed, 43 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 1e7c55191d98..a9616cd64e37 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -457,49 +457,6 @@ static int __sysmmu_enable(struct sysmmu_drvdata *data, return ret; }
-/* __exynos_sysmmu_enable: Enables System MMU - * - * returns -error if an error occurred and System MMU is not enabled, - * 0 if the System MMU has been just enabled and 1 if System MMU was already - * enabled before. - */ -static int __exynos_sysmmu_enable(struct device *dev, phys_addr_t pgtable, - struct iommu_domain *domain) -{ - int ret = 0; - unsigned long flags; - struct exynos_iommu_owner *owner = dev->archdata.iommu; - struct sysmmu_drvdata *data; - - BUG_ON(!has_sysmmu(dev)); - - data = dev_get_drvdata(owner->sysmmu); - - ret = __sysmmu_enable(data, pgtable, domain); - if (ret >= 0) - data->master = dev; - - return ret; -} - -static bool exynos_sysmmu_disable(struct device *dev) -{ - unsigned long flags; - bool disabled = true; - struct exynos_iommu_owner *owner = dev->archdata.iommu; - struct sysmmu_drvdata *data; - - BUG_ON(!has_sysmmu(dev)); - - data = dev_get_drvdata(owner->sysmmu); - - disabled = __sysmmu_disable(data); - if (disabled) - data->master = NULL; - - return disabled; -} - static void __sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data, sysmmu_iova_t iova) {
The driver doesn't need to do anything important in device add/remove callbacks, because initialization will be done from device-tree specific callbacks added later. IOMMU groups created by current code were never used.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 28 ---------------------------- 1 file changed, 28 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index a9616cd64e37..93b97df772f1 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -1056,32 +1056,6 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain, return phys; }
-static int exynos_iommu_add_device(struct device *dev) -{ - struct iommu_group *group; - int ret; - - 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); - iommu_group_put(group); - - return ret; -} - -static void exynos_iommu_remove_device(struct device *dev) -{ - iommu_group_remove_device(dev); -} - static const struct iommu_ops exynos_iommu_ops = { .domain_init = exynos_iommu_domain_init, .domain_destroy = exynos_iommu_domain_destroy, @@ -1090,8 +1064,6 @@ static const struct iommu_ops exynos_iommu_ops = { .map = exynos_iommu_map, .unmap = exynos_iommu_unmap, .iova_to_phys = exynos_iommu_iova_to_phys, - .add_device = exynos_iommu_add_device, - .remove_device = exynos_iommu_remove_device, .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, };
This patch adds support for assigning more than one SYSMMU controller to the master device. This has been achieved simply by chaning the struct device pointer in struct exynos_iommu_owner into the list of struct sysmmu_drvdata of all controllers assigned to the given master device.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 93b97df772f1..77dec32c59ef 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -186,7 +186,7 @@ static char *sysmmu_fault_name[SYSMMU_FAULTS_NUM] = {
/* attached to dev.archdata.iommu of the master device */ struct exynos_iommu_owner { - struct device *sysmmu; + struct list_head clients; };
struct exynos_iommu_domain { @@ -207,6 +207,7 @@ struct sysmmu_drvdata { spinlock_t lock; struct iommu_domain *domain; struct list_head domain_node; + struct list_head owner_node; phys_addr_t pgtable; int version; }; @@ -694,8 +695,7 @@ static int exynos_iommu_attach_device(struct iommu_domain *domain, if (!has_sysmmu(dev)) return -ENODEV;
- data = dev_get_drvdata(owner->sysmmu); - if (data) { + list_for_each_entry(data, &owner->clients, owner_node) { ret = __sysmmu_enable(data, pagetable, domain); if (ret >= 0) { data->master = dev; @@ -723,7 +723,7 @@ static void exynos_iommu_detach_device(struct iommu_domain *domain, { struct exynos_iommu_domain *priv = domain->priv; phys_addr_t pagetable = virt_to_phys(priv->pgtable); - struct sysmmu_drvdata *data; + struct sysmmu_drvdata *data, *next; unsigned long flags; int found = 0;
@@ -731,14 +731,13 @@ static void exynos_iommu_detach_device(struct iommu_domain *domain, return;
spin_lock_irqsave(&priv->lock, flags); - list_for_each_entry(data, &priv->clients, domain_node) { + list_for_each_entry_safe(data, next, &priv->clients, domain_node) { if (data->master == dev) { if (__sysmmu_disable(data)) { data->master = NULL; list_del_init(&data->domain_node); } found = true; - break; } } spin_unlock_irqrestore(&priv->lock, flags);
This patch fixes support for runtime power management for SYSMMU controllers, so they are enabled when master device is attached.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 77dec32c59ef..27742c4d5a76 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -696,6 +696,7 @@ static int exynos_iommu_attach_device(struct iommu_domain *domain, return -ENODEV;
list_for_each_entry(data, &owner->clients, owner_node) { + pm_runtime_get_sync(data->sysmmu); ret = __sysmmu_enable(data, pagetable, domain); if (ret >= 0) { data->master = dev; @@ -737,6 +738,7 @@ static void exynos_iommu_detach_device(struct iommu_domain *domain, data->master = NULL; list_del_init(&data->domain_node); } + pm_runtime_put(data->sysmmu); found = true; } }
This patch renames some variables to make the code easier to understand. 'domain' is replaced by 'iommu_domain' (more generic entity) and really meaning less 'priv' by 'domain' to reflect its purpose.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 191 ++++++++++++++++++++++--------------------- 1 file changed, 97 insertions(+), 94 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 27742c4d5a76..4c9b9b9e099f 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -430,8 +430,8 @@ static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data) clk_disable(data->clk_master); }
-static int __sysmmu_enable(struct sysmmu_drvdata *data, - phys_addr_t pgtable, struct iommu_domain *domain) +static int __sysmmu_enable(struct sysmmu_drvdata *data, phys_addr_t pgtable, + struct iommu_domain *iommu_domain) { int ret = 0; unsigned long flags; @@ -439,7 +439,7 @@ static int __sysmmu_enable(struct sysmmu_drvdata *data, spin_lock_irqsave(&data->lock, flags); if (set_sysmmu_active(data)) { data->pgtable = pgtable; - data->domain = domain; + data->domain = iommu_domain;
__sysmmu_enable_nocount(data);
@@ -603,92 +603,93 @@ static inline void pgtable_flush(void *vastart, void *vaend) virt_to_phys(vaend)); }
-static int exynos_iommu_domain_init(struct iommu_domain *domain) +static int exynos_iommu_domain_init(struct iommu_domain *iommu_domain) { - struct exynos_iommu_domain *priv; + struct exynos_iommu_domain *domain; int i;
- priv = kzalloc(sizeof(*priv), GFP_KERNEL); - if (!priv) + domain = kzalloc(sizeof(*domain), GFP_KERNEL); + if (!domain) return -ENOMEM;
- priv->pgtable = (sysmmu_pte_t *)__get_free_pages(GFP_KERNEL, 2); - if (!priv->pgtable) + domain->pgtable = (sysmmu_pte_t *)__get_free_pages(GFP_KERNEL, 2); + if (!domain->pgtable) goto err_pgtable;
- priv->lv2entcnt = (short *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1); - if (!priv->lv2entcnt) + domain->lv2entcnt = (short *) + __get_free_pages(GFP_KERNEL | __GFP_ZERO, 1); + if (!domain->lv2entcnt) goto err_counter;
/* Workaround for System MMU v3.3 to prevent caching 1MiB mapping */ for (i = 0; i < NUM_LV1ENTRIES; i += 8) { - priv->pgtable[i + 0] = ZERO_LV2LINK; - priv->pgtable[i + 1] = ZERO_LV2LINK; - priv->pgtable[i + 2] = ZERO_LV2LINK; - priv->pgtable[i + 3] = ZERO_LV2LINK; - priv->pgtable[i + 4] = ZERO_LV2LINK; - priv->pgtable[i + 5] = ZERO_LV2LINK; - priv->pgtable[i + 6] = ZERO_LV2LINK; - priv->pgtable[i + 7] = ZERO_LV2LINK; + domain->pgtable[i + 0] = ZERO_LV2LINK; + domain->pgtable[i + 1] = ZERO_LV2LINK; + domain->pgtable[i + 2] = ZERO_LV2LINK; + domain->pgtable[i + 3] = ZERO_LV2LINK; + domain->pgtable[i + 4] = ZERO_LV2LINK; + domain->pgtable[i + 5] = ZERO_LV2LINK; + domain->pgtable[i + 6] = ZERO_LV2LINK; + domain->pgtable[i + 7] = ZERO_LV2LINK; }
- pgtable_flush(priv->pgtable, priv->pgtable + NUM_LV1ENTRIES); + pgtable_flush(domain->pgtable, domain->pgtable + NUM_LV1ENTRIES);
- spin_lock_init(&priv->lock); - spin_lock_init(&priv->pgtablelock); - INIT_LIST_HEAD(&priv->clients); + spin_lock_init(&domain->lock); + spin_lock_init(&domain->pgtablelock); + INIT_LIST_HEAD(&domain->clients);
- domain->geometry.aperture_start = 0; - domain->geometry.aperture_end = ~0UL; - domain->geometry.force_aperture = true; + iommu_domain->geometry.aperture_start = 0; + iommu_domain->geometry.aperture_end = ~0UL; + iommu_domain->geometry.force_aperture = true;
- domain->priv = priv; + iommu_domain->priv = domain; return 0;
err_counter: - free_pages((unsigned long)priv->pgtable, 2); + free_pages((unsigned long)domain->pgtable, 2); err_pgtable: - kfree(priv); + kfree(domain); return -ENOMEM; }
-static void exynos_iommu_domain_destroy(struct iommu_domain *domain) +static void exynos_iommu_domain_destroy(struct iommu_domain *iommu_domain) { - struct exynos_iommu_domain *priv = domain->priv; + struct exynos_iommu_domain *domain = iommu_domain->priv; struct sysmmu_drvdata *data; unsigned long flags; int i;
- WARN_ON(!list_empty(&priv->clients)); + WARN_ON(!list_empty(&domain->clients));
- spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&domain->lock, flags);
- list_for_each_entry(data, &priv->clients, domain_node) { + list_for_each_entry(data, &domain->clients, domain_node) { if (__sysmmu_disable(data)) data->master = NULL; list_del_init(&data->domain_node); }
- spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&domain->lock, flags);
for (i = 0; i < NUM_LV1ENTRIES; i++) - if (lv1ent_page(priv->pgtable + i)) + if (lv1ent_page(domain->pgtable + i)) kmem_cache_free(lv2table_kmem_cache, - phys_to_virt(lv2table_base(priv->pgtable + i))); + phys_to_virt(lv2table_base(domain->pgtable + i)));
- free_pages((unsigned long)priv->pgtable, 2); - free_pages((unsigned long)priv->lv2entcnt, 1); - kfree(domain->priv); - domain->priv = NULL; + free_pages((unsigned long)domain->pgtable, 2); + free_pages((unsigned long)domain->lv2entcnt, 1); + kfree(iommu_domain->priv); + iommu_domain->priv = NULL; }
-static int exynos_iommu_attach_device(struct iommu_domain *domain, +static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, struct device *dev) { struct exynos_iommu_owner *owner = dev->archdata.iommu; - struct exynos_iommu_domain *priv = domain->priv; + struct exynos_iommu_domain *domain = iommu_domain->priv; struct sysmmu_drvdata *data; - phys_addr_t pagetable = virt_to_phys(priv->pgtable); + phys_addr_t pagetable = virt_to_phys(domain->pgtable); unsigned long flags; int ret = -ENODEV;
@@ -697,13 +698,13 @@ static int exynos_iommu_attach_device(struct iommu_domain *domain,
list_for_each_entry(data, &owner->clients, owner_node) { pm_runtime_get_sync(data->sysmmu); - ret = __sysmmu_enable(data, pagetable, domain); + ret = __sysmmu_enable(data, pagetable, iommu_domain); if (ret >= 0) { data->master = dev;
- spin_lock_irqsave(&priv->lock, flags); - list_add_tail(&data->domain_node, &priv->clients); - spin_unlock_irqrestore(&priv->lock, flags); + spin_lock_irqsave(&domain->lock, flags); + list_add_tail(&data->domain_node, &domain->clients); + spin_unlock_irqrestore(&domain->lock, flags); } }
@@ -719,11 +720,11 @@ static int exynos_iommu_attach_device(struct iommu_domain *domain, return ret; }
-static void exynos_iommu_detach_device(struct iommu_domain *domain, +static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain, struct device *dev) { - struct exynos_iommu_domain *priv = domain->priv; - phys_addr_t pagetable = virt_to_phys(priv->pgtable); + struct exynos_iommu_domain *domain = iommu_domain->priv; + phys_addr_t pagetable = virt_to_phys(domain->pgtable); struct sysmmu_drvdata *data, *next; unsigned long flags; int found = 0; @@ -731,8 +732,8 @@ static void exynos_iommu_detach_device(struct iommu_domain *domain, if (!has_sysmmu(dev)) return;
- spin_lock_irqsave(&priv->lock, flags); - list_for_each_entry_safe(data, next, &priv->clients, domain_node) { + spin_lock_irqsave(&domain->lock, flags); + list_for_each_entry_safe(data, next, &domain->clients, domain_node) { if (data->master == dev) { if (__sysmmu_disable(data)) { data->master = NULL; @@ -742,7 +743,7 @@ static void exynos_iommu_detach_device(struct iommu_domain *domain, found = true; } } - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&domain->lock, flags);
if (found) dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", @@ -751,7 +752,7 @@ static void exynos_iommu_detach_device(struct iommu_domain *domain, dev_err(dev, "%s: No IOMMU is attached\n", __func__); }
-static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *priv, +static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain, sysmmu_pte_t *sent, sysmmu_iova_t iova, short *pgcounter) { if (lv1ent_section(sent)) { @@ -793,17 +794,17 @@ static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *priv, if (need_flush_flpd_cache) { struct sysmmu_drvdata *data;
- spin_lock(&priv->lock); - list_for_each_entry(data, &priv->clients, domain_node) + spin_lock(&domain->lock); + list_for_each_entry(data, &domain->clients, domain_node) sysmmu_tlb_invalidate_flpdcache(data, iova); - spin_unlock(&priv->lock); + spin_unlock(&domain->lock); } }
return page_entry(sent, iova); }
-static int lv1set_section(struct exynos_iommu_domain *priv, +static int lv1set_section(struct exynos_iommu_domain *domain, sysmmu_pte_t *sent, sysmmu_iova_t iova, phys_addr_t paddr, short *pgcnt) { @@ -828,17 +829,17 @@ static int lv1set_section(struct exynos_iommu_domain *priv,
pgtable_flush(sent, sent + 1);
- spin_lock(&priv->lock); + spin_lock(&domain->lock); if (lv1ent_page_zero(sent)) { struct sysmmu_drvdata *data; /* * Flushing FLPD cache in System MMU v3.3 that may cache a FLPD * entry by speculative prefetch of SLPD which has no mapping. */ - list_for_each_entry(data, &priv->clients, domain_node) + list_for_each_entry(data, &domain->clients, domain_node) sysmmu_tlb_invalidate_flpdcache(data, iova); } - spin_unlock(&priv->lock); + spin_unlock(&domain->lock);
return 0; } @@ -898,74 +899,76 @@ static int lv2set_page(sysmmu_pte_t *pent, phys_addr_t paddr, size_t size, * than or equal to 128KiB. * - Start address of an I/O virtual region must be aligned by 128KiB. */ -static int exynos_iommu_map(struct iommu_domain *domain, unsigned long l_iova, - phys_addr_t paddr, size_t size, int prot) +static int exynos_iommu_map(struct iommu_domain *iommu_domain, + unsigned long l_iova, phys_addr_t paddr, size_t size, + int prot) { - struct exynos_iommu_domain *priv = domain->priv; + struct exynos_iommu_domain *domain = iommu_domain->priv; sysmmu_pte_t *entry; sysmmu_iova_t iova = (sysmmu_iova_t)l_iova; unsigned long flags; int ret = -ENOMEM;
- BUG_ON(priv->pgtable == NULL); + BUG_ON(domain->pgtable == NULL);
- spin_lock_irqsave(&priv->pgtablelock, flags); + spin_lock_irqsave(&domain->pgtablelock, flags);
- entry = section_entry(priv->pgtable, iova); + entry = section_entry(domain->pgtable, iova);
if (size == SECT_SIZE) { - ret = lv1set_section(priv, entry, iova, paddr, - &priv->lv2entcnt[lv1ent_offset(iova)]); + ret = lv1set_section(domain, entry, iova, paddr, + &domain->lv2entcnt[lv1ent_offset(iova)]); } else { sysmmu_pte_t *pent;
- pent = alloc_lv2entry(priv, entry, iova, - &priv->lv2entcnt[lv1ent_offset(iova)]); + pent = alloc_lv2entry(domain, entry, iova, + &domain->lv2entcnt[lv1ent_offset(iova)]);
if (IS_ERR(pent)) ret = PTR_ERR(pent); else ret = lv2set_page(pent, paddr, size, - &priv->lv2entcnt[lv1ent_offset(iova)]); + &domain->lv2entcnt[lv1ent_offset(iova)]); }
if (ret) pr_err("%s: Failed(%d) to map %#zx bytes @ %#x\n", __func__, ret, size, iova);
- spin_unlock_irqrestore(&priv->pgtablelock, flags); + spin_unlock_irqrestore(&domain->pgtablelock, flags);
return ret; }
-static void exynos_iommu_tlb_invalidate_entry(struct exynos_iommu_domain *priv, - sysmmu_iova_t iova, size_t size) +static +void exynos_iommu_tlb_invalidate_entry(struct exynos_iommu_domain *domain, + sysmmu_iova_t iova, size_t size) { struct sysmmu_drvdata *data; unsigned long flags;
- spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&domain->lock, flags);
- list_for_each_entry(data, &priv->clients, domain_node) + list_for_each_entry(data, &domain->clients, domain_node) sysmmu_tlb_invalidate_entry(data, iova, size);
- spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&domain->lock, flags); }
-static size_t exynos_iommu_unmap(struct iommu_domain *domain, +static size_t exynos_iommu_unmap(struct iommu_domain *iommu_domain, unsigned long l_iova, size_t size) { - struct exynos_iommu_domain *priv = domain->priv; + struct exynos_iommu_domain *domain = iommu_domain->priv; sysmmu_iova_t iova = (sysmmu_iova_t)l_iova; sysmmu_pte_t *ent; size_t err_pgsize; unsigned long flags;
- BUG_ON(priv->pgtable == NULL); + BUG_ON(domain->pgtable == NULL);
- spin_lock_irqsave(&priv->pgtablelock, flags); + spin_lock_irqsave(&domain->pgtablelock, flags);
- ent = section_entry(priv->pgtable, iova); + ent = section_entry(domain->pgtable, iova);
if (lv1ent_section(ent)) { if (WARN_ON(size < SECT_SIZE)) { @@ -999,7 +1002,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, *ent = 0; size = SPAGE_SIZE; pgtable_flush(ent, ent + 1); - priv->lv2entcnt[lv1ent_offset(iova)] += 1; + domain->lv2entcnt[lv1ent_offset(iova)] += 1; goto done; }
@@ -1013,15 +1016,15 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, pgtable_flush(ent, ent + SPAGES_PER_LPAGE);
size = LPAGE_SIZE; - priv->lv2entcnt[lv1ent_offset(iova)] += SPAGES_PER_LPAGE; + domain->lv2entcnt[lv1ent_offset(iova)] += SPAGES_PER_LPAGE; done: - spin_unlock_irqrestore(&priv->pgtablelock, flags); + spin_unlock_irqrestore(&domain->pgtablelock, flags);
- exynos_iommu_tlb_invalidate_entry(priv, iova, size); + exynos_iommu_tlb_invalidate_entry(domain, iova, size);
return size; err: - spin_unlock_irqrestore(&priv->pgtablelock, flags); + spin_unlock_irqrestore(&domain->pgtablelock, flags);
pr_err("%s: Failed: size(%#zx) @ %#x is smaller than page size %#zx\n", __func__, size, iova, err_pgsize); @@ -1029,17 +1032,17 @@ err: return 0; }
-static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain, +static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *iommu_domain, dma_addr_t iova) { - struct exynos_iommu_domain *priv = domain->priv; + struct exynos_iommu_domain *domain = iommu_domain->priv; sysmmu_pte_t *entry; unsigned long flags; phys_addr_t phys = 0;
- spin_lock_irqsave(&priv->pgtablelock, flags); + spin_lock_irqsave(&domain->pgtablelock, flags);
- entry = section_entry(priv->pgtable, iova); + entry = section_entry(domain->pgtable, iova);
if (lv1ent_section(entry)) { phys = section_phys(entry) + section_offs(iova); @@ -1052,7 +1055,7 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain, phys = spage_phys(entry) + spage_offs(iova); }
- spin_unlock_irqrestore(&priv->pgtablelock, flags); + spin_unlock_irqrestore(&domain->pgtablelock, flags);
return phys; }
Add a few words of comment to all internal structures used by the driver.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 49 +++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 4c9b9b9e099f..a5b32cdeec2d 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -184,32 +184,49 @@ static char *sysmmu_fault_name[SYSMMU_FAULTS_NUM] = { "UNKNOWN FAULT" };
-/* attached to dev.archdata.iommu of the master device */ +/* + * This structure is attached to dev.archdata.iommu of the master device + * on device add, contains a list of SYSMMU controllers defined by device tree, + * which are bound to given master device. It is usually referenced by 'owner' + * pointer. + */ struct exynos_iommu_owner { - struct list_head clients; + struct list_head clients; /* list of sysmmu_drvdata.owner_node */ };
+/* + * This structure is stored in ->priv field of generic struct iommu_domain, + * contains list of SYSMMU controllers from all master devices, which has been + * attached to this domain and page tables of IO address space defined by this + * domain. It is usually referenced by 'domain' pointer. + */ struct exynos_iommu_domain { - struct list_head clients; /* list of sysmmu_drvdata.node */ + struct list_head clients; /* list of sysmmu_drvdata.domain_node */ sysmmu_pte_t *pgtable; /* lv1 page table, 16KB */ short *lv2entcnt; /* free lv2 entry counter for each section */ - spinlock_t lock; /* lock for this structure */ + spinlock_t lock; /* lock for modyfying list of clients */ spinlock_t pgtablelock; /* lock for modifying page table @ pgtable */ };
+/* + * This structure hold all data of a single SYSMMU controller, this includes + * hw resources like registers and clocks, pointers and list nodes to connect + * it to all other structures, internal state and parameters read from device + * tree. It is usually referenced by 'data' pointer. + */ struct sysmmu_drvdata { - struct device *sysmmu; /* System MMU's device descriptor */ - struct device *master; /* Owner of system MMU */ - void __iomem *sfrbase; - struct clk *clk; - struct clk *clk_master; - int activations; - spinlock_t lock; - struct iommu_domain *domain; - struct list_head domain_node; - struct list_head owner_node; - phys_addr_t pgtable; - int version; + struct device *sysmmu; /* SYSMMU controller device */ + struct device *master; /* master device (owner of given SYSMMU) */ + void __iomem *sfrbase; /* our registers */ + struct clk *clk; /* SYSMMU's clock */ + struct clk *clk_master; /* master's device clock */ + int activations; /* number of calls to sysmmu_enable */ + spinlock_t lock; /* lock for modyfying enable/disable state */ + struct iommu_domain *domain; /* domain we belong to */ + struct list_head domain_node; /* node for domain clients list */ + struct list_head owner_node; /* node for owner clients list */ + phys_addr_t pgtable; /* assigned page table structure */ + int version; /* our version */ };
static bool set_sysmmu_active(struct sysmmu_drvdata *data)
Removed following unused includes: <linux/mm.h>, <linux/errno.h>, <linux/memblock.h> and <linux/export.h>.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index a5b32cdeec2d..cd28dc09db39 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -12,19 +12,15 @@ #define DEBUG #endif
-#include <linux/io.h> -#include <linux/interrupt.h> -#include <linux/platform_device.h> -#include <linux/slab.h> -#include <linux/pm_runtime.h> #include <linux/clk.h> #include <linux/err.h> -#include <linux/mm.h> +#include <linux/io.h> #include <linux/iommu.h> -#include <linux/errno.h> +#include <linux/interrupt.h> #include <linux/list.h> -#include <linux/memblock.h> -#include <linux/export.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/slab.h>
#include <asm/cacheflush.h> #include <asm/pgtable.h>
This patch introduces IOMMU_OF_DECLARE-based initialization to the driver, which replaces subsys_initcall-based procedure. exynos_iommu_of_setup ensures that each sysmmu controller is probed before its master device.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index cd28dc09db39..88f9afe641a0 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -13,16 +13,21 @@ #endif
#include <linux/clk.h> +#include <linux/dma-mapping.h> #include <linux/err.h> #include <linux/io.h> #include <linux/iommu.h> #include <linux/interrupt.h> #include <linux/list.h> +#include <linux/of.h> +#include <linux/of_iommu.h> +#include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/slab.h>
#include <asm/cacheflush.h> +#include <asm/dma-iommu.h> #include <asm/pgtable.h>
typedef u32 sysmmu_iova_t; @@ -1084,6 +1089,8 @@ static const struct iommu_ops exynos_iommu_ops = { .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, };
+static int init_done; + static int __init exynos_iommu_init(void) { int ret; @@ -1116,6 +1123,8 @@ static int __init exynos_iommu_init(void) goto err_set_iommu; }
+ init_done = true; + return 0; err_set_iommu: kmem_cache_free(lv2table_kmem_cache, zero_lv2_table); @@ -1125,4 +1134,21 @@ err_reg_driver: kmem_cache_destroy(lv2table_kmem_cache); return ret; } -subsys_initcall(exynos_iommu_init); + +static int __init exynos_iommu_of_setup(struct device_node *np) +{ + struct platform_device *pdev; + + if (!init_done) + exynos_iommu_init(); + + pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root); + if (IS_ERR(pdev)) + return PTR_ERR(pdev); + + of_iommu_set_ops(np, &exynos_iommu_ops); + return 0; +} + +IOMMU_OF_DECLARE(exynos_iommu_of, "samsung,exynos-sysmmu", + exynos_iommu_of_setup);
Hi Marek,
Thank you for the patch.
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
This patch introduces IOMMU_OF_DECLARE-based initialization to the driver, which replaces subsys_initcall-based procedure. exynos_iommu_of_setup ensures that each sysmmu controller is probed before its master device.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index cd28dc09db39..88f9afe641a0 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c
[snip]
@@ -1125,4 +1134,21 @@ err_reg_driver: kmem_cache_destroy(lv2table_kmem_cache); return ret; } -subsys_initcall(exynos_iommu_init);
+static int __init exynos_iommu_of_setup(struct device_node *np) +{
- struct platform_device *pdev;
- if (!init_done)
exynos_iommu_init();- pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
- if (IS_ERR(pdev))
return PTR_ERR(pdev);
If we end up having to create the IOMMU platform devices from within the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround to me. I wonder whether it wouldn't then be better to let the driver core instantiate the IOMMU platform device from DT as for all other devices, and use device notifiers to defer probe of the bus masters until the required IOMMU(s) are registered.
Will, what's your opinion on that ?
- of_iommu_set_ops(np, &exynos_iommu_ops);
- return 0;
+}
+IOMMU_OF_DECLARE(exynos_iommu_of, "samsung,exynos-sysmmu",
exynos_iommu_of_setup);
On Sun, Dec 14, 2014 at 02:45:36PM +0200, Laurent Pinchart wrote:
Hi Marek,
Thank you for the patch.
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
This patch introduces IOMMU_OF_DECLARE-based initialization to the driver, which replaces subsys_initcall-based procedure. exynos_iommu_of_setup ensures that each sysmmu controller is probed before its master device.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index cd28dc09db39..88f9afe641a0 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c
[snip]
@@ -1125,4 +1134,21 @@ err_reg_driver: kmem_cache_destroy(lv2table_kmem_cache); return ret; } -subsys_initcall(exynos_iommu_init);
+static int __init exynos_iommu_of_setup(struct device_node *np) +{
- struct platform_device *pdev;
- if (!init_done)
exynos_iommu_init();- pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
- if (IS_ERR(pdev))
return PTR_ERR(pdev);If we end up having to create the IOMMU platform devices from within the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround to me. I wonder whether it wouldn't then be better to let the driver core instantiate the IOMMU platform device from DT as for all other devices, and use device notifiers to defer probe of the bus masters until the required IOMMU(s) are registered.
Notifiers don't work very well for this. Notifier blocks are supposed to return a very limited number of values, so sneaking in a -EPROBE_DEFER isn't likely to work out very well.
This was in fact one of Hiroshi's proposals over a year ago and got refused because of those reasons. The next solution was to introduce a function, not very much unlike the of_iommu_configure() that would be called in the core prior to calling into the driver's ->probe() callback so that it could handle this at probe time (as opposed to device creation time). That way the core can easily defer probe if the IOMMU is not there yet. At the same time it can simply use the driver model without requiring per-architecture hacks or workarounds.
Note that there is really no need for any of this configuration or initialization to happen at device creation time. Drivers won't be able to use the IOMMU or DMA APIs until their .probe(), so handling this any earlier is completely unnecessary.
Thierry
On Sun, Dec 14, 2014 at 12:45:36PM +0000, Laurent Pinchart wrote:
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
This patch introduces IOMMU_OF_DECLARE-based initialization to the driver, which replaces subsys_initcall-based procedure. exynos_iommu_of_setup ensures that each sysmmu controller is probed before its master device.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index cd28dc09db39..88f9afe641a0 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c
[snip]
@@ -1125,4 +1134,21 @@ err_reg_driver: kmem_cache_destroy(lv2table_kmem_cache); return ret; } -subsys_initcall(exynos_iommu_init);
+static int __init exynos_iommu_of_setup(struct device_node *np) +{
- struct platform_device *pdev;
- if (!init_done)
exynos_iommu_init();- pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
- if (IS_ERR(pdev))
return PTR_ERR(pdev);If we end up having to create the IOMMU platform devices from within the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround to me. I wonder whether it wouldn't then be better to let the driver core instantiate the IOMMU platform device from DT as for all other devices, and use device notifiers to defer probe of the bus masters until the required IOMMU(s) are registered.
Will, what's your opinion on that ?
Creating the platform device manually for the IOMMU is indeed grotty, but I don't really understand why it's needed. Interrupt controllers, for example, seem to get by without one.
Will
Hi Will,
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
On Sun, Dec 14, 2014 at 12:45:36PM +0000, Laurent Pinchart wrote:
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
This patch introduces IOMMU_OF_DECLARE-based initialization to the driver, which replaces subsys_initcall-based procedure. exynos_iommu_of_setup ensures that each sysmmu controller is probed before its master device.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index cd28dc09db39..88f9afe641a0 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c
[snip]
@@ -1125,4 +1134,21 @@ err_reg_driver: kmem_cache_destroy(lv2table_kmem_cache); return ret; }
-subsys_initcall(exynos_iommu_init);
+static int __init exynos_iommu_of_setup(struct device_node *np) +{
- struct platform_device *pdev;
- if (!init_done)
exynos_iommu_init();- pdev = of_platform_device_create(np, NULL,
platform_bus_type.dev_root);
- if (IS_ERR(pdev))
return PTR_ERR(pdev);If we end up having to create the IOMMU platform devices from within the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround to me. I wonder whether it wouldn't then be better to let the driver core instantiate the IOMMU platform device from DT as for all other devices, and use device notifiers to defer probe of the bus masters until the required IOMMU(s) are registered.
Will, what's your opinion on that ?
Creating the platform device manually for the IOMMU is indeed grotty, but I don't really understand why it's needed. Interrupt controllers, for example, seem to get by without one.
There's several reasons, one of the most compelling ones I can think of at the moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers instead. Note that IRQ controllers that are further away from the CPU (such as GPIO-based IRQ controllers) are real platform devices and use runtime PM.
IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote:
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
On Sun, Dec 14, 2014 at 12:45:36PM +0000, Laurent Pinchart wrote:
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
+static int __init exynos_iommu_of_setup(struct device_node *np) +{
- struct platform_device *pdev;
- if (!init_done)
exynos_iommu_init();- pdev = of_platform_device_create(np, NULL,
platform_bus_type.dev_root);
- if (IS_ERR(pdev))
return PTR_ERR(pdev);If we end up having to create the IOMMU platform devices from within the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround to me. I wonder whether it wouldn't then be better to let the driver core instantiate the IOMMU platform device from DT as for all other devices, and use device notifiers to defer probe of the bus masters until the required IOMMU(s) are registered.
Will, what's your opinion on that ?
Creating the platform device manually for the IOMMU is indeed grotty, but I don't really understand why it's needed. Interrupt controllers, for example, seem to get by without one.
There's several reasons, one of the most compelling ones I can think of at the moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers instead. Note that IRQ controllers that are further away from the CPU (such as GPIO-based IRQ controllers) are real platform devices and use runtime PM.
Ok, that's a good point, but the IOMMU will still probe later anyway, right?
IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs.
The usual answer is `we should model buses properly', but that's really not practical afaict.
Will
Hi Will,
On Monday 15 December 2014 17:43:02 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote:
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
On Sun, Dec 14, 2014 at 12:45:36PM +0000, Laurent Pinchart wrote:
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
+static int __init exynos_iommu_of_setup(struct device_node *np) +{
- struct platform_device *pdev;
- if (!init_done)
exynos_iommu_init();- pdev = of_platform_device_create(np, NULL,
platform_bus_type.dev_root);
- if (IS_ERR(pdev))
return PTR_ERR(pdev);If we end up having to create the IOMMU platform devices from within the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround to me. I wonder whether it wouldn't then be better to let the driver core instantiate the IOMMU platform device from DT as for all other devices, and use device notifiers to defer probe of the bus masters until the required IOMMU(s) are registered.
Will, what's your opinion on that ?
Creating the platform device manually for the IOMMU is indeed grotty, but I don't really understand why it's needed. Interrupt controllers, for example, seem to get by without one.
There's several reasons, one of the most compelling ones I can think of at the moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers instead. Note that IRQ controllers that are further away from the CPU (such as GPIO-based IRQ controllers) are real platform devices and use runtime PM.
Ok, that's a good point, but the IOMMU will still probe later anyway, right?
That depends on the driver implementation, using OF node match an IOMMU driver doesn't have to register a struct driver. Assuming we require IOMMU drivers to register a struct driver, a platform device should then be probed at a later time.
However, if we wait until the IOMMU gets probed to initialize runtime PM and make it functional, we'll be back in square one if the IOMMU gets probed after the bus master, as the bus master could start issuing bus requests at probe time with the IOMMU not powered yet.
IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could we postpone it to just before probe time ?
The usual answer is `we should model buses properly', but that's really not practical afaict.
On Mon, Dec 15, 2014 at 05:53:48PM +0000, Laurent Pinchart wrote:
Hi Will,
Hello again :)
On Monday 15 December 2014 17:43:02 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote:
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
Creating the platform device manually for the IOMMU is indeed grotty, but I don't really understand why it's needed. Interrupt controllers, for example, seem to get by without one.
There's several reasons, one of the most compelling ones I can think of at the moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers instead. Note that IRQ controllers that are further away from the CPU (such as GPIO-based IRQ controllers) are real platform devices and use runtime PM.
Ok, that's a good point, but the IOMMU will still probe later anyway, right?
That depends on the driver implementation, using OF node match an IOMMU driver doesn't have to register a struct driver. Assuming we require IOMMU drivers to register a struct driver, a platform device should then be probed at a later time.
However, if we wait until the IOMMU gets probed to initialize runtime PM and make it functional, we'll be back in square one if the IOMMU gets probed after the bus master, as the bus master could start issuing bus requests at probe time with the IOMMU not powered yet.
True, but couldn't the early init code do enough to get the thing functional? That said, I'm showing my ignorance here as I'm not familiar with the PM code (and the software models I have for the SMMU clearly don't implement anything in this regard).
IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
Will
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:53:48PM +0000, Laurent Pinchart wrote:
Hi Will,
Hello again :)
On Monday 15 December 2014 17:43:02 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote:
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
Creating the platform device manually for the IOMMU is indeed grotty, but I don't really understand why it's needed. Interrupt controllers, for example, seem to get by without one.
There's several reasons, one of the most compelling ones I can think of at the moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers instead. Note that IRQ controllers that are further away from the CPU (such as GPIO-based IRQ controllers) are real platform devices and use runtime PM.
Ok, that's a good point, but the IOMMU will still probe later anyway, right?
That depends on the driver implementation, using OF node match an IOMMU driver doesn't have to register a struct driver. Assuming we require IOMMU drivers to register a struct driver, a platform device should then be probed at a later time.
However, if we wait until the IOMMU gets probed to initialize runtime PM and make it functional, we'll be back in square one if the IOMMU gets probed after the bus master, as the bus master could start issuing bus requests at probe time with the IOMMU not powered yet.
True, but couldn't the early init code do enough to get the thing functional? That said, I'm showing my ignorance here as I'm not familiar with the PM code (and the software models I have for the SMMU clearly don't implement anything in this regard).
We're reaching the limits of my knowledge as well. If the IOMMU is in a power domain different than the bus masters the driver would at least need to ensure that the power domain is turned on, which might be a bit hackish without runtime PM.
IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
Hello,
On 2014-12-15 19:19, Laurent Pinchart wrote:
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:53:48PM +0000, Laurent Pinchart wrote:
On Monday 15 December 2014 17:43:02 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote:
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
Creating the platform device manually for the IOMMU is indeed grotty, but I don't really understand why it's needed. Interrupt controllers, for example, seem to get by without one.
There's several reasons, one of the most compelling ones I can think of at the moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers instead. Note that IRQ controllers that are further away from the CPU (such as GPIO-based IRQ controllers) are real platform devices and use runtime PM.
Ok, that's a good point, but the IOMMU will still probe later anyway, right?
That depends on the driver implementation, using OF node match an IOMMU driver doesn't have to register a struct driver. Assuming we require IOMMU drivers to register a struct driver, a platform device should then be probed at a later time.
However, if we wait until the IOMMU gets probed to initialize runtime PM and make it functional, we'll be back in square one if the IOMMU gets probed after the bus master, as the bus master could start issuing bus requests at probe time with the IOMMU not powered yet.
True, but couldn't the early init code do enough to get the thing functional? That said, I'm showing my ignorance here as I'm not familiar with the PM code (and the software models I have for the SMMU clearly don't implement anything in this regard).
We're reaching the limits of my knowledge as well. If the IOMMU is in a power domain different than the bus masters the driver would at least need to ensure that the power domain is turned on, which might be a bit hackish without runtime PM.
In case of Exynos SoC both IOMMU and master device are in the same power domain. During iommu_attach() call driver needs to have power domain enabled, but power domain driver is probed from arch_initcall(). I wanted to move power domain initialization earlier to ensure that it will happen before IOMMU driver probe, that not really a problem. Right now it usually works, because power domains are enabled by bootloader.
However please not that I would really like to have something merged. The discussion about iommu controllers lasts for about 2 years and we still don't have ANYTHING working in mainline, so lets merge the of_iommu glue and then check how the remaining issues can be solved.
Best regards
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
I believe we could postpone it to probe time, but I'd rather not. The way I see the arguments in favor, we have mainly cosmetic arguments:
- Doing it at probe time would eliminate the ugly section magic hack - iommu drivers could be implemented as loadable modules with platform drivers, for consistency with most other drivers
On the other hand, I see:
- DMA configuration is traditionally done at device creation time, and changing that is more likely to introduce bugs than leaving it where it is. - On a lot of machines, the IOMMU is optional, and the probe function cannot know the difference between an IOMMU driver that is left out of the kernel and one that will be initialized later, using a fixed entry point for initializing the IOMMU makes the behavior consistent
There is a third option in theory, which is to only enable the IOMMU as part of dma_set_mask(). I've done that in the past on powerpc, but the new approach seems cleaner.
Arnd
Hi Arnd,
On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote:
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
I believe we could postpone it to probe time, but I'd rather not. The way I see the arguments in favor, we have mainly cosmetic arguments:
- Doing it at probe time would eliminate the ugly section magic hack
- iommu drivers could be implemented as loadable modules with platform drivers, for consistency with most other drivers
The main argument, from my point of view, is that handling IOMMUs are normal platform devices allow using all the kernel infrastructure that depends on a struct device. The dev_* print helpers are nice to have, but what would make a big difference is the ability to use runtime PM.
On the other hand, I see:
- DMA configuration is traditionally done at device creation time, and changing that is more likely to introduce bugs than leaving it where it is.
- On a lot of machines, the IOMMU is optional, and the probe function cannot know the difference between an IOMMU driver that is left out of the kernel and one that will be initialized later, using a fixed entry point for initializing the IOMMU makes the behavior consistent
I'm not advocating for IOMMU support being built as a loadable module. It might be nice from a development point of view, but that's about it.
There is a third option in theory, which is to only enable the IOMMU as part of dma_set_mask(). I've done that in the past on powerpc, but the new approach seems cleaner.
On Tuesday 16 December 2014 14:07:11 Laurent Pinchart wrote:
Hi Arnd,
On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote:
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
I believe we could postpone it to probe time, but I'd rather not. The way I see the arguments in favor, we have mainly cosmetic arguments:
- Doing it at probe time would eliminate the ugly section magic hack
- iommu drivers could be implemented as loadable modules with platform drivers, for consistency with most other drivers
The main argument, from my point of view, is that handling IOMMUs are normal platform devices allow using all the kernel infrastructure that depends on a struct device. The dev_* print helpers are nice to have, but what would make a big difference is the ability to use runtime PM.
Right, I agree that would be nice.
On the other hand, I see:
- DMA configuration is traditionally done at device creation time, and changing that is more likely to introduce bugs than leaving it where it is.
- On a lot of machines, the IOMMU is optional, and the probe function cannot know the difference between an IOMMU driver that is left out of the kernel and one that will be initialized later, using a fixed entry point for initializing the IOMMU makes the behavior consistent
I'm not advocating for IOMMU support being built as a loadable module. It might be nice from a development point of view, but that's about it.
We'd still have to deal with deferred probing, or with ordering the iommu initcalls before the dma master initcalls in some other way, so the problem is not that different from loadable modules. Do you have an idea for how to solve that?
Arnd
Hi Arnd,
On Tuesday 16 December 2014 13:10:59 Arnd Bergmann wrote:
On Tuesday 16 December 2014 14:07:11 Laurent Pinchart wrote:
On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote:
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
> IOMMUs are not as low-level as system interrupt controllers or > system clocks. I'm beginning to agree with Thierry that they should > be treated as normal platform devices as they're not required > earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
I believe we could postpone it to probe time, but I'd rather not. The way I see the arguments in favor, we have mainly cosmetic arguments:
- Doing it at probe time would eliminate the ugly section magic hack
- iommu drivers could be implemented as loadable modules with platform drivers, for consistency with most other drivers
The main argument, from my point of view, is that handling IOMMUs are normal platform devices allow using all the kernel infrastructure that depends on a struct device. The dev_* print helpers are nice to have, but what would make a big difference is the ability to use runtime PM.
Right, I agree that would be nice.
On the other hand, I see:
DMA configuration is traditionally done at device creation time, and changing that is more likely to introduce bugs than leaving it where it is.
On a lot of machines, the IOMMU is optional, and the probe function cannot know the difference between an IOMMU driver that is left out of the kernel and one that will be initialized later, using a fixed entry point for initializing the IOMMU makes the behavior consistent
I'm not advocating for IOMMU support being built as a loadable module. It might be nice from a development point of view, but that's about it.
We'd still have to deal with deferred probing, or with ordering the iommu initcalls before the dma master initcalls in some other way, so the problem is not that different from loadable modules. Do you have an idea for how to solve that?
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
Arnd
Hi Arnd,
On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ?
On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote:
On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ?
It's a policy decision that should only depend on the user. Modifying the DT is wrong here IMHO because the device is still connected to the IOMMU in hardware and we should correctly represent that.
You can normally set 'iommu=off' or 'iommu=force' on the command line to force it on or off rather than letting the IOMMU driver decide whether the device turns it on. Not enabling the IOMMU at all should really just behave the same way as 'iommu=off', anything else would be very confusing.
Arnd
Hi Arnd,
On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote:
On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote:
On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ?
It's a policy decision that should only depend on the user. Modifying the DT is wrong here IMHO because the device is still connected to the IOMMU in hardware and we should correctly represent that.
I was thinking about setting status = "disabled" on the IOMMU nodes, not removing the IOMMU references in the bus master nodes.
You can normally set 'iommu=off' or 'iommu=force' on the command line to force it on or off rather than letting the IOMMU driver decide whether the device turns it on. Not enabling the IOMMU at all should really just behave the same way as 'iommu=off', anything else would be very confusing.
Setting iommu=off sounds fine to me. The IOMMU core would then return a "no IOMMU present" error instead of -EPROBE_DEFER, and probing of the bus masters would continue without IOMMU support.
On Wednesday 17 December 2014 18:02:51 Laurent Pinchart wrote:
On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote:
On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote:
On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ?
It's a policy decision that should only depend on the user. Modifying the DT is wrong here IMHO because the device is still connected to the IOMMU in hardware and we should correctly represent that.
I was thinking about setting status = "disabled" on the IOMMU nodes, not removing the IOMMU references in the bus master nodes.
But that still requires a modification of the DT. The hardware is the same, so I don't see why we should update the dtb based on kernel configuration.
Arnd
Hi Arnd,
On Wednesday 17 December 2014 22:58:47 Arnd Bergmann wrote:
On Wednesday 17 December 2014 18:02:51 Laurent Pinchart wrote:
On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote:
On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote:
On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ?
It's a policy decision that should only depend on the user. Modifying the DT is wrong here IMHO because the device is still connected to the IOMMU in hardware and we should correctly represent that.
I was thinking about setting status = "disabled" on the IOMMU nodes, not removing the IOMMU references in the bus master nodes.
But that still requires a modification of the DT. The hardware is the same, so I don't see why we should update the dtb based on kernel configuration.
It wouldn't be the first time we encode configuration information in DT, but I agree it's not an optimal solution. Setting iommu=off on the kernel command line is better, and should be easy to implement.
Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann:
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
This is similar to the problems we discussed with the componentized device framework and in the end everybody agreed on a simple rule: if the device node is enabled in the DT there must be a driver bound to it before other devices depending on this node can be probed.
If we apply the same logic to the IOMMU situation we get two possibilities to allow bypassing the IOMMU: 1. completely disable the IOMMU node in the DT 2. leave node enabled in DT, but add a bypass property
Obviously the second option still requires to have the IOMMU driver available, but is more flexible. Right now everything depends on the IOMMU being in passthrough mode at reset with no driver bound. If a device comes around where this isn't the case thing will break with the current assumptions or the first option.
Having the IOMMU node enabled in DT with no driver available to the kernel seems like an invalid configuration which should be expected to break. Exactly the same thing as with componentized devices...
Regards, Lucas
On Wednesday 17 December 2014 15:53:25 Lucas Stach wrote:
Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann:
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
This is similar to the problems we discussed with the componentized device framework and in the end everybody agreed on a simple rule: if the device node is enabled in the DT there must be a driver bound to it before other devices depending on this node can be probed.
If we apply the same logic to the IOMMU situation we get two possibilities to allow bypassing the IOMMU:
- completely disable the IOMMU node in the DT
- leave node enabled in DT, but add a bypass property
Obviously the second option still requires to have the IOMMU driver available, but is more flexible. Right now everything depends on the IOMMU being in passthrough mode at reset with no driver bound. If a device comes around where this isn't the case thing will break with the current assumptions or the first option.
Having the IOMMU node enabled in DT with no driver available to the kernel seems like an invalid configuration which should be expected to break. Exactly the same thing as with componentized devices...
One differences here is that for the IOMMU, we should generally be able to fall back to swiotlb (currently only on ARM64, not ARM32, until someone adds support). I can see your point regarding machines that have a mandatory IOMMU with no fallback when there is no driver, but we can support them by making the iommu driver selected through Kconfig for that platform, while still allowing other platforms to work with drivers left out of the kernel.
Arnd
Hi Arnd,
On Wednesday 17 December 2014 16:56:53 Arnd Bergmann wrote:
On Wednesday 17 December 2014 15:53:25 Lucas Stach wrote:
Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann:
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:
If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses.
This is similar to the problems we discussed with the componentized device framework and in the end everybody agreed on a simple rule: if the device node is enabled in the DT there must be a driver bound to it before other devices depending on this node can be probed.
If we apply the same logic to the IOMMU situation we get two possibilities to allow bypassing the IOMMU:
- completely disable the IOMMU node in the DT
- leave node enabled in DT, but add a bypass property
Obviously the second option still requires to have the IOMMU driver available, but is more flexible. Right now everything depends on the IOMMU being in passthrough mode at reset with no driver bound. If a device comes around where this isn't the case thing will break with the current assumptions or the first option.
Having the IOMMU node enabled in DT with no driver available to the kernel seems like an invalid configuration which should be expected to break. Exactly the same thing as with componentized devices...
One differences here is that for the IOMMU, we should generally be able to fall back to swiotlb
Or to nothing at all if the device can DMA to the allocated memory directly.
(currently only on ARM64, not ARM32, until someone adds support). I can see your point regarding machines that have a mandatory IOMMU with no fallback when there is no driver, but we can support them by making the iommu driver selected through Kconfig for that platform, while still allowing other platforms to work with drivers left out of the kernel.
The question is how to tell the kernel not to wait for an IOMMU that will never be there. Would a kernel command line argument be an acceptable solution or do we need something more automatic ?
On Thursday 18 December 2014 22:36:14 Laurent Pinchart wrote:
(currently only on ARM64, not ARM32, until someone adds support). I can see your point regarding machines that have a mandatory IOMMU with no fallback when there is no driver, but we can support them by making the iommu driver selected through Kconfig for that platform, while still allowing other platforms to work with drivers left out of the kernel.
The question is how to tell the kernel not to wait for an IOMMU that will never be there. Would a kernel command line argument be an acceptable solution or do we need something more automatic ?
I would hope that we can find an automatic solution without relying on the command line.
Unfortunately, I also remembered one case in support of what Lucas mentioned and that would break this: There is at least one SoC that uses cache-coherent DMA only when the IOMMU (ARM SMMU IIRC)is enabled, but is non-coherent otherwise. We can't really express that in DT, so we actually do rely on the IOMMU driver to be present on this machine when any "iommus" properties are used, as they would always come in combination with "dma-coherent" properties that are otherwise wrong.
We can still enforce this by requiring the smmu driver to be built into the kernel on this platform, but simply saying that the device cannot support DMA as long as there is an iommus property but no driver for it does make a lot of sense.
Note that there are more than two ways that the kernel could treat the situation of probing a device with a valid iommus reference but no driver loaded for the iommu:
a) assume all iommu drivers are initialized early, so use linear or swiotlb dma_map_ops, and probe the driver normally. This breaks the scenario with conditionally coherent devices, and requires doing the iommu init early b) assume all iommu drivers are initialized early, so disallow any DMA by setting a zero dma_mask but allow the driver to be probed using polling I/O mode (useful for slow devices like UART or SPI) This breaks devices that require DMA but can fall back to linear or swiotlb mappings, and requires doing the iommu init early. c) defer probing until an iommu driver is gets initialized. This breaks both the cases where we could fall back to a sane behavior without iommu, but it would let us use a proper driver with regular power management etc.
Arnd
This patch adds implementation of of_xlate callback, which prepares masters device for attaching to IOMMU. This callback is called during creating devices from device tree.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/iommu/exynos-iommu.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 88f9afe641a0..213db88ca52c 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -1078,6 +1078,33 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *iommu_domain, return phys; }
+static int exynos_iommu_of_xlate(struct device *dev, + struct of_phandle_args *spec) +{ + struct exynos_iommu_owner *owner = dev->archdata.iommu; + struct platform_device *sysmmu = of_find_device_by_node(spec->np); + struct sysmmu_drvdata *data; + + if (!sysmmu) + return -ENODEV; + + data = platform_get_drvdata(sysmmu); + if (!data) + return -ENODEV; + + if (!owner) { + owner = kzalloc(sizeof(*owner), GFP_KERNEL); + if (!owner) + return -ENOMEM; + + INIT_LIST_HEAD(&owner->clients); + dev->archdata.iommu = owner; + } + + list_add_tail(&data->owner_node, &owner->clients); + return 0; +} + static const struct iommu_ops exynos_iommu_ops = { .domain_init = exynos_iommu_domain_init, .domain_destroy = exynos_iommu_domain_destroy, @@ -1087,6 +1114,7 @@ static const struct iommu_ops exynos_iommu_ops = { .unmap = exynos_iommu_unmap, .iova_to_phys = exynos_iommu_iova_to_phys, .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, + .of_xlate = exynos_iommu_of_xlate, };
static int init_done;
Hey Marek, Inki,
On Wed, 2014-11-19 at 12:15 +0100, Marek Szyprowski wrote:
Hello Everyone,
This is another attempt to finally make Exynos SYSMMU driver fully integrated with DMA-mapping subsystem. The main change from previous version is a rebase onto latest "automatic DMA configuration for IOMMU masters" patches from Will Deacon.
Do you happen to know if anyone is working on iommu/dma-mapping patches for Exynos 5 based on this patchset?
For some background to that question, We (re-)discovered yesterday that the out-of-tree exynos-reference kernel iommu patches are required to get HDMI out working on exynos 5 boards. The current situation in mainline is rather broken, HDMI output without CONFIG_DRM_EXYNOS_IOMMU results in just displaying stripes[0]. While turning on CONFIG_DRM_EXYNOS_IOMMU causes a kernel oops at boot....
0: http://people.collabora.com/~sjoerd/14120001.jpg
Hello,
On 2014-12-02 10:59, Sjoerd Simons wrote:
Hey Marek, Inki,
On Wed, 2014-11-19 at 12:15 +0100, Marek Szyprowski wrote:
Hello Everyone,
This is another attempt to finally make Exynos SYSMMU driver fully integrated with DMA-mapping subsystem. The main change from previous version is a rebase onto latest "automatic DMA configuration for IOMMU masters" patches from Will Deacon.
Do you happen to know if anyone is working on iommu/dma-mapping patches for Exynos 5 based on this patchset?
I hope to add Exynos5 SYSMMU patches to the next iteration of my patchset, but I doubt it will get into v3.19-rc1.
For some background to that question, We (re-)discovered yesterday that the out-of-tree exynos-reference kernel iommu patches are required to get HDMI out working on exynos 5 boards. The current situation in mainline is rather broken, HDMI output without CONFIG_DRM_EXYNOS_IOMMU results in just displaying stripes[0]. While turning on CONFIG_DRM_EXYNOS_IOMMU causes a kernel oops at boot....
We have observed similar issues with Exynos4 based boards, when LCD0 power domain was turned off and only TV power domain has been powered on. Please check the power domain configuration. Maybe in case of Exynos5 the same issue is caused by the interaction between DISP1 and GSCL domains.
Best regards
Hello Marek,
On Fri, Dec 5, 2014 at 11:22 AM, Marek Szyprowski m.szyprowski@samsung.com wrote:
On 2014-12-02 10:59, Sjoerd Simons wrote:
This is another attempt to finally make Exynos SYSMMU driver fully integrated with DMA-mapping subsystem. The main change from previous version is a rebase onto latest "automatic DMA configuration for IOMMU masters" patches from Will Deacon.
Do you happen to know if anyone is working on iommu/dma-mapping patches for Exynos 5 based on this patchset?
I hope to add Exynos5 SYSMMU patches to the next iteration of my patchset, but I doubt it will get into v3.19-rc1.
For some background to that question, We (re-)discovered yesterday that the out-of-tree exynos-reference kernel iommu patches are required to get HDMI out working on exynos 5 boards. The current situation in mainline is rather broken, HDMI output without CONFIG_DRM_EXYNOS_IOMMU results in just displaying stripes[0]. While turning on CONFIG_DRM_EXYNOS_IOMMU causes a kernel oops at boot....
We have observed similar issues with Exynos4 based boards, when LCD0 power domain was turned off and only TV power domain has been powered on. Please check the power domain configuration. Maybe in case of Exynos5 the same
So IIUC what you are saying is that enabling the Exynos DRM IOMMU support has the side effect of turning on all the power domains needed by HDMI? I've compared the power domains configuration in mainline with the downstream exynos-reference [0] tree and I didn't find any differences.
issue is caused by the interaction between DISP1 and GSCL domains.
I was also not able to find a dependency betwen GSCL and DISP1 power domains in the Exynos 5420 manual. But If that's the case then your patch to add support for sub-power domains on Exynos [1] will also be needed, right?
AFAICT from the manual, all the used modules in the case of HDMI + Display (LCD, DP, HDMI and MIXER) needs only the DISP1 power domain to be enabled which BTW was removed for Exynos5420 on commit d51cad7 ("ARM: dts: remove display power domain for exynos5420"). It seems to work just because the power domain is turned on by the bootloader.
Also I tried forcing the kernel to not disable unused power domains by passing the pd_ignore_unused parameter to the kernel command line. I see on the kernel log a "genpd: Not disabling unused power domains" message but HDMI output still has the stripes that Sjoerd mentioned. Do you know if Exynos DRM HDMI in mainline is supposed to work without SysMMU / IOMMU support?
Thanks a lot for your help and suggestions.
Best regards
Marek Szyprowski, PhD Samsung R&D Institute Poland
Best regards, Javier
[0]: https://github.com/exynos-reference/kernel/commits/exynos5-v3.17-rc1-chrome [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308335.h...
On 01/06/2015 06:49 PM, Javier Martinez Canillas wrote:
Hello Marek,
On Fri, Dec 5, 2014 at 11:22 AM, Marek Szyprowski m.szyprowski@samsung.com wrote:
On 2014-12-02 10:59, Sjoerd Simons wrote:
This is another attempt to finally make Exynos SYSMMU driver fully integrated with DMA-mapping subsystem. The main change from previous version is a rebase onto latest "automatic DMA configuration for IOMMU masters" patches from Will Deacon.
Do you happen to know if anyone is working on iommu/dma-mapping patches for Exynos 5 based on this patchset?
I hope to add Exynos5 SYSMMU patches to the next iteration of my patchset, but I doubt it will get into v3.19-rc1.
For some background to that question, We (re-)discovered yesterday that the out-of-tree exynos-reference kernel iommu patches are required to get HDMI out working on exynos 5 boards. The current situation in mainline is rather broken, HDMI output without CONFIG_DRM_EXYNOS_IOMMU results in just displaying stripes[0]. While turning on CONFIG_DRM_EXYNOS_IOMMU causes a kernel oops at boot....
We have observed similar issues with Exynos4 based boards, when LCD0 power domain was turned off and only TV power domain has been powered on. Please check the power domain configuration. Maybe in case of Exynos5 the same
So IIUC what you are saying is that enabling the Exynos DRM IOMMU support has the side effect of turning on all the power domains needed by HDMI? I've compared the power domains configuration in mainline with the downstream exynos-reference [0] tree and I didn't find any differences.
issue is caused by the interaction between DISP1 and GSCL domains.
I was also not able to find a dependency betwen GSCL and DISP1 power domains in the Exynos 5420 manual. But If that's the case then your patch to add support for sub-power domains on Exynos [1] will also be needed, right?
AFAICT from the manual, all the used modules in the case of HDMI + Display (LCD, DP, HDMI and MIXER) needs only the DISP1 power domain to be enabled which BTW was removed for Exynos5420 on commit d51cad7 ("ARM: dts: remove display power domain for exynos5420"). It seems to work just because the power domain is turned on by the bootloader.
Also I tried forcing the kernel to not disable unused power domains by passing the pd_ignore_unused parameter to the kernel command line. I see on the kernel log a "genpd: Not disabling unused power domains" message but HDMI output still has the stripes that Sjoerd mentioned. Do you know if Exynos DRM HDMI in mainline is supposed to work without SysMMU / IOMMU support?
I don't think iommu support and power domain issue are related. I also get displaying stripes via hdmi but it is just power domain issue regardless iommu support.
I observed 8th bit from 0x1445000C register of mixer is set to 1 with displaying stripes. It means "The graphic layer0 line buffer underflow". There was same underflow issue on Exynos4 based boards. As Marek said, because LCD0 power domain was turned off.
I just tried to turn off DISP1 power domain at u-boot and DISP1 power domain is turned on from kernel hdmi and mixer driver on odroid xu3 board. As the result, i can see displaying penguin logo from hdmi.
But the problem exists still because it is failed to control on/off of DISP1 power domain more than twice from kernel hdmi and mixer driver.[0]
Thanks.
[0]: [ 63.477922] Power domain power-domain disable failed [ 63.481416] power-domain: Power-off latency exceeded, new value 10613042 ns [ 63.499805] power-domain: Power-on latency exceeded, new value 8863333 ns [ 63.506766] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0280000 [ 63.514191] Internal error: : 1008 [#1] PREEMPT SMP ARM [ 63.519386] Modules linked in: [ 63.522425] CPU: 0 PID: 1499 Comm: modetest Not tainted 3.18.1-00007-g04f5a4b-dirty #13 [ 63.530393] task: eddd3400 ti: edfd0000 task.ti: edfd0000 [ 63.535779] PC is at mixer_dpms+0x1dc/0x704 [ 63.539931] LR is at __mutex_unlock_slowpath+0xa8/0x194 [ 63.545125] pc : [<c02c0540>] lr : [<c04c3eb8>] psr: 60000013 [ 63.545125] sp : edfd1c98 ip : ee709ef4 fp : ee752aa0 [ 63.556561] r10: c07124b8 r9 : ee752800 r8 : c0712a38 [ 63.561761] r7 : c0712a38 r6 : ee451a80 r5 : ee523a28 r4 : ee523a10 [ 63.568260] r3 : f0280000 r2 : eddd3400 r1 : 035c035b r0 : ee523a28 [ 63.574760] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [ 63.581864] Control: 10c5387d Table: 6d81006a DAC: 00000015 [ 63.587584] Process modetest (pid: 1499, stack limit = 0xedfd0238) [ 63.593736] Stack: (0xedfd1c98 to 0xedfd2000) [ 63.598070] 1c80: c02c0364 c0766b8c [ 63.606219] 1ca0: 00000000 edf8d000 c0712a38 ee753c00 ee752800 c07124b8 ee752aa0 c02b5118 [ 63.614365] 1cc0: edf8d900 ee422c08 ee422810 ee753c00 edf8d900 ee752800 ee753c00 c0712a38 [ 63.622510] 1ce0: edf8d000 00000001 edf8d900 c02b52c4 ee752a9c ee753c00 edf8d000 c02901cc [ 63.630656] 1d00: 00000000 00000000 00000001 edf8d014 00000000 00000000 00000000 ee753c68 [ 63.638801] 1d20: ee753c00 c0766b8c 00000000 00000000 00000018 dededede 00000000 30323931 [ 63.646946] 1d40: 38303178 00000030 00000000 00000000 00000000 00000000 00000000 00000000 [ 63.655092] 1d60: 00000048 00024414 00000780 000007d8 00000804 00000898 00000000 00000438 [ 63.663237] 1d80: 0000043c 00000441 00000465 00000000 00000005 00000256 00000150 00024414 [ 63.671382] 1da0: 00000780 00000780 00000898 000007d8 00000804 00000898 00000000 00000438 [ 63.679528] 1dc0: 00000438 00000465 0000043c 00000441 00000465 00000000 00000000 0000003c [ 63.687674] 1de0: 00000000 00000000 00000001 ee753c00 ee752a7c ee5a2b80 ee752a88 ee752aa0 [ 63.695820] 1e00: eddd2c00 c0766b8c 00000001 c0290d18 00000000 c0040374 ee753c10 edfd0000 [ 63.703965] 1e20: ee752800 00000001 00000000 ed82bec0 00000000 00000000 ee753c00 ee752aa0 [ 63.712110] 1e40: c07124b8 00000000 ee753c10 edfa8580 ee753c00 00000000 edc8b500 00000000 [ 63.720256] 1e60: ed8827a8 ee525580 ee752958 c02a5f58 ee5a2b80 00000001 00000000 edc8b500 [ 63.728402] 1e80: 00000000 c02971b8 ee752800 edc8b500 ee752800 edc8b500 ee75285c ee752800 [ 63.736547] 1ea0: ee752834 c02971f4 ee752800 c0766b8c ee75285c c029c3c0 ed8827ac ed882700 [ 63.744692] 1ec0: ee75285c c029c7c4 ee24b0e8 00000001 edfa8f80 ee752960 60000013 00000000 [ 63.752838] 1ee0: ee7a9508 ed882000 ede78d60 00000000 edf8d490 ee0bd7b8 00000008 ed882008 [ 63.760983] 1f00: 00000000 c00cbf78 00000000 00000000 eddd374c 00000000 c072ad84 eddd3400 [ 63.769129] 1f20: edfd0008 00000000 ed874e44 c003777c eddd375c eddd3400 edfd0000 ed874e00 [ 63.777274] 1f40: edfd0008 c0022af8 00000000 00000001 ed874e04 ee78d898 ee78d8a0 c00b1d2c [ 63.785420] 1f60: 000000dd edcdb700 00000000 edfd0000 000000f8 c000e824 edfd0000 00000000 [ 63.793566] 1f80: 00000000 c0023ee8 000a3d78 b6f2a770 b6f2a770 000000f8 c000e824 c0023f64 [ 63.801711] 1fa0: 000a3d78 c000e6a0 000a3d78 b6f2a770 00000000 000a3d64 00000008 00000000 [ 63.809857] 1fc0: 000a3d78 b6f2a770 b6f2a770 000000f8 b6e56248 00000000 0000fd4c 00000000 [ 63.818002] 1fe0: b6fa2000 bed38ba8 b6e1a4dc b6e883d4 60000010 00000000 ffffffff ffffffff [ 63.826162] [<c02c0540>] (mixer_dpms) from [<c02b5118>] (exynos_drm_crtc_dpms+0x6c/0x17c) [ 63.834296] [<c02b5118>] (exynos_drm_crtc_dpms) from [<c02b52c4>] (exynos_drm_crtc_commit+0x14/0x44) [ 63.843400] [<c02b52c4>] (exynos_drm_crtc_commit) from [<c02901cc>] (drm_crtc_helper_set_mode+0x3d0/0x51c) [ 63.853016] [<c02901cc>] (drm_crtc_helper_set_mode) from [<c0290d18>] (drm_crtc_helper_set_config+0x87c/0x9dc) [ 63.862982] [<c0290d18>] (drm_crtc_helper_set_config) from [<c02a5f58>] (drm_mode_set_config_internal+0x58/0xd4) [ 63.873119] [<c02a5f58>] (drm_mode_set_config_internal) from [<c02971b8>] (restore_fbdev_mode+0xcc/0xec) [ 63.882562] [<c02971b8>] (restore_fbdev_mode) from [<c02971f4>] (drm_fb_helper_restore_fbdev_mode_unlocked+0x1c/0x30) [ 63.893135] [<c02971f4>] (drm_fb_helper_restore_fbdev_mode_unlocked) from [<c029c3c0>] (drm_lastclose+0x34/0x118) [ 63.903358] [<c029c3c0>] (drm_lastclose) from [<c029c7c4>] (drm_release+0x320/0x4c8) [ 63.911071] [<c029c7c4>] (drm_release) from [<c00cbf78>] (__fput+0x80/0x1c8) [ 63.918095] [<c00cbf78>] (__fput) from [<c003777c>] (task_work_run+0xac/0xe4) [ 63.925204] [<c003777c>] (task_work_run) from [<c0022af8>] (do_exit+0x2cc/0x958) [ 63.932563] [<c0022af8>] (do_exit) from [<c0023ee8>] (do_group_exit+0x4c/0xb8) [ 63.939754] [<c0023ee8>] (do_group_exit) from [<c0023f64>] (__wake_up_parent+0x0/0x18) [ 63.947636] Code: e1a00005 e5c43011 eb080e99 e5943044 (e5936000) [ 63.953701] ---[ end trace d3294b7b867ca713 ]--- [ 63.958290] Fixing recursive fault but reboot is needed!
Hello Joonyoung,
On 01/07/2015 03:03 AM, Joonyoung Shim wrote:
On 01/06/2015 06:49 PM, Javier Martinez Canillas wrote:
Also I tried forcing the kernel to not disable unused power domains by passing the pd_ignore_unused parameter to the kernel command line. I see on the kernel log a "genpd: Not disabling unused power domains" message but HDMI output still has the stripes that Sjoerd mentioned. Do you know if Exynos DRM HDMI in mainline is supposed to work without SysMMU / IOMMU support?
I don't think iommu support and power domain issue are related. I also get displaying stripes via hdmi but it is just power domain issue regardless iommu support.
I observed 8th bit from 0x1445000C register of mixer is set to 1 with displaying stripes. It means "The graphic layer0 line buffer underflow". There was same underflow issue on Exynos4 based boards. As Marek said, because LCD0 power domain was turned off.
Interesting, thanks a lot for sharing this information.
I just tried to turn off DISP1 power domain at u-boot and DISP1 power domain is turned on from kernel hdmi and mixer driver on odroid xu3 board. As the result, i can see displaying penguin logo from hdmi.
Can you share the patches you are using to turn on the DISP1 power domain since AFAIU the kernel does not know about the DISP1 power domain after commit d51cad7df871 ("ARM: dts: remove display power domain for exynos5420").
I tried reverting that commit before so the kernel knows about the DISP1 power domain and booting with pd_ignore_unused but still had the stripes.
But the problem exists still because it is failed to control on/off of DISP1 power domain more than twice from kernel hdmi and mixer driver.[0]
Something that is not clear to me is how display panel is working on the Peach boards if this is a power domain issue since according to the manual both the modules used for display (LCD controller and DP) and the modules used for HDMI (MIXER and HDMI) belong to the same power domain (DISP1).
Or am I misunderstanding something?
Thanks a lot for your help and best regards, Javier
Hi Javier,
On 01/07/2015 06:33 PM, Javier Martinez Canillas wrote:
Hello Joonyoung,
On 01/07/2015 03:03 AM, Joonyoung Shim wrote:
On 01/06/2015 06:49 PM, Javier Martinez Canillas wrote:
Also I tried forcing the kernel to not disable unused power domains by passing the pd_ignore_unused parameter to the kernel command line. I see on the kernel log a "genpd: Not disabling unused power domains" message but HDMI output still has the stripes that Sjoerd mentioned. Do you know if Exynos DRM HDMI in mainline is supposed to work without SysMMU / IOMMU support?
I don't think iommu support and power domain issue are related. I also get displaying stripes via hdmi but it is just power domain issue regardless iommu support.
I observed 8th bit from 0x1445000C register of mixer is set to 1 with displaying stripes. It means "The graphic layer0 line buffer underflow". There was same underflow issue on Exynos4 based boards. As Marek said, because LCD0 power domain was turned off.
Interesting, thanks a lot for sharing this information.
I just tried to turn off DISP1 power domain at u-boot and DISP1 power domain is turned on from kernel hdmi and mixer driver on odroid xu3 board. As the result, i can see displaying penguin logo from hdmi.
Can you share the patches you are using to turn on the DISP1 power domain since AFAIU the kernel does not know about the DISP1 power domain after commit d51cad7df871 ("ARM: dts: remove display power domain for exynos5420").
I tried reverting that commit before so the kernel knows about the DISP1 power domain and booting with pd_ignore_unused but still had the stripes.
I add DISP1 power domain on dts and please refer below patch[0] with some modification on hdmi phy(Actually, i think this is not related). You also should disable DISP1 power domain from bootloader.
But the problem exists still because it is failed to control on/off of DISP1 power domain more than twice from kernel hdmi and mixer driver.[0]
Something that is not clear to me is how display panel is working on the Peach boards if this is a power domain issue since according to the manual both the modules used for display (LCD controller and DP) and the modules used for HDMI (MIXER and HDMI) belong to the same power domain (DISP1).
I don't know about that because i just tested on odroid xu3 board without display panel. Hmm, It can be any conditions to success on/off power domain e.g. power state of clocks and of on/off order display devices.
Is DISP1 power domain disabled on Peach boards to save power, not always on?
Or am I misunderstanding something?
Thanks a lot for your help and best regards, Javier
Thanks.
[0]: --- arch/arm/boot/dts/exynos5420.dtsi | 10 ++++++++++ drivers/clk/samsung/clk-exynos5420.c | 4 ++-- drivers/gpu/drm/exynos/exynos_hdmi.c | 8 ++------ include/dt-bindings/clock/exynos5420.h | 2 ++ 4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi index 8617a03..ff9ad4a 100644 --- a/arch/arm/boot/dts/exynos5420.dtsi +++ b/arch/arm/boot/dts/exynos5420.dtsi @@ -270,6 +270,14 @@ reg = <0x10044120 0x20>; };
+ disp1_pd: power-domain@100440C0 { + compatible = "samsung,exynos4210-pd"; + reg = <0x100440C0 0x20>; + clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MOUT_SW_ACLK200>, + <&clock CLK_MOUT_USER_ACLK200_DISP1>; + clock-names = "oscclk", "pclk0", "clk0"; + }; + pinctrl_0: pinctrl@13400000 { compatible = "samsung,exynos5420-pinctrl"; reg = <0x13400000 0x1000>; @@ -704,6 +712,7 @@ "sclk_hdmiphy", "mout_hdmi"; phy = <&hdmiphy>; samsung,syscon-phandle = <&pmu_system_controller>; + samsung,power-domain = <&disp1_pd>; status = "disabled"; };
@@ -717,6 +726,7 @@ interrupts = <0 94 0>; clocks = <&clock CLK_MIXER>, <&clock CLK_SCLK_HDMI>; clock-names = "mixer", "sclk_hdmi"; + samsung,power-domain = <&disp1_pd>; };
gsc_0: video-scaler@13e00000 { diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c index 848d602..52ba0e6 100644 --- a/drivers/clk/samsung/clk-exynos5420.c +++ b/drivers/clk/samsung/clk-exynos5420.c @@ -635,7 +635,7 @@ static struct samsung_mux_clock exynos5x_mux_clks[] __initdata = { SRC_TOP3, 0, 1), MUX(0, "mout_user_aclk400_mscl", mout_user_aclk400_mscl_p, SRC_TOP3, 4, 1), - MUX(0, "mout_user_aclk200_disp1", mout_user_aclk200_disp1_p, + MUX(CLK_MOUT_USER_ACLK200_DISP1, "mout_user_aclk200_disp1", mout_user_aclk200_disp1_p, SRC_TOP3, 8, 1), MUX(0, "mout_user_aclk200_fsys2", mout_user_aclk200_fsys2_p, SRC_TOP3, 12, 1), @@ -693,7 +693,7 @@ static struct samsung_mux_clock exynos5x_mux_clks[] __initdata = { SRC_TOP10, 0, 1), MUX(0, "mout_sw_aclk400_mscl", mout_sw_aclk400_mscl_p, SRC_TOP10, 4, 1), - MUX(0, "mout_sw_aclk200", mout_sw_aclk200_p, SRC_TOP10, 8, 1), + MUX(CLK_MOUT_SW_ACLK200, "mout_sw_aclk200", mout_sw_aclk200_p, SRC_TOP10, 8, 1), MUX(0, "mout_sw_aclk200_fsys2", mout_sw_aclk200_fsys2_p, SRC_TOP10, 12, 1), MUX(0, "mout_sw_aclk400_wcore", mout_sw_aclk400_wcore_p, diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 563a19e..f3cdf80 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1664,7 +1664,6 @@ static void hdmi_mode_apply(struct hdmi_context *hdata)
static void hdmiphy_conf_reset(struct hdmi_context *hdata) { - u8 buffer[2]; u32 reg;
clk_disable_unprepare(hdata->res.sclk_hdmi); @@ -1672,11 +1671,8 @@ static void hdmiphy_conf_reset(struct hdmi_context *hdata) clk_prepare_enable(hdata->res.sclk_hdmi);
/* operation mode */ - buffer[0] = 0x1f; - buffer[1] = 0x00; - - if (hdata->hdmiphy_port) - i2c_master_send(hdata->hdmiphy_port, buffer, 2); + hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE, + HDMI_PHY_ENABLE_MODE_SET);
if (hdata->type == HDMI_TYPE13) reg = HDMI_V13_PHY_RSTOUT; diff --git a/include/dt-bindings/clock/exynos5420.h b/include/dt-bindings/clock/exynos5420.h index 8dc0913..15b9bb2 100644 --- a/include/dt-bindings/clock/exynos5420.h +++ b/include/dt-bindings/clock/exynos5420.h @@ -204,6 +204,8 @@ #define CLK_MOUT_MAUDIO0 643 #define CLK_MOUT_USER_ACLK333 644 #define CLK_MOUT_SW_ACLK333 645 +#define CLK_MOUT_USER_ACLK200_DISP1 646 +#define CLK_MOUT_SW_ACLK200 647
/* divider clocks */ #define CLK_DOUT_PIXEL 768
Hello Joonyoung,
On 01/07/2015 10:55 AM, Joonyoung Shim wrote:
I don't think iommu support and power domain issue are related. I also get displaying stripes via hdmi but it is just power domain issue regardless iommu support.
I observed 8th bit from 0x1445000C register of mixer is set to 1 with displaying stripes. It means "The graphic layer0 line buffer underflow". There was same underflow issue on Exynos4 based boards. As Marek said, because LCD0 power domain was turned off.
Interesting, thanks a lot for sharing this information.
I just tried to turn off DISP1 power domain at u-boot and DISP1 power domain is turned on from kernel hdmi and mixer driver on odroid xu3 board. As the result, i can see displaying penguin logo from hdmi.
Can you share the patches you are using to turn on the DISP1 power domain since AFAIU the kernel does not know about the DISP1 power domain after commit d51cad7df871 ("ARM: dts: remove display power domain for exynos5420").
I tried reverting that commit before so the kernel knows about the DISP1 power domain and booting with pd_ignore_unused but still had the stripes.
I add DISP1 power domain on dts and please refer below patch[0] with some modification on hdmi phy(Actually, i think this is not related). You also should disable DISP1 power domain from bootloader.
Thanks a lot for the patch. With your changes I have some HDMI output on my Peach Pi. But the HDMI output is broken since the resolution is very low and the video as a lot of visual artifacts.
In your change you are also adding clock phandles for the pd oscillator clock and the CLK_MOUT_SW_ACLK200, CLK_MOUT_USER_ACLK200_DISP1 pairs for the parent input and input clocks of the devices attached to the pd.
And also making changes to the clocks in the clk-exynos5420 driver. Can you please explain the rationale for those changes? I'm asking because without your clock changes (only adding the DISP1 pd and making the devices as consumers), I've HDMI output too but video is even worse. This [0] is the minimal change I have on top of 3.19-rc3 to have some output.
The docs I've access to, don't have information about the clock hierarchy. So each time there is a clock issue, I've a hard time to figure out what's happening.
So there seems to be two issues here, one is the mixer and hdmi modules not being attached to the DISP1 power domain and another one is the clocks setup not being correct to have proper HDMI video output.
But the problem exists still because it is failed to control on/off of DISP1 power domain more than twice from kernel hdmi and mixer driver.[0]
I didn't have this issue when testing your patch against 3.19-rc2. From your log I see that you are testing on a 3.18.1. So maybe makes sense to test with the latest kernel version since this HDMI issue qualifies as an 3.19-rc fix?
Since commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes") that landed in 3.19-rc1, I see that the power domain is powered on when a device is attached. So maybe that is what makes a difference here?
Something that is not clear to me is how display panel is working on the Peach boards if this is a power domain issue since according to the manual both the modules used for display (LCD controller and DP) and the modules used for HDMI (MIXER and HDMI) belong to the same power domain (DISP1).
I don't know about that because i just tested on odroid xu3 board without display panel. Hmm, It can be any conditions to success on/off power domain e.g. power state of clocks and of on/off order display devices.
My guess is that this is again clock related. Since if I add the DISP1 pd to the DTS and make the fimd node as a consumer of the pd, the display panel works correctly (same than current mainline that doesn't have the node).
But, if I also make the mixer and hdmi nodes as consumer of the DISP1, then the exynos dp driver's probe function fails as shown in [1]. In this case, the HDMI output works and I see that the DISP1 power domain is not powered off so the only thing I can think is that is a clocks issue.
Is DISP1 power domain disabled on Peach boards to save power, not always on?
No AFAICT, currently as I mentioned before the DISP1 power domain was removed from the exynos5420.dtsi. So since the kernel does not know about it, should be always on. With your patch, it should not be disabled either since it has devices attached to and I confirmed that is not powered off anymore once all the attached devices' probe functions have been deferred and retried again.
Thanks a lot for your help and best regards, Javier
[0]: diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi index 517e50f6760b..9966fe7bb0ec 100644 --- a/arch/arm/boot/dts/exynos5420.dtsi +++ b/arch/arm/boot/dts/exynos5420.dtsi @@ -265,6 +265,11 @@ clock-names = "oscclk", "pclk0", "clk0"; };
+ disp_pd: power-domain@100440C0 { + compatible = "samsung,exynos4210-pd"; + reg = <0x100440C0 0x20>; + }; + msc_pd: power-domain@10044120 { compatible = "samsung,exynos4210-pd"; reg = <0x10044120 0x20>; @@ -535,6 +540,7 @@ };
fimd: fimd@14400000 { + samsung,power-domain = <&disp_pd>; clocks = <&clock CLK_SCLK_FIMD1>, <&clock CLK_FIMD1>; clock-names = "sclk_fimd", "fimd"; }; @@ -710,6 +716,7 @@ phy = <&hdmiphy>; samsung,syscon-phandle = <&pmu_system_controller>; status = "disabled"; + samsung,power-domain = <&disp_pd>; };
hdmiphy: hdmiphy@145D0000 { @@ -722,6 +729,7 @@ interrupts = <0 94 0>; clocks = <&clock CLK_MIXER>, <&clock CLK_SCLK_HDMI>; clock-names = "mixer", "sclk_hdmi"; + samsung,power-domain = <&disp_pd>; };
gsc_0: video-scaler@13e00000 {
[1]: exynos-dp 145b0000.dp-controller: EDID data does not include any extensions. exynos-dp 145b0000.dp-controller: EDID Read success! exynos-dp 145b0000.dp-controller: Link Training Clock Recovery success exynos-dp 145b0000.dp-controller: Link Training success! exynos-dp 145b0000.dp-controller: Timeout of video streamclk ok exynos-dp 145b0000.dp-controller: unable to config video
Hi,
On 01/09/2015 01:42 AM, Javier Martinez Canillas wrote:
Hello Joonyoung,
On 01/07/2015 10:55 AM, Joonyoung Shim wrote:
I don't think iommu support and power domain issue are related. I also get displaying stripes via hdmi but it is just power domain issue regardless iommu support.
I observed 8th bit from 0x1445000C register of mixer is set to 1 with displaying stripes. It means "The graphic layer0 line buffer underflow". There was same underflow issue on Exynos4 based boards. As Marek said, because LCD0 power domain was turned off.
Interesting, thanks a lot for sharing this information.
I just tried to turn off DISP1 power domain at u-boot and DISP1 power domain is turned on from kernel hdmi and mixer driver on odroid xu3 board. As the result, i can see displaying penguin logo from hdmi.
Can you share the patches you are using to turn on the DISP1 power domain since AFAIU the kernel does not know about the DISP1 power domain after commit d51cad7df871 ("ARM: dts: remove display power domain for exynos5420").
I tried reverting that commit before so the kernel knows about the DISP1 power domain and booting with pd_ignore_unused but still had the stripes.
I add DISP1 power domain on dts and please refer below patch[0] with some modification on hdmi phy(Actually, i think this is not related). You also should disable DISP1 power domain from bootloader.
Thanks a lot for the patch. With your changes I have some HDMI output on my Peach Pi. But the HDMI output is broken since the resolution is very low and the video as a lot of visual artifacts.
In your change you are also adding clock phandles for the pd oscillator clock and the CLK_MOUT_SW_ACLK200, CLK_MOUT_USER_ACLK200_DISP1 pairs for the parent input and input clocks of the devices attached to the pd.
And also making changes to the clocks in the clk-exynos5420 driver. Can you please explain the rationale for those changes? I'm asking because without your clock changes (only adding the DISP1 pd and making the devices as consumers), I've HDMI output too but video is even worse. This [0] is the minimal change I have on top of 3.19-rc3 to have some output.
I just refer below patches, http://comments.gmane.org/gmane.linux.kernel.samsung-soc/34576
But i'm not sure whether DISP1 power domain is same case with MFC power domain.
The docs I've access to, don't have information about the clock hierarchy. So each time there is a clock issue, I've a hard time to figure out what's happening.
So there seems to be two issues here, one is the mixer and hdmi modules not being attached to the DISP1 power domain and another one is the clocks setup not being correct to have proper HDMI video output.
Hmm, i can see normal hdmi output still from latest upstream kernel(3.19-rc4) with my kernel changes and u-boot changes(DISP1 power domain disable) of prior mail on odroid xu3 board.
But the problem exists still because it is failed to control on/off of DISP1 power domain more than twice from kernel hdmi and mixer driver.[0]
I didn't have this issue when testing your patch against 3.19-rc2. From your log I see that you are testing on a 3.18.1. So maybe makes sense to test with the latest kernel version since this HDMI issue qualifies as an 3.19-rc fix?
Since commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes") that landed in 3.19-rc1, I see that the power domain is powered on when a device is attached. So maybe that is what makes a difference here?
I'm not sure, but i get same error results from 3.19-rc4. Did you test using exynos drm driver? I used modetest of libdrm
Something that is not clear to me is how display panel is working on the Peach boards if this is a power domain issue since according to the manual both the modules used for display (LCD controller and DP) and the modules used for HDMI (MIXER and HDMI) belong to the same power domain (DISP1).
I don't know about that because i just tested on odroid xu3 board without display panel. Hmm, It can be any conditions to success on/off power domain e.g. power state of clocks and of on/off order display devices.
My guess is that this is again clock related. Since if I add the DISP1 pd to the DTS and make the fimd node as a consumer of the pd, the display panel works correctly (same than current mainline that doesn't have the node).
But, if I also make the mixer and hdmi nodes as consumer of the DISP1, then the exynos dp driver's probe function fails as shown in [1]. In this case, the HDMI output works and I see that the DISP1 power domain is not powered off so the only thing I can think is that is a clocks issue.
DP also is related with DISP1 power domain, so it think DP driver should consider DISP1 power domain.
Is DISP1 power domain disabled on Peach boards to save power, not always on?
No AFAICT, currently as I mentioned before the DISP1 power domain was removed from the exynos5420.dtsi. So since the kernel does not know about it, should be always on. With your patch, it should not be disabled either since it has devices attached to and I confirmed that is not powered off anymore once all the attached devices' probe functions have been deferred and retried again.
OK, finally, we should add DISP1 power domain in dts file and be able to control on/off of DISP1 power domain.
Thanks.
Thanks a lot for your help and best regards, Javier
[0]: diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi index 517e50f6760b..9966fe7bb0ec 100644 --- a/arch/arm/boot/dts/exynos5420.dtsi +++ b/arch/arm/boot/dts/exynos5420.dtsi @@ -265,6 +265,11 @@ clock-names = "oscclk", "pclk0", "clk0"; };
- disp_pd: power-domain@100440C0 {
compatible = "samsung,exynos4210-pd";reg = <0x100440C0 0x20>;- };
- msc_pd: power-domain@10044120 { compatible = "samsung,exynos4210-pd"; reg = <0x10044120 0x20>;
@@ -535,6 +540,7 @@ }; fimd: fimd@14400000 {
samsung,power-domain = <&disp_pd>;@@ -710,6 +716,7 @@ phy = <&hdmiphy>; samsung,syscon-phandle = <&pmu_system_controller>; status = "disabled";
samsung,power-domain = <&disp_pd>;hdmiphy: hdmiphy@145D0000 { @@ -722,6 +729,7 @@ interrupts = <0 94 0>; clocks = <&clock CLK_MIXER>, <&clock CLK_SCLK_HDMI>; clock-names = "mixer", "sclk_hdmi";
samsung,power-domain = <&disp_pd>;gsc_0: video-scaler@13e00000 {
[1]: exynos-dp 145b0000.dp-controller: EDID data does not include any extensions. exynos-dp 145b0000.dp-controller: EDID Read success! exynos-dp 145b0000.dp-controller: Link Training Clock Recovery success exynos-dp 145b0000.dp-controller: Link Training success! exynos-dp 145b0000.dp-controller: Timeout of video streamclk ok exynos-dp 145b0000.dp-controller: unable to config video
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi,
On 01/12/2015 03:40 PM, Joonyoung Shim wrote:
Hi,
On 01/09/2015 01:42 AM, Javier Martinez Canillas wrote:
Hello Joonyoung,
On 01/07/2015 10:55 AM, Joonyoung Shim wrote:
I don't think iommu support and power domain issue are related. I also get displaying stripes via hdmi but it is just power domain issue regardless iommu support.
I observed 8th bit from 0x1445000C register of mixer is set to 1 with displaying stripes. It means "The graphic layer0 line buffer underflow". There was same underflow issue on Exynos4 based boards. As Marek said, because LCD0 power domain was turned off.
Interesting, thanks a lot for sharing this information.
I just tried to turn off DISP1 power domain at u-boot and DISP1 power domain is turned on from kernel hdmi and mixer driver on odroid xu3 board. As the result, i can see displaying penguin logo from hdmi.
Can you share the patches you are using to turn on the DISP1 power domain since AFAIU the kernel does not know about the DISP1 power domain after commit d51cad7df871 ("ARM: dts: remove display power domain for exynos5420").
I tried reverting that commit before so the kernel knows about the DISP1 power domain and booting with pd_ignore_unused but still had the stripes.
I add DISP1 power domain on dts and please refer below patch[0] with some modification on hdmi phy(Actually, i think this is not related). You also should disable DISP1 power domain from bootloader.
Thanks a lot for the patch. With your changes I have some HDMI output on my Peach Pi. But the HDMI output is broken since the resolution is very low and the video as a lot of visual artifacts.
In your change you are also adding clock phandles for the pd oscillator clock and the CLK_MOUT_SW_ACLK200, CLK_MOUT_USER_ACLK200_DISP1 pairs for the parent input and input clocks of the devices attached to the pd.
And also making changes to the clocks in the clk-exynos5420 driver. Can you please explain the rationale for those changes? I'm asking because without your clock changes (only adding the DISP1 pd and making the devices as consumers), I've HDMI output too but video is even worse. This [0] is the minimal change I have on top of 3.19-rc3 to have some output.
I just refer below patches, http://comments.gmane.org/gmane.linux.kernel.samsung-soc/34576
But i'm not sure whether DISP1 power domain is same case with MFC power domain.
The docs I've access to, don't have information about the clock hierarchy. So each time there is a clock issue, I've a hard time to figure out what's happening.
So there seems to be two issues here, one is the mixer and hdmi modules not being attached to the DISP1 power domain and another one is the clocks setup not being correct to have proper HDMI video output.
Hmm, i can see normal hdmi output still from latest upstream kernel(3.19-rc4) with my kernel changes and u-boot changes(DISP1 power domain disable) of prior mail on odroid xu3 board.
But the problem exists still because it is failed to control on/off of DISP1 power domain more than twice from kernel hdmi and mixer driver.[0]
I didn't have this issue when testing your patch against 3.19-rc2. From your log I see that you are testing on a 3.18.1. So maybe makes sense to test with the latest kernel version since this HDMI issue qualifies as an 3.19-rc fix?
Since commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes") that landed in 3.19-rc1, I see that the power domain is powered on when a device is attached. So maybe that is what makes a difference here?
I also get ashake hdmi output since commit 2ed127697eb1, it causes on and off of DISP1 power domain when hdmi/mixer driver probe is defered because regulator driver is not probed yet.
I'm not sure why it causes abnormal hdmi operation.
Thanks.
I'm not sure, but i get same error results from 3.19-rc4. Did you test using exynos drm driver? I used modetest of libdrm
Hello Joonyoung,
On 01/12/2015 07:40 AM, Joonyoung Shim wrote:
And also making changes to the clocks in the clk-exynos5420 driver. Can you please explain the rationale for those changes? I'm asking because without your clock changes (only adding the DISP1 pd and making the devices as consumers), I've HDMI output too but video is even worse. This [0] is the minimal change I have on top of 3.19-rc3 to have some output.
I just refer below patches, http://comments.gmane.org/gmane.linux.kernel.samsung-soc/34576
But i'm not sure whether DISP1 power domain is same case with MFC power domain.
Thanks a lot for sharing those patches, now your changes are much more clear to me.
So there seems to be two issues here, one is the mixer and hdmi modules not being attached to the DISP1 power domain and another one is the clocks setup not being correct to have proper HDMI video output.
Hmm, i can see normal hdmi output still from latest upstream kernel(3.19-rc4) with my kernel changes and u-boot changes(DISP1 power domain disable) of prior mail on odroid xu3 board.
I thought you said on another email that after commit 2ed127697eb1 which landed on 3.19-rc1 you had bad HDMI output?
In your changes, it was missing the SW_ACLK_400_DISP1 and USER_ACLK_400_DISP1 clock mux outputs that goes to internal buses in the DISP1. Adding IDs for these in the exynos5420 clock driver and to the parent and input clock paris list in the DISP1 power domain gives me a good HDMI output on 3.19-rc2.
Also, the SW_ACLK_300_DISP1 and USER_ACLK_300_DISP1 are needed for the FIMD parent and input clock respectively. Adding those to the clocks list of the DISP1 power domain gives me working display + HDMI on my Exynos5800 Peach Pi.
These are the changes I have now [0]. Please let me know what you think.
I didn't have this issue when testing your patch against 3.19-rc2. From your log I see that you are testing on a 3.18.1. So maybe makes sense to test with the latest kernel version since this HDMI issue qualifies as an 3.19-rc fix?
Since commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes") that landed in 3.19-rc1, I see that the power domain is powered on when a device is attached. So maybe that is what makes a difference here?
I'm not sure, but i get same error results from 3.19-rc4. Did you test using exynos drm driver? I used modetest of libdrm
Yes, I was not able to trigger that by running modetest but by turning off my HDMI monitor and then turning it on again. When the monitor is turned on then I see a "Power domain power-domain disable failed" and the imprecise external abort error.
I had to disable CONFIG_DRM_EXYNOS_DP in order to trigger though and that is why I was not able to reproduce it before.
I think though that this is a separate issue of the HDMI not working since power domains should be able to have many consumers devices and I see that other power domains are used that way.
Best regards, Javier
[0]: diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi index 0ac5e0810e97..53b0a03843f2 100644 --- a/arch/arm/boot/dts/exynos5420.dtsi +++ b/arch/arm/boot/dts/exynos5420.dtsi @@ -270,6 +270,19 @@ reg = <0x10044120 0x20>; };
+ disp1_pd: power-domain@100440C0 { + compatible = "samsung,exynos4210-pd"; + reg = <0x100440C0 0x20>; + clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MOUT_SW_ACLK200>, + <&clock CLK_MOUT_USER_ACLK200_DISP1>, + <&clock CLK_MOUT_SW_ACLK300>, + <&clock CLK_MOUT_USER_ACLK300_DISP1>, + <&clock CLK_MOUT_SW_ACLK400>, + <&clock CLK_MOUT_USER_ACLK400_DISP1>; + clock-names = "oscclk", "pclk0", "clk0", + "pclk1", "clk1", "pclk2", "clk2"; + }; + pinctrl_0: pinctrl@13400000 { compatible = "samsung,exynos5420-pinctrl"; reg = <0x13400000 0x1000>; @@ -537,6 +550,7 @@ fimd: fimd@14400000 { clocks = <&clock CLK_SCLK_FIMD1>, <&clock CLK_FIMD1>; clock-names = "sclk_fimd", "fimd"; + samsung,power-domain = <&disp1_pd>; };
adc: adc@12D10000 { @@ -710,6 +724,7 @@ phy = <&hdmiphy>; samsung,syscon-phandle = <&pmu_system_controller>; status = "disabled"; + samsung,power-domain = <&disp1_pd>; };
hdmiphy: hdmiphy@145D0000 { @@ -722,6 +737,7 @@ interrupts = <0 94 0>; clocks = <&clock CLK_MIXER>, <&clock CLK_SCLK_HDMI>; clock-names = "mixer", "sclk_hdmi"; + samsung,power-domain = <&disp1_pd>; };
gsc_0: video-scaler@13e00000 { diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c index 848d602efc06..07d666cc6a29 100644 --- a/drivers/clk/samsung/clk-exynos5420.c +++ b/drivers/clk/samsung/clk-exynos5420.c @@ -635,8 +635,8 @@ static struct samsung_mux_clock exynos5x_mux_clks[] __initdata = { SRC_TOP3, 0, 1), MUX(0, "mout_user_aclk400_mscl", mout_user_aclk400_mscl_p, SRC_TOP3, 4, 1), - MUX(0, "mout_user_aclk200_disp1", mout_user_aclk200_disp1_p, - SRC_TOP3, 8, 1), + MUX(CLK_MOUT_USER_ACLK200_DISP1, "mout_user_aclk200_disp1", + mout_user_aclk200_disp1_p, SRC_TOP3, 8, 1), MUX(0, "mout_user_aclk200_fsys2", mout_user_aclk200_fsys2_p, SRC_TOP3, 12, 1), MUX(0, "mout_user_aclk400_wcore", mout_user_aclk400_wcore_p, @@ -663,8 +663,8 @@ static struct samsung_mux_clock exynos5x_mux_clks[] __initdata = { MUX(CLK_MOUT_USER_ACLK333, "mout_user_aclk333", mout_user_aclk333_p, SRC_TOP4, 28, 1),
- MUX(0, "mout_user_aclk400_disp1", mout_user_aclk400_disp1_p, - SRC_TOP5, 0, 1), + MUX(CLK_MOUT_USER_ACLK400_DISP1, "mout_user_aclk400_disp1", + mout_user_aclk400_disp1_p, SRC_TOP5, 0, 1), MUX(0, "mout_user_aclk66_psgen", mout_user_aclk66_peric_p, SRC_TOP5, 4, 1), MUX(0, "mout_user_aclk333_g2d", mout_user_aclk333_g2d_p, @@ -675,8 +675,8 @@ static struct samsung_mux_clock exynos5x_mux_clks[] __initdata = { SRC_TOP5, 16, 1), MUX(0, "mout_user_aclk300_jpeg", mout_user_aclk300_jpeg_p, SRC_TOP5, 20, 1), - MUX(0, "mout_user_aclk300_disp1", mout_user_aclk300_disp1_p, - SRC_TOP5, 24, 1), + MUX(CLK_MOUT_USER_ACLK300_DISP1, "mout_user_aclk300_disp1", + mout_user_aclk300_disp1_p, SRC_TOP5, 24, 1), MUX(0, "mout_user_aclk300_gscl", mout_user_aclk300_gscl_p, SRC_TOP5, 28, 1),
@@ -693,7 +693,8 @@ static struct samsung_mux_clock exynos5x_mux_clks[] __initdata = { SRC_TOP10, 0, 1), MUX(0, "mout_sw_aclk400_mscl", mout_sw_aclk400_mscl_p, SRC_TOP10, 4, 1), - MUX(0, "mout_sw_aclk200", mout_sw_aclk200_p, SRC_TOP10, 8, 1), + MUX(CLK_MOUT_SW_ACLK200, "mout_sw_aclk200", mout_sw_aclk200_p, + SRC_TOP10, 8, 1), MUX(0, "mout_sw_aclk200_fsys2", mout_sw_aclk200_fsys2_p, SRC_TOP10, 12, 1), MUX(0, "mout_sw_aclk400_wcore", mout_sw_aclk400_wcore_p, @@ -717,8 +718,8 @@ static struct samsung_mux_clock exynos5x_mux_clks[] __initdata = { MUX(CLK_MOUT_SW_ACLK333, "mout_sw_aclk333", mout_sw_aclk333_p, SRC_TOP11, 28, 1),
- MUX(0, "mout_sw_aclk400_disp1", mout_sw_aclk400_disp1_p, - SRC_TOP12, 4, 1), + MUX(CLK_MOUT_SW_ACLK400, "mout_sw_aclk400_disp1", + mout_sw_aclk400_disp1_p, SRC_TOP12, 4, 1), MUX(0, "mout_sw_aclk333_g2d", mout_sw_aclk333_g2d_p, SRC_TOP12, 8, 1), MUX(0, "mout_sw_aclk266_g2d", mout_sw_aclk266_g2d_p, @@ -726,8 +727,8 @@ static struct samsung_mux_clock exynos5x_mux_clks[] __initdata = { MUX(0, "mout_sw_aclk_g3d", mout_sw_aclk_g3d_p, SRC_TOP12, 16, 1), MUX(0, "mout_sw_aclk300_jpeg", mout_sw_aclk300_jpeg_p, SRC_TOP12, 20, 1), - MUX(0, "mout_sw_aclk300_disp1", mout_sw_aclk300_disp1_p, - SRC_TOP12, 24, 1), + MUX(CLK_MOUT_SW_ACLK300, "mout_sw_aclk300_disp1", + mout_sw_aclk300_disp1_p, SRC_TOP12, 24, 1), MUX(0, "mout_sw_aclk300_gscl", mout_sw_aclk300_gscl_p, SRC_TOP12, 28, 1),
diff --git a/include/dt-bindings/clock/exynos5420.h b/include/dt-bindings/clock/exynos5420.h index 8dc0913f1775..99da0d117a7d 100644 --- a/include/dt-bindings/clock/exynos5420.h +++ b/include/dt-bindings/clock/exynos5420.h @@ -204,6 +204,12 @@ #define CLK_MOUT_MAUDIO0 643 #define CLK_MOUT_USER_ACLK333 644 #define CLK_MOUT_SW_ACLK333 645 +#define CLK_MOUT_USER_ACLK200_DISP1 646 +#define CLK_MOUT_SW_ACLK200 647 +#define CLK_MOUT_USER_ACLK300_DISP1 648 +#define CLK_MOUT_SW_ACLK300 649 +#define CLK_MOUT_USER_ACLK400_DISP1 650 +#define CLK_MOUT_SW_ACLK400 651
/* divider clocks */ #define CLK_DOUT_PIXEL 768
Hi,
On 01/13/2015 01:09 AM, Javier Martinez Canillas wrote:
Hello Joonyoung,
On 01/12/2015 07:40 AM, Joonyoung Shim wrote:
And also making changes to the clocks in the clk-exynos5420 driver. Can you please explain the rationale for those changes? I'm asking because without your clock changes (only adding the DISP1 pd and making the devices as consumers), I've HDMI output too but video is even worse. This [0] is the minimal change I have on top of 3.19-rc3 to have some output.
I just refer below patches, http://comments.gmane.org/gmane.linux.kernel.samsung-soc/34576
But i'm not sure whether DISP1 power domain is same case with MFC power domain.
Thanks a lot for sharing those patches, now your changes are much more clear to me.
So there seems to be two issues here, one is the mixer and hdmi modules not being attached to the DISP1 power domain and another one is the clocks setup not being correct to have proper HDMI video output.
Hmm, i can see normal hdmi output still from latest upstream kernel(3.19-rc4) with my kernel changes and u-boot changes(DISP1 power domain disable) of prior mail on odroid xu3 board.
I thought you said on another email that after commit 2ed127697eb1 which landed on 3.19-rc1 you had bad HDMI output?
In your changes, it was missing the SW_ACLK_400_DISP1 and USER_ACLK_400_DISP1 clock mux outputs that goes to internal buses in the DISP1. Adding IDs for these in the exynos5420 clock driver and to the parent and input clock paris list in the DISP1 power domain gives me a good HDMI output on 3.19-rc2.
Also, the SW_ACLK_300_DISP1 and USER_ACLK_300_DISP1 are needed for the FIMD parent and input clock respectively. Adding those to the clocks list of the DISP1 power domain gives me working display + HDMI on my Exynos5800 Peach Pi.
These are the changes I have now [0]. Please let me know what you think.
Good, it's working with your patch without u-boot changes and reverting of commit 2ed127697eb.
I didn't have this issue when testing your patch against 3.19-rc2. From your log I see that you are testing on a 3.18.1. So maybe makes sense to test with the latest kernel version since this HDMI issue qualifies as an 3.19-rc fix?
Since commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes") that landed in 3.19-rc1, I see that the power domain is powered on when a device is attached. So maybe that is what makes a difference here?
I'm not sure, but i get same error results from 3.19-rc4. Did you test using exynos drm driver? I used modetest of libdrm
Yes, I was not able to trigger that by running modetest but by turning off my HDMI monitor and then turning it on again. When the monitor is turned on then I see a "Power domain power-domain disable failed" and the imprecise external abort error.
I had to disable CONFIG_DRM_EXYNOS_DP in order to trigger though and that is why I was not able to reproduce it before.
I think though that this is a separate issue of the HDMI not working since power domains should be able to have many consumers devices and I see that other power domains are used that way.
OK, we need more investigation.
Thanks.
On 01/13/2015 02:24 PM, Joonyoung Shim wrote:
Hi,
On 01/13/2015 01:09 AM, Javier Martinez Canillas wrote:
Hello Joonyoung,
On 01/12/2015 07:40 AM, Joonyoung Shim wrote:
And also making changes to the clocks in the clk-exynos5420 driver. Can you please explain the rationale for those changes? I'm asking because without your clock changes (only adding the DISP1 pd and making the devices as consumers), I've HDMI output too but video is even worse. This [0] is the minimal change I have on top of 3.19-rc3 to have some output.
I just refer below patches, http://comments.gmane.org/gmane.linux.kernel.samsung-soc/34576
But i'm not sure whether DISP1 power domain is same case with MFC power domain.
Thanks a lot for sharing those patches, now your changes are much more clear to me.
So there seems to be two issues here, one is the mixer and hdmi modules not being attached to the DISP1 power domain and another one is the clocks setup not being correct to have proper HDMI video output.
Hmm, i can see normal hdmi output still from latest upstream kernel(3.19-rc4) with my kernel changes and u-boot changes(DISP1 power domain disable) of prior mail on odroid xu3 board.
I thought you said on another email that after commit 2ed127697eb1 which landed on 3.19-rc1 you had bad HDMI output?
In your changes, it was missing the SW_ACLK_400_DISP1 and USER_ACLK_400_DISP1 clock mux outputs that goes to internal buses in the DISP1. Adding IDs for these in the exynos5420 clock driver and to the parent and input clock paris list in the DISP1 power domain gives me a good HDMI output on 3.19-rc2.
Also, the SW_ACLK_300_DISP1 and USER_ACLK_300_DISP1 are needed for the FIMD parent and input clock respectively. Adding those to the clocks list of the DISP1 power domain gives me working display + HDMI on my Exynos5800 Peach Pi.
These are the changes I have now [0]. Please let me know what you think.
Good, it's working with your patch without u-boot changes and reverting of commit 2ed127697eb.
But i also get stripe hdmi output if hdmi/mixer drivers aren't defered probed, because DISP1 power domain isn't disabled on booting by defered probe so is always on.
Thanks.
Hello Joonyoung,
On 01/13/2015 09:40 AM, Joonyoung Shim wrote:
These are the changes I have now [0]. Please let me know what you think.
Good, it's working with your patch without u-boot changes and reverting of commit 2ed127697eb.
But i also get stripe hdmi output if hdmi/mixer drivers aren't defered probed, because DISP1 power domain isn't disabled on booting by defered probe so is always on.
Could you please elaborate on this? I'm not sure I undestood what you meant so it would be great if you can give me the steps to reproduce your issue.
Best regards, Javier
Hello Joonyoung,
On 01/13/2015 06:24 AM, Joonyoung Shim wrote:
Also, the SW_ACLK_300_DISP1 and USER_ACLK_300_DISP1 are needed for the FIMD parent and input clock respectively. Adding those to the clocks list of the DISP1 power domain gives me working display + HDMI on my Exynos5800 Peach Pi.
These are the changes I have now [0]. Please let me know what you think.
Good, it's working with your patch without u-boot changes and reverting of commit 2ed127697eb.
Perfect, I'll split the the clk and DTS changes in different patches and post the series once we figure out the "power-domain disable failed" issue.
Yes, I was not able to trigger that by running modetest but by turning off my HDMI monitor and then turning it on again. When the monitor is turned on then I see a "Power domain power-domain disable failed" and the imprecise external abort error.
I had to disable CONFIG_DRM_EXYNOS_DP in order to trigger though and that is why I was not able to reproduce it before.
I think though that this is a separate issue of the HDMI not working since power domains should be able to have many consumers devices and I see that other power domains are used that way.
OK, we need more investigation.
Agreed, since even though I think this is a separate issue, I'll prefer to not add a known bug as a side effect of having a working HDMI.
Best regard, Javier
Hello Joonyoung,
On 01/13/2015 06:24 AM, Joonyoung Shim wrote:
Yes, I was not able to trigger that by running modetest but by turning off my HDMI monitor and then turning it on again. When the monitor is turned on then I see a "Power domain power-domain disable failed" and the imprecise external abort error.
I had to disable CONFIG_DRM_EXYNOS_DP in order to trigger though and that is why I was not able to reproduce it before.
I think though that this is a separate issue of the HDMI not working since power domains should be able to have many consumers devices and I see that other power domains are used that way.
OK, we need more investigation.
I dug further on this issue and found that the cause is that the exynos_mixer driver needs some clocks (CLK_HDMI and CLK_SCLK_HDMI) grabbed by exynos_hdmi to be kept enabled after hdmi_poweroff (drivers/gpu/drm/exynos/exynos_hdmi.c).
Otherwise, any access to mixer device registers leads to an imprecise external abort error. The following change [0] to the Exynos DRM HDMI driver makes the issue to not happen and I can successfully execute:
# echo 1 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
Only not disabling the hdmi clock [1]: is enough but doing so makes sometimes the DISP1 power domain disabling fails. It doesn't seem to have side effect though since I also see the signal in the HDMI display to go standby and then on again.
# echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # echo 1 > /sys/devices/platform/exynos-drm/graphics/fb0/blank [ 63.089080] Power domain disp1-power-domain disable failed # echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank #
That error message when both clocks are not disabled on hdmi_poweroff() though.
I tried different things like set parent of mixer clock to hdmi clock instead of aclk200_disp1 or make the Exynos DRM mixer driver to grab the hdmi clock from DT and prepare_enable from mixer_poweron() but in all cases the same imprecise external abort error was triggered by mixer_poweron() trying to access the mixer registers.
Any ideas?
Thanks a lot and best regards, Javier
[0]: diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 5765a161abdd..0887911cfdd5 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -2084,8 +2084,8 @@ static void hdmi_poweroff(struct exynos_drm_display *display)
cancel_delayed_work(&hdata->hotplug_work);
- clk_disable_unprepare(res->sclk_hdmi); - clk_disable_unprepare(res->hdmi);
/* reset pmu hdmiphy control bit to disable hdmiphy */ regmap_update_bits(hdata->pmureg, PMU_HDMI_PHY_CONTROL,
[1]: diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 5765a161abdd..628bff96d543 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -2085,7 +2085,7 @@ static void hdmi_poweroff(struct exynos_drm_display *display) cancel_delayed_work(&hdata->hotplug_work);
clk_disable_unprepare(res->sclk_hdmi); - clk_disable_unprepare(res->hdmi);
/* reset pmu hdmiphy control bit to disable hdmiphy */ regmap_update_bits(hdata->pmureg, PMU_HDMI_PHY_CONTROL,
On 01/14/2015 01:19 AM, Javier Martinez Canillas wrote:
I dug further on this issue and found that the cause is that the exynos_mixer driver needs some clocks (CLK_HDMI and CLK_SCLK_HDMI) grabbed by exynos_hdmi to be kept enabled after hdmi_poweroff (drivers/gpu/drm/exynos/exynos_hdmi.c).
Otherwise, any access to mixer device registers leads to an imprecise external abort error. The following change [0] to the Exynos DRM HDMI driver makes the issue to not happen and I can successfully execute:
# echo 1 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
Only not disabling the hdmi clock [1]: is enough but doing so makes sometimes the DISP1 power domain disabling fails. It doesn't seem to have side effect though since I also see the signal in the HDMI display to go standby and then on again.
# echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # echo 1 > /sys/devices/platform/exynos-drm/graphics/fb0/blank [ 63.089080] Power domain disp1-power-domain disable failed # echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank #
That error message when both clocks are not disabled on hdmi_poweroff() though.
This should be: "That error message is not shown when both clocks are disabled".
Best regards, Javier
Hi,
On 01/14/2015 09:24 AM, Javier Martinez Canillas wrote:
On 01/14/2015 01:19 AM, Javier Martinez Canillas wrote:
I dug further on this issue and found that the cause is that the exynos_mixer driver needs some clocks (CLK_HDMI and CLK_SCLK_HDMI) grabbed by exynos_hdmi to be kept enabled after hdmi_poweroff (drivers/gpu/drm/exynos/exynos_hdmi.c).
Otherwise, any access to mixer device registers leads to an imprecise external abort error. The following change [0] to the Exynos DRM HDMI driver makes the issue to not happen and I can successfully execute:
# echo 1 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
Did you execute this operation repeatedly? Still i get error when i execute this more than twice with your change [0].
Thanks.
Only not disabling the hdmi clock [1]: is enough but doing so makes sometimes the DISP1 power domain disabling fails. It doesn't seem to have side effect though since I also see the signal in the HDMI display to go standby and then on again.
# echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # echo 1 > /sys/devices/platform/exynos-drm/graphics/fb0/blank [ 63.089080] Power domain disp1-power-domain disable failed # echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank #
That error message when both clocks are not disabled on hdmi_poweroff() though.
This should be: "That error message is not shown when both clocks are disabled".
Best regards, Javier
Hello Joonyoung,
On 01/20/2015 12:12 PM, Joonyoung Shim wrote:
I dug further on this issue and found that the cause is that the exynos_mixer driver needs some clocks (CLK_HDMI and CLK_SCLK_HDMI) grabbed by exynos_hdmi to be kept enabled after hdmi_poweroff (drivers/gpu/drm/exynos/exynos_hdmi.c).
Otherwise, any access to mixer device registers leads to an imprecise external abort error. The following change [0] to the Exynos DRM HDMI driver makes the issue to not happen and I can successfully execute:
# echo 1 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
Did you execute this operation repeatedly? Still i get error when i execute this more than twice with your change [0].
You mean that you are seeing the "Power domain power-domain disable failed" message but the system is not crashing or that you are still having the system crash?
I've seen the former when running multiple times but I have not seen the crash when the hdmi clock is not disabled on hdmi_poweroff().
Thanks.
Best regards, Javier
Hello,
On 2014-12-02 10:59, Sjoerd Simons wrote:
Hey Marek, Inki,
On Wed, 2014-11-19 at 12:15 +0100, Marek Szyprowski wrote:
Hello Everyone,
This is another attempt to finally make Exynos SYSMMU driver fully integrated with DMA-mapping subsystem. The main change from previous version is a rebase onto latest "automatic DMA configuration for IOMMU masters" patches from Will Deacon.
Do you happen to know if anyone is working on iommu/dma-mapping patches for Exynos 5 based on this patchset?
Just to let all of You know - I posted a new version of this patchset with Exynos 5250 & 5420/5422/5800 DTS patches:
http://lists.linaro.org/pipermail/linaro-mm-sig/2015-January/004327.html
Best regards
On Fri, 2015-01-16 at 11:33 +0100, Marek Szyprowski wrote:
Hello,
On 2014-12-02 10:59, Sjoerd Simons wrote:
Hey Marek, Inki,
On Wed, 2014-11-19 at 12:15 +0100, Marek Szyprowski wrote:
Hello Everyone,
This is another attempt to finally make Exynos SYSMMU driver fully integrated with DMA-mapping subsystem. The main change from previous version is a rebase onto latest "automatic DMA configuration for IOMMU masters" patches from Will Deacon.
Do you happen to know if anyone is working on iommu/dma-mapping patches for Exynos 5 based on this patchset?
Just to let all of You know - I posted a new version of this patchset with Exynos 5250 & 5420/5422/5800 DTS patches:
http://lists.linaro.org/pipermail/linaro-mm-sig/2015-January/004327.html
Great thanks!. As there are quite a few dependencies, do you happen to have a public git tree with the combined work in ? Would make testing a lot simpler.
linaro-mm-sig@lists.linaro.org
-
Arnd Bergmann -
Javier Martinez Canillas -
Javier Martinez Canillas -
Joonyoung Shim -
Laurent Pinchart -
Lucas Stach -
Marek Szyprowski -
Sjoerd Simons -
Thierry Reding -
Will Deacon