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

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.

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

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

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

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

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:

Hi Matthias,

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:

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

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:

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

Hi Matthias,

Thank you for the patch.

On Thursday, 19 October 2017 14:26:07 EEST Matthias Brugger wrote:

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

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

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:

quoted

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) ?

quoted

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

regards
Philipp

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:

quoted

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 
?

quoted

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

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:

quoted

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

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:

quoted

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:

quoted

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.

quoted

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.

quoted

quoted

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

On 10/19/2017 03:06 PM, Philipp Zabel wrote:

On Thu, 2017-10-19 at 15:53 +0300, Laurent Pinchart wrote:

quoted

Hi Matthias,

Thank you for the patch.

On Thursday, 19 October 2017 14:26:07 EEST Matthias Brugger wrote:

quoted

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

quoted

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

quoted

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) ?

quoted

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

regards
Philipp

Hi, Matthias:

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:

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

On 10/20/2017 11:16 AM, CK Hu wrote:

Hi, Matthias:

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:

quoted

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

quoted

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

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:

quoted

Hi Philipp,

On Thursday, 19 October 2017 16:01:54 EEST Philipp Zabel wrote:

quoted

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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

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:

quoted

On Thursday, 19 October 2017 16:01:54 EEST Philipp Zabel wrote:

quoted

On Thu, 2017-10-19 at 13:26 +0200, Matthias Brugger wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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