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 11:04, James Clark wrote:
> 
> 
> 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);
> 
> 

Actually, even better we can have an "int flags" argument rather than
having to work out what a load of booleans mean. And then use these:

  TRACE_ID_WANT_ODD | TRACE_ID_WANT_PREFERRED

  flags = TRACE_ID_WANT_ODD
  flags |= preferred_id > 0 ? TRACE_ID_WANT_PREFERRED : 0;
  coresight_trace_id_alloc_new_id(id_map, preferred_id, flags);

>> +	}
>>  	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 *