Rockpis wifi fixes

[brentr]

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Jira reference number AR-1268 AR-1269 AR-1270

The fixMACaddress udev rules are generic enough to be used on all boards where random MAC addresses are a problem. However, for now, the feature is only enabled as part of the RockPi S BSP. Other BSPs could also copy the directory package/bsp/fixMACaddress to the $destination if desired.

How Has This Been Tested?

Generated new images, verified that WiFi address is now fixed across reboots. Verified that applying SDIO clock speed DT overlay increases WiFi throughput to >2MB/s Ran WiFi for half day to confirm it was stable at this rate.

Checklist:

• [x ] My code follows the style guidelines of this project
• [x ] I have performed a self-review of my own code
• [x ] I have commented my code, particularly in hard-to-understand areas
• [? ] I have made corresponding changes to the documentation
• [x ] My changes generate no new warnings
• [? ] Any dependent changes have been merged and published in downstream modules

[paolosabatino]

Mmmh, I'm not into rockpis, but I still have some considerations...

• I'm not sure the overlays will work... I mean, it works when you create a fresh image because "accidentally" the device tree overlays will fit into the image - and it looks to me that they will be put as source dts, and not as dtbo, but device tree overlays are part of the kernel and thus should be provided as kernel patches, otherwise an upgrade will zap them from your existing installation (maybe you need this patch to properly compile overlays by kernel Makefile and this other patch can be used as guidance on how to bring in overlays
• the fixMACaddress directory in bsp... mmh, does not sound very well to me, you should put that file into bsp/rockpis/fix-mac-address.sh IMHO, without the overlay-like paths
• there are two .rules files, one of them look useless to me. The other one should be put into bsp/rockpis directory
• the script and rules files should be copied as you did in family_tweaks_bsp() function, but individually (and maybe install could be used to give proper permissions, especially important for the script file)
• Using the partition UUID for setting the MAC address is a way, but if you prefer something more stable, it would be better to use the serial number of the chip that can be accessed from efuse (/sys/bus/nvmem/devices/rockchip-efuse0/nvmem, on rk3328 the serial is available from byte 7 to 23, dunno on rk3308, an example for rk3318/28 is this)
• SDIO bus running at 1MHz or 10MHz looks very detrimental to me; I don't know if there are some hardware limitations (probably there are, due to small sizes), but you may also try to increase (or even decrease!) the pin strength and see what happens.
• rockchip or even better rk322x 32-bit families can be a useful guidance all around, since the 4.4 kernel has been developed a lot in the past for those families.

[brentr]

I'm just getting into rockpis myself, as a very power efficient little IOT gateway...

• The dts overlays cannot be part of the standard kernel because many are only applicable to the newer silicon. (on board v1.3) Ideally, the kernel would determine which overlays to install based on the silicon variant. Do you know how that might be accomplished? Also, overclocking is not something that should be wired into the standard kernel. I thought the armbian-add-overlay command was intended for users as simple interface for adding source dts files. Having the source (.dts) lets people experiment on the target device. I do not see how installing a new kernel would "zap" the dts overlays being installed by u-boot, unless that new kernel's device tree layout changed dramatically. Could you explain?
• The advantage of the overlay directory structure is that it let's one avoid having to modify install scripts whenever it changes. cp -a preserves the file attributes, particularly the -x flag. Later in the install process, all these files are made owned by root:root anyway, so the ownership of the files is not relevant.
• There is a .rule file in the generic package for possible use on other devices. It is a template with no rules enabled. When I searched the forums for this issue, users of many different devices complained about changing MAC addresses.
• As noted above, that would make the script harder to maintain when overlay files change. Permissions are copied with the files in the overlay The RK3308 does not have the same path layout. The closest I could find was /sys/bus/nvmem/devices/rockchip-otp0, but it has only 4 bytes that look like they might be a serial number of some sort. I realize it's not ideal, but the partition UUID is generic. It works the same way across all devices. I agree that, when we come up with a better, generic UUID to identify the machine, we certainly should employ that to generate fixed MAC addresses.

I'm going to add a README in the /boot/overlay directory to help users decide which .dts files are applicable to their boards and mention how to install them.

[paolosabatino]

1. I didn't mean the overlays should be merged into kernel main device tree, I intended that the overlays should be compiled as kernel device tree overlays (dtbo), and should be compiled and installed on par with the kernel. That's the reason why I pointed to existing patches. If you manually put them in /boot/dtb/overlay, as I see is happening right now, first it is wrong because the right path would be /boot/dtb/rockchip/overlay since arm64 device trees have the vendor prefix, but also they will be removed when linux-dtb deb package is installed on a further apt upgrade that involves the kernel. armbian-add-overlay is for user-added overlays AFAIK, but those that you are proposing are system/board overlays. Another good reason to do the right way is that armbian-config will allow the user to chose and enable the overlays via simpler menu options.
2. There are two reasons the overlay structure is not good in this case: the former is the fact that no other family do such thing, the latter is that if, for a reason or another, the permissions are not right on your building machine, they propagate to the final image. This is something that may seem remote, but if and when happen will hurt the maintainer a lot. Single files and install script is the de-facto standard way to do that for all other families, doing it with explicit permissions.
3. I have nothing against a fixed MAC address, just saying that the place where the script is put pollutes the bsp directory, and it has not reason to stay there since it is a workaround for rk3308. Proper place to set the MAC address is usually u-boot, but I understand it may require out of scope patches.
4. The partition UUID may be generic, but simply if you install the same image on two devices you may end up with the same MAC address on their interfaces. On the opposite two different images on the same machine will end up changing the MAC address of the very same machine, which is not advisable. The efuse/otp is a good source of data which changes for every single chip, so it is the best candidate IMHO for such job.

Believe me that these are suggestions coming from past experience, and are not criticism against the patches.

[brentr]

1. I'll investigate using kernel overlays for these patches.
2. The install command has the option of installing whole directories. I'll look into replacing cp -a with install -d
3. As noted previously, the random MAC address issue impacts many Armbian supported boards, not just the rk3308. Fixing this in u-boot would be better, but also much more work. Have you really never seen this issue before in your own work?
4. I acknowledged that the using partition UUID is not ideal. But, I can't think of any better alternative that would not require custom coding for each board. When we discover one, we should update fixMACaddr to use it. Until then, I propose we use the partition UUID as a fallback, allowing each BSP to overwrite this default /lib/udev/fixEtherAddr script with a board specific alternative as part of its BSP tweaks install. The UUID works well for the common use case.

[brentr]

I've hacked up a version of fixEtherAddr that uses five bytes that appear to be a CPU serial number in /sys/bus/nvmem/devices/rockchip-otp0 I suppose every other effected board type can implement their own variant of fixEtherAddr using this one as a template.

Regarding the slow SDIO clocking: I verified that the same boards get full >50Mb/s WiFi throughput running the legacy kernel. I suspected it to be a drive strength issue too, but I see no differences in the pinctrl on those pins. Admittedly, I'm pretty new at this stuff. Most of my previous experience is pre device-tree. Any thoughts on where to look next?

[brentr]

1. As I read your comments more carefully, it seems to me that you may have misunderstood how this patch is handling the device tree modifications: a. The five .dts files relevant to some RockPi-S board revs are preloaded in /boot/overlay b. A README file there instructs the user which overlays are applicable to their board and shows how to install them with the new armbian-add-overlay command The .dtbo's ultimately end up in /boot/overlay-user There is thus no danger that a kernel update will overwrite them.

2. I have replaced "cp -a ..." with a series of "install" commands. However... /boot/dtb contains hundreds of dtb files, 99% of which are not applicable this board. All of them have their +x attribute set, even through none are executable, likely because they were carelessly installed with the standard "install" utility, which defaults to 755 for directories and files, unless explicitly overridden. As an experiment, I removed all but the single 55KByte rk3308-rock-pi-s.dtb file referenced in /proc/cmdline for a net savings of 11MB and the board still functioned flawlessly. [Why do we carry all those irrelevant .dtb files in every image?]

3 & 4. As you suggested, I replaced the generic, UUID based fixMACaddr script with one specific to the RK33308 based on the SOC unique serial number pointed out to me. This is definitely an improvement. Thank you.

The updated patch makes no modifications outside of the RockPi-S's board support package. It allows the board to boot from its EMMC chip and increases WiFi throughput by 8 times. I would like to revisit the device tree overlay issue in October. I've no more time right now.

[paolosabatino]

1. I agree, I thought they were put into /boot/dtb/overlay, not /boot/overlay. Anyway those patches are still not meant to be put in /boot/overlay because those are still not user patches, but system/board patches. I again stress the fact they should be compiled along the kernel, so they will be packaged in the proper linux-dtb deb package, installed into /boot/dtb/rockchip/overlay as .dtbo files, automatically upgraded when new/updated kernels are available and the user can use armbian-config to apply them.
2. I may guess that all the device trees for arm64 targets are compiled by kernel Makefile, thus all of them are packaged into linux-dtb deb package. I don't know the details of the dtb packaging scripts, you may wish to investigate and propose a separate PR to addess the executable flag issue, nonetheless the plethora of dtbs I guess would require some kernel Makefile patching (should not be complicated though).

I could revise the device tree thing for you if you wish - should not take long -; I just have no rockpi devices to test

[paolosabatino]

Here it is a quick patch file to apply to the root of armbian that will remove the overlay files from filesystem and add a patch file that pushes them along other rk3308 overlay patches.

Applying with git apply should be enough

0001-move-rockpis-overlay-into-kernel-patches-remove-from.patch.gz

[brentr]

Thanks for this. I'm on vacation until late August. I will revisit this when I return.

[igorpecovnik]

Hi @brentr Got any time looking into this? It would be worth finishing it.

[brentr]

Caught Covid on vacation. Pretty much recovered now. I'm diagnosing why the WiFi chip's SDIO clock can't run faster than 10Mhz on the new kernel, but can run 50Mhz on the legacy one. Once I fix that, I'll produce a patch that should also address paolosabatino's concerns. Is it reasonable to expect that, after these issues are resolved, the Rock-PI-S can be officially supported again?

[igorpecovnik]

Thank you for your response and that you are well. Covid won't leave us anytime soon, sadly :(

Is it reasonable to expect that, after these issues are resolved, the Rock-PI-S can be officially supported again?

Yes. Well, by the book we need to have someone that will be act as a maintainer and be noted here. This doesn't necessarily means it has to be you or someone that has knowledge fixing problems. Keeping volunteers interested in this job is difficult and resourceful, regulars are overloaded, HW keep coming in :) Understanding support status on kernel upgrades (working, broken, this and that doesn't work) is already a good enough and valuable information that comes from that direction since workarounds usually exists in installing older versions and not update.

[brentr]

This pull request now includes a fix that enables the RK3308 (RockPI-S) to drive the SDIO clock at high speed. There was a register initialized to select 1.8V signaling in the legacy kernel that did not make it into the mainline. Further, the mechanism used to initialize this register (as a side effect of loading a special "io-domains" driver) was not happening by the time the SDIO bus was being probed by the MMC driver. I suspect it worked in the 4.4 kernel, but breaks in 5.x due to increased parallelism.
My fix was to move the critical register init into the pinctrl driver, which loads very early. I suspect that this issue will bite other Rockchip SOCs, but, for now, I'm only addressing the RK3308.

Since this has become a kernel patch, I've moved all the files that I had put into /boot/overlay into the directory where the kernel overlays normally go. This, hopefully, will address [paolosabatino]'s (https://github.com/paolosabatino) concerns.

I have indicated that I'd be willing to act as maintainer for the RockPI-S as the board fits into the IOT work I'm doing at my day job :-) I didn't realize I had to had myself to the list, so... just added myself to https://docs.armbian.com/Release_Board-Maintainers via a pull request.

[paolosabatino]

So for what I understand the io-domains driver was completely missing the rk3308 soc?

As far as I know, other main rockchip socs already have their controlling bits in the io-domains driver: it is important to inform the GRF or whatever that a voltage is used in place of another for a specific power plane, but it is just what you already discovered.

I will take a detailed look to the pull request soon, but for what I see right now I would suggest you to split the rockpis-sdio-speedup.patch into more proper patches:

• one patch to introduce the rk3308 into the io-domains driver
• one (or multiple) patch for overlays in the format overlays-xx-description.patch as they are already proposed in the rockchip64 branch. I agree that a patch chain like that is absolutely not the best way to handle that, but they historically are in such form and mixing things would introduce another maintenance burden.

[brentr]

I updated the PR, factoring the big sdio-speedup patch into a bunch of components which have all been named similarly to existing patches. I had been wondering generally about how armbian patches would continue to be applied in the proper order. Sequence numbers in the names are a good idea.

Regarding the RK3308 io-domains driver... rk3308-0002-iodomains.patch: Adds rk3308 support rk3308-0003-pinctrl-io-voltage-domains.patch: removes it again

The io-domains driver does not get probed in time for it to initialize before other drivers try to access other I/Os in their own initialization. In particular, the SDIO bus for the WiFi chip is accessed before its pins' voltages have been configured.

I could not find any straightforward way to let the kernel know that the io-domains must initialize before others. There is a producer/consumer relationship between pinctrl and others, but none between io-domains and other I/O pin users.

In the case of the RK3308, the only thing the io-domains driver does is to set a bit in a GRF register when it loads (its probe entry point is called). Since it can be argued that this should be part of pinctrl, as it optimizes the output drivers for the pins' voltages, I moved this register initialization from io-domains to pinctrl. Currently, this is a special case for the RK3308 only, but I suspect that io-domains is initializing too late for other SOCs as well. The effects may not be obvious. For the RockPI-S, it showed up as a couple retried SDIO failures during device discovery. Between the 2nd and 3rd try, the io-domains driver did its magic, so the 3rd (final) try succeeded. You can't tell anything went wrong until you look at the kernel log.

If you know a straightforward way in which one could ensure io-domains initializes before they are used, I'd be happy to rework this. As it is, one need not build the io-domains driver in a custom kernel for the RK3308.

[igorpecovnik]

how armbian patches would continue to be applied in the proper order.

With using series.conf file you can define order and path. Example: https://github.com/armbian/build/tree/master/patch/kernel/archive/sunxi-5.19

[paolosabatino]

The io-domains driver does not get probed in time for it to initialize before other drivers try to access other I/Os in their own initialization. In particular, the SDIO bus for the WiFi chip is accessed before its pins' voltages have been configured.

A trick that incredibly works for voltage regulators is to put the non-dependent regulators before the dependent regulators in the device tree. (a dependent regulator is a regulator that has a reference to another regulator in vin-supply property, to be clear). If the order is "inverted" the dependent regulator triggers an error and will get reprobed later, but some devices that depend on that regulator don't like this (like HDMI) and often do not work. This seems in contradiction with the kernel rule that says that nodes in the device tree must be in alphabetical order, but I guess the bottom driver should handle the delayed initialization, which is not always true. Anyway all of this to say that you may try to bring the io-domains node higher in the device tree and see if it gets initialized first.

In the case of the RK3308, the only thing the io-domains driver does is to set a bit in a GRF register when it loads (its probe entry point is called). Since it can be argued that this should be part of pinctrl, as it optimizes the output drivers for the pins' voltages, I moved this register initialization from io-domains to pinctrl. Currently, this is a special case for the RK3308 only, but I suspect that io-domains is initializing too late for other SOCs as well. The effects may not be obvious. For the RockPI-S, it showed up as a couple retried SDIO failures during device discovery. Between the 2nd and 3rd try, the io-domains driver did its magic, so the 3rd (final) try succeeded. You can't tell anything went wrong until you look at the kernel log.

As long as you mention it, I remember a case when I was working on a tvbox but later I solved it tuning the strength of the sdio pins. This could also be related to the Orange PI 4 LTS that I have here refusing to work with sdcard with kernel 5.19, but working so far with kernel 5.15.

I updated the PR, factoring the big sdio-speedup patch into a bunch of components which have all been named similarly to existing patches. I had been wondering generally about how armbian patches would continue to be applied in the proper order. Sequence numbers in the names are a good idea.

Thanks for this. When some commitees will be established hopefully there will be some clear rules on how to handle patch naming and ordering. In the meantime there is the patch series as mentioned by @igorpecovnik and implemented in this pull request, but still there are some concerns and things are still pending.

[brentr]

I think the problem is that the io-domains driver, in the case of the RK3308, is not being referenced by any other in the devicetree. [Actually, I didn't see any reference to io-domains in other drivers] Therefore, from the kernel's prospective, there's no reason to start io-domains early.

I don't think this is a problem with the regulator drivers. I've tried adding regulator properties to the RK3308 SDIO MMC device to see if that would force io-domains to probe earlier, but it had no effect. [I suspect because none of the regulator devices are dependent on io-domains.]

On your Orange PI 4 LTS running 5.19, you might try recording when io-domains probes with a kprintf or other means. I'd be curious to find out what you learn.

To guarantee the io-domains driver always probes before the pins it configures are used, we would need to make it a dependency of each of the drivers that use those pins, which is impractical. Or, we could force pinctrl's probe function to probe io-domains (if it is enabled). I don't know how to code this. Do you?

This lead me to the solution I used in the patch, as io-domains had become a NOP for the RK3308 in the 5.18 and 5.19 kernels and, what it did in the 4.4 kernel was trivial to move into pinctrl.

I'm willing to try other solutions that don't require touching many other drivers.

If you still really think that the merely reordering the (regulator) definitions in the device tree could fix this, please suggest an ordering you believe will work. I'll be happy to give it a quick try.

Or, if you can replicate the problem with your own Orange PI 4 LTS running 5.19, you might experiment with reordering the devicetree definitions there.

[paolosabatino]

I don't think this is a problem with the regulator drivers. I've tried adding regulator properties to the RK3308 SDIO MMC device to see if that would force io-domains to probe earlier, but it had no effect. [I suspect because none of the regulator devices are dependent on io-domains.]

I was citing the voltage regulator as an example: it looks like the kernel probes the device as soon as it encounters it in the device tree; I was just suggesting to move the io-domains node upwards, maybe right after the voltage regulators it depends from, to see if gets probed earlier than sdio device.

On your Orange PI 4 LTS running 5.19, you might try recording when io-domains probes with a kprintf or other means. I'd be curious to find out what you learn.

Yes, I could try this and see when the io-domains gets probed on rk3399; and see if changing the position in the device tree changes something.

[brentr]

I did try that before giving up and moving the functionality from io-domains to the pinctrl driver. Since this is a tree structure, the order in which the nodes are visited may not correspond to the order of definitions in the .dts files. And, even if we could find a way to order the definitions that worked in this kernel, it might break later on if someone were to change the devicetree traversal order.

[brentr]

Tried again to come up with an ordering of devicetree nodes that would probe io-domains before accessing pins... It can be done, but it would require a major reorganization of Rockchip .dts and .dtsi files Currently, the pinctrl driver explicitly populates all its child nodes when it probes. This causes any drivers in the devicetree defined under pinctrl to be probed near the end of pinctrl's probe().

All the MMC drivers are defined in the .dtsi as children of pinctrl.
Thus, they get probed at the end of pinctrl's probe() in the order in which they appear in the dts source file.
I verified that if the io-domains driver is defined as the first child of pinctrl it will probe before the others, but... io-domains gets its grf base address from its parent node, which is assumed to be the grf node. So, I tried making that grf node a child of pinctrl defined before emmc/mmc and sdio. But... When I do that, the system does not boot. (no console messages after "Starting kernel")

Even it I could get this approach to work, it would involve modifications to .dtsi file and thus require tweaks to all the Rockchip .dts files.

We may have stumbled onto a significant structural bug in the Rockchip platform support that only bites us in the newer kernels. Perhaps we should see if upstream is aware of this issue? (How?)

This PR seems a reasonable interim workaround until someone comes up with a better one. It deals with the immediate problem for the RK3308 without effecting the other RK SOCs.

The mods are only to the Rockchip pinctrl driver and the RK3308 specific devicetree definitions and are easily reverted when we get a better fix.

[paolosabatino]

Well I would not move everything into pinctrl, expecially the grf node which is clearly a separate driver and subsystem, altough the pinctrl driver interacts with the GRF registers to control the GPIOs and it looks a bit strange that the grf driver (and thus io-domain) gets probed later than pinctrl.

As a note there is some documentation in the kernel about the io-domain binding/driver that clearly states that the bit has to be kept at a higher value than real voltage: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/rockchip-io-domain.yaml - maybe it rings some bells...

Anyway you did enough tries, I see that the pinctrl thing could be a viable enough workaround but I wonder if the same workaround could not be put directly in the grf driver (maybe it is a better place?), as long as you say that the bit is fixed for rk3308.

As a whole, at this time the PR is nice enough for me.

[igorpecovnik]

@brentr images were rebult, one confirmation is enough https://www.armbian.com/rockpi-s/ Also please leave some contacts for access to website - if some remark needs to be added, and other infra.

[brentr]

@igorpecovnik Thanks -- That was fast! The website description needs to be updated to reflect that the console has moved back to UART0 and that one can now flash the edge kernel to SDnand Also, I've only tested the legacy and the edge kernels (5.19.y) The 5.15.y kernel is untested.
Do I need to patch and test the 5.15.y kernel? I'd rather not.

I've filled in my contact info. Please let me know how to update the RockPIs webpage.

[igorpecovnik]

That was fast!

Not always that fast ;) Most of it is well automatised, while auto-test on hardware is a bit more difficult.

The website

You should be able to edit data.

Also, I've only tested the legacy and the edge kernels (5.19.y)

One solution is this https://github.com/armbian/build/tree/igorpecovnik-patch-1 as 5.19.y is anyway soon going to be next LTS. Perhaps not for all, but here we can stick to it.

Thank you.

[brentr]

That patch looks good to me. Shall I make a PR of it? Or, will you apply it? Still not finding how to edit the web page. Sorry. Could you send a URL or hint? Just discovered many Rockchip boards with Mainline kernel lack driver support for RNG. This was causing the RockPIs to hang for 15s early in userspace init. I'm readying a PR to forward port the RNG driver from legacy. It seems to work great. (boots 15 seconds faster:)

[brentr]

I installed the git repo for editing the docs: https://github.com/armbian/documentation But, I still can't find how to edit the content of: https://www.armbian.com/rockpi-s/ Sorry if I'm missing something obvious...

[igorpecovnik]

Shall I make a PR of it? Or, will you apply it?

I will open a PR, just need a formal review to merge it.

Still not finding how to edit the web page. Sorry. Could you send a URL or hint?

Should get credentials on email. Hmm ... sent them again. Check your email.

boots 15 seconds faster:)

Great!!!

[brentr]

I missed the 1st email, got the 2nd. All good now. Thanks.

[brentr]

I've updated https://www.armbian.com/rockpi-s/ I'll test the edge kernel images after your PR for that comes through.