[PATCH v3 0/9] video: sunxi: rework DE2 driver
Hi Anatolij,
can you please take this series? This is the first part of Jernej's rework, which was checked quite thoroughly. I left out the patches with issues for a later post. I pushed a branch to: https://source.denx.de/u-boot/custodians/u-boot-sunxi/-/commits/de2-fixes-pa...
Thanks, Andre -----------------
This series is the first part of a major rework to the DE2 mixer and accompanying DW-HDMI platform driver. Main goal was to drop redundant device specific code, and later use the DT as a source of information. The latter part has still issues (breaks on A64), so this version only covers the first part of the cleanups.
Besides those cleanups the first patches now filter the HDMI modes based on the pixel clock and search for additional detailed timings in the EDID extension block. This change allows to use 4K monitors - the base EDID block typically presents only a 4k@60 timing, which exceeds the maximum DE2 pixel clock. Other suitable timings, which are appropriate for this HDMI 1.4 compliant controller, are contained in extension block.
Tested on an H5 and an A64 board.
Jernej Skrabec (9): video: sunxi: Add mode_valid callback to sunxi_dw_hdmi common: edid: check for digital display earlier common: edid: extract code for detailed timing search common: edid: Search for valid timing in extension block video: sunxi: Use DW-HDMI hpd function video: sunxi: Remove check for ddc-i2c-bus property video: sunxi: Remove TV probe from DE2 video: sunxi: dw-hdmi: read source_id later video: sunxi: de2: switch to public uclass functions
common/edid.c | 68 ++++++++++++++++++++--------- drivers/video/sunxi/sunxi_de2.c | 44 +++++-------------- drivers/video/sunxi/sunxi_dw_hdmi.c | 48 ++++++-------------- 3 files changed, 72 insertions(+), 88 deletions(-)
From: Jernej Skrabec jernej.skrabec@siol.net
Currently driver accepts all resolution which won't work on 4k screens. Add validation callback which limits acceptable resolutions to 297 MHz.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/video/sunxi/sunxi_dw_hdmi.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/video/sunxi/sunxi_dw_hdmi.c b/drivers/video/sunxi/sunxi_dw_hdmi.c index 0b8cefc311e..e3811a2ec15 100644 --- a/drivers/video/sunxi/sunxi_dw_hdmi.c +++ b/drivers/video/sunxi/sunxi_dw_hdmi.c @@ -305,6 +305,12 @@ static int sunxi_dw_hdmi_read_edid(struct udevice *dev, u8 *buf, int buf_size) return dw_hdmi_read_edid(&priv->hdmi, buf, buf_size); }
+static bool sunxi_dw_hdmi_mode_valid(struct udevice *dev, + const struct display_timing *timing) +{ + return timing->pixelclock.typ <= 297000000; +} + static int sunxi_dw_hdmi_enable(struct udevice *dev, int panel_bpp, const struct display_timing *edid) { @@ -388,6 +394,7 @@ static int sunxi_dw_hdmi_probe(struct udevice *dev) static const struct dm_display_ops sunxi_dw_hdmi_ops = { .read_edid = sunxi_dw_hdmi_read_edid, .enable = sunxi_dw_hdmi_enable, + .mode_valid = sunxi_dw_hdmi_mode_valid, };
U_BOOT_DRIVER(sunxi_dw_hdmi) = {
From: Jernej Skrabec jernej.skrabec@siol.net
When searching for detailed timing in EDID, check for digital display earlier. There is no point parsing other parameters if this flag is not present.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Andre Przywara andre.przywara@arm.com --- common/edid.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/common/edid.c b/common/edid.c index 553ab8fd01a..1cb7177742e 100644 --- a/common/edid.c +++ b/common/edid.c @@ -185,6 +185,11 @@ int edid_get_timing_validate(u8 *buf, int buf_size, return -EINVAL; }
+ if (!EDID1_INFO_VIDEO_INPUT_DIGITAL(*edid)) { + debug("%s: Not a digital display\n", __func__); + return -ENOSYS; + } + if (!EDID1_INFO_FEATURE_PREFERRED_TIMING_MODE(*edid)) { debug("%s: No preferred timing\n", __func__); return -ENOENT; @@ -211,10 +216,6 @@ int edid_get_timing_validate(u8 *buf, int buf_size, if (!timing_done) return -EINVAL;
- if (!EDID1_INFO_VIDEO_INPUT_DIGITAL(*edid)) { - debug("%s: Not a digital display\n", __func__); - return -ENOSYS; - } if (edid->version != 1 || edid->revision < 4) { debug("%s: EDID version %d.%d does not have required info\n", __func__, edid->version, edid->revision);
From: Jernej Skrabec jernej.skrabec@siol.net
Code which searches for valid detailed timing entry will be used in more places. Extract it.
No functional change is made. However, descriptors are casted to edid_detailed_timing instead of edid_monitor_descriptor. Descriptor can be of either type, but since we're interested only in DTD, it is more fitting to cast to edid_detailed_timing.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Andre Przywara andre.przywara@arm.com --- common/edid.c | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-)
diff --git a/common/edid.c b/common/edid.c index 1cb7177742e..a6c875d9c8e 100644 --- a/common/edid.c +++ b/common/edid.c @@ -169,6 +169,29 @@ static bool cea_is_hdmi_vsdb_present(struct edid_cea861_info *info) return false; }
+static bool edid_find_valid_timing(void *buf, int count, + struct display_timing *timing, + bool (*mode_valid)(void *priv, + const struct display_timing *timing), + void *mode_valid_priv) +{ + struct edid_detailed_timing *t = buf; + bool found = false; + int i; + + for (i = 0; i < count && !found; i++, t++) + if (EDID_DETAILED_TIMING_PIXEL_CLOCK(*t) != 0) { + decode_timing((u8 *)t, timing); + if (mode_valid) + found = mode_valid(mode_valid_priv, + timing); + else + found = true; + } + + return found; +} + int edid_get_timing_validate(u8 *buf, int buf_size, struct display_timing *timing, int *panel_bits_per_colourp, @@ -177,8 +200,7 @@ int edid_get_timing_validate(u8 *buf, int buf_size, void *mode_valid_priv) { struct edid1_info *edid = (struct edid1_info *)buf; - bool timing_done; - int i; + bool found;
if (buf_size < sizeof(*edid) || edid_check_info(edid)) { debug("%s: Invalid buffer\n", __func__); @@ -195,25 +217,10 @@ int edid_get_timing_validate(u8 *buf, int buf_size, return -ENOENT; }
- /* Look for detailed timing */ - timing_done = false; - for (i = 0; i < 4; i++) { - struct edid_monitor_descriptor *desc; - - desc = &edid->monitor_details.descriptor[i]; - if (desc->zero_flag_1 != 0) { - decode_timing((u8 *)desc, timing); - if (mode_valid) - timing_done = mode_valid(mode_valid_priv, - timing); - else - timing_done = true; - - if (timing_done) - break; - } - } - if (!timing_done) + /* Look for detailed timing in base EDID */ + found = edid_find_valid_timing(edid->monitor_details.descriptor, 4, + timing, mode_valid, mode_valid_priv); + if (!found) return -EINVAL;
if (edid->version != 1 || edid->revision < 4) {
From: Jernej Skrabec jernej.skrabec@siol.net
One of my monitors have only 4k@60 timing in base EDID block which is out of range for devices with HDMI 1.4. It turns out that it has additional detailed timings in CTA-861 Extension Block and two of them are appropriate for HDMI 1.4.
Add additional search for valid detailed timing in extension block.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Acked-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Andre Przywara andre.przywara@arm.com --- common/edid.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/common/edid.c b/common/edid.c index a6c875d9c8e..14d8836c360 100644 --- a/common/edid.c +++ b/common/edid.c @@ -220,6 +220,24 @@ int edid_get_timing_validate(u8 *buf, int buf_size, /* Look for detailed timing in base EDID */ found = edid_find_valid_timing(edid->monitor_details.descriptor, 4, timing, mode_valid, mode_valid_priv); + + /* Look for detailed timing in CTA-861 Extension Block */ + if (!found && edid->extension_flag && buf_size >= EDID_EXT_SIZE) { + struct edid_cea861_info *info = + (struct edid_cea861_info *)(buf + sizeof(*edid)); + + if (info->extension_tag == EDID_CEA861_EXTENSION_TAG) { + int count = EDID_CEA861_DTD_COUNT(*info); + int offset = info->dtd_offset; + int size = count * sizeof(struct edid_detailed_timing); + + if (offset >= 4 && offset + size < EDID_SIZE) + found = edid_find_valid_timing( + (u8*)info + offset, count, timing, + mode_valid, mode_valid_priv); + } + } + if (!found) return -EINVAL;
Hi,
On 22/04/2021 02:14, Andre Przywara wrote:
From: Jernej Skrabec jernej.skrabec@siol.net
One of my monitors have only 4k@60 timing in base EDID block which is out of range for devices with HDMI 1.4. It turns out that it has additional detailed timings in CTA-861 Extension Block and two of them are appropriate for HDMI 1.4.
Add additional search for valid detailed timing in extension block.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Acked-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Andre Przywara andre.przywara@arm.com
common/edid.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/common/edid.c b/common/edid.c index a6c875d9c8e..14d8836c360 100644 --- a/common/edid.c +++ b/common/edid.c @@ -220,6 +220,24 @@ int edid_get_timing_validate(u8 *buf, int buf_size, /* Look for detailed timing in base EDID */ found = edid_find_valid_timing(edid->monitor_details.descriptor, 4, timing, mode_valid, mode_valid_priv);
- /* Look for detailed timing in CTA-861 Extension Block */
- if (!found && edid->extension_flag && buf_size >= EDID_EXT_SIZE) {
struct edid_cea861_info *info =(struct edid_cea861_info *)(buf + sizeof(*edid));if (info->extension_tag == EDID_CEA861_EXTENSION_TAG) {int count = EDID_CEA861_DTD_COUNT(*info);int offset = info->dtd_offset;int size = count * sizeof(struct edid_detailed_timing);if (offset >= 4 && offset + size < EDID_SIZE)found = edid_find_valid_timing((u8*)info + offset, count, timing,mode_valid, mode_valid_priv);}- }
- if (!found) return -EINVAL;
Thanks for doing that, we had the same issue on Amlogic SoCs !
Neil
From: Jernej Skrabec jernej.skrabec@siol.net
It turns out that there are two ways how hot plug detection can be done. One is standard way for DW HDMI controller - checking bit 2 in 0x3004 register. Another way is applicable only to Allwinner custom PHY - by checking bit 19 in register 0x10038. Both methods are equally good as far as we know.
Use standard method in order to reduce amount of custom code.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/video/sunxi/sunxi_dw_hdmi.c | 34 +++++------------------------ 1 file changed, 6 insertions(+), 28 deletions(-)
diff --git a/drivers/video/sunxi/sunxi_dw_hdmi.c b/drivers/video/sunxi/sunxi_dw_hdmi.c index e3811a2ec15..37e78ff2411 100644 --- a/drivers/video/sunxi/sunxi_dw_hdmi.c +++ b/drivers/video/sunxi/sunxi_dw_hdmi.c @@ -114,28 +114,6 @@ static void sunxi_dw_hdmi_phy_init(void) writel(0x42494E47, &phy->unscramble); }
-static int sunxi_dw_hdmi_get_plug_in_status(void) -{ - struct sunxi_hdmi_phy * const phy = - (struct sunxi_hdmi_phy *)(SUNXI_HDMI_BASE + HDMI_PHY_OFFS); - - return !!(readl(&phy->status) & (1 << 19)); -} - -static int sunxi_dw_hdmi_wait_for_hpd(void) -{ - ulong start; - - start = get_timer(0); - do { - if (sunxi_dw_hdmi_get_plug_in_status()) - return 0; - udelay(100); - } while (get_timer(start) < 300); - - return -1; -} - static void sunxi_dw_hdmi_phy_set(uint clock, int phy_div) { struct sunxi_hdmi_phy * const phy = @@ -370,12 +348,6 @@ static int sunxi_dw_hdmi_probe(struct udevice *dev)
sunxi_dw_hdmi_phy_init();
- ret = sunxi_dw_hdmi_wait_for_hpd(); - if (ret < 0) { - debug("hdmi can not get hpd signal\n"); - return -1; - } - priv->hdmi.ioaddr = SUNXI_HDMI_BASE; priv->hdmi.i2c_clk_high = 0xd8; priv->hdmi.i2c_clk_low = 0xfe; @@ -383,6 +355,12 @@ static int sunxi_dw_hdmi_probe(struct udevice *dev) priv->hdmi.phy_set = sunxi_dw_hdmi_phy_cfg; priv->mux = uc_plat->source_id;
+ ret = dw_hdmi_phy_wait_for_hpd(&priv->hdmi); + if (ret < 0) { + debug("hdmi can not get hpd signal\n"); + return -1; + } + uclass_get_device_by_phandle(UCLASS_I2C, dev, "ddc-i2c-bus", &priv->hdmi.ddc_bus);
From: Jernej Skrabec jernej.skrabec@siol.net
No Allwinner board with DW-HDMI controller use separate I2C bus for EDID read. Remove that check.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/video/sunxi/sunxi_dw_hdmi.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/video/sunxi/sunxi_dw_hdmi.c b/drivers/video/sunxi/sunxi_dw_hdmi.c index 37e78ff2411..6d2bc206fc2 100644 --- a/drivers/video/sunxi/sunxi_dw_hdmi.c +++ b/drivers/video/sunxi/sunxi_dw_hdmi.c @@ -361,9 +361,6 @@ static int sunxi_dw_hdmi_probe(struct udevice *dev) return -1; }
- uclass_get_device_by_phandle(UCLASS_I2C, dev, "ddc-i2c-bus", - &priv->hdmi.ddc_bus); - dw_hdmi_init(&priv->hdmi);
return 0;
From: Jernej Skrabec jernej.skrabec@siol.net
TV driver was never fully implemented. Remove search for it from DE2 driver.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/video/sunxi/sunxi_de2.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/video/sunxi/sunxi_de2.c b/drivers/video/sunxi/sunxi_de2.c index a3e21aa5f13..6b836a01194 100644 --- a/drivers/video/sunxi/sunxi_de2.c +++ b/drivers/video/sunxi/sunxi_de2.c @@ -281,20 +281,7 @@ static int sunxi_de2_probe(struct udevice *dev)
debug("%s: hdmi display not found (ret=%d)\n", __func__, ret);
- ret = uclass_find_device_by_name(UCLASS_DISPLAY, - "sunxi_tve", &disp); - if (ret) { - debug("%s: tv not found (ret=%d)\n", __func__, ret); - return ret; - } - - ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, 1, true); - if (ret) - return ret; - - video_set_flush_dcache(dev, 1); - - return 0; + return -ENODEV; }
static int sunxi_de2_bind(struct udevice *dev)
From: Jernej Skrabec jernej.skrabec@siol.net
There is no real need to read source_id at probe time. It also doesn't make sense to store it in driver private data since it's already stored in class platform data. While this looks like cleanup (and it is), it's also important for DE2 driver rework because this info will be filled later (after probe is already executed).
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/video/sunxi/sunxi_dw_hdmi.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/video/sunxi/sunxi_dw_hdmi.c b/drivers/video/sunxi/sunxi_dw_hdmi.c index 6d2bc206fc2..19ed80b48a4 100644 --- a/drivers/video/sunxi/sunxi_dw_hdmi.c +++ b/drivers/video/sunxi/sunxi_dw_hdmi.c @@ -20,7 +20,6 @@
struct sunxi_dw_hdmi_priv { struct dw_hdmi hdmi; - int mux; };
struct sunxi_hdmi_phy { @@ -294,6 +293,7 @@ static int sunxi_dw_hdmi_enable(struct udevice *dev, int panel_bpp, { struct sunxi_hdmi_phy * const phy = (struct sunxi_hdmi_phy *)(SUNXI_HDMI_BASE + HDMI_PHY_OFFS); + struct display_plat *uc_plat = dev_get_uclass_plat(dev); struct sunxi_dw_hdmi_priv *priv = dev_get_priv(dev); int ret;
@@ -301,7 +301,7 @@ static int sunxi_dw_hdmi_enable(struct udevice *dev, int panel_bpp, if (ret) return ret;
- sunxi_dw_hdmi_lcdc_init(priv->mux, edid, panel_bpp); + sunxi_dw_hdmi_lcdc_init(uc_plat->source_id, edid, panel_bpp);
if (edid->flags & DISPLAY_FLAGS_VSYNC_LOW) setbits_le32(&phy->pol, 0x200); @@ -324,7 +324,6 @@ static int sunxi_dw_hdmi_enable(struct udevice *dev, int panel_bpp,
static int sunxi_dw_hdmi_probe(struct udevice *dev) { - struct display_plat *uc_plat = dev_get_uclass_plat(dev); struct sunxi_dw_hdmi_priv *priv = dev_get_priv(dev); struct sunxi_ccm_reg * const ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE; @@ -353,7 +352,6 @@ static int sunxi_dw_hdmi_probe(struct udevice *dev) priv->hdmi.i2c_clk_low = 0xfe; priv->hdmi.reg_io_width = 1; priv->hdmi.phy_set = sunxi_dw_hdmi_phy_cfg; - priv->mux = uc_plat->source_id;
ret = dw_hdmi_phy_wait_for_hpd(&priv->hdmi); if (ret < 0) {
From: Jernej Skrabec jernej.skrabec@siol.net
Currently DE2 driver uses functions which are defined in internal headers. They are not meant to be used outside of uclass framework. Switch DE2 driver to public ones. This has additional benefit that device_probe doesn't need to be called manually.
Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/video/sunxi/sunxi_de2.c | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/drivers/video/sunxi/sunxi_de2.c b/drivers/video/sunxi/sunxi_de2.c index 6b836a01194..e02d359cd25 100644 --- a/drivers/video/sunxi/sunxi_de2.c +++ b/drivers/video/sunxi/sunxi_de2.c @@ -19,8 +19,6 @@ #include <asm/io.h> #include <asm/arch/clock.h> #include <asm/arch/display2.h> -#include <dm/device-internal.h> -#include <dm/uclass-internal.h> #include <linux/bitops.h> #include "simplefb_common.h"
@@ -198,13 +196,6 @@ static int sunxi_de2_init(struct udevice *dev, ulong fbbase,
disp_uc_plat->source_id = mux;
- ret = device_probe(disp); - if (ret) { - debug("%s: device '%s' display won't probe (ret=%d)\n", - __func__, dev->name, ret); - return ret; - } - ret = display_read_timing(disp, &timing); if (ret) { debug("%s: Failed to read timings\n", __func__); @@ -245,8 +236,8 @@ 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); + ret = uclass_get_device_by_driver(UCLASS_DISPLAY, + DM_DRIVER_GET(sunxi_lcd), &disp); if (!ret) { int mux;
@@ -262,8 +253,8 @@ static int sunxi_de2_probe(struct udevice *dev)
debug("%s: lcd display not found (ret=%d)\n", __func__, ret);
- ret = uclass_find_device_by_name(UCLASS_DISPLAY, - "sunxi_dw_hdmi", &disp); + ret = uclass_get_device_by_driver(UCLASS_DISPLAY, + DM_DRIVER_GET(sunxi_dw_hdmi), &disp); if (!ret) { int mux; if (IS_ENABLED(CONFIG_MACH_SUNXI_H3_H5)) @@ -332,8 +323,8 @@ int sunxi_simplefb_setup(void *blob) mux = 1;
/* Skip simplefb setting if DE2 / HDMI is not present */ - ret = uclass_find_device_by_name(UCLASS_VIDEO, - "sunxi_de2", &de2); + ret = uclass_get_device_by_driver(UCLASS_VIDEO, + DM_DRIVER_GET(sunxi_de2), &de2); if (ret) { debug("DE2 not present\n"); return 0; @@ -342,8 +333,8 @@ int sunxi_simplefb_setup(void *blob) return 0; }
- ret = uclass_find_device_by_name(UCLASS_DISPLAY, - "sunxi_dw_hdmi", &hdmi); + ret = uclass_get_device_by_driver(UCLASS_DISPLAY, + DM_DRIVER_GET(sunxi_dw_hdmi), &hdmi); if (ret) { debug("HDMI not present\n"); } else if (device_active(hdmi)) { @@ -355,8 +346,8 @@ int sunxi_simplefb_setup(void *blob) debug("HDMI present but not probed\n"); }
- ret = uclass_find_device_by_name(UCLASS_DISPLAY, - "sunxi_lcd", &lcd); + ret = uclass_get_device_by_driver(UCLASS_DISPLAY, + DM_DRIVER_GET(sunxi_lcd), &lcd); if (ret) debug("LCD not present\n"); else if (device_active(lcd))
On Thu, 22 Apr 2021 01:14:25 +0100 Andre Przywara andre.przywara@arm.com wrote: ...
Jernej Skrabec (9): video: sunxi: Add mode_valid callback to sunxi_dw_hdmi common: edid: check for digital display earlier common: edid: extract code for detailed timing search common: edid: Search for valid timing in extension block video: sunxi: Use DW-HDMI hpd function video: sunxi: Remove check for ddc-i2c-bus property video: sunxi: Remove TV probe from DE2 video: sunxi: dw-hdmi: read source_id later video: sunxi: de2: switch to public uclass functions
common/edid.c | 68 ++++++++++++++++++++--------- drivers/video/sunxi/sunxi_de2.c | 44 +++++-------------- drivers/video/sunxi/sunxi_dw_hdmi.c | 48 ++++++-------------- 3 files changed, 72 insertions(+), 88 deletions(-)
Series applied to u-boot-video/master, thanks!
-- Anatolij
participants (3)
-
Anatolij Gustschin -
Andre Przywara -
Neil Armstrong