Re: clock driver

[York Sun <yorksun-KZfg59tc24xl57MIdRCFDg@public.gmane.org>]

On 05/27/2015 09:42 AM, Sebastian Hesselbarth wrote:
> On 27.05.2015 17:07, York Sun wrote:
>> On 05/27/2015 12:09 AM, Sebastian Hesselbarth wrote:
> [...]
>>> Also, why should a user ever be able to mess with the clocks? If you
>>> allow a user to change the clock rate of any output, you have to
>>> consider that he will likely be able to crash your system easily.
>>>
>>> As long as you cannot give a clear requirement for user-configurable
>>> clocks - especially in the detail of the driver you mentioned -
>>> mainline kernel is not the place for such a driver.
>>
>> This driver I am proposing supports SI5338 in a generic way. It can take device
>> tree as its default configuration. However I am using it differently, explained
>> in detail below.
> [...]
>>> (a) Clocks are limited to the PCI card and only need a limited set of
>>> configurable clocks. You should add functions to load the registers
>>> with either the full register map or parts of it in a table based
>>> approach. You don't expose the clocks with CCF but deal with rate
>>> change requests internally in the PCI driver. You could also consider
>>> to have the initial clock configuration as part of some firmware blob
>>> you request with the PCI driver.
>>
>> That's right. I only need to change a small portion of the configuration, such
>> as frequency, but keeping the reset the same, including output driver voltage,
>> input clock, etc.
> [...]
>> My application has a host SoC booting up Linux. Then the clocks on PCIe (FPGA)
>> cards get initialized with their clocks. The clocks are not used by host SoC, so
>> setting the wrong clocks doesn't crash the system. Each PCIe card has up to four
>> clock chips (with four clocks on each chip). It is required for users to be able
>> to change the clocks after system boots up.
> 
> Consider a userspace configurable clock driver, load the FPGA design
> which depends on a specific frequency generated by Si5338 and let the
> user mess with your sysfs files - that will certainly crash your system.
> 
> Still, I do not see any requirement for a clock driver for that use
> case. You have to load the FPGA design or at least configure it to
> use the Si5338 generated clocks _after_ configuring Si5338. You'll
> have to have a user interface for FPGA bitfile loading, so you can
> add another one for the clock generator config.
> 
>> I wrote my driver for the PCIe cards so the clocks can be initialized using the
>> data provided. But changing the clocks, or initializing with another set of
>> configuration requires an interface. There are many ways to solve this. I would
>> like to keep the clock driver generic so it can be reused. It looks like CCF may
>> not be the best fit for such driver. What is an acceptable way to write this
>> driver so it can be in the mainline kernel, or other maintained projects (I am
>> not aware of any though)?
> 
> IMHO "generic" as in a generic mainline kernel clock driver just means
> that other _drivers_ can request any clock rate from that chip. If you
> want to write a CCF driver for Si5338, you'll have to make your PCIe
> driver request that clock and hide the userspace configuration within
> your PCIe driver.
> 
> Adding userspace interfaces to generic CCF clock drivers will not happen
> just because messing with the clocks will break a running system. As I
> said before, AFAIKS i2c-dev should give you enough of an interface to
> configure the clock generator from userspace.
>

Sebastian,

Thanks for the insight. Looks like I should give up upstreaming this driver. I
will find other ways to make this driver available if anyone wants to use it.

York