Re: [PATCH 00/29] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem
Hello, On 2014-08-19 01:32, Joerg Roedel wrote: On Tue, Aug 05, 2014 at 12:47:28PM +0200, Marek Szyprowski wrote: .../devicetree/bindings/iommu/samsung,sysmmu.txt | 93 ++- Documentation/power/notifiers.txt | 14 + arch/arm/boot/dts/exynos4.dtsi | 118 arch/arm/boot/dts/exynos4210.dtsi | 23 + arch/arm/boot/dts/exynos4x12.dtsi | 82 +++ arch/arm/include/asm/dma-iommu.h | 36 ++ arch/arm/mach-exynos/pm_domains.c | 12 +- arch/arm/mach-integrator/impd1.c | 2 +- arch/arm/mm/dma-mapping.c | 47 ++ drivers/base/bus.c | 4 +- drivers/base/dd.c | 10 +- drivers/base/platform.c| 2 +- drivers/base/power/domain.c| 70 ++- drivers/clk/samsung/clk-exynos4.c | 1 + drivers/gpu/drm/exynos/exynos_drm_fimc.c | 1 + drivers/gpu/drm/exynos/exynos_drm_fimd.c | 26 +- drivers/gpu/drm/exynos/exynos_drm_g2d.c| 1 + drivers/gpu/drm/exynos/exynos_drm_gsc.c| 1 + drivers/gpu/drm/exynos/exynos_drm_rotator.c| 1 + drivers/gpu/drm/exynos/exynos_mixer.c | 1 + drivers/iommu/exynos-iommu.c | 663 + drivers/iommu/iommu.c | 3 + drivers/media/platform/s5p-mfc/s5p_mfc.c | 107 ++-- drivers/pci/host/pci-mvebu.c | 2 +- drivers/pci/host/pci-rcar-gen2.c | 2 +- drivers/pci/host/pci-tegra.c | 2 +- drivers/pci/host/pcie-rcar.c | 2 +- drivers/soc/tegra/pmc.c| 2 +- include/dt-bindings/clock/exynos4.h| 10 +- include/linux/device.h | 12 +- include/linux/iommu.h | 1 + include/linux/pm.h | 2 + include/linux/pm_domain.h | 19 + 33 files changed, 1016 insertions(+), 356 deletions(-) This touches a lot of non-iommu stuff. What is your strategy on getting this in, do you plan to get the non-iommu changes merged first or do you want to collect the respective Acks and merge this all through one tree? Those patches are posted as one patchset mainly to demonstrate how to get everything to work together. I also posted this as a single patch series to get some feedback from other iommu developers, especially all those involved in the generic iommu dt bindings. For merging, I will split them into smaller series and try to get respective acks. Best regards -- Marek Szyprowski, PhD Samsung RD Institute Poland ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
Hi Hiroshi,
-Original Message-
From: Hiroshi Doyu [mailto:hd...@nvidia.com]
Sent: Thursday, August 14, 2014 9:35 PM
To: Sethi Varun-B16395
Cc: Hiroshi Doyu; Thierry Reding; Stephen Warren; Arnd Bergmann; Will
Deacon; Mark Rutland; devicet...@vger.kernel.org; Olof Johansson;
iommu@lists.linux-foundation.org; Rob Herring; linux-te...@vger.kernel.org;
linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
Hi Varun,
Varun Sethi varun.se...@freescale.com writes:
-Original Message-
From: iommu-boun...@lists.linux-foundation.org [mailto:iommu-
boun...@lists.linux-foundation.org] On Behalf Of Hiroshi Doyu
Sent: Thursday, August 14, 2014 12:18 PM
To: Thierry Reding; Stephen Warren; Arnd Bergmann; Will Deacon
Cc: Mark Rutland; devicet...@vger.kernel.org; Olof Johansson;
iommu@lists.linux-foundation.org; Rob Herring;
linux-te...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree
bindings
Thierry Reding thierry.red...@gmail.com writes:
+Multiple-master IOMMU:
+--
+
+ iommu {
+ /* the specifier represents the ID of the master */
+ #iommu-cells = 1;
+ };
+
+ master@1 {
+ /* device has master ID 42 in the IOMMU */
+ iommus = {/iommu} 42;
+ };
+
+ master@2 {
+ /* device has master IDs 23 and 24 in the IOMMU */
+ iommus = {/iommu} 23, {/iommu} 24;
+ };
I think that this master ID will be parsed in IOMMU driver. For
example, ARM,SMMU expects streamID as master ID, right?
If a SoC has a feature to configure to assign streamID to devices at
runtime, streamID is not equal to master ID.
iommus = {/smmu} soc specific master ID;
soc master ID needs to be translated into streamID by SoC SW. It
seems that ARM,SMMU kernel driver doesn't expect this kind of ID
translation. If ARM,SMMU kernel driver is used as is, soc master ID
would be incompatible? ARM,SMMU needs such translation before
parsing. Is this my understanding right?
If so I think that this master ID configuration/translation may be
quite reasonable requirment for SoC using ARM,SMMU.
Can we consider this ID translation within ARM,SMMU compatibility?
IOW, is it possible to implement some SoC specific hook for ID
translation/configuration in ARM,SMMU kernel driver?
Can the id translation be done using a SMR mask?
No, SoC master ID is completely independenf of SMR.
Also, for dynamic stream ID allocation we would need to represent the
specific master register (to store the stream ID) in the device tree.
I assmue that the above means that iMX has such configuration register to map
steramID and a device dynamically.
We have per master registers for setting the stream ID on the Layerscape
platforms. My point was that we would need the iommu master node
to include a reference to the master id register.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 42;
master-id-reg = phandle offset
};
-Varun
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
Varun Sethi varun.se...@freescale.com writes:
Hi Hiroshi,
-Original Message-
From: Hiroshi Doyu [mailto:hd...@nvidia.com]
Sent: Thursday, August 14, 2014 9:35 PM
To: Sethi Varun-B16395
Cc: Hiroshi Doyu; Thierry Reding; Stephen Warren; Arnd Bergmann; Will
Deacon; Mark Rutland; devicet...@vger.kernel.org; Olof Johansson;
iommu@lists.linux-foundation.org; Rob Herring; linux-te...@vger.kernel.org;
linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
Hi Varun,
Varun Sethi varun.se...@freescale.com writes:
-Original Message-
From: iommu-boun...@lists.linux-foundation.org [mailto:iommu-
boun...@lists.linux-foundation.org] On Behalf Of Hiroshi Doyu
Sent: Thursday, August 14, 2014 12:18 PM
To: Thierry Reding; Stephen Warren; Arnd Bergmann; Will Deacon
Cc: Mark Rutland; devicet...@vger.kernel.org; Olof Johansson;
iommu@lists.linux-foundation.org; Rob Herring;
linux-te...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree
bindings
Thierry Reding thierry.red...@gmail.com writes:
+Multiple-master IOMMU:
+--
+
+ iommu {
+ /* the specifier represents the ID of the master */
+ #iommu-cells = 1;
+ };
+
+ master@1 {
+ /* device has master ID 42 in the IOMMU */
+ iommus = {/iommu} 42;
+ };
+
+ master@2 {
+ /* device has master IDs 23 and 24 in the IOMMU */
+ iommus = {/iommu} 23, {/iommu} 24;
+ };
I think that this master ID will be parsed in IOMMU driver. For
example, ARM,SMMU expects streamID as master ID, right?
If a SoC has a feature to configure to assign streamID to devices at
runtime, streamID is not equal to master ID.
iommus = {/smmu} soc specific master ID;
soc master ID needs to be translated into streamID by SoC SW. It
seems that ARM,SMMU kernel driver doesn't expect this kind of ID
translation. If ARM,SMMU kernel driver is used as is, soc master ID
would be incompatible? ARM,SMMU needs such translation before
parsing. Is this my understanding right?
If so I think that this master ID configuration/translation may be
quite reasonable requirment for SoC using ARM,SMMU.
Can we consider this ID translation within ARM,SMMU compatibility?
IOW, is it possible to implement some SoC specific hook for ID
translation/configuration in ARM,SMMU kernel driver?
Can the id translation be done using a SMR mask?
No, SoC master ID is completely independenf of SMR.
Also, for dynamic stream ID allocation we would need to represent the
specific master register (to store the stream ID) in the device tree.
I assmue that the above means that iMX has such configuration register to map
steramID and a device dynamically.
We have per master registers for setting the stream ID on the
Layerscape platforms. My point was that we would need the iommu master
node to include a reference to the master id register.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 42;
master-id-reg = phandle offset
};
In the above, for iommus= bindings, you wouldn't need to break
ARM,SMMU compatibility at all if you set streamID exactly as below.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 'any given streamID';
master-id-reg = phandle offset
};
And your SoC needs to register bus_notifier and ADD_DEVICE should
configure to map 'any given streamID' to a device via the above
register. This wouldn't need any modification from ARM,SMMU driver and
keep the iommus bindings as it is.
IOW, SoC only needs to register ADD_DEVICE in bus_notifier to map
StreamID to a device. This needs to be executed earlier than IOMMU bus's
ADD_DEVICE, though.
Is my understanding right?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
-Original Message-
From: Hiroshi Doyu [mailto:hd...@nvidia.com]
Sent: Tuesday, August 19, 2014 3:34 PM
To: Sethi Varun-B16395; Will Deacon
Cc: Hiroshi Doyu; Thierry Reding; Stephen Warren; Arnd Bergmann; Mark
Rutland; devicet...@vger.kernel.org; Olof Johansson; iommu@lists.linux-
foundation.org; Rob Herring; linux-te...@vger.kernel.org; linux-arm-
ker...@lists.infradead.org; Yoder Stuart-B08248
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
Varun Sethi varun.se...@freescale.com writes:
Hi Hiroshi,
-Original Message-
From: Hiroshi Doyu [mailto:hd...@nvidia.com]
Sent: Thursday, August 14, 2014 9:35 PM
To: Sethi Varun-B16395
Cc: Hiroshi Doyu; Thierry Reding; Stephen Warren; Arnd Bergmann; Will
Deacon; Mark Rutland; devicet...@vger.kernel.org; Olof Johansson;
iommu@lists.linux-foundation.org; Rob Herring;
linux-te...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree
bindings
Hi Varun,
Varun Sethi varun.se...@freescale.com writes:
-Original Message-
From: iommu-boun...@lists.linux-foundation.org [mailto:iommu-
boun...@lists.linux-foundation.org] On Behalf Of Hiroshi Doyu
Sent: Thursday, August 14, 2014 12:18 PM
To: Thierry Reding; Stephen Warren; Arnd Bergmann; Will Deacon
Cc: Mark Rutland; devicet...@vger.kernel.org; Olof Johansson;
iommu@lists.linux-foundation.org; Rob Herring;
linux-te...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree
bindings
Thierry Reding thierry.red...@gmail.com writes:
+Multiple-master IOMMU:
+--
+
+ iommu {
+ /* the specifier represents the ID of the master */
+ #iommu-cells = 1;
+ };
+
+ master@1 {
+ /* device has master ID 42 in the IOMMU */
+ iommus = {/iommu} 42;
+ };
+
+ master@2 {
+ /* device has master IDs 23 and 24 in the IOMMU */
+ iommus = {/iommu} 23, {/iommu} 24;
+ };
I think that this master ID will be parsed in IOMMU driver. For
example, ARM,SMMU expects streamID as master ID, right?
If a SoC has a feature to configure to assign streamID to devices
at runtime, streamID is not equal to master ID.
iommus = {/smmu} soc specific master ID;
soc master ID needs to be translated into streamID by SoC SW.
It seems that ARM,SMMU kernel driver doesn't expect this kind of
ID translation. If ARM,SMMU kernel driver is used as is, soc master ID
would be incompatible? ARM,SMMU needs such translation before
parsing. Is this my understanding right?
If so I think that this master ID configuration/translation may be
quite reasonable requirment for SoC using ARM,SMMU.
Can we consider this ID translation within ARM,SMMU compatibility?
IOW, is it possible to implement some SoC specific hook for ID
translation/configuration in ARM,SMMU kernel driver?
Can the id translation be done using a SMR mask?
No, SoC master ID is completely independenf of SMR.
Also, for dynamic stream ID allocation we would need to represent
the specific master register (to store the stream ID) in the device tree.
I assmue that the above means that iMX has such configuration
register to map steramID and a device dynamically.
We have per master registers for setting the stream ID on the
Layerscape platforms. My point was that we would need the iommu master
node to include a reference to the master id register.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 42;
master-id-reg = phandle offset };
In the above, for iommus= bindings, you wouldn't need to break
ARM,SMMU compatibility at all if you set streamID exactly as below.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 'any given streamID';
master-id-reg = phandle offset
};
And your SoC needs to register bus_notifier and ADD_DEVICE should configure
to map 'any given streamID' to a device via the above register. This wouldn't
need any modification from ARM,SMMU driver and keep the iommus bindings
as it is.
IOW, SoC only needs to register ADD_DEVICE in bus_notifier to map StreamID
to a device. This needs to be executed earlier than IOMMU bus's ADD_DEVICE,
though.
Is my understanding right?
I don't think that SOC specific code needs a bus notifier for setting the
stream ID. It can be done as a part of SOC specific initialization. The device
tree can be updated to reflect the correct stream ID (SMMU driver can get the
updated stream ID from device tree).
I was thinking more on the lines of updating the device
Re: [PATCH 1/1] iommu/vt-d : clear old root entry for dump kernel
On Mon, Aug 18, 2014 at 11:27:01PM +, Li, Zhen-Hua wrote: : [fault reason 01] Present bit in root entry is clear It appears when iommu initializing in the kdump kernel. Hmm, do you have an explanation how this can happen? From how I read the code, the kdump kernel disables translation on the IOMMU, then sets a new root entry, and then re-enabled translation. To me it looks like there is no point in time where translation is enabled and the root-entry is clear (present-bit==0). But obviously I am missing something if you see the message above. Btw, have you looked into this patch-set posted earlier this year: https://lkml.org/lkml/2014/4/24/836 It approaches the same problem-space, but also cares about in-flight DMA. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
Varun Sethi varun.se...@freescale.com writes:
Also, for dynamic stream ID allocation we would need to represent
the specific master register (to store the stream ID) in the device
tree.
I assmue that the above means that iMX has such configuration
register to map steramID and a device dynamically.
We have per master registers for setting the stream ID on the
Layerscape platforms. My point was that we would need the iommu master
node to include a reference to the master id register.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 42;
master-id-reg = phandle offset };
In the above, for iommus= bindings, you wouldn't need to break
ARM,SMMU compatibility at all if you set streamID exactly as below.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 'any given streamID';
master-id-reg = phandle offset
};
And your SoC needs to register bus_notifier and ADD_DEVICE should configure
to map 'any given streamID' to a device via the above register. This wouldn't
need any modification from ARM,SMMU driver and keep the iommus bindings
as it is.
IOW, SoC only needs to register ADD_DEVICE in bus_notifier to map StreamID
to a device. This needs to be executed earlier than IOMMU bus's ADD_DEVICE,
though.
Is my understanding right?
I don't think that SOC specific code needs a bus notifier for setting
the stream ID. It can be done as a part of SOC specific
initialization. The device tree can be updated to reflect the correct
stream ID (SMMU driver can get the updated stream ID from device
tree).
That's possible.
I was thinking more on the lines of updating the device stream id
while attaching a device to the domain.
I thought the same but this would break the ARM,SMMU /compatibility/
since the 1st param of iommus= is always expected as streamID.
If streamID can be assigned dynamically like PCIe, not like streamID
statically set in DT, how should we describe this dynmaic steramID
shifting/assignment in DT?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu: exynos: Fix trivial typos
On Mon, Aug 04, 2014 at 10:06:28AM +0530, Sachin Kamat wrote: Fixed trivial typos and grammar to improve readability. Changed w/a to workaround. Signed-off-by: Sachin Kamat sachin.ka...@samsung.com --- drivers/iommu/exynos-iommu.c | 51 ++-- 1 file changed, 26 insertions(+), 25 deletions(-) Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: Don't store SIRTP request
On Mon, Aug 11, 2014 at 01:13:25PM +0200, Jan Kiszka wrote: Don't store the SIRTP request bit in the register state. It will otherwise become sticky and could request an Interrupt Remap Table Pointer update on each command register write. Found while starting to emulate IR in QEMU, not by observing problems on real hardware. Signed-off-by: Jan Kiszka jan.kis...@siemens.com --- drivers/iommu/intel_irq_remapping.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) Applied, thanks Jan. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/vt-d: Add new macros for invalidation event
On Fri, Aug 15, 2014 at 09:55:51AM +0800, Li, Zhen-Hua wrote: According to intel's spec Intel® Virtualization Technology for Directed I/O, Revision: 1.3 , February 2011, Chaper 10.4.25 to 10.4.28 There are four registers IECTL_REG 0xa0Invalidation event control register IEDATA_REG 0xa4Invalidation event data register IEADDR_REG 0xa8Invalidation event address register IEUADDR_REG 0xacInvalidation event upper address register Through they are not used in kernel in the latest version, the defination should be added to kernel as well as other registers. Signed-off-by: Li, Zhen-Hua zhen-h...@hp.com --- include/linux/intel-iommu.h | 4 1 file changed, 4 insertions(+) diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h index a65208a..15fafd5 100644 --- a/include/linux/intel-iommu.h +++ b/include/linux/intel-iommu.h @@ -56,6 +56,10 @@ #define DMAR_IQ_SHIFT4 /* Invalidation queue head/tail shift */ #define DMAR_IQA_REG 0x90/* Invalidation queue addr register */ #define DMAR_ICS_REG 0x9c/* Invalidation complete status register */ +#define DMAR_IECTL_REG 0xa0/* Invalidation event control register */ +#define DMAR_IEDATA_REG 0xa4/* Invalidation event data register */ +#define DMAR_IEADDR_REG 0xa8/* Invalidation event address register */ +#define DMAR_IEUADDR_REG 0xac/* Invalidation event upper address register */ #define DMAR_IRTA_REG0xb8/* Interrupt remapping table addr register */ #define OFFSET_STRIDE(9) There is no point in adding register defines that are not used anywhere. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Replace rcu_assign_pointer() with RCU_INIT_POINTER()
On Mon, Aug 18, 2014 at 03:20:56PM +0300, Andreea-Cristina Bernat wrote: The use of rcu_assign_pointer() is NULLing out the pointer. According to RCU_INIT_POINTER()'s block comment: 1. This use of RCU_INIT_POINTER() is NULLing out the pointer it is better to use it instead of rcu_assign_pointer() because it has a smaller overhead. The following Coccinelle semantic patch was used: @@ @@ - rcu_assign_pointer + RCU_INIT_POINTER (..., NULL) Signed-off-by: Andreea-Cristina Bernat bernat@gmail.com Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions
On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap. But sg-offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s-offset + s-length' you use it the same way). So when you pass iova + offset the result will no longer be page-aligned. You should force sg-offset == 0 and sg-length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API. Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg). Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too. I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions). Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/29] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem
Hello, On 2014-08-19 13:39, Andreas Färber wrote: Hi Marek and Inki, Am 19.08.2014 08:07, schrieb Marek Szyprowski: On 2014-08-19 01:32, Joerg Roedel wrote: On Tue, Aug 05, 2014 at 12:47:28PM +0200, Marek Szyprowski wrote: [...] 33 files changed, 1016 insertions(+), 356 deletions(-) This touches a lot of non-iommu stuff. What is your strategy on getting this in, do you plan to get the non-iommu changes merged first or do you want to collect the respective Acks and merge this all through one tree? Those patches are posted as one patchset mainly to demonstrate how to get everything to work together. I also posted this as a single patch series to get some feedback from other iommu developers, especially all those involved in the generic iommu dt bindings. For merging, I will split them into smaller series and try to get respective acks. I'm working on 5250 based Spring Chromebook and noticed that v3.17-rc1 got some more iommu support. With the new CONFIG_DRM_EXYNOS_IOMMU=y my machine stops booting. So I'm wondering, is any of this a fix for 3.17, or is all of this unrelated -next material? This is probably a side effect of patch 3170447c1f264d51b8d1f3898bf2588588a64fdc (iommu/exynos: Select ARM_DMA_USE_IOMMU). It added selection of ARM_DMA_USE_IOMMU symbol, on which IOMMU support in Exynos DRM subsystem depends. However selecting this symbol is all that this patch does, without providing any code code which implements real support for ARM DMA IOMMU integration, which is needed by Exynos DRM driver. Please disable CONFIG_DRM_EXYNOS_IOMMU in kernel .config and your system should be bootable again. Also, are you or someone working on the respective DT changes for Exynos5? I can prepare DT changes for Exynos5 as well, but first I wanted to clarify if everyone involved in generic IOMMU bindings and Exynos IOMMU driver agrees on my proposal. Best regards -- Marek Szyprowski, PhD Samsung RD Institute Poland ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
-Original Message-
From: Hiroshi Doyu [mailto:hd...@nvidia.com]
Sent: Tuesday, August 19, 2014 4:33 PM
To: Sethi Varun-B16395; Will Deacon
Cc: Hiroshi Doyu; Thierry Reding; Stephen Warren; Arnd Bergmann; Mark
Rutland; devicet...@vger.kernel.org; Olof Johansson; iommu@lists.linux-
foundation.org; Rob Herring; linux-te...@vger.kernel.org; linux-arm-
ker...@lists.infradead.org; Yoder Stuart-B08248
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
Varun Sethi varun.se...@freescale.com writes:
Also, for dynamic stream ID allocation we would need to
represent the specific master register (to store the stream ID) in the
device tree.
I assmue that the above means that iMX has such configuration
register to map steramID and a device dynamically.
We have per master registers for setting the stream ID on the
Layerscape platforms. My point was that we would need the iommu
master node to include a reference to the master id register.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 42;
master-id-reg = phandle offset };
In the above, for iommus= bindings, you wouldn't need to break
ARM,SMMU compatibility at all if you set streamID exactly as below.
master@1 {
/* device has master ID 42 in the IOMMU */
iommus = {/iommu} 'any given streamID';
master-id-reg = phandle offset
};
And your SoC needs to register bus_notifier and ADD_DEVICE should
configure to map 'any given streamID' to a device via the above
register. This wouldn't need any modification from ARM,SMMU driver
and keep the iommus bindings as it is.
IOW, SoC only needs to register ADD_DEVICE in bus_notifier to map
StreamID to a device. This needs to be executed earlier than IOMMU
bus's ADD_DEVICE, though.
Is my understanding right?
I don't think that SOC specific code needs a bus notifier for setting
the stream ID. It can be done as a part of SOC specific
initialization. The device tree can be updated to reflect the correct
stream ID (SMMU driver can get the updated stream ID from device
tree).
That's possible.
I was thinking more on the lines of updating the device stream id
while attaching a device to the domain.
I thought the same but this would break the ARM,SMMU /compatibility/ since
the 1st param of iommus= is always expected as streamID.
If streamID can be assigned dynamically like PCIe, not like streamID
statically
set in DT, how should we describe this dynmaic steramID shifting/assignment in
DT?
Can you dynamically program the stream ID for the device? If yes, then you
require a unique numberspace for allocating a streamID. I am not clear on the
stream ID translation requirement.
-Varun
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
Hi Will,
-Original Message-
From: iommu-boun...@lists.linux-foundation.org [mailto:iommu-
boun...@lists.linux-foundation.org] On Behalf Of Will Deacon
Sent: Friday, August 15, 2014 5:21 PM
To: Hiroshi Doyu
Cc: Mark Rutland; devicet...@vger.kernel.org; Stephen Warren; Arnd
Bergmann; Rob Herring; iommu@lists.linux-foundation.org; Thierry Reding;
linux-te...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH v5] devicetree: Add generic IOMMU device tree bindings
On Thu, Aug 14, 2014 at 07:47:42AM +0100, Hiroshi Doyu wrote:
Thierry Reding thierry.red...@gmail.com writes:
+Multiple-master IOMMU:
+--
+
+ iommu {
+ /* the specifier represents the ID of the master */
+ #iommu-cells = 1;
+ };
+
+ master@1 {
+ /* device has master ID 42 in the IOMMU */
+ iommus = {/iommu} 42;
+ };
+
+ master@2 {
+ /* device has master IDs 23 and 24 in the IOMMU */
+ iommus = {/iommu} 23, {/iommu} 24;
+ };
I think that this master ID will be parsed in IOMMU driver. For
example, ARM,SMMU expects streamID as master ID, right?
If a SoC has a feature to configure to assign streamID to devices at
runtime, streamID is not equal to master ID.
iommus = {/smmu} soc specific master ID;
soc master ID needs to be translated into streamID by SoC SW. It
seems that ARM,SMMU kernel driver doesn't expect this kind of ID
translation. If ARM,SMMU kernel driver is used as is, soc master ID
would be incompatible? ARM,SMMU needs such translation before parsing.
Is this my understanding right?
If so I think that this master ID configuration/translation may be
quite reasonable requirment for SoC using ARM,SMMU.
Can we consider this ID translation within ARM,SMMU compatibility?
IOW, is it possible to implement some SoC specific hook for ID
translation/configuration in ARM,SMMU kernel driver?
I think there's some confusion here. The ARM architected SMMU does not
perform any StreamID translation -- it sees an incoming ID and uses that to
lookup a set of translation tables.
I don't completely agree with this. In case of MMU-500 don't we have the
TBUID + device assigned stream ID representing the stream ID for matching at
the TCU? In case of our platform, we have hot pluggable device objects
comprising of devices connected to different TBUs. We have a requirement to use
a single stream ID for all the distributed objects. We will have to use the
stream ID masking capability to mask out the TBU ID.
-Varun
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/6] iommu/arm-smmu: add support for iova_to_phys through ATS1PR
On Wed, Aug 13, 2014 at 01:51:36AM +0100, Mitchel Humpherys wrote:
Currently, we provide the iommu_ops.iova_to_phys service by doing a
table walk in software to translate IO virtual addresses to physical
addresses. On SMMUs that support it, it can be useful to ask the SMMU
itself to do the translation. This can be used to warm the TLBs for an
SMMU. It can also be useful for testing and hardware validation.
I'm not really sold on the usefulness of this feature. If you want hardware
validation features, I'd rather do something through debugfs, but your
use-case for warming the TLBs is intriguing. Do you have an example use-case
with performance figures?
Since the address translation registers are optional on SMMUv2, only
enable hardware translations when using SMMUv1 or when SMMU_IDR0.S1TS=1
and SMMU_IDR0.ATOSNS=0, as described in the ARM SMMU v1-v2 spec.
[...]
+static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
+ dma_addr_t iova)
+{
+ struct arm_smmu_domain *smmu_domain = domain-priv;
+ struct arm_smmu_device *smmu = smmu_domain-smmu;
+ struct arm_smmu_cfg *cfg = smmu_domain-cfg;
+ struct device *dev = smmu-dev;
+ void __iomem *cb_base;
+ int count = 0;
+ u64 phys;
+
+ arm_smmu_enable_clocks(smmu);
+
+ cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg-cbndx);
+
+ if (smmu-version == 1) {
+ u32 reg = iova 0xF000;
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
+ } else {
+ u64 reg = iova 0xf000;
+ writeq_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
We don't have writeq for arch/arm/.
+ }
+
+ mb();
Why?
+ while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) ATSR_ACTIVE) {
+ if (++count == ATSR_LOOP_TIMEOUT) {
+ dev_err(dev,
+ iova to phys timed out on 0x%pa for %s.
Falling back to software table walk.\n,
+ iova, dev_name(dev));
+ arm_smmu_disable_clocks(smmu);
+ return arm_smmu_iova_to_phys_soft(domain, iova);
+ }
+ cpu_relax();
+ }
Do you know what happened to Olav's patches to make this sort of code
generic?
@@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device
*smmu)
return -ENODEV;
}
+ if (smmu-version == 1 || (!(id ID0_ATOSNS) (id ID0_S1TS))) {
Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS
is also clear.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 6/6] iommu/arm-smmu: add .domain_{set,get}_attr for coherent walk control
On Wed, Aug 13, 2014 at 01:51:39AM +0100, Mitchel Humpherys wrote:
Under certain conditions coherent hardware translation table walks can
result in degraded performance. Add a new domain attribute to
disable/enable this feature in generic code along with the domain
attribute setter and getter to handle it in the ARM SMMU driver.
Again, it would be nice to have some information about these cases and the
performance issues that you are seeing.
@@ -1908,11 +1917,15 @@ static int arm_smmu_domain_get_attr(struct
iommu_domain *domain,
enum iommu_attr attr, void *data)
{
struct arm_smmu_domain *smmu_domain = domain-priv;
+ struct arm_smmu_cfg *cfg = smmu_domain-cfg;
switch (attr) {
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain-stage == ARM_SMMU_DOMAIN_NESTED);
return 0;
+ case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
+ *((bool *)data) = cfg-htw_disable;
+ return 0;
I think I'd be more comfortable using int instead of bool for this, as it
could well end up in the user ABI if vfio decides to make use of it. While
we're here, let's also add an attributes bitmap to the arm_smmu_domain
instead of having a bool in the arm_smmu_cfg.
default:
return -ENODEV;
}
@@ -1922,6 +1935,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain
*domain,
enum iommu_attr attr, void *data)
{
struct arm_smmu_domain *smmu_domain = domain-priv;
+ struct arm_smmu_cfg *cfg = smmu_domain-cfg;
switch (attr) {
case DOMAIN_ATTR_NESTING:
@@ -1933,6 +1947,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain
*domain,
smmu_domain-stage = ARM_SMMU_DOMAIN_S1;
return 0;
+ case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
+ cfg-htw_disable = *((bool *)data);
+ return 0;
default:
return -ENODEV;
}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0550286df4..8a6449857a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -81,6 +81,7 @@ enum iommu_attr {
DOMAIN_ATTR_FSL_PAMU_ENABLE,
DOMAIN_ATTR_FSL_PAMUV1,
DOMAIN_ATTR_NESTING,/* two stages of translation */
+ DOMAIN_ATTR_COHERENT_HTW_DISABLE,
I wonder whether we should make this ARM-specific. Can you take a quick look
to see if any of the other IOMMUs can potentially benefit from this?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/6] iommu/arm-smmu: implement generic DT bindings
[adding Rob, Mark, Arnd, Thierry and Hiroshi]
On Wed, Aug 13, 2014 at 01:51:37AM +0100, Mitchel Humpherys wrote:
Generic IOMMU device tree bindings were recently added in
[devicetree: Add generic IOMMU device tree bindings]. Implement the
bindings in the ARM SMMU driver.
See Documentation/devicetree/bindings/iommu/iommu.txt for the bindings
themselves.
Could you look at moving the parsing code into a separate file please (maybe
under lib/ ?). That way, other IOMMUs can use the same binding without the
boilerplate having to be rewritten each time.
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 63c6707fad..22e25f3172 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -538,25 +538,32 @@ static int insert_smmu_master(struct arm_smmu_device
*smmu,
return 0;
}
+struct iommus_entry {
+ struct list_head list;
+ struct device_node *node;
+ u16 streamids[MAX_MASTER_STREAMIDS];
+ int num_sids;
+};
+
static int register_smmu_master(struct arm_smmu_device *smmu,
- struct device *dev,
- struct of_phandle_args *masterspec)
+ struct iommus_entry *entry)
{
int i;
struct arm_smmu_master *master;
+ struct device *dev = smmu-dev;
- master = find_smmu_master(smmu, masterspec-np);
+ master = find_smmu_master(smmu, entry-node);
if (master) {
dev_err(dev,
rejecting multiple registrations for master device
%s\n,
- masterspec-np-name);
+ entry-node-name);
return -EBUSY;
}
- if (masterspec-args_count MAX_MASTER_STREAMIDS) {
+ if (entry-num_sids MAX_MASTER_STREAMIDS) {
dev_err(dev,
reached maximum number (%d) of stream IDs for master
device %s\n,
- MAX_MASTER_STREAMIDS, masterspec-np-name);
+ MAX_MASTER_STREAMIDS, entry-node-name);
return -ENOSPC;
}
@@ -564,15 +571,58 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
if (!master)
return -ENOMEM;
- master-of_node = masterspec-np;
- master-cfg.num_streamids = masterspec-args_count;
+ master-of_node = entry-node;
+ master-cfg.num_streamids = entry-num_sids;
for (i = 0; i master-cfg.num_streamids; ++i)
- master-cfg.streamids[i] = masterspec-args[i];
+ master-cfg.streamids[i] = entry-streamids[i];
return insert_smmu_master(smmu, master);
}
+static int arm_smmu_parse_iommus_properties(struct arm_smmu_device *smmu,
+ int *num_masters)
+{
+ struct of_phandle_args iommuspec;
+ struct device_node *dn;
+
+ for_each_node_with_property(dn, iommus) {
+ int arg_ind = 0;
+ struct iommus_entry *entry, *n;
+ LIST_HEAD(iommus);
+
+ while (!of_parse_phandle_with_args(dn, iommus, #iommu-cells,
+ arg_ind, iommuspec)) {
We need to check that the phandle does indeed point at one of our SMMUs
here, in case we have a system with multiple IOMMU types, all using the
generic binding.
+ int i;
+
+ list_for_each_entry(entry, iommus, list)
+ if (entry-node == dn)
+ break;
Oh, yuck, this is really nasty to parse...
+ if (entry-list == iommus) {
Where is entry initialised the first time around?
+ entry = devm_kzalloc(smmu-dev, sizeof(*entry),
+ GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
You need to of_node_put the guys you've got back from the phandle parsing
code.
+ entry-node = dn;
+ list_add(entry-list, iommus);
+ }
+ entry-num_sids = iommuspec.args_count;
+ for (i = 0; i entry-num_sids; ++i)
+ entry-streamids[i] = iommuspec.args[i];
+ arg_ind++;
Isn't this defined by #iommu-cells?
+ }
+
+ list_for_each_entry_safe(entry, n, iommus, list) {
Why the _safe variant? This is a local list, right?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/6] iommu/arm-smmu: add support for specifying clocks
On Wed, Aug 13, 2014 at 01:51:34AM +0100, Mitchel Humpherys wrote:
On some platforms with tight power constraints it is polite to only
leave your clocks on for as long as you absolutely need them. Currently
we assume that all clocks necessary for SMMU register access are always
on.
Add some optional device tree properties to specify any clocks that are
necessary for SMMU register access and turn them on and off as needed.
If no clocks are specified in the device tree things continue to work
the way they always have: we assume all necessary clocks are always
turned on.
How does this interact with an SMMU in bypass mode?
[...]
+static int arm_smmu_enable_clocks(struct arm_smmu_device *smmu)
+{
+ int i, ret = 0;
+
+ for (i = 0; i smmu-num_clocks; ++i) {
+ ret = clk_prepare_enable(smmu-clocks[i]);
+ if (ret) {
+ dev_err(smmu-dev, Couldn't enable clock #%d\n, i);
+ while (i--)
+ clk_disable_unprepare(smmu-clocks[i]);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static void arm_smmu_disable_clocks(struct arm_smmu_device *smmu)
+{
+ int i;
+
+ for (i = 0; i smmu-num_clocks; ++i)
+ clk_disable_unprepare(smmu-clocks[i]);
+}
What stops theses from racing with each other when there are multiple
clocks? I also assume that the clk API ignores calls to clk_enable_prepare
for a clk that's already enabled? I couldn't find that code...
/* Wait for any pending TLB invalidations to complete */
static void arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
{
@@ -644,11 +672,15 @@ static irqreturn_t arm_smmu_context_fault(int irq, void
*dev)
struct arm_smmu_device *smmu = smmu_domain-smmu;
void __iomem *cb_base;
+ arm_smmu_enable_clocks(smmu);
How can I get a context interrupt from an SMMU without its clocks enabled?
[...]
+int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
{
unsigned long size;
void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
@@ -2027,10 +2124,16 @@ static int arm_smmu_device_dt_probe(struct
platform_device *pdev)
}
dev_notice(dev, registered %d master devices\n, i);
- err = arm_smmu_device_cfg_probe(smmu);
+ err = arm_smmu_init_clocks(smmu);
if (err)
goto out_put_masters;
+ arm_smmu_enable_clocks(smmu);
+
+ err = arm_smmu_device_cfg_probe(smmu);
+ if (err)
+ goto out_disable_clocks;
+
parse_driver_options(smmu);
if (smmu-version 1
@@ -2039,7 +2142,7 @@ static int arm_smmu_device_dt_probe(struct
platform_device *pdev)
found only %d context interrupt(s) but %d required\n,
smmu-num_context_irqs, smmu-num_context_banks);
err = -ENODEV;
- goto out_put_masters;
+ goto out_disable_clocks;
}
for (i = 0; i smmu-num_global_irqs; ++i) {
@@ -2061,12 +2164,16 @@ static int arm_smmu_device_dt_probe(struct
platform_device *pdev)
spin_unlock(arm_smmu_devices_lock);
arm_smmu_device_reset(smmu);
+ arm_smmu_disable_clocks(smmu);
I wonder if this is really the right thing to do. Rather than the
fine-grained clock enable/disable you have, why don't we just enable in
domain_init and disable in domain_destroy, with refcounting for the clocks?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/6] iommu/arm-smmu: add support for specifying regulators
On Wed, Aug 13, 2014 at 01:51:35AM +0100, Mitchel Humpherys wrote: On some power-constrained platforms it's useful to disable power when a device is not in use. Add support for specifying regulators for SMMUs and only leave power on as long as the SMMU is in use (attached). ... and for bypass mode? My comments for clks largely apply here too -- I'd much rather see this in domain_init/domain_destroy with appropriate refcounting. It's just too much of a mess having these calls littered around the driver. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/6] iommu/arm-smmu: implement generic DT bindings
Will Deacon will.dea...@arm.com writes:
[adding Rob, Mark, Arnd, Thierry and Hiroshi]
On Wed, Aug 13, 2014 at 01:51:37AM +0100, Mitchel Humpherys wrote:
Generic IOMMU device tree bindings were recently added in
[devicetree: Add generic IOMMU device tree bindings]. Implement the
bindings in the ARM SMMU driver.
See Documentation/devicetree/bindings/iommu/iommu.txt for the bindings
themselves.
Could you look at moving the parsing code into a separate file please (maybe
under lib/ ?). That way, other IOMMUs can use the same binding without the
boilerplate having to be rewritten each time.
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 63c6707fad..22e25f3172 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -538,25 +538,32 @@ static int insert_smmu_master(struct arm_smmu_device
*smmu,
return 0;
}
+struct iommus_entry {
+struct list_head list;
+struct device_node *node;
+u16 streamids[MAX_MASTER_STREAMIDS];
+int num_sids;
+};
+
static int register_smmu_master(struct arm_smmu_device *smmu,
-struct device *dev,
-struct of_phandle_args *masterspec)
+struct iommus_entry *entry)
{
int i;
struct arm_smmu_master *master;
+struct device *dev = smmu-dev;
-master = find_smmu_master(smmu, masterspec-np);
+master = find_smmu_master(smmu, entry-node);
if (master) {
dev_err(dev,
rejecting multiple registrations for master device
%s\n,
-masterspec-np-name);
+entry-node-name);
return -EBUSY;
}
-if (masterspec-args_count MAX_MASTER_STREAMIDS) {
+if (entry-num_sids MAX_MASTER_STREAMIDS) {
dev_err(dev,
reached maximum number (%d) of stream IDs for master
device %s\n,
-MAX_MASTER_STREAMIDS, masterspec-np-name);
+MAX_MASTER_STREAMIDS, entry-node-name);
return -ENOSPC;
}
@@ -564,15 +571,58 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
if (!master)
return -ENOMEM;
-master-of_node = masterspec-np;
-master-cfg.num_streamids = masterspec-args_count;
+master-of_node = entry-node;
+master-cfg.num_streamids = entry-num_sids;
for (i = 0; i master-cfg.num_streamids; ++i)
-master-cfg.streamids[i] = masterspec-args[i];
+master-cfg.streamids[i] = entry-streamids[i];
return insert_smmu_master(smmu, master);
}
+static int arm_smmu_parse_iommus_properties(struct arm_smmu_device *smmu,
+int *num_masters)
+{
+struct of_phandle_args iommuspec;
+struct device_node *dn;
+
+for_each_node_with_property(dn, iommus) {
+int arg_ind = 0;
+struct iommus_entry *entry, *n;
+LIST_HEAD(iommus);
You may want to use the macro, of_property_for_each_phandle_with_args()
to parse iommus=.
https://patchwork.ozlabs.org/patch/354073/
+
+while (!of_parse_phandle_with_args(dn, iommus, #iommu-cells,
+arg_ind, iommuspec)) {
We need to check that the phandle does indeed point at one of our SMMUs
here, in case we have a system with multiple IOMMU types, all using the
generic binding.
+int i;
+
+list_for_each_entry(entry, iommus, list)
+if (entry-node == dn)
+break;
Oh, yuck, this is really nasty to parse...
+if (entry-list == iommus) {
Where is entry initialised the first time around?
+entry = devm_kzalloc(smmu-dev, sizeof(*entry),
+GFP_KERNEL);
+if (!entry)
+return -ENOMEM;
You need to of_node_put the guys you've got back from the phandle parsing
code.
+entry-node = dn;
+list_add(entry-list, iommus);
+}
+entry-num_sids = iommuspec.args_count;
+for (i = 0; i entry-num_sids; ++i)
+entry-streamids[i] = iommuspec.args[i];
+arg_ind++;
Isn't this defined by #iommu-cells?
+}
+
+list_for_each_entry_safe(entry, n, iommus, list) {
Why the _safe variant? This is a local list, right?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions
On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap. But sg-offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s-offset + s-length' you use it the same way). So when you pass iova + offset the result will no longer be page-aligned. You should force sg-offset == 0 and sg-length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API. Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg). Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too. Do we have a use case where the unmap_sg() implementation would be different than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() completely. I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions). Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here. I suggested doing a if (ops is not NULL) return ops(); else return default_ops(); to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm. -- Regards, Laurent Pinchart ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/6] iommu/arm-smmu: add support for iova_to_phys through ATS1PR
On Tue, Aug 19 2014 at 05:44:32 AM, Will Deacon will.dea...@arm.com wrote:
On Wed, Aug 13, 2014 at 01:51:36AM +0100, Mitchel Humpherys wrote:
Currently, we provide the iommu_ops.iova_to_phys service by doing a
table walk in software to translate IO virtual addresses to physical
addresses. On SMMUs that support it, it can be useful to ask the SMMU
itself to do the translation. This can be used to warm the TLBs for an
SMMU. It can also be useful for testing and hardware validation.
I'm not really sold on the usefulness of this feature. If you want hardware
validation features, I'd rather do something through debugfs, but your
use-case for warming the TLBs is intriguing. Do you have an example use-case
with performance figures?
I'm afraid I don't have an example use case or performance numbers at
the moment...
Since the address translation registers are optional on SMMUv2, only
enable hardware translations when using SMMUv1 or when SMMU_IDR0.S1TS=1
and SMMU_IDR0.ATOSNS=0, as described in the ARM SMMU v1-v2 spec.
[...]
+static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
+dma_addr_t iova)
+{
+struct arm_smmu_domain *smmu_domain = domain-priv;
+struct arm_smmu_device *smmu = smmu_domain-smmu;
+struct arm_smmu_cfg *cfg = smmu_domain-cfg;
+struct device *dev = smmu-dev;
+void __iomem *cb_base;
+int count = 0;
+u64 phys;
+
+arm_smmu_enable_clocks(smmu);
+
+cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg-cbndx);
+
+if (smmu-version == 1) {
+u32 reg = iova 0xF000;
+writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
+} else {
+u64 reg = iova 0xf000;
+writeq_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
We don't have writeq for arch/arm/.
Ah yes looks like this is an MSM-ism that never made it upstream since
it wouldn't be guaranteed to be atomic. I'll make sure to do arm32
compiles on upstream kernels for future patches, sorry!
I guess we could use asm-generic/io-64-nonatomic-lo-hi.h but I can
also re-work this to be two separate writel's.
+}
+
+mb();
Why?
My thought was that if we start polling ATSR_ACTIVE prematurely (before
the write to ATS1PR actually finishes) all heck could break loose? Not
sure if that's a bogus assumption due to device memory being strongly
ordered?
+while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) ATSR_ACTIVE) {
+if (++count == ATSR_LOOP_TIMEOUT) {
+dev_err(dev,
+iova to phys timed out on 0x%pa for %s.
Falling back to software table walk.\n,
+iova, dev_name(dev));
+arm_smmu_disable_clocks(smmu);
+return arm_smmu_iova_to_phys_soft(domain, iova);
+}
+cpu_relax();
+}
Do you know what happened to Olav's patches to make this sort of code
generic?
I assume you're talking about this, right?
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267943.html
Yeah looks like he never sent an update since it was part of a series
that wasn't going to make it in (the qsmmu driver). I can always bring
that patch (actually Matt Wagantall's patch) in here and rework this to
use that.
@@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device
*smmu)
return -ENODEV;
}
+if (smmu-version == 1 || (!(id ID0_ATOSNS) (id ID0_S1TS))) {
Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS
is also clear.
I was looking at Section 4.1.1 of ARM IHI 0062C ID091613 which states:
In SMMUv2, the address translation registers are OPTIONAL. The
address translation registers are implemented only when both:
o The SMMU_IDR0.S1TS bit is set to 1.
o The SMMU_IDR0.ATOSNS bit is set to 0.
I assume you're referring to section 9.6.1 of the same document:
ATOSNS, bit[26]
Address Translation Operations Not Supported. The possible values of
this bit are:
0 Address translation operations are supported. Stage 1
translation is not supported, that is, the S1TS bit is set to 0.
1 Address translation operations are not supported. Stage 1
translation is supported, that is, the S1TS bit is set to 1.
If that really means that S1TS and ATOSNS always have the same value
then Section 4.1.1 doesn't make any sense. Or am I missing something?
-Mitch
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions
On 8/19/2014 4:59 AM, Joerg Roedel wrote: On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap. But sg-offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s-offset + s-length' you use it the same way). So when you pass iova + offset the result will no longer be page-aligned. You should force sg-offset == 0 and sg-length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API. Ok, but I don't think we want to force a specific alignment here (as I discussed earlier with Will D.). So I would just do something like iommu_map(domain, iova + offset, phys, s-length, prot); offset += s-length; Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg). Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too. So are you looking for something like this then: int iommu_map_sg(struct iommu_domain *domain, unsigned long iova, struct scatterlist *sg, unsigned int nents, int prot, unsigned long flags) int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova, struct scatterlist *sg, unsigned int nents, unsigned long flags) This makes unmapping code more complicated (and slow) though. For example the way I would implement the fallback would be to iterate through the sglist first to calculate the total length to unmap and then call iommu_unmap(). I know we talked about removing the flag argument in the map call. However, the TLB_NO_INV usage is actually needed for both map and unmap. We have HW that requires TLB invalidate on MAP and TLB inv. is also required if you implement your mapping function to support replacing an existing mapping... Rob, did you have an issue with this API before or was it just an issue with not having the iova in the unmap call? I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions). Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here. Joerg ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions
On 8/19/2014 9:11 AM, Laurent Pinchart wrote: On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap. But sg-offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s-offset + s-length' you use it the same way). So when you pass iova + offset the result will no longer be page-aligned. You should force sg-offset == 0 and sg-length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API. Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg). Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too. Do we have a use case where the unmap_sg() implementation would be different than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() completely. I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions). Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here. I suggested doing a if (ops is not NULL) return ops(); else return default_ops(); to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm. Both Thierry R. and Konrad W. argued for modifying the drivers instead so I implemented what the majority wanted. :-) Olav -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/6] iommu/arm-smmu: add support for specifying clocks
Hi Will,
On 8/19/2014 5:58 AM, Will Deacon wrote:
On Wed, Aug 13, 2014 at 01:51:34AM +0100, Mitchel Humpherys wrote:
On some platforms with tight power constraints it is polite to only
leave your clocks on for as long as you absolutely need them. Currently
we assume that all clocks necessary for SMMU register access are always
on.
Add some optional device tree properties to specify any clocks that are
necessary for SMMU register access and turn them on and off as needed.
If no clocks are specified in the device tree things continue to work
the way they always have: we assume all necessary clocks are always
turned on.
How does this interact with an SMMU in bypass mode?
Do you mean if you have a platform that requires clock and power
management but we leave the SMMU in bypass (i.e. no one calls into the
SMMU driver) how are the clock/power managed?
Clients of the SMMU driver are required to vote for clocks and power
when they know they need to use the SMMU. However, the clock and power
needed to be on for the SMMU to service bus masters aren't necessarily
the same as the ones needed to read/write registers...See below.
[...]
+static int arm_smmu_enable_clocks(struct arm_smmu_device *smmu)
+{
+int i, ret = 0;
+
+for (i = 0; i smmu-num_clocks; ++i) {
+ret = clk_prepare_enable(smmu-clocks[i]);
+if (ret) {
+dev_err(smmu-dev, Couldn't enable clock #%d\n, i);
+while (i--)
+clk_disable_unprepare(smmu-clocks[i]);
+break;
+}
+}
+
+return ret;
+}
+
+static void arm_smmu_disable_clocks(struct arm_smmu_device *smmu)
+{
+int i;
+
+for (i = 0; i smmu-num_clocks; ++i)
+clk_disable_unprepare(smmu-clocks[i]);
+}
What stops theses from racing with each other when there are multiple
clocks? I also assume that the clk API ignores calls to clk_enable_prepare
for a clk that's already enabled? I couldn't find that code...
All the clock APIs are reference counted yes. Not sure what you mean by
racing with each other? When you call to enable a clock the call does
not return until the clock is already ON (or OFF).
/* Wait for any pending TLB invalidations to complete */
static void arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
{
@@ -644,11 +672,15 @@ static irqreturn_t arm_smmu_context_fault(int irq,
void *dev)
struct arm_smmu_device *smmu = smmu_domain-smmu;
void __iomem *cb_base;
+arm_smmu_enable_clocks(smmu);
How can I get a context interrupt from an SMMU without its clocks enabled?
Good question. At least in our SoC we usually have at least 1 core
clock and 1 programming clock. Both clocks are needed to read/write
registers but only 1 of the clocks is needed for the SMMU to service bus
master requests.
[...]
+int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
{
unsigned long size;
void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
@@ -2027,10 +2124,16 @@ static int arm_smmu_device_dt_probe(struct
platform_device *pdev)
}
dev_notice(dev, registered %d master devices\n, i);
-err = arm_smmu_device_cfg_probe(smmu);
+err = arm_smmu_init_clocks(smmu);
if (err)
goto out_put_masters;
+arm_smmu_enable_clocks(smmu);
+
+err = arm_smmu_device_cfg_probe(smmu);
+if (err)
+goto out_disable_clocks;
+
parse_driver_options(smmu);
if (smmu-version 1
@@ -2039,7 +2142,7 @@ static int arm_smmu_device_dt_probe(struct
platform_device *pdev)
found only %d context interrupt(s) but %d required\n,
smmu-num_context_irqs, smmu-num_context_banks);
err = -ENODEV;
-goto out_put_masters;
+goto out_disable_clocks;
}
for (i = 0; i smmu-num_global_irqs; ++i) {
@@ -2061,12 +2164,16 @@ static int arm_smmu_device_dt_probe(struct
platform_device *pdev)
spin_unlock(arm_smmu_devices_lock);
arm_smmu_device_reset(smmu);
+arm_smmu_disable_clocks(smmu);
I wonder if this is really the right thing to do. Rather than the
fine-grained clock enable/disable you have, why don't we just enable in
domain_init and disable in domain_destroy, with refcounting for the clocks?
So the whole point of all of this is that we try to save power. As Mitch
wrote in the commit text we want to only leave the clock and power on
for as short period of time as possible.
Thanks,
Olav
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 6/6] iommu/arm-smmu: add .domain_{set, get}_attr for coherent walk control
On Tue, Aug 19 2014 at 05:48:07 AM, Will Deacon will.dea...@arm.com wrote:
On Wed, Aug 13, 2014 at 01:51:39AM +0100, Mitchel Humpherys wrote:
Under certain conditions coherent hardware translation table walks can
result in degraded performance. Add a new domain attribute to
disable/enable this feature in generic code along with the domain
attribute setter and getter to handle it in the ARM SMMU driver.
Again, it would be nice to have some information about these cases and the
performance issues that you are seeing.
Basically, the data I'm basing these statements on is: that's what the
HW folks tell me :). I believe it's specific to our hardware, not ARM
IP. Unfortunately, I don't think I could share the specifics even if I
had them, but I can try to press the issue if you want me to.
@@ -1908,11 +1917,15 @@ static int arm_smmu_domain_get_attr(struct
iommu_domain *domain,
enum iommu_attr attr, void *data)
{
struct arm_smmu_domain *smmu_domain = domain-priv;
+struct arm_smmu_cfg *cfg = smmu_domain-cfg;
switch (attr) {
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain-stage == ARM_SMMU_DOMAIN_NESTED);
return 0;
+case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
+*((bool *)data) = cfg-htw_disable;
+return 0;
I think I'd be more comfortable using int instead of bool for this, as it
could well end up in the user ABI if vfio decides to make use of it. While
we're here, let's also add an attributes bitmap to the arm_smmu_domain
instead of having a bool in the arm_smmu_cfg.
Sounds good. I'll make these changes in v2.
default:
return -ENODEV;
}
@@ -1922,6 +1935,7 @@ static int arm_smmu_domain_set_attr(struct
iommu_domain *domain,
enum iommu_attr attr, void *data)
{
struct arm_smmu_domain *smmu_domain = domain-priv;
+struct arm_smmu_cfg *cfg = smmu_domain-cfg;
switch (attr) {
case DOMAIN_ATTR_NESTING:
@@ -1933,6 +1947,9 @@ static int arm_smmu_domain_set_attr(struct
iommu_domain *domain,
smmu_domain-stage = ARM_SMMU_DOMAIN_S1;
return 0;
+case DOMAIN_ATTR_COHERENT_HTW_DISABLE:
+cfg-htw_disable = *((bool *)data);
+return 0;
default:
return -ENODEV;
}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0550286df4..8a6449857a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -81,6 +81,7 @@ enum iommu_attr {
DOMAIN_ATTR_FSL_PAMU_ENABLE,
DOMAIN_ATTR_FSL_PAMUV1,
DOMAIN_ATTR_NESTING,/* two stages of translation */
+DOMAIN_ATTR_COHERENT_HTW_DISABLE,
I wonder whether we should make this ARM-specific. Can you take a quick look
to see if any of the other IOMMUs can potentially benefit from this?
Yeah looks like amd_iommu.c and intel-iommu.c are using
IOMMU_CAP_CACHE_COHERENCY which seems to be the same thing (at least
that's how we're treating it in arm-smmu.c). AMD's doesn't look
configurable but Intel's does, so perhaps they would benefit from this.
-Mitch
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/6] iommu/arm-smmu: add support for specifying clocks
On Tue, Aug 19 2014 at 05:58:34 AM, Will Deacon will.dea...@arm.com wrote: I also assume that the clk API ignores calls to clk_enable_prepare for a clk that's already enabled? I couldn't find that code... That's clk_prepare_enable, not clk_enable_prepare. It's in linux/clk.h. -Mitch -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions
On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote: On 8/19/2014 9:11 AM, Laurent Pinchart wrote: On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap. But sg-offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s-offset + s-length' you use it the same way). So when you pass iova + offset the result will no longer be page-aligned. You should force sg-offset == 0 and sg-length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API. Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg). Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too. Do we have a use case where the unmap_sg() implementation would be different than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() completely. I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions). Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here. I suggested doing a if (ops is not NULL) return ops(); else return default_ops(); to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm. Both Thierry R. and Konrad W. argued for modifying the drivers instead so I implemented what the majority wanted. :-) I'm not blaming you :-) I was just wondering what their rationale was. -- Regards, Laurent Pinchart ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/29] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem
Hi, Am 19.08.2014 14:01, schrieb Marek Szyprowski: On 2014-08-19 13:39, Andreas Färber wrote: I'm working on 5250 based Spring Chromebook and noticed that v3.17-rc1 got some more iommu support. With the new CONFIG_DRM_EXYNOS_IOMMU=y my machine stops booting. So I'm wondering, is any of this a fix for 3.17, or is all of this unrelated -next material? This is probably a side effect of patch 3170447c1f264d51b8d1f3898bf2588588a64fdc (iommu/exynos: Select ARM_DMA_USE_IOMMU). It added selection of ARM_DMA_USE_IOMMU symbol, on which IOMMU support in Exynos DRM subsystem depends. However selecting this symbol is all that this patch does, without providing any code code which implements real support for ARM DMA IOMMU integration, which is needed by Exynos DRM driver. Please disable CONFIG_DRM_EXYNOS_IOMMU in kernel .config and your system should be bootable again. Yes, that's what my report implied. :) I'm bringing this up for -rc2 though: It sounds as if that option should remain unavailable until the required code is in? Thanks. Also, are you or someone working on the respective DT changes for Exynos5? I can prepare DT changes for Exynos5 as well, but first I wanted to clarify if everyone involved in generic IOMMU bindings and Exynos IOMMU driver agrees on my proposal. Sure. I'm updating my old spring-bridge.v6 branch [1] to 3.17-rc1 [2], which involves three drm bridge patches rebased onto the new drm panel prepare/unprepare infrastructure plus two LVDS DT patches in addition to the IOMMU patches. That old branch included DT changes for 5250 in ARM: dts: add System MMU nodes of Exynos SoCs, but that'll probably need updating for the new bindings. Regards, Andreas [1] https://github.com/afaerber/linux/commits/spring-bridge.v6 [2] https://github.com/afaerber/linux/commits/spring-next -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/29] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem
Hi Marek and Inki, Am 19.08.2014 08:07, schrieb Marek Szyprowski: On 2014-08-19 01:32, Joerg Roedel wrote: On Tue, Aug 05, 2014 at 12:47:28PM +0200, Marek Szyprowski wrote: [...] 33 files changed, 1016 insertions(+), 356 deletions(-) This touches a lot of non-iommu stuff. What is your strategy on getting this in, do you plan to get the non-iommu changes merged first or do you want to collect the respective Acks and merge this all through one tree? Those patches are posted as one patchset mainly to demonstrate how to get everything to work together. I also posted this as a single patch series to get some feedback from other iommu developers, especially all those involved in the generic iommu dt bindings. For merging, I will split them into smaller series and try to get respective acks. I'm working on 5250 based Spring Chromebook and noticed that v3.17-rc1 got some more iommu support. With the new CONFIG_DRM_EXYNOS_IOMMU=y my machine stops booting. So I'm wondering, is any of this a fix for 3.17, or is all of this unrelated -next material? Also, are you or someone working on the respective DT changes for Exynos5? Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/4] VFIO: PLATFORM: Return device tree info for a platform device node
This RFC's intention is to show what an interface to access device node properties for VFIO_PLATFORM can look like. If a device tree node corresponding to a platform device bound by VFIO_PLATFORM is available, this patch series will allow the user to query the properties associated with this device node. This can be useful for userspace drivers to automatically query parameters related to the device. An API to return data from a device's device tree has been proposed before on these lists. The API proposed here is slightly different. Properties to parse from the device tree are not indexed by a numerical id. The host system doesn't guarantee any specific ordering for the available properties, or that those will remain the same; while this does not happen in practice, there is nothing from the host changing the device nodes during operation. So properties are accessed by property name. The type of the property accessed must also be known by the user. Properties types implemented in this RFC: - VFIO_DEVTREE_ARR_TYPE_STRING (strings sepparated by the null character) - VFIO_DEVTREE_ARR_TYPE_U32 - VFIO_DEVTREE_ARR_TYPE_U16 - VFIO_DEVTREE_ARR_TYPE_U8 These can all be access by the ioctl VFIO_DEVICE_GET_DEVTREE_INFO. A new ioctl was preferred instead of shoehorning the functionality in VFIO_DEVICE_GET_INFO. The structure exchanged looks like this: You'll have to forgive my ignorance on the history. But with the dtc tool already supporting a filesystem representation (--in-format=fs), with the dtc tool source already built into qemu, and having an example implementation of such an interface in /proc/device-tree/ why is an ioctl interface preferred over a filesystem interface? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/6] iommu/arm-smmu: implement generic DT bindings
On Tuesday 19 August 2014, Will Deacon wrote:
+static int arm_smmu_parse_iommus_properties(struct arm_smmu_device *smmu,
+ int *num_masters)
+{
+ struct of_phandle_args iommuspec;
+ struct device_node *dn;
+
+ for_each_node_with_property(dn, iommus) {
+ int arg_ind = 0;
+ struct iommus_entry *entry, *n;
+ LIST_HEAD(iommus);
+
+ while (!of_parse_phandle_with_args(dn, iommus,
#iommu-cells,
+ arg_ind, iommuspec))
{
We need to check that the phandle does indeed point at one of our SMMUs
here, in case we have a system with multiple IOMMU types, all using the
generic binding.
Why does the iommu driver do this at all? The of_parse_phandle_with_args()
call should really be part of the iommu common code when we probe the devices in
of_dma_configure() as far as I can tell.
The way I would have expected it to happen is to have some code that
ensures the iommu drivers are instantiated before we do call
of_platform_populate,
and then of_dma_configure() looks for whether there is an iommu property or not
and calls into the respective iommu driver for doing the setup if there is.
Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] powerpc: fsl_pamu_domain: Fix build error
Fix build failure in fsl_pamu_domain.o caused as follows drivers/iommu/fsl_pamu_domain.c: In function 'pamu_domain_init': drivers/iommu/fsl_pamu_domain.c:1103:17: error: 'pci_bus_type' undeclared (first use in this function) drivers/iommu/fsl_pamu_domain.c:1103:17: note: each undeclared identifier is reported only once for each function it appears in make[2]: *** [drivers/iommu/fsl_pamu_domain.o] Error 1 make[1]: *** [drivers/iommu] Error 2 make: *** [drivers] Error 2 We fix this by trying to dereference pci_bus_type only if CONFIG_PCI is defined. Signed-off-by: Pranith Kumar bobby.pr...@gmail.com --- drivers/iommu/fsl_pamu_domain.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c index 61d1daf..bb56f86 100644 --- a/drivers/iommu/fsl_pamu_domain.c +++ b/drivers/iommu/fsl_pamu_domain.c @@ -1099,7 +1099,9 @@ int pamu_domain_init(void) return ret; bus_set_iommu(platform_bus_type, fsl_pamu_ops); +#ifdef CONFIG_PCI bus_set_iommu(pci_bus_type, fsl_pamu_ops); +#endif return ret; } -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions
On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote: On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote: On 8/19/2014 9:11 AM, Laurent Pinchart wrote: On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote: On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote: If the alignment is not correct then iommu_map() will return error. Not sure what other option we have here (and why make it different behavior than iommu_map which just return error when it is not aligned properly). I don't think we want to force any kind of alignment automatically. I would rather have the API tell me I am doing something wrong than having the function aligning the values and possibly undermap or overmap. But sg-offset is an offset into the page (at least it is used that way in the DMA-API and since you do 'page_len = s-offset + s-length' you use it the same way). So when you pass iova + offset the result will no longer be page-aligned. You should force sg-offset == 0 and sg-length to be page-aligned instead. This makes more sense because the IOMMU-API works on (io)-page granularity and not on arbitrary phys-addr ranges like the DMA-API. Yes, I am aware of that. However, several people prefer this than passing in scatterlist. It is not very convenient to pass a scatterlist in some use cases. Someone mentioned a use case where they would have to create a dummy sg list and populate it with the iova just to do an unmap. I believe we would have to do this also. There is no use for sglist when unmapping. However, would like to keep separate API from iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg). Keeping it symetric is not more complicated, the caller just needs to keep the sg-list used for mapping around. I prefer the unmap_sg call to work in sg-lists too. Do we have a use case where the unmap_sg() implementation would be different than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() completely. I thought that was why we added the default fallback and set all the drivers to point to these fallback functions. Several people wanted this so that we don't have to have NULL-check in these functions (and have the functions be simple inline functions). Okay, since you add these call-backs to all drivers I think I can live with not doing a pointer check here. I suggested doing a if (ops is not NULL) return ops(); else return default_ops(); to avoid modifying all drivers. I'm not sure why that wasn't received with much enthusiasm. Both Thierry R. and Konrad W. argued for modifying the drivers instead so I implemented what the majority wanted. :-) I'm not blaming you :-) I was just wondering what their rationale was. In my opinion it's much more direct that way. It means that if a driver doesn't implement it, it won't fall back to some default implementation instead. Providing an explicit helper like this makes it obvious that the driver is using a default implementation rather than making things work magically. It's easier to see in the driver that there's the potential to optimize. It also has the side-effect of keeping the core code cleaner in my opinion, since the core iommu_map_sg() and iommu_unmap_sg() functions can now blindly call into drivers directly rather than performing the various checks to see if they implement the required functionality. Thierry pgpegQTLTiKiv.pgp Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
