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

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.

Here, for example, explaining the following things would help: - Why are you creating yet another file - What is the situation with old Allwinner SoCs that should share the same code - What are the expected users - Which SoC / board have you tested it on

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?

• 	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().

• lcdc_init(lcdc);
• sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

• 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?

+#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?

• 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?

Thanks, Maxime