[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Marek Szyprowski]

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(-)

-- 
1.9.2

[PATCH v3 01/19] iommu: fix const qualifier in of_iommu_set_ops

[Marek Szyprowski]

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)
-- 
1.9.2

[PATCH v3 02/19] iommu: fix initialization without 'add_device' callback

[Marek Szyprowski]

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);
 
-- 
1.9.2

[PATCH v3 03/19] arm: dma-mapping: add missing check for iommu

[Marek Szyprowski]

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);
-- 
1.9.2

[PATCH v3 04/19] drm: exynos: detach from default dma-mapping domain on init

[Marek Szyprowski]

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");
-- 
1.9.2

[PATCH v3 05/19] arm: exynos: pm_domains: add support for devices registered before arch_initcall

[Marek Szyprowski]

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);
-- 
1.9.2

[PATCH v3 06/19] ARM: dts: exynos4: add sysmmu nodes

[Marek Szyprowski]

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 at 13800000 {
@@ -644,5 +650,116 @@
 		samsung,power-domain = <&pd_lcd0>;
 		samsung,sysreg = <&sys_reg>;
 		status = "disabled";
+		iommus = <&sysmmu_fimd0>;
+	};
+
+	sysmmu_mfc_l: sysmmu at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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>;
+	};
 };
-- 
1.9.2

[PATCH v3 07/19] iommu: exynos: don't read version register on every tlb operation

[Marek Szyprowski]

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)) {
-- 
1.9.2

[PATCH v3 08/19] iommu: exynos: remove unused functions

[Marek Szyprowski]

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;
-- 
1.9.2

[PATCH v3 09/19] iommu: exynos: remove useless spinlock

[Marek Szyprowski]

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;
 }
 
-- 
1.9.2

[PATCH v3 10/19] iommu: exynos: refactor function parameters to simplify code

[Marek Szyprowski]

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);
 }
-- 
1.9.2

[PATCH v3 11/19] iommu: exynos: remove unused functions, part 2

[Marek Szyprowski]

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)
 {
-- 
1.9.2

[PATCH v3 12/19] iommu: exynos: remove useless device_add/remove callbacks

[Marek Szyprowski]

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,
 };
 
-- 
1.9.2

[PATCH v3 13/19] iommu: exynos: add support for binding more than one sysmmu to master device

[Marek Szyprowski]

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);
-- 
1.9.2

[PATCH v3 14/19] iommu: exynos: add support for runtime_pm

[Marek Szyprowski]

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;
 		}
 	}
-- 
1.9.2

[PATCH v3 15/19] iommu: exynos: rename variables to reflect their purpose

[Marek Szyprowski]

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;
 }
-- 
1.9.2

[PATCH v3 16/19] iommu: exynos: document internal structures

[Marek Szyprowski]

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)
-- 
1.9.2

[PATCH v3 17/19] iommu: exynos: remove excessive includes and sort others alphabetically

[Marek Szyprowski]

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>
-- 
1.9.2

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Marek Szyprowski]

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);
-- 
1.9.2

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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);

-- 
Regards,

Laurent Pinchart

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141215/dfb39a50/attachment.sig>

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Will Deacon]

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

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Will Deacon]

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

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Will Deacon]

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

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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?

-- 
Regards,

Laurent Pinchart

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Arnd Bergmann]

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

[PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Arnd Bergmann]

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

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[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.

	Arnd

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Arnd Bergmann]

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

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Arnd Bergmann]

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

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Lucas Stach]

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

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Arnd Bergmann]

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:
> 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...

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

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Laurent Pinchart]

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:
> > 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...
> 
> 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 ?

-- 
Regards,

Laurent Pinchart

[Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall

[Arnd Bergmann]

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

[PATCH v3 19/19] iommu: exynos: add callback for initializing devices from device tree

[Marek Szyprowski]

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;
-- 
1.9.2

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Sjoerd Simons]

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

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6170 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141202/a29ad0cc/attachment-0001.bin>

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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.html

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Joonyoung Shim]

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!

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Joonyoung Shim]

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 at 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 at 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 at 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
-- 
1.9.1

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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 at 100440C0 {
+		compatible = "samsung,exynos4210-pd";
+		reg = <0x100440C0 0x20>;
+	};
+
 	msc_pd: power-domain at 10044120 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044120 0x20>;
@@ -535,6 +540,7 @@
 	};
 
 	fimd: fimd at 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 at 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 at 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

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Joonyoung Shim]

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 at 100440C0 {
> +		compatible = "samsung,exynos4210-pd";
> +		reg = <0x100440C0 0x20>;
> +	};
> +
>  	msc_pd: power-domain at 10044120 {
>  		compatible = "samsung,exynos4210-pd";
>  		reg = <0x10044120 0x20>;
> @@ -535,6 +540,7 @@
>  	};
>  
>  	fimd: fimd at 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 at 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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Joonyoung Shim]

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
> 

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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 at 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 at 13400000 {
 		compatible = "samsung,exynos5420-pinctrl";
 		reg = <0x13400000 0x1000>;
@@ -537,6 +550,7 @@
 	fimd: fimd at 14400000 {
 		clocks = <&clock CLK_SCLK_FIMD1>, <&clock CLK_FIMD1>;
 		clock-names = "sclk_fimd", "fimd";
+		samsung,power-domain = <&disp1_pd>;
 	};
 
 	adc: adc at 12D10000 {
@@ -710,6 +724,7 @@
 		phy = <&hdmiphy>;
 		samsung,syscon-phandle = <&pmu_system_controller>;
 		status = "disabled";
+		samsung,power-domain = <&disp1_pd>;
 	};
 
 	hdmiphy: hdmiphy at 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 at 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

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Joonyoung Shim]

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.

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Joonyoung Shim]

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.

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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,

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Joonyoung Shim]

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
> 

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Javier Martinez Canillas]

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

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Sjoerd Simons]

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.

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6170 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150116/fb6a2234/attachment-0001.bin>