Re: [PATCH v8 13/13] coresight-tpdm: Add nodes for dsb msr support

On 22/08/2023 06:26, Tao Zhang wrote:

Add the nodes for DSB subunit MSR(mux select register) support. The TPDM MSR (mux select register) interface is an optional interface and associated bank of registers per TPDM subunit. The intent of mux select registers is to control muxing structures driving the TPDM’s’ various subunit interfaces.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

.../ABI/testing/sysfs-bus-coresight-devices-tpdm | 8 +++ drivers/hwtracing/coresight/coresight-tpdm.c | 79 ++++++++++++++++++++++ drivers/hwtracing/coresight/coresight-tpdm.h | 12 ++++ 3 files changed, 99 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm index 46a5535..9b7e172 100644 --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm @@ -162,3 +162,11 @@ Description: Accepts only one of the 2 values - 0 or 1. 0 : Set the DSB pattern type to value. 1 : Set the DSB pattern type to toggle.

+What: /sys/bus/coresight/devices/<tpdm-name>/dsb_msr/msr[0:31] +Date: March 2023 +KernelVersion 6.5 +Contact: Jinlong Mao (QUIC) quic_jinlmao@quicinc.com, Tao Zhang (QUIC) quic_taozha@quicinc.com +Description:

• (RW) Set/Get the MSR(mux select register) for the DSB subunit
  

• TPDM.
  

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 9b0e060..367c5e2 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -51,6 +51,9 @@ static ssize_t tpdm_simple_dataset_show(struct device *dev, case DSB_PATT_MASK: return sysfs_emit(buf, "0x%x\n", drvdata->dsb->patt_mask[tpdm_attr->idx]);

• case DSB_MSR:

• return sysfs_emit(buf, "0x%x\n",
  

• 		drvdata->dsb->msr[tpdm_attr->idx]);
  

  default: return -EINVAL; }

@@ -84,6 +87,9 @@ static ssize_t tpdm_simple_dataset_store(struct device *dev, case DSB_PATT_MASK: drvdata->dsb->patt_mask[tpdm_attr->idx] = val; break;

• case DSB_MSR:

• drvdata->dsb->msr[tpdm_attr->idx] = val;
  

• break;
  

  default: spin_unlock(&drvdata->spinlock); return -EINVAL;

@@ -110,6 +116,22 @@ static umode_t tpdm_dsb_is_visible(struct kobject *kobj, return 0; } +static umode_t tpdm_dsb_msr_is_visible(struct kobject *kobj,

• 			   struct attribute *attr, int n)
  

+{

• struct device *dev = kobj_to_dev(kobj);
• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (drvdata && tpdm_has_dsb_dataset(drvdata)) {

• of_property_read_u32(drvdata->dev->of_node,
  

• 	   "qcom,dsb_msr_num", &drvdata->dsb->msr_num);
  

Why don't we read this during probe and use that here ?

• if (drvdata->dsb->msr_num != 0)
  

Also, could we not check the index of the attribute and make sure it is available on this TPDM ?

attr->idx < drvdata->dsb->msr_num ??

• 	return attr->mode;
  

• }
• return 0;

+}

• static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata) { if (tpdm_has_dsb_dataset(drvdata)) {

@@ -162,6 +184,18 @@ static void set_dsb_tier(struct tpdm_drvdata *drvdata, u32 *val) } +static void set_dsb_msr(struct tpdm_drvdata *drvdata) +{

• int i;
• if (drvdata->dsb->msr_num == 0)

• return;
  

You don;t need this special case, the loop handles it already

• for (i = 0; i < drvdata->dsb->msr_num; i++)

• writel_relaxed(drvdata->dsb->msr[i],
  

• 	   drvdata->base + TPDM_DSB_MSR(i));
  

+}

• static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) { u32 val, i;

@@ -186,6 +220,8 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) set_dsb_tier(drvdata, &val); writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);

• set_dsb_msr(drvdata);
• val = readl_relaxed(drvdata->base + TPDM_DSB_CR); /* Set the mode of DSB dataset */ set_dsb_mode(drvdata, &val);

@@ -707,6 +743,42 @@ static struct attribute *tpdm_dsb_patt_attrs[] = { NULL, }; +static struct attribute *tpdm_dsb_msr_attrs[] = {

• DSB_MSR_ATTR(0),
• DSB_MSR_ATTR(1),
• DSB_MSR_ATTR(2),
• DSB_MSR_ATTR(3),
• DSB_MSR_ATTR(4),
• DSB_MSR_ATTR(5),
• DSB_MSR_ATTR(6),
• DSB_MSR_ATTR(7),
• DSB_MSR_ATTR(8),
• DSB_MSR_ATTR(9),
• DSB_MSR_ATTR(10),
• DSB_MSR_ATTR(11),
• DSB_MSR_ATTR(12),
• DSB_MSR_ATTR(13),
• DSB_MSR_ATTR(14),
• DSB_MSR_ATTR(15),
• DSB_MSR_ATTR(16),
• DSB_MSR_ATTR(17),
• DSB_MSR_ATTR(18),
• DSB_MSR_ATTR(19),
• DSB_MSR_ATTR(20),
• DSB_MSR_ATTR(21),
• DSB_MSR_ATTR(22),
• DSB_MSR_ATTR(23),
• DSB_MSR_ATTR(24),
• DSB_MSR_ATTR(25),
• DSB_MSR_ATTR(26),
• DSB_MSR_ATTR(27),
• DSB_MSR_ATTR(28),
• DSB_MSR_ATTR(29),
• DSB_MSR_ATTR(30),
• DSB_MSR_ATTR(31),
• NULL,

+};

• static struct attribute *tpdm_dsb_attrs[] = { &dev_attr_dsb_mode.attr, &dev_attr_dsb_patt_ts.attr,

@@ -739,12 +811,19 @@ static struct attribute_group tpdm_dsb_patt_grp = { .name = "dsb_patt", }; +static struct attribute_group tpdm_dsb_msr_grp = {

• .attrs = tpdm_dsb_msr_attrs,
• .is_visible = tpdm_dsb_msr_is_visible,
• .name = "dsb_msr",

+};

• static const struct attribute_group *tpdm_attr_grps[] = { &tpdm_attr_grp, &tpdm_dsb_attrs_grp, &tpdm_dsb_edge_grp, &tpdm_dsb_trig_patt_grp, &tpdm_dsb_patt_grp,
• &tpdm_dsb_msr_grp, NULL, };

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h index 9173e80..270e946 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.h +++ b/drivers/hwtracing/coresight/coresight-tpdm.h @@ -18,6 +18,7 @@ #define TPDM_DSB_XPMR(n) (0x7E8 + (n * 4)) #define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) #define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) +#define TPDM_DSB_MSR(n) (0x980 + (n * 4)) /* Enable bit for DSB subunit */ #define TPDM_DSB_CR_ENA BIT(0) @@ -90,6 +91,8 @@ #define TPDM_DSB_MAX_EDCMR 8 /* MAX number of DSB pattern */ #define TPDM_DSB_MAX_PATT 8 +/* MAX number of DSB MSR */ +#define TPDM_DSB_MAX_MSR 32 #define tpdm_simple_dataset_ro(name, mem, idx, max) \ (&((struct tpdm_dataset_attribute[]) { \ @@ -136,16 +139,22 @@ tpdm_simple_dataset_rw(tpmr##nr, \ DSB_PATT_MASK, nr, TPDM_DSB_MAX_PATT) +#define DSB_MSR_ATTR(nr) \

• tpdm_simple_dataset_rw(msr##nr,			\
  

• DSB_MSR, nr, TPDM_DSB_MAX_MSR)
  

• /**
  • struct dsb_dataset - specifics associated to dsb dataset
  • @mode: DSB programming mode
  • @edge_ctrl_idx Index number of the edge control
• • @msr_num Number of MSR supported by DSB TPDM
  • @edge_ctrl: Save value for edge control
  • @edge_ctrl_mask: Save value for edge control mask
  • @patt_val: Save value for pattern
  • @patt_mask: Save value for pattern mask
  • @trig_patt: Save value for trigger pattern
  • @trig_patt_mask: Save value for trigger pattern mask
• • @msr Save value for MSR
  • @patt_ts: Enable/Disable pattern timestamp
  • @patt_type: Set pattern type
  • @trig_ts: Enable/Disable trigger timestamp.

@@ -154,12 +163,14 @@ struct dsb_dataset { u32 mode; u32 edge_ctrl_idx;

• u32 msr_num;

Why should this be part of the dataset ? If I remember correctly, this data structure is zeroed on reset via sysfs ? Shouldn't that be part of the drvdata-> instead ? So that you read it once during probe and don't mess with it.

Suzuki

u32 edge_ctrl[TPDM_DSB_MAX_EDCR]; u32 edge_ctrl_mask[TPDM_DSB_MAX_EDCMR]; u32 patt_val[TPDM_DSB_MAX_PATT]; u32 patt_mask[TPDM_DSB_MAX_PATT]; u32 trig_patt[TPDM_DSB_MAX_PATT]; u32 trig_patt_mask[TPDM_DSB_MAX_PATT];

• u32 msr[TPDM_DSB_MAX_MSR]; bool patt_ts; bool patt_type; bool trig_ts;

@@ -195,6 +206,7 @@ enum dataset_mem { DSB_TRIG_PATT_MASK, DSB_PATT, DSB_PATT_MASK,

• DSB_MSR, };

/**