Message ID | 1613619715-28785-1-git-send-email-victor.liu@nxp.com |
---|---|
Headers | show |
Series | Add some DRM bridge drivers support for i.MX8qm/qxp SoCs | expand |
Hey Liu, This patch looks good to me. Reviewed-by: Robert Foss <robert.foss@linaro.org> On Thu, 18 Feb 2021 at 04:56, Liu Ying <victor.liu@nxp.com> wrote: > > This patch adds documentations for RGB666_1X30_CPADLO, RGB888_1X30_CPADLO, > RGB666_1X36_CPADLO and RGB888_1X36_CPADLO bus formats used by i.MX8qm/qxp > pixel combiner. The RGB pixels with padding low per component are > transmitted on a 30-bit input bus(10-bit per component) from a display > controller or a 36-bit output bus(12-bit per component) to a pixel link. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v3->v4: > * No change. > > v2->v3: > * No change. > > v1->v2: > * No change. > > .../userspace-api/media/v4l/subdev-formats.rst | 156 +++++++++++++++++++++ > 1 file changed, 156 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst > index 7f16cbe..201c16d 100644 > --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst > +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst > @@ -1488,6 +1488,80 @@ The following tables list existing packed RGB formats. > - b\ :sub:`2` > - b\ :sub:`1` > - b\ :sub:`0` > + * .. _MEDIA-BUS-FMT-RGB666-1X30-CPADLO: > + > + - MEDIA_BUS_FMT_RGB666_1X30-CPADLO > + - 0x101e > + - > + - 0 > + - 0 > + - r\ :sub:`5` > + - r\ :sub:`4` > + - r\ :sub:`3` > + - r\ :sub:`2` > + - r\ :sub:`1` > + - r\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > + - g\ :sub:`5` > + - g\ :sub:`4` > + - g\ :sub:`3` > + - g\ :sub:`2` > + - g\ :sub:`1` > + - g\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > + - b\ :sub:`5` > + - b\ :sub:`4` > + - b\ :sub:`3` > + - b\ :sub:`2` > + - b\ :sub:`1` > + - b\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > + * .. _MEDIA-BUS-FMT-RGB888-1X30-CPADLO: > + > + - MEDIA_BUS_FMT_RGB888_1X30-CPADLO > + - 0x101f > + - > + - 0 > + - 0 > + - r\ :sub:`7` > + - r\ :sub:`6` > + - r\ :sub:`5` > + - r\ :sub:`4` > + - r\ :sub:`3` > + - r\ :sub:`2` > + - r\ :sub:`1` > + - r\ :sub:`0` > + - 0 > + - 0 > + - g\ :sub:`7` > + - g\ :sub:`6` > + - g\ :sub:`5` > + - g\ :sub:`4` > + - g\ :sub:`3` > + - g\ :sub:`2` > + - g\ :sub:`1` > + - g\ :sub:`0` > + - 0 > + - 0 > + - b\ :sub:`7` > + - b\ :sub:`6` > + - b\ :sub:`5` > + - b\ :sub:`4` > + - b\ :sub:`3` > + - b\ :sub:`2` > + - b\ :sub:`1` > + - b\ :sub:`0` > + - 0 > + - 0 > * .. _MEDIA-BUS-FMT-ARGB888-1X32: > > - MEDIA_BUS_FMT_ARGB888_1X32 > @@ -1665,6 +1739,88 @@ The following table list existing packed 36bit wide RGB formats. > - 2 > - 1 > - 0 > + * .. _MEDIA-BUS-FMT-RGB666-1X36-CPADLO: > + > + - MEDIA_BUS_FMT_RGB666_1X36_CPADLO > + - 0x1020 > + - > + - r\ :sub:`5` > + - r\ :sub:`4` > + - r\ :sub:`3` > + - r\ :sub:`2` > + - r\ :sub:`1` > + - r\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > + - 0 > + - 0 > + - g\ :sub:`5` > + - g\ :sub:`4` > + - g\ :sub:`3` > + - g\ :sub:`2` > + - g\ :sub:`1` > + - g\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > + - 0 > + - 0 > + - b\ :sub:`5` > + - b\ :sub:`4` > + - b\ :sub:`3` > + - b\ :sub:`2` > + - b\ :sub:`1` > + - b\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > + - 0 > + - 0 > + * .. _MEDIA-BUS-FMT-RGB888-1X36-CPADLO: > + > + - MEDIA_BUS_FMT_RGB888_1X36_CPADLO > + - 0x1021 > + - > + - r\ :sub:`7` > + - r\ :sub:`6` > + - r\ :sub:`5` > + - r\ :sub:`4` > + - r\ :sub:`3` > + - r\ :sub:`2` > + - r\ :sub:`1` > + - r\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > + - g\ :sub:`7` > + - g\ :sub:`6` > + - g\ :sub:`5` > + - g\ :sub:`4` > + - g\ :sub:`3` > + - g\ :sub:`2` > + - g\ :sub:`1` > + - g\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > + - b\ :sub:`7` > + - b\ :sub:`6` > + - b\ :sub:`5` > + - b\ :sub:`4` > + - b\ :sub:`3` > + - b\ :sub:`2` > + - b\ :sub:`1` > + - b\ :sub:`0` > + - 0 > + - 0 > + - 0 > + - 0 > * .. _MEDIA-BUS-FMT-RGB121212-1X36: > > - MEDIA_BUS_FMT_RGB121212_1X36 > -- > 2.7.4 >
Hey Liu, With the below nit straightened out, feel free to add my r-b. Reviewed-by: Robert Foss <robert.foss@linaro.org> On Thu, 18 Feb 2021 at 04:58, Liu Ying <victor.liu@nxp.com> wrote: > > This patch adds a drm bridge driver for i.MX8qm/qxp pixel combiner. > The pixel combiner takes two output streams from a single display > controller and manipulates the two streams to support a number > of modes(bypass, pixel combine, YUV444 to YUV422, split_RGB) configured > as either one screen, two screens, or virtual screens. The pixel > combiner is also responsible for generating some of the control signals > for the pixel link output channel. For now, the driver only supports > the bypass mode. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v3->v4: > * No change. > > v2->v3: > * No change. > > v1->v2: > * No change. > > drivers/gpu/drm/bridge/Kconfig | 2 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/imx/Kconfig | 8 + > drivers/gpu/drm/bridge/imx/Makefile | 1 + > .../gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 452 +++++++++++++++++++++ > 5 files changed, 464 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/imx/Kconfig > create mode 100644 drivers/gpu/drm/bridge/imx/Makefile > create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index e4110d6c..84944e0 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -256,6 +256,8 @@ source "drivers/gpu/drm/bridge/adv7511/Kconfig" > > source "drivers/gpu/drm/bridge/cadence/Kconfig" > > +source "drivers/gpu/drm/bridge/imx/Kconfig" > + > source "drivers/gpu/drm/bridge/synopsys/Kconfig" > > endmenu > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 86e7acc..bc80cae 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -27,4 +27,5 @@ obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o > > obj-y += analogix/ > obj-y += cadence/ > +obj-y += imx/ > obj-y += synopsys/ > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig > new file mode 100644 > index 00000000..f1c91b6 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > @@ -0,0 +1,8 @@ > +config DRM_IMX8QXP_PIXEL_COMBINER > + tristate "Freescale i.MX8QM/QXP pixel combiner" > + depends on OF > + depends on COMMON_CLK > + select DRM_KMS_HELPER > + help > + Choose this to enable pixel combiner found in > + Freescale i.MX8qm/qxp processors. > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile > new file mode 100644 > index 00000000..7d7c8d6 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > new file mode 100644 > index 00000000..cd5b1be > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > @@ -0,0 +1,452 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/* > + * Copyright 2020 NXP > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_graph.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > + > +#include <drm/drm_atomic_state_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_print.h> > + > +#define PC_CTRL_REG 0x0 > +#define PC_COMBINE_ENABLE BIT(0) > +#define PC_DISP_BYPASS(n) BIT(1 + 21 * (n)) > +#define PC_DISP_HSYNC_POLARITY(n) BIT(2 + 11 * (n)) > +#define PC_DISP_HSYNC_POLARITY_POS(n) DISP_HSYNC_POLARITY(n) > +#define PC_DISP_VSYNC_POLARITY(n) BIT(3 + 11 * (n)) > +#define PC_DISP_VSYNC_POLARITY_POS(n) DISP_VSYNC_POLARITY(n) > +#define PC_DISP_DVALID_POLARITY(n) BIT(4 + 11 * (n)) > +#define PC_DISP_DVALID_POLARITY_POS(n) DISP_DVALID_POLARITY(n) > +#define PC_VSYNC_MASK_ENABLE BIT(5) > +#define PC_SKIP_MODE BIT(6) > +#define PC_SKIP_NUMBER_MASK GENMASK(12, 7) > +#define PC_SKIP_NUMBER(n) FIELD_PREP(PC_SKIP_NUMBER_MASK, (n)) > +#define PC_DISP0_PIX_DATA_FORMAT_MASK GENMASK(18, 16) > +#define PC_DISP0_PIX_DATA_FORMAT(fmt) \ > + FIELD_PREP(PC_DISP0_PIX_DATA_FORMAT_MASK, (fmt)) > +#define PC_DISP1_PIX_DATA_FORMAT_MASK GENMASK(21, 19) > +#define PC_DISP1_PIX_DATA_FORMAT(fmt) \ > + FIELD_PREP(PC_DISP1_PIX_DATA_FORMAT_MASK, (fmt)) > + > +#define PC_BUF_PARA_REG 0x10 This register is unused, keeping it in here to avoid future headaches seems like a good idea. > +#define PC_BUF_ACTIVE_DEPTH_MASK GENMASK(10, 0) > +#define PC_BUF_ACTIVE_DEPTH(n) FIELD_PREP(PC_BUF_ACTIVE_DEPTH_MASK, (n)) > + > +#define PC_SW_RESET_REG 0x20 > +#define PC_SW_RESET_N BIT(0) > +#define PC_DISP_SW_RESET_N(n) BIT(1 + (n)) > +#define PC_FULL_RESET_N (PC_SW_RESET_N | \ > + PC_DISP_SW_RESET_N(0) | \ > + PC_DISP_SW_RESET_N(1)) > + > +#define PC_REG_SET 0x4 > +#define PC_REG_CLR 0x8 > + > +#define DRIVER_NAME "imx8qxp-pixel-combiner" > + > +enum imx8qxp_pc_pix_data_format { > + RGB, > + YUV444, > + YUV422, > + SPLIT_RGB, YUV444, YUV422 & SPLIT_RGB are also unused, but if their values are compatible with the PC_DISP0_PIX_DATA_FORMAT macro I think keeping them around for future reference is a good idea. > +}; > + > +struct imx8qxp_pc_channel { > + struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > + struct imx8qxp_pc *pc; > + unsigned int stream_id; > + bool is_available; > +}; > + > +struct imx8qxp_pc { > + struct device *dev; > + struct imx8qxp_pc_channel ch[2]; > + struct clk *clk_apb; > + void __iomem *base; > +}; > + > +static inline u32 imx8qxp_pc_read(struct imx8qxp_pc *pc, unsigned int offset) > +{ > + return readl(pc->base + offset); > +} > + > +static inline void > +imx8qxp_pc_write(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > +{ > + writel(value, pc->base + offset); > +} > + > +static inline void > +imx8qxp_pc_write_set(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > +{ > + imx8qxp_pc_write(pc, offset + PC_REG_SET, value); > +} > + > +static inline void > +imx8qxp_pc_write_clr(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > +{ > + imx8qxp_pc_write(pc, offset + PC_REG_CLR, value); > +} > + > +static enum drm_mode_status > +imx8qxp_pc_bridge_mode_valid(struct drm_bridge *bridge, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + if (mode->hdisplay > 2560) > + return MODE_BAD_HVALUE; > + > + return MODE_OK; > +} > + > +static int imx8qxp_pc_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > + struct imx8qxp_pc *pc = ch->pc; > + > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > + DRM_DEV_ERROR(pc->dev, > + "do not support creating a drm_connector\n"); > + return -EINVAL; > + } > + > + if (!bridge->encoder) { > + DRM_DEV_ERROR(pc->dev, "missing encoder\n"); > + return -ENODEV; > + } > + > + return drm_bridge_attach(bridge->encoder, > + ch->next_bridge, bridge, > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > +} > + > +static void > +imx8qxp_pc_bridge_mode_set(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + const struct drm_display_mode *adjusted_mode) > +{ > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > + struct imx8qxp_pc *pc = ch->pc; > + u32 val; > + int ret; > + > + ret = pm_runtime_get_sync(pc->dev); > + if (ret < 0) > + DRM_DEV_ERROR(pc->dev, > + "failed to get runtime PM sync: %d\n", ret); > + > + ret = clk_prepare_enable(pc->clk_apb); > + if (ret) > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > + __func__, ret); > + > + /* HSYNC to pixel link is active low. */ > + imx8qxp_pc_write_clr(pc, PC_CTRL_REG, > + PC_DISP_HSYNC_POLARITY(ch->stream_id)); > + > + /* VSYNC to pixel link is active low. */ > + imx8qxp_pc_write_clr(pc, PC_CTRL_REG, > + PC_DISP_VSYNC_POLARITY(ch->stream_id)); > + > + /* Data enable to pixel link is active high. */ > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, > + PC_DISP_DVALID_POLARITY(ch->stream_id)); > + > + /* Mask the first frame output which may be incomplete. */ > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, PC_VSYNC_MASK_ENABLE); > + > + /* Only support RGB currently. */ > + val = imx8qxp_pc_read(pc, PC_CTRL_REG); > + if (ch->stream_id == 0) { > + val &= ~PC_DISP0_PIX_DATA_FORMAT_MASK; > + val |= PC_DISP0_PIX_DATA_FORMAT(RGB); > + } else { > + val &= ~PC_DISP1_PIX_DATA_FORMAT_MASK; > + val |= PC_DISP1_PIX_DATA_FORMAT(RGB); > + } > + imx8qxp_pc_write(pc, PC_CTRL_REG, val); > + > + /* Only support bypass mode currently. */ > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, PC_DISP_BYPASS(ch->stream_id)); > + > + clk_disable_unprepare(pc->clk_apb); > +} > + > +static void imx8qxp_pc_bridge_atomic_disable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > +{ > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > + struct imx8qxp_pc *pc = ch->pc; > + int ret; > + > + ret = pm_runtime_put(pc->dev); > + if (ret < 0) > + DRM_DEV_ERROR(pc->dev, "failed to put runtime PM: %d\n", ret); > +} > + > +static const u32 imx8qxp_pc_bus_output_fmts[] = { > + MEDIA_BUS_FMT_RGB888_1X36_CPADLO, > + MEDIA_BUS_FMT_RGB666_1X36_CPADLO, > +}; > + > +static bool imx8qxp_pc_bus_output_fmt_supported(u32 fmt) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(imx8qxp_pc_bus_output_fmts); i++) { > + if (imx8qxp_pc_bus_output_fmts[i] == fmt) > + return true; > + } > + > + return false; > +} > + > +static u32 * > +imx8qxp_pc_bridge_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) > +{ > + u32 *input_fmts; > + > + if (!imx8qxp_pc_bus_output_fmt_supported(output_fmt)) > + return NULL; > + > + *num_input_fmts = 1; > + > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + switch (output_fmt) { > + case MEDIA_BUS_FMT_RGB888_1X36_CPADLO: > + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X30_CPADLO; > + break; > + case MEDIA_BUS_FMT_RGB666_1X36_CPADLO: > + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X30_CPADLO; > + break; > + default: > + kfree(input_fmts); > + input_fmts = NULL; > + break; > + } > + > + return input_fmts; > +} > + > +static u32 * > +imx8qxp_pc_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + unsigned int *num_output_fmts) > +{ > + *num_output_fmts = ARRAY_SIZE(imx8qxp_pc_bus_output_fmts); > + return kmemdup(imx8qxp_pc_bus_output_fmts, > + sizeof(imx8qxp_pc_bus_output_fmts), GFP_KERNEL); > +} > + > +static const struct drm_bridge_funcs imx8qxp_pc_bridge_funcs = { > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_reset = drm_atomic_helper_bridge_reset, > + .mode_valid = imx8qxp_pc_bridge_mode_valid, > + .attach = imx8qxp_pc_bridge_attach, > + .mode_set = imx8qxp_pc_bridge_mode_set, > + .atomic_disable = imx8qxp_pc_bridge_atomic_disable, > + .atomic_get_input_bus_fmts = > + imx8qxp_pc_bridge_atomic_get_input_bus_fmts, > + .atomic_get_output_bus_fmts = > + imx8qxp_pc_bridge_atomic_get_output_bus_fmts, > +}; > + > +static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) > +{ > + struct imx8qxp_pc *pc; > + struct imx8qxp_pc_channel *ch; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct device_node *child, *remote; > + u32 i; > + int ret; > + > + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL); > + if (!pc) > + return -ENOMEM; > + > + pc->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(pc->base)) > + return PTR_ERR(pc->base); > + > + pc->dev = dev; > + > + pc->clk_apb = devm_clk_get(dev, "apb"); > + if (IS_ERR(pc->clk_apb)) { > + ret = PTR_ERR(pc->clk_apb); > + if (ret != -EPROBE_DEFER) > + DRM_DEV_ERROR(dev, "failed to get apb clock: %d\n", > + ret); > + return ret; > + } > + > + platform_set_drvdata(pdev, pc); > + pm_runtime_enable(dev); > + > + for_each_available_child_of_node(np, child) { > + ret = of_property_read_u32(child, "reg", &i); > + if (ret || i > 1) { > + ret = -EINVAL; > + DRM_DEV_ERROR(dev, > + "invalid channel(%u) node address\n", i); > + goto free_child; > + } > + > + ch = &pc->ch[i]; > + ch->pc = pc; > + ch->stream_id = i; > + > + remote = of_graph_get_remote_node(child, 1, 0); > + if (!remote) { > + ret = -ENODEV; > + DRM_DEV_ERROR(dev, > + "channel%u failed to get port1's remote node: %d\n", > + i, ret); > + goto free_child; > + } > + > + ch->next_bridge = of_drm_find_bridge(remote); > + if (!ch->next_bridge) { > + of_node_put(remote); > + ret = -EPROBE_DEFER; > + DRM_DEV_DEBUG_DRIVER(dev, > + "channel%u failed to find next bridge: %d\n", > + i, ret); > + goto free_child; > + } > + > + of_node_put(remote); > + > + ch->bridge.driver_private = ch; > + ch->bridge.funcs = &imx8qxp_pc_bridge_funcs; > + ch->bridge.of_node = child; > + ch->is_available = true; > + > + drm_bridge_add(&ch->bridge); > + } > + > + return 0; > + > +free_child: > + of_node_put(child); > + > + if (i == 1 && pc->ch[0].next_bridge) > + drm_bridge_remove(&pc->ch[0].bridge); > + > + pm_runtime_disable(dev); > + return ret; > +} > + > +static int imx8qxp_pc_bridge_remove(struct platform_device *pdev) > +{ > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > + struct imx8qxp_pc_channel *ch; > + int i; > + > + for (i = 0; i < 2; i++) { > + ch = &pc->ch[i]; > + > + if (!ch->is_available) > + continue; > + > + drm_bridge_remove(&ch->bridge); > + ch->is_available = false; > + } > + > + pm_runtime_disable(&pdev->dev); > + > + return 0; > +} > + > +static int __maybe_unused imx8qxp_pc_runtime_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > + int ret; > + > + ret = clk_prepare_enable(pc->clk_apb); > + if (ret) > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > + __func__, ret); > + > + /* Disable pixel combiner by full reset. */ > + imx8qxp_pc_write_clr(pc, PC_SW_RESET_REG, PC_FULL_RESET_N); > + > + clk_disable_unprepare(pc->clk_apb); > + > + /* Ensure the reset takes effect. */ > + usleep_range(10, 20); > + > + return ret; > +} > + > +static int __maybe_unused imx8qxp_pc_runtime_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > + int ret; > + > + ret = clk_prepare_enable(pc->clk_apb); > + if (ret) { > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > + __func__, ret); > + return ret; > + } > + > + /* out of reset */ > + imx8qxp_pc_write_set(pc, PC_SW_RESET_REG, PC_FULL_RESET_N); > + > + clk_disable_unprepare(pc->clk_apb); > + > + return ret; > +} > + > +static const struct dev_pm_ops imx8qxp_pc_pm_ops = { > + SET_RUNTIME_PM_OPS(imx8qxp_pc_runtime_suspend, > + imx8qxp_pc_runtime_resume, NULL) > +}; > + > +static const struct of_device_id imx8qxp_pc_dt_ids[] = { > + { .compatible = "fsl,imx8qm-pixel-combiner", }, > + { .compatible = "fsl,imx8qxp-pixel-combiner", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, imx8qxp_pc_dt_ids); > + > +static struct platform_driver imx8qxp_pc_bridge_driver = { > + .probe = imx8qxp_pc_bridge_probe, > + .remove = imx8qxp_pc_bridge_remove, > + .driver = { > + .pm = &imx8qxp_pc_pm_ops, > + .name = DRIVER_NAME, > + .of_match_table = imx8qxp_pc_dt_ids, > + }, > +}; > +module_platform_driver(imx8qxp_pc_bridge_driver); > + > +MODULE_DESCRIPTION("i.MX8QM/QXP pixel combiner bridge driver"); > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" DRIVER_NAME); > -- > 2.7.4 >
Hi Robert, On Fri, 2021-02-26 at 14:07 +0100, Robert Foss wrote: > Hey Liu, > > With the below nit straightened out, feel free to add my r-b. > > Reviewed-by: Robert Foss <robert.foss@linaro.org> Thanks for reviewing this patch. > > On Thu, 18 Feb 2021 at 04:58, Liu Ying <victor.liu@nxp.com> wrote: > > This patch adds a drm bridge driver for i.MX8qm/qxp pixel combiner. > > The pixel combiner takes two output streams from a single display > > controller and manipulates the two streams to support a number > > of modes(bypass, pixel combine, YUV444 to YUV422, split_RGB) configured > > as either one screen, two screens, or virtual screens. The pixel > > combiner is also responsible for generating some of the control signals > > for the pixel link output channel. For now, the driver only supports > > the bypass mode. > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > v3->v4: > > * No change. > > > > v2->v3: > > * No change. > > > > v1->v2: > > * No change. > > > > drivers/gpu/drm/bridge/Kconfig | 2 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/imx/Kconfig | 8 + > > drivers/gpu/drm/bridge/imx/Makefile | 1 + > > .../gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 452 +++++++++++++++++++++ > > 5 files changed, 464 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/imx/Kconfig > > create mode 100644 drivers/gpu/drm/bridge/imx/Makefile > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > > index e4110d6c..84944e0 100644 > > --- a/drivers/gpu/drm/bridge/Kconfig > > +++ b/drivers/gpu/drm/bridge/Kconfig > > @@ -256,6 +256,8 @@ source "drivers/gpu/drm/bridge/adv7511/Kconfig" > > > > source "drivers/gpu/drm/bridge/cadence/Kconfig" > > > > +source "drivers/gpu/drm/bridge/imx/Kconfig" > > + > > source "drivers/gpu/drm/bridge/synopsys/Kconfig" > > > > endmenu > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > > index 86e7acc..bc80cae 100644 > > --- a/drivers/gpu/drm/bridge/Makefile > > +++ b/drivers/gpu/drm/bridge/Makefile > > @@ -27,4 +27,5 @@ obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o > > > > obj-y += analogix/ > > obj-y += cadence/ > > +obj-y += imx/ > > obj-y += synopsys/ > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig > > new file mode 100644 > > index 00000000..f1c91b6 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > > @@ -0,0 +1,8 @@ > > +config DRM_IMX8QXP_PIXEL_COMBINER > > + tristate "Freescale i.MX8QM/QXP pixel combiner" > > + depends on OF > > + depends on COMMON_CLK > > + select DRM_KMS_HELPER > > + help > > + Choose this to enable pixel combiner found in > > + Freescale i.MX8qm/qxp processors. > > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile > > new file mode 100644 > > index 00000000..7d7c8d6 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/imx/Makefile > > @@ -0,0 +1 @@ > > +obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o > > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > new file mode 100644 > > index 00000000..cd5b1be > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > @@ -0,0 +1,452 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +/* > > + * Copyright 2020 NXP > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_graph.h> > > +#include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > + > > +#include <drm/drm_atomic_state_helper.h> > > +#include <drm/drm_bridge.h> > > +#include <drm/drm_print.h> > > + > > +#define PC_CTRL_REG 0x0 > > +#define PC_COMBINE_ENABLE BIT(0) > > +#define PC_DISP_BYPASS(n) BIT(1 + 21 * (n)) > > +#define PC_DISP_HSYNC_POLARITY(n) BIT(2 + 11 * (n)) > > +#define PC_DISP_HSYNC_POLARITY_POS(n) DISP_HSYNC_POLARITY(n) > > +#define PC_DISP_VSYNC_POLARITY(n) BIT(3 + 11 * (n)) > > +#define PC_DISP_VSYNC_POLARITY_POS(n) DISP_VSYNC_POLARITY(n) > > +#define PC_DISP_DVALID_POLARITY(n) BIT(4 + 11 * (n)) > > +#define PC_DISP_DVALID_POLARITY_POS(n) DISP_DVALID_POLARITY(n) > > +#define PC_VSYNC_MASK_ENABLE BIT(5) > > +#define PC_SKIP_MODE BIT(6) > > +#define PC_SKIP_NUMBER_MASK GENMASK(12, 7) > > +#define PC_SKIP_NUMBER(n) FIELD_PREP(PC_SKIP_NUMBER_MASK, (n)) > > +#define PC_DISP0_PIX_DATA_FORMAT_MASK GENMASK(18, 16) > > +#define PC_DISP0_PIX_DATA_FORMAT(fmt) \ > > + FIELD_PREP(PC_DISP0_PIX_DATA_FORMAT_MASK, (fmt)) > > +#define PC_DISP1_PIX_DATA_FORMAT_MASK GENMASK(21, 19) > > +#define PC_DISP1_PIX_DATA_FORMAT(fmt) \ > > + FIELD_PREP(PC_DISP1_PIX_DATA_FORMAT_MASK, (fmt)) > > + > > +#define PC_BUF_PARA_REG 0x10 > > This register is unused, keeping it in here to avoid future headaches > seems like a good idea. Yes, for now, this register is unused. It will be used to set the below PC_BUF_ACTIVE_DEPTH field when non-bypass modes are enabled, I think. Though you said 'keeping it', you actually want me to drop it for now, right? > > > +#define PC_BUF_ACTIVE_DEPTH_MASK GENMASK(10, 0) > > +#define PC_BUF_ACTIVE_DEPTH(n) FIELD_PREP(PC_BUF_ACTIVE_DEPTH_MASK, (n)) > > + > > +#define PC_SW_RESET_REG 0x20 > > +#define PC_SW_RESET_N BIT(0) > > +#define PC_DISP_SW_RESET_N(n) BIT(1 + (n)) > > +#define PC_FULL_RESET_N (PC_SW_RESET_N | \ > > + PC_DISP_SW_RESET_N(0) | \ > > + PC_DISP_SW_RESET_N(1)) > > + > > +#define PC_REG_SET 0x4 > > +#define PC_REG_CLR 0x8 > > + > > +#define DRIVER_NAME "imx8qxp-pixel-combiner" > > + > > +enum imx8qxp_pc_pix_data_format { > > + RGB, > > + YUV444, > > + YUV422, > > + SPLIT_RGB, > > YUV444, YUV422 & SPLIT_RGB are also unused, but if their values are > compatible with the PC_DISP0_PIX_DATA_FORMAT macro I think keeping > them around for future reference is a good idea. Yes, YUV444, YUV422 & SPLIT_RGB are compatible with the PC_DISP{0, 1}_PIX_DATA_FORMAT macros, so I'll keep them here. Regards, Liu Ying > > > +}; > > + > > +struct imx8qxp_pc_channel { > > + struct drm_bridge bridge; > > + struct drm_bridge *next_bridge; > > + struct imx8qxp_pc *pc; > > + unsigned int stream_id; > > + bool is_available; > > +}; > > + > > +struct imx8qxp_pc { > > + struct device *dev; > > + struct imx8qxp_pc_channel ch[2]; > > + struct clk *clk_apb; > > + void __iomem *base; > > +}; > > + > > +static inline u32 imx8qxp_pc_read(struct imx8qxp_pc *pc, unsigned int offset) > > +{ > > + return readl(pc->base + offset); > > +} > > + > > +static inline void > > +imx8qxp_pc_write(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > +{ > > + writel(value, pc->base + offset); > > +} > > + > > +static inline void > > +imx8qxp_pc_write_set(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > +{ > > + imx8qxp_pc_write(pc, offset + PC_REG_SET, value); > > +} > > + > > +static inline void > > +imx8qxp_pc_write_clr(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > +{ > > + imx8qxp_pc_write(pc, offset + PC_REG_CLR, value); > > +} > > + > > +static enum drm_mode_status > > +imx8qxp_pc_bridge_mode_valid(struct drm_bridge *bridge, > > + const struct drm_display_info *info, > > + const struct drm_display_mode *mode) > > +{ > > + if (mode->hdisplay > 2560) > > + return MODE_BAD_HVALUE; > > + > > + return MODE_OK; > > +} > > + > > +static int imx8qxp_pc_bridge_attach(struct drm_bridge *bridge, > > + enum drm_bridge_attach_flags flags) > > +{ > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > + struct imx8qxp_pc *pc = ch->pc; > > + > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > + DRM_DEV_ERROR(pc->dev, > > + "do not support creating a drm_connector\n"); > > + return -EINVAL; > > + } > > + > > + if (!bridge->encoder) { > > + DRM_DEV_ERROR(pc->dev, "missing encoder\n"); > > + return -ENODEV; > > + } > > + > > + return drm_bridge_attach(bridge->encoder, > > + ch->next_bridge, bridge, > > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > +} > > + > > +static void > > +imx8qxp_pc_bridge_mode_set(struct drm_bridge *bridge, > > + const struct drm_display_mode *mode, > > + const struct drm_display_mode *adjusted_mode) > > +{ > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > + struct imx8qxp_pc *pc = ch->pc; > > + u32 val; > > + int ret; > > + > > + ret = pm_runtime_get_sync(pc->dev); > > + if (ret < 0) > > + DRM_DEV_ERROR(pc->dev, > > + "failed to get runtime PM sync: %d\n", ret); > > + > > + ret = clk_prepare_enable(pc->clk_apb); > > + if (ret) > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > + __func__, ret); > > + > > + /* HSYNC to pixel link is active low. */ > > + imx8qxp_pc_write_clr(pc, PC_CTRL_REG, > > + PC_DISP_HSYNC_POLARITY(ch->stream_id)); > > + > > + /* VSYNC to pixel link is active low. */ > > + imx8qxp_pc_write_clr(pc, PC_CTRL_REG, > > + PC_DISP_VSYNC_POLARITY(ch->stream_id)); > > + > > + /* Data enable to pixel link is active high. */ > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, > > + PC_DISP_DVALID_POLARITY(ch->stream_id)); > > + > > + /* Mask the first frame output which may be incomplete. */ > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, PC_VSYNC_MASK_ENABLE); > > + > > + /* Only support RGB currently. */ > > + val = imx8qxp_pc_read(pc, PC_CTRL_REG); > > + if (ch->stream_id == 0) { > > + val &= ~PC_DISP0_PIX_DATA_FORMAT_MASK; > > + val |= PC_DISP0_PIX_DATA_FORMAT(RGB); > > + } else { > > + val &= ~PC_DISP1_PIX_DATA_FORMAT_MASK; > > + val |= PC_DISP1_PIX_DATA_FORMAT(RGB); > > + } > > + imx8qxp_pc_write(pc, PC_CTRL_REG, val); > > + > > + /* Only support bypass mode currently. */ > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, PC_DISP_BYPASS(ch->stream_id)); > > + > > + clk_disable_unprepare(pc->clk_apb); > > +} > > + > > +static void imx8qxp_pc_bridge_atomic_disable(struct drm_bridge *bridge, > > + struct drm_bridge_state *old_bridge_state) > > +{ > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > + struct imx8qxp_pc *pc = ch->pc; > > + int ret; > > + > > + ret = pm_runtime_put(pc->dev); > > + if (ret < 0) > > + DRM_DEV_ERROR(pc->dev, "failed to put runtime PM: %d\n", ret); > > +} > > + > > +static const u32 imx8qxp_pc_bus_output_fmts[] = { > > + MEDIA_BUS_FMT_RGB888_1X36_CPADLO, > > + MEDIA_BUS_FMT_RGB666_1X36_CPADLO, > > +}; > > + > > +static bool imx8qxp_pc_bus_output_fmt_supported(u32 fmt) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(imx8qxp_pc_bus_output_fmts); i++) { > > + if (imx8qxp_pc_bus_output_fmts[i] == fmt) > > + return true; > > + } > > + > > + return false; > > +} > > + > > +static u32 * > > +imx8qxp_pc_bridge_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) > > +{ > > + u32 *input_fmts; > > + > > + if (!imx8qxp_pc_bus_output_fmt_supported(output_fmt)) > > + return NULL; > > + > > + *num_input_fmts = 1; > > + > > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > > + if (!input_fmts) > > + return NULL; > > + > > + switch (output_fmt) { > > + case MEDIA_BUS_FMT_RGB888_1X36_CPADLO: > > + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X30_CPADLO; > > + break; > > + case MEDIA_BUS_FMT_RGB666_1X36_CPADLO: > > + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X30_CPADLO; > > + break; > > + default: > > + kfree(input_fmts); > > + input_fmts = NULL; > > + break; > > + } > > + > > + return input_fmts; > > +} > > + > > +static u32 * > > +imx8qxp_pc_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > > + struct drm_bridge_state *bridge_state, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state, > > + unsigned int *num_output_fmts) > > +{ > > + *num_output_fmts = ARRAY_SIZE(imx8qxp_pc_bus_output_fmts); > > + return kmemdup(imx8qxp_pc_bus_output_fmts, > > + sizeof(imx8qxp_pc_bus_output_fmts), GFP_KERNEL); > > +} > > + > > +static const struct drm_bridge_funcs imx8qxp_pc_bridge_funcs = { > > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > + .atomic_reset = drm_atomic_helper_bridge_reset, > > + .mode_valid = imx8qxp_pc_bridge_mode_valid, > > + .attach = imx8qxp_pc_bridge_attach, > > + .mode_set = imx8qxp_pc_bridge_mode_set, > > + .atomic_disable = imx8qxp_pc_bridge_atomic_disable, > > + .atomic_get_input_bus_fmts = > > + imx8qxp_pc_bridge_atomic_get_input_bus_fmts, > > + .atomic_get_output_bus_fmts = > > + imx8qxp_pc_bridge_atomic_get_output_bus_fmts, > > +}; > > + > > +static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) > > +{ > > + struct imx8qxp_pc *pc; > > + struct imx8qxp_pc_channel *ch; > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + struct device_node *child, *remote; > > + u32 i; > > + int ret; > > + > > + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL); > > + if (!pc) > > + return -ENOMEM; > > + > > + pc->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(pc->base)) > > + return PTR_ERR(pc->base); > > + > > + pc->dev = dev; > > + > > + pc->clk_apb = devm_clk_get(dev, "apb"); > > + if (IS_ERR(pc->clk_apb)) { > > + ret = PTR_ERR(pc->clk_apb); > > + if (ret != -EPROBE_DEFER) > > + DRM_DEV_ERROR(dev, "failed to get apb clock: %d\n", > > + ret); > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, pc); > > + pm_runtime_enable(dev); > > + > > + for_each_available_child_of_node(np, child) { > > + ret = of_property_read_u32(child, "reg", &i); > > + if (ret || i > 1) { > > + ret = -EINVAL; > > + DRM_DEV_ERROR(dev, > > + "invalid channel(%u) node address\n", i); > > + goto free_child; > > + } > > + > > + ch = &pc->ch[i]; > > + ch->pc = pc; > > + ch->stream_id = i; > > + > > + remote = of_graph_get_remote_node(child, 1, 0); > > + if (!remote) { > > + ret = -ENODEV; > > + DRM_DEV_ERROR(dev, > > + "channel%u failed to get port1's remote node: %d\n", > > + i, ret); > > + goto free_child; > > + } > > + > > + ch->next_bridge = of_drm_find_bridge(remote); > > + if (!ch->next_bridge) { > > + of_node_put(remote); > > + ret = -EPROBE_DEFER; > > + DRM_DEV_DEBUG_DRIVER(dev, > > + "channel%u failed to find next bridge: %d\n", > > + i, ret); > > + goto free_child; > > + } > > + > > + of_node_put(remote); > > + > > + ch->bridge.driver_private = ch; > > + ch->bridge.funcs = &imx8qxp_pc_bridge_funcs; > > + ch->bridge.of_node = child; > > + ch->is_available = true; > > + > > + drm_bridge_add(&ch->bridge); > > + } > > + > > + return 0; > > + > > +free_child: > > + of_node_put(child); > > + > > + if (i == 1 && pc->ch[0].next_bridge) > > + drm_bridge_remove(&pc->ch[0].bridge); > > + > > + pm_runtime_disable(dev); > > + return ret; > > +} > > + > > +static int imx8qxp_pc_bridge_remove(struct platform_device *pdev) > > +{ > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > + struct imx8qxp_pc_channel *ch; > > + int i; > > + > > + for (i = 0; i < 2; i++) { > > + ch = &pc->ch[i]; > > + > > + if (!ch->is_available) > > + continue; > > + > > + drm_bridge_remove(&ch->bridge); > > + ch->is_available = false; > > + } > > + > > + pm_runtime_disable(&pdev->dev); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused imx8qxp_pc_runtime_suspend(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > + int ret; > > + > > + ret = clk_prepare_enable(pc->clk_apb); > > + if (ret) > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > + __func__, ret); > > + > > + /* Disable pixel combiner by full reset. */ > > + imx8qxp_pc_write_clr(pc, PC_SW_RESET_REG, PC_FULL_RESET_N); > > + > > + clk_disable_unprepare(pc->clk_apb); > > + > > + /* Ensure the reset takes effect. */ > > + usleep_range(10, 20); > > + > > + return ret; > > +} > > + > > +static int __maybe_unused imx8qxp_pc_runtime_resume(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > + int ret; > > + > > + ret = clk_prepare_enable(pc->clk_apb); > > + if (ret) { > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > + __func__, ret); > > + return ret; > > + } > > + > > + /* out of reset */ > > + imx8qxp_pc_write_set(pc, PC_SW_RESET_REG, PC_FULL_RESET_N); > > + > > + clk_disable_unprepare(pc->clk_apb); > > + > > + return ret; > > +} > > + > > +static const struct dev_pm_ops imx8qxp_pc_pm_ops = { > > + SET_RUNTIME_PM_OPS(imx8qxp_pc_runtime_suspend, > > + imx8qxp_pc_runtime_resume, NULL) > > +}; > > + > > +static const struct of_device_id imx8qxp_pc_dt_ids[] = { > > + { .compatible = "fsl,imx8qm-pixel-combiner", }, > > + { .compatible = "fsl,imx8qxp-pixel-combiner", }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, imx8qxp_pc_dt_ids); > > + > > +static struct platform_driver imx8qxp_pc_bridge_driver = { > > + .probe = imx8qxp_pc_bridge_probe, > > + .remove = imx8qxp_pc_bridge_remove, > > + .driver = { > > + .pm = &imx8qxp_pc_pm_ops, > > + .name = DRIVER_NAME, > > + .of_match_table = imx8qxp_pc_dt_ids, > > + }, > > +}; > > +module_platform_driver(imx8qxp_pc_bridge_driver); > > + > > +MODULE_DESCRIPTION("i.MX8QM/QXP pixel combiner bridge driver"); > > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_ALIAS("platform:" DRIVER_NAME); > > -- > > 2.7.4 > >
On Mon, 1 Mar 2021 at 10:07, Liu Ying <victor.liu@nxp.com> wrote: > > Hi Robert, > > On Fri, 2021-02-26 at 14:07 +0100, Robert Foss wrote: > > Hey Liu, > > > > With the below nit straightened out, feel free to add my r-b. > > > > Reviewed-by: Robert Foss <robert.foss@linaro.org> > > Thanks for reviewing this patch. > > > > > On Thu, 18 Feb 2021 at 04:58, Liu Ying <victor.liu@nxp.com> wrote: > > > This patch adds a drm bridge driver for i.MX8qm/qxp pixel combiner. > > > The pixel combiner takes two output streams from a single display > > > controller and manipulates the two streams to support a number > > > of modes(bypass, pixel combine, YUV444 to YUV422, split_RGB) configured > > > as either one screen, two screens, or virtual screens. The pixel > > > combiner is also responsible for generating some of the control signals > > > for the pixel link output channel. For now, the driver only supports > > > the bypass mode. > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > --- > > > v3->v4: > > > * No change. > > > > > > v2->v3: > > > * No change. > > > > > > v1->v2: > > > * No change. > > > > > > drivers/gpu/drm/bridge/Kconfig | 2 + > > > drivers/gpu/drm/bridge/Makefile | 1 + > > > drivers/gpu/drm/bridge/imx/Kconfig | 8 + > > > drivers/gpu/drm/bridge/imx/Makefile | 1 + > > > .../gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 452 +++++++++++++++++++++ > > > 5 files changed, 464 insertions(+) > > > create mode 100644 drivers/gpu/drm/bridge/imx/Kconfig > > > create mode 100644 drivers/gpu/drm/bridge/imx/Makefile > > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > > > index e4110d6c..84944e0 100644 > > > --- a/drivers/gpu/drm/bridge/Kconfig > > > +++ b/drivers/gpu/drm/bridge/Kconfig > > > @@ -256,6 +256,8 @@ source "drivers/gpu/drm/bridge/adv7511/Kconfig" > > > > > > source "drivers/gpu/drm/bridge/cadence/Kconfig" > > > > > > +source "drivers/gpu/drm/bridge/imx/Kconfig" > > > + > > > source "drivers/gpu/drm/bridge/synopsys/Kconfig" > > > > > > endmenu > > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > > > index 86e7acc..bc80cae 100644 > > > --- a/drivers/gpu/drm/bridge/Makefile > > > +++ b/drivers/gpu/drm/bridge/Makefile > > > @@ -27,4 +27,5 @@ obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o > > > > > > obj-y += analogix/ > > > obj-y += cadence/ > > > +obj-y += imx/ > > > obj-y += synopsys/ > > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig > > > new file mode 100644 > > > index 00000000..f1c91b6 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > > > @@ -0,0 +1,8 @@ > > > +config DRM_IMX8QXP_PIXEL_COMBINER > > > + tristate "Freescale i.MX8QM/QXP pixel combiner" > > > + depends on OF > > > + depends on COMMON_CLK > > > + select DRM_KMS_HELPER > > > + help > > > + Choose this to enable pixel combiner found in > > > + Freescale i.MX8qm/qxp processors. > > > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile > > > new file mode 100644 > > > index 00000000..7d7c8d6 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/bridge/imx/Makefile > > > @@ -0,0 +1 @@ > > > +obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o > > > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > > new file mode 100644 > > > index 00000000..cd5b1be > > > --- /dev/null > > > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > > @@ -0,0 +1,452 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > + > > > +/* > > > + * Copyright 2020 NXP > > > + */ > > > + > > > +#include <linux/bitfield.h> > > > +#include <linux/clk.h> > > > +#include <linux/delay.h> > > > +#include <linux/module.h> > > > +#include <linux/of.h> > > > +#include <linux/of_graph.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/pm_runtime.h> > > > + > > > +#include <drm/drm_atomic_state_helper.h> > > > +#include <drm/drm_bridge.h> > > > +#include <drm/drm_print.h> > > > + > > > +#define PC_CTRL_REG 0x0 > > > +#define PC_COMBINE_ENABLE BIT(0) > > > +#define PC_DISP_BYPASS(n) BIT(1 + 21 * (n)) > > > +#define PC_DISP_HSYNC_POLARITY(n) BIT(2 + 11 * (n)) > > > +#define PC_DISP_HSYNC_POLARITY_POS(n) DISP_HSYNC_POLARITY(n) > > > +#define PC_DISP_VSYNC_POLARITY(n) BIT(3 + 11 * (n)) > > > +#define PC_DISP_VSYNC_POLARITY_POS(n) DISP_VSYNC_POLARITY(n) > > > +#define PC_DISP_DVALID_POLARITY(n) BIT(4 + 11 * (n)) > > > +#define PC_DISP_DVALID_POLARITY_POS(n) DISP_DVALID_POLARITY(n) > > > +#define PC_VSYNC_MASK_ENABLE BIT(5) > > > +#define PC_SKIP_MODE BIT(6) > > > +#define PC_SKIP_NUMBER_MASK GENMASK(12, 7) > > > +#define PC_SKIP_NUMBER(n) FIELD_PREP(PC_SKIP_NUMBER_MASK, (n)) > > > +#define PC_DISP0_PIX_DATA_FORMAT_MASK GENMASK(18, 16) > > > +#define PC_DISP0_PIX_DATA_FORMAT(fmt) \ > > > + FIELD_PREP(PC_DISP0_PIX_DATA_FORMAT_MASK, (fmt)) > > > +#define PC_DISP1_PIX_DATA_FORMAT_MASK GENMASK(21, 19) > > > +#define PC_DISP1_PIX_DATA_FORMAT(fmt) \ > > > + FIELD_PREP(PC_DISP1_PIX_DATA_FORMAT_MASK, (fmt)) > > > + > > > +#define PC_BUF_PARA_REG 0x10 > > > > This register is unused, keeping it in here to avoid future headaches > > seems like a good idea. > > Yes, for now, this register is unused. > It will be used to set the below PC_BUF_ACTIVE_DEPTH field when > non-bypass modes are enabled, I think. > > Though you said 'keeping it', you actually want me to drop it for now, > right? If there is a 2nd series coming which will enable non-bypass modes, then maybe add it in that series instead. > > > > > > +#define PC_BUF_ACTIVE_DEPTH_MASK GENMASK(10, 0) > > > +#define PC_BUF_ACTIVE_DEPTH(n) FIELD_PREP(PC_BUF_ACTIVE_DEPTH_MASK, (n)) > > > + > > > +#define PC_SW_RESET_REG 0x20 > > > +#define PC_SW_RESET_N BIT(0) > > > +#define PC_DISP_SW_RESET_N(n) BIT(1 + (n)) > > > +#define PC_FULL_RESET_N (PC_SW_RESET_N | \ > > > + PC_DISP_SW_RESET_N(0) | \ > > > + PC_DISP_SW_RESET_N(1)) > > > + > > > +#define PC_REG_SET 0x4 > > > +#define PC_REG_CLR 0x8 > > > + > > > +#define DRIVER_NAME "imx8qxp-pixel-combiner" > > > + > > > +enum imx8qxp_pc_pix_data_format { > > > + RGB, > > > + YUV444, > > > + YUV422, > > > + SPLIT_RGB, > > > > YUV444, YUV422 & SPLIT_RGB are also unused, but if their values are > > compatible with the PC_DISP0_PIX_DATA_FORMAT macro I think keeping > > them around for future reference is a good idea. > > Yes, YUV444, YUV422 & SPLIT_RGB are compatible with the > PC_DISP{0, 1}_PIX_DATA_FORMAT macros, so I'll keep them here. > > Regards, > Liu Ying > > > > > > +}; > > > + > > > +struct imx8qxp_pc_channel { > > > + struct drm_bridge bridge; > > > + struct drm_bridge *next_bridge; > > > + struct imx8qxp_pc *pc; > > > + unsigned int stream_id; > > > + bool is_available; > > > +}; > > > + > > > +struct imx8qxp_pc { > > > + struct device *dev; > > > + struct imx8qxp_pc_channel ch[2]; > > > + struct clk *clk_apb; > > > + void __iomem *base; > > > +}; > > > + > > > +static inline u32 imx8qxp_pc_read(struct imx8qxp_pc *pc, unsigned int offset) > > > +{ > > > + return readl(pc->base + offset); > > > +} > > > + > > > +static inline void > > > +imx8qxp_pc_write(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > > +{ > > > + writel(value, pc->base + offset); > > > +} > > > + > > > +static inline void > > > +imx8qxp_pc_write_set(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > > +{ > > > + imx8qxp_pc_write(pc, offset + PC_REG_SET, value); > > > +} > > > + > > > +static inline void > > > +imx8qxp_pc_write_clr(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > > +{ > > > + imx8qxp_pc_write(pc, offset + PC_REG_CLR, value); > > > +} > > > + > > > +static enum drm_mode_status > > > +imx8qxp_pc_bridge_mode_valid(struct drm_bridge *bridge, > > > + const struct drm_display_info *info, > > > + const struct drm_display_mode *mode) > > > +{ > > > + if (mode->hdisplay > 2560) > > > + return MODE_BAD_HVALUE; > > > + > > > + return MODE_OK; > > > +} > > > + > > > +static int imx8qxp_pc_bridge_attach(struct drm_bridge *bridge, > > > + enum drm_bridge_attach_flags flags) > > > +{ > > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > > + struct imx8qxp_pc *pc = ch->pc; > > > + > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > > + DRM_DEV_ERROR(pc->dev, > > > + "do not support creating a drm_connector\n"); > > > + return -EINVAL; > > > + } > > > + > > > + if (!bridge->encoder) { > > > + DRM_DEV_ERROR(pc->dev, "missing encoder\n"); > > > + return -ENODEV; > > > + } > > > + > > > + return drm_bridge_attach(bridge->encoder, > > > + ch->next_bridge, bridge, > > > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > > +} > > > + > > > +static void > > > +imx8qxp_pc_bridge_mode_set(struct drm_bridge *bridge, > > > + const struct drm_display_mode *mode, > > > + const struct drm_display_mode *adjusted_mode) > > > +{ > > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > > + struct imx8qxp_pc *pc = ch->pc; > > > + u32 val; > > > + int ret; > > > + > > > + ret = pm_runtime_get_sync(pc->dev); > > > + if (ret < 0) > > > + DRM_DEV_ERROR(pc->dev, > > > + "failed to get runtime PM sync: %d\n", ret); > > > + > > > + ret = clk_prepare_enable(pc->clk_apb); > > > + if (ret) > > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > > + __func__, ret); > > > + > > > + /* HSYNC to pixel link is active low. */ > > > + imx8qxp_pc_write_clr(pc, PC_CTRL_REG, > > > + PC_DISP_HSYNC_POLARITY(ch->stream_id)); > > > + > > > + /* VSYNC to pixel link is active low. */ > > > + imx8qxp_pc_write_clr(pc, PC_CTRL_REG, > > > + PC_DISP_VSYNC_POLARITY(ch->stream_id)); > > > + > > > + /* Data enable to pixel link is active high. */ > > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, > > > + PC_DISP_DVALID_POLARITY(ch->stream_id)); > > > + > > > + /* Mask the first frame output which may be incomplete. */ > > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, PC_VSYNC_MASK_ENABLE); > > > + > > > + /* Only support RGB currently. */ > > > + val = imx8qxp_pc_read(pc, PC_CTRL_REG); > > > + if (ch->stream_id == 0) { > > > + val &= ~PC_DISP0_PIX_DATA_FORMAT_MASK; > > > + val |= PC_DISP0_PIX_DATA_FORMAT(RGB); > > > + } else { > > > + val &= ~PC_DISP1_PIX_DATA_FORMAT_MASK; > > > + val |= PC_DISP1_PIX_DATA_FORMAT(RGB); > > > + } > > > + imx8qxp_pc_write(pc, PC_CTRL_REG, val); > > > + > > > + /* Only support bypass mode currently. */ > > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, PC_DISP_BYPASS(ch->stream_id)); > > > + > > > + clk_disable_unprepare(pc->clk_apb); > > > +} > > > + > > > +static void imx8qxp_pc_bridge_atomic_disable(struct drm_bridge *bridge, > > > + struct drm_bridge_state *old_bridge_state) > > > +{ > > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > > + struct imx8qxp_pc *pc = ch->pc; > > > + int ret; > > > + > > > + ret = pm_runtime_put(pc->dev); > > > + if (ret < 0) > > > + DRM_DEV_ERROR(pc->dev, "failed to put runtime PM: %d\n", ret); > > > +} > > > + > > > +static const u32 imx8qxp_pc_bus_output_fmts[] = { > > > + MEDIA_BUS_FMT_RGB888_1X36_CPADLO, > > > + MEDIA_BUS_FMT_RGB666_1X36_CPADLO, > > > +}; > > > + > > > +static bool imx8qxp_pc_bus_output_fmt_supported(u32 fmt) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(imx8qxp_pc_bus_output_fmts); i++) { > > > + if (imx8qxp_pc_bus_output_fmts[i] == fmt) > > > + return true; > > > + } > > > + > > > + return false; > > > +} > > > + > > > +static u32 * > > > +imx8qxp_pc_bridge_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) > > > +{ > > > + u32 *input_fmts; > > > + > > > + if (!imx8qxp_pc_bus_output_fmt_supported(output_fmt)) > > > + return NULL; > > > + > > > + *num_input_fmts = 1; > > > + > > > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > > > + if (!input_fmts) > > > + return NULL; > > > + > > > + switch (output_fmt) { > > > + case MEDIA_BUS_FMT_RGB888_1X36_CPADLO: > > > + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X30_CPADLO; > > > + break; > > > + case MEDIA_BUS_FMT_RGB666_1X36_CPADLO: > > > + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X30_CPADLO; > > > + break; > > > + default: > > > + kfree(input_fmts); > > > + input_fmts = NULL; > > > + break; > > > + } > > > + > > > + return input_fmts; > > > +} > > > + > > > +static u32 * > > > +imx8qxp_pc_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > > > + struct drm_bridge_state *bridge_state, > > > + struct drm_crtc_state *crtc_state, > > > + struct drm_connector_state *conn_state, > > > + unsigned int *num_output_fmts) > > > +{ > > > + *num_output_fmts = ARRAY_SIZE(imx8qxp_pc_bus_output_fmts); > > > + return kmemdup(imx8qxp_pc_bus_output_fmts, > > > + sizeof(imx8qxp_pc_bus_output_fmts), GFP_KERNEL); > > > +} > > > + > > > +static const struct drm_bridge_funcs imx8qxp_pc_bridge_funcs = { > > > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > > + .atomic_reset = drm_atomic_helper_bridge_reset, > > > + .mode_valid = imx8qxp_pc_bridge_mode_valid, > > > + .attach = imx8qxp_pc_bridge_attach, > > > + .mode_set = imx8qxp_pc_bridge_mode_set, > > > + .atomic_disable = imx8qxp_pc_bridge_atomic_disable, > > > + .atomic_get_input_bus_fmts = > > > + imx8qxp_pc_bridge_atomic_get_input_bus_fmts, > > > + .atomic_get_output_bus_fmts = > > > + imx8qxp_pc_bridge_atomic_get_output_bus_fmts, > > > +}; > > > + > > > +static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) > > > +{ > > > + struct imx8qxp_pc *pc; > > > + struct imx8qxp_pc_channel *ch; > > > + struct device *dev = &pdev->dev; > > > + struct device_node *np = dev->of_node; > > > + struct device_node *child, *remote; > > > + u32 i; > > > + int ret; > > > + > > > + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL); > > > + if (!pc) > > > + return -ENOMEM; > > > + > > > + pc->base = devm_platform_ioremap_resource(pdev, 0); > > > + if (IS_ERR(pc->base)) > > > + return PTR_ERR(pc->base); > > > + > > > + pc->dev = dev; > > > + > > > + pc->clk_apb = devm_clk_get(dev, "apb"); > > > + if (IS_ERR(pc->clk_apb)) { > > > + ret = PTR_ERR(pc->clk_apb); > > > + if (ret != -EPROBE_DEFER) > > > + DRM_DEV_ERROR(dev, "failed to get apb clock: %d\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > + platform_set_drvdata(pdev, pc); > > > + pm_runtime_enable(dev); > > > + > > > + for_each_available_child_of_node(np, child) { > > > + ret = of_property_read_u32(child, "reg", &i); > > > + if (ret || i > 1) { > > > + ret = -EINVAL; > > > + DRM_DEV_ERROR(dev, > > > + "invalid channel(%u) node address\n", i); > > > + goto free_child; > > > + } > > > + > > > + ch = &pc->ch[i]; > > > + ch->pc = pc; > > > + ch->stream_id = i; > > > + > > > + remote = of_graph_get_remote_node(child, 1, 0); > > > + if (!remote) { > > > + ret = -ENODEV; > > > + DRM_DEV_ERROR(dev, > > > + "channel%u failed to get port1's remote node: %d\n", > > > + i, ret); > > > + goto free_child; > > > + } > > > + > > > + ch->next_bridge = of_drm_find_bridge(remote); > > > + if (!ch->next_bridge) { > > > + of_node_put(remote); > > > + ret = -EPROBE_DEFER; > > > + DRM_DEV_DEBUG_DRIVER(dev, > > > + "channel%u failed to find next bridge: %d\n", > > > + i, ret); > > > + goto free_child; > > > + } > > > + > > > + of_node_put(remote); > > > + > > > + ch->bridge.driver_private = ch; > > > + ch->bridge.funcs = &imx8qxp_pc_bridge_funcs; > > > + ch->bridge.of_node = child; > > > + ch->is_available = true; > > > + > > > + drm_bridge_add(&ch->bridge); > > > + } > > > + > > > + return 0; > > > + > > > +free_child: > > > + of_node_put(child); > > > + > > > + if (i == 1 && pc->ch[0].next_bridge) > > > + drm_bridge_remove(&pc->ch[0].bridge); > > > + > > > + pm_runtime_disable(dev); > > > + return ret; > > > +} > > > + > > > +static int imx8qxp_pc_bridge_remove(struct platform_device *pdev) > > > +{ > > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > > + struct imx8qxp_pc_channel *ch; > > > + int i; > > > + > > > + for (i = 0; i < 2; i++) { > > > + ch = &pc->ch[i]; > > > + > > > + if (!ch->is_available) > > > + continue; > > > + > > > + drm_bridge_remove(&ch->bridge); > > > + ch->is_available = false; > > > + } > > > + > > > + pm_runtime_disable(&pdev->dev); > > > + > > > + return 0; > > > +} > > > + > > > +static int __maybe_unused imx8qxp_pc_runtime_suspend(struct device *dev) > > > +{ > > > + struct platform_device *pdev = to_platform_device(dev); > > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > > + int ret; > > > + > > > + ret = clk_prepare_enable(pc->clk_apb); > > > + if (ret) > > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > > + __func__, ret); > > > + > > > + /* Disable pixel combiner by full reset. */ > > > + imx8qxp_pc_write_clr(pc, PC_SW_RESET_REG, PC_FULL_RESET_N); > > > + > > > + clk_disable_unprepare(pc->clk_apb); > > > + > > > + /* Ensure the reset takes effect. */ > > > + usleep_range(10, 20); > > > + > > > + return ret; > > > +} > > > + > > > +static int __maybe_unused imx8qxp_pc_runtime_resume(struct device *dev) > > > +{ > > > + struct platform_device *pdev = to_platform_device(dev); > > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > > + int ret; > > > + > > > + ret = clk_prepare_enable(pc->clk_apb); > > > + if (ret) { > > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > > + __func__, ret); > > > + return ret; > > > + } > > > + > > > + /* out of reset */ > > > + imx8qxp_pc_write_set(pc, PC_SW_RESET_REG, PC_FULL_RESET_N); > > > + > > > + clk_disable_unprepare(pc->clk_apb); > > > + > > > + return ret; > > > +} > > > + > > > +static const struct dev_pm_ops imx8qxp_pc_pm_ops = { > > > + SET_RUNTIME_PM_OPS(imx8qxp_pc_runtime_suspend, > > > + imx8qxp_pc_runtime_resume, NULL) > > > +}; > > > + > > > +static const struct of_device_id imx8qxp_pc_dt_ids[] = { > > > + { .compatible = "fsl,imx8qm-pixel-combiner", }, > > > + { .compatible = "fsl,imx8qxp-pixel-combiner", }, > > > + { /* sentinel */ } > > > +}; > > > +MODULE_DEVICE_TABLE(of, imx8qxp_pc_dt_ids); > > > + > > > +static struct platform_driver imx8qxp_pc_bridge_driver = { > > > + .probe = imx8qxp_pc_bridge_probe, > > > + .remove = imx8qxp_pc_bridge_remove, > > > + .driver = { > > > + .pm = &imx8qxp_pc_pm_ops, > > > + .name = DRIVER_NAME, > > > + .of_match_table = imx8qxp_pc_dt_ids, > > > + }, > > > +}; > > > +module_platform_driver(imx8qxp_pc_bridge_driver); > > > + > > > +MODULE_DESCRIPTION("i.MX8QM/QXP pixel combiner bridge driver"); > > > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); > > > +MODULE_LICENSE("GPL v2"); > > > +MODULE_ALIAS("platform:" DRIVER_NAME); > > > -- > > > 2.7.4 > > > >
On Mon, 2021-03-01 at 11:56 +0100, Robert Foss wrote: > On Mon, 1 Mar 2021 at 10:07, Liu Ying <victor.liu@nxp.com> wrote: > > Hi Robert, > > > > On Fri, 2021-02-26 at 14:07 +0100, Robert Foss wrote: > > > Hey Liu, > > > > > > With the below nit straightened out, feel free to add my r-b. > > > > > > Reviewed-by: Robert Foss <robert.foss@linaro.org> > > > > Thanks for reviewing this patch. > > > > > On Thu, 18 Feb 2021 at 04:58, Liu Ying <victor.liu@nxp.com> wrote: > > > > This patch adds a drm bridge driver for i.MX8qm/qxp pixel combiner. > > > > The pixel combiner takes two output streams from a single display > > > > controller and manipulates the two streams to support a number > > > > of modes(bypass, pixel combine, YUV444 to YUV422, split_RGB) configured > > > > as either one screen, two screens, or virtual screens. The pixel > > > > combiner is also responsible for generating some of the control signals > > > > for the pixel link output channel. For now, the driver only supports > > > > the bypass mode. > > > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > --- > > > > v3->v4: > > > > * No change. > > > > > > > > v2->v3: > > > > * No change. > > > > > > > > v1->v2: > > > > * No change. > > > > > > > > drivers/gpu/drm/bridge/Kconfig | 2 + > > > > drivers/gpu/drm/bridge/Makefile | 1 + > > > > drivers/gpu/drm/bridge/imx/Kconfig | 8 + > > > > drivers/gpu/drm/bridge/imx/Makefile | 1 + > > > > .../gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 452 +++++++++++++++++++++ > > > > 5 files changed, 464 insertions(+) > > > > create mode 100644 drivers/gpu/drm/bridge/imx/Kconfig > > > > create mode 100644 drivers/gpu/drm/bridge/imx/Makefile > > > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > > > > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > > > > index e4110d6c..84944e0 100644 > > > > --- a/drivers/gpu/drm/bridge/Kconfig > > > > +++ b/drivers/gpu/drm/bridge/Kconfig > > > > @@ -256,6 +256,8 @@ source "drivers/gpu/drm/bridge/adv7511/Kconfig" > > > > > > > > source "drivers/gpu/drm/bridge/cadence/Kconfig" > > > > > > > > +source "drivers/gpu/drm/bridge/imx/Kconfig" > > > > + > > > > source "drivers/gpu/drm/bridge/synopsys/Kconfig" > > > > > > > > endmenu > > > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > > > > index 86e7acc..bc80cae 100644 > > > > --- a/drivers/gpu/drm/bridge/Makefile > > > > +++ b/drivers/gpu/drm/bridge/Makefile > > > > @@ -27,4 +27,5 @@ obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o > > > > > > > > obj-y += analogix/ > > > > obj-y += cadence/ > > > > +obj-y += imx/ > > > > obj-y += synopsys/ > > > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig > > > > new file mode 100644 > > > > index 00000000..f1c91b6 > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > > > > @@ -0,0 +1,8 @@ > > > > +config DRM_IMX8QXP_PIXEL_COMBINER > > > > + tristate "Freescale i.MX8QM/QXP pixel combiner" > > > > + depends on OF > > > > + depends on COMMON_CLK > > > > + select DRM_KMS_HELPER > > > > + help > > > > + Choose this to enable pixel combiner found in > > > > + Freescale i.MX8qm/qxp processors. > > > > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile > > > > new file mode 100644 > > > > index 00000000..7d7c8d6 > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/bridge/imx/Makefile > > > > @@ -0,0 +1 @@ > > > > +obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o > > > > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > > > new file mode 100644 > > > > index 00000000..cd5b1be > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c > > > > @@ -0,0 +1,452 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > + > > > > +/* > > > > + * Copyright 2020 NXP > > > > + */ > > > > + > > > > +#include <linux/bitfield.h> > > > > +#include <linux/clk.h> > > > > +#include <linux/delay.h> > > > > +#include <linux/module.h> > > > > +#include <linux/of.h> > > > > +#include <linux/of_graph.h> > > > > +#include <linux/platform_device.h> > > > > +#include <linux/pm_runtime.h> > > > > + > > > > +#include <drm/drm_atomic_state_helper.h> > > > > +#include <drm/drm_bridge.h> > > > > +#include <drm/drm_print.h> > > > > + > > > > +#define PC_CTRL_REG 0x0 > > > > +#define PC_COMBINE_ENABLE BIT(0) > > > > +#define PC_DISP_BYPASS(n) BIT(1 + 21 * (n)) > > > > +#define PC_DISP_HSYNC_POLARITY(n) BIT(2 + 11 * (n)) > > > > +#define PC_DISP_HSYNC_POLARITY_POS(n) DISP_HSYNC_POLARITY(n) > > > > +#define PC_DISP_VSYNC_POLARITY(n) BIT(3 + 11 * (n)) > > > > +#define PC_DISP_VSYNC_POLARITY_POS(n) DISP_VSYNC_POLARITY(n) > > > > +#define PC_DISP_DVALID_POLARITY(n) BIT(4 + 11 * (n)) > > > > +#define PC_DISP_DVALID_POLARITY_POS(n) DISP_DVALID_POLARITY(n) > > > > +#define PC_VSYNC_MASK_ENABLE BIT(5) > > > > +#define PC_SKIP_MODE BIT(6) > > > > +#define PC_SKIP_NUMBER_MASK GENMASK(12, 7) > > > > +#define PC_SKIP_NUMBER(n) FIELD_PREP(PC_SKIP_NUMBER_MASK, (n)) > > > > +#define PC_DISP0_PIX_DATA_FORMAT_MASK GENMASK(18, 16) > > > > +#define PC_DISP0_PIX_DATA_FORMAT(fmt) \ > > > > + FIELD_PREP(PC_DISP0_PIX_DATA_FORMAT_MASK, (fmt)) > > > > +#define PC_DISP1_PIX_DATA_FORMAT_MASK GENMASK(21, 19) > > > > +#define PC_DISP1_PIX_DATA_FORMAT(fmt) \ > > > > + FIELD_PREP(PC_DISP1_PIX_DATA_FORMAT_MASK, (fmt)) > > > > + > > > > +#define PC_BUF_PARA_REG 0x10 > > > > > > This register is unused, keeping it in here to avoid future headaches > > > seems like a good idea. > > > > Yes, for now, this register is unused. > > It will be used to set the below PC_BUF_ACTIVE_DEPTH field when > > non-bypass modes are enabled, I think. > > > > Though you said 'keeping it', you actually want me to drop it for now, > > right? > > If there is a 2nd series coming which will enable non-bypass modes, > then maybe add it in that series instead. Ok, I'll drop PC_BUF_PARA_REG for now in the next version. Thanks, Liu Ying > > > > > +#define PC_BUF_ACTIVE_DEPTH_MASK GENMASK(10, 0) > > > > +#define PC_BUF_ACTIVE_DEPTH(n) FIELD_PREP(PC_BUF_ACTIVE_DEPTH_MASK, (n)) > > > > + > > > > +#define PC_SW_RESET_REG 0x20 > > > > +#define PC_SW_RESET_N BIT(0) > > > > +#define PC_DISP_SW_RESET_N(n) BIT(1 + (n)) > > > > +#define PC_FULL_RESET_N (PC_SW_RESET_N | \ > > > > + PC_DISP_SW_RESET_N(0) | \ > > > > + PC_DISP_SW_RESET_N(1)) > > > > + > > > > +#define PC_REG_SET 0x4 > > > > +#define PC_REG_CLR 0x8 > > > > + > > > > +#define DRIVER_NAME "imx8qxp-pixel-combiner" > > > > + > > > > +enum imx8qxp_pc_pix_data_format { > > > > + RGB, > > > > + YUV444, > > > > + YUV422, > > > > + SPLIT_RGB, > > > > > > YUV444, YUV422 & SPLIT_RGB are also unused, but if their values are > > > compatible with the PC_DISP0_PIX_DATA_FORMAT macro I think keeping > > > them around for future reference is a good idea. > > > > Yes, YUV444, YUV422 & SPLIT_RGB are compatible with the > > PC_DISP{0, 1}_PIX_DATA_FORMAT macros, so I'll keep them here. > > > > Regards, > > Liu Ying > > > > > > +}; > > > > + > > > > +struct imx8qxp_pc_channel { > > > > + struct drm_bridge bridge; > > > > + struct drm_bridge *next_bridge; > > > > + struct imx8qxp_pc *pc; > > > > + unsigned int stream_id; > > > > + bool is_available; > > > > +}; > > > > + > > > > +struct imx8qxp_pc { > > > > + struct device *dev; > > > > + struct imx8qxp_pc_channel ch[2]; > > > > + struct clk *clk_apb; > > > > + void __iomem *base; > > > > +}; > > > > + > > > > +static inline u32 imx8qxp_pc_read(struct imx8qxp_pc *pc, unsigned int offset) > > > > +{ > > > > + return readl(pc->base + offset); > > > > +} > > > > + > > > > +static inline void > > > > +imx8qxp_pc_write(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > > > +{ > > > > + writel(value, pc->base + offset); > > > > +} > > > > + > > > > +static inline void > > > > +imx8qxp_pc_write_set(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > > > +{ > > > > + imx8qxp_pc_write(pc, offset + PC_REG_SET, value); > > > > +} > > > > + > > > > +static inline void > > > > +imx8qxp_pc_write_clr(struct imx8qxp_pc *pc, unsigned int offset, u32 value) > > > > +{ > > > > + imx8qxp_pc_write(pc, offset + PC_REG_CLR, value); > > > > +} > > > > + > > > > +static enum drm_mode_status > > > > +imx8qxp_pc_bridge_mode_valid(struct drm_bridge *bridge, > > > > + const struct drm_display_info *info, > > > > + const struct drm_display_mode *mode) > > > > +{ > > > > + if (mode->hdisplay > 2560) > > > > + return MODE_BAD_HVALUE; > > > > + > > > > + return MODE_OK; > > > > +} > > > > + > > > > +static int imx8qxp_pc_bridge_attach(struct drm_bridge *bridge, > > > > + enum drm_bridge_attach_flags flags) > > > > +{ > > > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > > > + struct imx8qxp_pc *pc = ch->pc; > > > > + > > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > > > + DRM_DEV_ERROR(pc->dev, > > > > + "do not support creating a drm_connector\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (!bridge->encoder) { > > > > + DRM_DEV_ERROR(pc->dev, "missing encoder\n"); > > > > + return -ENODEV; > > > > + } > > > > + > > > > + return drm_bridge_attach(bridge->encoder, > > > > + ch->next_bridge, bridge, > > > > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > > > +} > > > > + > > > > +static void > > > > +imx8qxp_pc_bridge_mode_set(struct drm_bridge *bridge, > > > > + const struct drm_display_mode *mode, > > > > + const struct drm_display_mode *adjusted_mode) > > > > +{ > > > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > > > + struct imx8qxp_pc *pc = ch->pc; > > > > + u32 val; > > > > + int ret; > > > > + > > > > + ret = pm_runtime_get_sync(pc->dev); > > > > + if (ret < 0) > > > > + DRM_DEV_ERROR(pc->dev, > > > > + "failed to get runtime PM sync: %d\n", ret); > > > > + > > > > + ret = clk_prepare_enable(pc->clk_apb); > > > > + if (ret) > > > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > > > + __func__, ret); > > > > + > > > > + /* HSYNC to pixel link is active low. */ > > > > + imx8qxp_pc_write_clr(pc, PC_CTRL_REG, > > > > + PC_DISP_HSYNC_POLARITY(ch->stream_id)); > > > > + > > > > + /* VSYNC to pixel link is active low. */ > > > > + imx8qxp_pc_write_clr(pc, PC_CTRL_REG, > > > > + PC_DISP_VSYNC_POLARITY(ch->stream_id)); > > > > + > > > > + /* Data enable to pixel link is active high. */ > > > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, > > > > + PC_DISP_DVALID_POLARITY(ch->stream_id)); > > > > + > > > > + /* Mask the first frame output which may be incomplete. */ > > > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, PC_VSYNC_MASK_ENABLE); > > > > + > > > > + /* Only support RGB currently. */ > > > > + val = imx8qxp_pc_read(pc, PC_CTRL_REG); > > > > + if (ch->stream_id == 0) { > > > > + val &= ~PC_DISP0_PIX_DATA_FORMAT_MASK; > > > > + val |= PC_DISP0_PIX_DATA_FORMAT(RGB); > > > > + } else { > > > > + val &= ~PC_DISP1_PIX_DATA_FORMAT_MASK; > > > > + val |= PC_DISP1_PIX_DATA_FORMAT(RGB); > > > > + } > > > > + imx8qxp_pc_write(pc, PC_CTRL_REG, val); > > > > + > > > > + /* Only support bypass mode currently. */ > > > > + imx8qxp_pc_write_set(pc, PC_CTRL_REG, PC_DISP_BYPASS(ch->stream_id)); > > > > + > > > > + clk_disable_unprepare(pc->clk_apb); > > > > +} > > > > + > > > > +static void imx8qxp_pc_bridge_atomic_disable(struct drm_bridge *bridge, > > > > + struct drm_bridge_state *old_bridge_state) > > > > +{ > > > > + struct imx8qxp_pc_channel *ch = bridge->driver_private; > > > > + struct imx8qxp_pc *pc = ch->pc; > > > > + int ret; > > > > + > > > > + ret = pm_runtime_put(pc->dev); > > > > + if (ret < 0) > > > > + DRM_DEV_ERROR(pc->dev, "failed to put runtime PM: %d\n", ret); > > > > +} > > > > + > > > > +static const u32 imx8qxp_pc_bus_output_fmts[] = { > > > > + MEDIA_BUS_FMT_RGB888_1X36_CPADLO, > > > > + MEDIA_BUS_FMT_RGB666_1X36_CPADLO, > > > > +}; > > > > + > > > > +static bool imx8qxp_pc_bus_output_fmt_supported(u32 fmt) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(imx8qxp_pc_bus_output_fmts); i++) { > > > > + if (imx8qxp_pc_bus_output_fmts[i] == fmt) > > > > + return true; > > > > + } > > > > + > > > > + return false; > > > > +} > > > > + > > > > +static u32 * > > > > +imx8qxp_pc_bridge_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) > > > > +{ > > > > + u32 *input_fmts; > > > > + > > > > + if (!imx8qxp_pc_bus_output_fmt_supported(output_fmt)) > > > > + return NULL; > > > > + > > > > + *num_input_fmts = 1; > > > > + > > > > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > > > > + if (!input_fmts) > > > > + return NULL; > > > > + > > > > + switch (output_fmt) { > > > > + case MEDIA_BUS_FMT_RGB888_1X36_CPADLO: > > > > + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X30_CPADLO; > > > > + break; > > > > + case MEDIA_BUS_FMT_RGB666_1X36_CPADLO: > > > > + input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X30_CPADLO; > > > > + break; > > > > + default: > > > > + kfree(input_fmts); > > > > + input_fmts = NULL; > > > > + break; > > > > + } > > > > + > > > > + return input_fmts; > > > > +} > > > > + > > > > +static u32 * > > > > +imx8qxp_pc_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > > > > + struct drm_bridge_state *bridge_state, > > > > + struct drm_crtc_state *crtc_state, > > > > + struct drm_connector_state *conn_state, > > > > + unsigned int *num_output_fmts) > > > > +{ > > > > + *num_output_fmts = ARRAY_SIZE(imx8qxp_pc_bus_output_fmts); > > > > + return kmemdup(imx8qxp_pc_bus_output_fmts, > > > > + sizeof(imx8qxp_pc_bus_output_fmts), GFP_KERNEL); > > > > +} > > > > + > > > > +static const struct drm_bridge_funcs imx8qxp_pc_bridge_funcs = { > > > > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > > > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > > > + .atomic_reset = drm_atomic_helper_bridge_reset, > > > > + .mode_valid = imx8qxp_pc_bridge_mode_valid, > > > > + .attach = imx8qxp_pc_bridge_attach, > > > > + .mode_set = imx8qxp_pc_bridge_mode_set, > > > > + .atomic_disable = imx8qxp_pc_bridge_atomic_disable, > > > > + .atomic_get_input_bus_fmts = > > > > + imx8qxp_pc_bridge_atomic_get_input_bus_fmts, > > > > + .atomic_get_output_bus_fmts = > > > > + imx8qxp_pc_bridge_atomic_get_output_bus_fmts, > > > > +}; > > > > + > > > > +static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) > > > > +{ > > > > + struct imx8qxp_pc *pc; > > > > + struct imx8qxp_pc_channel *ch; > > > > + struct device *dev = &pdev->dev; > > > > + struct device_node *np = dev->of_node; > > > > + struct device_node *child, *remote; > > > > + u32 i; > > > > + int ret; > > > > + > > > > + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL); > > > > + if (!pc) > > > > + return -ENOMEM; > > > > + > > > > + pc->base = devm_platform_ioremap_resource(pdev, 0); > > > > + if (IS_ERR(pc->base)) > > > > + return PTR_ERR(pc->base); > > > > + > > > > + pc->dev = dev; > > > > + > > > > + pc->clk_apb = devm_clk_get(dev, "apb"); > > > > + if (IS_ERR(pc->clk_apb)) { > > > > + ret = PTR_ERR(pc->clk_apb); > > > > + if (ret != -EPROBE_DEFER) > > > > + DRM_DEV_ERROR(dev, "failed to get apb clock: %d\n", > > > > + ret); > > > > + return ret; > > > > + } > > > > + > > > > + platform_set_drvdata(pdev, pc); > > > > + pm_runtime_enable(dev); > > > > + > > > > + for_each_available_child_of_node(np, child) { > > > > + ret = of_property_read_u32(child, "reg", &i); > > > > + if (ret || i > 1) { > > > > + ret = -EINVAL; > > > > + DRM_DEV_ERROR(dev, > > > > + "invalid channel(%u) node address\n", i); > > > > + goto free_child; > > > > + } > > > > + > > > > + ch = &pc->ch[i]; > > > > + ch->pc = pc; > > > > + ch->stream_id = i; > > > > + > > > > + remote = of_graph_get_remote_node(child, 1, 0); > > > > + if (!remote) { > > > > + ret = -ENODEV; > > > > + DRM_DEV_ERROR(dev, > > > > + "channel%u failed to get port1's remote node: %d\n", > > > > + i, ret); > > > > + goto free_child; > > > > + } > > > > + > > > > + ch->next_bridge = of_drm_find_bridge(remote); > > > > + if (!ch->next_bridge) { > > > > + of_node_put(remote); > > > > + ret = -EPROBE_DEFER; > > > > + DRM_DEV_DEBUG_DRIVER(dev, > > > > + "channel%u failed to find next bridge: %d\n", > > > > + i, ret); > > > > + goto free_child; > > > > + } > > > > + > > > > + of_node_put(remote); > > > > + > > > > + ch->bridge.driver_private = ch; > > > > + ch->bridge.funcs = &imx8qxp_pc_bridge_funcs; > > > > + ch->bridge.of_node = child; > > > > + ch->is_available = true; > > > > + > > > > + drm_bridge_add(&ch->bridge); > > > > + } > > > > + > > > > + return 0; > > > > + > > > > +free_child: > > > > + of_node_put(child); > > > > + > > > > + if (i == 1 && pc->ch[0].next_bridge) > > > > + drm_bridge_remove(&pc->ch[0].bridge); > > > > + > > > > + pm_runtime_disable(dev); > > > > + return ret; > > > > +} > > > > + > > > > +static int imx8qxp_pc_bridge_remove(struct platform_device *pdev) > > > > +{ > > > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > > > + struct imx8qxp_pc_channel *ch; > > > > + int i; > > > > + > > > > + for (i = 0; i < 2; i++) { > > > > + ch = &pc->ch[i]; > > > > + > > > > + if (!ch->is_available) > > > > + continue; > > > > + > > > > + drm_bridge_remove(&ch->bridge); > > > > + ch->is_available = false; > > > > + } > > > > + > > > > + pm_runtime_disable(&pdev->dev); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int __maybe_unused imx8qxp_pc_runtime_suspend(struct device *dev) > > > > +{ > > > > + struct platform_device *pdev = to_platform_device(dev); > > > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(pc->clk_apb); > > > > + if (ret) > > > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > > > + __func__, ret); > > > > + > > > > + /* Disable pixel combiner by full reset. */ > > > > + imx8qxp_pc_write_clr(pc, PC_SW_RESET_REG, PC_FULL_RESET_N); > > > > + > > > > + clk_disable_unprepare(pc->clk_apb); > > > > + > > > > + /* Ensure the reset takes effect. */ > > > > + usleep_range(10, 20); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int __maybe_unused imx8qxp_pc_runtime_resume(struct device *dev) > > > > +{ > > > > + struct platform_device *pdev = to_platform_device(dev); > > > > + struct imx8qxp_pc *pc = platform_get_drvdata(pdev); > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(pc->clk_apb); > > > > + if (ret) { > > > > + DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n", > > > > + __func__, ret); > > > > + return ret; > > > > + } > > > > + > > > > + /* out of reset */ > > > > + imx8qxp_pc_write_set(pc, PC_SW_RESET_REG, PC_FULL_RESET_N); > > > > + > > > > + clk_disable_unprepare(pc->clk_apb); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static const struct dev_pm_ops imx8qxp_pc_pm_ops = { > > > > + SET_RUNTIME_PM_OPS(imx8qxp_pc_runtime_suspend, > > > > + imx8qxp_pc_runtime_resume, NULL) > > > > +}; > > > > + > > > > +static const struct of_device_id imx8qxp_pc_dt_ids[] = { > > > > + { .compatible = "fsl,imx8qm-pixel-combiner", }, > > > > + { .compatible = "fsl,imx8qxp-pixel-combiner", }, > > > > + { /* sentinel */ } > > > > +}; > > > > +MODULE_DEVICE_TABLE(of, imx8qxp_pc_dt_ids); > > > > + > > > > +static struct platform_driver imx8qxp_pc_bridge_driver = { > > > > + .probe = imx8qxp_pc_bridge_probe, > > > > + .remove = imx8qxp_pc_bridge_remove, > > > > + .driver = { > > > > + .pm = &imx8qxp_pc_pm_ops, > > > > + .name = DRIVER_NAME, > > > > + .of_match_table = imx8qxp_pc_dt_ids, > > > > + }, > > > > +}; > > > > +module_platform_driver(imx8qxp_pc_bridge_driver); > > > > + > > > > +MODULE_DESCRIPTION("i.MX8QM/QXP pixel combiner bridge driver"); > > > > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); > > > > +MODULE_LICENSE("GPL v2"); > > > > +MODULE_ALIAS("platform:" DRIVER_NAME); > > > > -- > > > > 2.7.4 > > > >
Hey Liu, Thanks for submitting this patch. I only have one comment below. With that addressed, feel free to add my r-b. Reviewed-by: Robert Foss <robert.foss@linaro.org> On Thu, 18 Feb 2021 at 04:59, Liu Ying <victor.liu@nxp.com> wrote: > > This patch adds a drm bridge driver for i.MX8qm/qxp display pixel link. > The pixel link forms a standard asynchronous linkage between > pixel sources(display controller or camera module) and pixel > consumers(imaging or displays). It consists of two distinct > functions, a pixel transfer function and a control interface. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v3->v4: > * No change. > > v2->v3: > * Drop two macros which help define functions and define them directly. > * Properly disable all pixel link controls to POR value by calling > imx8qxp_pixel_link_disable_all_controls() from > imx8qxp_pixel_link_bridge_probe(). > > v1->v2: > * Rewrite the function to find the next bridge by properly using OF APIs > and dropping unnecessary DT validation. (Rob) > > drivers/gpu/drm/bridge/imx/Kconfig | 8 + > drivers/gpu/drm/bridge/imx/Makefile | 1 + > drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 426 ++++++++++++++++++++++++ > 3 files changed, 435 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig > index f1c91b6..4d1f027 100644 > --- a/drivers/gpu/drm/bridge/imx/Kconfig > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > @@ -6,3 +6,11 @@ config DRM_IMX8QXP_PIXEL_COMBINER > help > Choose this to enable pixel combiner found in > Freescale i.MX8qm/qxp processors. > + > +config DRM_IMX8QXP_PIXEL_LINK > + tristate "Freescale i.MX8QM/QXP display pixel link" > + depends on OF > + select DRM_KMS_HELPER > + help > + Choose this to enable display pixel link found in > + Freescale i.MX8qm/qxp processors. > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile > index 7d7c8d6..c15469f 100644 > --- a/drivers/gpu/drm/bridge/imx/Makefile > +++ b/drivers/gpu/drm/bridge/imx/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o > +obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c > new file mode 100644 > index 00000000..2e5ba4a > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c > @@ -0,0 +1,426 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/* > + * Copyright 2020 NXP > + */ > + > +#include <linux/firmware/imx/svc/misc.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_graph.h> > +#include <linux/platform_device.h> > + > +#include <drm/drm_atomic_state_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_print.h> > + > +#include <dt-bindings/firmware/imx/rsrc.h> > + > +#define DRIVER_NAME "imx8qxp-display-pixel-link" > +#define PL_MAX_MST_ADDR 3 > +#define PL_MAX_NEXT_BRIDGES 2 > + > +struct imx8qxp_pixel_link { > + struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > + struct device *dev; > + struct imx_sc_ipc *ipc_handle; > + int id; > + int stream_id; > + int dc_id; > + u32 sink_rsc; > + u32 mst_addr; > + u8 mst_addr_ctrl; > + u8 mst_en_ctrl; > + u8 mst_vld_ctrl; > + u8 sync_ctrl; > +}; > + > +static void imx8qxp_pixel_link_enable_mst_en(struct imx8qxp_pixel_link *pl) > +{ > + int ret; > + > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > + pl->mst_en_ctrl, true); > + if (ret) > + DRM_DEV_ERROR(pl->dev, > + "failed to enable DC%d stream%d pixel link mst_en: %d\n", > + pl->dc_id, pl->stream_id, ret); > +} > + > +static void imx8qxp_pixel_link_enable_mst_vld(struct imx8qxp_pixel_link *pl) > +{ > + int ret; > + > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > + pl->mst_vld_ctrl, true); > + if (ret) > + DRM_DEV_ERROR(pl->dev, > + "failed to enable DC%d stream%d pixel link mst_vld: %d\n", > + pl->dc_id, pl->stream_id, ret); > +} > + > +static void imx8qxp_pixel_link_enable_sync(struct imx8qxp_pixel_link *pl) > +{ > + int ret; > + > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > + pl->sync_ctrl, true); > + if (ret) > + DRM_DEV_ERROR(pl->dev, > + "failed to enable DC%d stream%d pixel link sync: %d\n", > + pl->dc_id, pl->stream_id, ret); > +} > + > +static int imx8qxp_pixel_link_disable_mst_en(struct imx8qxp_pixel_link *pl) > +{ > + int ret; > + > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > + pl->mst_en_ctrl, false); > + if (ret) > + DRM_DEV_ERROR(pl->dev, > + "failed to disable DC%d stream%d pixel link mst_en: %d\n", > + pl->dc_id, pl->stream_id, ret); > + > + return ret; > +} > + > +static int imx8qxp_pixel_link_disable_mst_vld(struct imx8qxp_pixel_link *pl) > +{ > + int ret; > + > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > + pl->mst_vld_ctrl, false); > + if (ret) > + DRM_DEV_ERROR(pl->dev, > + "failed to disable DC%d stream%d pixel link mst_vld: %d\n", > + pl->dc_id, pl->stream_id, ret); > + > + return ret; > +} > + > +static int imx8qxp_pixel_link_disable_sync(struct imx8qxp_pixel_link *pl) > +{ > + int ret; > + > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > + pl->sync_ctrl, false); > + if (ret) > + DRM_DEV_ERROR(pl->dev, > + "failed to disable DC%d stream%d pixel link sync: %d\n", > + pl->dc_id, pl->stream_id, ret); > + > + return ret; > +} > + > +static void imx8qxp_pixel_link_set_mst_addr(struct imx8qxp_pixel_link *pl) > +{ > + int ret; > + > + ret = imx_sc_misc_set_control(pl->ipc_handle, > + pl->sink_rsc, pl->mst_addr_ctrl, > + pl->mst_addr); > + if (ret) > + DRM_DEV_ERROR(pl->dev, > + "failed to set DC%d stream%d pixel link mst addr(%u): %d\n", > + pl->dc_id, pl->stream_id, pl->mst_addr, ret); > +} > + > +static int imx8qxp_pixel_link_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + struct imx8qxp_pixel_link *pl = bridge->driver_private; > + > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > + DRM_DEV_ERROR(pl->dev, > + "do not support creating a drm_connector\n"); > + return -EINVAL; > + } > + > + if (!bridge->encoder) { > + DRM_DEV_ERROR(pl->dev, "missing encoder\n"); > + return -ENODEV; > + } > + > + return drm_bridge_attach(bridge->encoder, > + pl->next_bridge, bridge, > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > +} > + > +static void > +imx8qxp_pixel_link_bridge_mode_set(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + const struct drm_display_mode *adjusted_mode) > +{ > + struct imx8qxp_pixel_link *pl = bridge->driver_private; > + > + imx8qxp_pixel_link_set_mst_addr(pl); > +} > + > +static void imx8qxp_pixel_link_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > +{ > + struct imx8qxp_pixel_link *pl = bridge->driver_private; > + > + imx8qxp_pixel_link_enable_mst_en(pl); > + imx8qxp_pixel_link_enable_mst_vld(pl); > + imx8qxp_pixel_link_enable_sync(pl); > +} > + > +static void imx8qxp_pixel_link_bridge_atomic_disable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > +{ > + struct imx8qxp_pixel_link *pl = bridge->driver_private; > + > + imx8qxp_pixel_link_disable_mst_en(pl); > + imx8qxp_pixel_link_disable_mst_vld(pl); > + imx8qxp_pixel_link_disable_sync(pl); > +} > + > +static const u32 imx8qxp_pixel_link_bus_output_fmts[] = { > + MEDIA_BUS_FMT_RGB888_1X36_CPADLO, > + MEDIA_BUS_FMT_RGB666_1X36_CPADLO, > +}; > + > +static bool imx8qxp_pixel_link_bus_output_fmt_supported(u32 fmt) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(imx8qxp_pixel_link_bus_output_fmts); i++) { > + if (imx8qxp_pixel_link_bus_output_fmts[i] == fmt) > + return true; > + } > + > + return false; > +} > + > +static u32 * > +imx8qxp_pixel_link_bridge_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) > +{ > + u32 *input_fmts; > + > + if (!imx8qxp_pixel_link_bus_output_fmt_supported(output_fmt)) > + return NULL; > + > + *num_input_fmts = 1; > + > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + input_fmts[0] = output_fmt; > + > + return input_fmts; > +} > + > +static u32 * > +imx8qxp_pixel_link_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + unsigned int *num_output_fmts) > +{ > + *num_output_fmts = ARRAY_SIZE(imx8qxp_pixel_link_bus_output_fmts); > + return kmemdup(imx8qxp_pixel_link_bus_output_fmts, > + sizeof(imx8qxp_pixel_link_bus_output_fmts), GFP_KERNEL); > +} > + > +static const struct drm_bridge_funcs imx8qxp_pixel_link_bridge_funcs = { > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_reset = drm_atomic_helper_bridge_reset, > + .attach = imx8qxp_pixel_link_bridge_attach, > + .mode_set = imx8qxp_pixel_link_bridge_mode_set, > + .atomic_enable = imx8qxp_pixel_link_bridge_atomic_enable, > + .atomic_disable = imx8qxp_pixel_link_bridge_atomic_disable, > + .atomic_get_input_bus_fmts = > + imx8qxp_pixel_link_bridge_atomic_get_input_bus_fmts, > + .atomic_get_output_bus_fmts = > + imx8qxp_pixel_link_bridge_atomic_get_output_bus_fmts, > +}; > + > +static int imx8qxp_pixel_link_disable_all_controls(struct imx8qxp_pixel_link *pl) > +{ > + int ret; > + > + ret = imx8qxp_pixel_link_disable_mst_en(pl); > + if (ret) > + return ret; > + > + ret = imx8qxp_pixel_link_disable_mst_vld(pl); > + if (ret) > + return ret; > + > + return imx8qxp_pixel_link_disable_sync(pl); > +} > + > +static struct drm_bridge * > +imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl) > +{ > + struct device_node *np = pl->dev->of_node; > + struct device_node *port, *remote; > + struct drm_bridge *next_bridge[PL_MAX_NEXT_BRIDGES]; > + u32 port_id; > + bool found_port = false; > + int reg, ep_cnt = 0; > + int bridge_sel = 0; /* select the first next bridge by default */ Is this comment on the wrong line? > + > + for (port_id = 1; port_id <= PL_MAX_MST_ADDR + 1; port_id++) { > + port = of_graph_get_port_by_id(np, port_id); > + if (!port) > + continue; > + > + if (of_device_is_available(port)) { > + found_port = true; > + of_node_put(port); > + break; > + } > + > + of_node_put(port); > + } > + > + if (!found_port) { > + DRM_DEV_ERROR(pl->dev, "no available output port\n"); > + return ERR_PTR(-ENODEV); > + } > + > + for (reg = 0; reg < PL_MAX_NEXT_BRIDGES; reg++) { > + remote = of_graph_get_remote_node(np, port_id, reg); > + if (!remote) > + continue; > + > + if (!of_device_is_available(remote->parent)) { > + DRM_DEV_DEBUG(pl->dev, > + "port%u endpoint%u remote parent is not available\n", > + port_id, reg); > + of_node_put(remote); > + continue; > + } > + > + next_bridge[ep_cnt] = of_drm_find_bridge(remote); > + if (!next_bridge[ep_cnt]) { > + of_node_put(remote); > + return ERR_PTR(-EPROBE_DEFER); > + } > + > + /* specially select the next bridge with companion PXL2DPI */ > + if (of_find_property(remote, "fsl,companion-pxl2dpi", NULL)) > + bridge_sel = ep_cnt; > + > + ep_cnt++; > + > + of_node_put(remote); > + } > + > + pl->mst_addr = port_id - 1; > + > + return next_bridge[bridge_sel]; > +} > + > +static int imx8qxp_pixel_link_bridge_probe(struct platform_device *pdev) > +{ > + struct imx8qxp_pixel_link *pl; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + int ret; > + > + pl = devm_kzalloc(dev, sizeof(*pl), GFP_KERNEL); > + if (!pl) > + return -ENOMEM; > + > + ret = imx_scu_get_handle(&pl->ipc_handle); > + if (ret) { > + if (ret != -EPROBE_DEFER) > + DRM_DEV_ERROR(dev, "failed to get SCU ipc handle: %d\n", > + ret); > + return ret; > + } > + > + pl->id = of_alias_get_id(np, "dc_pl"); > + if (pl->id < 0) { > + DRM_DEV_ERROR(dev, > + "failed to get pixel link node alias id: %d\n", > + pl->id); > + return pl->id; > + } > + > + pl->dev = dev; > + > + pl->dc_id = pl->id / 2; > + pl->stream_id = pl->id % 2; > + > + pl->sink_rsc = pl->dc_id ? IMX_SC_R_DC_1 : IMX_SC_R_DC_0; > + > + if (pl->stream_id == 0) { > + pl->mst_addr_ctrl = IMX_SC_C_PXL_LINK_MST1_ADDR; > + pl->mst_en_ctrl = IMX_SC_C_PXL_LINK_MST1_ENB; > + pl->mst_vld_ctrl = IMX_SC_C_PXL_LINK_MST1_VLD; > + pl->sync_ctrl = IMX_SC_C_SYNC_CTRL0; > + } else { > + pl->mst_addr_ctrl = IMX_SC_C_PXL_LINK_MST2_ADDR; > + pl->mst_en_ctrl = IMX_SC_C_PXL_LINK_MST2_ENB; > + pl->mst_vld_ctrl = IMX_SC_C_PXL_LINK_MST2_VLD; > + pl->sync_ctrl = IMX_SC_C_SYNC_CTRL1; > + } > + > + /* disable all controls to POR default */ > + ret = imx8qxp_pixel_link_disable_all_controls(pl); > + if (ret) > + return ret; > + > + pl->next_bridge = imx8qxp_pixel_link_find_next_bridge(pl); > + if (IS_ERR(pl->next_bridge)) { > + ret = PTR_ERR(pl->next_bridge); > + if (ret != -EPROBE_DEFER) > + DRM_DEV_ERROR(dev, "failed to find next bridge: %d\n", > + ret); > + return ret; > + } > + > + platform_set_drvdata(pdev, pl); > + > + pl->bridge.driver_private = pl; > + pl->bridge.funcs = &imx8qxp_pixel_link_bridge_funcs; > + pl->bridge.of_node = np; > + > + drm_bridge_add(&pl->bridge); > + > + return ret; > +} > + > +static int imx8qxp_pixel_link_bridge_remove(struct platform_device *pdev) > +{ > + struct imx8qxp_pixel_link *pl = platform_get_drvdata(pdev); > + > + drm_bridge_remove(&pl->bridge); > + > + return 0; > +} > + > +static const struct of_device_id imx8qxp_pixel_link_dt_ids[] = { > + { .compatible = "fsl,imx8qm-dc-pixel-link", }, > + { .compatible = "fsl,imx8qxp-dc-pixel-link", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, imx8qxp_pixel_link_dt_ids); > + > +static struct platform_driver imx8qxp_pixel_link_bridge_driver = { > + .probe = imx8qxp_pixel_link_bridge_probe, > + .remove = imx8qxp_pixel_link_bridge_remove, > + .driver = { > + .of_match_table = imx8qxp_pixel_link_dt_ids, > + .name = DRIVER_NAME, > + }, > +}; > +module_platform_driver(imx8qxp_pixel_link_bridge_driver); > + > +MODULE_DESCRIPTION("i.MX8QXP/QM display pixel link bridge driver"); > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" DRIVER_NAME); > -- > 2.7.4 >
Hi Robert, On Tue, 2021-03-02 at 14:53 +0100, Robert Foss wrote: > Hey Liu, > > Thanks for submitting this patch. > > I only have one comment below. With that addressed, feel free to add my r-b. > > Reviewed-by: Robert Foss <robert.foss@linaro.org> Thanks for reviewing this patch. > > On Thu, 18 Feb 2021 at 04:59, Liu Ying <victor.liu@nxp.com> wrote: > > This patch adds a drm bridge driver for i.MX8qm/qxp display pixel link. > > The pixel link forms a standard asynchronous linkage between > > pixel sources(display controller or camera module) and pixel > > consumers(imaging or displays). It consists of two distinct > > functions, a pixel transfer function and a control interface. > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > v3->v4: > > * No change. > > > > v2->v3: > > * Drop two macros which help define functions and define them directly. > > * Properly disable all pixel link controls to POR value by calling > > imx8qxp_pixel_link_disable_all_controls() from > > imx8qxp_pixel_link_bridge_probe(). > > > > v1->v2: > > * Rewrite the function to find the next bridge by properly using OF APIs > > and dropping unnecessary DT validation. (Rob) > > > > drivers/gpu/drm/bridge/imx/Kconfig | 8 + > > drivers/gpu/drm/bridge/imx/Makefile | 1 + > > drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 426 ++++++++++++++++++++++++ > > 3 files changed, 435 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c > > > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig > > index f1c91b6..4d1f027 100644 > > --- a/drivers/gpu/drm/bridge/imx/Kconfig > > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > > @@ -6,3 +6,11 @@ config DRM_IMX8QXP_PIXEL_COMBINER > > help > > Choose this to enable pixel combiner found in > > Freescale i.MX8qm/qxp processors. > > + > > +config DRM_IMX8QXP_PIXEL_LINK > > + tristate "Freescale i.MX8QM/QXP display pixel link" > > + depends on OF > > + select DRM_KMS_HELPER > > + help > > + Choose this to enable display pixel link found in > > + Freescale i.MX8qm/qxp processors. > > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile > > index 7d7c8d6..c15469f 100644 > > --- a/drivers/gpu/drm/bridge/imx/Makefile > > +++ b/drivers/gpu/drm/bridge/imx/Makefile > > @@ -1 +1,2 @@ > > obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o > > +obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o > > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c > > new file mode 100644 > > index 00000000..2e5ba4a > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c > > @@ -0,0 +1,426 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +/* > > + * Copyright 2020 NXP > > + */ > > + > > +#include <linux/firmware/imx/svc/misc.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_graph.h> > > +#include <linux/platform_device.h> > > + > > +#include <drm/drm_atomic_state_helper.h> > > +#include <drm/drm_bridge.h> > > +#include <drm/drm_print.h> > > + > > +#include <dt-bindings/firmware/imx/rsrc.h> > > + > > +#define DRIVER_NAME "imx8qxp-display-pixel-link" > > +#define PL_MAX_MST_ADDR 3 > > +#define PL_MAX_NEXT_BRIDGES 2 > > + > > +struct imx8qxp_pixel_link { > > + struct drm_bridge bridge; > > + struct drm_bridge *next_bridge; > > + struct device *dev; > > + struct imx_sc_ipc *ipc_handle; > > + int id; > > + int stream_id; > > + int dc_id; > > + u32 sink_rsc; > > + u32 mst_addr; > > + u8 mst_addr_ctrl; > > + u8 mst_en_ctrl; > > + u8 mst_vld_ctrl; > > + u8 sync_ctrl; > > +}; > > + > > +static void imx8qxp_pixel_link_enable_mst_en(struct imx8qxp_pixel_link *pl) > > +{ > > + int ret; > > + > > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > > + pl->mst_en_ctrl, true); > > + if (ret) > > + DRM_DEV_ERROR(pl->dev, > > + "failed to enable DC%d stream%d pixel link mst_en: %d\n", > > + pl->dc_id, pl->stream_id, ret); > > +} > > + > > +static void imx8qxp_pixel_link_enable_mst_vld(struct imx8qxp_pixel_link *pl) > > +{ > > + int ret; > > + > > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > > + pl->mst_vld_ctrl, true); > > + if (ret) > > + DRM_DEV_ERROR(pl->dev, > > + "failed to enable DC%d stream%d pixel link mst_vld: %d\n", > > + pl->dc_id, pl->stream_id, ret); > > +} > > + > > +static void imx8qxp_pixel_link_enable_sync(struct imx8qxp_pixel_link *pl) > > +{ > > + int ret; > > + > > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > > + pl->sync_ctrl, true); > > + if (ret) > > + DRM_DEV_ERROR(pl->dev, > > + "failed to enable DC%d stream%d pixel link sync: %d\n", > > + pl->dc_id, pl->stream_id, ret); > > +} > > + > > +static int imx8qxp_pixel_link_disable_mst_en(struct imx8qxp_pixel_link *pl) > > +{ > > + int ret; > > + > > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > > + pl->mst_en_ctrl, false); > > + if (ret) > > + DRM_DEV_ERROR(pl->dev, > > + "failed to disable DC%d stream%d pixel link mst_en: %d\n", > > + pl->dc_id, pl->stream_id, ret); > > + > > + return ret; > > +} > > + > > +static int imx8qxp_pixel_link_disable_mst_vld(struct imx8qxp_pixel_link *pl) > > +{ > > + int ret; > > + > > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > > + pl->mst_vld_ctrl, false); > > + if (ret) > > + DRM_DEV_ERROR(pl->dev, > > + "failed to disable DC%d stream%d pixel link mst_vld: %d\n", > > + pl->dc_id, pl->stream_id, ret); > > + > > + return ret; > > +} > > + > > +static int imx8qxp_pixel_link_disable_sync(struct imx8qxp_pixel_link *pl) > > +{ > > + int ret; > > + > > + ret = imx_sc_misc_set_control(pl->ipc_handle, pl->sink_rsc, > > + pl->sync_ctrl, false); > > + if (ret) > > + DRM_DEV_ERROR(pl->dev, > > + "failed to disable DC%d stream%d pixel link sync: %d\n", > > + pl->dc_id, pl->stream_id, ret); > > + > > + return ret; > > +} > > + > > +static void imx8qxp_pixel_link_set_mst_addr(struct imx8qxp_pixel_link *pl) > > +{ > > + int ret; > > + > > + ret = imx_sc_misc_set_control(pl->ipc_handle, > > + pl->sink_rsc, pl->mst_addr_ctrl, > > + pl->mst_addr); > > + if (ret) > > + DRM_DEV_ERROR(pl->dev, > > + "failed to set DC%d stream%d pixel link mst addr(%u): %d\n", > > + pl->dc_id, pl->stream_id, pl->mst_addr, ret); > > +} > > + > > +static int imx8qxp_pixel_link_bridge_attach(struct drm_bridge *bridge, > > + enum drm_bridge_attach_flags flags) > > +{ > > + struct imx8qxp_pixel_link *pl = bridge->driver_private; > > + > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > + DRM_DEV_ERROR(pl->dev, > > + "do not support creating a drm_connector\n"); > > + return -EINVAL; > > + } > > + > > + if (!bridge->encoder) { > > + DRM_DEV_ERROR(pl->dev, "missing encoder\n"); > > + return -ENODEV; > > + } > > + > > + return drm_bridge_attach(bridge->encoder, > > + pl->next_bridge, bridge, > > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > +} > > + > > +static void > > +imx8qxp_pixel_link_bridge_mode_set(struct drm_bridge *bridge, > > + const struct drm_display_mode *mode, > > + const struct drm_display_mode *adjusted_mode) > > +{ > > + struct imx8qxp_pixel_link *pl = bridge->driver_private; > > + > > + imx8qxp_pixel_link_set_mst_addr(pl); > > +} > > + > > +static void imx8qxp_pixel_link_bridge_atomic_enable(struct drm_bridge *bridge, > > + struct drm_bridge_state *old_bridge_state) > > +{ > > + struct imx8qxp_pixel_link *pl = bridge->driver_private; > > + > > + imx8qxp_pixel_link_enable_mst_en(pl); > > + imx8qxp_pixel_link_enable_mst_vld(pl); > > + imx8qxp_pixel_link_enable_sync(pl); > > +} > > + > > +static void imx8qxp_pixel_link_bridge_atomic_disable(struct drm_bridge *bridge, > > + struct drm_bridge_state *old_bridge_state) > > +{ > > + struct imx8qxp_pixel_link *pl = bridge->driver_private; > > + > > + imx8qxp_pixel_link_disable_mst_en(pl); > > + imx8qxp_pixel_link_disable_mst_vld(pl); > > + imx8qxp_pixel_link_disable_sync(pl); > > +} > > + > > +static const u32 imx8qxp_pixel_link_bus_output_fmts[] = { > > + MEDIA_BUS_FMT_RGB888_1X36_CPADLO, > > + MEDIA_BUS_FMT_RGB666_1X36_CPADLO, > > +}; > > + > > +static bool imx8qxp_pixel_link_bus_output_fmt_supported(u32 fmt) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(imx8qxp_pixel_link_bus_output_fmts); i++) { > > + if (imx8qxp_pixel_link_bus_output_fmts[i] == fmt) > > + return true; > > + } > > + > > + return false; > > +} > > + > > +static u32 * > > +imx8qxp_pixel_link_bridge_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) > > +{ > > + u32 *input_fmts; > > + > > + if (!imx8qxp_pixel_link_bus_output_fmt_supported(output_fmt)) > > + return NULL; > > + > > + *num_input_fmts = 1; > > + > > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > > + if (!input_fmts) > > + return NULL; > > + > > + input_fmts[0] = output_fmt; > > + > > + return input_fmts; > > +} > > + > > +static u32 * > > +imx8qxp_pixel_link_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > > + struct drm_bridge_state *bridge_state, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state, > > + unsigned int *num_output_fmts) > > +{ > > + *num_output_fmts = ARRAY_SIZE(imx8qxp_pixel_link_bus_output_fmts); > > + return kmemdup(imx8qxp_pixel_link_bus_output_fmts, > > + sizeof(imx8qxp_pixel_link_bus_output_fmts), GFP_KERNEL); > > +} > > + > > +static const struct drm_bridge_funcs imx8qxp_pixel_link_bridge_funcs = { > > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > + .atomic_reset = drm_atomic_helper_bridge_reset, > > + .attach = imx8qxp_pixel_link_bridge_attach, > > + .mode_set = imx8qxp_pixel_link_bridge_mode_set, > > + .atomic_enable = imx8qxp_pixel_link_bridge_atomic_enable, > > + .atomic_disable = imx8qxp_pixel_link_bridge_atomic_disable, > > + .atomic_get_input_bus_fmts = > > + imx8qxp_pixel_link_bridge_atomic_get_input_bus_fmts, > > + .atomic_get_output_bus_fmts = > > + imx8qxp_pixel_link_bridge_atomic_get_output_bus_fmts, > > +}; > > + > > +static int imx8qxp_pixel_link_disable_all_controls(struct imx8qxp_pixel_link *pl) > > +{ > > + int ret; > > + > > + ret = imx8qxp_pixel_link_disable_mst_en(pl); > > + if (ret) > > + return ret; > > + > > + ret = imx8qxp_pixel_link_disable_mst_vld(pl); > > + if (ret) > > + return ret; > > + > > + return imx8qxp_pixel_link_disable_sync(pl); > > +} > > + > > +static struct drm_bridge * > > +imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl) > > +{ > > + struct device_node *np = pl->dev->of_node; > > + struct device_node *port, *remote; > > + struct drm_bridge *next_bridge[PL_MAX_NEXT_BRIDGES]; > > + u32 port_id; > > + bool found_port = false; > > + int reg, ep_cnt = 0; > > + int bridge_sel = 0; /* select the first next bridge by default */ > > Is this comment on the wrong line? I'll make this comment occupy a line just prior to this line in the next version. Thanks, Liu Ying > > > + > > + for (port_id = 1; port_id <= PL_MAX_MST_ADDR + 1; port_id++) { > > + port = of_graph_get_port_by_id(np, port_id); > > + if (!port) > > + continue; > > + > > + if (of_device_is_available(port)) { > > + found_port = true; > > + of_node_put(port); > > + break; > > + } > > + > > + of_node_put(port); > > + } > > + > > + if (!found_port) { > > + DRM_DEV_ERROR(pl->dev, "no available output port\n"); > > + return ERR_PTR(-ENODEV); > > + } > > + > > + for (reg = 0; reg < PL_MAX_NEXT_BRIDGES; reg++) { > > + remote = of_graph_get_remote_node(np, port_id, reg); > > + if (!remote) > > + continue; > > + > > + if (!of_device_is_available(remote->parent)) { > > + DRM_DEV_DEBUG(pl->dev, > > + "port%u endpoint%u remote parent is not available\n", > > + port_id, reg); > > + of_node_put(remote); > > + continue; > > + } > > + > > + next_bridge[ep_cnt] = of_drm_find_bridge(remote); > > + if (!next_bridge[ep_cnt]) { > > + of_node_put(remote); > > + return ERR_PTR(-EPROBE_DEFER); > > + } > > + > > + /* specially select the next bridge with companion PXL2DPI */ > > + if (of_find_property(remote, "fsl,companion-pxl2dpi", NULL)) > > + bridge_sel = ep_cnt; > > + > > + ep_cnt++; > > + > > + of_node_put(remote); > > + } > > + > > + pl->mst_addr = port_id - 1; > > + > > + return next_bridge[bridge_sel]; > > +} > > + > > +static int imx8qxp_pixel_link_bridge_probe(struct platform_device *pdev) > > +{ > > + struct imx8qxp_pixel_link *pl; > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + int ret; > > + > > + pl = devm_kzalloc(dev, sizeof(*pl), GFP_KERNEL); > > + if (!pl) > > + return -ENOMEM; > > + > > + ret = imx_scu_get_handle(&pl->ipc_handle); > > + if (ret) { > > + if (ret != -EPROBE_DEFER) > > + DRM_DEV_ERROR(dev, "failed to get SCU ipc handle: %d\n", > > + ret); > > + return ret; > > + } > > + > > + pl->id = of_alias_get_id(np, "dc_pl"); > > + if (pl->id < 0) { > > + DRM_DEV_ERROR(dev, > > + "failed to get pixel link node alias id: %d\n", > > + pl->id); > > + return pl->id; > > + } > > + > > + pl->dev = dev; > > + > > + pl->dc_id = pl->id / 2; > > + pl->stream_id = pl->id % 2; > > + > > + pl->sink_rsc = pl->dc_id ? IMX_SC_R_DC_1 : IMX_SC_R_DC_0; > > + > > + if (pl->stream_id == 0) { > > + pl->mst_addr_ctrl = IMX_SC_C_PXL_LINK_MST1_ADDR; > > + pl->mst_en_ctrl = IMX_SC_C_PXL_LINK_MST1_ENB; > > + pl->mst_vld_ctrl = IMX_SC_C_PXL_LINK_MST1_VLD; > > + pl->sync_ctrl = IMX_SC_C_SYNC_CTRL0; > > + } else { > > + pl->mst_addr_ctrl = IMX_SC_C_PXL_LINK_MST2_ADDR; > > + pl->mst_en_ctrl = IMX_SC_C_PXL_LINK_MST2_ENB; > > + pl->mst_vld_ctrl = IMX_SC_C_PXL_LINK_MST2_VLD; > > + pl->sync_ctrl = IMX_SC_C_SYNC_CTRL1; > > + } > > + > > + /* disable all controls to POR default */ > > + ret = imx8qxp_pixel_link_disable_all_controls(pl); > > + if (ret) > > + return ret; > > + > > + pl->next_bridge = imx8qxp_pixel_link_find_next_bridge(pl); > > + if (IS_ERR(pl->next_bridge)) { > > + ret = PTR_ERR(pl->next_bridge); > > + if (ret != -EPROBE_DEFER) > > + DRM_DEV_ERROR(dev, "failed to find next bridge: %d\n", > > + ret); > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, pl); > > + > > + pl->bridge.driver_private = pl; > > + pl->bridge.funcs = &imx8qxp_pixel_link_bridge_funcs; > > + pl->bridge.of_node = np; > > + > > + drm_bridge_add(&pl->bridge); > > + > > + return ret; > > +} > > + > > +static int imx8qxp_pixel_link_bridge_remove(struct platform_device *pdev) > > +{ > > + struct imx8qxp_pixel_link *pl = platform_get_drvdata(pdev); > > + > > + drm_bridge_remove(&pl->bridge); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id imx8qxp_pixel_link_dt_ids[] = { > > + { .compatible = "fsl,imx8qm-dc-pixel-link", }, > > + { .compatible = "fsl,imx8qxp-dc-pixel-link", }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, imx8qxp_pixel_link_dt_ids); > > + > > +static struct platform_driver imx8qxp_pixel_link_bridge_driver = { > > + .probe = imx8qxp_pixel_link_bridge_probe, > > + .remove = imx8qxp_pixel_link_bridge_remove, > > + .driver = { > > + .of_match_table = imx8qxp_pixel_link_dt_ids, > > + .name = DRIVER_NAME, > > + }, > > +}; > > +module_platform_driver(imx8qxp_pixel_link_bridge_driver); > > + > > +MODULE_DESCRIPTION("i.MX8QXP/QM display pixel link bridge driver"); > > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_ALIAS("platform:" DRIVER_NAME); > > -- > > 2.7.4 > >