Message-ID: <c0d531ec-9201-bfd8-e6c3-9140698b0697@arm.com>
Date:   Wed, 13 Jun 2018 14:35:09 +0100
From:   Suzuki K Poulose <Suzuki.Poulose@....com>
To:     Matt Sealey <Matt.Sealey@....com>
Cc:     Rob Herring <robh@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "mathieu.poirier@...aro.org" <mathieu.poirier@...aro.org>,
        Sudeep Holla <Sudeep.Holla@....com>,
        Mark Rutland <Mark.Rutland@....com>,
        "frowand.list@...il.com" <frowand.list@...il.com>,
        Charles Garcia-Tobin <Charles.Garcia-Tobin@....com>,
        John Horley <John.Horley@....com>,
        "mike.leach@...aro.org" <mike.leach@...aro.org>,
        "coresight@...ts.linaro.org" <coresight@...ts.linaro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph
 bindings

Hi Matt,

Thanks for your comments, responses inline.

On 13/06/18 13:49, Matt Sealey wrote:
> Suzuki,
> 
> Why not use “unit”?
> 
> I believe we had this discussion years ago about numbering serial ports and sdhci (i.e. how do you know it’s UART0 or UART1 from just the address? Some SoC’s don’t address sequentially *or* in a forward direction) - I believe it’s not exactly codified in ePAPR, not am I sure where it may be otherwise, but it exists.

We have different situation here. We need to know *the port number* as understood by the
hardware, so that we can enable *the specific* port for a given path.

> 
> I agree with Rob on the slave-mode nonsense, this is an SPI controller concept weirdly stuffed into a directed graph which implicitly tells you the data direction - it’s a rooted tree (just like DT!).

Btw, the "slave-mode" is not a standard DT graph binding. It is not part of the
generic DT graph binding. In fact the generic bindings stay away from the direction
aspect and explicitly mentions the same.

> 
> For the case of a funnel each device supplying trace should end up into an input node - numbered with a unit - and all those nodes should point to the output node as endpoints. Describing the hardware as a black box is probably less of a good idea than showing that it’s a funnel, or replicator by showing the internal paths. You wouldn’t need to “number” ports with a unit except where the HW needs to differentiate between them, and you don’t need reg or a node address to do it.
> 

As I mentioned above, we need the hardware numbers to enable the "specific" port.

E.g, :

static void funnel_enable_hw(struct funnel_drvdata *drvdata, int port)
{
         u32 functl;

         CS_UNLOCK(drvdata->base);

         functl = readl_relaxed(drvdata->base + FUNNEL_FUNCTL);
         functl &= ~FUNNEL_HOLDTIME_MASK;
         functl |= FUNNEL_HOLDTIME;
         functl |= (1 << port);
         writel_relaxed(functl, drvdata->base + FUNNEL_FUNCTL);
         writel_relaxed(drvdata->priority, drvdata->base + FUNNEL_PRICTL);

         CS_LOCK(drvdata->base);
}


> If you really need to parse full graphs in both directions (find root, find leaf) then could we simply introduce properties which list the phandles of all uplink sources, as linked lists point to the list head?

No we don't need to parse it in both ways, up and down. Btw, the trace paths
are not statically created. They are done at runtime, as configured by the
user. So all we need to do is have a list of the ports and the devices it
is connected to (of course with direction information). I would stay
away from duplicating the platform code when something already does
a good job.

> 
> This gives a way to validate that the graph starts and ends the way we expect, and also allows every port to be associated with being a required path between any two devices without parsing the *whole* graph (although you still need to do that to find the route to sinks).

Coming back to your suggestion of "unit", what does it imply ?
Its too generic a term for something as concrete as a port number.

Cheers
Suzuki

> 
> Ta,
> Matt
> 
> Sent from my iPhone
> 
>> On Jun 13, 2018, at 04:45, Suzuki K Poulose <Suzuki.Poulose@....com> wrote:
>>
>> Hi Rob,
>>
>>> On 12/06/18 21:48, Rob Herring wrote:
>>>> On Fri, Jun 01, 2018 at 02:16:05PM +0100, Suzuki K Poulose wrote:
>>>> The coresight drivers relied on default bindings for graph
>>>> in DT, while reusing the "reg" field of the "ports" to indicate
>>>> the actual hardware port number for the connections. However,
>>>> with the rules getting stricter w.r.t to the address mismatch
>>>> with the label, it is no longer possible to use the port address
>>>> field for the hardware port number. Hence, we add an explicit
>>>> property to denote the hardware port number, "coresight,hwid"
>>>> which must be specified for each "endpoint".
>>>>
>>>> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
>>>> Cc: Sudeep Holla <sudeep.holla@....com>
>>>> Cc: Rob Herring <robh@...nel.org>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>>>> ---
>>>>   .../devicetree/bindings/arm/coresight.txt          | 26 +++++++++---
>>>>   drivers/hwtracing/coresight/of_coresight.c         | 46 ++++++++++++++++------
>>>>   2 files changed, 54 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
>>>> index bd36e40..385581a 100644
>>>> --- a/Documentation/devicetree/bindings/arm/coresight.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
>>>> @@ -104,7 +104,11 @@ properties to uniquely identify the connection details.
>>>>       "slave-mode"
>>>>      * Hardware Port number at the component:
>>>> -     -  The hardware port number is assumed to be the address of the "port" component.
>>>> +   - (Obsolete) The hardware port number is assumed to be the address of the "port" component.
>>>> +   - Each "endpoint" must define the hardware port of the local end of the
>>>> +     connection using the following property:
>>>> +    "coresight,hwid" - 32bit integer, hardware port number at the local end.
>>> "coresight" is not a vendor and properties are in the form
>>> [<vendor>,]<prop-name>.
>>
>> OK. The issue here is that a coresight component could be an Arm IP or
>> a custom partner IP. So, the vendor could be either arm or the partner id.
>> However, this property is kind of a generic one for the Coresight family,
>> which is why we opted for "coresight". What is the guideline for such
>> cases ?
>>
>> Or in other words I see the following possible options :
>>
>> 1) coresight,hwid    - coresight generic
>> 2) arm,coresight-hwid    - arm vendor, however the device could be from any vendor.
>> 3) hwid            - Generic
>> 4) none of the above, something completely different.
>>
>> What do you recommend from the above ?
>>
>>>> +
>>>>       Example:
>>>> @@ -120,6 +124,7 @@ Example:
>>>>               etb_in_port: endpoint@0 {
>>> There shouldn't be a unit address here because there is no reg property.
>>>>                   slave-mode;
>>>>                   remote-endpoint = <&replicator_out_port0>;
>>>> +                coresight,hwid = <0>;
>>> It doesn't make sense for these to be in the endpoint. If you had
>>> multiple endpoints, then you would have to duplicate it. "ports" are
>>> a single data stream. "endpoints" are connections to that stream. So if
>>> you have a muxed (input) or fanout/1-to-many (output) connection, then
>>> you have multiple endpoints.
>>
>> We do have many-to-1 input (e.g, funnels) and 1-to-many outputs
>> (e.g replicators). However, we have (so far) used only one endpoint per
>> port.
>>
>> Also we could potentially have multiple data streams flowing through
>> the ports, which gets filtered to different ports in 1-to-many components
>> (read programmable-replicator).
>>
>> So the point is we have a shared path which carries different data
>> streams with mux/demux components. I am open for suggestions based on
>> the above facts.
>>
>>> The same applied to the slave-mode property, but that ship has sailed.
>>> No reason to continue that though.
>>>>               };
>>>>           };
>>>>       };
>>>> @@ -134,6 +139,7 @@ Example:
>>>>               tpiu_in_port: endpoint@0 {
>>>>                   slave-mode;
>>>>                   remote-endpoint = <&replicator_out_port1>;
>>>> +                coresight,hwid = <0>;
>>>>               };
>>>>           };
>>>>       };
>>>> @@ -154,6 +160,7 @@ Example:
>>>>                   reg = <0>;
>>>>                   replicator_out_port0: endpoint {
>>>>                       remote-endpoint = <&etb_in_port>;
>>>> +                    coresight,hwid = <0>;
>>>>                   };
>>>>               };
>>>>   @@ -161,15 +168,17 @@ Example:
>>>>                   reg = <1>;
>>>>                   replicator_out_port1: endpoint {
>>>>                       remote-endpoint = <&tpiu_in_port>;
>>>> +                    coresight,hwid = <1>;
>>>>                   };
>>>>               };
>>>>                 /* replicator input port */
>>>>               port@2 {
>>>> -                reg = <0>;
>>>> +                reg = <1>;
>>> This will still get flagged as an error. reg must be 2 here.
>>
>> Sorry, thats a mistake. I will fix it.
>>
>> Cheers
>> Suzuki

Powered by blists - more mailing lists