Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3540-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <678721b5-4636-4268-836e-98c7a8ce36ba@penguintechs.org>
Date: Fri, 12 Apr 2024 13:10:49 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH BlueZ v1] tools/btattach: Add support for more QCA soc
 types
Content-Language: en-US
To: Zijun Hu <quic_zijuhu@quicinc.com>, luiz.dentz@gmail.com,
 marcel@holtmann.org, jiangzp@google.com
Cc: linux-bluetooth@vger.kernel.org
References: <1710914907-30012-1-git-send-email-quic_zijuhu@quicinc.com>
 <1712939188-25529-1-git-send-email-quic_zijuhu@quicinc.com>
 <1712939188-25529-5-git-send-email-quic_zijuhu@quicinc.com>
From: Wren Turkal <wt@penguintechs.org>
In-Reply-To: <1712939188-25529-5-git-send-email-quic_zijuhu@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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

-- 
You're more amazing than you think!