Re: Purpose of openwrt-devel?
On 24-03-2024 06:35, Elliott Mitchell wrote: On Sat, Mar 23, 2024 at 10:02:39PM +0100, Olliver Schinagl wrote: On 23-03-2024 18:51, Elliott Mitchell wrote: On Sat, Mar 23, 2024 at 03:15:44AM +0100, Olliver Schinagl wrote: Odd thing about what you put in parentheses. I've been trying to get a discussion going about that for months, yet seems no one notices the suggestion. This was a distinct goal of the script, to hopefully get that discussion going. To update all targets at once? How is that useful?! Taking the unusual step of splitting a paragraph since that is needed in this case. I've cited two specific reasons in a recent message. I keep citing those reasons again and again and again. Yet get no response. This makes it *much* easier to change defaults in "generic". Instead of needing to touch a bunch of configurations with different versions, you can simply touch all configurations with one version. If you're unlucky and a new kernel cycle starts before approval or rejection, you can rebase one copy onto the rename commit and another onto the merge commit. You then rebase these on top of each other and then squash, the result is you're onto the latest with minimal trouble. Without this trying to modify values effecting multiple devices is an absolute nightmare (believe me, I know!). Hmm, afaik this is what openwrt does right now, the generic config is applied to all targets, and you put your device specific configuration in your platform config. This makes sense, because you don't want to compile all drivers for all targets into your tiny router image, that is restricted to 4MiB. So if there's something I'm missing, I do appologize :) Uh huh. If we were playing horseshoes with thermonuclear handgrenades, that response would get 0 points. You completely missed the point. Let me add another advantage of doing everything at once. The generated commits do not properly rebase. Rebasing retains much of the advantage, but degrades things. But we neve rebase master? So this would only affect the local changes for the developer? I do admit 'rebasing the local tree to get the latest status' is something needed for sure ... I'll check what happens if I do that as I'm curious how and what breaks. As such doing all the duplication in one event allows someone who can directly commit do the task for everyone *once* per update cycle (presently once per year). Device maintainers can then base off of that point and not worry about having non-rebasable commits. Then there are the things `kernel_upgrade.pl` can do which `kernel_bump.sh` has no equivalent. But what are they. And how are they relevant? You've been typing about how yours could upgrade everything by being called multiple times. Since I was aiming to get the above issue discussed `scripts/kernel_upgrade.pl` has featured the capability to update everything all at once, from the start. The kitchen sink :) But honestly, have that discussion first with the maintainers. Hauke is already much against it; which I get. Having worked on a few targets, they are so diverse, bumping two at once just doesn't make sense (today!). I will not fully type up my viewpoint. Bisecting inherently involves heuristics. There may be an even number of changes between any two points, so bisecting involves arbitrarily choosing from a set. The `git bisect skip` command exists because it *cannot* be perfect. Meanwhile --find-copies-harder is sufficiently slow to call `git blame` effectively broken on OpenWRT. It is too slow to be used for most of its intended use cases. In fact only upgrading a single board was a feature which had to be added. Since I added fewer assumptions, mine makes no distinction between upgrading targets versus subtargets. It can do multiple of both at the same time without any restrictions and a single invocation. In the process, only 2 commits are generated. The under .5s is for updating *everything*. I have no idea how long it would take to update everything, but again, the time factor is so irrelevant, it doesn't atter. Also, I too only have 2 commits now, which until we get `git cp` will remain the minimum, but more then a 'cp && git add'. I haven't tested, but isn't that simply abandoning the approach of having history on everything and resorting to only having history on the up to date version? I did think that was a viable approach, but it isn't what seemed to gain concensus. ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Purpose of openwrt-devel?
On 23-03-2024 18:51, Elliott Mitchell wrote: On Sat, Mar 23, 2024 at 03:15:44AM +0100, Olliver Schinagl wrote: True enough. Though I will cite this as an example of the care used in the design. I can't argue the design, not because I can't read it; but because I'm sure you've put a lot of care and attention into it, hence your pation for it :) Odd thing about what you put in parentheses. I've been trying to get a discussion going about that for months, yet seems no one notices the suggestion. This was a distinct goal of the script, to hopefully get that discussion going. To update all targets at once? How is that useful?! Taking the unusual step of splitting a paragraph since that is needed in this case. I've cited two specific reasons in a recent message. I keep citing those reasons again and again and again. Yet get no response. This makes it *much* easier to change defaults in "generic". Instead of needing to touch a bunch of configurations with different versions, you can simply touch all configurations with one version. If you're unlucky and a new kernel cycle starts before approval or rejection, you can rebase one copy onto the rename commit and another onto the merge commit. You then rebase these on top of each other and then squash, the result is you're onto the latest with minimal trouble. Without this trying to modify values effecting multiple devices is an absolute nightmare (believe me, I know!). Hmm, afaik this is what openwrt does right now, the generic config is applied to all targets, and you put your device specific configuration in your platform config. This makes sense, because you don't want to compile all drivers for all targets into your tiny router image, that is restricted to 4MiB. So if there's something I'm missing, I do appologize :) Without a doubt copying the configurations and patches is only a single trivial step. Yet unless you're aware of a board/device which doesn't copy configurations and patches as an early step, this is no reason not to do them all at once. For that matter, if it is such a small step why bother with a script? Ah, yes, well the initial idea was 'lets put it on the wiki' but then I can assure you, noone will do it (unless maintainers are super ademant. Then there's the debugging 'you did it wrong, please do it right'. It's again, all about the human factor here. Telling some one 'just run this script, done' makes it less error prone and easier to do. Nothing more, nothing less. I think the goal is for it to _be_ super simple. That's probably a step too far, this is using magical got internals. But sure. I'd thing someone sees the git mv, git commit, git checkout and figures ohh, i think i get it', but of course to understanf deeper research is still needed. I just disagree with making things (appear to the general reader) very complex, and then expect them to research the theory is to far. Yet all this leaves me concerned about assumptions being made. I've already pointed to one example (it assumes you're on a local branch). While the name is not the sort most humans would use, it is still creating and deleting a branch when it doesn't need to. I've got an odd suspicion it will start to require more features of your development environment. But why would you not do this as a human? That's what the original instructions where, so that's what humans would do, right? Checkout a branch, do the work, merge it back. So you found a better way, which is amendable. However it's too hard to grok for _most_ humans :p But for what it's worth, I've put up the question on the ML whether it would be usefull to have `git cp` and solve it in a far nicer way :) Doubt we'll see it before the end of the year :) If you examine the result, you might also discover its approach has some rather substantial advantages. At this time I believe with the second commit it offers a proper superset of your script's functionality. I wonder what this super set is though and why it is so badly needed ... Your knowledge level is showing. I've had set theory in units yes. But its still not clear which superior features the Perl version offers. How is it massivly better. How is the result majorly different? What is so super (pun intended) in your approach? The UI approach for `kernel_upgrade.pl` is rather distinct from what `kernel_bump.sh` has. I'm unsure how closely what it does matches the behavior of your script. Yet the modified `kernel_bump.sh` performs similar to what you have, by invoking `kernel_upgrade.pl` once with appropriate arguments. I'll test it soon ;) Just got some personal stuff to deal with ... You're not the only person who doesn't devote 100% time to OpenWRT. Theory was the better capabilities would become clear with a bit of experimentation. Then there are the things `kernel_upgrade.pl` can do which `kernel_bump.sh` has no equivalent. But what are the
Re: Purpose of openwrt-devel?
Heyb Elliot, On March 21, 2024 10:28:29 p.m. GMT+01:00, Elliott Mitchell wrote: >On Thu, Mar 21, 2024 at 10:00:46AM +0100, Olliver Schinagl wrote: >> On 20-03-2024 01:34, Elliott Mitchell wrote: >> > On Mon, Mar 18, 2024 at 10:53:12AM +0100, Olliver Schinagl wrote: >> >> I expect this to be done very rarely and by users that know what they >> >> are doing, but just "automating" a few logical git commands. >> >> >> >> Performance is not a key-driver here. It's too rarely used. >> > True, though being faster is nice. >> >> While true, I don't think we even have to start arguing if runtime is >> less then a single second. This on my 12 year old PC, granted, the CPU >> is only 8 years and the nvme SSD only 5. >> >> ./scripts/kernel_bump.sh -p realtek -s 5.15 -t 6.6 0,40s user 0,33s >> system 105% cpu 0,694 total >> >> ./scripts/kernel_bump.sh -p ramips -s 6.1 -t 6.6 0,40s user 0,40s >> system 106% cpu 0,753 total >> >> Even if you bumped all repo's (with a dumb for-loop) we'd be talking 30 >> seconds to do _all_ of them at once (which never happens). > >On a computer of similar class, but with *much* slower storage >(fileserver is sick and underperforming): real0m0.477s > >So if this was directly to an SSD, 2 orders of magnitude. Sure, but not significant in any way or form. Its still in both cases significantly less then one second. The script has executed before my finger has left the enter key. There is no purpose or advantage here. If the script ran 10 seconds, it wouldn't even matter IMO. > > >Odd thing about what you put in parentheses. I've been trying to get a >discussion going about that for months, yet seems no one notices the >suggestion. This was a distinct goal of the script, to hopefully get >that discussion going. To update all targets at once? How is that useful?! If a target is fully upstream, there is nearly nothing to migrate, no patches etc. So maybe the kernel config. Sure expanding (either script) to accept multiple platforms would be trivial or accept a commit pair per platform andere just loop over the script fort each target. But this is something not feasible for decades to come. Bumping a kernel version pretty much always requires additional work. Config migration, rebasing patches, testing on actual hardware. Its just not even worth considering. Also, a quick note on skipping kernel versions, generally openwrt seems to only support two versions at a time. You could have just a single one, or skip 5. The problem/work, is adapting the target to actually function again. Bigger jumps just means different/more work on the patches, but nothing else really. > > >> >> Leaving the tree in failed state imo is a feature. We switch from the >> >> normal branch to a special branch to do all operations. The user can >> >> always force ably switch back. Ultimately, this is a choice, can a user >> >> fix things and inspect failures, or 'oh it failed, lets reset'. Reset >> >> instructions during cleanup is a good idea however. >> > Therein lines a concern. Why does yours switch to a special branch? >> > It is not human, it doesn't need a computer to keep track of commits for >> > it. As such it shouldn't need a branch. >> >> Why is this a problem? Why can't a script that is intended to remove >> manual labor, behave like a human. There comes the readability and >> maintainability argument once again. If a human can read it, he can >> modify it. If the script fails, or a special case pops up, a human can >> do those steps manually quite easily. >> >> I'm a big beliver in KISS. So yes, the script is not perfect and doesn't >> have shiney gold plated parts. But it is simple, can be understood by a >> human, by a non-developer even I'd argue. >> >> In the end, computers do what humans tell them to do. In the end, humans >> reading things is far more important, then super-optimizing a script, >> that's run once or twice a year by a human developer. >> >> And using a branch does have its advantages too. We can switch to the >> branch and examine if things go wrong. Again, this is something a human >> would do too :) > >A human can tell `git` to move to an unreferenced commit. Useful to know >how if things go wrong. Though I will admit by having your script be so >close to things many people do, does make it more obvious to more people. > Yeo yhats my point, humane neef tot read, write, understand whats ging on, at least in general ways. >Yet if that is an issue they should be looking at the URL where the >approach came from and readi
Re: Purpose of openwrt-devel?
On 20-03-2024 01:34, Elliott Mitchell wrote: On Mon, Mar 18, 2024 at 10:53:12AM +0100, Olliver Schinagl wrote: I expect this to be done very rarely and by users that know what they are doing, but just "automating" a few logical git commands. Performance is not a key-driver here. It's too rarely used. True, though being faster is nice. While true, I don't think we even have to start arguing if runtime is less then a single second. This on my 12 year old PC, granted, the CPU is only 8 years and the nvme SSD only 5. ./scripts/kernel_bump.sh -p realtek -s 5.15 -t 6.6 0,40s user 0,33s system 105% cpu 0,694 total ./scripts/kernel_bump.sh -p ramips -s 6.1 -t 6.6 0,40s user 0,40s system 106% cpu 0,753 total Even if you bumped all repo's (with a dumb for-loop) we'd be talking 30 seconds to do _all_ of them at once (which never happens). Leaving the tree in failed state imo is a feature. We switch from the normal branch to a special branch to do all operations. The user can always force ably switch back. Ultimately, this is a choice, can a user fix things and inspect failures, or 'oh it failed, lets reset'. Reset instructions during cleanup is a good idea however. Therein lines a concern. Why does yours switch to a special branch? It is not human, it doesn't need a computer to keep track of commits for it. As such it shouldn't need a branch. Why is this a problem? Why can't a script that is intended to remove manual labor, behave like a human. There comes the readability and maintainability argument once again. If a human can read it, he can modify it. If the script fails, or a special case pops up, a human can do those steps manually quite easily. I'm a big beliver in KISS. So yes, the script is not perfect and doesn't have shiney gold plated parts. But it is simple, can be understood by a human, by a non-developer even I'd argue. In the end, computers do what humans tell them to do. In the end, humans reading things is far more important, then super-optimizing a script, that's run once or twice a year by a human developer. And using a branch does have its advantages too. We can switch to the branch and examine if things go wrong. Again, this is something a human would do too :) This is one of the things I meant by "Your shell script is essentially replicating the actions a human at a shell might take to perform the task". Some things work better for a shell script than the approaches humans would use. As I wrote above, I think this is _better_ anyways :) Since I went to the trouble of examining your script and testing it, might you reciprocate with similar courtesy? Even if you cannot fill the role of reviewer, you can still fill the role of tester. You are correct, your script is far to big and complex to review (not only for someone who doesn't know perl :p) but I will try to make some room to run it. If you examine the result, you might also discover its approach has some rather substantial advantages. At this time I believe with the second commit it offers a proper superset of your script's functionality. I wonder what this super set is though and why it is so badly needed ... Olliver ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Purpose of openwrt-devel?
On 18-03-2024 01:34, Elliott Mitchell wrote: On Sun, Mar 17, 2024 at 11:44:45AM +0100, p...@oranjevos.nl wrote: Op 16 mrt. 2024, om 07:46 heeft Elliott Mitchell het volgende geschreven: On Sat, Mar 16, 2024 at 02:19:27PM +0800, Chuanhong Guo wrote: And more comments on the perl thing: A maintainer needs to be familiar with perl to review or take your patches. I could probably vaguely understand what a perl script is doing by quickly learning the syntax, but I can't decide whether the script is good or not. Nobody is explicitly NACKing your patch or saying it's worse than the bash version just because it's written in perl. Maintainers who don't understand perl simply don't have the knowledge to judge the script, so the patch is left for others. If such a maintainer doesn't show up, your patch won't be taken. It doesn't matter if your script is superior or not. That makes forward progress impossible. If it provides superior results then perhaps the thing which only one person understands is acceptable? As long as they maintain it, provide reasonable explanations and help others work on gaining proper understanding, isn't that good enough? Ever considered to implement the kernel bumps based on 'git fast-import' in sh script in stead of perl ? Well... The choice of Perl+fast-import was guided by my aims. I wanted to do as little as possible as possible to the working tree in order to reduce problems from someone trying the script in a dirty tree. For POSIX shell this simply isn't so advantageous. Unfortunately you've caused me to wonder about it a bit, so... First thoughts. Should be possible. This isn't nearly so fast or robust since fast-import is a *binary* protocol, *not* a text protocol. In particular it uses line-feeds, *not* newlines (subtle, but critical difference). Second thought. Pretty difficult. Perl was simple due to being able to open a pipe and leave it around stuck to a variable. Shell isn't really well-suited to this. Third thought. Above I was thinking of an approach similar to what I did with Perl. If instead a more traditional fast-import fixed stream approach was used, this is actually suitable for shell operation. So, yes indeed shell+fast-import is quite doable. I'm unsure of it being particularly advantageous. This would need a *bunch* of temporary files to hold intermediate work before merging everything together. My goal though was to do the job well, not to show off fast-import. I expect this to be done very rarely and by users that know what they are doing, but just "automating" a few logical git commands. Performance is not a key-driver here. It's too rarely used. Users that do not know what they are doing? Are they the ones doing kernel bumps? I will add a little warning/error when the kernel tree is a non-clean state, and prevent the script from running. But going back (or trying to fix a failure) should still b e preferred, to avoid starting the process when it's already known it can't continue. Leaving the tree in failed state imo is a feature. We switch from the normal branch to a special branch to do all operations. The user can always force ably switch back. Ultimately, this is a choice, can a user fix things and inspect failures, or 'oh it failed, lets reset'. Reset instructions during cleanup is a good idea however. ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Purpose of openwrt-devel?
Hey Elliott, On 14-03-2024 01:50, Elliott Mitchell wrote: On Wed, Mar 13, 2024 at 09:43:06AM +0100, Olliver Schinagl wrote: On 13-03-2024 08:46, Felix Baumann wrote: Am 13. März 2024 05:11:23 MEZ schrieb Elliott Mitchell: I must challenge this. If patches via the mailing list were accepted, then we should see things sent to the mailing list getting into the repository. Yet many patches get no attention. Some get reviews from various people, yet then never get into the main repository. It's the same for Github, some stuff doesn't get in and remains there. There might be a difference what kind of PRs are send to the mailing list and you get attention of different committers when sending to mailing list vs sending to Github. Github patches might be accepted more easily when it's just a new device for a well established target. I feel like patches on the mailing list are ignored, when committers don't have time for review or don't feel confident enough to do it well (not their field of expertise). Or if it's written in a language they don't feel confident reviewing. *PERSONALLY* I think mailing list reviews are on their way out. People have found that there are easier and better ways. Granted, some folks still _prefer_ mailing list reviews. *I PERSONALLY* do not at all. I hate my mailbox being full with threads of stuff I have no attention for at that moment, it just adds noise for me. And ignoring it for a while just puts huge amounts of e-mails in my mailbox, that become useless after a while. Though I much rather would like to see GitLab then GitHub use :p but that's more the FOSS spirit, and avoiding anything Microsoft where possible :p Mailing list reviews do have their moments. Notably I thought some parts might deserve wider discussion. Also by sending it here I was trying to engage with the person who originally found the solution. I am another person who is concerned about GitHub. The degree of copyright infringement by AI isn't known to be large, but there are hints of trouble on the horizon. Good news is Git is very much P2P and moving things between Git servers is easy. But as Daniel pointed out, that's just the code, everything else valueable, you "loose". Issues, reviews, CI etc etc. But see my reply to Daniel for more agreement. Huh. Parts of that look suspicious. Those commit messages look *very* similar to my version 2. I was jumping between documentation sources when writing it. Not sure what is surprising to you, since the mail thread was listed in the MR and your perl code was even referenced (not _directly_ I admit). Obviously I was using your messaging format as that was discussed on the mailing list and I didn't want to deviate from those messages, also they made a lot of sense anyway. "Fair Use" if anything :p A Court of Law would need to decide Fair Use, but I'm pretty sure this would fail. Good news is this isn't enough to bother. The actual code of course has nothing to do with the perl script, as you right full say 'I know nothing of perl', as does probably most of the development community by now. Which is sad for perl, but 'it is what it is'. In no way was there any ill intent. I just wanted my kernel tree bump for the realtek target, and didn't want to install, learn etc perl to try things out. Sorry for that on my part. The real problem here is you made two critical errors in your handling of this. But we're only talking textual strings, English language. Maybe I came up with nearly similar sounding things because a) English is not my native language and b) I've seen this before 'somewhere on the internet. So re-using your 'code' is a bit of a stretch. Or are you suggesting I am plagarizing everybody with these words I am writing right here? They where all used too. ChatGPT could have been used. Your code in itself, I didn't use, or look at really. First, credit the original author for everything. Open source depends heavily on reputation so letting people know doing this as a script was my idea has high value to me. I take the above as an apology, so there there is little to do, besides drop this portion. Thank you! Though to be fair, the mailing list posted, with the 'manual git way of doing it' was used :p Second, I knew nothing of https://github.com/openwrt/openwrt/pull/14713 until yesterday and Robert Marko mentioning something, triggering me to go searching: https://lists.openwrt.org/pipermail/openwrt-devel/2024-March/042423.html Had I known of the pull earlier I would have gone to #14713, but I had no idea the discussion had been diverted away to GitHub. I suppose that is one of the issues being discussed already, two distinct review tracks. In light of this, I think the qualities of the two scripts and their merits should be considered. Sure thing, as I wrote on github; we can always interate and improve on a v2. One item I was trying to draw attention to was
Re: Purpose of openwrt-devel?
Hey Daniel, On 14-03-2024 02:40, Daniel Golle wrote: On Wed, Mar 13, 2024 at 05:50:36PM -0700, Elliott Mitchell wrote: Anyway. The true dependency on Github is everything they offer besides git: Bug tracker, Pull Requests, comments made on PRs, comments made on commits, ... scraping all that data is much more tricky should we ever desire to move that somewhere else. Indeed. And while I certainly won't intend to dig up the GitLab vs github discussion, it's been had, it's been done, it's been decided. But this is what might have been missed in the earlier discussion. "Picking up and moving elsewhere" is much much harder now. With Gitlab (be it freedesktop's or whatever), you can export and self-host, keeping everything as yours. And, afaik gitlab allows a more seemless mailinglist + gitforge integration. But the ship is sailed, so lets not dwell on this further (unless others seriously want to re-consider :p) Imho for people who don't like following submitted patches as emails in their inbox there is patchwork. I think patchwork is an atroctity to work with, but that's comming from someone who doesn't use it much and doesn't understand it, so I'm certainly biased. Having everything in two places -- Github and patchwork -- certainly isn't perfect and I'd also rather want to see all that in a single one-size-fits-them-all interface like sourcehut. However, I also got neither resources nor experience in hosting such as service in the scale we would need. Afaik gitlab does have mailing list integration-ish support. But again, it was decided, and until we re-evaluate, it is what it is. And those are my 2 ;) Olliver Just my 2 cents. Huh. Parts of that look suspicious. Those commit messages look *very* similar to my version 2. I was jumping between documentation sources when writing it. Not sure what is surprising to you, since the mail thread was listed in the MR and your perl code was even referenced (not _directly_ I admit). Obviously I was using your messaging format as that was discussed on the mailing list and I didn't want to deviate from those messages, also they made a lot of sense anyway. "Fair Use" if anything :p A Court of Law would need to decide Fair Use, but I'm pretty sure this would fail. Good news is this isn't enough to bother. The actual code of course has nothing to do with the perl script, as you right full say 'I know nothing of perl', as does probably most of the development community by now. Which is sad for perl, but 'it is what it is'. In no way was there any ill intent. I just wanted my kernel tree bump for the realtek target, and didn't want to install, learn etc perl to try things out. Sorry for that on my part. The real problem here is you made two critical errors in your handling of this. First, credit the original author for everything. Open source depends heavily on reputation so letting people know doing this as a script was my idea has high value to me. I take the above as an apology, so there there is little to do, besides drop this portion. Second, I knew nothing of https://github.com/openwrt/openwrt/pull/14713 until yesterday and Robert Marko mentioning something, triggering me to go searching: https://lists.openwrt.org/pipermail/openwrt-devel/2024-March/042423.html Had I known of the pull earlier I would have gone to #14713, but I had no idea the discussion had been diverted away to GitHub. In light of this, I think the qualities of the two scripts and their merits should be considered. One item I was trying to draw attention to was perhaps the current approach to handling kernel version changes isn't very good. If you're trying to change configuration which effects multiple devices, you end up having to *constantly* watch out for configuration files appearing and disappearing. This is painful to deal with. This has the useful advantage of producing fewer merge commits. Instead of 44 merges/kernel version, there would be only 1. I estimate OpenWRT is doing around 3000 commits/year, so this is 1.5% versus 0.03% commit merges. If a typical bisect session touches 12 commits then this is 16.5% versus 0.45% encountering a merge. Then there are sessions which encounter 2+ merges. The biggest difference between the two isn't the language choice, but the overall designs. Your shell script is essentially replicating the actions a human at a shell might take to perform the task (more or less precisely the actions suggested by the original source). As such you likely recognize all the commands used in the script. My Perl script uses a very different approach. Pretty much all of Git's interface is designed to be readily parseable and useful for scripting. Some portions are really meant /only/ for script use and not meant for interactive use. The real reason everyone was having a hard time understanding my script was not that it was written in Perl. The real reason everyone was having a hard time with the
Re: Purpose of openwrt-devel?
On 13-03-2024 08:46, Felix Baumann wrote: Am 13. März 2024 05:11:23 MEZ schrieb Elliott Mitchell: I must challenge this. If patches via the mailing list were accepted, then we should see things sent to the mailing list getting into the repository. Yet many patches get no attention. Some get reviews from various people, yet then never get into the main repository. It's the same for Github, some stuff doesn't get in and remains there. There might be a difference what kind of PRs are send to the mailing list and you get attention of different committers when sending to mailing list vs sending to Github. Github patches might be accepted more easily when it's just a new device for a well established target. I feel like patches on the mailing list are ignored, when committers don't have time for review or don't feel confident enough to do it well (not their field of expertise). Or if it's written in a language they don't feel confident reviewing. *PERSONALLY* I think mailing list reviews are on their way out. People have found that there are easier and better ways. Granted, some folks still _prefer_ mailing list reviews. *I PERSONALLY* do not at all. I hate my mailbox being full with threads of stuff I have no attention for at that moment, it just adds noise for me. And ignoring it for a while just puts huge amounts of e-mails in my mailbox, that become useless after a while. Though I much rather would like to see GitLab then GitHub use :p but that's more the FOSS spirit, and avoiding anything Microsoft where possible :p Even the Linux kernel (forgive me for omitting the link, though I can find it if really needed, they are just not easy google terms) is discussing this; but there's a few technicalities holding them back for the most part (See Linus's rant against github a few years ago, but diff-range, comment on commit-message are two key points). Further more, I think it is fair to realize that very few developers that exist, as said before, prefer different ways of working. This sucks, but means we have two 'groups' of reviewers in two different locations. In the end it's up to one committer to do the merge. If Noone replies, then that doesn't happen and the patch will only collect dust. Yes, that might be stupid if there was no critical comment on a patch but that is how it currently appears to be. This is still voluntary work and people choose themselves what to spend time on. I realize this is not a satisfying answer. I won't comment on how your code was used as a base for the script earlier this year. I'm not involved enough in the project to handle this. Regardless of copyright (since I'm just a layman): people didn't tell you about it or mentioned you in the commit that was merged. On a social level that was not okay, that is all I can say. I'm sure Noone meant harm but still: Noone thought about consequences. I cc-ed Olliver to let him know about this mail thread. Thanks! In light of that Huh. Parts of that look suspicious. Those commit messages look *very* similar to my version 2. I was jumping between documentation sources when writing it. Not sure what is surprising to you, since the mail thread was listed in the MR and your perl code was even referenced (not _directly_ I admit). Obviously I was using your messaging format as that was discussed on the mailing list and I didn't want to deviate from those messages, also they made a lot of sense anyway. "Fair Use" if anything :p The actual code of course has nothing to do with the perl script, as you right full say 'I know nothing of perl', as does probably most of the development community by now. Which is sad for perl, but 'it is what it is'. In no way was there any ill intent. I just wanted my kernel tree bump for the realtek target, and didn't want to install, learn etc perl to try things out. Sorry for that on my part. Regards Felix Baumann ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Linux kernel 6.1 or 6.6 for OpenWrt 24.x release?
Hi, I track the status of the Linux kernel 6.1 migration in this github issue:https://github.com/openwrt/openwrt/issues/14546 There are still many targets on kernel 5.15 without testing support for kernel 6.1 in OpenWrt master. I assume that we need at least 4 months to get everything to 6.1 and more or less stable. Kernel 6.1 support is also missing for some important targets like lantiq, realtek and ramips. Which kernel should we use for the next major OpenWrt release? We have two options and I would like to get some feedback on these: 1. Do the OpenWrt 24.X release with kernel 6.1. Branch off when all or most of the targets are on kernel 6.1 by default. 2. Do the OpenWrt 24.X release with kernel 6.6. Branch off when all or most of the targets are on kernel 6.6 by default. Do not do any stable OpenWrt release which supports kernel 6.1. Doing a OpenWrt release with multiple kernels cases too much maintenance effort from my point of view based on previews experience. I think with kernel 6.1 we can branch off at around May 2024. With kernel 6.6 we could probably branch off around September 2024. The final release will be out about 2 to 4 months later. Currently OpenWrt releases are about 1.5 years behind the Linux LTS releases. When we use kernel 6.1 for the next release we will continue to stay 1.5 years behind. When we switch to kernel 6.6 and do not do any release with kernel 6.1 we will probably only stay 10 months behind Linux LTS kernels. There is already a PR requiring kernel 6.6: https://github.com/openwrt/openwrt/pull/14357 Currently I would prefer to use kernel 6.6 to get closer to the recent Linux LTS releases. Hauke As a developer, having had to do the 5.15 to 6.1 migration, I was frustrated to see that 6.6 was about to be released and knew I had to do the same work + more _again_. So from a developers point of view, Being closer to mainline also means potentially fewer times doing the work. Also having a smaller gap is always much nicer. While I get that some people prefer to make smaller steps or what not, they often forget the amount of work these transitions can be. And while one can argue, 'but you have to do the same work anyway, just in smaller steps, it's also about time and focus. Spending an hour extra during a 2 hour session to make the step, is better then having to spend 2x 1 hour. So yes please, 6.6 asap :) ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFC PATCH 4/7] realtek: add RTL8380 switch port LED driver
On 30-07-2022 00:02, Sander Vanheule wrote:
On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
On 16-07-2022 21:09, Sander Vanheule wrote:
RTL8380 switch SoCs can control up to 3×28 LEDs to display status of the
switch's ethernet port. This driver allows to address these LEDs and
provides direct control, blink offloading, and switch port status
offloading.
Since offloading of the generic netdev trigger does not exist, this
driver provides a private trigger which achieves the same and is named
"realtek-switchport". Each LED will have a file name "rtl_hw_trigger" in
/sys/class/leds/$LED, where the requested trigger mode can be written
to. If an unsupported mode is requested, this will be reported to the
user. When the trigger is then activated, the LED will be added to a
group of LEDs with the same trigger conditions. If it is not possible to
add the LED to a group, the user must use an already existing trigger
condition, or use direct LED control with a software trigger.
Some common modes (i.e. values for "rtl_hw_trigger") are:
- Link present, blink on activity: 1f
- 100M/10M link, blink on activity: f
- 1G link present: 10
Signed-off-by: Sander Vanheule
---
.../drivers/pinctrl/pinctrl-rtl-switchcore.c | 370 ++
...s-add-RTL8380-switch-port-LED-driver.patch | 64 +++
target/linux/realtek/rtl838x/config-5.10 | 1 +
3 files changed, 435 insertions(+)
create mode 100644
target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-
switchcore.c
create mode 100644
target/linux/realtek/patches-5.10/202-leds-add-RTL8380-switch-
port-LED-driver.patch
diff --git
a/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
b/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
new file mode 100644
index ..0caf925989b6
--- /dev/null
+++ b/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include
for ARRAY_SIZE()
+#include
Will add.
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "core.h"
+#include "pinmux.h"
+
+/**
+ * struct rtl_swcore_mux_desc - switchcore pin group information
+ *
+ * Pins are frequently muxed between alternative functions, but the control
+ * bits for the muxes are scattered throught the switchcore's register space.
+ * Provide a regmap-based interface to flexibly manage these mux fields, which
+ * may vary in size and do not always provide a GPIO function.
+ *
+ * @name: name to identify the pin group
+ * @field_desc: description of the register field with mux control bits
+ * @functions: NULL terminated array of function names
+ * @pins: array of numbers of the pins in this group
+ * @npins: number of pins in this group
+ */
+struct rtl_swcore_mux_desc {
+ const char *name;
+ struct reg_field field;
+ const unsigned int *pins;
+ unsigned int npins;
+};
+
+#define SWITCHCORE_MUX(_name, _field, _pins) { \
+ .name = (_name),\
+ .field = _field,\
+ .pins = (_pins),\
+ .npins = ARRAY_SIZE(_pins), \
+ }
+
+/**
+ * struct rtl_swcore_mux_setting - stored mux setting
+ *
+ * @mux: pointer to the mux descriptor
+ * @value: value to write in the mux's register field to apply this setting
+ */
+struct rtl_swcore_mux_setting {
+ const struct rtl_swcore_mux_desc *mux;
+ unsigned int value;
mux_setting? value is so ... undefined ;)
That's because it is very generic, and just contains a register field value
The struct is already named mux_setting and mux_setting->mux_setting is too much
redundancy IMHO.
yeah true, but `value` is still a weird name, it must represent
'something' right? In any case remember that naming things is hard ;)
but a good name can make quite the difference. I fully agree having
mux_setting->setting is dumb,
what about rtl_swcore_mux_ctrl->mux_setting? idk; just thinking out-loud
here.
+};
+
+/**
+ * struct rtl_swcore_function_desc - switchcore function information
+ *
+ * @name: name of this function
+ * @settings: list of mux settings that enable this function on said mux
+ * @nsettings: length of the @settings list
+ */
+struct rtl_swcore_function_desc {
+ const char *name;
+ const struct rtl_swcore_mux_setting *settings;
+ unsigned int nsettings;
+};
+
+#define SWITCHCORE_FUNCTION(_name, _settings) { \
+ .name = (_name),\
+ .settings = (_settings),\
+ .nsettings = ARRAY_SIZE(_settings), \
+ }
+
+struct rtl_swcore_config {
+ const struct pinctrl_pin_d
Re: [RFC PATCH 5/7] realtek: rtl838x: replace pinctrl-single
On 29-07-2022 23:41, Sander Vanheule wrote:
On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
On 16-07-2022 21:09, Sander Vanheule wrote:
Replace the pinctrl-single node with the dedicated pinctrl driver for
RTL838x SoCs. The node names are kept to stay compatible with existing
references.
Signed-off-by: Sander Vanheule
---
target/linux/realtek/dts-5.10/rtl838x.dtsi | 38 ++
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/target/linux/realtek/dts-5.10/rtl838x.dtsi
b/target/linux/realtek/dts-
5.10/rtl838x.dtsi
index 11cabc3f63cb..6aac2be95368 100644
--- a/target/linux/realtek/dts-5.10/rtl838x.dtsi
+++ b/target/linux/realtek/dts-5.10/rtl838x.dtsi
@@ -169,33 +169,29 @@
};
};
- pinmux: pinmux@1b001000 {
- compatible = "pinctrl-single";
- reg = <0x1b001000 0x4>;
+ switchcore: switchcore-bus@1b00 {
+ compatible = "realtek,rtl8380-switchcore", "syscon";
+ reg = <0x1b00 0x1>;
- pinctrl-single,bit-per-mux;
- pinctrl-single,register-width = <32>;
- pinctrl-single,function-mask = <0x1>;
- #pinctrl-cells = <2>;
+ hw_led_sys: led-sys {
+ status = "disabled";
- enable_uart1: pinmux_enable_uart1 {
- pinctrl-single,bits = <0x0 0x10 0x10>;
+ label = "green:status";
};
- };
- /* LED_GLB_CTRL */
- pinmux_led: pinmux@1b00a000 {
- compatible = "pinctrl-single";
- reg = <0x1b00a000 0x4>;
+ pinctrl {
+ compatible = "realtek,rtl8380-pinctrl";
- pinctrl-single,bit-per-mux;
- pinctrl-single,register-width = <32>;
- pinctrl-single,function-mask = <0x1>;
- #pinctrl-cells = <2>;
+ /* enable GPIO 0 */
+ pinmux_disable_sys_led: sys-led-mux {
+ groups = "sys-led";
+ function = "gpio";
+ };
with your changes, why not use the hw function?
Because I am of the opinion that 64ms or 1024ms toggle interval is
insufficient. And then
we have to hope that OpenWrt will always choose the fast/slow blinking
intervals in such a
way that it ends up with the correct selection. Otherwise there will be no way
for the
user to distinguish the enter-failsafe moment from the rest of the boot.
I actually agree strongly with you here after giving it some more
thought. For example, I want my LED to be first (obviously) be managed
by openwrt for _consistent_ openwrt behavior; and then once the board is
up; I want the power-led to be an 'inverted heartbeat'. So yes, having
it by default be a GPIO, and then later let the user use some hardware
accelerated functionality, makes a lot more sense, which I doubt we'll
ever need.
Suppose it is useful for the boot scenario's, unmanaged switches etc,
where you don't have an OS running yet, but other then that, I agree the
hw accelerated blink doens't offer much once linux is running and can do
a much better job.
(out of context here, but the same can be said for the netdev LEDs,
having them initially as GPIO's or manually controlled leds is better,
as that also allows a startup script to run some sort of LED tests we
often see switches doing).
- /* enable GPIO 0 */
- pinmux_disable_sys_led: disable_sys_led {
- pinctrl-single,bits = <0x0 0x0 0x8000>;
+ enable_uart1: uart1-mux {
_uart1 { status=disabled }; reads weird though :p
This is just here for easy use later. It doesn't actually do anything until
another node
request this in their pinctrl property.
I meant the name of the alias :)
+ groups = "uart1";
+ function = "uart1";
+ };
};
};
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFC PATCH 3/7] realtek: add pinctrl for Realtek maple
On 29-07-2022 23:34, Sander Vanheule wrote:
On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
On 16-07-2022 21:09, Sander Vanheule wrote:
Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
RTL838x has been tested, but functionality should be near identical.
Since control bits for different muxes are scattered throughout the
switch core's register space, pin control features are provided by a
table of regmap fields. This should be flexible enough to allow
extension to other SoC generations providing similar features.
Signed-off-by: Sander Vanheule
---
.../drivers/leds/leds-rtl-switch-port.c | 668 ++
...inctrl-add-pinctrl-for-Realtek-maple.patch | 51 ++
target/linux/realtek/rtl838x/config-5.10 | 1 +
3 files changed, 720 insertions(+)
create mode 100644
target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-
port.c
create mode 100644
target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-
Realtek-maple.patch
diff --git a/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
new file mode 100644
index ..76dfede7ba15
--- /dev/null
+++ b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
@@ -0,0 +1,668 @@
+// SPDX-License-Identifier: GPL-2.0
+
Probably want bitops.h for BIT()
+#include
Will add.
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/*
+ * Realtek switch port LED
+ *
+ * The switch ASIC can control up to three LEDs per phy, based on a number of
+ * matching conditions. Alternatively, each individual LED output can also be
+ * configured for manual control.
+ */
Is this an internal define? `REALTEK_PORT_LED_TRIGGER_NONE` (etc) maybe
better? (or LEDTRIG if it really must be shorter)
It wasn't until much later that I figured out that this was the
abbreviation for port trigger *doh*
I got tired of typing so much by prefixing everything with REALTEK_... This is
in fact an
internal definition, used only for the private trigger.
The translation between DT trigger flags and the actual HW trigger settings may
look like
an unnecessary complication here, but I've done this with broader compatibility
in mind.
On RTL93xx, the HW trigger also uses (different) bit flags [1], instead of an
enumeration
of triggers [2]. Other hardware, from other vendors, would also use a different
aproach,
but there is currently no framework yet to offload netdev triggers.
[1] https://svanheule.net/realtek/longan/register/led_set0_0_ctrl
[2] https://svanheule.net/realtek/maple/register/led_mode_ctrl
+#define PTRG_NONE 0
+#define PTRG_ACT_RXBIT(0)
+#define PTRG_ACT_TXBIT(1)
+#define PTRG_ACT PTRG_ACT_RX | PTRG_ACT_TX
+#define PTRG_LINK_10 BIT(2)
+#define PTRG_LINK_100 BIT(3)
+#define PTRG_LINK_1000 BIT(4)
These flags should actually go into a header somewhere, but this was still
development
code.
+
+struct led_port_blink_mode {
+ u16 interval; /* Toggle interval in ms */
+ u8 mode; /* ASIC mode bits */
+};
+
+#define REALTEK_PORT_LED_BLINK_COUNT 6
+struct led_port_modes {
realtek_led_port_modes?
+ u8 off;
+ u8 on;
+ struct led_port_blink_mode blink[REALTEK_PORT_LED_BLINK_COUNT];
ah here's the other list. So would it make sense to re-use this same
structure for the sys-led, or is that over-optimizing things ...
Like I noted on the other patch, it could make sense, yes.
Why though are on/off and the blink entries separated? Isn't 'off' a
interval of 'inviite (max-int)' and on a blink rate of 0? While you may
have to do some logic later, it keeps the interface more consistent?
idk; just some food for thought.
Because Realtek wouldn't be the same company if everything made sense. The value for
"off"
is actually 5. 6 and 7 are just more blink intervals [3]. I guess they decided
to add more
rates, but didn't want to break backwards compatibilty somewhere.
[3] https://svanheule.net/realtek/maple/register/led_sw_p_ctrl
+};
+
+struct led_port_group {
+ struct regmap_field *setting;
+ unsigned int size;
+ /* bitmap to keep track of associated ports */
Can you explain a bit what's supposed to be here? right now its just an
it pointer, but appearantly it will point at some bitmap? can we not
make a struct representing this bitmap and then use that instead?
This is how bitmaps are defined in the kernel. cpumask structs do wrap their
bitmap in a
struct, but I see little point in adding an extra layer here.
+ unsigned long *ports;
+};
+
+struct switch_port_led_config;
can we do this forward declaration the other way around? You forward
declare it here because need the TYPE for the cfg pointer.
Later however, you
Re: [RFC PATCH 2/7] realtek: add switch core MFD driver
Hey Sander,
On 29-07-2022 22:19, Sander Vanheule wrote:
On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
Hey Sander,
On 16-07-2022 21:09, Sander Vanheule wrote:
Realtek managed switch SoCs such as the RTL8380 consist of a MIPS CPU
with a number of basic peripherals, and an ethernet switch peripheral.
Besides performing ethernet related tasks, this switch core also
provides SoC management features. These switch core features are badly
separated, and cannot be divided into distinct IO ranges.
This MFD core driver is intended to manage the switch core regmap, and
to expose some limited features that don't warrant their own driver.
These include SoC identification and system LED management.
Signed-off-by: Sander Vanheule
---
.../drivers/mfd/realtek-switchcore.c | 244 ++
...0-mfd-add-Realtek-switch-core-driver.patch | 50
target/linux/realtek/rtl838x/config-5.10 | 2 +
3 files changed, 296 insertions(+)
create mode 100644
target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
create mode 100644
target/linux/realtek/patches-5.10/200-mfd-add-Realtek-switch-
core-driver.patch
diff --git a/target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
b/target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
new file mode 100644
index ..5b3f6eee6fe2
--- /dev/null
+++ b/target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
@@ -0,0 +1,244 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include
Probably want bitops.h here as well, as BIT() and GENMASK() come from there
Will add.
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct realtek_switchcore_ctrl;
I take it a switchcore is the generic name, and an rtl8380 etc just
implements one? Makes sense if so.
Switchcore is the name of the ASIC peripheral that handles the networking, and
is really
the system controller as well, managing pin muxes and such. RTL838x (Maple) SoC
have one
register layout, RTL839x (Cypress) has another set. In one generation (RTL83xx,
RTL93xx)
the provided features are usually similar, but every SoC type really requires
its own
implementation.
+
+struct realtek_switchcore_data {
+ struct reg_field sys_led_field;
+ const struct mfd_cell *mfd_devices;
+ unsigned int mfd_device_count;
+ void (*probe_model_name)(const struct realtek_switchcore_ctrl *ctrl);
+};
+
+struct realtek_switchcore_ctrl {
+ struct device *dev;
+ struct regmap *map;
+ const struct realtek_switchcore_data *data;
+ struct led_classdev sys_led;
+ struct regmap_field *sys_led_field;
+ bool active_low;
+};
+
+/*
+ * Model name probe
+ *
+ * Reads the family-specific MODEL_NAME_INFO register
+ * to identify the SoC model and revision
+ */
any reasons to put the defines close to the function rather then before
the code as is commonly done? I'm not saying it's a bad idea; it just
not what is done normally ...
I put the RTL838x defines here, because I was planning to put the information
for one SoC
type together. The register definitions could go into a separate MFD header
though, but I
kept them here for now to more easily maintain an overview.
+#define RTL8380_REG_MODEL_NAME_INFO0x00d4
+#define RTL8380_REG_CHIP_INFO 0x00d8
+#define MODEL_NAME_CHAR_XLATE(val) ((val) ? 'A' + (val) - 1 : '\0')
+#define MODEL_NAME_CHAR(reg, msb,
lsb) (MODEL_NAME_CHAR_XLATE(FIELD_GET(GENMASK((msb), (lsb)), (val
+
These macro's are certainly not SoC-specific, and (following conventions)
should indeed be
higher up in the file.
+#define RTL8380_REG_INT_RW_CTRL0x0058
_personally_ i prefer alignment with spaces here so that any changes in
tab with (for indentation) is not affecting the alignment here.
+
+static void rtl8380_probe_model_name(const struct realtek_switchcore_ctrl
*ctrl)
+{
+ char model_char[4] = {0, 0, 0, 0};
While I love being explicit, what if you have [20] :)
Then I shouldn't be putting this on the stack either, when this becomes too
big. But for
all current SoC series, 3 is the maximum number of characters that can be
defined in the
model name (plus NULL termination)
I was being a smart-ass; but you answerd yourself below, that {} is
probably better as your initializer.
+ char model_char[4] = { 0x00 };
is probably easier ;)
memset() would also work, but I should be able to use {} if we just want to
empty-
initialise a const-size array.
sure, but the { 0x00 }, initializer auto-repeats itself for all entries.
+ char chip_rev;
+ u32 model_id;
+ u32 val = 0;
+
+ regmap_read(ctrl->map, RTL8380_REG_MODEL_NAME_INFO, );
+ model_id = FIELD_GET(GENMASK(31, 16), val);
+ model_char[0] = MODEL_NAME_CHAR(val, 15, 11);
+ model_char[1] = MODEL_NAME_CHAR(val, 10, 6);
+ model_char[2] = MODEL_NAME_CHAR(val, 5
Re: [RFC PATCH 7/7] realtek: Netgear GS110TPPv1: define port LEDs
On 29-07-2022 15:31, Sander Vanheule wrote:
On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
On 16-07-2022 21:09, Sander Vanheule wrote:
Add the port LEDs for lan1-lan8 to the device tree for the GS110TPP v1.
To reproduce the same behaviour as stock firmware, green should be
LINK/ACT 1G, and amber should be LINK/ACT 100M/10M:
for i in $(seq 1 8); do
echo 13 > /sys/class/leds/green:lan-$i/rtl_hw_trigger
echo realtek-switchport > /sys/class/leds/green:lan-$i/trigger
echo f > /sys/class/leds/amber:lan-$i/rtl_hw_trigger
echo realtek-switchport > /sys/class/leds/amber:lan-$i/trigger
done
Signed-off-by: Sander Vanheule
---
.../dts-5.10/rtl8380_netgear_gs110tpp-v1.dts | 30 +++
1 file changed, 30 insertions(+)
diff --git a/target/linux/realtek/dts-5.10/rtl8380_netgear_gs110tpp-v1.dts
b/target/linux/realtek/dts-5.10/rtl8380_netgear_gs110tpp-v1.dts
index 1ff209cee363..897699bea2c3 100644
--- a/target/linux/realtek/dts-5.10/rtl8380_netgear_gs110tpp-v1.dts
+++ b/target/linux/realtek/dts-5.10/rtl8380_netgear_gs110tpp-v1.dts
@@ -43,3 +43,33 @@
{
status = "okay";
};
+
+#define LAN_LED_LABEL(p, n)STRINGIZE(p ## n)
+#define LED_LABEL_GREEN(p) LAN_LED_LABEL(green:lan-, p)
+#define LED_LABEL_AMBER(p) LAN_LED_LABEL(amber:lan-, p)
+#define NETGEAR_LED(_phy, _port) \
+ led@ ## _phy ##.0 { \
+ reg = < _phy 0 >; \
+ label = LED_LABEL_GREEN(_port) ;\
+ }; \
+ led@ ## _phy ## .1 {\
+ reg = < _phy 1 >; \
+ label = LED_LABEL_AMBER(_port) ;\
+ }
+
+ {
+ port-leds {
+ compatible = "realtek,rtl8380-port-led";
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ NETGEAR_LED(8,1);
+ NETGEAR_LED(9,2);
+ NETGEAR_LED(10,3);
+ NETGEAR_LED(11,4);
+ NETGEAR_LED(12,5);
+ NETGEAR_LED(13,6);
+ NETGEAR_LED(14,7);
+ NETGEAR_LED(15,8);
+ };
+};
ugh, is it relaly worth it to do this? can we not just write it out; yes
it's a lot of writing (you can use the define to generate it and copy
paste it ;p)
I agree it's a bit opaque, and it doesn't allow you to add a label to the
second port LED. But I
was writing this by hand and didn't want to do endless copy-pasting when
modifying the LED list.
The list also misses two LEDs, for device ports 10 and 11, so now I'm doubting
if I actually tested
this...
Sadly, I can't test this until I had 93xx support to your patch series;
which I'll try to do as time allows, as I really want this for my switch :)
Best,
Sander
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFC PATCH 6/7] realtek: Zyxel GS1900-8: define port LEDs
On 29-07-2022 15:27, Sander Vanheule wrote:
Hi Oliver,
Thanks for all the feedback, I'll go over it tonight. In the future, could you
CC me (as the patch
author), other any reviewers, and people who were CC-ed before? Not everybody
may be subscribed to
the mailing list (not even necessarily the patch author).
absolutly; I'm not subsribed either; so I downloaded the mbox file from
patchwork; and did a 'reply-list'; guess thunderbird simply doesn't know
about other reviewers based on the mbox file; which is obvious of course ...
On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
On 16-07-2022 21:09, Sander Vanheule wrote:
Add all port LEDs to the device tree for the GS1900-8. To reproduce the
same behaviour as stock firmware, the LEDs need to light up on all
link speeds, and blink on link activity:
echo 1f > /sys/class/leds/lan?/rtl_hw_trigger
echo realtek-switchport > /sys/class/leds/lan?/trigger
Signed-off-by: Sander Vanheule
---
.../dts-5.10/rtl8380_zyxel_gs1900-8.dts | 41 +++
1 file changed, 41 insertions(+)
diff --git a/target/linux/realtek/dts-5.10/rtl8380_zyxel_gs1900-8.dts
b/target/linux/realtek/dts-5.10/rtl8380_zyxel_gs1900-8.dts
index e9c5efe60392..41266b701aca 100644
--- a/target/linux/realtek/dts-5.10/rtl8380_zyxel_gs1900-8.dts
+++ b/target/linux/realtek/dts-5.10/rtl8380_zyxel_gs1900-8.dts
@@ -10,3 +10,44 @@
{
/delete-node/ poe_enable;
};
+
+ {
+ port-leds {
+ compatible = "realtek,rtl8380-port-led";
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ led@8.0 {
+ reg = <8 0>;
+ label = "lan1";
+ };
+ led@9.0 {
+ reg = <9 0>;
+ label = "lan2";
+ };
+ led@10.0 {
+ reg = <10 0>;
+ label = "lan3";
+ };
+ led@11.0 {
+ reg = <11 0>;
+ label = "lan4";
+ };
+ led@12.0 {
+ reg = <12 0>;
+ label = "lan5";
+ };
+ led@13.0 {
+ reg = <13 0>;
+ label = "lan6";
+ };
+ led@14.0 {
+ reg = <14 0>;
+ label = "lan7";
+ };
+ led@15.0 {
+ reg = <15 0>;
+ label = "lan8";
+ };
+ };
+};
nothing else to say then 'love it' :) I'm not familiar with the GS1900;
but it only has a single LED with one color?
That's correct. Each port on this device has one green LED to indicate status.
I was lazy and didn't
add the 'color', 'function', 'function-enumerator' properties...
heh, part of our job description right :) I'd still add them to make it
more clear, also doesn't that help when they show up in sysfs?
Can we set the trigger
already here in the dt like we do with others?
arch/arm/boot/dts/am335x-bone-common.dtsi:31: linux,default-trigger =
"heartbeat";
for example ...
You actually can. The trigger that reproduces stock behaviour is
"realtek-switchport". But! Before
you enable that trigger, you need to write an appropriate trigger mask to the
custom config property
in /sys/class/leds/$PORT_LED. Otherwise the trigger value will be "disabled".
Setting that value
from DT is not possible though. So I figured that the recommended way to
configure this HW trigger
would be to run a script at system init.
the net-effect would still be the same right? no led would show up until
the startup script launches, so having the `realtek-switchport` trigger
makes it more descriptive? I think it's an oversight though of the dts
parser, as if you define the trigger 'blink' you should be able to set
up a delay_on/off from the dts as well. but that's a separate discussion :)
Best,
Sander
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFC PATCH 7/7] realtek: Netgear GS110TPPv1: define port LEDs
On 16-07-2022 21:09, Sander Vanheule wrote:
Add the port LEDs for lan1-lan8 to the device tree for the GS110TPP v1.
To reproduce the same behaviour as stock firmware, green should be
LINK/ACT 1G, and amber should be LINK/ACT 100M/10M:
for i in $(seq 1 8); do
echo 13 > /sys/class/leds/green:lan-$i/rtl_hw_trigger
echo realtek-switchport > /sys/class/leds/green:lan-$i/trigger
echo f > /sys/class/leds/amber:lan-$i/rtl_hw_trigger
echo realtek-switchport > /sys/class/leds/amber:lan-$i/trigger
done
Signed-off-by: Sander Vanheule
---
.../dts-5.10/rtl8380_netgear_gs110tpp-v1.dts | 30 +++
1 file changed, 30 insertions(+)
diff --git a/target/linux/realtek/dts-5.10/rtl8380_netgear_gs110tpp-v1.dts
b/target/linux/realtek/dts-5.10/rtl8380_netgear_gs110tpp-v1.dts
index 1ff209cee363..897699bea2c3 100644
--- a/target/linux/realtek/dts-5.10/rtl8380_netgear_gs110tpp-v1.dts
+++ b/target/linux/realtek/dts-5.10/rtl8380_netgear_gs110tpp-v1.dts
@@ -43,3 +43,33 @@
{
status = "okay";
};
+
+#define LAN_LED_LABEL(p, n)STRINGIZE(p ## n)
+#define LED_LABEL_GREEN(p) LAN_LED_LABEL(green:lan-, p)
+#define LED_LABEL_AMBER(p) LAN_LED_LABEL(amber:lan-, p)
+#define NETGEAR_LED(_phy, _port) \
+ led@ ## _phy ##.0 { \
+ reg = < _phy 0 >; \
+ label = LED_LABEL_GREEN(_port) ;\
+ }; \
+ led@ ## _phy ## .1 {\
+ reg = < _phy 1 >; \
+ label = LED_LABEL_AMBER(_port) ;\
+ }
+
+ {
+ port-leds {
+ compatible = "realtek,rtl8380-port-led";
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ NETGEAR_LED(8,1);
+ NETGEAR_LED(9,2);
+ NETGEAR_LED(10,3);
+ NETGEAR_LED(11,4);
+ NETGEAR_LED(12,5);
+ NETGEAR_LED(13,6);
+ NETGEAR_LED(14,7);
+ NETGEAR_LED(15,8);
+ };
+};
ugh, is it relaly worth it to do this? can we not just write it out; yes
it's a lot of writing (you can use the define to generate it and copy
paste it ;p)
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFC PATCH 6/7] realtek: Zyxel GS1900-8: define port LEDs
On 16-07-2022 21:09, Sander Vanheule wrote:
Add all port LEDs to the device tree for the GS1900-8. To reproduce the
same behaviour as stock firmware, the LEDs need to light up on all
link speeds, and blink on link activity:
echo 1f > /sys/class/leds/lan?/rtl_hw_trigger
echo realtek-switchport > /sys/class/leds/lan?/trigger
Signed-off-by: Sander Vanheule
---
.../dts-5.10/rtl8380_zyxel_gs1900-8.dts | 41 +++
1 file changed, 41 insertions(+)
diff --git a/target/linux/realtek/dts-5.10/rtl8380_zyxel_gs1900-8.dts
b/target/linux/realtek/dts-5.10/rtl8380_zyxel_gs1900-8.dts
index e9c5efe60392..41266b701aca 100644
--- a/target/linux/realtek/dts-5.10/rtl8380_zyxel_gs1900-8.dts
+++ b/target/linux/realtek/dts-5.10/rtl8380_zyxel_gs1900-8.dts
@@ -10,3 +10,44 @@
{
/delete-node/ poe_enable;
};
+
+ {
+ port-leds {
+ compatible = "realtek,rtl8380-port-led";
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ led@8.0 {
+ reg = <8 0>;
+ label = "lan1";
+ };
+ led@9.0 {
+ reg = <9 0>;
+ label = "lan2";
+ };
+ led@10.0 {
+ reg = <10 0>;
+ label = "lan3";
+ };
+ led@11.0 {
+ reg = <11 0>;
+ label = "lan4";
+ };
+ led@12.0 {
+ reg = <12 0>;
+ label = "lan5";
+ };
+ led@13.0 {
+ reg = <13 0>;
+ label = "lan6";
+ };
+ led@14.0 {
+ reg = <14 0>;
+ label = "lan7";
+ };
+ led@15.0 {
+ reg = <15 0>;
+ label = "lan8";
+ };
+ };
+};
nothing else to say then 'love it' :) I'm not familiar with the GS1900;
but it only has a single LED with one color? Can we set the trigger
already here in the dt like we do with others?
arch/arm/boot/dts/am335x-bone-common.dtsi:31: linux,default-trigger =
"heartbeat";
for example ...
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFC PATCH 2/7] realtek: add switch core MFD driver
Hey Sander,
On 16-07-2022 21:09, Sander Vanheule wrote:
Realtek managed switch SoCs such as the RTL8380 consist of a MIPS CPU
with a number of basic peripherals, and an ethernet switch peripheral.
Besides performing ethernet related tasks, this switch core also
provides SoC management features. These switch core features are badly
separated, and cannot be divided into distinct IO ranges.
This MFD core driver is intended to manage the switch core regmap, and
to expose some limited features that don't warrant their own driver.
These include SoC identification and system LED management.
Signed-off-by: Sander Vanheule
---
.../drivers/mfd/realtek-switchcore.c | 244 ++
...0-mfd-add-Realtek-switch-core-driver.patch | 50
target/linux/realtek/rtl838x/config-5.10 | 2 +
3 files changed, 296 insertions(+)
create mode 100644
target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
create mode 100644
target/linux/realtek/patches-5.10/200-mfd-add-Realtek-switch-core-driver.patch
diff --git a/target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
b/target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
new file mode 100644
index ..5b3f6eee6fe2
--- /dev/null
+++ b/target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
@@ -0,0 +1,244 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include
Probably want bitops.h here as well, as BIT() and GENMASK() come from there
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct realtek_switchcore_ctrl;
I take it a switchcore is the generic name, and an rtl8380 etc just
implements one? Makes sense if so.
+
+struct realtek_switchcore_data {
+ struct reg_field sys_led_field;
+ const struct mfd_cell *mfd_devices;
+ unsigned int mfd_device_count;
+ void (*probe_model_name)(const struct realtek_switchcore_ctrl *ctrl);
+};
+
+struct realtek_switchcore_ctrl {
+ struct device *dev;
+ struct regmap *map;
+ const struct realtek_switchcore_data *data;
+ struct led_classdev sys_led;
+ struct regmap_field *sys_led_field;
+ bool active_low;
+};
+
+/*
+ * Model name probe
+ *
+ * Reads the family-specific MODEL_NAME_INFO register
+ * to identify the SoC model and revision
+ */
any reasons to put the defines close to the function rather then before
the code as is commonly done? I'm not saying it's a bad idea; it just
not what is done normally ...
+#define RTL8380_REG_MODEL_NAME_INFO0x00d4
+#define RTL8380_REG_CHIP_INFO 0x00d8
+#define MODEL_NAME_CHAR_XLATE(val) ((val) ? 'A' + (val) - 1 : '\0')
+#define MODEL_NAME_CHAR(reg, msb, lsb)
(MODEL_NAME_CHAR_XLATE(FIELD_GET(GENMASK((msb), (lsb)), (val
+
+#define RTL8380_REG_INT_RW_CTRL0x0058
_personally_ i prefer alignment with spaces here so that any changes in
tab with (for indentation) is not affecting the alignment here.
+
+static void rtl8380_probe_model_name(const struct realtek_switchcore_ctrl
*ctrl)
+{
+char model_char[4] = {0, 0, 0, 0};
While I love being explicit, what if you have [20] :)
+char model_char[4] = { 0x00 };
is probably easier ;)
+char chip_rev;
+u32 model_id;
+u32 val = 0;
+
+regmap_read(ctrl->map, RTL8380_REG_MODEL_NAME_INFO, );
+model_id = FIELD_GET(GENMASK(31, 16), val);
+model_char[0] = MODEL_NAME_CHAR(val, 15, 11);
+model_char[1] = MODEL_NAME_CHAR(val, 10, 6);
+model_char[2] = MODEL_NAME_CHAR(val, 5, 1);
+
+/* CHIP_INFO register must be unlocked by writing 0xa to the top bits */
+regmap_write(ctrl->map, RTL8380_REG_CHIP_INFO, FIELD_PREP(GENMASK(31, 28),
0xa));
Personally, I prefer self-explanatory code; so having a function
'rtl_switchcore_unlock() makes it readable as code, and allows for re-use
+regmap_read(ctrl->map, RTL8380_REG_CHIP_INFO, );
+chip_rev = MODEL_NAME_CHAR(val, 20, 16) ?: '0';
I'm new to this syntax, so MODEL_NAME_CHAR produces a value, and if so
it gets used, otherwise '0', right? so it's the same as:
+chip_rev = MODEL_NAME_CHAR(val, 20, 16) ? MODEL_NAME_CHAR(val, 20, 16)
: '0';
This surely is from a newer version of the C standard is it not? I would expect
that you assign either '0' or 'nothing'? Learned something new today!
+
+dev_info(ctrl->dev, "found RTL%04x%s rev. %c\n", model_id, model_char,
chip_rev);
+}
+
+/*
+ * Realtek hardware system LED
+ *
+ * The switch SoC supports one hardware managed direct LED output
+ * to manage a system LED, with two supported blinking rates.
+ */
+enum {
+ SWITCHCORE_SYS_LED_OFF = 0,
+ SWITCHCORE_SYS_LED_BLINK_64MS,
+ SWITCHCORE_SYS_LED_BLINK_1024MS,
+ SWITCHCORE_SYS_LED_ON
+};
I recall someone else in your patch series/this patch, you have a struct
with off/on and an struct afaik with the various blink rates, is there
any relation? Does this need to
Re: [RFC PATCH 3/7] realtek: add pinctrl for Realtek maple
On 16-07-2022 21:09, Sander Vanheule wrote:
Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
RTL838x has been tested, but functionality should be near identical.
Since control bits for different muxes are scattered throughout the
switch core's register space, pin control features are provided by a
table of regmap fields. This should be flexible enough to allow
extension to other SoC generations providing similar features.
Signed-off-by: Sander Vanheule
---
.../drivers/leds/leds-rtl-switch-port.c | 668 ++
...inctrl-add-pinctrl-for-Realtek-maple.patch | 51 ++
target/linux/realtek/rtl838x/config-5.10 | 1 +
3 files changed, 720 insertions(+)
create mode 100644
target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
create mode 100644
target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-maple.patch
diff --git
a/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
new file mode 100644
index ..76dfede7ba15
--- /dev/null
+++ b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
@@ -0,0 +1,668 @@
+// SPDX-License-Identifier: GPL-2.0
+
Probably want bitops.h for BIT()
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/*
+ * Realtek switch port LED
+ *
+ * The switch ASIC can control up to three LEDs per phy, based on a number of
+ * matching conditions. Alternatively, each individual LED output can also be
+ * configured for manual control.
+ */
Is this an internal define? `REALTEK_PORT_LED_TRIGGER_NONE` (etc) maybe
better? (or LEDTRIG if it really must be shorter)
It wasn't until much later that I figured out that this was the
abbreviation for port trigger *doh*
+#define PTRG_NONE 0
+#define PTRG_ACT_RXBIT(0)
+#define PTRG_ACT_TXBIT(1)
+#define PTRG_ACT PTRG_ACT_RX | PTRG_ACT_TX
+#define PTRG_LINK_10 BIT(2)
+#define PTRG_LINK_100 BIT(3)
+#define PTRG_LINK_1000 BIT(4)
+
+struct led_port_blink_mode {
+ u16 interval; /* Toggle interval in ms */
+ u8 mode; /* ASIC mode bits */
+};
+
+#define REALTEK_PORT_LED_BLINK_COUNT 6
+struct led_port_modes {
realtek_led_port_modes?
+ u8 off;
+ u8 on;
+ struct led_port_blink_mode blink[REALTEK_PORT_LED_BLINK_COUNT];
ah here's the other list. So would it make sense to re-use this same
structure for the sys-led, or is that over-optimizing things ...
Why though are on/off and the blink entries separated? Isn't 'off' a
interval of 'inviite (max-int)' and on a blink rate of 0? While you may
have to do some logic later, it keeps the interface more consistent?
idk; just some food for thought.
+};
+
+struct led_port_group {
+ struct regmap_field *setting;
+ unsigned int size;
+ /* bitmap to keep track of associated ports */
Can you explain a bit what's supposed to be here? right now its just an
it pointer, but appearantly it will point at some bitmap? can we not
make a struct representing this bitmap and then use that instead?
+ unsigned long *ports;
+};
+
+struct switch_port_led_config;
can we do this forward declaration the other way around? You forward
declare it here because need the TYPE for the cfg pointer.
Later however, you need it to indicate the argument of a function
pointer. I think the type declaration ways heavier and makes it thus
'nicer' to do it the other way around. though that does indeed mean you
have to do 2 forard declarations for your two structs, but would still
be cleaner imo
+
+struct switch_port_led_ctrl {
+ struct device *dev;
+ struct regmap *map;
+ const struct switch_port_led_config *cfg;
+ struct mutex lock;
+ struct led_port_group *groups;
+};
+
+struct switch_port_led {
+ struct led_classdev led;
+ struct switch_port_led_ctrl *ctrl;
+ unsigned int port;
+ unsigned int index;
+ u32 trigger_flags;
+ struct led_port_group *current_group;
+};
+
+struct switch_port_led_config {
+ unsigned int port_count;
+ unsigned int port_led_count;
+ unsigned int group_count;
+ const struct led_port_modes *modes;
+ /* Set the number of LEDs per port */
+ int (*port_led_init)(struct switch_port_led_ctrl *ctrl,
+ unsigned int led_count);
what is the significance for both these comments (above and below)? the
comment states to set the leds per port; but the function is called
led_init? Below the function is called set enabled, so that's pretty
obvious that you set the port enabled switch port led?
+ /* Enable or disable all LEDs for a certain port */
+ int (*set_port_enabled)(struct switch_port_led *led, bool enabled);
Re: [RFC PATCH 4/7] realtek: add RTL8380 switch port LED driver
On 16-07-2022 21:09, Sander Vanheule wrote:
RTL8380 switch SoCs can control up to 3×28 LEDs to display status of the
switch's ethernet port. This driver allows to address these LEDs and
provides direct control, blink offloading, and switch port status
offloading.
Since offloading of the generic netdev trigger does not exist, this
driver provides a private trigger which achieves the same and is named
"realtek-switchport". Each LED will have a file name "rtl_hw_trigger" in
/sys/class/leds/$LED, where the requested trigger mode can be written
to. If an unsupported mode is requested, this will be reported to the
user. When the trigger is then activated, the LED will be added to a
group of LEDs with the same trigger conditions. If it is not possible to
add the LED to a group, the user must use an already existing trigger
condition, or use direct LED control with a software trigger.
Some common modes (i.e. values for "rtl_hw_trigger") are:
- Link present, blink on activity: 1f
- 100M/10M link, blink on activity: f
- 1G link present: 10
Signed-off-by: Sander Vanheule
---
.../drivers/pinctrl/pinctrl-rtl-switchcore.c | 370 ++
...s-add-RTL8380-switch-port-LED-driver.patch | 64 +++
target/linux/realtek/rtl838x/config-5.10 | 1 +
3 files changed, 435 insertions(+)
create mode 100644
target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
create mode 100644
target/linux/realtek/patches-5.10/202-leds-add-RTL8380-switch-port-LED-driver.patch
diff --git
a/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
b/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
new file mode 100644
index ..0caf925989b6
--- /dev/null
+++ b/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include
for ARRAY_SIZE()
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "core.h"
+#include "pinmux.h"
+
+/**
+ * struct rtl_swcore_mux_desc - switchcore pin group information
+ *
+ * Pins are frequently muxed between alternative functions, but the control
+ * bits for the muxes are scattered throught the switchcore's register space.
+ * Provide a regmap-based interface to flexibly manage these mux fields, which
+ * may vary in size and do not always provide a GPIO function.
+ *
+ * @name: name to identify the pin group
+ * @field_desc: description of the register field with mux control bits
+ * @functions: NULL terminated array of function names
+ * @pins: array of numbers of the pins in this group
+ * @npins: number of pins in this group
+ */
+struct rtl_swcore_mux_desc {
+ const char *name;
+ struct reg_field field;
+ const unsigned int *pins;
+ unsigned int npins;
+};
+
+#define SWITCHCORE_MUX(_name, _field, _pins) { \
+ .name = (_name),\
+ .field = _field,\
+ .pins = (_pins),\
+ .npins = ARRAY_SIZE(_pins), \
+ }
+
+/**
+ * struct rtl_swcore_mux_setting - stored mux setting
+ *
+ * @mux: pointer to the mux descriptor
+ * @value: value to write in the mux's register field to apply this setting
+ */
+struct rtl_swcore_mux_setting {
+ const struct rtl_swcore_mux_desc *mux;
+ unsigned int value;
mux_setting? value is so ... undefined ;)
+};
+
+/**
+ * struct rtl_swcore_function_desc - switchcore function information
+ *
+ * @name: name of this function
+ * @settings: list of mux settings that enable this function on said mux
+ * @nsettings: length of the @settings list
+ */
+struct rtl_swcore_function_desc {
+ const char *name;
+ const struct rtl_swcore_mux_setting *settings;
+ unsigned int nsettings;
+};
+
+#define SWITCHCORE_FUNCTION(_name, _settings) { \
+ .name = (_name),\
+ .settings = (_settings),\
+ .nsettings = ARRAY_SIZE(_settings), \
+ }
+
+struct rtl_swcore_config {
+ const struct pinctrl_pin_desc *pins;
+ unsigned int npins;
+ const struct rtl_swcore_function_desc *functions;
+ unsigned int nfunctions;
+ const struct rtl_swcore_mux_desc **groups;
+ unsigned int ngroups;
+};
+
+struct rtl_swcore_pinctrl {
+ struct pinctrl_desc pdesc;
+ struct device *dev;
+ const struct rtl_swcore_config *config;
+ struct regmap_field **mux_fields;
+};
+
+/*
+ * RTL838x chips come in LQFP packages with 216 pins. Pins are indexed
+ * counter-clockwise, starting with pin 1 at the bottom left.
+ * Map package pin {1..216} to pinctrl pin number {0..215}.
+ */
+static const struct pinctrl_pin_desc rtl8380_swcore_pins[] = {
+ /* JTAG pins
Re: [RFC PATCH 5/7] realtek: rtl838x: replace pinctrl-single
On 16-07-2022 21:09, Sander Vanheule wrote:
Replace the pinctrl-single node with the dedicated pinctrl driver for
RTL838x SoCs. The node names are kept to stay compatible with existing
references.
Signed-off-by: Sander Vanheule
---
target/linux/realtek/dts-5.10/rtl838x.dtsi | 38 ++
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/target/linux/realtek/dts-5.10/rtl838x.dtsi
b/target/linux/realtek/dts-5.10/rtl838x.dtsi
index 11cabc3f63cb..6aac2be95368 100644
--- a/target/linux/realtek/dts-5.10/rtl838x.dtsi
+++ b/target/linux/realtek/dts-5.10/rtl838x.dtsi
@@ -169,33 +169,29 @@
};
};
- pinmux: pinmux@1b001000 {
- compatible = "pinctrl-single";
- reg = <0x1b001000 0x4>;
+ switchcore: switchcore-bus@1b00 {
+ compatible = "realtek,rtl8380-switchcore", "syscon";
+ reg = <0x1b00 0x1>;
- pinctrl-single,bit-per-mux;
- pinctrl-single,register-width = <32>;
- pinctrl-single,function-mask = <0x1>;
- #pinctrl-cells = <2>;
+ hw_led_sys: led-sys {
+ status = "disabled";
- enable_uart1: pinmux_enable_uart1 {
- pinctrl-single,bits = <0x0 0x10 0x10>;
+ label = "green:status";
};
- };
- /* LED_GLB_CTRL */
- pinmux_led: pinmux@1b00a000 {
- compatible = "pinctrl-single";
- reg = <0x1b00a000 0x4>;
+ pinctrl {
+ compatible = "realtek,rtl8380-pinctrl";
- pinctrl-single,bit-per-mux;
- pinctrl-single,register-width = <32>;
- pinctrl-single,function-mask = <0x1>;
- #pinctrl-cells = <2>;
+ /* enable GPIO 0 */
+ pinmux_disable_sys_led: sys-led-mux {
+ groups = "sys-led";
+ function = "gpio";
+ };
with your changes, why not use the hw function?
- /* enable GPIO 0 */
- pinmux_disable_sys_led: disable_sys_led {
- pinctrl-single,bits = <0x0 0x0 0x8000>;
+ enable_uart1: uart1-mux {
_uart1 { status=disabled }; reads weird though :p
+ groups = "uart1";
+ function = "uart1";
+ };
};
};
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
