Hi Sean,
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.
Thanks for the explanation. Now it is clear.
- 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.
Yes, correct.
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.
Ok.
Just informative - in U-Boot I do use DTS ccf node (for iMX6) to have this driver probed with DM.
, 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.
Yes. Correct.
This is why I think the calls which I removed were unnecessary.
Those calls were for composite clocks. And as I've pointed out earlier the address of e.g. struct clk_gate2 is the same as first its element - the struct clk in this case.
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.
Then - OK. Please resubmit the patch with section added to doc/imx/clk/ccf.txt
(If possible please also review the rest of this document - more review - the better).
- 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 structuc_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 useunion 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#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.
I would prefer to not mix the meaning of struct clk members. The xlate is supposed to work with DM/DTS, so we shall not add any new meaning for it.
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?
This was just my proposition. There is no "solid" use case for this function.
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
drivers which don't use CCF never use any fields in clk other than ->dev and ->id
- Clock consumers have almost the same api
- Every clock driver will need to be changed, but most
- 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
Solid arguments.
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.
Yes. I do agree here.
Please resubmit the aforementioned patch (with ccf.txt doc extension). I will pull it.
Then, if you like, please prepare the patch set for CCF optimization. We could discuss it in detail until the v2020.04 merge window opens. There shall be enough time to have an agreement and prepare those patches for pulling.
--Sean
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de