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

[Sean Anderson]

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.

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

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

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. If that clock is a composite clock, the
clk_composite_get_rate will then choose the *real* get_rate function to
call, and will call it with the appropriate component clock. 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! 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.

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

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