SMI Address selection
{P3_LED_2, P3_LED_1} signals are used to define decoded
Serial Management Interface(SMI) addresses of
C_MDC/C_MDIO for command registers access
00 : Use 7 to 12 SMI addresses
01 : Use 15 to 20 SMI addresses
10 : Use 23 to 28 SMI addresses
11 : Use 31 and 0 to 4 SMI addresses (default)
I don't know the magnitude of the slow down three MDIO bus operations instead of one would cause. Milliseconds? Microseconds? Also please don't forget the other two bullet points.
Banana Pi BPI-R2 sets the bootstrap pin for indirect access. It's being modified by the DSA subdriver to direct access. What now?
Not just missing info, even wrong info. If you tell me you've verified that the datasheet you have been linking says the truth you've convinced me. Also "your" datasheet is dated 2014-2015, mine is dated 2014...
Depends on the other load on the host this is not insignificant. Not just because of the actual MDIO ops, but also because of context-switching, busy-polling-loops on the way, ...
Why not avoid it?
If the board was made in that way and (typically) there is at most one or two more other PHYs on the same board which are always on different addresses (0x8 and 0xA are common choices), and at most there are 2 MT753x chips with 5 PHYs each total -- why bother? What do you gain by liberating those 5 addresses? Are you planning to build a giant MT753x-based switch with 1Gbit/s backhaul?!
Why would Clause 45 access using mt7530_phy_read_c45 be accessing Clause 45 via Clause 22? Isn't actually the opposite true: The MDIO device of the switch (typically at 0x1f) can be reached only via Clause 22, so when using indirect access to the PHYs we are using Clause 45 via IAC over mt7530-32bit-reg/value-coding over Clause 22?
Simplification in terms of (few) lines of code removed, yes. But if that's the only argument and it comes at the cost of (at least) a 3x increase on PHY operations I don't see the point, especially as it would also be a diversion from what MediaTek tests and does QA for, and what most existing boards have hence been tested and designed for.
Ouch. I start getting the impression that adding a dt-property to keep (or even set) MHWTRAP_PHY_ACCESS bit and use indirect access would be the most reasonable thing. That would get us the best of both options, and also make sure that behavior for existing boards and their DT doesn't change at all.
I've confirmed it by pulling one of the documented pins high before reset deassertion, so yes. It'd be great if MediaTek could publish an open version for MT7530 and MT7531 like they did with their recent SoCs.
Sure, that makes sense. I just thought that since MT7531 is a newer model and it doesn't provide direct access, it must be that the load must not be significant enough to care.
Not particularly, no.
This switch intellectual property supports Clause 45 access. Currently with direct access, we access the switch and the switch PHYs via the MDIO bus the switch listens on. This is usually the SoC's MDIO bus. We use Clause 22 to access because not all SoCs support Clause 45. If we were to use indirect access, Clause 45 access to switch PHYs would always be supported as the dependency shifts from the SoC to the indirect access control register. So we can switch to Clause 45 access for the core operations I've mentioned which would reduce the MDIO bus interactions.
I actually have the patches ready for this, and they've resulted in a net reduction of 132 lines.
I only have Banana Pi BPI-R2 that has MT7530 and is not of MT7621 SoC, and indirect access is set on the board, so my current evidence points to the contrary. Unless you know other non-MT7621 boards where direct access is set.
This sounds like a nightmare to implement and will bring complexity to both device trees and the DSA subdriver as we would have to make the DSA subdriver use the indirect access method for specific MT7530 switches. And there isn't any benefit. We're not saving the system from more load, now it's purely about respecting the bootstrapping configuration. Sorry, I don't even want to think about this.
Edit: You also wouldn't know whether or not your board would have the relevant pin pulled high or low without either modifying the DSA driver to read it from the trap register to tell you or look at the device schematics (which mostly won't be public).
For MT7531 they did, but much older MT7621 and MT7530 IP is still guarded, though various versions of the datasheet can easily be found online, with watermarks removed and nobody seems to even remotely fear being persecuted for that.
MT7531 being newer is typically also used on newer boards with more powerful CPUs and may allow higher MDC (?). When using a low-powered SoC to perform traffic shaping in software, for example, you can see the difference of having to do PHY polling every second or not. Keeping all non-forwarding-related background loads as minimal as possible does make sense and is worth some added complexity in terms of LOC.
Afaik phylink will decide wether to use C45-over-C22 or native C45 depending on the MDIO bus controller's capabilities. Are you aware of any SoC used with MT753x which cannot do C45?
And just to demonstrate: A C45-over-C22 read means one C22 write to MII_MMD_CTRL and a consecutive read from MII_MMD_DATA. Using MT7530 IAC means accessing the switch 32-bit wide registers, that's one C22 write and two C22 read for each register access operation. Now if you look at mt7531_ind_c45_phy_read you understand that this is something we will generally want to avoid unless really needed, because it means (at least, because of 3x readx_poll_timeout) two read and two write operations on those 32-bit registers, ie. 12 MDIO bus operations for a single C45 read from the PHY when using indirect access (best case).
So esp. on MT7621: Why burden this little MIPS core with lots of readx_poll_timeout unless we really have to?
On newer SoCs it doesn't hurt so much, and apart from exotic MT7623 BPi-R2 most boards with discrete MT753x ICs actually support interrupts. Recently in the low-pin-count mania of MT7981 there are again more boards missing the MT7531_INT because vendors decide to use the pin for other purposes... But that's a dual-core Cortex-A53 and vendors usually crank up the MDC...
Maybe you misunderstood my suggestion. I suggest to keep forcing direct PHY access on MT7530 and MT7621 (which is what we have always been doing from day one of the mt7530.c driver) and only use indirect access on that IP if a property mediate,force-indirect-phy-access is set in the board DT). Boards which require indirect access because of address overlap may set this property. Everybody else doesn't get burdened by more than another conditional jump instruction (in resulting asm) for each MDIO direct access OP.
What I know is the bus read and write methods are hardcoded on the driver. mt7530-mdio uses bus->write and bus->read which are Clause 22 access methods. Clause 45 access methods are bus->write_c45 and bus->read_c45.
We know that MT7621 has interrupts and I know that standalone MT7530 comes with SoCs that are as powerful as the SoCs MT7531 comes with. So, on MT7621, I will do traffic shaping in software and compare the throughput with indirect and direct PHY access to see how much indirect PHY access affects performance.
Yes, stmmac the non gmac4/xgmac variant. Check stmmac_mdio_register() on drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c for the assigning of the bus read and write functions.
Edit: arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts has mt7531. Its SoC's ethernet is "snps,dwmac-4.20a" which is given has_gmac4 so Clause 45 must be supported on this one. So I don't see any device trees on mainline Linux where the SoC doesn't support Clause 45. You don't mean to suggest we should use Clause 45 for accessing the switch, right? That would restrict which SoCs the switch can be controlled on.
Yes I did. What's wrong with your suggestion now that I seem to understand it is that you want to utilise the device tree to enable/disable a feature. That is not what dt-bindings are for. This would be implemented in userspace, not by a device tree property.
That's true only for the early core_* functions which are used before the switch is fully initialized. The main mt7530_phy_* functions forward calls to mdiobus_*_nested functions, which will decide this based on bus capabilities.
Obviously that will not show anything, as MT7621 uses interrupts and doesn't do PHY polling hence. I'd suggest to use MT7623 under full load for this experiment which is not as high-powered as the more recent Aarch64-based SoCs typically used with MT7531.
No. Is that even possible with the MT753x family? I'd assume that the SMI interface is C22-only or are there ways to access switch registers using C45 at all?
Well, the only reason to use indirect access on MT7530 and MT7621 would be to avoid address collisions with other PHYs or switches present on the same MDIO bus. And that's a hardware property, so it should be in DT. It doesn't need to be an additional property, but we can simply check for other PHYs being defined in DT at addresses which would be used by MT7530 if using direct access to the switch PHYs and only in that case use indirect PHY access.
I don't see how that's the case. There're the mdiobus_{read,write}_nested() for Clause 22, and mdiobus_c45_{read,write}_nested() for Clause 45. I've already documented the code path for mdiobus_{read,write}_nested() above.
I don't know.
I didn't argue whether or not the attribute is a hardware property. If you said that the property would be described on boards where the bootstrapping pin is pulled high, that would've made sense. Using it to enable/disable a feature on the hardware is incorrect use of the device tree specification.
I don't think this would ever happen in a board design made by a sane organisation. This would be dealt with at the stage of designing the board, therefore, this code path on the driver would never have a use.
So why would mdiobus_c45_{read,write}_nested() use Clause-45-over-Clause-22 on a Clause-45-capable bus?
I thought that was your no.1 argument for always using indirect access on MT7530?
If that is not needed, then lets please just forget about both. Lets keep using direct PHY access on MT7530 and MT7621. Bear in mind that not all PHYs can easily be configured for arbitrary MDIO bus addresses, usually there are 2 or 3 bits of freedom while the remaining 2 bits are fixed. If someone was to put 2x MT753x on a board which also got some additional external PHYs (connected either as Port 5/6 to the MD753x or to a MAC provided by the SoC) it could be a problem -- and I thought that was the case for your upcoming design and the motivation for always using indirect addressing even on MT7530 and MT7621.
As you have seen in your inbox, I have a patch ready for that case, it would check whether there are PHY address collisions and only in that case use indirect access while not bothering everyone else with that.
It wouldn't. Why are you asking me this? I was trying to explain that, what I see is that phylink does not do dynamic deciding of whether to use Clause 22 or Clause 45 access methods, it is hardcoded on the driver that utilises the MDIO bus.
No, it's not a case of necessity. It's just an observation. My folks don't need freeing up 5 PHY addresses on the MDIO bus. My only motive is simplifying the code on mainline. The third benefit (Clause 45 method access to Clause 45 registers instead of Clause 45 over Clause 22 method access to reduce MDIO bus interactions) comes as a by-product of it. Whilst your patch could be made to provide the latter, it can't provide the former.
If I'm following this conversation properly, your point why indirect access should be avoided if possible has been about your theory that there will be network performance degradation if it's used on a board without interrupts. Don't get me wrong when I say "theory", I haven't seen the evidence of such degradation, that's why I call it that. Therefore, as I said previously, I will test it and confirm your theory. After that, I can let this go.
So core_rmw is now only used by core_set and core_clear. Both functions are used exactly 2 times each during the setup phase of the switch. They are not used for PHY polling or any other PHY access after those total of 4 calls during initial setup. Your recent patch which I have reviewed has already majorly simplified that and reduced the need to individual locking and has also reduced the C45-over-C22 overhead.
It is obvious that MDIO operations will be multiplied, and there are several busy-poll-loops as well as lock/unlock operations involved.
All that is costly, I don't see a need to measure that, and you should be able to see that too, you've even already shown the call-trace in your previous post and the difference in length, number of function calls (context switching), locks, poll-loops, ... says it all. Why do both of us need to spend more time on this?
You can refer to this patch to see the simplification in detail. This patch applies over my simplification that I have recently submitted.
And yes, your point that the core operations are only used once during initial setup still stands.
Sorry, but I'm not convinced by reading this that whether or not the overhead is negligible. That's why I will spend some time testing it. I am not asking you to allocate any more time on this than you already did, and I thank you for that.
Edit: I would also like to point out that the phy_read_c22, phy_write_c22, phy_read_c45, and phy_write_c45 members of the mt753x_info structure would become unnecessary. I can't tell how beneficial this would be to small processors like MT7621.
Yes, I know, that's the patch I was talking about and have already sent an answer to, because I've reviewed and tested it last night.
The thing it, of course, that a decision to be made on test results (rather than on code facts which can be understood by everyone by just reading the code), that test (or those tests) will have to be reproduced, and I already see the debate about the test setup and conditions coming up on the horizon... What are you going to measure (jitter in round-trip time of a high number of small packets per second, I hope, as that would emulate e.g. several voice calls where jitter does matter and quickly result in both ends setting up larger buffers to compensate for the worst-case, and hence increase call latency as a result). What will be the test setup (routing in software path with some shaper qdisc near the obtainable maximum speed on the SoC, I hope)? What workload will you use to make all CPU cores busy with other background tasks in kernel or userspace (I'd suggest stuff like openssl speed). And so on...
All that being said, I appreciate you take this serious and are going to throw some science methods on it.
Reducing the size of that struct by 16 bytes (4x 32-bit pointer on those small 32-bit CPUs) could make a difference, indeed. Caches are small. However, if the cost is having several additional readx_poll_timeout and pointer dereference actions (which may anyway invalidate the cache and also mean additional memory read/write ops) in the codepaths instead, I'd conclude that the costs outweigh the benefits.
If you're talking about the one linked below as the one that you've reviewed, the patch on my previous reply is a different patch that applies on top of the one you've reviewed.
These are all very helpful suggestions. I was thinking of routing between interfaces while using cake for traffic shaping, without any flow offloading options enabled.
If you tell me that PHYs even on the very old MT7530 and MT7621 are fine with native Clause-45 and there was never a real reason to use Clause-22 MMD register access, then cool, of course this is a very good idea to do. I don't really have time to test this on a lot of boards now, but if you test it on any random MT7621 board, the BPi-R2 with a dedicated MT7530 and the BPi-R64 with a MT7531 chip then it will probably work fine on all of those.
Sounds about right, just make sure you have a meaningful way to measure RTT and jitter, for example by "flood-pinging" with ICMP echo, but that's not as realistic as using dedicated traffic generator and receiver to emit a high number of UDP RTP-like streams with unique IP addresses and ports.
Yes, I've tested it on an MT7621 board and BPI-R2.
There was a real reason. As I said previously, with direct PHY access, Clause 22 must be used to ensure access to PHYs in case of SoCs that don't support Clause 45. Once we switch to indirect PHY address, we know that the indirect PHY access register supports Clause 45.
Because MT7531 only provides indirect access, the driver already uses Clause 45 to access the PHY registers. You can look at my patch that enables EEE on MT7531.
Imho it should be done with C45 if possible and C45-over-C22 only if needed. You can easily know if C45 is supported by the bus by checking if the bus->read_c45 and bus->write_c45 function pointers are non-NULL.
@Gingernut
I was using stock firmware for my device, suddenly it has bricked, only Wifi LED will be turned on after power on. No response on attempting rescue mode booting, no response on uart connection via usbttl.
I'd like to flash the nand as the last try. I just saw you shared full ubi0 backup before, but it seems expired now. Could you pleasd share the full nand content backup with me?
Can someone tell me in detail how to revert to stock asus with facinstall.
I tried many times according to what the remittor says but I didn't succeed.
Revert every time to openwrt.
