Re: [U-Boot] [PATCH v7 01/12] clk: Use dummy clk_get_by_* functions when CONFIG_CLK is disabled

2016-09-15 Thread Masahiro Yamada 
Hi Paul,

2016-09-12 0:14 GMT+09:00 Paul Burton :
> On 11/09/16 14:25, Masahiro Yamada wrote:
>> Hi Paul,
>>
>> 2016-09-09 17:01 GMT+09:00 Paul Burton :
>>> On 09/09/16 04:15, Masahiro Yamada wrote:
 2016-09-08 15:47 GMT+09:00 Paul Burton :
> 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.



-- 
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v7 01/12] clk: Use dummy clk_get_by_* functions when CONFIG_CLK is disabled

2016-09-12 Thread Stephen Warren 

On 09/11/2016 07:25 AM, Masahiro Yamada wrote:

Hi Paul,

2016-09-09 17:01 GMT+09:00 Paul Burton :

On 09/09/16 04:15, Masahiro Yamada wrote:

2016-09-08 15:47 GMT+09:00 Paul Burton :

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.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v7 01/12] clk: Use dummy clk_get_by_* functions when CONFIG_CLK is disabled

2016-09-11 Thread Paul Burton 
On 11/09/16 14:25, Masahiro Yamada wrote:
> Hi Paul,
> 
> 2016-09-09 17:01 GMT+09:00 Paul Burton :
>> On 09/09/16 04:15, Masahiro Yamada wrote:
>>> 2016-09-08 15:47 GMT+09:00 Paul Burton :
 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v7 01/12] clk: Use dummy clk_get_by_* functions when CONFIG_CLK is disabled

2016-09-11 Thread Masahiro Yamada 
Hi Paul,

2016-09-09 17:01 GMT+09:00 Paul Burton :
> On 09/09/16 04:15, Masahiro Yamada wrote:
>> 2016-09-08 15:47 GMT+09:00 Paul Burton :
>>> 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?



-- 
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v7 01/12] clk: Use dummy clk_get_by_* functions when CONFIG_CLK is disabled

2016-09-09 Thread Paul Burton 
On 09/09/16 04:15, Masahiro Yamada wrote:
> 2016-09-08 15:47 GMT+09:00 Paul Burton :
>> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v7 01/12] clk: Use dummy clk_get_by_* functions when CONFIG_CLK is disabled

2016-09-08 Thread Masahiro Yamada 
2016-09-08 15:47 GMT+09:00 Paul Burton :
> 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?




-- 
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot