diff mbox series

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

Message ID 20240705090049.1656986-3-quic_jiegan@quicinc.com (mailing list archive)
State Not Applicable
Headers show
Series Coresight: Add Coresight Control Unit driver | expand

Commit Message

Jie Gan July 5, 2024, 9 a.m. UTC
Add binding document for Coresight Control Unit device.

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

Comments

Krzysztof Kozlowski July 5, 2024, 9:07 a.m. UTC | #1
On 05/07/2024 11:00, Jie Gan wrote:
> Add binding document for Coresight Control Unit device.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.
</form letter>

Or stop developing on some old tree. It's some sort of weird pattern in
entire Qualcomm Coresight - everything developed on old kernels.

You must work on latest mainline or maintainer or linux-next tree, not
some old Qualcomm tree. Your v5.15, v5.19, v6.4 or v6.8 or whatever you
have there: BIG NOPE.

> 
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---

Subject: it never ends with full stop.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

>  .../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

Drop _pclk

> +
> +  reg-names:

Please follow DTS coding style about order of properties.

> +    items:
> +      - const: ccu-base
> +
> +  in-ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    unevaluatedProperties:

This was never tested and it cannot reliably work.

Sorry, this is waste of our time.


> +      patternProperties:
> +        '^port(@[0-7])?$':
> +          description: Input connections from CoreSight Trace bus
> +          $ref: /schemas/graph.yaml#/properties/port
> +
> +          properties:
> +            qcom,ccu-atid-offset:
> +              description:
> +                Offset to the Coresight Control Unit component's ATID register
> +                that is used by specific TMC ETR. The ATID register can be programed based
> +                on the trace id to filter out specific trace data which gets into ETR buffer.
> +              $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> +  - compatible
> +  - reg
> +  - in-ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    syscon@1001000 {

That's not a syscon.

> +        compatible = "qcom,coresight-ccu";
> +        reg = <0x1001000 0x1000>;
> +        reg-names = "ccu-base";
> +

Best regards,
Krzysztof
Rob Herring July 5, 2024, 10:38 a.m. UTC | #2
On Fri, 05 Jul 2024 17:00:47 +0800, Jie Gan wrote:
> Add binding document for Coresight Control Unit device.
> 
> 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
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml: unevaluatedProperties: Missing additionalProperties/unevaluatedProperties constraint

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240705090049.1656986-3-quic_jiegan@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Jie Gan July 8, 2024, 12:47 a.m. UTC | #3
On Fri, Jul 05, 2024 at 11:07:22AM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2024 11:00, Jie Gan wrote:
> > Add binding document for Coresight Control Unit device.
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> </form letter>
> 
> Or stop developing on some old tree. It's some sort of weird pattern in
> entire Qualcomm Coresight - everything developed on old kernels.
> 
> You must work on latest mainline or maintainer or linux-next tree, not
> some old Qualcomm tree. Your v5.15, v5.19, v6.4 or v6.8 or whatever you
> have there: BIG NOPE.
> 
> > 
> > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > ---
> 
> Subject: it never ends with full stop.
> 
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> > +    items:
> > +      - const: apb_pclk
> 
> Drop _pclk
> 
> > +
> > +  reg-names:
> 
> Please follow DTS coding style about order of properties.
> 
> > +    items:
> > +      - const: ccu-base
> > +
> > +  in-ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    unevaluatedProperties:
> 
> This was never tested and it cannot reliably work.
> 
> Sorry, this is waste of our time.
> 
>
Sorry about that. I will re-check the dt-binding file.
 
> > +      patternProperties:
> > +        '^port(@[0-7])?$':
> > +          description: Input connections from CoreSight Trace bus
> > +          $ref: /schemas/graph.yaml#/properties/port
> > +
> > +          properties:
> > +            qcom,ccu-atid-offset:
> > +              description:
> > +                Offset to the Coresight Control Unit component's ATID register
> > +                that is used by specific TMC ETR. The ATID register can be programed based
> > +                on the trace id to filter out specific trace data which gets into ETR buffer.
> > +              $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - in-ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    syscon@1001000 {
> 
> That's not a syscon.
> 
> > +        compatible = "qcom,coresight-ccu";
> > +        reg = <0x1001000 0x1000>;
> > +        reg-names = "ccu-base";
> > +
> 
> Best regards,
> Krzysztof
> 

Thanks,
Jie
Suzuki K Poulose July 8, 2024, 9:41 a.m. UTC | #4
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 ?

> 
> 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)

I believe I mentioned this in the previous posting too ?

Suzuki
Jie Gan July 8, 2024, 10:10 a.m. UTC | #5
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.

>
> I believe I mentioned this in the previous posting too ?
Yes, you mentioned before. I moved it from TMC filed to CCU filed.

> 
> Suzuki
>
Jie Gan July 8, 2024, 10:25 a.m. UTC | #6
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.

So I think it should be better to define it in device tree node.

> 
> >
> > I believe I mentioned this in the previous posting too ?
> Yes, you mentioned before. I moved it from TMC filed to CCU filed.
> 
> > 
> > Suzuki
> > 
>
Suzuki K Poulose July 8, 2024, 12:50 p.m. UTC | #7
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.

> 
> So I think it should be better to define it in device tree node.

No. See above.

Suzuki

> 
>>
>>>
>>> I believe I mentioned this in the previous posting too ?
>> Yes, you mentioned before. I moved it from TMC filed to CCU filed.
>>
>>>
>>> Suzuki
>>>
>>
Jie Gan July 9, 2024, 6 a.m. UTC | #8
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.
> 
> Suzuki
> 
> > 
> > > 
> > > > 
> > > > I believe I mentioned this in the previous posting too ?
> > > Yes, you mentioned before. I moved it from TMC filed to CCU filed.
> > > 
> > > > 
> > > > Suzuki
> > > > 
> > > 
> 

Thanks,
Jie
Jie Gan July 11, 2024, 8:36 a.m. UTC | #9
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)

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.

Thanks!

> > 
> > Suzuki
> > 
> > > 
> > > > 
> > > > > 
> > > > > I believe I mentioned this in the previous posting too ?
> > > > Yes, you mentioned before. I moved it from TMC filed to CCU filed.
> > > > 
> > > > > 
> > > > > Suzuki
> > > > > 
> > > > 
> > 
> 
> Thanks,
> Jie
> 

Thanks,
Jie
Suzuki K Poulose July 11, 2024, 9:32 a.m. UTC | #10
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
Jie Gan July 15, 2024, 8:46 a.m. UTC | #11
On Thu, Jul 11, 2024 at 10:32:17AM +0100, Suzuki K Poulose wrote:
> 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.

This solution dropped. We will consider another proper solution to address the issue.

> 
> > 
> > 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 ?
>
Sorry for the late reply.
I will introduce the features expected to be merged into the community.

Excepts for data filter feature for ETR, there are three more major features supported by CCU:
1. Byte counter: It is responsible for moving trace data from ETR buffer to main memory(e.g. main DDR RAM or SD card) to prevent
older trace data from being overwritten by newer data. There is a register named QDSS_CS_QDSSCSR_ETRIRQCTRL in CCU device that
controls this feature. The value of this register defines the number of bytes that when moved by the ETR AXI interface. The system will trigger
an interrupt to transfer the data when the data amount exceeds the specified value. ETR must be enabled when use this interrupt function.

for example, When the value 1000 is set in the register (in binary), it indicates that the data amount is 64 bytes.
The interrupt is triggered when the data amount exceeds 64 bytes.
(Binary 1000 equals 8 in decimal. Multiplying this by 8 gives a data amount of 64 bytes.) 

Here is the link for the patch series for Byte counter feature.
https://lore.kernel.org/linux-arm-msm/20230813151253.38128-1-quic_jinlmao@quicinc.com/

2. MSR for TPDM: QC introduced a feature called Switched Always on Debug(SWAO). The component AOSS that utilize the SWAO have designed with 4 CMB TPDM
and 1 DSB TPDM units. Control of bus selection that will reach TPDM CMB are done through CCU.
The registers that designed to control the bus selection:
CS_SWAOCSR_TPDM_PRIO0_CMB_CONTROL
CS_SWAOCSR_TPDM_PRIO1_CMB_CONTROL
CS_SWAOCSR_TPDM_PRIO2_CMB_CONTROL
CS_SWAOCSR_TPDM_PRIO3_CMB_CONTROL

for example, the value programmed into the register represents:
00000 for BCM
00100 for DDRAUX
01000 for VRM
...

3. Direct to USB(D2USB): There is a register called CS_QDSSCSR_QDSS_DIR2USB in CCU to control the D2USB feature. When sets, the data in ETR will directly
output to the USB over the existing system bus infrastructure.

Besides, the CCU also support some features like timestamp, Hardware Event Control for STM.

> In the worst case, you could define register bases for each port in the
> CCU device node.
We will consider this solution as plan b that we do not anticipate implementing.

> 
> TDFU@.. {
> 	reg = <base-address 4K>
> 
> 	<I don't know the standard property name for> =
> 	List of {
> 	<port number>, <Offset from base>,
> 	}
> }
> 
> Suzuki
> 

Thanks,
Jie
Jie Gan July 18, 2024, 2:35 a.m. UTC | #12
On Fri, Jul 05, 2024 at 11:07:22AM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2024 11:00, Jie Gan wrote:
> > Add binding document for Coresight Control Unit device.
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> </form letter>
> 
> Or stop developing on some old tree. It's some sort of weird pattern in
> entire Qualcomm Coresight - everything developed on old kernels.
> 
> You must work on latest mainline or maintainer or linux-next tree, not
> some old Qualcomm tree. Your v5.15, v5.19, v6.4 or v6.8 or whatever you
> have there: BIG NOPE.
> 
> > 
> > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > ---
> 
> Subject: it never ends with full stop.
> 
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> >  .../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
> 
> Drop _pclk
Hi, Suzuki,
As Krzysztof proposed, we have to change the clock name from apb_pclk to apb.
According to the new clock name, I updated the inline function coresight_get_enable_apb_pclk
to vote the clock with the new name.

Is that ok with the change? Or any other suggestions?

static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev)
{
        struct clk *pclk;
        int ret;

        pclk = clk_get(dev, "apb_pclk");
        if (IS_ERR(pclk)) {
                pclk = clk_get(dev, "apb");    //added line
                if (IS_ERR(pclk))
                        return NULL;
        }

        ret = clk_prepare_enable(pclk);
        if (ret) {
                clk_put(pclk);
                return ERR_PTR(ret);
        }
        return pclk;
}

> 
> > +
> > +  reg-names:
> 
> Please follow DTS coding style about order of properties.
> 
> > +    items:
> > +      - const: ccu-base
> > +
> > +  in-ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    unevaluatedProperties:
> 
> This was never tested and it cannot reliably work.
> 
> Sorry, this is waste of our time.
> 
> 
> > +      patternProperties:
> > +        '^port(@[0-7])?$':
> > +          description: Input connections from CoreSight Trace bus
> > +          $ref: /schemas/graph.yaml#/properties/port
> > +
> > +          properties:
> > +            qcom,ccu-atid-offset:
> > +              description:
> > +                Offset to the Coresight Control Unit component's ATID register
> > +                that is used by specific TMC ETR. The ATID register can be programed based
> > +                on the trace id to filter out specific trace data which gets into ETR buffer.
> > +              $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - in-ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    syscon@1001000 {
> 
> That's not a syscon.
> 
> > +        compatible = "qcom,coresight-ccu";
> > +        reg = <0x1001000 0x1000>;
> > +        reg-names = "ccu-base";
> > +
> 
> Best regards,
> Krzysztof
> 

Thanks,
Jie
diff mbox series

Patch

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:
+              description:
+                Offset to the Coresight Control Unit component's ATID register
+                that is used by specific TMC ETR. The ATID register can be programed based
+                on the trace id to filter out specific trace data which gets into ETR buffer.
+              $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+  - in-ports
+
+additionalProperties: false
+
+examples:
+  - |
+    syscon@1001000 {
+        compatible = "qcom,coresight-ccu";
+        reg = <0x1001000 0x1000>;
+        reg-names = "ccu-base";
+
+        in-ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+                ccu_in_port0: endpoint {
+                    qcom,ccu-atid-offset = <0x1f>;
+                    remote-endpoint = <&etr0_out_port>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                ccu_in_port1: endpoint {
+                    qcom,ccu-atid-offset = <0x2f>;
+                    remote-endpoint = <&etr1_out_port>;
+                };
+            };
+        };
+    };