mxsfb on i.MX28 uses bypassed ref_xtal 24MHz clock for LCD

mxsfb on i.MX28 uses bypassed ref_xtal 24MHz clock for LCD

[Hector Palacios]

Hello,

I was testing the framebuffer on an i.MX28 based platform and found out that the lcdif 
clock is actually bypassed to use the ref_xtal of 24MHz, which makes the 
clk_set_rate() call in the driver useless (as well as the pixclock settings in 
fb_videomode variables in mach-mxs.c):

static void mxsfb_enable_controller(struct fb_info *fb_info)
{
	struct mxsfb_info *host = to_imxfb_host(fb_info);
	u32 reg;

	dev_dbg(&host->pdev->dev, "%s\n", __func__);

	clk_prepare_enable(host->clk);
	clk_set_rate(host->clk, PICOS2KHZ(fb_info->var.pixclock) * 1000U);

This can easily be checked by probing the pixel clock signal.
Is this known or did it go unnoticed because the supported displays work ok with the 
bypassed 24MHz frequency?
If using a pixel clock below 24MHz, the divisor will round to 2 thus generating a 
12MHz signal.

On the MX28EVK board with the Seiko 4.3" LCD, I do not see the penguin or any image in 
v3.8 or v3.9 though the framebuffer device is present and correctly loaded and 
backlight on.
This LCD works in kernel v2.6.35 with the same 24MHz frequency.
@Fabio, does this LCD work out of the box on v3.9? Am I missing something?

Thank you.
-- 
Héctor Palacios

Re: mxsfb on i.MX28 uses bypassed ref_xtal 24MHz clock for LCD

[Fabio Estevam]

On 05/17/2013 10:57 AM, Hector Palacios wrote:

> On the MX28EVK board with the Seiko 4.3" LCD, I do not see the penguin
> or any image in v3.8 or v3.9 though the framebuffer device is present
> and correctly loaded and backlight on.
> This LCD works in kernel v2.6.35 with the same 24MHz frequency.
> @Fabio, does this LCD work out of the box on v3.9? Am I missing something?

Yes, works fine here.

Make sure you update both the kernel and dtb files on your tests.

Regards,

Fabio Estevam

Re: mxsfb on i.MX28 uses bypassed ref_xtal 24MHz clock for LCD

[Hector Palacios]

Dear Fabio,

On 05/17/2013 04:07 PM, Fabio Estevam wrote:
> On 05/17/2013 10:57 AM, Hector Palacios wrote:
>
>> On the MX28EVK board with the Seiko 4.3" LCD, I do not see the penguin
>> or any image in v3.8 or v3.9 though the framebuffer device is present
>> and correctly loaded and backlight on.
>> This LCD works in kernel v2.6.35 with the same 24MHz frequency.
>> @Fabio, does this LCD work out of the box on v3.9? Am I missing something?
>
> Yes, works fine here.
>
> Make sure you update both the kernel and dtb files on your tests.

Agh. Now it works on the EVK. I had something wrong with the backlight (maybe a bad DTB).

Do you have any comment on the fact that the lcdif clock is bypassed to the 24MHz ref 
clock? Doesn't this make useless the definition of pixelclk for the different displays 
in mach-mxs.c?
You can only obtain whole divisors of this frequency (24, 12, 6, 4), which may not be 
accurate enough to fit some displays.

Regards,
-- 
Héctor Palacios

Re: mxsfb on i.MX28 uses bypassed ref_xtal 24MHz clock for LCD

[Fabio Estevam]

On Mon, May 20, 2013 at 11:59 AM, Hector Palacios
<hector.palacios@digi.com> wrote:

> Agh. Now it works on the EVK. I had something wrong with the backlight
> (maybe a bad DTB).
>
> Do you have any comment on the fact that the lcdif clock is bypassed to the
> 24MHz ref clock? Doesn't this make useless the definition of pixelclk for
> the different displays in mach-mxs.c?
> You can only obtain whole divisors of this frequency (24, 12, 6, 4), which
> may not be accurate enough to fit some displays.

I haven't had a chance to look at it yet, but feel free to submit
patches if you think you can improve the driver.

Thanks,

Fabio Estevam

mxsfb on i.MX28 uses bypassed ref_xtal 24MHz clock for LCD

[Hector Palacios]

On 05/17/2013 03:57 PM, Hector Palacios wrote:
> Hello,
>
> I was testing the framebuffer on an i.MX28 based platform and found out that the lcdif
> clock is actually bypassed to use the ref_xtal of 24MHz, which makes the
> clk_set_rate() call in the driver useless (as well as the pixclock settings in
> fb_videomode variables in mach-mxs.c):
>
> static void mxsfb_enable_controller(struct fb_info *fb_info)
> {
>      struct mxsfb_info *host = to_imxfb_host(fb_info);
>      u32 reg;
>
>      dev_dbg(&host->pdev->dev, "%s\n", __func__);
>
>      clk_prepare_enable(host->clk);
>      clk_set_rate(host->clk, PICOS2KHZ(fb_info->var.pixclock) * 1000U);
>
> This can easily be checked by probing the pixel clock signal.
> Is this known or did it go unnoticed because the supported displays work ok with the
> bypassed 24MHz frequency?
> If using a pixel clock below 24MHz, the divisor will round to 2 thus generating a
> 12MHz signal.

Going back to this, after a while. The lcdif clock hierarchy is the following:

ref_clk (24MHz)-----\
                      +--+ lcdif_sel (MUX type)
frac1_clk (480Mhz)--/   |
                         \--+ lcdif_div (DIV type)
                            |
                            \-- lcdif (GATE type)

The mxsfb driver retrieves the 'lcdif_div' clock from the DT, but the 'lcdif_sel' is 
by default sourced from 'ref_clk'. Since the driver doesn't know about the different 
clocks that can source its clk parent from, would it be acceptable to select the 
parent during the general clocks initialization?

diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
index fd17995..211b4d9 100644
--- a/drivers/clk/mxs/clk-imx28.c
+++ b/drivers/clk/mxs/clk-imx28.c
@@ -246,6 +246,12 @@ int __init mx28_clocks_init(void)
	clk_data.clk_num = ARRAY_SIZE(clks);
	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);

+	/*
+	 * Select PLL as the parent source of lcdif_sel clk to have a finer
+	 * granularity when calculating the LCD pixelclock
+	 */
+	clk_set_parent(clks[lcdif_sel], clks[ref_pix]);
+
	clk_register_clkdev(clks[enet_out], NULL, "enet_out");

	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)


Without this, the pixelclock can only be a power of two divisor of 24MHz, and I need a 
20MHz signal, otherwise I see bad pixels in true color images on my LCD.

Best regards,
--
Hector Palacios

mxsfb on i.MX28 uses bypassed ref_xtal 24MHz clock for LCD

[Shawn Guo]

On Fri, Sep 20, 2013 at 03:50:23PM +0200, Hector Palacios wrote:
> On 05/17/2013 03:57 PM, Hector Palacios wrote:
> >Hello,
> >
> >I was testing the framebuffer on an i.MX28 based platform and found out that the lcdif
> >clock is actually bypassed to use the ref_xtal of 24MHz, which makes the
> >clk_set_rate() call in the driver useless (as well as the pixclock settings in
> >fb_videomode variables in mach-mxs.c):
> >
> >static void mxsfb_enable_controller(struct fb_info *fb_info)
> >{
> >     struct mxsfb_info *host = to_imxfb_host(fb_info);
> >     u32 reg;
> >
> >     dev_dbg(&host->pdev->dev, "%s\n", __func__);
> >
> >     clk_prepare_enable(host->clk);
> >     clk_set_rate(host->clk, PICOS2KHZ(fb_info->var.pixclock) * 1000U);
> >
> >This can easily be checked by probing the pixel clock signal.
> >Is this known or did it go unnoticed because the supported displays work ok with the
> >bypassed 24MHz frequency?
> >If using a pixel clock below 24MHz, the divisor will round to 2 thus generating a
> >12MHz signal.
> 
> Going back to this, after a while. The lcdif clock hierarchy is the following:
> 
> ref_clk (24MHz)-----\
>                      +--+ lcdif_sel (MUX type)
> frac1_clk (480Mhz)--/   |
>                         \--+ lcdif_div (DIV type)
>                            |
>                            \-- lcdif (GATE type)
> 
> The mxsfb driver retrieves the 'lcdif_div' clock from the DT, but
> the 'lcdif_sel' is by default sourced from 'ref_clk'. Since the
> driver doesn't know about the different clocks that can source its
> clk parent from, would it be acceptable to select the parent during
> the general clocks initialization?

I'm fine with it.

Shawn

> diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
> index fd17995..211b4d9 100644
> --- a/drivers/clk/mxs/clk-imx28.c
> +++ b/drivers/clk/mxs/clk-imx28.c
> @@ -246,6 +246,12 @@ int __init mx28_clocks_init(void)
> 	clk_data.clk_num = ARRAY_SIZE(clks);
> 	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> 
> +	/*
> +	 * Select PLL as the parent source of lcdif_sel clk to have a finer
> +	 * granularity when calculating the LCD pixelclock
> +	 */
> +	clk_set_parent(clks[lcdif_sel], clks[ref_pix]);
> +
> 	clk_register_clkdev(clks[enet_out], NULL, "enet_out");
> 
> 	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> 
> 
> Without this, the pixelclock can only be a power of two divisor of
> 24MHz, and I need a 20MHz signal, otherwise I see bad pixels in true
> color images on my LCD.
> 
> Best regards,
> --
> Hector Palacios

mxsfb on i.MX28 uses bypassed ref_xtal 24MHz clock for LCD

[Fabio Estevam]

Hi Hector,

On Fri, Sep 20, 2013 at 10:50 AM, Hector Palacios
<hector.palacios@digi.com> wrote:

> --- a/drivers/clk/mxs/clk-imx28.c
> +++ b/drivers/clk/mxs/clk-imx28.c
> @@ -246,6 +246,12 @@ int __init mx28_clocks_init(void)
>         clk_data.clk_num = ARRAY_SIZE(clks);
>         of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>
> +       /*
> +        * Select PLL as the parent source of lcdif_sel clk to have a finer
> +        * granularity when calculating the LCD pixelclock
> +        */
> +       clk_set_parent(clks[lcdif_sel], clks[ref_pix]);
> +
>         clk_register_clkdev(clks[enet_out], NULL, "enet_out");
>
>         for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
>
>
> Without this, the pixelclock can only be a power of two divisor of 24MHz,
> and I need a 20MHz signal, otherwise I see bad pixels in true color images
> on my LCD.

When you submit this patch, you can add:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>