Re: [PATCH v1 1/7] dt-bindings: arm: Add support for Coresight TGU trace

[Krzysztof Kozlowski <krzk@kernel.org>]

On 02/09/2024 05:14, songchai wrote:
> 
> On 8/30/2024 6:11 PM, Krzysztof Kozlowski wrote:
>> On 30/08/2024 11:23, songchai wrote:
>>> The Trigger Generation Unit (TGU) is designed to detect patterns or
>>> sequences within a specific region of the System on Chip (SoC). Once
>>> configured and activated, it monitors sense inputs and can detect a
>>> pre-programmed state or sequence across clock cycles, subsequently
>>> producing a trigger.
>>>
>>>     TGU configuration space
>>>          offset table
>>>   x-------------------------x
>>>   |                         |
>>>   |                         |
>>>   |                         |                           Step configuration
>>>   |                         |                             space layout
>>>   |   coresight management  |                           x-------------x
>>>   |        registers        |                     |---> |             |
>>>   |                         |                     |     |  reserve    |
>>>   |                         |                     |     |             |
>>>   |-------------------------|                     |     |-------------|
>>>   |                         |                     |     | prioroty[3] |
>>>   |         step[7]         |<--                  |     |-------------|
>>>   |-------------------------|   |                 |     | prioroty[2] |
>>>   |                         |   |                 |     |-------------|
>>>   |           ...           |   |Steps region     |     | prioroty[1] |
>>>   |                         |   |                 |     |-------------|
>>>   |-------------------------|   |                 |     | prioroty[0] |
>>>   |                         |<--                  |     |-------------|
>>>   |         step[0]         |-------------------->      |             |
>>>   |-------------------------|                           |  condition  |
>>>   |                         |                           |             |
>>>   |     control and status  |                           x-------------x
>>>   |           space         |                           |             |
>>>   x-------------------------x                           |Timer/Counter|
>>>                                                         |             |
>>> 						       x-------------x
>>> TGU Configuration in Hardware
>>>
>>> The TGU provides a step region for user configuration, similar
>>> to a flow chart. Each step region consists of three register clusters:
>>>
>>> 1.Priority Region: Sets the required signals with priority.
>>> 2.Condition Region: Defines specific requirements (e.g., signal A
>>> reaches three times) and the subsequent action once the requirement is
>>> met.
>>> 3.Timer/Counter (Optional): Provides timing or counting functionality.
>>>
>>> Add a new coresight-tgu.yaml file to describe the bindings required to
>>> define the TGU in the device trees.
>>>
>>> Signed-off-by: songchai<quic_songchai@quicinc.com>
>> It feels like you are using login name as real name. Please investigate
>> this and confirm whether latin transcription/transliteration of your
>> name is like above.
> yes.. it's my login name. Will use my real name in next version.
>>
>>> ---
>>>   .../bindings/arm/qcom,coresight-tgu.yaml      | 136 ++++++++++++++++++
>>>   1 file changed, 136 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml
>>> new file mode 100644
>>> index 000000000000..c261252e33e0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml
>>> @@ -0,0 +1,136 @@
>>> +# 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-tgu.yaml#
>>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Trigger Generation Unit - TGU
>>> +
>>> +description: |
>>> +  The Trigger Generation Unit (TGU) is a Data Engine which can be utilized
>>> +  to sense a plurality of signals and create a trigger into the CTI or
>>> +  generate interrupts to processors. The TGU is like the trigger circuit
>>> +  of a Logic Analyzer.The corresponding trigger logic can be realized by
>>> +  configuring the conditions for each step after sensing the signal.
>>> +  Once setup and enabled, it will observe sense inputs and based upon
>>> +  the activity of those inputs, even over clock cycles, may detect a
>>> +  preprogrammed state/sequence and then produce a trigger or interrupt.
>>> +
>>> +  The primary use case of the TGU is to detect patterns or sequences on a
>>> +  given set of signals within some region of the SoC.
>>> +
>>> +maintainers:
>>> +  - Mao Jinlong<quic_jinlmao@quicinc.com>
>>> +  - Sam Chai<quic_songchai@quicinc.com>
>>> +
>>> +# Need a custom select here or 'arm,primecell' will match on lots of nodes
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        enum:
>>> +          - qcom,coresight-tgu
>>> +  required:
>>> +    - compatible
>>> +
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^tgu(@[0-9a-f]+)$"
>> Drop the pattern (and anyway @ is not optional).
> 
> There will be several TGUs in the SoC, and we want to use the address of 
> each to distinguish them.

How is this related?

> 
> This is why we added the nodename pattern here.

How is this related?

> 
> Additionally, I noticed that both TPDM and CTI also use this format to 
> define the nodename.
> 
> Could you please share any concerns you have about using this pattern?

I don't get why you insist, but sure:
The name does not follow DT spec recommendation, plus child schema is
not really supposed to define the names.

> 
>>
>>> +  compatible:
>>> +    items:
>>> +      - const: qcom,coresight-tgu
>>> +      - const: arm,primecell
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: apb_pclk
>>> +
>>> +  qcom,tgu-steps:
>>> +    description:
>>> +      The trigger logic is realized by configuring each step after sensing
>>> +      the signal. The parameter here is used to describe the maximum of steps
>>> +      that could be configured in the current TGU.
>> Why this is board or SoC level property? All below also feel like
>> unnecessary stuff from downstream.
> 
> There are actually four properties used to describe the number of 
> registers with different functionality for TGUs at the SoC level.
> 
> Each TGU may have a different number of registers, so we need to add 
> these four properties to describe each TGU’s hardware design.

Each TGU on the same SoC?



Best regards,
Krzysztof