Re: [PATCH v2 5/5] OMAPDSS: HDMI: Add support to dump clocks through
On Fri, 2011-09-23 at 11:22 +0530, K, Mythri P wrote: - What is dcofreq? Looking at the code, it tells if the pixel clock is 1000MHz. Why is such a field needed, can't the HDMI driver manage that itself? And if it's needed, why is it called dcofreq, the name doesn't make much sense to me. It is DCO frequency, It suggest the frequency selector range , The field is not DCO frequency, it's a boolean, 0 or 1. That's why the name doesn't really make sense to me. HDMI_PLL_CONFIGURATION2 (3:1) has to be set accordingly by the driver depending on whether the CLKOUTLDO is greater than or less than 1000Mhz, but anyways the decision is taken by the driver. But can't it be done in the ti_hdmi driver, at the same time when programming the registers? Why do we need to set the boolean beforehand. Also the name is as suggested by TRM . I couldn't find boolean dcofreq in the TRM. - We are doing HDMI PLL calculations in the omapdss drivers hdmi.c file. The PLL calculations are PLL specific, and thus should be in the specific HDMI implementation file, right? + seq_printf(s, DISPC clock source %s (%s)\t(%s)\n, + dss_get_generic_clk_source_name(dispc_clk_src), + dss_feat_get_clk_source_name(dispc_clk_src), + dispc_clk_src == OMAP_DSS_CLK_SRC_FCK ? + off : on); Why do you print DISPC clock source? How is that part of HDMI clock configuration? Reason is to check whether the DISPC clock source is PRCM / DSI PLL, Because DSI PLL might not be sufficient. But it's already printed by the DISPC section, and it's not part of HDMI, so I don't quite see the need. What do you mean DSI PLL might not be sufficient? We can get higher DISPC clocks with DSI PLL than with PRCM. Tomi -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] OMAPDSS: HDMI: Add support to dump clocks through
Hi, On Fri, Sep 23, 2011 at 11:31 AM, Tomi Valkeinen tomi.valkei...@ti.com wrote: On Fri, 2011-09-23 at 11:22 +0530, K, Mythri P wrote: - What is dcofreq? Looking at the code, it tells if the pixel clock is 1000MHz. Why is such a field needed, can't the HDMI driver manage that itself? And if it's needed, why is it called dcofreq, the name doesn't make much sense to me. It is DCO frequency, It suggest the frequency selector range , The field is not DCO frequency, it's a boolean, 0 or 1. That's why the name doesn't really make sense to me. HDMI_PLL_CONFIGURATION2 (3:1) has to be set accordingly by the driver depending on whether the CLKOUTLDO is greater than or less than 1000Mhz, but anyways the decision is taken by the driver. But can't it be done in the ti_hdmi driver, at the same time when programming the registers? Why do we need to set the boolean beforehand. It can be done, It is not a boolean , boolean logic is used to determine the value 0x2 / 0x4. Page # 101. (DCO frequency). Also the name is as suggested by TRM . I couldn't find boolean dcofreq in the TRM. - We are doing HDMI PLL calculations in the omapdss drivers hdmi.c file. The PLL calculations are PLL specific, and thus should be in the specific HDMI implementation file, right? + seq_printf(s, DISPC clock source %s (%s)\t(%s)\n, + dss_get_generic_clk_source_name(dispc_clk_src), + dss_feat_get_clk_source_name(dispc_clk_src), + dispc_clk_src == OMAP_DSS_CLK_SRC_FCK ? + off : on); Why do you print DISPC clock source? How is that part of HDMI clock configuration? Reason is to check whether the DISPC clock source is PRCM / DSI PLL, Because DSI PLL might not be sufficient. But it's already printed by the DISPC section, and it's not part of HDMI, so I don't quite see the need. What do you mean DSI PLL might not be sufficient? We can get higher DISPC clocks with DSI PLL than with PRCM. Well , from the older calculation / values passed to DSI , it was seen that DSI PLL was in the order of 156Mhz, and PRCM with 186Mhz. This had resulted in underflow issues with HDMI. Thanks and regards, Mythri. Tomi -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] OMAPDSS: HDMI: Add support to dump clocks through
On Fri, 2011-09-23 at 12:10 +0530, K, Mythri P wrote: Hi, On Fri, Sep 23, 2011 at 11:31 AM, Tomi Valkeinen tomi.valkei...@ti.com wrote: On Fri, 2011-09-23 at 11:22 +0530, K, Mythri P wrote: - What is dcofreq? Looking at the code, it tells if the pixel clock is 1000MHz. Why is such a field needed, can't the HDMI driver manage that itself? And if it's needed, why is it called dcofreq, the name doesn't make much sense to me. It is DCO frequency, It suggest the frequency selector range , The field is not DCO frequency, it's a boolean, 0 or 1. That's why the name doesn't really make sense to me. HDMI_PLL_CONFIGURATION2 (3:1) has to be set accordingly by the driver depending on whether the CLKOUTLDO is greater than or less than 1000Mhz, but anyways the decision is taken by the driver. But can't it be done in the ti_hdmi driver, at the same time when programming the registers? Why do we need to set the boolean beforehand. It can be done, It is not a boolean , boolean logic is used to determine the value 0x2 / 0x4. Page # 101. (DCO frequency). The code says: pi-dcofreq = phy 1000 * 100; so it is a boolean. What do you mean DSI PLL might not be sufficient? We can get higher DISPC clocks with DSI PLL than with PRCM. Well , from the older calculation / values passed to DSI , it was seen that DSI PLL was in the order of 156Mhz, and PRCM with 186Mhz. This had resulted in underflow issues with HDMI. But that's just up to the particular configuration used. DSI PLL can be configured to reach the maximum DSS FCLK, but with PRCM this is not usually possible. I see your point that it is relevant for HDMI to work properly, but there are many other things that are also needed for HDMI to work properly and we don't print them. And as the DISPC clk source is already printed above in the DISPC section, I don't see the need for that here. Tomi -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] OMAPDSS: HDMI: Add support to dump clocks through
From: Mythri P K mythr...@ti.com
Add support to dump the HDMI regm, regn, and other clock parameters.
Signed-off-by: Mythri P K mythr...@ti.com
---
drivers/video/omap2/dss/dss.c |3 +++
drivers/video/omap2/dss/dss.h |1 +
drivers/video/omap2/dss/hdmi.c | 35 +++
3 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 3e09726..76e2bcd 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -732,6 +732,9 @@ void dss_debug_dump_clocks(struct seq_file *s)
#ifdef CONFIG_OMAP2_DSS_DSI
dsi_dump_clocks(s);
#endif
+#ifdef CONFIG_OMAP4_DSS_HDMI
+ hdmi_dump_clocks(s);
+#endif
}
#endif
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 8652007..ef8770a 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -473,6 +473,7 @@ void hdmi_uninit_platform_driver(void);
int hdmi_init_display(struct omap_dss_device *dssdev);
unsigned long hdmi_get_pixel_clock(void);
void hdmi_dump_regs(struct seq_file *s);
+void hdmi_dump_clocks(struct seq_file *s);
#else
static inline int hdmi_init_display(struct omap_dss_device *dssdev)
{
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 3262f0f..8930998 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -33,6 +33,8 @@
#include linux/pm_runtime.h
#include linux/clk.h
#include video/omapdss.h
+#include linux/seq_file.h
+
#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
#include sound/soc.h
@@ -454,6 +456,39 @@ void hdmi_dump_regs(struct seq_file *s)
mutex_unlock(hdmi.lock);
}
+void hdmi_dump_clocks(struct seq_file *s)
+{
+ enum omap_dss_clk_source dispc_clk_src;
+
+ dispc_clk_src = dss_get_dispc_clk_source();
+
+ if (hdmi_runtime_get())
+ return;
+
+ seq_printf(s, - HDMI PLL -\n);
+
+ seq_printf(s, regm\t%u\n, hdmi.ip_data.pll_data.regm);
+
+ seq_printf(s, regmf\t%u\n, hdmi.ip_data.pll_data.regmf);
+
+ seq_printf(s, dcofreq\t%u\n, hdmi.ip_data.pll_data.dcofreq);
+
+ seq_printf(s, regsd\t%u\n, hdmi.ip_data.pll_data.regsd);
+
+ seq_printf(s, DISPC clock source %s (%s)\t(%s)\n,
+ dss_get_generic_clk_source_name(dispc_clk_src),
+ dss_feat_get_clk_source_name(dispc_clk_src),
+ dispc_clk_src == OMAP_DSS_CLK_SRC_FCK ?
+ off : on);
+
+ seq_printf(s, hdmi %s source rate = %lu\n,
+ hdmi.ip_data.pll_data.refsel == HDMI_REFSEL_SYSCLK ?
+ sysclk : pclk/ref1/ref2,
+ clk_get_rate(hdmi.sys_clk));
+
+ hdmi_runtime_put();
+}
+
int omapdss_hdmi_read_edid(u8 *buf, int len)
{
int r;
--
1.7.5.4
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] OMAPDSS: HDMI: Add support to dump clocks through
On Thu, 2011-09-22 at 13:37 +0530, mythr...@ti.com wrote:
From: Mythri P K mythr...@ti.com
Add support to dump the HDMI regm, regn, and other clock parameters.
The subjects of this and previous patch seem to be still broken. And
while at it, you could fix missing periods and misplaced spaces (like
foo ,bar) in the descriptions of this patch series.
+void hdmi_dump_clocks(struct seq_file *s)
+{
+ enum omap_dss_clk_source dispc_clk_src;
+
+ dispc_clk_src = dss_get_dispc_clk_source();
+
+ if (hdmi_runtime_get())
+ return;
+
+ seq_printf(s, - HDMI PLL -\n);
+
+ seq_printf(s, regm\t%u\n, hdmi.ip_data.pll_data.regm);
+
+ seq_printf(s, regmf\t%u\n, hdmi.ip_data.pll_data.regmf);
+
+ seq_printf(s, dcofreq\t%u\n, hdmi.ip_data.pll_data.dcofreq);
+
+ seq_printf(s, regsd\t%u\n, hdmi.ip_data.pll_data.regsd);
Printing the dividers is fine, but I think we're usually more interested
in the resulting clocks. So you should print also the clocks. Possibly
internal clocks (like Fint for DSI) also, but at least the output
clocks. I believe for HDMI PLL they are CLKOUTLDO and CLKDCOLDO.
Looking at the dividers also brings up two things not directly related
to this patch:
- What is dcofreq? Looking at the code, it tells if the pixel clock is
1000MHz. Why is such a field needed, can't the HDMI driver manage that
itself? And if it's needed, why is it called dcofreq, the name doesn't
make much sense to me.
- We are doing HDMI PLL calculations in the omapdss drivers hdmi.c file.
The PLL calculations are PLL specific, and thus should be in the
specific HDMI implementation file, right?
+ seq_printf(s, DISPC clock source %s (%s)\t(%s)\n,
+ dss_get_generic_clk_source_name(dispc_clk_src),
+ dss_feat_get_clk_source_name(dispc_clk_src),
+ dispc_clk_src == OMAP_DSS_CLK_SRC_FCK ?
+ off : on);
Why do you print DISPC clock source? How is that part of HDMI clock
configuration?
+
+ seq_printf(s, hdmi %s source rate = %lu\n,
+ hdmi.ip_data.pll_data.refsel == HDMI_REFSEL_SYSCLK ?
+ sysclk : pclk/ref1/ref2,
+ clk_get_rate(hdmi.sys_clk));
Here I think it would be better to use the same format as the already
existing outputs (DSI). And as the PLL source is base clock, it's more
logical to print it first, like DSI does.
Tomi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] OMAPDSS: HDMI: Add support to dump clocks through
Hi,
On Thu, Sep 22, 2011 at 5:31 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
On Thu, 2011-09-22 at 13:37 +0530, mythr...@ti.com wrote:
From: Mythri P K mythr...@ti.com
Add support to dump the HDMI regm, regn, and other clock parameters.
The subjects of this and previous patch seem to be still broken. And
while at it, you could fix missing periods and misplaced spaces (like
foo ,bar) in the descriptions of this patch series.
Taken.
+void hdmi_dump_clocks(struct seq_file *s)
+{
+ enum omap_dss_clk_source dispc_clk_src;
+
+ dispc_clk_src = dss_get_dispc_clk_source();
+
+ if (hdmi_runtime_get())
+ return;
+
+ seq_printf(s, - HDMI PLL -\n);
+
+ seq_printf(s, regm\t%u\n, hdmi.ip_data.pll_data.regm);
+
+ seq_printf(s, regmf\t%u\n, hdmi.ip_data.pll_data.regmf);
+
+ seq_printf(s, dcofreq\t%u\n, hdmi.ip_data.pll_data.dcofreq);
+
+ seq_printf(s, regsd\t%u\n, hdmi.ip_data.pll_data.regsd);
Printing the dividers is fine, but I think we're usually more interested
in the resulting clocks. So you should print also the clocks. Possibly
internal clocks (like Fint for DSI) also, but at least the output
clocks. I believe for HDMI PLL they are CLKOUTLDO and CLKDCOLDO.
The clkoutldo would be the pixel clock which is a synthesis of these parameters,
I could print.
Looking at the dividers also brings up two things not directly related
to this patch:
- What is dcofreq? Looking at the code, it tells if the pixel clock is
1000MHz. Why is such a field needed, can't the HDMI driver manage that
itself? And if it's needed, why is it called dcofreq, the name doesn't
make much sense to me.
It is DCO frequency, It suggest the frequency selector range ,
HDMI_PLL_CONFIGURATION2 (3:1) has to be set accordingly by the driver
depending on whether the CLKOUTLDO is greater than or less than
1000Mhz, but anyways the decision is taken by the driver.
Also the name is as suggested by TRM .
- We are doing HDMI PLL calculations in the omapdss drivers hdmi.c file.
The PLL calculations are PLL specific, and thus should be in the
specific HDMI implementation file, right?
+ seq_printf(s, DISPC clock source %s (%s)\t(%s)\n,
+ dss_get_generic_clk_source_name(dispc_clk_src),
+ dss_feat_get_clk_source_name(dispc_clk_src),
+ dispc_clk_src == OMAP_DSS_CLK_SRC_FCK ?
+ off : on);
Why do you print DISPC clock source? How is that part of HDMI clock
configuration?
Reason is to check whether the DISPC clock source is PRCM / DSI PLL,
Because DSI PLL might not be sufficient.
+
+ seq_printf(s, hdmi %s source rate = %lu\n,
+ hdmi.ip_data.pll_data.refsel == HDMI_REFSEL_SYSCLK ?
+ sysclk : pclk/ref1/ref2,
+ clk_get_rate(hdmi.sys_clk));
Here I think it would be better to use the same format as the already
existing outputs (DSI). And as the PLL source is base clock, it's more
logical to print it first, like DSI does.
Sure can move it. I shall print sysclk and output clock frequency first.
Tomi
Thanks and regards,
Mythri.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
