Hi Johan,
Having a default BDA list from NVM BDA tag value will prevent developers
from using the device if there is no user space app(In Fluoride) to set
the BDA. Therefore, we are requesting to use default address check patch,
so that developer can change the NVM BDA to make use of the device.
List Of default Addresses:
---------------------------------------------------------
| BDA | Chipset |
---------------------------------------------------------
| 39 80 10 00 00 20 | WCN3988 with ROM Version 0x0200 |
---------------------------------------------------------
| 39 80 12 74 08 00 | WCN3988 with ROM Version 0x0201 |
---------------------------------------------------------
| 39 90 21 64 07 00 | WCN3990 |
---------------------------------------------------------
| 39 98 00 00 5A AD | WCN3991 |
---------------------------------------------------------
| 00 00 00 00 5A AD | QCA DEFAULT |
---------------------------------------------------------
On 4/27/2024 3:21 PM, Johan Hovold wrote:
On Fri, Apr 26, 2024 at 10:23:15AM -0700, Doug Anderson wrote:
On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <johan+linaro@xxxxxxxxxx> wrote:
The default device address apparently comes from the NVM configuration
file and can differ quite a bit.
Store the default address when parsing the configuration file and use it
to determine whether the controller has been provisioned with an
address.
This makes sure that devices without a unique address start as
unconfigured unless a valid address has been provided in the devicetree.
int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
enum qca_btsoc_type soc_type)
{
@@ -351,6 +348,11 @@ static void qca_tlv_check_data(struct hci_dev *hdev,
/* Update NVM tags as needed */
switch (tag_id) {
+ case EDL_TAG_ID_BD_ADDR:
+ if (tag_len != sizeof(bdaddr_t))
+ break;
+ memcpy(&config->bdaddr, tlv_nvm->data, sizeof(bdaddr_t));
+ break;
case EDL_TAG_ID_HCI:
nit: blank line after "break" ?
Possibly, the driver isn't really consistent here and only two case
statements have such a newline after break.
Also note that on my firmware I never see this tag and thus your patch
breaks trogdor. Specifically I put a printout here and it never gets
hit.
Thanks for the quick test. As the parser is modifying the configuration
file I assumed it was correct and tested...
I printed all the tags/lengths:
[ 17.961087] DOUG: id 0xde02, len 0x0010
[ 17.965081] DOUG: id 0x0000, len 0x0000
[ 17.969050] DOUG: id 0x0000, len 0x0011
[ 17.973025] DOUG: id 0x0000, len 0x0a00
[ 17.976991] DOUG: id 0x0303, len 0x0303
[ 17.981066] DOUG: id 0x0033, len 0x1001
Probably EDL_TAG_ID_BD_ADDR should have been 0xde02, not just 2.
No, the parser is apparently broken and fails to consider an extra
four-byte header found in some NVM files and just happily parses and
potentially modifies (sic!) random bytes.
I've fixed the parser so that it works also on configuration files with
the extra header (apnv??.bin, crnv??[u].bin) and can read out the
default address for all NVM files in linux-firmware that have one
(otherwise all-zeroes is printed below):
bluetooth hci0: bd_addr = 39:80:10:00:00:20 (qca/apnv10.bin)
bluetooth hci0: bd_addr = 39:80:12:74:08:00 (qca/apnv11.bin)
bluetooth hci0: bd_addr = 39:90:21:64:07:00 (qca/crnv21.bin)
bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32.bin)
bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32u.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.301)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.302)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.309)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.301)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.302)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.309)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/htnv20.bin)
bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b09)
bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b0a)
bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.bin)
bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin)
bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin)
bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_00230302.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_eu.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_i2s_eu.bin)
bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000200.bin)
bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000201.bin)
bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000300.bin)
bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302.bin)
bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302_eu.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0104.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0105.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0106.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0107.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0109.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0110.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010a.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010b.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_0303.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010a.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010b.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_0303.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf.bin)
bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00190200.bin)
It looks like we're being lucky and the parser is at least not
corrupting the configuration files with the extra header currently in
linux-firmware, but if it ever interprets a random 0x0011 or 0x001b word
as a tag it would.
Fixing the parser means that we would start modifying the configuration
also for files with the extra header. This involves configuring the baud
rate and enabling a deep sleep feature.
Presumably this is something that should be done also on Trogdor, but
this would obviously have to be tested first. I guess we can keep
skipping this step until it has been verified and just read out the
address for now.
@@ -624,6 +626,9 @@ static int qca_check_bdaddr(struct hci_dev *hdev)
if (bacmp(&hdev->public_addr, BDADDR_ANY))
return 0;
+ if (!bacmp(&config->bdaddr, BDADDR_ANY))
+ return 0;
The above test feels non-obvious enough to deserve a comment. Could
you add one? That would also help alleviate my confusion since I
_think_ your if test is unneeded and maybe wrong? Let's say that the
firmware didn't have a default address stored in it. It still seems
like we could try to read the address and then if the firmware gave
back BDADDR_ANY (0) we should set the `HCI_QUIRK_USE_BDADDR_PROPERTY`
property, right?
You're right. I'll drop this check when revisiting this next week.
Johan
-Janaki Ram
