Hi
I noticed that there are some CONFIG_HAVE_CLK vs CONFIG_COMMON_CLK mismatch.
Because of it, I got compile error at clk_set_min_rate() on SH.
SH will have HAVE_CLK, but doesn't have COMMON_CLK.
> ARCH=sh make allyesconfig
> make
...
drivers/devfreq/tegra30-devfreq.o: In function `tegra_devfreq_target':
tegra30-devfreq.c:(.text+0x368): undefined reference to `clk_set_min_rate'
clk_set_min_rate() is under HAVE_CLK at clk.h
--- clk.h ---
=> #ifdef CONFIG_HAVE_CLK
...
int clk_set_min_rate(struct clk *clk, unsigned long rate);
...
#else /* !CONFIG_HAVE_CLK */
static inline int clk_set_min_rate(struct clk *clk, unsigned long rate)
...
-------------
It is implemented at clk.c.
But it will be compiled via COMMON_CLK
--- Makefile ---
...
=> obj-$(CONFIG_COMMON_CLK) += clk.o
...
----------------
Thank you for your help !!
Best regards
---
Kuninori Morimoto
On 12.12.19 03:09, Kuninori Morimoto wrote:
> I noticed that there are some CONFIG_HAVE_CLK vs CONFIG_COMMON_CLK mismatch.
> Because of it, I got compile error at clk_set_min_rate() on SH.
> SH will have HAVE_CLK, but doesn't have COMMON_CLK.
>
> > ARCH=sh make allyesconfig
> > make
> ...
> drivers/devfreq/tegra30-devfreq.o: In function `tegra_devfreq_target':
> tegra30-devfreq.c:(.text+0x368): undefined reference to `clk_set_min_rate'
>
> clk_set_min_rate() is under HAVE_CLK at clk.h
>
> --- clk.h ---
> => #ifdef CONFIG_HAVE_CLK
> ...
> int clk_set_min_rate(struct clk *clk, unsigned long rate);
> ...
> #else /* !CONFIG_HAVE_CLK */
> static inline int clk_set_min_rate(struct clk *clk, unsigned long rate)
> ...
> -------------
>
> It is implemented at clk.c.
> But it will be compiled via COMMON_CLK
>
> --- Makefile ---
> ...
> => obj-$(CONFIG_COMMON_CLK) += clk.o
You've got CONFIG_HAVE_CLK enabled, but CONFIG_COMMON_CLK disabled ?
hmm, the whole CONFIG_HAVE_CLK looks a bit weird to me. I wonder what's
the actual purpose of having this arch-specific.
IMHO, we should sort out whether there are some things that some arch
really *needs*, and what could be optional - then split that into
separate modules along this line.
It seems that clk_set_min_rate() belongs to CONFIG_COMMON_CLK, and
tegra30-devfreq.c needds to depend on CONFIG_COMMON_CLK.
--mtx
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
Quoting Enrico Weigelt, metux IT consult (2019-12-12 02:57:14)
> On 12.12.19 03:09, Kuninori Morimoto wrote:
>
> > I noticed that there are some CONFIG_HAVE_CLK vs CONFIG_COMMON_CLK mismatch.
> > Because of it, I got compile error at clk_set_min_rate() on SH.
> > SH will have HAVE_CLK, but doesn't have COMMON_CLK.
> >
> > > ARCH=sh make allyesconfig
> > > make
> > ...
> > drivers/devfreq/tegra30-devfreq.o: In function `tegra_devfreq_target':
> > tegra30-devfreq.c:(.text+0x368): undefined reference to `clk_set_min_rate'
> >
> > clk_set_min_rate() is under HAVE_CLK at clk.h
> >
> > --- clk.h ---
> > => #ifdef CONFIG_HAVE_CLK
> > ...
> > int clk_set_min_rate(struct clk *clk, unsigned long rate);
> > ...
> > #else /* !CONFIG_HAVE_CLK */
> > static inline int clk_set_min_rate(struct clk *clk, unsigned long rate)
> > ...
> > -------------
> >
> > It is implemented at clk.c.
> > But it will be compiled via COMMON_CLK
> >
> > --- Makefile ---
> > ...
> > => obj-$(CONFIG_COMMON_CLK) += clk.o
>
> You've got CONFIG_HAVE_CLK enabled, but CONFIG_COMMON_CLK disabled ?
>
> hmm, the whole CONFIG_HAVE_CLK looks a bit weird to me. I wonder what's
> the actual purpose of having this arch-specific.
>
> IMHO, we should sort out whether there are some things that some arch
> really *needs*, and what could be optional - then split that into
> separate modules along this line.
>
> It seems that clk_set_min_rate() belongs to CONFIG_COMMON_CLK, and
> tegra30-devfreq.c needds to depend on CONFIG_COMMON_CLK.
>
Years ago there wasn't a common clk framework. Just CONFIG_HAVE_CLK and
architectures implementing the API defined in the clk.h header file.
Then the common clk framework was created and we got CONFIG_COMMON_CLK.
When new clk API features are added to the common clk framework, we
typically limit their implementation and scope to CONFIG_COMMON_CLK so
that architectures are encouraged to migrate to the common clk
framework. I'm not really tracking the other implementations of the clk
API, but I thought we were down to a handful of implementations that
haven't migrated. I suppose SH is one of the big ones.
Hi
> > > --- clk.h ---
> > > => #ifdef CONFIG_HAVE_CLK
> > > ...
> > > int clk_set_min_rate(struct clk *clk, unsigned long rate);
> > > ...
> > > #else /* !CONFIG_HAVE_CLK */
> > > static inline int clk_set_min_rate(struct clk *clk, unsigned long rate)
> > > ...
> > > -------------
(snip)
> > > --- Makefile ---
> > > ...
> > > => obj-$(CONFIG_COMMON_CLK) += clk.o
> >
> > You've got CONFIG_HAVE_CLK enabled, but CONFIG_COMMON_CLK disabled ?
> >
> > hmm, the whole CONFIG_HAVE_CLK looks a bit weird to me. I wonder what's
> > the actual purpose of having this arch-specific.
> >
> > IMHO, we should sort out whether there are some things that some arch
> > really *needs*, and what could be optional - then split that into
> > separate modules along this line.
> >
> > It seems that clk_set_min_rate() belongs to CONFIG_COMMON_CLK, and
> > tegra30-devfreq.c needds to depend on CONFIG_COMMON_CLK.
> >
>
> Years ago there wasn't a common clk framework. Just CONFIG_HAVE_CLK and
> architectures implementing the API defined in the clk.h header file.
> Then the common clk framework was created and we got CONFIG_COMMON_CLK.
> When new clk API features are added to the common clk framework, we
> typically limit their implementation and scope to CONFIG_COMMON_CLK so
> that architectures are encouraged to migrate to the common clk
> framework. I'm not really tracking the other implementations of the clk
> API, but I thought we were down to a handful of implementations that
> haven't migrated. I suppose SH is one of the big ones.
I investigated about SH / HAVE_CLK / COMMON_CLK.
In clk.h, some functions are defined under HAVE_CLK.
For example clk_enable().
--- include/linux/clk.h ---
...
=> #ifdef CONFIG_HAVE_CLK
...
int clk_set_rate(struct clk *clk, unsigned long rate);
...
---------------------------
But, it is implementated under COMMON_CLK.
--- drivers/clk/clk.c ---
...
int clk_set_rate(struct clk *clk, unsigned long rate)
...
--- drivers/clk/Makefiles ---
...
=> obj-$(CONFIG_COMMON_CLK) += clk.o
...
-----------------------------
OTOH, SH has HAVE_CLK, but not have COMMON_CLK.
And, it has own clock implementation at drivers/sh/clk/core.c.
--- drivers/sh/clk/core.c ---
...
int clk_set_rate(struct clk *clk, unsigned long rate)
...
--- drivers/sh/clk/Makefile ---
...
=> obj-y := core.o
...
-------------------------------
These mean, HAVE_CLK vs COMMON_CLK mismatch under clk.h
is very matching to SH own clock.
In other words, if we correct clk.h HAVE_CLK vs COMMON_CLK,
It breaks SH clk.
It is very confusable for me.
But difficult to solve it.
So far, I will add "depends on COMMON_CLK" to driver side.
Thank you for your help !!
Best regards
---
Kuninori Morimoto