Re: [PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value

[Suzuki K Poulose <suzuki.poulose@arm.com>]

Hi Mike

On 13/12/2024 14:49, Mike Leach wrote:
> Current coresight_timeout function spins on a bit on a test register,
> till bit value achieved or timeout hit.
> 
> Add another function to return the full value of the register being
> tested.
> 

The patch looks good to me. However, this change doesn't bring much 
value, other than avoiding a read at the caller side. I guess it is
simpler to leave things as is and let the caller read the register value
while logging rather than providing another variant.

I am inclined to keep the interface simpler.


> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 50 +++++++++++++++-----
>   include/linux/coresight.h                    |  2 +
>   2 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index ea38ecf26fcb..feb1a1db355f 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1017,32 +1017,37 @@ static void coresight_remove_conns(struct coresight_device *csdev)
>   }
>   
>   /**
> - * coresight_timeout - loop until a bit has changed to a specific register
> - *			state.
> + * coresight_timeout_retval - loop until a bit has changed to a specific register
> + *			      state. Return final register value
>    * @csa: coresight device access for the device
>    * @offset: Offset of the register from the base of the device.
>    * @position: the position of the bit of interest.
>    * @value: the value the bit should have.
> + * @rval:  the last read value of the register being tested.
>    *
>    * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
>    * TIMEOUT_US has elapsed, which ever happens first.
>    */
> -int coresight_timeout(struct csdev_access *csa, u32 offset,
> -		      int position, int value)
> +int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
> +			     int position, int value, u32 *rval)
>   {
> -	int i;
> -	u32 val;
> +	int i, rc = -EAGAIN;
> +	u32 val = 0;
>   
>   	for (i = TIMEOUT_US; i > 0; i--) {
>   		val = csdev_access_read32(csa, offset);
>   		/* waiting on the bit to go from 0 to 1 */
>   		if (value) {
> -			if (val & BIT(position))
> -				return 0;
> +			if (val & BIT(position)) {
> +				rc = 0;
> +				goto return_rval;
> +			}
>   		/* waiting on the bit to go from 1 to 0 */
>   		} else {
> -			if (!(val & BIT(position)))
> -				return 0;
> +			if (!(val & BIT(position))) {
> +				rc = 0;
> +				goto return_rval;
> +			}
>   		}
>   
>   		/*
> @@ -1054,7 +1059,30 @@ int coresight_timeout(struct csdev_access *csa, u32 offset,
>   			udelay(1);
>   	}
>   
> -	return -EAGAIN;
> +return_rval:
> +	*rval = val;

super minor nit: we could be flexible in accepting a "NULL" rval and 
thus keep the normal variants pass the NULL straight in. i.e,

	if (rval)
		*rval = val;

> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(coresight_timeout_retval);
> +
> +/**
> + * coresight_timeout - loop until a bit has changed to a specific register
> + *		       state
> + * @csa: coresight device access for the device
> + * @offset: Offset of the register from the base of the device.
> + * @position: the position of the bit of interest.
> + * @value: the value the bit should have.
> + *
> + * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
> + * TIMEOUT_US has elapsed, which ever happens first.
> + */
> +int coresight_timeout(struct csdev_access *csa, u32 offset,
> +		      int position, int value)
> +{
> +	u32 rval = 0;
> +
> +	return coresight_timeout_retval(csa, offset, position, value, &rval);

	return core_timeout_retval(csa, offset, position, value, NULL);

Suzuki

>   }
>   EXPORT_SYMBOL_GPL(coresight_timeout);
>   
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 055ce5cd5c44..29cb71e82b0b 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -639,6 +639,8 @@ extern int coresight_enable_sysfs(struct coresight_device *csdev);
>   extern void coresight_disable_sysfs(struct coresight_device *csdev);
>   extern int coresight_timeout(struct csdev_access *csa, u32 offset,
>   			     int position, int value);
> +extern int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
> +				    int position, int value, u32 *rval);
>   
>   extern int coresight_claim_device(struct coresight_device *csdev);
>   extern int coresight_claim_device_unlocked(struct coresight_device *csdev);