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

From: Zijun Hu
Date: Tue Nov 02 2021 - 05:42:35 EST

Next message: James Clark: "Re: [PATCH v2 2/2] kallsyms: ignore arm mapping symbols when loading module"
Previous message: Zqiang: "Re: [PATCH] ALSA: seq: Fix RCU stall in snd_seq_write()"
In reply to: Marcel Holtmann: "Re: [PATCH v1 3/3] Bluetooth: hci_qca: Add support for QTI bluetooth MAPLE"
Next in thread: Marcel Holtmann: "Re: [PATCH v1 3/3] Bluetooth: hci_qca: Add support for QTI bluetooth MAPLE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 11/2/2021 5:22 PM, Marcel Holtmann wrote:
> 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.

> Regards
>
> Marcel
>

Next message: James Clark: "Re: [PATCH v2 2/2] kallsyms: ignore arm mapping symbols when loading module"
Previous message: Zqiang: "Re: [PATCH] ALSA: seq: Fix RCU stall in snd_seq_write()"
In reply to: Marcel Holtmann: "Re: [PATCH v1 3/3] Bluetooth: hci_qca: Add support for QTI bluetooth MAPLE"
Next in thread: Marcel Holtmann: "Re: [PATCH v1 3/3] Bluetooth: hci_qca: Add support for QTI bluetooth MAPLE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]