diff mbox series

[v2,06/16] drm/imx: Add i.MX8qxp Display Controller display engine

Message ID 20240712093243.2108456-7-victor.liu@nxp.com (mailing list archive)
State Superseded
Headers show
Series Add Freescale i.MX8qxp Display Controller support | expand

Commit Message

Liu Ying July 12, 2024, 9:32 a.m. UTC
i.MX8qxp Display Controller display engine consists of all processing
units that operate in a display clock domain.  Add minimal feature
support with FrameGen and TCon so that the engine can output display
timings.  The display engine driver as a master binds FrameGen and
TCon drivers as components.  While at it, the display engine driver
is a component to be bound with the upcoming DRM driver.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
* Use OF alias id to get instance id.
* Add dev member to struct dc_tc.

 drivers/gpu/drm/imx/Kconfig     |   1 +
 drivers/gpu/drm/imx/Makefile    |   1 +
 drivers/gpu/drm/imx/dc/Kconfig  |   5 +
 drivers/gpu/drm/imx/dc/Makefile |   5 +
 drivers/gpu/drm/imx/dc/dc-de.c  | 151 +++++++++++++
 drivers/gpu/drm/imx/dc/dc-de.h  |  62 ++++++
 drivers/gpu/drm/imx/dc/dc-drv.c |  32 +++
 drivers/gpu/drm/imx/dc/dc-drv.h |  24 +++
 drivers/gpu/drm/imx/dc/dc-fg.c  | 366 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/dc/dc-tc.c  | 137 ++++++++++++
 10 files changed, 784 insertions(+)
 create mode 100644 drivers/gpu/drm/imx/dc/Kconfig
 create mode 100644 drivers/gpu/drm/imx/dc/Makefile
 create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c
 create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h
 create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c
 create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h
 create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c
 create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c

Comments

Dmitry Baryshkov July 27, 2024, 4:11 p.m. UTC | #1
On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote:
> i.MX8qxp Display Controller display engine consists of all processing
> units that operate in a display clock domain.  Add minimal feature
> support with FrameGen and TCon so that the engine can output display
> timings.  The display engine driver as a master binds FrameGen and
> TCon drivers as components.  While at it, the display engine driver
> is a component to be bound with the upcoming DRM driver.

Generic question: why do you need so many small subdrivers? Are they
used to represent the flexibility of the pipeline? Can you instantiate
these units directly from the de(?) driver and reference created
structures without the need to create subdevices?

> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> v2:
> * Use OF alias id to get instance id.
> * Add dev member to struct dc_tc.
> 
>  drivers/gpu/drm/imx/Kconfig     |   1 +
>  drivers/gpu/drm/imx/Makefile    |   1 +
>  drivers/gpu/drm/imx/dc/Kconfig  |   5 +
>  drivers/gpu/drm/imx/dc/Makefile |   5 +
>  drivers/gpu/drm/imx/dc/dc-de.c  | 151 +++++++++++++
>  drivers/gpu/drm/imx/dc/dc-de.h  |  62 ++++++
>  drivers/gpu/drm/imx/dc/dc-drv.c |  32 +++
>  drivers/gpu/drm/imx/dc/dc-drv.h |  24 +++
>  drivers/gpu/drm/imx/dc/dc-fg.c  | 366 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/dc/dc-tc.c  | 137 ++++++++++++
>  10 files changed, 784 insertions(+)
>  create mode 100644 drivers/gpu/drm/imx/dc/Kconfig
>  create mode 100644 drivers/gpu/drm/imx/dc/Makefile
>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c
>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h
>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c
>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h
>  create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c
>  create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c
> 
> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
> index 03535a15dd8f..3e8c6edbc17c 100644
> --- a/drivers/gpu/drm/imx/Kconfig
> +++ b/drivers/gpu/drm/imx/Kconfig
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
> +source "drivers/gpu/drm/imx/dc/Kconfig"
>  source "drivers/gpu/drm/imx/dcss/Kconfig"
>  source "drivers/gpu/drm/imx/ipuv3/Kconfig"
>  source "drivers/gpu/drm/imx/lcdc/Kconfig"
> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
> index 86f38e7c7422..c7b317640d71 100644
> --- a/drivers/gpu/drm/imx/Makefile
> +++ b/drivers/gpu/drm/imx/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +obj-$(CONFIG_DRM_IMX8_DC) += dc/
>  obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
>  obj-$(CONFIG_DRM_IMX) += ipuv3/
>  obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
> new file mode 100644
> index 000000000000..32d7471c49d0
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/Kconfig
> @@ -0,0 +1,5 @@
> +config DRM_IMX8_DC
> +	tristate "Freescale i.MX8 Display Controller Graphics"
> +	depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST)
> +	help
> +	  enable Freescale i.MX8 Display Controller(DC) graphics support
> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
> new file mode 100644
> index 000000000000..56de82d53d4d
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o
> +
> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o
> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c
> new file mode 100644
> index 000000000000..2c8268b76b08
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-de.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/component.h>
> +#include <linux/container_of.h>
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <drm/drm_managed.h>
> +
> +#include "dc-de.h"
> +#include "dc-drv.h"
> +
> +#define POLARITYCTRL		0xc
> +#define  POLEN_HIGH		BIT(2)
> +
> +struct dc_de_priv {
> +	struct dc_de engine;
> +	void __iomem *reg_top;
> +};
> +
> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de)
> +{
> +	return container_of(de, struct dc_de_priv, engine);
> +}
> +
> +static inline void
> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value)
> +{
> +	struct dc_de_priv *priv = to_de_priv(de);
> +
> +	writel(value, priv->reg_top + offset);

Is there a point in this wrapper? Can you call writel directly? This
question generally applies to the driver. I see a lot of small functions
which can be inlined without losing the clarity.

> +}
> +
> +static void dc_dec_init(struct dc_de *de)
> +{
> +	dc_dec_write(de, POLARITYCTRL, POLEN_HIGH);
> +}
> +
> +static int dc_de_bind(struct device *dev, struct device *master, void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct dc_drm_device *dc_drm = data;
> +	struct dc_de_priv *priv;
> +	int ret;
> +
> +	priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top");
> +	if (IS_ERR(priv->reg_top))
> +		return PTR_ERR(priv->reg_top);
> +
> +	priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload");
> +	if (priv->engine.irq_shdld < 0)
> +		return priv->engine.irq_shdld;
> +
> +	priv->engine.irq_framecomplete =
> +				platform_get_irq_byname(pdev, "framecomplete");
> +	if (priv->engine.irq_framecomplete < 0)
> +		return priv->engine.irq_framecomplete;
> +
> +	priv->engine.irq_seqcomplete =
> +				platform_get_irq_byname(pdev, "seqcomplete");
> +	if (priv->engine.irq_seqcomplete < 0)
> +		return priv->engine.irq_seqcomplete;
> +
> +	priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine");

Is this alias documented somewhere? Is it Acked by DT maintainers?

> +	if (priv->engine.id < 0) {
> +		dev_err(dev, "failed to get alias id: %d\n", priv->engine.id);
> +		return priv->engine.id;
> +	}
> +
> +	priv->engine.dev = dev;
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	dc_drm->de[priv->engine.id] = &priv->engine;
> +
> +	return 0;
> +}
> +
Liu Ying July 30, 2024, 6:25 a.m. UTC | #2
On 07/28/2024, Dmitry Baryshkov wrote:
> On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote:
>> i.MX8qxp Display Controller display engine consists of all processing
>> units that operate in a display clock domain.  Add minimal feature
>> support with FrameGen and TCon so that the engine can output display
>> timings.  The display engine driver as a master binds FrameGen and
>> TCon drivers as components.  While at it, the display engine driver
>> is a component to be bound with the upcoming DRM driver.
> 
> Generic question: why do you need so many small subdrivers? Are they

As we model processing units, interrupt controller, display engine
and pixel engine as devices, relevant drivers are created to bind
them.

Maxime insisted on splitting the main display controller(the overall
IP) into separate devices.  Also, Rob asked me to document every
processing units and the other sub-devices in v2.  So, splitting the
controller is kinda accepted from both DT PoV and DRM PoV.

> used to represent the flexibility of the pipeline? Can you instantiate

No. They are just used to bind the devices created from DT.

> these units directly from the de(?) driver and reference created
> structures without the need to create subdevices?

Given the separated devices created from DT, I can't.

> 
>>
>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>> ---
>> v2:
>> * Use OF alias id to get instance id.
>> * Add dev member to struct dc_tc.
>>
>>  drivers/gpu/drm/imx/Kconfig     |   1 +
>>  drivers/gpu/drm/imx/Makefile    |   1 +
>>  drivers/gpu/drm/imx/dc/Kconfig  |   5 +
>>  drivers/gpu/drm/imx/dc/Makefile |   5 +
>>  drivers/gpu/drm/imx/dc/dc-de.c  | 151 +++++++++++++
>>  drivers/gpu/drm/imx/dc/dc-de.h  |  62 ++++++
>>  drivers/gpu/drm/imx/dc/dc-drv.c |  32 +++
>>  drivers/gpu/drm/imx/dc/dc-drv.h |  24 +++
>>  drivers/gpu/drm/imx/dc/dc-fg.c  | 366 ++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/imx/dc/dc-tc.c  | 137 ++++++++++++
>>  10 files changed, 784 insertions(+)
>>  create mode 100644 drivers/gpu/drm/imx/dc/Kconfig
>>  create mode 100644 drivers/gpu/drm/imx/dc/Makefile
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c
>>
>> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
>> index 03535a15dd8f..3e8c6edbc17c 100644
>> --- a/drivers/gpu/drm/imx/Kconfig
>> +++ b/drivers/gpu/drm/imx/Kconfig
>> @@ -1,5 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  
>> +source "drivers/gpu/drm/imx/dc/Kconfig"
>>  source "drivers/gpu/drm/imx/dcss/Kconfig"
>>  source "drivers/gpu/drm/imx/ipuv3/Kconfig"
>>  source "drivers/gpu/drm/imx/lcdc/Kconfig"
>> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
>> index 86f38e7c7422..c7b317640d71 100644
>> --- a/drivers/gpu/drm/imx/Makefile
>> +++ b/drivers/gpu/drm/imx/Makefile
>> @@ -1,5 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  
>> +obj-$(CONFIG_DRM_IMX8_DC) += dc/
>>  obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
>>  obj-$(CONFIG_DRM_IMX) += ipuv3/
>>  obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
>> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
>> new file mode 100644
>> index 000000000000..32d7471c49d0
>> --- /dev/null
>> +++ b/drivers/gpu/drm/imx/dc/Kconfig
>> @@ -0,0 +1,5 @@
>> +config DRM_IMX8_DC
>> +	tristate "Freescale i.MX8 Display Controller Graphics"
>> +	depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST)
>> +	help
>> +	  enable Freescale i.MX8 Display Controller(DC) graphics support
>> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
>> new file mode 100644
>> index 000000000000..56de82d53d4d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/imx/dc/Makefile
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o
>> +
>> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o
>> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c
>> new file mode 100644
>> index 000000000000..2c8268b76b08
>> --- /dev/null
>> +++ b/drivers/gpu/drm/imx/dc/dc-de.c
>> @@ -0,0 +1,151 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright 2024 NXP
>> + */
>> +
>> +#include <linux/component.h>
>> +#include <linux/container_of.h>
>> +#include <linux/io.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#include <drm/drm_managed.h>
>> +
>> +#include "dc-de.h"
>> +#include "dc-drv.h"
>> +
>> +#define POLARITYCTRL		0xc
>> +#define  POLEN_HIGH		BIT(2)
>> +
>> +struct dc_de_priv {
>> +	struct dc_de engine;
>> +	void __iomem *reg_top;
>> +};
>> +
>> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de)
>> +{
>> +	return container_of(de, struct dc_de_priv, engine);
>> +}
>> +
>> +static inline void
>> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value)
>> +{
>> +	struct dc_de_priv *priv = to_de_priv(de);
>> +
>> +	writel(value, priv->reg_top + offset);
> 
> Is there a point in this wrapper? Can you call writel directly? This

At least, it helps finding read/write ops upon interested devices through
'git grep'.

Also, since we have dc_*_write_mask() helpers, it doesn't look too bad to
have dc_*_read/write() helpers.

> question generally applies to the driver. I see a lot of small functions
> which can be inlined without losing the clarity.

Can you please point out typical ones?

> 
>> +}
>> +
>> +static void dc_dec_init(struct dc_de *de)
>> +{
>> +	dc_dec_write(de, POLARITYCTRL, POLEN_HIGH);
>> +}
>> +
>> +static int dc_de_bind(struct device *dev, struct device *master, void *data)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct dc_drm_device *dc_drm = data;
>> +	struct dc_de_priv *priv;
>> +	int ret;
>> +
>> +	priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top");
>> +	if (IS_ERR(priv->reg_top))
>> +		return PTR_ERR(priv->reg_top);
>> +
>> +	priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload");
>> +	if (priv->engine.irq_shdld < 0)
>> +		return priv->engine.irq_shdld;
>> +
>> +	priv->engine.irq_framecomplete =
>> +				platform_get_irq_byname(pdev, "framecomplete");
>> +	if (priv->engine.irq_framecomplete < 0)
>> +		return priv->engine.irq_framecomplete;
>> +
>> +	priv->engine.irq_seqcomplete =
>> +				platform_get_irq_byname(pdev, "seqcomplete");
>> +	if (priv->engine.irq_seqcomplete < 0)
>> +		return priv->engine.irq_seqcomplete;
>> +
>> +	priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine");
> 
> Is this alias documented somewhere? Is it Acked by DT maintainers?

I see aliases nodes in about 10 .yaml files as examples.
If needed, I can add them to examples.

Rob said "Ideally, no" to use alias in v1. However, IMHO, it is the only
appropriate way to get instance id. In v1 review cycles, we've seen kinda
4 ways:

1) fsl,dc-*-id DT property
   Rejected by Krzystof.

2) OF alias

3) OF graph ports (Rob)
   This doesn't directly get instance id but just tell the connections.
   Since there are too many input/output options between some processing
   units, I hope we don't end up using this approach, as I mentioned in v1.
   It seems be difficult for display driver to handle those ports.   

   VC4 Hardware Video Scaler(HVS) is not using OF graph ports to tell the
   connections to display controllers, either. See brcm,bcm2835-hvs.yaml.
 
4) fsl,imx8qxp-dc-*{id} DT compatible string
   It doesn't seem necessary to add the id information to compatible string.

> 
>> +	if (priv->engine.id < 0) {
>> +		dev_err(dev, "failed to get alias id: %d\n", priv->engine.id);
>> +		return priv->engine.id;
>> +	}
>> +
>> +	priv->engine.dev = dev;
>> +
>> +	dev_set_drvdata(dev, priv);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dc_drm->de[priv->engine.id] = &priv->engine;
>> +
>> +	return 0;
>> +}
>> +
>
Dmitry Baryshkov July 31, 2024, 11:54 a.m. UTC | #3
On Tue, Jul 30, 2024 at 02:25:41PM GMT, Liu Ying wrote:
> On 07/28/2024, Dmitry Baryshkov wrote:
> > On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote:
> >> i.MX8qxp Display Controller display engine consists of all processing
> >> units that operate in a display clock domain.  Add minimal feature
> >> support with FrameGen and TCon so that the engine can output display
> >> timings.  The display engine driver as a master binds FrameGen and
> >> TCon drivers as components.  While at it, the display engine driver
> >> is a component to be bound with the upcoming DRM driver.
> > 
> > Generic question: why do you need so many small subdrivers? Are they
> 
> As we model processing units, interrupt controller, display engine
> and pixel engine as devices, relevant drivers are created to bind
> them.
> 
> Maxime insisted on splitting the main display controller(the overall
> IP) into separate devices.  Also, Rob asked me to document every
> processing units and the other sub-devices in v2.  So, splitting the
> controller is kinda accepted from both DT PoV and DRM PoV.

I went back to the previous series, where Maxime commented on the
"multiple devices glued together". With the split architecture it
becomes even more strange, because now you have a separate IRQ
controller which enumerates interrupts for all subdevices and then glues
them back.

If it was an actually split device, I'd have expected that each of
subdevices has interrupts going to the external controller, without the
glue controller. Or that the glue controller has a limited set of the
externally-generated interrupts that it further splits into per-block
IRQs.

Could you please point out the patches that describe and implement the
<&dc0_irqsteer> device?

> 
> > used to represent the flexibility of the pipeline? Can you instantiate
> 
> No. They are just used to bind the devices created from DT.
> 
> > these units directly from the de(?) driver and reference created
> > structures without the need to create subdevices?
> 
> Given the separated devices created from DT, I can't.

I'd allow Maxime to override me here, but I think that subblocks should
not be described in DT, unless that is required to describe
possible versatility in the display pipeline.

> >>
> >> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> >> ---
> >> v2:
> >> * Use OF alias id to get instance id.
> >> * Add dev member to struct dc_tc.
> >>
> >>  drivers/gpu/drm/imx/Kconfig     |   1 +
> >>  drivers/gpu/drm/imx/Makefile    |   1 +
> >>  drivers/gpu/drm/imx/dc/Kconfig  |   5 +
> >>  drivers/gpu/drm/imx/dc/Makefile |   5 +
> >>  drivers/gpu/drm/imx/dc/dc-de.c  | 151 +++++++++++++
> >>  drivers/gpu/drm/imx/dc/dc-de.h  |  62 ++++++
> >>  drivers/gpu/drm/imx/dc/dc-drv.c |  32 +++
> >>  drivers/gpu/drm/imx/dc/dc-drv.h |  24 +++
> >>  drivers/gpu/drm/imx/dc/dc-fg.c  | 366 ++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/imx/dc/dc-tc.c  | 137 ++++++++++++
> >>  10 files changed, 784 insertions(+)
> >>  create mode 100644 drivers/gpu/drm/imx/dc/Kconfig
> >>  create mode 100644 drivers/gpu/drm/imx/dc/Makefile
> >>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c
> >>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h
> >>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c
> >>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h
> >>  create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c
> >>  create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c
> >>
> >> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
> >> index 03535a15dd8f..3e8c6edbc17c 100644
> >> --- a/drivers/gpu/drm/imx/Kconfig
> >> +++ b/drivers/gpu/drm/imx/Kconfig
> >> @@ -1,5 +1,6 @@
> >>  # SPDX-License-Identifier: GPL-2.0-only
> >>  
> >> +source "drivers/gpu/drm/imx/dc/Kconfig"
> >>  source "drivers/gpu/drm/imx/dcss/Kconfig"
> >>  source "drivers/gpu/drm/imx/ipuv3/Kconfig"
> >>  source "drivers/gpu/drm/imx/lcdc/Kconfig"
> >> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
> >> index 86f38e7c7422..c7b317640d71 100644
> >> --- a/drivers/gpu/drm/imx/Makefile
> >> +++ b/drivers/gpu/drm/imx/Makefile
> >> @@ -1,5 +1,6 @@
> >>  # SPDX-License-Identifier: GPL-2.0
> >>  
> >> +obj-$(CONFIG_DRM_IMX8_DC) += dc/
> >>  obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
> >>  obj-$(CONFIG_DRM_IMX) += ipuv3/
> >>  obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
> >> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
> >> new file mode 100644
> >> index 000000000000..32d7471c49d0
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/imx/dc/Kconfig
> >> @@ -0,0 +1,5 @@
> >> +config DRM_IMX8_DC
> >> +	tristate "Freescale i.MX8 Display Controller Graphics"
> >> +	depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST)
> >> +	help
> >> +	  enable Freescale i.MX8 Display Controller(DC) graphics support
> >> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
> >> new file mode 100644
> >> index 000000000000..56de82d53d4d
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/imx/dc/Makefile
> >> @@ -0,0 +1,5 @@
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +
> >> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o
> >> +
> >> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o
> >> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c
> >> new file mode 100644
> >> index 000000000000..2c8268b76b08
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/imx/dc/dc-de.c
> >> @@ -0,0 +1,151 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/*
> >> + * Copyright 2024 NXP
> >> + */
> >> +
> >> +#include <linux/component.h>
> >> +#include <linux/container_of.h>
> >> +#include <linux/io.h>
> >> +#include <linux/mod_devicetable.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm.h>
> >> +#include <linux/pm_runtime.h>
> >> +
> >> +#include <drm/drm_managed.h>
> >> +
> >> +#include "dc-de.h"
> >> +#include "dc-drv.h"
> >> +
> >> +#define POLARITYCTRL		0xc
> >> +#define  POLEN_HIGH		BIT(2)
> >> +
> >> +struct dc_de_priv {
> >> +	struct dc_de engine;
> >> +	void __iomem *reg_top;
> >> +};
> >> +
> >> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de)
> >> +{
> >> +	return container_of(de, struct dc_de_priv, engine);
> >> +}
> >> +
> >> +static inline void
> >> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value)
> >> +{
> >> +	struct dc_de_priv *priv = to_de_priv(de);
> >> +
> >> +	writel(value, priv->reg_top + offset);
> > 
> > Is there a point in this wrapper? Can you call writel directly? This
> 
> At least, it helps finding read/write ops upon interested devices through
> 'git grep'.

git grep writel also works.

> 
> Also, since we have dc_*_write_mask() helpers, it doesn't look too bad to
> have dc_*_read/write() helpers.

Please use regmap_update_bits instead of dc_*_write_mask.

> 
> > question generally applies to the driver. I see a lot of small functions
> > which can be inlined without losing the clarity.
> 
> Can you please point out typical ones?

dc_fg_enable_shden(), dc_fg_syncmode(), dc_dec_init()

To provide an example, this is the code from dc_crtc_atomic_enable().

	dc_fg_displaymode(dc_crtc->fg, FG_DM_SEC_ON_TOP);
	dc_fg_panic_displaymode(dc_crtc->fg, FG_DM_CONSTCOL);
	dc_fg_cfg_videomode(dc_crtc->fg, adj);

the FG parameters are fixed here. I'd expect a single call from dc_dcrtc
to dc_fg, which internally does all the settings. This removes a need to
export low-level details to other modules.

> 
> > 
> >> +}
> >> +
> >> +static void dc_dec_init(struct dc_de *de)
> >> +{
> >> +	dc_dec_write(de, POLARITYCTRL, POLEN_HIGH);
> >> +}
> >> +
> >> +static int dc_de_bind(struct device *dev, struct device *master, void *data)
> >> +{
> >> +	struct platform_device *pdev = to_platform_device(dev);
> >> +	struct dc_drm_device *dc_drm = data;
> >> +	struct dc_de_priv *priv;
> >> +	int ret;
> >> +
> >> +	priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
> >> +	if (!priv)
> >> +		return -ENOMEM;
> >> +
> >> +	priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top");
> >> +	if (IS_ERR(priv->reg_top))
> >> +		return PTR_ERR(priv->reg_top);
> >> +
> >> +	priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload");
> >> +	if (priv->engine.irq_shdld < 0)
> >> +		return priv->engine.irq_shdld;
> >> +
> >> +	priv->engine.irq_framecomplete =
> >> +				platform_get_irq_byname(pdev, "framecomplete");
> >> +	if (priv->engine.irq_framecomplete < 0)
> >> +		return priv->engine.irq_framecomplete;
> >> +
> >> +	priv->engine.irq_seqcomplete =
> >> +				platform_get_irq_byname(pdev, "seqcomplete");
> >> +	if (priv->engine.irq_seqcomplete < 0)
> >> +		return priv->engine.irq_seqcomplete;
> >> +
> >> +	priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine");
> > 
> > Is this alias documented somewhere? Is it Acked by DT maintainers?
> 
> I see aliases nodes in about 10 .yaml files as examples.
> If needed, I can add them to examples.
> 
> Rob said "Ideally, no" to use alias in v1. However, IMHO, it is the only
> appropriate way to get instance id. In v1 review cycles, we've seen kinda
> 4 ways:
> 
> 1) fsl,dc-*-id DT property
>    Rejected by Krzystof.
> 
> 2) OF alias
> 
> 3) OF graph ports (Rob)
>    This doesn't directly get instance id but just tell the connections.
>    Since there are too many input/output options between some processing
>    units, I hope we don't end up using this approach, as I mentioned in v1.
>    It seems be difficult for display driver to handle those ports.   
> 
>    VC4 Hardware Video Scaler(HVS) is not using OF graph ports to tell the
>    connections to display controllers, either. See brcm,bcm2835-hvs.yaml.
>  
> 4) fsl,imx8qxp-dc-*{id} DT compatible string
>    It doesn't seem necessary to add the id information to compatible string.

For the similar issue we ended up hardcoding IO address / masks into the
driver. This is far from being optimal (and I'd like to get away from
it). If we were designing drm/msm from scratch now, we'd probably have used OF
graph port IDs.

> 
> > 
> >> +	if (priv->engine.id < 0) {
> >> +		dev_err(dev, "failed to get alias id: %d\n", priv->engine.id);
> >> +		return priv->engine.id;
> >> +	}
> >> +
> >> +	priv->engine.dev = dev;
> >> +
> >> +	dev_set_drvdata(dev, priv);
> >> +
> >> +	ret = devm_pm_runtime_enable(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	dc_drm->de[priv->engine.id] = &priv->engine;
> >> +
> >> +	return 0;
> >> +}
> >> +
> > 
> 
> -- 
> Regards,
> Liu Ying
>
Liu Ying Aug. 5, 2024, 3:23 a.m. UTC | #4
On 07/31/2024, Dmitry Baryshkov wrote:
> On Tue, Jul 30, 2024 at 02:25:41PM GMT, Liu Ying wrote:
>> On 07/28/2024, Dmitry Baryshkov wrote:
>>> On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote:
>>>> i.MX8qxp Display Controller display engine consists of all processing
>>>> units that operate in a display clock domain.  Add minimal feature
>>>> support with FrameGen and TCon so that the engine can output display
>>>> timings.  The display engine driver as a master binds FrameGen and
>>>> TCon drivers as components.  While at it, the display engine driver
>>>> is a component to be bound with the upcoming DRM driver.
>>>
>>> Generic question: why do you need so many small subdrivers? Are they
>>
>> As we model processing units, interrupt controller, display engine
>> and pixel engine as devices, relevant drivers are created to bind
>> them.
>>
>> Maxime insisted on splitting the main display controller(the overall
>> IP) into separate devices.  Also, Rob asked me to document every
>> processing units and the other sub-devices in v2.  So, splitting the
>> controller is kinda accepted from both DT PoV and DRM PoV.
> 
> I went back to the previous series, where Maxime commented on the
> "multiple devices glued together". With the split architecture it
> becomes even more strange, because now you have a separate IRQ
> controller which enumerates interrupts for all subdevices and then glues
> them back.
> 
> If it was an actually split device, I'd have expected that each of
> subdevices has interrupts going to the external controller, without the
> glue controller. Or that the glue controller has a limited set of the
> externally-generated interrupts that it further splits into per-block
> IRQs.

People may have different opinions on whether the main display controller
should be split into sub-devices or not, or even how it should be split,
which looks quite subjective.  Given this situation, I tend to follow
the kind of agreement reached by Rob and Maxime that it should be split
entirely and each processing unit should have a dt-binding doc.

> 
> Could you please point out the patches that describe and implement the
> <&dc0_irqsteer> device?

drivers/irqchip/irq-imx-irqsteer.c
Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.yaml

You may find an irqsteer DT node in patch 13/16 in v2.

> 
>>
>>> used to represent the flexibility of the pipeline? Can you instantiate
>>
>> No. They are just used to bind the devices created from DT.
>>
>>> these units directly from the de(?) driver and reference created
>>> structures without the need to create subdevices?
>>
>> Given the separated devices created from DT, I can't.
> 
> I'd allow Maxime to override me here, but I think that subblocks should
> not be described in DT, unless that is required to describe
> possible versatility in the display pipeline.
> 
>>>>
>>>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>>>> ---
>>>> v2:
>>>> * Use OF alias id to get instance id.
>>>> * Add dev member to struct dc_tc.
>>>>
>>>>  drivers/gpu/drm/imx/Kconfig     |   1 +
>>>>  drivers/gpu/drm/imx/Makefile    |   1 +
>>>>  drivers/gpu/drm/imx/dc/Kconfig  |   5 +
>>>>  drivers/gpu/drm/imx/dc/Makefile |   5 +
>>>>  drivers/gpu/drm/imx/dc/dc-de.c  | 151 +++++++++++++
>>>>  drivers/gpu/drm/imx/dc/dc-de.h  |  62 ++++++
>>>>  drivers/gpu/drm/imx/dc/dc-drv.c |  32 +++
>>>>  drivers/gpu/drm/imx/dc/dc-drv.h |  24 +++
>>>>  drivers/gpu/drm/imx/dc/dc-fg.c  | 366 ++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/imx/dc/dc-tc.c  | 137 ++++++++++++
>>>>  10 files changed, 784 insertions(+)
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/Kconfig
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/Makefile
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c
>>>>
>>>> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
>>>> index 03535a15dd8f..3e8c6edbc17c 100644
>>>> --- a/drivers/gpu/drm/imx/Kconfig
>>>> +++ b/drivers/gpu/drm/imx/Kconfig
>>>> @@ -1,5 +1,6 @@
>>>>  # SPDX-License-Identifier: GPL-2.0-only
>>>>  
>>>> +source "drivers/gpu/drm/imx/dc/Kconfig"
>>>>  source "drivers/gpu/drm/imx/dcss/Kconfig"
>>>>  source "drivers/gpu/drm/imx/ipuv3/Kconfig"
>>>>  source "drivers/gpu/drm/imx/lcdc/Kconfig"
>>>> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
>>>> index 86f38e7c7422..c7b317640d71 100644
>>>> --- a/drivers/gpu/drm/imx/Makefile
>>>> +++ b/drivers/gpu/drm/imx/Makefile
>>>> @@ -1,5 +1,6 @@
>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>  
>>>> +obj-$(CONFIG_DRM_IMX8_DC) += dc/
>>>>  obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
>>>>  obj-$(CONFIG_DRM_IMX) += ipuv3/
>>>>  obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
>>>> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
>>>> new file mode 100644
>>>> index 000000000000..32d7471c49d0
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/imx/dc/Kconfig
>>>> @@ -0,0 +1,5 @@
>>>> +config DRM_IMX8_DC
>>>> +	tristate "Freescale i.MX8 Display Controller Graphics"
>>>> +	depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST)
>>>> +	help
>>>> +	  enable Freescale i.MX8 Display Controller(DC) graphics support
>>>> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
>>>> new file mode 100644
>>>> index 000000000000..56de82d53d4d
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/imx/dc/Makefile
>>>> @@ -0,0 +1,5 @@
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o
>>>> +
>>>> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o
>>>> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c
>>>> new file mode 100644
>>>> index 000000000000..2c8268b76b08
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/imx/dc/dc-de.c
>>>> @@ -0,0 +1,151 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Copyright 2024 NXP
>>>> + */
>>>> +
>>>> +#include <linux/component.h>
>>>> +#include <linux/container_of.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_platform.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pm.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +
>>>> +#include <drm/drm_managed.h>
>>>> +
>>>> +#include "dc-de.h"
>>>> +#include "dc-drv.h"
>>>> +
>>>> +#define POLARITYCTRL		0xc
>>>> +#define  POLEN_HIGH		BIT(2)
>>>> +
>>>> +struct dc_de_priv {
>>>> +	struct dc_de engine;
>>>> +	void __iomem *reg_top;
>>>> +};
>>>> +
>>>> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de)
>>>> +{
>>>> +	return container_of(de, struct dc_de_priv, engine);
>>>> +}
>>>> +
>>>> +static inline void
>>>> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value)
>>>> +{
>>>> +	struct dc_de_priv *priv = to_de_priv(de);
>>>> +
>>>> +	writel(value, priv->reg_top + offset);
>>>
>>> Is there a point in this wrapper? Can you call writel directly? This
>>
>> At least, it helps finding read/write ops upon interested devices through
>> 'git grep'.
> 
> git grep writel also works.

I said "interested devices".  For example, write ops upon LayerBlend can
be easily found by 'git grep dc_lb_write'.  'git grep writel' is not enough
to tell interested device.

> 
>>
>> Also, since we have dc_*_write_mask() helpers, it doesn't look too bad to
>> have dc_*_read/write() helpers.
> 
> Please use regmap_update_bits instead of dc_*_write_mask.

Then, you are suggesting to use both regmap_update_bits and writel.
This doesn't look very consistent.  Why not use regmap_write and
regmap_update_bits?

Anyway, this is a bit bike-shedding.  If you don't have too strong opinion
on this, I'd hope to keep the read/write ops as-is.

> 
>>
>>> question generally applies to the driver. I see a lot of small functions
>>> which can be inlined without losing the clarity.
>>
>> Can you please point out typical ones?
> 
> dc_fg_enable_shden(), dc_fg_syncmode(), dc_dec_init()

I'll inline them and some others.

> 
> To provide an example, this is the code from dc_crtc_atomic_enable().
> 
> 	dc_fg_displaymode(dc_crtc->fg, FG_DM_SEC_ON_TOP);
> 	dc_fg_panic_displaymode(dc_crtc->fg, FG_DM_CONSTCOL);
> 	dc_fg_cfg_videomode(dc_crtc->fg, adj);
> 
> the FG parameters are fixed here. I'd expect a single call from dc_dcrtc
> to dc_fg, which internally does all the settings. This removes a need to
> export low-level details to other modules.

The display modes set by dc_fg_displaymode() and dc_fg_panic_displaymode()
are kinda key settings for KMS.  IMHO, setting them in ->atomic_enable()
makes maintenance and code reading a bit easier though with trivial and
neglectable performance penalty since they are done for each mode setting.

Anyway, since you asked, I'll move the two to dc_fg_init() and move
some others where apply.

> 
>>
>>>
>>>> +}
>>>> +
>>>> +static void dc_dec_init(struct dc_de *de)
>>>> +{
>>>> +	dc_dec_write(de, POLARITYCTRL, POLEN_HIGH);
>>>> +}
>>>> +
>>>> +static int dc_de_bind(struct device *dev, struct device *master, void *data)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct dc_drm_device *dc_drm = data;
>>>> +	struct dc_de_priv *priv;
>>>> +	int ret;
>>>> +
>>>> +	priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top");
>>>> +	if (IS_ERR(priv->reg_top))
>>>> +		return PTR_ERR(priv->reg_top);
>>>> +
>>>> +	priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload");
>>>> +	if (priv->engine.irq_shdld < 0)
>>>> +		return priv->engine.irq_shdld;
>>>> +
>>>> +	priv->engine.irq_framecomplete =
>>>> +				platform_get_irq_byname(pdev, "framecomplete");
>>>> +	if (priv->engine.irq_framecomplete < 0)
>>>> +		return priv->engine.irq_framecomplete;
>>>> +
>>>> +	priv->engine.irq_seqcomplete =
>>>> +				platform_get_irq_byname(pdev, "seqcomplete");
>>>> +	if (priv->engine.irq_seqcomplete < 0)
>>>> +		return priv->engine.irq_seqcomplete;
>>>> +
>>>> +	priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine");
>>>
>>> Is this alias documented somewhere? Is it Acked by DT maintainers?
>>
>> I see aliases nodes in about 10 .yaml files as examples.
>> If needed, I can add them to examples.
>>
>> Rob said "Ideally, no" to use alias in v1. However, IMHO, it is the only
>> appropriate way to get instance id. In v1 review cycles, we've seen kinda
>> 4 ways:
>>
>> 1) fsl,dc-*-id DT property
>>    Rejected by Krzystof.
>>
>> 2) OF alias
>>
>> 3) OF graph ports (Rob)
>>    This doesn't directly get instance id but just tell the connections.
>>    Since there are too many input/output options between some processing
>>    units, I hope we don't end up using this approach, as I mentioned in v1.
>>    It seems be difficult for display driver to handle those ports.   
>>
>>    VC4 Hardware Video Scaler(HVS) is not using OF graph ports to tell the
>>    connections to display controllers, either. See brcm,bcm2835-hvs.yaml.
>>  
>> 4) fsl,imx8qxp-dc-*{id} DT compatible string
>>    It doesn't seem necessary to add the id information to compatible string.
> 
> For the similar issue we ended up hardcoding IO address / masks into the
> driver. This is far from being optimal (and I'd like to get away from

I thought about using IO address to tell instance id in driver before v1,
and chose not to do that since it's not straightforward and kinda abusing IO
address.

> it). If we were designing drm/msm from scratch now, we'd probably have used OF
> graph port IDs.

Don't know drm/msm and it's backing HW(s), so not sure how OF graph ports
can bring benefits for it.  For i.MX8 DC, it will be too complex for DT and
display driver to use OF graph ports. 

> 
>>
>>>
>>>> +	if (priv->engine.id < 0) {
>>>> +		dev_err(dev, "failed to get alias id: %d\n", priv->engine.id);
>>>> +		return priv->engine.id;
>>>> +	}
>>>> +
>>>> +	priv->engine.dev = dev;
>>>> +
>>>> +	dev_set_drvdata(dev, priv);
>>>> +
>>>> +	ret = devm_pm_runtime_enable(dev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	dc_drm->de[priv->engine.id] = &priv->engine;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>
>>
>> -- 
>> Regards,
>> Liu Ying
>>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
index 03535a15dd8f..3e8c6edbc17c 100644
--- a/drivers/gpu/drm/imx/Kconfig
+++ b/drivers/gpu/drm/imx/Kconfig
@@ -1,5 +1,6 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 
+source "drivers/gpu/drm/imx/dc/Kconfig"
 source "drivers/gpu/drm/imx/dcss/Kconfig"
 source "drivers/gpu/drm/imx/ipuv3/Kconfig"
 source "drivers/gpu/drm/imx/lcdc/Kconfig"
diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
index 86f38e7c7422..c7b317640d71 100644
--- a/drivers/gpu/drm/imx/Makefile
+++ b/drivers/gpu/drm/imx/Makefile
@@ -1,5 +1,6 @@ 
 # SPDX-License-Identifier: GPL-2.0
 
+obj-$(CONFIG_DRM_IMX8_DC) += dc/
 obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
 obj-$(CONFIG_DRM_IMX) += ipuv3/
 obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
new file mode 100644
index 000000000000..32d7471c49d0
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/Kconfig
@@ -0,0 +1,5 @@ 
+config DRM_IMX8_DC
+	tristate "Freescale i.MX8 Display Controller Graphics"
+	depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST)
+	help
+	  enable Freescale i.MX8 Display Controller(DC) graphics support
diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
new file mode 100644
index 000000000000..56de82d53d4d
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/Makefile
@@ -0,0 +1,5 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o
+
+obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o
diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c
new file mode 100644
index 000000000000..2c8268b76b08
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-de.c
@@ -0,0 +1,151 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/component.h>
+#include <linux/container_of.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+
+#include <drm/drm_managed.h>
+
+#include "dc-de.h"
+#include "dc-drv.h"
+
+#define POLARITYCTRL		0xc
+#define  POLEN_HIGH		BIT(2)
+
+struct dc_de_priv {
+	struct dc_de engine;
+	void __iomem *reg_top;
+};
+
+static inline struct dc_de_priv *to_de_priv(struct dc_de *de)
+{
+	return container_of(de, struct dc_de_priv, engine);
+}
+
+static inline void
+dc_dec_write(struct dc_de *de, unsigned int offset, u32 value)
+{
+	struct dc_de_priv *priv = to_de_priv(de);
+
+	writel(value, priv->reg_top + offset);
+}
+
+static void dc_dec_init(struct dc_de *de)
+{
+	dc_dec_write(de, POLARITYCTRL, POLEN_HIGH);
+}
+
+static int dc_de_bind(struct device *dev, struct device *master, void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dc_drm_device *dc_drm = data;
+	struct dc_de_priv *priv;
+	int ret;
+
+	priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top");
+	if (IS_ERR(priv->reg_top))
+		return PTR_ERR(priv->reg_top);
+
+	priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload");
+	if (priv->engine.irq_shdld < 0)
+		return priv->engine.irq_shdld;
+
+	priv->engine.irq_framecomplete =
+				platform_get_irq_byname(pdev, "framecomplete");
+	if (priv->engine.irq_framecomplete < 0)
+		return priv->engine.irq_framecomplete;
+
+	priv->engine.irq_seqcomplete =
+				platform_get_irq_byname(pdev, "seqcomplete");
+	if (priv->engine.irq_seqcomplete < 0)
+		return priv->engine.irq_seqcomplete;
+
+	priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine");
+	if (priv->engine.id < 0) {
+		dev_err(dev, "failed to get alias id: %d\n", priv->engine.id);
+		return priv->engine.id;
+	}
+
+	priv->engine.dev = dev;
+
+	dev_set_drvdata(dev, priv);
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	dc_drm->de[priv->engine.id] = &priv->engine;
+
+	return 0;
+}
+
+static const struct component_ops dc_de_ops = {
+	.bind = dc_de_bind,
+};
+
+static int dc_de_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	ret = devm_of_platform_populate(&pdev->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = component_add(&pdev->dev, &dc_de_ops);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to add component\n");
+
+	return 0;
+}
+
+static void dc_de_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &dc_de_ops);
+}
+
+static int dc_de_runtime_resume(struct device *dev)
+{
+	struct dc_de_priv *priv = dev_get_drvdata(dev);
+	struct dc_de *engine = &priv->engine;
+
+	dc_dec_init(engine);
+	dc_fg_init(engine->fg);
+	dc_tc_init(engine->tc);
+
+	return 0;
+}
+
+static const struct dev_pm_ops dc_de_pm_ops = {
+	RUNTIME_PM_OPS(NULL, dc_de_runtime_resume, NULL)
+};
+
+static const struct of_device_id dc_de_dt_ids[] = {
+	{ .compatible = "fsl,imx8qxp-dc-display-engine", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, dc_de_dt_ids);
+
+struct platform_driver dc_de_driver = {
+	.probe = dc_de_probe,
+	.remove_new = dc_de_remove,
+	.driver = {
+		.name = "imx8-dc-display-engine",
+		.of_match_table = dc_de_dt_ids,
+		.pm = pm_sleep_ptr(&dc_de_pm_ops),
+	},
+};
diff --git a/drivers/gpu/drm/imx/dc/dc-de.h b/drivers/gpu/drm/imx/dc/dc-de.h
new file mode 100644
index 000000000000..3417059c40b9
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-de.h
@@ -0,0 +1,62 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2024 NXP
+ */
+
+#ifndef __DC_DISPLAY_ENGINE_H__
+#define __DC_DISPLAY_ENGINE_H__
+
+#include <linux/device.h>
+#include <drm/drm_modes.h>
+
+#define DC_DISPLAYS	2
+
+enum dc_fg_syncmode {
+	FG_SYNCMODE_OFF,	/* No side-by-side synchronization. */
+};
+
+enum dc_fg_dm {
+	FG_DM_CONSTCOL = 0x1,	/* Constant Color Background is shown. */
+	FG_DM_SEC_ON_TOP = 0x5,	/* Both inputs overlaid with secondary on top. */
+};
+
+struct dc_fg {
+};
+
+struct dc_tc {
+	struct device *dev;
+};
+
+struct dc_de {
+	struct device *dev;
+	struct dc_fg *fg;
+	struct dc_tc *tc;
+	int irq_shdld;
+	int irq_framecomplete;
+	int irq_seqcomplete;
+	int id;
+};
+
+/* Frame Generator Unit */
+void dc_fg_syncmode(struct dc_fg *fg, enum dc_fg_syncmode mode);
+void dc_fg_cfg_videomode(struct dc_fg *fg, struct drm_display_mode *m);
+void dc_fg_displaymode(struct dc_fg *fg, enum dc_fg_dm mode);
+void dc_fg_panic_displaymode(struct dc_fg *fg, enum dc_fg_dm mode);
+void dc_fg_enable(struct dc_fg *fg);
+void dc_fg_disable(struct dc_fg *fg);
+void dc_fg_shdtokgen(struct dc_fg *fg);
+u32 dc_fg_get_frame_index(struct dc_fg *fg);
+int dc_fg_get_line_index(struct dc_fg *fg);
+bool dc_fg_wait_for_frame_index_moving(struct dc_fg *fg);
+bool dc_fg_secondary_requests_to_read_empty_fifo(struct dc_fg *fg);
+void dc_fg_secondary_clear_channel_status(struct dc_fg *fg);
+int dc_fg_wait_for_secondary_syncup(struct dc_fg *fg);
+void dc_fg_enable_clock(struct dc_fg *fg);
+void dc_fg_disable_clock(struct dc_fg *fg);
+enum drm_mode_status dc_fg_check_clock(struct dc_fg *fg, int clk_khz);
+void dc_fg_init(struct dc_fg *fg);
+
+/* Timing Controller Unit */
+void dc_tc_init(struct dc_tc *tc);
+
+#endif /* __DC_DISPLAY_ENGINE_H__ */
diff --git a/drivers/gpu/drm/imx/dc/dc-drv.c b/drivers/gpu/drm/imx/dc/dc-drv.c
new file mode 100644
index 000000000000..e5910a82dd4d
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-drv.c
@@ -0,0 +1,32 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "dc-drv.h"
+
+static struct platform_driver * const dc_drivers[] = {
+	&dc_de_driver,
+	&dc_fg_driver,
+	&dc_tc_driver,
+};
+
+static int __init dc_drm_init(void)
+{
+	return platform_register_drivers(dc_drivers, ARRAY_SIZE(dc_drivers));
+}
+
+static void __exit dc_drm_exit(void)
+{
+	platform_unregister_drivers(dc_drivers, ARRAY_SIZE(dc_drivers));
+}
+
+module_init(dc_drm_init);
+module_exit(dc_drm_exit);
+
+MODULE_DESCRIPTION("i.MX8 Display Controller DRM Driver");
+MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/imx/dc/dc-drv.h b/drivers/gpu/drm/imx/dc/dc-drv.h
new file mode 100644
index 000000000000..e1290d9a0a99
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-drv.h
@@ -0,0 +1,24 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2024 NXP
+ */
+
+#ifndef __DC_DRV_H__
+#define __DC_DRV_H__
+
+#include <linux/platform_device.h>
+
+#include <drm/drm_device.h>
+
+#include "dc-de.h"
+
+struct dc_drm_device {
+	struct drm_device base;
+	struct dc_de *de[DC_DISPLAYS];
+};
+
+extern struct platform_driver dc_de_driver;
+extern struct platform_driver dc_fg_driver;
+extern struct platform_driver dc_tc_driver;
+
+#endif /* __DC_DRV_H__ */
diff --git a/drivers/gpu/drm/imx/dc/dc-fg.c b/drivers/gpu/drm/imx/dc/dc-fg.c
new file mode 100644
index 000000000000..3e9a8abee93e
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-fg.c
@@ -0,0 +1,366 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/jiffies.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_managed.h>
+#include <drm/drm_modes.h>
+
+#include "dc-de.h"
+#include "dc-drv.h"
+
+#define FGSTCTRL		0x8
+#define  FGSYNCMODE_MASK	0x6
+#define  FGSYNCMODE(n)		((n) << 6)
+#define  SHDEN			BIT(0)
+
+#define HTCFG1			0xc
+#define  HTOTAL(n)		((((n) - 1) & 0x3fff) << 16)
+#define  HACT(n)		((n) & 0x3fff)
+
+#define HTCFG2			0x10
+#define  HSEN			BIT(31)
+#define  HSBP(n)		((((n) - 1) & 0x3fff) << 16)
+#define  HSYNC(n)		(((n) - 1) & 0x3fff)
+
+#define VTCFG1			0x14
+#define  VTOTAL(n)		((((n) - 1) & 0x3fff) << 16)
+#define  VACT(n)		((n) & 0x3fff)
+
+#define VTCFG2			0x18
+#define  VSEN			BIT(31)
+#define  VSBP(n)		((((n) - 1) & 0x3fff) << 16)
+#define  VSYNC(n)		(((n) - 1) & 0x3fff)
+
+#define PKICKCONFIG		0x2c
+#define SKICKCONFIG		0x30
+#define  EN			BIT(31)
+#define  ROW(n)			(((n) & 0x3fff) << 16)
+#define  COL(n)			((n) & 0x3fff)
+
+#define PACFG			0x54
+#define SACFG			0x58
+#define  STARTX(n)		(((n) + 1) & 0x3fff)
+#define  STARTY(n)		(((((n) + 1) & 0x3fff)) << 16)
+
+#define FGINCTRL		0x5c
+#define FGINCTRLPANIC		0x60
+#define  FGDM_MASK		0x7
+#define  ENPRIMALPHA		BIT(3)
+#define  ENSECALPHA		BIT(4)
+
+#define FGCCR			0x64
+#define  CCGREEN(g)		(((g) & 0x3ff) << 10)
+
+#define FGENABLE		0x68
+#define  FGEN			BIT(0)
+
+#define FGSLR			0x6c
+#define  SHDTOKGEN		BIT(0)
+
+#define FGTIMESTAMP		0x74
+#define  FRAMEINDEX_SHIFT	14
+#define  FRAMEINDEX_MASK	(0x3ffff << FRAMEINDEX_SHIFT)
+#define  LINEINDEX_MASK		0x3fff
+
+#define FGCHSTAT		0x78
+#define  SECSYNCSTAT		BIT(24)
+#define  SFIFOEMPTY		BIT(16)
+
+#define FGCHSTATCLR		0x7c
+#define  CLRSECSTAT		BIT(16)
+
+#define KHZ			1000
+
+struct dc_fg_priv {
+	struct dc_fg fg;
+	struct device *dev;
+	void __iomem *reg;
+	struct clk *clk_disp;
+};
+
+static inline struct dc_fg_priv *to_fg_priv(struct dc_fg *fg)
+{
+	return container_of(fg, struct dc_fg_priv, fg);
+}
+
+static inline u32 dc_fg_read(struct dc_fg *fg, unsigned int offset)
+{
+	struct dc_fg_priv *priv = to_fg_priv(fg);
+
+	return readl(priv->reg + offset);
+}
+
+static inline void
+dc_fg_write(struct dc_fg *fg, unsigned int offset, u32 value)
+{
+	struct dc_fg_priv *priv = to_fg_priv(fg);
+
+	writel(value, priv->reg + offset);
+}
+
+static inline void
+dc_fg_write_mask(struct dc_fg *fg, unsigned int offset, u32 mask, u32 value)
+{
+	u32 tmp;
+
+	tmp = dc_fg_read(fg, offset);
+	tmp &= ~mask;
+	dc_fg_write(fg, offset, tmp | value);
+}
+
+static void dc_fg_enable_shden(struct dc_fg *fg)
+{
+	dc_fg_write_mask(fg, FGSTCTRL, SHDEN, SHDEN);
+}
+
+void dc_fg_syncmode(struct dc_fg *fg, enum dc_fg_syncmode mode)
+{
+	dc_fg_write_mask(fg, FGSTCTRL, FGSYNCMODE_MASK, FGSYNCMODE(mode));
+}
+
+void dc_fg_cfg_videomode(struct dc_fg *fg, struct drm_display_mode *m)
+{
+	struct dc_fg_priv *priv = to_fg_priv(fg);
+	u32 hact, htotal, hsync, hsbp;
+	u32 vact, vtotal, vsync, vsbp;
+	u32 kick_row, kick_col;
+	int ret;
+
+	hact = m->crtc_hdisplay;
+	htotal = m->crtc_htotal;
+	hsync = m->crtc_hsync_end - m->crtc_hsync_start;
+	hsbp = m->crtc_htotal - m->crtc_hsync_start;
+
+	vact = m->crtc_vdisplay;
+	vtotal = m->crtc_vtotal;
+	vsync = m->crtc_vsync_end - m->crtc_vsync_start;
+	vsbp = m->crtc_vtotal - m->crtc_vsync_start;
+
+	/* video mode */
+	dc_fg_write(fg, HTCFG1, HACT(hact)   | HTOTAL(htotal));
+	dc_fg_write(fg, HTCFG2, HSYNC(hsync) | HSBP(hsbp) | HSEN);
+	dc_fg_write(fg, VTCFG1, VACT(vact)   | VTOTAL(vtotal));
+	dc_fg_write(fg, VTCFG2, VSYNC(vsync) | VSBP(vsbp) | VSEN);
+
+	kick_col = hact + 1;
+	kick_row = vact;
+
+	/* pkickconfig */
+	dc_fg_write(fg, PKICKCONFIG, COL(kick_col) | ROW(kick_row) | EN);
+
+	/* skikconfig */
+	dc_fg_write(fg, SKICKCONFIG, COL(kick_col) | ROW(kick_row) | EN);
+
+	/* primary and secondary area position configuration */
+	dc_fg_write(fg, PACFG, STARTX(0) | STARTY(0));
+	dc_fg_write(fg, SACFG, STARTX(0) | STARTY(0));
+
+	/* alpha */
+	dc_fg_write_mask(fg, FGINCTRL,      ENPRIMALPHA | ENSECALPHA, 0);
+	dc_fg_write_mask(fg, FGINCTRLPANIC, ENPRIMALPHA | ENSECALPHA, 0);
+
+	/* constant color is green(used in panic mode)  */
+	dc_fg_write(fg, FGCCR, CCGREEN(0x3ff));
+
+	ret = clk_set_rate(priv->clk_disp, m->clock * KHZ);
+	if (ret < 0)
+		dev_err(priv->dev,
+			"failed to set display clock rate: %d\n", ret);
+}
+
+void dc_fg_displaymode(struct dc_fg *fg, enum dc_fg_dm mode)
+{
+	dc_fg_write_mask(fg, FGINCTRL, FGDM_MASK, mode);
+}
+
+void dc_fg_panic_displaymode(struct dc_fg *fg, enum dc_fg_dm mode)
+{
+	dc_fg_write_mask(fg, FGINCTRLPANIC, FGDM_MASK, mode);
+}
+
+void dc_fg_enable(struct dc_fg *fg)
+{
+	dc_fg_write(fg, FGENABLE, FGEN);
+}
+
+void dc_fg_disable(struct dc_fg *fg)
+{
+	dc_fg_write(fg, FGENABLE, 0);
+}
+
+void dc_fg_shdtokgen(struct dc_fg *fg)
+{
+	dc_fg_write(fg, FGSLR, SHDTOKGEN);
+}
+
+u32 dc_fg_get_frame_index(struct dc_fg *fg)
+{
+	u32 val = dc_fg_read(fg, FGTIMESTAMP);
+
+	return (val & FRAMEINDEX_MASK) >> FRAMEINDEX_SHIFT;
+}
+
+int dc_fg_get_line_index(struct dc_fg *fg)
+{
+	u32 val = dc_fg_read(fg, FGTIMESTAMP);
+
+	return val & LINEINDEX_MASK;
+}
+
+bool dc_fg_wait_for_frame_index_moving(struct dc_fg *fg)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(100);
+	u32 frame_index, last_frame_index;
+
+	frame_index = dc_fg_get_frame_index(fg);
+	do {
+		last_frame_index = frame_index;
+		frame_index = dc_fg_get_frame_index(fg);
+	} while (last_frame_index == frame_index &&
+		 time_before(jiffies, timeout));
+
+	return last_frame_index != frame_index;
+}
+
+bool dc_fg_secondary_requests_to_read_empty_fifo(struct dc_fg *fg)
+{
+	u32 val;
+
+	val = dc_fg_read(fg, FGCHSTAT);
+
+	return !!(val & SFIFOEMPTY);
+}
+
+void dc_fg_secondary_clear_channel_status(struct dc_fg *fg)
+{
+	dc_fg_write(fg, FGCHSTATCLR, CLRSECSTAT);
+}
+
+int dc_fg_wait_for_secondary_syncup(struct dc_fg *fg)
+{
+	struct dc_fg_priv *priv = to_fg_priv(fg);
+	u32 val;
+
+	return readl_poll_timeout(priv->reg + FGCHSTAT, val,
+				  val & SECSYNCSTAT, 5, 100000);
+}
+
+void dc_fg_enable_clock(struct dc_fg *fg)
+{
+	struct dc_fg_priv *priv = to_fg_priv(fg);
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk_disp);
+	if (ret)
+		dev_err(priv->dev, "failed to enable display clock: %d\n", ret);
+}
+
+void dc_fg_disable_clock(struct dc_fg *fg)
+{
+	struct dc_fg_priv *priv = to_fg_priv(fg);
+
+	clk_disable_unprepare(priv->clk_disp);
+}
+
+enum drm_mode_status dc_fg_check_clock(struct dc_fg *fg, int clk_khz)
+{
+	struct dc_fg_priv *priv = to_fg_priv(fg);
+	unsigned long rounded_rate;
+
+	rounded_rate = clk_round_rate(priv->clk_disp, clk_khz * KHZ);
+
+	if (rounded_rate != clk_khz * KHZ)
+		return MODE_NOCLOCK;
+
+	return MODE_OK;
+}
+
+void dc_fg_init(struct dc_fg *fg)
+{
+	dc_fg_enable_shden(fg);
+	dc_fg_syncmode(fg, FG_SYNCMODE_OFF);
+}
+
+static int dc_fg_bind(struct device *dev, struct device *master, void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dc_drm_device *dc_drm = data;
+	struct dc_fg_priv *priv;
+	struct dc_de *de;
+	int id;
+
+	priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->reg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->reg))
+		return PTR_ERR(priv->reg);
+
+	priv->clk_disp = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk_disp))
+		return dev_err_probe(dev, PTR_ERR(priv->clk_disp),
+				     "failed to get display clock\n");
+
+	id = of_alias_get_id(dev->of_node, "dc0-framegen");
+	if (id < 0) {
+		dev_err(dev, "failed to get alias id: %d\n", id);
+		return id;
+	}
+
+	priv->dev = dev;
+
+	de = dc_drm->de[id];
+	de->fg = &priv->fg;
+
+	return 0;
+}
+
+static const struct component_ops dc_fg_ops = {
+	.bind = dc_fg_bind,
+};
+
+static int dc_fg_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	ret = component_add(&pdev->dev, &dc_fg_ops);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to add component\n");
+
+	return 0;
+}
+
+static void dc_fg_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &dc_fg_ops);
+}
+
+static const struct of_device_id dc_fg_dt_ids[] = {
+	{ .compatible = "fsl,imx8qxp-dc-framegen", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, dc_fg_dt_ids);
+
+struct platform_driver dc_fg_driver = {
+	.probe = dc_fg_probe,
+	.remove_new = dc_fg_remove,
+	.driver = {
+		.name = "imx8-dc-framegen",
+		.of_match_table = dc_fg_dt_ids,
+	},
+};
diff --git a/drivers/gpu/drm/imx/dc/dc-tc.c b/drivers/gpu/drm/imx/dc/dc-tc.c
new file mode 100644
index 000000000000..29f42496a409
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-tc.c
@@ -0,0 +1,137 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/component.h>
+#include <linux/container_of.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_managed.h>
+
+#include "dc-drv.h"
+#include "dc-de.h"
+
+#define TCON_CTRL	0x410
+#define  CTRL_RST_VAL	0x01401408
+
+/* red: MAPBIT 29-20, green: MAPBIT 19-10, blue: MAPBIT 9-0 */
+#define MAPBIT3_0	0x418
+#define MAPBIT7_4	0x41c
+#define MAPBIT11_8	0x420
+#define MAPBIT15_12	0x424
+#define MAPBIT19_16	0x428
+#define MAPBIT23_20	0x42c
+#define MAPBIT27_24	0x430
+#define MAPBIT31_28	0x434
+#define MAPBIT34_32	0x438
+
+struct dc_tc_priv {
+	struct dc_tc tc;
+	void __iomem *reg;
+};
+
+static inline struct dc_tc_priv *to_tc_priv(struct dc_tc *tc)
+{
+	return container_of(tc, struct dc_tc_priv, tc);
+}
+
+static inline void dc_tc_write(struct dc_tc *tc, unsigned int offset, u32 value)
+{
+	struct dc_tc_priv *priv = to_tc_priv(tc);
+
+	writel(value, priv->reg + offset);
+}
+
+static void dc_tc_set_fmt(struct dc_tc *tc)
+{
+	/*
+	 * The pixels reach TCON are always in 30-bit BGR format.
+	 * The first bridge always receives pixels in 30-bit RGB format.
+	 * So, map the format to MEDIA_BUS_FMT_RGB101010_1X30.
+	 */
+	dc_tc_write(tc, MAPBIT3_0,   0x17161514);
+	dc_tc_write(tc, MAPBIT7_4,   0x1b1a1918);
+	dc_tc_write(tc, MAPBIT11_8,  0x0b0a1d1c);
+	dc_tc_write(tc, MAPBIT15_12, 0x0f0e0d0c);
+	dc_tc_write(tc, MAPBIT19_16, 0x13121110);
+	dc_tc_write(tc, MAPBIT23_20, 0x03020100);
+	dc_tc_write(tc, MAPBIT27_24, 0x07060504);
+	dc_tc_write(tc, MAPBIT31_28, 0x00000908);
+}
+
+void dc_tc_init(struct dc_tc *tc)
+{
+	/* reset TCON_CTRL to POR default so that TCON works in bypass mode */
+	dc_tc_write(tc, TCON_CTRL, CTRL_RST_VAL);
+	dc_tc_set_fmt(tc);
+}
+
+static int dc_tc_bind(struct device *dev, struct device *master, void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dc_drm_device *dc_drm = data;
+	struct dc_tc_priv *priv;
+	struct dc_de *de;
+	int id;
+
+	priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->reg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->reg))
+		return PTR_ERR(priv->reg);
+
+	id = of_alias_get_id(dev->of_node, "dc0-tcon");
+	if (id < 0) {
+		dev_err(dev, "failed to get alias id: %d\n", id);
+		return id;
+	}
+
+	de = dc_drm->de[id];
+	de->tc = &priv->tc;
+	de->tc->dev = dev;
+
+	return 0;
+}
+
+static const struct component_ops dc_tc_ops = {
+	.bind = dc_tc_bind,
+};
+
+static int dc_tc_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	ret = component_add(&pdev->dev, &dc_tc_ops);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to add component\n");
+
+	return 0;
+}
+
+static void dc_tc_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &dc_tc_ops);
+}
+
+static const struct of_device_id dc_tc_dt_ids[] = {
+	{ .compatible = "fsl,imx8qxp-dc-tcon", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, dc_tc_dt_ids);
+
+struct platform_driver dc_tc_driver = {
+	.probe = dc_tc_probe,
+	.remove_new = dc_tc_remove,
+	.driver = {
+		.name = "imx8-dc-tcon",
+		.of_match_table = dc_tc_dt_ids,
+	},
+};