Re: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

On 9/17/18 6:39 PM, Geert Uytterhoeven wrote:
> In some SoCs multiple hardware blocks may share a reset control.
> The existing reset control API for shared resets will only assert such a
> reset when the drivers for all hardware blocks agree.
> The existing exclusive reset control API still allows to assert such a
> reset, but that impacts all other hardware blocks sharing the reset.
> 
> Sometimes a driver needs to reset a specific hardware block, and be 100%
> sure it has no impact on other hardware blocks.  This is e.g. the case
> for virtualization with device pass-through, where the host wants to
> reset any exported device before and after exporting it for use by the
> guest, for isolation.
> 
> Hence a new flag for dedicated resets is added to the internal methods,
> with a new public reset_control_get_dedicated() method, to obtain an
> exclusive handle to a reset that is dedicated to one specific hardware
> block.
> 
> This supports both DT-based and lookup-based reset controls.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> ---
> v4:
>   - New.
> 
> Notes:
>   - Dedicated lookup-based reset controls were not tested,
>   - Several internal functions now take 3 boolean flags, and should
>     probably be converted to take a bitmask instead,
>   - I think __device_reset() should call __reset_control_get() with
>     dedicated=true.  However, that will impact existing users,
why should it?
>   - Should a different error than -EINVAL be returned on failure?
> ---
>  drivers/reset/core.c  | 76 ++++++++++++++++++++++++++++++++++++++-----
>  include/linux/reset.h | 60 ++++++++++++++++++++++------------
>  2 files changed, 107 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 225e34c56b94a2e3..5bc4eeca70c0fcc2 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -459,9 +459,38 @@ static void __reset_control_put_internal(struct reset_control *rstc)
>  	kref_put(&rstc->refcnt, __reset_control_release);
>  }
>  
> +static bool __of_reset_is_dedicated(const struct device_node *node,
> +				    const struct of_phandle_args args)
> +{
> +	struct of_phandle_args args2;
> +	struct device_node *node2;
> +	int index, ret;
> +
> +	for_each_node_with_property(node2, "resets") {
> +		if (node == node2)
> +			continue;
> +
> +		for (index = 0; ; index++) {
> +			ret = of_parse_phandle_with_args(node2, "resets",
> +							 "#reset-cells", index,
> +							 &args2);
> +			if (ret)
> +				break;
> +
> +			if (args2.np == args.np &&
> +			    args2.args_count == args.args_count &&
> +			    !memcmp(args2.args, args.args,
> +				    args.args_count * sizeof(args.args[0])))
> +				return false;
You need to call of_node_put(args2.np) (see of_parse_phandle_with_args
kernel doc)

Isn't it sufficient to check device_node handles are equal?

Thanks

Eric

> +		}
> +	}
> +
> +	return true;
> +}
> +
>  struct reset_control *__of_reset_control_get(struct device_node *node,
>  				     const char *id, int index, bool shared,
> -				     bool optional)
> +				     bool optional, bool dedicated)
>  {
>  	struct reset_control *rstc;
>  	struct reset_controller_dev *r, *rcdev;
> @@ -514,6 +543,11 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
>  		return ERR_PTR(rstc_id);
>  	}
>  
> +	if (dedicated && !__of_reset_is_dedicated(node, args)) {
> +		mutex_unlock(&reset_list_mutex);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	/* reset_list_mutex also protects the rcdev's reset_control list */
>  	rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
>  
> @@ -541,9 +575,25 @@ __reset_controller_by_name(const char *name)
>  	return NULL;
>  }
>  
> +static bool __reset_is_dedicated(const struct reset_control_lookup *lookup)
> +{
> +	const struct reset_control_lookup *lookup2;
> +
> +	list_for_each_entry(lookup, &reset_lookup_list, list) {
> +		if (lookup2 == lookup)
> +			continue;
> +
> +		if (lookup2->provider == lookup->provider &&
> +		    lookup2->index == lookup->index)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  static struct reset_control *
>  __reset_control_get_from_lookup(struct device *dev, const char *con_id,
> -				bool shared, bool optional)
> +				bool shared, bool optional, bool dedicated)
>  {
>  	const struct reset_control_lookup *lookup;
>  	struct reset_controller_dev *rcdev;
> @@ -562,6 +612,11 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
>  		if ((!con_id && !lookup->con_id) ||
>  		    ((con_id && lookup->con_id) &&
>  		     !strcmp(con_id, lookup->con_id))) {
> +			if (dedicated && !__reset_is_dedicated(lookup)) {
> +				mutex_unlock(&reset_lookup_mutex);
> +				return ERR_PTR(-EINVAL);
> +			}
> +
>  			mutex_lock(&reset_list_mutex);
>  			rcdev = __reset_controller_by_name(lookup->provider);
>  			if (!rcdev) {
> @@ -588,13 +643,15 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
>  }
>  
>  struct reset_control *__reset_control_get(struct device *dev, const char *id,
> -					  int index, bool shared, bool optional)
> +					  int index, bool shared,
> +					  bool optional, bool dedicated)
>  {
>  	if (dev->of_node)
>  		return __of_reset_control_get(dev->of_node, id, index, shared,
> -					      optional);
> +					      optional, dedicated);
>  
> -	return __reset_control_get_from_lookup(dev, id, shared, optional);
> +	return __reset_control_get_from_lookup(dev, id, shared, optional,
> +					       dedicated);
>  }
>  EXPORT_SYMBOL_GPL(__reset_control_get);
>  
> @@ -635,7 +692,7 @@ static void devm_reset_control_release(struct device *dev, void *res)
>  
>  struct reset_control *__devm_reset_control_get(struct device *dev,
>  				     const char *id, int index, bool shared,
> -				     bool optional)
> +				     bool optional, bool dedicated)
>  {
>  	struct reset_control **ptr, *rstc;
>  
> @@ -644,7 +701,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
>  	if (!ptr)
>  		return ERR_PTR(-ENOMEM);
>  
> -	rstc = __reset_control_get(dev, id, index, shared, optional);
> +	rstc = __reset_control_get(dev, id, index, shared, optional, dedicated);
>  	if (!IS_ERR(rstc)) {
>  		*ptr = rstc;
>  		devres_add(dev, ptr);
> @@ -671,7 +728,7 @@ int __device_reset(struct device *dev, bool optional)
>  	struct reset_control *rstc;
>  	int ret;
>  
> -	rstc = __reset_control_get(dev, NULL, 0, 0, optional);
> +	rstc = __reset_control_get(dev, NULL, 0, false, optional, false);
>  	if (IS_ERR(rstc))
>  		return PTR_ERR(rstc);
>  
> @@ -735,7 +792,8 @@ of_reset_control_array_get(struct device_node *np, bool shared, bool optional)
>  		return ERR_PTR(-ENOMEM);
>  
>  	for (i = 0; i < num; i++) {
> -		rstc = __of_reset_control_get(np, NULL, i, shared, optional);
> +		rstc = __of_reset_control_get(np, NULL, i, shared, optional,
> +					      false);
>  		if (IS_ERR(rstc))
>  			goto err_rst;
>  		resets->rstc[i] = rstc;
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 09732c36f3515a1e..6ca6e108b612f923 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -17,15 +17,15 @@ int reset_control_status(struct reset_control *rstc);
>  
>  struct reset_control *__of_reset_control_get(struct device_node *node,
>  				     const char *id, int index, bool shared,
> -				     bool optional);
> +				     bool optional, bool dedicated);
>  struct reset_control *__reset_control_get(struct device *dev, const char *id,
>  					  int index, bool shared,
> -					  bool optional);
> +					  bool optional, bool dedicated);
>  void reset_control_put(struct reset_control *rstc);
>  int __device_reset(struct device *dev, bool optional);
>  struct reset_control *__devm_reset_control_get(struct device *dev,
>  				     const char *id, int index, bool shared,
> -				     bool optional);
> +				     bool optional, bool dedicated);
>  
>  struct reset_control *devm_reset_control_array_get(struct device *dev,
>  						   bool shared, bool optional);
> @@ -66,21 +66,23 @@ static inline int __device_reset(struct device *dev, bool optional)
>  static inline struct reset_control *__of_reset_control_get(
>  					struct device_node *node,
>  					const char *id, int index, bool shared,
> -					bool optional)
> +					bool optional, bool dedicated)
>  {
>  	return optional ? NULL : ERR_PTR(-ENOTSUPP);
>  }
>  
>  static inline struct reset_control *__reset_control_get(
>  					struct device *dev, const char *id,
> -					int index, bool shared, bool optional)
> +					int index, bool shared, bool optional,
> +					bool dedicated)
>  {
>  	return optional ? NULL : ERR_PTR(-ENOTSUPP);
>  }
>  
>  static inline struct reset_control *__devm_reset_control_get(
>  					struct device *dev, const char *id,
> -					int index, bool shared, bool optional)
> +					int index, bool shared, bool optional,
> +					bool dedicated)
>  {
>  	return optional ? NULL : ERR_PTR(-ENOTSUPP);
>  }
> @@ -127,7 +129,25 @@ static inline int device_reset_optional(struct device *dev)
>  static inline struct reset_control *
>  __must_check reset_control_get_exclusive(struct device *dev, const char *id)
>  {
> -	return __reset_control_get(dev, id, 0, false, false);
> +	return __reset_control_get(dev, id, 0, false, false, false);
> +}
> +
> +/**
> + * reset_control_get_dedicated - Lookup and obtain an exclusive reference
> + *                               to a dedicated reset controller.
> + * @dev: device to be reset by the controller
> + * @id: reset line name
> + *
> + * Returns a struct reset_control or IS_ERR() condition containing errno.
> + * If this function is called more than once for the same reset_control it will
> + * return -EBUSY.
> + *
> + * Use of id names is optional.
> + */
> +static inline struct reset_control *
> +__must_check reset_control_get_dedicated(struct device *dev, const char *id)
> +{
> +	return __reset_control_get(dev, id, 0, false, false, true);
>  }
>  
>  /**
> @@ -155,19 +175,19 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
>  static inline struct reset_control *reset_control_get_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __reset_control_get(dev, id, 0, true, false);
> +	return __reset_control_get(dev, id, 0, true, false, false);
>  }
>  
>  static inline struct reset_control *reset_control_get_optional_exclusive(
>  					struct device *dev, const char *id)
>  {
> -	return __reset_control_get(dev, id, 0, false, true);
> +	return __reset_control_get(dev, id, 0, false, true, false);
>  }
>  
>  static inline struct reset_control *reset_control_get_optional_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __reset_control_get(dev, id, 0, true, true);
> +	return __reset_control_get(dev, id, 0, true, true, false);
>  }
>  
>  /**
> @@ -183,7 +203,7 @@ static inline struct reset_control *reset_control_get_optional_shared(
>  static inline struct reset_control *of_reset_control_get_exclusive(
>  				struct device_node *node, const char *id)
>  {
> -	return __of_reset_control_get(node, id, 0, false, false);
> +	return __of_reset_control_get(node, id, 0, false, false, false);
>  }
>  
>  /**
> @@ -208,7 +228,7 @@ static inline struct reset_control *of_reset_control_get_exclusive(
>  static inline struct reset_control *of_reset_control_get_shared(
>  				struct device_node *node, const char *id)
>  {
> -	return __of_reset_control_get(node, id, 0, true, false);
> +	return __of_reset_control_get(node, id, 0, true, false, false);
>  }
>  
>  /**
> @@ -225,7 +245,7 @@ static inline struct reset_control *of_reset_control_get_shared(
>  static inline struct reset_control *of_reset_control_get_exclusive_by_index(
>  					struct device_node *node, int index)
>  {
> -	return __of_reset_control_get(node, NULL, index, false, false);
> +	return __of_reset_control_get(node, NULL, index, false, false, false);
>  }
>  
>  /**
> @@ -253,7 +273,7 @@ static inline struct reset_control *of_reset_control_get_exclusive_by_index(
>  static inline struct reset_control *of_reset_control_get_shared_by_index(
>  					struct device_node *node, int index)
>  {
> -	return __of_reset_control_get(node, NULL, index, true, false);
> +	return __of_reset_control_get(node, NULL, index, true, false, false);
>  }
>  
>  /**
> @@ -272,7 +292,7 @@ static inline struct reset_control *
>  __must_check devm_reset_control_get_exclusive(struct device *dev,
>  					      const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, false, false);
> +	return __devm_reset_control_get(dev, id, 0, false, false, false);
>  }
>  
>  /**
> @@ -287,19 +307,19 @@ __must_check devm_reset_control_get_exclusive(struct device *dev,
>  static inline struct reset_control *devm_reset_control_get_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, true, false);
> +	return __devm_reset_control_get(dev, id, 0, true, false, false);
>  }
>  
>  static inline struct reset_control *devm_reset_control_get_optional_exclusive(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, false, true);
> +	return __devm_reset_control_get(dev, id, 0, false, true, false);
>  }
>  
>  static inline struct reset_control *devm_reset_control_get_optional_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, true, true);
> +	return __devm_reset_control_get(dev, id, 0, true, true, false);
>  }
>  
>  /**
> @@ -317,7 +337,7 @@ static inline struct reset_control *devm_reset_control_get_optional_shared(
>  static inline struct reset_control *
>  devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
>  {
> -	return __devm_reset_control_get(dev, NULL, index, false, false);
> +	return __devm_reset_control_get(dev, NULL, index, false, false, false);
>  }
>  
>  /**
> @@ -333,7 +353,7 @@ devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
>  static inline struct reset_control *
>  devm_reset_control_get_shared_by_index(struct device *dev, int index)
>  {
> -	return __devm_reset_control_get(dev, NULL, index, true, false);
> +	return __devm_reset_control_get(dev, NULL, index, true, false, false);
>  }
>  
>  /*
> 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux