From: Suzuki K Poulose <Suzuki.Poulose@arm.com>
To: Matt Sealey <Matt.Sealey@arm.com>
Cc: Rob Herring <robh@kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>,
Sudeep Holla <Sudeep.Holla@arm.com>,
Mark Rutland <Mark.Rutland@arm.com>,
"frowand.list@gmail.com" <frowand.list@gmail.com>,
Charles Garcia-Tobin <Charles.Garcia-Tobin@arm.com>,
John Horley <John.Horley@arm.com>,
"mike.leach@linaro.org" <mike.leach@linaro.org>,
"coresight@lists.linaro.org" <coresight@lists.linaro.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
Date: Wed, 13 Jun 2018 18:07:42 +0100 [thread overview]
Message-ID: <075b99a1-2a29-f6dc-e9f2-99f895c27d35@arm.com> (raw)
In-Reply-To: <HE1PR0802MB24127EC935B9AF6F82AB3B6EEE7E0@HE1PR0802MB2412.eurprd08.prod.outlook.com>
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
next prev parent reply other threads:[~2018-06-13 17:07 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-01 13:15 [RFC PATCH 0/8] coresight: Update device tree bindings Suzuki K Poulose
2018-06-01 13:16 ` [RFC PATCH 1/8] dts: binding: coresight: Document graph bindings Suzuki K Poulose
2018-06-01 16:14 ` Mathieu Poirier
2018-06-01 13:16 ` [RFC PATCH 2/8] coresight: Fix remote endpoint parsing Suzuki K Poulose
2018-06-01 19:38 ` Mathieu Poirier
2018-06-01 19:46 ` Mathieu Poirier
2018-06-04 10:34 ` Suzuki K Poulose
2018-06-01 13:16 ` [RFC PATCH 3/8] coresight: Cleanup platform description data Suzuki K Poulose
2018-06-01 13:16 ` [RFC PATCH 4/8] coresight: platform: Cleanup coresight connection handling Suzuki K Poulose
2018-06-01 13:16 ` [RFC PATCH 5/8] coresight: Handle errors in finding input/output ports Suzuki K Poulose
2018-06-01 13:16 ` [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings Suzuki K Poulose
2018-06-01 20:26 ` Mathieu Poirier
2018-06-12 20:48 ` Rob Herring
2018-06-13 9:45 ` Suzuki K Poulose
2018-06-13 12:49 ` Matt Sealey
2018-06-13 13:35 ` Suzuki K Poulose
2018-06-13 13:57 ` Rob Herring
2018-06-13 15:47 ` Matt Sealey
2018-06-13 17:07 ` Suzuki K Poulose [this message]
2018-06-13 19:40 ` Mathieu Poirier
2018-06-13 21:07 ` Matt Sealey
2018-06-14 8:53 ` Suzuki K Poulose
2018-06-14 13:59 ` Rob Herring
2018-06-14 15:04 ` Matt Sealey
2018-06-15 9:58 ` Suzuki K Poulose
2018-07-03 9:44 ` Suzuki K Poulose
2018-06-01 13:16 ` [RFC PATCH 7/8] dts: coresight: Define new bindings for direction of data flow Suzuki K Poulose
2018-06-01 20:39 ` Mathieu Poirier
2018-06-04 14:20 ` Suzuki K Poulose
2018-06-01 13:16 ` [RFC PATCH 8/8] dts: juno: Update coresight bindings for hw port Suzuki K Poulose
2018-06-01 20:59 ` Mathieu Poirier
2018-06-01 21:04 ` [RFC PATCH 0/8] coresight: Update device tree bindings Mathieu Poirier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=075b99a1-2a29-f6dc-e9f2-99f895c27d35@arm.com \
--to=suzuki.poulose@arm.com \
--cc=Charles.Garcia-Tobin@arm.com \
--cc=John.Horley@arm.com \
--cc=Mark.Rutland@arm.com \
--cc=Matt.Sealey@arm.com \
--cc=Sudeep.Holla@arm.com \
--cc=coresight@lists.linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=frowand.list@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@linaro.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).