Re: [PATCH v2 2/3] coresight: Add support to get preferred id for system trace sources

[James Clark <james.clark@arm.com>]

On 03/06/2024 10:43, Mao Jinlong wrote:
> Dynamic trace id was introduced in coresight subsystem, so trace id is
> allocated dynamically. However, some hardware ATB source has static trace
> id and it cannot be changed via software programming. For such source,
> it can call coresight_get_source_traceid to get the fixed trace id from
> device node and pass id to coresight_trace_id_get_system_id to reserve
> the id.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../hwtracing/coresight/coresight-platform.c  | 26 +++++++++++++++
>  drivers/hwtracing/coresight/coresight-stm.c   |  2 +-
>  .../hwtracing/coresight/coresight-trace-id.c  | 32 ++++++++++++-------
>  .../hwtracing/coresight/coresight-trace-id.h  |  5 ++-
>  include/linux/coresight.h                     |  1 +
>  5 files changed, 53 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 9d550f5697fa..8dd3cbd676b8 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -183,6 +183,17 @@ static int of_coresight_get_cpu(struct device *dev)
>  	return cpu;
>  }
>  
> +/*
> + * of_coresight_get_trace_id: Get the atid of a source device.
> + *
> + * Returns 0 on success.
> + */
> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
> +{
> +
> +	return of_property_read_u32(dev->of_node, "trace-id", id);
> +}
> +
>  /*
>   * of_coresight_parse_endpoint : Parse the given output endpoint @ep
>   * and fill the connection information in @pdata->out_conns
> @@ -782,6 +793,12 @@ static inline int acpi_coresight_get_cpu(struct device *dev)
>  {
>  	return -ENODEV;
>  }
> +
> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
> +{
> +	return -ENODEV;
> +}
> +
>  #endif
>  
>  int coresight_get_cpu(struct device *dev)
> @@ -794,6 +811,15 @@ int coresight_get_cpu(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(coresight_get_cpu);
>  
> +int coresight_get_source_traceid(struct device *dev, u32 *id)
> +{
> +	if (!is_of_node(dev->fwnode))
> +		return -EINVAL;
> +
> +	return of_coresight_get_trace_id(dev, id);
> +}
> +EXPORT_SYMBOL_GPL(coresight_get_source_traceid);
> +
>  struct coresight_platform_data *
>  coresight_get_platform_data(struct device *dev)
>  {
> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> index e1c62820dfda..802f9e4ae570 100644
> --- a/drivers/hwtracing/coresight/coresight-stm.c
> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> @@ -901,7 +901,7 @@ static int __stm_probe(struct device *dev, struct resource *res)
>  		goto stm_unregister;
>  	}
>  
> -	trace_id = coresight_trace_id_get_system_id();
> +	trace_id = coresight_trace_id_get_system_id(0);

I would #define 0 to "TRACE_ID_ANY" or something like that so it's
obvious what it means.

>  	if (trace_id < 0) {
>  		ret = trace_id;
>  		goto cs_unregister;
> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
> index af5b4ef59cea..5c25c75a2f08 100644
> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
> @@ -75,20 +75,23 @@ static int coresight_trace_id_find_odd_id(struct coresight_trace_id_map *id_map)
>   * Allocate new ID and set in use
>   *
>   * if @preferred_id is a valid id then try to use that value if available.
> + * if @only_preferred is true, if @preferred_id is used, return error EINVAL.

if @only_preferred is true @preferred_id must be free, otherwise return
error -EINVAL

>   * if @preferred_id is not valid and @prefer_odd_id is true, try for odd id.
>   *
>   * Otherwise allocate next available ID.
>   */
>  static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map,
> -					   int preferred_id, bool prefer_odd_id)
> +			   int preferred_id, bool prefer_odd_id, bool only_preferred)
>  {
>  	int id = 0;
>  
>  	/* for backwards compatibility, cpu IDs may use preferred value */
> -	if (IS_VALID_CS_TRACE_ID(preferred_id) &&
> -	    !test_bit(preferred_id, id_map->used_ids)) {
> -		id = preferred_id;
> -		goto trace_id_allocated;
> +	if (IS_VALID_CS_TRACE_ID(preferred_id)) {
> +		if (!test_bit(preferred_id, id_map->used_ids)) {
> +			id = preferred_id;
> +			goto trace_id_allocated;
> +		} else if (WARN(only_preferred, "Trace ID %d is used.\n", preferred_id))
> +			return -EINVAL;
>  	} else if (prefer_odd_id) {
>  	/* may use odd ids to avoid preferred legacy cpu IDs */
>  		id = coresight_trace_id_find_odd_id(id_map);
> @@ -175,7 +178,7 @@ static int coresight_trace_id_map_get_cpu_id(int cpu, struct coresight_trace_id_
>  	 */
>  	id = coresight_trace_id_alloc_new_id(id_map,
>  					     CORESIGHT_LEGACY_CPU_TRACE_ID(cpu),
> -					     false);
> +					     false, false);
>  	if (!IS_VALID_CS_TRACE_ID(id))
>  		goto get_cpu_id_out_unlock;
>  
> @@ -222,14 +225,21 @@ static void coresight_trace_id_map_put_cpu_id(int cpu, struct coresight_trace_id
>  	DUMP_ID_MAP(id_map);
>  }
>  
> -static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map)
> +static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map,
> +				int preferred_id, bool only_preferred)

Looks like "only_preferred" was left on here by mistake and it's not
used. For this function preferred_id != 0 means the same.

>  {
>  	unsigned long flags;
>  	int id;
>  
>  	spin_lock_irqsave(&id_map_lock, flags);
> -	/* prefer odd IDs for system components to avoid legacy CPU IDS */
> -	id = coresight_trace_id_alloc_new_id(id_map, 0, true);
> +
> +	if (preferred_id > 0) {
> +		/* use preferred id if it is available */
> +		id = coresight_trace_id_alloc_new_id(id_map, preferred_id, false, true);
> +	} else {
> +		/* prefer odd IDs for system components to avoid legacy CPU IDS */
> +		id = coresight_trace_id_alloc_new_id(id_map, 0, true, false);

prefer_odd_id is different in each of these calls which I think is a
mistake?

You could do one function call to avoid that:

  only_preferred = preferred_id > 0
  coresight_trace_id_alloc_new_id(id_map, preferred_id, true,
                                  only_preferred);


> +	}
>  	spin_unlock_irqrestore(&id_map_lock, flags);
>  
>  	DUMP_ID(id);
> @@ -269,9 +279,9 @@ int coresight_trace_id_read_cpu_id(int cpu)
>  }
>  EXPORT_SYMBOL_GPL(coresight_trace_id_read_cpu_id);
>  
> -int coresight_trace_id_get_system_id(void)
> +int coresight_trace_id_get_system_id(int id)
>  {
> -	return coresight_trace_id_map_get_system_id(&id_map_default);
> +	return coresight_trace_id_map_get_system_id(&id_map_default, id, true);
>  }
>  EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);
>  
> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h
> index 3797777d367e..9189a33c5857 100644
> --- a/drivers/hwtracing/coresight/coresight-trace-id.h
> +++ b/drivers/hwtracing/coresight/coresight-trace-id.h
> @@ -118,9 +118,12 @@ int coresight_trace_id_read_cpu_id(int cpu);
>   *
>   * Used to allocate IDs for system trace sources such as STM.
>   *
> + * @id: Preferred id value. If id is 0, get a free id from id map. If id is greater

0 -> TRACE_ID_ANY

> + *      than 0, get a preferred id.
> + *
>   * return: Trace ID or -EINVAL if allocation is impossible.
>   */
> -int coresight_trace_id_get_system_id(void);
> +int coresight_trace_id_get_system_id(int id);
>  
>  /**
>   * Release an allocated system trace ID.
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index f09ace92176e..0599303be326 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -643,6 +643,7 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
>  void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
>  
>  extern int coresight_get_cpu(struct device *dev);
> +extern int coresight_get_source_traceid(struct device *dev, u32 *id);
>  
>  struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
>  struct coresight_connection *