Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
Hi Rob, On 14/06/18 14:59, Rob Herring wrote: On Thu, Jun 14, 2018 at 2:53 AM, Suzuki K Poulose wrote: On 13/06/18 22:07, Matt Sealey wrote: -Original Message- From: Mathieu Poirier 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. No, the above comment is about using "unit" ( if it is a standard property for specifying something specific to hardware) instead of "coresight,hwid". I would prefer to stick to the DT graph bindings, because : "unit" is not a standard property and I don't like it either. 1) The connections are bi-directional => Well, not necessarily bi-directional in terms of the data flow. But the connection information is critical. i.e, we need information about both the end-points of a connection, which the DT graph bindings solves. All we are missing is a way for specifying the "hardware port" number and the direction of flow. I don't see why do we need to create something new just for these two properties for something that exists today and works reasonably well for the usecase. If you have "in-ports" and "out-ports" nodes, then that gives you direction and you can use reg for the "hardware port". in-ports { port@0 { reg = <0>; endpoint {...}; }; port@1 { reg = <1>; endpoint {...}; }; }; out-ports { port@0 { reg = <0>; endpoint {...}; }; }; I'll need to check, but dtc may need an update to not warn about this. I did a quick check with the upstream dtc tool and the above form is doesn't generate any errors. Mathieu, Rob, Shall I proceed with the proposal above ? Suzuki
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
On 14/06/18 14:59, Rob Herring wrote: On Thu, Jun 14, 2018 at 2:53 AM, Suzuki K Poulose wrote: On 13/06/18 22:07, Matt Sealey wrote: -Original Message- From: Mathieu Poirier 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. No, the above comment is about using "unit" ( if it is a standard property for specifying something specific to hardware) instead of "coresight,hwid". I would prefer to stick to the DT graph bindings, because : "unit" is not a standard property and I don't like it either. 1) The connections are bi-directional => Well, not necessarily bi-directional in terms of the data flow. But the connection information is critical. i.e, we need information about both the end-points of a connection, which the DT graph bindings solves. All we are missing is a way for specifying the "hardware port" number and the direction of flow. I don't see why do we need to create something new just for these two properties for something that exists today and works reasonably well for the usecase. Rob, If you have "in-ports" and "out-ports" nodes, then that gives you direction and you can use reg for the "hardware port". in-ports { port@0 { reg = <0>; endpoint {...}; }; port@1 { reg = <1>; endpoint {...}; }; }; out-ports { port@0 { reg = <0>; endpoint {...}; }; }; I'll need to check, but dtc may need an update to not warn about this. Ok, that looks good. What are you trying to say about being sharply different than ACPI? The proposed Coresight ACPI draft bindings are based on the ACPI Graph bindings (just like the DT graph bindings and is compatible with it, in terms of the APIs. i.e, fwnode_graph_* operations work for both ACPI and DT alike). So, what Mathieu, in turn means is, if we depart from the DT Graph bindings, which I personally don't see any benefit in. If DT bindings can be reused for ACPI, that's fine, but don't expect any DT bindings to be accepted simply because they match ACPI bindings. The proposed bindings are not making it compatible with the ACPI. In fact, the ACPI bindings do need something similar to give us an explicit hardware port number, which needs to be worked out. The only common theme shared between the proposed ACPI and current DT bindings are the Generic Graph bindings for describing the connections, which allows sharing the parsing code independent of the platform, using fwnode_ops. Suzuki
RE: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
Hi Rob, To take this in a somewhat different direction... > > No, the above comment is about using "unit" ( if it is a standard > > property for specifying something specific to hardware) instead of > > "coresight,hwid". I would prefer to stick to the DT graph bindings, > > because : > > "unit" is not a standard property and I don't like it either. Fair enough. > If you have "in-ports" and "out-ports" nodes, then that gives you > direction and you can use reg for the "hardware port". > > in-ports { > port@0 { > reg = <0>; > endpoint {...}; > }; > port@1 { > reg = <1>; > endpoint {...}; > }; > }; > out-ports { > port@0 { > reg = <0>; > endpoint {...}; > }; > }; > > I'll need to check, but dtc may need an update to not warn about this. If the requirement that unit-address and the low #address-cells of reg now being forced by a validation tool is screwing us over here, we can't really change the tool; that's a IEEE 1275-1994 requirement we'd be hacking around. Here's a thought - why don't we define the ports in each component in a separate graph? Describing the ATB topology as a bus with a graph structure and then having the components simply reference the port nodes would make it much easier to design. Part of the problem is trying to describe an ATB bus via a component on an APB bus. You lose all the niceties of describing a bus with bus subcomponents. Lets take the ATB bus outside.. by describing the ingress and egress of the component you have no idea where in the graph you are at the point of entry (you could start parsing at a funnel, which means travelling around looking for other units that match other compatibles). If the CoreSight subsystem could have a graph pre-made of the full topology, then the components can point to how they wrap parts of that topology. You can also do away with ridiculous non-programmable funnels and replicator nodes, since the trace graph would encapsulate that information without having to instantiate a stub driver for it. That doesn't solve 'unit' or 'unit-address' or 'reg' or anything but it would keep us in graph bindings and abstract the ATB topology from the components. Other fun stuff; describing bridge components and bus widths within the graph could give abilities to describe bandwidths over each trace path. In any case, what happens when you have to decide what your funnel port priorities are, you'll have to define a new property for that, and it won't be 'reg'. Trying not to add anything to the graph bindings is one thing but trying to shoehorn it in either by doing that or inventing wholly device-specific properties are just as bad. Perhaps we can use the path@unit-address:arguments <-- parameters here to add extra information about the port. 'my-args' would pull it out in Forth, I don't know how FDT code deals with it (but there should be something, since we use it for console definition in /chosen). They're entirely specific to the node in question, but not used in path matching. Why can't we decide on a graph-binding update that allows specifying something simple as a demultiplexer output in a way the driver can understand and map to the hardware? > If DT bindings can be reused for ACPI, that's fine However in my opinion not the preferred way to do it.. > any DT bindings to be accepted simply because they match ACPI > bindings. I'm sure ACPI has capability of a much classier way of describing things, but I have some fear that it's just OF bindings stuffed in a _DSD.. DT and ACPI have the responsibility of shouldering the burden of describing hardware. Any attempts to try and make some commonality to promote brevity of Linux code is totally out of scope.. You can always abstract whatever definition into whatever the Linux drivers need at the time. That's kind of the point. Locking in to 'how it's currently coded' is stifling when realizing the first attempt didn't actually adequately do what it was intended. Ta, Matt IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
On Thu, Jun 14, 2018 at 2:53 AM, Suzuki K Poulose wrote: > On 13/06/18 22:07, Matt Sealey wrote: >> >> >> >>> -Original Message- >>> From: Mathieu Poirier >>> 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. > > > No, the above comment is about using "unit" ( if it is a standard property > for specifying something specific to hardware) instead of "coresight,hwid". > I would prefer to stick to the DT graph bindings, because : "unit" is not a standard property and I don't like it either. > > 1) The connections are bi-directional => Well, not necessarily > bi-directional > in terms of the data flow. But the connection information is critical. i.e, > we need information about both the end-points of a connection, which the DT > graph bindings solves. > > All we are missing is a way for specifying the "hardware port" number and > the > direction of flow. I don't see why do we need to create something new just > for > these two properties for something that exists today and works reasonably > well > for the usecase. If you have "in-ports" and "out-ports" nodes, then that gives you direction and you can use reg for the "hardware port". in-ports { port@0 { reg = <0>; endpoint {...}; }; port@1 { reg = <1>; endpoint {...}; }; }; out-ports { port@0 { reg = <0>; endpoint {...}; }; }; I'll need to check, but dtc may need an update to not warn about this. > >> >> What are you trying to say about being sharply different than ACPI? > > > The proposed Coresight ACPI draft bindings are based on the ACPI Graph > bindings > (just like the DT graph bindings and is compatible with it, in terms of the > APIs. > i.e, fwnode_graph_* operations work for both ACPI and DT alike). > > So, what Mathieu, in turn means is, if we depart from the DT Graph bindings, > which > I personally don't see any benefit in. If DT bindings can be reused for ACPI, that's fine, but don't expect any DT bindings to be accepted simply because they match ACPI bindings. Rob
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
On 13/06/18 22:07, Matt Sealey wrote: -Original Message- From: Mathieu Poirier 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. No, the above comment is about using "unit" ( if it is a standard property for specifying something specific to hardware) instead of "coresight,hwid". I would prefer to stick to the DT graph bindings, because : 1) The connections are bi-directional => Well, not necessarily bi-directional in terms of the data flow. But the connection information is critical. i.e, we need information about both the end-points of a connection, which the DT graph bindings solves. All we are missing is a way for specifying the "hardware port" number and the direction of flow. I don't see why do we need to create something new just for these two properties for something that exists today and works reasonably well for the usecase. What are you trying to say about being sharply different than ACPI? The proposed Coresight ACPI draft bindings are based on the ACPI Graph bindings (just like the DT graph bindings and is compatible with it, in terms of the APIs. i.e, fwnode_graph_* operations work for both ACPI and DT alike). So, what Mathieu, in turn means is, if we depart from the DT Graph bindings, which I personally don't see any benefit in. Suzuki
RE: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
> -Original Message- > From: Mathieu Poirier > > > 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. What are you trying to say about being sharply different than ACPI? Ta, Matt IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
On 13 June 2018 at 11:07, Suzuki K Poulose 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 > > > 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 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". > >> >>
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 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 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 par
RE: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
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 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 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 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" 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? 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. > 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 or mmcblkp3 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? Ta, Matt IMPORTANT NOTICE: Th
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
On Wed, Jun 13, 2018 at 7:35 AM, Suzuki K Poulose wrote: > 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!). OF graph is not directional. All links must be bi-directional and in fact dtc checks that now. The parent node should know the numbering and direction of each port. > 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. I really don't like slave-mode nor coresight,hwid. I would prefer to see getting rid of both and splitting ports into "in-ports" and "out-ports" nodes instead of a single "ports" node. Then you don't need any of these properties and reg can be used as the hwid. Rob
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 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 Cc: Sudeep Holla Cc: Rob Herring Signed-off-by: Suzuki K Poulose --- .../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 [,]. OK. The issue here is that a coresight component could
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
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. 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!). 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. 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? 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). Ta, Matt Sent from my iPhone > On Jun 13, 2018, at 04:45, Suzuki K Poulose 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 >>> Cc: Sudeep Holla >>> Cc: Rob Herring >>> Signed-off-by: Suzuki K Poulose >>> --- >>> .../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 >> [,]. > > 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 da
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
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 Cc: Sudeep Holla Cc: Rob Herring Signed-off-by: Suzuki K Poulose --- .../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 [,]. 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
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
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 > Cc: Sudeep Holla > Cc: Rob Herring > Signed-off-by: Suzuki K Poulose > --- > .../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 [,]. > + > > > 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. 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. Rob
Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
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 > Cc: Sudeep Holla > Cc: Rob Herring > Signed-off-by: Suzuki K Poulose > --- > .../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. > + > > > Example: > @@ -120,6 +124,7 @@ Example: > etb_in_port: endpoint@0 { > slave-mode; > remote-endpoint = <&replicator_out_port0>; > + coresight,hwid = <0>; > }; > }; > }; > @@ -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>; > replicator_in_port0: endpoint { > slave-mode; > remote-endpoint = <&funnel_out_port0>; > + coresight,hwid = <0>; > }; > }; > }; > @@ -191,31 +200,35 @@ Example: > funnel_out_port0: endpoint { > remote-endpoint = > <&replicator_in_port0>; > + coresight,hwid = <0>; > }; > }; > > /* funnel input ports */ > port@1 { > - reg = <0>; > + reg = <1>; > funnel_in_port0: endpoint { > slave-mode; > remote-endpoint = <&ptm0_out_port>; > + coresight,hwid = <0>; > }; > }; > > port@2 { > - reg = <1>; > + reg = <2>; > funnel_in_port1: endpoint { > slave-mode; > remote-endpoint = <&ptm1_out_port>; > + coresight,hwid = <1>; > }; > }; > > port@3 { > - reg = <2>; > + reg = <3>; > funnel_in_port2: e