Message-ID: <075b99a1-2a29-f6dc-e9f2-99f895c27d35@arm.com>
Date:   Wed, 13 Jun 2018 18:07:42 +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,

On 13/06/18 16:47, Matt Sealey wrote:
> Hi 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.
> 
> For the purposes of abstraction, each port will have the property of having
> a node which is pointed to by other nodes, and in the case of a true ATB
> endpoint, no other nodes behind it.
> 
> It doesn't matter what the HW numbers it as as long as the driver can derive
> it from whatever you put in the DT. So a funnel (which is ~8 ports muxed into
> one output):
> 
>     f1p0: port {
>        unit = <0>;
>        endpoint = <&f1out>;
>     };
>     f1p1: port {
>        unit = <4>;
>        endpoint = <&f1out>;
>     };
>     f1out: port {
>        endpoint = <&etf1>;
>     };
> 
> "unit" here is specific to the driver's understanding of ports within it's

I may be missing, but is "unit" something that already exists and used by
DT bindings already ? Or is this something new that we are proposing ?

> own cycle of the graph. For a replicator you can invert the logic - input
> ports don't need a unit, but the two outputs are filtered in CoreSight not

I would prefer to make the new property mandatory for all the ports to avoid
a potential problem in the future.

How do you represent a TMC-ETF which has one input and one output connection ?
Also what happens if we ever get a component which has m-to-n connections ?

> by leg but by transiting ATB ID in groups of 16 IDs. In that case maybe
> you would want to describe all 8 possible units on each leg with the first
> ID it would filter? Or just list tuples of filter IDs <id, first, last>

I am failing to follow the ATB ID group description above. As per the TRM,
e.g, replicator filters the "trace stream" based on the "trace ID", which I
believe can be programmed via IDFILTER<n> register. So why would we need that
to be part of the DT ?

> 
> Who cares, really, as long as the driver knows what it means.
> 
> You don't need to namespace every property.
> 
>> As I mentioned above, we need the hardware numbers to enable the
>> "specific" port.
> 
> Okay and how is this not able to be prescribed in a binding for "arm,coresight-funnel"
> that:
> 
> "input ports are numbered from 0 to N where N is the maximum input port
> number. This number is identified with the "unit" property, which directly
> corresponds to the bit position in the funnel Ctrl_Reg register, and the
> bit position multiplied by 3 for each 3-bit priority in the funnel
> Priority_Ctrl_Reg, with N having a maximum of the defined register bitfield
> DEVID[PORTCOUNT], minus one, for that component"

The description looks over complicated to me at least, even after having known
bit of the programming interface of the components. I would prefer staying
closer to the terms used in the TRM ("slave/master" interfaces) and make it
easier for people to write the DT.

> 
> Or a replicator:
> 
> "output ports are numbered per the CoreSight ATB Replicator specification,
> unit corresponding to the IDFILTERn register controlling ID filters for
> that leg, with a maximum of the defined register bitfield DEVID[PORTNUM],
> minus one"
> 
> One could clarify it, even, with labels for readability ("label" definitely
> is a well defined if also completely arbitrary property).
> 
> ..
> 
>> 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);
>> }
>>
>> 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.
> 
> You do realize this isn't how the hardware works, correct?

The "trace paths" mentioned above were indeed the software path, which
was constructed at runtime. The graph connections are indeed a one time
parsing at probe time and as you said they don't change. And by configuring,
I mean selecting the "source" and the "sink".

> 
> Trace paths are fixed, they may diverge with different configurations, but
> the full CoreSight topology (all funnels, replicators and intermediary
> Components) is entirely unchangeable.
> 
> The DT should provide the information to provide a reference acyclic directed
> graph of the entire topology (or entirely reasonably programmable topology where
> at all possible) - if a user wants to trace from ETM_0 then they only
> have particular paths to particular sinks, for instance ETM_0 and ETF_0
> may be on their own path, so you cannot just "configure as a user"
> a path from ETM_1 to ETF_0 since there isn't one.

> 
> Walking said graphs with the knowledge that CoreSight specifically disallows
> loopbacks in ATB topology is basic computer science problem - literally a
> matter of topological sorting. But let's build a graph once and traverse it -
> don't build the graph partially each time or try and build it to cross-check
> every time. The paths are wires in the design, lets not fake to the user
> that there is any configurability in that or try and encode that in the
> DT.

Sorry for the confusion, as explained above, it is indeed a one time pass.

> 
>> Coming back to your suggestion of "unit", what does it imply ?
> 
> Whatever the driver likes. For uart and mmc, it was just a spurious number
> but it could be applied as the end of, say, ttyS<N> or mmcblk<N>p3 or used
> in any other driver-specific manner. The number you put in is up to you,
> but the valid numbers would be in the binding for that particular device.
> 
>> Its too generic a term for something as concrete as a port number.
> 
> Is it?
> 
> Why would you need a whole other property type to encode a u32 that
> describes an arbitrary number specific to that hardware device?

So, if the suggestion is to use an existing property "unit", I am fine
with it, if people agree to it.


Thanks for the comments.

Cheers,
Suzuki

Powered by blists - more mailing lists