On 12/15/2024 6:21 AM, Konrad Dybcio wrote:
On 10.12.2024 4:15 AM, Jie Gan wrote:
The Coresight TMC Control Unit hosts miscellaneous configuration registers
which control various features related to TMC ETR sink.
Based on the trace ID, which is programmed in the related CTCU ATID
register of a specific ETR, trace data with that trace ID gets into
the ETR buffer, while other trace data gets dropped.
Enabling source device sets one bit of the ATID register based on
source device's trace ID.
Disabling source device resets the bit according to the source
device's trace ID.
Signed-off-by: Jie Gan <quic_jiegan@xxxxxxxxxxx>
---
[...]
+static int __ctcu_set_etr_traceid(struct coresight_device *csdev,
+ u8 traceid,
+ int port_num,
+ bool enable)
+{
+ uint32_t atid_offset;
+ struct ctcu_drvdata *drvdata;
+ unsigned long flags;
+ uint32_t reg_offset;
+ int bit;
+ uint32_t val;
+
+ if (!IS_VALID_CS_TRACE_ID(traceid) || port_num < 0)
+ return -EINVAL;
+
+ drvdata = dev_get_drvdata(csdev->dev.parent);
+ if (IS_ERR_OR_NULL(drvdata))
+ return -EINVAL;
+
+ atid_offset = drvdata->atid_offset[port_num];
+ if (atid_offset == 0)
+ return -EINVAL;
+
+ spin_lock_irqsave(&drvdata->spin_lock, flags);
guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
and drop the unlocks
Sure, will fix in next version.
+ CS_UNLOCK(drvdata->base);
+
+ reg_offset = CTCU_ATID_REG_OFFSET(traceid, atid_offset);
+ bit = CTCU_ATID_REG_BIT(traceid);
+ if (reg_offset - atid_offset >= CTCU_ATID_REG_SIZE ||
+ bit >= CORESIGHT_TRACE_IDS_MAX) {
+ CS_LOCK(drvdata);
+ spin_unlock_irqrestore(&drvdata->spin_lock, flags);
+ return -EINVAL;
+ }
+
+ val = ctcu_readl(drvdata, reg_offset);
+ if (enable)
+ val = val | BIT(bit);
+ else
+ val = val & ~BIT(bit);
+ ctcu_writel(drvdata, val, reg_offset);
+
+ CS_LOCK(drvdata->base);
+ spin_unlock_irqrestore(&drvdata->spin_lock, flags);
+
+ return 0;
+}
+
+static int ctcu_get_active_port(struct coresight_device *sink, struct coresight_device *helper)
+{
+ int port, i;
+
+ for (i = 0; i < sink->pdata->nr_outconns; ++i) {
+ if (sink->pdata->out_conns[i]->dest_dev) {
+ port = sink->pdata->out_conns[i]->dest_port;
+ return port;
Return sink->....
Sure, will fix in next version.
+ }
+ }
+
+ return -EINVAL;
+}
+
+/*
+ * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
+ *
+ * Returns 0 indicates success. None-zero result means failure.
+ */
+static int ctcu_set_etr_traceid(struct coresight_device *csdev,
+ struct cs_sink_data *sink_data,
+ bool enable)
+{
+ int port_num;
+
+ if (!IS_VALID_CS_TRACE_ID(sink_data->traceid) ||
+ (csdev == NULL) ||
I'm not sure this can be null by the time it reaches this function
Will check.
+ (sink_data->sink == NULL)) {
+ dev_dbg(&csdev->dev, "Invalid parameters\n");
dev_err?
It's better with dev_err, will fix.
+ return -EINVAL;
+ }
+
+ port_num = ctcu_get_active_port(sink_data->sink, csdev);
+ if (port_num < 0)
+ return -EINVAL;
+
+ dev_dbg(&csdev->dev, "traceid is %d\n", sink_data->traceid);
+
+ return __ctcu_set_etr_traceid(csdev, sink_data->traceid, port_num, enable);
+}
+
+static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode,
+ void *data)
+{
+ int ret = 0;
Unnecessary initialization (you instantly overwrite it), also below
+ struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+
+ ret = ctcu_set_etr_traceid(csdev, sink_data, true);
+ if (ret)
+ dev_dbg(&csdev->dev, "enable data filter failed\n");
Since the this function returns an int, maybe return ctcu_set_etr_traceid()
and let the upper layer throw an error (also for some other functions in this
file)
Sure, will check.
+
+ return 0;
+}
+
+static int ctcu_disable(struct coresight_device *csdev, void *data)
+{> + int ret = 0;
+ struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+
+ ret = ctcu_set_etr_traceid(csdev, sink_data, false);
+ if (ret)
+ dev_dbg(&csdev->dev, "disable data filter failed\n");
+
+ return 0;
+}
[...]
enum coresight_dev_subtype_helper {
CORESIGHT_DEV_SUBTYPE_HELPER_CATU,
- CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI
+ CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI,
+ CORESIGHT_DEV_SUBTYPE_HELPER_CTCU
Please add a comma here too, so that future additions will be
less noisy
Sure, will add a comma after CORESIGHT_DEV_SUBTYPE_HELPER_CTCU.
Thanks,
Jie
