Re: [PATCH v1 3/3] Bluetooth: hci_qca: Add support for QTI bluetooth MAPLE

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Zijun,

>>>>>>>> Add support for MAPLE integrated within SOC, it is mounted on
>>>>>>>> a virtual tty port and powered on/off via relevant IOCTL, neither
>>>>>>>> IBS nor RAMPATCH downloading is not required.
>>>>>>>> 
>>>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> drivers/bluetooth/btqca.c   | 13 ++++++++++++-
>>>>>>>> drivers/bluetooth/btqca.h   | 13 +++++++++++++
>>>>>>>> drivers/bluetooth/hci_qca.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>> 3 files changed, 71 insertions(+), 2 deletions(-)
>>>>>>>> 
>>>>>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>>>>>>>> index be04d74037d2..b83d2ecefe5d 100644
>>>>>>>> --- a/drivers/bluetooth/btqca.c
>>>>>>>> +++ b/drivers/bluetooth/btqca.c
>>>>>>>> @@ -255,6 +255,8 @@ static void qca_tlv_check_data(struct hci_dev *hdev,
>>>>>>>> 		BT_DBG("TLV Type\t\t : 0x%x", type_len & 0x000000ff);
>>>>>>>> 		BT_DBG("Length\t\t : %d bytes", length);
>>>>>>>> 
>>>>>>>> +		if (qca_is_maple(soc_type))
>>>>>>>> +			break;
>>>>>>>> 		idx = 0;
>>>>>>>> 		data = tlv->data;
>>>>>>>> 		while (idx < length) {
>>>>>>>> @@ -552,6 +554,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>>>> 	rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
>>>>>>>> 
>>>>>>>> 	/* Download rampatch file */
>>>>>>>> +	if (qca_is_maple(soc_type))
>>>>>>>> +		goto download_nvm;
>>>>>>>> +
>>>>>>>> 	config.type = TLV_TYPE_PATCH;
>>>>>>>> 	if (qca_is_wcn399x(soc_type)) {
>>>>>>>> 		snprintf(config.fwname, sizeof(config.fwname),
>>>>>>>> @@ -580,6 +585,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>>>> 	/* Give the controller some time to get ready to receive the NVM */
>>>>>>>> 	msleep(10);
>>>>>>>> 
>>>>>>>> +download_nvm:
>>>>>>>> 	/* Download NVM configuration */
>>>>>>>> 	config.type = TLV_TYPE_NVM;
>>>>>>>> 	if (firmware_name)
>>>>>>>> @@ -597,6 +603,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>>>> 	else if (soc_type == QCA_QCA6390)
>>>>>>>> 		snprintf(config.fwname, sizeof(config.fwname),
>>>>>>>> 			 "qca/htnv%02x.bin", rom_ver);
>>>>>>>> +	else if (qca_is_maple(soc_type))
>>>>>>>> +		snprintf(config.fwname, sizeof(config.fwname),
>>>>>>>> +			 "qca/mpnv%02x.bin", rom_ver);
>>>>>>>> 	else if (soc_type == QCA_WCN6750)
>>>>>>>> 		snprintf(config.fwname, sizeof(config.fwname),
>>>>>>>> 			 "qca/msnv%02x.bin", rom_ver);
>>>>>>>> @@ -609,6 +618,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>>>> 		bt_dev_err(hdev, "QCA Failed to download NVM (%d)", err);
>>>>>>>> 		return err;
>>>>>>>> 	}
>>>>>>>> +	if (qca_is_maple(soc_type))
>>>>>>>> +		msleep(MAPLE_NVM_READY_DELAY_MS);
>>>>>>>> 
>>>>>>>> 	if (soc_type >= QCA_WCN3991) {
>>>>>>>> 		err = qca_disable_soc_logging(hdev);
>>>>>>>> @@ -637,7 +648,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>>>> 		return err;
>>>>>>>> 	}
>>>>>>>> 
>>>>>>>> -	if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) {
>>>>>>>> +	if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || qca_is_maple(soc_type)) {
>>>>>>>> 		/* get fw build info */
>>>>>>>> 		err = qca_read_fw_build_info(hdev);
>>>>>>>> 		if (err < 0)
>>>>>>>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>>>>>>>> index 30afa7703afd..0a5a7d1daa71 100644
>>>>>>>> --- a/drivers/bluetooth/btqca.h
>>>>>>>> +++ b/drivers/bluetooth/btqca.h
>>>>>>>> @@ -46,6 +46,8 @@
>>>>>>>> 
>>>>>>>> #define QCA_FW_BUILD_VER_LEN		255
>>>>>>>> 
>>>>>>>> +#define MAPLE_NVM_READY_DELAY_MS        1500
>>>>>>>> +#define MAPLE_POWER_CONTROL_DELAY_MS    50
>>>>>>>> 
>>>>>>>> enum qca_baudrate {
>>>>>>>> 	QCA_BAUDRATE_115200 	= 0,
>>>>>>>> @@ -145,6 +147,7 @@ enum qca_btsoc_type {
>>>>>>>> 	QCA_WCN3991,
>>>>>>>> 	QCA_QCA6390,
>>>>>>>> 	QCA_WCN6750,
>>>>>>>> +	QCA_MAPLE,
>>>>>>>> };
>>>>>>>> 
>>>>>>>> #if IS_ENABLED(CONFIG_BT_QCA)
>>>>>>>> @@ -167,6 +170,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
>>>>>>>> 	return soc_type == QCA_WCN6750;
>>>>>>>> }
>>>>>>>> 
>>>>>>>> +static inline bool qca_is_maple(enum qca_btsoc_type soc_type)
>>>>>>>> +{
>>>>>>>> +	return soc_type == QCA_MAPLE;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> #else
>>>>>>>> 
>>>>>>>> static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>>>>>>> @@ -204,6 +212,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
>>>>>>>> 	return false;
>>>>>>>> }
>>>>>>>> 
>>>>>>>> +static inline bool qca_is_maple(enum qca_btsoc_type soc_type)
>>>>>>>> +{
>>>>>>>> +	return false;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>>>>>>>> {
>>>>>>>> 	return -EOPNOTSUPP;
>>>>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>>>>> index dd768a8ed7cb..f1d9670719c4 100644
>>>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>>>> @@ -70,6 +70,10 @@
>>>>>>>> #define QCA_CRASHBYTE_PACKET_LEN	1096
>>>>>>>> #define QCA_MEMDUMP_BYTE		0xFB
>>>>>>>> 
>>>>>>>> +#ifndef IOCTL_IPC_BOOT
>>>>>>>> +#define IOCTL_IPC_BOOT                  0xBE
>>>>>>>> +#endif
>>>>>>> 
>>>>>>> You send this command, but never use it.  Where is the driver code that
>>>>>>> uses this command?
>>>>>>> 
>>>>>> qca_maple_power_control() will use it.  this driver depends on bt_tty kernel module
>>>>>> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/drivers/soc/qcom/bt_tty.c?h=NHSS.QSDK.11.5.0.5.r2
>>>>> 
>>>>> You can not add code to the kernel that is not used by the kernel
>>>>> itself.  That driver needs to be in the tree as well, why is it not
>>>>> submitted now too?
>>>>> 
>>>>>>> And why not tabs?
>>>>>>> 
>>>>>>> And why is this patch series not properly threaded so tools can pick it
>>>>>>> up and find them?
>>>>>>> 
>>>>>>> And why the odd named ioctl that is different from other ones in this
>>>>>>> file?
>>>>>>> 
>>>>>> that IOCTL name is defined by that module.
>>>>>> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/include/linux/bt.h?h=NHSS.QSDK.11.5.0.5.r2
>>>>> 
>>>>> Again, it needs to be in the tree.
>>>>> 
>>>>>>> And why not just use normal power management hooks for doing things like
>>>>>>> turning on and off the hardware like all other drivers?
>>>>>>> 
>>>>>> this device is special.
>>>>> 
>>>>> All drivers and devices are special and unique.  Just like all of them :)
>>>>> 
>>>>> What is so odd about this device that it can not work with the existing
>>>>> infrastructure that the kernel has for all of the hundreds of thousands
>>>>> of other devices it supports?
>>>>> 
>>>>>> it seems BT maintainer decides to drop this patch.
>>>>> 
>>>>> Of course, at the very least because there is no in-kernel user, why
>>>>> would you accept such a patch if you were the maintainer?
>>>>> 
>>>>> Please submit your driver first.
>>>> 
>>>> this power on via ioctl is nasty business. I am so happy that we got rid of
>>>> the crucks when we finally landed serdev.
>>>> 
>>>> Some people are working on power sequence support and alike. This needs to
>>>> use proper infrastructure or extend existing infrastructure. To fit the
>>>> needs.
>>>> 
>>>> I am just 100% certain, that booting an IPC via an ioctl isn’t it. We
>>>> really suffered through it in the 2.4 kernel days. The hardware needs to
>>>> be described properly in device tree and the kernel needs to take all
>>>> the appropriate actions if a Bluetooth device is powered on via its
>>>> standard power on procedure. And that is through bluetoothd (or if you
>>>> use some other Bluetooth userspace) via the exposed API from the kernel.
>>>> 
>>> thank you. the IOCTL purpose is to bootup the special bluetooth controller.
>>> i have verified this change.
>>> i will submit this change to linux-ipq-5.4 firstly even if need to pick up many changes firstly.
>> 
>> I am not sure this is fully understood yet. Do _not_ use an ioctl to boot the Bluetooth
>> controller. Power on/off of Bluetooth hardware happens via the standard interface used
>> by bluetoothd. The Bluetooth transport driver (in your case hci_qca) has to do everything
>> needed when a) serdev->probe is called and b) hdev->open is called. Any other path to
>> power your hardware is (bluntly put) wrong.
>> 
> for all the other present controllers supported by hci_qca,  ALL the finial initialization job is Done driver itself(hci_qca)
> via hdev->setup which call qca_setup(), the initialization includes power control, clock control, driver BT enable
> pin, download F/W.

and what I am saying, it has to be the same for this hardware as well. You need to
re-design your driver / patch.

On a side note, you don’t have to add support for this hardware into hci_qca. You
can write a brand new driver to support your hardware. If hci_qca is no fit, then
start from scratch. However same rules apply work in the constraints of serdev->probe
and hdev->open to establish the Bluetooth transport.

The hdev->setup is post-transport setup and doesn’t really apply to anything powering
on the controller.

Regards

Marcel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux