[resend PATCH v4 0/5] arm/arm64: mediatek: Fix mmsys device probing

[resend PATCH v4 0/5] arm/arm64: mediatek: Fix mmsys device probing

[matthias.bgg]

Changes since v3:
- use platform device to probe clock driver
- add Acked-by CK Hu for the probe deferred patch

Changes since v2:
- fix kconfig typo (shame on me)
- delete __initconst from mm_clocks as converted to a platform driver
  
Changes since v1:
- add binding documentation
- ddp: use regmap_update_bits
- ddp: ignore EPROBE_DEFER on clock probing
- mfd: delete mmsys_private
- add Reviewed-by and Acked-by tags
 
MMSYS in Mediatek SoCs has some registers to control clock gates (which is 
used in the clk driver) and some registers to set the routing and enable
the differnet blocks of the display subsystem.

Up to now both drivers, clock and drm are probed with the same device tree
compatible. But only the first driver get probed, which in effect breaks
graphics on mt8173 and mt2701.

This patch uses a platform device registration in the DRM driver, which
will trigger the probe of the corresponding clock driver. It was tested on the
bananapi-r2 and the Acer R13 Chromebook.

[resend PATCH v4 1/5] drm/mediatek: Use regmap for register access

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

The mmsys memory space is shared between the drm and the
clk driver. Use regmap to access it.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c |  4 +--
 drivers/gpu/drm/mediatek/mtk_drm_ddp.c  | 38 ++++++++++---------------
 drivers/gpu/drm/mediatek/mtk_drm_ddp.h  |  4 +--
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  | 13 +++------
 drivers/gpu/drm/mediatek/mtk_drm_drv.h  |  2 +-
 5 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 658b8dd45b83..4c65873b4867 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -33,7 +33,7 @@
  * @enabled: records whether crtc_enable succeeded
  * @planes: array of 4 drm_plane structures, one for each overlay plane
  * @pending_planes: whether any plane has pending changes to be applied
- * @config_regs: memory mapped mmsys configuration register space
+ * @config_regs: regmap mapped mmsys configuration register space
  * @mutex: handle to one of the ten disp_mutex streams
  * @ddp_comp_nr: number of components in ddp_comp
  * @ddp_comp: array of pointers the mtk_ddp_comp structures used by this crtc
@@ -48,7 +48,7 @@ struct mtk_drm_crtc {
 	struct drm_plane		planes[OVL_LAYER_NR];
 	bool				pending_planes;
 
-	void __iomem			*config_regs;
+	struct regmap			*config_regs;
 	struct mtk_disp_mutex		*mutex;
 	unsigned int			ddp_comp_nr;
 	struct mtk_ddp_comp		**ddp_comp;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
index 8130f3dab661..bafc5c77c4fb 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
@@ -185,53 +185,45 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
 	return value;
 }
 
-static void mtk_ddp_sout_sel(void __iomem *config_regs,
+static void mtk_ddp_sout_sel(struct regmap *config_regs,
 			     enum mtk_ddp_comp_id cur,
 			     enum mtk_ddp_comp_id next)
 {
 	if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0)
-		writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
-			       config_regs + DISP_REG_CONFIG_OUT_SEL);
+		regmap_write(config_regs, DISP_REG_CONFIG_OUT_SEL,
+				BLS_TO_DSI_RDMA1_TO_DPI1);
 }
 
-void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
+void mtk_ddp_add_comp_to_path(struct regmap *config_regs,
 			      enum mtk_ddp_comp_id cur,
 			      enum mtk_ddp_comp_id next)
 {
-	unsigned int addr, value, reg;
+	unsigned int addr, value;
 
 	value = mtk_ddp_mout_en(cur, next, &addr);
-	if (value) {
-		reg = readl_relaxed(config_regs + addr) | value;
-		writel_relaxed(reg, config_regs + addr);
-	}
+	if (value)
+		regmap_update_bits(config_regs, addr, value, value);
 
 	mtk_ddp_sout_sel(config_regs, cur, next);
 
 	value = mtk_ddp_sel_in(cur, next, &addr);
-	if (value) {
-		reg = readl_relaxed(config_regs + addr) | value;
-		writel_relaxed(reg, config_regs + addr);
-	}
+	if (value)
+		regmap_update_bits(config_regs, addr, value, value);
 }
 
-void mtk_ddp_remove_comp_from_path(void __iomem *config_regs,
+void mtk_ddp_remove_comp_from_path(struct regmap *config_regs,
 				   enum mtk_ddp_comp_id cur,
 				   enum mtk_ddp_comp_id next)
 {
-	unsigned int addr, value, reg;
+	unsigned int addr, value;
 
 	value = mtk_ddp_mout_en(cur, next, &addr);
-	if (value) {
-		reg = readl_relaxed(config_regs + addr) & ~value;
-		writel_relaxed(reg, config_regs + addr);
-	}
+	if (value)
+		regmap_update_bits(config_regs, addr, value, 0);
 
 	value = mtk_ddp_sel_in(cur, next, &addr);
-	if (value) {
-		reg = readl_relaxed(config_regs + addr) & ~value;
-		writel_relaxed(reg, config_regs + addr);
-	}
+	if (value)
+		regmap_update_bits(config_regs, addr, value, 0);
 }
 
 struct mtk_disp_mutex *mtk_disp_mutex_get(struct device *dev, unsigned int id)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
index f9a799168077..32e12f33b76a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
@@ -20,10 +20,10 @@ struct regmap;
 struct device;
 struct mtk_disp_mutex;
 
-void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
+void mtk_ddp_add_comp_to_path(struct regmap *config_regs,
 			      enum mtk_ddp_comp_id cur,
 			      enum mtk_ddp_comp_id next);
-void mtk_ddp_remove_comp_from_path(void __iomem *config_regs,
+void mtk_ddp_remove_comp_from_path(struct regmap *config_regs,
 				   enum mtk_ddp_comp_id cur,
 				   enum mtk_ddp_comp_id next);
 
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index b2ad19bd1e00..dd249cf5121e 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -21,6 +21,7 @@
 #include <drm/drm_of.h>
 #include <linux/component.h>
 #include <linux/iommu.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
@@ -393,7 +394,6 @@ static int mtk_drm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mtk_drm_private *private;
-	struct resource *mem;
 	struct device_node *node;
 	struct component_match *match = NULL;
 	int ret;
@@ -407,14 +407,9 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	INIT_WORK(&private->commit.work, mtk_atomic_work);
 	private->data = of_device_get_match_data(dev);
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	private->config_regs = devm_ioremap_resource(dev, mem);
-	if (IS_ERR(private->config_regs)) {
-		ret = PTR_ERR(private->config_regs);
-		dev_err(dev, "Failed to ioremap mmsys-config resource: %d\n",
-			ret);
-		return ret;
-	}
+	private->config_regs = syscon_node_to_regmap(dev->of_node);
+	if (IS_ERR(private->config_regs))
+		return PTR_ERR(private->config_regs);
 
 	/* Iterate over sibling DISP function blocks */
 	for_each_child_of_node(dev->of_node->parent, node) {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
index 4edc0023684c..86cec19193c4 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -44,7 +44,7 @@ struct mtk_drm_private {
 
 	struct device_node *mutex_node;
 	struct device *mutex_dev;
-	void __iomem *config_regs;
+	struct regmap *config_regs;
 	struct device_node *comp_node[DDP_COMPONENT_ID_MAX];
 	struct mtk_ddp_comp *ddp_comp[DDP_COMPONENT_ID_MAX];
 	const struct mtk_mmsys_driver_data *data;
-- 
2.17.1

[resend PATCH v4 2/5] clk: mediatek: mt2701-mmsys: switch to platform device probing

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

Switch probing for the MMSYS to support invocation to a plain
paltform device. The driver will be probed by the DRM subsystem.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 drivers/clk/mediatek/clk-mt2701-mm.c | 42 ++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2701-mm.c b/drivers/clk/mediatek/clk-mt2701-mm.c
index fe1f85072fc5..200b1842b94b 100644
--- a/drivers/clk/mediatek/clk-mt2701-mm.c
+++ b/drivers/clk/mediatek/clk-mt2701-mm.c
@@ -12,14 +12,20 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/module.h>
 #include <linux/clk-provider.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 
 #include "clk-mtk.h"
 #include "clk-gate.h"
 
 #include <dt-bindings/clock/mt2701-clk.h>
 
+struct clk_mt2701_mm_priv {
+	struct clk_onecell_data *clk_data;
+};
+
 static const struct mtk_gate_regs disp0_cg_regs = {
 	.set_ofs = 0x0104,
 	.clr_ofs = 0x0108,
@@ -87,23 +93,25 @@ static const struct mtk_gate mm_clks[] = {
 	GATE_DISP1(CLK_MM_TVE_FMM, "mm_tve_fmm", "mm_sel", 14),
 };
 
-static const struct of_device_id of_match_clk_mt2701_mm[] = {
-	{ .compatible = "mediatek,mt2701-mmsys", },
-	{}
-};
-
 static int clk_mt2701_mm_probe(struct platform_device *pdev)
 {
-	struct clk_onecell_data *clk_data;
 	int r;
-	struct device_node *node = pdev->dev.of_node;
+	struct device_node *node = pdev->dev.parent->of_node;
+	struct clk_mt2701_mm_priv *private;
+
+	private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL);
+	if (!private)
+		return -ENOMEM;
 
-	clk_data = mtk_alloc_clk_data(CLK_MM_NR);
+	private->clk_data = mtk_alloc_clk_data(CLK_MM_NR);
+
+	platform_set_drvdata(pdev, private);
 
 	mtk_clk_register_gates(node, mm_clks, ARRAY_SIZE(mm_clks),
-						clk_data);
+					private->clk_data);
 
-	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+	r = of_clk_add_provider(node, of_clk_src_onecell_get,
+					private->clk_data);
 	if (r)
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
@@ -112,12 +120,22 @@ static int clk_mt2701_mm_probe(struct platform_device *pdev)
 	return r;
 }
 
+static int clk_mt2701_mm_remove(struct platform_device *pdev)
+{
+	struct clk_mt2701_mm_priv *private = platform_get_drvdata(pdev);
+
+	kfree(private->clk_data);
+	kfree(private);
+
+	return 0;
+}
+
 static struct platform_driver clk_mt2701_mm_drv = {
 	.probe = clk_mt2701_mm_probe,
+	.remove = clk_mt2701_mm_remove,
 	.driver = {
 		.name = "clk-mt2701-mm",
-		.of_match_table = of_match_clk_mt2701_mm,
 	},
 };
 
-builtin_platform_driver(clk_mt2701_mm_drv);
+module_platform_driver(clk_mt2701_mm_drv);
-- 
2.17.1

[resend PATCH v4 3/5] clk: mediatek: mt8173: switch mmsys to platform device probing

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

Switch probing for the MMSYS to support invocation to a
plain paltform device. The driver will be probed by the DRM subsystem.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 drivers/clk/mediatek/clk-mt8173.c | 51 ++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 96c292c3e440..10b6a0251123 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -13,8 +13,11 @@
  */
 
 #include <linux/clk.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
 
 #include "clk-mtk.h"
 #include "clk-gate.h"
@@ -791,7 +794,7 @@ static const struct mtk_gate_regs mm1_cg_regs __initconst = {
 		.ops = &mtk_clk_gate_ops_setclr,		\
 	}
 
-static const struct mtk_gate mm_clks[] __initconst = {
+static const struct mtk_gate mm_clks[] = {
 	/* MM0 */
 	GATE_MM0(CLK_MM_SMI_COMMON, "mm_smi_common", "mm_sel", 0),
 	GATE_MM0(CLK_MM_SMI_LARB0, "mm_smi_larb0", "mm_sel", 1),
@@ -1152,22 +1155,56 @@ static void __init mtk_imgsys_init(struct device_node *node)
 }
 CLK_OF_DECLARE(mtk_imgsys, "mediatek,mt8173-imgsys", mtk_imgsys_init);
 
-static void __init mtk_mmsys_init(struct device_node *node)
-{
+struct mtk_mmsys_priv {
 	struct clk_onecell_data *clk_data;
+};
+
+static int mtk_mmsys_probe(struct platform_device *pdev)
+{
 	int r;
+	struct device_node *node;
+	struct mtk_mmsys_priv *private;
+
+	node = pdev->dev.parent->of_node;
 
-	clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
+	private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL);
+	if (!private)
+		return -ENOMEM;
+
+	private->clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
+
+	platform_set_drvdata(pdev, private);
 
 	mtk_clk_register_gates(node, mm_clks, ARRAY_SIZE(mm_clks),
-						clk_data);
+					private->clk_data);
 
-	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+	r = of_clk_add_provider(node, of_clk_src_onecell_get,
+					private->clk_data);
 	if (r)
 		pr_err("%s(): could not register clock provider: %d\n",
 			__func__, r);
+
+	return r;
+}
+
+static int mtk_mmsys_remove(struct platform_device *pdev)
+{
+	struct mtk_mmsys_priv *private = platform_get_drvdata(pdev);
+
+	kfree(private->clk_data);
+	kfree(private);
+
+	return 0;
 }
-CLK_OF_DECLARE(mtk_mmsys, "mediatek,mt8173-mmsys", mtk_mmsys_init);
+
+static struct platform_driver clk_mt8173_mm_drv = {
+	.probe = mtk_mmsys_probe,
+	.probe = mtk_mmsys_remove,
+	.driver = {
+		.name = "clk-mt8173-mm",
+	},
+};
+module_platform_driver(clk_mt8173_mm_drv);
 
 static void __init mtk_vdecsys_init(struct device_node *node)
 {
-- 
2.17.1

[resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

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 <mbrugger@suse.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index dd249cf5121e..c946aea722e5 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -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;
 }
 
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
index 86cec19193c4..200eee5de419 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -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;
 
-- 
2.17.1

Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[Sean Wang]

On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> 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 {
		compatible = "mediatek,mt7623-audsys",
			     "mediatek,mt2701-audsys",
			     "syscon";
			...

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

	
	Sean


> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index dd249cf5121e..c946aea722e5 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -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;
>  }
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> index 86cec19193c4..200eee5de419 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> @@ -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;
>  

Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[Matthias Brugger]

Hi Sean,

On 18/07/18 04:57, Sean Wang wrote:
> On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote:
>> From: Matthias Brugger <mbrugger@suse.com>
>>
>> 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.

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?

> 
> 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 {
> 		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

> 	
> 	Sean
> 
> 
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>> ---
>>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
>>  drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> index dd249cf5121e..c946aea722e5 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> @@ -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;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>> index 86cec19193c4..200eee5de419 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>> @@ -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;
>>  
> 
> 

Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[Matthias Brugger]

On 18/07/18 09:27, Matthias Brugger wrote:
> Hi Sean,
> 
> On 18/07/18 04:57, Sean Wang wrote:
>> On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote:
>>> From: Matthias Brugger <mbrugger@suse.com>
>>>
>>> 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.
> 
> 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?
> 

I forgot the reference:
[1] https://patchwork.kernel.org/patch/10367797/

>>
>> 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 {
>> 		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
> 
>> 	
>> 	Sean
>>
>>
>>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>>> ---
>>>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
>>>  drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>>> index dd249cf5121e..c946aea722e5 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>>> @@ -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;
>>>  }
>>>  
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>>> index 86cec19193c4..200eee5de419 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>>> @@ -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;
>>>  
>>
>>

Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[Sean Wang]

On Wed, 2018-07-18 at 09:27 +0200, Matthias Brugger wrote:
> Hi Sean,
> 
> On 18/07/18 04:57, Sean Wang wrote:
> > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote:
> >> From: Matthias Brugger <mbrugger@suse.com>
> >>
> >> 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.
> 
> 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?
> 

Yes, my initial thought is that I would like to embed drm platform
device into mmsys. It is the easiest way to solve problems caused by
compatible string conflict we met here in short term.


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

But for long-term goal, device has to define its own dt-binding before
the driver is being implemented. Currently, drm driver uses wrong
compatible string,  "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys",
that are owned by mmsys device [1], not by drm device.

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


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

We can use embed a platform device into mmsys for solving issue caused
by the older device tree binding.

But for long-term, it's neccessary to have a drm device dt-binding
document allowing drm driver and its components driver to follow. 



> > 
> > 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 {
> > 		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
> 
> > 	
> > 	Sean
> > 
> > 
> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> >> ---
> >>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
> >>  drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
> >>  2 files changed, 20 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> >> index dd249cf5121e..c946aea722e5 100644
> >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> >> @@ -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;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> >> index 86cec19193c4..200eee5de419 100644
> >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> >> @@ -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;
> >>  
> > 
> > 

Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[Laurent Pinchart]

Hi Sean,

On Wednesday, 18 July 2018 05:57:35 EEST Sean Wang wrote:
> On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote:
> > From: Matthias Brugger <mbrugger@suse.com>
> > 
> > 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 {
> 		
compatible = "mediatek,mt7623-audsys",
> 		
	     "mediatek,mt2701-audsys",
> 		
	     "syscon";
> 		
	...
> 
> 		
afe: audio-controller {
> 		
	compatible = "mediatek,mt7623-audio",
> 		
		
     "mediatek,mt2701-audio";
> 
> 		
	...
> 		
};
> 	};

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>;
		
..
	};

> > Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> > ---
> > 
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
> >  2 files changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index dd249cf5121e..c946aea722e5
> > 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -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;
> >  
> >  }
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index 86cec19193c4..200eee5de419
> > 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > @@ -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;


-- 
Regards,

Laurent Pinchart

Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[Sean Wang]

On Wed, 2018-07-18 at 11:05 +0300, Laurent Pinchart wrote:
> Hi Sean,
> 
> On Wednesday, 18 July 2018 05:57:35 EEST Sean Wang wrote:
> > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote:
> > > From: Matthias Brugger <mbrugger@suse.com>
> > > 
> > > 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 {
> > 		
> compatible = "mediatek,mt7623-audsys",
> > 		
> 	     "mediatek,mt2701-audsys",
> > 		
> 	     "syscon";
> > 		
> 	...
> > 
> > 		
> afe: audio-controller {
> > 		
> 	compatible = "mediatek,mt7623-audio",
> > 		
> 		
>      "mediatek,mt2701-audio";
> > 
> > 		
> 	...
> > 		
> };
> > 	};
> 
> 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
 
> 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>;
> 		
> ..
> 	};
> 
> > > Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++
> > >  drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
> > >  2 files changed, 20 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index dd249cf5121e..c946aea722e5
> > > 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > @@ -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;
> > >  
> > >  }
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index 86cec19193c4..200eee5de419
> > > 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > > @@ -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;
> 
> 

Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[Stephen Boyd]

Quoting Sean Wang (2018-07-18 03:06:27)
> On Wed, 2018-07-18 at 11:05 +0300, Laurent Pinchart wrote:
> > Hi Sean,
> > 
> > On Wednesday, 18 July 2018 05:57:35 EEST Sean Wang wrote:
> > > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote:
> > > > From: Matthias Brugger <mbrugger@suse.com>
> > > > 
> > > > 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 {
> > >             
> > compatible = "mediatek,mt7623-audsys",
> > >             
> >            "mediatek,mt2701-audsys",
> > >             
> >            "syscon";
> > >             
> >       ...
> > > 
> > >             
> > afe: audio-controller {
> > >             
> >       compatible = "mediatek,mt7623-audio",
> > >             
> >               
> >      "mediatek,mt2701-audio";
> > > 
> > >             
> >       ...
> > >             
> > };
> > >     };
> > 
> > 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
>  

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/

Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev

[Stephen Boyd]

Quoting Stephen Boyd (2018-07-26 00:35:14)
> Quoting Sean Wang (2018-07-18 03:06:27)
> > On Wed, 2018-07-18 at 11:05 +0300, Laurent Pinchart wrote:
> > > 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.
> > 
> > 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/
> 

Please resend this series. This is too old now and I'm dropping this
from (the very tail) of my queue.

[resend PATCH v4 5/5] drm: mediatek: Omit warning on probe defers

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

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 <mbrugger@suse.com>
Acked-by: CK Hu <ck.hu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
index bafc5c77c4fb..6b399348a2dc 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
@@ -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);
 	}
 
-- 
2.17.1

Re: [resend PATCH v4 5/5] drm: mediatek: Omit warning on probe defers

[Sean Wang]

On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> 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.
> 

so if we list drm device as a child device of mmsys we don't need the
patch, and that make more sense.

> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> Acked-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index bafc5c77c4fb..6b399348a2dc 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -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);
>  	}
>