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

[Lukasz Majewski]

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


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

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

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

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

>  	unsigned long parent_rate = clk_get_parent_rate(clk);
>  	unsigned int val;
>  
> @@ -150,8 +149,7 @@ int divider_get_val(unsigned long rate, unsigned
> long parent_rate, 
>  static ulong clk_divider_set_rate(struct clk *clk, unsigned long
> rate) {
> -	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);
>  	unsigned long parent_rate = clk_get_parent_rate(clk);
>  	int value;
>  	u32 val;
> diff --git a/drivers/clk/clk-fixed-factor.c
> b/drivers/clk/clk-fixed-factor.c index 711b0588bc..d2401cf440 100644
> --- a/drivers/clk/clk-fixed-factor.c
> +++ b/drivers/clk/clk-fixed-factor.c
> @@ -18,8 +18,7 @@
>  
>  static ulong clk_factor_recalc_rate(struct clk *clk)
>  {
> -	struct clk_fixed_factor *fix =
> -		to_clk_fixed_factor(dev_get_clk_ptr(clk->dev));
> +	struct clk_fixed_factor *fix = to_clk_fixed_factor(clk);
>  	unsigned long parent_rate = clk_get_parent_rate(clk);
>  	unsigned long long int rate;
>  
> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> index 70b8794554..b2933bc24a 100644
> --- a/drivers/clk/clk-gate.c
> +++ b/drivers/clk/clk-gate.c
> @@ -43,8 +43,7 @@
>   */
>  static void clk_gate_endisable(struct clk *clk, int enable)
>  {
> -	struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_gate *gate = to_clk_gate(clk);
>  	int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
>  	u32 reg;
>  
> @@ -86,8 +85,7 @@ static int clk_gate_disable(struct clk *clk)
>  
>  int clk_gate_is_enabled(struct clk *clk)
>  {
> -	struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_gate *gate = to_clk_gate(clk);
>  	u32 reg;
>  
>  #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF)
> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> index 5acc0b8cbd..67b4afef28 100644
> --- a/drivers/clk/clk-mux.c
> +++ b/drivers/clk/clk-mux.c
> @@ -35,8 +35,7 @@
>  int clk_mux_val_to_index(struct clk *clk, u32 *table, unsigned int
> flags, unsigned int val)
>  {
> -	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_mux *mux = to_clk_mux(clk);
>  	int num_parents = mux->num_parents;
>  
>  	if (table) {
> @@ -79,8 +78,7 @@ unsigned int clk_mux_index_to_val(u32 *table,
> unsigned int flags, u8 index) 
>  u8 clk_mux_get_parent(struct clk *clk)
>  {
> -	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_mux *mux = to_clk_mux(clk);
>  	u32 val;
>  
>  #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF)
> @@ -97,8 +95,7 @@ u8 clk_mux_get_parent(struct clk *clk)
>  static int clk_fetch_parent_index(struct clk *clk,
>  				  struct clk *parent)
>  {
> -	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_mux *mux = to_clk_mux(clk);
>  
>  	int i;
>  
> @@ -115,8 +112,7 @@ static int clk_fetch_parent_index(struct clk *clk,
>  
>  static int clk_mux_set_parent(struct clk *clk, struct clk *parent)
>  {
> -	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_mux *mux = to_clk_mux(clk);
>  	int index;
>  	u32 val;
>  	u32 reg;
> diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c
> index 1b9db6e791..e32c0cb53e 100644
> --- a/drivers/clk/imx/clk-gate2.c
> +++ b/drivers/clk/imx/clk-gate2.c
> @@ -37,7 +37,7 @@ struct clk_gate2 {
>  
>  static int clk_gate2_enable(struct clk *clk)
>  {
> -	struct clk_gate2 *gate =
> to_clk_gate2(dev_get_clk_ptr(clk->dev));
> +	struct clk_gate2 *gate = to_clk_gate2(clk);
>  	u32 reg;
>  
>  	reg = readl(gate->reg);
> @@ -50,7 +50,7 @@ static int clk_gate2_enable(struct clk *clk)
>  
>  static int clk_gate2_disable(struct clk *clk)
>  {
> -	struct clk_gate2 *gate =
> to_clk_gate2(dev_get_clk_ptr(clk->dev));
> +	struct clk_gate2 *gate = to_clk_gate2(clk);
>  	u32 reg;
>  
>  	reg = readl(gate->reg);




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/20200126/35d7b875/attachment.sig>