New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Zyxel NSA310s #2984
Conversation
…read of https://forum.openwrt.org/t/serial-install-problems-on-nsa310 Many thanks to @bobafetthotmail Signed-off-by: ThBexx <thomas.beckler@hotmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot write info about hardware and installation method in commit message.
pinctrl-names = "default"; | ||
|
||
pmx_sata0: pmx-sata0 { | ||
marvell,pins ; /* NA */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that required?
}; | ||
|
||
pmx_sata1: pmx-sata1 { | ||
marvell,pins ; /* NA */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that required?
}; | ||
|
||
/* | ||
pmx_buzzer: pmx-buzzer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that required?
pinctrl-names = "default"; | ||
pinctrl-0 = <&pmx_usb_power &pmx_pwr_sata1>; | ||
|
||
usb0_power: regulator@1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider move it to user-space. Regulator-fixed cannot be changed from system.
&pmx_led_hdd1_green &pmx_led_hdd1_red>; | ||
pinctrl-names = "default"; | ||
|
||
green-sys { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed led aliases for status, booting, sysupgrade, etc...
It is resurrect support: 0ebdf0c ;) |
* 2 of the License, or (at your option) any later version. | ||
* | ||
* Based upon the board setup file created by Peter Schildmann | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an SPDX license identifier instead.
pinctrl: pin-controller@10000 { | ||
pinctrl-names = "default"; | ||
|
||
pmx_sata0: pmx-sata0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those labels don't seem to be used anywhere. Remove them?
pmx_usb_power: pmx-usb-power { | ||
marvell,pins = "mpp21"; /* OK */ | ||
marvell,function = "gpio"; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From here, indent is broken. Please fix the whole file, tab only.
}; | ||
|
||
pmx_pwr_off: pmx-pwr-off { | ||
marvell,pins = "mpp27"; /* OK */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the meaning of these comments?
marvell,pins = "mpp20"; | ||
marvell,function = "gpio"; | ||
}; | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused (=commented out) node.
}; | ||
|
||
serial@12000 { | ||
status = "ok"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use "okay" instead of "ok" (also for all other cases.)
pinctrl-0 = <&pmx_btn_reset &pmx_btn_copy &pmx_btn_power>; | ||
pinctrl-names = "default"; | ||
|
||
button@1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have named the nodes after there purpose here, e.g. "power" instead of button@1 for this one.
label = "Power Button"; | ||
linux,code = <KEY_POWER>; | ||
gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add empty lines between nodes.
gpio-leds { | ||
compatible = "gpio-leds"; | ||
pinctrl-0 = <&pmx_led_hdd2_green &pmx_led_hdd2_red | ||
&pmx_led_usb_green |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is lonely ...
label = "nsa310s:green:sys"; | ||
gpios = <&gpio0 28 GPIO_ACTIVE_HIGH>; | ||
linux,default-trigger = "default-on"; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty lines between nodes.
pinctrl-names = "default"; | ||
|
||
green-sys { | ||
label = "nsa310s:green:sys"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nsa301s:green:system ?
Despite, OpenWrt frequently uses inverted order for the node name, so in this case "system_green"
label = "nsa310s:red:hdd2"; | ||
gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>; | ||
}; | ||
green-usb { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is only one color, I'd just use "usb" for the node name.
partition@0 { | ||
label = "uboot"; | ||
reg = <0x00c0000 0x0080000>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty line between nodes.
|
||
partition@0 { | ||
label = "uboot"; | ||
reg = <0x00c0000 0x0080000>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's before?
}; | ||
|
||
&mdio { | ||
status = "okay"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty line after status.
|
||
&mdio { | ||
status = "okay"; | ||
ethphy0: ethernet-phy@1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ethphy1? ephy1?
&pcie0 { | ||
status = "okay"; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove useless empty line at EOF
Despite the several formatting issues in the DTS, please also update your commit message/title according to the formal requirements: |
@ThBexx and @adrianschmutzler I provided the dts file in a forum thread to do a quick hack and kinda "support" the nsa310s if you hijack the nsa310b patch with this and use bodhi's u-boot instead of the stock u-boot. I suggest looking at the old dts file that was part of OpenWrt a while ago (the device was never fully supported, but some parts of it were committed) Btw, the "Tony Dinh" in the commit is most likely the "bodhi" from the project I mentioned as that's one of his email accounts. also @ThBexx unless rules changed since I contributed the NSA325 and 310, the OpenWrt project is not likely to accept this device if the user must install bodhi's u-boot to use it. So you will either
|
thank for the all the inputs, as this was more a copy and paste from bodhi's implementation Doozan Forum. I fully understand your remarks on the bootloader. Unfortunately this is far beyond my skills but I may try with your guidance. Additionally I don't have a NSA310s at home, so I can't try changes. I try to get 'fialot' onboard (the one from the forum thread). |
U-boot have already support for this Zyxel: It looks like prepared for second stage. But it can be made as first stage. Maybe You just need make some OpenWrt adjustment like Seagate dockstar? It should be easy. |
Hi Alberto, bodhi (Tony) here! This NSA310s u-boot has been mainlined since way back (when Luka was the one of the Kirkwood maintainers). Basically, I developed this u-boot, and then Gerald and Luka cleaned it up to make it suitable for mainline submission. cat /usr/src/u-boot-2019.01/board/zyxel/nsa310s/MAINTAINERS
So, OpenWrt can just use the current mainline u-boot. Luka also sent my version of the NSA310S DTS to Linux kernel mailing list (after he cleaned it up) but it was not reviewed for submission (don't know what was the reason). My version of the DTS (Thomas's submission) is a downstream hobbyist version (has many formatting problems that I was never care to deal with). Luka's cleaned-up version is fit for OpenWrt mainline (before he sent it). I could find in my email boxes and post it here for OpenWrt if Thomas agree to that. -bodhi |
Please see my response in the GitHub pull-request:
#2984
Thanks,
-bodhi
|
Absolutely fine for me. Thank you for your comments and your contribution |
Here is the content of Luka email patch.
|
The cleanup DTS posted in GitHub. Strange that I got no Github email
notification from the post. So I send this email in case anybody missed the
notification.
…-bodhi
On Mon, May 4, 2020 at 9:33 AM Thomas Beckler ***@***.***> wrote:
Luka's cleaned-up version is fit for OpenWrt mainline (before he sent it).
I could find in my email boxes and post it here for OpenWrt if Thomas agree
to that.
Absolutely fine for me. Thank you for your comments and your contribution
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2984 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVWCYRZWZQTIDNKXYC54S3RP3U4BANCNFSM4MX2NV4Q>
.
|
So, now we have multiple versions of device support patches and multiple options to make the device boot in an acceptable way. However, the initial author stated he does not own the device. So, to go on with this, we need somebody to put all this together (where the initial author stated he couldn't) and somebody willing and capable to test. Please sort this out. If somebody different from the initial author wants to continue, it would be easiest if he/she opened a new PR on his own account, and we close this one. If nobody is interested pushing this forward, I will close this PR. |
I've built images from this PR and can test on a device borrowed from a friend, exactly to add the support. Expect to hear from me tomorrow. Some instructions on your way of installation would be welcome. |
@Leo-PL Thanks, but at the moment this PR needs to be changed first, as the installation method is not acceptable for OpenWrt. However, your offer might make it easier for somebody to pick up the work without the actual need to own the device. |
I will prepare support for nsa310s based on code dropped with 0ebdf0c and this PR. |
Please look #3008. |
Since the initial author stated he wouldn't be able to do the necessary adjustments, and there is a replacement PR available, I'm closing this. Thanks for bringing this up to this point. Please make sure the initial author is credited in the new proposal. |
following the forum thread of https://forum.openwrt.org/t/serial-install-problems-on-nsa310
Many thanks to @bobafetthotmail
Signed-off-by: ThBexx thomas.beckler@hotmail.com