by AngeloGioacchino Del Regno

[permalink] [raw]

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
> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>> + *
>>> + * Based on driver in downstream ChromeOS v5.15 kernel.
>>> + *
>>> + * Copyright (c) 2014 MediaTek Inc.
>>> + * Author: Chiawen Lee <[email protected]>
>>> + */
>>> +
>>> +#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

2024-05-30 15:39:00

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH 3/6] dt-bindings: gpu: powervr-rogue: Add MediaTek MT8173 GPU

On Thu, May 30, 2024 at 04:35:02PM +0800, Chen-Yu Tsai wrote:
> The MediaTek MT8173 comes with a PowerVR Rogue GX6250, which is one
> of the Series6XT GPUs, another sub-family of the Rogue family.
>
> This was part of the very first few versions of the PowerVR submission,
> but was later dropped. The compatible string has been updated to follow
> the new naming scheme adopted for the AXE series.
>
> In a previous iteration of the PowerVR binding submission [1], the
> number of clocks required for the 6XT family was mentioned to be
> always 3. This is also reflected here.
>
> [1] https://lore.kernel.org/dri-devel/[email protected]/
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>

Reviewed-by: Conor Dooley <[email protected]>

Thanks,
Conor.

Attachments:

(No filename) (828.00 B)
signature.asc (235.00 B)
Download all attachments

2024-05-30 16:20:13

by kernel test robot

[permalink] [raw]

On Thu, Jun 13, 2024 at 4:10 AM Chen-Yu Tsai <[email protected]> wrote:
>
> On Tue, Jun 4, 2024 at 12:18 PM Chen-Yu Tsai <[email protected]> wrote:
> >
> > On Fri, May 31, 2024 at 9:37 PM Frank Binns <[email protected]> wrote:
> > >
> > > Hi ChenYu,
> > >
> > > On Thu, 2024-05-30 at 16:35 +0800, Chen-Yu Tsai wrote:
> > > > The MediaTek MT8173 comes with a PowerVR Rogue GX6250, which is one
> > > > of the Series6XT GPUs, another sub-family of the Rogue family.
> > >
> > > I've added Adam Ford who sent out some DT related patches [1] for the Renesas
> > > variant of GX6250 and the GX6650 (another Series6XT GPU).
> > >
> > > >
> > > > This was part of the very first few versions of the PowerVR submission,
> > > > but was later dropped. The compatible string has been updated to follow
> > > > the new naming scheme adopted for the AXE series.
> > > >
> > > > In a previous iteration of the PowerVR binding submission [1], the
> > > > number of clocks required for the 6XT family was mentioned to be
> > > > always 3. This is also reflected here.
> > > >
> > > > [1] https://lore.kernel.org/dri-devel/[email protected]/
> > > >
> > > > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > > > ---
> > > > .../bindings/gpu/img,powervr-rogue.yaml | 24 +++++++++++++++----
> > > > 1 file changed, 20 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> > > > index 256e252f8087..48aa205b66b4 100644
> > > > --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> > > > +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> > > > @@ -12,10 +12,17 @@ maintainers:
> > > >
> > > > properties:
> > > > compatible:
> > > > - items:
> > > > - - enum:
> > > > - - ti,am62-gpu
> > > > - - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
> > > > + oneOf:
> > > > + - items:
> > > > + - enum:
> > > > + - mediatek,mt8173-gpu
> > > > + # PowerVR 6XT GPU model/revision is fully discoverable
> > > > + - const: img,powervr-6xt
> > > > + - items:
> > > > + - enum:
> > > > + - ti,am62-gpu
> > > > + # IMG AXE GPU model/revision is fully discoverable
> > > > + - const: img,img-axe
> > >
> > > The Series6XT GPU models have differing numbers of power domains (either 2, 4 or
> > > 5). Whereas, the AXE GPUs have a single power domain, so I assume there should
> > > be a related change here.
> > >
> > > The GX6250 has two power domains (lets call them A and B). There's a constraint
> > > that if domain B is powered then domain A must also be powered.
> > >
> > > In patch 6 [2] it's setting the power domain to MT8173_POWER_DOMAIN_MFG, which I
> > > believe corresponds to power domain B. I assume this works because the MTK power
> > > controller driver is encoding the constraint above, meaning that when we disable
> > > or enable MT8173_POWER_DOMAIN_MFG it's also disabling/enabling MT8173_POWER_DOMA
> > > IN_MFG_2D (domain A).
> >
> > It could also be that the power domains are split in the glue layer and there
> > is some sequencing handled there. I'll reach out to MediaTek to see if they
> > can dig up some design specifics.
>
> Unfortunately they said they no longer have that information.
>
> > I assume you would like to see the separate power domains properly modeled
> > in the device tree?
>
> So how should we go about this? Adam, do you have this information for
> your platform?

In the Renesas platform, I only had to add one clock [1] and one
power-domain [2] to get the GPU to come up. In Renesas' downstream
driver, they only use one clock, but Geert, the Renesas tree
maintainer, sent me the three clocks to assign to the 6xt graphics if
it's determined that 3 clocks are required. In terms of the power
domain, there appear to be 2, but one is dependent on another, so
turning on the 'B' power domain turns on the 'A' power domain
automatically. I should note that I can't get the graphics to
function, since the 6xt isn't supported yet, so there could be some
elements missing that I am unaware of.

adam

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clk/renesas?h=next-20240613&id=f7b0dfffd3e0897ca73916a0c3d3fb61c61df51e
[2] - https://patchwork.kernel.org/project/linux-renesas-soc/patch/[email protected]/

>
> Thanks
> ChenYu
>
> >
> > Thanks
> > ChenYu
> >
> > > Thanks
> > > Frank
> > >
> > > [1] https://lists.freedesktop.org/archives/dri-devel/2024-February/443548.html
> > > [2] https://lists.freedesktop.org/archives/dri-devel/2024-May/455833.html
> > >
> > > >
> > > > reg:
> > > > maxItems: 1
> > > > @@ -56,6 +63,15 @@ allOf:
> > > > properties:
> > > > clocks:
> > > > maxItems: 1
> > > > + - if:
> > > > + properties:
> > > > + compatible:
> > > > + contains:
> > > > + const: img,powervr-6xt
> > > > + then:
> > > > + properties:
> > > > + clocks:
> > > > + minItems: 3
> > > >
> > > > examples:
> > > > - |