lkml.org 
[lkml]   [2017]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 4/5] ARM: davinci: convert to common clock framework
From
Date
On 12/19/2017 07:47 AM, Sekhar Nori wrote:
> Hi David,
>
> On Saturday 09 December 2017 07:45 AM, David Lechner wrote:
>> This converts the clocks in mach-davinci to the common clock framework.
>>
>> Most of the patch just involves renaming struct clk to struct davinci_clk.
>> There is also a struct clk_hw added to provide the bridge between the
>> existing clock implementation and the common clock framework.
>>
>> The clk_get_parent and clk_set_parent callbacks are dropped because all
>> clocks currently (effectivly) have a single parent, in which case the
>> common clock framework does not want you to implement these functions
>> yourself.
>>
>> clk_unregister() is dropped because it is not used anywhere in
>> mach-davinci.
>>
>> EXPORT_SYMBOL() is removed from functions not used outside of mach-davinci.
>>
>> Fixed checkpatch.pl warning about bare use of unsigned in dump_clock().
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>
> The cleanups leading upto this patch look fine, but I am not sure about
> this patch itself. Ideally, we should have moved to drivers/clk also.
> And shared code with keystone too since the PSC and PLL implementations
> of the two architectures are quite similar.
>
> I could think of this as an intermediate step while we get there, but I
> am afraid of the churn that would cause. For example, if we reuse
> keystone driver, we will be using clk_psc and we can get rid of the
> davinci_clk that this patch introduces.
>
> So unless there is big roadblock to moving to drivers/clk, we should
> probably do that in one shot.
>

Yes, this is intended as an intermediate step. My thinking was that it
would be better to do one thing at a time, so if something breaks, it
will be easier to find the problem than if we try to do everything at
once. But, you are right, there will be quite a few extra intermediate
steps required, for example, I have realized that in order to use the
davinci_clk concurrently with other clocks in the common clock
framework, at least the parent pointer and children linked list will
have to be removed, introducing another intermediate step.

While avoiding the davinci_clk step would be ideal, there is quite a bit
that needs to be done first before this will be possible. And the
downside I see to doing it this way is that no one will be able to test
any of the preparatory patches because they depend on the common clock
framework. We won't even be able to compile them until all the pieces
are in place and we can enable the common clock kernel configuration option.

Here is a list of some of the issues I know about so far preventing me
from doing everything at once at this point in time:

* Reentrancy of clk_enable() on non-SMP systems is broken in the common
clock framework [1][2], but this reentrancy is needed for the DA8xx USB
PHY PLL clock [3].

* drivers/remoteproc/da8xx_remoteproc.c calls
davinci_clk_reset_{assert,deassert}() in arch/arm/mach-davinci/clock.c.
This needs to be moved to the PSC driver in drivers/clk. The reset
framework in drivers/reset looks ideal for handling this, but it is
currently device tree only and it looks like we need it to work on
non-device tree boards. So, our options are: to move this hack to the
PSC driver in drivers/clk/ *or* to update the reset framework to work
with non-device tree boards *or* convert all of the mach-davinci boards
to device tree only.

* The device tree bindings for "ti,keystone,psc-clock" are not the best
and I am really not keen on reusing them. What makes sense to me would
be to define a device tree node that represents the entire PSC IP block
with child nodes for each clock output. Something like this:

power-controller@2350000 {
compatible = "ti,davinci-psc";
reg = <0x2350000 0xb00>;

...

clkhyperlink0: clkhyperlink0 {
#clock-cells = <0>;
clock-output-names = "hyperlink-0";
clocks = <&chipclk12>;
power-domain = <5>;
module-domain = <12>;
};

...
};

But the keystone bindings assign the entire PSC register space to each
clock individually (twice). And the really ugly part is that the module
domain and the power domain numbers of each PSC "clock" is embedded in
the register address. So, if you have reg = <0x02350030 0xb00>,
<0x02350014 0x400>; what this really means is that the clock driver will
iomap 0x02350000 (notice the 00 instead of the 30 and 14 at the end of
each register and that the base 0x02350000 is essentially listed twice
with two different sizes) and it means that this particular clock has a
module domain of 12 (0x30 / 4 = 12, 4 comes from the fact these are
32-bit registers) and a power domain of 5 (0x14 / 4 = 5). You can find
the numbers 12 and 5 easily in the SRM, but the numbers used in the
device tree bindings are pretty meaningless. Furthermore, the driver
itself stores the base address in a global variable which won't work for
DA8XX since it has two PSCs instead of one. So, for this case, I'm
really tempted to just write a new driver with better device tree
bindings rather than trying to fix up the keystone driver to make it
compatible with davinci and better device tree bindings while still
maintaining backwards compatibility with wacky device tree bindings.

* The keystone PLL driver and device tree bindings are in better shape,
but they cannot be used as-is with davinci family processors since the
registers are laid out differently. I have a work-in-progress patch for
this that I started quite a while back [4][5].

* All of the keystone clock drivers are currently device tree only, so
they will need to be modified *or* as mentioned above all mach-davinci
boards could be converted to device tree only.

[1]: https://patchwork.kernel.org/patch/10108437/
[2]: https://patchwork.kernel.org/patch/10115483/
[3]: https://patchwork.kernel.org/patch/9449741/
[4]:
https://github.com/dlech/ev3dev-kernel/commit/5e7bf2070aa03283b605d7b86864758d24f83aa8
[5]:
https://github.com/dlech/ev3dev-kernel/commit/1e34686ff57a673c8655eaedacec4d65d48f93b3


---

Also, a bit more background on my motivation here. Really, all I want is
to be able to use the "pwm-clock" device tree bindings to supply a clock
to a Bluetooth chip. But the driver for it requires the common clock
framework.

This is just a spare time project for me, which is why I have just done
the bare minimum to get CONFIG_COMMON_CLK enabled rather than trying to
fix all of the hard problems to do thing the "right way". As you have
seen above, there is still quite a bit of work remaining to be done,
especially when doing this just for fun. It is probably just wishful
thinking, but I kind of hoped that if I was able to get things this far,
maybe some other interested parties might be able to help with one of
the various pieces.

One way or another, I suppose I will get my Bluetooth clock eventually. :-)

\
 
 \ /
  Last update: 2017-12-20 00:12    [W:0.203 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site