Re: [PATCH v2 3/5] coresight-tnoc: add nodes to configure flush

[Leo Yan <leo.yan@arm.com>]

On Wed, Feb 26, 2025 at 07:05:52PM +0800, Yuanfang Zhang wrote:
> 
> Two nodes for configure flush are added here:
> 1. flush_req: write 1 to initiates a flush sequence.
> 
> 2. flush_state: read this node to get flush status. 0: sequence in
> progress; 1: sequence has been completed.
> 
> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-tnoc.c | 73 ++++++++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tnoc.h |  4 ++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
> index fad8e61f05ef25989aba1be342c547f835e8953a..20231f28ddcb6a60d9b3c1ca3e0ca4d731dac39c 100644
> --- a/drivers/hwtracing/coresight/coresight-tnoc.c
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
> @@ -16,6 +16,78 @@
>  #include "coresight-tnoc.h"
>  #include "coresight-trace-id.h"
> 
> +static ssize_t flush_req_store(struct device *dev,
> +                              struct device_attribute *attr,
> +                              const char *buf,
> +                              size_t size)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +       struct coresight_device *csdev = drvdata->csdev;
> +       unsigned long val;
> +       u32 reg;
> +
> +       if (kstrtoul(buf, 10, &val))
> +               return -EINVAL;
> +
> +       if (val != 1)
> +               return -EINVAL;
> +
> +       spin_lock(&drvdata->spinlock);
> +       if (csdev->refcnt == 0) {
> +               spin_unlock(&drvdata->spinlock);
> +               return -EPERM;
> +       }
> +
> +       reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> +       reg = reg | TRACE_NOC_CTRL_FLUSHREQ;
> +       writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL);

How can userspace determine when to trigger a flush?

Generally, a driver kicks off a flush operation for a hardware before
reading data from buffer or when disable a link path.  I don't know the
hardware mechanism of TNOC, but seems to me, it does not make sense to
let the userspace to trigger a hardware flush, given the userspace has
no knowledge for device's state.

Furthermore, based on my understanding for patch 02 and 03, the working
flow is also concerned me.  IIUC, you want to use the driver to create
a linkage and then use userspace program to poll state and trigger
flushing.  Could you explain why use this way for managing the device?

Thanks,
Leo

> +
> +       spin_unlock(&drvdata->spinlock);
> +
> +       return size;
> +}
> +static DEVICE_ATTR_WO(flush_req);
> +
> +/*
> + * flush-sequence status:
> + * value 0: sequence in progress;
> + * value 1: sequence has been completed.
> + */
> +static ssize_t flush_status_show(struct device *dev,
> +                                struct device_attribute *attr,
> +                                char *buf)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +       struct coresight_device *csdev = drvdata->csdev;
> +       u32 val;
> +
> +       spin_lock(&drvdata->spinlock);
> +       if (csdev->refcnt == 0) {
> +               spin_unlock(&drvdata->spinlock);
> +               return -EPERM;
> +       }
> +
> +       val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> +       spin_unlock(&drvdata->spinlock);
> +       return sysfs_emit(buf, "%lu\n", BMVAL(val, 2, 2));
> +}
> +static DEVICE_ATTR_RO(flush_status);
> +
> +static struct attribute *trace_noc_attrs[] = {
> +       &dev_attr_flush_req.attr,
> +       &dev_attr_flush_status.attr,
> +       NULL,
> +};
> +
> +static struct attribute_group trace_noc_attr_grp = {
> +       .attrs = trace_noc_attrs,
> +};
> +
> +static const struct attribute_group *trace_noc_attr_grps[] = {
> +       &trace_noc_attr_grp,
> +       NULL,
> +};
> +
>  static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
>  {
>         u32 val;
> @@ -142,6 +214,7 @@ static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
>                 return ret;
> 
>         desc.ops = &trace_noc_cs_ops;
> +       desc.groups = trace_noc_attr_grps;
>         desc.type = CORESIGHT_DEV_TYPE_LINK;
>         desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
>         desc.pdata = adev->dev.platform_data;
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
> index b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8..d0fe8f52709ff4147d66dbf90987595012cfaa4e 100644
> --- a/drivers/hwtracing/coresight/coresight-tnoc.h
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
> @@ -10,6 +10,10 @@
> 
>  /* Enable generation of output ATB traffic.*/
>  #define TRACE_NOC_CTRL_PORTEN  BIT(0)
> +/* Writing 1 to initiate a flush sequence.*/
> +#define TRACE_NOC_CTRL_FLUSHREQ        BIT(1)
> +/* 0: sequence in progress; 1: sequence has been completed.*/
> +#define TRACE_NOC_CTRL_FLUSHSTATUS     BIT(2)
>  /* Writing 1 to issue a FREQ or FREQ_TS packet*/
>  #define TRACE_NOC_CTRL_FREQTSREQ       BIT(5)
>  /* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
> 
> --
> 2.34.1
> 
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org