mbox series

[v4,00/14] Add some DRM bridge drivers support for i.MX8qm/qxp SoCs

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

Message

Liu Ying Feb. 18, 2021, 3:41 a.m. UTC
Hi,

This is the v4 series to add some DRM bridge drivers support
for i.MX8qm/qxp SoCs.

The bridges may chain one by one to form display pipes to support
LVDS displays.  The relevant display controller is DPU embedded in
i.MX8qm/qxp SoCs.

The DPU KMS driver can be found at:
https://www.spinics.net/lists/arm-kernel/msg871357.html

This series supports the following display pipes:
1) i.MX8qxp:
prefetch eng -> DPU -> pixel combiner -> pixel link ->
pixel link to DPI(PXL2DPI) -> LVDS display bridge(LDB)

2) i.MX8qm:
prefetch eng -> DPU -> pixel combiner -> pixel link -> LVDS display bridge(LDB)


Patch 1/14 adds LVDS PHY configuration options, which has already been sent
with the following series to add Mixel combo PHY found in i.MX8qxp:
https://www.spinics.net/lists/arm-kernel/msg862560.html

Patch 2/14 and 3/14 add bus formats used by PXL2DPI.

Patch 4/14 ~ 13/14 add drm bridge drivers and dt-bindings support for the bridges.

Patch 14/14 updates MAINTAINERS.


I've tested this series with a koe,tx26d202vm0bwa dual link LVDS panel and
a LVDS to HDMI bridge(with a downstream drm bridge driver).


Welcome comments, thanks.

v3->v4:
* Use 'fsl,sc-resource' DT property to get the SCU resource ID associated with
  the PXL2DPI instance instead of using alias ID. (Rob)
* Add Rob's R-b tag on patch 11/14.

v2->v3:
* Drop 'fsl,syscon' DT properties from fsl,imx8qxp-ldb.yaml and
  fsl,imx8qxp-pxl2dpi.yaml. (Rob)
* Mention the CSR module controls LDB and PXL2DPI in fsl,imx8qxp-ldb.yaml and
  fsl,imx8qxp-pxl2dpi.yaml.
* Call syscon_node_to_regmap() to get regmaps from LDB bridge helper driver
  and PXL2DPI bridger driver instead of syscon_regmap_lookup_by_phandle().
* Drop two macros from pixel link bridge driver 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().
* Add Rob's R-b tags on patch 4/14 and 6/14.

v1->v2:
* Rebase the series upon the latest drm-misc-next branch(5.11-rc2 based).
* Use graph schema in the dt-bindings of the bridges. (Laurent)
* Require all four pixel link output ports in fsl,imx8qxp-pixel-link.yaml.
  (Laurent)
* Side note i.MX8qm/qxp LDB official name 'pixel mapper' in fsl,imx8qxp-ldb.yaml.
  (Laurent)
* Mention pixel link is accessed via SCU firmware in fsl,imx8qxp-pixel-link.yaml.
  (Rob)
* Use enum instead of oneOf + const for the reg property of pixel combiner
  channels in fsl,imx8qxp-pixel-combiner.yaml. (Rob)
* Rewrite the function to find the next bridge in pixel link bridge driver
  by properly using OF APIs and dropping unnecessary DT validation. (Rob)
* Drop unnecessary port availability check in i.MX8qxp pixel link to DPI
  bridge driver.
* Drop unnecessary DT validation from i.MX8qxp LDB bridge driver.
* Use of_graph_get_endpoint_by_regs() and of_graph_get_remote_endpoint() to
  get the input remote endpoint in imx8qxp_ldb_set_di_id() of i.MX8qxp LDB
  bridge driver.
* Avoid using companion_port OF node after putting it in
  imx8qxp_ldb_parse_dt_companion() of i.MX8qxp LDB bridge driver.
* Drop unnecessary check for maximum available LDB channels from
  i.MX8qm LDB bridge driver.
* Mention i.MX8qm/qxp LDB official name 'pixel mapper' in i.MX8qm/qxp LDB
  bridge drivers and Kconfig help messages.
Liu Ying (14):
  phy: Add LVDS configuration options
  media: uapi: Add some RGB bus formats for i.MX8qm/qxp pixel combiner
  media: docs: Add some RGB bus formats for i.MX8qm/qxp pixel combiner
  dt-bindings: display: bridge: Add i.MX8qm/qxp pixel combiner binding
  drm/bridge: imx: Add i.MX8qm/qxp pixel combiner support
  dt-bindings: display: bridge: Add i.MX8qm/qxp display pixel link
    binding
  drm/bridge: imx: Add i.MX8qm/qxp display pixel link support
  dt-bindings: display: bridge: Add i.MX8qxp pixel link to DPI binding
  drm/bridge: imx: Add i.MX8qxp pixel link to DPI support
  drm/bridge: imx: Add LDB driver helper support
  dt-bindings: display: bridge: Add i.MX8qm/qxp LVDS display bridge
    binding
  drm/bridge: imx: Add LDB support for i.MX8qxp
  drm/bridge: imx: Add LDB support for i.MX8qm
  MAINTAINERS: add maintainer for DRM bridge drivers for i.MX SoCs

 .../bindings/display/bridge/fsl,imx8qxp-ldb.yaml   | 173 +++++
 .../display/bridge/fsl,imx8qxp-pixel-combiner.yaml | 144 +++++
 .../display/bridge/fsl,imx8qxp-pixel-link.yaml     | 106 +++
 .../display/bridge/fsl,imx8qxp-pxl2dpi.yaml        | 108 ++++
 .../userspace-api/media/v4l/subdev-formats.rst     | 156 +++++
 MAINTAINERS                                        |  10 +
 drivers/gpu/drm/bridge/Kconfig                     |   2 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/imx/Kconfig                 |  52 ++
 drivers/gpu/drm/bridge/imx/Makefile                |   6 +
 drivers/gpu/drm/bridge/imx/imx-ldb-helper.c        | 248 +++++++
 drivers/gpu/drm/bridge/imx/imx8qm-ldb.c            | 585 +++++++++++++++++
 drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c           | 719 +++++++++++++++++++++
 .../gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c    | 452 +++++++++++++
 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c    | 426 ++++++++++++
 drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c       | 485 ++++++++++++++
 include/drm/bridge/imx_ldb_helper.h                |  98 +++
 include/linux/phy/phy-lvds.h                       |  48 ++
 include/linux/phy/phy.h                            |   4 +
 include/uapi/linux/media-bus-format.h              |   6 +-
 20 files changed, 3828 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx8qxp-ldb.yaml
 create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx8qxp-pixel-combiner.yaml
 create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx8qxp-pixel-link.yaml
 create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx8qxp-pxl2dpi.yaml
 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/imx-ldb-helper.c
 create mode 100644 drivers/gpu/drm/bridge/imx/imx8qm-ldb.c
 create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
 create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
 create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
 create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
 create mode 100644 include/drm/bridge/imx_ldb_helper.h
 create mode 100644 include/linux/phy/phy-lvds.h

Comments

Robert Foss Feb. 26, 2021, 12:44 p.m. UTC | #1
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
>
Robert Foss Feb. 26, 2021, 1:07 p.m. UTC | #2
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
>
Liu Ying March 1, 2021, 9:05 a.m. UTC | #3
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
> >
Robert Foss March 1, 2021, 10:56 a.m. UTC | #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
> > >
>
Liu Ying March 2, 2021, 1:36 a.m. UTC | #5
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
> > > >
Robert Foss March 2, 2021, 1:53 p.m. UTC | #6
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
>
Liu Ying March 3, 2021, 6:59 a.m. UTC | #7
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
> >