Hi,
I did few tests, see results inline.
On Tue, Sep 19, 2017 at 12:00 PM, Vasily Khoruzhick anarsoul@gmail.com wrote:
On Tue, Sep 19, 2017 at 1:33 AM, Maxime Ripard maxime.ripard@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 anarsoul@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 anarsoul@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