Hi Simon,
> +Stephen
>
> Hi Lukasz,
>
> On Thu, 31 Jan 2019 at 02:04, Lukasz Majewski <lu...@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 <lu...@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: lu...@denx.de
pgpToC_Qb4rOR.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
