From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sebastian Hesselbarth Subject: Re: [PATCH] ARM: mvebu: Add Netgear ReadyNAS 2120 board Date: Sun, 10 Nov 2013 00:57:17 +0100 Message-ID: <527ECBDD.7050607@gmail.com> References: <87bo1tmibv.fsf@natisbad.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <87bo1tmibv.fsf-LkuqDEemtHBg9hUCZPvPmw@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Arnaud Ebalard , Jason Cooper , Andrew Lunn , Gregory Clement , Ezequiel Garcia , Thomas Petazzoni Cc: Russell King , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Rob Herring , Pawel Moll , Mark Rutland , Stephen Warren , Ian Campbell , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Guenter Roeck List-Id: devicetree@vger.kernel.org On 11/09/2013 09:27 PM, Arnaud Ebalard wrote: > All hardware parts of the (mv78230 Armada XP based) NETGEAR ReadyNAS > 2120 are supported by mainline kernel (USB 3.0 and eSATA rear ports, > USB 2.0 front port, Gigabit controller and PHYs for the two rear ports, > serial port, LEDs, Buttons, 88SE9170 SATA controllers, three G762 fan > controllers, G751 temperature sensor) except for: > > - the Intersil ISL12057 I2C RTC Chip, > - the Armada NAND controller. > > Support for both of those is currently work in progress and does not > prevent boot. > > Signed-off-by: Arnaud Ebalard > --- [...] > arch/arm/boot/dts/Makefile | 1 + > arch/arm/boot/dts/armada-xp-netgear-rn2120.dts | 289 +++++++++++++++++++++++++ Arnaud, thanks for providing this! I do have some comments below. we recently had a discussion about the naming of new DTS files, which proposes to name those after vendor,board.dts (or vendor-board.dts). I personally prefer vendor,board.dts which would give netgear,readynas-2120.dts based on your compatible below. The final call for ',' vs '-' has not been made, so I suggest Jason makes a call here. [...] > diff --git a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts > new file mode 100644 > index 0000000..2651ba3 > --- /dev/null > +++ b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts > @@ -0,0 +1,289 @@ > +/* > + * Device Tree file for NETGEAR ReadyNAS 2120 > + * > + * Copyright (C) 2013, Arnaud EBALARD > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + */ > + > +/dts-v1/; > + > +#include "armada-xp-mv78230.dtsi" > + > +/ { > + model = "NETGEAR ReadyNAS 2120"; > + compatible = "netgear,readynas-2120", "marvell,armadaxp-mv78230", "marvell,armadaxp", "marvell,armada-370-xp"; > + > + chosen { > + bootargs = "console=ttyS0,115200 earlyprintk"; > + }; > + > + memory { > + device_type = "memory"; > + reg = <0 0x00000000 0 0x80000000>; /* 2GB */ > + }; > + > + soc { > + ranges = + MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>; > + [...] > + internal-regs { > + pinctrl { > + poweroff: poweroff { > + marvell,pins = "mpp42"; > + marvell,function = "gpio"; > + }; > + > + power_key_pin: power-key-pin { > + marvell,pins = "mpp27"; > + marvell,function = "gpio"; > + }; > + > + reset_key_pin: reset-key-pin { > + marvell,pins = "mpp41"; > + marvell,function = "gpio"; > + }; > + > + sata1_led_pin: sata1-led-pin { > + marvell,pins = "mpp31"; > + marvell,function = "gpio"; > + }; > + > + sata2_led_pin: sata2-led-pin { > + marvell,pins = "mpp40"; > + marvell,function = "gpio"; > + }; > + > + sata3_led_pin: sata3-led-pin { > + marvell,pins = "mpp44"; > + marvell,function = "gpio"; > + }; > + > + sata4_led_pin: sata4-led-pin { > + marvell,pins = "mpp47"; > + marvell,function = "gpio"; > + }; > + > + sata1_power_pin: sata1-power-pin { > + marvell,pins = "mpp24"; > + marvell,function = "gpio"; > + }; > + > + sata2_power_pin: sata2-power-pin { > + marvell,pins = "mpp25"; > + marvell,function = "gpio"; > + }; > + > + sata3_power_pin: sata3-power-pin { > + marvell,pins = "mpp26"; > + marvell,function = "gpio"; > + }; > + > + sata4_power_pin: sata4-power-pin { > + marvell,pins = "mpp28"; > + marvell,function = "gpio"; > + }; > + > + sata1_pres_pin: sata1-pres-pin { > + marvell,pins = "mpp32"; > + marvell,function = "gpio"; > + }; > + > + sata2_pres_pin: sata2-pres-pin { > + marvell,pins = "mpp33"; > + marvell,function = "gpio"; > + }; > + > + sata3_pres_pin: sata3-pres-pin { > + marvell,pins = "mpp34"; > + marvell,function = "gpio"; > + }; > + > + sata4_pres_pin: sata4-pres-pin { > + marvell,pins = "mpp35"; > + marvell,function = "gpio"; > + }; > + > + err_led_pin: err-led-pin { > + marvell,pins = "mpp45"; > + marvell,function = "gpio"; > + }; > + }; > + > + serial@12000 { > + clock-frequency = <250000000>; Where does that 250MHz come from? If it is an internal clock and we already have a DT clock provider for it, please use clocks = <&provider num>; If it is TCLK it can be optained from <&coreclk 0>. > + status = "okay"; > + }; > + > + mdio { > + phy0: ethernet-phy@0 { > + reg = <0>; Can you evaluate PHYs compatible from u-boot or post vendor:prodid from MDIO registers? > + }; > + > + phy1: ethernet-phy@1 { > + reg = <1>; > + }; > + }; > + > + ethernet@70000 { > + status = "okay"; > + phy = <&phy0>; > + phy-mode = "rgmii-id"; > + }; > + > + ethernet@74000 { > + status = "okay"; > + phy = <&phy1>; > + phy-mode = "rgmii-id"; > + }; > + > + /* Front USB 2.0 port */ > + usb@50000 { > + status = "okay"; > + }; > + > + i2c@11000 { > + compatible = "marvell,mv64xxx-i2c"; > + clock-frequency = <400000>; > + status = "okay"; > + > + /* Rear fan #1 of 3 (Protechnic MGT4012XB-O20, > + * 8000RPM) near eSATA port */ > + g762_fan1: g762@3e { > + compatible = "gmt,g762"; > + reg = <0x3e>; > + clocks = <&g762_clk>; /* input clock */ > + fan_gear_mode = <0>; > + fan_startv = <1>; > + pwm_polarity = <0>; > + }; > + > + /* Rear fan #2 of 3 at the center */ > + g762_fan2: g762@48 { > + compatible = "gmt,g762"; > + reg = <0x48>; > + clocks = <&g762_clk>; /* input clock */ > + fan_gear_mode = <0>; > + fan_startv = <1>; > + pwm_polarity = <0>; > + }; > + > + /* Rear fan #3 of 3 */ > + g762_fan3: g762@49 { > + compatible = "gmt,g762"; > + reg = <0x49>; > + clocks = <&g762_clk>; /* input clock */ > + fan_gear_mode = <0>; > + fan_startv = <1>; > + pwm_polarity = <0>; > + }; > + > + g751: g751@4c { > + compatible = "gmt,g751"; > + reg = <0x4c>; > + }; > + }; > + }; > + }; > + > + clocks { > + #address-cells = <1>; > + #size-cells = <0>; Your nodes below neither have a reg property nor can they be addressed in any way. Both properties above are not required, so please remove them. > + g762_clk: fixedclk { s/fixedclk/oscillator/ ? > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <32768>; > + }; > + }; > + > + gpio_leds { > + compatible = "gpio-leds"; > + pinctrl-0 = <&sata1_led_pin &sata2_led_pin &err_led_pin > + &sata3_led_pin &sata4_led_pin>; > + pinctrl-names = "default"; > + > + red_sata1_led { > + label = "rn2120:red:sata1"; > + gpios = <&gpio0 31 0>; /* GPIO 31 Active High */ You can #include then use gpios = <&gpio0 31 GPIO_ACTIVE_HIGH> and get rid of the comments. > + default-state = "off"; > + }; > + > + red_sata2_led { > + label = "rn2120:red:sata2"; > + gpios = <&gpio1 8 0>; /* GPIO 40 Active High */ > + default-state = "off"; > + }; > + > + red_sata3_led { > + label = "rn2120:red:sata3"; > + gpios = <&gpio1 12 0>; /* GPIO 44 Active High */ > + default-state = "off"; > + }; > + > + red_sata4_led { > + label = "rn2120:red:sata4"; > + gpios = <&gpio1 15 0>; /* GPIO 47 Active High */ > + default-state = "off"; > + }; > + > + red_err_led { > + label = "rn2120:red:err"; > + gpios = <&gpio1 13 1>; /* GPIO 45 Active Low */ > + default-state = "off"; > + }; > + }; > + > + gpio_keys { > + compatible = "gpio-keys"; > + #address-cells = <1>; > + #size-cells = <0>; Again, no addressing of those buttons possible. > + pinctrl-0 = <&power_key_pin > + &reset_key_pin>; > + pinctrl-names = "default"; > + > + button@1 { Instead of button@n you can name the nodes after their function, e.g. power_button, reset_button, ... I know both comments above are in the current binding documentation (at least for gpio-keys-polled.txt, there is no gpio-keys.txt); but IMHO both "bus-like" structure of gpio_keys and numbering of buttons just looks so wrong. Sebastian > + label = "Power Button"; > + linux,code = <116>; /* KEY_POWER */ > + gpios = <&gpio0 27 0>; > + }; > + > + button@2 { > + label = "Reset Button"; > + linux,code = <0x198>; /* KEY_RESTART */ > + gpios = <&gpio1 9 1>; > + }; > + }; > + > + gpio_poweroff { > + compatible = "gpio-poweroff"; > + pinctrl-0 = <&poweroff>; > + pinctrl-names = "default"; > + gpios = <&gpio1 10 1>; > + }; > +}; > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html