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

[Sean Anderson]

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.

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

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

>> , 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. This is
why I think the calls which I removed were unnecessary.

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.

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

Well, the CCF doesn't use xlate or register, so this field is unused at
the moment.

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

> 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
		- Clock consumers have almost the same api
		- Every clock driver will need to be changed, but most
		  drivers which don't use CCF never use any fields in
		  clk other than ->dev and ->id
	- 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

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.

	--Sean