Received-SPF: pass (google.com: domain of linux-kernel+bounces-23029-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ff154c61-69de-4791-a8a1-8c1ff2a496d1@quicinc.com>
Date: Thu, 11 Jan 2024 11:28:01 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] coresight: Add coresight name support
Content-Language: en-US
To: James Clark <james.clark@arm.com>,
        Suzuki K Poulose
	<suzuki.poulose@arm.com>,
        Mike Leach <mike.leach@linaro.org>
CC: <coresight@lists.linaro.org>, <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        "Tingwei
 Zhang" <quic_tingweiz@quicinc.com>,
        Yuanfang Zhang
	<quic_yuanfang@quicinc.com>,
        Tao Zhang <quic_taozha@quicinc.com>, Leo Yan
	<leo.yan@linaro.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>
References: <20231228093321.5522-1-quic_jinlmao@quicinc.com>
 <12ce6e5d-6e4d-fb99-eb82-dece97423bfb@arm.com>
From: Jinlong Mao <quic_jinlmao@quicinc.com>
In-Reply-To: <12ce6e5d-6e4d-fb99-eb82-dece97423bfb@arm.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 12/28/2023 7:26 PM, James Clark wrote:
> 
> 
> On 28/12/2023 09:33, Mao Jinlong wrote:
>> Add coresight name support for custom names which will be
>> easy to identify the device by the name.
>>
> 
> I suppose this is more of a V2 because the subject is the same as the
> one sent earlier this year. But it looks like the discussion on the
> previous one wasn't resolved.

I will make next patch as V2

> 
> With the main issues to solve being:
> 
>   * It would be nice to use the existing root node name instead of adding
>     a new property. But at the same time DT nodes are supposed to have
>     generic names.
> 
>   * This only works for DT and not ACPI
> 
> To me it seems like adding the new property is just a "cheat" to get
> around not being allowed to have a specific name for the root node. But
> if we admit that we need a name I don't see the benefit of not putting
> the name where the node is already named.
> 
> Using the root node name at this point would also undo the hard coded
> per-cpu naming of the CTI and ETM devices, so maybe it would be nice,
> but it's just too late. That means that a new field is necessary.
> Although that field could be a boolean like "use-root-name-for-display"
> or something like that. In the end it probably doesn't really make a
> difference whether it's that or a name string.
> 
> And maybe the answer to the ACPI question is just that if anyone needs
> it, they can add it in the future. It doesn't seem like it would
> conflict with anything we do here.
> 
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   .../hwtracing/coresight/coresight-cti-core.c  | 20 ++++++++------
>>   drivers/hwtracing/coresight/coresight-dummy.c | 10 ++++---
>>   .../hwtracing/coresight/coresight-platform.c  | 27 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c  | 10 ++++---
>>   include/linux/coresight.h                     |  1 +
>>   5 files changed, 53 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
>> index 3999d0a2cb60..60a1e76064a9 100644
>> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
>> @@ -902,14 +902,18 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>>   	/* default to powered - could change on PM notifications */
>>   	drvdata->config.hw_powered = true;
>>   
>> -	/* set up device name - will depend if cpu bound or otherwise */
>> -	if (drvdata->ctidev.cpu >= 0)
>> -		cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
>> -					       drvdata->ctidev.cpu);
>> -	else
>> -		cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
> 
> Can we put the new name stuff inside coresight_alloc_device_name()? Then
> it happens by default for every device.
> 
> I know Suzuki said previously to do it per-device, but the new DT
> property is just "coresight-name", so it's generic. Rather than being
> specific like "cti-name". So I don't see the benefit of duplicating the
> code at this point if we do decide to do it.
I will add it inside the coresight_alloc_device_name.
> 
>> -	if (!cti_desc.name)
>> -		return -ENOMEM;
>> +	cti_desc.name = coresight_get_device_name(dev);
>> +	if (!cti_desc.name) {
>> +		/* set up device name - will depend if cpu bound or otherwise */
>> +		if (drvdata->ctidev.cpu >= 0)
>> +			cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
>> +						       drvdata->ctidev.cpu);
>> +		else {
>> +			cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
>> +			if (!cti_desc.name)
>> +				return -ENOMEM;
>> +		}
>> +	}
> 
>>   
>>   	/* setup CPU power management handling for CPU bound CTI devices. */
>>   	ret = cti_pm_setup(drvdata);
>> diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
>> index e4deafae7bc2..b19cd400df79 100644
>> --- a/drivers/hwtracing/coresight/coresight-dummy.c
>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>> @@ -76,10 +76,12 @@ static int dummy_probe(struct platform_device *pdev)
>>   	struct coresight_desc desc = { 0 };
>>   
>>   	if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
>> -
>> -		desc.name = coresight_alloc_device_name(&source_devs, dev);
>> -		if (!desc.name)
>> -			return -ENOMEM;
>> +		desc.name = coresight_get_device_name(dev);
>> +		if (!desc.name) {
>> +			desc.name = coresight_alloc_device_name(&source_devs, dev);
>> +			if (!desc.name)
>> +				return -ENOMEM;
>> +		}
>>   
>>   		desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>>   		desc.subtype.source_subtype =
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>> index 9d550f5697fa..284aa22a06b7 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -183,6 +183,18 @@ static int of_coresight_get_cpu(struct device *dev)
>>   	return cpu;
>>   }
>>   
>> +static const char *of_coresight_get_device_name(struct device *dev)
>> +{
>> +	const char *name = NULL;
>> +
>> +	if (!dev->of_node)
>> +		return NULL;
>> +
>> +	of_property_read_string(dev->of_node, "coresight-name", &name);
> 
> Do you need to update the binding docs with this new property?
Yes. I will update the binding doc.
> 
> Also a minor nit: Maybe "display-name" is better? "Coresight" is
> implied, and the node is already named, although that node name isn't
> used for display purposes, but this one is.
 From Suzuki's comments, "device-name" should be better. What do you think ?

Thanks
Jinlong Mao