Date:   Fri, 1 Jun 2018 15:04:39 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Suzuki K Poulose <suzuki.poulose@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, sudeep.holla@....com,
        robh@...nel.org, mark.rutland@....com, frowand.list@...il.com,
        matt.sealey@....com, charles.garcia-tobin@....com,
        john.horley@....com, mike.leach@...aro.org,
        coresight@...ts.linaro.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [RFC PATCH 0/8] coresight: Update device tree bindings

On Fri, Jun 01, 2018 at 02:15:59PM +0100, Suzuki K Poulose wrote:
> Coresight uses DT graph bindings to describe the connections of the
> components. However we have some undocumented usage of the bindings
> to describe some of the properties of the connections.
> 
> The coresight driver needs to know the hardware ports invovled
> in the connection and the direction of data flow to effectively
> manage the trace sessions. So far we have relied on the "port"
> address (as described by the generic graph bindings) to represent
> the hardware port of the component for a connection.
> 
> The hardware uses separate numbering scheme for input and output
> ports, which implies, we could have two different (input and output)
> ports with the same port number. This could create problems in the
> graph bindings where the label of the port wouldn't match the address.
> 
> e.g, with the existing bindings we get :
> 
> 	port@0{				// Output port 0
> 		reg = <0>;
> 		...
> 	};
> 
> 	port@1{
> 		reg = <0>;		// Input port 0
> 		endpoint {
> 			slave-mode;
> 			...
> 		};
> 	};
> 
> With the new enforcement in the DT rules, mismatches in label and address
> are not allowed (as see in the case for port@1). So, we need a new mechanism
> to describe the hardware port number reliably.
> 
> Also, we relied on an undocumented "slave-mode" property (see the above
> example) to indicate if the port is an input port. Let us formalise and
> switch to a new property to describe the direction of data flow.
> 
> There were three options considered for the hardware port number scheme:
> 
>  1) Use natural ordering in the DT to infer the hardware port number.
>   i.e, Mandate that the all ports are listed in the DT and in the ascending
>   order for each class (input and output respectively).
>    Pros :
>       - We don't need new properties and if the existing DTS list them in
>         order (which most of them do), they work out of the box.
>    Cons :
>       - We must list all the ports even if the system cannot/shouldn't use
>         it.
>       - It is prone to human errors (if the order is not kept).
> 
>  2) Use an explicit property to list both the direction and the hw port
>     number and direction. Define "coresight,hwid" as 2 member array of u32,
>     where the members are port number and the direction respectively.
> 	e.g
> 
> 	port@0{
> 		reg = <0>;
> 		endpoint {
> 			coresight,hwid = <0 1>;	// Port # 0, Output
> 		}
> 	};
> 
> 	port@1{
> 		reg = <1>;
> 		endpoint {
> 			coresight,hwid = <0 0>;	// Port # 0, Input
> 		};
> 	};
> 
> 	Pros:
> 	  - The bindings are formal but not so reader friendly and could potentially
> 	    lead to human errors.
> 	Cons:
> 	  - Backward compatiblity is lost.
>  3) Use explicit properties (implemented in the series) for the hardware
>     port id and direction. We define a new property "coresight,hwid" for
>     each endpoint in coresight devices to specify the hardware port number
>     explicitly. Also use a separate property "direction" to specify the
>     direction of the data flow.
> 
> 	e.g,
> 
> 	port@0{
> 		reg = <0>;
> 		endpoint {
> 			direction = <1>;	// Output
> 			coresight,hwid = <0>;	// Port # 0
> 		}
> 	};
> 
> 	port@1{
> 		reg = <1>;
> 		endpoint {
> 			direction = <0>;	// Input
> 			coresight,hwid = <0>;	// Port # 0
> 		};
> 	};
> 
>     Pros:
>        - The bindings are formal and reader friendly, and less prone to errors.
>     Cons:
>        - Backward compatibility is lost.
> 
> 
> This series achieves implements Option (3) listed above while still retaining
> the backward compatibility. The driver now issues a warning (once) when it
> encounters the old bindings.
> It also cleans up the platform parsing code to reduce the memory usage by
> reusing the platform description. The series also includes the
> changes for Juno platform as an example. If there are no objections
> to the approach, I could post the series, converting all the
> in-kernel DTS to the new binding.
> 
> Suzuki K Poulose (8):
>   dts: binding: coresight: Document graph bindings
>   coresight: Fix remote endpoint parsing
>   coresight: Cleanup platform description data
>   coresight: platform: Cleanup coresight connection handling
>   coresight: Handle errors in finding input/output ports
>   dts: coresight: Clean up the device tree graph bindings
>   dts: coresight: Define new bindings for direction of data flow
>   dts: juno: Update coresight bindings for hw port
> 
>  .../devicetree/bindings/arm/coresight.txt          |  52 ++++++++--
>  arch/arm64/boot/dts/arm/juno-base.dtsi             |  82 +++++++++++----
>  arch/arm64/boot/dts/arm/juno.dts                   |   5 +-
>  drivers/hwtracing/coresight/coresight.c            |  28 ++----
>  drivers/hwtracing/coresight/of_coresight.c         | 111 ++++++++++++---------
>  include/linux/coresight.h                          |  11 +-
>  6 files changed, 181 insertions(+), 108 deletions(-)

Aside from the comments I've already posted I'm pretty much good with this set.
Please rebase the next revision on my "next" branch and run checkpatch.pl on the
set.  Patch 6/8 and 7/8 are generating warnings.

Thanks,
Mathieu

> 
> -- 
> 2.7.4
> 

Powered by blists - more mailing lists