Message-ID: <CANLsYkx9cjV-bWg7aLg-kJT7x7DtwTHdsYzzM8A3+cFFnu_mNg@mail.gmail.com>
Date:   Wed, 13 Jun 2018 13:40:07 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Suzuki K Poulose <Suzuki.Poulose@....com>
Cc:     Matt Sealey <Matt.Sealey@....com>, Rob Herring <robh@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.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

On 13 June 2018 at 11:07, Suzuki K Poulose <Suzuki.Poulose@....com> wrote:
> 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.

If we're going to have something sharply different than ACPI I prefer
Rob's idea.

Mathieu

>
>
> Thanks for the comments.
>
> Cheers,
> Suzuki

Powered by blists - more mailing lists