[OpenWrt-Devel] Added WIZnet WizFi630A Platform based on Ralink RT5350

From: Tobias Welz <tw@wiznet.eu>


Signed-off-by: Tobias Welz <tw@wiznet.eu>
---
  .../linux/ramips/base-files/etc/board.d/02_network |    5 +
  target/linux/ramips/base-files/etc/diag.sh         |    3 +
  target/linux/ramips/base-files/lib/ramips.sh       |    3 +
  .../ramips/base-files/lib/upgrade/platform.sh      |    1 +
  target/linux/ramips/dts/WIZFI630A.dts              |  294 
++++++++++++++++++++
  target/linux/ramips/image/Makefile                 |    4 +-
  target/linux/ramips/rt305x/profiles/wizfi630a.mk   |   18 ++
  7 files changed, 327 insertions(+), 1 deletion(-)
  create mode 100644 target/linux/ramips/dts/WIZFI630A.dts
  create mode 100644 target/linux/ramips/rt305x/profiles/wizfi630a.mk

Hello Tobias,

I saw on patchwork that your patch is already marked as accepted, but
I have a lot of comments, please see them inline, below.

PS. Sorry for being so pedantic, but it took me lot of time to clean
ramips target few weeks ago.

2015-08-17 22:47 GMT+02:00 Tobias Welz <tw@wiznet.eu>:
> From: Tobias Welz <tw@wiznet.eu>

Your patch message should have "ramips:" prefix.
It would be also nice to place a small description about the hardware
you are adding support for and this is the right place for it.

>
>
> Signed-off-by: Tobias Welz <tw@wiznet.eu>
> ---
>  .../linux/ramips/base-files/etc/board.d/02_network |    5 +
>  target/linux/ramips/base-files/etc/diag.sh         |    3 +
>  target/linux/ramips/base-files/lib/ramips.sh       |    3 +
>  .../ramips/base-files/lib/upgrade/platform.sh      |    1 +
>  target/linux/ramips/dts/WIZFI630A.dts              |  294
> ++++++++++++++++++++
>  target/linux/ramips/image/Makefile                 |    4 +-
>  target/linux/ramips/rt305x/profiles/wizfi630a.mk   |   18 ++
>  7 files changed, 327 insertions(+), 1 deletion(-)
>  create mode 100644 target/linux/ramips/dts/WIZFI630A.dts
>  create mode 100644 target/linux/ramips/rt305x/profiles/wizfi630a.mk
>
> diff --git a/target/linux/ramips/base-files/etc/board.d/02_network
> b/target/linux/ramips/base-files/etc/board.d/02_network
> index 6a2f042..cfd0b26 100755
> --- a/target/linux/ramips/base-files/etc/board.d/02_network
> +++ b/target/linux/ramips/base-files/etc/board.d/02_network
> @@ -217,6 +217,7 @@ ramips_setup_interfaces()
>                 ucidef_add_switch_vlan "switch0" "1" "0 1 2 3 6t"
>                 ucidef_add_switch_vlan "switch0" "2" "5 6t"
>                 ;;
> +       wizfi630a | \

Please, follow the general convention in this file and use "|\"
instead of " | \".

>         y1s)
>                 ucidef_set_interfaces_lan_wan "eth0.1" "eth0.2"
>                 ucidef_add_switch "switch0" "1" "1"
> @@ -320,6 +321,10 @@ ramips_setup_macs()
>         wcr-150gn)
>                 wan_mac=$(mtd_get_mac_binary factory 40)
>                 ;;
> +       wizfi630a)
> +               lan_mac=$(mtd_get_mac_binary factory 4)
> +               wan_mac=$(mtd_get_mac_binary factory 40)
> +               ;;

Please, follow the general convention and keep boards in alphabetical
order (board are sorted in two passes: in file and in common blocks):

[...]
wcr-150gn)
[...]
whr-1166d|\
whr-300hp2|\
whr-600d|\
wsr-600)
[...]

--> this is the right place for your board

wsr-1166)
[...]

>         whr-1166d|\
>         whr-300hp2|\
>         whr-600d|\
> diff --git a/target/linux/ramips/base-files/etc/diag.sh
> b/target/linux/ramips/base-files/etc/diag.sh
> index a4911b0..54b84e5 100644
> --- a/target/linux/ramips/base-files/etc/diag.sh
> +++ b/target/linux/ramips/base-files/etc/diag.sh
> @@ -143,6 +143,9 @@ get_status_led() {
>         whr-600d)
>                 status_led="$board:orange:wifi"
>                 ;;
> +       wizfi630a)
> +               status_led="wizfi630a::run"
> +               ;;

Same as above, please keep boards in alphabetical order:

[...]
wcr-150gn|\
wl-351)
[...]
whr-g300n|\
wzr-agl300nh)
[...]

--> this is the right place for your board

wsr-1166|\
wsr-600)

>         rt-n10-plus|\
>         tew-691gr|\
>         tew-692gr|\
> diff --git a/target/linux/ramips/base-files/lib/ramips.sh
> b/target/linux/ramips/base-files/lib/ramips.sh
> index 5cafd45..b6d1023 100755
> --- a/target/linux/ramips/base-files/lib/ramips.sh
> +++ b/target/linux/ramips/base-files/lib/ramips.sh
> @@ -367,6 +367,9 @@ ramips_board_detect() {
>         *"WIZARD 8800")
>                 name="wizard8800"
>                 ;;
> +       *"WIZnet WizFi630A")
> +               name="wizfi630a"
> +               ;;

I don't see any reason here to use whole name of the device, including
manufacturer name (WIZnet WizFi630A) - there are no other similar
models on the list and as you can see we use here *"..." pattern.
Please, follow the general convention in this file and use only
board/model (WizFi630A) name and keep boards in alphabetical order.

>         *"WL-330N")
>                 name="wl-330n"
>                 ;;
> diff --git a/target/linux/ramips/base-files/lib/upgrade/platform.sh
> b/target/linux/ramips/base-files/lib/upgrade/platform.sh
> index 53e7070..947a328 100755
> --- a/target/linux/ramips/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/ramips/base-files/lib/upgrade/platform.sh
> @@ -107,6 +107,7 @@ platform_check_image() {
>         whr-300hp2|\
>         whr-600d|\
>         whr-g300n|\
> +       wizfi630a |\

Please, follow the general convention in this file and use "|\"
instead of " |\".

>         wl-330n|\
>         wl-330n3g|\
>         wl-341v3|\
> diff --git a/target/linux/ramips/dts/WIZFI630A.dts
> b/target/linux/ramips/dts/WIZFI630A.dts
> new file mode 100644
> index 0000000..0d44c71
> --- /dev/null
> +++ b/target/linux/ramips/dts/WIZFI630A.dts
> @@ -0,0 +1,294 @@
>
> +/dts-v1/;
> +
> +/include/ "rt5350.dtsi"
> +
> +/ {
> +       compatible = "wizfi630a", "ralink,rt5350-soc";
> +       model = "WIZnet WizFi630A";
> +
> +       chosen {
> +               bootargs = "console=ttyS1,115200";
> +       };
> +
> +       palmbus@10000000 {
> +               gpio1: gpio@660 {
> +                       status = "okay";
> +               };
> +
> +               spi@b00 {
> +                       status = "okay";
> +
> +                       m25p80@0 {
> +                               #address-cells = <1>;
> +                               #size-cells = <1>;
> +                               compatible = "w25q128";
> +                               reg = <0>;
> +                               linux,modalias = "m25p80", "w25q128";
> +                               spi-max-frequency = <10000000>;
> +
> +                               partition@0 {
> +                                       label = "uboot";
> +                                       reg = <0x0 0x30000>;
> +                                       read-only;
> +                               };
> +
> +                               partition@30000 {
> +                                       label = "uboot-env";
> +                                       reg = <0x30000 0x10000>;
> +                                       read-only;
> +                               };
> +
> +                               factory: partition@40000 {
> +                                       label = "factory";
> +                                       reg = <0x40000 0x10000>;
> +                                       read-only;
> +                               };
> +
> +                               partition@50000 {
> +                                       label = "firmware";
> +                                       reg = <0x50000 0xfb0000>;
> +                               };
> +                       };
> +               };
> +
> +               uart@500 {
> +                       compatible = "ralink,mt7620a-uart",
> "ralink,rt2880-uart", "ns16550a";
> +                       reg = <0x500 0x100>;
> +
> +                       resets = <&rstctrl 12>;
> +                       reset-names = "uart";
> +
> +                       interrupt-parent = <&intc>;
> +                       interrupts = <5>;
> +
> +                       reg-shift = <2>;
> +
> +                       status = "okay";

Are these empty lines really needed here?

> +               };
> +
> +

Keep only one empty line here, please.

> +               uartlite@c00 {
> +                       compatible = "ralink,mt7620a-uart",
> "ralink,rt2880-uart", "ns16550a";
> +                       reg = <0xc00 0x100>;
> +
> +                       resets = <&rstctrl 19>;
> +                       reset-names = "uartl";
> +
> +                       interrupt-parent = <&intc>;
> +                       interrupts = <12>;
> +
> +                       reg-shift = <2>;
> +
> +                       pinctrl-names = "default";
> +                       pinctrl-0 = <&uartlite_pins>;

Are these empty lines really needed here too?

> +               };
> +       };
> +
> +       pinctrl {
> +               state_default: pinctrl0 {
> +                       gpio {
> +                               ralink,group = "i2c", "jtag" ;
> +                               ralink,function = "gpio";
> +                       };
> +               };
> +
> +               uartf_gpio_pins: uartf_gpio {
> +                       uartf_gpio {
> +                               ralink,group = "uartf";
> +                               ralink,function = "uartf";
>
> +                       };
> +               };
> +
> +               uartlite_pins: uartlite {
> +                       uart {
> +                               ralink,group = "uartlite";
> +                               ralink,function = "uartlite";
> +                       };
> +               };
> +       };
> +
> +       ethernet@10100000 {
> +               mtd-mac-address = <&factory 0x4>;
> +       };
> +
> +       esw@10110000 {
> +               ralink,portmap = <0x17>;
> +       };
> +
> +       wmac@10180000 {
> +               ralink,mtd-eeprom = <&factory 0>;
> +       };
> +
> +       ehci@101c0000 {
> +               status = "okay";
> +       };
> +
> +       ohci@101c1000 {
> +               status = "okay";
> +       };
> +
> +       gpio-export {
>
> +               compatible = "gpio-export";
> +               #size-cells = <0>;

Empty line here, please.

> +/*
> +               gpio22 {
> +                       // WAN +                        gpio-export,name =
> "gpio22";

This line looks broken (missing new line after comment line).

> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio1 0 0>; // path lfd dir
> +               };

Empty line here, please.

> +               gpio23 {
> +                       // LAN1 +                       gpio-export,name =
> "gpio23";

This line looks broken (missing new line after comment line).

> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio1 1 0>;
> +               };

Empty line here, please.

> +               gpio24 {
> +                       // LAN2 +                       gpio-export,name =
> "gpio24";

This line looks broken (missing new line after comment line).

> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio1 2 0>;
> +               };
> +
> +               // UARTF - UART1 +              gpio7 {

This line looks broken (missing new line after comment line).

> +                       // UARTF_RTS_N - HW Pin 6
> +                       gpio-export,name = "gpio7";
> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 7 0>;
> +               };

Empty line here, please.

> +               gpio8 {
> +                       // UARTF_TXD - HW Pin 10 +
> gpio-export,name = "gpio8";

This line looks broken (missing new line after comment line).

> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 8 0>;
> +               };

Empty line here, please.

> +               gpio9 {
> +                       // UARTF_CTS_N - HW Pin 5
> +                       gpio-export,name = "gpio9";
> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 9 0>;
> +               };

Empty line here, please.

> +               gpio10 {
> +                       // UARTF_RXD - HW Pin 9
> +                       gpio-export,name = "gpio10";
> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 10 0>;
> +               };

Empty line here, please.

> +               gpio11 {
> +                       // UARTF_DTR_N - HW Pin 8
> +                       gpio-export,name = "gpio11";
> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 11 0>;
> +               };

Empty line here, please.

> +               gpio12 {
> +                       // UARTF_DCD_N - HW Pin 12 +
> gpio-export,name = "gpio12";

This line looks broken (missing new line).

> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 12 0>;
> +               };

Empty line here, please.

> +               gpio13 {
> +                       // UARTF_DSR_N - HW Pin 11
> +                       gpio-export,name = "gpio13";
> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 13 0>;
> +               };

Empty line here, please.

> +               gpio14 {
> +                       // UARTF_RIN - HW Pin 7 +
> gpio-export,name = "gpio14";

This line looks broken (missing new line after comment line).

> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 14 0>;
> +               };
> +
> +               // UARTLITE - UART2 +           gpio15 {

This line looks broken (missing new line after comment line).

> +                       // UART2_TXD  - HW Pin 20 +
> gpio-export,name = "gpio15";

This line looks broken (missing new line after comment line).

> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 15 0>;
> +               };

Empty line here, please.

> +               gpio16 {
> +                       // UART2_RXD  - HW Pin 18 +
> gpio-export,name = "gpio16";

This line looks broken (missing new line after comment line).

> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 16 0>;
> +               };
> +

Move this empty line...

> +*/

... here, please.

> +/*
> +               // If you want to use this comment out the corrosponding
> gpio-keys-polled
> +               gpio0 {
> +                       // wps btn
> +                       gpio-export,name = "gpio0";
> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 0 1>;
> +               };
> +*/
> +
> +               gpio19 {
> +                       // SCM1
> +                       gpio-export,name = "gpio19";
> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 19 1>;
> +               };

Empty line here, please.

> +               gpio2 {
> +                       // SCM2
> +                       gpio-export,name = "gpio2";
> +                       gpio-export,direction_may_change = <1>;
> +                       gpios = <&gpio0 2 1>;
> +               };
> +

This empty line is unnecessary here.

> +       };
> +
> +       gpio-leds {
> +               compatible = "gpio-leds";

Empty line here, please.

> +               run {
> +                       label = "wizfi630a::run";
> +                       gpios = <&gpio0 1 1>;
> +               };

Empty line here, please.

> +               uart1 {
> +                       label = "wizfi630a::uart1";
> +                       gpios = <&gpio0 18 1>;
> +               };

Empty line here, please.

> +               uart2 {
> +                       label = "wizfi630a::uart2";
> +                       gpios = <&gpio0 21 1>;
> +               };

Empty line here, please.

> +               wps {
> +                       label = "wizfi630a::wps";
> +                       gpios = <&gpio0 20 1>;
> +               };
> +       };
> +
> +       gpio-keys-polled {
>
> +               compatible = "gpio-keys-polled";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               poll-interval = <20>;

Empty line here, please.

> +               reset {
> +                       label = "reset";
> +                       gpios = <&gpio0 17 1>;
> +                       linux,code = <0x198>;
> +               };
> +
> +               wps {
> +                       label = "wps";
> +                       gpios = <&gpio0 0 1>;
> +                       linux,code = <0x211>;
> +               };

Empty line here, please.

> +/*
> +               // If you want to use this comment out the corrosponding
> export
> +               scm1 {
> +                       label = "SCM1";
> +                       gpios = <&gpio0 19 1>;
> +                       linux,code = <0x100>;
> +               };
> +
> +               scm2 {
> +                       label = "SCM2";
> +                       gpios = <&gpio0 2 1>;
> +                       linux,code = <0x101>;
> +               };
> +*/
> +       };
> +};
> diff --git a/target/linux/ramips/image/Makefile
> b/target/linux/ramips/image/Makefile
> index 2f4d4cb..ac2e83c 100644
> --- a/target/linux/ramips/image/Makefile
> +++ b/target/linux/ramips/image/Makefile
> @@ -608,7 +608,6 @@ Image/Build/Profile/W502U=$(call
> BuildFirmware/Default8M/$(1),$(1),w502u,W502U)
>   Image/Build/Profile/WCR150GN=$(call
> BuildFirmware/Default4M/$(1),$(1),wcr150gn,WCR150GN)
>  -
>  Image/Build/Profile/MZK-DP150N=$(call
> BuildFirmware/Default4M/$(1),$(1),mzk-dp150n,MZK-DP150N)
>   buffalo_whrg300n_mtd_size=3801088
> @@ -631,6 +630,8 @@ Image/Build/Profile/WHRG300N=$(call
> BuildFirmware/WHRG300N/$(1),$(1))
>   Image/Build/Profile/WIZARD8800=$(call
> BuildFirmware/Default8M/$(1),$(1),wizard-8800,WIZARD8800,Linux Kernel Image)
>  +Image/Build/Profile/WIZFI630A=$(call
> BuildFirmware/Default16M/$(1),$(1),wizfi630a,WIZFI630A)
> +
>  Image/Build/Profile/WL-330N=$(call
> BuildFirmware/Default4M/$(1),$(1),wl-330n,WL-330N)
>   Image/Build/Profile/WL-330N3G=$(call
> BuildFirmware/Default4M/$(1),$(1),wl-330n3g,WL-330N3G)
> @@ -763,6 +764,7 @@ define Image/Build/Profile/Default
>         $(call Image/Build/Profile/WCR150GN,$(1))
>         $(call Image/Build/Profile/WHRG300N,$(1))
>         $(call Image/Build/Profile/WIZARD8800,$(1))
> +       $(call Image/Build/Profile/WIZFI630A,$(1))
>         $(call Image/Build/Profile/WL-330N,$(1))
>         $(call Image/Build/Profile/WL-330N3G,$(1))
>         $(call Image/Build/Profile/WL-341V3,$(1))
> diff --git a/target/linux/ramips/rt305x/profiles/wizfi630a.mk
> b/target/linux/ramips/rt305x/profiles/wizfi630a.mk

This filename is wrong, please follow the general convention and:
- keep all boards from same manufacturer in file "manufacturer.mk" (in
this case: wiznet.mk)
- keep all profiles inside this file in alphabetical order
- follow the general structure, see below

> new file mode 100644
> index 0000000..2d2c001
> --- /dev/null
> +++ b/target/linux/ramips/rt305x/profiles/wizfi630a.mk
> @@ -0,0 +1,18 @@
> +#
> +# Copyright (C) 2014 OpenWrt.org

Shouldn't it be 2015? :)

> +#
> +# This is free software, licensed under the GNU General Public License v2.
> +# See /LICENSE for more information.
> +#
> +
> +define Profile/WIZFI630A
> +       NAME:=WIZnet WizFi630A
> +       PACKAGES:=\
> +               kmod-usb2 +endef
>
> +
> +define Profile/WIZFI630A/Description
> +       Package set for WIZnet WizFi630A board
> +endef
> +

This empty line is unnecessary here.
Please, see other *.mk profile files structure.

> +$(eval $(call Profile,WIZFI630A))
> --
> 1.7.10.4
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel