[PATCH 0/4] Add dt support for exynos hdmiphy settings

[PATCH 0/4] Add dt support for exynos hdmiphy settings

[Shirish S]

For various revisions of a chipset if the signal pattern is changed for every
revision, then the phy setting need to be updated correspondingly by measuring
the signal.
For getting correct signals the clock level and data de-emphasis 
levels needs to be adjusted.
Since only these 2 values matter,we can move the same to dt, 
wherein we can have different dt files for every revision. 

This is an initial patchset towards achieving the same 
for exynos 5250 and can be later extended to future chipsets.

V2: replaced moving of entire phy config structure with only
required and justifiable conf registers.

V3: Incorporated Mark Rutland's comments.

V4: Rebased and included cros5250-common.dtsi.

Shirish S (4):
  ARM: dts: smdk5250: Add hdmi phy settings
  ARM: dts: arndale: Add hdmi phy settings
  ARM: exynos: dts: cros5250: Add hdmi phy settings
  drm: exynos: hdmi: Add dt support for hdmiphy settings

 .../devicetree/bindings/video/exynos_hdmi.txt      |   34 +++++++++
 arch/arm/boot/dts/cros5250-common.dtsi             |   75 +++++++++++++++++++
 arch/arm/boot/dts/exynos5250-arndale.dts           |   75 +++++++++++++++++++
 arch/arm/boot/dts/exynos5250-smdk5250.dts          |   75 +++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   79 +++++++++++++++++++-
 5 files changed, 334 insertions(+), 4 deletions(-)

-- 
1.7.9.5

--
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 1/4] ARM: dts: smdk5250: Add hdmi phy settings

[Shirish S]

This patch moves the hdmi phy setting to smdk5250
dts,as its more of a per board configuration and
also shall be easier for supporting future chipsets.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos5250-smdk5250.dts |   75 +++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 2538b32..e1f4e08 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -220,6 +220,81 @@
 
 	hdmi {
 		hpd-gpio = <&gpx3 7 0>;
+		hdmiphy-configs {
+			/*
+			* Eye diagram test passed for:
+			* Data de-emphasis: -0.7dB & Data Level: 880mV
+			* i.e., 0010 0110 = 0x26
+			* and Clock level of 515mV and diff 1030mV
+			* i.e., 0x66
+			*/
+			nr-configs = <13>;
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config1: config1 {
+				pixel-clock = <27000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config2: config2 {
+				pixel-clock = <27027000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config3: config3 {
+				pixel-clock = <36000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config4: config4 {
+				pixel-clock = <40000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config5: config5 {
+				pixel-clock = <65000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config6: config6 {
+				pixel-clock = <74176000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config7: config7 {
+				pixel-clock = <74250000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config8: config8 {
+				pixel-clock = <83500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config9: config9 {
+				pixel-clock = <106500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config10: config10 {
+				pixel-clock = <108000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config11: config11 {
+				pixel-clock = <146250000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config12: config12 {
+				pixel-clock = <148500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		};
 	};
 
 	codec@11000000 {
-- 
1.7.9.5

--
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 2/4] ARM: dts: arndale: Add hdmi phy settings

[Shirish S]

This patch moves the hdmi phy setting to arndale dts,
as its more of a per board configuration and also
shall be easier for supporting future chipsets.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos5250-arndale.dts |   75 ++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index cee55fa..c771ba3 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -475,6 +475,81 @@
 		vdd_osc-supply = <&ldo10_reg>;
 		vdd_pll-supply = <&ldo8_reg>;
 		vdd-supply = <&ldo8_reg>;
+		hdmiphy-configs {
+			/*
+			* Eye diagram test passed for:
+			* Data de-emphasis: -0.7dB & Data Level: 880mV
+			* i.e., 0010 0110 = 0x26
+			* and Clock level of 515mV and diff 1030mV
+			* i.e., 0x66
+			*/
+			nr-configs = <13>;
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config1: config1 {
+				pixel-clock = <27000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config2: config2 {
+				pixel-clock = <27027000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config3: config3 {
+				pixel-clock = <36000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config4: config4 {
+				pixel-clock = <40000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config5: config5 {
+				pixel-clock = <65000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config6: config6 {
+				pixel-clock = <74176000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config7: config7 {
+				pixel-clock = <74250000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config8: config8 {
+				pixel-clock = <83500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config9: config9 {
+				pixel-clock = <106500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config10: config10 {
+				pixel-clock = <108000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config11: config11 {
+				pixel-clock = <146250000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config12: config12 {
+				pixel-clock = <148500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		};
 	};
 
 	regulators {
-- 
1.7.9.5

--
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 3/4] ARM: exynos: dts: cros5250: Add hdmi phy settings

[Shirish S]

This patch moves the hdmi phy setting to arndale dts,
as its more of a per board configuration and also
shall be easier for supporting future chipsets.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/cros5250-common.dtsi |   75 ++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/arch/arm/boot/dts/cros5250-common.dtsi b/arch/arm/boot/dts/cros5250-common.dtsi
index dc259e8b..3cd1779 100644
--- a/arch/arm/boot/dts/cros5250-common.dtsi
+++ b/arch/arm/boot/dts/cros5250-common.dtsi
@@ -301,6 +301,81 @@
 
 	hdmi {
 		hpd-gpio = <&gpx3 7 0>;
+		hdmiphy-configs {
+			/*
+			* Eye diagram test passed for:
+			* Data de-emphasis: -0.7dB & Data Level: 880mV
+			* i.e., 0010 0110 = 0x26
+			* and Clock level of 515mV and diff 1030mV
+			* i.e., 0x66
+			*/
+			nr-configs = <13>;
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config1: config1 {
+				pixel-clock = <27000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config2: config2 {
+				pixel-clock = <27027000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config3: config3 {
+				pixel-clock = <36000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config4: config4 {
+				pixel-clock = <40000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config5: config5 {
+				pixel-clock = <65000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config6: config6 {
+				pixel-clock = <74176000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config7: config7 {
+				pixel-clock = <74250000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config8: config8 {
+				pixel-clock = <83500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config9: config9 {
+				pixel-clock = <106500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config10: config10 {
+				pixel-clock = <108000000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config11: config11 {
+				pixel-clock = <146250000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+			config12: config12 {
+				pixel-clock = <148500000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		};
 	};
 
 	gpio-keys {
-- 
1.7.9.5

--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

This patch adds dt support to hdmiphy config settings
as it is board specific and depends on the signal pattern
of board.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   34 +++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   79 +++++++++++++++++++-
 2 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
index 323983b..c685c90 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -13,6 +13,32 @@ Required properties:
 	b) pin number within the gpio controller.
 	c) optional flags and pull up/down.
 
+- hdmiphy-configs: following information about the hdmiphy config settings.
+        a) "nr-configs" specifies the number of pixel clocks supported.
+	b) "config<N>: config<N>" specifies the phy configuration settings,
+		wher 'N' denotes the number of iteration.
+		"pixel-clock" specifies the pixel clock
+		"conifig-de-emphasis-level" specifies the 8 bit configuration
+		of Data De-emphasis levels,below shown is example for
+		data de-emphasis register at address 0x145D0040.
+			hdmiphy@38[16] for bits[3:0] permitted values:
+				0000 means 760 mVdiff && 1111 means 1400 mVdiff
+				1LSB corresponds to 20mVdiff
+			hdmiphy@38[16] for bits[7:4] permitted values:
+				0000 	0dB
+				0001	-0.25dB
+				0010	-0.7dB
+				0011	-1.15dB
+				1111	-7.45dB
+		"config-clock-level" specifies the 8 bit configuration for
+		the corresponding clock level, for example if 0x145D005C
+		is the address of clock level register.
+			hdmiphy@38[23] for bits [1:0] permitted values:
+				00 means 0 mVdiff && 11 means 60 mVdiff
+			hdmiphy@38[23] for bits [7:3] permitted values:
+				00000 is 790 mVdiff
+				11111 is 1430 mVdiff
+				1LSB corresponds to 20mVdiff
 Example:
 
 	hdmi {
@@ -20,4 +46,12 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-configs {
+			nr-configs = <1>;
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		}
 	};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 32ce9a6..065ac1f 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -197,6 +197,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -256,7 +259,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -785,8 +788,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 		confs = hdmiphy_v13_configs;
 		count = ARRAY_SIZE(hdmiphy_v13_configs);
 	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
+		confs = hdata->confs;
+		count = hdata->nr_confs;
 	} else
 		return -EINVAL;
 
@@ -1415,7 +1418,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 	if (hdata->type == HDMI_TYPE13)
 		hdmiphy_data = hdmiphy_v13_configs[i].conf;
 	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+		hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1894,6 +1897,65 @@ fail:
 	return -ENODEV;
 }
 
+static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
+						struct hdmi_context *hdata)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dev_np = dev->of_node;
+	struct device_node *phy_conf, *cfg_np;
+	int i = 0;
+
+	/* Initialize with default config */
+	hdata->confs = hdmiphy_v14_configs;
+
+	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
+	if (phy_conf == NULL) {
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		DRM_ERROR("Did not find hdmiphy-configs node\n");
+		return -ENODEV;
+	}
+
+	if (of_property_read_u32(phy_conf, "nr-configs", &hdata->nr_confs)) {
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		DRM_ERROR("Failed to get the number of configurations");
+		return -EINVAL;
+	}
+
+	if (hdata->nr_confs != ARRAY_SIZE(hdmiphy_v14_configs)) {
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		DRM_ERROR("mismatch in the user specified number of configs\n");
+		return -EINVAL;
+	}
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (!of_find_property(cfg_np, "pixel-clock", NULL))
+			continue;
+
+		if (of_property_read_u32_array(cfg_np, "pixel-clock",
+					&hdata->confs[i].pixel_clock, 1)) {
+				DRM_ERROR("Failed to get pixel clock\n");
+				return -EINVAL;
+		}
+
+		/* Overwrite the data de-emphasis and data level */
+		if (of_property_read_u8_array(cfg_np,
+			 "config-de-emphasis-level",
+					 &hdata->confs[i].conf[16], 1)) {
+				DRM_ERROR("Failed to get conf\n");
+				return -EINVAL;
+		}
+		/* Overwrite the clock level diff */
+		if (of_property_read_u8_array(cfg_np, "config-clock-level",
+					&hdata->confs[i].conf[23], 1)) {
+				DRM_ERROR("Failed to get conf\n");
+				return -EINVAL;
+		}
+		i++;
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -2024,6 +2086,15 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* get hdmiphy confs */
+	if (hdata->type == HDMI_TYPE14) {
+		ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+		if (ret) {
+			DRM_ERROR("failed to get user defined config,will use
+			default configs, eye diagram tests may fail\n");
+		}
+	}
+
 	hdmi_display.dev = dev;
 	exynos_drm_display_register(&hdmi_display);
 
-- 
1.7.9.5

--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Mark Rutland]

On Tue, Oct 29, 2013 at 08:12:32AM +0000, Shirish S wrote:
> This patch adds dt support to hdmiphy config settings
> as it is board specific and depends on the signal pattern
> of board.
> 
> Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../devicetree/bindings/video/exynos_hdmi.txt      |   34 +++++++++
>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   79 +++++++++++++++++++-
>  2 files changed, 109 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> index 323983b..c685c90 100644
> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> @@ -13,6 +13,32 @@ Required properties:
>  	b) pin number within the gpio controller.
>  	c) optional flags and pull up/down.
>  
> +- hdmiphy-configs: following information about the hdmiphy config settings.
> +        a) "nr-configs" specifies the number of pixel clocks supported.

I really don't see why this is necessary. It's redundant, and it's easy
for the driver to derive this from the number of config<N> nodes, which
it can count.

> +	b) "config<N>: config<N>" specifies the phy configuration settings,
> +		wher 'N' denotes the number of iteration.

The number of iteration?

> +		"pixel-clock" specifies the pixel clock
> +		"conifig-de-emphasis-level" specifies the 8 bit configuration
> +		of Data De-emphasis levels,below shown is example for
> +		data de-emphasis register at address 0x145D0040.
> +			hdmiphy@38[16] for bits[3:0] permitted values:
> +				0000 means 760 mVdiff && 1111 means 1400 mVdiff
> +				1LSB corresponds to 20mVdiff
> +			hdmiphy@38[16] for bits[7:4] permitted values:
> +				0000 	0dB
> +				0001	-0.25dB
> +				0010	-0.7dB
> +				0011	-1.15dB
> +				1111	-7.45dB
> +		"config-clock-level" specifies the 8 bit configuration for
> +		the corresponding clock level, for example if 0x145D005C
> +		is the address of clock level register.

I don't understand what this intended to mean.

> +			hdmiphy@38[23] for bits [1:0] permitted values:
> +				00 means 0 mVdiff && 11 means 60 mVdiff
> +			hdmiphy@38[23] for bits [7:3] permitted values:
> +				00000 is 790 mVdiff
> +				11111 is 1430 mVdiff
> +				1LSB corresponds to 20mVdiff

That last line was confusing. Why not state that this is a value between
790 and 1430 mV in 20mV increments?

Thanks,
Mark.
--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

Hi,

On Fri, Nov 15, 2013 at 9:47 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, Oct 29, 2013 at 08:12:32AM +0000, Shirish S wrote:
>> This patch adds dt support to hdmiphy config settings
>> as it is board specific and depends on the signal pattern
>> of board.
>>
>> Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  .../devicetree/bindings/video/exynos_hdmi.txt      |   34 +++++++++
>>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   79 +++++++++++++++++++-
>>  2 files changed, 109 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> index 323983b..c685c90 100644
>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> @@ -13,6 +13,32 @@ Required properties:
>>       b) pin number within the gpio controller.
>>       c) optional flags and pull up/down.
>>
>> +- hdmiphy-configs: following information about the hdmiphy config settings.
>> +        a) "nr-configs" specifies the number of pixel clocks supported.
>
> I really don't see why this is necessary. It's redundant, and it's easy
> for the driver to derive this from the number of config<N> nodes, which
> it can count.
>
Agreed, i have removed this field and now use the pixel clock to update the
required values.
>> +     b) "config<N>: config<N>" specifies the phy configuration settings,
>> +             wher 'N' denotes the number of iteration.
>
> The number of iteration?
Corrected in next patch set.
>
>> +             "pixel-clock" specifies the pixel clock
>> +             "conifig-de-emphasis-level" specifies the 8 bit configuration
>> +             of Data De-emphasis levels,below shown is example for
>> +             data de-emphasis register at address 0x145D0040.
>> +                     hdmiphy@38[16] for bits[3:0] permitted values:
>> +                             0000 means 760 mVdiff && 1111 means 1400 mVdiff
>> +                             1LSB corresponds to 20mVdiff
>> +                     hdmiphy@38[16] for bits[7:4] permitted values:
>> +                             0000    0dB
>> +                             0001    -0.25dB
>> +                             0010    -0.7dB
>> +                             0011    -1.15dB
>> +                             1111    -7.45dB
>> +             "config-clock-level" specifies the 8 bit configuration for
>> +             the corresponding clock level, for example if 0x145D005C
>> +             is the address of clock level register.
>
> I don't understand what this intended to mean.
Have updated in next patch set, hope its understandable.
>
>> +                     hdmiphy@38[23] for bits [1:0] permitted values:
>> +                             00 means 0 mVdiff && 11 means 60 mVdiff
>> +                     hdmiphy@38[23] for bits [7:3] permitted values:
>> +                             00000 is 790 mVdiff
>> +                             11111 is 1430 mVdiff
>> +                             1LSB corresponds to 20mVdiff
>
> That last line was confusing. Why not state that this is a value between
> 790 and 1430 mV in 20mV increments?
>
Agreed, have made the change in next patch set.
> Thanks,
> Mark.
Thanks,
Shirish S
--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

This patch adds dt support to hdmiphy config settings
as it is board specific and depends on the signal pattern
of board.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   33 +++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
 2 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
index 323983b..1021c74 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -13,6 +13,31 @@ Required properties:
 	b) pin number within the gpio controller.
 	c) optional flags and pull up/down.
 
+- hdmiphy-configs: following information about the hdmiphy config settings.
+        a) "nr-configs" specifies the number of pixel clocks supported.
+	b) "config<N>: config<N>" specifies the phy configuration settings,
+		where 'N' denotes the number of configuration, since every
+		pixel clock can have its unique configuration.
+		"pixel-clock" specifies the pixel clock
+		"conifig-de-emphasis-level" provides fine control of TMDS data
+		 pre emphasis, below shown is example for
+		data de-emphasis register at address 0x145D0040.
+			hdmiphy@38[16] for bits[3:0] permitted values are in
+			the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
+			increments for every LSB
+			hdmiphy@38[16] for bits[7:4] permitted values are in
+			the range of 0dB to -7.45dB at increments of -0.45dB
+			for every LSB.
+		"config-clock-level" provides fine control of TMDS data
+		amplitude for each channel,
+		for example if 0x145D005C is the address of clock level
+		register then,
+			hdmiphy@38[23] for bits [1:0] permitted values are in
+			the range of 0 mVdiff & 60 mVdiff for each channel at
+			increments 20 mVdiff of amplitude levels for every LSB,
+			hdmiphy@38[23] for bits [7:3] permitted values are in
+			the range of 790 and 1430 mV at 20mV increments for
+			every LSB.
 Example:
 
 	hdmi {
@@ -20,4 +45,12 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-configs {
+			nr-configs = <1>;
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		}
 	};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 32ce9a6..5f599e3 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -197,6 +197,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -256,7 +259,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -785,8 +788,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 		confs = hdmiphy_v13_configs;
 		count = ARRAY_SIZE(hdmiphy_v13_configs);
 	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
+		confs = hdata->confs;
+		count = hdata->nr_confs;
 	} else
 		return -EINVAL;
 
@@ -1415,7 +1418,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 	if (hdata->type == HDMI_TYPE13)
 		hdmiphy_data = hdmiphy_v13_configs[i].conf;
 	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+		hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1894,6 +1897,63 @@ fail:
 	return -ENODEV;
 }
 
+static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
+						struct hdmi_context *hdata)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dev_np = dev->of_node;
+	struct device_node *phy_conf, *cfg_np;
+	int i, pixel_clock = 0;
+
+	/* Initialize with default config */
+	hdata->confs = hdmiphy_v14_configs;
+	hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+
+	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
+	if (phy_conf == NULL) {
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		DRM_ERROR("Did not find hdmiphy-configs node\n");
+		return -ENODEV;
+	}
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (!of_find_property(cfg_np, "pixel-clock", NULL))
+			continue;
+
+		if (of_property_read_u32(cfg_np, "pixel-clock",
+							&pixel_clock, 1)) {
+				DRM_ERROR("Failed to get pixel clock\n");
+				return -EINVAL;
+		}
+
+		for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
+			if (hdata->confs[i].pixel_clock == pixel_clock)
+				/* Update the data de-emphasis and data level */
+				if (of_property_read_u8_array(cfg_np,
+					 "config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				if (of_property_read_u8_array(cfg_np,
+					 "config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				/* Update the clock level diff */
+				if (of_property_read_u8_array(cfg_np,
+					 "config-clock-level",
+					&hdata->confs[i].conf[23], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+		}
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* get hdmiphy confs */
+	if (hdata->type == HDMI_TYPE14) {
+		ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+		if (ret) {
+			DRM_ERROR("failed to get user defined config,will use
+			default configs, eye diagram tests may fail\n");
+		}
+	}
+
 	hdmi_display.dev = dev;
 	exynos_drm_display_register(&hdmi_display);
 
-- 
1.7.9.5

--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

This patch adds dt support to hdmiphy config settings
as it is board specific and depends on the signal pattern
of board.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   33 +++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
 2 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
index 323983b..1021c74 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -13,6 +13,31 @@ Required properties:
 	b) pin number within the gpio controller.
 	c) optional flags and pull up/down.
 
+- hdmiphy-configs: following information about the hdmiphy config settings.
+        a) "nr-configs" specifies the number of pixel clocks supported.
+	b) "config<N>: config<N>" specifies the phy configuration settings,
+		where 'N' denotes the number of configuration, since every
+		pixel clock can have its unique configuration.
+		"pixel-clock" specifies the pixel clock
+		"conifig-de-emphasis-level" provides fine control of TMDS data
+		 pre emphasis, below shown is example for
+		data de-emphasis register at address 0x145D0040.
+			hdmiphy@38[16] for bits[3:0] permitted values are in
+			the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
+			increments for every LSB
+			hdmiphy@38[16] for bits[7:4] permitted values are in
+			the range of 0dB to -7.45dB at increments of -0.45dB
+			for every LSB.
+		"config-clock-level" provides fine control of TMDS data
+		amplitude for each channel,
+		for example if 0x145D005C is the address of clock level
+		register then,
+			hdmiphy@38[23] for bits [1:0] permitted values are in
+			the range of 0 mVdiff & 60 mVdiff for each channel at
+			increments 20 mVdiff of amplitude levels for every LSB,
+			hdmiphy@38[23] for bits [7:3] permitted values are in
+			the range of 790 and 1430 mV at 20mV increments for
+			every LSB.
 Example:
 
 	hdmi {
@@ -20,4 +45,12 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-configs {
+			nr-configs = <1>;
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		}
 	};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 32ce9a6..5f599e3 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -197,6 +197,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -256,7 +259,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -785,8 +788,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 		confs = hdmiphy_v13_configs;
 		count = ARRAY_SIZE(hdmiphy_v13_configs);
 	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
+		confs = hdata->confs;
+		count = hdata->nr_confs;
 	} else
 		return -EINVAL;
 
@@ -1415,7 +1418,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 	if (hdata->type == HDMI_TYPE13)
 		hdmiphy_data = hdmiphy_v13_configs[i].conf;
 	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+		hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1894,6 +1897,63 @@ fail:
 	return -ENODEV;
 }
 
+static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
+						struct hdmi_context *hdata)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dev_np = dev->of_node;
+	struct device_node *phy_conf, *cfg_np;
+	int i, pixel_clock = 0;
+
+	/* Initialize with default config */
+	hdata->confs = hdmiphy_v14_configs;
+	hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+
+	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
+	if (phy_conf == NULL) {
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		DRM_ERROR("Did not find hdmiphy-configs node\n");
+		return -ENODEV;
+	}
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (!of_find_property(cfg_np, "pixel-clock", NULL))
+			continue;
+
+		if (of_property_read_u32(cfg_np, "pixel-clock",
+							&pixel_clock, 1)) {
+				DRM_ERROR("Failed to get pixel clock\n");
+				return -EINVAL;
+		}
+
+		for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
+			if (hdata->confs[i].pixel_clock == pixel_clock)
+				/* Update the data de-emphasis and data level */
+				if (of_property_read_u8_array(cfg_np,
+					 "config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				if (of_property_read_u8_array(cfg_np,
+					 "config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				/* Update the clock level diff */
+				if (of_property_read_u8_array(cfg_np,
+					 "config-clock-level",
+					&hdata->confs[i].conf[23], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+		}
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* get hdmiphy confs */
+	if (hdata->type == HDMI_TYPE14) {
+		ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+		if (ret) {
+			DRM_ERROR("failed to get user defined config,will use
+			default configs, eye diagram tests may fail\n");
+		}
+	}
+
 	hdmi_display.dev = dev;
 	exynos_drm_display_register(&hdmi_display);
 
-- 
1.7.9.5

--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

Hi Mark,
I have uploaded patch set 5 and 6 back to back, but unfortunately
still nr-configs is stuck in the example explanation in the
exynos_hdmi.txt, just to inform you that am waiting for your review
comments so that i can rectify it along with them in the next patch
set.
Regards,
Shirish S

On Mon, Nov 18, 2013 at 11:37 AM, Shirish S <shirish-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Hi,
>
> On Fri, Nov 15, 2013 at 9:47 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> On Tue, Oct 29, 2013 at 08:12:32AM +0000, Shirish S wrote:
>>> This patch adds dt support to hdmiphy config settings
>>> as it is board specific and depends on the signal pattern
>>> of board.
>>>
>>> Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>> ---
>>>  .../devicetree/bindings/video/exynos_hdmi.txt      |   34 +++++++++
>>>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   79 +++++++++++++++++++-
>>>  2 files changed, 109 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>> index 323983b..c685c90 100644
>>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>> @@ -13,6 +13,32 @@ Required properties:
>>>       b) pin number within the gpio controller.
>>>       c) optional flags and pull up/down.
>>>
>>> +- hdmiphy-configs: following information about the hdmiphy config settings.
>>> +        a) "nr-configs" specifies the number of pixel clocks supported.
>>
>> I really don't see why this is necessary. It's redundant, and it's easy
>> for the driver to derive this from the number of config<N> nodes, which
>> it can count.
>>
> Agreed, i have removed this field and now use the pixel clock to update the
> required values.
>>> +     b) "config<N>: config<N>" specifies the phy configuration settings,
>>> +             wher 'N' denotes the number of iteration.
>>
>> The number of iteration?
> Corrected in next patch set.
>>
>>> +             "pixel-clock" specifies the pixel clock
>>> +             "conifig-de-emphasis-level" specifies the 8 bit configuration
>>> +             of Data De-emphasis levels,below shown is example for
>>> +             data de-emphasis register at address 0x145D0040.
>>> +                     hdmiphy@38[16] for bits[3:0] permitted values:
>>> +                             0000 means 760 mVdiff && 1111 means 1400 mVdiff
>>> +                             1LSB corresponds to 20mVdiff
>>> +                     hdmiphy@38[16] for bits[7:4] permitted values:
>>> +                             0000    0dB
>>> +                             0001    -0.25dB
>>> +                             0010    -0.7dB
>>> +                             0011    -1.15dB
>>> +                             1111    -7.45dB
>>> +             "config-clock-level" specifies the 8 bit configuration for
>>> +             the corresponding clock level, for example if 0x145D005C
>>> +             is the address of clock level register.
>>
>> I don't understand what this intended to mean.
> Have updated in next patch set, hope its understandable.
>>
>>> +                     hdmiphy@38[23] for bits [1:0] permitted values:
>>> +                             00 means 0 mVdiff && 11 means 60 mVdiff
>>> +                     hdmiphy@38[23] for bits [7:3] permitted values:
>>> +                             00000 is 790 mVdiff
>>> +                             11111 is 1430 mVdiff
>>> +                             1LSB corresponds to 20mVdiff
>>
>> That last line was confusing. Why not state that this is a value between
>> 790 and 1430 mV in 20mV increments?
>>
> Agreed, have made the change in next patch set.
>> Thanks,
>> Mark.
> Thanks,
> Shirish S
--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

This patch adds dt support to hdmiphy config settings
as it is board specific and depends on the signal pattern
of board.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   31 ++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
 2 files changed, 104 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
index 323983b..6eeb333 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -13,6 +13,30 @@ Required properties:
 	b) pin number within the gpio controller.
 	c) optional flags and pull up/down.
 
+- hdmiphy-configs: following information about the hdmiphy config settings.
+	a) "config<N>: config<N>" specifies the phy configuration settings,
+		where 'N' denotes the number of configuration, since every
+		pixel clock can have its unique configuration.
+		"pixel-clock" specifies the pixel clock
+		"conifig-de-emphasis-level" provides fine control of TMDS data
+		 pre emphasis, below shown is example for
+		data de-emphasis register at address 0x145D0040.
+			hdmiphy@38[16] for bits[3:0] permitted values are in
+			the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
+			increments for every LSB
+			hdmiphy@38[16] for bits[7:4] permitted values are in
+			the range of 0dB to -7.45dB at increments of -0.45dB
+			for every LSB.
+		"config-clock-level" provides fine control of TMDS data
+		amplitude for each channel,
+		for example if 0x145D005C is the address of clock level
+		register then,
+			hdmiphy@38[23] for bits [1:0] permitted values are in
+			the range of 0 mVdiff & 60 mVdiff for each channel at
+			increments 20 mVdiff of amplitude levels for every LSB,
+			hdmiphy@38[23] for bits [7:3] permitted values are in
+			the range of 790 and 1430 mV at 20mV increments for
+			every LSB.
 Example:
 
 	hdmi {
@@ -20,4 +44,11 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-configs {
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		}
 	};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 32ce9a6..5f599e3 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -197,6 +197,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -256,7 +259,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -785,8 +788,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 		confs = hdmiphy_v13_configs;
 		count = ARRAY_SIZE(hdmiphy_v13_configs);
 	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
+		confs = hdata->confs;
+		count = hdata->nr_confs;
 	} else
 		return -EINVAL;
 
@@ -1415,7 +1418,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 	if (hdata->type == HDMI_TYPE13)
 		hdmiphy_data = hdmiphy_v13_configs[i].conf;
 	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+		hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1894,6 +1897,63 @@ fail:
 	return -ENODEV;
 }
 
+static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
+						struct hdmi_context *hdata)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dev_np = dev->of_node;
+	struct device_node *phy_conf, *cfg_np;
+	int i, pixel_clock = 0;
+
+	/* Initialize with default config */
+	hdata->confs = hdmiphy_v14_configs;
+	hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+
+	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
+	if (phy_conf == NULL) {
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		DRM_ERROR("Did not find hdmiphy-configs node\n");
+		return -ENODEV;
+	}
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (!of_find_property(cfg_np, "pixel-clock", NULL))
+			continue;
+
+		if (of_property_read_u32(cfg_np, "pixel-clock",
+							&pixel_clock, 1)) {
+				DRM_ERROR("Failed to get pixel clock\n");
+				return -EINVAL;
+		}
+
+		for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
+			if (hdata->confs[i].pixel_clock == pixel_clock)
+				/* Update the data de-emphasis and data level */
+				if (of_property_read_u8_array(cfg_np,
+					 "config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				if (of_property_read_u8_array(cfg_np,
+					 "config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				/* Update the clock level diff */
+				if (of_property_read_u8_array(cfg_np,
+					 "config-clock-level",
+					&hdata->confs[i].conf[23], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+		}
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* get hdmiphy confs */
+	if (hdata->type == HDMI_TYPE14) {
+		ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+		if (ret) {
+			DRM_ERROR("failed to get user defined config,will use
+			default configs, eye diagram tests may fail\n");
+		}
+	}
+
 	hdmi_display.dev = dev;
 	exynos_drm_display_register(&hdmi_display);
 
-- 
1.7.9.5

--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Inki Dae]

Hi Shirish,

2013/11/25 Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>:
> This patch adds dt support to hdmiphy config settings
> as it is board specific and depends on the signal pattern
> of board.
>
> Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../devicetree/bindings/video/exynos_hdmi.txt      |   31 ++++++++
>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
>  2 files changed, 104 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> index 323983b..6eeb333 100644
> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> @@ -13,6 +13,30 @@ Required properties:
>         b) pin number within the gpio controller.
>         c) optional flags and pull up/down.
>
> +- hdmiphy-configs: following information about the hdmiphy config settings.
> +       a) "config<N>: config<N>" specifies the phy configuration settings,
> +               where 'N' denotes the number of configuration, since every
> +               pixel clock can have its unique configuration.
> +               "pixel-clock" specifies the pixel clock
> +               "conifig-de-emphasis-level" provides fine control of TMDS data
> +                pre emphasis, below shown is example for
> +               data de-emphasis register at address 0x145D0040.
> +                       hdmiphy@38[16] for bits[3:0] permitted values are in
> +                       the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
> +                       increments for every LSB
> +                       hdmiphy@38[16] for bits[7:4] permitted values are in
> +                       the range of 0dB to -7.45dB at increments of -0.45dB
> +                       for every LSB.
> +               "config-clock-level" provides fine control of TMDS data
> +               amplitude for each channel,
> +               for example if 0x145D005C is the address of clock level
> +               register then,
> +                       hdmiphy@38[23] for bits [1:0] permitted values are in
> +                       the range of 0 mVdiff & 60 mVdiff for each channel at
> +                       increments 20 mVdiff of amplitude levels for every LSB,
> +                       hdmiphy@38[23] for bits [7:3] permitted values are in
> +                       the range of 790 and 1430 mV at 20mV increments for
> +                       every LSB.
>  Example:
>
>         hdmi {
> @@ -20,4 +44,11 @@ Example:
>                 reg = <0x14530000 0x100000>;
>                 interrupts = <0 95 0>;
>                 hpd-gpio = <&gpx3 7 1>;
> +               hdmiphy-configs {
> +                       config0: config0 {
> +                               pixel-clock = <25200000>;
> +                               config-de-emphasis-level =  /bits/ 8 <0x26>;
> +                               config-clock-level =  /bits/ 8 < 0x66>;
> +                       };
> +               }
>         };
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 32ce9a6..5f599e3 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -197,6 +197,9 @@ struct hdmi_context {
>
>         struct hdmi_resources           res;
>
> +       struct hdmiphy_config           *confs;
> +       int                             nr_confs;
> +
>         int                             hpd_gpio;
>
>         enum hdmi_type                  type;
> @@ -256,7 +259,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
>         },
>  };
>
> -static const struct hdmiphy_config hdmiphy_v14_configs[] = {
> +static struct hdmiphy_config hdmiphy_v14_configs[] = {
>         {
>                 .pixel_clock = 25200000,
>                 .conf = {
> @@ -785,8 +788,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
>                 confs = hdmiphy_v13_configs;
>                 count = ARRAY_SIZE(hdmiphy_v13_configs);
>         } else if (hdata->type == HDMI_TYPE14) {
> -               confs = hdmiphy_v14_configs;
> -               count = ARRAY_SIZE(hdmiphy_v14_configs);
> +               confs = hdata->confs;
> +               count = hdata->nr_confs;
>         } else
>                 return -EINVAL;
>
> @@ -1415,7 +1418,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
>         if (hdata->type == HDMI_TYPE13)
>                 hdmiphy_data = hdmiphy_v13_configs[i].conf;
>         else
> -               hdmiphy_data = hdmiphy_v14_configs[i].conf;
> +               hdmiphy_data = hdata->confs[i].conf;
>
>         memcpy(buffer, hdmiphy_data, 32);
>         ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
> @@ -1894,6 +1897,63 @@ fail:
>         return -ENODEV;
>  }
>
> +static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
> +                                               struct hdmi_context *hdata)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *dev_np = dev->of_node;
> +       struct device_node *phy_conf, *cfg_np;
> +       int i, pixel_clock = 0;
> +
> +       /* Initialize with default config */
> +       hdata->confs = hdmiphy_v14_configs;
> +       hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
> +
> +       phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
> +       if (phy_conf == NULL) {
> +               hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
> +               DRM_ERROR("Did not find hdmiphy-configs node\n");
> +               return -ENODEV;
> +       }
> +
> +       for_each_child_of_node(phy_conf, cfg_np) {
> +               if (!of_find_property(cfg_np, "pixel-clock", NULL))
> +                       continue;
> +
> +               if (of_property_read_u32(cfg_np, "pixel-clock",
> +                                                       &pixel_clock, 1)) {

Have you ever built? see the below declaration,

static inline int of_property_read_u32(const struct device_node *np,
                                                      const char *propname,
                                                      u32 *out_value);

> +                               DRM_ERROR("Failed to get pixel clock\n");
> +                               return -EINVAL;
> +               }
> +
> +               for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
> +                       if (hdata->confs[i].pixel_clock == pixel_clock)
> +                               /* Update the data de-emphasis and data level */
> +                               if (of_property_read_u8_array(cfg_np,
> +                                        "config-de-emphasis-level",
> +                                       &hdata->confs[i].conf[16], 1)) {
> +                                       DRM_ERROR("Failed to get conf\n");
> +                                       return -EINVAL;
> +                               }
> +                               if (of_property_read_u8_array(cfg_np,
> +                                        "config-de-emphasis-level",
> +                                       &hdata->confs[i].conf[16], 1)) {
> +                                       DRM_ERROR("Failed to get conf\n");
> +                                       return -EINVAL;
> +                               }
> +                               /* Update the clock level diff */
> +                               if (of_property_read_u8_array(cfg_np,
> +                                        "config-clock-level",
> +                                       &hdata->confs[i].conf[23], 1)) {
> +                                       DRM_ERROR("Failed to get conf\n");
> +                                       return -EINVAL;
> +                               }
> +               }
> +       }
> +       return 0;
> +
> +}
> +
>  static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
>                                         (struct device *dev)
>  {
> @@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
>                 goto err_hdmiphy;
>         }
>
> +       /* get hdmiphy confs */
> +       if (hdata->type == HDMI_TYPE14) {
> +               ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
> +               if (ret) {
> +                       DRM_ERROR("failed to get user defined config,will use
> +                       default configs, eye diagram tests may fail\n");

build error?

> +               }
> +       }
> +
>         hdmi_display.dev = dev;
>         exynos_drm_display_register(&hdmi_display);
>
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

Hi,

On Tue, Nov 26, 2013 at 6:30 AM, Inki Dae <inki.dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi Shirish,
>
> 2013/11/25 Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>:
>> This patch adds dt support to hdmiphy config settings
>> as it is board specific and depends on the signal pattern
>> of board.
>>
>> Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  .../devicetree/bindings/video/exynos_hdmi.txt      |   31 ++++++++
>>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
>>  2 files changed, 104 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> index 323983b..6eeb333 100644
>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> @@ -13,6 +13,30 @@ Required properties:
>>         b) pin number within the gpio controller.
>>         c) optional flags and pull up/down.
>>
>> +- hdmiphy-configs: following information about the hdmiphy config settings.
>> +       a) "config<N>: config<N>" specifies the phy configuration settings,
>> +               where 'N' denotes the number of configuration, since every
>> +               pixel clock can have its unique configuration.
>> +               "pixel-clock" specifies the pixel clock
>> +               "conifig-de-emphasis-level" provides fine control of TMDS data
>> +                pre emphasis, below shown is example for
>> +               data de-emphasis register at address 0x145D0040.
>> +                       hdmiphy@38[16] for bits[3:0] permitted values are in
>> +                       the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
>> +                       increments for every LSB
>> +                       hdmiphy@38[16] for bits[7:4] permitted values are in
>> +                       the range of 0dB to -7.45dB at increments of -0.45dB
>> +                       for every LSB.
>> +               "config-clock-level" provides fine control of TMDS data
>> +               amplitude for each channel,
>> +               for example if 0x145D005C is the address of clock level
>> +               register then,
>> +                       hdmiphy@38[23] for bits [1:0] permitted values are in
>> +                       the range of 0 mVdiff & 60 mVdiff for each channel at
>> +                       increments 20 mVdiff of amplitude levels for every LSB,
>> +                       hdmiphy@38[23] for bits [7:3] permitted values are in
>> +                       the range of 790 and 1430 mV at 20mV increments for
>> +                       every LSB.
>>  Example:
>>
>>         hdmi {
>> @@ -20,4 +44,11 @@ Example:
>>                 reg = <0x14530000 0x100000>;
>>                 interrupts = <0 95 0>;
>>                 hpd-gpio = <&gpx3 7 1>;
>> +               hdmiphy-configs {
>> +                       config0: config0 {
>> +                               pixel-clock = <25200000>;
>> +                               config-de-emphasis-level =  /bits/ 8 <0x26>;
>> +                               config-clock-level =  /bits/ 8 < 0x66>;
>> +                       };
>> +               }
>>         };
>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> index 32ce9a6..5f599e3 100644
>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> @@ -197,6 +197,9 @@ struct hdmi_context {
>>
>>         struct hdmi_resources           res;
>>
>> +       struct hdmiphy_config           *confs;
>> +       int                             nr_confs;
>> +
>>         int                             hpd_gpio;
>>
>>         enum hdmi_type                  type;
>> @@ -256,7 +259,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
>>         },
>>  };
>>
>> -static const struct hdmiphy_config hdmiphy_v14_configs[] = {
>> +static struct hdmiphy_config hdmiphy_v14_configs[] = {
>>         {
>>                 .pixel_clock = 25200000,
>>                 .conf = {
>> @@ -785,8 +788,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
>>                 confs = hdmiphy_v13_configs;
>>                 count = ARRAY_SIZE(hdmiphy_v13_configs);
>>         } else if (hdata->type == HDMI_TYPE14) {
>> -               confs = hdmiphy_v14_configs;
>> -               count = ARRAY_SIZE(hdmiphy_v14_configs);
>> +               confs = hdata->confs;
>> +               count = hdata->nr_confs;
>>         } else
>>                 return -EINVAL;
>>
>> @@ -1415,7 +1418,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
>>         if (hdata->type == HDMI_TYPE13)
>>                 hdmiphy_data = hdmiphy_v13_configs[i].conf;
>>         else
>> -               hdmiphy_data = hdmiphy_v14_configs[i].conf;
>> +               hdmiphy_data = hdata->confs[i].conf;
>>
>>         memcpy(buffer, hdmiphy_data, 32);
>>         ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
>> @@ -1894,6 +1897,63 @@ fail:
>>         return -ENODEV;
>>  }
>>
>> +static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
>> +                                               struct hdmi_context *hdata)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *dev_np = dev->of_node;
>> +       struct device_node *phy_conf, *cfg_np;
>> +       int i, pixel_clock = 0;
>> +
>> +       /* Initialize with default config */
>> +       hdata->confs = hdmiphy_v14_configs;
>> +       hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
>> +
>> +       phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
>> +       if (phy_conf == NULL) {
>> +               hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
>> +               DRM_ERROR("Did not find hdmiphy-configs node\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       for_each_child_of_node(phy_conf, cfg_np) {
>> +               if (!of_find_property(cfg_np, "pixel-clock", NULL))
>> +                       continue;
>> +
>> +               if (of_property_read_u32(cfg_np, "pixel-clock",
>> +                                                       &pixel_clock, 1)) {
>
> Have you ever built? see the below declaration,
>
> static inline int of_property_read_u32(const struct device_node *np,
>                                                       const char *propname,
>                                                       u32 *out_value);
>
Yep, i missed it, will update it in the next patch set.
>> +                               DRM_ERROR("Failed to get pixel clock\n");
>> +                               return -EINVAL;
>> +               }
>> +
>> +               for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
>> +                       if (hdata->confs[i].pixel_clock == pixel_clock)
>> +                               /* Update the data de-emphasis and data level */
>> +                               if (of_property_read_u8_array(cfg_np,
>> +                                        "config-de-emphasis-level",
>> +                                       &hdata->confs[i].conf[16], 1)) {
>> +                                       DRM_ERROR("Failed to get conf\n");
>> +                                       return -EINVAL;
>> +                               }
>> +                               if (of_property_read_u8_array(cfg_np,
>> +                                        "config-de-emphasis-level",
>> +                                       &hdata->confs[i].conf[16], 1)) {
>> +                                       DRM_ERROR("Failed to get conf\n");
>> +                                       return -EINVAL;
>> +                               }
>> +                               /* Update the clock level diff */
>> +                               if (of_property_read_u8_array(cfg_np,
>> +                                        "config-clock-level",
>> +                                       &hdata->confs[i].conf[23], 1)) {
>> +                                       DRM_ERROR("Failed to get conf\n");
>> +                                       return -EINVAL;
>> +                               }
>> +               }
>> +       }
>> +       return 0;
>> +
>> +}
>> +
>>  static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
>>                                         (struct device *dev)
>>  {
>> @@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
>>                 goto err_hdmiphy;
>>         }
>>
>> +       /* get hdmiphy confs */
>> +       if (hdata->type == HDMI_TYPE14) {
>> +               ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
>> +               if (ret) {
>> +                       DRM_ERROR("failed to get user defined config,will use
>> +                       default configs, eye diagram tests may fail\n");
>
> build error?
>
>> +               }
>> +       }
>> +
>>         hdmi_display.dev = dev;
>>         exynos_drm_display_register(&hdmi_display);
>>
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Regards,
Shirish S
--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

This patch adds dt support to hdmiphy config settings
as it is board specific and depends on the signal pattern
of board.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   31 ++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
 2 files changed, 104 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
index 323983b..6eeb333 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -13,6 +13,30 @@ Required properties:
 	b) pin number within the gpio controller.
 	c) optional flags and pull up/down.
 
+- hdmiphy-configs: following information about the hdmiphy config settings.
+	a) "config<N>: config<N>" specifies the phy configuration settings,
+		where 'N' denotes the number of configuration, since every
+		pixel clock can have its unique configuration.
+		"pixel-clock" specifies the pixel clock
+		"conifig-de-emphasis-level" provides fine control of TMDS data
+		 pre emphasis, below shown is example for
+		data de-emphasis register at address 0x145D0040.
+			hdmiphy@38[16] for bits[3:0] permitted values are in
+			the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
+			increments for every LSB
+			hdmiphy@38[16] for bits[7:4] permitted values are in
+			the range of 0dB to -7.45dB at increments of -0.45dB
+			for every LSB.
+		"config-clock-level" provides fine control of TMDS data
+		amplitude for each channel,
+		for example if 0x145D005C is the address of clock level
+		register then,
+			hdmiphy@38[23] for bits [1:0] permitted values are in
+			the range of 0 mVdiff & 60 mVdiff for each channel at
+			increments 20 mVdiff of amplitude levels for every LSB,
+			hdmiphy@38[23] for bits [7:3] permitted values are in
+			the range of 790 and 1430 mV at 20mV increments for
+			every LSB.
 Example:
 
 	hdmi {
@@ -20,4 +44,11 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-configs {
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		}
 	};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 32ce9a6..7934c6e 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -197,6 +197,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -256,7 +259,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -785,8 +788,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 		confs = hdmiphy_v13_configs;
 		count = ARRAY_SIZE(hdmiphy_v13_configs);
 	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
+		confs = hdata->confs;
+		count = hdata->nr_confs;
 	} else
 		return -EINVAL;
 
@@ -1415,7 +1418,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 	if (hdata->type == HDMI_TYPE13)
 		hdmiphy_data = hdmiphy_v13_configs[i].conf;
 	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+		hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1894,6 +1897,63 @@ fail:
 	return -ENODEV;
 }
 
+static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
+						struct hdmi_context *hdata)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dev_np = dev->of_node;
+	struct device_node *phy_conf, *cfg_np;
+	int i, pixel_clock = 0;
+
+	/* Initialize with default config */
+	hdata->confs = hdmiphy_v14_configs;
+	hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+
+	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
+	if (phy_conf == NULL) {
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		DRM_ERROR("Did not find hdmiphy-configs node\n");
+		return -ENODEV;
+	}
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (!of_find_property(cfg_np, "pixel-clock", NULL))
+			continue;
+
+		if (of_property_read_u32(cfg_np, "pixel-clock",
+							&pixel_clock)) {
+				DRM_ERROR("Failed to get pixel clock\n");
+				return -EINVAL;
+		}
+
+		for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
+			if (hdata->confs[i].pixel_clock == pixel_clock)
+				/* Update the data de-emphasis and data level */
+				if (of_property_read_u8_array(cfg_np,
+					 "config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				if (of_property_read_u8_array(cfg_np,
+					 "config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				/* Update the clock level diff */
+				if (of_property_read_u8_array(cfg_np,
+					 "config-clock-level",
+					&hdata->confs[i].conf[23], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+		}
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* get hdmiphy confs */
+	if (hdata->type == HDMI_TYPE14) {
+		ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+		if (ret) {
+			DRM_ERROR("failed to get user defined config,will use
+			default configs, eye diagram tests may fail\n");
+		}
+	}
+
 	hdmi_display.dev = dev;
 	exynos_drm_display_register(&hdmi_display);
 
-- 
1.7.9.5

--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Tomasz Figa]

Hi Shirish,

Please see my comments inline.

On Monday 25 of November 2013 14:24:39 Shirish S wrote:
> This patch adds dt support to hdmiphy config settings
> as it is board specific and depends on the signal pattern
> of board.
> 
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> ---
>  .../devicetree/bindings/video/exynos_hdmi.txt      |   31 ++++++++
>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
>  2 files changed, 104 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> index 323983b..6eeb333 100644
> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> @@ -13,6 +13,30 @@ Required properties:
>  	b) pin number within the gpio controller.
>  	c) optional flags and pull up/down.
>  
> +- hdmiphy-configs: following information about the hdmiphy config settings.

Is this node required or optional? If it's required, then it breaks
compatibility with already existing DTBs, which is not desirable.

> +	a) "config<N>: config<N>" specifies the phy configuration settings,
> +		where 'N' denotes the number of configuration, since every
> +		pixel clock can have its unique configuration.

Node names should not have any semantic meaning for parsing code. I know
that there are already existing bindings which rely on presence of
particularly named nodes, but that's not right and new bindings should
not follow that.

Also what do you need the label of each config node for?

Generally from parsing perspective you shouldn't really care about node
names. All you seem to do in the driver is iterating over all specified
nodes and matching them with internal driver data using pixel clock
frequency.

> +		"pixel-clock" specifies the pixel clock

Vendor-specific properties should have vendor prefix, so this one should
be called "samsung,pixel-clock".

> +		"conifig-de-emphasis-level" provides fine control of TMDS data

Typo: s/conifig/config

Also it should be called "samsung,de-emphasis-level".

> +		 pre emphasis, below shown is example for
> +		data de-emphasis register at address 0x145D0040.
> +			hdmiphy@38[16] for bits[3:0] permitted values are in
> +			the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
> +			increments for every LSB
> +			hdmiphy@38[16] for bits[7:4] permitted values are in
> +			the range of 0dB to -7.45dB at increments of -0.45dB
> +			for every LSB.
> +		"config-clock-level" provides fine control of TMDS data

"samsung,clock-level"

> +		amplitude for each channel,
> +		for example if 0x145D005C is the address of clock level
[snip]
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 32ce9a6..5f599e3 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
[snip]
> +static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
> +						struct hdmi_context *hdata)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *dev_np = dev->of_node;
> +	struct device_node *phy_conf, *cfg_np;
> +	int i, pixel_clock = 0;
> +
> +	/* Initialize with default config */
> +	hdata->confs = hdmiphy_v14_configs;
> +	hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
> +
> +	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");

of_find_node_by_name() does not do what you need here. Please refer to
its implementation to learn why.

What you need here is of_get_child_by_name().

> +	if (phy_conf == NULL) {
> +		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
> +		DRM_ERROR("Did not find hdmiphy-configs node\n");
> +		return -ENODEV;
> +	}
> +
> +	for_each_child_of_node(phy_conf, cfg_np) {
> +		if (!of_find_property(cfg_np, "pixel-clock", NULL))
> +			continue;

This check is not needed. You can simply check the return value of
of_property_read_u32() below (as you already do anyway).

> +
> +		if (of_property_read_u32(cfg_np, "pixel-clock",
> +							&pixel_clock, 1)) {
> +				DRM_ERROR("Failed to get pixel clock\n");
> +				return -EINVAL;
> +		}
> +
> +		for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {

The code would be much cleaner if you simply used the loop to find the
config you need and then do the rest outside of the loop.

> +			if (hdata->confs[i].pixel_clock == pixel_clock)
> +				/* Update the data de-emphasis and data level */
> +				if (of_property_read_u8_array(cfg_np,
> +					 "config-de-emphasis-level",
> +					&hdata->confs[i].conf[16], 1)) {
> +					DRM_ERROR("Failed to get conf\n");
> +					return -EINVAL;
> +				}
> +				if (of_property_read_u8_array(cfg_np,
> +					 "config-de-emphasis-level",
> +					&hdata->confs[i].conf[16], 1)) {
> +					DRM_ERROR("Failed to get conf\n");
> +					return -EINVAL;
> +				}

Why do you parse this property twice?

> +				/* Update the clock level diff */
> +				if (of_property_read_u8_array(cfg_np,
> +					 "config-clock-level",
> +					&hdata->confs[i].conf[23], 1)) {
> +					DRM_ERROR("Failed to get conf\n");
> +					return -EINVAL;
> +				}
> +		}
> +	}
> +	return 0;
> +
> +}
> +
>  static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
>  					(struct device *dev)
>  {
> @@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
>  		goto err_hdmiphy;
>  	}
>  
> +	/* get hdmiphy confs */
> +	if (hdata->type == HDMI_TYPE14) {

Why is this used only for HDMI_TYPE14?

Best regards,
Tomasz

Re: [PATCH 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

Hi Tomasz,
Thanks for the reivew, please see my replies inline.

On Fri, Nov 29, 2013 at 10:56 PM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi Shirish,
>
> Please see my comments inline.
>
> On Monday 25 of November 2013 14:24:39 Shirish S wrote:
>> This patch adds dt support to hdmiphy config settings
>> as it is board specific and depends on the signal pattern
>> of board.
>>
>> Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  .../devicetree/bindings/video/exynos_hdmi.txt      |   31 ++++++++
>>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
>>  2 files changed, 104 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> index 323983b..6eeb333 100644
>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> @@ -13,6 +13,30 @@ Required properties:
>>       b) pin number within the gpio controller.
>>       c) optional flags and pull up/down.
>>
>> +- hdmiphy-configs: following information about the hdmiphy config settings.
>
> Is this node required or optional? If it's required, then it breaks
> compatibility with already existing DTBs, which is not desirable.
>
Yes its an Optional-but-recommended node, and i have mentioned the same
in this document in next patch set(v9).
>> +     a) "config<N>: config<N>" specifies the phy configuration settings,
>> +             where 'N' denotes the number of configuration, since every
>> +             pixel clock can have its unique configuration.
>
> Node names should not have any semantic meaning for parsing code. I know
> that there are already existing bindings which rely on presence of
> particularly named nodes, but that's not right and new bindings should
> not follow that.
>
I referred Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
for the implementation, am not clear with what you want me to do here, however
the requirement seems similar as pinctrl, can u kindly suggest any
existing newer
implementations to refer.
> Also what do you need the label of each config node for?
>
Each label here is a different pixel clock and corresponding phy setting, and
it may vary from one pixel clock to other hence i need one for each config node.
> Generally from parsing perspective you shouldn't really care about node
> names. All you seem to do in the driver is iterating over all specified
> nodes and matching them with internal driver data using pixel clock
> frequency.
>
True, that is what i intended to do.I think for the requirement
at hand, this should be fine.
>> +             "pixel-clock" specifies the pixel clock
>
> Vendor-specific properties should have vendor prefix, so this one should
> be called "samsung,pixel-clock".
>
Agreed, updated in the next patch set(v9).
>> +             "conifig-de-emphasis-level" provides fine control of TMDS data
>
> Typo: s/conifig/config
>
> Also it should be called "samsung,de-emphasis-level".
>
Agreed, updated in the next patch set(v9).
>> +              pre emphasis, below shown is example for
>> +             data de-emphasis register at address 0x145D0040.
>> +                     hdmiphy@38[16] for bits[3:0] permitted values are in
>> +                     the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
>> +                     increments for every LSB
>> +                     hdmiphy@38[16] for bits[7:4] permitted values are in
>> +                     the range of 0dB to -7.45dB at increments of -0.45dB
>> +                     for every LSB.
>> +             "config-clock-level" provides fine control of TMDS data
>
> "samsung,clock-level"
>
Agreed, updated in the next patch set(v9).
>> +             amplitude for each channel,
>> +             for example if 0x145D005C is the address of clock level
> [snip]
>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> index 32ce9a6..5f599e3 100644
>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> [snip]
>> +static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
>> +                                             struct hdmi_context *hdata)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct device_node *dev_np = dev->of_node;
>> +     struct device_node *phy_conf, *cfg_np;
>> +     int i, pixel_clock = 0;
>> +
>> +     /* Initialize with default config */
>> +     hdata->confs = hdmiphy_v14_configs;
>> +     hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
>> +
>> +     phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
>
> of_find_node_by_name() does not do what you need here. Please refer to
> its implementation to learn why.
>
> What you need here is of_get_child_by_name().
>
Agreed, updated in the next patch set(v9).
>> +     if (phy_conf == NULL) {
>> +             hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
>> +             DRM_ERROR("Did not find hdmiphy-configs node\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     for_each_child_of_node(phy_conf, cfg_np) {
>> +             if (!of_find_property(cfg_np, "pixel-clock", NULL))
>> +                     continue;
>
> This check is not needed. You can simply check the return value of
> of_property_read_u32() below (as you already do anyway).
>
Agreed, updated in the next patch set(v9).
>> +
>> +             if (of_property_read_u32(cfg_np, "pixel-clock",
>> +                                                     &pixel_clock, 1)) {
>> +                             DRM_ERROR("Failed to get pixel clock\n");
>> +                             return -EINVAL;
>> +             }
>> +
>> +             for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
>
> The code would be much cleaner if you simply used the loop to find the
> config you need and then do the rest outside of the loop.
>
As you can see below, i need to update 2 values in the phy array,
which are 16 and 23 indexed values,
for me to move this out of the for loop would need to add a 3
dimensional array and run this
for loop again. Can we consider the below to be ok for the requirement at hand?
>> +                     if (hdata->confs[i].pixel_clock == pixel_clock)
>> +                             /* Update the data de-emphasis and data level */
>> +                             if (of_property_read_u8_array(cfg_np,
>> +                                      "config-de-emphasis-level",
>> +                                     &hdata->confs[i].conf[16], 1)) {
>> +                                     DRM_ERROR("Failed to get conf\n");
>> +                                     return -EINVAL;
>> +                             }
>> +                             if (of_property_read_u8_array(cfg_np,
>> +                                      "config-de-emphasis-level",
>> +                                     &hdata->confs[i].conf[16], 1)) {
>> +                                     DRM_ERROR("Failed to get conf\n");
>> +                                     return -EINVAL;
>> +                             }
>
> Why do you parse this property twice?
>
My bad, have updated in the next patch set.
>> +                             /* Update the clock level diff */
>> +                             if (of_property_read_u8_array(cfg_np,
>> +                                      "config-clock-level",
>> +                                     &hdata->confs[i].conf[23], 1)) {
>> +                                     DRM_ERROR("Failed to get conf\n");
>> +                                     return -EINVAL;
>> +                             }
>> +             }
>> +     }
>> +     return 0;
>> +
>> +}
>> +
>>  static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
>>                                       (struct device *dev)
>>  {
>> @@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
>>               goto err_hdmiphy;
>>       }
>>
>> +     /* get hdmiphy confs */
>> +     if (hdata->type == HDMI_TYPE14) {
>
> Why is this used only for HDMI_TYPE14?
>
Have extended it to both HDMI_TYPE13 and 14.However i dont have tested
values for TYPE13,
hence this would be dummy for that version.

> Best regards,
> Tomasz
>
Thanks & Regards,
Shirish S
--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

This patch adds dt support to hdmiphy config settings
as it is board specific and depends on the signal pattern
of board.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   34 ++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   89 ++++++++++++++++----
 2 files changed, 105 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
index 323983b..0766e6e 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -13,6 +13,31 @@ Required properties:
 	b) pin number within the gpio controller.
 	c) optional flags and pull up/down.
 
+Optional-but-recommended properties:
+- hdmiphy-configs: following information about the hdmiphy config settings.
+	a) "config<N>: config<N>" specifies the phy configuration settings,
+		where 'N' denotes the number of configuration, since every
+		pixel clock can have its unique configuration.
+		"samsung,pixel-clock" specifies the pixel clock
+		"samsung,de-emphasis-level" provides fine control of TMDS data
+		 pre emphasis, below shown is example for
+		data de-emphasis register at address 0x145D0040.
+			hdmiphy@38[16] for bits[3:0] permitted values are in
+			the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
+			increments for every LSB
+			hdmiphy@38[16] for bits[7:4] permitted values are in
+			the range of 0dB to -7.45dB at increments of -0.45dB
+			for every LSB.
+		"samsung,clock-level" provides fine control of TMDS data
+		amplitude for each channel,
+		for example if 0x145D005C is the address of clock level
+		register then,
+			hdmiphy@38[23] for bits [1:0] permitted values are in
+			the range of 0 mVdiff & 60 mVdiff for each channel at
+			increments 20 mVdiff of amplitude levels for every LSB,
+			hdmiphy@38[23] for bits [7:3] permitted values are in
+			the range of 790 and 1430 mV at 20mV increments for
+			every LSB.
 Example:
 
 	hdmi {
@@ -20,4 +45,13 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-configs {
+			config0: config0 {
+				samsung,pixel-clock = <25200000>;
+				samsung,de-emphasis-level =  /bits/ 8 <0x26>;
+				samsung,clock-level =  /bits/ 8 < 0x66>;
+			};
+
+			/* ... */
+		}
 	};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index a0e10ae..2fa0074 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -200,6 +200,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -259,7 +262,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -771,20 +774,10 @@ static struct edid *hdmi_get_edid(void *ctx, struct drm_connector *connector)
 
 static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 {
-	const struct hdmiphy_config *confs;
-	int count, i;
-
-	if (hdata->type == HDMI_TYPE13) {
-		confs = hdmiphy_v13_configs;
-		count = ARRAY_SIZE(hdmiphy_v13_configs);
-	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
-	} else
-		return -EINVAL;
+	int i;
 
-	for (i = 0; i < count; i++)
-		if (confs[i].pixel_clock == pixel_clock)
+	for (i = 0; i < hdata->nr_confs; i++)
+		if (hdata->confs[i].pixel_clock == pixel_clock)
 			return i;
 
 	DRM_DEBUG_KMS("Could not find phy config for %d\n", pixel_clock);
@@ -1363,10 +1356,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 		return;
 	}
 
-	if (hdata->type == HDMI_TYPE13)
-		hdmiphy_data = hdmiphy_v13_configs[i].conf;
-	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+	hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1858,6 +1848,62 @@ void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
 		hdmi_hdmiphy = hdmiphy;
 }
 
+static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
+						struct hdmi_context *hdata)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dev_np = dev->of_node;
+	struct device_node *phy_conf, *cfg_np;
+	int i, pixel_clock = 0;
+
+	/* Initialize with default config */
+	switch (hdata->type) {
+	case HDMI_TYPE13:
+		hdata->confs = hdmiphy_v13_configs;
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v13_configs);
+		break;
+	case HDMI_TYPE14:
+		hdata->confs = hdmiphy_v14_configs;
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		break;
+	default:
+		DRM_ERROR("Did not find valid HDMI version\n");
+		break;
+	}
+
+	phy_conf = of_get_child_by_name(dev_np, "hdmiphy-configs");
+	if (phy_conf == NULL) {
+		DRM_ERROR("Did not find hdmiphy-configs node\n");
+		return -ENODEV;
+	}
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (of_property_read_u32(cfg_np, "samsung,pixel-clock",
+							&pixel_clock))
+			continue;
+
+		for (i = 0; i < ARRAY_SIZE(hdata->nr_confs); i++) {
+			if (hdata->confs[i].pixel_clock == pixel_clock)
+				/* Update the data de-emphasis and data level */
+				if (of_property_read_u8_array(cfg_np,
+					 "samsung,config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				/* Update the clock level diff */
+				if (of_property_read_u8_array(cfg_np,
+					 "samsung,config-clock-level",
+					&hdata->confs[i].conf[23], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+		}
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -1986,6 +2032,13 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* assign hdmiphy configurations */
+	ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+	if (ret) {
+		DRM_ERROR("failed to get user defined config,will use
+			default configs, eye diagram tests may fail\n");
+	}
+
 	/* Attach HDMI Driver to common hdmi. */
 	exynos_hdmi_drv_attach(drm_hdmi_ctx);
 
-- 
1.7.9.5

--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

+ linux-samsung-soc mailing list.

On Wed, Dec 4, 2013 at 10:05 AM, Shirish S <shirish@chromium.org> wrote:
> Hi Tomasz,
> Thanks for the reivew, please see my replies inline.
>
> On Fri, Nov 29, 2013 at 10:56 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Shirish,
>>
>> Please see my comments inline.
>>
>> On Monday 25 of November 2013 14:24:39 Shirish S wrote:
>>> This patch adds dt support to hdmiphy config settings
>>> as it is board specific and depends on the signal pattern
>>> of board.
>>>
>>> Signed-off-by: Shirish S <s.shirish@samsung.com>
>>> ---
>>>  .../devicetree/bindings/video/exynos_hdmi.txt      |   31 ++++++++
>>>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
>>>  2 files changed, 104 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>> index 323983b..6eeb333 100644
>>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>> @@ -13,6 +13,30 @@ Required properties:
>>>       b) pin number within the gpio controller.
>>>       c) optional flags and pull up/down.
>>>
>>> +- hdmiphy-configs: following information about the hdmiphy config settings.
>>
>> Is this node required or optional? If it's required, then it breaks
>> compatibility with already existing DTBs, which is not desirable.
>>
> Yes its an Optional-but-recommended node, and i have mentioned the same
> in this document in next patch set(v9).
>>> +     a) "config<N>: config<N>" specifies the phy configuration settings,
>>> +             where 'N' denotes the number of configuration, since every
>>> +             pixel clock can have its unique configuration.
>>
>> Node names should not have any semantic meaning for parsing code. I know
>> that there are already existing bindings which rely on presence of
>> particularly named nodes, but that's not right and new bindings should
>> not follow that.
>>
> I referred Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> for the implementation, am not clear with what you want me to do here, however
> the requirement seems similar as pinctrl, can u kindly suggest any
> existing newer
> implementations to refer.
>> Also what do you need the label of each config node for?
>>
> Each label here is a different pixel clock and corresponding phy setting, and
> it may vary from one pixel clock to other hence i need one for each config node.
>> Generally from parsing perspective you shouldn't really care about node
>> names. All you seem to do in the driver is iterating over all specified
>> nodes and matching them with internal driver data using pixel clock
>> frequency.
>>
> True, that is what i intended to do.I think for the requirement
> at hand, this should be fine.
>>> +             "pixel-clock" specifies the pixel clock
>>
>> Vendor-specific properties should have vendor prefix, so this one should
>> be called "samsung,pixel-clock".
>>
> Agreed, updated in the next patch set(v9).
>>> +             "conifig-de-emphasis-level" provides fine control of TMDS data
>>
>> Typo: s/conifig/config
>>
>> Also it should be called "samsung,de-emphasis-level".
>>
> Agreed, updated in the next patch set(v9).
>>> +              pre emphasis, below shown is example for
>>> +             data de-emphasis register at address 0x145D0040.
>>> +                     hdmiphy@38[16] for bits[3:0] permitted values are in
>>> +                     the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
>>> +                     increments for every LSB
>>> +                     hdmiphy@38[16] for bits[7:4] permitted values are in
>>> +                     the range of 0dB to -7.45dB at increments of -0.45dB
>>> +                     for every LSB.
>>> +             "config-clock-level" provides fine control of TMDS data
>>
>> "samsung,clock-level"
>>
> Agreed, updated in the next patch set(v9).
>>> +             amplitude for each channel,
>>> +             for example if 0x145D005C is the address of clock level
>> [snip]
>>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>>> index 32ce9a6..5f599e3 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> [snip]
>>> +static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
>>> +                                             struct hdmi_context *hdata)
>>> +{
>>> +     struct device *dev = &pdev->dev;
>>> +     struct device_node *dev_np = dev->of_node;
>>> +     struct device_node *phy_conf, *cfg_np;
>>> +     int i, pixel_clock = 0;
>>> +
>>> +     /* Initialize with default config */
>>> +     hdata->confs = hdmiphy_v14_configs;
>>> +     hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
>>> +
>>> +     phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
>>
>> of_find_node_by_name() does not do what you need here. Please refer to
>> its implementation to learn why.
>>
>> What you need here is of_get_child_by_name().
>>
> Agreed, updated in the next patch set(v9).
>>> +     if (phy_conf == NULL) {
>>> +             hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
>>> +             DRM_ERROR("Did not find hdmiphy-configs node\n");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     for_each_child_of_node(phy_conf, cfg_np) {
>>> +             if (!of_find_property(cfg_np, "pixel-clock", NULL))
>>> +                     continue;
>>
>> This check is not needed. You can simply check the return value of
>> of_property_read_u32() below (as you already do anyway).
>>
> Agreed, updated in the next patch set(v9).
>>> +
>>> +             if (of_property_read_u32(cfg_np, "pixel-clock",
>>> +                                                     &pixel_clock, 1)) {
>>> +                             DRM_ERROR("Failed to get pixel clock\n");
>>> +                             return -EINVAL;
>>> +             }
>>> +
>>> +             for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
>>
>> The code would be much cleaner if you simply used the loop to find the
>> config you need and then do the rest outside of the loop.
>>
> As you can see below, i need to update 2 values in the phy array,
> which are 16 and 23 indexed values,
> for me to move this out of the for loop would need to add a 3
> dimensional array and run this
> for loop again. Can we consider the below to be ok for the requirement at hand?
>>> +                     if (hdata->confs[i].pixel_clock == pixel_clock)
>>> +                             /* Update the data de-emphasis and data level */
>>> +                             if (of_property_read_u8_array(cfg_np,
>>> +                                      "config-de-emphasis-level",
>>> +                                     &hdata->confs[i].conf[16], 1)) {
>>> +                                     DRM_ERROR("Failed to get conf\n");
>>> +                                     return -EINVAL;
>>> +                             }
>>> +                             if (of_property_read_u8_array(cfg_np,
>>> +                                      "config-de-emphasis-level",
>>> +                                     &hdata->confs[i].conf[16], 1)) {
>>> +                                     DRM_ERROR("Failed to get conf\n");
>>> +                                     return -EINVAL;
>>> +                             }
>>
>> Why do you parse this property twice?
>>
> My bad, have updated in the next patch set.
>>> +                             /* Update the clock level diff */
>>> +                             if (of_property_read_u8_array(cfg_np,
>>> +                                      "config-clock-level",
>>> +                                     &hdata->confs[i].conf[23], 1)) {
>>> +                                     DRM_ERROR("Failed to get conf\n");
>>> +                                     return -EINVAL;
>>> +                             }
>>> +             }
>>> +     }
>>> +     return 0;
>>> +
>>> +}
>>> +
>>>  static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
>>>                                       (struct device *dev)
>>>  {
>>> @@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
>>>               goto err_hdmiphy;
>>>       }
>>>
>>> +     /* get hdmiphy confs */
>>> +     if (hdata->type == HDMI_TYPE14) {
>>
>> Why is this used only for HDMI_TYPE14?
>>
> Have extended it to both HDMI_TYPE13 and 14.However i dont have tested
> values for TYPE13,
> hence this would be dummy for that version.
>
>> Best regards,
>> Tomasz
>>
> Thanks & Regards,
> Shirish S

[PATCH 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

This patch adds dt support to hdmiphy config settings
as it is board specific and depends on the signal pattern
of board.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   34 ++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   89 ++++++++++++++++----
 2 files changed, 105 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
index 323983b..0766e6e 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -13,6 +13,31 @@ Required properties:
 	b) pin number within the gpio controller.
 	c) optional flags and pull up/down.
 
+Optional-but-recommended properties:
+- hdmiphy-configs: following information about the hdmiphy config settings.
+	a) "config<N>: config<N>" specifies the phy configuration settings,
+		where 'N' denotes the number of configuration, since every
+		pixel clock can have its unique configuration.
+		"samsung,pixel-clock" specifies the pixel clock
+		"samsung,de-emphasis-level" provides fine control of TMDS data
+		 pre emphasis, below shown is example for
+		data de-emphasis register at address 0x145D0040.
+			hdmiphy@38[16] for bits[3:0] permitted values are in
+			the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
+			increments for every LSB
+			hdmiphy@38[16] for bits[7:4] permitted values are in
+			the range of 0dB to -7.45dB at increments of -0.45dB
+			for every LSB.
+		"samsung,clock-level" provides fine control of TMDS data
+		amplitude for each channel,
+		for example if 0x145D005C is the address of clock level
+		register then,
+			hdmiphy@38[23] for bits [1:0] permitted values are in
+			the range of 0 mVdiff & 60 mVdiff for each channel at
+			increments 20 mVdiff of amplitude levels for every LSB,
+			hdmiphy@38[23] for bits [7:3] permitted values are in
+			the range of 790 and 1430 mV at 20mV increments for
+			every LSB.
 Example:
 
 	hdmi {
@@ -20,4 +45,13 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-configs {
+			config0: config0 {
+				samsung,pixel-clock = <25200000>;
+				samsung,de-emphasis-level =  /bits/ 8 <0x26>;
+				samsung,clock-level =  /bits/ 8 < 0x66>;
+			};
+
+			/* ... */
+		}
 	};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index a0e10ae..2fa0074 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -200,6 +200,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -259,7 +262,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -771,20 +774,10 @@ static struct edid *hdmi_get_edid(void *ctx, struct drm_connector *connector)
 
 static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 {
-	const struct hdmiphy_config *confs;
-	int count, i;
-
-	if (hdata->type == HDMI_TYPE13) {
-		confs = hdmiphy_v13_configs;
-		count = ARRAY_SIZE(hdmiphy_v13_configs);
-	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
-	} else
-		return -EINVAL;
+	int i;
 
-	for (i = 0; i < count; i++)
-		if (confs[i].pixel_clock == pixel_clock)
+	for (i = 0; i < hdata->nr_confs; i++)
+		if (hdata->confs[i].pixel_clock == pixel_clock)
 			return i;
 
 	DRM_DEBUG_KMS("Could not find phy config for %d\n", pixel_clock);
@@ -1363,10 +1356,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 		return;
 	}
 
-	if (hdata->type == HDMI_TYPE13)
-		hdmiphy_data = hdmiphy_v13_configs[i].conf;
-	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+	hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1858,6 +1848,62 @@ void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
 		hdmi_hdmiphy = hdmiphy;
 }
 
+static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
+						struct hdmi_context *hdata)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dev_np = dev->of_node;
+	struct device_node *phy_conf, *cfg_np;
+	int i, pixel_clock = 0;
+
+	/* Initialize with default config */
+	switch (hdata->type) {
+	case HDMI_TYPE13:
+		hdata->confs = hdmiphy_v13_configs;
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v13_configs);
+		break;
+	case HDMI_TYPE14:
+		hdata->confs = hdmiphy_v14_configs;
+		hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
+		break;
+	default:
+		DRM_ERROR("Did not find valid HDMI version\n");
+		break;
+	}
+
+	phy_conf = of_get_child_by_name(dev_np, "hdmiphy-configs");
+	if (phy_conf == NULL) {
+		DRM_ERROR("Did not find hdmiphy-configs node\n");
+		return -ENODEV;
+	}
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (of_property_read_u32(cfg_np, "samsung,pixel-clock",
+							&pixel_clock))
+			continue;
+
+		for (i = 0; i < ARRAY_SIZE(hdata->nr_confs); i++) {
+			if (hdata->confs[i].pixel_clock == pixel_clock)
+				/* Update the data de-emphasis and data level */
+				if (of_property_read_u8_array(cfg_np,
+					 "samsung,config-de-emphasis-level",
+					&hdata->confs[i].conf[16], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+				/* Update the clock level diff */
+				if (of_property_read_u8_array(cfg_np,
+					 "samsung,config-clock-level",
+					&hdata->confs[i].conf[23], 1)) {
+					DRM_ERROR("Failed to get conf\n");
+					return -EINVAL;
+				}
+		}
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -1986,6 +2032,13 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* assign hdmiphy configurations */
+	ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+	if (ret) {
+		DRM_ERROR("failed to get user defined config,will use
+			default configs, eye diagram tests may fail\n");
+	}
+
 	/* Attach HDMI Driver to common hdmi. */
 	exynos_hdmi_drv_attach(drm_hdmi_ctx);
 
-- 
1.7.9.5

--
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 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Tomasz Figa]

On Thursday 19 of December 2013 17:42:28 Shirish S wrote:
> This patch adds dt support to hdmiphy config settings
> as it is board specific and depends on the signal pattern
> of board.
> 
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> ---
>  .../devicetree/bindings/video/exynos_hdmi.txt      |   34 ++++++++
>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   89 ++++++++++++++++----
>  2 files changed, 105 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> index 323983b..0766e6e 100644
> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> @@ -13,6 +13,31 @@ Required properties:
>  	b) pin number within the gpio controller.
>  	c) optional flags and pull up/down.
>  
> +Optional-but-recommended properties:
> +- hdmiphy-configs: following information about the hdmiphy config settings.
> +	a) "config<N>: config<N>" specifies the phy configuration settings,

Why do you need this "config<N>: " part? (This is called "label" in DT
terminology by the way and can be used to reference the node from
properties of other nodes, by so called "phandle".)

> +		where 'N' denotes the number of configuration, since every
> +		pixel clock can have its unique configuration.
> +		"samsung,pixel-clock" specifies the pixel clock
> +		"samsung,de-emphasis-level" provides fine control of TMDS data
> +		 pre emphasis, below shown is example for
> +		data de-emphasis register at address 0x145D0040.
> +			hdmiphy@38[16] for bits[3:0] permitted values are in
> +			the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
> +			increments for every LSB
> +			hdmiphy@38[16] for bits[7:4] permitted values are in
> +			the range of 0dB to -7.45dB at increments of -0.45dB
> +			for every LSB.
> +		"samsung,clock-level" provides fine control of TMDS data
> +		amplitude for each channel,
> +		for example if 0x145D005C is the address of clock level
> +		register then,
> +			hdmiphy@38[23] for bits [1:0] permitted values are in
> +			the range of 0 mVdiff & 60 mVdiff for each channel at
> +			increments 20 mVdiff of amplitude levels for every LSB,
> +			hdmiphy@38[23] for bits [7:3] permitted values are in
> +			the range of 790 and 1430 mV at 20mV increments for
> +			every LSB.
>  Example:
>  
>  	hdmi {
> @@ -20,4 +45,13 @@ Example:
>  		reg = <0x14530000 0x100000>;
>  		interrupts = <0 95 0>;
>  		hpd-gpio = <&gpx3 7 1>;
> +		hdmiphy-configs {
> +			config0: config0 {
> +				samsung,pixel-clock = <25200000>;
> +				samsung,de-emphasis-level =  /bits/ 8 <0x26>;

nit: Two spaces before "/bits/".

> +				samsung,clock-level =  /bits/ 8 < 0x66>;

nit: Two spaces before "/bits/" and incorrect space after "<".

Generally the list of configurations should look like below:

	phy-configs {
		#address-cells = <1>;
		#size-cells = <0>;

		config@0 {
			reg = <0>;
			/* other properties... */
		};

		config@1 {
			reg = <1>;
			/* other properties... */
		};

		/* ... */
	};

This is how bus-like structures should be represented in device tree.
Also, since this is HDMI node, maybe it's enough to call the node simply
phy-configs. Please rework the patches to use this correct representation.

> +
> +			/* ... */
> +		}
>  	};
[snip]
> +	for_each_child_of_node(phy_conf, cfg_np) {
> +		if (of_property_read_u32(cfg_np, "samsung,pixel-clock",
> +							&pixel_clock))
> +			continue;
> +
> +		for (i = 0; i < ARRAY_SIZE(hdata->nr_confs); i++) {
> +			if (hdata->confs[i].pixel_clock == pixel_clock)

Can you have more than one config with the same pixel clock?

Even if not, the code could be made more readable if the code
below is moved outside the if and continue keyword is used instead.

Best regards,
Tomasz

Re: [PATCH 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

[Shirish S]

Hi Tomasz,
Thanks for the review comments, please find my replies inline.

On Thu, Dec 19, 2013 at 6:49 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> On Thursday 19 of December 2013 17:42:28 Shirish S wrote:
>> This patch adds dt support to hdmiphy config settings
>> as it is board specific and depends on the signal pattern
>> of board.
>>
>> Signed-off-by: Shirish S <s.shirish@samsung.com>
>> ---
>>  .../devicetree/bindings/video/exynos_hdmi.txt      |   34 ++++++++
>>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   89 ++++++++++++++++----
>>  2 files changed, 105 insertions(+), 18 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> index 323983b..0766e6e 100644
>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> @@ -13,6 +13,31 @@ Required properties:
>>       b) pin number within the gpio controller.
>>       c) optional flags and pull up/down.
>>
>> +Optional-but-recommended properties:
>> +- hdmiphy-configs: following information about the hdmiphy config settings.
>> +     a) "config<N>: config<N>" specifies the phy configuration settings,
>
> Why do you need this "config<N>: " part? (This is called "label" in DT
> terminology by the way and can be used to reference the node from
> properties of other nodes, by so called "phandle".)
>
The config is required for every pixel clock that the IP supports,
since in the parsing i iterate through all pixel clocks, i have used 'N'.
However, if you proposed approach below is ok, then all of this shall
be removed.
>> +             where 'N' denotes the number of configuration, since every
>> +             pixel clock can have its unique configuration.
>> +             "samsung,pixel-clock" specifies the pixel clock
>> +             "samsung,de-emphasis-level" provides fine control of TMDS data
>> +              pre emphasis, below shown is example for
>> +             data de-emphasis register at address 0x145D0040.
>> +                     hdmiphy@38[16] for bits[3:0] permitted values are in
>> +                     the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
>> +                     increments for every LSB
>> +                     hdmiphy@38[16] for bits[7:4] permitted values are in
>> +                     the range of 0dB to -7.45dB at increments of -0.45dB
>> +                     for every LSB.
>> +             "samsung,clock-level" provides fine control of TMDS data
>> +             amplitude for each channel,
>> +             for example if 0x145D005C is the address of clock level
>> +             register then,
>> +                     hdmiphy@38[23] for bits [1:0] permitted values are in
>> +                     the range of 0 mVdiff & 60 mVdiff for each channel at
>> +                     increments 20 mVdiff of amplitude levels for every LSB,
>> +                     hdmiphy@38[23] for bits [7:3] permitted values are in
>> +                     the range of 790 and 1430 mV at 20mV increments for
>> +                     every LSB.
>>  Example:
>>
>>       hdmi {
>> @@ -20,4 +45,13 @@ Example:
>>               reg = <0x14530000 0x100000>;
>>               interrupts = <0 95 0>;
>>               hpd-gpio = <&gpx3 7 1>;
>> +             hdmiphy-configs {
>> +                     config0: config0 {
>> +                             samsung,pixel-clock = <25200000>;
>> +                             samsung,de-emphasis-level =  /bits/ 8 <0x26>;
>
> nit: Two spaces before "/bits/".
have corrected in the next patchset.
>
>> +                             samsung,clock-level =  /bits/ 8 < 0x66>;
>
> nit: Two spaces before "/bits/" and incorrect space after "<".
have corrected in the next patchset.
>
> Generally the list of configurations should look like below:
>
>         phy-configs {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 config@0 {
>                         reg = <0>;
>                         /* other properties... */
>                 };
>
>                 config@1 {
>                         reg = <1>;
>                         /* other properties... */
>                 };
>
>                 /* ... */
>         };
>
> This is how bus-like structures should be represented in device tree.
> Also, since this is HDMI node, maybe it's enough to call the node simply
> phy-configs. Please rework the patches to use this correct representation.
>
I think there is slight misunderstanding here, the configs that i want
to mention are not physical entities,and
hence dont think would be a better way, i am planning to use the below
method, if you think its the right approach then i shall do the rework
and submit the patch:
phy-configs {
                        #pixel-clock = <1>;
                        #de-emphasis-level = <1>;
                        #clock-level = <1>;
                        phy-map = <25200000 0x26 0x66>,
                                        <27000000  0x26 0x66>,
                                        /*....so on....*/,
                };>> +
>> +                     /* ... */
>> +             }
>>       };
> [snip]
>> +     for_each_child_of_node(phy_conf, cfg_np) {
>> +             if (of_property_read_u32(cfg_np, "samsung,pixel-clock",
>> +                                                     &pixel_clock))
>> +                     continue;
>> +
>> +             for (i = 0; i < ARRAY_SIZE(hdata->nr_confs); i++) {
>> +                     if (hdata->confs[i].pixel_clock == pixel_clock)
>
> Can you have more than one config with the same pixel clock?
>
No, right now i intend to have one config parameters for every
corresponding pixel clock.
> Even if not, the code could be made more readable if the code
> below is moved outside the if and continue keyword is used instead.
>
The parsing mechanism shall be altered according to the new approach
proposed above.
Kindly let me know your thoughts about the approach.

> Best regards,
> Tomasz
>
Thanks & Regards,
Shirish S