| 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: <CAGXv+5GFaudGqm4C9CY-_spiXcyWk7OvWHTdkehsyrV4sO1Ndw@mail.gmail.com>
Date: Fri, 5 Sep 2025 16:53:29 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
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
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.
ChenYu
> > + .ops = &mtk_clk_gate_ops_setclr, \
> > + }
> > +
> > +#define GATE_MDP1(_id, _name, _parent, _shift) { \
> > + .id = _id, \
> > + .name = _name, \
> > + .parent_name = _parent, \
> > + .regs = &mdp1_cg_regs, \
> > + .shift = _shift, \
> > + .ops = &mtk_clk_gate_ops_setclr, \
> > + }
> > +
> > +#define GATE_MDP2(_id, _name, _parent, _shift) { \
> > + .id = _id, \
> > + .name = _name, \
> > + .parent_name = _parent, \
> > + .regs = &mdp2_cg_regs, \
> > + .shift = _shift, \
> > + .flags = CLK_OPS_PARENT_ENABLE, \
> > + .ops = &mtk_clk_gate_ops_setclr, \
> > + }
> > +
>
> ..snip..
>
> > +
> > +static const struct mtk_clk_desc mdp_mcd = {
> > + .clks = mdp_clks,
> > + .num_clks = ARRAY_SIZE(mdp_clks),
> > + .need_runtime_pm = true,
> > +};
> > +
> > +static const struct of_device_id of_match_clk_mt8196_mdpsys[] = {
> > + { .compatible = "mediatek,mt8196-mdpsys1", .data = &mdp1_mcd },
> > + { .compatible = "mediatek,mt8196-mdpsys0", .data = &mdp_mcd },
>
> 0 comes before 1, swap those entries please.
>
> After applying the proposed fixes
>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mdpsys);
> > +
> > +static struct platform_driver clk_mt8196_mdpsys_drv = {
> > + .probe = mtk_clk_simple_probe,
> > + .remove = mtk_clk_simple_remove,
> > + .driver = {
> > + .name = "clk-mt8196-mdpsys",
> > + .of_match_table = of_match_clk_mt8196_mdpsys,
> > + },
> > +};
> > +module_platform_driver(clk_mt8196_mdpsys_drv);
> > +
> > +MODULE_DESCRIPTION("MediaTek MT8196 Multimedia Data Path clocks driver");
> > +MODULE_LICENSE("GPL");
>
Powered by blists - more mailing lists