arm/arm64: mediatek: Fix mmsys device probing

From: Matthias Brugger <[email protected]>

The MMSYS subsystem includes clocks and drm components. This patch adds an initailization path through a platform device for the clock part, so that both drivers get probed from the same device tree compatible.

Signed-off-by Matthias Brugger <[email protected]>

---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
 2 files changed, 20 insertions(+)

drivers/gpu/drm/mediatek/mtk_drm_drv.c +18 -0

index dd249cf5121e..c946aea722e5 100644
@@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
 	.ext_path = mt2701_mtk_ddp_ext,
 	.ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext),
 	.shadow_register = true,
+	.clk_drv_name = "clk-mt2701-mm",
 };
 
 static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {

@@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
 	.main_len = ARRAY_SIZE(mt8173_mtk_ddp_main),
 	.ext_path = mt8173_mtk_ddp_ext,
 	.ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext),
+	.clk_drv_name = "clk-mt8173-mm",
 };
 
 static int mtk_drm_kms_init(struct drm_device *drm)

@@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	if (IS_ERR(private->config_regs))
 		return PTR_ERR(private->config_regs);
 
+	if (private->data->clk_drv_name) {
+		private->clk_dev = platform_device_register_data(dev,
+						private->data->clk_drv_name, -1,
+						NULL, 0);
+
+		if (IS_ERR(private->clk_dev)) {
+			pr_err("failed to register %s platform device\n",
+						private->data->clk_drv_name);
+
+			return PTR_ERR(private->clk_dev);
+		}
+	}
+
 	/* Iterate over sibling DISP function blocks */
 	for_each_child_of_node(dev->of_node->parent, node) {
 		const struct of_device_id *of_id;

@@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev)
 	for (i = 0; i < DDP_COMPONENT_ID_MAX; i++)
 		of_node_put(private->comp_node[i]);
 
+	if (private->clk_dev)
+		platform_device_unregister(private->clk_dev);
+
 	return 0;
 }
 

drivers/gpu/drm/mediatek/mtk_drm_drv.h +2 -0

index 86cec19193c4..200eee5de419 100644
@@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data {
 	const enum mtk_ddp_comp_id *ext_path;
 	unsigned int ext_len;
 	bool shadow_register;
+	const char *clk_drv_name;
 };
 
 struct mtk_drm_private {
 	struct drm_device *drm;
 	struct device *dma_dev;
+	struct platform_device *clk_dev;
 
 	unsigned int num_pipes;
 

Quoted 7 lines · On Wed, 2018-07-18 at 00:03 +0200, [email protected] wrote:

From: Matthias Brugger <[email protected]>

The MMSYS subsystem includes clocks and drm components. This patch adds an initailization path through a platform device for the clock part, so that both drivers get probed from the same device tree compatible.

Sorry for that I should have a response earlier for the series.

Some points I felt they're not exactly right and should be fixed up before we're moving on

Currently, drm driver have a wrong reference to the dt-binding, "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all for the subsystem exporting clock and reset line such common resource to its sub-devices. Every subsystem has a similar shape. I hope mmsys shouldn't be an exception.

DRM device needs to have its own dt-binding show how connections between DRM components being made and its node should be put under mmsys node.

Quoted 1 line

In this way, it becomes easy to see how the topology of the subsystem is

and grows, like a tree "device tree", instead of hiding the details in the implementation.

The similar example we already did for audsys on mt2701 and mt7623 as below

	audsys: clock-controller@11220000 {
		compatible = "mediatek,mt7623-audsys",
			     "mediatek,mt2701-audsys",
			     "syscon";
			...

		afe: audio-controller {
			compatible = "mediatek,mt7623-audio",
				     "mediatek,mt2701-audio";
			
			...
		};
	};

Sean

Quoted 74 lines

Signed-off-by Matthias Brugger <[email protected]>

---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
 2 files changed, 20 insertions(+)

drivers/gpu/drm/mediatek/mtk_drm_drv.c +18 -0

index dd249cf5121e..c946aea722e5 100644
@@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
 	.ext_path = mt2701_mtk_ddp_ext,
 	.ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext),
 	.shadow_register = true,
+	.clk_drv_name = "clk-mt2701-mm",
 };
 
 static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {

@@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
 	.main_len = ARRAY_SIZE(mt8173_mtk_ddp_main),
 	.ext_path = mt8173_mtk_ddp_ext,
 	.ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext),
+	.clk_drv_name = "clk-mt8173-mm",
 };
 
 static int mtk_drm_kms_init(struct drm_device *drm)

@@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	if (IS_ERR(private->config_regs))
 		return PTR_ERR(private->config_regs);
 
+	if (private->data->clk_drv_name) {
+		private->clk_dev = platform_device_register_data(dev,
+						private->data->clk_drv_name, -1,
+						NULL, 0);
+
+		if (IS_ERR(private->clk_dev)) {
+			pr_err("failed to register %s platform device\n",
+						private->data->clk_drv_name);
+
+			return PTR_ERR(private->clk_dev);
+		}
+	}
+
 	/* Iterate over sibling DISP function blocks */
 	for_each_child_of_node(dev->of_node->parent, node) {
 		const struct of_device_id *of_id;

@@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev)
 	for (i = 0; i < DDP_COMPONENT_ID_MAX; i++)
 		of_node_put(private->comp_node[i]);
 
+	if (private->clk_dev)
+		platform_device_unregister(private->clk_dev);
+
 	return 0;
 }
 

drivers/gpu/drm/mediatek/mtk_drm_drv.h +2 -0

index 86cec19193c4..200eee5de419 100644
@@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data {
 	const enum mtk_ddp_comp_id *ext_path;
 	unsigned int ext_len;
 	bool shadow_register;
+	const char *clk_drv_name;
 };
 
 struct mtk_drm_private {
 	struct drm_device *drm;
 	struct device *dma_dev;
+	struct platform_device *clk_dev;
 
 	unsigned int num_pipes;
 

Hi Sean,

Quoted 11 lines · Sean Wang

Quoted 7 lines · On Wed, 2018-07-18 at 00:03 +0200, [email protected] wrote:

From: Matthias Brugger <[email protected]>

The MMSYS subsystem includes clocks and drm components. This patch adds an initailization path through a platform device for the clock part, so that both drivers get probed from the same device tree compatible.

Sorry for that I should have a response earlier for the series.

Quoted 8 lines

Some points I felt they're not exactly right and should be fixed up before we're moving on

Currently, drm driver have a wrong reference to the dt-binding, "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all for the subsystem exporting clock and reset line such common resource to its sub-devices. Every subsystem has a similar shape. I hope mmsys shouldn't be an exception.

I'm not sure if I understand what you mean. You proposed to use a platform device [1] and now you say we should change the DTS?

What do you mean by "drm driver has a wrong reference to the dt-binding"? How would you want to name it and most important, what would be your approach to have backwards compatibility to older device tree bindings?

Quoted 10 lines

DRM device needs to have its own dt-binding show how connections between DRM components being made and its node should be put under mmsys node.

Quoted 1 line

In this way, it becomes easy to see how the topology of the subsystem is

and grows, like a tree "device tree", instead of hiding the details in the implementation.

The similar example we already did for audsys on mt2701 and mt7623 as below

Quoted 14 lines

	audsys: clock-controller@11220000 {
		compatible = "mediatek,mt7623-audsys",
			     "mediatek,mt2701-audsys",
			     "syscon";
			...

		afe: audio-controller {
			compatible = "mediatek,mt7623-audio",
				     "mediatek,mt2701-audio";
			
			...
		};
	};

I think this approach is not working as, due to backwards compatibility, we have to make sure that the DRM driver gets probed with the mmsys binding.

Regards,
Matthias

Quoted 80 lines

Sean

Quoted 74 lines

Signed-off-by Matthias Brugger <[email protected]>

---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
 2 files changed, 20 insertions(+)

drivers/gpu/drm/mediatek/mtk_drm_drv.c +18 -0

index dd249cf5121e..c946aea722e5 100644
@@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
 	.ext_path = mt2701_mtk_ddp_ext,
 	.ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext),
 	.shadow_register = true,
+	.clk_drv_name = "clk-mt2701-mm",
 };
 
 static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {

@@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
 	.main_len = ARRAY_SIZE(mt8173_mtk_ddp_main),
 	.ext_path = mt8173_mtk_ddp_ext,
 	.ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext),
+	.clk_drv_name = "clk-mt8173-mm",
 };
 
 static int mtk_drm_kms_init(struct drm_device *drm)

@@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	if (IS_ERR(private->config_regs))
 		return PTR_ERR(private->config_regs);
 
+	if (private->data->clk_drv_name) {
+		private->clk_dev = platform_device_register_data(dev,
+						private->data->clk_drv_name, -1,
+						NULL, 0);
+
+		if (IS_ERR(private->clk_dev)) {
+			pr_err("failed to register %s platform device\n",
+						private->data->clk_drv_name);
+
+			return PTR_ERR(private->clk_dev);
+		}
+	}
+
 	/* Iterate over sibling DISP function blocks */
 	for_each_child_of_node(dev->of_node->parent, node) {
 		const struct of_device_id *of_id;

@@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev)
 	for (i = 0; i < DDP_COMPONENT_ID_MAX; i++)
 		of_node_put(private->comp_node[i]);
 
+	if (private->clk_dev)
+		platform_device_unregister(private->clk_dev);
+
 	return 0;
 }
 

drivers/gpu/drm/mediatek/mtk_drm_drv.h +2 -0

index 86cec19193c4..200eee5de419 100644
@@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data {
 	const enum mtk_ddp_comp_id *ext_path;
 	unsigned int ext_len;
 	bool shadow_register;
+	const char *clk_drv_name;
 };
 
 struct mtk_drm_private {
 	struct drm_device *drm;
 	struct device *dma_dev;
+	struct platform_device *clk_dev;
 
 	unsigned int num_pipes;
 

Hi Sean,

Quoted 14 lines · Sean Wang

Quoted 7 lines · On Wed, 2018-07-18 at 00:03 +0200, [email protected] wrote:

From: Matthias Brugger <[email protected]>

The MMSYS subsystem includes clocks and drm components. This patch adds an initailization path through a platform device for the clock part, so that both drivers get probed from the same device tree compatible.

Sorry for that I should have a response earlier for the series.

Some points I felt they're not exactly right and should be fixed up before we're moving on

Quoted 5 lines

Currently, drm driver have a wrong reference to the dt-binding, "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all for the subsystem exporting clock and reset line such common resource to its sub-devices. Every subsystem has a similar shape. I hope mmsys shouldn't be an exception.

I'm not sure if I understand what you mean. You proposed to use a platform device [1] and now you say we should change the DTS?

What do you mean by "drm driver has a wrong reference to the dt-binding"? How

would you want to name it and most important, what would be your approach to have backwards compatibility to older device tree bindings?

Quoted 10 lines

DRM device needs to have its own dt-binding show how connections between DRM components being made and its node should be put under mmsys node.

Quoted 1 line

In this way, it becomes easy to see how the topology of the subsystem is

and grows, like a tree "device tree", instead of hiding the details in the implementation.

The similar example we already did for audsys on mt2701 and mt7623 as below

Quoted 14 lines

	audsys: clock-controller@11220000 {
		compatible = "mediatek,mt7623-audsys",
			     "mediatek,mt2701-audsys",
			     "syscon";
			...

		afe: audio-controller {
			compatible = "mediatek,mt7623-audio",
				     "mediatek,mt2701-audio";
			
			...
		};
	};

I think this approach is not working as, due to backwards compatibility, we have to make sure that the DRM driver gets probed with the mmsys binding.

Regards,
Matthias

Quoted 80 lines

Sean

Quoted 74 lines

Signed-off-by Matthias Brugger <[email protected]>

---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
 2 files changed, 20 insertions(+)

drivers/gpu/drm/mediatek/mtk_drm_drv.c +18 -0

index dd249cf5121e..c946aea722e5 100644
@@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
 	.ext_path = mt2701_mtk_ddp_ext,
 	.ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext),
 	.shadow_register = true,
+	.clk_drv_name = "clk-mt2701-mm",
 };
 
 static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {

@@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
 	.main_len = ARRAY_SIZE(mt8173_mtk_ddp_main),
 	.ext_path = mt8173_mtk_ddp_ext,
 	.ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext),
+	.clk_drv_name = "clk-mt8173-mm",
 };
 
 static int mtk_drm_kms_init(struct drm_device *drm)

@@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	if (IS_ERR(private->config_regs))
 		return PTR_ERR(private->config_regs);
 
+	if (private->data->clk_drv_name) {
+		private->clk_dev = platform_device_register_data(dev,
+						private->data->clk_drv_name, -1,
+						NULL, 0);
+
+		if (IS_ERR(private->clk_dev)) {
+			pr_err("failed to register %s platform device\n",
+						private->data->clk_drv_name);
+
+			return PTR_ERR(private->clk_dev);
+		}
+	}
+
 	/* Iterate over sibling DISP function blocks */
 	for_each_child_of_node(dev->of_node->parent, node) {
 		const struct of_device_id *of_id;

@@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev)
 	for (i = 0; i < DDP_COMPONENT_ID_MAX; i++)
 		of_node_put(private->comp_node[i]);
 
+	if (private->clk_dev)
+		platform_device_unregister(private->clk_dev);
+
 	return 0;
 }
 

drivers/gpu/drm/mediatek/mtk_drm_drv.h +2 -0

index 86cec19193c4..200eee5de419 100644
@@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data {
 	const enum mtk_ddp_comp_id *ext_path;
 	unsigned int ext_len;
 	bool shadow_register;
+	const char *clk_drv_name;
 };
 
 struct mtk_drm_private {
 	struct drm_device *drm;
 	struct device *dma_dev;
+	struct platform_device *clk_dev;
 
 	unsigned int num_pipes;
 

Quoted 6 lines · On Wed, 2018-07-18 at 00:03 +0200, [email protected] wrote:

From: Matthias Brugger <[email protected]>

The MMSYS subsystem includes clocks and drm components. This patch adds an initailization path through a platform device for the clock part, so that both drivers get probed from the same device tree compatible.

Sorry for that I should have a response earlier for the series.

Some points I felt they're not exactly right and should be fixed up before we're moving on

Currently, drm driver have a wrong reference to the dt-binding, "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all for the subsystem exporting clock and reset line such common resource to its sub-devices. Every subsystem has a similar shape. I hope mmsys shouldn't be an exception.

DRM device needs to have its own dt-binding show how connections between DRM components being made and its node should be put under mmsys node.

In this way, it becomes easy to see how the topology of the subsystem is and grows, like a tree "device tree", instead of hiding the details in the implementation.

The similar example we already did for audsys on mt2701 and mt7623 as below

	audsys: clock-controller@11220000 {

	};

Quoted 7 lines

Signed-off-by Matthias Brugger <[email protected]>

---

 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
 2 files changed, 20 insertions(+)

drivers/gpu/drm/mediatek/mtk_drm_drv.c

Quoted 9 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index dd249cf5121e..c946aea722e5 100644

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

@@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data

mt2701_mmsys_driver_data = {>

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

 	.ext_path = mt2701_mtk_ddp_ext,
 	.ext_len = 

Quoted 11 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

 	.shadow_register = true,
 
+	.clk_drv_name = "clk-mt2701-mm",
 
 };
 
 static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {

@@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data

mt8173_mmsys_driver_data = {>
.main_len =

drivers/gpu/drm/mediatek/mtk_drm_drv.c

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

 	.ext_path = mt8173_mtk_ddp_ext,
 	.ext_len = 

Quoted 5 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+	.clk_drv_name = "clk-mt8173-mm",
 
 };

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 417

Quoted 8 lines

static int mtk_drm_kms_init(struct drm_device *drm)

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

@@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device
 *pdev)
 
 	if (IS_ERR(private->config_regs))
 
 

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 419

Quoted 3 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+	if (private->data->clk_drv_name) {
+		

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 420

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 421

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 423

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+
+		

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 424

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 425

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 427

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+
+		

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 428

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

Quoted 4 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+	}
+
 

/* Iterate over sibling DISP

Quoted 1 line

for_each_child_of_node(dev->of_node-

parent, node) {

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_remove(struct platform_device · line 532

Quoted 5 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

@@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device
 *pdev)
 
 	for (i = 0; i < 

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_remove(struct platform_device *pdev) · line 535

Quoted 3 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+	if (private->clk_dev)
+		

Quoted 6 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+
 
 	return 0;
 
 }

drivers/gpu/drm/mediatek/mtk_drm_drv.h struct mtk_mmsys_driver_data { · line 35

Quoted 8 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index 86cec19193c4..200eee5de419 100644

drivers/gpu/drm/mediatek/mtk_drm_drv.h +0 -0

@@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data {
 
 	const enum mtk_ddp_comp_id 

Quoted 15 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.h +2 -0

 	unsigned int ext_len;
 	bool shadow_register;
 
+	const char *clk_drv_name;
 
 };
 
 struct mtk_drm_private {
 
 	struct drm_device *drm;
 	struct device *dma_dev;
 
+	struct platform_device *clk_dev;
 
 	unsigned int num_pipes;

Hi Sean,

Quoted 22 lines · On Wednesday, 18 July 2018 05:57:35 EEST Sean Wang wrote:

Quoted 6 lines · On Wed, 2018-07-18 at 00:03 +0200, [email protected] wrote:

From: Matthias Brugger <[email protected]>

The MMSYS subsystem includes clocks and drm components. This patch adds an initailization path through a platform device for the clock part, so that both drivers get probed from the same device tree compatible.

Sorry for that I should have a response earlier for the series.

Some points I felt they're not exactly right and should be fixed up before we're moving on

Currently, drm driver have a wrong reference to the dt-binding, "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all for the subsystem exporting clock and reset line such common resource to its sub-devices. Every subsystem has a similar shape. I hope mmsys shouldn't be an exception.

DRM device needs to have its own dt-binding show how connections between DRM components being made and its node should be put under mmsys node.

Quoted 9 lines

In this way, it becomes easy to see how the topology of the subsystem is and grows, like a tree "device tree", instead of hiding the details in the implementation.

The similar example we already did for audsys on mt2701 and mt7623 as below

	audsys: clock-controller@11220000 {

compatible = "mediatek,mt7623-audsys",

Quoted 1 line

	     "mediatek,mt2701-audsys",

Quoted 1 line

	     "syscon";

Quoted 1 line

	...

Quoted 2 lines

afe: audio-controller {

Quoted 1 line

	compatible = "mediatek,mt7623-audio",

Quoted 1 line

     "mediatek,mt2701-audio";

Quoted 2 lines

	...

Quoted 1 line

};

Quoted 1 line

	};

This looks very strange to me. I'm not familiar with the hardware architecture, but a clock controller that includes an audio controller seems like a very weird design. It's usually the other way around, you have an audio

controller, and it also contains hardware that produces clocks, and possibly handles miscellaneous audio-related routing tasks. I would thus expect the reverse in the device tree:

	afe: audio-controller@11220000 {

compatible = "mediatek,mt7623-audio",

	     "mediatek,mt2701-audio";

..

audsys: clock-controller {
		
	compatible = "mediatek,mt7623-audsys",
		
		
     "mediatek,mt2701-audsys",
		
		
     "syscon";
		
	...

};
	};

And if audsys only exposes clocks, you don't even need a subnode to represent it, the audio controller itself can be a clock provider.

	afe: audio-controller@11220000 {

compatible = "mediatek,mt7623-audio",

	     "mediatek,mt2701-audio";

#clock-cells = <1>;

..

	};

Quoted 7 lines

Quoted 7 lines

Signed-off-by Matthias Brugger <[email protected]>

---

 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
 2 files changed, 20 insertions(+)

Quoted 9 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c

Quoted 9 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index dd249cf5121e..c946aea722e5 100644

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

@@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data

mt2701_mmsys_driver_data = {>

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

 	.ext_path = mt2701_mtk_ddp_ext,
 	.ext_len = 

ARRAY_SIZE(mt2701_mtk_ddp_ext),

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

 	.shadow_register = true,
 
+	.clk_drv_name = "clk-mt2701-mm",
 
 };
 
 static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {

@@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data

mt8173_mmsys_driver_data = {>
.main_len =

ARRAY_SIZE(mt8173_mtk_ddp_main),

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

 	.ext_path = mt8173_mtk_ddp_ext,
 	.ext_len = 

ARRAY_SIZE(mt8173_mtk_ddp_ext),

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+	.clk_drv_name = "clk-mt8173-mm",
 
 };

Quoted 8 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 417

Quoted 8 lines

static int mtk_drm_kms_init(struct drm_device *drm)

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

@@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device
 *pdev)
 
 	if (IS_ERR(private->config_regs))
 
 

return PTR_ERR(private->config_regs);

Quoted 3 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 419

Quoted 3 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+	if (private->data->clk_drv_name) {
+		

private->clk_dev = platform_device_register_data(dev,

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 420

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

private->data->clk_drv_name, -1,

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 421

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

NULL, 0);

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 423

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+
+		

if (IS_ERR(private->clk_dev)) {

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 424

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

pr_err("failed to register %s platform
device\n",

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 425

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

private->data->clk_drv_name);

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 427

Quoted 2 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+
+		

	return PTR_ERR(private->clk_dev);

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_probe(struct platform_device · line 428

Quoted 1 line

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+		

}

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+	}
+
 

/* Iterate over sibling DISP

function blocks */

Quoted 4 lines

Quoted 1 line

for_each_child_of_node(dev->of_node-

parent, node) {

Quoted 2 lines

const struct of_device_id *of_id;

Quoted 5 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_remove(struct platform_device · line 532

Quoted 5 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +0 -0

@@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device
 *pdev)
 
 	for (i = 0; i < 

DDP_COMPONENT_ID_MAX; i++)

Quoted 2 lines

Quoted 2 lines

of_node_put(private->comp_node[i]);

Quoted 3 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c static int mtk_drm_remove(struct platform_device *pdev) · line 535

Quoted 3 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +2 -0

+	if (private->clk_dev)
+		

platform_device_unregister(private->clk_dev);

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.c +1 -0

+
 
 	return 0;
 
 }

Quoted 8 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.h struct mtk_mmsys_driver_data { · line 35

Quoted 8 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index 86cec19193c4..200eee5de419 100644

drivers/gpu/drm/mediatek/mtk_drm_drv.h +0 -0

@@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data {
 
 	const enum mtk_ddp_comp_id 

*ext_path;

Quoted 15 lines

Quoted 15 lines

drivers/gpu/drm/mediatek/mtk_drm_drv.h +2 -0

 	unsigned int ext_len;
 	bool shadow_register;
 
+	const char *clk_drv_name;
 
 };
 
 struct mtk_drm_private {
 
 	struct drm_device *drm;
 	struct device *dma_dev;
 
+	struct platform_device *clk_dev;
 
 	unsigned int num_pipes;

Quoted 51 lines

Hi Sean,

Quoted 22 lines · On Wednesday, 18 July 2018 05:57:35 EEST Sean Wang wrote:

Quoted 6 lines · matthias.bgg at kernel.org

From: Matthias Brugger <[email protected]>

The MMSYS subsystem includes clocks and drm components. This patch adds an initailization path through a platform device for the clock part, so that both drivers get probed from the same device tree compatible.

Sorry for that I should have a response earlier for the series.

Some points I felt they're not exactly right and should be fixed up before we're moving on

Currently, drm driver have a wrong reference to the dt-binding, "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all for the subsystem exporting clock and reset line such common resource to its sub-devices. Every subsystem has a similar shape. I hope mmsys shouldn't be an exception.

DRM device needs to have its own dt-binding show how connections between DRM components being made and its node should be put under mmsys node.

Quoted 9 lines

In this way, it becomes easy to see how the topology of the subsystem is and grows, like a tree "device tree", instead of hiding the details in the implementation.

The similar example we already did for audsys on mt2701 and mt7623 as below

    audsys: clock-controller at 11220000 {

compatible = "mediatek,mt7623-audsys",

Quoted 1 line

           "mediatek,mt2701-audsys",

Quoted 1 line

           "syscon";

Quoted 1 line

      ...

Quoted 2 lines

afe: audio-controller {

Quoted 1 line

      compatible = "mediatek,mt7623-audio",

Quoted 1 line

     "mediatek,mt2701-audio";

Quoted 2 lines

Quoted 8 lines

      ...

Quoted 1 line

};

Quoted 1 line

    };

This looks very strange to me. I'm not familiar with the hardware architecture, but a clock controller that includes an audio controller seems like a very weird design. It's usually the other way around, you have an audio

yes, naming audsys as clock controller is really not good. it should be worth a better naming.

mtk subsystem AFAIK works as a container, at least provides clocks, reset, syscon access, these common resource to these devices running on the subsystem. It also has a power gate independent from other subsystem, that can be controlled when these devices all powered down or once up.

And it should be better that we don't assume what exact devices are running on since it is possible that there're different devices running on the same subsystem per SoC. mtk has many subsystem working in this way. mmsys is just a case. we can see in [1]

[1]
https://elixir.bootlin.com/linux/v4.18-rc5/source/Documentation/devicetree/bindings/arm/mediatek

Quoting Sean Wang (2018-07-18 03:06:27)

Quoted 16 lines · Laurent Pinchart

Quoted 5 lines

Hi Sean,

This looks very strange to me. I'm not familiar with the hardware architecture, but a clock controller that includes an audio controller seems like a very weird design. It's usually the other way around, you have an audio

yes, naming audsys as clock controller is really not good. it should be worth a better naming.

mtk subsystem AFAIK works as a container, at least provides clocks, reset, syscon access, these common resource to these devices running on the subsystem. It also has a power gate independent from other subsystem, that can be controlled when these devices all powered down or once up.

Quoted 9 lines

And it should be better that we don't assume what exact devices are running on since it is possible that there're different devices running on the same subsystem per SoC. mtk has many subsystem working in this way. mmsys is just a case. we can see in [1]

[1]
https://elixir.bootlin.com/linux/v4.18-rc5/source/Documentation/devicetree/bindings/arm/mediatek

Sean, is this an ack for this patch series? I think the consensus is to not modify DT to add a subnode for the "clk" part of the hardware, but instead either register the clks from the device driver that has the clk hardware inside of it, or do what this patch series does and register a device in software from the "parent" device driver so that logically clk things are contained in drivers/clk/

From: Matthias Brugger <[email protected]>

It can happen that the clock drivers wasn't probed before the ddp driver gets invoked. The driver used to omit a warning that the driver failed to get the clocks. Omit this error on the defered probe path.

Signed-off-by Matthias Brugger <[email protected]>

Acked-by CK Hu <[email protected]>

---
 drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/mediatek/mtk_drm_ddp.c +2 -1

index bafc5c77c4fb..6b399348a2dc 100644
@@ -374,7 +374,8 @@ static int mtk_ddp_probe(struct platform_device *pdev)
 
 	ddp->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(ddp->clk)) {
-		dev_err(dev, "Failed to get clock\n");
+		if (PTR_ERR(ddp->clk) != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get clock\n");
 		return PTR_ERR(ddp->clk);
 	}