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

[Vasily Khoruzhick]

Hi Maxime,

On Wed, Sep 20, 2017 at 11:51 PM, Maxime Ripard
<maxime.ripard at free-electrons.com> wrote:
> 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?

OK.

>> >>> +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.

I've already told you that I tried, and HDMI works fine without it,
but LCD doesn't.
Feel free to play with the code yourself if you don't believe me:
https://github.com/anarsoul/u-boot-pine64

>> >>> +     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.

I'm not sure what common code you're talking about. I've already moved
out lcdc_pll_set(). Moving pinmux
configuration out into common code doesn't look reasonable. It's
different for A64 -- for A64 it configures
GPD(0)-GPD(21) as function, while for other SoCs it's GPD(18)-GPD(27)
or GPD(0)-GPD(27) depending
on SoC model. Anyway, pinmux configuration code for DE1 contains a
number of ifdef-s that are not necessary
for DE2 -- these SoCs don't have DE2 and thus won't be supported.

Do you have any other comments before I send v3?

Regards,
Vasily

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com