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

From: David Lechner
Date: Wed Feb 07 2018 - 17:21:59 EST


On 02/07/2018 07:45 AM, Bartosz Golaszewski wrote:
From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>

Make all devices managed by PSCs use the genpd driver.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
---
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?