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@xxxxxxxx> 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.