| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Oct 2022 12:36:14 +0200
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
To: Bo-Chen Chen <rex-bc.chen@...iatek.com>, sboyd@...nel.org,
mturquette@...libre.com, matthias.bgg@...il.com,
p.zabel@...gutronix.de
Cc: runyang.chen@...iatek.com, miles.chen@...iatek.com,
wenst@...omium.org, nfraprado@...labora.com,
linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org,
Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH v6 1/3] reset: mediatek: Move MediaTek system clock reset
to reset/mediatek
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@...nel.org>
> Signed-off-by: Bo-Chen Chen <rex-bc.chen@...iatek.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
Powered by blists - more mailing lists