Hi Sean,
On 1/27/20 6:40 PM, Lukasz Majewski wrote:
The real problem with the current approach (IMO) is that there is a mismatch between the clock structure and the clock function. If one calls clk_get_rate on some clock, the get_rate function is chosen based on clk->dev->ops.
Yes, exactly. This seems logical to me -> the "top" structure is struct clk, which is a device with proper ops. (And in U-Boot the 'central' data structure with DM is struct udevice).
If that clock is a composite clock, the clk_composite_get_rate
I've found only clk_composite_set_rate @ drivers/clk/clk-composite.c
But, yes it calls its own clk_ops (*mux_ops, *rate_ops, *gate_ops).
will then choose the *real* get_rate function to call, and will call it with the appropriate component clock.
Yes, the struct clk_composite has several clocks defined.
But then, the get_rate function says "Aha, I know better than you what clock should be passed to me" and calls itself with the composite clock struct instead!
Wouldn't clk_get_rate just return 0 when it discovers that clk->dev is NULL for not bound driver (the clk which builds a composite driver)?
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.
clk_gate_*, clk_fixed_divider_*, clk_mux_*, etc. So this description is really for the case where only the first section of this patch is applied.
This is really unintitive behaviour. If anything, this kind of behaviour should be moved up to clk_get_rate, where it can't cause any harm in composite clocks.
Even better, the composite clocks shall be handled in the same way as non composite ones.
To achieve that:
- The struct clk shall be passed to all clk functions (IIRC this is
now true in U-Boot):
- then clk IP block specific structure (e.g. struct
clk_gate2) are accessible via container_of on clk pointer
Ok, so I'm a bit confused about the design decisions here. It seems to me that there are two uses for struct clk:
- struct clk as used by drivers not using the CCF. Here, the structure is effectively an extended parameter list, containing all the data needed to operate on some clock. clk->dev->priv contains whatever the driver wants, and
doesn't necessarily have a struct clk. Because these clocks are mostly just parameters, they can be created with xlate and request; there is no one "canonical" struct clk for any given logical clock. These clocks can appear on a device tree, and be acquired by clk_get_by_*.
Yes.
- 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<------------
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.
, 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).
- There shall be clk->dev filled always. In the dev one
shall use dev->ops to do the necessary work (even for composite clocks components)
Do you mean having a struct device for every clock, even components of a composite clock? I think this is unnecessary, especially for a system with lots of composite clocks. Storing the clock ops in the composite clock's struct works fine, and is conceptually simple.
I agree. We shall avoid creating/instantiating unnecessary udevices.
- 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?
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; }
I'm just wondering if "normal" clocks shall set this clk->get_parent pointer or continue to use clk->dev->parent?
--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