[U-Boot] [RESEND PATCH 5/5] sunxi: video: add LCD support to DE2 driver

[Maxime Ripard]

On Thu, Sep 21, 2017 at 05:33:36AM +0000, Vasily Khoruzhick wrote:
> >>> +     ret = uclass_find_device_by_name(UCLASS_DISPLAY,
> >>> +                                      "sunxi_lcd", &disp);
> >>> +     if (!ret) {
> >>> +             int mux;
> >>> +
> >>> +             mux = 0;
> >>> +
> >>> +             ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, mux,
> >>> +                                  false);
> >>> +             if (!ret) {
> >>> +                     video_set_flush_dcache(dev, 1);
> >>
> >> Why do you need to flush the dcache here?
> >
> > Copied from HDMI driver init. If it's not necessary why it's here for HDMI?
> 
> DE2 is not cache aware, so CPU should flush dcache after updating
> framebuffer. If I remove this line, dcache isn't flushed when
> framebuffer is updated, and thus picture is a total mess (black
> background with some white stripes).

Ah, so the framebuffer is mapped cacheable. Ok, that's unusual, but
that works. Can you put that in a comment?

> >>> +static int sunxi_lcd_enable(struct udevice *dev, int bpp,
> >>> +                           const struct display_timing *edid)
> >>> +{
> >>> +     struct sunxi_ccm_reg * const ccm =
> >>> +            (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> >>> +     struct sunxi_lcdc_reg * const lcdc =
> >>> +            (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
> >>> +     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
> >>> +     struct udevice *backlight;
> >>> +     int clk_div, clk_double, ret;
> >>> +
> >>> +     /* Reset off */
> >>> +     setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
> >>> +
> >>> +     /* Clock on */
> >>> +     setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);
> >>
> >> This has nothing to do with using a panel or not, it should be in
> >> lcdc_init().
> >
> > Why? We don't need neither take it out of reset nor turn the clock on
> > it if LCD is not used (e.g. HDMI-only case).
> 
> I'm leaving it here. It's not necessary for HDMI, and it doesn't work
> without it for LCD.

I'm pretty sure it still needs to be done for HDMI. LCD0 is the TCON,
and the TCON is used for HDMI too.

> >>> +     lcdc_init(lcdc);
> >>> +     sunxi_lcdc_config_pinmux();
> >>
> >> This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate
> >> it?
> >
> > Because the one that sunxi_lcdc_tcon0_mode_set() calls is
> > DE1-specific. I don't want to split out that code that won't be used
> > by DE2 driver.

Then move out the common code. It's kind of weird though, since the
DE1 vs DE2 stuff is basically only for the layers part. The TCON is
always there, and is mostly the same. So you should be able to re-use
that with minor modifications.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170921/5ec0a32a/attachment.sig>