Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings

[Suzuki K Poulose]

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