Message-ID: <20150528061121.22384.20782@quantum>
Date:	Wed, 27 May 2015 23:11:21 -0700
From:	Michael Turquette <mturquette@...aro.org>
To:	andrey <andrey@...hel.com>, "Guenter Roeck" <linux@...ck-us.net>
Cc:	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
	lee.jones@...aro.org, sebastian.hesselbarth@...il.com,
	rabeeh@...id-run.com, "York Sun" <yorksun@...escale.com>
Subject: Re: clock driver

Quoting andrey (2015-05-27 17:29:00)
> 
> 
> ---- On Wed, 27 May 2015 17:10:06 -0700 Guenter Roeck<linux@...ck-us.net> wrote ---- 
>  > On 05/27/2015 04:58 PM, andrey wrote: 
>  > > 
>  > > 
>  > > ---- On Wed, 27 May 2015 16:08:12 -0700 Guenter Roeck<linux@...ck-us.net> wrote ---- 
>  > > 
>  > >   > On 05/27/2015 12:44 PM, andrey wrote: 
>  > >   > > Hello all, 
>  > >   > > Let me add a comment on using sysfs to simplify user space access to the clock 
>  > >   > > features as opposed to controlling them from a driver that uses the clock chip driver. 
>  > >   > > 
>  > >   > > It is common to use such advanced clock chips with the FPGA devices (as me and 
>  > >   > > York do), and a lot of development (HDL code) is done before a fancy higher-level 
>  > >   > > driver is even started. And it is not just a temporary stage needed by a small minority 
>  > >   > > of developers - as HDL coding gets more to the the core of many new devices running 
>  > >   > > Linux kernel, it makes sense to create the chip drivers more developer-friendly, not 
>  > >   > > just for the final use in a higher level device driver - modification of the HDL code 
>  > >   > > (most modern FPGA are programmed at runtime) makes it a new device that may 
>  > >   > > need a new driver. 
>  > >   > > I'm sure that it is not just for me, when it starts with the chip driver that supports 
>  > >   > > low-level functionality exposing it to the user space, and then working on the HDL 
>  > >   > > code using Python scripts at that stage. And only later in the development designing 
>  > >   > > the higher level device drivers that may not need all of the chip functionality. And such 
>  > >   > > higher level driver will work for our systems, but other developers who work on their 
>  > >   > > embedded systems will again need access to low level chip functionality, and will have 
>  > >   > > to redo the same work all over again. This I believe is a rationale of exposing such 
>  > >   > > chip-specific hardware features (not all of them are probably easy to fit into a specific 
>  > >   > > standard model) to the user space scripts. 
>  > >   > > 
>  > >   > > I wrote the initial driver code for our system 
>  > >   > > ( https://github.com/Elphel/linux-elphel/blob/master/src/drivers/misc/si5338.c ) and 
>  > >   > > being very far from being a kernel developer myself (I'm more of a hardware guy) 
>  > >   > > I didn't even try to satisfy the required coding style and submit it, so I'm very thankful 
>  > >   > > to York who re-wrote the code and is trying to make it usable to others. 
>  > >   > > 
>  > >   > 
>  > >   > Line wraps at ~75 columns would make this a bet easier to read. 
>  > > 
>  > > Guenter, I'm sorry for using "rich text" email settings. 
>  > > 
>  > >   > 
>  > >   > A more generic solution to your problem might be to implement a driver 
>  > >   > similar to i2c-dev, which exports raw i2c device information to user space. 
>  > >   > In your case, you would export information about the clocks in the system, 
>  > >   > possibly through sysfs (i2c-dev uses ioctl which is a bit old-fashioned). 
>  > > 
>  > > I was trying to make it safer to use low-level functionality of the particular 
>  > > (and rather popular) clock chip and to avoid using SiLabs proprietary tools to 
>  > > generate required settings offline. Using just raw i2c would require to have 
>  > > large user space program to calculate valid settings for the device. 
>  > > 
>  > > I would consider this chip as both a generic clock device that can fit into 
>  > > a standard framework and simultaneously a unique device that offers specific 
>  > > functionality outside of the framework. I thought that sysfs (instead of 
>  > > "old-fashioned" ioctl I used in such cases before) can offer 
>  > > hardware developer-friendly solution as a supplement to in-framework 
>  > > basic functionality. 
>  > > 
>  > > Device driver for this chip makes it possible to avoid proprietary configuration 
>  > > software and calculate register settings at runtime, minimizing requirements to 
>  > > the user space software and so the time developers of the new embedded 
>  > > systems will need to (re-)implement these important chip-specific  features. 
>  > > 
>  >  
>  > I think we are in violent agreement ;-). Only question was how to implement 
>  > sysfs (or user space access) support, where my preference would be a more 
>  > generic solution. 
> 
> Guenter,
> 
> I just considered this chip as a "frontier" device, not yet a member of an established
> class of similar ones. It may be possible to generalize later, extracting common
> functionality to a more abstract interface. But we just need this device support now,
> and when this one will become a member of some generic class - "frontier" will again
> move a step farther, new devices will emerge that stick out of the nice frameworks.

Hi Andrey,

I think this is a cool problem to solve. As far as frontier devices go I
really can't say. Other companies create similar clock generators that
are programmed at run-time over i2c. And we already have support merged
for Silicon Labs 5351 and 570 devices:

	drivers/clk/clk-si5351.c
	drivers/clk/clk-si5351.h
	drivers/clk/clk-si570.c

I'm not sure that we need to group such devices into a new "class". The
Linux common clock framework (ccf) has two classes: clock providers and
clock consumers. We haven't needed any more classification than that
thus far.

I took a look at your github code and it looks like you expose registers
individually as sysfs files. That is a start, but a better abstraction
is to consider the clock input signals that your pcie/fpga device will
take as input. With a proper clock driver for the silabs part your
pcie/fpga driver could hopefully just call clk_prepare_enable and
clk_set_rate and clk_set_parent as needed on those clocks to configure
them for consumption by the downstream fpga.

According to the data sheet[0] it looks like there are not many clock
outputs (CLK0{A,B}, CLK1{A,B}, CLK2{A,B}, and CLK3{A,B}).

At what point do you know how the clocks should be configured? I am
guessing that your fpga driver (e.g. the clock consumer) figures that
out as bytestream blobs are loaded? So that seems like the right call
site to start enabling clocks and configuring rates, instead of stuffing
that into the silabs driver (e.g. the clock provider).

York,

There is already a way to clock drivers to extend the debugfs interfaces
for per-driver stuff. The Nvidia Tegra guys do this already in their
out-of-tree test modules. Do you think such an interface might be
helpful to you? See:

clk_debugfs_add_file in drivers/clk/clk.c
and,
http://lkml.kernel.org/r/<1403794853-16928-1-git-send-email-pdeschrijver@...dia.com>

So your silabs clock generator driver could export some custom knobs
which help out in the early phases of development.

Ideally this interface would not be a register-level interface, but an
output signal type interface. Here is an example of the files you will
have by default:

$ ls /sys/kernel/debug/clk/clk0a/
clk_accuracy      clk_flags           clk_phase          clk_rate
clk_enable_count  clk_notifier_count  clk_prepare_count

With the custom debugfs interface you might add a "clk_set_rate" file
where user space can write to it and change the frequency of that clock
signal. You can do the same to change mux settings and clock gates as
well.

A userspace tool might even be able to take the clockbuilder data to do
this, if someone is willing to write it.

[0] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf

Regards,
Mike

> 
> Andrey
> 
>  >  
>  > Thanks, 
>  > Guenter 
>  >  
>  > > Andrey 
>  > > 
>  > >   > 
>  > >   > This would be a driver independent solution, and work for all clock drivers. 
>  > >   > It might still not be accepted by Mike and Stephen, due to the risk, but it 
>  > >   > might be worth a try. After all, using i2c-dev to access i2c devices directly 
>  > >   > is just as risky. 
>  > >   > 
>  > >   > In my opinion, it is always better to have a driver in the upstream kernel, 
>  > >   > if possible one that uses a standard framework. That makes it much easier 
>  > >   > to support going forward. 
>  > >   > 
>  > >   > Guenter 
>  > >   > 
>  > >   > 
>  > > 
>  > > 
>  > > 
>  > > 
>  >  
>  > 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists