Messages in this thread Patch in this message | | | Date | Tue, 25 Oct 2022 12:36:14 +0200 | Subject | Re: [PATCH v6 1/3] reset: mediatek: Move MediaTek system clock reset to reset/mediatek | From | AngeloGioacchino Del Regno <> |
| |
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@kernel.org> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- > 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.
> 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"
> + .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.
> + .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.
> + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mtk_rst_register_clk_rst_data); > +
Apart from that, looks good.
Regards, Angelo
| |