Hi Jie,
[ + Rob ]
On Wed, Apr 02, 2025 at 08:55:51AM +0800, Jie Gan wrote:
[...]
> > > {
> > > - struct clk *pclk = NULL;
> > > + WARN_ON(!pclk);
> > > if (!dev_is_amba(dev)) {
> > > - pclk = devm_clk_get_enabled(dev, "apb_pclk");
> > > - if (IS_ERR(pclk))
> > > - pclk = devm_clk_get_enabled(dev, "apb");
> > > + *pclk = devm_clk_get_enabled(dev, "apb_pclk");
> > > + if (IS_ERR(*pclk))
> > > + *pclk = devm_clk_get_enabled(dev, "apb");
> > > + if (IS_ERR(*pclk))
> > > + return PTR_ERR(*pclk);
> > > + } else {
> > > + /* Don't enable pclk for an AMBA device */
> > > + *pclk = NULL;
> >
> > Now the "apb" clock won't be enabled for amba devices. I'm assuming
> > that's fine if the clock was always called "apb_pclk" for them, but the
> > commit that added the new clock name didn't specify any special casing
> > either.
> >
> > Can we have a comment that says it's deliberate? But the more I think
> > about it the more I'm confused why CTCU needed a different clock name to
> > be defined, when all the other Coresight devices use "apb_pclk".
>
> Hi James,
>
> The original clock-name for CTCU is apb_pclk, but the dt-binding maintainer
> request me to change it to apb, that's why the clock name is different from
> others.
>
> I am not why we need apb instead of apb_pclk in dt-binding. Maybe some rules
> have changed for dt-binding requirement.
My conclusion is that if a device is an Arm Primecell peripheral, it
should use the clock name "apb_pclk" (See the DT binding doc [1]).
CTCU is not an Arm Primecell peripheral, so it does not need to strictly
follow up the clock naming for Primecell peripheral.
In Arm CoreSight framework, for code consistency, I would suggest
using the clock naming "apb_pclk" for the APB clock for a newly added
device that even it is not a Primecell peripheral.
(We don't need to make any change to the CTCU driver, as we need to
remain compatible with existed DTB blobs).
Cc'ing Rob in case he has any suggestions.
Thanks,
Leo
[1] Documentation/devicetree/bindings/arm/primecell.yaml
This series is to enable AUX pause and resume on Arm CoreSight.
The first patch extracts the trace unit controlling operations to two
functions. These two functions will be used by AUX pause and resume.
Patches 02 and 03 change the ETMv4 driver to prepare callback functions
for AUX pause and resume.
Patch 04 changes the ETM perf layer to support AUX pause and resume in a
perf session. The patches 05 updates buffer on AUX pause occasion,
which can mitigate the trace data lose issue.
Patch 07 documents the AUX pause usages with Arm CoreSight.
This patch set has been verified on the Hikey960 board and TC platform.
The previous one uses ETR and the later uses TRBE as sink.
It is suggested to disable CPUIdle (add `nohlt` option in Linux command
line) when verifying this series. ETM and funnel drivers are found
issues during CPU suspend and resume which will be addressed separately.
Changes from v2:
- Rebased on CoreSight next branch.
- Dropped the uAPI 'update_buf_on_pause' and updated document
respectively (Suzuki).
- Renamed ETM callbacks to .pause_perf() and .resume_perf() (Suzuki).
- Minor improvement for error handling in the AUX resume flow.
Changes from v1:
- Added validation function pointers in pause and resume APIs (Mike).
Leo Yan (6):
coresight: etm4x: Extract the trace unit controlling
coresight: Introduce pause and resume APIs for source
coresight: etm4x: Hook pause and resume callbacks
coresight: perf: Support AUX trace pause and resume
coresight: perf: Update buffer on AUX pause
Documentation: coresight: Document AUX pause and resume
.../trace/coresight/coresight-perf.rst | 31 ++++
drivers/hwtracing/coresight/coresight-core.c | 22 +++
.../hwtracing/coresight/coresight-etm-perf.c | 84 +++++++++-
.../coresight/coresight-etm4x-core.c | 143 +++++++++++++-----
drivers/hwtracing/coresight/coresight-etm4x.h | 2 +
drivers/hwtracing/coresight/coresight-priv.h | 2 +
include/linux/coresight.h | 4 +
7 files changed, 246 insertions(+), 42 deletions(-)
--
2.34.1
Hi,
On Wed, 2 Apr 2025 at 01:50, Jie Gan <quic_jiegan(a)quicinc.com> wrote:
>
>
>
> On 4/1/2025 5:56 PM, Mike Leach wrote:
> > Hi,
> >
> > On Tue, 1 Apr 2025 at 07:11, Anshuman Khandual
> > <anshuman.khandual(a)arm.com> wrote:
> >>
> >> On 4/1/25 07:12, Jie Gan wrote:
> >>> The trace_id in coresight_path may contain an error number which means a
> >>> negative integer, but the current type of the trace_id is u8. Change the
> >>> type to int to fix it.
> >>>
> >>> Reported-by: Dan Carpenter <dan.carpenter(a)linaro.org>
> >>> Fixes: 3c03c49b2fa5 ("Coresight: Introduce a new struct coresight_path")
> >>> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
> >>
> >> LGTM
> >>
> >> Reviewed-by: Anshuman Khandual <anshuman.khandual(a)arm.com>
> >>
> >>> ---
> >>> include/linux/coresight.h | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> >>> index d79a242b271d..c2bf10c43e7c 100644
> >>> --- a/include/linux/coresight.h
> >>> +++ b/include/linux/coresight.h
> >>> @@ -337,7 +337,7 @@ static struct coresight_dev_list (var) = { \
> >>> */
> >>> struct coresight_path {
> >>> struct list_head path_list;
> >>> - u8 trace_id;
> >>> + int trace_id;
> >>> };
> >>>
> >>> enum cs_mode {
> >
> > There are many places in the Coresight drivers that assign a u8
> > traceid from the path trace ID.
> >
> > e.g.
> > In coresight-etm4x-core.c : etm4_enable_perf()
> >
> > drvdata->trcid = path->trace_id;
> >
> > drvdata->trcid is defined as a u8 - the reason being trace IDs are
> > 128 bits wide with some reserved values.
> >
> > Will this not just trigger the same issue if path->trace_id is changed
> > to an int? Even if not it is inconsistent handling of the trace ID
> > values.
> >
> > Trace ID errors should be handled by returning an invalid trace ID
> > value - were the trace ID value will fail the macro
> > IS_VALID_CS_TRACE_ID(), or separate the return of a trace ID from an
> > error return in a function.
> >
>
> Hi Mike,
>
> The path->trace_id is verified after it has been assigned with the logic
> you mentioned:
>
> if (!IS_VALID_CS_TRACE_ID(path->trace_id))
> goto err_path;
>
> So it should be safe to assign to another u8 parameter, like you mentioned:
>
> In coresight-etm4x-core.c : etm4_enable_perf()
>
> drvdata->trcid = path->trace_id;
>
It is safe but will it not trigger a warning just like the one you are
trying to fix as the types are mismatched?
Mike
> Thanks,
> Jie
>
>
> > Regards
> >
> > Mike
> >
> >
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
This series is to enable AUX pause and resume on Arm CoreSight.
The first patch extracts the trace unit controlling operations to two
functions. These two functions will be used by AUX pause and resume.
Patches 02 and 03 change the ETMv4 driver to prepare callback functions
for AUX pause and resume.
Patch 04 changes the ETM perf layer to support AUX pause and resume in a
perf session. The patch 05 re-enables sinks after buffer update, based
on it, the patch 06 updates buffer on AUX pause occasion, which can
mitigate the trace data lose issue.
Patch 07 documents the AUX pause usages with Arm CoreSight.
This patch set has been verified on the Hikey960 board.
It is suggested to disable CPUIdle (add `nohlt` option in Linux command
line) when verifying this series. ETM and funnel drivers are found
issues during CPU suspend and resume which will be addressed separately.
Changes from v3:
- Re-enabled sink in buffer update callbacks (Suzuki).
Changes from v2:
- Rebased on CoreSight next branch.
- Dropped the uAPI 'update_buf_on_pause' and updated document
respectively (Suzuki).
- Renamed ETM callbacks to .pause_perf() and .resume_perf() (Suzuki).
- Minor improvement for error handling in the AUX resume flow.
Changes from v1:
- Added validation function pointers in pause and resume APIs (Mike).
Leo Yan (7):
coresight: etm4x: Extract the trace unit controlling
coresight: Introduce pause and resume APIs for source
coresight: etm4x: Hook pause and resume callbacks
coresight: perf: Support AUX trace pause and resume
coresight: tmc: Re-enable sink after buffer update
coresight: perf: Update buffer on AUX pause
Documentation: coresight: Document AUX pause and resume
Documentation/trace/coresight/coresight-perf.rst | 31 +++++++++
drivers/hwtracing/coresight/coresight-core.c | 22 +++++++
drivers/hwtracing/coresight/coresight-etm-perf.c | 84 +++++++++++++++++++++++-
drivers/hwtracing/coresight/coresight-etm4x-core.c | 143 +++++++++++++++++++++++++++++------------
drivers/hwtracing/coresight/coresight-etm4x.h | 2 +
drivers/hwtracing/coresight/coresight-priv.h | 2 +
drivers/hwtracing/coresight/coresight-tmc-etf.c | 9 +++
drivers/hwtracing/coresight/coresight-tmc-etr.c | 10 +++
include/linux/coresight.h | 4 ++
9 files changed, 265 insertions(+), 42 deletions(-)
--
2.34.1
This series fixes and improves clock usage in the Arm CoreSight drivers.
Based on the DT binding documents, the trace clock (atclk) is defined in
some CoreSight modules, but the corresponding drivers to support it are
absent. In most cases, the issue is hidden because atclk is shared by
multiple CoreSight modules and is enabled anyway by other drivers.
The first three patches address this issue.
The programming clock (pclk) management in CoreSight drivers does not
use the devm_XXX() variant APIs, so the drivers needs to manually
disable and release clocks for errors and for normal module exit.
However, the drivers miss to disable clocks during module exit.
Another issue is pclk might be enabled twice in init phase - once by
AMBA bus driver, and again by CoreSight drivers.
Patches 04 and 05 fix pclk issues.
The atclk may also not be disabled in CoreSight drivers during module
exit. This is fixed in patch 06.
Patches 07 to 09 refactor the clock related code. Patch 07 makes the
clock enabling sequence consistent. Patch 08 removes redundant
condition checks and adds error handling in runtime PM. Patch 09
consolidats the clock initialization into a central place.
This series is verified on Arm64 Hikey960 platform.
Leo Yan (9):
coresight: tmc: Support atclk
coresight: catu: Support atclk
coresight: etm4x: Support atclk
coresight: Disable programming clock properly
coresight: Avoid enable programming clock duplicately
coresight: Disable trace bus clock properly
coresight: Make clock sequence consistent
coresight: Refactor runtime PM
coresight: Consolidate clock enabling
drivers/hwtracing/coresight/coresight-catu.c | 53 ++++++++++++++++-----------------
drivers/hwtracing/coresight/coresight-catu.h | 1 +
drivers/hwtracing/coresight/coresight-cpu-debug.c | 41 +++++++++-----------------
drivers/hwtracing/coresight/coresight-ctcu-core.c | 24 +++++----------
drivers/hwtracing/coresight/coresight-etb10.c | 18 ++++--------
drivers/hwtracing/coresight/coresight-etm3x-core.c | 17 ++++-------
drivers/hwtracing/coresight/coresight-etm4x-core.c | 32 ++++++++++----------
drivers/hwtracing/coresight/coresight-etm4x.h | 4 ++-
drivers/hwtracing/coresight/coresight-funnel.c | 66 +++++++++++++++---------------------------
drivers/hwtracing/coresight/coresight-replicator.c | 63 ++++++++++++++--------------------------
drivers/hwtracing/coresight/coresight-stm.c | 34 +++++++++-------------
drivers/hwtracing/coresight/coresight-tmc-core.c | 48 +++++++++++++++---------------
drivers/hwtracing/coresight/coresight-tmc.h | 2 ++
drivers/hwtracing/coresight/coresight-tpiu.c | 36 ++++++++++-------------
include/linux/coresight.h | 47 ++++++++++++++++++------------
15 files changed, 206 insertions(+), 280 deletions(-)
--
2.34.1
On 28/03/2025 10:38 pm, Yabin Cui wrote:
> When tracing ETM data on multiple CPUs concurrently via the
> perf interface, the CATU device is shared across different CPU
> paths. This can lead to race conditions when multiple CPUs attempt
> to enable or disable the CATU device simultaneously.
>
> To address these race conditions, this patch introduces the
> following changes:
>
> 1. The enable and disable operations for the CATU device are not
> reentrant. Therefore, a spinlock is added to ensure that only
> one CPU can enable or disable a given CATU device at any point
> in time.
>
> 2. A reference counter is used to manage the enable/disable state
> of the CATU device. The device is enabled when the first CPU
> requires it and is only disabled when the last CPU finishes
> using it. This ensures the device remains active as long as at
> least one CPU needs it.
>
> Signed-off-by: Yabin Cui <yabinc(a)google.com>
> ---
> drivers/hwtracing/coresight/coresight-catu.c | 29 ++++++++++++++------
> drivers/hwtracing/coresight/coresight-catu.h | 1 +
> 2 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index 275cc0d9f505..834a7ffbbdbc 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -458,12 +458,19 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
> static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
> void *data)
> {
> - int rc;
> + int rc = 0;
> + unsigned long flags;
> struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
>
> - CS_UNLOCK(catu_drvdata->base);
> - rc = catu_enable_hw(catu_drvdata, mode, data);
> - CS_LOCK(catu_drvdata->base);
> + spin_lock_irqsave(&catu_drvdata->spinlock, flags);
Hi Yabin,
This needs to be a raw_spinlock since [1]. Also you might as well use
the new guard() thing to save someone find-and-replacing it later.
But I'm wondering if this is accurate. The ETR's refcount is dependent
on the pid of the owner of the trace session:
/* Do not proceed if this device is associated with another session */
if (drvdata->pid != -1 && drvdata->pid != pid) {
rc = -EBUSY;
goto unlock_out;
}
/*
* No HW configuration is needed if the sink is already in
* use for this session.
*/
if (drvdata->pid == pid) {
csdev->refcnt++;
goto unlock_out;
}
If the helpers get enabled first, could this mean that CATU gets
associated with a different session than the ETR? Maybe not, but it
would be easier to understand if the core code handled the refcounting
and locking for linked devices.
[1]:
https://lore.kernel.org/all/20250306121110.1647948-3-yeoreum.yun@arm.com/
Thanks
James
> + if (csdev->refcnt == 0) {
> + CS_UNLOCK(catu_drvdata->base);
> + rc = catu_enable_hw(catu_drvdata, mode, data);
> + CS_LOCK(catu_drvdata->base);
> + }
> + if (!rc)
> + csdev->refcnt++;
> + spin_unlock_irqrestore(&catu_drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -486,12 +493,17 @@ static int catu_disable_hw(struct catu_drvdata *drvdata)
>
> static int catu_disable(struct coresight_device *csdev, void *__unused)
> {
> - int rc;
> + int rc = 0;
> + unsigned long flags;
> struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
>
> - CS_UNLOCK(catu_drvdata->base);
> - rc = catu_disable_hw(catu_drvdata);
> - CS_LOCK(catu_drvdata->base);
> + spin_lock_irqsave(&catu_drvdata->spinlock, flags);
> + if (--csdev->refcnt == 0) {
> + CS_UNLOCK(catu_drvdata->base);
> + rc = catu_disable_hw(catu_drvdata);
> + CS_LOCK(catu_drvdata->base);
> + }
> + spin_unlock_irqrestore(&catu_drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -550,6 +562,7 @@ static int __catu_probe(struct device *dev, struct resource *res)
> dev->platform_data = pdata;
>
> drvdata->base = base;
> + spin_lock_init(&drvdata->spinlock);
> catu_desc.access = CSDEV_ACCESS_IOMEM(base);
> catu_desc.pdata = pdata;
> catu_desc.dev = dev;
> diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
> index 141feac1c14b..663282ec6381 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.h
> +++ b/drivers/hwtracing/coresight/coresight-catu.h
> @@ -65,6 +65,7 @@ struct catu_drvdata {
> void __iomem *base;
> struct coresight_device *csdev;
> int irq;
> + spinlock_t spinlock;
> };
>
> #define CATU_REG32(name, offset) \
On 4/1/25 07:12, Jie Gan wrote:
> The trace_id in coresight_path may contain an error number which means a
> negative integer, but the current type of the trace_id is u8. Change the
> type to int to fix it.
>
> Reported-by: Dan Carpenter <dan.carpenter(a)linaro.org>
> Fixes: 3c03c49b2fa5 ("Coresight: Introduce a new struct coresight_path")
> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
LGTM
Reviewed-by: Anshuman Khandual <anshuman.khandual(a)arm.com>
> ---
> include/linux/coresight.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index d79a242b271d..c2bf10c43e7c 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -337,7 +337,7 @@ static struct coresight_dev_list (var) = { \
> */
> struct coresight_path {
> struct list_head path_list;
> - u8 trace_id;
> + int trace_id;
> };
>
> enum cs_mode {