Re: [PATCH v6 01/18] video: tegra20: dc: diverge DC per-SOC

["Thierry Reding" <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9883 bytes --]

On Tue Jan 23, 2024 at 6:16 PM CET, Svyatoslav Ryhel wrote:
[...]
> diff --git a/arch/arm/include/asm/arch-tegra114/display.h b/arch/arm/include/asm/arch-tegra114/display.h
> new file mode 100644
> index 0000000000..9411525799
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-tegra114/display.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + *  (C) Copyright 2010
> + *  NVIDIA Corporation <www.nvidia.com>
> + */
> +
> +#ifndef __ASM_ARCH_TEGRA_DISPLAY_H
> +#define __ASM_ARCH_TEGRA_DISPLAY_H
> +
> +#include <asm/arch-tegra/dc.h>
> +
> +/* This holds information about a window which can be displayed */
> +struct disp_ctl_win {
> +	enum win_color_depth_id fmt;	/* Color depth/format */
> +	unsigned int	bpp;		/* Bits per pixel */
> +	phys_addr_t	phys_addr;	/* Physical address in memory */
> +	unsigned int	x;		/* Horizontal address offset (bytes) */
> +	unsigned int	y;		/* Veritical address offset (bytes) */
> +	unsigned int	w;		/* Width of source window */
> +	unsigned int	h;		/* Height of source window */
> +	unsigned int	stride;		/* Number of bytes per line */
> +	unsigned int	out_x;		/* Left edge of output window (col) */
> +	unsigned int	out_y;		/* Top edge of output window (row) */
> +	unsigned int	out_w;		/* Width of output window in pixels */
> +	unsigned int	out_h;		/* Height of output window in pixels */
> +};
> +
> +#endif /*__ASM_ARCH_TEGRA_DISPLAY_H*/

One of the earlier patches in the series gets rid of this per-SoC header
file in favor of a common one. Did this end up here by mistake? It
doesn't seem to be used.

> diff --git a/drivers/video/tegra20/tegra-dc.c b/drivers/video/tegra20/tegra-dc.c
> index f53ad46397..7605e77bc1 100644
> --- a/drivers/video/tegra20/tegra-dc.c
> +++ b/drivers/video/tegra20/tegra-dc.c
> @@ -3,7 +3,6 @@
>   * Copyright (c) 2011 The Chromium OS Authors.
>   */
>  
> -#include <common.h>
>  #include <backlight.h>
>  #include <dm.h>
>  #include <fdtdec.h>
> @@ -23,10 +22,15 @@
>  #include <asm/arch/pinmux.h>
>  #include <asm/arch/pwm.h>
>  #include <asm/arch/display.h>
> -#include <asm/arch-tegra/timer.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +/* Holder of Tegra per-SOC DC differences */
> +struct tegra_dc_soc_info {
> +	bool has_timer;
> +	bool has_rgb;
> +};
> +
>  /* Information about the display controller */
>  struct tegra_lcd_priv {
>  	int width;			/* width in pixels */
> @@ -35,6 +39,7 @@ struct tegra_lcd_priv {
>  	struct display_timing timing;
>  	struct udevice *panel;
>  	struct dc_ctlr *dc;		/* Display controller regmap */
> +	const struct tegra_dc_soc_info *soc;
>  	fdt_addr_t frame_buffer;	/* Address of frame buffer */
>  	unsigned pixel_clock;		/* Pixel clock in Hz */
>  	int dc_clk[2];			/* Contains clk and its parent */
> @@ -43,8 +48,8 @@ struct tegra_lcd_priv {
>  
>  enum {
>  	/* Maximum LCD size we support */
> -	LCD_MAX_WIDTH		= 1920,
> -	LCD_MAX_HEIGHT		= 1200,
> +	LCD_MAX_WIDTH		= 2560,
> +	LCD_MAX_HEIGHT		= 1600,
>  	LCD_MAX_LOG2_BPP	= VIDEO_BPP16,
>  };
>  
> @@ -110,9 +115,9 @@ static void update_window(struct tegra_lcd_priv *priv,
>  	writel(val, &dc->cmd.state_ctrl);
>  }
>  
> -static int update_display_mode(struct dc_disp_reg *disp,
> -			       struct tegra_lcd_priv *priv)
> +static int update_display_mode(struct tegra_lcd_priv *priv)
>  {
> +	struct dc_disp_reg *disp = &priv->dc->disp;
>  	struct display_timing *dt = &priv->timing;
>  	unsigned long val;
>  	unsigned long rate;
> @@ -128,14 +133,16 @@ static int update_display_mode(struct dc_disp_reg *disp,
>  	       &disp->front_porch);
>  	writel(dt->hactive.typ | (dt->vactive.typ << 16), &disp->disp_active);
>  
> -	val = DE_SELECT_ACTIVE << DE_SELECT_SHIFT;
> -	val |= DE_CONTROL_NORMAL << DE_CONTROL_SHIFT;
> -	writel(val, &disp->data_enable_opt);
> +	if (priv->soc->has_rgb) {
> +		val = DE_SELECT_ACTIVE << DE_SELECT_SHIFT;
> +		val |= DE_CONTROL_NORMAL << DE_CONTROL_SHIFT;
> +		writel(val, &disp->data_enable_opt);
>  
> -	val = DATA_FORMAT_DF1P1C << DATA_FORMAT_SHIFT;
> -	val |= DATA_ALIGNMENT_MSB << DATA_ALIGNMENT_SHIFT;
> -	val |= DATA_ORDER_RED_BLUE << DATA_ORDER_SHIFT;
> -	writel(val, &disp->disp_interface_ctrl);
> +		val = DATA_FORMAT_DF1P1C << DATA_FORMAT_SHIFT;
> +		val |= DATA_ALIGNMENT_MSB << DATA_ALIGNMENT_SHIFT;
> +		val |= DATA_ORDER_RED_BLUE << DATA_ORDER_SHIFT;
> +		writel(val, &disp->disp_interface_ctrl);
> +	}
>  
>  	/*
>  	 * The pixel clock divider is in 7.1 format (where the bottom bit
> @@ -147,7 +154,8 @@ static int update_display_mode(struct dc_disp_reg *disp,
>  	div = ((rate * 2 + priv->pixel_clock / 2) / priv->pixel_clock) - 2;
>  	debug("Display clock %lu, divider %lu\n", rate, div);
>  
> -	writel(0x00010001, &disp->shift_clk_opt);
> +	if (priv->soc->has_rgb)
> +		writel(0x00010001, &disp->shift_clk_opt);
>  
>  	val = PIXEL_CLK_DIVIDER_PCD1 << PIXEL_CLK_DIVIDER_SHIFT;
>  	val |= div << SHIFT_CLK_DIVIDER_SHIFT;
> @@ -174,6 +182,7 @@ static void basic_init(struct dc_cmd_reg *cmd)
>  	writel(val, &cmd->disp_pow_ctrl);
>  
>  	val = readl(&cmd->disp_cmd);
> +	val &= ~CTRL_MODE_MASK;
>  	val |= CTRL_MODE_C_DISPLAY << CTRL_MODE_SHIFT;

This seems unrelated to the rest here, but probably not worth splitting
it into a separate patch. I vaguely recall that this wasn't really
necessary because we do the reset prior to initialization and the
register is all zeroes by default?

>  	writel(val, &cmd->disp_cmd);
>  }
> @@ -229,8 +238,8 @@ static void rgb_enable(struct dc_com_reg *com)
>  		writel(rgb_sel_tab[i], &com->pin_output_sel[i]);
>  }
>  
> -static int setup_window(struct disp_ctl_win *win,
> -			struct tegra_lcd_priv *priv)
> +static int setup_window(struct tegra_lcd_priv *priv,
> +			struct disp_ctl_win *win)
>  {
>  	if (priv->rotation) {
>  		win->x = priv->width * 2;
> @@ -274,12 +283,11 @@ static int setup_window(struct disp_ctl_win *win,
>   * You should pass in the U-Boot address here, and check the contents of
>   * struct tegra_lcd_priv to see what was actually chosen.
>   *
> - * @param blob			Device tree blob
>   * @param priv			Driver's private data
>   * @param default_lcd_base	Default address of LCD frame buffer
>   * Return: 0 if ok, -1 on error (unsupported bits per pixel)
>   */
> -static int tegra_display_probe(const void *blob, struct tegra_lcd_priv *priv,
> +static int tegra_display_probe(struct tegra_lcd_priv *priv,
>  			       void *default_lcd_base)
>  {
>  	struct disp_ctl_win window;
> @@ -288,7 +296,7 @@ static int tegra_display_probe(const void *blob, struct tegra_lcd_priv *priv,
>  	priv->frame_buffer = (u32)default_lcd_base;
>  
>  	/*
> -	 * We halve the rate if DISP1 paret is PLLD, since actual parent
> +	 * We halve the rate if DISP1 parent is PLLD, since actual parent
>  	 * is plld_out0 which is PLLD divided by 2.
>  	 */
>  	if (priv->dc_clk[1] == CLOCK_ID_DISPLAY)
> @@ -303,13 +311,17 @@ static int tegra_display_probe(const void *blob, struct tegra_lcd_priv *priv,
>  			       rate);
>  
>  	basic_init(&priv->dc->cmd);
> -	basic_init_timer(&priv->dc->disp);
> -	rgb_enable(&priv->dc->com);
> +
> +	if (priv->soc->has_timer)
> +		basic_init_timer(&priv->dc->disp);
> +
> +	if (priv->soc->has_rgb)
> +		rgb_enable(&priv->dc->com);
>  
>  	if (priv->pixel_clock)
> -		update_display_mode(&priv->dc->disp, priv);
> +		update_display_mode(priv);
>  
> -	if (setup_window(&window, priv))
> +	if (setup_window(priv, &window))
>  		return -1;
>  
>  	update_window(priv, &window);
> @@ -322,7 +334,6 @@ static int tegra_lcd_probe(struct udevice *dev)
>  	struct video_uc_plat *plat = dev_get_uclass_plat(dev);
>  	struct video_priv *uc_priv = dev_get_uclass_priv(dev);
>  	struct tegra_lcd_priv *priv = dev_get_priv(dev);
> -	const void *blob = gd->fdt_blob;
>  	int ret;
>  
>  	/* Initialize the Tegra display controller */
> @@ -330,8 +341,8 @@ static int tegra_lcd_probe(struct udevice *dev)
>  	funcmux_select(PERIPH_ID_DISP1, FUNCMUX_DEFAULT);
>  #endif
>  
> -	if (tegra_display_probe(blob, priv, (void *)plat->base)) {
> -		printf("%s: Failed to probe display driver\n", __func__);
> +	if (tegra_display_probe(priv, (void *)plat->base)) {
> +		debug("%s: Failed to probe display driver\n", __func__);

Shouldn't this remain a printf() to make it more visible in the logs
what's going on? I guess people will notice anyway when the display
doesn't turn on, but this is good information to know when
troubleshooting.

>  		return -1;
>  	}
>  
> @@ -383,6 +394,8 @@ static int tegra_lcd_of_to_plat(struct udevice *dev)
>  		return -EINVAL;
>  	}
>  
> +	priv->soc = (struct tegra_dc_soc_info *)dev_get_driver_data(dev);
> +
>  	ret = clock_decode_pair(dev, priv->dc_clk);
>  	if (ret < 0) {
>  		debug("%s: Cannot decode clocks for '%s' (ret = %d)\n",
> @@ -464,19 +477,43 @@ static int tegra_lcd_bind(struct udevice *dev)
>  static const struct video_ops tegra_lcd_ops = {
>  };
>  
> +static const struct tegra_dc_soc_info tegra20_dc_soc_info = {
> +	.has_timer = true,
> +	.has_rgb = true,
> +};
> +
> +static const struct tegra_dc_soc_info tegra30_dc_soc_info = {
> +	.has_timer = false,
> +	.has_rgb = true,
> +};
> +
> +static const struct tegra_dc_soc_info tegra114_dc_soc_info = {
> +	.has_timer = false,
> +	.has_rgb = false,
> +};

My recollection is that technically Tegra114 still supports RGB, but it
ends up never being used on any of the platforms that I know of. On
Linux we base the decision to initialize the corresponding registers on
the status property of the "rgb" node of each display controller.
Perhaps that's something that U-Boot could do as well? That would avoid
programming these registers on Tegra20 and Tegra30 devices that don't
use RGB.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]