CoreSight

coresight@lists.linaro.org

6 participants
2303 discussions

Re: [PATCH v2 1/3] coresight: catu: Introduce refcount and spinlock for enabling/disabling

by Suzuki K Poulose

On 03/04/2025 18:53, Yabin Cui wrote: > On Wed, Apr 2, 2025 at 6:01 AM Leo Yan <leo.yan(a)arm.com> wrote: >> >> Hi Yabin, >> >> On Tue, Apr 01, 2025 at 06:21:59PM -0700, Yabin Cui wrote: >> >> [...] >> >>>> @@ -486,12 +491,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; >>>> struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); >>>> + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); >>>> >>>> - CS_UNLOCK(catu_drvdata->base); >>>> - rc = catu_disable_hw(catu_drvdata); >>>> - CS_LOCK(catu_drvdata->base); >>>> + if (--csdev->refcnt == 0) { >>>> + CS_UNLOCK(catu_drvdata->base); >>>> + rc = catu_disable_hw(catu_drvdata); >>>> + CS_LOCK(catu_drvdata->base); >>>> + } else { >>>> + rc = -EBUSY; >> >> This is not an error if the decremented reference counter is not zero. >> It should return 0. Otherwise, the change looks good to me. > > In coresight_disable_helpers(), the return value of catu_disable() > isn't checked. > The -EBUSY return was used for consistency with other refcounted > disable functions > like tmc_disable_etf_sink() and tmc_disable_etr_sink(). I'm happy to > change it back > to 0 if you believe that would be the more accurate return value here. Please stick to 0 here. This indicates there was no errors w.r.t the current session. This is similar to what we do for TMC ETR for e.g. Suzuki > >> >> Thanks, >> Leo

1 week, 5 days

[PATCH v1 0/9] coresight: Fix and improve clock usage

by Leo Yan

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

1 week, 6 days

[PATCH v4 0/7] Arm CoreSight: Support AUX pause and resume

by Leo Yan

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

2 weeks

[PATCH] coresight: Break to error handler if connection can't be made

by James Clark

Change the return to a break so that the of_node_put()s in the error handler are hit on failure. Fixes: 3d4ff657e454 ("coresight: Dynamically add connections") Reported-by: Dan Carpenter <dan.carpenter(a)linaro.org> Closes: https://lore.kernel.org/linux-arm-kernel/3b026b3f-2cb2-49ef-aa20-8b14220f53… Signed-off-by: James Clark <james.clark(a)linaro.org> --- drivers/hwtracing/coresight/coresight-platform.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 8192ba3279f0..39851a13ff89 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -267,7 +267,8 @@ static int of_coresight_parse_endpoint(struct device *dev, new_conn = coresight_add_out_conn(dev, pdata, &conn); if (IS_ERR_VALUE(new_conn)) { fwnode_handle_put(conn.dest_fwnode); - return PTR_ERR(new_conn); + ret = PTR_ERR(new_conn); + break; } /* Connection record updated */ } while (0); --- base-commit: 5442d22da7dbff3ba8c6720fc6f23ea4934d402d change-id: 20250402-james-cs-ret-break-fix-ad38103af5f6 Best regards, -- James Clark <james.clark(a)linaro.org>

2 weeks

Re: [PATCH v2 3/3] coresight: core: Disable helpers for devices that fail to enable

by Leo Yan

On Tue, Apr 01, 2025 at 06:22:46PM -0700, Yabin Cui wrote: > On Tue, Apr 1, 2025 at 6:19 PM Yabin Cui <yabinc(a)google.com> wrote: > > > > When enabling a SINK or LINK type coresight device fails, the > > associated helpers should be disabled. > > > > Signed-off-by: Yabin Cui <yabinc(a)google.com> > > Suggested-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> > > --- > > drivers/hwtracing/coresight/coresight-core.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > > index fb43ef6a3b1f..e3270d9b46c9 100644 > > --- a/drivers/hwtracing/coresight/coresight-core.c > > +++ b/drivers/hwtracing/coresight/coresight-core.c > > @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, > > /* Enable all helpers adjacent to the path first */ > > ret = coresight_enable_helpers(csdev, mode, path); > > if (ret) > > - goto err; > > + goto err_helper; > > /* > > * ETF devices are tricky... They can be a link or a sink, > > * depending on how they are configured. If an ETF has been > > @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, > > switch (type) { > > case CORESIGHT_DEV_TYPE_SINK: > > ret = coresight_enable_sink(csdev, mode, sink_data); > > - /* > > - * Sink is the first component turned on. If we > > - * failed to enable the sink, there are no components > > - * that need disabling. Disabling the path here > > - * would mean we could disrupt an existing session. > > - */ > > if (ret) > > - goto out; > > + goto err; > > break; > > case CORESIGHT_DEV_TYPE_SOURCE: > > /* sources are enabled from either sysFS or Perf */ > > @@ -507,6 +501,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, > > out: > > return ret; > > err: > > + coresight_disable_helpers(csdev); Given it is unconditionally to enable helpers, I would suggest we disable helper unconditionally. Thus, we don't need to add a new 'err_helper' tag, simply reuse the 'err' tag would be fine. I would like to know if mainatiners have different opinions. Thanks, Leo > > +err_helper: > > coresight_disable_path_from(path, nd); > > goto out; > > } > > -- > > 2.49.0.472.ge94155a9ec-goog > >

2 weeks

Re: [PATCH v2 2/3] coresight: catu: Prevent concurrent PERF and SYSFS mode enablement

by Leo Yan

On Tue, Apr 01, 2025 at 06:18:31PM -0700, Yabin Cui wrote: > A CATU device can be enabled for either PERF mode or SYSFS mode, but > not both simultaneously. Consider the following race condition: > > CPU 1 enables CATU for PERF mode. > CPU 2 enables CATU for SYSFS mode. It only increases refcnt. > CPU 2 enables ETR for SYSFS mode. > CPU 1 fails to enable ETR for PERF mode. > > This results in a CATU being enabled in PERF mode and an ETR enabled > in SYSFS mode, leading to unknown behavior. > > To prevent this situation, this patch checks enabled mode of a > CATU device before increasing its refcnt. Yes. We have also observed the same issue. I am currently working on refactoring the Arm CoreSight locking, my plan is to use coresight_path to maintain mode. Given it is uncommon case to use two modes concurrently in Arm CoreSight, I assume this is not an urgent issue. Could we defer this patch? My understanding is that this issue will be prevented with checking the path mode, rather than checking modes in seperate modules. To be clear, I am fine with other two patches in the series. Thanks, Leo > Signed-off-by: Yabin Cui <yabinc(a)google.com> > Suggested-by: James Clark <james.clark(a)linaro.org> > Suggested-by: Leo Yan <leo.yan(a)arm.com> > --- > drivers/hwtracing/coresight/coresight-catu.c | 6 +++++- > drivers/hwtracing/coresight/coresight-catu.h | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c > index b1d490dd7249..0caf3a3e75ce 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.c > +++ b/drivers/hwtracing/coresight/coresight-catu.c > @@ -466,7 +466,10 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, > CS_UNLOCK(catu_drvdata->base); > rc = catu_enable_hw(catu_drvdata, mode, data); > CS_LOCK(catu_drvdata->base); > - } > + if (!rc) > + catu_drvdata->mode = mode; > + } else if (catu_drvdata->mode != mode) > + rc = -EBUSY; > if (!rc) > csdev->refcnt++; > return rc; > @@ -499,6 +502,7 @@ static int catu_disable(struct coresight_device *csdev, void *__unused) > CS_UNLOCK(catu_drvdata->base); > rc = catu_disable_hw(catu_drvdata); > CS_LOCK(catu_drvdata->base); > + catu_drvdata->mode = CS_MODE_DISABLED; > } else { > rc = -EBUSY; > } > diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h > index 755776cd19c5..ea406464f0a8 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.h > +++ b/drivers/hwtracing/coresight/coresight-catu.h > @@ -66,6 +66,7 @@ struct catu_drvdata { > struct coresight_device *csdev; > int irq; > raw_spinlock_t spinlock; > + enum cs_mode mode; > }; > > #define CATU_REG32(name, offset) \ > -- > 2.49.0.472.ge94155a9ec-goog >

2 weeks

Re: [PATCH v2 1/3] coresight: catu: Introduce refcount and spinlock for enabling/disabling

by Leo Yan

Hi Yabin, On Tue, Apr 01, 2025 at 06:21:59PM -0700, Yabin Cui wrote: [...] > > @@ -486,12 +491,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; > > struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); > > + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); > > > > - CS_UNLOCK(catu_drvdata->base); > > - rc = catu_disable_hw(catu_drvdata); > > - CS_LOCK(catu_drvdata->base); > > + if (--csdev->refcnt == 0) { > > + CS_UNLOCK(catu_drvdata->base); > > + rc = catu_disable_hw(catu_drvdata); > > + CS_LOCK(catu_drvdata->base); > > + } else { > > + rc = -EBUSY; This is not an error if the decremented reference counter is not zero. It should return 0. Otherwise, the change looks good to me. Thanks, Leo

2 weeks

[PATCH v3 0/6] Arm CoreSight: Support AUX pause and resume

by Leo Yan

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

2 weeks

Re: [bug report] coresight: Dynamically add connections

by James Clark

On 02/04/2025 12:00 pm, Dan Carpenter wrote: > Hello James Clark, > > Commit 3d4ff657e454 ("coresight: Dynamically add connections") from > Apr 25, 2023 (linux-next), leads to the following Smatch static > checker warning: > > drivers/hwtracing/coresight/coresight-platform.c:279 of_coresight_parse_endpoint() > warn: inconsistent refcounting 'rep->kobj.kref.refcount.refs.counter': > > drivers/hwtracing/coresight/coresight-platform.c > 196 static int of_coresight_parse_endpoint(struct device *dev, > 197 struct device_node *ep, > 198 struct coresight_platform_data *pdata) > 199 { > 200 int ret = 0; > 201 struct of_endpoint endpoint, rendpoint; > 202 struct device_node *rparent = NULL; > 203 struct device_node *rep = NULL; > 204 struct device *rdev = NULL; > 205 struct fwnode_handle *rdev_fwnode; > 206 struct coresight_connection conn = {}; > 207 struct coresight_connection *new_conn; > 208 > 209 do { > 210 /* Parse the local port details */ > 211 if (of_graph_parse_endpoint(ep, &endpoint)) > 212 break; > 213 /* > 214 * Get a handle on the remote endpoint and the device it is > 215 * attached to. > 216 */ > 217 rep = of_graph_get_remote_endpoint(ep); > 218 if (!rep) > 219 break; > 220 rparent = of_coresight_get_port_parent(rep); > 221 if (!rparent) > 222 break; > 223 if (of_graph_parse_endpoint(rep, &rendpoint)) > 224 break; > 225 > 226 rdev_fwnode = of_fwnode_handle(rparent); > 227 /* If the remote device is not available, defer probing */ > 228 rdev = coresight_find_device_by_fwnode(rdev_fwnode); > 229 if (!rdev) { > 230 ret = -EPROBE_DEFER; > 231 break; > 232 } > 233 > 234 conn.src_port = endpoint.port; > 235 /* > 236 * Hold the refcount to the target device. This could be > 237 * released via: > 238 * 1) coresight_release_platform_data() if the probe fails or > 239 * this device is unregistered. > 240 * 2) While removing the target device via > 241 * coresight_remove_match() > 242 */ > 243 conn.dest_fwnode = fwnode_handle_get(rdev_fwnode); > 244 conn.dest_port = rendpoint.port; > 245 > 246 /* > 247 * Get the firmware node of the filter source through the > 248 * reference. This could be used to filter the source in > 249 * building path. > 250 */ > 251 conn.filter_src_fwnode = > 252 fwnode_find_reference(&ep->fwnode, "filter-source", 0); > 253 if (IS_ERR(conn.filter_src_fwnode)) { > 254 conn.filter_src_fwnode = NULL; > 255 } else { > 256 conn.filter_src_dev = > 257 coresight_find_csdev_by_fwnode(conn.filter_src_fwnode); > 258 if (conn.filter_src_dev && > 259 !coresight_is_device_source(conn.filter_src_dev)) { > 260 dev_warn(dev, "port %d: Filter handle is not a trace source : %s\n", > 261 conn.src_port, dev_name(&conn.filter_src_dev->dev)); > 262 conn.filter_src_dev = NULL; > 263 conn.filter_src_fwnode = NULL; > 264 } > 265 } > 266 > 267 new_conn = coresight_add_out_conn(dev, pdata, &conn); > 268 if (IS_ERR_VALUE(new_conn)) { > 269 fwnode_handle_put(conn.dest_fwnode); > 270 return PTR_ERR(new_conn); > > Should this be a break statement instead of return statement? > Hi Dan, Yes it probably should, I'll send a patch. Thanks James

2 weeks

Re: [PATCH v1 9/9] coresight: Consolidate clock enabling

by Leo Yan

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

2 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight