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

[Sean Anderson]

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
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_*.
	- 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). These
	  clocks cannot appear in a device tree, 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.
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.

> 	- 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.

> 
> 	- 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;
	/* ... */
};

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;
	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...

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.

--Sean