[V3,04/10] clk: imx: Support building SCU clock driver as module

Commit Message

Anson Huang June 29, 2020, 5:53 a.m. UTC

There are more and more requirements of building SoC specific drivers
as modules, add support for building SCU clock driver as module to meet
the requirement.

Signed-off-by: Anson Huang <[e-mail]>
---
Changes since V2:
	- use separated line for each file which is included for build;
	- include linux/export.h where necessary.
---
 drivers/clk/imx/Kconfig        | 4 ++--
 drivers/clk/imx/Makefile       | 6 +++---
 drivers/clk/imx/clk-lpcg-scu.c | 2 ++
 drivers/clk/imx/clk-scu.c      | 5 +++++
 4 files changed, 12 insertions(+), 5 deletions(-)

Comments

Arnd Bergmann June 29, 2020, 11:37 a.m. UTC | #1

On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[e-mail]> wrote:

> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -21,9 +21,9 @@ obj-$(CONFIG_MXC_CLK) += \
>         clk-sscg-pll.o \
>         clk-pll14xx.o
>
> -obj-$(CONFIG_MXC_CLK_SCU) += \
> -       clk-scu.o \
> -       clk-lpcg-scu.o
> +mxc-clk-scu-objs += clk-lpcg-scu.o
> +mxc-clk-scu-objs += clk-scu.o
> +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o

It looks like the two modules are tightly connected, one is useless without the
other. How about linking them into a combined module and dropping the
export statement?

      Arnd

Anson Huang June 29, 2020, 12:53 p.m. UTC | #2

Hi, Arnd


> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[e-mail]>
> wrote:
> 
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -21,9 +21,9 @@ obj-$(CONFIG_MXC_CLK) += \
> >         clk-sscg-pll.o \
> >         clk-pll14xx.o
> >
> > -obj-$(CONFIG_MXC_CLK_SCU) += \
> > -       clk-scu.o \
> > -       clk-lpcg-scu.o
> > +mxc-clk-scu-objs += clk-lpcg-scu.o
> > +mxc-clk-scu-objs += clk-scu.o
> > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> 
> It looks like the two modules are tightly connected, one is useless without the
> other. How about linking them into a combined module and dropping the
> export statement?
> 

From HW perspective, the SCU clock driver and LPCG SCU clock driver are different,
SCU clock driver is for those clocks controlled by system controller (M4 which runs a firmware),
while LPCG SCU clock is for those clock gates inside module, which means AP core can
control it directly via register access, no need to via SCU API.

So, I think it is NOT that tightly connected, it is because they are both for i.MX8 SoCs with SCU
inside, so they are put together in the Makefile.

If the export statement is acceptable, I think it is better to just keep it, make sense?

Thanks,
Anson

Arnd Bergmann June 29, 2020, 1:20 p.m. UTC | #3

On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <[e-mail]> wrote:
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > module
> >
> > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[e-mail]>
> > wrote:
> >
> > > --- a/drivers/clk/imx/Makefile
> > > +++ b/drivers/clk/imx/Makefile
> > > @@ -21,9 +21,9 @@ obj-$(CONFIG_MXC_CLK) += \
> > >         clk-sscg-pll.o \
> > >         clk-pll14xx.o
> > >
> > > -obj-$(CONFIG_MXC_CLK_SCU) += \
> > > -       clk-scu.o \
> > > -       clk-lpcg-scu.o
> > > +mxc-clk-scu-objs += clk-lpcg-scu.o
> > > +mxc-clk-scu-objs += clk-scu.o
> > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> >
> > It looks like the two modules are tightly connected, one is useless without the
> > other. How about linking them into a combined module and dropping the
> > export statement?
> >
>
> From HW perspective, the SCU clock driver and LPCG SCU clock driver are different,
> SCU clock driver is for those clocks controlled by system controller (M4 which runs a firmware),
> while LPCG SCU clock is for those clock gates inside module, which means AP core can
> control it directly via register access, no need to via SCU API.

Sorry, I misread the patch in multiple ways. First of all, you already put
clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and
I had only looked at clk-scu.c.

What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
(and possibly future chip-specific files) into a loadable module and drop
the export.

> So, I think it is NOT that tightly connected, it is because they are both for i.MX8 SoCs with SCU
> inside, so they are put together in the Makefile.
>
> If the export statement is acceptable, I think it is better to just keep it, make sense?

There is nothing wrong with the export as such, this was just an
idea to simplify the logic.

      Arnd

Anson Huang June 29, 2020, 2:52 p.m. UTC | #4

Hi, Arnd

> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <[e-mail]>
> wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > driver as module
> > >
> > > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[e-mail]>
> > > wrote:
> > >
> > > > --- a/drivers/clk/imx/Makefile
> > > > +++ b/drivers/clk/imx/Makefile
> > > > @@ -21,9 +21,9 @@ obj-$(CONFIG_MXC_CLK) += \
> > > >         clk-sscg-pll.o \
> > > >         clk-pll14xx.o
> > > >
> > > > -obj-$(CONFIG_MXC_CLK_SCU) += \
> > > > -       clk-scu.o \
> > > > -       clk-lpcg-scu.o
> > > > +mxc-clk-scu-objs += clk-lpcg-scu.o mxc-clk-scu-objs += clk-scu.o
> > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > >
> > > It looks like the two modules are tightly connected, one is useless
> > > without the other. How about linking them into a combined module and
> > > dropping the export statement?
> > >
> >
> > From HW perspective, the SCU clock driver and LPCG SCU clock driver
> > are different, SCU clock driver is for those clocks controlled by
> > system controller (M4 which runs a firmware), while LPCG SCU clock is
> > for those clock gates inside module, which means AP core can control it
> directly via register access, no need to via SCU API.
> 
> Sorry, I misread the patch in multiple ways. First of all, you already put
> clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and I had
> only looked at clk-scu.c.
> 
> What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
> (and possibly future chip-specific files) into a loadable module and drop the
> export.

Sorry, could you please advise more details about how to do it in Makefile?
I tried below but it looks like NOT working. multiple definition of module_init() error reported.

obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o
clk-imx-y += clk-scu.o clk-lpcg-scu.o
clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o

Thanks,
Anson

Arnd Bergmann June 29, 2020, 3:08 p.m. UTC | #5

On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <[e-mail]> wrote:
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
> > On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <[e-mail]> wrote:
> >
> > Sorry, I misread the patch in multiple ways. First of all, you already put
> > clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and I had
> > only looked at clk-scu.c.
> >
> > What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
> > (and possibly future chip-specific files) into a loadable module and drop the
> > export.
>
> Sorry, could you please advise more details about how to do it in Makefile?
> I tried below but it looks like NOT working. multiple definition of module_init() error reported.
>
> obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o
> clk-imx-y += clk-scu.o clk-lpcg-scu.o
> clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o

Right, you can't have multiple module_init() in a module, so what I suggested
earlier won't work any more as soon as you add a second chip that uses the
clk-scu driver, and then you have to use separate modules, or some hack
that calls the init functions one at a time, which is probably worse.

If it's only imx8qxp, you can do it like this:

obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
clk-imx-scu-y         += clk-scu.o clk-imx8qxp.o
clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o

If you already know that the scu driver is going to be used in future
chips, then just stay with what you have now, using a separate module
per file, exporting the symbols as needed.

        Arnd

Anson Huang June 29, 2020, 3:19 p.m. UTC | #6

Hi, Arnd


> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <[e-mail]>
> wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > driver as module On Mon, Jun 29, 2020 at 2:53 PM Anson Huang
> <[e-mail]> wrote:
> > >
> > > Sorry, I misread the patch in multiple ways. First of all, you
> > > already put clk-scu.o and clk-lpcg-scu.o files into a combined
> > > loadable module, and I had only looked at clk-scu.c.
> > >
> > > What I actually meant here was to link clk-scu.o together with
> > > clk-imx8qxp.o (and possibly future chip-specific files) into a
> > > loadable module and drop the export.
> >
> > Sorry, could you please advise more details about how to do it in Makefile?
> > I tried below but it looks like NOT working. multiple definition of
> module_init() error reported.
> >
> > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o clk-imx-y += clk-scu.o
> > clk-lpcg-scu.o
> > clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> 
> Right, you can't have multiple module_init() in a module, so what I suggested
> earlier won't work any more as soon as you add a second chip that uses the
> clk-scu driver, and then you have to use separate modules, or some hack that
> calls the init functions one at a time, which is probably worse.
> 
> If it's only imx8qxp, you can do it like this:
> 
> obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
> clk-imx-scu-y         += clk-scu.o clk-imx8qxp.o
> clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o
> 
> If you already know that the scu driver is going to be used in future chips, then
> just stay with what you have now, using a separate module per file, exporting
> the symbols as needed.
> 

Thanks, and yes, I know that scu clk driver will be used for future i.MX8X chips with
SCU inside, the current i.MX8QXP clock driver can NOT cover all i.MX8X chips, so
I will stay with the exporting symbols.

Thanks,
Anson

Dong Aisheng June 30, 2020, 3:36 a.m. UTC | #7

On Tue, Jun 30, 2020 at 3:36 AM Arnd Bergmann <[e-mail]> wrote:
>
> On Mon, Jun 29, 2020 at 2:53 PM Anson Huang <[e-mail]> wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > > module
> > >
> > > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang <[e-mail]>
> > > wrote:
> > >
> > > > --- a/drivers/clk/imx/Makefile
> > > > +++ b/drivers/clk/imx/Makefile
> > > > @@ -21,9 +21,9 @@ obj-$(CONFIG_MXC_CLK) += \
> > > >         clk-sscg-pll.o \
> > > >         clk-pll14xx.o
> > > >
> > > > -obj-$(CONFIG_MXC_CLK_SCU) += \
> > > > -       clk-scu.o \
> > > > -       clk-lpcg-scu.o
> > > > +mxc-clk-scu-objs += clk-lpcg-scu.o
> > > > +mxc-clk-scu-objs += clk-scu.o
> > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > >
> > > It looks like the two modules are tightly connected, one is useless without the
> > > other. How about linking them into a combined module and dropping the
> > > export statement?
> > >
> >
> > From HW perspective, the SCU clock driver and LPCG SCU clock driver are different,
> > SCU clock driver is for those clocks controlled by system controller (M4 which runs a firmware),
> > while LPCG SCU clock is for those clock gates inside module, which means AP core can
> > control it directly via register access, no need to via SCU API.
>
> Sorry, I misread the patch in multiple ways. First of all, you already put
> clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and
> I had only looked at clk-scu.c.
>
> What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
> (and possibly future chip-specific files) into a loadable module and drop
> the export.

It sounds like a good idea to me.
Actually I planned to combine them into one driver in the future.

Regards
Aisheng

>
> > So, I think it is NOT that tightly connected, it is because they are both for i.MX8 SoCs with SCU
> > inside, so they are put together in the Makefile.
> >
> > If the export statement is acceptable, I think it is better to just keep it, make sense?
>
> There is nothing wrong with the export as such, this was just an
> idea to simplify the logic.
>
>       Arnd

Dong Aisheng June 30, 2020, 3:55 a.m. UTC | #8

On Tue, Jun 30, 2020 at 5:16 AM Anson Huang <[e-mail]> wrote:
>
> Hi, Arnd
>
>
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > module
> >
> > On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <[e-mail]>
> > wrote:
> > > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > > driver as module On Mon, Jun 29, 2020 at 2:53 PM Anson Huang
> > <[e-mail]> wrote:
> > > >
> > > > Sorry, I misread the patch in multiple ways. First of all, you
> > > > already put clk-scu.o and clk-lpcg-scu.o files into a combined
> > > > loadable module, and I had only looked at clk-scu.c.
> > > >
> > > > What I actually meant here was to link clk-scu.o together with
> > > > clk-imx8qxp.o (and possibly future chip-specific files) into a
> > > > loadable module and drop the export.
> > >
> > > Sorry, could you please advise more details about how to do it in Makefile?
> > > I tried below but it looks like NOT working. multiple definition of
> > module_init() error reported.
> > >
> > > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o clk-imx-y += clk-scu.o
> > > clk-lpcg-scu.o
> > > clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> >
> > Right, you can't have multiple module_init() in a module, so what I suggested
> > earlier won't work any more as soon as you add a second chip that uses the
> > clk-scu driver, and then you have to use separate modules, or some hack that
> > calls the init functions one at a time, which is probably worse.
> >
> > If it's only imx8qxp, you can do it like this:
> >
> > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
> > clk-imx-scu-y         += clk-scu.o clk-imx8qxp.o
> > clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o
> >
> > If you already know that the scu driver is going to be used in future chips, then
> > just stay with what you have now, using a separate module per file, exporting
> > the symbols as needed.
> >
>
> Thanks, and yes, I know that scu clk driver will be used for future i.MX8X chips with
> SCU inside, the current i.MX8QXP clock driver can NOT cover all i.MX8X chips, so
> I will stay with the exporting symbols.
>

SCU clock driver is a common driver for all SCU based platforms.
Current i.MX8QXP SCU clock driver will be extended to support all
future SCU based platforms.
So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG  is similar.
Maybe you can give a try as Arnd suggested.

Regards
Aisheng

> Thanks,
> Anson

Anson Huang July 1, 2020, 7:19 a.m. UTC | #9

Hi, Arnd


> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Tue, Jun 30, 2020 at 5:16 AM Anson Huang <[e-mail]>
> wrote:
> >
> > Hi, Arnd
> >
> >
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > driver as module
> > >
> > > On Mon, Jun 29, 2020 at 4:52 PM Anson Huang <[e-mail]>
> > > wrote:
> > > > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU
> > > > > clock driver as module On Mon, Jun 29, 2020 at 2:53 PM Anson
> > > > > Huang
> > > <[e-mail]> wrote:
> > > > >
> > > > > Sorry, I misread the patch in multiple ways. First of all, you
> > > > > already put clk-scu.o and clk-lpcg-scu.o files into a combined
> > > > > loadable module, and I had only looked at clk-scu.c.
> > > > >
> > > > > What I actually meant here was to link clk-scu.o together with
> > > > > clk-imx8qxp.o (and possibly future chip-specific files) into a
> > > > > loadable module and drop the export.
> > > >
> > > > Sorry, could you please advise more details about how to do it in
> Makefile?
> > > > I tried below but it looks like NOT working. multiple definition
> > > > of
> > > module_init() error reported.
> > > >
> > > > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o clk-imx-y += clk-scu.o
> > > > clk-lpcg-scu.o
> > > > clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> > >
> > > Right, you can't have multiple module_init() in a module, so what I
> > > suggested earlier won't work any more as soon as you add a second
> > > chip that uses the clk-scu driver, and then you have to use separate
> > > modules, or some hack that calls the init functions one at a time, which is
> probably worse.
> > >
> > > If it's only imx8qxp, you can do it like this:
> > >
> > > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
> > > clk-imx-scu-y         += clk-scu.o clk-imx8qxp.o
> > > clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o
> > >
> > > If you already know that the scu driver is going to be used in
> > > future chips, then just stay with what you have now, using a
> > > separate module per file, exporting the symbols as needed.
> > >
> >
> > Thanks, and yes, I know that scu clk driver will be used for future
> > i.MX8X chips with SCU inside, the current i.MX8QXP clock driver can
> > NOT cover all i.MX8X chips, so I will stay with the exporting symbols.
> >
> 
> SCU clock driver is a common driver for all SCU based platforms.
> Current i.MX8QXP SCU clock driver will be extended to support all future SCU
> based platforms.
> So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG  is
> similar.
> Maybe you can give a try as Arnd suggested.
> 

Do we really need to link clk-scu and i.MX8QXP clock driver together just to avoid some export?
I met some build issues if using this method, the i.MX8QXP module build is OK, but other platforms
like i.MX8MM/i.MX8MN/i.MX8MQ/i.MX8MP clock drivers are just skipped for build, even these
i.MX8M clock config are existing in .config, anyone know why? Looks like the change in Makefile for
i.MX8QXP clock driver introduce this issue.

I think doing such big change in order to save some exports looks like NOT that worth, is it acceptable
to just keep the original one in Makefile, just to have mxc-clk.ko, clk-scu.ko and clk-imx8qxp.ko etc..

thanks,
Anson

.config:
CONFIG_MXC_CLK=m
CONFIG_MXC_CLK_SCU=m
CONFIG_CLK_IMX8MM=m
CONFIG_CLK_IMX8MN=m
CONFIG_CLK_IMX8MP=m
CONFIG_CLK_IMX8MQ=m
CONFIG_CLK_IMX8QXP=m

Makefile:
mxc-clk-objs += clk-fixup-mux.o
mxc-clk-objs += clk-frac-pll.o
mxc-clk-objs += clk-gate2.o
mxc-clk-objs += clk-gate-exclusive.o
mxc-clk-objs += clk-pfd.o
mxc-clk-objs += clk-pfdv2.o
mxc-clk-objs += clk-pllv1.o
mxc-clk-objs += clk-pllv2.o
mxc-clk-objs += clk-pllv3.o
mxc-clk-objs += clk-pllv4.o
mxc-clk-objs += clk-pll14xx.o
mxc-clk-objs += clk-sscg-pll.o
obj-$(CONFIG_MXC_CLK) += mxc-clk.o

obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o

obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg-scu.o
clk-imx-scu-$(CONFIG_CLK_IMX8QXP) += clk-scu.o clk-imx8qxp.o
clk-imx-lpcg-scu-$(CONFIG_CLK_IMX8QXP) += clk-lpcg-scu.o clk-imx8qxp-lpcg.o

Arnd Bergmann July 1, 2020, 8:46 a.m. UTC | #10

On Wed, Jul 1, 2020 at 9:19 AM Anson Huang <[e-mail]> wrote:
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module
> > On Tue, Jun 30, 2020 at 5:16 AM Anson Huang <[e-mail]> wrote:
> >
> > SCU clock driver is a common driver for all SCU based platforms.
> > Current i.MX8QXP SCU clock driver will be extended to support all future SCU
> > based platforms.
> > So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG  is
> > similar.
> > Maybe you can give a try as Arnd suggested.
> >
>
> Do we really need to link clk-scu and i.MX8QXP clock driver together just to avoid some export?

It was just meant to be easier than exporting a symbol and dealing with module
dependencies. If it's not easier, then don't.

> I met some build issues if using this method, the i.MX8QXP module build is OK, but other platforms
> like i.MX8MM/i.MX8MN/i.MX8MQ/i.MX8MP clock drivers are just skipped for build, even these
> i.MX8M clock config are existing in .config, anyone know why? Looks like the change in Makefile for
> i.MX8QXP clock driver introduce this issue.

You have a ":=" instead of "+=" typo, so all earlier "+=" are ignored:

> obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
>
> obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg-scu.o

                                   ^^^^^^^^

      Arnd

Anson Huang July 1, 2020, 9:28 a.m. UTC | #11

Hi, Arnd

> Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> On Wed, Jul 1, 2020 at 9:19 AM Anson Huang <[e-mail]>
> wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > driver as module On Tue, Jun 30, 2020 at 5:16 AM Anson Huang
> <[e-mail]> wrote:
> > >
> > > SCU clock driver is a common driver for all SCU based platforms.
> > > Current i.MX8QXP SCU clock driver will be extended to support all
> > > future SCU based platforms.
> > > So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG
> > > is similar.
> > > Maybe you can give a try as Arnd suggested.
> > >
> >
> > Do we really need to link clk-scu and i.MX8QXP clock driver together just to
> avoid some export?
> 
> It was just meant to be easier than exporting a symbol and dealing with
> module dependencies. If it's not easier, then don't.
> 
> > I met some build issues if using this method, the i.MX8QXP module
> > build is OK, but other platforms like
> i.MX8MM/i.MX8MN/i.MX8MQ/i.MX8MP
> > clock drivers are just skipped for build, even these i.MX8M clock
> > config are existing in .config, anyone know why? Looks like the change in
> Makefile for i.MX8QXP clock driver introduce this issue.
> 
> You have a ":=" instead of "+=" typo, so all earlier "+=" are ignored:
> 
> > obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> > obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> > obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> > obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> >
> > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg-scu.o
> 
>                                    ^^^^^^^^

Thanks, I will give another try, I will make the common clk part all linked into each
platform's clock driver, then many exports can be saved.

Thanks,
Anson

Anson Huang July 1, 2020, 9:40 a.m. UTC | #12

Hi, Arnd

> Subject: RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> module
> 
> Hi, Arnd
> 
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > driver as module
> >
> > On Wed, Jul 1, 2020 at 9:19 AM Anson Huang <[e-mail]>
> > wrote:
> > > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > > driver as module On Tue, Jun 30, 2020 at 5:16 AM Anson Huang
> > <[e-mail]> wrote:
> > > >
> > > > SCU clock driver is a common driver for all SCU based platforms.
> > > > Current i.MX8QXP SCU clock driver will be extended to support all
> > > > future SCU based platforms.
> > > > So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG
> > > > is similar.
> > > > Maybe you can give a try as Arnd suggested.
> > > >
> > >
> > > Do we really need to link clk-scu and i.MX8QXP clock driver together
> > > just to
> > avoid some export?
> >
> > It was just meant to be easier than exporting a symbol and dealing
> > with module dependencies. If it's not easier, then don't.
> >
> > > I met some build issues if using this method, the i.MX8QXP module
> > > build is OK, but other platforms like
> > i.MX8MM/i.MX8MN/i.MX8MQ/i.MX8MP
> > > clock drivers are just skipped for build, even these i.MX8M clock
> > > config are existing in .config, anyone know why? Looks like the
> > > change in
> > Makefile for i.MX8QXP clock driver introduce this issue.
> >
> > You have a ":=" instead of "+=" typo, so all earlier "+=" are ignored:
> >
> > > obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> > > obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> > > obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> > > obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> > >
> > > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg-scu.o
> >
> >                                    ^^^^^^^^
> 
> Thanks, I will give another try, I will make the common clk part all linked into
> each platform's clock driver, then many exports can be saved.

Just tried, it works for i.MX8QXP, andcorrect one thing, other platforms
need more complicated change if want to support them in the same way, so I
plan to ONLY do this change for i.MX8QXP if it is acceptable.

Thanks,
Anson

Arnd Bergmann July 1, 2020, 9:54 a.m. UTC | #13

On Wed, Jul 1, 2020 at 11:40 AM Anson Huang <[e-mail]> wrote:
> > Subject: RE: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > module
> >
> >
> > Thanks, I will give another try, I will make the common clk part all linked into
> > each platform's clock driver, then many exports can be saved.
>
> Just tried, it works for i.MX8QXP, andcorrect one thing, other platforms
> need more complicated change if want to support them in the same way, so I
> plan to ONLY do this change for i.MX8QXP if it is acceptable.

Yes, that is what I expected anyway.

      Arnd

Patch

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index db0253f..ded0643 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -5,8 +5,8 @@  config MXC_CLK
 	def_bool ARCH_MXC
 
 config MXC_CLK_SCU
-	bool
-	depends on IMX_SCU
+	tristate "IMX SCU clock"
+	depends on ARCH_MXC && IMX_SCU
 
 config CLK_IMX8MM
 	bool "IMX8MM CCM Clock Driver"
diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index 928f874..c6574a3 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -21,9 +21,9 @@  obj-$(CONFIG_MXC_CLK) += \
 	clk-sscg-pll.o \
 	clk-pll14xx.o
 
-obj-$(CONFIG_MXC_CLK_SCU) += \
-	clk-scu.o \
-	clk-lpcg-scu.o
+mxc-clk-scu-objs += clk-lpcg-scu.o
+mxc-clk-scu-objs += clk-scu.o
+obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
 
 obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
 obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
diff --git a/drivers/clk/imx/clk-lpcg-scu.c b/drivers/clk/imx/clk-lpcg-scu.c
index a73a799..d4e78ca 100644
--- a/drivers/clk/imx/clk-lpcg-scu.c
+++ b/drivers/clk/imx/clk-lpcg-scu.c
@@ -6,6 +6,7 @@ 
 
 #include <linux/clk-provider.h>
 #include <linux/err.h>
+#include <linux/export.h>
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -114,3 +115,4 @@  struct clk_hw *imx_clk_lpcg_scu(const char *name, const char *parent_name,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(imx_clk_lpcg_scu);
diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index b8b2072..9688981 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c
@@ -8,6 +8,7 @@ 
 #include <linux/arm-smccc.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 
 #include "clk-scu.h"
@@ -132,6 +133,7 @@  int imx_clk_scu_init(void)
 {
 	return imx_scu_get_handle(&ccm_ipc_handle);
 }
+EXPORT_SYMBOL_GPL(imx_clk_scu_init);
 
 /*
  * clk_scu_recalc_rate - Get clock rate for a SCU clock
@@ -387,3 +389,6 @@  struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(__imx_clk_scu);
+
+MODULE_LICENSE("GPL v2");