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

CCF clocks should always use the struct clock passed to their methods for
extracting the driver-specific clock information struct. Previously, many
functions would use the clk->dev->priv if the device was bound. This could cause
problems with composite clocks. 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. 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.

The simple solution to this problem is to just always use the supplied struct
clock. 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.

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

Hi Sean

> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Sean Anderson
> Sent: Thursday, January 16, 2020 6:48 AM
> To: U-Boot Mailing List
> Subject: [PATCH v2 01/11] clk: Always use the supplied struct clk
>
> CCF clocks should always use the struct clock passed to their methods for extracting the driver-specific clock information struct. Previously, many functions would use the clk->dev->priv if the device was bound. This could cause problems with composite clocks. 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. 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.
>
> The simple solution to this problem is to just always use the supplied struct clock. 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.
>
> 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(-)

This patch is clock relative patch, if it is not a necessary patch for
Sipeed Maix support.
Please remove patch 1/11 and 2/11, and send them in another patch-sets.

Rick,
Thanks

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

> This patch is clock relative patch, if it is not a necessary patch for
> Sipeed Maix support.
> Please remove patch 1/11 and 2/11, and send them in another patch-sets.

Patches 10 and 11 rely on these first two patches. Should I just
reference the split off patches in the cover letter?

Hi Sean,

> > This patch is clock relative patch, if it is not a necessary patch for
> > Sipeed Maix support.
> > Please remove patch 1/11 and 2/11, and send them in another patch-sets.
>
> Patches 10 and 11 rely on these first two patches. Should I just
> reference the split off patches in the cover letter?

I am not sure the modifications of clk are ok for other people.
It will be better to send pure riscv relative patch-sets.

Thanks,
Rick

> I am not sure the modifications of clk are ok for other people.
> It will be better to send pure riscv relative patch-sets.

Hm, well at the moment removing the dependency on these two patches
would probably require a substantial rewrite of patch 11, which would
primarily consist of duplicating functionality currently found in the
clk_composite driver. I actually think the way clk_composite works at
the moment is very confusing, since the imx code which uses it uses some
custom functions to smooth out clk_composite's behaviour. These two
patches are probably the ones I would like to see merged most of all,
since it would make implementing complex clock drivers like on this
board much easier. I originally submitted these patches around a month
ago [1, 2], so I was hoping that I'd have recieved feedback one way or
the other by now.

[1] https://patchwork.ozlabs.org/patch/1215327/
[2] https://patchwork.ozlabs.org/patch/1215328/

> [1] https://patchwork.ozlabs.org/patch/1215327/
> [2] https://patchwork.ozlabs.org/patch/1215328/

err, these references should be

[1] https://patchwork.ozlabs.org/patch/1215335/
[2] https://patchwork.ozlabs.org/patch/1215337/

Hi Sean,

> > I am not sure the modifications of clk are ok for other people.
> > It will be better to send pure riscv relative patch-sets.  
> 
> Hm, well at the moment removing the dependency on these two patches
> would probably require a substantial rewrite of patch 11, which would
> primarily consist of duplicating functionality currently found in the
> clk_composite driver. I actually think the way clk_composite works at
> the moment is very confusing, since the imx code which uses it uses
> some custom functions to smooth out clk_composite's behaviour. These
> two patches are probably the ones I would like to see merged most of
> all, since it would make implementing complex clock drivers like on
> this board much easier. I originally submitted these patches around a
> month ago [1, 2], so I was hoping that I'd have recieved feedback one
> way or the other by now.
> 
> [1] https://patchwork.ozlabs.org/patch/1215327/
> [2] https://patchwork.ozlabs.org/patch/1215328/

I saw your patches. Unfortunately, there was the Christmas/New year's
break and afterwards I had some more urgent tasks to do. Apologize for
that...

What I would like to see here is to reuse (or better - make the code
less confusing) the code.

Rationale - the CCF was ported from iMX6Q Linux code from the outset.

Then Peng (CC'ed) wanted to adjust it to support composite clocks from
i.MX8.

As a result the CCF drifted to be an iMX aligned, but the goal is to
have it usable for other archs as well (and reuse from Linux as much as
possible).


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/20200124/075ca6dc/attachment.sig>

On 1/24/20 9:27 AM, Lukasz Majewski wrote:
> I saw your patches. Unfortunately, there was the Christmas/New year's
> break and afterwards I had some more urgent tasks to do. Apologize for
> that...
> 
> What I would like to see here is to reuse (or better - make the code
> less confusing) the code.
> 
> Rationale - the CCF was ported from iMX6Q Linux code from the outset.
> 
> Then Peng (CC'ed) wanted to adjust it to support composite clocks from
> i.MX8.
> 
> As a result the CCF drifted to be an iMX aligned, but the goal is to
> have it usable for other archs as well (and reuse from Linux as much as
> possible).

Ok, so I believe that these patches are a step in that direction. Is
there any specific feedback you would like to give regarding them? I've
tried to infer what good default behaviour should be. However, I'm sure
there are places where someone more familiar with the CCF would be able
to comment on.

--Sean

Hi Sean,

> On 1/24/20 9:27 AM, Lukasz Majewski wrote:
> > I saw your patches. Unfortunately, there was the Christmas/New
> > year's break and afterwards I had some more urgent tasks to do.
> > Apologize for that...
> > 
> > What I would like to see here is to reuse (or better - make the code
> > less confusing) the code.
> > 
> > Rationale - the CCF was ported from iMX6Q Linux code from the
> > outset.
> > 
> > Then Peng (CC'ed) wanted to adjust it to support composite clocks
> > from i.MX8.
> > 
> > As a result the CCF drifted to be an iMX aligned, but the goal is to
> > have it usable for other archs as well (and reuse from Linux as
> > much as possible).  
> 
> Ok, so I believe that these patches are a step in that direction. Is
> there any specific feedback you would like to give regarding them?
> I've tried to infer what good default behaviour should be. However,
> I'm sure there are places where someone more familiar with the CCF
> would be able to comment on.

I will do the review by the end of the weekend.

> 
> --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/20200125/bce98745/attachment.sig>

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>

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

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>

On 1/27/20 6:40 PM, Lukasz Majewski wrote:
>> 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)?

Yes, but then clk_get_parent throws a fit, which gets called by
clk_gate_*, clk_fixed_divider_*, clk_mux_*, etc.  So this description is
really for the case where only the first section of this patch is
applied.

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

Ok, so I'm a bit confused about the design decisions here. It seems to
me that there are two uses for struct clk:
	- struct clk as used by drivers not using the CCF. Here, the
	  structure is effectively an extended parameter list,
	  containing all the data needed to operate on some clock.
	  clk->dev->priv contains whatever the driver wants, and doesn't
	  necessarily have a struct clk. Because these clocks are mostly
	  just parameters, they can be created with xlate and request;
	  there is no one "canonical" struct clk for any given logical
	  clock. These clocks can appear on a device tree, and be
	  acquired by clk_get_by_*.
	- 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). These
	  clocks cannot appear in a device tree, 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.
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.

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

Do you mean having a struct device for every clock, even components of a
composite clock? I think this is unnecessary, especially for a system
with lots of composite clocks. Storing the clock ops in the composite
clock's struct works fine, and is conceptually simple.

> 
> 	- 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;
	/* ... */
};

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

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.

--Sean

Hi Sean,

> On 1/27/20 6:40 PM, Lukasz Majewski wrote:
> >> 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)?  
> 
> 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.

> clk_gate_*, clk_fixed_divider_*, clk_mux_*, etc.  So this description
> is really for the case where only the first section of this patch is
> applied.
> 
> >> 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  
> 
> Ok, so I'm a bit confused about the design decisions here. It seems to
> me that there are two uses for struct clk:
> 	- struct clk as used by drivers not using the CCF. Here, the
> 	  structure is effectively an extended parameter list,
> 	  containing all the data needed to operate on some clock.
> 	  clk->dev->priv contains whatever the driver wants, and
> doesn't necessarily have a struct clk. Because these clocks are mostly
> 	  just parameters, they can be created with xlate and request;
> 	  there is no one "canonical" struct clk for any given logical
> 	  clock. These clocks can appear on a device tree, and be
> 	  acquired by clk_get_by_*.

Yes.

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

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

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

> 
> > 	- 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)  
> 
> Do you mean having a struct device for every clock, even components
> of a composite clock? I think this is unnecessary, especially for a
> system with lots of composite clocks. Storing the clock ops in the
> composite clock's struct works fine, and is conceptually simple.

I agree. We shall avoid creating/instantiating unnecessary udevices.

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

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

I'm just wondering if "normal" clocks shall set this clk->get_parent
pointer or continue to use clk->dev->parent?

> 
> --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/20200130/0431a6cb/attachment.sig>

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

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>