| Message ID | 1682586037-25973-8-git-send-email-quic_taozha@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support to configure TPDM DSB subunit | expand |
On 27/04/2023 10:00, Tao Zhang wrote: > Add the nodes to set value for DSB edge control and DSB edge > control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR > resgisters to configure edge control. DSB edge detection control > 00: Rising edge detection > 01: Falling edge detection > 10: Rising and falling edge detection (toggle detection) > And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to > configure mask. Eight 32 bit registers providing DSB interface > edge detection mask control. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++ > drivers/hwtracing/coresight/coresight-tpdm.c | 135 ++++++++++++++++++++- > drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++ > 3 files changed, 187 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > index 348e167..a57f000 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > @@ -60,3 +60,35 @@ Description: > Bit[3] : Set to 0 for low performance mode. > Set to 1 for high performance mode. > Bit[4:8] : Select byte lane for high performance mode. > + > +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl > +Date: March 2023 > +KernelVersion 6.3 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> > +Description: > + Read/Write a set of the edge control registers of the DSB > + in TPDM. > + > + Expected format is the following: > + <integer1> <integer2> <integer3> > + > + Where: > + <integer1> : Start EDCR register number > + <integer2> : End EDCR register number > + <integer3> : The value need to be written > + > +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask > +Date: March 2023 > +KernelVersion 6.3 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> > +Description: > + Read/Write a set of the edge control mask registers of the > + DSB in TPDM. > + > + Expected format is the following: > + <integer1> <integer2> <integer3> > + > + Where: > + <integer1> : Start EDCMR register number > + <integer2> : End EDCMR register number > + <integer3> : The value need to be written > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 1bacaa5..a40e458 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -80,7 +80,14 @@ static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val) > > static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) > { > - u32 val; > + u32 val, i; > + > + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) > + writel_relaxed(drvdata->dsb->edge_ctrl[i], > + drvdata->base + TPDM_DSB_EDCR(i)); > + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) > + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], > + drvdata->base + TPDM_DSB_EDCMR(i)); Do all TPDM DSBs have MAX_EDCR registers ? Or some have less than that ? If it is latter, do we need special care to avoid writing to inexistent registers ? > > val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); > /* Set trigger timestamp */ > @@ -313,6 +320,130 @@ static ssize_t dsb_mode_store(struct device *dev, > } > static DEVICE_ATTR_RW(dsb_mode); > > +static ssize_t dsb_edge_ctrl_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i; > + > + spin_lock(&drvdata->spinlock); > + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { > + size += sysfs_emit_at(buf, size, > + "Index:0x%x Val:0x%x\n", i, > + drvdata->dsb->edge_ctrl[i]); It may be safe, but please add a check to make sure that we don't overflow. At least bail out when we hit a return of 0, indicating reached the end of buffer. > + } > + spin_unlock(&drvdata->spinlock); > + return size; > +} > + > +/* > + * value 1: Start EDCR register number > + * value 2: End EDCR register number > + * value 3: The value need to be written > + * The EDCR registers can include up to 16 32-bit registers, and each > + * one can be configured to control up to 16 edge detections(2 bits > + * control one edge detection). So a total 256 edge detections can be > + * configured. So the starting number(value 1) and ending number(value 2) > + * cannot be greater than 256, and value 1 should be less than value 2. > + * The following values are the rage of value 3. > + * 0 - Rising edge detection > + * 1 - Falling edge detection > + * 2 - Rising and falling edge detection (toggle detection) > + */ > +static ssize_t dsb_edge_ctrl_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val, mask, start, end, edge_ctrl, edge_ctrl_shift; > + int i, reg; > + > + if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3) > + return -EINVAL; > + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) || > + (start > end) || (edge_ctrl > 0x2)) > + return -EPERM; > + > + spin_lock(&drvdata->spinlock); > + for (i = start; i <= end; i++) { > + /* > + * There are 2 bit per DSB Edge Control line. > + * Thus we have 16 lines in a 32bit word. > + */ > + reg = EDCR_TO_WORD_IDX(i); > + mask = EDCR_TO_WORD_MASK(i); > + val = drvdata->dsb->edge_ctrl[reg]; > + edge_ctrl_shift = EDCR_TO_WORD_VAL(edge_ctrl, i); > + bitmap_replace(&val, &val, &edge_ctrl_shift, &mask, 32); Could we simply do : reg &= ~mask; reg |= FIELD_PREP(mask, edge_ctrl); > + drvdata->dsb->edge_ctrl[reg] = val; > + } > + spin_unlock(&drvdata->spinlock); > + > + return size; > +} > +static DEVICE_ATTR_RW(dsb_edge_ctrl); > + > +static ssize_t dsb_edge_ctrl_mask_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i; > + > + spin_lock(&drvdata->spinlock); > + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) { > + size += sysfs_emit_at(buf, size, > + "Index:0x%x Val:0x%x\n", i, > + drvdata->dsb->edge_ctrl_mask[i]); > + } > + spin_unlock(&drvdata->spinlock); > + return size; > +} > + > +/* > + * value 1: Start EDCMR register number > + * value 2: End EDCMR register number > + * value 3: The value need to be written > + */ > +static ssize_t dsb_edge_ctrl_mask_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long start, end, val; > + u32 set; > + int i, reg; > + > + if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3) > + return -EINVAL; > + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) > + || (start > end) || (val & ~1UL)) > + return -EPERM; > + > + spin_lock(&drvdata->spinlock); > + for (i = start; i <= end; i++) { > + /* > + * There is 1 bit per DSB Edge Control Mark line. > + * Thus we have 32 lines in a 32bit word. > + */ > + reg = EDCMR_TO_WORD_IDX(i); > + set = drvdata->dsb->edge_ctrl_mask[reg]; > + if (val) > + set |= BIT(EDCR_TO_WORD_SHIFT(i)); > + else > + set &= ~BIT(EDCR_TO_WORD_SHIFT(i)); > + drvdata->dsb->edge_ctrl_mask[reg] = set; > + } > + spin_unlock(&drvdata->spinlock); > + return size; > +} > +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask); > + > static ssize_t dsb_trig_type_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -385,6 +516,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts); > > static struct attribute *tpdm_dsb_attrs[] = { > &dev_attr_dsb_mode.attr, > + &dev_attr_dsb_edge_ctrl.attr, > + &dev_attr_dsb_edge_ctrl_mask.attr, > &dev_attr_dsb_trig_ts.attr, > &dev_attr_dsb_trig_type.attr, > NULL, > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h > index 79df07e..f25dcdec 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.h > +++ b/drivers/hwtracing/coresight/coresight-tpdm.h > @@ -12,6 +12,8 @@ > /* DSB Subunit Registers */ > #define TPDM_DSB_CR (0x780) > #define TPDM_DSB_TIER (0x784) > +#define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) > +#define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) > > /* Enable bit for DSB subunit */ > #define TPDM_DSB_CR_ENA BIT(0) > @@ -34,6 +36,15 @@ > #define TPDM_DSB_TEST_MODE GENMASK(10, 9) > #define TPDM_DSB_HPSEL GENMASK(6, 2) > > +#define EDCRS_PER_WORD 16 > +#define EDCR_TO_WORD_IDX(r) ((r) / EDCRS_PER_WORD) > +#define EDCR_TO_WORD_SHIFT(r) ((r % EDCRS_PER_WORD) * 2) > +#define EDCR_TO_WORD_VAL(val, r) (val << EDCR_TO_WORD_SHIFT(r)) > +#define EDCR_TO_WORD_MASK(r) EDCR_TO_WORD_VAL(0x3, r) minor nit: add a new line here please > +#define EDCMRS_PER_WORD 32 > +#define EDCMR_TO_WORD_IDX(r) ((r) / EDCMRS_PER_WORD) > +#define EDCMR_TO_WORD_SHIFT(r) ((r) % EDCMRS_PER_WORD) > + > /* TPDM integration test registers */ > #define TPDM_ITATBCNTRL (0xEF0) > #define TPDM_ITCNTRL (0xF00) > @@ -60,14 +71,24 @@ > #define TPDM_PIDR0_DS_IMPDEF BIT(0) > #define TPDM_PIDR0_DS_DSB BIT(1) > > +#define TPDM_DSB_MAX_LINES 256 > +/* MAX number of EDCR registers */ > +#define TPDM_DSB_MAX_EDCR 16 > +/* MAX number of EDCMR registers */ > +#define TPDM_DSB_MAX_EDCMR 8 > + > /** > * struct dsb_dataset - specifics associated to dsb dataset > * @mode: DSB programming mode > + * @edge_ctrl: Save value for edge control > + * @edge_ctrl_mask: Save value for edge control mask > * @trig_ts: Enable/Disable trigger timestamp. > * @trig_type: Enable/Disable trigger type. > */ > struct dsb_dataset { > u32 mode; > + u32 edge_ctrl[TPDM_DSB_MAX_EDCR]; > + u32 edge_ctrl_mask[TPDM_DSB_MAX_EDCMR]; minor nit: Please align it with the fields below. > bool trig_ts; > bool trig_type; > }; Suzuki
On 6/1/2023 8:14 PM, Suzuki K Poulose wrote: > On 27/04/2023 10:00, Tao Zhang wrote: >> Add the nodes to set value for DSB edge control and DSB edge >> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR >> resgisters to configure edge control. DSB edge detection control >> 00: Rising edge detection >> 01: Falling edge detection >> 10: Rising and falling edge detection (toggle detection) >> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to >> configure mask. Eight 32 bit registers providing DSB interface >> edge detection mask control. >> >> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >> --- >> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++ >> drivers/hwtracing/coresight/coresight-tpdm.c | 135 >> ++++++++++++++++++++- >> drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++ >> 3 files changed, 187 insertions(+), 1 deletion(-) >> >> diff --git >> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> index 348e167..a57f000 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> @@ -60,3 +60,35 @@ Description: >> Bit[3] : Set to 0 for low performance mode. >> Set to 1 for high performance mode. >> Bit[4:8] : Select byte lane for high performance mode. >> + >> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl >> +Date: March 2023 >> +KernelVersion 6.3 >> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang >> (QUIC) <quic_taozha@quicinc.com> >> +Description: >> + Read/Write a set of the edge control registers of the DSB >> + in TPDM. >> + >> + Expected format is the following: >> + <integer1> <integer2> <integer3> >> + >> + Where: >> + <integer1> : Start EDCR register number >> + <integer2> : End EDCR register number >> + <integer3> : The value need to be written >> + >> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask >> +Date: March 2023 >> +KernelVersion 6.3 >> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang >> (QUIC) <quic_taozha@quicinc.com> >> +Description: >> + Read/Write a set of the edge control mask registers of the >> + DSB in TPDM. >> + >> + Expected format is the following: >> + <integer1> <integer2> <integer3> >> + >> + Where: >> + <integer1> : Start EDCMR register number >> + <integer2> : End EDCMR register number >> + <integer3> : The value need to be written >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >> b/drivers/hwtracing/coresight/coresight-tpdm.c >> index 1bacaa5..a40e458 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >> @@ -80,7 +80,14 @@ static void set_trigger_type(struct tpdm_drvdata >> *drvdata, u32 *val) >> static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) >> { >> - u32 val; >> + u32 val, i; >> + >> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) >> + writel_relaxed(drvdata->dsb->edge_ctrl[i], >> + drvdata->base + TPDM_DSB_EDCR(i)); >> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) >> + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], >> + drvdata->base + TPDM_DSB_EDCMR(i)); > > Do all TPDM DSBs have MAX_EDCR registers ? Or some have less than that ? > If it is latter, do we need special care to avoid writing to inexistent > registers ? > You are right, not all DSB TPDMs have MAX_EDCR registers. In our design, the inexistent register addresses are not occupied and safe for accessing. Currently we don't have a good way to know the quantity of EDCR/EDCMR registers for DSB TPDMs. The only way we can think of is to set it in device tree manually. Do you have other suggestion for this? >> val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); >> /* Set trigger timestamp */ >> @@ -313,6 +320,130 @@ static ssize_t dsb_mode_store(struct device *dev, >> } >> static DEVICE_ATTR_RW(dsb_mode); >> +static ssize_t dsb_edge_ctrl_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + ssize_t size = 0; >> + int i; >> + >> + spin_lock(&drvdata->spinlock); >> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { >> + size += sysfs_emit_at(buf, size, >> + "Index:0x%x Val:0x%x\n", i, >> + drvdata->dsb->edge_ctrl[i]); > > It may be safe, but please add a check to make sure that we don't > overflow. At least bail out when we hit a return of 0, indicating > reached the end of buffer. > Can I add the following check to replace the current code?? int ret = 0; for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { ret = sysfs_emit_at(buf, size, "Index:0x%x Val:0x%x\n", i, drvdata->dsb->edge_ctrl[i]); if (!ret) { dev_warn(drvdata->dev, "The buffer has been overflowed\n"); spin_unlock(&drvdata->spinlock); return size; } else size += ret; } >> + } >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> + >> +/* >> + * value 1: Start EDCR register number >> + * value 2: End EDCR register number >> + * value 3: The value need to be written >> + * The EDCR registers can include up to 16 32-bit registers, and each >> + * one can be configured to control up to 16 edge detections(2 bits >> + * control one edge detection). So a total 256 edge detections can be >> + * configured. So the starting number(value 1) and ending >> number(value 2) >> + * cannot be greater than 256, and value 1 should be less than value 2. >> + * The following values are the rage of value 3. >> + * 0 - Rising edge detection >> + * 1 - Falling edge detection >> + * 2 - Rising and falling edge detection (toggle detection) >> + */ >> +static ssize_t dsb_edge_ctrl_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, >> + size_t size) >> +{ >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + unsigned long val, mask, start, end, edge_ctrl, edge_ctrl_shift; >> + int i, reg; >> + >> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3) >> + return -EINVAL; >> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) || >> + (start > end) || (edge_ctrl > 0x2)) >> + return -EPERM; >> + >> + spin_lock(&drvdata->spinlock); >> + for (i = start; i <= end; i++) { >> + /* >> + * There are 2 bit per DSB Edge Control line. >> + * Thus we have 16 lines in a 32bit word. >> + */ >> + reg = EDCR_TO_WORD_IDX(i); >> + mask = EDCR_TO_WORD_MASK(i); >> + val = drvdata->dsb->edge_ctrl[reg]; > >> + edge_ctrl_shift = EDCR_TO_WORD_VAL(edge_ctrl, i); >> + bitmap_replace(&val, &val, &edge_ctrl_shift, &mask, 32); > > Could we simply do : > > reg &= ~mask; > reg |= FIELD_PREP(mask, edge_ctrl); > Perhaps "FIELD_PREP" cannot be used here since "mask" must be constant in this macro. But in our code, the variable "mask" is not constant. > >> + drvdata->dsb->edge_ctrl[reg] = val; >> + } >> + spin_unlock(&drvdata->spinlock); >> + >> + return size; >> +} >> +static DEVICE_ATTR_RW(dsb_edge_ctrl); >> + >> +static ssize_t dsb_edge_ctrl_mask_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + ssize_t size = 0; >> + int i; >> + >> + spin_lock(&drvdata->spinlock); >> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) { >> + size += sysfs_emit_at(buf, size, >> + "Index:0x%x Val:0x%x\n", i, >> + drvdata->dsb->edge_ctrl_mask[i]); >> + } >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> + >> +/* >> + * value 1: Start EDCMR register number >> + * value 2: End EDCMR register number >> + * value 3: The value need to be written >> + */ >> +static ssize_t dsb_edge_ctrl_mask_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, >> + size_t size) >> +{ >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + unsigned long start, end, val; >> + u32 set; >> + int i, reg; >> + >> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3) >> + return -EINVAL; >> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) >> + || (start > end) || (val & ~1UL)) >> + return -EPERM; >> + >> + spin_lock(&drvdata->spinlock); >> + for (i = start; i <= end; i++) { >> + /* >> + * There is 1 bit per DSB Edge Control Mark line. >> + * Thus we have 32 lines in a 32bit word. >> + */ >> + reg = EDCMR_TO_WORD_IDX(i); >> + set = drvdata->dsb->edge_ctrl_mask[reg]; >> + if (val) >> + set |= BIT(EDCR_TO_WORD_SHIFT(i)); >> + else >> + set &= ~BIT(EDCR_TO_WORD_SHIFT(i)); >> + drvdata->dsb->edge_ctrl_mask[reg] = set; >> + } >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask); >> + >> static ssize_t dsb_trig_type_show(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> @@ -385,6 +516,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts); >> static struct attribute *tpdm_dsb_attrs[] = { >> &dev_attr_dsb_mode.attr, >> + &dev_attr_dsb_edge_ctrl.attr, >> + &dev_attr_dsb_edge_ctrl_mask.attr, >> &dev_attr_dsb_trig_ts.attr, >> &dev_attr_dsb_trig_type.attr, >> NULL, >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h >> b/drivers/hwtracing/coresight/coresight-tpdm.h >> index 79df07e..f25dcdec 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >> @@ -12,6 +12,8 @@ >> /* DSB Subunit Registers */ >> #define TPDM_DSB_CR (0x780) >> #define TPDM_DSB_TIER (0x784) >> +#define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) >> +#define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) >> /* Enable bit for DSB subunit */ >> #define TPDM_DSB_CR_ENA BIT(0) >> @@ -34,6 +36,15 @@ >> #define TPDM_DSB_TEST_MODE GENMASK(10, 9) >> #define TPDM_DSB_HPSEL GENMASK(6, 2) >> +#define EDCRS_PER_WORD 16 >> +#define EDCR_TO_WORD_IDX(r) ((r) / EDCRS_PER_WORD) >> +#define EDCR_TO_WORD_SHIFT(r) ((r % EDCRS_PER_WORD) * 2) >> +#define EDCR_TO_WORD_VAL(val, r) (val << EDCR_TO_WORD_SHIFT(r)) >> +#define EDCR_TO_WORD_MASK(r) EDCR_TO_WORD_VAL(0x3, r) > > minor nit: add a new line here please Sure, I will update this in the next patch series. > >> +#define EDCMRS_PER_WORD 32 >> +#define EDCMR_TO_WORD_IDX(r) ((r) / EDCMRS_PER_WORD) >> +#define EDCMR_TO_WORD_SHIFT(r) ((r) % EDCMRS_PER_WORD) >> + >> /* TPDM integration test registers */ >> #define TPDM_ITATBCNTRL (0xEF0) >> #define TPDM_ITCNTRL (0xF00) >> @@ -60,14 +71,24 @@ >> #define TPDM_PIDR0_DS_IMPDEF BIT(0) >> #define TPDM_PIDR0_DS_DSB BIT(1) >> +#define TPDM_DSB_MAX_LINES 256 >> +/* MAX number of EDCR registers */ >> +#define TPDM_DSB_MAX_EDCR 16 >> +/* MAX number of EDCMR registers */ >> +#define TPDM_DSB_MAX_EDCMR 8 >> + >> /** >> * struct dsb_dataset - specifics associated to dsb dataset >> * @mode: DSB programming mode >> + * @edge_ctrl: Save value for edge control >> + * @edge_ctrl_mask: Save value for edge control mask >> * @trig_ts: Enable/Disable trigger timestamp. >> * @trig_type: Enable/Disable trigger type. >> */ >> struct dsb_dataset { >> u32 mode; >> + u32 edge_ctrl[TPDM_DSB_MAX_EDCR]; >> + u32 edge_ctrl_mask[TPDM_DSB_MAX_EDCMR]; > > minor nit: Please align it with the fields below. Sure, I will update this in the next patch series. Best, Tao > >> bool trig_ts; >> bool trig_type; >> }; > > Suzuki
On 02/06/2023 09:21, Tao Zhang wrote: > > On 6/1/2023 8:14 PM, Suzuki K Poulose wrote: >> On 27/04/2023 10:00, Tao Zhang wrote: >>> Add the nodes to set value for DSB edge control and DSB edge >>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR >>> resgisters to configure edge control. DSB edge detection control >>> 00: Rising edge detection >>> 01: Falling edge detection >>> 10: Rising and falling edge detection (toggle detection) >>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to >>> configure mask. Eight 32 bit registers providing DSB interface >>> edge detection mask control. >>> >>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>> --- >>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++ >>> drivers/hwtracing/coresight/coresight-tpdm.c | 135 >>> ++++++++++++++++++++- >>> drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++ >>> 3 files changed, 187 insertions(+), 1 deletion(-) >>> >>> diff --git >>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>> index 348e167..a57f000 100644 >>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>> @@ -60,3 +60,35 @@ Description: >>> Bit[3] : Set to 0 for low performance mode. >>> Set to 1 for high performance mode. >>> Bit[4:8] : Select byte lane for high performance mode. >>> + >>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl >>> +Date: March 2023 >>> +KernelVersion 6.3 >>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang >>> (QUIC) <quic_taozha@quicinc.com> >>> +Description: >>> + Read/Write a set of the edge control registers of the DSB >>> + in TPDM. >>> + >>> + Expected format is the following: >>> + <integer1> <integer2> <integer3> >>> + >>> + Where: >>> + <integer1> : Start EDCR register number >>> + <integer2> : End EDCR register number >>> + <integer3> : The value need to be written >>> + >>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask >>> +Date: March 2023 >>> +KernelVersion 6.3 >>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang >>> (QUIC) <quic_taozha@quicinc.com> >>> +Description: >>> + Read/Write a set of the edge control mask registers of the >>> + DSB in TPDM. >>> + >>> + Expected format is the following: >>> + <integer1> <integer2> <integer3> >>> + >>> + Where: >>> + <integer1> : Start EDCMR register number >>> + <integer2> : End EDCMR register number >>> + <integer3> : The value need to be written >>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >>> b/drivers/hwtracing/coresight/coresight-tpdm.c >>> index 1bacaa5..a40e458 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>> @@ -80,7 +80,14 @@ static void set_trigger_type(struct tpdm_drvdata >>> *drvdata, u32 *val) >>> static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) >>> { >>> - u32 val; >>> + u32 val, i; >>> + >>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) >>> + writel_relaxed(drvdata->dsb->edge_ctrl[i], >>> + drvdata->base + TPDM_DSB_EDCR(i)); >>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) >>> + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], >>> + drvdata->base + TPDM_DSB_EDCMR(i)); >> >> Do all TPDM DSBs have MAX_EDCR registers ? Or some have less than that ? >> If it is latter, do we need special care to avoid writing to inexistent >> registers ? >> > You are right, not all DSB TPDMs have MAX_EDCR registers. In our design, > the inexistent register addresses > > are not occupied and safe for accessing. > > Currently we don't have a good way to know the quantity of EDCR/EDCMR > registers for DSB TPDMs. > > The only way we can think of is to set it in device tree manually. > > Do you have other suggestion for this? > >>> val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); >>> /* Set trigger timestamp */ >>> @@ -313,6 +320,130 @@ static ssize_t dsb_mode_store(struct device *dev, >>> } >>> static DEVICE_ATTR_RW(dsb_mode); >>> +static ssize_t dsb_edge_ctrl_show(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + ssize_t size = 0; >>> + int i; >>> + >>> + spin_lock(&drvdata->spinlock); >>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { >>> + size += sysfs_emit_at(buf, size, >>> + "Index:0x%x Val:0x%x\n", i, >>> + drvdata->dsb->edge_ctrl[i]); >> >> It may be safe, but please add a check to make sure that we don't >> overflow. At least bail out when we hit a return of 0, indicating >> reached the end of buffer. >> > Can I add the following check to replace the current code?? > > int ret = 0; > > > for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { > > ret = sysfs_emit_at(buf, size, "Index:0x%x Val:0x%x\n", i, > drvdata->dsb->edge_ctrl[i]); > > if (!ret) { > > dev_warn(drvdata->dev, "The buffer has been overflowed\n"); You don't need this, it already triggers a WARN() in sysfs_emit_at(). So you could do: for (....) { unsigned long bytes = sysfs_emit_at(buf, size, ....); if (bytes <= 0) break; size += bytes; } > > spin_unlock(&drvdata->spinlock); > > return size; > > } else > > size += ret; > > } > >>> + } >>> + spin_unlock(&drvdata->spinlock); >>> + return size; >>> +} >>> + >>> +/* >>> + * value 1: Start EDCR register number >>> + * value 2: End EDCR register number >>> + * value 3: The value need to be written >>> + * The EDCR registers can include up to 16 32-bit registers, and each >>> + * one can be configured to control up to 16 edge detections(2 bits >>> + * control one edge detection). So a total 256 edge detections can be >>> + * configured. So the starting number(value 1) and ending >>> number(value 2) >>> + * cannot be greater than 256, and value 1 should be less than value 2. >>> + * The following values are the rage of value 3. >>> + * 0 - Rising edge detection >>> + * 1 - Falling edge detection >>> + * 2 - Rising and falling edge detection (toggle detection) >>> + */ >>> +static ssize_t dsb_edge_ctrl_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, >>> + size_t size) >>> +{ >>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + unsigned long val, mask, start, end, edge_ctrl, edge_ctrl_shift; >>> + int i, reg; >>> + >>> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3) >>> + return -EINVAL; >>> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) || >>> + (start > end) || (edge_ctrl > 0x2)) >>> + return -EPERM; >>> + >>> + spin_lock(&drvdata->spinlock); >>> + for (i = start; i <= end; i++) { >>> + /* >>> + * There are 2 bit per DSB Edge Control line. >>> + * Thus we have 16 lines in a 32bit word. >>> + */ >>> + reg = EDCR_TO_WORD_IDX(i); >>> + mask = EDCR_TO_WORD_MASK(i); >>> + val = drvdata->dsb->edge_ctrl[reg]; >> >>> + edge_ctrl_shift = EDCR_TO_WORD_VAL(edge_ctrl, i); >>> + bitmap_replace(&val, &val, &edge_ctrl_shift, &mask, 32); >> >> Could we simply do : >> >> reg &= ~mask; >> reg |= FIELD_PREP(mask, edge_ctrl); >> > Perhaps "FIELD_PREP" cannot be used here since "mask" must be constant > in this macro. Ah, you are right. Sorry about that. > > But in our code, the variable "mask" is not constant. Still I think using the bitmap_replace is an overkill. We could simply do: val &= ~mask; val |= EDCR_TO_WORD_VAL(edge_ctrl, i); Suzuki >> >>> + drvdata->dsb->edge_ctrl[reg] = val; >>> + } >>> + spin_unlock(&drvdata->spinlock); >>> + >>> + return size; >>> +} >>> +static DEVICE_ATTR_RW(dsb_edge_ctrl); >>> + >>> +static ssize_t dsb_edge_ctrl_mask_show(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + ssize_t size = 0; >>> + int i; >>> + >>> + spin_lock(&drvdata->spinlock); >>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) { >>> + size += sysfs_emit_at(buf, size, >>> + "Index:0x%x Val:0x%x\n", i, >>> + drvdata->dsb->edge_ctrl_mask[i]); >>> + } >>> + spin_unlock(&drvdata->spinlock); >>> + return size; >>> +} >>> + >>> +/* >>> + * value 1: Start EDCMR register number >>> + * value 2: End EDCMR register number >>> + * value 3: The value need to be written >>> + */ >>> +static ssize_t dsb_edge_ctrl_mask_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, >>> + size_t size) >>> +{ >>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + unsigned long start, end, val; >>> + u32 set; >>> + int i, reg; >>> + >>> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3) >>> + return -EINVAL; >>> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) >>> + || (start > end) || (val & ~1UL)) >>> + return -EPERM; >>> + >>> + spin_lock(&drvdata->spinlock); >>> + for (i = start; i <= end; i++) { >>> + /* >>> + * There is 1 bit per DSB Edge Control Mark line. >>> + * Thus we have 32 lines in a 32bit word. >>> + */ >>> + reg = EDCMR_TO_WORD_IDX(i); >>> + set = drvdata->dsb->edge_ctrl_mask[reg]; >>> + if (val) >>> + set |= BIT(EDCR_TO_WORD_SHIFT(i)); >>> + else >>> + set &= ~BIT(EDCR_TO_WORD_SHIFT(i)); >>> + drvdata->dsb->edge_ctrl_mask[reg] = set; >>> + } >>> + spin_unlock(&drvdata->spinlock); >>> + return size; >>> +} >>> +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask); >>> + >>> static ssize_t dsb_trig_type_show(struct device *dev, >>> struct device_attribute *attr, char *buf) >>> { >>> @@ -385,6 +516,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts); >>> static struct attribute *tpdm_dsb_attrs[] = { >>> &dev_attr_dsb_mode.attr, >>> + &dev_attr_dsb_edge_ctrl.attr, >>> + &dev_attr_dsb_edge_ctrl_mask.attr, >>> &dev_attr_dsb_trig_ts.attr, >>> &dev_attr_dsb_trig_type.attr, >>> NULL, >>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h >>> b/drivers/hwtracing/coresight/coresight-tpdm.h >>> index 79df07e..f25dcdec 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>> @@ -12,6 +12,8 @@ >>> /* DSB Subunit Registers */ >>> #define TPDM_DSB_CR (0x780) >>> #define TPDM_DSB_TIER (0x784) >>> +#define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) >>> +#define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) >>> /* Enable bit for DSB subunit */ >>> #define TPDM_DSB_CR_ENA BIT(0) >>> @@ -34,6 +36,15 @@ >>> #define TPDM_DSB_TEST_MODE GENMASK(10, 9) >>> #define TPDM_DSB_HPSEL GENMASK(6, 2) >>> +#define EDCRS_PER_WORD 16 >>> +#define EDCR_TO_WORD_IDX(r) ((r) / EDCRS_PER_WORD) >>> +#define EDCR_TO_WORD_SHIFT(r) ((r % EDCRS_PER_WORD) * 2) >>> +#define EDCR_TO_WORD_VAL(val, r) (val << EDCR_TO_WORD_SHIFT(r)) >>> +#define EDCR_TO_WORD_MASK(r) EDCR_TO_WORD_VAL(0x3, r) >> >> minor nit: add a new line here please > Sure, I will update this in the next patch series. >> >>> +#define EDCMRS_PER_WORD 32 >>> +#define EDCMR_TO_WORD_IDX(r) ((r) / EDCMRS_PER_WORD) >>> +#define EDCMR_TO_WORD_SHIFT(r) ((r) % EDCMRS_PER_WORD) >>> + >>> /* TPDM integration test registers */ >>> #define TPDM_ITATBCNTRL (0xEF0) >>> #define TPDM_ITCNTRL (0xF00) >>> @@ -60,14 +71,24 @@ >>> #define TPDM_PIDR0_DS_IMPDEF BIT(0) >>> #define TPDM_PIDR0_DS_DSB BIT(1) >>> +#define TPDM_DSB_MAX_LINES 256 >>> +/* MAX number of EDCR registers */ >>> +#define TPDM_DSB_MAX_EDCR 16 >>> +/* MAX number of EDCMR registers */ >>> +#define TPDM_DSB_MAX_EDCMR 8 >>> + >>> /** >>> * struct dsb_dataset - specifics associated to dsb dataset >>> * @mode: DSB programming mode >>> + * @edge_ctrl: Save value for edge control >>> + * @edge_ctrl_mask: Save value for edge control mask >>> * @trig_ts: Enable/Disable trigger timestamp. >>> * @trig_type: Enable/Disable trigger type. >>> */ >>> struct dsb_dataset { >>> u32 mode; >>> + u32 edge_ctrl[TPDM_DSB_MAX_EDCR]; >>> + u32 edge_ctrl_mask[TPDM_DSB_MAX_EDCMR]; >> >> minor nit: Please align it with the fields below. > > Sure, I will update this in the next patch series. > > > Best, > > Tao > >> >>> bool trig_ts; >>> bool trig_type; >>> }; >> >> Suzuki
On 02/06/2023 09:45, Suzuki K Poulose wrote: > On 02/06/2023 09:21, Tao Zhang wrote: >> >> On 6/1/2023 8:14 PM, Suzuki K Poulose wrote: >>> On 27/04/2023 10:00, Tao Zhang wrote: >>>> Add the nodes to set value for DSB edge control and DSB edge >>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR >>>> resgisters to configure edge control. DSB edge detection control >>>> 00: Rising edge detection >>>> 01: Falling edge detection >>>> 10: Rising and falling edge detection (toggle detection) >>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to >>>> configure mask. Eight 32 bit registers providing DSB interface >>>> edge detection mask control. >>>> >>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>>> --- >>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++ >>>> drivers/hwtracing/coresight/coresight-tpdm.c | 135 >>>> ++++++++++++++++++++- >>>> drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++ >>>> 3 files changed, 187 insertions(+), 1 deletion(-) >>>> >>>> diff --git >>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> index 348e167..a57f000 100644 >>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> @@ -60,3 +60,35 @@ Description: >>>> Bit[3] : Set to 0 for low performance mode. >>>> Set to 1 for high performance mode. >>>> Bit[4:8] : Select byte lane for high performance mode. >>>> + >>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl >>>> +Date: March 2023 >>>> +KernelVersion 6.3 >>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>> +Description: >>>> + Read/Write a set of the edge control registers of the DSB >>>> + in TPDM. >>>> + >>>> + Expected format is the following: >>>> + <integer1> <integer2> <integer3> >>>> + >>>> + Where: >>>> + <integer1> : Start EDCR register number >>>> + <integer2> : End EDCR register number >>>> + <integer3> : The value need to be written >>>> + >>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask >>>> +Date: March 2023 >>>> +KernelVersion 6.3 >>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>> +Description: >>>> + Read/Write a set of the edge control mask registers of the >>>> + DSB in TPDM. >>>> + >>>> + Expected format is the following: >>>> + <integer1> <integer2> <integer3> >>>> + >>>> + Where: >>>> + <integer1> : Start EDCMR register number >>>> + <integer2> : End EDCMR register number >>>> + <integer3> : The value need to be written >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >>>> b/drivers/hwtracing/coresight/coresight-tpdm.c >>>> index 1bacaa5..a40e458 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>> @@ -80,7 +80,14 @@ static void set_trigger_type(struct tpdm_drvdata >>>> *drvdata, u32 *val) >>>> static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) >>>> { >>>> - u32 val; >>>> + u32 val, i; >>>> + >>>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) >>>> + writel_relaxed(drvdata->dsb->edge_ctrl[i], >>>> + drvdata->base + TPDM_DSB_EDCR(i)); >>>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) >>>> + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], >>>> + drvdata->base + TPDM_DSB_EDCMR(i)); >>> >>> Do all TPDM DSBs have MAX_EDCR registers ? Or some have less than that ? >>> If it is latter, do we need special care to avoid writing to inexistent >>> registers ? >>> >> You are right, not all DSB TPDMs have MAX_EDCR registers. In our >> design, the inexistent register addresses >> >> are not occupied and safe for accessing. Does the TRM for the component say so ? Or is it by luck ? If the spec says it is RAZ/WriteIgnore, then we could keep the code as it is, with a comment. Otherwise, we could add a DT property. So please get this clarified with the H/W designers. Suzuki
On 6/2/2023 4:45 PM, Suzuki K Poulose wrote: > On 02/06/2023 09:21, Tao Zhang wrote: >> >> On 6/1/2023 8:14 PM, Suzuki K Poulose wrote: >>> On 27/04/2023 10:00, Tao Zhang wrote: >>>> Add the nodes to set value for DSB edge control and DSB edge >>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR >>>> resgisters to configure edge control. DSB edge detection control >>>> 00: Rising edge detection >>>> 01: Falling edge detection >>>> 10: Rising and falling edge detection (toggle detection) >>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to >>>> configure mask. Eight 32 bit registers providing DSB interface >>>> edge detection mask control. >>>> >>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>>> --- >>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++ >>>> drivers/hwtracing/coresight/coresight-tpdm.c | 135 >>>> ++++++++++++++++++++- >>>> drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++ >>>> 3 files changed, 187 insertions(+), 1 deletion(-) >>>> >>>> diff --git >>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> index 348e167..a57f000 100644 >>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> @@ -60,3 +60,35 @@ Description: >>>> Bit[3] : Set to 0 for low performance mode. >>>> Set to 1 for high performance mode. >>>> Bit[4:8] : Select byte lane for high performance mode. >>>> + >>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl >>>> +Date: March 2023 >>>> +KernelVersion 6.3 >>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>> +Description: >>>> + Read/Write a set of the edge control registers of the DSB >>>> + in TPDM. >>>> + >>>> + Expected format is the following: >>>> + <integer1> <integer2> <integer3> >>>> + >>>> + Where: >>>> + <integer1> : Start EDCR register number >>>> + <integer2> : End EDCR register number >>>> + <integer3> : The value need to be written >>>> + >>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask >>>> +Date: March 2023 >>>> +KernelVersion 6.3 >>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>> +Description: >>>> + Read/Write a set of the edge control mask registers of the >>>> + DSB in TPDM. >>>> + >>>> + Expected format is the following: >>>> + <integer1> <integer2> <integer3> >>>> + >>>> + Where: >>>> + <integer1> : Start EDCMR register number >>>> + <integer2> : End EDCMR register number >>>> + <integer3> : The value need to be written >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >>>> b/drivers/hwtracing/coresight/coresight-tpdm.c >>>> index 1bacaa5..a40e458 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>> @@ -80,7 +80,14 @@ static void set_trigger_type(struct tpdm_drvdata >>>> *drvdata, u32 *val) >>>> static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) >>>> { >>>> - u32 val; >>>> + u32 val, i; >>>> + >>>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) >>>> + writel_relaxed(drvdata->dsb->edge_ctrl[i], >>>> + drvdata->base + TPDM_DSB_EDCR(i)); >>>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) >>>> + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], >>>> + drvdata->base + TPDM_DSB_EDCMR(i)); >>> >>> Do all TPDM DSBs have MAX_EDCR registers ? Or some have less than >>> that ? >>> If it is latter, do we need special care to avoid writing to inexistent >>> registers ? >>> >> You are right, not all DSB TPDMs have MAX_EDCR registers. In our >> design, the inexistent register addresses >> >> are not occupied and safe for accessing. >> >> Currently we don't have a good way to know the quantity of EDCR/EDCMR >> registers for DSB TPDMs. >> >> The only way we can think of is to set it in device tree manually. >> >> Do you have other suggestion for this? >> >>>> val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); >>>> /* Set trigger timestamp */ >>>> @@ -313,6 +320,130 @@ static ssize_t dsb_mode_store(struct device >>>> *dev, >>>> } >>>> static DEVICE_ATTR_RW(dsb_mode); >>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev, >>>> + struct device_attribute *attr, >>>> + char *buf) >>>> +{ >>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + ssize_t size = 0; >>>> + int i; >>>> + >>>> + spin_lock(&drvdata->spinlock); >>>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { >>>> + size += sysfs_emit_at(buf, size, >>>> + "Index:0x%x Val:0x%x\n", i, >>>> + drvdata->dsb->edge_ctrl[i]); >>> >>> It may be safe, but please add a check to make sure that we don't >>> overflow. At least bail out when we hit a return of 0, indicating >>> reached the end of buffer. >>> >> Can I add the following check to replace the current code?? >> >> int ret = 0; >> >> >> for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { >> >> ret = sysfs_emit_at(buf, size, "Index:0x%x Val:0x%x\n", i, >> drvdata->dsb->edge_ctrl[i]); >> >> if (!ret) { >> >> dev_warn(drvdata->dev, "The buffer has been overflowed\n"); > > You don't need this, it already triggers a WARN() in sysfs_emit_at(). > So you could do: > > for (....) { > unsigned long bytes = sysfs_emit_at(buf, size, ....); > > if (bytes <= 0) > break; > size += bytes; > } > Sure, I will update in the next patch series. > >> >> spin_unlock(&drvdata->spinlock); >> >> return size; >> >> } else >> >> size += ret; >> >> } >> >>>> + } >>>> + spin_unlock(&drvdata->spinlock); >>>> + return size; >>>> +} >>>> + >>>> +/* >>>> + * value 1: Start EDCR register number >>>> + * value 2: End EDCR register number >>>> + * value 3: The value need to be written >>>> + * The EDCR registers can include up to 16 32-bit registers, and each >>>> + * one can be configured to control up to 16 edge detections(2 bits >>>> + * control one edge detection). So a total 256 edge detections can be >>>> + * configured. So the starting number(value 1) and ending >>>> number(value 2) >>>> + * cannot be greater than 256, and value 1 should be less than >>>> value 2. >>>> + * The following values are the rage of value 3. >>>> + * 0 - Rising edge detection >>>> + * 1 - Falling edge detection >>>> + * 2 - Rising and falling edge detection (toggle detection) >>>> + */ >>>> +static ssize_t dsb_edge_ctrl_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, >>>> + size_t size) >>>> +{ >>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + unsigned long val, mask, start, end, edge_ctrl, edge_ctrl_shift; >>>> + int i, reg; >>>> + >>>> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3) >>>> + return -EINVAL; >>>> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= >>>> TPDM_DSB_MAX_LINES) || >>>> + (start > end) || (edge_ctrl > 0x2)) >>>> + return -EPERM; >>>> + >>>> + spin_lock(&drvdata->spinlock); >>>> + for (i = start; i <= end; i++) { >>>> + /* >>>> + * There are 2 bit per DSB Edge Control line. >>>> + * Thus we have 16 lines in a 32bit word. >>>> + */ >>>> + reg = EDCR_TO_WORD_IDX(i); >>>> + mask = EDCR_TO_WORD_MASK(i); >>>> + val = drvdata->dsb->edge_ctrl[reg]; >>> >>>> + edge_ctrl_shift = EDCR_TO_WORD_VAL(edge_ctrl, i); >>>> + bitmap_replace(&val, &val, &edge_ctrl_shift, &mask, 32); >>> >>> Could we simply do : >>> >>> reg &= ~mask; >>> reg |= FIELD_PREP(mask, edge_ctrl); >>> >> Perhaps "FIELD_PREP" cannot be used here since "mask" must be >> constant in this macro. > > Ah, you are right. Sorry about that. > >> >> But in our code, the variable "mask" is not constant. > > Still I think using the bitmap_replace is an overkill. We could simply > do: > val &= ~mask; > val |= EDCR_TO_WORD_VAL(edge_ctrl, i); Sure, I will update in the next patch series. Best, Tao > > Suzuki > >>> >>>> + drvdata->dsb->edge_ctrl[reg] = val; >>>> + } >>>> + spin_unlock(&drvdata->spinlock); >>>> + >>>> + return size; >>>> +} >>>> +static DEVICE_ATTR_RW(dsb_edge_ctrl); >>>> + >>>> +static ssize_t dsb_edge_ctrl_mask_show(struct device *dev, >>>> + struct device_attribute *attr, >>>> + char *buf) >>>> +{ >>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + ssize_t size = 0; >>>> + int i; >>>> + >>>> + spin_lock(&drvdata->spinlock); >>>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) { >>>> + size += sysfs_emit_at(buf, size, >>>> + "Index:0x%x Val:0x%x\n", i, >>>> + drvdata->dsb->edge_ctrl_mask[i]); >>>> + } >>>> + spin_unlock(&drvdata->spinlock); >>>> + return size; >>>> +} >>>> + >>>> +/* >>>> + * value 1: Start EDCMR register number >>>> + * value 2: End EDCMR register number >>>> + * value 3: The value need to be written >>>> + */ >>>> +static ssize_t dsb_edge_ctrl_mask_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, >>>> + size_t size) >>>> +{ >>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + unsigned long start, end, val; >>>> + u32 set; >>>> + int i, reg; >>>> + >>>> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3) >>>> + return -EINVAL; >>>> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) >>>> + || (start > end) || (val & ~1UL)) >>>> + return -EPERM; >>>> + >>>> + spin_lock(&drvdata->spinlock); >>>> + for (i = start; i <= end; i++) { >>>> + /* >>>> + * There is 1 bit per DSB Edge Control Mark line. >>>> + * Thus we have 32 lines in a 32bit word. >>>> + */ >>>> + reg = EDCMR_TO_WORD_IDX(i); >>>> + set = drvdata->dsb->edge_ctrl_mask[reg]; >>>> + if (val) >>>> + set |= BIT(EDCR_TO_WORD_SHIFT(i)); >>>> + else >>>> + set &= ~BIT(EDCR_TO_WORD_SHIFT(i)); >>>> + drvdata->dsb->edge_ctrl_mask[reg] = set; >>>> + } >>>> + spin_unlock(&drvdata->spinlock); >>>> + return size; >>>> +} >>>> +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask); >>>> + >>>> static ssize_t dsb_trig_type_show(struct device *dev, >>>> struct device_attribute *attr, char *buf) >>>> { >>>> @@ -385,6 +516,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts); >>>> static struct attribute *tpdm_dsb_attrs[] = { >>>> &dev_attr_dsb_mode.attr, >>>> + &dev_attr_dsb_edge_ctrl.attr, >>>> + &dev_attr_dsb_edge_ctrl_mask.attr, >>>> &dev_attr_dsb_trig_ts.attr, >>>> &dev_attr_dsb_trig_type.attr, >>>> NULL, >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h >>>> b/drivers/hwtracing/coresight/coresight-tpdm.h >>>> index 79df07e..f25dcdec 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>> @@ -12,6 +12,8 @@ >>>> /* DSB Subunit Registers */ >>>> #define TPDM_DSB_CR (0x780) >>>> #define TPDM_DSB_TIER (0x784) >>>> +#define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) >>>> +#define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) >>>> /* Enable bit for DSB subunit */ >>>> #define TPDM_DSB_CR_ENA BIT(0) >>>> @@ -34,6 +36,15 @@ >>>> #define TPDM_DSB_TEST_MODE GENMASK(10, 9) >>>> #define TPDM_DSB_HPSEL GENMASK(6, 2) >>>> +#define EDCRS_PER_WORD 16 >>>> +#define EDCR_TO_WORD_IDX(r) ((r) / EDCRS_PER_WORD) >>>> +#define EDCR_TO_WORD_SHIFT(r) ((r % EDCRS_PER_WORD) * 2) >>>> +#define EDCR_TO_WORD_VAL(val, r) (val << EDCR_TO_WORD_SHIFT(r)) >>>> +#define EDCR_TO_WORD_MASK(r) EDCR_TO_WORD_VAL(0x3, r) >>> >>> minor nit: add a new line here please >> Sure, I will update this in the next patch series. >>> >>>> +#define EDCMRS_PER_WORD 32 >>>> +#define EDCMR_TO_WORD_IDX(r) ((r) / EDCMRS_PER_WORD) >>>> +#define EDCMR_TO_WORD_SHIFT(r) ((r) % EDCMRS_PER_WORD) >>>> + >>>> /* TPDM integration test registers */ >>>> #define TPDM_ITATBCNTRL (0xEF0) >>>> #define TPDM_ITCNTRL (0xF00) >>>> @@ -60,14 +71,24 @@ >>>> #define TPDM_PIDR0_DS_IMPDEF BIT(0) >>>> #define TPDM_PIDR0_DS_DSB BIT(1) >>>> +#define TPDM_DSB_MAX_LINES 256 >>>> +/* MAX number of EDCR registers */ >>>> +#define TPDM_DSB_MAX_EDCR 16 >>>> +/* MAX number of EDCMR registers */ >>>> +#define TPDM_DSB_MAX_EDCMR 8 >>>> + >>>> /** >>>> * struct dsb_dataset - specifics associated to dsb dataset >>>> * @mode: DSB programming mode >>>> + * @edge_ctrl: Save value for edge control >>>> + * @edge_ctrl_mask: Save value for edge control mask >>>> * @trig_ts: Enable/Disable trigger timestamp. >>>> * @trig_type: Enable/Disable trigger type. >>>> */ >>>> struct dsb_dataset { >>>> u32 mode; >>>> + u32 edge_ctrl[TPDM_DSB_MAX_EDCR]; >>>> + u32 edge_ctrl_mask[TPDM_DSB_MAX_EDCMR]; >>> >>> minor nit: Please align it with the fields below. >> >> Sure, I will update this in the next patch series. >> >> >> Best, >> >> Tao >> >>> >>>> bool trig_ts; >>>> bool trig_type; >>>> }; >>> >>> Suzuki >
On 02/06/2023 15:38, Tao Zhang wrote: > > On 6/2/2023 4:45 PM, Suzuki K Poulose wrote: >> On 02/06/2023 09:21, Tao Zhang wrote: >>> >>> On 6/1/2023 8:14 PM, Suzuki K Poulose wrote: >>>> On 27/04/2023 10:00, Tao Zhang wrote: >>>>> Add the nodes to set value for DSB edge control and DSB edge >>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR >>>>> resgisters to configure edge control. DSB edge detection control >>>>> 00: Rising edge detection >>>>> 01: Falling edge detection >>>>> 10: Rising and falling edge detection (toggle detection) >>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to >>>>> configure mask. Eight 32 bit registers providing DSB interface >>>>> edge detection mask control. >>>>> >>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>>>> --- >>>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++ >>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 135 >>>>> ++++++++++++++++++++- >>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++ >>>>> 3 files changed, 187 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git >>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>>> index 348e167..a57f000 100644 >>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>>> @@ -60,3 +60,35 @@ Description: >>>>> Bit[3] : Set to 0 for low performance mode. >>>>> Set to 1 for high performance mode. >>>>> Bit[4:8] : Select byte lane for high performance mode. >>>>> + >>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl >>>>> +Date: March 2023 >>>>> +KernelVersion 6.3 >>>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>>> +Description: >>>>> + Read/Write a set of the edge control registers of the DSB >>>>> + in TPDM. >>>>> + >>>>> + Expected format is the following: >>>>> + <integer1> <integer2> <integer3> >>>>> + >>>>> + Where: >>>>> + <integer1> : Start EDCR register number >>>>> + <integer2> : End EDCR register number >>>>> + <integer3> : The value need to be written >>>>> + >>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask >>>>> +Date: March 2023 >>>>> +KernelVersion 6.3 >>>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>>> +Description: >>>>> + Read/Write a set of the edge control mask registers of the >>>>> + DSB in TPDM. >>>>> + >>>>> + Expected format is the following: >>>>> + <integer1> <integer2> <integer3> >>>>> + >>>>> + Where: >>>>> + <integer1> : Start EDCMR register number >>>>> + <integer2> : End EDCMR register number >>>>> + <integer3> : The value need to be written >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> index 1bacaa5..a40e458 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> @@ -80,7 +80,14 @@ static void set_trigger_type(struct tpdm_drvdata >>>>> *drvdata, u32 *val) >>>>> static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) >>>>> { >>>>> - u32 val; >>>>> + u32 val, i; >>>>> + >>>>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) >>>>> + writel_relaxed(drvdata->dsb->edge_ctrl[i], >>>>> + drvdata->base + TPDM_DSB_EDCR(i)); >>>>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) >>>>> + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], >>>>> + drvdata->base + TPDM_DSB_EDCMR(i)); >>>> >>>> Do all TPDM DSBs have MAX_EDCR registers ? Or some have less than >>>> that ? >>>> If it is latter, do we need special care to avoid writing to inexistent >>>> registers ? >>>> >>> You are right, not all DSB TPDMs have MAX_EDCR registers. In our >>> design, the inexistent register addresses >>> >>> are not occupied and safe for accessing. >>> >>> Currently we don't have a good way to know the quantity of EDCR/EDCMR >>> registers for DSB TPDMs. >>> >>> The only way we can think of is to set it in device tree manually. >>> >>> Do you have other suggestion for this? >>> >>>>> val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); >>>>> /* Set trigger timestamp */ >>>>> @@ -313,6 +320,130 @@ static ssize_t dsb_mode_store(struct device >>>>> *dev, >>>>> } >>>>> static DEVICE_ATTR_RW(dsb_mode); >>>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev, >>>>> + struct device_attribute *attr, >>>>> + char *buf) >>>>> +{ >>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>>> + ssize_t size = 0; >>>>> + int i; >>>>> + >>>>> + spin_lock(&drvdata->spinlock); >>>>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { >>>>> + size += sysfs_emit_at(buf, size, >>>>> + "Index:0x%x Val:0x%x\n", i, >>>>> + drvdata->dsb->edge_ctrl[i]); >>>> >>>> It may be safe, but please add a check to make sure that we don't >>>> overflow. At least bail out when we hit a return of 0, indicating >>>> reached the end of buffer. >>>> >>> Can I add the following check to replace the current code?? >>> >>> int ret = 0; >>> >>> >>> for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { >>> >>> ret = sysfs_emit_at(buf, size, "Index:0x%x Val:0x%x\n", i, >>> drvdata->dsb->edge_ctrl[i]); >>> >>> if (!ret) { >>> >>> dev_warn(drvdata->dev, "The buffer has been overflowed\n"); >> >> You don't need this, it already triggers a WARN() in sysfs_emit_at(). >> So you could do: >> >> for (....) { >> unsigned long bytes = sysfs_emit_at(buf, size, ....); >> >> if (bytes <= 0) >> break; >> size += bytes; >> } >> > Sure, I will update in the next patch series. >> >>> >>> spin_unlock(&drvdata->spinlock); >>> >>> return size; >>> >>> } else >>> >>> size += ret; >>> >>> } >>> >>>>> + } >>>>> + spin_unlock(&drvdata->spinlock); >>>>> + return size; >>>>> +} >>>>> + >>>>> +/* >>>>> + * value 1: Start EDCR register number >>>>> + * value 2: End EDCR register number >>>>> + * value 3: The value need to be written >>>>> + * The EDCR registers can include up to 16 32-bit registers, and each >>>>> + * one can be configured to control up to 16 edge detections(2 bits >>>>> + * control one edge detection). So a total 256 edge detections can be >>>>> + * configured. So the starting number(value 1) and ending >>>>> number(value 2) >>>>> + * cannot be greater than 256, and value 1 should be less than >>>>> value 2. >>>>> + * The following values are the rage of value 3. >>>>> + * 0 - Rising edge detection >>>>> + * 1 - Falling edge detection >>>>> + * 2 - Rising and falling edge detection (toggle detection) >>>>> + */ >>>>> +static ssize_t dsb_edge_ctrl_store(struct device *dev, >>>>> + struct device_attribute *attr, >>>>> + const char *buf, >>>>> + size_t size) >>>>> +{ >>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>>> + unsigned long val, mask, start, end, edge_ctrl, edge_ctrl_shift; >>>>> + int i, reg; >>>>> + >>>>> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3) >>>>> + return -EINVAL; >>>>> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= >>>>> TPDM_DSB_MAX_LINES) || >>>>> + (start > end) || (edge_ctrl > 0x2)) >>>>> + return -EPERM; >>>>> + >>>>> + spin_lock(&drvdata->spinlock); >>>>> + for (i = start; i <= end; i++) { >>>>> + /* >>>>> + * There are 2 bit per DSB Edge Control line. >>>>> + * Thus we have 16 lines in a 32bit word. >>>>> + */ >>>>> + reg = EDCR_TO_WORD_IDX(i); >>>>> + mask = EDCR_TO_WORD_MASK(i); >>>>> + val = drvdata->dsb->edge_ctrl[reg]; >>>> >>>>> + edge_ctrl_shift = EDCR_TO_WORD_VAL(edge_ctrl, i); >>>>> + bitmap_replace(&val, &val, &edge_ctrl_shift, &mask, 32); >>>> >>>> Could we simply do : >>>> >>>> reg &= ~mask; >>>> reg |= FIELD_PREP(mask, edge_ctrl); >>>> >>> Perhaps "FIELD_PREP" cannot be used here since "mask" must be >>> constant in this macro. >> >> Ah, you are right. Sorry about that. >> >>> >>> But in our code, the variable "mask" is not constant. >> >> Still I think using the bitmap_replace is an overkill. We could simply >> do: >> val &= ~mask; >> val |= EDCR_TO_WORD_VAL(edge_ctrl, i); Since we don't need mask any longer we could even do : val &= ~EDCR_TO_WORD_MASK(i); val |= EDCR_TO_WORD_VAL(edge_ctrl, i); Suzuki
On 6/2/2023 5:00 PM, Suzuki K Poulose wrote: > On 02/06/2023 09:45, Suzuki K Poulose wrote: >> On 02/06/2023 09:21, Tao Zhang wrote: >>> >>> On 6/1/2023 8:14 PM, Suzuki K Poulose wrote: >>>> On 27/04/2023 10:00, Tao Zhang wrote: >>>>> Add the nodes to set value for DSB edge control and DSB edge >>>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR >>>>> resgisters to configure edge control. DSB edge detection control >>>>> 00: Rising edge detection >>>>> 01: Falling edge detection >>>>> 10: Rising and falling edge detection (toggle detection) >>>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to >>>>> configure mask. Eight 32 bit registers providing DSB interface >>>>> edge detection mask control. >>>>> >>>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>>>> --- >>>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++ >>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 135 >>>>> ++++++++++++++++++++- >>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++ >>>>> 3 files changed, 187 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git >>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>>> index 348e167..a57f000 100644 >>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>>> @@ -60,3 +60,35 @@ Description: >>>>> Bit[3] : Set to 0 for low performance mode. >>>>> Set to 1 for high performance mode. >>>>> Bit[4:8] : Select byte lane for high performance mode. >>>>> + >>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl >>>>> +Date: March 2023 >>>>> +KernelVersion 6.3 >>>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>>> +Description: >>>>> + Read/Write a set of the edge control registers of the DSB >>>>> + in TPDM. >>>>> + >>>>> + Expected format is the following: >>>>> + <integer1> <integer2> <integer3> >>>>> + >>>>> + Where: >>>>> + <integer1> : Start EDCR register number >>>>> + <integer2> : End EDCR register number >>>>> + <integer3> : The value need to be written >>>>> + >>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask >>>>> +Date: March 2023 >>>>> +KernelVersion 6.3 >>>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>>> +Description: >>>>> + Read/Write a set of the edge control mask registers of the >>>>> + DSB in TPDM. >>>>> + >>>>> + Expected format is the following: >>>>> + <integer1> <integer2> <integer3> >>>>> + >>>>> + Where: >>>>> + <integer1> : Start EDCMR register number >>>>> + <integer2> : End EDCMR register number >>>>> + <integer3> : The value need to be written >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> index 1bacaa5..a40e458 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> @@ -80,7 +80,14 @@ static void set_trigger_type(struct >>>>> tpdm_drvdata *drvdata, u32 *val) >>>>> static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) >>>>> { >>>>> - u32 val; >>>>> + u32 val, i; >>>>> + >>>>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) >>>>> + writel_relaxed(drvdata->dsb->edge_ctrl[i], >>>>> + drvdata->base + TPDM_DSB_EDCR(i)); >>>>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) >>>>> + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], >>>>> + drvdata->base + TPDM_DSB_EDCMR(i)); >>>> >>>> Do all TPDM DSBs have MAX_EDCR registers ? Or some have less than >>>> that ? >>>> If it is latter, do we need special care to avoid writing to >>>> inexistent >>>> registers ? >>>> >>> You are right, not all DSB TPDMs have MAX_EDCR registers. In our >>> design, the inexistent register addresses >>> >>> are not occupied and safe for accessing. > > Does the TRM for the component say so ? Or is it by luck ? If the spec > says it is RAZ/WriteIgnore, then we could keep the code as it is, > with a comment. Otherwise, we could add a DT property. So please get > this clarified with the H/W designers. Confirmed with H/W designers, these addresses are reserved for the maximum quantity of EDCR/EDCMR registers. It is safe to write data to them and it will not impact anything. Best, Tao > > Suzuki
diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm index 348e167..a57f000 100644 --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm @@ -60,3 +60,35 @@ Description: Bit[3] : Set to 0 for low performance mode. Set to 1 for high performance mode. Bit[4:8] : Select byte lane for high performance mode. + +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl +Date: March 2023 +KernelVersion 6.3 +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> +Description: + Read/Write a set of the edge control registers of the DSB + in TPDM. + + Expected format is the following: + <integer1> <integer2> <integer3> + + Where: + <integer1> : Start EDCR register number + <integer2> : End EDCR register number + <integer3> : The value need to be written + +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask +Date: March 2023 +KernelVersion 6.3 +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> +Description: + Read/Write a set of the edge control mask registers of the + DSB in TPDM. + + Expected format is the following: + <integer1> <integer2> <integer3> + + Where: + <integer1> : Start EDCMR register number + <integer2> : End EDCMR register number + <integer3> : The value need to be written diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 1bacaa5..a40e458 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -80,7 +80,14 @@ static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val) static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) { - u32 val; + u32 val, i; + + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) + writel_relaxed(drvdata->dsb->edge_ctrl[i], + drvdata->base + TPDM_DSB_EDCR(i)); + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], + drvdata->base + TPDM_DSB_EDCMR(i)); val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); /* Set trigger timestamp */ @@ -313,6 +320,130 @@ static ssize_t dsb_mode_store(struct device *dev, } static DEVICE_ATTR_RW(dsb_mode); +static ssize_t dsb_edge_ctrl_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); + ssize_t size = 0; + int i; + + spin_lock(&drvdata->spinlock); + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { + size += sysfs_emit_at(buf, size, + "Index:0x%x Val:0x%x\n", i, + drvdata->dsb->edge_ctrl[i]); + } + spin_unlock(&drvdata->spinlock); + return size; +} + +/* + * value 1: Start EDCR register number + * value 2: End EDCR register number + * value 3: The value need to be written + * The EDCR registers can include up to 16 32-bit registers, and each + * one can be configured to control up to 16 edge detections(2 bits + * control one edge detection). So a total 256 edge detections can be + * configured. So the starting number(value 1) and ending number(value 2) + * cannot be greater than 256, and value 1 should be less than value 2. + * The following values are the rage of value 3. + * 0 - Rising edge detection + * 1 - Falling edge detection + * 2 - Rising and falling edge detection (toggle detection) + */ +static ssize_t dsb_edge_ctrl_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t size) +{ + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); + unsigned long val, mask, start, end, edge_ctrl, edge_ctrl_shift; + int i, reg; + + if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3) + return -EINVAL; + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) || + (start > end) || (edge_ctrl > 0x2)) + return -EPERM; + + spin_lock(&drvdata->spinlock); + for (i = start; i <= end; i++) { + /* + * There are 2 bit per DSB Edge Control line. + * Thus we have 16 lines in a 32bit word. + */ + reg = EDCR_TO_WORD_IDX(i); + mask = EDCR_TO_WORD_MASK(i); + val = drvdata->dsb->edge_ctrl[reg]; + edge_ctrl_shift = EDCR_TO_WORD_VAL(edge_ctrl, i); + bitmap_replace(&val, &val, &edge_ctrl_shift, &mask, 32); + drvdata->dsb->edge_ctrl[reg] = val; + } + spin_unlock(&drvdata->spinlock); + + return size; +} +static DEVICE_ATTR_RW(dsb_edge_ctrl); + +static ssize_t dsb_edge_ctrl_mask_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); + ssize_t size = 0; + int i; + + spin_lock(&drvdata->spinlock); + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) { + size += sysfs_emit_at(buf, size, + "Index:0x%x Val:0x%x\n", i, + drvdata->dsb->edge_ctrl_mask[i]); + } + spin_unlock(&drvdata->spinlock); + return size; +} + +/* + * value 1: Start EDCMR register number + * value 2: End EDCMR register number + * value 3: The value need to be written + */ +static ssize_t dsb_edge_ctrl_mask_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t size) +{ + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); + unsigned long start, end, val; + u32 set; + int i, reg; + + if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3) + return -EINVAL; + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) + || (start > end) || (val & ~1UL)) + return -EPERM; + + spin_lock(&drvdata->spinlock); + for (i = start; i <= end; i++) { + /* + * There is 1 bit per DSB Edge Control Mark line. + * Thus we have 32 lines in a 32bit word. + */ + reg = EDCMR_TO_WORD_IDX(i); + set = drvdata->dsb->edge_ctrl_mask[reg]; + if (val) + set |= BIT(EDCR_TO_WORD_SHIFT(i)); + else + set &= ~BIT(EDCR_TO_WORD_SHIFT(i)); + drvdata->dsb->edge_ctrl_mask[reg] = set; + } + spin_unlock(&drvdata->spinlock); + return size; +} +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask); + static ssize_t dsb_trig_type_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -385,6 +516,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts); static struct attribute *tpdm_dsb_attrs[] = { &dev_attr_dsb_mode.attr, + &dev_attr_dsb_edge_ctrl.attr, + &dev_attr_dsb_edge_ctrl_mask.attr, &dev_attr_dsb_trig_ts.attr, &dev_attr_dsb_trig_type.attr, NULL, diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h index 79df07e..f25dcdec 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.h +++ b/drivers/hwtracing/coresight/coresight-tpdm.h @@ -12,6 +12,8 @@ /* DSB Subunit Registers */ #define TPDM_DSB_CR (0x780) #define TPDM_DSB_TIER (0x784) +#define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) +#define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) /* Enable bit for DSB subunit */ #define TPDM_DSB_CR_ENA BIT(0) @@ -34,6 +36,15 @@ #define TPDM_DSB_TEST_MODE GENMASK(10, 9) #define TPDM_DSB_HPSEL GENMASK(6, 2) +#define EDCRS_PER_WORD 16 +#define EDCR_TO_WORD_IDX(r) ((r) / EDCRS_PER_WORD) +#define EDCR_TO_WORD_SHIFT(r) ((r % EDCRS_PER_WORD) * 2) +#define EDCR_TO_WORD_VAL(val, r) (val << EDCR_TO_WORD_SHIFT(r)) +#define EDCR_TO_WORD_MASK(r) EDCR_TO_WORD_VAL(0x3, r) +#define EDCMRS_PER_WORD 32 +#define EDCMR_TO_WORD_IDX(r) ((r) / EDCMRS_PER_WORD) +#define EDCMR_TO_WORD_SHIFT(r) ((r) % EDCMRS_PER_WORD) + /* TPDM integration test registers */ #define TPDM_ITATBCNTRL (0xEF0) #define TPDM_ITCNTRL (0xF00) @@ -60,14 +71,24 @@ #define TPDM_PIDR0_DS_IMPDEF BIT(0) #define TPDM_PIDR0_DS_DSB BIT(1) +#define TPDM_DSB_MAX_LINES 256 +/* MAX number of EDCR registers */ +#define TPDM_DSB_MAX_EDCR 16 +/* MAX number of EDCMR registers */ +#define TPDM_DSB_MAX_EDCMR 8 + /** * struct dsb_dataset - specifics associated to dsb dataset * @mode: DSB programming mode + * @edge_ctrl: Save value for edge control + * @edge_ctrl_mask: Save value for edge control mask * @trig_ts: Enable/Disable trigger timestamp. * @trig_type: Enable/Disable trigger type. */ struct dsb_dataset { u32 mode; + u32 edge_ctrl[TPDM_DSB_MAX_EDCR]; + u32 edge_ctrl_mask[TPDM_DSB_MAX_EDCMR]; bool trig_ts; bool trig_type; };
Add the nodes to set value for DSB edge control and DSB edge control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR resgisters to configure edge control. DSB edge detection control 00: Rising edge detection 01: Falling edge detection 10: Rising and falling edge detection (toggle detection) And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to configure mask. Eight 32 bit registers providing DSB interface edge detection mask control. Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> --- .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++ drivers/hwtracing/coresight/coresight-tpdm.c | 135 ++++++++++++++++++++- drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++ 3 files changed, 187 insertions(+), 1 deletion(-)