LinuxLists.cc - [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

[permalink] [raw]

Subject: [PATCH 2/2] Adding lenovo in vendor

Signed-off-by: benoitm974 <[email protected]>
---
Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 46a311e..de81a87 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -72,6 +72,7 @@ keymile Keymile GmbH
lacie LaCie
lantiq Lantiq Semiconductor
lg LG Corporation
+lenovo LENOVO
linux Linux-specific binding
lsi LSI Corp. (LSI Logic)
lltc Linear Technology Corporation
--
1.9.1

2014-07-23 13:45:55

[permalink] [raw]

Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

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 <[email protected]>

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 <[email protected]>
> + *
> + * 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.

2014-07-23 13:48:04

[permalink] [raw]

Subject: Re: [PATCH 2/2] Adding lenovo in vendor

Benoit,

The same comments regard patch submission apply to this one as well.

On Wed, Jul 23, 2014 at 05:07:12AM -0700, benoitm974 wrote:

Please add a commit entry here describing Lenovo. Keep it short and
sweet and maybe a link to their webpage.

> Signed-off-by: benoitm974 <[email protected]>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 46a311e..de81a87 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -72,6 +72,7 @@ keymile Keymile GmbH
> lacie LaCie
> lantiq Lantiq Semiconductor
> lg LG Corporation
> +lenovo LENOVO

is this their official, registered company name?

> linux Linux-specific binding
> lsi LSI Corp. (LSI Logic)
> lltc Linear Technology Corporation

thx,

Jason.

2014-07-23 13:49:13

[permalink] [raw]

Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2014-07-23 14:20:14

[permalink] [raw]

Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

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

2014-07-23 15:15:44

[permalink] [raw]

Subject: Re: [PATCH 2/2] Adding lenovo in vendor

> > +lenovo LENOVO
>
> is this their official, registered company name?

http://www.lenovo.com/ww/lenovo/FAQs.html says

1. What is Lenovo’s stock symbol (ticker)?
HKSE: 992.hk
ADR (Level I): LNVGY

So i think lenovo is probably the more understandable vendor prefix,
even if it is not the official stock ticker.

However there official registered company name appears to be
Lenovo Group Limited.

Andrew

2014-07-23 15:46:09

by Sebastian Hesselbarth

[permalink] [raw]

Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

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 <[email protected]>
>> + *
>> + * 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

2014-07-23 15:53:19

[permalink] [raw]

Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

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 <[email protected]> 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

2014-07-23 16:55:30

[permalink] [raw]

Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

> 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

2014-07-23 17:27:08

[permalink] [raw]

Subject: Re: [PATCH 2/2] Adding lenovo in vendor

On Wed, Jul 23, 2014 at 05:10:33PM +0200, Andrew Lunn wrote:
> > > +lenovo LENOVO
> >
> > is this their official, registered company name?
>
> http://www.lenovo.com/ww/lenovo/FAQs.html says
>
> 1. What is Lenovo’s stock symbol (ticker)?
> HKSE: 992.hk
> ADR (Level I): LNVGY
>
> So i think lenovo is probably the more understandable vendor prefix,
> even if it is not the official stock ticker.
>
> However there official registered company name appears to be
> Lenovo Group Limited.

Agreed, Benoit, please do 'lenovo Lenovo Group Limited'

thx,

Jason.

2014-07-23 21:03:23

[permalink] [raw]

Subject: Re: [PATCH 2/2] Adding lenovo in vendor

Looking at the vendor list it should then be "Lenovo Group Ltd." since no other vendor except 1 use Limited all other use Ltd.
Fine with this ?
Le 23 juil. 2014 ? 19:26, Jason Cooper <[email protected]> a ?crit :

> On Wed, Jul 23, 2014 at 05:10:33PM +0200, Andrew Lunn wrote:
>>>> +lenovo LENOVO
>>>
>>> is this their official, registered company name?
>>
>> http://www.lenovo.com/ww/lenovo/FAQs.html says
>>
>> 1. What is Lenovo?s stock symbol (ticker)?
>> HKSE: 992.hk
>> ADR (Level I): LNVGY
>>
>> So i think lenovo is probably the more understandable vendor prefix,
>> even if it is not the official stock ticker.
>>
>> However there official registered company name appears to be
>> Lenovo Group Limited.
>
> Agreed, Benoit, please do 'lenovo Lenovo Group Limited'
>
> thx,
>
> Jason.

2014-07-23 21:15:28

[permalink] [raw]

Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

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 <[email protected]> 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

2014-07-23 22:31:59

[permalink] [raw]

Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas

> 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

2014-07-23 23:26:32