lkml.org 
[lkml]   [2018]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 5/7] ARM: dts: da850: add power-domains properties to device nodes
    From
    Date
    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?

    \
     
     \ /
      Last update: 2018-02-07 23:22    [W:4.097 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site