linux-kernel - Re: [RFC PATCH] ARM: dts: Add support for Turris Omnia

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20161123081923.p5eqgxf33mtqiqza@perseus.defre.kleine-koenig.org>
Date:   Wed, 23 Nov 2016 09:19:23 +0100
From:   Uwe Kleine-König <uwe@...ine-koenig.org>
To:     Tomas Hlavacek <tmshlvck@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Jason Cooper <jason@...edaemon.net>,
        Andrew Lunn <andrew@...n.ch>,
        Gregory Clement <gregory.clement@...e-electrons.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] ARM: dts: Add support for Turris Omnia

Hello Tomas,

calling it v4 would be nice.

On Wed, Nov 23, 2016 at 01:09:20AM +0100, Tomas Hlavacek wrote:
> Turris Omnia board by CZ.NIC:
> 
>   * Marvell Armada 385 SoC
>   * 1 or 2 GB DDR3
>   * eMMC
>   * 8 MB SPI flash (U-Boot and rescue Linux image)
>   * 88E1514 PHY
>   * 88E6176 Ethernet switch (not supported)
> 
> Supported board revision: CZ11NIC13 (production board).
> 
> Signed-off-by: Tomas Hlavacek <tmshlvck@...il.com>

As you picked my v3, you should keep my S-o-b.

> ---
> Changes since Uwe's version:
> 
> - add MBUS regions (needed for Marvell CESA)
> - remove rtc disable (WFM with CZ11NIC13 = production board)

If I do

	mw 0xf10184a0 0xfd4d4cfa

in the boot loader, it seems to work for me, too. You don't need that?

> - cleanup comments
> 
> Unsupported peripherals:
> - MV88E7176 switch
> - SFP
> - LEDs

LEDs is not that bad IMHO, because they work. You just cannot change
their function, but they blink according to their default trigger.
 
> ---
>  arch/arm/boot/dts/Makefile                    |   1 +
>  arch/arm/boot/dts/armada-385-turris-omnia.dts | 279 ++++++++++++++++++++++++++
>  2 files changed, 280 insertions(+)
>  create mode 100644 arch/arm/boot/dts/armada-385-turris-omnia.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index befcd26..f1d3b9ff 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -920,6 +920,7 @@ dtb-$(CONFIG_MACH_ARMADA_38X) += \
>  	armada-385-db-ap.dtb \
>  	armada-385-linksys-caiman.dtb \
>  	armada-385-linksys-cobra.dtb \
> +	armada-385-turris-omnia.dtb \
>  	armada-388-clearfog.dtb \
>  	armada-388-db.dtb \
>  	armada-388-gp.dtb \
> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> new file mode 100644
> index 0000000..5ef3d62
> --- /dev/null
> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> @@ -0,0 +1,279 @@
> +/*
> + * Device Tree file for the Turris Omnia
> + * Schematic available at https://www.turris.cz/doc/_media/rtrom01-schema.pdf
> + *
> + * Copyright (C) 2016 Uwe Kleine-König <uwe@...ine-koenig.org>
> + * Copyright (C) 2016 Tomas Hlavacek <tmshlvkc@...il.com>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) 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.
> + *
> + * Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + *     obtaining a copy of this software and associated documentation
> + *     files (the "Software"), to deal in the Software without
> + *     restriction, including without limitation the rights to use,
> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> + *     sell copies of the Software, and to permit persons to whom the
> + *     Software is furnished to do so, subject to the following
> + *     conditions:
> + *
> + *     The above copyright notice and this permission notice shall be
> + *     included in all copies or substantial portions of the Software.
> + *
> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *     OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include "armada-385.dtsi"
> +
> +/ {
> +	model = "Turris Omnia";
> +	compatible = "cznic,turris-omnia", "marvell,armada385", \
> +			"marvell,armada380";

You don't need a \ here AFAIK.

> +
> +	chosen {
> +		stdout-path = &uart0;
> +	};
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x00000000 0x40000000>; /* 1024 MB */
> +	};
> +
> +	soc {
> +		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000
> +			  MBUS_ID(0x01, 0x1d) 0 0xfff00000 0x100000
> +			  MBUS_ID(0x09, 0x19) 0 0xf1100000 0x10000
> +			  MBUS_ID(0x09, 0x15) 0 0xf1110000 0x10000>;
> +
> +		internal-regs {
> +
> +			/* USB part of the PCIe2/USB 2.0 port */
> +			usb@...00 {
> +				status = "okay";
> +			};
> +
> +			sata@...00 {
> +				status = "okay";
> +			};
> +
> +			sdhci@...00 {
> +				pinctrl-names = "default";
> +				pinctrl-0 = <&sdhci_pins>;
> +				status = "okay";
> +
> +				bus-width = <8>;
> +				no-1-8-v;
> +				non-removable;
> +			};
> +
> +			usb3@...00 {
> +				status = "okay";
> +			};
> +
> +			usb3@...00 {
> +				status = "okay";
> +			};
> +		};
> +
> +		pcie-controller {
> +			status = "okay";
> +
> +			pcie@1,0 {
> +				/* Port 0, Lane 0 */
> +				status = "okay";
> +			};
> +
> +			pcie@2,0 {
> +				/* Port 1, Lane 0 */
> +				status = "okay";
> +			};
> +
> +			pcie@3,0 {
> +				/* Port 2, Lane 0 */
> +				status = "okay";
> +			};
> +		};
> +	};
> +};
> +
> +/* Connected to 88E6176 switch, port 6 */
> +&eth0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&ge0_rgmii_pins>;
> +	status = "okay";
> +	phy-mode = "rgmii-id";
> +
> +	fixed-link {
> +		speed = <1000>;
> +		full-duplex;
> +	};
> +};
> +
> +/* Connected to 88E6176 switch, port 5 */
> +&eth1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&ge1_rgmii_pins>;
> +	status = "okay";
> +	phy-mode = "rgmii-id";
> +
> +	fixed-link {
> +		speed = <1000>;
> +		full-duplex;
> +	};
> +};
> +
> +/* WAN port */
> +&eth2 {
> +	status = "okay";
> +	phy-mode = "sgmii";
> +	phy = <&phy1>;
> +};
> +
> +&i2c0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c0_pins>;
> +	status = "okay";
> +
> +	i2cmux@70 {
> +		compatible = "nxp,pca9547";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0x70>;
> +		status = "okay";
> +
> +		i2c@0 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0>;
> +			status = "okay";
> +
> +			/* STM32F0 command interface at address 0x2a.
> +			 * STM32F0 LED interface at address 0x2b.
> +			 */

Should this better be:

			/*
			 * STM32F0 command interface at address 0x2a.
			 * STM32F0 LED interface at address 0x2b.
			 */

As is recommended for comments in .c?

> +
> +			eeprom@54 {
> +				compatible = "at,24c64";
> +				reg = <0x54>;
> +
> +				/* The EEPROM contains data for bootloader.
> +				 * Contents:
> +				 *	struct omnia_eeprom {
> +				 *		u32 magic; (=0x0341a034)
> +				 *		u32 ramsize;
ramsize in GiB?

> +				 *		char region[4] (=0x0);
This is for the WLAN regdomain, right?

> +				 *		u32 crc32;
> +				 *	};
> +				 */
ditto for the comment format.

> +			};
> +		};
> +
> +		/* Channel 1: Routed to PCIe0/mSATA connector (CN7A).
> +		 * Channel 2: Routed to PCIe1/USB2 connector (CN61A).
> +		 * Channel 3: Routed to PCIe2 connector (CN62A).
> +		 * Channel 4: Routed to SFP+.
> +		 * Channel 5: ATSHA204A at address 0x64.
> +		 * Channel 6: Routed to user pin header CN11.
> +		 */
I'd like to keep the busses as Andrew already pointed out. For example
this might make it possible to use i2c-tools to read out the mac address
from the ATSHA.

> +		i2c@7 {
> +			/* GPIO expander for SFP+ signals */
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <7>;
> +
> +			wangpio: gpio@71 {
> +				compatible = "nxp,pca9538";
> +				reg = <0x71>;
> +				interrupt-parent = <&gpio1>;
> +				interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +			};
> +		};
> +	};
> +};
> +
> +&mdio {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mdio_pins>;
> +	status = "okay";
> +
> +	phy1: phy@1 {
> +		status = "okay";
> +		compatible = "ethernet-phy-id0141.0DD1", \
> +				"ethernet-phy-ieee802.3-c22";

Drop the \

> +		reg = <1>;
> +		/* IRQ is connected to PCA9538 pin 7. Currently it
> +		 * can not be utilized.
> +		 */
> +	};
> +
> +	/* Switch MV88E7176 at address 0x10. */
> +};
> +
> +&pinctrl {
> +	spi0cs1_pins: spi0-pins-0cs1 {
> +		marvell,pins = "mpp26";
> +		marvell,function = "spi0";
> +	};

Why did you drop the pcawan pinctrl?

> +};
> +
> +&spi0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&spi0_pins &spi0cs1_pins>;

Oh, this is wrong (already in my patch): this is cs0 not cs1.

> +	status = "okay";
> +
> +	spi-nor@0 {
> +		compatible = "spansion,s25fl164k", "jedec,spi-nor";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0>;
> +		spi-max-frequency = <40000000>;
> +
> +		partition@0 {
> +			reg = <0x0 0x00100000>;
> +			label = "U-Boot";
> +		};
> +
> +		partition@1 {
> +			reg = <0x00100000 0x00700000>;
> +			label = "Rescue system";
> +		};
> +	};
> +
> +	/* SPI0 + CS1 (MPP26) is routed to a pin header CN11. */

Looks strange. What about

	/* MISO, MOSI, SCLK and CS1 are routed to pin header CN11 */

Maybe also add the node for this pin to &pinctrl, but don't use it in
&spi0.pinctrl-0? This would nicely document the MPP26 part.

> +};
> +
> +&uart0 {
> +	/* Pin header CN10. */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_pins>;
> +	status = "okay";
> +};
> +
> +&uart1 {
> +	/* Pin header CN11. */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_pins>;
> +	status = "okay";
> +};
> +

Trailing new line

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)