On 24/2/20 6:52, CK Hu wrote:
>
> Hi,
>
> On Fri, 2020-02-21 at 18:10 +0100, Enric Balletbo i Serra wrote:
>> Hi,
>>
>> On 21/2/20 12:53, Matthias Brugger wrote:
>>>
>>>
>>> On 21/02/2020 12:37, Enric Balletbo i Serra wrote:
>>>> Hi CK and Matthias,
>>>>
>>>> On 21/2/20 12:11, CK Hu wrote:
>>>>> Hi, Matthias:
>>>>>
>>>>> On Fri, 2020-02-21 at 11:24 +0100, Matthias Brugger wrote:
>>>>>>
>>>>>> On 21/02/2020 10:27, CK Hu wrote:
>>>>>>> Hi, Enric:
>>>>>>>
>>>>>>> On Fri, 2020-02-21 at 09:56 +0100, Enric Balletbo i Serra wrote:
>>>>>>>> Hi CK,
>>>>>>>>
>>>>>>>> Thanks for your quick answer.
>>>>>>>>
>>>>>>>> On 21/2/20 5:39, CK Hu wrote:
>>>>>>>>> Hi, Enric:
>>>>>>>>>
>>>>>>>>> On Thu, 2020-02-20 at 18:21 +0100, Enric Balletbo i Serra wrote:
>>>>>>>>>> Dear all,
>>>>>>>>>>
>>>>>>>>>> Those patches are intended to solve an old standing issue on some
>>>>>>>>>> Mediatek devices (mt8173, mt2701 and mt2712) in a slightly different
>>>>>>>>>> way
>>>>>>>>>> to the precedent series.
>>>>>>>>>>
>>>>>>>>>> Up to now both drivers, clock and drm are probed with the same
>>>>>>>>>> device tree
>>>>>>>>>> compatible. But only the first driver get probed, which in effect
>>>>>>>>>> breaks
>>>>>>>>>> graphics on those devices.
>>>>>>>>>>
>>>>>>>>>> The version eight of the series tries to solve the problem with a
>>>>>>>>>> different approach than the previous series but similar to how is
>>>>>>>>>> solved
>>>>>>>>>> on other Mediatek devices.
>>>>>>>>>>
>>>>>>>>>> The MMSYS (Multimedia subsystem) in Mediatek SoCs has some registers
>>>>>>>>>> to
>>>>>>>>>> control clock gates (which is used in the clk driver) and some
>>>>>>>>>> registers
>>>>>>>>>> to set the routing and enable the differnet blocks of the display
>>>>>>>>>> and MDP (Media Data Path) subsystem. On this series the clk driver is
>>>>>>>>>> not a pure clock controller but a system controller that can provide
>>>>>>>>>> access to the shared registers between the different drivers that
>>>>>>>>>> need
>>>>>>>>>> it (mediatek-drm and mediatek-mdp). And the biggest change is, that
>>>>>>>>>> in
>>>>>>>>>> this version, clk driver is the entry point (parent) which will
>>>>>>>>>> trigger
>>>>>>>>>> the probe of the corresponding mediatek-drm driver and pass its MMSYS
>>>>>>>>>> platform data for display configuration.
>>>>>>>>>
>>>>>>>>> When mmsys is a system controller, I prefer to place mmsys in
>>>>>>>>> drivers/soc/mediatek, and it share registers for clock, display, and
>>>>>>>>> mdp
>>>>>>>>> driver. This means the probe function is placed in
>>>>>>>>> drivers/soc/mediatek ,its display clock function, mdp clock function
>>>>>>>>> are
>>>>>>>>> placed in drivers/clk, display routing are placed in drivers/gpu/drm,
>>>>>>>>> and mdp routing are placed in dirvers/video.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I understand what you mean but I am not sure this makes the code
>>>>>>>> clearer and
>>>>>>>> useful. The driver in drivers/soc/mediatek will be a simple dummy
>>>>>>>> implementation
>>>>>>>> of a "simple-mfd" device (a driver that simply matches with
>>>>>>>> "mediatek,mt8173-mmsys" and instantiates the "clk-mt8173-mm" and the
>>>>>>>> "mediatek-drm" driver (note that mediatek-mdp" is already instantiated
>>>>>>>> via
>>>>>>>> device-tree).
>>>>>>>>
>>>>>>>
>>>>>>> It's clear that mmsys is neither a pure clock controller nor a pure
>>>>>>> routing controller for display and mdp.
>>>>>>>
>>>>>>>> It'd be nice had a proper device-tree with a "simple-mfd" for mmsys
>>>>>>>> from the
>>>>>>>> beginning representing how really hardwware is, but I think that,
>>>>>>>> change this
>>>>>>>> now, will break backward compatibility.
>>>>>>>
>>>>>>> Maybe this is a solution. Current device tree would work only on old
>>>>>>> kernel version with a bug, so this mean there is no any device tree
>>>>>>> works on kernel version without bug. Why do we compatible with such
>>>>>>> device tree?
>>>>>>>
>>>>>>
>>>>
>>>> So the only reason why current DT worked at some point is because there
>>>> was a
>>>> kernel bug?
>>>>
>>>
>>> I'd say you can call it a kernel bug:
>>> https://patchwork.kernel.org/patch/10367877/#22078767
>>>
>>>
>>>> If that's the case I think we shouldn't worry about break DT compatibility
>>>> (I'm
>>>> sorry for those that having a buggy kernel makes display working)
>>>>
>>>>>> The idea behind this is, that the device-tree could be passed by some
>>>>>> boot
>>>>>> firmware, so that the OS do not care about it. For this we need a stable
>>>>>> DTS as
>>>>>> otherwise newer kernel with older FW would break.
>>>>>>
>>>>>> DTS is supposed to be just a different description of the HW like ACPI.
>>>>>> So it
>>>>>> has to be compatible (newer kernel with older DTS and if possible vice
>>>>>> versa).
>>>>>
>>>>> In my view, there is no FW (except some bug-inside FW) which works on
>>>>> this dts, so this dts is in a initial state. I think the compatibility
>>>>> is based on that dts correctly describe the HW. If we find this dts does
>>>>> not correctly describe the HW and it's in a initial state, should we
>>>>> still make it compatible?
>>>>>
>>>>
>>>> In this case I think we don't need to worry about buggy kernels, the only
>>>> thing
>>>> that we need to take in consideration is that mmsys is instantiated on
>>>> both (the
>>>> old DT and the new DT), we shouldn't expect display working (because never
>>>> worked, right?)
>>>>
>>>> With that in mind, I agree that is a good opportunity to fix properly the
>>>> HW
>>>> topology.
>>>>
>>>> What thing that worries me is that I see this pattern on all Mediatek
>>>> SoCs, does
>>>> this mean that display was never well supported for Mediatek SoCs?
>>>>
>>>
>>> That's exactly the case. Actually there were some patches for mt762x (can't
>>> remember if mt7623 or mt7622) whcih got pushed back, because we would need
>>> to
>>> fix the mmsys problem first.
>>>
>>> Ok, so if we come to the conclusion that this actually only worked because
>>> of a
>>> bug, then we can remodel the whole thing.
>>> In this case, I think the best would be to do what CK proposed:
>>> https://patchwork.kernel.org/patch/11381201/#23158169
>>>
>>> Making everything below 0x14000000 a subdevice of mmsys (mdp, ovl, rdma, you
>>> name it).
>>>
>>
>>
>> I see the MMSYS space as config registers to route the different ports in the
>> drm and video subsystem, so, as phandle of the display (drivers/gpur/drm) and
>> video (drivers/video) subsystem. What about something like this?
>>
>> mmsys: syscon@14000000 {
>> compatible = "mediatek,mt8173-mmsys", "syscon";
>> reg = <0 0x14000000 0 0x1000>;
>> power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>> assigned-clocks = <&topckgen CLK_TOP_MM_SEL>;
>> assigned-clock-rates = <400000000>;
>> #clock-cells = <1>;
>> };
>>
>> Basically is what we have with
>>
>> * [PATCH v8 4/6] clk: mediatek: mt8173: Switch MMSYS to platform driver
>>
>> And
>>
>> display-subsystem {
>> compatible = "mediatek,display-subsystem";
>> mediatek,mmsys = <&mmsys>; /* phandle to the routing config registers */
>> ports = <&ovl0>, <&ovl1>, < ... >
>> }
>>
>> We are introducing a new compatible that is not describing hardware but how
>> hardware is grouped, which I think is fine.
>>
>> The mediatek-drm driver will need a bit of rework but not much I guess.
>>
>> What is still not clear to me is the mdp part because doesn't seem to have an
>> entry point like the display part, instead it uses one port as entry point.
>>
>> mdp_rdma0: rdma@14001000 {
>> compatible = "mediatek,mt8173-mdp-rdma",
>> "mediatek,mt8173-mdp";
>>
>> AFAICS that driver is not touching MMSYS config registers to route the mdp
>> path,
>> only is getting the clocks, but I assume it will do in the future. In such
>> case,
>> maybe we could do something similar to the display-subsystem node?
>>
>
> Your proposal is to probe clock driver by mmsys device and probe display
> driver by another device. You have two choice to probe display driver: one is
> to create a virtual device that group display devices and probe display
> driver by this device. Another one is to choose one display device, such as
> OVL, to probe display driver.
>
> I do not like a virtual device solution. In some Mediatek SoC, the routing is
> so flexible that one function block could be placed in display pipe or mdp
> pipe by different routing.
>
> mmsys device control the display routing and display clock. OVL control.
> display overlay function. Both devices control partial display function, so
> probing display driver by which one looks the same for me.
>
> I prefer to probe display driver by mmsys device because it has more
> information of display pipe line and OVL just focus on overlay function. Only
> when it's difficult to probe display driver by mmsys device, we have to probe
> display driver by OVL.
>
> I think mmsys is really a multi-function device, and the device tree
> description may look like:
>
> mmsys: system-controller@14000000 {
> compatible = "mediatek,mt8173-mmsys", "syscon", "simple-mfd";
> reg = <0 0x14000000 0 0x1000>;
> power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> assigned-clocks = <&topckgen CLK_TOP_MM_SEL>;
> assigned-clock-rates = <400000000>;
> #clock-cells = <1>;
>
> route {
> compatible = "mediatek,mt8173-mmsys-route";
Are you suggesting move the mediatek-drm routing to a new mt8173-mmsys-route
driver? Or this is a more generic routing device? Is this routing device already
implemented somewhere?
> };
>
> clock {
> compatible = "mediatek,mt8173-mmsys-clk";
> };
> };
>
> But from mt6797 register map [1], mmsys have many function such as fake
> engine, memory delay, reset,....
> Should we break each function into a sub device?
> Or we do not break any function (include clock and routing) into sub device?
> Or just break these two function into device, remove "simple-mfd" from mmsys,
> so the rest control is in mmsys top device?
>
How do you see move mmsys to drivers/soc/mediatek and instantiate the clk and
mediatek-drm driver
mmsys: syscon@14000000 {
compatible = "mediatek,mt8173-mmsys", "syscon", "simple-mfd";
reg = <0 0x14000000 0 0x1000>;
power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
clock-controller {
compatible = "mediatek,clk-mt8173-mm"
assigned-clocks = <&topckgen CLK_TOP_MM_SEL>;
assigned-clock-rates = <400000000>;
#clock-cells = <1>;
};
display-subsystem {
compatible = "mediatek,display-subsystem";
};
};
Regards,
Enric
> [1]
> https://www.96boards.org/documentation/consumer/mediatekx20/additional-docs/docs/MT6797_Register_Table_Part_2.pdf
>
> Regards,
> CK
>
>
>> Regards,
>> Enric
>>
>>
>>> Regards,
>>> Matthias
>>>
>>>> Thanks.
>>>>
>>>>> If you have better solution, just let's forget this.
>>>>>
>>>>> Regards,
>>>>> CK
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Matthias
>>>>>>
>>>>>>> Regards,
>>>>>>> CK
>>>>>>>
>>>>>>>>
>>>>>>>> IMHO I think that considering the clk driver as entry point is fine,
>>>>>>>> but this is
>>>>>>>> something that the clock maintainers should decide.
>>>>>>>>
>>>>>>>> Also note that this is not only a MT8173 problem I am seeing the same
>>>>>>>> problem on
>>>>>>>> all other Mediatek SoCs.
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> CK
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> All this series was tested on the Acer R13 Chromebook only.
>>>>>>>>>>
>>>>>>>>>> For reference, here are the links to the old discussions:
>>>>>>>>>>
>>>>>>>>>> * v7:
>>>>>>>>>> https://patchwork.kernel.org/project/linux-mediatek/list/?series=241217
>>>>>>>>>> * v6:
>>>>>>>>>> https://patchwork.kernel.org/project/linux-mediatek/list/?series=213219
>>>>>>>>>> * v5:
>>>>>>>>>> https://patchwork.kernel.org/project/linux-mediatek/list/?series=44063
>>>>>>>>>> * v4:
>>>>>>>>>> * https://patchwork.kernel.org/patch/10530871/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10530883/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10530885/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10530911/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10530913/
>>>>>>>>>> * v3:
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367857/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367861/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367877/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367875/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367885/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367883/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367889/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367907/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367909/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10367905/
>>>>>>>>>> * v2: No relevant discussion, see v3
>>>>>>>>>> * v1:
>>>>>>>>>> * https://patchwork.kernel.org/patch/10016497/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10016499/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10016505/
>>>>>>>>>> * https://patchwork.kernel.org/patch/10016507/
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> Enric
>>>>>>>>>>
>>>>>>>>>> Changes in v8:
>>>>>>>>>> - Be a builtin_platform_driver like other mediatek mmsys drivers.
>>>>>>>>>> - New patches introduced in this series.
>>>>>>>>>>
>>>>>>>>>> Changes in v7:
>>>>>>>>>> - Add R-by from CK
>>>>>>>>>> - Add R-by from CK
>>>>>>>>>> - Fix check of return value of of_clk_get
>>>>>>>>>> - Fix identation
>>>>>>>>>> - Free clk_data->clks as well
>>>>>>>>>> - Get rid of private data structure
>>>>>>>>>>
>>>>>>>>>> Enric Balletbo i Serra (2):
>>>>>>>>>> drm/mediatek: Move MMSYS configuration to
>>>>>>>>>> include/linux/platform_data
>>>>>>>>>> clk/drm: mediatek: Fix mediatek-drm device probing
>>>>>>>>>>
>>>>>>>>>> Matthias Brugger (4):
>>>>>>>>>> drm/mediatek: Use regmap for register access
>>>>>>>>>> drm/mediatek: Omit warning on probe defers
>>>>>>>>>> media: mtk-mdp: Check return value of of_clk_get
>>>>>>>>>> clk: mediatek: mt8173: Switch MMSYS to platform driver
>>>>>>>>>>
>>>>>>>>>> drivers/clk/mediatek/Kconfig | 6 +
>>>>>>>>>> drivers/clk/mediatek/Makefile | 1 +
>>>>>>>>>> drivers/clk/mediatek/clk-mt2701-mm.c | 30 +++
>>>>>>>>>> drivers/clk/mediatek/clk-mt2712-mm.c | 44 +++++
>>>>>>>>>> drivers/clk/mediatek/clk-mt8173-mm.c | 172
>>>>>>>>>> ++++++++++++++++++
>>>>>>>>>> drivers/clk/mediatek/clk-mt8173.c | 104 -----------
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_disp_color.c | 5 +-
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 5 +-
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 5 +-
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_dpi.c | 12 +-
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 4 +-
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 53 +++---
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_drm_ddp.h | 4 +-
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 56 +-----
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 113 +-----------
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 13 +-
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_dsi.c | 8 +-
>>>>>>>>>> drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 +-
>>>>>>>>>> drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 6 +
>>>>>>>>>> include/linux/platform_data/mtk_mmsys.h | 73 ++++++++
>>>>>>>>>> 20 files changed, 401 insertions(+), 317 deletions(-)
>>>>>>>>>> create mode 100644 drivers/clk/mediatek/clk-mt8173-mm.c
>>>>>>>>>> create mode 100644 include/linux/platform_data/mtk_mmsys.h
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Linux-mediatek mailing list
>>>>>>>> [email protected]
>>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Linux-mediatek mailing list
>>>>>> [email protected]
>>>>>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>>>>>
>>>
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
>
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
