Re: [PATCH v6 1/3] reset: mediatek: Move MediaTek system clock reset to reset/mediatek

From: Rex-BC Chen (陳柏辰)
Date: Mon Oct 31 2022 - 01:15:28 EST

On Tue, 2022-10-25 at 12:36 +0200, AngeloGioacchino Del Regno wrote:
> Il 21/10/22 12:48, Bo-Chen Chen ha scritto:
> > To manager MediaTek system clock reset easier, we move the driver
> > to
> > drivers/reset/mediatek.
> >
> > - Create reset/mediatek folder.
> > - Move clk/mediatek/reset.c to reset/mediatek/reset-mediatek-
> > sysclk.c
> > - Because we don't want to build in unsed static variable, we use
> > clk
> > KConfig to separate them. For example, when we use MT8186, we
> > don't
> > want to build in the static constants for MT8195.
> > - Move reset data which are scattered around the mediatek drivers
> > to
> > reset-mtxxxx.c.
> > - There are two version for mtk_reset_init because some mediatek
> > clock
> > drivers (mt8135 and mt8173) are using device_node instead of
> > device,
> > so we need to add two version for the init function.
> >
> > Suggested-by: Stephen Boyd <sboyd@xxxxxxxxxx>
> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@xxxxxxxxxxxx>
> > ---
> > drivers/clk/mediatek/Kconfig | 1 +
> > drivers/clk/mediatek/Makefile | 2 +-
> > drivers/clk/mediatek/clk-mt2701-eth.c | 10 +-
> > drivers/clk/mediatek/clk-mt2701-g3d.c | 10 +-
> > drivers/clk/mediatek/clk-mt2701-hif.c | 10 +-
> > drivers/clk/mediatek/clk-mt2701.c | 22 +-
> > drivers/clk/mediatek/clk-mt2712.c | 22 +-
> > drivers/clk/mediatek/clk-mt6795-infracfg.c | 22 +-
> > drivers/clk/mediatek/clk-mt6795-pericfg.c | 20 +-
> > drivers/clk/mediatek/clk-mt7622-eth.c | 10 +-
> > drivers/clk/mediatek/clk-mt7622-hif.c | 12 +-
> > drivers/clk/mediatek/clk-mt7622.c | 22 +-
> > drivers/clk/mediatek/clk-mt7629-eth.c | 10 +-
> > drivers/clk/mediatek/clk-mt7629-hif.c | 12 +-
> > drivers/clk/mediatek/clk-mt8135.c | 23 +-
> > drivers/clk/mediatek/clk-mt8173.c | 22 +-
> > drivers/clk/mediatek/clk-mt8183.c | 15 +-
> > drivers/clk/mediatek/clk-mt8186-infra_ao.c | 23 +-
> > drivers/clk/mediatek/clk-mt8192.c | 27 +-
> > drivers/clk/mediatek/clk-mt8195-infra_ao.c | 28 +-
> > drivers/clk/mediatek/clk-mtk.c | 5 +-
> > drivers/clk/mediatek/clk-mtk.h | 5 +-
> > drivers/clk/mediatek/reset.c | 233 -----------
> > drivers/reset/Kconfig | 1 +
> > drivers/reset/Makefile | 1 +
> > drivers/reset/mediatek/Kconfig | 5 +
> > drivers/reset/mediatek/Makefile | 13 +
> > .../reset/mediatek/reset-mediatek-sysclk.c | 388
> > ++++++++++++++++++
> > drivers/reset/mediatek/reset-mt2701.c | 102 +++++
> > drivers/reset/mediatek/reset-mt2712.c | 42 ++
> > drivers/reset/mediatek/reset-mt6795.c | 61 +++
> > drivers/reset/mediatek/reset-mt7622.c | 91 ++++
> > drivers/reset/mediatek/reset-mt7629.c | 62 +++
> > drivers/reset/mediatek/reset-mt8135.c | 43 ++
> > drivers/reset/mediatek/reset-mt8173.c | 43 ++
> > drivers/reset/mediatek/reset-mt8183.c | 31 ++
> > drivers/reset/mediatek/reset-mt8186.c | 39 ++
> > drivers/reset/mediatek/reset-mt8192.c | 43 ++
> > drivers/reset/mediatek/reset-mt8195.c | 44 ++
> > .../linux/reset/reset-mediatek-sysclk.h | 62 +--
> > 40 files changed, 1080 insertions(+), 557 deletions(-)
> > delete mode 100644 drivers/clk/mediatek/reset.c
> > create mode 100644 drivers/reset/mediatek/Kconfig
> > create mode 100644 drivers/reset/mediatek/Makefile
> > create mode 100644 drivers/reset/mediatek/reset-mediatek-sysclk.c
> > create mode 100644 drivers/reset/mediatek/reset-mt2701.c
> > create mode 100644 drivers/reset/mediatek/reset-mt2712.c
> > create mode 100644 drivers/reset/mediatek/reset-mt6795.c
> > create mode 100644 drivers/reset/mediatek/reset-mt7622.c
> > create mode 100644 drivers/reset/mediatek/reset-mt7629.c
> > create mode 100644 drivers/reset/mediatek/reset-mt8135.c
> > create mode 100644 drivers/reset/mediatek/reset-mt8173.c
> > create mode 100644 drivers/reset/mediatek/reset-mt8183.c
> > create mode 100644 drivers/reset/mediatek/reset-mt8186.c
> > create mode 100644 drivers/reset/mediatek/reset-mt8192.c
> > create mode 100644 drivers/reset/mediatek/reset-mt8195.c
> > rename drivers/clk/mediatek/reset.h => include/linux/reset/reset-
> > mediatek-sysclk.h (59%)
> >
> > diff --git a/drivers/clk/mediatek/Kconfig
> > b/drivers/clk/mediatek/Kconfig
> > index 843cea0c7a44..e372f145eada 100644
> > --- a/drivers/clk/mediatek/Kconfig
> > +++ b/drivers/clk/mediatek/Kconfig
> > @@ -8,6 +8,7 @@ menu "Clock driver for MediaTek SoC"
> > config COMMON_CLK_MEDIATEK
> > tristate
> > select RESET_CONTROLLER
> > + select RESET_MEDIATEK_SYSCLK
> > help
> > MediaTek SoCs' clock support.
> >
>
> ..snip..
>
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index 3e7e5fd633a8..5cef7ccc9a7d 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -1,6 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> > obj-y += core.o
> > obj-y += hisilicon/
> > +obj-y += mediatek/
>
> I'd be more for
>
> obj-$(CONFIG_ARCH_MEDIATEK) += mediatek/
>
> as there's no reason to even compile these if MTK support isn't
> enabled at all.
>

Hello Angelo,

thanks for your review.
I obj-y += mediatek/ because if I don't write like this, it will build
fail for x86.
Is there any suggestion for this?

/tmp/src_kernel/prebuilt/toolchain/0day/gcc/x86_64-linux/bin/x86_64-
linux-ld: vmlinux.o: in function `mtk_pericfg_init':
clk-mt8135.c:(.init.text+0x12a2b7): undefined reference to
`mtk_reset_init_with_node'
/tmp/src_kernel/prebuilt/toolchain/0day/gcc/x86_64-linux/bin/x86_64-
linux-ld: vmlinux.o: in function `mtk_infrasys_init':
clk-mt8135.c:(.init.text+0x12a3bb): undefined reference to
`mtk_reset_init_with_node'
/tmp/src_kernel/prebuilt/toolchain/0day/gcc/x86_64-linux/bin/x86_64-
linux-ld: vmlinux.o: in function `mtk_infrasys_init':
clk-mt8173.c:(.init.text+0x12ac47): undefined reference to
`mtk_reset_init_with_node'
/tmp/src_kernel/prebuilt/toolchain/0day/gcc/x86_64-linux/bin/x86_64-
linux-ld: vmlinux.o: in function `mtk_pericfg_init':
clk-mt8173.c:(.init.text+0x12ad25): undefined reference to
`mtk_reset_init_with_node'
/tmp/src_kernel/kernel/mediatek/scripts/Makefile.vmlinux:34: recipe for
target 'vmlinux' failed
make[3]: *** [vmlinux] Error 1
make[3]: Target '__default' not remade because of errors.
/tmp/src_kernel/kernel/mediatek/Makefile:1236: recipe for target
'vmlinux' failed
make[2]: *** [vmlinux] Error 2
make[2]: Target '__all' not remade because of errors.
make[2]: Leaving directory '/tmp/out_kernel/out/allyesconfig.x86_64'
Makefile:231: recipe for target '__sub-make' failed
make[1]: *** [__sub-make] Error 2
make[1]: Target '__all' not remade because of errors.
make[1]: Leaving directory '/tmp/src_kernel/kernel/mediatek'
build/core/kbuild_test.mk:61: recipe for target 'all' failed
make: *** [all] Error 2
[11:44:04] Error: failed to build allyesconfig.x86_64

> > obj-$(CONFIG_ARCH_STI) += sti/
> > obj-$(CONFIG_ARCH_TEGRA) += tegra/
> > obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
> > diff --git a/drivers/reset/mediatek/Kconfig
> > b/drivers/reset/mediatek/Kconfig
> > new file mode 100644
> > index 000000000000..a416cb938753
> > --- /dev/null
> > +++ b/drivers/reset/mediatek/Kconfig
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
>
> Similarly, we should at this point also do....
>
> if ARCH_MEDIATEK
>
> > +config RESET_MEDIATEK_SYSCLK
> > + tristate "MediaTek System Clock Reset Driver"
> > + help
> > + This enables the system clock reset driver for MediaTek SoCs.
>
> endif # ARCH_MEDIATEK
>
> > diff --git a/drivers/reset/mediatek/Makefile
> > b/drivers/reset/mediatek/Makefile
> > new file mode 100644
> > index 000000000000..83f26c2cecdd
> > --- /dev/null
> > +++ b/drivers/reset/mediatek/Makefile
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_RESET_MEDIATEK_SYSCLK) += reset-mediatek-sysclk.o
> > +obj-$(CONFIG_COMMON_CLK_MT2701) += reset-mt2701.o
> > +obj-$(CONFIG_COMMON_CLK_MT2712) += reset-mt2712.o
> > +obj-$(CONFIG_COMMON_CLK_MT6795) += reset-mt6795.o
> > +obj-$(CONFIG_COMMON_CLK_MT7622) += reset-mt7622.o
> > +obj-$(CONFIG_COMMON_CLK_MT7629) += reset-mt7629.o
> > +obj-$(CONFIG_COMMON_CLK_MT8135) += reset-mt8135.o
> > +obj-$(CONFIG_COMMON_CLK_MT8173) += reset-mt8173.o
> > +obj-$(CONFIG_COMMON_CLK_MT8183) += reset-mt8183.o
> > +obj-$(CONFIG_COMMON_CLK_MT8186) += reset-mt8186.o
> > +obj-$(CONFIG_COMMON_CLK_MT8192) += reset-mt8192.o
> > +obj-$(CONFIG_COMMON_CLK_MT8195) += reset-mt8195.o
> > diff --git a/drivers/reset/mediatek/reset-mediatek-sysclk.c
> > b/drivers/reset/mediatek/reset-mediatek-sysclk.c
> > new file mode 100644
> > index 000000000000..9cf115e66a4d
> > --- /dev/null
> > +++ b/drivers/reset/mediatek/reset-mediatek-sysclk.c
>
> ..snip..
>
> > +
> > +static struct mtk_rst_id mtk_sysclk_reset_ids[] = {
>
> ..snip..
>
> > + {
> > + .name = "mt2712-peri-rst",
> > + .driver_data = MTK_RST_ID_MT2712_PERI,
> > + },
> > + {
> > + .name = "mt6795-ifa",
>
> Keep the names consistent please... "mt6795-infra-rst"
>

The reason I rename it as this because the limitation of name in
struct auxiliary_device_id. It's 32. The
KBUILD_MODNAME "clk_mt6795_infracfg" and "clk_mt6795_pericfg" is fixed,
so I need to modify the device_name.

refs:
https://elixir.bootlin.com/linux/latest/source/include/linux/mod_devicetable.h#L854

> > + .driver_data = MTK_RST_ID_MT6795_INFRA,
> > + },
> > + {
> > + .name = "mt6795-peri",
>
> mt6795-peri-rst
>
> > + .driver_data = MTK_RST_ID_MT6795_PERI,
> > + },
> > + {
> > + .name = "mt7622-eth-rst",
> > + .driver_data = MTK_RST_ID_MT7622_ETH,
> > + },
> > + {
> > + .name = "mt7622-usb-rst",
> > + .driver_data = MTK_RST_ID_MT7622_SSUSBSYS,
> > + },
> > + {
> > + .name = "mt7622-pcie-rst",
> > + .driver_data = MTK_RST_ID_MT7622_PCIESYS,
> > + },
> > + {
> > + .name = "mt7622-infrasys-rst",
> > + .driver_data = MTK_RST_ID_MT7622_INFRASYS,
> > + },
> > + {
> > + .name = "mt7622-pericfg-rst",
> > + .driver_data = MTK_RST_ID_MT7622_PERICFG,
> > + },
> > + {
> > + .name = "mt7629-ethsys-rst",
> > + .driver_data = MTK_RST_ID_MT7629_ETHSYS,
> > + },
> > + {
> > + .name = "mt7629-usb-rst",
> > + .driver_data = MTK_RST_ID_MT7629_SSUSBSYS,
> > + },
> > + {
> > + .name = "mt7629-pcie-rst",
> > + .driver_data = MTK_RST_ID_MT7629_PCIESYS,
> > + },
> > + {
> > + .name = "clk_mt8135.mt8135-infrasys-rst",
>
> Why do we have this "clk_mt8135." prefix here (and also mt8173), when
> all of
> the others don't have any prefix?
>
> That's not consistent and shall be changed, unless there's a valid
> reason not to.
>

I do this because in patch [3/3], other name will be added
KBUILD_MODNAME and for 8135 and 8173, we can not use the aux bus
interface because there is no have not device provided by them.

> > + .driver_data = MTK_RST_ID_MT8135_INFRASYS,
> > + },
> > + {
> > + .name = "clk_mt8135.mt8135-pericfg-rst",
> > + .driver_data = MTK_RST_ID_MT8135_PERICFG,
> > + },
> > + {
> > + .name = "clk_mt8173.mt8173-infracfg-rst",
> > + .driver_data = MTK_RST_ID_MT8173_INFRACFG,
> > + },
> > + {
> > + .name = "clk_mt8173.mt8173-pericfg-rst",
> > + .driver_data = MTK_RST_ID_MT8173_PERICFG,
> > + },
> > + {
> > + .name = "mt8183-infra-rst",
> > + .driver_data = MTK_RST_ID_MT8183_INFRA,
> > + },
> > + {
> > + .name = "mt8186-infra-ao-rst",
> > + .driver_data = MTK_RST_ID_MT8186_INFRA_AO,
> > + },
> > + {
> > + .name = "mt8192-infra-rst",
> > + .driver_data = MTK_RST_ID_MT8192_INFRA,
> > + },
> > + {
> > + .name = "mt8195-infra-ao-rst",
> > + .driver_data = MTK_RST_ID_MT8195_INFRA_AO,
> > + },
> > + {
> > + },
> > +};
> > +
>
> ..snip..
>
> > +
> > +int mtk_rst_register_clk_rst_data(u32 index, struct
> > mtk_clk_rst_data *data)
> > +{
> > + if (index >= MTK_RST_ID_MAX)
> > + return -EINVAL;
> > +
> > + p_clk_rst_data[index] = data;
> > +
> > + pr_info("%s, register mediatek sysclock reset(%d).\n",
> > __func__, index);
>
> Is this really informative?
> There's sysfs telling you infos about drivers that has been (or
> hasn't been yet)
> registered... so, please change that to a pr_debug() instead.
>

ok, I will do this.

BRs,
Bo-Chen
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_rst_register_clk_rst_data);
> > +
>
> Apart from that, looks good.
>
> Regards,
> Angelo