Hi Andre, On Thu, 4 Feb 2021 at 18:08, Andre Przywara <andre.przyw...@arm.com> wrote: > > From: Jagan Teki <ja...@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 <ja...@amarulasolutions.com> > [Andre: rebase and smaller fixes] > Signed-off-by: Andre Przywara <andre.przyw...@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 <s...@chromium.org> Some thoughts below > 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_VIDEO I wonder whether instead VIDEO_SUNXI should depend on DM_VIDEO ? > + select DISPLAY > imply VIDEO_DT_SIMPLEFB > default y > ---help--- > - Say Y here to add support for using a cfb console on the HDMI, LCD > - or VGA output found on most sunxi devices. See doc/README.video for > - info 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 provide > + a 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()? > - 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