Hi Enric, Thanks for your review. On Wed, 2021-07-28 at 12:56 +0200, Enric Balletbo Serra wrote: > Hi Jason, > > Missatge de Jason-JH Lin <jason-jh.lin@xxxxxxxxxxxx> del dia dt., 27 > de jul. 2021 a les 4:53: > > > > Hi Enric, > > > > On Mon, 2021-07-26 at 12:08 +0200, Enric Balletbo Serra wrote: > > > Hi Jason, > > > > > > Missatge de Jason-JH Lin <jason-jh.lin@xxxxxxxxxxxx> del dia dl., > > > 26 > > > de jul. 2021 a les 9:02: > > > > > > > > On Fri, 2021-07-23 at 13:13 +0200, Enric Balletbo Serra wrote: > > > > > Hi Jason, > > > > > > > > > > Thank you for your patch. > > > > > > > > > > Missatge de jason-jh.lin <jason-jh.lin@xxxxxxxxxxxx> del dia > > > > > dj., > > > > > 22 > > > > > de jul. 2021 a les 11:26: > > > > > > > > > > > > There are 2 display hardware path in mt8195, namely vdosys0 > > > > > > and > > > > > > vdosys1, so add their definition in mtk-mmsys > > > > > > documentation. > > > > > > > > > > > > > > > > Just having 2 display hardware paths is not a reason to have > > > > > two > > > > > compatibles, isn't the IP block the same? Why do you need to > > > > > introduce > > > > > the two compatibles? > > > > > > > > > > Thanks, > > > > > Enric > > > > > > > > > > > > > Hi Enric, > > > > > > > > Thanks for reviewing my patch. > > > > > > > > The reason for using two compatibles is that vdosys0 and > > > > vdosys1 > > > > are > > > > different IP blocks. > > > > > > > > > > With that there are different IP blocks, what do you mean? Do you > > > mean > > > that there are two completely different blocks with completely > > > different functionalities? > > > > > > Or that there is the same IP block twice? I mean, of course, the > > > registers are different but has exactly the same functionality. > > > > > > > They are not the same IP block twice. > > Although both vdosys0 and vdosys1 will probe meiatek-drm driver, > > but > > the components on their hardware path are different and their > > output > > panel are also different. > > > > > > Because mmsys provides clock control, other display function > > > > blocks > > > > may > > > > use them as clock provider. > > > > > > > > E.g. > > > > 1. mmsys with compatible="mediatek,mt8195-vdosys0" > > > > [v4,1/6] arm64: dts: mt8195: add display node for vdosys0 > > > > > > > > > > > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20210723090233.24007-2-jason-jh.lin@xxxxxxxxxxxx/__;!!CTRNKA9wMg0ARbw!xHjKwv34W7ETFcmQPXRViylF2LbV7C7pE8OeJeNvA93jDdzr_ZBRRm8aIUCvAHSD_qGo$ > > > > > > > > > > > > ovl0: disp_ovl@1c000000 { > > > > ... > > > > clocks = <&vdosys0 CLK_VDO0_DISP_OVL0>; > > > > ... > > > > }; > > > > > > > > 2. mmsys with compatible="mediatek,mt8195-vdosys1" > > > > [v2,06/14] arm64: dts: mt8195: add display node for vdosys1 > > > > > > > > > > > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20210722094551.15255-7-nancy.lin@xxxxxxxxxxxx/__;!!CTRNKA9wMg0ARbw!xHjKwv34W7ETFcmQPXRViylF2LbV7C7pE8OeJeNvA93jDdzr_ZBRRm8aIUCvAP0FOfkc$ > > > > > > > > > > > > vdo1_rdma0: vdo1_rdma@1c104000 { > > > > ... > > > > clocks = <&vdosys1 CLK_VDO1_MDP_RDMA0>; > > > > ... > > > > }; > > > > > > > > > > Note that I am talking without knowing the hardware in detail, > > > but I > > > am wondering why I can't have something like this, where every > > > mmsys > > > is a clock and reset controller provider. > > > > > > vdosys0: syscon@14000000 { > > > compatible = "mediatek,mt8195-mmsys", "syscon"; > > > reg = <0 0x14000000 0 0x1000>; > > > #clock-cells = <1>; > > > #reset-cells = <1>; > > > }; > > > > > > vdosys1: syscon@15000000 { > > > compatible = "mediatek,mt8195-mmsys", "syscon"; > > > reg = <0 0x15000000 0 0x1000>; > > > #clock-cells = <1>; > > > #reset-cells = <1>; > > > }; > > > > > > ovl0: disp_ovl@1c000000 { > > > ... > > > clocks = <&vdosys0 CLK_VDO0_DISP_OVL0>; > > > ... > > > }; > > > > > > vdo1_rdma0: vdo1_rdma@1c104000 { > > > ... > > > clocks = <&vdosys1 CLK_VDO1_MDP_RDMA0>; > > > ... > > > }; > > > > > > What are the differences between vdosys0 and vdosys1 from a > > > hardware > > > point of view? > > > > > > Cheers, > > > Enric > > > > > > > Regards, > > > > Jason-JH.Lin > > > > > > > > From a hardware point of view, the components and the ouptut panel > > of > > vdosys0 and vdosys1: > > 1. The components on meiatek-drm of vdosys0 are OVL0, RDMA0, > > COLOR0, > > CCORR, AAL0, GAMMA, DITHER, DSC0, MERGE0, DP_INTF0 and its output > > panel > > is eDP. > > > > 2. The components on meiatek-drm of vdosys1 are PSEUDO_OVL, MERGE5, > > DP_INTF1 and its ouptut panel is DP. > > > > > > The resaon for using two compatibales 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. > > > > So IIUC the two mmsys block ares basically the same, they provide the > same, a reset controller, a clock controller and being able to write > to the routing registers, and what you only need to identify is which > one is is vdosys0 and which one is vdosys1 to apply different routing > tables? Can these routing tables change at runtime? Or are they > hardware fixed? > > Regards, > Enric > In the case of vdosys1 DP_TX switch to HDMI_TX, routing tables will change at runtime. Refer to mt8173.dtsi: https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8173.dtsi I forgot to put the power domain property in vdosys0 dts node. So I'll add this into DRM vdosys0 series at the next version: vdosys0: syscon@1c01a000 { compatible = "mediatek,mt8195-vdosys0", "syscon"; reg = <0 0x1c01a000 0 0x1000>; power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS0>; ... }; I'll also tell Nancy to add this: vdosys1: syscon@1c100000 { compatible = "mediatek,mt8195-vdosys1", "syscon"; reg = <0 0x1c100000 0 0x1000>; power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS1>; ... }; I'll also add the power-domian description into this yaml. power-domains: description: A phandle and PM domain specifier as defined by bindings of the power controller specified by phandle. See Documentation/devicetree/bindings/power/power-domain.yaml for details. 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 domain. So I think it is more appropriate to use 2 compatibles to identify which mmsys represents the pipeline. Regards, Jason-JH.Lin > > Their driver data in mtk_drm_drv.c is defined here: > > [v4,4/6] drm/mediatek: add mediatek-drm of vdosys0 support for > > mt8195 https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20210723090233.24007-5-jason-jh.lin@xxxxxxxxxxxx/ > > > > [v2,14/14] drm/mediatek: add mediatek-drm of vdosys1 support for > > MT8195 https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20210722094551.15255-15-nancy.lin@xxxxxxxxxxxx > > > > > > I think using the same compatible is unable to do this. Or do you > > have > > other suggestions to do this with the same compatibe? > > > > Regards, > > Jason-JH.Lin > > > > > > > > Signed-off-by: jason-jh.lin <jason-jh.lin@xxxxxxxxxxxx> > > > > > > --- > > > > > > this patch is base on [1][2] > > > > > > > > > > > > [1] dt-bindings: arm: mediatek: mmsys: convert to YAML > > > > > > format > > > > > > - > > > > > > > > > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20210519161847.3747352-1-fparent@xxxxxxxxxxxx/ > > > > > > > > > > > > [2] dt-bindings: arm: mediatek: mmsys: add MT8365 SoC > > > > > > binding > > > > > > - > > > > > > > > > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20210519161847.3747352-2-fparent@xxxxxxxxxxxx/ > > > > > > > > > > > > --- > > > > > > .../devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml > > > > > > | > > > > > > 2 ++ > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > diff --git > > > > > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,m > > > > > > msys > > > > > > .yam > > > > > > l > > > > > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,m > > > > > > msys > > > > > > .yam > > > > > > l > > > > > > index 2d4ff0ce387b..0789a9614f12 100644 > > > > > > --- > > > > > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,m > > > > > > msys > > > > > > .yam > > > > > > l > > > > > > +++ > > > > > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,m > > > > > > msys > > > > > > .yam > > > > > > l > > > > > > @@ -30,6 +30,8 @@ properties: > > > > > > - mediatek,mt8173-mmsys > > > > > > - mediatek,mt8183-mmsys > > > > > > - mediatek,mt8365-mmsys > > > > > > + - mediatek,mt8195-vdosys0 > > > > > > + - mediatek,mt8195-vdosys1 > > > > > > - const: syscon > > > > > > - items: > > > > > > - const: mediatek,mt7623-mmsys > > > > > > -- > > > > > > 2.18.0 > > > > > > > > > > > > > > -- > > > > -- > > Jason-JH Lin <jason-jh.lin@xxxxxxxxxxxx> -- Jason-JH Lin <jason-jh.lin@xxxxxxxxxxxx>
