| 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: <0e5a5b36-6969-416d-9a16-6c71d7986c73@collabora.com>
Date: Thu, 30 May 2024 14:48:28 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Frank Binns <frank.binns@...tec.com>, Matt Coster
<matt.coster@...tec.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
dri-devel@...ts.freedesktop.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/6] clk: mediatek: Add mt8173-mfgtop driver
Il 30/05/24 12:16, Chen-Yu Tsai ha scritto:
> On Thu, May 30, 2024 at 5:59 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.com> wrote:
>>
>> Il 30/05/24 10:35, Chen-Yu Tsai ha scritto:
>>> The MFG (GPU) block on the MT8173 has a small glue layer, named MFG_TOP
>>> in the datasheet, that contains clock gates, some power sequence signal
>>> delays, and other unknown registers that get toggled when the GPU is
>>> powered on.
>>>
>>> The clock gates are exposed as clocks provided by a clock controller,
>>> while the power sequencing bits are exposed as one singular power domain.
>>>
>>> Signed-off-by: Chen-Yu Tsai <wenst@...omium.org>
>>> ---
>>> drivers/clk/mediatek/Kconfig | 9 +
>>> drivers/clk/mediatek/Makefile | 1 +
>>> drivers/clk/mediatek/clk-mt8173-mfgtop.c | 240 +++++++++++++++++++++++
>>> 3 files changed, 250 insertions(+)
>>> create mode 100644 drivers/clk/mediatek/clk-mt8173-mfgtop.c
>>>
>>> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
>>> index 70a005e7e1b1..9e279c739f1c 100644
>>> --- a/drivers/clk/mediatek/Kconfig
>>> +++ b/drivers/clk/mediatek/Kconfig
>>> @@ -500,6 +500,15 @@ config COMMON_CLK_MT8173_IMGSYS
>>> help
>>> This driver supports MediaTek MT8173 imgsys clocks.
>>>
>>> +config COMMON_CLK_MT8173_MFGTOP
>>> + tristate "Clock and power driver for MediaTek MT8173 mfgtop"
>>> + depends on COMMON_CLK_MT8173
>>> + default COMMON_CLK_MT8173
>>> + select PM_GENERIC_DOMAINS
>>> + select PM_GENERIC_DOMAINS_OF
>>> + help
>>> + This driver supports MediaTek MT8173 mfgtop clocks and power domain.
>>> +
>>> config COMMON_CLK_MT8173_MMSYS
>>> tristate "Clock driver for MediaTek MT8173 mmsys"
>>> depends on COMMON_CLK_MT8173
>>> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
>>> index eeccfa039896..fdd3a76e12a1 100644
>>> --- a/drivers/clk/mediatek/Makefile
>>> +++ b/drivers/clk/mediatek/Makefile
>>> @@ -77,6 +77,7 @@ obj-$(CONFIG_COMMON_CLK_MT8167_VDECSYS) += clk-mt8167-vdec.o
>>> obj-$(CONFIG_COMMON_CLK_MT8173) += clk-mt8173-apmixedsys.o clk-mt8173-infracfg.o \
>>> clk-mt8173-pericfg.o clk-mt8173-topckgen.o
>>> obj-$(CONFIG_COMMON_CLK_MT8173_IMGSYS) += clk-mt8173-img.o
>>> +obj-$(CONFIG_COMMON_CLK_MT8173_MFGTOP) += clk-mt8173-mfgtop.o
>>> obj-$(CONFIG_COMMON_CLK_MT8173_MMSYS) += clk-mt8173-mm.o
>>> obj-$(CONFIG_COMMON_CLK_MT8173_VDECSYS) += clk-mt8173-vdecsys.o
>>> obj-$(CONFIG_COMMON_CLK_MT8173_VENCSYS) += clk-mt8173-vencsys.o
>>> diff --git a/drivers/clk/mediatek/clk-mt8173-mfgtop.c b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
>>> new file mode 100644
>>> index 000000000000..85fa7a7453ed
>>> --- /dev/null
>>> +++ b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
>>> @@ -0,0 +1,240 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2024 Google LLC
>>> + * Author: Chen-Yu Tsai <wenst@...omium.org>
>>> + *
>>> + * Based on driver in downstream ChromeOS v5.15 kernel.
>>> + *
>>> + * Copyright (c) 2014 MediaTek Inc.
>>> + * Author: Chiawen Lee <chiawen.lee@...iatek.com>
>>> + */
>>> +
>>> +#include <dt-bindings/clock/mt8173-clk.h>
>>> +
>>> +#include <linux/bitfield.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_domain.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +#include "clk-gate.h"
>>> +#include "clk-mtk.h"
>>> +
>>> +static const struct mtk_gate_regs mfg_cg_regs = {
>>> + .sta_ofs = 0x0000,
>>> + .clr_ofs = 0x0008,
>>> + .set_ofs = 0x0004,
>>> +};
>>> +
>>> +#define GATE_MFG(_id, _name, _parent, _shift, _flags) \
>>> + GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, _shift, &mtk_clk_gate_ops_setclr, _flags)
>>
>> Extra tabulation: please fix
>
> One tab instead of two? OK.
>
Yeah.
>>> +
>>> +/* TODO: The block actually has dividers for the core and mem clocks. */
>>> +static const struct mtk_gate mfg_clks[] = {
>>> + GATE_MFG(CLK_MFG_AXI, "mfg_axi", "axi_mfg_in_sel", 0, CLK_SET_RATE_PARENT),
>>> + GATE_MFG(CLK_MFG_MEM, "mfg_mem", "mem_mfg_in_sel", 1, CLK_SET_RATE_PARENT),
>>> + GATE_MFG(CLK_MFG_G3D, "mfg_g3d", "mfg_sel", 2, CLK_SET_RATE_PARENT),
>>> + GATE_MFG(CLK_MFG_26M, "mfg_26m", "clk26m", 3, 0),
>>> +};
>>> +
>>> +static const struct mtk_clk_desc mfg_desc = {
>>> + .clks = mfg_clks,
>>> + .num_clks = ARRAY_SIZE(mfg_clks),
>>> +};
>>> +
>>> +struct mt8173_mfgtop_data {
>>> + struct clk_hw_onecell_data *clk_data;
>>> + struct regmap *regmap;
>>> + struct generic_pm_domain genpd;
>>> + struct of_phandle_args parent_pd, child_pd;
>>> + struct clk *clk_26m;
>>> +};
>>> +
>>> +static const struct of_device_id of_match_clk_mt8173_mfgtop[] = {
>>> + { .compatible = "mediatek,mt8173-mfgtop", .data = &mfg_desc },
>>> + { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_match_clk_mt8173_mfgtop);
>>
>> Please move of_match_clk_mt8173_mfgtop before clk_mt8173_mfgtop_drv for consistency
>> with all the other clock drivers.
>
> Ack.
>
>>> +
>>> +/* Delay count in clock cycles */
>>> +#define MFG_ACTIVE_POWER_CON0 0x24
>>> + #define RST_B_DELAY_CNT GENMASK(7, 0) /* pwr_rst_b de-assert delay during power-up */
>>> + #define CLK_EN_DELAY_CNT GENMASK(15, 8) /* CLK_DIS deassert delay during power-up */
>>> + #define CLK_DIS_DELAY_CNT GENMASK(23, 16) /* CLK_DIS assert delay during power-down */
>>
>> The reason why I had EVT_FORCE_ABORT and ACTIVE_PWRCTL_EN in my driver is to
>> document that we're keeping the event force abort disabled and, more importantly,
>> we are keeping the "active power control" feature disabled.
>>
>> Please, add those two - or at least the ACTIVE_PWRCTL_EN - to keep that documented,
>> or this information will be lost for sure.
>> If in the future the ACTIVE_PWRCTL feature will become usable, it's going to be
>> just a 30 seconds change, as the info is already there.
>
> OK.
>
>>> +
>>> +#define MFG_ACTIVE_POWER_CON1 0x28
>>> + #define PWR_ON_S_DELAY_CNT GENMASK(7, 0) /* pwr_on_s assert delay during power-up */
>>> + #define ISO_DELAY_CNT GENMASK(15, 8) /* ISO assert delay during power-down */
>>> + #define ISOOFF_DELAY_CNT GENMASK(23, 16) /* ISO de-assert delay during power-up */
>>> + #define RST__DELAY_CNT GENMASK(31, 24) /* pwr_rsb_b assert delay during power-down */
>>> +
>>> +static int clk_mt8173_mfgtop_power_on(struct generic_pm_domain *domain)
>>> +{
>>> + struct mt8173_mfgtop_data *data = container_of(domain, struct mt8173_mfgtop_data, genpd);
>>> +
>>> + /* drives internal power management */
>>> + clk_prepare_enable(data->clk_26m);
>>> +
>>> + /* Power on/off delays for various signals */
>>> + regmap_write(data->regmap, MFG_ACTIVE_POWER_CON0,
>>> + FIELD_PREP(RST_B_DELAY_CNT, 77) |
>>> + FIELD_PREP(CLK_EN_DELAY_CNT, 61) |
>>> + FIELD_PREP(CLK_DIS_DELAY_CNT, 60));
>>
>> I get that this is kinda odd to read, but still...
>>
>> FIELD_PREP(CLK_DIS_DELAY_CNT, 60) |
>> FIELD_PREP(ACTIVE_PWRCTL_EN, 0));
>>
>> ...please :-)
>
> Sure.
>
>>> + regmap_write(data->regmap, MFG_ACTIVE_POWER_CON1,
>>> + FIELD_PREP(PWR_ON_S_DELAY_CNT, 11) |
>>> + FIELD_PREP(ISO_DELAY_CNT, 68) |
>>> + FIELD_PREP(ISOOFF_DELAY_CNT, 69) |
>>> + FIELD_PREP(RST__DELAY_CNT, 77));
>>> +
>>> + /* Magic numbers related to core switch sequence and delays */
>>> + regmap_write(data->regmap, 0xe0, 0x7a710184);
>>> + regmap_write(data->regmap, 0xe4, 0x835f6856);
>>> + regmap_write(data->regmap, 0xe8, 0x002b0234);
>>> + regmap_write(data->regmap, 0xec, 0x80000000);
>>> + regmap_write(data->regmap, 0xa0, 0x08000000);
>>
>> Is there any way to retrieve information about what those registers are?
>
> I asked. They said the project was too long ago, and they could only
> figure out that it had something to do with core switch sequencing and
> delays between each core, which is what I put in the comment there.
>
That's a bit sad to read, but okay, I guess we'll call it a day and keep the
magic numbers around, as that's the only option. :-(
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int clk_mt8173_mfgtop_power_off(struct generic_pm_domain *domain)
>>> +{
>>> + struct mt8173_mfgtop_data *data = container_of(domain, struct mt8173_mfgtop_data, genpd);
>>> +
>>> + /* Magic numbers related to core switch sequence and delays */
>>> + regmap_write(data->regmap, 0xec, 0);
>>> +
>>> + /* drives internal power management */
>>> + clk_disable_unprepare(data->clk_26m);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int clk_mt8173_mfgtop_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct device_node *node = dev->of_node;
>>> + struct mt8173_mfgtop_data *data;
>>> + int ret;
>>> +
>>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> + if (!data)
>>> + return -ENOMEM;
>>> +
>>> + platform_set_drvdata(pdev, data);
>>> +
>>> + data->clk_data = mtk_devm_alloc_clk_data(dev, ARRAY_SIZE(mfg_clks));
>>> + if (!data->clk_data)
>>> + return -ENOMEM;
>>> +
>>> + /* MTK clock gates also uses regmap */
>>> + data->regmap = device_node_to_regmap(node);
>>> + if (IS_ERR(data->regmap))
>>> + return dev_err_probe(dev, PTR_ERR(data->regmap), "Failed to get regmap\n");
>>> +
>>> + data->child_pd.np = node;
>>> + data->child_pd.args_count = 0;
>>> + ret = of_parse_phandle_with_args(node, "power-domains", "#power-domain-cells", 0,
>>> + &data->parent_pd);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to parse power domain\n");
>>> +
>>> + devm_pm_runtime_enable(dev);
>>> + /*
>>> + * Do a pm_runtime_resume_and_get() to workaround a possible
>>> + * deadlock between clk_register() and the genpd framework.
>>> + */
>>> + ret = pm_runtime_resume_and_get(dev);
>>> + if (ret) {
>>> + dev_err_probe(dev, ret, "Failed to runtime resume device\n");
>>> + goto put_of_node;
>>> + }
>>> +
>>> + ret = mtk_clk_register_gates(dev, node, mfg_clks, ARRAY_SIZE(mfg_clks),
>>> + data->clk_data);
>>> + if (ret) {
>>> + dev_err_probe(dev, ret, "Failed to register clock gates\n");
>>> + goto put_pm_runtime;
>>> + }
>>> +
>>> + data->clk_26m = clk_hw_get_clk(data->clk_data->hws[CLK_MFG_26M], "26m");
>>> + if (IS_ERR(data->clk_26m)) {
>>> + dev_err_probe(dev, PTR_ERR(data->clk_26m), "Failed to get 26 MHz clock\n");
>>> + goto unregister_clks;
>>> + }
>>> +
>>> + ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, data->clk_data);
>>> + if (ret) {
>>> + dev_err_probe(dev, ret, "Failed to add clk OF provider\n");
>>> + goto put_26m_clk;
>>> + }
>>> +
>>> + data->genpd.name = "mfg_apm";
>>
>> "mfg-apm" or "mfg-pwr" please!
>
> Ack.
>
>> Everything else looks good.
>>
>> Thanks for taking care of that, I started this work way too much time ago and
>> realistically I wouldn't have been able to finish it due to time constraints.
>>
>> It's great to see that *finally* we can get some GPU upstream on this old SoC.
>> As its CPUs are really slow, LLVMPipe is quite unusable from a UX perspective
>> hence its only big issue was the lack of 3D HW acceleration.
>
> I think there's still more work on the GPU driver side. I was digging
> through the mailing list to find ways to get it running, and evidently
> it doesn't fully support zink yet.
>
There's still more work to do, yes, but it's still a great advancement.
>> This makes machines embedding this SoC usable, and that's simply awesome.
>
> I'll give the patches a week to simmer while I go work on some
> other stuff.
>
Sure, no worries.
Cheers!
Angelo
> ChenYu
Powered by blists - more mailing lists