Hi Matthias,

Thanks for the review.

On Fri, 2021-08-06 at 14:20 +0200, Matthias Brugger wrote:

On 28/07/2021 07:34, Nancy.Lin wrote:

quoted

Hi Enric,

Thanks for your review.

On Fri, 2021-07-23 at 13:05 +0200, Enric Balletbo Serra wrote:

quoted

Hi Nancy,

Thank you for your patch.

Missatge de Nancy.Lin [off-list ref] del dia dj., 22 de
jul.
2021 a les 11:45:

quoted

Add mt8195 vdosys1 clock driver name and routing table to
the driver data of mtk-mmsys.

Signed-off-by: Nancy.Lin <redacted>
---
 drivers/soc/mediatek/mt8195-mmsys.h    | 83
++++++++++++++++++++++++--
 drivers/soc/mediatek/mtk-mmsys.c       | 10 ++++
 include/linux/soc/mediatek/mtk-mmsys.h |  2 +
 3 files changed, 90 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/mediatek/mt8195-mmsys.h

b/drivers/soc/mediatek/mt8195-mmsys.h
index 73e9e8286d50..104ba575f765 100644

--- a/drivers/soc/mediatek/mt8195-mmsys.h
+++ b/drivers/soc/mediatek/mt8195-mmsys.h

@@ -64,16 +64,16 @@
 #define

SOUT_TO_VPP_MERGE0_P1_SEL                              (1
<< 0)

 #define
MT8195_VDO1_MERGE0_ASYNC_SOUT_SEL                      0xf40
-#define
SOUT_TO_HDR_VDO_FE0                                    (0
<< 0)

This definition was introduced in this patch [1] that didn't land
yet.
And you're removing it now. Could you sync with Jason and only
introduce the bits that are needed for your patches. Also all the
comments I made to the Jason's patch apply here.

[1] 

https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20210723090233.24007-3-jason-jh.lin@mediatek.com/__;!!CTRNKA9wMg0ARbw!0rDdPxfBPcZC9icK37sCxT55RMqwRngO0BF4-uDwgYZP7UwQkx7iidkINqLBb7yi$

quoted

quoted

OK, I will sync with Jason and modify it.

quoted

quoted

+#define
SOUT_TO_MIXER_IN1_SEL                                  (1
<< 0)

 #define
MT8195_VDO1_MERGE1_ASYNC_SOUT_SEL                      0xf44
-#define
SOUT_TO_HDR_VDO_FE1                                    (0
<< 0)
+#define
SOUT_TO_MIXER_IN2_SEL                                  (1
<< 0)

 #define
MT8195_VDO1_MERGE2_ASYNC_SOUT_SEL                      0xf48
-#define
SOUT_TO_HDR_GFX_FE0                                    (0
<< 0)
+#define
SOUT_TO_MIXER_IN3_SEL                                  (1
<< 0)

 #define
MT8195_VDO1_MERGE3_ASYNC_SOUT_SEL                      0xf4c
-#define
SOUT_TO_HDR_GFX_FE1                                    (0
<< 0)
+#define
SOUT_TO_MIXER_IN4_SEL                                  (1
<< 0)

 #define
MT8195_VDO1_MIXER_IN1_SOUT_SEL                         0xf58
 #define
MIXER_IN1_SOUT_TO_DISP_MIXER                           (0
<< 0)

@@ -88,7 +88,7 @@
 #define

MIXER_IN4_SOUT_TO_DISP_MIXER                           (0
<< 0)

 #define
MT8195_VDO1_MIXER_OUT_SOUT_SEL                         0xf34
-#define
MIXER_SOUT_TO_HDR_VDO_BE0                              (0
<< 0)
+#define
MIXER_SOUT_TO_MERGE4_ASYNC_SEL                         (1
<< 0)

 #define
MT8195_VDO1_MERGE4_SOUT_SEL                            0xf18
 #define
MERGE4_SOUT_TO_VDOSYS0                                 (0
<< 0)

@@ -185,6 +185,79 @@ static const struct mtk_mmsys_routes

mmsys_mt8195_routing_table[] = {
        }, {
                DDP_COMPONENT_DSC0, DDP_COMPONENT_MERGE0,
                MT8195_VDO0_SEL_OUT,
SOUT_DSC_WRAP0_OUT_TO_VPP_MERGE
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_VPP_MERGE0_P0_SEL_IN,
VPP_MERGE0_P0_SEL_IN_FROM_MDP_RDMA0
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_VPP_MERGE0_P1_SEL_IN,
VPP_MERGE0_P1_SEL_IN_FROM_MDP_RDMA1
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_VPP_MERGE1_P0_SEL_IN,
VPP_MERGE1_P0_SEL_IN_FROM_MDP_RDMA2
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MERGE0_ASYNC_SOUT_SEL,
SOUT_TO_MIXER_IN1_SEL
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MERGE1_ASYNC_SOUT_SEL,
SOUT_TO_MIXER_IN2_SEL
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MERGE2_ASYNC_SOUT_SEL,
SOUT_TO_MIXER_IN3_SEL
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MERGE3_ASYNC_SOUT_SEL,
SOUT_TO_MIXER_IN4_SEL
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MIXER_OUT_SOUT_SEL,
MIXER_SOUT_TO_MERGE4_ASYNC_SEL
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MIXER_IN1_SEL_IN,
MIXER_IN1_SEL_IN_FROM_MERGE0_ASYNC_SOUT
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MIXER_IN2_SEL_IN,
MIXER_IN2_SEL_IN_FROM_MERGE1_ASYNC_SOUT
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MIXER_IN3_SEL_IN,
MIXER_IN3_SEL_IN_FROM_MERGE2_ASYNC_SOUT
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MIXER_IN4_SEL_IN,
MIXER_IN4_SEL_IN_FROM_MERGE3_ASYNC_SOUT
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MIXER_SOUT_SEL_IN,
MIXER_SOUT_SEL_IN_FROM_DISP_MIXER
+       },
+       {
+               DDP_COMPONENT_PSEUDO_OVL, DDP_COMPONENT_MERGE5,
+               MT8195_VDO1_MERGE4_ASYNC_SEL_IN,
MERGE4_ASYNC_SEL_IN_FROM_MIXER_OUT_SOUT
+       },
+       {
+               DDP_COMPONENT_MERGE5, DDP_COMPONENT_DPI1,
+               MT8195_VDO1_DISP_DPI1_SEL_IN,
DISP_DPI1_SEL_IN_FROM_VPP_MERGE4_MOUT
+       },
+       {
+               DDP_COMPONENT_MERGE5, DDP_COMPONENT_DPI1,
+               MT8195_VDO1_MERGE4_SOUT_SEL,
MERGE4_SOUT_TO_DPI1_SEL
+       },
+       {
+               DDP_COMPONENT_MERGE5, DDP_COMPONENT_DP_INTF1,
+               MT8195_VDO1_DISP_DP_INTF0_SEL_IN,
+               DISP_DP_INTF0_SEL_IN_FROM_VPP_MERGE4_MOUT
+       },
+       {
+               DDP_COMPONENT_MERGE5, DDP_COMPONENT_DP_INTF1,
+               MT8195_VDO1_MERGE4_SOUT_SEL,
MERGE4_SOUT_TO_DP_INTF0_SEL
        }
 };

diff --git a/drivers/soc/mediatek/mtk-mmsys.c

b/drivers/soc/mediatek/mtk-mmsys.c
index 1fb241750897..9e31aad6c5c8 100644

--- a/drivers/soc/mediatek/mtk-mmsys.c
+++ b/drivers/soc/mediatek/mtk-mmsys.c

@@ -59,6 +59,12 @@ static const struct mtk_mmsys_driver_data

mt8195_vdosys0_driver_data = {
        .num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
 };

+static const struct mtk_mmsys_driver_data
mt8195_vdosys1_driver_data = {
+       .clk_driver = "clk-mt8195-vdo1",
+       .routes = mmsys_mt8195_routing_table,
+       .num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
+};
+
 struct mtk_mmsys {
        void __iomem *regs;
        const struct mtk_mmsys_driver_data *data;

@@ -168,6 +174,10 @@ static const struct of_device_id

of_match_mtk_mmsys[] = {
                .compatible = "mediatek,mt8195-vdosys0",
                .data = &mt8195_vdosys0_driver_data,
        },
+       {
+               .compatible = "mediatek,mt8195-vdosys1",

Why do you need a second compatible, isn't this the same IP
block? I
mean, I understand that you have 2 mmsys blocks, but both are the
same
IP block, right? or are they different?

Thanks,
  Enric

They(vdosys0 and vdosys1) are different IP block.

Please explain in what they are different. From what I see, you use
the same
routing table for both compatibles and only register another platform
device for
a second clock. Is that correct?

Regards,
Matthias

From a hardware point of view, the HW engines of vdosys0 and vdosys1
are different.
1. The components on meiatek-drm of vdosys0 are OVL0, RDMA0, COLOR0,
CCORR, AAL0, GAMMA, DITHER, DSC0, MERGE0, DP_INTF0. Its output panel is
eDP.
2. The components on meiatek-drm of vdosys1 are OVL_ADAPTOR, MERGE5,
DP_INTF1. Its output panel is DP.

In the SoC before, such as mt8173, it has 2 pipelines binding to one
mmsys with the same clock driver and the same power domain.

In mt8195, 2 pipelines are binding to different mmsys, such as vdosys0
and vdosys1. Each mmsys uses different clock drivers and different
power domains.

So I think it is more appropriate to use 2 compatibles to identify
which mmsys represents the pipeline. Another reason for using two
compatibles is that we use different driver
data in mtk-mmsys.c and mtk_drm_drv.c to identify the corresponding
mmsys is vdosys0 or vdosys1.

Their driver data in mtk_drm_drv.c is defined here:
[v7,13/13] drm/mediatek: add mediatek-drm of vdosys0 support for mt8195

https://patchwork.kernel.org/project/linux-mediatek/patch/20210815145610.2050-14-jason-jh.lin@mediatek.com/

[v2,14/14] drm/mediatek: add mediatek-drm of vdosys1 support for MT8195

https://patchwork.kernel.org/project/linux-mediatek/patch/20210722094551.15255-15-nancy.lin@mediatek.com/


There is another series talking about the two mmsys compatible.

https://patchwork.kernel.org/project/linux-mediatek/patch/20210722092624.14401-2-jason-jh.lin@mediatek.com/

Regards,
Nancy

quoted

quoted

quoted

+               .data = &mt8195_vdosys1_driver_data,
+       },
        { }
 };

diff --git a/include/linux/soc/mediatek/mtk-mmsys.h

b/include/linux/soc/mediatek/mtk-mmsys.h
index 34cb605e5df9..338c71570aeb 100644

--- a/include/linux/soc/mediatek/mtk-mmsys.h
+++ b/include/linux/soc/mediatek/mtk-mmsys.h

@@ -49,6 +49,8 @@ enum mtk_ddp_comp_id {
        DDP_COMPONENT_DSC1,
        DDP_COMPONENT_DSC1_VIRTUAL0,
        DDP_COMPONENT_DP_INTF0,
+       DDP_COMPONENT_DP_INTF1,
+       DDP_COMPONENT_PSEUDO_OVL,
        DDP_COMPONENT_ID_MAX,
 };

--

2.18.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel