Re: [PATCHv3 34/35] ARM: dts: dra7: add system control module node

[Tero Kristo <t-kristo@ti.com>]

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