Coresight device pid can be retrieved from its iomem base address, which is
stored in 'struct etm4x_drvdata'. This drops pid argument from etm4_probe()
and 'struct etm4_init_arg'. Instead etm4_check_arch_features() derives the
coresight device pid with a new helper coresight_get_pid(), right before it
is consumed in etm4_hisi_match_pid().
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
.../coresight/coresight-etm4x-core.c | 21 +++++++------------
include/linux/coresight.h | 12 +++++++++++
2 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 5d77571a8df9..3521838ab4fb 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -66,7 +66,6 @@ static u64 etm4_get_access_type(struct etmv4_config *config);
static enum cpuhp_state hp_online;
struct etm4_init_arg {
- unsigned int pid;
struct device *dev;
struct csdev_access *csa;
};
@@ -370,8 +369,10 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
}
static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
- unsigned int id)
+ struct csdev_access *csa)
{
+ unsigned int id = coresight_get_pid(csa);
+
if (etm4_hisi_match_pid(id))
set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
}
@@ -385,7 +386,7 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
}
static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
- unsigned int id)
+ struct csdev_access *csa)
{
}
#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
@@ -1165,7 +1166,7 @@ static void etm4_init_arch_data(void *info)
etm4_os_unlock_csa(drvdata, csa);
etm4_cs_unlock(drvdata, csa);
- etm4_check_arch_features(drvdata, init_arg->pid);
+ etm4_check_arch_features(drvdata, csa);
/* find all capabilities of the tracing unit */
etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
@@ -2048,7 +2049,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
return 0;
}
-static int etm4_probe(struct device *dev, u32 etm_pid)
+static int etm4_probe(struct device *dev)
{
struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
struct csdev_access access = { 0 };
@@ -2077,7 +2078,6 @@ static int etm4_probe(struct device *dev, u32 etm_pid)
init_arg.dev = dev;
init_arg.csa = &access;
- init_arg.pid = etm_pid;
/*
* Serialize against CPUHP callbacks to avoid race condition
@@ -2124,7 +2124,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
drvdata->base = base;
dev_set_drvdata(dev, drvdata);
- ret = etm4_probe(dev, id->id);
+ ret = etm4_probe(dev);
if (!ret)
pm_runtime_put(&adev->dev);
@@ -2146,12 +2146,7 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
- /*
- * System register based devices could match the
- * HW by reading appropriate registers on the HW
- * and thus we could skip the PID.
- */
- ret = etm4_probe(&pdev->dev, 0);
+ ret = etm4_probe(&pdev->dev);
pm_runtime_put(&pdev->dev);
return ret;
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index f19a47b9bb5a..f85b041ea475 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -370,6 +370,18 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
return csa->read(offset, true, false);
}
+#define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
+
+static inline u32 coresight_get_pid(struct csdev_access *csa)
+{
+ u32 i, pid = 0;
+
+ for (i = 0; i < 4; i++)
+ pid |= csdev_access_relaxed_read32(csa, CORESIGHT_PIDRn(i)) << (i * 8);
+
+ return pid;
+}
+
static inline u64 csdev_access_relaxed_read_pair(struct csdev_access *csa,
u32 lo_offset, u32 hi_offset)
{
--
2.25.1On 27/03/2023 06:05, Anshuman Khandual wrote:
> Coresight device pid can be retrieved from its iomem base address, which is
> stored in 'struct etm4x_drvdata'. This drops pid argument from etm4_probe()
> and 'struct etm4_init_arg'. Instead etm4_check_arch_features() derives the
> coresight device pid with a new helper coresight_get_pid(), right before it
> is consumed in etm4_hisi_match_pid().
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: coresight@lists.linaro.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> .../coresight/coresight-etm4x-core.c | 21 +++++++------------
> include/linux/coresight.h | 12 +++++++++++
> 2 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 5d77571a8df9..3521838ab4fb 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -66,7 +66,6 @@ static u64 etm4_get_access_type(struct etmv4_config *config);
> static enum cpuhp_state hp_online;
>
> struct etm4_init_arg {
> - unsigned int pid;
> struct device *dev;
> struct csdev_access *csa;
> };
> @@ -370,8 +369,10 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
> }
>
> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
> - unsigned int id)
> + struct csdev_access *csa)
> {
> + unsigned int id = coresight_get_pid(csa);
> +
This throws up the following error on an ETE.
ete: trying to read unsupported register @fe0
So, I guess this must be performed only for iomem based
devices. System instruction based device must be identified
by MIDR_EL1/REVIDR_EL1 if needed for specific erratum.
This is not required now. So, we could bail out early
if we are system instruction based device.
> if (etm4_hisi_match_pid(id))
> set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
> }
> @@ -385,7 +386,7 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
> }
>
> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
> - unsigned int id)
> + struct csdev_access *csa)
> {
> }
> #endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
> @@ -1165,7 +1166,7 @@ static void etm4_init_arch_data(void *info)
> etm4_os_unlock_csa(drvdata, csa);
> etm4_cs_unlock(drvdata, csa);
>
> - etm4_check_arch_features(drvdata, init_arg->pid);
> + etm4_check_arch_features(drvdata, csa);
>
> /* find all capabilities of the tracing unit */
> etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
> @@ -2048,7 +2049,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
> return 0;
> }
>
> -static int etm4_probe(struct device *dev, u32 etm_pid)
> +static int etm4_probe(struct device *dev)
> {
> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
> struct csdev_access access = { 0 };
> @@ -2077,7 +2078,6 @@ static int etm4_probe(struct device *dev, u32 etm_pid)
>
> init_arg.dev = dev;
> init_arg.csa = &access;
> - init_arg.pid = etm_pid;
>
> /*
> * Serialize against CPUHP callbacks to avoid race condition
> @@ -2124,7 +2124,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
>
> drvdata->base = base;
> dev_set_drvdata(dev, drvdata);
> - ret = etm4_probe(dev, id->id);
> + ret = etm4_probe(dev);
> if (!ret)
> pm_runtime_put(&adev->dev);
>
> @@ -2146,12 +2146,7 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
> pm_runtime_set_active(&pdev->dev);
> pm_runtime_enable(&pdev->dev);
>
> - /*
> - * System register based devices could match the
> - * HW by reading appropriate registers on the HW
> - * and thus we could skip the PID.
> - */
> - ret = etm4_probe(&pdev->dev, 0);
> + ret = etm4_probe(&pdev->dev);
>
> pm_runtime_put(&pdev->dev);
> return ret;
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index f19a47b9bb5a..f85b041ea475 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -370,6 +370,18 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
> return csa->read(offset, true, false);
> }
>
> +#define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
> +
> +static inline u32 coresight_get_pid(struct csdev_access *csa)
> +{
> + u32 i, pid = 0;
> +
> + for (i = 0; i < 4; i++)
> + pid |= csdev_access_relaxed_read32(csa, CORESIGHT_PIDRn(i)) << (i * 8);
Given the above, we could make this iomem specific.
Suzuki
> +
> + return pid;
> +}
> +
> static inline u64 csdev_access_relaxed_read_pair(struct csdev_access *csa,
> u32 lo_offset, u32 hi_offset)
> {
On 3/31/23 16:36, Suzuki K Poulose wrote:
> On 27/03/2023 06:05, Anshuman Khandual wrote:
>> Coresight device pid can be retrieved from its iomem base address, which is
>> stored in 'struct etm4x_drvdata'. This drops pid argument from etm4_probe()
>> and 'struct etm4_init_arg'. Instead etm4_check_arch_features() derives the
>> coresight device pid with a new helper coresight_get_pid(), right before it
>> is consumed in etm4_hisi_match_pid().
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Leo Yan <leo.yan@linaro.org>
>> Cc: coresight@lists.linaro.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> .../coresight/coresight-etm4x-core.c | 21 +++++++------------
>> include/linux/coresight.h | 12 +++++++++++
>> 2 files changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 5d77571a8df9..3521838ab4fb 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -66,7 +66,6 @@ static u64 etm4_get_access_type(struct etmv4_config *config);
>> static enum cpuhp_state hp_online;
>> struct etm4_init_arg {
>> - unsigned int pid;
>> struct device *dev;
>> struct csdev_access *csa;
>> };
>> @@ -370,8 +369,10 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
>> }
>> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> - unsigned int id)
>> + struct csdev_access *csa)
>> {
>> + unsigned int id = coresight_get_pid(csa);
>> +
>
> This throws up the following error on an ETE.
>
> ete: trying to read unsupported register @fe0
>
> So, I guess this must be performed only for iomem based
> devices. System instruction based device must be identified
> by MIDR_EL1/REVIDR_EL1 if needed for specific erratum.
> This is not required now. So, we could bail out early
> if we are system instruction based device.
Will bail out on non iomem devices via drvdata->base address switch.
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -373,9 +373,10 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
struct csdev_access *csa)
{
- unsigned int id = coresight_get_pid(csa);
+ if (!drvdata->base)
+ return;
- if (etm4_hisi_match_pid(id))
+ if (etm4_hisi_match_pid(coresight_get_pid(csa)))
set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
}
#else
>
>
>> if (etm4_hisi_match_pid(id))
>> set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
>> }
>> @@ -385,7 +386,7 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
>> }
>> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> - unsigned int id)
>> + struct csdev_access *csa)
>> {
>> }
>> #endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
>> @@ -1165,7 +1166,7 @@ static void etm4_init_arch_data(void *info)
>> etm4_os_unlock_csa(drvdata, csa);
>> etm4_cs_unlock(drvdata, csa);
>> - etm4_check_arch_features(drvdata, init_arg->pid);
>> + etm4_check_arch_features(drvdata, csa);
>> /* find all capabilities of the tracing unit */
>> etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>> @@ -2048,7 +2049,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
>> return 0;
>> }
>> -static int etm4_probe(struct device *dev, u32 etm_pid)
>> +static int etm4_probe(struct device *dev)
>> {
>> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
>> struct csdev_access access = { 0 };
>> @@ -2077,7 +2078,6 @@ static int etm4_probe(struct device *dev, u32 etm_pid)
>> init_arg.dev = dev;
>> init_arg.csa = &access;
>> - init_arg.pid = etm_pid;
>> /*
>> * Serialize against CPUHP callbacks to avoid race condition
>> @@ -2124,7 +2124,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
>> drvdata->base = base;
>> dev_set_drvdata(dev, drvdata);
>> - ret = etm4_probe(dev, id->id);
>> + ret = etm4_probe(dev);
>> if (!ret)
>> pm_runtime_put(&adev->dev);
>> @@ -2146,12 +2146,7 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
>> pm_runtime_set_active(&pdev->dev);
>> pm_runtime_enable(&pdev->dev);
>> - /*
>> - * System register based devices could match the
>> - * HW by reading appropriate registers on the HW
>> - * and thus we could skip the PID.
>> - */
>> - ret = etm4_probe(&pdev->dev, 0);
>> + ret = etm4_probe(&pdev->dev);
>> pm_runtime_put(&pdev->dev);
>> return ret;
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index f19a47b9bb5a..f85b041ea475 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -370,6 +370,18 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
>> return csa->read(offset, true, false);
>> }
>> +#define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
>> +
>> +static inline u32 coresight_get_pid(struct csdev_access *csa)
>> +{
>> + u32 i, pid = 0;
>> +
>> + for (i = 0; i < 4; i++)
>> + pid |= csdev_access_relaxed_read32(csa, CORESIGHT_PIDRn(i)) << (i * 8);
>
> Given the above, we could make this iomem specific.
We could change coresight_get_pid() to take iomem base address instead
and fetch the pid. But is not the existing csdev_access based approach
better and more generic ?
#define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
static inline u32 coresight_get_pid(void __iomem *base)
{
u32 i, pid = 0;
for (i = 0; i < 4; i++)
pid |= readl(base + CORESIGHT_PIDRn(i)) << (i * 8);
return pid;
}
On 17/05/2023 05:32, Anshuman Khandual wrote:
>
>
> On 3/31/23 16:36, Suzuki K Poulose wrote:
>> On 27/03/2023 06:05, Anshuman Khandual wrote:
>>> Coresight device pid can be retrieved from its iomem base address, which is
>>> stored in 'struct etm4x_drvdata'. This drops pid argument from etm4_probe()
>>> and 'struct etm4_init_arg'. Instead etm4_check_arch_features() derives the
>>> coresight device pid with a new helper coresight_get_pid(), right before it
>>> is consumed in etm4_hisi_match_pid().
>>>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: Leo Yan <leo.yan@linaro.org>
>>> Cc: coresight@lists.linaro.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>> .../coresight/coresight-etm4x-core.c | 21 +++++++------------
>>> include/linux/coresight.h | 12 +++++++++++
>>> 2 files changed, 20 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index 5d77571a8df9..3521838ab4fb 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -66,7 +66,6 @@ static u64 etm4_get_access_type(struct etmv4_config *config);
>>> static enum cpuhp_state hp_online;
>>> struct etm4_init_arg {
>>> - unsigned int pid;
>>> struct device *dev;
>>> struct csdev_access *csa;
>>> };
>>> @@ -370,8 +369,10 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
>>> }
>>> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>>> - unsigned int id)
>>> + struct csdev_access *csa)
>>> {
>>> + unsigned int id = coresight_get_pid(csa);
>>> +
>>
>> This throws up the following error on an ETE.
>>
>> ete: trying to read unsupported register @fe0
>>
>> So, I guess this must be performed only for iomem based
>> devices. System instruction based device must be identified
>> by MIDR_EL1/REVIDR_EL1 if needed for specific erratum.
>> This is not required now. So, we could bail out early
>> if we are system instruction based device.
>
> Will bail out on non iomem devices via drvdata->base address switch.
>
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -373,9 +373,10 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
> struct csdev_access *csa)
> {
> - unsigned int id = coresight_get_pid(csa);
> + if (!drvdata->base)
> + return;
I would use !csa->io_mem to be more readerf friendly.
>
> - if (etm4_hisi_match_pid(id))
> + if (etm4_hisi_match_pid(coresight_get_pid(csa)))
> set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
> }
> #else
>
>>
>>
>>> if (etm4_hisi_match_pid(id))
>>> set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
>>> }
>>> @@ -385,7 +386,7 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
>>> }
>>> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>>> - unsigned int id)
>>> + struct csdev_access *csa)
>>> {
>>> }
>>> #endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
>>> @@ -1165,7 +1166,7 @@ static void etm4_init_arch_data(void *info)
>>> etm4_os_unlock_csa(drvdata, csa);
>>> etm4_cs_unlock(drvdata, csa);
>>> - etm4_check_arch_features(drvdata, init_arg->pid);
>>> + etm4_check_arch_features(drvdata, csa);
>>> /* find all capabilities of the tracing unit */
>>> etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>>> @@ -2048,7 +2049,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
>>> return 0;
>>> }
>>> -static int etm4_probe(struct device *dev, u32 etm_pid)
>>> +static int etm4_probe(struct device *dev)
>>> {
>>> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
>>> struct csdev_access access = { 0 };
>>> @@ -2077,7 +2078,6 @@ static int etm4_probe(struct device *dev, u32 etm_pid)
>>> init_arg.dev = dev;
>>> init_arg.csa = &access;
>>> - init_arg.pid = etm_pid;
>>> /*
>>> * Serialize against CPUHP callbacks to avoid race condition
>>> @@ -2124,7 +2124,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
>>> drvdata->base = base;
>>> dev_set_drvdata(dev, drvdata);
>>> - ret = etm4_probe(dev, id->id);
>>> + ret = etm4_probe(dev);
>>> if (!ret)
>>> pm_runtime_put(&adev->dev);
>>> @@ -2146,12 +2146,7 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
>>> pm_runtime_set_active(&pdev->dev);
>>> pm_runtime_enable(&pdev->dev);
>>> - /*
>>> - * System register based devices could match the
>>> - * HW by reading appropriate registers on the HW
>>> - * and thus we could skip the PID.
>>> - */
>>> - ret = etm4_probe(&pdev->dev, 0);
>>> + ret = etm4_probe(&pdev->dev);
>>> pm_runtime_put(&pdev->dev);
>>> return ret;
>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>> index f19a47b9bb5a..f85b041ea475 100644
>>> --- a/include/linux/coresight.h
>>> +++ b/include/linux/coresight.h
>>> @@ -370,6 +370,18 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
>>> return csa->read(offset, true, false);
>>> }
>>> +#define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
>>> +
>>> +static inline u32 coresight_get_pid(struct csdev_access *csa)
>>> +{
>>> + u32 i, pid = 0;
>>> +
>>> + for (i = 0; i < 4; i++)
>>> + pid |= csdev_access_relaxed_read32(csa, CORESIGHT_PIDRn(i)) << (i * 8);
>>
>> Given the above, we could make this iomem specific.
>
> We could change coresight_get_pid() to take iomem base address instead
> and fetch the pid. But is not the existing csdev_access based approach
> better and more generic ?
Yes, true, lets leave it with csdev_access, as it is coresight specific
and not ETMv4.
Cheers
Suzuki
On 3/31/2023 4:06 AM, Suzuki K Poulose wrote:
> On 27/03/2023 06:05, Anshuman Khandual wrote:
>> Coresight device pid can be retrieved from its iomem base address,
>> which is
>> stored in 'struct etm4x_drvdata'. This drops pid argument from
>> etm4_probe()
>> and 'struct etm4_init_arg'. Instead etm4_check_arch_features() derives
>> the
>> coresight device pid with a new helper coresight_get_pid(), right
>> before it
>> is consumed in etm4_hisi_match_pid().
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Leo Yan <leo.yan@linaro.org>
>> Cc: coresight@lists.linaro.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> .../coresight/coresight-etm4x-core.c | 21 +++++++------------
>> include/linux/coresight.h | 12 +++++++++++
>> 2 files changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 5d77571a8df9..3521838ab4fb 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -66,7 +66,6 @@ static u64 etm4_get_access_type(struct etmv4_config
>> *config);
>> static enum cpuhp_state hp_online;
>> struct etm4_init_arg {
>> - unsigned int pid;
>> struct device *dev;
>> struct csdev_access *csa;
>> };
>> @@ -370,8 +369,10 @@ static void etm4_disable_arch_specific(struct
>> etmv4_drvdata *drvdata)
>> }
>> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> - unsigned int id)
>> + struct csdev_access *csa)
>> {
>> + unsigned int id = coresight_get_pid(csa);
>> +
>
> This throws up the following error on an ETE.
>
> ete: trying to read unsupported register @fe0
>
> So, I guess this must be performed only for iomem based
> devices. System instruction based device must be identified
> by MIDR_EL1/REVIDR_EL1 if needed for specific erratum.
> This is not required now. So, we could bail out early
> if we are system instruction based device.
Besides this, the PID is limited to (I think) 4 bits of ID. TRCIDRs
offer revision information, but nothing manufacturer specific save for
the designer. Register fields like MIDR_EL1 Variant + PartNum + Revision
and TRCPIDR3 REVAND offer help. It may be a combination of registers are
needed for a manufacturer to adequately ID a part to apply an erratum.
Perhaps you could at least cache MIDR_EL1 for possible future use?
>
>
>> if (etm4_hisi_match_pid(id))
>> set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
>> }
>> @@ -385,7 +386,7 @@ static void etm4_disable_arch_specific(struct
>> etmv4_drvdata *drvdata)
>> }
>> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> - unsigned int id)
>> + struct csdev_access *csa)
>> {
>> }
>> #endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
>> @@ -1165,7 +1166,7 @@ static void etm4_init_arch_data(void *info)
>> etm4_os_unlock_csa(drvdata, csa);
>> etm4_cs_unlock(drvdata, csa);
>> - etm4_check_arch_features(drvdata, init_arg->pid);
>> + etm4_check_arch_features(drvdata, csa);
>> /* find all capabilities of the tracing unit */
>> etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>> @@ -2048,7 +2049,7 @@ static int etm4_add_coresight_dev(struct
>> etm4_init_arg *init_arg)
>> return 0;
>> }
>> -static int etm4_probe(struct device *dev, u32 etm_pid)
>> +static int etm4_probe(struct device *dev)
>> {
>> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
>> struct csdev_access access = { 0 };
>> @@ -2077,7 +2078,6 @@ static int etm4_probe(struct device *dev, u32
>> etm_pid)
>> init_arg.dev = dev;
>> init_arg.csa = &access;
>> - init_arg.pid = etm_pid;
>> /*
>> * Serialize against CPUHP callbacks to avoid race condition
>> @@ -2124,7 +2124,7 @@ static int etm4_probe_amba(struct amba_device
>> *adev, const struct amba_id *id)
>> drvdata->base = base;
>> dev_set_drvdata(dev, drvdata);
>> - ret = etm4_probe(dev, id->id);
>> + ret = etm4_probe(dev);
>> if (!ret)
>> pm_runtime_put(&adev->dev);
>> @@ -2146,12 +2146,7 @@ static int etm4_probe_platform_dev(struct
>> platform_device *pdev)
>> pm_runtime_set_active(&pdev->dev);
>> pm_runtime_enable(&pdev->dev);
>> - /*
>> - * System register based devices could match the
>> - * HW by reading appropriate registers on the HW
>> - * and thus we could skip the PID.
>> - */
>> - ret = etm4_probe(&pdev->dev, 0);
>> + ret = etm4_probe(&pdev->dev);
>> pm_runtime_put(&pdev->dev);
>> return ret;
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index f19a47b9bb5a..f85b041ea475 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -370,6 +370,18 @@ static inline u32
>> csdev_access_relaxed_read32(struct csdev_access *csa,
>> return csa->read(offset, true, false);
>> }
>> +#define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
>> +
>> +static inline u32 coresight_get_pid(struct csdev_access *csa)
>> +{
>> + u32 i, pid = 0;
>> +
>> + for (i = 0; i < 4; i++)
>> + pid |= csdev_access_relaxed_read32(csa, CORESIGHT_PIDRn(i))
>> << (i * 8);
>
> Given the above, we could make this iomem specific.
>
> Suzuki
>
>
>> +
>> + return pid;
>> +}
>> +
>> static inline u64 csdev_access_relaxed_read_pair(struct csdev_access
>> *csa,
>> u32 lo_offset, u32 hi_offset)
>> {
>
On 31/03/2023 22:24, Steve Clevenger wrote:
>
>
> On 3/31/2023 4:06 AM, Suzuki K Poulose wrote:
>> On 27/03/2023 06:05, Anshuman Khandual wrote:
>>> Coresight device pid can be retrieved from its iomem base address,
>>> which is
>>> stored in 'struct etm4x_drvdata'. This drops pid argument from
>>> etm4_probe()
>>> and 'struct etm4_init_arg'. Instead etm4_check_arch_features() derives
>>> the
>>> coresight device pid with a new helper coresight_get_pid(), right
>>> before it
>>> is consumed in etm4_hisi_match_pid().
>>>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: Leo Yan <leo.yan@linaro.org>
>>> Cc: coresight@lists.linaro.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>> .../coresight/coresight-etm4x-core.c | 21 +++++++------------
>>> include/linux/coresight.h | 12 +++++++++++
>>> 2 files changed, 20 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index 5d77571a8df9..3521838ab4fb 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -66,7 +66,6 @@ static u64 etm4_get_access_type(struct etmv4_config
>>> *config);
>>> static enum cpuhp_state hp_online;
>>> struct etm4_init_arg {
>>> - unsigned int pid;
>>> struct device *dev;
>>> struct csdev_access *csa;
>>> };
>>> @@ -370,8 +369,10 @@ static void etm4_disable_arch_specific(struct
>>> etmv4_drvdata *drvdata)
>>> }
>>> static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>>> - unsigned int id)
>>> + struct csdev_access *csa)
>>> {
>>> + unsigned int id = coresight_get_pid(csa);
>>> +
>>
>> This throws up the following error on an ETE.
>>
>> ete: trying to read unsupported register @fe0
>>
>> So, I guess this must be performed only for iomem based
>> devices. System instruction based device must be identified
>> by MIDR_EL1/REVIDR_EL1 if needed for specific erratum.
>> This is not required now. So, we could bail out early
>> if we are system instruction based device.
>
> Besides this, the PID is limited to (I think) 4 bits of ID. TRCIDRs
> offer revision information, but nothing manufacturer specific save for
> the designer. Register fields like MIDR_EL1 Variant + PartNum + Revision
> and TRCPIDR3 REVAND offer help. It may be a combination of registers are
> needed for a manufacturer to adequately ID a part to apply an erratum.
> Perhaps you could at least cache MIDR_EL1 for possible future use?
Like I said, if we ever need them, we could add it. I don't see a point
in storing it right now, if we don't use it.
Suzuki
© 2016 - 2026 Red Hat, Inc.