[v2,3/4] clk: Provide an always-on clock domain framework

Much h/w contain clocks which if turned off would prove fatal.  The
only way to recover is to restart the board(s).  This driver takes
references to clocks which are required to be always-on in order to
prevent the common clk framework from trying to turn them off during
the clk_disabled_unused() procedure.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/clk/Makefile    |  1 +
 drivers/clk/clkdomain.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 drivers/clk/clkdomain.c

Hi Lee,

On Wed, 18 Feb 2015, Lee Jones wrote:
> +/*
> + * ST Clock Domain

minor nit, as v2 is a generic driver, comment needs updating.

regards,

Peter.

Quoting Lee Jones (2015-02-18 08:15:00)
> Much h/w contain clocks which if turned off would prove fatal.  The
> only way to recover is to restart the board(s).  This driver takes
> references to clocks which are required to be always-on in order to
> prevent the common clk framework from trying to turn them off during
> the clk_disabled_unused() procedure.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/clk/Makefile    |  1 +
>  drivers/clk/clkdomain.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
>  create mode 100644 drivers/clk/clkdomain.c
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index d5fba5b..d9e2718 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_HAVE_CLK)          += clk-devres.o
>  obj-$(CONFIG_CLKDEV_LOOKUP)    += clkdev.o
>  obj-$(CONFIG_COMMON_CLK)       += clk.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-divider.o
> +obj-$(CONFIG_COMMON_CLK)       += clkdomain.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-factor.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-rate.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-gate.o
> diff --git a/drivers/clk/clkdomain.c b/drivers/clk/clkdomain.c
> new file mode 100644
> index 0000000..8c83f99
> --- /dev/null
> +++ b/drivers/clk/clkdomain.c

Naming is hard. I'm not sure clkdomain.c is the best expression. Maybe
clk-alwon.c? I see ALWON used alot amongst hardware people who live in a
world of eight-character naming limitations.

> @@ -0,0 +1,63 @@
> +/*
> + * ST Clock Domain
> + *
> + * Copyright (C) 2015 STMicroelectronics – All Rights Reserved
> + *
> + * Author: Lee Jones <lee.jones@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/clk-private.h>

If this header still existed I would berate you mercilessly. Luckily for
you it no longer exists and only causes a compile error ;-)

> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +static void ao_clock_domain_hog_clock(struct platform_device *pdev, int index)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct clk *clk;
> +       int ret;
> +
> +       clk = of_clk_get(np, index);
> +       if (IS_ERR(clk)) {
> +               dev_warn(&pdev->dev, "Failed get clock %s[%d]: %li\n",
> +                        np->full_name, index, PTR_ERR(clk));
> +               return;
> +       }
> +
> +       ret = clk_prepare_enable(clk);
> +       if (ret)
> +               dev_warn(&pdev->dev, "Failed to enable clock: %s\n", clk->name);
> +}
> +
> +static int ao_clock_domain_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       int nclks, i;
> +
> +       nclks = of_count_phandle_with_args(np, "clocks", "#clock-cells");

Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
minutes writing that function and I need people to use it so I can get a
return on my investment.

Otherwise the patch looks good. I believe that this method is targeting
always-on clock in a production environment, which is different from the
CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
hardware or dealing with a platform that has incomplete driver support.

I wonder if there is a clever way for existing clock providers
(expressed in DT) to use this without having to create a separate node
of clocks with the "always-on-clk-domain" flag. Possibly the common
clock binding could declare some always-on flag that is standardized?
Then the framework core could use this code easily. Not sure if that is
a good idea though...

Regards,
Mike

> +
> +       for (i = 0; i < nclks; i++)
> +               ao_clock_domain_hog_clock(pdev, i);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id ao_clock_domain_match[] = {
> +       { .compatible = "always-on-clk-domain" },
> +       { },
> +};
> +
> +static struct platform_driver ao_clock_domain_driver = {
> +       .probe = ao_clock_domain_probe,
> +       .driver = {
> +               .name = "always-on-clk-domain",
> +               .of_match_table = ao_clock_domain_match,
> +       },
> +};
> +module_platform_driver(ao_clock_domain_driver);
> -- 
> 1.9.1
>

On Mon, 23 Feb 2015, Mike Turquette wrote:

> Quoting Lee Jones (2015-02-18 08:15:00)
> > Much h/w contain clocks which if turned off would prove fatal.  The
> > only way to recover is to restart the board(s).  This driver takes
> > references to clocks which are required to be always-on in order to
> > prevent the common clk framework from trying to turn them off during
> > the clk_disabled_unused() procedure.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/clk/Makefile    |  1 +
> >  drivers/clk/clkdomain.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 64 insertions(+)
> >  create mode 100644 drivers/clk/clkdomain.c
> > 
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index d5fba5b..d9e2718 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -3,6 +3,7 @@ obj-$(CONFIG_HAVE_CLK)          += clk-devres.o
> >  obj-$(CONFIG_CLKDEV_LOOKUP)    += clkdev.o
> >  obj-$(CONFIG_COMMON_CLK)       += clk.o
> >  obj-$(CONFIG_COMMON_CLK)       += clk-divider.o
> > +obj-$(CONFIG_COMMON_CLK)       += clkdomain.o
> >  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-factor.o
> >  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-rate.o
> >  obj-$(CONFIG_COMMON_CLK)       += clk-gate.o
> > diff --git a/drivers/clk/clkdomain.c b/drivers/clk/clkdomain.c
> > new file mode 100644
> > index 0000000..8c83f99
> > --- /dev/null
> > +++ b/drivers/clk/clkdomain.c
> 
> Naming is hard. I'm not sure clkdomain.c is the best expression. Maybe
> clk-alwon.c? I see ALWON used alot amongst hardware people who live in a
> world of eight-character naming limitations.

If you can have clk-fractional-divider.c in your subsystem, I'm sure
clk-always-on.c will be suitable.

> > @@ -0,0 +1,63 @@
> > +/*
> > + * Always on Clock Domain

=;-)

> > + * Copyright (C) 2015 STMicroelectronics – All Rights Reserved
> > + *
> > + * Author: Lee Jones <lee.jones@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/clk-private.h>
> 
> If this header still existed I would berate you mercilessly. Luckily for
> you it no longer exists and only causes a compile error ;-)

Noted.

You may wish to update: Documentation/clk.txt accordingly.

> > +#include <linux/clk-provider.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +static void ao_clock_domain_hog_clock(struct platform_device *pdev, int index)
> > +{
> > +       struct device_node *np = pdev->dev.of_node;
> > +       struct clk *clk;
> > +       int ret;
> > +
> > +       clk = of_clk_get(np, index);
> > +       if (IS_ERR(clk)) {
> > +               dev_warn(&pdev->dev, "Failed get clock %s[%d]: %li\n",
> > +                        np->full_name, index, PTR_ERR(clk));
> > +               return;
> > +       }
> > +
> > +       ret = clk_prepare_enable(clk);
> > +       if (ret)
> > +               dev_warn(&pdev->dev, "Failed to enable clock: %s\n", clk->name);
> > +}
> > +
> > +static int ao_clock_domain_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *np = pdev->dev.of_node;
> > +       int nclks, i;
> > +
> > +       nclks = of_count_phandle_with_args(np, "clocks", "#clock-cells");
> 
> Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> minutes writing that function and I need people to use it so I can get a
> return on my investment.

My middle name is RoI.  I'm on it.

> Otherwise the patch looks good. I believe that this method is targeting
> always-on clock in a production environment, which is different from the
> CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> hardware or dealing with a platform that has incomplete driver support.
> 
> I wonder if there is a clever way for existing clock providers
> (expressed in DT) to use this without having to create a separate node
> of clocks with the "always-on-clk-domain" flag. Possibly the common
> clock binding could declare some always-on flag that is standardized?
> Then the framework core could use this code easily. Not sure if that is
> a good idea though...

I think having both would be a good idea.  If all clocks supplied by a
provider should be left on, then a property inside the provider node
could be a good way to describe that.  In our case only some of them
are required, so I consider this concept to be better.

> > +
> > +       for (i = 0; i < nclks; i++)
> > +               ao_clock_domain_hog_clock(pdev, i);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id ao_clock_domain_match[] = {
> > +       { .compatible = "always-on-clk-domain" },
> > +       { },
> > +};
> > +
> > +static struct platform_driver ao_clock_domain_driver = {
> > +       .probe = ao_clock_domain_probe,
> > +       .driver = {
> > +               .name = "always-on-clk-domain",
> > +               .of_match_table = ao_clock_domain_match,
> > +       },
> > +};
> > +module_platform_driver(ao_clock_domain_driver);

On Mon, Feb 23, 2015 at 11:23 AM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Lee Jones (2015-02-18 08:15:00)
>> Much h/w contain clocks which if turned off would prove fatal.  The
>> only way to recover is to restart the board(s).  This driver takes
>> references to clocks which are required to be always-on in order to
>> prevent the common clk framework from trying to turn them off during
>> the clk_disabled_unused() procedure.

[...]

>> +static int ao_clock_domain_probe(struct platform_device *pdev)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       int nclks, i;
>> +
>> +       nclks = of_count_phandle_with_args(np, "clocks", "#clock-cells");
>
> Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> minutes writing that function and I need people to use it so I can get a
> return on my investment.
>
> Otherwise the patch looks good. I believe that this method is targeting
> always-on clock in a production environment, which is different from the
> CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> hardware or dealing with a platform that has incomplete driver support.

There is also the usecase of keep clocks on until I load a module that
properly handles my hardware (e.g simplefb). We have a simplefb node
with clocks and the simplefb driver jumps thru some hoops to hand-off
clocks to the real driver. I don't really like it and don't want to
see more examples. And there is the case of I thought I would never
manage this clock, but kernel subsystems evolve and now I want to
manage a clock. This should not require a DT update to do so.

Neither of these may be Lee's usecase, but I want to see them covered
by the binding.

> I wonder if there is a clever way for existing clock providers
> (expressed in DT) to use this without having to create a separate node
> of clocks with the "always-on-clk-domain" flag. Possibly the common
> clock binding could declare some always-on flag that is standardized?
> Then the framework core could use this code easily. Not sure if that is
> a good idea though...

I would prefer to see the always on clocks just listed within the
clock controller's node rather than creating made up nodes with clock
properties. This should be always-on until claimed IMO, but that
aspect is the OS's problem, not a DT problem.

Rob

On Wed, 25 Feb 2015, Rob Herring wrote:

> On Mon, Feb 23, 2015 at 11:23 AM, Mike Turquette <mturquette@linaro.org> wrote:
> > Quoting Lee Jones (2015-02-18 08:15:00)
> >> Much h/w contain clocks which if turned off would prove fatal.  The
> >> only way to recover is to restart the board(s).  This driver takes
> >> references to clocks which are required to be always-on in order to
> >> prevent the common clk framework from trying to turn them off during
> >> the clk_disabled_unused() procedure.
> 
> [...]
> 
> >> +static int ao_clock_domain_probe(struct platform_device *pdev)
> >> +{
> >> +       struct device_node *np = pdev->dev.of_node;
> >> +       int nclks, i;
> >> +
> >> +       nclks = of_count_phandle_with_args(np, "clocks", "#clock-cells");
> >
> > Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> > minutes writing that function and I need people to use it so I can get a
> > return on my investment.
> >
> > Otherwise the patch looks good. I believe that this method is targeting
> > always-on clock in a production environment, which is different from the
> > CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> > hardware or dealing with a platform that has incomplete driver support.
> 
> There is also the usecase of keep clocks on until I load a module that
> properly handles my hardware (e.g simplefb). We have a simplefb node
> with clocks and the simplefb driver jumps thru some hoops to hand-off
> clocks to the real driver. I don't really like it and don't want to
> see more examples. And there is the case of I thought I would never
> manage this clock, but kernel subsystems evolve and now I want to
> manage a clock. This should not require a DT update to do so.
> 
> Neither of these may be Lee's usecase, but I want to see them covered
> by the binding.
> 
> > I wonder if there is a clever way for existing clock providers
> > (expressed in DT) to use this without having to create a separate node
> > of clocks with the "always-on-clk-domain" flag. Possibly the common
> > clock binding could declare some always-on flag that is standardized?
> > Then the framework core could use this code easily. Not sure if that is
> > a good idea though...
> 
> I would prefer to see the always on clocks just listed within the
> clock controller's node rather than creating made up nodes with clock
> properties.

> This should be always-on until claimed IMO, but that
> aspect is the OS's problem, not a DT problem.

I disagree with this point.  There are likely to be many unclaimed,
but perfectly gateable clocks in a system, which will consume power
unnecessarily.  The clk framework does the right thing by turning all
unclaimed clocks off IMHO.  This only leaves a small use-case where we
need to artificially claim some which must not be gated.

The other way to do is, as you mentioned is list the clocks which must
stay on in the clock source node, but this will still require a
binding.  It will also require a much more complicated framework
driver.

    clkprovider@xxxxxxxx {
            always-on-clks = <1, 2, 4, 5, 7>;
    };

Quoting Rob Herring (2015-02-25 07:24:43)
> On Mon, Feb 23, 2015 at 11:23 AM, Mike Turquette <mturquette@linaro.org> wrote:
> > Quoting Lee Jones (2015-02-18 08:15:00)
> >> Much h/w contain clocks which if turned off would prove fatal.  The
> >> only way to recover is to restart the board(s).  This driver takes
> >> references to clocks which are required to be always-on in order to
> >> prevent the common clk framework from trying to turn them off during
> >> the clk_disabled_unused() procedure.
> 
> [...]
> 
> >> +static int ao_clock_domain_probe(struct platform_device *pdev)
> >> +{
> >> +       struct device_node *np = pdev->dev.of_node;
> >> +       int nclks, i;
> >> +
> >> +       nclks = of_count_phandle_with_args(np, "clocks", "#clock-cells");
> >
> > Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> > minutes writing that function and I need people to use it so I can get a
> > return on my investment.
> >
> > Otherwise the patch looks good. I believe that this method is targeting
> > always-on clock in a production environment, which is different from the
> > CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> > hardware or dealing with a platform that has incomplete driver support.
> 
> There is also the usecase of keep clocks on until I load a module that
> properly handles my hardware (e.g simplefb). We have a simplefb node
> with clocks and the simplefb driver jumps thru some hoops to hand-off
> clocks to the real driver. I don't really like it and don't want to
> see more examples. And there is the case of I thought I would never
> manage this clock, but kernel subsystems evolve and now I want to
> manage a clock. This should not require a DT update to do so.

Regarding the latter case, this is a violation of the intent of
always-on clocks. I think a firmly worded sentence in the binding should
suffice.

> 
> Neither of these may be Lee's usecase, but I want to see them covered
> by the binding.
> 
> > I wonder if there is a clever way for existing clock providers
> > (expressed in DT) to use this without having to create a separate node
> > of clocks with the "always-on-clk-domain" flag. Possibly the common
> > clock binding could declare some always-on flag that is standardized?
> > Then the framework core could use this code easily. Not sure if that is
> > a good idea though...
> 
> I would prefer to see the always on clocks just listed within the
> clock controller's node rather than creating made up nodes with clock
> properties. This should be always-on until claimed IMO, but that
> aspect is the OS's problem, not a DT problem.

So the common clock binding could have a new always-on list added to it,
but the clock can still be claimed and gated by drivers? I'll think on
it a bit but always-on is not the right name in that case. It is a more
general solution however (since it could still cover the sub-case of
clocks always remaining on since a driver never claims them).

Regards,
Mike

> 
> Rob

Quoting Lee Jones (2015-02-25 07:48:08)
> On Wed, 25 Feb 2015, Rob Herring wrote:
> 
> > On Mon, Feb 23, 2015 at 11:23 AM, Mike Turquette <mturquette@linaro.org> wrote:
> > > Quoting Lee Jones (2015-02-18 08:15:00)
> > >> Much h/w contain clocks which if turned off would prove fatal.  The
> > >> only way to recover is to restart the board(s).  This driver takes
> > >> references to clocks which are required to be always-on in order to
> > >> prevent the common clk framework from trying to turn them off during
> > >> the clk_disabled_unused() procedure.
> > 
> > [...]
> > 
> > >> +static int ao_clock_domain_probe(struct platform_device *pdev)
> > >> +{
> > >> +       struct device_node *np = pdev->dev.of_node;
> > >> +       int nclks, i;
> > >> +
> > >> +       nclks = of_count_phandle_with_args(np, "clocks", "#clock-cells");
> > >
> > > Minor nitpick: please use of_clk_get_parent_count. I spent a solid 5
> > > minutes writing that function and I need people to use it so I can get a
> > > return on my investment.
> > >
> > > Otherwise the patch looks good. I believe that this method is targeting
> > > always-on clock in a production environment, which is different from the
> > > CLK_IGNORE_UNUSED stuff which typically is helpful while bringing up new
> > > hardware or dealing with a platform that has incomplete driver support.
> > 
> > There is also the usecase of keep clocks on until I load a module that
> > properly handles my hardware (e.g simplefb). We have a simplefb node
> > with clocks and the simplefb driver jumps thru some hoops to hand-off
> > clocks to the real driver. I don't really like it and don't want to
> > see more examples. And there is the case of I thought I would never
> > manage this clock, but kernel subsystems evolve and now I want to
> > manage a clock. This should not require a DT update to do so.
> > 
> > Neither of these may be Lee's usecase, but I want to see them covered
> > by the binding.
> > 
> > > I wonder if there is a clever way for existing clock providers
> > > (expressed in DT) to use this without having to create a separate node
> > > of clocks with the "always-on-clk-domain" flag. Possibly the common
> > > clock binding could declare some always-on flag that is standardized?
> > > Then the framework core could use this code easily. Not sure if that is
> > > a good idea though...
> > 
> > I would prefer to see the always on clocks just listed within the
> > clock controller's node rather than creating made up nodes with clock
> > properties.
> 
> > This should be always-on until claimed IMO, but that
> > aspect is the OS's problem, not a DT problem.
> 
> I disagree with this point.  There are likely to be many unclaimed,
> but perfectly gateable clocks in a system, which will consume power
> unnecessarily.  The clk framework does the right thing by turning all
> unclaimed clocks off IMHO.  This only leaves a small use-case where we
> need to artificially claim some which must not be gated.

I might have misread both of your mails, but I think you two are
actually in agreement. You both support a common property which lists
the always-on clocks inside of the common clock binding, no?

> 
> The other way to do is, as you mentioned is list the clocks which must
> stay on in the clock source node, but this will still require a
> binding.  It will also require a much more complicated framework
> driver.
> 
>     clkprovider@xxxxxxxx {
>             always-on-clks = <1, 2, 4, 5, 7>;
>     };

This should pose no burden on the driver. Since always-on-clks is in the
common clock binding it should be handled by the framework core. At
clk_register-time we can check for always-on-clks, walk the list and see
if we have a match. It's ugly O(n^2) but it works.

Thoughts?

Mike

> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog