Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Aradhya Bhatia <aradhya.bhatia@linux.dev>]

Hi Devarsh,

Thanks for the patches.

On 31/12/24 14:34, Devarsh Thakkar wrote:
> Enable display for AM62L DSS [1] which supports only a single display
> pipeline using a single overlay manager, single video port and a single
> video lite pipeline which does not support scaling.
> 
> The output of video port is routed to SoC boundary via DPI interface and
> the DPI signals from the video port are also routed to DSI Tx controller
> present within the SoC.
> 
> [1]: Section 11.7 (Display Subsystem and Peripherals)
> Link : https://www.ti.com/lit/pdf/sprujb4
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 34 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
>  drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index cacb5f3d8085..cd322d60b825 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -376,6 +376,35 @@ const struct dispc_features dispc_am62a7_feats = {
>  	.vid_order = { 1, 0 },
>  };
>  
> +const struct dispc_features dispc_am62l_feats = {
> +	.max_pclk_khz = {
> +		[DISPC_VP_DPI] = 165000,

The TRM mentions that the max the VP PLL can go is 150MHz, so maybe you
might need to update this.

That said, as far as I understand, the IP itself can support 165 MHz,
and I am wondering, what would we do if there comes out a new SoC that
uses AM62L DSS as is, but just bumps up the PLL capacity to 165MHz.

It would be odd to have a ditto feats structure with just the frequency
updated.

> +	},
> +
> +	.subrev = DISPC_AM62L,
> +
> +	.common = "common",
> +	.common_regs = tidss_am65x_common_regs,

Also, I don't think we should utilize this as is.

The AM62L common regions is different, and is, at best, a subset of the
AM65x/AM62x common register space.

For example, registers VID_IRQ{STATUS, ENABLE}_0 have been dropped,
along with VP_IRQ{STATUS, ENABLE}_1.

- Which brings to my next concern...

> +
> +	.num_vps = 1,
> +	.vp_name = { "vp1" },
> +	.ovr_name = { "ovr1" },
> +	.vpclk_name =  { "vp1" },
> +	.vp_bus_type = { DISPC_VP_DPI },
> +
> +	.vp_feat = { .color = {
> +			.has_ctm = true,
> +			.gamma_size = 256,
> +			.gamma_type = TIDSS_GAMMA_8BIT,
> +		},
> +	},
> +
> +	.num_planes = 1,
> +	.vid_name = { "vidl1" },
> +	.vid_lite = { true },
> +	.vid_order = { 0 },

...

Usually, VID1 is the first video pipeline, while VIDL1 remains the
second. Which is why vid1 occupies the index 0, and vidl1 occupies 1 -
even from the hardware perspective.

While AM62L has only one video (lite) pipeline - vidl1, and there is no
vid1, the hardware still treats vidl1 at index 1.

For example, the TRM has defined DISPC_VID_IRQ{STATUS, ENABLE}_1 (and
not _0) in the common region.

So, the vid_order here should be 1, not 0.


Regards
Aradhya