| Message ID | 1544662938-5020-2-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: imx: add imx8qxp clock support | expand |
Quoting Aisheng Dong (2018-12-12 17:07:52) > The patch introduces CONFIG_MXC_CLK option for legacy MMIO clocks, > this is required to compile legacy MMIO clock conditionally when adding > SCU based clocks for MX8 platforms later. > > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <kernel@pengutronix.de> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: Michael Turquette <mturquette@baylibre.com> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index a47430b..8a9440a 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -72,7 +72,7 @@ obj-$(CONFIG_ARCH_DAVINCI) += davinci/ > obj-$(CONFIG_H8300) += h8300/ > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > obj-y += imgtec/ > -obj-$(CONFIG_ARCH_MXC) += imx/ > +obj-y += imx/ > obj-y += ingenic/ > obj-$(CONFIG_ARCH_K3) += keystone/ > obj-$(CONFIG_ARCH_KEYSTONE) += keystone/ > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig > new file mode 100644 > index 0000000..37478ba > --- /dev/null > +++ b/drivers/clk/imx/Kconfig > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# common clock support for NXP i.MX SoC family. > +config MXC_CLK > + bool > + def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ) Ok I looked closer, SOC_IMX8MQ is only being introduced now and has only been in linux-next for a few days. Having such a config symbol is highly unusual for arm64 code. Has that been accepted by arm-soc? I don't know why it would be because we don't add SoC specific config options anymore. I imagine it will be rejected and we'll need to have this new MXC_CLK config be enabled in the defconfig instead of selected from the arch layer. Given all that, is there any damage if this is just a def_bool ARCH_MXC right now? If it's enabled on the platforms that don't use it because they have SCU, does it actually matter? Or we just get some code bloat? If it's just some extra code, then even more reason to make a user visible config option that can be selected in the configurations that care to save some space. > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile > index 5c0b11e..f850424 100644 > --- a/drivers/clk/imx/Makefile > +++ b/drivers/clk/imx/Makefile > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0 > > -obj-y += \ > +obj-$(CONFIG_MXC_CLK) += \ > clk.o \ > clk-busy.o \ > clk-composite-8m.o \
On Thu, Dec 13, 2018 at 3:57 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Aisheng Dong (2018-12-12 17:07:52) > > The patch introduces CONFIG_MXC_CLK option for legacy MMIO clocks, > > this is required to compile legacy MMIO clock conditionally when adding > > SCU based clocks for MX8 platforms later. > > > > Cc: Shawn Guo <shawnguo@kernel.org> > > Cc: Sascha Hauer <kernel@pengutronix.de> > > Cc: Fabio Estevam <fabio.estevam@nxp.com> > > Cc: Stephen Boyd <sboyd@kernel.org> > > Cc: Michael Turquette <mturquette@baylibre.com> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > > index a47430b..8a9440a 100644 > > --- a/drivers/clk/Makefile > > +++ b/drivers/clk/Makefile > > @@ -72,7 +72,7 @@ obj-$(CONFIG_ARCH_DAVINCI) += davinci/ > > obj-$(CONFIG_H8300) += h8300/ > > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > > obj-y += imgtec/ > > -obj-$(CONFIG_ARCH_MXC) += imx/ > > +obj-y += imx/ > > obj-y += ingenic/ > > obj-$(CONFIG_ARCH_K3) += keystone/ > > obj-$(CONFIG_ARCH_KEYSTONE) += keystone/ > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig > > new file mode 100644 > > index 0000000..37478ba > > --- /dev/null > > +++ b/drivers/clk/imx/Kconfig > > @@ -0,0 +1,5 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +# common clock support for NXP i.MX SoC family. > > +config MXC_CLK > > + bool > > + def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ) > > Ok I looked closer, SOC_IMX8MQ is only being introduced now and has only > been in linux-next for a few days. Having such a config symbol is highly > unusual for arm64 code. Has that been accepted by arm-soc? I don't know > why it would be because we don't add SoC specific config options > anymore. I imagine it will be rejected and we'll need to have this new > MXC_CLK config be enabled in the defconfig instead of selected from the > arch layer. > > Given all that, is there any damage if this is just a def_bool ARCH_MXC > right now? If it's enabled on the platforms that don't use it because > they have SCU, does it actually matter? Or we just get some code bloat? > If it's just some extra code, then even more reason to make a user > visible config option that can be selected in the configurations that > care to save some space. On arm64 we generally add an ARCH_<family> and leave it at that. So, ARCH_MXC would be suitable, but we shouldn't add more finegrained than that. -Olof
On Thu, Dec 13, 2018 at 04:02:34PM +0800, Olof Johansson wrote: ... > > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > > > index a47430b..8a9440a 100644 > > > --- a/drivers/clk/Makefile > > > +++ b/drivers/clk/Makefile > > > @@ -72,7 +72,7 @@ obj-$(CONFIG_ARCH_DAVINCI) += davinci/ > > > obj-$(CONFIG_H8300) += h8300/ > > > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > > > obj-y += imgtec/ > > > -obj-$(CONFIG_ARCH_MXC) += imx/ > > > +obj-y += imx/ > > > obj-y += ingenic/ > > > obj-$(CONFIG_ARCH_K3) += keystone/ > > > obj-$(CONFIG_ARCH_KEYSTONE) += keystone/ > > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig > > > new file mode 100644 > > > index 0000000..37478ba > > > --- /dev/null > > > +++ b/drivers/clk/imx/Kconfig > > > @@ -0,0 +1,5 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# common clock support for NXP i.MX SoC family. > > > +config MXC_CLK > > > + bool > > > + def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ) > > > > Ok I looked closer, SOC_IMX8MQ is only being introduced now and has only > > been in linux-next for a few days. Having such a config symbol is highly > > unusual for arm64 code. Has that been accepted by arm-soc? I don't know > > why it would be because we don't add SoC specific config options > > anymore. I imagine it will be rejected and we'll need to have this new > > MXC_CLK config be enabled in the defconfig instead of selected from the > > arch layer. > > > > Given all that, is there any damage if this is just a def_bool ARCH_MXC > > right now? If it's enabled on the platforms that don't use it because > > they have SCU, does it actually matter? Or we just get some code bloat? > > If it's just some extra code, then even more reason to make a user > > visible config option that can be selected in the configurations that > > care to save some space. > > On arm64 we generally add an ARCH_<family> and leave it at that. So, > ARCH_MXC would be suitable, but we shouldn't add more finegrained than > that. Olof, I'm just about to send pull request for i.MX8MQ support, which introduced SOC_IMX8MQ option. It sounds like we should rework the patches to get SOC_IMX8MQ killed? Shawn
On 18-12-13 16:12:41, Shawn Guo wrote: > On Thu, Dec 13, 2018 at 04:02:34PM +0800, Olof Johansson wrote: > ... > > > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > > > > index a47430b..8a9440a 100644 > > > > --- a/drivers/clk/Makefile > > > > +++ b/drivers/clk/Makefile > > > > @@ -72,7 +72,7 @@ obj-$(CONFIG_ARCH_DAVINCI) += davinci/ > > > > obj-$(CONFIG_H8300) += h8300/ > > > > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > > > > obj-y += imgtec/ > > > > -obj-$(CONFIG_ARCH_MXC) += imx/ > > > > +obj-y += imx/ > > > > obj-y += ingenic/ > > > > obj-$(CONFIG_ARCH_K3) += keystone/ > > > > obj-$(CONFIG_ARCH_KEYSTONE) += keystone/ > > > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig > > > > new file mode 100644 > > > > index 0000000..37478ba > > > > --- /dev/null > > > > +++ b/drivers/clk/imx/Kconfig > > > > @@ -0,0 +1,5 @@ > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > +# common clock support for NXP i.MX SoC family. > > > > +config MXC_CLK > > > > + bool > > > > + def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ) > > > > > > Ok I looked closer, SOC_IMX8MQ is only being introduced now and has only > > > been in linux-next for a few days. Having such a config symbol is highly > > > unusual for arm64 code. Has that been accepted by arm-soc? I don't know > > > why it would be because we don't add SoC specific config options > > > anymore. I imagine it will be rejected and we'll need to have this new > > > MXC_CLK config be enabled in the defconfig instead of selected from the > > > arch layer. > > > > > > Given all that, is there any damage if this is just a def_bool ARCH_MXC > > > right now? If it's enabled on the platforms that don't use it because > > > they have SCU, does it actually matter? Or we just get some code bloat? > > > If it's just some extra code, then even more reason to make a user > > > visible config option that can be selected in the configurations that > > > care to save some space. > > > > On arm64 we generally add an ARCH_<family> and leave it at that. So, > > ARCH_MXC would be suitable, but we shouldn't add more finegrained than > > that. > > Olof, > > I'm just about to send pull request for i.MX8MQ support, which > introduced SOC_IMX8MQ option. It sounds like we should rework the > patches to get SOC_IMX8MQ killed? > That was introduced because of the way the clock were introduced by prior IMXs. What I suggest is to change the way the clocks are built a little bit. Something like this: diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index 5c0b11e..fdea7d4 100644 --- a/drivers/clk/imx/Makefile +++ b/drivers/clk/imx/Makefile @@ -3,13 +3,11 @@ obj-y += \ clk.o \ clk-busy.o \ - clk-composite-8m.o \ clk-cpu.o \ clk-composite-7ulp.o \ clk-divider-gate.o \ clk-fixup-div.o \ clk-fixup-mux.o \ - clk-frac-pll.o \ clk-gate-exclusive.o \ clk-gate2.o \ clk-pfd.o \ @@ -18,7 +16,6 @@ obj-y += \ clk-pllv2.o \ clk-pllv3.o \ clk-pllv4.o \ - clk-sccg-pll.o obj-$(CONFIG_SOC_IMX1) += clk-imx1.o obj-$(CONFIG_SOC_IMX21) += clk-imx21.o @@ -34,5 +31,9 @@ obj-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o obj-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o obj-$(CONFIG_SOC_IMX7D) += clk-imx7d.o obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o -obj-$(CONFIG_SOC_IMX8MQ) += clk-imx8mq.o obj-$(CONFIG_SOC_VF610) += clk-vf610.o + +obj-$(CONFIG_ARCH_MXC) += clk-imx8mq.o \ + clk-sccg-pll.o \ + clk-composite-8m.o \ + clk-frac-pll.o This way the frac, composite-8m and the sccg will only be built for arm64, and we also get rid of the SOC_IMX8MQ. But then, arm64 IMX clocks will not follow the same pattern as the arm32 ones. If that's ok with everyone, I already have a patch that solves this issue. > Shawn
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Thursday, December 13, 2018 3:58 PM > To: linux-clk@vger.kernel.org; Aisheng Dong <aisheng.dong@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com; > shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de; Aisheng Dong > <aisheng.dong@nxp.com>; Arnd Bergmann <arnd@arndb.de>; Olof Johansson > <olof@lixom.net> > Subject: Re: [PATCH V11 1/7] clk: imx: add configuration option for mmio clks > > Quoting Aisheng Dong (2018-12-12 17:07:52) > > The patch introduces CONFIG_MXC_CLK option for legacy MMIO clocks, > > this is required to compile legacy MMIO clock conditionally when > > adding SCU based clocks for MX8 platforms later. > > > > Cc: Shawn Guo <shawnguo@kernel.org> > > Cc: Sascha Hauer <kernel@pengutronix.de> > > Cc: Fabio Estevam <fabio.estevam@nxp.com> > > Cc: Stephen Boyd <sboyd@kernel.org> > > Cc: Michael Turquette <mturquette@baylibre.com> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> diff --git > > a/drivers/clk/Makefile b/drivers/clk/Makefile index a47430b..8a9440a > > 100644 > > --- a/drivers/clk/Makefile > > +++ b/drivers/clk/Makefile > > @@ -72,7 +72,7 @@ obj-$(CONFIG_ARCH_DAVINCI) += > davinci/ > > obj-$(CONFIG_H8300) += h8300/ > > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > > obj-y += imgtec/ > > -obj-$(CONFIG_ARCH_MXC) += imx/ > > +obj-y += imx/ > > obj-y += ingenic/ > > obj-$(CONFIG_ARCH_K3) += keystone/ > > obj-$(CONFIG_ARCH_KEYSTONE) += keystone/ > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig new > > file mode 100644 index 0000000..37478ba > > --- /dev/null > > +++ b/drivers/clk/imx/Kconfig > > @@ -0,0 +1,5 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +# common clock support for NXP i.MX SoC family. > > +config MXC_CLK > > + bool > > + def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && > > +SOC_IMX8MQ) > > Ok I looked closer, SOC_IMX8MQ is only being introduced now and has only > been in linux-next for a few days. Having such a config symbol is highly unusual > for arm64 code. Has that been accepted by arm-soc? I don't know why it > would be because we don't add SoC specific config options anymore. I imagine > it will be rejected and we'll need to have this new MXC_CLK config be enabled > in the defconfig instead of selected from the arch layer. > > Given all that, is there any damage if this is just a def_bool ARCH_MXC right > now? If it's enabled on the platforms that don't use it because they have SCU, > does it actually matter? Or we just get some code bloat? > If it's just some extra code, then even more reason to make a user visible config > option that can be selected in the configurations that care to save some space. > It make sense to me. No actual damage as we support multiplatforms using both MXC_CLK and MXC_CLK_SCU on ARMv8. And for users who want to save one of them, they do can change via memuconfig later. I will make it a def_bool ARCH_MXC. Thanks for the suggestion. BTW, as I also introduced SOC_IMX8QXP in Patch 4 and 7. diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile +obj-$(CONFIG_SOC_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o According to your former suggestion, I think we should remove it. So I'm going to make imx8qxp clk an user selectable configuration option which will select the common MXC_CLK_SCU. So a bit difference from former discussion, MXC_CLK_SCU will still be kept a hide option. This way is mostly like how QCOM does and should work for IMX as well. Then it will look like: config MXC_CLK_SCU bool config CLK_IMX8QXP bool "IMX8QXP SCU Clock" depends on ARCH_MXC && IMX_SCU && ARM64 select MXC_CLK_SCU help Build the driver for IMX8QXP SCU based clocks. If any issue, please let me know. Regards Dong Aisheng > > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index > > 5c0b11e..f850424 100644 > > --- a/drivers/clk/imx/Makefile > > +++ b/drivers/clk/imx/Makefile > > @@ -1,6 +1,6 @@ > > # SPDX-License-Identifier: GPL-2.0 > > > > -obj-y += \ > > +obj-$(CONFIG_MXC_CLK) += \ > > clk.o \ > > clk-busy.o \ > > clk-composite-8m.o \
Quoting Aisheng Dong (2018-12-13 04:49:07) > > -----Original Message----- > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > Sent: Thursday, December 13, 2018 3:58 PM > > To: linux-clk@vger.kernel.org; Aisheng Dong <aisheng.dong@nxp.com> > > Cc: linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com; > > shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>; dl-linux-imx > > <linux-imx@nxp.com>; kernel@pengutronix.de; Aisheng Dong > > <aisheng.dong@nxp.com>; Arnd Bergmann <arnd@arndb.de>; Olof Johansson > > <olof@lixom.net> > > Subject: Re: [PATCH V11 1/7] clk: imx: add configuration option for mmio clks > > > > Quoting Aisheng Dong (2018-12-12 17:07:52) > > > The patch introduces CONFIG_MXC_CLK option for legacy MMIO clocks, > > > this is required to compile legacy MMIO clock conditionally when > > > adding SCU based clocks for MX8 platforms later. > > > > > > Cc: Shawn Guo <shawnguo@kernel.org> > > > Cc: Sascha Hauer <kernel@pengutronix.de> > > > Cc: Fabio Estevam <fabio.estevam@nxp.com> > > > Cc: Stephen Boyd <sboyd@kernel.org> > > > Cc: Michael Turquette <mturquette@baylibre.com> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> diff --git > > > a/drivers/clk/Makefile b/drivers/clk/Makefile index a47430b..8a9440a > > > 100644 > > > --- a/drivers/clk/Makefile > > > +++ b/drivers/clk/Makefile > > > @@ -72,7 +72,7 @@ obj-$(CONFIG_ARCH_DAVINCI) += > > davinci/ > > > obj-$(CONFIG_H8300) += h8300/ > > > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > > > obj-y += imgtec/ > > > -obj-$(CONFIG_ARCH_MXC) += imx/ > > > +obj-y += imx/ > > > obj-y += ingenic/ > > > obj-$(CONFIG_ARCH_K3) += keystone/ > > > obj-$(CONFIG_ARCH_KEYSTONE) += keystone/ > > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig new > > > file mode 100644 index 0000000..37478ba > > > --- /dev/null > > > +++ b/drivers/clk/imx/Kconfig > > > @@ -0,0 +1,5 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# common clock support for NXP i.MX SoC family. > > > +config MXC_CLK > > > + bool > > > + def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && > > > +SOC_IMX8MQ) > > > > Ok I looked closer, SOC_IMX8MQ is only being introduced now and has only > > been in linux-next for a few days. Having such a config symbol is highly unusual > > for arm64 code. Has that been accepted by arm-soc? I don't know why it > > would be because we don't add SoC specific config options anymore. I imagine > > it will be rejected and we'll need to have this new MXC_CLK config be enabled > > in the defconfig instead of selected from the arch layer. > > > > Given all that, is there any damage if this is just a def_bool ARCH_MXC right > > now? If it's enabled on the platforms that don't use it because they have SCU, > > does it actually matter? Or we just get some code bloat? > > If it's just some extra code, then even more reason to make a user visible config > > option that can be selected in the configurations that care to save some space. > > > > It make sense to me. No actual damage as we support multiplatforms using both > MXC_CLK and MXC_CLK_SCU on ARMv8. And for users who want to save one of them, > they do can change via memuconfig later. > I will make it a def_bool ARCH_MXC. > Thanks for the suggestion. > > BTW, as I also introduced SOC_IMX8QXP in Patch 4 and 7. > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile > +obj-$(CONFIG_SOC_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o > > According to your former suggestion, I think we should remove it. > So I'm going to make imx8qxp clk an user selectable configuration option which > will select the common MXC_CLK_SCU. So a bit difference from former discussion, > MXC_CLK_SCU will still be kept a hide option. > > This way is mostly like how QCOM does and should work for IMX as well. > > Then it will look like: > config MXC_CLK_SCU > bool > > config CLK_IMX8QXP > bool "IMX8QXP SCU Clock" > depends on ARCH_MXC && IMX_SCU && ARM64 > select MXC_CLK_SCU > help > Build the driver for IMX8QXP SCU based clocks. > > If any issue, please let me know. Ok, so a library pattern, where MXC_CLK_SCU is the library of code that various SoC specific clk drivers use. Seems fine to me.
On Thu, Dec 13, 2018 at 4:13 PM Shawn Guo <shawnguo@kernel.org> wrote: > > On Thu, Dec 13, 2018 at 04:02:34PM +0800, Olof Johansson wrote: > ... > > > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > > > > index a47430b..8a9440a 100644 > > > > --- a/drivers/clk/Makefile > > > > +++ b/drivers/clk/Makefile > > > > @@ -72,7 +72,7 @@ obj-$(CONFIG_ARCH_DAVINCI) += davinci/ > > > > obj-$(CONFIG_H8300) += h8300/ > > > > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > > > > obj-y += imgtec/ > > > > -obj-$(CONFIG_ARCH_MXC) += imx/ > > > > +obj-y += imx/ > > > > obj-y += ingenic/ > > > > obj-$(CONFIG_ARCH_K3) += keystone/ > > > > obj-$(CONFIG_ARCH_KEYSTONE) += keystone/ > > > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig > > > > new file mode 100644 > > > > index 0000000..37478ba > > > > --- /dev/null > > > > +++ b/drivers/clk/imx/Kconfig > > > > @@ -0,0 +1,5 @@ > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > +# common clock support for NXP i.MX SoC family. > > > > +config MXC_CLK > > > > + bool > > > > + def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ) > > > > > > Ok I looked closer, SOC_IMX8MQ is only being introduced now and has only > > > been in linux-next for a few days. Having such a config symbol is highly > > > unusual for arm64 code. Has that been accepted by arm-soc? I don't know > > > why it would be because we don't add SoC specific config options > > > anymore. I imagine it will be rejected and we'll need to have this new > > > MXC_CLK config be enabled in the defconfig instead of selected from the > > > arch layer. > > > > > > Given all that, is there any damage if this is just a def_bool ARCH_MXC > > > right now? If it's enabled on the platforms that don't use it because > > > they have SCU, does it actually matter? Or we just get some code bloat? > > > If it's just some extra code, then even more reason to make a user > > > visible config option that can be selected in the configurations that > > > care to save some space. > > > > On arm64 we generally add an ARCH_<family> and leave it at that. So, > > ARCH_MXC would be suitable, but we shouldn't add more finegrained than > > that. > > Olof, > > I'm just about to send pull request for i.MX8MQ support, which > introduced SOC_IMX8MQ option. It sounds like we should rework the > patches to get SOC_IMX8MQ killed? Yeah, I think the suggestions downthread are good approaches to avoid it. Some platforms create SOC config options if they need it under drivers/soc, but it will depend on your platform if you truly need it; for most cases I think it'll just make dependencies more complicated. -Olof
[...] > > > > It make sense to me. No actual damage as we support multiplatforms > > using both MXC_CLK and MXC_CLK_SCU on ARMv8. And for users who want > to > > save one of them, they do can change via memuconfig later. > > I will make it a def_bool ARCH_MXC. > > Thanks for the suggestion. > > > > BTW, as I also introduced SOC_IMX8QXP in Patch 4 and 7. > > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile > > +obj-$(CONFIG_SOC_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o > > > > According to your former suggestion, I think we should remove it. > > So I'm going to make imx8qxp clk an user selectable configuration > > option which will select the common MXC_CLK_SCU. So a bit difference > > from former discussion, MXC_CLK_SCU will still be kept a hide option. > > > > This way is mostly like how QCOM does and should work for IMX as well. > > > > Then it will look like: > > config MXC_CLK_SCU > > bool > > > > config CLK_IMX8QXP > > bool "IMX8QXP SCU Clock" > > depends on ARCH_MXC && IMX_SCU && ARM64 > > select MXC_CLK_SCU > > help > > Build the driver for IMX8QXP SCU based clocks. > > > > If any issue, please let me know. > > Ok, so a library pattern, where MXC_CLK_SCU is the library of code that various > SoC specific clk drivers use. Seems fine to me. Thanks. Can you help take a look at V12 series to see if this new approach is okay to you? Regards Dong Aisheng
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 2dc12bf..d2f0bb5 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -294,6 +294,7 @@ source "drivers/clk/actions/Kconfig" source "drivers/clk/bcm/Kconfig" source "drivers/clk/hisilicon/Kconfig" source "drivers/clk/imgtec/Kconfig" +source "drivers/clk/imx/Kconfig" source "drivers/clk/ingenic/Kconfig" source "drivers/clk/keystone/Kconfig" source "drivers/clk/mediatek/Kconfig" diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index a47430b..8a9440a 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -72,7 +72,7 @@ obj-$(CONFIG_ARCH_DAVINCI) += davinci/ obj-$(CONFIG_H8300) += h8300/ obj-$(CONFIG_ARCH_HISI) += hisilicon/ obj-y += imgtec/ -obj-$(CONFIG_ARCH_MXC) += imx/ +obj-y += imx/ obj-y += ingenic/ obj-$(CONFIG_ARCH_K3) += keystone/ obj-$(CONFIG_ARCH_KEYSTONE) += keystone/ diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig new file mode 100644 index 0000000..37478ba --- /dev/null +++ b/drivers/clk/imx/Kconfig @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: GPL-2.0 +# common clock support for NXP i.MX SoC family. +config MXC_CLK + bool + def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ) diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index 5c0b11e..f850424 100644 --- a/drivers/clk/imx/Makefile +++ b/drivers/clk/imx/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 -obj-y += \ +obj-$(CONFIG_MXC_CLK) += \ clk.o \ clk-busy.o \ clk-composite-8m.o \
The patch introduces CONFIG_MXC_CLK option for legacy MMIO clocks, this is required to compile legacy MMIO clock conditionally when adding SCU based clocks for MX8 platforms later. Cc: Shawn Guo <shawnguo@kernel.org> Cc: Sascha Hauer <kernel@pengutronix.de> Cc: Fabio Estevam <fabio.estevam@nxp.com> Cc: Stephen Boyd <sboyd@kernel.org> Cc: Michael Turquette <mturquette@baylibre.com> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- v10->v11: * break the dependency on the arch Kconfig v1->v10: no changes --- drivers/clk/Kconfig | 1 + drivers/clk/Makefile | 2 +- drivers/clk/imx/Kconfig | 5 +++++ drivers/clk/imx/Makefile | 2 +- 4 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 drivers/clk/imx/Kconfig