From: Bartosz Golaszewski <[email protected]>
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
From: Bartosz Golaszewski <[email protected]>
We can now use the generic power domains driver in DT mode. Reflect
that in the defconfig.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
Automatically select the genpd framework in DT mode.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
Make all devices managed by PSCs use the genpd driver.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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@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@8000 {
compatible = "ti,edma3-tptc";
@@ -446,6 +451,7 @@
interrupts = <13>;
interrupt-names = "edm3_tcerrint";
clocks = <&psc0 1>;
+ power-domains = <&pwc0>;
};
edma0_tptc1: tptc@8400 {
compatible = "ti,edma3-tptc";
@@ -453,6 +459,7 @@
interrupts = <32>;
interrupt-names = "edm3_tcerrint";
clocks = <&psc0 2>;
+ power-domains = <&pwc0>;
};
edma1: edma@230000 {
compatible = "ti,edma3-tpcc";
@@ -465,6 +472,7 @@
ti,tptcs = <&edma1_tptc0 7>;
clocks = <&psc1 0>;
+ power-domains = <&pwc1>;
};
edma1_tptc0: tptc@238000 {
compatible = "ti,edma3-tptc";
@@ -472,6 +480,7 @@
interrupts = <95>;
interrupt-names = "edm3_tcerrint";
clocks = <&psc1 21>;
+ power-domains = <&pwc1>;
};
serial0: serial@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@10c000 {
@@ -489,6 +499,7 @@
reg-shift = <2>;
interrupts = <53>;
clocks = <&psc1 12>;
+ power-domains = <&pwc1>;
status = "disabled";
};
serial2: serial@10d000 {
@@ -498,6 +509,7 @@
reg-shift = <2>;
interrupts = <61>;
clocks = <&psc1 13>;
+ power-domains = <&pwc1>;
status = "disabled";
};
rtc0: rtc@23000 {
@@ -523,6 +535,7 @@
#address-cells = <1>;
#size-cells = <0>;
clocks = <&psc1 11>;
+ power-domains = <&pwc1>;
status = "disabled";
};
clocksource: timer@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@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@300000 {
@@ -585,6 +601,7 @@
clocks = <&psc1 17>, <&ehrpwm_tbclk>;
clock-names = "fck", "tbclk";
status = "disabled";
+ power-domains = <&pwc1>;
};
ehrpwm1: pwm@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@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@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@220000 {
@@ -729,6 +756,7 @@
36
>;
clocks = <&psc1 5>;
+ power-domains = <&pwc1>;
status = "disabled";
};
usb1: usb@225000 {
@@ -738,6 +766,7 @@
phys = <&usb_phy 1>;
phy-names = "usb-phy";
clocks = <&psc1 2>;
+ power-domains = <&pwc1>;
status = "disabled";
};
gpio: gpio@226000 {
@@ -756,6 +785,7 @@
interrupt-controller;
#interrupt-cells = <2>;
clocks = <&psc1 3>;
+ power-domains = <&pwc1>;
clock-names = "gpio";
};
psc1: clock-controller@227000 {
@@ -788,6 +818,7 @@
<&edma0 0 1>;
dma-names = "tx", "rx";
clocks = <&psc1 7>;
+ power-domains = <&pwc1>;
};
lcdc: display@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@b0000000 {
--
2.16.1
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
Add two power controller nodes corresponding with the two PSC modules
present on the da850 SoC.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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@10000 {
+ compatible = "ti,davinci-pm-domains";
+ #power-domain-cells = <0>;
+ };
pll0: clock-controller@11000 {
compatible = "ti,da850-pll0";
reg = <0x11000 0x1000>;
@@ -757,6 +766,10 @@
<&async3_clk>;
clock-names = "pll0_sysclk2", "pll0_sysclk4", "async3";
};
+ pwc1: power-controller@227000 {
+ compatible = "ti,davinci-pm-domains";
+ #power-domain-cells = <0>;
+ };
pinconf: pin-controller@22c00c {
compatible = "ti,da850-pupd";
reg = <0x22c00c 0x8>;
--
2.16.1
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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 <[email protected]>");
--
2.16.1
From: Bartosz Golaszewski <[email protected]>
Add a simple document for the DaVinci genpd driver. We use clock pm
exclusively hence no reg property.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
.../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@227000 {
+ compatible = "ti,davinci-pm-domains";
+ #power-domain-cells = <0>;
+};
--
2.16.1
On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add a simple document for the DaVinci genpd driver. We use clock pm
> exclusively hence no reg property.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> .../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@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?
On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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 <[email protected]>");
>
Likewise, I don't think we need a separate driver for this. Just
register a genpd provider from the PSC clock driver.
On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add two power controller nodes corresponding with the two PSC modules
> present on the da850 SoC.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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@10000 {
> + compatible = "ti,davinci-pm-domains";
> + #power-domain-cells = <0>;
> + };
> pll0: clock-controller@11000 {
> compatible = "ti,da850-pll0";
> reg = <0x11000 0x1000>;
> @@ -757,6 +766,10 @@
> <&async3_clk>;
> clock-names = "pll0_sysclk2", "pll0_sysclk4", "async3";
> };
> + pwc1: power-controller@227000 {
> + compatible = "ti,davinci-pm-domains";
> + #power-domain-cells = <0>;
> + };
> pinconf: pin-controller@22c00c {
> compatible = "ti,da850-pupd";
> reg = <0x22c00c 0x8>;
>
On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Make all devices managed by PSCs use the genpd driver.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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@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@8000 {
> compatible = "ti,edma3-tptc";
> @@ -446,6 +451,7 @@
> interrupts = <13>;
> interrupt-names = "edm3_tcerrint";
> clocks = <&psc0 1>;
> + power-domains = <&pwc0>;
> };
> edma0_tptc1: tptc@8400 {
> compatible = "ti,edma3-tptc";
> @@ -453,6 +459,7 @@
> interrupts = <32>;
> interrupt-names = "edm3_tcerrint";
> clocks = <&psc0 2>;
> + power-domains = <&pwc0>;
> };
> edma1: edma@230000 {
> compatible = "ti,edma3-tpcc";
> @@ -465,6 +472,7 @@
>
> ti,tptcs = <&edma1_tptc0 7>;
> clocks = <&psc1 0>;
> + power-domains = <&pwc1>;
> };
> edma1_tptc0: tptc@238000 {
> compatible = "ti,edma3-tptc";
> @@ -472,6 +480,7 @@
> interrupts = <95>;
> interrupt-names = "edm3_tcerrint";
> clocks = <&psc1 21>;
> + power-domains = <&pwc1>;
> };
> serial0: serial@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@10c000 {
> @@ -489,6 +499,7 @@
> reg-shift = <2>;
> interrupts = <53>;
> clocks = <&psc1 12>;
> + power-domains = <&pwc1>;
> status = "disabled";
> };
> serial2: serial@10d000 {
> @@ -498,6 +509,7 @@
> reg-shift = <2>;
> interrupts = <61>;
> clocks = <&psc1 13>;
> + power-domains = <&pwc1>;
> status = "disabled";
> };
> rtc0: rtc@23000 {
> @@ -523,6 +535,7 @@
> #address-cells = <1>;
> #size-cells = <0>;
> clocks = <&psc1 11>;
> + power-domains = <&pwc1>;
> status = "disabled";
> };
> clocksource: timer@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@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@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@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@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@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@220000 {
> @@ -729,6 +756,7 @@
> 36
> >;
> clocks = <&psc1 5>;
> + power-domains = <&pwc1>;
> status = "disabled";
> };
> usb1: usb@225000 {
> @@ -738,6 +766,7 @@
> phys = <&usb_phy 1>;
> phy-names = "usb-phy";
> clocks = <&psc1 2>;
> + power-domains = <&pwc1>;
> status = "disabled";
> };
> gpio: gpio@226000 {
> @@ -756,6 +785,7 @@
> interrupt-controller;
> #interrupt-cells = <2>;
> clocks = <&psc1 3>;
> + power-domains = <&pwc1>;
> clock-names = "gpio";
> };
> psc1: clock-controller@227000 {
> @@ -788,6 +818,7 @@
> <&edma0 0 1>;
> dma-names = "tx", "rx";
> clocks = <&psc1 7>;
> + power-domains = <&pwc1>;
> };
>
> lcdc: display@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@b0000000 {
>
Bigger picture question: what is the benefit of adding
power-domain nodes to all of these devices if they are
already working?
On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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.
2018-02-07 22:47 GMT+01:00 David Lechner <[email protected]>:
> On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
>>
>> From: Bartosz Golaszewski <[email protected]>
>>
>> Add a simple document for the DaVinci genpd driver. We use clock pm
>> exclusively hence no reg property.
>>
>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>> ---
>> .../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@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
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
2018-02-08 10:30 GMT+01:00 Sekhar Nori <[email protected]>:
> 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
On Thursday 08 February 2018 03:24 PM, Bartosz Golaszewski wrote:
> 2018-02-08 10:30 GMT+01:00 Sekhar Nori <[email protected]>:
>> 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
2018-02-08 13:56 GMT+01:00 Sekhar Nori <[email protected]>:
> On Thursday 08 February 2018 03:24 PM, Bartosz Golaszewski wrote:
>> 2018-02-08 10:30 GMT+01:00 Sekhar Nori <[email protected]>:
>>> 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
On Thursday 08 February 2018 06:57 PM, Bartosz Golaszewski wrote:
> 2018-02-08 13:56 GMT+01:00 Sekhar Nori <[email protected]>:
>> On Thursday 08 February 2018 03:24 PM, Bartosz Golaszewski wrote:
>>> 2018-02-08 10:30 GMT+01:00 Sekhar Nori <[email protected]>:
>>>> 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
Bartosz Golaszewski <[email protected]> writes:
> 2018-02-07 22:47 GMT+01:00 David Lechner <[email protected]>:
>> On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
>>>
>>> From: Bartosz Golaszewski <[email protected]>
>>>
>>> Add a simple document for the DaVinci genpd driver. We use clock pm
>>> exclusively hence no reg property.
>>>
>>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>>> ---
>>> .../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@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
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 <[email protected]>
>>
>> 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
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 <[email protected]>
>>>
>>> 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.
2018-02-18 4:41 GMT+01:00 David Lechner <[email protected]>:
> 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 <[email protected]>
>>>>
>>>> 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