[PATCH v2 0/5] Fix the parse_dt of exynos dsi and remove the OF graph

[PATCH v2 0/5] Fix the parse_dt of exynos dsi and remove the OF graph

[Hoegeun Kwon]

Hi,

The dsi + panel is a parental relationship, so OF grpah is not needed.
Therefore, the current dsi_parse_dt function will throw an error,
because there is no linked OF graph for case such as fimd + dsi +
panel.

So the 1/5 patch parse the Pll, burst and esc clock frequency
properties in dsi_parse_dt and modified to create a bridge_node only
if there is an OF graph associated with dsi.

Also fixed the dts, which depend on the 1/5 patch. So removed the
ports node and move burst and esc clock frequency properties to the
parent (DSI node).

Changes for V2:
- Added the clear explanation for commit. (1/5 patch)
- Fixed it to the same subject as the actual work. (2/5 ~ 5/5 patches)

Best Regards,
Hoegeun

Hoegeun Kwon (5):
  drm/exynos: dsi: Fix the parse_dt function
  arm64: dts: exynos: Remove the OF graph from DSI node for exynos5433
    dts
  arm: dts: Remove the OF graph from DSI node for exynos3250 dts
  arm: dts: Remove the OF graph from DSI node for exynos4412 dts
  arm: dts: Remove the OF graph from DSI node for exynos4210 dts

 arch/arm/boot/dts/exynos3250-rinato.dts            | 23 ++--------------
 arch/arm/boot/dts/exynos4210-trats.dts             | 23 ++--------------
 arch/arm/boot/dts/exynos4412-trats2.dts            | 23 ++--------------
 .../boot/dts/exynos/exynos5433-tm2-common.dtsi     | 16 ++---------
 drivers/gpu/drm/exynos/exynos_drm_dsi.c            | 32 ++++++----------------
 5 files changed, 16 insertions(+), 101 deletions(-)

-- 
1.9.1

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

[PATCH v2 1/5] drm/exynos: dsi: Fix the parse_dt function

[Hoegeun Kwon]

The dsi + panel is a parental relationship, so OF grpah is not needed.
Therefore, the current dsi_parse_dt function will throw an error,
because there is no linked OF graph for case such as fimd + dsi +
panel. So this patch parse the Pll, burst and esc clock frequency
properties in dsi_parse_dt and modified to create a bridge_node only
if there is an OF graph associated with dsi.
So I think the ABI breakage is needed.

Signed-off-by: Hoegeun Kwon <hoegeun.kwon-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index e07cb1f..214d486 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1652,39 +1652,23 @@ static int exynos_dsi_parse_dt(struct exynos_dsi *dsi)
 	if (ret < 0)
 		return ret;
 
-	ep = of_graph_get_endpoint_by_regs(node, DSI_PORT_OUT, 0);
-	if (!ep) {
-		dev_err(dev, "no output port with endpoint specified\n");
-		return -EINVAL;
-	}
-
-	ret = exynos_dsi_of_read_u32(ep, "samsung,burst-clock-frequency",
+	ret = exynos_dsi_of_read_u32(node, "samsung,burst-clock-frequency",
 				     &dsi->burst_clk_rate);
 	if (ret < 0)
-		goto end;
+		return ret;
 
-	ret = exynos_dsi_of_read_u32(ep, "samsung,esc-clock-frequency",
+	ret = exynos_dsi_of_read_u32(node, "samsung,esc-clock-frequency",
 				     &dsi->esc_clk_rate);
 	if (ret < 0)
-		goto end;
-
-	of_node_put(ep);
+		return ret;
 
 	ep = of_graph_get_next_endpoint(node, NULL);
-	if (!ep) {
-		ret = -EINVAL;
-		goto end;
-	}
-
-	dsi->bridge_node = of_graph_get_remote_port_parent(ep);
-	if (!dsi->bridge_node) {
-		ret = -EINVAL;
-		goto end;
+	if (ep) {
+		dsi->bridge_node = of_graph_get_remote_port_parent(ep);
+		of_node_put(ep);
 	}
-end:
-	of_node_put(ep);
 
-	return ret;
+	return 0;
 }
 
 static int exynos_dsi_bind(struct device *dev, struct device *master,
-- 
1.9.1

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

[PATCH v2 2/5] arm64: dts: exynos: Remove the OF graph from DSI node for exynos5433 dts

[Hoegeun Kwon]

The OF graph is not needed because the panel is a child of dsi. So
Remove the ports node and move burst and esc clock frequency
properties to the parent (DSI node).

Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
---
 arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
index 6ce93a3..77ba238 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
@@ -298,23 +298,11 @@
 	status = "okay";
 	vddcore-supply = <&ldo6_reg>;
 	vddio-supply = <&ldo7_reg>;
+	samsung,burst-clock-frequency = <512000000>;
+	samsung,esc-clock-frequency = <16000000>;
 	samsung,pll-clock-frequency = <24000000>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&te_irq>;
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@1 {
-			reg = <1>;
-
-			dsi_out: endpoint {
-				samsung,burst-clock-frequency = <512000000>;
-				samsung,esc-clock-frequency = <16000000>;
-			};
-		};
-	};
 };
 
 &hdmi {
-- 
1.9.1

[PATCH v2 3/5] arm: dts: Remove the OF graph from DSI node for exynos3250 dts

[Hoegeun Kwon]

The OF graph is not needed because the panel is a child of dsi. So
Remove the ports node and move burst and esc clock frequency
properties to the parent (DSI node).

Signed-off-by: Hoegeun Kwon <hoegeun.kwon-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos3250-rinato.dts | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/arch/arm/boot/dts/exynos3250-rinato.dts b/arch/arm/boot/dts/exynos3250-rinato.dts
index 548413e..82e676a 100644
--- a/arch/arm/boot/dts/exynos3250-rinato.dts
+++ b/arch/arm/boot/dts/exynos3250-rinato.dts
@@ -215,24 +215,11 @@
 &dsi_0 {
 	vddcore-supply = <&ldo6_reg>;
 	vddio-supply = <&ldo6_reg>;
+	samsung,burst-clock-frequency = <250000000>;
+	samsung,esc-clock-frequency = <20000000>;
 	samsung,pll-clock-frequency = <24000000>;
 	status = "okay";
 
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@1 {
-			reg = <1>;
-
-			dsi_out: endpoint {
-				remote-endpoint = <&dsi_in>;
-				samsung,burst-clock-frequency = <250000000>;
-				samsung,esc-clock-frequency = <20000000>;
-			};
-		};
-	};
-
 	panel@0 {
 		compatible = "samsung,s6e63j0x03";
 		reg = <0>;
@@ -262,12 +249,6 @@
 				vsync-len = <2>;
 			};
 		};
-
-		port {
-			dsi_in: endpoint {
-				remote-endpoint = <&dsi_out>;
-			};
-		};
 	};
 };
 
-- 
1.9.1

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

[PATCH v2 4/5] arm: dts: Remove the OF graph from DSI node for exynos4412 dts

[Hoegeun Kwon]

The OF graph is not needed because the panel is a child of dsi. So
Remove the ports node and move burst and esc clock frequency
properties to the parent (DSI node).

Signed-off-by: Hoegeun Kwon <hoegeun.kwon-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos4412-trats2.dts | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts
index 41ecd6d..86ce5e5 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -385,24 +385,11 @@
 &dsi_0 {
 	vddcore-supply = <&ldo8_reg>;
 	vddio-supply = <&ldo10_reg>;
+	samsung,burst-clock-frequency = <500000000>;
+	samsung,esc-clock-frequency = <20000000>;
 	samsung,pll-clock-frequency = <24000000>;
 	status = "okay";
 
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@1 {
-			reg = <1>;
-
-			dsi_out: endpoint {
-				remote-endpoint = <&dsi_in>;
-				samsung,burst-clock-frequency = <500000000>;
-				samsung,esc-clock-frequency = <20000000>;
-			};
-		};
-	};
-
 	panel@0 {
 		compatible = "samsung,s6e8aa0";
 		reg = <0>;
@@ -430,12 +417,6 @@
 				vsync-len = <2>;
 			};
 		};
-
-		port {
-			dsi_in: endpoint {
-				remote-endpoint = <&dsi_out>;
-			};
-		};
 	};
 };
 
-- 
1.9.1

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

[PATCH v2 5/5] arm: dts: Remove the OF graph from DSI node for exynos4210 dts

[Hoegeun Kwon]

The OF graph is not needed because the panel is a child of dsi. So
Remove the ports node and move burst and esc clock frequency
properties to the parent (DSI node).

Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
---
 arch/arm/boot/dts/exynos4210-trats.dts | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-trats.dts b/arch/arm/boot/dts/exynos4210-trats.dts
index 0ca1b4d..9452bed 100644
--- a/arch/arm/boot/dts/exynos4210-trats.dts
+++ b/arch/arm/boot/dts/exynos4210-trats.dts
@@ -197,24 +197,11 @@
 &dsi_0 {
 	vddcore-supply = <&vusb_reg>;
 	vddio-supply = <&vmipi_reg>;
+	samsung,burst-clock-frequency = <500000000>;
+	samsung,esc-clock-frequency = <20000000>;
 	samsung,pll-clock-frequency = <24000000>;
 	status = "okay";
 
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@1 {
-			reg = <1>;
-
-			dsi_out: endpoint {
-				remote-endpoint = <&dsi_in>;
-				samsung,burst-clock-frequency = <500000000>;
-				samsung,esc-clock-frequency = <20000000>;
-			};
-		};
-	};
-
 	panel@0 {
 		reg = <0>;
 		compatible = "samsung,s6e8aa0";
@@ -242,12 +229,6 @@
 				vsync-len = <2>;
 			};
 		};
-
-		port {
-			dsi_in: endpoint {
-				remote-endpoint = <&dsi_out>;
-			};
-		};
 	};
 };
 
-- 
1.9.1

Re: [PATCH v2 1/5] drm/exynos: dsi: Fix the parse_dt function

[Andrzej Hajda]

On 09.02.2017 02:26, Hoegeun Kwon wrote:
> The dsi + panel is a parental relationship, so OF grpah is not needed.
> Therefore, the current dsi_parse_dt function will throw an error,
> because there is no linked OF graph for case such as fimd + dsi +
> panel. So this patch parse the Pll, burst and esc clock frequency
> properties in dsi_parse_dt and modified to create a bridge_node only
> if there is an OF graph associated with dsi.
> So I think the ABI breakage is needed.
>
> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>

Nice diffstat, more importantly it fixes bad design of early days of
of_graph.

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

One comment below.

> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 32 ++++++++------------------------
>  1 file changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index e07cb1f..214d486 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1652,39 +1652,23 @@ static int exynos_dsi_parse_dt(struct exynos_dsi *dsi)
>  	if (ret < 0)
>  		return ret;
>  
> -	ep = of_graph_get_endpoint_by_regs(node, DSI_PORT_OUT, 0);
> -	if (!ep) {
> -		dev_err(dev, "no output port with endpoint specified\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = exynos_dsi_of_read_u32(ep, "samsung,burst-clock-frequency",
> +	ret = exynos_dsi_of_read_u32(node, "samsung,burst-clock-frequency",
>  				     &dsi->burst_clk_rate);
>  	if (ret < 0)
> -		goto end;
> +		return ret;
>  
> -	ret = exynos_dsi_of_read_u32(ep, "samsung,esc-clock-frequency",
> +	ret = exynos_dsi_of_read_u32(node, "samsung,esc-clock-frequency",
>  				     &dsi->esc_clk_rate);
>  	if (ret < 0)
> -		goto end;
> -
> -	of_node_put(ep);
> +		return ret;
>  
>  	ep = of_graph_get_next_endpoint(node, NULL);
> -	if (!ep) {
> -		ret = -EINVAL;
> -		goto end;
> -	}
> -
> -	dsi->bridge_node = of_graph_get_remote_port_parent(ep);
> -	if (!dsi->bridge_node) {
> -		ret = -EINVAL;
> -		goto end;
> +	if (ep) {
> +		dsi->bridge_node = of_graph_get_remote_port_parent(ep);
> +		of_node_put(ep);

It looks like there is no of_node_put on dsi->bridge_node, but this is
for another patch.

Regards
Andrzej

>  	}
> -end:
> -	of_node_put(ep);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int exynos_dsi_bind(struct device *dev, struct device *master,


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/5] arm64: dts: exynos: Remove the OF graph from DSI node for exynos5433 dts

[Andrzej Hajda]

On 09.02.2017 02:26, Hoegeun Kwon wrote:
> The OF graph is not needed because the panel is a child of dsi. So
> Remove the ports node and move burst and esc clock frequency
> properties to the parent (DSI node).
>
> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>

For the whole patchset:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
--
Regards
Andrzej

> ---
>  arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
> index 6ce93a3..77ba238 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
> @@ -298,23 +298,11 @@
>  	status = "okay";
>  	vddcore-supply = <&ldo6_reg>;
>  	vddio-supply = <&ldo7_reg>;
> +	samsung,burst-clock-frequency = <512000000>;
> +	samsung,esc-clock-frequency = <16000000>;
>  	samsung,pll-clock-frequency = <24000000>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&te_irq>;
> -
> -	ports {
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		port@1 {
> -			reg = <1>;
> -
> -			dsi_out: endpoint {
> -				samsung,burst-clock-frequency = <512000000>;
> -				samsung,esc-clock-frequency = <16000000>;
> -			};
> -		};
> -	};
>  };
>  
>  &hdmi {


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm/exynos: dsi: Fix the parse_dt function

[Hoegeun Kwon]

On 02/09/2017 03:57 PM, Andrzej Hajda wrote:
> On 09.02.2017 02:26, Hoegeun Kwon wrote:
>> The dsi + panel is a parental relationship, so OF grpah is not needed.
>> Therefore, the current dsi_parse_dt function will throw an error,
>> because there is no linked OF graph for case such as fimd + dsi +
>> panel. So this patch parse the Pll, burst and esc clock frequency
>> properties in dsi_parse_dt and modified to create a bridge_node only
>> if there is an OF graph associated with dsi.
>> So I think the ABI breakage is needed.
>>
>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Nice diffstat, more importantly it fixes bad design of early days of
> of_graph.
>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>
> One comment below.
>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 32 ++++++++------------------------
>>   1 file changed, 8 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> index e07cb1f..214d486 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> @@ -1652,39 +1652,23 @@ static int exynos_dsi_parse_dt(struct exynos_dsi *dsi)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	ep = of_graph_get_endpoint_by_regs(node, DSI_PORT_OUT, 0);
>> -	if (!ep) {
>> -		dev_err(dev, "no output port with endpoint specified\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	ret = exynos_dsi_of_read_u32(ep, "samsung,burst-clock-frequency",
>> +	ret = exynos_dsi_of_read_u32(node, "samsung,burst-clock-frequency",
>>   				     &dsi->burst_clk_rate);
>>   	if (ret < 0)
>> -		goto end;
>> +		return ret;
>>   
>> -	ret = exynos_dsi_of_read_u32(ep, "samsung,esc-clock-frequency",
>> +	ret = exynos_dsi_of_read_u32(node, "samsung,esc-clock-frequency",
>>   				     &dsi->esc_clk_rate);
>>   	if (ret < 0)
>> -		goto end;
>> -
>> -	of_node_put(ep);
>> +		return ret;
>>   
>>   	ep = of_graph_get_next_endpoint(node, NULL);
>> -	if (!ep) {
>> -		ret = -EINVAL;
>> -		goto end;
>> -	}
>> -
>> -	dsi->bridge_node = of_graph_get_remote_port_parent(ep);
>> -	if (!dsi->bridge_node) {
>> -		ret = -EINVAL;
>> -		goto end;
>> +	if (ep) {
>> +		dsi->bridge_node = of_graph_get_remote_port_parent(ep);
>> +		of_node_put(ep);
> It looks like there is no of_node_put on dsi->bridge_node, but this is
> for another patch.

Thanks for your review.

Yes right, so i will make another patch.

Best Regards,
Hoegeun

>
> Regards
> Andrzej
>
>>   	}
>> -end:
>> -	of_node_put(ep);
>>   
>> -	return ret;
>> +	return 0;
>>   }
>>   
>>   static int exynos_dsi_bind(struct device *dev, struct device *master,
>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/5] Fix the parse_dt of exynos dsi and remove the OF graph

[Inki Dae]

Hi Krzysztof,

Can you merge patch 2 and 5 to your tree so that they can go to mainline? Otherwise, I can merge them to my tree if you give me acked-by.

Thanks,
Inki Dae


2017년 02월 09일 10:26에 Hoegeun Kwon 이(가) 쓴 글:
> Hi,
> 
> The dsi + panel is a parental relationship, so OF grpah is not needed.
> Therefore, the current dsi_parse_dt function will throw an error,
> because there is no linked OF graph for case such as fimd + dsi +
> panel.
> 
> So the 1/5 patch parse the Pll, burst and esc clock frequency
> properties in dsi_parse_dt and modified to create a bridge_node only
> if there is an OF graph associated with dsi.
> 
> Also fixed the dts, which depend on the 1/5 patch. So removed the
> ports node and move burst and esc clock frequency properties to the
> parent (DSI node).
> 
> Changes for V2:
> - Added the clear explanation for commit. (1/5 patch)
> - Fixed it to the same subject as the actual work. (2/5 ~ 5/5 patches)
> 
> Best Regards,
> Hoegeun
> 
> Hoegeun Kwon (5):
>   drm/exynos: dsi: Fix the parse_dt function
>   arm64: dts: exynos: Remove the OF graph from DSI node for exynos5433
>     dts
>   arm: dts: Remove the OF graph from DSI node for exynos3250 dts
>   arm: dts: Remove the OF graph from DSI node for exynos4412 dts
>   arm: dts: Remove the OF graph from DSI node for exynos4210 dts
> 
>  arch/arm/boot/dts/exynos3250-rinato.dts            | 23 ++--------------
>  arch/arm/boot/dts/exynos4210-trats.dts             | 23 ++--------------
>  arch/arm/boot/dts/exynos4412-trats2.dts            | 23 ++--------------
>  .../boot/dts/exynos/exynos5433-tm2-common.dtsi     | 16 ++---------
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c            | 32 ++++++----------------
>  5 files changed, 16 insertions(+), 101 deletions(-)
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/5] Fix the parse_dt of exynos dsi and remove the OF graph

[Krzysztof Kozlowski]

On Mon, Feb 20, 2017 at 9:07 AM, Inki Dae <inki.dae@samsung.com> wrote:
> Hi Krzysztof,
>
> Can you merge patch 2 and 5 to your tree so that they can go to mainline? Otherwise, I can merge them to my tree if you give me acked-by.

Hi,

Hm? Why do you need them to be merged? Do you mean that the first
patch breaks not only ABI but also bisectability? Then this has to be
reworked because:

1. The DTS patches cannot go through non-DT tree so no, you cannot
take them into your branch. I will merge them according to point 4
below.
2. Bisectability has to be preserved.
3. DTS changes in general should not be a dependency for driver changes.
4 . Usually DTS patches wait for driver to be acked by devicetree
maintainers or merging by driver maintainer. This did not happen so
DTS patches wait. Also these were sent too late to merge for this
cycle so the only chance is v4.12.

Best regards,
Krzysztof

Re: [PATCH v2 0/5] Fix the parse_dt of exynos dsi and remove the OF graph

[Inki Dae]

2017년 02월 20일 16:27에 Krzysztof Kozlowski 이(가) 쓴 글:
> On Mon, Feb 20, 2017 at 9:07 AM, Inki Dae <inki.dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> Hi Krzysztof,
>>
>> Can you merge patch 2 and 5 to your tree so that they can go to mainline? Otherwise, I can merge them to my tree if you give me acked-by.
> 
> Hi,
> 
> Hm? Why do you need them to be merged? Do you mean that the first
> patch breaks not only ABI but also bisectability? Then this has to be
> reworked because:
> 
> 1. The DTS patches cannot go through non-DT tree so no, you cannot
> take them into your branch. I will merge them according to point 4
> below.
> 2. Bisectability has to be preserved.
> 3. DTS changes in general should not be a dependency for driver changes.

Krzysztof,

I know that. I didn't check you gave a comment to Hoegun do rework this patch series. I just checked reviewed-by from Andrzej H. and thought no problem.
 
Ideally you are right. DT changes should be no any dependency of driver changes but now Exynos DT is broken.
So if we have to keep the bisectability between driver and device tree patches - this means that all drivers should always keep the backward compatibility - then the drivers will be messed up.

About this, we had a discussion and I think you agreed,
https://patchwork.kernel.org/patch/9477957/

Do you want the backward compatibility to be kept always when new dt binding relevant patch - which fixes up the wrong usage of dt binding - is posted?
It would be good for the backward compatibility to be kept as long as we can do but there would be several cases to make the driver to be messed up like this patch series.

> 4 . Usually DTS patches wait for driver to be acked by devicetree
> maintainers or merging by driver maintainer. This did not happen so
> DTS patches wait. Also these were sent too late to merge for this
> cycle so the only chance is v4.12.

Yes, maybe.


Thanks,
Inki Dae


> 
> Best regards,
> Krzysztof
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/5] Fix the parse_dt of exynos dsi and remove the OF graph

[Krzysztof Kozlowski]

On Mon, Feb 20, 2017 at 10:32 AM, Inki Dae <inki.dae@samsung.com> wrote:
>
>
> 2017년 02월 20일 16:27에 Krzysztof Kozlowski 이(가) 쓴 글:
>> On Mon, Feb 20, 2017 at 9:07 AM, Inki Dae <inki.dae@samsung.com> wrote:
>>> Hi Krzysztof,
>>>
>>> Can you merge patch 2 and 5 to your tree so that they can go to mainline? Otherwise, I can merge them to my tree if you give me acked-by.
>>
>> Hi,
>>
>> Hm? Why do you need them to be merged? Do you mean that the first
>> patch breaks not only ABI but also bisectability? Then this has to be
>> reworked because:
>>
>> 1. The DTS patches cannot go through non-DT tree so no, you cannot
>> take them into your branch. I will merge them according to point 4
>> below.
>> 2. Bisectability has to be preserved.
>> 3. DTS changes in general should not be a dependency for driver changes.
>
> Krzysztof,
>
> I know that. I didn't check you gave a comment to Hoegun do rework this patch series. I just checked reviewed-by from Andrzej H. and thought no problem.
>
> Ideally you are right. DT changes should be no any dependency of driver changes but now Exynos DT is broken.

I didn't receive any bug reports that Exynos DT is broken so I am
quite surprised hearing that. What do you mean by that?

> So if we have to keep the bisectability between driver and device tree patches - this means that all drivers should always keep the backward compatibility - then the drivers will be messed up.

No, you are mixing two things. Bisectability is different than
backward compatibility. For the backward DT ABI compatibility, we
agreed that in this case it can be dropped. However bisectability is
something totally different. The DTS changes always go through
separate branch, so the driver has to be *always* ready for such case
and should still be fully bisectable. This is a general rule existing
for long time and every deviation from the rule is pointed out by
arm-soc guys. Whenever I accepted breaking of this rule, I had hard
times with arm-soc so no - I am no longer giving up basic rule just
because developer might not care.

There are many ways to solve the bisectability problem. Recently for
example Marek found nice way to solve the PMU syscon handle dependency
[1]. One can also merge first DTS changes and in next release, merge
the driver.

> About this, we had a discussion and I think you agreed,
> https://patchwork.kernel.org/patch/9477957/

Again, this discussion was about DT ABI. Not bisectability.

> Do you want the backward compatibility to be kept always when new dt binding relevant patch - which fixes up the wrong usage of dt binding - is posted?
> It would be good for the backward compatibility to be kept as long as we can do but there would be several cases to make the driver to be messed up like this patch series.

Again, DT ABI not bisectability.

Best regards,
Krzysztof

[1] https://git.kernel.org/cgit/linux/kernel/git/krzk/linux.git/commit/?h=for-v4.11/drivers-soc-exynos-pmu-the-joy-never-ends&id=76640b84bd7a9d68c70d8bc8ecd02cdc4bd8855e

Re: [PATCH v2 3/5] arm: dts: Remove the OF graph from DSI node for exynos3250 dts

[Krzysztof Kozlowski]

On Thu, Feb 9, 2017 at 3:26 AM, Hoegeun Kwon <hoegeun.kwon@samsung.com> wrote:
> The OF graph is not needed because the panel is a child of dsi. So
> Remove the ports node and move burst and esc clock frequency
> properties to the parent (DSI node).
>
> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> ---
>  arch/arm/boot/dts/exynos3250-rinato.dts | 23 ++---------------------
>  1 file changed, 2 insertions(+), 21 deletions(-)
>

Please use scripts/get_maintainers.pl to get the list of people and
maling lists to CC.

Best regards,
Krzysztof

Re: [PATCH v2 0/5] Fix the parse_dt of exynos dsi and remove the OF graph

[Inki Dae]

2017년 02월 20일 17:55에 Krzysztof Kozlowski 이(가) 쓴 글:
> On Mon, Feb 20, 2017 at 10:32 AM, Inki Dae <inki.dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>
>>
>> 2017년 02월 20일 16:27에 Krzysztof Kozlowski 이(가) 쓴 글:
>>> On Mon, Feb 20, 2017 at 9:07 AM, Inki Dae <inki.dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>>> Hi Krzysztof,
>>>>
>>>> Can you merge patch 2 and 5 to your tree so that they can go to mainline? Otherwise, I can merge them to my tree if you give me acked-by.
>>>
>>> Hi,
>>>
>>> Hm? Why do you need them to be merged? Do you mean that the first
>>> patch breaks not only ABI but also bisectability? Then this has to be
>>> reworked because:
>>>
>>> 1. The DTS patches cannot go through non-DT tree so no, you cannot
>>> take them into your branch. I will merge them according to point 4
>>> below.
>>> 2. Bisectability has to be preserved.
>>> 3. DTS changes in general should not be a dependency for driver changes.
>>
>> Krzysztof,
>>
>> I know that. I didn't check you gave a comment to Hoegun do rework this patch series. I just checked reviewed-by from Andrzej H. and thought no problem.
>>
>> Ideally you are right. DT changes should be no any dependency of driver changes but now Exynos DT is broken.
> 
> I didn't receive any bug reports that Exynos DT is broken so I am
> quite surprised hearing that. What do you mean by that?

Wrong usage of Display relevant DT ABI is being fixed without the backward compatibility of the DT ABI. This means device driver doesn't consider old DT binding.
So the use of old DTB binary will make kernel booting not to work correctly.

> 
>> So if we have to keep the bisectability between driver and device tree patches - this means that all drivers should always keep the backward compatibility - then the drivers will be messed up.
> 
> No, you are mixing two things. Bisectability is different than
> backward compatibility. For the backward DT ABI compatibility, we
> agreed that in this case it can be dropped. However bisectability is
> something totally different. The DTS changes always go through
> separate branch, so the driver has to be *always* ready for such case
> and should still be fully bisectable. This is a general rule existing

I meant that if this patch series considers old DT binding to keep the bisectability, then this driver would be messed up because the driver should try to bind same properties at different places and one of these two cases should be bound.
So this driver doesn't consider the old DT binding, which means that the old DTB binary isn't compatible with this driver - exynos_drm_dsi.c.

This means if only one side is merged then this driver wouldn't work correctly. 

> for long time and every deviation from the rule is pointed out by
> arm-soc guys. Whenever I accepted breaking of this rule, I had hard
> times with arm-soc so no - I am no longer giving up basic rule just
> because developer might not care.

Understood and reasonable. Seems I just hurried up. For the bisectability, this driver will have a little messed up code temporarily, and this messed up code will be removed after dt change is merged.
This thing was really what I want to avoid - if even the messed up code should be keeped forever for the backward compatibility then it would be really terribble. :(


Do you have a better idea?

> 
> There are many ways to solve the bisectability problem. Recently for
> example Marek found nice way to solve the PMU syscon handle dependency

Thanks for share but this would be a different case.

Thanks,
Inki Dae

> [1]. One can also merge first DTS changes and in next release, merge
> the driver.
> 
>> About this, we had a discussion and I think you agreed,
>> https://patchwork.kernel.org/patch/9477957/
> 
> Again, this discussion was about DT ABI. Not bisectability.
> 
>> Do you want the backward compatibility to be kept always when new dt binding relevant patch - which fixes up the wrong usage of dt binding - is posted?
>> It would be good for the backward compatibility to be kept as long as we can do but there would be several cases to make the driver to be messed up like this patch series.
> 
> Again, DT ABI not bisectability.
> 
> Best regards,
> Krzysztof
> 
> [1] https://git.kernel.org/cgit/linux/kernel/git/krzk/linux.git/commit/?h=for-v4.11/drivers-soc-exynos-pmu-the-joy-never-ends&id=76640b84bd7a9d68c70d8bc8ecd02cdc4bd8855e
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/5] Fix the parse_dt of exynos dsi and remove the OF graph

[Krzysztof Kozlowski]

On Tue, Feb 21, 2017 at 1:24 AM, Inki Dae <inki.dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>> Ideally you are right. DT changes should be no any dependency of driver changes but now Exynos DT is broken.
>>
>> I didn't receive any bug reports that Exynos DT is broken so I am
>> quite surprised hearing that. What do you mean by that?
>
> Wrong usage of Display relevant DT ABI is being fixed without the backward compatibility of the DT ABI. This means device driver doesn't consider old DT binding.
> So the use of old DTB binary will make kernel booting not to work correctly.

First you wrote that Exynos DT ABI is broken. Now you mention that
wrong usage is being fixed and old DTB will not be supported. I really
don't get what was your idea. Either something is broken or not. This
is not a Shrodinger's cat.

>
>>
>>> So if we have to keep the bisectability between driver and device tree patches - this means that all drivers should always keep the backward compatibility - then the drivers will be messed up.
>>
>> No, you are mixing two things. Bisectability is different than
>> backward compatibility. For the backward DT ABI compatibility, we
>> agreed that in this case it can be dropped. However bisectability is
>> something totally different. The DTS changes always go through
>> separate branch, so the driver has to be *always* ready for such case
>> and should still be fully bisectable. This is a general rule existing
>
> I meant that if this patch series considers old DT binding to keep the bisectability, then this driver would be messed up because the driver should try to bind same properties at different places and one of these two cases should be bound.
> So this driver doesn't consider the old DT binding, which means that the old DTB binary isn't compatible with this driver - exynos_drm_dsi.c.

Which is wrong. The driver breaks bisectability on first commit.

Let me put it simple:
1. First commit breaks all existing in-kernel DTS. When someone tries
such commit during bisect, he will see some kind of failure. This
means that bisectability is broken on first commit, regardless whether
the rest of patches is on the same branch or not.

2. If the DTS are applied directly after first commit, the
bisectability breakage gap is one-commit wide.

3. DTS commits go to different tree and different branch. This is a
general rule. This means that bisectability breakage gap will be much
wider, span over different trees and branches. This is bad. Bad by
design.

4. Overall: these patches will break bisectability *always*. How big
the breakage will be, depends on way of applying.

5. My merge window was closed some weeks ago. I sent an private email
to Samsung folks, *including you*, on 24th of Jan reminding about
arm-soc merge cycle and that it will close soon. You did not respond.
Yesterday, v4.10 was released which opens Linus' merge window and now
you want to apply these commits. The worst possible timing.
Irresponsible.

So let me put it straight. Please fix the first commit so it will not
break bisectability.

> This means if only one side is merged then this driver wouldn't work correctly.
>
>> for long time and every deviation from the rule is pointed out by
>> arm-soc guys. Whenever I accepted breaking of this rule, I had hard
>> times with arm-soc so no - I am no longer giving up basic rule just
>> because developer might not care.
>
> Understood and reasonable. Seems I just hurried up. For the bisectability, this driver will have a little messed up code temporarily, and this messed up code will be removed after dt change is merged.

Not necessarily. Maybe this can be made in a smart way. I believed
that no one cared to keep it reasonable and that is a problem.

> This thing was really what I want to avoid - if even the messed up code should be keeped forever for the backward compatibility then it would be really terribble. :(

Again you are mixing DT ABI backward compatibility with bisectability.
There is no point of explaining this again...

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html