Greg,
Please find a couple of fixes for coresight self-hosted tracing for v6.3. Kindly
consider pulling.
Suzuki
The following changes since commit eeac8ede17557680855031c6f305ece2378af326:
Linux 6.3-rc2 (2023-03-12 16:36:44 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-fixes-v6.3
for you to fetch changes up to 735e7b30a53a1679c050cddb73f5e5316105d2e3:
coresight: etm4x: Do not access TRCIDR1 for identification (2023-03-21 12:31:02 +0000)
----------------------------------------------------------------
coresight: Fixes for v6.3
Fixes for coresight subsystem includes:
- Fix etm4_enable_hw to program all the address comparator pairs (instead of
half of them)
- Do not access TRCIDR1 register without OSLK cleared in etm4_probe for mmio
access.
Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com>
----------------------------------------------------------------
Steve Clevenger (1):
coresight-etm4: Fix for() loop drvdata->nr_addr_cmp range bug
Suzuki K Poulose (1):
coresight: etm4x: Do not access TRCIDR1 for identification
drivers/hwtracing/coresight/coresight-etm4x-core.c | 24 +++++++++-------------
drivers/hwtracing/coresight/coresight-etm4x.h | 20 ++++++------------
2 files changed, 16 insertions(+), 28 deletions(-)
On 20/03/2023 14:17, Rob Herring wrote:
> On Thu, Mar 16, 2023 at 10:05 PM Anshuman Khandual
> <anshuman.khandual(a)arm.com> wrote:
>>
>> CoreSight ETM4x devices could be accessed either via MMIO (handled via
>> amba_driver) or CPU system instructions (handled via platform driver). But
>> this has the following issues :
>>
>> - Each new CPU comes up with its own PID and thus we need to keep on
>> adding the "known" PIDs to get it working with AMBA driver. While
>> the ETM4 architecture (and CoreSight architecture) defines way to
>> identify a device as ETM4. Thus older kernels won't be able to
>> "discover" a newer CPU, unless we add the PIDs.
>
> But v8.4 discourages MMIO access, so this problem will go away on its
> own. Even if not, adding IDs to stable kernels is standard practice
> whether it is PCI VID/PID, compatible string or AMBA PID.
Yes, it would eventually go away. As for adding the PIDs, the
fundamental issue is, unlike other drivers, except for the "PIDs"
everything else is architected and each CPU has this PID alone
different and we have plenty of CPUs implementaions out there.
But all that said, since we added this as an AMBA driver in the first
place (all for simply getting the apb_clk management), I am happy to
choose the "Add PIDs to stable kernel approach" for this problem.
>
>> - With ACPI, the ETM4x devices have the same HID to identify the device
>> irrespective of the mode of access. This creates a problem where two
>> different drivers (both AMBA based driver and platform driver) would
>> hook into the "HID" and could conflict. e.g., if AMBA driver gets
>> hold of a non-MMIO device, the probe fails. If we have single driver
>> hooked into the given "HID", we could handle them seamlessly,
>> irrespective of the mode of access.
>
> Why are we changing DT for ACPI? Just always use the platform driver
> for ACPI and leave DT systems alone.
This was mainly due to (1), given we have a platform driver anyway for
ACPI. As mentioned above, we could leave the DT alone.
>
>> - CoreSight is heavily dependent on the runtime power management. With
>> ACPI, amba_driver doesn't get us anywhere with handling the power
>> and thus one need to always turn the power ON to use them. Moving to
>> platform driver gives us the power management for free.
>
> This sounds like an issue for any amba driver. If this is an issue,
> solve it for everyone, not just work around it in one driver.
This alone wouldn't be sufficient. We need a platform driver anyway to
handle the two different modes in ACPI for ETMs. But this will be a
an option for the other CoreSight components which are always MMIO.
Thanks
Suzuki
>
> When someone puts another primecell device into an ACPI system, are we
> going to go do the same one-off change in that driver too? (We kind of
> already did with SBSA UART...)
>
> Rob
CoreSight ETM4x architecture clearly provides ways to identify a device
via registers in the "Management" class, TRCDEVARCH and TRCDEVTYPE. These
registers can be accessed without the Trace domain being powered on.
We additionally added TRCIDR1 as fallback in order to cover for any
ETMs that may not have implemented TRCDEVARCH. So far, nobody has
reported hitting a WARNING we placed to catch such systems.
Also, more importantly it is problematic to access TRCIDR1, which is a
"Trace" register via MMIO access, without clearing the OSLK. But we cannot
mess with the OSLK until we know for sure that this is an ETMv4 device.
Thus, this kind of creates a chicken and egg problem unnecessarily for
systems "which are compliant" to the ETMv4 architecture.
Let us remove the TRCIDR1 fall back check and rely only on TRCDEVARCH.
Reported-by: Steve Clevenger <scclevenger(a)os.amperecomputing.com>
Link: https://lore.kernel.org/all/143540e5623d4c7393d24833f2b80600d8d745d2.167788…
Cc: Mike Leach <mike.leach(a)linaro.org>
Cc: James Clark <james.clark(a)arm.com>
Fixes: 8b94db1edaee ("coresight: etm4x: Use TRCDEVARCH for component discovery")
Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com>
Reviewed-by: Mike Leach <mike.leach(a)linaro.org>
---
Changes since v2:
- Remove unused etm_tridr_to_arch() helper
- Add comment to explain why TRCIDR1 cannot be used.
---
.../coresight/coresight-etm4x-core.c | 22 ++++++++-----------
drivers/hwtracing/coresight/coresight-etm4x.h | 20 +++++------------
2 files changed, 15 insertions(+), 27 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 104333c2c8a3..4c15fae534f3 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1070,25 +1070,21 @@ static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
struct csdev_access *csa)
{
u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
- u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1);
/*
* All ETMs must implement TRCDEVARCH to indicate that
- * the component is an ETMv4. To support any broken
- * implementations we fall back to TRCIDR1 check, which
- * is not really reliable.
+ * the component is an ETMv4. Even though TRCIDR1 also
+ * contains the information, it is part of the "Trace"
+ * register and must be accessed with the OSLK cleared,
+ * with MMIO. But we cannot touch the OSLK until we are
+ * sure this is an ETM. So rely only on the TRCDEVARCH.
*/
- if ((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) {
- drvdata->arch = etm_devarch_to_arch(devarch);
- } else {
- pr_warn("CPU%d: ETM4x incompatible TRCDEVARCH: %x, falling back to TRCIDR1\n",
- smp_processor_id(), devarch);
-
- if (ETM_TRCIDR1_ARCH_MAJOR(idr1) != ETM_TRCIDR1_ARCH_ETMv4)
- return false;
- drvdata->arch = etm_trcidr_to_arch(idr1);
+ if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH) {
+ pr_warn_once("TRCDEVARCH doesn't match ETMv4 architecture\n");
+ return false;
}
+ drvdata->arch = etm_devarch_to_arch(devarch);
*csa = CSDEV_ACCESS_IOMEM(drvdata->base);
return true;
}
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 434f4e95ee17..27c8a9901868 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -753,14 +753,12 @@
* TRCDEVARCH - CoreSight architected register
* - Bits[15:12] - Major version
* - Bits[19:16] - Minor version
- * TRCIDR1 - ETM architected register
- * - Bits[11:8] - Major version
- * - Bits[7:4] - Minor version
- * We must rely on TRCDEVARCH for the version information,
- * however we don't want to break the support for potential
- * old implementations which might not implement it. Thus
- * we fall back to TRCIDR1 if TRCDEVARCH is not implemented
- * for memory mapped components.
+ *
+ * We must rely only on TRCDEVARCH for the version information. Even though,
+ * TRCIDR1 also provides the architecture version, it is a "Trace" register
+ * and as such must be accessed only with Trace power domain ON. This may
+ * not be available at probe time.
+ *
* Now to make certain decisions easier based on the version
* we use an internal representation of the version in the
* driver, as follows :
@@ -786,12 +784,6 @@ static inline u8 etm_devarch_to_arch(u32 devarch)
ETM_DEVARCH_REVISION(devarch));
}
-static inline u8 etm_trcidr_to_arch(u32 trcidr1)
-{
- return ETM_ARCH_VERSION(ETM_TRCIDR1_ARCH_MAJOR(trcidr1),
- ETM_TRCIDR1_ARCH_MINOR(trcidr1));
-}
-
enum etm_impdef_type {
ETM4_IMPDEF_HISI_CORE_COMMIT,
ETM4_IMPDEF_FEATURE_MAX,
--
2.34.1
CoreSight ETM4x architecture clearly provides ways to identify a device
via registers in the "Management" class, TRCDEVARCH and TRCDEVTYPE. These
registers can be accessed without the Trace domain being powered on.
We additionally added TRCIDR1 as fallback in order to cover for any
ETMs that may not have implemented TRCDEVARCH. So far, nobody has
reported hitting a WARNING we placed to catch such systems.
Also, more importantly it is problematic to access TRCIDR1, which is a "Trace"
register via MMIO access, without clearing the OSLK. But we cannot
mess with the OSLK until we know for sure that this is an ETMv4 device.
Thus, this kind of creates a chicken and egg problem unnecessarily for systems
"which are compliant" to the ETMv4 architecture.
Let us remove the TRCIDR1 fall back check and rely only on TRCDEVARCH.
Reported-by: Steve Clevenger <scclevenger(a)os.amperecomputing.com>
Link: https://lore.kernel.org/all/143540e5623d4c7393d24833f2b80600d8d745d2.167788…
Cc: Mike Leach <mike.leach(a)linaro.org>
Cc: James Clark <james.clark(a)arm.com>
Fixes: 8b94db1edaee ("coresight: etm4x: Use TRCDEVARCH for component discovery")
Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com>
---
.../hwtracing/coresight/coresight-etm4x-core.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 104333c2c8a3..c1b72d892d7d 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1070,25 +1070,17 @@ static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
struct csdev_access *csa)
{
u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
- u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1);
/*
* All ETMs must implement TRCDEVARCH to indicate that
- * the component is an ETMv4. To support any broken
- * implementations we fall back to TRCIDR1 check, which
- * is not really reliable.
+ * the component is an ETMv4
*/
- if ((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) {
- drvdata->arch = etm_devarch_to_arch(devarch);
- } else {
- pr_warn("CPU%d: ETM4x incompatible TRCDEVARCH: %x, falling back to TRCIDR1\n",
- smp_processor_id(), devarch);
-
- if (ETM_TRCIDR1_ARCH_MAJOR(idr1) != ETM_TRCIDR1_ARCH_ETMv4)
- return false;
- drvdata->arch = etm_trcidr_to_arch(idr1);
+ if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH) {
+ pr_warn_once("TRCDEVARCH doesn't match ETMv4 architecture\n");
+ return false;
}
+ drvdata->arch = etm_devarch_to_arch(devarch);
*csa = CSDEV_ACCESS_IOMEM(drvdata->base);
return true;
}
--
2.34.1
On 17/03/2023 20:06, Rob Herring wrote:
> On Fri, Mar 17, 2023 at 11:03 AM Suzuki K Poulose
> <suzuki.poulose(a)arm.com> wrote:
>>
>> Hi Rob
>>
>> Thanks for your response.
>>
>> On 17/03/2023 14:52, Rob Herring wrote:
>>> On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
>>> <anshuman.khandual(a)arm.com> wrote:
>>>>
>>>> Allow other drivers to claim a device, disregarding the "priority" of
>>>> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
>>>> (AMBA Bus) or via CPU system instructions.
>>>
>>> The OS can pick which one, use both, or this is a system integration
>>> time decision?
>>
>> Not an OS choice. Historically, this has always been MMIO accessed but
>> with v8.4 TraceFiltering support, CPUs are encouraged to use system
>> instructions and obsolete MMIO. So, yes, MMIO is still possible but
>> something that is discouraged and have to be decided at system
>> integration time.
>>
>>>
>>>> The CoreSight ETM4x platform
>>>> driver can now handle both types of devices. In order to make sure the
>>>> driver gets to handle the "MMIO based" devices, which always had the
>>>> "arm,primecell" compatible, we have two options :
>>>>
>>>> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
>>>> for an older kernel without the support.
>>>>
>>>> 2) The other option is to allow OF code to "ignore" the arm,primecell
>>>> priority for a selected list of compatibles. This would make sure that
>>>> both older kernels and the new kernels work fine without breaking
>>>> the functionality. The new DTS could always have the "arm,primecell"
>>>> removed.
>>>
>>> 3) Drop patches 6 and 7 and just register as both AMBA and platform
>>> drivers. It's just some extra boilerplate. I would also do different
>>> compatible strings for CPU system instruction version (assuming this
>>> is an integration time decision).
>>
>> The system instruction (and the reigster layouts) are all part of the
>> ETMv4/ETE architecture and specific capabilities/features are
>> discoverable, just like the Arm CPUs. Thus we don't need special
>> versions within the ETMv4x or ETE minor versions. As of now, we have
>> one for etm4x and another for ete.
>
> I just meant 2 new compatible strings. One each for ETMv4x and ETE,
> but different from the 2 existing ones. It is different h/w presented
> to the OS, so different compatible.
>
Sorry, was not very clear here.
Right now, we have :
1) arm,coresight-etm4x && arm,primecell - For AMBA based devices
2) arm,coresight-etm4x-sysreg - For system instruction based
3) arm,embedded-trace-extension - For ETE
>> One problem with the AMBA driver in place is having to keep on adding
>> new PIDs for the CPUs. The other option is to have a blanket mask
>> for matching the PIDs with AMBA_UCI_ID checks.
>
> But if MMIO access is discouraged, then new h/w would use the platform
> driver(s), not the amba driver, and you won't have to add PIDs.
Yes for v8.4 onwards. Alternatively, the newer DTS could skip
arm,primecell in the entry and that would kick the platform driver
in. So, that should be fine I guess.
Kind regards
Suzuki
>
> Rob
Hi Rob
Thanks for your response.
On 17/03/2023 14:52, Rob Herring wrote:
> On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
> <anshuman.khandual(a)arm.com> wrote:
>>
>> Allow other drivers to claim a device, disregarding the "priority" of
>> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
>> (AMBA Bus) or via CPU system instructions.
>
> The OS can pick which one, use both, or this is a system integration
> time decision?
Not an OS choice. Historically, this has always been MMIO accessed but
with v8.4 TraceFiltering support, CPUs are encouraged to use system
instructions and obsolete MMIO. So, yes, MMIO is still possible but
something that is discouraged and have to be decided at system
integration time.
>
>> The CoreSight ETM4x platform
>> driver can now handle both types of devices. In order to make sure the
>> driver gets to handle the "MMIO based" devices, which always had the
>> "arm,primecell" compatible, we have two options :
>>
>> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
>> for an older kernel without the support.
>>
>> 2) The other option is to allow OF code to "ignore" the arm,primecell
>> priority for a selected list of compatibles. This would make sure that
>> both older kernels and the new kernels work fine without breaking
>> the functionality. The new DTS could always have the "arm,primecell"
>> removed.
>
> 3) Drop patches 6 and 7 and just register as both AMBA and platform
> drivers. It's just some extra boilerplate. I would also do different
> compatible strings for CPU system instruction version (assuming this
> is an integration time decision).
The system instruction (and the reigster layouts) are all part of the
ETMv4/ETE architecture and specific capabilities/features are
discoverable, just like the Arm CPUs. Thus we don't need special
versions within the ETMv4x or ETE minor versions. As of now, we have
one for etm4x and another for ete.
One problem with the AMBA driver in place is having to keep on adding
new PIDs for the CPUs. The other option is to have a blanket mask
for matching the PIDs with AMBA_UCI_ID checks.
>
>>
>> This patch implements Option (2).
>>
>> Cc: Rob Herring <robh+dt(a)kernel.org>
>> Cc: Frank Rowand <frowand.list(a)gmail.com>
>> Cc: Russell King (Oracle) <linux(a)armlinux.org.uk>
>> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
>> Cc: devicetree(a)vger.kernel.org
>> Cc: linux-kernel(a)vger.kernel.org
>> Co-developed-by: Suzuki Poulose <suzuki.poulose(a)arm.com>
>> Signed-off-by: Suzuki Poulose <suzuki.poulose(a)arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual(a)arm.com>
>> ---
>> drivers/of/platform.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index b2bd2e783445..59ff1a38ccaa 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -325,6 +325,13 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
>> return NULL;
>> }
>>
>> +static const struct of_device_id of_ignore_amba_table[] = {
>> +#ifdef CONFIG_CORESIGHT_SOURCE_ETM4X
>> + { .compatible = "arm,coresight-etm4x" },
>> +#endif
>> + {} /* NULL terminated */
>> +};
>> +
>> /**
>> * of_platform_bus_create() - Create a device for a node and its children.
>> * @bus: device node of the bus to instantiate
>> @@ -373,7 +380,8 @@ static int of_platform_bus_create(struct device_node *bus,
>> platform_data = auxdata->platform_data;
>> }
>>
>> - if (of_device_is_compatible(bus, "arm,primecell")) {
>> + if (of_device_is_compatible(bus, "arm,primecell") &&
>> + unlikely(!of_match_node(of_ignore_amba_table, bus))) {
>
> of_match_node is going to take orders of magnitude longer than any
> difference unlikely() would make. Drop it.
Agreed.
Suzuki
>
>> /*
>> * Don't return an error here to keep compatibility with older
>> * device tree files.
>> --
>> 2.25.1
>>
CoreSight ETM4x devices could be accessed either via MMIO (handled via
amba_driver) or CPU system instructions (handled via platform driver). But
this has the following issues :
- Each new CPU comes up with its own PID and thus we need to keep on
adding the "known" PIDs to get it working with AMBA driver. While
the ETM4 architecture (and CoreSight architecture) defines way to
identify a device as ETM4. Thus older kernels won't be able to
"discover" a newer CPU, unless we add the PIDs.
- With ACPI, the ETM4x devices have the same HID to identify the device
irrespective of the mode of access. This creates a problem where two
different drivers (both AMBA based driver and platform driver) would
hook into the "HID" and could conflict. e.g., if AMBA driver gets
hold of a non-MMIO device, the probe fails. If we have single driver
hooked into the given "HID", we could handle them seamlessly,
irrespective of the mode of access.
- CoreSight is heavily dependent on the runtime power management. With
ACPI, amba_driver doesn't get us anywhere with handling the power
and thus one need to always turn the power ON to use them. Moving to
platform driver gives us the power management for free.
Due to all of the above, we are moving the MMIO based etm4x devices to be
supported via platform driver. The series makes the existing platform
driver generic to handle both type of the access modes. With that we can
also remove the etm4x amba driver.
Finally, we need a way to make sure the new driver gets control of the
ETM4x device on a DT based system. CoreSight devices have always had the
"arm,primecell" in the compatible list. But the way this is handled
currently in OF code is a bit messy. The ETM4x devices are identified by
"arm,coresight-etm4x". The platform driver can never get a chance to probe
these devices, since the "arm,primecell" takes priority and is hard-coded
in the OF code. We have two options here :
1) Remove the arm,primecell from all DTS. This is fine for "new" kernels
with this change. But, for existing boards, using an older kernel will
break. Thus, is not preferred.
2) Add a white list of "compatibles" where the "priority" of the
"arm,primecell" can be ignored.
The series implements (2) above and applies on 6.3-rc2.
Cc: Steve Clevenger <scclevenger(a)os.amperecomputing.com>
Cc: Rob Herring <robh+dt(a)kernel.org>
Cc: Frank Rowand <frowand.list(a)gmail.com>
Cc: Russell King (Oracle) <linux(a)armlinux.org.uk>
Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael(a)kernel.org>
Cc: Len Brown <lenb(a)kernel.org>
Cc: Sudeep Holla <sudeep.holla(a)arm.com>
Cc: Lorenzo Pieralisi <lpieralisi(a)kernel.org>
Cc: Mathieu Poirier <mathieu.poirier(a)linaro.org>
Cc: Suzuki K Poulose <suzuki.poulose(a)arm.com>
Cc: Mike Leach <mike.leach(a)linaro.org>
Cc: Leo Yan <leo.yan(a)linaro.org>
Cc: devicetree(a)vger.kernel.org
Cc: linux-acpi(a)vger.kernel.org
Cc: coresight(a)lists.linaro.org
Cc: linux-arm-kernel(a)lists.infradead.org
Cc: linux-kernel(a)vger.kernel.org
Anshuman Khandual (6):
coresight: etm4x: Allocate and device assign 'struct etmv4_drvdata' earlier
coresight: etm4x: Drop iomem 'base' argument from etm4_probe()
coresight: etm4x: Drop pid argument from etm4_probe()
coresight: etm4x: Change etm4_platform_driver driver for MMIO devices
of/platform: Skip coresight etm4x devices from AMBA bus
coresight: etm4x: Drop the AMBA driver
Suzuki Poulose (1):
coresight: etm4x: Add ACPI support in platform driver
drivers/acpi/acpi_amba.c | 1 -
.../coresight/coresight-etm4x-core.c | 171 ++++++++----------
drivers/hwtracing/coresight/coresight-etm4x.h | 3 +
drivers/of/platform.c | 10 +-
include/linux/coresight.h | 56 ++++++
5 files changed, 143 insertions(+), 98 deletions(-)
--
2.25.1