Re: [PATCH v2 03/19] clk: mediatek: Add MT8188 topckgen clock support

Re: [PATCH v2 03/19] clk: mediatek: Add MT8188 topckgen clock support

[AngeloGioacchino Del Regno]

Il 24/10/22 11:42, Garmin.Chang ha scritto:
> Add MT8188 topckgen clock controller which provides muxes, dividers
> to handle variety clock selection in other IP blocks.
> 
> Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> ---
>   drivers/clk/mediatek/Makefile              |    2 +-
>   drivers/clk/mediatek/clk-mt8188-topckgen.c | 1337 ++++++++++++++++++++
>   2 files changed, 1338 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/clk/mediatek/clk-mt8188-topckgen.c
> 
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index 957530b4303f..4fc7de25f036 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -82,7 +82,7 @@ obj-$(CONFIG_COMMON_CLK_MT8186) += clk-mt8186-mcu.o clk-mt8186-topckgen.o clk-mt
>   				   clk-mt8186-mfg.o clk-mt8186-mm.o clk-mt8186-wpe.o \
>   				   clk-mt8186-img.o clk-mt8186-vdec.o clk-mt8186-venc.o \
>   				   clk-mt8186-cam.o clk-mt8186-mdp.o clk-mt8186-ipe.o
> -obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o
> +obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o clk-mt8188-topckgen.o
>   obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192.o
>   obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
>   obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> diff --git a/drivers/clk/mediatek/clk-mt8188-topckgen.c b/drivers/clk/mediatek/clk-mt8188-topckgen.c
> new file mode 100644
> index 000000000000..c6ac6c2558ae
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8188-topckgen.c
> @@ -0,0 +1,1337 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Garmin Chang <garmin.chang@mediatek.com>
> +
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
> +
> +#include "clk-gate.h"
> +#include "clk-mtk.h"
> +#include "clk-mux.h"
> +
> +static DEFINE_SPINLOCK(mt8188_clk_lock);
> +
> +static const struct mtk_fixed_clk top_fixed_clks[] = {
> +	FIXED_CLK(CLK_TOP_ULPOSC1, "ulposc_ck1", NULL, 260000000),
> +	FIXED_CLK(CLK_TOP_MPHONE_SLAVE_BCK, "mphone_slave_bck", NULL, 49152000),
> +	FIXED_CLK(CLK_TOP_PAD_FPC, "pad_fpc_ck", NULL, 50000000),
> +	FIXED_CLK(CLK_TOP_466M_FMEM, "hd_466m_fmem_ck", NULL, 533000000),
> +	FIXED_CLK(CLK_TOP_PEXTP_PIPE, "pextp_pipe", NULL, 250000000),
> +	FIXED_CLK(CLK_TOP_DSI_PHY, "dsi_phy", NULL, 500000000),
> +};
> +
> +static const struct mtk_fixed_factor top_divs[] = {
> +	FACTOR(CLK_TOP_MFGPLL, "mfgpll_ck", "mfgpll", 1, 1),
> +	FACTOR(CLK_TOP_MAINPLL, "mainpll_ck", "mainpll", 1, 1),

Are you sure that mainpll and univpll should have CLK_SET_RATE_PARENT?
This will give issues in GPU DVFS, as this is the case with all MediaTek SoCs.

Please check this series and add it as a dependency of yours:
https://patchwork.kernel.org/project/linux-mediatek/list/?series=688135

> +	FACTOR(CLK_TOP_MAINPLL_D3, "mainpll_d3", "mainpll_ck", 1, 3),
> +	FACTOR(CLK_TOP_MAINPLL_D4, "mainpll_d4", "mainpll_ck", 1, 4),

..snip..

> +
> +static struct mtk_composite top_muxes[] = {
> +	/* CLK_MISC_CFG_3 */
> +	MUX(CLK_TOP_MFG_CK_FAST_REF, "mfg_ck_fast_ref", mfg_fast_ref_parents, 0x0250, 8, 1),
> +};

CLK_TOP_MFG_CK_FAST_REF is not a composite clock: you can register it with a call
to devm_clk_hw_register_mux() in the probe function.

Check clk-mt8195-topckgen.c upstream for an example.

Regards,
Angelo

Re: [PATCH v2 03/19] clk: mediatek: Add MT8188 topckgen clock support

[Garmin Chang (張家銘)]

On Thu, 2022-10-27 at 10:21 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/10/22 11:42, Garmin.Chang ha scritto:
> > Add MT8188 topckgen clock controller which provides muxes, dividers
> > to handle variety clock selection in other IP blocks.
> > 
> > Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> > ---
> >   drivers/clk/mediatek/Makefile              |    2 +-
> >   drivers/clk/mediatek/clk-mt8188-topckgen.c | 1337
> > ++++++++++++++++++++
> >   2 files changed, 1338 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/clk/mediatek/clk-mt8188-topckgen.c
> > 
> > diff --git a/drivers/clk/mediatek/Makefile
> > b/drivers/clk/mediatek/Makefile
> > index 957530b4303f..4fc7de25f036 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -82,7 +82,7 @@ obj-$(CONFIG_COMMON_CLK_MT8186) += clk-mt8186-
> > mcu.o clk-mt8186-topckgen.o clk-mt
> >   				   clk-mt8186-mfg.o clk-mt8186-mm.o
> > clk-mt8186-wpe.o \
> >   				   clk-mt8186-img.o clk-mt8186-vdec.o
> > clk-mt8186-venc.o \
> >   				   clk-mt8186-cam.o clk-mt8186-mdp.o
> > clk-mt8186-ipe.o
> > -obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o
> > +obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o clk-
> > mt8188-topckgen.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> > diff --git a/drivers/clk/mediatek/clk-mt8188-topckgen.c
> > b/drivers/clk/mediatek/clk-mt8188-topckgen.c
> > new file mode 100644
> > index 000000000000..c6ac6c2558ae
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mt8188-topckgen.c
> > @@ -0,0 +1,1337 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Garmin Chang <garmin.chang@mediatek.com>
> > +
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
> > +
> > +#include "clk-gate.h"
> > +#include "clk-mtk.h"
> > +#include "clk-mux.h"
> > +
> > +static DEFINE_SPINLOCK(mt8188_clk_lock);
> > +
> > +static const struct mtk_fixed_clk top_fixed_clks[] = {
> > +	FIXED_CLK(CLK_TOP_ULPOSC1, "ulposc_ck1", NULL, 260000000),
> > +	FIXED_CLK(CLK_TOP_MPHONE_SLAVE_BCK, "mphone_slave_bck", NULL,
> > 49152000),
> > +	FIXED_CLK(CLK_TOP_PAD_FPC, "pad_fpc_ck", NULL, 50000000),
> > +	FIXED_CLK(CLK_TOP_466M_FMEM, "hd_466m_fmem_ck", NULL,
> > 533000000),
> > +	FIXED_CLK(CLK_TOP_PEXTP_PIPE, "pextp_pipe", NULL, 250000000),
> > +	FIXED_CLK(CLK_TOP_DSI_PHY, "dsi_phy", NULL, 500000000),
> > +};
> > +
> > +static const struct mtk_fixed_factor top_divs[] = {
> > +	FACTOR(CLK_TOP_MFGPLL, "mfgpll_ck", "mfgpll", 1, 1),
> > +	FACTOR(CLK_TOP_MAINPLL, "mainpll_ck", "mainpll", 1, 1),
> 
> Are you sure that mainpll and univpll should have
> CLK_SET_RATE_PARENT?
> This will give issues in GPU DVFS, as this is the case with all
> MediaTek SoCs.
> 
> Please check this series and add it as a dependency of yours:
> 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=688135__;!!CTRNKA9wMg0ARbw!0ko8NpKA8KW1vp4lw-WRRe2pw_N0Z-2gbeAkzhfmwf9yK25-Bw7TZFfqTTWM_FQFF6A$
>  
> 
> > +	FACTOR(CLK_TOP_MAINPLL_D3, "mainpll_d3", "mainpll_ck", 1, 3),
> > +	FACTOR(CLK_TOP_MAINPLL_D4, "mainpll_d4", "mainpll_ck", 1, 4),
> 
> ..snip..
> 
> > +
> > +static struct mtk_composite top_muxes[] = {
> > +	/* CLK_MISC_CFG_3 */
> > +	MUX(CLK_TOP_MFG_CK_FAST_REF, "mfg_ck_fast_ref",
> > mfg_fast_ref_parents, 0x0250, 8, 1),
> > +};
> 
> CLK_TOP_MFG_CK_FAST_REF is not a composite clock: you can register it
> with a call
> to devm_clk_hw_register_mux() in the probe function.
> 
> Check clk-mt8195-topckgen.c upstream for an example.
> 
> Regards,
> Angelo
> 

  Ok, I will use devm_clk_hw_register_mux() in the next version.

> Thanks,
> Best Regards,
> Garmin

Re: [PATCH v2 02/19] clk: mediatek: Add MT8188 apmixedsys clock support

[AngeloGioacchino Del Regno]

Il 24/10/22 11:42, Garmin.Chang ha scritto:
> Add MT8188 apmixedsys clock controller which provides Plls
> generated from SoC 26m and ssusb clock gate control.
> 
> Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> Change-Id: I3b1b44155cc5bfe5ba6e860de857e7e9f48b66a7
> ---
>   drivers/clk/mediatek/Kconfig                 |  11 ++
>   drivers/clk/mediatek/Makefile                |   1 +
>   drivers/clk/mediatek/clk-mt8188-apmixedsys.c | 153 +++++++++++++++++++
>   3 files changed, 165 insertions(+)
>   create mode 100644 drivers/clk/mediatek/clk-mt8188-apmixedsys.c
> 

..snip..

> diff --git a/drivers/clk/mediatek/clk-mt8188-apmixedsys.c b/drivers/clk/mediatek/clk-mt8188-apmixedsys.c
> new file mode 100644
> index 000000000000..f09e11d0261e
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8188-apmixedsys.c

..snip..

> +
> +static int clk_mt8188_apmixed_probe(struct platform_device *pdev)
> +{
> +	struct clk_hw_onecell_data *clk_data;
> +	struct device_node *node = pdev->dev.of_node;
> +	int r;
> +
> +	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
> +	if (!clk_data)
> +		return -ENOMEM;
> +
> +	r = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
> +	if (r)
> +		goto free_apmixed_data;
> +
> +	r = mtk_clk_register_gates(node, apmixed_clks, ARRAY_SIZE(apmixed_clks), clk_data);

Please use mtk_clk_register_gates_with_dev().

Regards,
Angelo

Re: [PATCH v2 02/19] clk: mediatek: Add MT8188 apmixedsys clock support

[Garmin Chang (張家銘)]

On Thu, 2022-10-27 at 10:21 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/10/22 11:42, Garmin.Chang ha scritto:
> > Add MT8188 apmixedsys clock controller which provides Plls
> > generated from SoC 26m and ssusb clock gate control.
> > 
> > Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> > Change-Id: I3b1b44155cc5bfe5ba6e860de857e7e9f48b66a7
> > ---
> >   drivers/clk/mediatek/Kconfig                 |  11 ++
> >   drivers/clk/mediatek/Makefile                |   1 +
> >   drivers/clk/mediatek/clk-mt8188-apmixedsys.c | 153
> > +++++++++++++++++++
> >   3 files changed, 165 insertions(+)
> >   create mode 100644 drivers/clk/mediatek/clk-mt8188-apmixedsys.c
> > 
> 
> ..snip..
> 
> > diff --git a/drivers/clk/mediatek/clk-mt8188-apmixedsys.c
> > b/drivers/clk/mediatek/clk-mt8188-apmixedsys.c
> > new file mode 100644
> > index 000000000000..f09e11d0261e
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mt8188-apmixedsys.c
> 
> ..snip..
> 
> > +
> > +static int clk_mt8188_apmixed_probe(struct platform_device *pdev)
> > +{
> > +	struct clk_hw_onecell_data *clk_data;
> > +	struct device_node *node = pdev->dev.of_node;
> > +	int r;
> > +
> > +	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
> > +	if (!clk_data)
> > +		return -ENOMEM;
> > +
> > +	r = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls),
> > clk_data);
> > +	if (r)
> > +		goto free_apmixed_data;
> > +
> > +	r = mtk_clk_register_gates(node, apmixed_clks,
> > ARRAY_SIZE(apmixed_clks), clk_data);
> 
> Please use mtk_clk_register_gates_with_dev().
> 
> Regards,
> Angelo


  Ok, I will use mtk_clk_register_gates_with_dev() in the next version.

> Thanks,
> Best Regards,
> Garmin

Re: [PATCH v2 04/19] clk: mediatek: Add MT8188 peripheral clock support

[AngeloGioacchino Del Regno]

Il 24/10/22 11:42, Garmin.Chang ha scritto:
> Add MT8188 peripheral clock controller which provides clock
> gate control for ethernet/flashif/pcie/ssusb.
> 
> Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>

..snip..

> +
> +static const struct of_device_id of_match_clk_mt8188_peri_ao[] = {
> +	{
> +		.compatible = "mediatek,mt8188-pericfg_ao",
> +		.data = &peri_ao_desc,
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +

Here and for all of the other drivers that you're introducing with this series...

please compress this array to one line per entry, like so:

static const struct of_device_id of_match_clk_mt8188_peri_ao[] = {
	{ .compatible = "mediatek,mt8188-pericfg_ao", .data = &peri_ao_desc },
	{ /* sentinel */ }
};

Regards,
Angelo

Re: [PATCH v2 04/19] clk: mediatek: Add MT8188 peripheral clock support

[Garmin Chang (張家銘)]

On Thu, 2022-10-27 at 10:22 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/10/22 11:42, Garmin.Chang ha scritto:
> > Add MT8188 peripheral clock controller which provides clock
> > gate control for ethernet/flashif/pcie/ssusb.
> > 
> > Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> 
> ..snip..
> 
> > +
> > +static const struct of_device_id of_match_clk_mt8188_peri_ao[] = {
> > +	{
> > +		.compatible = "mediatek,mt8188-pericfg_ao",
> > +		.data = &peri_ao_desc,
> > +	}, {
> > +		/* sentinel */
> > +	}
> > +};
> > +
> 
> Here and for all of the other drivers that you're introducing with
> this series...
> 
> please compress this array to one line per entry, like so:
> 
> static const struct of_device_id of_match_clk_mt8188_peri_ao[] = {
> 	{ .compatible = "mediatek,mt8188-pericfg_ao", .data =
> &peri_ao_desc },
> 	{ /* sentinel */ }
> };
> 
> Regards,
> Angelo
> 

  Ok, I will modify all drivers in the next version.

> Thanks,
> Best Regards,
> Garmin

Re: [PATCH v2 10/19] clk: mediatek: Add MT8188 mfgcfg clock support

[AngeloGioacchino Del Regno]

Il 24/10/22 11:42, Garmin.Chang ha scritto:
> Add MT8188 mfg clock controller which provides clock gate
> control for GPU.
> 
> Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> ---
>   drivers/clk/mediatek/Makefile         |  2 +-
>   drivers/clk/mediatek/clk-mt8188-mfg.c | 50 +++++++++++++++++++++++++++
>   2 files changed, 51 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/clk/mediatek/clk-mt8188-mfg.c
> 
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index 21b05e880a3a..cd8870c28146 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -85,7 +85,7 @@ obj-$(CONFIG_COMMON_CLK_MT8186) += clk-mt8186-mcu.o clk-mt8186-topckgen.o clk-mt
>   obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o clk-mt8188-topckgen.o \
>   				   clk-mt8188-peri_ao.o clk-mt8188-infra_ao.o \
>   				   clk-mt8188-cam.o clk-mt8188-ccu.o clk-mt8188-img.o \
> -				   clk-mt8188-ipe.o
> +				   clk-mt8188-ipe.o clk-mt8188-mfg.o
>   obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192.o
>   obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
>   obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> diff --git a/drivers/clk/mediatek/clk-mt8188-mfg.c b/drivers/clk/mediatek/clk-mt8188-mfg.c
> new file mode 100644
> index 000000000000..3a75cd7443fd
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8188-mfg.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Garmin Chang <garmin.chang@mediatek.com>
> +
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
> +
> +#include "clk-gate.h"
> +#include "clk-mtk.h"
> +
> +static const struct mtk_gate_regs mfgcfg_cg_regs = {
> +	.set_ofs = 0x4,
> +	.clr_ofs = 0x8,
> +	.sta_ofs = 0x0,
> +};
> +
> +#define GATE_MFGCFG(_id, _name, _parent, _shift)			\
> +	GATE_MTK(_id, _name, _parent, &mfgcfg_cg_regs, _shift, &mtk_clk_gate_ops_setclr)
> +
> +static const struct mtk_gate mfgcfg_clks[] = {
> +	GATE_MFGCFG(CLK_MFGCFG_BG3D, "mfgcfg_bg3d", "top_mfg_core_tmp", 0),
> +};

This will make it impossible to properly perform GPU DVFS.

Hint:

#define GATE_MFG(_id, _name, _parent, _shift)			\
	GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs,	\
		       _shift, &mtk_clk_gate_ops_setclr,	\
		       CLK_SET_RATE_PARENT)

Regards,
Angelo

Re: [PATCH v2 10/19] clk: mediatek: Add MT8188 mfgcfg clock support

[Garmin Chang (張家銘)]

On Thu, 2022-10-27 at 10:23 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/10/22 11:42, Garmin.Chang ha scritto:
> > Add MT8188 mfg clock controller which provides clock gate
> > control for GPU.
> > 
> > Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> > ---
> >   drivers/clk/mediatek/Makefile         |  2 +-
> >   drivers/clk/mediatek/clk-mt8188-mfg.c | 50
> > +++++++++++++++++++++++++++
> >   2 files changed, 51 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/clk/mediatek/clk-mt8188-mfg.c
> > 
> > diff --git a/drivers/clk/mediatek/Makefile
> > b/drivers/clk/mediatek/Makefile
> > index 21b05e880a3a..cd8870c28146 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -85,7 +85,7 @@ obj-$(CONFIG_COMMON_CLK_MT8186) += clk-mt8186-
> > mcu.o clk-mt8186-topckgen.o clk-mt
> >   obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o clk-
> > mt8188-topckgen.o \
> >   				   clk-mt8188-peri_ao.o clk-mt8188-
> > infra_ao.o \
> >   				   clk-mt8188-cam.o clk-mt8188-ccu.o
> > clk-mt8188-img.o \
> > -				   clk-mt8188-ipe.o
> > +				   clk-mt8188-ipe.o clk-mt8188-mfg.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> > diff --git a/drivers/clk/mediatek/clk-mt8188-mfg.c
> > b/drivers/clk/mediatek/clk-mt8188-mfg.c
> > new file mode 100644
> > index 000000000000..3a75cd7443fd
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mt8188-mfg.c
> > @@ -0,0 +1,50 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Garmin Chang <garmin.chang@mediatek.com>
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/platform_device.h>
> > +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
> > +
> > +#include "clk-gate.h"
> > +#include "clk-mtk.h"
> > +
> > +static const struct mtk_gate_regs mfgcfg_cg_regs = {
> > +	.set_ofs = 0x4,
> > +	.clr_ofs = 0x8,
> > +	.sta_ofs = 0x0,
> > +};
> > +
> > +#define GATE_MFGCFG(_id, _name, _parent, _shift)			
> > \
> > +	GATE_MTK(_id, _name, _parent, &mfgcfg_cg_regs, _shift,
> > &mtk_clk_gate_ops_setclr)
> > +
> > +static const struct mtk_gate mfgcfg_clks[] = {
> > +	GATE_MFGCFG(CLK_MFGCFG_BG3D, "mfgcfg_bg3d", "top_mfg_core_tmp",
> > 0),
> > +};
> 
> This will make it impossible to properly perform GPU DVFS.
> 
> Hint:
> 
> #define GATE_MFG(_id, _name, _parent, _shift)			\
> 	GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs,	\
> 		       _shift, &mtk_clk_gate_ops_setclr,	\
> 		       CLK_SET_RATE_PARENT)
> 
> Regards,
> Angelo
> 

  Ok, I will modify them in the next version.

> 
> Thanks,
> Best Regards,
> Garmin

Re: [PATCH v2 08/19] clk: mediatek: Add MT8188 imgsys clock support

[AngeloGioacchino Del Regno]

Il 24/10/22 11:42, Garmin.Chang ha scritto:
> Add MT8188 imgsys clock controllers which provide clock gate
> control for image IP blocks.
> 
> Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> ---
>   drivers/clk/mediatek/Makefile         |   2 +-
>   drivers/clk/mediatek/clk-mt8188-img.c | 124 ++++++++++++++++++++++++++
>   2 files changed, 125 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/clk/mediatek/clk-mt8188-img.c
> 
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index bd0a2aa5b6fa..242b49bafa9e 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -84,7 +84,7 @@ obj-$(CONFIG_COMMON_CLK_MT8186) += clk-mt8186-mcu.o clk-mt8186-topckgen.o clk-mt
>   				   clk-mt8186-cam.o clk-mt8186-mdp.o clk-mt8186-ipe.o
>   obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o clk-mt8188-topckgen.o \
>   				   clk-mt8188-peri_ao.o clk-mt8188-infra_ao.o \
> -				   clk-mt8188-cam.o clk-mt8188-ccu.o
> +				   clk-mt8188-cam.o clk-mt8188-ccu.o clk-mt8188-img.o
>   obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192.o
>   obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
>   obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> diff --git a/drivers/clk/mediatek/clk-mt8188-img.c b/drivers/clk/mediatek/clk-mt8188-img.c
> new file mode 100644
> index 000000000000..00f3bbf4d502
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8188-img.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Garmin Chang <garmin.chang@mediatek.com>
> +
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
> +
> +#include "clk-gate.h"
> +#include "clk-mtk.h"
> +
> +static const struct mtk_gate_regs imgsys_cg_regs = {
> +	.set_ofs = 0x4,
> +	.clr_ofs = 0x8,
> +	.sta_ofs = 0x0,
> +};
> +
> +#define GATE_IMGSYS(_id, _name, _parent, _shift)			\
> +	GATE_MTK(_id, _name, _parent, &imgsys_cg_regs, _shift, &mtk_clk_gate_ops_setclr)
> +
> +static const struct mtk_gate imgsys_main_clks[] = {
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_LARB9, "imgsys_main_larb9", "top_img", 0),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW0, "imgsys_main_traw0", "top_img", 1),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW1, "imgsys_main_traw1", "top_img", 2),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_VCORE_GALS, "imgsys_main_vcore_gals", "top_img", 3),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_DIP0, "imgsys_main_dip0", "top_img", 8),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE0, "imgsys_main_wpe0", "top_img", 9),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_IPE, "imgsys_main_ipe", "top_img", 10),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE1, "imgsys_main_wpe1", "top_img", 12),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE2, "imgsys_main_wpe2", "top_img", 13),
> +	GATE_IMGSYS(CLK_IMGSYS_MAIN_GALS, "imgsys_main_gals", "top_img", 31),
> +};
> +
> +static const struct mtk_gate imgsys_wpe1_clks[] = {
> +	GATE_IMGSYS(CLK_IMGSYS_WPE1_LARB11, "imgsys_wpe1_larb11", "top_img", 0),
> +	GATE_IMGSYS(CLK_IMGSYS_WPE1, "imgsys_wpe1", "top_img", 1),
> +};
> +
> +static const struct mtk_gate imgsys_wpe2_clks[] = {
> +	GATE_IMGSYS(CLK_IMGSYS_WPE2_LARB11, "imgsys_wpe2_larb11", "top_img", 0),
> +	GATE_IMGSYS(CLK_IMGSYS_WPE2, "imgsys_wpe2", "top_img", 1),
> +};
> +
> +static const struct mtk_gate imgsys_wpe3_clks[] = {
> +	GATE_IMGSYS(CLK_IMGSYS_WPE3_LARB11, "imgsys_wpe3_larb11", "top_img", 0),
> +	GATE_IMGSYS(CLK_IMGSYS_WPE3, "imgsys_wpe3", "top_img", 1),
> +};
> +
> +static const struct mtk_gate imgsys1_dip_top_clks[] = {
> +	GATE_IMGSYS(CLK_IMGSYS1_DIP_TOP_LARB10, "imgsys1_dip_larb10", "top_img", 0),
> +	GATE_IMGSYS(CLK_IMGSYS1_DIP_TOP_DIP_TOP, "imgsys1_dip_dip_top", "top_img", 1),
> +};
> +
> +static const struct mtk_gate imgsys1_dip_nr_clks[] = {
> +	GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_LARB15, "imgsys1_dip_nr_larb15", "top_img", 0),
> +	GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_DIP_NR, "imgsys1_dip_nr_dip_nr", "top_img", 1),
> +};
> +
> +static const struct mtk_clk_desc imgsys_main_desc = {
> +	.clks = imgsys_main_clks,
> +	.num_clks = ARRAY_SIZE(imgsys_main_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys_wpe1_desc = {
> +	.clks = imgsys_wpe1_clks,
> +	.num_clks = ARRAY_SIZE(imgsys_wpe1_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys_wpe2_desc = {
> +	.clks = imgsys_wpe2_clks,
> +	.num_clks = ARRAY_SIZE(imgsys_wpe2_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys_wpe3_desc = {
> +	.clks = imgsys_wpe3_clks,
> +	.num_clks = ARRAY_SIZE(imgsys_wpe3_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys1_dip_top_desc = {
> +	.clks = imgsys1_dip_top_clks,
> +	.num_clks = ARRAY_SIZE(imgsys1_dip_top_clks),
> +};
> +
> +static const struct mtk_clk_desc imgsys1_dip_nr_desc = {
> +	.clks = imgsys1_dip_nr_clks,
> +	.num_clks = ARRAY_SIZE(imgsys1_dip_nr_clks),
> +};
> +
> +static const struct of_device_id of_match_clk_mt8188_imgsys_main[] = {
> +	{
> +		.compatible = "mediatek,mt8188-imgsys",
> +		.data = &imgsys_main_desc,
> +	}, {
> +		.compatible = "mediatek,mt8188-imgsys_wpe1",

I know that this was done in other clock drivers as well, but can we please
stop using underscores in devicetree compatibles?
That makes them look more consistent with the rest of the DT.

"mediatek,mt8188-imgsys-wpe1", as an example, would look a bit better.

P.S.: Please do the same on all of the other drivers that you are introducing
       with this series.

Thanks,
Angelo

Re: [PATCH v2 08/19] clk: mediatek: Add MT8188 imgsys clock support

[Garmin Chang (張家銘)]

On Thu, 2022-10-27 at 10:25 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/10/22 11:42, Garmin.Chang ha scritto:
> > Add MT8188 imgsys clock controllers which provide clock gate
> > control for image IP blocks.
> > 
> > Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> > ---
> >   drivers/clk/mediatek/Makefile         |   2 +-
> >   drivers/clk/mediatek/clk-mt8188-img.c | 124
> > ++++++++++++++++++++++++++
> >   2 files changed, 125 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/clk/mediatek/clk-mt8188-img.c
> > 
> > diff --git a/drivers/clk/mediatek/Makefile
> > b/drivers/clk/mediatek/Makefile
> > index bd0a2aa5b6fa..242b49bafa9e 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -84,7 +84,7 @@ obj-$(CONFIG_COMMON_CLK_MT8186) += clk-mt8186-
> > mcu.o clk-mt8186-topckgen.o clk-mt
> >   				   clk-mt8186-cam.o clk-mt8186-mdp.o
> > clk-mt8186-ipe.o
> >   obj-$(CONFIG_COMMON_CLK_MT8188) += clk-mt8188-apmixedsys.o clk-
> > mt8188-topckgen.o \
> >   				   clk-mt8188-peri_ao.o clk-mt8188-
> > infra_ao.o \
> > -				   clk-mt8188-cam.o clk-mt8188-ccu.o
> > +				   clk-mt8188-cam.o clk-mt8188-ccu.o
> > clk-mt8188-img.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
> >   obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> > diff --git a/drivers/clk/mediatek/clk-mt8188-img.c
> > b/drivers/clk/mediatek/clk-mt8188-img.c
> > new file mode 100644
> > index 000000000000..00f3bbf4d502
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mt8188-img.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Garmin Chang <garmin.chang@mediatek.com>
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/platform_device.h>
> > +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
> > +
> > +#include "clk-gate.h"
> > +#include "clk-mtk.h"
> > +
> > +static const struct mtk_gate_regs imgsys_cg_regs = {
> > +	.set_ofs = 0x4,
> > +	.clr_ofs = 0x8,
> > +	.sta_ofs = 0x0,
> > +};
> > +
> > +#define GATE_IMGSYS(_id, _name, _parent, _shift)			
> > \
> > +	GATE_MTK(_id, _name, _parent, &imgsys_cg_regs, _shift,
> > &mtk_clk_gate_ops_setclr)
> > +
> > +static const struct mtk_gate imgsys_main_clks[] = {
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_LARB9, "imgsys_main_larb9",
> > "top_img", 0),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW0, "imgsys_main_traw0",
> > "top_img", 1),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW1, "imgsys_main_traw1",
> > "top_img", 2),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_VCORE_GALS,
> > "imgsys_main_vcore_gals", "top_img", 3),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_DIP0, "imgsys_main_dip0",
> > "top_img", 8),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE0, "imgsys_main_wpe0",
> > "top_img", 9),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_IPE, "imgsys_main_ipe", "top_img",
> > 10),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE1, "imgsys_main_wpe1",
> > "top_img", 12),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_WPE2, "imgsys_main_wpe2",
> > "top_img", 13),
> > +	GATE_IMGSYS(CLK_IMGSYS_MAIN_GALS, "imgsys_main_gals",
> > "top_img", 31),
> > +};
> > +
> > +static const struct mtk_gate imgsys_wpe1_clks[] = {
> > +	GATE_IMGSYS(CLK_IMGSYS_WPE1_LARB11, "imgsys_wpe1_larb11",
> > "top_img", 0),
> > +	GATE_IMGSYS(CLK_IMGSYS_WPE1, "imgsys_wpe1", "top_img", 1),
> > +};
> > +
> > +static const struct mtk_gate imgsys_wpe2_clks[] = {
> > +	GATE_IMGSYS(CLK_IMGSYS_WPE2_LARB11, "imgsys_wpe2_larb11",
> > "top_img", 0),
> > +	GATE_IMGSYS(CLK_IMGSYS_WPE2, "imgsys_wpe2", "top_img", 1),
> > +};
> > +
> > +static const struct mtk_gate imgsys_wpe3_clks[] = {
> > +	GATE_IMGSYS(CLK_IMGSYS_WPE3_LARB11, "imgsys_wpe3_larb11",
> > "top_img", 0),
> > +	GATE_IMGSYS(CLK_IMGSYS_WPE3, "imgsys_wpe3", "top_img", 1),
> > +};
> > +
> > +static const struct mtk_gate imgsys1_dip_top_clks[] = {
> > +	GATE_IMGSYS(CLK_IMGSYS1_DIP_TOP_LARB10, "imgsys1_dip_larb10",
> > "top_img", 0),
> > +	GATE_IMGSYS(CLK_IMGSYS1_DIP_TOP_DIP_TOP, "imgsys1_dip_dip_top",
> > "top_img", 1),
> > +};
> > +
> > +static const struct mtk_gate imgsys1_dip_nr_clks[] = {
> > +	GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_LARB15, "imgsys1_dip_nr_larb15",
> > "top_img", 0),
> > +	GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_DIP_NR, "imgsys1_dip_nr_dip_nr",
> > "top_img", 1),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys_main_desc = {
> > +	.clks = imgsys_main_clks,
> > +	.num_clks = ARRAY_SIZE(imgsys_main_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys_wpe1_desc = {
> > +	.clks = imgsys_wpe1_clks,
> > +	.num_clks = ARRAY_SIZE(imgsys_wpe1_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys_wpe2_desc = {
> > +	.clks = imgsys_wpe2_clks,
> > +	.num_clks = ARRAY_SIZE(imgsys_wpe2_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys_wpe3_desc = {
> > +	.clks = imgsys_wpe3_clks,
> > +	.num_clks = ARRAY_SIZE(imgsys_wpe3_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys1_dip_top_desc = {
> > +	.clks = imgsys1_dip_top_clks,
> > +	.num_clks = ARRAY_SIZE(imgsys1_dip_top_clks),
> > +};
> > +
> > +static const struct mtk_clk_desc imgsys1_dip_nr_desc = {
> > +	.clks = imgsys1_dip_nr_clks,
> > +	.num_clks = ARRAY_SIZE(imgsys1_dip_nr_clks),
> > +};
> > +
> > +static const struct of_device_id of_match_clk_mt8188_imgsys_main[]
> > = {
> > +	{
> > +		.compatible = "mediatek,mt8188-imgsys",
> > +		.data = &imgsys_main_desc,
> > +	}, {
> > +		.compatible = "mediatek,mt8188-imgsys_wpe1",
> 
> I know that this was done in other clock drivers as well, but can we
> please
> stop using underscores in devicetree compatibles?
> That makes them look more consistent with the rest of the DT.
> 
> "mediatek,mt8188-imgsys-wpe1", as an example, would look a bit
> better.
> 
> P.S.: Please do the same on all of the other drivers that you are
> introducing
>        with this series.
> 
> Thanks,
> Angelo

Thank you for your suggestion.
Ok, I'll check all drivers for this underscores problem and fix them in
V3.

Thanks,
Best Regards,
Garmin

Re: [PATCH v2 01/19] dt-bindings: ARM: MediaTek: Add new document bindings of MT8188 clock

[Krzysztof Kozlowski]

On 24/10/2022 05:42, Garmin.Chang wrote:
> Add the new binding documentation for system clock
> and functional clock on MediaTek MT8188.
> 
> Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> ---
>  .../arm/mediatek/mediatek,mt8188-clock.yaml   |  70 ++
>  .../mediatek/mediatek,mt8188-sys-clock.yaml   |  55 ++
>  .../dt-bindings/clock/mediatek,mt8188-clk.h   | 733 ++++++++++++++++++
>  3 files changed, 858 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-clock.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-sys-clock.yaml
>  create mode 100644 include/dt-bindings/clock/mediatek,mt8188-clk.h
> 
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-clock.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-clock.yaml
> new file mode 100644
> index 000000000000..49dc681e6601
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-clock.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,mt8188-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Functional Clock Controller for MT8188
> +
> +maintainers:
> +  - Garmin Chang <garmin.chang@mediatek.com>
> +
> +description: |
> +  The clock architecture in MediaTek like below
> +  PLLs -->
> +          dividers -->
> +                      muxes
> +                           -->
> +                              clock gate
> +
> +  The devices provide clock gate control in different IP blocks.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8188-adsp_audio26m

No underscores in compatibles.

> +      - mediatek,mt8188-imp_iic_wrap_c
> +      - mediatek,mt8188-imp_iic_wrap_en
> +      - mediatek,mt8188-imp_iic_wrap_w
> +      - mediatek,mt8188-mfgcfg
> +      - mediatek,mt8188-vppsys0
> +      - mediatek,mt8188-wpesys
> +      - mediatek,mt8188-wpesys_vpp0
> +      - mediatek,mt8188-vppsys1
> +      - mediatek,mt8188-imgsys
> +      - mediatek,mt8188-imgsys_wpe1
> +      - mediatek,mt8188-imgsys_wpe2
> +      - mediatek,mt8188-imgsys_wpe3
> +      - mediatek,mt8188-imgsys1_dip_top
> +      - mediatek,mt8188-imgsys1_dip_nr
> +      - mediatek,mt8188-ipesys
> +      - mediatek,mt8188-camsys
> +      - mediatek,mt8188-camsys_rawa
> +      - mediatek,mt8188-camsys_yuva
> +      - mediatek,mt8188-camsys_rawb
> +      - mediatek,mt8188-camsys_yuvb
> +      - mediatek,mt8188-ccusys
> +      - mediatek,mt8188-vdecsys_soc
> +      - mediatek,mt8188-vdecsys
> +      - mediatek,mt8188-vencsys

Blank line here

> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    imp_iic_wrap_c: clock-controller@11283000 {

Drop the label, not used,

> +        compatible = "mediatek,mt8188-imp_iic_wrap_c";
> +        reg = <0x11283000 0x1000>;
> +        #clock-cells = <1>;
> +    };
> +
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-sys-clock.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-sys-clock.yaml
> new file mode 100644
> index 000000000000..35962b3746e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-sys-clock.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,mt8188-sys-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek System Clock Controller for MT8188
> +
> +maintainers:
> +  - Garmin Chang <garmin.chang@mediatek.com>
> +
> +description: |
> +  The clock architecture in MediaTek like below
> +  PLLs -->
> +          dividers -->
> +                      muxes
> +                           -->
> +                              clock gate
> +
> +  The apmixedsys provides most of PLLs which generated from SoC 26m.
> +  The topckgen provides dividers and muxes which provide the clock source to other IP blocks.
> +  The infracfg_ao provides clock gate in peripheral and infrastructure IP blocks.
> +  The mcusys provides mux control to select the clock source in AP MCU.
> +  The device nodes also provide the system control capacity for configuration.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - mediatek,mt8188-topckgen
> +          - mediatek,mt8188-infracfg_ao

Same comment.

> +          - mediatek,mt8188-apmixedsys
> +          - mediatek,mt8188-pericfg_ao
> +      - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    topckgen: syscon@10000000 {

Drop label.

> +        compatible = "mediatek,mt8188-topckgen", "syscon";
> +        reg = <0x10000000 0x1000>;
> +        #clock-cells = <1>;
Best regards,
Krzysztof

Re: [PATCH v2 01/19] dt-bindings: ARM: MediaTek: Add new document bindings of MT8188 clock

[Garmin Chang (張家銘)]

On Thu, 2022-10-27 at 09:42 -0400, Krzysztof Kozlowski wrote:
> On 24/10/2022 05:42, Garmin.Chang wrote:
> > Add the new binding documentation for system clock
> > and functional clock on MediaTek MT8188.
> > 
> > Signed-off-by: Garmin.Chang <Garmin.Chang@mediatek.com>
> > ---
> >  .../arm/mediatek/mediatek,mt8188-clock.yaml   |  70 ++
> >  .../mediatek/mediatek,mt8188-sys-clock.yaml   |  55 ++
> >  .../dt-bindings/clock/mediatek,mt8188-clk.h   | 733
> > ++++++++++++++++++
> >  3 files changed, 858 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-
> > clock.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-sys-
> > clock.yaml
> >  create mode 100644 include/dt-bindings/clock/mediatek,mt8188-clk.h
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-
> > clock.yaml
> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-
> > clock.yaml
> > new file mode 100644
> > index 000000000000..49dc681e6601
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-
> > clock.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/arm/mediatek/mediatek,mt8188-clock.yaml*__;Iw!!CTRNKA9wMg0ARbw!wZzNWz-zx7aQt1mTYTK-EfPEClJbCe_W0mEIFXwO3yquqcZp6T7_NpqtvnDuCceTAZs$
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!wZzNWz-zx7aQt1mTYTK-EfPEClJbCe_W0mEIFXwO3yquqcZp6T7_NpqtvnDuHuGTk0U$
> >  
> > +
> > +title: MediaTek Functional Clock Controller for MT8188
> > +
> > +maintainers:
> > +  - Garmin Chang <garmin.chang@mediatek.com>
> > +
> > +description: |
> > +  The clock architecture in MediaTek like below
> > +  PLLs -->
> > +          dividers -->
> > +                      muxes
> > +                           -->
> > +                              clock gate
> > +
> > +  The devices provide clock gate control in different IP blocks.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt8188-adsp_audio26m
> 
> No underscores in compatibles.
> 
> > +      - mediatek,mt8188-imp_iic_wrap_c
> > +      - mediatek,mt8188-imp_iic_wrap_en
> > +      - mediatek,mt8188-imp_iic_wrap_w
> > +      - mediatek,mt8188-mfgcfg
> > +      - mediatek,mt8188-vppsys0
> > +      - mediatek,mt8188-wpesys
> > +      - mediatek,mt8188-wpesys_vpp0
> > +      - mediatek,mt8188-vppsys1
> > +      - mediatek,mt8188-imgsys
> > +      - mediatek,mt8188-imgsys_wpe1
> > +      - mediatek,mt8188-imgsys_wpe2
> > +      - mediatek,mt8188-imgsys_wpe3
> > +      - mediatek,mt8188-imgsys1_dip_top
> > +      - mediatek,mt8188-imgsys1_dip_nr
> > +      - mediatek,mt8188-ipesys
> > +      - mediatek,mt8188-camsys
> > +      - mediatek,mt8188-camsys_rawa
> > +      - mediatek,mt8188-camsys_yuva
> > +      - mediatek,mt8188-camsys_rawb
> > +      - mediatek,mt8188-camsys_yuvb
> > +      - mediatek,mt8188-ccusys
> > +      - mediatek,mt8188-vdecsys_soc
> > +      - mediatek,mt8188-vdecsys
> > +      - mediatek,mt8188-vencsys
> 
> Blank line here
> 
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#clock-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    imp_iic_wrap_c: clock-controller@11283000 {
> 
> Drop the label, not used,
> 
> > +        compatible = "mediatek,mt8188-imp_iic_wrap_c";
> > +        reg = <0x11283000 0x1000>;
> > +        #clock-cells = <1>;
> > +    };
> > +
> > diff --git
> > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-
> > sys-clock.yaml
> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-
> > sys-clock.yaml
> > new file mode 100644
> > index 000000000000..35962b3746e1
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt8188-
> > sys-clock.yaml
> > @@ -0,0 +1,55 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/arm/mediatek/mediatek,mt8188-sys-clock.yaml*__;Iw!!CTRNKA9wMg0ARbw!wZzNWz-zx7aQt1mTYTK-EfPEClJbCe_W0mEIFXwO3yquqcZp6T7_NpqtvnDu6W88Oqk$
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!wZzNWz-zx7aQt1mTYTK-EfPEClJbCe_W0mEIFXwO3yquqcZp6T7_NpqtvnDuHuGTk0U$
> >  
> > +
> > +title: MediaTek System Clock Controller for MT8188
> > +
> > +maintainers:
> > +  - Garmin Chang <garmin.chang@mediatek.com>
> > +
> > +description: |
> > +  The clock architecture in MediaTek like below
> > +  PLLs -->
> > +          dividers -->
> > +                      muxes
> > +                           -->
> > +                              clock gate
> > +
> > +  The apmixedsys provides most of PLLs which generated from SoC
> > 26m.
> > +  The topckgen provides dividers and muxes which provide the clock
> > source to other IP blocks.
> > +  The infracfg_ao provides clock gate in peripheral and
> > infrastructure IP blocks.
> > +  The mcusys provides mux control to select the clock source in AP
> > MCU.
> > +  The device nodes also provide the system control capacity for
> > configuration.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - mediatek,mt8188-topckgen
> > +          - mediatek,mt8188-infracfg_ao
> 
> Same comment.
> 
> > +          - mediatek,mt8188-apmixedsys
> > +          - mediatek,mt8188-pericfg_ao
> > +      - const: syscon
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#clock-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    topckgen: syscon@10000000 {
> 
> Drop label.
> 
> > +        compatible = "mediatek,mt8188-topckgen", "syscon";
> > +        reg = <0x10000000 0x1000>;
> > +        #clock-cells = <1>;
> 
> Best regards,
> Krzysztof
> 

  Thank you for your suggestions.

  I will modify underscores in compatibles, add blank line and drop    
  label.

> Thanks,
> Best Regards,
> Garmin