| Message ID | f98b460493f8cc650977db993fdb8c3336ef895e.1595193767.git.sander@svanheule.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ath79: support for TP-Link EAP2x5 1-port devices | expand |
Hi, > -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] > On Behalf Of Sander Vanheule > Sent: Sonntag, 19. Juli 2020 23:24 > To: openwrt-devel@lists.openwrt.org > Cc: Sander Vanheule <sander@svanheule.net>; Julien Dusser > <julien.dusser@free.fr> > Subject: [RFC PATCH v2 3/6] ath79: prepare for 1-port TP-Link EAP2x5 devices > > TP-Link has developed a number of access points based on the AP152 > reference board. In the EAP-series of 802.11ac access points, this includes > the following devices with one ethernet port: > * EAP225 v1/v2 > * EAP225 v3 > * EAP225-Outdoor v1 > * EAP245 v1 > > Since the only differences between these devices are the ath10k wireless > radios and LEDs, a common base is provided for the overlapping support > requirements. > > Hardware commonalities: > * SoC: QCA9563-AL3A MIPS 74kc v5.0 @ 775MHz, AHB @ 258MHz > * RAM: 128MiB DDR2 @ 650MHz > * Flash: 16MiB SPI NOR > * Wi-Fi 2.4GHz: provided by SoC > * Wi-Fi 5Ghz: ath10k chip on PCIe > * Ethernet: AR8033-AL1A, one 1GbE port (802.3at PoE) > > This patch was originally developed by Julien Dusser for the EAP245 v1, and > was adapted by Sander Vanheule to support more devices. > > Signed-off-by: Julien Dusser <julien.dusser@free.fr> > Signed-off-by: Sander Vanheule <sander@svanheule.net> technically, if Julien is first SoB, you should also put him in the author (From:) field. > --- > .../dts/qca9563_tplink_eap2x5_1port.dtsi | 139 ++++++++++++++++++ > target/linux/ath79/image/generic-tp-link.mk | 10 ++ > 2 files changed, 149 insertions(+) > create mode 100644 > target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > diff --git a/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > b/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > new file mode 100644 > index 0000000000..24f0b4f0ce > --- /dev/null > +++ b/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > @@ -0,0 +1,139 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT #include Not really important, but I'd prefer an empty line after the license. (Broken in my mail anyway, but I looked at it in patchwork.) > +<dt-bindings/gpio/gpio.h> #include <dt-bindings/input/input.h> > + > +#include "qca956x.dtsi" > + > +/ { > + aliases { > + label-mac-device = ð0; > + }; > + > + keys { > + compatible = "gpio-keys"; > + > + reset { > + label = "Reset button"; > + linux,code = <KEY_RESTART>; > + gpios = <&gpio 2 GPIO_ACTIVE_LOW>; > + debounce-interval = <60>; > + }; > + }; > +}; > + > +&pcie { > + status = "okay"; > +}; > + > +&uart { > + status = "okay"; > +}; > + > +&spi { > + status = "okay"; > + num-cs = <1>; > + > + flash@0 { > + compatible = "jedec,spi-nor"; > + reg = <0>; > + spi-max-frequency = <25000000>; > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "u-boot"; > + reg = <0x000000 0x020000>; > + read-only; > + }; > + > + partition@20000 { > + label = "partition-table"; > + reg = <0x020000 0x010000>; > + read-only; > + }; > + > + info: partition@30000 { > + label = "info"; > + reg = <0x030000 0x010000>; > + read-only; > + }; > + > + partition@40000 { > + compatible = "openwrt,elf"; > + label = "firmware"; > + reg = <0x040000 0xec0000>; > + }; > + > + partition@f00000 { > + label = "config"; > + reg = <0xf00000 0x030000>; > + read-only; > + }; > + > + partition@f30000 { > + label = "mutil-log"; > + reg = <0xf30000 0x080000>; > + read-only; > + }; > + > + partition@fb0000 { > + label = "oops"; > + reg = <0xfb0000 0x040000>; > + read-only; > + }; > + > + art: partition@ff0000 { > + label = "art"; > + reg = <0xff0000 0x010000>; > + read-only; > + }; > + }; > + }; > +}; > + > +&pinmux { > + mdio_pins: pinux_mdio_pins { Typo for the node name (pinux). Despite, including pinmux for the node name actually doesn't make much sense, as it's part of &pinmux anyway. If you want it, it would make sense to add it to the DT label, as this one is actually used in global context. I'd be fine with "mdio_pins: mdio_pins" as well, though, as "pins" already tells whats going on. > + /* GPIO 8 as MDC(0x21), GPIO 10 as MDIO(0x20) */ > + pinctrl-single,bits = <0x8 0x00000021 0x000000ff>, > + <0x8 0x00200000 0x00ff0000>; Err, is there a specific reason why you don't use: pinctrl-single,bits = <0x8 0x00200021 0x00ff00ff>; Or is the second offset supposed to 0xc or something? > + }; > +}; > + > +&mdio0 { > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&mdio_pins>; > + > + phy-mask = <0x10>; > + > + phy4: ethernet-phy@4 { > + reg = <4>; > + reset-gpios = <&gpio 11 GPIO_ACTIVE_LOW>; > + }; > +}; > + > +ð0 { > + status = "okay"; > + > + phy-handle = <&phy4>; > + phy-mode = "sgmii"; > + > + mtd-mac-address = <&info 0x8>; > + > + qca956x-serdes-fixup; > + > + gmac-config { > + device = <&gmac>; > + }; > +}; > + > +&wmac { > + status = "okay"; > + > + mtd-cal-data = <&art 0x1000>; > + mtd-mac-address = <&info 0x8>; > +}; > diff --git a/target/linux/ath79/image/generic-tp-link.mk > b/target/linux/ath79/image/generic-tp-link.mk > index 8a26e4bebe..d2cc8d09bd 100644 > --- a/target/linux/ath79/image/generic-tp-link.mk > +++ b/target/linux/ath79/image/generic-tp-link.mk > @@ -362,6 +362,16 @@ define Device/tplink_cpe610-v2 endef > TARGET_DEVICES += tplink_cpe610-v2 > > +define Device/tplink_eap2x5_1port > + $(Device/tplink-safeloader) > + SOC := qca9563 > + IMAGE_SIZE := 15104k > + LOADER_TYPE := elf > + KERNEL := kernel-bin | append-dtb | lzma | loader-kernel > + KERNEL_INITRAMFS := $$(KERNEL) > + IMAGE/factory.bin := append-rootfs | tplink-safeloader factory | > +pad-extra 128 endef > + > define Device/tplink_eap245-v3 This packaging of patches into different projects really makes it hard to review. But anyway, limiting the device definition to 1port doesn't make much sense conceptually (in contrast to the DTS, were it is useful). I'd go for tplink_eap2x5 here and already use it for the eap245-v3 in this patch. Only IMAGE_SIZE would need to be moved to the devices then, and that's better anyway. Best Adrian > $(Device/tplink-safeloader) > SOC := qca9563 > -- > 2.26.2 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Hi Adrian, On Mon, 2020-07-20 at 00:25 +0200, mail@adrianschmutzler.de wrote: > Hi, > > > Signed-off-by: Julien Dusser <julien.dusser@free.fr> > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > > technically, if Julien is first SoB, you should also put him in the > author (From:) field. The DTSI is derived from Julien's DTS for the EAP245 v1 [1]. By now I've made so many modifications, that I don't think Julien should be the person responsible for this file any more. But I would still like to credit his work. I'll swap the SoB order to reflect this. [1] https://github.com/j-d-r/openwrt/blob/ae7dc9bf871cd9f27ccc1f4bff335ab5e79bcae9/target/linux/ath79/dts/qca9563_tplink_eap245-v1.dts > > --- > > .../dts/qca9563_tplink_eap2x5_1port.dtsi | 139 > > ++++++++++++++++++ > > target/linux/ath79/image/generic-tp-link.mk | 10 ++ > > 2 files changed, 149 insertions(+) > > create mode 100644 > > target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > > > diff --git > > a/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > b/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > new file mode 100644 > > index 0000000000..24f0b4f0ce > > --- /dev/null > > +++ b/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > @@ -0,0 +1,139 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT #include > > Not really important, but I'd prefer an empty line after the license. > (Broken in my mail anyway, but I looked at it in patchwork.) Fixed. > > +&pinmux { > > + mdio_pins: pinux_mdio_pins { > > Typo for the node name (pinux). Despite, including pinmux for the > node name actually doesn't make much sense, as it's part of &pinmux > anyway. > If you want it, it would make sense to add it to the DT label, as > this one is actually used in global context. > > I'd be fine with "mdio_pins: mdio_pins" as well, though, as "pins" > already tells whats going on. I went with your suggestion and changed it to "mdio_pins: mdio_pins". > > + /* GPIO 8 as MDC(0x21), GPIO 10 as MDIO(0x20) */ > > + pinctrl-single,bits = <0x8 0x00000021 0x000000ff>, > > + <0x8 0x00200000 0x00ff0000>; > > Err, is there a specific reason why you don't use: > > pinctrl-single,bits = <0x8 0x00200021 0x00ff00ff>; > > Or is the second offset supposed to 0xc or something? The lines are correct and entirely as intended. But I just didn't think about merging the two lines... *facepalm* Merged, and swapped the order in the comment to 10/8 to have the same order as in the specified bits. > > diff --git a/target/linux/ath79/image/generic-tp-link.mk > > b/target/linux/ath79/image/generic-tp-link.mk > > index 8a26e4bebe..d2cc8d09bd 100644 > > --- a/target/linux/ath79/image/generic-tp-link.mk > > +++ b/target/linux/ath79/image/generic-tp-link.mk > > @@ -362,6 +362,16 @@ define Device/tplink_cpe610-v2 endef > > TARGET_DEVICES += tplink_cpe610-v2 > > > > +define Device/tplink_eap2x5_1port > > + $(Device/tplink-safeloader) > > + SOC := qca9563 > > + IMAGE_SIZE := 15104k > > + LOADER_TYPE := elf > > + KERNEL := kernel-bin | append-dtb | lzma | loader-kernel > > + KERNEL_INITRAMFS := $$(KERNEL) > > + IMAGE/factory.bin := append-rootfs | tplink-safeloader factory | > > +pad-extra 128 endef > > + > > define Device/tplink_eap245-v3 > > This packaging of patches into different projects really makes it > hard to review. > > But anyway, limiting the device definition to 1port doesn't make much > sense conceptually (in contrast to the DTS, were it is useful). > > I'd go for tplink_eap2x5 here and already use it for the eap245-v3 in > this patch. > > Only IMAGE_SIZE would need to be moved to the devices then, and > that's better anyway. > Sorry for making this harder than it should be. I didn't want to duplicate the patches already on GitHub, and is one of the reasons I submitted this to the list as an RFC, without the complete patch set. If it's okay for you (and ynezz) I will add these patches to the EAP245 v3 pull request for the review (and change the topic and cover letter accordingly, merge the duplicated makefile code). Best, Sander
> -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] > On Behalf Of Sander Vanheule > Sent: Montag, 20. Juli 2020 18:28 > To: mail@adrianschmutzler.de > Cc: ynezz@true.cz; openwrt-devel@lists.openwrt.org; Julien Dusser > <julien.dusser@free.fr> > Subject: Re: [RFC PATCH v2 3/6] ath79: prepare for 1-port TP-Link EAP2x5 > devices > > Hi Adrian, > > On Mon, 2020-07-20 at 00:25 +0200, mail@adrianschmutzler.de wrote: > > Hi, > > > > > Signed-off-by: Julien Dusser <julien.dusser@free.fr> > > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > > > > technically, if Julien is first SoB, you should also put him in the > > author (From:) field. > > The DTSI is derived from Julien's DTS for the EAP245 v1 [1]. By now I've made > so many modifications, that I don't think Julien should be the person > responsible for this file any more. But I would still like to credit his work. I'll > swap the SoB order to reflect this. Swapping is not such a good idea IMO. The Series of SoB statements (and possibly Co-developed-by) should essentially give the history of what happened to a patch from top to bottom. However, in that particular case you are not reusing/updating a patch from Julien, but you are just using something he "invented" for a different purpose. In that context, adding his Signed-off-by is actually wrong if you are strict, unless he has explicit provided it for this subject. So, for this _separate_ subject - adding a DTSI for 1-port devices - I personally think it should just bear your name. Julien will receive credit for the EAP245 v1 device support. Same argument goes for similar cases in your patchset I'm currently not aware of: If the initial patch for the subject at hand is from Julien, he should be author and first SoB. If you picked up something he did and applied it to a different purpose, you are the author. You may mentioned him in the commit message text then (if necessary, for the DTSI I wouldn't even consider that necessary). Best Adrian > > [1] https://github.com/j-d- > r/openwrt/blob/ae7dc9bf871cd9f27ccc1f4bff335ab5e79bcae9/target/linux/a > th79/dts/qca9563_tplink_eap245-v1.dts > > > > > --- > > > .../dts/qca9563_tplink_eap2x5_1port.dtsi | 139 > > > ++++++++++++++++++ > > > target/linux/ath79/image/generic-tp-link.mk | 10 ++ > > > 2 files changed, 149 insertions(+) > > > create mode 100644 > > > target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > > > > > diff --git > > > a/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > > b/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > > new file mode 100644 > > > index 0000000000..24f0b4f0ce > > > --- /dev/null > > > +++ b/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi > > > @@ -0,0 +1,139 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT #include > > > > Not really important, but I'd prefer an empty line after the license. > > (Broken in my mail anyway, but I looked at it in patchwork.) > > Fixed. > > > > > +&pinmux { > > > + mdio_pins: pinux_mdio_pins { > > > > Typo for the node name (pinux). Despite, including pinmux for the node > > name actually doesn't make much sense, as it's part of &pinmux anyway. > > If you want it, it would make sense to add it to the DT label, as this > > one is actually used in global context. > > > > I'd be fine with "mdio_pins: mdio_pins" as well, though, as "pins" > > already tells whats going on. > > I went with your suggestion and changed it to "mdio_pins: mdio_pins". > > > > > + /* GPIO 8 as MDC(0x21), GPIO 10 as MDIO(0x20) */ > > > + pinctrl-single,bits = <0x8 0x00000021 0x000000ff>, > > > + <0x8 0x00200000 0x00ff0000>; > > > > Err, is there a specific reason why you don't use: > > > > pinctrl-single,bits = <0x8 0x00200021 0x00ff00ff>; > > > > Or is the second offset supposed to 0xc or something? > > The lines are correct and entirely as intended. But I just didn't think about > merging the two lines... *facepalm* Merged, and swapped the order in the > comment to 10/8 to have the same order as in the specified bits. > > > > > diff --git a/target/linux/ath79/image/generic-tp-link.mk > > > b/target/linux/ath79/image/generic-tp-link.mk > > > index 8a26e4bebe..d2cc8d09bd 100644 > > > --- a/target/linux/ath79/image/generic-tp-link.mk > > > +++ b/target/linux/ath79/image/generic-tp-link.mk > > > @@ -362,6 +362,16 @@ define Device/tplink_cpe610-v2 endef > > > TARGET_DEVICES += tplink_cpe610-v2 > > > > > > +define Device/tplink_eap2x5_1port > > > + $(Device/tplink-safeloader) > > > + SOC := qca9563 > > > + IMAGE_SIZE := 15104k > > > + LOADER_TYPE := elf > > > + KERNEL := kernel-bin | append-dtb | lzma | loader-kernel > > > + KERNEL_INITRAMFS := $$(KERNEL) > > > + IMAGE/factory.bin := append-rootfs | tplink-safeloader factory | > > > +pad-extra 128 endef > > > + > > > define Device/tplink_eap245-v3 > > > > This packaging of patches into different projects really makes it hard > > to review. > > > > But anyway, limiting the device definition to 1port doesn't make much > > sense conceptually (in contrast to the DTS, were it is useful). > > > > I'd go for tplink_eap2x5 here and already use it for the eap245-v3 in > > this patch. > > > > Only IMAGE_SIZE would need to be moved to the devices then, and that's > > better anyway. > > > > Sorry for making this harder than it should be. I didn't want to duplicate the > patches already on GitHub, and is one of the reasons I submitted this to the > list as an RFC, without the complete patch set. > > If it's okay for you (and ynezz) I will add these patches to the EAP245 > v3 pull request for the review (and change the topic and cover letter > accordingly, merge the duplicated makefile code). > > Best, > Sander > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff --git a/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi b/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi new file mode 100644 index 0000000000..24f0b4f0ce --- /dev/null +++ b/target/linux/ath79/dts/qca9563_tplink_eap2x5_1port.dtsi @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/input/input.h> + +#include "qca956x.dtsi" + +/ { + aliases { + label-mac-device = ð0; + }; + + keys { + compatible = "gpio-keys"; + + reset { + label = "Reset button"; + linux,code = <KEY_RESTART>; + gpios = <&gpio 2 GPIO_ACTIVE_LOW>; + debounce-interval = <60>; + }; + }; +}; + +&pcie { + status = "okay"; +}; + +&uart { + status = "okay"; +}; + +&spi { + status = "okay"; + num-cs = <1>; + + flash@0 { + compatible = "jedec,spi-nor"; + reg = <0>; + spi-max-frequency = <25000000>; + + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "u-boot"; + reg = <0x000000 0x020000>; + read-only; + }; + + partition@20000 { + label = "partition-table"; + reg = <0x020000 0x010000>; + read-only; + }; + + info: partition@30000 { + label = "info"; + reg = <0x030000 0x010000>; + read-only; + }; + + partition@40000 { + compatible = "openwrt,elf"; + label = "firmware"; + reg = <0x040000 0xec0000>; + }; + + partition@f00000 { + label = "config"; + reg = <0xf00000 0x030000>; + read-only; + }; + + partition@f30000 { + label = "mutil-log"; + reg = <0xf30000 0x080000>; + read-only; + }; + + partition@fb0000 { + label = "oops"; + reg = <0xfb0000 0x040000>; + read-only; + }; + + art: partition@ff0000 { + label = "art"; + reg = <0xff0000 0x010000>; + read-only; + }; + }; + }; +}; + +&pinmux { + mdio_pins: pinux_mdio_pins { + /* GPIO 8 as MDC(0x21), GPIO 10 as MDIO(0x20) */ + pinctrl-single,bits = <0x8 0x00000021 0x000000ff>, + <0x8 0x00200000 0x00ff0000>; + }; +}; + +&mdio0 { + status = "okay"; + + pinctrl-names = "default"; + pinctrl-0 = <&mdio_pins>; + + phy-mask = <0x10>; + + phy4: ethernet-phy@4 { + reg = <4>; + reset-gpios = <&gpio 11 GPIO_ACTIVE_LOW>; + }; +}; + +ð0 { + status = "okay"; + + phy-handle = <&phy4>; + phy-mode = "sgmii"; + + mtd-mac-address = <&info 0x8>; + + qca956x-serdes-fixup; + + gmac-config { + device = <&gmac>; + }; +}; + +&wmac { + status = "okay"; + + mtd-cal-data = <&art 0x1000>; + mtd-mac-address = <&info 0x8>; +}; diff --git a/target/linux/ath79/image/generic-tp-link.mk b/target/linux/ath79/image/generic-tp-link.mk index 8a26e4bebe..d2cc8d09bd 100644 --- a/target/linux/ath79/image/generic-tp-link.mk +++ b/target/linux/ath79/image/generic-tp-link.mk @@ -362,6 +362,16 @@ define Device/tplink_cpe610-v2 endef TARGET_DEVICES += tplink_cpe610-v2 +define Device/tplink_eap2x5_1port + $(Device/tplink-safeloader) + SOC := qca9563 + IMAGE_SIZE := 15104k + LOADER_TYPE := elf + KERNEL := kernel-bin | append-dtb | lzma | loader-kernel + KERNEL_INITRAMFS := $$(KERNEL) + IMAGE/factory.bin := append-rootfs | tplink-safeloader factory | pad-extra 128 +endef + define Device/tplink_eap245-v3 $(Device/tplink-safeloader) SOC := qca9563