mbox series

[v1,0/2] Bluetooth: qca: Add tool btattach support for more QCA soc types

Message ID 1710914907-30012-1-git-send-email-quic_zijuhu@quicinc.com
Headers show
Series Bluetooth: qca: Add tool btattach support for more QCA soc types | expand

Message

Zijun Hu March 20, 2024, 6:08 a.m. UTC
Kernel header drivers/bluetooth/btqca.h defines many soc types as following:
enum qca_btsoc_type {
	QCA_INVALID = -1,
	QCA_AR3002,
	QCA_ROME,
	QCA_WCN3988,
	QCA_WCN3990,
	QCA_WCN3998,
	QCA_WCN3991,
	QCA_QCA2066,
	QCA_QCA6390,
	QCA_WCN6750,
	QCA_WCN6855,
	QCA_WCN7850,
	QCA_MAX,
};
and every soc type stands for a kind of QCA BT controller, but tool btattach currenlty
only supports default soc type QCA_ROME, this patch series are to add support for other
all other QCA soc types by adding a option for tool btattach to specify soc type.

Zijun Hu (2):
  Bluetooth: hci_ldisc: Add a ioctl HCIUARTSETPROTODATA
  Bluetooth: qca: Fix wrong soc_type returned for tool btattach

 drivers/bluetooth/btqca.h     |  1 +
 drivers/bluetooth/hci_ldisc.c | 10 ++++++++++
 drivers/bluetooth/hci_qca.c   |  8 +++++++-
 drivers/bluetooth/hci_uart.h  |  3 +++
 4 files changed, 21 insertions(+), 1 deletion(-)

Comments

Wren Turkal April 12, 2024, 8:10 p.m. UTC | #1
On 4/12/24 9:26 AM, Zijun Hu wrote:
> Tool btattach currently only supports QCA default soc type
> QCA_ROME, this change adds support for all other QCA soc types
> by adding a option to specify soc type.
> ---
>   tools/btattach.c   | 29 ++++++++++++++++++++++++-----
>   tools/btattach.rst |  2 ++
>   tools/hciattach.h  |  2 ++
>   3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/btattach.c b/tools/btattach.c
> index 4ce1be78d69c..024b0c7a289c 100644
> --- a/tools/btattach.c
> +++ b/tools/btattach.c
> @@ -97,7 +97,8 @@ static void local_version_callback(const void *data, uint8_t size,
>   }
>   
>   static int attach_proto(const char *path, unsigned int proto,
> -			unsigned int speed, bool flowctl, unsigned int flags)
> +			unsigned int speed, bool flowctl, unsigned int flags,
> +			unsigned long soc_type)
>   {
>   	int fd, dev_id;
>   
> @@ -111,6 +112,16 @@ static int attach_proto(const char *path, unsigned int proto,
>   		return -1;
>   	}
>   
> +	if ((proto == HCI_UART_QCA) && (soc_type > 0)) {
> +		if (ioctl(fd, HCIUARTSETPROTODATA, soc_type) < 0) {
> +			fprintf(stderr,
> +				"Failed to set soc_type(%lu) for protocol qca\n",
> +				soc_type);
> +			close(fd);
> +			return -1;
> +		}
> +	}
> +
>   	if (ioctl(fd, HCIUARTSETPROTO, proto) < 0) {
>   		perror("Failed to set protocol");
>   		close(fd);
> @@ -181,6 +192,7 @@ static void usage(void)
>   		"\t-A, --amp <device>     Attach AMP controller\n"
>   		"\t-P, --protocol <proto> Specify protocol type\n"
>   		"\t-S, --speed <baudrate> Specify which baudrate to use\n"
> +		"\t-T, --type <soc_type>  Specify soc_type for protocol qca\n"
>   		"\t-N, --noflowctl        Disable flow control\n"
>   		"\t-h, --help             Show help options\n");
>   }
> @@ -190,6 +202,7 @@ static const struct option main_options[] = {
>   	{ "amp",      required_argument, NULL, 'A' },
>   	{ "protocol", required_argument, NULL, 'P' },
>   	{ "speed",    required_argument, NULL, 'S' },
> +	{ "type",     required_argument, NULL, 'T' },

I am guessing this means that there is no way to determine the soc from 
the kernel without the assist of the IOCTL? I also see this is a 
required parm. Is this not something that can use something like a 
devicetree for discovery so that the type of soc can be a property of 
the system instead of being manually specified?

>   	{ "noflowctl",no_argument,       NULL, 'N' },
>   	{ "version",  no_argument,       NULL, 'v' },
>   	{ "help",     no_argument,       NULL, 'h' },
> @@ -221,12 +234,13 @@ int main(int argc, char *argv[])
>   	bool flowctl = true, raw_device = false;
>   	int exit_status, count = 0, proto_id = HCI_UART_H4;
>   	unsigned int speed = B115200;
> +	unsigned long soc_type = 0;
>   
>   	for (;;) {
>   		int opt;
>   
> -		opt = getopt_long(argc, argv, "B:A:P:S:NRvh",
> -						main_options, NULL);
> +		opt = getopt_long(argc, argv, "B:A:P:S:T:NRvh",
> +				  main_options, NULL);
>   		if (opt < 0)
>   			break;
>   
> @@ -237,6 +251,9 @@ int main(int argc, char *argv[])
>   		case 'A':
>   			amp_path = optarg;
>   			break;
> +		case 'T':
> +			soc_type = strtoul(optarg, NULL, 0);
> +			break;
>   		case 'P':
>   			proto = optarg;
>   			break;
> @@ -298,7 +315,8 @@ int main(int argc, char *argv[])
>   		if (raw_device)
>   			flags = (1 << HCI_UART_RAW_DEVICE);
>   
> -		fd = attach_proto(bredr_path, proto_id, speed, flowctl, flags);
> +		fd = attach_proto(bredr_path, proto_id, speed, flowctl, flags,
> +				  soc_type);
>   		if (fd >= 0) {
>   			mainloop_add_fd(fd, 0, uart_callback, NULL, NULL);
>   			count++;
> @@ -317,7 +335,8 @@ int main(int argc, char *argv[])
>   		if (raw_device)
>   			flags = (1 << HCI_UART_RAW_DEVICE);
>   
> -		fd = attach_proto(amp_path, proto_id, speed, flowctl, flags);
> +		fd = attach_proto(amp_path, proto_id, speed, flowctl, flags,
> +				  soc_type);
>   		if (fd >= 0) {
>   			mainloop_add_fd(fd, 0, uart_callback, NULL, NULL);
>   			count++;
> diff --git a/tools/btattach.rst b/tools/btattach.rst
> index 787d5c49e3bb..4aad3b915641 100644
> --- a/tools/btattach.rst
> +++ b/tools/btattach.rst
> @@ -62,6 +62,8 @@ OPTIONS
>   
>   -S baudrate, --speed baudrate       Specify wich baudrate to use
>   
> +-T soc_type, --type soc_type        Specify soc_type for protocol qca
> +
>   -N, --noflowctl            Disable flow control
>   
>   -v, --version              Show version
> diff --git a/tools/hciattach.h b/tools/hciattach.h
> index dfa4c1e7abe7..998a2a9a8460 100644
> --- a/tools/hciattach.h
> +++ b/tools/hciattach.h
> @@ -19,6 +19,8 @@
>   #define HCIUARTGETDEVICE	_IOR('U', 202, int)
>   #define HCIUARTSETFLAGS		_IOW('U', 203, int)
>   #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> +#define HCIUARTSETPROTODATA    _IOW('U', 205, unsigned long)
> +
>   
>   #define HCI_UART_H4	0
>   #define HCI_UART_BCSP	1
Zijun Hu April 13, 2024, 2:44 a.m. UTC | #2
On 4/13/2024 4:10 AM, Wren Turkal wrote:
> On 4/12/24 9:26 AM, Zijun Hu wrote:
>> Tool btattach currently only supports QCA default soc type
>> QCA_ROME, this change adds support for all other QCA soc types
>> by adding a option to specify soc type.
>> ---
>>   tools/btattach.c   | 29 ++++++++++++++++++++++++-----
>>   tools/btattach.rst |  2 ++
>>   tools/hciattach.h  |  2 ++
>>   3 files changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/btattach.c b/tools/btattach.c
>> index 4ce1be78d69c..024b0c7a289c 100644
>> --- a/tools/btattach.c
>> +++ b/tools/btattach.c
>> @@ -97,7 +97,8 @@ static void local_version_callback(const void *data, uint8_t size,
>>   }
>>     static int attach_proto(const char *path, unsigned int proto,
>> -            unsigned int speed, bool flowctl, unsigned int flags)
>> +            unsigned int speed, bool flowctl, unsigned int flags,
>> +            unsigned long soc_type)
>>   {
>>       int fd, dev_id;
>>   @@ -111,6 +112,16 @@ static int attach_proto(const char *path, unsigned int proto,
>>           return -1;
>>       }
>>   +    if ((proto == HCI_UART_QCA) && (soc_type > 0)) {
>> +        if (ioctl(fd, HCIUARTSETPROTODATA, soc_type) < 0) {
>> +            fprintf(stderr,
>> +                "Failed to set soc_type(%lu) for protocol qca\n",
>> +                soc_type);
>> +            close(fd);
>> +            return -1;
>> +        }
>> +    }
>> +
>>       if (ioctl(fd, HCIUARTSETPROTO, proto) < 0) {
>>           perror("Failed to set protocol");
>>           close(fd);
>> @@ -181,6 +192,7 @@ static void usage(void)
>>           "\t-A, --amp <device>     Attach AMP controller\n"
>>           "\t-P, --protocol <proto> Specify protocol type\n"
>>           "\t-S, --speed <baudrate> Specify which baudrate to use\n"
>> +        "\t-T, --type <soc_type>  Specify soc_type for protocol qca\n"
>>           "\t-N, --noflowctl        Disable flow control\n"
>>           "\t-h, --help             Show help options\n");
>>   }
>> @@ -190,6 +202,7 @@ static const struct option main_options[] = {
>>       { "amp",      required_argument, NULL, 'A' },
>>       { "protocol", required_argument, NULL, 'P' },
>>       { "speed",    required_argument, NULL, 'S' },
>> +    { "type",     required_argument, NULL, 'T' },
> 
> I am guessing this means that there is no way to determine the soc from the kernel without the assist of the IOCTL? I also see this is a required parm. Is this not something that can use something like a devicetree for discovery so that the type of soc can be a property of the system instead of being manually specified?
> 
yes for tool btattch scenario. tool btattch is mainly used before the final board configuration phase(change DT|APCI to enabel BT), so it can't get such soc type info from board configuration.
for tool btattach, it doesn't need to touch any system configuration and is mainly used for variant evaluation tests before the final product implementation phase

let me take below process to explain its usage scenario.
Customer often buys a BT module from a vendor for BT evaluation, the BT module have BT chip embedded and are externally powered, the module also has a BT UART converted mini usb port,
they connects the BT module to generic ubntu PC by a USB cable, then they run tool btattach at the machine to enable BT and perform variants BT tests, when the evaluation results is expected,
they maybe buy the embedded chip and integrated into there target product's PCB, then change and compile DT to enable BT formally.

thanks 
>>       { "noflowctl",no_argument,       NULL, 'N' },
>>       { "version",  no_argument,       NULL, 'v' },
>>       { "help",     no_argument,       NULL, 'h' },
>> @@ -221,12 +234,13 @@ int main(int argc, char *argv[])
>>       bool flowctl = true, raw_device = false;
>>       int exit_status, count = 0, proto_id = HCI_UART_H4;
>>       unsigned int speed = B115200;
>> +    unsigned long soc_type = 0;
>>         for (;;) {
>>           int opt;
>>   -        opt = getopt_long(argc, argv, "B:A:P:S:NRvh",
>> -                        main_options, NULL);
>> +        opt = getopt_long(argc, argv, "B:A:P:S:T:NRvh",
>> +                  main_options, NULL);
>>           if (opt < 0)
>>               break;
>>   @@ -237,6 +251,9 @@ int main(int argc, char *argv[])
>>           case 'A':
>>               amp_path = optarg;
>>               break;
>> +        case 'T':
>> +            soc_type = strtoul(optarg, NULL, 0);
>> +            break;
>>           case 'P':
>>               proto = optarg;
>>               break;
>> @@ -298,7 +315,8 @@ int main(int argc, char *argv[])
>>           if (raw_device)
>>               flags = (1 << HCI_UART_RAW_DEVICE);
>>   -        fd = attach_proto(bredr_path, proto_id, speed, flowctl, flags);
>> +        fd = attach_proto(bredr_path, proto_id, speed, flowctl, flags,
>> +                  soc_type);
>>           if (fd >= 0) {
>>               mainloop_add_fd(fd, 0, uart_callback, NULL, NULL);
>>               count++;
>> @@ -317,7 +335,8 @@ int main(int argc, char *argv[])
>>           if (raw_device)
>>               flags = (1 << HCI_UART_RAW_DEVICE);
>>   -        fd = attach_proto(amp_path, proto_id, speed, flowctl, flags);
>> +        fd = attach_proto(amp_path, proto_id, speed, flowctl, flags,
>> +                  soc_type);
>>           if (fd >= 0) {
>>               mainloop_add_fd(fd, 0, uart_callback, NULL, NULL);
>>               count++;
>> diff --git a/tools/btattach.rst b/tools/btattach.rst
>> index 787d5c49e3bb..4aad3b915641 100644
>> --- a/tools/btattach.rst
>> +++ b/tools/btattach.rst
>> @@ -62,6 +62,8 @@ OPTIONS
>>     -S baudrate, --speed baudrate       Specify wich baudrate to use
>>   +-T soc_type, --type soc_type        Specify soc_type for protocol qca
>> +
>>   -N, --noflowctl            Disable flow control
>>     -v, --version              Show version
>> diff --git a/tools/hciattach.h b/tools/hciattach.h
>> index dfa4c1e7abe7..998a2a9a8460 100644
>> --- a/tools/hciattach.h
>> +++ b/tools/hciattach.h
>> @@ -19,6 +19,8 @@
>>   #define HCIUARTGETDEVICE    _IOR('U', 202, int)
>>   #define HCIUARTSETFLAGS        _IOW('U', 203, int)
>>   #define HCIUARTGETFLAGS        _IOR('U', 204, int)
>> +#define HCIUARTSETPROTODATA    _IOW('U', 205, unsigned long)
>> +
>>     #define HCI_UART_H4    0
>>   #define HCI_UART_BCSP    1
>
Wren Turkal April 13, 2024, 3:12 a.m. UTC | #3
On 4/12/24 7:44 PM, quic_zijuhu wrote:
> On 4/13/2024 4:10 AM, Wren Turkal wrote:
>> On 4/12/24 9:26 AM, Zijun Hu wrote:
>>> Tool btattach currently only supports QCA default soc type
>>> QCA_ROME, this change adds support for all other QCA soc types
>>> by adding a option to specify soc type.
>>> ---
>>>    tools/btattach.c   | 29 ++++++++++++++++++++++++-----
>>>    tools/btattach.rst |  2 ++
>>>    tools/hciattach.h  |  2 ++
>>>    3 files changed, 28 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/btattach.c b/tools/btattach.c
>>> index 4ce1be78d69c..024b0c7a289c 100644
>>> --- a/tools/btattach.c
>>> +++ b/tools/btattach.c
>>> @@ -97,7 +97,8 @@ static void local_version_callback(const void *data, uint8_t size,
>>>    }
>>>      static int attach_proto(const char *path, unsigned int proto,
>>> -            unsigned int speed, bool flowctl, unsigned int flags)
>>> +            unsigned int speed, bool flowctl, unsigned int flags,
>>> +            unsigned long soc_type)
>>>    {
>>>        int fd, dev_id;
>>>    @@ -111,6 +112,16 @@ static int attach_proto(const char *path, unsigned int proto,
>>>            return -1;
>>>        }
>>>    +    if ((proto == HCI_UART_QCA) && (soc_type > 0)) {
>>> +        if (ioctl(fd, HCIUARTSETPROTODATA, soc_type) < 0) {
>>> +            fprintf(stderr,
>>> +                "Failed to set soc_type(%lu) for protocol qca\n",
>>> +                soc_type);
>>> +            close(fd);
>>> +            return -1;
>>> +        }
>>> +    }
>>> +
>>>        if (ioctl(fd, HCIUARTSETPROTO, proto) < 0) {
>>>            perror("Failed to set protocol");
>>>            close(fd);
>>> @@ -181,6 +192,7 @@ static void usage(void)
>>>            "\t-A, --amp <device>     Attach AMP controller\n"
>>>            "\t-P, --protocol <proto> Specify protocol type\n"
>>>            "\t-S, --speed <baudrate> Specify which baudrate to use\n"
>>> +        "\t-T, --type <soc_type>  Specify soc_type for protocol qca\n"
>>>            "\t-N, --noflowctl        Disable flow control\n"
>>>            "\t-h, --help             Show help options\n");
>>>    }
>>> @@ -190,6 +202,7 @@ static const struct option main_options[] = {
>>>        { "amp",      required_argument, NULL, 'A' },
>>>        { "protocol", required_argument, NULL, 'P' },
>>>        { "speed",    required_argument, NULL, 'S' },
>>> +    { "type",     required_argument, NULL, 'T' },
>>
>> I am guessing this means that there is no way to determine the soc from the kernel without the assist of the IOCTL? I also see this is a required parm. Is this not something that can use something like a devicetree for discovery so that the type of soc can be a property of the system instead of being manually specified?
>>
> yes for tool btattch scenario. tool btattch is mainly used before the final board configuration phase(change DT|APCI to enabel BT), so it can't get such soc type info from board configuration.
> for tool btattach, it doesn't need to touch any system configuration and is mainly used for variant evaluation tests before the final product implementation phase
> 
> let me take below process to explain its usage scenario.
> Customer often buys a BT module from a vendor for BT evaluation, the BT module have BT chip embedded and are externally powered, the module also has a BT UART converted mini usb port,
> they connects the BT module to generic ubntu PC by a USB cable, then they run tool btattach at the machine to enable BT and perform variants BT tests, when the evaluation results is expected,
> they maybe buy the embedded chip and integrated into there target product's PCB, then change and compile DT to enable BT formally.
Thanks for the explanation for a total newb. This is really cool to 
learn about. Really appreciate your taking the time to help me out.

> thanks
>>>        { "noflowctl",no_argument,       NULL, 'N' },
>>>        { "version",  no_argument,       NULL, 'v' },
>>>        { "help",     no_argument,       NULL, 'h' },
>>> @@ -221,12 +234,13 @@ int main(int argc, char *argv[])
>>>        bool flowctl = true, raw_device = false;
>>>        int exit_status, count = 0, proto_id = HCI_UART_H4;
>>>        unsigned int speed = B115200;
>>> +    unsigned long soc_type = 0;
>>>          for (;;) {
>>>            int opt;
>>>    -        opt = getopt_long(argc, argv, "B:A:P:S:NRvh",
>>> -                        main_options, NULL);
>>> +        opt = getopt_long(argc, argv, "B:A:P:S:T:NRvh",
>>> +                  main_options, NULL);
>>>            if (opt < 0)
>>>                break;
>>>    @@ -237,6 +251,9 @@ int main(int argc, char *argv[])
>>>            case 'A':
>>>                amp_path = optarg;
>>>                break;
>>> +        case 'T':
>>> +            soc_type = strtoul(optarg, NULL, 0);
>>> +            break;
>>>            case 'P':
>>>                proto = optarg;
>>>                break;
>>> @@ -298,7 +315,8 @@ int main(int argc, char *argv[])
>>>            if (raw_device)
>>>                flags = (1 << HCI_UART_RAW_DEVICE);
>>>    -        fd = attach_proto(bredr_path, proto_id, speed, flowctl, flags);
>>> +        fd = attach_proto(bredr_path, proto_id, speed, flowctl, flags,
>>> +                  soc_type);
>>>            if (fd >= 0) {
>>>                mainloop_add_fd(fd, 0, uart_callback, NULL, NULL);
>>>                count++;
>>> @@ -317,7 +335,8 @@ int main(int argc, char *argv[])
>>>            if (raw_device)
>>>                flags = (1 << HCI_UART_RAW_DEVICE);
>>>    -        fd = attach_proto(amp_path, proto_id, speed, flowctl, flags);
>>> +        fd = attach_proto(amp_path, proto_id, speed, flowctl, flags,
>>> +                  soc_type);
>>>            if (fd >= 0) {
>>>                mainloop_add_fd(fd, 0, uart_callback, NULL, NULL);
>>>                count++;
>>> diff --git a/tools/btattach.rst b/tools/btattach.rst
>>> index 787d5c49e3bb..4aad3b915641 100644
>>> --- a/tools/btattach.rst
>>> +++ b/tools/btattach.rst
>>> @@ -62,6 +62,8 @@ OPTIONS
>>>      -S baudrate, --speed baudrate       Specify wich baudrate to use
>>>    +-T soc_type, --type soc_type        Specify soc_type for protocol qca
>>> +
>>>    -N, --noflowctl            Disable flow control
>>>      -v, --version              Show version
>>> diff --git a/tools/hciattach.h b/tools/hciattach.h
>>> index dfa4c1e7abe7..998a2a9a8460 100644
>>> --- a/tools/hciattach.h
>>> +++ b/tools/hciattach.h
>>> @@ -19,6 +19,8 @@
>>>    #define HCIUARTGETDEVICE    _IOR('U', 202, int)
>>>    #define HCIUARTSETFLAGS        _IOW('U', 203, int)
>>>    #define HCIUARTGETFLAGS        _IOR('U', 204, int)
>>> +#define HCIUARTSETPROTODATA    _IOW('U', 205, unsigned long)
>>> +
>>>      #define HCI_UART_H4    0
>>>    #define HCI_UART_BCSP    1
>>
>
Luiz Augusto von Dentz April 17, 2024, 9:39 p.m. UTC | #4
Hi Zijun,

On Wed, Apr 17, 2024 at 8:53 AM Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>
> qca_soc_type() returns wrong soc type QCA_ROME when use tool btattach
> for any other soc type such as QCA_QCA2066, and wrong soc type will
> cause later initialization failure, fixed by using user specified
> soc type for hci_uart derived from tool btattach.

Then we have to fix qca_soc_type or explain what is going on that it
can't detect the soc_type if initialized via btattach?

>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/bluetooth/btqca.h   | 1 +
>  drivers/bluetooth/hci_qca.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index dc31984f71dc..a148d4c4e1bd 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -153,6 +153,7 @@ enum qca_btsoc_type {
>         QCA_WCN6750,
>         QCA_WCN6855,
>         QCA_WCN7850,
> +       QCA_MAX,
>  };
>
>  #if IS_ENABLED(CONFIG_BT_QCA)
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index c04b97332bca..7c3577a4887c 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -238,12 +238,17 @@ static void qca_dmp_hdr(struct hci_dev *hdev, struct sk_buff *skb);
>
>  static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
>  {
> +       /* For Non-serdev device, hu->proto_data records soc type
> +        * set by ioctl HCIUARTSETPROTODATA.
> +        */
> +       int proto_data = (int)hu->proto_data;
>         enum qca_btsoc_type soc_type;
>
>         if (hu->serdev) {
>                 struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
> -
>                 soc_type = qsd->btsoc_type;
> +       } else if ((proto_data > 0) && (proto_data < QCA_MAX)) {
> +               soc_type = (enum qca_btsoc_type)proto_data;

Like I said a vendor specific ioctl will not gonna fly with me,
specially since each vendor may need a different size to describe
their controller version, etc,

>         } else {
>                 soc_type = QCA_ROME;
>         }
> @@ -2281,6 +2286,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>                 return -ENOMEM;
>
>         qcadev->serdev_hu.serdev = serdev;
> +       qcadev->serdev_hu.proto_data = 0;
>         data = device_get_match_data(&serdev->dev);
>         serdev_device_set_drvdata(serdev, qcadev);
>         device_property_read_string(&serdev->dev, "firmware-name",
> --
> 2.7.4
>
Zijun Hu April 18, 2024, 1:26 a.m. UTC | #5
On 4/18/2024 5:39 AM, Luiz Augusto von Dentz wrote:
> Hi Zijun,
> 
> On Wed, Apr 17, 2024 at 8:53 AM Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>>
>> qca_soc_type() returns wrong soc type QCA_ROME when use tool btattach
>> for any other soc type such as QCA_QCA2066, and wrong soc type will
>> cause later initialization failure, fixed by using user specified
>> soc type for hci_uart derived from tool btattach.
> 
> Then we have to fix qca_soc_type or explain what is going on that it
> can't detect the soc_type if initialized via btattach?
> 
perhaps, my commit message is not precise and cause some mistook.

for tool btattach, only default QCA_ROME is used, there are no way to 
get right soc type for other BT controllers. so i introduce a ioctl to let user specify
soc type info used. so i fix qca_soc_type() to use user specified soc type for tool btattach
case.

1) different soc types have different responses for VSC which is used to detect soc type
as shown by. so soc_type is can't be detected and it  is needed by config by DT|ACPI or user specified.
int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
			 enum qca_btsoc_type soc_type)

2) soc type is a critical info, and it is used everywhere by hci_qca driver, it is also used to
decide which BT firmware to download as shown qca_uart_setup(), it soc type is not right. it will download
error BT firmware and cause serious results.

i will correct commit message for this patch.

>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/bluetooth/btqca.h   | 1 +
>>  drivers/bluetooth/hci_qca.c | 8 +++++++-
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index dc31984f71dc..a148d4c4e1bd 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -153,6 +153,7 @@ enum qca_btsoc_type {
>>         QCA_WCN6750,
>>         QCA_WCN6855,
>>         QCA_WCN7850,
>> +       QCA_MAX,
>>  };
>>
>>  #if IS_ENABLED(CONFIG_BT_QCA)
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index c04b97332bca..7c3577a4887c 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -238,12 +238,17 @@ static void qca_dmp_hdr(struct hci_dev *hdev, struct sk_buff *skb);
>>
>>  static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
>>  {
>> +       /* For Non-serdev device, hu->proto_data records soc type
>> +        * set by ioctl HCIUARTSETPROTODATA.
>> +        */
>> +       int proto_data = (int)hu->proto_data;
>>         enum qca_btsoc_type soc_type;
>>
>>         if (hu->serdev) {
>>                 struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
>> -
>>                 soc_type = qsd->btsoc_type;
>> +       } else if ((proto_data > 0) && (proto_data < QCA_MAX)) {
>> +               soc_type = (enum qca_btsoc_type)proto_data;
> 
> Like I said a vendor specific ioctl will not gonna fly with me,
> specially since each vendor may need a different size to describe
> their controller version, etc,
> 
i have comments about this part of this question in reply for  [PATCH v2 3/4]

hci_uart->proto_data is a protocol specified unsigned long data, it is parsed
by specific protocol, for protocol, it is parsed as soc type. so force cast to 
(enum qca_btsoc_type).

hci_uart->proto_data is mostly similar as @data of struct struct of_device_id defined by
below header file. it is assigned with misc data type and explained by specific device driver.
include/linux/mod_devicetable.h:
struct of_device_id {
	char	name[32];
	char	type[32];
	char	compatible[128];
	const void *data;
};

  
>>         } else {
>>                 soc_type = QCA_ROME;
>>         }
>> @@ -2281,6 +2286,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>                 return -ENOMEM;
>>
>>         qcadev->serdev_hu.serdev = serdev;
>> +       qcadev->serdev_hu.proto_data = 0;
>>         data = device_get_match_data(&serdev->dev);
>>         serdev_device_set_drvdata(serdev, qcadev);
>>         device_property_read_string(&serdev->dev, "firmware-name",
>> --
>> 2.7.4
>>
> 
>