[PATCH] arm64: dts: juno: fix graph node unit addresses for coresight components

[PATCH] arm64: dts: juno: fix graph node unit addresses for coresight components

[Sudeep Holla]

Currently the coresight components graph node unit addresses are
continuous for both input and output ports while the "reg" properties
are restarted for input and output ports separately. This results is
the following DTC warnings:

 (graph_port): /etf@20010000/ports/port@1: graph node unit address error, expected "0"
 (graph_port): /etf@20140000/ports/port@1: graph node unit address error, expected "0"
 (graph_port): /funnel@20040000/ports/port@1: graph node unit address error, expected "0"
 (graph_port): /funnel@20040000/ports/port@2: graph node unit address error, expected "1"
 (graph_port): /funnel@20040000/ports/port@3: graph node unit address error, expected "2"
 (graph_port): /funnel@20130000/ports/port@1: graph node unit address error, expected "0"
 (graph_port): /funnel@20150000/ports/port@1: graph node unit address error, expected "0"
 (graph_port): /funnel@20150000/ports/port@2: graph node unit address error, expected "1"
 (graph_port): /funnel@220c0000/ports/port@1: graph node unit address error, expected "0"
 (graph_port): /funnel@220c0000/ports/port@2: graph node unit address error, expected "1"
 (graph_port): /funnel@230c0000/ports/port@1: graph node unit address error, expected "0"
 (graph_port): /funnel@230c0000/ports/port@2: graph node unit address error, expected "1"
 (graph_port): /funnel@230c0000/ports/port@3: graph node unit address error, expected "2"
 (graph_port): /funnel@230c0000/ports/port@4: graph node unit address error, expected "3"
 (graph_port): /replicator@20120000/ports/port@2: graph node unit address error, expected "0"

This patch makes even the reg property to follow the continuous
numbering as in the graph node unit address.

Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/boot/dts/arm/juno-base.dtsi    | 20 ++++++++++----------
 arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi |  8 ++++----
 arch/arm64/boot/dts/arm/juno.dts          |  2 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

Hi Suzuki/Mathieu,

I did a quick scan @ drivers/hwtracing/coresight/of_coresight.c to check
if reg field is being used or not and whether this change causes any
regression. I don't think so, but I may be wrong, let me know.

diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
index 708a15887af4..dbeca292a57c 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -131,7 +131,7 @@
 
 			/* output port */
 			port@1 {
-				reg = <0>;
+				reg = <1>;
 				etf0_out_port: endpoint {
 				};
 			};
@@ -175,7 +175,7 @@
 
 			/* input ports */
 			port@1 {
-				reg = <0>;
+				reg = <1>;
 				main_funnel_in_port0: endpoint {
 					slave-mode;
 					remote-endpoint = <&cluster0_funnel_out_port>;
@@ -183,7 +183,7 @@
 			};
 
 			port@2 {
-				reg = <1>;
+				reg = <2>;
 				main_funnel_in_port1: endpoint {
 					slave-mode;
 					remote-endpoint = <&cluster1_funnel_out_port>;
@@ -265,7 +265,7 @@
 			};
 
 			port@1 {
-				reg = <0>;
+				reg = <1>;
 				cluster0_funnel_in_port0: endpoint {
 					slave-mode;
 					remote-endpoint = <&cluster0_etm0_out_port>;
@@ -273,7 +273,7 @@
 			};
 
 			port@2 {
-				reg = <1>;
+				reg = <2>;
 				cluster0_funnel_in_port1: endpoint {
 					slave-mode;
 					remote-endpoint = <&cluster0_etm1_out_port>;
@@ -347,7 +347,7 @@
 			};
 
 			port@1 {
-				reg = <0>;
+				reg = <1>;
 				cluster1_funnel_in_port0: endpoint {
 					slave-mode;
 					remote-endpoint = <&cluster1_etm0_out_port>;
@@ -355,21 +355,21 @@
 			};
 
 			port@2 {
-				reg = <1>;
+				reg = <2>;
 				cluster1_funnel_in_port1: endpoint {
 					slave-mode;
 					remote-endpoint = <&cluster1_etm1_out_port>;
 				};
 			};
 			port@3 {
-				reg = <2>;
+				reg = <3>;
 				cluster1_funnel_in_port2: endpoint {
 					slave-mode;
 					remote-endpoint = <&cluster1_etm2_out_port>;
 				};
 			};
 			port@4 {
-				reg = <3>;
+				reg = <4>;
 				cluster1_funnel_in_port3: endpoint {
 					slave-mode;
 					remote-endpoint = <&cluster1_etm3_out_port>;
@@ -476,7 +476,7 @@
 
 			/* replicator input port */
 			port@2 {
-				reg = <0>;
+				reg = <2>;
 				replicator_in_port0: endpoint {
 					slave-mode;
 				};
diff --git a/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi b/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi
index 21287f2d75d3..a99f311c8dcb 100644
--- a/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi
@@ -21,7 +21,7 @@
 
 			/* input port */
 			port@1 {
-				reg = <0>;
+				reg = <1>;
 				csys1_funnel_in_port0: endpoint {
 					slave-mode;
 				};
@@ -52,7 +52,7 @@
 
 			/* output port */
 			port@1 {
-				reg = <0>;
+				reg = <1>;
 				etf1_out_port: endpoint {
 					remote-endpoint = <&csys2_funnel_in_port1>;
 				};
@@ -81,7 +81,7 @@
 
 			/* input ports */
 			port@1 {
-				reg = <0>;
+				reg = <1>;
 				csys2_funnel_in_port0: endpoint {
 					slave-mode;
 					remote-endpoint = <&etf0_out_port>;
@@ -89,7 +89,7 @@
 			};
 
 			port@2 {
-				reg = <1>;
+				reg = <2>;
 				csys2_funnel_in_port1: endpoint {
 					slave-mode;
 					remote-endpoint = <&etf1_out_port>;
diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index aa3b341a7547..34e940e7465b 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -260,7 +260,7 @@
 &main_funnel {
 	ports {
 		port@3 {
-			reg = <2>;
+			reg = <3>;
 			main_funnel_in_port2: endpoint {
 				slave-mode;
 				remote-endpoint = <&stm_out_port>;
-- 
2.7.4

Re: [PATCH] arm64: dts: juno: fix graph node unit addresses for coresight components

[Suzuki K Poulose]

On 05/16/2018 11:34 AM, Sudeep Holla wrote:
> Currently the coresight components graph node unit addresses are 
> continuous for both input and output ports while the "reg"
> properties are restarted for input and output ports separately. This
> results is the following DTC warnings:
> 
> (graph_port): /etf@20010000/ports/port@1: graph node unit address
> error, expected "0" (graph_port): /etf@20140000/ports/port@1: graph
> node unit address error, expected "0" (graph_port):
> /funnel@20040000/ports/port@1: graph node unit address error,
> expected "0" (graph_port): /funnel@20040000/ports/port@2: graph node
> unit address error, expected "1" (graph_port):
> /funnel@20040000/ports/port@3: graph node unit address error,
> expected "2" (graph_port): /funnel@20130000/ports/port@1: graph node
> unit address error, expected "0" (graph_port):
> /funnel@20150000/ports/port@1: graph node unit address error,
> expected "0" (graph_port): /funnel@20150000/ports/port@2: graph node
> unit address error, expected "1" (graph_port):
> /funnel@220c0000/ports/port@1: graph node unit address error,
> expected "0" (graph_port): /funnel@220c0000/ports/port@2: graph node
> unit address error, expected "1" (graph_port):
> /funnel@230c0000/ports/port@1: graph node unit address error,
> expected "0" (graph_port): /funnel@230c0000/ports/port@2: graph node
> unit address error, expected "1" (graph_port):
> /funnel@230c0000/ports/port@3: graph node unit address error,
> expected "2" (graph_port): /funnel@230c0000/ports/port@4: graph node
> unit address error, expected "3" (graph_port):
> /replicator@20120000/ports/port@2: graph node unit address error,
> expected "0"
> 
> This patch makes even the reg property to follow the continuous 
> numbering as in the graph node unit address.
> 
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Mathieu Poirier
> <mathieu.poirier@linaro.org> Cc: Liviu Dudau <liviu.dudau@arm.com> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>> --- 
> arch/arm64/boot/dts/arm/juno-base.dtsi    | 20 ++++++++++---------- 
> arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi |  8 ++++---- 
> arch/arm64/boot/dts/arm/juno.dts          |  2 +- 3 files changed, 15
> insertions(+), 15 deletions(-)
> 
> Hi Suzuki/Mathieu,
> 
> I did a quick scan @ drivers/hwtracing/coresight/of_coresight.c to
> check if reg field is being used or not and whether this change
> causes any regression. I don't think so, but I may be wrong, let me
> know.

Unfortunately, I think this would break the components like funnel,
where we need the input port number for the connected master to enable
the port. Similarly for the output port number for master components in
the paths. I have a set of patches which address this by taking care of
the port number order to find out the hardware port number.

I will dust it up and send it. That would bring up another important
question.

How do we deal with the change in the port number scheme ? e.g, should
the new kernel support DTBs with old scheme ? If so, how do we specify
that the DT uses new scheme.

Cheers
Suzuki

Re: [PATCH] arm64: dts: juno: fix graph node unit addresses for coresight components

[Sudeep Holla]

On 16/05/18 12:23, Suzuki K Poulose wrote:
> On 05/16/2018 11:34 AM, Sudeep Holla wrote:

[..]

>> Hi Suzuki/Mathieu,
>>
>> I did a quick scan @ drivers/hwtracing/coresight/of_coresight.c to
>> check if reg field is being used or not and whether this change
>> causes any regression. I don't think so, but I may be wrong, let me
>> know.
> 
> Unfortunately, I think this would break the components like funnel,
> where we need the input port number for the connected master to enable
> the port. Similarly for the output port number for master components in
> the paths. I have a set of patches which address this by taking care of
> the port number order to find out the hardware port number.
> 

Ah ok, I now see of_graph_parse_endpoint, sorry for missing that.

> I will dust it up and send it. That would bring up another important
> question.
> 

Cool

> How do we deal with the change in the port number scheme ? e.g, should
> the new kernel support DTBs with old scheme ? 

IIUC, that's needed for backward compatibility as it was used schema.
Again I may be wrong.

> If so, how do we specify that the DT uses new scheme.

Perhaps, add something to indicate the change in numbering scheme ?

-- 
Regards,
Sudeep

Re: [PATCH] arm64: dts: juno: fix graph node unit addresses for coresight components

[Mathieu Poirier]

On 16 May 2018 at 05:49, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 16/05/18 12:23, Suzuki K Poulose wrote:
>> On 05/16/2018 11:34 AM, Sudeep Holla wrote:
>
> [..]
>
>>> Hi Suzuki/Mathieu,
>>>
>>> I did a quick scan @ drivers/hwtracing/coresight/of_coresight.c to
>>> check if reg field is being used or not and whether this change
>>> causes any regression. I don't think so, but I may be wrong, let me
>>> know.
>>
>> Unfortunately, I think this would break the components like funnel,
>> where we need the input port number for the connected master to enable
>> the port. Similarly for the output port number for master components in
>> the paths. I have a set of patches which address this by taking care of
>> the port number order to find out the hardware port number.
>>
>
> Ah ok, I now see of_graph_parse_endpoint, sorry for missing that.

The problem is not with of_graph_parse_endpoint(), that will work just
fine.  In fact you can add whatever number you want there without
impact on how devices see each other in the framework.  The problem is
that the port numbering doesn't reflect the HW layout anymore and as
such  you can't rely on the port value when configuring the HW.

>
>> I will dust it up and send it. That would bring up another important
>> question.
>>
>
> Cool
>
>> How do we deal with the change in the port number scheme ? e.g, should
>> the new kernel support DTBs with old scheme ?
>

DT files following the old scheme will spew out warnings like we're
getting on Juno and are bound to be fixed.

> IIUC, that's needed for backward compatibility as it was used schema.
> Again I may be wrong.
>
>> If so, how do we specify that the DT uses new scheme.
>
> Perhaps, add something to indicate the change in numbering scheme ?

The current customers should be moved to the new scheme.  That way we
don't have to support two different DT scheme (where one will die
eventually).

>
> --
> Regards,
> Sudeep

Re: [PATCH] arm64: dts: juno: fix graph node unit addresses for coresight components

[Sudeep Holla]

On 16/05/18 18:29, Mathieu Poirier wrote:
> On 16 May 2018 at 05:49, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 16/05/18 12:23, Suzuki K Poulose wrote:
>>> On 05/16/2018 11:34 AM, Sudeep Holla wrote:
>>
>> [..]
>>
>>>> Hi Suzuki/Mathieu,
>>>>
>>>> I did a quick scan @ drivers/hwtracing/coresight/of_coresight.c to
>>>> check if reg field is being used or not and whether this change
>>>> causes any regression. I don't think so, but I may be wrong, let me
>>>> know.
>>>
>>> Unfortunately, I think this would break the components like funnel,
>>> where we need the input port number for the connected master to enable
>>> the port. Similarly for the output port number for master components in
>>> the paths. I have a set of patches which address this by taking care of
>>> the port number order to find out the hardware port number.
>>>
>>
>> Ah ok, I now see of_graph_parse_endpoint, sorry for missing that.
> 
> The problem is not with of_graph_parse_endpoint(), that will work just
> fine.  In fact you can add whatever number you want there without
> impact on how devices see each other in the framework.  The problem is
> that the port numbering doesn't reflect the HW layout anymore and as
> such  you can't rely on the port value when configuring the HW.
> 

Exactly, I had a chat with Suzuki. One option is to introduce new hwid
field to fix that. If the field is present, it will be used and reg
field will be ignored. In that way, you won't break the compatibility.
Just my thoughts. I am not sure if we need to support old kernel with
updated DTs though. That may be difficult.

>>
>>> I will dust it up and send it. That would bring up another important
>>> question.
>>>
>>
>> Cool
>>
>>> How do we deal with the change in the port number scheme ? e.g, should
>>> the new kernel support DTBs with old scheme ?
>>
> 
> DT files following the old scheme will spew out warnings like we're
> getting on Juno and are bound to be fixed.
> 
True, they can pick up the new changes too along with changes just
fixing the warning.

>> IIUC, that's needed for backward compatibility as it was used schema.
>> Again I may be wrong.
>>
>>> If so, how do we specify that the DT uses new scheme.
>>
>> Perhaps, add something to indicate the change in numbering scheme ?
> 
> The current customers should be moved to the new scheme.  That way we
> don't have to support two different DT scheme (where one will die
> eventually).
> 

But the problem is if someone has DT which can't be modified and we move
to new scheme only in the kernel, then they will see regression on their
platforms. If that's fine, it simplifies things a lot. But I am not so
sure of that ...

-- 
Regards,
Sudeep

Re: [PATCH] arm64: dts: juno: fix graph node unit addresses for coresight components

[Mathieu Poirier]

On 16 May 2018 at 11:40, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 16/05/18 18:29, Mathieu Poirier wrote:
>> On 16 May 2018 at 05:49, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>
>>> On 16/05/18 12:23, Suzuki K Poulose wrote:
>>>> On 05/16/2018 11:34 AM, Sudeep Holla wrote:
>>>
>>> [..]
>>>
>>>>> Hi Suzuki/Mathieu,
>>>>>
>>>>> I did a quick scan @ drivers/hwtracing/coresight/of_coresight.c to
>>>>> check if reg field is being used or not and whether this change
>>>>> causes any regression. I don't think so, but I may be wrong, let me
>>>>> know.
>>>>
>>>> Unfortunately, I think this would break the components like funnel,
>>>> where we need the input port number for the connected master to enable
>>>> the port. Similarly for the output port number for master components in
>>>> the paths. I have a set of patches which address this by taking care of
>>>> the port number order to find out the hardware port number.
>>>>
>>>
>>> Ah ok, I now see of_graph_parse_endpoint, sorry for missing that.
>>
>> The problem is not with of_graph_parse_endpoint(), that will work just
>> fine.  In fact you can add whatever number you want there without
>> impact on how devices see each other in the framework.  The problem is
>> that the port numbering doesn't reflect the HW layout anymore and as
>> such  you can't rely on the port value when configuring the HW.
>>
>
> Exactly, I had a chat with Suzuki. One option is to introduce new hwid
> field to fix that. If the field is present, it will be used and reg
> field will be ignored. In that way, you won't break the compatibility.

I had the same idea a while back and something Grant Likely and I
talked about at ELC-E 2017 in Prague.  This is the way I would prefer
to move forward on this.

> Just my thoughts. I am not sure if we need to support old kernel with
> updated DTs though. That may be difficult.

Not sure that can be done.  The only way to do that would be to
continue using the reg property, which isn't possible anymore.  As
such said DT wouldn't be upstream and I'm not sure we need to care
about those.

>
>>>
>>>> I will dust it up and send it. That would bring up another important
>>>> question.
>>>>
>>>
>>> Cool
>>>
>>>> How do we deal with the change in the port number scheme ? e.g, should
>>>> the new kernel support DTBs with old scheme ?
>>>
>>
>> DT files following the old scheme will spew out warnings like we're
>> getting on Juno and are bound to be fixed.
>>
> True, they can pick up the new changes too along with changes just
> fixing the warning.
>
>>> IIUC, that's needed for backward compatibility as it was used schema.
>>> Again I may be wrong.
>>>
>>>> If so, how do we specify that the DT uses new scheme.
>>>
>>> Perhaps, add something to indicate the change in numbering scheme ?
>>
>> The current customers should be moved to the new scheme.  That way we
>> don't have to support two different DT scheme (where one will die
>> eventually).
>>
>
> But the problem is if someone has DT which can't be modified and we move
> to new scheme only in the kernel, then they will see regression on their
> platforms. If that's fine, it simplifies things a lot. But I am not so
> sure of that ...

You have a point.  The code could check for both reg and hwid
properties.  If both are there hwid is preferred.  Otherwise the reg
property is used.  That wouldn't be too difficult to implement and
avoid introducing a regression.  Of course all this is fantasy until
we actually try things out.

Thanks,
Mathieu

>
> --
> Regards,
> Sudeep