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

Commit Message

Aisheng Dong Dec. 13, 2018, 1:07 a.m. UTC

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 <[e-mail]>
Cc: Sascha Hauer <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Michael Turquette <[e-mail]>
Signed-off-by: Dong Aisheng <[e-mail]>
---
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

Comments

Stephen Boyd Dec. 13, 2018, 7:57 a.m. UTC | #1

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 <[e-mail]>
> Cc: Sascha Hauer <[e-mail]>
> Cc: Fabio Estevam <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Michael Turquette <[e-mail]>
> Signed-off-by: Dong Aisheng <[e-mail]>
> 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 \

Olof Johansson Dec. 13, 2018, 8:02 a.m. UTC | #2

On Thu, Dec 13, 2018 at 3:57 PM Stephen Boyd <[e-mail]> 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 <[e-mail]>
> > Cc: Sascha Hauer <[e-mail]>
> > Cc: Fabio Estevam <[e-mail]>
> > Cc: Stephen Boyd <[e-mail]>
> > Cc: Michael Turquette <[e-mail]>
> > Signed-off-by: Dong Aisheng <[e-mail]>
> > 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

Shawn Guo Dec. 13, 2018, 8:12 a.m. UTC | #3

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

Abel Vesa Dec. 13, 2018, 12:04 p.m. UTC | #4

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

Aisheng Dong Dec. 13, 2018, 12:49 p.m. UTC | #5

> -----Original Message-----
> From: Stephen Boyd [mailto:[e-mail]]
> Sent: Thursday, December 13, 2018 3:58 PM
> To: [e-mail]; Aisheng Dong <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Fabio Estevam <[e-mail]>; dl-linux-imx
> <[e-mail]>; [e-mail]; Aisheng Dong
> <[e-mail]>; Arnd Bergmann <[e-mail]>; Olof Johansson
> <[e-mail]>
> 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 <[e-mail]>
> > Cc: Sascha Hauer <[e-mail]>
> > Cc: Fabio Estevam <[e-mail]>
> > Cc: Stephen Boyd <[e-mail]>
> > Cc: Michael Turquette <[e-mail]>
> > Signed-off-by: Dong Aisheng <[e-mail]> 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 \

Stephen Boyd Dec. 14, 2018, 1:32 a.m. UTC | #6

Quoting Aisheng Dong (2018-12-13 04:49:07)
> > -----Original Message-----
> > From: Stephen Boyd [mailto:[e-mail]]
> > Sent: Thursday, December 13, 2018 3:58 PM
> > To: [e-mail]; Aisheng Dong <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; Fabio Estevam <[e-mail]>; dl-linux-imx
> > <[e-mail]>; [e-mail]; Aisheng Dong
> > <[e-mail]>; Arnd Bergmann <[e-mail]>; Olof Johansson
> > <[e-mail]>
> > 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 <[e-mail]>
> > > Cc: Sascha Hauer <[e-mail]>
> > > Cc: Fabio Estevam <[e-mail]>
> > > Cc: Stephen Boyd <[e-mail]>
> > > Cc: Michael Turquette <[e-mail]>
> > > Signed-off-by: Dong Aisheng <[e-mail]> 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.

Olof Johansson Dec. 14, 2018, 1:36 a.m. UTC | #7

On Thu, Dec 13, 2018 at 4:13 PM Shawn Guo <[e-mail]> 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

Aisheng Dong Dec. 14, 2018, 1:50 a.m. UTC | #8

[...]
> >
> > 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

Patch

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 \