Hi, I did few tests, see results inline.
On Tue, Sep 19, 2017 at 12:00 PM, Vasily Khoruzhick <anars...@gmail.com> wrote: > On Tue, Sep 19, 2017 at 1:33 AM, Maxime Ripard > <maxime.rip...@free-electrons.com> wrote: >> On Mon, Sep 18, 2017 at 10:04:21PM -0700, Vasily Khoruzhick wrote: >>> Extend DE2 driver with LCD support >> >> (All) your commit messages could use a bit more details. > > OK, will add in v2. > >> Here, for example, explaining the following things would help: >> - Why are you creating yet another file > > Are you talking about any specific file? I guess adding another driver > justifies creation of another file. > >> - What is the situation with old Allwinner SoCs that should share >> the same code > > As far as I can tell, DE2 is present in H3, V3s, A64 and newer. LCD is > supported in A64 only. I.e. hardware is not present > in H3 or V3s > >> - What are the expected users > > Pinebook > >> - Which SoC / board have you tested it on > > A64 / Pinebook > >> >> etc... >> >>> Signed-off-by: Vasily Khoruzhick <anars...@gmail.com> >>> --- >>> arch/arm/mach-sunxi/Kconfig | 2 +- >>> drivers/video/sunxi/Makefile | 2 +- >>> drivers/video/sunxi/sunxi_de2.c | 17 +++++ >>> drivers/video/sunxi/sunxi_lcd.c | 142 >>> ++++++++++++++++++++++++++++++++++++++++ >>> 4 files changed, 161 insertions(+), 2 deletions(-) >>> create mode 100644 drivers/video/sunxi/sunxi_lcd.c >>> >>> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig >>> index 2309f59999..06d697e3a7 100644 >>> --- a/arch/arm/mach-sunxi/Kconfig >>> +++ b/arch/arm/mach-sunxi/Kconfig >>> @@ -680,7 +680,7 @@ config VIDEO_LCD_MODE >>> >>> config VIDEO_LCD_DCLK_PHASE >>> int "LCD panel display clock phase" >>> - depends on VIDEO >>> + depends on VIDEO || DM_VIDEO >>> default 1 >>> ---help--- >>> Select LCD panel display clock phase shift, range 0-3. >>> diff --git a/drivers/video/sunxi/Makefile b/drivers/video/sunxi/Makefile >>> index 0d64c2021f..8c91766c24 100644 >>> --- a/drivers/video/sunxi/Makefile >>> +++ b/drivers/video/sunxi/Makefile >>> @@ -6,4 +6,4 @@ >>> # >>> >>> obj-$(CONFIG_VIDEO_SUNXI) += sunxi_display.o lcdc.o tve_common.o >>> ../videomodes.o >>> -obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o >>> +obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o >>> sunxi_lcd.o >>> diff --git a/drivers/video/sunxi/sunxi_de2.c >>> b/drivers/video/sunxi/sunxi_de2.c >>> index ee67764ac5..a838bbacd1 100644 >>> --- a/drivers/video/sunxi/sunxi_de2.c >>> +++ b/drivers/video/sunxi/sunxi_de2.c >>> @@ -232,6 +232,23 @@ static int sunxi_de2_probe(struct udevice *dev) >>> if (!(gd->flags & GD_FLG_RELOC)) >>> return 0; >>> >>> + 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). >>> + return 0; >>> + } >>> + } >>> + >>> + debug("%s: lcd display not found (ret=%d)\n", __func__, ret); >>> + >>> ret = uclass_find_device_by_name(UCLASS_DISPLAY, >>> "sunxi_dw_hdmi", &disp); >>> if (!ret) { >>> diff --git a/drivers/video/sunxi/sunxi_lcd.c >>> b/drivers/video/sunxi/sunxi_lcd.c >>> new file mode 100644 >>> index 0000000000..154eb5835e >>> --- /dev/null >>> +++ b/drivers/video/sunxi/sunxi_lcd.c >>> @@ -0,0 +1,142 @@ >>> +/* >>> + * Allwinner LCD driver >>> + * >>> + * (C) Copyright 2017 Vasily Khoruzhick <anars...@gmail.com> >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +#include <common.h> >>> +#include <display.h> >>> +#include <video_bridge.h> >>> +#include <backlight.h> >>> +#include <dm.h> >>> +#include <edid.h> >>> +#include <asm/io.h> >>> +#include <asm/arch/clock.h> >>> +#include <asm/arch/lcdc.h> >>> +#include <asm/arch/gpio.h> >>> +#include <asm/gpio.h> >>> + >>> +struct sunxi_lcd_priv { >>> + struct display_timing timing; >>> + int panel_bpp; >>> +}; >>> + >>> +static void sunxi_lcdc_config_pinmux(void) >>> +{ >>> + int pin; >>> + for (pin = SUNXI_GPD(0); pin <= SUNXI_GPD(21); pin++) { >>> + sunxi_gpio_set_cfgpin(pin, SUNXI_GPD_LCD0); >>> + sunxi_gpio_set_drv(pin, 3); >>> + } >>> +} >>> + >>> +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. > >>> + 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. > >>> + lcdc_pll_set(ccm, 0, edid->pixelclock.typ / 1000, >>> + &clk_div, &clk_double); >>> + lcdc_tcon0_mode_set(lcdc, edid, clk_div, false, >>> + priv->panel_bpp, CONFIG_VIDEO_LCD_DCLK_PHASE); >>> + lcdc_enable(lcdc, priv->panel_bpp); >>> + >>> + ret = uclass_get_device(UCLASS_PANEL_BACKLIGHT, 0, &backlight); >>> + if (!ret) >>> + backlight_enable(backlight); >>> + >>> + return 0; >>> +} >>> + >>> +static int sunxi_lcd_read_timing(struct udevice *dev, >>> + struct display_timing *timing) >>> +{ >>> + struct sunxi_lcd_priv *priv = dev_get_priv(dev); >>> + memcpy(timing, &priv->timing, sizeof(struct display_timing)); >>> + >>> + return 0; >>> +} >>> + >>> +static int sunxi_lcd_probe(struct udevice *dev) >>> +{ >>> + struct udevice *cdev; >>> + struct sunxi_lcd_priv *priv = dev_get_priv(dev); >>> + int ret; >>> + >>> + /* make sure that clock is active */ >>> + clock_set_pll10(432000000); >> >> Why do you need it active, and why at that rate? > > Will check. Not necessary, will drop in v2. >>> + >>> +#ifdef CONFIG_VIDEO_BRIDGE >>> + /* Try to get timings from bridge first */ >>> + ret = uclass_get_device(UCLASS_VIDEO_BRIDGE, 0, &cdev); >>> + if (!ret) { >>> + u8 edid[EDID_SIZE]; >>> + int channel_bpp; >>> + >>> + ret = video_bridge_attach(cdev); >>> + if (ret) { >>> + debug("video bridge attach failed: %d\n", ret); >>> + return ret; >>> + } >>> + ret = video_bridge_read_edid(cdev, edid, EDID_SIZE); >>> + if (ret <= 0) { >>> + debug("video bridge failed to read edid: %d\n", ret); >>> + return ret ? ret : -EIO; >>> + } >>> + ret = edid_get_timing(edid, ret, &priv->timing, &channel_bpp); >>> + priv->panel_bpp = channel_bpp * 3; >>> + return ret; >>> + } >>> + debug("video bridge not found: %d\n", ret); >>> +#endif >>> + /* Fallback to timings from DT if there's no bridge */ >>> + ret = uclass_get_device(UCLASS_PANEL, 0, &cdev); >>> + if (ret) { >>> + debug("video panel not found: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + if (fdtdec_decode_display_timing(gd->fdt_blob, dev_of_offset(cdev), >>> + 0, &priv->timing)) { >>> + debug("%s: Failed to decode display timing\n", __func__); >>> + return -EINVAL; >>> + } >> >> How does that work with simple-panel style bindings that are most of >> the panels merged these days? > > It should just work, but I haven't even tested this part of the code - > I don't have a panel with parallel interface nor A64 board to connect > it to. > Do you want me to drop it and make this driver dependent on > CONFIG_VIDEO_BRIDGE I tested missing bridge codepath as Jerjej suggested and it works fine. I extended it with reading panel bpp from fdt to make it even better. >>> + priv->panel_bpp = 16; >>> + >>> + return 0; >>> +} >>> + >>> +static const struct dm_display_ops sunxi_lcd_ops = { >>> + .read_timing = sunxi_lcd_read_timing, >>> + .enable = sunxi_lcd_enable, >>> +}; >>> + >>> +U_BOOT_DRIVER(sunxi_lcd) = { >>> + .name = "sunxi_lcd", >>> + .id = UCLASS_DISPLAY, >>> + .ops = &sunxi_lcd_ops, >>> + .probe = sunxi_lcd_probe, >>> + .priv_auto_alloc_size = sizeof(struct sunxi_lcd_priv), >>> +}; >>> + >>> +#ifdef CONFIG_MACH_SUN50I >> >> Why do you restrict it to the A64 here? > > Because the hardware is present in A64 only. > >> Thanks, >> 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