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:
> >
> > 1. 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 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?
> 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: lu...@denx.de
pgpHbi2rTwkIE.pgp
Description: OpenPGP digital signature
