[LEDE-DEV] [PATCH] ramips: Add support for the Unielec U7621-6

[Mathias Kresin]

Hey Christian,

find my comments inline.

04.11.2017 21:53, Kristian Evensen:
> Notes:
> * According to the specifications on the Unielec website, two LEDs
> should be controllable via GPIO. I was not able to find the pins.

Have you tired to set more/all pinmux groups to function GPIO?

Given the fact that your pincntrl node doesn't look like the usual 
copy'n'paste, I would guess you know what you're doing here and the 
answer is yes.


> --- a/target/linux/ramips/base-files/lib/ramips.sh
> +++ b/target/linux/ramips/base-files/lib/ramips.sh
> @@ -508,6 +508,9 @@ ramips_board_detect() {
>   	*"U25AWF-H1")
>   		name="u25awf-h1"
>   		;;
> +	*"Unielec U7621-6 (256M RAM/16M flash)")
> +		name="u7621-6-256M-16M"
> +		;;
>   	*"UBNT-ERX")
>   		name="ubnt-erx"
>   		;;

Doesn't look like the correct alphabetical order.

> index 0000000000..fff6206e44
> --- /dev/null
> +++ b/target/linux/ramips/dts/U7621-6-256M-16M.dts
> @@ -0,0 +1,54 @@
> +/*
> + *  BSD LICENSE
> + *
> + *  Copyright(c) 2017 Kristian Evensen <kristian.evensen at gmail.com>.
> + *  All rights reserved.
> + *
> + *  Redistribution and use in source and binary forms, with or without
> + *  modification, are permitted provided that the following conditions
> + *  are met:
> + *
> + *    * Redistributions of source code must retain the above copyright
> + *      notice, this list of conditions and the following disclaimer.
> + *    * Redistributions in binary form must reproduce the above copyright
> + *      notice, this list of conditions and the following disclaimer in
> + *      the documentation and/or other materials provided with the
> + *      distribution.
> + *    * Neither the name of Broadcom Corporation nor the names of its
> + *      contributors may be used to endorse or promote products derived
> + *      from this software without specific prior written permission.
> + *
> + *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +/dts-v1/;
> +
> +#include "U7621-6.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +
> +/ {
> +	compatible = "unielec,u7621-6-256m-16m", "unielec,u7621-6", "mediatek,mt7621-soc";
> +	model = "Unielec U7621-6 (256M RAM/16M flash)";
> +
> +	memory at 0 {
> +		device_type = "memory";
> +		reg = <0x0 0x10000000>;
> +	};
> +};
> +
> +&firmware {
> +	reg = <0x50000 0xfb0000>;
> +};

I know, I've done it the same way in the past, but it turned out to be 
really hard to read. I would prefer to have the full spi node in the 
U7621-6-256M-16M.dts.

If someone adds support for more memory/flash size variants, we can create:

- U7621-6.dtsi
- U7621-6-256M-ram.dtsi containing only the memory@ node
- U7621-6-16M-flash.dtsi containing only the spi@ node

and include the files in that order in a U7621-6-256M-16M.dts. This way 
we should be able to cover all variants without any duplications.

> diff --git a/target/linux/ramips/image/mt7621.mk b/target/linux/ramips/image/mt7621.mk
> index 8bd7e0318f..1bdd0024e4 100644
> --- a/target/linux/ramips/image/mt7621.mk
> +++ b/target/linux/ramips/image/mt7621.mk
> @@ -225,6 +225,15 @@ define Device/timecloud
>   endef
>   TARGET_DEVICES += timecloud
>   
> +define Device/u7621-6-256M-16M
> +  DTS := U7621-6-256M-16M
> +  IMAGE_SIZE := 16777216

Looks wrong to me. The firmware partition has a size of 0xfb0000, which 
is 16449536 bytes or 16064k to be better readable.

Mathias