Re: [PATCH v5 19/27] clk: mediatek: Add MT8196 mdpsys clock support

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 05/09/25 10:53, Chen-Yu Tsai ha scritto:
> On Fri, Sep 5, 2025 at 4:39 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 29/08/25 11:19, Laura Nao ha scritto:
>>> Add support for the MT8196 mdpsys clock controller, which provides clock
>>> gate control for MDP.
>>>
>>> Signed-off-by: Laura Nao <laura.nao@collabora.com>
>>> ---
>>>    drivers/clk/mediatek/Kconfig             |   7 +
>>>    drivers/clk/mediatek/Makefile            |   1 +
>>>    drivers/clk/mediatek/clk-mt8196-mdpsys.c | 186 +++++++++++++++++++++++
>>>    3 files changed, 194 insertions(+)
>>>    create mode 100644 drivers/clk/mediatek/clk-mt8196-mdpsys.c
>>>
>>> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
>>> index 8e5cdae80748..68ac08cf8e82 100644
>>> --- a/drivers/clk/mediatek/Kconfig
>>> +++ b/drivers/clk/mediatek/Kconfig
>>> @@ -1024,6 +1024,13 @@ config COMMON_CLK_MT8196_MCUSYS
>>>        help
>>>          This driver supports MediaTek MT8196 mcusys clocks.
>>>
>>> +config COMMON_CLK_MT8196_MDPSYS
>>> +     tristate "Clock driver for MediaTek MT8196 mdpsys"
>>> +     depends on COMMON_CLK_MT8196
>>> +     default COMMON_CLK_MT8196
>>> +     help
>>> +       This driver supports MediaTek MT8196 mdpsys clocks.
>>> +
>>>    config COMMON_CLK_MT8196_PEXTPSYS
>>>        tristate "Clock driver for MediaTek MT8196 pextpsys"
>>>        depends on COMMON_CLK_MT8196
>>> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
>>> index 46358623c3e5..d2d8bc43e45b 100644
>>> --- a/drivers/clk/mediatek/Makefile
>>> +++ b/drivers/clk/mediatek/Makefile
>>> @@ -155,6 +155,7 @@ obj-$(CONFIG_COMMON_CLK_MT8196) += clk-mt8196-apmixedsys.o clk-mt8196-topckgen.o
>>>                                   clk-mt8196-peri_ao.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8196_IMP_IIC_WRAP) += clk-mt8196-imp_iic_wrap.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8196_MCUSYS) += clk-mt8196-mcu.o
>>> +obj-$(CONFIG_COMMON_CLK_MT8196_MDPSYS) += clk-mt8196-mdpsys.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8196_PEXTPSYS) += clk-mt8196-pextp.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8196_UFSSYS) += clk-mt8196-ufs_ao.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-mt8365.o
>>> diff --git a/drivers/clk/mediatek/clk-mt8196-mdpsys.c b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
>>> new file mode 100644
>>> index 000000000000..a46b1627f1f3
>>> --- /dev/null
>>> +++ b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
>>> @@ -0,0 +1,186 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2025 MediaTek Inc.
>>> + *                    Guangjie Song <guangjie.song@mediatek.com>
>>> + * Copyright (c) 2025 Collabora Ltd.
>>> + *                    Laura Nao <laura.nao@collabora.com>
>>> + */
>>> +#include <dt-bindings/clock/mediatek,mt8196-clock.h>
>>> +
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#include "clk-gate.h"
>>> +#include "clk-mtk.h"
>>> +
>>> +static const struct mtk_gate_regs mdp0_cg_regs = {
>>> +     .set_ofs = 0x104,
>>> +     .clr_ofs = 0x108,
>>> +     .sta_ofs = 0x100,
>>> +};
>>> +
>>> +static const struct mtk_gate_regs mdp1_cg_regs = {
>>> +     .set_ofs = 0x114,
>>> +     .clr_ofs = 0x118,
>>> +     .sta_ofs = 0x110,
>>> +};
>>> +
>>> +static const struct mtk_gate_regs mdp2_cg_regs = {
>>> +     .set_ofs = 0x124,
>>> +     .clr_ofs = 0x128,
>>> +     .sta_ofs = 0x120,
>>> +};
>>> +
>>> +#define GATE_MDP0(_id, _name, _parent, _shift) {     \
>>> +             .id = _id,                              \
>>> +             .name = _name,                          \
>>> +             .parent_name = _parent,                 \
>>> +             .regs = &mdp0_cg_regs,                  \
>>> +             .shift = _shift,                        \
>>> +             .flags = CLK_OPS_PARENT_ENABLE,         \
>>
>> Why would MDP0 and MDP2 be different, as in why would MDP1 be so special to not
>> need CLK_OPS_PARENT_ENABLE while the others do?
>>
>> Either they all do, or they all don't.
>>
>> I guess they all don't, but I'm not sure how you tested that at all, since the
>> only way to test this is downstream (and upstream will very likely be different
>> from that).
>>
>> Even though I think they don't need that - please add back CLK_OPS_PARENT_ENABLE
>> to GATE_MDP1 to be safe, as in (all) MediaTek SoCs the multimedia subsystem is
>> kinda separate from the rest.
> 
> That kind of doesn't fly since the parent of mdp_f26m is clk26m, not the
> mdp clock. So either this block doesn't need a clock for register access
> or this clock is going to be broken.
> 
> This is why I raised the question about the validity of the flag in the
> first place.
> 
>> +       GATE_MDP1(CLK_MDP_F26M, "mdp_f26m", "clk26m", 27),
> 
>> Once MT8196 MDP support is upstreamed, we will be able to run a number of tests
>> to evaluate whether this flag is really needed or not.
>>
>> After all, if it turns out we can remove it, it's going to be a 3 lines patch,
>> not a big deal.
> 
> That also works. Though IMO it makes the error harder to notice.
> 

Okay, I understand your point.

Let's keep CLK_OPS_PARENT_ENABLE out of MDP1.

Cheers,
Angelo