| Message ID | 1662626705-13097-1-git-send-email-quic_taozha@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support to configure TPDM DSB subunit | expand |
On Thu, Sep 08, 2022 at 04:44:57PM +0800, Tao Zhang wrote: > Add property "qcom,dsb-elem-size" to support DSB element for TPDA. > Specifies the DSB element size supported by each monitor connected > to the aggregator on each port. Should be specified in pairs (port, > dsb element size). What is DSB? > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml > index eb9bfc5..1bb3fdf 100644 > --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml > @@ -40,6 +40,13 @@ properties: > minItems: 1 > maxItems: 2 > > + qcom,dsb-elem-size: > + description: | > + Specifies the DSB element size supported by each monitor > + connected to the aggregator on each port. Should be specified > + in pairs (port, dsb element size). > + $ref: /schemas/types.yaml#/definitions/uint32-array The binding (not yet upstream) says there is just 1 port (port 0). So why do you need more than a single uint32? Rob
在 9/8/2022 6:54 PM, Krzysztof Kozlowski 写道: > On 08/09/2022 10:44, Tao Zhang wrote: >> Add property "qcom,dsb-elem-size" to support DSB element for TPDA. >> Specifies the DSB element size supported by each monitor connected >> to the aggregator on each port. Should be specified in pairs (port, >> dsb element size). >> >> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >> --- >> Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> index eb9bfc5..1bb3fdf 100644 >> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> @@ -40,6 +40,13 @@ properties: >> minItems: 1 >> maxItems: 2 >> >> + qcom,dsb-elem-size: >> + description: | >> + Specifies the DSB element size supported by each monitor >> + connected to the aggregator on each port. Should be specified >> + in pairs (port, dsb element size). >> + $ref: /schemas/types.yaml#/definitions/uint32-array > So it is rather uint32-matrix (need to describe the items subschema). > What about maxItems? > > Best regards, > Krzysztof Yes, indeed it should be uint32-matrix here. I will update in the next release. The "maxItems" cannot be known explicitly because it depends on how many DSB subunit TPDMs are connected to the TPDA input ports. Usually the number of the items is 1 to several, but there is no limit to its maximum value. Best regards, Tao
在 9/13/2022 8:48 AM, Rob Herring 写道: > On Thu, Sep 08, 2022 at 04:44:57PM +0800, Tao Zhang wrote: >> Add property "qcom,dsb-elem-size" to support DSB element for TPDA. >> Specifies the DSB element size supported by each monitor connected >> to the aggregator on each port. Should be specified in pairs (port, >> dsb element size). > What is DSB? The full name of DSB is "Discrete Single Bit". The DSB element size supported by different DSB subunit TPDMs is different, so TPDA needs to be informed through configuration in device tree. >> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >> --- >> Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> index eb9bfc5..1bb3fdf 100644 >> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> @@ -40,6 +40,13 @@ properties: >> minItems: 1 >> maxItems: 2 >> >> + qcom,dsb-elem-size: >> + description: | >> + Specifies the DSB element size supported by each monitor >> + connected to the aggregator on each port. Should be specified >> + in pairs (port, dsb element size). >> + $ref: /schemas/types.yaml#/definitions/uint32-array > The binding (not yet upstream) says there is just 1 port (port 0). So > why do you need more than a single uint32? > > Rob TPDA(Trace, Profiling and Diagnostics Aggregator) is to provide packetization, funneling and timestamping of TPDM data. Multiple monitors are connected to different input ports of TPDA. - - - - - - - - - - - - | TPDM 0| | TPDM 1 | | TPDM 2| - - - - - - - - - - - - | | | |_ _ _ _ _ _ | _ _ _ _ | | | | | | | ------------------ | TPDA | ------------------ There may be multiple DSB subunit TPDMs connected to different input ports of the same TPDA, so we need to use port here to define the distinction in device tree. Best regards, Tao
On 13/09/2022 09:00, Tao Zhang wrote: >>> + qcom,dsb-elem-size: >>> + description: | >>> + Specifies the DSB element size supported by each monitor >>> + connected to the aggregator on each port. Should be specified >>> + in pairs (port, dsb element size). >>> + $ref: /schemas/types.yaml#/definitions/uint32-array >> So it is rather uint32-matrix (need to describe the items subschema). >> What about maxItems? >> >> Best regards, >> Krzysztof > > Yes, indeed it should be uint32-matrix here. I will update in the next > release. > > The "maxItems" cannot be known explicitly because it depends on how many > DSB subunit TPDMs are connected to the TPDA input ports. > > Usually the number of the items is 1 to several, but there is no limit > to its maximum value. OK, thanks. Best regards, Krzysztof
Hi On 08/09/2022 09:44, Tao Zhang wrote: > DSB subunit need to be configured in enablement and disablement. > A struct that specifics associated to dsb dataset is needed. It > saves the configuration and parameters of the dsb datasets. This > change is to add this struct and initialize the configuration of > DSB subunit. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > drivers/hwtracing/coresight/coresight-tpdm.c | 44 ++++++++++++++++++++++++++-- > drivers/hwtracing/coresight/coresight-tpdm.h | 17 +++++++++++ > 2 files changed, 58 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 88df3e6..69ea453 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -24,6 +24,22 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) > { > u32 val; > > + val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); > + /* Set trigger timestamp */ > + if (drvdata->dsb->trig_ts) What happens if this instance doesn't have a DSB set ? Have you tested this on a system without the DSB ? Suzuki
On 08/09/2022 09:45, Tao Zhang wrote: > Add nodes to configure trigger pattern and trigger pattern mask. > Each DSB subunit TPDM has n(0-7) XPR registers to configure > trigger pattern match output. And each DSB subunit TPDM has > m(0-7) XPMR registers to configure trigger pattern mask match > output. > Similar to the previous patch, please add a comment describing what the values are supposed to mean ? And similar comments to the previous patch. Suzuki
On 08/09/2022 09:45, Tao Zhang wrote: > Add nodes to configure the timestamp request based on input > pattern match. Each TPDM that support DSB subunit has n(0-7) TPR > registers to configure value for timestamp request based on input > pattern match, and has m(0-7) TPMR registers to configure pattern > mask for timestamp request. > Add nodes to enable/disable pattern timestamp and set pattern > timestamp type. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > drivers/hwtracing/coresight/coresight-tpdm.c | 189 +++++++++++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tpdm.h | 14 ++ > 2 files changed, 203 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 648bbe6..4212ff4 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -32,6 +32,13 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) > drvdata->base + TPDM_DSB_EDCMR(i)); > > for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { > + writel_relaxed(drvdata->dsb->patt_val[i], > + drvdata->base + TPDM_DSB_TPR(i)); > + writel_relaxed(drvdata->dsb->patt_mask[i], > + drvdata->base + TPDM_DSB_TPMR(i)); > + } > + > + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { > writel_relaxed(drvdata->dsb->trig_patt_val[i], > drvdata->base + TPDM_DSB_XPR(i)); > writel_relaxed(drvdata->dsb->trig_patt_mask[i], > @@ -39,6 +46,16 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) > } > > val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); > + /* Set pattern timestamp type and enablement */ > + if (drvdata->dsb->patt_ts) { > + val |= TPDM_DSB_PATT_TSENAB; > + if (drvdata->dsb->patt_type) > + val |= TPDM_DSB_PATT_TYPE; > + else > + val &= ~TPDM_DSB_PATT_TYPE; > + } else { > + val &= ~TPDM_DSB_PATT_TSENAB; > + } > /* Set trigger timestamp */ > if (drvdata->dsb->trig_ts) > val |= TPDM_DSB_XTRIG_TSENAB; > @@ -411,6 +428,174 @@ static ssize_t dsb_edge_ctrl_mask_store(struct device *dev, > } > static DEVICE_ATTR_RW(dsb_edge_ctrl_mask); > > +static ssize_t dsb_patt_val_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 = 0; > + > + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) > + return -EPERM; > + > + spin_lock(&drvdata->spinlock); > + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->dsb->patt_val[i]); > + } > + spin_unlock(&drvdata->spinlock); > + return size; > +} > + > +/* > + * value 1: Index of TPR register > + * value 2: Value need to be written > + */ > +static ssize_t dsb_patt_val_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 index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) || > + index >= TPDM_DSB_MAX_PATT) > + return -EPERM; > + > + spin_lock(&drvdata->spinlock); > + drvdata->dsb->patt_val[index] = val; > + spin_unlock(&drvdata->spinlock); > + return size; > +} > +static DEVICE_ATTR_RW(dsb_patt_val); > + > +static ssize_t dsb_patt_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 = 0; > + > + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) > + return -EPERM; > + > + spin_lock(&drvdata->spinlock); > + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->dsb->patt_mask[i]); > + } > + spin_unlock(&drvdata->spinlock); > + return size; > +} > + > +/* > + * value 1: Index of TPMR register > + * value 2: Value need to be written > + */ > +static ssize_t dsb_patt_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 index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) || > + index >= TPDM_DSB_MAX_PATT) > + return -EPERM; > + > + spin_lock(&drvdata->spinlock); > + drvdata->dsb->patt_mask[index] = val; > + spin_unlock(&drvdata->spinlock); > + return size; > +} > +static DEVICE_ATTR_RW(dsb_patt_mask); > + > +static ssize_t dsb_patt_ts_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%u\n", > + (unsigned int)drvdata->dsb->patt_ts); Please use sysfs_emit() everywhere (in the previous patches too) for such operations. I have finished reviewing this series. Suzuki
On 26/10/2022 09:10, Tao Zhang wrote: > Hi Suzuki, > > 在 10/24/2022 6:02 PM, Suzuki K Poulose 写道: >> Hi >> >> On 08/09/2022 09:44, Tao Zhang wrote: >>> DSB subunit need to be configured in enablement and disablement. >>> A struct that specifics associated to dsb dataset is needed. It >>> saves the configuration and parameters of the dsb datasets. This >>> change is to add this struct and initialize the configuration of >>> DSB subunit. >>> >>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>> --- >>> drivers/hwtracing/coresight/coresight-tpdm.c | 44 >>> ++++++++++++++++++++++++++-- >>> drivers/hwtracing/coresight/coresight-tpdm.h | 17 +++++++++++ >>> 2 files changed, 58 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >>> b/drivers/hwtracing/coresight/coresight-tpdm.c >>> index 88df3e6..69ea453 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>> @@ -24,6 +24,22 @@ static void tpdm_enable_dsb(struct tpdm_drvdata >>> *drvdata) >>> { >>> u32 val; >>> + val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); >>> + /* Set trigger timestamp */ >>> + if (drvdata->dsb->trig_ts) >> >> What happens if this instance doesn't have a DSB set ? Have >> you tested this on a system without the DSB ? >> > The function "tpdm_enable_dsb" will only be called when it is checked > that the DSB dataset is present. > > And only the TPDM that supports the DSB dataset will have the DSB TIER > register. > > If the TPDM doesn't support the DSB dataset, this instance should not be > run. Otherwise, it will cause that the incorrect register is accessed. Thanks, this is what happens when you send something that is not queued anywhwere. Please provide a reference tree in the future, for ease of reviewing such things Suzuki
Hi Suzuki, On 10/25/2022 6:00 PM, Suzuki K Poulose wrote: > On 08/09/2022 09:45, Tao Zhang wrote: >> Add nodes to configure the timestamp request based on input >> pattern match. Each TPDM that support DSB subunit has n(0-7) TPR >> registers to configure value for timestamp request based on input >> pattern match, and has m(0-7) TPMR registers to configure pattern >> mask for timestamp request. >> Add nodes to enable/disable pattern timestamp and set pattern >> timestamp type. >> >> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >> --- >> drivers/hwtracing/coresight/coresight-tpdm.c | 189 >> +++++++++++++++++++++++++++ >> drivers/hwtracing/coresight/coresight-tpdm.h | 14 ++ >> 2 files changed, 203 insertions(+) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >> b/drivers/hwtracing/coresight/coresight-tpdm.c >> index 648bbe6..4212ff4 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >> @@ -32,6 +32,13 @@ static void tpdm_enable_dsb(struct tpdm_drvdata >> *drvdata) >> drvdata->base + TPDM_DSB_EDCMR(i)); >> for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { >> + writel_relaxed(drvdata->dsb->patt_val[i], >> + drvdata->base + TPDM_DSB_TPR(i)); >> + writel_relaxed(drvdata->dsb->patt_mask[i], >> + drvdata->base + TPDM_DSB_TPMR(i)); >> + } >> + >> + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { >> writel_relaxed(drvdata->dsb->trig_patt_val[i], >> drvdata->base + TPDM_DSB_XPR(i)); >> writel_relaxed(drvdata->dsb->trig_patt_mask[i], >> @@ -39,6 +46,16 @@ static void tpdm_enable_dsb(struct tpdm_drvdata >> *drvdata) >> } >> val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); >> + /* Set pattern timestamp type and enablement */ >> + if (drvdata->dsb->patt_ts) { >> + val |= TPDM_DSB_PATT_TSENAB; >> + if (drvdata->dsb->patt_type) >> + val |= TPDM_DSB_PATT_TYPE; >> + else >> + val &= ~TPDM_DSB_PATT_TYPE; >> + } else { >> + val &= ~TPDM_DSB_PATT_TSENAB; >> + } >> /* Set trigger timestamp */ >> if (drvdata->dsb->trig_ts) >> val |= TPDM_DSB_XTRIG_TSENAB; >> @@ -411,6 +428,174 @@ static ssize_t dsb_edge_ctrl_mask_store(struct >> device *dev, >> } >> static DEVICE_ATTR_RW(dsb_edge_ctrl_mask); >> +static ssize_t dsb_patt_val_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 = 0; >> + >> + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) >> + return -EPERM; >> + >> + spin_lock(&drvdata->spinlock); >> + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { >> + size += scnprintf(buf + size, PAGE_SIZE - size, >> + "Index: 0x%x Value: 0x%x\n", i, >> + drvdata->dsb->patt_val[i]); >> + } >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> + >> +/* >> + * value 1: Index of TPR register >> + * value 2: Value need to be written >> + */ >> +static ssize_t dsb_patt_val_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 index, val; >> + >> + if (sscanf(buf, "%lx %lx", &index, &val) != 2) >> + return -EINVAL; >> + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) || >> + index >= TPDM_DSB_MAX_PATT) >> + return -EPERM; >> + >> + spin_lock(&drvdata->spinlock); >> + drvdata->dsb->patt_val[index] = val; >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> +static DEVICE_ATTR_RW(dsb_patt_val); >> + >> +static ssize_t dsb_patt_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 = 0; >> + >> + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) >> + return -EPERM; >> + >> + spin_lock(&drvdata->spinlock); >> + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { >> + size += scnprintf(buf + size, PAGE_SIZE - size, >> + "Index: 0x%x Value: 0x%x\n", i, >> + drvdata->dsb->patt_mask[i]); >> + } >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> + >> +/* >> + * value 1: Index of TPMR register >> + * value 2: Value need to be written >> + */ >> +static ssize_t dsb_patt_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 index, val; >> + >> + if (sscanf(buf, "%lx %lx", &index, &val) != 2) >> + return -EINVAL; >> + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) || >> + index >= TPDM_DSB_MAX_PATT) >> + return -EPERM; >> + >> + spin_lock(&drvdata->spinlock); >> + drvdata->dsb->patt_mask[index] = val; >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> +static DEVICE_ATTR_RW(dsb_patt_mask); >> + >> +static ssize_t dsb_patt_ts_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + >> + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) >> + return -EPERM; >> + >> + return scnprintf(buf, PAGE_SIZE, "%u\n", >> + (unsigned int)drvdata->dsb->patt_ts); > > Please use sysfs_emit() everywhere (in the previous patches too) > for such operations. > Sure, I will update this in the next patch series. > I have finished reviewing this series. > > Suzuki > _______________________________________________ > CoreSight mailing list -- coresight@lists.linaro.org > To unsubscribe send an email to coresight-leave@lists.linaro.org