[U-Boot] [PATCH v5 10/15] clk: Add fixed-factor clock driver

[Anup Patel]

On Tue, Feb 19, 2019 at 8:47 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Anup,
>
> On Tue, 5 Feb 2019 at 06:05, Anup Patel <Anup.Patel at wdc.com> wrote:
> >
> > This patch adds fixed-factor clock driver which derives clock
> > rate by dividing (div) and multiplying (mult) fixed factors
> > to a parent clock.
>
> Did someone else send a similar patch recently?

The patch numbering got changed after v5 because we
dropped few patches.

The v7 patch is "[PATCH v7 09/15] clk: Add fixed-factor clock driver"
https://patchwork.ozlabs.org/patch/1039638/

>
> >
> > Signed-off-by: Atish Patra <atish.patra at wdc.com>
> > Signed-off-by: Anup Patel <anup.patel at wdc.com>
> > ---
> >  arch/sandbox/dts/test.dts      |  8 ++++
> >  drivers/clk/Makefile           |  4 +-
> >  drivers/clk/clk_fixed_factor.c | 72 ++++++++++++++++++++++++++++++++++
> >  test/dm/clk.c                  |  5 ++-
> >  4 files changed, 87 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/clk/clk_fixed_factor.c
> >
> > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> > index 1d011ded7c..cb8d686e46 100644
> > --- a/arch/sandbox/dts/test.dts
> > +++ b/arch/sandbox/dts/test.dts
> > @@ -203,6 +203,14 @@
> >                         #clock-cells = <0>;
> >                         clock-frequency = <1234>;
> >                 };
> > +
> > +               clk_fixed_factor: clk-fixed-factor {
> > +                       compatible = "fixed-factor-clock";
> > +                       #clock-cells = <0>;
> > +                       clock-div = <3>;
> > +                       clock-mult = <2>;
> > +                       clocks = <&clk_fixed>;
>
> Is there a binding file for this somewhere? Please can you include it
> in doc/device-tree-bindings?

Fixed factor clock is standard Linux style clock. We are following
Linux DT bindings for fixed factor clock (just like fixed rate clock).

For Linux DT bindings refer,
<linux_source>/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt

>
> > +               };
> >         };
> >
> >         clk_sandbox: clk-sbox {
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 2f4446568c..fa59259ea3 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -4,7 +4,9 @@
> >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> >  #
> >
> > -obj-$(CONFIG_$(SPL_TPL_)CLK) += clk-uclass.o clk_fixed_rate.o
> > +obj-$(CONFIG_$(SPL_TPL_)CLK) += clk-uclass.o
> > +obj-$(CONFIG_$(SPL_TPL_)CLK) += clk_fixed_rate.o
>
> Why change that? It is not related to your patch.

Well, this was crossing 80 characters per-line.

>
> > +obj-$(CONFIG_$(SPL_TPL_)CLK) += clk_fixed_factor.o
> >
> >  obj-y += imx/
> >  obj-y += tegra/
> > diff --git a/drivers/clk/clk_fixed_factor.c b/drivers/clk/clk_fixed_factor.c
> > new file mode 100644
> > index 0000000000..3487c11729
> > --- /dev/null
> > +++ b/drivers/clk/clk_fixed_factor.c
> > @@ -0,0 +1,72 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> > + *
> > + * Author: Anup Patel <anup.patel at wdc.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <clk-uclass.h>
> > +#include <div64.h>
> > +#include <dm.h>
> > +
> > +struct clk_fixed_factor {
> > +       struct clk parent;
> > +       unsigned int div;
> > +       unsigned int mult;
> > +};
> > +
> > +#define to_clk_fixed_factor(dev)       \
> > +       ((struct clk_fixed_factor *)dev_get_platdata(dev))
> > +
> > +static ulong clk_fixed_factor_get_rate(struct clk *clk)
> > +{
> > +       uint64_t ret;
>
> How about 'rate' instead?

Okay, will update.

>
> > +       struct clk_fixed_factor *ff = to_clk_fixed_factor(clk->dev);
> > +
> > +       if (clk->id != 0)
> > +               return -EINVAL;
> > +
> > +       ret = clk_get_rate(&ff->parent);
> > +       if (IS_ERR_VALUE(ret))
> > +               return ret;
> > +
> > +       do_div(ret, ff->div);
> > +
> > +       return ret * ff->mult;
> > +}
> > +
> > +const struct clk_ops clk_fixed_factor_ops = {
> > +       .get_rate = clk_fixed_factor_get_rate,
> > +};
> > +
> > +static int clk_fixed_factor_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       int err;
> > +       struct clk_fixed_factor *ff = to_clk_fixed_factor(dev);
> > +
> > +       err = clk_get_by_index(dev, 0, &ff->parent);
> > +       if (err)
> > +               return err;
> > +
> > +       ff->div = dev_read_u32_default(dev, "clock-div", 1);
> > +       ff->mult = dev_read_u32_default(dev, "clock-mult", 1);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id clk_fixed_factor_match[] = {
> > +       {
> > +               .compatible = "fixed-factor-clock",
> > +       },
> > +       { /* sentinel */ }
> > +};
> > +
> > +U_BOOT_DRIVER(clk_fixed_factor) = {
> > +       .name = "fixed_factor_clock",
> > +       .id = UCLASS_CLK,
> > +       .of_match = clk_fixed_factor_match,
> > +       .ofdata_to_platdata = clk_fixed_factor_ofdata_to_platdata,
> > +       .platdata_auto_alloc_size = sizeof(struct clk_fixed_factor),
> > +       .ops = &clk_fixed_factor_ops,
> > +};
> > diff --git a/test/dm/clk.c b/test/dm/clk.c
> > index 898c034e27..112d5cbbc9 100644
> > --- a/test/dm/clk.c
> > +++ b/test/dm/clk.c
> > @@ -12,12 +12,15 @@
> >
> >  static int dm_test_clk(struct unit_test_state *uts)
> >  {
> > -       struct udevice *dev_fixed, *dev_clk, *dev_test;
> > +       struct udevice *dev_fixed, *dev_fixed_factor, *dev_clk, *dev_test;
> >         ulong rate;
> >
> >         ut_assertok(uclass_get_device_by_name(UCLASS_CLK, "clk-fixed",
> >                                               &dev_fixed));
> >
> > +       ut_assertok(uclass_get_device_by_name(UCLASS_CLK, "clk-fixed-factor",
> > +                                             &dev_fixed_factor));
> > +
>
> Could you test a little more - e.g. set the clock rate and check it?

Fixed factor clock (just like Fixed rate clock) does not provide set_rate()
so testing set clock rate won't be applicable here.

>
> >         ut_assertok(uclass_get_device_by_name(UCLASS_CLK, "clk-sbox",
> >                                               &dev_clk));
> >         ut_asserteq(0, sandbox_clk_query_enable(dev_clk, SANDBOX_CLK_ID_SPI));

Regards,
Anup