diff mbox series

[1/2] realtek: Add generic zyxel_gs1900 image definition

Message ID 20210224224331.1019961-1-hauke@hauke-m.de
State Accepted
Delegated to: Hauke Mehrtens
Headers show
Series [1/2] realtek: Add generic zyxel_gs1900 image definition | expand

Commit Message

Hauke Mehrtens Feb. 24, 2021, 10:43 p.m. UTC 
Add a new common device definition for the Zyxel GS1900 line of
switches.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 target/linux/realtek/image/Makefile | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Comments

Stefan Lippers-Hollmann Feb. 25, 2021, 6:56 a.m. UTC | #1 
Hi

On 2021-02-24, Hauke Mehrtens wrote:
> Add a new common device definition for the Zyxel GS1900 line of
> switches.
[...]
> -define Device/zyxel_gs1900-10hp
> +define Device/zyxel_gs1900
>    SOC := rtl8380
>    IMAGE_SIZE := 6976k
>    DEVICE_VENDOR := ZyXEL
>    DEVICE_MODEL := GS1900-10HP
>    UIMAGE_MAGIC := 0x83800000
> +  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI | uImage gzip
> +endef

I'm wondering if this attempt to deal the gs1900 switch family really
improves the situation for these devices. While IMAGE_SIZE and
UIMAGE_MAGIC might indeed by rather generic to most (all?) members of
the gs1900 family, SOC might not be (GS1900-24, GS1900-24E, GS1900-24HP
are RTL8382M, GS1900-48 and GS1900-48HP are RTL8393 - admittedly, I do
not know how different the resulting DTS would need to be, as these
devices are not supported yet) and zyxel-vers is different for every
single model (aside from GS1900-8HPv1 && GS1900-8HPv2):

GS1900-8:	AAHH
GS1900-8HPv1:	AAHI
GS1900-8HPv2:	AAHI
GS1900-10HP:	AAZI
GS1900-16:	AAHJ
GS1900-24:	AAHL
GS1900-24E:	AAHK
GS1900-24EP:	ABTO
GS1900-24HP:	AAHM
GS1900-24HPv2:	ABTP
GS1900-48:	AAHN
GS1900-48HP:	AAHO
GS1900-48HPv2:	ABTQ

Most of these should be supportable by OpenWrt.

Regards
	Stefan Lippers-Hollmann
Birger Koblitz Feb. 25, 2021, 7:36 a.m. UTC | #2 
Hi,

On 25.02.21 07:56, Stefan Lippers-Hollmann wrote:
> Hi
>
> I'm wondering if this attempt to deal the gs1900 switch family really
> improves the situation for these devices. While IMAGE_SIZE and
> UIMAGE_MAGIC might indeed by rather generic to most (all?) members of
At least for the GS1900-48 the UIMAGE_MAGIC is indeed still  0x83800000
and not 0x839whatever because of the RTL8393 SoC.

> the gs1900 family, SOC might not be (GS1900-24, GS1900-24E, GS1900-24HP
> are RTL8382M, GS1900-48 and GS1900-48HP are RTL8393 - admittedly, I do
> not know how different the resulting DTS would need to be, as these
> devices are not supported yet) and zyxel-vers is different for every
> single model (aside from GS1900-8HPv1 && GS1900-8HPv2):
The .dts for each device are very similar, they only differ in the number of
ports and whether there are SFP ports, but they need to include a SoC
specific .dtsi for small differences like CPU-speed. These changes might become
bigger in the future however, because upstream seems to favor explicit
definitions in the .dtsi over auto-detection in the drivers. Also we might need
to use regmap to map registers whose addresses seem to get randomly shuffled
for the Ethernt and Switch functionality between SoC revisions. At the moment
this is handled by SoC-specific function and register address tables in the drivers,
but with regmap they would go into the SoC .dtsi.

Birger
Adrian Schmutzler Feb. 25, 2021, 10:10 a.m. UTC | #3 
Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Stefan Lippers-Hollmann
> Sent: Donnerstag, 25. Februar 2021 07:56
> To: Hauke Mehrtens <hauke@hauke-m.de>
> Cc: openwrt-devel@lists.openwrt.org
> Subject: Re: [PATCH 1/2] realtek: Add generic zyxel_gs1900 image definition
> 
> Hi
> 
> On 2021-02-24, Hauke Mehrtens wrote:
> > Add a new common device definition for the Zyxel GS1900 line of
> > switches.
> [...]
> > -define Device/zyxel_gs1900-10hp
> > +define Device/zyxel_gs1900
> >    SOC := rtl8380
> >    IMAGE_SIZE := 6976k
> >    DEVICE_VENDOR := ZyXEL
> >    DEVICE_MODEL := GS1900-10HP
> >    UIMAGE_MAGIC := 0x83800000
> > +  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers
> > +AAHI | uImage gzip endef
> 
> I'm wondering if this attempt to deal the gs1900 switch family really improves
> the situation for these devices. While IMAGE_SIZE and UIMAGE_MAGIC
> might indeed by rather generic to most (all?) members of the gs1900 family,
> SOC might not be (GS1900-24, GS1900-24E, GS1900-24HP are RTL8382M,
> GS1900-48 and GS1900-48HP are RTL8393 - admittedly, I do not know how
> different the resulting DTS would need to be, as these devices are not
> supported yet) and zyxel-vers is different for every single model (aside from
> GS1900-8HPv1 && GS1900-8HPv2):

Common definitions in Makefiles and DTS are two different questions. It's not uncommon that shared nodes in Makefile are possible where DTSes are still individual. Anyway, one could make zyxel-vers a variable and move back SOC to the devices when necessary:

DEVICE_VARS += ZYXEL_VERS

define Device/zyxel_gs1900
   IMAGE_SIZE := 6976k
   DEVICE_VENDOR := ZyXEL
   UIMAGE_MAGIC := 0x83800000
   KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers $$$$(ZYXEL_VERS) 
	| uImage gzip
endef

define Device/zyxel_gs1900-10hp
   $(Device/zyxel_gs1900)
   SOC := rtl8380
   DEVICE_MODEL := GS1900-10HP
   ZYXEL_VERS := AAZI
endef
TARGET_DEVICES += zyxel_gs1900-10hp

That would still keep the kernel_initramfs recipe in the common node. Of course, this might eventually hit a point where having the common node is more effort than benefit.

Best

Adrian Schmutzler

> 
> GS1900-8:	AAHH
> GS1900-8HPv1:	AAHI
> GS1900-8HPv2:	AAHI
> GS1900-10HP:	AAZI
> GS1900-16:	AAHJ
> GS1900-24:	AAHL
> GS1900-24E:	AAHK
> GS1900-24EP:	ABTO
> GS1900-24HP:	AAHM
> GS1900-24HPv2:	ABTP
> GS1900-48:	AAHN
> GS1900-48HP:	AAHO
> GS1900-48HPv2:	ABTQ
> 
> Most of these should be supportable by OpenWrt.
> 
> Regards
> 	Stefan Lippers-Hollmann
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Adrian Schmutzler Feb. 25, 2021, 10:11 a.m. UTC | #4 
Hi Hauke,

> 
> -define Device/zyxel_gs1900-10hp
> +define Device/zyxel_gs1900
>    SOC := rtl8380
>    IMAGE_SIZE := 6976k
>    DEVICE_VENDOR := ZyXEL
>    DEVICE_MODEL := GS1900-10HP

DEVICE_MODEL should be removed in the shared node. You put it in the individual nodes below.

Best

Adrian

>    UIMAGE_MAGIC := 0x83800000
> +  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI
> +| uImage gzip endef
> +
> +define Device/zyxel_gs1900-10hp
> +  $(Device/zyxel_gs1900)
> +  DEVICE_MODEL := GS1900-10HP
>    KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAZI |
> uImage gzip  endef  TARGET_DEVICES += zyxel_gs1900-10hp
> 
>  define Device/zyxel_gs1900-8hp-v1
> -  SOC := rtl8380
> -  IMAGE_SIZE := 6976k
> -  DEVICE_VENDOR := ZyXEL
> +  $(Device/zyxel_gs1900)
>    DEVICE_MODEL := GS1900-8HP
>    DEVICE_VARIANT := v1
>    DEVICE_PACKAGES += lua-rs232
> -  UIMAGE_MAGIC := 0x83800000
> -  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI |
> uImage gzip  endef  TARGET_DEVICES += zyxel_gs1900-8hp-v1
> 
>  define Device/zyxel_gs1900-8hp-v2
> -  SOC := rtl8380
> -  IMAGE_SIZE := 6976k
> -  DEVICE_VENDOR := ZyXEL
> +  $(Device/zyxel_gs1900)
>    DEVICE_MODEL := GS1900-8HP
>    DEVICE_VARIANT := v2
>    DEVICE_PACKAGES += lua-rs232
> -  UIMAGE_MAGIC := 0x83800000
> -  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI |
> uImage gzip  endef  TARGET_DEVICES += zyxel_gs1900-8hp-v2
> 
> --
> 2.30.0
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Bjørn Mork Feb. 25, 2021, 5:10 p.m. UTC | #5 
Stefan Lippers-Hollmann <s.l-h@gmx.de> writes:
> On 2021-02-24, Hauke Mehrtens wrote:
>> Add a new common device definition for the Zyxel GS1900 line of
>> switches.
> [...]
>> -define Device/zyxel_gs1900-10hp
>> +define Device/zyxel_gs1900
>>    SOC := rtl8380
>>    IMAGE_SIZE := 6976k
>>    DEVICE_VENDOR := ZyXEL
>>    DEVICE_MODEL := GS1900-10HP
>>    UIMAGE_MAGIC := 0x83800000
>> +  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI | uImage gzip
>> +endef
>
> I'm wondering if this attempt to deal the gs1900 switch family really
> improves the situation for these devices. While IMAGE_SIZE and
> UIMAGE_MAGIC might indeed by rather generic to most (all?) members of
> the gs1900 family, SOC might not be (GS1900-24, GS1900-24E, GS1900-24HP
> are RTL8382M, GS1900-48 and GS1900-48HP are RTL8393 - admittedly, I do
> not know how different the resulting DTS would need to be, as these
> devices are not supported yet) and zyxel-vers is different for every
> single model (aside from GS1900-8HPv1 && GS1900-8HPv2):
>
> GS1900-8:	AAHH
> GS1900-8HPv1:	AAHI
> GS1900-8HPv2:	AAHI
> GS1900-10HP:	AAZI
> GS1900-16:	AAHJ
> GS1900-24:	AAHL
> GS1900-24E:	AAHK
> GS1900-24EP:	ABTO
> GS1900-24HP:	AAHM
> GS1900-24HPv2:	ABTP
> GS1900-48:	AAHN
> GS1900-48HP:	AAHO
> GS1900-48HPv2:	ABTQ
>
> Most of these should be supportable by OpenWrt.

Note that it is techincally possibly to build images which support more
than one "zyxel-vers", or even all of them, That's what the stock
firmware does.

The only reason I opted for one specific "zyxel-vers" per device is that
we use a device specific DTS.  Having a matching "zyxel-vers" prevents
flashing the wrong OpenWrt image from stock.


Bjørn
Hauke Mehrtens Feb. 27, 2021, 4:01 p.m. UTC | #6 
On 2/25/21 11:10 AM, Adrian Schmutzler wrote:
> Hi,
> 
>> -----Original Message-----
>> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
>> On Behalf Of Stefan Lippers-Hollmann
>> Sent: Donnerstag, 25. Februar 2021 07:56
>> To: Hauke Mehrtens <hauke@hauke-m.de>
>> Cc: openwrt-devel@lists.openwrt.org
>> Subject: Re: [PATCH 1/2] realtek: Add generic zyxel_gs1900 image definition
>>
>> Hi
>>
>> On 2021-02-24, Hauke Mehrtens wrote:
>>> Add a new common device definition for the Zyxel GS1900 line of
>>> switches.
>> [...]
>>> -define Device/zyxel_gs1900-10hp
>>> +define Device/zyxel_gs1900
>>>     SOC := rtl8380
>>>     IMAGE_SIZE := 6976k
>>>     DEVICE_VENDOR := ZyXEL
>>>     DEVICE_MODEL := GS1900-10HP
>>>     UIMAGE_MAGIC := 0x83800000
>>> +  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers
>>> +AAHI | uImage gzip endef
>>
>> I'm wondering if this attempt to deal the gs1900 switch family really improves
>> the situation for these devices. While IMAGE_SIZE and UIMAGE_MAGIC
>> might indeed by rather generic to most (all?) members of the gs1900 family,
>> SOC might not be (GS1900-24, GS1900-24E, GS1900-24HP are RTL8382M,
>> GS1900-48 and GS1900-48HP are RTL8393 - admittedly, I do not know how
>> different the resulting DTS would need to be, as these devices are not
>> supported yet) and zyxel-vers is different for every single model (aside from
>> GS1900-8HPv1 && GS1900-8HPv2):
> 
> Common definitions in Makefiles and DTS are two different questions. It's not uncommon that shared nodes in Makefile are possible where DTSes are still individual. Anyway, one could make zyxel-vers a variable and move back SOC to the devices when necessary:
> 
> DEVICE_VARS += ZYXEL_VERS
> 
> define Device/zyxel_gs1900
>     IMAGE_SIZE := 6976k
>     DEVICE_VENDOR := ZyXEL
>     UIMAGE_MAGIC := 0x83800000
>     KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers $$$$(ZYXEL_VERS)
> 	| uImage gzip
> endef
> 
> define Device/zyxel_gs1900-10hp
>     $(Device/zyxel_gs1900)
>     SOC := rtl8380
>     DEVICE_MODEL := GS1900-10HP
>     ZYXEL_VERS := AAZI
> endef
> TARGET_DEVICES += zyxel_gs1900-10hp
> 
> That would still keep the kernel_initramfs recipe in the common node. Of course, this might eventually hit a point where having the common node is more effort than benefit.

I will update it like this and also remove the DEVICE_MODEL.

Hauke
Adrian Schmutzler Feb. 27, 2021, 4:03 p.m. UTC | #7 
> > DEVICE_VARS += ZYXEL_VERS
> >
> > define Device/zyxel_gs1900
> >     IMAGE_SIZE := 6976k
> >     DEVICE_VENDOR := ZyXEL
> >     UIMAGE_MAGIC := 0x83800000
> >     KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers
> $$$$(ZYXEL_VERS)
> > 	| uImage gzip
> > endef
> >
> > define Device/zyxel_gs1900-10hp
> >     $(Device/zyxel_gs1900)
> >     SOC := rtl8380
> >     DEVICE_MODEL := GS1900-10HP
> >     ZYXEL_VERS := AAZI
> > endef
> > TARGET_DEVICES += zyxel_gs1900-10hp
> >
> > That would still keep the kernel_initramfs recipe in the common node. Of
> course, this might eventually hit a point where having the common node is
> more effort than benefit.
> 
> I will update it like this and also remove the DEVICE_MODEL.

Note that the wrapped line for KERNEL_INITRAMFS is broken in my example ...

Best

Adrian

> 
> Hauke
Hauke Mehrtens Feb. 27, 2021, 4:04 p.m. UTC | #8 
On 2/25/21 6:10 PM, Bjørn Mork wrote:
> Stefan Lippers-Hollmann <s.l-h@gmx.de> writes:
>> On 2021-02-24, Hauke Mehrtens wrote:
>>> Add a new common device definition for the Zyxel GS1900 line of
>>> switches.
>> [...]
>>> -define Device/zyxel_gs1900-10hp
>>> +define Device/zyxel_gs1900
>>>     SOC := rtl8380
>>>     IMAGE_SIZE := 6976k
>>>     DEVICE_VENDOR := ZyXEL
>>>     DEVICE_MODEL := GS1900-10HP
>>>     UIMAGE_MAGIC := 0x83800000
>>> +  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI | uImage gzip
>>> +endef
>>
>> I'm wondering if this attempt to deal the gs1900 switch family really
>> improves the situation for these devices. While IMAGE_SIZE and
>> UIMAGE_MAGIC might indeed by rather generic to most (all?) members of
>> the gs1900 family, SOC might not be (GS1900-24, GS1900-24E, GS1900-24HP
>> are RTL8382M, GS1900-48 and GS1900-48HP are RTL8393 - admittedly, I do
>> not know how different the resulting DTS would need to be, as these
>> devices are not supported yet) and zyxel-vers is different for every
>> single model (aside from GS1900-8HPv1 && GS1900-8HPv2):
>>
>> GS1900-8:	AAHH
>> GS1900-8HPv1:	AAHI
>> GS1900-8HPv2:	AAHI
>> GS1900-10HP:	AAZI
>> GS1900-16:	AAHJ
>> GS1900-24:	AAHL
>> GS1900-24E:	AAHK
>> GS1900-24EP:	ABTO
>> GS1900-24HP:	AAHM
>> GS1900-24HPv2:	ABTP
>> GS1900-48:	AAHN
>> GS1900-48HP:	AAHO
>> GS1900-48HPv2:	ABTQ
>>
>> Most of these should be supportable by OpenWrt.
> 
> Note that it is techincally possibly to build images which support more
> than one "zyxel-vers", or even all of them, That's what the stock
> firmware does.
> 
> The only reason I opted for one specific "zyxel-vers" per device is that
> we use a device specific DTS.  Having a matching "zyxel-vers" prevents
> flashing the wrong OpenWrt image from stock.


I used the wrong AAHI magic and it was possible to falsh the image over 
the Web UI, buit the image did not boot, it was unable to find the root 
fs for me.

Where is this check done?

I was unable to extract the original firmware with binwalk which would 
be nice to get some more information about how it is structured.

Hauke
Bjørn Mork Feb. 27, 2021, 5 p.m. UTC | #9 
Hauke Mehrtens <hauke@hauke-m.de> writes:

> I used the wrong AAHI magic and it was possible to falsh the image
> over the Web UI, buit the image did not boot, it was unable to find
> the root fs for me.

That's odd.  Didn't work in my tests, but I've only tested the
GS1900-10HP with the latest stock firmware.

But rootfs?  Flashing from stock will only work with the initramfs
image.  The stock web ui will cut the image according to the uimage
header, without warning.  So everything has to be "inside" the uimage.

But there is something wrong with our image code if you are able to
flash anything else than the initramfs image from stock.  Specifically:
The "zyxel-vers" stuff should *not* be appended to the kernel in the
sysupgrade image.

> Where is this check done?

AFAICS, only in the stock firmware upgrade "app".

> I was unable to extract the original firmware with binwalk which would
> be nice to get some more information about how it is structured.


The GPL drop at https://biot.com/gpl/GS1900-10HP(V2.60(AAZI.2)C0).zip is
pretty complete.  Only a few files are binary blobs.  And everything
necessary to build an image is included.

There isn't anything weird with the stock image.  It's a standard uimage
with a non-standard magic.  The list of supported hardware versions is an
ascii text file at the end of the real image, but inside the size and
checksum coverage of the uimage header.



Bjørn
Hauke Mehrtens Feb. 28, 2021, 5:38 p.m. UTC | #10 
On 2/27/21 6:00 PM, Bjørn Mork wrote:
> Hauke Mehrtens <hauke@hauke-m.de> writes:
> 
>> I used the wrong AAHI magic and it was possible to falsh the image
>> over the Web UI, buit the image did not boot, it was unable to find
>> the root fs for me.
> 
> That's odd.  Didn't work in my tests, but I've only tested the
> GS1900-10HP with the latest stock firmware.
> 
> But rootfs?  Flashing from stock will only work with the initramfs
> image.  The stock web ui will cut the image according to the uimage
> header, without warning.  So everything has to be "inside" the uimage.

Ok, I used the sysupgrade tar, I was surprised that this was accepted at 
all. ;-)

> But there is something wrong with our image code if you are able to
> flash anything else than the initramfs image from stock.  Specifically:
> The "zyxel-vers" stuff should *not* be appended to the kernel in the
> sysupgrade image.
> 
>> Where is this check done?
> 
> AFAICS, only in the stock firmware upgrade "app".
> 
>> I was unable to extract the original firmware with binwalk which would
>> be nice to get some more information about how it is structured.
> 
> 
> The GPL drop at https://biot.com/gpl/GS1900-10HP(V2.60(AAZI.2)C0).zip is
> pretty complete.  Only a few files are binary blobs.  And everything
> necessary to build an image is included.

Thanks I will have a look at this.

> There isn't anything weird with the stock image.  It's a standard uimage
> with a non-standard magic.  The list of supported hardware versions is an
> ascii text file at the end of the real image, but inside the size and
> checksum coverage of the uimage header.

Hauke
diff mbox series

Patch

diff --git a/target/linux/realtek/image/Makefile b/target/linux/realtek/image/Makefile
index 424726c8a9fb..fcb5e55e7ab6 100644
--- a/target/linux/realtek/image/Makefile
+++ b/target/linux/realtek/image/Makefile
@@ -84,37 +84,35 @@  define Device/netgear_gs110tpp-v1
 endef
 TARGET_DEVICES += netgear_gs110tpp-v1
 
-define Device/zyxel_gs1900-10hp
+define Device/zyxel_gs1900
   SOC := rtl8380
   IMAGE_SIZE := 6976k
   DEVICE_VENDOR := ZyXEL
   DEVICE_MODEL := GS1900-10HP
   UIMAGE_MAGIC := 0x83800000
+  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI | uImage gzip
+endef
+
+define Device/zyxel_gs1900-10hp
+  $(Device/zyxel_gs1900)
+  DEVICE_MODEL := GS1900-10HP
   KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAZI | uImage gzip
 endef
 TARGET_DEVICES += zyxel_gs1900-10hp
 
 define Device/zyxel_gs1900-8hp-v1
-  SOC := rtl8380
-  IMAGE_SIZE := 6976k
-  DEVICE_VENDOR := ZyXEL
+  $(Device/zyxel_gs1900)
   DEVICE_MODEL := GS1900-8HP
   DEVICE_VARIANT := v1
   DEVICE_PACKAGES += lua-rs232
-  UIMAGE_MAGIC := 0x83800000
-  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI | uImage gzip
 endef
 TARGET_DEVICES += zyxel_gs1900-8hp-v1
 
 define Device/zyxel_gs1900-8hp-v2
-  SOC := rtl8380
-  IMAGE_SIZE := 6976k
-  DEVICE_VENDOR := ZyXEL
+  $(Device/zyxel_gs1900)
   DEVICE_MODEL := GS1900-8HP
   DEVICE_VARIANT := v2
   DEVICE_PACKAGES += lua-rs232
-  UIMAGE_MAGIC := 0x83800000
-  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | zyxel-vers AAHI | uImage gzip
 endef
 TARGET_DEVICES += zyxel_gs1900-8hp-v2