Re: AM335x OMAP2 common clock external fixed-clock registration

[Michael Welling]

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.
> 
> Sebastian
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/