| Message ID | 1424891085-10392-35-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tero, * Tero Kristo <t-kristo@ti.com> [150225 11:09]: > Add node for system control module, and move all the existing system > control IO space users under this new node as its children. A new node > for scm_conf area is also added. ... > --- a/arch/arm/boot/dts/dra7.dtsi > +++ b/arch/arm/boot/dts/dra7.dtsi > @@ -203,26 +203,47 @@ > }; > }; > > + scm: scm@4a002000 { > + compatible = "ti,dra7-ctrl", "simple-bus"; > + reg = <0x4a002000 0x1400>, > + <0x4a003400 0x600>, > + <0x4ae0c000 0x600>; > + #address-cells = <2>; > + #size-cells = <1>; > + ranges = <0 0 0x4a002000 0x1400>, > + <1 0 0x4a003400 0x600>, > + <2 0 0x4ae0c000 0x600>; > + > + scm_conf: tisyscon@0,0 { > + compatible = "syscon"; > + reg = <0 0x0 0x1400>; > + #address-cells = <1>; > + #size-cells = <1>; > + }; > + > + dra7_pmx_core: pinmux@1,0 { > + compatible = "ti,dra7-padconf", > + "pinctrl-single"; > + reg = <1 0x0 0x0464>; > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <1>; > + interrupt-controller; > + pinctrl-single,register-width = <32>; > + pinctrl-single,function-mask = <0x3fffffff>; > + }; > + }; Wouldn't it make more sense to have separate device_scm, core_scm and wkup_scm instead of stuffing multiple ranges here? Or are there other reasons for the multiple ranges? Regards, Tony
On 03/11/2015 07:17 PM, Tony Lindgren wrote: > Hi Tero, > > * Tero Kristo <t-kristo@ti.com> [150225 11:09]: >> Add node for system control module, and move all the existing system >> control IO space users under this new node as its children. A new node >> for scm_conf area is also added. > ... > >> --- a/arch/arm/boot/dts/dra7.dtsi >> +++ b/arch/arm/boot/dts/dra7.dtsi >> @@ -203,26 +203,47 @@ >> }; >> }; >> >> + scm: scm@4a002000 { >> + compatible = "ti,dra7-ctrl", "simple-bus"; >> + reg = <0x4a002000 0x1400>, >> + <0x4a003400 0x600>, >> + <0x4ae0c000 0x600>; >> + #address-cells = <2>; >> + #size-cells = <1>; >> + ranges = <0 0 0x4a002000 0x1400>, >> + <1 0 0x4a003400 0x600>, >> + <2 0 0x4ae0c000 0x600>; >> + >> + scm_conf: tisyscon@0,0 { >> + compatible = "syscon"; >> + reg = <0 0x0 0x1400>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + }; >> + >> + dra7_pmx_core: pinmux@1,0 { >> + compatible = "ti,dra7-padconf", >> + "pinctrl-single"; >> + reg = <1 0x0 0x0464>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + #interrupt-cells = <1>; >> + interrupt-controller; >> + pinctrl-single,register-width = <32>; >> + pinctrl-single,function-mask = <0x3fffffff>; >> + }; >> + }; > > Wouldn't it make more sense to have separate device_scm, core_scm and > wkup_scm instead of stuffing multiple ranges here? > > Or are there other reasons for the multiple ranges? Yea that was the alternative I was thinking about, I ended up with this for some reason. I think personally I liked having them all under the same SCM part, because they are nicely grouped then, and well, its the same system control part in the chip. We can split it up easily of course. Should we have a higher level scm part and then have core_scm and wkup_scm under this followed by the sub-functions, or just drop the top level scm part completely? This same question applies to omap4 + omap5 also. In some part for omap3 also, as it also has pmx_core + pmx_wkup separately, even if they are part of the same register space. Anyway, just a political decision from your side, I am fine either way. :) -Tero
* Tero Kristo <t-kristo@ti.com> [150311 12:09]: > On 03/11/2015 07:17 PM, Tony Lindgren wrote: > >Hi Tero, > > > >* Tero Kristo <t-kristo@ti.com> [150225 11:09]: > >>Add node for system control module, and move all the existing system > >>control IO space users under this new node as its children. A new node > >>for scm_conf area is also added. > >... > > > >>--- a/arch/arm/boot/dts/dra7.dtsi > >>+++ b/arch/arm/boot/dts/dra7.dtsi > >>@@ -203,26 +203,47 @@ > >> }; > >> }; > >> > >>+ scm: scm@4a002000 { > >>+ compatible = "ti,dra7-ctrl", "simple-bus"; > >>+ reg = <0x4a002000 0x1400>, > >>+ <0x4a003400 0x600>, > >>+ <0x4ae0c000 0x600>; > >>+ #address-cells = <2>; > >>+ #size-cells = <1>; > >>+ ranges = <0 0 0x4a002000 0x1400>, > >>+ <1 0 0x4a003400 0x600>, > >>+ <2 0 0x4ae0c000 0x600>; > >>+ > >>+ scm_conf: tisyscon@0,0 { > >>+ compatible = "syscon"; > >>+ reg = <0 0x0 0x1400>; > >>+ #address-cells = <1>; > >>+ #size-cells = <1>; > >>+ }; > >>+ > >>+ dra7_pmx_core: pinmux@1,0 { > >>+ compatible = "ti,dra7-padconf", > >>+ "pinctrl-single"; > >>+ reg = <1 0x0 0x0464>; > >>+ #address-cells = <1>; > >>+ #size-cells = <0>; > >>+ #interrupt-cells = <1>; > >>+ interrupt-controller; > >>+ pinctrl-single,register-width = <32>; > >>+ pinctrl-single,function-mask = <0x3fffffff>; > >>+ }; > >>+ }; > > > >Wouldn't it make more sense to have separate device_scm, core_scm and > >wkup_scm instead of stuffing multiple ranges here? > > > >Or are there other reasons for the multiple ranges? > > Yea that was the alternative I was thinking about, I ended up with this for > some reason. I think personally I liked having them all under the same SCM > part, because they are nicely grouped then, and well, its the same system > control part in the chip. We can split it up easily of course. Should we > have a higher level scm part and then have core_scm and wkup_scm under this > followed by the sub-functions, or just drop the top level scm part > completely? Well I'd model it after the hardware so we can have one or more scm driver instances managing the clock for those blocks. If we squash them together, we won't have a chance to pass interrupts and clocks device tree property to the right driver instance. And for example 5432 TRM has them as separate devices in "Figure 18-1. Control Module Overview". I don't think we need the top level scm to group them under, these are all connected seprately to the interconnect, right? > This same question applies to omap4 + omap5 also. In some part for omap3 > also, as it also has pmx_core + pmx_wkup separately, even if they are part > of the same register space. > > Anyway, just a political decision from your side, I am fine either way. :) OK thanks for confirming that, to me it makes sense to set them up as separate instances then. Regards, Tony
On 03/11/2015 09:26 PM, Tony Lindgren wrote: > * Tero Kristo <t-kristo@ti.com> [150311 12:09]: >> On 03/11/2015 07:17 PM, Tony Lindgren wrote: >>> Hi Tero, >>> >>> * Tero Kristo <t-kristo@ti.com> [150225 11:09]: >>>> Add node for system control module, and move all the existing system >>>> control IO space users under this new node as its children. A new node >>>> for scm_conf area is also added. >>> ... >>> >>>> --- a/arch/arm/boot/dts/dra7.dtsi >>>> +++ b/arch/arm/boot/dts/dra7.dtsi >>>> @@ -203,26 +203,47 @@ >>>> }; >>>> }; >>>> >>>> + scm: scm@4a002000 { >>>> + compatible = "ti,dra7-ctrl", "simple-bus"; >>>> + reg = <0x4a002000 0x1400>, >>>> + <0x4a003400 0x600>, >>>> + <0x4ae0c000 0x600>; >>>> + #address-cells = <2>; >>>> + #size-cells = <1>; >>>> + ranges = <0 0 0x4a002000 0x1400>, >>>> + <1 0 0x4a003400 0x600>, >>>> + <2 0 0x4ae0c000 0x600>; >>>> + >>>> + scm_conf: tisyscon@0,0 { >>>> + compatible = "syscon"; >>>> + reg = <0 0x0 0x1400>; >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + }; >>>> + >>>> + dra7_pmx_core: pinmux@1,0 { >>>> + compatible = "ti,dra7-padconf", >>>> + "pinctrl-single"; >>>> + reg = <1 0x0 0x0464>; >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + #interrupt-cells = <1>; >>>> + interrupt-controller; >>>> + pinctrl-single,register-width = <32>; >>>> + pinctrl-single,function-mask = <0x3fffffff>; >>>> + }; >>>> + }; >>> >>> Wouldn't it make more sense to have separate device_scm, core_scm and >>> wkup_scm instead of stuffing multiple ranges here? >>> >>> Or are there other reasons for the multiple ranges? >> >> Yea that was the alternative I was thinking about, I ended up with this for >> some reason. I think personally I liked having them all under the same SCM >> part, because they are nicely grouped then, and well, its the same system >> control part in the chip. We can split it up easily of course. Should we >> have a higher level scm part and then have core_scm and wkup_scm under this >> followed by the sub-functions, or just drop the top level scm part >> completely? > > Well I'd model it after the hardware so we can have one or more scm driver > instances managing the clock for those blocks. If we squash them together, > we won't have a chance to pass interrupts and clocks device tree property > to the right driver instance. And for example 5432 TRM has them as separate > devices in "Figure 18-1. Control Module Overview". > > I don't think we need the top level scm to group them under, these are all > connected seprately to the interconnect, right? Yea, can't really think of any real need for the top-level node. > >> This same question applies to omap4 + omap5 also. In some part for omap3 >> also, as it also has pmx_core + pmx_wkup separately, even if they are part >> of the same register space. >> >> Anyway, just a political decision from your side, I am fine either way. :) > > OK thanks for confirming that, to me it makes sense to set them up as > separate instances then. All right, you got fair points there, I'll rework this for next revision of the set. Had a quick look at OMAP3 TRM and it is also basically listing these as separate instances also, so I'll change all OMAP3+. -Tero
* Tero Kristo <t-kristo@ti.com> [150311 12:57]: > On 03/11/2015 09:26 PM, Tony Lindgren wrote: > >* Tero Kristo <t-kristo@ti.com> [150311 12:09]: > >>On 03/11/2015 07:17 PM, Tony Lindgren wrote: > >>>Hi Tero, > >>> > >>>* Tero Kristo <t-kristo@ti.com> [150225 11:09]: > >>>>Add node for system control module, and move all the existing system > >>>>control IO space users under this new node as its children. A new node > >>>>for scm_conf area is also added. > >>>... > >>> > >>>>--- a/arch/arm/boot/dts/dra7.dtsi > >>>>+++ b/arch/arm/boot/dts/dra7.dtsi > >>>>@@ -203,26 +203,47 @@ > >>>> }; > >>>> }; > >>>> > >>>>+ scm: scm@4a002000 { > >>>>+ compatible = "ti,dra7-ctrl", "simple-bus"; > >>>>+ reg = <0x4a002000 0x1400>, > >>>>+ <0x4a003400 0x600>, > >>>>+ <0x4ae0c000 0x600>; > >>>>+ #address-cells = <2>; > >>>>+ #size-cells = <1>; > >>>>+ ranges = <0 0 0x4a002000 0x1400>, > >>>>+ <1 0 0x4a003400 0x600>, > >>>>+ <2 0 0x4ae0c000 0x600>; > >>>>+ > >>>>+ scm_conf: tisyscon@0,0 { > >>>>+ compatible = "syscon"; > >>>>+ reg = <0 0x0 0x1400>; > >>>>+ #address-cells = <1>; > >>>>+ #size-cells = <1>; > >>>>+ }; > >>>>+ > >>>>+ dra7_pmx_core: pinmux@1,0 { > >>>>+ compatible = "ti,dra7-padconf", > >>>>+ "pinctrl-single"; > >>>>+ reg = <1 0x0 0x0464>; > >>>>+ #address-cells = <1>; > >>>>+ #size-cells = <0>; > >>>>+ #interrupt-cells = <1>; > >>>>+ interrupt-controller; > >>>>+ pinctrl-single,register-width = <32>; > >>>>+ pinctrl-single,function-mask = <0x3fffffff>; > >>>>+ }; > >>>>+ }; > >>> > >>>Wouldn't it make more sense to have separate device_scm, core_scm and > >>>wkup_scm instead of stuffing multiple ranges here? > >>> > >>>Or are there other reasons for the multiple ranges? > >> > >>Yea that was the alternative I was thinking about, I ended up with this for > >>some reason. I think personally I liked having them all under the same SCM > >>part, because they are nicely grouped then, and well, its the same system > >>control part in the chip. We can split it up easily of course. Should we > >>have a higher level scm part and then have core_scm and wkup_scm under this > >>followed by the sub-functions, or just drop the top level scm part > >>completely? > > > >Well I'd model it after the hardware so we can have one or more scm driver > >instances managing the clock for those blocks. If we squash them together, > >we won't have a chance to pass interrupts and clocks device tree property > >to the right driver instance. And for example 5432 TRM has them as separate > >devices in "Figure 18-1. Control Module Overview". > > > >I don't think we need the top level scm to group them under, these are all > >connected seprately to the interconnect, right? > > Yea, can't really think of any real need for the top-level node. > > > > >>This same question applies to omap4 + omap5 also. In some part for omap3 > >>also, as it also has pmx_core + pmx_wkup separately, even if they are part > >>of the same register space. > >> > >>Anyway, just a political decision from your side, I am fine either way. :) > > > >OK thanks for confirming that, to me it makes sense to set them up as > >separate instances then. > > All right, you got fair points there, I'll rework this for next revision of > the set. Had a quick look at OMAP3 TRM and it is also basically listing > these as separate instances also, so I'll change all OMAP3+. Oh OK. Tony
diff --git a/Documentation/devicetree/bindings/arm/omap/ctrl.txt b/Documentation/devicetree/bindings/arm/omap/ctrl.txt index 3574d67..f84f4ea 100644 --- a/Documentation/devicetree/bindings/arm/omap/ctrl.txt +++ b/Documentation/devicetree/bindings/arm/omap/ctrl.txt @@ -21,6 +21,7 @@ Required properties: "ti,omap3-scrm" "ti,omap4-ctrl" "ti,omap5-ctrl" + "ti,dra7-ctrl" - reg: Contains Control Module register address range (base address and length) diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi index 5827fed..230d22d 100644 --- a/arch/arm/boot/dts/dra7.dtsi +++ b/arch/arm/boot/dts/dra7.dtsi @@ -203,26 +203,47 @@ }; }; + scm: scm@4a002000 { + compatible = "ti,dra7-ctrl", "simple-bus"; + reg = <0x4a002000 0x1400>, + <0x4a003400 0x600>, + <0x4ae0c000 0x600>; + #address-cells = <2>; + #size-cells = <1>; + ranges = <0 0 0x4a002000 0x1400>, + <1 0 0x4a003400 0x600>, + <2 0 0x4ae0c000 0x600>; + + scm_conf: tisyscon@0,0 { + compatible = "syscon"; + reg = <0 0x0 0x1400>; + #address-cells = <1>; + #size-cells = <1>; + }; + + dra7_pmx_core: pinmux@1,0 { + compatible = "ti,dra7-padconf", + "pinctrl-single"; + reg = <1 0x0 0x0464>; + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <1>; + interrupt-controller; + pinctrl-single,register-width = <32>; + pinctrl-single,function-mask = <0x3fffffff>; + }; + }; + counter32k: counter@4ae04000 { compatible = "ti,omap-counter32k"; reg = <0x4ae04000 0x40>; ti,hwmods = "counter_32k"; }; - dra7_ctrl_core: ctrl_core@4a002000 { - compatible = "syscon"; - reg = <0x4a002000 0x6d0>; - }; - - dra7_ctrl_general: tisyscon@4a002e00 { - compatible = "syscon"; - reg = <0x4a002e00 0x7c>; - }; - pbias_regulator: pbias_regulator { compatible = "ti,pbias-omap"; - reg = <0 0x4>; - syscon = <&dra7_ctrl_general>; + reg = <0xe00 0x4>; + syscon = <&scm_conf>; pbias_mmc_reg: pbias_mmc_omap5 { regulator-name = "pbias_mmc_omap5"; regulator-min-microvolt = <1800000>; @@ -230,17 +251,6 @@ }; }; - dra7_pmx_core: pinmux@4a003400 { - compatible = "ti,dra7-padconf", "pinctrl-single"; - reg = <0x4a003400 0x0464>; - #address-cells = <1>; - #size-cells = <0>; - #interrupt-cells = <1>; - interrupt-controller; - pinctrl-single,register-width = <32>; - pinctrl-single,function-mask = <0x3fffffff>; - }; - sdma: dma-controller@4a056000 { compatible = "ti,omap4430-sdma"; reg = <0x4a056000 0x1000>; @@ -1410,7 +1420,7 @@ compatible = "ti,dra7-d_can"; ti,hwmods = "dcan1"; reg = <0x4ae3c000 0x2000>; - syscon-raminit = <&dra7_ctrl_core 0x558 0>; + syscon-raminit = <&scm_conf 0x558 0>; interrupts = <GIC_SPI 222 IRQ_TYPE_LEVEL_HIGH>; clocks = <&dcan1_sys_clk_mux>; status = "disabled"; @@ -1420,7 +1430,7 @@ compatible = "ti,dra7-d_can"; ti,hwmods = "dcan2"; reg = <0x48480000 0x2000>; - syscon-raminit = <&dra7_ctrl_core 0x558 1>; + syscon-raminit = <&scm_conf 0x558 1>; interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>; clocks = <&sys_clkin1>; status = "disabled";
Add node for system control module, and move all the existing system control IO space users under this new node as its children. A new node for scm_conf area is also added. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- .../devicetree/bindings/arm/omap/ctrl.txt | 1 + arch/arm/boot/dts/dra7.dtsi | 60 ++++++++++++-------- 2 files changed, 36 insertions(+), 25 deletions(-)