On Thu, 2024-05-30 at 18:16 +0800, Chen-Yu Tsai wrote: > On Thu, May 30, 2024 at 5:59 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxx> > > > + * > > > + * Based on driver in downstream ChromeOS v5.15 kernel. > > > + * > > > + * Copyright (c) 2014 MediaTek Inc. > > > + * Author: Chiawen Lee <chiawen.lee@xxxxxxxxxxxx> > > > + */ > > > + > > > +#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. > > > > + > > > +/* 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. > > > > + > > > + 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. Upstream Mesa is still missing a lot of changes taking the driver up to Vulkan 1.0 on AXE-1-16M, which is the GPU we've mainly been focusing on. This work can be found in our Mesa repo on FDO GitLab [1]. Support for GX6250 (Series 6XT) and BXS-4-64 is currently incomplete (these are the other GPUs we've been adding support for [2]). The changes haven't made it upstream yet as we're in the process of reworking the compiler and compiler/driver interface side of things to address the inevitable comments we'd get as part of upstreaming. This work should be complete soon and will go a long way towards improving support for / making it easier to support more GPUs on the compiler side. In parallel to this, we've implemented the Vulkan extensions, optional features, etc needed by Zink [2] and we're currently fixing GLES conformance issues. Again, we've been focusing on AXE-1-16M. Once we've got GLES conformance passing we'll be switching our focus to completing support for the BXS-4-64. Thanks Frank [1] https://gitlab.freedesktop.org/imagination/mesa/-/tree/powervr-mesa-next [2] https://docs.mesa3d.org/drivers/powervr.html [3] https://gitlab.freedesktop.org/imagination/mesa/-/tree/dev/zink > > > 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. > > ChenYu