[PATCH v5 00/12] arm/arm64: mediatek: Fix mmsys device probing

[PATCH v5 00/12] arm/arm64: mediatek: Fix mmsys device probing

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

This is version four of the series. The biggest change are the last
four patches which introduce how this should be handled in the future.
Instead of creating the platform device in the DRM driver the device
tree has in the mmsys memory range a child node to probe the clock
part. That breaks backwards compatibility, so I only introduce that for
SoCs which are not available to the general public (mt2712e) or only
have the mmsys clock driver part implemented (mt6797).


Changes since v4:
- fix missing regmap accessors in drm diver (patch 1)
- omit probe deffered warning on all drivers (patch 5)
- update drm and clk bindings (patch 6 and 7)
- put mmsys clock part in dts child node of mmsys. Only done
for HW where no dts backport compatible breakage is expected 
(either DRM driver not yet implemented or no HW available to
the public) (patch 9 to 12)

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.


Matthias Brugger (12):
  drm/mediatek: Use regmap for register access
  clk: mediatek: mt2701-mmsys: switch to platform device probing
  clk: mediatek: mt8173: switch mmsys to platform device probing
  drm/mediatek: Add support for mmsys through a pdev
  drm: mediatek: Omit warning on probe defers
  drm/mediatek: update dt-bindings
  dt-bindings: clock: mediatek: delete mmsys clocks
  dt-bindings: mediatek: Change the binding for mmsys clocks
  arm64: dts: mt2712e: Use the new mmsys clock compatible
  arm64: dts: mt6797: Use the new mmsys clock compatible
  clk: mediatek: mt2712e: Probe with new compatible
  clk: mediatek: mt6797: Probe with new compatible

 .../bindings/arm/mediatek/mediatek,mmsys.txt  | 24 +++++----
 .../display/mediatek/mediatek,disp.txt        | 34 +++++++-----
 arch/arm64/boot/dts/mediatek/mt2712e.dtsi     |  8 ++-
 arch/arm64/boot/dts/mediatek/mt6797.dtsi      |  8 ++-
 drivers/clk/mediatek/clk-mt2701-mm.c          | 42 ++++++++++-----
 drivers/clk/mediatek/clk-mt2712-mm.c          |  9 ++--
 drivers/clk/mediatek/clk-mt6797-mm.c          |  9 ++--
 drivers/clk/mediatek/clk-mt8173.c             | 51 +++++++++++++++---
 drivers/gpu/drm/mediatek/mtk_disp_color.c     |  4 +-
 drivers/gpu/drm/mediatek/mtk_disp_ovl.c       |  4 +-
 drivers/gpu/drm/mediatek/mtk_disp_rdma.c      |  4 +-
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |  4 +-
 drivers/gpu/drm/mediatek/mtk_drm_ddp.c        | 53 ++++++++-----------
 drivers/gpu/drm/mediatek/mtk_drm_ddp.h        |  4 +-
 drivers/gpu/drm/mediatek/mtk_drm_drv.c        | 34 +++++++++---
 drivers/gpu/drm/mediatek/mtk_drm_drv.h        |  4 +-
 drivers/gpu/drm/mediatek/mtk_dsi.c            |  6 ++-
 17 files changed, 200 insertions(+), 102 deletions(-)

-- 
2.19.1

[PATCH v5 01/12] 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  | 50 +++++++++++--------------
 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, 30 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 92ecb9bf982c..cc34660eb946 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
@@ -49,7 +49,7 @@ struct mtk_drm_crtc {
 	unsigned int			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 579ce28d801d..b06cd9d4b525 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
@@ -339,61 +339,53 @@ 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);
 	} else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
-		writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
-			       config_regs + DISP_REG_CONFIG_OUT_SEL);
-		writel_relaxed(DSI_SEL_IN_RDMA,
-			       config_regs + DISP_REG_CONFIG_DSI_SEL);
-		writel_relaxed(DPI_SEL_IN_BLS,
-			       config_regs + DISP_REG_CONFIG_DPI_SEL);
+		regmap_write(config_regs, DISP_REG_CONFIG_OUT_SEL,
+				BLS_TO_DPI_RDMA1_TO_DSI);
+		regmap_write(config_regs, DISP_REG_CONFIG_DSI_SEL,
+				DSI_SEL_IN_RDMA);
+		regmap_write(config_regs, DISP_REG_CONFIG_DPI_SEL,
+				DPI_SEL_IN_BLS);
 	}
 }
 
-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 050adce2ca89..99dd612a6683 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>
@@ -455,7 +456,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;
@@ -469,14 +469,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 9ee8bf1bf886..ab0adbd7d4ee 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -47,7 +47,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.19.1

[PATCH v5 02/12] 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.19.1

[PATCH v5 03/12] 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.19.1

Re: [PATCH v5 03/12] clk: mediatek: mt8173: switch mmsys to platform device probing

[Hsin-Yi Wang]

On Fri, Nov 16, 2018 at 12:54 PM <matthias.bgg@kernel.org> wrote:
>
> 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>
> ---

> +
> +static struct platform_driver clk_mt8173_mm_drv = {
> +       .probe = mtk_mmsys_probe,
> +       .probe = mtk_mmsys_remove,
Should be .remove?

> +       .driver = {
> +               .name = "clk-mt8173-mm",
> +       },
> +};
> +module_platform_driver(clk_mt8173_mm_drv);
>
>  static void __init mtk_vdecsys_init(struct device_node *node)
>  {

Re: [PATCH v5 03/12] clk: mediatek: mt8173: switch mmsys to platform device probing

[Matthias Brugger]

On 31/10/2019 05:17, Hsin-Yi Wang wrote:
> On Fri, Nov 16, 2018 at 12:54 PM <matthias.bgg@kernel.org> wrote:
>>
>> 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>
>> ---
> 
>> +
>> +static struct platform_driver clk_mt8173_mm_drv = {
>> +       .probe = mtk_mmsys_probe,
>> +       .probe = mtk_mmsys_remove,
> Should be .remove?
> 

Yes, definitely.

>> +       .driver = {
>> +               .name = "clk-mt8173-mm",
>> +       },
>> +};
>> +module_platform_driver(clk_mt8173_mm_drv);
>>
>>  static void __init mtk_vdecsys_init(struct device_node *node)
>>  {

[PATCH v5 04/12] 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 | 23 +++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 99dd612a6683..18fc761ba94f 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -199,6 +199,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 mt2712_mmsys_driver_data = {
@@ -215,6 +216,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)
@@ -473,6 +475,24 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	if (IS_ERR(private->config_regs))
 		return PTR_ERR(private->config_regs);
 
+	/*
+	 * For legacy reasons we need to probe the clock driver via
+	 * a platfomr device. This is outdated and should not be used
+	 * in newer SoCs.
+	 */
+	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;
@@ -577,6 +597,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 ab0adbd7d4ee..515ac4cae922 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -37,11 +37,13 @@ struct mtk_mmsys_driver_data {
 	unsigned int third_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.19.1

Re: [PATCH v5 04/12] drm/mediatek: Add support for mmsys through a pdev

[CK Hu]

Hi, Matthias:

On Fri, 2018-11-16 at 13:54 +0100, 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.

Looks good to me except one coding style preference.

Reviewed-by: CK Hu <ck.hu@mediatek.com>

> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 23 +++++++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_drv.h |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 99dd612a6683..18fc761ba94f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -199,6 +199,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 mt2712_mmsys_driver_data = {
> @@ -215,6 +216,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)
> @@ -473,6 +475,24 @@ static int mtk_drm_probe(struct platform_device *pdev)
>  	if (IS_ERR(private->config_regs))
>  		return PTR_ERR(private->config_regs);
>  
> +	/*
> +	 * For legacy reasons we need to probe the clock driver via
> +	 * a platfomr device. This is outdated and should not be used
> +	 * in newer SoCs.
> +	 */
> +	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);

I would like to align to the right of '('.

Regards,
CK

> +
> +			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;
> @@ -577,6 +597,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 ab0adbd7d4ee..515ac4cae922 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> @@ -37,11 +37,13 @@ struct mtk_mmsys_driver_data {
>  	unsigned int third_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;
>  

[PATCH v5 05/12] drm: mediatek: Omit warning on probe defers

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

It can happen that the mmsys clock drivers aren't probed before the
platform driver gets invoked. The platform driver used to print 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>
---
 drivers/gpu/drm/mediatek/mtk_disp_color.c | 4 +++-
 drivers/gpu/drm/mediatek/mtk_disp_ovl.c   | 4 +++-
 drivers/gpu/drm/mediatek/mtk_disp_rdma.c  | 4 +++-
 drivers/gpu/drm/mediatek/mtk_drm_ddp.c    | 3 ++-
 drivers/gpu/drm/mediatek/mtk_dsi.c        | 6 ++++--
 5 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_color.c b/drivers/gpu/drm/mediatek/mtk_disp_color.c
index f609b62b8be6..1ea3178d4c18 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_color.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_color.c
@@ -126,7 +126,9 @@ static int mtk_disp_color_probe(struct platform_device *pdev)
 	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
 				&mtk_disp_color_funcs);
 	if (ret) {
-		dev_err(dev, "Failed to initialize component: %d\n", ret);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to initialize component: %d\n",
+					ret);
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
index 28d191192945..5ebbcaa4e70e 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
@@ -293,7 +293,9 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
 	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
 				&mtk_disp_ovl_funcs);
 	if (ret) {
-		dev_err(dev, "Failed to initialize component: %d\n", ret);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to initialize component: %d\n",
+					ret);
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
index b0a5cffe345a..59a08ed5fea5 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
@@ -295,7 +295,9 @@ static int mtk_disp_rdma_probe(struct platform_device *pdev)
 	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
 				&mtk_disp_rdma_funcs);
 	if (ret) {
-		dev_err(dev, "Failed to initialize component: %d\n", ret);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to initialize component: %d\n",
+					ret);
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
index b06cd9d4b525..b76a2d071a97 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
@@ -566,7 +566,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);
 	}
 
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 90109a0d6fff..cc6de75636c3 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -1103,14 +1103,16 @@ static int mtk_dsi_probe(struct platform_device *pdev)
 	dsi->engine_clk = devm_clk_get(dev, "engine");
 	if (IS_ERR(dsi->engine_clk)) {
 		ret = PTR_ERR(dsi->engine_clk);
-		dev_err(dev, "Failed to get engine clock: %d\n", ret);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get engine clock: %d\n", ret);
 		return ret;
 	}
 
 	dsi->digital_clk = devm_clk_get(dev, "digital");
 	if (IS_ERR(dsi->digital_clk)) {
 		ret = PTR_ERR(dsi->digital_clk);
-		dev_err(dev, "Failed to get digital clock: %d\n", ret);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get digital clock: %d\n", ret);
 		return ret;
 	}
 
-- 
2.19.1

Re: [PATCH v5 05/12] drm: mediatek: Omit warning on probe defers

[CK Hu]

Hi, Matthias:

On Fri, 2018-11-16 at 13:54 +0100, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> It can happen that the mmsys clock drivers aren't probed before the
> platform driver gets invoked. The platform driver used to print a warning
> that the driver failed to get the clocks. Omit this error on
> the defered probe path.

This patch looks good to me, but you have not modified the sub driver in
HDMI path. We could let HDMI path print the warning and someone send
another patch later, or you modify for HDMI path in this patch.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_color.c | 4 +++-
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c   | 4 +++-
>  drivers/gpu/drm/mediatek/mtk_disp_rdma.c  | 4 +++-
>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c    | 3 ++-
>  drivers/gpu/drm/mediatek/mtk_dsi.c        | 6 ++++--
>  5 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_color.c b/drivers/gpu/drm/mediatek/mtk_disp_color.c
> index f609b62b8be6..1ea3178d4c18 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_color.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_color.c
> @@ -126,7 +126,9 @@ static int mtk_disp_color_probe(struct platform_device *pdev)
>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
>  				&mtk_disp_color_funcs);
>  	if (ret) {
> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to initialize component: %d\n",
> +					ret);

I would like one more blank line here.

>  		return ret;
>  	}
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 28d191192945..5ebbcaa4e70e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -293,7 +293,9 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
>  				&mtk_disp_ovl_funcs);
>  	if (ret) {
> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to initialize component: %d\n",
> +					ret);

I would like to align to the right of '('.

Regards,
CK

>  		return ret;
>  	}
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> index b0a5cffe345a..59a08ed5fea5 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> @@ -295,7 +295,9 @@ static int mtk_disp_rdma_probe(struct platform_device *pdev)
>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
>  				&mtk_disp_rdma_funcs);
>  	if (ret) {
> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to initialize component: %d\n",
> +					ret);
>  		return ret;
>  	}
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index b06cd9d4b525..b76a2d071a97 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -566,7 +566,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);
>  	}
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 90109a0d6fff..cc6de75636c3 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1103,14 +1103,16 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>  	dsi->engine_clk = devm_clk_get(dev, "engine");
>  	if (IS_ERR(dsi->engine_clk)) {
>  		ret = PTR_ERR(dsi->engine_clk);
> -		dev_err(dev, "Failed to get engine clock: %d\n", ret);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get engine clock: %d\n", ret);
>  		return ret;
>  	}
>  
>  	dsi->digital_clk = devm_clk_get(dev, "digital");
>  	if (IS_ERR(dsi->digital_clk)) {
>  		ret = PTR_ERR(dsi->digital_clk);
> -		dev_err(dev, "Failed to get digital clock: %d\n", ret);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get digital clock: %d\n", ret);
>  		return ret;
>  	}
>  

Re: [PATCH v5 05/12] drm: mediatek: Omit warning on probe defers

[Matthias Brugger]

On 19/11/2018 06:38, CK Hu wrote:
> Hi, Matthias:
> 
> On Fri, 2018-11-16 at 13:54 +0100, matthias.bgg@kernel.org wrote:
>> From: Matthias Brugger <mbrugger@suse.com>
>>
>> It can happen that the mmsys clock drivers aren't probed before the
>> platform driver gets invoked. The platform driver used to print a warning
>> that the driver failed to get the clocks. Omit this error on
>> the defered probe path.
> 
> This patch looks good to me, but you have not modified the sub driver in
> HDMI path. We could let HDMI path print the warning and someone send
> another patch later, or you modify for HDMI path in this patch.

Sure, I'll add this in v6. After inspecting the code, I think we will need to
also check for not initialized clocks in mtk_mdp_comp_init, as the driver for
now does not even check if the clocks are present. What do you think?

I'll address the coding style issue you metioned below as well.

Regards,
Matthias

>>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>> ---
>>  drivers/gpu/drm/mediatek/mtk_disp_color.c | 4 +++-
>>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c   | 4 +++-
>>  drivers/gpu/drm/mediatek/mtk_disp_rdma.c  | 4 +++-
>>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c    | 3 ++-
>>  drivers/gpu/drm/mediatek/mtk_dsi.c        | 6 ++++--
>>  5 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_color.c b/drivers/gpu/drm/mediatek/mtk_disp_color.c
>> index f609b62b8be6..1ea3178d4c18 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_disp_color.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_color.c
>> @@ -126,7 +126,9 @@ static int mtk_disp_color_probe(struct platform_device *pdev)
>>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
>>  				&mtk_disp_color_funcs);
>>  	if (ret) {
>> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to initialize component: %d\n",
>> +					ret);
> 
> I would like one more blank line here.
> 
>>  		return ret;
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
>> index 28d191192945..5ebbcaa4e70e 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
>> @@ -293,7 +293,9 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
>>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
>>  				&mtk_disp_ovl_funcs);
>>  	if (ret) {
>> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to initialize component: %d\n",
>> +					ret);
> 
> I would like to align to the right of '('.
> 
> Regards,
> CK
> 
>>  		return ret;
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
>> index b0a5cffe345a..59a08ed5fea5 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
>> @@ -295,7 +295,9 @@ static int mtk_disp_rdma_probe(struct platform_device *pdev)
>>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
>>  				&mtk_disp_rdma_funcs);
>>  	if (ret) {
>> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to initialize component: %d\n",
>> +					ret);
>>  		return ret;
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
>> index b06cd9d4b525..b76a2d071a97 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
>> @@ -566,7 +566,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);
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index 90109a0d6fff..cc6de75636c3 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -1103,14 +1103,16 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>>  	dsi->engine_clk = devm_clk_get(dev, "engine");
>>  	if (IS_ERR(dsi->engine_clk)) {
>>  		ret = PTR_ERR(dsi->engine_clk);
>> -		dev_err(dev, "Failed to get engine clock: %d\n", ret);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to get engine clock: %d\n", ret);
>>  		return ret;
>>  	}
>>  
>>  	dsi->digital_clk = devm_clk_get(dev, "digital");
>>  	if (IS_ERR(dsi->digital_clk)) {
>>  		ret = PTR_ERR(dsi->digital_clk);
>> -		dev_err(dev, "Failed to get digital clock: %d\n", ret);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to get digital clock: %d\n", ret);
>>  		return ret;
>>  	}
>>  
> 
> 

Re: [PATCH v5 05/12] drm: mediatek: Omit warning on probe defers

[CK Hu]

Hi, Matthias:

On Mon, 2018-11-19 at 10:26 +0100, Matthias Brugger wrote:
> 
> On 19/11/2018 06:38, CK Hu wrote:
> > Hi, Matthias:
> > 
> > On Fri, 2018-11-16 at 13:54 +0100, matthias.bgg@kernel.org wrote:
> >> From: Matthias Brugger <mbrugger@suse.com>
> >>
> >> It can happen that the mmsys clock drivers aren't probed before the
> >> platform driver gets invoked. The platform driver used to print a warning
> >> that the driver failed to get the clocks. Omit this error on
> >> the defered probe path.
> > 
> > This patch looks good to me, but you have not modified the sub driver in
> > HDMI path. We could let HDMI path print the warning and someone send
> > another patch later, or you modify for HDMI path in this patch.
> 
> Sure, I'll add this in v6. After inspecting the code, I think we will need to
> also check for not initialized clocks in mtk_mdp_comp_init, as the driver for
> now does not even check if the clocks are present. What do you think?

Yes, we do really need to consider mdp driver because mmsys clock
include mdp clock. You remind me that mmsys control 4 major function:
drm routing, drm clock, mdp routing, and mdp clock. Your design let the
mmsys device as drm device (control drm routing) and create a sub device
as clock device (control drm clock, mdp clock). If one day mdp device
(may need control drm routing) need to control the register of mdp
routing, would mdp device be a sub device? Or we need not to consider
this because it need not to access mmsys register now?

Regards,
CK

> 
> I'll address the coding style issue you metioned below as well.
> 
> Regards,
> Matthias
> 
> >>
> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> >> ---
> >>  drivers/gpu/drm/mediatek/mtk_disp_color.c | 4 +++-
> >>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c   | 4 +++-
> >>  drivers/gpu/drm/mediatek/mtk_disp_rdma.c  | 4 +++-
> >>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c    | 3 ++-
> >>  drivers/gpu/drm/mediatek/mtk_dsi.c        | 6 ++++--
> >>  5 files changed, 15 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_color.c b/drivers/gpu/drm/mediatek/mtk_disp_color.c
> >> index f609b62b8be6..1ea3178d4c18 100644
> >> --- a/drivers/gpu/drm/mediatek/mtk_disp_color.c
> >> +++ b/drivers/gpu/drm/mediatek/mtk_disp_color.c
> >> @@ -126,7 +126,9 @@ static int mtk_disp_color_probe(struct platform_device *pdev)
> >>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
> >>  				&mtk_disp_color_funcs);
> >>  	if (ret) {
> >> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> >> +		if (ret != -EPROBE_DEFER)
> >> +			dev_err(dev, "Failed to initialize component: %d\n",
> >> +					ret);
> > 
> > I would like one more blank line here.
> > 
> >>  		return ret;
> >>  	}
> >>  
> >> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> >> index 28d191192945..5ebbcaa4e70e 100644
> >> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> >> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> >> @@ -293,7 +293,9 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
> >>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
> >>  				&mtk_disp_ovl_funcs);
> >>  	if (ret) {
> >> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> >> +		if (ret != -EPROBE_DEFER)
> >> +			dev_err(dev, "Failed to initialize component: %d\n",
> >> +					ret);
> > 
> > I would like to align to the right of '('.
> > 
> > Regards,
> > CK
> > 
> >>  		return ret;
> >>  	}
> >>  
> >> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> >> index b0a5cffe345a..59a08ed5fea5 100644
> >> --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> >> +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> >> @@ -295,7 +295,9 @@ static int mtk_disp_rdma_probe(struct platform_device *pdev)
> >>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
> >>  				&mtk_disp_rdma_funcs);
> >>  	if (ret) {
> >> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> >> +		if (ret != -EPROBE_DEFER)
> >> +			dev_err(dev, "Failed to initialize component: %d\n",
> >> +					ret);
> >>  		return ret;
> >>  	}
> >>  
> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> >> index b06cd9d4b525..b76a2d071a97 100644
> >> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> >> @@ -566,7 +566,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);
> >>  	}
> >>  
> >> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> >> index 90109a0d6fff..cc6de75636c3 100644
> >> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> >> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> >> @@ -1103,14 +1103,16 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> >>  	dsi->engine_clk = devm_clk_get(dev, "engine");
> >>  	if (IS_ERR(dsi->engine_clk)) {
> >>  		ret = PTR_ERR(dsi->engine_clk);
> >> -		dev_err(dev, "Failed to get engine clock: %d\n", ret);
> >> +		if (ret != -EPROBE_DEFER)
> >> +			dev_err(dev, "Failed to get engine clock: %d\n", ret);
> >>  		return ret;
> >>  	}
> >>  
> >>  	dsi->digital_clk = devm_clk_get(dev, "digital");
> >>  	if (IS_ERR(dsi->digital_clk)) {
> >>  		ret = PTR_ERR(dsi->digital_clk);
> >> -		dev_err(dev, "Failed to get digital clock: %d\n", ret);
> >> +		if (ret != -EPROBE_DEFER)
> >> +			dev_err(dev, "Failed to get digital clock: %d\n", ret);
> >>  		return ret;
> >>  	}
> >>  
> > 
> > 

Re: [PATCH v5 05/12] drm: mediatek: Omit warning on probe defers

[CK Hu]

Hi,

On Tue, 2018-11-20 at 12:05 +0800, CK Hu wrote:
> Hi, Matthias:
> 
> On Mon, 2018-11-19 at 10:26 +0100, Matthias Brugger wrote:
> > 
> > On 19/11/2018 06:38, CK Hu wrote:
> > > Hi, Matthias:
> > > 
> > > On Fri, 2018-11-16 at 13:54 +0100, matthias.bgg@kernel.org wrote:
> > >> From: Matthias Brugger <mbrugger@suse.com>
> > >>
> > >> It can happen that the mmsys clock drivers aren't probed before the
> > >> platform driver gets invoked. The platform driver used to print a warning
> > >> that the driver failed to get the clocks. Omit this error on
> > >> the defered probe path.
> > > 
> > > This patch looks good to me, but you have not modified the sub driver in
> > > HDMI path. We could let HDMI path print the warning and someone send
> > > another patch later, or you modify for HDMI path in this patch.
> > 
> > Sure, I'll add this in v6. After inspecting the code, I think we will need to
> > also check for not initialized clocks in mtk_mdp_comp_init, as the driver for
> > now does not even check if the clocks are present. What do you think?
> 
> Yes, we do really need to consider mdp driver because mmsys clock
> include mdp clock. You remind me that mmsys control 4 major function:
> drm routing, drm clock, mdp routing, and mdp clock. Your design let the
> mmsys device as drm device (control drm routing) and create a sub device
> as clock device (control drm clock, mdp clock). If one day mdp device
> (may need control drm routing) need to control the register of mdp

Sorry for typo: 'mdp device (may need control mdp routing)'

Regards,
CK

> routing, would mdp device be a sub device? Or we need not to consider
> this because it need not to access mmsys register now?
> 
> Regards,
> CK
> 
> > 
> > I'll address the coding style issue you metioned below as well.
> > 
> > Regards,
> > Matthias
> > 
> > >>
> > >> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> > >> ---
> > >>  drivers/gpu/drm/mediatek/mtk_disp_color.c | 4 +++-
> > >>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c   | 4 +++-
> > >>  drivers/gpu/drm/mediatek/mtk_disp_rdma.c  | 4 +++-
> > >>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c    | 3 ++-
> > >>  drivers/gpu/drm/mediatek/mtk_dsi.c        | 6 ++++--
> > >>  5 files changed, 15 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_color.c b/drivers/gpu/drm/mediatek/mtk_disp_color.c
> > >> index f609b62b8be6..1ea3178d4c18 100644
> > >> --- a/drivers/gpu/drm/mediatek/mtk_disp_color.c
> > >> +++ b/drivers/gpu/drm/mediatek/mtk_disp_color.c
> > >> @@ -126,7 +126,9 @@ static int mtk_disp_color_probe(struct platform_device *pdev)
> > >>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
> > >>  				&mtk_disp_color_funcs);
> > >>  	if (ret) {
> > >> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> > >> +		if (ret != -EPROBE_DEFER)
> > >> +			dev_err(dev, "Failed to initialize component: %d\n",
> > >> +					ret);
> > > 
> > > I would like one more blank line here.
> > > 
> > >>  		return ret;
> > >>  	}
> > >>  
> > >> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > >> index 28d191192945..5ebbcaa4e70e 100644
> > >> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > >> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > >> @@ -293,7 +293,9 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
> > >>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
> > >>  				&mtk_disp_ovl_funcs);
> > >>  	if (ret) {
> > >> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> > >> +		if (ret != -EPROBE_DEFER)
> > >> +			dev_err(dev, "Failed to initialize component: %d\n",
> > >> +					ret);
> > > 
> > > I would like to align to the right of '('.
> > > 
> > > Regards,
> > > CK
> > > 
> > >>  		return ret;
> > >>  	}
> > >>  
> > >> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > >> index b0a5cffe345a..59a08ed5fea5 100644
> > >> --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > >> +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > >> @@ -295,7 +295,9 @@ static int mtk_disp_rdma_probe(struct platform_device *pdev)
> > >>  	ret = mtk_ddp_comp_init(dev, dev->of_node, &priv->ddp_comp, comp_id,
> > >>  				&mtk_disp_rdma_funcs);
> > >>  	if (ret) {
> > >> -		dev_err(dev, "Failed to initialize component: %d\n", ret);
> > >> +		if (ret != -EPROBE_DEFER)
> > >> +			dev_err(dev, "Failed to initialize component: %d\n",
> > >> +					ret);
> > >>  		return ret;
> > >>  	}
> > >>  
> > >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > >> index b06cd9d4b525..b76a2d071a97 100644
> > >> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > >> @@ -566,7 +566,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);
> > >>  	}
> > >>  
> > >> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > >> index 90109a0d6fff..cc6de75636c3 100644
> > >> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > >> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > >> @@ -1103,14 +1103,16 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> > >>  	dsi->engine_clk = devm_clk_get(dev, "engine");
> > >>  	if (IS_ERR(dsi->engine_clk)) {
> > >>  		ret = PTR_ERR(dsi->engine_clk);
> > >> -		dev_err(dev, "Failed to get engine clock: %d\n", ret);
> > >> +		if (ret != -EPROBE_DEFER)
> > >> +			dev_err(dev, "Failed to get engine clock: %d\n", ret);
> > >>  		return ret;
> > >>  	}
> > >>  
> > >>  	dsi->digital_clk = devm_clk_get(dev, "digital");
> > >>  	if (IS_ERR(dsi->digital_clk)) {
> > >>  		ret = PTR_ERR(dsi->digital_clk);
> > >> -		dev_err(dev, "Failed to get digital clock: %d\n", ret);
> > >> +		if (ret != -EPROBE_DEFER)
> > >> +			dev_err(dev, "Failed to get digital clock: %d\n", ret);
> > >>  		return ret;
> > >>  	}
> > >>  
> > > 
> > > 
> 

Aw: Re: [PATCH v5 05/12] drm: mediatek

[Frank Wunderlich]

Hi,

i got v5-patches working on bpi-r2 (mt7623) with a Patch from Ryder.lee and 2 from Bibby Hsieh on 4.19-rc1

https://github.com/frank-w/BPI-R2-4.14/commits/4.19-hdmiv5

but after i tried to include them on 4.19.2, i got a strange behaviour (stretched and pink font instead of white/grey)

see here for detailed info:

http://forum.banana-pi.org/t/kernel-4-19-rc1-for-testers/6618/75

there are some patches between rc1 and final which i tried to revert, but no luck till now (cannot revert, no effect on the issue or no output). Had informed Bibby Hsieh...

is there any idea, why this happen?

regards Frank

Re: Aw: Re: [PATCH v5 05/12] drm: mediatek

[Matthias Brugger]

On 20/11/2018 09:26, Frank Wunderlich wrote:
> Hi,
> 
> i got v5-patches working on bpi-r2 (mt7623) with a Patch from Ryder.lee and 2 from Bibby Hsieh on 4.19-rc1
> 
> https://github.com/frank-w/BPI-R2-4.14/commits/4.19-hdmiv5
> 

I don't see the patches applied to this tree. Apart from that this tree has a
lot of other patches applied. It differs greatly from mainline, so nothing we
should discuss on this mailinglist.

Regards,
Matthias

> but after i tried to include them on 4.19.2, i got a strange behaviour (stretched and pink font instead of white/grey)
> 
> see here for detailed info:
> 
> http://forum.banana-pi.org/t/kernel-4-19-rc1-for-testers/6618/75
> 
> there are some patches between rc1 and final which i tried to revert, but no luck till now (cannot revert, no effect on the issue or no output). Had informed Bibby Hsieh...
> 
> is there any idea, why this happen?
> 
> regards Frank
> 

Aw: Re: Re: [PATCH v5 05/12] drm: mediatek

[Frank Wunderlich]

right this branch based on rc1 with some other commits, but v5-patches are applied on Oct 3rd (listed in Oct 5th)

frank@frank-N56VZ:/media/data_ext/bpi-r2-kernel/github
[11:29:10]$ git checkout 4.19-hdmiv5
Already on '4.19-hdmiv5'
Your branch is up to date with 'origin/4.19-hdmiv5'.
frank@frank-N56VZ:/media/data_ext/bpi-r2-kernel/github
[11:29:13]$ make kernelversion
4.19.0-rc1
frank@frank-N56VZ:/media/data_ext/bpi-r2-kernel/github
[11:29:19]$ git log --oneline
...
fc14b8c515de drm/mediatek: add a error return value when clock driver has been prepared
0dc856306aaf drm/mediatek: implement connection from BLS to DPI0
3d0a6749bfe3 drm/mediatek: add hdmi driver for MT2701 and MT7623
b03e1b353b28 drm/mediatek: add support for SPDIF audio in HDMI
c5564966272e drm/mediatek: separate hdmi phy to different file
dee3954828db drm/mediatek: add dpi driver for mt2701 and mt7623
a838ae8b415c drm/mediatek: convert dpi driver to use drm_of_find_panel_or_bridge
f7f9f7c080ae drm/mediatek: add clock factor for different IC
be1a5447330f drm/mediatek: adjust EDGE to match clock and data
fec4504ea318 drm/mediatek: move hardware register to node data
db992e429b9c drm/mediatek: add refcount for DPI power on/off
...

compared with 4.19.0-release there is merge https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?id=53b3b6bbfde6aae8d1ededc86ad4e0e1e00eb5f8 which seems to break v5-patches

> Gesendet: Dienstag, 20. November 2018 um 11:14 Uhr
> Von: "Matthias Brugger" <matthias.bgg@gmail.com>
> An: "Frank Wunderlich" <frank-w@public-files.de>, "CK Hu" <ck.hu@mediatek.com>
> Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, sean.wang@kernel.org, "Matthias Brugger" <mbrugger@suse.com>, airlied@linux.ie, mturquette@baylibre.com, sean.wang@mediatek.com, sboyd@codeaurora.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, wens@csie.org, robh+dt@kernel.org, rdunlap@infradead.org, laurent.pinchart@ideasonboard.com, p.zabel@pengutronix.de, matthias.bgg@kernel.org, ulrich.hecht+renesas@gmail.com, linux-mediatek@lists.infradead.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> Betreff: Re: Aw: Re: [PATCH v5 05/12] drm: mediatek
>
> 
> 
> On 20/11/2018 09:26, Frank Wunderlich wrote:
> > Hi,
> > 
> > i got v5-patches working on bpi-r2 (mt7623) with a Patch from Ryder.lee and 2 from Bibby Hsieh on 4.19-rc1
> > 
> > https://github.com/frank-w/BPI-R2-4.14/commits/4.19-hdmiv5
> > 
> 
> I don't see the patches applied to this tree. Apart from that this tree has a
> lot of other patches applied. It differs greatly from mainline, so nothing we
> should discuss on this mailinglist.
> 
> Regards,
> Matthias
> 
> > but after i tried to include them on 4.19.2, i got a strange behaviour (stretched and pink font instead of white/grey)
> > 
> > see here for detailed info:
> > 
> > http://forum.banana-pi.org/t/kernel-4-19-rc1-for-testers/6618/75
> > 
> > there are some patches between rc1 and final which i tried to revert, but no luck till now (cannot revert, no effect on the issue or no output). Had informed Bibby Hsieh...
> > 
> > is there any idea, why this happen?
> > 
> > regards Frank
> > 
> 

Re: Aw: Re: Re: [PATCH v5 05/12] drm: mediatek

[Matthias Brugger]

On 20/11/2018 11:34, Frank Wunderlich wrote:
> right this branch based on rc1 with some other commits, but v5-patches are applied on Oct 3rd (listed in Oct 5th)
> 
> frank@frank-N56VZ:/media/data_ext/bpi-r2-kernel/github
> [11:29:10]$ git checkout 4.19-hdmiv5
> Already on '4.19-hdmiv5'
> Your branch is up to date with 'origin/4.19-hdmiv5'.
> frank@frank-N56VZ:/media/data_ext/bpi-r2-kernel/github
> [11:29:13]$ make kernelversion
> 4.19.0-rc1
> frank@frank-N56VZ:/media/data_ext/bpi-r2-kernel/github
> [11:29:19]$ git log --oneline
> ...
> fc14b8c515de drm/mediatek: add a error return value when clock driver has been prepared
> 0dc856306aaf drm/mediatek: implement connection from BLS to DPI0
> 3d0a6749bfe3 drm/mediatek: add hdmi driver for MT2701 and MT7623
> b03e1b353b28 drm/mediatek: add support for SPDIF audio in HDMI
> c5564966272e drm/mediatek: separate hdmi phy to different file
> dee3954828db drm/mediatek: add dpi driver for mt2701 and mt7623
> a838ae8b415c drm/mediatek: convert dpi driver to use drm_of_find_panel_or_bridge
> f7f9f7c080ae drm/mediatek: add clock factor for different IC
> be1a5447330f drm/mediatek: adjust EDGE to match clock and data
> fec4504ea318 drm/mediatek: move hardware register to node data
> db992e429b9c drm/mediatek: add refcount for DPI power on/off
> ...

This patches are not part of this patch series.

> 
> compared with 4.19.0-release there is merge https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?id=53b3b6bbfde6aae8d1ededc86ad4e0e1e00eb5f8 which seems to break v5-patches
> 
>> Gesendet: Dienstag, 20. November 2018 um 11:14 Uhr
>> Von: "Matthias Brugger" <matthias.bgg@gmail.com>
>> An: "Frank Wunderlich" <frank-w@public-files.de>, "CK Hu" <ck.hu@mediatek.com>
>> Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, sean.wang@kernel.org, "Matthias Brugger" <mbrugger@suse.com>, airlied@linux.ie, mturquette@baylibre.com, sean.wang@mediatek.com, sboyd@codeaurora.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, wens@csie.org, robh+dt@kernel.org, rdunlap@infradead.org, laurent.pinchart@ideasonboard.com, p.zabel@pengutronix.de, matthias.bgg@kernel.org, ulrich.hecht+renesas@gmail.com, linux-mediatek@lists.infradead.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org
>> Betreff: Re: Aw: Re: [PATCH v5 05/12] drm: mediatek
>>
>>
>>
>> On 20/11/2018 09:26, Frank Wunderlich wrote:
>>> Hi,
>>>
>>> i got v5-patches working on bpi-r2 (mt7623) with a Patch from Ryder.lee and 2 from Bibby Hsieh on 4.19-rc1
>>>
>>> https://github.com/frank-w/BPI-R2-4.14/commits/4.19-hdmiv5
>>>
>>
>> I don't see the patches applied to this tree. Apart from that this tree has a
>> lot of other patches applied. It differs greatly from mainline, so nothing we
>> should discuss on this mailinglist.
>>
>> Regards,
>> Matthias
>>
>>> but after i tried to include them on 4.19.2, i got a strange behaviour (stretched and pink font instead of white/grey)
>>>
>>> see here for detailed info:
>>>
>>> http://forum.banana-pi.org/t/kernel-4-19-rc1-for-testers/6618/75
>>>
>>> there are some patches between rc1 and final which i tried to revert, but no luck till now (cannot revert, no effect on the issue or no output). Had informed Bibby Hsieh...
>>>
>>> is there any idea, why this happen?
>>>
>>> regards Frank
>>>
>>

Re: [PATCH v5 05/12] drm: mediatek: Omit warning on probe defers

[Matthias Brugger]

On 20/11/2018 05:05, CK Hu wrote:
> Hi, Matthias:
> 
> On Mon, 2018-11-19 at 10:26 +0100, Matthias Brugger wrote:
>>
>> On 19/11/2018 06:38, CK Hu wrote:
>>> Hi, Matthias:
>>>
>>> On Fri, 2018-11-16 at 13:54 +0100, matthias.bgg@kernel.org wrote:
>>>> From: Matthias Brugger <mbrugger@suse.com>
>>>>
>>>> It can happen that the mmsys clock drivers aren't probed before the
>>>> platform driver gets invoked. The platform driver used to print a warning
>>>> that the driver failed to get the clocks. Omit this error on
>>>> the defered probe path.
>>>
>>> This patch looks good to me, but you have not modified the sub driver in
>>> HDMI path. We could let HDMI path print the warning and someone send
>>> another patch later, or you modify for HDMI path in this patch.
>>
>> Sure, I'll add this in v6. After inspecting the code, I think we will need to
>> also check for not initialized clocks in mtk_mdp_comp_init, as the driver for
>> now does not even check if the clocks are present. What do you think?
> 
> Yes, we do really need to consider mdp driver because mmsys clock
> include mdp clock. You remind me that mmsys control 4 major function:
> drm routing, drm clock, mdp routing, and mdp clock. Your design let the
> mmsys device as drm device (control drm routing) and create a sub device
> as clock device (control drm clock, mdp clock). If one day mdp device
> (may need control drm routing) need to control the register of mdp
> routing, would mdp device be a sub device? Or we need not to consider
> this because it need not to access mmsys register now?
> 

I think we should for now concentrate to fix the clock probing issue. If in the
future we will need to access drm routing from the mdp device, we can have a
look into this.

Sounds good?
Regards,
Matthias

Re: [PATCH v5 05/12] drm: mediatek: Omit warning on probe defers

[CK Hu]

Hi, Matthias:

On Tue, 2018-11-20 at 11:19 +0100, Matthias Brugger wrote:
> 
> On 20/11/2018 05:05, CK Hu wrote:
> > Hi, Matthias:
> > 
> > On Mon, 2018-11-19 at 10:26 +0100, Matthias Brugger wrote:
> >>
> >> On 19/11/2018 06:38, CK Hu wrote:
> >>> Hi, Matthias:
> >>>
> >>> On Fri, 2018-11-16 at 13:54 +0100, matthias.bgg@kernel.org wrote:
> >>>> From: Matthias Brugger <mbrugger@suse.com>
> >>>>
> >>>> It can happen that the mmsys clock drivers aren't probed before the
> >>>> platform driver gets invoked. The platform driver used to print a warning
> >>>> that the driver failed to get the clocks. Omit this error on
> >>>> the defered probe path.
> >>>
> >>> This patch looks good to me, but you have not modified the sub driver in
> >>> HDMI path. We could let HDMI path print the warning and someone send
> >>> another patch later, or you modify for HDMI path in this patch.
> >>
> >> Sure, I'll add this in v6. After inspecting the code, I think we will need to
> >> also check for not initialized clocks in mtk_mdp_comp_init, as the driver for
> >> now does not even check if the clocks are present. What do you think?
> > 
> > Yes, we do really need to consider mdp driver because mmsys clock
> > include mdp clock. You remind me that mmsys control 4 major function:
> > drm routing, drm clock, mdp routing, and mdp clock. Your design let the
> > mmsys device as drm device (control drm routing) and create a sub device
> > as clock device (control drm clock, mdp clock). If one day mdp device
> > (may need control drm routing) need to control the register of mdp
> > routing, would mdp device be a sub device? Or we need not to consider
> > this because it need not to access mmsys register now?
> > 
> 
> I think we should for now concentrate to fix the clock probing issue. If in the
> future we will need to access drm routing from the mdp device, we can have a
> look into this.
> 
> Sounds good?

Sounds good to me.

Regards,
CK

> Regards,
> Matthias

[PATCH v5 06/12] drm/mediatek: update dt-bindings

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

Add mmsys bindings description.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 .../display/mediatek/mediatek,disp.txt        | 30 +++++++++++--------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
index 8469de510001..4b008d992398 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
@@ -27,20 +27,24 @@ Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
 
 Required properties (all function blocks):
 - compatible: "mediatek,<chip>-disp-<function>", one of
-	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
-	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
-	"mediatek,<chip>-disp-wdma"  - write DMA
-	"mediatek,<chip>-disp-color" - color processor
-	"mediatek,<chip>-disp-aal"   - adaptive ambient light controller
-	"mediatek,<chip>-disp-gamma" - gamma correction
-	"mediatek,<chip>-disp-merge" - merge streams from two RDMA sources
-	"mediatek,<chip>-disp-split" - split stream to two encoders
-	"mediatek,<chip>-disp-ufoe"  - data compression engine
-	"mediatek,<chip>-dsi"        - DSI controller, see mediatek,dsi.txt
-	"mediatek,<chip>-dpi"        - DPI controller, see mediatek,dpi.txt
-	"mediatek,<chip>-disp-mutex" - display mutex
-	"mediatek,<chip>-disp-od"    - overdrive
+	"mediatek,<chip>-disp-ovl"		- overlay (4 layers, blending, csc)
+	"mediatek,<chip>-disp-rdma"		- read DMA / line buffer
+	"mediatek,<chip>-disp-wdma"		- write DMA
+	"mediatek,<chip>-disp-color"		- color processor
+	"mediatek,<chip>-disp-aal"		- adaptive ambient light controller
+	"mediatek,<chip>-disp-gamma"		- gamma correction
+	"mediatek,<chip>-disp-merge"		- merge streams from two RDMA sources
+	"mediatek,<chip>-disp-split"		- split stream to two encoders
+	"mediatek,<chip>-disp-ufoe"		- data compression engine
+	"mediatek,<chip>-dsi"			- DSI controller, see mediatek,dsi.txt
+	"mediatek,<chip>-dpi"			- DPI controller, see mediatek,dpi.txt
+	"mediatek,<chip>-disp-mutex"		- display mutex
+	"mediatek,<chip>-disp-od"		- overdrive
+	"mediatek,<chip>-mmsys", "syscon"	- provide clocks and components management
   the supported chips are mt2701, mt2712 and mt8173.
+  For mt7623, compatible must be:
+	"mediatek,mt7623-<component>" , "mediatek,mt2701-<component>"
+
 - reg: Physical base address and length of the function block register space
 - interrupts: The interrupt signal from the function block (required, except for
   merge and split function blocks).
-- 
2.19.1

Re: [PATCH v5 06/12] drm/mediatek: update dt-bindings

[Rob Herring]

On Fri, Nov 16, 2018 at 01:54:43PM +0100, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>

The subject is pretty vague...

> 
> Add mmsys bindings description.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  .../display/mediatek/mediatek,disp.txt        | 30 +++++++++++--------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index 8469de510001..4b008d992398 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -27,20 +27,24 @@ Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
>  
>  Required properties (all function blocks):
>  - compatible: "mediatek,<chip>-disp-<function>", one of
> -	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
> -	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
> -	"mediatek,<chip>-disp-wdma"  - write DMA
> -	"mediatek,<chip>-disp-color" - color processor
> -	"mediatek,<chip>-disp-aal"   - adaptive ambient light controller
> -	"mediatek,<chip>-disp-gamma" - gamma correction
> -	"mediatek,<chip>-disp-merge" - merge streams from two RDMA sources
> -	"mediatek,<chip>-disp-split" - split stream to two encoders
> -	"mediatek,<chip>-disp-ufoe"  - data compression engine
> -	"mediatek,<chip>-dsi"        - DSI controller, see mediatek,dsi.txt
> -	"mediatek,<chip>-dpi"        - DPI controller, see mediatek,dpi.txt
> -	"mediatek,<chip>-disp-mutex" - display mutex
> -	"mediatek,<chip>-disp-od"    - overdrive
> +	"mediatek,<chip>-disp-ovl"		- overlay (4 layers, blending, csc)
> +	"mediatek,<chip>-disp-rdma"		- read DMA / line buffer
> +	"mediatek,<chip>-disp-wdma"		- write DMA
> +	"mediatek,<chip>-disp-color"		- color processor
> +	"mediatek,<chip>-disp-aal"		- adaptive ambient light controller
> +	"mediatek,<chip>-disp-gamma"		- gamma correction
> +	"mediatek,<chip>-disp-merge"		- merge streams from two RDMA sources
> +	"mediatek,<chip>-disp-split"		- split stream to two encoders
> +	"mediatek,<chip>-disp-ufoe"		- data compression engine
> +	"mediatek,<chip>-dsi"			- DSI controller, see mediatek,dsi.txt
> +	"mediatek,<chip>-dpi"			- DPI controller, see mediatek,dpi.txt
> +	"mediatek,<chip>-disp-mutex"		- display mutex
> +	"mediatek,<chip>-disp-od"		- overdrive
> +	"mediatek,<chip>-mmsys", "syscon"	- provide clocks and components management

A lot of reformatting for a 1 line change. It's fine if you want to 
leave this as one patch, but make the commit msg clear what's really 
changing here.

>    the supported chips are mt2701, mt2712 and mt8173.
> +  For mt7623, compatible must be:
> +	"mediatek,mt7623-<component>" , "mediatek,mt2701-<component>"
> +
>  - reg: Physical base address and length of the function block register space
>  - interrupts: The interrupt signal from the function block (required, except for
>    merge and split function blocks).
> -- 
> 2.19.1
> 

[PATCH v5 07/12] dt-bindings: clock: mediatek: delete mmsys clocks

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

Some SoCs will now load the clock part of mmsys via
a platform device from the dsiplay driver.
Remove the compatible from the clock bindings description.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 .../devicetree/bindings/arm/mediatek/mediatek,mmsys.txt        | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
index 15d977afad31..4468345f8b1a 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
@@ -6,11 +6,8 @@ The Mediatek mmsys controller provides various clocks to the system.
 Required Properties:
 
 - compatible: Should be one of:
-	- "mediatek,mt2701-mmsys", "syscon"
 	- "mediatek,mt2712-mmsys", "syscon"
 	- "mediatek,mt6797-mmsys", "syscon"
-	- "mediatek,mt7623-mmsys", "mediatek,mt2701-mmsys", "syscon"
-	- "mediatek,mt8173-mmsys", "syscon"
 - #clock-cells: Must be 1
 
 The mmsys controller uses the common clk binding from
-- 
2.19.1

Re: [PATCH v5 07/12] dt-bindings: clock: mediatek: delete mmsys clocks

[Rob Herring]

On Fri, 16 Nov 2018 13:54:44 +0100, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> Some SoCs will now load the clock part of mmsys via
> a platform device from the dsiplay driver.
> Remove the compatible from the clock bindings description.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  .../devicetree/bindings/arm/mediatek/mediatek,mmsys.txt        | 3 ---
>  1 file changed, 3 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

On SoCs with no publical available HW or no working graphic stack
we change the devicetree binding for the mmsys clock part. This
way we don't need to register a platform device explicitly in the
drm driver. Instead we can create a mmsys child which invokes the
clock driver.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 .../bindings/arm/mediatek/mediatek,mmsys.txt  | 21 ++++++++++++-------
 .../display/mediatek/mediatek,disp.txt        |  4 ++++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
index 4468345f8b1a..d4e205981363 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
@@ -1,4 +1,4 @@
-Mediatek mmsys controller
+Mediatek mmsys clock controller
 ============================
 
 The Mediatek mmsys controller provides various clocks to the system.
@@ -6,18 +6,25 @@ The Mediatek mmsys controller provides various clocks to the system.
 Required Properties:
 
 - compatible: Should be one of:
-	- "mediatek,mt2712-mmsys", "syscon"
-	- "mediatek,mt6797-mmsys", "syscon"
+	- "mediatek,mt2712-mmsys-clk", "syscon"
+	- "mediatek,mt6797-mmsys-clk", "syscon"
 - #clock-cells: Must be 1
 
-The mmsys controller uses the common clk binding from
+The mmsys clock controller uses the common clk binding from
 Documentation/devicetree/bindings/clock/clock-bindings.txt
 The available clocks are defined in dt-bindings/clock/mt*-clk.h.
+It is a child of the mmsys block, see binding at:
+Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
 
 Example:
 
-mmsys: clock-controller@14000000 {
-	compatible = "mediatek,mt8173-mmsys", "syscon";
+mmsys: syscon@14000000 {
+	compatible = "mediatek,mt2712-mmsys", "syscon", "simple-mfd";
 	reg = <0 0x14000000 0 0x1000>;
-	#clock-cells = <1>;
+
+	mmsys_clk: clock-controller@14000000 {
+		compatible = "mediatek,mt2712-mmsys-clk";
+		#clock-cells = <1>;
+	};
+
 };
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
index 4b008d992398..38c708cb7e55 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
@@ -54,6 +54,10 @@ Required properties (all function blocks):
   DPI controller nodes have multiple clock inputs. These are documented in
   mediatek,dsi.txt and mediatek,dpi.txt, respectively.
 
+Some chips have a separate binding for the clock controller, which is a child node
+of the mmsys device, for more information see:
+Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
+
 Required properties (DMA function blocks):
 - compatible: Should be one of
 	"mediatek,<chip>-disp-ovl"
-- 
2.19.1

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Rob Herring]

On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> On SoCs with no publical available HW or no working graphic stack
> we change the devicetree binding for the mmsys clock part. This
> way we don't need to register a platform device explicitly in the
> drm driver. Instead we can create a mmsys child which invokes the
> clock driver.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  .../bindings/arm/mediatek/mediatek,mmsys.txt  | 21 ++++++++++++-------
>  .../display/mediatek/mediatek,disp.txt        |  4 ++++
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
> index 4468345f8b1a..d4e205981363 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
> @@ -1,4 +1,4 @@
> -Mediatek mmsys controller
> +Mediatek mmsys clock controller
>  ============================
>  
>  The Mediatek mmsys controller provides various clocks to the system.
> @@ -6,18 +6,25 @@ The Mediatek mmsys controller provides various clocks to the system.
>  Required Properties:
>  
>  - compatible: Should be one of:
> -	- "mediatek,mt2712-mmsys", "syscon"
> -	- "mediatek,mt6797-mmsys", "syscon"
> +	- "mediatek,mt2712-mmsys-clk", "syscon"
> +	- "mediatek,mt6797-mmsys-clk", "syscon"

Doesn't match the example.

>  - #clock-cells: Must be 1
>  
> -The mmsys controller uses the common clk binding from
> +The mmsys clock controller uses the common clk binding from
>  Documentation/devicetree/bindings/clock/clock-bindings.txt
>  The available clocks are defined in dt-bindings/clock/mt*-clk.h.
> +It is a child of the mmsys block, see binding at:
> +Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>  
>  Example:
>  
> -mmsys: clock-controller@14000000 {
> -	compatible = "mediatek,mt8173-mmsys", "syscon";
> +mmsys: syscon@14000000 {
> +	compatible = "mediatek,mt2712-mmsys", "syscon", "simple-mfd";
>  	reg = <0 0x14000000 0 0x1000>;
> -	#clock-cells = <1>;
> +
> +	mmsys_clk: clock-controller@14000000 {
> +		compatible = "mediatek,mt2712-mmsys-clk";
> +		#clock-cells = <1>;

This goes against the general direction of not defining separate nodes 
for providers with no resources.

Why do you need this and what does it buy if you have to continue to 
support the existing chips?

Rob

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Matthias Brugger]

[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]



On 11/17/18 12:15 AM, Rob Herring wrote:
> On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
>> From: Matthias Brugger <mbrugger@suse.com>
>>
>> On SoCs with no publical available HW or no working graphic stack
>> we change the devicetree binding for the mmsys clock part. This
>> way we don't need to register a platform device explicitly in the
>> drm driver. Instead we can create a mmsys child which invokes the
>> clock driver.
>>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>> ---
>>  .../bindings/arm/mediatek/mediatek,mmsys.txt  | 21 ++++++++++++-------
>>  .../display/mediatek/mediatek,disp.txt        |  4 ++++
>>  2 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
>> index 4468345f8b1a..d4e205981363 100644
>> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
>> @@ -1,4 +1,4 @@
>> -Mediatek mmsys controller
>> +Mediatek mmsys clock controller
>>  ============================
>>  
>>  The Mediatek mmsys controller provides various clocks to the system.
>> @@ -6,18 +6,25 @@ The Mediatek mmsys controller provides various clocks to the system.
>>  Required Properties:
>>  
>>  - compatible: Should be one of:
>> -	- "mediatek,mt2712-mmsys", "syscon"
>> -	- "mediatek,mt6797-mmsys", "syscon"
>> +	- "mediatek,mt2712-mmsys-clk", "syscon"
>> +	- "mediatek,mt6797-mmsys-clk", "syscon"
> 
> Doesn't match the example.>
>>  - #clock-cells: Must be 1
>>  
>> -The mmsys controller uses the common clk binding from
>> +The mmsys clock controller uses the common clk binding from
>>  Documentation/devicetree/bindings/clock/clock-bindings.txt
>>  The available clocks are defined in dt-bindings/clock/mt*-clk.h.
>> +It is a child of the mmsys block, see binding at:
>> +Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>>  
>>  Example:
>>  
>> -mmsys: clock-controller@14000000 {
>> -	compatible = "mediatek,mt8173-mmsys", "syscon";
>> +mmsys: syscon@14000000 {
>> +	compatible = "mediatek,mt2712-mmsys", "syscon", "simple-mfd";
>>  	reg = <0 0x14000000 0 0x1000>;
>> -	#clock-cells = <1>;
>> +
>> +	mmsys_clk: clock-controller@14000000 {
>> +		compatible = "mediatek,mt2712-mmsys-clk";
>> +		#clock-cells = <1>;
> 
> This goes against the general direction of not defining separate nodes 
> for providers with no resources.
> 
> Why do you need this and what does it buy if you have to continue to 
> support the existing chips?
> 

It would show explicitly that the mmsys block is used to probe two
drivers, one for the gpu and one for the clocks. Otherwise that is
hidden in the drm driver code. I think it is cleaner to describe that in
the device tree.

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 1805 bytes --]

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Rob Herring]

On Sun, Nov 18, 2018 at 11:12 AM Matthias Brugger
<matthias.bgg@gmail.com> wrote:
>
>
>
> On 11/17/18 12:15 AM, Rob Herring wrote:
> > On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
> >> From: Matthias Brugger <mbrugger@suse.com>
> >>
> >> On SoCs with no publical available HW or no working graphic stack
> >> we change the devicetree binding for the mmsys clock part. This
> >> way we don't need to register a platform device explicitly in the
> >> drm driver. Instead we can create a mmsys child which invokes the
> >> clock driver.
> >>
> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> >> ---
> >>  .../bindings/arm/mediatek/mediatek,mmsys.txt  | 21 ++++++++++++-------
> >>  .../display/mediatek/mediatek,disp.txt        |  4 ++++
> >>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
> >> index 4468345f8b1a..d4e205981363 100644
> >> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
> >> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt
> >> @@ -1,4 +1,4 @@
> >> -Mediatek mmsys controller
> >> +Mediatek mmsys clock controller
> >>  ============================
> >>
> >>  The Mediatek mmsys controller provides various clocks to the system.
> >> @@ -6,18 +6,25 @@ The Mediatek mmsys controller provides various clocks to the system.
> >>  Required Properties:
> >>
> >>  - compatible: Should be one of:
> >> -    - "mediatek,mt2712-mmsys", "syscon"
> >> -    - "mediatek,mt6797-mmsys", "syscon"
> >> +    - "mediatek,mt2712-mmsys-clk", "syscon"
> >> +    - "mediatek,mt6797-mmsys-clk", "syscon"
> >
> > Doesn't match the example.>
> >>  - #clock-cells: Must be 1
> >>
> >> -The mmsys controller uses the common clk binding from
> >> +The mmsys clock controller uses the common clk binding from
> >>  Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>  The available clocks are defined in dt-bindings/clock/mt*-clk.h.
> >> +It is a child of the mmsys block, see binding at:
> >> +Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> >>
> >>  Example:
> >>
> >> -mmsys: clock-controller@14000000 {
> >> -    compatible = "mediatek,mt8173-mmsys", "syscon";
> >> +mmsys: syscon@14000000 {
> >> +    compatible = "mediatek,mt2712-mmsys", "syscon", "simple-mfd";
> >>      reg = <0 0x14000000 0 0x1000>;
> >> -    #clock-cells = <1>;
> >> +
> >> +    mmsys_clk: clock-controller@14000000 {
> >> +            compatible = "mediatek,mt2712-mmsys-clk";
> >> +            #clock-cells = <1>;
> >
> > This goes against the general direction of not defining separate nodes
> > for providers with no resources.
> >
> > Why do you need this and what does it buy if you have to continue to
> > support the existing chips?
> >
>
> It would show explicitly that the mmsys block is used to probe two
> drivers, one for the gpu and one for the clocks. Otherwise that is
> hidden in the drm driver code. I think it is cleaner to describe that in
> the device tree.

No, that's maybe cleaner for the driver implementation in the Linux
kernel. What about other OS's or when Linux drivers and subsystems
needs change? Cleaner for DT is design bindings that reflect the h/w.
Hardware is sometimes just messy.

Rob

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Stephen Boyd]

Quoting Rob Herring (2018-11-19 11:15:16)
> On Sun, Nov 18, 2018 at 11:12 AM Matthias Brugger
> <matthias.bgg@gmail.com> wrote:
> > On 11/17/18 12:15 AM, Rob Herring wrote:
> > > On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
> > >> -    #clock-cells = <1>;
> > >> +
> > >> +    mmsys_clk: clock-controller@14000000 {
> > >> +            compatible = "mediatek,mt2712-mmsys-clk";
> > >> +            #clock-cells = <1>;
> > >
> > > This goes against the general direction of not defining separate nodes
> > > for providers with no resources.
> > >
> > > Why do you need this and what does it buy if you have to continue to
> > > support the existing chips?
> > >
> >
> > It would show explicitly that the mmsys block is used to probe two
> > drivers, one for the gpu and one for the clocks. Otherwise that is
> > hidden in the drm driver code. I think it is cleaner to describe that in
> > the device tree.
> 
> No, that's maybe cleaner for the driver implementation in the Linux
> kernel. What about other OS's or when Linux drivers and subsystems
> needs change? Cleaner for DT is design bindings that reflect the h/w.
> Hardware is sometimes just messy.
> 

I agree. I fail to see what this patch series is doing besides changing
driver probe and device creation methods and making a backwards
incompatible change to DT. Is there any other benefit here?

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Matthias Brugger]

On 21/11/2018 17:46, Stephen Boyd wrote:
> Quoting Rob Herring (2018-11-19 11:15:16)
>> On Sun, Nov 18, 2018 at 11:12 AM Matthias Brugger
>> <matthias.bgg@gmail.com> wrote:
>>> On 11/17/18 12:15 AM, Rob Herring wrote:
>>>> On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
>>>>> -    #clock-cells = <1>;
>>>>> +
>>>>> +    mmsys_clk: clock-controller@14000000 {
>>>>> +            compatible = "mediatek,mt2712-mmsys-clk";
>>>>> +            #clock-cells = <1>;
>>>>
>>>> This goes against the general direction of not defining separate nodes
>>>> for providers with no resources.
>>>>
>>>> Why do you need this and what does it buy if you have to continue to
>>>> support the existing chips?
>>>>
>>>
>>> It would show explicitly that the mmsys block is used to probe two
>>> drivers, one for the gpu and one for the clocks. Otherwise that is
>>> hidden in the drm driver code. I think it is cleaner to describe that in
>>> the device tree.
>>
>> No, that's maybe cleaner for the driver implementation in the Linux
>> kernel. What about other OS's or when Linux drivers and subsystems
>> needs change? Cleaner for DT is design bindings that reflect the h/w.
>> Hardware is sometimes just messy.
>>
> 
> I agree. I fail to see what this patch series is doing besides changing
> driver probe and device creation methods and making a backwards
> incompatible change to DT. Is there any other benefit here?
> 

You are referring whole series?
Citing the cover letter:
"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 (sic!) 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."

DT is broken right now, because two drivers rely on the same node, which gets
consumed just once. The new DT introduced does not break anything because it is
only used for boards that: "[..] are not available to the general public
(mt2712e) or only have the mmsys clock driver part implemented (mt6797)."

Anyway, I'll send a new version which uses the platform device in the DRM driver
for all SoCs.

Regards,
Matthias

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Stephen Boyd]

Quoting Matthias Brugger (2018-11-21 09:09:52)
> 
> 
> On 21/11/2018 17:46, Stephen Boyd wrote:
> > Quoting Rob Herring (2018-11-19 11:15:16)
> >> On Sun, Nov 18, 2018 at 11:12 AM Matthias Brugger
> >> <matthias.bgg@gmail.com> wrote:
> >>> On 11/17/18 12:15 AM, Rob Herring wrote:
> >>>> On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
> >>>>> -    #clock-cells = <1>;
> >>>>> +
> >>>>> +    mmsys_clk: clock-controller@14000000 {
> >>>>> +            compatible = "mediatek,mt2712-mmsys-clk";
> >>>>> +            #clock-cells = <1>;
> >>>>
> >>>> This goes against the general direction of not defining separate nodes
> >>>> for providers with no resources.
> >>>>
> >>>> Why do you need this and what does it buy if you have to continue to
> >>>> support the existing chips?
> >>>>
> >>>
> >>> It would show explicitly that the mmsys block is used to probe two
> >>> drivers, one for the gpu and one for the clocks. Otherwise that is
> >>> hidden in the drm driver code. I think it is cleaner to describe that in
> >>> the device tree.
> >>
> >> No, that's maybe cleaner for the driver implementation in the Linux
> >> kernel. What about other OS's or when Linux drivers and subsystems
> >> needs change? Cleaner for DT is design bindings that reflect the h/w.
> >> Hardware is sometimes just messy.
> >>
> > 
> > I agree. I fail to see what this patch series is doing besides changing
> > driver probe and device creation methods and making a backwards
> > incompatible change to DT. Is there any other benefit here?
> > 
> 
> You are referring whole series?
> Citing the cover letter:
> "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 (sic!) 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.

Ouch!

> 
> 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."

Alright, please don't add nodes in DT just to make device drivers probe.
Instead, register clks from the drm driver or create a child platform
device for the clk bits purely in the drm driver and have that probe the
associated clk driver from there.

> 
> DT is broken right now, because two drivers rely on the same node, which gets
> consumed just once. The new DT introduced does not break anything because it is
> only used for boards that: "[..] are not available to the general public
> (mt2712e) or only have the mmsys clock driver part implemented (mt6797)."

Ok, so backwards compatibility is irrelevant then. Sounds fine to me.

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Matthias Brugger]

On 30/11/2018 07:43, Stephen Boyd wrote:
> Quoting Matthias Brugger (2018-11-21 09:09:52)
>>
>>
>> On 21/11/2018 17:46, Stephen Boyd wrote:
>>> Quoting Rob Herring (2018-11-19 11:15:16)
>>>> On Sun, Nov 18, 2018 at 11:12 AM Matthias Brugger
>>>> <matthias.bgg@gmail.com> wrote:
>>>>> On 11/17/18 12:15 AM, Rob Herring wrote:
>>>>>> On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
>>>>>>> -    #clock-cells = <1>;
>>>>>>> +
>>>>>>> +    mmsys_clk: clock-controller@14000000 {
>>>>>>> +            compatible = "mediatek,mt2712-mmsys-clk";
>>>>>>> +            #clock-cells = <1>;
>>>>>>
>>>>>> This goes against the general direction of not defining separate nodes
>>>>>> for providers with no resources.
>>>>>>
>>>>>> Why do you need this and what does it buy if you have to continue to
>>>>>> support the existing chips?
>>>>>>
>>>>>
>>>>> It would show explicitly that the mmsys block is used to probe two
>>>>> drivers, one for the gpu and one for the clocks. Otherwise that is
>>>>> hidden in the drm driver code. I think it is cleaner to describe that in
>>>>> the device tree.
>>>>
>>>> No, that's maybe cleaner for the driver implementation in the Linux
>>>> kernel. What about other OS's or when Linux drivers and subsystems
>>>> needs change? Cleaner for DT is design bindings that reflect the h/w.
>>>> Hardware is sometimes just messy.
>>>>
>>>
>>> I agree. I fail to see what this patch series is doing besides changing
>>> driver probe and device creation methods and making a backwards
>>> incompatible change to DT. Is there any other benefit here?
>>>
>>
>> You are referring whole series?
>> Citing the cover letter:
>> "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 (sic!) 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.
> 
> Ouch!
> 

Yes :)

>>
>> 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."
> 
> Alright, please don't add nodes in DT just to make device drivers probe.
> Instead, register clks from the drm driver or create a child platform
> device for the clk bits purely in the drm driver and have that probe the
> associated clk driver from there.
> 

I'll make the other SoCs probe via a child platform device from the drm driver,
as already done in 2/12 and 3/12.

Regards,
Matthias

>>
>> DT is broken right now, because two drivers rely on the same node, which gets
>> consumed just once. The new DT introduced does not break anything because it is
>> only used for boards that: "[..] are not available to the general public
>> (mt2712e) or only have the mmsys clock driver part implemented (mt6797)."
> 
> Ok, so backwards compatibility is irrelevant then. Sounds fine to me.
> 
> 

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[CK Hu]

Hi, Matthias:

On Fri, 2018-11-30 at 16:59 +0800, Matthias Brugger wrote:
> 
> On 30/11/2018 07:43, Stephen Boyd wrote:
> > Quoting Matthias Brugger (2018-11-21 09:09:52)
> >>
> >>
> >> On 21/11/2018 17:46, Stephen Boyd wrote:
> >>> Quoting Rob Herring (2018-11-19 11:15:16)
> >>>> On Sun, Nov 18, 2018 at 11:12 AM Matthias Brugger
> >>>> <matthias.bgg@gmail.com> wrote:
> >>>>> On 11/17/18 12:15 AM, Rob Herring wrote:
> >>>>>> On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
> >>>>>>> -    #clock-cells = <1>;
> >>>>>>> +
> >>>>>>> +    mmsys_clk: clock-controller@14000000 {
> >>>>>>> +            compatible = "mediatek,mt2712-mmsys-clk";
> >>>>>>> +            #clock-cells = <1>;
> >>>>>>
> >>>>>> This goes against the general direction of not defining separate nodes
> >>>>>> for providers with no resources.
> >>>>>>
> >>>>>> Why do you need this and what does it buy if you have to continue to
> >>>>>> support the existing chips?
> >>>>>>
> >>>>>
> >>>>> It would show explicitly that the mmsys block is used to probe two
> >>>>> drivers, one for the gpu and one for the clocks. Otherwise that is
> >>>>> hidden in the drm driver code. I think it is cleaner to describe that in
> >>>>> the device tree.
> >>>>
> >>>> No, that's maybe cleaner for the driver implementation in the Linux
> >>>> kernel. What about other OS's or when Linux drivers and subsystems
> >>>> needs change? Cleaner for DT is design bindings that reflect the h/w.
> >>>> Hardware is sometimes just messy.
> >>>>
> >>>
> >>> I agree. I fail to see what this patch series is doing besides changing
> >>> driver probe and device creation methods and making a backwards
> >>> incompatible change to DT. Is there any other benefit here?
> >>>
> >>
> >> You are referring whole series?
> >> Citing the cover letter:
> >> "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 (sic!) 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.
> > 
> > Ouch!
> > 
> 
> Yes :)
> 
> >>
> >> 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."
> > 
> > Alright, please don't add nodes in DT just to make device drivers probe.
> > Instead, register clks from the drm driver or create a child platform
> > device for the clk bits purely in the drm driver and have that probe the
> > associated clk driver from there.
> > 
> 
> I'll make the other SoCs probe via a child platform device from the drm driver,
> as already done in 2/12 and 3/12.

This series have been pending for half an year, would you keep going on
this series? If you're busy, I could complete this series, but I need to
know what you have plan to do.

I guess that 1/12 ~ 5/12 is for MT2701/MT8173 and that patches meet this
discussion. 6/12 ~ 12/12 is for MT2712/MT6797 but that patches does not
meet this discussion. So the unfinished work is to make MT2712/MT6797 to
align MT2701/MT8173, is this right?

Regards,
CK

> 
> Regards,
> Matthias
> 
> >>
> >> DT is broken right now, because two drivers rely on the same node, which gets
> >> consumed just once. The new DT introduced does not break anything because it is
> >> only used for boards that: "[..] are not available to the general public
> >> (mt2712e) or only have the mmsys clock driver part implemented (mt6797)."
> > 
> > Ok, so backwards compatibility is irrelevant then. Sounds fine to me.
> > 
> > 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Matthias Brugger]

Hi CK-Hu,

On 01/07/2019 05:55, CK Hu wrote:
> Hi, Matthias:
> 
> On Fri, 2018-11-30 at 16:59 +0800, Matthias Brugger wrote:
>>
>> On 30/11/2018 07:43, Stephen Boyd wrote:
>>> Quoting Matthias Brugger (2018-11-21 09:09:52)
>>>>
>>>>
>>>> On 21/11/2018 17:46, Stephen Boyd wrote:
>>>>> Quoting Rob Herring (2018-11-19 11:15:16)
>>>>>> On Sun, Nov 18, 2018 at 11:12 AM Matthias Brugger
>>>>>> <matthias.bgg@gmail.com> wrote:
>>>>>>> On 11/17/18 12:15 AM, Rob Herring wrote:
>>>>>>>> On Fri, Nov 16, 2018 at 01:54:45PM +0100, matthias.bgg@kernel.org wrote:
>>>>>>>>> -    #clock-cells = <1>;
>>>>>>>>> +
>>>>>>>>> +    mmsys_clk: clock-controller@14000000 {
>>>>>>>>> +            compatible = "mediatek,mt2712-mmsys-clk";
>>>>>>>>> +            #clock-cells = <1>;
>>>>>>>>
>>>>>>>> This goes against the general direction of not defining separate nodes
>>>>>>>> for providers with no resources.
>>>>>>>>
>>>>>>>> Why do you need this and what does it buy if you have to continue to
>>>>>>>> support the existing chips?
>>>>>>>>
>>>>>>>
>>>>>>> It would show explicitly that the mmsys block is used to probe two
>>>>>>> drivers, one for the gpu and one for the clocks. Otherwise that is
>>>>>>> hidden in the drm driver code. I think it is cleaner to describe that in
>>>>>>> the device tree.
>>>>>>
>>>>>> No, that's maybe cleaner for the driver implementation in the Linux
>>>>>> kernel. What about other OS's or when Linux drivers and subsystems
>>>>>> needs change? Cleaner for DT is design bindings that reflect the h/w.
>>>>>> Hardware is sometimes just messy.
>>>>>>
>>>>>
>>>>> I agree. I fail to see what this patch series is doing besides changing
>>>>> driver probe and device creation methods and making a backwards
>>>>> incompatible change to DT. Is there any other benefit here?
>>>>>
>>>>
>>>> You are referring whole series?
>>>> Citing the cover letter:
>>>> "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 (sic!) 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.
>>>
>>> Ouch!
>>>
>>
>> Yes :)
>>
>>>>
>>>> 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."
>>>
>>> Alright, please don't add nodes in DT just to make device drivers probe.
>>> Instead, register clks from the drm driver or create a child platform
>>> device for the clk bits purely in the drm driver and have that probe the
>>> associated clk driver from there.
>>>
>>
>> I'll make the other SoCs probe via a child platform device from the drm driver,
>> as already done in 2/12 and 3/12.
> 
> This series have been pending for half an year, would you keep going on
> this series? If you're busy, I could complete this series, but I need to
> know what you have plan to do.
> 

You are right, it took far too long for me to respond with a new version of the
series. The problem I face is, that I use my mt8173 based chromebook for
testing. It needs some downstream patches and broke somewhere between my last
email and a few month ago. I wasn't able to get serial console to work, which
made things even more complicated. Anyway, long story short, I got sidetracked
with other stuff and didn't send a new version.

If you have time to work on this, I'd happy to see things being pushed forward
by you :)

> I guess that 1/12 ~ 5/12 is for MT2701/MT8173 and that patches meet this
> discussion. 6/12 ~ 12/12 is for MT2712/MT6797 but that patches does not
> meet this discussion. So the unfinished work is to make MT2712/MT6797 to
> align MT2701/MT8173, is this right?

After re-reading the emails I think the missing part is, to probe the clocks
from the DRM driver instead of adding a new devicetree binding for them.

Regards,
Matthias

> 
> Regards,
> CK
> 
>>
>> Regards,
>> Matthias
>>
>>>>
>>>> DT is broken right now, because two drivers rely on the same node, which gets
>>>> consumed just once. The new DT introduced does not break anything because it is
>>>> only used for boards that: "[..] are not available to the general public
>>>> (mt2712e) or only have the mmsys clock driver part implemented (mt6797)."
>>>
>>> Ok, so backwards compatibility is irrelevant then. Sounds fine to me.
>>>
>>>
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 
> 

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Ulrich Hecht]

> On July 4, 2019 at 11:08 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> You are right, it took far too long for me to respond with a new version of the
> series. The problem I face is, that I use my mt8173 based chromebook for
> testing. It needs some downstream patches and broke somewhere between my last
> email and a few month ago.

If that Chromebook is an Acer R13 and you need a working kernel, you may want to have a look at https://github.com/uli/kernel/tree/elm-working-5.2 .

CU
Uli

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[CK Hu]

Hi, Uli:

On Thu, 2019-07-04 at 17:33 +0200, Ulrich Hecht wrote:
> > On July 4, 2019 at 11:08 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> > You are right, it took far too long for me to respond with a new version of the
> > series. The problem I face is, that I use my mt8173 based chromebook for
> > testing. It needs some downstream patches and broke somewhere between my last
> > email and a few month ago.
> 
> If that Chromebook is an Acer R13 and you need a working kernel, you may want to have a look at https://github.com/uli/kernel/tree/elm-working-5.2 .

Thanks for your sample code, and your implementation is different than
Matthias' version. In your version, mmsys is a single device which has
clock function and display function, the clock function is placed in
clock driver folder and display function is placed in drm driver folder.
In Matthias' version, clock function is a sub device of mmsys. I've no
idea of which one is better. I would get more information to make better
decision.

Regards,
CK

> 
> CU
> Uli
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v5 08/12] dt-bindings: mediatek: Change the binding for mmsys clocks

[Matthias Brugger]

On 05/07/2019 03:35, CK Hu wrote:
> Hi, Uli:
> 
> On Thu, 2019-07-04 at 17:33 +0200, Ulrich Hecht wrote:
>>> On July 4, 2019 at 11:08 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
>>> You are right, it took far too long for me to respond with a new version of the
>>> series. The problem I face is, that I use my mt8173 based chromebook for
>>> testing. It needs some downstream patches and broke somewhere between my last
>>> email and a few month ago.
>>
>> If that Chromebook is an Acer R13 and you need a working kernel, you may want to have a look at https://github.com/uli/kernel/tree/elm-working-5.2 .
> 
> Thanks for your sample code, and your implementation is different than
> Matthias' version. In your version, mmsys is a single device which has
> clock function and display function, the clock function is placed in
> clock driver folder and display function is placed in drm driver folder.
> In Matthias' version, clock function is a sub device of mmsys. I've no
> idea of which one is better. I would get more information to make better
> decision.
> 

From the discussion we had here and the last comments from Stephen I thin what
we should do is:
- create a platform driver for the mmsys clock part, in the drivers/clk/mediatek/...
- the DRM driver creates a platform device which will probe the clock driver.

This way you won't need to change any compatible.

You will have to re-read the discussions in the different versions of this series.
My first approach was to usa a mfd driver for mmsys, which was rejected.
The last approach was to create a DTS sub node, also rejected :)

Regards,
Matthias

> Regards,
> CK
> 
>>
>> CU
>> Uli
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 
> 
> 

[PATCH v5 09/12] arm64: dts: mt2712e: Use the new mmsys clock compatible

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

Move the clock part of the mmsys in a child node.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
index ee627a7c7b45..dd6837df92c7 100644
--- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
@@ -412,9 +412,13 @@
 	};
 
 	mmsys: syscon@14000000 {
-		compatible = "mediatek,mt2712-mmsys", "syscon";
+		compatible = "mediatek,mt2712-mmsys", "syscon", "simple-mfd";
 		reg = <0 0x14000000 0 0x1000>;
-		#clock-cells = <1>;
+
+		mmsys_clk: clock-controller@14000000 {
+			compatible = "mediatek,mt2712-mmsys-clk";
+			#clock-cells = <1>;
+		};
 	};
 
 	imgsys: syscon@15000000 {
-- 
2.19.1

[PATCH v5 10/12] arm64: dts: mt6797: Use the new mmsys clock compatible

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

Move the clock part of mmsys in a child node.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 arch/arm64/boot/dts/mediatek/mt6797.dtsi | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt6797.dtsi b/arch/arm64/boot/dts/mediatek/mt6797.dtsi
index 4beaa71107d7..3ca635c81de1 100644
--- a/arch/arm64/boot/dts/mediatek/mt6797.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt6797.dtsi
@@ -206,9 +206,13 @@
 	};
 
 	mmsys: mmsys_config@14000000 {
-		compatible = "mediatek,mt6797-mmsys", "syscon";
+		compatible = "mediatek,mt6797-mmsys", "syscon", "simple-mfd";
 		reg = <0 0x14000000 0 0x1000>;
-		#clock-cells = <1>;
+
+		mmsys_clk: clock-controller@14000000 {
+			compatible = "mediatek,mt6797-mmsys-clk";
+			#clock-cells = <1>;
+		};
 	};
 
 	imgsys: imgsys_config@15000000  {
-- 
2.19.1

[PATCH v5 11/12] clk: mediatek: mt2712e: Probe with new compatible

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

The clock node is now a child of the mmsys node.
Update the driver to support this and thenew compatible
in the driver.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 drivers/clk/mediatek/clk-mt2712-mm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2712-mm.c b/drivers/clk/mediatek/clk-mt2712-mm.c
index a8b4b6d42488..5f4ee8f0deaa 100644
--- a/drivers/clk/mediatek/clk-mt2712-mm.c
+++ b/drivers/clk/mediatek/clk-mt2712-mm.c
@@ -138,14 +138,15 @@ static int clk_mt2712_mm_probe(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
 	int r;
-	struct device_node *node = pdev->dev.of_node;
+	struct device	parent = pdev->dev.parent;
 
 	clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
 
-	mtk_clk_register_gates(node, mm_clks, ARRAY_SIZE(mm_clks),
+	mtk_clk_register_gates(parent->of_node, mm_clks, ARRAY_SIZE(mm_clks),
 			clk_data);
 
-	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+	r = of_clk_add_provider(parent->of_node, of_clk_src_onecell_get,
+			clk_data);
 
 	if (r != 0)
 		pr_err("%s(): could not register clock provider: %d\n",
@@ -155,7 +156,7 @@ static int clk_mt2712_mm_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id of_match_clk_mt2712_mm[] = {
-	{ .compatible = "mediatek,mt2712-mmsys", },
+	{ .compatible = "mediatek,mt2712-mmsys-clk", },
 	{}
 };
 
-- 
2.19.1

[PATCH v5 12/12] clk: mediatek: mt6797: Probe with new compatible

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

The clock node is now a child of the mmsys node.
Update the driver to support this and thenew compatible
in the driver.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 drivers/clk/mediatek/clk-mt6797-mm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt6797-mm.c b/drivers/clk/mediatek/clk-mt6797-mm.c
index c57d3eed270d..051bab99d10f 100644
--- a/drivers/clk/mediatek/clk-mt6797-mm.c
+++ b/drivers/clk/mediatek/clk-mt6797-mm.c
@@ -101,7 +101,7 @@ static const struct mtk_gate mm_clks[] = {
 };
 
 static const struct of_device_id of_match_clk_mt6797_mm[] = {
-	{ .compatible = "mediatek,mt6797-mmsys", },
+	{ .compatible = "mediatek,mt6797-mmsys-clk", },
 	{}
 };
 
@@ -109,14 +109,15 @@ static int clk_mt6797_mm_probe(struct platform_device *pdev)
 {
 	struct clk_onecell_data *clk_data;
 	int r;
-	struct device_node *node = pdev->dev.of_node;
+	struct device *parent = pdev->dev.parent;
 
 	clk_data = mtk_alloc_clk_data(CLK_MM_NR);
 
-	mtk_clk_register_gates(node, mm_clks, ARRAY_SIZE(mm_clks),
+	mtk_clk_register_gates(parent->of_node, mm_clks, ARRAY_SIZE(mm_clks),
 			       clk_data);
 
-	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+	r = of_clk_add_provider(parent->of_node, of_clk_src_onecell_get,
+			clk_data);
 	if (r)
 		dev_err(&pdev->dev,
 			"could not register clock provider: %s: %d\n",
-- 
2.19.1