| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <db803bd3-4e7d-4aa2-bdd1-6e0b14812c03@collabora.com>
Date: Mon, 15 Sep 2025 12:33:02 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Laura Nao <laura.nao@...labora.com>, mturquette@...libre.com,
sboyd@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
matthias.bgg@...il.com, p.zabel@...gutronix.de, richardcochran@...il.com,
guangjie.song@...iatek.com, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
netdev@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH v5 19/27] clk: mediatek: Add MT8196 mdpsys clock support
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@...labora.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@...labora.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@...iatek.com>
>>> + * Copyright (c) 2025 Collabora Ltd.
>>> + * Laura Nao <laura.nao@...labora.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
Powered by blists - more mailing lists