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?
>
>> + 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).
>> + 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
