[PATCH 00/12] OMAPDSS: use DSI PLL clk for DPI

[PATCH 00/12] OMAPDSS: use DSI PLL clk for DPI

[Tomi Valkeinen]

Hi,

The aim of this series is to enable using DSI PLL as the pixel clock source for
DPI output. There are the following main parts:

* first 4 patches are slightly related generic improvements/fixes
* fix DSI PLL problem related to non-50% duty cycle
* fix DSI PLL problem related to powering
* changes to the clock handling to always use DSI PLL for DPI when possible

I've tested the series on OMAP4 Panda, OMAP4 SDP and OMAP3 Beagle.

 Tomi

Tomi Valkeinen (12):
  OMAPFB: remove use of extended edid block
  OMAPFB: improve mode selection from EDID
  OMAPDSS: fix DPI & DSI init order
  OMAPDSS: fix DSI2 PLL clk names
  OMAPDSS: DSI: skip odd dividers when pck >= 100MHz
  OMAPDSS: DSI: workaround for HSDiv problem
  OMAPDSS: add dss_calc_clock_rates() back
  OMAPDSS: setup default dss fck
  OMAPDSS: hide dss_select_dispc_clk_source()
  OMAPDSS: DPI: use dpi.dsidev to see whether to use dsi pll
  OMAPDSS: DPI: verify if DSI PLL is operational
  OMAPDSS: DPI: always use DSI PLL if available

 drivers/video/omap2/dss/core.c           |   12 ++--
 drivers/video/omap2/dss/dpi.c            |   99 +++++++++++++++++++++---------
 drivers/video/omap2/dss/dsi.c            |   14 ++++-
 drivers/video/omap2/dss/dss.c            |   67 +++++++++++++++++++-
 drivers/video/omap2/dss/dss.h            |    2 +-
 drivers/video/omap2/dss/hdmi.c           |    8 ---
 drivers/video/omap2/omapfb/omapfb-main.c |   16 ++---
 7 files changed, 160 insertions(+), 58 deletions(-)

-- 
1.7.10.4

[PATCH 00/12] OMAPDSS: use DSI PLL clk for DPI

[Tomi Valkeinen]

Hi,

The aim of this series is to enable using DSI PLL as the pixel clock source for
DPI output. There are the following main parts:

* first 4 patches are slightly related generic improvements/fixes
* fix DSI PLL problem related to non-50% duty cycle
* fix DSI PLL problem related to powering
* changes to the clock handling to always use DSI PLL for DPI when possible

I've tested the series on OMAP4 Panda, OMAP4 SDP and OMAP3 Beagle.

 Tomi

Tomi Valkeinen (12):
  OMAPFB: remove use of extended edid block
  OMAPFB: improve mode selection from EDID
  OMAPDSS: fix DPI & DSI init order
  OMAPDSS: fix DSI2 PLL clk names
  OMAPDSS: DSI: skip odd dividers when pck >= 100MHz
  OMAPDSS: DSI: workaround for HSDiv problem
  OMAPDSS: add dss_calc_clock_rates() back
  OMAPDSS: setup default dss fck
  OMAPDSS: hide dss_select_dispc_clk_source()
  OMAPDSS: DPI: use dpi.dsidev to see whether to use dsi pll
  OMAPDSS: DPI: verify if DSI PLL is operational
  OMAPDSS: DPI: always use DSI PLL if available

 drivers/video/omap2/dss/core.c           |   12 ++--
 drivers/video/omap2/dss/dpi.c            |   99 +++++++++++++++++++++---------
 drivers/video/omap2/dss/dsi.c            |   14 ++++-
 drivers/video/omap2/dss/dss.c            |   67 +++++++++++++++++++-
 drivers/video/omap2/dss/dss.h            |    2 +-
 drivers/video/omap2/dss/hdmi.c           |    8 ---
 drivers/video/omap2/omapfb/omapfb-main.c |   16 ++---
 7 files changed, 160 insertions(+), 58 deletions(-)

-- 
1.7.10.4

[PATCH 01/12] OMAPFB: remove use of extended edid block

[Tomi Valkeinen]

It seems that using the second EDID block causes more problems than is
of any help. The first mode in the extended block will get
FB_MODE_IS_FIRST set, which will override the first mode from the first
EDID block, thus making the default videomode selection not to work
properly.

This patch removes the use of the extended edid block for now.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/omapfb/omapfb-main.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index bc225e4..0358b14 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2274,9 +2274,6 @@ static int omapfb_find_best_mode(struct omap_dss_device *display,
 
 	fb_edid_to_monspecs(edid, specs);
 
-	if (edid[126] > 0)
-		fb_edid_add_monspecs(edid + 0x80, specs);
-
 	best_xres = 0;
 	best_idx = -1;
 
-- 
1.7.10.4

[PATCH 01/12] OMAPFB: remove use of extended edid block

[Tomi Valkeinen]

It seems that using the second EDID block causes more problems than is
of any help. The first mode in the extended block will get
FB_MODE_IS_FIRST set, which will override the first mode from the first
EDID block, thus making the default videomode selection not to work
properly.

This patch removes the use of the extended edid block for now.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/omapfb/omapfb-main.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index bc225e4..0358b14 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2274,9 +2274,6 @@ static int omapfb_find_best_mode(struct omap_dss_device *display,
 
 	fb_edid_to_monspecs(edid, specs);
 
-	if (edid[126] > 0)
-		fb_edid_add_monspecs(edid + 0x80, specs);
-
 	best_xres = 0;
 	best_idx = -1;
 
-- 
1.7.10.4

Re: [PATCH 01/12] OMAPFB: remove use of extended edid block

[Archit Taneja]

On Tuesday 30 October 2012 09:39 PM, Tomi Valkeinen wrote:
> It seems that using the second EDID block causes more problems than is
> of any help. The first mode in the extended block will get
> FB_MODE_IS_FIRST set, which will override the first mode from the first
> EDID block, thus making the default videomode selection not to work
> properly.
>
> This patch removes the use of the extended edid block for now.

Looks like you had posted this one(and also the next one)in the previous 
cleanup/fixes series also.

Archit

Re: [PATCH 01/12] OMAPFB: remove use of extended edid block

[Archit Taneja]

On Tuesday 30 October 2012 09:39 PM, Tomi Valkeinen wrote:
> It seems that using the second EDID block causes more problems than is
> of any help. The first mode in the extended block will get
> FB_MODE_IS_FIRST set, which will override the first mode from the first
> EDID block, thus making the default videomode selection not to work
> properly.
>
> This patch removes the use of the extended edid block for now.

Looks like you had posted this one(and also the next one)in the previous 
cleanup/fixes series also.

Archit

Re: [PATCH 01/12] OMAPFB: remove use of extended edid block

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On 2012-10-31 08:10, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:39 PM, Tomi Valkeinen wrote:
>> It seems that using the second EDID block causes more problems than is
>> of any help. The first mode in the extended block will get
>> FB_MODE_IS_FIRST set, which will override the first mode from the first
>> EDID block, thus making the default videomode selection not to work
>> properly.
>>
>> This patch removes the use of the extended edid block for now.
> 
> Looks like you had posted this one(and also the next one)in the previous
> cleanup/fixes series also.

Ah, so I did. Too many work branches =). Thanks for pointing it out.

 Tomi




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 01/12] OMAPFB: remove use of extended edid block

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On 2012-10-31 08:10, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:39 PM, Tomi Valkeinen wrote:
>> It seems that using the second EDID block causes more problems than is
>> of any help. The first mode in the extended block will get
>> FB_MODE_IS_FIRST set, which will override the first mode from the first
>> EDID block, thus making the default videomode selection not to work
>> properly.
>>
>> This patch removes the use of the extended edid block for now.
> 
> Looks like you had posted this one(and also the next one)in the previous
> cleanup/fixes series also.

Ah, so I did. Too many work branches =). Thanks for pointing it out.

 Tomi




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[PATCH 02/12] OMAPFB: improve mode selection from EDID

[Tomi Valkeinen]

The current omapfb code goes over all the modes found from the monitors
EDID data, and searches for a mode that is compatible with the DSS
hardware and has the highest x-res.

While this works ok as such, it proves problematic when using DSI PLL
for pixel clock. Calculating DSI PLL dividers is not the fastest of the
operations, and while doing it for one mode is usually ok, doing it for
20 modes is noticable.

Also, the first mode given in the EDID data should be the native mode of
the monitor, and thus also the best mode, so if that can be used, no
need to look further.

This patch changes the code to use the first mode that is compatible
with the DSS hardware.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/omapfb/omapfb-main.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 0358b14..6e5334c 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2258,7 +2258,7 @@ static int omapfb_find_best_mode(struct omap_dss_device *display,
 {
 	struct fb_monspecs *specs;
 	u8 *edid;
-	int r, i, best_xres, best_idx, len;
+	int r, i, best_idx, len;
 
 	if (!display->driver->read_edid)
 		return -ENODEV;
@@ -2274,7 +2274,6 @@ static int omapfb_find_best_mode(struct omap_dss_device *display,
 
 	fb_edid_to_monspecs(edid, specs);
 
-	best_xres = 0;
 	best_idx = -1;
 
 	for (i = 0; i < specs->modedb_len; ++i) {
@@ -2290,16 +2289,20 @@ static int omapfb_find_best_mode(struct omap_dss_device *display,
 		if (m->xres = 2880 || m->xres = 1440)
 			continue;
 
+		if (m->vmode & FB_VMODE_INTERLACED ||
+				m->vmode & FB_VMODE_DOUBLE)
+			continue;
+
 		fb_videomode_to_omap_timings(m, display, &t);
 
 		r = display->driver->check_timings(display, &t);
-		if (r = 0 && best_xres < m->xres) {
-			best_xres = m->xres;
+		if (r = 0) {
 			best_idx = i;
+			break;
 		}
 	}
 
-	if (best_xres = 0) {
+	if (best_idx = -1) {
 		r = -ENOENT;
 		goto err2;
 	}
-- 
1.7.10.4

[PATCH 02/12] OMAPFB: improve mode selection from EDID

[Tomi Valkeinen]

The current omapfb code goes over all the modes found from the monitors
EDID data, and searches for a mode that is compatible with the DSS
hardware and has the highest x-res.

While this works ok as such, it proves problematic when using DSI PLL
for pixel clock. Calculating DSI PLL dividers is not the fastest of the
operations, and while doing it for one mode is usually ok, doing it for
20 modes is noticable.

Also, the first mode given in the EDID data should be the native mode of
the monitor, and thus also the best mode, so if that can be used, no
need to look further.

This patch changes the code to use the first mode that is compatible
with the DSS hardware.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/omapfb/omapfb-main.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 0358b14..6e5334c 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2258,7 +2258,7 @@ static int omapfb_find_best_mode(struct omap_dss_device *display,
 {
 	struct fb_monspecs *specs;
 	u8 *edid;
-	int r, i, best_xres, best_idx, len;
+	int r, i, best_idx, len;
 
 	if (!display->driver->read_edid)
 		return -ENODEV;
@@ -2274,7 +2274,6 @@ static int omapfb_find_best_mode(struct omap_dss_device *display,
 
 	fb_edid_to_monspecs(edid, specs);
 
-	best_xres = 0;
 	best_idx = -1;
 
 	for (i = 0; i < specs->modedb_len; ++i) {
@@ -2290,16 +2289,20 @@ static int omapfb_find_best_mode(struct omap_dss_device *display,
 		if (m->xres == 2880 || m->xres == 1440)
 			continue;
 
+		if (m->vmode & FB_VMODE_INTERLACED ||
+				m->vmode & FB_VMODE_DOUBLE)
+			continue;
+
 		fb_videomode_to_omap_timings(m, display, &t);
 
 		r = display->driver->check_timings(display, &t);
-		if (r == 0 && best_xres < m->xres) {
-			best_xres = m->xres;
+		if (r == 0) {
 			best_idx = i;
+			break;
 		}
 	}
 
-	if (best_xres == 0) {
+	if (best_idx == -1) {
 		r = -ENOENT;
 		goto err2;
 	}
-- 
1.7.10.4

[PATCH 03/12] OMAPDSS: fix DPI & DSI init order

[Tomi Valkeinen]

DPI may use DSI PLL, so it depends on DSI. However, currently DPI driver
is added first, which causes DPI initialization to fail when it tries to
get the DSI PLL.

This patch changes the init order to fix this.

A better solution would be to separate DSI PLL and DSI drivers. They
have dependencies, though, but we could still have DSI PLL as an
independent entity that we could initialize before any of the output
drivers.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/core.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 00aa026..d25374c 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -502,6 +502,9 @@ static int __init omap_dss_bus_register(void)
 
 /* INIT */
 static int (*dss_output_drv_reg_funcs[])(void) __initdata = {
+#ifdef CONFIG_OMAP2_DSS_DSI
+	dsi_init_platform_driver,
+#endif
 #ifdef CONFIG_OMAP2_DSS_DPI
 	dpi_init_platform_driver,
 #endif
@@ -514,15 +517,15 @@ static int (*dss_output_drv_reg_funcs[])(void) __initdata = {
 #ifdef CONFIG_OMAP2_DSS_VENC
 	venc_init_platform_driver,
 #endif
-#ifdef CONFIG_OMAP2_DSS_DSI
-	dsi_init_platform_driver,
-#endif
 #ifdef CONFIG_OMAP4_DSS_HDMI
 	hdmi_init_platform_driver,
 #endif
 };
 
 static void (*dss_output_drv_unreg_funcs[])(void) __exitdata = {
+#ifdef CONFIG_OMAP2_DSS_DSI
+	dsi_uninit_platform_driver,
+#endif
 #ifdef CONFIG_OMAP2_DSS_DPI
 	dpi_uninit_platform_driver,
 #endif
@@ -535,9 +538,6 @@ static void (*dss_output_drv_unreg_funcs[])(void) __exitdata = {
 #ifdef CONFIG_OMAP2_DSS_VENC
 	venc_uninit_platform_driver,
 #endif
-#ifdef CONFIG_OMAP2_DSS_DSI
-	dsi_uninit_platform_driver,
-#endif
 #ifdef CONFIG_OMAP4_DSS_HDMI
 	hdmi_uninit_platform_driver,
 #endif
-- 
1.7.10.4

[PATCH 03/12] OMAPDSS: fix DPI & DSI init order

[Tomi Valkeinen]

DPI may use DSI PLL, so it depends on DSI. However, currently DPI driver
is added first, which causes DPI initialization to fail when it tries to
get the DSI PLL.

This patch changes the init order to fix this.

A better solution would be to separate DSI PLL and DSI drivers. They
have dependencies, though, but we could still have DSI PLL as an
independent entity that we could initialize before any of the output
drivers.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/core.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 00aa026..d25374c 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -502,6 +502,9 @@ static int __init omap_dss_bus_register(void)
 
 /* INIT */
 static int (*dss_output_drv_reg_funcs[])(void) __initdata = {
+#ifdef CONFIG_OMAP2_DSS_DSI
+	dsi_init_platform_driver,
+#endif
 #ifdef CONFIG_OMAP2_DSS_DPI
 	dpi_init_platform_driver,
 #endif
@@ -514,15 +517,15 @@ static int (*dss_output_drv_reg_funcs[])(void) __initdata = {
 #ifdef CONFIG_OMAP2_DSS_VENC
 	venc_init_platform_driver,
 #endif
-#ifdef CONFIG_OMAP2_DSS_DSI
-	dsi_init_platform_driver,
-#endif
 #ifdef CONFIG_OMAP4_DSS_HDMI
 	hdmi_init_platform_driver,
 #endif
 };
 
 static void (*dss_output_drv_unreg_funcs[])(void) __exitdata = {
+#ifdef CONFIG_OMAP2_DSS_DSI
+	dsi_uninit_platform_driver,
+#endif
 #ifdef CONFIG_OMAP2_DSS_DPI
 	dpi_uninit_platform_driver,
 #endif
@@ -535,9 +538,6 @@ static void (*dss_output_drv_unreg_funcs[])(void) __exitdata = {
 #ifdef CONFIG_OMAP2_DSS_VENC
 	venc_uninit_platform_driver,
 #endif
-#ifdef CONFIG_OMAP2_DSS_DSI
-	dsi_uninit_platform_driver,
-#endif
 #ifdef CONFIG_OMAP4_DSS_HDMI
 	hdmi_uninit_platform_driver,
 #endif
-- 
1.7.10.4

[PATCH 04/12] OMAPDSS: fix DSI2 PLL clk names

[Tomi Valkeinen]

dss_generic_clk_source_names is missing the names for clocks from DSI2
PLL. Add them.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dss.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 37ee465..a85bd05 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -97,6 +97,8 @@ static const char * const dss_generic_clk_source_names[] = {
 	[OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC]	= "DSI_PLL_HSDIV_DISPC",
 	[OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DSI]	= "DSI_PLL_HSDIV_DSI",
 	[OMAP_DSS_CLK_SRC_FCK]			= "DSS_FCK",
+	[OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC]	= "DSI_PLL2_HSDIV_DISPC",
+	[OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DSI]	= "DSI_PLL2_HSDIV_DSI",
 };
 
 static inline void dss_write_reg(const struct dss_reg idx, u32 val)
-- 
1.7.10.4

[PATCH 04/12] OMAPDSS: fix DSI2 PLL clk names

[Tomi Valkeinen]

dss_generic_clk_source_names is missing the names for clocks from DSI2
PLL. Add them.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dss.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 37ee465..a85bd05 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -97,6 +97,8 @@ static const char * const dss_generic_clk_source_names[] = {
 	[OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC]	= "DSI_PLL_HSDIV_DISPC",
 	[OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DSI]	= "DSI_PLL_HSDIV_DSI",
 	[OMAP_DSS_CLK_SRC_FCK]			= "DSS_FCK",
+	[OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC]	= "DSI_PLL2_HSDIV_DISPC",
+	[OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DSI]	= "DSI_PLL2_HSDIV_DSI",
 };
 
 static inline void dss_write_reg(const struct dss_reg idx, u32 val)
-- 
1.7.10.4

[PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz

[Tomi Valkeinen]

The DSI PLL and HSDivider can be used to generate the pixel clock for
LCD overlay manager, which then goes to DPI output. On the DPI output
pin the voltage of the signal is shifted from the OMAP's internal
minimal voltage to 1.8V range. The shifting is not instant, and the
higher the clock frequency, the less time there is to shift the signal
to nominal voltage.

If the HSDivider's divider is greater than one and odd, the resulting
pixel clock does not have 50% duty cycle. For example, with a divider of
3, the duty cycle is 33%.

When combining high frequency (in the area of 140MHz+) and non-50% duty
cycle, it has been observed the the shifter does not have enough time to
shift the voltage enough, and this leads to bad signal which is rejected
by monitors.

As a workaround this patch makes the divider calculation skip all odd
dividers when the required pixel clock is over 100MHz.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dsi.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 7d0db2b..d0e35da 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1386,6 +1386,11 @@ retry:
 				cur.dsi_pll_hsdiv_dispc_clk  					cur.clkin4ddr / cur.regm_dispc;
 
+				if (cur.regm_dispc > 1 &&
+						cur.regm_dispc % 2 != 0 &&
+						req_pck >= 1000000)
+					continue;
+
 				/* this will narrow down the search a bit,
 				 * but still give pixclocks below what was
 				 * requested */
-- 
1.7.10.4

[PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz

[Tomi Valkeinen]

The DSI PLL and HSDivider can be used to generate the pixel clock for
LCD overlay manager, which then goes to DPI output. On the DPI output
pin the voltage of the signal is shifted from the OMAP's internal
minimal voltage to 1.8V range. The shifting is not instant, and the
higher the clock frequency, the less time there is to shift the signal
to nominal voltage.

If the HSDivider's divider is greater than one and odd, the resulting
pixel clock does not have 50% duty cycle. For example, with a divider of
3, the duty cycle is 33%.

When combining high frequency (in the area of 140MHz+) and non-50% duty
cycle, it has been observed the the shifter does not have enough time to
shift the voltage enough, and this leads to bad signal which is rejected
by monitors.

As a workaround this patch makes the divider calculation skip all odd
dividers when the required pixel clock is over 100MHz.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dsi.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 7d0db2b..d0e35da 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1386,6 +1386,11 @@ retry:
 				cur.dsi_pll_hsdiv_dispc_clk =
 					cur.clkin4ddr / cur.regm_dispc;
 
+				if (cur.regm_dispc > 1 &&
+						cur.regm_dispc % 2 != 0 &&
+						req_pck >= 1000000)
+					continue;
+
 				/* this will narrow down the search a bit,
 				 * but still give pixclocks below what was
 				 * requested */
-- 
1.7.10.4

Re: [PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz

[Archit Taneja]

On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
> The DSI PLL and HSDivider can be used to generate the pixel clock for
> LCD overlay manager, which then goes to DPI output. On the DPI output
> pin the voltage of the signal is shifted from the OMAP's internal
> minimal voltage to 1.8V range. The shifting is not instant, and the
> higher the clock frequency, the less time there is to shift the signal
> to nominal voltage.
>
> If the HSDivider's divider is greater than one and odd, the resulting
> pixel clock does not have 50% duty cycle. For example, with a divider of
> 3, the duty cycle is 33%.
>
> When combining high frequency (in the area of 140MHz+) and non-50% duty
> cycle, it has been observed the the shifter does not have enough time to
> shift the voltage enough, and this leads to bad signal which is rejected
> by monitors.

Is this something seen on OMAP3 also? I guess it must be since it's the 
same DSI IP.

>
> As a workaround this patch makes the divider calculation skip all odd
> dividers when the required pixel clock is over 100MHz.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/dss/dsi.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 7d0db2b..d0e35da 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -1386,6 +1386,11 @@ retry:
>   				cur.dsi_pll_hsdiv_dispc_clk =
>   					cur.clkin4ddr / cur.regm_dispc;
>
> +				if (cur.regm_dispc > 1 &&
> +						cur.regm_dispc % 2 != 0 &&
> +						req_pck >= 1000000)
> +					continue;
> +

Why do we do the req_pck check here? Can't we do it much earlier? We 
could bail out right in the beginning of dsi_pll_calc_clock_div_pck() if 
we see that req_pck is greater than 100 Mhz.

Also, we could maybe have a comment (or in the commit message) saying 
that we chose the 100 Mhz to make it a safe bet.

Archit

Re: [PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz

[Archit Taneja]

On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
> The DSI PLL and HSDivider can be used to generate the pixel clock for
> LCD overlay manager, which then goes to DPI output. On the DPI output
> pin the voltage of the signal is shifted from the OMAP's internal
> minimal voltage to 1.8V range. The shifting is not instant, and the
> higher the clock frequency, the less time there is to shift the signal
> to nominal voltage.
>
> If the HSDivider's divider is greater than one and odd, the resulting
> pixel clock does not have 50% duty cycle. For example, with a divider of
> 3, the duty cycle is 33%.
>
> When combining high frequency (in the area of 140MHz+) and non-50% duty
> cycle, it has been observed the the shifter does not have enough time to
> shift the voltage enough, and this leads to bad signal which is rejected
> by monitors.

Is this something seen on OMAP3 also? I guess it must be since it's the 
same DSI IP.

>
> As a workaround this patch makes the divider calculation skip all odd
> dividers when the required pixel clock is over 100MHz.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/dss/dsi.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 7d0db2b..d0e35da 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -1386,6 +1386,11 @@ retry:
>   				cur.dsi_pll_hsdiv_dispc_clk >   					cur.clkin4ddr / cur.regm_dispc;
>
> +				if (cur.regm_dispc > 1 &&
> +						cur.regm_dispc % 2 != 0 &&
> +						req_pck >= 1000000)
> +					continue;
> +

Why do we do the req_pck check here? Can't we do it much earlier? We 
could bail out right in the beginning of dsi_pll_calc_clock_div_pck() if 
we see that req_pck is greater than 100 Mhz.

Also, we could maybe have a comment (or in the commit message) saying 
that we chose the 100 Mhz to make it a safe bet.

Archit

Re: [PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

On 2012-10-31 08:45, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>> The DSI PLL and HSDivider can be used to generate the pixel clock for
>> LCD overlay manager, which then goes to DPI output. On the DPI output
>> pin the voltage of the signal is shifted from the OMAP's internal
>> minimal voltage to 1.8V range. The shifting is not instant, and the
>> higher the clock frequency, the less time there is to shift the signal
>> to nominal voltage.
>>
>> If the HSDivider's divider is greater than one and odd, the resulting
>> pixel clock does not have 50% duty cycle. For example, with a divider of
>> 3, the duty cycle is 33%.
>>
>> When combining high frequency (in the area of 140MHz+) and non-50% duty
>> cycle, it has been observed the the shifter does not have enough time to
>> shift the voltage enough, and this leads to bad signal which is rejected
>> by monitors.
> 
> Is this something seen on OMAP3 also? I guess it must be since it's the
> same DSI IP.

I have not seen this on OMAP3, but I'm 99% sure the same problem happens
there. But I guess there are many small things affecting the signal
quality, it could be that on omap3 beagleboard the resulting signal
voltage is still inside the standard range, even if odd dividers weaken it.

And I also think that we have the same problem with logic and pixel
clock dividers. My understanding is that all these simple dividers (i.e.
not a PLL or such) are made the same way, and, for example, divider of 3
is produced by keeping the output clock low for 2 cycles of the original
clock, and high for 1 cycle. Which leads to 33% duty cycle.

However, as the actual problem only materializes with high frequencies,
in practice we don't have a problem with pck or lck dividers. The reason
is that if we used pcd or lcd of 3, and the resulting pixel clock would
be > 100, the incoming DSS func clock would be around 300. Which is much
over the limit, and thus this scenario doesn't happen.

>> As a workaround this patch makes the divider calculation skip all odd
>> dividers when the required pixel clock is over 100MHz.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/video/omap2/dss/dsi.c |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/video/omap2/dss/dsi.c
>> b/drivers/video/omap2/dss/dsi.c
>> index 7d0db2b..d0e35da 100644
>> --- a/drivers/video/omap2/dss/dsi.c
>> +++ b/drivers/video/omap2/dss/dsi.c
>> @@ -1386,6 +1386,11 @@ retry:
>>                   cur.dsi_pll_hsdiv_dispc_clk =
>>                       cur.clkin4ddr / cur.regm_dispc;
>>
>> +                if (cur.regm_dispc > 1 &&
>> +                        cur.regm_dispc % 2 != 0 &&
>> +                        req_pck >= 1000000)
>> +                    continue;
>> +
> 
> Why do we do the req_pck check here? Can't we do it much earlier? We
> could bail out right in the beginning of dsi_pll_calc_clock_div_pck() if
> we see that req_pck is greater than 100 Mhz.

I think you misunderstood the patch. We don't skip or fail calculations
for pck > 100. What we do is we skip odd dividers if pck > 100.

> Also, we could maybe have a comment (or in the commit message) saying
> that we chose the 100 Mhz to make it a safe bet.

Hmm, yes, I should point out that 100MHz is just a guesstimate.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

On 2012-10-31 08:45, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>> The DSI PLL and HSDivider can be used to generate the pixel clock for
>> LCD overlay manager, which then goes to DPI output. On the DPI output
>> pin the voltage of the signal is shifted from the OMAP's internal
>> minimal voltage to 1.8V range. The shifting is not instant, and the
>> higher the clock frequency, the less time there is to shift the signal
>> to nominal voltage.
>>
>> If the HSDivider's divider is greater than one and odd, the resulting
>> pixel clock does not have 50% duty cycle. For example, with a divider of
>> 3, the duty cycle is 33%.
>>
>> When combining high frequency (in the area of 140MHz+) and non-50% duty
>> cycle, it has been observed the the shifter does not have enough time to
>> shift the voltage enough, and this leads to bad signal which is rejected
>> by monitors.
> 
> Is this something seen on OMAP3 also? I guess it must be since it's the
> same DSI IP.

I have not seen this on OMAP3, but I'm 99% sure the same problem happens
there. But I guess there are many small things affecting the signal
quality, it could be that on omap3 beagleboard the resulting signal
voltage is still inside the standard range, even if odd dividers weaken it.

And I also think that we have the same problem with logic and pixel
clock dividers. My understanding is that all these simple dividers (i.e.
not a PLL or such) are made the same way, and, for example, divider of 3
is produced by keeping the output clock low for 2 cycles of the original
clock, and high for 1 cycle. Which leads to 33% duty cycle.

However, as the actual problem only materializes with high frequencies,
in practice we don't have a problem with pck or lck dividers. The reason
is that if we used pcd or lcd of 3, and the resulting pixel clock would
be > 100, the incoming DSS func clock would be around 300. Which is much
over the limit, and thus this scenario doesn't happen.

>> As a workaround this patch makes the divider calculation skip all odd
>> dividers when the required pixel clock is over 100MHz.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/video/omap2/dss/dsi.c |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/video/omap2/dss/dsi.c
>> b/drivers/video/omap2/dss/dsi.c
>> index 7d0db2b..d0e35da 100644
>> --- a/drivers/video/omap2/dss/dsi.c
>> +++ b/drivers/video/omap2/dss/dsi.c
>> @@ -1386,6 +1386,11 @@ retry:
>>                   cur.dsi_pll_hsdiv_dispc_clk =
>>                       cur.clkin4ddr / cur.regm_dispc;
>>
>> +                if (cur.regm_dispc > 1 &&
>> +                        cur.regm_dispc % 2 != 0 &&
>> +                        req_pck >= 1000000)
>> +                    continue;
>> +
> 
> Why do we do the req_pck check here? Can't we do it much earlier? We
> could bail out right in the beginning of dsi_pll_calc_clock_div_pck() if
> we see that req_pck is greater than 100 Mhz.

I think you misunderstood the patch. We don't skip or fail calculations
for pck > 100. What we do is we skip odd dividers if pck > 100.

> Also, we could maybe have a comment (or in the commit message) saying
> that we chose the 100 Mhz to make it a safe bet.

Hmm, yes, I should point out that 100MHz is just a guesstimate.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz

[Archit Taneja]

On Wednesday 31 October 2012 12:56 PM, Tomi Valkeinen wrote:
> On 2012-10-31 08:45, Archit Taneja wrote:
>> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>>> The DSI PLL and HSDivider can be used to generate the pixel clock for
>>> LCD overlay manager, which then goes to DPI output. On the DPI output
>>> pin the voltage of the signal is shifted from the OMAP's internal
>>> minimal voltage to 1.8V range. The shifting is not instant, and the
>>> higher the clock frequency, the less time there is to shift the signal
>>> to nominal voltage.
>>>
>>> If the HSDivider's divider is greater than one and odd, the resulting
>>> pixel clock does not have 50% duty cycle. For example, with a divider of
>>> 3, the duty cycle is 33%.
>>>
>>> When combining high frequency (in the area of 140MHz+) and non-50% duty
>>> cycle, it has been observed the the shifter does not have enough time to
>>> shift the voltage enough, and this leads to bad signal which is rejected
>>> by monitors.
>>
>> Is this something seen on OMAP3 also? I guess it must be since it's the
>> same DSI IP.
>
> I have not seen this on OMAP3, but I'm 99% sure the same problem happens
> there. But I guess there are many small things affecting the signal
> quality, it could be that on omap3 beagleboard the resulting signal
> voltage is still inside the standard range, even if odd dividers weaken it.
>
> And I also think that we have the same problem with logic and pixel
> clock dividers. My understanding is that all these simple dividers (i.e.
> not a PLL or such) are made the same way, and, for example, divider of 3
> is produced by keeping the output clock low for 2 cycles of the original
> clock, and high for 1 cycle. Which leads to 33% duty cycle.
>
> However, as the actual problem only materializes with high frequencies,
> in practice we don't have a problem with pck or lck dividers. The reason
> is that if we used pcd or lcd of 3, and the resulting pixel clock would
> be > 100, the incoming DSS func clock would be around 300. Which is much
> over the limit, and thus this scenario doesn't happen.
>
>>> As a workaround this patch makes the divider calculation skip all odd
>>> dividers when the required pixel clock is over 100MHz.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>    drivers/video/omap2/dss/dsi.c |    5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/video/omap2/dss/dsi.c
>>> b/drivers/video/omap2/dss/dsi.c
>>> index 7d0db2b..d0e35da 100644
>>> --- a/drivers/video/omap2/dss/dsi.c
>>> +++ b/drivers/video/omap2/dss/dsi.c
>>> @@ -1386,6 +1386,11 @@ retry:
>>>                    cur.dsi_pll_hsdiv_dispc_clk =
>>>                        cur.clkin4ddr / cur.regm_dispc;
>>>
>>> +                if (cur.regm_dispc > 1 &&
>>> +                        cur.regm_dispc % 2 != 0 &&
>>> +                        req_pck >= 1000000)
>>> +                    continue;
>>> +
>>
>> Why do we do the req_pck check here? Can't we do it much earlier? We
>> could bail out right in the beginning of dsi_pll_calc_clock_div_pck() if
>> we see that req_pck is greater than 100 Mhz.
>
> I think you misunderstood the patch. We don't skip or fail calculations
> for pck > 100. What we do is we skip odd dividers if pck > 100.

Ah okay. I missed that point.

Archit

Re: [PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz

[Archit Taneja]

On Wednesday 31 October 2012 12:56 PM, Tomi Valkeinen wrote:
> On 2012-10-31 08:45, Archit Taneja wrote:
>> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>>> The DSI PLL and HSDivider can be used to generate the pixel clock for
>>> LCD overlay manager, which then goes to DPI output. On the DPI output
>>> pin the voltage of the signal is shifted from the OMAP's internal
>>> minimal voltage to 1.8V range. The shifting is not instant, and the
>>> higher the clock frequency, the less time there is to shift the signal
>>> to nominal voltage.
>>>
>>> If the HSDivider's divider is greater than one and odd, the resulting
>>> pixel clock does not have 50% duty cycle. For example, with a divider of
>>> 3, the duty cycle is 33%.
>>>
>>> When combining high frequency (in the area of 140MHz+) and non-50% duty
>>> cycle, it has been observed the the shifter does not have enough time to
>>> shift the voltage enough, and this leads to bad signal which is rejected
>>> by monitors.
>>
>> Is this something seen on OMAP3 also? I guess it must be since it's the
>> same DSI IP.
>
> I have not seen this on OMAP3, but I'm 99% sure the same problem happens
> there. But I guess there are many small things affecting the signal
> quality, it could be that on omap3 beagleboard the resulting signal
> voltage is still inside the standard range, even if odd dividers weaken it.
>
> And I also think that we have the same problem with logic and pixel
> clock dividers. My understanding is that all these simple dividers (i.e.
> not a PLL or such) are made the same way, and, for example, divider of 3
> is produced by keeping the output clock low for 2 cycles of the original
> clock, and high for 1 cycle. Which leads to 33% duty cycle.
>
> However, as the actual problem only materializes with high frequencies,
> in practice we don't have a problem with pck or lck dividers. The reason
> is that if we used pcd or lcd of 3, and the resulting pixel clock would
> be > 100, the incoming DSS func clock would be around 300. Which is much
> over the limit, and thus this scenario doesn't happen.
>
>>> As a workaround this patch makes the divider calculation skip all odd
>>> dividers when the required pixel clock is over 100MHz.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>    drivers/video/omap2/dss/dsi.c |    5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/video/omap2/dss/dsi.c
>>> b/drivers/video/omap2/dss/dsi.c
>>> index 7d0db2b..d0e35da 100644
>>> --- a/drivers/video/omap2/dss/dsi.c
>>> +++ b/drivers/video/omap2/dss/dsi.c
>>> @@ -1386,6 +1386,11 @@ retry:
>>>                    cur.dsi_pll_hsdiv_dispc_clk >>>                        cur.clkin4ddr / cur.regm_dispc;
>>>
>>> +                if (cur.regm_dispc > 1 &&
>>> +                        cur.regm_dispc % 2 != 0 &&
>>> +                        req_pck >= 1000000)
>>> +                    continue;
>>> +
>>
>> Why do we do the req_pck check here? Can't we do it much earlier? We
>> could bail out right in the beginning of dsi_pll_calc_clock_div_pck() if
>> we see that req_pck is greater than 100 Mhz.
>
> I think you misunderstood the patch. We don't skip or fail calculations
> for pck > 100. What we do is we skip odd dividers if pck > 100.

Ah okay. I missed that point.

Archit

[PATCH 06/12] OMAPDSS: DSI: workaround for HSDiv problem

[Tomi Valkeinen]

It looks like on many OMAP versions powers for both HSClk and HSDiv to
be enabled to have a functional HSDiv.

This patch fixes the issue by forcing both powers on.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dsi.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index d0e35da..26d1a78 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1741,6 +1741,12 @@ int dsi_pll_init(struct platform_device *dsidev, bool enable_hsclk,
 
 	DSSDBG("PLL init\n");
 
+	/*
+	 * It seems that on many OMAPs we need to enable both to have a
+	 * functional HSDivider.
+	 */
+	enable_hsclk = enable_hsdiv = true;
+
 	if (dsi->vdds_dsi_reg = NULL) {
 		struct regulator *vdds_dsi;
 
-- 
1.7.10.4

[PATCH 06/12] OMAPDSS: DSI: workaround for HSDiv problem

[Tomi Valkeinen]

It looks like on many OMAP versions powers for both HSClk and HSDiv to
be enabled to have a functional HSDiv.

This patch fixes the issue by forcing both powers on.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dsi.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index d0e35da..26d1a78 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1741,6 +1741,12 @@ int dsi_pll_init(struct platform_device *dsidev, bool enable_hsclk,
 
 	DSSDBG("PLL init\n");
 
+	/*
+	 * It seems that on many OMAPs we need to enable both to have a
+	 * functional HSDivider.
+	 */
+	enable_hsclk = enable_hsdiv = true;
+
 	if (dsi->vdds_dsi_reg == NULL) {
 		struct regulator *vdds_dsi;
 
-- 
1.7.10.4

[PATCH 07/12] OMAPDSS: add dss_calc_clock_rates() back

[Tomi Valkeinen]

dss_calc_clock_rates() was removed earlier as it was not used, but it is
needed for DSI PLL calculations, so this patch adds it back.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dss.c |   23 +++++++++++++++++++++++
 drivers/video/omap2/dss/dss.h |    1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index a85bd05..5affa86 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -432,6 +432,29 @@ enum omap_dss_clk_source dss_get_lcd_clk_source(enum omap_channel channel)
 	}
 }
 
+/* calculate clock rates using dividers in cinfo */
+int dss_calc_clock_rates(struct dss_clock_info *cinfo)
+{
+	if (dss.dpll4_m4_ck) {
+		unsigned long prate;
+
+		if (cinfo->fck_div > dss.feat->fck_div_max ||
+				cinfo->fck_div = 0)
+			return -EINVAL;
+
+		prate = clk_get_rate(clk_get_parent(dss.dpll4_m4_ck));
+
+		cinfo->fck = prate / cinfo->fck_div *
+			dss.feat->dss_fck_multiplier;
+	} else {
+		if (cinfo->fck_div != 0)
+			return -EINVAL;
+		cinfo->fck = clk_get_rate(dss.dss_clk);
+	}
+
+	return 0;
+}
+
 int dss_set_clock_div(struct dss_clock_info *cinfo)
 {
 	if (dss.dpll4_m4_ck) {
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index fb89165..ae4e618 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -297,6 +297,7 @@ void dss_set_venc_output(enum omap_dss_venc_type type);
 void dss_set_dac_pwrdn_bgz(bool enable);
 
 unsigned long dss_get_dpll4_rate(void);
+int dss_calc_clock_rates(struct dss_clock_info *cinfo);
 int dss_set_clock_div(struct dss_clock_info *cinfo);
 int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
 		struct dispc_clock_info *dispc_cinfo);
-- 
1.7.10.4

[PATCH 07/12] OMAPDSS: add dss_calc_clock_rates() back

[Tomi Valkeinen]

dss_calc_clock_rates() was removed earlier as it was not used, but it is
needed for DSI PLL calculations, so this patch adds it back.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dss.c |   23 +++++++++++++++++++++++
 drivers/video/omap2/dss/dss.h |    1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index a85bd05..5affa86 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -432,6 +432,29 @@ enum omap_dss_clk_source dss_get_lcd_clk_source(enum omap_channel channel)
 	}
 }
 
+/* calculate clock rates using dividers in cinfo */
+int dss_calc_clock_rates(struct dss_clock_info *cinfo)
+{
+	if (dss.dpll4_m4_ck) {
+		unsigned long prate;
+
+		if (cinfo->fck_div > dss.feat->fck_div_max ||
+				cinfo->fck_div == 0)
+			return -EINVAL;
+
+		prate = clk_get_rate(clk_get_parent(dss.dpll4_m4_ck));
+
+		cinfo->fck = prate / cinfo->fck_div *
+			dss.feat->dss_fck_multiplier;
+	} else {
+		if (cinfo->fck_div != 0)
+			return -EINVAL;
+		cinfo->fck = clk_get_rate(dss.dss_clk);
+	}
+
+	return 0;
+}
+
 int dss_set_clock_div(struct dss_clock_info *cinfo)
 {
 	if (dss.dpll4_m4_ck) {
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index fb89165..ae4e618 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -297,6 +297,7 @@ void dss_set_venc_output(enum omap_dss_venc_type type);
 void dss_set_dac_pwrdn_bgz(bool enable);
 
 unsigned long dss_get_dpll4_rate(void);
+int dss_calc_clock_rates(struct dss_clock_info *cinfo);
 int dss_set_clock_div(struct dss_clock_info *cinfo);
 int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
 		struct dispc_clock_info *dispc_cinfo);
-- 
1.7.10.4

[PATCH 08/12] OMAPDSS: setup default dss fck

[Tomi Valkeinen]

We don't currently set the dss fck when starting up. This is not a
problem, as we setup the fck later when configuring the pixel clocks. Or
this is how it was for omap2, for the rest of the omaps this may not be
so.

For DSI, HDMI and also for DPI when using DSI PLL, we don't need to
change the dss fck, and thus it may be left unconfigured. Usually the
dss fck is already setup fine by default, but we can't trust this.

This patch sets the dss fck to maximum at probe time.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dss.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 5affa86..034cc1a 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -485,6 +485,36 @@ unsigned long dss_get_dpll4_rate(void)
 		return 0;
 }
 
+static int dss_setup_default_clock(void)
+{
+	unsigned long max_dss_fck, prate;
+	unsigned fck_div;
+	struct dss_clock_info dss_cinfo = { 0 };
+	int r;
+
+	if (dss.dpll4_m4_ck = NULL)
+		return 0;
+
+	max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
+
+	prate = dss_get_dpll4_rate();
+
+	fck_div = DIV_ROUND_UP(prate * dss.feat->dss_fck_multiplier,
+			max_dss_fck);
+
+	dss_cinfo.fck_div = fck_div;
+
+	r = dss_calc_clock_rates(&dss_cinfo);
+	if (r)
+		return r;
+
+	r = dss_set_clock_div(&dss_cinfo);
+	if (r)
+		return r;
+
+	return 0;
+}
+
 int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
 		struct dispc_clock_info *dispc_cinfo)
 {
@@ -913,6 +943,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
 	dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
 	dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
 
+	r = dss_setup_default_clock();
+	if (r)
+		goto err_setup_clocks;
+
 	rev = dss_read_reg(DSS_REVISION);
 	printk(KERN_INFO "OMAP DSS rev %d.%d\n",
 			FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
@@ -923,6 +957,8 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_setup_clocks:
+	dss_runtime_put();
 err_runtime_get:
 	pm_runtime_disable(&pdev->dev);
 	dss_put_clocks();
-- 
1.7.10.4

[PATCH 08/12] OMAPDSS: setup default dss fck

[Tomi Valkeinen]

We don't currently set the dss fck when starting up. This is not a
problem, as we setup the fck later when configuring the pixel clocks. Or
this is how it was for omap2, for the rest of the omaps this may not be
so.

For DSI, HDMI and also for DPI when using DSI PLL, we don't need to
change the dss fck, and thus it may be left unconfigured. Usually the
dss fck is already setup fine by default, but we can't trust this.

This patch sets the dss fck to maximum at probe time.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dss.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 5affa86..034cc1a 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -485,6 +485,36 @@ unsigned long dss_get_dpll4_rate(void)
 		return 0;
 }
 
+static int dss_setup_default_clock(void)
+{
+	unsigned long max_dss_fck, prate;
+	unsigned fck_div;
+	struct dss_clock_info dss_cinfo = { 0 };
+	int r;
+
+	if (dss.dpll4_m4_ck == NULL)
+		return 0;
+
+	max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
+
+	prate = dss_get_dpll4_rate();
+
+	fck_div = DIV_ROUND_UP(prate * dss.feat->dss_fck_multiplier,
+			max_dss_fck);
+
+	dss_cinfo.fck_div = fck_div;
+
+	r = dss_calc_clock_rates(&dss_cinfo);
+	if (r)
+		return r;
+
+	r = dss_set_clock_div(&dss_cinfo);
+	if (r)
+		return r;
+
+	return 0;
+}
+
 int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
 		struct dispc_clock_info *dispc_cinfo)
 {
@@ -913,6 +943,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
 	dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
 	dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
 
+	r = dss_setup_default_clock();
+	if (r)
+		goto err_setup_clocks;
+
 	rev = dss_read_reg(DSS_REVISION);
 	printk(KERN_INFO "OMAP DSS rev %d.%d\n",
 			FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
@@ -923,6 +957,8 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_setup_clocks:
+	dss_runtime_put();
 err_runtime_get:
 	pm_runtime_disable(&pdev->dev);
 	dss_put_clocks();
-- 
1.7.10.4

Re: [PATCH 08/12] OMAPDSS: setup default dss fck

[Archit Taneja]

On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
> We don't currently set the dss fck when starting up. This is not a
> problem, as we setup the fck later when configuring the pixel clocks. Or
> this is how it was for omap2, for the rest of the omaps this may not be
> so.
>
> For DSI, HDMI and also for DPI when using DSI PLL, we don't need to
> change the dss fck, and thus it may be left unconfigured. Usually the
> dss fck is already setup fine by default, but we can't trust this.
>
> This patch sets the dss fck to maximum at probe time.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/dss/dss.c |   36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
>
> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> index 5affa86..034cc1a 100644
> --- a/drivers/video/omap2/dss/dss.c
> +++ b/drivers/video/omap2/dss/dss.c
> @@ -485,6 +485,36 @@ unsigned long dss_get_dpll4_rate(void)
>   		return 0;
>   }
>
> +static int dss_setup_default_clock(void)
> +{
> +	unsigned long max_dss_fck, prate;
> +	unsigned fck_div;
> +	struct dss_clock_info dss_cinfo = { 0 };
> +	int r;
> +
> +	if (dss.dpll4_m4_ck == NULL)
> +		return 0;
> +
> +	max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
> +
> +	prate = dss_get_dpll4_rate();

Not related to this patch, but maybe we could change the 
dss_get_dpll4_rate() name and dss.dpll4_m4_clk to something better. 
Maybe something like dss_fck_parent?

> +
> +	fck_div = DIV_ROUND_UP(prate * dss.feat->dss_fck_multiplier,
> +			max_dss_fck);
> +
> +	dss_cinfo.fck_div = fck_div;
> +
> +	r = dss_calc_clock_rates(&dss_cinfo);
> +	if (r)
> +		return r;
> +
> +	r = dss_set_clock_div(&dss_cinfo);
> +	if (r)
> +		return r;
> +
> +	return 0;
> +}
> +
>   int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>   		struct dispc_clock_info *dispc_cinfo)
>   {
> @@ -913,6 +943,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
>   	dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
>   	dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
>
> +	r = dss_setup_default_clock();
> +	if (r)
> +		goto err_setup_clocks;

Maybe it's safer to call this before we do a dss_runtime_get(). On 
OMAP4, DSS_FCLK is needed to access registers also. Changing it's rate 
might not be liked by the DSS HW. Also, it seems more logical to call it 
after dss_get_clocks() in omap_dsshw_probe(), then we sort of group the 
clock related stuff together.

Archit

> +
>   	rev = dss_read_reg(DSS_REVISION);
>   	printk(KERN_INFO "OMAP DSS rev %d.%d\n",
>   			FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
> @@ -923,6 +957,8 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
>
>   	return 0;
>
> +err_setup_clocks:
> +	dss_runtime_put();
>   err_runtime_get:
>   	pm_runtime_disable(&pdev->dev);
>   	dss_put_clocks();
>

Re: [PATCH 08/12] OMAPDSS: setup default dss fck

[Archit Taneja]

On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
> We don't currently set the dss fck when starting up. This is not a
> problem, as we setup the fck later when configuring the pixel clocks. Or
> this is how it was for omap2, for the rest of the omaps this may not be
> so.
>
> For DSI, HDMI and also for DPI when using DSI PLL, we don't need to
> change the dss fck, and thus it may be left unconfigured. Usually the
> dss fck is already setup fine by default, but we can't trust this.
>
> This patch sets the dss fck to maximum at probe time.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/dss/dss.c |   36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
>
> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> index 5affa86..034cc1a 100644
> --- a/drivers/video/omap2/dss/dss.c
> +++ b/drivers/video/omap2/dss/dss.c
> @@ -485,6 +485,36 @@ unsigned long dss_get_dpll4_rate(void)
>   		return 0;
>   }
>
> +static int dss_setup_default_clock(void)
> +{
> +	unsigned long max_dss_fck, prate;
> +	unsigned fck_div;
> +	struct dss_clock_info dss_cinfo = { 0 };
> +	int r;
> +
> +	if (dss.dpll4_m4_ck = NULL)
> +		return 0;
> +
> +	max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
> +
> +	prate = dss_get_dpll4_rate();

Not related to this patch, but maybe we could change the 
dss_get_dpll4_rate() name and dss.dpll4_m4_clk to something better. 
Maybe something like dss_fck_parent?

> +
> +	fck_div = DIV_ROUND_UP(prate * dss.feat->dss_fck_multiplier,
> +			max_dss_fck);
> +
> +	dss_cinfo.fck_div = fck_div;
> +
> +	r = dss_calc_clock_rates(&dss_cinfo);
> +	if (r)
> +		return r;
> +
> +	r = dss_set_clock_div(&dss_cinfo);
> +	if (r)
> +		return r;
> +
> +	return 0;
> +}
> +
>   int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>   		struct dispc_clock_info *dispc_cinfo)
>   {
> @@ -913,6 +943,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
>   	dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
>   	dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
>
> +	r = dss_setup_default_clock();
> +	if (r)
> +		goto err_setup_clocks;

Maybe it's safer to call this before we do a dss_runtime_get(). On 
OMAP4, DSS_FCLK is needed to access registers also. Changing it's rate 
might not be liked by the DSS HW. Also, it seems more logical to call it 
after dss_get_clocks() in omap_dsshw_probe(), then we sort of group the 
clock related stuff together.

Archit

> +
>   	rev = dss_read_reg(DSS_REVISION);
>   	printk(KERN_INFO "OMAP DSS rev %d.%d\n",
>   			FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
> @@ -923,6 +957,8 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
>
>   	return 0;
>
> +err_setup_clocks:
> +	dss_runtime_put();
>   err_runtime_get:
>   	pm_runtime_disable(&pdev->dev);
>   	dss_put_clocks();
>

Re: [PATCH 08/12] OMAPDSS: setup default dss fck

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

On 2012-10-31 08:31, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>> We don't currently set the dss fck when starting up. This is not a
>> problem, as we setup the fck later when configuring the pixel clocks. Or
>> this is how it was for omap2, for the rest of the omaps this may not be
>> so.
>>
>> For DSI, HDMI and also for DPI when using DSI PLL, we don't need to
>> change the dss fck, and thus it may be left unconfigured. Usually the
>> dss fck is already setup fine by default, but we can't trust this.
>>
>> This patch sets the dss fck to maximum at probe time.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/video/omap2/dss/dss.c |   36
>> ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/video/omap2/dss/dss.c
>> b/drivers/video/omap2/dss/dss.c
>> index 5affa86..034cc1a 100644
>> --- a/drivers/video/omap2/dss/dss.c
>> +++ b/drivers/video/omap2/dss/dss.c
>> @@ -485,6 +485,36 @@ unsigned long dss_get_dpll4_rate(void)
>>           return 0;
>>   }
>>
>> +static int dss_setup_default_clock(void)
>> +{
>> +    unsigned long max_dss_fck, prate;
>> +    unsigned fck_div;
>> +    struct dss_clock_info dss_cinfo = { 0 };
>> +    int r;
>> +
>> +    if (dss.dpll4_m4_ck == NULL)
>> +        return 0;
>> +
>> +    max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
>> +
>> +    prate = dss_get_dpll4_rate();
> 
> Not related to this patch, but maybe we could change the
> dss_get_dpll4_rate() name and dss.dpll4_m4_clk to something better.
> Maybe something like dss_fck_parent?

I agree. Or, even better, we should fix the omap clk data/code so that
we could set the dss fck, without this trickery. But I have no idea if
that's easy or difficult.

>> +
>> +    fck_div = DIV_ROUND_UP(prate * dss.feat->dss_fck_multiplier,
>> +            max_dss_fck);
>> +
>> +    dss_cinfo.fck_div = fck_div;
>> +
>> +    r = dss_calc_clock_rates(&dss_cinfo);
>> +    if (r)
>> +        return r;
>> +
>> +    r = dss_set_clock_div(&dss_cinfo);
>> +    if (r)
>> +        return r;
>> +
>> +    return 0;
>> +}
>> +
>>   int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info
>> *dss_cinfo,
>>           struct dispc_clock_info *dispc_cinfo)
>>   {
>> @@ -913,6 +943,10 @@ static int __init omap_dsshw_probe(struct
>> platform_device *pdev)
>>       dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
>>       dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
>>
>> +    r = dss_setup_default_clock();
>> +    if (r)
>> +        goto err_setup_clocks;
> 
> Maybe it's safer to call this before we do a dss_runtime_get(). On
> OMAP4, DSS_FCLK is needed to access registers also. Changing it's rate
> might not be liked by the DSS HW. Also, it seems more logical to call it
> after dss_get_clocks() in omap_dsshw_probe(), then we sort of group the
> clock related stuff together.

Yes, good point. I don't think DSS has any problems with the clock
changing, as long as it's not outputting an image (but I'm not sure). In
any case, it makes sense to setup the clocks dss_get_clocks as you said.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 08/12] OMAPDSS: setup default dss fck

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

On 2012-10-31 08:31, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>> We don't currently set the dss fck when starting up. This is not a
>> problem, as we setup the fck later when configuring the pixel clocks. Or
>> this is how it was for omap2, for the rest of the omaps this may not be
>> so.
>>
>> For DSI, HDMI and also for DPI when using DSI PLL, we don't need to
>> change the dss fck, and thus it may be left unconfigured. Usually the
>> dss fck is already setup fine by default, but we can't trust this.
>>
>> This patch sets the dss fck to maximum at probe time.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/video/omap2/dss/dss.c |   36
>> ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/video/omap2/dss/dss.c
>> b/drivers/video/omap2/dss/dss.c
>> index 5affa86..034cc1a 100644
>> --- a/drivers/video/omap2/dss/dss.c
>> +++ b/drivers/video/omap2/dss/dss.c
>> @@ -485,6 +485,36 @@ unsigned long dss_get_dpll4_rate(void)
>>           return 0;
>>   }
>>
>> +static int dss_setup_default_clock(void)
>> +{
>> +    unsigned long max_dss_fck, prate;
>> +    unsigned fck_div;
>> +    struct dss_clock_info dss_cinfo = { 0 };
>> +    int r;
>> +
>> +    if (dss.dpll4_m4_ck == NULL)
>> +        return 0;
>> +
>> +    max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
>> +
>> +    prate = dss_get_dpll4_rate();
> 
> Not related to this patch, but maybe we could change the
> dss_get_dpll4_rate() name and dss.dpll4_m4_clk to something better.
> Maybe something like dss_fck_parent?

I agree. Or, even better, we should fix the omap clk data/code so that
we could set the dss fck, without this trickery. But I have no idea if
that's easy or difficult.

>> +
>> +    fck_div = DIV_ROUND_UP(prate * dss.feat->dss_fck_multiplier,
>> +            max_dss_fck);
>> +
>> +    dss_cinfo.fck_div = fck_div;
>> +
>> +    r = dss_calc_clock_rates(&dss_cinfo);
>> +    if (r)
>> +        return r;
>> +
>> +    r = dss_set_clock_div(&dss_cinfo);
>> +    if (r)
>> +        return r;
>> +
>> +    return 0;
>> +}
>> +
>>   int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info
>> *dss_cinfo,
>>           struct dispc_clock_info *dispc_cinfo)
>>   {
>> @@ -913,6 +943,10 @@ static int __init omap_dsshw_probe(struct
>> platform_device *pdev)
>>       dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
>>       dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
>>
>> +    r = dss_setup_default_clock();
>> +    if (r)
>> +        goto err_setup_clocks;
> 
> Maybe it's safer to call this before we do a dss_runtime_get(). On
> OMAP4, DSS_FCLK is needed to access registers also. Changing it's rate
> might not be liked by the DSS HW. Also, it seems more logical to call it
> after dss_get_clocks() in omap_dsshw_probe(), then we sort of group the
> clock related stuff together.

Yes, good point. I don't think DSS has any problems with the clock
changing, as long as it's not outputting an image (but I'm not sure). In
any case, it makes sense to setup the clocks dss_get_clocks as you said.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[PATCH 09/12] OMAPDSS: hide dss_select_dispc_clk_source()

[Tomi Valkeinen]

dss.c currently exposes functions to configure the dispc source clock
and lcd source clock. There are configured separately from the output
drivers.

However, there is no safe way for the output drivers to handle dispc
clock, as it's shared between the outputs. Thus, if, say, the DSI driver
sets up DSI PLL and configures both the dispc and lcd clock sources to
that DSI PLL, the resulting dispc clock could be too low for, say, HDMI.

Thus the output drivers should really only be concerned about the lcd
clock, which is what the output drivers actually use. There's lot to do
to clean up the dss clock handling, but this patch takes one step
forward and removes the use of dss_select_dispc_clk_source() from the
output drivers.

After this patch, the output drivers only configure the lcd source
clock. On omap4+ the dispc src clock is never changed from the default
PRCM source. On omap3, where the dispc and lcd clocks are actually the
same, setting the lcd clock source sets the dispc clock source.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dpi.c  |    6 ++++--
 drivers/video/omap2/dss/dsi.c  |    3 ---
 drivers/video/omap2/dss/dss.c  |    6 ++++--
 drivers/video/omap2/dss/dss.h  |    1 -
 drivers/video/omap2/dss/hdmi.c |    8 --------
 5 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 56748cf..9df62cf 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -77,6 +77,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
 		unsigned long pck_req, unsigned long *fck, int *lck_div,
 		int *pck_div)
 {
+	struct omap_overlay_manager *mgr = dssdev->output->manager;
 	struct dsi_clock_info dsi_cinfo;
 	struct dispc_clock_info dispc_cinfo;
 	int r;
@@ -90,7 +91,8 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
 	if (r)
 		return r;
 
-	dss_select_dispc_clk_source(dssdev->clocks.dispc.dispc_fclk_src);
+	dss_select_lcd_clk_source(mgr->id,
+			dssdev->clocks.dispc.channel.lcd_clk_src);
 
 	dpi.mgr_config.clock_info = dispc_cinfo;
 
@@ -272,7 +274,7 @@ void omapdss_dpi_display_disable(struct omap_dss_device *dssdev)
 	dss_mgr_disable(mgr);
 
 	if (dpi_use_dsi_pll(dssdev)) {
-		dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
+		dss_select_lcd_clk_source(mgr->id, OMAP_DSS_CLK_SRC_FCK);
 		dsi_pll_uninit(dpi.dsidev, true);
 		dsi_runtime_put(dpi.dsidev);
 	}
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 26d1a78..70bd7a5 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4720,7 +4720,6 @@ static int dsi_display_init_dsi(struct omap_dss_device *dssdev)
 	if (r)
 		goto err1;
 
-	dss_select_dispc_clk_source(dssdev->clocks.dispc.dispc_fclk_src);
 	dss_select_dsi_clk_source(dsi->module_id, dssdev->clocks.dsi.dsi_fclk_src);
 	dss_select_lcd_clk_source(mgr->id,
 			dssdev->clocks.dispc.channel.lcd_clk_src);
@@ -4755,7 +4754,6 @@ static int dsi_display_init_dsi(struct omap_dss_device *dssdev)
 err3:
 	dsi_cio_uninit(dsidev);
 err2:
-	dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
 	dss_select_dsi_clk_source(dsi->module_id, OMAP_DSS_CLK_SRC_FCK);
 	dss_select_lcd_clk_source(mgr->id, OMAP_DSS_CLK_SRC_FCK);
 
@@ -4782,7 +4780,6 @@ static void dsi_display_uninit_dsi(struct omap_dss_device *dssdev,
 	dsi_vc_enable(dsidev, 2, 0);
 	dsi_vc_enable(dsidev, 3, 0);
 
-	dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
 	dss_select_dsi_clk_source(dsi->module_id, OMAP_DSS_CLK_SRC_FCK);
 	dss_select_lcd_clk_source(mgr->id, OMAP_DSS_CLK_SRC_FCK);
 	dsi_cio_uninit(dsidev);
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 034cc1a..086ed56 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -304,7 +304,7 @@ static void dss_dump_regs(struct seq_file *s)
 #undef DUMPREG
 }
 
-void dss_select_dispc_clk_source(enum omap_dss_clk_source clk_src)
+static void dss_select_dispc_clk_source(enum omap_dss_clk_source clk_src)
 {
 	struct platform_device *dsidev;
 	int b;
@@ -375,8 +375,10 @@ void dss_select_lcd_clk_source(enum omap_channel channel,
 	struct platform_device *dsidev;
 	int b, ix, pos;
 
-	if (!dss_has_feature(FEAT_LCD_CLK_SRC))
+	if (!dss_has_feature(FEAT_LCD_CLK_SRC)) {
+		dss_select_dispc_clk_source(clk_src);
 		return;
+	}
 
 	switch (clk_src) {
 	case OMAP_DSS_CLK_SRC_FCK:
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index ae4e618..e8435ea 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -284,7 +284,6 @@ void dss_sdi_init(int datapairs);
 int dss_sdi_enable(void);
 void dss_sdi_disable(void);
 
-void dss_select_dispc_clk_source(enum omap_dss_clk_source clk_src);
 void dss_select_dsi_clk_source(int dsi_module,
 		enum omap_dss_clk_source clk_src);
 void dss_select_lcd_clk_source(enum omap_channel channel,
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index adcc906..dc6d72c 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -552,14 +552,6 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	/* Make selection of HDMI in DSS */
 	dss_select_hdmi_venc_clk_source(DSS_HDMI_M_PCLK);
 
-	/* Select the dispc clock source as PRCM clock, to ensure that it is not
-	 * DSI PLL source as the clock selected by DSI PLL might not be
-	 * sufficient for the resolution selected / that can be changed
-	 * dynamically by user. This can be moved to single location , say
-	 * Boardfile.
-	 */
-	dss_select_dispc_clk_source(dssdev->clocks.dispc.dispc_fclk_src);
-
 	/* bypass TV gamma table */
 	dispc_enable_gamma_table(0);
 
-- 
1.7.10.4

[PATCH 09/12] OMAPDSS: hide dss_select_dispc_clk_source()

[Tomi Valkeinen]

dss.c currently exposes functions to configure the dispc source clock
and lcd source clock. There are configured separately from the output
drivers.

However, there is no safe way for the output drivers to handle dispc
clock, as it's shared between the outputs. Thus, if, say, the DSI driver
sets up DSI PLL and configures both the dispc and lcd clock sources to
that DSI PLL, the resulting dispc clock could be too low for, say, HDMI.

Thus the output drivers should really only be concerned about the lcd
clock, which is what the output drivers actually use. There's lot to do
to clean up the dss clock handling, but this patch takes one step
forward and removes the use of dss_select_dispc_clk_source() from the
output drivers.

After this patch, the output drivers only configure the lcd source
clock. On omap4+ the dispc src clock is never changed from the default
PRCM source. On omap3, where the dispc and lcd clocks are actually the
same, setting the lcd clock source sets the dispc clock source.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dpi.c  |    6 ++++--
 drivers/video/omap2/dss/dsi.c  |    3 ---
 drivers/video/omap2/dss/dss.c  |    6 ++++--
 drivers/video/omap2/dss/dss.h  |    1 -
 drivers/video/omap2/dss/hdmi.c |    8 --------
 5 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 56748cf..9df62cf 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -77,6 +77,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
 		unsigned long pck_req, unsigned long *fck, int *lck_div,
 		int *pck_div)
 {
+	struct omap_overlay_manager *mgr = dssdev->output->manager;
 	struct dsi_clock_info dsi_cinfo;
 	struct dispc_clock_info dispc_cinfo;
 	int r;
@@ -90,7 +91,8 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
 	if (r)
 		return r;
 
-	dss_select_dispc_clk_source(dssdev->clocks.dispc.dispc_fclk_src);
+	dss_select_lcd_clk_source(mgr->id,
+			dssdev->clocks.dispc.channel.lcd_clk_src);
 
 	dpi.mgr_config.clock_info = dispc_cinfo;
 
@@ -272,7 +274,7 @@ void omapdss_dpi_display_disable(struct omap_dss_device *dssdev)
 	dss_mgr_disable(mgr);
 
 	if (dpi_use_dsi_pll(dssdev)) {
-		dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
+		dss_select_lcd_clk_source(mgr->id, OMAP_DSS_CLK_SRC_FCK);
 		dsi_pll_uninit(dpi.dsidev, true);
 		dsi_runtime_put(dpi.dsidev);
 	}
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 26d1a78..70bd7a5 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4720,7 +4720,6 @@ static int dsi_display_init_dsi(struct omap_dss_device *dssdev)
 	if (r)
 		goto err1;
 
-	dss_select_dispc_clk_source(dssdev->clocks.dispc.dispc_fclk_src);
 	dss_select_dsi_clk_source(dsi->module_id, dssdev->clocks.dsi.dsi_fclk_src);
 	dss_select_lcd_clk_source(mgr->id,
 			dssdev->clocks.dispc.channel.lcd_clk_src);
@@ -4755,7 +4754,6 @@ static int dsi_display_init_dsi(struct omap_dss_device *dssdev)
 err3:
 	dsi_cio_uninit(dsidev);
 err2:
-	dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
 	dss_select_dsi_clk_source(dsi->module_id, OMAP_DSS_CLK_SRC_FCK);
 	dss_select_lcd_clk_source(mgr->id, OMAP_DSS_CLK_SRC_FCK);
 
@@ -4782,7 +4780,6 @@ static void dsi_display_uninit_dsi(struct omap_dss_device *dssdev,
 	dsi_vc_enable(dsidev, 2, 0);
 	dsi_vc_enable(dsidev, 3, 0);
 
-	dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
 	dss_select_dsi_clk_source(dsi->module_id, OMAP_DSS_CLK_SRC_FCK);
 	dss_select_lcd_clk_source(mgr->id, OMAP_DSS_CLK_SRC_FCK);
 	dsi_cio_uninit(dsidev);
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 034cc1a..086ed56 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -304,7 +304,7 @@ static void dss_dump_regs(struct seq_file *s)
 #undef DUMPREG
 }
 
-void dss_select_dispc_clk_source(enum omap_dss_clk_source clk_src)
+static void dss_select_dispc_clk_source(enum omap_dss_clk_source clk_src)
 {
 	struct platform_device *dsidev;
 	int b;
@@ -375,8 +375,10 @@ void dss_select_lcd_clk_source(enum omap_channel channel,
 	struct platform_device *dsidev;
 	int b, ix, pos;
 
-	if (!dss_has_feature(FEAT_LCD_CLK_SRC))
+	if (!dss_has_feature(FEAT_LCD_CLK_SRC)) {
+		dss_select_dispc_clk_source(clk_src);
 		return;
+	}
 
 	switch (clk_src) {
 	case OMAP_DSS_CLK_SRC_FCK:
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index ae4e618..e8435ea 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -284,7 +284,6 @@ void dss_sdi_init(int datapairs);
 int dss_sdi_enable(void);
 void dss_sdi_disable(void);
 
-void dss_select_dispc_clk_source(enum omap_dss_clk_source clk_src);
 void dss_select_dsi_clk_source(int dsi_module,
 		enum omap_dss_clk_source clk_src);
 void dss_select_lcd_clk_source(enum omap_channel channel,
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index adcc906..dc6d72c 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -552,14 +552,6 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
 	/* Make selection of HDMI in DSS */
 	dss_select_hdmi_venc_clk_source(DSS_HDMI_M_PCLK);
 
-	/* Select the dispc clock source as PRCM clock, to ensure that it is not
-	 * DSI PLL source as the clock selected by DSI PLL might not be
-	 * sufficient for the resolution selected / that can be changed
-	 * dynamically by user. This can be moved to single location , say
-	 * Boardfile.
-	 */
-	dss_select_dispc_clk_source(dssdev->clocks.dispc.dispc_fclk_src);
-
 	/* bypass TV gamma table */
 	dispc_enable_gamma_table(0);
 
-- 
1.7.10.4

Re: [PATCH 09/12] OMAPDSS: hide dss_select_dispc_clk_source()

[Archit Taneja]

On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
> dss.c currently exposes functions to configure the dispc source clock
> and lcd source clock. There are configured separately from the output
> drivers.
>
> However, there is no safe way for the output drivers to handle dispc
> clock, as it's shared between the outputs. Thus, if, say, the DSI driver
> sets up DSI PLL and configures both the dispc and lcd clock sources to
> that DSI PLL, the resulting dispc clock could be too low for, say, HDMI.
>
> Thus the output drivers should really only be concerned about the lcd
> clock, which is what the output drivers actually use. There's lot to do
> to clean up the dss clock handling, but this patch takes one step
> forward and removes the use of dss_select_dispc_clk_source() from the
> output drivers.
>
> After this patch, the output drivers only configure the lcd source
> clock. On omap4+ the dispc src clock is never changed from the default
> PRCM source. On omap3, where the dispc and lcd clocks are actually the
> same, setting the lcd clock source sets the dispc clock source.

Maybe we could have one call to dss_select_dispc_clk_source() in 
omap_dsshw_porbe(). This is not necessary now, but if we support a 
splash screen on bootloader, and skip the hwmod resets, we might want to 
switch back our dispc clock source to PRCM if the output drivers don't 
to it. This is just a point though, we don't necessarily need it right now.

Archit

Re: [PATCH 09/12] OMAPDSS: hide dss_select_dispc_clk_source()

[Archit Taneja]

On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
> dss.c currently exposes functions to configure the dispc source clock
> and lcd source clock. There are configured separately from the output
> drivers.
>
> However, there is no safe way for the output drivers to handle dispc
> clock, as it's shared between the outputs. Thus, if, say, the DSI driver
> sets up DSI PLL and configures both the dispc and lcd clock sources to
> that DSI PLL, the resulting dispc clock could be too low for, say, HDMI.
>
> Thus the output drivers should really only be concerned about the lcd
> clock, which is what the output drivers actually use. There's lot to do
> to clean up the dss clock handling, but this patch takes one step
> forward and removes the use of dss_select_dispc_clk_source() from the
> output drivers.
>
> After this patch, the output drivers only configure the lcd source
> clock. On omap4+ the dispc src clock is never changed from the default
> PRCM source. On omap3, where the dispc and lcd clocks are actually the
> same, setting the lcd clock source sets the dispc clock source.

Maybe we could have one call to dss_select_dispc_clk_source() in 
omap_dsshw_porbe(). This is not necessary now, but if we support a 
splash screen on bootloader, and skip the hwmod resets, we might want to 
switch back our dispc clock source to PRCM if the output drivers don't 
to it. This is just a point though, we don't necessarily need it right now.

Archit

Re: [PATCH 09/12] OMAPDSS: hide dss_select_dispc_clk_source()

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2076 bytes --]

On 2012-10-31 08:54, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>> dss.c currently exposes functions to configure the dispc source clock
>> and lcd source clock. There are configured separately from the output
>> drivers.
>>
>> However, there is no safe way for the output drivers to handle dispc
>> clock, as it's shared between the outputs. Thus, if, say, the DSI driver
>> sets up DSI PLL and configures both the dispc and lcd clock sources to
>> that DSI PLL, the resulting dispc clock could be too low for, say, HDMI.
>>
>> Thus the output drivers should really only be concerned about the lcd
>> clock, which is what the output drivers actually use. There's lot to do
>> to clean up the dss clock handling, but this patch takes one step
>> forward and removes the use of dss_select_dispc_clk_source() from the
>> output drivers.
>>
>> After this patch, the output drivers only configure the lcd source
>> clock. On omap4+ the dispc src clock is never changed from the default
>> PRCM source. On omap3, where the dispc and lcd clocks are actually the
>> same, setting the lcd clock source sets the dispc clock source.
> 
> Maybe we could have one call to dss_select_dispc_clk_source() in
> omap_dsshw_porbe(). This is not necessary now, but if we support a
> splash screen on bootloader, and skip the hwmod resets, we might want to
> switch back our dispc clock source to PRCM if the output drivers don't
> to it. This is just a point though, we don't necessarily need it right now.

If we're showing the image from the bootloader, we can't change the
clock sources if we want to keep the display working. We either need to
accept and use the config the bootloader did, or reset the dss and start
over with our config.

But you're right, even if we don't support the splash screen from the
bootloader, we can't be sure if the dss hw was reset or not, and what
the clock source is. So it probably is safer to set the clock source at
probe time (and actually all other configs also).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 09/12] OMAPDSS: hide dss_select_dispc_clk_source()

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2076 bytes --]

On 2012-10-31 08:54, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>> dss.c currently exposes functions to configure the dispc source clock
>> and lcd source clock. There are configured separately from the output
>> drivers.
>>
>> However, there is no safe way for the output drivers to handle dispc
>> clock, as it's shared between the outputs. Thus, if, say, the DSI driver
>> sets up DSI PLL and configures both the dispc and lcd clock sources to
>> that DSI PLL, the resulting dispc clock could be too low for, say, HDMI.
>>
>> Thus the output drivers should really only be concerned about the lcd
>> clock, which is what the output drivers actually use. There's lot to do
>> to clean up the dss clock handling, but this patch takes one step
>> forward and removes the use of dss_select_dispc_clk_source() from the
>> output drivers.
>>
>> After this patch, the output drivers only configure the lcd source
>> clock. On omap4+ the dispc src clock is never changed from the default
>> PRCM source. On omap3, where the dispc and lcd clocks are actually the
>> same, setting the lcd clock source sets the dispc clock source.
> 
> Maybe we could have one call to dss_select_dispc_clk_source() in
> omap_dsshw_porbe(). This is not necessary now, but if we support a
> splash screen on bootloader, and skip the hwmod resets, we might want to
> switch back our dispc clock source to PRCM if the output drivers don't
> to it. This is just a point though, we don't necessarily need it right now.

If we're showing the image from the bootloader, we can't change the
clock sources if we want to keep the display working. We either need to
accept and use the config the bootloader did, or reset the dss and start
over with our config.

But you're right, even if we don't support the splash screen from the
bootloader, we can't be sure if the dss hw was reset or not, and what
the clock source is. So it probably is safer to set the clock source at
probe time (and actually all other configs also).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[PATCH 10/12] OMAPDSS: DPI: use dpi.dsidev to see whether to use dsi pll

[Tomi Valkeinen]

Instead of using dpi_use_dsi_pll() to check if dsi pll is to be used, we
can just check if dpi.dsidev != NULL.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dpi.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 9df62cf..fc1ec7d 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -137,7 +137,7 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
 	unsigned long pck;
 	int r = 0;
 
-	if (dpi_use_dsi_pll(dssdev))
+	if (dpi.dsidev)
 		r = dpi_set_dsi_clk(dssdev, t->pixel_clock * 1000, &fck,
 				&lck_div, &pck_div);
 	else
@@ -216,7 +216,7 @@ int omapdss_dpi_display_enable(struct omap_dss_device *dssdev)
 	if (r)
 		goto err_src_sel;
 
-	if (dpi_use_dsi_pll(dssdev)) {
+	if (dpi.dsidev) {
 		r = dsi_runtime_get(dpi.dsidev);
 		if (r)
 			goto err_get_dsi;
@@ -244,10 +244,10 @@ int omapdss_dpi_display_enable(struct omap_dss_device *dssdev)
 
 err_mgr_enable:
 err_set_mode:
-	if (dpi_use_dsi_pll(dssdev))
+	if (dpi.dsidev)
 		dsi_pll_uninit(dpi.dsidev, true);
 err_dsi_pll_init:
-	if (dpi_use_dsi_pll(dssdev))
+	if (dpi.dsidev)
 		dsi_runtime_put(dpi.dsidev);
 err_get_dsi:
 err_src_sel:
@@ -273,7 +273,7 @@ void omapdss_dpi_display_disable(struct omap_dss_device *dssdev)
 
 	dss_mgr_disable(mgr);
 
-	if (dpi_use_dsi_pll(dssdev)) {
+	if (dpi.dsidev) {
 		dss_select_lcd_clk_source(mgr->id, OMAP_DSS_CLK_SRC_FCK);
 		dsi_pll_uninit(dpi.dsidev, true);
 		dsi_runtime_put(dpi.dsidev);
@@ -319,7 +319,7 @@ int dpi_check_timings(struct omap_dss_device *dssdev,
 	if (timings->pixel_clock = 0)
 		return -EINVAL;
 
-	if (dpi_use_dsi_pll(dssdev)) {
+	if (dpi.dsidev) {
 		struct dsi_clock_info dsi_cinfo;
 		r = dsi_pll_calc_clock_div_pck(dpi.dsidev,
 				timings->pixel_clock * 1000,
-- 
1.7.10.4

[PATCH 10/12] OMAPDSS: DPI: use dpi.dsidev to see whether to use dsi pll

[Tomi Valkeinen]

Instead of using dpi_use_dsi_pll() to check if dsi pll is to be used, we
can just check if dpi.dsidev != NULL.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dpi.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 9df62cf..fc1ec7d 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -137,7 +137,7 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
 	unsigned long pck;
 	int r = 0;
 
-	if (dpi_use_dsi_pll(dssdev))
+	if (dpi.dsidev)
 		r = dpi_set_dsi_clk(dssdev, t->pixel_clock * 1000, &fck,
 				&lck_div, &pck_div);
 	else
@@ -216,7 +216,7 @@ int omapdss_dpi_display_enable(struct omap_dss_device *dssdev)
 	if (r)
 		goto err_src_sel;
 
-	if (dpi_use_dsi_pll(dssdev)) {
+	if (dpi.dsidev) {
 		r = dsi_runtime_get(dpi.dsidev);
 		if (r)
 			goto err_get_dsi;
@@ -244,10 +244,10 @@ int omapdss_dpi_display_enable(struct omap_dss_device *dssdev)
 
 err_mgr_enable:
 err_set_mode:
-	if (dpi_use_dsi_pll(dssdev))
+	if (dpi.dsidev)
 		dsi_pll_uninit(dpi.dsidev, true);
 err_dsi_pll_init:
-	if (dpi_use_dsi_pll(dssdev))
+	if (dpi.dsidev)
 		dsi_runtime_put(dpi.dsidev);
 err_get_dsi:
 err_src_sel:
@@ -273,7 +273,7 @@ void omapdss_dpi_display_disable(struct omap_dss_device *dssdev)
 
 	dss_mgr_disable(mgr);
 
-	if (dpi_use_dsi_pll(dssdev)) {
+	if (dpi.dsidev) {
 		dss_select_lcd_clk_source(mgr->id, OMAP_DSS_CLK_SRC_FCK);
 		dsi_pll_uninit(dpi.dsidev, true);
 		dsi_runtime_put(dpi.dsidev);
@@ -319,7 +319,7 @@ int dpi_check_timings(struct omap_dss_device *dssdev,
 	if (timings->pixel_clock == 0)
 		return -EINVAL;
 
-	if (dpi_use_dsi_pll(dssdev)) {
+	if (dpi.dsidev) {
 		struct dsi_clock_info dsi_cinfo;
 		r = dsi_pll_calc_clock_div_pck(dpi.dsidev,
 				timings->pixel_clock * 1000,
-- 
1.7.10.4

[PATCH 11/12] OMAPDSS: DPI: verify if DSI PLL is operational

[Tomi Valkeinen]

The SoCs that have DSI module should have a working DSI PLL. However,
some rare boards have not connected the powers to the DSI PLL.

This patch adds a function that tries to power up the DSI PLL, and
reports if that doesn't succeed. DPI uses this function to fall back to
PRCM clocks if DSI PLL doesn't work.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dpi.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index fc1ec7d..267caf0 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -361,6 +361,28 @@ void omapdss_dpi_set_data_lines(struct omap_dss_device *dssdev, int data_lines)
 }
 EXPORT_SYMBOL(omapdss_dpi_set_data_lines);
 
+static int __init dpi_verify_dsi_pll(struct platform_device *dsidev)
+{
+	int r;
+
+	/* do initial setup with the PLL to see if it is operational */
+
+	r = dsi_runtime_get(dsidev);
+	if (r)
+		return r;
+
+	r = dsi_pll_init(dsidev, 0, 1);
+	if (r) {
+		dsi_runtime_put(dsidev);
+		return r;
+	}
+
+	dsi_pll_uninit(dsidev, true);
+	dsi_runtime_put(dsidev);
+
+	return 0;
+}
+
 static int __init dpi_init_display(struct omap_dss_device *dssdev)
 {
 	DSSDBG("init_display\n");
@@ -383,6 +405,11 @@ static int __init dpi_init_display(struct omap_dss_device *dssdev)
 		enum omap_dss_clk_source dispc_fclk_src  			dssdev->clocks.dispc.dispc_fclk_src;
 		dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
+
+		if (dpi_verify_dsi_pll(dpi.dsidev)) {
+			dpi.dsidev = NULL;
+			DSSWARN("DSI PLL not operational\n");
+		}
 	}
 
 	return 0;
-- 
1.7.10.4

[PATCH 11/12] OMAPDSS: DPI: verify if DSI PLL is operational

[Tomi Valkeinen]

The SoCs that have DSI module should have a working DSI PLL. However,
some rare boards have not connected the powers to the DSI PLL.

This patch adds a function that tries to power up the DSI PLL, and
reports if that doesn't succeed. DPI uses this function to fall back to
PRCM clocks if DSI PLL doesn't work.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dpi.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index fc1ec7d..267caf0 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -361,6 +361,28 @@ void omapdss_dpi_set_data_lines(struct omap_dss_device *dssdev, int data_lines)
 }
 EXPORT_SYMBOL(omapdss_dpi_set_data_lines);
 
+static int __init dpi_verify_dsi_pll(struct platform_device *dsidev)
+{
+	int r;
+
+	/* do initial setup with the PLL to see if it is operational */
+
+	r = dsi_runtime_get(dsidev);
+	if (r)
+		return r;
+
+	r = dsi_pll_init(dsidev, 0, 1);
+	if (r) {
+		dsi_runtime_put(dsidev);
+		return r;
+	}
+
+	dsi_pll_uninit(dsidev, true);
+	dsi_runtime_put(dsidev);
+
+	return 0;
+}
+
 static int __init dpi_init_display(struct omap_dss_device *dssdev)
 {
 	DSSDBG("init_display\n");
@@ -383,6 +405,11 @@ static int __init dpi_init_display(struct omap_dss_device *dssdev)
 		enum omap_dss_clk_source dispc_fclk_src =
 			dssdev->clocks.dispc.dispc_fclk_src;
 		dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
+
+		if (dpi_verify_dsi_pll(dpi.dsidev)) {
+			dpi.dsidev = NULL;
+			DSSWARN("DSI PLL not operational\n");
+		}
 	}
 
 	return 0;
-- 
1.7.10.4

[PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

We currently get the decision whether to use PRCM or DSI PLL clock for
DPI from the board file. This is not a good way to handle it, and it
won't work with device tree.

This patch changes DPI to always use DSI PLL if it's available.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dpi.c |   64 ++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 267caf0..32e7dd5 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -49,28 +49,30 @@ static struct {
 	struct omap_dss_output output;
 } dpi;
 
-static struct platform_device *dpi_get_dsidev(enum omap_dss_clk_source clk)
+static struct platform_device *dpi_get_dsidev(enum omap_channel channel)
 {
-	int dsi_module;
-
-	dsi_module = clk = OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ? 0 : 1;
-
-	return dsi_get_dsidev_from_id(dsi_module);
+	switch (channel) {
+	case OMAP_DSS_CHANNEL_LCD:
+		return dsi_get_dsidev_from_id(0);
+	case OMAP_DSS_CHANNEL_LCD2:
+		return dsi_get_dsidev_from_id(1);
+	default:
+		return NULL;
+	}
 }
 
-static bool dpi_use_dsi_pll(struct omap_dss_device *dssdev)
+static enum omap_dss_clk_source dpi_get_alt_clk_src(enum omap_channel channel)
 {
-	if (dssdev->clocks.dispc.dispc_fclk_src =
-			OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ||
-			dssdev->clocks.dispc.dispc_fclk_src =
-			OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC ||
-			dssdev->clocks.dispc.channel.lcd_clk_src =
-			OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ||
-			dssdev->clocks.dispc.channel.lcd_clk_src =
-			OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC)
-		return true;
-	else
-		return false;
+	switch (channel) {
+	case OMAP_DSS_CHANNEL_LCD:
+		return OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC;
+	case OMAP_DSS_CHANNEL_LCD2:
+		return OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC;
+	default:
+		/* this shouldn't happen */
+		WARN_ON(1);
+		return OMAP_DSS_CLK_SRC_FCK;
+	}
 }
 
 static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
@@ -92,7 +94,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
 		return r;
 
 	dss_select_lcd_clk_source(mgr->id,
-			dssdev->clocks.dispc.channel.lcd_clk_src);
+			dpi_get_alt_clk_src(mgr->id));
 
 	dpi.mgr_config.clock_info = dispc_cinfo;
 
@@ -385,6 +387,8 @@ static int __init dpi_verify_dsi_pll(struct platform_device *dsidev)
 
 static int __init dpi_init_display(struct omap_dss_device *dssdev)
 {
+	struct platform_device *dsidev;
+
 	DSSDBG("init_display\n");
 
 	if (dss_has_feature(FEAT_DPI_USES_VDDS_DSI) &&
@@ -401,17 +405,23 @@ static int __init dpi_init_display(struct omap_dss_device *dssdev)
 		dpi.vdds_dsi_reg = vdds_dsi;
 	}
 
-	if (dpi_use_dsi_pll(dssdev)) {
-		enum omap_dss_clk_source dispc_fclk_src -			dssdev->clocks.dispc.dispc_fclk_src;
-		dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
+	/*
+	 * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
+	 * source for DPI is SoC integration detail, not something that should
+	 * be configured in the dssdev
+	 */
+	dsidev = dpi_get_dsidev(dssdev->channel);
 
-		if (dpi_verify_dsi_pll(dpi.dsidev)) {
-			dpi.dsidev = NULL;
-			DSSWARN("DSI PLL not operational\n");
-		}
+	if (dpi_verify_dsi_pll(dsidev)) {
+		dsidev = NULL;
+		DSSWARN("DSI PLL not operational\n");
 	}
 
+	if (dsidev)
+		DSSDBG("using DSI PLL for DPI clock\n");
+
+	dpi.dsidev = dsidev;
+
 	return 0;
 }
 
-- 
1.7.10.4

[PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

We currently get the decision whether to use PRCM or DSI PLL clock for
DPI from the board file. This is not a good way to handle it, and it
won't work with device tree.

This patch changes DPI to always use DSI PLL if it's available.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/dpi.c |   64 ++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 267caf0..32e7dd5 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -49,28 +49,30 @@ static struct {
 	struct omap_dss_output output;
 } dpi;
 
-static struct platform_device *dpi_get_dsidev(enum omap_dss_clk_source clk)
+static struct platform_device *dpi_get_dsidev(enum omap_channel channel)
 {
-	int dsi_module;
-
-	dsi_module = clk == OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ? 0 : 1;
-
-	return dsi_get_dsidev_from_id(dsi_module);
+	switch (channel) {
+	case OMAP_DSS_CHANNEL_LCD:
+		return dsi_get_dsidev_from_id(0);
+	case OMAP_DSS_CHANNEL_LCD2:
+		return dsi_get_dsidev_from_id(1);
+	default:
+		return NULL;
+	}
 }
 
-static bool dpi_use_dsi_pll(struct omap_dss_device *dssdev)
+static enum omap_dss_clk_source dpi_get_alt_clk_src(enum omap_channel channel)
 {
-	if (dssdev->clocks.dispc.dispc_fclk_src ==
-			OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ||
-			dssdev->clocks.dispc.dispc_fclk_src ==
-			OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC ||
-			dssdev->clocks.dispc.channel.lcd_clk_src ==
-			OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ||
-			dssdev->clocks.dispc.channel.lcd_clk_src ==
-			OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC)
-		return true;
-	else
-		return false;
+	switch (channel) {
+	case OMAP_DSS_CHANNEL_LCD:
+		return OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC;
+	case OMAP_DSS_CHANNEL_LCD2:
+		return OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC;
+	default:
+		/* this shouldn't happen */
+		WARN_ON(1);
+		return OMAP_DSS_CLK_SRC_FCK;
+	}
 }
 
 static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
@@ -92,7 +94,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
 		return r;
 
 	dss_select_lcd_clk_source(mgr->id,
-			dssdev->clocks.dispc.channel.lcd_clk_src);
+			dpi_get_alt_clk_src(mgr->id));
 
 	dpi.mgr_config.clock_info = dispc_cinfo;
 
@@ -385,6 +387,8 @@ static int __init dpi_verify_dsi_pll(struct platform_device *dsidev)
 
 static int __init dpi_init_display(struct omap_dss_device *dssdev)
 {
+	struct platform_device *dsidev;
+
 	DSSDBG("init_display\n");
 
 	if (dss_has_feature(FEAT_DPI_USES_VDDS_DSI) &&
@@ -401,17 +405,23 @@ static int __init dpi_init_display(struct omap_dss_device *dssdev)
 		dpi.vdds_dsi_reg = vdds_dsi;
 	}
 
-	if (dpi_use_dsi_pll(dssdev)) {
-		enum omap_dss_clk_source dispc_fclk_src =
-			dssdev->clocks.dispc.dispc_fclk_src;
-		dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
+	/*
+	 * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
+	 * source for DPI is SoC integration detail, not something that should
+	 * be configured in the dssdev
+	 */
+	dsidev = dpi_get_dsidev(dssdev->channel);
 
-		if (dpi_verify_dsi_pll(dpi.dsidev)) {
-			dpi.dsidev = NULL;
-			DSSWARN("DSI PLL not operational\n");
-		}
+	if (dpi_verify_dsi_pll(dsidev)) {
+		dsidev = NULL;
+		DSSWARN("DSI PLL not operational\n");
 	}
 
+	if (dsidev)
+		DSSDBG("using DSI PLL for DPI clock\n");
+
+	dpi.dsidev = dsidev;
+
 	return 0;
 }
 
-- 
1.7.10.4

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Archit Taneja]

On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
> We currently get the decision whether to use PRCM or DSI PLL clock for
> DPI from the board file. This is not a good way to handle it, and it
> won't work with device tree.
>
> This patch changes DPI to always use DSI PLL if it's available.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/dss/dpi.c |   64 ++++++++++++++++++++++++-----------------
>   1 file changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
> index 267caf0..32e7dd5 100644
> --- a/drivers/video/omap2/dss/dpi.c
> +++ b/drivers/video/omap2/dss/dpi.c
> @@ -49,28 +49,30 @@ static struct {
>   	struct omap_dss_output output;
>   } dpi;
>
> -static struct platform_device *dpi_get_dsidev(enum omap_dss_clk_source clk)
> +static struct platform_device *dpi_get_dsidev(enum omap_channel channel)
>   {
> -	int dsi_module;
> -
> -	dsi_module = clk == OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ? 0 : 1;
> -
> -	return dsi_get_dsidev_from_id(dsi_module);
> +	switch (channel) {
> +	case OMAP_DSS_CHANNEL_LCD:
> +		return dsi_get_dsidev_from_id(0);
> +	case OMAP_DSS_CHANNEL_LCD2:
> +		return dsi_get_dsidev_from_id(1);
> +	default:
> +		return NULL;
> +	}
>   }
>
> -static bool dpi_use_dsi_pll(struct omap_dss_device *dssdev)
> +static enum omap_dss_clk_source dpi_get_alt_clk_src(enum omap_channel channel)
>   {
> -	if (dssdev->clocks.dispc.dispc_fclk_src ==
> -			OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ||
> -			dssdev->clocks.dispc.dispc_fclk_src ==
> -			OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC ||
> -			dssdev->clocks.dispc.channel.lcd_clk_src ==
> -			OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ||
> -			dssdev->clocks.dispc.channel.lcd_clk_src ==
> -			OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC)
> -		return true;
> -	else
> -		return false;
> +	switch (channel) {
> +	case OMAP_DSS_CHANNEL_LCD:
> +		return OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC;
> +	case OMAP_DSS_CHANNEL_LCD2:
> +		return OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC;
> +	default:
> +		/* this shouldn't happen */
> +		WARN_ON(1);
> +		return OMAP_DSS_CLK_SRC_FCK;
> +	}
>   }
>
>   static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
> @@ -92,7 +94,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
>   		return r;
>
>   	dss_select_lcd_clk_source(mgr->id,
> -			dssdev->clocks.dispc.channel.lcd_clk_src);
> +			dpi_get_alt_clk_src(mgr->id));
>
>   	dpi.mgr_config.clock_info = dispc_cinfo;
>
> @@ -385,6 +387,8 @@ static int __init dpi_verify_dsi_pll(struct platform_device *dsidev)
>
>   static int __init dpi_init_display(struct omap_dss_device *dssdev)
>   {
> +	struct platform_device *dsidev;
> +
>   	DSSDBG("init_display\n");
>
>   	if (dss_has_feature(FEAT_DPI_USES_VDDS_DSI) &&
> @@ -401,17 +405,23 @@ static int __init dpi_init_display(struct omap_dss_device *dssdev)
>   		dpi.vdds_dsi_reg = vdds_dsi;
>   	}
>
> -	if (dpi_use_dsi_pll(dssdev)) {
> -		enum omap_dss_clk_source dispc_fclk_src =
> -			dssdev->clocks.dispc.dispc_fclk_src;
> -		dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
> +	/*
> +	 * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
> +	 * source for DPI is SoC integration detail, not something that should
> +	 * be configured in the dssdev
> +	 */

It is SoC integration detail, but it's flexible, it depends on which 
manager is connected to DPI output. If it's connected to LCD1, the 
source can be DSI1 PLL, if it's LCD2, it's source can be DSI2 PLL, if 
it's connected to TV manager, it's source "has to be" HDMI PLL. And 
these connections vary based on which OMAP revision we are on. We can 
only be certain on OMAP3 that the source for DPI pixel clock can be 
either PRCM or DSI PLL.

At the point of probe, we really don't know which manager is the DPI 
output connected to. Hence, we sort of use dssdev->channel to make a 
guess what manager we connect to in the future.

The right approach would be to figure this out at the time of enable, 
where we know which manager the DPI output is connected to. We could 
probably move the verification there too.

Archit

> +	dsidev = dpi_get_dsidev(dssdev->channel);
>
> -		if (dpi_verify_dsi_pll(dpi.dsidev)) {
> -			dpi.dsidev = NULL;
> -			DSSWARN("DSI PLL not operational\n");
> -		}
> +	if (dpi_verify_dsi_pll(dsidev)) {
> +		dsidev = NULL;
> +		DSSWARN("DSI PLL not operational\n");
>   	}
>
> +	if (dsidev)
> +		DSSDBG("using DSI PLL for DPI clock\n");
> +
> +	dpi.dsidev = dsidev;
> +
>   	return 0;
>   }
>
>

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Archit Taneja]

On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
> We currently get the decision whether to use PRCM or DSI PLL clock for
> DPI from the board file. This is not a good way to handle it, and it
> won't work with device tree.
>
> This patch changes DPI to always use DSI PLL if it's available.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/dss/dpi.c |   64 ++++++++++++++++++++++++-----------------
>   1 file changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
> index 267caf0..32e7dd5 100644
> --- a/drivers/video/omap2/dss/dpi.c
> +++ b/drivers/video/omap2/dss/dpi.c
> @@ -49,28 +49,30 @@ static struct {
>   	struct omap_dss_output output;
>   } dpi;
>
> -static struct platform_device *dpi_get_dsidev(enum omap_dss_clk_source clk)
> +static struct platform_device *dpi_get_dsidev(enum omap_channel channel)
>   {
> -	int dsi_module;
> -
> -	dsi_module = clk = OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ? 0 : 1;
> -
> -	return dsi_get_dsidev_from_id(dsi_module);
> +	switch (channel) {
> +	case OMAP_DSS_CHANNEL_LCD:
> +		return dsi_get_dsidev_from_id(0);
> +	case OMAP_DSS_CHANNEL_LCD2:
> +		return dsi_get_dsidev_from_id(1);
> +	default:
> +		return NULL;
> +	}
>   }
>
> -static bool dpi_use_dsi_pll(struct omap_dss_device *dssdev)
> +static enum omap_dss_clk_source dpi_get_alt_clk_src(enum omap_channel channel)
>   {
> -	if (dssdev->clocks.dispc.dispc_fclk_src =
> -			OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ||
> -			dssdev->clocks.dispc.dispc_fclk_src =
> -			OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC ||
> -			dssdev->clocks.dispc.channel.lcd_clk_src =
> -			OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC ||
> -			dssdev->clocks.dispc.channel.lcd_clk_src =
> -			OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC)
> -		return true;
> -	else
> -		return false;
> +	switch (channel) {
> +	case OMAP_DSS_CHANNEL_LCD:
> +		return OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC;
> +	case OMAP_DSS_CHANNEL_LCD2:
> +		return OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC;
> +	default:
> +		/* this shouldn't happen */
> +		WARN_ON(1);
> +		return OMAP_DSS_CLK_SRC_FCK;
> +	}
>   }
>
>   static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
> @@ -92,7 +94,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
>   		return r;
>
>   	dss_select_lcd_clk_source(mgr->id,
> -			dssdev->clocks.dispc.channel.lcd_clk_src);
> +			dpi_get_alt_clk_src(mgr->id));
>
>   	dpi.mgr_config.clock_info = dispc_cinfo;
>
> @@ -385,6 +387,8 @@ static int __init dpi_verify_dsi_pll(struct platform_device *dsidev)
>
>   static int __init dpi_init_display(struct omap_dss_device *dssdev)
>   {
> +	struct platform_device *dsidev;
> +
>   	DSSDBG("init_display\n");
>
>   	if (dss_has_feature(FEAT_DPI_USES_VDDS_DSI) &&
> @@ -401,17 +405,23 @@ static int __init dpi_init_display(struct omap_dss_device *dssdev)
>   		dpi.vdds_dsi_reg = vdds_dsi;
>   	}
>
> -	if (dpi_use_dsi_pll(dssdev)) {
> -		enum omap_dss_clk_source dispc_fclk_src > -			dssdev->clocks.dispc.dispc_fclk_src;
> -		dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
> +	/*
> +	 * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
> +	 * source for DPI is SoC integration detail, not something that should
> +	 * be configured in the dssdev
> +	 */

It is SoC integration detail, but it's flexible, it depends on which 
manager is connected to DPI output. If it's connected to LCD1, the 
source can be DSI1 PLL, if it's LCD2, it's source can be DSI2 PLL, if 
it's connected to TV manager, it's source "has to be" HDMI PLL. And 
these connections vary based on which OMAP revision we are on. We can 
only be certain on OMAP3 that the source for DPI pixel clock can be 
either PRCM or DSI PLL.

At the point of probe, we really don't know which manager is the DPI 
output connected to. Hence, we sort of use dssdev->channel to make a 
guess what manager we connect to in the future.

The right approach would be to figure this out at the time of enable, 
where we know which manager the DPI output is connected to. We could 
probably move the verification there too.

Archit

> +	dsidev = dpi_get_dsidev(dssdev->channel);
>
> -		if (dpi_verify_dsi_pll(dpi.dsidev)) {
> -			dpi.dsidev = NULL;
> -			DSSWARN("DSI PLL not operational\n");
> -		}
> +	if (dpi_verify_dsi_pll(dsidev)) {
> +		dsidev = NULL;
> +		DSSWARN("DSI PLL not operational\n");
>   	}
>
> +	if (dsidev)
> +		DSSDBG("using DSI PLL for DPI clock\n");
> +
> +	dpi.dsidev = dsidev;
> +
>   	return 0;
>   }
>
>

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

On 2012-10-31 09:26, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:

>> -    if (dpi_use_dsi_pll(dssdev)) {
>> -        enum omap_dss_clk_source dispc_fclk_src =
>> -            dssdev->clocks.dispc.dispc_fclk_src;
>> -        dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
>> +    /*
>> +     * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
>> +     * source for DPI is SoC integration detail, not something that
>> should
>> +     * be configured in the dssdev
>> +     */
> 
> It is SoC integration detail, but it's flexible, it depends on which
> manager is connected to DPI output. If it's connected to LCD1, the

Hmm, yes, the comment is a bit misleading. The DSI PLL is not used for
DPI, but for DISPC's LCD output. And DPI uses one of those LCD outputs.

> source can be DSI1 PLL, if it's LCD2, it's source can be DSI2 PLL, if
> it's connected to TV manager, it's source "has to be" HDMI PLL. And
> these connections vary based on which OMAP revision we are on. We can
> only be certain on OMAP3 that the source for DPI pixel clock can be
> either PRCM or DSI PLL.

On OMAP2 we can be certain the clock is PRCM =).

> At the point of probe, we really don't know which manager is the DPI
> output connected to. Hence, we sort of use dssdev->channel to make a
> guess what manager we connect to in the future.

Yep. My point was mainly that dssdev needs to go away, and we should
somehow decide the used channel internally.

> The right approach would be to figure this out at the time of enable,
> where we know which manager the DPI output is connected to. We could
> probably move the verification there too.

Who chooses which manager to use for DPI?

I'm not sure... I would really like to manage the basic setup, acquiring
the resources, etc. at probe time, and enable would only enable the display.

That means that we should somehow get a manager for DPI at probe time,
which may be a bit difficult, as we don't know what other displays there
will be. So if, say, DPI can use LCD1 or LCD2, but DSI can only use
LCD2, and at DPI's probe (presuming it's before DSI) we pick LCD2, DSI
won't work.

Anyway, while I'm not sure how to solve the above problem, I think we
should improve our init a bit. For DPI there are the following steps
done, in order:

- DPI device added
- DPI driver probe
- DPI panel device added
- DPI panel driver probe

We currently add the panel device in DPI driver's probe, and figure out
the DSI PLL at the same time. I think that should happen only when the
panel driver probe happens. The panel driver should call something like
dpi_get_output() or whatever, which acquires the DPI bus for the panel
driver, and this would probably also choose the manager.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

On 2012-10-31 09:26, Archit Taneja wrote:
> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:

>> -    if (dpi_use_dsi_pll(dssdev)) {
>> -        enum omap_dss_clk_source dispc_fclk_src =
>> -            dssdev->clocks.dispc.dispc_fclk_src;
>> -        dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
>> +    /*
>> +     * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
>> +     * source for DPI is SoC integration detail, not something that
>> should
>> +     * be configured in the dssdev
>> +     */
> 
> It is SoC integration detail, but it's flexible, it depends on which
> manager is connected to DPI output. If it's connected to LCD1, the

Hmm, yes, the comment is a bit misleading. The DSI PLL is not used for
DPI, but for DISPC's LCD output. And DPI uses one of those LCD outputs.

> source can be DSI1 PLL, if it's LCD2, it's source can be DSI2 PLL, if
> it's connected to TV manager, it's source "has to be" HDMI PLL. And
> these connections vary based on which OMAP revision we are on. We can
> only be certain on OMAP3 that the source for DPI pixel clock can be
> either PRCM or DSI PLL.

On OMAP2 we can be certain the clock is PRCM =).

> At the point of probe, we really don't know which manager is the DPI
> output connected to. Hence, we sort of use dssdev->channel to make a
> guess what manager we connect to in the future.

Yep. My point was mainly that dssdev needs to go away, and we should
somehow decide the used channel internally.

> The right approach would be to figure this out at the time of enable,
> where we know which manager the DPI output is connected to. We could
> probably move the verification there too.

Who chooses which manager to use for DPI?

I'm not sure... I would really like to manage the basic setup, acquiring
the resources, etc. at probe time, and enable would only enable the display.

That means that we should somehow get a manager for DPI at probe time,
which may be a bit difficult, as we don't know what other displays there
will be. So if, say, DPI can use LCD1 or LCD2, but DSI can only use
LCD2, and at DPI's probe (presuming it's before DSI) we pick LCD2, DSI
won't work.

Anyway, while I'm not sure how to solve the above problem, I think we
should improve our init a bit. For DPI there are the following steps
done, in order:

- DPI device added
- DPI driver probe
- DPI panel device added
- DPI panel driver probe

We currently add the panel device in DPI driver's probe, and figure out
the DSI PLL at the same time. I think that should happen only when the
panel driver probe happens. The panel driver should call something like
dpi_get_output() or whatever, which acquires the DPI bus for the panel
driver, and this would probably also choose the manager.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Archit Taneja]

On Friday 02 November 2012 03:38 PM, Tomi Valkeinen wrote:
> On 2012-10-31 09:26, Archit Taneja wrote:
>> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>
>>> -    if (dpi_use_dsi_pll(dssdev)) {
>>> -        enum omap_dss_clk_source dispc_fclk_src =
>>> -            dssdev->clocks.dispc.dispc_fclk_src;
>>> -        dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
>>> +    /*
>>> +     * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
>>> +     * source for DPI is SoC integration detail, not something that
>>> should
>>> +     * be configured in the dssdev
>>> +     */
>>
>> It is SoC integration detail, but it's flexible, it depends on which
>> manager is connected to DPI output. If it's connected to LCD1, the
>
> Hmm, yes, the comment is a bit misleading. The DSI PLL is not used for
> DPI, but for DISPC's LCD output. And DPI uses one of those LCD outputs.
>
>> source can be DSI1 PLL, if it's LCD2, it's source can be DSI2 PLL, if
>> it's connected to TV manager, it's source "has to be" HDMI PLL. And
>> these connections vary based on which OMAP revision we are on. We can
>> only be certain on OMAP3 that the source for DPI pixel clock can be
>> either PRCM or DSI PLL.
>
> On OMAP2 we can be certain the clock is PRCM =).
>
>> At the point of probe, we really don't know which manager is the DPI
>> output connected to. Hence, we sort of use dssdev->channel to make a
>> guess what manager we connect to in the future.
>
> Yep. My point was mainly that dssdev needs to go away, and we should
> somehow decide the used channel internally.
>
>> The right approach would be to figure this out at the time of enable,
>> where we know which manager the DPI output is connected to. We could
>> probably move the verification there too.
>
> Who chooses which manager to use for DPI?

If you are asking in terms of HW. The value in the DSS_CTRL bitfield 
decides which manager to use.

If you meant who/how should we choose this in software, then I don't 
know either.

>
> I'm not sure... I would really like to manage the basic setup, acquiring
> the resources, etc. at probe time, and enable would only enable the display.

One thing we could do is to grab all the possible resources that DPI can 
use for its pixel clock, and when it's enable time, see what all options 
it has. So, for example, for DPI on omap4, we could try to grab and 
verify all DSI PLL, HDMI PLL and PRCM. And then later choose the most 
appropriate one while enabling.

>
> That means that we should somehow get a manager for DPI at probe time,
> which may be a bit difficult, as we don't know what other displays there
> will be. So if, say, DPI can use LCD1 or LCD2, but DSI can only use
> LCD2, and at DPI's probe (presuming it's before DSI) we pick LCD2, DSI
> won't work.

Yes, it's not easy to know this at probe time. We could try to allocate 
resources at the time mgr->set_output() is called. We could have an 
output specific op. dss_mgr_set_output() could look like:

dss_mgr_set_output(mgr, output)
{
	/* Do the older stuff */
	...
	...

	output->get_and_config_resources(output);
}

In dpi.c:

dss_dpi_get_and_config_resources(output)
{
	switch (output->manager->id) {
	case LCD:
		Get DSI 1 PLL;
	case LCD2:
		Get DSI2 PLL;
	case TV:
		Get HDMI PLL;
	}

	/*
	 * Also set the DSS_CTRL bits here to tell which manager
	 * we need to connect to
	 */

	dss_select_dpi_manager(output->manager->id);
}

omapdss_output_ops dpi_ops = {
	.get_and_config_resources = dss_dpi_get_and_config_resources,
	...
};

However, even though this approach might be correct in the sense that we 
confiugre dpi when we know what manager we are connected to, I have to 
say that its not nice at all. Especially because setting manager output 
links is very omapdss-apply-ish. But I feel we would need to do 
something similar in omapdrm too.

>
> Anyway, while I'm not sure how to solve the above problem, I think we
> should improve our init a bit. For DPI there are the following steps
> done, in order:
>
> - DPI device added
> - DPI driver probe
> - DPI panel device added
> - DPI panel driver probe
>
> We currently add the panel device in DPI driver's probe, and figure out
> the DSI PLL at the same time. I think that should happen only when the
> panel driver probe happens. The panel driver should call something like
> dpi_get_output() or whatever, which acquires the DPI bus for the panel
> driver, and this would probably also choose the manager.

Hmm, that makes sense. Anyway, I don't think it's really bad if we refer 
to dssdev->channel for now.

I think we could have a clearer picture of this when we understand how 
omapdrm sets the links between its entities and how CPF would link the 
output-panel side of things.

Archit

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Archit Taneja]

On Friday 02 November 2012 03:38 PM, Tomi Valkeinen wrote:
> On 2012-10-31 09:26, Archit Taneja wrote:
>> On Tuesday 30 October 2012 09:40 PM, Tomi Valkeinen wrote:
>
>>> -    if (dpi_use_dsi_pll(dssdev)) {
>>> -        enum omap_dss_clk_source dispc_fclk_src >>> -            dssdev->clocks.dispc.dispc_fclk_src;
>>> -        dpi.dsidev = dpi_get_dsidev(dispc_fclk_src);
>>> +    /*
>>> +     * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
>>> +     * source for DPI is SoC integration detail, not something that
>>> should
>>> +     * be configured in the dssdev
>>> +     */
>>
>> It is SoC integration detail, but it's flexible, it depends on which
>> manager is connected to DPI output. If it's connected to LCD1, the
>
> Hmm, yes, the comment is a bit misleading. The DSI PLL is not used for
> DPI, but for DISPC's LCD output. And DPI uses one of those LCD outputs.
>
>> source can be DSI1 PLL, if it's LCD2, it's source can be DSI2 PLL, if
>> it's connected to TV manager, it's source "has to be" HDMI PLL. And
>> these connections vary based on which OMAP revision we are on. We can
>> only be certain on OMAP3 that the source for DPI pixel clock can be
>> either PRCM or DSI PLL.
>
> On OMAP2 we can be certain the clock is PRCM =).
>
>> At the point of probe, we really don't know which manager is the DPI
>> output connected to. Hence, we sort of use dssdev->channel to make a
>> guess what manager we connect to in the future.
>
> Yep. My point was mainly that dssdev needs to go away, and we should
> somehow decide the used channel internally.
>
>> The right approach would be to figure this out at the time of enable,
>> where we know which manager the DPI output is connected to. We could
>> probably move the verification there too.
>
> Who chooses which manager to use for DPI?

If you are asking in terms of HW. The value in the DSS_CTRL bitfield 
decides which manager to use.

If you meant who/how should we choose this in software, then I don't 
know either.

>
> I'm not sure... I would really like to manage the basic setup, acquiring
> the resources, etc. at probe time, and enable would only enable the display.

One thing we could do is to grab all the possible resources that DPI can 
use for its pixel clock, and when it's enable time, see what all options 
it has. So, for example, for DPI on omap4, we could try to grab and 
verify all DSI PLL, HDMI PLL and PRCM. And then later choose the most 
appropriate one while enabling.

>
> That means that we should somehow get a manager for DPI at probe time,
> which may be a bit difficult, as we don't know what other displays there
> will be. So if, say, DPI can use LCD1 or LCD2, but DSI can only use
> LCD2, and at DPI's probe (presuming it's before DSI) we pick LCD2, DSI
> won't work.

Yes, it's not easy to know this at probe time. We could try to allocate 
resources at the time mgr->set_output() is called. We could have an 
output specific op. dss_mgr_set_output() could look like:

dss_mgr_set_output(mgr, output)
{
	/* Do the older stuff */
	...
	...

	output->get_and_config_resources(output);
}

In dpi.c:

dss_dpi_get_and_config_resources(output)
{
	switch (output->manager->id) {
	case LCD:
		Get DSI 1 PLL;
	case LCD2:
		Get DSI2 PLL;
	case TV:
		Get HDMI PLL;
	}

	/*
	 * Also set the DSS_CTRL bits here to tell which manager
	 * we need to connect to
	 */

	dss_select_dpi_manager(output->manager->id);
}

omapdss_output_ops dpi_ops = {
	.get_and_config_resources = dss_dpi_get_and_config_resources,
	...
};

However, even though this approach might be correct in the sense that we 
confiugre dpi when we know what manager we are connected to, I have to 
say that its not nice at all. Especially because setting manager output 
links is very omapdss-apply-ish. But I feel we would need to do 
something similar in omapdrm too.

>
> Anyway, while I'm not sure how to solve the above problem, I think we
> should improve our init a bit. For DPI there are the following steps
> done, in order:
>
> - DPI device added
> - DPI driver probe
> - DPI panel device added
> - DPI panel driver probe
>
> We currently add the panel device in DPI driver's probe, and figure out
> the DSI PLL at the same time. I think that should happen only when the
> panel driver probe happens. The panel driver should call something like
> dpi_get_output() or whatever, which acquires the DPI bus for the panel
> driver, and this would probably also choose the manager.

Hmm, that makes sense. Anyway, I don't think it's really bad if we refer 
to dssdev->channel for now.

I think we could have a clearer picture of this when we understand how 
omapdrm sets the links between its entities and how CPF would link the 
output-panel side of things.

Archit

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

On 2012-11-02 12:44, Archit Taneja wrote:

> Hmm, that makes sense. Anyway, I don't think it's really bad if we refer
> to dssdev->channel for now.

It is, because dssdev->channel doesn't exist with DT.

With DT we either need to figure out the channel in omapdss at runtime,
or add a property to the DT data telling the channel. And adding such a
property is not correct, as DT should be about describing the HW.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

On 2012-11-02 12:44, Archit Taneja wrote:

> Hmm, that makes sense. Anyway, I don't think it's really bad if we refer
> to dssdev->channel for now.

It is, because dssdev->channel doesn't exist with DT.

With DT we either need to figure out the channel in omapdss at runtime,
or add a property to the DT data telling the channel. And adding such a
property is not correct, as DT should be about describing the HW.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Archit Taneja]

On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
> On 2012-11-02 12:44, Archit Taneja wrote:
>
>> Hmm, that makes sense. Anyway, I don't think it's really bad if we refer
>> to dssdev->channel for now.
>
> It is, because dssdev->channel doesn't exist with DT.
>
> With DT we either need to figure out the channel in omapdss at runtime,
> or add a property to the DT data telling the channel. And adding such a
> property is not correct, as DT should be about describing the HW.

Ok.

I don't totally agree with your idea of figuring out the manager in 
panel the panel's probe. If it's done in the panel driver's probe 
itself, then by this point of time we have already set 
mgr->output->device links. If omapdss only does this stuff, then 
omapfb/omapdrm have just the job of connecting the overlays to the 
manager. Do you think that's okay?

Archit

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Archit Taneja]

On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
> On 2012-11-02 12:44, Archit Taneja wrote:
>
>> Hmm, that makes sense. Anyway, I don't think it's really bad if we refer
>> to dssdev->channel for now.
>
> It is, because dssdev->channel doesn't exist with DT.
>
> With DT we either need to figure out the channel in omapdss at runtime,
> or add a property to the DT data telling the channel. And adding such a
> property is not correct, as DT should be about describing the HW.

Ok.

I don't totally agree with your idea of figuring out the manager in 
panel the panel's probe. If it's done in the panel driver's probe 
itself, then by this point of time we have already set 
mgr->output->device links. If omapdss only does this stuff, then 
omapfb/omapdrm have just the job of connecting the overlays to the 
manager. Do you think that's okay?

Archit

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

On 2012-11-02 13:09, Archit Taneja wrote:
> On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
>> On 2012-11-02 12:44, Archit Taneja wrote:
>>
>>> Hmm, that makes sense. Anyway, I don't think it's really bad if we refer
>>> to dssdev->channel for now.
>>
>> It is, because dssdev->channel doesn't exist with DT.
>>
>> With DT we either need to figure out the channel in omapdss at runtime,
>> or add a property to the DT data telling the channel. And adding such a
>> property is not correct, as DT should be about describing the HW.
> 
> Ok.
> 
> I don't totally agree with your idea of figuring out the manager in
> panel the panel's probe. If it's done in the panel driver's probe
> itself, then by this point of time we have already set
> mgr->output->device links. If omapdss only does this stuff, then

Hmm, I'm not sure I understand what's your point above? If figuring out
the mgr is done in panel's probe, the mgr->output link is not yet made
before that time.

> omapfb/omapdrm have just the job of connecting the overlays to the
> manager. Do you think that's okay?

Yes, that's how I think it should be. I don't see why omapfb/omapdrm
should care about which manager is being used for the output, it doesn't
really matter as long there is one and it works.

Then again, I don't have anything against omapfb/omapdrm choosing the
manager, but I don't see how they would have any better idea of which
manager to use than omapdss.

But as doing the connections at probe time is a bit problematic, perhaps
we should have a new step in this whole sequence. Something like
"connect" or whatever, which would lock the required blocks in the whole
pipeline, and acquire the required resources that couldn't be gotten at
probe time.

But even then, choosing the manager is not easy, as whoever chooses the
manager needs to observe all the possible displays used at the same time...

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

On 2012-11-02 13:09, Archit Taneja wrote:
> On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
>> On 2012-11-02 12:44, Archit Taneja wrote:
>>
>>> Hmm, that makes sense. Anyway, I don't think it's really bad if we refer
>>> to dssdev->channel for now.
>>
>> It is, because dssdev->channel doesn't exist with DT.
>>
>> With DT we either need to figure out the channel in omapdss at runtime,
>> or add a property to the DT data telling the channel. And adding such a
>> property is not correct, as DT should be about describing the HW.
> 
> Ok.
> 
> I don't totally agree with your idea of figuring out the manager in
> panel the panel's probe. If it's done in the panel driver's probe
> itself, then by this point of time we have already set
> mgr->output->device links. If omapdss only does this stuff, then

Hmm, I'm not sure I understand what's your point above? If figuring out
the mgr is done in panel's probe, the mgr->output link is not yet made
before that time.

> omapfb/omapdrm have just the job of connecting the overlays to the
> manager. Do you think that's okay?

Yes, that's how I think it should be. I don't see why omapfb/omapdrm
should care about which manager is being used for the output, it doesn't
really matter as long there is one and it works.

Then again, I don't have anything against omapfb/omapdrm choosing the
manager, but I don't see how they would have any better idea of which
manager to use than omapdss.

But as doing the connections at probe time is a bit problematic, perhaps
we should have a new step in this whole sequence. Something like
"connect" or whatever, which would lock the required blocks in the whole
pipeline, and acquire the required resources that couldn't be gotten at
probe time.

But even then, choosing the manager is not easy, as whoever chooses the
manager needs to observe all the possible displays used at the same time...

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Archit Taneja]

On Friday 02 November 2012 04:58 PM, Tomi Valkeinen wrote:
> On 2012-11-02 13:09, Archit Taneja wrote:
>> On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
>>> On 2012-11-02 12:44, Archit Taneja wrote:
>>>
>>>> Hmm, that makes sense. Anyway, I don't think it's really bad if we refer
>>>> to dssdev->channel for now.
>>>
>>> It is, because dssdev->channel doesn't exist with DT.
>>>
>>> With DT we either need to figure out the channel in omapdss at runtime,
>>> or add a property to the DT data telling the channel. And adding such a
>>> property is not correct, as DT should be about describing the HW.
>>
>> Ok.
>>
>> I don't totally agree with your idea of figuring out the manager in
>> panel the panel's probe. If it's done in the panel driver's probe
>> itself, then by this point of time we have already set
>> mgr->output->device links. If omapdss only does this stuff, then
>
> Hmm, I'm not sure I understand what's your point above? If figuring out
> the mgr is done in panel's probe, the mgr->output link is not yet made
> before that time.

My point is that we are trying to find a manager at panel's probe 
itself. It think that's what we do now. But one of your recent patch 
moves that to omapfb.

>
>> omapfb/omapdrm have just the job of connecting the overlays to the
>> manager. Do you think that's okay?
>
> Yes, that's how I think it should be. I don't see why omapfb/omapdrm
> should care about which manager is being used for the output, it doesn't
> really matter as long there is one and it works.
>
> Then again, I don't have anything against omapfb/omapdrm choosing the
> manager, but I don't see how they would have any better idea of which
> manager to use than omapdss.
>
> But as doing the connections at probe time is a bit problematic, perhaps
> we should have a new step in this whole sequence. Something like
> "connect" or whatever, which would lock the required blocks in the whole
> pipeline, and acquire the required resources that couldn't be gotten at
> probe time.
>
> But even then, choosing the manager is not easy, as whoever chooses the
> manager needs to observe all the possible displays used at the same time...

Right. I was wondering if omapfb/omapdrm could understand the 'all 
possible displays information' better compared to a panel's probe.

Even omapdrm/omafb can't be perfect because we could insert a panel 
driver module at any time, and omapfb/omapdrm may miss that out.

Archit

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Archit Taneja]

On Friday 02 November 2012 04:58 PM, Tomi Valkeinen wrote:
> On 2012-11-02 13:09, Archit Taneja wrote:
>> On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
>>> On 2012-11-02 12:44, Archit Taneja wrote:
>>>
>>>> Hmm, that makes sense. Anyway, I don't think it's really bad if we refer
>>>> to dssdev->channel for now.
>>>
>>> It is, because dssdev->channel doesn't exist with DT.
>>>
>>> With DT we either need to figure out the channel in omapdss at runtime,
>>> or add a property to the DT data telling the channel. And adding such a
>>> property is not correct, as DT should be about describing the HW.
>>
>> Ok.
>>
>> I don't totally agree with your idea of figuring out the manager in
>> panel the panel's probe. If it's done in the panel driver's probe
>> itself, then by this point of time we have already set
>> mgr->output->device links. If omapdss only does this stuff, then
>
> Hmm, I'm not sure I understand what's your point above? If figuring out
> the mgr is done in panel's probe, the mgr->output link is not yet made
> before that time.

My point is that we are trying to find a manager at panel's probe 
itself. It think that's what we do now. But one of your recent patch 
moves that to omapfb.

>
>> omapfb/omapdrm have just the job of connecting the overlays to the
>> manager. Do you think that's okay?
>
> Yes, that's how I think it should be. I don't see why omapfb/omapdrm
> should care about which manager is being used for the output, it doesn't
> really matter as long there is one and it works.
>
> Then again, I don't have anything against omapfb/omapdrm choosing the
> manager, but I don't see how they would have any better idea of which
> manager to use than omapdss.
>
> But as doing the connections at probe time is a bit problematic, perhaps
> we should have a new step in this whole sequence. Something like
> "connect" or whatever, which would lock the required blocks in the whole
> pipeline, and acquire the required resources that couldn't be gotten at
> probe time.
>
> But even then, choosing the manager is not easy, as whoever chooses the
> manager needs to observe all the possible displays used at the same time...

Right. I was wondering if omapfb/omapdrm could understand the 'all 
possible displays information' better compared to a panel's probe.

Even omapdrm/omafb can't be perfect because we could insert a panel 
driver module at any time, and omapfb/omapdrm may miss that out.

Archit

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]

On 2012-11-02 13:56, Archit Taneja wrote:
> On Friday 02 November 2012 04:58 PM, Tomi Valkeinen wrote:
>> On 2012-11-02 13:09, Archit Taneja wrote:
>>> On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
>>>> On 2012-11-02 12:44, Archit Taneja wrote:
>>>>
>>>>> Hmm, that makes sense. Anyway, I don't think it's really bad if we
>>>>> refer
>>>>> to dssdev->channel for now.
>>>>
>>>> It is, because dssdev->channel doesn't exist with DT.
>>>>
>>>> With DT we either need to figure out the channel in omapdss at runtime,
>>>> or add a property to the DT data telling the channel. And adding such a
>>>> property is not correct, as DT should be about describing the HW.
>>>
>>> Ok.
>>>
>>> I don't totally agree with your idea of figuring out the manager in
>>> panel the panel's probe. If it's done in the panel driver's probe
>>> itself, then by this point of time we have already set
>>> mgr->output->device links. If omapdss only does this stuff, then
>>
>> Hmm, I'm not sure I understand what's your point above? If figuring out
>> the mgr is done in panel's probe, the mgr->output link is not yet made
>> before that time.
> 
> My point is that we are trying to find a manager at panel's probe
> itself. It think that's what we do now. But one of your recent patch
> moves that to omapfb.

Ah. Yes, that's true.

>>> omapfb/omapdrm have just the job of connecting the overlays to the
>>> manager. Do you think that's okay?
>>
>> Yes, that's how I think it should be. I don't see why omapfb/omapdrm
>> should care about which manager is being used for the output, it doesn't
>> really matter as long there is one and it works.
>>
>> Then again, I don't have anything against omapfb/omapdrm choosing the
>> manager, but I don't see how they would have any better idea of which
>> manager to use than omapdss.
>>
>> But as doing the connections at probe time is a bit problematic, perhaps
>> we should have a new step in this whole sequence. Something like
>> "connect" or whatever, which would lock the required blocks in the whole
>> pipeline, and acquire the required resources that couldn't be gotten at
>> probe time.
>>
>> But even then, choosing the manager is not easy, as whoever chooses the
>> manager needs to observe all the possible displays used at the same
>> time...
> 
> Right. I was wondering if omapfb/omapdrm could understand the 'all
> possible displays information' better compared to a panel's probe.
> 
> Even omapdrm/omafb can't be perfect because we could insert a panel
> driver module at any time, and omapfb/omapdrm may miss that out.

True, omapdrm/fb may have a better idea. It's still unclear though.
Currently we have quite strict order in the sequence the modules need to
be loaded, which is quite bad and causes issues. We should make things
more dynamic, so that the initialization of the drivers could happen
more freely.

But that creates more problems: when booting up, omapfb starts. But
omapfb can't know if all the panel drivers have already been loaded.
omapfb may see that DVI is the default display, but what should it do if
DVI doesn't have a driver yet? It could wait, but perhaps the driver for
DVI will never even be loaded.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]

On 2012-11-02 13:56, Archit Taneja wrote:
> On Friday 02 November 2012 04:58 PM, Tomi Valkeinen wrote:
>> On 2012-11-02 13:09, Archit Taneja wrote:
>>> On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
>>>> On 2012-11-02 12:44, Archit Taneja wrote:
>>>>
>>>>> Hmm, that makes sense. Anyway, I don't think it's really bad if we
>>>>> refer
>>>>> to dssdev->channel for now.
>>>>
>>>> It is, because dssdev->channel doesn't exist with DT.
>>>>
>>>> With DT we either need to figure out the channel in omapdss at runtime,
>>>> or add a property to the DT data telling the channel. And adding such a
>>>> property is not correct, as DT should be about describing the HW.
>>>
>>> Ok.
>>>
>>> I don't totally agree with your idea of figuring out the manager in
>>> panel the panel's probe. If it's done in the panel driver's probe
>>> itself, then by this point of time we have already set
>>> mgr->output->device links. If omapdss only does this stuff, then
>>
>> Hmm, I'm not sure I understand what's your point above? If figuring out
>> the mgr is done in panel's probe, the mgr->output link is not yet made
>> before that time.
> 
> My point is that we are trying to find a manager at panel's probe
> itself. It think that's what we do now. But one of your recent patch
> moves that to omapfb.

Ah. Yes, that's true.

>>> omapfb/omapdrm have just the job of connecting the overlays to the
>>> manager. Do you think that's okay?
>>
>> Yes, that's how I think it should be. I don't see why omapfb/omapdrm
>> should care about which manager is being used for the output, it doesn't
>> really matter as long there is one and it works.
>>
>> Then again, I don't have anything against omapfb/omapdrm choosing the
>> manager, but I don't see how they would have any better idea of which
>> manager to use than omapdss.
>>
>> But as doing the connections at probe time is a bit problematic, perhaps
>> we should have a new step in this whole sequence. Something like
>> "connect" or whatever, which would lock the required blocks in the whole
>> pipeline, and acquire the required resources that couldn't be gotten at
>> probe time.
>>
>> But even then, choosing the manager is not easy, as whoever chooses the
>> manager needs to observe all the possible displays used at the same
>> time...
> 
> Right. I was wondering if omapfb/omapdrm could understand the 'all
> possible displays information' better compared to a panel's probe.
> 
> Even omapdrm/omafb can't be perfect because we could insert a panel
> driver module at any time, and omapfb/omapdrm may miss that out.

True, omapdrm/fb may have a better idea. It's still unclear though.
Currently we have quite strict order in the sequence the modules need to
be loaded, which is quite bad and causes issues. We should make things
more dynamic, so that the initialization of the drivers could happen
more freely.

But that creates more problems: when booting up, omapfb starts. But
omapfb can't know if all the panel drivers have already been loaded.
omapfb may see that DVI is the default display, but what should it do if
DVI doesn't have a driver yet? It could wait, but perhaps the driver for
DVI will never even be loaded.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Rob Clark]

On 11/05/2012 02:55 AM, Tomi Valkeinen wrote:
>>> But even then, choosing the manager is not easy, as whoever chooses the
>>> >>manager needs to observe all the possible displays used at the same
>>> >>time...
>> >
>> >Right. I was wondering if omapfb/omapdrm could understand the 'all
>> >possible displays information' better compared to a panel's probe.
>> >
>> >Even omapdrm/omafb can't be perfect because we could insert a panel
>> >driver module at any time, and omapfb/omapdrm may miss that out.
> True, omapdrm/fb may have a better idea. It's still unclear though.
> Currently we have quite strict order in the sequence the modules need to
> be loaded, which is quite bad and causes issues. We should make things
> more dynamic, so that the initialization of the drivers could happen
> more freely.
>
> But that creates more problems: when booting up, omapfb starts. But
> omapfb can't know if all the panel drivers have already been loaded.
> omapfb may see that DVI is the default display, but what should it do if
> DVI doesn't have a driver yet? It could wait, but perhaps the driver for
> DVI will never even be loaded.

The encoder which is connected to the crtc (manager) is picked by 
combination of encoder->possible_crtcs bitmask and 
connector->best_encoder().  We could keep things limited so that the 
association of crtc to encoder (manager to output, roughly) never 
changes, but this isn't really the right thing to do.  It is better that 
the dssdev not rely on knowing the manager it is attached to at probe 
time, but instead grab resources more dynamically.

Also, at the moment we don't really have any notification to userspace 
about new encoders/connectors showing up (or conversely, being 
removed).  Only about existing connectors being plugged/unplugged.  The 
closest analogy is perhaps the USB display devices, but even there it is 
only the entire drm device that is plugged/unplugged.  And TBH I don't 
really see the point in supporting panel drivers being dynamically 
loaded.  It isn't like someone is dynamically soldering on a new display 
connector to some board that is running.  I think omapfb or omapdrm 
probe should trigger registering the compiled-in panel drivers, so that 
it can be sure that the dssdev's pop up before it goes and creates drm 
connector objects.  Currently we have to hack around this in omapdrm 
with late_initcall() to ensure the panel drivers are probed first, but 
that is an ugly hack that I'd like to get rid of.

BR,
-R

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Rob Clark]

On 11/05/2012 02:55 AM, Tomi Valkeinen wrote:
>>> But even then, choosing the manager is not easy, as whoever chooses the
>>> >>manager needs to observe all the possible displays used at the same
>>> >>time...
>> >
>> >Right. I was wondering if omapfb/omapdrm could understand the 'all
>> >possible displays information' better compared to a panel's probe.
>> >
>> >Even omapdrm/omafb can't be perfect because we could insert a panel
>> >driver module at any time, and omapfb/omapdrm may miss that out.
> True, omapdrm/fb may have a better idea. It's still unclear though.
> Currently we have quite strict order in the sequence the modules need to
> be loaded, which is quite bad and causes issues. We should make things
> more dynamic, so that the initialization of the drivers could happen
> more freely.
>
> But that creates more problems: when booting up, omapfb starts. But
> omapfb can't know if all the panel drivers have already been loaded.
> omapfb may see that DVI is the default display, but what should it do if
> DVI doesn't have a driver yet? It could wait, but perhaps the driver for
> DVI will never even be loaded.

The encoder which is connected to the crtc (manager) is picked by 
combination of encoder->possible_crtcs bitmask and 
connector->best_encoder().  We could keep things limited so that the 
association of crtc to encoder (manager to output, roughly) never 
changes, but this isn't really the right thing to do.  It is better that 
the dssdev not rely on knowing the manager it is attached to at probe 
time, but instead grab resources more dynamically.

Also, at the moment we don't really have any notification to userspace 
about new encoders/connectors showing up (or conversely, being 
removed).  Only about existing connectors being plugged/unplugged.  The 
closest analogy is perhaps the USB display devices, but even there it is 
only the entire drm device that is plugged/unplugged.  And TBH I don't 
really see the point in supporting panel drivers being dynamically 
loaded.  It isn't like someone is dynamically soldering on a new display 
connector to some board that is running.  I think omapfb or omapdrm 
probe should trigger registering the compiled-in panel drivers, so that 
it can be sure that the dssdev's pop up before it goes and creates drm 
connector objects.  Currently we have to hack around this in omapdrm 
with late_initcall() to ensure the panel drivers are probed first, but 
that is an ugly hack that I'd like to get rid of.

BR,
-R

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

On 2012-11-05 16:21, Rob Clark wrote:
> On 11/05/2012 02:55 AM, Tomi Valkeinen wrote:
>>>> But even then, choosing the manager is not easy, as whoever chooses the
>>>> >>manager needs to observe all the possible displays used at the same
>>>> >>time...
>>> >
>>> >Right. I was wondering if omapfb/omapdrm could understand the 'all
>>> >possible displays information' better compared to a panel's probe.
>>> >
>>> >Even omapdrm/omafb can't be perfect because we could insert a panel
>>> >driver module at any time, and omapfb/omapdrm may miss that out.
>> True, omapdrm/fb may have a better idea. It's still unclear though.
>> Currently we have quite strict order in the sequence the modules need to
>> be loaded, which is quite bad and causes issues. We should make things
>> more dynamic, so that the initialization of the drivers could happen
>> more freely.
>>
>> But that creates more problems: when booting up, omapfb starts. But
>> omapfb can't know if all the panel drivers have already been loaded.
>> omapfb may see that DVI is the default display, but what should it do if
>> DVI doesn't have a driver yet? It could wait, but perhaps the driver for
>> DVI will never even be loaded.
> 
> The encoder which is connected to the crtc (manager) is picked by
> combination of encoder->possible_crtcs bitmask and
> connector->best_encoder().  We could keep things limited so that the
> association of crtc to encoder (manager to output, roughly) never
> changes, but this isn't really the right thing to do.  It is better that
> the dssdev not rely on knowing the manager it is attached to at probe
> time, but instead grab resources more dynamically.
> 
> Also, at the moment we don't really have any notification to userspace
> about new encoders/connectors showing up (or conversely, being
> removed).  Only about existing connectors being plugged/unplugged.  The
> closest analogy is perhaps the USB display devices, but even there it is
> only the entire drm device that is plugged/unplugged.  And TBH I don't
> really see the point in supporting panel drivers being dynamically
> loaded.  It isn't like someone is dynamically soldering on a new display
> connector to some board that is running.  I think omapfb or omapdrm
> probe should trigger registering the compiled-in panel drivers, so that
> it can be sure that the dssdev's pop up before it goes and creates drm
> connector objects.  Currently we have to hack around this in omapdrm
> with late_initcall() to ensure the panel drivers are probed first, but
> that is an ugly hack that I'd like to get rid of.

We have panel devices and panel drivers, each of which can appear at any
time. Both are needed for the panel probe to happen. If we don't support
device hotplugging (dynamic creation of devices), we need to use
late_initcall for omapfb/drm. At least I don't see any other option.

You say that omapdrm should trigger registering of the drivers. How
would that work? Do you mean that the panel drivers would register
themselves to some common list, and omapdrm would go through this list
when drm is loaded, calling probe for the items in the list? I guess
that's doable, but... It's not how kernel drivers are supposed to work,
and so doesn't sound very clean approach to me.

I think we should support proper hotplugging of the panels. This would
fix the problem about init order, but it would also give us device
hotplug support. Obviously nobody is going to solder panel to a running
board, but I don't see any reason why panels, or, more likely, panels on
an add-on boards (like the capes being discussed in omap ml) would not
be hotpluggable using whatever connector is used on the particular use case.

And even if we don't support removing of the devices, things like the
add-on capes could cause the panel on the cape to be identified at some
late time (the panel is not described in the board file or DT data, but
found at runtime depending on the ID of the cape). This would add
another step to the init sequence that should be just right, if we don't
support hotplug.

Yes, I know it's not simple =). And I'm fine with simpler approach for
the time being, but I'd like full hotplug to be the future goal. At
least the common panel framework should not create restrictions about
this, even if drm wouldn't allow device hotplug.

 Tomi

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

On 2012-11-05 16:21, Rob Clark wrote:
> On 11/05/2012 02:55 AM, Tomi Valkeinen wrote:
>>>> But even then, choosing the manager is not easy, as whoever chooses the
>>>> >>manager needs to observe all the possible displays used at the same
>>>> >>time...
>>> >
>>> >Right. I was wondering if omapfb/omapdrm could understand the 'all
>>> >possible displays information' better compared to a panel's probe.
>>> >
>>> >Even omapdrm/omafb can't be perfect because we could insert a panel
>>> >driver module at any time, and omapfb/omapdrm may miss that out.
>> True, omapdrm/fb may have a better idea. It's still unclear though.
>> Currently we have quite strict order in the sequence the modules need to
>> be loaded, which is quite bad and causes issues. We should make things
>> more dynamic, so that the initialization of the drivers could happen
>> more freely.
>>
>> But that creates more problems: when booting up, omapfb starts. But
>> omapfb can't know if all the panel drivers have already been loaded.
>> omapfb may see that DVI is the default display, but what should it do if
>> DVI doesn't have a driver yet? It could wait, but perhaps the driver for
>> DVI will never even be loaded.
> 
> The encoder which is connected to the crtc (manager) is picked by
> combination of encoder->possible_crtcs bitmask and
> connector->best_encoder().  We could keep things limited so that the
> association of crtc to encoder (manager to output, roughly) never
> changes, but this isn't really the right thing to do.  It is better that
> the dssdev not rely on knowing the manager it is attached to at probe
> time, but instead grab resources more dynamically.
> 
> Also, at the moment we don't really have any notification to userspace
> about new encoders/connectors showing up (or conversely, being
> removed).  Only about existing connectors being plugged/unplugged.  The
> closest analogy is perhaps the USB display devices, but even there it is
> only the entire drm device that is plugged/unplugged.  And TBH I don't
> really see the point in supporting panel drivers being dynamically
> loaded.  It isn't like someone is dynamically soldering on a new display
> connector to some board that is running.  I think omapfb or omapdrm
> probe should trigger registering the compiled-in panel drivers, so that
> it can be sure that the dssdev's pop up before it goes and creates drm
> connector objects.  Currently we have to hack around this in omapdrm
> with late_initcall() to ensure the panel drivers are probed first, but
> that is an ugly hack that I'd like to get rid of.

We have panel devices and panel drivers, each of which can appear at any
time. Both are needed for the panel probe to happen. If we don't support
device hotplugging (dynamic creation of devices), we need to use
late_initcall for omapfb/drm. At least I don't see any other option.

You say that omapdrm should trigger registering of the drivers. How
would that work? Do you mean that the panel drivers would register
themselves to some common list, and omapdrm would go through this list
when drm is loaded, calling probe for the items in the list? I guess
that's doable, but... It's not how kernel drivers are supposed to work,
and so doesn't sound very clean approach to me.

I think we should support proper hotplugging of the panels. This would
fix the problem about init order, but it would also give us device
hotplug support. Obviously nobody is going to solder panel to a running
board, but I don't see any reason why panels, or, more likely, panels on
an add-on boards (like the capes being discussed in omap ml) would not
be hotpluggable using whatever connector is used on the particular use case.

And even if we don't support removing of the devices, things like the
add-on capes could cause the panel on the cape to be identified at some
late time (the panel is not described in the board file or DT data, but
found at runtime depending on the ID of the cape). This would add
another step to the init sequence that should be just right, if we don't
support hotplug.

Yes, I know it's not simple =). And I'm fine with simpler approach for
the time being, but I'd like full hotplug to be the future goal. At
least the common panel framework should not create restrictions about
this, even if drm wouldn't allow device hotplug.

 Tomi

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Rob Clark]

On Tue, Nov 6, 2012 at 7:41 AM, Tomi Valkeinen <tomba@iki.fi> wrote:
> On 2012-11-05 16:21, Rob Clark wrote:
>> On 11/05/2012 02:55 AM, Tomi Valkeinen wrote:
>>>>> But even then, choosing the manager is not easy, as whoever chooses the
>>>>> >>manager needs to observe all the possible displays used at the same
>>>>> >>time...
>>>> >
>>>> >Right. I was wondering if omapfb/omapdrm could understand the 'all
>>>> >possible displays information' better compared to a panel's probe.
>>>> >
>>>> >Even omapdrm/omafb can't be perfect because we could insert a panel
>>>> >driver module at any time, and omapfb/omapdrm may miss that out.
>>> True, omapdrm/fb may have a better idea. It's still unclear though.
>>> Currently we have quite strict order in the sequence the modules need to
>>> be loaded, which is quite bad and causes issues. We should make things
>>> more dynamic, so that the initialization of the drivers could happen
>>> more freely.
>>>
>>> But that creates more problems: when booting up, omapfb starts. But
>>> omapfb can't know if all the panel drivers have already been loaded.
>>> omapfb may see that DVI is the default display, but what should it do if
>>> DVI doesn't have a driver yet? It could wait, but perhaps the driver for
>>> DVI will never even be loaded.
>>
>> The encoder which is connected to the crtc (manager) is picked by
>> combination of encoder->possible_crtcs bitmask and
>> connector->best_encoder().  We could keep things limited so that the
>> association of crtc to encoder (manager to output, roughly) never
>> changes, but this isn't really the right thing to do.  It is better that
>> the dssdev not rely on knowing the manager it is attached to at probe
>> time, but instead grab resources more dynamically.
>>
>> Also, at the moment we don't really have any notification to userspace
>> about new encoders/connectors showing up (or conversely, being
>> removed).  Only about existing connectors being plugged/unplugged.  The
>> closest analogy is perhaps the USB display devices, but even there it is
>> only the entire drm device that is plugged/unplugged.  And TBH I don't
>> really see the point in supporting panel drivers being dynamically
>> loaded.  It isn't like someone is dynamically soldering on a new display
>> connector to some board that is running.  I think omapfb or omapdrm
>> probe should trigger registering the compiled-in panel drivers, so that
>> it can be sure that the dssdev's pop up before it goes and creates drm
>> connector objects.  Currently we have to hack around this in omapdrm
>> with late_initcall() to ensure the panel drivers are probed first, but
>> that is an ugly hack that I'd like to get rid of.
>
> We have panel devices and panel drivers, each of which can appear at any
> time. Both are needed for the panel probe to happen. If we don't support
> device hotplugging (dynamic creation of devices), we need to use
> late_initcall for omapfb/drm. At least I don't see any other option.
>
> You say that omapdrm should trigger registering of the drivers. How
> would that work? Do you mean that the panel drivers would register
> themselves to some common list, and omapdrm would go through this list
> when drm is loaded, calling probe for the items in the list? I guess
> that's doable, but... It's not how kernel drivers are supposed to work,
> and so doesn't sound very clean approach to me.

I mean, similar to how we handle the subdev for dmm..  the
omap_drm_init() does the platform_driver_register() for the dmm device
before the platform_driver_register() for omapdrm itself, so we know
if there is a dmm device, the driver gets probed first before omapdrm.

It could be a matter of iterating through a list, or something like
this.. that is basically an implementation detail.  But the end result
is that the order the drivers are registered is controlled so the
probe sequence works out properly (not to mention suspend/resume
sequence).

> I think we should support proper hotplugging of the panels. This would
> fix the problem about init order, but it would also give us device
> hotplug support. Obviously nobody is going to solder panel to a running
> board, but I don't see any reason why panels, or, more likely, panels on
> an add-on boards (like the capes being discussed in omap ml) would not
> be hotpluggable using whatever connector is used on the particular use case.
>
> And even if we don't support removing of the devices, things like the
> add-on capes could cause the panel on the cape to be identified at some
> late time (the panel is not described in the board file or DT data, but
> found at runtime depending on the ID of the cape). This would add
> another step to the init sequence that should be just right, if we don't
> support hotplug.

If capes are really hot-pluggable, then maybe it is worth thinking
about how to make this more dynamic.  Although it is a bigger problem,
which involves userspace being aware that connectors can dynamically
appear/disappear.  And the dynamic disappearing is something I worry
about more.. it adds the possibility of all sorts of interesting race
conditions, such as connectors disappearing in the middle of modeset.
I prefer not making things more complicated and error prone than they
need to be.  If there is not a legitimate use case for connector hw
dynamically appearing/disappearing then I don't think we should go
there.  It sounds nice and simple and clean, but in reality I think it
just introduces a whole lot of ways for things to go wrong.  A wise
man once said:

https://github.com/robclark/kernel-omap4/blob/master/Documentation/SubmittingPatches#L700

BR,
-R

> Yes, I know it's not simple =). And I'm fine with simpler approach for
> the time being, but I'd like full hotplug to be the future goal. At
> least the common panel framework should not create restrictions about
> this, even if drm wouldn't allow device hotplug.
>
>  Tomi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Rob Clark]

On Tue, Nov 6, 2012 at 7:41 AM, Tomi Valkeinen <tomba@iki.fi> wrote:
> On 2012-11-05 16:21, Rob Clark wrote:
>> On 11/05/2012 02:55 AM, Tomi Valkeinen wrote:
>>>>> But even then, choosing the manager is not easy, as whoever chooses the
>>>>> >>manager needs to observe all the possible displays used at the same
>>>>> >>time...
>>>> >
>>>> >Right. I was wondering if omapfb/omapdrm could understand the 'all
>>>> >possible displays information' better compared to a panel's probe.
>>>> >
>>>> >Even omapdrm/omafb can't be perfect because we could insert a panel
>>>> >driver module at any time, and omapfb/omapdrm may miss that out.
>>> True, omapdrm/fb may have a better idea. It's still unclear though.
>>> Currently we have quite strict order in the sequence the modules need to
>>> be loaded, which is quite bad and causes issues. We should make things
>>> more dynamic, so that the initialization of the drivers could happen
>>> more freely.
>>>
>>> But that creates more problems: when booting up, omapfb starts. But
>>> omapfb can't know if all the panel drivers have already been loaded.
>>> omapfb may see that DVI is the default display, but what should it do if
>>> DVI doesn't have a driver yet? It could wait, but perhaps the driver for
>>> DVI will never even be loaded.
>>
>> The encoder which is connected to the crtc (manager) is picked by
>> combination of encoder->possible_crtcs bitmask and
>> connector->best_encoder().  We could keep things limited so that the
>> association of crtc to encoder (manager to output, roughly) never
>> changes, but this isn't really the right thing to do.  It is better that
>> the dssdev not rely on knowing the manager it is attached to at probe
>> time, but instead grab resources more dynamically.
>>
>> Also, at the moment we don't really have any notification to userspace
>> about new encoders/connectors showing up (or conversely, being
>> removed).  Only about existing connectors being plugged/unplugged.  The
>> closest analogy is perhaps the USB display devices, but even there it is
>> only the entire drm device that is plugged/unplugged.  And TBH I don't
>> really see the point in supporting panel drivers being dynamically
>> loaded.  It isn't like someone is dynamically soldering on a new display
>> connector to some board that is running.  I think omapfb or omapdrm
>> probe should trigger registering the compiled-in panel drivers, so that
>> it can be sure that the dssdev's pop up before it goes and creates drm
>> connector objects.  Currently we have to hack around this in omapdrm
>> with late_initcall() to ensure the panel drivers are probed first, but
>> that is an ugly hack that I'd like to get rid of.
>
> We have panel devices and panel drivers, each of which can appear at any
> time. Both are needed for the panel probe to happen. If we don't support
> device hotplugging (dynamic creation of devices), we need to use
> late_initcall for omapfb/drm. At least I don't see any other option.
>
> You say that omapdrm should trigger registering of the drivers. How
> would that work? Do you mean that the panel drivers would register
> themselves to some common list, and omapdrm would go through this list
> when drm is loaded, calling probe for the items in the list? I guess
> that's doable, but... It's not how kernel drivers are supposed to work,
> and so doesn't sound very clean approach to me.

I mean, similar to how we handle the subdev for dmm..  the
omap_drm_init() does the platform_driver_register() for the dmm device
before the platform_driver_register() for omapdrm itself, so we know
if there is a dmm device, the driver gets probed first before omapdrm.

It could be a matter of iterating through a list, or something like
this.. that is basically an implementation detail.  But the end result
is that the order the drivers are registered is controlled so the
probe sequence works out properly (not to mention suspend/resume
sequence).

> I think we should support proper hotplugging of the panels. This would
> fix the problem about init order, but it would also give us device
> hotplug support. Obviously nobody is going to solder panel to a running
> board, but I don't see any reason why panels, or, more likely, panels on
> an add-on boards (like the capes being discussed in omap ml) would not
> be hotpluggable using whatever connector is used on the particular use case.
>
> And even if we don't support removing of the devices, things like the
> add-on capes could cause the panel on the cape to be identified at some
> late time (the panel is not described in the board file or DT data, but
> found at runtime depending on the ID of the cape). This would add
> another step to the init sequence that should be just right, if we don't
> support hotplug.

If capes are really hot-pluggable, then maybe it is worth thinking
about how to make this more dynamic.  Although it is a bigger problem,
which involves userspace being aware that connectors can dynamically
appear/disappear.  And the dynamic disappearing is something I worry
about more.. it adds the possibility of all sorts of interesting race
conditions, such as connectors disappearing in the middle of modeset.
I prefer not making things more complicated and error prone than they
need to be.  If there is not a legitimate use case for connector hw
dynamically appearing/disappearing then I don't think we should go
there.  It sounds nice and simple and clean, but in reality I think it
just introduces a whole lot of ways for things to go wrong.  A wise
man once said:

https://github.com/robclark/kernel-omap4/blob/master/Documentation/SubmittingPatches#L700

BR,
-R

> Yes, I know it's not simple =). And I'm fine with simpler approach for
> the time being, but I'd like full hotplug to be the future goal. At
> least the common panel framework should not create restrictions about
> this, even if drm wouldn't allow device hotplug.
>
>  Tomi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5101 bytes --]

On 2012-11-06 16:40, Rob Clark wrote:

> I mean, similar to how we handle the subdev for dmm..  the
> omap_drm_init() does the platform_driver_register() for the dmm device
> before the platform_driver_register() for omapdrm itself, so we know
> if there is a dmm device, the driver gets probed first before omapdrm.

Well, I consider that a bit hacky too. That's not how linux device
framework is supposed to be used. I know it makes life easier to do the
registering like that, though.

> It could be a matter of iterating through a list, or something like
> this.. that is basically an implementation detail.  But the end result
> is that the order the drivers are registered is controlled so the
> probe sequence works out properly (not to mention suspend/resume
> sequence).

I feel that this kind of solution just tries to solve the generic
problem of init/suspend ordering in a single driver, instead of fixing
the device framework itself.

Or, of course it's possible that our drive architecture just sucks, and
the device framework is fine. In that case the workaround is even worse,
and we should fix our drivers.

>> I think we should support proper hotplugging of the panels. This would
>> fix the problem about init order, but it would also give us device
>> hotplug support. Obviously nobody is going to solder panel to a running
>> board, but I don't see any reason why panels, or, more likely, panels on
>> an add-on boards (like the capes being discussed in omap ml) would not
>> be hotpluggable using whatever connector is used on the particular use case.
>>
>> And even if we don't support removing of the devices, things like the
>> add-on capes could cause the panel on the cape to be identified at some
>> late time (the panel is not described in the board file or DT data, but
>> found at runtime depending on the ID of the cape). This would add
>> another step to the init sequence that should be just right, if we don't
>> support hotplug.
> 
> If capes are really hot-pluggable, then maybe it is worth thinking
> about how to make this more dynamic.  Although it is a bigger problem,
> which involves userspace being aware that connectors can dynamically
> appear/disappear.  And the dynamic disappearing is something I worry
> about more.. it adds the possibility of all sorts of interesting race
> conditions, such as connectors disappearing in the middle of modeset.
> I prefer not making things more complicated and error prone than they
> need to be.  If there is not a legitimate use case for connector hw
> dynamically appearing/disappearing then I don't think we should go
> there.  It sounds nice and simple and clean, but in reality I think it
> just introduces a whole lot of ways for things to go wrong.  A wise

Yes, I agree that it complicates things.

> man once said:
> 
> https://github.com/robclark/kernel-omap4/blob/master/Documentation/SubmittingPatches#L700

I've done things simple lots of times in the omapdss driver, only to
have to rewrite the thing in more complex way later to accommodate new
scenarios. I think it's good to write the code in a bit more generic way
than the use case at the moment of writing requires, because more often
than not, it'll save time in the future.

Hotplugging is not some abstract future scenario, we already have
hardware that could use it. For example, omap3 SDP board has a
switchable output to DVI or LCD panel. In this case we know what the two
options are, but the disabled component is still effectually removed
from the system, and plugged back in when it's enabled.

Hotplug is not a high priority item, but I do wish we get it supported
in common panel framework. Then it's at least possible to extend drm in
the future to support it.



Anyway, this makes me wonder... omapdrm currently maps the elements of
the whole video pipeline to drm elements (encoder, connector, etc).
Would it make more sense to just map the DISPC to these drm elements?
Connector would then be the output from DISPC.

This would map the drm elements to the static hardware blocks, and the
meaning of those blocks would be quite similar to what they are in the
desktop world (I guess).

The panel driver, the external chips, and the DSS internal output blocks
(dsi, dpi, ...) would be handled separately from those drm elements. The
DSS internal blocks are static, of course, but they can be effectively
considered the same way as external chips.

The omapdrm driver needs of course to access those separate elements
also, but that shouldn't be a problem. If omapdrm needs to call a
function in the panel driver, all it needs to do is go through the chain
to find the panel. Well, except if one output connected two two panels
via a bridge chip...

And if drm is at some point extended to support panel drivers, or chains
of external display entities, it would be easier to add that support.

What would it require the manage the elements like that? Would it help?
It sounds to me that this would simplify the model.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5101 bytes --]

On 2012-11-06 16:40, Rob Clark wrote:

> I mean, similar to how we handle the subdev for dmm..  the
> omap_drm_init() does the platform_driver_register() for the dmm device
> before the platform_driver_register() for omapdrm itself, so we know
> if there is a dmm device, the driver gets probed first before omapdrm.

Well, I consider that a bit hacky too. That's not how linux device
framework is supposed to be used. I know it makes life easier to do the
registering like that, though.

> It could be a matter of iterating through a list, or something like
> this.. that is basically an implementation detail.  But the end result
> is that the order the drivers are registered is controlled so the
> probe sequence works out properly (not to mention suspend/resume
> sequence).

I feel that this kind of solution just tries to solve the generic
problem of init/suspend ordering in a single driver, instead of fixing
the device framework itself.

Or, of course it's possible that our drive architecture just sucks, and
the device framework is fine. In that case the workaround is even worse,
and we should fix our drivers.

>> I think we should support proper hotplugging of the panels. This would
>> fix the problem about init order, but it would also give us device
>> hotplug support. Obviously nobody is going to solder panel to a running
>> board, but I don't see any reason why panels, or, more likely, panels on
>> an add-on boards (like the capes being discussed in omap ml) would not
>> be hotpluggable using whatever connector is used on the particular use case.
>>
>> And even if we don't support removing of the devices, things like the
>> add-on capes could cause the panel on the cape to be identified at some
>> late time (the panel is not described in the board file or DT data, but
>> found at runtime depending on the ID of the cape). This would add
>> another step to the init sequence that should be just right, if we don't
>> support hotplug.
> 
> If capes are really hot-pluggable, then maybe it is worth thinking
> about how to make this more dynamic.  Although it is a bigger problem,
> which involves userspace being aware that connectors can dynamically
> appear/disappear.  And the dynamic disappearing is something I worry
> about more.. it adds the possibility of all sorts of interesting race
> conditions, such as connectors disappearing in the middle of modeset.
> I prefer not making things more complicated and error prone than they
> need to be.  If there is not a legitimate use case for connector hw
> dynamically appearing/disappearing then I don't think we should go
> there.  It sounds nice and simple and clean, but in reality I think it
> just introduces a whole lot of ways for things to go wrong.  A wise

Yes, I agree that it complicates things.

> man once said:
> 
> https://github.com/robclark/kernel-omap4/blob/master/Documentation/SubmittingPatches#L700

I've done things simple lots of times in the omapdss driver, only to
have to rewrite the thing in more complex way later to accommodate new
scenarios. I think it's good to write the code in a bit more generic way
than the use case at the moment of writing requires, because more often
than not, it'll save time in the future.

Hotplugging is not some abstract future scenario, we already have
hardware that could use it. For example, omap3 SDP board has a
switchable output to DVI or LCD panel. In this case we know what the two
options are, but the disabled component is still effectually removed
from the system, and plugged back in when it's enabled.

Hotplug is not a high priority item, but I do wish we get it supported
in common panel framework. Then it's at least possible to extend drm in
the future to support it.



Anyway, this makes me wonder... omapdrm currently maps the elements of
the whole video pipeline to drm elements (encoder, connector, etc).
Would it make more sense to just map the DISPC to these drm elements?
Connector would then be the output from DISPC.

This would map the drm elements to the static hardware blocks, and the
meaning of those blocks would be quite similar to what they are in the
desktop world (I guess).

The panel driver, the external chips, and the DSS internal output blocks
(dsi, dpi, ...) would be handled separately from those drm elements. The
DSS internal blocks are static, of course, but they can be effectively
considered the same way as external chips.

The omapdrm driver needs of course to access those separate elements
also, but that shouldn't be a problem. If omapdrm needs to call a
function in the panel driver, all it needs to do is go through the chain
to find the panel. Well, except if one output connected two two panels
via a bridge chip...

And if drm is at some point extended to support panel drivers, or chains
of external display entities, it would be easier to add that support.

What would it require the manage the elements like that? Would it help?
It sounds to me that this would simplify the model.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Rob Clark]

On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 2012-11-06 16:40, Rob Clark wrote:
>
>> I mean, similar to how we handle the subdev for dmm..  the
>> omap_drm_init() does the platform_driver_register() for the dmm device
>> before the platform_driver_register() for omapdrm itself, so we know
>> if there is a dmm device, the driver gets probed first before omapdrm.
>
> Well, I consider that a bit hacky too. That's not how linux device
> framework is supposed to be used. I know it makes life easier to do the
> registering like that, though.
>
>> It could be a matter of iterating through a list, or something like
>> this.. that is basically an implementation detail.  But the end result
>> is that the order the drivers are registered is controlled so the
>> probe sequence works out properly (not to mention suspend/resume
>> sequence).
>
> I feel that this kind of solution just tries to solve the generic
> problem of init/suspend ordering in a single driver, instead of fixing
> the device framework itself.
>
> Or, of course it's possible that our drive architecture just sucks, and
> the device framework is fine. In that case the workaround is even worse,
> and we should fix our drivers.

well, I guess by splitting things up into many pieces, we push the
device framework in ways that it is not pushed on desktop.  But I've
enough on my plate so I wasn't going to volunteer to try and fix that
;-)

controlling the order that the drivers are registered is currently the
most straightforward way to not have problems w/ probe/init/suspend..

>>> I think we should support proper hotplugging of the panels. This would
>>> fix the problem about init order, but it would also give us device
>>> hotplug support. Obviously nobody is going to solder panel to a running
>>> board, but I don't see any reason why panels, or, more likely, panels on
>>> an add-on boards (like the capes being discussed in omap ml) would not
>>> be hotpluggable using whatever connector is used on the particular use case.
>>>
>>> And even if we don't support removing of the devices, things like the
>>> add-on capes could cause the panel on the cape to be identified at some
>>> late time (the panel is not described in the board file or DT data, but
>>> found at runtime depending on the ID of the cape). This would add
>>> another step to the init sequence that should be just right, if we don't
>>> support hotplug.
>>
>> If capes are really hot-pluggable, then maybe it is worth thinking
>> about how to make this more dynamic.  Although it is a bigger problem,
>> which involves userspace being aware that connectors can dynamically
>> appear/disappear.  And the dynamic disappearing is something I worry
>> about more.. it adds the possibility of all sorts of interesting race
>> conditions, such as connectors disappearing in the middle of modeset.
>> I prefer not making things more complicated and error prone than they
>> need to be.  If there is not a legitimate use case for connector hw
>> dynamically appearing/disappearing then I don't think we should go
>> there.  It sounds nice and simple and clean, but in reality I think it
>> just introduces a whole lot of ways for things to go wrong.  A wise
>
> Yes, I agree that it complicates things.
>
>> man once said:
>>
>> https://github.com/robclark/kernel-omap4/blob/master/Documentation/SubmittingPatches#L700
>
> I've done things simple lots of times in the omapdss driver, only to
> have to rewrite the thing in more complex way later to accommodate new
> scenarios. I think it's good to write the code in a bit more generic way
> than the use case at the moment of writing requires, because more often
> than not, it'll save time in the future.

well, if it is not effecting userspace ABI, then I think, when in
doubt I prefer to start simple.  It can always be changed later if
needed.

> Hotplugging is not some abstract future scenario, we already have
> hardware that could use it. For example, omap3 SDP board has a
> switchable output to DVI or LCD panel. In this case we know what the two
> options are, but the disabled component is still effectually removed
> from the system, and plugged back in when it's enabled.

I would look at this as two different connectors which can not be used
at the same time.  You have this scenario with desktop graphics cards.

> Hotplug is not a high priority item, but I do wish we get it supported
> in common panel framework. Then it's at least possible to extend drm in
> the future to support it.
>
>
>
> Anyway, this makes me wonder... omapdrm currently maps the elements of
> the whole video pipeline to drm elements (encoder, connector, etc).
> Would it make more sense to just map the DISPC to these drm elements?
> Connector would then be the output from DISPC.

I think:

  plane->overlay
  crtc->manager

is pretty clear.  And really

  encoder->output

should be the way it is..  on the branch w/ omapdss/omapdrm kms
re-write, this is how it is for plane/crtc, except for now:

  encoder+connector->dssdev

Basically the encoder is doing the "control" stuff (power on/off, set
timings, etc), and the connector is only doing non control stuff
(detect, reading edid, etc).

But I think this will probably change a bit as CFP comes into the
picture.  Currently the drm connector is somewhat a "passive" element,
but I think this will have to change a bit w/ CFP.

> This would map the drm elements to the static hardware blocks, and the
> meaning of those blocks would be quite similar to what they are in the
> desktop world (I guess).
>
> The panel driver, the external chips, and the DSS internal output blocks
> (dsi, dpi, ...) would be handled separately from those drm elements. The
> DSS internal blocks are static, of course, but they can be effectively
> considered the same way as external chips.

I think dsi/dpi/etc map to encoder.  The big question is where the
panel's fit.  But to userspace somehow this should look like
connectors.  I think:

  encoder->output
  connector->panel

could work.. although connector is less passive than KMS currently
assumes.  And "panel" could really be a whole chain in the case of
bridge chips, etc.  I don't know, maybe there are better ways.  But I
think userspace really just wants to know "which monitor" which is
basically connector.

> The omapdrm driver needs of course to access those separate elements
> also, but that shouldn't be a problem. If omapdrm needs to call a
> function in the panel driver, all it needs to do is go through the chain
> to find the panel. Well, except if one output connected two two panels
> via a bridge chip...

yeah, that is a really ugly case in our hw since it is quite
non-transparent (ie. implications about use of planes, etc).

> And if drm is at some point extended to support panel drivers, or chains
> of external display entities, it would be easier to add that support.
>
> What would it require the manage the elements like that? Would it help?
> It sounds to me that this would simplify the model.

I'm not really entirely sure..  other than at least other drivers
supporting CFP will have the same requirements ;-)

I guess the two best options are either bury some sort of chain of
panel drivers in the connector, or introduce some internal elements in
DRM which are not necessarily visible to userspace.  (Or at least
userspace should have the option to ignore it for backwards
compatibility.  For atomic pageflip/modeset, the converting of
everything to properties makes it easier to think about exposing new
KMS mode object types to userspace.)

Either way, I think we need to break this into stages.  And first
stage is to ignore chains of panel drivers and just support single
panel drivers (ignoring bridges, etc).  I'm not entirely sure what the
second stage should be, but am open to suggestions and seeing how it
evolves.

BR,
-R

>  Tomi
>
>

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Rob Clark]

On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 2012-11-06 16:40, Rob Clark wrote:
>
>> I mean, similar to how we handle the subdev for dmm..  the
>> omap_drm_init() does the platform_driver_register() for the dmm device
>> before the platform_driver_register() for omapdrm itself, so we know
>> if there is a dmm device, the driver gets probed first before omapdrm.
>
> Well, I consider that a bit hacky too. That's not how linux device
> framework is supposed to be used. I know it makes life easier to do the
> registering like that, though.
>
>> It could be a matter of iterating through a list, or something like
>> this.. that is basically an implementation detail.  But the end result
>> is that the order the drivers are registered is controlled so the
>> probe sequence works out properly (not to mention suspend/resume
>> sequence).
>
> I feel that this kind of solution just tries to solve the generic
> problem of init/suspend ordering in a single driver, instead of fixing
> the device framework itself.
>
> Or, of course it's possible that our drive architecture just sucks, and
> the device framework is fine. In that case the workaround is even worse,
> and we should fix our drivers.

well, I guess by splitting things up into many pieces, we push the
device framework in ways that it is not pushed on desktop.  But I've
enough on my plate so I wasn't going to volunteer to try and fix that
;-)

controlling the order that the drivers are registered is currently the
most straightforward way to not have problems w/ probe/init/suspend..

>>> I think we should support proper hotplugging of the panels. This would
>>> fix the problem about init order, but it would also give us device
>>> hotplug support. Obviously nobody is going to solder panel to a running
>>> board, but I don't see any reason why panels, or, more likely, panels on
>>> an add-on boards (like the capes being discussed in omap ml) would not
>>> be hotpluggable using whatever connector is used on the particular use case.
>>>
>>> And even if we don't support removing of the devices, things like the
>>> add-on capes could cause the panel on the cape to be identified at some
>>> late time (the panel is not described in the board file or DT data, but
>>> found at runtime depending on the ID of the cape). This would add
>>> another step to the init sequence that should be just right, if we don't
>>> support hotplug.
>>
>> If capes are really hot-pluggable, then maybe it is worth thinking
>> about how to make this more dynamic.  Although it is a bigger problem,
>> which involves userspace being aware that connectors can dynamically
>> appear/disappear.  And the dynamic disappearing is something I worry
>> about more.. it adds the possibility of all sorts of interesting race
>> conditions, such as connectors disappearing in the middle of modeset.
>> I prefer not making things more complicated and error prone than they
>> need to be.  If there is not a legitimate use case for connector hw
>> dynamically appearing/disappearing then I don't think we should go
>> there.  It sounds nice and simple and clean, but in reality I think it
>> just introduces a whole lot of ways for things to go wrong.  A wise
>
> Yes, I agree that it complicates things.
>
>> man once said:
>>
>> https://github.com/robclark/kernel-omap4/blob/master/Documentation/SubmittingPatches#L700
>
> I've done things simple lots of times in the omapdss driver, only to
> have to rewrite the thing in more complex way later to accommodate new
> scenarios. I think it's good to write the code in a bit more generic way
> than the use case at the moment of writing requires, because more often
> than not, it'll save time in the future.

well, if it is not effecting userspace ABI, then I think, when in
doubt I prefer to start simple.  It can always be changed later if
needed.

> Hotplugging is not some abstract future scenario, we already have
> hardware that could use it. For example, omap3 SDP board has a
> switchable output to DVI or LCD panel. In this case we know what the two
> options are, but the disabled component is still effectually removed
> from the system, and plugged back in when it's enabled.

I would look at this as two different connectors which can not be used
at the same time.  You have this scenario with desktop graphics cards.

> Hotplug is not a high priority item, but I do wish we get it supported
> in common panel framework. Then it's at least possible to extend drm in
> the future to support it.
>
>
>
> Anyway, this makes me wonder... omapdrm currently maps the elements of
> the whole video pipeline to drm elements (encoder, connector, etc).
> Would it make more sense to just map the DISPC to these drm elements?
> Connector would then be the output from DISPC.

I think:

  plane->overlay
  crtc->manager

is pretty clear.  And really

  encoder->output

should be the way it is..  on the branch w/ omapdss/omapdrm kms
re-write, this is how it is for plane/crtc, except for now:

  encoder+connector->dssdev

Basically the encoder is doing the "control" stuff (power on/off, set
timings, etc), and the connector is only doing non control stuff
(detect, reading edid, etc).

But I think this will probably change a bit as CFP comes into the
picture.  Currently the drm connector is somewhat a "passive" element,
but I think this will have to change a bit w/ CFP.

> This would map the drm elements to the static hardware blocks, and the
> meaning of those blocks would be quite similar to what they are in the
> desktop world (I guess).
>
> The panel driver, the external chips, and the DSS internal output blocks
> (dsi, dpi, ...) would be handled separately from those drm elements. The
> DSS internal blocks are static, of course, but they can be effectively
> considered the same way as external chips.

I think dsi/dpi/etc map to encoder.  The big question is where the
panel's fit.  But to userspace somehow this should look like
connectors.  I think:

  encoder->output
  connector->panel

could work.. although connector is less passive than KMS currently
assumes.  And "panel" could really be a whole chain in the case of
bridge chips, etc.  I don't know, maybe there are better ways.  But I
think userspace really just wants to know "which monitor" which is
basically connector.

> The omapdrm driver needs of course to access those separate elements
> also, but that shouldn't be a problem. If omapdrm needs to call a
> function in the panel driver, all it needs to do is go through the chain
> to find the panel. Well, except if one output connected two two panels
> via a bridge chip...

yeah, that is a really ugly case in our hw since it is quite
non-transparent (ie. implications about use of planes, etc).

> And if drm is at some point extended to support panel drivers, or chains
> of external display entities, it would be easier to add that support.
>
> What would it require the manage the elements like that? Would it help?
> It sounds to me that this would simplify the model.

I'm not really entirely sure..  other than at least other drivers
supporting CFP will have the same requirements ;-)

I guess the two best options are either bury some sort of chain of
panel drivers in the connector, or introduce some internal elements in
DRM which are not necessarily visible to userspace.  (Or at least
userspace should have the option to ignore it for backwards
compatibility.  For atomic pageflip/modeset, the converting of
everything to properties makes it easier to think about exposing new
KMS mode object types to userspace.)

Either way, I think we need to break this into stages.  And first
stage is to ignore chains of panel drivers and just support single
panel drivers (ignoring bridges, etc).  I'm not entirely sure what the
second stage should be, but am open to suggestions and seeing how it
evolves.

BR,
-R

>  Tomi
>
>

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 6815 bytes --]

On 2012-11-07 16:32, Rob Clark wrote:
> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

>> Hotplugging is not some abstract future scenario, we already have
>> hardware that could use it. For example, omap3 SDP board has a
>> switchable output to DVI or LCD panel. In this case we know what the two
>> options are, but the disabled component is still effectually removed
>> from the system, and plugged back in when it's enabled.
> 
> I would look at this as two different connectors which can not be used
> at the same time.  You have this scenario with desktop graphics cards.

Yes, that's an option with fixed amount of display devices. But doesn't
work for capes.

>> Hotplug is not a high priority item, but I do wish we get it supported
>> in common panel framework. Then it's at least possible to extend drm in
>> the future to support it.
>>
>>
>>
>> Anyway, this makes me wonder... omapdrm currently maps the elements of
>> the whole video pipeline to drm elements (encoder, connector, etc).
>> Would it make more sense to just map the DISPC to these drm elements?
>> Connector would then be the output from DISPC.
> 
> I think:
> 
>   plane->overlay
>   crtc->manager
> 
> is pretty clear.  And really
> 
>   encoder->output
> 
> should be the way it is..  on the branch w/ omapdss/omapdrm kms

I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
chain. The primary "output" is in the DISPC module, the overlay manager.
That's where the timings, pixel clock, etc. are programmed. The second
step, our output, is really a converter IP. It receives the primary
output, converts it and outputs something else. Just like an external
converter chip would do.

And the output can be quite a bit anything. For example, with DBI or DSI
command mode outputs we don't have any of the conventional video
timings. It doesn't make sense to program, say, video blanking periods
to those outputs. But even with DBI and DSI we do have video blanking
periods in the DISPC's output, the ovl mgr.

Of course, at the end of the chain we have a panel that uses normal
video timings (well, most likely but not necessarily), and so we could
program those timings at the end of the chain, in the block before the
panel. But even then the encoder doesn't really map to the DSS's output
block, as the DSS's output block may not have the conventional timings
(like DBI), or they may be something totally different than what we get
in the end of the chain to the panel.

So I think mapping encoder to output will not work with multiple display
blocks in a chain. Thus I'd see the encoder would better match the
DISPC's output, or alternatively perhaps the block which is just before
the panel (whatever that is, sometimes it can be OMAP's DSI/HDMI/etc).
However, the latter may be a bit strange as the block could be an
external component, possibly hotpluggable.

> re-write, this is how it is for plane/crtc, except for now:
> 
>   encoder+connector->dssdev
> 
> Basically the encoder is doing the "control" stuff (power on/off, set
> timings, etc), and the connector is only doing non control stuff
> (detect, reading edid, etc).
> 
> But I think this will probably change a bit as CFP comes into the
> picture.  Currently the drm connector is somewhat a "passive" element,
> but I think this will have to change a bit w/ CFP.
> 
>> This would map the drm elements to the static hardware blocks, and the
>> meaning of those blocks would be quite similar to what they are in the
>> desktop world (I guess).
>>
>> The panel driver, the external chips, and the DSS internal output blocks
>> (dsi, dpi, ...) would be handled separately from those drm elements. The
>> DSS internal blocks are static, of course, but they can be effectively
>> considered the same way as external chips.
> 
> I think dsi/dpi/etc map to encoder.  The big question is where the
> panel's fit.  But to userspace somehow this should look like
> connectors.  I think:
> 
>   encoder->output
>   connector->panel
> 
> could work.. although connector is less passive than KMS currently
> assumes.  And "panel" could really be a whole chain in the case of
> bridge chips, etc.  I don't know, maybe there are better ways.  But I
> think userspace really just wants to know "which monitor" which is
> basically connector.

Hmm yes. Well, even if we map encoder and connector to the ovl manager,
the userspace could see which monitor there is. Obviously we need to
make changes for that to work, but as a model it feels a lot more
natural to me than using output and panel for encoder and connector.

Perhaps it's wrong to say "map connector to ovl mgr". It would be more
like "this connector observes the chain connected to this ovl mgr", even
though the connector wouldn't observe any block in the chain directly.
Just observing the plug in/out status, etc.

But I think encoder really maps quite well directly to the output side
of overlay manager.

>> The omapdrm driver needs of course to access those separate elements
>> also, but that shouldn't be a problem. If omapdrm needs to call a
>> function in the panel driver, all it needs to do is go through the chain
>> to find the panel. Well, except if one output connected two two panels
>> via a bridge chip...
> 
> yeah, that is a really ugly case in our hw since it is quite
> non-transparent (ie. implications about use of planes, etc).

Not really in this case. You're perhaps thinking about connecting two
outputs to a single panel. Which is problematic also.

We don't have in sights a board that splits one output to two panels, so
I think we should just ignore that for now. But two outputs for one
panel is on the table.

>> And if drm is at some point extended to support panel drivers, or chains
>> of external display entities, it would be easier to add that support.
>>
>> What would it require the manage the elements like that? Would it help?
>> It sounds to me that this would simplify the model.
> 
> I'm not really entirely sure..  other than at least other drivers
> supporting CFP will have the same requirements ;-)
> 
> I guess the two best options are either bury some sort of chain of
> panel drivers in the connector, or introduce some internal elements in
> DRM which are not necessarily visible to userspace.  (Or at least
> userspace should have the option to ignore it for backwards
> compatibility.  For atomic pageflip/modeset, the converting of
> everything to properties makes it easier to think about exposing new
> KMS mode object types to userspace.)

Yes, I don't think we should or need to expose these new elements to
userspace, at least in the first place.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 6815 bytes --]

On 2012-11-07 16:32, Rob Clark wrote:
> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

>> Hotplugging is not some abstract future scenario, we already have
>> hardware that could use it. For example, omap3 SDP board has a
>> switchable output to DVI or LCD panel. In this case we know what the two
>> options are, but the disabled component is still effectually removed
>> from the system, and plugged back in when it's enabled.
> 
> I would look at this as two different connectors which can not be used
> at the same time.  You have this scenario with desktop graphics cards.

Yes, that's an option with fixed amount of display devices. But doesn't
work for capes.

>> Hotplug is not a high priority item, but I do wish we get it supported
>> in common panel framework. Then it's at least possible to extend drm in
>> the future to support it.
>>
>>
>>
>> Anyway, this makes me wonder... omapdrm currently maps the elements of
>> the whole video pipeline to drm elements (encoder, connector, etc).
>> Would it make more sense to just map the DISPC to these drm elements?
>> Connector would then be the output from DISPC.
> 
> I think:
> 
>   plane->overlay
>   crtc->manager
> 
> is pretty clear.  And really
> 
>   encoder->output
> 
> should be the way it is..  on the branch w/ omapdss/omapdrm kms

I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
chain. The primary "output" is in the DISPC module, the overlay manager.
That's where the timings, pixel clock, etc. are programmed. The second
step, our output, is really a converter IP. It receives the primary
output, converts it and outputs something else. Just like an external
converter chip would do.

And the output can be quite a bit anything. For example, with DBI or DSI
command mode outputs we don't have any of the conventional video
timings. It doesn't make sense to program, say, video blanking periods
to those outputs. But even with DBI and DSI we do have video blanking
periods in the DISPC's output, the ovl mgr.

Of course, at the end of the chain we have a panel that uses normal
video timings (well, most likely but not necessarily), and so we could
program those timings at the end of the chain, in the block before the
panel. But even then the encoder doesn't really map to the DSS's output
block, as the DSS's output block may not have the conventional timings
(like DBI), or they may be something totally different than what we get
in the end of the chain to the panel.

So I think mapping encoder to output will not work with multiple display
blocks in a chain. Thus I'd see the encoder would better match the
DISPC's output, or alternatively perhaps the block which is just before
the panel (whatever that is, sometimes it can be OMAP's DSI/HDMI/etc).
However, the latter may be a bit strange as the block could be an
external component, possibly hotpluggable.

> re-write, this is how it is for plane/crtc, except for now:
> 
>   encoder+connector->dssdev
> 
> Basically the encoder is doing the "control" stuff (power on/off, set
> timings, etc), and the connector is only doing non control stuff
> (detect, reading edid, etc).
> 
> But I think this will probably change a bit as CFP comes into the
> picture.  Currently the drm connector is somewhat a "passive" element,
> but I think this will have to change a bit w/ CFP.
> 
>> This would map the drm elements to the static hardware blocks, and the
>> meaning of those blocks would be quite similar to what they are in the
>> desktop world (I guess).
>>
>> The panel driver, the external chips, and the DSS internal output blocks
>> (dsi, dpi, ...) would be handled separately from those drm elements. The
>> DSS internal blocks are static, of course, but they can be effectively
>> considered the same way as external chips.
> 
> I think dsi/dpi/etc map to encoder.  The big question is where the
> panel's fit.  But to userspace somehow this should look like
> connectors.  I think:
> 
>   encoder->output
>   connector->panel
> 
> could work.. although connector is less passive than KMS currently
> assumes.  And "panel" could really be a whole chain in the case of
> bridge chips, etc.  I don't know, maybe there are better ways.  But I
> think userspace really just wants to know "which monitor" which is
> basically connector.

Hmm yes. Well, even if we map encoder and connector to the ovl manager,
the userspace could see which monitor there is. Obviously we need to
make changes for that to work, but as a model it feels a lot more
natural to me than using output and panel for encoder and connector.

Perhaps it's wrong to say "map connector to ovl mgr". It would be more
like "this connector observes the chain connected to this ovl mgr", even
though the connector wouldn't observe any block in the chain directly.
Just observing the plug in/out status, etc.

But I think encoder really maps quite well directly to the output side
of overlay manager.

>> The omapdrm driver needs of course to access those separate elements
>> also, but that shouldn't be a problem. If omapdrm needs to call a
>> function in the panel driver, all it needs to do is go through the chain
>> to find the panel. Well, except if one output connected two two panels
>> via a bridge chip...
> 
> yeah, that is a really ugly case in our hw since it is quite
> non-transparent (ie. implications about use of planes, etc).

Not really in this case. You're perhaps thinking about connecting two
outputs to a single panel. Which is problematic also.

We don't have in sights a board that splits one output to two panels, so
I think we should just ignore that for now. But two outputs for one
panel is on the table.

>> And if drm is at some point extended to support panel drivers, or chains
>> of external display entities, it would be easier to add that support.
>>
>> What would it require the manage the elements like that? Would it help?
>> It sounds to me that this would simplify the model.
> 
> I'm not really entirely sure..  other than at least other drivers
> supporting CFP will have the same requirements ;-)
> 
> I guess the two best options are either bury some sort of chain of
> panel drivers in the connector, or introduce some internal elements in
> DRM which are not necessarily visible to userspace.  (Or at least
> userspace should have the option to ignore it for backwards
> compatibility.  For atomic pageflip/modeset, the converting of
> everything to properties makes it easier to think about exposing new
> KMS mode object types to userspace.)

Yes, I don't think we should or need to expose these new elements to
userspace, at least in the first place.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Rob Clark]

On Wed, Nov 7, 2012 at 9:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 2012-11-07 16:32, Rob Clark wrote:
>> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>>> Hotplugging is not some abstract future scenario, we already have
>>> hardware that could use it. For example, omap3 SDP board has a
>>> switchable output to DVI or LCD panel. In this case we know what the two
>>> options are, but the disabled component is still effectually removed
>>> from the system, and plugged back in when it's enabled.
>>
>> I would look at this as two different connectors which can not be used
>> at the same time.  You have this scenario with desktop graphics cards.
>
> Yes, that's an option with fixed amount of display devices. But doesn't
> work for capes.

Only if capes are hotpluggable.. otherwise probe what cape(s?) are
present at boot time (is this possible to detect a cape from sw?), and
create the associated connector(s).

Anyways, I think we are stretching a bit hard for use cases for
hot-pluggable panels..  I just prefer to ignore hotplug for now and
come back to it when there is a more legitimate use-case.

>>> Hotplug is not a high priority item, but I do wish we get it supported
>>> in common panel framework. Then it's at least possible to extend drm in
>>> the future to support it.
>>>
>>>
>>>
>>> Anyway, this makes me wonder... omapdrm currently maps the elements of
>>> the whole video pipeline to drm elements (encoder, connector, etc).
>>> Would it make more sense to just map the DISPC to these drm elements?
>>> Connector would then be the output from DISPC.
>>
>> I think:
>>
>>   plane->overlay
>>   crtc->manager
>>
>> is pretty clear.  And really
>>
>>   encoder->output
>>
>> should be the way it is..  on the branch w/ omapdss/omapdrm kms
>
> I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
> chain. The primary "output" is in the DISPC module, the overlay manager.
> That's where the timings, pixel clock, etc. are programmed. The second
> step, our output, is really a converter IP. It receives the primary
> output, converts it and outputs something else. Just like an external
> converter chip would do.
>

the timings, pixel clock, vblank/framedone irqs, that all maps to crtc.

encoder = converter, so I think this fits.  "An encoder takes pixel
data from a CRTC and converts it to a format suitable for any attached
connectors" (from drm docbook)

> And the output can be quite a bit anything. For example, with DBI or DSI
> command mode outputs we don't have any of the conventional video
> timings. It doesn't make sense to program, say, video blanking periods
> to those outputs. But even with DBI and DSI we do have video blanking
> periods in the DISPC's output, the ovl mgr.

the encoder has mode_fixup() which can alter the timings that end up
getting set, which might be a way to account for this.  I guess really
it should be the panel driver that is telling the encoder what
adjusted timings to give to the crtc.. so the panel driver doesn't
quite map to connector.

> Of course, at the end of the chain we have a panel that uses normal
> video timings (well, most likely but not necessarily), and so we could
> program those timings at the end of the chain, in the block before the
> panel. But even then the encoder doesn't really map to the DSS's output
> block, as the DSS's output block may not have the conventional timings
> (like DBI), or they may be something totally different than what we get
> in the end of the chain to the panel.
>
> So I think mapping encoder to output will not work with multiple display
> blocks in a chain. Thus I'd see the encoder would better match the
> DISPC's output, or alternatively perhaps the block which is just before
> the panel (whatever that is, sometimes it can be OMAP's DSI/HDMI/etc).
> However, the latter may be a bit strange as the block could be an
> external component, possibly hotpluggable.
>
>> re-write, this is how it is for plane/crtc, except for now:
>>
>>   encoder+connector->dssdev
>>
>> Basically the encoder is doing the "control" stuff (power on/off, set
>> timings, etc), and the connector is only doing non control stuff
>> (detect, reading edid, etc).
>>
>> But I think this will probably change a bit as CFP comes into the
>> picture.  Currently the drm connector is somewhat a "passive" element,
>> but I think this will have to change a bit w/ CFP.
>>
>>> This would map the drm elements to the static hardware blocks, and the
>>> meaning of those blocks would be quite similar to what they are in the
>>> desktop world (I guess).
>>>
>>> The panel driver, the external chips, and the DSS internal output blocks
>>> (dsi, dpi, ...) would be handled separately from those drm elements. The
>>> DSS internal blocks are static, of course, but they can be effectively
>>> considered the same way as external chips.
>>
>> I think dsi/dpi/etc map to encoder.  The big question is where the
>> panel's fit.  But to userspace somehow this should look like
>> connectors.  I think:
>>
>>   encoder->output
>>   connector->panel
>>
>> could work.. although connector is less passive than KMS currently
>> assumes.  And "panel" could really be a whole chain in the case of
>> bridge chips, etc.  I don't know, maybe there are better ways.  But I
>> think userspace really just wants to know "which monitor" which is
>> basically connector.
>
> Hmm yes. Well, even if we map encoder and connector to the ovl manager,
> the userspace could see which monitor there is. Obviously we need to
> make changes for that to work, but as a model it feels a lot more
> natural to me than using output and panel for encoder and connector.

well, what we call 'ovl mgr' is really where the (vblank) interrupt
generation is.  This really should be the crtc.  I tried it
differently initially, and it wasn't really working out.

> Perhaps it's wrong to say "map connector to ovl mgr". It would be more
> like "this connector observes the chain connected to this ovl mgr", even
> though the connector wouldn't observe any block in the chain directly.
> Just observing the plug in/out status, etc.
>
> But I think encoder really maps quite well directly to the output side
> of overlay manager.
>
>>> The omapdrm driver needs of course to access those separate elements
>>> also, but that shouldn't be a problem. If omapdrm needs to call a
>>> function in the panel driver, all it needs to do is go through the chain
>>> to find the panel. Well, except if one output connected two two panels
>>> via a bridge chip...
>>
>> yeah, that is a really ugly case in our hw since it is quite
>> non-transparent (ie. implications about use of planes, etc).
>
> Not really in this case. You're perhaps thinking about connecting two
> outputs to a single panel. Which is problematic also.

yes, sorry, I read that backwards..

> We don't have in sights a board that splits one output to two panels, so
> I think we should just ignore that for now. But two outputs for one
> panel is on the table.
>
>>> And if drm is at some point extended to support panel drivers, or chains
>>> of external display entities, it would be easier to add that support.
>>>
>>> What would it require the manage the elements like that? Would it help?
>>> It sounds to me that this would simplify the model.
>>
>> I'm not really entirely sure..  other than at least other drivers
>> supporting CFP will have the same requirements ;-)
>>
>> I guess the two best options are either bury some sort of chain of
>> panel drivers in the connector, or introduce some internal elements in
>> DRM which are not necessarily visible to userspace.  (Or at least
>> userspace should have the option to ignore it for backwards
>> compatibility.  For atomic pageflip/modeset, the converting of
>> everything to properties makes it easier to think about exposing new
>> KMS mode object types to userspace.)
>
> Yes, I don't think we should or need to expose these new elements to
> userspace, at least in the first place.

It *might* be useful someday, if there are ever any settings for a
bridge chip, etc, which we'd want to expose to userspace as
properties..  but I think we can come back to that later.

BR,
-R

>  Tomi
>
>

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Rob Clark]

On Wed, Nov 7, 2012 at 9:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 2012-11-07 16:32, Rob Clark wrote:
>> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>>> Hotplugging is not some abstract future scenario, we already have
>>> hardware that could use it. For example, omap3 SDP board has a
>>> switchable output to DVI or LCD panel. In this case we know what the two
>>> options are, but the disabled component is still effectually removed
>>> from the system, and plugged back in when it's enabled.
>>
>> I would look at this as two different connectors which can not be used
>> at the same time.  You have this scenario with desktop graphics cards.
>
> Yes, that's an option with fixed amount of display devices. But doesn't
> work for capes.

Only if capes are hotpluggable.. otherwise probe what cape(s?) are
present at boot time (is this possible to detect a cape from sw?), and
create the associated connector(s).

Anyways, I think we are stretching a bit hard for use cases for
hot-pluggable panels..  I just prefer to ignore hotplug for now and
come back to it when there is a more legitimate use-case.

>>> Hotplug is not a high priority item, but I do wish we get it supported
>>> in common panel framework. Then it's at least possible to extend drm in
>>> the future to support it.
>>>
>>>
>>>
>>> Anyway, this makes me wonder... omapdrm currently maps the elements of
>>> the whole video pipeline to drm elements (encoder, connector, etc).
>>> Would it make more sense to just map the DISPC to these drm elements?
>>> Connector would then be the output from DISPC.
>>
>> I think:
>>
>>   plane->overlay
>>   crtc->manager
>>
>> is pretty clear.  And really
>>
>>   encoder->output
>>
>> should be the way it is..  on the branch w/ omapdss/omapdrm kms
>
> I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
> chain. The primary "output" is in the DISPC module, the overlay manager.
> That's where the timings, pixel clock, etc. are programmed. The second
> step, our output, is really a converter IP. It receives the primary
> output, converts it and outputs something else. Just like an external
> converter chip would do.
>

the timings, pixel clock, vblank/framedone irqs, that all maps to crtc.

encoder == converter, so I think this fits.  "An encoder takes pixel
data from a CRTC and converts it to a format suitable for any attached
connectors" (from drm docbook)

> And the output can be quite a bit anything. For example, with DBI or DSI
> command mode outputs we don't have any of the conventional video
> timings. It doesn't make sense to program, say, video blanking periods
> to those outputs. But even with DBI and DSI we do have video blanking
> periods in the DISPC's output, the ovl mgr.

the encoder has mode_fixup() which can alter the timings that end up
getting set, which might be a way to account for this.  I guess really
it should be the panel driver that is telling the encoder what
adjusted timings to give to the crtc.. so the panel driver doesn't
quite map to connector.

> Of course, at the end of the chain we have a panel that uses normal
> video timings (well, most likely but not necessarily), and so we could
> program those timings at the end of the chain, in the block before the
> panel. But even then the encoder doesn't really map to the DSS's output
> block, as the DSS's output block may not have the conventional timings
> (like DBI), or they may be something totally different than what we get
> in the end of the chain to the panel.
>
> So I think mapping encoder to output will not work with multiple display
> blocks in a chain. Thus I'd see the encoder would better match the
> DISPC's output, or alternatively perhaps the block which is just before
> the panel (whatever that is, sometimes it can be OMAP's DSI/HDMI/etc).
> However, the latter may be a bit strange as the block could be an
> external component, possibly hotpluggable.
>
>> re-write, this is how it is for plane/crtc, except for now:
>>
>>   encoder+connector->dssdev
>>
>> Basically the encoder is doing the "control" stuff (power on/off, set
>> timings, etc), and the connector is only doing non control stuff
>> (detect, reading edid, etc).
>>
>> But I think this will probably change a bit as CFP comes into the
>> picture.  Currently the drm connector is somewhat a "passive" element,
>> but I think this will have to change a bit w/ CFP.
>>
>>> This would map the drm elements to the static hardware blocks, and the
>>> meaning of those blocks would be quite similar to what they are in the
>>> desktop world (I guess).
>>>
>>> The panel driver, the external chips, and the DSS internal output blocks
>>> (dsi, dpi, ...) would be handled separately from those drm elements. The
>>> DSS internal blocks are static, of course, but they can be effectively
>>> considered the same way as external chips.
>>
>> I think dsi/dpi/etc map to encoder.  The big question is where the
>> panel's fit.  But to userspace somehow this should look like
>> connectors.  I think:
>>
>>   encoder->output
>>   connector->panel
>>
>> could work.. although connector is less passive than KMS currently
>> assumes.  And "panel" could really be a whole chain in the case of
>> bridge chips, etc.  I don't know, maybe there are better ways.  But I
>> think userspace really just wants to know "which monitor" which is
>> basically connector.
>
> Hmm yes. Well, even if we map encoder and connector to the ovl manager,
> the userspace could see which monitor there is. Obviously we need to
> make changes for that to work, but as a model it feels a lot more
> natural to me than using output and panel for encoder and connector.

well, what we call 'ovl mgr' is really where the (vblank) interrupt
generation is.  This really should be the crtc.  I tried it
differently initially, and it wasn't really working out.

> Perhaps it's wrong to say "map connector to ovl mgr". It would be more
> like "this connector observes the chain connected to this ovl mgr", even
> though the connector wouldn't observe any block in the chain directly.
> Just observing the plug in/out status, etc.
>
> But I think encoder really maps quite well directly to the output side
> of overlay manager.
>
>>> The omapdrm driver needs of course to access those separate elements
>>> also, but that shouldn't be a problem. If omapdrm needs to call a
>>> function in the panel driver, all it needs to do is go through the chain
>>> to find the panel. Well, except if one output connected two two panels
>>> via a bridge chip...
>>
>> yeah, that is a really ugly case in our hw since it is quite
>> non-transparent (ie. implications about use of planes, etc).
>
> Not really in this case. You're perhaps thinking about connecting two
> outputs to a single panel. Which is problematic also.

yes, sorry, I read that backwards..

> We don't have in sights a board that splits one output to two panels, so
> I think we should just ignore that for now. But two outputs for one
> panel is on the table.
>
>>> And if drm is at some point extended to support panel drivers, or chains
>>> of external display entities, it would be easier to add that support.
>>>
>>> What would it require the manage the elements like that? Would it help?
>>> It sounds to me that this would simplify the model.
>>
>> I'm not really entirely sure..  other than at least other drivers
>> supporting CFP will have the same requirements ;-)
>>
>> I guess the two best options are either bury some sort of chain of
>> panel drivers in the connector, or introduce some internal elements in
>> DRM which are not necessarily visible to userspace.  (Or at least
>> userspace should have the option to ignore it for backwards
>> compatibility.  For atomic pageflip/modeset, the converting of
>> everything to properties makes it easier to think about exposing new
>> KMS mode object types to userspace.)
>
> Yes, I don't think we should or need to expose these new elements to
> userspace, at least in the first place.

It *might* be useful someday, if there are ever any settings for a
bridge chip, etc, which we'd want to expose to userspace as
properties..  but I think we can come back to that later.

BR,
-R

>  Tomi
>
>

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4910 bytes --]

On 2012-11-07 21:18, Rob Clark wrote:
> On Wed, Nov 7, 2012 at 9:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 2012-11-07 16:32, Rob Clark wrote:
>>> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>>>> Hotplugging is not some abstract future scenario, we already have
>>>> hardware that could use it. For example, omap3 SDP board has a
>>>> switchable output to DVI or LCD panel. In this case we know what the two
>>>> options are, but the disabled component is still effectually removed
>>>> from the system, and plugged back in when it's enabled.
>>>
>>> I would look at this as two different connectors which can not be used
>>> at the same time.  You have this scenario with desktop graphics cards.
>>
>> Yes, that's an option with fixed amount of display devices. But doesn't
>> work for capes.
> 
> Only if capes are hotpluggable.. otherwise probe what cape(s?) are
> present at boot time (is this possible to detect a cape from sw?), and
> create the associated connector(s).

Well, a cape can be anything. For beaglebone they have capes with
eeprom, and you can detect it.

The reason I'd like to have hotplug is that it would simplify panel
drivers in the case where we have multiple possible panels for the same
output, like the DVI/LCD case above for omap3 SDP.

If we don't have hotplug, then both DVI and LCD panel devices are
present at the same time, and they will share resources. In the minimum
they are sharing the video output, but more often than not they share
gpios/powers/etc.

It's normal that a driver will acquire resources for its device in its
probe, and thus we would have two drivers acquiring the same resources
at boot time, leading to the other driver failing. We currently manage
this by acquiring the resources late, only when the panel is being
enabled. But I think that's rather ugly.

It would be much cleaner if the panel device does not exist at all if
the panel is disconnected, and is created only when it is connected.
This of course creates the problem of who is responsible for creating
the panel device, and what triggers it. I think that's case specific,
and for capes, it'd be the cape driver.

But then again, I guess it's acceptable that we don't allow changing the
plugged-in panels at runtime. The user would have to select them with
kernel parameters or such. I guess this would be ok for capes and
development boards. I'm not aware of a production board that would
switch panels at runtime, although I know these were on the table in Nokia.

> Anyways, I think we are stretching a bit hard for use cases for
> hot-pluggable panels..  I just prefer to ignore hotplug for now and
> come back to it when there is a more legitimate use-case.

Ok, fair enough. But let's keep hotplug in mind, and if we're going to
create code that would make hotplug impossible to implement, let's stop
for a moment and think if we can do that in some other way.

>> I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
>> chain. The primary "output" is in the DISPC module, the overlay manager.
>> That's where the timings, pixel clock, etc. are programmed. The second
>> step, our output, is really a converter IP. It receives the primary
>> output, converts it and outputs something else. Just like an external
>> converter chip would do.
>>
> 
> the timings, pixel clock, vblank/framedone irqs, that all maps to crtc.
> 
> encoder == converter, so I think this fits.  "An encoder takes pixel
> data from a CRTC and converts it to a format suitable for any attached
> connectors" (from drm docbook)

Oh, ok. Then what you say makes sense. I thought encoder contains the
timings, as you said previously: "Basically the encoder is doing the
"control" stuff (power on/off, set timings, etc)".

In the case we have a long chain of display blocks, encoder could cover
all of them, not just the output block of OMAP? I don't know where the
panel goes, though.

>> And the output can be quite a bit anything. For example, with DBI or DSI
>> command mode outputs we don't have any of the conventional video
>> timings. It doesn't make sense to program, say, video blanking periods
>> to those outputs. But even with DBI and DSI we do have video blanking
>> periods in the DISPC's output, the ovl mgr.
> 
> the encoder has mode_fixup() which can alter the timings that end up
> getting set, which might be a way to account for this.  I guess really
> it should be the panel driver that is telling the encoder what
> adjusted timings to give to the crtc.. so the panel driver doesn't
> quite map to connector.

The panel can't say what timings to give to crtc, it depends on what's
between the crtc and the panel. In case of OMAP DSI, the dsi driver
needs to specify the timings for crtc, based on the panel.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4910 bytes --]

On 2012-11-07 21:18, Rob Clark wrote:
> On Wed, Nov 7, 2012 at 9:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 2012-11-07 16:32, Rob Clark wrote:
>>> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>>>> Hotplugging is not some abstract future scenario, we already have
>>>> hardware that could use it. For example, omap3 SDP board has a
>>>> switchable output to DVI or LCD panel. In this case we know what the two
>>>> options are, but the disabled component is still effectually removed
>>>> from the system, and plugged back in when it's enabled.
>>>
>>> I would look at this as two different connectors which can not be used
>>> at the same time.  You have this scenario with desktop graphics cards.
>>
>> Yes, that's an option with fixed amount of display devices. But doesn't
>> work for capes.
> 
> Only if capes are hotpluggable.. otherwise probe what cape(s?) are
> present at boot time (is this possible to detect a cape from sw?), and
> create the associated connector(s).

Well, a cape can be anything. For beaglebone they have capes with
eeprom, and you can detect it.

The reason I'd like to have hotplug is that it would simplify panel
drivers in the case where we have multiple possible panels for the same
output, like the DVI/LCD case above for omap3 SDP.

If we don't have hotplug, then both DVI and LCD panel devices are
present at the same time, and they will share resources. In the minimum
they are sharing the video output, but more often than not they share
gpios/powers/etc.

It's normal that a driver will acquire resources for its device in its
probe, and thus we would have two drivers acquiring the same resources
at boot time, leading to the other driver failing. We currently manage
this by acquiring the resources late, only when the panel is being
enabled. But I think that's rather ugly.

It would be much cleaner if the panel device does not exist at all if
the panel is disconnected, and is created only when it is connected.
This of course creates the problem of who is responsible for creating
the panel device, and what triggers it. I think that's case specific,
and for capes, it'd be the cape driver.

But then again, I guess it's acceptable that we don't allow changing the
plugged-in panels at runtime. The user would have to select them with
kernel parameters or such. I guess this would be ok for capes and
development boards. I'm not aware of a production board that would
switch panels at runtime, although I know these were on the table in Nokia.

> Anyways, I think we are stretching a bit hard for use cases for
> hot-pluggable panels..  I just prefer to ignore hotplug for now and
> come back to it when there is a more legitimate use-case.

Ok, fair enough. But let's keep hotplug in mind, and if we're going to
create code that would make hotplug impossible to implement, let's stop
for a moment and think if we can do that in some other way.

>> I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
>> chain. The primary "output" is in the DISPC module, the overlay manager.
>> That's where the timings, pixel clock, etc. are programmed. The second
>> step, our output, is really a converter IP. It receives the primary
>> output, converts it and outputs something else. Just like an external
>> converter chip would do.
>>
> 
> the timings, pixel clock, vblank/framedone irqs, that all maps to crtc.
> 
> encoder == converter, so I think this fits.  "An encoder takes pixel
> data from a CRTC and converts it to a format suitable for any attached
> connectors" (from drm docbook)

Oh, ok. Then what you say makes sense. I thought encoder contains the
timings, as you said previously: "Basically the encoder is doing the
"control" stuff (power on/off, set timings, etc)".

In the case we have a long chain of display blocks, encoder could cover
all of them, not just the output block of OMAP? I don't know where the
panel goes, though.

>> And the output can be quite a bit anything. For example, with DBI or DSI
>> command mode outputs we don't have any of the conventional video
>> timings. It doesn't make sense to program, say, video blanking periods
>> to those outputs. But even with DBI and DSI we do have video blanking
>> periods in the DISPC's output, the ovl mgr.
> 
> the encoder has mode_fixup() which can alter the timings that end up
> getting set, which might be a way to account for this.  I guess really
> it should be the panel driver that is telling the encoder what
> adjusted timings to give to the crtc.. so the panel driver doesn't
> quite map to connector.

The panel can't say what timings to give to crtc, it depends on what's
between the crtc and the panel. In case of OMAP DSI, the dsi driver
needs to specify the timings for crtc, based on the panel.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]