Message-ID: <ec33c0fd-94b4-0ee0-0b96-5d50fee33c59@ideasonboard.com>
Date:   Tue, 20 Dec 2022 14:52:39 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Aradhya Bhatia <a-bhatia1@...com>, Jyri Sarha <jyri.sarha@....fi>,
        Rob Herring <robh+dt@...nel.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc:     DRI Development List <dri-devel@...ts.freedesktop.org>,
        Devicetree List <devicetree@...r.kernel.org>,
        Linux Kernel List <linux-kernel@...r.kernel.org>,
        Nishanth Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Rahul T R <r-ravikumar@...com>,
        Devarsh Thakkar <devarsht@...com>,
        Jayesh Choudhary <j-choudhary@...com>
Subject: Re: [PATCH v6 2/5] drm/tidss: Add support for AM625 DSS

Hi,

On 19/11/2022 19:30, Aradhya Bhatia wrote:
> Add support for the DSS controller on TI's new AM625 SoC in the tidss
> driver.
> 
> The first video port (VP0) in am625-dss can output OLDI signals through
> 2 OLDI TXes. A 3rd output port has been added with "DISPC_VP_OLDI" bus
> type.
> 
> DSS controllers before AM625 had a 1 to 1 coupling between Videoports
> and Output Ports. Since this stands no longer to be true for AM625 DSS,
> this couppling has been separated with the introduction of output port
> based variables. This will help address the addition of the 2nd OLDI TX
> over VP0 as the 3rd output port.

This patch does three things:
- Renames "port" to "vp" where applicable
- Adds output ports
- Adds AM625

I think at least the AM625 parts should be a separate patch, but 
preferably all three would be separate patches.

> Signed-off-by: Aradhya Bhatia <a-bhatia1@...com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 80 ++++++++++++++++++++++++++---
>   drivers/gpu/drm/tidss/tidss_dispc.h | 15 ++++--
>   drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
>   drivers/gpu/drm/tidss/tidss_drv.h   |  5 +-
>   drivers/gpu/drm/tidss/tidss_irq.h   |  2 +-
>   drivers/gpu/drm/tidss/tidss_kms.c   |  2 +-
>   6 files changed, 90 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index ad93acc9abd2..dbc6a5b617ca 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -93,10 +93,13 @@ const struct dispc_features dispc_k2g_feats = {
>   	.common_regs = tidss_k2g_common_regs,
>   
>   	.num_vps = 1,
> +	.num_output_ports = 1,
> +	.oldi_supported = false,

I'd prefer "has_oldi", the style is used in other places too.

>   	.vp_name = { "vp1" },
>   	.ovr_name = { "ovr1" },
>   	.vpclk_name =  { "vp1" },
>   	.vp_bus_type = { DISPC_VP_DPI },
> +	.output_port_bus_type = { DISPC_VP_DPI },

It would make sense to re-arrange these a bit, so that lines related to 
VPs are together, and lines related to output ports would be together.

>   	.vp_feat = { .color = {
>   			.has_ctm = true,
> @@ -168,10 +171,13 @@ const struct dispc_features dispc_am65x_feats = {
>   	.common_regs = tidss_am65x_common_regs,
>   
>   	.num_vps = 2,
> +	.num_output_ports = 2,
> +	.oldi_supported = true,
>   	.vp_name = { "vp1", "vp2" },
>   	.ovr_name = { "ovr1", "ovr2" },
>   	.vpclk_name =  { "vp1", "vp2" },
>   	.vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
> +	.output_port_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
>   
>   	.vp_feat = { .color = {
>   			.has_ctm = true,
> @@ -257,12 +263,16 @@ const struct dispc_features dispc_j721e_feats = {
>   	.common_regs = tidss_j721e_common_regs,
>   
>   	.num_vps = 4,
> +	.num_output_ports = 4,
> +	.oldi_supported = false,
>   	.vp_name = { "vp1", "vp2", "vp3", "vp4" },
>   	.ovr_name = { "ovr1", "ovr2", "ovr3", "ovr4" },
>   	.vpclk_name = { "vp1", "vp2", "vp3", "vp4" },
>   	/* Currently hard coded VP routing (see dispc_initial_config()) */
>   	.vp_bus_type =	{ DISPC_VP_INTERNAL, DISPC_VP_DPI,
>   			  DISPC_VP_INTERNAL, DISPC_VP_DPI, },
> +	.output_port_bus_type =	{ DISPC_VP_INTERNAL, DISPC_VP_DPI,
> +			  DISPC_VP_INTERNAL, DISPC_VP_DPI, },
>   	.vp_feat = { .color = {
>   			.has_ctm = true,
>   			.gamma_size = 1024,
> @@ -275,6 +285,59 @@ const struct dispc_features dispc_j721e_feats = {
>   	.vid_order = { 1, 3, 0, 2 },
>   };
>   
> +const struct dispc_features dispc_am625_feats = {
> +	.max_pclk_khz = {
> +		[DISPC_VP_DPI] = 165000,
> +		[DISPC_VP_OLDI] = 165000,
> +	},
> +
> +	.scaling = {
> +		.in_width_max_5tap_rgb = 1280,
> +		.in_width_max_3tap_rgb = 2560,
> +		.in_width_max_5tap_yuv = 2560,
> +		.in_width_max_3tap_yuv = 4096,
> +		.upscale_limit = 16,
> +		.downscale_limit_5tap = 4,
> +		.downscale_limit_3tap = 2,
> +		/*
> +		 * The max supported pixel inc value is 255. The value
> +		 * of pixel inc is calculated like this: 1+(xinc-1)*bpp.
> +		 * The maximum bpp of all formats supported by the HW
> +		 * is 8. So the maximum supported xinc value is 32,
> +		 * because 1+(32-1)*8 < 255 < 1+(33-1)*4.
> +		 */
> +		.xinc_max = 32,
> +	},
> +
> +	.subrev = DISPC_AM625,
> +
> +	.common = "common",
> +	.common_regs = tidss_am65x_common_regs,
> +
> +	.num_vps = 2,
> +	/* note: the 3rd port is not representative of a 3rd pipeline */
> +	.num_output_ports = 3,
> +	.oldi_supported = true,
> +	.vp_name = { "vp1", "vp2" },
> +	.ovr_name = { "ovr1", "ovr2" },
> +	.vpclk_name =  { "vp1", "vp2" },
> +	.vp_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI },
> +	.output_port_bus_type = { DISPC_VP_OLDI, DISPC_VP_DPI, DISPC_VP_OLDI },

This is a bit confusing: DISPC_VP_* defines are used for both 
vp_bus_type (which makes sense), but it's also used for 
output_port_bus_type.

The code uses both vp_bus_type and output_port_bus_type, but my initial 
reaction was that we should only have one of these. Perhaps the 
output_port_bus_type, as the VPs should be identical. The differences 
are after the VP.

However, I'm not sure if that's an easy change. But maybe instead of 
output_port_bus_type we could have output_port_source, which lists, for 
each output port, the VP it uses as a source. Here it would be { 0, 1, 0 
}. With that, the code can get the output's VP bus_type.

> +	.vp_feat = { .color = {
> +			.has_ctm = true,
> +			.gamma_size = 256,
> +			.gamma_type = TIDSS_GAMMA_8BIT,
> +		},
> +	},
> +
> +	.num_planes = 2,
> +	/* note: vid is plane_id 0 and vidl1 is plane_id 1 */
> +	.vid_name = { "vid", "vidl1" },
> +	.vid_lite = { false, true, },
> +	.vid_order = { 1, 0 },
> +};
> +
>   static const u16 *dispc_common_regmap;
>   
>   struct dss_vp_data {
> @@ -287,12 +350,12 @@ struct dispc_device {
>   
>   	void __iomem *base_common;
>   	void __iomem *base_vid[TIDSS_MAX_PLANES];
> -	void __iomem *base_ovr[TIDSS_MAX_PORTS];
> -	void __iomem *base_vp[TIDSS_MAX_PORTS];
> +	void __iomem *base_ovr[TIDSS_MAX_VPS];
> +	void __iomem *base_vp[TIDSS_MAX_VPS];
>   
>   	struct regmap *oldi_io_ctrl;
>   
> -	struct clk *vp_clk[TIDSS_MAX_PORTS];
> +	struct clk *vp_clk[TIDSS_MAX_VPS];
>   
>   	const struct dispc_features *feat;
>   
> @@ -300,7 +363,7 @@ struct dispc_device {
>   
>   	bool is_enabled;
>   
> -	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
> +	struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>   
>   	u32 *fourccs;
>   	u32 num_fourccs;
> @@ -778,6 +841,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
>   		return dispc_k2g_read_and_clear_irqstatus(dispc);
>   	case DISPC_AM65X:
>   	case DISPC_J721E:
> +	case DISPC_AM625:

In switch cases like these, I think it makes sense to group the cases. 
Here the AM6 cases should be together, and I'd put the AM625 on top so 
that the cases are sorted.

>   		return dispc_k3_read_and_clear_irqstatus(dispc);
>   	default:
>   		WARN_ON(1);
> @@ -793,6 +857,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
>   		break;
>   	case DISPC_AM65X:
>   	case DISPC_J721E:
> +	case DISPC_AM625:
>   		dispc_k3_set_irqenable(dispc, mask);
>   		break;
>   	default:
> @@ -1116,7 +1181,7 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>   					 const struct drm_display_mode *mode)
>   {
>   	u32 hsw, hfp, hbp, vsw, vfp, vbp;
> -	enum dispc_vp_bus_type bus_type;
> +	enum dispc_bus_type bus_type;
>   	int max_pclk;
>   
>   	bus_type = dispc->feat->vp_bus_type[hw_videoport];
> @@ -1282,6 +1347,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
>   					x, y, layer);
>   		break;
>   	case DISPC_AM65X:
> +	case DISPC_AM625:
>   		dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
>   					  x, y, layer);
>   		break;
> @@ -2205,6 +2271,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
>   		break;
>   	case DISPC_AM65X:
>   	case DISPC_J721E:
> +	case DISPC_AM625:
>   		dispc_k3_plane_init(dispc);
>   		break;
>   	default:
> @@ -2310,6 +2377,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
>   		dispc_k2g_vp_write_gamma_table(dispc, hw_videoport);
>   		break;
>   	case DISPC_AM65X:
> +	case DISPC_AM625:
>   		dispc_am65x_vp_write_gamma_table(dispc, hw_videoport);
>   		break;
>   	case DISPC_J721E:
> @@ -2583,7 +2651,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
>   		REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
>   		REG_GET(dispc, DSS_SYSSTATUS, 3, 3));
>   
> -	if (dispc->feat->subrev == DISPC_AM65X)
> +	if (dispc->feat->oldi_supported)
>   		dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
>   			REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
>   			REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index e49432f0abf5..51db500590ee 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -50,7 +50,7 @@ struct dispc_errata {
>   	bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
>   };
>   
> -enum dispc_vp_bus_type {
> +enum dispc_bus_type {
>   	DISPC_VP_DPI,		/* DPI output */
>   	DISPC_VP_OLDI,		/* OLDI (LVDS) output */
>   	DISPC_VP_INTERNAL,	/* SoC internal routing */

If you rename the enum to dispc_bus_type, the enum value name doesn't 
match anymore as it still contains VP. If you go with the suggestion of 
not having output_port_bus_type at all, then you can keep this as 
dispc_vp_bus_type.

But if we need types for both VPs and outputs, I think it would be 
better to have separate enums for them.

> @@ -61,6 +61,7 @@ enum dispc_dss_subrevision {
>   	DISPC_K2G,
>   	DISPC_AM65X,
>   	DISPC_J721E,
> +	DISPC_AM625,
>   };
>   
>   struct dispc_features {
> @@ -74,10 +75,13 @@ struct dispc_features {
>   	const char *common;
>   	const u16 *common_regs;
>   	u32 num_vps;
> -	const char *vp_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
> -	const char *ovr_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
> -	const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */
> -	const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
> +	u32 num_output_ports;
> +	bool oldi_supported;
> +	const char *vp_name[TIDSS_MAX_VPS]; /* Should match dt reg names */
> +	const char *ovr_name[TIDSS_MAX_VPS]; /* Should match dt reg names */
> +	const char *vpclk_name[TIDSS_MAX_VPS]; /* Should match dt clk names */
> +	const enum dispc_bus_type vp_bus_type[TIDSS_MAX_VPS];
> +	const enum dispc_bus_type output_port_bus_type[TIDSS_MAX_OUTPUT_PORTS];
>   	struct tidss_vp_feat vp_feat;
>   	u32 num_planes;
>   	const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */
> @@ -88,6 +92,7 @@ struct dispc_features {
>   extern const struct dispc_features dispc_k2g_feats;
>   extern const struct dispc_features dispc_am65x_feats;
>   extern const struct dispc_features dispc_j721e_feats;
> +extern const struct dispc_features dispc_am625_feats;
>   
>   void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask);
>   dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc);
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 15cd9b91b7e2..c5e119828823 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -235,6 +235,7 @@ static const struct of_device_id tidss_of_table[] = {
>   	{ .compatible = "ti,k2g-dss", .data = &dispc_k2g_feats, },
>   	{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
>   	{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
> +	{ .compatible = "ti,am625-dss", .data = &dispc_am625_feats, },
>   	{ }
>   };
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index d7f27b0b0315..0ce7ee5ccd5b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -9,8 +9,9 @@
>   
>   #include <linux/spinlock.h>
>   
> -#define TIDSS_MAX_PORTS 4
> +#define TIDSS_MAX_VPS 4
>   #define TIDSS_MAX_PLANES 4
> +#define TIDSS_MAX_OUTPUT_PORTS 4
>   
>   typedef u32 dispc_irq_t;
>   
> @@ -22,7 +23,7 @@ struct tidss_device {
>   	struct dispc_device *dispc;
>   
>   	unsigned int num_crtcs;
> -	struct drm_crtc *crtcs[TIDSS_MAX_PORTS];
> +	struct drm_crtc *crtcs[TIDSS_MAX_VPS];
>   
>   	unsigned int num_planes;
>   	struct drm_plane *planes[TIDSS_MAX_PLANES];
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.h b/drivers/gpu/drm/tidss/tidss_irq.h
> index b512614d5863..a753f5e3ce15 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.h
> +++ b/drivers/gpu/drm/tidss/tidss_irq.h
> @@ -35,7 +35,7 @@
>   
>   #define DSS_IRQ_VP_BIT_N(ch, bit)	(4 + 4 * (ch) + (bit))
>   #define DSS_IRQ_PLANE_BIT_N(plane, bit) \
> -	(DSS_IRQ_VP_BIT_N(TIDSS_MAX_PORTS, 0) + 1 * (plane) + (bit))
> +	(DSS_IRQ_VP_BIT_N(TIDSS_MAX_VPS, 0) + 1 * (plane) + (bit))
>   
>   #define DSS_IRQ_VP_BIT(ch, bit)	BIT(DSS_IRQ_VP_BIT_N((ch), (bit)))
>   #define DSS_IRQ_PLANE_BIT(plane, bit) \
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index afb2879980c6..fc9c4eefd31d 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -123,7 +123,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>   	u32 max_vps = feat->num_vps;
>   	u32 max_planes = feat->num_planes;
>   
> -	struct pipe pipes[TIDSS_MAX_PORTS];
> +	struct pipe pipes[TIDSS_MAX_VPS];
>   	u32 num_pipes = 0;
>   	u32 crtc_mask;
>   

  Tomi

Powered by blists - more mailing lists