[PATCH 03/10] Coresight: Add driver to support Coresight device TPDM
Suzuki K Poulose
suzuki.poulose at arm.com
Thu Nov 4 02:37:05 PDT 2021
Tao,
Some additional comments below.
On 02/11/2021 17:59, Mathieu Poirier wrote:
> Good morning,
>
>
> On Thu, Oct 21, 2021 at 03:38:49PM +0800, Tao Zhang wrote:
>> Add driver to support Coresight device TPDM. This driver provides
>> support for configuring monitor. Monitors are primarily responsible
>> for data set collection and support the ability to collect any
>> permutation of data set types. Monitors are also responsible for
>> interaction with system cross triggering.
>
> As far as I can tell there is nothing related to CTIs in this patch. And if
> there is, it is not documented.
>
Please could you add a separate file documenting the TPDM and
some of the specific details (what is used by the driver, e.g
PERPHID0/1 et) under Documentation/trace/coresight/ , in a separate
patch
>>
>> Signed-off-by: Tao Zhang <quic_taozha at quicinc.com>
>> ---
>> .../bindings/arm/coresight-tpdm.yaml | 86 +++
>
> As checkpatch says, this should be in a separate file.
>
>> MAINTAINERS | 5 +
>
> Since this is a coresight device Suzuki and I will continue the maintenance.
> The get_maintainer script will make sure you care CC'ed on patches related to
> the TPDM/TPDA drivers, and we would typically requried a "Reviewed-by" tag from
> you before merging.
>
>> drivers/hwtracing/coresight/Kconfig | 9 +
>> drivers/hwtracing/coresight/Makefile | 1 +
>> drivers/hwtracing/coresight/coresight-tpdm.c | 583 ++++++++++++++++++
>> 5 files changed, 684 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>> create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.c
>>
>> diff --git a/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>> new file mode 100644
>> index 000000000000..44541075d77f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>> @@ -0,0 +1,86 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/coresight-tpdm.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Trace, Profiling and Diagnostics Monitor - TPDM
>> +
>> +description: |
>> + The TPDM or Monitor serves as data collection component for various dataset
>> + types specified in the QPMDA spec. It covers Basic Counts (BC), Tenure
>> + Counts (TC), Continuous Multi-Bit (CMB), and Discrete Single Bit (DSB). It
>> + performs data collection in the data producing clock domain and transfers it
>> + to the data collection time domain, generally ATB clock domain.
>> +
>> + The primary use case of the TPDM is to collect data from different data
>> + sources and send it to a TPDA for packetization, timestamping, and funneling.
>> +
>> +maintainers:
>> + - Tao Zhang <quic_taozha at quicinc.com>
>> +
>> +properties:
>> + $nodename:
>> + pattern: "^tpdm(@[0-9a-f]+)$"
>> + compatible:
>> + items:
>> + - const: arm,primecell
You must have a compatible that identifies this as "tpdm", just like
the other components, even though it is not functional.
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + reg-names:
>> + items:
>> + - const: tpdm-base
Is the reg-name really necessary ?
>> +
>> + atid:
>> + maxItems: 1
>> + description: |
>> + The QPMDA specification repurposed the ATID field of the AMBA ATB
>> + specification to use it to convey packetization information to the
>> + Aggregator.
Please could you describe how this affects the device in the doc
requested above.
>> +
>> + out-ports:
>> + description: |
>> + Output connections from the TPDM to legacy CoreSight trace bus.
>> + $ref: /schemas/graph.yaml#/properties/ports
>> + properties:
>> + port:
>> + description: Output connection from the TPDM to legacy CoreSight
>> + Trace bus.
>> + $ref: /schemas/graph.yaml#/properties/port
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - reg-names
>> + - atid
>> + - clocks
>> + - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + # minimum TPDM definition.
>> + - |
>> + tpdm at 6980000 {
>> + compatible = "arm,primecell";
Like other components, we must have :
compatible = "qcom,<new-compatible>", "arm,primecell";
>> + reg = <0x6980000 0x1000>;
>> + reg-names = "tpdm-base";
>> +
>> + clocks = <&aoss_qmp>;
>> + clock-names = "apb_pclk";
>> +
>> + atid = <78>;
>> + out-ports {
>> + port {
>> + tpdm_turing_out_funnel_turing: endpoint {
>> + remote-endpoint =
>> + <&funnel_turing_in_tpdm_turing>;
>> + };
>> + };
>> + };
>> + };
>> +
>> +...
>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> index b6c4a48140ec..e7392a0dddeb 100644
>> --- a/drivers/hwtracing/coresight/Makefile
>> +++ b/drivers/hwtracing/coresight/Makefile
>> @@ -25,5 +25,6 @@ obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
>> obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o
>> obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o
>> obj-$(CONFIG_CORESIGHT_TRBE) += coresight-trbe.o
>> +obj-$(CONFIG_CORESIGHT_TPDM) += coresight-tpdm.o
>> coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \
>> coresight-cti-sysfs.o
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
>> new file mode 100644
>> index 000000000000..906776c859d6
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
...
>> +
>> +#ifdef CONFIG_CORESIGHT_TPDM_DEFAULT_ENABLE
>> +static int boot_enable = 1;
>> +#else
>> +static int boot_enable;
>> +#endif
>
> That isn't the proper way to do this. Look at how it is done in
> coresight-etm4x.c
>
>> +
>> +struct gpr_dataset {
>> + DECLARE_BITMAP(gpr_dirty, TPDM_GPR_REGS_MAX);
>> + uint32_t gp_regs[TPDM_GPR_REGS_MAX];
>
> Shouldn't this be u32?
>
>> +};
>> +
>> +struct bc_dataset {
>> + enum tpdm_mode capture_mode;
>> + enum tpdm_mode retrieval_mode;
>> + uint32_t sat_mode;
>> + uint32_t enable_counters;
>> + uint32_t clear_counters;
>> + uint32_t enable_irq;
>> + uint32_t clear_irq;
>> + uint32_t trig_val_lo[TPDM_BC_MAX_COUNTERS];
>> + uint32_t trig_val_hi[TPDM_BC_MAX_COUNTERS];
>> + uint32_t enable_ganging;
>> + uint32_t overflow_val[TPDM_BC_MAX_OVERFLOW];
>> + uint32_t msr[TPDM_BC_MAX_MSR];
>> +};
>> +
>> +struct tc_dataset {
>> + enum tpdm_mode capture_mode;
>> + enum tpdm_mode retrieval_mode;
>> + bool sat_mode;
>> + uint32_t enable_counters;
>> + uint32_t clear_counters;
>> + uint32_t enable_irq;
>> + uint32_t clear_irq;
>> + uint32_t trig_sel[TPDM_TC_MAX_TRIG];
>> + uint32_t trig_val_lo[TPDM_TC_MAX_TRIG];
>> + uint32_t trig_val_hi[TPDM_TC_MAX_TRIG];
>> + uint32_t msr[TPDM_TC_MAX_MSR];
>> +};
>> +
>> +struct dsb_dataset {
>> + uint32_t mode;
>> + uint32_t edge_ctrl[TPDM_DSB_MAX_EDCR];
>> + uint32_t edge_ctrl_mask[TPDM_DSB_MAX_EDCR / 2];
>> + uint32_t patt_val[TPDM_DSB_MAX_PATT];
>> + uint32_t patt_mask[TPDM_DSB_MAX_PATT];
>> + bool patt_ts;
>> + bool patt_type;
>> + uint32_t trig_patt_val[TPDM_DSB_MAX_PATT];
>> + uint32_t trig_patt_mask[TPDM_DSB_MAX_PATT];
>> + bool trig_ts;
>> + bool trig_type;
>> + uint32_t select_val[TPDM_DSB_MAX_SELECT];
>> + uint32_t msr[TPDM_DSB_MAX_MSR];
>> +};
>> +
>> +struct mcmb_dataset {
>> + uint8_t mcmb_trig_lane;
>> + uint8_t mcmb_lane_select;
>> +};
>> +
>> +struct cmb_dataset {
>> + bool trace_mode;
>> + uint32_t cycle_acc;
>> + uint32_t patt_val[TPDM_CMB_PATT_CMP];
>> + uint32_t patt_mask[TPDM_CMB_PATT_CMP];
>> + bool patt_ts;
>> + uint32_t trig_patt_val[TPDM_CMB_PATT_CMP];
>> + uint32_t trig_patt_mask[TPDM_CMB_PATT_CMP];
>> + bool trig_ts;
>> + bool ts_all;
>> + uint32_t msr[TPDM_CMB_MAX_MSR];
>> + uint8_t read_ctl_reg;
>> + struct mcmb_dataset *mcmb;
>> +};
>> +
>> +DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
>> +
>> +struct tpdm_drvdata {
>> + void __iomem *base;
>> + struct device *dev;
>> + struct coresight_device *csdev;
>> + int nr_tclk;
>> + struct clk **tclk;
>> + int nr_treg;
>> + struct regulator **treg;
>> + struct mutex lock;
>> + bool enable;
>> + bool clk_enable;
>> + DECLARE_BITMAP(datasets, TPDM_DATASETS);
>> + DECLARE_BITMAP(enable_ds, TPDM_DATASETS);
>> + enum tpdm_support_type tc_trig_type;
>> + enum tpdm_support_type bc_trig_type;
>> + enum tpdm_support_type bc_gang_type;
>> + uint32_t bc_counters_avail;
>> + uint32_t tc_counters_avail;
>> + struct gpr_dataset *gpr;
>> + struct bc_dataset *bc;
>> + struct tc_dataset *tc;
>> + struct dsb_dataset *dsb;
>> + struct cmb_dataset *cmb;
>> + int traceid;
>> + uint32_t version;
>> + bool msr_support;
>> + bool msr_fix_req;
>> + bool cmb_msr_skip;
>> +};
>
> All of these should also be in a header file and properly documented.
>
>> +
>> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata);
>
> This isn't needed.
>
>> +
>> +static void __tpdm_enable(struct tpdm_drvdata *drvdata)
>> +{
>> + TPDM_UNLOCK(drvdata);
>> +
>> + if (drvdata->clk_enable)
>> + tpdm_writel(drvdata, 0x1, TPDM_CLK_CTRL);
>> +
>> + TPDM_LOCK(drvdata);
>> +}
>> +
>> +static const struct coresight_ops_source tpdm_source_ops = {
>> + .trace_id = tpdm_trace_id,
>> + .enable = tpdm_enable,
>> + .disable = tpdm_disable,
>> +};
>> +
>> +static const struct coresight_ops tpdm_cs_ops = {
>> + .source_ops = &tpdm_source_ops,
>> +};
>> +
>> +static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata)
>> +{
>> + if (test_bit(TPDM_DS_GPR, drvdata->datasets)) {
>> + drvdata->gpr = devm_kzalloc(drvdata->dev, sizeof(*drvdata->gpr),
>> + GFP_KERNEL);
>> + if (!drvdata->gpr)
>> + return -ENOMEM;
>> + }
>> + if (test_bit(TPDM_DS_BC, drvdata->datasets)) {
>> + drvdata->bc = devm_kzalloc(drvdata->dev, sizeof(*drvdata->bc),
>> + GFP_KERNEL);
>> + if (!drvdata->bc)
>> + return -ENOMEM;
>> + }
>> + if (test_bit(TPDM_DS_TC, drvdata->datasets)) {
>> + drvdata->tc = devm_kzalloc(drvdata->dev, sizeof(*drvdata->tc),
>> + GFP_KERNEL);
>> + if (!drvdata->tc)
>> + return -ENOMEM;
>> + }
>> + if (test_bit(TPDM_DS_DSB, drvdata->datasets)) {
>> + drvdata->dsb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->dsb),
>> + GFP_KERNEL);
>> + if (!drvdata->dsb)
>> + return -ENOMEM;
>> + }
>> + if (test_bit(TPDM_DS_CMB, drvdata->datasets)) {
>> + drvdata->cmb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->cmb),
>> + GFP_KERNEL);
>> + if (!drvdata->cmb)
>> + return -ENOMEM;
>> + } else if (test_bit(TPDM_DS_MCMB, drvdata->datasets)) {
>> + drvdata->cmb = devm_kzalloc(drvdata->dev, sizeof(*drvdata->cmb),
>> + GFP_KERNEL);
>> + if (!drvdata->cmb)
>> + return -ENOMEM;
>> + drvdata->cmb->mcmb = devm_kzalloc(drvdata->dev,
>> + sizeof(*drvdata->cmb->mcmb),
>> + GFP_KERNEL);
>> + if (!drvdata->cmb->mcmb)
>> + return -ENOMEM;
>
> How can I understand what the above does when:
>
> 1) There isn't a single line of comments.
> 2) I don't know the HW.
> 3) I don't have access to the documentation.
+1
>
>> + }
>> + return 0;
>> +}
>> +
>> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
>> +{
>> + if (test_bit(TPDM_DS_BC, drvdata->datasets))
>> + drvdata->bc->retrieval_mode = TPDM_MODE_ATB;
>> +
>> + if (test_bit(TPDM_DS_TC, drvdata->datasets))
>> + drvdata->tc->retrieval_mode = TPDM_MODE_ATB;
>> +
>> + if (test_bit(TPDM_DS_DSB, drvdata->datasets)) {
>> + drvdata->dsb->trig_ts = true;
>> + drvdata->dsb->trig_type = false;
>> + }
>> +
>> + if (test_bit(TPDM_DS_CMB, drvdata->datasets) ||
>> + test_bit(TPDM_DS_MCMB, drvdata->datasets))
>> + drvdata->cmb->trig_ts = true;
>> +}
>> +
>> +static int tpdm_parse_of_data(struct tpdm_drvdata *drvdata)
>> +{
>> + int i, ret;
>> + const char *tclk_name, *treg_name;
>> + struct device_node *node = drvdata->dev->of_node;
>> +
>> + drvdata->clk_enable = of_property_read_bool(node, "qcom,clk-enable");
>> + drvdata->msr_fix_req = of_property_read_bool(node, "qcom,msr-fix-req");
>> + drvdata->cmb_msr_skip = of_property_read_bool(node,
>> + "qcom,cmb-msr-skip");
>> +
These properties must be listed as optional/mandatory in the DT binding
with proper description.
>> + drvdata->nr_tclk = of_property_count_strings(node, "qcom,tpdm-clks");
>> + if (drvdata->nr_tclk > 0) {
>> + drvdata->tclk = devm_kzalloc(drvdata->dev, drvdata->nr_tclk *
>> + sizeof(*drvdata->tclk),
>> + GFP_KERNEL);
>> + if (!drvdata->tclk)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < drvdata->nr_tclk; i++) {
>> + ret = of_property_read_string_index(node,
>> + "qcom,tpdm-clks", i, &tclk_name);
>> + if (ret)
>> + return ret;
>> +
>> + drvdata->tclk[i] = devm_clk_get(drvdata->dev,
>> + tclk_name);
>> + if (IS_ERR(drvdata->tclk[i]))
>> + return PTR_ERR(drvdata->tclk[i]);
>> + }
>> + }
>> +
>> + drvdata->nr_treg = of_property_count_strings(node, "qcom,tpdm-regs");
Where is this documented in the DT ?
>> + if (drvdata->nr_treg > 0) {
>> + drvdata->treg = devm_kzalloc(drvdata->dev, drvdata->nr_treg *
>> + sizeof(*drvdata->treg),
>> + GFP_KERNEL);
>> + if (!drvdata->treg)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < drvdata->nr_treg; i++) {
>> + ret = of_property_read_string_index(node,
>> + "qcom,tpdm-regs", i, &treg_name);
>> + if (ret)
>> + return ret;
>> +
>> + drvdata->treg[i] = devm_regulator_get(drvdata->dev,
>> + treg_name);
>> + if (IS_ERR(drvdata->treg[i]))
>> + return PTR_ERR(drvdata->treg[i]);
>> + }
>> + }
>
> _None_ of the above are defined in the yaml file and/or part of the example that
> is shown there. Moreover they don't appear in patch 10/10 where TPDM and TPDA are
> supposed to be introduced. I will comment on patch 10/10 when I'm done with
> this one.
+1
Suzuki
More information about the linux-arm-kernel
mailing list