Messages in this thread | | | Date | Tue, 28 Mar 2023 19:29:48 +0800 | Subject | Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema | From | Hao Zhang <> |
| |
Hi Mike,
On 3/28/2023 6:12 PM, Mike Leach wrote: > Hi, > > As per my comments in the previous patch in this set.... > > On Mon, 27 Mar 2023 at 08:38, Hao Zhang <quic_hazha@quicinc.com> wrote: >> >> Hi Krzysztof, >> >> On 3/25/2023 7:49 PM, Krzysztof Kozlowski wrote: >>> On 24/03/2023 07:16, Hao Zhang wrote: >>>> Add new coresight-dummy.yaml file describing the bindings required >>>> to define coresight dummy trace in the device trees. >>>> >>> >>> Subject: drop second/last, redundant "YAML schema". The "dt-bindings" >>> prefix is already stating that these are bindings and all new must be DT >>> schema. You cannot add anything else, so this is redundant. >>> >> I will take your advice to drop redundant part of title in the next >> version of patch. >>> >>>> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com> >>>> --- >>>> .../bindings/arm/qcom,coresight-dummy.yaml | 118 ++++++++++++++++++ >>>> 1 file changed, 118 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml >>>> new file mode 100644 >>>> index 000000000000..7b719b084d72 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml >>>> @@ -0,0 +1,118 @@ >>>> +# 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-dummy.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: QCOM Coresight Dummy component >>>> + >>>> +description: | >>>> + The Coresight Dummy component is for the specific devices that HLOS don't have >>>> + permission to access or configure. Such as Coresight sink EUD, some TPDMs etc. >>>> + So there need driver to register dummy devices as Coresight devices. Provide >>>> + Coresight API for dummy device operations, such as enabling and disabling >>>> + dummy devices. Build the Coresight path for dummy sink or dummy source for >>>> + debugging. >>>> + >>>> + The primary use case of the coresight dummy is to build path for dummy sink or >>>> + dummy source. >>>> + >>>> +maintainers: >>>> + - Mao Jinlong <quic_jinlmao@quicinc.com> >>>> + - Tao Zhang <quic_taozha@quicinc.com> >>>> + - Hao Zhang <quic_hazha@quicinc.com> >>>> + >>>> +select: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + enum: >>>> + - qcom,coresight-dummy > > Can we have coresight-dummy-source and coresight-dummy-sink?
Sure, I will take your advice in the next version of patch.
> >>>> + required: >>>> + - compatible >>> >>> Why do you need the select? >>> >> This is a mistake, will remove it in the next version of patch. >>>> + >>>> +properties: >>>> + $nodename: >>>> + pattern: "^dummy_.*(sink|source)_[0-9]+.*$" >>> >>> We do not enforce node names in individual bindings. Why do you need it? >>> Plus underscore is not even proper character... >>> >> I will remove this node. >> >>>> + compatible: >>>> + items: >>> >>> Drop items. You have only one item, so no need for list. >> >> I will take your advice and update it in the next version of patch. >> >>>> + - const: qcom,coresight-dummy >>>> + >>>> + qcom,dummy-sink: >>>> + type: boolean >>>> + description: >>>> + Indicates that the type of this coresight node is dummy sink. >>> >>> You just duplicated property name. Write something useful. >>> >>>> + >>>> + qcom,dummy-source: >>>> + type: boolean >>>> + description: >>>> + Indicates that the type of this coresight node is dummy source. >>> >>> You just duplicated property name. Write something useful. >>> >> > > These properties not required if the compatible name is more specific >
I will update it in the next version of patch.
>> Sure, I will add more details for it. >> >>>> + >>>> + out-ports: >>>> + description: | >>> >>> No need for | >>> >>>> + Output connections from the dummy source to Coresight Trace bus. >>>> + $ref: /schemas/graph.yaml#/properties/ports >>>> + >>>> + properties: >>>> + port: >>>> + description: Output connection from the dummy source to Coresight >>>> + Trace bus. >>>> + $ref: /schemas/graph.yaml#/properties/port >>>> + >>>> + in-ports: >>>> + description: | >>> >>> Ditto >>> >> I will remove it in the next version of patch. >> >>>> + Input connections from the CoreSight Trace bus to dummy sink. >>>> + $ref: /schemas/graph.yaml#/properties/ports >>>> + >>>> + properties: >>>> + port: >>>> + description: Input connection from the Coresight Trace bus to >>>> + dummy sink. >>>> + $ref: /schemas/graph.yaml#/properties/port >>>> + >>>> +required: >>>> + - compatible >>>> + > > The binding should constrain out ports to dummy-source only, and in > ports to dummy sink only. > > Regards > > Mike >
I will update it according to your advice in the next version of patch.
Thanks, Hao
>>>> +additionalProperties: false >>>> + >>>> +oneOf: >>>> + - required: >>>> + - qcom,dummy-sink >>>> + - required: >>>> + - qcom,dummy-source >>>> + >>>> +examples: >>>> + # minimum dummy sink definition. dummy sink connect to coresight replicator. >>>> + - | >>>> + dummy_sink_1 { >>> >>> Node names should be generic, so "sink" >>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >>> >>>> + compatible = "qcom,coresight-dummy"; >>>> + qcom,dummy-sink; >>>> + >>>> + in-ports { >>>> + port { >>>> + eud_in_replicator_swao: endpoint { >>>> + remote-endpoint = >>>> + <&replicator_swao_out_eud>; >>> >>> Why line break after =? >>> >> >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> + >>>> + # minimum dummy source definition. dummy source connect to coresight funnel. >>> >>> If you use sentences, then start with capital letter. >>> >> >> I will update it according to your advice in the next version of patch. >> >>>> + - | >>>> + dummy_source_1 { >>>> + compatible = "qcom,coresight-dummy"; >>>> + qcom,dummy-source; >>>> + >>>> + out-ports { >>>> + port { >>>> + dummy_riscv_out_funnel_swao: endpoint { >>>> + remote-endpoint = >>>> + <&funnel_swao_in_dummy_riscv>; >>> >>> Why line break? >> >> I copy it from device tree and keep the original format, will correct >> the format in the next version of patch. >> >> Thanks, >> Hao >> >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> + >>>> +... >>> >>> Best regards, >>> Krzysztof >>> > > >
| |