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.
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@14000000 { #clock-cells = <1>; };
+dispsys: display-system { + compatible = "mediatek,mt2701-dispsys"; + mediatek,mmsys = <&mmsys>; +} + ovl0: ovl@1400c000 { compatible = "mediatek,mt8173-disp-ovl"; reg = <0 0x1400c000 0 0x1000>;
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@14000000 { #clock-cells = <1>; };
+dispsys: display-system {
- compatible = "mediatek,mt2701-dispsys";
- mediatek,mmsys = <&mmsys>;
+}
ovl0: ovl@1400c000 { compatible = "mediatek,mt8173-disp-ovl"; reg = <0 0x1400c000 0 0x1000>;
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@14000000 { #clock-cells = <1>; };
+dispsys: display-system {
- compatible = "mediatek,mt2701-dispsys";
- mediatek,mmsys = <&mmsys>;
+}
ovl0: ovl@1400c000 { compatible = "mediatek,mt8173-disp-ovl"; reg = <0 0x1400c000 0 0x1000>;
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@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@1400c000 { compatible = "mediatek,mt8173-disp-ovl"; reg = <0 0x1400c000 0 0x1000>;
regards Philipp
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@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@1400c000 { compatible = "mediatek,mt8173-disp-ovl"; reg = <0 0x1400c000 0 0x1000>;
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@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.
- 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.
- 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.
- 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@1400c000 { compatible = "mediatek,mt8173-disp-ovl"; reg = <0 0x1400c000 0 0x1000>;
regards Philipp
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@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.
- 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).
- 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
- 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@1400c000 { compatible = "mediatek,mt8173-disp-ovl"; reg = <0 0x1400c000 0 0x1000>;
regards Philipp
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 |= 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 |= 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 &= ~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 &= ~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;
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@14001000 { compatible = "mediatek,mt8173-mdp-rdma", "mediatek,mt8173-mdp";
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@14001000 { compatible = "mediatek,mt8173-mdp-rdma", "mediatek,mt8173-mdp";
regards Philipp
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@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:
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@14001000 { compatible = "mediatek,mt8173-mdp-rdma", "mediatek,mt8173-mdp";
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@14010000 { compatible = "mediatek,mt2701-smi-larb"; reg = <0 0x14010000 0 0x1000>;
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:
- 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.
- 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
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:
- 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 ?
- 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 ?
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:
- 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.
- 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
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:
- 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.
- 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/Additiona...
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:
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:
- 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.
- 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.
dri-devel@lists.freedesktop.org
-
CK Hu -
Laurent Pinchart -
Matthias Brugger -
Matthias Brugger -
Philipp Zabel -
Ryder Lee