Message-ID: <e365b6da-f7f3-4a77-9ebc-a5db349bedbb@kernel.org>
Date: Tue, 3 Sep 2024 13:30:59 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: songchai <quic_songchai@...cinc.com>,
 Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
 <mike.leach@...aro.org>, James Clark <james.clark@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Andy Gross <agross@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
 Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: linux-kernel@...r.kernel.org, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v1 1/7] dt-bindings: arm: Add support for Coresight TGU
 trace

On 03/09/2024 05:16, songchai wrote:
> 
> On 9/2/2024 6:24 PM, Krzysztof Kozlowski wrote:
>> On 02/09/2024 12:10, songchai wrote:
>>> On 9/2/2024 4:05 PM, Krzysztof Kozlowski wrote:
>>>> On 02/09/2024 09:24, songchai wrote:
>>>>> On 9/2/2024 3:02 PM, Krzysztof Kozlowski wrote:
>>>>>> 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@...cinc.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@...cinc.com>
>>>>>>>>> +  - Sam Chai<quic_songchai@...cinc.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.
>>>>> Thanks for you clarification, will drop in the next version.
>>>>>>>>> +  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?
>>>>> yes, in other words, there will be several TGUs in the SoC.
>>>> This I understood, but I am asking if each TGU on the same SoC will have
>>>> different number of registers and other properties?
>>> yes, each TGU has a different number of registers.
>>>
>>> For example, some TGUs support 6 steps, while others support only 4 steps.
>>>
>>> These variations depend on the hardware design, so we need properties to
>>> describe.
>> You keep avoiding the answer. ON THE SAME SOC.
>>
>> Point to your upstream DTS using this.
> 
> I reviewed the hardware design of our chipset and found the following:
> 
>   * In the SM8450, there are three TGUs on the same SoC. However, only
>     one TGU has timer and counter registers to support time/counter
>     functionality.
>   * Additionally, in the SM8450, the “tgu-regs” configuration varies:
>     some TGUs have 8 registers, while others have only 5 registers.
> 
> To answer question - yes, on the same soc, will have different number of 
> registers and other properties.

Thank you, this is valid reason for the property.

Best regards,
Krzysztof

Powered by blists - more mailing lists