Re: [PATCH v12 18/31] iommu/exynos: allow having multiple System MMUs for a master H/W
On Tue, 06 May 2014 20:05:14 +0200, Tomasz Figa wrote: On 27.04.2014 09:37, Shaik Ameer Basha wrote: From: Cho KyongHo pullip@samsung.com 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 | 545 ++ 1 file changed, 335 insertions(+), 210 deletions(-) diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index fefedec3..c2e6365 100755 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -117,6 +117,10 @@ #define REG_PB1_EADDR 0x058 #define has_sysmmu(dev) (dev-archdata.iommu != NULL) +#define for_each_sysmmu_list(dev, list_data) \ + list_for_each_entry(list_data, \ + ((struct exynos_iommu_owner *)dev-archdata.iommu)-mmu_list, \ + entry) Sorry, NAK. Please don't add this kind of complexity and business logic to low level code. We want the configuration functions to be simple, easy to read, maintain and extend. The proper way to do it is to let the IOMMUs be grouped together on IOMMU subsystem level, so that each IOMMU consumer driver would see just one IOMMU, but then IOMMU driver callbacks would handle just particular instances of the IOMMU IP blocks, without any loops, lists and other crazy code... It is done in IOMMU driver internally. IOMMU consumer driver(IOMMU client device driver?) sees IOMMU domain but IOMMU itself. How to handle IOMMUs of the client device is just in charge of IOMMU driver according to the hardwired bus topology. If a master and IOMMU device should be 1:1 relationship as you contend, the master device driver should be responsible for express the bus topology in its business logic. I think it is not good as I told earlier. IOMMU group is different from the grouping System MMUs in Exynos IOMMU driver. IOMMU group is invented to group several IOMMUs on different bus with different masters to assign the same IOMMU domain. It is useful for user level virtualization but for expressing bus topology. Exynos IOMMU driver just groups System MMUs that have the same master device. Even though Exynos IOMMU driver implements IOMMU group which is already implemented by 20/31 patch, such looping is still required. However, I will consider other elegant way of interation of multiple System MMUs. Regards, KyongHo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 18/31] iommu/exynos: allow having multiple System MMUs for a master H/W
On 27.04.2014 09:37, Shaik Ameer Basha wrote: From: Cho KyongHo pullip@samsung.com 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 | 545 ++ 1 file changed, 335 insertions(+), 210 deletions(-) diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index fefedec3..c2e6365 100755 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -117,6 +117,10 @@ #define REG_PB1_EADDR 0x058 #define has_sysmmu(dev) (dev-archdata.iommu != NULL) +#define for_each_sysmmu_list(dev, list_data) \ + list_for_each_entry(list_data, \ + ((struct exynos_iommu_owner *)dev-archdata.iommu)-mmu_list, \ + entry) Sorry, NAK. Please don't add this kind of complexity and business logic to low level code. We want the configuration functions to be simple, easy to read, maintain and extend. The proper way to do it is to let the IOMMUs be grouped together on IOMMU subsystem level, so that each IOMMU consumer driver would see just one IOMMU, but then IOMMU driver callbacks would handle just particular instances of the IOMMU IP blocks, without any loops, lists and other crazy code... Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 18/31] iommu/exynos: allow having multiple System MMUs for a master H/W
On Mon, 28 Apr 2014 16:08:14 +0530, Tushar Behera wrote: On 04/27/2014 01:07 PM, Shaik Ameer Basha wrote: From: Cho KyongHo pullip@samsung.com 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 Since you are posting the patches, you should also add your Signed-of-by. --- drivers/iommu/exynos-iommu.c | 545 ++ 1 file changed, 335 insertions(+), 210 deletions(-) diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index fefedec3..c2e6365 100755 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c [ ... ] static int sysmmu_pm_genpd_save_state(struct device *dev) @@ -1215,7 +1349,7 @@ static int sysmmu_pm_genpd_save_state(struct device *dev) ret = cb(dev); if (ret == 0) - sysmmu_save_state(client-sysmmu); + sysmmu_save_state(dev); client is now unused, remove the variable. return ret; } @@ -1238,13 +1372,13 @@ static int sysmmu_pm_genpd_restore_state(struct device *dev) if (!cb dev-driver dev-driver-pm) cb = dev-driver-pm-runtime_resume; - sysmmu_restore_state(client-sysmmu); + sysmmu_restore_state(dev); if (cb) ret = cb(dev); if (ret) - sysmmu_save_state(client-sysmmu); + sysmmu_restore_state(dev); client is now unused, remove the variable. Ok. Thanks. KyongHo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 18/31] iommu/exynos: allow having multiple System MMUs for a master H/W
On 04/27/2014 01:07 PM, Shaik Ameer Basha wrote: From: Cho KyongHo pullip@samsung.com 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 Since you are posting the patches, you should also add your Signed-of-by. --- drivers/iommu/exynos-iommu.c | 545 ++ 1 file changed, 335 insertions(+), 210 deletions(-) diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index fefedec3..c2e6365 100755 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c [ ... ] static int sysmmu_pm_genpd_save_state(struct device *dev) @@ -1215,7 +1349,7 @@ static int sysmmu_pm_genpd_save_state(struct device *dev) ret = cb(dev); if (ret == 0) - sysmmu_save_state(client-sysmmu); + sysmmu_save_state(dev); client is now unused, remove the variable. return ret; } @@ -1238,13 +1372,13 @@ static int sysmmu_pm_genpd_restore_state(struct device *dev) if (!cb dev-driver dev-driver-pm) cb = dev-driver-pm-runtime_resume; - sysmmu_restore_state(client-sysmmu); + sysmmu_restore_state(dev); if (cb) ret = cb(dev); if (ret) - sysmmu_save_state(client-sysmmu); + sysmmu_restore_state(dev); client is now unused, remove the variable. -- Tushar Behera ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
