On Sun, 7 Feb 2021 07:37:34 -0700 Simon Glass sjg@chromium.org wrote:
Hi Simon,
On Thu, 4 Feb 2021 at 18:08, Andre Przywara andre.przywara@arm.com wrote:
From: Jagan Teki jagan@amarulasolutions.com
DM_VIDEO migration deadline is already expired, but around 80 Allwinner boards are still using video in a legacy way.
===================== WARNING ====================== This board does not use CONFIG_DM_VIDEO Please update the board to use CONFIG_DM_VIDEO before the v2019.07 release. Failure to update by the deadline may result in board removal. See doc/driver-model/migration.rst for more info. ====================================================
Convert the legacy video driver over to the DM_VIDEO framework. This is a minimal conversion: it doesn't use the DT for finding its resources, nor does it use DM clocks or DM devices for the outputs (LCD, HDMI, CVBS).
Tested in Bananapi M1+ Plus 1920x1200 HDMI out. (Jagan)
Signed-off-by: Jagan Teki jagan@amarulasolutions.com [Andre: rebase and smaller fixes] Signed-off-by: Andre Przywara andre.przywara@arm.com
Hi,
I picked this one up to get rid of the warnings. I dropped the BMP support for now (v2 1/3 and v2 2/3), I need to have a closer look, as I am not convinced this was the right solution.
Cheers, Andre
Changelog v2 .. v3:
- rebase against master, fixing up renamed variables and structs
- replace enum with #define
- remove BMP from Kconfig
- fix memory node size calculation in simplefb setup
arch/arm/mach-sunxi/Kconfig | 9 +- drivers/video/sunxi/sunxi_display.c | 262 ++++++++++++++++------------ include/configs/sunxi-common.h | 17 -- scripts/config_whitelist.txt | 1 - 4 files changed, 157 insertions(+), 132 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Some thoughts below
Many thanks for having a thorough look, much appreciated.
I will have a closer look at your comments which I didn't reply to below.
For the other points: To be honest, I haven't checked every line in this patch, my goal was merely to get it merged (this time), to prevent feature removal and drop the nasty buildman warnings. I see quite some cleanup potential here (some I already mentioned in the commit message above), but wanted to get the main DM_VIDEO conversion done first (as I think last time some discussion already prevented a merge). So my plan was to queue this for next ASAP, then send more cleanup patches on top, before the next merge window. I understand that it's typically done the other way around, but given this is dragging on for a while, is long overdue and works for me (TM) as it is, I would prefer a functional patch first, with cleanups on top.
diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig index 0135575ca1e..a29d11505aa 100644 --- a/arch/arm/mach-sunxi/Kconfig +++ b/arch/arm/mach-sunxi/Kconfig @@ -816,13 +816,14 @@ config VIDEO_SUNXI depends on !MACH_SUN9I depends on !MACH_SUN50I depends on !SUN50I_GEN_H6
select VIDEO
select DM_VIDEOI wonder whether instead VIDEO_SUNXI should depend on DM_VIDEO ?
So I was always wondering about the logic behind that. My understanding would be: This driver is (now) implementing the DM_VIDEO interface. Any user (at board or SoC level) would just be selecting this driver, without caring about which driver interface it uses. So as this driver is (now) DM_VIDEO only, it would signal this via this select line.
If that is not how it's meant to be, can you explain the the idea behind this, please?
Also: with CONFIG_VIDEO now dying, CONFIG_DM_VIDEO somewhat loses its purpose, doesn't it?
select DISPLAY imply VIDEO_DT_SIMPLEFB default y ---help---
Say Y here to add support for using a cfb console on the HDMI, LCDor VGA output found on most sunxi devices. See doc/README.video forinfo on how to select the video output and mode.
Say Y here to add support for using a graphical console on the HDMI,LCD or VGA output found on older sunxi devices. This will also providea simple_framebuffer device for Linux.config VIDEO_HDMI bool "HDMI output support" diff --git a/drivers/video/sunxi/sunxi_display.c b/drivers/video/sunxi/sunxi_display.c index f52aba4d21c..61498d1642f 100644 --- a/drivers/video/sunxi/sunxi_display.c +++ b/drivers/video/sunxi/sunxi_display.c @@ -7,6 +7,8 @@ */
#include <common.h> +#include <display.h> +#include <dm.h> #include <cpu_func.h> #include <efi_loader.h> #include <init.h> @@ -28,7 +30,9 @@ #include <fdt_support.h> #include <i2c.h> #include <malloc.h> +#include <video.h> #include <video_fb.h> +#include <dm/uclass-internal.h>
Do you need that? Internal things should be avoided if posssible.
#include "../videomodes.h" #include "../anx9804.h" #include "../hitachi_tx18d42vm_lcd.h" @@ -45,6 +49,11 @@
DECLARE_GLOBAL_DATA_PTR;
+/* Maximum LCD size we support */ +#define LCD_MAX_WIDTH 3840 +#define LCD_MAX_HEIGHT 2160 +#define LCD_MAX_LOG2_BPP VIDEO_BPP32
enum sunxi_monitor { sunxi_monitor_none, sunxi_monitor_dvi, @@ -59,12 +68,11 @@ enum sunxi_monitor { #define SUNXI_MONITOR_LAST sunxi_monitor_composite_pal_nc
struct sunxi_display {
GraphicDevice graphic_device; enum sunxi_monitor monitor; unsigned int depth; unsigned int fb_addr; unsigned int fb_size;These last three are repeated from the uclass info. But fb_addr seems to sometimes be different from the ucass frame buffer, in which case I wonder how output actually works.
If you do actually need these, can you please document them here?
-} sunxi_display; +};
const struct ctfb_res_modes composite_video_modes[2] = { /* x y hz pixclk ps/kHz le ri up lo hs vs s vmode */
[..]
-static void sunxi_mode_set(const struct ctfb_res_modes *mode, +static void sunxi_mode_set(struct sunxi_display *sunxi_display,
const struct ctfb_res_modes *mode, unsigned int address){
enum sunxi_monitor monitor = sunxi_display->monitor; int __maybe_unused clk_div, clk_double; struct sunxi_lcdc_reg * const lcdc = (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE; struct sunxi_tve_reg * __maybe_unused const tve = (struct sunxi_tve_reg *)SUNXI_TVE0_BASE;
switch (sunxi_display.monitor) {
switch (sunxi_display->monitor) { case sunxi_monitor_none: break; case sunxi_monitor_dvi: case sunxi_monitor_hdmi:#ifdef CONFIG_VIDEO_HDMI
I wonder if all of these can use IS_ENABLED()?
Yes, in fact I have patches to replace all #ifdefs with IS_ENABLED(), except the sun6i reset gate ones (which would break compilation on non-sun6i). And those should be tackled by proper UCLASS_CLK usage. But I wanted to get this basic conversion out first, see above.
The rest of your comments make sense on the first glance, I will try to fix the easy things for a v4.
Cheers, Andre
sunxi_composer_mode_set(mode, address);sunxi_lcdc_tcon1_mode_set(mode, &clk_div, &clk_double, 0);sunxi_hdmi_mode_set(mode, clk_div, clk_double);
sunxi_composer_mode_set(mode, address, monitor);sunxi_lcdc_tcon1_mode_set(mode, &clk_div, &clk_double, 0, monitor);sunxi_hdmi_mode_set(mode, clk_div, clk_double, monitor); sunxi_composer_enable();
lcdc_enable(lcdc, sunxi_display.depth);
lcdc_enable(lcdc, sunxi_display->depth); sunxi_hdmi_enable();#endif break;
[..]
-void *video_hw_init(void) +static int sunxi_de_probe(struct udevice *dev)
This function is way too long and would benefit from splitting into several parts IMO.
{
static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
struct video_priv *uc_priv = dev_get_uclass_priv(dev);struct video_uc_plat *plat = dev_get_uclass_plat(dev);struct sunxi_display *sunxi_display = dev_get_priv(dev); const struct ctfb_res_modes *mode; struct ctfb_res_modes custom; const char *options;@@ -1067,10 +1079,8 @@ void *video_hw_init(void) char mon[16]; char *lcd_mode = CONFIG_VIDEO_LCD_MODE;
memset(&sunxi_display, 0, sizeof(struct sunxi_display));video_get_ctfb_res_modes(RES_MODE_1024x768, 24, &mode,&sunxi_display.depth, &options);
&sunxi_display->depth, &options);#ifdef CONFIG_VIDEO_HDMI hpd = video_get_option_int(options, "hpd", 1); hpd_delay = video_get_option_int(options, "hpd_delay", 500); @@ -1078,35 +1088,35 @@ void *video_hw_init(void) #endif overscan_x = video_get_option_int(options, "overscan_x", -1); overscan_y = video_get_option_int(options, "overscan_y", -1);
sunxi_display.monitor = sunxi_get_default_mon(true);
sunxi_display->monitor = sunxi_get_default_mon(true); video_get_option_string(options, "monitor", mon, sizeof(mon),
sunxi_get_mon_desc(sunxi_display.monitor));
sunxi_get_mon_desc(sunxi_display->monitor)); for (i = 0; i <= SUNXI_MONITOR_LAST; i++) { if (strcmp(mon, sunxi_get_mon_desc(i)) == 0) {
sunxi_display.monitor = i;
sunxi_display->monitor = i; break; } } if (i > SUNXI_MONITOR_LAST) printf("Unknown monitor: '%s', falling back to '%s'\n",
mon, sunxi_get_mon_desc(sunxi_display.monitor));
mon, sunxi_get_mon_desc(sunxi_display->monitor));#ifdef CONFIG_VIDEO_HDMI /* If HDMI/DVI is selected do HPD & EDID, and handle fallback */
if (sunxi_display.monitor == sunxi_monitor_dvi ||sunxi_display.monitor == sunxi_monitor_hdmi) {
if (sunxi_display->monitor == sunxi_monitor_dvi ||sunxi_display->monitor == sunxi_monitor_hdmi) { /* Always call hdp_detect, as it also enables clocks, etc. */ hdmi_present = (sunxi_hdmi_hpd_detect(hpd_delay) == 1); if (hdmi_present && edid) { printf("HDMI connected: ");
if (sunxi_hdmi_edid_get_mode(&custom, true) == 0)
if (sunxi_hdmi_edid_get_mode(sunxi_display, &custom, true) == 0) mode = &custom; else hdmi_present = false; } /* Fall back to EDID in case HPD failed */ if (edid && !hdmi_present) {
if (sunxi_hdmi_edid_get_mode(&custom, false) == 0) {
if (sunxi_hdmi_edid_get_mode(sunxi_display, &custom, false) == 0) { mode = &custom; hdmi_present = true; }@@ -1114,38 +1124,39 @@ void *video_hw_init(void) /* Shut down when display was not found */ if ((hpd || edid) && !hdmi_present) { sunxi_hdmi_shutdown();
sunxi_display.monitor = sunxi_get_default_mon(false);
sunxi_display->monitor = sunxi_get_default_mon(false); } /* else continue with hdmi/dvi without a cable connected */ }#endif
switch (sunxi_display.monitor) {
switch (sunxi_display->monitor) { case sunxi_monitor_none:
return NULL;
printf("Unknown monitor\n");return -EINVAL; case sunxi_monitor_dvi: case sunxi_monitor_hdmi: if (!sunxi_has_hdmi()) { printf("HDMI/DVI not supported on this board\n");
sunxi_display.monitor = sunxi_monitor_none;return NULL;
sunxi_display->monitor = sunxi_monitor_none;return -EINVAL; } break; case sunxi_monitor_lcd: if (!sunxi_has_lcd()) { printf("LCD not supported on this board\n");
sunxi_display.monitor = sunxi_monitor_none;return NULL;
sunxi_display->monitor = sunxi_monitor_none;return -EINVAL; }
sunxi_display.depth = video_get_params(&custom, lcd_mode);
sunxi_display->depth = video_get_params(&custom, lcd_mode); mode = &custom; break; case sunxi_monitor_vga: if (!sunxi_has_vga()) { printf("VGA not supported on this board\n");
sunxi_display.monitor = sunxi_monitor_none;return NULL;
sunxi_display->monitor = sunxi_monitor_none;return -EINVAL; }
sunxi_display.depth = 18;
sunxi_display->depth = 18; break; case sunxi_monitor_composite_pal: case sunxi_monitor_composite_ntsc:@@ -1153,85 +1164,103 @@ void *video_hw_init(void) case sunxi_monitor_composite_pal_nc: if (!sunxi_has_composite()) { printf("Composite video not supported on this board\n");
sunxi_display.monitor = sunxi_monitor_none;return NULL;
sunxi_display->monitor = sunxi_monitor_none;return -EINVAL; }
if (sunxi_display.monitor == sunxi_monitor_composite_pal ||sunxi_display.monitor == sunxi_monitor_composite_pal_nc)
if (sunxi_display->monitor == sunxi_monitor_composite_pal ||sunxi_display->monitor == sunxi_monitor_composite_pal_nc) mode = &composite_video_modes[0]; else mode = &composite_video_modes[1];
sunxi_display.depth = 24;
sunxi_display->depth = 24; break; } /* Yes these defaults are quite high, overscan on composite sucks... */ if (overscan_x == -1)
overscan_x = sunxi_is_composite() ? 32 : 0;
overscan_x = sunxi_is_composite(sunxi_display->monitor) ? 32 : 0; if (overscan_y == -1)
overscan_y = sunxi_is_composite() ? 20 : 0;
overscan_y = sunxi_is_composite(sunxi_display->monitor) ? 20 : 0;
sunxi_display.fb_size =(mode->xres * mode->yres * 4 + 0xfff) & ~0xfff;
sunxi_display->fb_size = plat->size; overscan_offset = (overscan_y * mode->xres + overscan_x) * 4; /* We want to keep the fb_base for simplefb page aligned, where as * the sunxi dma engines will happily accept an unaligned address. */ if (overscan_offset)
sunxi_display.fb_size += 0x1000;if (sunxi_display.fb_size > CONFIG_SUNXI_MAX_FB_SIZE) {printf("Error need %dkB for fb, but only %dkB is reserved\n",sunxi_display.fb_size >> 10,CONFIG_SUNXI_MAX_FB_SIZE >> 10);return NULL;}
sunxi_display->fb_size += 0x1000; printf("Setting up a %dx%d%s %s console (overscan %dx%d)\n", mode->xres, mode->yres, (mode->vmode == FB_VMODE_INTERLACED) ? "i" : "",
sunxi_get_mon_desc(sunxi_display.monitor),
sunxi_get_mon_desc(sunxi_display->monitor), overscan_x, overscan_y);
gd->fb_base = gd->bd->bi_dram[0].start +gd->bd->bi_dram[0].size - sunxi_display.fb_size;
sunxi_display->fb_addr = plat->base; sunxi_engines_init();#ifdef CONFIG_EFI_LOADER
efi_add_memory_map(gd->fb_base, sunxi_display.fb_size,
efi_add_memory_map(sunxi_display->fb_addr, sunxi_display->fb_size, EFI_RESERVED_MEMORY_TYPE);#endif
fb_dma_addr = gd->fb_base - CONFIG_SYS_SDRAM_BASE;sunxi_display.fb_addr = gd->fb_base;
fb_dma_addr = sunxi_display->fb_addr - CONFIG_SYS_SDRAM_BASE; if (overscan_offset) { fb_dma_addr += 0x1000 - (overscan_offset & 0xfff);
sunxi_display.fb_addr += (overscan_offset + 0xfff) & ~0xfff;memset((void *)gd->fb_base, 0, sunxi_display.fb_size);flush_cache(gd->fb_base, sunxi_display.fb_size);
sunxi_display->fb_addr += (overscan_offset + 0xfff) & ~0xfff;ALIGN(overscan_offset, 0x100) ?
memset((void *)sunxi_display->fb_addr, 0, sunxi_display->fb_size);flush_cache(sunxi_display->fb_addr, sunxi_display->fb_size);Driver model clears the frame buffer. Is this needed?
}
sunxi_mode_set(mode, fb_dma_addr);
sunxi_mode_set(sunxi_display, mode, fb_dma_addr); /* * These are the only members of this structure that are used. All the * others are driver specific. The pitch is stored in plnSizeX. */
graphic_device->frameAdrs = sunxi_display.fb_addr;graphic_device->gdfIndex = GDF_32BIT_X888RGB;graphic_device->gdfBytesPP = 4;graphic_device->winSizeX = mode->xres - 2 * overscan_x;graphic_device->winSizeY = mode->yres - 2 * overscan_y;graphic_device->plnSizeX = mode->xres * graphic_device->gdfBytesPP;return graphic_device;
uc_priv->bpix = VIDEO_BPP32;uc_priv->xsize = mode->xres;uc_priv->ysize = mode->yres;video_set_flush_dcache(dev, 1);true
return 0;}
+static int sunxi_de_bind(struct udevice *dev) +{
struct video_uc_plat *plat = dev_get_uclass_plat(dev);plat->size = LCD_MAX_WIDTH * LCD_MAX_HEIGHT *(1 << LCD_MAX_LOG2_BPP) / 8;Should use enum video_log2_bpp here. Also see VNBYTES().
return 0;+}
+static const struct video_ops sunxi_de_ops = { +};
+U_BOOT_DRIVER(sunxi_de) = {
.name = "sunxi_de",.id = UCLASS_VIDEO,.ops = &sunxi_de_ops,.bind = sunxi_de_bind,.probe = sunxi_de_probe,.priv_auto = sizeof(struct sunxi_display),Should ideally have an _priv suffix
.flags = DM_FLAG_PRE_RELOC,+};
+U_BOOT_DRVINFO(sunxi_de) = {
.name = "sunxi_de"+};
[..]
Regards, Simon