[U-Boot] [PATCH v2 03/17] dm: clk: Define clk_get_parent_rate() for clk operations

[Lukasz Majewski <lukma@denx.de>]

Hi Simon,

> +Stephen
> 
> Hi Lukasz,
> 
> On Thu, 31 Jan 2019 at 02:04, Lukasz Majewski <lukma@denx.de> wrote:
> >
> > This commit adds the clk_get_parent_rate() function, which is
> > responsible for getting the rate of parent clock.
> > Unfortunately, u-boot's DM support for getting parent is different
> > (the parent relationship is in udevice) than the one in common clock
> > framework (CCF) in Linux.
> >
> > To alleviate this problem - the clk_get_parent_rate() function has
> > been introduced to clk-uclass.c.
> >
> > As written in the in-code comment - some clocks do not set clk->id
> > (and require it to be set to 0) and hence the standard
> > ckl_{request|get_rate| free} API is used.
> >
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >
> > Changes in v2: None
> >
> >  drivers/clk/clk-uclass.c | 41
> > +++++++++++++++++++++++++++++++++++++++++ include/clk.h
> > |  9 +++++++++ 2 files changed, 50 insertions(+)  
> 
> Can we please call this from test/dm/clk.c?

Ok.

> 
> >
> > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > index 6d7a514006..f1640dda67 100644
> > --- a/drivers/clk/clk-uclass.c
> > +++ b/drivers/clk/clk-uclass.c
> > @@ -340,6 +340,47 @@ ulong clk_get_rate(struct clk *clk)
> >         return ops->get_rate(clk);
> >  }
> >
> > +ulong clk_get_parent_rate(struct clk *clk)
> > +{
> > +       const struct clk_ops *ops;
> > +       struct udevice *pdev;
> > +       struct clk pclk;
> > +       ulong rate;
> > +       int ret;
> > +
> > +       debug("%s(clk=%p)\n", __func__, clk);
> > +
> > +       pdev = clk->dev->parent;  
> 
> dev_get_parent(clk->dev)

Yes, correct.

> 
> > +       if (!pdev)
> > +               return -ENODEV;  
> 
> Not needed, all devices have parents except the root, which is not in
> UCLASS_CLK

Ok.

> 
> > +
> > +       ops = clk_dev_ops(pdev);
> > +       if (!ops->get_rate)
> > +               return -ENOSYS;
> > +
> > +       /*
> > +        * We do use memset, clk_{request|get_rate|free}
> > +        * as there are clocks - like the "fixed" ones, which
> > +        * doesn't posses the clk wrapper struct (just added to
> > +        * UCLASS_CLK) and explicitly check if clk->id = 0.
> > +        *
> > +        * In fact the "clock" resources (like ops, description)
> > +        * are accessed via udevice structure (pdev - parent's one)
> > +        */
> > +  
> 
> drop blank line. Also is that comment wrapped to use the full number
> of columns?

I will refactor the comment.

> 
> Also, this seems like a bug that should be fixed?

Yes, this seems strange to me:

The main structure passed in the clock API in U-boot is struct clk *clk
pointer.

However, the fixed clocks (clk_fixed_rate.c) require the clk->id to be
set to 0. This is strange as imposes in practice passing new, memset'ed
to 0 clk structure pointer. Moreover, they doesn't have corresponding
struct clk definition and exists only linked in UCLASS_CLK.

The pattern to get clock rate:

memset(clk, 0, sizeof(clk);
clk_request(pdev, clk)  --> clk->dev = pdev [*]; (this is _the_ defacto
						clock assignment)
clk_get_rate(clk)
clk_free(clk)

is also used in socfpga [1] and 'clk dump' command and IMHO is wrong
(but I cannot understand why it was done in that way - we _shall_ pass
pointer to struct clk, not rely on udevices - like in [*]).

Such approach causes the passed clk pointer to be useless as it is NULL
when we call clk_get_rate() recursively.
And for this reason the struct clk *clk pointer is stored in
driver_data for udevice.


[1] - arch/arm/mach-socfpga/clock_manager_arria10.c

> 
> > +       memset(&pclk, 0, sizeof(pclk));
> > +       ret = clk_request(pdev, &pclk);
> > +       if (ret) {
> > +               printf("%s: pclk: %s request failed!\n", __func__,
> > pdev->name);
> > +               return ret;
> > +       }
> > +
> > +       rate = clk_get_rate(&pclk);
> > +       clk_free(&pclk);
> > +
> > +       return rate;
> > +}
> > +
> >  ulong clk_set_rate(struct clk *clk, ulong rate)
> >  {
> >         const struct clk_ops *ops = clk_dev_ops(clk->dev);
> > diff --git a/include/clk.h b/include/clk.h
> > index f6fbcc6634..8224295ec3 100644
> > --- a/include/clk.h
> > +++ b/include/clk.h
> > @@ -238,6 +238,15 @@ int clk_free(struct clk *clk);
> >  ulong clk_get_rate(struct clk *clk);
> >
> >  /**
> > + * clk_get_parent_rate() - Get parent of current clock rate.
> > + *
> > + * @clk:       A clock struct that was previously successfully
> > requested by
> > + *             clk_request/get_by_*().
> > + * @return clock rate in Hz, or -ve error code.
> > + */
> > +ulong clk_get_parent_rate(struct clk *clk);
> > +
> > +/**
> >   * clk_set_rate() - Set current clock rate.
> >   *
> >   * @clk:       A clock struct that was previously successfully
> > requested by --
> > 2.11.0
> >  
> 
> Regards,
> Simon




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: <http://lists.denx.de/pipermail/u-boot/attachments/20190131/3ffb682d/attachment.sig>