Hi Maxime, On Wed, Sep 20, 2017 at 11:51 PM, Maxime Ripard <maxime.rip...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot