Re: [PATCH] Revert "Bluetooth: hci_qca: Set BDA quirk bit if fwnode exists in DT"

[Janaki Ramaiah Thota <quic_janathot@quicinc.com>]

On 4/3/2024 5:54 PM, Johan Hovold wrote:
> On Fri, Mar 29, 2024 at 12:55:40PM +0530, Janaki Ramaiah Thota wrote:
>> On 3/28/2024 8:53 PM, Johan Hovold wrote:
>>> On Thu, Mar 28, 2024 at 08:25:16PM +0530, Janaki Ramaiah Thota wrote:
> 
>>>> We made this change to configure the device which supports persistent
>>>> memory for the BD-Address
>>>
>>> Can you say something more about which devices support persistent
>>> storage for the address? Is that all or just some of the chip variants?
> 
>> Most of the devices support persistent storage, and bd-address storage
>> is chosen based on the OEM and Target.
> 
> Can you be more specific about which devices support it (or say which do
> not)?
> 

We know below BT chipsets supports persistent storage(OTP) for BDA
WCN7850, WCN6855, WCN6750

> Is the address stored in some external eeprom or similar which the OEM
> can choose to populate?
>
  
This persistent storage is One Time Programmable (OTP) reserved memory
which resides in the BT chip.

>>>> So to make device functional in both scenarios we are adding a new
>>>> property in dts file to distinguish persistent and non-persistent
>>>> support of BD Address and set HCI_QUIRK_USE_BDADDR_PROPERTY bit
>>>> accordingly
>>>
>>> Depending on the answer to my questions above, you may be able to infer
>>> this from the compatible string and/or you can read out the address from
>>> the device and only set the quirk if it's set to the default address.
>>>
>>> You should not need to add a new property for this.
> 
>> As per my understanding, altering the compatible string may cause duplicate
>> configuration, right ?
> 

Yes, we are correct.

> If it's the same device and just a different configuration then we can't
> use the compatible string for this.
> 
> It seems we need a patch like the below to address this. But please
> provide some more details (e.g. answers to the questions above) so I can
> determine what the end result should look like.
> 
> Johan
> 
> 
>  From 9719effe80fcc17518131816fdfeb1824cfa08b6 Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan+linaro@kernel.org>
> Date: Thu, 20 Apr 2023 14:10:55 +0200
> Subject: [PATCH] Bluetooth: btqca: add invalid device address check
> 
> Some Qualcomm Bluetooth controllers lack persistent storage for the
> device address and therefore end up using the default address
> 00:00:00:00:5a:ad.
> 
> Apparently this depends on how the controller has been integrated so we
> can not use the device type alone to determine when the address is
> valid.
> 
> Instead read back the address during setup() and only set the
> HCI_QUIRK_USE_BDADDR_PROPERTY flag when needed.
> 
> Fixes: de79a9df1692 ("Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY")
> Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts")
> Fixes: 6945795bc81a ("Bluetooth: fix use-bdaddr-property quirk")
> Cc: stable@vger.kernel.org	# 6.5
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/bluetooth/btqca.c   | 33 +++++++++++++++++++++++++++++++++
>   drivers/bluetooth/hci_qca.c |  2 --
>   2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 19cfc342fc7b..15124157372c 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -15,6 +15,8 @@
>   
>   #define VERSION "0.1"
>   
> +#define QCA_BDADDR_DEFAULT (&(bdaddr_t) {{ 0xad, 0x5a, 0x00, 0x00, 0x00, 0x00 }})
> +
>   int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>   			 enum qca_btsoc_type soc_type)
>   {
> @@ -612,6 +614,35 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>   }
>   EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
>   
> +static void qca_check_bdaddr(struct hci_dev *hdev)
> +{
> +	struct hci_rp_read_bd_addr *bda;
> +	struct sk_buff *skb;
> +	int err;
> +
> +	if (bacmp(&hdev->public_addr, BDADDR_ANY))
> +		return;
> +
> +	skb = __hci_cmd_sync(hdev, HCI_OP_READ_BD_ADDR, 0, NULL,
> +			     HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		err = PTR_ERR(skb);
> +		bt_dev_err(hdev, "Failed to read device address (%d)", err);
> +		return;
> +	}
> +
> +	if (skb->len != sizeof(*bda)) {
> +		bt_dev_err(hdev, "Device address length mismatch");
> +		goto free_skb;
> +	}
> +
> +	bda = (struct hci_rp_read_bd_addr *)skb->data;
> +	if (!bacmp(&bda->bdaddr, QCA_BDADDR_DEFAULT))
> +		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
> +free_skb:
> +	kfree_skb(skb);
> +}
> +
>   static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>   		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
>   {
> @@ -818,6 +849,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   		break;
>   	}
>   
> +	qca_check_bdaddr(hdev);
> +
>   	bt_dev_info(hdev, "QCA setup on UART is completed");
>   
>   	return 0;
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index b266db18c8cc..b621a0a40ea4 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1908,8 +1908,6 @@ static int qca_setup(struct hci_uart *hu)
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> -		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
> -
>   		qcadev = serdev_device_get_drvdata(hu->serdev);
>   		if (qcadev->bdaddr_property_broken)
>   			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);

Thanks for the patch. This change looks fine and it will resolve the current OTP issue.

--
Thanks,
JanakiRam