[RFC resend] arm64: mt8173: Fix Acer Chromebooks mmsys probe problem

[RFC resend] arm64: mt8173: Fix Acer Chromebooks mmsys probe problem

[Matthias Brugger]

In theory the MMSYS device tree identifier is matches twice, by the clk driver 
and the DRM subsystem. But the kernel only matches the first driver for a 
device (clk) and discards the second one. This breaks graphics on mt8173 and 
most probably on mt2701 as well.

MMSYS in Mediatek SoCs has some registers to control clock gates (which is 
used in the clk driver) and some registers to enable the differnet blocks of 
the display subsystem. The kernel uses the binding to load the central 
comoponent of the distplay subsystem, which in place probes all the other 
components and enables the present ones in the MMSYS.

We found us with the problem, that we need to change and therefor break one 
of the two bindings, or the DRM one or the clock driver one.

Apart from that the DRM subysystem does access the MMSYS registers via relaxed 
reads/writes. But the it should to so via regmap, as the registers are shared.

Possible solutions:
1) We add a new mediatek,mt8173-mmsys-clk node, which lives as a simple-mfd 
under the actual mmsys node. We change the clock driver to probe on this 
binding. This would make sense as the clock gate register live completly in 
the MMSYS configuration registers.

2) As the nodes of the DRM subsystem just need some of the registers of MMSYS 
we add a new binding mediatek,mt8173-dispsys which probes the central 
component of the DRM system. It has only a handle to mt8173-mmsys to access 
the registerspace via regmap functions.

In this patchset I implemented 2). Please take into account, that this is a 
RFC. I had no time to actually test the verison on real HW. Some of the 
register accesses should be done using regmap_update instead of 
regmap_read + regmap_write.

This RFC shall only show how solution 2) would look like. We can use it as 
discussion to see how we circumvent the actual situation.

[RFC resend 1/4] dt-bindings: display: mediatek: add drm binding

[Matthias Brugger]

DRM subysystem and clock driver shared the same compatible mmsys.
This stopped does not work, as only the first driver for a compatible
gets probed. We change the comaptible to the new DRM identifier to fix
this.

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

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
index 383183a89164..6db652463e64 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
@@ -27,6 +27,7 @@ Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
 
 Required properties (all function blocks):
 - compatible: "mediatek,<chip>-disp-<function>", one of
+	"mediatek,<chip>-dispsys"    - central component for the DRM system
 	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
 	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
 	"mediatek,<chip>-disp-wdma"  - write DMA
@@ -71,6 +72,11 @@ mmsys: clock-controller at 14000000 {
 	#clock-cells = <1>;
 };
 
+dispsys: display-system {
+	compatible = "mediatek,mt2701-dispsys";
+	mediatek,mmsys = <&mmsys>;
+}
+
 ovl0: ovl at 1400c000 {
 	compatible = "mediatek,mt8173-disp-ovl";
 	reg = <0 0x1400c000 0 0x1000>;
-- 
2.14.2

[RFC resend 2/4] drm/mediatek: Add new compatible to probe multimedia subsystem.

[Matthias Brugger]

The mmsys block is an independent block that probes the
clock driver. The multimedia subsystem therefore does not get
probed. We add a new compatible for the multimedia subsystem.

We pass the mmsys registers via syscon and access them using regmap
instead of relaxed read/write.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c |  4 ++--
 drivers/gpu/drm/mediatek/mtk_drm_ddp.c  | 30 +++++++++++++++++-------------
 drivers/gpu/drm/mediatek/mtk_drm_ddp.h  |  4 ++--
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  | 17 ++++++-----------
 drivers/gpu/drm/mediatek/mtk_drm_drv.h  |  2 +-
 5 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 658b8dd45b83..4c65873b4867 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -33,7 +33,7 @@
  * @enabled: records whether crtc_enable succeeded
  * @planes: array of 4 drm_plane structures, one for each overlay plane
  * @pending_planes: whether any plane has pending changes to be applied
- * @config_regs: memory mapped mmsys configuration register space
+ * @config_regs: regmap mapped mmsys configuration register space
  * @mutex: handle to one of the ten disp_mutex streams
  * @ddp_comp_nr: number of components in ddp_comp
  * @ddp_comp: array of pointers the mtk_ddp_comp structures used by this crtc
@@ -48,7 +48,7 @@ struct mtk_drm_crtc {
 	struct drm_plane		planes[OVL_LAYER_NR];
 	bool				pending_planes;
 
-	void __iomem			*config_regs;
+	struct regmap			*config_regs;
 	struct mtk_disp_mutex		*mutex;
 	unsigned int			ddp_comp_nr;
 	struct mtk_ddp_comp		**ddp_comp;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
index 8130f3dab661..1227d6db07da 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
@@ -185,16 +185,16 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
 	return value;
 }
 
-static void mtk_ddp_sout_sel(void __iomem *config_regs,
+static void mtk_ddp_sout_sel(struct regmap *config_regs,
 			     enum mtk_ddp_comp_id cur,
 			     enum mtk_ddp_comp_id next)
 {
 	if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0)
-		writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
-			       config_regs + DISP_REG_CONFIG_OUT_SEL);
+		regmap_write(config_regs, DISP_REG_CONFIG_OUT_SEL,
+				BLS_TO_DSI_RDMA1_TO_DPI1);
 }
 
-void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
+void mtk_ddp_add_comp_to_path(struct regmap *config_regs,
 			      enum mtk_ddp_comp_id cur,
 			      enum mtk_ddp_comp_id next)
 {
@@ -202,20 +202,22 @@ void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
 
 	value = mtk_ddp_mout_en(cur, next, &addr);
 	if (value) {
-		reg = readl_relaxed(config_regs + addr) | value;
-		writel_relaxed(reg, config_regs + addr);
+		regmap_read(config_regs, addr, &reg);
+		reg |= value;
+		regmap_write(config_regs, addr, reg);
 	}
 
 	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);
+		regmap_read(config_regs, addr, &reg);
+		reg |= value;
+		regmap_write(config_regs, addr, reg);
 	}
 }
 
-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)
 {
@@ -223,14 +225,16 @@ void mtk_ddp_remove_comp_from_path(void __iomem *config_regs,
 
 	value = mtk_ddp_mout_en(cur, next, &addr);
 	if (value) {
-		reg = readl_relaxed(config_regs + addr) & ~value;
-		writel_relaxed(reg, config_regs + addr);
+		regmap_read(config_regs, addr, &reg);
+		reg &= ~value;
+		regmap_write(config_regs, addr, reg);
 	}
 
 	value = mtk_ddp_sel_in(cur, next, &addr);
 	if (value) {
-		reg = readl_relaxed(config_regs + addr) & ~value;
-		writel_relaxed(reg, config_regs + addr);
+		regmap_read(config_regs, addr, &reg);
+		reg &= ~value;
+		regmap_write(config_regs, addr, reg);
 	}
 }
 
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 a2ca90fc403c..f013d0039821 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>
@@ -385,7 +386,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;
@@ -399,14 +399,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_regmap_lookup_by_compatible("mediatek,mmsys");
+	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) {
@@ -550,9 +545,9 @@ static SIMPLE_DEV_PM_OPS(mtk_drm_pm_ops, mtk_drm_sys_suspend,
 			 mtk_drm_sys_resume);
 
 static const struct of_device_id mtk_drm_of_ids[] = {
-	{ .compatible = "mediatek,mt2701-mmsys",
+	{ .compatible = "mediatek,mt2701-dispsys",
 	  .data = &mt2701_mmsys_driver_data},
-	{ .compatible = "mediatek,mt8173-mmsys",
+	{ .compatible = "mediatek,mt8173-dispsys",
 	  .data = &mt8173_mmsys_driver_data},
 	{ }
 };
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
index c3378c452c0a..c6fa0ad458e8 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -44,7 +44,7 @@ struct mtk_drm_private {
 
 	struct device_node *mutex_node;
 	struct device *mutex_dev;
-	void __iomem *config_regs;
+	struct regmap *config_regs;
 	struct device_node *comp_node[DDP_COMPONENT_ID_MAX];
 	struct mtk_ddp_comp *ddp_comp[DDP_COMPONENT_ID_MAX];
 	const struct mtk_mmsys_driver_data *data;
-- 
2.14.2

[RFC resend 3/4] arm64: dts: mt8173: Fix drm subsystem

[Matthias Brugger]

DRM subysystem and clock driver shared the same compatible mmsys.
This stopped does not work, as only the first driver for a compatible
gets probed. We change the comaptible to the new DRM identifier to fix
this.

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

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index b99a27372965..89db0a3f5950 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -803,6 +803,11 @@
 			#clock-cells = <1>;
 		};
 
+		dispsys: display-system {
+			compatible = "mediatek,mt2701-dispsys";
+			mediatek,mmsys = <&mmsys>;
+		}
+
 		mdp_rdma0: rdma at 14001000 {
 			compatible = "mediatek,mt8173-mdp-rdma",
 				     "mediatek,mt8173-mdp";
-- 
2.14.2

[RFC resend 4/4] arm: dts: mt2701: Fix drm subsystem

[Matthias Brugger]

DRM subysystem and clock driver shared the same compatible mmsys.
This stopped does not work, as only the first driver for a compatible
gets probed. We change the comaptible to the new DRM identifier to fix
this.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 arch/arm/boot/dts/mt2701.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
index afe12e5b51f9..1168ad69bb4e 100644
--- a/arch/arm/boot/dts/mt2701.dtsi
+++ b/arch/arm/boot/dts/mt2701.dtsi
@@ -530,6 +530,11 @@
 		#clock-cells = <1>;
 	};
 
+	dispsys: display-system {
+		compatible = "mediatek,mt2701-dispsys";
+		mediatek,mmsys = <&mmsys>;
+	}
+
 	larb0: larb at 14010000 {
 		compatible = "mediatek,mt2701-smi-larb";
 		reg = <0 0x14010000 0 0x1000>;
-- 
2.14.2

[RFC resend 1/4] dt-bindings: display: mediatek: add drm binding

[Ryder Lee]

Hi Matthias,

Should I base on your changes and resend this patch series
https://patchwork.kernel.org/patch/9980061/ ?

I add a similar node - display_components, but your approach is better
than mine.

Thanks.

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> DRM subysystem and clock driver shared the same compatible mmsys.
> This stopped does not work, as only the first driver for a compatible
> gets probed. We change the comaptible to the new DRM identifier to fix
> this.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  .../devicetree/bindings/display/mediatek/mediatek,disp.txt          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index 383183a89164..6db652463e64 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -27,6 +27,7 @@ Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
>  
>  Required properties (all function blocks):
>  - compatible: "mediatek,<chip>-disp-<function>", one of
> +	"mediatek,<chip>-dispsys"    - central component for the DRM system
>  	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
>  	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
>  	"mediatek,<chip>-disp-wdma"  - write DMA
> @@ -71,6 +72,11 @@ mmsys: clock-controller at 14000000 {
>  	#clock-cells = <1>;
>  };
>  
> +dispsys: display-system {
> +	compatible = "mediatek,mt2701-dispsys";
> +	mediatek,mmsys = <&mmsys>;
> +}
> +
>  ovl0: ovl at 1400c000 {
>  	compatible = "mediatek,mt8173-disp-ovl";
>  	reg = <0 0x1400c000 0 0x1000>;

[RFC resend 1/4] dt-bindings: display: mediatek: add drm binding

[Philipp Zabel]

Hi Matthias,

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> DRM subysystem and clock driver shared the same compatible mmsys.
> This stopped does not work, as only the first driver for a compatible
> gets probed. We change the comaptible to the new DRM identifier to fix
> this.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  .../devicetree/bindings/display/mediatek/mediatek,disp.txt          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index 383183a89164..6db652463e64 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -27,6 +27,7 @@ Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
>  
>  Required properties (all function blocks):
>  - compatible: "mediatek,<chip>-disp-<function>", one of
> +	"mediatek,<chip>-dispsys"    - central component for the DRM system
>  	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
>  	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
>  	"mediatek,<chip>-disp-wdma"  - write DMA
> @@ -71,6 +72,11 @@ mmsys: clock-controller at 14000000 {
>  	#clock-cells = <1>;
>  };
>  
> +dispsys: display-system {

Could we call this node display-subsystem for consistency with i.MX and
Rockchip device trees?

With that change,
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

> +	compatible = "mediatek,mt2701-dispsys";
> +	mediatek,mmsys = <&mmsys>;
> +}
> +
>  ovl0: ovl at 1400c000 {
>  	compatible = "mediatek,mt8173-disp-ovl";
>  	reg = <0 0x1400c000 0 0x1000>;

regards
Philipp

[RFC resend 3/4] arm64: dts: mt8173: Fix drm subsystem

[Philipp Zabel]

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> DRM subysystem and clock driver shared the same compatible mmsys.
> This stopped does not work, as only the first driver for a compatible
> gets probed. We change the comaptible to the new DRM identifier to fix
> this.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index b99a27372965..89db0a3f5950 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -803,6 +803,11 @@
>  			#clock-cells = <1>;
>  		};
>  
> +		dispsys: display-system {
> +			compatible = "mediatek,mt2701-dispsys";

Same comment as for patch 1, I'd prefer the node to be called
"display-
subsystem" instead.

With that changed,
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

> +			mediatek,mmsys = <&mmsys>;
> +		}
> +
>  		mdp_rdma0: rdma at 14001000 {
>  			compatible = "mediatek,mt8173-mdp-rdma",
>  				     "mediatek,mt8173-mdp";

regards
Philipp

[RFC resend 1/4] dt-bindings: display: mediatek: add drm binding

[Laurent Pinchart]

Hi Matthias,

Thank you for the patch.

On Thursday, 19 October 2017 14:26:07 EEST Matthias Brugger wrote:
> DRM subysystem and clock driver shared the same compatible mmsys.
> This stopped does not work, as only the first driver for a compatible
> gets probed. We change the comaptible to the new DRM identifier to fix
> this.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  .../devicetree/bindings/display/mediatek/mediatek,disp.txt          | 6 +++
>  1 file changed, 6 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index 383183a89164..6db652463e64 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -27,6 +27,7 @@
> Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
> 
>  Required properties (all function blocks):
>  - compatible: "mediatek,<chip>-disp-<function>", one of
> +	"mediatek,<chip>-dispsys"    - central component for the DRM system
>  	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
>  	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
>  	"mediatek,<chip>-disp-wdma"  - write DMA
> @@ -71,6 +72,11 @@ mmsys: clock-controller at 14000000 {
>  	#clock-cells = <1>;
>  };
> 
> +dispsys: display-system {
> +	compatible = "mediatek,mt2701-dispsys";
> +	mediatek,mmsys = <&mmsys>;
> +}

So this node doesn't correspond to an IP core but is meant as a top-level 
entry point for the operating system. This leads me to three questions.

1. Is there any IP core in the Mediatek display subsystem that could be 
considered (or at least used) as a top-level entry point ? That would be my 
preferred solution as I'm not fond of DT nodes not describing hardware.

2. If there's no such IP core, are all the display subsystem IP cores grouped 
together in one MMIO register range ? If so we could move them as children of 
this new display system node which, even if doesn't describe an IP core, would 
describe the way the display IP cores are grouped in the hardware, and would 
thus be a hardware description.

3. If the answer to the second question is also negative, shouldn't this 
display system node reference all other display IP DT nodes (through direct 
phandles and/or OF graph bindings) ?

>  ovl0: ovl at 1400c000 {
>  	compatible = "mediatek,mt8173-disp-ovl";
>  	reg = <0 0x1400c000 0 0x1000>;

-- 
Regards,

Laurent Pinchart

[RFC resend] arm64: mt8173: Fix Acer Chromebooks mmsys probe problem

[Philipp Zabel]

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> In theory the MMSYS device tree identifier is matches twice, by the clk driver 
> and the DRM subsystem. But the kernel only matches the first driver for a 
> device (clk) and discards the second one. This breaks graphics on mt8173 and 
> most probably on mt2701 as well.
> 
> MMSYS in Mediatek SoCs has some registers to control clock gates (which is 
> used in the clk driver) and some registers to enable the differnet blocks of 
> the display subsystem. The kernel uses the binding to load the central 
> comoponent of the distplay subsystem, which in place probes all the other 
> components and enables the present ones in the MMSYS.
> 
> We found us with the problem, that we need to change and therefor break one 
> of the two bindings, or the DRM one or the clock driver one.
> 
> Apart from that the DRM subysystem does access the MMSYS registers via relaxed 
> reads/writes. But the it should to so via regmap, as the registers are shared.
> 
> Possible solutions:
> 1) We add a new mediatek,mt8173-mmsys-clk node, which lives as a simple-mfd 
> under the actual mmsys node. We change the clock driver to probe on this 
> binding. This would make sense as the clock gate register live completly in 
> the MMSYS configuration registers.

The reason why the drm driver matches against the mmsys node in the
first place is that we wanted to avoid 2).
Also, mmsys is not a pure clock controller, as it also contains the
display path configuration in its register space.

> 2) As the nodes of the DRM subsystem just need some of the registers of MMSYS 
> we add a new binding mediatek,mt8173-dispsys which probes the central 
> component of the DRM system. It has only a handle to mt8173-mmsys to access 
> the registerspace via regmap functions.
> 
> In this patchset I implemented 2). Please take into account, that this is a 
> RFC. I had no time to actually test the verison on real HW. Some of the 
> register accesses should be done using regmap_update instead of 
> regmap_read + regmap_write.
> 
> This RFC shall only show how solution 2) would look like. We can use it as 
> discussion to see how we circumvent the actual situation.

Or we could leave the bindings untouched and create one platform device
from the other or even set up the clocks from the drm driver?

regards
Philipp

[RFC resend 1/4] dt-bindings: display: mediatek: add drm binding

[Philipp Zabel]

On Thu, 2017-10-19 at 15:53 +0300, Laurent Pinchart wrote:
> Hi Matthias,
> 
> Thank you for the patch.
> 
> On Thursday, 19 October 2017 14:26:07 EEST Matthias Brugger wrote:
> > DRM subysystem and clock driver shared the same compatible mmsys.
> > This stopped does not work, as only the first driver for a compatible
> > gets probed. We change the comaptible to the new DRM identifier to fix
> > this.
> > 
> > Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> > ---
> >  .../devicetree/bindings/display/mediatek/mediatek,disp.txt          | 6 +++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> > b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> > index 383183a89164..6db652463e64 100644
> > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> > @@ -27,6 +27,7 @@
> > Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
> > 
> >  Required properties (all function blocks):
> >  - compatible: "mediatek,<chip>-disp-<function>", one of
> > +	"mediatek,<chip>-dispsys"    - central component for the DRM system
> >  	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
> >  	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
> >  	"mediatek,<chip>-disp-wdma"  - write DMA
> > @@ -71,6 +72,11 @@ mmsys: clock-controller at 14000000 {
> >  	#clock-cells = <1>;
> >  };
> > 
> > +dispsys: display-system {
> > +	compatible = "mediatek,mt2701-dispsys";
> > +	mediatek,mmsys = <&mmsys>;
> > +}
> 
> So this node doesn't correspond to an IP core but is meant as a top-level 
> entry point for the operating system. This leads me to three questions.
> 
> 1. Is there any IP core in the Mediatek display subsystem that could be 
> considered (or at least used) as a top-level entry point ? That would be my 
> preferred solution as I'm not fond of DT nodes not describing hardware.

At least on MT8173 that node is MMSYS, which it is currently matching
against. The issue, if I understand correctly, is that the clocks
provided by this same region were previously created via CLK_OF_DECLARE,
and are now changed to a separate clock driver that matches to the same 
node.

> 2. If there's no such IP core, are all the display subsystem IP cores grouped 
> together in one MMIO register range ? If so we could move them as children of 
> this new display system node which, even if doesn't describe an IP core, would 
> describe the way the display IP cores are grouped in the hardware, and would 
> thus be a hardware description.
> 
> 3. If the answer to the second question is also negative, shouldn't this 
> display system node reference all other display IP DT nodes (through direct 
> phandles and/or OF graph bindings) ?
> 
> >  ovl0: ovl at 1400c000 {
> >  	compatible = "mediatek,mt8173-disp-ovl";
> >  	reg = <0 0x1400c000 0 0x1000>;
> 

regards
Philipp

[RFC resend] arm64: mt8173: Fix Acer Chromebooks mmsys probe problem

[Laurent Pinchart]

Hi Philipp,

On Thursday, 19 October 2017 16:01:54 EEST Philipp Zabel wrote:
> On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> > In theory the MMSYS device tree identifier is matches twice, by the clk
> > driver and the DRM subsystem. But the kernel only matches the first
> > driver for a device (clk) and discards the second one. This breaks
> > graphics on mt8173 and most probably on mt2701 as well.
> > 
> > MMSYS in Mediatek SoCs has some registers to control clock gates (which is
> > used in the clk driver) and some registers to enable the differnet blocks
> > of the display subsystem. The kernel uses the binding to load the central
> > comoponent of the distplay subsystem, which in place probes all the other
> > components and enables the present ones in the MMSYS.
> > 
> > We found us with the problem, that we need to change and therefor break
> > one
> > of the two bindings, or the DRM one or the clock driver one.
> > 
> > Apart from that the DRM subysystem does access the MMSYS registers via
> > relaxed reads/writes. But the it should to so via regmap, as the
> > registers are shared.
> > 
> > Possible solutions:
> > 1) We add a new mediatek,mt8173-mmsys-clk node, which lives as a
> > simple-mfd under the actual mmsys node. We change the clock driver to
> > probe on this binding. This would make sense as the clock gate register
> > live completly in the MMSYS configuration registers.
> 
> The reason why the drm driver matches against the mmsys node in the
> first place is that we wanted to avoid 2).

Why did you want to avoid 2) ?

> Also, mmsys is not a pure clock controller, as it also contains the
> display path configuration in its register space.

Which makes the mmsys related to display, but more in a syscon (combining 
clocks and routing, and I assume other miscellaneous features that wouldn't 
fit nicely in the other display-related IP cores) way than actually being part 
of the display subsystem. Or does mmsys only provide display-related features 
?

> > 2) As the nodes of the DRM subsystem just need some of the registers of
> > MMSYS we add a new binding mediatek,mt8173-dispsys which probes the
> > central component of the DRM system. It has only a handle to mt8173-mmsys
> > to access the registerspace via regmap functions.
> > 
> > In this patchset I implemented 2). Please take into account, that this is
> > a RFC. I had no time to actually test the verison on real HW. Some of the
> > register accesses should be done using regmap_update instead of
> > regmap_read + regmap_write.
> > 
> > This RFC shall only show how solution 2) would look like. We can use it as
> > discussion to see how we circumvent the actual situation.
> 
> Or we could leave the bindings untouched and create one platform device
> from the other or even set up the clocks from the drm driver?

Does mmsys provide features (such as clocks) to non-display IP cores ?

-- 
Regards,

Laurent Pinchart

[RFC resend 1/4] dt-bindings: display: mediatek: add drm binding

[Matthias Brugger]

On 10/19/2017 02:19 PM, Ryder Lee wrote:
> Hi Matthias,
> 
> Should I base on your changes and resend this patch series
> https://patchwork.kernel.org/patch/9980061/ ?
> 
> I add a similar node - display_components, but your approach is better
> than mine.
> 

You series should have the same issue as the Ulrich sees on the chromebook.
Basically you have two nodes which both bind to mediatek,mt7623-mmsys.

The only difference here is, that your clock drivers is a
builtin_platform_driver while on mt8173 it get's probed earlier as it is defined
as CLK_OF_DECLARE.

Do you see both drivers getting probed? I don't have my mt7623 board at hand
right now to check this.

In any case, please wait until we found a way to fix the issue before we add
these bindings.

Regards,
Matthias

PS @ryder: I have the rest of the series on my radar, between today and tomorrow
I will look into this

> Thanks.
> 
> On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
>> DRM subysystem and clock driver shared the same compatible mmsys.
>> This stopped does not work, as only the first driver for a compatible
>> gets probed. We change the comaptible to the new DRM identifier to fix
>> this.
>>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>> ---
>>  .../devicetree/bindings/display/mediatek/mediatek,disp.txt          | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>> index 383183a89164..6db652463e64 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>> @@ -27,6 +27,7 @@ Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
>>  
>>  Required properties (all function blocks):
>>  - compatible: "mediatek,<chip>-disp-<function>", one of
>> +	"mediatek,<chip>-dispsys"    - central component for the DRM system
>>  	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
>>  	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
>>  	"mediatek,<chip>-disp-wdma"  - write DMA
>> @@ -71,6 +72,11 @@ mmsys: clock-controller at 14000000 {
>>  	#clock-cells = <1>;
>>  };
>>  
>> +dispsys: display-system {
>> +	compatible = "mediatek,mt2701-dispsys";
>> +	mediatek,mmsys = <&mmsys>;
>> +}
>> +
>>  ovl0: ovl at 1400c000 {
>>  	compatible = "mediatek,mt8173-disp-ovl";
>>  	reg = <0 0x1400c000 0 0x1000>;
> 
> 
> 

[RFC resend] arm64: mt8173: Fix Acer Chromebooks mmsys probe problem

[Philipp Zabel]

Hi Laurent,

On Thu, 2017-10-19 at 16:39 +0300, Laurent Pinchart wrote:
> Hi Philipp,
> 
> On Thursday, 19 October 2017 16:01:54 EEST Philipp Zabel wrote:
> > On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> > > In theory the MMSYS device tree identifier is matches twice, by the clk
> > > driver and the DRM subsystem. But the kernel only matches the first
> > > driver for a device (clk) and discards the second one. This breaks
> > > graphics on mt8173 and most probably on mt2701 as well.
> > > 
> > > MMSYS in Mediatek SoCs has some registers to control clock gates (which is
> > > used in the clk driver) and some registers to enable the differnet blocks
> > > of the display subsystem. The kernel uses the binding to load the central
> > > comoponent of the distplay subsystem, which in place probes all the other
> > > components and enables the present ones in the MMSYS.
> > > 
> > > We found us with the problem, that we need to change and therefor break
> > > one
> > > of the two bindings, or the DRM one or the clock driver one.
> > > 
> > > Apart from that the DRM subysystem does access the MMSYS registers via
> > > relaxed reads/writes. But the it should to so via regmap, as the
> > > registers are shared.
> > > 
> > > Possible solutions:
> > > 1) We add a new mediatek,mt8173-mmsys-clk node, which lives as a
> > > simple-mfd under the actual mmsys node. We change the clock driver to
> > > probe on this binding. This would make sense as the clock gate register
> > > live completly in the MMSYS configuration registers.
> > 
> > The reason why the drm driver matches against the mmsys node in the
> > first place is that we wanted to avoid 2).
> 
> Why did you want to avoid 2) ?

Because the "display-subsystem" node does not represent a real device,
it's just there to probe the driver that stitches all the DISP
components together.

> > Also, mmsys is not a pure clock controller, as it also contains the
> > display path configuration in its register space.
> 
> Which makes the mmsys related to display, but more in a syscon (combining 
> clocks and routing, and I assume other miscellaneous features that wouldn't 
> fit nicely in the other display-related IP cores) way than actually being part 
> of the display subsystem. Or does mmsys only provide display-related features 
> ?

All devices in the 0x14000000 - 0x14ffffff memory range are part of the
MMSYS system. That includes the MMSYS control or system configuration
block at 0x14000000 - 0x14000fff as well as all the related MDP (media
data path) and DISP (display data path) blocks that follow. The DISP
blocks are purely display related, while the MDP blocks implement
implement mem2mem functions like scaling and conversion.

> > > 2) As the nodes of the DRM subsystem just need some of the registers of
> > > MMSYS we add a new binding mediatek,mt8173-dispsys which probes the
> > > central component of the DRM system. It has only a handle to mt8173-mmsys
> > > to access the registerspace via regmap functions.
> > > 
> > > In this patchset I implemented 2). Please take into account, that this is
> > > a RFC. I had no time to actually test the verison on real HW. Some of the
> > > register accesses should be done using regmap_update instead of
> > > regmap_read + regmap_write.
> > > 
> > > This RFC shall only show how solution 2) would look like. We can use it as
> > > discussion to see how we circumvent the actual situation.
> > 
> > Or we could leave the bindings untouched and create one platform device
> > from the other or even set up the clocks from the drm driver?
> 
> Does mmsys provide features (such as clocks) to non-display IP cores ?

The MMSYS control block provides clocks for the DISP (display data path)
and MDP (multimedia data path) blocks, as well as the routing between
them, but not to anything outside of the MMSYS system.

regards
Philipp

[RFC resend 1/4] dt-bindings: display: mediatek: add drm binding

[Matthias Brugger]

On 10/19/2017 03:06 PM, Philipp Zabel wrote:
> On Thu, 2017-10-19 at 15:53 +0300, Laurent Pinchart wrote:
>> Hi Matthias,
>>
>> Thank you for the patch.
>>
>> On Thursday, 19 October 2017 14:26:07 EEST Matthias Brugger wrote:
>>> DRM subysystem and clock driver shared the same compatible mmsys.
>>> This stopped does not work, as only the first driver for a compatible
>>> gets probed. We change the comaptible to the new DRM identifier to fix
>>> this.
>>>
>>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>>> ---
>>>  .../devicetree/bindings/display/mediatek/mediatek,disp.txt          | 6 +++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>>> b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>>> index 383183a89164..6db652463e64 100644
>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
>>> @@ -27,6 +27,7 @@
>>> Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
>>>
>>>  Required properties (all function blocks):
>>>  - compatible: "mediatek,<chip>-disp-<function>", one of
>>> +	"mediatek,<chip>-dispsys"    - central component for the DRM system
>>>  	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
>>>  	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
>>>  	"mediatek,<chip>-disp-wdma"  - write DMA
>>> @@ -71,6 +72,11 @@ mmsys: clock-controller at 14000000 {
>>>  	#clock-cells = <1>;
>>>  };
>>>
>>> +dispsys: display-system {
>>> +	compatible = "mediatek,mt2701-dispsys";
>>> +	mediatek,mmsys = <&mmsys>;
>>> +}
>>
>> So this node doesn't correspond to an IP core but is meant as a top-level 
>> entry point for the operating system. This leads me to three questions.
>>
>> 1. Is there any IP core in the Mediatek display subsystem that could be 
>> considered (or at least used) as a top-level entry point ? That would be my 
>> preferred solution as I'm not fond of DT nodes not describing hardware.
> 
> At least on MT8173 that node is MMSYS, which it is currently matching
> against. The issue, if I understand correctly, is that the clocks
> provided by this same region were previously created via CLK_OF_DECLARE,
> and are now changed to a separate clock driver that matches to the same 
> node.
> 

Exactly that seems to be the problem we hit. I have to setup my chromebook to do
some changes, but that won't happen before the week after next week.

So yes, the MMSYS is the top-level-entry point for the display subsystem, the
clocks in mmsys are just a small part of the whole register block. I will cite
the cover letter which unfortunately wasn't send, because I broke my email setup:
"Possible solutions:
1) We add a new mediatek,mt8173-mmsys-clk node, which lives as a simple-mfd
under the actual mmsys node. We change the clock driver to probe on this
binding. This would make sense as the clock gate register live completely in
the MMSYS configuration registers.

2) As the nodes of the DRM subsystem just need some of the registers of MMSYS
we add a new binding mediatek,mt8173-dispsys which probes the central
component of the DRM system. It has only a handle to mt8173-mmsys to access
the registerspace via regmap functions."

So this patch set implemented solution 2).


>> 2. If there's no such IP core, are all the display subsystem IP cores grouped 
>> together in one MMIO register range ? If so we could move them as children of 
>> this new display system node which, even if doesn't describe an IP core, would 
>> describe the way the display IP cores are grouped in the hardware, and would 
>> thus be a hardware description.
>>

They are all mapped somewhere at 140xxxxx. But there are other components which
are used by other HW blocks smi_common especially. So I'm not sure which impact
that move would have.

The MMSYS for mt8173 actually enables the overlays and set's the multiplexer for
the output path. Does this make sense? It's the first time I've a deeper look in
such a driver, so maybe I don't grasp everything.

Regards,
Matthias

>> 3. If the answer to the second question is also negative, shouldn't this 
>> display system node reference all other display IP DT nodes (through direct 
>> phandles and/or OF graph bindings) ?
>>
>>>  ovl0: ovl at 1400c000 {
>>>  	compatible = "mediatek,mt8173-disp-ovl";
>>>  	reg = <0 0x1400c000 0 0x1000>;
>>
> 
> regards
> Philipp
> 

[RFC resend 3/4] arm64: dts: mt8173: Fix drm subsystem

[CK Hu]

Hi, Matthias:

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> DRM subysystem and clock driver shared the same compatible mmsys.
> This stopped does not work, as only the first driver for a compatible
> gets probed. We change the comaptible to the new DRM identifier to fix
> this.
> 
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index b99a27372965..89db0a3f5950 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -803,6 +803,11 @@
>  			#clock-cells = <1>;
>  		};
>  
> +		dispsys: display-system {
> +			compatible = "mediatek,mt2701-dispsys";

Why do you probe "mediatek,mt2701-dispsys" in mt8173.dtsi?

Regards,
CK

> +			mediatek,mmsys = <&mmsys>;
> +		}
> +
>  		mdp_rdma0: rdma at 14001000 {
>  			compatible = "mediatek,mt8173-mdp-rdma",
>  				     "mediatek,mt8173-mdp";

[RFC resend 3/4] arm64: dts: mt8173: Fix drm subsystem

[Matthias Brugger]

On 10/20/2017 11:16 AM, CK Hu wrote:
> Hi, Matthias:
> 
> On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
>> DRM subysystem and clock driver shared the same compatible mmsys.
>> This stopped does not work, as only the first driver for a compatible
>> gets probed. We change the comaptible to the new DRM identifier to fix
>> this.
>>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>> ---
>>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> index b99a27372965..89db0a3f5950 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> @@ -803,6 +803,11 @@
>>  			#clock-cells = <1>;
>>  		};
>>  
>> +		dispsys: display-system {
>> +			compatible = "mediatek,mt2701-dispsys";
> 
> Why do you probe "mediatek,mt2701-dispsys" in mt8173.dtsi?
> 

That's actually a copy-paste-error. Thanks for noting!

Matthias

> Regards,
> CK
> 
>> +			mediatek,mmsys = <&mmsys>;
>> +		}
>> +
>>  		mdp_rdma0: rdma at 14001000 {
>>  			compatible = "mediatek,mt8173-mdp-rdma",
>>  				     "mediatek,mt8173-mdp";
> 
> 

[RFC resend] arm64: mt8173: Fix Acer Chromebooks mmsys probe problem

[CK Hu]

Hi,

On Thu, 2017-10-19 at 16:54 +0200, Philipp Zabel wrote:
> Hi Laurent,
> 
> On Thu, 2017-10-19 at 16:39 +0300, Laurent Pinchart wrote:
> > Hi Philipp,
> > 
> > On Thursday, 19 October 2017 16:01:54 EEST Philipp Zabel wrote:
> > > On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> > > > In theory the MMSYS device tree identifier is matches twice, by the clk
> > > > driver and the DRM subsystem. But the kernel only matches the first
> > > > driver for a device (clk) and discards the second one. This breaks
> > > > graphics on mt8173 and most probably on mt2701 as well.
> > > > 
> > > > MMSYS in Mediatek SoCs has some registers to control clock gates (which is
> > > > used in the clk driver) and some registers to enable the differnet blocks
> > > > of the display subsystem. The kernel uses the binding to load the central
> > > > comoponent of the distplay subsystem, which in place probes all the other
> > > > components and enables the present ones in the MMSYS.
> > > > 
> > > > We found us with the problem, that we need to change and therefor break
> > > > one
> > > > of the two bindings, or the DRM one or the clock driver one.
> > > > 
> > > > Apart from that the DRM subysystem does access the MMSYS registers via
> > > > relaxed reads/writes. But the it should to so via regmap, as the
> > > > registers are shared.
> > > > 
> > > > Possible solutions:
> > > > 1) We add a new mediatek,mt8173-mmsys-clk node, which lives as a
> > > > simple-mfd under the actual mmsys node. We change the clock driver to
> > > > probe on this binding. This would make sense as the clock gate register
> > > > live completly in the MMSYS configuration registers.
> > > 
> > > The reason why the drm driver matches against the mmsys node in the
> > > first place is that we wanted to avoid 2).
> > 
> > Why did you want to avoid 2) ?
> 
> Because the "display-subsystem" node does not represent a real device,
> it's just there to probe the driver that stitches all the DISP
> components together.
> 
> > > Also, mmsys is not a pure clock controller, as it also contains the
> > > display path configuration in its register space.
> > 
> > Which makes the mmsys related to display, but more in a syscon (combining 
> > clocks and routing, and I assume other miscellaneous features that wouldn't 
> > fit nicely in the other display-related IP cores) way than actually being part 
> > of the display subsystem. Or does mmsys only provide display-related features 
> > ?
> 
> All devices in the 0x14000000 - 0x14ffffff memory range are part of the
> MMSYS system. That includes the MMSYS control or system configuration
> block at 0x14000000 - 0x14000fff as well as all the related MDP (media
> data path) and DISP (display data path) blocks that follow. The DISP
> blocks are purely display related, while the MDP blocks implement
> implement mem2mem functions like scaling and conversion.
> 
> > > > 2) As the nodes of the DRM subsystem just need some of the registers of
> > > > MMSYS we add a new binding mediatek,mt8173-dispsys which probes the
> > > > central component of the DRM system. It has only a handle to mt8173-mmsys
> > > > to access the registerspace via regmap functions.
> > > > 
> > > > In this patchset I implemented 2). Please take into account, that this is
> > > > a RFC. I had no time to actually test the verison on real HW. Some of the
> > > > register accesses should be done using regmap_update instead of
> > > > regmap_read + regmap_write.
> > > > 
> > > > This RFC shall only show how solution 2) would look like. We can use it as
> > > > discussion to see how we circumvent the actual situation.
> > > 
> > > Or we could leave the bindings untouched and create one platform device
> > > from the other or even set up the clocks from the drm driver?
> > 
> > Does mmsys provide features (such as clocks) to non-display IP cores ?
> 
> The MMSYS control block provides clocks for the DISP (display data path)
> and MDP (multimedia data path) blocks, as well as the routing between
> them, but not to anything outside of the MMSYS system.

According to register table of mediatek x20 mmsys [1] and Philipp's
statement, I think mmsys is neither clock-controller nor display
controller. It's a combination of multiple function. The four major
function is display's clock-control, mdp's clock-control, display's
routing, and mdp's routing. So we have two choice:

1) Centralize these multiple function control inside mmsys device: This
means there is only a mmsys device which contains function of
clock-control, display, and mdp.

2) Separate these multiple function to different device: mmsys is the
major device which owns the register resource but does nothing. The
function is controlled by three virtual device: mmsys-clock-controller,
display-controller, and mdp-controller.

I prefer 2) because these function seems independent. 

Regards,
CK

[1]
https://www.96boards.org/documentation/ConsumerEdition/MediaTekX20/AdditionalDocs/

> 
> regards
> Philipp

[RFC resend] arm64: mt8173: Fix Acer Chromebooks mmsys probe problem

[Laurent Pinchart]

Hi Philipp,

On Thursday, 19 October 2017 17:54:16 EEST Philipp Zabel wrote:
> On Thu, 2017-10-19 at 16:39 +0300, Laurent Pinchart wrote:
> > On Thursday, 19 October 2017 16:01:54 EEST Philipp Zabel wrote:
> >> On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:
> >>> In theory the MMSYS device tree identifier is matches twice, by the
> >>> clk driver and the DRM subsystem. But the kernel only matches the
> >>> first driver for a device (clk) and discards the second one. This
> >>> breaks graphics on mt8173 and most probably on mt2701 as well.
> >>> 
> >>> MMSYS in Mediatek SoCs has some registers to control clock gates
> >>> (which is used in the clk driver) and some registers to enable the
> >>> differnet blocks of the display subsystem. The kernel uses the binding
> >>> to load the central comoponent of the distplay subsystem, which in
> >>> place probes all the other components and enables the present ones in
> >>> the MMSYS.
> >>> 
> >>> We found us with the problem, that we need to change and therefor
> >>> break one of the two bindings, or the DRM one or the clock driver one.
> >>> 
> >>> Apart from that the DRM subysystem does access the MMSYS registers via
> >>> relaxed reads/writes. But the it should to so via regmap, as the
> >>> registers are shared.
> >>> 
> >>> Possible solutions:
> >>> 1) We add a new mediatek,mt8173-mmsys-clk node, which lives as a
> >>> simple-mfd under the actual mmsys node. We change the clock driver to
> >>> probe on this binding. This would make sense as the clock gate
> >>> register live completly in the MMSYS configuration registers.
> >> 
> >> The reason why the drm driver matches against the mmsys node in the
> >> first place is that we wanted to avoid 2).
> > 
> > Why did you want to avoid 2) ?
> 
> Because the "display-subsystem" node does not represent a real device,
> it's just there to probe the driver that stitches all the DISP components
> together.

I'm not a big supported of such DT nodes for "logical" devices either. When 
possible I prefer describing the relationship between display IP cores using 
OF graph DT bindings.

In some cases (and I don't know if the Mediatek display is one of them) there 
is no single IP core that can be considered as a master from a display point 
of view. This is inconvenient for device drivers as there's no clear place for 
an entry point. I could thus be convinced to accept DT bindings for a logical 
display DT node when there's really no other good choice.

> >> Also, mmsys is not a pure clock controller, as it also contains the
> >> display path configuration in its register space.
> > 
> > Which makes the mmsys related to display, but more in a syscon (combining
> > clocks and routing, and I assume other miscellaneous features that
> > wouldn't fit nicely in the other display-related IP cores) way than
> > actually being part of the display subsystem. Or does mmsys only provide
> > display-related features ?
> 
> All devices in the 0x14000000 - 0x14ffffff memory range are part of the
> MMSYS system. That includes the MMSYS control or system configuration
> block at 0x14000000 - 0x14000fff as well as all the related MDP (media
> data path) and DISP (display data path) blocks that follow. The DISP
> blocks are purely display related, while the MDP blocks implement
> implement mem2mem functions like scaling and conversion.

Without more information about the hardware it's hard to tell whether the DT 
nodes for the DISP and MDP IP cores should be children of the MMSYS DT node or 
not. In any case, it looks like the MMSYS is a syscon that provides 
miscellaneous functions not fitting anywhere else.

On simple solution, which shouldn't require DT changes, would be to merge the 
MMSYS clock driver into the mediatek DRM driver, and register the MMSYS clocks 
at probe time instead of using CLK_OF_DECLARE.

Another option would be to handle MMSYS as an MFD. That shouldn't require DT 
changes either.

> >>> 2) As the nodes of the DRM subsystem just need some of the registers
> >>> of MMSYS we add a new binding mediatek,mt8173-dispsys which probes the
> >>> central component of the DRM system. It has only a handle to mt8173-
> >>> mmsys to access the registerspace via regmap functions.
> >>> 
> >>> In this patchset I implemented 2). Please take into account, that this
> >>> is a RFC. I had no time to actually test the verison on real HW. Some
> >>> of the register accesses should be done using regmap_update instead of
> >>> regmap_read + regmap_write.
> >>> 
> >>> This RFC shall only show how solution 2) would look like. We can use
> >>> it as discussion to see how we circumvent the actual situation.
> >> 
> >> Or we could leave the bindings untouched and create one platform device
> >> from the other or even set up the clocks from the drm driver?
> > 
> > Does mmsys provide features (such as clocks) to non-display IP cores ?
> 
> The MMSYS control block provides clocks for the DISP (display data path)
> and MDP (multimedia data path) blocks, as well as the routing between
> them, but not to anything outside of the MMSYS system.

-- 
Regards,

Laurent Pinchart