Re: [PATCH 08/20] coresight: dts: Cleanup device tree graph bindings

[Mathieu Poirier]

On Tue, Jun 05, 2018 at 10:43:19PM +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 <mathieu.poirier@xxxxxxxxxx>
> Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> ---
>  .../devicetree/bindings/arm/coresight.txt          | 29 ++++++++++---
>  drivers/hwtracing/coresight/of_coresight.c         | 49 +++++++++++++++++-----
>  2 files changed, 62 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index ed6b555..bf75ab3 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -108,8 +108,13 @@ following 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.
> +     - Each "endpoint" must define the hardware port of the local end of the
> +       connection using the following property :
> +
> +	"coresight,hwid" - 32bit integer, local hardware port.
> +
> +     - [ ** Obsolete ** ] The hardware port number is assumed to be the address
> +       of the "port" component.
>  
>  
>  
> @@ -126,6 +131,7 @@ Example:
>  			etb_in_port: endpoint@0 {
>  				slave-mode;
>  				remote-endpoint = <&replicator_out_port0>;
> +				coresight,hwid = <0>;
>  			};
>  		};
>  	};
> @@ -140,6 +146,7 @@ Example:
>  			tpiu_in_port: endpoint@0 {
>  				slave-mode;
>  				remote-endpoint = <&replicator_out_port1>;
> +				coresight,hwid = <0>;
>  			};
>  		};
>  	};
> @@ -160,6 +167,7 @@ Example:
>  				reg = <0>;
>  				replicator_out_port0: endpoint {
>  					remote-endpoint = <&etb_in_port>;
> +					coresight,hwid = <0>;
>  				};
>  			};
>  
> @@ -167,15 +175,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>;
>  				};
>  			};
>  		};
> @@ -197,31 +207,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: endpoint {
>  					slave-mode;
>  					remote-endpoint = <&etm0_out_port>;
> +					coresight,hwid = <2>;
>  				};
>  			};
>  
> @@ -239,6 +253,7 @@ Example:
>  		port {
>  			ptm0_out_port: endpoint {
>  				remote-endpoint = <&funnel_in_port0>;
> +				coresight,hwid = <0>;
>  			};
>  		};
>  	};
> @@ -253,6 +268,7 @@ Example:
>  		port {
>  			ptm1_out_port: endpoint {
>  				remote-endpoint = <&funnel_in_port1>;
> +				coresight,hwid = <0>;
>  			};
>  		};
>  	};
> @@ -269,6 +285,7 @@ Example:
>  		port {
>  			stm_out_port: endpoint {
>  				remote-endpoint = <&main_funnel_in_port2>;
> +				coresight,hwid = <0>;
>  			};
>  		};
>  	};

For the binding part:
Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index d01a9ce..d23d7dd 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -99,6 +99,31 @@ int of_coresight_get_cpu(const struct device_node *node)
>  EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
>  
>  /*
> + * of_coresight_endpoint_get_port_id : Get the hardware port number for the
> + * given endpoint device node. Prefer the explicit "coresight,hwid" property
> + * over the endpoint register id (obsolete bindings).
> + */
> +static int of_coresight_endpoint_get_port_id(struct device *dev,
> +					     struct device_node *ep_node)
> +{
> +	struct of_endpoint ep;
> +	int rc, port_id;
> +
> +
> +	if (!of_property_read_u32(ep_node, "coresight,hwid", &port_id))
> +		return port_id;
> +
> +	rc = of_graph_parse_endpoint(ep_node, &ep);
> +	if (rc)
> +		return rc;
> +	dev_warn_once(dev,
> +		      "ep%d: Mandatory \"coresight,hwid\" property missing.\n",
> +		      ep.port);
> +	dev_warn_once(dev, "DT uses obsolete coresight bindings\n");
> +	return ep.port;
> +}
> +
> +/*
>   * of_coresight_parse_endpoint : Parse the given output endpoint @ep
>   * and fill the connection information in *@pconn.
>   *
> @@ -109,11 +134,11 @@ EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
>   *	 0	- If the parsing completed without any fatal errors.
>   *	-Errno	- Fatal error, abort the scanning.
>   */
> -static int of_coresight_parse_endpoint(struct device_node *ep,
> +static int of_coresight_parse_endpoint(struct device *dev,
> +				       struct device_node *ep,
>  				       struct coresight_connection **pconn)
>  {
> -	int ret = 0;
> -	struct of_endpoint endpoint, rendpoint;
> +	int ret = 0, local_port, child_port;
>  	struct device_node *rparent = NULL;
>  	struct device_node *rep = NULL;
>  	struct device *rdev = NULL;
> @@ -128,7 +153,8 @@ static int of_coresight_parse_endpoint(struct device_node *ep,
>  			break;
>  
>  		/* Parse the local port details */
> -		if (of_graph_parse_endpoint(ep, &endpoint))
> +		local_port = of_coresight_endpoint_get_port_id(dev, ep);
> +		if (local_port < 0)
>  			break;
>  		/*
>  		 * Get a handle on the remote endpoint and the device it is
> @@ -140,9 +166,6 @@ static int of_coresight_parse_endpoint(struct device_node *ep,
>  		rparent = of_graph_get_port_parent(rep);
>  		if (!rparent)
>  			break;
> -		if (of_graph_parse_endpoint(rep, &rendpoint))
> -			break;
> -
>  		/* If the remote device is not available, defer probing */
>  		rdev = of_coresight_get_endpoint_device(rparent);
>  		if (!rdev) {
> @@ -150,9 +173,15 @@ static int of_coresight_parse_endpoint(struct device_node *ep,
>  			break;
>  		}
>  
> -		conn->outport = endpoint.port;
> +		child_port = of_coresight_endpoint_get_port_id(rdev, rep);
> +		if (child_port < 0) {
> +			ret = 0;

Why returning '0' on an error condition?  Same for 'local_port' above.

> +			break;
> +		}
> +
> +		conn->outport = local_port;
>  		conn->child_name = dev_name(rdev);
> -		conn->child_port = rendpoint.port;
> +		conn->child_port = child_port;
>  		/* Move the connection record */
>  		(*pconn)++;
>  	} while (0);
> @@ -200,7 +229,7 @@ of_get_coresight_platform_data(struct device *dev,
>  		ep = of_graph_get_next_endpoint(node, ep);
>  		if (!ep)
>  			break;
> -		ret = of_coresight_parse_endpoint(ep, &conn);
> +		ret = of_coresight_parse_endpoint(dev, ep, &conn);
>  		if (ret)
>  			return ERR_PTR(ret);
>  	} while (ep);
> -- 
> 2.7.4
>