Re: [PATCH v3 09/26] coresight: Convert coresight_timeout to use access abstraction

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org,
	coresight@lists.linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/26] coresight: Convert coresight_timeout to use access abstraction
Date: Wed, 4 Nov 2020 10:42:56 +0000	[thread overview]
Message-ID: <b1323f88-d6b8-5525-4d2f-d001236f8860@arm.com> (raw)
In-Reply-To: <20201103180329.GB2855763@xps15>

On 11/3/20 6:03 PM, Mathieu Poirier wrote:
> On Wed, Oct 28, 2020 at 10:09:28PM +0000, Suzuki K Poulose wrote:
>> Convert the generic routines to use the new access abstraction layer
>> gradually, starting with coresigth_timeout.
>>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---

>> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
>> index 5baf29510f1b..34c74b05c542 100644
>> --- a/drivers/hwtracing/coresight/coresight-catu.c
>> +++ b/drivers/hwtracing/coresight/coresight-catu.c
>> @@ -401,8 +401,9 @@ static const struct attribute_group *catu_groups[] = {
>>   
>>   static inline int catu_wait_for_ready(struct catu_drvdata *drvdata)
>>   {
>> -	return coresight_timeout(drvdata->base,
>> -				 CATU_STATUS, CATU_STATUS_READY, 1);
>> +	struct csdev_access *csa = &drvdata->csdev->access;
>> +
>> +	return coresight_timeout(csa, CATU_STATUS, CATU_STATUS_READY, 1);
>>   }
>>   
>>   static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
>> index e96deaca8cab..42ba989a6b5e 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1412,23 +1412,26 @@ static void coresight_remove_conns(struct coresight_device *csdev)
>>   }
>>   
>>   /**
>> - * coresight_timeout - loop until a bit has changed to a specific state.
>> - * @addr: base address of the area of interest.
>> - * @offset: address of a register, starting from @addr.
>> + * coresight_timeout - loop until a bit has changed to a specific register
>> + *			state.
>> + * @csa: coresight device access for the device
>> + * @offset: Offset of the register from the base of the device.
>>    * @position: the position of the bit of interest.
>>    * @value: the value the bit should have.
>>    *
>>    * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
>>    * TIMEOUT_US has elapsed, which ever happens first.
>>    */
>> -
>> -int coresight_timeout(void __iomem *addr, u32 offset, int position, int value)
>> +int coresight_timeout(struct csdev_access *csa,
>> +		      u32 offset,
>> +		      int position,
>> +		      int value)
> 
> There is no need for stacking, please maximise the 80 characters.  The function
> stubs in coresight.h should also be revised.
> 
>>   {
>>   	int i;
>>   	u32 val;
>>   
>>   	for (i = TIMEOUT_US; i > 0; i--) {
>> -		val = __raw_readl(addr + offset);
>> +		val = csdev_access_read32(csa, offset);
> 
> I vaguely remember commenting on this, or perhaps it was on some othe patch you
> wrote...  Anyways, I think it is a good thing to go from an unordered access to
> an ordered access for the timeout function.

Yes, you did mention it in the v1.

> 
> With the above:
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Thanks, will address the comments.

Suzuki