[PATCH v2 01/11] clk: Always use the supplied struct clk

[Lukasz Majewski <lukma@denx.de>]

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: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200130/0431a6cb/attachment.sig>