Re: [PATCH v2 2/3] dt-bindings: arm: Adds CoreSight CSR hardware definitions
From: Krzysztof Kozlowski
Date: Tue Aug 22 2023 - 02:04:48 EST
On 13/08/2023 17:12, Mao Jinlong wrote:
> Adds new coresight-csr.yaml file describing the bindings required
> to define csr in the device trees.
>
> Signed-off-by: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
> ---
> .../bindings/arm/qcom,coresight-csr.yaml | 130 ++++++++++++++++++
> MAINTAINERS | 2 +-
> include/dt-bindings/arm/coresight-csr-dt.h | 12 ++
> 3 files changed, 143 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-csr.yaml
> create mode 100644 include/dt-bindings/arm/coresight-csr-dt.h
>
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-csr.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-csr.yaml
> new file mode 100644
> index 000000000000..de4baa335fdb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-csr.yaml
> @@ -0,0 +1,130 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/qcom,coresight-csr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CoreSight Slave Register - CSR
> +
> +description: |
> + CoreSight Slave Register block hosts miscellaneous configuration registers.
> + Those configuration registers can be used to control, various coresight
> + configurations.
> +
> +maintainers:
> + - Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
> + - Hao Zhang <quic_hazha@xxxxxxxxxxx>
> +
> +properties:
> + $nodename:
> + pattern: "^csr(@[0-9a-f]+)$"
Blank line. Or even drop the nodename, we do not enforce names in the
individual bindings and I do not get why "csr" should be a recommended
name. It's not really generic, but specific.
> + compatible:
> + items:
> + - const: qcom,coresight-csr
> +
> + reg:
> + minItems: 1
> + maxItems: 2
Why is this flexible? One device has only one register layout... or you
want to say that compatible is not specific but generic?
Anyway, items needs to be described.
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + items:
> + - const: apb_pclk
> +
> + # size cells and address cells required if assoc_device node present.
> + "#size-cells":
> + const: 0
> +
> + "#address-cells":
> + const: 1
> +
> +patternProperties:
> + '^assoc_device@([0-9]+)$':
No underscores.
Aren't you now creating duplicated nodes for devices? ETRs for example
have their device nodes, right? So here you would be creating second
one? If that's the case, then it looks wrong.
> + type: object
> + description:
> + A assocated device child node which describes the required configs
> + between this CSR and another hardware device. This device may be ETR or
> + TPDM device.
> +
> + properties:
> + reg:
> + maxItems: 1
> +
> + arm,cs-dev-assoc:
> + $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 CSR
> + will be enabled. If the associated device has not been registered
> + then the node name will be stored as the assocated name for later
> + resolution.
> +
> + qcom,cs-dev-type:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Device type of the Assocated device. Types are in coresight-csr-dt.h.
> +
> + qcom,csr-bytecntr-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + The ETR irqctrl register offset. If the assocated device is ETR
> + device and there are more than one ETR devices, this property need
> + to be added.
> +
> + interrupts:
> + minItems: 1
> +
> + interrupt-names:
> + minItems: 1
> +
> + required:
> + - reg
> + - qcom,cs-dev-type
> + - qcom,cs-dev-assoc
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> + # minimum CSR definition.
> + - |
> + csr@10001000 {
> + compatible = "qcom,coresight-csr";
> + reg = <0 0x10001000 0 0x1000>;
> +
> + clocks = <&aoss_qmp>;
> + clock-names = "apb_pclk";
> + };
> + # Assocated with ETR device
> + - |
> + #include <dt-bindings/arm/coresight-csr-dt.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + csr@10001000 {
> + compatible = "qcom,coresight-csr";
> + reg = <0 0x10001000 0 0x1000>;
> +
> + clocks = <&aoss_qmp>;
> + clock-names = "apb_pclk";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + assoc_device@0 {
> + reg = <0>;
> + qcom,cs-dev-type = <CSR_ASSOC_DEV_ETR>;
> + qcom,cs-dev-assoc = <&tmc_etr>;
> + qcom,csr-bytecntr-offset = <0x6c>;
> + interrupts = <GIC_SPI 270 IRQ_TYPE_EDGE_RISING>;
> + interrupt-names = "byte-cntr-irq";
> + };
> + };
> +...
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d516295978a4..3ed81a8fd1d0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2042,7 +2042,7 @@ F: Documentation/devicetree/bindings/arm/arm,trace-buffer-extension.yaml
> F: Documentation/devicetree/bindings/arm/qcom,coresight-*.yaml
> F: Documentation/trace/coresight/*
> F: drivers/hwtracing/coresight/*
> -F: include/dt-bindings/arm/coresight-cti-dt.h
> +F: include/dt-bindings/arm/coresight-*.h
> F: include/linux/coresight*
> F: samples/coresight/*
> F: tools/perf/arch/arm/util/auxtrace.c
> diff --git a/include/dt-bindings/arm/coresight-csr-dt.h b/include/dt-bindings/arm/coresight-csr-dt.h
> new file mode 100644
> index 000000000000..804b9bbeb2bd
> --- /dev/null
> +++ b/include/dt-bindings/arm/coresight-csr-dt.h
Use the same naming pattern as for bindings, so qcom,coresight-csr if it
is qcom, or arm,coresight-csr if it is ARM. DT is for sure redundant.
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * This header provides constants for the defined device
> + * types on CoreSight CSR.
> + */
> +
> +#ifndef _DT_BINDINGS_ARM_CORESIGHT_CSR_DT_H
> +#define _DT_BINDINGS_ARM_CORESIGHT_CSR_DT_H
> +
> +#define CSR_ASSOC_DEV_ETR 1
> +
> +#endif /*_DT_BINDINGS_ARM_CORESIGHT_CSR_DT_H */
Best regards,
Krzysztof