Unified SMMU driver among Tegra SoCs
Subsystems (9)
v2 cover Unified SMMU driver among Tegra SoCs
Hi,
This series provides:
(1) Unified SMMU driver among Tegra SoCs
(2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
(3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.
There's some discussion[1] about device population order, which can solve the following patches.
[HACK] of: dev_node has struct device pointer
ARM: tegra: Populate AHB/IOMMU earlier than others
Also "ARM: dma-mapping: Drop GFP_COMP for DMA memory allocations" may not be necessary by [2]
Tested IOMMU functionality with T30 SD/MMC. Any further testing with T114 and/or other devices would be really appreciated.
v2:
Updated based on Thierry Reding's and Stephen Warren's feedback
v1:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/180267.html
Hiroshi Doyu (22):
[HACK] of: dev_node has struct device pointer
ARM: tegra: Populate AHB/IOMMU earlier than others
ARM: tegra: Create a DT header defining swgroups ID
ARM: dt: tegra30: iommu: Add "nvidia,swgroup"
ARM: dt: tegra30: iommu: Add "nvidia,memory-client"
ARM: dt: tegra114: iommu: Fix IOMMU register address
ARM: dt: tegra114: iommu: Add "nvidia,swgroups"
ARM: dt: tegra114: Add "nvidia,memory-client"
amba: Move AHB to core_initcall
iommu/tegra: smmu: Move IOMMU to core_initcall iommu/tegra: smmu: Add Tegra 114 support
iommu/tegra: smmu: Select ARM_DMA_USE_IOMMU in Kconfig iommu/tegra: smmu: Create default IOVA maps iommu/tegra: smmu: Register platform_device to IOMMU dynamically iommu/tegra: smmu: Calculate ASID register offset by ID iommu/tegra: smmu: Get "nvidia,swgroup" from DT iommu/tegra: smmu: Unfied driver for Tegra SoCs iommu/tegra: smmu: Use dt-bindings MACRO
iommu/tegra: smmu: Workaround PCIe IOMMU'able iommu/tegra: smmu: Get "nvidia,memory-client" from DT iommu/tegra: smmu: Support Multiple ASID
ARM: dma-mapping: Drop GFP_COMP for DMA memory allocations
.../bindings/iommu/nvidia,tegra30-smmu.txt | 20 +- arch/arm/boot/dts/tegra114.dtsi | 13 +- arch/arm/boot/dts/tegra30.dtsi | 21 ++ arch/arm/mach-tegra/tegra.c | 22 ++ arch/arm/mm/dma-mapping.c | 7 + drivers/amba/tegra-ahb.c | 7 +- drivers/iommu/Kconfig | 1 + drivers/iommu/tegra-smmu.c | 211 +++++++++++--------- drivers/of/base.c | 23 +++ drivers/of/platform.c | 8 + include/dt-bindings/memory/tegra-swgroup.h | 50 +++++ include/linux/of.h | 16 ++ 12 files changed, 296 insertions(+), 103 deletions(-) create mode 100644 include/dt-bindings/memory/tegra-swgroup.h
Signature · 4 lines
1.7.9.5 [1] https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/thread.html#36542 [2] http://marc.info/?l=linux-tegra&m=137173895620957&w=1
PATCH v2 1/22 of: dev_node has struct device pointer
To prevent of_platform_populate() from trying to populate duplicate devices if a device has been already populated.
---
Need to take care of early_platform_devices, or alternative solution.
--- drivers/of/base.c | 23 +++++++++++++++++++++++ drivers/of/platform.c | 8 ++++++++ include/linux/of.h | 16 ++++++++++++++++ 3 files changed, 47 insertions(+)
drivers/of/base.c +23 -0
@@ -230,6 +230,29 @@ const void *of_get_property(const struct device_node *np, const char *name, }EXPORT_SYMBOL (of_get_property); +struct device *of_get_device (const struct device_node *node) +{ +struct device *dev; +unsigned long flags; + +raw_spin_lock_irqsave (&devtree_lock, flags); + dev = node->dev; +raw_spin_unlock_irqrestore (&devtree_lock, flags); + +return dev; +} +EXPORT_SYMBOL (of_get_device); + +void of_set_device (struct device_node *node,struct device *dev) +{ +unsigned long flags; + +raw_spin_lock_irqsave (&devtree_lock, flags); + node->dev = dev; +raw_spin_unlock_irqrestore (&devtree_lock, flags); +} +EXPORT_SYMBOL (of_set_device); +/** Checks if the given "compat" string matches one of the strings in * the device's "compatible" property */
drivers/of/platform.c +8 -0
@@ -203,10 +203,17 @@ struct platform_device *of_platform_device_create_pdata( struct device *parent) {struct platform_device *dev; +struct device *tmp;if (!of_device_is_available (np))return NULL ; + tmp =of_get_device (np); +if (tmp) { +dev_info (tmp,"Already populated\n" ); +return to_platform_device (tmp); + } + dev =of_device_alloc (np, bus_id, parent);if (!dev)return NULL ;
@@ -228,6 +235,7 @@ struct platform_device *of_platform_device_create_pdata(return NULL ; } +of_set_device (np, &dev->dev);return dev; }
include/linux/of.h +16 -0
@@ -60,6 +60,9 @@ struct device_node {struct kref kref;unsigned long _flags;void *data; + +struct device *dev;/* Set only after populated */ +#if defined(CONFIG_SPARC )const char *path_component_name;unsigned int unique_id;
@@ -268,6 +271,8 @@ extern const void *of_get_property(const struct device_node *node, int *lenp);#define for_each_property_of_node (dn, pp) \ for (pp = dn->properties; pp != NULL; pp = pp->next) +extern struct device *of_get_device (const struct device_node *node); +extern void of_set_device (struct device_node *node,struct device *dev);extern int of_n_addr_cells (struct device_node *np);extern int of_n_size_cells (struct device_node *np);
@@ -459,6 +464,17 @@ static inline const void *of_get_property(const struct device_node *node,return NULL ; } +static inline struct device *of_get_device (const struct device_node *node) +{ +return NULL ; +} + +static inline void of_set_device (const struct device_node *node, +struct device *dev); +{ +return -ENOSYS ; +} +static inline int of_property_read_u64 (const struct device_node *np,const char *propname,u64 *out_value) {
Signature · 1 line
1.7.9.5
PATCH v2 1/22 of: dev_node has struct device pointer
Quoted 2 lines · Hiroshi Doyu
To prevent
of_platform_populate()from trying to populate duplicate devices if a device has been already populated.
You need to send drivers/of patches to the DT maintainer and devicetree-discuss mailing list.
Quoted 3 lines
Signed-off-by Hiroshi Doyu <[email protected]>---
Need to take care ofearly_platform_devices, or alternative solution.
I assume that's a TODO item... Are you planning on fleshing out this patch to address that issue?
drivers/of/base.c const void *of_get_property(const struct device_node *np, const char *name, · line 240
Quoted 10 lines
drivers/of/base.c +8
-0+struct device *of_get_device (const struct device_node *node) +{ +struct device *dev; +unsigned long flags; + +raw_spin_lock_irqsave (&devtree_lock, flags); + dev = node->dev; +raw_spin_unlock_irqrestore (&devtree_lock, flags);
The read (and write in of_set_device()) aren't atomic already? I guess perhaps they aren't necessarily. It sure seems like some higher-level locking is needed in of_platform_create_pdata() though, since there's quite a window there between the get() and set() calls. Or, is there already some locking in place above that, in which case, I 'm not sure why there's a need for this level of locking here...
drivers/of/platform.c struct platform_device *of_platform_device_create_pdata( · line 215
Quoted 7 lines
drivers/of/platform.c +5
-0+ tmp =of_get_device (np); +if (tmp) { +dev_info (tmp,"Already populated\n" ); +return to_platform_device (tmp); + }
Did you check all callers of this function to make sure that they also don't do any kind of double-processing?
include/linux/of.h struct device_node { · line 66
Quoted 10 lines
include/linux/of.h +3
-0@@ -60,6 +60,9 @@ struct device_node {struct kref kref;unsigned long _flags;void *data; + +struct device *dev;/* Set only after populated */ +#if defined(CONFIG_SPARC )
Are the extra blank lines needed?
PATCH v2 1/22 of: dev_node has struct device pointer
Quoted 7 lines · Stephen Warren
Quoted 2 lines · Hiroshi Doyu
To prevent
of_platform_populate()from trying to populate duplicate devices if a device has been already populated.You need to send
drivers/ofpatches to the DT maintainer and devicetree-discuss mailing list.
Quoted 6 lines
Quoted 3 lines
Signed-off-by Hiroshi Doyu <[email protected]>---
Need to take care ofearly_platform_devices, or alternative solution.I assume that's a TODO item... Are you planning on fleshing out this patch to address that issue?
There was some more discussion about it, which can be found in the following thread:
https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036934.html
That's one of the last messages and it should have most of the background. If not you may have to walk up the thread for more context. In a nutshell I raised the question as to why we can't simply call of_platform_populate() earlier and side-step all the workarounds that have found their way into the kernel to side-step the issue of their not being a struct device associated with the struct device_node.
If of_platform_populate() can be made to run early, then we can restore a whole lot of consistency in how "early" drivers on an OF kernel work.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130716/db0a8b7f/attachment.sig>
PATCH v2 1/22 of: dev_node has struct device pointer
Thierry Reding <[email protected]> wrote @ Wed, 17 Jul 2013 01:16:30 +0200:
Quoted 20 lines
* PGP Signed by an unknown key
Quoted 7 lines · Stephen Warren
Quoted 2 lines · Hiroshi Doyu
To prevent
of_platform_populate()from trying to populate duplicate devices if a device has been already populated.You need to send
drivers/ofpatches to the DT maintainer and devicetree-discuss mailing list.Quoted 6 lines
Quoted 3 lines
Signed-off-by Hiroshi Doyu <[email protected]>---
Need to take care ofearly_platform_devices, or alternative solution.I assume that's a TODO item... Are you planning on fleshing out this patch to address that issue?
There was some more discussion about it, which can be found in the following thread:
Quoted 8 lines
https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036934.htmlThat's one of the last messages and it should have most of the background. If not you may have to walk up the thread for more context. In a nutshell I raised the question as to why we can't simply call
of_platform_populate()earlier and side-step all the workarounds that have found their way into the kernel to side-step the issue of their not being a struct device associated with the structdevice_node.
This is the thread which Lorenzo started originally. He may have some plan to implement/proceed with this, Lorenzo?
PATCH v2 1/22 of: dev_node has struct device pointer
Hi Hiroshi,
Quoted 31 lines · Hiroshi Doyu
Thierry Reding <[email protected]> wrote @ Wed, 17 Jul 2013 01:16:30 +0200:
Quoted 20 lines
* PGP Signed by an unknown key
Quoted 7 lines · Stephen Warren
Quoted 2 lines · Hiroshi Doyu
To prevent
of_platform_populate()from trying to populate duplicate devices if a device has been already populated.You need to send
drivers/ofpatches to the DT maintainer and devicetree-discuss mailing list.Quoted 6 lines
Quoted 3 lines
Signed-off-by Hiroshi Doyu <[email protected]>---
Need to take care ofearly_platform_devices, or alternative solution.I assume that's a TODO item... Are you planning on fleshing out this patch to address that issue?
There was some more discussion about it, which can be found in the following thread:
Quoted 8 lines
https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036934.htmlThat's one of the last messages and it should have most of the background. If not you may have to walk up the thread for more context. In a nutshell I raised the question as to why we can't simply call
of_platform_populate()earlier and side-step all the workarounds that have found their way into the kernel to side-step the issue of their not being a struct device associated with the structdevice_node.
Quoted 2 lines
This is the thread which Lorenzo started originally. He may have some plan to implement/proceed with this, Lorenzo?
I was trying to raise the issue and apparently I succeeded. For now, I put in place yet-another-temporary-solution to the problem in one of the drivers I need for power management, but certainly the problem is not solved, and I can't allocate time to do it myself unfortunately.
I am more than happy to review patches though.
Thanks,
Lorenzo
PATCH v2 2/22 ARM: tegra: Populate AHB/IOMMU earlier than others
In reply to Hiroshi DoyuPopulate AHB/IOMMU earlier than others. IOMMU depends on AHB. IOMMU needs to be instanciated earlier than others so that IOMMU can register other platform devices as IOMMU'able. Once IOMMU is populated, IOMMU/AHB nodes are detached to prevent another registeration.
--- arch/arm/mach-tegra/tegra.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
arch/arm/mach-tegra/tegra.c +22 -0
@@ -80,6 +80,26 @@ static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = { {} }; +static void tegra_of_platform_populate_iommu (void ) +{ +int i; +struct platform_device *pdev; +const char *const path[] = {"/ahb" ,"/iommu" , }; + +for (i =0 ; i <ARRAY_SIZE (path); i++) { +struct device_node *np; + + np =of_find_node_by_path (path[i]); +if (!np) +break ; + + pdev =of_platform_device_create (np,NULL ,NULL ); +of_node_put (np); +if (!pdev) +break ; + } +} +static void __init tegra_dt_init (void ) {struct soc_device_attribute *soc_dev_attr;
@@ -107,6 +127,8 @@ static void __init tegra_dt_init(void) parent =soc_device_to_device (soc_dev); +tegra_of_platform_populate_iommu (); +/* * Finished with the static registrations now; fill in the missing * devices
Signature · 1 line
1.7.9.5
PATCH v2 2/22 ARM: tegra: Populate AHB/IOMMU earlier than others
Quoted 5 lines · Hiroshi Doyu
Populate AHB/IOMMU earlier than others. IOMMU depends on AHB. IOMMU needs to be instanciated earlier than others so that IOMMU can register other platform devices as IOMMU'able. Once IOMMU is populated, IOMMU/AHB nodes are detached to prevent another registeration.
Hmmm. I'm still in two minds about this. If just the IOMMU was needed, then this wouldn't be too bad. But then there's the dependency on the AHB device too. And in general, there could be, say, a dependency on a SoC power-controller, clock controller, reset module, etc. This could become a huge slippery slope.
Modelling the IOMMU as an explicit bus with the bus-master-capable devices underneath it would make everything work automatically...
arch/arm/mach-tegra/tegra.c static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = { · line 92
Quoted 12 lines
arch/arm/mach-tegra/tegra.c +10
-0+static void tegra_of_platform_populate_iommu (void ) +{ +int i; +struct platform_device *pdev; +const char *const path[] = {"/ahb" ,"/iommu" , }; + +for (i =0 ; i <ARRAY_SIZE (path); i++) { +struct device_node *np; + + np =of_find_node_by_path (path[i]);
I think this should at least look up the nodes by compatible value. See how e.g. mach-tegra/pmc.c finds the right DT node.
PATCH v2 3/22 ARM: tegra: Create a DT header defining swgroups ID
In reply to Hiroshi DoyuCreate a header file to define the swgroup IDs used by the IOMMU(SMMU) binding. "swgroup" is a group of H/W clients in Tegra SoC from S/W POV.
This will allow the same header to be used by both device tree files, and drivers implementing this binding, which guarantees that the two stay in sync. This also makes device trees more readable by using names instead of magic numbers.
--- include/dt-bindings/memory/tegra-swgroup.h | 50 ++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 include/dt-bindings/memory/tegra-swgroup.h
include/dt-bindings/memory/tegra-swgroup.h new file +50 -0
@@ -0,0 +1,50 @@ +/* +* This header provides constants for binding nvidia,swgroup ID +*/ + +#ifndef _DT_BINDINGS_MEMORY_TEGRA_SWGROUP_H +#define _DT_BINDINGS_MEMORY_TEGRA_SWGROUP_H + +#define TEGRA_SWGROUP_AFI 0 +#define TEGRA_SWGROUP_AVPC 1 +#define TEGRA_SWGROUP_DC 2 +#define TEGRA_SWGROUP_DCB 3 +#define TEGRA_SWGROUP_EPP 4 +#define TEGRA_SWGROUP_G2 5 +#define TEGRA_SWGROUP_HC 6 +#define TEGRA_SWGROUP_HDA 7 +#define TEGRA_SWGROUP_ISP 8 +#define TEGRA_SWGROUP_ISP2 SWGID_ISP +/* UNUSED: 9 */ +/* UNUSED: 10 */ +#define TEGRA_SWGROUP_MPE 11 +#define TEGRA_SWGROUP_MSENC SWGID_MPE +#define TEGRA_SWGROUP_NV 12 +#define TEGRA_SWGROUP_NV2 13 +#define TEGRA_SWGROUP_PPCS 14 +#define TEGRA_SWGROUP_SATA2 15 +#define TEGRA_SWGROUP_SATA 16 +#define TEGRA_SWGROUP_VDE 17 +#define TEGRA_SWGROUP_VI 18 +#define TEGRA_SWGROUP_VIC 19 +#define TEGRA_SWGROUP_XUSB_HOST 20 +#define TEGRA_SWGROUP_XUSB_DEV 21 +#define TEGRA_SWGROUP_A9AVP 22 +#define TEGRA_SWGROUP_TSEC 23 +#define TEGRA_SWGROUP_PPCS1 24 +/* UNUSED: 25 */ +/* UNUSED: 26 */ +/* UNUSED: 27 */ +/* UNUSED: 28 */ +/* UNUSED: 29 */ +/* UNUSED: 30 */ +/* UNUSED: 31 */ + +/* Reserved: 32-63 */ + +#define TEGRA_SWGROUP_BIT (x) (1ULL << TEGRA_SWGROUP_##x) + +#define TEGRA30_SWGROUP_ALL <0x00000000 0x000779ff> +#define TEGRA114_SWGROUP_ALL <0x00000000 0x01b659fe> + +#endif /* _DT_BINDINGS_MEMORY_TEGRA_SWGROUP_H */
Signature · 1 line
1.7.9.5
PATCH v2 3/22 ARM: tegra: Create a DT header defining swgroups ID
Quoted 7 lines · Hiroshi Doyu
Create a header file to define the swgroup IDs used by the IOMMU(SMMU) binding. "swgroup" is a group of H/W clients in Tegra SoC from S/W POV.
This will allow the same header to be used by both device tree files, and drivers implementing this binding, which guarantees that the two stay in sync. This also makes device trees more readable by using names instead of magic numbers.
Why does the driver need these constants; shouldn't it simply support 0..n SW group IDs that all work the same way? Or, is there some reason for it to know the identities?
include/dt-bindings/memory/tegra-swgroup.h
Quoted 1 line
include/dt-bindings/memory/tegra-swgroup.h +0
-0
s/memory/iommu/?
s/tegra-swgroup.h/tegra-smmu.h/?
Quoted 1 line
+#define
TEGRA_SWGROUP_AFI0
Should this file document which values are valid for which SoCs? Should there be separate files for each SoC; I don't know if it's guaranteed that IDs won't be re-assigned between different SoCs.
include/dt-bindings/memory/tegra-swgroup.h line 48
Quoted 4 lines
include/dt-bindings/memory/tegra-swgroup.h +4
-0+#define TEGRA_SWGROUP_BIT (x) (1ULL << TEGRA_SWGROUP_##x) + +#define TEGRA30_SWGROUP_ALL <0x00000000 0x000779ff> +#define TEGRA114_SWGROUP_ALL <0x00000000 0x01b659fe>
Are those 3 values needed by DT files, or just the driver?
PATCH v2 3/22 ARM: tegra: Create a DT header defining swgroups ID
Stephen Warren <[email protected]> wrote @ Wed, 17 Jul 2013 01:07:12 +0200:
Quoted 9 lines · Hiroshi Doyu
Quoted 7 lines
Create a header file to define the swgroup IDs used by the IOMMU(SMMU) binding. "swgroup" is a group of H/W clients in Tegra SoC from S/W POV.
This will allow the same header to be used by both device tree files, and drivers implementing this binding, which guarantees that the two stay in sync. This also makes device trees more readable by using names instead of magic numbers.
Quoted 7 lines
Why does the driver need these constants; shouldn't it simply support 0..n SW group IDs that all work the same way? Or, is there some reason for it to know the identities?
include/dt-bindings/memory/tegra-swgroup.h
Quoted 1 line
include/dt-bindings/memory/tegra-swgroup.h +0
-0s/memory/iommu/?
This info would be used in PMC code as well, which H/W belongs to which swgroup. So I use the term "memory(-client)" instead of "iommu".
Quoted 1 line
s/tegra-swgroup.h/tegra-smmu.h/?
Is "tegra-memory-clients.h" preferable, then?
Quoted 5 lines
Quoted 1 line
+#define
TEGRA_SWGROUP_AFI0Should this file document which values are valid for which SoCs? Should there be separate files for each SoC; I don't know if it's guaranteed that IDs won't be re-assigned between different SoCs.
I've checked all SoC which I can refer. Those IDs are calculated from the register offset. But I haven't got any explicit guarantee from H/W designers yet.
Quoted 6 lines
include/dt-bindings/memory/tegra-swgroup.h line 48
Quoted 4 lines
include/dt-bindings/memory/tegra-swgroup.h +4
-0+#define TEGRA_SWGROUP_BIT (x) (1ULL << TEGRA_SWGROUP_##x) + +#define TEGRA30_SWGROUP_ALL <0x00000000 0x000779ff> +#define TEGRA114_SWGROUP_ALL <0x00000000 0x01b659fe>Are those 3 values needed by DT files, or just the driver?
Only from DT.
PATCH v2 3/22 ARM: tegra: Create a DT header defining swgroups ID
Quoted 15 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Wed, 17 Jul 2013 01:07:12 +0200:
Quoted 13 lines · Hiroshi Doyu
Quoted 7 lines
Create a header file to define the swgroup IDs used by the IOMMU(SMMU) binding. "swgroup" is a group of H/W clients in Tegra SoC from S/W POV.
This will allow the same header to be used by both device tree files, and drivers implementing this binding, which guarantees that the two stay in sync. This also makes device trees more readable by using names instead of magic numbers.
Why does the driver need these constants; shouldn't it simply support 0..n SW group IDs that all work the same way? Or, is there some reason for it to know the identities?
Quoted 6 lines
Quoted 3 lines
include/dt-bindings/memory/tegra-swgroup.h
Quoted 1 line
include/dt-bindings/memory/tegra-swgroup.h +0
-0s/memory/iommu/?
This info would be used in PMC code as well, which H/W belongs to which swgroup. So I use the term "memory(-client)" instead of "iommu".
Why does the PMC care? An explanation of exactly what SW needs to know about these groups and why would be a good idea.
Quoted 3 lines
Quoted 1 line
s/tegra-swgroup.h/tegra-smmu.h/?
Is "
tegra-memory-clients.h" preferable, then?
Perhaps, although aren't these IDs specific to the connection between modules and the SMMU? Or, are these IDs some more global concept; the HW modules in question pass the same IDs to both the SMMU and to other HW modules for some reason?
Quoted 8 lines
Quoted 6 lines
include/dt-bindings/memory/tegra-swgroup.h line 48
Quoted 4 lines
include/dt-bindings/memory/tegra-swgroup.h +4
-0+#define TEGRA_SWGROUP_BIT (x) (1ULL << TEGRA_SWGROUP_##x) + +#define TEGRA30_SWGROUP_ALL <0x00000000 0x000779ff> +#define TEGRA114_SWGROUP_ALL <0x00000000 0x01b659fe>Are those 3 values needed by DT files, or just the driver?
Only from DT.
If the values are only needed to write the DT content, I'd tend to prefer not defining them in a header in <dt-bindings/>; I consider that directory a definition of the constants that define the ABI itself, not as a random place to put useful things for DT. You can include headers in the *.dts directory itself.
PATCH v2 3/22 ARM: tegra: Create a DT header defining swgroups ID
Quoted 20 lines · On Mon, 29 Jul 2013 19:45:15 +0200 Stephen Warren <[email protected]> wrote:
Quoted 19 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Wed, 17 Jul 2013 01:07:12 +0200:
Quoted 16 lines · Hiroshi Doyu
Quoted 7 lines
Create a header file to define the swgroup IDs used by the IOMMU(SMMU) binding. "swgroup" is a group of H/W clients in Tegra SoC from S/W POV.
This will allow the same header to be used by both device tree files, and drivers implementing this binding, which guarantees that the two stay in sync. This also makes device trees more readable by using names instead of magic numbers.
Why does the driver need these constants; shouldn't it simply support 0..n SW group IDs that all work the same way? Or, is there some reason for it to know the identities?
include/dt-bindings/memory/tegra-swgroup.h
Quoted 1 line
include/dt-bindings/memory/tegra-swgroup.h +0
-0s/memory/iommu/?
Quoted 5 lines
Quoted 2 lines
This info would be used in PMC code as well, which H/W belongs to which swgroup. So I use the term "memory(-client)" instead of "iommu".
Why does the PMC care? An explanation of exactly what SW needs to know about these groups and why would be a good idea.
Joseph, can you explain a bit here?
PATCH v2 4/22 ARM: dt: tegra30: iommu: Add "nvidia,swgroup"
In reply to Hiroshi DoyuThis is a bitmap that indicates which HardWare Accelerators(HWA) are supported on Tegra30 SoC.
--- arch/arm/boot/dts/tegra30.dtsi | 2 ++ 1 file changed, 2 insertions(+)
arch/arm/boot/dts/tegra30.dtsi +2 -0
@@ -1,6 +1,7 @@ #include <dt-bindings/clock/tegra30-car.h> #include <dt-bindings/gpio/tegra-gpio.h> #include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/memory/tegra-swgroup.h> #include "skeleton.dtsi"
@@ -461,6 +462,7 @@ 0x7000f228 0x05c>; nvidia,#asids = <4>; /* # of ASIDs */ dma-window = <0 0x40000000>; /* IOVA start & length */ + nvidia,swgroup = TEGRA30_SWGROUP_ALL; nvidia,ahb = <&ahb>; };
Signature · 1 line
1.7.9.5
PATCH v2 4/22 ARM: dt: tegra30: iommu: Add "nvidia,swgroup"
arch/arm/boot/dts/tegra30.dtsi
Quoted 6 lines · Hiroshi Doyu
This is a bitmap that indicates which HardWare Accelerators(HWA) are supported on Tegra30 SoC.
arch/arm/boot/dts/tegra30.dtsi +1
-0+ nvidia,swgroup = TEGRA30_SWGROUP_ALL;
That property isn't in the binding document. We can't add properties that aren't documented.
Macros should not include structural syntax; the < > around the cell value is part of the macro the way you've written it, but shouldn't be. It'd be simpler to just write:
nvidia,swgroup = <0x00000000 0x000779ff>;
We should only extend the binding in a backwards-compatible fashion, so we can't require that DTs contain this value. At least, there should be a default value defined for when the property doesn't exist.
PATCH v2 5/22 ARM: dt: tegra30: iommu: Add "nvidia,memory-client"
In reply to Hiroshi DoyuAdd "nvidia,memory-client" to identify which swgroup ID a device belongs to.
--- arch/arm/boot/dts/tegra30.dtsi | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
arch/arm/boot/dts/tegra30.dtsi +19 -0
@@ -23,6 +23,7 @@ interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */ <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */ clocks = <&tegra_car TEGRA30_CLK_HOST1X>; + nvidia,memory-client = <TEGRA_SWGROUP_HC>; #address-cells = <1>; #size-cells = <1>;
@@ -34,6 +35,7 @@ reg = <0x54040000 0x00040000>; interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_MPE>; + nvidia,memory-client = <TEGRA_SWGROUP_MPE>; }; vi {
@@ -41,6 +43,7 @@ reg = <0x54080000 0x00040000>; interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_VI>; + nvidia,memory-client = <TEGRA_SWGROUP_VI>; }; epp {
@@ -48,6 +51,7 @@ reg = <0x540c0000 0x00040000>; interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_EPP>; + nvidia,memory-client = <TEGRA_SWGROUP_EPP>; }; isp {
@@ -55,6 +59,7 @@ reg = <0x54100000 0x00040000>; interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_ISP>; + nvidia,memory-client = <TEGRA_SWGROUP_ISP>; }; gr2d {
@@ -62,6 +67,7 @@ reg = <0x54140000 0x00040000>; interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_GR2D>; + nvidia,memory-client = <TEGRA_SWGROUP_G2>; }; gr3d {
@@ -69,6 +75,8 @@ reg = <0x54180000 0x00040000>; clocks = <&tegra_car 24 &tegra_car 98>; clock-names = "3d", "3d2"; + nvidia,memory-client = <TEGRA_SWGROUP_NV + TEGRA_SWGROUP_NV2>; }; dc at 54200000 {
@@ -78,6 +86,7 @@ clocks = <&tegra_car TEGRA30_CLK_DISP1>, <&tegra_car TEGRA30_CLK_PLL_P>; clock-names = "disp1", "parent"; + nvidia,memory-client = <TEGRA_SWGROUP_DC>; rgb { status = "disabled";
@@ -91,6 +100,7 @@ clocks = <&tegra_car TEGRA30_CLK_DISP2>, <&tegra_car TEGRA30_CLK_PLL_P>; clock-names = "disp2", "parent"; + nvidia,memory-client = <TEGRA_SWGROUP_DCB>; rgb { status = "disabled";
@@ -247,6 +257,7 @@ interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; nvidia,dma-request-selector = <&apbdma 8>; clocks = <&tegra_car TEGRA30_CLK_UARTA>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
@@ -257,6 +268,7 @@ interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>; nvidia,dma-request-selector = <&apbdma 9>; clocks = <&tegra_car TEGRA30_CLK_UARTB>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
@@ -267,6 +279,7 @@ interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>; nvidia,dma-request-selector = <&apbdma 10>; clocks = <&tegra_car TEGRA30_CLK_UARTC>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
@@ -277,6 +290,7 @@ interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>; nvidia,dma-request-selector = <&apbdma 19>; clocks = <&tegra_car TEGRA30_CLK_UARTD>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
@@ -287,6 +301,7 @@ interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>; nvidia,dma-request-selector = <&apbdma 20>; clocks = <&tegra_car TEGRA30_CLK_UARTE>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
@@ -536,6 +551,7 @@ reg = <0x78000000 0x200>; interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_SDMMC1>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
@@ -544,6 +560,7 @@ reg = <0x78000200 0x200>; interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_SDMMC2>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
@@ -552,6 +569,7 @@ reg = <0x78000400 0x200>; interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_SDMMC3>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
@@ -560,6 +578,7 @@ reg = <0x78000600 0x200>; interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA30_CLK_SDMMC4>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disabled"; };
Signature · 1 line
1.7.9.5
PATCH v2 5/22 ARM: dt: tegra30: iommu: Add "nvidia, memory-client"
Quoted 2 lines · Hiroshi Doyu
Add "nvidia,memory-client" to identify which swgroup ID a device belongs to.
This property also isn't in the documentation.
Again, if client devices are required to have this new property, the DT isn't backwards-compatible; fail:-(
arch/arm/boot/dts/tegra30.dtsi
Quoted 3 lines
arch/arm/boot/dts/tegra30.dtsi +1
-0+ nvidia,memory-client = <TEGRA_SWGROUP_HC>;
What if there's more than one SMMU in the system, e.g. since there are multiple different master-capable buses? Including the SMMU phandle in the property seems like a good idea so that the SMMU driver knows which devices are affected by it (although a bus structure with affected devices under the SMMU represented as nodes under the SMMU DT node seems a slightly more direct way of representing that, although that solution does only support systems where bus-master transactions flow up the bus structure in the same way that CPU transactions flow down it).
What about:
nvidia,smmu = <&smmu TEGRA_SWGROUP_HC>;
?
arch/arm/boot/dts/tegra30.dtsi line 79
Quoted 6 lines
arch/arm/boot/dts/tegra30.dtsi +2
-0@@ -69,6 +75,8 @@ reg = <0x54180000 0x00040000>; clocks = <&tegra_car 24 &tegra_car 98>; clock-names = "3d", "3d2"; + nvidia,memory-client = <TEGRA_SWGROUP_NV + TEGRA_SWGROUP_NV2>;
Oh, if there can be more than one, I suggest adding "s" to the end of the property name.
PATCH v2 5/22 ARM: dt: tegra30: iommu: Add "nvidia,memory-client"
Stephen Warren <[email protected]> wrote @ Wed, 17 Jul 2013 01:14:33 +0200:
Quoted 8 lines · Hiroshi Doyu
Quoted 2 lines
Add "nvidia,memory-client" to identify which swgroup ID a device belongs to.
This property also isn't in the documentation.
Again, if client devices are required to have this new property, the DT isn't backwards-compatible; fail:-(
If there's no this property, just falling back to use the system default address space. I think that this won't break the compatibility basically. Only problem may be the inconsistency of setting this property among the HWs in the same group. If one of them is set in one AS, but not for the other, then the other H/W won't work. If we really want to have this kind of constraint implmeneted in DT, some kind of bus model is needed.
PATCH v2 5/22 ARM: dt: tegra30: iommu: Add "nvidia, memory-client"
Quoted 8 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Wed, 17 Jul 2013 01:14:33 +0200:
Quoted 6 lines · Hiroshi Doyu
Quoted 2 lines
Add "nvidia,memory-client" to identify which swgroup ID a device belongs to.
This property also isn't in the documentation.
Quoted 7 lines
Quoted 2 lines
Again, if client devices are required to have this new property, the DT isn't backwards-compatible; fail:-(
If there's no this property, just falling back to use the system default address space. I think that this won't break the compatibility basically. Only problem may be the inconsistency of setting this property among the HWs in the same group.
When you say "group" there, what kind of group? I thought that each HW module had its own "SWGROUP ID", and hence there are no "HWs in the same group"?
PATCH v2 5/22 ARM: dt: tegra30: iommu: Add "nvidia,memory-client"
Quoted 12 lines · On Mon, 29 Jul 2013 19:47:04 +0200 Stephen Warren <[email protected]> wrote:
Quoted 11 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Wed, 17 Jul 2013 01:14:33 +0200:
Quoted 8 lines · Hiroshi Doyu
Quoted 2 lines
Add "nvidia,memory-client" to identify which swgroup ID a device belongs to.
This property also isn't in the documentation.
Again, if client devices are required to have this new property, the DT isn't backwards-compatible; fail:-(
Quoted 8 lines
Quoted 4 lines
If there's no this property, just falling back to use the system default address space. I think that this won't break the compatibility basically. Only problem may be the inconsistency of setting this property among the HWs in the same group.
When you say "group" there, what kind of group? I thought that each HW module had its own "SWGROUP ID", and hence there are no "HWs in the same group"?
A "swgroup" can have multiple HWs. So there are 3 concept here.
swgroup: one client to issue request from MC perspective
HW: swgroup can have multiple HWs(real H/W units) device driver: Can have multiple swgroups.
PATCH v2 6/22 ARM: dt: tegra114: iommu: Fix IOMMU register address
In reply to Hiroshi DoyuFix IOMMU register address.
--- arch/arm/boot/dts/tegra114.dtsi | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/arm/boot/dts/tegra114.dtsi +3 -3
@@ -318,9 +318,9 @@ iommu { compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu"; - reg = <0x7000f010 0x02c - 0x7000f1f0 0x010 - 0x7000f228 0x074>; + reg = <0x70019010 0x02c + 0x700191f0 0x010 + 0x70019228 0x074>; nvidia,#asids = <4>; dma-window = <0 0x40000000>; nvidia,swgroups = <0x18659fe>;
Signature · 1 line
1.7.9.5
PATCH v2 6/22 ARM: dt: tegra114: iommu: Fix IOMMU register address
Quoted 1 line · Hiroshi Doyu
Fix IOMMU register address.
Oh dear, how serious is this?
That incorrect node got added in v3.9 - a long time ago. Does the SMMU driver touch registers during probe() or under any other condition right now? If so, can that hang the CPU or end up trashing some other HW module's registers?
I'm trying to determine whether we need to urgently send this patch as a fix Cc: stable.
PATCH v2 6/22 ARM: dt: tegra114: iommu: Fix IOMMU register address
Quoted 5 lines · Stephen Warren
Quoted 1 line · Hiroshi Doyu
Fix IOMMU register address.
Oh dear, how serious is this?
Quoted 7 lines
That incorrect node got added in v3.9 - a long time ago. Does the SMMU driver touch registers during
probe()or under any other condition right now? If so, can that hang the CPU or end up trashing some other HW module's registers?I'm trying to determine whether we need to urgently send this patch as a fix Cc: stable.
Judging by the TRM there's nothing in the range from 0x7000ec00 to 0x7000f7ff, so hopefully we got lucky.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130716/346a833a/attachment.sig>
PATCH v2 7/22 ARM: dt: tegra114: iommu: Add "nvidia,swgroups"
In reply to Hiroshi DoyuThis is a bitmap that indicates which HardWare Accelerators(HWA) are supported on Tegra114 SoC.
--- arch/arm/boot/dts/tegra114.dtsi | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/boot/dts/tegra114.dtsi +2 -1
@@ -1,6 +1,7 @@ #include <dt-bindings/clock/tegra114-car.h> #include <dt-bindings/gpio/tegra-gpio.h> #include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/memory/tegra-swgroup.h> #include "skeleton.dtsi"
@@ -323,7 +324,7 @@ 0x70019228 0x074>; nvidia,#asids = <4>; dma-window = <0 0x40000000>; - nvidia,swgroups = <0x18659fe>; + nvidia,swgroups = TEGRA114_SWGROUP_ALL; nvidia,ahb = <&ahb>; };
Signature · 1 line
1.7.9.5
PATCH v2 8/22 ARM: dt: tegra114: Add "nvidia,memory-client"
In reply to Hiroshi DoyuAdd "nvidia,memory-client" to identify which swgroup ID a device belongs to.
--- arch/arm/boot/dts/tegra114.dtsi | 4 ++++ 1 file changed, 4 insertions(+)
arch/arm/boot/dts/tegra114.dtsi +4 -0
@@ -404,6 +404,7 @@ reg = <0x78000000 0x200>; interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA114_CLK_SDMMC1>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disable"; };
@@ -412,6 +413,7 @@ reg = <0x78000200 0x200>; interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA114_CLK_SDMMC2>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disable"; };
@@ -420,6 +422,7 @@ reg = <0x78000400 0x200>; interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA114_CLK_SDMMC3>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disable"; };
@@ -428,6 +431,7 @@ reg = <0x78000600 0x200>; interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA114_CLK_SDMMC4>; + nvidia,memory-client = <TEGRA_SWGROUP_PPCS>; status = "disable"; };
Signature · 1 line
1.7.9.5
PATCH v2 9/22 amba: Move AHB to core_initcall
In reply to Hiroshi DoyuMove AHB to core_initcall in order to instanciate this device earlier than others since IOMMU depends on AHB and then IOMMU driver needs to reigster other platform devices as IOMMU'able.
--- drivers/amba/tegra-ahb.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/amba/tegra-ahb.c +6 -1
@@ -283,7 +283,12 @@ static struct platform_driver tegra_ahb_driver = { .pm = &tegra_ahb_pm, }, }; -module_platform_driver (tegra_ahb_driver); + +static int tegra_ahb_init (void ) +{ +return platform_driver_register (&tegra_ahb_driver); +} +core_initcall (tegra_ahb_init);MODULE_AUTHOR ("Hiroshi DOYU <[email protected]>" );MODULE_DESCRIPTION ("Tegra AHB driver" );
Signature · 1 line
1.7.9.5
PATCH v2 9/22 amba: Move AHB to core_initcall
Quoted 3 lines · Hiroshi Doyu
Move AHB to
core_initcallin order to instanciate this device earlier than others since IOMMU depends on AHB and then IOMMU driver needs to reigster other platform devices as IOMMU'able.
Oh dear, this really is looking like we shouldn't be playing games by attempting to force the initialization order. An explicit bus structure and/or having client devices go and attach to the SMMU, would mean we didn't have to play games like this:-(
PATCH v2 10/22 iommu/tegra: smmu: Move IOMMU to core_initcall
In reply to Hiroshi DoyuMove IOMMU to core_initcall in order to instanciate this device earlier than others since IOMMU driver needs to reigster other platform devices as IOMMU'able.
--- drivers/iommu/tegra-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/tegra-smmu.c +1 -1
@@ -1288,7 +1288,7 @@ static void __exit tegra_smmu_exit(void)platform_driver_unregister (&tegra_smmu_driver); } -subsys_initcall (tegra_smmu_init); +core_initcall (tegra_smmu_init);module_exit (tegra_smmu_exit);MODULE_DESCRIPTION ("IOMMU API for SMMU in Tegra30" );
Signature · 1 line
1.7.9.5
PATCH v2 10/22 iommu/tegra: smmu: Move IOMMU to core_initcall
Quoted 3 lines · Hiroshi Doyu
Move IOMMU to
core_initcallin order to instanciate this device earlier than others since IOMMU driver needs to reigster other platform devices as IOMMU'able.
If both the AHB and SMMU driver are core_initcall, there's no guarantee at all re: which order they'll get probed in. Deferred probe won't help here, since I don't think it's guaranteed to be repeated, so if the probe order is:
1) SMMU (fails since AHB isn't available)
2) AHB
... then the SMMU might not get probed until /much/ later; probably after devices that are affected by the SMMU are probed.
However, it all works out if the probe order just accidentally happens to be:
1) AHB
2) SMMU.
So, this really doesn't look like a viable solution.
PATCH v2 11/22 iommu/tegra: smmu: Add Tegra 114 support
In reply to Hiroshi DoyuTegra 114 SMMU uses the almost same logic as one in Tegra 30 except that they have different HaredWare Accelerators(HWA). Those difference is provided from DT.
--- .../bindings/iommu/nvidia,tegra30-smmu.txt | 2 +- drivers/iommu/tegra-smmu.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +1 -1
@@ -1,7 +1,7 @@ NVIDIA Tegra 30 IOMMU H/W, SMMU (System Memory Management Unit) Required properties: -- compatible : "nvidia,tegra30-smmu" +- compatible : "nvidia,tegra114-smmu", "nvidia,tegra30-smmu" - reg : Should contain 3 register banks(address and length) for each of the SMMU register blocks. - interrupts : Should contain MC General interrupt.
drivers/iommu/tegra-smmu.c +1 -0
@@ -1262,6 +1262,7 @@ const struct dev_pm_ops tegra_smmu_pm_ops = { };static struct of_device_id tegra_smmu_of_match[] = { + { .compatible ="nvidia,tegra114-smmu" , }, { .compatible ="nvidia,tegra30-smmu" , }, { }, };
Signature · 1 line
1.7.9.5
PATCH v2 11/22 iommu/tegra: smmu: Add Tegra 114 support
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt line 4
Quoted 9 lines · Hiroshi Doyu
Tegra 114 SMMU uses the almost same logic as one in Tegra 30 except that they have different HaredWare Accelerators(HWA). Those difference is provided from DT.
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +1
-1Required properties: -- compatible : "nvidia,tegra30-smmu" +- compatible : "nvidia,tegra114-smmu", "nvidia,tegra30-smmu"
So that we don't have to list every single chip in that list, we've often written that as:
compatible : "nvidia,tegra<chip>-smmu".
drivers/iommu/tegra-smmu.c const struct dev_pm_ops tegra_smmu_pm_ops = { · line 1266
Quoted 5 lines
drivers/iommu/tegra-smmu.c +1
-0static struct of_device_id tegra_smmu_of_match[] = { + { .compatible ="nvidia,tegra114-smmu" , }, { .compatible ="nvidia,tegra30-smmu" , },
So, there are no HW differences, and hence no driver changes required? In that case, there's no point making the driver explicitly support nvidia,tegra114-smmu, since the DT will always be:
compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
... and hence always match on the existing nvidia,tegra30-smmu entry in that table.
PATCH v2 12/22 iommu/tegra: smmu: Select ARM_DMA_USE_IOMMU in Kconfig
In reply to Hiroshi DoyuSMMU expects ARM_DMA_USE_IOMMU to be enabled to use DMA mapping API by default.
--- drivers/iommu/Kconfig | 1 + 1 file changed, 1 insertion(+)
drivers/iommu/Kconfig +1 -0
@@ -160,6 +160,7 @@ config TEGRA_IOMMU_SMMU bool "Tegra SMMU IOMMU Support" depends on ARCH_TEGRA && TEGRA_AHB select IOMMU_API + select ARM_DMA_USE_IOMMU help Enables support for remapping discontiguous physical memory shared with the operating system into contiguous I/O virtual
Signature · 1 line
1.7.9.5
PATCH v2 13/22 iommu/tegra: smmu: Create default IOVA maps
In reply to Hiroshi DoyuCreate default IOVA maps at boot-up which can be attached to devices.
--- drivers/iommu/tegra-smmu.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
drivers/iommu/tegra-smmu.c +26 -2
@@ -319,6 +319,8 @@ struct smmu_device {struct device_node *ahb; +struct dma_iommu_mapping **map; +int num_as;struct smmu_as as[0 ];/* Run-time allocated array */ };
@@ -1144,6 +1146,26 @@ static int tegra_smmu_resume(struct device *dev)return err; } +static int tegra_smmu_create_default_map (struct smmu_device *smmu) +{ +int i; + +for (i =0 ; i < smmu->num_as; i++) { +dma_addr_t base = smmu->iovmm_base; +size_t size = smmu->page_count <<PAGE_SHIFT ; + + smmu->map[i] =arm_iommu_create_mapping (&platform_bus_type, + base, size,0 ); +if (WARN_ON (IS_ERR (smmu->map[i]))) +return -EINVAL ; + +dev_dbg (smmu->dev,"map:%p %08x-%08x\n" , + smmu->map[i], base, base + size -1 ); + } + +return 0 ; +} +static int tegra_smmu_probe (struct platform_device *pdev) {struct smmu_device *smmu;
@@ -1160,13 +1182,15 @@ static int tegra_smmu_probe(struct platform_device *pdev)if (of_property_read_u32 (dev->of_node,"nvidia,#asids" , &asids))return -ENODEV ; - bytes =sizeof (*smmu) + asids *sizeof (*smmu->as); + bytes =sizeof (*smmu) + asids * (sizeof (*smmu->as) + +sizeof (struct dma_iommu_mapping *)); smmu =devm_kzalloc (dev, bytes,GFP_KERNEL );if (!smmu) {dev_err (dev,"failed to allocate smmu_device\n" );return -ENOMEM ; } + smmu->map = (struct dma_iommu_mapping **)(smmu->as + asids); smmu->nregs = pdev->num_resources; smmu->regs =devm_kzalloc (dev,2 * smmu->nregs *sizeof (*smmu->regs),GFP_KERNEL );
@@ -1238,7 +1262,7 @@ static int tegra_smmu_probe(struct platform_device *pdev)smmu_debugfs_create (smmu); smmu_handle = smmu;bus_set_iommu (&platform_bus_type, &smmu_iommu_ops); -return 0 ; +return tegra_smmu_create_default_map (smmu); }static int tegra_smmu_remove (struct platform_device *pdev)
Signature · 1 line
1.7.9.5
PATCH v2 13/22 iommu/tegra: smmu: Create default IOVA maps
Quoted 12 lines · Hiroshi Doyu
Create default IOVA maps at boot-up which can be attached to devices.
drivers/iommu/tegra-smmu.c +0
-0static int tegra_smmu_probe (struct platform_device *pdev) {struct smmu_device *smmu;@@ -1160,13 +1182,15 @@ static int tegra_smmu_probe(struct platform_device *pdev)if (of_property_read_u32 (dev->of_node,"nvidia,#asids" , &asids))return -ENODEV ;
drivers/iommu/tegra-smmu.c static int tegra_smmu_probe(struct platform_device *pdev) · line 1193
Quoted 10 lines
drivers/iommu/tegra-smmu.c +3
-1- bytes =sizeof (*smmu) + asids *sizeof (*smmu->as); + bytes =sizeof (*smmu) + asids * (sizeof (*smmu->as) + +sizeof (struct dma_iommu_mapping *)); smmu =devm_kzalloc (dev, bytes,GFP_KERNEL );if (!smmu) {dev_err (dev,"failed to allocate smmu_device\n" );return -ENOMEM ; } + smmu->map = (struct dma_iommu_mapping **)(smmu->as + asids);
Shouldn't "+ asids" be "+ asids * sizeof(*smmu->as)" to match the calculation of "bytes" above?
PATCH v2 13/22 iommu/tegra: smmu: Create default IOVA maps
Stephen Warren <[email protected]> wrote @ Thu, 18 Jul 2013 22:10:29 +0200:
Quoted 22 lines · Hiroshi Doyu
Quoted 21 lines
Create default IOVA maps at boot-up which can be attached to devices.
drivers/iommu/tegra-smmu.c +2
-1static int tegra_smmu_probe (struct platform_device *pdev) {struct smmu_device *smmu;@@ -1160,13 +1182,15 @@ static int tegra_smmu_probe(struct platform_device *pdev)if (of_property_read_u32 (dev->of_node,"nvidia,#asids" , &asids))return -ENODEV ; - bytes =sizeof (*smmu) + asids *sizeof (*smmu->as); + bytes =sizeof (*smmu) + asids * (sizeof (*smmu->as) + +sizeof (struct dma_iommu_mapping *)); smmu =devm_kzalloc (dev, bytes,GFP_KERNEL );if (!smmu) {dev_err (dev,"failed to allocate smmu_device\n" );return -ENOMEM ; }
Quoted 4 lines
drivers/iommu/tegra-smmu.c static int tegra_smmu_probe(struct platform_device *pdev) · line 1193
Quoted 1 line
drivers/iommu/tegra-smmu.c +1
-0+ smmu->map = (struct dma_iommu_mapping **)(smmu->as + asids);Shouldn't "+ asids" be "+ asids *
sizeof(*smmu->as)" to match the calculation of "bytes" above?
The structure is:
smmu {
....
struct dma_iommu_mapping **map;
....
struct smmu_as as[0];
};I think that this is correct, but is the following better?
+ smmu->map = (struct dma_iommu_mapping **)&smmu->as[asids];
PATCH v2 13/22 iommu/tegra: smmu: Create default IOVA maps
Quoted 31 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Thu, 18 Jul 2013 22:10:29 +0200:
Quoted 22 lines · Hiroshi Doyu
Quoted 21 lines
Create default IOVA maps at boot-up which can be attached to devices.
drivers/iommu/tegra-smmu.c +2
-1static int tegra_smmu_probe (struct platform_device *pdev) {struct smmu_device *smmu;@@ -1160,13 +1182,15 @@ static int tegra_smmu_probe(struct platform_device *pdev)if (of_property_read_u32 (dev->of_node,"nvidia,#asids" , &asids))return -ENODEV ; - bytes =sizeof (*smmu) + asids *sizeof (*smmu->as); + bytes =sizeof (*smmu) + asids * (sizeof (*smmu->as) + +sizeof (struct dma_iommu_mapping *)); smmu =devm_kzalloc (dev, bytes,GFP_KERNEL );if (!smmu) {dev_err (dev,"failed to allocate smmu_device\n" );return -ENOMEM ; }Quoted 4 lines
drivers/iommu/tegra-smmu.c static int tegra_smmu_probe(struct platform_device *pdev) · line 1193
Quoted 1 line
drivers/iommu/tegra-smmu.c +1
-0+ smmu->map = (struct dma_iommu_mapping **)(smmu->as + asids);Shouldn't "+ asids" be "+ asids *
sizeof(*smmu->as)" to match the calculation of "bytes" above?The structure is:
Quoted 8 lines
smmu { .... struct dma_iommu_mapping **map; .... struct smmu_as as[0]; };I think that this is correct, but is the following better?
Oh right, I guess since that's pointer-math, the compiler already multiplies by the array entry size.
Quoted 1 line
+
smmu->map= (structdma_iommu_mapping**)&smmu->as[asids];
That would be a bit more obvious.
PATCH v2 14/22 iommu/tegra: smmu: Register platform_device to IOMMU dynamically
In reply to Hiroshi DoyuRegister platform_devices to IOMMU dynamically via ops->{add,remove}_device().
--- drivers/iommu/tegra-smmu.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
drivers/iommu/tegra-smmu.c +37 -0
@@ -39,6 +39,7 @@#include <asm/page.h> #include <asm/cacheflush.h> +#include <asm/dma-iommu.h> enum smmu_hwgrp {HWGRP_AFI ,
@@ -949,6 +950,40 @@ static void smmu_iommu_domain_destroy(struct iommu_domain *domain)dev_dbg (smmu->dev,"smmu_as@%p\n" , as); } +/* +* ASID[0] for the system default +* ASID[1] for PPCS, which has SDMMC +* ASID[2][3].. open for drivers, first come, first served. +*/ +enum { +SYSTEM_DEFAULT , +SYSTEM_PROTECTED , +}; + +static int smmu_iommu_add_device (struct device *dev) +{ +int err; +struct dma_iommu_mapping *map = smmu_handle->map[SYSTEM_DEFAULT ]; + +if (!map) + goto out; + + err =arm_iommu_attach_device (dev, map); +if (err) { +dev_err (dev,"Failed to attach to map %p\n" , map); +return err; + } +out: +dev_dbg (dev,"Attached to map %p\n" , map); +return 0 ; +} + +static void smmu_iommu_remove_device (struct device *dev) +{ +dev_dbg (dev,"Detaching from map %p\n" ,to_dma_iommu_mapping (dev)); +arm_iommu_detach_device (dev); +} +static struct iommu_ops smmu_iommu_ops = { .domain_init = smmu_iommu_domain_init, .domain_destroy = smmu_iommu_domain_destroy,
@@ -958,6 +993,8 @@ static struct iommu_ops smmu_iommu_ops = { .unmap = smmu_iommu_unmap, .iova_to_phys = smmu_iommu_iova_to_phys, .domain_has_cap = smmu_iommu_domain_has_cap, + .add_device = smmu_iommu_add_device, + .remove_device = smmu_iommu_remove_device, .pgsize_bitmap = SMMU_IOMMU_PGSIZES, };
Signature · 1 line
1.7.9.5
PATCH v2 14/22 iommu/tegra: smmu: Register platform_device to IOMMU dynamically
drivers/iommu/tegra-smmu.c static void smmu_iommu_domain_destroy(struct iommu_domain *domain) · line 955
Quoted 8 lines · Hiroshi Doyu
Register
platform_devicesto IOMMU dynamically via ops->{add,remove}_device().drivers/iommu/tegra-smmu.c +3
-0+/* +* ASID[0] for the system default +* ASID[1] for PPCS, which has SDMMC
I have no idea what PPCS is. The patch description for 21/22 implies much more than SDMMC is "in PPCS"...
drivers/iommu/tegra-smmu.c static void smmu_iommu_domain_destroy(struct iommu_domain *domain) · line 961
Quoted 6 lines
+ * ASID[2][3].. open for drivers, first come, first served.
+ */drivers/iommu/tegra-smmu.c +4
-0+enum { +SYSTEM_DEFAULT , +SYSTEM_PROTECTED , +};
Why hard-code this mapping? Can't devices be assigned to ASIDs based on a DT property? I assume there's nothing in the SMMU HW that requires specific ASIDs to be used?
I don't see anything in this series which implements the "open for drivers" part of the description for ASID 2/3/...
Perhaps not so relevant here since this patch only uses SYSTEM_DEFAULT as the ASID number, but don't you need to validate that smmu->num_as is large enough to support all values in the enum definition above?
Since this patch doesn't actually use anything other than SYSTEM_DEFAULT, I wonder if this patch shouldn't be squashed into patch 21/22, where multiple ASIDs are actually used?
PATCH v2 14/22 iommu/tegra: smmu: Register platform_device to IOMMU dynamically
Stephen Warren <[email protected]> wrote @ Thu, 18 Jul 2013 22:17:21 +0200:
Quoted 12 lines · Hiroshi Doyu
drivers/iommu/tegra-smmu.c static void smmu_iommu_domain_destroy(struct iommu_domain *domain) · line 955
Quoted 8 lines
Register
platform_devicesto IOMMU dynamically via ops->{add,remove}_device().drivers/iommu/tegra-smmu.c +3
-0+/* +* ASID[0] for the system default +* ASID[1] for PPCS, which has SDMMC I have no idea what PPCS is. The patch description for 21/22 implies much more than SDMMC is "in PPCS"...
"PPCS" is all AHB bus children/(client).
Quoted 10 lines
drivers/iommu/tegra-smmu.c static void smmu_iommu_domain_destroy(struct iommu_domain *domain) · line 961
Quoted 6 lines
+ * ASID[2][3].. open for drivers, first come, first served.
+ */drivers/iommu/tegra-smmu.c +4
-0+enum { +SYSTEM_DEFAULT , +SYSTEM_PROTECTED , +};Why hard-code this mapping? Can't devices be assigned to ASIDs based on a DT property? I assume there's nothing in the SMMU HW that requires specific ASIDs to be used?
Right. Where should those policy be passed, board DT files?
Quoted 2 lines
I don't see anything in this series which implements the "open for drivers" part of the description for ASID 2/3/...
I expect that some smart HW(host1x) will use AS[2/3] for their smarter usage.
PATCH v2 14/22 iommu/tegra: smmu: Register platform_device to IOMMU dynamically
Quoted 8 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Thu, 18 Jul 2013 22:17:21 +0200:
Quoted 6 lines · Hiroshi Doyu
Quoted 4 lines
Register
platform_devicesto IOMMU dynamically via ops->{add,remove}_device().drivers/iommu/tegra-smmu.c +0
-0
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
+/*
+ * ASID[0] for the system default
+ * ASID[1] for PPCS, which has SDMMCI have no idea what PPCS is. The patch description for 21/22 implies much more than SDMMC is "in PPCS"...
"PPCS" is all AHB bus children/(client).
Just to be clear: I was hoping that the comment would be fixed.
Quoted 11 lines
Quoted 10 lines
drivers/iommu/tegra-smmu.c static void smmu_iommu_domain_destroy(struct iommu_domain *domain) · line 961
Quoted 6 lines
+ * ASID[2][3].. open for drivers, first come, first served.
+ */drivers/iommu/tegra-smmu.c +4
-0+enum { +SYSTEM_DEFAULT , +SYSTEM_PROTECTED , +};Why hard-code this mapping? Can't devices be assigned to ASIDs based on a DT property? I assume there's nothing in the SMMU HW that requires specific ASIDs to be used?
Quoted 1 line
Right. Where should those policy be passed, board DT files?
Is the HW module (== SW group?) to ASID mapping policy or is a particular mapping required by HW?
Can the ASID mapping just happen dynamically in SW rather than DT dictating a particular mapping?
I assume the mapping must be set up before any HW is used, and can't be modified later, so there's no much chance of deferring any policy decisions to user-space etc.?
PATCH v2 14/22 iommu/tegra: smmu: Register platform_device to IOMMU dynamically
On Mon, 29 Jul 2013 19:54:09 +0200
Stephen Warren <[email protected]> wrote:
...
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Why hard-code this mapping? Can't devices be assigned to ASIDs based on a DT property? I assume there's nothing in the SMMU HW that requires specific ASIDs to be used?
Right. Where should those policy be passed, board DT files?
Is the HW module (== SW group?) to ASID mapping policy or is a particular mapping required by HW?
only policy.
But swgroup can have multiple HWs, they need to be set in the same swgroup.
Quoted 2 lines
Can the ASID mapping just happen dynamically in SW rather than DT dictating a particular mapping?
No info about ASID in DT, at least, right now.
If a board DT file is the place to put policy, then it can be an option.
Quoted 3 lines
I assume the mapping must be set up before any HW is used, and can't be modified later, so there's no much chance of deferring any policy decisions to user-space etc.?
I think that both cases are needed.
Some traditional devices(MMC) can be changed later as it may have rootfs on it. For some smarter devices like host1x, it want to change ASID dynamically per request/context.
PATCH v2 15/22 iommu/tegra: smmu: Calculate ASID register offset by ID
In reply to Hiroshi DoyuCalculate ASID register offset by ID so that we can get rid of SoC specific MACROs. This is needed for the unified SMMU driver over Tegra SoCs.
--- drivers/iommu/tegra-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/tegra-smmu.c +1 -1
@@ -256,7 +256,7 @@ static const u32 smmu_hwgrp_asid_reg[] = {HWGRP_INIT (VDE ), HWGRP_INIT(VI), }; -#define HWGRP_ASID_REG (x) (smmu_hwgrp_asid_reg[x]) +#define HWGRP_ASID_REG (x) ((x) * sizeof(u32) + SMMU_AFI_ASID)/* * Per client for address space
Signature · 1 line
1.7.9.5
PATCH v2 15/22 iommu/tegra: smmu: Calculate ASID register offset by ID
drivers/iommu/tegra-smmu.c static const u32 smmu_hwgrp_asid_reg[] = { · line 259
Quoted 8 lines · Hiroshi Doyu
Calculate ASID register offset by ID so that we can get rid of SoC specific MACROs. This is needed for the unified SMMU driver over Tegra SoCs.
drivers/iommu/tegra-smmu.c +1
-1-#define HWGRP_ASID_REG (x) (smmu_hwgrp_asid_reg[x]) +#define HWGRP_ASID_REG (x) ((x) * sizeof(u32) + SMMU_AFI_ASID)
I think this removes the last use of smmu_hwgrp_asid_reg[]. Don't you want to delete that in this patch?
PATCH v2 16/22 iommu/tegra: smmu: Get "nvidia,swgroup" from DT
In reply to Hiroshi DoyuThis provides the info about which H/W Accelerators are supported on Tegra SoC. This info is passed from DT. This is necessary to have the unified SMMU driver among Tegra SoCs. Instead of using platform data, DT passes "nvidia,swgroup" now. DT is mandatory in Tegra.
--- .../bindings/iommu/nvidia,tegra30-smmu.txt | 6 +++- drivers/iommu/tegra-smmu.c | 31 +++++++++----------- 2 files changed, 19 insertions(+), 18 deletions(-)
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +5 -1
@@ -8,14 +8,18 @@ Required properties: - nvidia,#asids : # of ASIDs - dma-window : IOVA start address and length. - nvidia,ahb : phandle to the ahb bus connected to SMMU. +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). + Each bit represents one swgroup. The assignments may be found in header + file <dt-bindings/memory/tegra-swgroup.h>. Example: - smmu { + iommu { compatible = "nvidia,tegra30-smmu"; reg = <0x7000f010 0x02c 0x7000f1f0 0x010 0x7000f228 0x05c>; nvidia,#asids = <4>; /* # of ASIDs */ dma-window = <0 0x40000000>; /* IOVA start & length */ + nvidia,swgroups = TEGRA30_SWGROUP_ALL; nvidia,ahb = <&ahb>; };
drivers/iommu/tegra-smmu.c +14 -17
@@ -265,7 +265,7 @@ struct smmu_client {struct device *dev;struct list_head list;struct smmu_as *as; -u32 hwgrp; +u64 hwgrp; };/*
@@ -307,6 +307,8 @@ struct smmu_device {struct device *dev;struct page *avp_vector_page;/* dummy page shared by all AS's */ +u64 swgroup;/* swgroup ID bitmap */ +/* * Register image savers for suspend/resume */
@@ -382,10 +384,10 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)*/ #define FLUSH_SMMU_REGS (smmu) smmu_read(smmu, SMMU_CONFIG) -#define smmu_client_hwgrp (c) (u32)((c)->dev->platform_data) +#define smmu_client_hwgrp (c) (c->as->smmu->swgroup)static int __smmu_client_set_hwgrp (struct smmu_client *c, -unsigned long map,int on) +u64 map,int on) {int i;struct smmu_as *as = c->as;
@@ -398,12 +400,11 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,if (!on) map =smmu_client_hwgrp (c); -for_each_set_bit (i, &map,HWGRP_COUNT ) { +for_each_set_bit (i, (unsigned long *)&map, +sizeof (map) *BITS_PER_BYTE ) { offs =HWGRP_ASID_REG (i); val =smmu_read (smmu, offs);if (on) { -if (WARN_ON (val & mask)) - goto err_hw_busy; val |= mask; }else {WARN_ON ((val & mask) == mask);
@@ -414,15 +415,6 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,FLUSH_SMMU_REGS (smmu); c->hwgrp = map;return 0 ; - -err_hw_busy: -for_each_set_bit (i, &map,HWGRP_COUNT ) { - offs =HWGRP_ASID_REG (i); - val =smmu_read (smmu, offs); - val &= ~mask; -smmu_write (smmu, val, offs); - } -return -EBUSY ; }static int smmu_client_set_hwgrp (struct smmu_client *c,u32 map,int on)
@@ -794,7 +786,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,struct smmu_as *as = domain->priv;struct smmu_device *smmu = as->smmu;struct smmu_client *client, *c; -u32 map; +u64 map;int err; client =devm_kzalloc (smmu->dev,sizeof (*c),GFP_KERNEL );
@@ -802,7 +794,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,return -ENOMEM ; client->dev = dev; client->as = as; - map = (unsigned long )dev->platform_data; + map = smmu->swgroup;if (!map)return -EINVAL ;
@@ -1210,6 +1202,7 @@ static int tegra_smmu_probe(struct platform_device *pdev)int i, asids, err =0 ;dma_addr_t uninitialized_var (base );size_t bytes,uninitialized_var (size ); +u64 swgroup;if (smmu_handle)return -EIO ;
@@ -1219,6 +1212,9 @@ static int tegra_smmu_probe(struct platform_device *pdev)if (of_property_read_u32 (dev->of_node,"nvidia,#asids" , &asids))return -ENODEV ; +if (of_property_read_u64 (dev->of_node,"nvidia,swgroup" , &swgroup)) +return -ENODEV ; + bytes =sizeof (*smmu) + asids * (sizeof (*smmu->as) +sizeof (struct dma_iommu_mapping *)); smmu =devm_kzalloc (dev, bytes,GFP_KERNEL );
@@ -1267,6 +1263,7 @@ static int tegra_smmu_probe(struct platform_device *pdev) smmu->num_as = asids; smmu->iovmm_base = base; smmu->page_count = size; + smmu->swgroup = swgroup; smmu->translation_enable_0 = ~0 ; smmu->translation_enable_1 = ~0 ;
Signature · 1 line
1.7.9.5
PATCH v2 16/22 iommu/tegra: smmu: Get "nvidia,swgroup" from DT
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt Required properties: · line 13
Quoted 10 lines · Hiroshi Doyu
This provides the info about which H/W Accelerators are supported on Tegra SoC. This info is passed from DT. This is necessary to have the unified SMMU driver among Tegra SoCs. Instead of using platform data, DT passes "nvidia,swgroup" now. DT is mandatory in Tegra.
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +3
-0+- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). + Each bit represents one swgroup. The assignments may be found in header + file <dt-bindings/memory/tegra-swgroup.h>.
There needs to be a default for this field if one is not specified so that existing DTs continue to work without modification.
How many cells big is this property?
Is this really a bitmap of HWAs? Surely it's a bitmap of SMMU client IDs?
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt Required properties: · line 16
Quoted 3 lines
Example:
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +1
-1- smmu { + iommu {
The node name shouldn't be interpreted by anything, so there should be no need to change it at all. Certainly, it should not be changed by this patch, since this patch is all about SMMU client IDs.
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt Required properties: · line 23
Quoted 1 line
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +1
-0+ nvidia,swgroups = TEGRA30_SWGROUP_ALL;
As I mentioned before, macros shouldn't include syntax/structure, just data values.
drivers/iommu/tegra-smmu.c struct smmu_client { · line 268
Quoted 8 lines
drivers/iommu/tegra-smmu.c +1
-1@@ -265,7 +265,7 @@ struct smmu_client {struct device *dev;struct list_head list;struct smmu_as *as; -u32 hwgrp; +u64 hwgrp;
Why is that "hwgrp" not "swgrp"? Don't they represent the same thing?
drivers/iommu/tegra-smmu.c struct smmu_device { · line 310
Quoted 5 lines
drivers/iommu/tegra-smmu.c +1
-0@@ -307,6 +307,8 @@ struct smmu_device {struct device *dev;struct page *avp_vector_page;/* dummy page shared by all AS's */ +u64 swgroup;/* swgroup ID bitmap */
If that's a bitmap, then it represents multiple things, so "swgroups"?
drivers/iommu/tegra-smmu.c static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs) · line 387
Quoted 6 lines
drivers/iommu/tegra-smmu.c +1
-1@@ -382,10 +384,10 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)*/ #define FLUSH_SMMU_REGS (smmu) smmu_read(smmu, SMMU_CONFIG) -#define smmu_client_hwgrp (c) (u32)((c)->dev->platform_data) +#define smmu_client_hwgrp (c) (c->as->smmu->swgroup)
hwgrp-vs-swgrp inconsistency again.
Is this series git bisectable? I worry that by the time patch 14 is applied, the SMMU starts affecting client devices, whereas none of those client devices have swgroup values defined as their client data, and hence without this patch also applied, things might blow up in interesting ways.
I wonder why the code was ever using dev->platform_data in this way; the platform data for a device should have its structure/semantics defined by the driver for that device, not by an SMMU that happens to affect that device. Ick!
drivers/iommu/tegra-smmu.c static int __smmu_client_set_hwgrp(struct smmu_client *c, · line 404
Quoted 13 lines
drivers/iommu/tegra-smmu.c +3
-2static int __smmu_client_set_hwgrp (struct smmu_client *c, -unsigned long map,int on) +u64 map,int on) {int i;struct smmu_as *as = c->as;@@ -398,12 +400,11 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,if (!on) map =smmu_client_hwgrp (c); -for_each_set_bit (i, &map,HWGRP_COUNT ) { +for_each_set_bit (i, (unsigned long *)&map, +sizeof (map) *BITS_PER_BYTE ) {
Why change the type if it just forces you to add this cast?
drivers/iommu/tegra-smmu.c static int __smmu_client_set_hwgrp(struct smmu_client *c, · line 406
Quoted 5 lines
drivers/iommu/tegra-smmu.c +0
-2offs =HWGRP_ASID_REG (i); val =smmu_read (smmu, offs);if (on) { -if (WARN_ON (val & mask)) - goto err_hw_busy;
Why?
PATCH v2 16/22 iommu/tegra: smmu: Get "nvidia,swgroup" from DT
Stephen Warren <[email protected]> wrote @ Thu, 18 Jul 2013 22:28:42 +0200:
Quoted 6 lines · Hiroshi Doyu
Quoted 4 lines
This provides the info about which H/W Accelerators are supported on Tegra SoC. This info is passed from DT. This is necessary to have the unified SMMU driver among Tegra SoCs. Instead of using platform data, DT passes "nvidia,swgroup" now. DT is mandatory in Tegra.
Quoted 8 lines
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt Required properties: · line 13
Quoted 5 lines
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +3
-0+- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). + Each bit represents one swgroup. The assignments may be found in header + file <dt-bindings/memory/tegra-swgroup.h>.There needs to be a default for this field if one is not specified so that existing DTs continue to work without modification.
Only enabling PPCS(AHB) can be an option because PPCS has SD/MMC where rootfs can be located ususally.
Quoted 1 line
How many cells big is this property?
64
Quoted 2 lines
Is this really a bitmap of HWAs? Surely it's a bitmap of SMMU client IDs?
At least this info can be used for PMC too.
.....
Quoted 11 lines
drivers/iommu/tegra-smmu.c struct smmu_client { · line 268
Quoted 8 lines
drivers/iommu/tegra-smmu.c +1
-1@@ -265,7 +265,7 @@ struct smmu_client {struct device *dev;struct list_head list;struct smmu_as *as; -u32 hwgrp; +u64 hwgrp;Why is that "hwgrp" not "swgrp"? Don't they represent the same thing?
They are same but initial SMMU driver used the term "hwgroup". Should this be renamed with another patch or can it be left as it is?
....
Quoted 10 lines
Quoted 10 lines
drivers/iommu/tegra-smmu.c +1
-1static int __smmu_client_set_hwgrp (struct smmu_client *c, -unsigned long map,int on) +u64 map,int on) {drivers/iommu/tegra-smmu.c +0
-0int i;struct smmu_as *as = c->as;@@ -398,12 +400,11 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,if (!on) map =smmu_client_hwgrp (c);
Quoted 5 lines
drivers/iommu/tegra-smmu.c static int __smmu_client_set_hwgrp(struct smmu_client *c, · line 404
Quoted 3 lines
drivers/iommu/tegra-smmu.c +2
-1-for_each_set_bit (i, &map,HWGRP_COUNT ) { +for_each_set_bit (i, (unsigned long *)&map, +sizeof (map) *BITS_PER_BYTE ) {Why change the type if it just forces you to add this cast?
u32 map; -> u64 map;
for_each_set_bit() expects "unsigned long *" for any length of bitmap.
PATCH v2 16/22 iommu/tegra: smmu: Get "nvidia,swgroup" from DT
Quoted 14 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Thu, 18 Jul 2013 22:28:42 +0200:
Quoted 12 lines · Hiroshi Doyu
Quoted 10 lines
This provides the info about which H/W Accelerators are supported on Tegra SoC. This info is passed from DT. This is necessary to have the unified SMMU driver among Tegra SoCs. Instead of using platform data, DT passes "nvidia,swgroup" now. DT is mandatory in Tegra.
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +3
-0+- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). + Each bit represents one swgroup. The assignments may be found in header + file <dt-bindings/memory/tegra-swgroup.h>.
Quoted 5 lines
Quoted 2 lines
There needs to be a default for this field if one is not specified so that existing DTs continue to work without modification.
Only enabling PPCS(AHB) can be an option because PPCS has SD/MMC where rootfs can be located ususally.
There's no reason that the root filesystem has to be on SD/MMC. Either way, the DT binding shouldn't be influenced by the root fs location at all.
I think more explanation of exactly what this property does and why is required.
Quoted 3 lines
Quoted 1 line
How many cells big is this property?
64
I assume that's bits, so 2 cells? To be clear: the document needs to include this information, not just this email thread.
Quoted 4 lines
Quoted 2 lines
Is this really a bitmap of HWAs? Surely it's a bitmap of SMMU client IDs?
At least this info can be used for PMC too.
How and why? A complete explanation of how the SMMU and PMC are expected to interact is required.
The PMC DT binding should include all information related to the PMC; the binding definitions probably shouldn't expect a PMC driver to go grovelling in an SMMU node to find information.
Quoted 13 lines
.....
Quoted 11 lines
drivers/iommu/tegra-smmu.c struct smmu_client { · line 268
Quoted 8 lines
drivers/iommu/tegra-smmu.c +1
-1@@ -265,7 +265,7 @@ struct smmu_client {struct device *dev;struct list_head list;struct smmu_as *as; -u32 hwgrp; +u64 hwgrp;Why is that "hwgrp" not "swgrp"? Don't they represent the same thing?
Quoted 2 lines
They are same but initial SMMU driver used the term "hwgroup". Should this be renamed with another patch or can it be left as it is?
I thought there had already been a patch to do this rename. Was it not complete? If so, that work should probably be completed.
Quoted 17 lines
....
Quoted 10 lines
Quoted 10 lines
static int __smmu_client_set_hwgrp(struct
smmu_client*c,drivers/iommu/tegra-smmu.c +1
-1- unsigned long map, int on) + u64 map, int on) {int i;struct smmu_as *as = c->as;@@ -398,12 +400,11 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,if (!on) map =smmu_client_hwgrp (c);Quoted 5 lines
drivers/iommu/tegra-smmu.c static int __smmu_client_set_hwgrp(struct smmu_client *c, · line 404
Quoted 3 lines
drivers/iommu/tegra-smmu.c +2
-1-for_each_set_bit (i, &map,HWGRP_COUNT ) { +for_each_set_bit (i, (unsigned long *)&map, +sizeof (map) *BITS_PER_BYTE ) {Why change the type if it just forces you to add this cast?
Quoted 3 lines
u32 map; -> u64 map;
for_each_set_bit()expects "unsigned long *" for any length of bitmap.
Shouldn't the map just be an "unsigned long map[]" then, so no casts are needed anywhere? Equally, that pointer could just be passed to the function rather than copying the data to the stack?
PATCH v2 17/22 iommu/tegra: smmu: Unfied driver for Tegra SoCs
In reply to Hiroshi DoyuSupport multiple generation of Tegra SoCs with this unified SMMU driver. Necessary info is expected to be passed from DT so that this patch can remove obsolete code now.
--- drivers/iommu/tegra-smmu.c | 76 +------------------------------------------- 1 file changed, 1 insertion(+), 75 deletions(-)
drivers/iommu/tegra-smmu.c +1 -75
@@ -41,46 +41,6 @@#include <asm/cacheflush.h> #include <asm/dma-iommu.h> -enum smmu_hwgrp { -HWGRP_AFI , -HWGRP_AVPC , -HWGRP_DC , -HWGRP_DCB , -HWGRP_EPP , -HWGRP_G2 , -HWGRP_HC , -HWGRP_HDA , -HWGRP_ISP , -HWGRP_MPE , -HWGRP_NV , -HWGRP_NV2 , -HWGRP_PPCS , -HWGRP_SATA , -HWGRP_VDE , -HWGRP_VI , - -HWGRP_COUNT , - -HWGRP_END = ~0 , -}; - -#define HWG_AFI (1 << HWGRP_AFI) -#define HWG_AVPC (1 << HWGRP_AVPC) -#define HWG_DC (1 << HWGRP_DC) -#define HWG_DCB (1 << HWGRP_DCB) -#define HWG_EPP (1 << HWGRP_EPP) -#define HWG_G2 (1 << HWGRP_G2) -#define HWG_HC (1 << HWGRP_HC) -#define HWG_HDA (1 << HWGRP_HDA) -#define HWG_ISP (1 << HWGRP_ISP) -#define HWG_MPE (1 << HWGRP_MPE) -#define HWG_NV (1 << HWGRP_NV) -#define HWG_NV2 (1 << HWGRP_NV2) -#define HWG_PPCS (1 << HWGRP_PPCS) -#define HWG_SATA (1 << HWGRP_SATA) -#define HWG_VDE (1 << HWGRP_VDE) -#define HWG_VI (1 << HWGRP_VI) -/* bitmap of the page sizes currently supported */ #define SMMU_IOMMU_PGSIZES (SZ_4K)
@@ -150,21 +110,7 @@ enum {#define SMMU_TRANSLATION_ENABLE_2 0x230#define SMMU_AFI_ASID 0x238/* PCIE */ -#define SMMU_AVPC_ASID 0x23c/* AVP */ -#define SMMU_DC_ASID 0x240/* Display controller */ -#define SMMU_DCB_ASID 0x244/* Display controller B */ -#define SMMU_EPP_ASID 0x248/* Encoder pre-processor */ -#define SMMU_G2_ASID 0x24c/* 2D engine */ -#define SMMU_HC_ASID 0x250/* Host1x */ -#define SMMU_HDA_ASID 0x254/* High-def audio */ -#define SMMU_ISP_ASID 0x258/* Image signal processor */ -#define SMMU_MPE_ASID 0x264/* MPEG encoder */ -#define SMMU_NV_ASID 0x268/* (3D) */ -#define SMMU_NV2_ASID 0x26c/* (3D) */ -#define SMMU_PPCS_ASID 0x270/* AHB */ -#define SMMU_SATA_ASID 0x278/* SATA */ -#define SMMU_VDE_ASID 0x27c/* Video decoder */ -#define SMMU_VI_ASID 0x280/* Video input */ +#define SMMU_SWGRP_ASID_BASE SMMU_AFI_ASID#define SMMU_PDE_NEXT_SHIFT 28
@@ -236,26 +182,6 @@ enum {#define __smmu_client_enable_hwgrp (c, m) __smmu_client_set_hwgrp(c, m, 1)#define __smmu_client_disable_hwgrp (c) __smmu_client_set_hwgrp(c, 0, 0) -#define HWGRP_INIT (client) [HWGRP_##client] = SMMU_##client##_ASID - -static const u32 smmu_hwgrp_asid_reg[] = { -HWGRP_INIT (AFI ), -HWGRP_INIT (AVPC ), -HWGRP_INIT (DC ), -HWGRP_INIT (DCB ), -HWGRP_INIT (EPP ), -HWGRP_INIT (G2 ), -HWGRP_INIT (HC ), -HWGRP_INIT (HDA ), -HWGRP_INIT (ISP ), -HWGRP_INIT (MPE ), -HWGRP_INIT (NV ), -HWGRP_INIT (NV2 ), -HWGRP_INIT (PPCS ), -HWGRP_INIT (SATA ), -HWGRP_INIT (VDE ), -HWGRP_INIT (VI ), -};#define HWGRP_ASID_REG (x) ((x) * sizeof(u32) + SMMU_AFI_ASID)/*
Signature · 1 line
1.7.9.5
PATCH v2 18/22 iommu/tegra: smmu: Use dt-bindings MACRO
In reply to Hiroshi DoyuUse dt-bindings MACRO instead of SoC dependent MACROs.
--- drivers/iommu/tegra-smmu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/iommu/tegra-smmu.c +3 -1
@@ -41,6 +41,8 @@#include <asm/cacheflush.h> #include <asm/dma-iommu.h> +#include <dt-bindings/memory/tegra-swgroup.h> +/* bitmap of the page sizes currently supported */ #define SMMU_IOMMU_PGSIZES (SZ_4K)
@@ -744,7 +746,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,* Reserve "page zero" for AVP vectors using a common dummy * page. */ -if (map &HWG_AVPC ) { +if (map &TEGRA_SWGROUP_BIT (AVPC )) {struct page *page; page = as->smmu->avp_vector_page;
Signature · 1 line
1.7.9.5
PATCH v2 18/22 iommu/tegra: smmu: Use dt-bindings MACRO
drivers/iommu/tegra-smmu.c static int smmu_iommu_attach_dev(struct iommu_domain *domain, · line 749
Quoted 6 lines · Hiroshi Doyu
Use dt-bindings MACRO instead of SoC dependent MACROs.
drivers/iommu/tegra-smmu.c +1
-1-if (map &HWG_AVPC ) { +if (map &TEGRA_SWGROUP_BIT (AVPC )) {
Patch 17 removes the definition of HWG_AVPC. So, git bisect is broken.
PATCH v2 19/22 iommu/tegra: smmu: Workaround PCIe IOMMU'able
In reply to Hiroshi DoyuMake PCIe work as it is. IOMMU support can be implemented later. We need the same kind of registration framework for PCIe as platform_bus does currently.
--- drivers/iommu/tegra-smmu.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/iommu/tegra-smmu.c +3 -0
@@ -330,6 +330,9 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,for_each_set_bit (i, (unsigned long *)&map,sizeof (map) *BITS_PER_BYTE ) { +if (i ==TEGRA_SWGROUP_AFI )/* FIXME: IOMMU'able PCIe */ +continue ; + offs =HWGRP_ASID_REG (i); val =smmu_read (smmu, offs);if (on) {
Signature · 1 line
1.7.9.5
PATCH v2 19/22 iommu/tegra: smmu: Workaround PCIe IOMMU'able
drivers/iommu/tegra-smmu.c static int __smmu_client_set_hwgrp(struct smmu_client *c, · line 334
Quoted 10 lines · Hiroshi Doyu
Make PCIe work as it is. IOMMU support can be implemented later. We need the same kind of registration framework for PCIe as
platform_busdoes currently.drivers/iommu/tegra-smmu.c +2
-0for_each_set_bit (i, (unsigned long *)&map,sizeof (map) *BITS_PER_BYTE ) { +if (i ==TEGRA_SWGROUP_AFI )/* FIXME: IOMMU'able PCIe */ +continue ;
Isn't it just a fluke that TEGRA_SWGROUP_AFI is defined identically for the SoCs that we support upstream?
If this change is required, don't we need it before the SMMU starts enabling itself for clients, so that git bisect is maintained?
By deferring the question of registering the IOMMU for PCIe, ar we going to require non-backwards-compatible changes to DT for the PCIe controller at some later point in time?
PATCH v2 20/22 iommu/tegra: smmu: Get "nvidia, memory-client" from DT
In reply to Hiroshi DoyuThis provides the info about which swgroups a device belongs to. This info is passed from DT. This is necessary for the unified SMMU driver among Tegra SoCs since each has different H/W accelerators.
--- .../bindings/iommu/nvidia,tegra30-smmu.txt | 12 ++++++++++++ drivers/iommu/tegra-smmu.c | 20 +++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +12 -0
@@ -11,6 +11,7 @@ Required properties: - nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). Each bit represents one swgroup. The assignments may be found in header file <dt-bindings/memory/tegra-swgroup.h>. +- nvidia,memory-client: Indicates which swgroups a device belongs to. Example: iommu {
@@ -23,3 +24,14 @@ Example: nvidia,swgroups = TEGRA30_SWGROUP_ALL; nvidia,ahb = <&ahb>; }; + + host1x { + compatible = "nvidia,tegra30-host1x", "simple-bus"; + nvidia,memory-client = <TEGRA_SWGROUP_HC>; + .... + gr3d { + compatible = "nvidia,tegra30-gr3d"; + nvidia,memory-client = <TEGRA_SWGROUP_NV + TEGRA_SWGROUP_NV2>; + .... + };
drivers/iommu/tegra-smmu.c +19 -1
@@ -314,6 +314,24 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)#define smmu_client_hwgrp (c) (c->as->smmu->swgroup) +static u64 smmu_of_get_memory_client (struct device *dev) +{ +size_t bytes; +const char *propname ="nvidia,memory-client" ; +const __be32 *prop; +int i; +u64 swgroup =0 ; + + prop =of_get_property (dev->of_node, propname, &bytes); +if (!prop || !bytes) +return 0 ; + +for (i =0 ; i < bytes /sizeof (u32); i++, prop++) + swgroup |=1ULL <<be32_to_cpup (prop); + +return swgroup; +} +static int __smmu_client_set_hwgrp (struct smmu_client *c,u64 map,int on) {
@@ -725,7 +743,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,return -ENOMEM ; client->dev = dev; client->as = as; - map = smmu->swgroup; + map =smmu_of_get_memory_client (dev);if (!map)return -EINVAL ;
Signature · 1 line
1.7.9.5
PATCH v2 20/22 iommu/tegra: smmu: Get "nvidia,memory-client" from DT
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt Required properties: · line 14
Quoted 11 lines · Hiroshi Doyu
This provides the info about which swgroups a device belongs to. This info is passed from DT. This is necessary for the unified SMMU driver among Tegra SoCs since each has different H/W accelerators.
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +1
-0@@ -11,6 +11,7 @@ Required properties: - nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA). Each bit represents one swgroup. The assignments may be found in header file <dt-bindings/memory/tegra-swgroup.h>. +- nvidia,memory-client: Indicates which swgroups a device belongs to.
The list of properties you're adding to is for the SMMU node itself. If specific properties need to exist in SMMU client nodes, you need to create a separate section/list in the document for that. For example, see the clear distinction between sections (1) and (2) in Documentation/devicetree/bindings/gpio/gpio.txt.
What is the format/size of data in this property?
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt Example: · line 35
Quoted 4 lines
From the example below:
Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt +2
-0+ nvidia,memory-client = <TEGRA_SWGROUP_NV + TEGRA_SWGROUP_NV2>;
... it seems like multiple entries are allowed. Should the property be named "nvidia,memory-clients" instead (a plural)? "memory-clients" seems like new terminology that exists just for this property. Is "swgroups" the correct term here?
drivers/iommu/tegra-smmu.c static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs) · line 330
Quoted 6 lines
drivers/iommu/tegra-smmu.c +3
-0+static u64 smmu_of_get_memory_client (struct device *dev) + for (i = 0; i < bytes / sizeof(u32); i++, prop++) + swgroup |=1ULL <<be32_to_cpup (prop);
Oh, so it's a list of bit numbers instead of a list of masks. Definitely good to specify in the binding doc!
PATCH v2 21/22 iommu/tegra: smmu: Support Multiple ASID
In reply to Hiroshi DoyuSupport Multiple Address Space(AS). Tegra SMMU can have multiple ASes. We reserve 2 of them for static assignment, AS[0] for system default, AS[1] for AHB clusters as protected domain from others, where there are many traditional pheripheral devices like USB, SD/MMC. They should be isolated from some smart devices like host1x for system robustness. Even if smart devices behaves wrongly, the traditional devices(SD/MMC, USB) wouldn't be affected, and the system could continue most likely.
--- drivers/iommu/tegra-smmu.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/iommu/tegra-smmu.c +13 -2
@@ -904,7 +904,18 @@ enum {static int smmu_iommu_add_device (struct device *dev) {int err; -struct dma_iommu_mapping *map = smmu_handle->map[SYSTEM_DEFAULT ]; +u64 swgroup; +struct dma_iommu_mapping *map =NULL ; + + swgroup =smmu_of_get_memory_client (dev); +switch (swgroup) { +case TEGRA_SWGROUP_BIT (PPCS ): + map = smmu_handle->map[SYSTEM_PROTECTED ]; +break ; +default : + map = smmu_handle->map[SYSTEM_DEFAULT ]; +break ; + }if (!map) goto out;
@@ -915,7 +926,7 @@ static int smmu_iommu_add_device(struct device *dev)return err; } out: -dev_dbg (dev,"Attached to map %p\n" , map); +dev_dbg (dev,"Attached to map %p, swgroup:%08llx\n" , map, swgroup);return 0 ; }
Signature · 1 line
1.7.9.5
PATCH v2 21/22 iommu/tegra: smmu: Support Multiple ASID
Quoted 15 lines · Hiroshi Doyu
Support Multiple Address Space(AS). Tegra SMMU can have multiple ASes. We reserve 2 of them for static assignment, AS[0] for system default, AS[1] for AHB clusters as protected domain from others, where there are many traditional pheripheral devices like USB, SD/MMC. They should be isolated from some smart devices like host1x for system robustness. Even if smart devices behaves wrongly, the traditional
devices(SD/MMC, USB)wouldn't be affected, and the system could continue most likely.drivers/iommu/tegra-smmu.c +0
-1static int smmu_iommu_add_device (struct device *dev) {int err; -struct dma_iommu_mapping *map = smmu_handle->map[SYSTEM_DEFAULT ];
drivers/iommu/tegra-smmu.c enum { · line 916
Quoted 10 lines
drivers/iommu/tegra-smmu.c +10
-0+u64 swgroup; +struct dma_iommu_mapping *map =NULL ; + + swgroup =smmu_of_get_memory_client (dev); +switch (swgroup) { +case TEGRA_SWGROUP_BIT (PPCS ): + map = smmu_handle->map[SYSTEM_PROTECTED ]; +break ; +default : + map = smmu_handle->map[SYSTEM_DEFAULT ];
I already mentioned this, but just for completeness: What is smmu->num_as <= SYSTEM_DEFAULT?
PATCH v2 21/22 iommu/tegra: smmu: Support Multiple ASID
Stephen Warren <[email protected]> wrote @ Thu, 18 Jul 2013 22:43:06 +0200:
Quoted 21 lines · Hiroshi Doyu
Quoted 20 lines
Support Multiple Address Space(AS). Tegra SMMU can have multiple ASes. We reserve 2 of them for static assignment, AS[0] for system default, AS[1] for AHB clusters as protected domain from others, where there are many traditional pheripheral devices like USB, SD/MMC. They should be isolated from some smart devices like host1x for system robustness. Even if smart devices behaves wrongly, the traditional
devices(SD/MMC, USB)wouldn't be affected, and the system could continue most likely.drivers/iommu/tegra-smmu.c +5
-1static int smmu_iommu_add_device (struct device *dev) {int err; -struct dma_iommu_mapping *map = smmu_handle->map[SYSTEM_DEFAULT ]; +u64 swgroup; +struct dma_iommu_mapping *map =NULL ; + + swgroup =smmu_of_get_memory_client (dev); +switch (swgroup) {
Quoted 8 lines
drivers/iommu/tegra-smmu.c enum { · line 916
Quoted 5 lines
drivers/iommu/tegra-smmu.c +5
-0+case TEGRA_SWGROUP_BIT (PPCS ): + map = smmu_handle->map[SYSTEM_PROTECTED ]; +break ; +default : + map = smmu_handle->map[SYSTEM_DEFAULT ];I already mentioned this, but just for completeness: What is
smmu->num_as<=SYSTEM_DEFAULT?
I think that this belongs to the system operation policy. Which H/W should be configured to which Address Space(AS). This put all AHB clients(PPCS) into AS[1](SYSTEM_PROTECTED), and the rest into AS[0](SYSTEM_DEFAULT). AHB clients are mainly traditional H/Ws like USB and SD/MMC so that they should be kept separated from the smart IOMMU clients like host1x.
Is there any place to configure this kind of board specific policy rather than here?
PATCH v2 21/22 iommu/tegra: smmu: Support Multiple ASID
Quoted 39 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Thu, 18 Jul 2013 22:43:06 +0200:
Quoted 21 lines · Hiroshi Doyu
Quoted 20 lines
Support Multiple Address Space(AS). Tegra SMMU can have multiple ASes. We reserve 2 of them for static assignment, AS[0] for system default, AS[1] for AHB clusters as protected domain from others, where there are many traditional pheripheral devices like USB, SD/MMC. They should be isolated from some smart devices like host1x for system robustness. Even if smart devices behaves wrongly, the traditional
devices(SD/MMC, USB)wouldn't be affected, and the system could continue most likely.drivers/iommu/tegra-smmu.c +5
-1static int smmu_iommu_add_device (struct device *dev) {int err; -struct dma_iommu_mapping *map = smmu_handle->map[SYSTEM_DEFAULT ]; +u64 swgroup; +struct dma_iommu_mapping *map =NULL ; + + swgroup =smmu_of_get_memory_client (dev); +switch (swgroup) {Quoted 8 lines
drivers/iommu/tegra-smmu.c enum { · line 916
Quoted 5 lines
drivers/iommu/tegra-smmu.c +5
-0+case TEGRA_SWGROUP_BIT (PPCS ): + map = smmu_handle->map[SYSTEM_PROTECTED ]; +break ; +default : + map = smmu_handle->map[SYSTEM_DEFAULT ];I already mentioned this, but just for completeness: What is
smmu->num_as<=SYSTEM_DEFAULT?I think that this belongs to the system operation policy. Which H/W should be configured to which Address Space(AS). This put all AHB clients(PPCS) into AS[1](SYSTEM_PROTECTED), and the rest into AS[0](SYSTEM_DEFAULT). AHB clients are mainly traditional H/Ws like USB and SD/MMC so that they should be kept separated from the smart IOMMU clients like host1x.
Quoted 2 lines
Is there any place to configure this kind of board specific policy rather than here?
I'm not sure that answers the question I asked.
I mean that the driver expects that two AS always exist; SYSTEM_PROTECTED and SYSTEM_DEFAULT. However, the set of extant ASs is IIRC defined by the DT content. What if the DT doesn't define two ASs? Shouldn't there at least be an error-check for this case, so the driver doesn't just blindly continue and access smmu_handle->map[1] when the map[] array only has 1 entry?
PATCH v2 21/22 iommu/tegra: smmu: Support Multiple ASID
On Mon, 29 Jul 2013 19:41:23 +0200
Stephen Warren <[email protected]> wrote:
...
Quoted 10 lines
Quoted 9 lines
I think that this belongs to the system operation policy. Which H/W should be configured to which Address Space(AS). This put all AHB clients(PPCS) into AS[1](SYSTEM_PROTECTED), and the rest into AS[0](SYSTEM_DEFAULT). AHB clients are mainly traditional H/Ws like USB and SD/MMC so that they should be kept separated from the smart IOMMU clients like host1x.
Is there any place to configure this kind of board specific policy rather than here?
Quoted 8 lines
I'm not sure that answers the question I asked.
I mean that the driver expects that two AS always exist;
SYSTEM_PROTECTEDandSYSTEM_DEFAULT. However, the set of extant ASs is IIRC defined by the DT content. What if the DT doesn't define two ASs? Shouldn't there at least be an error-check for this case, so the driver doesn't just blindly continue and access smmu_handle->map[1] when the map[] array only has 1 entry?
OK, now I got it.
We can create multiple maps on the same AS in theory. So the limitation is only for how many maps S/W creaed. This can/should be checked only within kernel side. No constraints from DT.
PATCH v2 22/22 ARM: dma-mapping: Drop GFP_COMP for DMA memory allocations
In reply to Hiroshi Doyuarm_iommu_alloc_attrs wants to split pages after allocation in order to reduce the memory footprint. This does not work well with GFP_COMP pages, so drop this flag before allocation.
(ref: ea2e7057c0234cfb8b09467d8f137760d371fc72)
--- arch/arm/mm/dma-mapping.c | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm/mm/dma-mapping.c +7 -0
@@ -1322,6 +1322,13 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,struct page **pages;void *addr =NULL ; +/* Following is a work-around (a.k.a. hack) to prevent pages +* with __GFP_COMP being passed to split_page() which cannot +* handle them. The real problem is that this flag probably +* should be 0 on ARM as it is not supported on this +* platform--see CONFIG_HUGETLB_PAGE. */ + gfp &= ~(__GFP_COMP); + *handle =DMA_ERROR_CODE ; size =PAGE_ALIGN (size);
Signature · 1 line
1.7.9.5
PATCH v2 22/22 ARM: dma-mapping: Drop GFP_COMP for DMA memory allocations
Quoted 5 lines · Hiroshi Doyu
arm_iommu_alloc_attrswants to split pages after allocation in order to reduce the memory footprint. This does not work well withGFP_COMPpages, so drop this flag before allocation.(ref:
ea2e7057c0234cfb8b09467d8f137760d371fc72)
Include the commit description too, so people have a clue what that commit is.
Presumably this fix can go through the core ARM tree independently from this series? Or, will this series cause problems if applied without this patch being applied first?
BTW, I don't see anything in this series that adds the mandatory properties to DT for all this to work. I assume if I just apply this series, everything will blow up? If so, it's usual to say "[PATCH RFC" rather than "[PATCH" in the email subject.
v2 cover Unified SMMU driver among Tegra SoCs
In reply to Hiroshi DoyuAm Freitag, 5. Juli 2013, 13:44:35 schrieb Hiroshi Doyu:
Quoted 5 lines
Hi,
This series provides:
(1) Unified SMMU driver among Tegra SoCs
does this affect tegra2 in any way?
Quoted 60 lines
(2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
(3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.There's some discussion[1] about device population order, which can solve the following patches.
[HACK] of:
dev_nodehas struct device pointer
ARM: tegra: Populate AHB/IOMMU earlier than othersAlso "ARM: dma-mapping: Drop
GFP_COMPfor DMA memory allocations" may not be necessary by [2]Tested IOMMU functionality with T30 SD/MMC. Any further testing with T114 and/or other devices would be really appreciated.
v2:
Updated based on Thierry Reding's and Stephen Warren's feedbackv1:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/180267.htmlHiroshi Doyu (22):
[HACK] of:dev_nodehas struct device pointer
ARM: tegra: Populate AHB/IOMMU earlier than others
ARM: tegra: Create a DT header defining swgroups ID
ARM: dt: tegra30: iommu: Add "nvidia,swgroup"
ARM: dt: tegra30: iommu: Add "nvidia,memory-client"
ARM: dt: tegra114: iommu: Fix IOMMU register address
ARM: dt: tegra114: iommu: Add "nvidia,swgroups"
ARM: dt: tegra114: Add "nvidia,memory-client"
amba: Move AHB tocore_initcall
iommu/tegra: smmu: Move IOMMU tocore_initcalliommu/tegra: smmu: Add Tegra 114 support
iommu/tegra: smmu: SelectARM_DMA_USE_IOMMUinKconfigiommu/tegra: smmu: Create default IOVA maps iommu/tegra: smmu: Registerplatform_deviceto IOMMU dynamically iommu/tegra: smmu: Calculate ASID register offset by ID iommu/tegra: smmu: Get "nvidia,swgroup" from DT iommu/tegra: smmu: Unfied driver for Tegra SoCs iommu/tegra: smmu: Use dt-bindings MACRO
iommu/tegra: smmu: Workaround PCIe IOMMU'able iommu/tegra: smmu: Get "nvidia,memory-client" from DT iommu/tegra: smmu: Support Multiple ASID
ARM: dma-mapping: DropGFP_COMPfor DMA memory allocations.../bindings/iommu/nvidia,tegra30-smmu.txt | 20 +- arch/arm/boot/dts/tegra114.dtsi | 13 +- arch/arm/boot/dts/tegra30.dtsi | 21 ++ arch/arm/mach-tegra/tegra.c | 22 ++ arch/arm/mm/dma-mapping.c | 7 + drivers/amba/tegra-ahb.c | 7 +- drivers/iommu/Kconfig | 1 + drivers/iommu/tegra-smmu.c | 211 +++++++++++--------- drivers/of/base.c | 23 +++ drivers/of/platform.c | 8 + include/dt-bindings/memory/tegra-swgroup.h | 50 +++++ include/linux/of.h | 16 ++ 12 files changed, 296 insertions(+), 103 deletions(-) create mode 100644 include/dt-bindings/memory/tegra-swgroup.h
v2 cover Unified SMMU driver among Tegra SoCs
Hi Marc,
Marc Dietrich <[email protected]> wrote @ Fri, 5 Jul 2013 13:03:58 +0200:
Quoted 8 lines
Am Freitag, 5. Juli 2013, 13:44:35 schrieb Hiroshi Doyu:
Quoted 5 lines
Hi,
This series provides:
(1) Unified SMMU driver among Tegra SoCs
does this affect tegra2 in any way?
No, at all for Tegra 20.
This driver should work with Tegra 30, 114 and 148.
A message that is not in the archive (sent off-list or not yet received): 1373021097-32420-1-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
Hiroshi Doyu <[email protected]> wrote @ Fri, 5 Jul 2013 12:44:35 +0200:
Quoted 11 lines
Hi,
This series provides:
(1) Unified SMMU driver among Tegra SoCs
(2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
(3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.There's some discussion[1] about device population order, which can solve the following patches.
Quoted 8 lines
[HACK] of:
dev_nodehas struct device pointer
ARM: tegra: Populate AHB/IOMMU earlier than othersAlso "ARM: dma-mapping: Drop
GFP_COMPfor DMA memory allocations" may not be necessary by [2]Tested IOMMU functionality with T30 SD/MMC. Any further testing with T114 and/or other devices would be really appreciated.
For tegra device driver maintainers, please try this series to make your device work with IOMMU enabled. I guess that some of drivers may need to set its swgroup ID correctly in DT, and also you may need to check the usage of DMA mapping API correctly. For your convenience, you can get this series by git fetch:
git://nv-tegra.nvidia.com/user/hdoyu/linux.git smmu-upstreaming@20130705
Although I will not be always online for a few weeks, any feedback would be really appreciated.
A message that is not in the archive (sent off-list or not yet received): 20130708.114736.1280783845180530098.hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
Quoted 19 lines · Hiroshi Doyu
Hiroshi Doyu <[email protected]> wrote @ Fri, 5 Jul 2013 12:44:35 +0200:
Quoted 17 lines
Hi,
This series provides:
(1) Unified SMMU driver among Tegra SoCs
(2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
(3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.There's some discussion[1] about device population order, which can solve the following patches.
[HACK] of:
dev_nodehas struct device pointer
ARM: tegra: Populate AHB/IOMMU earlier than othersAlso "ARM: dma-mapping: Drop
GFP_COMPfor DMA memory allocations" may not be necessary by [2]
Quoted 8 lines
Quoted 2 lines
Tested IOMMU functionality with T30 SD/MMC. Any further testing with T114 and/or other devices would be really appreciated.
For tegra device driver maintainers, please try this series to make your device work with IOMMU enabled. I guess that some of drivers may need to set its swgroup ID correctly in DT, and also you may need to check the usage of DMA mapping API correctly. For your convenience, you can get this series by git fetch:
So are you saying this series will break some device drivers unless some driver changes and/or DT additions are made? If so, those changes should be (an early) part of this series; we can't break functionality.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Stephen Warren <[email protected]> wrote @ Mon, 8 Jul 2013 17:49:37 +0200:
Quoted 9 lines
Quoted 5 lines
For tegra device driver maintainers, please try this series to make your device work with IOMMU enabled. I guess that some of drivers may need to set its swgroup ID correctly in DT, and also you may need to check the usage of DMA mapping API correctly. For your convenience, you can get this series by git fetch:
So are you saying this series will break some device drivers unless some driver changes and/or DT additions are made? If so, those changes should be (an early) part of this series; we can't break functionality.
Agree. Do we have any list of tests to run, especially for device drivers?
A message that is not in the archive (sent off-list or not yet received): 20130708.201148.1964850060334610089.hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
Quoted 8 lines · Hiroshi Doyu
Stephen Warren <[email protected]> wrote @ Mon, 8 Jul 2013 17:49:37 +0200:
Quoted 6 lines
Quoted 5 lines
For tegra device driver maintainers, please try this series to make your device work with IOMMU enabled. I guess that some of drivers may need to set its swgroup ID correctly in DT, and also you may need to check the usage of DMA mapping API correctly. For your convenience, you can get this series by git fetch:
Quoted 5 lines
Quoted 3 lines
So are you saying this series will break some device drivers unless some driver changes and/or DT additions are made? If so, those changes should be (an early) part of this series; we can't break functionality.
Agree. Do we have any list of tests to run, especially for device drivers?
The best bet is to look through arch/arm/boot/dts/tegra*.dtsi, find the list of all Tegra devices, and test each driver, on all chips.
I'll also send you a link to our internal Wiki with the test-cases I typically run.
Hiroshi Doyu <[email protected]> wrote @ Mon, 08 Jul 2013 11:47:36 +0300 (EEST):
Quoted 22 lines
Hiroshi Doyu <[email protected]> wrote @ Fri, 5 Jul 2013 12:44:35 +0200:
Quoted 11 lines
Hi,
This series provides:
(1) Unified SMMU driver among Tegra SoCs
(2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
(3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.There's some discussion[1] about device population order, which can solve the following patches.
Quoted 8 lines
[HACK] of:
dev_nodehas struct device pointer
ARM: tegra: Populate AHB/IOMMU earlier than othersAlso "ARM: dma-mapping: Drop
GFP_COMPfor DMA memory allocations" may not be necessary by [2]Tested IOMMU functionality with T30 SD/MMC. Any further testing with T114 and/or other devices would be really appreciated.
Quoted 7 lines
For tegra device driver maintainers, please try this series to make your device work with IOMMU enabled. I guess that some of drivers may need to set its swgroup ID correctly in DT, and also you may need to check the usage of DMA mapping API correctly. For your convenience, you can get this series by git fetch:
git://nv-tegra.nvidia.com/user/hdoyu/linux.gitsmmu-upstreaming@20130705
Sorry for the delay of v3, I'm preparing v3. I will send them out early Oct.