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

Suzuki K Poulose
2020-11-04T10:43:00Z

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