On Thu, Mar 13, 2014 at 02:06:45PM +0000, Antoine Ténart wrote:
> Adds initial support for the Marvell Armada 1500 pro (BG2Q) SoC (Berlin family).
> The SoC has nodes for cpu, l2 cache controller, interrupt controllers, local
> timer, apb timers and uarts for now.
>
> Signed-off-by: Antoine Ténart <antoine.tenart@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
> ---
> arch/arm/boot/dts/berlin2q.dtsi | 167 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 167 insertions(+)
> create mode 100644 arch/arm/boot/dts/berlin2q.dtsi
>
> diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
> new file mode 100644
> index 000000000000..1cb76031dfe6
> --- /dev/null
> +++ b/arch/arm/boot/dts/berlin2q.dtsi
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (C) 2014 Antoine Ténart <antoine.tenart@xxxxxxxxxxxxxxxxxx>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +#include "skeleton.dtsi"
> +
> +/ {
> + model = "Marvell Armada 1500 pro (BG2-Q) SoC";
> + compatible = "marvell,berlin2q", "marvell,berlin";
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu@0 {
> + compatible = "arm,cortex-a9";
> + device_type = "cpu";
> + next-level-cache = <&l2>;
> + reg = <0>;
> + };
> +
> + cpu@1 {
> + compatible = "arm,cortex-a9";
> + device_type = "cpu";
> + next-level-cache = <&l2>;
> + reg = <1>;
> + };
> +
> + cpu@2 {
> + compatible = "arm,cortex-a9";
> + device_type = "cpu";
> + next-level-cache = <&l2>;
> + reg = <2>;
> + };
> +
> + cpu@3 {
> + compatible = "arm,cortex-a9";
> + device_type = "cpu";
> + next-level-cache = <&l2>;
> + reg = <3>;
> + };
> + };
> +
> + clocks {
> + #address-cells = <0>;
> + #size-cells = <0>;
> +
> + smclk: sysmgr-clock {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <25000000>;
> + };
> +
> + sysclk: system-clock {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <400000000>;
> + };
> + };
I am very much not keen on having container nodes like this.
A "clocks" container is non-standard, and not guarantee to probe. The
fact these clocks probe currently is an artifact of the current
organisation of Linux rather than any conscious decision.
The #address-cells and #size-cells properties are useless, as no
address space is defined. The node is not a simple-bus, has no ranges,
and doesn't have a compatible string that implies an address space.
Is there any reason you need to use a clocks container here and can't
put these directly under the root?
> +
> + soc {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + ranges = <0 0xf7000000 0x1000000>;
> + interrupt-parent = <&gic>;
> +
> + l2: l2-cache-controller@ac0000 {
> + compatible = "arm,pl310-cache";
> + reg = <0xac0000 0x1000>;
> + cache-level = <2>;
> + };
> +
> + local-timer@ad0600 {
> + compatible = "arm,cortex-a9-twd-timer";
> + reg = <0xad0600 0x20>;
> + clocks = <&sysclk>;
> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
> + status = "okay";
Given that nodes are assumed by default to be okay, this can be dropped.
I think it only makes sense to have a status property in a common dtsi
where the value is "disabled".
Otherwise, this dtsi looks sane to me.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
