From: Mike Leach <mike.leach@linaro.org>
To: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com>
Cc: Coresight ML <coresight@lists.linaro.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
devicetree@vger.kernel.org,
"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
Mathieu Poirier <mathieu.poirier@linaro.org>
Subject: Re: [PATCH v5 05/14] dt-bindings: arm: Adds CoreSight CTI hardware definitions.
Date: Fri, 29 Nov 2019 13:57:24 +0000 [thread overview]
Message-ID: <CAJ9a7Vj2gGBhZrkGqx+3caDiRuZ4VosRMgVB8B7mKydJtW_=Qw@mail.gmail.com> (raw)
In-Reply-To: <f2150b39-c662-4eea-a556-27d8ebb51735@arm.com>
Hi Suzuki,
On Thu, 28 Nov 2019 at 18:38, Suzuki Kuruppassery Poulose
<suzuki.poulose@arm.com> wrote:
>
> Hi Mike,
>
> On 19/11/2019 23:19, Mike Leach wrote:
> > Adds new coresight-cti.yaml file describing the bindings required to define
> > CTI in the device trees.
> >
> > Adds an include file to dt-bindings/arm to define constants describing
> > common signal functionality used in CoreSight and generic usage.
>
> The documentation looks really nice and helpful. Some very minor nits
> below.
>
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
>
> > ---
> > .../bindings/arm/coresight-cti.yaml | 303 ++++++++++++++++++
> > .../devicetree/bindings/arm/coresight.txt | 7 +
> > MAINTAINERS | 2 +
> > include/dt-bindings/arm/coresight-cti-dt.h | 37 +++
> > 4 files changed, 349 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/arm/coresight-cti.yaml
> > create mode 100644 include/dt-bindings/arm/coresight-cti-dt.h
> >
> > diff --git a/Documentation/devicetree/bindings/arm/coresight-cti.yaml b/Documentation/devicetree/bindings/arm/coresight-cti.yaml
> > new file mode 100644
> > index 000000000000..882c72f1c798
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/coresight-cti.yaml
> > @@ -0,0 +1,303 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright 2019 Linaro Ltd.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/coresight-cti.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM Coresight Cross Trigger Interface (CTI) device.
> > +
> > +description: |
> > + The CoreSight Embedded Cross Trigger (ECT) consists of CTI devices connected
> > + to one or more CoreSight components and/or a CPU, with CTIs interconnected in
> > + a star topology via the CTM (which is not programmable). The ECT components
>
> nit: CTM is not expanded anywhere here. For the sake of completeness,
> you may do that here.
>
> i.e, s/"a start topology via the CTM"/"a start topology via the Cross
> Trigger Matrix (CTM)"/
>
Agreed - I think the expansion got lost in the many re-arrangement of this doc.
>
> > + are not part of the trace generation data path and are thus not part of the
> > + CoreSight graph described in the general CoreSight bindings file
> > + coresight.txt.
> > +
> > + The CTI component properties define the connections between the individual
> > + CTI and the components it is directly connected to, consisting of input and
> > + output hardware trigger signals. CTIs can have a maximum number of input and
> > + output hardware trigger signals (8 each for v1 CTI, 32 each for v2 CTI). The
> > + number is defined at design time, the maximum of each defined in the DEVID
> > + register.
> > +
> > + CTIs are interconnected in a star topology via the CTM, using a number of
> > + programmable channels usually 4, but again implementation defined and
>
> nit: "programmable channels, usually 4, but..." ?
>
OK
> > + described in the DEVID register. The star topology is not required to be
> > + described in the bindings as the actual connections are software
> > + programmable.
> > +
> > + In general the connections between CTI and components via the trigger signals
> > + are implementation defined, other than when v8 core and ETM is present.
>
> nite: are implementation defined, *except when they are connected to an
> Arm v8 compatible CPU or an ETM* ?
>
>
Agreed - clearer.
> > + The v8 architecture defines the required signal connections between CPU core
>
> nit: "The Arm v8"
>
> > + and CTI, and ETM and CTI, if the ETM if present.
> > +
> > + When only minimal information is available for the CTI trigger connections,
> > + then a minimal driver binding can be declare with no explicit trigger
> > + signals. This will result in the using the DEVID register to set the
> > + input and output triggers and channels in use. Any user / client
> > + application will require additional information on the connections
> > + between the CTI and other components for correct operation. This minimal
> > + binding may be used when using the Integration Control registers to
> > + discover connections between CTI and other CoreSight components,
>
> How about "When the CTI trigger connection information is unavailable,
> the driver detects the number of triggers and channels from the DEVID
> register and makes them available. The Integration Control registers
> can be then used to discover the connections for this CTI device
> to other CoreSight components".
>
> Since we recommend the use of the "Integration Control registers", which
> is not normally available unless you play around the code, it will be a
> good idea to metion, what the user needs to do to make the registers
> available. (One more reason to use the CONFIG symbol, makes that
> easier.)
>
Agreed - need to explain the use case for this and implications of
using a minimal binding.
>
> > +
> > + Certain triggers between CoreSight devices and the CTI have specific types
> > + and usages. These can be defined along with the signal indexes with the
> > + constants defined in <dt-bindings/arm/coresight-cti-dt.h>
> > +
> > + For example a CTI connected to a core will usually have a DBGREQ signal. This
> > + is defined in the binding as type PE_EDBGREQ. These types will appear in an
> > + optional array alongside the signal indexes. Omitting types will default all
> > + signals to GEN_IO.
> > +
> > + Note that some hardware trigger signals can be connected to non-CoreSight
> > + components (e.g. UART etc) depending on hardware implementation.
> > +
> > +maintainers:
> > + - Mike Leach <mike.leach@linaro.org>
> > +
> > +allOf:
> > + - $ref: /schemas/arm/primecell.yaml#
> > +
> > +# Need a custom select here or 'arm,primecell' will match on lots of nodes
> > +select:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - arm,coresight-cti
> > + required:
> > + - compatible
> > +
> > +properties:
> > + $nodename:
> > + pattern: "^cti(@[0-9a-f,]+)*$"
> > + compatible:
> > + items:
> > + - const: arm,coresight-cti
> > + - const: arm,primecell
> > +
> > + reg:
> > + items:
> > + - description: device programming registers
> > +
> > + arm,cti-v8-arch:
>
> If possible, please could we make this :
>
> "arm,cti-arm-v8-architected"
>
> to be more meaning ful ?
>
Per my comments to Rob, the v8-cti case will be a compatible option,
so this will disappear as an attribute.
> > + type: boolean
> > + description:
> > + This CTI follows the v8 architecturally mandated layout for a CTI.
> > + Bindings declaring this must declare a cpu, and optionally a single
> > + arm,cs-dev-assoc may be present to define an attached ETM. No additional
> > + trig-conns nodes are permitted. The driver will build a connection model
> > + according to architectural requirements. This will include a filter on
> > + the CPU dbgreq signal as described above.
> > +
> > + cpu:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/phandle
> > + description: Handle to cpu this device is associated with.
> > +
> > + arm,cti-ctm-id:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + Defines the CTM this CTI is connected to, in large systems with multiple
> > + separate CTI/CTM nets. Typically multi-socket systems where the CTM is
> > + propagated between sockets.
> > +
> > + arm,cs-dev-assoc:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/phandle
> > + description:
> > + defines a phandle reference to an associated CoreSight trace device.
> > + When the associated trace device is enabled, then the respective CTI
> > + will be enabled. Use in a trig-conns node, or in CTI base node when
> > + arm,cti-v8-arch present. If the associated device has not been registered
> > + then the node name will be stored as the connection name for later
> > + resolution. If the associated device is not a CoreSight device or not
> > + registered then the node name will remain the connection name and
> > + automatic enabling will not occur.
> > +
> > +patternProperties:
> > + '^trig_conns@[0-9]+$':
>
> I understand these bindings have been around for quite long and is too
> late to make any changes. So, feel free to ignore this suggestion below
> and I am perfectly fine with it.
>
> --- Begin silly comments Or Skip to DONE ----
>
> Could we make the property names a bit more obvious ? Since they are
> supposed to be written by other people (unlike our variable names), it
> always makes sense to have expanded, meaningful names:
>
> s/trig_conns@/triggers@ ?
>
> s/arm,trig-{in,out}-sigs/arm,cti-{in,out}-triggers
> s/arm,trig-{in,out}-types/arm,cti-{in,out}-trigger-types
>
> "arm,trig-xxx" property name doesn't really imply that it is for cti.
> So, the above changes makes it explicit and more reader friendly.
>
> > + type: object
> > + description:
> > + A trigger connections child node which describes the trigger signals
> > + between this CTI and another hardware device. This device may be a CPU,
> > + CoreSight device, any other hardware device or simple external IO lines.
> > + The connection may have both input and output triggers, or only one or the
> > + other.
> > +
> > + properties:
> > +
> > + arm,trig-in-sigs:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 1
> > + maxItems: 32
> > + description:
> > + List of CTI trigger in signal numbers in use by a trig-conns node.
> > +
> > + arm,trig-in-types:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 1
> > + maxItems: 32
> > + description:
> > + List of constants representing the types for the CTI trigger in
> > + signals. Types in this array match to the corresponding signal in the
> > + arm,trig-in-sigs array. If the -types array is smaller, or omitted
> > + completely, then the types will default to GEN_IO.
> > +
> > + arm,trig-out-sigs:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 1
> > + maxItems: 32
> > + description:
> > + List of CTI trigger out signal numbers in use by a trig-conns node.
> > +
> > + arm,trig-out-types:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 1
> > + maxItems: 32
> > + description:
> > + List of constants representing the types for the CTI trigger out
> > + signals. Types in this array match to the corresponding signal
> > + in the arm,trig-out-sigs array. If the "-types" array is smaller,
> > + or omitted completely, then the types will default to GEN_IO.
> > +
> > + arm,trig-filters:
>
> arm,cti-trigger-filters ?
>
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 1
> > + maxItems: 32
> > + description:
> > + List of CTI trigger out signals that will be blocked from becoming
> > + active, unless filtering is disabled on the driver.
> > +
> > + arm,trig-conn-name:
>
> arm,cti-trigger-name ?
>
>
> --- DONE or End of silly comments ---
>
I appreciate the comments on naming, but my feeling is to
differentiate between the trigger signals and the trigger connections
group. hence the sub-node has group trig_cons.
I was hoping that users would get that these are CTI related by the
fact that the root node is cti@...
> > + # Implementation defined CTI - CPU + ETM connections explicitly defined..
> > + # Shows use of type constants from dt-bindings/arm/coresight-cti-dt.h
> > + - |
> > + #include <dt-bindings/arm/coresight-cti-dt.h>
> > +
> > + cti@858000 {
> > + compatible = "arm,coresight-cti", "arm,primecell";
> > + reg = <0x858000 0x1000>;
> > +
> > + clocks = <&soc_smc50mhz>;
> > + clock-names = "apb_pclk";
> > +
> > + arm,cti-ctm-id = <1>;
> > +
> > + trig-conns@0 {
> > + arm,trig-in-sigs = <4 5 6 7>;
> > + arm,trig-in-types = <ETM_EXTOUT
> > + ETM_EXTOUT
> > + ETM_EXTOUT
> > + ETM_EXTOUT>;
> > + arm,trig-out-sigs = <4 5 6 7>;
> > + arm,trig-out-types = <ETM_EXTIN
> > + ETM_EXTIN
> > + ETM_EXTIN
> > + ETM_EXTIN>;
> > + arm,cs-dev-assoc = <&etm0>;
> > + };
> > +
> > + trig-conns@1 {
> > + cpu = <&CPU0>;
> > + arm,trig-in-sigs = <0 1>;
> > + arm,trig-in-types = <PE_DBGTRIGGER
> > + PE_PMUIRQ>;
> > + arm,trig-out-sigs=<0 1 2 >;
> > + arm,trig-out-types = <PE_EDBGREQ
> > + PE_DBGRESTART
> > + PE_CTIIRQ>;
> > +
> > + arm,trig-filters = <0>;
> > + };
> > + };
> > + # Implementation defined CTI - none CoreSight component connections.
>
> nit: s/none/non ?
>
OK.
> Rest looks fine to me.
>
> Suzuki
Thanks
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
next prev parent reply other threads:[~2019-11-29 13:57 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-19 23:18 [PATCH v5 00/14] CoreSight CTI Driver Mike Leach
2019-11-19 23:18 ` [PATCH v5 01/14] coresight: cti: Initial " Mike Leach
2019-11-21 20:21 ` Mathieu Poirier
2019-11-29 12:05 ` Mike Leach
2019-12-03 16:53 ` Mathieu Poirier
2019-11-25 19:03 ` Suzuki Kuruppassery Poulose
2019-11-29 12:06 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 02/14] coresight: cti: Add sysfs coresight mgmt reg access Mike Leach
2019-11-22 17:19 ` Mathieu Poirier
2019-11-19 23:19 ` [PATCH v5 03/14] coresight: cti: Add sysfs access to program function regs Mike Leach
2019-11-27 18:26 ` Suzuki Kuruppassery Poulose
2019-11-29 12:47 ` Mike Leach
2019-11-28 10:54 ` Suzuki Kuruppassery Poulose
2019-11-28 17:20 ` Mathieu Poirier
2019-11-28 18:00 ` Suzuki Kuruppassery Poulose
2019-11-29 12:50 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 04/14] coresight: cti: Add sysfs trigger / channel programming API Mike Leach
2019-11-22 18:40 ` Mathieu Poirier
2019-11-27 18:40 ` Suzuki Kuruppassery Poulose
2019-11-29 13:01 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 05/14] dt-bindings: arm: Adds CoreSight CTI hardware definitions Mike Leach
2019-11-20 19:06 ` Mathieu Poirier
2019-11-20 22:39 ` Mike Leach
2019-11-22 23:33 ` Rob Herring
2019-11-29 13:50 ` Mike Leach
2019-11-29 14:12 ` Suzuki Kuruppassery Poulose
2019-11-28 18:38 ` Suzuki Kuruppassery Poulose
2019-11-29 13:57 ` Mike Leach [this message]
2019-11-19 23:19 ` [PATCH v5 06/14] coresight: cti: Add device tree support for v8 arch CTI Mike Leach
2019-11-25 19:00 ` Mathieu Poirier
2019-11-29 11:33 ` Suzuki Kuruppassery Poulose
2019-12-03 10:59 ` Mike Leach
2019-12-03 11:28 ` Suzuki Kuruppassery Poulose
2019-12-03 12:25 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 07/14] coresight: cti: Add device tree support for custom CTI Mike Leach
2019-11-25 21:22 ` Mathieu Poirier
2019-11-29 14:16 ` Suzuki Kuruppassery Poulose
2019-11-29 21:11 ` Mathieu Poirier
2019-11-29 14:18 ` Suzuki Kuruppassery Poulose
2019-12-03 14:05 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 08/14] coresight: cti: Enable CTI associated with devices Mike Leach
2019-11-25 22:45 ` Mathieu Poirier
2019-12-05 16:33 ` Mike Leach
2019-11-29 18:28 ` Suzuki Kuruppassery Poulose
2019-11-29 21:25 ` Mathieu Poirier
2019-12-05 16:33 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 09/14] coresight: cti: Add connection information to sysfs Mike Leach
2019-11-27 18:09 ` Mathieu Poirier
2019-12-06 16:24 ` Mike Leach
2019-12-02 9:47 ` Suzuki Kuruppassery Poulose
2019-12-06 16:24 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 10/14] dt-bindings: qcom: Add CTI options for qcom msm8916 Mike Leach
2019-11-27 18:18 ` Mathieu Poirier
2019-11-19 23:19 ` [PATCH v5 11/14] dt-bindings: arm: Juno platform - add CTI entries to device tree Mike Leach
2019-11-27 18:25 ` Mathieu Poirier
2019-11-19 23:19 ` [PATCH v5 12/14] dt-bindings: hisilicon: Add CTI bindings for hi-6220 Mike Leach
2019-11-19 23:19 ` [PATCH v5 13/14] docs: coresight: Update documentation for CoreSight to cover CTI Mike Leach
2019-11-27 19:00 ` Mathieu Poirier
2019-12-02 10:43 ` Suzuki Kuruppassery Poulose
2019-12-06 17:39 ` Mike Leach
2019-11-19 23:19 ` [PATCH v5 14/14] docs: sysfs: coresight: Add sysfs ABI documentation for CTI Mike Leach
2019-11-27 19:08 ` Mathieu Poirier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJ9a7Vj2gGBhZrkGqx+3caDiRuZ4VosRMgVB8B7mKydJtW_=Qw@mail.gmail.com' \
--to=mike.leach@linaro.org \
--cc=coresight@lists.linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=suzuki.poulose@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).