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 struct >>> udevice) }; >>> >>> The overhead is to have extra 4 bytes (or use union >>> and 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#L27 >> /* ... */ >> }; >> >> 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
