All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
  2017-04-12  4:03 ` Dong Aisheng
@ 2017-04-11 17:01   ` Florian Fainelli
  -1 siblings, 0 replies; 49+ messages in thread
From: Florian Fainelli @ 2017-04-11 17:01 UTC (permalink / raw)
  To: Dong Aisheng, linux-clk
  Cc: leonard.crestez, ping.bai, anson.huang, viresh.kumar, mturquette,
	rjw, linux-kernel, broonie, octavian.purdila, kernel,
	fabio.estevam, yibin.gong, shawnguo, sboyd, linux-arm-kernel

On 04/11/2017 09:03 PM, Dong Aisheng wrote:
> These helper function allows drivers to get several clk consumers in
> one operation.  If any of the clk cannot be acquired then any clks
> that were got will be put before returning to the caller.
> 
> The idea firstly came out when i wanted to clean up and optimize
> imx6q-cpufreq driver which needs to handle a lot of clocks as it
> becomes a bit mess.
> 
> e.g. drivers/cpufreq/imx6q-cpufreq.c
> 
> You will see we need get 7 clocks during driver probe.
> (Add there will be more, e.g. pll1, pll1_bypass, pll1_bypass_src,
> in the future when adding busfreq support).
> 
> static int imx6q_cpufreq_probe(struct platform_device *pdev)
> {
> ....
> 	arm_clk = clk_get(cpu_dev, "arm");
> 	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> 	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> 	step_clk = clk_get(cpu_dev, "step");
> 	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> 	if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
> 	    IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
> 		dev_err(cpu_dev, "failed to get clocks\n");
> 		ret = -ENOENT;
> 		goto put_clk;
> 	}
> 
> 	if (of_machine_is_compatible("fsl,imx6ul")) {
> 		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
> 		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
> 		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
> 			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
> 			ret = -ENOENT;
> 			goto put_clk;
> 		}
> 	}
> ....
> put_clk: 
>         if (!IS_ERR(arm_clk))
>                 clk_put(arm_clk);
>         if (!IS_ERR(pll1_sys_clk))
>                 clk_put(pll1_sys_clk);
> 	...........
> }
> 
> Together with the err path handling for each clocks, it does make
> things a bit ugly.
> 
> Since we already have regulator_bulk_get accessories, i thought we
> probably could introduce clk_bulk_get as well to handle such case to
> ease the driver owners' life. 
> 
> Besides IMX cpufreq driver, there is also some similar cases
> in kernel which could befinit from this api as well.
> e.g.
> drivers/cpufreq/tegra124-cpufreq.c
> drivers/cpufreq/s3c2412-cpufreq.c
> sound/soc/samsung/smdk_spdif.c
> arch/arm/mach-omap1/serial.c
> ...
> 
> And actually, if we handle clocks more than 3, then it might be
> worthy to try, which there is quite many manay in kernel and
> that probably could save a lot codes.
> 
> This is a RFC patch intending to bring up the idea to discuss.
> 
> Comments are welcome and appreciated!

Thanks a lot for doing this, we have historically done something similar
on ARCH_BRCMSTB platforms in our downstream tree with a concept of a
"software" clock which has multiple parents (yikes), using the bulk
accessors would essentially allow us to remove part of this hack, so I
am all in favor of this!

> 
> Dong Aisheng (3):
>   clk: add clk_bulk_get accessories
>   clk: add managed version of clk_bulk_get
>   cpufreq: imx6q: refine clk operations
> 
>  drivers/clk/clk-devres.c        |  36 +++++++++++
>  drivers/clk/clk.c               |  99 +++++++++++++++++++++++++++++
>  drivers/clk/clkdev.c            |  42 +++++++++++++
>  drivers/cpufreq/imx6q-cpufreq.c | 119 ++++++++++++++++-------------------
>  include/linux/clk.h             | 135 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 365 insertions(+), 66 deletions(-)
> 


-- 
Florian

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
@ 2017-04-11 17:01   ` Florian Fainelli
  0 siblings, 0 replies; 49+ messages in thread
From: Florian Fainelli @ 2017-04-11 17:01 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/11/2017 09:03 PM, Dong Aisheng wrote:
> These helper function allows drivers to get several clk consumers in
> one operation.  If any of the clk cannot be acquired then any clks
> that were got will be put before returning to the caller.
> 
> The idea firstly came out when i wanted to clean up and optimize
> imx6q-cpufreq driver which needs to handle a lot of clocks as it
> becomes a bit mess.
> 
> e.g. drivers/cpufreq/imx6q-cpufreq.c
> 
> You will see we need get 7 clocks during driver probe.
> (Add there will be more, e.g. pll1, pll1_bypass, pll1_bypass_src,
> in the future when adding busfreq support).
> 
> static int imx6q_cpufreq_probe(struct platform_device *pdev)
> {
> ....
> 	arm_clk = clk_get(cpu_dev, "arm");
> 	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> 	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> 	step_clk = clk_get(cpu_dev, "step");
> 	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> 	if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
> 	    IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
> 		dev_err(cpu_dev, "failed to get clocks\n");
> 		ret = -ENOENT;
> 		goto put_clk;
> 	}
> 
> 	if (of_machine_is_compatible("fsl,imx6ul")) {
> 		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
> 		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
> 		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
> 			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
> 			ret = -ENOENT;
> 			goto put_clk;
> 		}
> 	}
> ....
> put_clk: 
>         if (!IS_ERR(arm_clk))
>                 clk_put(arm_clk);
>         if (!IS_ERR(pll1_sys_clk))
>                 clk_put(pll1_sys_clk);
> 	...........
> }
> 
> Together with the err path handling for each clocks, it does make
> things a bit ugly.
> 
> Since we already have regulator_bulk_get accessories, i thought we
> probably could introduce clk_bulk_get as well to handle such case to
> ease the driver owners' life. 
> 
> Besides IMX cpufreq driver, there is also some similar cases
> in kernel which could befinit from this api as well.
> e.g.
> drivers/cpufreq/tegra124-cpufreq.c
> drivers/cpufreq/s3c2412-cpufreq.c
> sound/soc/samsung/smdk_spdif.c
> arch/arm/mach-omap1/serial.c
> ...
> 
> And actually, if we handle clocks more than 3, then it might be
> worthy to try, which there is quite many manay in kernel and
> that probably could save a lot codes.
> 
> This is a RFC patch intending to bring up the idea to discuss.
> 
> Comments are welcome and appreciated!

Thanks a lot for doing this, we have historically done something similar
on ARCH_BRCMSTB platforms in our downstream tree with a concept of a
"software" clock which has multiple parents (yikes), using the bulk
accessors would essentially allow us to remove part of this hack, so I
am all in favor of this!

> 
> Dong Aisheng (3):
>   clk: add clk_bulk_get accessories
>   clk: add managed version of clk_bulk_get
>   cpufreq: imx6q: refine clk operations
> 
>  drivers/clk/clk-devres.c        |  36 +++++++++++
>  drivers/clk/clk.c               |  99 +++++++++++++++++++++++++++++
>  drivers/clk/clkdev.c            |  42 +++++++++++++
>  drivers/cpufreq/imx6q-cpufreq.c | 119 ++++++++++++++++-------------------
>  include/linux/clk.h             | 135 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 365 insertions(+), 66 deletions(-)
> 


-- 
Florian

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-12  4:03   ` Dong Aisheng
@ 2017-04-11 17:19     ` Leonard Crestez
  -1 siblings, 0 replies; 49+ messages in thread
From: Leonard Crestez @ 2017-04-11 17:19 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: linux-kernel, linux-arm-kernel, kernel, broonie, yibin.gong, rjw,
	viresh.kumar, mturquette, sboyd, shawnguo, fabio.estevam,
	anson.huang, ping.bai, octavian.purdila, linux-clk

On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> +/**
> + * clk_bulk_enable - ungate a bulk of clocks
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_enable must not sleep
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> +{
> +       int ret;
> +       int i;
> +
> +       for (i = 0; i < num_clks; i++) {
> +               ret = clk_enable(clks[i].clk);
> +               if (ret) {
> +                       pr_err("Failed to enable clk '%s': %d\n",
> +                               clks[i].id, ret);
> +                       goto err;
> +               }
> +       }
> +
> +       return 0;
> +
> +err:
> +       while (--i >= 0)
> +               clk_put(clks[i].clk);

Shouldn't this be clk_disable?

And you can probably use clk_bulk_disable(i, clks) instead

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-11 17:19     ` Leonard Crestez
  0 siblings, 0 replies; 49+ messages in thread
From: Leonard Crestez @ 2017-04-11 17:19 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> +/**
> + * clk_bulk_enable - ungate a bulk of clocks
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_enable must not sleep
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> +{
> +???????int ret;
> +???????int i;
> +
> +???????for (i = 0; i < num_clks; i++) {
> +???????????????ret = clk_enable(clks[i].clk);
> +???????????????if (ret) {
> +???????????????????????pr_err("Failed to enable clk '%s': %d\n",
> +???????????????????????????????clks[i].id, ret);
> +???????????????????????goto err;
> +???????????????}
> +???????}
> +
> +???????return 0;
> +
> +err:
> +???????while (--i >= 0)
> +???????????????clk_put(clks[i].clk);

Shouldn't this be clk_disable?

And you can probably use clk_bulk_disable(i, clks) instead

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 3/3] cpufreq: imx6q: refine clk operations
  2017-04-12  4:03   ` Dong Aisheng
@ 2017-04-11 17:48     ` Leonard Crestez
  -1 siblings, 0 replies; 49+ messages in thread
From: Leonard Crestez @ 2017-04-11 17:48 UTC (permalink / raw)
  To: Dong Aisheng, linux-clk
  Cc: linux-kernel, linux-arm-kernel, kernel, broonie, yibin.gong, rjw,
	viresh.kumar, mturquette, sboyd, shawnguo, fabio.estevam,
	anson.huang, ping.bai, octavian.purdila

On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> +static int num_clks;
> +static struct clk_bulk_data clks[] = {
> +	{ .id = "arm" },
> +	{ .id = "pll1_sys" },
> +	{ .id = "step" },
> +	{ .id = "pll1_sw" },
> +	{ .id = "pll2_pfd2_396m" },
> +	{ .id = "pll2_bus" },
> +	{ .id = "secondary_sel" },
> +};

The .id is only required for initialization, it seems strange to keep
it around runtime data. It might be better for this API to work with an
array of clk* and separate array of names (or clk_bulk_init_data if we
need flags). Variable references would be shorter and it would allow
more data to be const.

> -put_clk:
> -	if (!IS_ERR(arm_clk))
> -		clk_put(arm_clk);
> -	if (!IS_ERR(pll1_sys_clk))
> -		clk_put(pll1_sys_clk);
> -	if (!IS_ERR(pll1_sw_clk))
> -		clk_put(pll1_sw_clk);
> -	if (!IS_ERR(step_clk))
> -		clk_put(step_clk);
> -	if (!IS_ERR(pll2_pfd2_396m_clk))
> -		clk_put(pll2_pfd2_396m_clk);
> -	if (!IS_ERR(pll2_bus_clk))
> -		clk_put(pll2_bus_clk);
> -	if (!IS_ERR(secondary_sel_clk))
> -		clk_put(secondary_sel_clk);
> +
> +	clk_bulk_put(num_clks, clks);
> +put_node:
>  	of_node_put(np);
> +
>  	return ret;
>  }


My subjective opinion is that a better way to clean this up would be to
have a single imx6q_cpufreq_clean function that takes all resources and
does stuff like:

if (!IS_ERR(clk)) clk_put(clk);
clk = NULL;

That function can be called from both _remove and failed _probe without
having to keep track of which resources have been allocated until then.
Just free and NULL all clocks/regulators and simplify control flow.

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 3/3] cpufreq: imx6q: refine clk operations
@ 2017-04-11 17:48     ` Leonard Crestez
  0 siblings, 0 replies; 49+ messages in thread
From: Leonard Crestez @ 2017-04-11 17:48 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> +static int num_clks;
> +static struct clk_bulk_data clks[] = {
> +	{ .id = "arm" },
> +	{ .id = "pll1_sys" },
> +	{ .id = "step" },
> +	{ .id = "pll1_sw" },
> +	{ .id = "pll2_pfd2_396m" },
> +	{ .id = "pll2_bus" },
> +	{ .id = "secondary_sel" },
> +};

The .id is only required for initialization, it seems strange to keep
it around runtime data. It might be better for this API to work with an
array of clk* and separate array of names (or clk_bulk_init_data if we
need flags). Variable references would be shorter and it would allow
more data to be const.

> -put_clk:
> -	if (!IS_ERR(arm_clk))
> -		clk_put(arm_clk);
> -	if (!IS_ERR(pll1_sys_clk))
> -		clk_put(pll1_sys_clk);
> -	if (!IS_ERR(pll1_sw_clk))
> -		clk_put(pll1_sw_clk);
> -	if (!IS_ERR(step_clk))
> -		clk_put(step_clk);
> -	if (!IS_ERR(pll2_pfd2_396m_clk))
> -		clk_put(pll2_pfd2_396m_clk);
> -	if (!IS_ERR(pll2_bus_clk))
> -		clk_put(pll2_bus_clk);
> -	if (!IS_ERR(secondary_sel_clk))
> -		clk_put(secondary_sel_clk);
> +
> +	clk_bulk_put(num_clks, clks);
> +put_node:
> ?	of_node_put(np);
> +
> ?	return ret;
> ?}


My subjective opinion is that a better way to clean this up would be to
have a single imx6q_cpufreq_clean function that takes all resources and
does stuff like:

if (!IS_ERR(clk)) clk_put(clk);
clk = NULL;

That function can be called from both _remove and failed _probe without
having to keep track of which resources have been allocated until then.
Just free and NULL all clocks/regulators and simplify control flow.

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
@ 2017-04-12  4:03 ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-12  4:03 UTC (permalink / raw)
  To: linux-clk
  Cc: linux-kernel, linux-arm-kernel, aisheng.dong, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, sboyd, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

These helper function allows drivers to get several clk consumers in
one operation.  If any of the clk cannot be acquired then any clks
that were got will be put before returning to the caller.

The idea firstly came out when i wanted to clean up and optimize
imx6q-cpufreq driver which needs to handle a lot of clocks as it
becomes a bit mess.

e.g. drivers/cpufreq/imx6q-cpufreq.c

You will see we need get 7 clocks during driver probe.
(Add there will be more, e.g. pll1, pll1_bypass, pll1_bypass_src,
in the future when adding busfreq support).

static int imx6q_cpufreq_probe(struct platform_device *pdev)
{
....
	arm_clk = clk_get(cpu_dev, "arm");
	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
	step_clk = clk_get(cpu_dev, "step");
	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
	if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
	    IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
		dev_err(cpu_dev, "failed to get clocks\n");
		ret = -ENOENT;
		goto put_clk;
	}

	if (of_machine_is_compatible("fsl,imx6ul")) {
		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
			ret = -ENOENT;
			goto put_clk;
		}
	}
....
put_clk: 
        if (!IS_ERR(arm_clk))
                clk_put(arm_clk);
        if (!IS_ERR(pll1_sys_clk))
                clk_put(pll1_sys_clk);
	...........
}

Together with the err path handling for each clocks, it does make
things a bit ugly.

Since we already have regulator_bulk_get accessories, i thought we
probably could introduce clk_bulk_get as well to handle such case to
ease the driver owners' life. 

Besides IMX cpufreq driver, there is also some similar cases
in kernel which could befinit from this api as well.
e.g.
drivers/cpufreq/tegra124-cpufreq.c
drivers/cpufreq/s3c2412-cpufreq.c
sound/soc/samsung/smdk_spdif.c
arch/arm/mach-omap1/serial.c
...

And actually, if we handle clocks more than 3, then it might be
worthy to try, which there is quite many manay in kernel and
that probably could save a lot codes.

This is a RFC patch intending to bring up the idea to discuss.

Comments are welcome and appreciated!

Dong Aisheng (3):
  clk: add clk_bulk_get accessories
  clk: add managed version of clk_bulk_get
  cpufreq: imx6q: refine clk operations

 drivers/clk/clk-devres.c        |  36 +++++++++++
 drivers/clk/clk.c               |  99 +++++++++++++++++++++++++++++
 drivers/clk/clkdev.c            |  42 +++++++++++++
 drivers/cpufreq/imx6q-cpufreq.c | 119 ++++++++++++++++-------------------
 include/linux/clk.h             | 135 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 365 insertions(+), 66 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
@ 2017-04-12  4:03 ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-12  4:03 UTC (permalink / raw)
  To: linux-arm-kernel

These helper function allows drivers to get several clk consumers in
one operation.  If any of the clk cannot be acquired then any clks
that were got will be put before returning to the caller.

The idea firstly came out when i wanted to clean up and optimize
imx6q-cpufreq driver which needs to handle a lot of clocks as it
becomes a bit mess.

e.g. drivers/cpufreq/imx6q-cpufreq.c

You will see we need get 7 clocks during driver probe.
(Add there will be more, e.g. pll1, pll1_bypass, pll1_bypass_src,
in the future when adding busfreq support).

static int imx6q_cpufreq_probe(struct platform_device *pdev)
{
....
	arm_clk = clk_get(cpu_dev, "arm");
	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
	step_clk = clk_get(cpu_dev, "step");
	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
	if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
	    IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
		dev_err(cpu_dev, "failed to get clocks\n");
		ret = -ENOENT;
		goto put_clk;
	}

	if (of_machine_is_compatible("fsl,imx6ul")) {
		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
			ret = -ENOENT;
			goto put_clk;
		}
	}
....
put_clk: 
        if (!IS_ERR(arm_clk))
                clk_put(arm_clk);
        if (!IS_ERR(pll1_sys_clk))
                clk_put(pll1_sys_clk);
	...........
}

Together with the err path handling for each clocks, it does make
things a bit ugly.

Since we already have regulator_bulk_get accessories, i thought we
probably could introduce clk_bulk_get as well to handle such case to
ease the driver owners' life. 

Besides IMX cpufreq driver, there is also some similar cases
in kernel which could befinit from this api as well.
e.g.
drivers/cpufreq/tegra124-cpufreq.c
drivers/cpufreq/s3c2412-cpufreq.c
sound/soc/samsung/smdk_spdif.c
arch/arm/mach-omap1/serial.c
...

And actually, if we handle clocks more than 3, then it might be
worthy to try, which there is quite many manay in kernel and
that probably could save a lot codes.

This is a RFC patch intending to bring up the idea to discuss.

Comments are welcome and appreciated!

Dong Aisheng (3):
  clk: add clk_bulk_get accessories
  clk: add managed version of clk_bulk_get
  cpufreq: imx6q: refine clk operations

 drivers/clk/clk-devres.c        |  36 +++++++++++
 drivers/clk/clk.c               |  99 +++++++++++++++++++++++++++++
 drivers/clk/clkdev.c            |  42 +++++++++++++
 drivers/cpufreq/imx6q-cpufreq.c | 119 ++++++++++++++++-------------------
 include/linux/clk.h             | 135 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 365 insertions(+), 66 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-12  4:03 ` Dong Aisheng
@ 2017-04-12  4:03   ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-12  4:03 UTC (permalink / raw)
  To: linux-clk
  Cc: linux-kernel, linux-arm-kernel, aisheng.dong, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, sboyd, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

These helper function allows drivers to get several clk consumers in
one operation.  If any of the clk cannot be acquired then any clks
that were got will be put before returning to the caller.

This can relieve the driver owners' life who needs to handle many clocks,
as well as each clock error reporting.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Anson Huang <anson.huang@nxp.com>
Cc: Robin Gong <yibin.gong@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: Octavian Purdila <octavian.purdila@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/clk/clk.c    |  99 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clkdev.c |  42 +++++++++++++++++++
 include/linux/clk.h  | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cddddbe..622f8069 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_unprepare);
 
+/**
+ * clk_bulk_unprepare - undo preparation of a bulk of clock sources
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
+{
+	int i;
+
+	for (i = 0; i < num_clks; i++)
+		clk_unprepare(clks[i].clk);
+}
+EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
+
 static int clk_core_prepare(struct clk_core *core)
 {
 	int ret = 0;
@@ -584,6 +601,38 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+/**
+ * clk_bulk_prepare - prepare a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_prepare may sleep, which differentiates it from clk_bulk_enable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+
+int __must_check clk_bulk_prepare(int num_clks, struct clk_bulk_data *clks)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < num_clks; i++) {
+		ret = clk_prepare(clks[i].clk);
+		if (ret) {
+			pr_err("Failed to prepare clk '%s': %d\n",
+				clks[i].id, ret);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0)
+		clk_unprepare(clks[i].clk);
+
+	return  ret;
+}
+
 static void clk_core_disable(struct clk_core *core)
 {
 	lockdep_assert_held(&enable_lock);
@@ -640,6 +689,24 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+/**
+ * clk_bulk_disable - gate a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_disable must not sleep, which differentiates it from
+ * clk_bulk_unprepare. clk_bulk_disable must be called before
+ * clk_bulk_unprepare.
+ */
+void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks)
+{
+	int i;
+
+	for (i = 0; i < num_clks; i++)
+		clk_disable(clks[i].clk);
+}
+EXPORT_SYMBOL_GPL(clk_bulk_disable);
+
 static int clk_core_enable(struct clk_core *core)
 {
 	int ret = 0;
@@ -709,6 +776,38 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+/**
+ * clk_bulk_enable - ungate a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_enable must not sleep
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < num_clks; i++) {
+		ret = clk_enable(clks[i].clk);
+		if (ret) {
+			pr_err("Failed to enable clk '%s': %d\n",
+				clks[i].id, ret);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0)
+		clk_put(clks[i].clk);
+
+	return  ret;
+}
+EXPORT_SYMBOL_GPL(clk_bulk_enable);
+
 static int clk_core_prepare_enable(struct clk_core *core)
 {
 	int ret;
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index bb8a77a..141f2e6 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -209,12 +209,54 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL(clk_get);
 
+int clk_bulk_get(struct device *dev, int num_clks,
+		 struct clk_bulk_data *clks)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < num_clks; i++)
+		clks[i].clk = NULL;
+
+	for (i = 0; i < num_clks; i++) {
+		clks[i].clk = clk_get(dev, clks[i].id);
+		if (IS_ERR(clks[i].clk)) {
+			ret = PTR_ERR(clks[i].clk);
+			dev_err(dev, "Failed to get clk '%s': %d\n",
+				clks[i].id, ret);
+			clks[i].clk = NULL;
+			goto err;
+		}
+
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0)
+		clk_put(clks[i].clk);
+
+	return ret;
+}
+EXPORT_SYMBOL(clk_bulk_get);
+
 void clk_put(struct clk *clk)
 {
 	__clk_put(clk);
 }
 EXPORT_SYMBOL(clk_put);
 
+void clk_bulk_put(int num_clks, struct clk_bulk_data *clks)
+{
+	int i;
+
+	for (i = 0; i < num_clks; i++) {
+		clk_put(clks[i].clk);
+		clks[i].clk = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(clk_bulk_put);
+
 static void __clkdev_add(struct clk_lookup *cl)
 {
 	mutex_lock(&clocks_mutex);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index e9d36b3..1d05b66 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -77,6 +77,21 @@ struct clk_notifier_data {
 	unsigned long		new_rate;
 };
 
+/**
+ * struct clk_bulk_data - Data used for bulk clk operations.
+ *
+ * @id: clock consumer ID
+ * @clk: struct clk * to store the associated clock
+ *
+ * The CLK APIs provide a series of clk_bulk_() API calls as
+ * a convenience to consumers which require multiple clks.  This
+ * structure is used to manage data for these calls.
+ */
+struct clk_bulk_data {
+	const char		*id;
+	struct clk		*clk;
+};
+
 #ifdef CONFIG_COMMON_CLK
 
 /**
@@ -185,12 +200,19 @@ static inline bool clk_is_match(const struct clk *p, const struct clk *q)
  */
 #ifdef CONFIG_HAVE_CLK_PREPARE
 int clk_prepare(struct clk *clk);
+int __must_check clk_bulk_prepare(int num_clks, struct clk_bulk_data *clks);
 #else
 static inline int clk_prepare(struct clk *clk)
 {
 	might_sleep();
 	return 0;
 }
+
+static inline int clk_bulk_prepare(int num_clks, struct clk_bulk_data *clks)
+{
+	might_sleep();
+	return 0;
+}
 #endif
 
 /**
@@ -204,11 +226,16 @@ static inline int clk_prepare(struct clk *clk)
  */
 #ifdef CONFIG_HAVE_CLK_PREPARE
 void clk_unprepare(struct clk *clk);
+void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks);
 #else
 static inline void clk_unprepare(struct clk *clk)
 {
 	might_sleep();
 }
+static inline void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
+{
+	might_sleep();
+}
 #endif
 
 #ifdef CONFIG_HAVE_CLK
@@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
 struct clk *clk_get(struct device *dev, const char *id);
 
 /**
+ * clk_bulk_get - lookup and obtain a number of references to clock producer.
+ * @dev: device for clock "consumer"
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * This helper function allows drivers to get several clk consumers in one
+ * operation. If any of the clk cannot be acquired then any clks
+ * that were obtained will be freed before returning to the caller.
+ *
+ * Returns 0 if all clocks specified in clk_bulk_data table are obtained
+ * successfully, or valid IS_ERR() condition containing errno.
+ * The implementation uses @dev and @clk_bulk_data.id to determine the
+ * clock consumer, and thereby the clock producer.
+ * (IOW, @id may be identical strings, but clk_get may return different
+ * clock producers depending on @dev.) The clock returned is stored in
+ * each @clk_bulk_data.clk field.
+ *
+ * Drivers must assume that the clock source is not enabled.
+ *
+ * clk_bulk_get should not be called from within interrupt context.
+ */
+
+int __must_check clk_bulk_get(struct device *dev, int num_clks,
+			      struct clk_bulk_data *clks);
+
+/**
  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
  * @dev: device for clock "consumer"
  * @id: clock consumer ID
@@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
 int clk_enable(struct clk *clk);
 
 /**
+ * clk_bulk_enable - inform the system when the bulk of clock source should
+ *		     be running.
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * If the clock can not be enabled/disabled all, this should return success.
+ *
+ * May be called from atomic contexts.
+ *
+ * Returns success (0) or negative errno.
+ */
+int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
+
+/**
  * clk_disable - inform the system when the clock source is no longer required.
  * @clk: clock source
  *
@@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_bulk_disable - inform the system when the bulk of clock source is no
+ *		      longer required.
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * Inform the system that a bulk of clock source is no longer required by
+ * a driver and may be shut down.
+ *
+ * May be called from atomic contexts.
+ *
+ * Implementation detail: if the bulk of clock source is shared between
+ * multiple drivers, clk_bulk_enable() calls must be balanced by the
+ * same number of clk_bulk_disable() calls for the clock source to be
+ * disabled.
+ */
+void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
@@ -314,6 +399,19 @@ unsigned long clk_get_rate(struct clk *clk);
 void clk_put(struct clk *clk);
 
 /**
+ * clk_bulk_put	- "free" the clock source
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * Note: drivers must ensure that all clk_bulk_enable calls made on this
+ * clock source are balanced by clk_bulk_disable calls prior to calling
+ * this function.
+ *
+ * clk_bulk_put should not be called from within interrupt context.
+ */
+void clk_bulk_put(int num_clks, struct clk_bulk_data *clks);
+
+/**
  * devm_clk_put	- "free" a managed clock source
  * @dev: device used to acquire the clock
  * @clk: clock source acquired with devm_clk_get()
@@ -445,6 +543,12 @@ static inline struct clk *clk_get(struct device *dev, const char *id)
 	return NULL;
 }
 
+static inline int clk_bulk_get(struct device *dev, int num_clks,
+			       struct clk_bulk_data *clks)
+{
+	return NULL;
+}
+
 static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 {
 	return NULL;
@@ -458,6 +562,8 @@ static inline struct clk *devm_get_clk_from_child(struct device *dev,
 
 static inline void clk_put(struct clk *clk) {}
 
+static inline void clk_bulk_put(int num_clks, struct clk_bulk_data *clks) {}
+
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
 static inline int clk_enable(struct clk *clk)
@@ -465,8 +571,17 @@ static inline int clk_enable(struct clk *clk)
 	return 0;
 }
 
+static inline int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
+{
+	return 0;
+}
+
 static inline void clk_disable(struct clk *clk) {}
 
+
+static inline void clk_bulk_disable(int num_clks,
+				    struct clk_bulk_data *clks) {}
+
 static inline unsigned long clk_get_rate(struct clk *clk)
 {
 	return 0;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-12  4:03   ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-12  4:03 UTC (permalink / raw)
  To: linux-arm-kernel

These helper function allows drivers to get several clk consumers in
one operation.  If any of the clk cannot be acquired then any clks
that were got will be put before returning to the caller.

This can relieve the driver owners' life who needs to handle many clocks,
as well as each clock error reporting.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Anson Huang <anson.huang@nxp.com>
Cc: Robin Gong <yibin.gong@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: Octavian Purdila <octavian.purdila@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/clk/clk.c    |  99 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clkdev.c |  42 +++++++++++++++++++
 include/linux/clk.h  | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cddddbe..622f8069 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_unprepare);
 
+/**
+ * clk_bulk_unprepare - undo preparation of a bulk of clock sources
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
+{
+	int i;
+
+	for (i = 0; i < num_clks; i++)
+		clk_unprepare(clks[i].clk);
+}
+EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
+
 static int clk_core_prepare(struct clk_core *core)
 {
 	int ret = 0;
@@ -584,6 +601,38 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+/**
+ * clk_bulk_prepare - prepare a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_prepare may sleep, which differentiates it from clk_bulk_enable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+
+int __must_check clk_bulk_prepare(int num_clks, struct clk_bulk_data *clks)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < num_clks; i++) {
+		ret = clk_prepare(clks[i].clk);
+		if (ret) {
+			pr_err("Failed to prepare clk '%s': %d\n",
+				clks[i].id, ret);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0)
+		clk_unprepare(clks[i].clk);
+
+	return  ret;
+}
+
 static void clk_core_disable(struct clk_core *core)
 {
 	lockdep_assert_held(&enable_lock);
@@ -640,6 +689,24 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+/**
+ * clk_bulk_disable - gate a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_disable must not sleep, which differentiates it from
+ * clk_bulk_unprepare. clk_bulk_disable must be called before
+ * clk_bulk_unprepare.
+ */
+void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks)
+{
+	int i;
+
+	for (i = 0; i < num_clks; i++)
+		clk_disable(clks[i].clk);
+}
+EXPORT_SYMBOL_GPL(clk_bulk_disable);
+
 static int clk_core_enable(struct clk_core *core)
 {
 	int ret = 0;
@@ -709,6 +776,38 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+/**
+ * clk_bulk_enable - ungate a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_enable must not sleep
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < num_clks; i++) {
+		ret = clk_enable(clks[i].clk);
+		if (ret) {
+			pr_err("Failed to enable clk '%s': %d\n",
+				clks[i].id, ret);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0)
+		clk_put(clks[i].clk);
+
+	return  ret;
+}
+EXPORT_SYMBOL_GPL(clk_bulk_enable);
+
 static int clk_core_prepare_enable(struct clk_core *core)
 {
 	int ret;
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index bb8a77a..141f2e6 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -209,12 +209,54 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL(clk_get);
 
+int clk_bulk_get(struct device *dev, int num_clks,
+		 struct clk_bulk_data *clks)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < num_clks; i++)
+		clks[i].clk = NULL;
+
+	for (i = 0; i < num_clks; i++) {
+		clks[i].clk = clk_get(dev, clks[i].id);
+		if (IS_ERR(clks[i].clk)) {
+			ret = PTR_ERR(clks[i].clk);
+			dev_err(dev, "Failed to get clk '%s': %d\n",
+				clks[i].id, ret);
+			clks[i].clk = NULL;
+			goto err;
+		}
+
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0)
+		clk_put(clks[i].clk);
+
+	return ret;
+}
+EXPORT_SYMBOL(clk_bulk_get);
+
 void clk_put(struct clk *clk)
 {
 	__clk_put(clk);
 }
 EXPORT_SYMBOL(clk_put);
 
+void clk_bulk_put(int num_clks, struct clk_bulk_data *clks)
+{
+	int i;
+
+	for (i = 0; i < num_clks; i++) {
+		clk_put(clks[i].clk);
+		clks[i].clk = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(clk_bulk_put);
+
 static void __clkdev_add(struct clk_lookup *cl)
 {
 	mutex_lock(&clocks_mutex);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index e9d36b3..1d05b66 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -77,6 +77,21 @@ struct clk_notifier_data {
 	unsigned long		new_rate;
 };
 
+/**
+ * struct clk_bulk_data - Data used for bulk clk operations.
+ *
+ * @id: clock consumer ID
+ * @clk: struct clk * to store the associated clock
+ *
+ * The CLK APIs provide a series of clk_bulk_() API calls as
+ * a convenience to consumers which require multiple clks.  This
+ * structure is used to manage data for these calls.
+ */
+struct clk_bulk_data {
+	const char		*id;
+	struct clk		*clk;
+};
+
 #ifdef CONFIG_COMMON_CLK
 
 /**
@@ -185,12 +200,19 @@ static inline bool clk_is_match(const struct clk *p, const struct clk *q)
  */
 #ifdef CONFIG_HAVE_CLK_PREPARE
 int clk_prepare(struct clk *clk);
+int __must_check clk_bulk_prepare(int num_clks, struct clk_bulk_data *clks);
 #else
 static inline int clk_prepare(struct clk *clk)
 {
 	might_sleep();
 	return 0;
 }
+
+static inline int clk_bulk_prepare(int num_clks, struct clk_bulk_data *clks)
+{
+	might_sleep();
+	return 0;
+}
 #endif
 
 /**
@@ -204,11 +226,16 @@ static inline int clk_prepare(struct clk *clk)
  */
 #ifdef CONFIG_HAVE_CLK_PREPARE
 void clk_unprepare(struct clk *clk);
+void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks);
 #else
 static inline void clk_unprepare(struct clk *clk)
 {
 	might_sleep();
 }
+static inline void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
+{
+	might_sleep();
+}
 #endif
 
 #ifdef CONFIG_HAVE_CLK
@@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
 struct clk *clk_get(struct device *dev, const char *id);
 
 /**
+ * clk_bulk_get - lookup and obtain a number of references to clock producer.
+ * @dev: device for clock "consumer"
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * This helper function allows drivers to get several clk consumers in one
+ * operation. If any of the clk cannot be acquired then any clks
+ * that were obtained will be freed before returning to the caller.
+ *
+ * Returns 0 if all clocks specified in clk_bulk_data table are obtained
+ * successfully, or valid IS_ERR() condition containing errno.
+ * The implementation uses @dev and @clk_bulk_data.id to determine the
+ * clock consumer, and thereby the clock producer.
+ * (IOW, @id may be identical strings, but clk_get may return different
+ * clock producers depending on @dev.) The clock returned is stored in
+ * each @clk_bulk_data.clk field.
+ *
+ * Drivers must assume that the clock source is not enabled.
+ *
+ * clk_bulk_get should not be called from within interrupt context.
+ */
+
+int __must_check clk_bulk_get(struct device *dev, int num_clks,
+			      struct clk_bulk_data *clks);
+
+/**
  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
  * @dev: device for clock "consumer"
  * @id: clock consumer ID
@@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
 int clk_enable(struct clk *clk);
 
 /**
+ * clk_bulk_enable - inform the system when the bulk of clock source should
+ *		     be running.
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * If the clock can not be enabled/disabled all, this should return success.
+ *
+ * May be called from atomic contexts.
+ *
+ * Returns success (0) or negative errno.
+ */
+int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
+
+/**
  * clk_disable - inform the system when the clock source is no longer required.
  * @clk: clock source
  *
@@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_bulk_disable - inform the system when the bulk of clock source is no
+ *		      longer required.
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * Inform the system that a bulk of clock source is no longer required by
+ * a driver and may be shut down.
+ *
+ * May be called from atomic contexts.
+ *
+ * Implementation detail: if the bulk of clock source is shared between
+ * multiple drivers, clk_bulk_enable() calls must be balanced by the
+ * same number of clk_bulk_disable() calls for the clock source to be
+ * disabled.
+ */
+void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
@@ -314,6 +399,19 @@ unsigned long clk_get_rate(struct clk *clk);
 void clk_put(struct clk *clk);
 
 /**
+ * clk_bulk_put	- "free" the clock source
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * Note: drivers must ensure that all clk_bulk_enable calls made on this
+ * clock source are balanced by clk_bulk_disable calls prior to calling
+ * this function.
+ *
+ * clk_bulk_put should not be called from within interrupt context.
+ */
+void clk_bulk_put(int num_clks, struct clk_bulk_data *clks);
+
+/**
  * devm_clk_put	- "free" a managed clock source
  * @dev: device used to acquire the clock
  * @clk: clock source acquired with devm_clk_get()
@@ -445,6 +543,12 @@ static inline struct clk *clk_get(struct device *dev, const char *id)
 	return NULL;
 }
 
+static inline int clk_bulk_get(struct device *dev, int num_clks,
+			       struct clk_bulk_data *clks)
+{
+	return NULL;
+}
+
 static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 {
 	return NULL;
@@ -458,6 +562,8 @@ static inline struct clk *devm_get_clk_from_child(struct device *dev,
 
 static inline void clk_put(struct clk *clk) {}
 
+static inline void clk_bulk_put(int num_clks, struct clk_bulk_data *clks) {}
+
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
 static inline int clk_enable(struct clk *clk)
@@ -465,8 +571,17 @@ static inline int clk_enable(struct clk *clk)
 	return 0;
 }
 
+static inline int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
+{
+	return 0;
+}
+
 static inline void clk_disable(struct clk *clk) {}
 
+
+static inline void clk_bulk_disable(int num_clks,
+				    struct clk_bulk_data *clks) {}
+
 static inline unsigned long clk_get_rate(struct clk *clk)
 {
 	return 0;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
  2017-04-12  4:03 ` Dong Aisheng
@ 2017-04-12  4:03   ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-12  4:03 UTC (permalink / raw)
  To: linux-clk
  Cc: linux-kernel, linux-arm-kernel, aisheng.dong, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, sboyd, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

This patch introduces the managed version of clk_bulk_get.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Anson Huang <anson.huang@nxp.com>
Cc: Robin Gong <yibin.gong@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: Octavian Purdila <octavian.purdila@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/clk/clk-devres.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 22 +++++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 3a218c3..c7fb31d 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -34,6 +34,42 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get);
 
+struct clk_bulk_devres {
+	struct clk_bulk_data *clks;
+	int num_clks;
+};
+
+static void devm_clk_bulk_release(struct device *dev, void *res)
+{
+	struct clk_bulk_devres *devres = res;
+
+	clk_bulk_put(devres->num_clks, devres->clks);
+}
+
+int devm_clk_bulk_get(struct device *dev, int num_clks,
+		      struct clk_bulk_data *clks)
+{
+	struct clk_bulk_devres *devres;
+	int ret;
+
+	devres = devres_alloc(devm_clk_bulk_release,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	ret = clk_bulk_get(dev, num_clks, clks);
+	if (!ret) {
+		devres->clks = clks;
+		devres->num_clks = num_clks;
+		devres_add(dev, devres);
+	} else {
+		devres_free(devres);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_clk_bulk_get);
+
 static int devm_clk_match(struct device *dev, void *res, void *data)
 {
 	struct clk **c = res;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d05b66..3fc6010 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -278,11 +278,25 @@ struct clk *clk_get(struct device *dev, const char *id);
  *
  * clk_bulk_get should not be called from within interrupt context.
  */
-
 int __must_check clk_bulk_get(struct device *dev, int num_clks,
 			      struct clk_bulk_data *clks);
 
 /**
+ * devm_clk_bulk_get - managed get multiple clk consumers
+ * @dev: device for clock "consumer"
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * Return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation with management, the clks will
+ * automatically be freed when the device is unbound.
+ */
+int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
+				   struct clk_bulk_data *clks);
+
+/**
  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
  * @dev: device for clock "consumer"
  * @id: clock consumer ID
@@ -554,6 +568,12 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 	return NULL;
 }
 
+static inline int devm_clk_bulk_get(struct device *dev, int num_clks,
+				    struct clk_bulk_data *clks)
+{
+	return NULL;
+}
+
 static inline struct clk *devm_get_clk_from_child(struct device *dev,
 				struct device_node *np, const char *con_id)
 {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
@ 2017-04-12  4:03   ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-12  4:03 UTC (permalink / raw)
  To: linux-arm-kernel

This patch introduces the managed version of clk_bulk_get.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Anson Huang <anson.huang@nxp.com>
Cc: Robin Gong <yibin.gong@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: Octavian Purdila <octavian.purdila@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/clk/clk-devres.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 22 +++++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 3a218c3..c7fb31d 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -34,6 +34,42 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get);
 
+struct clk_bulk_devres {
+	struct clk_bulk_data *clks;
+	int num_clks;
+};
+
+static void devm_clk_bulk_release(struct device *dev, void *res)
+{
+	struct clk_bulk_devres *devres = res;
+
+	clk_bulk_put(devres->num_clks, devres->clks);
+}
+
+int devm_clk_bulk_get(struct device *dev, int num_clks,
+		      struct clk_bulk_data *clks)
+{
+	struct clk_bulk_devres *devres;
+	int ret;
+
+	devres = devres_alloc(devm_clk_bulk_release,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	ret = clk_bulk_get(dev, num_clks, clks);
+	if (!ret) {
+		devres->clks = clks;
+		devres->num_clks = num_clks;
+		devres_add(dev, devres);
+	} else {
+		devres_free(devres);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_clk_bulk_get);
+
 static int devm_clk_match(struct device *dev, void *res, void *data)
 {
 	struct clk **c = res;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d05b66..3fc6010 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -278,11 +278,25 @@ struct clk *clk_get(struct device *dev, const char *id);
  *
  * clk_bulk_get should not be called from within interrupt context.
  */
-
 int __must_check clk_bulk_get(struct device *dev, int num_clks,
 			      struct clk_bulk_data *clks);
 
 /**
+ * devm_clk_bulk_get - managed get multiple clk consumers
+ * @dev: device for clock "consumer"
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * Return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation with management, the clks will
+ * automatically be freed when the device is unbound.
+ */
+int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
+				   struct clk_bulk_data *clks);
+
+/**
  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
  * @dev: device for clock "consumer"
  * @id: clock consumer ID
@@ -554,6 +568,12 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 	return NULL;
 }
 
+static inline int devm_clk_bulk_get(struct device *dev, int num_clks,
+				    struct clk_bulk_data *clks)
+{
+	return NULL;
+}
+
 static inline struct clk *devm_get_clk_from_child(struct device *dev,
 				struct device_node *np, const char *con_id)
 {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [RFC PATCH 3/3] cpufreq: imx6q: refine clk operations
  2017-04-12  4:03 ` Dong Aisheng
@ 2017-04-12  4:03   ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-12  4:03 UTC (permalink / raw)
  To: linux-clk
  Cc: linux-kernel, linux-arm-kernel, aisheng.dong, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, sboyd, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

Use clk_bulk_get to ease the driver clocks handling.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Anson Huang <anson.huang@nxp.com>
Cc: Robin Gong <yibin.gong@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: Octavian Purdila <octavian.purdila@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/cpufreq/imx6q-cpufreq.c | 119 ++++++++++++++++++----------------------
 1 file changed, 53 insertions(+), 66 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 7719b02..6158910 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -24,15 +24,29 @@ static struct regulator *arm_reg;
 static struct regulator *pu_reg;
 static struct regulator *soc_reg;
 
-static struct clk *arm_clk;
-static struct clk *pll1_sys_clk;
-static struct clk *pll1_sw_clk;
-static struct clk *step_clk;
-static struct clk *pll2_pfd2_396m_clk;
-
-/* clk used by i.MX6UL */
-static struct clk *pll2_bus_clk;
-static struct clk *secondary_sel_clk;
+enum IMX6_CPUFREQ_CLKS {
+	ARM,
+	PLL1_SYS,
+	STEP,
+	PLL1_SW,
+	PLL2_PFD2_396M,
+	/* MX6UL requires two more clks */
+	PLL2_BUS,
+	SECONDARY_SEL,
+};
+#define IMX6Q_CPUFREQ_CLK_NUM		5
+#define IMX6UL_CPUFREQ_CLK_NUM		7
+
+static int num_clks;
+static struct clk_bulk_data clks[] = {
+	{ .id = "arm" },
+	{ .id = "pll1_sys" },
+	{ .id = "step" },
+	{ .id = "pll1_sw" },
+	{ .id = "pll2_pfd2_396m" },
+	{ .id = "pll2_bus" },
+	{ .id = "secondary_sel" },
+};
 
 static struct device *cpu_dev;
 static bool free_opp;
@@ -51,7 +65,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 	new_freq = freq_table[index].frequency;
 	freq_hz = new_freq * 1000;
-	old_freq = clk_get_rate(arm_clk) / 1000;
+	old_freq = clk_get_rate(clks[ARM].clk) / 1000;
 
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
 	if (IS_ERR(opp)) {
@@ -109,25 +123,27 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 		 * voltage of 528MHz, so lower the CPU frequency to one
 		 * half before changing CPU frequency.
 		 */
-		clk_set_rate(arm_clk, (old_freq >> 1) * 1000);
-		clk_set_parent(pll1_sw_clk, pll1_sys_clk);
-		if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk))
-			clk_set_parent(secondary_sel_clk, pll2_bus_clk);
+		clk_set_rate(clks[ARM].clk, (old_freq >> 1) * 1000);
+		clk_set_parent(clks[PLL1_SW].clk, clks[PLL1_SYS].clk);
+		if (freq_hz > clk_get_rate(clks[PLL2_PFD2_396M].clk))
+			clk_set_parent(clks[SECONDARY_SEL].clk,
+				       clks[PLL2_BUS].clk);
 		else
-			clk_set_parent(secondary_sel_clk, pll2_pfd2_396m_clk);
-		clk_set_parent(step_clk, secondary_sel_clk);
-		clk_set_parent(pll1_sw_clk, step_clk);
+			clk_set_parent(clks[SECONDARY_SEL].clk,
+				       clks[PLL2_PFD2_396M].clk);
+		clk_set_parent(clks[STEP].clk, clks[SECONDARY_SEL].clk);
+		clk_set_parent(clks[PLL1_SW].clk, clks[STEP].clk);
 	} else {
-		clk_set_parent(step_clk, pll2_pfd2_396m_clk);
-		clk_set_parent(pll1_sw_clk, step_clk);
-		if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) {
-			clk_set_rate(pll1_sys_clk, new_freq * 1000);
-			clk_set_parent(pll1_sw_clk, pll1_sys_clk);
+		clk_set_parent(clks[STEP].clk, clks[PLL2_PFD2_396M].clk);
+		clk_set_parent(clks[PLL1_SW].clk, clks[STEP].clk);
+		if (freq_hz > clk_get_rate(clks[PLL2_PFD2_396M].clk)) {
+			clk_set_rate(clks[PLL1_SYS].clk, new_freq * 1000);
+			clk_set_parent(clks[PLL1_SW].clk, clks[PLL1_SYS].clk);
 		}
 	}
 
 	/* Ensure the arm clock divider is what we expect */
-	ret = clk_set_rate(arm_clk, new_freq * 1000);
+	ret = clk_set_rate(clks[ARM].clk, new_freq * 1000);
 	if (ret) {
 		dev_err(cpu_dev, "failed to set clock rate: %d\n", ret);
 		regulator_set_voltage_tol(arm_reg, volt_old, 0);
@@ -161,7 +177,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
-	policy->clk = arm_clk;
+	policy->clk = clks[ARM].clk;
 	return cpufreq_generic_init(policy, freq_table, transition_latency);
 }
 
@@ -197,27 +213,14 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	arm_clk = clk_get(cpu_dev, "arm");
-	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
-	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
-	step_clk = clk_get(cpu_dev, "step");
-	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
-	if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
-	    IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
-		dev_err(cpu_dev, "failed to get clocks\n");
-		ret = -ENOENT;
-		goto put_clk;
-	}
+	if (of_machine_is_compatible("fsl,imx6ul"))
+		num_clks = IMX6UL_CPUFREQ_CLK_NUM;
+	else
+		num_clks = IMX6Q_CPUFREQ_CLK_NUM;
 
-	if (of_machine_is_compatible("fsl,imx6ul")) {
-		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
-		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
-		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
-			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
-			ret = -ENOENT;
-			goto put_clk;
-		}
-	}
+	ret = clk_bulk_get(cpu_dev, num_clks, clks);
+	if (ret)
+		goto put_node;
 
 	arm_reg = regulator_get(cpu_dev, "arm");
 	pu_reg = regulator_get_optional(cpu_dev, "pu");
@@ -354,22 +357,11 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		regulator_put(pu_reg);
 	if (!IS_ERR(soc_reg))
 		regulator_put(soc_reg);
-put_clk:
-	if (!IS_ERR(arm_clk))
-		clk_put(arm_clk);
-	if (!IS_ERR(pll1_sys_clk))
-		clk_put(pll1_sys_clk);
-	if (!IS_ERR(pll1_sw_clk))
-		clk_put(pll1_sw_clk);
-	if (!IS_ERR(step_clk))
-		clk_put(step_clk);
-	if (!IS_ERR(pll2_pfd2_396m_clk))
-		clk_put(pll2_pfd2_396m_clk);
-	if (!IS_ERR(pll2_bus_clk))
-		clk_put(pll2_bus_clk);
-	if (!IS_ERR(secondary_sel_clk))
-		clk_put(secondary_sel_clk);
+
+	clk_bulk_put(num_clks, clks);
+put_node:
 	of_node_put(np);
+
 	return ret;
 }
 
@@ -383,13 +375,8 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
 	if (!IS_ERR(pu_reg))
 		regulator_put(pu_reg);
 	regulator_put(soc_reg);
-	clk_put(arm_clk);
-	clk_put(pll1_sys_clk);
-	clk_put(pll1_sw_clk);
-	clk_put(step_clk);
-	clk_put(pll2_pfd2_396m_clk);
-	clk_put(pll2_bus_clk);
-	clk_put(secondary_sel_clk);
+
+	clk_bulk_put(num_clks, clks);
 
 	return 0;
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [RFC PATCH 3/3] cpufreq: imx6q: refine clk operations
@ 2017-04-12  4:03   ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-12  4:03 UTC (permalink / raw)
  To: linux-arm-kernel

Use clk_bulk_get to ease the driver clocks handling.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Anson Huang <anson.huang@nxp.com>
Cc: Robin Gong <yibin.gong@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: Octavian Purdila <octavian.purdila@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/cpufreq/imx6q-cpufreq.c | 119 ++++++++++++++++++----------------------
 1 file changed, 53 insertions(+), 66 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 7719b02..6158910 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -24,15 +24,29 @@ static struct regulator *arm_reg;
 static struct regulator *pu_reg;
 static struct regulator *soc_reg;
 
-static struct clk *arm_clk;
-static struct clk *pll1_sys_clk;
-static struct clk *pll1_sw_clk;
-static struct clk *step_clk;
-static struct clk *pll2_pfd2_396m_clk;
-
-/* clk used by i.MX6UL */
-static struct clk *pll2_bus_clk;
-static struct clk *secondary_sel_clk;
+enum IMX6_CPUFREQ_CLKS {
+	ARM,
+	PLL1_SYS,
+	STEP,
+	PLL1_SW,
+	PLL2_PFD2_396M,
+	/* MX6UL requires two more clks */
+	PLL2_BUS,
+	SECONDARY_SEL,
+};
+#define IMX6Q_CPUFREQ_CLK_NUM		5
+#define IMX6UL_CPUFREQ_CLK_NUM		7
+
+static int num_clks;
+static struct clk_bulk_data clks[] = {
+	{ .id = "arm" },
+	{ .id = "pll1_sys" },
+	{ .id = "step" },
+	{ .id = "pll1_sw" },
+	{ .id = "pll2_pfd2_396m" },
+	{ .id = "pll2_bus" },
+	{ .id = "secondary_sel" },
+};
 
 static struct device *cpu_dev;
 static bool free_opp;
@@ -51,7 +65,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 	new_freq = freq_table[index].frequency;
 	freq_hz = new_freq * 1000;
-	old_freq = clk_get_rate(arm_clk) / 1000;
+	old_freq = clk_get_rate(clks[ARM].clk) / 1000;
 
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
 	if (IS_ERR(opp)) {
@@ -109,25 +123,27 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 		 * voltage of 528MHz, so lower the CPU frequency to one
 		 * half before changing CPU frequency.
 		 */
-		clk_set_rate(arm_clk, (old_freq >> 1) * 1000);
-		clk_set_parent(pll1_sw_clk, pll1_sys_clk);
-		if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk))
-			clk_set_parent(secondary_sel_clk, pll2_bus_clk);
+		clk_set_rate(clks[ARM].clk, (old_freq >> 1) * 1000);
+		clk_set_parent(clks[PLL1_SW].clk, clks[PLL1_SYS].clk);
+		if (freq_hz > clk_get_rate(clks[PLL2_PFD2_396M].clk))
+			clk_set_parent(clks[SECONDARY_SEL].clk,
+				       clks[PLL2_BUS].clk);
 		else
-			clk_set_parent(secondary_sel_clk, pll2_pfd2_396m_clk);
-		clk_set_parent(step_clk, secondary_sel_clk);
-		clk_set_parent(pll1_sw_clk, step_clk);
+			clk_set_parent(clks[SECONDARY_SEL].clk,
+				       clks[PLL2_PFD2_396M].clk);
+		clk_set_parent(clks[STEP].clk, clks[SECONDARY_SEL].clk);
+		clk_set_parent(clks[PLL1_SW].clk, clks[STEP].clk);
 	} else {
-		clk_set_parent(step_clk, pll2_pfd2_396m_clk);
-		clk_set_parent(pll1_sw_clk, step_clk);
-		if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) {
-			clk_set_rate(pll1_sys_clk, new_freq * 1000);
-			clk_set_parent(pll1_sw_clk, pll1_sys_clk);
+		clk_set_parent(clks[STEP].clk, clks[PLL2_PFD2_396M].clk);
+		clk_set_parent(clks[PLL1_SW].clk, clks[STEP].clk);
+		if (freq_hz > clk_get_rate(clks[PLL2_PFD2_396M].clk)) {
+			clk_set_rate(clks[PLL1_SYS].clk, new_freq * 1000);
+			clk_set_parent(clks[PLL1_SW].clk, clks[PLL1_SYS].clk);
 		}
 	}
 
 	/* Ensure the arm clock divider is what we expect */
-	ret = clk_set_rate(arm_clk, new_freq * 1000);
+	ret = clk_set_rate(clks[ARM].clk, new_freq * 1000);
 	if (ret) {
 		dev_err(cpu_dev, "failed to set clock rate: %d\n", ret);
 		regulator_set_voltage_tol(arm_reg, volt_old, 0);
@@ -161,7 +177,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
-	policy->clk = arm_clk;
+	policy->clk = clks[ARM].clk;
 	return cpufreq_generic_init(policy, freq_table, transition_latency);
 }
 
@@ -197,27 +213,14 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	arm_clk = clk_get(cpu_dev, "arm");
-	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
-	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
-	step_clk = clk_get(cpu_dev, "step");
-	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
-	if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
-	    IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
-		dev_err(cpu_dev, "failed to get clocks\n");
-		ret = -ENOENT;
-		goto put_clk;
-	}
+	if (of_machine_is_compatible("fsl,imx6ul"))
+		num_clks = IMX6UL_CPUFREQ_CLK_NUM;
+	else
+		num_clks = IMX6Q_CPUFREQ_CLK_NUM;
 
-	if (of_machine_is_compatible("fsl,imx6ul")) {
-		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
-		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
-		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
-			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
-			ret = -ENOENT;
-			goto put_clk;
-		}
-	}
+	ret = clk_bulk_get(cpu_dev, num_clks, clks);
+	if (ret)
+		goto put_node;
 
 	arm_reg = regulator_get(cpu_dev, "arm");
 	pu_reg = regulator_get_optional(cpu_dev, "pu");
@@ -354,22 +357,11 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		regulator_put(pu_reg);
 	if (!IS_ERR(soc_reg))
 		regulator_put(soc_reg);
-put_clk:
-	if (!IS_ERR(arm_clk))
-		clk_put(arm_clk);
-	if (!IS_ERR(pll1_sys_clk))
-		clk_put(pll1_sys_clk);
-	if (!IS_ERR(pll1_sw_clk))
-		clk_put(pll1_sw_clk);
-	if (!IS_ERR(step_clk))
-		clk_put(step_clk);
-	if (!IS_ERR(pll2_pfd2_396m_clk))
-		clk_put(pll2_pfd2_396m_clk);
-	if (!IS_ERR(pll2_bus_clk))
-		clk_put(pll2_bus_clk);
-	if (!IS_ERR(secondary_sel_clk))
-		clk_put(secondary_sel_clk);
+
+	clk_bulk_put(num_clks, clks);
+put_node:
 	of_node_put(np);
+
 	return ret;
 }
 
@@ -383,13 +375,8 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
 	if (!IS_ERR(pu_reg))
 		regulator_put(pu_reg);
 	regulator_put(soc_reg);
-	clk_put(arm_clk);
-	clk_put(pll1_sys_clk);
-	clk_put(pll1_sw_clk);
-	clk_put(step_clk);
-	clk_put(pll2_pfd2_396m_clk);
-	clk_put(pll2_bus_clk);
-	clk_put(secondary_sel_clk);
+
+	clk_bulk_put(num_clks, clks);
 
 	return 0;
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
  2017-04-11 17:01   ` Florian Fainelli
@ 2017-04-13 13:58     ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 13:58 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Dong Aisheng, linux-clk, leonard.crestez, ping.bai, anson.huang,
	viresh.kumar, mturquette, rjw, linux-kernel, broonie,
	octavian.purdila, kernel, fabio.estevam, yibin.gong, shawnguo,
	sboyd, linux-arm-kernel

On Tue, Apr 11, 2017 at 10:01:29AM -0700, Florian Fainelli wrote:
...
> > This is a RFC patch intending to bring up the idea to discuss.
> > 
> > Comments are welcome and appreciated!
> 
> Thanks a lot for doing this, we have historically done something similar
> on ARCH_BRCMSTB platforms in our downstream tree with a concept of a
> "software" clock which has multiple parents (yikes), using the bulk
> accessors would essentially allow us to remove part of this hack, so I
> am all in favor of this!

Glad to see someone else has the same idea! :-)

Thanks for the supporting!

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
@ 2017-04-13 13:58     ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 13:58 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Apr 11, 2017 at 10:01:29AM -0700, Florian Fainelli wrote:
...
> > This is a RFC patch intending to bring up the idea to discuss.
> > 
> > Comments are welcome and appreciated!
> 
> Thanks a lot for doing this, we have historically done something similar
> on ARCH_BRCMSTB platforms in our downstream tree with a concept of a
> "software" clock which has multiple parents (yikes), using the bulk
> accessors would essentially allow us to remove part of this hack, so I
> am all in favor of this!

Glad to see someone else has the same idea! :-)

Thanks for the supporting!

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-11 17:19     ` Leonard Crestez
@ 2017-04-13 14:02       ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 14:02 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, linux-kernel, linux-arm-kernel, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, sboyd, shawnguo,
	fabio.estevam, anson.huang, ping.bai, octavian.purdila,
	linux-clk

On Tue, Apr 11, 2017 at 08:19:19PM +0300, Leonard Crestez wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> > +/**
> > + * clk_bulk_enable - ungate a bulk of clocks
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_enable must not sleep
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> > +{
> > +       int ret;
> > +       int i;
> > +
> > +       for (i = 0; i < num_clks; i++) {
> > +               ret = clk_enable(clks[i].clk);
> > +               if (ret) {
> > +                       pr_err("Failed to enable clk '%s': %d\n",
> > +                               clks[i].id, ret);
> > +                       goto err;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +
> > +err:
> > +       while (--i >= 0)
> > +               clk_put(clks[i].clk);
> 
> Shouldn't this be clk_disable?
> 

Good catch!
Will change in the formal version if Maintainer accepts this idea.

> And you can probably use clk_bulk_disable(i, clks) instead

Probably i'd prefer keep clk_disable to match with clk_enable
to make things more clear.

Thanks

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-13 14:02       ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 14:02 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Apr 11, 2017 at 08:19:19PM +0300, Leonard Crestez wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> > +/**
> > + * clk_bulk_enable - ungate a bulk of clocks
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_enable must not sleep
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> > +{
> > +???????int ret;
> > +???????int i;
> > +
> > +???????for (i = 0; i < num_clks; i++) {
> > +???????????????ret = clk_enable(clks[i].clk);
> > +???????????????if (ret) {
> > +???????????????????????pr_err("Failed to enable clk '%s': %d\n",
> > +???????????????????????????????clks[i].id, ret);
> > +???????????????????????goto err;
> > +???????????????}
> > +???????}
> > +
> > +???????return 0;
> > +
> > +err:
> > +???????while (--i >= 0)
> > +???????????????clk_put(clks[i].clk);
> 
> Shouldn't this be clk_disable?
> 

Good catch!
Will change in the formal version if Maintainer accepts this idea.

> And you can probably use clk_bulk_disable(i, clks) instead

Probably i'd prefer keep clk_disable to match with clk_enable
to make things more clear.

Thanks

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 3/3] cpufreq: imx6q: refine clk operations
  2017-04-11 17:48     ` Leonard Crestez
@ 2017-04-13 14:21       ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 14:21 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, linux-clk, linux-kernel, linux-arm-kernel, kernel,
	broonie, yibin.gong, rjw, viresh.kumar, mturquette, sboyd,
	shawnguo, fabio.estevam, anson.huang, ping.bai, octavian.purdila

On Tue, Apr 11, 2017 at 08:48:28PM +0300, Leonard Crestez wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> > +static int num_clks;
> > +static struct clk_bulk_data clks[] = {
> > +	{ .id = "arm" },
> > +	{ .id = "pll1_sys" },
> > +	{ .id = "step" },
> > +	{ .id = "pll1_sw" },
> > +	{ .id = "pll2_pfd2_396m" },
> > +	{ .id = "pll2_bus" },
> > +	{ .id = "secondary_sel" },
> > +};
> 
> The .id is only required for initialization, it seems strange to keep
> it around runtime data.

Well, this is mainly referencing how regulator bulk does the job.

> It might be better for this API to work with an
> array of clk* and separate array of names (or clk_bulk_init_data if we
> need flags). Variable references would be shorter and it would allow
> more data to be const.

It also has side effect that we then need one more param for each API.
Is that worth?

> > -put_clk:
> > -	if (!IS_ERR(arm_clk))
> > -		clk_put(arm_clk);
> > -	if (!IS_ERR(pll1_sys_clk))
> > -		clk_put(pll1_sys_clk);
> > -	if (!IS_ERR(pll1_sw_clk))
> > -		clk_put(pll1_sw_clk);
> > -	if (!IS_ERR(step_clk))
> > -		clk_put(step_clk);
> > -	if (!IS_ERR(pll2_pfd2_396m_clk))
> > -		clk_put(pll2_pfd2_396m_clk);
> > -	if (!IS_ERR(pll2_bus_clk))
> > -		clk_put(pll2_bus_clk);
> > -	if (!IS_ERR(secondary_sel_clk))
> > -		clk_put(secondary_sel_clk);
> > +
> > +	clk_bulk_put(num_clks, clks);
> > +put_node:
> >  	of_node_put(np);
> > +
> >  	return ret;
> >  }
> 
> 
> My subjective opinion is that a better way to clean this up would be to
> have a single imx6q_cpufreq_clean function that takes all resources and
> does stuff like:
> 
> if (!IS_ERR(clk)) clk_put(clk);
> clk = NULL;
> 
> That function can be called from both _remove and failed _probe without
> having to keep track of which resources have been allocated until then.
> Just free and NULL all clocks/regulators and simplify control flow.
> 

I once thought of that way.

Now i'd like to remove them rather than form them into a function
which can't permanently fix the issue.

But, if Maintainers dislike it, we could do that.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 3/3] cpufreq: imx6q: refine clk operations
@ 2017-04-13 14:21       ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 14:21 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Apr 11, 2017 at 08:48:28PM +0300, Leonard Crestez wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> > +static int num_clks;
> > +static struct clk_bulk_data clks[] = {
> > +	{ .id = "arm" },
> > +	{ .id = "pll1_sys" },
> > +	{ .id = "step" },
> > +	{ .id = "pll1_sw" },
> > +	{ .id = "pll2_pfd2_396m" },
> > +	{ .id = "pll2_bus" },
> > +	{ .id = "secondary_sel" },
> > +};
> 
> The .id is only required for initialization, it seems strange to keep
> it around runtime data.

Well, this is mainly referencing how regulator bulk does the job.

> It might be better for this API to work with an
> array of clk* and separate array of names (or clk_bulk_init_data if we
> need flags). Variable references would be shorter and it would allow
> more data to be const.

It also has side effect that we then need one more param for each API.
Is that worth?

> > -put_clk:
> > -	if (!IS_ERR(arm_clk))
> > -		clk_put(arm_clk);
> > -	if (!IS_ERR(pll1_sys_clk))
> > -		clk_put(pll1_sys_clk);
> > -	if (!IS_ERR(pll1_sw_clk))
> > -		clk_put(pll1_sw_clk);
> > -	if (!IS_ERR(step_clk))
> > -		clk_put(step_clk);
> > -	if (!IS_ERR(pll2_pfd2_396m_clk))
> > -		clk_put(pll2_pfd2_396m_clk);
> > -	if (!IS_ERR(pll2_bus_clk))
> > -		clk_put(pll2_bus_clk);
> > -	if (!IS_ERR(secondary_sel_clk))
> > -		clk_put(secondary_sel_clk);
> > +
> > +	clk_bulk_put(num_clks, clks);
> > +put_node:
> > ?	of_node_put(np);
> > +
> > ?	return ret;
> > ?}
> 
> 
> My subjective opinion is that a better way to clean this up would be to
> have a single imx6q_cpufreq_clean function that takes all resources and
> does stuff like:
> 
> if (!IS_ERR(clk)) clk_put(clk);
> clk = NULL;
> 
> That function can be called from both _remove and failed _probe without
> having to keep track of which resources have been allocated until then.
> Just free and NULL all clocks/regulators and simplify control flow.
> 

I once thought of that way.

Now i'd like to remove them rather than form them into a function
which can't permanently fix the issue.

But, if Maintainers dislike it, we could do that.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-12  4:03   ` Dong Aisheng
@ 2017-04-13 14:25     ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 14:25 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: linux-clk, linux-kernel, linux-arm-kernel, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, sboyd, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On Wed, Apr 12, 2017 at 12:03:27PM +0800, Dong Aisheng wrote:
...
> @@ -445,6 +543,12 @@ static inline struct clk *clk_get(struct device *dev, const char *id)
>  	return NULL;
>  }
>  
> +static inline int clk_bulk_get(struct device *dev, int num_clks,
> +			       struct clk_bulk_data *clks)
> +{
> +	return NULL;

Here needs to be changed to 'return 0'.

It's catched by 0day robot.

include/linux/clk.h: In function 'clk_bulk_get':
include/linux/stddef.h:7:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
#define NULL ((void *)0)

Good job! Robot!

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-13 14:25     ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 14:25 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Apr 12, 2017 at 12:03:27PM +0800, Dong Aisheng wrote:
...
> @@ -445,6 +543,12 @@ static inline struct clk *clk_get(struct device *dev, const char *id)
>  	return NULL;
>  }
>  
> +static inline int clk_bulk_get(struct device *dev, int num_clks,
> +			       struct clk_bulk_data *clks)
> +{
> +	return NULL;

Here needs to be changed to 'return 0'.

It's catched by 0day robot.

include/linux/clk.h: In function 'clk_bulk_get':
include/linux/stddef.h:7:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
#define NULL ((void *)0)

Good job! Robot!

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
  2017-04-12  4:03   ` Dong Aisheng
@ 2017-04-13 14:37     ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 14:37 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: linux-clk, linux-kernel, linux-arm-kernel, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, sboyd, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On Wed, Apr 12, 2017 at 12:03:28PM +0800, Dong Aisheng wrote:
> This patch introduces the managed version of clk_bulk_get.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Anson Huang <anson.huang@nxp.com>
> Cc: Robin Gong <yibin.gong@nxp.com>
> Cc: Bai Ping <ping.bai@nxp.com>
> Cc: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: Octavian Purdila <octavian.purdila@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/clk/clk-devres.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h      | 22 +++++++++++++++++++++-
>  2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 3a218c3..c7fb31d 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -34,6 +34,42 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
>  }
>  EXPORT_SYMBOL(devm_clk_get);
>  
> +struct clk_bulk_devres {
> +	struct clk_bulk_data *clks;
> +	int num_clks;
> +};
> +
> +static void devm_clk_bulk_release(struct device *dev, void *res)
> +{
> +	struct clk_bulk_devres *devres = res;
> +
> +	clk_bulk_put(devres->num_clks, devres->clks);
> +}
> +
> +int devm_clk_bulk_get(struct device *dev, int num_clks,
> +		      struct clk_bulk_data *clks)
> +{
> +	struct clk_bulk_devres *devres;
> +	int ret;
> +
> +	devres = devres_alloc(devm_clk_bulk_release,
> +			      sizeof(*devres), GFP_KERNEL);
> +	if (!devres)
> +		return -ENOMEM;
> +
> +	ret = clk_bulk_get(dev, num_clks, clks);


Another catch by 0day robot.

   drivers/built-in.o: In function `devm_clk_bulk_get':
>> (.text+0x1930e): undefined reference to `clk_bulk_get'
   drivers/built-in.o: In function `devm_clk_bulk_release':
>> clk-devres.c:(.text+0x19370): undefined reference to `clk_bulk_put'

clk_bulk_get is defined in clkdev.c which depends on CONFIG_CLKDEV_LOOKUP.
However, some platforms like m68k may not select CLKDEV_LOOKUP but
select HAVE_CLK. Thus compiling devm_clk_bulk_get may cause a undefined
reference to 'clk_bulk_get'.

Since clk_bulk_get is built upon the platform specific clk_get api,
clk_bulk_get can also be used by that platform accordingly.

Then we probably could move clk_bulk_get into clk-devres.c as well which
is controlled by common CONFIG_HAVE_CLK to benifit all platforms.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
@ 2017-04-13 14:37     ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-13 14:37 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Apr 12, 2017 at 12:03:28PM +0800, Dong Aisheng wrote:
> This patch introduces the managed version of clk_bulk_get.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Anson Huang <anson.huang@nxp.com>
> Cc: Robin Gong <yibin.gong@nxp.com>
> Cc: Bai Ping <ping.bai@nxp.com>
> Cc: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: Octavian Purdila <octavian.purdila@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/clk/clk-devres.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h      | 22 +++++++++++++++++++++-
>  2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 3a218c3..c7fb31d 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -34,6 +34,42 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
>  }
>  EXPORT_SYMBOL(devm_clk_get);
>  
> +struct clk_bulk_devres {
> +	struct clk_bulk_data *clks;
> +	int num_clks;
> +};
> +
> +static void devm_clk_bulk_release(struct device *dev, void *res)
> +{
> +	struct clk_bulk_devres *devres = res;
> +
> +	clk_bulk_put(devres->num_clks, devres->clks);
> +}
> +
> +int devm_clk_bulk_get(struct device *dev, int num_clks,
> +		      struct clk_bulk_data *clks)
> +{
> +	struct clk_bulk_devres *devres;
> +	int ret;
> +
> +	devres = devres_alloc(devm_clk_bulk_release,
> +			      sizeof(*devres), GFP_KERNEL);
> +	if (!devres)
> +		return -ENOMEM;
> +
> +	ret = clk_bulk_get(dev, num_clks, clks);


Another catch by 0day robot.

   drivers/built-in.o: In function `devm_clk_bulk_get':
>> (.text+0x1930e): undefined reference to `clk_bulk_get'
   drivers/built-in.o: In function `devm_clk_bulk_release':
>> clk-devres.c:(.text+0x19370): undefined reference to `clk_bulk_put'

clk_bulk_get is defined in clkdev.c which depends on CONFIG_CLKDEV_LOOKUP.
However, some platforms like m68k may not select CLKDEV_LOOKUP but
select HAVE_CLK. Thus compiling devm_clk_bulk_get may cause a undefined
reference to 'clk_bulk_get'.

Since clk_bulk_get is built upon the platform specific clk_get api,
clk_bulk_get can also be used by that platform accordingly.

Then we probably could move clk_bulk_get into clk-devres.c as well which
is controlled by common CONFIG_HAVE_CLK to benifit all platforms.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-12  4:03   ` Dong Aisheng
  (?)
@ 2017-04-13 19:56     ` Geert Uytterhoeven
  -1 siblings, 0 replies; 49+ messages in thread
From: Geert Uytterhoeven @ 2017-04-13 19:56 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: linux-clk, linux-kernel, linux-arm-kernel, Sascha Hauer,
	Mark Brown, yibin.gong, Rafael J. Wysocki, Viresh Kumar,
	Michael Turquette, Stephen Boyd, Shawn Guo, Fabio Estevam,
	anson.huang, ping.bai, leonard.crestez, octavian.purdila

On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_unprepare);
>
> +/**
> + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)

unsigned int num_clks (everywhere)

> +{
> +       int i;

unsigned int i (everywhere)

> +
> +       for (i = 0; i < num_clks; i++)
> +               clk_unprepare(clks[i].clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);

This does mean you have to change your "while (--i >= 0)" loops.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-13 19:56     ` Geert Uytterhoeven
  0 siblings, 0 replies; 49+ messages in thread
From: Geert Uytterhoeven @ 2017-04-13 19:56 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: linux-clk, linux-kernel, linux-arm-kernel, Sascha Hauer,
	Mark Brown, yibin.gong, Rafael J. Wysocki, Viresh Kumar,
	Michael Turquette, Stephen Boyd, Shawn Guo, Fabio Estevam,
	anson.huang, ping.bai, leonard.crestez, octavian.purdila

On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_unprepare);
>
> +/**
> + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)

unsigned int num_clks (everywhere)

> +{
> +       int i;

unsigned int i (everywhere)

> +
> +       for (i = 0; i < num_clks; i++)
> +               clk_unprepare(clks[i].clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);

This does mean you have to change your "while (--i >= 0)" loops.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-13 19:56     ` Geert Uytterhoeven
  0 siblings, 0 replies; 49+ messages in thread
From: Geert Uytterhoeven @ 2017-04-13 19:56 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_unprepare);
>
> +/**
> + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)

unsigned int num_clks (everywhere)

> +{
> +       int i;

unsigned int i (everywhere)

> +
> +       for (i = 0; i < num_clks; i++)
> +               clk_unprepare(clks[i].clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);

This does mean you have to change your "while (--i >= 0)" loops.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-11 17:19     ` Leonard Crestez
  (?)
@ 2017-04-13 19:57       ` Geert Uytterhoeven
  -1 siblings, 0 replies; 49+ messages in thread
From: Geert Uytterhoeven @ 2017-04-13 19:57 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, linux-kernel, linux-arm-kernel, Sascha Hauer,
	Mark Brown, yibin.gong, Rafael J. Wysocki, Viresh Kumar,
	Michael Turquette, Stephen Boyd, Shawn Guo, Fabio Estevam,
	anson.huang, ping.bai, octavian.purdila, linux-clk

On Tue, Apr 11, 2017 at 7:19 PM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
>> +/**
>> + * clk_bulk_enable - ungate a bulk of clocks
>> + * @num_clks: the number of clk_bulk_data
>> + * @clks: the clk_bulk_data table being ungated
>> + *
>> + * clk_bulk_enable must not sleep
>> + * Returns 0 on success, -EERROR otherwise.
>> + */
>> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
>> +{
>> +       int ret;
>> +       int i;
>> +
>> +       for (i = 0; i < num_clks; i++) {
>> +               ret = clk_enable(clks[i].clk);
>> +               if (ret) {
>> +                       pr_err("Failed to enable clk '%s': %d\n",
>> +                               clks[i].id, ret);
>> +                       goto err;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +
>> +err:
>> +       while (--i >= 0)
>> +               clk_put(clks[i].clk);
>
> Shouldn't this be clk_disable?
>
> And you can probably use clk_bulk_disable(i, clks) instead

To avoid nasty surprises, clk_bulk_disable() should loop over all clocks
in reverse order.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-13 19:57       ` Geert Uytterhoeven
  0 siblings, 0 replies; 49+ messages in thread
From: Geert Uytterhoeven @ 2017-04-13 19:57 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Dong Aisheng, linux-kernel, linux-arm-kernel, Sascha Hauer,
	Mark Brown, yibin.gong, Rafael J. Wysocki, Viresh Kumar,
	Michael Turquette, Stephen Boyd, Shawn Guo, Fabio Estevam,
	anson.huang, ping.bai, octavian.purdila, linux-clk

On Tue, Apr 11, 2017 at 7:19 PM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
>> +/**
>> + * clk_bulk_enable - ungate a bulk of clocks
>> + * @num_clks: the number of clk_bulk_data
>> + * @clks: the clk_bulk_data table being ungated
>> + *
>> + * clk_bulk_enable must not sleep
>> + * Returns 0 on success, -EERROR otherwise.
>> + */
>> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
>> +{
>> +       int ret;
>> +       int i;
>> +
>> +       for (i = 0; i < num_clks; i++) {
>> +               ret = clk_enable(clks[i].clk);
>> +               if (ret) {
>> +                       pr_err("Failed to enable clk '%s': %d\n",
>> +                               clks[i].id, ret);
>> +                       goto err;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +
>> +err:
>> +       while (--i >= 0)
>> +               clk_put(clks[i].clk);
>
> Shouldn't this be clk_disable?
>
> And you can probably use clk_bulk_disable(i, clks) instead

To avoid nasty surprises, clk_bulk_disable() should loop over all clocks
in reverse order.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-13 19:57       ` Geert Uytterhoeven
  0 siblings, 0 replies; 49+ messages in thread
From: Geert Uytterhoeven @ 2017-04-13 19:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Apr 11, 2017 at 7:19 PM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
>> +/**
>> + * clk_bulk_enable - ungate a bulk of clocks
>> + * @num_clks: the number of clk_bulk_data
>> + * @clks: the clk_bulk_data table being ungated
>> + *
>> + * clk_bulk_enable must not sleep
>> + * Returns 0 on success, -EERROR otherwise.
>> + */
>> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
>> +{
>> +       int ret;
>> +       int i;
>> +
>> +       for (i = 0; i < num_clks; i++) {
>> +               ret = clk_enable(clks[i].clk);
>> +               if (ret) {
>> +                       pr_err("Failed to enable clk '%s': %d\n",
>> +                               clks[i].id, ret);
>> +                       goto err;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +
>> +err:
>> +       while (--i >= 0)
>> +               clk_put(clks[i].clk);
>
> Shouldn't this be clk_disable?
>
> And you can probably use clk_bulk_disable(i, clks) instead

To avoid nasty surprises, clk_bulk_disable() should loop over all clocks
in reverse order.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-13 19:56     ` Geert Uytterhoeven
  (?)
@ 2017-04-14 16:14       ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-14 16:14 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Dong Aisheng, linux-clk, linux-kernel, linux-arm-kernel,
	Sascha Hauer, Mark Brown, yibin.gong, Rafael J. Wysocki,
	Viresh Kumar, Michael Turquette, Stephen Boyd, Shawn Guo,
	Fabio Estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

Hi Geert,

On Thu, Apr 13, 2017 at 09:56:31PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_unprepare);
> >
> > +/**
> > + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
> 
> unsigned int num_clks (everywhere)
> 
> > +{
> > +       int i;
> 
> unsigned int i (everywhere)

Any special purpose?

Looks like 'int i' for a loop is widely used in kernel.

Would you please help clarify more?

> > +
> > +       for (i = 0; i < num_clks; i++)
> > +               clk_unprepare(clks[i].clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
> 
> This does mean you have to change your "while (--i >= 0)" loops.

Is that really necessary as i thought the clk_bulk_get/put does not
guarantee any clk operation orders within the bulk?
Should we need add that support?

And currently this does the same thing as bulk regulator.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-14 16:14       ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-14 16:14 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Dong Aisheng, linux-clk, linux-kernel, linux-arm-kernel,
	Sascha Hauer, Mark Brown, yibin.gong, Rafael J. Wysocki,
	Viresh Kumar, Michael Turquette, Stephen Boyd, Shawn Guo,
	Fabio Estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

Hi Geert,

On Thu, Apr 13, 2017 at 09:56:31PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_unprepare);
> >
> > +/**
> > + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
> 
> unsigned int num_clks (everywhere)
> 
> > +{
> > +       int i;
> 
> unsigned int i (everywhere)

Any special purpose?

Looks like 'int i' for a loop is widely used in kernel.

Would you please help clarify more?

> > +
> > +       for (i = 0; i < num_clks; i++)
> > +               clk_unprepare(clks[i].clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
> 
> This does mean you have to change your "while (--i >= 0)" loops.

Is that really necessary as i thought the clk_bulk_get/put does not
guarantee any clk operation orders within the bulk?
Should we need add that support?

And currently this does the same thing as bulk regulator.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-14 16:14       ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-04-14 16:14 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Geert,

On Thu, Apr 13, 2017 at 09:56:31PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_unprepare);
> >
> > +/**
> > + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
> 
> unsigned int num_clks (everywhere)
> 
> > +{
> > +       int i;
> 
> unsigned int i (everywhere)

Any special purpose?

Looks like 'int i' for a loop is widely used in kernel.

Would you please help clarify more?

> > +
> > +       for (i = 0; i < num_clks; i++)
> > +               clk_unprepare(clks[i].clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
> 
> This does mean you have to change your "while (--i >= 0)" loops.

Is that really necessary as i thought the clk_bulk_get/put does not
guarantee any clk operation orders within the bulk?
Should we need add that support?

And currently this does the same thing as bulk regulator.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
  2017-04-12  4:03   ` Dong Aisheng
@ 2017-04-22  2:55     ` Stephen Boyd
  -1 siblings, 0 replies; 49+ messages in thread
From: Stephen Boyd @ 2017-04-22  2:55 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: linux-clk, linux-kernel, linux-arm-kernel, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On 04/12, Dong Aisheng wrote:
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d05b66..3fc6010 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -278,11 +278,25 @@ struct clk *clk_get(struct device *dev, const char *id);
>   *
>   * clk_bulk_get should not be called from within interrupt context.
>   */
> -

Should be in previous patch?

>  int __must_check clk_bulk_get(struct device *dev, int num_clks,
>  			      struct clk_bulk_data *clks);
>  
>  /**
> + * devm_clk_bulk_get - managed get multiple clk consumers
> + * @dev: device for clock "consumer"
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * Return 0 on success, an errno on failure.
> + *
> + * This helper function allows drivers to get several regulator

s/regulator/clk/

> + * consumers in one operation with management, the clks will
> + * automatically be freed when the device is unbound.
> + */
> +int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,

Thanks for the __must_check. We need to add more __must_check to
clk APIs.

> +				   struct clk_bulk_data *clks);
> +
> +/**

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
@ 2017-04-22  2:55     ` Stephen Boyd
  0 siblings, 0 replies; 49+ messages in thread
From: Stephen Boyd @ 2017-04-22  2:55 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/12, Dong Aisheng wrote:
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d05b66..3fc6010 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -278,11 +278,25 @@ struct clk *clk_get(struct device *dev, const char *id);
>   *
>   * clk_bulk_get should not be called from within interrupt context.
>   */
> -

Should be in previous patch?

>  int __must_check clk_bulk_get(struct device *dev, int num_clks,
>  			      struct clk_bulk_data *clks);
>  
>  /**
> + * devm_clk_bulk_get - managed get multiple clk consumers
> + * @dev: device for clock "consumer"
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * Return 0 on success, an errno on failure.
> + *
> + * This helper function allows drivers to get several regulator

s/regulator/clk/

> + * consumers in one operation with management, the clks will
> + * automatically be freed when the device is unbound.
> + */
> +int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,

Thanks for the __must_check. We need to add more __must_check to
clk APIs.

> +				   struct clk_bulk_data *clks);
> +
> +/**

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
  2017-04-13 14:37     ` Dong Aisheng
@ 2017-04-22  2:58       ` Stephen Boyd
  -1 siblings, 0 replies; 49+ messages in thread
From: Stephen Boyd @ 2017-04-22  2:58 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: Dong Aisheng, linux-clk, linux-kernel, linux-arm-kernel, kernel,
	broonie, yibin.gong, rjw, viresh.kumar, mturquette, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On 04/13, Dong Aisheng wrote:
> On Wed, Apr 12, 2017 at 12:03:28PM +0800, Dong Aisheng wrote:
> 
>    drivers/built-in.o: In function `devm_clk_bulk_get':
> >> (.text+0x1930e): undefined reference to `clk_bulk_get'
>    drivers/built-in.o: In function `devm_clk_bulk_release':
> >> clk-devres.c:(.text+0x19370): undefined reference to `clk_bulk_put'
> 
> clk_bulk_get is defined in clkdev.c which depends on CONFIG_CLKDEV_LOOKUP.
> However, some platforms like m68k may not select CLKDEV_LOOKUP but
> select HAVE_CLK. Thus compiling devm_clk_bulk_get may cause a undefined
> reference to 'clk_bulk_get'.
> 
> Since clk_bulk_get is built upon the platform specific clk_get api,
> clk_bulk_get can also be used by that platform accordingly.
> 
> Then we probably could move clk_bulk_get into clk-devres.c as well which
> is controlled by common CONFIG_HAVE_CLK to benifit all platforms.

clk-devres is for devm* things. I'd just make another file for
now, clk-bulk.c or something like that. When everyone moves to
common clk, we can fold it into clk.c, or not because clk.c is
rather large right now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
@ 2017-04-22  2:58       ` Stephen Boyd
  0 siblings, 0 replies; 49+ messages in thread
From: Stephen Boyd @ 2017-04-22  2:58 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/13, Dong Aisheng wrote:
> On Wed, Apr 12, 2017 at 12:03:28PM +0800, Dong Aisheng wrote:
> 
>    drivers/built-in.o: In function `devm_clk_bulk_get':
> >> (.text+0x1930e): undefined reference to `clk_bulk_get'
>    drivers/built-in.o: In function `devm_clk_bulk_release':
> >> clk-devres.c:(.text+0x19370): undefined reference to `clk_bulk_put'
> 
> clk_bulk_get is defined in clkdev.c which depends on CONFIG_CLKDEV_LOOKUP.
> However, some platforms like m68k may not select CLKDEV_LOOKUP but
> select HAVE_CLK. Thus compiling devm_clk_bulk_get may cause a undefined
> reference to 'clk_bulk_get'.
> 
> Since clk_bulk_get is built upon the platform specific clk_get api,
> clk_bulk_get can also be used by that platform accordingly.
> 
> Then we probably could move clk_bulk_get into clk-devres.c as well which
> is controlled by common CONFIG_HAVE_CLK to benifit all platforms.

clk-devres is for devm* things. I'd just make another file for
now, clk-bulk.c or something like that. When everyone moves to
common clk, we can fold it into clk.c, or not because clk.c is
rather large right now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
  2017-04-12  4:03 ` Dong Aisheng
@ 2017-04-22  3:04   ` Stephen Boyd
  -1 siblings, 0 replies; 49+ messages in thread
From: Stephen Boyd @ 2017-04-22  3:04 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: linux-clk, linux-kernel, linux-arm-kernel, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On 04/12, Dong Aisheng wrote:
> 
> Together with the err path handling for each clocks, it does make
> things a bit ugly.
> 
> Since we already have regulator_bulk_get accessories, i thought we
> probably could introduce clk_bulk_get as well to handle such case to
> ease the driver owners' life. 
> 
> Besides IMX cpufreq driver, there is also some similar cases
> in kernel which could befinit from this api as well.
> e.g.
> drivers/cpufreq/tegra124-cpufreq.c
> drivers/cpufreq/s3c2412-cpufreq.c
> sound/soc/samsung/smdk_spdif.c
> arch/arm/mach-omap1/serial.c
> ...
> 
> And actually, if we handle clocks more than 3, then it might be
> worthy to try, which there is quite many manay in kernel and
> that probably could save a lot codes.
> 
> This is a RFC patch intending to bring up the idea to discuss.
> 

Idea seems fine to me. Please also add Russell King, as we need
an ack from him on the clk.h API changes.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
@ 2017-04-22  3:04   ` Stephen Boyd
  0 siblings, 0 replies; 49+ messages in thread
From: Stephen Boyd @ 2017-04-22  3:04 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/12, Dong Aisheng wrote:
> 
> Together with the err path handling for each clocks, it does make
> things a bit ugly.
> 
> Since we already have regulator_bulk_get accessories, i thought we
> probably could introduce clk_bulk_get as well to handle such case to
> ease the driver owners' life. 
> 
> Besides IMX cpufreq driver, there is also some similar cases
> in kernel which could befinit from this api as well.
> e.g.
> drivers/cpufreq/tegra124-cpufreq.c
> drivers/cpufreq/s3c2412-cpufreq.c
> sound/soc/samsung/smdk_spdif.c
> arch/arm/mach-omap1/serial.c
> ...
> 
> And actually, if we handle clocks more than 3, then it might be
> worthy to try, which there is quite many manay in kernel and
> that probably could save a lot codes.
> 
> This is a RFC patch intending to bring up the idea to discuss.
> 

Idea seems fine to me. Please also add Russell King, as we need
an ack from him on the clk.h API changes.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-12  4:03   ` Dong Aisheng
@ 2017-04-22  3:16     ` Stephen Boyd
  -1 siblings, 0 replies; 49+ messages in thread
From: Stephen Boyd @ 2017-04-22  3:16 UTC (permalink / raw)
  To: Dong Aisheng
  Cc: linux-clk, linux-kernel, linux-arm-kernel, kernel, broonie,
	yibin.gong, rjw, viresh.kumar, mturquette, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On 04/12, Dong Aisheng wrote:
>  
>  #ifdef CONFIG_HAVE_CLK
> @@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
>  struct clk *clk_get(struct device *dev, const char *id);
>  
>  /**
> + * clk_bulk_get - lookup and obtain a number of references to clock producer.
> + * @dev: device for clock "consumer"
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * This helper function allows drivers to get several clk consumers in one
> + * operation. If any of the clk cannot be acquired then any clks
> + * that were obtained will be freed before returning to the caller.
> + *
> + * Returns 0 if all clocks specified in clk_bulk_data table are obtained
> + * successfully, or valid IS_ERR() condition containing errno.
> + * The implementation uses @dev and @clk_bulk_data.id to determine the
> + * clock consumer, and thereby the clock producer.
> + * (IOW, @id may be identical strings, but clk_get may return different
> + * clock producers depending on @dev.) The clock returned is stored in

This comment is inaccurate. Only one dev is possible with this
API.

> + * each @clk_bulk_data.clk field.
> + *
> + * Drivers must assume that the clock source is not enabled.
> + *
> + * clk_bulk_get should not be called from within interrupt context.
> + */
> +

Drop space.

> +int __must_check clk_bulk_get(struct device *dev, int num_clks,
> +			      struct clk_bulk_data *clks);
> +
> +/**
>   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>   * @dev: device for clock "consumer"
>   * @id: clock consumer ID
> @@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
>  int clk_enable(struct clk *clk);
>  
>  /**
> + * clk_bulk_enable - inform the system when the bulk of clock source should
> + *		     be running.
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * If the clock can not be enabled/disabled all, this should return success.
> + *
> + * May be called from atomic contexts.
> + *
> + * Returns success (0) or negative errno.
> + */
> +int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
> +
> +/**
>   * clk_disable - inform the system when the clock source is no longer required.
>   * @clk: clock source
>   *
> @@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * clk_bulk_disable - inform the system when the bulk of clock source is no
> + *		      longer required.
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * Inform the system that a bulk of clock source is no longer required by
> + * a driver and may be shut down.
> + *
> + * May be called from atomic contexts.
> + *
> + * Implementation detail: if the bulk of clock source is shared between

I'm not sure "bulk of clock source" is the correct terminology.
Perhaps "set of clks"?

> + * multiple drivers, clk_bulk_enable() calls must be balanced by the
> + * same number of clk_bulk_disable() calls for the clock source to be
> + * disabled.
> + */
> +void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);

We can mark clk_bulk_data structure as const here? Probably
applies in other places as well in this patch.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-04-22  3:16     ` Stephen Boyd
  0 siblings, 0 replies; 49+ messages in thread
From: Stephen Boyd @ 2017-04-22  3:16 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/12, Dong Aisheng wrote:
>  
>  #ifdef CONFIG_HAVE_CLK
> @@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
>  struct clk *clk_get(struct device *dev, const char *id);
>  
>  /**
> + * clk_bulk_get - lookup and obtain a number of references to clock producer.
> + * @dev: device for clock "consumer"
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * This helper function allows drivers to get several clk consumers in one
> + * operation. If any of the clk cannot be acquired then any clks
> + * that were obtained will be freed before returning to the caller.
> + *
> + * Returns 0 if all clocks specified in clk_bulk_data table are obtained
> + * successfully, or valid IS_ERR() condition containing errno.
> + * The implementation uses @dev and @clk_bulk_data.id to determine the
> + * clock consumer, and thereby the clock producer.
> + * (IOW, @id may be identical strings, but clk_get may return different
> + * clock producers depending on @dev.) The clock returned is stored in

This comment is inaccurate. Only one dev is possible with this
API.

> + * each @clk_bulk_data.clk field.
> + *
> + * Drivers must assume that the clock source is not enabled.
> + *
> + * clk_bulk_get should not be called from within interrupt context.
> + */
> +

Drop space.

> +int __must_check clk_bulk_get(struct device *dev, int num_clks,
> +			      struct clk_bulk_data *clks);
> +
> +/**
>   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>   * @dev: device for clock "consumer"
>   * @id: clock consumer ID
> @@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
>  int clk_enable(struct clk *clk);
>  
>  /**
> + * clk_bulk_enable - inform the system when the bulk of clock source should
> + *		     be running.
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * If the clock can not be enabled/disabled all, this should return success.
> + *
> + * May be called from atomic contexts.
> + *
> + * Returns success (0) or negative errno.
> + */
> +int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
> +
> +/**
>   * clk_disable - inform the system when the clock source is no longer required.
>   * @clk: clock source
>   *
> @@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * clk_bulk_disable - inform the system when the bulk of clock source is no
> + *		      longer required.
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * Inform the system that a bulk of clock source is no longer required by
> + * a driver and may be shut down.
> + *
> + * May be called from atomic contexts.
> + *
> + * Implementation detail: if the bulk of clock source is shared between

I'm not sure "bulk of clock source" is the correct terminology.
Perhaps "set of clks"?

> + * multiple drivers, clk_bulk_enable() calls must be balanced by the
> + * same number of clk_bulk_disable() calls for the clock source to be
> + * disabled.
> + */
> +void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);

We can mark clk_bulk_data structure as const here? Probably
applies in other places as well in this patch.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
  2017-04-22  3:04   ` Stephen Boyd
@ 2017-05-08 11:07     ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-05-08 11:07 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dong Aisheng, linux-clk, linux-kernel, linux-arm-kernel, kernel,
	broonie, yibin.gong, rjw, viresh.kumar, mturquette, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On Fri, Apr 21, 2017 at 08:04:19PM -0700, Stephen Boyd wrote:
> On 04/12, Dong Aisheng wrote:
> > 
> > Together with the err path handling for each clocks, it does make
> > things a bit ugly.
> > 
> > Since we already have regulator_bulk_get accessories, i thought we
> > probably could introduce clk_bulk_get as well to handle such case to
> > ease the driver owners' life. 
> > 
> > Besides IMX cpufreq driver, there is also some similar cases
> > in kernel which could befinit from this api as well.
> > e.g.
> > drivers/cpufreq/tegra124-cpufreq.c
> > drivers/cpufreq/s3c2412-cpufreq.c
> > sound/soc/samsung/smdk_spdif.c
> > arch/arm/mach-omap1/serial.c
> > ...
> > 
> > And actually, if we handle clocks more than 3, then it might be
> > worthy to try, which there is quite many manay in kernel and
> > that probably could save a lot codes.
> > 
> > This is a RFC patch intending to bring up the idea to discuss.
> > 
> 
> Idea seems fine to me. Please also add Russell King, as we need
> an ack from him on the clk.h API changes.
> 

Great! Thanks! And sorry to miss Russell King.
Quite willing to add him to help review in the next series.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories
@ 2017-05-08 11:07     ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-05-08 11:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Apr 21, 2017 at 08:04:19PM -0700, Stephen Boyd wrote:
> On 04/12, Dong Aisheng wrote:
> > 
> > Together with the err path handling for each clocks, it does make
> > things a bit ugly.
> > 
> > Since we already have regulator_bulk_get accessories, i thought we
> > probably could introduce clk_bulk_get as well to handle such case to
> > ease the driver owners' life. 
> > 
> > Besides IMX cpufreq driver, there is also some similar cases
> > in kernel which could befinit from this api as well.
> > e.g.
> > drivers/cpufreq/tegra124-cpufreq.c
> > drivers/cpufreq/s3c2412-cpufreq.c
> > sound/soc/samsung/smdk_spdif.c
> > arch/arm/mach-omap1/serial.c
> > ...
> > 
> > And actually, if we handle clocks more than 3, then it might be
> > worthy to try, which there is quite many manay in kernel and
> > that probably could save a lot codes.
> > 
> > This is a RFC patch intending to bring up the idea to discuss.
> > 
> 
> Idea seems fine to me. Please also add Russell King, as we need
> an ack from him on the clk.h API changes.
> 

Great! Thanks! And sorry to miss Russell King.
Quite willing to add him to help review in the next series.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
  2017-04-22  3:16     ` Stephen Boyd
@ 2017-05-08 11:34       ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-05-08 11:34 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dong Aisheng, linux-clk, linux-kernel, linux-arm-kernel, kernel,
	broonie, yibin.gong, rjw, viresh.kumar, mturquette, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On Fri, Apr 21, 2017 at 08:16:47PM -0700, Stephen Boyd wrote:
> On 04/12, Dong Aisheng wrote:
> >  
> >  #ifdef CONFIG_HAVE_CLK
> > @@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
> >  struct clk *clk_get(struct device *dev, const char *id);
> >  
> >  /**
> > + * clk_bulk_get - lookup and obtain a number of references to clock producer.
> > + * @dev: device for clock "consumer"
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * This helper function allows drivers to get several clk consumers in one
> > + * operation. If any of the clk cannot be acquired then any clks
> > + * that were obtained will be freed before returning to the caller.
> > + *
> > + * Returns 0 if all clocks specified in clk_bulk_data table are obtained
> > + * successfully, or valid IS_ERR() condition containing errno.
> > + * The implementation uses @dev and @clk_bulk_data.id to determine the
> > + * clock consumer, and thereby the clock producer.
> > + * (IOW, @id may be identical strings, but clk_get may return different
> > + * clock producers depending on @dev.) The clock returned is stored in
> 
> This comment is inaccurate. Only one dev is possible with this
> API.
> 

Good catch! Will fix it.

> > + * each @clk_bulk_data.clk field.
> > + *
> > + * Drivers must assume that the clock source is not enabled.
> > + *
> > + * clk_bulk_get should not be called from within interrupt context.
> > + */
> > +
> 
> Drop space.
> 

Got it

> > +int __must_check clk_bulk_get(struct device *dev, int num_clks,
> > +			      struct clk_bulk_data *clks);
> > +
> > +/**
> >   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
> >   * @dev: device for clock "consumer"
> >   * @id: clock consumer ID
> > @@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
> >  int clk_enable(struct clk *clk);
> >  
> >  /**
> > + * clk_bulk_enable - inform the system when the bulk of clock source should
> > + *		     be running.
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * If the clock can not be enabled/disabled all, this should return success.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Returns success (0) or negative errno.
> > + */
> > +int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
> > +
> > +/**
> >   * clk_disable - inform the system when the clock source is no longer required.
> >   * @clk: clock source
> >   *
> > @@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
> >  void clk_disable(struct clk *clk);
> >  
> >  /**
> > + * clk_bulk_disable - inform the system when the bulk of clock source is no
> > + *		      longer required.
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * Inform the system that a bulk of clock source is no longer required by
> > + * a driver and may be shut down.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Implementation detail: if the bulk of clock source is shared between
> 
> I'm not sure "bulk of clock source" is the correct terminology.
> Perhaps "set of clks"?
> 

Good to me, will change.

> > + * multiple drivers, clk_bulk_enable() calls must be balanced by the
> > + * same number of clk_bulk_disable() calls for the clock source to be
> > + * disabled.
> > + */
> > +void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);
> 
> We can mark clk_bulk_data structure as const here? Probably
> applies in other places as well in this patch.
> 

Sounds good, will change the following API,
clk_bulk_{enable|disable}
clk_bulk_{prepare|unprepare}
except clk_bulk_{get|put} which needs update clk_bulk_date.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 1/3] clk: add clk_bulk_get accessories
@ 2017-05-08 11:34       ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-05-08 11:34 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Apr 21, 2017 at 08:16:47PM -0700, Stephen Boyd wrote:
> On 04/12, Dong Aisheng wrote:
> >  
> >  #ifdef CONFIG_HAVE_CLK
> > @@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
> >  struct clk *clk_get(struct device *dev, const char *id);
> >  
> >  /**
> > + * clk_bulk_get - lookup and obtain a number of references to clock producer.
> > + * @dev: device for clock "consumer"
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * This helper function allows drivers to get several clk consumers in one
> > + * operation. If any of the clk cannot be acquired then any clks
> > + * that were obtained will be freed before returning to the caller.
> > + *
> > + * Returns 0 if all clocks specified in clk_bulk_data table are obtained
> > + * successfully, or valid IS_ERR() condition containing errno.
> > + * The implementation uses @dev and @clk_bulk_data.id to determine the
> > + * clock consumer, and thereby the clock producer.
> > + * (IOW, @id may be identical strings, but clk_get may return different
> > + * clock producers depending on @dev.) The clock returned is stored in
> 
> This comment is inaccurate. Only one dev is possible with this
> API.
> 

Good catch! Will fix it.

> > + * each @clk_bulk_data.clk field.
> > + *
> > + * Drivers must assume that the clock source is not enabled.
> > + *
> > + * clk_bulk_get should not be called from within interrupt context.
> > + */
> > +
> 
> Drop space.
> 

Got it

> > +int __must_check clk_bulk_get(struct device *dev, int num_clks,
> > +			      struct clk_bulk_data *clks);
> > +
> > +/**
> >   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
> >   * @dev: device for clock "consumer"
> >   * @id: clock consumer ID
> > @@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
> >  int clk_enable(struct clk *clk);
> >  
> >  /**
> > + * clk_bulk_enable - inform the system when the bulk of clock source should
> > + *		     be running.
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * If the clock can not be enabled/disabled all, this should return success.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Returns success (0) or negative errno.
> > + */
> > +int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
> > +
> > +/**
> >   * clk_disable - inform the system when the clock source is no longer required.
> >   * @clk: clock source
> >   *
> > @@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
> >  void clk_disable(struct clk *clk);
> >  
> >  /**
> > + * clk_bulk_disable - inform the system when the bulk of clock source is no
> > + *		      longer required.
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * Inform the system that a bulk of clock source is no longer required by
> > + * a driver and may be shut down.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Implementation detail: if the bulk of clock source is shared between
> 
> I'm not sure "bulk of clock source" is the correct terminology.
> Perhaps "set of clks"?
> 

Good to me, will change.

> > + * multiple drivers, clk_bulk_enable() calls must be balanced by the
> > + * same number of clk_bulk_disable() calls for the clock source to be
> > + * disabled.
> > + */
> > +void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);
> 
> We can mark clk_bulk_data structure as const here? Probably
> applies in other places as well in this patch.
> 

Sounds good, will change the following API,
clk_bulk_{enable|disable}
clk_bulk_{prepare|unprepare}
except clk_bulk_{get|put} which needs update clk_bulk_date.

Regards
Dong Aisheng

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
  2017-04-22  2:55     ` Stephen Boyd
@ 2017-05-08 11:37       ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-05-08 11:37 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dong Aisheng, linux-clk, linux-kernel, linux-arm-kernel, kernel,
	broonie, yibin.gong, rjw, viresh.kumar, mturquette, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On Fri, Apr 21, 2017 at 07:55:47PM -0700, Stephen Boyd wrote:
> On 04/12, Dong Aisheng wrote:
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 1d05b66..3fc6010 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -278,11 +278,25 @@ struct clk *clk_get(struct device *dev, const char *id);
> >   *
> >   * clk_bulk_get should not be called from within interrupt context.
> >   */
> > -
> 
> Should be in previous patch?
> 

Yes, will fix.

> >  int __must_check clk_bulk_get(struct device *dev, int num_clks,
> >  			      struct clk_bulk_data *clks);
> >  
> >  /**
> > + * devm_clk_bulk_get - managed get multiple clk consumers
> > + * @dev: device for clock "consumer"
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * Return 0 on success, an errno on failure.
> > + *
> > + * This helper function allows drivers to get several regulator
> 
> s/regulator/clk/
> 

ditto

> > + * consumers in one operation with management, the clks will
> > + * automatically be freed when the device is unbound.
> > + */
> > +int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
> 
> Thanks for the __must_check. We need to add more __must_check to
> clk APIs.
> 

Yes, just easy to do it from the beginning. :-)

Regards
Dong Aisheng

> > +				   struct clk_bulk_data *clks);
> > +
> > +/**
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
@ 2017-05-08 11:37       ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-05-08 11:37 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Apr 21, 2017 at 07:55:47PM -0700, Stephen Boyd wrote:
> On 04/12, Dong Aisheng wrote:
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 1d05b66..3fc6010 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -278,11 +278,25 @@ struct clk *clk_get(struct device *dev, const char *id);
> >   *
> >   * clk_bulk_get should not be called from within interrupt context.
> >   */
> > -
> 
> Should be in previous patch?
> 

Yes, will fix.

> >  int __must_check clk_bulk_get(struct device *dev, int num_clks,
> >  			      struct clk_bulk_data *clks);
> >  
> >  /**
> > + * devm_clk_bulk_get - managed get multiple clk consumers
> > + * @dev: device for clock "consumer"
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * Return 0 on success, an errno on failure.
> > + *
> > + * This helper function allows drivers to get several regulator
> 
> s/regulator/clk/
> 

ditto

> > + * consumers in one operation with management, the clks will
> > + * automatically be freed when the device is unbound.
> > + */
> > +int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
> 
> Thanks for the __must_check. We need to add more __must_check to
> clk APIs.
> 

Yes, just easy to do it from the beginning. :-)

Regards
Dong Aisheng

> > +				   struct clk_bulk_data *clks);
> > +
> > +/**
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
  2017-04-22  2:58       ` Stephen Boyd
@ 2017-05-08 11:41         ` Dong Aisheng
  -1 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-05-08 11:41 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dong Aisheng, linux-clk, linux-kernel, linux-arm-kernel, kernel,
	broonie, yibin.gong, rjw, viresh.kumar, mturquette, shawnguo,
	fabio.estevam, anson.huang, ping.bai, leonard.crestez,
	octavian.purdila

On Fri, Apr 21, 2017 at 07:58:37PM -0700, Stephen Boyd wrote:
> On 04/13, Dong Aisheng wrote:
> > On Wed, Apr 12, 2017 at 12:03:28PM +0800, Dong Aisheng wrote:
> > 
> >    drivers/built-in.o: In function `devm_clk_bulk_get':
> > >> (.text+0x1930e): undefined reference to `clk_bulk_get'
> >    drivers/built-in.o: In function `devm_clk_bulk_release':
> > >> clk-devres.c:(.text+0x19370): undefined reference to `clk_bulk_put'
> > 
> > clk_bulk_get is defined in clkdev.c which depends on CONFIG_CLKDEV_LOOKUP.
> > However, some platforms like m68k may not select CLKDEV_LOOKUP but
> > select HAVE_CLK. Thus compiling devm_clk_bulk_get may cause a undefined
> > reference to 'clk_bulk_get'.
> > 
> > Since clk_bulk_get is built upon the platform specific clk_get api,
> > clk_bulk_get can also be used by that platform accordingly.
> > 
> > Then we probably could move clk_bulk_get into clk-devres.c as well which
> > is controlled by common CONFIG_HAVE_CLK to benifit all platforms.
> 
> clk-devres is for devm* things. I'd just make another file for
> now, clk-bulk.c or something like that. When everyone moves to
> common clk, we can fold it into clk.c, or not because clk.c is
> rather large right now.
> 

Thanks for the suggestion.
Much agree with you that getting a new file to handle them is better.
Will do them in clk-bulk.c first.

Regards
Dong Aisheng

> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [RFC PATCH 2/3] clk: add managed version of clk_bulk_get
@ 2017-05-08 11:41         ` Dong Aisheng
  0 siblings, 0 replies; 49+ messages in thread
From: Dong Aisheng @ 2017-05-08 11:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Apr 21, 2017 at 07:58:37PM -0700, Stephen Boyd wrote:
> On 04/13, Dong Aisheng wrote:
> > On Wed, Apr 12, 2017 at 12:03:28PM +0800, Dong Aisheng wrote:
> > 
> >    drivers/built-in.o: In function `devm_clk_bulk_get':
> > >> (.text+0x1930e): undefined reference to `clk_bulk_get'
> >    drivers/built-in.o: In function `devm_clk_bulk_release':
> > >> clk-devres.c:(.text+0x19370): undefined reference to `clk_bulk_put'
> > 
> > clk_bulk_get is defined in clkdev.c which depends on CONFIG_CLKDEV_LOOKUP.
> > However, some platforms like m68k may not select CLKDEV_LOOKUP but
> > select HAVE_CLK. Thus compiling devm_clk_bulk_get may cause a undefined
> > reference to 'clk_bulk_get'.
> > 
> > Since clk_bulk_get is built upon the platform specific clk_get api,
> > clk_bulk_get can also be used by that platform accordingly.
> > 
> > Then we probably could move clk_bulk_get into clk-devres.c as well which
> > is controlled by common CONFIG_HAVE_CLK to benifit all platforms.
> 
> clk-devres is for devm* things. I'd just make another file for
> now, clk-bulk.c or something like that. When everyone moves to
> common clk, we can fold it into clk.c, or not because clk.c is
> rather large right now.
> 

Thanks for the suggestion.
Much agree with you that getting a new file to handle them is better.
Will do them in clk-bulk.c first.

Regards
Dong Aisheng

> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 49+ messages in thread

end of thread, other threads:[~2017-05-08 11:41 UTC | newest]

Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-12  4:03 [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories Dong Aisheng
2017-04-12  4:03 ` Dong Aisheng
2017-04-11 17:01 ` Florian Fainelli
2017-04-11 17:01   ` Florian Fainelli
2017-04-13 13:58   ` Dong Aisheng
2017-04-13 13:58     ` Dong Aisheng
2017-04-12  4:03 ` [RFC PATCH 1/3] clk: add " Dong Aisheng
2017-04-12  4:03   ` Dong Aisheng
2017-04-11 17:19   ` Leonard Crestez
2017-04-11 17:19     ` Leonard Crestez
2017-04-13 14:02     ` Dong Aisheng
2017-04-13 14:02       ` Dong Aisheng
2017-04-13 19:57     ` Geert Uytterhoeven
2017-04-13 19:57       ` Geert Uytterhoeven
2017-04-13 19:57       ` Geert Uytterhoeven
2017-04-13 14:25   ` Dong Aisheng
2017-04-13 14:25     ` Dong Aisheng
2017-04-13 19:56   ` Geert Uytterhoeven
2017-04-13 19:56     ` Geert Uytterhoeven
2017-04-13 19:56     ` Geert Uytterhoeven
2017-04-14 16:14     ` Dong Aisheng
2017-04-14 16:14       ` Dong Aisheng
2017-04-14 16:14       ` Dong Aisheng
2017-04-22  3:16   ` Stephen Boyd
2017-04-22  3:16     ` Stephen Boyd
2017-05-08 11:34     ` Dong Aisheng
2017-05-08 11:34       ` Dong Aisheng
2017-04-12  4:03 ` [RFC PATCH 2/3] clk: add managed version of clk_bulk_get Dong Aisheng
2017-04-12  4:03   ` Dong Aisheng
2017-04-13 14:37   ` Dong Aisheng
2017-04-13 14:37     ` Dong Aisheng
2017-04-22  2:58     ` Stephen Boyd
2017-04-22  2:58       ` Stephen Boyd
2017-05-08 11:41       ` Dong Aisheng
2017-05-08 11:41         ` Dong Aisheng
2017-04-22  2:55   ` Stephen Boyd
2017-04-22  2:55     ` Stephen Boyd
2017-05-08 11:37     ` Dong Aisheng
2017-05-08 11:37       ` Dong Aisheng
2017-04-12  4:03 ` [RFC PATCH 3/3] cpufreq: imx6q: refine clk operations Dong Aisheng
2017-04-12  4:03   ` Dong Aisheng
2017-04-11 17:48   ` Leonard Crestez
2017-04-11 17:48     ` Leonard Crestez
2017-04-13 14:21     ` Dong Aisheng
2017-04-13 14:21       ` Dong Aisheng
2017-04-22  3:04 ` [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories Stephen Boyd
2017-04-22  3:04   ` Stephen Boyd
2017-05-08 11:07   ` Dong Aisheng
2017-05-08 11:07     ` Dong Aisheng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.