пт, 6 лист. 2026 р. о 16:38 Thierry Reding <[email protected]> пише:
>
> 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 <[email protected]> # DC command transfer
> > Signed-off-by: Svyatoslav Ryhel <[email protected]>
> > ---
> > 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 <[email protected]>
> > + */
> > +
> > +#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?
>
Check v5 and talk to Mikko!
> > +
> > +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.
>
Actually, decent.
> [...]
> > +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.
>
Aaaaaaaaaand? What you propose?
> > +
> > +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.
>
Interesting idea.
> [...]
> > +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.
>
This is redundant. DBI is quite rare and does not need to be
specifically defined in dtsi
> > +
> > + 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", ®);
> > + 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()?
>
Fair point.
> Thierry
