Hi, On 08.05.22 13:11, Sander Vanheule wrote: > Hi, > > Sorry I didn't get back to this any sooner. > > On Wed, 2022-04-27 at 20:16 +0200, Birger Koblitz wrote: >> Hi, > It's still not clear to me what issue or issues you are fixing exactly with > this patch, > and in what way. The patch fixes a bug that was introduced during the merging of the Zyxel XGS1250 code which also is an issue for the XGS1210. The problem as stated with the submitted patch is that a reset of the SerDes connecting to the 8 1GBit ports was done. This is a single XGMII link to the RTL8218D, which provides 8 1GBit ports via this link.
The handling of (US)XGMII links is entirely different from the handling of SGMII/HiSGMII, 1000BX or 10GR with a completely different code-path on both the RTL9300 and the RTL9310. The difference is that the internal SerDes is in fact turned "off" in the latter case, and in the former it is put into a suitable XGMII mode. For (Hi)SGMII/1000BX/10GR the SerDes is then put (in its outside "off" state") into what is on RTL931x a suitable "fibre" mode and on RTL9300 into a "forced" mode. All these modes then need to be suitably RX calibrated and the pre- main and post- amplifiers set up properly for TX. The bug was to do a complete SerDes reset on all SerDes links, although the code for setting up the XGMII link was not there, thereby wiping the (RX/TX) setup done by u-boot and breaking the 8 1GBit ports. The 10GBit SFP+ links are mostly understood (from the Ubiquiti USW switch), so here we use the appropriate setup code, which e.g. is necessary when someone pulls a module out and puts another in. > > Is the suggested change in behaviour required for SFP+ modules to completely > fix bringing > up new links? Or is this only part of the solution? Your reply makes me think > it's the It is only part of the solution. It makes swapping out 10GBit (fiber!) modules possible. It does not allow any 1GBit modules. > latter. If the changed behaviour for SFP+ modules here is fixing an issue, > please describe > the issue as well as you can in this patch, and also describe any remaining > issues. The patch only fixes what says it does. I don't see why I should list here all remaining issues, especially since I am probably not aware of all of them. What works and does not work is part of the description coming with the device commit. As the commit messages says it merely fixes a bug introduced at that time. > Otherwise, if this change by itself leaves SFP+ equally broken as before, > please just have > this patch fix the GbE links, and keep other changes for a later patch. No, it is necessary to keep the possibility to swap out 10G modules. > > The network code on this platform is really not my area of expertise, so any > explanation > on what is wrong, and why it needs to be fixed in this way, can help me > understand. It can > also provide crucial information for others working on this code, since I > think we need to > use every opportunity to (publicly) document the behaviour of this hardware. I do not believe it is necessary for anyone to understand a patch to commit it. The question here is about a sanity check. The code was tested as it says, and the commit message states what it does. Documentation is better collected in the code so that it is always at hand and in fact the explanation I give above can be found in the comments for the proposed patches for support of all the serdes modes of the RTL9300 and the ongoing work with the RTL931x. I would rather not waste time on this 2-line patch (!) which makes the XGS12x0 users happy now (they cannot use any of the 1GBit ports as it stands since several weeks now) and invest it in solving things properly. Cheers, Birger _______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
