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

Message ID 1528235011-30691-9-git-send-email-suzuki.poulose@arm.com
State Changes Requested
Headers show
Series
  • coresight: Update device tree bindings
Related show

Commit Message

Suzuki K Poulose June 5, 2018, 9:43 p.m.
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@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 .../devicetree/bindings/arm/coresight.txt          | 29 ++++++++++---
 drivers/hwtracing/coresight/of_coresight.c         | 49 +++++++++++++++++-----
 2 files changed, 62 insertions(+), 16 deletions(-)

Comments

Mathieu Poirier June 8, 2018, 9:22 p.m. | #1
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@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  .../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@linaro.org>

> 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
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Suzuki K Poulose June 11, 2018, 9:22 a.m. | #2
On 08/06/18 22:22, Mathieu Poirier wrote:
> 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@linaro.org>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   .../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"


>>   	};
> 
> For the binding part:
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
>> 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

...

>> +/*
>>    * 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.

Please note the return value description here. Further comments below.

>>    *	-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.
> 

If we are unable to parse a port, we can simply ignore the port and continue, which
is what we have today with the existing code. I didn't change it and still think
it is the best effort thing. We could spit a warning for such cases, if really needed.
Also, the parsing code almost never fails at the moment. If it fails to find "reg" field,
it is assumed to be '0'. Either way ignoring it seems harmless. That said I am open
to suggestions.

Cheers
Suzuki
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mathieu Poirier June 11, 2018, 4:52 p.m. | #3
On 11 June 2018 at 03:22, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 08/06/18 22:22, Mathieu Poirier wrote:
>>
>> 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@linaro.org>
>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>   .../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"
>
>
>
>>>         };
>>
>>
>> For the binding part:
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>
>>> 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
>
>
> ...
>
>>> +/*
>>>    * 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.
>
>
> Please note the return value description here. Further comments below.
>
>>>    *    -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.
>>
>
> If we are unable to parse a port, we can simply ignore the port and
> continue, which
> is what we have today with the existing code. I didn't change it and still
> think
> it is the best effort thing. We could spit a warning for such cases, if
> really needed.
> Also, the parsing code almost never fails at the moment. If it fails to find
> "reg" field,
> it is assumed to be '0'. Either way ignoring it seems harmless. That said I
> am open
> to suggestions.

Looking at the original code I remember not mandating enpoints to be
valid for debugging purposes.  That certainly helps when building up a
device tree file but also has the side effect of silently overlooking
specification problems.  Fortunately the revamping you did on that
part of the code makes it very easy to change that, something I think
we should take advantage of (it can only lead to positive scenarios
where defective specifications get pointed out).

That being said and because the original behaviour is just as
permissive, you can leave as is.

Thanks,
Mathieu

>
> Cheers
> Suzuki
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Suzuki K Poulose June 11, 2018, 4:55 p.m. | #4
On 11/06/18 17:52, Mathieu Poirier wrote:
> On 11 June 2018 at 03:22, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 08/06/18 22:22, Mathieu Poirier wrote:
>>>
>>> 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@linaro.org>
>>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>>> Cc: Rob Herring <robh@kernel.org>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>>    .../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"
>>
>>
>>
>>>>          };
>>>
>>>
>>> For the binding part:
>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

...

>>>> @@ -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.
>>>
>>
>> If we are unable to parse a port, we can simply ignore the port and
>> continue, which
>> is what we have today with the existing code. I didn't change it and still
>> think
>> it is the best effort thing. We could spit a warning for such cases, if
>> really needed.
>> Also, the parsing code almost never fails at the moment. If it fails to find
>> "reg" field,
>> it is assumed to be '0'. Either way ignoring it seems harmless. That said I
>> am open
>> to suggestions.
> 
> Looking at the original code I remember not mandating enpoints to be
> valid for debugging purposes.  That certainly helps when building up a
> device tree file but also has the side effect of silently overlooking
> specification problems.  Fortunately the revamping you did on that
> part of the code makes it very easy to change that, something I think
> we should take advantage of (it can only lead to positive scenarios
> where defective specifications get pointed out).
> 
> That being said and because the original behaviour is just as
> permissive, you can leave as is.

Thanks. So can I assume the Reviewed-by applies for the code now ?

Suzuki
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mathieu Poirier June 11, 2018, 9:51 p.m. | #5
On 11 June 2018 at 10:55, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 11/06/18 17:52, Mathieu Poirier wrote:
>>
>> On 11 June 2018 at 03:22, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>>>
>>> On 08/06/18 22:22, Mathieu Poirier wrote:
>>>>
>>>>
>>>> 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@linaro.org>
>>>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>>>> Cc: Rob Herring <robh@kernel.org>
>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> ---
>>>>>    .../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"
>>>
>>>
>>>
>>>
>>>>>          };
>>>>
>>>>
>>>>
>>>> For the binding part:
>>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>
>
> ...
>
>>>>> @@ -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.
>>>>
>>>
>>> If we are unable to parse a port, we can simply ignore the port and
>>> continue, which
>>> is what we have today with the existing code. I didn't change it and
>>> still
>>> think
>>> it is the best effort thing. We could spit a warning for such cases, if
>>> really needed.
>>> Also, the parsing code almost never fails at the moment. If it fails to
>>> find
>>> "reg" field,
>>> it is assumed to be '0'. Either way ignoring it seems harmless. That said
>>> I
>>> am open
>>> to suggestions.
>>
>>
>> Looking at the original code I remember not mandating enpoints to be
>> valid for debugging purposes.  That certainly helps when building up a
>> device tree file but also has the side effect of silently overlooking
>> specification problems.  Fortunately the revamping you did on that
>> part of the code makes it very easy to change that, something I think
>> we should take advantage of (it can only lead to positive scenarios
>> where defective specifications get pointed out).
>>
>> That being said and because the original behaviour is just as
>> permissive, you can leave as is.
>
>
> Thanks. So can I assume the Reviewed-by applies for the code now ?

Yes

>
> Suzuki
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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>;
 			};
 		};
 	};
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;
+			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);