Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
Hi KyongHo Cho,
On Fri, Mar 14, 2014 at 10:40 AM, Cho KyongHo pullip@samsung.com wrote:
Some master device descriptor like fimc-is which is an abstraction
of very complex H/W may have multiple System MMUs. For those devices,
the design of the link between System MMU and its master H/W is needed
to be reconsidered.
A link structure, sysmmu_list_data is introduced that provides a link
to master H/W and that has a pointer to the device descriptor of a
System MMU. Given a device descriptor of a master H/W, it is possible
to traverse all System MMUs that must be controlled along with the
master H/W.
Signed-off-by: Cho KyongHo pullip@samsung.com
---
drivers/iommu/exynos-iommu.c | 534
++
1 file changed, 333 insertions(+), 201 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 84ba29a..7489343 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -128,6 +128,10 @@
#define __master_clk_disable(data) __clk_gate_ctrl(data, clk_master, dis)
[snip]
+static int __init __sysmmu_init_master(struct device *dev)
+{
+ int ret;
+ int i = 0;
+ struct device_node *node;
+
+ while ((node = of_parse_phandle(dev-of_node, mmu-masters, i++))) {
struct platform_device *master = of_find_device_by_node(node);
+ struct exynos_iommu_owner *owner;
+ struct sysmmu_list_data *list_data;
if (!master) {
dev_err(dev, %s: mmu-master '%s' not found\n,
__func__, node-name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
- if (master-dev.archdata.iommu != NULL) {
- dev_err(dev, %s: '%s' is master of other MMU\n,
- __func__, node-name);
- return -EINVAL;
+ owner = master-dev.archdata.iommu;
+ if (!owner) {
+ owner = devm_kzalloc(dev, sizeof(*owner), GFP_KERNEL);
+ if (!owner) {
+ dev_err(dev,
+ %s: Failed to allocate owner structure\n,
+ __func__);
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ INIT_LIST_HEAD(owner-mmu_list);
+ INIT_LIST_HEAD(owner-client);
+ owner-dev = master-dev;
+ spin_lock_init(owner-lock);
+
+ master-dev.archdata.iommu = owner;
}
+ list_data = devm_kzalloc(dev, sizeof(*list_data), GFP_KERNEL);
+ if (!list_data) {
+ dev_err(dev,
+ %s: Failed to allocate sysmmu_list_data\n,
+ __func__);
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ INIT_LIST_HEAD(list_data-entry);
+ list_data-sysmmu = dev;
+
/*
-* archdata.iommu will be initialized with exynos_iommu_client
-* in sysmmu_hook_driver_register().
+* System MMUs are attached in the order of the presence
+* in device tree
*/
- master-dev.archdata.iommu = dev;
+ list_add_tail(list_data-entry, owner-mmu_list);
}
- data-sysmmu = dev;
- rwlock_init(data-lock);
+ return 0;
+err:
+ while ((node = of_parse_phandle(dev-of_node, mmu-masters, i++))) {
Don't we need to reinitialize variable 'i' here before using?
i = 0;
Regards,
Shaik Ameer Basha
+ struct platform_device *master = of_find_device_by_node(node);
+ struct exynos_iommu_owner *owner;
+ struct sysmmu_list_data *list_data;
- platform_set_drvdata(pdev, data);
+ if (!master)
+ continue;
- pm_runtime_enable(dev);
- data-runtime_active = !pm_runtime_enabled(dev);
+ owner = master-dev.archdata.iommu;
+ if (!owner)
+ continue;
- dev_dbg(dev, Probed and initialized\n);
- return 0;
+ for_each_sysmmu_list(owner-dev, list_data) {
+ if (list_data-sysmmu == dev) {
+ list_del(list_data-entry);
+ kfree(list_data);
+ break;
+ }
+ }
+ }
+
+ return ret;
}
[snip]
___
iommu mailing list
iommu@lists.linux-foundation.org
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On Tue, 22 Apr 2014 18:53:51 +0530, Shaik Ameer Basha wrote:
Hi KyongHo Cho,
On Fri, Mar 14, 2014 at 10:40 AM, Cho KyongHo pullip@samsung.com wrote:
Some master device descriptor like fimc-is which is an abstraction
of very complex H/W may have multiple System MMUs. For those devices,
the design of the link between System MMU and its master H/W is needed
to be reconsidered.
A link structure, sysmmu_list_data is introduced that provides a link
to master H/W and that has a pointer to the device descriptor of a
System MMU. Given a device descriptor of a master H/W, it is possible
to traverse all System MMUs that must be controlled along with the
master H/W.
Signed-off-by: Cho KyongHo pullip@samsung.com
---
drivers/iommu/exynos-iommu.c | 534
++
1 file changed, 333 insertions(+), 201 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 84ba29a..7489343 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -128,6 +128,10 @@
#define __master_clk_disable(data) __clk_gate_ctrl(data, clk_master,
dis)
[snip]
+static int __init __sysmmu_init_master(struct device *dev)
+{
+ int ret;
+ int i = 0;
+ struct device_node *node;
+
+ while ((node = of_parse_phandle(dev-of_node, mmu-masters, i++)))
{
struct platform_device *master =
of_find_device_by_node(node);
+ struct exynos_iommu_owner *owner;
+ struct sysmmu_list_data *list_data;
if (!master) {
dev_err(dev, %s: mmu-master '%s' not found\n,
__func__, node-name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
- if (master-dev.archdata.iommu != NULL) {
- dev_err(dev, %s: '%s' is master of other MMU\n,
- __func__, node-name);
- return -EINVAL;
+ owner = master-dev.archdata.iommu;
+ if (!owner) {
+ owner = devm_kzalloc(dev, sizeof(*owner),
GFP_KERNEL);
+ if (!owner) {
+ dev_err(dev,
+ %s: Failed to allocate owner structure\n,
+ __func__);
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ INIT_LIST_HEAD(owner-mmu_list);
+ INIT_LIST_HEAD(owner-client);
+ owner-dev = master-dev;
+ spin_lock_init(owner-lock);
+
+ master-dev.archdata.iommu = owner;
}
+ list_data = devm_kzalloc(dev, sizeof(*list_data),
GFP_KERNEL);
+ if (!list_data) {
+ dev_err(dev,
+ %s: Failed to allocate sysmmu_list_data\n,
+ __func__);
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ INIT_LIST_HEAD(list_data-entry);
+ list_data-sysmmu = dev;
+
/*
-* archdata.iommu will be initialized with
exynos_iommu_client
-* in sysmmu_hook_driver_register().
+* System MMUs are attached in the order of the presence
+* in device tree
*/
- master-dev.archdata.iommu = dev;
+ list_add_tail(list_data-entry, owner-mmu_list);
}
- data-sysmmu = dev;
- rwlock_init(data-lock);
+ return 0;
+err:
+ while ((node = of_parse_phandle(dev-of_node, mmu-masters, i++)))
{
Don't we need to reinitialize variable 'i' here before using?
i = 0;
Oh. You are right.
Thanks.
+ struct platform_device *master =
of_find_device_by_node(node);
+ struct exynos_iommu_owner *owner;
+ struct sysmmu_list_data *list_data;
- platform_set_drvdata(pdev, data);
+ if (!master)
+ continue;
- pm_runtime_enable(dev);
- data-runtime_active = !pm_runtime_enabled(dev);
+ owner = master-dev.archdata.iommu;
+ if (!owner)
+ continue;
- dev_dbg(dev, Probed and initialized\n);
- return 0;
+ for_each_sysmmu_list(owner-dev, list_data) {
+ if (list_data-sysmmu == dev) {
+ list_del(list_data-entry);
+ kfree(list_data);
+
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On 21.03.2014 06:21, Cho KyongHo wrote:
On Thu, 20 Mar 2014 11:54:58 +0100, Tomasz Figa wrote:
On 20.03.2014 11:22, Cho KyongHo wrote:
On Wed, 19 Mar 2014 16:14:57 +0100, Tomasz Figa wrote:
On 19.03.2014 14:20, Tomasz Figa wrote:
On 19.03.2014 01:39, Cho KyongHo wrote:
On Tue, 18 Mar 2014 15:26:48 +0100, Tomasz Figa wrote:
On 18.03.2014 14:01, Cho KyongHo wrote:
On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote:
Hi KyongHo,
On 14.03.2014 06:10, Cho KyongHo wrote:
Some master device descriptor like fimc-is which is an abstraction
of very complex H/W may have multiple System MMUs. For those devices,
the design of the link between System MMU and its master H/W is
needed
to be reconsidered.
A link structure, sysmmu_list_data is introduced that provides a link
to master H/W and that has a pointer to the device descriptor of a
System MMU. Given a device descriptor of a master H/W, it is possible
to traverse all System MMUs that must be controlled along with the
master H/W.
NAK.
A device driver should handle particular hardware instances
separately,
without abstracting a virtual hardware instance consisting of multiple
physical ones.
If such abstraction is needed, it should be done above the
exynos-iommu
driver, e.g. by something like iommu-composite driver that would
aggregate several IOMMUs. Keep in mind that such IOMMUs in a group
could
be different, e.g. different Exynos SysMMU versions or even completely
different IPs handled by different drivers.
Still, I don't think there is a real need for such abstraction.
Instead,
related drivers shall be fixed to properly handle multiple memory
masters and their IOMMUs.
G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother
SoC like
Exynos5250 does not.
I don't understand why you are negative to this approach.
This is the simplest than the others.
Let me show you an example.
FIMC-IS driver just controls MCU in FIMC-IS subsystem and the
firmware of
the MCU controls all other peripherals in the subsystem. Each
peripherals
have their own System MMU. Moreover, the configuration of the
peripherals
varies according to the SoCs.
If System MMU driver accepts multiple masters, everything is done in
DT.
But I worry that it is not easy if System MMU driver does not support
multiple masters.
I believe I have stated enough reasons why this kind of implementation
is bad. I'm not going to waste time repeating myself.
Your concerns presented above are valid, however they are not related to
what is wrong with this patch. I have given you two proper ways to
handle this, none should be forced upon particular IOMMU master drivers
- their authors should have the chance to select the method that works
best for them.
I don't still understand why you think this patch is wrong.
I think this is the best way not to think for all the driver developers
about other things than their business logic.
I agree, but one of the ways I proposed (an iommu-composite layer above
the IOMMU low level drivers) doesn't add any extra responsibility of
driver developers.
Moreover, it's this kind of business logic in low level drivers that is
adding more responsibility, because it introduces additional complexity
and makes the driver harder to read, maintain and extend in future.
This does not hurt anyone and I think this is good enough.
Well, it is barely good enough. It is a good practice to make a low
level driver handle a single device instance and this is how Linux
driver model is designed.
Moreover, a single device tree node _must_ represent a single hardware
block, so you can't group multiple SysMMUs into a single device tree node.
OK, you add nodes for single SysMMUs devices which is fine, sorry. I was
under impression that one kernel device (struct device) corresponds to
multiple SysMMUs, but this was before your patches, sorry. So one issue
less, but it's still not good.
Ok. Understood why you have mentioned such.
Furthermore, if you force grouping of SysMMUs into a single virtual one,
you enforce using the same address space for all masters of some
particular hardware blocks, while potentially driver developers would
like to separate them.
Probably some clarification is needed. Your other patch adds:
sysmmu_fimd0w04: sysmmu@1464 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1464 0x1000;
interrupt-parent = combiner;
interrupts = 3 2;
clock-names = sysmmu, master;
clocks = clock 422, clock 421;
samsung,power-domain = disp_pd;
mmu-masters = fimd;
};
sysmmu_fimd0w123: sysmmu@1468 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1468 0x1000;
interrupt-parent = combiner;
interrupts = 3 0;
clock-names = sysmmu, master;
clocks = clock 423, clock 421;
samsung,power-domain =
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On Wed, 19 Mar 2014 16:14:57 +0100, Tomasz Figa wrote:
On 19.03.2014 14:20, Tomasz Figa wrote:
On 19.03.2014 01:39, Cho KyongHo wrote:
On Tue, 18 Mar 2014 15:26:48 +0100, Tomasz Figa wrote:
On 18.03.2014 14:01, Cho KyongHo wrote:
On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote:
Hi KyongHo,
On 14.03.2014 06:10, Cho KyongHo wrote:
Some master device descriptor like fimc-is which is an abstraction
of very complex H/W may have multiple System MMUs. For those devices,
the design of the link between System MMU and its master H/W is
needed
to be reconsidered.
A link structure, sysmmu_list_data is introduced that provides a link
to master H/W and that has a pointer to the device descriptor of a
System MMU. Given a device descriptor of a master H/W, it is possible
to traverse all System MMUs that must be controlled along with the
master H/W.
NAK.
A device driver should handle particular hardware instances
separately,
without abstracting a virtual hardware instance consisting of multiple
physical ones.
If such abstraction is needed, it should be done above the
exynos-iommu
driver, e.g. by something like iommu-composite driver that would
aggregate several IOMMUs. Keep in mind that such IOMMUs in a group
could
be different, e.g. different Exynos SysMMU versions or even completely
different IPs handled by different drivers.
Still, I don't think there is a real need for such abstraction.
Instead,
related drivers shall be fixed to properly handle multiple memory
masters and their IOMMUs.
G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother
SoC like
Exynos5250 does not.
I don't understand why you are negative to this approach.
This is the simplest than the others.
Let me show you an example.
FIMC-IS driver just controls MCU in FIMC-IS subsystem and the
firmware of
the MCU controls all other peripherals in the subsystem. Each
peripherals
have their own System MMU. Moreover, the configuration of the
peripherals
varies according to the SoCs.
If System MMU driver accepts multiple masters, everything is done in
DT.
But I worry that it is not easy if System MMU driver does not support
multiple masters.
I believe I have stated enough reasons why this kind of implementation
is bad. I'm not going to waste time repeating myself.
Your concerns presented above are valid, however they are not related to
what is wrong with this patch. I have given you two proper ways to
handle this, none should be forced upon particular IOMMU master drivers
- their authors should have the chance to select the method that works
best for them.
I don't still understand why you think this patch is wrong.
I think this is the best way not to think for all the driver developers
about other things than their business logic.
I agree, but one of the ways I proposed (an iommu-composite layer above
the IOMMU low level drivers) doesn't add any extra responsibility of
driver developers.
Moreover, it's this kind of business logic in low level drivers that is
adding more responsibility, because it introduces additional complexity
and makes the driver harder to read, maintain and extend in future.
This does not hurt anyone and I think this is good enough.
Well, it is barely good enough. It is a good practice to make a low
level driver handle a single device instance and this is how Linux
driver model is designed.
Moreover, a single device tree node _must_ represent a single hardware
block, so you can't group multiple SysMMUs into a single device tree node.
OK, you add nodes for single SysMMUs devices which is fine, sorry. I was
under impression that one kernel device (struct device) corresponds to
multiple SysMMUs, but this was before your patches, sorry. So one issue
less, but it's still not good.
Ok. Understood why you have mentioned such.
Furthermore, if you force grouping of SysMMUs into a single virtual one,
you enforce using the same address space for all masters of some
particular hardware blocks, while potentially driver developers would
like to separate them.
Probably some clarification is needed. Your other patch adds:
sysmmu_fimd0w04: sysmmu@1464 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1464 0x1000;
interrupt-parent = combiner;
interrupts = 3 2;
clock-names = sysmmu, master;
clocks = clock 422, clock 421;
samsung,power-domain = disp_pd;
mmu-masters = fimd;
};
sysmmu_fimd0w123: sysmmu@1468 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1468 0x1000;
interrupt-parent = combiner;
interrupts = 3 0;
clock-names = sysmmu, master;
clocks = clock 423, clock 421;
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On 20.03.2014 11:22, Cho KyongHo wrote:
On Wed, 19 Mar 2014 16:14:57 +0100, Tomasz Figa wrote:
On 19.03.2014 14:20, Tomasz Figa wrote:
On 19.03.2014 01:39, Cho KyongHo wrote:
On Tue, 18 Mar 2014 15:26:48 +0100, Tomasz Figa wrote:
On 18.03.2014 14:01, Cho KyongHo wrote:
On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote:
Hi KyongHo,
On 14.03.2014 06:10, Cho KyongHo wrote:
Some master device descriptor like fimc-is which is an abstraction
of very complex H/W may have multiple System MMUs. For those devices,
the design of the link between System MMU and its master H/W is
needed
to be reconsidered.
A link structure, sysmmu_list_data is introduced that provides a link
to master H/W and that has a pointer to the device descriptor of a
System MMU. Given a device descriptor of a master H/W, it is possible
to traverse all System MMUs that must be controlled along with the
master H/W.
NAK.
A device driver should handle particular hardware instances
separately,
without abstracting a virtual hardware instance consisting of multiple
physical ones.
If such abstraction is needed, it should be done above the
exynos-iommu
driver, e.g. by something like iommu-composite driver that would
aggregate several IOMMUs. Keep in mind that such IOMMUs in a group
could
be different, e.g. different Exynos SysMMU versions or even completely
different IPs handled by different drivers.
Still, I don't think there is a real need for such abstraction.
Instead,
related drivers shall be fixed to properly handle multiple memory
masters and their IOMMUs.
G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother
SoC like
Exynos5250 does not.
I don't understand why you are negative to this approach.
This is the simplest than the others.
Let me show you an example.
FIMC-IS driver just controls MCU in FIMC-IS subsystem and the
firmware of
the MCU controls all other peripherals in the subsystem. Each
peripherals
have their own System MMU. Moreover, the configuration of the
peripherals
varies according to the SoCs.
If System MMU driver accepts multiple masters, everything is done in
DT.
But I worry that it is not easy if System MMU driver does not support
multiple masters.
I believe I have stated enough reasons why this kind of implementation
is bad. I'm not going to waste time repeating myself.
Your concerns presented above are valid, however they are not related to
what is wrong with this patch. I have given you two proper ways to
handle this, none should be forced upon particular IOMMU master drivers
- their authors should have the chance to select the method that works
best for them.
I don't still understand why you think this patch is wrong.
I think this is the best way not to think for all the driver developers
about other things than their business logic.
I agree, but one of the ways I proposed (an iommu-composite layer above
the IOMMU low level drivers) doesn't add any extra responsibility of
driver developers.
Moreover, it's this kind of business logic in low level drivers that is
adding more responsibility, because it introduces additional complexity
and makes the driver harder to read, maintain and extend in future.
This does not hurt anyone and I think this is good enough.
Well, it is barely good enough. It is a good practice to make a low
level driver handle a single device instance and this is how Linux
driver model is designed.
Moreover, a single device tree node _must_ represent a single hardware
block, so you can't group multiple SysMMUs into a single device tree node.
OK, you add nodes for single SysMMUs devices which is fine, sorry. I was
under impression that one kernel device (struct device) corresponds to
multiple SysMMUs, but this was before your patches, sorry. So one issue
less, but it's still not good.
Ok. Understood why you have mentioned such.
Furthermore, if you force grouping of SysMMUs into a single virtual one,
you enforce using the same address space for all masters of some
particular hardware blocks, while potentially driver developers would
like to separate them.
Probably some clarification is needed. Your other patch adds:
sysmmu_fimd0w04: sysmmu@1464 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1464 0x1000;
interrupt-parent = combiner;
interrupts = 3 2;
clock-names = sysmmu, master;
clocks = clock 422, clock 421;
samsung,power-domain = disp_pd;
mmu-masters = fimd;
};
sysmmu_fimd0w123: sysmmu@1468 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1468 0x1000;
interrupt-parent = combiner;
interrupts = 3 0;
clock-names = sysmmu, master;
clocks = clock 423, clock 421;
samsung,power-domain = disp_pd;
mmu-masters = fimd;
};
From such description, in future FIMD
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On Thu, 20 Mar 2014 11:54:58 +0100, Tomasz Figa wrote:
On 20.03.2014 11:22, Cho KyongHo wrote:
On Wed, 19 Mar 2014 16:14:57 +0100, Tomasz Figa wrote:
On 19.03.2014 14:20, Tomasz Figa wrote:
On 19.03.2014 01:39, Cho KyongHo wrote:
On Tue, 18 Mar 2014 15:26:48 +0100, Tomasz Figa wrote:
On 18.03.2014 14:01, Cho KyongHo wrote:
On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote:
Hi KyongHo,
On 14.03.2014 06:10, Cho KyongHo wrote:
Some master device descriptor like fimc-is which is an abstraction
of very complex H/W may have multiple System MMUs. For those devices,
the design of the link between System MMU and its master H/W is
needed
to be reconsidered.
A link structure, sysmmu_list_data is introduced that provides a link
to master H/W and that has a pointer to the device descriptor of a
System MMU. Given a device descriptor of a master H/W, it is possible
to traverse all System MMUs that must be controlled along with the
master H/W.
NAK.
A device driver should handle particular hardware instances
separately,
without abstracting a virtual hardware instance consisting of multiple
physical ones.
If such abstraction is needed, it should be done above the
exynos-iommu
driver, e.g. by something like iommu-composite driver that would
aggregate several IOMMUs. Keep in mind that such IOMMUs in a group
could
be different, e.g. different Exynos SysMMU versions or even completely
different IPs handled by different drivers.
Still, I don't think there is a real need for such abstraction.
Instead,
related drivers shall be fixed to properly handle multiple memory
masters and their IOMMUs.
G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother
SoC like
Exynos5250 does not.
I don't understand why you are negative to this approach.
This is the simplest than the others.
Let me show you an example.
FIMC-IS driver just controls MCU in FIMC-IS subsystem and the
firmware of
the MCU controls all other peripherals in the subsystem. Each
peripherals
have their own System MMU. Moreover, the configuration of the
peripherals
varies according to the SoCs.
If System MMU driver accepts multiple masters, everything is done in
DT.
But I worry that it is not easy if System MMU driver does not support
multiple masters.
I believe I have stated enough reasons why this kind of implementation
is bad. I'm not going to waste time repeating myself.
Your concerns presented above are valid, however they are not related to
what is wrong with this patch. I have given you two proper ways to
handle this, none should be forced upon particular IOMMU master drivers
- their authors should have the chance to select the method that works
best for them.
I don't still understand why you think this patch is wrong.
I think this is the best way not to think for all the driver developers
about other things than their business logic.
I agree, but one of the ways I proposed (an iommu-composite layer above
the IOMMU low level drivers) doesn't add any extra responsibility of
driver developers.
Moreover, it's this kind of business logic in low level drivers that is
adding more responsibility, because it introduces additional complexity
and makes the driver harder to read, maintain and extend in future.
This does not hurt anyone and I think this is good enough.
Well, it is barely good enough. It is a good practice to make a low
level driver handle a single device instance and this is how Linux
driver model is designed.
Moreover, a single device tree node _must_ represent a single hardware
block, so you can't group multiple SysMMUs into a single device tree node.
OK, you add nodes for single SysMMUs devices which is fine, sorry. I was
under impression that one kernel device (struct device) corresponds to
multiple SysMMUs, but this was before your patches, sorry. So one issue
less, but it's still not good.
Ok. Understood why you have mentioned such.
Furthermore, if you force grouping of SysMMUs into a single virtual one,
you enforce using the same address space for all masters of some
particular hardware blocks, while potentially driver developers would
like to separate them.
Probably some clarification is needed. Your other patch adds:
sysmmu_fimd0w04: sysmmu@1464 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1464 0x1000;
interrupt-parent = combiner;
interrupts = 3 2;
clock-names = sysmmu, master;
clocks = clock 422, clock 421;
samsung,power-domain = disp_pd;
mmu-masters = fimd;
};
sysmmu_fimd0w123: sysmmu@1468 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1468 0x1000;
interrupt-parent = combiner;
interrupts = 3 0;
clock-names = sysmmu,
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On 19.03.2014 01:39, Cho KyongHo wrote: On Tue, 18 Mar 2014 15:26:48 +0100, Tomasz Figa wrote: On 18.03.2014 14:01, Cho KyongHo wrote: On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote: Hi KyongHo, On 14.03.2014 06:10, Cho KyongHo wrote: Some master device descriptor like fimc-is which is an abstraction of very complex H/W may have multiple System MMUs. For those devices, the design of the link between System MMU and its master H/W is needed to be reconsidered. A link structure, sysmmu_list_data is introduced that provides a link to master H/W and that has a pointer to the device descriptor of a System MMU. Given a device descriptor of a master H/W, it is possible to traverse all System MMUs that must be controlled along with the master H/W. NAK. A device driver should handle particular hardware instances separately, without abstracting a virtual hardware instance consisting of multiple physical ones. If such abstraction is needed, it should be done above the exynos-iommu driver, e.g. by something like iommu-composite driver that would aggregate several IOMMUs. Keep in mind that such IOMMUs in a group could be different, e.g. different Exynos SysMMU versions or even completely different IPs handled by different drivers. Still, I don't think there is a real need for such abstraction. Instead, related drivers shall be fixed to properly handle multiple memory masters and their IOMMUs. G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother SoC like Exynos5250 does not. I don't understand why you are negative to this approach. This is the simplest than the others. Let me show you an example. FIMC-IS driver just controls MCU in FIMC-IS subsystem and the firmware of the MCU controls all other peripherals in the subsystem. Each peripherals have their own System MMU. Moreover, the configuration of the peripherals varies according to the SoCs. If System MMU driver accepts multiple masters, everything is done in DT. But I worry that it is not easy if System MMU driver does not support multiple masters. I believe I have stated enough reasons why this kind of implementation is bad. I'm not going to waste time repeating myself. Your concerns presented above are valid, however they are not related to what is wrong with this patch. I have given you two proper ways to handle this, none should be forced upon particular IOMMU master drivers - their authors should have the chance to select the method that works best for them. I don't still understand why you think this patch is wrong. I think this is the best way not to think for all the driver developers about other things than their business logic. I agree, but one of the ways I proposed (an iommu-composite layer above the IOMMU low level drivers) doesn't add any extra responsibility of driver developers. Moreover, it's this kind of business logic in low level drivers that is adding more responsibility, because it introduces additional complexity and makes the driver harder to read, maintain and extend in future. This does not hurt anyone and I think this is good enough. Well, it is barely good enough. It is a good practice to make a low level driver handle a single device instance and this is how Linux driver model is designed. Moreover, a single device tree node _must_ represent a single hardware block, so you can't group multiple SysMMUs into a single device tree node. Furthermore, if you force grouping of SysMMUs into a single virtual one, you enforce using the same address space for all masters of some particular hardware blocks, while potentially driver developers would like to separate them. A good interface design should not enforce any particular implementation and this is what we should stick to in this case as well. If you want to provide another layer between master device and system mmu as you mentioned, you do that. This patch does not restrict it. It does, as mentioned above. With a device tree listing multiple SysMMUs as one, you can't separate them. Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On 19.03.2014 14:20, Tomasz Figa wrote:
On 19.03.2014 01:39, Cho KyongHo wrote:
On Tue, 18 Mar 2014 15:26:48 +0100, Tomasz Figa wrote:
On 18.03.2014 14:01, Cho KyongHo wrote:
On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote:
Hi KyongHo,
On 14.03.2014 06:10, Cho KyongHo wrote:
Some master device descriptor like fimc-is which is an abstraction
of very complex H/W may have multiple System MMUs. For those devices,
the design of the link between System MMU and its master H/W is
needed
to be reconsidered.
A link structure, sysmmu_list_data is introduced that provides a link
to master H/W and that has a pointer to the device descriptor of a
System MMU. Given a device descriptor of a master H/W, it is possible
to traverse all System MMUs that must be controlled along with the
master H/W.
NAK.
A device driver should handle particular hardware instances
separately,
without abstracting a virtual hardware instance consisting of multiple
physical ones.
If such abstraction is needed, it should be done above the
exynos-iommu
driver, e.g. by something like iommu-composite driver that would
aggregate several IOMMUs. Keep in mind that such IOMMUs in a group
could
be different, e.g. different Exynos SysMMU versions or even completely
different IPs handled by different drivers.
Still, I don't think there is a real need for such abstraction.
Instead,
related drivers shall be fixed to properly handle multiple memory
masters and their IOMMUs.
G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother
SoC like
Exynos5250 does not.
I don't understand why you are negative to this approach.
This is the simplest than the others.
Let me show you an example.
FIMC-IS driver just controls MCU in FIMC-IS subsystem and the
firmware of
the MCU controls all other peripherals in the subsystem. Each
peripherals
have their own System MMU. Moreover, the configuration of the
peripherals
varies according to the SoCs.
If System MMU driver accepts multiple masters, everything is done in
DT.
But I worry that it is not easy if System MMU driver does not support
multiple masters.
I believe I have stated enough reasons why this kind of implementation
is bad. I'm not going to waste time repeating myself.
Your concerns presented above are valid, however they are not related to
what is wrong with this patch. I have given you two proper ways to
handle this, none should be forced upon particular IOMMU master drivers
- their authors should have the chance to select the method that works
best for them.
I don't still understand why you think this patch is wrong.
I think this is the best way not to think for all the driver developers
about other things than their business logic.
I agree, but one of the ways I proposed (an iommu-composite layer above
the IOMMU low level drivers) doesn't add any extra responsibility of
driver developers.
Moreover, it's this kind of business logic in low level drivers that is
adding more responsibility, because it introduces additional complexity
and makes the driver harder to read, maintain and extend in future.
This does not hurt anyone and I think this is good enough.
Well, it is barely good enough. It is a good practice to make a low
level driver handle a single device instance and this is how Linux
driver model is designed.
Moreover, a single device tree node _must_ represent a single hardware
block, so you can't group multiple SysMMUs into a single device tree node.
OK, you add nodes for single SysMMUs devices which is fine, sorry. I was
under impression that one kernel device (struct device) corresponds to
multiple SysMMUs, but this was before your patches, sorry. So one issue
less, but it's still not good.
Furthermore, if you force grouping of SysMMUs into a single virtual one,
you enforce using the same address space for all masters of some
particular hardware blocks, while potentially driver developers would
like to separate them.
Probably some clarification is needed. Your other patch adds:
sysmmu_fimd0w04: sysmmu@1464 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1464 0x1000;
interrupt-parent = combiner;
interrupts = 3 2;
clock-names = sysmmu, master;
clocks = clock 422, clock 421;
samsung,power-domain = disp_pd;
mmu-masters = fimd;
};
sysmmu_fimd0w123: sysmmu@1468 {
compatible = samsung,sysmmu-v3.3;
reg = 0x1468 0x1000;
interrupt-parent = combiner;
interrupts = 3 0;
clock-names = sysmmu, master;
clocks = clock 423, clock 421;
samsung,power-domain = disp_pd;
mmu-masters = fimd;
};
From such description, in future FIMD driver won't be able to determine
which SysMMU is used for windows 0 and 4 and which for windows 1, 2 and
3. However it would be desirable to
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote: Hi KyongHo, On 14.03.2014 06:10, Cho KyongHo wrote: Some master device descriptor like fimc-is which is an abstraction of very complex H/W may have multiple System MMUs. For those devices, the design of the link between System MMU and its master H/W is needed to be reconsidered. A link structure, sysmmu_list_data is introduced that provides a link to master H/W and that has a pointer to the device descriptor of a System MMU. Given a device descriptor of a master H/W, it is possible to traverse all System MMUs that must be controlled along with the master H/W. NAK. A device driver should handle particular hardware instances separately, without abstracting a virtual hardware instance consisting of multiple physical ones. If such abstraction is needed, it should be done above the exynos-iommu driver, e.g. by something like iommu-composite driver that would aggregate several IOMMUs. Keep in mind that such IOMMUs in a group could be different, e.g. different Exynos SysMMU versions or even completely different IPs handled by different drivers. Still, I don't think there is a real need for such abstraction. Instead, related drivers shall be fixed to properly handle multiple memory masters and their IOMMUs. G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother SoC like Exynos5250 does not. I don't understand why you are negative to this approach. This is the simplest than the others. Let me show you an example. FIMC-IS driver just controls MCU in FIMC-IS subsystem and the firmware of the MCU controls all other peripherals in the subsystem. Each peripherals have their own System MMU. Moreover, the configuration of the peripherals varies according to the SoCs. If System MMU driver accepts multiple masters, everything is done in DT. But I worry that it is not easy if System MMU driver does not support multiple masters. Thank you. KyongHo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On 18.03.2014 14:01, Cho KyongHo wrote: On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote: Hi KyongHo, On 14.03.2014 06:10, Cho KyongHo wrote: Some master device descriptor like fimc-is which is an abstraction of very complex H/W may have multiple System MMUs. For those devices, the design of the link between System MMU and its master H/W is needed to be reconsidered. A link structure, sysmmu_list_data is introduced that provides a link to master H/W and that has a pointer to the device descriptor of a System MMU. Given a device descriptor of a master H/W, it is possible to traverse all System MMUs that must be controlled along with the master H/W. NAK. A device driver should handle particular hardware instances separately, without abstracting a virtual hardware instance consisting of multiple physical ones. If such abstraction is needed, it should be done above the exynos-iommu driver, e.g. by something like iommu-composite driver that would aggregate several IOMMUs. Keep in mind that such IOMMUs in a group could be different, e.g. different Exynos SysMMU versions or even completely different IPs handled by different drivers. Still, I don't think there is a real need for such abstraction. Instead, related drivers shall be fixed to properly handle multiple memory masters and their IOMMUs. G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother SoC like Exynos5250 does not. I don't understand why you are negative to this approach. This is the simplest than the others. Let me show you an example. FIMC-IS driver just controls MCU in FIMC-IS subsystem and the firmware of the MCU controls all other peripherals in the subsystem. Each peripherals have their own System MMU. Moreover, the configuration of the peripherals varies according to the SoCs. If System MMU driver accepts multiple masters, everything is done in DT. But I worry that it is not easy if System MMU driver does not support multiple masters. I believe I have stated enough reasons why this kind of implementation is bad. I'm not going to waste time repeating myself. Your concerns presented above are valid, however they are not related to what is wrong with this patch. I have given you two proper ways to handle this, none should be forced upon particular IOMMU master drivers - their authors should have the chance to select the method that works best for them. Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On Tue, 18 Mar 2014 15:26:48 +0100, Tomasz Figa wrote: On 18.03.2014 14:01, Cho KyongHo wrote: On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote: Hi KyongHo, On 14.03.2014 06:10, Cho KyongHo wrote: Some master device descriptor like fimc-is which is an abstraction of very complex H/W may have multiple System MMUs. For those devices, the design of the link between System MMU and its master H/W is needed to be reconsidered. A link structure, sysmmu_list_data is introduced that provides a link to master H/W and that has a pointer to the device descriptor of a System MMU. Given a device descriptor of a master H/W, it is possible to traverse all System MMUs that must be controlled along with the master H/W. NAK. A device driver should handle particular hardware instances separately, without abstracting a virtual hardware instance consisting of multiple physical ones. If such abstraction is needed, it should be done above the exynos-iommu driver, e.g. by something like iommu-composite driver that would aggregate several IOMMUs. Keep in mind that such IOMMUs in a group could be different, e.g. different Exynos SysMMU versions or even completely different IPs handled by different drivers. Still, I don't think there is a real need for such abstraction. Instead, related drivers shall be fixed to properly handle multiple memory masters and their IOMMUs. G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother SoC like Exynos5250 does not. I don't understand why you are negative to this approach. This is the simplest than the others. Let me show you an example. FIMC-IS driver just controls MCU in FIMC-IS subsystem and the firmware of the MCU controls all other peripherals in the subsystem. Each peripherals have their own System MMU. Moreover, the configuration of the peripherals varies according to the SoCs. If System MMU driver accepts multiple masters, everything is done in DT. But I worry that it is not easy if System MMU driver does not support multiple masters. I believe I have stated enough reasons why this kind of implementation is bad. I'm not going to waste time repeating myself. Your concerns presented above are valid, however they are not related to what is wrong with this patch. I have given you two proper ways to handle this, none should be forced upon particular IOMMU master drivers - their authors should have the chance to select the method that works best for them. I don't still understand why you think this patch is wrong. I think this is the best way not to think for all the driver developers about other things than their business logic. This does not hurt anyone and I think this is good enough. If you want to provide another layer between master device and system mmu as you mentioned, you do that. This patch does not restrict it. Regards, KyongHo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
Hi KyongHo, On 14.03.2014 06:10, Cho KyongHo wrote: Some master device descriptor like fimc-is which is an abstraction of very complex H/W may have multiple System MMUs. For those devices, the design of the link between System MMU and its master H/W is needed to be reconsidered. A link structure, sysmmu_list_data is introduced that provides a link to master H/W and that has a pointer to the device descriptor of a System MMU. Given a device descriptor of a master H/W, it is possible to traverse all System MMUs that must be controlled along with the master H/W. NAK. A device driver should handle particular hardware instances separately, without abstracting a virtual hardware instance consisting of multiple physical ones. If such abstraction is needed, it should be done above the exynos-iommu driver, e.g. by something like iommu-composite driver that would aggregate several IOMMUs. Keep in mind that such IOMMUs in a group could be different, e.g. different Exynos SysMMU versions or even completely different IPs handled by different drivers. Still, I don't think there is a real need for such abstraction. Instead, related drivers shall be fixed to properly handle multiple memory masters and their IOMMUs. Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
