On Sun, 7 Feb 2021 07:37:34 -0700 Simon Glass <s...@chromium.org> wrote:
Hi Simon, > 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 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_VIDEO > > I 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, 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()? 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