Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <84b31c59-81d3-c83d-ece9-a120b1cdcdd7@arinc9.com>
Date:   Fri, 12 May 2023 08:40:56 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] mips: dts: ralink: Add support for TP-Link HC220 G5 v1
 board.
To:     Liviu Dudau <liviu@dudau.co.uk>
Cc:     Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        Paul Burton <paulburton@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Sergio Paracuellos <sergio.paracuellos@gmail.com>,
        linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
References: <20230509200125.309026-1-liviu@dudau.co.uk>
 <43301707-8763-2a9f-956d-1ea0ae004a56@arinc9.com>
 <ZF0RxoLRyl78s8sf@bart.dudau.co.uk>
Content-Language: en-US
From:   =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>
In-Reply-To: <ZF0RxoLRyl78s8sf@bart.dudau.co.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 11.05.2023 18:03, Liviu Dudau wrote:
> On Thu, May 11, 2023 at 03:41:30PM +0200, Arınç ÜNAL wrote:
>> On 9.05.2023 22:01, Liviu Dudau wrote:
>>> This WiFi AP is based on a MT7621 SoC with 128MiB RAM, 128MiB NAND,
>>> a MT7603 2.4GHz WiFi and a MT7663 5GHz WiFi chips integrated on the board,
>>> connected to the main SoC over PCIe.
>>>
>>> The GMAC1 on the SoC is connected to PHY0 on the GSW and can be used to
>>> improve routing bandwidth.
>>>
>>> The device uses NMBM over NAND, which is not currently supported in the
>>> mainline, so NAND node is skipped in this revision.
>>>
>>> Signed-off-by: Liviu Dudau <liviu@dudau.co.uk>
>>
>> This is great to see. I'm going to mainline all the MT7621 devicetrees on
>> OpenWrt at some point, this is a good step for this.
> 
> AFAIK this board is not supported by OpenWrt at all. So the flow will be
> the other way this time :)
> 
>>
>>> ---
>>>    arch/mips/boot/dts/ralink/Makefile            |   3 +-
>>>    .../dts/ralink/mt7621-tplink-hc220_g5.dts     | 126 ++++++++++++++++++
>>>    2 files changed, 128 insertions(+), 1 deletion(-)
>>>    create mode 100644 arch/mips/boot/dts/ralink/mt7621-tplink-hc220_g5.dts
>>>
>>> diff --git a/arch/mips/boot/dts/ralink/Makefile b/arch/mips/boot/dts/ralink/Makefile
>>> index 11732b8c8163a..c57a2b40876b4 100644
>>> --- a/arch/mips/boot/dts/ralink/Makefile
>>> +++ b/arch/mips/boot/dts/ralink/Makefile
>>> @@ -8,6 +8,7 @@ dtb-$(CONFIG_DTB_VOCORE2)	+= vocore2.dtb
>>>    dtb-$(CONFIG_SOC_MT7621) += \
>>>    	mt7621-gnubee-gb-pc1.dtb \
>>> -	mt7621-gnubee-gb-pc2.dtb
>>> +	mt7621-gnubee-gb-pc2.dtb \
>>> +	mt7621-tplink-hc220_g5.dtb
>>>    obj-$(CONFIG_BUILTIN_DTB)	+= $(addsuffix .o, $(dtb-y))
>>> diff --git a/arch/mips/boot/dts/ralink/mt7621-tplink-hc220_g5.dts b/arch/mips/boot/dts/ralink/mt7621-tplink-hc220_g5.dts
>>> new file mode 100644
>>> index 0000000000000..83d15711907d0
>>> --- /dev/null
>>> +++ b/arch/mips/boot/dts/ralink/mt7621-tplink-hc220_g5.dts
>>> @@ -0,0 +1,126 @@
>>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +/dts-v1/;
>>> +
>>> +#include "mt7621.dtsi"
>>> +
>>> +#include <dt-bindings/gpio/gpio.h>
>>> +#include <dt-bindings/input/input.h>
>>> +#include <dt-bindings/leds/common.h>
>>> +
>>> +/ {
>>> +	compatible = "tplink,hc220-g5", "mediatek,mt7621-soc";
>>
>> tplink,hc220-g5-v1 should fit better. Also please make another patch to add
>> the compatible string under Boards with Mediatek/Ralink MT7621 SoC on
>> Documentation/devicetree/bindings/mips/ralink.yaml.
> 
> Will do another patch with the documentation update. I will also send a v2
> incorporating all your review comments, thanks for taking the time!
> 
> Regarding the full name: I'm quite tempted to drop the v1(.0) entirely as I'm
> not really sure that TP-Link will release another board. Doing a search now
> I fail to find on any TP-Link results the mention of the version.

What I understand is TP-Link always call their first hardware release v1 
as they are notorious for introducing new versions to cut costs as much 
as possible. On the downloads website for the product, "v1" is there on 
the product name.

https://www.tp-link.com/us/support/download/hc220-g5/

> 
>>
>>> +	model = "TP-Link HC220 G5 v1.0";
>>
>> "TP-Link HC220 G5 v1" should be enough.
>>
>>> +
>>> +	memory@0 {
>>> +		device_type = "memory";
>>> +		reg = <0x0 0x0 0x0 0x8000000>;
>>> +	};
>>> +
>>> +	chosen {
>>> +		/* bootargs = "earlycon=uart8260,mmio32,0x1e000c00 root=/dev/ram0 kgdboc=ttyS0,115200 ip=192.168.88.1:192.168.88.2:::hc220:eth1:none kgdbcon console=ttyS0,115200"; */
>>> +		/* bootargs = "console=ttyS0,115200 earlycon=uart8260,mmio32,0x1e000c00 root=/dev/ram0"; */
>>> +		bootargs = "console=ttyS0,115200 root=/dev/nfs ip=192.168.88.2:192.168.88.5::255.255.255.0:hc220_g5:eth1:none nfsroot=192.168.88.5:/mips,vers=4,sec=sys ro rootwait";
>>> +	};
>>> +
>>> +	gpio-keys {
>>> +		compatible = "gpio-keys";
>>> +
>>> +		key-reset {
>>> +			label = "reset";
>>> +			gpios = <&gpio 8 GPIO_ACTIVE_LOW>;
>>> +			linux,code = <KEY_RESTART>;
>>> +		};
>>> +
>>> +		key-wps {
>>> +			label = "wps";
>>> +			gpios = <&gpio 16 GPIO_ACTIVE_LOW>;
>>> +			linux,code = <KEY_WPS_BUTTON>;
>>> +		};
>>> +	};
>>> +
>>> +	leds {
>>> +		compatible = "gpio-leds";
>>> +
>>> +		red {
>>> +			color = <LED_COLOR_ID_RED>;
>>> +			function = LED_FUNCTION_FAULT;
>>> +			gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
>>> +		};
>>> +
>>> +		green {
>>> +			color = <LED_COLOR_ID_GREEN>;
>>> +			function = LED_FUNCTION_POWER;
>>> +			gpios = <&gpio 14 GPIO_ACTIVE_HIGH>;
>>> +			linux,default-trigger = "default-on";
>>> +		};
>>> +
>>> +		blue {
>>> +			color = <LED_COLOR_ID_BLUE>;
>>> +			function = LED_FUNCTION_WPS;
>>> +			gpios = <&gpio 15 GPIO_ACTIVE_HIGH>;
>>> +		};
>>> +	};
>>> +
>>> +	resetc: reset-controller {
>>> +		compatible = "ralink,rt2880-reset";
>>> +		#reset-cells = <1>;
>>> +	};
>>> +
>>> +	mtd {
>>> +		compatible = "mediatek,mt7622-nfc";
>>> +	};
>>> +};
>>> +
>>> +&i2c {
>>> +	status = "okay";
>>> +};
>>> +
>>> +&pcie {
>>> +	status = "okay";
>>> +};
>>> +
>>> +&spi0 {
>>> +	status = "okay";
>>> +
>>> +	flash@0 {
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +		compatible = "jedec,spi-nor";
>>> +		reg = <0>;
>>> +		spi-max-frequency = <50000000>;
>>> +	};
>>> +};
>>> +
>>> +/* gmac1 connected to MT7530's phy0 */
>>> +&gmac1 {
>>> +	status = "okay";
>>> +	phy-handle = <&ethphy0>;
>>> +};
>>> +
>>> +&mdio {
>>> +	/* MT7530's phy0 */
>>> +	ethphy0: ethernet-phy@0 {
>>> +		reg = <0>;
>>> +		phy-mode = "rgmii";
>>> +	};
>>> +};
>>
>> These are partially wrong, check the mt7621.dtsi on mainline. Or better,
>> don't do it. I'm very close to adding support for changing the DSA conduit
>> for user ports. I suggest you just add port@0 to the DSA switch node below.
>>
> 
> I don't claim to be an expert on DSA or how MT7530 connects to MT7621, however
> I do remember trying various combinations and failing to make all the ports
> work. I blame TP-Link on having a weird setup, but to be sure I will try your
> suggestion if I manage to understand exactly where I'm "partially wrong" :)

You're not wrong, the bindings are. You must disable fixed-link. The 
properties status okay and phy-mode rgmii on ethphy0 is not necessary so 
these can be dropped. Take a look at the GB-PC2 bindings, rgmii-rxid is 
specific to GB-PC2 so no need to do that:

https://github.com/torvalds/linux/blob/master/arch/mips/boot/dts/ralink/mt7621-gnubee-gb-pc2.dts#L114

If you define the port under the DSA switch node, it should just work.

Arınç