CONFIG_COMMON_CLK vs CONFIG_HAVE_CLK

CONFIG_COMMON_CLK vs CONFIG_HAVE_CLK

[Kuninori Morimoto]

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

Re: CONFIG_COMMON_CLK vs CONFIG_HAVE_CLK

[Enrico Weigelt, metux IT consult]

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
info@metux.net -- +49-151-27565287

Re: CONFIG_COMMON_CLK vs CONFIG_HAVE_CLK

[Stephen Boyd]

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.

Re: CONFIG_COMMON_CLK vs CONFIG_HAVE_CLK

[Kuninori Morimoto]

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