Re: [PATCH v4 05/10] coresight-tpdm: Add integration test support

[Jinlong Mao <quic_jinlmao@quicinc.com>]

Hi Greg,

On 3/24/2022 8:28 PM, Greg Kroah-Hartman wrote:
> On Thu, Mar 24, 2022 at 08:17:29PM +0800, Mao Jinlong wrote:
>> Integration test for tpdm can help to generate the data for
>> verification of the topology during TPDM software bring up.
>>
>> Sample:
>> echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink
>> echo 1 > /sys/bus/coresight/devices/tpdm1/enable_source
>> echo 1 > /sys/bus/coresight/devices/tpdm1/integration_test
>> echo 2 > /sys/bus/coresight/devices/tpdm1/integration_test
>> cat /dev/tmc_etf0 > /data/etf-tpdm1.bin
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/Kconfig          |  9 ++++
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 56 +++++++++++++++++++-
>>   drivers/hwtracing/coresight/coresight-tpdm.h |  8 +++
>>   3 files changed, 72 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index 5c506a1cd08f..60248fef4089 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -214,4 +214,13 @@ config CORESIGHT_TPDM
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called coresight-tpdm.
>>   
>> +config CORESIGHT_TPDM_INTEGRATION_TEST
>> +	bool "Enable CoreSight Integration Test For TPDM"
>> +	depends on CORESIGHT_TPDM
>> +	help
>> +	  This option adds support for the CoreSight integration test on this
>> +	  devie. Coresight architecture provides integration control modes of
>> +	  operation to facilitate integration testing and software bringup
>> +	  and/or to instrument topology discovery. The TPDM utilizes integration
>> +	  mode to accomplish integration testing and software bringup.
> Why is this a Kconfig option?  Why would you never not want this?
I refer to "CONFIG_CORESIGHT_CTI_INTEGRATION_REGS" for integration mode.
>
>>   endif
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index a8d257a591f3..cddd398be0cd 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -124,7 +124,60 @@ static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
>>   			__set_bit(i, drvdata->datasets);
>>   	}
>>   	CS_LOCK(drvdata->base);
>> - }
>> +}
>> +
>> +#ifdef CONFIG_CORESIGHT_TPDM_INTEGRATION_TEST
> Try to keep #ifdefs out of .c files please.
I will check to see if it is possible for the integration test function.
>> +static ssize_t integration_test_store(struct device *dev,
>> +					  struct device_attribute *attr,
>> +					  const char *buf,
>> +					  size_t size)
>> +{
>> +	int i, ret = 0;
>> +	unsigned long val;
>> +	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +	ret = kstrtoul(buf, 10, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (val != 1 && val != 2)
>> +		return -EINVAL;
>> +
>> +	if (!drvdata->enable)
>> +		return -EINVAL;
>> +
>> +	if (val == 1)
>> +		val = ATBCNTRL_VAL_64;
>> +	else
>> +		val = ATBCNTRL_VAL_32;
>> +	CS_UNLOCK(drvdata->base);
>> +	writel_relaxed(0x1, drvdata->base + TPDM_ITCNTRL);
>> +
>> +	for (i = 1; i < 5; i++)
>> +		writel_relaxed(val, drvdata->base + TPDM_ITATBCNTRL);
>> +
>> +	writel_relaxed(0, drvdata->base + TPDM_ITCNTRL);
>> +	CS_LOCK(drvdata->base);
>> +	return size;
>> +}
>> +static DEVICE_ATTR_WO(integration_test);
>> +#endif /* CORESIGHT_TPDM_INTEGRATION_TEST */
>> +
>> +static struct attribute *tpdm_attrs[] = {
>> +#ifdef CONFIG_CORESIGHT_TPDM_INTEGRATION_TEST
>> +	&dev_attr_integration_test.attr,
>> +#endif /* CORESIGHT_TPDM_INTEGRATION_TEST */
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group tpdm_attr_grp = {
>> +	.attrs = tpdm_attrs,
>> +};
>> +
>> +static const struct attribute_group *tpdm_attr_grps[] = {
>> +	&tpdm_attr_grp,
>> +	NULL,
>> +};
> ATTRIBUTE_GROUPS()?
>
> thanks,
>
> greg k-h

Thanks

Jinlong Mao