[V9,1/7] clk: imx: add configuration option for mmio clks

Message ID 1543934041-12572-2-git-send-email-aisheng.dong@nxp.com
State New
Headers show
Series
  • clk: imx: add imx8qxp clock support
Related show

Commit Message

Aisheng Dong Dec. 4, 2018, 2:39 p.m.
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>
---
 arch/arm/mach-imx/Kconfig | 11 +++++++++++
 drivers/clk/Kconfig       |  1 +
 drivers/clk/imx/Kconfig   |  5 +++++
 drivers/clk/imx/Makefile  |  2 +-
 4 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/imx/Kconfig

Comments

Stephen Boyd Dec. 10, 2018, 8:55 p.m. | #1
Quoting Aisheng DONG (2018-12-04 06:39:22)
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index c12a05c..303082c 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -570,6 +580,7 @@ config SOC_IMX7ULP
>  config SOC_VF610
>         bool "Vybrid Family VF610 support"
>         select ARM_GIC if ARCH_MULTI_V7
> +       select MXC_CLK
>         select PINCTRL_VF610
>  

Instead of select can we break this dependency on the arch Kconfig and
have:

	config MXC_CLK
		def_bool ARCH_MXC && !ARM64

> 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) += \

Because this changes to obj-$(CONFIG) we should change the
drivers/clk/Makefile to have obj-y for this Makefile instead of
depending on ARCH_MXC.
Aisheng Dong Dec. 11, 2018, 2:36 a.m. | #2
> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: Tuesday, December 11, 2018 4:55 AM
[...]
> Subject: Re: [PATCH V9 1/7] clk: imx: add configuration option for mmio clks
> 
> Quoting Aisheng DONG (2018-12-04 06:39:22)
> > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > index c12a05c..303082c 100644
> > --- a/arch/arm/mach-imx/Kconfig
> > +++ b/arch/arm/mach-imx/Kconfig
> > @@ -570,6 +580,7 @@ config SOC_IMX7ULP  config SOC_VF610
> >         bool "Vybrid Family VF610 support"
> >         select ARM_GIC if ARCH_MULTI_V7
> > +       select MXC_CLK
> >         select PINCTRL_VF610
> >
> 
> Instead of select can we break this dependency on the arch Kconfig and
> have:
> 
> 	config MXC_CLK
> 		def_bool ARCH_MXC && !ARM64
> 

Thanks for the suggestion.
One little problem is that we also have LS1021 supported under ARCH_MXC which
does not use MXC_CLK.
 
> > 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) += \
> 
> Because this changes to obj-$(CONFIG) we should change the
> drivers/clk/Makefile to have obj-y for this Makefile instead of depending on
> ARCH_MXC.

We also use ARCH_MXC for ARMv8 SoC clocks.

Regards
Dong Aisheng
Stephen Boyd Dec. 11, 2018, 7:29 p.m. | #3
Quoting Aisheng Dong (2018-12-10 18:36:29)
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > Sent: Tuesday, December 11, 2018 4:55 AM
> [...]
> > Subject: Re: [PATCH V9 1/7] clk: imx: add configuration option for mmio clks
> > 
> > Quoting Aisheng DONG (2018-12-04 06:39:22)
> > > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > > index c12a05c..303082c 100644
> > > --- a/arch/arm/mach-imx/Kconfig
> > > +++ b/arch/arm/mach-imx/Kconfig
> > > @@ -570,6 +580,7 @@ config SOC_IMX7ULP  config SOC_VF610
> > >         bool "Vybrid Family VF610 support"
> > >         select ARM_GIC if ARCH_MULTI_V7
> > > +       select MXC_CLK
> > >         select PINCTRL_VF610
> > >
> > 
> > Instead of select can we break this dependency on the arch Kconfig and
> > have:
> > 
> >       config MXC_CLK
> >               def_bool ARCH_MXC && !ARM64
> > 
> 
> Thanks for the suggestion.
> One little problem is that we also have LS1021 supported under ARCH_MXC which
> does not use MXC_CLK.

Ok. So then your change is also limiting the compilation of
drivers/clk/imx/ to only the platforms that select it now, instead of
all ARCH_MXC like it was done before.

We can also have def_bool <big list of SoC platforms> if that helps for
the optimization that this patch is making.

arm-soc has generally pushed against having so many different arch level
Kconfig options because they make for unwieldy Kconfig fragments that
may become conflicting. Instead, the options for the different SoCs are
removed and we just have one for the platform and rely on defconfigs to
pick the right drivers. 

Why can't we do that here?

>  
> > > 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) += \
> > 
> > Because this changes to obj-$(CONFIG) we should change the
> > drivers/clk/Makefile to have obj-y for this Makefile instead of depending on
> > ARCH_MXC.
> 
> We also use ARCH_MXC for ARMv8 SoC clocks.

Yes, and? I'm suggesting drivers/clk/Makefile have an obj-y +=
drivers/clk/imx/ and then decide to compile or not compile the MXC_CLK
"core" bits based on CONFIG_MXC_CLK config option.
Aisheng Dong Dec. 12, 2018, 1:09 a.m. | #4
> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Quoting Aisheng Dong (2018-12-10 18:36:29)
> > > -----Original Message-----
> > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > > Sent: Tuesday, December 11, 2018 4:55 AM
> > [...]
> > > Subject: Re: [PATCH V9 1/7] clk: imx: add configuration option for
> > > mmio clks
> > >
> > > Quoting Aisheng DONG (2018-12-04 06:39:22)
> > > > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > > > index c12a05c..303082c 100644
> > > > --- a/arch/arm/mach-imx/Kconfig
> > > > +++ b/arch/arm/mach-imx/Kconfig
> > > > @@ -570,6 +580,7 @@ config SOC_IMX7ULP  config SOC_VF610
> > > >         bool "Vybrid Family VF610 support"
> > > >         select ARM_GIC if ARCH_MULTI_V7
> > > > +       select MXC_CLK
> > > >         select PINCTRL_VF610
> > > >
> > >
> > > Instead of select can we break this dependency on the arch Kconfig
> > > and
> > > have:
> > >
> > >       config MXC_CLK
> > >               def_bool ARCH_MXC && !ARM64
> > >
> >
> > Thanks for the suggestion.
> > One little problem is that we also have LS1021 supported under
> > ARCH_MXC which does not use MXC_CLK.
> 
> Ok. So then your change is also limiting the compilation of drivers/clk/imx/ to
> only the platforms that select it now, instead of all ARCH_MXC like it was done
> before.
> 
> We can also have def_bool <big list of SoC platforms> if that helps for the
> optimization that this patch is making.
> 
> arm-soc has generally pushed against having so many different arch level
> Kconfig options because they make for unwieldy Kconfig fragments that may
> become conflicting. Instead, the options for the different SoCs are removed
> and we just have one for the platform and rely on defconfigs to pick the right
> drivers.
> 

Thanks for telling the detailed background. Very clear now.

> Why can't we do that here?
> 
> >
> > > > 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) += \
> > >
> > > Because this changes to obj-$(CONFIG) we should change the
> > > drivers/clk/Makefile to have obj-y for this Makefile instead of
> > > depending on ARCH_MXC.
> >
> > We also use ARCH_MXC for ARMv8 SoC clocks.
> 
> Yes, and? I'm suggesting drivers/clk/Makefile have an obj-y += drivers/clk/imx/
> and then decide to compile or not compile the MXC_CLK "core" bits based on
> CONFIG_MXC_CLK config option.

Sorry for the missunderstanding before.
I guess you point is have an ojb-y += drivrs/clk/imx/
Then in drivers/clk/imx/Kconfig
config MXC_CLK
		def_bool ARCH_MXC && !ARM64
		select MXC_clk bits
config MXC_CLK_SCU
		def_bool ARCH_MXC && ARM64
		select SCU clock bits

If wrong please let me know.

Will update the patch series and re-send.
Thanks for the suggestion.

Regards
Dong Aisheng
Aisheng Dong Dec. 12, 2018, 10:27 a.m. | #5
Hi Stephen,

> > > > > 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) += \
> > > >
> > > > Because this changes to obj-$(CONFIG) we should change the
> > > > drivers/clk/Makefile to have obj-y for this Makefile instead of
> > > > depending on ARCH_MXC.
> > >
> > > We also use ARCH_MXC for ARMv8 SoC clocks.
> >
> > Yes, and? I'm suggesting drivers/clk/Makefile have an obj-y +=
> > drivers/clk/imx/ and then decide to compile or not compile the MXC_CLK
> > "core" bits based on CONFIG_MXC_CLK config option.
> 
> Sorry for the missunderstanding before.
> I guess you point is have an ojb-y += drivrs/clk/imx/ Then in
> drivers/clk/imx/Kconfig config MXC_CLK
> 		def_bool ARCH_MXC && !ARM64
> 		select MXC_clk bits
> config MXC_CLK_SCU
> 		def_bool ARCH_MXC && ARM64
> 		select SCU clock bits
> 
> If wrong please let me know.
> 
> Will update the patch series and re-send.
> Thanks for the suggestion.
> 

I tried and met another problem that MX8MQ (ARM64) is also
using legacy MXC_CLK. (And 3 more MX8M series based chips.)

That means I have to write something like:
config MXC_CLK
        bool
        def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ)

config MXC_CLK_SCU
        bool
        def_bool (ARCH_MXC && ARM64 && !SOC_IMX8MQ)

But it can't work as ARM64 supports multiplatforms.
e.g. we have also SOC_IMX8QXP, SOC_IMX8QM, SOC_IMX8DM ...

Do you have a suggestion about this?

Regards
Dong Aisheng

> Regards
> Dong Aisheng
Stephen Boyd Dec. 12, 2018, 10:23 p.m. | #6
Quoting Aisheng Dong (2018-12-12 02:27:24)
> Hi Stephen,
> 
> > > > > > 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) += \
> > > > >
> > > > > Because this changes to obj-$(CONFIG) we should change the
> > > > > drivers/clk/Makefile to have obj-y for this Makefile instead of
> > > > > depending on ARCH_MXC.
> > > >
> > > > We also use ARCH_MXC for ARMv8 SoC clocks.
> > >
> > > Yes, and? I'm suggesting drivers/clk/Makefile have an obj-y +=
> > > drivers/clk/imx/ and then decide to compile or not compile the MXC_CLK
> > > "core" bits based on CONFIG_MXC_CLK config option.
> > 
> > Sorry for the missunderstanding before.
> > I guess you point is have an ojb-y += drivrs/clk/imx/ Then in
> > drivers/clk/imx/Kconfig config MXC_CLK
> >               def_bool ARCH_MXC && !ARM64
> >               select MXC_clk bits
> > config MXC_CLK_SCU
> >               def_bool ARCH_MXC && ARM64
> >               select SCU clock bits
> > 
> > If wrong please let me know.
> > 
> > Will update the patch series and re-send.
> > Thanks for the suggestion.

Yes that looks like what I'm asking for.

> > 
> 
> I tried and met another problem that MX8MQ (ARM64) is also
> using legacy MXC_CLK. (And 3 more MX8M series based chips.)
> 
> That means I have to write something like:
> config MXC_CLK
>         bool
>         def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ)
> 
> config MXC_CLK_SCU
>         bool
>         def_bool (ARCH_MXC && ARM64 && !SOC_IMX8MQ)
> 
> But it can't work as ARM64 supports multiplatforms.
> e.g. we have also SOC_IMX8QXP, SOC_IMX8QM, SOC_IMX8DM ...
> 
> Do you have a suggestion about this?
> 

Can you put the enabling of MXC_CLK_SCU in the defconfig and exposed it
as a user selectable option? And keep hiding the MXC_CLK option behind
the def_bool? I hope that MXC_CLK could be user selectable eventually,
but the def_bool is there to make it easier to bridge the transition and
update everyone's defconfigs while it is moved into the defconfig.
Aisheng Dong Dec. 13, 2018, 12:32 a.m. | #7
> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: Thursday, December 13, 2018 6:24 AM
> 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
> Subject: RE: [PATCH V9 1/7] clk: imx: add configuration option for mmio clks
> 
> Quoting Aisheng Dong (2018-12-12 02:27:24)
> > Hi Stephen,
> >
> > > > > > > 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) += \
> > > > > >
> > > > > > Because this changes to obj-$(CONFIG) we should change the
> > > > > > drivers/clk/Makefile to have obj-y for this Makefile instead
> > > > > > of depending on ARCH_MXC.
> > > > >
> > > > > We also use ARCH_MXC for ARMv8 SoC clocks.
> > > >
> > > > Yes, and? I'm suggesting drivers/clk/Makefile have an obj-y +=
> > > > drivers/clk/imx/ and then decide to compile or not compile the
> > > > MXC_CLK "core" bits based on CONFIG_MXC_CLK config option.
> > >
> > > Sorry for the missunderstanding before.
> > > I guess you point is have an ojb-y += drivrs/clk/imx/ Then in
> > > drivers/clk/imx/Kconfig config MXC_CLK
> > >               def_bool ARCH_MXC && !ARM64
> > >               select MXC_clk bits
> > > config MXC_CLK_SCU
> > >               def_bool ARCH_MXC && ARM64
> > >               select SCU clock bits
> > >
> > > If wrong please let me know.
> > >
> > > Will update the patch series and re-send.
> > > Thanks for the suggestion.
> 
> Yes that looks like what I'm asking for.
> 
> > >
> >
> > I tried and met another problem that MX8MQ (ARM64) is also using
> > legacy MXC_CLK. (And 3 more MX8M series based chips.)
> >
> > That means I have to write something like:
> > config MXC_CLK
> >         bool
> >         def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 &&
> > SOC_IMX8MQ)
> >
> > config MXC_CLK_SCU
> >         bool
> >         def_bool (ARCH_MXC && ARM64 && !SOC_IMX8MQ)
> >
> > But it can't work as ARM64 supports multiplatforms.
> > e.g. we have also SOC_IMX8QXP, SOC_IMX8QM, SOC_IMX8DM ...
> >
> > Do you have a suggestion about this?
> >
> 
> Can you put the enabling of MXC_CLK_SCU in the defconfig and exposed it as a
> user selectable option? And keep hiding the MXC_CLK option behind the
> def_bool? I hope that MXC_CLK could be user selectable eventually, but the
> def_bool is there to make it easier to bridge the transition and update
> everyone's defconfigs while it is moved into the defconfig.

Sounds like a good suggestion.
I will do it now.

In order to avoid breaking MX8MQ, I will keep MXC_CLK as
config MXC_CLK
         bool
         def_bool (ARCH_MXC && !ARM64) || (ARCH_MXC && ARM64 && SOC_IMX8MQ)

and make MXC_CLK_SCU a user selectable option.

And finally we will make MXC_CLK selectable as well after the transition.

If any issue please let me know.

Regards
Dong Aisheng

Patch

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index c12a05c..303082c 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -57,23 +57,27 @@  config SOC_IMX21
 	select CPU_ARM926T
 	select IMX_HAVE_IOMUX_V1
 	select MXC_AVIC
+	select MXC_CLK
 
 config SOC_IMX27
 	bool
 	select CPU_ARM926T
 	select IMX_HAVE_IOMUX_V1
 	select MXC_AVIC
+	select MXC_CLK
 	select PINCTRL_IMX27
 
 config SOC_IMX31
 	bool
 	select CPU_V6
 	select MXC_AVIC
+	select MXC_CLK
 
 config SOC_IMX35
 	bool
 	select ARCH_MXC_IOMUX_V3
 	select MXC_AVIC
+	select MXC_CLK
 	select PINCTRL_IMX35
 
 if ARCH_MULTI_V5
@@ -417,6 +421,7 @@  config SOC_IMX1
 	bool "i.MX1 support"
 	select CPU_ARM920T
 	select MXC_AVIC
+	select MXC_CLK
 	select PINCTRL_IMX1
 	help
 	  This enables support for Freescale i.MX1 processor
@@ -430,6 +435,7 @@  config SOC_IMX25
 	select ARCH_MXC_IOMUX_V3
 	select CPU_ARM926T
 	select MXC_AVIC
+	select MXC_CLK
 	select PINCTRL_IMX25
 	help
 	  This enables support for Freescale i.MX25 processor
@@ -442,6 +448,7 @@  comment "Cortex-A platforms"
 config SOC_IMX5
 	bool
 	select HAVE_IMX_SRC
+	select MXC_CLK
 	select MXC_TZIC
 
 config	SOC_IMX50
@@ -478,6 +485,7 @@  config SOC_IMX6
 	select HAVE_IMX_MMDC
 	select HAVE_IMX_SRC
 	select MFD_SYSCON
+	select MXC_CLK
 	select PL310_ERRATA_769419 if CACHE_L2X0
 
 config SOC_IMX6Q
@@ -545,10 +553,12 @@  config SOC_IMX7D_CA7
 	select HAVE_IMX_MMDC
 	select HAVE_IMX_SRC
 	select IMX_GPCV2
+	select MXC_CLK
 
 config SOC_IMX7D_CM4
 	bool
 	select ARMV7M_SYSTICK
+	select MXC_CLK
 
 config SOC_IMX7D
 	bool "i.MX7 Dual support"
@@ -570,6 +580,7 @@  config SOC_IMX7ULP
 config SOC_VF610
 	bool "Vybrid Family VF610 support"
 	select ARM_GIC if ARCH_MULTI_V7
+	select MXC_CLK
 	select PINCTRL_VF610
 
 	help
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 81cdb4e..1dbfcc2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -287,6 +287,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/imx/Kconfig b/drivers/clk/imx/Kconfig
new file mode 100644
index 0000000..43a3ecc
--- /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
+	depends on ARCH_MXC
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 \