| Message ID | 20160908064739.25313-2-paul.burton@imgtec.com |
|---|---|
| State | Accepted |
| Delegated to: | Daniel Schwierzeck |
| Headers | show |
2016-09-08 15:47 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: > The implementations of clk_get_by_index & clk_get_by_name are only > available when CONFIG_CLK is enabled. Unless I am missing something, I think this statement also applies to other clk API functions such as clk_request(), clk_free(), clk_get_rate(), etc. > Provide the dummies when this is > not the case in order to avoid build failures. Why are other functions OK without dummy stubs?
On 09/09/16 04:15, Masahiro Yamada wrote: > 2016-09-08 15:47 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: >> The implementations of clk_get_by_index & clk_get_by_name are only >> available when CONFIG_CLK is enabled. > > Unless I am missing something, > I think this statement also applies to other clk API functions > such as clk_request(), clk_free(), clk_get_rate(), etc. Hi Masahiro, Yes, I agree. To be clear though, this patch doesn't add any new stub functions it simply makes the conditions for the existing ones being provided match the conditions for the real implementations not being provided. >> Provide the dummies when this is >> not the case in order to avoid build failures. > > Why are other functions OK without dummy stubs? In general, I presume because they aren't used. In the specific case I'm using clk_get_by_index for (drivers/serial/ns16550.c in patch 2 of this series) the fact that the dummy clk_get_by_index always returns an error will cause the compiler to optimise out a call to clk_get_rate so any dummy implementation provided for it wouldn't really get used. Thanks, Paul
Hi Paul, 2016-09-09 17:01 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: > On 09/09/16 04:15, Masahiro Yamada wrote: >> 2016-09-08 15:47 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: >>> The implementations of clk_get_by_index & clk_get_by_name are only >>> available when CONFIG_CLK is enabled. >> >> Unless I am missing something, >> I think this statement also applies to other clk API functions >> such as clk_request(), clk_free(), clk_get_rate(), etc. > > Hi Masahiro, > > Yes, I agree. To be clear though, this patch doesn't add any new stub > functions it simply makes the conditions for the existing ones being > provided match the conditions for the real implementations not being > provided. > >>> Provide the dummies when this is >>> not the case in order to avoid build failures. >> >> Why are other functions OK without dummy stubs? > > In general, I presume because they aren't used. > > In the specific case I'm using clk_get_by_index for > (drivers/serial/ns16550.c in patch 2 of this series) the fact that the > dummy clk_get_by_index always returns an error will cause the compiler > to optimise out a call to clk_get_rate so any dummy implementation > provided for it wouldn't really get used. I see, but I do not think it is a good idea to rely on the optimization by compiler in this case. Could you add stubs for other APIs, please?
On 11/09/16 14:25, Masahiro Yamada wrote: > Hi Paul, > > 2016-09-09 17:01 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: >> On 09/09/16 04:15, Masahiro Yamada wrote: >>> 2016-09-08 15:47 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: >>>> The implementations of clk_get_by_index & clk_get_by_name are only >>>> available when CONFIG_CLK is enabled. >>> >>> Unless I am missing something, >>> I think this statement also applies to other clk API functions >>> such as clk_request(), clk_free(), clk_get_rate(), etc. >> >> Hi Masahiro, >> >> Yes, I agree. To be clear though, this patch doesn't add any new stub >> functions it simply makes the conditions for the existing ones being >> provided match the conditions for the real implementations not being >> provided. >> >>>> Provide the dummies when this is >>>> not the case in order to avoid build failures. >>> >>> Why are other functions OK without dummy stubs? >> >> In general, I presume because they aren't used. >> >> In the specific case I'm using clk_get_by_index for >> (drivers/serial/ns16550.c in patch 2 of this series) the fact that the >> dummy clk_get_by_index always returns an error will cause the compiler >> to optimise out a call to clk_get_rate so any dummy implementation >> provided for it wouldn't really get used. > > I see, but I do not think it is a good idea > to rely on the optimization by compiler in this case. > > Could you add stubs for other APIs, please? Hi Masahiro, I don't mind doing that if it's deemed the right approach, but I see it as very much a separate change to this patch which fixes the condition for providing the existing stubs so would like if we can consider it separately. As to whether providing stubs for the other functions is correct - I'm not so sure. Right now, code that makes use of the clock functions but can also function without CONFIG_CLK will either: - Correctly check for errors from the clk_get_* functions and see calls to the other clock functions get optimised out. This is what happens in the ns16550 code after my patch 2 of this series, and it is also the case for other code already in U-Boot (eg. drivers/usb/host/ehci-generic.c). - Get it wrong, not check for errors from clk_get_* properly & get a fairly easy to track down link error when building U-Boot. If we add stub functions for the rest of the clk_* functions then that second case would become a runtime error rather than a link time one, and could be missed or overlooked much more easily. If we were to make the stubs do something like the Linux BUILD_BUG_ON macro then that would solve that problem, but would require all users of clk_ functions to #ifdef on CONFIG_CLK. So I actually think the current situation isn't so bad. Simon: as author of this clock code, what was your intent here? Thanks, Paul
On 09/11/2016 07:25 AM, Masahiro Yamada wrote: > Hi Paul, > > 2016-09-09 17:01 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: >> On 09/09/16 04:15, Masahiro Yamada wrote: >>> 2016-09-08 15:47 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: >>>> The implementations of clk_get_by_index & clk_get_by_name are only >>>> available when CONFIG_CLK is enabled. >>> >>> Unless I am missing something, >>> I think this statement also applies to other clk API functions >>> such as clk_request(), clk_free(), clk_get_rate(), etc. >> >> Hi Masahiro, >> >> Yes, I agree. To be clear though, this patch doesn't add any new stub >> functions it simply makes the conditions for the existing ones being >> provided match the conditions for the real implementations not being >> provided. >> >>>> Provide the dummies when this is >>>> not the case in order to avoid build failures. >>> >>> Why are other functions OK without dummy stubs? >> >> In general, I presume because they aren't used. >> >> In the specific case I'm using clk_get_by_index for >> (drivers/serial/ns16550.c in patch 2 of this series) the fact that the >> dummy clk_get_by_index always returns an error will cause the compiler >> to optimise out a call to clk_get_rate so any dummy implementation >> provided for it wouldn't really get used. > > I see, but I do not think it is a good idea > to rely on the optimization by compiler in this case. FWIW, I'm pretty sure that the Linux kernel relies on exactly the same technique with success.
Hi Paul, 2016-09-12 0:14 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: > On 11/09/16 14:25, Masahiro Yamada wrote: >> Hi Paul, >> >> 2016-09-09 17:01 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: >>> On 09/09/16 04:15, Masahiro Yamada wrote: >>>> 2016-09-08 15:47 GMT+09:00 Paul Burton <paul.burton@imgtec.com>: >>>>> The implementations of clk_get_by_index & clk_get_by_name are only >>>>> available when CONFIG_CLK is enabled. >>>> >>>> Unless I am missing something, >>>> I think this statement also applies to other clk API functions >>>> such as clk_request(), clk_free(), clk_get_rate(), etc. >>> >>> Hi Masahiro, >>> >>> Yes, I agree. To be clear though, this patch doesn't add any new stub >>> functions it simply makes the conditions for the existing ones being >>> provided match the conditions for the real implementations not being >>> provided. >>> >>>>> Provide the dummies when this is >>>>> not the case in order to avoid build failures. >>>> >>>> Why are other functions OK without dummy stubs? >>> >>> In general, I presume because they aren't used. >>> >>> In the specific case I'm using clk_get_by_index for >>> (drivers/serial/ns16550.c in patch 2 of this series) the fact that the >>> dummy clk_get_by_index always returns an error will cause the compiler >>> to optimise out a call to clk_get_rate so any dummy implementation >>> provided for it wouldn't really get used. >> >> I see, but I do not think it is a good idea >> to rely on the optimization by compiler in this case. >> >> Could you add stubs for other APIs, please? > > Hi Masahiro, > > I don't mind doing that if it's deemed the right approach, but I see it > as very much a separate change to this patch which fixes the condition > for providing the existing stubs so would like if we can consider it > separately. > > As to whether providing stubs for the other functions is correct - I'm > not so sure. Right now, code that makes use of the clock functions but > can also function without CONFIG_CLK will either: > > - Correctly check for errors from the clk_get_* functions and see > calls to the other clock functions get optimised out. This is what > happens in the ns16550 code after my patch 2 of this series, and it is > also the case for other code already in U-Boot (eg. > drivers/usb/host/ehci-generic.c). > > - Get it wrong, not check for errors from clk_get_* properly & get a > fairly easy to track down link error when building U-Boot. > > If we add stub functions for the rest of the clk_* functions then that > second case would become a runtime error rather than a link time one, > and could be missed or overlooked much more easily. If we were to make > the stubs do something like the Linux BUILD_BUG_ON macro then that would > solve that problem, but would require all users of clk_ functions to > #ifdef on CONFIG_CLK. So I actually think the current situation isn't so > bad. OK, thanks for explaining this. (For the case, perhaps drivers that need clk can have "depends on CLK" in Kconfig. Anyway, this should be considered separately.) So, let's go this series as it is.
diff --git a/include/clk.h b/include/clk.h index dc18b03..9b24522 100644 --- a/include/clk.h +++ b/include/clk.h @@ -59,7 +59,7 @@ struct clk { unsigned long id; }; -#if CONFIG_IS_ENABLED(OF_CONTROL) +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK) struct phandle_2_cell; int clk_get_by_index_platdata(struct udevice *dev, int index, struct phandle_2_cell *cells, struct clk *clk);