Hi Maxime,
On Wed, Sep 20, 2017 at 11:51 PM, Maxime Ripard maxime.ripard@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