Skip to content
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

Lantiq: xrx200: Add support for AVM Fritzbox 3390 #3656

Closed

Conversation

cocktailyogi
Copy link
Contributor

@cocktailyogi cocktailyogi commented Dec 6, 2020

All the work has been done some time ago by Andreas Böhler alias @andyboeh and was already documented in this closed Pull-Request: #2662

I have rebased that changes to Kernel 5.x, tested it and are now filing this pull request, because andyboeh did not want to continue this PR. I added some HW-details here:
https://openwrt.org/inbox/toh/avm/avm_fritzbox_3390

Status:
avm_fritz3390 is working with 5 GHz WIFI and tested for a couple of weeks as DSL-Router

2.4GHz not supported
(2.4 Ghz is implemented by a second SOC via "WASP-Interface". andyboeh did basic work on it, but to me it seemed to complicated and I am not interested in 2.4 Ghz WIFI of this box.) For details see https://www.aboehler.at/doku/doku.php/projects:fritz3390

@cocktailyogi cocktailyogi changed the title Lantiq: Add support for AVM Fritzbox 3390 Lantiq: xrx200: Add support for AVM Fritzbox 3390 Dec 6, 2020
@@ -47,6 +47,9 @@ lantiq_setup_interfaces()
avm,fritz7430)
ucidef_add_switch "switch0" \
"2:lan:3" "3:lan:4" "4:lan:1" "5:lan:2" "6t@eth0"
avm,fritz3390)
ucidef_add_switch "switch0" \
"0:lan:3" "1:lan:4" "2:lan:2" "4:lan:1" "5:lan:5" "6t@eth0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is port number 5 connected to WiFi?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, from what I understood it seems to be the connection to the second SOC (also know as WASP-interface)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove LAN5 entirely for now, as it causes more confusion than it serves - see my other comments.

@abajk
Copy link
Contributor

abajk commented Dec 6, 2020

Maybe squash some commits

@cocktailyogi
Copy link
Contributor Author

Okay, I have integrated your suggestions.
But I was not able to squash my commits. I googled how to squash commits on Github, but I am not sure if I understand how to do it. I am afriad that I might bring in this more mess, than it helps. So could someone guide me?

Yogi

@jschwartzenberg
Copy link
Contributor

But I was not able to squash my commits. I googled how to squash commits on Github, but I am not sure if I understand how to do it. I am afriad that I might bring in this more mess, than it helps.

You have to do this using Git (not GitHub). First rebase using:
git rebase -i <parent commit>

Then in your text editor you can use squash/fixup.

Then:
git push -f

You have to use -f for a forced push as you're rewriting history. (The alternative would be a new branch & new PR, but GitHub handles squashes & forced pushes to existing branches well in PRs.)

In case you want to keep the original commit set around as a precaution, create another branch first and then switch back to this one before you squash & push.

@cocktailyogi cocktailyogi force-pushed the avm_fritz3390_wifi_rebased branch 6 times, most recently from 1dea752 to 06b9bd7 Compare December 6, 2020 21:24
@cocktailyogi
Copy link
Contributor Author

Maybe squash some commits

Done :-)

Thx to @jschwartzenberg

@adschm
Copy link
Member

adschm commented Dec 6, 2020

This should be tidied up:

  • Create commit by what they do, not by their history (e.g. one commit for fixing arch-wide gpio, one commit for adding devices, one commit for another separate subject)
  • Add missing commit messages
  • Take care to properly show/document authorship
  • There is no kernel 4.19 support anymore

Apart from that, be aware that the last PR was closed due to lack of interest, so it's probably that this one will meet the same fate and your work might be in vain (apart from individuals using the PR as is)

@adschm adschm added needs changes target/lantiq pull request/issue for lantiq target labels Dec 6, 2020
@cocktailyogi
Copy link
Contributor Author

* There is no kernel 4.19 support anymore

I know, but what does this mean here. I did not include anything regarding Kernel 4.19

* Take care to properly show/document authorship

Is done, I think

* I just had squasehd my commits, because it was recommented. Now ther eis ne commit, what I did:

Rebasing the work somebody else has done. Basically this is one step.

@andyboeh
Copy link
Contributor

andyboeh commented Dec 7, 2020

Why are you basing your work on my avm_fritz3390_wifi branch instead of the avm_fritz3390 branch? The _wifi branch has all the infrastructure in place to bring up WiFi, whereas the other branch only includes the stuff you are actually trying to get included.

The problem here is that your commits first add a lot of stuff that you remove afterwards. So basically, your PR should come up with two commits:

  • One that adds the gpio-ranges define for all VR9 devices
  • One that adds device support

See #2662 for reference, where exactly this is done.

gpios = <&gpio 34 GPIO_ACTIVE_HIGH>;
};
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This export is only required if you bring up the secondary WiFi SoC. Personally, I would remove it for now - please wait for other reviewer's comments before doing so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we should decide about this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I woud prefer to remove this node.

speed = <1000>;
full-duplex;
};
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This node is only required for the WiFI SoC. On 4.19, this had no effect without a further patch because the xrx200 driver did not support fixed-link nodes. I don't know about 5.4 so please verify that.

@@ -47,6 +47,9 @@ lantiq_setup_interfaces()
avm,fritz7430)
ucidef_add_switch "switch0" \
"2:lan:3" "3:lan:4" "4:lan:1" "5:lan:2" "6t@eth0"
avm,fritz3390)
ucidef_add_switch "switch0" \
"0:lan:3" "1:lan:4" "2:lan:2" "4:lan:1" "5:lan:5" "6t@eth0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove LAN5 entirely for now, as it causes more confusion than it serves - see my other comments.

@cocktailyogi
Copy link
Contributor Author

Why are you basing your work on my avm_fritz3390_wifi branch instead of the avm_fritz3390 branch? The _wifi branch has all the infrastructure in place to bring up WiFi, whereas the other branch only includes the stuff you are actually trying to get included.

The problem here is that your commits first add a lot of stuff that you remove afterwards. So basically, your PR should come up with two commits:

* One that adds the `gpio-ranges` define for all VR9 devices

* One that adds device support

See #2662 for reference, where exactly this is done.

Hi Andy, thx for your reply.
The answer is easy: Because I am beginner and not used to this big Github-Community. Is just wanted to use my Fritzbox3390 on Openwrt and make your work available for everyone in the future. This is easier, when it is fully comitted into Openwrt. I had it, if work is done and nobody respects it. Sorry that is used your wrong branch.
Do you think, I should close this PR and redo the job based on your other branch and file an new clean PR? I can do this, if this helps.

Yogi

@andyboeh
Copy link
Contributor

andyboeh commented Dec 7, 2020

The answer is easy: Because I am beginner and not used to this big Github-Community. Is just wanted to use my Fritzbox3390 on Openwrt and make your work available for everyone in the future. This is easier, when it is fully comitted into Openwrt. I had it, if work is done and nobody respects it. Sorry that is used your wrong branch.

No worries, it just would have saved you a lot of work.

Do you think, I should close this PR and redo the job based on your other branch and file an new clean PR? I can do this, if this helps.

No, just squash it properly - same effect.

@cocktailyogi
Copy link
Contributor Author

cocktailyogi commented Dec 7, 2020

Okay, I squashed it more. Now it looks like the Wifi-Stuff was never in here.

@adschm
Copy link
Member

adschm commented Dec 7, 2020

I know, but what does this mean here. I did not include anything regarding Kernel 4.19

target/linux/lantiq/files-4.19/arch/mips/boot/dts/lantiq/vr9_avm_fritz3390.dts ?

@adschm
Copy link
Member

adschm commented Dec 7, 2020

Okay, I squashed it more. Now it looks like the Wifi-Stuff was never in here.

As both andyboeh and me stated: This needs only two commits: one for the gpio-ranges, and one for device support.

Keeping somebody elses work does not mean you cannot change it anymore, you just need to document what you changed in the commit message ...

@adschm
Copy link
Member

adschm commented Dec 15, 2020

One of the commits still doesn't have a commit message.

Which one? On my screen they both have commit-messages.

lantiq: VR9: fix gpio-hog by defining the GPIO ranges

@cocktailyogi
Copy link
Contributor Author

One of the commits still doesn't have a commit message.

Which one? On my screen they both have commit-messages.

lantiq: VR9: fix gpio-hog by defining the GPIO ranges

added more text ;-)

@abajk
Copy link
Contributor

abajk commented Dec 19, 2020

One of the commits still doesn't have a commit message.

Which one? On my screen they both have commit-messages.

lantiq: VR9: fix gpio-hog by defining the GPIO ranges

added more text ;-)

It would be good to explain in the commit description that this change is needed by gpio-hog on Fritzbox 3390.

@cocktailyogi
Copy link
Contributor Author

One of the commits still doesn't have a commit message.

Which one? On my screen they both have commit-messages.

lantiq: VR9: fix gpio-hog by defining the GPIO ranges

added more text ;-)

It would be good to explain in the commit description that this change is needed by gpio-hog on Fritzbox 3390.

done

andyboeh and others added 2 commits December 20, 2020 09:11
The FRITZ!Box 3390 actually contains two SoCs, one Lantiq with a
5GHz WiFi and one AR9342 with a 2.4GHz WiFi. Only the Lantiq
has access to the flash memory, the Atheros runs fully from RAM.

Specifications
--------------

  - Lantiq 500 MHz
  - 128MiB RAM
  - 128MiB NAND
  - 256k Flash
  - AR9580 5GHz WiFi
  - AR9342 560 MHz
  - 64MiB RAM
  - AR9328 2.4GHz WiFi

Remarks
-------

This commit only adds support for the Lantiq side of things and
prepares the drivers for communication with the Atheros SoC. Thus,
only 5GHz WiFi works by default, the 2.4GHz WiFi will be added via
another target.

Some kernel patches will be required to add support for the Atheros SoC.

Installation
------------

Use the eva_ramboot.py script to boot the initramfs image. Then, transfer
the sysupgrade image to the device and run sysupgrade to flash it to the
NAND.

Signed-off-by: Andreas Böhler <dev@aboehler.at>

-----------------------------------------------------------

Rebased support for AVM Fritzbox 3390
Tested as DSL-Router with 5 GHz Wifi.
2.4GHz Wifi is not functiional. 2.4Ghz is implemented in HW via 2nd SOC, which is booted via internal Ethernet,
called WASP-Interface, which is not covered by this Commits

Changed wpad-basic-wolfssl as default

add "pinctrl-0" nand settings. Now it is similar to Fritzbox 3370 with Micron-NAND, which has similar architecture and Nand

remove gpio-setting for WASP-reset

removed unused nand-device

changed ethernet mode to rgmii

Corrected NAND Bank width according to specs

Signed-off-by: Joachim Cerny <cocktail_yogi@web.de>
defined gpio-ranges according to Reference: https://github.com/torvalds/linux/blob/master/drivers/pinctrl/pinctrl-xway.c#L864
This change is needed by gpio-hog on Fritzbox 3390
thx to @abajk

Signed-off-by: Joachim Cerny <cocktail_yogi@web.de>
@abajk
Copy link
Contributor

abajk commented Dec 20, 2020

Acked-by: Aleksander Jan Bajkowski A.Bajkowski@stud.elka.pw.edu.pl

@cocktailyogi
Copy link
Contributor Author

Okay, whats next? I have done all requested changes. Now its time to finish things.

@jschwartzenberg
Copy link
Contributor

Okay, whats next? I have done all requested changes. Now its time to finish things.

At least one more code review is necessary, but to obtain the best quality it never hurts to have even more people look at the changes. Maybe you could ask in #openwrt on Freenode, ask through the mailing list or the forum?

Hopefully a second reviewer can at least join in!

@cocktailyogi
Copy link
Contributor Author

cocktailyogi commented Dec 23, 2020

@adschm
Could you please have a look at the final changes? You have done a lot of review-work and helped a lot. It would be nice, if you could approve this.

Yogi

@cocktailyogi
Copy link
Contributor Author

Okay, whats next? I have done all requested changes. Now its time to finish things.

At least one more code review is necessary, but to obtain the best quality it never hurts to have even more people look at the changes. Maybe you could ask in #openwrt on Freenode, ask through the mailing list or the forum?

Hopefully a second reviewer can at least join in!

Why cannot you do the codereview?

@jschwartzenberg
Copy link
Contributor

Why cannot you do the codereview?

I'm not an OpenWRT developer. I just hope to see this merged because Fritzboxes are also commonly used where I live.

People that are part of the OpenWRT GitHub organization need to review & approve I think.

@cocktailyogi
Copy link
Contributor Author

Why cannot you do the codereview?

I'm not an OpenWRT developer. I just hope to see this merged because Fritzboxes are also commonly used where I live.

People that are part of the OpenWRT GitHub organization need to review & approve I think.

Merry Christmas,

you are a helpful guy and have good knowledge. they should promote you into developer status. In our area people are using plenty of AVM-devices. Lets hack them :-)

@jschwartzenberg
Copy link
Contributor

Merry Christmas,

Merry Christmas to you too and/or Merry Grav-Mass ;)
Hope everybody can enjoy these days!

you are a helpful guy and have good knowledge. they should promote you into developer status. In our area people are using plenty of AVM-devices. Lets hack them :-)

Haha not really. I know Git and while I am a software developer, C is not a language I know well and I know little about OpenWRT as well. Even if they would promote me so that I could approve a PR like this, I would never do it.

We need to be patient here. OpenWRT and also the Linux kernel could only be where they are now because people took care to make sure changes going in were of high quality. Code reviews are an important part of software development. At work I am known for stating that while a feature might already implemented and in the code review phase, that could still mean it is far from done as during the code review it might turn out to be necessary to rewrite the entire implementation of it.

Do not be discouraged! Having one approval for this is already a major milestone. It is not unlikely you will be asked to change some things before there will be a second approval. This can be a long process, but it is only so much for the better once your changes end up in OpenWRT. As all changes go through these processes, it ensures that once this router model is supported it will continue to work in the future as others bring in their changes. Having well-understood code also helps new developers to step in and contribute.

With typical proprietary firmware, the vendors have access to all hardware to be supported so they can actually test each and every release they put out on all their models. OpenWRT does not have this, so code quality is much more important.

So get this further going, I would recommend asking around in the OpenWRT community as I suggested. Being acquainted there will also create trust. Even when changes are merged in, it is possible some after-care is needed too. Showing interest and being involved with the OpenWRT community also creates the necessary trust that you will be around when such after-care is needed. It is not possible to expect OpenWRT project members to reach out to contributors when facing issues. They rely on being able to understand issues themselves, but having original contributors in reach is ideal. Can they expect you to monitor their bug tracker from time to time and reply to Fritzbox 3390 issues for example?

A second thing I recommend doing is contacting your ISP and/or AVM. Explain them the benefits you see in them supplying resources. If AVM as a vendor would at least provide minimal resources to OpenWRT development, it would already be great.

I hope this helps! Good luck!!

@cocktailyogi
Copy link
Contributor Author

cocktailyogi commented Jan 8, 2021 via email

@hauke
Copy link
Member

hauke commented Apr 3, 2021

Thank you for the pull request, I applied it to master.

@hauke hauke closed this Apr 3, 2021
@jschwartzenberg
Copy link
Contributor

Great to see progress on the AVM boxes!! Thanks a lot everybody!

@cocktailyogi cocktailyogi deleted the avm_fritz3390_wifi_rebased branch April 6, 2021 17:46
@Gisi0
Copy link

Gisi0 commented May 9, 2021

Great to have an image for this, I tried to install the snapshot and get this error using eva_ramboot.py
I hope somebody can help me.

SETENV memsize 0x07ae4000 Traceback (most recent call last): File "./eva_ramboot.py", line 40, in <module> adam('SETENV memsize 0x%08x'%(addr)) File "./eva_ramboot.py", line 33, in adam resp = ftp.sendcmd(cmd) File "/usr/lib/python3.7/ftplib.py", line 273, in sendcmd return self.getresp() File "/usr/lib/python3.7/ftplib.py", line 246, in getresp raise error_perm(resp) ftplib.error_perm: 500 'SETENV memsize 0x07ae4000': command not understood.

@sfritz2
Copy link

sfritz2 commented May 22, 2021

Works fine for me. If it is not working for you, flash a 3370 image (https://openwrt.org/toh/avm/fritz.box.wlan.3370?s[]=3370 ), use the micron version. After make a sysupgrade to 3390.

@sfritz2
Copy link

sfritz2 commented May 22, 2021

also try to set first the ip to 192.168.178.5 and make a ping on boot, ttl should be 250. if not, you're not in bootloader and restart the fritzbox.

@Gisi0
Copy link

Gisi0 commented May 23, 2021

Got it to run, forgot a switch between the box and my pc. Network was to late up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs reviewer target/lantiq pull request/issue for lantiq target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants