Hi Lucas,
On Fri, 16 Dec 2022 22:07:42 +0100
Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
> This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a
> full timing generator and can switch between different video sources. On
> the i.MX8MP however the only supported source is the LCDIF. The block
> just needs to be powered up and told about the polarity of the video
> sync signals to act in bypass mode.
>
> Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Tested-by: Marek Vasut <marex@xxxxxxx>
> ---
> drivers/gpu/drm/bridge/imx/Kconfig | 7 +
> drivers/gpu/drm/bridge/imx/Makefile | 1 +
> drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 202 +++++++++++++++++++
> 3 files changed, 210 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
>
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
> index d828d8bfd893..e6cc4000bccd 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -53,4 +53,11 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
> Choose this to enable support for the internal HDMI encoder found
> on the i.MX8MP SoC.
>
> +config DRM_IMX8MP_HDMI_PVI
> + tristate "i.MX8MP HDMI PVI bridge support"
> + depends on OF
> + help
> + Choose this to enable support for the internal HDMI TX Parallel
> + Video Interface found on the i.MX8MP SoC.
> +
> endif # ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
> index 03b0074ae538..b0fd56550dad 100644
> --- a/drivers/gpu/drm/bridge/imx/Makefile
> +++ b/drivers/gpu/drm/bridge/imx/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
> obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o
>
> obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi.o
> +obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o
> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
> new file mode 100644
> index 000000000000..30d40c21dabb
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * Copyright (C) 2022 Pengutronix, Lucas Stach <kernel@xxxxxxxxxxxxxx>
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_crtc.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_graph.h>
> +#include <linux/pm_runtime.h>
> +
> +#define HTX_PVI_CTL 0x0
Personally I would s/CTL/CTRL/, to be consistent with the manual and
thus more search-friendly.
> +#define PVI_CTL_OP_VSYNC_POL BIT(18)
> +#define PVI_CTL_OP_HSYNC_POL BIT(17)
> +#define PVI_CTL_OP_DE_POL BIT(16)
> +#define PVI_CTL_INP_VSYNC_POL BIT(14)
> +#define PVI_CTL_INP_HSYNC_POL BIT(13)
> +#define PVI_CTL_INP_DE_POL BIT(12)
> +#define PVI_CTL_INPUT_LCDIF BIT(2)
According to the reference manual there is actually a 2-bit field here:
HTX_PVI_MODE, using bits 2:1, and whose "LCDIF" value is 0b10. Thus
while it obviously won't change the resulting code, it seems more
correct to define this as (2 << 1).
> +static void imx8mp_hdmi_pvi_bridge_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state)
> +{
> + struct drm_atomic_state *state = bridge_state->base.state;
> + struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge);
> + struct drm_connector_state *conn_state;
> + const struct drm_display_mode *mode;
> + struct drm_crtc_state *crtc_state;
> + struct drm_connector *connector;
> + u32 bus_flags, val;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> +
> + if (WARN_ON(pm_runtime_resume_and_get(pvi->dev)))
> + return;
> +
> + mode = &crtc_state->adjusted_mode;
> +
> + val = PVI_CTL_INPUT_LCDIF;
> +
> + if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> + val |= PVI_CTL_OP_VSYNC_POL | PVI_CTL_INP_VSYNC_POL;
> +
> + if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> + val |= PVI_CTL_OP_HSYNC_POL | PVI_CTL_INP_HSYNC_POL;
> +
> + if (pvi->next_bridge->timings)
> + bus_flags = pvi->next_bridge->timings->input_bus_flags;
> + else if (bridge_state)
> + bus_flags = bridge_state->input_bus_cfg.flags;
> +
> + if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
> + val |= PVI_CTL_OP_DE_POL | PVI_CTL_INP_DE_POL;
> +
> + writel(val, pvi->regs + HTX_PVI_CTL);
> + val |= PVI_CTL_EN;
> + writel(val, pvi->regs + HTX_PVI_CTL);
I guess I'm missing something here: why can't one just write the
register once, with the enable bit set? I tried removing the first
writel() and everything seems to work just the same.
With these fixed:
Reviewed-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
And definitely:
[Tested on a custom board using modetest on v6.5-rc6]
Tested-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
