Re: [PATCH v3 03/13] coresight: stm: Update STM driver to use Trace ID API

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

On 09/08/2022 23:33, Mike Leach wrote:
> Updates the STM driver to use the trace ID allocation API.
> This uses the _system_id calls to allocate an ID on device poll,
> and release on device remove.
> 
> The sysfs access to the STMTRACEIDR register has been changed from RW
> to RO. Having this value as writable is not appropriate for the new
> Trace ID scheme - and had potential to cause errors in the previous
> scheme if values clashed with other sources.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-stm.c | 41 +++++++--------------
>   1 file changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> index bb14a3a8a921..9ef3e923a930 100644
> --- a/drivers/hwtracing/coresight/coresight-stm.c
> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> @@ -31,6 +31,7 @@
>   #include <linux/stm.h>
>   
>   #include "coresight-priv.h"
> +#include "coresight-trace-id.h"
>   
>   #define STMDMASTARTR			0xc04
>   #define STMDMASTOPR			0xc08
> @@ -615,24 +616,7 @@ static ssize_t traceid_show(struct device *dev,
>   	val = drvdata->traceid;
>   	return sprintf(buf, "%#lx\n", val);
>   }
> -
> -static ssize_t traceid_store(struct device *dev,
> -			     struct device_attribute *attr,
> -			     const char *buf, size_t size)
> -{
> -	int ret;
> -	unsigned long val;
> -	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> -
> -	ret = kstrtoul(buf, 16, &val);
> -	if (ret)
> -		return ret;
> -
> -	/* traceid field is 7bit wide on STM32 */
> -	drvdata->traceid = val & 0x7f;
> -	return size;
> -}
> -static DEVICE_ATTR_RW(traceid);
> +static DEVICE_ATTR_RO(traceid);
>   
>   #define coresight_stm_reg(name, offset)	\
>   	coresight_simple_reg32(struct stm_drvdata, name, offset)
> @@ -819,14 +803,6 @@ static void stm_init_default_data(struct stm_drvdata *drvdata)
>   	 */
>   	drvdata->stmsper = ~0x0;
>   
> -	/*
> -	 * The trace ID value for *ETM* tracers start at CPU_ID * 2 + 0x10 and
> -	 * anything equal to or higher than 0x70 is reserved.  Since 0x00 is
> -	 * also reserved the STM trace ID needs to be higher than 0x00 and
> -	 * lowner than 0x10.
> -	 */
> -	drvdata->traceid = 0x1;
> -
>   	/* Set invariant transaction timing on all channels */
>   	bitmap_clear(drvdata->chs.guaranteed, 0, drvdata->numsp);
>   }
> @@ -854,7 +830,7 @@ static void stm_init_generic_data(struct stm_drvdata *drvdata,
>   
>   static int stm_probe(struct amba_device *adev, const struct amba_id *id)
>   {
> -	int ret;
> +	int ret, trace_id;
>   	void __iomem *base;
>   	struct device *dev = &adev->dev;
>   	struct coresight_platform_data *pdata = NULL;
> @@ -938,12 +914,22 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id)
>   		goto stm_unregister;
>   	}
>   
> +	trace_id = coresight_trace_id_get_system_id();
> +	if (trace_id < 0) {

The above API returns "INVALID_ID" and not a negative error status.
I think it is better to fix the API to return:

   ret < 0  - If there is any error
            - Otherwise a positive integer
And the users should be kept unaware of which ID is valid or invalid.

Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel