[PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Pantelis Antoniou
pantelis.antoniou at konsulko.com
Tue Dec 2 02:30:37 PST 2014
Hi Rob,
> On Dec 2, 2014, at 01:54 , Rob Herring <robherring2 at gmail.com> wrote:
>
> Adding Grant and Pantelis...
>
> On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon <will.deacon at arm.com> wrote:
>> IOMMU drivers must be initialised before any of their upstream devices,
>> otherwise the relevant iommu_ops won't be configured for the bus in
>> question. To solve this, a number of IOMMU drivers use initcalls to
>> initialise the driver before anything has a chance to be probed.
>>
>> Whilst this solves the immediate problem, it leaves the job of probing
>> the IOMMU completely separate from the iommu_ops to configure the IOMMU,
>> which are called on a per-bus basis and require the driver to figure out
>> exactly which instance of the IOMMU is being requested. In particular,
>> the add_device callback simply passes a struct device to the driver,
>> which then has to parse firmware tables or probe buses to identify the
>> relevant IOMMU instance.
>>
>> This patch takes the first step in addressing this problem by adding an
>> early initialisation pass for IOMMU drivers, giving them the ability to
>> store some per-instance data in their iommu_ops structure and store that
>> in their of_node. This can later be used when parsing OF masters to
>> identify the IOMMU instance in question.
>>
>> Acked-by: Arnd Bergmann <arnd at arndb.de>
>> Acked-by: Joerg Roedel <jroedel at suse.de>
>> Acked-by: Marek Szyprowski <m.szyprowski at samsung.com>
>> Tested-by: Robin Murphy <robin.murphy at arm.com>
>> Signed-off-by: Will Deacon <will.deacon at arm.com>
>> ---
>> drivers/iommu/of_iommu.c | 17 +++++++++++++++++
>> include/asm-generic/vmlinux.lds.h | 2 ++
>> include/linux/iommu.h | 2 ++
>> include/linux/of_iommu.h | 25 +++++++++++++++++++++++++
>> 4 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index e550ccb7634e..89b903406968 100644
>> --- a/drivers/iommu/of_iommu.c
>> +++ b/drivers/iommu/of_iommu.c
>> @@ -22,6 +22,9 @@
>> #include <linux/of.h>
>> #include <linux/of_iommu.h>
>>
>> +static const struct of_device_id __iommu_of_table_sentinel
>> + __used __section(__iommu_of_table_end);
>> +
>> /**
>> * of_get_dma_window - Parse *dma-window property and returns 0 if found.
>> *
>> @@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(of_get_dma_window);
>> +
>> +void __init of_iommu_init(void)
>> +{
>> + struct device_node *np;
>> + const struct of_device_id *match, *matches = &__iommu_of_table;
>> +
>> + for_each_matching_node_and_match(np, matches, &match) {
>> + const of_iommu_init_fn init_fn = match->data;
>> +
>> + if (init_fn(np))
>> + pr_err("Failed to initialise IOMMU %s\n",
>> + of_node_full_name(np));
>> + }
>> +}
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index aa70cbda327c..bee5d683074d 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -164,6 +164,7 @@
>> #define CLKSRC_OF_TABLES() OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
>> #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
>> #define CLK_OF_TABLES() OF_TABLE(CONFIG_COMMON_CLK, clk)
>> +#define IOMMU_OF_TABLES() OF_TABLE(CONFIG_OF_IOMMU, iommu)
>> #define RESERVEDMEM_OF_TABLES() OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
>> #define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
>> #define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
>> @@ -497,6 +498,7 @@
>> CLK_OF_TABLES() \
>> RESERVEDMEM_OF_TABLES() \
>> CLKSRC_OF_TABLES() \
>> + IOMMU_OF_TABLES() \
>> CPU_METHOD_OF_TABLES() \
>> KERNEL_DTB() \
>> IRQCHIP_OF_MATCH_TABLE() \
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index e6a7c9ff72f2..7b83f9f8e11d 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -103,6 +103,7 @@ enum iommu_attr {
>> * @domain_get_attr: Query domain attributes
>> * @domain_set_attr: Change domain attributes
>> * @pgsize_bitmap: bitmap of supported page sizes
>> + * @priv: per-instance data private to the iommu driver
>> */
>> struct iommu_ops {
>> bool (*capable)(enum iommu_cap);
>> @@ -133,6 +134,7 @@ struct iommu_ops {
>> u32 (*domain_get_windows)(struct iommu_domain *domain);
>>
>> unsigned long pgsize_bitmap;
>> + void *priv;
>> };
>>
>> #define IOMMU_GROUP_NOTIFY_ADD_DEVICE 1 /* Device added */
>> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
>> index 51a560f34bca..5762cdc8effe 100644
>> --- a/include/linux/of_iommu.h
>> +++ b/include/linux/of_iommu.h
>> @@ -1,12 +1,17 @@
>> #ifndef __OF_IOMMU_H
>> #define __OF_IOMMU_H
>>
>> +#include <linux/iommu.h>
>> +#include <linux/of.h>
>> +
>> #ifdef CONFIG_OF_IOMMU
>>
>> extern int of_get_dma_window(struct device_node *dn, const char *prefix,
>> int index, unsigned long *busno, dma_addr_t *addr,
>> size_t *size);
>>
>> +extern void of_iommu_init(void);
>> +
>> #else
>>
>> static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
>> @@ -16,6 +21,26 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
>> return -EINVAL;
>> }
>>
>> +static inline void of_iommu_init(void) { }
>> +
>> #endif /* CONFIG_OF_IOMMU */
>>
>> +static inline void of_iommu_set_ops(struct device_node *np,
>> + const struct iommu_ops *ops)
>> +{
>> + np->data = (struct iommu_ops *)ops;
>> +}
>> +
>> +static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>> +{
>> + return np->data;
>> +}
>
> This may collide with other users. While use of it is rare, PPC uses
> it in its PCI code. The OF_DYNAMIC code frees it but never actually
> sets it. There may be some coming usage with the DT overlay code or
> that's just a bug. Pantelis or Grant can comment. If not, I think we
> really should try to get rid of this pointer rather than expand it's
> usage.
>
That assignment is not good as it is. Under OF_DYNAMIC on of_node_release it will attempt
to kfree it.
For now kdup’ing the ops structure should be enough, but I’d like to see this removed eventually.
This data pointer just has to go. As far as I can tell it is used in CELL PPC and in the
freescale timers but IMHO it’s just a bad idea to hang per-driver data in the device node.
It is simple enough to use an IDR or something when you need to grab data related to the
device node instead of stashing stuff in the data pointer.
> I didn't see a user of this. I'm guessing that is coming in a SMMU patch?
>
> Rob
Regards
— Pantelis
More information about the linux-arm-kernel
mailing list