RE: [PATCH V11 1/7] clk: imx: add configuration option for mmio clks

[Stephen Boyd <sboyd@kernel.org>]

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.