Re: [PATCH 1/5] reset: add acquired/released state for exclusive reset controls

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6053 bytes --]

On Thu, Feb 21, 2019 at 04:28:58PM +0100, Thierry Reding wrote:
> On Thu, Feb 21, 2019 at 04:25:53PM +0100, Thierry Reding wrote:
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > There are cases where a driver needs explicit control over a reset line
> > that is exclusively conneted to its device, but this control has to be
> > temporarily handed over to the power domain controller to handle reset
> > requirements during power transitions.
> > Allow multiple exclusive reset controls to be requested in 'released'
> > state for the same physical reset line, only one of which can be
> > acquired at the same time.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/reset/core.c  | 139 ++++++++++++++++++++++++++++++++++++++----
> >  include/linux/reset.h |  93 ++++++++++++++++++++++------
> >  2 files changed, 200 insertions(+), 32 deletions(-)
> 
> Hi Philipp,
> 
> the bulk of this is unchanged relative to what you had posted
> originally. I squashed in the few things that we had already discussed
> earlier (EINVAL -> EPERM) and a couple of minor fixes for issues that I
> found while working with this.
> 
> Attached is my fixup patch which contains all the changes I made on top
> of your version and that I squashed into this.
> 
> Thierry

Hi Philipp,

do you have any further comments on this series?

Thierry

> --- >8 ---
> commit aa618d0b63eec676d9ea8db91a4c5fdc9330fc6b
> Author: Thierry Reding <treding@nvidia.com>
> Date:   Mon Feb 18 11:32:46 2019 +0100
> 
>     fixup! reset: add acquired/released state for exclusive reset controls
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index c6a7a4474142..1e8a42b16f23 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -65,6 +65,17 @@ struct reset_control_array {
>  	struct reset_control *rstc[];
>  };
>  
> +static const char *rcdev_name(struct reset_controller_dev *rcdev)
> +{
> +	if (rcdev->dev)
> +		return dev_name(rcdev->dev);
> +
> +	if (rcdev->of_node)
> +		return rcdev->of_node->full_name;
> +
> +	return NULL;
> +}
> +
>  /**
>   * of_reset_simple_xlate - translate reset_spec to the reset line number
>   * @rcdev: a pointer to the reset controller device
> @@ -276,7 +287,7 @@ int reset_control_reset(struct reset_control *rstc)
>  			return 0;
>  	} else {
>  		if (!rstc->acquired)
> -			return -EINVAL;
> +			return -EPERM;
>  	}
>  
>  	ret = rstc->rcdev->ops->reset(rstc->rcdev, rstc->id);
> @@ -340,8 +351,11 @@ int reset_control_assert(struct reset_control *rstc)
>  		if (!rstc->rcdev->ops->assert)
>  			return -ENOTSUPP;
>  
> -		if (!rstc->acquired)
> -			return -EINVAL;
> +		if (!rstc->acquired) {
> +			WARN(1, "reset %s (ID: %u) is not acquired\n",
> +			     rcdev_name(rstc->rcdev), rstc->id);
> +			return -EPERM;
> +		}
>  	}
>  
>  	return rstc->rcdev->ops->assert(rstc->rcdev, rstc->id);
> @@ -378,8 +392,11 @@ int reset_control_deassert(struct reset_control *rstc)
>  		if (atomic_inc_return(&rstc->deassert_count) != 1)
>  			return 0;
>  	} else {
> -		if (!rstc->acquired)
> -			return -EINVAL;
> +		if (!rstc->acquired) {
> +			WARN(1, "reset %s (ID: %u) is not acquired\n",
> +			     rcdev_name(rstc->rcdev), rstc->id);
> +			return -EPERM;
> +		}
>  	}
>  
>  	/*
> @@ -417,15 +434,43 @@ int reset_control_status(struct reset_control *rstc)
>  }
>  EXPORT_SYMBOL_GPL(reset_control_status);
>  
> +/**
> + * reset_control_acquire() - acquires a reset control for exclusive use
> + * @rstc: reset control
> + *
> + * This is used to explicitly acquire a reset control for exclusive use. Note
> + * that exclusive resets are requested as acquired by default. In order for a
> + * second consumer to be able to control the reset, the first consumer has to
> + * release it first. Typically the easiest way to achieve this is to call the
> + * reset_control_get_exclusive_released() to obtain an instance of the reset
> + * control. Such reset controls are not acquired by default.
> + *
> + * Consumers implementing shared access to an exclusive reset need to follow
> + * a specific protocol in order to work together. Before consumers can change
> + * a reset they must acquire exclusive access using reset_control_acquire().
> + * After they are done operating the reset, they must release exclusive access
> + * with a call to reset_control_release(). Consumers are not granted exclusive
> + * access to the reset as long as another consumer hasn't released a reset.
> + *
> + * See also: reset_control_release()
> + */
>  int reset_control_acquire(struct reset_control *rstc)
>  {
>  	struct reset_control *rc;
>  
> -	if (!rstc || rstc->acquired)
> +	if (!rstc)
>  		return 0;
>  
> +	if (WARN_ON(IS_ERR(rstc)))
> +		return -EINVAL;
> +
>  	mutex_lock(&reset_list_mutex);
>  
> +	if (rstc->acquired) {
> +		mutex_unlock(&reset_list_mutex);
> +		return 0;
> +	}
> +
>  	list_for_each_entry(rc, &rstc->rcdev->reset_control_head, list) {
>  		if (rstc != rc && rstc->id == rc->id) {
>  			if (rc->acquired) {
> @@ -435,13 +480,28 @@ int reset_control_acquire(struct reset_control *rstc)
>  		}
>  	}
>  
> +	rstc->acquired = true;
> +
>  	mutex_unlock(&reset_list_mutex);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(reset_control_acquire);
>  
> +/**
> + * reset_control_release() - releases exclusive access to a reset control
> + * @rstc: reset control
> + *
> + * Releases exclusive access right to a reset control previously obtained by a
> + * call to reset_control_acquire(). Until a consumer calls this function, no
> + * other consumers will be granted exclusive access.
> + *
> + * See also: reset_control_acquire()
> + */
>  void reset_control_release(struct reset_control *rstc)
>  {
> +	if (!rstc || WARN_ON(IS_ERR(rstc)))
> +		return;
> +
>  	rstc->acquired = false;
>  }
>  EXPORT_SYMBOL_GPL(reset_control_release);



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]