Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

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 <[email protected]>

---
 drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

drivers/iommu/exynos-iommu.c +0 -0

index cd28dc09db39..88f9afe641a0 100644

drivers/iommu/exynos-iommu.c +1 -1

@@ -1125,4 +1134,21 @@ err_reg_driver:
 	kmem_cache_destroy(lv2table_kmem_cache);
 	return ret;
 }
-subsys_initcall(exynos_iommu_init);
+

drivers/iommu/exynos-iommu.c +7 -0

+
+	of_iommu_set_ops(np, &exynos_iommu_ops);
+	return 0;
+}
+
+IOMMU_OF_DECLARE(exynos_iommu_of, "samsung,exynos-sysmmu",
+		 exynos_iommu_of_setup);

Hi Marek,

Thank you for the patch.

Quoted 14 lines · 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 <[email protected]>

---
 drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

drivers/iommu/exynos-iommu.c +0 -0

index cd28dc09db39..88f9afe641a0 100644

[snip]

Quoted 15 lines

drivers/iommu/exynos-iommu.c +10 -1

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

drivers/iommu/exynos-iommu.c err_reg_driver: · line 1147

Quoted 1 line

drivers/iommu/exynos-iommu.c +1 -0

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

Quoted 14 lines · 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 <[email protected]>

---
 drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

drivers/iommu/exynos-iommu.c +0 -0

index cd28dc09db39..88f9afe641a0 100644

[snip]

Quoted 6 lines

drivers/iommu/exynos-iommu.c +1 -1

@@ -1125,4 +1134,21 @@ err_reg_driver:
 	kmem_cache_destroy(lv2table_kmem_cache);
 	return ret;
 }
-subsys_initcall(exynos_iommu_init);
+

drivers/iommu/exynos-iommu.c err_reg_driver: · line 1147

Quoted 10 lines

drivers/iommu/exynos-iommu.c +10 -0

+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 ?

Quoted 46 lines · Laurent Pinchart

Quoted 15 lines · 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 <[email protected]>

---

 drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

drivers/iommu/exynos-iommu.c +0 -0

index cd28dc09db39..88f9afe641a0 100644

[snip]

Quoted 17 lines

drivers/iommu/exynos-iommu.c +9 -1

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

drivers/iommu/exynos-iommu.c err_reg_driver: · line 1147

Quoted 2 lines

drivers/iommu/exynos-iommu.c +2 -0

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

Quoted 1 line

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.

Quoted 23 lines · On Monday 15 December 2014 17:17:00 Will Deacon wrote:

Quoted 21 lines · Laurent Pinchart

drivers/iommu/exynos-iommu.c err_reg_driver: · line 1147

Quoted 11 lines · On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:

drivers/iommu/exynos-iommu.c +8 -0

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

drivers/iommu/exynos-iommu.c +2 -0

+	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 ?

Quoted 3 lines

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.

Quoted 28 lines · Laurent Pinchart

Quoted 26 lines · On Monday 15 December 2014 17:17:00 Will Deacon wrote:

Quoted 20 lines · Laurent Pinchart

drivers/iommu/exynos-iommu.c err_reg_driver: · line 1147

Quoted 11 lines · On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:

drivers/iommu/exynos-iommu.c +8 -0

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

drivers/iommu/exynos-iommu.c +2 -0

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

Quoted 1 line

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.

Quoted 5 lines

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?

Quoted 4 lines

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.

Hi Will,

Quoted 6 lines · Laurent Pinchart

Quoted 5 lines · On Monday 15 December 2014 17:17:00 Will Deacon wrote:

Quoted 3 lines

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 4 lines

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 ?

Quoted 1 line · Laurent Pinchart

Hi Will,

Hello again :)

Quoted 21 lines · On Monday 15 December 2014 17:43:02 Will Deacon wrote:

Quoted 6 lines · Laurent Pinchart

Quoted 5 lines · On Monday 15 December 2014 17:17:00 Will Deacon wrote:

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 4 lines

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

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

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?

Quoted 23 lines · On Monday 15 December 2014 18:13:23 Will Deacon wrote:

Quoted 22 lines · Laurent Pinchart

Quoted 5 lines · On Monday 15 December 2014 17:43:02 Will Deacon wrote:

Quoted 4 lines · Laurent Pinchart

Quoted 3 lines · 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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

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?

Quoted 12 lines · On Monday 15 December 2014 18:13:23 Will Deacon wrote:

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

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 ?

Quoted 1 line

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.

Hi Arnd,

Quoted 18 lines · On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote:

Quoted 5 lines · On Monday 15 December 2014 18:13:23 Will Deacon wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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:

Quoted 3 lines

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

Quoted 9 lines

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.

Quoted 20 lines · On Tuesday 16 December 2014 14:07:11 Laurent Pinchart wrote:

Quoted 19 lines · On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote:

Quoted 10 lines · On Monday 15 December 2014 18:13:23 Will Deacon wrote:

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

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.

Quoted 4 lines

Quoted 2 lines

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:

Quoted 8 lines

Quoted 3 lines

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

Quoted 11 lines

Quoted 10 lines

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

Quoted 2 lines

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.

Quoted 8 lines · 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.

Quoted 10 lines · On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:

Quoted 8 lines · 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.

Quoted 4 lines

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 ?

Quoted 21 lines · On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote:

Quoted 19 lines · On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:

Quoted 11 lines · 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.

Quoted 2 lines

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.

Quoted 25 lines · On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote:

Quoted 23 lines · On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote:

Quoted 13 lines · On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:

Quoted 11 lines · 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.

Quoted 6 lines

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 ?

Quoted 3 lines

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.

Quoted 28 lines · On Wednesday 17 December 2014 18:02:51 Laurent Pinchart wrote:

Quoted 26 lines · On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote:

Quoted 18 lines · On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote:

Quoted 11 lines · On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote:

Quoted 9 lines · 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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 9 lines

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.

Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann:

Quoted 11 lines · On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:

Quoted 9 lines

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.

Quoted 5 lines

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

Quoted 33 lines · On Wednesday 17 December 2014 15:53:25 Lucas Stach wrote:

Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann:

Quoted 10 lines · On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote:

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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

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.

Hey Marek, Inki,

Quoted 6 lines · 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.

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

Quoted 8 lines · Sjoerd Simons

Quoted 5 lines

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.

Quoted 6 lines

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

Hello Marek,

Quoted 22 lines · Marek Szyprowski

Quoted 8 lines · Sjoerd Simons

Quoted 5 lines

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.

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 3 lines

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?

Quoted 8 lines · Javier Martinez Canillas

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]

Hello Joonyoung,

Quoted 10 lines · Joonyoung Shim

Quoted 8 lines · Javier Martinez Canillas

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?

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 22 lines

Quoted 10 lines

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.

Quoted 4 lines

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").

Quoted 3 lines

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.

Quoted 3 lines

Quoted 3 lines

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]

Quoted 6 lines

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?

Hello Joonyoung,

Quoted 22 lines · Joonyoung Shim

Quoted 22 lines

Quoted 10 lines

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.

Quoted 4 lines

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").

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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?

Quoted 7 lines

Quoted 6 lines

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

Quoted 5 lines

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.

Quoted 3 lines

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

arch/arm/boot/dts/exynos5420.dtsi +8 -0

index 517e50f6760b..9966fe7bb0ec 100644
@@ -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

Hi,

Quoted 42 lines · Javier Martinez Canillas

Hello Joonyoung,

Quoted 22 lines · Joonyoung Shim

Quoted 22 lines

Quoted 10 lines

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.

Quoted 4 lines

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").

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 12 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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?

Quoted 4 lines

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

Hello Joonyoung,

Quoted 7 lines · Joonyoung Shim

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

Quoted 5 lines

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.

Quoted 5 lines

Quoted 5 lines

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?

Quoted 8 lines

Quoted 4 lines

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.

Hi,

Quoted 42 lines · Javier Martinez Canillas

Hello Joonyoung,

Quoted 7 lines · Joonyoung Shim

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 5 lines · Javier Martinez Canillas

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

Quoted 7 lines

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

Quoted 4 lines

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.

Quoted 8 lines

# 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

Quoted 9 lines

Quoted 9 lines

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:

Quoted 3 lines

Quoted 3 lines

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

Hey Marek, Inki,

Quoted 6 lines · 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.

Do you happen to know if anyone is working on iommu/dma-mapping patches for Exynos 5 based on this patchset?

Hello,

Quoted 9 lines · Sjoerd Simons

Hey Marek, Inki,

Quoted 6 lines · 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.

Quoted 2 lines

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