Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ar71xx: Fix 5 GHz Wi-Fi on NBG6716 #655

Closed
wants to merge 1 commit into from

Conversation

malaakso
Copy link
Contributor

NBG6716 has calibration data in the PCI-E card EEPROM, not in flash.

See: https://forum.lede-project.org/t/17-01-1-2-3-qca988x-ath10k-5ghz-firmware-crashed-zyxel-nbg6716/1911?u=malaakso

Signed-off-by: Matti Laakso matti.laakso@outlook.com

@pepe2k pepe2k added the target/ar71xx pull request/issue for ar71xx target label Jan 15, 2018
@pepe2k
Copy link
Member

pepe2k commented Jan 17, 2018

Hi @malaakso,

It seems that manufacturer has different versions of this device or even different versions of the AC card installed inside. Photos of the AC card from FCC don't show any EEPROM on it (see: I88NBG6716).

And actually, I have this device here and it has two calibration data in mtd2 partition which means that your change would break these devices. This issue requires more sophisticated fix (e.g. check if calibration data exists in flash and don't use it if it doesn't).

Cheers,
Piotr

@pepe2k pepe2k self-assigned this Jan 17, 2018
@malaakso
Copy link
Contributor Author

@pepe2k Yeah, that's something I feared could be the case. I think it is enough to check the first few bytes of the calibration data on flash to make sure that it is valid, and go from there. I'll update the patch. I assume the calibration-data-in-flash contains the default MAC address (00:03:7f:12:34:56) in your case?

@pepe2k
Copy link
Member

pepe2k commented Jan 17, 2018

@malaakso,

I think it is enough to check the first few bytes of the calibration data on flash to make sure that it is valid, and go from there. I'll update the patch.

Sounds good to me.

I assume the calibration-data-in-flash contains the default MAC address (00:03:7f:12:34:56) in your case?

Both radios MACs in calibration data seem fake and don't match what's on label but they are different than yours. What's more, they are the same in at least two devices I had access to:

2G: 00:02:6F:12:34:56
5G: EC:43:F6:F9:18:09
Label: FC:F5:28:Dx:xx:xx

Cheers,
Piotr

@malaakso
Copy link
Contributor Author

@pepe2k Please review.

Copy link
Member

@pepe2k pepe2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@malaakso, thanks for your work on this. Please have a look at my comments below.

@@ -18,12 +18,19 @@ ath10kcal_extract() {
local part=$1
local offset=$2
local count=$3
local mtd
local mtd size1 size2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer variable name which is more descriptive, maybe cal_size?
(why just one, see below)


mtd=$(find_mtd_chardev $part)
[ -n "$mtd" ] || \
ath10kcal_die "no mtd device found for partition $part"

# Check that the calibration data size in header equals the desired size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about possible regression if we apply this check globally, for all boards.
Are you able point out any information about header layout and if it's the same for all ath10k chipsets?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the header layout is the same for all current ath10k chips, both cal and pre-cal data. The same check is made in Qualcomm driver.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me where I can find it (link or what source file look into)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!
I think it's safe to keep that check for all boards.


mtd=$(find_mtd_chardev $part)
[ -n "$mtd" ] || \
ath10kcal_die "no mtd device found for partition $part"

# Check that the calibration data size in header equals the desired size
size1=`dd if=$mtd bs=2 count=1 skip=$(( $offset / 2 )) conv=swab 2>/dev/null | hexdump -ve '1/2 "%.4x"'`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use var=$(...) instead of var=`...` . And, what about something different (no need to change count to hex):

cal_size=$(dd if=$mtd bs=2 count=1 skip=$(($offset/2)) conv=swab 2>/dev/null | hexdump -ve '1/2 "%d"')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I was somehow so completely fixated on getting it to hex that the easier route eluded me. I'll send a new version soon.

size2=`printf %.4x $count`

[ $size1 != $size2 ] && \
ath10kcal_die "no calibration data found from $part"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ath10kcal_die "no calibration data found in $part"

size1=`dd if=$mtd bs=2 count=1 skip=$(( $offset / 2 )) conv=swab 2>/dev/null | hexdump -ve '1/2 "%.4x"'`
size2=`printf %.4x $count`

[ $size1 != $size2 ] && \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing: [ "$count" = "$cal_size" ] || \

Some NBG6716 do not have ath10k calibration data in flash, only in chip OTP. To
determine if flash has valid calibration data, the first two bytes telling the
length of the calibration data are checked against the requested length. If the
lengths match, calibration data is valid and read from flash.

See: https://forum.lede-project.org/t/17-01-1-2-3-qca988x-ath10k-5ghz-firmware-crashed-zyxel-nbg6716/1911?u=malaakso

Signed-off-by: Matti Laakso <matti.laakso@outlook.com>
@malaakso
Copy link
Contributor Author

@pepe2k Updated pull request, please review.

@pepe2k
Copy link
Member

pepe2k commented Feb 19, 2018

@malaakso sorry for the delay, I have merged your changes into my staging tree, thanks!

Cheers,
Piotr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target/ar71xx pull request/issue for ar71xx target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants