diff mbox series

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

Message ID 20170919050421.23172-5-anarsoul@gmail.com
State Superseded
Delegated to: Anatolij Gustschin
Headers show
Series [U-Boot,RESEND,1/5] dm: video: bridge: add operation to read EDID | expand

Commit Message

Vasily Khoruzhick Sept. 19, 2017, 5:04 a.m. UTC
Extend DE2 driver with LCD support

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

Comments

Maxime Ripard Sept. 19, 2017, 8:33 a.m. UTC | #1
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
Vasily Khoruzhick Sept. 19, 2017, 7 p.m. UTC | #2
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?

>
>> +                     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).

>> +     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.

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

>
>> +     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
Jernej Škrabec Sept. 19, 2017, 8:06 p.m. UTC | #3
Hi,

Dne torek, 19. september 2017 ob 21:00:30 CEST je Vasily Khoruzhick 
napisal(a):
> 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?
> 

When I was developing HDMI driver, it proved necessary, since screen was not 
rendered correctly otherwise. I guess simple test without this line would show 
if it is really necessary or not.

> >> +                     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).
> 
> >> +     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.
> 

Is this taken from my not yet mainline TVE driver? There it was needed since 
TVE unit clock has same parent as DE2 clock and since DE2 was not yet set up 
at that time, TVE plug in detection didn't work.

If I'm not mistaken, there is no special encoder for LCD interface after TCON, 
so this line should not be needed. Again, simple test can be performed.

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

You don't need separate setup. Just add simple panel to DT which describes 
current panel and instead of edid_get_timing() use 
fdtdec_decode_display_timing()...

> >> +     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.

The only other device with DE2 and LCD interface is R40 (not counting 
rebranded SoCs), which is not yet supported with this driver. It would be bad 
to probe it on H3 and H5, since they have slightly different clock registers.

Best regards,
Jernej
Icenowy Zheng Sept. 19, 2017, 11:26 p.m. UTC | #4
于 2017年9月20日 GMT+08:00 上午3:00:30, Vasily Khoruzhick <anarsoul@gmail.com> 写到:
>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

The only display output of V3s is LCD.

>
>>   - 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?
>
>>
>>> +                     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).
>
>>> +     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.
>
>>> +
>>> +#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
>
>>
>>> +     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
Vasily Khoruzhick Sept. 21, 2017, 5:33 a.m. UTC | #5
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
Maxime Ripard Sept. 21, 2017, 6:51 a.m. UTC | #6
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?

> >>> +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.

> >>> +     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.

Maxime
Icenowy Zheng Sept. 21, 2017, 7:01 a.m. UTC | #7
在 2017-09-21 14:51,Maxime Ripard 写道:
> 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?

I think currently in U-Boot all DRAM is mapped cacheable.

> 
>> >>> +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.

All TCONs come with DE2 are single-channel, either channel0 or channel1.

On A64 TCON0 has channel0 and TCON1 has channel1.

> 
>> >>> +     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.
> 
> Maxime
Vasily Khoruzhick Sept. 22, 2017, 4:42 a.m. UTC | #8
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
Maxime Ripard Sept. 22, 2017, 2:44 p.m. UTC | #9
On Fri, Sep 22, 2017 at 04:42:24AM +0000, Vasily Khoruzhick wrote:
> >> >>> +     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.

Again, DE1 vs DE2 has nothing to do in the discussion

DE1 devices will look like this:
DE1 -> TCON -> HDMI / LCD / whatever -> pins

DE2 will be:
DE2 -> TCON -> HDMI / LCD / whatever -> pins

The only thing not in common between these two cases is the display
engine used. Everything else should be common (except for special
cases, like the HDMI controller itself that changed as well).

Maxime
Vasily Khoruzhick Sept. 22, 2017, 3:45 p.m. UTC | #10
After discussing it with Maxime in IRC I decided to wait till pinctrl
driver for sunxi is ready.

Please kindly disregard this series.

Regards,
Vasily

On Fri, Sep 22, 2017 at 7:44 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Fri, Sep 22, 2017 at 04:42:24AM +0000, Vasily Khoruzhick wrote:
>> >> >>> +     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.
>
> Again, DE1 vs DE2 has nothing to do in the discussion
>
> DE1 devices will look like this:
> DE1 -> TCON -> HDMI / LCD / whatever -> pins
>
> DE2 will be:
> DE2 -> TCON -> HDMI / LCD / whatever -> pins
>
> The only thing not in common between these two cases is the display
> engine used. Everything else should be common (except for special
> cases, like the HDMI controller itself that changed as well).
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
diff mbox series

Patch

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);
+			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);
+
+	lcdc_init(lcdc);
+	sunxi_lcdc_config_pinmux();
+	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);
+
+#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;
+	}
+	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
+U_BOOT_DEVICE(sunxi_lcd) = {
+	.name = "sunxi_lcd"
+};
+#endif