Hi Suzuki,
On 10/24/2022 11:47 PM, Suzuki K Poulose wrote:
On 08/09/2022 09:45, Tao Zhang wrote:
Add the nodes to set value for DSB edge control and DSB edge control mask. Each DSB subunit TPDM has n(n<16) EDCR resgisters to configure edge control. And each DSB subunit TPDM has m(m<8) ECDMR registers to configure edge control mask.
Please could you describe what these values represent ? Size of the "value". Without the specs, it is really difficult to comprehend the code.
Please add comments
The registers EDCR control DSB edge detection, and the following values should be writen to them. 00 - Rising edge detection 01 - Falling edge detection 10 - Rising and falling edge detection (toggle detection) The registers EDCMR are eight 32-bit registers, and they provide DSB interface edge detection mask control.
Signed-off-by: Tao Zhang quic_taozha@quicinc.com
drivers/hwtracing/coresight/coresight-tpdm.c | 130 ++++++++++++++++++++++++++- drivers/hwtracing/coresight/coresight-tpdm.h | 11 +++ 2 files changed, 140 insertions(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 7265793..14bcf2b 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -22,7 +22,14 @@ DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm"); static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) { - u32 val, mode; + u32 val, mode, 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_EDCR / 2; i++)
Please use "TPDM_DSB_MAX_EDMCR" for consistency and ease of following the code.
Sure, I will update this in the next patch series.
- 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 */ @@ -278,6 +285,125 @@ 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;
+ if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) + return -EPERM;
+ spin_lock(&drvdata->spinlock); + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { + size += scnprintf(buf + size, PAGE_SIZE - 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
What are the constraints on these inputs ? Please add a comment here. And may be also add a user scenario if possible to describe the values and its effects on the unit ?
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 value2. The following values(value 3) should be writen to EDCR registers. 00 - Rising edge detection 01 - Falling edge detection 10 - 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 start, end, edge_ctrl; + uint32_t val; + int i, bit, reg;
+ if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3) + return -EINVAL; + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) || + (start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) || + edge_ctrl > 0x2) + return -EPERM;
+ spin_lock(&drvdata->spinlock); + for (i = start; i <= end; i++) { + reg = i / (NUM_OF_BITS / 2); + bit = i % (NUM_OF_BITS / 2);
NUM_OF_BITS in what ? That is too generic.
It means the number of bits of a register. The 32-bit register has 32 bits(NUM_OF_BITS), and each one can be configured to control 16(NUM_OF_BITS / 2) edge detectioins. I will update this in the next patch series.
+ bit = bit * 2;
minor nit: Name "bit" is confusing here. s/bit/{index or even field}?
It should be named "index" here. I will update this in the next patch series.
+ val = drvdata->dsb->edge_ctrl[reg]; + val = val & ~GENMASK((bit + 1), bit); + val = val | (edge_ctrl << bit); + 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;
+ if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) + return -EPERM;
+ spin_lock(&drvdata->spinlock); + for (i = 0; i < TPDM_DSB_MAX_EDCR / 2; i++) { + size += scnprintf(buf + size, PAGE_SIZE - 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, bit, reg;
+ if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3) + return -EINVAL; + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) ||
Please use is_visible() hook.
Sure, I will update this in the next patch series.
+ (start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES)) + return -EPERM;
No constraints on "val"? 0 or 1 ?
EDCMR registers are 32-bit registers, and every bit of them provides DSB interface edge detection mask control. So they can be configured to control up to 256 DSB interface edge detection masks. So the number of "start" and "end" should not be greater than 256 here.
+ spin_lock(&drvdata->spinlock); + for (i = start; i <= end; i++) { + reg = i / NUM_OF_BITS; + bit = (i % NUM_OF_BITS);
+ set = drvdata->dsb->edge_ctrl_mask[reg]; + if (val) + set = set | BIT(bit);
minor nit: set |= ..
Sure, I will update this in the next patch series.
+ else + set = set & ~BIT(bit);
set &= .. ?
Sure, I will update this in the next patch series.
- 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) @@ -359,6 +485,8 @@ static ssize_t dsb_trig_ts_store(struct device *dev, 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 4d57488..ed03c68 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) @@ -28,6 +30,8 @@ #define TPDM_DSB_MODE_HPBYTESEL(val) BMVAL(val, 4, 8) #define TPDM_MODE_ALL (0xFFFFFFF) +#define NUM_OF_BITS 32
/* TPDM integration test registers */ #define TPDM_ITATBCNTRL (0xEF0) #define TPDM_ITCNTRL (0xF00) @@ -54,14 +58,21 @@ #define TPDM_PIDR0_DS_IMPDEF BIT(0) #define TPDM_PIDR0_DS_DSB BIT(1) +#define TPDM_DSB_MAX_EDCR 16 > +#define TPDM_DSB_MAX_LINES 256
A comment here would help : e.g, EDCR has 2 bits for each lines. Thus we can support upto (16 * 32) / 2 lines.
May be, even use that math above instead of hard coding 256 ? And explain what each bits would do ?
I will add more comments here in the next patch series.
/** * 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_EDCR / 2];
Please use TPDM_DSB_MAX_EDCMR instead.
Sure. I will update this in the next patch series.
Suzuki
bool trig_ts; bool trig_type; };
Best Regards
Tao