| Message ID | 1406117232-5962-1-git-send-email-yahoo@perenite.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Benoit,
Thanks for the patch! A few minor things:
Please Cc: the mvebu maintainers patches regarding Armada XP SoCs, I
almost missed this. ;-)
I know, we probably need to update MAINTAINERS for the dts{i} files...
Also, Please adjust your Subject line to start with "ARM: mvebu: ...",
you can use 'git log --oneline -- arch/arm/boot/dts/armada*' to get an
idea of what we prefer to see.
On Wed, Jul 23, 2014 at 05:07:11AM -0700, benoitm974 wrote:
> Signed-off-by: benoitm974 <yahoo@perenite.com>
Please use your real name here, as well as in the From: of the email.
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts | 267 +++++++++++++++++++++++++
> 2 files changed, 268 insertions(+)
> create mode 100644 arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index adb5ed9..f759dd2 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -438,6 +438,7 @@ dtb-$(CONFIG_MACH_ARMADA_XP) += \
> armada-xp-db.dtb \
> armada-xp-gp.dtb \
> armada-xp-netgear-rn2120.dtb \
> + armada-xp-lenovo-ix4300d.dtb \
> armada-xp-matrix.dtb \
> armada-xp-openblocks-ax3-4.dtb
Please place in alphabetical order. Yes, I know it wasn't to begin
with. :( Feel free to fix it up while you are adding your line.
> dtb-$(CONFIG_MACH_DOVE) += dove-cm-a510.dtb \
> diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts
> new file mode 100644
> index 0000000..e04e7a6
> --- /dev/null
> +++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts
> @@ -0,0 +1,267 @@
> +/*
> + * Device Tree file for LENOVO IX4-300d
> + *
> + * Copyright (C) 2014, Benoit Masson <yahoo@perenite.com>
> + *
> + * 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 <dt-bindings/input/input.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include "armada-xp-mv78230.dtsi"
> +
> +/ {
> + model = "LENOVO IX4-300d";
> + compatible = "lenovo,ix4-300d", "marvell,armadaxp-mv78230", "marvell,armadaxp", "marvell,armada-370-xp";
> +
> + chosen {
> + bootargs = "console=ttyS0,115200 earlyprintk";
> + };
> +
> + memory {
> + device_type = "memory";
> + reg = <0 0x00000000 0 0x20000000>; /* 512MB */
> + };
> +
> + soc {
> + ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
> + MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
> +
> + pcie-controller {
> + status = "okay";
> +
> + pcie@1,0 {
> + /* Port 0, Lane 0 */
> + status = "okay";
> + };
> + pcie@5,0 {
> + /* Port 1, Lane 0 */
> + status = "okay";
> + };
> +
spurious extra line, and it looks like you have some whitespace issues.
Please make sure you use leading tabs.
> + };
> +
> + internal-regs {
> + pinctrl {
> + poweroff: poweroff {
> + marvell,pins = "mpp24";
> + marvell,function = "gpio";
> + };
> +
> + power_button_pin: power-button-pin {
> + marvell,pins = "mpp44";
> + marvell,function = "gpio";
> + };
> +
> + reset_button_pin: reset-button-pin {
> + marvell,pins = "mpp45";
> + marvell,function = "gpio";
> + };
> + select_button_pin: select-button-pin {
> + marvell,pins = "mpp41";
> + marvell,function = "gpio";
> + };
> +
> + scroll_button_pin: scroll-button-pin {
> + marvell,pins = "mpp42";
> + marvell,function = "gpio";
> + };
> + hdd_led_pin: hdd-led-pin {
> + marvell,pins = "mpp26";
> + marvell,function = "gpio";
> + };
More leading tabs issues in this block.
> + };
> +
> + serial@12000 {
> + clocks = <&coreclk 0>;
> + status = "okay";
> + };
> +
> + mdio {
> + phy0: ethernet-phy@0 { /* Marvell 88E1318 */
> + reg = <0>;
> + };
> +
> + phy1: ethernet-phy@1 { /* Marvell 88E1318 */
> + reg = <1>;
> + };
> + };
> +
> + ethernet@70000 {
> + status = "okay";
> + phy = <&phy0>;
> + phy-mode = "rgmii-id";
> + };
> +
> + ethernet@74000 {
> + status = "okay";
> + phy = <&phy1>;
> + phy-mode = "rgmii-id";
> + };
> +
> + usb@50000 {
> + status = "okay";
> + };
> +
> + usb@51000 {
> + status = "okay";
> + };
And here.
> +
> + i2c@11000 {
> + compatible = "marvell,mv78230-a0-i2c", "marvell,mv64xxx-i2c";
> + clock-frequency = <400000>;
And here.
> + status = "okay";
> +
> + adt7473@2e {
> + compatible = "adt7473";
> + reg = <0x2e>;
> + };
> +
> + pcf8563@51 {
> + compatible = "pcf8563";
> + reg = <0x51>;
> + };
> +
> + };
Need an empty line here.
> + nand@d0000 {
> + status = "okay";
> + num-cs = <1>;
> + marvell,nand-keep-config;
> + marvell,nand-enable-arbiter;
> + nand-on-flash-bbt;
> +
> + partition@0 {
> + label = "u-boot";
> + reg = <0x0000000 0xe0000>;
> + read-only;
> + };
> +
> + partition@e0000 {
> + label = "u-boot-env";
> + reg = <0xe0000 0x20000>;
> + read-only;
> + };
> +
> + partition@100000 {
> + label = "u-boot-env2";
> + reg = <0x100000 0x20000>;
> + read-only;
> + };
> +
> + partition@120000 {
> + label = "zImage";
> + reg = <0x120000 0x400000>;
> + };
> +
> + partition@520000 {
> + label = "initrd";
> + reg = <0x520000 0x400000>;
> + };
> + partition@xE00000 {
> + label = "boot";
> + reg = <0xE00000 0x3F200000>;
> + };
> + partition@flash {
> + label = "flash";
> + reg = <0x0 0x40000000>;
> + };
> + };
> +
Don't need this empty line.
> + };
> + };
Empty line here.
> + gpio-keys {
> + compatible = "gpio-keys";
> + pinctrl-0 = <&power_button_pin &reset_button_pin &select_button_pin &scroll_button_pin>;
Yeah... I think you get the point ;-) please check the rest of the
patch. I'll stop mentioning it.
> + pinctrl-names = "default";
> +
> + power-button {
> + label = "Power Button";
> + linux,code = <KEY_POWER>;
> + gpios = <&gpio1 12 GPIO_ACTIVE_HIGH>;
> + };
> + reset-button {
> + label = "Reset Button";
> + linux,code = <KEY_RESTART>;
> + gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
> + };
> + select-button {
> + label = "Select Button";
> + linux,code = <BTN_SELECT>;
> + gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
> + };
> + scroll-button {
> + label = "Scroll Button";
> + linux,code = <KEY_SCROLLDOWN>;
> + gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
> + };
> + };
> +
> + spi3 {
> + compatible = "spi-gpio";
> + status = "okay";
> + gpio-sck = <&gpio0 25 0>;
> + gpio-mosi = <&gpio1 15 0>; /*gpio 47*/
> + cs-gpios = <&gpio0 27 0 >;
I know no one else does it, but please use GPIO_ACTIVE_HIGH for these
three.
> + num-chipselects = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + gpio2: gpio2@0 {
> + compatible = "fairchild,74hc595";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0>;
> + registers-number = <2>;
> + spi-max-frequency = <100000>;
> + };
> + };
> +
> + gpio-leds {
> + compatible = "gpio-leds";
> + pinctrl-0 = <&hdd_led_pin>;
> + pinctrl-names = "default";
> +
> + hdd-led {
> + label = "ix4300d:blue:hdd";
> + gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>;
> + default-state = "off";
> + };
> +
> + power-led {
> + label = "ix4300d:power";
> + gpios = <&gpio2 1 GPIO_ACTIVE_LOW>;
> + linux,default-trigger = "timer"; /* init blinking while booting */
Watch >80 char lines.
> + default-state = "on";
> + };
> +
> + sysfail-led {
> + label = "ix4300d:sysfail:red";
> + gpios = <&gpio2 2 GPIO_ACTIVE_HIGH>;
> + default-state = "off";
> + };
> + sys-led {
> + label = "ix4300d:sys:blue";
> + gpios = <&gpio2 3 GPIO_ACTIVE_HIGH>;
> + default-state = "off";
> + };
> + hddfail-led {
> + label = "ix4300d:hddfail:red";
> + gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>;
> + default-state = "off";
> + };
> +
> + };
> + /* warning: you need both eth1 & 0 to be initialize for poweroff to shutdown otherwise it reboots */
Same here, please convert to multi-line comment.
> + gpio-poweroff {
> + compatible = "gpio-poweroff";
> + pinctrl-0 = <&poweroff>;
> + pinctrl-names = "default";
> + gpios = <&gpio0 24 GPIO_ACTIVE_HIGH>;
> + };
> +
> +};
This is a great first version, it really just a bunch of minor details
to fixup for v2. Please Cc myself, Andrew, Gregory, and Sebastian when
you send v2. I've included them in the Cc.
thx,
Jason.
Benoit, On Wed, Jul 23, 2014 at 05:07:11AM -0700, benoitm974 wrote: I forgot to mention, please add a commit message describing this device and possibly a good link to the company website describing it. thx, Jason. > Signed-off-by: benoitm974 <yahoo@perenite.com> > --- > arch/arm/boot/dts/Makefile | 1 + > arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts | 267 +++++++++++++++++++++++++ > 2 files changed, 268 insertions(+) > create mode 100644 arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts > > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile > index adb5ed9..f759dd2 100644 > --- a/arch/arm/boot/dts/Makefile > +++ b/arch/arm/boot/dts/Makefile > @@ -438,6 +438,7 @@ dtb-$(CONFIG_MACH_ARMADA_XP) += \ > armada-xp-db.dtb \ > armada-xp-gp.dtb \ > armada-xp-netgear-rn2120.dtb \ > + armada-xp-lenovo-ix4300d.dtb \ > armada-xp-matrix.dtb \ > armada-xp-openblocks-ax3-4.dtb > dtb-$(CONFIG_MACH_DOVE) += dove-cm-a510.dtb \ > diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts > new file mode 100644 > index 0000000..e04e7a6 > --- /dev/null > +++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts > @@ -0,0 +1,267 @@ > +/* > + * Device Tree file for LENOVO IX4-300d > + * > + * Copyright (C) 2014, Benoit Masson <yahoo@perenite.com> > + * > + * 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 <dt-bindings/input/input.h> > +#include <dt-bindings/gpio/gpio.h> > +#include "armada-xp-mv78230.dtsi" > + > +/ { > + model = "LENOVO IX4-300d"; > + compatible = "lenovo,ix4-300d", "marvell,armadaxp-mv78230", "marvell,armadaxp", "marvell,armada-370-xp"; > + > + chosen { > + bootargs = "console=ttyS0,115200 earlyprintk"; > + }; > + > + memory { > + device_type = "memory"; > + reg = <0 0x00000000 0 0x20000000>; /* 512MB */ > + }; > + > + soc { > + ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 > + MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>; > + > + pcie-controller { > + status = "okay"; > + > + pcie@1,0 { > + /* Port 0, Lane 0 */ > + status = "okay"; > + }; > + pcie@5,0 { > + /* Port 1, Lane 0 */ > + status = "okay"; > + }; > + > + }; > + > + internal-regs { > + pinctrl { > + poweroff: poweroff { > + marvell,pins = "mpp24"; > + marvell,function = "gpio"; > + }; > + > + power_button_pin: power-button-pin { > + marvell,pins = "mpp44"; > + marvell,function = "gpio"; > + }; > + > + reset_button_pin: reset-button-pin { > + marvell,pins = "mpp45"; > + marvell,function = "gpio"; > + }; > + select_button_pin: select-button-pin { > + marvell,pins = "mpp41"; > + marvell,function = "gpio"; > + }; > + > + scroll_button_pin: scroll-button-pin { > + marvell,pins = "mpp42"; > + marvell,function = "gpio"; > + }; > + hdd_led_pin: hdd-led-pin { > + marvell,pins = "mpp26"; > + marvell,function = "gpio"; > + }; > + }; > + > + serial@12000 { > + clocks = <&coreclk 0>; > + status = "okay"; > + }; > + > + mdio { > + phy0: ethernet-phy@0 { /* Marvell 88E1318 */ > + reg = <0>; > + }; > + > + phy1: ethernet-phy@1 { /* Marvell 88E1318 */ > + reg = <1>; > + }; > + }; > + > + ethernet@70000 { > + status = "okay"; > + phy = <&phy0>; > + phy-mode = "rgmii-id"; > + }; > + > + ethernet@74000 { > + status = "okay"; > + phy = <&phy1>; > + phy-mode = "rgmii-id"; > + }; > + > + usb@50000 { > + status = "okay"; > + }; > + > + usb@51000 { > + status = "okay"; > + }; > + > + i2c@11000 { > + compatible = "marvell,mv78230-a0-i2c", "marvell,mv64xxx-i2c"; > + clock-frequency = <400000>; > + status = "okay"; > + > + adt7473@2e { > + compatible = "adt7473"; > + reg = <0x2e>; > + }; > + > + pcf8563@51 { > + compatible = "pcf8563"; > + reg = <0x51>; > + }; > + > + }; > + nand@d0000 { > + status = "okay"; > + num-cs = <1>; > + marvell,nand-keep-config; > + marvell,nand-enable-arbiter; > + nand-on-flash-bbt; > + > + partition@0 { > + label = "u-boot"; > + reg = <0x0000000 0xe0000>; > + read-only; > + }; > + > + partition@e0000 { > + label = "u-boot-env"; > + reg = <0xe0000 0x20000>; > + read-only; > + }; > + > + partition@100000 { > + label = "u-boot-env2"; > + reg = <0x100000 0x20000>; > + read-only; > + }; > + > + partition@120000 { > + label = "zImage"; > + reg = <0x120000 0x400000>; > + }; > + > + partition@520000 { > + label = "initrd"; > + reg = <0x520000 0x400000>; > + }; > + partition@xE00000 { > + label = "boot"; > + reg = <0xE00000 0x3F200000>; > + }; > + partition@flash { > + label = "flash"; > + reg = <0x0 0x40000000>; > + }; > + }; > + > + }; > + }; > + gpio-keys { > + compatible = "gpio-keys"; > + pinctrl-0 = <&power_button_pin &reset_button_pin &select_button_pin &scroll_button_pin>; > + pinctrl-names = "default"; > + > + power-button { > + label = "Power Button"; > + linux,code = <KEY_POWER>; > + gpios = <&gpio1 12 GPIO_ACTIVE_HIGH>; > + }; > + reset-button { > + label = "Reset Button"; > + linux,code = <KEY_RESTART>; > + gpios = <&gpio1 13 GPIO_ACTIVE_LOW>; > + }; > + select-button { > + label = "Select Button"; > + linux,code = <BTN_SELECT>; > + gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; > + }; > + scroll-button { > + label = "Scroll Button"; > + linux,code = <KEY_SCROLLDOWN>; > + gpios = <&gpio1 10 GPIO_ACTIVE_LOW>; > + }; > + }; > + > + spi3 { > + compatible = "spi-gpio"; > + status = "okay"; > + gpio-sck = <&gpio0 25 0>; > + gpio-mosi = <&gpio1 15 0>; /*gpio 47*/ > + cs-gpios = <&gpio0 27 0 >; > + num-chipselects = <1>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + gpio2: gpio2@0 { > + compatible = "fairchild,74hc595"; > + gpio-controller; > + #gpio-cells = <2>; > + reg = <0>; > + registers-number = <2>; > + spi-max-frequency = <100000>; > + }; > + }; > + > + gpio-leds { > + compatible = "gpio-leds"; > + pinctrl-0 = <&hdd_led_pin>; > + pinctrl-names = "default"; > + > + hdd-led { > + label = "ix4300d:blue:hdd"; > + gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>; > + default-state = "off"; > + }; > + > + power-led { > + label = "ix4300d:power"; > + gpios = <&gpio2 1 GPIO_ACTIVE_LOW>; > + linux,default-trigger = "timer"; /* init blinking while booting */ > + default-state = "on"; > + }; > + > + sysfail-led { > + label = "ix4300d:sysfail:red"; > + gpios = <&gpio2 2 GPIO_ACTIVE_HIGH>; > + default-state = "off"; > + }; > + sys-led { > + label = "ix4300d:sys:blue"; > + gpios = <&gpio2 3 GPIO_ACTIVE_HIGH>; > + default-state = "off"; > + }; > + hddfail-led { > + label = "ix4300d:hddfail:red"; > + gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>; > + default-state = "off"; > + }; > + > + }; > + /* warning: you need both eth1 & 0 to be initialize for poweroff to shutdown otherwise it reboots */ > + gpio-poweroff { > + compatible = "gpio-poweroff"; > + pinctrl-0 = <&poweroff>; > + pinctrl-names = "default"; > + gpios = <&gpio0 24 GPIO_ACTIVE_HIGH>; > + }; > + > +}; > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Benoit, > > > + }; > > + > > + serial@12000 { > > + clocks = <&coreclk 0>; I don't think you need the clocks property. It should be already set in armada-xp.dtsi. > > + adt7473@2e { > > + compatible = "adt7473"; Please include a vendor prefix here. > > + reg = <0x2e>; > > + }; > > + > > + pcf8563@51 { > > + compatible = "pcf8563"; and a vendor prefix here. > > + /* warning: you need both eth1 & 0 to be initialize for poweroff to shutdown otherwise it reboots */ What do you mean by initialized? Driver loaded? Interface up? > This is a great first version I agree with Jason, well done. Andrew
On 07/23/2014 03:45 PM, Jason Cooper wrote: > Thanks for the patch! A few minor things: Benoit, I also have some minor remarks just because I stumble over them all the time. >> --- >> arch/arm/boot/dts/Makefile | 1 + >> arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts | 267 +++++++++++++++++++++++++ I looked it up and Lenovo names it "ix4-300d", maybe you should rename the dts "armada-xp-lenovo-ix4-300d.dts" then. [...] >> diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts >> new file mode 100644 >> index 0000000..e04e7a6 >> --- /dev/null >> +++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts >> @@ -0,0 +1,267 @@ >> +/* >> + * Device Tree file for LENOVO IX4-300d It is spelled "Lenovo" and the full name is "Lenovo Iomega ix4-300d". As it is the initial commit, we should be more sensitive on the case here. >> + * Copyright (C) 2014, Benoit Masson <yahoo@perenite.com> >> + * >> + * 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 <dt-bindings/input/input.h> >> +#include <dt-bindings/gpio/gpio.h> >> +#include "armada-xp-mv78230.dtsi" >> + >> +/ { >> + model = "LENOVO IX4-300d"; Same comment about "Lenovo Iomega ix4-300d" >> + compatible = "lenovo,ix4-300d", "marvell,armadaxp-mv78230", "marvell,armadaxp", "marvell,armada-370-xp"; >> + >> + chosen { >> + bootargs = "console=ttyS0,115200 earlyprintk"; Consider to add stdout-path = "/soc/internal-regs/serial@12000"; That way you would not have to fix it up for e.g. Barebox. Unfortunately, neither Armada 370 nor XP dtsi define node labels yet, so you have to put the full node path. >> + }; >> + [...] >> + soc { >> + ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 >> + MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>; >> + >> + pcie-controller { >> + status = "okay"; >> + >> + pcie@1,0 { >> + /* Port 0, Lane 0 */ >> + status = "okay"; >> + }; >> + pcie@5,0 { >> + /* Port 1, Lane 0 */ >> + status = "okay"; >> + }; >> + Any chance you know what is connected to above PCIe ports? If so, put a comment above the node itself naming the attached device, e.g. /* USB 3.0 xHCI controller */ pcie@1,0 {...} Also, I saw you have a good impression about the GPIOs used on the ix4-300d. If you also know the PERST# GPIOs, add them with reset-gpios = <&gpioN M GPIO_ACTIVE_LOW>; where N, M follow gpio# = (32 * N) + M, i.e. N=1, M=22 for gpio54. >> + }; >> + >> + internal-regs { >> + pinctrl { >> + poweroff: poweroff { nit: poweroff_pin: poweroff-pin {...} like the ones below? >> + marvell,pins = "mpp24"; >> + marvell,function = "gpio"; >> + }; >> + >> + power_button_pin: power-button-pin { >> + marvell,pins = "mpp44"; >> + marvell,function = "gpio"; >> + }; >> + >> + reset_button_pin: reset-button-pin { >> + marvell,pins = "mpp45"; >> + marvell,function = "gpio"; >> + }; >> + select_button_pin: select-button-pin { >> + marvell,pins = "mpp41"; >> + marvell,function = "gpio"; >> + }; >> + >> + scroll_button_pin: scroll-button-pin { >> + marvell,pins = "mpp42"; >> + marvell,function = "gpio"; >> + }; >> + hdd_led_pin: hdd-led-pin { >> + marvell,pins = "mpp26"; >> + marvell,function = "gpio"; >> + }; [...] >> + }; >> + >> + serial@12000 { >> + clocks = <&coreclk 0>; As Andrew already said, no need to duplicate the clocks property. >> + status = "okay"; >> + }; [...] >> + spi3 { >> + compatible = "spi-gpio"; >> + status = "okay"; >> + gpio-sck = <&gpio0 25 0>; >> + gpio-mosi = <&gpio1 15 0>; /*gpio 47*/ >> + cs-gpios = <&gpio0 27 0 >; [...] >> + num-chipselects = <1>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + gpio2: gpio2@0 { >> + compatible = "fairchild,74hc595"; Freaky we have a driver for a 74x595 8-bit shift register ;) >> + gpio-controller; >> + #gpio-cells = <2>; >> + reg = <0>; >> + registers-number = <2>; >> + spi-max-frequency = <100000>; >> + }; >> + }; >> + >> + gpio-leds { >> + compatible = "gpio-leds"; >> + pinctrl-0 = <&hdd_led_pin>; >> + pinctrl-names = "default"; >> + >> + hdd-led { >> + label = "ix4300d:blue:hdd"; Can you get rid of "ix4300d:" or rename it to "ix4-300d:" without breaking any userspace stuff? If so, please update all LEDs. >> + gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>; >> + default-state = "off"; >> + }; [...] >> + /* warning: you need both eth1 & 0 to be initialize for poweroff to shutdown otherwise it reboots */ I don't get the comment. > This is a great first version Yup, Thanks! Sebastian
Hi Andrew I've taken your comment into account, yet for the PHY init comment, see my answer below. Le 23 juil. 2014 à 16:14, Andrew Lunn <andrew@lunn.ch> a écrit : > >>> + /* warning: you need both eth1 & 0 to be initialize for poweroff to shutdown otherwise it reboots */ > > What do you mean by initialized? Driver loaded? Interface up? Well actually the PHY need to be initialized (at least 1 mII reg written), which from marvel LSP driver always occurs, while it doesn't with mainline PHY driver (drivers/net/phy/marvell.c), so the only simple way I found to have at least one PHY reg on both interface written is to have both eth up at OS config level. Probably the best option would be to have a reg-init = <reg offset value> on both phy dts definition but the current armada mii doesn't support this dts config... > >> This is a great first version > > I agree with Jason, well done. > Thanks ! > Andrew
> Well actually the PHY need to be initialized (at least 1 mII reg > written), which from marvel LSP driver always occurs, while it > doesn't with mainline PHY driver (drivers/net/phy/marvell.c), so the > only simple way I found to have at least one PHY reg on both > interface written is to have both eth up at OS config level. Thanks for the information. This sounds like a wake on LAN feature. I've seen other Marvell hardware connect a PHY LED output pin to the circuit controlling the main power supply. When the PHY detects the magic wake-up packet, it 'blinks' the LED so turning the power back on. My guess is, the register write to the PHY is configuring the LED. Do you have the datasheet for the PHY? Can you check this? > Probably the best option would be to have a reg-init = <reg offset > value> on both phy dts definition but the current armada mii doesn't > support this dts config... Once we understand what is going on here, we can consider adding support for this. Andrew
Well after several test with the original BSP driver I found that this is not fully WOL related. It could be that the LED pin is linked to poweron for WOL, yet here the issue is on shutdown pulling up MPP24 should poweroff the deice but only reboots it except is both PHY have some reg written to...
By the way the current nv_neta does not seems to support WOL for 88e1318 and still having them both up is enough to have poweroff to work, which is why I'm putting the WOL as a side track.
Both phy are :
marvell,88e1318
example of a minimal reg write that lead MPP24 to shutdown instead of rebooting on original BSP driver
XXXXX BasicInit
XXXXXXXXXXXXXX phyAdr 0: regOffs: 16 data: 3 caller: mvEthE1310PhyBasicInit+0x2c/0x58
XXXXXXXXXXXXXX phyAdr 0: regOffs: 10 data: 830 caller: mvEthE1310PhyBasicInit+0x3c/0x58
XXXXXXXXXXXXXX phyAdr 0: regOffs: 16 data: 0 caller: mvEthE1310PhyBasicInit+0x4c/0x58
XXXXX BasicInit
XXXXXXXXXXXXXX phyAdr 1: regOffs: 16 data: 3 caller: mvEthE1310PhyBasicInit+0x2c/0x58
XXXXXXXXXXXXXX phyAdr 1: regOffs: 10 data: 830 caller: mvEthE1310PhyBasicInit+0x3c/0x58
XXXXXXXXXXXXXX phyAdr 1: regOffs: 16 data: 0 caller: mvEthE1310PhyBasicInit+0x4c/0x58
I've tried something like:
compatible = "marvell,88e1318";
device_type = "ethernet-phy";
marvell,reg-init =
/* Init led/activity workaround for MP24 shutdown. */
<3 0x10 0 0x830>;
In the dts but without luck
Regards,
Benoit
Le 23 juil. 2014 à 18:49, Andrew Lunn <andrew@lunn.ch> a écrit :
>> Well actually the PHY need to be initialized (at least 1 mII reg
>> written), which from marvel LSP driver always occurs, while it
>> doesn't with mainline PHY driver (drivers/net/phy/marvell.c), so the
>> only simple way I found to have at least one PHY reg on both
>> interface written is to have both eth up at OS config level.
>
> Thanks for the information. This sounds like a wake on LAN feature.
> I've seen other Marvell hardware connect a PHY LED output pin to the
> circuit controlling the main power supply. When the PHY detects the
> magic wake-up packet, it 'blinks' the LED so turning the power back
> on.
>
> My guess is, the register write to the PHY is configuring the LED. Do
> you have the datasheet for the PHY? Can you check this?
>
>> Probably the best option would be to have a reg-init = <reg offset
>> value> on both phy dts definition but the current armada mii doesn't
>> support this dts config...
>
> Once we understand what is going on here, we can consider adding
> support for this.
>
> Andrew
> Both phy are : > marvell,88e1318 I don't have the datasheet for this specific phy model, but i do have the datasheet for another similar phy. > example of a minimal reg write that lead MPP24 to shutdown instead > of rebooting on original BSP driver > XXXXX BasicInit I'm assuming regOffs is decimal, and data is hex? > phyAdr 0: regOffs: 16 data: 3 Copper Specific control register. 3 means Polarity Reversal Disable & Jabber function disable. > phyAdr 0: regOffs: 10 data: 830 Reg 10 is the 1000BASE-T Status register, which is read only! So this is not making much sense. Are we missing some changes to the page register? Register 16 of page 3 is the LED control register. Andrew
Well I'm glad you spend time on this. So I'll explain more in detail where I am on this one. I have the vendor 3.0.29 kernel patched with LSP drivers from Marvell (which includes the PHY & Net drivers). With this version (called LSPkernel now on) pulling MPP24 GPIO output up shutdown the power of the device. At first I though it could be watchdog or wol related so I deep dive into both drivers and start uncommenting register write by register write, in the end the LSPKernel start rebooting instead of poweroff when no phy register were written at all... which is when I start thinking about that its was maybe not really which reg were written but that both phy were to be writtent to at least on reg for the MPP24 to poweroff the device and that worked out. So as soon as both phy get "m88e1318_config_aneg" fucntion called from "drivers/net/phy/marvell.c" it works fine. Also maybe you can help me there although the marvell.c has m88e1318_get_wol & m88e1318_set_wol setup as part of the MARVELL_PHY_ID_88E1318S calling "ethtool -s eth0 wol g" from debian return a non-supported error .. any though ? Benoit Le 24 juil. 2014 à 00:26, Andrew Lunn <andrew@lunn.ch> a écrit : >> Both phy are : >> marvell,88e1318 > > I don't have the datasheet for this specific phy model, but i do have > the datasheet for another similar phy. > >> example of a minimal reg write that lead MPP24 to shutdown instead >> of rebooting on original BSP driver > >> XXXXX BasicInit > > I'm assuming regOffs is decimal, and data is hex? > >> phyAdr 0: regOffs: 16 data: 3 > > Copper Specific control register. 3 means Polarity Reversal Disable & > Jabber function disable. > >> phyAdr 0: regOffs: 10 data: 830 > > Reg 10 is the 1000BASE-T Status register, which is read only! > > So this is not making much sense. Are we missing some changes to the > page register? Register 16 of page 3 is the LED control register. > > Andrew >
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index adb5ed9..f759dd2 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -438,6 +438,7 @@ dtb-$(CONFIG_MACH_ARMADA_XP) += \ armada-xp-db.dtb \ armada-xp-gp.dtb \ armada-xp-netgear-rn2120.dtb \ + armada-xp-lenovo-ix4300d.dtb \ armada-xp-matrix.dtb \ armada-xp-openblocks-ax3-4.dtb dtb-$(CONFIG_MACH_DOVE) += dove-cm-a510.dtb \ diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts new file mode 100644 index 0000000..e04e7a6 --- /dev/null +++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts @@ -0,0 +1,267 @@ +/* + * Device Tree file for LENOVO IX4-300d + * + * Copyright (C) 2014, Benoit Masson <yahoo@perenite.com> + * + * 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 <dt-bindings/input/input.h> +#include <dt-bindings/gpio/gpio.h> +#include "armada-xp-mv78230.dtsi" + +/ { + model = "LENOVO IX4-300d"; + compatible = "lenovo,ix4-300d", "marvell,armadaxp-mv78230", "marvell,armadaxp", "marvell,armada-370-xp"; + + chosen { + bootargs = "console=ttyS0,115200 earlyprintk"; + }; + + memory { + device_type = "memory"; + reg = <0 0x00000000 0 0x20000000>; /* 512MB */ + }; + + soc { + ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 + MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>; + + pcie-controller { + status = "okay"; + + pcie@1,0 { + /* Port 0, Lane 0 */ + status = "okay"; + }; + pcie@5,0 { + /* Port 1, Lane 0 */ + status = "okay"; + }; + + }; + + internal-regs { + pinctrl { + poweroff: poweroff { + marvell,pins = "mpp24"; + marvell,function = "gpio"; + }; + + power_button_pin: power-button-pin { + marvell,pins = "mpp44"; + marvell,function = "gpio"; + }; + + reset_button_pin: reset-button-pin { + marvell,pins = "mpp45"; + marvell,function = "gpio"; + }; + select_button_pin: select-button-pin { + marvell,pins = "mpp41"; + marvell,function = "gpio"; + }; + + scroll_button_pin: scroll-button-pin { + marvell,pins = "mpp42"; + marvell,function = "gpio"; + }; + hdd_led_pin: hdd-led-pin { + marvell,pins = "mpp26"; + marvell,function = "gpio"; + }; + }; + + serial@12000 { + clocks = <&coreclk 0>; + status = "okay"; + }; + + mdio { + phy0: ethernet-phy@0 { /* Marvell 88E1318 */ + reg = <0>; + }; + + phy1: ethernet-phy@1 { /* Marvell 88E1318 */ + reg = <1>; + }; + }; + + ethernet@70000 { + status = "okay"; + phy = <&phy0>; + phy-mode = "rgmii-id"; + }; + + ethernet@74000 { + status = "okay"; + phy = <&phy1>; + phy-mode = "rgmii-id"; + }; + + usb@50000 { + status = "okay"; + }; + + usb@51000 { + status = "okay"; + }; + + i2c@11000 { + compatible = "marvell,mv78230-a0-i2c", "marvell,mv64xxx-i2c"; + clock-frequency = <400000>; + status = "okay"; + + adt7473@2e { + compatible = "adt7473"; + reg = <0x2e>; + }; + + pcf8563@51 { + compatible = "pcf8563"; + reg = <0x51>; + }; + + }; + nand@d0000 { + status = "okay"; + num-cs = <1>; + marvell,nand-keep-config; + marvell,nand-enable-arbiter; + nand-on-flash-bbt; + + partition@0 { + label = "u-boot"; + reg = <0x0000000 0xe0000>; + read-only; + }; + + partition@e0000 { + label = "u-boot-env"; + reg = <0xe0000 0x20000>; + read-only; + }; + + partition@100000 { + label = "u-boot-env2"; + reg = <0x100000 0x20000>; + read-only; + }; + + partition@120000 { + label = "zImage"; + reg = <0x120000 0x400000>; + }; + + partition@520000 { + label = "initrd"; + reg = <0x520000 0x400000>; + }; + partition@xE00000 { + label = "boot"; + reg = <0xE00000 0x3F200000>; + }; + partition@flash { + label = "flash"; + reg = <0x0 0x40000000>; + }; + }; + + }; + }; + gpio-keys { + compatible = "gpio-keys"; + pinctrl-0 = <&power_button_pin &reset_button_pin &select_button_pin &scroll_button_pin>; + pinctrl-names = "default"; + + power-button { + label = "Power Button"; + linux,code = <KEY_POWER>; + gpios = <&gpio1 12 GPIO_ACTIVE_HIGH>; + }; + reset-button { + label = "Reset Button"; + linux,code = <KEY_RESTART>; + gpios = <&gpio1 13 GPIO_ACTIVE_LOW>; + }; + select-button { + label = "Select Button"; + linux,code = <BTN_SELECT>; + gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; + }; + scroll-button { + label = "Scroll Button"; + linux,code = <KEY_SCROLLDOWN>; + gpios = <&gpio1 10 GPIO_ACTIVE_LOW>; + }; + }; + + spi3 { + compatible = "spi-gpio"; + status = "okay"; + gpio-sck = <&gpio0 25 0>; + gpio-mosi = <&gpio1 15 0>; /*gpio 47*/ + cs-gpios = <&gpio0 27 0 >; + num-chipselects = <1>; + #address-cells = <1>; + #size-cells = <0>; + + gpio2: gpio2@0 { + compatible = "fairchild,74hc595"; + gpio-controller; + #gpio-cells = <2>; + reg = <0>; + registers-number = <2>; + spi-max-frequency = <100000>; + }; + }; + + gpio-leds { + compatible = "gpio-leds"; + pinctrl-0 = <&hdd_led_pin>; + pinctrl-names = "default"; + + hdd-led { + label = "ix4300d:blue:hdd"; + gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>; + default-state = "off"; + }; + + power-led { + label = "ix4300d:power"; + gpios = <&gpio2 1 GPIO_ACTIVE_LOW>; + linux,default-trigger = "timer"; /* init blinking while booting */ + default-state = "on"; + }; + + sysfail-led { + label = "ix4300d:sysfail:red"; + gpios = <&gpio2 2 GPIO_ACTIVE_HIGH>; + default-state = "off"; + }; + sys-led { + label = "ix4300d:sys:blue"; + gpios = <&gpio2 3 GPIO_ACTIVE_HIGH>; + default-state = "off"; + }; + hddfail-led { + label = "ix4300d:hddfail:red"; + gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>; + default-state = "off"; + }; + + }; + /* warning: you need both eth1 & 0 to be initialize for poweroff to shutdown otherwise it reboots */ + gpio-poweroff { + compatible = "gpio-poweroff"; + pinctrl-0 = <&poweroff>; + pinctrl-names = "default"; + gpios = <&gpio0 24 GPIO_ACTIVE_HIGH>; + }; + +};
Signed-off-by: benoitm974 <yahoo@perenite.com> --- arch/arm/boot/dts/Makefile | 1 + arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts | 267 +++++++++++++++++++++++++ 2 files changed, 268 insertions(+) create mode 100644 arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts