Hi,
On Thu, Sep 22, 2011 at 5:31 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
> On Thu, 2011-09-22 at 13:37 +0530, mythripk@xxxxxx wrote:
>> From: Mythri P K <mythripk@xxxxxx>
>>
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
