Re: AM335x OMAP2 common clock external fixed-clock registration

Michael Welling
2015-04-16T22:09:32Z

On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> On 16.04.2015 18:17, Michael Welling wrote:
> >On Thu, Apr 16, 2015 at 07:32:32AM +0300, Tero Kristo wrote:
> >>On 04/15/2015 11:51 PM, Michael Welling wrote:
> >>>On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
> >>>>On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <mwelling@ieee.org> wrote:
> [...]
> >>>>>There is still an issue with the si5351.
> >>>>>
> >>>>>I had to comment out the clk_put here for the frequency to show up:
> >>>>>http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
> >>>>>
> >>>>>Ideas?
> >>>>
> >>>>What is the most recent upstream commit that you are based on?
> >>>
> >>>I am working from 4.0.0-rc7.
> >>>
> >>>7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
> >>
> >>Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get
> >>in the first place, as far as I understand this destroys the clock
> >>handle, which is still being used later in the code.
> >
> >Not sure how this ever worked. This has been in the code since the
> >initial commit.
>
> The reason it worked before may be related with recent rework of
> clk_put() itself and clk cookies instead of pointers. I lost track on
> the recent clk subsystem changes here, sorry.
>
> However, droping the clk immediately surely isn't right.
> The thing is, we can remove the clk_put() just because there is no
> _remove() for that driver. I remember that back in the days the driver
> was mainlined, clk removal wasn't too easy.
>
> FWIW, as soon as _remove() support will be added by someone, we'll have
> to rethink passing struct clk* by platform_data or at least
> double-check if we ever used [of_]clk_get() to obtain it.
>
> Mind to send a patch removing the clk_put() on !IS_ERR and add a proper
> error path instead? While of_clk_get() is the only calls that need
> cleanup on error in si5351_dt_parse() we should probably move that
> calls to the end of this function. Otherwise we'd also have to cleanup
> on every of_parse_foo() failure.

What would be the proper error path?
What cleanup is required?

It should be noted that there are more deep rooted issues with the driver
that I have noticed. For one the driver behaves differently if the debugging
is on and when it is off.

Here is what the kernel reports with debugging off:
root@som3517-som200:~# cat /sys/kernel/debug/clk/clk_summary
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------
ref27 0 0 27000000 0 0
xtal 0 0 27000000 0 0
pllb 0 0 599999994 0 0
ms0 0 0 12499999 0 0
clk0 0 0 12499999 0 0
plla 0 0 599999994 0 0
ms2 0 0 8219178 0 0
clk2 0 0 8219178 0 0
ms1 0 0 94117646 0 0
clk1 0 0 94117646 0 0

Here is what the kernel reports with debugging on:
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------
ref27 0 0 27000000 0 0
xtal 0 0 27000000 0 0
pllb 0 0 884736000 0 0
ms0 0 0 18432000 0 0
clk0 0 0 18432000 0 0
plla 0 0 897023997 0 0
ms2 0 0 12287999 0 0
clk2 0 0 12287999 0 0
ms1 0 0 140709646 0 0
clk1 0 0 140709646 0 0

Note this is with the following devicetree entry:
si5351: clock-generator {
#address-cells = <1>;
#size-cells = <0>;
#clock-cells = <1>;
compatible = "silabs,si5351a-msop";
reg = <0x60>;
status = "okay";

/* connect xtal input to 27MHz reference */
clocks = <&ref27>;

/* connect xtal input as source of pll0 and pll1 */
silabs,pll-source = <0 0>, <1 0>;

clkout0: clkout0 {
reg = <0>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <1>;
silabs,clock-source = <0>;
silabs,pll-master;
clock-frequency = <18432000>;
};

clkout1: clkout1 {
reg = <1>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <0>;
silabs,clock-source = <0>;
clock-frequency = <8000000>;
};

clkout2: clkout2 {
reg = <2>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <0>;
silabs,clock-source = <0>;
silabs,pll-master;
clock-frequency = <12288000>;
};
};

I am losing hope that this driver is stable enough to even use in production.
>
> Sebastian
>