Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 11/07/2024 09:36, JieGan wrote:
> On Tue, Jul 09, 2024 at 02:00:25PM +0800, JieGan wrote:
>> On Mon, Jul 08, 2024 at 01:50:11PM +0100, Suzuki K Poulose wrote:
>>> On 08/07/2024 11:25, JieGan wrote:
>>>> On Mon, Jul 08, 2024 at 06:10:28PM +0800, JieGan wrote:
>>>>> On Mon, Jul 08, 2024 at 10:41:55AM +0100, Suzuki K Poulose wrote:
>>>>>> On 05/07/2024 10:00, Jie Gan wrote:
>>>>>>> Add binding document for Coresight Control Unit device.
>>>>>>
>>>>>> nit: This is again too generic ? corsight-tmc-control-unit ? After all
>>>>>> thats what it is and not a *generic* coresight control unit ?
>>>>>>
>>>>> coresight-tmc-control-unit is much better. We will check it.
>>>>>>>
>>>>>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>>>>>> ---
>>>>>>>     .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
>>>>>>>     1 file changed, 87 insertions(+)
>>>>>>>     create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..9bb8ced393a7
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>>>>> @@ -0,0 +1,87 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: CoreSight Control Unit
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
>>>>>>> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>>>> +  - Jie Gan <quic_jiegan@quicinc.com>
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  The Coresight Control unit controls various Coresight behaviors.
>>>>>>> +  Used to enable/disable ETR’s data filter function based on trace ID.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: qcom,coresight-ccu
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  clocks:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  clock-names:
>>>>>>> +    items:
>>>>>>> +      - const: apb_pclk
>>>>>>> +
>>>>>>> +  reg-names:
>>>>>>> +    items:
>>>>>>> +      - const: ccu-base
>>>>>>> +
>>>>>>> +  in-ports:
>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>> +
>>>>>>> +    unevaluatedProperties:
>>>>>>> +      patternProperties:
>>>>>>> +        '^port(@[0-7])?$':
>>>>>>> +          description: Input connections from CoreSight Trace bus
>>>>>>> +          $ref: /schemas/graph.yaml#/properties/port
>>>>>>> +
>>>>>>> +          properties:
>>>>>>> +            qcom,ccu-atid-offset:
>>>>>>
>>>>>> Why do we need this atid offset ? Couldn't this be mapped to the "port"
>>>>>> number ?
>>>>>>
>>>>>> e.g, input-port 0 on CCU => Offset x
>>>>>>        input-port 1 on CCU => (Offset x + Size of 1 region)
>>>>> If the first ATID offset remains constant, it appears to be feasible.
>>>>> We will consider the possibility of this solution.
>>>> We just checked the ATID offset varies across different hardware platforms.
>>>> It defined as 0xf4 on some platforms, and some others defined as 0xf8.
>>>
>>> What do you mean ? The offset where you apply the filter changes across
>>> different platforms ? or different "tmc-control-unit" implementations ?
>>> Is this discoverable from the device ? We could use different
>>> compatibles for different "types" of the "devices". Simply adding
>>> something in the DT is not the right way.
>>
>> I got your point here. I believe we should consult our hardware engineers first to check it.
>> We need to figure out the design of ATID offset from hardware perspective. Then we can
>> propose an alternative approach, such as associating the offset with a compitable value,
>> to resolve this issue.
>>
>>>
>>>>
>>>> So I think it should be better to define it in device tree node.
>>>
>>> No. See above.
> 
> 
> Hi Suzuki
> 
> There is a new solution for CCU device. We would like to separate one CCU device into several helper
> devices, each responsible for one feature of the CCU device.
> 
> For the data filter feature, we will define the address of the AITD Register that included by CCU in DT
> as base address of the helper node. So the driver code can easily program the register field with the base address.
> With this solution, we may need define several helper nodes in DT file to support different features for CCU and each
> helper device needs a driver to support its behavior.
> 
> for example, ATID register for ETR0 with base address 0x10000f8: (tmp name used, TDFU for tmc data filter unit)

That looks like a hack to me and don't prefer that and there would be
multiple mappings for a single device and that could complicate device
resource accounting.

> 
> TDFU@10000f8 {
> ...
> }
> 
> ATID register for ETR1 with base address 0x1000108:
> TDFU@1000108 {
> ...
> }
> 
> The CCU device supports various features and the data filter feature for ETR is one of those features. How to support
> those features with one helper_enable function is a serious challenge. That's why we would like to separate it.
> Meanwhile, This solution can also resolve the offset issue.
> 
> We are looking forward your opinions with this proposal.

We need to know what other functionalities are supported and how that
will be used ?

In the worst case, you could define register bases for each port in the
CCU device node.

TDFU@.. {
	reg = <base-address 4K>

	<I don't know the standard property name for> =
	List of {
	<port number>, <Offset from base>,
	}
}

Suzuki