realtek: Refactor rtl930x leds #11898
Conversation
| RTL93XX_LED_SET_LINK_ACTIVITY | | ||
| RTL93XX_LED_SET_LINK_UP_SOLID | | ||
| RTL93XX_LED_SET_5G_LINK | | ||
| RTL93XX_LED_SET_100M_LINK |
There was a problem hiding this comment.
btw, I'd recommend also adding the 10M here, by default, we get no light all, and officially 10M isn't supported, but of course it works just fine, and amber is 100/10 on GE ports ...
There was a problem hiding this comment.
Officially not supported by Zyxel? Because the LED controller hardware does appear to support it.
There was a problem hiding this comment.
10M? it works fine on my RTL8226 ports, but you have your aquantica MAC's there, so they may not support it.
I thus only recommended it, if you can confirm (with ethtool -s 10M on the host) if the port actually works. It does for me, and the leds indicate it all properly. Btw, the 10/100M support on those ports I added later in my patch series only, though I don't think it affects the aquantia? I can't test that, but if you start testing this stuff, take my WIP or DEV branches's tip to get all those patches and fixes.
zyxel ignores 10M, but having a 'black' led, when 10M is connected is super weird too, even if it works. And disabling it in openwrt/software feels wrong too. If the hardware fully works with it, why not?
github-actions
bot
added
the
target/realtek
|
@svanheule any thoughts at all? |
There was a problem hiding this comment.
I won't have time to review this completely today, so I can only get back to this in March. The refactor of the LED controller's init code is a bit to big to review the half hour I have right now :-/ (but you're right that it is needed, there was too much magic going on)
Feel free to ping another dev to get this reviewed/merged. @Ansuel has been doing some upstream LED stuff for qca8k.
| 0x3, | ||
| of_property_read_bool(node, "active-high") ? RTL930X_LED_GLB_CTRL_LED_ACTIVE : 0 | | ||
| 0x1, | ||
| RTL930X_LED_GLB_CTRL); |
There was a problem hiding this comment.
Why are you changing this into a single statement? You need a nested ternary operator and so many line breaks to keep it clean...
I still fail to see how active-high is better than active-low from the DT perspective. There is currently no precedent for the former in upstream LED bindings, so the only argument would be to align with the hardware default. But that's not something that can be relied on to already be set correctly, so it doesn't help to simplify the code.
There was a problem hiding this comment.
so two points, this is just an intermediate commit, if you look at the final result, it'll all make sense, in short, I'm removing all magic values and give meaning to all values.
We can always refactor this to be slightly better too, but other then storing the intermittent value in a variable, there's not much more we can do.
As for 'still fail to see'. I still fail to see why we had to change it from active-low to active-high before? All changes are arbitrary. So before, we had it like the vendor made it. You changed it to something arbitrary that 'felt right to you'. Until the led framework 'mandates/prefers a default, we should stick (like most, I think) with whatever the vendor intended as default (how good or bad it may be), rather then arbitrary defaults, because in the end, that's what the device developers, users etc, everybody will expect.
In the end, you'd argue 'this will be moot anyway', because the new driver will replace this anyway, though 'the default' will matter.
| led-num = <( | ||
| RTL93XX_LED_NUM0 | /* Amber */ | ||
| RTL93XX_LED_NUM1 /* Lime */ | ||
| )>; |
There was a problem hiding this comment.
led-set and led-num aren't upstreamable. This is also very RTL93xx-specific, and doesn't allow a user to reference an LED (for standard OpenWrt software control), which is needed on some devices (e.g. Cisco SG220). But good shouldn't the evil of perfect, and I'm willing to accept this as a stop-gap solution to improve device description.
My proposed led controller driver handled this by requiring every used LED to be defined individually. The driver would then determine how to configure the hardware in the most appropriate way. So in a sense what you're adding here is similar.
There was a problem hiding this comment.
led-setandled-numaren't upstreamable. This is also very RTL93xx-specific, and doesn't allow a user to reference an LED (for standard OpenWrt software control), which is needed on some devices (e.g. Cisco SG220). But good shouldn't the evil of perfect, and I'm willing to accept this as a stop-gap solution to improve device description.
Well we already had led-set, there's nothing new here, I only added led-num. And yes, they would have to be 'realtek,rtl93xx-led-set' at the least :p but I also do not intend to upstream either of those (hopefully). These are temporary, until we can go to a better driver. And then, it'll need to cover all our cases obviously.
My proposed led controller driver handled this by requiring every used LED to be defined individually. The driver would then determine how to configure the hardware in the most appropriate way. So in a sense what you're adding here is similar.
Yeah, I saw that, but then, I don't like that either. While this is certainly the wrong place to discuss future design :p why I didn't like it, is because that allows you to have an arbitrary led setup, which sounds perfect, except you are restricted to the 4 (for rtl93xx), 3 afaik for rtl839x? So now you have some very deep complexity in figuring this out. Ideally, you'd refer a led here via a phandle and which 'mode', but the mode bit doesn't exist yet (which you struggled with too). But again, wrong time/place. And something we should deff. have a brainstorming session over.
| RTL93XX_LED_NUM1 | /* Amber | Red */ | ||
| RTL93XX_LED_NUM2 | /* Lime | Green */ | ||
| RTL93XX_LED_NUM3 /* off | Blue */ | ||
| )>; |
There was a problem hiding this comment.
I still can't believe they wired the LEDs this way... I think it makes it pretty much impossible to allow all five LED components to be configured to userspace control (because one pin is essentially a mux now).
There was a problem hiding this comment.
five? thought it was 4? anyway, yeah, and that's exactly my point above, it's not so black/white trivial. So (ignoring the warts and uglyness what this patch propses) it is atleast 'somewhat' logical, but even that falls on its ass (when you look at the led configuration, 1G_ACTIVE etc) where you have to keep in mind that it's a mux you are setting up ...
It's just very nasty, not just how they hooked up the leds, more how this is what the hardware allows (an in documentation encourages to a point).
| else | ||
| port_mask_copper[REALTEK_PORT_ARRAY_INDEX(port, 1)] |= RTL931X_LED_PORT_COPR_MASK_CTRL_PMASK(port, true); | ||
| /* TODO need to figure out if leds are either/or */ | ||
| port_mask_combo[REALTEK_PORT_ARRAY_INDEX(port, 1)] |= RTL931X_LED_PORT_COMBO_MASK_CTRL_PMASK(port, true); |
There was a problem hiding this comment.
On RTL839x at least, it worked to always set both the fiber and copper mask (if a port had LEDs), and then to only clear the combo mask if there were separate copper/fiber status LEDs. Setting the combo mask effectively combined the two LED outputs into one, so even if there is no actual combo port, I could use "copper + fiber + combo".
There was a problem hiding this comment.
rtl93xx seems to be slightly different, I couldn't get it working this way 100%, but I was messing around with a lot of things. Also, not actually having the hardware that needs this config setup, makes it also hard to confirm how it ACTUALLY works. I wrote in the register document what the datasheet more or less says, but that's not that usefull either... :(
| RTL93XX_LED_SET_LINK_ACTIVITY | | ||
| RTL93XX_LED_SET_LINK_UP_SOLID | | ||
| RTL93XX_LED_SET_5G_LINK | | ||
| RTL93XX_LED_SET_100M_LINK |
There was a problem hiding this comment.
Officially not supported by Zyxel? Because the LED controller hardware does appear to support it.
oliv3r
force-pushed
the
refactor_leds
branch
3 times, most recently
from
ae2aedc
to
e1354f2
Compare
target/linux/realtek/files-5.15/arch/mips/include/asm/mach-rtl838x/mach-rtl83xx.h
Show resolved
Hide resolved
|
@svanheule so, a month later; do you have any new insights and thoughts? |
Registers are often used as bitmaps or multi-bitmaps, causing registers to spread over several registers per port. E.g. we can only fit 32 ports in a single register if it uses a single bit bitmap, but already 2 registers when using two bit bitmaps. To make these calculations easier and reduce the need to having to retype these often, add some simple helpers for these. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
The realtek SoC's are by default 'active-low' on the LED's. Instead of forcing something here, instead lets follow the default of the vendor/SoC. Further more, we don't need to write the same register multiple times to change each individual bit/setting. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
Each port needs two LED-related settings. Which LED-set to use and how many of LEDs are connected to a port. Since these are related, but not coupled, we can't use any algorithm to figure this out otherwise, as a LED-set definition could account for 4 LEDs, but a port may only wire 2 of them. E.g. a switch with 8 ports 4 with POE 4 without, which could mean 4 ports with 4 LEDs (2 LAN LEDs, 2 POE LEDs) and 4 ports with just 2 LEDs (lan). To be more future-proof, we store this as a bitmap, so that in theory, we could determine 'led 3 and led 0' are connected. The rtl930x hardware doesn't seem to support this however. It is trivial to convert this to an actual number of leds with the `hweightN` series of bitop functions. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
The current LED init function doesn't work properly as it does not account for led counts for example. Since the current function is already a bit weird, has some magic and can be simplified a little to improve readabilility, lets go ahead and do that. This re-write also changes to use the proper compatible setting, and the dts files that use this are being updated to match. The functions for rtl930x and rtl931x are near identical, the only difference of course beeing the addresses and other macro's. In the future, we can hopefully combine these. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
The zyxel switch sets up the led-set property to 0xffff, which wasn't even used, nor intended. So instead, set it to a more sensible value, rightfully indicating 'not used'. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
We do not have to be magical about these values, we know what they mean, so lets use defines instead to make reading easier. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
The alias is unused and doens't serve any purpose. Lets drop the alias. While here, also make the sets plural, as we potentially store more then one set. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
There are more (xgs1210) switches based on the same platform that require a different set of leds (2G5 ports). We do however have enough led-sets available, to define one set for all switches, allowing us to reduce duplicate code. set 0 is for standard 1G ports set 1 is for 2G5 ports set 2 is for 10G copper ports set 3 is for 1/10G SFP+ ports Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
|
There's a patch set on the linux netdev (and linux-leds) mailling list, currently in its fifth revision: The patch set defines a generic interface for PHY or MAC controlled network port LEDs and I think we should conform to that interface. However, it does not implement offloading of the netdev trigger, as that is non-trivial and out of scope for the initial support. When I have more time, I'll try to have a more detailed look at how these patches, my proposed led controller driver for maple/cypress, and the netdev proposal can be joined into a more uniform approach of exposing the LED controllers. At this moment however, I feel that more attention should be going towards getting actually functional network support on rtl930x first, but you choose what to work on. |
Sure, that's long time, and there's a few bits and bobs that need improvement there as well. Fundamentally, we need something today, for all switches that are not xgs1250. This makes things work today. It's still ugly and crap, but useful. Your upstream variant also takes a few shortcuts that I'm sort of dissagreeing with, and shouldn't be using MFD as it is now, but lets not discuss that here :) We simply have to stop waiting for 'upstream only' and fix stuff locally first. Again, exposure. But sure, lets not merge anything, lets not get support to potential users, and keep a carrot stick out for the next 5 years. Perfect being the enemy of somewhat better. I think you should really start asking yourself; will this patch/MR improve the current situation. If the answer is 'yes' or 'a little' just start merging stuff, because it can't get worse, and waiting said 5 years, is not really an option. |
|
My 2 cents, we should not be toxic on the topic... I think the idea of referring that patchset was to find all the problem with the implementation and improve it even more. If you notice the revision is done with Andrew patch as we are catching each problem with the implementation for marvell phy. (and I refactored lots of the current implementation that was specific to qca8k already) So Ideally some hack and stuff are accepted but we should at least stick to an upstream proposal and check if there are any problem with them. It's really to help upstream and at the same time already prepare something when the feature is actually supported. |
I'm all in favor on that! And I'm very excited that you are pushing to extend the led framework to support switching states ontop of that! :D
Yeah, I saw, there was a lot in that series though; to make it a little harder to digest the interesting bits from it though.
So this is where you miss-understood surely. I'm not against the upstream approach at all. What this patch does, is enable LED support for rtl93xx based switches using our current (horrible and ugly) code base. The current implementation hard-codes the leds, using magic values from the DTS, which only work on the XGS1250 specifically. Any other switch and it doesn't work reliably anymore. This patch cleans up the led_init function a little, makes it readable, improves its quality a little, (can't fix everything because a) we'll throw all of this away with the proper LED driver once it exists) and b) with the realtek stuff, once you pull a thread, things quickly unravel :( ... I have 6 rtl93xx switches in my tree now, that more or less depend on this series to get their leds working today. Since this mr is now MORE than 2 months old now, and assuming it'll take a while before the led stuff is finalized (not just the framework, but also the driver), we're talking a 6 months delay. I'd rather get support to our users out now, so they can give feedback, so we know we are doing the right thing, so that we are understanding how it all works. So, if there's no technical concerns, just merge this, so we can move forward. And as we move forward, potentially have 6 new targets with the next openwrt release, and then migrate this to the upstream stuff when ready. As I always say, baby-steps. Sometimes you have to improve the ugly, before you get the good :) Maybe that makes more sense now? |
|
@svanheule since you're semi-active again, with merging jan's recent changes (which don't do anything positive code-quality wise :p) can we also move forward with this little patch to get the leds working on rtl93xx switches? |
This MR refactors the current led_init functionality of the rtl930x. It is only a small stepping stone for later, but helps us understand the RTL930x behavior better.
Note, that it also fixes LEDS on non XGS1250-12 targets, which would cause leds to be off as it doesn't account for led-counts.