[PATCH 0/7] ARM: davinci: add genpd support

[PATCH 0/7] ARM: davinci: add genpd support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Hi Sekhar et al,

please take a look at the following patches. They add a simple genpd
driver and use it in DT mode on da850 boards.

I was trying to use genpd in legacy mode too, but couldn't find neither
any interfaces nor users that would do that. For now I added a check in
arch/arm/mach-davinci/pm_domain.c that disables the clock pm setup if
we're using genpd.

This series applies on top of and has been tested with David Lechner's
for-bartosz branch. It fixes the clock look-up issues we faced with
lcdc and emac.

Bartosz Golaszewski (7):
  dt-bindings: soc: new driver for DaVinci genpd
  soc: davinci: new genpd driver
  ARM: davinci: don't setup pm_clk if we're using genpd
  ARM: dts: da850: add power controller nodes
  ARM: dts: da850: add power-domains properties to device nodes
  ARM: davinci: select generic power domains for DaVinci in DT mode
  ARM: davinci_all_defconfig: select the DaVinci genpd driver in DT mode

 .../bindings/soc/ti,davinci-pm-domains.txt         |  13 +++
 arch/arm/boot/dts/da850.dtsi                       |  46 ++++++++
 arch/arm/configs/davinci_all_defconfig             |   2 +
 arch/arm/mach-davinci/Kconfig                      |   1 +
 arch/arm/mach-davinci/pm_domain.c                  |   9 +-
 drivers/soc/Kconfig                                |   1 +
 drivers/soc/Makefile                               |   1 +
 drivers/soc/davinci/Kconfig                        |  16 +++
 drivers/soc/davinci/Makefile                       |   1 +
 drivers/soc/davinci/davinci_pm_domains.c           | 125 +++++++++++++++++++++
 10 files changed, 214 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
 create mode 100644 drivers/soc/davinci/Kconfig
 create mode 100644 drivers/soc/davinci/Makefile
 create mode 100644 drivers/soc/davinci/davinci_pm_domains.c

-- 
2.16.1

[PATCH 1/7] dt-bindings: soc: new driver for DaVinci genpd

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a simple document for the DaVinci genpd driver. We use clock pm
exclusively hence no reg property.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../devicetree/bindings/soc/ti,davinci-pm-domains.txt       | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt

diff --git a/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt b/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
new file mode 100644
index 000000000000..935d063c7b35
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
@@ -0,0 +1,13 @@
+Device tree bindings for the genpd driver for Texas Instruments DaVinci SoCs
+
+Required properties:
+
+- compatible:           must be "ti,davinci-pm-domains"
+- #power-domain-cells:  must be 0
+
+Example:
+
+pwc1: power-controller at 227000 {
+	compatible = "ti,davinci-pm-domains";
+	#power-domain-cells = <0>;
+};
-- 
2.16.1

[PATCH 1/7] dt-bindings: soc: new driver for DaVinci genpd

[David Lechner]

On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add a simple document for the DaVinci genpd driver. We use clock pm
> exclusively hence no reg property.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   .../devicetree/bindings/soc/ti,davinci-pm-domains.txt       | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt b/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
> new file mode 100644
> index 000000000000..935d063c7b35
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
> @@ -0,0 +1,13 @@
> +Device tree bindings for the genpd driver for Texas Instruments DaVinci SoCs
> +
> +Required properties:
> +
> +- compatible:           must be "ti,davinci-pm-domains"
> +- #power-domain-cells:  must be 0
> +
> +Example:
> +
> +pwc1: power-controller at 227000 {
> +	compatible = "ti,davinci-pm-domains";
> +	#power-domain-cells = <0>;
> +};
> 


We already have the PSC @227000. Why not just add
#power-domain-cells = <0>; to that node instead of creating
a new "device" when this is really the same device?

[PATCH 1/7] dt-bindings: soc: new driver for DaVinci genpd

[Bartosz Golaszewski]

2018-02-07 22:47 GMT+01:00 David Lechner <david@lechnology.com>:
> On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
>>
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Add a simple document for the DaVinci genpd driver. We use clock pm
>> exclusively hence no reg property.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>   .../devicetree/bindings/soc/ti,davinci-pm-domains.txt       | 13
>> +++++++++++++
>>   1 file changed, 13 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
>> b/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
>> new file mode 100644
>> index 000000000000..935d063c7b35
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
>> @@ -0,0 +1,13 @@
>> +Device tree bindings for the genpd driver for Texas Instruments DaVinci
>> SoCs
>> +
>> +Required properties:
>> +
>> +- compatible:           must be "ti,davinci-pm-domains"
>> +- #power-domain-cells:  must be 0
>> +
>> +Example:
>> +
>> +pwc1: power-controller at 227000 {
>> +       compatible = "ti,davinci-pm-domains";
>> +       #power-domain-cells = <0>;
>> +};
>>
>
>
> We already have the PSC @227000. Why not just add
> #power-domain-cells = <0>; to that node instead of creating
> a new "device" when this is really the same device?

I thought about it too, but then noticed that most architectures do
use a separate genpd driver even if it only calls routines placed in
their respective clock driver.

Let me prepare a v2 with this approach though.

Thanks,
Bartosz

[PATCH 1/7] dt-bindings: soc: new driver for DaVinci genpd

[Kevin Hilman]

Bartosz Golaszewski <brgl@bgdev.pl> writes:

> 2018-02-07 22:47 GMT+01:00 David Lechner <david@lechnology.com>:
>> On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
>>>
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> Add a simple document for the DaVinci genpd driver. We use clock pm
>>> exclusively hence no reg property.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> ---
>>>   .../devicetree/bindings/soc/ti,davinci-pm-domains.txt       | 13
>>> +++++++++++++
>>>   1 file changed, 13 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
>>> b/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
>>> new file mode 100644
>>> index 000000000000..935d063c7b35
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/ti,davinci-pm-domains.txt
>>> @@ -0,0 +1,13 @@
>>> +Device tree bindings for the genpd driver for Texas Instruments DaVinci
>>> SoCs
>>> +
>>> +Required properties:
>>> +
>>> +- compatible:           must be "ti,davinci-pm-domains"
>>> +- #power-domain-cells:  must be 0
>>> +
>>> +Example:
>>> +
>>> +pwc1: power-controller at 227000 {
>>> +       compatible = "ti,davinci-pm-domains";
>>> +       #power-domain-cells = <0>;
>>> +};
>>>
>>
>>
>> We already have the PSC @227000. Why not just add
>> #power-domain-cells = <0>; to that node instead of creating
>> a new "device" when this is really the same device?
>
> I thought about it too, but then noticed that most architectures do
> use a separate genpd driver even if it only calls routines placed in
> their respective clock driver.
>
> Let me prepare a v2 with this approach though.

Yes, I agree with David.  Just making the PSC be a power-controller is a
good approach.

Kevin

[PATCH 2/7] soc: davinci: new genpd driver

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a simple clock_pm-based driver for DaVinci SoCs. This is required
to complete the transision to using the common clock framework for this
architecture.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/soc/Kconfig                      |   1 +
 drivers/soc/Makefile                     |   1 +
 drivers/soc/davinci/Kconfig              |  16 ++++
 drivers/soc/davinci/Makefile             |   1 +
 drivers/soc/davinci/davinci_pm_domains.c | 125 +++++++++++++++++++++++++++++++
 5 files changed, 144 insertions(+)
 create mode 100644 drivers/soc/davinci/Kconfig
 create mode 100644 drivers/soc/davinci/Makefile
 create mode 100644 drivers/soc/davinci/davinci_pm_domains.c

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index fc9e98047421..f318899a6cf6 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -4,6 +4,7 @@ source "drivers/soc/actions/Kconfig"
 source "drivers/soc/amlogic/Kconfig"
 source "drivers/soc/atmel/Kconfig"
 source "drivers/soc/bcm/Kconfig"
+source "drivers/soc/davinci/Kconfig"
 source "drivers/soc/fsl/Kconfig"
 source "drivers/soc/imx/Kconfig"
 source "drivers/soc/mediatek/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index deecb16e7256..8e5fe23d6678 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_AT91)		+= atmel/
 obj-y				+= bcm/
 obj-$(CONFIG_ARCH_DOVE)		+= dove/
 obj-$(CONFIG_MACH_DOVE)		+= dove/
+obj-$(CONFIG_ARCH_DAVINCI)	+= davinci/
 obj-y				+= fsl/
 obj-$(CONFIG_ARCH_MXC)		+= imx/
 obj-$(CONFIG_SOC_XWAY)		+= lantiq/
diff --git a/drivers/soc/davinci/Kconfig b/drivers/soc/davinci/Kconfig
new file mode 100644
index 000000000000..7ee44f4ff41e
--- /dev/null
+++ b/drivers/soc/davinci/Kconfig
@@ -0,0 +1,16 @@
+#
+# TI DaVinci SOC drivers
+#
+menuconfig SOC_DAVINCI
+	bool "TI DaVinci SOC drivers support"
+
+if SOC_DAVINCI
+
+config DAVINCI_PM_DOMAINS
+	tristate "TI DaVinci PM Domains Driver"
+	depends on ARCH_DAVINCI
+	depends on PM_GENERIC_DOMAINS
+	help
+	  Generic power domains driver for TI DaVinci family of SoCs.
+
+endif # SOC_DAVINCI
diff --git a/drivers/soc/davinci/Makefile b/drivers/soc/davinci/Makefile
new file mode 100644
index 000000000000..822b03c18260
--- /dev/null
+++ b/drivers/soc/davinci/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_DAVINCI_PM_DOMAINS)        += davinci_pm_domains.o
diff --git a/drivers/soc/davinci/davinci_pm_domains.c b/drivers/soc/davinci/davinci_pm_domains.c
new file mode 100644
index 000000000000..c27d4c404d52
--- /dev/null
+++ b/drivers/soc/davinci/davinci_pm_domains.c
@@ -0,0 +1,125 @@
+/*
+ * TI DaVinci Generic Power Domain Driver
+ *
+ * Copyright (C) 2018 BayLibre SAS
+ *         Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_clock.h>
+
+struct davinci_genpd_ctx {
+	struct device *dev;
+	struct generic_pm_domain pd;
+	struct clk *pm_clk;
+};
+
+static int davinci_genpd_attach_dev(struct generic_pm_domain *domain,
+				    struct device *dev)
+{
+	struct davinci_genpd_ctx *ctx;
+	struct clk *clk;
+	int rv;
+
+	ctx = container_of(domain, struct davinci_genpd_ctx, pd);
+
+	if (dev->pm_domain)
+		return -EEXIST;
+
+	/*
+	 * DaVinci always uses a single clock for power-management. We assume
+	 * it's the first one in the clocks property.
+	 */
+	clk = of_clk_get(dev->of_node, 0);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	rv = pm_clk_create(dev);
+	if (rv) {
+		clk_put(clk);
+		return rv;
+	}
+
+	rv = pm_clk_add_clk(dev, clk);
+	if (rv) {
+		pm_clk_destroy(dev);
+		return rv;
+	}
+
+	dev_pm_domain_set(dev, &domain->domain);
+	ctx->pm_clk = clk;
+
+	return 0;
+}
+
+static void davinci_genpd_detach_dev(struct generic_pm_domain *domain,
+				     struct device *dev)
+{
+	struct davinci_genpd_ctx *ctx;
+
+	ctx = container_of(domain, struct davinci_genpd_ctx, pd);
+
+	pm_clk_remove_clk(dev, ctx->pm_clk);
+	pm_clk_destroy(dev);
+}
+
+static const struct of_device_id davinci_genpd_matches[] = {
+	{ .compatible = "ti,davinci-pm-domains", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, davinci_genpd_matches);
+
+static int davinci_genpd_probe(struct platform_device *pdev)
+{
+	struct davinci_genpd_ctx *pd;
+	struct device_node *np;
+	struct device *dev;
+
+	dev = &pdev->dev;
+	np = dev->of_node;
+
+	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+
+	pd->pd.name = devm_kasprintf(dev, GFP_KERNEL,
+				     "davinci_pwc_pd%d",
+				     of_alias_get_id(np, "pwc"));
+	if (!pd->pd.name)
+		return -ENOMEM;
+
+	pd->dev = dev;
+	pd->pd.attach_dev = davinci_genpd_attach_dev;
+	pd->pd.detach_dev = davinci_genpd_detach_dev;
+	pd->pd.flags |= GENPD_FLAG_PM_CLK;
+
+	pm_genpd_init(&pd->pd, NULL, true);
+
+	return of_genpd_add_provider_simple(np, &pd->pd);
+}
+
+static struct platform_driver davinci_genpd_driver = {
+	.probe = davinci_genpd_probe,
+	.driver = {
+		.name = "davinci_genpd",
+		.of_match_table = davinci_genpd_matches,
+	},
+};
+module_platform_driver(davinci_genpd_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TI DaVinci Generic Power Domains driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
-- 
2.16.1

[PATCH 2/7] soc: davinci: new genpd driver

[David Lechner]

On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add a simple clock_pm-based driver for DaVinci SoCs. This is required
> to complete the transision to using the common clock framework for this
> architecture.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   drivers/soc/Kconfig                      |   1 +
>   drivers/soc/Makefile                     |   1 +
>   drivers/soc/davinci/Kconfig              |  16 ++++
>   drivers/soc/davinci/Makefile             |   1 +
>   drivers/soc/davinci/davinci_pm_domains.c | 125 +++++++++++++++++++++++++++++++
>   5 files changed, 144 insertions(+)
>   create mode 100644 drivers/soc/davinci/Kconfig
>   create mode 100644 drivers/soc/davinci/Makefile
>   create mode 100644 drivers/soc/davinci/davinci_pm_domains.c
> 
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index fc9e98047421..f318899a6cf6 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -4,6 +4,7 @@ source "drivers/soc/actions/Kconfig"
>   source "drivers/soc/amlogic/Kconfig"
>   source "drivers/soc/atmel/Kconfig"
>   source "drivers/soc/bcm/Kconfig"
> +source "drivers/soc/davinci/Kconfig"
>   source "drivers/soc/fsl/Kconfig"
>   source "drivers/soc/imx/Kconfig"
>   source "drivers/soc/mediatek/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index deecb16e7256..8e5fe23d6678 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_AT91)		+= atmel/
>   obj-y				+= bcm/
>   obj-$(CONFIG_ARCH_DOVE)		+= dove/
>   obj-$(CONFIG_MACH_DOVE)		+= dove/
> +obj-$(CONFIG_ARCH_DAVINCI)	+= davinci/
>   obj-y				+= fsl/
>   obj-$(CONFIG_ARCH_MXC)		+= imx/
>   obj-$(CONFIG_SOC_XWAY)		+= lantiq/
> diff --git a/drivers/soc/davinci/Kconfig b/drivers/soc/davinci/Kconfig
> new file mode 100644
> index 000000000000..7ee44f4ff41e
> --- /dev/null
> +++ b/drivers/soc/davinci/Kconfig
> @@ -0,0 +1,16 @@
> +#
> +# TI DaVinci SOC drivers
> +#
> +menuconfig SOC_DAVINCI
> +	bool "TI DaVinci SOC drivers support"
> +
> +if SOC_DAVINCI
> +
> +config DAVINCI_PM_DOMAINS
> +	tristate "TI DaVinci PM Domains Driver"
> +	depends on ARCH_DAVINCI
> +	depends on PM_GENERIC_DOMAINS
> +	help
> +	  Generic power domains driver for TI DaVinci family of SoCs.
> +
> +endif # SOC_DAVINCI
> diff --git a/drivers/soc/davinci/Makefile b/drivers/soc/davinci/Makefile
> new file mode 100644
> index 000000000000..822b03c18260
> --- /dev/null
> +++ b/drivers/soc/davinci/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_DAVINCI_PM_DOMAINS)        += davinci_pm_domains.o
> diff --git a/drivers/soc/davinci/davinci_pm_domains.c b/drivers/soc/davinci/davinci_pm_domains.c
> new file mode 100644
> index 000000000000..c27d4c404d52
> --- /dev/null
> +++ b/drivers/soc/davinci/davinci_pm_domains.c
> @@ -0,0 +1,125 @@
> +/*
> + * TI DaVinci Generic Power Domain Driver
> + *
> + * Copyright (C) 2018 BayLibre SAS
> + *         Bartosz Golaszewski <bgolaszewski@baylibre.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_clock.h>
> +
> +struct davinci_genpd_ctx {
> +	struct device *dev;
> +	struct generic_pm_domain pd;
> +	struct clk *pm_clk;
> +};
> +
> +static int davinci_genpd_attach_dev(struct generic_pm_domain *domain,
> +				    struct device *dev)
> +{
> +	struct davinci_genpd_ctx *ctx;
> +	struct clk *clk;
> +	int rv;
> +
> +	ctx = container_of(domain, struct davinci_genpd_ctx, pd);
> +
> +	if (dev->pm_domain)
> +		return -EEXIST;
> +
> +	/*
> +	 * DaVinci always uses a single clock for power-management. We assume
> +	 * it's the first one in the clocks property.
> +	 */
> +	clk = of_clk_get(dev->of_node, 0);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	rv = pm_clk_create(dev);
> +	if (rv) {
> +		clk_put(clk);
> +		return rv;
> +	}
> +
> +	rv = pm_clk_add_clk(dev, clk);
> +	if (rv) {
> +		pm_clk_destroy(dev);
> +		return rv;
> +	}
> +
> +	dev_pm_domain_set(dev, &domain->domain);
> +	ctx->pm_clk = clk;
> +
> +	return 0;
> +}
> +
> +static void davinci_genpd_detach_dev(struct generic_pm_domain *domain,
> +				     struct device *dev)
> +{
> +	struct davinci_genpd_ctx *ctx;
> +
> +	ctx = container_of(domain, struct davinci_genpd_ctx, pd);
> +
> +	pm_clk_remove_clk(dev, ctx->pm_clk);
> +	pm_clk_destroy(dev);
> +}
> +
> +static const struct of_device_id davinci_genpd_matches[] = {
> +	{ .compatible = "ti,davinci-pm-domains", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, davinci_genpd_matches);
> +
> +static int davinci_genpd_probe(struct platform_device *pdev)
> +{
> +	struct davinci_genpd_ctx *pd;
> +	struct device_node *np;
> +	struct device *dev;
> +
> +	dev = &pdev->dev;
> +	np = dev->of_node;
> +
> +	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +
> +	pd->pd.name = devm_kasprintf(dev, GFP_KERNEL,
> +				     "davinci_pwc_pd%d",
> +				     of_alias_get_id(np, "pwc"));
> +	if (!pd->pd.name)
> +		return -ENOMEM;
> +
> +	pd->dev = dev;
> +	pd->pd.attach_dev = davinci_genpd_attach_dev;
> +	pd->pd.detach_dev = davinci_genpd_detach_dev;
> +	pd->pd.flags |= GENPD_FLAG_PM_CLK;
> +
> +	pm_genpd_init(&pd->pd, NULL, true);
> +
> +	return of_genpd_add_provider_simple(np, &pd->pd);
> +}
> +
> +static struct platform_driver davinci_genpd_driver = {
> +	.probe = davinci_genpd_probe,
> +	.driver = {
> +		.name = "davinci_genpd",
> +		.of_match_table = davinci_genpd_matches,
> +	},
> +};
> +module_platform_driver(davinci_genpd_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("TI DaVinci Generic Power Domains driver");
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> 

Likewise, I don't think we need a separate driver for this. Just
register a genpd provider from the PSC clock driver.

[PATCH 2/7] soc: davinci: new genpd driver

[Sekhar Nori]

On Wednesday 07 February 2018 07:15 PM, Bartosz Golaszewski wrote:
> +	/*
> +	 * DaVinci always uses a single clock for power-management. We assume
> +	 * it's the first one in the clocks property.
> +	 */
> +	clk = of_clk_get(dev->of_node, 0);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);

We already get this today with drivers/base/power/clock_ops.c once
.con_ids list is dropped from pm_clk_notifier_block (which I think it
should).

If there is no reason to introduce thus functionality at this stage,
perhaps we should wait till such a time when its clearly needed?

Thanks,
Sekhar

[PATCH 2/7] soc: davinci: new genpd driver

[Bartosz Golaszewski]

2018-02-08 10:30 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Wednesday 07 February 2018 07:15 PM, Bartosz Golaszewski wrote:
>> +     /*
>> +      * DaVinci always uses a single clock for power-management. We assume
>> +      * it's the first one in the clocks property.
>> +      */
>> +     clk = of_clk_get(dev->of_node, 0);
>> +     if (IS_ERR(clk))
>> +             return PTR_ERR(clk);
>
> We already get this today with drivers/base/power/clock_ops.c once
> .con_ids list is dropped from pm_clk_notifier_block (which I think it
> should).
>
> If there is no reason to introduce thus functionality at this stage,
> perhaps we should wait till such a time when its clearly needed?
>
> Thanks,
> Sekhar

If I understand correctly: once we drop the con_ids list, we end up
calling clk_get(dev, NULL) from pm_clk_acquire(), which matches
against the clock with NULL con_id, which may not necessarily be the
first clock in the list.

I'm working on extending the psc driver with the power-domain code (as
suggested by David), which should be much smaller and simplier - how
about that?

Thanks,
Bartosz

[PATCH 2/7] soc: davinci: new genpd driver

[Sekhar Nori]

On Thursday 08 February 2018 03:24 PM, Bartosz Golaszewski wrote:
> 2018-02-08 10:30 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Wednesday 07 February 2018 07:15 PM, Bartosz Golaszewski wrote:
>>> +     /*
>>> +      * DaVinci always uses a single clock for power-management. We assume
>>> +      * it's the first one in the clocks property.
>>> +      */
>>> +     clk = of_clk_get(dev->of_node, 0);
>>> +     if (IS_ERR(clk))
>>> +             return PTR_ERR(clk);
>>
>> We already get this today with drivers/base/power/clock_ops.c once
>> .con_ids list is dropped from pm_clk_notifier_block (which I think it
>> should).
>>
>> If there is no reason to introduce thus functionality at this stage,
>> perhaps we should wait till such a time when its clearly needed?
>>
>> Thanks,
>> Sekhar
> 
> If I understand correctly: once we drop the con_ids list, we end up
> calling clk_get(dev, NULL) from pm_clk_acquire(), which matches
> against the clock with NULL con_id, which may not necessarily be the
> first clock in the list.

Hmm, not sure of this. In __of_clk_get_by_name() called by clk_get():

int index = 0;

/*
 * For named clocks, first look up the name in the
 * "clock-names" property.  If it cannot be found, then
 * index will be an error code, and of_clk_get() will fail.
 */
if (name)
	index = of_property_match_string(np, "clock-names", name);

So, if no con_id is provided (name == NULL), then index is set to 0
which will always get the first clock in clocks = list.

Thanks,
Sekhar

[PATCH 2/7] soc: davinci: new genpd driver

[Bartosz Golaszewski]

2018-02-08 13:56 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Thursday 08 February 2018 03:24 PM, Bartosz Golaszewski wrote:
>> 2018-02-08 10:30 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>>> On Wednesday 07 February 2018 07:15 PM, Bartosz Golaszewski wrote:
>>>> +     /*
>>>> +      * DaVinci always uses a single clock for power-management. We assume
>>>> +      * it's the first one in the clocks property.
>>>> +      */
>>>> +     clk = of_clk_get(dev->of_node, 0);
>>>> +     if (IS_ERR(clk))
>>>> +             return PTR_ERR(clk);
>>>
>>> We already get this today with drivers/base/power/clock_ops.c once
>>> .con_ids list is dropped from pm_clk_notifier_block (which I think it
>>> should).
>>>
>>> If there is no reason to introduce thus functionality at this stage,
>>> perhaps we should wait till such a time when its clearly needed?
>>>
>>> Thanks,
>>> Sekhar
>>
>> If I understand correctly: once we drop the con_ids list, we end up
>> calling clk_get(dev, NULL) from pm_clk_acquire(), which matches
>> against the clock with NULL con_id, which may not necessarily be the
>> first clock in the list.
>
> Hmm, not sure of this. In __of_clk_get_by_name() called by clk_get():
>
> int index = 0;
>
> /*
>  * For named clocks, first look up the name in the
>  * "clock-names" property.  If it cannot be found, then
>  * index will be an error code, and of_clk_get() will fail.
>  */
> if (name)
>         index = of_property_match_string(np, "clock-names", name);
>
> So, if no con_id is provided (name == NULL), then index is set to 0
> which will always get the first clock in clocks = list.
>

But we're talking here about device tree mode. In legacy mode the
device_node pointer will be NULL, __of_clk_get_by_name() will return
-ENOENT and we'll end up calling clk_get_sys() -> clk_find(). We'll
then iterate over the clock entries and check the following:

(...)
152                 if (p->con_id) {
153                         if (!con_id || strcmp(p->con_id, con_id))
154                                 continue;
155                         match += 1;
156                 }
(...)

So we'll skip the first clock if it has a con_id and we passed an
empty con_id to clk_get().

Bartosz

[PATCH 2/7] soc: davinci: new genpd driver

[Sekhar Nori]

On Thursday 08 February 2018 06:57 PM, Bartosz Golaszewski wrote:
> 2018-02-08 13:56 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Thursday 08 February 2018 03:24 PM, Bartosz Golaszewski wrote:
>>> 2018-02-08 10:30 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>>>> On Wednesday 07 February 2018 07:15 PM, Bartosz Golaszewski wrote:
>>>>> +     /*
>>>>> +      * DaVinci always uses a single clock for power-management. We assume
>>>>> +      * it's the first one in the clocks property.
>>>>> +      */
>>>>> +     clk = of_clk_get(dev->of_node, 0);
>>>>> +     if (IS_ERR(clk))
>>>>> +             return PTR_ERR(clk);
>>>>
>>>> We already get this today with drivers/base/power/clock_ops.c once
>>>> .con_ids list is dropped from pm_clk_notifier_block (which I think it
>>>> should).
>>>>
>>>> If there is no reason to introduce thus functionality at this stage,
>>>> perhaps we should wait till such a time when its clearly needed?
>>>>
>>>> Thanks,
>>>> Sekhar
>>>
>>> If I understand correctly: once we drop the con_ids list, we end up
>>> calling clk_get(dev, NULL) from pm_clk_acquire(), which matches
>>> against the clock with NULL con_id, which may not necessarily be the
>>> first clock in the list.
>>
>> Hmm, not sure of this. In __of_clk_get_by_name() called by clk_get():
>>
>> int index = 0;
>>
>> /*
>>  * For named clocks, first look up the name in the
>>  * "clock-names" property.  If it cannot be found, then
>>  * index will be an error code, and of_clk_get() will fail.
>>  */
>> if (name)
>>         index = of_property_match_string(np, "clock-names", name);
>>
>> So, if no con_id is provided (name == NULL), then index is set to 0
>> which will always get the first clock in clocks = list.
>>
> 
> But we're talking here about device tree mode. In legacy mode the
> device_node pointer will be NULL, __of_clk_get_by_name() will return
> -ENOENT and we'll end up calling clk_get_sys() -> clk_find(). We'll
> then iterate over the clock entries and check the following:
> 
> (...)
> 152                 if (p->con_id) {
> 153                         if (!con_id || strcmp(p->con_id, con_id))
> 154                                 continue;
> 155                         match += 1;
> 156                 }
> (...)
> 
> So we'll skip the first clock if it has a con_id and we passed an
> empty con_id to clk_get().

You are right. I missed taking the effect on legacy boot into
consideration. Please send v2 then, and consider this objection withdrawn.

Thanks,
Sekhar

[PATCH 3/7] ARM: davinci: don't setup pm_clk if we're using genpd

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

As the first step in switching to using the genpd driver in DT mode,
check if a relevant genpd node exists and don't setup the clock pm in
this case.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/pm_domain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-davinci/pm_domain.c b/arch/arm/mach-davinci/pm_domain.c
index 78eac2c0c146..98a4f3fcba50 100644
--- a/arch/arm/mach-davinci/pm_domain.c
+++ b/arch/arm/mach-davinci/pm_domain.c
@@ -13,6 +13,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/pm_clock.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 static struct dev_pm_domain davinci_pm_domain = {
 	.ops = {
@@ -28,7 +29,13 @@ static struct pm_clk_notifier_block platform_bus_notifier = {
 
 static int __init davinci_pm_runtime_init(void)
 {
-	pm_clk_add_notifier(&platform_bus_type, &platform_bus_notifier);
+	struct device_node *np;
+
+	/* Use pm_clk as fallback if we're not using genpd. */
+	np = of_find_compatible_node(NULL, NULL, "ti,davinci-pm-domains");
+	if (!np)
+		pm_clk_add_notifier(&platform_bus_type,
+				    &platform_bus_notifier);
 
 	return 0;
 }
-- 
2.16.1

[PATCH 4/7] ARM: dts: da850: add power controller nodes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add two power controller nodes corresponding with the two PSC modules
present on the da850 SoC.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 3a1f2ced05ca..ac2eef4e6b7c 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -24,6 +24,11 @@
 		};
 	};
 
+	aliases {
+		pwc0 = &pwc0;
+		pwc1 = &pwc1;
+	};
+
 	opp_table: opp-table {
 		compatible = "operating-points-v2";
 
@@ -141,6 +146,10 @@
 				      "pll0_sysclk4", "pll0_sysclk6",
 				      "async1";
 		};
+		pwc0: power-controller at 10000 {
+			compatible = "ti,davinci-pm-domains";
+			#power-domain-cells = <0>;
+		};
 		pll0: clock-controller at 11000 {
 			compatible = "ti,da850-pll0";
 			reg = <0x11000 0x1000>;
@@ -757,6 +766,10 @@
 				 <&async3_clk>;
 			clock-names = "pll0_sysclk2", "pll0_sysclk4", "async3";
 		};
+		pwc1: power-controller at 227000 {
+			compatible = "ti,davinci-pm-domains";
+			#power-domain-cells = <0>;
+		};
 		pinconf: pin-controller at 22c00c {
 			compatible = "ti,da850-pupd";
 			reg = <0x22c00c 0x8>;
-- 
2.16.1

[PATCH 4/7] ARM: dts: da850: add power controller nodes

[David Lechner]

On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add two power controller nodes corresponding with the two PSC modules
> present on the da850 SoC.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   arch/arm/boot/dts/da850.dtsi | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 3a1f2ced05ca..ac2eef4e6b7c 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -24,6 +24,11 @@
>   		};
>   	};
>   
> +	aliases {
> +		pwc0 = &pwc0;
> +		pwc1 = &pwc1;
> +	};

Why do we need aliases? (would be nice to have the reason in the commit
message if there is a good reason)

> +
>   	opp_table: opp-table {
>   		compatible = "operating-points-v2";
>   
> @@ -141,6 +146,10 @@
>   				      "pll0_sysclk4", "pll0_sysclk6",
>   				      "async1";
>   		};
> +		pwc0: power-controller at 10000 {
> +			compatible = "ti,davinci-pm-domains";
> +			#power-domain-cells = <0>;
> +		};
>   		pll0: clock-controller at 11000 {
>   			compatible = "ti,da850-pll0";
>   			reg = <0x11000 0x1000>;
> @@ -757,6 +766,10 @@
>   				 <&async3_clk>;
>   			clock-names = "pll0_sysclk2", "pll0_sysclk4", "async3";
>   		};
> +		pwc1: power-controller at 227000 {
> +			compatible = "ti,davinci-pm-domains";
> +			#power-domain-cells = <0>;
> +		};
>   		pinconf: pin-controller at 22c00c {
>   			compatible = "ti,da850-pupd";
>   			reg = <0x22c00c 0x8>;
> 

[PATCH 5/7] ARM: dts: da850: add power-domains properties to device nodes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Make all devices managed by PSCs use the genpd driver.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index ac2eef4e6b7c..a2a1a3b7de3c 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -83,6 +83,7 @@
 			ti,intc-size = <101>;
 			reg = <0xfffee000 0x2000>;
 			clocks = <&psc0 6>;
+			power-domains = <&pwc0>;
 		};
 	};
 	clocks: clocks {
@@ -125,6 +126,7 @@
 		interrupt-parent = <&intc>;
 		interrupts = <28>;
 		clocks = <&psc0 15>;
+		power-domains = <&pwc0>;
 		status = "disabled";
 	};
 	soc at 1c00000 {
@@ -400,12 +402,14 @@
 				clocks = <&psc1 1>, <&usb_refclkin>,
 					 <&pll0_auxclk>;
 				clock-names = "fck", "usb_refclkin", "auxclk";
+				power-domains = <&pwc1>;
 			};
 			ehrpwm_tbclk: ehrpwm_tbclk {
 				compatible = "ti,da830-tbclksync";
 				#clock-cells = <0>;
 				clocks = <&psc1 17>;
 				clock-names = "fck";
+				power-domains = <&pwc1>;
 			};
 			div4p5_clk: div4.5 {
 				compatible = "ti,da830-div4p5ena";
@@ -439,6 +443,7 @@
 
 			ti,tptcs = <&edma0_tptc0 7>, <&edma0_tptc1 0>;
 			clocks = <&psc0 0>;
+			power-domains = <&pwc0>;
 		};
 		edma0_tptc0: tptc at 8000 {
 			compatible = "ti,edma3-tptc";
@@ -446,6 +451,7 @@
 			interrupts = <13>;
 			interrupt-names = "edm3_tcerrint";
 			clocks = <&psc0 1>;
+			power-domains = <&pwc0>;
 		};
 		edma0_tptc1: tptc at 8400 {
 			compatible = "ti,edma3-tptc";
@@ -453,6 +459,7 @@
 			interrupts = <32>;
 			interrupt-names = "edm3_tcerrint";
 			clocks = <&psc0 2>;
+			power-domains = <&pwc0>;
 		};
 		edma1: edma at 230000 {
 			compatible = "ti,edma3-tpcc";
@@ -465,6 +472,7 @@
 
 			ti,tptcs = <&edma1_tptc0 7>;
 			clocks = <&psc1 0>;
+			power-domains = <&pwc1>;
 		};
 		edma1_tptc0: tptc at 238000 {
 			compatible = "ti,edma3-tptc";
@@ -472,6 +480,7 @@
 			interrupts = <95>;
 			interrupt-names = "edm3_tcerrint";
 			clocks = <&psc1 21>;
+			power-domains = <&pwc1>;
 		};
 		serial0: serial at 42000 {
 			compatible = "ti,da830-uart", "ns16550a";
@@ -480,6 +489,7 @@
 			reg-shift = <2>;
 			interrupts = <25>;
 			clocks = <&psc0 9>;
+			power-domains = <&pwc0>;
 			status = "disabled";
 		};
 		serial1: serial at 10c000 {
@@ -489,6 +499,7 @@
 			reg-shift = <2>;
 			interrupts = <53>;
 			clocks = <&psc1 12>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 		serial2: serial at 10d000 {
@@ -498,6 +509,7 @@
 			reg-shift = <2>;
 			interrupts = <61>;
 			clocks = <&psc1 13>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 		rtc0: rtc at 23000 {
@@ -523,6 +535,7 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			clocks = <&psc1 11>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 		clocksource: timer at 20000 {
@@ -545,6 +558,7 @@
 			dmas = <&edma0 16 0>, <&edma0 17 0>;
 			dma-names = "rx", "tx";
 			clocks = <&psc0 5>;
+			power-domains = <&pwc0>;
 			status = "disabled";
 		};
 		vpif: video at 217000 {
@@ -552,6 +566,7 @@
 			reg = <0x217000 0x1000>;
 			interrupts = <92>;
 			clocks = <&psc1 9>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 
 			/* VPIF capture port */
@@ -575,6 +590,7 @@
 			dmas = <&edma1 28 0>, <&edma1 29 0>;
 			dma-names = "rx", "tx";
 			clocks = <&psc1 18>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 		ehrpwm0: pwm at 300000 {
@@ -585,6 +601,7 @@
 			clocks = <&psc1 17>, <&ehrpwm_tbclk>;
 			clock-names = "fck", "tbclk";
 			status = "disabled";
+			power-domains = <&pwc1>;
 		};
 		ehrpwm1: pwm at 302000 {
 			compatible = "ti,da850-ehrpwm", "ti,am3352-ehrpwm",
@@ -592,6 +609,7 @@
 			#pwm-cells = <3>;
 			reg = <0x302000 0x2000>;
 			clocks = <&psc1 17>, <&ehrpwm_tbclk>;
+			power-domains = <&pwc1>;
 			clock-names = "fck", "tbclk";
 			status = "disabled";
 		};
@@ -601,6 +619,7 @@
 			#pwm-cells = <3>;
 			reg = <0x306000 0x80>;
 			clocks = <&psc1 20>;
+			power-domains = <&pwc1>;
 			clock-names = "fck";
 			status = "disabled";
 		};
@@ -610,6 +629,7 @@
 			#pwm-cells = <3>;
 			reg = <0x307000 0x80>;
 			clocks = <&psc1 20>;
+			power-domains = <&pwc1>;
 			clock-names = "fck";
 			status = "disabled";
 		};
@@ -619,6 +639,7 @@
 			#pwm-cells = <3>;
 			reg = <0x308000 0x80>;
 			clocks = <&psc1 20>;
+			power-domains = <&pwc1>;
 			clock-names = "fck";
 			status = "disabled";
 		};
@@ -633,6 +654,7 @@
 			dmas = <&edma0 14 0>, <&edma0 15 0>;
 			dma-names = "rx", "tx";
 			clocks = <&psc0 4>;
+			power-domains = <&pwc0>;
 			status = "disabled";
 		};
 		spi1: spi at 30e000 {
@@ -646,6 +668,7 @@
 			dmas = <&edma0 18 0>, <&edma0 19 0>;
 			dma-names = "rx", "tx";
 			clocks = <&psc1 10>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 		usb0: usb at 200000 {
@@ -658,6 +681,7 @@
 			phys = <&usb_phy 0>;
 			phy-names = "usb-phy";
 			clocks = <&psc1 1>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 
 			#address-cells = <1>;
@@ -682,6 +706,7 @@
 				#dma-cells = <2>;
 				#dma-channels = <4>;
 				clocks = <&psc1 1>;
+				power-domains = <&pwc1>;
 				status = "okay";
 			};
 		};
@@ -690,6 +715,7 @@
 			reg = <0x218000 0x2000>, <0x22c018 0x4>;
 			interrupts = <67>;
 			clocks = <&psc1 8>, <&sata_refclk>;
+			power-domains = <&pwc1>;
 			clock-names = "fck", "refclk";
 			status = "disabled";
 		};
@@ -713,6 +739,7 @@
 			reg = <0x224000 0x1000>;
 			clocks = <&psc1 5>;
 			clock-names = "fck";
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 		eth0: ethernet at 220000 {
@@ -729,6 +756,7 @@
 					36
 					>;
 			clocks = <&psc1 5>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 		usb1: usb at 225000 {
@@ -738,6 +766,7 @@
 			phys = <&usb_phy 1>;
 			phy-names = "usb-phy";
 			clocks = <&psc1 2>;
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 		gpio: gpio at 226000 {
@@ -756,6 +785,7 @@
 			interrupt-controller;
 			#interrupt-cells = <2>;
 			clocks = <&psc1 3>;
+			power-domains = <&pwc1>;
 			clock-names = "gpio";
 		};
 		psc1: clock-controller at 227000 {
@@ -788,6 +818,7 @@
 				<&edma0 0 1>;
 			dma-names = "tx", "rx";
 			clocks = <&psc1 7>;
+			power-domains = <&pwc1>;
 		};
 
 		lcdc: display at 213000 {
@@ -797,6 +828,7 @@
 			max-pixelclock = <37500>;
 			clocks = <&psc1 16>;
 			clock-names = "fck";
+			power-domains = <&pwc1>;
 			status = "disabled";
 		};
 	};
@@ -809,6 +841,7 @@
 		ranges = <0 0 0x60000000 0x08000000
 			  1 0 0x68000000 0x00008000>;
 		clocks = <&psc0 3>;
+		power-domains = <&pwc0>;
 		status = "disabled";
 	};
 	memctrl: memory-controller at b0000000 {
-- 
2.16.1

[PATCH 5/7] ARM: dts: da850: add power-domains properties to device nodes

[David Lechner]

On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Make all devices managed by PSCs use the genpd driver.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   arch/arm/boot/dts/da850.dtsi | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index ac2eef4e6b7c..a2a1a3b7de3c 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -83,6 +83,7 @@
>   			ti,intc-size = <101>;
>   			reg = <0xfffee000 0x2000>;
>   			clocks = <&psc0 6>;
> +			power-domains = <&pwc0>;

If we are making this a power domain consumer, we can probably drop the
clock property.

>   		};
>   	};
>   	clocks: clocks {
> @@ -125,6 +126,7 @@
>   		interrupt-parent = <&intc>;
>   		interrupts = <28>;
>   		clocks = <&psc0 15>;
> +		power-domains = <&pwc0>;
>   		status = "disabled";
>   	};
>   	soc at 1c00000 {
> @@ -400,12 +402,14 @@
>   				clocks = <&psc1 1>, <&usb_refclkin>,
>   					 <&pll0_auxclk>;
>   				clock-names = "fck", "usb_refclkin", "auxclk";
> +				power-domains = <&pwc1>;

If we are going to use a power domain here, this driver will need to be re-written.

>   			};
>   			ehrpwm_tbclk: ehrpwm_tbclk {
>   				compatible = "ti,da830-tbclksync";
>   				#clock-cells = <0>;
>   				clocks = <&psc1 17>;
>   				clock-names = "fck";
> +				power-domains = <&pwc1>;

This is a clock node, not a (platform) device node, so I am not sure having
a power domain here makes sense.

>   			};
>   			div4p5_clk: div4.5 {
>   				compatible = "ti,da830-div4p5ena";
> @@ -439,6 +443,7 @@
>   
>   			ti,tptcs = <&edma0_tptc0 7>, <&edma0_tptc1 0>;
>   			clocks = <&psc0 0>;
> +			power-domains = <&pwc0>;

We really just need power-domains here, not clocks. Same applies to
the rest of the edma* nodes below.

>   		};
>   		edma0_tptc0: tptc at 8000 {
>   			compatible = "ti,edma3-tptc";
> @@ -446,6 +451,7 @@
>   			interrupts = <13>;
>   			interrupt-names = "edm3_tcerrint";
>   			clocks = <&psc0 1>;
> +			power-domains = <&pwc0>;
>   		};
>   		edma0_tptc1: tptc at 8400 {
>   			compatible = "ti,edma3-tptc";
> @@ -453,6 +459,7 @@
>   			interrupts = <32>;
>   			interrupt-names = "edm3_tcerrint";
>   			clocks = <&psc0 2>;
> +			power-domains = <&pwc0>;
>   		};
>   		edma1: edma at 230000 {
>   			compatible = "ti,edma3-tpcc";
> @@ -465,6 +472,7 @@
>   
>   			ti,tptcs = <&edma1_tptc0 7>;
>   			clocks = <&psc1 0>;
> +			power-domains = <&pwc1>;
>   		};
>   		edma1_tptc0: tptc at 238000 {
>   			compatible = "ti,edma3-tptc";
> @@ -472,6 +480,7 @@
>   			interrupts = <95>;
>   			interrupt-names = "edm3_tcerrint";
>   			clocks = <&psc1 21>;
> +			power-domains = <&pwc1>;
>   		};
>   		serial0: serial at 42000 {
>   			compatible = "ti,da830-uart", "ns16550a";
> @@ -480,6 +489,7 @@
>   			reg-shift = <2>;
>   			interrupts = <25>;
>   			clocks = <&psc0 9>;
> +			power-domains = <&pwc0>;
>   			status = "disabled";
>   		};
>   		serial1: serial at 10c000 {
> @@ -489,6 +499,7 @@
>   			reg-shift = <2>;
>   			interrupts = <53>;
>   			clocks = <&psc1 12>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   		serial2: serial at 10d000 {
> @@ -498,6 +509,7 @@
>   			reg-shift = <2>;
>   			interrupts = <61>;
>   			clocks = <&psc1 13>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   		rtc0: rtc at 23000 {
> @@ -523,6 +535,7 @@
>   			#address-cells = <1>;
>   			#size-cells = <0>;
>   			clocks = <&psc1 11>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   		clocksource: timer at 20000 {
> @@ -545,6 +558,7 @@
>   			dmas = <&edma0 16 0>, <&edma0 17 0>;
>   			dma-names = "rx", "tx";
>   			clocks = <&psc0 5>;
> +			power-domains = <&pwc0>;
>   			status = "disabled";
>   		};
>   		vpif: video at 217000 {
> @@ -552,6 +566,7 @@
>   			reg = <0x217000 0x1000>;
>   			interrupts = <92>;
>   			clocks = <&psc1 9>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   
>   			/* VPIF capture port */
> @@ -575,6 +590,7 @@
>   			dmas = <&edma1 28 0>, <&edma1 29 0>;
>   			dma-names = "rx", "tx";
>   			clocks = <&psc1 18>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   		ehrpwm0: pwm at 300000 {
> @@ -585,6 +601,7 @@
>   			clocks = <&psc1 17>, <&ehrpwm_tbclk>;
>   			clock-names = "fck", "tbclk";
>   			status = "disabled";
> +			power-domains = <&pwc1>;

nit picking: it would be nice to have power-domains before status just
to be consistent in the order.

>   		};
>   		ehrpwm1: pwm at 302000 {
>   			compatible = "ti,da850-ehrpwm", "ti,am3352-ehrpwm",
> @@ -592,6 +609,7 @@
>   			#pwm-cells = <3>;
>   			reg = <0x302000 0x2000>;
>   			clocks = <&psc1 17>, <&ehrpwm_tbclk>;
> +			power-domains = <&pwc1>;
>   			clock-names = "fck", "tbclk";

nit picking: it would be nice to keep clocks and clock-names next to each
other. Also several places below.

>   			status = "disabled";
>   		};
> @@ -601,6 +619,7 @@
>   			#pwm-cells = <3>;
>   			reg = <0x306000 0x80>;
>   			clocks = <&psc1 20>;
> +			power-domains = <&pwc1>;
>   			clock-names = "fck";
>   			status = "disabled";
>   		};
> @@ -610,6 +629,7 @@
>   			#pwm-cells = <3>;
>   			reg = <0x307000 0x80>;
>   			clocks = <&psc1 20>;
> +			power-domains = <&pwc1>;
>   			clock-names = "fck";
>   			status = "disabled";
>   		};
> @@ -619,6 +639,7 @@
>   			#pwm-cells = <3>;
>   			reg = <0x308000 0x80>;
>   			clocks = <&psc1 20>;
> +			power-domains = <&pwc1>;
>   			clock-names = "fck";
>   			status = "disabled";
>   		};
> @@ -633,6 +654,7 @@
>   			dmas = <&edma0 14 0>, <&edma0 15 0>;
>   			dma-names = "rx", "tx";
>   			clocks = <&psc0 4>;
> +			power-domains = <&pwc0>;
>   			status = "disabled";
>   		};
>   		spi1: spi at 30e000 {
> @@ -646,6 +668,7 @@
>   			dmas = <&edma0 18 0>, <&edma0 19 0>;
>   			dma-names = "rx", "tx";
>   			clocks = <&psc1 10>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   		usb0: usb at 200000 {
> @@ -658,6 +681,7 @@
>   			phys = <&usb_phy 0>;
>   			phy-names = "usb-phy";
>   			clocks = <&psc1 1>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   
>   			#address-cells = <1>;
> @@ -682,6 +706,7 @@
>   				#dma-cells = <2>;
>   				#dma-channels = <4>;
>   				clocks = <&psc1 1>;
> +				power-domains = <&pwc1>;

This node should not have clocks or power-domains since it is
a child node. Instead, the parent node should have clock-ranges.
I have already fixed this in my v7 branch.

>   				status = "okay";
>   			};
>   		};
> @@ -690,6 +715,7 @@
>   			reg = <0x218000 0x2000>, <0x22c018 0x4>;
>   			interrupts = <67>;
>   			clocks = <&psc1 8>, <&sata_refclk>;
> +			power-domains = <&pwc1>;
>   			clock-names = "fck", "refclk";
>   			status = "disabled";
>   		};
> @@ -713,6 +739,7 @@
>   			reg = <0x224000 0x1000>;
>   			clocks = <&psc1 5>;
>   			clock-names = "fck";
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   		eth0: ethernet at 220000 {
> @@ -729,6 +756,7 @@
>   					36
>   					>;
>   			clocks = <&psc1 5>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   		usb1: usb at 225000 {
> @@ -738,6 +766,7 @@
>   			phys = <&usb_phy 1>;
>   			phy-names = "usb-phy";
>   			clocks = <&psc1 2>;
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   		gpio: gpio at 226000 {
> @@ -756,6 +785,7 @@
>   			interrupt-controller;
>   			#interrupt-cells = <2>;
>   			clocks = <&psc1 3>;
> +			power-domains = <&pwc1>;
>   			clock-names = "gpio";
>   		};
>   		psc1: clock-controller at 227000 {
> @@ -788,6 +818,7 @@
>   				<&edma0 0 1>;
>   			dma-names = "tx", "rx";
>   			clocks = <&psc1 7>;
> +			power-domains = <&pwc1>;
>   		};
>   
>   		lcdc: display at 213000 {
> @@ -797,6 +828,7 @@
>   			max-pixelclock = <37500>;
>   			clocks = <&psc1 16>;
>   			clock-names = "fck";
> +			power-domains = <&pwc1>;
>   			status = "disabled";
>   		};
>   	};
> @@ -809,6 +841,7 @@
>   		ranges = <0 0 0x60000000 0x08000000
>   			  1 0 0x68000000 0x00008000>;
>   		clocks = <&psc0 3>;
> +		power-domains = <&pwc0>;
>   		status = "disabled";
>   	};
>   	memctrl: memory-controller at b0000000 {
> 

Bigger picture question: what is the benefit of adding
power-domain nodes to all of these devices if they are
already working?

[PATCH 6/7] ARM: davinci: select generic power domains for DaVinci in DT mode

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Automatically select the genpd framework in DT mode.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
index da8a039d65f9..cb5dd239e375 100644
--- a/arch/arm/mach-davinci/Kconfig
+++ b/arch/arm/mach-davinci/Kconfig
@@ -60,6 +60,7 @@ config MACH_DA8XX_DT
 	depends on ARCH_DAVINCI_DA850
 	select PINCTRL
 	select TIMER_OF
+	select PM_GENERIC_DOMAINS if PM
 	help
 	  Say y here to include support for TI DaVinci DA850 based using
 	  Flattened Device Tree. More information at Documentation/devicetree
-- 
2.16.1

[PATCH 7/7] ARM: davinci_all_defconfig: select the DaVinci genpd driver in DT mode

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We can now use the generic power domains driver in DT mode. Reflect
that in the defconfig.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/configs/davinci_all_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
index a5d9fcd9dd9c..3bff3b78b0c3 100644
--- a/arch/arm/configs/davinci_all_defconfig
+++ b/arch/arm/configs/davinci_all_defconfig
@@ -211,6 +211,8 @@ CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_OMAP=m
 CONFIG_DMADEVICES=y
 CONFIG_TI_EDMA=y
+CONFIG_SOC_DAVINCI=y
+CONFIG_DAVINCI_PM_DOMAINS=y
 CONFIG_MEMORY=y
 CONFIG_TI_AEMIF=m
 CONFIG_DA8XX_DDRCTL=y
-- 
2.16.1

[PATCH 0/7] ARM: davinci: add genpd support

[David Lechner]

On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Hi Sekhar et al,
> 
> please take a look at the following patches. They add a simple genpd
> driver and use it in DT mode on da850 boards.
> 
> I was trying to use genpd in legacy mode too, but couldn't find neither
> any interfaces nor users that would do that. For now I added a check in
> arch/arm/mach-davinci/pm_domain.c that disables the clock pm setup if
> we're using genpd.
> 
> This series applies on top of and has been tested with David Lechner's
> for-bartosz branch. It fixes the clock look-up issues we faced with
> lcdc and emac.

I'm starting to think that it makes more sense to just make the PSC driver
a power-domain and reset provider rather than a clock provider. It is
unfortunate that genpd is DT only.

[PATCH 0/7] ARM: davinci: add genpd support

[Sekhar Nori]

Hi David,

On Thursday 08 February 2018 04:13 AM, David Lechner wrote:
> On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Hi Sekhar et al,
>>
>> please take a look at the following patches. They add a simple genpd
>> driver and use it in DT mode on da850 boards.
>>
>> I was trying to use genpd in legacy mode too, but couldn't find neither
>> any interfaces nor users that would do that. For now I added a check in
>> arch/arm/mach-davinci/pm_domain.c that disables the clock pm setup if
>> we're using genpd.
>>
>> This series applies on top of and has been tested with David Lechner's
>> for-bartosz branch. It fixes the clock look-up issues we faced with
>> lcdc and emac.
> 
> I'm starting to think that it makes more sense to just make the PSC driver
> a power-domain and reset provider rather than a clock provider. It is
> unfortunate that genpd is DT only.

This will mean that the only way to enable clocks on DaVinci is to use
pm_runtime() calls. We do still have drivers which depend on clk api for
enabling clocks, so I am not sure its feasible just yet.

I think a question like this can arise for any gate clock.

Thanks,
Sekhar

[PATCH 0/7] ARM: davinci: add genpd support

[David Lechner]

On 02/09/2018 06:42 AM, Sekhar Nori wrote:
> Hi David,
> 
> On Thursday 08 February 2018 04:13 AM, David Lechner wrote:
>> On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> Hi Sekhar et al,
>>>
>>> please take a look at the following patches. They add a simple genpd
>>> driver and use it in DT mode on da850 boards.
>>>
>>> I was trying to use genpd in legacy mode too, but couldn't find neither
>>> any interfaces nor users that would do that. For now I added a check in
>>> arch/arm/mach-davinci/pm_domain.c that disables the clock pm setup if
>>> we're using genpd.
>>>
>>> This series applies on top of and has been tested with David Lechner's
>>> for-bartosz branch. It fixes the clock look-up issues we faced with
>>> lcdc and emac.
>>
>> I'm starting to think that it makes more sense to just make the PSC driver
>> a power-domain and reset provider rather than a clock provider. It is
>> unfortunate that genpd is DT only.
> 
> This will mean that the only way to enable clocks on DaVinci is to use
> pm_runtime() calls. We do still have drivers which depend on clk api for
> enabling clocks, so I am not sure its feasible just yet.
> 
> I think a question like this can arise for any gate clock.
> 

I've incorporated parts of this series into my v7 work in progress
using the suggestions I made about using the existing PSC device
as the power domain provider.

[PATCH 0/7] ARM: davinci: add genpd support

[Bartosz Golaszewski]

2018-02-18 4:41 GMT+01:00 David Lechner <david@lechnology.com>:
> On 02/09/2018 06:42 AM, Sekhar Nori wrote:
>>
>> Hi David,
>>
>> On Thursday 08 February 2018 04:13 AM, David Lechner wrote:
>>>
>>> On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
>>>>
>>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>>
>>>> Hi Sekhar et al,
>>>>
>>>> please take a look at the following patches. They add a simple genpd
>>>> driver and use it in DT mode on da850 boards.
>>>>
>>>> I was trying to use genpd in legacy mode too, but couldn't find neither
>>>> any interfaces nor users that would do that. For now I added a check in
>>>> arch/arm/mach-davinci/pm_domain.c that disables the clock pm setup if
>>>> we're using genpd.
>>>>
>>>> This series applies on top of and has been tested with David Lechner's
>>>> for-bartosz branch. It fixes the clock look-up issues we faced with
>>>> lcdc and emac.
>>>
>>>
>>> I'm starting to think that it makes more sense to just make the PSC
>>> driver
>>> a power-domain and reset provider rather than a clock provider. It is
>>> unfortunate that genpd is DT only.
>>
>>
>> This will mean that the only way to enable clocks on DaVinci is to use
>> pm_runtime() calls. We do still have drivers which depend on clk api for
>> enabling clocks, so I am not sure its feasible just yet.
>>
>> I think a question like this can arise for any gate clock.
>>
>
> I've incorporated parts of this series into my v7 work in progress
> using the suggestions I made about using the existing PSC device
> as the power domain provider.
>

FYI I had added the power domain functionality to the v6 PSC driver
here: https://github.com/brgl/linux/commits/topic/davinci-genpd-final-v2

Thanks,
Bartosz