Hi Lukasz,
On 1/29/20 7:29 PM, Lukasz Majewski wrote:
Yes, but then clk_get_parent throws a fit, which gets called by
Could you explain what "throw a fit" means here? I'm a bit confused.
Ok, so imagine I have a clk_divider in a composite clock. When clk_get_rate gets called on the composite clock, the composite clock then calls clk_divider_get_rate on the divider clock. The first thing that function does is call clk_get_parent_rate, which in turn calls clk_get_parent. This last call fails because the divider clock has a NULL ->dev. The end behaviour is that the parent rate looks like 0, which causes the divider to in turn return 0 as its rate. So while it doesn't throw a fit, we still end up with a bad result.
- struct clk as used by CCF clocks. Here the structure
contains specific information configuring each clock. Each struct clk refers to a different logical clock. clk->dev->priv contains a struct clk (though this may not be the same struct clk).
The struct clk pointer is now stored in the struct's udevice uclass_priv pointer.
For CCF it looks like below:
struct clk_gate2 -> struct clk -> struct udevice *dev -> struct udevice /|\ | | | -------------uclass_priv<------------
Right, so at best doing dev_get_clk_ptr(clk->dev) in something like clk_divider_set_rate is a no-op, and at worst it breaks something.
These clocks cannot appear in a device tree
I think they could, but I've followed the approach of Linux CCF in e.g. i.MX6Q SoC.
They could, but none of them have compatible strings at the moment.
, and hence cannot be acquired by clk_get_by_* (except for clk_get_by_id which doesn't use the device tree). These clocks are not created by xlate and request.
Correct. Those clocks are instantiated in SoC specific file. For example in ./drivers/clk/imx/clk-imx6q.c
With this in mind, I'm not sure why one would ever need to call dev_get_clk_ptr. In the first case, clk->dev->priv could be anything, and probably not a clock. In the second case, one already has the "canonical" struct clk.
The problem is that clocks are linked together with struct udevice (_NOT_ struct clk). All clocks are linked together and have the same uclass - UCLASS_CLK.
To access clock - one needs to search this doubly linked list and you get struct udevice from it. (for example in ./cmd/clk.c)
And then you need a "back pointer" to struct clk to use clock ops/functions. And this back pointer is stored in struct udevice's uclass_priv pointer (accessed via dev_get_clk_ptr).
Right, but clock implementations will never need to do this. Whatever clock they get passed is going to be the clock they should use. This is why I think the calls which I removed were unnecessary.
In fact, through this discussion I think the patch as-submitted is probably the least-disruptive way to make composite clocks work in a useful way. The only thing it does is that sometimes clk->dev->priv != clk, but I don't think that anyone was relying on this being the case.
- The struct clk has flags field (clk->flags). New flags: -- CCF_CLK_COMPOSITE_REGISTERED (visible with dm
tree) -- CCF_CLK_COMPOSITE_HIDDEN (used internally to gate, mux, etc. the composite clock)
-- clk->dev->flags with CCF_CLK_COMPOSITE_REGISTERED set puts all "servant" clocks to its child_head list (clk->dev->child_head).
__OR__ -- we extend the ccf core to use struct uc_clk_priv (as described: https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40) which would have struct uc_clk_priv { struct clk *c; /* back pointer to clk */ struct clk_composite *cc; }; struct clk_composite { struct clk *mux; struct clk *gate; ... (no ops here - they shall be in structudevice) };
The overhead is to have extra 4 bytes (or use unionand check CCF_CLK_COMPOSITE flags).
For me the more natural seems the approach with using clk->dev->child_head (maybe with some extra new flags defined). U-Boot handles lists pretty well and maybe OF_PLATDATA could be used as well.
I like the private data approach, but couldn't we use the existing clk->data field? E.g. instead of having
struct clk_foo { struct clk clk;
This is the approach took from the Linux kernel. This is somewhat similar to having the struct clk_hw: https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-gate2.c#L...
/* ... */ };
clk_register_foo(...) { struct clk_foo *foo; struct clk *clk;
foo = kzalloc(sizeof(*foo)); /* ... */
clk = foo->clk; clk_register(...); }
int clk_foo_get_rate(struct clk *clk) { struct clk_foo *foo = to_clk_foo(clk); /* ... */ }
we do
struct clk_foo { /* ... */ };
clk_register_foo(...) { struct clk_foo *foo; struct clk *clk;
foo = kzalloc(sizeof(*foo)); clk = kzalloc(sizeof(*clk)); /* ... */
clk->data = foo;
According to the description of struct clk definition (@ ./include/clk.h) the data field has other purposes.
Maybe we shall add a new member of struct clk?
Well, the CCF doesn't use xlate or register, so this field is unused at the moment.
clk_register(...); }
int clk_foo_get_rate(struct clk *clk) { struct clk_foo *foo = (struct clk_foo *)clk->data; /* ... */ }
Of course, now that I have written this all out, it really feels like the container_of approach all over again...
Indeed. Even the access cost is the same as the struct clk is always placed as the first element of e.g. struct clk_gate2
I think the best way of approaching this may be to simply introduce a get_parent op. CCF already does something like this with clk_mux_get_parent. This would also allow for some clocks to have a NULL ->dev.
I've thought about this some time ago and wondered if struct clk shouldn't be extended a bit.
Maybe adding there a pointer to "get_parent" would simplify the overall design of CCF?
Then one could set this callback pointer in e.g. clk_register_gate2 (or clk_register_composite) and set clk->dev to NULL to indicate "composite" clock?
So we would have:
static inline bool is_composite_clk(struct clk *clk) { return !clk->dev && clk->flags == CCF_CLK_COMPOSITE; }
What would use that predicate?
I'm just wondering if "normal" clocks shall set this clk->get_parent pointer or continue to use clk->dev->parent?
Hm, well after thinking it over, I think with the current system having a method for this would not do anything useful, since you still need to get the ops from the udevice (and then you could just use the default behaviour).
If I could make big sweeping changes clock uclass, I would do something like: - Split off of_xlate, request, and free into a new clk_device_ops struct, with an optional pointer to clk_ops - Create a new struct clk_handle containing id, data, a pointer to struct udevice, and a pointer to struct clk - Add get_parent to clk_ops and change all ops to work on struct clk_handle - Add a pointer to clk_ops in struct clk, and remove dev, id, and data.
So for users, the api would look like
struct clk_handle clk = clk_get_by_index(dev, 0, &clk); clk_enable(&clk); ulong rate = clk_get_rate(&clk);
Method calls would roughly look like
ulong clk_get_rate(struct clk_handle *h) { struct clk_ops *ops;
if (h->clk) ops = h->clk->ops; else ops = clk_dev_ops(h->dev)->ops; return ops->get_rate(h); }
Getting the parent would use h->clk->ops->get_parent if it exists, and otherwise use h->dev to figure it out.
For non-CCF drivers, the implementation could look something like
ulong foo_get_rate(struct clk_handle *h) { /* Do something with h->id and h->dev */ }
struct foo_clk_ops = { .get_rate = foo_get_rate, };
struct foo_clk_driver_ops = { .ops = &foo_clk_ops, };
For drivers using the CCF, the implementation could look like
struct clk_gate *foo_gate;
int foo_probe(struct udevice *dev) { foo_gate = /* ... */; }
int foo_request(struct clk_handle *h) { h->clk = foo_gate->clk; }
struct foo_clk_driver_ops = { .request = foo_request, };
So why these changes? - Clear separation between the different duties which are both currently handled by struct clk - Simplification for drivers using the CCF - Minimal changes for existing users - Clock consumers have almost the same api - Every clock driver will need to be changed, but most drivers which don't use CCF never use any fields in clk other than ->dev and ->id - No need to get the "canonical" clock, since it is pointed at by clk_handle->clk - Reduction in memory/driver usage since CCF clocks no longer need a udevice for each clock - Less function calls since each driver using CCF no longer needs to be a proxy for clock ops
Now these are big changes, and definitely would be the subject of their own patch series. As-is, I think the patch as it exists now is the best way to get the most functionality from clk_composite with the least changes to other code.
--Sean