[PATCH] cn913x: add support for iEi Puzzle-M901/Puzzle-M902

[Adrian Schmutzler]

Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces at lists.openwrt.org]
> On Behalf Of eveans2002 at gmail.com
> Sent: Sonntag, 6. Juni 2021 23:56
> To: openwrt-devel at lists.openwrt.org
> Cc: Ian Chang <ianchang at ieiworld.com>
> Subject: [PATCH] cn913x: add support for iEi Puzzle-M901/Puzzle-M902

Looks like you sent in an early alpha-state patch.

Please remove all the unfinished/debugging stuff from the DTS and/or label this as RFC.

A few selected comments below.

> 
> From: Ian Chang <ianchang at ieiworld.com>
> 
>  Hardware specification
>  ----------------------
>  * CN9130 SoC, Quad-core ARMv8 Cortex-72 @ 2200 MHz
>  * 4 GB DDR
>  * 4 GB eMMC
>  mmcblk0
>  ├─mmcblk0p1    64M  kernel_1
>  ├─mmcblk0p2    64M  kernel_2
>  ├─mmcblk0p3   512M  rootfs_1
>  ├─mmcblk0p4   512M  rootfs_2
>  ├─mmcblk0p5   512M  Reserved
>  ├─mmcblk0p6    64M  Reserved
>  └─mmcblk0p7   1.8G  rootfs_data
> 
>  * 4 MB (SPI Flash)
>  * 6 x 2.5 Gigabit  ports (Puzzle-M901)
>     - External PHY with 6 ports (AQR112R)
>  * 6 x 2.5 Gigabit ports (Puzzle-M902)
>     - External PHY with 6 ports (AQR112R)
>    3 x 10 Gigabit ports (Puzzle-M902)
>     - External PHY with 3 ports (AQR113R)
> 
>  * 4 x Front panel LED
>  * 1 x USB 3.0
>  * Reset button on Rear panel
>  * UART (115200 8N1,header on PCB)

Commit message lacks flashing instructions.

[...]

> --- /dev/null
> +++ b/target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-
> armada
> +++ -common.dtsi
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

It's "GPL-2.0-or-later OR MIT".

> +	cp0_sfp_eth0: sfp-eth at 0 {
> +
> +		/*
> +		 * SFP cages are unconnected on early PCBs because of an
> the I2C
> +		 * lanes not being connected. Prevent the port for being
> +		 * unusable by disabling the SFP node.
> +		 */
> +        /* status = "disabled"; */

Remove debugging stuff like this.

> +	};
> +};
> +
> +&uart0 {
> +	status = "okay";
> +};
> +
> +&cp0_uart0 {
> +    status = "okay";

Indent should be only tabs in DTS.

> +		cp0_spi0_pins: cp0-spi-pins-0 {
> +			marvell,pins = "mpp13", "mpp14", "mpp15", "mpp16";
> +			marvell,function = "spi1";
> +		};
> +	};
> +};
> \ No newline at end of file

Please add the newlines ...

> diff --git a/target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-
> m901-cn9131-db.dts
> b/target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-m901-
> cn9131-db.dts
> new file mode 100644
> index 0000000000..d6879613b6
> --- /dev/null
> +++ b/target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-m901-
> c
> +++ n9131-db.dts
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2019 Marvell International Ltd.
> + *
> + * Device tree for the CN9131-DB board.
> + */
> +
> +#include "puzzle-m901-cn9130-db.dts"

Deriving one dts from another is bad practice. That's what DTSI files are for.

> +
> +/ {
> +	model = "Puzzle-M901";
> +	compatible = "marvell,cn9131", "marvell,cn9130",
> +		     "marvell,armada-ap807-quad", "marvell,armada-ap807";

Why is model and compatible completely different. Please decide for one name and then use it consistently.

> +
> +	aliases {
> +		i2c0 = &cp1_i2c0;
> +		ethernet3 = &cp1_eth0;
> +		ethernet4 = &cp1_eth1;
> +		ethernet5 = &cp1_eth2;
> +		gpio3 = &cp1_gpio1;
> +                gpio4 = &cp1_gpio2;

Indent .............

> diff --git a/target/linux/mvebu/image/cortexa72.mk
> b/target/linux/mvebu/image/cortexa72.mk
> index 1440c07a0b..c04ad2ae9e 100644
> --- a/target/linux/mvebu/image/cortexa72.mk
> +++ b/target/linux/mvebu/image/cortexa72.mk
> @@ -43,3 +43,27 @@ define Device/marvell_macchiatobin-singleshot
>    SUPPORTED_DEVICES := marvell,armada8040-mcbin-singleshot
>  endef
>  TARGET_DEVICES += marvell_macchiatobin-singleshot
> +
> +define Device/marvell_puzzle-m901-cn9131-db
> +  $(call Device/Default-arm64)
> +  DEVICE_VENDOR := iEi
> +  DEVICE_MODEL := Puzzle-M901
> +  DEVICE_DTS := puzzle-m901-cn9131-db

Please name the DTS file properly so the default DEVICE_DTS can be used (and the one in device definition dropped).

> +  IMAGE/sdcard.img.gz := boot-img-ext4 | sdcard-img-ext4 | gzip |
> +append-metadata
> +  IMAGES += factory.bin
> +  IMAGE/factory.bin := append-kernel | pad-to 64k
> +  SUPPORTED_DEVICES :=marvell,cn9131 marvell,puzzle-m901-cn9131-db

One should be enough. It should be the one autoassigned, i.e. the device node name with "," instead of "_".

Best

Adrian 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openpgp-digital-signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.openwrt.org/pipermail/openwrt-devel/attachments/20210607/9217e7a3/attachment.sig>