linux-kernel - Re: [PATCH v5] Bluetooth: Add hci_nxp to hci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <AM9PR04MB86031859F17AE8D5B563D103E7F79@AM9PR04MB8603.eurprd04.prod.outlook.com>
Date:   Mon, 2 Jan 2023 14:08:11 +0000
From:   Neeraj sanjay kale <neeraj.sanjaykale@....com>
To:     Marcel Holtmann <marcel@...tmann.org>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>
CC:     Johan Hedberg <johan.hedberg@...il.com>,
        Paul Menzel <pmenzel@...gen.mpg.de>,
        Amitkumar Karwar <amitkumar.karwar@....com>,
        Rohit Fule <rohit.fule@....com>,
        Sherry Sun <sherry.sun@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>
Subject: Re: [PATCH v5] Bluetooth: Add hci_nxp to hci_uart module to support
 NXP BT chipsets

Hi Marcel,

> From: Marcel Holtmann <marcel@...tmann.org>
> To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
> Cc: Neeraj sanjay kale <neeraj.sanjaykale@....com>; Johan Hedberg
> <johan.hedberg@...il.com>; Paul Menzel <pmenzel@...gen.mpg.de>;
> Amitkumar Karwar <amitkumar.karwar@....com>; Rohit Fule
> <rohit.fule@....com>; Sherry Sun <sherry.sun@....com>; LKML <linux-
> kernel@...r.kernel.org>; linux-bluetooth@...r.kernel.org
> 
> Hi Luiz,
> 
> >>> Add hci_nxp to the hci_uart module which adds support for the NXP BT
> >>> chips. This driver has Power Save feature that will put the NXP
> >>> bluetooth chip into sleep state, whenever there is no activity for
> >>> certain duration of time (2000ms), and will be woken up when any
> >>> activity is to be initiated.
> >>>
> >>> The Power Save feature can be configured with the following set of
> >>> commands (optional):
> >>> hcitool -i hci0 cmd 3F 23 02 00 00    (enable Power Save)
> >>> hcitool -i hci0 cmd 3F 23 03 00 00    (disable Power Save)
> >>> where,
> >>> OGF = 0x3F (vendor specific command) OCF = 0x23 (command to set
> >>> Power Save state) arg[0] = 0x02 (disable Power Save) arg[0] = 0x03
> >>> (enable Power Save) arg[1,2,...] = XX (don't care)
> >>>
> >>> The sleep/wake-up source can be configured with the following set of
> >>> commands (optional):
> >>> hcitool -i hci0 cmd 3F 53 03 14 01 FF    (set UART break method)
> >>> hcitool -i hci0 cmd 3F 53 03 14 00 FF    (set UART DSR method)
> >>> where,
> >>> OGF = 0x3F (vendor specific command) OCF = 0x53 (command to set
> >>> sleep and wake-up source) arg[0] = 0x00 (Chip to host method NONE)
> >>> arg[0] = 0x01 (Chip to host method UART DTR) arg[0] = 0x02 (Chip to
> >>> host method UART BREAK) arg[0] = 0x03 (Chip to host method GPIO)
> >>> arg[1] = 0x14 (Chip to host GPIO[20] if arg[0] is 0x03, else 0xFF)
> >>> arg[2] = 0x00 (Host to chip method UART DSR) arg[2] = 0x01 (Host to
> >>> chip method UART BREAK) arg[3] = 0xXX (Reserved for future use)
> >>>
> >>> By default, the hci_nxp sets power save enable, chip to host wake-up
> >>> source as GPIO and host to chip sleep and wake-up source as UART
> >>> break during driver initialization, by sending the respective
> >>> commands to the chip.
> >>>
> >>> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
> >>> ---
> >>> v2: Changed the subject/summary lines and added more details in the
> >>> description. (Paul Menzel)
> >>> v3: Made internal functions static, optimized the code, added few
> >>> comments. (Sherry Sun)
> >>> v4: Reworked entire code to send vendor commands cmd23 and cmd53
> by
> >>> using __hci_cmd_sync. (Luiz Augusto von Dentz)
> >>> v5: Used hci_command_hdr and combined OGF+OCF into a single
> opcode.
> >>> (Luiz Augusto von Dentz)
> >>> ---
> >>> MAINTAINERS                   |   6 +
> >>> drivers/bluetooth/Kconfig     |  10 +
> >>> drivers/bluetooth/Makefile    |   1 +
> >>> drivers/bluetooth/hci_ldisc.c |   6 +
> >>> drivers/bluetooth/hci_nxp.c   | 592
> ++++++++++++++++++++++++++++++++++
> >>> drivers/bluetooth/hci_nxp.h   |  94 ++++++
> >>> drivers/bluetooth/hci_uart.h  |   8 +-
> >>> 7 files changed, 716 insertions(+), 1 deletion(-) create mode 100644
> >>> drivers/bluetooth/hci_nxp.c create mode 100644
> >>> drivers/bluetooth/hci_nxp.h
> >>
> >> so this is a clear NAK. Add this as serdev driver and not hook
> >> further into the mess that is the HCI line discipline.
> >
> > I wonder if we should make it more clear somehow, perhaps include a
> > text on the likes of BT_HCIUART that is deprecated and new drivers
> > shall use BT_HCIUART_SERDEV instead.
> 
> not even that. They need to be separate drivers. A long time ago I posted the
> skeleton for btuart.ko and bt3wire.ko and that is where this has to go.
> 
> Regards
> 
> Marcel

Thanks for your comment. 
Based on your comment, I re-worked the entire driver with reference to following patches:
https://www.spinics.net/lists/linux-bluetooth/msg74918.html
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/bluetooth/btmtkuart.c

I am able to create a standalone btnxp.ko which is able to run basic BT operations along with FW download with NXP chipsets.

However I have now hit a blocker. The NXP chipsets require support for break signal, by which the host can put the chip into sleep, and wake it up.

However, it seems that the serdev API's in https://elixir.bootlin.com/linux/v6.1-rc8/source/include/linux/serdev.h do not support break assertion over serial TX line.

Is there any plan for serdev to support break signaling?

Any help on this blocker would be highly appreciated.

Thanks,
Neeraj