Hi,
first of all I apologize for the quite long attachment but I think
it is useful for the following discussion.
I recently read about the newly introduced common clock framework (ccf)
and wondered if this could be also used for external, e.g. i2c attached,
clock generators.
Based on my current understanding of the framework I wrote such a
driver and now I want to present it here for clarification of some
remarks I have regarding the framework itself.
Please do not see this driver as mature but as some kind of
proof-of-concept. I have the driver somewhat running but stumbled
upon some issues.
First I want to give a brief overview of the intended use case of
this driver:
It is a driver for a clock generator that is externally attached to
a Marvell Dove (arm/mach-dove) SoC. It will provide driver configurable
clocks that are connected to dedicated clock inputs of the SoC, e.g.
external audio clock for i2s controller.
The basic intention I had in mind when writing this driver was to
add it during platform init and pass a list of clock aliases and clock
hierarchy description to allow the receiving driver, e.g. i2s, to set
the rate of the supplied clock without poking the clock generator
directly.
Please comment on the following aspects:
- there is no clk_unregister which is okay for platform clocks but
should be there since clock generators can be detached
- the clock generator has two plls and up to 8 clocks; inside the
clk_ops it is quite hard to find the correct struct clk_hw when
using container_of()
- most of clk_ops are spin-locked but i2c drivers tend to sleep
during read or write; this causes a "BUG: scheduling while atomic"
I know that ccf is quite new but it is well suited for generic,
i.e. platform independent, external clock generator drivers. Maybe
I got the overall concept just wrong but maybe this RFC helps
to straighten things up for future drivers.
Regards,
Sebastian
On Thu, May 10, 2012 at 03:11:55AM +0200, Sebastian Hesselbarh wrote:
Always CC maintainers on mails, things get lost on high volume lists.
> I recently read about the newly introduced common clock framework (ccf)
> and wondered if this could be also used for external, e.g. i2c attached,
> clock generators.
Yes, it should be able to be used for this. I've been posting a driver
for the wm831x PMICs for quite some considerable time.
> static inline u8 si5351_reg_read(struct si5351_driver_data *data, u8 addr)
> {
> return (u8)i2c_smbus_read_byte_data(data->client, addr);
> }
I'd suggest using regmap for the cache support (which helps with
read/modify/write cycles) and because...
> static int si5351_xtal_enable(struct clk_hw *hw)
> {
> struct si5351_driver_data *sidata =
> container_of(hw, struct si5351_driver_data, hw_xtal);
> u8 reg;
>
> reg = si5351_reg_read(sidata, SI5351_FANOUT_ENABLE);
> reg |= SI5351_XTAL_ENABLE;
> si5351_reg_write(sidata, SI5351_FANOUT_ENABLE, reg);
> return 0;
> }
...we should be able to factor things like this out of the drivers using
regmap (we've been getting some great results doing that for regulator
over this release).
> static const struct clk_ops si5351_xtal_ops = {
> .enable = si5351_xtal_enable,
> .disable = si5351_xtal_disable,
These should be prepare() and unprepare() - enable() and disable() are
called from atomic context so you can't do I2C I/O.
> static u8 si5351_clkout_get_parent(struct clk_hw *hw)
> {
> struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
> return 0;
> }
If we need empty functions like this we should fix the framework.
> sidata = kzalloc(sizeof(struct si5351_driver_data), GFP_KERNEL);
> if (sidata == NULL) {
> dev_err(&client->dev, "unable to allocate driver data\n");
> return -ENOMEM;
> }
devm_kzalloc().
On 05/13/2012 02:29 PM, Mark Brown wrote:
> Always CC maintainers on mails, things get lost on high volume lists.
Yeah, right after posting I thought of this but didn't want to bother
you again. I read a lot of kernel code since then and some things are
much more clearer to me now. Besides the driver is almost done now and
works as expected. Will have to clean up things and will post it as
a patch then.
> I'd suggest using regmap for the cache support (which helps with
> read/modify/write cycles) and because...
I will have a look at regmap before I post a new driver patch for
sure.
>> static const struct clk_ops si5351_xtal_ops = {
>> .enable = si5351_xtal_enable,
>> .disable = si5351_xtal_disable,
>
> These should be prepare() and unprepare() - enable() and disable() are
> called from atomic context so you can't do I2C I/O.
You are right. The kernel code states this correctly and I should have
read it more carefully.
>> static u8 si5351_clkout_get_parent(struct clk_hw *hw)
>> {
>> struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
>> return 0;
>> }
>
> If we need empty functions like this we should fix the framework.
Of course, this function is not empty at all but wasn't done at the
time I posted the RFC.
One question that remains from my original RFC is the missing
clk_unregister() as i2c can be removed there also should be an
function to unregister previously registered clocks?
Sebastian
On Sun, May 13, 2012 at 06:30:55PM +0200, Sebastian Hesselbarh wrote:
> One question that remains from my original RFC is the missing
> clk_unregister() as i2c can be removed there also should be an
> function to unregister previously registered clocks?
One of the patches I've been sending adds a dummy clk_unregister() for
the sake of making the drivers look nicer - practically speaking it's
not likely to be terribly important as these things don't get unloaded
terribly often. It looks like that patch didn't get applied either.
Mike's stuff is due to hit -next on Monday so I was planning to refresh
my driver and resend it then, probably with the unregister() though I'm
considering just removing the unregistration code to try to get make it
a bit more likely to get applied.
On 05/13/2012 06:43 PM, Mark Brown wrote:
> One of the patches I've been sending adds a dummy clk_unregister() for
> the sake of making the drivers look nicer - practically speaking it's
> not likely to be terribly important as these things don't get unloaded
> terribly often. It looks like that patch didn't get applied either.
Well, of course I don't plan to unload the driver ever but basically it
is possible..
One more thing I thought about: The platform I currently use needs to
pass the external clocks to the platform devices that can use them
later. IMHO the correct way of creating clocks would be:
- register i2c clock driver and let it register its clocks with names
like e.g. si5351, clkout0. The clock driver itself cannot and should
not know who uses it later on.
- let drivers look for e.g. kirkwood-i2s.1, extclk because the i2s
driver cannot know where the external clock comes from.
- have a board-specific function that configures clock hierarchy and
create suitable clk_aliases e.g.
si5351,clkout0 = kirkwood-i2s.1,extclk.
Currently I added a callback function pointer to the platform data
passed to the i2c clock driver that is called at the end of clock
driver probe. I doubt it will be accepted that way but can't think
of any other way..
Sebastian
On Sun, May 13, 2012 at 07:11:09PM +0200, Sebastian Hesselbarh wrote:
> One more thing I thought about: The platform I currently use needs
> to pass the external clocks to the platform devices that can use
> them
> later. IMHO the correct way of creating clocks would be:
> - register i2c clock driver and let it register its clocks with names
> like e.g. si5351, clkout0. The clock driver itself cannot and should
> not know who uses it later on.
> - let drivers look for e.g. kirkwood-i2s.1, extclk because the i2s
> driver cannot know where the external clock comes from.
> - have a board-specific function that configures clock hierarchy and
> create suitable clk_aliases e.g.
> si5351,clkout0 = kirkwood-i2s.1,extclk.
> Currently I added a callback function pointer to the platform data
> passed to the i2c clock driver that is called at the end of clock
> driver probe. I doubt it will be accepted that way but can't think
> of any other way..
This is what clkdev is for - it provides such a layer of indirection.
The only trick right now is that it needs the struct clk to add the
lookup but some platform data for your driver would probably do the
trick in the short term, I don't know what the plan is long term with
that stuff.
On Sun, May 13, 2012 at 10:11 AM, Sebastian Hesselbarh
<[email protected]> wrote:
> On 05/13/2012 06:43 PM, Mark Brown wrote:
>>
>> One of the patches I've been sending adds a dummy clk_unregister() for
>> the sake of making the drivers look nicer - practically speaking it's
>> not likely to be terribly important as these things don't get unloaded
>> terribly often. ?It looks like that patch didn't get applied either.
>
>
> Well, of course I don't plan to unload the driver ever but basically it
> is possible..
>
Mark,
Seems like the clk_unregister patch fell through the cracks. My
mistake. I'm going to be sending another pull request to arm-soc with
Orion/Kirkwood support. I'll add your patch in at that time.
Sebastian,
Please CC me on the next revision of your patch. Mark's review hit
all of the high points for this version.
Regards,
Mike
On Mon, May 14, 2012 at 11:08:58AM -0700, Turquette, Mike wrote:
> Seems like the clk_unregister patch fell through the cracks. My
> mistake. I'm going to be sending another pull request to arm-soc with
> Orion/Kirkwood support. I'll add your patch in at that time.
Ah, great. I dropped usage from the wm831x driver I just posted, I'd
rather get that merged if I can, but I can easily add it back - I'll
either roll it in to the next version or send a followup if this version
is basically OK.
Hi Sebastian,
On 10.05.2012 03:11, Sebastian Hesselbarh wrote:
> first of all I apologize for the quite long attachment but I think
> it is useful for the following discussion.
>
> I recently read about the newly introduced common clock framework (ccf)
> and wondered if this could be also used for external, e.g. i2c attached,
> clock generators.
>
> Based on my current understanding of the framework I wrote such a
> driver and now I want to present it here for clarification of some
> remarks I have regarding the framework itself.
May I kindly ask what happened to this driver? Are you planning to do
another spin for submission?
Many thanks,
Daniel
> Please do not see this driver as mature but as some kind of
> proof-of-concept. I have the driver somewhat running but stumbled
> upon some issues.
>
> First I want to give a brief overview of the intended use case of
> this driver:
>
> It is a driver for a clock generator that is externally attached to
> a Marvell Dove (arm/mach-dove) SoC. It will provide driver configurable
> clocks that are connected to dedicated clock inputs of the SoC, e.g.
> external audio clock for i2s controller.
>
> The basic intention I had in mind when writing this driver was to
> add it during platform init and pass a list of clock aliases and clock
> hierarchy description to allow the receiving driver, e.g. i2s, to set
> the rate of the supplied clock without poking the clock generator
> directly.
>
> Please comment on the following aspects:
> - there is no clk_unregister which is okay for platform clocks but
> should be there since clock generators can be detached
> - the clock generator has two plls and up to 8 clocks; inside the
> clk_ops it is quite hard to find the correct struct clk_hw when
> using container_of()
> - most of clk_ops are spin-locked but i2c drivers tend to sleep
> during read or write; this causes a "BUG: scheduling while atomic"
>
> I know that ccf is quite new but it is well suited for generic,
> i.e. platform independent, external clock generator drivers. Maybe
> I got the overall concept just wrong but maybe this RFC helps
> to straighten things up for future drivers.
>
> Regards,
> Sebastian
>
>
>
>
>
>
On 10/11/2012 10:34 AM, Daniel Mack wrote:
>> I recently read about the newly introduced common clock framework (ccf)
>> and wondered if this could be also used for external, e.g. i2c attached,
>> clock generators.
>>
>> Based on my current understanding of the framework I wrote such a
>> driver and now I want to present it here for clarification of some
>> remarks I have regarding the framework itself.
>
> May I kindly ask what happened to this driver? Are you planning to do
> another spin for submission?
Daniel,
the driver is still on my list but moved more and more downwards as I
got caught by mach-dove DT integration. There is still some work to do,
namely regmap and DT. It is used by some off-mainline tree for SolidRun
CuBox quite successfully since then. Does any of you work rely on
a working si5351 driver?
Sebastian
On 11.10.2012 18:00, Sebastian Hesselbarth wrote:
> On 10/11/2012 10:34 AM, Daniel Mack wrote:
>>> I recently read about the newly introduced common clock framework (ccf)
>>> and wondered if this could be also used for external, e.g. i2c attached,
>>> clock generators.
>>>
>>> Based on my current understanding of the framework I wrote such a
>>> driver and now I want to present it here for clarification of some
>>> remarks I have regarding the framework itself.
>>
>> May I kindly ask what happened to this driver? Are you planning to do
>> another spin for submission?
>
> Daniel,
>
> the driver is still on my list but moved more and more downwards as I
> got caught by mach-dove DT integration. There is still some work to do,
> namely regmap and DT. It is used by some off-mainline tree for SolidRun
> CuBox quite successfully since then. Does any of you work rely on
> a working si5351 driver?
Yes, it does actually. I can hack around it for now, but at some point,
a proper driver is needed. And yours looks quite feature complete, so it
would be easiest to finish this one :)
Do you still have access to hardware you wrote the driver for? Let me
know if you need any help around here.
Thanks,
Daniel
Adding LAKML and devtree as there might be people willing to comment about
DT representation of i2c-attached clock generators, too.
On 10/12/2012 08:17 PM, Daniel Mack wrote:
> On 11.10.2012 18:00, Sebastian Hesselbarth wrote:
>> [...]
>> Does any of you work rely on a working si5351 driver?
>
> Yes, it does actually. I can hack around it for now, but at some point,
> a proper driver is needed. And yours looks quite feature complete, so it
> would be easiest to finish this one :)
Well, yes it is some kind of feature complete except regmap and DT. Adding
regmap to the driver should be quite easy but with DT I am still thinking
of the best way to represent the internal connections between OSC, PLLs, and
CLKOUTs. In the last version of the driver I had a callback that was
board specific to setup these connections but with DT there will be no board
specific code anymore.
Maybe one of the common clk maintainers can give a hint how this could be
done in a clean way. The question is how to represent a i2c-attached clock
generator config in DT where you want to setup clock parents of CLKOUTs and
PLLs.
A possible solution would be something like this:
si5351a@60 {
compatible = "silabs,si5351a";
reg = <0x60>;
si_osc: osc {
compatible = "fixed-clock";
clock-frequency = <270000000>;
};
si_plla: pll@0 {
compatible = "silabs,si5351-pll";
/* hook-up plla to osc input */
clocks = <&si_osc>;
};
si_clkout0: clkout@0 {
compatible = "silabs,si5351-clkout";
/* hook-up clkout0 to plla */
clocks = <&si_plla>;
/* request target frequency */
clock-frequency = <148500000>;
};
};
Although this perfectly describes the clock hierarchy I still don't like
the sub-node style. Another flat solution would be something like:
si_osc: osc {
compatible = "fixed-clock";
clock-frequency = <270000000>;
};
si5351a@60 {
compatible = "silabs,si5351a";
reg = <0x60>;
clocks = <&si_osc>;
si5351-pll-config = <0 0 /* pll A to osc */
1 0>; /* pll B to osc */
si5351-clock-config = <0 0 148500000 /* clkout 0 to pll A, 148.5MHz */
1 0 0 /* clkout 1 to pll A, disabled */
2 1 24000000>; /* clkout 2 to pll B, 24.0Mhz */
};
> Do you still have access to hardware you wrote the driver for? Let me
> know if you need any help around here.
Yes, hardware is still available although I only have access to the Si5351a
with 3 clkouts. The code should be compatible for Si5351a with 8 clkouts and
code skeleton for 5351b (OSC and VXCO input) and 5351c (OSC and CLKIN) is
there but untested.
I've transferred the current driver version to my repository to work on. As soon
as I have regmap done, I will push it online and give you a note.
Sebastian
Hi,
On 14.10.2012 12:59, Sebastian Hesselbarth wrote:
> Adding LAKML and devtree as there might be people willing to comment about
> DT representation of i2c-attached clock generators, too.
>
> On 10/12/2012 08:17 PM, Daniel Mack wrote:
>> On 11.10.2012 18:00, Sebastian Hesselbarth wrote:
> >> [...]
>>> Does any of you work rely on a working si5351 driver?
>>
>> Yes, it does actually. I can hack around it for now, but at some point,
>> a proper driver is needed. And yours looks quite feature complete, so it
>> would be easiest to finish this one :)
>
> Well, yes it is some kind of feature complete except regmap and DT. Adding
> regmap to the driver should be quite easy but with DT I am still thinking
> of the best way to represent the internal connections between OSC, PLLs, and
> CLKOUTs. In the last version of the driver I had a callback that was
> board specific to setup these connections but with DT there will be no board
> specific code anymore.
>
> Maybe one of the common clk maintainers can give a hint how this could be
> done in a clean way. The question is how to represent a i2c-attached clock
> generator config in DT where you want to setup clock parents of CLKOUTs and
> PLLs.
>
> A possible solution would be something like this:
>
> si5351a@60 {
> compatible = "silabs,si5351a";
> reg = <0x60>;
>
> si_osc: osc {
> compatible = "fixed-clock";
> clock-frequency = <270000000>;
> };
>
> si_plla: pll@0 {
> compatible = "silabs,si5351-pll";
> /* hook-up plla to osc input */
> clocks = <&si_osc>;
> };
>
> si_clkout0: clkout@0 {
> compatible = "silabs,si5351-clkout";
> /* hook-up clkout0 to plla */
> clocks = <&si_plla>;
> /* request target frequency */
> clock-frequency = <148500000>;
> };
> };
>
> Although this perfectly describes the clock hierarchy I still don't like
> the sub-node style. Another flat solution would be something like:
I think the sub-node style above it nicer because it allows referencing
the individual clocks outputs with a phandle. We use this chip to
generate base-frequencies for audio clocks, and so we have to switch
between two freqs for the multiples of 22.5KHz and 24KHz at runtime.
>> Do you still have access to hardware you wrote the driver for? Let me
>> know if you need any help around here.
>
> Yes, hardware is still available although I only have access to the Si5351a
> with 3 clkouts. The code should be compatible for Si5351a with 8 clkouts and
> code skeleton for 5351b (OSC and VXCO input) and 5351c (OSC and CLKIN) is
> there but untested.
The 3 clkout model is the only one I have access to as well.
> I've transferred the current driver version to my repository to work on. As soon
> as I have regmap done, I will push it online and give you a note.
That's great. Let me know if you want me to test anything.
Thanks,
Daniel
On 10/14/2012 01:13 PM, Daniel Mack wrote:
> I think the sub-node style above it nicer because it allows referencing
> the individual clocks outputs with a phandle. We use this chip to
> generate base-frequencies for audio clocks, and so we have to switch
> between two freqs for the multiples of 22.5KHz and 24KHz at runtime.
Both examples allow you to have a phandle for all individual clock-outputs.
The examples weren't complete but with the sub-node style you'll reference
with e.g. <&clkout0> while the flat one will use <&si5351 0>. I still prefer
the flat-style as it will not allow to have a phandle of plls.
Sebastian