Messages in this thread | | | Subject | Re: [PATCH 5/7] ARM: dts: da850: add power-domains properties to device nodes | From | David Lechner <> | Date | Wed, 7 Feb 2018 16:21:58 -0600 |
| |
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@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?
| |