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

[Lukasz Majewski]

Hi Sean,

> Hi Lukasz,
> 
> Thanks for the feedback.
> 
> On 1/26/20 4:20 PM, Lukasz Majewski wrote:
> > Hi Sean,
> >   
> >> CCF clocks should always use the struct clock passed to their
> >> methods for extracting the driver-specific clock information
> >> struct.  
> > 
> > This couldn't be done for i.MX8 at least. There was an issue with
> > composite clock on this SoC.
> > 
> > (I've CC'ed Peng, who did this work for i.MX8 - I was
> > testing/developing the framework for i.MX6Q which doesn't support
> > composite clocks).
> > 
> > For some design decisions and the "overall picture" of CCF , please
> > see following doc entry:
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt
> >  
> 
> That documentation was helpful, but it tends to focus more on the
> "what" than the "why." Perhaps I can help update it when I have a
> better grasp on the CCF.

Update is more than welcome, as I was the only author of this document.
(It would be good to have it checked from the other "angle").

> 
> >> Previously, many functions would use the clk->dev->priv if the
> >> device was bound. This could cause problems with composite clocks.
> >>   
> > 
> > The problem (in short) is with combining Linux CCF design and
> > U-Boot's DM (more details in the link above).
> > 
> > All the clocks are linked with struct udevice (one search the linked
> > list for proper clock).
> > 
> > However, with Linux the "main" clock structure is 'struct clk,
> > which is embedded in CLK IP block specific structure (i.e. struct
> > clk_gate2).
> > 
> > Problem is that from struct clk we do have pointer to struct udevice
> > (*dev), but there is no direct pointer "up" from struct udevice to
> > struct clk (now we do use udevice's->uclass_priv).
> > 
> > So far so good ....
> > 
> > Problem started with iMX8, where we do have a composite clocks,
> > which would like to be seen as a single one (like struct pllX), but
> > they comprise of a few other "clocks".
> > 
> > To distinct them the clk_dev_binded() was added, which checks if the
> > struct udevice represents "top" composite clock (which shall be
> > visible with e.g. 'clk' command)
> >  
> >> The
> >> individual clocks in a composite clock did not have the ->dev field
> >> filled in. This was fine, because the device-specific clock
> >> information would be used. However, since there was no ->dev, there
> >> was no way to get the parent clock.   
> > 
> > Wasn't there any back pointer available? Or do we need to search the
> > clocks linked list and look for "bind" flag in top level composite
> > clock?  
> 
> This is just what the clk_get_parent [1] function does.
> 
> [1]
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/clk-uclass.c#L447

Ach. Right.

Then the struct clk is used - and as those clocks are not bound to
device (to avoid having it visible in e.g. dm tree command output) the
clk is passed instead. 

It works because the struct clk_composite (defined @ clk-provider.h)
has embedded struct clk in it and with container_of we get clk_ops....


> 
> >   
> >> This caused the recalc_rate
> >> method of the CCF divider clock to fail. One option would be to use
> >> the clk->priv field to get the composite clock and from there get
> >> the appropriate parent device. However, this would tie the
> >> implementation to the composite clock. In general, different
> >> devices should not rely on the contents of ->priv from another
> >> device.  
> > 
> > Maybe we shall follow:
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40
> >  
> 
> I think this might be a better option. I just didn't see any other
> uses of this pointer in the u-boot code. 
> 
> >>
> >> The simple solution to this problem is to just always use the
> >> supplied struct clock.   
> > 
> > I think that we took this approach in the past. Unfortunately, this
> > caused all clocks being exposed when 'clk' command was run.  
> 
> This is discussed further below, but an easy option is to just not
> register the component clocks.

You probably mean to not register clocks which 'struct clk' is embedded
in struct clk_composite?

Yes, this is what we do want - as we get:

	foo (composite clock):
		----------------> pll2
				  ----------> MUX2
					      ------> gate2

And in dm tree we do want to only see the 'foo'.

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

> 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

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

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


Sean, Peng, Simon, feel free to give your ideas and comments about
possible solutions.

> 
> > 
> > Peng - were there any other issues with i.MX8 composite clock
> > implementation?
> >   
> >> The composite clock now fills in the ->dev
> >> pointer of its child clocks. This allows child clocks to make calls
> >> like clk_get_parent() without issue.
> >>
> >> imx avoided the above problem by using a custom get_rate function
> >> with composite clocks.  
> > 
> > Do you refer to:
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/imx/clk-composite-8m.c#L30
> > 
> > There the clk is cast from (struct clk_composite *)clk->data
> > 
> > (now in U-Boot we do have 4! different implementations of struct
> > clk). 
> 
> Yes. This was really surprising when I saw it the first time, since it
> is effectively bypassing clk_composite_*.

Unfortunately, two struct clks are from Broadcomm devices, one for some
Linux video port and the fourth supports U-Boot's driver model (DM).

> 
> >>
> >> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> >> ---
> >>  drivers/clk/clk-composite.c    |  8 ++++++++
> >>  drivers/clk/clk-divider.c      |  6 ++----
> >>  drivers/clk/clk-fixed-factor.c |  3 +--
> >>  drivers/clk/clk-gate.c         |  6 ++----
> >>  drivers/clk/clk-mux.c          | 12 ++++--------
> >>  drivers/clk/imx/clk-gate2.c    |  4 ++--
> >>  6 files changed, 19 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/clk/clk-composite.c
> >> b/drivers/clk/clk-composite.c index a5626c33d1..d0f273d47f 100644
> >> --- a/drivers/clk/clk-composite.c
> >> +++ b/drivers/clk/clk-composite.c
> >> @@ -145,6 +145,14 @@ struct clk *clk_register_composite(struct
> >> device *dev, const char *name, goto err;
> >>  	}
> >>  
> >> +	if (composite->mux)
> >> +		composite->mux->dev = clk->dev;
> >> +	if (composite->rate)
> >> +		composite->rate->dev = clk->dev;
> >> +	if (composite->gate)
> >> +		composite->gate->dev = clk->dev;
> >> +
> >> +
> >>  	return clk;
> >>  
> >>  err:
> >> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> >> index 822e09b084..bfa05f24a3 100644
> >> --- a/drivers/clk/clk-divider.c
> >> +++ b/drivers/clk/clk-divider.c
> >> @@ -70,8 +70,7 @@ unsigned long divider_recalc_rate(struct clk *hw,
> >> unsigned long parent_rate, 
> >>  static ulong clk_divider_recalc_rate(struct clk *clk)
> >>  {
> >> -	struct clk_divider *divider =
> >> to_clk_divider(clk_dev_binded(clk) ?
> >> -			dev_get_clk_ptr(clk->dev) : clk);
> >> +	struct clk_divider *divider = to_clk_divider(clk);  
> > 
> > How do you differentiate the "top" level of composite clock from the
> > clocks from which the composite clock is built?
> > 
> > Now we do use the clk_dev_binded().  
> 
> With how I am using it, the clocks from which the composite clock are
> built are not registered with dm [2]. The only clock which gets bound
> is the composite clock. This follows the example in [3]. Since all the
> parameters are known at compile-time, I could even just have a big
> array with all the struct clk_dividers (or other component clocks) I
> need. However, I chose to model the imx code.
> 
> [2]
> https://github.com/Forty-Bot/u-boot/blob/maix_v3/drivers/clk/kendryte/clk.c#L84
> [3]
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/imx/clk-composite-8m.c#L117
> 
> --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/20200128/8e31ea41/attachment.sig>