Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751687AbbDLLoA (ORCPT ); Sun, 12 Apr 2015 07:44:00 -0400 Received: from forward1l.mail.yandex.net ([84.201.143.144]:35430 "EHLO forward1l.mail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751494AbbDLLn5 (ORCPT ); Sun, 12 Apr 2015 07:43:57 -0400 To: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , Linus Walleij , Wolfram Sang , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Andrew Lunn , Gregory Clement , Jason Cooper Subject: Fwd: Re: [PATCH 2/2] ARM: mvebu: dts: Add dts file for DLink DNS-327L X-PHP-Originating-Script: 501:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Sun, 12 Apr 2015 14:43:48 +0300 From: Andrew Message-ID: User-Agent: Roundcube Webmail/1.0.2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12212 Lines: 500 Sebastian Hesselbarth писал 12.04.2015 14:20: > On 11.04.2015 22:29, Andrew Andrianov wrote: >> Signed-off-by: Andrew Andrianov > > Andrew, > > thanks for providing this dts, please _always_ add a commit log > describing what the patch is about. The introduction in the cover > letter is nice, but it will be gone once the patches are applied, > so this patch needs some log, too. > > I also have a DNS-327L and I thought I started a dts, but either > I misremember or I just cannot find it. > > Anyways, some comments below. > >> --- >> arch/arm/boot/dts/Makefile | 1 + >> arch/arm/boot/dts/armada-370-dlink-dns327l.dts | 309 >> ++++++++++++++++++++++++ >> 2 files changed, 310 insertions(+) >> create mode 100644 arch/arm/boot/dts/armada-370-dlink-dns327l.dts >> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile >> index a1c776b..8535e4e 100644 >> --- a/arch/arm/boot/dts/Makefile >> +++ b/arch/arm/boot/dts/Makefile >> @@ -612,6 +612,7 @@ dtb-$(CONFIG_ARCH_ZYNQ) += \ >> zynq-zybo.dtb >> dtb-$(CONFIG_MACH_ARMADA_370) += \ >> armada-370-db.dtb \ >> + armada-370-dlink-dns327l.dtb \ >> armada-370-mirabox.dtb \ >> armada-370-netgear-rn102.dtb \ >> armada-370-netgear-rn104.dtb \ >> diff --git a/arch/arm/boot/dts/armada-370-dlink-dns327l.dts >> b/arch/arm/boot/dts/armada-370-dlink-dns327l.dts >> new file mode 100644 >> index 0000000..12bc072 >> --- /dev/null >> +++ b/arch/arm/boot/dts/armada-370-dlink-dns327l.dts >> @@ -0,0 +1,309 @@ >> +/* >> + * Device Tree file for DLINK DNS-327L > > Just a nit, but the company's logo uses "D-Link" so we should > use that in here. > > I am fine with the file named "-370-dlink-dns" though. > >> + * Copyright (C) 2014, Andrew Andrianov > > +2015? > >> + * 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. >> + */ > > Andrew Lunn already mentioned dual licensing here. > >> +/* Remaining mysteries: >> + * >> + * There's still something unknown on i2c address 0x13 > > Well, I can at least tell it is a "device" instead of "something" ;) > > I am ok with the comment about an "unknown device at i2c address 0x13". > >> + * CONFIG_ARM_MVEBU_V7_CPUIDLE=y causes hard freezes every 1-8 hours > > I don't think the dts is the right place for Linux issues. Not sure if that's a hardware weirdness or software issue (yet). Just checked - this goblin is there in 4.0-rc7. > >> + * >> + */ >> + >> +/dts-v1/; >> + >> +#include >> +#include >> +#include "armada-370.dtsi" >> + >> +/ { >> + model = "DLINK DNS-327L"; > > "D-Link DNS-327L" > >> + compatible = "dlink,dns327l", >> + "marvell,armada370", >> + "marvell,armada-370-xp"; >> + >> + chosen { >> + bootargs = "console=ttyS0,115200 earlyprintk"; >> + }; > > Andrew Lunn already mentioned stdout-path property. > >> + memory { >> + device_type = "memory"; >> + reg = <0x00000000 0x20000000>; /* 512 MiB */ >> + }; >> + >> + soc { >> + ranges = > + MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>; >> + >> + pcie-controller { >> + status = "okay"; >> + >> + /* Connected to Marvell SATA controller */ > > Really? It is actually using the two internal SATA ports. Ouch, just a left-over from the netgear DTS file > >> + pcie@1,0 { >> + /* Port 0, Lane 0 */ >> + status = "okay"; >> + }; >> + >> + /* Connected to NEC USB 3.0 controller */ > > Please just enable the port where the USB3 controller is connected > to. Could be the other one. > >> + pcie@2,0 { >> + /* Port 1, Lane 0 */ >> + status = "okay"; >> + }; >> + }; >> + >> + internal-regs { >> + serial@12000 { >> + status = "okay"; >> + }; >> + >> + serial@12100 { >> + status = "okay"; >> + }; > > &uart0 { status = "okay" }; > &uart1 { status = "okay" }; > > And move to bottom of the file. We try not to replay the bus structure > on board level all the time, if possible. > >> + sata@a0000 { >> + nr-ports = <2>; >> + status = "okay"; >> + }; > > Ok, sata has no node label, yet. It has to stay here. > >> + mdio { >> + phy0: ethernet-phy@0 { /* Marvell 88E1318 */ >> + reg = <0>; >> + }; >> + }; > > Something is wrong with indention of the node above and we do have > a node label for &mdio so it can also move. > >> + ethernet@74000 { >> + status = "okay"; >> + phy = <&phy0>; >> + phy-mode = "rgmii-id"; >> + }; > > ditto, ð1. > >> + usb@50000 { >> + status = "okay"; >> + }; > > Again, no node label, yet. > >> + i2c@11000 { >> + compatible = "marvell,mv64xxx-i2c"; >> + clock-frequency = <100000>; >> + status = "okay"; >> + }; > > &i2c0, so please move. > >> + nand@d0000 { > > Again, no node label, yet. > > I guess there will be a cleanup patch set adding proper labels to > armada-370-xp.dtsi some day. > >> + status = "okay"; >> + num-cs = <1>; > > I stumbled upon this on ix4-300d already while adding support for > pxa3xx nfc on barebox bootloader for the armada370/xp type of nfc > controller. > > It is most likely the flash is connected to cs0 just because it is one > selected if boot source is NAND. > > It could be that current barebox and Linux nfc driver deal with > the property differently, I haven't really checked yet. > >> + marvell,nand-keep-config; >> + marvell,nand-enable-arbiter; >> + nand-on-flash-bbt; > > Do you know the ECC scheme used? Any hints on how to find it apart from dumping NAND controller registers from bootloader ? > > If so, please add corresponding nand-ecc-strength and > nand-ecc-step-size properties. > >> + partition@0 { >> + label = "u-boot"; >> + /* 1.0 MiB */ >> + reg = <0x0000000 0x100000>; >> + read-only; >> + }; >> + >> + partition@100000 { >> + label = "u-boot-env"; >> + /* 128 KiB */ >> + reg = <0x100000 0x20000>; >> + read-only; >> + }; >> + >> + partition@120000 { >> + label = "uImage"; >> + /* 7 MiB */ >> + reg = <0x120000 0x700000>; >> + }; >> + >> + partition@820000 { >> + label = "ubifs"; >> + /* ~ 84 MiB */ >> + reg = <0x820000 0x54e0000>; >> + }; >> + >> + /* Hardwired into stock bootloader */ > > I don't get the comment above. The stock u-boot is hacked with a 'failsafe' kernel address. If for some reason running the 'bootcmd' fails, it reads 5MiBs from partition @ (5d00000 + 0x800) and tries to boot it. There's no way to change this via environment, only by replacing the bootloader. Personally I'm more happy with a simpler partition table, but I guess upstream should be oriented towards the stock bootloader. > >> + partition@800000 { > > partition@5d00000 > >> + label = "failsafe-uImage"; >> + /* 5 MiB */ >> + reg = <0x5d00000 0x500000>; >> + }; >> + >> + partition@6200000 { >> + label = "failsafe-fs"; >> + /* 29 MiB */ >> + reg = <0x6200000 0x1d00000>; >> + }; >> + >> + partition@7f00000 { >> + label = "bbt"; >> + /* 1 MiB for BBT */ >> + reg = <0x7f00000 0x100000>; >> + }; >> + }; >> + }; >> + }; >> + >> + gpio-keys { >> + compatible = "gpio-keys"; >> + pinctrl-0 = < >> + &backup_button_pin >> + &power_button_pin>; >> + pinctrl-names = "default"; >> + >> + power-button { >> + label = "Power Button"; >> + linux,code = ; >> + gpios = <&gpio2 1 GPIO_ACTIVE_LOW>; >> + }; >> + >> + backup-button { >> + label = "Backup Button"; >> + linux,code = ; > > Hmm, is KEY_COPY the best option? >> + gpios = <&gpio1 31 GPIO_ACTIVE_LOW>; >> + }; >> + >> + reset-button { >> + label = "Reset Button"; >> + linux,code = ; >> + gpios = <&gpio2 0 GPIO_ACTIVE_LOW>; >> + }; >> + >> + > > Double empty lines above. > >> + }; >> + >> + gpio-leds { >> + compatible = "gpio-leds"; >> + pinctrl-0 = < >> + &sata_l_amber_pin >> + &sata_r_amber_pin >> + &backup_led_pin>; >> + >> + pinctrl-names = "default"; >> + >> + sata-r-amber-pin { >> + label = "dns327l:amber:sata-r"; >> + gpios = <&gpio1 20 GPIO_ACTIVE_HIGH>; >> + default-state = "keep"; > > Wrong indention. > >> + }; >> + >> + sata-l-amber-pin { >> + label = "dns327l:amber:sata-l"; >> + gpios = <&gpio1 21 GPIO_ACTIVE_HIGH>; >> + default-state = "keep"; >> + }; >> + >> + backup-led-pin { >> + label = "dns327l:white:usb"; >> + gpios = <&gpio1 29 GPIO_ACTIVE_HIGH>; >> + default-state = "keep"; >> + }; >> + > > ditto for all gpio nodes above, and another empty line. > >> + }; >> + >> + regulators { >> + compatible = "simple-bus"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + pinctrl-0 = <&xhci_pwr_pin >> + &sata1_pwr_pin >> + &sata2_pwr_pin>; >> + >> + pinctrl-names = "default"; >> + >> + usb_power: regulator@1 { > > Wrong indention. > >> + compatible = "regulator-fixed"; >> + reg = <1>; >> + regulator-name = "USB3.0 Port Power"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + enable-active-high; >> + regulator-boot-on; >> + regulator-always-on; >> + gpio = <&gpio0 13 GPIO_ACTIVE_HIGH>; >> + }; >> + >> + sata1_power: regulator@2 { > > ditto. > >> + compatible = "regulator-fixed"; >> + reg = <1>; > > reg = <2>; > >> + regulator-name = "SATA-1 Power"; > > Front HDD LEDs are actually labeled "L" and "R", can you evaluate > which 0/1 matches L/R and rename the regulator names accordingly? > >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + enable-active-high; >> + regulator-always-on; >> + gpio = <&gpio1 22 GPIO_ACTIVE_HIGH>; >> + }; >> + >> + sata2_power: regulator@3 { > > Wrong indention. > >> + compatible = "regulator-fixed"; >> + reg = <1>; > > reg = <3>; > >> + regulator-name = "SATA-2 Power"; > > Same comment about the 0/1 vs L/R naming. > >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + enable-active-high; >> + regulator-always-on; >> + gpio = <&gpio1 24 GPIO_ACTIVE_HIGH>; >> + }; >> + }; >> +}; >> + >> +&pinctrl { >> + sata1_white_pin: sata1-white-pin { >> + marvell,pins = "mpp55"; >> + marvell,function = "sata1"; >> + }; >> + >> + sata_r_amber_pin: sata-r-amber-pin { >> + marvell,pins = "mpp52"; >> + marvell,function = "gpio"; >> + }; >> + >> + sata2_white_pin: sata2-white-pin { >> + marvell,pins = "mpp57"; >> + marvell,function = "sata0"; >> + }; >> + >> + sata_l_amber_pin: sata-l-amber-pin { >> + marvell,pins = "mpp53"; >> + marvell,function = "gpio"; >> + }; > > If you find the 0/1, L/R mapping you should use it all over this file. > > Sebastian > >> + >> + backup_led_pin: backup-led-pin { >> + marvell,pins = "mpp61"; >> + marvell,function = "gpo"; >> + }; >> + >> + xhci_pwr_pin: xhci-pwr-pin { >> + marvell,pins = "mpp13"; >> + marvell,function = "gpio"; >> + }; >> + >> + sata1_pwr_pin: sata1-pwr-pin { >> + marvell,pins = "mpp54"; >> + marvell,function = "gpio"; >> + }; >> + >> + sata2_pwr_pin: sata2-pwr-pin { >> + marvell,pins = "mpp56"; >> + marvell,function = "gpio"; >> + }; >> + >> + power_button_pin: power-button-pin { >> + marvell,pins = "mpp65"; >> + marvell,function = "gpio"; >> + }; >> + >> + backup_button_pin: backup-button-pin { >> + marvell,pins = "mpp63"; >> + marvell,function = "gpio"; >> + }; >> + >> + reset_button_pin: reset-button-pin { >> + marvell,pins = "mpp64"; >> + marvell,function = "gpio"; >> + }; >> +}; >> Thanks for the review, I'll resubmit the fixed patchset shortly. Please disregard my [PATCH v2] messages. I've send them the moment before I noticed your email and review. -- Regards, Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/