| Message ID | 1682586037-25973-1-git-send-email-quic_taozha@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support to configure TPDM DSB subunit | expand |
On 27/04/2023 10:00, Tao Zhang wrote: > Read the DSB element size from the device tree. Set the register > bit that controls the DSB element size of the corresponding port. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 1 + > drivers/hwtracing/coresight/coresight-tpda.c | 92 +++++++++++++++++++++++++--- > drivers/hwtracing/coresight/coresight-tpda.h | 4 ++ > drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- > include/linux/coresight.h | 1 + > 5 files changed, 90 insertions(+), 10 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 2af416b..f1eacbb 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct coresight_device *csdev, > > if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC && > subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE && > + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM && > subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) { > dev_err(&csdev->dev, "wrong device subtype in %s\n", function); > return -EINVAL; Please see the comment at the bottom. > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c > index 8d2b9d2..af9c72f 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.c > +++ b/drivers/hwtracing/coresight/coresight-tpda.c > @@ -21,6 +21,56 @@ > > DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda"); > > +/* Search and read element data size from the TPDM node in > + * the devicetree. Each input port of TPDA is connected to > + * a TPDM. Different TPDM supports different types of dataset, > + * and some may support more than one type of dataset. > + * Parameter "inport" is used to pass in the input port number > + * of TPDA, and it is set to 0 in the recursize call. > + * Parameter "parent" is used to pass in the original call. > + */ > +static int tpda_set_element_size(struct tpda_drvdata *drvdata, > + struct coresight_device *csdev, int inport, bool parent) > +{ > + static int nr_inport; > + int i; > + static bool tpdm_found; > + struct coresight_device *in_csdev; > + > + if (inport > (TPDA_MAX_INPORTS - 1)) > + return -EINVAL; > + > + if (parent) { > + nr_inport = inport; > + tpdm_found = false; > + } > + > + for (i = 0; i < csdev->pdata->nr_inconns; i++) { > + in_csdev = csdev->pdata->in_conns[i]->src_dev; > + if (!in_csdev) > + break; > + > + if (parent) > + if (csdev->pdata->in_conns[i]->dest_port != inport) > + continue; > + > + if (in_csdev->subtype.source_subtype We must match the in_csdev->type to be SOURCE && the subtype. > + == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) { > + of_property_read_u8(in_csdev->dev.parent->of_node, > + "qcom,dsb-element-size", &drvdata->dsb_esize[nr_inport]); > + if (!tpdm_found) > + tpdm_found = true; > + else > + dev_warn(drvdata->dev, > + "More than one TPDM is mapped to the TPDA input port %d.\n", > + nr_inport); When we know, we have found a source device, we don't need to recurse down and could simply 'continue' to the next one in the list and skip the call below. > + } > + tpda_set_element_size(drvdata, in_csdev, 0, false); > + } > + > + return 0; > +} > + > /* Settings pre enabling port control register */ > static void tpda_enable_pre_port(struct tpda_drvdata *drvdata) > { > @@ -32,26 +82,43 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata) > writel_relaxed(val, drvdata->base + TPDA_CR); > } > > -static void tpda_enable_port(struct tpda_drvdata *drvdata, int port) > +static int tpda_enable_port(struct tpda_drvdata *drvdata, int port) > { > u32 val; > > val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port)); > + /* > + * Configure aggregator port n DSB data set element size > + * Set the bit to 0 if the size is 32 > + * Set the bit to 1 if the size is 64 > + */ > + if (drvdata->dsb_esize[port] == 32) > + val &= ~TPDA_Pn_CR_DSBSIZE; > + else if (drvdata->dsb_esize[port] == 64) > + val |= TPDA_Pn_CR_DSBSIZE; > + else > + return -EINVAL; > + > /* Enable the port */ > val |= TPDA_Pn_CR_ENA; > writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port)); > + > + return 0; > } > > -static void __tpda_enable(struct tpda_drvdata *drvdata, int port) > +static int __tpda_enable(struct tpda_drvdata *drvdata, int port) > { > + int ret; > + > CS_UNLOCK(drvdata->base); > > if (!drvdata->csdev->enable) > tpda_enable_pre_port(drvdata); > > - tpda_enable_port(drvdata, port); > - > + ret = tpda_enable_port(drvdata, port); > CS_LOCK(drvdata->base); > + > + return ret; > } > > static int tpda_enable(struct coresight_device *csdev, > @@ -59,16 +126,23 @@ static int tpda_enable(struct coresight_device *csdev, > struct coresight_connection *out) > { > struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + int ret; > + > + ret = tpda_set_element_size(drvdata, csdev, in->dest_port, true); > + if (ret) > + return ret; > > spin_lock(&drvdata->spinlock); > - if (atomic_read(&in->dest_refcnt) == 0) > + if (atomic_read(&in->dest_refcnt) == 0) { > __tpda_enable(drvdata, in->dest_port); ret = ... ? > + if (!ret) { > + atomic_inc(&in->dest_refcnt); > + dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port); > + } > + } > > - atomic_inc(&in->dest_refcnt); This seems wrong, as we may fail to hold additional refcounts for the additional sessions ? > spin_unlock(&drvdata->spinlock); > - > - dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port); > - return 0; > + return ret; > } > > static void __tpda_disable(struct tpda_drvdata *drvdata, int port) > diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h > index 0399678..7332e9c 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.h > +++ b/drivers/hwtracing/coresight/coresight-tpda.h > @@ -10,6 +10,8 @@ > #define TPDA_Pn_CR(n) (0x004 + (n * 4)) > /* Aggregator port enable bit */ > #define TPDA_Pn_CR_ENA BIT(0) > +/* Aggregator port DSB data set element size bit */ > +#define TPDA_Pn_CR_DSBSIZE BIT(8) > > #define TPDA_MAX_INPORTS 32 > > @@ -23,6 +25,7 @@ > * @csdev: component vitals needed by the framework. > * @spinlock: lock for the drvdata value. > * @enable: enable status of the component. > + * @dsb_esize: DSB element size, it must be 32 or 64. minor nit: DSB element size for each inport, it must be 32 or 64 > */ > struct tpda_drvdata { > void __iomem *base; > @@ -30,6 +33,7 @@ struct tpda_drvdata { > struct coresight_device *csdev; > spinlock_t spinlock; > u8 atid; > + u8 dsb_esize[TPDA_MAX_INPORTS]; > }; > > #endif /* _CORESIGHT_CORESIGHT_TPDA_H */ > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index f4854af..ba1867f 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -205,7 +205,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > if (!desc.name) > return -ENOMEM; > desc.type = CORESIGHT_DEV_TYPE_SOURCE; > - desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS; > + desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM; > desc.ops = &tpdm_cs_ops; > desc.pdata = adev->dev.platform_data; > desc.dev = &adev->dev; Please could you split this change, i.e., introduction of SUBTYPE_SOURCE_TPDM and using this in TPDM driver, in a separate patch before this change. > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 225a5fa..6563896 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -60,6 +60,7 @@ enum coresight_dev_subtype_source { > CORESIGHT_DEV_SUBTYPE_SOURCE_PROC, > CORESIGHT_DEV_SUBTYPE_SOURCE_BUS, > CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE, > + CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM, > CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS, > }; >
On 23/05/2023 11:07, Suzuki K Poulose wrote: > On 27/04/2023 10:00, Tao Zhang wrote: >> Read the DSB element size from the device tree. Set the register >> bit that controls the DSB element size of the corresponding port. >> >> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >> --- >> drivers/hwtracing/coresight/coresight-core.c | 1 + >> drivers/hwtracing/coresight/coresight-tpda.c | 92 >> +++++++++++++++++++++++++--- >> drivers/hwtracing/coresight/coresight-tpda.h | 4 ++ >> drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- >> include/linux/coresight.h | 1 + >> 5 files changed, 90 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-core.c >> b/drivers/hwtracing/coresight/coresight-core.c >> index 2af416b..f1eacbb 100644 >> --- a/drivers/hwtracing/coresight/coresight-core.c >> +++ b/drivers/hwtracing/coresight/coresight-core.c >> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct >> coresight_device *csdev, >> if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC && >> subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE && >> + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM && >> subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) { >> dev_err(&csdev->dev, "wrong device subtype in %s\n", function); >> return -EINVAL; > > Please see the comment at the bottom. > >> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c >> b/drivers/hwtracing/coresight/coresight-tpda.c >> index 8d2b9d2..af9c72f 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpda.c >> +++ b/drivers/hwtracing/coresight/coresight-tpda.c >> @@ -21,6 +21,56 @@ >> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda"); >> +/* Search and read element data size from the TPDM node in >> + * the devicetree. Each input port of TPDA is connected to >> + * a TPDM. Different TPDM supports different types of dataset, >> + * and some may support more than one type of dataset. >> + * Parameter "inport" is used to pass in the input port number >> + * of TPDA, and it is set to 0 in the recursize call. >> + * Parameter "parent" is used to pass in the original call. >> + */ >> +static int tpda_set_element_size(struct tpda_drvdata *drvdata, >> + struct coresight_device *csdev, int inport, bool parent) The name parent is a bit confusing. It could imply parent device ? That is kind of inverse ? because, parent = true, indicates the parent device of tpda, which is not true. Could we simply say bool match_inport => When true, the dest_port of the connection from the csdev must match the inport ? And ... >> +{ >> + static int nr_inport; >> + int i; >> + static bool tpdm_found; >> + struct coresight_device *in_csdev; >> + >> + if (inport > (TPDA_MAX_INPORTS - 1)) >> + return -EINVAL; >> + >> + if (parent) { >> + nr_inport = inport; >> + tpdm_found = false; >> + } >> + >> + for (i = 0; i < csdev->pdata->nr_inconns; i++) { >> + in_csdev = csdev->pdata->in_conns[i]->src_dev; >> + if (!in_csdev) >> + break; >> + >> + if (parent) >> + if (csdev->pdata->in_conns[i]->dest_port != inport) >> + continue; The above can become : if (match_inport && csdev->pdata->in_conns[i]->dest_port != inport) continue; Suzuki
On 27/04/2023 10:00, Tao Zhang wrote: > TPDM device need a node to reset the configurations and status of > it. This change provides a node to reset the configurations and > disable the TPDM if it has been enabled. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 10 ++++++++ > drivers/hwtracing/coresight/coresight-tpdm.c | 27 ++++++++++++++++++++++ > 2 files changed, 37 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > index 4a58e64..686bdde 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > @@ -11,3 +11,13 @@ Description: > Accepts only one of the 2 values - 1 or 2. > 1 : Generate 64 bits data > 2 : Generate 32 bits data > + > +What: /sys/bus/coresight/devices/<tpdm-name>/reset > +Date: March 2023 > +KernelVersion 6.3 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> > +Description: > + (Write) Reset the dataset of the tpdm, and disable the tpdm. > + > + Accepts only one value - 1. > + 1 : Reset the dataset of the tpdm > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 6f8a8ab..2e64cfd 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -164,6 +164,32 @@ static int tpdm_datasets_setup(struct tpdm_drvdata *drvdata) > return 0; > } > > +static ssize_t reset_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + int ret = 0; > + unsigned long val; > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + ret = kstrtoul(buf, 10, &val); > + if (ret || val != 1) > + return -EINVAL; > + > + spin_lock(&drvdata->spinlock); > + tpdm_reset_datasets(drvdata); > + > + spin_unlock(&drvdata->spinlock); > + > + /* Disable tpdm if enabled */ > + if (drvdata->enable) > + coresight_disable_source(drvdata->csdev, NULL); I am not really keen on doing this behind the back. What about the path of components ? We could simply reject the request when the TPDA is enabled and let the user alway follow : 1) Disable the TPDM manually via sysfs 2) Reset the TPDM. So, please remove the disable step here. Suzuki > + > + return size; > +} > +static DEVICE_ATTR_WO(reset); > + > /* > * value 1: 64 bits test data > * value 2: 32 bits test data > @@ -204,6 +230,7 @@ static ssize_t integration_test_store(struct device *dev, > static DEVICE_ATTR_WO(integration_test); > > static struct attribute *tpdm_attrs[] = { > + &dev_attr_reset.attr, > &dev_attr_integration_test.attr, > NULL, > };
On 5/23/2023 10:48 PM, Suzuki K Poulose wrote: > On 23/05/2023 11:07, Suzuki K Poulose wrote: >> On 27/04/2023 10:00, Tao Zhang wrote: >>> Read the DSB element size from the device tree. Set the register >>> bit that controls the DSB element size of the corresponding port. >>> >>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>> --- >>> drivers/hwtracing/coresight/coresight-core.c | 1 + >>> drivers/hwtracing/coresight/coresight-tpda.c | 92 >>> +++++++++++++++++++++++++--- >>> drivers/hwtracing/coresight/coresight-tpda.h | 4 ++ >>> drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- >>> include/linux/coresight.h | 1 + >>> 5 files changed, 90 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-core.c >>> b/drivers/hwtracing/coresight/coresight-core.c >>> index 2af416b..f1eacbb 100644 >>> --- a/drivers/hwtracing/coresight/coresight-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-core.c >>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct >>> coresight_device *csdev, >>> if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC && >>> subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE && >>> + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM && >>> subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) { >>> dev_err(&csdev->dev, "wrong device subtype in %s\n", >>> function); >>> return -EINVAL; >> >> Please see the comment at the bottom. >> >>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c >>> b/drivers/hwtracing/coresight/coresight-tpda.c >>> index 8d2b9d2..af9c72f 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tpda.c >>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c >>> @@ -21,6 +21,56 @@ >>> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda"); >>> +/* Search and read element data size from the TPDM node in >>> + * the devicetree. Each input port of TPDA is connected to >>> + * a TPDM. Different TPDM supports different types of dataset, >>> + * and some may support more than one type of dataset. >>> + * Parameter "inport" is used to pass in the input port number >>> + * of TPDA, and it is set to 0 in the recursize call. >>> + * Parameter "parent" is used to pass in the original call. >>> + */ >>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata, >>> + struct coresight_device *csdev, int inport, bool >>> parent) > > The name parent is a bit confusing. It could imply parent device ? That > is kind of inverse ? because, parent = true, indicates the parent device > of tpda, which is not true. Could we simply say > > bool match_inport => When true, the dest_port of the connection from the > csdev must match the inport ? And ... > Sure, I will update this in the next patch series. >>> +{ >>> + static int nr_inport; >>> + int i; >>> + static bool tpdm_found; >>> + struct coresight_device *in_csdev; >>> + >>> + if (inport > (TPDA_MAX_INPORTS - 1)) >>> + return -EINVAL; >>> + >>> + if (parent) { >>> + nr_inport = inport; >>> + tpdm_found = false; >>> + } >>> + >>> + for (i = 0; i < csdev->pdata->nr_inconns; i++) { >>> + in_csdev = csdev->pdata->in_conns[i]->src_dev; >>> + if (!in_csdev) >>> + break; >>> + >>> + if (parent) >>> + if (csdev->pdata->in_conns[i]->dest_port != inport) >>> + continue; > > The above can become : > > if (match_inport && > csdev->pdata->in_conns[i]->dest_port != inport) > continue; Sure, I will update this in the next patch series. Best, Tao > > Suzuki >
On 5/23/2023 10:53 PM, Suzuki K Poulose wrote: > On 27/04/2023 10:00, Tao Zhang wrote: >> TPDM device need a node to reset the configurations and status of >> it. This change provides a node to reset the configurations and >> disable the TPDM if it has been enabled. >> >> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >> --- >> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 10 ++++++++ >> drivers/hwtracing/coresight/coresight-tpdm.c | 27 >> ++++++++++++++++++++++ >> 2 files changed, 37 insertions(+) >> >> diff --git >> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> index 4a58e64..686bdde 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> @@ -11,3 +11,13 @@ Description: >> Accepts only one of the 2 values - 1 or 2. >> 1 : Generate 64 bits data >> 2 : Generate 32 bits data >> + >> +What: /sys/bus/coresight/devices/<tpdm-name>/reset >> +Date: March 2023 >> +KernelVersion 6.3 >> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang >> (QUIC) <quic_taozha@quicinc.com> >> +Description: >> + (Write) Reset the dataset of the tpdm, and disable the tpdm. >> + >> + Accepts only one value - 1. >> + 1 : Reset the dataset of the tpdm >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >> b/drivers/hwtracing/coresight/coresight-tpdm.c >> index 6f8a8ab..2e64cfd 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >> @@ -164,6 +164,32 @@ static int tpdm_datasets_setup(struct >> tpdm_drvdata *drvdata) >> return 0; >> } >> +static ssize_t reset_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, >> + size_t size) >> +{ >> + int ret = 0; >> + unsigned long val; >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + >> + ret = kstrtoul(buf, 10, &val); >> + if (ret || val != 1) >> + return -EINVAL; >> + >> + spin_lock(&drvdata->spinlock); >> + tpdm_reset_datasets(drvdata); >> + >> + spin_unlock(&drvdata->spinlock); >> + >> + /* Disable tpdm if enabled */ >> + if (drvdata->enable) >> + coresight_disable_source(drvdata->csdev, NULL); > > I am not really keen on doing this behind the back. What about the > path of components ? We could simply reject the request when the TPDA > is enabled and let the user alway follow : > 1) Disable the TPDM manually via sysfs > 2) Reset the TPDM. > > So, please remove the disable step here. I will update this in the next patch series. Best, Tao > > Suzuki > > >> + >> + return size; >> +} >> +static DEVICE_ATTR_WO(reset); >> + >> /* >> * value 1: 64 bits test data >> * value 2: 32 bits test data >> @@ -204,6 +230,7 @@ static ssize_t integration_test_store(struct >> device *dev, >> static DEVICE_ATTR_WO(integration_test); >> static struct attribute *tpdm_attrs[] = { >> + &dev_attr_reset.attr, >> &dev_attr_integration_test.attr, >> NULL, >> }; >
On 25/05/2023 08:16, Tao Zhang wrote: > > On 5/23/2023 6:07 PM, Suzuki K Poulose wrote: >> On 27/04/2023 10:00, Tao Zhang wrote: >>> Read the DSB element size from the device tree. Set the register >>> bit that controls the DSB element size of the corresponding port. >>> >>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>> --- >>> drivers/hwtracing/coresight/coresight-core.c | 1 + >>> drivers/hwtracing/coresight/coresight-tpda.c | 92 >>> +++++++++++++++++++++++++--- >>> drivers/hwtracing/coresight/coresight-tpda.h | 4 ++ >>> drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- >>> include/linux/coresight.h | 1 + >>> 5 files changed, 90 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-core.c >>> b/drivers/hwtracing/coresight/coresight-core.c >>> index 2af416b..f1eacbb 100644 >>> --- a/drivers/hwtracing/coresight/coresight-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-core.c >>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct >>> coresight_device *csdev, >>> if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC && >>> subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE && >>> + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM && >>> subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) { >>> dev_err(&csdev->dev, "wrong device subtype in %s\n", >>> function); >>> return -EINVAL; >> >> Please see the comment at the bottom. >> >>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c >>> b/drivers/hwtracing/coresight/coresight-tpda.c >>> index 8d2b9d2..af9c72f 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tpda.c >>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c >>> @@ -21,6 +21,56 @@ >>> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda"); >>> +/* Search and read element data size from the TPDM node in >>> + * the devicetree. Each input port of TPDA is connected to >>> + * a TPDM. Different TPDM supports different types of dataset, >>> + * and some may support more than one type of dataset. >>> + * Parameter "inport" is used to pass in the input port number >>> + * of TPDA, and it is set to 0 in the recursize call. >>> + * Parameter "parent" is used to pass in the original call. >>> + */ >>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata, >>> + struct coresight_device *csdev, int inport, bool parent) >>> +{ >>> + static int nr_inport; >>> + int i; >>> + static bool tpdm_found; >>> + struct coresight_device *in_csdev; >>> + >>> + if (inport > (TPDA_MAX_INPORTS - 1)) >>> + return -EINVAL; >>> + >>> + if (parent) { >>> + nr_inport = inport; >>> + tpdm_found = false; >>> + } >>> + >>> + for (i = 0; i < csdev->pdata->nr_inconns; i++) { >>> + in_csdev = csdev->pdata->in_conns[i]->src_dev; >>> + if (!in_csdev) >>> + break; >>> + >>> + if (parent) >>> + if (csdev->pdata->in_conns[i]->dest_port != inport) >>> + continue; >>> + >>> + if (in_csdev->subtype.source_subtype >> >> We must match the in_csdev->type to be SOURCE && the subtype. > Sure, I will update it in the next patch series. >> >>> + == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) { >>> + of_property_read_u8(in_csdev->dev.parent->of_node, >>> + "qcom,dsb-element-size", >>> &drvdata->dsb_esize[nr_inport]); >>> + if (!tpdm_found) >>> + tpdm_found = true; >>> + else >>> + dev_warn(drvdata->dev, >>> + "More than one TPDM is mapped to the TPDA input >>> port %d.\n", >>> + nr_inport); >> >> When we know, we have found a source device, we don't need to recurse >> down and could simply 'continue' to the next one in the list and skip >> the call below. > > Actually, one input port on TPDA only can connect to one TPDM. In the > current design, it will > > find out all the TPDMs on one input port and warn the users all the > TPDMs it found. If we > > replace 'recurse down' as 'continue' here, it may not find some TPDMs > that might be connected > > incorrectly. What do you mean ? When you enter the if () above, the in_csdev is a source and it is TPDM. There must be no input connections TPDM, i.e. in_csdev, so no need to go further up the connection chain looking at the in_csdev. The loop will continue to analyse this device (where we found one TPDM already) and detect any further duplicate TPDMs connected. Suzuki
On 5/25/2023 5:08 PM, Suzuki K Poulose wrote: > On 25/05/2023 08:16, Tao Zhang wrote: >> >> On 5/23/2023 6:07 PM, Suzuki K Poulose wrote: >>> On 27/04/2023 10:00, Tao Zhang wrote: >>>> Read the DSB element size from the device tree. Set the register >>>> bit that controls the DSB element size of the corresponding port. >>>> >>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>>> --- >>>> drivers/hwtracing/coresight/coresight-core.c | 1 + >>>> drivers/hwtracing/coresight/coresight-tpda.c | 92 >>>> +++++++++++++++++++++++++--- >>>> drivers/hwtracing/coresight/coresight-tpda.h | 4 ++ >>>> drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- >>>> include/linux/coresight.h | 1 + >>>> 5 files changed, 90 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c >>>> b/drivers/hwtracing/coresight/coresight-core.c >>>> index 2af416b..f1eacbb 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-core.c >>>> +++ b/drivers/hwtracing/coresight/coresight-core.c >>>> @@ -1092,6 +1092,7 @@ static int coresight_validate_source(struct >>>> coresight_device *csdev, >>>> if (subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_PROC && >>>> subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE && >>>> + subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM && >>>> subtype != CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS) { >>>> dev_err(&csdev->dev, "wrong device subtype in %s\n", >>>> function); >>>> return -EINVAL; >>> >>> Please see the comment at the bottom. >>> >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c >>>> b/drivers/hwtracing/coresight/coresight-tpda.c >>>> index 8d2b9d2..af9c72f 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c >>>> @@ -21,6 +21,56 @@ >>>> DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda"); >>>> +/* Search and read element data size from the TPDM node in >>>> + * the devicetree. Each input port of TPDA is connected to >>>> + * a TPDM. Different TPDM supports different types of dataset, >>>> + * and some may support more than one type of dataset. >>>> + * Parameter "inport" is used to pass in the input port number >>>> + * of TPDA, and it is set to 0 in the recursize call. >>>> + * Parameter "parent" is used to pass in the original call. >>>> + */ >>>> +static int tpda_set_element_size(struct tpda_drvdata *drvdata, >>>> + struct coresight_device *csdev, int inport, bool >>>> parent) >>>> +{ >>>> + static int nr_inport; >>>> + int i; >>>> + static bool tpdm_found; >>>> + struct coresight_device *in_csdev; >>>> + >>>> + if (inport > (TPDA_MAX_INPORTS - 1)) >>>> + return -EINVAL; >>>> + >>>> + if (parent) { >>>> + nr_inport = inport; >>>> + tpdm_found = false; >>>> + } >>>> + >>>> + for (i = 0; i < csdev->pdata->nr_inconns; i++) { >>>> + in_csdev = csdev->pdata->in_conns[i]->src_dev; >>>> + if (!in_csdev) >>>> + break; >>>> + >>>> + if (parent) >>>> + if (csdev->pdata->in_conns[i]->dest_port != inport) >>>> + continue; >>>> + >>>> + if (in_csdev->subtype.source_subtype >>> >>> We must match the in_csdev->type to be SOURCE && the subtype. >> Sure, I will update it in the next patch series. >>> >>>> + == CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM) { >>>> + of_property_read_u8(in_csdev->dev.parent->of_node, >>>> + "qcom,dsb-element-size", >>>> &drvdata->dsb_esize[nr_inport]); >>>> + if (!tpdm_found) >>>> + tpdm_found = true; >>>> + else >>>> + dev_warn(drvdata->dev, >>>> + "More than one TPDM is mapped to the TPDA >>>> input port %d.\n", >>>> + nr_inport); >>> >>> When we know, we have found a source device, we don't need to recurse >>> down and could simply 'continue' to the next one in the list and skip >>> the call below. >> >> Actually, one input port on TPDA only can connect to one TPDM. In the >> current design, it will >> >> find out all the TPDMs on one input port and warn the users all the >> TPDMs it found. If we >> >> replace 'recurse down' as 'continue' here, it may not find some TPDMs >> that might be connected >> >> incorrectly. > > > What do you mean ? When you enter the if () above, the in_csdev is a > source and it is TPDM. There must be no input connections TPDM, i.e. > in_csdev, so no need to go further up the connection chain looking at > the in_csdev. The loop will continue to analyse this device (where we > found one TPDM already) and detect any further duplicate TPDMs > connected. Got it. You're right. I will update this in the next patch series as well. > > Suzuki > >
On 5/23/2023 3:50 PM, Tao Zhang wrote: > On 4/28/2023 12:53 AM, Suzuki K Poulose wrote: >> On 27/04/2023 10:00, Tao Zhang wrote: >>> Introduction of TPDM DSB subunit >>> DSB subunit is responsible for creating a dataset element, and is also >>> optionally responsible for packing it to fit multiple elements on a >>> single ATB transfer if possible in the configuration. The TPDM Core >>> Datapath requests timestamps be stored by the TPDA and then delivering >>> ATB sized data (depending on ATB width and element size, this could >>> be smaller or larger than a dataset element) to the ATB Mast FSM. >>> >>> The DSB subunit must be configured prior to enablement. This series >>> adds support for TPDM to configure the configure DSB subunit. >>> >>> Once this series patches are applied properly, the new tpdm nodes for >>> should be observed at the tpdm path /sys/bus/coresight/devices/tpdm* >>> which supports DSB subunit. >>> e.g. >>> /sys/devices/platform/soc@0/69d0000.tpdm/tpdm0#ls -l | grep dsb >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_edge_ctrl >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_edge_ctrl_mask >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_mode >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_patt_mask >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_patt_ts >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_patt_type >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_patt_val >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_trig_patt_mask >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_trig_patt_val >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_trig_ts >>> -rw-r--r-- 1 root root 4096 Jan 1 00:01 dsb_trig_type >>> >>> We can use the commands are similar to the below to configure the >>> TPDMs which support DSB subunit. Enable coresight sink first. >>> echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink >>> echo 1 > /sys/bus/coresight/devices/tpdm0/reset >>> echo 0x3 0x3 0x1 > /sys/bus/coresight/devices/tpdm0/dsb_edge_ctrl_mask >>> echo 0x6d 0x6d 0 > /sys/bus/coresight/devices/tpdm0/dsb_edge_ctrl >>> echo 1 > /sys/bus/coresight/devices/tpdm0/dsb_patt_ts >>> echo 1 > /sys/bus/coresight/devices/tpdm0/dsb_patt_type >>> echo 0 > /sys/bus/coresight/devices/tpdm0/dsb_trig_ts >>> echo 0 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/dsb_patt_mask >>> echo 0 0xFFFFFFFF > /sys/bus/coresight/devices/tpdm0/dsb_trig_patt_val >>> >>> This patch series depends on patch series "[PATCH v2 0/9] coresight: >>> Fix CTI module refcount leak by making it a helper device" >>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230425143542.2305069-14-james.clark@arm.com/ >>> >> >> There is v6 available for the above and there may be changes in the data >> structures. But the series is stable now, and may be you could cordinate >> with James and repost the series at rc1 ? > > This patch series has depended on James's v6 patch series. It's a > description mistake. > > The link I posted is James's v6 patch series. > > Would you mind continue to review this patch series first? > > > Tao > Hi Suzuki, Do you have more review comments on the rest of the patches(#5-#11) in this series? Or do you prefer me to update patches(#1-#4) and resubmit first? Best, Tao >> >> Suzuki >> >> _______________________________________________ >> CoreSight mailing list -- coresight@lists.linaro.org >> To unsubscribe send an email to coresight-leave@lists.linaro.org
On 01/06/2023 09:17, Tao Zhang wrote: > > On 5/23/2023 3:50 PM, Tao Zhang wrote: >> On 4/28/2023 12:53 AM, Suzuki K Poulose wrote: >>> On 27/04/2023 10:00, Tao Zhang wrote: >>>> Introduction of TPDM DSB subunit >>>> DSB subunit is responsible for creating a dataset element, and is also >>>> optionally responsible for packing it to fit multiple elements on a >>>> single ATB transfer if possible in the configuration. The TPDM Core >>>> Datapath requests timestamps be stored by the TPDA and then delivering >>>> ATB sized data (depending on ATB width and element size, this could >>>> be smaller or larger than a dataset element) to the ATB Mast FSM. >>>> >>>> The DSB subunit must be configured prior to enablement. This series >>>> adds support for TPDM to configure the configure DSB subunit. ... >>> There is v6 available for the above and there may be changes in the data >>> structures. But the series is stable now, and may be you could cordinate >>> with James and repost the series at rc1 ? >> >> This patch series has depended on James's v6 patch series. It's a >> description mistake. >> >> The link I posted is James's v6 patch series. >> >> Would you mind continue to review this patch series first? >> >> >> Tao >> > Hi Suzuki, > > > Do you have more review comments on the rest of the patches(#5-#11) in > this series? > > Or do you prefer me to update patches(#1-#4) and resubmit first? Apologoies for the delay. I will try to complete this series this week. Thanks Suzuki
On 27/04/2023 10:00, Tao Zhang wrote: > Add node to set and show programming mode for TPDM DSB subunit. > Once the DSB programming mode is set, it will be written to the > register DSB_CR. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 15 ++++++ > drivers/hwtracing/coresight/coresight-tpdm.c | 62 ++++++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tpdm.h | 16 ++++++ > 3 files changed, 93 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > index 77e67f2..348e167 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > @@ -45,3 +45,18 @@ Description: > Accepts only one of the 2 values - 0 or 1. > 0 : Set the DSB trigger type to false > 1 : Set the DSB trigger type to true > + > +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_mode > +Date: March 2023 > +KernelVersion 6.3 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> > +Description: > + (Write) Set the mode of DSB tpdm. Read the mode of DSB > + tpdm. > + > + Accepts the value needs to be greater than 0. What data > + bits do is listed below. > + Bit[0:1] : Test mode control bit for choosing the inputs. > + 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. > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 14f4352..1bacaa5 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -4,6 +4,7 @@ > */ > > #include <linux/amba/bus.h> > +#include <linux/bitfield.h> > #include <linux/bitmap.h> > #include <linux/coresight.h> > #include <linux/coresight-pmu.h> > @@ -43,6 +44,32 @@ static void tpdm_reset_datasets(struct tpdm_drvdata *drvdata) > } > } > > +static void set_dsb_test_mode(struct tpdm_drvdata *drvdata, u32 *val) > +{ > + u32 mode; > + > + mode = TPDM_DSB_MODE_TEST(drvdata->dsb->mode); > + *val &= ~TPDM_DSB_TEST_MODE; > + *val |= FIELD_PREP(TPDM_DSB_TEST_MODE, mode); > +} > + > +static void set_dsb_hpsel_mode(struct tpdm_drvdata *drvdata, u32 *val) > +{ > + u32 mode; > + > + mode = TPDM_DSB_MODE_HPBYTESEL(drvdata->dsb->mode); > + *val &= ~TPDM_DSB_HPSEL; > + *val |= FIELD_PREP(TPDM_DSB_HPSEL, mode); > +} > + > +static void set_dsb_perf_mode(struct tpdm_drvdata *drvdata, u32 *val) > +{ > + if (drvdata->dsb->mode & TPDM_DSB_MODE_PERF) > + *val |= TPDM_DSB_CR_MODE; > + else > + *val &= ~TPDM_DSB_CR_MODE; > +} > + > static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val) > { > if (drvdata->dsb->trig_type) > @@ -64,6 +91,12 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) > writel_relaxed(val, drvdata->base + TPDM_DSB_TIER); > > val = readl_relaxed(drvdata->base + TPDM_DSB_CR); > + /* Set the test accurate mode */ > + set_dsb_test_mode(drvdata, &val); > + /* Set the byte lane for high-performance mode */ > + set_dsb_hpsel_mode(drvdata, &val); > + /* Set the performance mode */ > + set_dsb_perf_mode(drvdata, &val); > /* Set trigger type */ > set_trigger_type(drvdata, &val); > /* Set the enable bit of DSB control register to 1 */ > @@ -252,6 +285,34 @@ static struct attribute_group tpdm_attr_grp = { > .attrs = tpdm_attrs, > }; > > +static ssize_t dsb_mode_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + return sysfs_emit(buf, "%lx\n", > + (unsigned long)drvdata->dsb->mode); > +} > + > +static ssize_t dsb_mode_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; > + > + if ((kstrtoul(buf, 0, &val)) || val < 0) > + return -EINVAL; > + > + spin_lock(&drvdata->spinlock); > + drvdata->dsb->mode = val & TPDM_MODE_ALL; > + spin_unlock(&drvdata->spinlock); > + return size; > +} > +static DEVICE_ATTR_RW(dsb_mode); > + > static ssize_t dsb_trig_type_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -323,6 +384,7 @@ 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_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 68f33bd..79df07e 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.h > +++ b/drivers/hwtracing/coresight/coresight-tpdm.h > @@ -15,11 +15,25 @@ > > /* Enable bit for DSB subunit */ > #define TPDM_DSB_CR_ENA BIT(0) > +/* Enable bit for DSB subunit perfmance mode */ > +#define TPDM_DSB_CR_MODE BIT(1) > /* Enable bit for DSB subunit trigger type */ > #define TPDM_DSB_CR_TRIG_TYPE BIT(12) > + > /* Enable bit for DSB subunit trigger timestamp */ > #define TPDM_DSB_TIER_XTRIG_TSENAB BIT(1) > > +/* DSB programming modes */ > +/* Test mode control bit*/ > +#define TPDM_DSB_MODE_TEST(val) (val & GENMASK(1, 0)) > +/* Perforceman mode */ minor nit: typo ^^ > +#define TPDM_DSB_MODE_PERF BIT(3) > +/* High performance mode */ > +#define TPDM_DSB_MODE_HPBYTESEL(val) (val & GENMASK(8, 4)) > +#define TPDM_MODE_ALL (0xFFFFFFF) GENMASK(27, 0) ? Also, why do we cover bits 27-0 ? Suzuki
On 27/04/2023 10:00, Tao Zhang wrote: > Add nodes to configure trigger pattern and trigger pattern mask. > Each DSB subunit TPDM has maximum of n(n<7) XPR registers to > configure trigger pattern match output. Eight 32 bit registers > providing DSB interface trigger output pattern match comparison. > And each DSB subunit TPDM has maximum of m(m<7) XPMR registers to > configure trigger pattern mask match output. Eight 32 bit > registers providing DSB interface trigger output pattern match > mask. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 30 ++++++++ > drivers/hwtracing/coresight/coresight-tpdm.c | 85 ++++++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tpdm.h | 8 ++ > 3 files changed, 123 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > index a57f000..c04c735 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > @@ -92,3 +92,33 @@ Description: > <integer1> : Start EDCMR register number > <integer2> : End EDCMR register number > <integer3> : The value need to be written > + > +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_trig_patt_val > +Date: March 2023 > +KernelVersion 6.3 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> > +Description: > + (Write) Set the trigger pattern value of DSB tpdm. > + Read the trigger pattern value of DSB tpdm. > + > + Expected format is the following: > + <integer1> <integer2> > + > + Where: > + <integer1> : Index number of XPR register, the range is 0 to 7 > + <integer2> : The value need to be written I assume the values written to the registers are not special and doesn't have meaning and thus need not be documented ? > + > +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_trig_patt_mask > +Date: March 2023 > +KernelVersion 6.3 Same as the previous one, 6.5 please > +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> > +Description: > + (Write) Set the trigger pattern mask of DSB tpdm. > + Read the trigger pattern mask of DSB tpdm. > + > + Expected format is the following: > + <integer1> <integer2> > + > + Where: > + <integer1> : Index number of XPMR register, the range is 0 to 7 > + <integer2> : The value need to be written > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index a40e458..9387bdf 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -89,6 +89,13 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) > writel_relaxed(drvdata->dsb->edge_ctrl_mask[i], > drvdata->base + TPDM_DSB_EDCMR(i)); > > + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { Same as the previous, can we safely assume that write to these registers won't trigger an Error if not impelemented ? > + writel_relaxed(drvdata->dsb->trig_patt_val[i], > + drvdata->base + TPDM_DSB_XPR(i)); > + writel_relaxed(drvdata->dsb->trig_patt_mask[i], > + drvdata->base + TPDM_DSB_XPMR(i)); > + } > + > val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); > /* Set trigger timestamp */ > if (drvdata->dsb->trig_ts) > @@ -444,6 +451,82 @@ static ssize_t dsb_edge_ctrl_mask_store(struct device *dev, > } > static DEVICE_ATTR_RW(dsb_edge_ctrl_mask); > > +static ssize_t dsb_trig_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; > + > + spin_lock(&drvdata->spinlock); > + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { > + size += sysfs_emit_at(buf, size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->dsb->trig_patt_val[i]); Please detect the return of 0 and break. Same below. > + } > + spin_unlock(&drvdata->spinlock); > + return size; > +} > + > +static ssize_t dsb_trig_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 (index >= TPDM_DSB_MAX_PATT) > + return -EPERM; > + > + spin_lock(&drvdata->spinlock); > + drvdata->dsb->trig_patt_val[index] = val; > + spin_unlock(&drvdata->spinlock); > + return size; > +} > +static DEVICE_ATTR_RW(dsb_trig_patt_val); > + > +static ssize_t dsb_trig_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; > + > + spin_lock(&drvdata->spinlock); > + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { > + size += sysfs_emit_at(buf, size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->dsb->trig_patt_mask[i]); > + } > + spin_unlock(&drvdata->spinlock); > + return size; Suzuki
On 6/1/2023 5:23 PM, Suzuki K Poulose wrote: > On 27/04/2023 10:00, Tao Zhang wrote: >> Add node to set and show programming mode for TPDM DSB subunit. >> Once the DSB programming mode is set, it will be written to the >> register DSB_CR. >> >> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >> --- >> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 15 ++++++ >> drivers/hwtracing/coresight/coresight-tpdm.c | 62 >> ++++++++++++++++++++++ >> drivers/hwtracing/coresight/coresight-tpdm.h | 16 ++++++ >> 3 files changed, 93 insertions(+) >> >> diff --git >> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> index 77e67f2..348e167 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> @@ -45,3 +45,18 @@ Description: >> Accepts only one of the 2 values - 0 or 1. >> 0 : Set the DSB trigger type to false >> 1 : Set the DSB trigger type to true >> + >> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_mode >> +Date: March 2023 >> +KernelVersion 6.3 >> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang >> (QUIC) <quic_taozha@quicinc.com> >> +Description: >> + (Write) Set the mode of DSB tpdm. Read the mode of DSB >> + tpdm. >> + >> + Accepts the value needs to be greater than 0. What data >> + bits do is listed below. >> + Bit[0:1] : Test mode control bit for choosing the inputs. >> + 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. >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >> b/drivers/hwtracing/coresight/coresight-tpdm.c >> index 14f4352..1bacaa5 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >> @@ -4,6 +4,7 @@ >> */ >> #include <linux/amba/bus.h> >> +#include <linux/bitfield.h> >> #include <linux/bitmap.h> >> #include <linux/coresight.h> >> #include <linux/coresight-pmu.h> >> @@ -43,6 +44,32 @@ static void tpdm_reset_datasets(struct >> tpdm_drvdata *drvdata) >> } >> } >> +static void set_dsb_test_mode(struct tpdm_drvdata *drvdata, u32 *val) >> +{ >> + u32 mode; >> + >> + mode = TPDM_DSB_MODE_TEST(drvdata->dsb->mode); >> + *val &= ~TPDM_DSB_TEST_MODE; >> + *val |= FIELD_PREP(TPDM_DSB_TEST_MODE, mode); >> +} >> + >> +static void set_dsb_hpsel_mode(struct tpdm_drvdata *drvdata, u32 *val) >> +{ >> + u32 mode; >> + >> + mode = TPDM_DSB_MODE_HPBYTESEL(drvdata->dsb->mode); >> + *val &= ~TPDM_DSB_HPSEL; >> + *val |= FIELD_PREP(TPDM_DSB_HPSEL, mode); >> +} >> + >> +static void set_dsb_perf_mode(struct tpdm_drvdata *drvdata, u32 *val) >> +{ >> + if (drvdata->dsb->mode & TPDM_DSB_MODE_PERF) >> + *val |= TPDM_DSB_CR_MODE; >> + else >> + *val &= ~TPDM_DSB_CR_MODE; >> +} >> + >> static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val) >> { >> if (drvdata->dsb->trig_type) >> @@ -64,6 +91,12 @@ static void tpdm_enable_dsb(struct tpdm_drvdata >> *drvdata) >> writel_relaxed(val, drvdata->base + TPDM_DSB_TIER); >> val = readl_relaxed(drvdata->base + TPDM_DSB_CR); >> + /* Set the test accurate mode */ >> + set_dsb_test_mode(drvdata, &val); >> + /* Set the byte lane for high-performance mode */ >> + set_dsb_hpsel_mode(drvdata, &val); >> + /* Set the performance mode */ >> + set_dsb_perf_mode(drvdata, &val); >> /* Set trigger type */ >> set_trigger_type(drvdata, &val); >> /* Set the enable bit of DSB control register to 1 */ >> @@ -252,6 +285,34 @@ static struct attribute_group tpdm_attr_grp = { >> .attrs = tpdm_attrs, >> }; >> +static ssize_t dsb_mode_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + >> + return sysfs_emit(buf, "%lx\n", >> + (unsigned long)drvdata->dsb->mode); >> +} >> + >> +static ssize_t dsb_mode_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; >> + >> + if ((kstrtoul(buf, 0, &val)) || val < 0) >> + return -EINVAL; >> + >> + spin_lock(&drvdata->spinlock); >> + drvdata->dsb->mode = val & TPDM_MODE_ALL; >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> +static DEVICE_ATTR_RW(dsb_mode); >> + >> static ssize_t dsb_trig_type_show(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> @@ -323,6 +384,7 @@ 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_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 68f33bd..79df07e 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >> @@ -15,11 +15,25 @@ >> /* Enable bit for DSB subunit */ >> #define TPDM_DSB_CR_ENA BIT(0) >> +/* Enable bit for DSB subunit perfmance mode */ >> +#define TPDM_DSB_CR_MODE BIT(1) >> /* Enable bit for DSB subunit trigger type */ >> #define TPDM_DSB_CR_TRIG_TYPE BIT(12) >> + >> /* Enable bit for DSB subunit trigger timestamp */ >> #define TPDM_DSB_TIER_XTRIG_TSENAB BIT(1) >> +/* DSB programming modes */ >> +/* Test mode control bit*/ >> +#define TPDM_DSB_MODE_TEST(val) (val & GENMASK(1, 0)) >> +/* Perforceman mode */ > > minor nit: typo ^^ I will update this in the next patch series. > >> +#define TPDM_DSB_MODE_PERF BIT(3) >> +/* High performance mode */ >> +#define TPDM_DSB_MODE_HPBYTESEL(val) (val & GENMASK(8, 4)) >> +#define TPDM_MODE_ALL (0xFFFFFFF) > > GENMASK(27, 0) ? > > Also, why do we cover bits 27-0 ? The TPDM mode is only represented by [0:8]bits. Can I replace it with "#define TPDM_DSB_MODE(val) (VAL & GENMASK(8, 0))"? Best, Tao > > Suzuki
On 6/2/2023 4:25 PM, Suzuki K Poulose wrote: > On 02/06/2023 03:58, Tao Zhang wrote: >> >> On 6/1/2023 5:23 PM, Suzuki K Poulose wrote: >>> On 27/04/2023 10:00, Tao Zhang wrote: >>>> Add node to set and show programming mode for TPDM DSB subunit. >>>> Once the DSB programming mode is set, it will be written to the >>>> register DSB_CR. >>>> >>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >>>> --- >>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 15 ++++++ >>>> drivers/hwtracing/coresight/coresight-tpdm.c | 62 >>>> ++++++++++++++++++++++ >>>> drivers/hwtracing/coresight/coresight-tpdm.h | 16 ++++++ >>>> 3 files changed, 93 insertions(+) >>>> >>>> diff --git >>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> index 77e67f2..348e167 100644 >>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >>>> @@ -45,3 +45,18 @@ Description: >>>> Accepts only one of the 2 values - 0 or 1. >>>> 0 : Set the DSB trigger type to false >>>> 1 : Set the DSB trigger type to true >>>> + >>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_mode >>>> +Date: March 2023 >>>> +KernelVersion 6.3 >>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao >>>> Zhang (QUIC) <quic_taozha@quicinc.com> >>>> +Description: >>>> + (Write) Set the mode of DSB tpdm. Read the mode of DSB >>>> + tpdm. >>>> + >>>> + Accepts the value needs to be greater than 0. What data >>>> + bits do is listed below. >>>> + Bit[0:1] : Test mode control bit for choosing the inputs. >>>> + 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. >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >>>> b/drivers/hwtracing/coresight/coresight-tpdm.c >>>> index 14f4352..1bacaa5 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>> @@ -4,6 +4,7 @@ >>>> */ >>>> #include <linux/amba/bus.h> >>>> +#include <linux/bitfield.h> >>>> #include <linux/bitmap.h> >>>> #include <linux/coresight.h> >>>> #include <linux/coresight-pmu.h> >>>> @@ -43,6 +44,32 @@ static void tpdm_reset_datasets(struct >>>> tpdm_drvdata *drvdata) >>>> } >>>> } >>>> +static void set_dsb_test_mode(struct tpdm_drvdata *drvdata, u32 >>>> *val) >>>> +{ >>>> + u32 mode; >>>> + >>>> + mode = TPDM_DSB_MODE_TEST(drvdata->dsb->mode); >>>> + *val &= ~TPDM_DSB_TEST_MODE; >>>> + *val |= FIELD_PREP(TPDM_DSB_TEST_MODE, mode); >>>> +} >>>> + >>>> +static void set_dsb_hpsel_mode(struct tpdm_drvdata *drvdata, u32 >>>> *val) >>>> +{ >>>> + u32 mode; >>>> + >>>> + mode = TPDM_DSB_MODE_HPBYTESEL(drvdata->dsb->mode); >>>> + *val &= ~TPDM_DSB_HPSEL; >>>> + *val |= FIELD_PREP(TPDM_DSB_HPSEL, mode); >>>> +} >>>> + >>>> +static void set_dsb_perf_mode(struct tpdm_drvdata *drvdata, u32 *val) >>>> +{ >>>> + if (drvdata->dsb->mode & TPDM_DSB_MODE_PERF) >>>> + *val |= TPDM_DSB_CR_MODE; >>>> + else >>>> + *val &= ~TPDM_DSB_CR_MODE; >>>> +} >>>> + >>>> static void set_trigger_type(struct tpdm_drvdata *drvdata, u32 *val) >>>> { >>>> if (drvdata->dsb->trig_type) >>>> @@ -64,6 +91,12 @@ static void tpdm_enable_dsb(struct tpdm_drvdata >>>> *drvdata) >>>> writel_relaxed(val, drvdata->base + TPDM_DSB_TIER); >>>> val = readl_relaxed(drvdata->base + TPDM_DSB_CR); >>>> + /* Set the test accurate mode */ >>>> + set_dsb_test_mode(drvdata, &val); >>>> + /* Set the byte lane for high-performance mode */ >>>> + set_dsb_hpsel_mode(drvdata, &val); >>>> + /* Set the performance mode */ >>>> + set_dsb_perf_mode(drvdata, &val); >>>> /* Set trigger type */ >>>> set_trigger_type(drvdata, &val); >>>> /* Set the enable bit of DSB control register to 1 */ >>>> @@ -252,6 +285,34 @@ static struct attribute_group tpdm_attr_grp = { >>>> .attrs = tpdm_attrs, >>>> }; >>>> +static ssize_t dsb_mode_show(struct device *dev, >>>> + struct device_attribute *attr, >>>> + char *buf) >>>> +{ >>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + >>>> + return sysfs_emit(buf, "%lx\n", >>>> + (unsigned long)drvdata->dsb->mode); >>>> +} >>>> + >>>> +static ssize_t dsb_mode_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; >>>> + >>>> + if ((kstrtoul(buf, 0, &val)) || val < 0) >>>> + return -EINVAL; >>>> + >>>> + spin_lock(&drvdata->spinlock); >>>> + drvdata->dsb->mode = val & TPDM_MODE_ALL; >>>> + spin_unlock(&drvdata->spinlock); >>>> + return size; >>>> +} >>>> +static DEVICE_ATTR_RW(dsb_mode); >>>> + >>>> static ssize_t dsb_trig_type_show(struct device *dev, >>>> struct device_attribute *attr, char *buf) >>>> { >>>> @@ -323,6 +384,7 @@ 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_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 68f33bd..79df07e 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>> @@ -15,11 +15,25 @@ >>>> /* Enable bit for DSB subunit */ >>>> #define TPDM_DSB_CR_ENA BIT(0) >>>> +/* Enable bit for DSB subunit perfmance mode */ >>>> +#define TPDM_DSB_CR_MODE BIT(1) >>>> /* Enable bit for DSB subunit trigger type */ >>>> #define TPDM_DSB_CR_TRIG_TYPE BIT(12) >>>> + >>>> /* Enable bit for DSB subunit trigger timestamp */ >>>> #define TPDM_DSB_TIER_XTRIG_TSENAB BIT(1) >>>> +/* DSB programming modes */ >>>> +/* Test mode control bit*/ >>>> +#define TPDM_DSB_MODE_TEST(val) (val & GENMASK(1, 0)) >>>> +/* Perforceman mode */ >>> >>> minor nit: typo ^^ >> I will update this in the next patch series. >>> >>>> +#define TPDM_DSB_MODE_PERF BIT(3) >>>> +/* High performance mode */ >>>> +#define TPDM_DSB_MODE_HPBYTESEL(val) (val & GENMASK(8, 4)) >>>> +#define TPDM_MODE_ALL (0xFFFFFFF) >>> >>> GENMASK(27, 0) ? >>> >>> Also, why do we cover bits 27-0 ? >> >> The TPDM mode is only represented by [0:8]bits. >> >> Can I replace it with "#define TPDM_DSB_MODE(val) (VAL & >> GENMASK(8, 0))"? > > #define TPDM_DSB_MODE_MASK GENMASK(8, 0) ? Sure, I will change it in the next patch series. Best, Tao > > Suzuki > _______________________________________________ > CoreSight mailing list -- coresight@lists.linaro.org > To unsubscribe send an email to coresight-leave@lists.linaro.org
On 27/04/2023 10:00, Tao Zhang wrote: > Add the nodes for DSB subunit MSR(mux select register) support. > The TPDM MSR (mux select register) interface is an optional > interface and associated bank of registers per TPDM subunit. > The intent of mux select registers is to control muxing structures > driving the TPDM’s’ various subunit interfaces. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 15 ++++++ > drivers/hwtracing/coresight/coresight-tpdm.c | 53 ++++++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tpdm.h | 3 ++ > 3 files changed, 71 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > index 639b6fb8..f746f25 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm > @@ -170,3 +170,18 @@ Description: > Accepts only one of the 2 values - 0 or 1. > 0 : Set the DSB pattern type to value. > 1 : Set the DSB pattern type to toggle. > + > +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_msr > +Date: March 2023 > +KernelVersion 6.3 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com> > +Description: > + (Write) Set the MSR(mux select register) of DSB tpdm. Read > + the MSR(mux select register) of DSB tpdm. > + > + Expected format is the following: > + <integer1> <integer2> > + > + Where: > + <integer1> : Index number of MSR register > + <integer2> : The value need to be written > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 627de36..5fe0bd5c 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -240,6 +240,14 @@ static int tpdm_datasets_setup(struct tpdm_drvdata *drvdata) > if (!drvdata->dsb) > return -ENOMEM; > } > + if (!of_property_read_u32(drvdata->dev->of_node, > + "qcom,dsb_msr_num", &drvdata->dsb->msr_num)) { > + drvdata->dsb->msr = devm_kzalloc(drvdata->dev, > + (drvdata->dsb->msr_num * sizeof(*drvdata->dsb->msr)), > + GFP_KERNEL); > + if (!drvdata->dsb->msr) > + return -ENOMEM; > + } > } > > return 0; > @@ -765,6 +773,50 @@ static ssize_t dsb_trig_ts_store(struct device *dev, > } > static DEVICE_ATTR_RW(dsb_trig_ts); > > +static ssize_t dsb_msr_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned int i; > + ssize_t size = 0; > + > + if (drvdata->dsb->msr_num == 0) > + return -EINVAL; > + > + spin_lock(&drvdata->spinlock); > + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { Shouldn't this be "i < drvdata->dsb->msr_num" ? > + size += sysfs_emit_at(buf, size, > + "%u 0x%x\n", i, drvdata->dsb->msr[i]); > + } > + spin_unlock(&drvdata->spinlock); > + > + return size; > +} > + > +static ssize_t dsb_msr_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned int num, val; > + int nval; > + > + if (drvdata->dsb->msr_num == 0) > + return -EINVAL; > + > + nval = sscanf(buf, "%u %x", &num, &val); > + if ((nval != 2) || (num >= (drvdata->dsb->msr_num - 1))) (num >= drvdata->dsb->msr_num) ? > + return -EINVAL; > + > + spin_lock(&drvdata->spinlock); > + drvdata->dsb->msr[num] = val; > + spin_unlock(&drvdata->spinlock); > + return size; > +} > +static DEVICE_ATTR_RW(dsb_msr); > + > static struct attribute *tpdm_dsb_attrs[] = { > &dev_attr_dsb_mode.attr, > &dev_attr_dsb_edge_ctrl.attr, > @@ -777,6 +829,7 @@ static struct attribute *tpdm_dsb_attrs[] = { > &dev_attr_dsb_trig_patt_mask.attr, > &dev_attr_dsb_trig_ts.attr, > &dev_attr_dsb_trig_type.attr, > + &dev_attr_dsb_msr.attr, > NULL, > }; > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h > index 9ad32a6..05e9f8e 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.h > +++ b/drivers/hwtracing/coresight/coresight-tpdm.h > @@ -18,6 +18,7 @@ > #define TPDM_DSB_XPMR(n) (0x7E8 + (n * 4)) > #define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) > #define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) > +#define TPDM_DSB_MSR(n) (0x980 + (n * 4)) > > /* Enable bit for DSB subunit */ > #define TPDM_DSB_CR_ENA BIT(0) > @@ -113,6 +114,8 @@ struct dsb_dataset { > u32 trig_patt_mask[TPDM_DSB_MAX_PATT]; > bool trig_ts; > bool trig_type; > + u32 msr_num; > + u32 *msr; > }; > > /** Where/when do we write to these registers in the DSB ? Suzuki
On 6/5/2023 6:24 PM, Suzuki K Poulose wrote: > On 27/04/2023 10:00, Tao Zhang wrote: >> Add the nodes for DSB subunit MSR(mux select register) support. >> The TPDM MSR (mux select register) interface is an optional >> interface and associated bank of registers per TPDM subunit. >> The intent of mux select registers is to control muxing structures >> driving the TPDM’s’ various subunit interfaces. >> >> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> >> --- >> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 15 ++++++ >> drivers/hwtracing/coresight/coresight-tpdm.c | 53 >> ++++++++++++++++++++++ >> drivers/hwtracing/coresight/coresight-tpdm.h | 3 ++ >> 3 files changed, 71 insertions(+) >> >> diff --git >> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> index 639b6fb8..f746f25 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm >> @@ -170,3 +170,18 @@ Description: >> Accepts only one of the 2 values - 0 or 1. >> 0 : Set the DSB pattern type to value. >> 1 : Set the DSB pattern type to toggle. >> + >> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_msr >> +Date: March 2023 >> +KernelVersion 6.3 >> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang >> (QUIC) <quic_taozha@quicinc.com> >> +Description: >> + (Write) Set the MSR(mux select register) of DSB tpdm. Read >> + the MSR(mux select register) of DSB tpdm. >> + >> + Expected format is the following: >> + <integer1> <integer2> >> + >> + Where: >> + <integer1> : Index number of MSR register >> + <integer2> : The value need to be written >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c >> b/drivers/hwtracing/coresight/coresight-tpdm.c >> index 627de36..5fe0bd5c 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >> @@ -240,6 +240,14 @@ static int tpdm_datasets_setup(struct >> tpdm_drvdata *drvdata) >> if (!drvdata->dsb) >> return -ENOMEM; >> } >> + if (!of_property_read_u32(drvdata->dev->of_node, >> + "qcom,dsb_msr_num", &drvdata->dsb->msr_num)) { >> + drvdata->dsb->msr = devm_kzalloc(drvdata->dev, >> + (drvdata->dsb->msr_num * >> sizeof(*drvdata->dsb->msr)), >> + GFP_KERNEL); >> + if (!drvdata->dsb->msr) >> + return -ENOMEM; >> + } >> } >> return 0; >> @@ -765,6 +773,50 @@ static ssize_t dsb_trig_ts_store(struct device >> *dev, >> } >> static DEVICE_ATTR_RW(dsb_trig_ts); >> +static ssize_t dsb_msr_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + unsigned int i; >> + ssize_t size = 0; >> + >> + if (drvdata->dsb->msr_num == 0) >> + return -EINVAL; >> + >> + spin_lock(&drvdata->spinlock); >> + for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { > > Shouldn't this be "i < drvdata->dsb->msr_num" ? Yes, I will update it to the next patch series. > > >> + size += sysfs_emit_at(buf, size, >> + "%u 0x%x\n", i, drvdata->dsb->msr[i]); >> + } >> + spin_unlock(&drvdata->spinlock); >> + >> + return size; >> +} >> + >> +static ssize_t dsb_msr_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, >> + size_t size) >> +{ >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + unsigned int num, val; >> + int nval; >> + >> + if (drvdata->dsb->msr_num == 0) >> + return -EINVAL; >> + >> + nval = sscanf(buf, "%u %x", &num, &val); >> + if ((nval != 2) || (num >= (drvdata->dsb->msr_num - 1))) > > (num >= drvdata->dsb->msr_num) ? Yes, I will update it to the next patch series. > >> + return -EINVAL; >> + >> + spin_lock(&drvdata->spinlock); >> + drvdata->dsb->msr[num] = val; >> + spin_unlock(&drvdata->spinlock); >> + return size; >> +} >> +static DEVICE_ATTR_RW(dsb_msr); >> + >> static struct attribute *tpdm_dsb_attrs[] = { >> &dev_attr_dsb_mode.attr, >> &dev_attr_dsb_edge_ctrl.attr, >> @@ -777,6 +829,7 @@ static struct attribute *tpdm_dsb_attrs[] = { >> &dev_attr_dsb_trig_patt_mask.attr, >> &dev_attr_dsb_trig_ts.attr, >> &dev_attr_dsb_trig_type.attr, >> + &dev_attr_dsb_msr.attr, >> NULL, >> }; >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h >> b/drivers/hwtracing/coresight/coresight-tpdm.h >> index 9ad32a6..05e9f8e 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >> @@ -18,6 +18,7 @@ >> #define TPDM_DSB_XPMR(n) (0x7E8 + (n * 4)) >> #define TPDM_DSB_EDCR(n) (0x808 + (n * 4)) >> #define TPDM_DSB_EDCMR(n) (0x848 + (n * 4)) >> +#define TPDM_DSB_MSR(n) (0x980 + (n * 4)) >> /* Enable bit for DSB subunit */ >> #define TPDM_DSB_CR_ENA BIT(0) >> @@ -113,6 +114,8 @@ struct dsb_dataset { >> u32 trig_patt_mask[TPDM_DSB_MAX_PATT]; >> bool trig_ts; >> bool trig_type; >> + u32 msr_num; >> + u32 *msr; >> }; >> /** > > > Where/when do we write to these registers in the DSB ? DSB MSR registers should be written in the DSB TPDM enablement function. I will update this to the next patch series. Best Tao > > Suzuki