Message-ID: <09e0616d-e135-41d7-bf5d-3f5c8535a0d5@ideasonboard.com>
Date: Tue, 11 Feb 2025 12:57:36 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>, Maxime Ripard <mripard@...nel.org>,
 David Airlie <airlied@...il.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Simona Vetter <simona@...ll.ch>, Nishanth Menon <nm@...com>,
 Vignesh Raghavendra <vigneshr@...com>, Devarsh Thakkar <devarsht@...com>,
 Praneeth Bajjuri <praneeth@...com>, Udit Kumar <u-kumar1@...com>,
 Jayesh Choudhary <j-choudhary@...com>,
 Francesco Dolcini <francesco@...cini.it>,
 DRI Development List <dri-devel@...ts.freedesktop.org>,
 Devicetree List <devicetree@...r.kernel.org>,
 Linux Kernel List <linux-kernel@...r.kernel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jyri Sarha <jyri.sarha@....fi>
Subject: Re: [PATCH v5 3/3] drm/tidss: Add OLDI bridge support

Hi,

On 09/02/2025 18:09, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@...com>
> 
> The AM62x and AM62Px SoCs feature 2 OLDI TXes each, which makes it
> possible to connect them in dual-link or cloned single-link OLDI display
> modes. The current OLDI support in tidss_dispc.c can only support for
> a single OLDI TX, connected to a VP and doesn't really support
> configuration of OLDIs in the other modes. The current OLDI support in
> tidss_dispc.c also works on the principle that the OLDI output can only
> be served by one, and only one, DSS video-port. This isn't the case in
> the AM62Px SoC, where there are 2 DSS controllers present that share the
> OLDI TXes.
> 
> Having their own devicetree and their own bridge entity will help
> support the various display modes and sharing possiblilities of the OLDI
> hardware.
> 
> For all these reasons, add support for the OLDI TXes as DRM bridges.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@...com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
> ---
>   drivers/gpu/drm/tidss/Makefile           |   3 +-
>   drivers/gpu/drm/tidss/tidss_dispc.c      |  20 +-
>   drivers/gpu/drm/tidss/tidss_dispc.h      |   4 +
>   drivers/gpu/drm/tidss/tidss_dispc_regs.h |  14 +
>   drivers/gpu/drm/tidss/tidss_drv.c        |   9 +
>   drivers/gpu/drm/tidss/tidss_drv.h        |   5 +
>   drivers/gpu/drm/tidss/tidss_oldi.c       | 558 +++++++++++++++++++++++
>   drivers/gpu/drm/tidss/tidss_oldi.h       |  51 +++
>   8 files changed, 662 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c
>   create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h
> 
> diff --git a/drivers/gpu/drm/tidss/Makefile b/drivers/gpu/drm/tidss/Makefile
> index 312645271014..b6d6becf1683 100644
> --- a/drivers/gpu/drm/tidss/Makefile
> +++ b/drivers/gpu/drm/tidss/Makefile
> @@ -7,6 +7,7 @@ tidss-y := tidss_crtc.o \
>   	tidss_irq.o \
>   	tidss_plane.o \
>   	tidss_scale_coefs.o \
> -	tidss_dispc.o
> +	tidss_dispc.o \
> +	tidss_oldi.o
>   
>   obj-$(CONFIG_DRM_TIDSS) += tidss.o
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 1ad711f8d2a8..8631a89e6155 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -466,6 +466,25 @@ static u32 dispc_vp_read(struct dispc_device *dispc, u32 hw_videoport, u16 reg)
>   	return ioread32(base + reg);
>   }
>   
> +void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport,
> +			  u32 oldi_cfg)
> +{
> +	u32 count = 0;
> +	u32 oldi_reset_bit = BIT(5 + hw_videoport);
> +
> +	dispc_vp_write(tidss->dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +
> +	if (oldi_cfg != 0) {
> +		while (!(oldi_reset_bit & dispc_read(tidss->dispc, DSS_SYSSTATUS)) &&
> +		       count < 10000)
> +			count++;
> +
> +		if (!(oldi_reset_bit & dispc_read(tidss->dispc, DSS_SYSSTATUS)))
> +			dev_warn(tidss->dispc->dev, "%s: timeout waiting OLDI reset done\n",
> +				 __func__);
> +	}
> +}

The timeout sounds like an error. Better to return an error value, and 
handle it in tidss_oldi_config()?

You could also

if (!oldi_cfg)
	return 0;

But would it actually be nicer to have a separate void function for 
disabling?

> +
>   /*
>    * TRM gives bitfields as start:end, where start is the higher bit
>    * number. For example 7:0
> @@ -1310,7 +1329,6 @@ void dispc_vp_disable_clk(struct dispc_device *dispc, u32 hw_videoport)
>    * Calculate the percentage difference between the requested pixel clock rate
>    * and the effective rate resulting from calculating the clock divider value.
>    */
> -static
>   unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate)
>   {
>   	int r = rate / 100, rr = real_rate / 100;
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index 086327d51a90..fab248f2055a 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -94,6 +94,10 @@ extern const struct dispc_features dispc_am62a7_feats;
>   extern const struct dispc_features dispc_am65x_feats;
>   extern const struct dispc_features dispc_j721e_feats;
>   
> +void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport,
> +			  u32 oldi_cfg);
> +unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate);
> +
>   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_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> index 13feedfe5d6d..03f7098029e6 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
> @@ -226,6 +226,20 @@ enum dispc_common_regs {
>   #define DISPC_VP_DSS_DMA_THREADSIZE		0x170 /* J721E */
>   #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS	0x174 /* J721E */
>   
> +/* OLDI Config Bits (DISPC_VP_DSS_OLDI_CFG) */
> +#define OLDI_ENABLE		BIT(0)
> +#define OLDI_MAP		(BIT(1) | BIT(2) | BIT(3))
> +#define OLDI_SRC		BIT(4)
> +#define OLDI_CLONE_MODE		BIT(5)
> +#define OLDI_MASTERSLAVE	BIT(6)
> +#define OLDI_DEPOL		BIT(7)
> +#define OLDI_MSB		BIT(8)
> +#define OLDI_LBEN		BIT(9)
> +#define OLDI_LBDATA		BIT(10)
> +#define OLDI_DUALMODESYNC	BIT(11)
> +#define OLDI_SOFTRST		BIT(12)
> +#define OLDI_TPATCFG		BIT(13)
> +
>   /*
>    * OLDI IO_CTRL register offsets. On AM654 the registers are found
>    * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 7c8fd6407d82..27b9f86f1eb2 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -24,6 +24,7 @@
>   #include "tidss_drv.h"
>   #include "tidss_kms.h"
>   #include "tidss_irq.h"
> +#include "tidss_oldi.h"
>   
>   /* Power management */
>   
> @@ -148,6 +149,10 @@ static int tidss_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> +	ret = tidss_oldi_init(tidss);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to init OLDI\n");
> +
>   	pm_runtime_enable(dev);
>   
>   	pm_runtime_set_autosuspend_delay(dev, 1000);
> @@ -204,6 +209,8 @@ static int tidss_probe(struct platform_device *pdev)
>   	pm_runtime_dont_use_autosuspend(dev);
>   	pm_runtime_disable(dev);
>   
> +	tidss_oldi_deinit(tidss);
> +
>   	return ret;
>   }
>   
> @@ -228,6 +235,8 @@ static void tidss_remove(struct platform_device *pdev)
>   	pm_runtime_dont_use_autosuspend(dev);
>   	pm_runtime_disable(dev);
>   
> +	tidss_oldi_deinit(tidss);
> +
>   	/* devm allocated dispc goes away with the dev so mark it NULL */
>   	dispc_remove(tidss);
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index d7f27b0b0315..6c0fe1d989ee 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -11,8 +11,10 @@
>   
>   #define TIDSS_MAX_PORTS 4
>   #define TIDSS_MAX_PLANES 4
> +#define TIDSS_MAX_OLDI_TXES 2
>   
>   typedef u32 dispc_irq_t;
> +struct tidss_oldi;
>   
>   struct tidss_device {
>   	struct drm_device ddev;		/* DRM device for DSS */
> @@ -27,6 +29,9 @@ struct tidss_device {
>   	unsigned int num_planes;
>   	struct drm_plane *planes[TIDSS_MAX_PLANES];
>   
> +	unsigned int num_oldis;
> +	struct tidss_oldi *oldis[TIDSS_MAX_OLDI_TXES];
> +
>   	unsigned int irq;
>   
>   	spinlock_t wait_lock;	/* protects the irq masks */
> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c
> new file mode 100644
> index 000000000000..4af13a01f546
> --- /dev/null
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
> @@ -0,0 +1,558 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 - Texas Instruments Incorporated
> + *
> + * Aradhya Bhatia <a-bhatia1@...com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_of.h>
> +
> +#include "tidss_dispc.h"
> +#include "tidss_dispc_regs.h"
> +#include "tidss_oldi.h"
> +
> +struct tidss_oldi {
> +	struct tidss_device	*tidss;
> +	struct device		*dev;
> +
> +	struct drm_bridge	bridge;
> +	struct drm_bridge	*next_bridge;
> +
> +	enum tidss_oldi_link_type link_type;
> +	const struct oldi_bus_format *bus_format;
> +	u32 oldi_instance;
> +	u32 companion_instance;
> +	u32 parent_vp;
> +
> +	struct clk *serial;
> +	struct regmap *io_ctrl;
> +};
> +
> +static const struct oldi_bus_format oldi_bus_formats[] = {
> +	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,	18, SPWG_18,	MEDIA_BUS_FMT_RGB666_1X18 },
> +	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,	24, SPWG_24,	MEDIA_BUS_FMT_RGB888_1X24 },
> +	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,	24, JEIDA_24,	MEDIA_BUS_FMT_RGB888_1X24 },
> +};
> +
> +#define OLDI_IDLE_CLK_HZ	25000000 /*25 MHz */
> +
> +static inline struct tidss_oldi *
> +drm_bridge_to_tidss_oldi(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct tidss_oldi, bridge);
> +}
> +
> +static int tidss_oldi_bridge_attach(struct drm_bridge *bridge,
> +				    enum drm_bridge_attach_flags flags)
> +{
> +	struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
> +
> +	if (!oldi->next_bridge) {
> +		dev_err(oldi->dev,
> +			"%s: OLDI%u Failure attach next bridge\n",
> +			__func__, oldi->oldi_instance);
> +		return -ENODEV;
> +	}
> +
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> +		dev_err(oldi->dev,
> +			"%s: OLDI%u DRM_BRIDGE_ATTACH_NO_CONNECTOR is mandatory.\n",
> +			__func__, oldi->oldi_instance);
> +		return -EINVAL;
> +	}
> +
> +	return drm_bridge_attach(bridge->encoder, oldi->next_bridge,
> +				 bridge, flags);
> +}
> +
> +static int
> +tidss_oldi_set_serial_clk(struct tidss_oldi *oldi, unsigned long rate)
> +{
> +	unsigned long new_rate;
> +	int ret;
> +
> +	ret = clk_set_rate(oldi->serial, rate);
> +	if (ret) {
> +		dev_err(oldi->dev,
> +			"OLDI%u: failed to set serial clk rate to %lu Hz\n",
> +			 oldi->oldi_instance, rate);
> +		return ret;
> +	}
> +
> +	new_rate = clk_get_rate(oldi->serial);
> +
> +	if (dispc_pclk_diff(rate, new_rate) > 5)
> +		dev_warn(oldi->dev,
> +			 "OLDI%u Clock rate %lu differs over 5%% from requested %lu\n",
> +			 oldi->oldi_instance, new_rate, rate);
> +
> +	dev_dbg(oldi->dev, "OLDI%u: new rate %lu Hz (requested %lu Hz)\n",
> +		oldi->oldi_instance, clk_get_rate(oldi->serial), rate);
> +
> +	return 0;
> +}
> +
> +static void tidss_oldi_tx_power(struct tidss_oldi *oldi, bool enable)
> +{
> +	u32 mask;
> +
> +	/*
> +	 * The power control bits are Active Low, and remain powered off by
> +	 * default. That is, the bits are set to 1. To power on the OLDI TXes,
> +	 * the bits must be cleared to 0. Since there are cases where not all
> +	 * OLDI TXes are being used, the power logic selectively powers them
> +	 * on.
> +	 * Setting the variable 'val' to particular bit masks, makes sure that
> +	 * the unrequired OLDI TXes remain powered off.
> +	 */
> +
> +	if (enable) {
> +		switch (oldi->link_type) {
> +		case OLDI_MODE_SINGLE_LINK:
> +			/* Power-on only the required OLDI TX's IO*/
> +			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) | OLDI_PWRDN_BG;
> +			break;
> +		case OLDI_MODE_CLONE_SINGLE_LINK:
> +		case OLDI_MODE_DUAL_LINK:
> +			/* Power-on both the OLDI TXes' IOs */
> +			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) |
> +			       OLDI_PWRDOWN_TX(oldi->companion_instance) |
> +			       OLDI_PWRDN_BG;
> +			break;
> +		default:
> +			/*
> +			 * This code execution should never reach here as any
> +			 * OLDI with an unsupported OLDI mode would never get
> +			 * registered in the first place.
> +			 * However, power-off the OLDI in concern just in case.
> +			 */
> +			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance);
> +			enable = false;
> +			break;
> +		}
> +	} else {
> +		switch (oldi->link_type) {
> +		case OLDI_MODE_CLONE_SINGLE_LINK:
> +		case OLDI_MODE_DUAL_LINK:
> +			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) |
> +			       OLDI_PWRDOWN_TX(oldi->companion_instance) |
> +			       OLDI_PWRDN_BG;
> +			break;
> +		case OLDI_MODE_SINGLE_LINK:
> +		default:
> +			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance);
> +			break;
> +		}
> +	}
> +
> +	regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, enable ? 0 : mask);
> +}
> +
> +static int tidss_oldi_config(struct tidss_oldi *oldi)
> +{
> +	const struct oldi_bus_format *bus_fmt = NULL;
> +	u32 oldi_cfg = 0;
> +
> +	bus_fmt = oldi->bus_format;
> +
> +	/*
> +	 * MASTERSLAVE and SRC bits of OLDI Config are always set to 0.
> +	 */
> +
> +	if (bus_fmt->data_width == 24)
> +		oldi_cfg |= OLDI_MSB;
> +	else if (bus_fmt->data_width != 18)
> +		dev_warn(oldi->dev,
> +			 "OLDI%u: DSS port width %d not supported\n",
> +			 oldi->oldi_instance, bus_fmt->data_width);
> +
> +	oldi_cfg |= OLDI_DEPOL;
> +
> +	oldi_cfg = (oldi_cfg & (~OLDI_MAP)) | (bus_fmt->oldi_mode_reg_val << 1);
> +
> +	oldi_cfg |= OLDI_SOFTRST;
> +
> +	oldi_cfg |= OLDI_ENABLE;
> +
> +	switch (oldi->link_type) {
> +	case OLDI_MODE_SINGLE_LINK:
> +		/* All configuration is done for this mode.  */
> +		break;
> +
> +	case OLDI_MODE_CLONE_SINGLE_LINK:
> +		oldi_cfg |= OLDI_CLONE_MODE;
> +		break;
> +
> +	case OLDI_MODE_DUAL_LINK:
> +		/* data-mapping field also indicates dual-link mode */
> +		oldi_cfg |= BIT(3);
> +		oldi_cfg |= OLDI_DUALMODESYNC;
> +		break;
> +
> +	default:
> +		dev_err(oldi->dev, "OLDI%u: Unsupported mode.\n",
> +			oldi->oldi_instance);
> +		return -EINVAL;
> +	}
> +
> +	tidss_configure_oldi(oldi->tidss, oldi->parent_vp, oldi_cfg);
> +
> +	return 0;
> +}
> +
> +static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge,
> +					 struct drm_bridge_state *old_bridge_state)
> +{
> +	struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
> +	struct drm_atomic_state *state = old_bridge_state->base.state;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_display_mode *mode;
> +
> +	connector = drm_atomic_get_new_connector_for_encoder(state,
> +							     bridge->encoder);
> +	if (WARN_ON(!connector))
> +		return;
> +
> +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> +	if (WARN_ON(!conn_state))
> +		return;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> +	if (WARN_ON(!crtc_state))
> +		return;
> +
> +	mode = &crtc_state->adjusted_mode;
> +
> +	/* Configure the OLDI params*/
> +	tidss_oldi_config(oldi);
> +
> +	/* Set the OLDI serial clock (7 times the pixel clock) */
> +	tidss_oldi_set_serial_clk(oldi, mode->clock * 7 * 1000);
> +
> +	/* Enable OLDI IO power */
> +	tidss_oldi_tx_power(oldi, true);
> +}
> +
> +static void tidss_oldi_atomic_post_disable(struct drm_bridge *bridge,
> +					   struct drm_bridge_state *old_bridge_state)
> +{
> +	struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
> +
> +	/* Disable OLDI IO power */
> +	tidss_oldi_tx_power(oldi, false);
> +
> +	/* Set the OLDI serial clock to IDLE Frequency */
> +	tidss_oldi_set_serial_clk(oldi, OLDI_IDLE_CLK_HZ);
> +
> +	/* Clear OLDI Config */
> +	tidss_configure_oldi(oldi->tidss, oldi->parent_vp, 0);
> +}
> +
> +#define MAX_INPUT_SEL_FORMATS	1
> +
> +static u32 *tidss_oldi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> +						 struct drm_bridge_state *bridge_state,
> +						 struct drm_crtc_state *crtc_state,
> +						 struct drm_connector_state *conn_state,
> +						 u32 output_fmt,
> +						 unsigned int *num_input_fmts)
> +{
> +	struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
> +	u32 *input_fmts;
> +	int i;
> +
> +	*num_input_fmts = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(oldi_bus_formats); i++)
> +		if (oldi_bus_formats[i].bus_fmt == output_fmt)
> +			break;
> +
> +	if (i == ARRAY_SIZE(oldi_bus_formats))
> +		return NULL;
> +
> +	input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
> +			     GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	*num_input_fmts = 1;
> +	input_fmts[0] = oldi_bus_formats[i].input_bus_fmt;
> +	oldi->bus_format = &oldi_bus_formats[i];
> +
> +	return input_fmts;
> +}
> +
> +static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
> +	.attach		= tidss_oldi_bridge_attach,

Looks like an extra tab there?

Other than those two cosmetic issues, I think this looks fine. I also 
tested on AM62-SK.

However, one more thing. We'll have a separate OLDI bridge, but we still 
will have the old OLDI code for AM65x in the tidss_dispc.c. And to mix 
things up, we will have some new OLDI code there too 
(tidss_configure_oldi). Could you check the AM65x specific code and 
perhaps rename the functions to am65x or such, to make this clearer. 
Perhaps also the DISPC_VP_OLDI should be DISPC_VP_OLDI_AM65X, as that VP 
type shouldn't be used for anything else.

  Tomi

Powered by blists - more mailing lists