On 04/03/15 09:06, Liu Ying wrote:
> The LCDIF engines embedded in i.MX6sl and i.MX6sx SoCs need the axi clock
> as the engine's system clock.  The clock should be enabled when accessing
> LCDIF registers, otherwise the kernel would hang up.  We should also keep
> the clock being enabled when the engine is being active to scan out frames

The text above is a bit confusing. Maybe just "... also keep the clock
enabled when..."

> from memory.  This patch makes sure the axi clock is enabled when accessing
> registers so that the kernel hang up issue can be fixed.
> 
> Reported-by: Peter Chen <[email protected]>
> Tested-by: Peter Chen <[email protected]>
> Cc: <[email protected]> # 3.19+
> Signed-off-by: Liu Ying <[email protected]>
> ---
> v1->v2:
> * Add 'Tested-by: Peter Chen <[email protected]>' tag.
> * Add 'Cc: <[email protected]> # 3.19+' tag.
> 
>  drivers/video/fbdev/mxsfb.c | 70 
> ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 56 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/video/fbdev/mxsfb.c b/drivers/video/fbdev/mxsfb.c
> index f8ac4a4..a8cf3b2 100644
> --- a/drivers/video/fbdev/mxsfb.c
> +++ b/drivers/video/fbdev/mxsfb.c
> @@ -316,6 +316,18 @@ static int mxsfb_check_var(struct fb_var_screeninfo *var,
>       return 0;
>  }
>  
> +static inline void mxsfb_enable_axi_clk(struct mxsfb_info *host)
> +{
> +     if (host->clk_axi)
> +             clk_prepare_enable(host->clk_axi);
> +}
> +
> +static inline void mxsfb_disable_axi_clk(struct mxsfb_info *host)
> +{
> +     if (host->clk_axi)
> +             clk_disable_unprepare(host->clk_axi);
> +}
> +
>  static void mxsfb_enable_controller(struct fb_info *fb_info)
>  {
>       struct mxsfb_info *host = to_imxfb_host(fb_info);
> @@ -333,14 +345,13 @@ static void mxsfb_enable_controller(struct fb_info 
> *fb_info)
>               }
>       }
>  
> -     if (host->clk_axi)
> -             clk_prepare_enable(host->clk_axi);
> -
>       if (host->clk_disp_axi)
>               clk_prepare_enable(host->clk_disp_axi);
>       clk_prepare_enable(host->clk);
>       clk_set_rate(host->clk, PICOS2KHZ(fb_info->var.pixclock) * 1000U);
>  
> +     mxsfb_enable_axi_clk(host);
> +

Is there some reason to move the clk enable to a different place here?

>       /* if it was disabled, re-enable the mode again */
>       writel(CTRL_DOTCLK_MODE, host->base + LCDC_CTRL + REG_SET);
>  
> @@ -380,11 +391,11 @@ static void mxsfb_disable_controller(struct fb_info 
> *fb_info)
>       reg = readl(host->base + LCDC_VDCTRL4);
>       writel(reg & ~VDCTRL4_SYNC_SIGNALS_ON, host->base + LCDC_VDCTRL4);
>  
> +     mxsfb_disable_axi_clk(host);
> +
>       clk_disable_unprepare(host->clk);
>       if (host->clk_disp_axi)
>               clk_disable_unprepare(host->clk_disp_axi);
> -     if (host->clk_axi)
> -             clk_disable_unprepare(host->clk_axi);

And same here for disable.

>       host->enabled = 0;
>  
> @@ -421,6 +432,8 @@ static int mxsfb_set_par(struct fb_info *fb_info)
>               mxsfb_disable_controller(fb_info);
>       }
>  
> +     mxsfb_enable_axi_clk(host);
> +
>       /* clear the FIFOs */
>       writel(CTRL1_FIFO_CLEAR, host->base + LCDC_CTRL1 + REG_SET);
>  
> @@ -438,6 +451,7 @@ static int mxsfb_set_par(struct fb_info *fb_info)
>               ctrl |= CTRL_SET_WORD_LENGTH(3);
>               switch (host->ld_intf_width) {
>               case STMLCDIF_8BIT:
> +                     mxsfb_disable_axi_clk(host);
>                       dev_err(&host->pdev->dev,
>                                       "Unsupported LCD bus width mapping\n");
>                       return -EINVAL;
> @@ -451,6 +465,7 @@ static int mxsfb_set_par(struct fb_info *fb_info)
>               writel(CTRL1_SET_BYTE_PACKAGING(0x7), host->base + LCDC_CTRL1);
>               break;
>       default:
> +             mxsfb_disable_axi_clk(host);
>               dev_err(&host->pdev->dev, "Unhandled color depth of %u\n",
>                               fb_info->var.bits_per_pixel);
>               return -EINVAL;
> @@ -504,6 +519,8 @@ static int mxsfb_set_par(struct fb_info *fb_info)
>                       fb_info->fix.line_length * fb_info->var.yoffset,
>                       host->base + host->devdata->next_buf);
>  
> +     mxsfb_disable_axi_clk(host);
> +
>       if (reenable)
>               mxsfb_enable_controller(fb_info);
>  
> @@ -582,10 +599,16 @@ static int mxsfb_pan_display(struct fb_var_screeninfo 
> *var,
>  
>       offset = fb_info->fix.line_length * var->yoffset;
>  
> +     if (!host->enabled)
> +             mxsfb_enable_axi_clk(host);
> +
>       /* update on next VSYNC */
>       writel(fb_info->fix.smem_start + offset,
>                       host->base + host->devdata->next_buf);
>  
> +     if (!host->enabled)
> +             mxsfb_disable_axi_clk(host);
> +

Why do you check for host->enabled here, but not elsewhere?

 Tomi


Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to