linux-kernel - Re: CONFIG_COMMON_CLK vs CONFIG_HAVE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87wob04z2g.wl-kuninori.morimoto.gx@renesas.com>
Date:   13 Dec 2019 13:41:27 +0900
From:   Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
To:     Stephen Boyd <sboyd@...nel.org>
Cc:     "Enrico Weigelt, metux IT consult" <lkml@...ux.net>,
        Michael Turquette <mturquette@...libre.com>,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: CONFIG_COMMON_CLK vs CONFIG_HAVE_CLK


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