Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3544-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ce11e5ce-13ec-4e40-8083-7319f72beb20@penguintechs.org>
Date: Fri, 12 Apr 2024 20:12:28 -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: quic_zijuhu <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>
 <678721b5-4636-4268-836e-98c7a8ce36ba@penguintechs.org>
 <5bcf5034-fea4-43c6-ac7d-db6f24b88512@quicinc.com>
From: Wren Turkal <wt@penguintechs.org>
In-Reply-To: <5bcf5034-fea4-43c6-ac7d-db6f24b88512@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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
>>
> 

-- 
You're more amazing than you think!