[PATCH v6 0/4] Add data-lanes and link-frequencies to dp_out endpoint

[PATCH v6 0/4] Add data-lanes and link-frequencies to dp_out endpoint

[Kuogee Hsieh]

Add DP both data-lanes and link-frequencies property to dp_out endpoint and support
functions to DP driver.

Kuogee Hsieh (4):
  arm64: dts: qcom: add data-lanes and link-freuencies into dp_out
    endpoint
  drm/msm/dp: parser data-lanes as property of dp_out endpoint
  drm/msm/dp: parser link-frequencies as property of dp_out endpoint
  drm/msm/dp: add support of max dp link rate

 .../bindings/display/msm/dp-controller.yaml        | 17 ++++++++
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi       |  6 ++-
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi     |  6 ++-
 drivers/gpu/drm/msm/dp/dp_display.c                |  4 ++
 drivers/gpu/drm/msm/dp/dp_panel.c                  |  7 ++--
 drivers/gpu/drm/msm/dp/dp_panel.h                  |  1 +
 drivers/gpu/drm/msm/dp/dp_parser.c                 | 46 ++++++++++++++++++----
 drivers/gpu/drm/msm/dp/dp_parser.h                 |  2 +
 8 files changed, 77 insertions(+), 12 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Kuogee Hsieh]

Move data-lanes property from mdss_dp node to dp_out endpoint. Also
add link-frequencies property into dp_out endpoint as well. The last
frequency specified at link-frequencies will be the max link rate
supported by DP.

Changes in v5:
-- revert changes at sc7180.dtsi and sc7280.dtsi
-- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi

Changes in v6:
-- add data-lanes and link-frequencies to yaml

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 .../devicetree/bindings/display/msm/dp-controller.yaml  | 17 +++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi            |  6 +++++-
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi          |  6 +++++-
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index 94bc6e1..af70343 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -90,6 +90,20 @@ properties:
         $ref: /schemas/graph.yaml#/properties/port
         description: Output endpoint of the controller
 
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+
+          properties:
+            link-frequencies: true
+            data-lanes: true
+
+          required:
+            - link-frequencies
+            - data-lanes
+
+          additionalProperties: false
+
 required:
   - compatible
   - reg
@@ -158,6 +172,9 @@ examples:
                 reg = <1>;
                 endpoint {
                     remote-endpoint = <&typec>;
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <160000000 270000000
+                                                  540000000 810000000>;
                 };
             };
         };
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 754d2d6..39f0844 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -812,7 +812,11 @@ hp_i2c: &i2c9 {
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&dp_hot_plug_det>;
-	data-lanes = <0 1>;
+};
+
+&dp_out {
+    data-lanes = <0  1>;
+    link-frequencies = /bits/ 64 <160000000 270000000 540000000>;
 };
 
 &pm6150_adc {
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 93e39fc..b7c343d 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -440,7 +440,11 @@ ap_i2c_tpm: &i2c14 {
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&dp_hot_plug_det>;
-	data-lanes = <0 1>;
+};
+
+&dp_out {
+	data-lanes = <0  1>;
+	link-frequencies = /bits/ 64 <160000000 270000000 540000000 810000000>;
 };
 
 &mdss_mdp {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 2/4] drm/msm/dp: parser data-lanes as property of dp_out endpoint

[Kuogee Hsieh]

Add capability to parser data-lanes as property of dp_out endpoint.
Also retain the original capability to parser data-lanes as property
of mdss_dp node to handle legacy case.

Changes in v6:
-- first patch after split parser patch into two

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_parser.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index dd73221..b06ff60 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -94,16 +94,32 @@ static int dp_parser_ctrl_res(struct dp_parser *parser)
 static int dp_parser_misc(struct dp_parser *parser)
 {
 	struct device_node *of_node = parser->pdev->dev.of_node;
-	int len;
+	struct device_node *endpoint;
+	int cnt;
+
+	/*
+	 * legacy code, data-lanes is the property of mdss_dp node
+	 */
+	cnt = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
+	if (cnt > 0) {
+		parser->max_dp_lanes = cnt;
+		return 0;
+	}
 
-	len = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
-	if (len < 0) {
-		DRM_WARN("Invalid property \"data-lanes\", default max DP lanes = %d\n",
-			 DP_MAX_NUM_DP_LANES);
-		len = DP_MAX_NUM_DP_LANES;
+	/*
+	 * data-lanes is the property of dp_out endpoint
+	 */
+	endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* port@1 */
+	if (endpoint) {
+		cnt = of_property_count_u32_elems(endpoint, "data-lanes");
+		if (cnt < 0)
+			parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
+		else
+			parser->max_dp_lanes = cnt;
+	} else {
+		parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
 	}
 
-	parser->max_dp_lanes = len;
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 3/4] drm/msm/dp: parser link-frequencies as property of dp_out endpoint

[Kuogee Hsieh]

Add capability to parser and retrieve max DP link supported rate from
link-frequencies property of dp_out endpoint.

Changes in v6:
-- second patch after split parser patch into two patches

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_parser.c | 16 ++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_parser.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index b06ff60..2006341 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -95,6 +95,7 @@ static int dp_parser_misc(struct dp_parser *parser)
 {
 	struct device_node *of_node = parser->pdev->dev.of_node;
 	struct device_node *endpoint;
+	u64 frequency;
 	int cnt;
 
 	/*
@@ -103,6 +104,7 @@ static int dp_parser_misc(struct dp_parser *parser)
 	cnt = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
 	if (cnt > 0) {
 		parser->max_dp_lanes = cnt;
+		parser->max_dp_link_rate = DP_LINK_RATE_HBR2; /* 540000 khz */
 		return 0;
 	}
 
@@ -116,8 +118,22 @@ static int dp_parser_misc(struct dp_parser *parser)
 			parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
 		else
 			parser->max_dp_lanes = cnt;
+
+		cnt = of_property_count_u64_elems(endpoint, "link-frequencies");
+		if (cnt < 0) {
+			parser->max_dp_link_rate = DP_LINK_RATE_HBR2; /* 540000 khz */
+		} else {
+			if (cnt > DP_MAX_NUM_DP_LANES)
+				cnt = DP_MAX_NUM_DP_LANES;
+
+			of_property_read_u64_index(endpoint, "link-frequencies",
+							cnt - 1, &frequency);
+
+			parser->max_dp_link_rate = (frequency / 1000); /* kbits */
+		}
 	} else {
 		parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
+		parser->max_dp_link_rate = DP_LINK_RATE_HBR2; /* 540000 khz */
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 866c1a8..3ddf639 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -15,6 +15,7 @@
 #define DP_LABEL "MDSS DP DISPLAY"
 #define DP_MAX_PIXEL_CLK_KHZ	675000
 #define DP_MAX_NUM_DP_LANES	4
+#define DP_LINK_RATE_HBR2       540000
 
 enum dp_pm_type {
 	DP_CORE_PM,
@@ -119,6 +120,7 @@ struct dp_parser {
 	struct dp_io io;
 	struct dp_display_data disp_data;
 	u32 max_dp_lanes;
+	u32 max_dp_link_rate;
 	struct drm_bridge *next_bridge;
 
 	int (*parse)(struct dp_parser *parser);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 4/4] drm/msm/dp: add support of max dp link rate

[Kuogee Hsieh]

By default, HBR2 (5.4G) is the max link link be supported. This patch add
the capability to support max link rate at HBR3 (8.1G).

Changes in v2:
-- add max link rate from dtsi

Changes in v3:
-- parser max_data_lanes and max_dp_link_rate from dp_out endpoint

Changes in v4:
-- delete unnecessary pr_err

Changes in v5:
-- split parser function into different patch

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 4 ++++
 drivers/gpu/drm/msm/dp/dp_panel.c   | 7 ++++---
 drivers/gpu/drm/msm/dp/dp_panel.h   | 1 +
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 29c9845..4fe2092 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -390,6 +390,10 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
 	struct edid *edid;
 
 	dp->panel->max_dp_lanes = dp->parser->max_dp_lanes;
+	dp->panel->max_dp_link_rate = dp->parser->max_dp_link_rate;
+
+	drm_dbg_dp(dp->drm_dev, "max_lanes=%d max_link_rate=%d\n",
+		dp->panel->max_dp_lanes, dp->panel->max_dp_link_rate);
 
 	rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector);
 	if (rc)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 5149ceb..933fa9c 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -75,12 +75,13 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
 	link_info->rate = drm_dp_bw_code_to_link_rate(dpcd[DP_MAX_LINK_RATE]);
 	link_info->num_lanes = dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK;
 
+	/* Limit data lanes from data-lanes of endpoint properity of dtsi */
 	if (link_info->num_lanes > dp_panel->max_dp_lanes)
 		link_info->num_lanes = dp_panel->max_dp_lanes;
 
-	/* Limit support upto HBR2 until HBR3 support is added */
-	if (link_info->rate >= (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4)))
-		link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4);
+	/* Limit link rate from link-frequencies of endpoint properity of dtsi */
+	if (link_info->rate > dp_panel->max_dp_link_rate)
+		link_info->rate = dp_panel->max_dp_link_rate;
 
 	drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
 	drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index d861197a..f04d021 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -50,6 +50,7 @@ struct dp_panel {
 
 	u32 vic;
 	u32 max_dp_lanes;
+	u32 max_dp_link_rate;
 
 	u32 max_bw_code;
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 2/4] drm/msm/dp: parser data-lanes as property of dp_out endpoint

[Dmitry Baryshkov]

On 01/12/2022 01:51, Kuogee Hsieh wrote:
> Add capability to parser data-lanes as property of dp_out endpoint.
> Also retain the original capability to parser data-lanes as property
> of mdss_dp node to handle legacy case.
> 
> Changes in v6:
> -- first patch after split parser patch into two
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_parser.c | 30 +++++++++++++++++++++++-------
>   1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index dd73221..b06ff60 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -94,16 +94,32 @@ static int dp_parser_ctrl_res(struct dp_parser *parser)
>   static int dp_parser_misc(struct dp_parser *parser)
>   {
>   	struct device_node *of_node = parser->pdev->dev.of_node;
> -	int len;
> +	struct device_node *endpoint;
> +	int cnt;
> +
> +	/*
> +	 * legacy code, data-lanes is the property of mdss_dp node
> +	 */
> +	cnt = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
> +	if (cnt > 0) {
> +		parser->max_dp_lanes = cnt;
> +		return 0;
> +	}
>   
> -	len = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
> -	if (len < 0) {
> -		DRM_WARN("Invalid property \"data-lanes\", default max DP lanes = %d\n",
> -			 DP_MAX_NUM_DP_LANES);
> -		len = DP_MAX_NUM_DP_LANES;
> +	/*
> +	 * data-lanes is the property of dp_out endpoint
> +	 */
> +	endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* port@1 */
> +	if (endpoint) {
> +		cnt = of_property_count_u32_elems(endpoint, "data-lanes");

drm_of_get_data_lanes_count(), or better drm_of_get_data_lanes_count_ep().

Also please check new property first, then check the legacy one.

So it would be:

cnt = drm_of_get_data_lanes_count_ep();
if (cnt < 0)
     cnt = drm_of_get_data_lanes_count();
if (cnt < 0) {
     DRM_WARN(...);
     cnt = DP_MAX_NUM_DP_LANES;
}


> +		if (cnt < 0)
> +			parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
> +		else
> +			parser->max_dp_lanes = cnt;
> +	} else {
> +		parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
>   	}
>   
> -	parser->max_dp_lanes = len;
>   	return 0;
>   }
>   

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Dmitry Baryshkov]

On 01/12/2022 01:51, Kuogee Hsieh wrote:
> Move data-lanes property from mdss_dp node to dp_out endpoint. Also
> add link-frequencies property into dp_out endpoint as well. The last
> frequency specified at link-frequencies will be the max link rate
> supported by DP.
> 
> Changes in v5:
> -- revert changes at sc7180.dtsi and sc7280.dtsi
> -- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi
> 
> Changes in v6:
> -- add data-lanes and link-frequencies to yaml
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   .../devicetree/bindings/display/msm/dp-controller.yaml  | 17 +++++++++++++++++

Separate patch. Also you didn't check the get_maintainers output, so 
required parties were not included into the distribution.

Also as you'd check the get_maintainers output, please fix other email 
addresses too.

>   arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi            |  6 +++++-
>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi          |  6 +++++-
>   3 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 94bc6e1..af70343 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -90,6 +90,20 @@ properties:
>           $ref: /schemas/graph.yaml#/properties/port
>           description: Output endpoint of the controller
>   
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +
> +          properties:
> +            link-frequencies: true
> +            data-lanes: true

No. Use $ref for both of them.

> +
> +          required:
> +            - link-frequencies
> +            - data-lanes

No, they are not required.

> +
> +          additionalProperties: false
> +

deprecation of old data-lanes property?

>   required:
>     - compatible
>     - reg
> @@ -158,6 +172,9 @@ examples:
>                   reg = <1>;
>                   endpoint {
>                       remote-endpoint = <&typec>;
> +                    data-lanes = <1 2>;
> +                    link-frequencies = /bits/ 64 <160000000 270000000
> +                                                  540000000 810000000>;

I guess the number of zeroes is wrong here. This is 160 MHz ... 810 Mhz, 
rather than 1.6 GHz ... 8.1 GHz

>                   };
>               };
>           };
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index 754d2d6..39f0844 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -812,7 +812,11 @@ hp_i2c: &i2c9 {
>   	status = "okay";
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&dp_hot_plug_det>;
> -	data-lanes = <0 1>;
> +};
> +
> +&dp_out {
> +    data-lanes = <0  1>;
> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000>;

Same comment here.

>   };
>   
>   &pm6150_adc {
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> index 93e39fc..b7c343d 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> @@ -440,7 +440,11 @@ ap_i2c_tpm: &i2c14 {
>   	status = "okay";
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&dp_hot_plug_det>;
> -	data-lanes = <0 1>;
> +};
> +
> +&dp_out {
> +	data-lanes = <0  1>;
> +	link-frequencies = /bits/ 64 <160000000 270000000 540000000 810000000>;

And here.

>   };
>   
>   &mdss_mdp {

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Dmitry Baryshkov]

On 01/12/2022 02:07, Dmitry Baryshkov wrote:
> On 01/12/2022 01:51, Kuogee Hsieh wrote:
>> Move data-lanes property from mdss_dp node to dp_out endpoint. Also
>> add link-frequencies property into dp_out endpoint as well. The last
>> frequency specified at link-frequencies will be the max link rate
>> supported by DP.
>>
>> Changes in v5:
>> -- revert changes at sc7180.dtsi and sc7280.dtsi
>> -- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi
>>
>> Changes in v6:
>> -- add data-lanes and link-frequencies to yaml
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   .../devicetree/bindings/display/msm/dp-controller.yaml  | 17 
>> +++++++++++++++++
> 
> Separate patch. Also you didn't check the get_maintainers output, so 
> required parties were not included into the distribution.
> 
> Also as you'd check the get_maintainers output, please fix other email 
> addresses too.
> 
>>   arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi            |  6 +++++-
>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi          |  6 +++++-
>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> index 94bc6e1..af70343 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -90,6 +90,20 @@ properties:
>>           $ref: /schemas/graph.yaml#/properties/port
>>           description: Output endpoint of the controller
>> +        properties:
>> +          endpoint:
>> +            $ref: /schemas/media/video-interfaces.yaml#
>> +
>> +          properties:
>> +            link-frequencies: true
>> +            data-lanes: true
> 
> No. Use $ref for both of them.
> 
>> +
>> +          required:
>> +            - link-frequencies
>> +            - data-lanes
> 
> No, they are not required.
> 
>> +
>> +          additionalProperties: false
>> +
> 
> deprecation of old data-lanes property?
> 
>>   required:
>>     - compatible
>>     - reg
>> @@ -158,6 +172,9 @@ examples:
>>                   reg = <1>;
>>                   endpoint {
>>                       remote-endpoint = <&typec>;
>> +                    data-lanes = <1 2>;
>> +                    link-frequencies = /bits/ 64 <160000000 270000000

s/1600/1620

>> +                                                  540000000 810000000>;
> 
> I guess the number of zeroes is wrong here. This is 160 MHz ... 810 Mhz, 
> rather than 1.6 GHz ... 8.1 GHz

Ok, I was wrong here. The old code definitely defaults to 570 
mega-something. Now I'd really like to read your description for the 
link-frequencies property, because the phy_configure_opts_dp::link_rate 
is clearly specified in Mb/s and it takes a fixed set of values from 
1.62 Gb/s up to 8.1 Gb/s.

I think the drm_dp_bw_code_to_link_rate() function is incorrect by 
itself, as it multiplies with 27000 (27 Mbps) rather than 270000 (0.27 
Gbps) as required by the standard. So first, we should fix the function, 
then all the rates would become logical.


> 
>>                   };
>>               };
>>           };
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> index 754d2d6..39f0844 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> @@ -812,7 +812,11 @@ hp_i2c: &i2c9 {
>>       status = "okay";
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&dp_hot_plug_det>;
>> -    data-lanes = <0 1>;
>> +};
>> +
>> +&dp_out {
>> +    data-lanes = <0  1>;
>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000>;
> 
> Same comment here.
> 
>>   };
>>   &pm6150_adc {
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> index 93e39fc..b7c343d 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> @@ -440,7 +440,11 @@ ap_i2c_tpm: &i2c14 {
>>       status = "okay";
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&dp_hot_plug_det>;
>> -    data-lanes = <0 1>;
>> +};
>> +
>> +&dp_out {
>> +    data-lanes = <0  1>;
>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000 
>> 810000000>;
> 
> And here.
> 
>>   };
>>   &mdss_mdp {
> 

-- 
With best wishes
Dmitry

Re: [PATCH v6 3/4] drm/msm/dp: parser link-frequencies as property of dp_out endpoint

[Dmitry Baryshkov]

On 01/12/2022 01:51, Kuogee Hsieh wrote:
> Add capability to parser and retrieve max DP link supported rate from
> link-frequencies property of dp_out endpoint.
> 
> Changes in v6:
> -- second patch after split parser patch into two patches
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_parser.c | 16 ++++++++++++++++
>   drivers/gpu/drm/msm/dp/dp_parser.h |  2 ++
>   2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index b06ff60..2006341 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -95,6 +95,7 @@ static int dp_parser_misc(struct dp_parser *parser)
>   {
>   	struct device_node *of_node = parser->pdev->dev.of_node;
>   	struct device_node *endpoint;
> +	u64 frequency;
>   	int cnt;
>   
>   	/*
> @@ -103,6 +104,7 @@ static int dp_parser_misc(struct dp_parser *parser)
>   	cnt = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
>   	if (cnt > 0) {
>   		parser->max_dp_lanes = cnt;
> +		parser->max_dp_link_rate = DP_LINK_RATE_HBR2; /* 540000 khz */
>   		return 0;
>   	}
>   
> @@ -116,8 +118,22 @@ static int dp_parser_misc(struct dp_parser *parser)
>   			parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
>   		else
>   			parser->max_dp_lanes = cnt;
> +
> +		cnt = of_property_count_u64_elems(endpoint, "link-frequencies");
> +		if (cnt < 0) {
> +			parser->max_dp_link_rate = DP_LINK_RATE_HBR2; /* 540000 khz */
> +		} else {
> +			if (cnt > DP_MAX_NUM_DP_LANES)
> +				cnt = DP_MAX_NUM_DP_LANES;

Why are you comparing the number of link frequencies with the number of 
lanes? You don't need the comparison at all.

> +
> +			of_property_read_u64_index(endpoint, "link-frequencies",
> +							cnt - 1, &frequency);

Checking of the return value?

> +
> +			parser->max_dp_link_rate = (frequency / 1000); /* kbits */
> +		}
>   	} else {
>   		parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
> +		parser->max_dp_link_rate = DP_LINK_RATE_HBR2; /* 540000 khz */

Instead of having all the nested if's and setting of max_dp_link rate in 
several branches, please add a function that returns either a valid rate 
or an error. Then you can simply check the result of that function and 
set the default.

>   	}
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 866c1a8..3ddf639 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -15,6 +15,7 @@
>   #define DP_LABEL "MDSS DP DISPLAY"
>   #define DP_MAX_PIXEL_CLK_KHZ	675000
>   #define DP_MAX_NUM_DP_LANES	4
> +#define DP_LINK_RATE_HBR2       540000
>   
>   enum dp_pm_type {
>   	DP_CORE_PM,
> @@ -119,6 +120,7 @@ struct dp_parser {
>   	struct dp_io io;
>   	struct dp_display_data disp_data;
>   	u32 max_dp_lanes;
> +	u32 max_dp_link_rate;
>   	struct drm_bridge *next_bridge;
>   
>   	int (*parse)(struct dp_parser *parser);

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Kuogee Hsieh]

On 11/30/2022 4:21 PM, Dmitry Baryshkov wrote:
> On 01/12/2022 02:07, Dmitry Baryshkov wrote:
>> On 01/12/2022 01:51, Kuogee Hsieh wrote:
>>> Move data-lanes property from mdss_dp node to dp_out endpoint. Also
>>> add link-frequencies property into dp_out endpoint as well. The last
>>> frequency specified at link-frequencies will be the max link rate
>>> supported by DP.
>>>
>>> Changes in v5:
>>> -- revert changes at sc7180.dtsi and sc7280.dtsi
>>> -- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi
>>>
>>> Changes in v6:
>>> -- add data-lanes and link-frequencies to yaml
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/display/msm/dp-controller.yaml  | 17 
>>> +++++++++++++++++
>>
>> Separate patch. Also you didn't check the get_maintainers output, so 
>> required parties were not included into the distribution.
>>
>> Also as you'd check the get_maintainers output, please fix other 
>> email addresses too.
>>
>>> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi            |  6 +++++-
>>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi          |  6 +++++-
>>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> index 94bc6e1..af70343 100644
>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> @@ -90,6 +90,20 @@ properties:
>>>           $ref: /schemas/graph.yaml#/properties/port
>>>           description: Output endpoint of the controller
>>> +        properties:
>>> +          endpoint:
>>> +            $ref: /schemas/media/video-interfaces.yaml#
>>> +
>>> +          properties:
>>> +            link-frequencies: true
>>> +            data-lanes: true
>>
>> No. Use $ref for both of them.
>>
>>> +
>>> +          required:
>>> +            - link-frequencies
>>> +            - data-lanes
>>
>> No, they are not required.
>>
>>> +
>>> +          additionalProperties: false
>>> +
>>
>> deprecation of old data-lanes property?
>>
>>>   required:
>>>     - compatible
>>>     - reg
>>> @@ -158,6 +172,9 @@ examples:
>>>                   reg = <1>;
>>>                   endpoint {
>>>                       remote-endpoint = <&typec>;
>>> +                    data-lanes = <1 2>;
>>> +                    link-frequencies = /bits/ 64 <160000000 270000000
>
> s/1600/1620
>
>>> + 540000000 810000000>;
>>
>> I guess the number of zeroes is wrong here. This is 160 MHz ... 810 
>> Mhz, rather than 1.6 GHz ... 8.1 GHz
>
> Ok, I was wrong here. The old code definitely defaults to 570 
> mega-something. Now I'd really like to read your description for the 
> link-frequencies property, because the 
> phy_configure_opts_dp::link_rate is clearly specified in Mb/s and it 
> takes a fixed set of values from 1.62 Gb/s up to 8.1 Gb/s.
>
> I think the drm_dp_bw_code_to_link_rate() function is incorrect by 
> itself, as it multiplies with 27000 (27 Mbps) rather than 270000 (0.27 
> Gbps) as required by the standard. So first, we should fix the 
> function, then all the rates would become logical.

no, drm_dp_bw_code_to_link_rate() is correct and should not be changes 
since it impact to other dp drivers too.

0.27Gbps/lane is specified at DP spec.

DP use 8b/10b coding rule (10 bits symbol contains 8 bits data).


>
>
>>
>>>                   };
>>>               };
>>>           };
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>> index 754d2d6..39f0844 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>> @@ -812,7 +812,11 @@ hp_i2c: &i2c9 {
>>>       status = "okay";
>>>       pinctrl-names = "default";
>>>       pinctrl-0 = <&dp_hot_plug_det>;
>>> -    data-lanes = <0 1>;
>>> +};
>>> +
>>> +&dp_out {
>>> +    data-lanes = <0  1>;
>>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000>;
>>
>> Same comment here.
>>
>>>   };
>>>   &pm6150_adc {
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> index 93e39fc..b7c343d 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> @@ -440,7 +440,11 @@ ap_i2c_tpm: &i2c14 {
>>>       status = "okay";
>>>       pinctrl-names = "default";
>>>       pinctrl-0 = <&dp_hot_plug_det>;
>>> -    data-lanes = <0 1>;
>>> +};
>>> +
>>> +&dp_out {
>>> +    data-lanes = <0  1>;
>>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000 
>>> 810000000>;
>>
>> And here.
>>
>>>   };
>>>   &mdss_mdp {
>>
>

Re: [PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Kuogee Hsieh]

On 11/30/2022 4:07 PM, Dmitry Baryshkov wrote:
> On 01/12/2022 01:51, Kuogee Hsieh wrote:
>> Move data-lanes property from mdss_dp node to dp_out endpoint. Also
>> add link-frequencies property into dp_out endpoint as well. The last
>> frequency specified at link-frequencies will be the max link rate
>> supported by DP.
>>
>> Changes in v5:
>> -- revert changes at sc7180.dtsi and sc7280.dtsi
>> -- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi
>>
>> Changes in v6:
>> -- add data-lanes and link-frequencies to yaml
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   .../devicetree/bindings/display/msm/dp-controller.yaml  | 17 
>> +++++++++++++++++
>
> Separate patch. Also you didn't check the get_maintainers output, so 
> required parties were not included into the distribution.
>
> Also as you'd check the get_maintainers output, please fix other email 
> addresses too.
>
>> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi            |  6 +++++-
>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi          |  6 +++++-
>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> index 94bc6e1..af70343 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -90,6 +90,20 @@ properties:
>>           $ref: /schemas/graph.yaml#/properties/port
>>           description: Output endpoint of the controller
>>   +        properties:
>> +          endpoint:
>> +            $ref: /schemas/media/video-interfaces.yaml#
>> +
>> +          properties:
>> +            link-frequencies: true
>> +            data-lanes: true
>
> No. Use $ref for both of them.
>
>> +
>> +          required:
>> +            - link-frequencies
>> +            - data-lanes
>
> No, they are not required.
>
>> +
>> +          additionalProperties: false
>> +
>
> deprecation of old data-lanes property?
there is no old data-lanes property.
>
>>   required:
>>     - compatible
>>     - reg
>> @@ -158,6 +172,9 @@ examples:
>>                   reg = <1>;
>>                   endpoint {
>>                       remote-endpoint = <&typec>;
>> +                    data-lanes = <1 2>;
>> +                    link-frequencies = /bits/ 64 <160000000 270000000
>> +                                                  540000000 810000000>;
>
> I guess the number of zeroes is wrong here. This is 160 MHz ... 810 
> Mhz, rather than 1.6 GHz ... 8.1 GHz
>
>>                   };
>>               };
>>           };
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> index 754d2d6..39f0844 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> @@ -812,7 +812,11 @@ hp_i2c: &i2c9 {
>>       status = "okay";
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&dp_hot_plug_det>;
>> -    data-lanes = <0 1>;
>> +};
>> +
>> +&dp_out {
>> +    data-lanes = <0  1>;
>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000>;
>
> Same comment here.
>
>>   };
>>     &pm6150_adc {
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> index 93e39fc..b7c343d 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> @@ -440,7 +440,11 @@ ap_i2c_tpm: &i2c14 {
>>       status = "okay";
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&dp_hot_plug_det>;
>> -    data-lanes = <0 1>;
>> +};
>> +
>> +&dp_out {
>> +    data-lanes = <0  1>;
>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000 
>> 810000000>;
>
> And here.
>
>>   };
>>     &mdss_mdp {
>

Re: [PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Dmitry Baryshkov]

On 01/12/2022 19:34, Kuogee Hsieh wrote:
> 
> On 11/30/2022 4:07 PM, Dmitry Baryshkov wrote:
>> On 01/12/2022 01:51, Kuogee Hsieh wrote:
>>> Move data-lanes property from mdss_dp node to dp_out endpoint. Also
>>> add link-frequencies property into dp_out endpoint as well. The last
>>> frequency specified at link-frequencies will be the max link rate
>>> supported by DP.
>>>
>>> Changes in v5:
>>> -- revert changes at sc7180.dtsi and sc7280.dtsi
>>> -- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi
>>>
>>> Changes in v6:
>>> -- add data-lanes and link-frequencies to yaml
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/display/msm/dp-controller.yaml  | 17 
>>> +++++++++++++++++
>>
>> Separate patch. Also you didn't check the get_maintainers output, so 
>> required parties were not included into the distribution.
>>
>> Also as you'd check the get_maintainers output, please fix other email 
>> addresses too.
>>
>>> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi            |  6 +++++-
>>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi          |  6 +++++-
>>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> index 94bc6e1..af70343 100644
>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> @@ -90,6 +90,20 @@ properties:
>>>           $ref: /schemas/graph.yaml#/properties/port
>>>           description: Output endpoint of the controller
>>>   +        properties:
>>> +          endpoint:
>>> +            $ref: /schemas/media/video-interfaces.yaml#
>>> +
>>> +          properties:
>>> +            link-frequencies: true
>>> +            data-lanes: true
>>
>> No. Use $ref for both of them.
>>
>>> +
>>> +          required:
>>> +            - link-frequencies
>>> +            - data-lanes
>>
>> No, they are not required.
>>
>>> +
>>> +          additionalProperties: false
>>> +
>>
>> deprecation of old data-lanes property?
> there is no old data-lanes property.

There is one:

$ grep -n data-lanes 
Documentation/devicetree/bindings/display/msm/dp-controller.yaml
82:  data-lanes:


-- 
With best wishes
Dmitry

Re: [PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Dmitry Baryshkov]

On 01/12/2022 19:32, Kuogee Hsieh wrote:
> 
> On 11/30/2022 4:21 PM, Dmitry Baryshkov wrote:
>> On 01/12/2022 02:07, Dmitry Baryshkov wrote:
>>> On 01/12/2022 01:51, Kuogee Hsieh wrote:
>>>> Move data-lanes property from mdss_dp node to dp_out endpoint. Also
>>>> add link-frequencies property into dp_out endpoint as well. The last
>>>> frequency specified at link-frequencies will be the max link rate
>>>> supported by DP.
>>>>
>>>> Changes in v5:
>>>> -- revert changes at sc7180.dtsi and sc7280.dtsi
>>>> -- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi
>>>>
>>>> Changes in v6:
>>>> -- add data-lanes and link-frequencies to yaml
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>   .../devicetree/bindings/display/msm/dp-controller.yaml  | 17 
>>>> +++++++++++++++++
>>>
>>> Separate patch. Also you didn't check the get_maintainers output, so 
>>> required parties were not included into the distribution.
>>>
>>> Also as you'd check the get_maintainers output, please fix other 
>>> email addresses too.
>>>
>>>> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi            |  6 +++++-
>>>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi          |  6 +++++-
>>>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
>>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> index 94bc6e1..af70343 100644
>>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> @@ -90,6 +90,20 @@ properties:
>>>>           $ref: /schemas/graph.yaml#/properties/port
>>>>           description: Output endpoint of the controller
>>>> +        properties:
>>>> +          endpoint:
>>>> +            $ref: /schemas/media/video-interfaces.yaml#
>>>> +
>>>> +          properties:
>>>> +            link-frequencies: true
>>>> +            data-lanes: true
>>>
>>> No. Use $ref for both of them.
>>>
>>>> +
>>>> +          required:
>>>> +            - link-frequencies
>>>> +            - data-lanes
>>>
>>> No, they are not required.
>>>
>>>> +
>>>> +          additionalProperties: false
>>>> +
>>>
>>> deprecation of old data-lanes property?
>>>
>>>>   required:
>>>>     - compatible
>>>>     - reg
>>>> @@ -158,6 +172,9 @@ examples:
>>>>                   reg = <1>;
>>>>                   endpoint {
>>>>                       remote-endpoint = <&typec>;
>>>> +                    data-lanes = <1 2>;
>>>> +                    link-frequencies = /bits/ 64 <160000000 270000000
>>
>> s/1600/1620
>>
>>>> + 540000000 810000000>;
>>>
>>> I guess the number of zeroes is wrong here. This is 160 MHz ... 810 
>>> Mhz, rather than 1.6 GHz ... 8.1 GHz
>>
>> Ok, I was wrong here. The old code definitely defaults to 570 
>> mega-something. Now I'd really like to read your description for the 
>> link-frequencies property, because the 
>> phy_configure_opts_dp::link_rate is clearly specified in Mb/s and it 
>> takes a fixed set of values from 1.62 Gb/s up to 8.1 Gb/s.
>>
>> I think the drm_dp_bw_code_to_link_rate() function is incorrect by 
>> itself, as it multiplies with 27000 (27 Mbps) rather than 270000 (0.27 
>> Gbps) as required by the standard. So first, we should fix the 
>> function, then all the rates would become logical.
> 
> no, drm_dp_bw_code_to_link_rate() is correct and should not be changes 
> since it impact to other dp drivers too.
> 
> 0.27Gbps/lane is specified at DP spec.
> 
> DP use 8b/10b coding rule (10 bits symbol contains 8 bits data).

At least it should get documentation that it returns Kylo-bytes per second.

But, getting back to link-frequencies. The documentation clearly says 
that it should be allowed data bus _frequencies_. And frequencies are 
measured in Hz, not in bits/sec or bytes/sec.


>>>>                   };
>>>>               };
>>>>           };
>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>>>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>> index 754d2d6..39f0844 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>> @@ -812,7 +812,11 @@ hp_i2c: &i2c9 {
>>>>       status = "okay";
>>>>       pinctrl-names = "default";
>>>>       pinctrl-0 = <&dp_hot_plug_det>;
>>>> -    data-lanes = <0 1>;
>>>> +};
>>>> +
>>>> +&dp_out {
>>>> +    data-lanes = <0  1>;
>>>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000>;
>>>
>>> Same comment here.
>>>
>>>>   };
>>>>   &pm6150_adc {
>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
>>>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>> index 93e39fc..b7c343d 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>> @@ -440,7 +440,11 @@ ap_i2c_tpm: &i2c14 {
>>>>       status = "okay";
>>>>       pinctrl-names = "default";
>>>>       pinctrl-0 = <&dp_hot_plug_det>;
>>>> -    data-lanes = <0 1>;
>>>> +};
>>>> +
>>>> +&dp_out {
>>>> +    data-lanes = <0  1>;
>>>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000 
>>>> 810000000>;
>>>
>>> And here.
>>>
>>>>   };
>>>>   &mdss_mdp {
>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Kuogee Hsieh]

On 12/1/2022 9:49 AM, Dmitry Baryshkov wrote:
> On 01/12/2022 19:32, Kuogee Hsieh wrote:
>>
>> On 11/30/2022 4:21 PM, Dmitry Baryshkov wrote:
>>> On 01/12/2022 02:07, Dmitry Baryshkov wrote:
>>>> On 01/12/2022 01:51, Kuogee Hsieh wrote:
>>>>> Move data-lanes property from mdss_dp node to dp_out endpoint. Also
>>>>> add link-frequencies property into dp_out endpoint as well. The last
>>>>> frequency specified at link-frequencies will be the max link rate
>>>>> supported by DP.
>>>>>
>>>>> Changes in v5:
>>>>> -- revert changes at sc7180.dtsi and sc7280.dtsi
>>>>> -- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi
>>>>>
>>>>> Changes in v6:
>>>>> -- add data-lanes and link-frequencies to yaml
>>>>>
>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> ---
>>>>>   .../devicetree/bindings/display/msm/dp-controller.yaml | 17 
>>>>> +++++++++++++++++
>>>>
>>>> Separate patch. Also you didn't check the get_maintainers output, 
>>>> so required parties were not included into the distribution.
>>>>
>>>> Also as you'd check the get_maintainers output, please fix other 
>>>> email addresses too.
>>>>
>>>>> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |  6 +++++-
>>>>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  6 +++++-
>>>>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git 
>>>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
>>>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>>> index 94bc6e1..af70343 100644
>>>>> --- 
>>>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>>> +++ 
>>>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>>> @@ -90,6 +90,20 @@ properties:
>>>>>           $ref: /schemas/graph.yaml#/properties/port
>>>>>           description: Output endpoint of the controller
>>>>> +        properties:
>>>>> +          endpoint:
>>>>> +            $ref: /schemas/media/video-interfaces.yaml#
>>>>> +
>>>>> +          properties:
>>>>> +            link-frequencies: true
>>>>> +            data-lanes: true
>>>>
>>>> No. Use $ref for both of them.
>>>>
>>>>> +
>>>>> +          required:
>>>>> +            - link-frequencies
>>>>> +            - data-lanes
>>>>
>>>> No, they are not required.
>>>>
>>>>> +
>>>>> +          additionalProperties: false
>>>>> +
>>>>
>>>> deprecation of old data-lanes property?
>>>>
>>>>>   required:
>>>>>     - compatible
>>>>>     - reg
>>>>> @@ -158,6 +172,9 @@ examples:
>>>>>                   reg = <1>;
>>>>>                   endpoint {
>>>>>                       remote-endpoint = <&typec>;
>>>>> +                    data-lanes = <1 2>;
>>>>> +                    link-frequencies = /bits/ 64 <160000000 
>>>>> 270000000
>>>
>>> s/1600/1620
>>>
>>>>> + 540000000 810000000>;
>>>>
>>>> I guess the number of zeroes is wrong here. This is 160 MHz ... 810 
>>>> Mhz, rather than 1.6 GHz ... 8.1 GHz
>>>
>>> Ok, I was wrong here. The old code definitely defaults to 570 
>>> mega-something. Now I'd really like to read your description for the 
>>> link-frequencies property, because the 
>>> phy_configure_opts_dp::link_rate is clearly specified in Mb/s and it 
>>> takes a fixed set of values from 1.62 Gb/s up to 8.1 Gb/s.
>>>
>>> I think the drm_dp_bw_code_to_link_rate() function is incorrect by 
>>> itself, as it multiplies with 27000 (27 Mbps) rather than 270000 
>>> (0.27 Gbps) as required by the standard. So first, we should fix the 
>>> function, then all the rates would become logical.
>>
>> no, drm_dp_bw_code_to_link_rate() is correct and should not be 
>> changes since it impact to other dp drivers too.
>>
>> 0.27Gbps/lane is specified at DP spec.
>>
>> DP use 8b/10b coding rule (10 bits symbol contains 8 bits data).
>
> At least it should get documentation that it returns Kylo-bytes per 
> second.
>
> But, getting back to link-frequencies. The documentation clearly says 
> that it should be allowed data bus _frequencies_. And frequencies are 
> measured in Hz, not in bits/sec or bytes/sec.

ok, in the case, we can specify link frequency (symbol rate), such as 
81000000000 (8.1G hz), at dtsi to match link-frequencies cocumentation.

then at parser, we have to divided by 10 to convert back to link rate 
and then divided by 1000 to  convert to kb.

is this work for you?

>
>
>>>>>                   };
>>>>>               };
>>>>>           };
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>>>>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>>> index 754d2d6..39f0844 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>>> @@ -812,7 +812,11 @@ hp_i2c: &i2c9 {
>>>>>       status = "okay";
>>>>>       pinctrl-names = "default";
>>>>>       pinctrl-0 = <&dp_hot_plug_det>;
>>>>> -    data-lanes = <0 1>;
>>>>> +};
>>>>> +
>>>>> +&dp_out {
>>>>> +    data-lanes = <0  1>;
>>>>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000>;
>>>>
>>>> Same comment here.
>>>>
>>>>>   };
>>>>>   &pm6150_adc {
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
>>>>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> index 93e39fc..b7c343d 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> @@ -440,7 +440,11 @@ ap_i2c_tpm: &i2c14 {
>>>>>       status = "okay";
>>>>>       pinctrl-names = "default";
>>>>>       pinctrl-0 = <&dp_hot_plug_det>;
>>>>> -    data-lanes = <0 1>;
>>>>> +};
>>>>> +
>>>>> +&dp_out {
>>>>> +    data-lanes = <0  1>;
>>>>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000 
>>>>> 810000000>;
>>>>
>>>> And here.
>>>>
>>>>>   };
>>>>>   &mdss_mdp {
>>>>
>>>
>