CoreSight January 2023

coresight@lists.linaro.org

10 participants
48 discussions

Re: [PATCH 2/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

by Suzuki K Poulose

On 20/01/2023 00:51, Steve Clevenger wrote: > Add 32-bit read/write access option for Ampere ETMv4.6 64-bit registers. > Ampere Computing erratum AC03_DEBUG_10 describes a design decision where > 64-bit read/write access is not supported for the ETMv4.6 implementation. > These 64-bit registers must be accessed as 2 ea. 32-bit registers. > AC03_DEBUG_10 is described in the AmpereOne Developer Errata: > https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-d… As with the previous comment, please : a) If this is because of the system instruction access support b) Document the erratum > > Fix drvdata->nr_addr_cmp for() loop range bug to drvdata->nr_addr_cmp * 2 > in etm_enable_hw. Good catch ! Please separate this out and send it as a fix. I can queue this. > > Signed-off-by: Steve Clevenger <scclevenger(a)os.amperecomputing.com> > --- > .../coresight/coresight-etm4x-core.c | 81 ++++++++++++++----- > drivers/hwtracing/coresight/coresight-etm4x.h | 32 ++++++++ > 2 files changed, 93 insertions(+), 20 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 533be1928a09..bf4daa649cdf 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -452,18 +452,31 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) > if (etm4x_sspcicrn_present(drvdata, i)) > etm4x_relaxed_write32(csa, config->ss_pe_cmp[i], TRCSSPCICRn(i)); > } > - for (i = 0; i < drvdata->nr_addr_cmp; i++) { > - etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i)); > - etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i)); > + for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) { > + if (drvdata->no_quad_mmio) { > + etm4x_split_write64(csa, config->addr_val[i], TRCACVRn(i)); > + etm4x_split_write64(csa, config->addr_acc[i], TRCACATRn(i)); > + } else { > + etm4x_relaxed_write64(csa, config->addr_val[i], TRCACVRn(i)); > + etm4x_relaxed_write64(csa, config->addr_acc[i], TRCACATRn(i)); > + } > + } Something like this can be leave some places out. I think we could hide it under the generic helpers and handle it there. May be "struct csdev_access" can cache this "no_quad_mmio" and do the right thing ? > + for (i = 0; i < drvdata->numcidc; i++) { > + if (drvdata->no_quad_mmio) > + etm4x_split_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i)); > + else > + etm4x_relaxed_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i)); > } > - for (i = 0; i < drvdata->numcidc; i++) > - etm4x_relaxed_write64(csa, config->ctxid_pid[i], TRCCIDCVRn(i)); > etm4x_relaxed_write32(csa, config->ctxid_mask0, TRCCIDCCTLR0); > if (drvdata->numcidc > 4) > etm4x_relaxed_write32(csa, config->ctxid_mask1, TRCCIDCCTLR1); > > - for (i = 0; i < drvdata->numvmidc; i++) > - etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i)); > + for (i = 0; i < drvdata->numvmidc; i++) { > + if (drvdata->no_quad_mmio) > + etm4x_split_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i)); > + else > + etm4x_relaxed_write64(csa, config->vmid_val[i], TRCVMIDCVRn(i)); > + } > etm4x_relaxed_write32(csa, config->vmid_mask0, TRCVMIDCCTLR0); > if (drvdata->numvmidc > 4) > etm4x_relaxed_write32(csa, config->vmid_mask1, TRCVMIDCCTLR1); > @@ -1670,8 +1683,13 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) > } > > for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) { > - state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i)); > - state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i)); > + if (drvdata->no_quad_mmio) { > + state->trcacvr[i] = etm4x_split_read64(csa, TRCACVRn(i)); > + state->trcacatr[i] = etm4x_split_read64(csa, TRCACATRn(i)); > + } else { > + state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i)); > + state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i)); > + } > } > > /* > @@ -1681,11 +1699,19 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) > * unit") of ARM IHI 0064D. > */ > > - for (i = 0; i < drvdata->numcidc; i++) > - state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i)); > + for (i = 0; i < drvdata->numcidc; i++) { > + if (drvdata->no_quad_mmio) > + state->trccidcvr[i] = etm4x_split_read64(csa, TRCCIDCVRn(i)); > + else > + state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i)); > + } > > - for (i = 0; i < drvdata->numvmidc; i++) > - state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i)); > + for (i = 0; i < drvdata->numvmidc; i++) { > + if (drvdata->no_quad_mmio) > + state->trcvmidcvr[i] = etm4x_split_read64(csa, TRCVMIDCVRn(i)); > + else > + state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i)); > + } > > state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0); > if (drvdata->numcidc > 4) > @@ -1799,15 +1825,28 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) > } > > for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) { > - etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i)); > - etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i)); > + if (drvdata->no_quad_mmio) { > + etm4x_split_write64(csa, state->trcacvr[i], TRCACVRn(i)); > + etm4x_split_write64(csa, state->trcacatr[i], TRCACATRn(i)); > + } else { > + etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i)); > + etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i)); > + } > } > > - for (i = 0; i < drvdata->numcidc; i++) > - etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i)); > + for (i = 0; i < drvdata->numcidc; i++) { > + if (drvdata->no_quad_mmio) > + etm4x_split_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i)); > + else > + etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i)); > + } > > - for (i = 0; i < drvdata->numvmidc; i++) > - etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i)); > + for (i = 0; i < drvdata->numvmidc; i++) { > + if (drvdata->no_quad_mmio) > + etm4x_split_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i)); > + else > + etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i)); > + } > > etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0); > if (drvdata->numcidc > 4) > @@ -2047,8 +2086,10 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) > * isolates the manufacturer JEP106 ID in the PID. > * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8) > */ > - if ((init_arg.pid & 0x000FF000) == 0x00096000) > + if ((init_arg.pid & 0x000FF000) == 0x00096000) { > drvdata->mmio_external = true; > + drvdata->no_quad_mmio = true; > + } > > /* > * Serialize against CPUHP callbacks to avoid race condition > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h > index cf4f9f2e1807..0650bcdff410 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x.h > +++ b/drivers/hwtracing/coresight/coresight-etm4x.h > @@ -1016,6 +1016,7 @@ struct etmv4_save_state { > * the trace unit. > * @arch_features: Bitmap of arch features of etmv4 devices. > * @mmio_external: True if ETM considers MMIO an external access. > + * @no_quad_mmio: True if ETM does not support 64-bit (quad) access. > */ > struct etmv4_drvdata { > void __iomem *base; > @@ -1069,6 +1070,7 @@ struct etmv4_drvdata { > bool skip_power_up; > DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX); > bool mmio_external; > + bool no_quad_mmio; > }; > > /* Address comparator access types */ > @@ -1093,6 +1095,36 @@ void etm4_config_trace_mode(struct etmv4_config *config); > u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit); > void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit); > > +/* 64-bit aligned to convert 64-bit access to 2 ea. 32-bit access */ > +#pragma pack(push, 8) > + > +struct etm_quad_split { > + u32 lsw; > + u32 msw; > +}; > + > +#pragma pack(pop) > + > +static inline u64 etm4x_split_read64(struct csdev_access *csa, unsigned int offset) > +{ > + struct etm_quad_split container; > + > + container.lsw = etm4x_read32(csa, offset); > + container.msw = etm4x_read32(csa, offset + sizeof(u32)); > + > + return *(u64 *) &container; Wouldn't this break with the "endianness" flip ? (Not that we have BE implementations). Could we not combine the two values to a 64bit value and pass that instead ? Similarly below. Suzuki > +} > + > +static inline void etm4x_split_write64(struct csdev_access *csa, u64 quad, unsigned int offset) > +{ > + struct etm_quad_split container; > + > + *(u64 *) &container = quad; > + > + etm4x_relaxed_write32(csa, container.lsw, offset); > + etm4x_relaxed_write32(csa, container.msw, offset + sizeof(u32)); > +} > + > static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata) > { > return drvdata->arch >= ETM_ARCH_ETE;

1 year, 7 months

[PATCH v2 0/9] Add support to configure TPDM DSB subunit

by Tao Zhang

From: Tao Zhang <taozha(a)qti.qualcomm.com> 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(a)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 "[v17,0/9] Coresight: Add support for TPDM and TPDA" https://patchwork.kernel.org/project/linux-arm-msm/cover/20230117145708.167… TPDM_DSB commit tree: https://git.codelinaro.org/clo/linux-kernel/coresight/-/tree/tpdm-dsb-v2 https://git.codelinaro.org/clo/linux-kernel/coresight/-/commits/tpdm-dsb-v2 Changes in V2: 1. Change the name of the property "qcom,dsb-elem-size" to "qcom,dsb-element-size" -- Suzuki K Poulose 2. Update the TPDA yaml file for the item "qcom,dsb-elem-size". -- Krzysztof Kozlowski 3. Add the full name of DSB in the description of the item "qcom,dsb-elem-size". -- Rob Herring Changes in V1: 1. Change the definition of the property "qcom,dsb-elem-size" from "uint32-array" to "uint32-matrix". -- Krzysztof Kozlowski 2. Add the full name of DSB. -- Rob Herring 3. Deal with 2 entries in an iteration in TPDA driver. -- Suzuki K Poulose 4. Divide the function "tpdm_datasets_alloc" into two functions, "tpdm_datasets_setup" and "tpdm_datasets_alloc". 5. Detecte the input string with the conventional semantics automatically, and constrain the size of the input value. -- Suzuki K Poulose 6. Use the hook function "is_visible()" to hide the DSB related knobs if the data sets are missing. -- Suzuki K Poulose 7. Use the macros "FIELD_GET" and "FIELD_PREP" to set the values. -- Suzuki K Poulose 8. Update the definition of the macros in TPDM driver. 9. Update the comments of the values for the nodes which are for DSB element creation and onfigure pattern match output. -- Suzuki K Poulose 10. Use API "sysfs_emit" to "replace scnprintf". -- Suzuki K Poulose Tao Zhang (9): dt-bindings: arm: Add support for DSB element coresight-tpda: Add DSB dataset support coresight-tpdm: Initialize DSB subunit configuration coresight-tpdm: Add reset node to TPDM node coresight-tpdm: Add nodes to set trigger timestamp and type coresight-tpdm: Add node to set dsb programming mode coresight-tpdm: Add nodes for dsb element creation coresight-tpdm: Add nodes to configure pattern match output coresight-tpdm: Add nodes for timestamp request .../bindings/arm/qcom,coresight-tpda.yaml | 22 + drivers/hwtracing/coresight/coresight-tpda.c | 62 ++ drivers/hwtracing/coresight/coresight-tpda.h | 4 + drivers/hwtracing/coresight/coresight-tpdm.c | 630 ++++++++++++++++++++- drivers/hwtracing/coresight/coresight-tpdm.h | 65 +++ 5 files changed, 778 insertions(+), 5 deletions(-) -- 2.7.4

1 year, 8 months

Re: [PATCH 31/35] Documentation: trace: correct spelling

by Suzuki K Poulose

On 27/01/2023 06:40, Randy Dunlap wrote: > Correct spelling problems for Documentation/trace/ as reported > by codespell. > > Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org> > Cc: Steven Rostedt <rostedt(a)goodmis.org> > Cc: Masami Hiramatsu <mhiramat(a)kernel.org> > Cc: Daniel Bristot de Oliveira <bristot(a)kernel.org> > Cc: linux-trace-kernel(a)vger.kernel.org > Cc: Mathieu Poirier <mathieu.poirier(a)linaro.org> > Cc: Suzuki K Poulose <suzuki.poulose(a)arm.com> > Cc: coresight(a)lists.linaro.org > Cc: linux-arm-kernel(a)lists.infradead.org > Cc: Jonathan Corbet <corbet(a)lwn.net> > Cc: linux-doc(a)vger.kernel.org > --- > Documentation/trace/coresight/coresight-etm4x-reference.rst | 2 +- > Documentation/trace/events.rst | 6 +++--- > Documentation/trace/fprobe.rst | 2 +- > Documentation/trace/ftrace-uses.rst | 2 +- > Documentation/trace/hwlat_detector.rst | 2 +- > Documentation/trace/rv/runtime-verification.rst | 2 +- > Documentation/trace/uprobetracer.rst | 2 +- > 7 files changed, 9 insertions(+), 9 deletions(-) > > diff -- a/Documentation/trace/coresight/coresight-etm4x-reference.rst b/Documentation/trace/coresight/coresight-etm4x-reference.rst > --- a/Documentation/trace/coresight/coresight-etm4x-reference.rst > +++ b/Documentation/trace/coresight/coresight-etm4x-reference.rst > @@ -675,7 +675,7 @@ Bit assignments shown below:- > reconstructed using only conditional branches. > > There is currently no support in Perf for supplying modified binaries to the decoder, so this > - feature is only inteded to be used for debugging purposes or with a 3rd party tool. > + feature is only intended to be used for debugging purposes or with a 3rd party tool. > > Choosing this option will result in a significant increase in the amount of trace generated - > possible danger of overflows, or fewer instructions covered. Note, that this option also For the above: Acked-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> Thanks Suzuki

1 year, 9 months

Re: [PATCH v2 1/9] dt-bindings: arm: Add support for DSB element

by Tao Zhang

Hi Krzysztof, On 1/19/2023 6:44 PM, Krzysztof Kozlowski wrote: > On 19/01/2023 08:41, Tao Zhang wrote: >> Add property "qcom,dsb-elem-size" to support DSB(Discrete Single >> Bit) 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(a)quicinc.com> >> Signed-off-by: Tao Zhang <taozha(a)qti.qualcomm.com> > You are the same person and it is still the same organization > (Qualcomm), right? Only one SoB. I will change and update this in the next patch series. > >> --- >> .../bindings/arm/qcom,coresight-tpda.yaml | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> index 2ec9b5b..298db7f 100644 >> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tpda.yaml >> @@ -58,6 +58,26 @@ properties: >> minItems: 1 >> maxItems: 2 >> >> + qcom,dsb-element-size: >> + description: | >> + Specifies the DSB(Discrete Single Bit) element size supported by >> + each monitor connected to the aggregator on each port. Should be >> + specified in pairs <port, dsb element size>. > s/port/port number/ It should be "port number" here. I will change "<port, dsb element size>" to "<port number, DSB element size>" in the next patch series. >> + >> + Note: The maximum value of the port number depends on how many >> + input ports the current TPDA has. DSB element size currently only >> + supports 32-bit and 64-bit. >> + $ref: /schemas/types.yaml#/definitions/uint32-matrix >> + items: > Are some reasonable maxItems known? This is related to hardware design, depending on how many input ports the TPDA has. We cannot limit it to a reasonable maximum value from the software. According to the existing hardware design, TPDA with the most input ports has about 30 input ports. But there may be TPDA with more input ports. > >> + items: >> + - description: | >> + "port" indicates TPDA input port number > What is "port"? You quoted it like it was some name of variable or > property. Where is then? The "port" here refers to the port number of other Coresight devices connected to the TPDA input port. I will change and update it in the next patch series. >> + minimum: 0 >> + - description: | >> + "dsb element size" indicates dsb element size > "A" indicates A. This sentence does not make sense. > > Also missing units. > > s/dsb/DSB/ "DSB element size" indicate the size of the element in DSB. DSB(Discrete Single Bit) is a data collection unit. I will change and update it in the next patch series. > >> + minimum: 0 >> + maximum: 64 >> + >> clocks: >> maxItems: 1 >> >> @@ -100,6 +120,8 @@ examples: >> compatible = "qcom,coresight-tpda", "arm,primecell"; >> reg = <0x6004000 0x1000>; >> >> + qcom,dsb-element-size = <0 32>; >> + >> clocks = <&aoss_qmp>; >> clock-names = "apb_pclk"; >> > Best regards, > Krzysztof Best, Tao

1 year, 9 months

Re: [PATCH] perf: cs-etm: Update decoder code for OpenCSD version 1.4

by James Clark

On 24/01/2023 21:50, Ian Rogers wrote: > On Fri, Jan 20, 2023 at 7:47 AM James Clark <james.clark(a)arm.com> wrote: >> >> >> >> On 20/01/2023 15:37, Mike Leach wrote: >>> OpenCSD version 1.4 is released with support for FEAT_ITE. >>> This adds a new packet type, with associated output element ID in >>> the packet type enum - OCSD_GEN_TRC_ELEM_INSTRUMENTATION. >>> >>> As we just ignore this packet in perf, add to the switch statement >>> to avoid the "enum not handled in switch error", but conditionally >>> so as not to break the perf build for older OpenCSD installations. >>> >>> Signed-off-by: Mike Leach <mike.leach(a)linaro.org> >>> --- >>> tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >>> index fa3aa9c0fb2e..48e7121880a9 100644 >>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >>> @@ -604,6 +604,9 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer( >>> case OCSD_GEN_TRC_ELEM_CUSTOM: >>> case OCSD_GEN_TRC_ELEM_SYNC_MARKER: >>> case OCSD_GEN_TRC_ELEM_MEMTRANS: >>> +#if (OCSD_VER_NUM >= 0x010400) >>> + case OCSD_GEN_TRC_ELEM_INSTRUMENTATION: >>> +#endif >>> default: >>> break; >>> } >> >> Checked the build with both old and new versions of OpenCSD and it's ok: >> >> Reviewed-by: James Clark <james.clark(a)arm.com> > > Acked-by: Ian Rogers <irogers(a)google.com> > > Thanks, > Ian Hi Arnaldo, Is it ok to merge this change? If anyone is building with the latest OpenCSD they will get a build error on the unhandled switch case, and we just got it in our CI too. I suppose we could disable the warning around this switch, but it's pretty rare to add new packets so might be best to leave it. Thanks James

1 year, 9 months

Re: [PATCH -next] coresight: tpda: fix return value check in tpda_probe()

by Suzuki K Poulose

On 29/01/2023 08:42, Yang Yingliang wrote: > devm_ioremap_resource() never returns NULL pointer, it > will return ERR_PTR() when it fails, so replace the check > with IS_ERR(). Thanks for the patch. > > Fixes: 5b7916625c01 ("Coresight: Add TPDA link driver") > Signed-off-by: Yang Yingliang <yangyingliang(a)huawei.com> > --- > drivers/hwtracing/coresight/coresight-tpda.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c > index 19c25c9f6157..6313b12880e0 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.c > +++ b/drivers/hwtracing/coresight/coresight-tpda.c > @@ -145,7 +145,7 @@ static int tpda_probe(struct amba_device *adev, const struct amba_id *id) > dev_set_drvdata(dev, drvdata); > > base = devm_ioremap_resource(dev, &adev->res); > - if (!base) > + if (IS_ERR(base)) > return -ENOMEM; I have fixed this up to : return PTR_ERR(base); for consistency. Thanks Suzuki

1 year, 9 months

Re: [PATCH] Coresight: tpda/tpdm: remove incorrect __exit annotation

by Suzuki K Poulose

On 27/01/2023 17:00, Arnd Bergmann wrote: > On Fri, Jan 27, 2023, at 17:46, Suzuki K Poulose wrote: >> On 26/01/2023 20:37, Arnd Bergmann wrote: >>> On Thu, Jan 26, 2023, at 19:02, Suzuki K Poulose wrote: >>>> On 26/01/2023 16:35, Arnd Bergmann wrote: >>>>> From: Arnd Bergmann <arnd(a)arndb.de> >>>> Thanks for the fix, I will queue this. Btw, I did try to >>>> reproduce it locally, but couldn't trigger the warnings, >>>> even with >>>> >>>> CONFIG_WERROR=y >>>> >>>> and all CORESIGHT configs builtin. I see other drivers doing the >>>> same outside coresight too. Just curious to know why is this >>>> any different. Is it specific to "bus" driver (e.g. AMBA) ? >>> >>> The warning comes from postprocessing the object file, it's got >>> nothing to do with the bus type, only with a symbol in .data >>> referencing a symbol in .init.text. Maybe there are some >>> config options that keep the section from getting discarded? >>> Or possibly you only built the files in this directory, but did >>> not get to the final link? >> >> I did a full kernel build. Also, I see a similar issue with the >> coresight-etm4x (by code inspection) driver. Did you not hit that ? >> >> May be there is a config option that is masking it on my end. But >> the case of etm4x driver is puzzling. >> >> $ git grep etm4_remove_amba >> drivers/hwtracing/coresight/coresight-etm4x-core.c >> drivers/hwtracing/coresight/coresight-etm4x-core.c:static void __exit >> etm4_remove_amba(struct amba_device *adev) >> drivers/hwtracing/coresight/coresight-etm4x-core.c: .remove >> = etm4_remove_amba, > > Indeed, that one clearly has the same but, but I have never > observed a warning for it. > > I checked one more thing and found that I only get the warning > for 32-bit Arm builds, but not arm64. Since the etm4x driver > 'depends on ARM64' for its use of asm/sysreg.h, > I never test-built it on 32-bit arm. > > From the git history of arch/arm64/kernel/vmlinux.lds.S, > I can see that arm64 never discards the .exit section, see > commit 07c802bd7c39 ("arm64: vmlinux.lds.S: don't discard > .exit.* sections at link-time"). That makes sense, thanks for getting to the bottom of this. I have pushed it to coresight next. https://git.kernel.org/coresight/c/0c1ccc158bbc Kind regards Suzuki > > Arnd

1 year, 9 months

Re: [PATCH] Coresight: tpda/tpdm: remove incorrect __exit annotation

by Suzuki K Poulose

On 26/01/2023 20:37, Arnd Bergmann wrote: > On Thu, Jan 26, 2023, at 19:02, Suzuki K Poulose wrote: >> On 26/01/2023 16:35, Arnd Bergmann wrote: >>> From: Arnd Bergmann <arnd(a)arndb.de> >> Thanks for the fix, I will queue this. Btw, I did try to >> reproduce it locally, but couldn't trigger the warnings, >> even with >> >> CONFIG_WERROR=y >> >> and all CORESIGHT configs builtin. I see other drivers doing the >> same outside coresight too. Just curious to know why is this >> any different. Is it specific to "bus" driver (e.g. AMBA) ? > > The warning comes from postprocessing the object file, it's got > nothing to do with the bus type, only with a symbol in .data > referencing a symbol in .init.text. Maybe there are some > config options that keep the section from getting discarded? > Or possibly you only built the files in this directory, but did > not get to the final link? I did a full kernel build. Also, I see a similar issue with the coresight-etm4x (by code inspection) driver. Did you not hit that ? May be there is a config option that is masking it on my end. But the case of etm4x driver is puzzling. $ git grep etm4_remove_amba drivers/hwtracing/coresight/coresight-etm4x-core.c drivers/hwtracing/coresight/coresight-etm4x-core.c:static void __exit etm4_remove_amba(struct amba_device *adev) drivers/hwtracing/coresight/coresight-etm4x-core.c: .remove = etm4_remove_amba, Suzuki > > Arnd

1 year, 9 months

Re: [PATCH] Coresight: tpda/tpdm: remove incorrect __exit annotation

by Suzuki K Poulose

Hi Arnd, On 26/01/2023 16:35, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd(a)arndb.de> > > 'remove' callbacks get called whenever a device is unbound from > the driver, which can get triggered from user space. > > Putting it into the __exit section means that the function gets > dropped in for built-in drivers, as pointed out by this build > warning: > > `tpda_remove' referenced in section `.data' of drivers/hwtracing/coresight/coresight-tpda.o: defined in discarded section `.exit.text' of drivers/hwtracing/coresight/coresight-tpda.o > `tpdm_remove' referenced in section `.data' of drivers/hwtracing/coresight/coresight-tpdm.o: defined in discarded section `.exit.text' of drivers/hwtracing/coresight/coresight-tpdm.o > Thanks for the fix, I will queue this. Btw, I did try to reproduce it locally, but couldn't trigger the warnings, even with CONFIG_WERROR=y and all CORESIGHT configs builtin. I see other drivers doing the same outside coresight too. Just curious to know why is this any different. Is it specific to "bus" driver (e.g. AMBA) ? Suzuki > Fixes: 5b7916625c01 ("Coresight: Add TPDA link driver") > Fixes: b3c71626a933 ("Coresight: Add coresight TPDM source driver") > Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> > --- > drivers/hwtracing/coresight/coresight-tpda.c | 2 +- > drivers/hwtracing/coresight/coresight-tpdm.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c > index 19c25c9f6157..382d648529e7 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.c > +++ b/drivers/hwtracing/coresight/coresight-tpda.c > @@ -174,7 +174,7 @@ static int tpda_probe(struct amba_device *adev, const struct amba_id *id) > return 0; > } > > -static void __exit tpda_remove(struct amba_device *adev) > +static void tpda_remove(struct amba_device *adev) > { > struct tpda_drvdata *drvdata = dev_get_drvdata(&adev->dev); > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index 349a82bb3270..9479a5e8c672 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -223,7 +223,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) > return 0; > } > > -static void __exit tpdm_remove(struct amba_device *adev) > +static void tpdm_remove(struct amba_device *adev) > { > struct tpdm_drvdata *drvdata = dev_get_drvdata(&adev->dev); >

1 year, 9 months

Re: [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready

by Suzuki K Poulose

Folks, On 24/01/2023 20:09, Yabin Cui wrote: > Ping for review. And I still can't reproduce it, even if I reduced the > timeout to 2us and tried different workloads. Any suggestions for how > to reproduce it? > I think we should go ahead and fix this in the driver to handle flaky hardware cases. But I would like this to be addressed for all the TMC types, not just ETRs, as Mike pointed out. Thanks Suzuki > Thanks, > Yabin > > On Tue, Jan 10, 2023 at 1:06 PM Yabin Cui <yabinc(a)google.com> wrote: >> >>> Do you have a reproducer for this or some more info? >>> For example is it a regression or has it always been there? And on which >>> platform. >> >> It happens on Pixel 6 and 7. We collect ETM data periodically from some >> internal dogfood devices. The problem has happened several times on >> dogfood devices. But I am still trying to reproduce it locally. >> >> We use the scatter-gather mode of ETR, and allocate a 4M buffer. In userspace, >> we use simpleperf in Android to collect system wide ETM data. What is special >> is, simpleperf disables and reenables perf events every 100ms to flush ETM >> data to perf aux buffer. >> >> Pixel 6 and 7 have hardware monitoring AXI traffic. The hardware finds ETR is >> trying to read from or write to a low invalid address (like 0x2E0000). The >> problem always happens right after the "tmc_etr: timeout while waiting for TMC >> to be Ready" message. And in almost all cases, I can find a "timeout while >> waiting for completion of Manual Flush" message from the previous session. >> >> One log history is below: >> [11484.610008][ C0] coresight tmc_etr0: timeout while waiting for >> completion of Manual Flush >> [11484.610177][ C0] coresight tmc_etr0: timeout while waiting for >> TMC to be Ready >> [11484.615367][ C0] coresight tmc_etr0: timeout while waiting for >> completion of Manual Flush >> [11484.615534][ C0] coresight tmc_etr0: timeout while waiting for >> TMC to be Ready >> [12089.486044][ C0] coresight tmc_etr0: timeout while waiting for >> TMC to be Ready >> AXI error report reading from invalid address >> >> Another log history is below: >> [76709.382650][ C5] coresight tmc_etf1: timeout while waiting for >> TMC to be Ready >> [76709.382852][ C7] coresight tmc_etr0: timeout while waiting for >> completion of Manual Flush >> [76709.382995][ C7] coresight tmc_etr0: timeout while waiting for >> TMC to be Ready >> [76709.384510][ C7] coresight tmc_etr0: timeout while waiting for >> completion of Manual Flush >> [76709.384649][ C7] coresight tmc_etr0: timeout while waiting for >> TMC to be Ready >> [76709.384838][ C0] coresight tmc_etr0: timeout while waiting for >> TMC to be Ready >> AIX error report writing to invalid address >> >> It seems if the previous manual flush doesn't finish gracefully, ETR may not be >> ready for the next enable (even after 10min as in the first log). And if we >> continue to enable ETR, an invalid AXI IO may happen. >> >> Thanks, >> Yabin >> >> On Tue, Jan 10, 2023 at 10:04 AM Suzuki K Poulose >> <suzuki.poulose(a)arm.com> wrote: >>> >>> On 10/01/2023 17:48, Mike Leach wrote: >>>> Hi, >>>> >>>> On Tue, 10 Jan 2023 at 09:30, James Clark <james.clark(a)arm.com> wrote: >>>>> >>>>> >>>>> >>>>> On 09/01/2023 23:43, Yabin Cui wrote: >>>>>> Otherwise, it may cause error in AXI bus and result in a kernel panic. >>>>> >>>>> Hi Yabin, >>>>> >>>>> Thanks for the fix. Do you have a reproducer for this or some more info? >>>>> For example is it a regression or has it always been there? And on which >>>>> platform. >>>>> >>>>> Thanks >>>>> James >>>>> >>>>>> >>>>>> Signed-off-by: Yabin Cui <yabinc(a)google.com> >>>>>> --- >>>>>> .../hwtracing/coresight/coresight-tmc-core.c | 4 +++- >>>>>> .../hwtracing/coresight/coresight-tmc-etr.c | 18 +++++++++++++++--- >>>>>> drivers/hwtracing/coresight/coresight-tmc.h | 2 +- >>>>>> 3 files changed, 19 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>>> index 07abf28ad725..c106d142e632 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>>> @@ -31,7 +31,7 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb"); >>>>>> DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf"); >>>>>> DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr"); >>>>>> >>>>>> -void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata) >>>>>> +int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata) >>>>>> { >>>>>> struct coresight_device *csdev = drvdata->csdev; >>>>>> struct csdev_access *csa = &csdev->access; >>>>>> @@ -40,7 +40,9 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata) >>>>>> if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) { >>>>>> dev_err(&csdev->dev, >>>>>> "timeout while waiting for TMC to be Ready\n"); >>>>>> + return -EBUSY; >>>>>> } >>>>>> + return 0; >>>>>> } >>>>>> >>>>>> void tmc_flush_and_stop(struct tmc_drvdata *drvdata) >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c >>>>>> index 867ad8bb9b0c..2da99dd41ed6 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >>>>>> @@ -983,15 +983,21 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata) >>>>>> etr_buf->ops->sync(etr_buf, rrp, rwp); >>>>>> } >>>>>> >>>>>> -static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata) >>>>>> +static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata) >>>>>> { >>>>>> u32 axictl, sts; >>>>>> struct etr_buf *etr_buf = drvdata->etr_buf; >>>>>> + int rc = 0; >>>>>> >>>>>> CS_UNLOCK(drvdata->base); >>>>>> >>>>>> /* Wait for TMCSReady bit to be set */ >>>>>> - tmc_wait_for_tmcready(drvdata); >>>>>> + rc = tmc_wait_for_tmcready(drvdata); >>>>>> + if (rc) { >>>>>> + dev_err(&drvdata->csdev->dev, "not ready ETR isn't enabled\n"); >>>>>> + CS_LOCK(drvdata->base); >>>>>> + return rc; >>>>>> + } >>>>>> >>>>>> writel_relaxed(etr_buf->size / 4, drvdata->base + TMC_RSZ); >>>>>> writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE); >>>>>> @@ -1032,6 +1038,7 @@ static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata) >>>>>> tmc_enable_hw(drvdata); >>>>>> >>>>>> CS_LOCK(drvdata->base); >>>>>> + return rc; >>>>>> } >>>>>> >>>>>> static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata, >>>>>> @@ -1060,7 +1067,12 @@ static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata, >>>>>> rc = coresight_claim_device(drvdata->csdev); >>>>>> if (!rc) { >>>>>> drvdata->etr_buf = etr_buf; >>>>>> - __tmc_etr_enable_hw(drvdata); >>>>>> + rc = __tmc_etr_enable_hw(drvdata); >>>>>> + if (rc) { >>>>>> + drvdata->etr_buf = NULL; >>>>>> + coresight_disclaim_device(drvdata->csdev); >>>>>> + tmc_etr_disable_catu(drvdata); >>>>>> + } >>>>>> } >>>>>> >>>>>> return rc; >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h >>>>>> index 66959557cf39..01c0382a29c0 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h >>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h >>>>>> @@ -255,7 +255,7 @@ struct tmc_sg_table { >>>>>> }; >>>>>> >>>>>> /* Generic functions */ >>>>>> -void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata); >>>>>> +int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata); >>>>>> void tmc_flush_and_stop(struct tmc_drvdata *drvdata); >>>>>> void tmc_enable_hw(struct tmc_drvdata *drvdata); >>>>>> void tmc_disable_hw(struct tmc_drvdata *drvdata); >>>> >>>> There is no point in waiting for a timeout, and then carrying on even >>>> when it is exceeded. As such this patch seems reasonable. >>>> We should also apply the same principle to the ETF and ETB devices >>>> which use the same tmc_wait_for_tmcready() function. >>> >>> +1 >>> >>> I am fine with pushing this change, as it is doing the right thing. >>> >>>> >>>> However - the concern is that this appears to be happening on starting >>>> the ETR - there should be no outstanding AXI operations that cause the >>>> system to not be ready - as we will either be using this the first >>>> time after reset, or we should have successfully stopped and flushed >>>> the ETR from the previous operation. This warrants further >>>> investigation - should we be extending the timeout - which is already >>>> at a rather generous 100uS, and do we also need to check the MemErr >>>> bit in the status register? >>> >>> It would be good to dump the value of TMC_STATUS to see what is going >>> on. >>> >>>> >>>> As James says, we need details of when and how the problem occurs - >>>> as far as I know it has not been seen on any systems we currently use >>>> (though could have been missed given the current code) >>> >>> +1 >>> >>> Kind regards >>> Suzuki >>> >>> >>>> >>>> Regards >>>> >>>> Mike >>>> >>>> >>>> -- >>>> Mike Leach >>>> Principal Engineer, ARM Ltd. >>>> Manchester Design Centre. UK >>>

1 year, 9 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight January 2023