Received-SPF: pass (google.com: domain of linux-kernel+bounces-75942-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <af8c690f-226b-4397-ad38-9f12f3c1bd15@arm.com>
Date: Thu, 22 Feb 2024 11:03:42 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V4 02/11] coresight: stm: Extract device name from AMBA
 pid based table lookup
Content-Language: en-US
To: Suzuki K Poulose <suzuki.poulose@arm.com>,
 linux-arm-kernel@lists.infradead.org
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>,
 Sudeep Holla <sudeep.holla@arm.com>, Mike Leach <mike.leach@linaro.org>,
 Maxime Coquelin <mcoquelin.stm32@gmail.com>,
 Alexandre Torgue <alexandre.torgue@foss.st.com>, linux-acpi@vger.kernel.org,
 linux-kernel@vger.kernel.org, coresight@lists.linaro.org,
 linux-stm32@st-md-mailman.stormreply.com
References: <20240123054608.1790189-1-anshuman.khandual@arm.com>
 <20240123054608.1790189-3-anshuman.khandual@arm.com>
 <56d56f85-6172-4fb1-a002-fe82ffc8a66a@arm.com>
From: Anshuman Khandual <anshuman.khandual@arm.com>
In-Reply-To: <56d56f85-6172-4fb1-a002-fe82ffc8a66a@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 2/15/24 16:25, Suzuki K Poulose wrote:
> On 23/01/2024 05:45, Anshuman Khandual wrote:
>> Instead of using AMBA private data field, extract the device name from AMBA
>> pid based table lookup using new coresight_get_uci_data_from_amba() helper.
>>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: James Clark <james.clark@arm.com>
>> Cc: coresight@lists.linaro.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-stm32@st-md-mailman.stormreply.com
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-priv.h | 10 ++++++++++
>>   drivers/hwtracing/coresight/coresight-stm.c  | 14 +++++++++++++-
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
>> index 767076e07970..68cbb036cec8 100644
>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> @@ -221,6 +221,16 @@ static inline void *coresight_get_uci_data(const struct amba_id *id)
>>       return uci_id->data;
>>   }
>>   +static inline void *coresight_get_uci_data_from_amba(const struct amba_id *table, u32 pid)
>> +{
>> +    while (table->mask) {
>> +        if ((table->id & table->mask) == pid)
> 
> Why are we masking table->id ? table->id is a static value that the
> driver wants to check for "variants" of a given device. The table->mask
> is there to filter out the "irrelevant" bits of the PID that we read
> from the device. So this should instead be:
> 
>         if ((table->mask & pid) == table->id)

Sure, will this change as suggested.

> 
>> +            return coresight_get_uci_data(table);
>> +        table++;
>> +    };
>> +    return NULL;
>> +}
>> +
>>   void coresight_release_platform_data(struct coresight_device *csdev,
>>                        struct device *dev,
>>                        struct coresight_platform_data *pdata);
>> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
>> index a1c27c901ad1..9cdca4f86cab 100644
>> --- a/drivers/hwtracing/coresight/coresight-stm.c
>> +++ b/drivers/hwtracing/coresight/coresight-stm.c
>> @@ -804,6 +804,18 @@ static void stm_init_generic_data(struct stm_drvdata *drvdata,
>>       drvdata->stm.set_options = stm_generic_set_options;
>>   }
>>   +#define STM_AMBA_MASK 0xfffff
>> +
>> +static const struct amba_id stm_ids[];
>> +
>> +static char *stm_csdev_name(struct coresight_device *csdev)
>> +{
>> +    u32 stm_pid = coresight_get_pid(&csdev->access) & STM_AMBA_MASK;
> 
> Similar to above:
> 
> Why do we apply a "custom" mask to the PID and later check the PID with
> that of the table->pid.
> 
> The way it is supposed work is :
> 
>     (table->mask & dev_pid) == table->pid
> 
> the table->mask is there for a reason: i.e., to get the relevant bits from the device_pid and compare it against "the" expected value (table->pid).

Understood, will change the helper as follows

static inline void *coresight_get_uci_data_from_amba(const struct amba_id *table, u32 pid)
{
        while (table->mask) {
                if ((pid & table->mask) == table->id)
                        return coresight_get_uci_data(table);
                table++;
        };
        return NULL;
}

and also drop both the custom masks STM_AMBA_MASK and TMC_AMBA_MASK.