Re: [PATCH v6 10/10] drm/tegra: Add support for 8-bit CPU interface

[Thierry Reding <thierry.reding@kernel.org>]

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

On Thu, Nov 05, 2026 at 04:05:10PM +0200, Svyatoslav Ryhel wrote:
> The display controller in Tegra20/30 SoCs features an 8-bit SPI interface
> that closely resembles the MIPI DBI Type B protocol and is referred to as
> '8-bit CPU'. Each display controller provides two such interfaces, which
> can be used to send MIPI DCS commands to initialize and control the panel
> while image data is transmitted via 16/18/24-line RGB.
> 
> The '8-bit CPU', although being the DC interface, is modeled using a
> bridge framework for flexibility and convenience. This implementation does
> not change the existing codebase besides registering the bus.
> 
> Suggested-by: Mikko Perttunen <mperttunen@nvidia.com> # DC command transfer
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/gpu/drm/tegra/Kconfig  |   1 +
>  drivers/gpu/drm/tegra/Makefile |   1 +
>  drivers/gpu/drm/tegra/dbi.c    | 576 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/tegra/dc.c     |   4 +
>  drivers/gpu/drm/tegra/dc.h     |   3 +
>  5 files changed, 585 insertions(+)
>  create mode 100644 drivers/gpu/drm/tegra/dbi.c
> 
> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> index 8a3b16aac5d68..6f57fd3985edc 100644
> --- a/drivers/gpu/drm/tegra/Kconfig
> +++ b/drivers/gpu/drm/tegra/Kconfig
> @@ -12,6 +12,7 @@ config DRM_TEGRA
>  	select DRM_BRIDGE_CONNECTOR
>  	select DRM_DISPLAY_DP_AUX_BUS
>  	select DRM_KMS_HELPER
> +	select DRM_MIPI_DBI
>  	select DRM_MIPI_DSI
>  	select DRM_PANEL
>  	select FB_DMAMEM_HELPERS if DRM_FBDEV_EMULATION
> diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
> index e399b40d64a1d..ae7fccb3d6421 100644
> --- a/drivers/gpu/drm/tegra/Makefile
> +++ b/drivers/gpu/drm/tegra/Makefile
> @@ -18,6 +18,7 @@ tegra-drm-y := \
>  	hdmi.o \
>  	mipi-phy.o \
>  	dsi.o \
> +	dbi.o \
>  	sor.o \
>  	dpaux.o \
>  	gr2d.o \
> diff --git a/drivers/gpu/drm/tegra/dbi.c b/drivers/gpu/drm/tegra/dbi.c
> new file mode 100644
> index 0000000000000..32d390f64ddbd
> --- /dev/null
> +++ b/drivers/gpu/drm/tegra/dbi.c
> @@ -0,0 +1,576 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nvidia Tegra20/30 MIPI-DBI Type B host driver
> + *
> + * Copyright (C) 2026 Svyatoslav Ryhel <clamor95@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/property.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_mipi_dbi_bus.h>
> +#include <drm/drm_of.h>
> +
> +#include <video/mipi_display.h>
> +
> +#include "dc.h"
> +
> +enum tegra_dbi_dc_signal {
> +	TEGRA_DBI_COMMAND,
> +	TEGRA_DBI_DATA
> +};

These look like register offsets for sending DBI commands, so maybe just
use defines for them?

> +
> +enum tegra_dbi_iface {
> +	TEGRA_DBI_INTERFACE_PRIMARY,
> +	TEGRA_DBI_INTERFACE_SECONDARY,
> +	TEGRA_DBI_INTERFACE_MAX
> +};

The only place where this seems to be used is in the definition of the
two interfaces. But in those cases PRIMARY and SECONDARY match the index
into the array and MAX is the size of the array, which can be determined
dynamically. Drop this.

> +
> +#define TEGRA_DBI_DATA_PINS_MAX			8
> +#define TEGRA_DBI_DEFAULT_BUS_WIDTH		24
> +
> +struct reg_entry {
> +	u32 offset;
> +	u32 value;
> +};
> +
> +static const u32 tegra_dbi_pin_regs[] = {
> +	DC_COM_PIN_OUTPUT_ENABLE(0),
> +	DC_COM_PIN_OUTPUT_ENABLE(1),
> +	DC_COM_PIN_OUTPUT_ENABLE(2),
> +	DC_COM_PIN_OUTPUT_ENABLE(3),
> +	DC_COM_PIN_OUTPUT_POLARITY(0),
> +	DC_COM_PIN_OUTPUT_POLARITY(1),
> +	DC_COM_PIN_OUTPUT_POLARITY(2),
> +	DC_COM_PIN_OUTPUT_POLARITY(3),
> +	DC_COM_PIN_OUTPUT_DATA(0),
> +	DC_COM_PIN_OUTPUT_DATA(1),
> +	DC_COM_PIN_OUTPUT_DATA(2),
> +	DC_COM_PIN_OUTPUT_DATA(3),
> +	DC_COM_PIN_OUTPUT_SELECT(0),
> +	DC_COM_PIN_OUTPUT_SELECT(1),
> +	DC_COM_PIN_OUTPUT_SELECT(2),
> +	DC_COM_PIN_OUTPUT_SELECT(3),
> +	DC_COM_PIN_OUTPUT_SELECT(4),
> +	DC_COM_PIN_OUTPUT_SELECT(5),
> +	DC_COM_PIN_OUTPUT_SELECT(6),
> +};
> +
> +struct tegra_dbi_priv {
> +	struct device *dev;
> +	struct tegra_dc *dc;
> +
> +	struct mipi_dbi dbi;
> +	struct mipi_dbi_host dbi_host;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next_bridge;
> +
> +	const struct tegra_dbi_interface *interface;
> +	struct reg_entry com_pin_backup[ARRAY_SIZE(tegra_dbi_pin_regs)];

You can just make this a u32 array since you're always iterating over
that tegra_dbi_pin_regs array anyway and can get the offset from there.
No need to duplicate that data into another set of variables.

[...]
> +static void tegra_dbi_pin_set(struct tegra_dc *dc, const struct tegra_dbi_pin *pin, bool value)
> +{
> +	u32 val = BIT(pin->pos + 1); /* unmask is 1 bit above position */
> +
> +	if (value)
> +		val |= BIT(pin->pos);
> +
> +	tegra_dc_writel(dc, val, pin->reg);
> +}

So the PIN_UNMASK exists for all registers. In that case, the definition
should probably not be intermixed like it was in that header "refactor
and expand" patch.

> +
> +static void tegra_dbi_write(struct tegra_dbi_priv *priv,
> +			    enum tegra_dbi_dc_signal signal, u8 value)
> +{
> +	struct tegra_dc *dc = priv->dc;
> +	const struct tegra_dbi_interface *interface = priv->interface;
> +
> +	tegra_dbi_pin_set(dc, &interface->dc, signal);
> +
> +	tegra_dbi_pin_set(dc, &interface->cs, 0);
> +	tegra_dbi_pin_set(dc, &interface->rw, 0);
> +
> +	for (int i = 0; i < TEGRA_DBI_DATA_PINS_MAX; i++)
> +		tegra_dbi_pin_set(dc, &interface->data[i], field_get(BIT(i), value));
> +
> +	tegra_dbi_pin_set(dc, &interface->cs, 1);
> +	tegra_dbi_pin_set(dc, &interface->rw, 1);
> +
> +	udelay(10);

Umpf... I guess we're effectively bit-banging here, but a udelay() here
is potentially a lot of spinning time. If I recall correctly this can
end up being called a lot, on the order of 100 or so times during panel
initialization.

> +
> +static void tegra_dbi_atomic_enable(struct drm_bridge *bridge,
> +				    struct drm_atomic_commit *state)
> +{
> +	struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge);
> +	struct drm_bridge *next_bridge = priv->next_bridge;
> +	struct tegra_dc *dc = priv->dc;
> +	u32 value;
> +
> +	value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
> +	value &= ~DISP_CTRL_MODE_MASK;
> +	tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
> +
> +	/* Disable all options except cursor if enabled */
> +	value = tegra_dc_readl(dc, DC_DISP_DISP_WIN_OPTIONS);
> +	value &= CURSOR_ENABLE;
> +	tegra_dc_writel(dc, value, DC_DISP_DISP_WIN_OPTIONS);
> +
> +	tegra_dc_commit(dc);
> +
> +	tegra_dc_writel(dc, V_PULSE1_ENABLE, DC_DISP_DISP_SIGNAL_OPTIONS0);
> +	tegra_dc_writel(dc, PULSE_POLARITY_LOW, DC_DISP_V_PULSE1_CONTROL);
> +
> +	tegra_dc_writel(dc, PULSE_END(1), DC_DISP_V_PULSE1_POSITION_A);
> +	tegra_dc_writel(dc, 0, DC_DISP_V_PULSE1_POSITION_B);
> +	tegra_dc_writel(dc, 0, DC_DISP_V_PULSE1_POSITION_C);
> +
> +	value = 1 << FRAME_INIT_SEQ_CYCLES_SHIFT |
> +		DC_SIGNAL_VPULSE1 << INIT_SEQ_DC_SIGNAL_SHIFT |
> +		INIT_SEQUENCE_MODE_PLCD | SEND_INIT_SEQUENCE;
> +	tegra_dc_writel(dc, value, DC_DISP_INIT_SEQ_CONTROL);
> +
> +	/* Init cycle 1: WRITE_MEMORY_START, set LCD_PCLK and COMMAND */
> +	tegra_dc_writel(dc, MIPI_DCS_WRITE_MEMORY_START, DC_DISP_SPI_INIT_SEQ_DATA_A);
> +	tegra_dc_writel(dc, 0x0, DC_DISP_SPI_INIT_SEQ_DATA_B);
> +	tegra_dc_writel(dc, 0x0, DC_DISP_SPI_INIT_SEQ_DATA_C);
> +	tegra_dc_writel(dc, BIT(12) | BIT(14), DC_DISP_SPI_INIT_SEQ_DATA_D);
> +
> +	/* Set LDC pin to V Pulse 1 */
> +	value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(6));
> +	value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LDC);
> +	value |= PIN_OUTPUT_SELECT_F4(PIN_OUTPUT_SELECT_LDC);
> +	tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(6));
> +
> +	value = SC1_H_QUALIFIER(SC_H_QUALIFIER_NONE) |
> +		SC0_V_QUALIFIER(SC_V_QUALIFIER_VACTIVE) |
> +		SC0_H_QUALIFIER(SC_H_QUALIFIER_HACTIVE);
> +	tegra_dc_writel(dc, value, DC_DISP_SHIFT_CLOCK_OPTIONS);
> +
> +	value = tegra_dc_readl(dc, DC_DISP_DISP_COLOR_CONTROL);
> +	value &= ~BASE_COLOR_SIZE_MASK;
> +
> +	switch (priv->bus_width) {
> +	case 16:
> +		value |= BASE_COLOR_SIZE_565;
> +		break;
> +	case 18:
> +		value |= BASE_COLOR_SIZE_666;
> +		break;
> +	case 24:
> +		value |= BASE_COLOR_SIZE_888;
> +		break;
> +	default:
> +		WARN(1, "%u bus width not supported\n", priv->bus_width);
> +		value |= BASE_COLOR_SIZE_888;
> +		break;
> +	}
> +
> +	tegra_dc_writel(dc, value, DC_DISP_DISP_COLOR_CONTROL);
> +
> +	/*
> +	 * Backup registers before unshadowing since actual register values
> +	 * may differ and their state backed up now is the correct one.
> +	 */
> +	for (int i = 0; i < ARRAY_SIZE(tegra_dbi_pin_regs); i++)
> +		priv->com_pin_backup[i].value = tegra_dc_readl(dc,
> +			priv->com_pin_backup[i].offset);
> +
> +	/* Unshadow to set up command and data pins */
> +	tegra_dc_writel(dc, WRITE_MUX | READ_MUX, DC_CMD_STATE_ACCESS);
> +
> +	if (priv->interface->interface)
> +		tegra_dbi_init_secondary_interface(priv);
> +	else
> +		tegra_dbi_init_primary_interface(priv);

Maybe hook this into priv->interface directly using a .init() callback.
That way you don't need to branch based on interface index here.

[...]
> +int tegra_dc_dbi_probe(struct tegra_dc *dc)
> +{
> +	struct device *dev = dc->dev;
> +	struct device_node *dbi_np __free(device_node) =
> +		of_get_available_child_by_name(dev_of_node(dev), "dbi");
> +	struct mipi_dbi_device *dbidev;
> +	struct tegra_dbi_priv *priv;
> +	struct device_node *np;
> +	u32 reg;
> +	int ret;
> +
> +	/* Missing node means that DBI is not used, continue DC probe */
> +	if (!dbi_np)
> +		return 0;

Maybe this should have an extra check to see if dbi_np is available.
That would help create this node in a DTSI file and just keep it
disabled by default.

> +
> +	priv = devm_drm_bridge_alloc(dev, struct tegra_dbi_priv, bridge,
> +				     &tegra_dbi_funcs);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
> +
> +	priv->dc = dc;
> +	priv->dev = dev;
> +
> +	ret = of_property_read_u32(dbi_np, "reg", &reg);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get reg\n");
> +
> +	if (reg != 0 && reg != 1)
> +		return dev_err_probe(dev, -EINVAL, "Wrong reg value: %d\n", reg);

Maybe sort these before the call to devm_drm_bridge_alloc()?

Thierry

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