Skip to contentSkip to conversation

soc: mediatek: mmsys: fix HDMI output on mt7623/bananapi-r2

PATCH In review Frank Wunderlich 4 participants 8 messages Jul 10, 2021 – Jul 26, 2021 Arm MediaTek

lore
Share & tools
lore

Frank Wunderlich

author
#0 lore raw

PATCH 1/1 soc: mediatek: mmsys: fix HDMI output on mt7623/bananapi-r2

From: Frank Wunderlich <[email protected]>

HDMI output was broken on mt7623/BPI-R2 in 5.13 because function for special output selection (mtk_mmsys_ddp_sout_sel) was dropped. This function wrote 3 registers at one time and so it is not compatible with the array-approach.

I re-added the old function and call this after the default routing table was applied. This default routing table can still be used as the only connection handled by mmsys on BPI-R2 is OVL0->RDMA0 and this is already present in the default routing table

Fixes 440147639ac7 ("soc: mediatek: mmsys: Use an array for setting the routing registers")
Signed-off-by Frank Wunderlich <[email protected]>
---
 drivers/soc/mediatek/mtk-mmsys.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
drivers/soc/mediatek/mtk-mmsys.c +21 -0
index 080660ef11bf..f91b7fdd417a 100644
@@ -57,6 +57,25 @@ struct mtk_mmsys {
 	const struct mtk_mmsys_driver_data *data;
 };
 
+static void mtk_mmsys_ddp_sout_sel(struct device *dev,
+				   enum mtk_ddp_comp_id cur,
+				   enum mtk_ddp_comp_id next)
+{
+	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
+
+	if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
+		writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
+			       mmsys->regs + DISP_REG_CONFIG_OUT_SEL);
+	} else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
+		writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
+			       mmsys->regs + DISP_REG_CONFIG_OUT_SEL);
+		writel_relaxed(DSI_SEL_IN_RDMA,
+			       mmsys->regs + DISP_REG_CONFIG_DSI_SEL);
+		writel_relaxed(DPI_SEL_IN_BLS,
+			       mmsys->regs + DISP_REG_CONFIG_DPI_SEL);
+	}
+}
+
 void mtk_mmsys_ddp_connect(struct device *dev,
 			   enum mtk_ddp_comp_id cur,
 			   enum mtk_ddp_comp_id next)

@@ -71,6 +90,8 @@ void mtk_mmsys_ddp_connect(struct device *dev,
 			reg = readl_relaxed(mmsys->regs + routes[i].addr) | routes[i].val;
 			writel_relaxed(reg, mmsys->regs + routes[i].addr);
 		}
+
+	mtk_mmsys_ddp_sout_sel(dev, cur, next);
 }
 EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_connect);
Signature · 7 lines
2.25.1


_______________________________________________
Linux-mediatek mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-mediatek
#1 lore raw

Hi

Quoted 32 lines · Frank Wunderlich

From: Frank Wunderlich <[email protected]>

HDMI output was broken on mt7623/BPI-R2 in 5.13 because function for special output selection (mtk_mmsys_ddp_sout_sel) was dropped. This function wrote 3 registers at one time and so it is not compatible with the array-approach.

I re-added the old function and call this after the default routing table was applied. This default routing table can still be used as the only connection handled by mmsys on BPI-R2 is OVL0->RDMA0 and this is already present in the default routing table

Fixes 440147639ac7 ("soc: mediatek: mmsys: Use an array for setting the routing registers")
Signed-off-by Frank Wunderlich <[email protected]>
---
  drivers/soc/mediatek/mtk-mmsys.c | 21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)
drivers/soc/mediatek/mtk-mmsys.c +6 -0
index 080660ef11bf..f91b7fdd417a 100644
@@ -57,6 +57,25 @@ struct mtk_mmsys {
  	const struct mtk_mmsys_driver_data *data;
  };
 
+static void mtk_mmsys_ddp_sout_sel(struct device *dev,
+				   enum mtk_ddp_comp_id cur,
+				   enum mtk_ddp_comp_id next)
+{
+	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
+

drivers/soc/mediatek/mtk-mmsys.c struct mtk_mmsys { · line 75

Quoted 10 lines
drivers/soc/mediatek/mtk-mmsys.c +10 -0
+	if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
+		writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
+			       mmsys->regs + DISP_REG_CONFIG_OUT_SEL);
+	} else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
+		writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
+			       mmsys->regs + DISP_REG_CONFIG_OUT_SEL);
+		writel_relaxed(DSI_SEL_IN_RDMA,
+			       mmsys->regs + DISP_REG_CONFIG_DSI_SEL);
+		writel_relaxed(DPI_SEL_IN_BLS,
+			       mmsys->regs + DISP_REG_CONFIG_DPI_SEL);

you can still use the array approach for this like that:

        {
                DDP_COMPONENT_BLS, DDP_COMPONENT_DSI0,
                DISP_REG_CONFIG_OUT_SEL, BLS_TO_DSI_RDMA1_TO_DPI1
        }, {
        {
                DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
                DISP_REG_CONFIG_OUT_SEL, BLS_TO_DPI_RDMA1_TO_DSI
        }, {
        {
                DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
                DISP_REG_CONFIG_DSI_SEL, DSI_SEL_IN_RDMA
        }, {
        {
                DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
                DISP_REG_CONFIG_DPI_SEL, DPI_SEL_IN_BLS
        }, {

Thanks,
Dafna

Quoted 16 lines
drivers/soc/mediatek/mtk-mmsys.c +5 -0
+	}
+}
+
  void mtk_mmsys_ddp_connect(struct device *dev,
  			   enum mtk_ddp_comp_id cur,
  			   enum mtk_ddp_comp_id next)

@@ -71,6 +90,8 @@ void mtk_mmsys_ddp_connect(struct device *dev,
  			reg = readl_relaxed(mmsys->regs + routes[i].addr) | routes[i].val;
  			writel_relaxed(reg, mmsys->regs + routes[i].addr);
  		}
+
+	mtk_mmsys_ddp_sout_sel(dev, cur, next);
  }
  EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_connect);
#2 lore raw
Quoted 72 lines

Gesendet: Montag, 12. Juli 2021 um 13:50 Uhr
Von: "Dafna Hirschfeld" <[email protected]>
An: "Frank Wunderlich" <[email protected]>, [email protected]
Cc: "Frank Wunderlich" <[email protected]>, "Matthias Brugger" <[email protected]>, "CK Hu" <[email protected]>, [email protected], [email protected], "Enric Balletbo Serra" <[email protected]>, "David Airlie" <[email protected]>, [email protected], "Daniel Vetter" <[email protected]>
Betreff: Re: [PATCH] soc: mediatek: mmsys: fix HDMI output on mt7623/bananapi-r2

Hi

Quoted 32 lines · Frank Wunderlich

From: Frank Wunderlich <[email protected]>

HDMI output was broken on mt7623/BPI-R2 in 5.13 because function for special output selection (mtk_mmsys_ddp_sout_sel) was dropped. This function wrote 3 registers at one time and so it is not compatible with the array-approach.

I re-added the old function and call this after the default routing table was applied. This default routing table can still be used as the only connection handled by mmsys on BPI-R2 is OVL0->RDMA0 and this is already present in the default routing table

Fixes 440147639ac7 ("soc: mediatek: mmsys: Use an array for setting the routing registers")
Signed-off-by Frank Wunderlich <[email protected]>
---
  drivers/soc/mediatek/mtk-mmsys.c | 21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)
drivers/soc/mediatek/mtk-mmsys.c +6 -0
index 080660ef11bf..f91b7fdd417a 100644
@@ -57,6 +57,25 @@ struct mtk_mmsys {
  	const struct mtk_mmsys_driver_data *data;
  };
 
+static void mtk_mmsys_ddp_sout_sel(struct device *dev,
+				   enum mtk_ddp_comp_id cur,
+				   enum mtk_ddp_comp_id next)
+{
+	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
+

drivers/soc/mediatek/mtk-mmsys.c struct mtk_mmsys { · line 75

Quoted 10 lines
drivers/soc/mediatek/mtk-mmsys.c +10 -0
+	if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
+		writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
+			       mmsys->regs + DISP_REG_CONFIG_OUT_SEL);
+	} else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
+		writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
+			       mmsys->regs + DISP_REG_CONFIG_OUT_SEL);
+		writel_relaxed(DSI_SEL_IN_RDMA,
+			       mmsys->regs + DISP_REG_CONFIG_DSI_SEL);
+		writel_relaxed(DPI_SEL_IN_BLS,
+			       mmsys->regs + DISP_REG_CONFIG_DPI_SEL);

you can still use the array approach for this like that:

         {
                 DDP_COMPONENT_BLS, DDP_COMPONENT_DSI0,
                 DISP_REG_CONFIG_OUT_SEL, BLS_TO_DSI_RDMA1_TO_DPI1
         }, {
         {
                 DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
                 DISP_REG_CONFIG_OUT_SEL, BLS_TO_DPI_RDMA1_TO_DSI
         }, {
         {
                 DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
                 DISP_REG_CONFIG_DSI_SEL, DSI_SEL_IN_RDMA
         }, {
         {
                 DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
                 DISP_REG_CONFIG_DPI_SEL, DPI_SEL_IN_BLS
         }, {
Quoted 2 lines

Thanks,
Dafna

these 4 blocks are already there (except your additional brackets) and it does not work with them (not added in 5.14)

91 static const struct mtk_mmsys_routes mmsys_default_routing_table[] = {
92 {
93 DDP_COMPONENT_BLS, DDP_COMPONENT_DSI0,
94 DISP_REG_CONFIG_OUT_SEL, BLS_TO_DSI_RDMA1_TO_DPI1 //(1)
95 }, {
96 DDP_COMPONENT_BLS, DDP_COMPONENT_DSI0,
97 DISP_REG_CONFIG_DSI_SEL, DSI_SEL_IN_BLS
98 }, {
99 DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
100 DISP_REG_CONFIG_OUT_SEL, BLS_TO_DPI_RDMA1_TO_DSI //(2)
101 }, {
102 DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
103 DISP_REG_CONFIG_DSI_SEL, DSI_SEL_IN_RDMA //(3)
104 }, {
105 DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
106 DISP_REG_CONFIG_DPI_SEL, DPI_SEL_IN_BLS //(4)
107 }, {

and code should apply all matching parts (2-4 for BLS=>dpi), but it does not work...maybe because new code reads and OR the new value instead of writing it (old code overwrites current register value with the new value without reading)

69 for (i = 0; i < mmsys->data->num_routes; i++)
70 if (cur == routes[i].from_comp && next == routes[i].to_comp) {
71 reg = readl_relaxed(mmsys->regs + routes[i].addr) | routes[i].val; //<<<<<<<<<<<
72 writel_relaxed(reg, mmsys->regs + routes[i].addr);
73 }

regards Frank

#3 lore raw
In reply to Dafna Hirschfeld

Hi,

it turns out that problem is the read+or of the new value

i reverted my patch and changed

reg = readl_relaxed(mmsys->regs + routes[i].addr) | routes[i].val; writel_relaxed(reg, mmsys->regs + routes[i].addr);

to

writel_relaxed(routes[i].val, mmsys->regs + routes[i].addr);

and it works too, but maybe it breaks other platforms

regards Frank

#4 lore raw

Hi,

Quoted 6 lines · Frank Wunderlich

Hi,

it turns out that problem is the read+or of the new value

i reverted my patch and changed

Quoted 8 lines

reg = readl_relaxed(mmsys->regs + routes[i].addr) | routes[i].val; writel_relaxed(reg, mmsys->regs + routes[i].addr);

to

writel_relaxed(routes[i].val, mmsys->regs + routes[i].addr);

and it works too, but maybe it breaks other platforms

Interesting, I can test if it fix that similar bug on mt8173 when resume from suspend.

Thanks,
Dafna

Quoted 3 lines

regards Frank

#5 lore raw
Quoted 7 lines · Frank Wunderlich
Quoted 6 lines

Hi,

it turns out that problem is the read+or of the new value

i reverted my patch and changed

Quoted 8 lines
Quoted 8 lines

reg = readl_relaxed(mmsys->regs + routes[i].addr) | routes[i].val; writel_relaxed(reg, mmsys->regs + routes[i].addr);

to

writel_relaxed(routes[i].val, mmsys->regs + routes[i].addr);

and it works too, but maybe it breaks other platforms

A gentle ping. Amy further comments which of both ways is the right one (restoring old output select function or write only without read+or)?

regards Frank

#6 lore raw

Hi, Frank:

Frank Wunderlich <[email protected]> 於 2021年7月25日 週日 上午1:06寫道：

Quoted 11 lines
Quoted 11 lines · Frank Wunderlich
Quoted 9 lines

Hi,

it turns out that problem is the read+or of the new value

i reverted my patch and changed

reg = readl_relaxed(mmsys->regs + routes[i].addr) | routes[i].val; writel_relaxed(reg, mmsys->regs + routes[i].addr);

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines

to

writel_relaxed(routes[i].val, mmsys->regs + routes[i].addr);

and it works too, but maybe it breaks other platforms

A gentle ping. Amy further comments which of both ways is the right one (restoring old output select function or write only without read+or)?

As I've discussed in [1], SOUT has many bits and need to be cleared before set new value. Of course, write only could do the clear, but for MOUT, it clear the bits that should not be cleared. If you want to use the table, you need to implement the 'mask'.

[1] https://patchwork.kernel.org/project/linux-mediatek/patch/trinity-937ebfa3-d123-42de-a289-3ad0dbc09782-1625830110576@3c-app-gmx-bap43/

Regards,
Chun-Kuang.

Quoted 2 lines

regards Frank

#7 lore raw

Hi

Quoted 8 lines

Gesendet: Montag, 26. Juli 2021 um 02:27 Uhr
Von: "Chun-Kuang Hu" <[email protected]>
As I've discussed in [1], SOUT has many bits and need to be cleared before set new value. Of course, write only could do the clear, but for MOUT, it clear the bits that should not be cleared. If you want to use the table, you need to implement the 'mask'.

[1] https://patchwork.kernel.org/project/linux-mediatek/patch/trinity-937ebfa3-d123-42de-a289-3ad0dbc09782-1625830110576@3c-app-gmx-bap43/

I've added the mask like you did here [1] on my repo [2]

and it works too...should i use you as author as i only take your code when posting?

regards Frank

[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2345186/5/drivers/soc/mediatek/mtk-mmsys.c#294
[2] https://github.com/frank-w/BPI-R2-4.14/commits/5.14-mmsys