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

Sun Nov 5 04:47:19 PST 2017

Hi Mathias,

Thanks for your comments.

On Sun, Nov 5, 2017 at 12:14 PM, Mathias Kresin <dev at kresin.me> wrote:
> 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.

I hope I know what I am doing at least :) The factory firmware
supports exporting all pins, so I checked each one and the only one
that triggered a reaction was 16. I also checked the different LEDs in
/sys/class/led. 18 was verified using the bootloader.

>> --- 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.

Thanks for pointing this out. I had originally left "Unielec" out of
the model and forgot to update this file when I added the name.

>
>
>> 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.

Good idea for structure. I will move the SPI-node to the dts-file.

>> 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.

Thanks, a good old C&P error.

By the way, I sometimes see the board cough up different
memory-related errors. For example, I have seen seg. faults for
applications that are run on startup, messages like "CPU 3 Unable to
handle kernel paging request at virtual address 1fffff9c, epc ==
80109d2c, ra == 8010a548" and "page dumped because: nonzero _count". I
have compared the memory layout, etc. of LEDE to that of the factory
firmware and it all seems to match up. Do you have any ideas what
might be wrong? I did a search and found some similar ramips-related
issues, but they all seem to have dealt with.

The board has always recovered by itself though, so I will apply the
comments and and submit a V2.

-Kristian