From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-13 03:15:02
Hi,
This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
N9xx devices. It has been tested on the Nokia N950, where it works
correctly. On Nokia N900 it currently fails during negotiation
(probably related to slightly incorrect serial settings/timings).
The N900's bcm2048 correctly answeres to alive check even before
negotiation (on N950 it does not work before negotiation), but replies
with an Hardware error event to the negotiation packet.
Apart from N900 support there are still two "features" missing in the
driver:
1. To save energy the bluetooth module can be put into sleep mode via a
GPIO. This gpio should be enabled before sending data via UART and
disabled once the transmission is done. I currently just keep the
GPIO always enabled.
2. It would be nice to have a bluetooth device exposed by the kernel
automatically without having to setup the tty disector first for
proper configurationless out of the box support. I could not find
a nice way to do this from the kernel, though.
On N950 the driver works with omap-serial and omap8250-serial drivers.
You can also find this series in the following branch:
https://git.kernel.org/cgit/linux/kernel/git/sre/linux-n900.git/log/?h=nokia-bluetooth
-- Sebastian
NeilBrown (1):
tty: add support for "tty slave" devices
Sebastian Reichel (6):
tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init
dt: bindings: Add nokia-bluetooth
Bluetooth: hci_uart: Add support for word alignment
Bluetooth: hci_nokia: Introduce new driver
ARM: dts: OMAP3-N900: Add bluetooth
ARM: dts: OMAP3-N950: Add bluetooth
.../devicetree/bindings/net/nokia-bluetooth.txt | 43 ++
Documentation/devicetree/bindings/serial/8250.txt | 4 +
arch/arm/boot/dts/omap3-n900.dts | 22 +-
arch/arm/boot/dts/omap3-n950-n9.dtsi | 34 +
drivers/bluetooth/Kconfig | 10 +
drivers/bluetooth/Makefile | 1 +
drivers/bluetooth/hci_h4.c | 10 +
drivers/bluetooth/hci_ldisc.c | 6 +
drivers/bluetooth/hci_nokia.c | 734 +++++++++++++++++++++
drivers/bluetooth/hci_nokia.h | 140 ++++
drivers/bluetooth/hci_uart.h | 9 +-
drivers/tty/serial/omap-serial.c | 3 +
drivers/tty/tty_io.c | 6 +
13 files changed, 1020 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt
create mode 100644 drivers/bluetooth/hci_nokia.c
create mode 100644 drivers/bluetooth/hci_nokia.h
--
2.8.1
From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-16 08:14:54
Hi,
On Sun, Aug 14, 2016 at 10:49:56AM +0200, Pavel Machek wrote:
It would be nice to have a line about why it is needed,
unfortunately include/linux/serial_core.h is not exactly helpful.
Plus you'll need to sign off the patch.
I should have had another look at the long patch
descriptions before sending the patchset ;)
-- Sebastian
From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-13 03:15:10
From: NeilBrown <redacted>
A "tty slave" is a device connected via UART. It may need a driver to,
for example, power the device on when the tty is opened, and power it
off when the tty is released.
Signed-off-by: NeilBrown <redacted>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
Documentation/devicetree/bindings/serial/8250.txt | 4 ++++
drivers/tty/tty_io.c | 6 ++++++
2 files changed, 10 insertions(+)
@@ -46,6 +46,10 @@ Optional properties: line respectively. It will use specified GPIO instead of the peripheral function pin for the UART feature. If unsure, don't specify this property.+Optional child node:+- a device connected to the uart can be specified as child node with+ compatible value.+ Note: * fsl,ns16550: ------------
@@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,retval=device_register(dev);if(retval)gotoerror;+if(device&&device->of_node)+/* Children are platform devices and will be+*runtime_pmmanagedbythistty.+*/+of_platform_populate(device->of_node,NULL,NULL,dev);returndev;
On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
quoted hunk
From: NeilBrown <redacted>
A "tty slave" is a device connected via UART. It may need a driver to,
for example, power the device on when the tty is opened, and power it
off when the tty is released.
Signed-off-by: NeilBrown <redacted>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
Documentation/devicetree/bindings/serial/8250.txt | 4 ++++
drivers/tty/tty_io.c | 6 ++++++
2 files changed, 10 insertions(+)
@@ -46,6 +46,10 @@ Optional properties: line respectively. It will use specified GPIO instead of the peripheral function pin for the UART feature. If unsure, don't specify this property.+Optional child node:+- a device connected to the uart can be specified as child node with+ compatible value.+ Note: * fsl,ns16550: ------------
@@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,retval=device_register(dev);if(retval)gotoerror;+if(device&&device->of_node)+/* Children are platform devices and will be+*runtime_pmmanagedbythistty.+*/+of_platform_populate(device->of_node,NULL,NULL,dev);
Why are these platform devices? And why only OF?
thanks,
greg k-h
From: Pavel Machek <hidden> Date: 2016-08-14 08:48:30
On Sat 2016-08-13 12:03:45, Greg Kroah-Hartman wrote:
On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
quoted
From: NeilBrown <redacted>
A "tty slave" is a device connected via UART. It may need a driver to,
for example, power the device on when the tty is opened, and power it
off when the tty is released.
Signed-off-by: NeilBrown <redacted>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
Acked-by: Pavel Machek <redacted>
quoted
@@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver, retval = device_register(dev); if (retval) goto error;+ if (device && device->of_node)+ /* Children are platform devices and will be+ * runtime_pm managed by this tty.+ */+ of_platform_populate(device->of_node, NULL, NULL, dev);
Why are these platform devices? And why only OF?
OF based systems are the only ones that have this problem, so that's
the only place where we can test this solution.
Given that these devices are connected over the UART, it seems right
to categorize them as platform devices... You can't connect PCI, SATA
or USB device over UART port.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun, Aug 14, 2016 at 10:48:22AM +0200, Pavel Machek wrote:
On Sat 2016-08-13 12:03:45, Greg Kroah-Hartman wrote:
quoted
On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
quoted
From: NeilBrown <redacted>
A "tty slave" is a device connected via UART. It may need a driver to,
for example, power the device on when the tty is opened, and power it
off when the tty is released.
Signed-off-by: NeilBrown <redacted>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
Acked-by: Pavel Machek <redacted>
quoted
quoted
@@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver, retval = device_register(dev); if (retval) goto error;+ if (device && device->of_node)+ /* Children are platform devices and will be+ * runtime_pm managed by this tty.+ */+ of_platform_populate(device->of_node, NULL, NULL, dev);
Why are these platform devices? And why only OF?
OF based systems are the only ones that have this problem, so that's
the only place where we can test this solution.
Given that these devices are connected over the UART, it seems right
to categorize them as platform devices... You can't connect PCI, SATA
or USB device over UART port.
No, that's a total abuse of the platform bus, please don't.
I've said before that a "serial" bus should be created and you can hang
devices off of it. For some reason that message keeps getting
ignored...
thanks,
greg k-h
From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-14 09:31:15
Hi,
On Sat, Aug 13, 2016 at 12:03:45PM +0200, Greg Kroah-Hartman wrote:
On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
quoted
From: NeilBrown <redacted>
A "tty slave" is a device connected via UART. It may need a driver to,
for example, power the device on when the tty is opened, and power it
off when the tty is released.
Signed-off-by: NeilBrown <redacted>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
Documentation/devicetree/bindings/serial/8250.txt | 4 ++++
drivers/tty/tty_io.c | 6 ++++++
2 files changed, 10 insertions(+)
@@ -46,6 +46,10 @@ Optional properties: line respectively. It will use specified GPIO instead of the peripheral function pin for the UART feature. If unsure, don't specify this property.+Optional child node:+- a device connected to the uart can be specified as child node with+ compatible value.+ Note: * fsl,ns16550: ------------
@@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,retval=device_register(dev);if(retval)gotoerror;+if(device&&device->of_node)+/* Children are platform devices and will be+*runtime_pmmanagedbythistty.+*/+of_platform_populate(device->of_node,NULL,NULL,dev);
Why are these platform devices?
And why only OF?
I just took this patch over from Neil to get bluetooth working
on N900/N950. Both of them are DT only (well N900 still has
boardcode, but that will be removed shortly), so it was enough
for me.
I guess you have something in mind, that's similar to e.g. i2c
and spi with a serial_client device and support to instanciate
it from DT, ACPI and boardcode?
-- Sebastian
On Sat, Aug 13, 2016 at 10:31:24PM +0200, Sebastian Reichel wrote:
Hi,
On Sat, Aug 13, 2016 at 12:03:45PM +0200, Greg Kroah-Hartman wrote:
quoted
On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
quoted
From: NeilBrown <redacted>
A "tty slave" is a device connected via UART. It may need a driver to,
for example, power the device on when the tty is opened, and power it
off when the tty is released.
Signed-off-by: NeilBrown <redacted>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
Documentation/devicetree/bindings/serial/8250.txt | 4 ++++
drivers/tty/tty_io.c | 6 ++++++
2 files changed, 10 insertions(+)
@@ -46,6 +46,10 @@ Optional properties: line respectively. It will use specified GPIO instead of the peripheral function pin for the UART feature. If unsure, don't specify this property.+Optional child node:+- a device connected to the uart can be specified as child node with+ compatible value.+ Note: * fsl,ns16550: ------------
@@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,retval=device_register(dev);if(retval)gotoerror;+if(device&&device->of_node)+/* Children are platform devices and will be+*runtime_pmmanagedbythistty.+*/+of_platform_populate(device->of_node,NULL,NULL,dev);
Why are these platform devices?
And why only OF?
I just took this patch over from Neil to get bluetooth working
on N900/N950. Both of them are DT only (well N900 still has
boardcode, but that will be removed shortly), so it was enough
for me.
I guess you have something in mind, that's similar to e.g. i2c
and spi with a serial_client device and support to instanciate
it from DT, ACPI and boardcode?
I'm not going to accept a OF-only patch to the tty layer like this,
especially one that abuses platform drivers.
See my response to Pavel for what this "should" look like.
thanks,
greg k-h
@@ -0,0 +1,43 @@+Nokia bluetooth UART devices+------++Some vendors have custom versions of their chips, that can be found in Nokia+devices. These chips are controlled differently, than the non-Nokia version,+so a different binding is required. All chips listed here implement the Nokia+H4+ protocol.++Required properties:++ - compatible: should be one of the following:+ * "nokia,brcm,bcm2048"+ * "nokia,ti,wl1271-bluetooth"+ - reset-gpios: Should specify the gpio for bluetooth reset+ - host-wakeup-gpios: Should specify the gpio for host wakeup+ - bluetooth-wakeup-gpios: Should specify the gpio for bluetooth wakeup+ - clock-names: Should be "sysclk"+ - clocks: Should contain a clock phandle for system clock++Example:++/ {+ /* controlled (enabled/disabled) directly by wl1271 */+ vctcxo: vctcxo {+ compatible = "fixed-clock";+ #clock-cells = <0>;+ clock-frequency = <38400000>;+ };+};++&uart2 {+ bluetooth {+ compatible = "nokia,ti,wl1271-bluetooth";++ reset-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; /* 26 */+ host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */+ bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */++ clocks = <&vctcxo>;+ clock-names = "sysclk";+ };++};
@@ -0,0 +1,43 @@+Nokia bluetooth UART devices+------++Some vendors have custom versions of their chips, that can be found in Nokia+devices. These chips are controlled differently, than the non-Nokia version,+so a different binding is required. All chips listed here implement the Nokia+H4+ protocol.++Required properties:++ - compatible: should be one of the following:+ * "nokia,brcm,bcm2048"+ * "nokia,ti,wl1271-bluetooth"
Perhaps these should be 2 separate strings. Something like
'"nokia,n900-bt", "brcm,bcm2048"'. However, if they are in no way
compatible with the default version from the vendors, then just a single
string is fine, but it doesn't need to be aligned to the vendor
compatible string. So just "nokia,n900-bcm2048" or similar is fine.
+ - reset-gpios: Should specify the gpio for bluetooth reset
+ - host-wakeup-gpios: Should specify the gpio for host wakeup
Should be interrupt instead?
+ - bluetooth-wakeup-gpios: Should specify the gpio for bluetooth wakeup
State direction and active state for gpios.
+ - clock-names: Should be "sysclk"
+ - clocks: Should contain a clock phandle for system clock
+
+Example:
+
+/ {
+ /* controlled (enabled/disabled) directly by wl1271 */
+ vctcxo: vctcxo {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <38400000>;
+ };
+};
+
+&uart2 {
I want to see a common serial device binding doc before accepting any
device bindings. It's not going to say much initially other than devices
are child nodes of uarts. Perhaps something on baudrate settings.
@@ -0,0 +1,43 @@+Nokia bluetooth UART devices+------++Some vendors have custom versions of their chips, that can be found in Nokia+devices. These chips are controlled differently, than the non-Nokia version,+so a different binding is required. All chips listed here implement the Nokia+H4+ protocol.++Required properties:++ - compatible: should be one of the following:+ * "nokia,brcm,bcm2048"+ * "nokia,ti,wl1271-bluetooth"
Perhaps these should be 2 separate strings. Something like
'"nokia,n900-bt", "brcm,bcm2048"'. However, if they are in no way
compatible with the default version from the vendors, then just a single
string is fine, but it doesn't need to be aligned to the vendor
compatible string. So just "nokia,n900-bcm2048" or similar is fine.
The default bcm2048 variant uses different initialization process
and does not use word alignment as far as I know. I think having
"brcm,bcm2048" in the compatible string is wrong.
I guess "brcm,bcm2048-nokia" would also be an option, since the
chip has been built buy broadcom, but it has a custom Nokia
interface.
quoted
+ - reset-gpios: Should specify the gpio for bluetooth reset
+ - host-wakeup-gpios: Should specify the gpio for host wakeup
Should be interrupt instead?
Yes this is mostly an interrupt, but I need to read the current
line state.
quoted
+ - bluetooth-wakeup-gpios: Should specify the gpio for bluetooth wakeup
State direction and active state for gpios.
ok.
quoted
+ - clock-names: Should be "sysclk"
+ - clocks: Should contain a clock phandle for system clock
+
+Example:
+
+/ {
+ /* controlled (enabled/disabled) directly by wl1271 */
+ vctcxo: vctcxo {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <38400000>;
+ };
+};
+
+&uart2 {
I want to see a common serial device binding doc before accepting any
device bindings. It's not going to say much initially other than devices
are child nodes of uarts. Perhaps something on baudrate settings.
Neil added a short sentence about this in "[RFC 2/7] tty: add
support for "tty slave" devices". I just took the unmodified patch
from Neil (*), so it's not in its own patch.
@@ -0,0 +1,43 @@+Nokia bluetooth UART devices+------++Some vendors have custom versions of their chips, that can be found in Nokia+devices. These chips are controlled differently, than the non-Nokia version,+so a different binding is required. All chips listed here implement the Nokia+H4+ protocol.++Required properties:++ - compatible: should be one of the following:+ * "nokia,brcm,bcm2048"+ * "nokia,ti,wl1271-bluetooth"
Perhaps these should be 2 separate strings. Something like
'"nokia,n900-bt", "brcm,bcm2048"'. However, if they are in no way
compatible with the default version from the vendors, then just a single
string is fine, but it doesn't need to be aligned to the vendor
compatible string. So just "nokia,n900-bcm2048" or similar is fine.
The default bcm2048 variant uses different initialization process
and does not use word alignment as far as I know. I think having
"brcm,bcm2048" in the compatible string is wrong.
Okay.
I guess "brcm,bcm2048-nokia" would also be an option, since the
chip has been built buy broadcom, but it has a custom Nokia
interface.
That would be okay. Though, in theory Nokia could have different
products with bcm2048 all with different versions.
I was going with defining it as a board level compatible string. Even
if chips are the same, s/w can need to know board level differences so
using the board vendor and name are common.
quoted
quoted
+ - reset-gpios: Should specify the gpio for bluetooth reset
+ - host-wakeup-gpios: Should specify the gpio for host wakeup
Should be interrupt instead?
Yes this is mostly an interrupt, but I need to read the current
line state.
When? If the interrupt is level triggered, then you can get the line
state based on whether you get an interrupt or not. If this needs to
be a wakeup source (see the wakeup source binding), then it needs to
be an interrupt.
Reading the line state is a common problem. It would be nice if the
irq API provided a function to read the line state though that is not
always possible.
quoted
quoted
+ - bluetooth-wakeup-gpios: Should specify the gpio for bluetooth wakeup
State direction and active state for gpios.
ok.
quoted
quoted
+ - clock-names: Should be "sysclk"
+ - clocks: Should contain a clock phandle for system clock
+
+Example:
+
+/ {
+ /* controlled (enabled/disabled) directly by wl1271 */
+ vctcxo: vctcxo {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <38400000>;
+ };
+};
+
+&uart2 {
I want to see a common serial device binding doc before accepting any
device bindings. It's not going to say much initially other than devices
are child nodes of uarts. Perhaps something on baudrate settings.
Neil added a short sentence about this in "[RFC 2/7] tty: add
support for "tty slave" devices". I just took the unmodified patch
from Neil (*), so it's not in its own patch.
But that is in the 8250 binding. It needs to be a common binding.
Rob
From: Pavel Machek <hidden> Date: 2016-08-17 15:55:35
Hi!
quoted
quoted
quoted
+ - reset-gpios: Should specify the gpio for bluetooth reset
+ - host-wakeup-gpios: Should specify the gpio for host wakeup
Should be interrupt instead?
Yes this is mostly an interrupt, but I need to read the current
line state.
When? If the interrupt is level triggered, then you can get the line
state based on whether you get an interrupt or not. If this needs to
be a wakeup source (see the wakeup source binding), then it needs to
be an interrupt.
From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-13 03:15:22
This will be used by Nokia's H4+ protocol, which
adds padding to packets to reach word alignment.
---
drivers/bluetooth/hci_h4.c | 10 ++++++++++
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 11 insertions(+)
@@ -121,6 +121,7 @@ struct h4_recv_pkt {u8loff;/* Data length offset in header */u8lsize;/* Data length field size */u16maxlen;/* Max overall packet length */+boolwordaligned;/* packets are word aligned */int(*recv)(structhci_dev*hdev,structsk_buff*skb);};
This will be used by Nokia's H4+ protocol, which
adds padding to packets to reach word alignment.
---
drivers/bluetooth/hci_h4.c | 10 ++++++++++
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 11 insertions(+)
u8 loff; /* Data length offset in header */
u8 lsize; /* Data length field size */
u16 maxlen; /* Max overall packet length */
+ bool wordaligned; /* packets are word aligned */
I wonder if not a u8 align would be a way better choice here. We set it to 1 for all existing packet types. And the Nokia driver can use 2 here.
int (*recv)(struct hci_dev *hdev, struct sk_buff *skb);
From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-16 07:51:12
Hi Marcel,
On Tue, Aug 16, 2016 at 09:05:19AM +0200, Marcel Holtmann wrote:
Hi Sebatian,
quoted
This will be used by Nokia's H4+ protocol, which
adds padding to packets to reach word alignment.
---
drivers/bluetooth/hci_h4.c | 10 ++++++++++
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 11 insertions(+)
u8 loff; /* Data length offset in header */
u8 lsize; /* Data length field size */
u16 maxlen; /* Max overall packet length */
+ bool wordaligned; /* packets are word aligned */
I wonder if not a u8 align would be a way better choice here. We
set it to 1 for all existing packet types. And the Nokia driver
can use 2 here.
Sounds less hacky than my approach. Also it made me notice, that my
code is not safe, since the buffer size is not checked. I will use
u8 align and fix the buffer size check.
-- Sebastian
From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-13 03:15:27
Add bcm2048 node and its system clock to the N900 device tree file.
Apart from that a reference to the new clock has been added to
wl1251 (which uses it, too).
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
arch/arm/boot/dts/omap3-n900.dts | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
From: Pavel Machek <hidden> Date: 2016-08-14 08:53:28
On Sat 2016-08-13 05:14:37, Sebastian Reichel wrote:
Add bcm2048 node and its system clock to the N900 device tree file.
Apart from that a reference to the new clock has been added to
wl1251 (which uses it, too).
Signed-off-by: Sebastian Reichel <sre@kernel.org>
@@ -86,6 +86,16 @@ config BT_HCIUART_H4SayYheretocompilesupportforHCIUART(H4)protocol.+configBT_HCIUART_NOKIA+bool"UART Nokia H4+ protocol support"
If the option is a bool, then you don't need module.h or any MODULE
macros, nor do you need any __exit or unregister related code.
Alternatively, if there is a use case for it to be modular, then maybe you
want to change the above to a tristate.
Paul.
--
+ depends on BT_HCIUART
+ help
+ Nokia H4+ is serial protocol for communication between Bluetooth
+ device and host. This protocol is required for Bluetooth devices
+ with UART interface in Nokia devices.
+
+ Say Y here to compile support for Nokia's H4+ protocol.
+
config BT_HCIUART_BCSP
bool "BCSP protocol support"
depends on BT_HCIUART
From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-15 01:12:19
Hi Paul,
On Sun, Aug 14, 2016 at 07:54:28PM -0400, Paul Gortmaker wrote:
On Fri, Aug 12, 2016 at 11:14 PM, Sebastian Reichel [off-list ref] wrote:
quoted
This driver adds support for Nokia H4+ procotol used
for example by Nokia's internet tablets (N770 - N950).
[...]
+config BT_HCIUART_NOKIA
+ bool "UART Nokia H4+ protocol support"
If the option is a bool, then you don't need module.h or any MODULE
macros, nor do you need any __exit or unregister related code.
Alternatively, if there is a use case for it to be modular, then maybe you
want to change the above to a tristate.
Actually it is bool and modular: It becomes part of the module
configured by the tristate CONFIG_BT_HCIUART option.
-- Sebastian
Say Y here to compile support for HCI UART (H4) protocol.
+config BT_HCIUART_NOKIA
+ bool "UART Nokia H4+ protocol support"
+ depends on BT_HCIUART
+ help
+ Nokia H4+ is serial protocol for communication between Bluetooth
+ device and host. This protocol is required for Bluetooth devices
+ with UART interface in Nokia devices.
+
+ Say Y here to compile support for Nokia's H4+ protocol.
+
config BT_HCIUART_BCSP
bool "BCSP protocol support"
depends on BT_HCIUART
I am not sure we want these to go through the Bluetooth core packet sending. They are not standard HCI packet and should stay within the driver. If you send them through the core they will cause problems with the monitor interface.
So does this nokia_get_fw_name really needs to be a separate function? Or can this just be done right here in this function? I prefer it to be done where it is actually used. Unless you use that name in many places.
Are you sure you can ignore the RADIO_PKT commands. They are used to set up the FM radio parts of the chip. They are standard HCI commands (in the case of Broadcom at least). At minimum it should be added a comment here that you are ignoring them on purpose.
+ case HCI_NOKIA_NEG_PKT:
+ case HCI_NOKIA_ALIVE_PKT:
And here I would also a comment on why are we ignore these commands and driving this all by ourselves.
Where is the CSR BC4 one here? I prefer if we only have support for the ones that are actually supported and detected. We can easily extend things later.
So does this nokia_get_fw_name really needs to be a separate function? Or can this just be done right here in this function? I prefer it to be done where it is actually used. Unless you use that name in many places.
Are you sure you can ignore the RADIO_PKT commands. They are used to set up the FM radio parts of the chip. They are standard HCI commands (in the case of Broadcom at least). At minimum it should be added a comment here that you are ignoring them on purpose.
quoted
+ case HCI_NOKIA_NEG_PKT:
+ case HCI_NOKIA_ALIVE_PKT:
And here I would also a comment on why are we ignore these commands and driving this all by ourselves.
Good question... In Pavel's version of bluetooth driver, which is
working on Nokia N900, is sent whole firmware at one __hci_cmd_sync
step. It does not skip any packets, plus he added this comment:
/* Note that this is timing-critical. If sending packets takes
* too long, initialization will fail.
*/
So really, can we skip those packets? And is not this reason why
this bluetooth driver does not work on Nokia N900?
--
Pali Rohár
pali.rohar@gmail.com
From: Sebastian Reichel <sre@kernel.org> Date: 2016-08-16 09:26:51
Hi,
On Tue, Aug 16, 2016 at 09:52:17AM +0200, Pali Rohár wrote:
quoted
quoted
+ case HCI_NOKIA_RADIO_PKT:
Are you sure you can ignore the RADIO_PKT commands. They are
used to set up the FM radio parts of the chip. They are standard
HCI commands (in the case of Broadcom at least). At minimum it
should be added a comment here that you are ignoring them on
purpose.
quoted
+ case HCI_NOKIA_NEG_PKT:
+ case HCI_NOKIA_ALIVE_PKT:
And here I would also a comment on why are we ignore these
commands and driving this all by ourselves.
Good question... In Pavel's version of bluetooth driver, which is
working on Nokia N900, is sent whole firmware at one __hci_cmd_sync
step. It does not skip any packets, plus he added this comment:
/* Note that this is timing-critical. If sending packets takes
* too long, initialization will fail.
*/
So really, can we skip those packets? And is not this reason why
this bluetooth driver does not work on Nokia N900?
Let's have a look - here is Pavel's version:
https://lwn.net/Articles/627201/
In pseudocode:
while(true) {
cmd = get_cmd_from_firmware();
if (!cmd) break;
__hci_cmd_sync(cmd);
}
This is not "whole firmware at one __hci_cmd_sync step", is it?
And obviously that wouldn't work. Next let's have a look at
"It does not skip any packets":
/* Skip first two packets */
if (++num <= 2)
continue;
Which are HCI_NOKIA_NEG_PKT and HCI_NOKIA_ALIVE_PKT. Those are
open-coded. By using the packets from the firmware we could drop the
negotiation/alive functions from the driver and remove quite a few
lines of code. I think it should only be done after finding the N900
bug, though. I found it quite useful, that first communication does not
happen through the firmware file.
Surely the radio packet is not ignored, but that part is not used on
N950 and N900 fails at first packet, so no way to test the radio
packet handling. I probably should add a /* TODO: check how to
handle radio packets */ for the radio packet entry. Or implement it
the way Marcel suggested and hope that it just works once the other
bug is found.
-- Sebastian
I am not sure we want these to go through the Bluetooth core
packet sending. They are not standard HCI packet and should stay
within the driver. If you send them through the core they will
cause problems with the monitor interface.
So does this nokia_get_fw_name really needs to be a separate
function? Or can this just be done right here in this function? I
prefer it to be done where it is actually used. Unless you use
that name in many places.
Are you sure you can ignore the RADIO_PKT commands. They are used
to set up the FM radio parts of the chip. They are standard HCI
commands (in the case of Broadcom at least). At minimum it should
be added a comment here that you are ignoring them on purpose.
I got the driver working on N950. I think it does not make use of
the radio packets at all. On N900 they may be needed, though. I do
not reach far enough in the firmware loading process to know for
sure.
If I remember correctly your template driver does bundle it together
with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
opcode size is u8 instead of u16. I ignored it for now, since I
could not properly test it.
quoted
+ case HCI_NOKIA_NEG_PKT:
+ case HCI_NOKIA_ALIVE_PKT:
And here I would also a comment on why are we ignore these
commands and driving this all by ourselves.
I think we could use the packets from the firmware instead
of doing it manually (On N900 they are bit identical to the
manually generated one - On N950 I have not yet checked), but
until N900 works having it coded explicitly helps debugging.
Where is the CSR BC4 one here? I prefer if we only have support
for the ones that are actually supported and detected. We can
easily extend things later.
I will drop CSR stuff. I don't have a device to test it.
ah this is a leftover. Currently the driver does not set
set_bdaddr() callback, since it differs between ti and bcm backend.
It looks like btbcm_set_bdaddr() can be used for the broadcom based
chips, though.
Are you sure you can ignore the RADIO_PKT commands. They are used
to set up the FM radio parts of the chip. They are standard HCI
commands (in the case of Broadcom at least). At minimum it should
be added a comment here that you are ignoring them on purpose.
I got the driver working on N950. I think it does not make use of
the radio packets at all. On N900 they may be needed, though. I do
not reach far enough in the firmware loading process to know for
sure.
If I remember correctly your template driver does bundle it together
with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
opcode size is u8 instead of u16. I ignored it for now, since I
could not properly test it.
that sounds heavily like a bug somehow. I remember having decoded the Broadcom firmware and there it really has to go via standard HCI command. And that is the default Broadcom FM radio command.
My assumption was that it was an initial misunderstanding by the driver itself. Can someone send me all the Nokia firmware files and I have a look at them.
quoted
quoted
+ case HCI_NOKIA_NEG_PKT:
+ case HCI_NOKIA_ALIVE_PKT:
And here I would also a comment on why are we ignore these
commands and driving this all by ourselves.
I think we could use the packets from the firmware instead
of doing it manually (On N900 they are bit identical to the
manually generated one - On N950 I have not yet checked), but
until N900 works having it coded explicitly helps debugging.
We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.
ah this is a leftover. Currently the driver does not set
set_bdaddr() callback, since it differs between ti and bcm backend.
It looks like btbcm_set_bdaddr() can be used for the broadcom based
chips, though.
Yes. For the Broadcom chip, just select the btbcm_set_bdaddr and set the module dependency correctly. We already do that for hci_bcm.c anyway. For the TI one, extra the opcode for from the original driver and just add a TI function inside the driver. I think adding a btti.c module is overkill at the moment. We can extract that later. Even btusb.c carries some set_bdaddr function in the main driver where splitting them out made no sense at the moment.
For this ones I would have use the HCI event ones.
My original patch had this:
+#define NOK_RECV_NEG \
+ .type = NOK_NEG_PKT, \
+ .hlen = NOK_NEG_HDR_SIZE, \
+ .loff = 0, \
+ .lsize = 1, \
+ .maxlen = HCI_MAX_EVENT_SIZE
+
+#define NOK_RECV_ALIVE \
+ .type = NOK_ALIVE_PKT, \
+ .hlen = NOK_ALIVE_HDR_SIZE, \
+ .loff = 0, \
+ .lsize = 1, \
+ .maxlen = HCI_MAX_EVENT_SIZE
+
+#define NOK_RECV_RADIO \
+ .type = NOK_RADIO_PKT, \
+ .hlen = HCI_EVENT_HDR_SIZE, \
+ .loff = 1, \
+ .lsize = 1, \
+ .maxlen = HCI_MAX_EVENT_SIZE
+
+static const struct h4_recv_pkt nok_recv_pkts[] = {
+ { H4_RECV_ACL, .recv = hci_recv_frame },
+ { H4_RECV_SCO, .recv = hci_recv_frame },
+ { H4_RECV_EVENT, .recv = hci_recv_frame },
+ { NOK_RECV_NEG, .recv = nok_recv_neg },
+ { NOK_RECV_ALIVE, .recv = nok_recv_alive },
+ { NOK_RECV_RADIO, .recv = nok_recv_radio },
With just these simple defines at the top:
+#define NOK_NEG_PKT 0x06
+#define NOK_ALIVE_PKT 0x07
+#define NOK_RADIO_PKT 0x08
+
+#define NOK_NEG_HDR_SIZE 1
+#define NOK_ALIVE_HDR_SIZE 1
And I would prefer if we keep it like that.
ok. I used explicit defines, since it looks like
a copy/paste error otherwise.
With the .align setting you also need to introduce NOK_RECV_ACL etc. with complete definition anyway. Just make sure to use the HCI_* defines where possible.
An alternative is to add the align parameter to h4_recv_buf. Which might be the better idea anyway since I doubt the alignment only applies to a single packet type. It should apply to all of them since otherwise it makes no sense.
Regards
Marcel
From: Pavel Machek <hidden> Date: 2016-08-16 20:05:58
Hi!
quoted
I think we could use the packets from the firmware instead
of doing it manually (On N900 they are bit identical to the
manually generated one - On N950 I have not yet checked), but
until N900 works having it coded explicitly helps debugging.
We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.
Are you sure you can ignore the RADIO_PKT commands. They are used
to set up the FM radio parts of the chip. They are standard HCI
commands (in the case of Broadcom at least). At minimum it should
be added a comment here that you are ignoring them on purpose.
I got the driver working on N950. I think it does not make use of
the radio packets at all. On N900 they may be needed, though. I do
not reach far enough in the firmware loading process to know for
sure.
If I remember correctly your template driver does bundle it together
with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
opcode size is u8 instead of u16. I ignored it for now, since I
could not properly test it.
that sounds heavily like a bug somehow. I remember having decoded the Broadcom firmware and there it really has to go via standard HCI command. And that is the default Broadcom FM radio command.
My assumption was that it was an initial misunderstanding by the driver itself. Can someone send me all the Nokia firmware files and I have a look at them.
quoted
quoted
+ case HCI_NOKIA_NEG_PKT:
+ case HCI_NOKIA_ALIVE_PKT:
And here I would also a comment on why are we ignore these
commands and driving this all by ourselves.
I think we could use the packets from the firmware instead
of doing it manually (On N900 they are bit identical to the
manually generated one - On N950 I have not yet checked), but
until N900 works having it coded explicitly helps debugging.
We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.
ah this is a leftover. Currently the driver does not set
set_bdaddr() callback, since it differs between ti and bcm backend.
It looks like btbcm_set_bdaddr() can be used for the broadcom based
chips, though.
Yes. For the Broadcom chip, just select the btbcm_set_bdaddr and set the module dependency correctly. We already do that for hci_bcm.c anyway. For the TI one, extra the opcode for from the original driver and just add a TI function inside the driver. I think adding a btti.c module is overkill at the moment. We can extract that later. Even btusb.c carries some set_bdaddr function in the main driver where splitting them out made no sense at the moment.
For this ones I would have use the HCI event ones.
My original patch had this:
+#define NOK_RECV_NEG \
+ .type = NOK_NEG_PKT, \
+ .hlen = NOK_NEG_HDR_SIZE, \
+ .loff = 0, \
+ .lsize = 1, \
+ .maxlen = HCI_MAX_EVENT_SIZE
+
+#define NOK_RECV_ALIVE \
+ .type = NOK_ALIVE_PKT, \
+ .hlen = NOK_ALIVE_HDR_SIZE, \
+ .loff = 0, \
+ .lsize = 1, \
+ .maxlen = HCI_MAX_EVENT_SIZE
+
+#define NOK_RECV_RADIO \
+ .type = NOK_RADIO_PKT, \
+ .hlen = HCI_EVENT_HDR_SIZE, \
+ .loff = 1, \
+ .lsize = 1, \
+ .maxlen = HCI_MAX_EVENT_SIZE
+
+static const struct h4_recv_pkt nok_recv_pkts[] = {
+ { H4_RECV_ACL, .recv = hci_recv_frame },
+ { H4_RECV_SCO, .recv = hci_recv_frame },
+ { H4_RECV_EVENT, .recv = hci_recv_frame },
+ { NOK_RECV_NEG, .recv = nok_recv_neg },
+ { NOK_RECV_ALIVE, .recv = nok_recv_alive },
+ { NOK_RECV_RADIO, .recv = nok_recv_radio },
With just these simple defines at the top:
+#define NOK_NEG_PKT 0x06
+#define NOK_ALIVE_PKT 0x07
+#define NOK_RADIO_PKT 0x08
+
+#define NOK_NEG_HDR_SIZE 1
+#define NOK_ALIVE_HDR_SIZE 1
And I would prefer if we keep it like that.
ok. I used explicit defines, since it looks like
a copy/paste error otherwise.
With the .align setting you also need to introduce NOK_RECV_ACL etc. with complete definition anyway. Just make sure to use the HCI_* defines where possible.
An alternative is to add the align parameter to h4_recv_buf. Which might be the better idea anyway since I doubt the alignment only applies to a single packet type. It should apply to all of them since otherwise it makes no sense.
Regards
Marcel
since it seems the driver is getting closer to be ready, lets merge this extra protocol identifier as a separate patch. Then we can adapt btattach as well for it.
Regards
Marcel
since it seems the driver is getting closer to be ready,
lets merge this extra protocol identifier as a separate patch.
depends on the definition of "the driver". It requires
the gpio information from DT, so it depends on the new
serial bus. I still have to implement that and it has
the potential to trigger a bit of discussion :)
Anyways I will split this out into its own patch later.
Then we can adapt btattach as well for it.
It's enough to set ldisc=N_HCI and proto=HCI_UART_NOKIA.
Everything else is done by the protocol driver. I still
think it should be done from the kernel and will try to
come up with something in the serial bus.
-- Sebastian
This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
N9xx devices. It has been tested on the Nokia N950, where it works
correctly. On Nokia N900 it currently fails during negotiation
(probably related to slightly incorrect serial settings/timings).
The N900's bcm2048 correctly answeres to alive check even before
negotiation (on N950 it does not work before negotiation), but replies
with an Hardware error event to the negotiation packet.
Apart from N900 support there are still two "features" missing in the
driver:
1. To save energy the bluetooth module can be put into sleep mode via a
GPIO. This gpio should be enabled before sending data via UART and
disabled once the transmission is done. I currently just keep the
GPIO always enabled.
2. It would be nice to have a bluetooth device exposed by the kernel
automatically without having to setup the tty disector first for
proper configurationless out of the box support. I could not find
a nice way to do this from the kernel, though.
currently using the HCI line discipline is the only way to attach a Bluetooth device to a serial line / UART.
However I have been advocating for a serial bus or UART bus for a long time now. It is needed especially for Bluetooth devices which have no business in being exposed as TTYs in the first place.
This is true for the ACPI and DT world actually. Some UARTs should not be exposed as TTY and just be stuck on a bus that can be enumerated and matched by a driver that knows how to handle it.
This goes along with the weird fetish to expose certain Bluetooth GPIOs as RFKILL switches. They are not that either since they just power the UART. They have nothing to do with a radio RFKILL switch. We fixed the Intel and Broadcom ones to be mapped into the driver. Having a proper bus would make this one also a lot easier.
Regards
Marcel
From: Rob Herring <robh+dt@kernel.org> Date: 2016-08-16 20:23:14
On Tue, Aug 16, 2016 at 2:10 AM, Marcel Holtmann [off-list ref] wrote:
Hi Sebastian,
quoted
This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
N9xx devices. It has been tested on the Nokia N950, where it works
correctly. On Nokia N900 it currently fails during negotiation
(probably related to slightly incorrect serial settings/timings).
The N900's bcm2048 correctly answeres to alive check even before
negotiation (on N950 it does not work before negotiation), but replies
with an Hardware error event to the negotiation packet.
Apart from N900 support there are still two "features" missing in the
driver:
1. To save energy the bluetooth module can be put into sleep mode via a
GPIO. This gpio should be enabled before sending data via UART and
disabled once the transmission is done. I currently just keep the
GPIO always enabled.
2. It would be nice to have a bluetooth device exposed by the kernel
automatically without having to setup the tty disector first for
proper configurationless out of the box support. I could not find
a nice way to do this from the kernel, though.
currently using the HCI line discipline is the only way to attach a Bluetooth device to a serial line / UART.
However I have been advocating for a serial bus or UART bus for a long time now. It is needed especially for Bluetooth devices which have no business in being exposed as TTYs in the first place.
This is true for the ACPI and DT world actually. Some UARTs should not be exposed as TTY and just be stuck on a bus that can be enumerated and matched by a driver that knows how to handle it.
To add to this, my initial thought has been to split off uart_port ops
for use by the uart bus/subsystem so we can reuse all the uart
drivers. Then driver ports can be registered either with the tty layer
or the uart bus (or perhaps registered with both and claimed by one
later). This is somewhat already done with the serio subsys (see
drivers/tty/serial/sunsu.c). I thought extending serio might be an
option, but it's data handling is pretty limited.
The transmit side seems pretty straightforward. The receive side is a
bit more complex as the buffering is all in the tty layer. It doesn't
seem so straightforward to share the tty buffer handling code. Perhaps
at least initially, a more simple buffering scheme can be used given
the packet oriented nature of the device protocols.
Rob