mbox series

[v5,00/10] Coresight: Add support for TPDM and TPDA

Message ID 20220412125035.40312-1-quic_jinlmao@quicinc.com
Headers show
Series Coresight: Add support for TPDM and TPDA | expand

Message

Mao Jinlong April 12, 2022, 12:50 p.m. UTC
This series adds support for the trace performance monitoring and
diagnostics hardware (TPDM and TPDA). It is composed of two major
elements.
a) Changes for original coresight framework to support for TPDM and TPDA.
b) Add driver code for TPDM and TPDA.

Introduction of changes for original coresight framework
Support TPDM as new coresight source.
Since only STM and ETM are supported as coresight source originally.
TPDM is a newly added coresight source. We need to change
the original way of saving coresight path to support more types source
for coresight driver.
The following patch is to add support more coresight sources.
    coresight: core: Use IDR for non-cpu bound sources' paths.

Introduction of TPDM and TPDA
TPDM - The trace performance monitoring and diagnostics monitor or TPDM in
short serves as data collection component for various dataset types
specified in the QPMDA(Qualcomm performance monitoring and diagnostics
architecture) spec. 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.
     Coresight: Add coresight TPDM source driver
     dt-bindings: arm: Adds CoreSight TPDM hardware definitions
     coresight-tpdm: Add DSB dataset support
     coresight-tpdm: Add integration test support
     docs: sysfs: coresight: Add sysfs ABI documentation for TPDM

TPDA - The trace performance monitoring and diagnostics aggregator or
TPDA in short serves as an arbitration and packetization engine for the
performance monitoring and diagnostics network as specified in the QPMDA
(Qualcomm performance monitoring and diagnostics architecture)
specification. The primary use case of the TPDA is to provide
packetization, funneling and timestamping of Monitor data as specified
in the QPMDA specification.
The following patch is to add driver for TPDA.
     Coresight: Add TPDA link driver
     dt-bindings: arm: Adds CoreSight TPDA hardware definitions

The last patch of this series is a device tree modification, which add
the TPDM and TPDA configuration to device tree for validating.
    ARM: dts: msm: Add coresight components for SM8250
    ARM: dts: msm: Add tpdm mm/prng for sm8250

Once this series patches are applied properly, the tpdm and tpda nodes
should be observed at the coresight path /sys/bus/coresight/devices
e.g.
/sys/bus/coresight/devices # ls -l | grep tpd
tpda0 -> ../../../devices/platform/soc@0/6004000.tpda/tpda0
tpdm0 -> ../../../devices/platform/soc@0/6c08000.mm.tpdm/tpdm0

We can use the commands are similar to the below to validate TPDMs.
Enable coresight sink first.

echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink
echo 1 > /sys/bus/coresight/devices/tpdm0/enable_source
echo 1 > /sys/bus/coresight/devices/tpdm0/integration_test
echo 2 > /sys/bus/coresight/devices/tpdm0/integration_test
The test data will be collected in the coresight sink which is enabled.
If rwp register of the sink is keeping updating when do
integration_test (by cat tmc_etf0/mgmt/rwp), it means there is data
generated from TPDM to sink.

There must be a tpda between tpdm and the sink. When there are some
other trace event hw components in the same HW block with tpdm, tpdm
and these hw components will connect to the coresight funnel. When
there is only tpdm trace hw in the HW block, tpdm will connect to
tpda directly.
  
    +---------------+                +-------------+
    |  tpdm@6c08000 |                |tpdm@684C000 |
    +-------|-------+                +------|------+
            |                               |
    +-------|-------+                       |
    | funnel@6c0b000|                       |
    +-------|-------+                       |
            |                               |
    +-------|-------+                       |
    |funnel@6c2d000 |                       |
    +-------|-------+                       |
            |                               |
            |    +---------------+          |
            +----- tpda@6004000  -----------+
                 +-------|-------+
                         |
                 +-------|-------+
                 |funnel@6005000 |
                 +---------------+

This patch series depends on patch series
"coresight: Add new API to allocate trace source ID values".
https://patchwork.kernel.org/project/linux-arm-kernel/cover/20220308205000.27646-1-mike.leach@linaro.org/ 

Changes from V4:
1. Keep the ETM source paths per-CPU and use IDR for other sources'
paths. (Suzuki K Poulose <suzuki.poulose@arm.com>)

Mao Jinlong (10):
  coresight: core: Use IDR for non-cpu bound sources' paths.
  Coresight: Add coresight TPDM source driver
  dt-bindings: arm: Adds CoreSight TPDM hardware definitions
  coresight-tpdm: Add DSB dataset support
  coresight-tpdm: Add integration test support
  docs: sysfs: coresight: Add sysfs ABI documentation for TPDM
  Coresight: Add TPDA link driver
  dt-bindings: arm: Adds CoreSight TPDA hardware definitions
  ARM: dts: msm: Add coresight components for SM8250
  ARM: dts: msm: Add tpdm mm/prng for sm8250

 .../testing/sysfs-bus-coresight-devices-tpdm  |  13 +
 .../bindings/arm/coresight-tpda.yaml          | 119 +++
 .../bindings/arm/coresight-tpdm.yaml          |  99 +++
 .../devicetree/bindings/arm/coresight.txt     |   7 +
 MAINTAINERS                                   |   1 +
 .../arm64/boot/dts/qcom/sm8250-coresight.dtsi | 708 ++++++++++++++++++
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |   2 +
 drivers/hwtracing/coresight/Kconfig           |  33 +
 drivers/hwtracing/coresight/Makefile          |   2 +
 drivers/hwtracing/coresight/coresight-core.c  |  42 +-
 drivers/hwtracing/coresight/coresight-tpda.c  | 192 +++++
 drivers/hwtracing/coresight/coresight-tpda.h  |  32 +
 drivers/hwtracing/coresight/coresight-tpdm.c  | 270 +++++++
 drivers/hwtracing/coresight/coresight-tpdm.h  |  61 ++
 include/linux/coresight.h                     |   1 +
 15 files changed, 1570 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
 create mode 100644 Documentation/devicetree/bindings/arm/coresight-tpda.yaml
 create mode 100644 Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
 create mode 100644 arch/arm64/boot/dts/qcom/sm8250-coresight.dtsi
 create mode 100644 drivers/hwtracing/coresight/coresight-tpda.c
 create mode 100644 drivers/hwtracing/coresight/coresight-tpda.h
 create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.c
 create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.h

Comments

Mike Leach April 14, 2022, 3:22 p.m. UTC | #1
Hi,

On Tue, 12 Apr 2022 at 13:51, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>
> Adds new coresight-tpdm.yaml file describing the bindings required
> to define tpdm in the device trees.
>
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../bindings/arm/coresight-tpdm.yaml          | 99 +++++++++++++++++++
>  .../devicetree/bindings/arm/coresight.txt     |  7 ++
>  MAINTAINERS                                   |  1 +
>  3 files changed, 107 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>
> diff --git a/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
> new file mode 100644
> index 000000000000..05210e0fc262
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +# Copyright (c) 2022 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 Implementation defined ((ImplDef),
> +  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:
> +  - Suzuki K Poulose <suzuki.poulose@arm.com>
> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
> +

These should be e-mail addresses of maintainers for this binding, not
the coresight sub-system.
See writing-schema.rst

> +properties:
> +  $nodename:
> +    pattern: "^tpdm(@[0-9a-f]+)$"
> +  compatible:
> +    items:
> +      - const: qcom,coresight-tpdm
> +      - const: arm,primecell
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +   items:
> +     - const: apb_pclk
> +
> +  out-ports:
> +    description: |
> +      Output connections from the TPDM to coresight funnle/tpda.
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    properties:
> +      port:
> +        description: Output connection from the TPDM to coresight
> +            funnel/tpda.
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  # minimum TPDM definition. TPDM connect to coresight funnel.
> +  - |
> +    tpdm@6980000 {
> +      compatible = "qcom,coresight-tpdm", "arm,primecell";
> +      reg = <0x6980000 0x1000>;
> +
> +      clocks = <&aoss_qmp>;
> +      clock-names = "apb_pclk";
> +
> +      out-ports {
> +        port {
> +          tpdm_turing_out_funnel_turing: endpoint {
> +            remote-endpoint =
> +              <&funnel_turing_in_tpdm_turing>;
> +          };
> +        };
> +      };
> +    };
> +  # minimum TPDM definition. TPDM connect to coresight TPDA.
> +  - |
> +    tpdm@684c000 {
> +      compatible = "qcom,coresight-tpdm", "arm,primecell";
> +      reg = <0x684c000 0x1000>;
> +
> +      clocks = <&aoss_qmp>;
> +      clock-names = "apb_pclk";
> +
> +      out-ports {
> +        port {
> +          tpdm_prng_out_tpda_qdss: endpoint {
> +            remote-endpoint =
> +              <&tpda_qdss_in_tpdm_prng>;
> +          };
> +        };
> +      };
> +    };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index c68d93a35b6c..f7ce8af48574 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -52,6 +52,10 @@ its hardware characteristcs.
>                         "arm,coresight-cti", "arm,primecell";
>                         See coresight-cti.yaml for full CTI definitions.
>
> +               - Trace, Profiling and Diagnostics Monitor (TPDM):
> +                       "qcom,coresight-tpdm", "arm,primecell";
> +                       See coresight-tpdm.yaml for full TPDM definitions.
> +
>         * reg: physical base address and length of the register
>           set(s) of the component.
>
> @@ -82,6 +86,9 @@ its hardware characteristcs.
>  * Required properties for Coresight Cross Trigger Interface (CTI)
>         See coresight-cti.yaml for full CTI definitions.
>
> +* Required properties for Trace, Profiling and Diagnostics Monitor (TPDM)
> +       See coresight-tpdm.yaml for full TPDM definitions.
> +
>  * Required properties for devices that don't show up on the AMBA bus, such as
>    non-configurable replicators and non-configurable funnels:
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 61d9f114c37f..0d39bb37935d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1977,6 +1977,7 @@ T:        git git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git
>  F:     Documentation/ABI/testing/sysfs-bus-coresight-devices-*
>  F:     Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt
>  F:     Documentation/devicetree/bindings/arm/coresight-cti.yaml
> +F:     Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>  F:     Documentation/devicetree/bindings/arm/coresight.txt
>  F:     Documentation/devicetree/bindings/arm/ete.yaml
>  F:     Documentation/devicetree/bindings/arm/trbe.yaml
> --
> 2.17.1
>

With the above:
Reviewed by: Mike Leach <mike.leach@linaro.org>
Mao Jinlong April 15, 2022, 9:56 a.m. UTC | #2
Hi Mike,

On 4/14/2022 11:22 PM, Mike Leach wrote:
> Hi,
>
> On Tue, 12 Apr 2022 at 13:51, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>> Adds new coresight-tpdm.yaml file describing the bindings required
>> to define tpdm in the device trees.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   .../bindings/arm/coresight-tpdm.yaml          | 99 +++++++++++++++++++
>>   .../devicetree/bindings/arm/coresight.txt     |  7 ++
>>   MAINTAINERS                                   |  1 +
>>   3 files changed, 107 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>> new file mode 100644
>> index 000000000000..05210e0fc262
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>> @@ -0,0 +1,99 @@
>> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
>> +# Copyright (c) 2022 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 Implementation defined ((ImplDef),
>> +  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:
>> +  - Suzuki K Poulose <suzuki.poulose@arm.com>
>> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
>> +
> These should be e-mail addresses of maintainers for this binding, not
> the coresight sub-system.
> See writing-schema.rst
I will update.
>
>> +properties:
>> +  $nodename:
>> +    pattern: "^tpdm(@[0-9a-f]+)$"
>> +  compatible:
>> +    items:
>> +      - const: qcom,coresight-tpdm
>> +      - const: arm,primecell
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +   items:
>> +     - const: apb_pclk
>> +
>> +  out-ports:
>> +    description: |
>> +      Output connections from the TPDM to coresight funnle/tpda.
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +    properties:
>> +      port:
>> +        description: Output connection from the TPDM to coresight
>> +            funnel/tpda.
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  # minimum TPDM definition. TPDM connect to coresight funnel.
>> +  - |
>> +    tpdm@6980000 {
>> +      compatible = "qcom,coresight-tpdm", "arm,primecell";
>> +      reg = <0x6980000 0x1000>;
>> +
>> +      clocks = <&aoss_qmp>;
>> +      clock-names = "apb_pclk";
>> +
>> +      out-ports {
>> +        port {
>> +          tpdm_turing_out_funnel_turing: endpoint {
>> +            remote-endpoint =
>> +              <&funnel_turing_in_tpdm_turing>;
>> +          };
>> +        };
>> +      };
>> +    };
>> +  # minimum TPDM definition. TPDM connect to coresight TPDA.
>> +  - |
>> +    tpdm@684c000 {
>> +      compatible = "qcom,coresight-tpdm", "arm,primecell";
>> +      reg = <0x684c000 0x1000>;
>> +
>> +      clocks = <&aoss_qmp>;
>> +      clock-names = "apb_pclk";
>> +
>> +      out-ports {
>> +        port {
>> +          tpdm_prng_out_tpda_qdss: endpoint {
>> +            remote-endpoint =
>> +              <&tpda_qdss_in_tpdm_prng>;
>> +          };
>> +        };
>> +      };
>> +    };
>> +
>> +...
>> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
>> index c68d93a35b6c..f7ce8af48574 100644
>> --- a/Documentation/devicetree/bindings/arm/coresight.txt
>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
>> @@ -52,6 +52,10 @@ its hardware characteristcs.
>>                          "arm,coresight-cti", "arm,primecell";
>>                          See coresight-cti.yaml for full CTI definitions.
>>
>> +               - Trace, Profiling and Diagnostics Monitor (TPDM):
>> +                       "qcom,coresight-tpdm", "arm,primecell";
>> +                       See coresight-tpdm.yaml for full TPDM definitions.
>> +
>>          * reg: physical base address and length of the register
>>            set(s) of the component.
>>
>> @@ -82,6 +86,9 @@ its hardware characteristcs.
>>   * Required properties for Coresight Cross Trigger Interface (CTI)
>>          See coresight-cti.yaml for full CTI definitions.
>>
>> +* Required properties for Trace, Profiling and Diagnostics Monitor (TPDM)
>> +       See coresight-tpdm.yaml for full TPDM definitions.
>> +
>>   * Required properties for devices that don't show up on the AMBA bus, such as
>>     non-configurable replicators and non-configurable funnels:
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 61d9f114c37f..0d39bb37935d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1977,6 +1977,7 @@ T:        git git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git
>>   F:     Documentation/ABI/testing/sysfs-bus-coresight-devices-*
>>   F:     Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt
>>   F:     Documentation/devicetree/bindings/arm/coresight-cti.yaml
>> +F:     Documentation/devicetree/bindings/arm/coresight-tpdm.yaml
>>   F:     Documentation/devicetree/bindings/arm/coresight.txt
>>   F:     Documentation/devicetree/bindings/arm/ete.yaml
>>   F:     Documentation/devicetree/bindings/arm/trbe.yaml
>> --
>> 2.17.1
>>
> With the above:
> Reviewed by: Mike Leach <mike.leach@linaro.org>
Mike Leach April 19, 2022, 9:23 a.m. UTC | #3
Hi,

On Tue, 12 Apr 2022 at 13:51, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>
> TPDM serves as data collection component for various dataset types.
> DSB(Discrete Single Bit) is one of the dataset types. DSB subunit
> can be enabled for data collection by writing 1 to the first bit of
> DSB_CR register. This change is to add enable/disable function for
> DSB dataset by writing DSB_CR register.
>
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-tpdm.c | 61 ++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tpdm.h | 21 +++++++
>  2 files changed, 82 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
> index 3900ae50670a..d7b970cdcf51 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
> @@ -20,7 +20,28 @@
>
>  DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
>
> +static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
> +{
> +       u32 val;
> +
> +       /* Set the enable bit of DSB control register to 1 */
> +       val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
> +       val = val | BIT(0);

Please use #defined values, declared in the header file rather than
BIT(x) here. (the etm4x drivers have been recently updated to use the
same pattern).
e.g.
 val |= TPDM_DSB_CR_ENA;

> +       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
> +}
> +
>  /* TPDM enable operations */
> +static void _tpdm_enable(struct tpdm_drvdata *drvdata)
> +{
> +       CS_UNLOCK(drvdata->base);
> +
> +       /* Check if DSB datasets is present for TPDM. */
> +       if (test_bit(TPDM_DS_DSB, drvdata->datasets))
> +               tpdm_enable_dsb(drvdata);
> +
> +       CS_LOCK(drvdata->base);
> +}
> +
>  static int tpdm_enable(struct coresight_device *csdev,
>                        struct perf_event *event, u32 mode)
>  {
> @@ -32,6 +53,7 @@ static int tpdm_enable(struct coresight_device *csdev,
>                 return -EBUSY;
>         }
>
> +       _tpdm_enable(drvdata);
>         drvdata->enable = true;
>         mutex_unlock(&drvdata->lock);
>
> @@ -39,7 +61,29 @@ static int tpdm_enable(struct coresight_device *csdev,
>         return 0;
>  }
>
> +static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
> +{
> +       u32 val;
> +
> +       /* Set the enable bit of DSB control register to 0 */
> +       val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
> +       val = val & ~BIT(0);

val &= ~TPDM_DSB_CR_ENA;

> +       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
> +}
> +
>  /* TPDM disable operations */
> +static void _tpdm_disable(struct tpdm_drvdata *drvdata)
> +{
> +       CS_UNLOCK(drvdata->base);
> +
> +       /* Check if DSB datasets is present for TPDM. */
> +       if (test_bit(TPDM_DS_DSB, drvdata->datasets))
> +               tpdm_disable_dsb(drvdata);
> +
> +       CS_LOCK(drvdata->base);
> +
> +}
> +
>  static void tpdm_disable(struct coresight_device *csdev,
>                          struct perf_event *event)
>  {
> @@ -51,6 +95,7 @@ static void tpdm_disable(struct coresight_device *csdev,
>                 return;
>         }
>
> +       _tpdm_disable(drvdata);
>         drvdata->enable = false;
>         mutex_unlock(&drvdata->lock);
>
> @@ -66,6 +111,21 @@ static const struct coresight_ops tpdm_cs_ops = {
>         .source_ops     = &tpdm_source_ops,
>  };
>
> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
> +{
> +       int i;
> +       u32 pidr;
> +
> +       CS_UNLOCK(drvdata->base);
> +       /*  Get the datasets present on the TPDM. */
> +       pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
> +       for (i = 0; i < TPDM_DATASETS; i++) {
> +               if (pidr & BIT(i))
> +                       __set_bit(i, drvdata->datasets);

Could this be considerably simpified? - there are a maximum of 7 bits
for datasets - and you are setting the same bit in drvdata->datasets
for each bit in pidr

e.g. if the datasets declaration is a simple unsigned long, then the
following is easier to read and understand:

drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);

The tests above could then become:
if (drvdata->datasets & BIT(TPDM_DS_DSB)) ...


> +       }
> +       CS_LOCK(drvdata->base);
> +}
> +
>  static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>         struct device *dev = &adev->dev;
> @@ -104,6 +164,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>         if (IS_ERR(drvdata->csdev))
>                 return PTR_ERR(drvdata->csdev);
>
> +       tpdm_init_default_data(drvdata);
>         /* Decrease pm refcount when probe is done.*/
>         pm_runtime_put(&adev->dev);
>
> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
> index 94a7748a5426..8f05070879c4 100644
> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
> @@ -6,6 +6,25 @@
>  #ifndef _CORESIGHT_CORESIGHT_TPDM_H
>  #define _CORESIGHT_CORESIGHT_TPDM_H
>
> +/* The max number of the datasets that TPDM supports */
> +#define TPDM_DATASETS       7
> +
> +/* DSB Subunit Registers */
> +#define TPDM_DSB_CR            (0x780)
> +

declare the enable bit here with comment documenting usage  - see above.

> +/**
> + * This enum is for PERIPHIDR0 register of TPDM.
> + * The fields [6:0] of PERIPHIDR0 are used to determine what
> + * interfaces and subunits are present on a given TPDM.
> + *
> + * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
> + * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
> + */
> +enum tpdm_dataset {
> +       TPDM_DS_IMPLDEF,
> +       TPDM_DS_DSB,
> +};
> +
>  /**
>   * struct tpdm_drvdata - specifics associated to an TPDM component
>   * @base:       memory mapped base address for this component.
> @@ -13,6 +32,7 @@
>   * @csdev:      component vitals needed by the framework.
>   * @lock:       lock for the enable value.
>   * @enable:     enable status of the component.
> + * @datasets:   The datasets types present of the TPDM.
>   */
>
>  struct tpdm_drvdata {
> @@ -21,6 +41,7 @@ struct tpdm_drvdata {
>         struct coresight_device *csdev;
>         struct mutex            lock;
>         bool                    enable;
> +       DECLARE_BITMAP(datasets, TPDM_DATASETS);

Could this simply be declared as an unsigned long? - then simplify the
code in the .c file.

Regards

Mike

>  };
>
>  #endif  /* _CORESIGHT_CORESIGHT_TPDM_H */
> --
> 2.17.1
>
Mike Leach April 19, 2022, 9:25 a.m. UTC | #4
Hi

On Tue, 12 Apr 2022 at 13:51, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>
> Add API usage document for sysfs API in TPDM driver.
>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../ABI/testing/sysfs-bus-coresight-devices-tpdm    | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> new file mode 100644
> index 000000000000..d70ba429f38d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> @@ -0,0 +1,13 @@
> +What:          /sys/bus/coresight/devices/<tpdm-name>/integration_test
> +Date:          April 2022
> +KernelVersion  5.18
> +Contact:       Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
> +Description:
> +               (Write) Run integration test for tpdm. Integration test
> +               will generate test data for tpdm. It can help to make
> +               sure that the trace path is enabled and the link configurations
> +               are fine.
> +
> +               value to this sysfs node:
> +               1 : Genreate 64 bits data
s/Genreate/Generate

> +               2 : Generate 32 bits data
> --
> 2.17.1
>

Reviewed by: Mike Leach <mike.leach@linaro.org>
Mike Leach April 19, 2022, 9:51 a.m. UTC | #5
Hi

On Tue, 12 Apr 2022 at 13:51, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>
> TPDA(Trace, Profiling and Diagnostics Aggregator) is
> to provide packetization, funneling and timestamping of
> TPDM data. Multiple monitors are connected to different
> input ports of TPDA.This change is to add tpda
> enable/disable/probe functions for coresight tpda driver.
>
>  - - - -         - - - -        - - - -
> | TPDM 0|      | TPDM 1 |     | TPDM 2|
>  - - - -         - - - -        - - - -
>     |               |             |
>     |_ _ _ _ _ _    |     _ _ _ _ |
>                 |   |    |
>                 |   |    |
>            ------------------
>           |        TPDA      |
>            ------------------
>
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  drivers/hwtracing/coresight/Kconfig          |  11 ++
>  drivers/hwtracing/coresight/Makefile         |   1 +
>  drivers/hwtracing/coresight/coresight-tpda.c | 192 +++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tpda.h |  32 ++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-tpda.c
>  create mode 100644 drivers/hwtracing/coresight/coresight-tpda.h
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 60248fef4089..317c5e7f4819 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -223,4 +223,15 @@ config CORESIGHT_TPDM_INTEGRATION_TEST
>           operation to facilitate integration testing and software bringup
>           and/or to instrument topology discovery. The TPDM utilizes integration
>           mode to accomplish integration testing and software bringup.
> +
> +config CORESIGHT_TPDA
> +       tristate "CoreSight Trace, Profiling & Diagnostics Aggregator driver"
> +       help
> +         This driver provides support for configuring aggregator. This is
> +         primarily useful for pulling the data sets from one or more
> +         attached monitors and pushing the resultant data out. Multiple
> +         monitors are connected on different input ports of TPDA.
> +
> +         To compile this driver as a module, choose M here: the module will be
> +         called coresight-tpda.
>  endif

TDPA / TDPM are functionally linked - it does not make sense to
configure one without the other.
Kernel configuration should reflect this - as a minimum TDPM
configuration should select TDPA.



> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 6bb9b1746bc7..1712d82e7260 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -26,5 +26,6 @@ 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
> +obj-$(CONFIG_CORESIGHT_TPDA) += coresight-tpda.o
>  coresight-cti-y := coresight-cti-core.o        coresight-cti-platform.o \
>                    coresight-cti-sysfs.o
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> new file mode 100644
> index 000000000000..9519990c68e2
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/amba/bus.h>
> +#include <linux/bitmap.h>
> +#include <linux/coresight.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include "coresight-priv.h"
> +#include "coresight-tpda.h"
> +#include "coresight-trace-id.h"
> +
> +DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
> +
> +/* Settings pre enabling port control register */
> +static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
> +{
> +       u32 val;
> +
> +       val = readl_relaxed(drvdata->base + TPDA_CR);
> +       val |= (drvdata->atid << 6);
> +       writel_relaxed(val, drvdata->base + TPDA_CR);
> +}
> +
> +static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)
> +{
> +       u32 val;
> +
> +       val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
> +       /* Enable the port */
> +       val = val | BIT(0);

#define a constant in the header to use here.

> +       writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> +}
> +
> +static void _tpda_enable(struct tpda_drvdata *drvdata, int port)
> +{
> +       CS_UNLOCK(drvdata->base);
> +
> +       if (!drvdata->enable)
> +               tpda_enable_pre_port(drvdata);
> +
> +       tpda_enable_port(drvdata, port);
> +
> +       CS_LOCK(drvdata->base);
> +}
> +
> +static int tpda_enable(struct coresight_device *csdev, int inport, int outport)
> +{
> +       struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       mutex_lock(&drvdata->lock);
> +       _tpda_enable(drvdata, inport);
> +       drvdata->enable = true;
> +       mutex_unlock(&drvdata->lock);
> +
> +       dev_info(drvdata->dev, "TPDA inport %d enabled\n", inport);
> +       return 0;
> +}
> +
> +static void _tpda_disable(struct tpda_drvdata *drvdata, int port)
> +{
> +       u32 val;
> +
> +       CS_UNLOCK(drvdata->base);
> +
> +       val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
> +       val = val & ~BIT(0);

use a #defined constant.

> +       writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
> +
> +       CS_LOCK(drvdata->base);
> +}
> +
> +static void tpda_disable(struct coresight_device *csdev, int inport,
> +                          int outport)
> +{
> +       struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       mutex_lock(&drvdata->lock);
> +       _tpda_disable(drvdata, inport);
> +       drvdata->enable = false;
> +       mutex_unlock(&drvdata->lock);
> +
> +       dev_info(drvdata->dev, "TPDA inport %d disabled\n", inport);
> +}
> +
> +static const struct coresight_ops_link tpda_link_ops = {
> +       .enable         = tpda_enable,
> +       .disable        = tpda_disable,
> +};
> +
> +static const struct coresight_ops tpda_cs_ops = {
> +       .link_ops       = &tpda_link_ops,
> +};
> +
> +static int tpda_init_default_data(struct tpda_drvdata *drvdata)
> +{
> +       int atid;
> +       /*
> +        * TPDA must has a unique atid. This atid can uniquely
> +        * identify the TPDM trace source connect to the TPDA.
> +        */
> +       atid = coresight_trace_id_get_system_id(coresight_get_trace_id_map());
> +       if (atid < 0)
> +               return atid;
> +
> +       drvdata->atid = atid;
> +       return 0;
> +}
> +
> +static int tpda_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +       int ret;
> +       struct device *dev = &adev->dev;
> +       struct coresight_platform_data *pdata;
> +       struct tpda_drvdata *drvdata;
> +       struct coresight_desc desc = { 0 };
> +
> +       pdata = coresight_get_platform_data(dev);
> +       if (IS_ERR(pdata))
> +               return PTR_ERR(pdata);
> +       adev->dev.platform_data = pdata;
> +
> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +       if (!drvdata)
> +               return -ENOMEM;
> +
> +       drvdata->dev = &adev->dev;
> +       dev_set_drvdata(dev, drvdata);
> +
> +       drvdata->base = devm_ioremap_resource(dev, &adev->res);
> +       if (!drvdata->base)
> +               return -ENOMEM;
> +
> +       mutex_init(&drvdata->lock);
> +
> +       ret = tpda_init_default_data(drvdata);
> +       if (ret)
> +               return ret;
> +
> +       desc.name = coresight_alloc_device_name(&tpda_devs, dev);
> +       if (!desc.name)
> +               return -ENOMEM;
> +       desc.type = CORESIGHT_DEV_TYPE_LINK;
> +       desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
> +       desc.ops = &tpda_cs_ops;
> +       desc.pdata = adev->dev.platform_data;
> +       desc.dev = &adev->dev;
> +       drvdata->csdev = coresight_register(&desc);
> +       if (IS_ERR(drvdata->csdev))
> +               return PTR_ERR(drvdata->csdev);
> +
> +       pm_runtime_put(&adev->dev);
> +
> +       dev_dbg(drvdata->dev, "TPDA initialized\n");
> +       return 0;
> +}
> +
> +/*
> + * Different TPDA has different periph id.
> + * The difference is 0-7 bits' value. So ignore 0-7 bits.
> + */
> +static struct amba_id tpda_ids[] = {
> +       {
> +               .id     = 0x000f0f00,
> +               .mask   = 0x000fff00,
> +       },
> +       { 0, 0},
> +};
> +
> +static struct amba_driver tpda_driver = {
> +       .drv = {
> +               .name   = "coresight-tpda",
> +               .owner  = THIS_MODULE,
> +               .suppress_bind_attrs = true,
> +       },
> +       .probe          = tpda_probe,
> +       .id_table       = tpda_ids,
> +};
> +

There is no code to release resources when this module is unloaded.
You need a .remove function, that as a minimum calls
coresight_unregister().

Regards

Mike

> +module_amba_driver(tpda_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Aggregator driver");
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> new file mode 100644
> index 000000000000..6ac33b9c1ea4
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _CORESIGHT_CORESIGHT_TPDA_H
> +#define _CORESIGHT_CORESIGHT_TPDA_H
> +
> +#define TPDA_CR                        (0x000)
> +#define TPDA_Pn_CR(n)          (0x004 + (n * 4))
> +
> +#define TPDA_MAX_INPORTS       32
> +
> +/**
> + * struct tpda_drvdata - specifics associated to an TPDA component
> + * @base:       memory mapped base address for this component.
> + * @dev:        The device entity associated to this component.
> + * @csdev:      component vitals needed by the framework.
> + * @lock:       lock for the enable value.
> + * @enable:     enable status of the component.
> + * @traceid:    trace source identification for the data packet by TPDA.
> + */
> +struct tpda_drvdata {
> +       void __iomem            *base;
> +       struct device           *dev;
> +       struct coresight_device *csdev;
> +       struct mutex            lock;
> +       bool                    enable;
> +       u32                     atid;
> +};
> +
> +#endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
> --
> 2.17.1
>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Mao Jinlong April 20, 2022, 2:41 p.m. UTC | #6
Thank you Mike for the review.

On 4/19/2022 5:23 PM, Mike Leach wrote:
> Hi,
>
> On Tue, 12 Apr 2022 at 13:51, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>> TPDM serves as data collection component for various dataset types.
>> DSB(Discrete Single Bit) is one of the dataset types. DSB subunit
>> can be enabled for data collection by writing 1 to the first bit of
>> DSB_CR register. This change is to add enable/disable function for
>> DSB dataset by writing DSB_CR register.
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 61 ++++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpdm.h | 21 +++++++
>>   2 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c
>> index 3900ae50670a..d7b970cdcf51 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -20,7 +20,28 @@
>>
>>   DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");
>>
>> +static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>> +{
>> +       u32 val;
>> +
>> +       /* Set the enable bit of DSB control register to 1 */
>> +       val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>> +       val = val | BIT(0);
> Please use #defined values, declared in the header file rather than
> BIT(x) here. (the etm4x drivers have been recently updated to use the
> same pattern).
> e.g.
>   val |= TPDM_DSB_CR_ENA;
I will update in next version.
>> +       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>> +}
>> +
>>   /* TPDM enable operations */
>> +static void _tpdm_enable(struct tpdm_drvdata *drvdata)
>> +{
>> +       CS_UNLOCK(drvdata->base);
>> +
>> +       /* Check if DSB datasets is present for TPDM. */
>> +       if (test_bit(TPDM_DS_DSB, drvdata->datasets))
>> +               tpdm_enable_dsb(drvdata);
>> +
>> +       CS_LOCK(drvdata->base);
>> +}
>> +
>>   static int tpdm_enable(struct coresight_device *csdev,
>>                         struct perf_event *event, u32 mode)
>>   {
>> @@ -32,6 +53,7 @@ static int tpdm_enable(struct coresight_device *csdev,
>>                  return -EBUSY;
>>          }
>>
>> +       _tpdm_enable(drvdata);
>>          drvdata->enable = true;
>>          mutex_unlock(&drvdata->lock);
>>
>> @@ -39,7 +61,29 @@ static int tpdm_enable(struct coresight_device *csdev,
>>          return 0;
>>   }
>>
>> +static void tpdm_disable_dsb(struct tpdm_drvdata *drvdata)
>> +{
>> +       u32 val;
>> +
>> +       /* Set the enable bit of DSB control register to 0 */
>> +       val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>> +       val = val & ~BIT(0);
> val &= ~TPDM_DSB_CR_ENA;
I will update in next version.
>
>> +       writel_relaxed(val, drvdata->base + TPDM_DSB_CR);
>> +}
>> +
>>   /* TPDM disable operations */
>> +static void _tpdm_disable(struct tpdm_drvdata *drvdata)
>> +{
>> +       CS_UNLOCK(drvdata->base);
>> +
>> +       /* Check if DSB datasets is present for TPDM. */
>> +       if (test_bit(TPDM_DS_DSB, drvdata->datasets))
>> +               tpdm_disable_dsb(drvdata);
>> +
>> +       CS_LOCK(drvdata->base);
>> +
>> +}
>> +
>>   static void tpdm_disable(struct coresight_device *csdev,
>>                           struct perf_event *event)
>>   {
>> @@ -51,6 +95,7 @@ static void tpdm_disable(struct coresight_device *csdev,
>>                  return;
>>          }
>>
>> +       _tpdm_disable(drvdata);
>>          drvdata->enable = false;
>>          mutex_unlock(&drvdata->lock);
>>
>> @@ -66,6 +111,21 @@ static const struct coresight_ops tpdm_cs_ops = {
>>          .source_ops     = &tpdm_source_ops,
>>   };
>>
>> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata)
>> +{
>> +       int i;
>> +       u32 pidr;
>> +
>> +       CS_UNLOCK(drvdata->base);
>> +       /*  Get the datasets present on the TPDM. */
>> +       pidr = readl_relaxed(drvdata->base + CORESIGHT_PERIPHIDR0);
>> +       for (i = 0; i < TPDM_DATASETS; i++) {
>> +               if (pidr & BIT(i))
>> +                       __set_bit(i, drvdata->datasets);
> Could this be considerably simpified? - there are a maximum of 7 bits
> for datasets - and you are setting the same bit in drvdata->datasets
> for each bit in pidr
>
> e.g. if the datasets declaration is a simple unsigned long, then the
> following is easier to read and understand:
>
> drvdata->datasets |= pidr & GENMASK(TPDM_DATASETS - 1, 0);
>
> The tests above could then become:
> if (drvdata->datasets & BIT(TPDM_DS_DSB)) ...
>
I will check your suggestion and update in next version.
>> +       }
>> +       CS_LOCK(drvdata->base);
>> +}
>> +
>>   static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>>   {
>>          struct device *dev = &adev->dev;
>> @@ -104,6 +164,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
>>          if (IS_ERR(drvdata->csdev))
>>                  return PTR_ERR(drvdata->csdev);
>>
>> +       tpdm_init_default_data(drvdata);
>>          /* Decrease pm refcount when probe is done.*/
>>          pm_runtime_put(&adev->dev);
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h
>> index 94a7748a5426..8f05070879c4 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -6,6 +6,25 @@
>>   #ifndef _CORESIGHT_CORESIGHT_TPDM_H
>>   #define _CORESIGHT_CORESIGHT_TPDM_H
>>
>> +/* The max number of the datasets that TPDM supports */
>> +#define TPDM_DATASETS       7
>> +
>> +/* DSB Subunit Registers */
>> +#define TPDM_DSB_CR            (0x780)
>> +
> declare the enable bit here with comment documenting usage  - see above.
>
>> +/**
>> + * This enum is for PERIPHIDR0 register of TPDM.
>> + * The fields [6:0] of PERIPHIDR0 are used to determine what
>> + * interfaces and subunits are present on a given TPDM.
>> + *
>> + * PERIPHIDR0[0] : Fix to 1 if ImplDef subunit present, else 0
>> + * PERIPHIDR0[1] : Fix to 1 if DSB subunit present, else 0
>> + */
>> +enum tpdm_dataset {
>> +       TPDM_DS_IMPLDEF,
>> +       TPDM_DS_DSB,
>> +};
>> +
>>   /**
>>    * struct tpdm_drvdata - specifics associated to an TPDM component
>>    * @base:       memory mapped base address for this component.
>> @@ -13,6 +32,7 @@
>>    * @csdev:      component vitals needed by the framework.
>>    * @lock:       lock for the enable value.
>>    * @enable:     enable status of the component.
>> + * @datasets:   The datasets types present of the TPDM.
>>    */
>>
>>   struct tpdm_drvdata {
>> @@ -21,6 +41,7 @@ struct tpdm_drvdata {
>>          struct coresight_device *csdev;
>>          struct mutex            lock;
>>          bool                    enable;
>> +       DECLARE_BITMAP(datasets, TPDM_DATASETS);
> Could this simply be declared as an unsigned long? - then simplify the
> code in the .c file.
>
> Regards
>
> Mike
>
>>   };
>>
>>   #endif  /* _CORESIGHT_CORESIGHT_TPDM_H */
>> --
>> 2.17.1
>>
>
Mao Jinlong April 20, 2022, 2:42 p.m. UTC | #7
On 4/19/2022 5:25 PM, Mike Leach wrote:
> Hi
>
> On Tue, 12 Apr 2022 at 13:51, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>> Add API usage document for sysfs API in TPDM driver.
>>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   .../ABI/testing/sysfs-bus-coresight-devices-tpdm    | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> new file mode 100644
>> index 000000000000..d70ba429f38d
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>> @@ -0,0 +1,13 @@
>> +What:          /sys/bus/coresight/devices/<tpdm-name>/integration_test
>> +Date:          April 2022
>> +KernelVersion  5.18
>> +Contact:       Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
>> +Description:
>> +               (Write) Run integration test for tpdm. Integration test
>> +               will generate test data for tpdm. It can help to make
>> +               sure that the trace path is enabled and the link configurations
>> +               are fine.
>> +
>> +               value to this sysfs node:
>> +               1 : Genreate 64 bits data
> s/Genreate/Generate
I will update this.
>
>> +               2 : Generate 32 bits data
>> --
>> 2.17.1
>>
> Reviewed by: Mike Leach <mike.leach@linaro.org>
>
>
Mao Jinlong April 20, 2022, 2:43 p.m. UTC | #8
Hi Mike,

On 4/19/2022 5:51 PM, Mike Leach wrote:
> Hi
>
> On Tue, 12 Apr 2022 at 13:51, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>> TPDA(Trace, Profiling and Diagnostics Aggregator) is
>> to provide packetization, funneling and timestamping of
>> TPDM data. Multiple monitors are connected to different
>> input ports of TPDA.This change is to add tpda
>> enable/disable/probe functions for coresight tpda driver.
>>
>>   - - - -         - - - -        - - - -
>> | TPDM 0|      | TPDM 1 |     | TPDM 2|
>>   - - - -         - - - -        - - - -
>>      |               |             |
>>      |_ _ _ _ _ _    |     _ _ _ _ |
>>                  |   |    |
>>                  |   |    |
>>             ------------------
>>            |        TPDA      |
>>             ------------------
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/Kconfig          |  11 ++
>>   drivers/hwtracing/coresight/Makefile         |   1 +
>>   drivers/hwtracing/coresight/coresight-tpda.c | 192 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpda.h |  32 ++++
>>   4 files changed, 236 insertions(+)
>>   create mode 100644 drivers/hwtracing/coresight/coresight-tpda.c
>>   create mode 100644 drivers/hwtracing/coresight/coresight-tpda.h
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index 60248fef4089..317c5e7f4819 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -223,4 +223,15 @@ config CORESIGHT_TPDM_INTEGRATION_TEST
>>            operation to facilitate integration testing and software bringup
>>            and/or to instrument topology discovery. The TPDM utilizes integration
>>            mode to accomplish integration testing and software bringup.
>> +
>> +config CORESIGHT_TPDA
>> +       tristate "CoreSight Trace, Profiling & Diagnostics Aggregator driver"
>> +       help
>> +         This driver provides support for configuring aggregator. This is
>> +         primarily useful for pulling the data sets from one or more
>> +         attached monitors and pushing the resultant data out. Multiple
>> +         monitors are connected on different input ports of TPDA.
>> +
>> +         To compile this driver as a module, choose M here: the module will be
>> +         called coresight-tpda.
>>   endif
> TDPA / TDPM are functionally linked - it does not make sense to
> configure one without the other.
> Kernel configuration should reflect this - as a minimum TDPM
> configuration should select TDPA.
>
>
>
>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> index 6bb9b1746bc7..1712d82e7260 100644
>> --- a/drivers/hwtracing/coresight/Makefile
>> +++ b/drivers/hwtracing/coresight/Makefile
>> @@ -26,5 +26,6 @@ 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
>> +obj-$(CONFIG_CORESIGHT_TPDA) += coresight-tpda.o
>>   coresight-cti-y := coresight-cti-core.o        coresight-cti-platform.o \
>>                     coresight-cti-sysfs.o
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
>> new file mode 100644
>> index 000000000000..9519990c68e2
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -0,0 +1,192 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/amba/bus.h>
>> +#include <linux/bitmap.h>
>> +#include <linux/coresight.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/fs.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "coresight-priv.h"
>> +#include "coresight-tpda.h"
>> +#include "coresight-trace-id.h"
>> +
>> +DEFINE_CORESIGHT_DEVLIST(tpda_devs, "tpda");
>> +
>> +/* Settings pre enabling port control register */
>> +static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>> +{
>> +       u32 val;
>> +
>> +       val = readl_relaxed(drvdata->base + TPDA_CR);
>> +       val |= (drvdata->atid << 6);
>> +       writel_relaxed(val, drvdata->base + TPDA_CR);
>> +}
>> +
>> +static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>> +{
>> +       u32 val;
>> +
>> +       val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
>> +       /* Enable the port */
>> +       val = val | BIT(0);
> #define a constant in the header to use here.
I will update it.
>
>> +       writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
>> +}
>> +
>> +static void _tpda_enable(struct tpda_drvdata *drvdata, int port)
>> +{
>> +       CS_UNLOCK(drvdata->base);
>> +
>> +       if (!drvdata->enable)
>> +               tpda_enable_pre_port(drvdata);
>> +
>> +       tpda_enable_port(drvdata, port);
>> +
>> +       CS_LOCK(drvdata->base);
>> +}
>> +
>> +static int tpda_enable(struct coresight_device *csdev, int inport, int outport)
>> +{
>> +       struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +       mutex_lock(&drvdata->lock);
>> +       _tpda_enable(drvdata, inport);
>> +       drvdata->enable = true;
>> +       mutex_unlock(&drvdata->lock);
>> +
>> +       dev_info(drvdata->dev, "TPDA inport %d enabled\n", inport);
>> +       return 0;
>> +}
>> +
>> +static void _tpda_disable(struct tpda_drvdata *drvdata, int port)
>> +{
>> +       u32 val;
>> +
>> +       CS_UNLOCK(drvdata->base);
>> +
>> +       val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port));
>> +       val = val & ~BIT(0);
> use a #defined constant.
>
>> +       writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port));
>> +
>> +       CS_LOCK(drvdata->base);
>> +}
>> +
>> +static void tpda_disable(struct coresight_device *csdev, int inport,
>> +                          int outport)
>> +{
>> +       struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +       mutex_lock(&drvdata->lock);
>> +       _tpda_disable(drvdata, inport);
>> +       drvdata->enable = false;
>> +       mutex_unlock(&drvdata->lock);
>> +
>> +       dev_info(drvdata->dev, "TPDA inport %d disabled\n", inport);
>> +}
>> +
>> +static const struct coresight_ops_link tpda_link_ops = {
>> +       .enable         = tpda_enable,
>> +       .disable        = tpda_disable,
>> +};
>> +
>> +static const struct coresight_ops tpda_cs_ops = {
>> +       .link_ops       = &tpda_link_ops,
>> +};
>> +
>> +static int tpda_init_default_data(struct tpda_drvdata *drvdata)
>> +{
>> +       int atid;
>> +       /*
>> +        * TPDA must has a unique atid. This atid can uniquely
>> +        * identify the TPDM trace source connect to the TPDA.
>> +        */
>> +       atid = coresight_trace_id_get_system_id(coresight_get_trace_id_map());
>> +       if (atid < 0)
>> +               return atid;
>> +
>> +       drvdata->atid = atid;
>> +       return 0;
>> +}
>> +
>> +static int tpda_probe(struct amba_device *adev, const struct amba_id *id)
>> +{
>> +       int ret;
>> +       struct device *dev = &adev->dev;
>> +       struct coresight_platform_data *pdata;
>> +       struct tpda_drvdata *drvdata;
>> +       struct coresight_desc desc = { 0 };
>> +
>> +       pdata = coresight_get_platform_data(dev);
>> +       if (IS_ERR(pdata))
>> +               return PTR_ERR(pdata);
>> +       adev->dev.platform_data = pdata;
>> +
>> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> +       if (!drvdata)
>> +               return -ENOMEM;
>> +
>> +       drvdata->dev = &adev->dev;
>> +       dev_set_drvdata(dev, drvdata);
>> +
>> +       drvdata->base = devm_ioremap_resource(dev, &adev->res);
>> +       if (!drvdata->base)
>> +               return -ENOMEM;
>> +
>> +       mutex_init(&drvdata->lock);
>> +
>> +       ret = tpda_init_default_data(drvdata);
>> +       if (ret)
>> +               return ret;
>> +
>> +       desc.name = coresight_alloc_device_name(&tpda_devs, dev);
>> +       if (!desc.name)
>> +               return -ENOMEM;
>> +       desc.type = CORESIGHT_DEV_TYPE_LINK;
>> +       desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
>> +       desc.ops = &tpda_cs_ops;
>> +       desc.pdata = adev->dev.platform_data;
>> +       desc.dev = &adev->dev;
>> +       drvdata->csdev = coresight_register(&desc);
>> +       if (IS_ERR(drvdata->csdev))
>> +               return PTR_ERR(drvdata->csdev);
>> +
>> +       pm_runtime_put(&adev->dev);
>> +
>> +       dev_dbg(drvdata->dev, "TPDA initialized\n");
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Different TPDA has different periph id.
>> + * The difference is 0-7 bits' value. So ignore 0-7 bits.
>> + */
>> +static struct amba_id tpda_ids[] = {
>> +       {
>> +               .id     = 0x000f0f00,
>> +               .mask   = 0x000fff00,
>> +       },
>> +       { 0, 0},
>> +};
>> +
>> +static struct amba_driver tpda_driver = {
>> +       .drv = {
>> +               .name   = "coresight-tpda",
>> +               .owner  = THIS_MODULE,
>> +               .suppress_bind_attrs = true,
>> +       },
>> +       .probe          = tpda_probe,
>> +       .id_table       = tpda_ids,
>> +};
>> +
> There is no code to release resources when this module is unloaded.
> You need a .remove function, that as a minimum calls
> coresight_unregister().
>
> Regards
>
> Mike
Yes. Need add remove function for tpda driver.
>
>> +module_amba_driver(tpda_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Aggregator driver");
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
>> new file mode 100644
>> index 000000000000..6ac33b9c1ea4
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef _CORESIGHT_CORESIGHT_TPDA_H
>> +#define _CORESIGHT_CORESIGHT_TPDA_H
>> +
>> +#define TPDA_CR                        (0x000)
>> +#define TPDA_Pn_CR(n)          (0x004 + (n * 4))
>> +
>> +#define TPDA_MAX_INPORTS       32
>> +
>> +/**
>> + * struct tpda_drvdata - specifics associated to an TPDA component
>> + * @base:       memory mapped base address for this component.
>> + * @dev:        The device entity associated to this component.
>> + * @csdev:      component vitals needed by the framework.
>> + * @lock:       lock for the enable value.
>> + * @enable:     enable status of the component.
>> + * @traceid:    trace source identification for the data packet by TPDA.
>> + */
>> +struct tpda_drvdata {
>> +       void __iomem            *base;
>> +       struct device           *dev;
>> +       struct coresight_device *csdev;
>> +       struct mutex            lock;
>> +       bool                    enable;
>> +       u32                     atid;
>> +};
>> +
>> +#endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
>> --
>> 2.17.1
>>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK