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

[Lukasz Majewski]

Hi Sean,

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

Thanks for the explanation. Now it is clear.

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

Yes, correct.

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

Ok.

Just informative - in U-Boot I do use DTS ccf node (for iMX6) to have
this driver probed with DM.

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

Yes. Correct.

> This
> is why I think the calls which I removed were unnecessary.

Those calls were for composite clocks. And as I've pointed out earlier
the address of e.g. struct clk_gate2 is the same as first its element -
the struct clk in this case.

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

Then - OK. Please resubmit the patch with section added to
doc/imx/clk/ccf.txt

(If possible please also review the rest of this document - more review
- the better).

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

I would prefer to not mix the meaning of struct clk members. The xlate
is supposed to work with DM/DTS, so we shall not add any new meaning for
it.

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

This was just my proposition. There is no "solid" use case for this
function.

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

Solid arguments.

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

Yes. I do agree here.

Please resubmit the aforementioned patch (with ccf.txt doc extension). I
will pull it.

Then, if you like, please prepare the patch set for CCF optimization.
We could discuss it in detail until the v2020.04 merge window opens.
There shall be enough time to have an agreement and prepare those
patches for pulling.

> 
> 	--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/20200131/958c935b/attachment.sig>