Mt7620a TX_SW_CFG problem

improving mt7620 support i've noticed duplicate TX_SW_CFG entry for RT6352 chip:

	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
} else if (rt2x00_rt(rt2x00dev, RT5390) ||
	   rt2x00_rt(rt2x00dev, RT5392) ||
	   rt2x00_rt(rt2x00dev, RT6352)) {
		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
} else if (rt2x00_rt(rt2x00dev, RT5592)) {
		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00000000);
		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
} else if (rt2x00_rt(rt2x00dev, RT6352)) {
		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000401);
		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x000C0000);
		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
		rt2800_register_write(rt2x00dev, MIMO_PS_CFG, 0x00000002);
		rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x00150F0F);

and got info that this redundant entry should be removed. after removing RT6352 from first TX_SW_CFG* group the device seem to work fine at first but the TX rate stays as low as 81 or 90 Mbps - similar issue i get with patched CC or DD image.

i've also tried to set RT6352 TX_SW_* entries to 404 and 80606, even tried the sum of the two - no go, 90 Mbps TX rate.

with both RT6352 TX_SW_* entries i get better TX rates (from 81-90 increased to 162-180 Mbps) and better effective throughput in both directions while using LEDE. (in openwrt this makes no difference, either CC or DD).

i wonder what is going on in the driver, does it constantly keeps switching between the two values (404->401, 80606->C0000) and is there a way to stabilize tx switch?

hi tom!

you are right, having two entries for RT6352 is obviously wrong. what happends is that only the first match (which is also for RT5390 and RT5392) gets executed because the if ... else statement works that way and the follow-up if statement is part of the else block of the previous statement and hence doesn't ever get executed. the clean solution would probably be to remove the match for RT6352 from the first statement which will get the block matching for RT6352 further down get executed.
can you prepare a patch which makes the changes in such way like you experienced the improved thoughput?

cheers

daniel

sorry if i didn't explain this well enough:

using

} else if (rt2x00_rt(rt2x00dev, RT5390) ||
	   rt2x00_rt(rt2x00dev, RT5392)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
	rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
	rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
} else if (rt2x00_rt(rt2x00dev, RT5592)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
	rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00000000);
	rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
} else if (rt2x00_rt(rt2x00dev, RT5350)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
} else if (rt2x00_rt(rt2x00dev, RT6352)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000401);
	rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x000C0000);
	rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
	rt2800_register_write(rt2x00dev, MIMO_PS_CFG, 0x00000002);
	rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x00150F0F);

or this

} else if (rt2x00_rt(rt2x00dev, RT5390) ||
	   rt2x00_rt(rt2x00dev, RT5392)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
	rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
	rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
} else if (rt2x00_rt(rt2x00dev, RT5592)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
	rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00000000);
	rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
} else if (rt2x00_rt(rt2x00dev, RT5350)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
} else if (rt2x00_rt(rt2x00dev, RT6352)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
	rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
	rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
	rt2800_register_write(rt2x00dev, MIMO_PS_CFG, 0x00000002);
	rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x00150F0F);

does not help with TX rate. only a combination of both works. is it possible that actual RT6352 registers are not executed for some reason?

I prepared a patch in my staging tree which fixes the obvious bug there.
See commit b10fe9ea3e (mac80211: rt2x00: apply correct TX_CFG_CFG* values for MT7620). I also added register descriptions for all three TX_SW_CFG* registers as found in the MT7620 PROGRAMMING GUIDE so we can have a more informed debate about what these should be set to in rt2x00. For now I just made sure that value applied are the same as in the vendor driver referenced in OpenWrt #22210, see RT6352_MACRegTable in rt6352.c.

some more testing, this seems to work for better TX rate:

} else if (rt2x00_rt(rt2x00dev, RT5390) ||
	   rt2x00_rt(rt2x00dev, RT5392) ||
	   rt2x00_rt(rt2x00dev, RT6352)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
	rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
	rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
} else if (rt2x00_rt(rt2x00dev, RT5592)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
	rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00000000);
	rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
} else if (rt2x00_rt(rt2x00dev, RT5350)) {
	rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
} else if (rt2x00_rt(rt2x00dev, RT6352)) {
	rt2800_register_write(rt2x00dev, MIMO_PS_CFG, 0x00000004);
	rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x00150F0F);
	rt2800_register_write(rt2x00dev, TX_ALC_VGA3, 0x06060606);

i've noticed in code we don't have RT6352 defined as other chips, but using RT5390 to identify this chip. could it be the source of the problem?

if (rt == RT5390 && rt2x00_is_soc(rt2x00dev))
	rt = RT6352;

The change you suggest will never reach code below the

} else if (rt2x00_rt(rt2x00dev, RT6352)) {

because the if-condition

} else if (rt2x00_rt(rt2x00dev, RT5390) ||
	   rt2x00_rt(rt2x00dev, RT5392) ||
	   rt2x00_rt(rt2x00dev, RT6352)) {

is already satisfied, hence the else part isn't ever executed. Hence the code you suggest now is semantically identical to the version before the fix, because the instruction pointer anyway never makes it's way to the lower part.

About detecting RT6352: Unfortunately RT6352 identfifies itself as RT5390, however, RT5390 is a PCIe chip and also got slightly different properties as RT6352 aka. MT7620. In Roman's original patch RT6352 was treated as RT5390 and all those differentiations were made based on the RF frontend detection being RF7620 (though they are not RF-specific). This was discussed when first submitting the patch upstream, see this thread on linux-wireless. Hence I introduced detecting RT6352 explicitely in the follow-up patch which then was accepted.

is it possible to hardcode 0x6352 value in driver and make it not read from chip address 0x10181000?

this is not required as it's already set from driver config channel function:
rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x00150F0F);

That's exactly what is done in the driver

        if (rt == RT5390 && rt2x00_is_soc(rt2x00dev))
                rt = RT6352;

This detection method seems to work reliable for all MT7620 variants, as they all mis-identify as RT5390 and the actual RT5390 simply isn't a WiSoC, hence using rt2x00_is_soc(rt2x00dev) to differentiate between RT5390 and RT6352 works perfectly. Did you observe any problem with that (the detected chipset is printed onto the kernel log, so it's easy to see if anything goes wrong there)?

I don't understand how this is related to the above. TX_PIN_CFG assignment is indeed a bit redundant, but that shouldn't harm. Where do you see the problem?

Also, lets get back to the TX_SW_CFG* values being programmed into the MAC. You were right that the if-clause as it was simply doesn't make sense. If you are under impression that MT7620 aka. RT6352 works better with the values intended for RT5390 and RT5392 rather than the values found in the vendor driver then that should be verified thoroughly and we should change it.
For now, please retest on your hardware and build LEDE from my staging tree.

i have thought rt5390 is 2x2 chip but when i checked specs it's 1x1, and TX path somehow seems to act more like it is 1 stream, when using lower transmit power it can get to 180Mbps but most of the time 135 or 150 Mbps (MCS6, MCS7) which are 1 stream rates. with same TX level quality i think driver needs only make better use of both chains, maybe even mimo powersaving affects this..

if the chip misidentification is causing this then i think differentiation should be done on lower level, i have tried to set register in DTS code but found it is readonly. it would be the best if the rt2x00 could read RT6352 value instead of RT5390 and i think it can be done since phy1 is probed ~50 sec after firstboot.

it's not good. i've thought you would read carefully my notes about specific registers, instead you just messed them up.

Well, please be more specific about your judgements, otherwise it's pretty hard to find a constructive way to deal with your criticizm. Also note that I'm well aware of diverging from your original suggestion and I explained in full length and using quite a lot of references why I did so. I won't just "do as you told" if all you got to add is that you believe that this made a mess without explaining why nor having suggested anything even remotely reasonable about that disired change of yours in the past (and yes, you'll need to make the argument why a certain change is needed and it should be plausible in terms of code being actually reachable during execution and either refer to what's in the datasheet or vendor driver).
I do apppreciate your efforts and try to make sense of contributions (which wouldn't be necessary if you actually submitted well-formated patches and submitted them to linux-wireless yourself). However, the source of my appreciation has never been your delightful attitude but rather just my will to get rt2x00 working well on MT7620. From your previous post I assume that changing MIMO_PS_CFG from 0x2 to 0x4 is the change you actually wanted to suggest...? That would be aligned with what's in the datasheet but isn't related to the other changes of TX_SW_CFGx...

The vendor driver does exactly the same as we do now to identify RT6352, please see

no

i have wrote in my emails what is wrong and you included these changes, please read again and get yourself together before making any more commits.

You have written a lot, and a lot of it simply didn't make sense to me, such as the whole debate about detecting RT6352 or changing code inside sections which are obviously unreachable -- yet I'm thankful for you to point out unreachable code and reviewing stuff which I haven't ever looked at closely enough. If you want a change to be included, write a patch and submit it. In case you are refering to changes you tried to submit on github (?), well, I had to clean them up because you were making changes on a patch which has already been merged upstream. Anyway, feel free to submit further patches to linux-wireless (preferably) or lede-dev mailing lists.

as i said, the whole function seems to work incorrect. i've put this and removed all other chips from rt2800_init_registers function

if (rt2x00_rt(rt2x00dev, RT5390)) {
rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
rt2800_register_write(rt2x00dev, MIMO_PS_CFG, 0x00000004);
rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x00150F0F);
rt2800_register_write(rt2x00dev, TX_ALC_VGA3, 0x06060606);
rt2800_register_write(rt2x00dev, TX0_BB_GAIN_ATTEN, 0x18181818);
rt2800_register_write(rt2x00dev, TX1_BB_GAIN_ATTEN, 0x18181818);
rt2800_register_write(rt2x00dev, TX0_RF_GAIN_ATTEN, 0x6C6C666C);
rt2800_register_write(rt2x00dev, TX1_RF_GAIN_ATTEN, 0x6C6C666C);
rt2800_register_write(rt2x00dev, TX0_RF_GAIN_CORRECT, 0x3630363A);
rt2800_register_write(rt2x00dev, TX1_RF_GAIN_CORRECT, 0x3630363A);
rt2800_register_read(rt2x00dev, TX_ALC_CFG_1, &reg);
rt2x00_set_field32(&reg, TX_ALC_CFG_1_ROS_BUSY_EN, 0);
rt2800_register_write(rt2x00dev, TX_ALC_CFG_1, reg);
}

this works for a somewhat better tx rates and device seems to work faster which can be noticed when opening luci pages. combination of all executed TX_SW_CFG registers (be it for RT5390 and RT6352 only or even some others) gets better rates but also some noticeable glitches when loading interfaces or wifi luci pages.

This topic was automatically closed 10 days after the last reply. New replies are no longer allowed.