Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1513360 > unrolled thread

Re: [PATCH v6 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Robin Murphy <robin.murphy@arm.com> - 2016-11-01 17:40 +0100
    Re: [PATCH v6 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-11-07 12:00 +0100

#1513360 — Re: [PATCH v6 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

Bikeshed alert...

On 18/10/16 17:04, Lorenzo Pieralisi wrote:
> The of_iommu_{set/get}_ops() API is used to associate a device
> tree node with a specific set of IOMMU operations. The same
> kernel interface is required on systems booting with ACPI, where
> devices are not associated with a device tree node, therefore
> the interface requires generalization.
> 
> The struct device fwnode member represents the fwnode token
> associated with the device and the struct it points at is firmware
> specific; regardless, it is initialized on both ACPI and DT systems
> and makes an ideal candidate to use it to associate a set of IOMMU
> operations to a given device, through its struct device.fwnode member
> pointer.
> 
> Convert the DT specific of_iommu_{set/get}_ops() interface to
> use struct device.fwnode as a look-up token, making the interface
> usable on ACPI systems.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> ---
>  drivers/iommu/iommu.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/iommu/of_iommu.c | 39 ---------------------------------------
>  include/linux/iommu.h    | 14 ++++++++++++++
>  include/linux/of_iommu.h | 12 ++++++++++--
>  4 files changed, 67 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9a2f196..320eb8c 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1615,6 +1615,49 @@ int iommu_request_dm_for_dev(struct device *dev)
>  	return ret;
>  }
>  
> +struct fwnode_iommu_node {

Having just pulled in this patch in isolation for some hacking, I
realise that by about the fifth time one reads "fwnode_iommu_node" it
just looks like meaningless gibberish. Can we just call it
"iommu_instance" instead, as that's what it's representing here?

> +	struct list_head list;
> +	struct fwnode_handle *fwnode;
> +	const struct iommu_ops *ops;
> +};
> +static LIST_HEAD(fwnode_iommu_list);
> +static DEFINE_SPINLOCK(fwnode_iommu_lock);
> +
> +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> +			  const struct iommu_ops *ops)
> +{
> +	struct fwnode_iommu_node *iommu =
> +				kzalloc(sizeof(*iommu), GFP_KERNEL);

(plus it shortens this line so it really doesn't the awkward break)

Apologies, (the original rubbish name was my fault anyway)
Robin.

> +
> +	if (WARN_ON(!iommu))
> +		return;
> +
> +	if (is_of_node(fwnode))
> +		of_node_get(to_of_node(fwnode));
> +
> +	INIT_LIST_HEAD(&iommu->list);
> +	iommu->fwnode = fwnode;
> +	iommu->ops = ops;
> +	spin_lock(&fwnode_iommu_lock);
> +	list_add_tail(&iommu->list, &fwnode_iommu_list);
> +	spin_unlock(&fwnode_iommu_lock);
> +}
> +
> +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_iommu_node *node;
> +	const struct iommu_ops *ops = NULL;
> +
> +	spin_lock(&fwnode_iommu_lock);
> +	list_for_each_entry(node, &fwnode_iommu_list, list)
> +		if (node->fwnode == fwnode) {
> +			ops = node->ops;
> +			break;
> +		}
> +	spin_unlock(&fwnode_iommu_lock);
> +	return ops;
> +}
> +
>  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>  		      const struct iommu_ops *ops)
>  {
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 5b82862..0f57ddc 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
>  }
>  EXPORT_SYMBOL_GPL(of_get_dma_window);
>  
> -struct of_iommu_node {
> -	struct list_head list;
> -	struct device_node *np;
> -	const struct iommu_ops *ops;
> -};
> -static LIST_HEAD(of_iommu_list);
> -static DEFINE_SPINLOCK(of_iommu_lock);
> -
> -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
> -{
> -	struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> -
> -	if (WARN_ON(!iommu))
> -		return;
> -
> -	of_node_get(np);
> -	INIT_LIST_HEAD(&iommu->list);
> -	iommu->np = np;
> -	iommu->ops = ops;
> -	spin_lock(&of_iommu_lock);
> -	list_add_tail(&iommu->list, &of_iommu_list);
> -	spin_unlock(&of_iommu_lock);
> -}
> -
> -const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> -{
> -	struct of_iommu_node *node;
> -	const struct iommu_ops *ops = NULL;
> -
> -	spin_lock(&of_iommu_lock);
> -	list_for_each_entry(node, &of_iommu_list, list)
> -		if (node->np == np) {
> -			ops = node->ops;
> -			break;
> -		}
> -	spin_unlock(&of_iommu_lock);
> -	return ops;
> -}
> -
>  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	struct of_phandle_args *iommu_spec = data;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 436dc21..15d5478 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -351,6 +351,9 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>  		      const struct iommu_ops *ops);
>  void iommu_fwspec_free(struct device *dev);
>  int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> +			  const struct iommu_ops *ops);
> +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode);
>  
>  #else /* CONFIG_IOMMU_API */
>  
> @@ -580,6 +583,17 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
>  	return -ENODEV;
>  }
>  
> +static inline void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> +					const struct iommu_ops *ops)
> +{
> +}
> +
> +static inline
> +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
> +{
> +	return NULL;
> +}
> +
>  #endif /* CONFIG_IOMMU_API */
>  
>  #endif /* __LINUX_IOMMU_H */
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index e80b9c7..7681007 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -31,8 +31,16 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
>  
>  #endif	/* CONFIG_OF_IOMMU */
>  
> -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
> -const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
> +static inline void of_iommu_set_ops(struct device_node *np,
> +				    const struct iommu_ops *ops)
> +{
> +	fwnode_iommu_set_ops(&np->fwnode, ops);
> +}
> +
> +static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> +{
> +	return fwnode_iommu_get_ops(&np->fwnode);
> +}
>  
>  extern struct of_device_id __iommu_of_table;
>  
> 

[toc] | [next] | [standalone]

#1516002

On Tue, Nov 01, 2016 at 04:36:10PM +0000, Robin Murphy wrote:
> Bikeshed alert...
> 
> On 18/10/16 17:04, Lorenzo Pieralisi wrote:
> > The of_iommu_{set/get}_ops() API is used to associate a device
> > tree node with a specific set of IOMMU operations. The same
> > kernel interface is required on systems booting with ACPI, where
> > devices are not associated with a device tree node, therefore
> > the interface requires generalization.
> > 
> > The struct device fwnode member represents the fwnode token
> > associated with the device and the struct it points at is firmware
> > specific; regardless, it is initialized on both ACPI and DT systems
> > and makes an ideal candidate to use it to associate a set of IOMMU
> > operations to a given device, through its struct device.fwnode member
> > pointer.
> > 
> > Convert the DT specific of_iommu_{set/get}_ops() interface to
> > use struct device.fwnode as a look-up token, making the interface
> > usable on ACPI systems.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Hanjun Guo <hanjun.guo@linaro.org>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Joerg Roedel <joro@8bytes.org>
> > ---
> >  drivers/iommu/iommu.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  drivers/iommu/of_iommu.c | 39 ---------------------------------------
> >  include/linux/iommu.h    | 14 ++++++++++++++
> >  include/linux/of_iommu.h | 12 ++++++++++--
> >  4 files changed, 67 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 9a2f196..320eb8c 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1615,6 +1615,49 @@ int iommu_request_dm_for_dev(struct device *dev)
> >  	return ret;
> >  }
> >  
> > +struct fwnode_iommu_node {
> 
> Having just pulled in this patch in isolation for some hacking, I
> realise that by about the fifth time one reads "fwnode_iommu_node" it
> just looks like meaningless gibberish. Can we just call it
> "iommu_instance" instead, as that's what it's representing here?

Agreed, how about iommu_(fw)entry ? Anyway, I will do, point taken.

> > +	struct list_head list;
> > +	struct fwnode_handle *fwnode;
> > +	const struct iommu_ops *ops;
> > +};
> > +static LIST_HEAD(fwnode_iommu_list);
> > +static DEFINE_SPINLOCK(fwnode_iommu_lock);
> > +
> > +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> > +			  const struct iommu_ops *ops)
> > +{
> > +	struct fwnode_iommu_node *iommu =
> > +				kzalloc(sizeof(*iommu), GFP_KERNEL);
> 
> (plus it shortens this line so it really doesn't the awkward break)
> 
> Apologies, (the original rubbish name was my fault anyway)

No worries, you have a point, will update for next (hopefully last)
posting.

Thanks !
Lorenzo

> Robin.
> 
> > +
> > +	if (WARN_ON(!iommu))
> > +		return;
> > +
> > +	if (is_of_node(fwnode))
> > +		of_node_get(to_of_node(fwnode));
> > +
> > +	INIT_LIST_HEAD(&iommu->list);
> > +	iommu->fwnode = fwnode;
> > +	iommu->ops = ops;
> > +	spin_lock(&fwnode_iommu_lock);
> > +	list_add_tail(&iommu->list, &fwnode_iommu_list);
> > +	spin_unlock(&fwnode_iommu_lock);
> > +}
> > +
> > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
> > +{
> > +	struct fwnode_iommu_node *node;
> > +	const struct iommu_ops *ops = NULL;
> > +
> > +	spin_lock(&fwnode_iommu_lock);
> > +	list_for_each_entry(node, &fwnode_iommu_list, list)
> > +		if (node->fwnode == fwnode) {
> > +			ops = node->ops;
> > +			break;
> > +		}
> > +	spin_unlock(&fwnode_iommu_lock);
> > +	return ops;
> > +}
> > +
> >  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> >  		      const struct iommu_ops *ops)
> >  {
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 5b82862..0f57ddc 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
> >  }
> >  EXPORT_SYMBOL_GPL(of_get_dma_window);
> >  
> > -struct of_iommu_node {
> > -	struct list_head list;
> > -	struct device_node *np;
> > -	const struct iommu_ops *ops;
> > -};
> > -static LIST_HEAD(of_iommu_list);
> > -static DEFINE_SPINLOCK(of_iommu_lock);
> > -
> > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
> > -{
> > -	struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> > -
> > -	if (WARN_ON(!iommu))
> > -		return;
> > -
> > -	of_node_get(np);
> > -	INIT_LIST_HEAD(&iommu->list);
> > -	iommu->np = np;
> > -	iommu->ops = ops;
> > -	spin_lock(&of_iommu_lock);
> > -	list_add_tail(&iommu->list, &of_iommu_list);
> > -	spin_unlock(&of_iommu_lock);
> > -}
> > -
> > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> > -{
> > -	struct of_iommu_node *node;
> > -	const struct iommu_ops *ops = NULL;
> > -
> > -	spin_lock(&of_iommu_lock);
> > -	list_for_each_entry(node, &of_iommu_list, list)
> > -		if (node->np == np) {
> > -			ops = node->ops;
> > -			break;
> > -		}
> > -	spin_unlock(&of_iommu_lock);
> > -	return ops;
> > -}
> > -
> >  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
> >  {
> >  	struct of_phandle_args *iommu_spec = data;
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 436dc21..15d5478 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -351,6 +351,9 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> >  		      const struct iommu_ops *ops);
> >  void iommu_fwspec_free(struct device *dev);
> >  int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> > +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> > +			  const struct iommu_ops *ops);
> > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode);
> >  
> >  #else /* CONFIG_IOMMU_API */
> >  
> > @@ -580,6 +583,17 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
> >  	return -ENODEV;
> >  }
> >  
> > +static inline void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> > +					const struct iommu_ops *ops)
> > +{
> > +}
> > +
> > +static inline
> > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
> > +{
> > +	return NULL;
> > +}
> > +
> >  #endif /* CONFIG_IOMMU_API */
> >  
> >  #endif /* __LINUX_IOMMU_H */
> > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> > index e80b9c7..7681007 100644
> > --- a/include/linux/of_iommu.h
> > +++ b/include/linux/of_iommu.h
> > @@ -31,8 +31,16 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
> >  
> >  #endif	/* CONFIG_OF_IOMMU */
> >  
> > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
> > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
> > +static inline void of_iommu_set_ops(struct device_node *np,
> > +				    const struct iommu_ops *ops)
> > +{
> > +	fwnode_iommu_set_ops(&np->fwnode, ops);
> > +}
> > +
> > +static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> > +{
> > +	return fwnode_iommu_get_ops(&np->fwnode);
> > +}
> >  
> >  extern struct of_device_id __iommu_of_table;
> >  
> > 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web