Re: [RFC PATCH 01/11] coresight: add CoreSight core layer framework

[Mathieu Poirier]

On 30 May 2014 11:25, Rob Herring <robherring2@xxxxxxxxx> wrote:
> On Fri, May 30, 2014 at 8:43 AM,  <mathieu.poirier@xxxxxxxxxx> wrote:
>> From: Pratik Patel <pratikp@xxxxxxxxxxxxxx>
>>
>> CoreSight components are compliant with the ARM CoreSight
>> architecture specification and can be connected in various
>> topologies to suite a particular SoCs tracing needs. These trace
>> components can generally be classified as sources, links and
>> sinks. Trace data produced by one or more sources flows through
>> the intermediate links connecting the source to the currently
>> selected sink.
>>
>> CoreSight framework provides an interface for the CoreSight trace
>> drivers to register themselves with. It's intended to build up a
>> topological view of the CoreSight components and configure the
>> right series of components on user input via debugfs.
>>
>> For eg., when enabling a source, framework builds up a path
>> consisting of all the components connecting the source to the
>> currently selected sink and enables all of them.
>>
>> Framework also supports switching between available sinks and
>> also provides status information to user space applications
>> through sysfs interface.
>>
>> Signed-off-by: Pratik Patel <pratikp@xxxxxxxxxxxxxx>
>> Signed-off-by: Panchaxari Prasannamurthy <panchaxari.prasannamurthy@xxxxxxxxxx>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>> ---
>>  .../devicetree/bindings/arm/coresight.txt          | 138 ++++
>>  drivers/Kconfig                                    |   2 +
>>  drivers/Makefile                                   |   1 +
>>  drivers/coresight/Kconfig                          |   9 +
>>  drivers/coresight/Makefile                         |   5 +
>>  drivers/coresight/coresight-priv.h                 |  46 ++
>>  drivers/coresight/coresight.c                      | 739 +++++++++++++++++++++
>>  drivers/coresight/of_coresight.c                   | 124 ++++
>>  include/linux/coresight.h                          | 181 +++++
>>  include/linux/of_coresight.h                       |  27 +
>>  10 files changed, 1272 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/coresight.txt
>>  create mode 100644 drivers/coresight/Kconfig
>>  create mode 100644 drivers/coresight/Makefile
>>  create mode 100644 drivers/coresight/coresight-priv.h
>>  create mode 100644 drivers/coresight/coresight.c
>>  create mode 100644 drivers/coresight/of_coresight.c
>>  create mode 100644 include/linux/coresight.h
>>  create mode 100644 include/linux/of_coresight.h
>>
>> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
>> new file mode 100644
>> index 0000000..3e21665
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
>> @@ -0,0 +1,138 @@
>> +* CoreSight Components
>> +
>> +CoreSight components are compliant with the ARM CoreSight architecture
>> +specification and can be connected in various topologies to suite a particular
>> +SoCs tracing needs. These trace components can generally be classified as sinks,
>> +links and sources. Trace data produced by one or more sources flows through the
>> +intermediate links connecting the source to the currently selected sink. Each
>> +CoreSight component device should use these properties to describe its hardware
>> +characteristcs.
>> +
>> +Required properties:
>> +
>> +- compatible : name of the component used for driver matching
>
> Where are the specific strings for components?

"Coresight" is made of multiple components with each component needing
a different driver.
>
>> +- reg : physical base address and length of the register set(s) of the component
>> +- coresight-id : unique integer identifier for the component
>
> compatible + reg is a unique id.

Some coresight IP blocks like the replicator don't have a reg property
- the "coresight-id" and "coresight-name" are the only way to identify
them properly.

>
>> +- coresight-name : unique descriptive name of the component
>
> compatible is a descriptive name.

That is what ends up under /sys/kernel/debug/coresight/.  Since there
can be multiple entity of the same type it is easier and more
convivial for users.  For instance: "coresight-etb-replicator" and
"coresight-tpiu-replicator" are easier to identify than
"coresight-replicator0" and "coresight-replicator1".

We need to keep "coresight-id" or "coresight-name" - I prefer the
former.  What is your take on this?

>
>> +- coresight-nr-inports : number of input ports on the component
>> +
>> +coresight-outports, coresight-child-list and coresight-child-ports lists will
>> +be of the same length and will have a one to one correspondence among the
>> +elements at the same list index.
>> +
>> +coresight-default-sink must be specified for one of the sink devices that is
>> +intended to be made the default sink. Other sink devices must not have this
>> +specified. Not specifying this property on any of the sinks is invalid.
>
> Why do you need the default-sink in DT? Isn't the configuration going
> to depend on the user?

Users can change this at will via debugfs.  It's a default boot time
configuration and can be removed if you're absolutely keen on it.
Please advise.

>
>> +
>> +Optional properties:
>> +
>> +- coresight-outports : list of output port numbers of this component
>> +- coresight-child-list : list of phandles pointing to the children of this
>> +                        component
>> +- coresight-child-ports : list of input port numbers of the children
>> +- coresight-default-sink : represents the default compile time CoreSight sink
>
> Seems like the OF graph helpers being defined for V4L2 could be useful
> here to describe the connections. We should not invent something new
> to describe arbitrary connections.

Very well, I'll take a look.

>
>> +- arm,buffer-size : size of contiguous buffer space for TMC ETR
>
> TMC ETR?

Trace Memory Controller in Embedded Trace Router configuration.

>
>> +- arm,cp14 : cp14 access to ETM registers is implemented and should be used
>> +- clocks : the clock associated to the coresight entity.
>> +- cpu: only valid for ETM/PTMs - the cpu this ETM/PTM is affined to.
>
> I think you need some separation of bindings by component rather than
> saying which components certain properties apply to.

Ok.

>
>> +
>> +Example:
>> +
>> +1. Bus declaration:
>> +       coresight {
>> +               compatible = "arm,coresight";
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               ranges;
>> +               ...
>> +               ...
>> +               ...
>> +        };
>> +
>> +        coresight {
>> +               compatible = "arm,coresight";
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               ranges = <0x20010000 0 0x20010000 0x1000
>> +                         0x20030000 0 0x20030000 0x1000
>> +                         0x20040000 0 0x20040000 0x1000
>> +                         0x2201c000 0 0x2201c000 0x1000
>> +                         0x2201d000 0 0x2201d000 0x1000
>> +                         0x2203c000 0 0x2203c000 0x1000
>> +                         0x2203d000 0 0x2203d000 0x1000
>> +                         0x2203e000 0 0x2203e000 0x1000>;
>
> This is not how ranges generally work. For no translation, use just
> "ranges;". Otherwise you should have base address and reg properties
> are just offsets from the base. Anyway, it's not really pertinent to
> your example.

In the case of that specific board doing a one-to-one mapping in
"ranges" was mandatory.  Otherwise the mapping wasn't done at all.

>
>> +               ...
>> +               ...
>> +               ...
>> +       };
>> +
>> +2. Sinks
>> +       etb: etb@20010000 {
>> +               compatible = "arm,coresight-etb";
>> +               reg = <0x20010000 0x1000>;
>> +
>> +               coresight-id = <0>;
>> +               coresight-name = "coresight-etb";
>> +               coresight-nr-inports = <1>;
>> +               coresight-default-sink;
>> +       };
>> +
>> +       tpiu: tpiu@20030000 {
>> +               compatible = "arm,coresight-tpiu";
>> +               reg = <0x20030000 0x1000>;
>> +
>> +               coresight-id = <1>;
>> +               coresight-name = "coresight-tpiu";
>> +               coresight-nr-inports = <1>;
>> +       };
>> +
>> +3. Links
>> +       replicator: replicator {
>> +               compatible = "arm,coresight-replicator";
>> +
>> +               coresight-id = <2>;
>> +               coresight-name = "coresight-replicator";
>> +               coresight-nr-inports = <1>;
>> +               coresight-outports = <0 1>;
>> +               coresight-child-list = <&etb &tpiu>;
>> +               coresight-child-ports = <0 0>;
>
> This could be one property instead of 2: <&etb 0>, <&tpiu 0>;
>

But then we'd have to do the parsing of said property in the code -
isn't that less desirable?

> Then add a #coresight-cells
>
>> +       };
>> +
>> +       funnel: funnel@20040000 {
>> +               compatible = "arm,coresight-funnel";
>> +               reg = <0x20040000 0x1000>;
>> +
>> +               coresight-id = <3>;
>> +               coresight-name = "coresight-funnel";
>> +               coresight-nr-inports = <8>;
>> +               coresight-outports = <0>;
>> +               coresight-child-list = <&replicator>;
>> +               coresight-child-ports = <0>;
>> +       };
>> +
>> +4. Sources
>> +       ptm0: ptm@2201c000 {
>> +               compatible = "arm,coresight-etm";
>> +               reg = <0x2201c000 0x1000>;
>> +
>> +               coresight-id = <9>;
>> +               coresight-name = "coresight-ptm0";
>> +               cpu = <&cpu0>;
>> +               coresight-nr-inports = <0>;
>
> A source can be implied by no inports property. Likewise for a sink.

Sources have  a compatible property to identify them.  A sink has a
single input port.  Did I get your comment right?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/