* [RFC 0/7] Nokia N9xx bluetooth driver @ 2016-08-13 3:14 Sebastian Reichel 2016-08-13 3:14 ` [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init Sebastian Reichel ` (7 more replies) 0 siblings, 8 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 3:14 UTC (permalink / raw) To: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby Cc: Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel @ 2016-08-13 3:14 ` Sebastian Reichel 2016-08-14 8:49 ` Pavel Machek 2016-08-13 3:14 ` [RFC 2/7] tty: add support for "tty slave" devices Sebastian Reichel ` (6 subsequent siblings) 7 siblings, 1 reply; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 3:14 UTC (permalink / raw) To: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby Cc: Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel --- drivers/tty/serial/omap-serial.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index a2a529994ba5..7c2c77789c2c 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -1542,6 +1542,9 @@ static struct omap_uart_port_info *of_get_uart_port_info(struct device *dev) of_property_read_u32(dev->of_node, "clock-frequency", &omap_up_info->uartclk); + + omap_up_info->flags = UPF_BOOT_AUTOCONF; + return omap_up_info; } -- 2.8.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init 2016-08-13 3:14 ` [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init Sebastian Reichel @ 2016-08-14 8:49 ` Pavel Machek 2016-08-16 8:14 ` Sebastian Reichel 0 siblings, 1 reply; 37+ messages in thread From: Pavel Machek @ 2016-08-14 8:49 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel 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. Acked-by: Pavel Machek <pavel@ucw.cz> Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init 2016-08-14 8:49 ` Pavel Machek @ 2016-08-16 8:14 ` Sebastian Reichel 0 siblings, 0 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-16 8:14 UTC (permalink / raw) To: Pavel Machek Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 345 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 2/7] tty: add support for "tty slave" devices 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel 2016-08-13 3:14 ` [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init Sebastian Reichel @ 2016-08-13 3:14 ` Sebastian Reichel 2016-08-13 10:03 ` Greg Kroah-Hartman 2016-08-13 3:14 ` [RFC 3/7] dt: bindings: Add nokia-bluetooth Sebastian Reichel ` (5 subsequent siblings) 7 siblings, 1 reply; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 3:14 UTC (permalink / raw) To: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby Cc: Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel, NeilBrown From: NeilBrown <neilb@suse.de> 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 <neilb@suse.de> 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(+) diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt index f5561ac7e17e..ecb74730f71b 100644 --- a/Documentation/devicetree/bindings/serial/8250.txt +++ b/Documentation/devicetree/bindings/serial/8250.txt @@ -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: ------------ diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index 734a635e7363..39ff5dcdfd50 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -95,6 +95,7 @@ #include <linux/seq_file.h> #include <linux/serial.h> #include <linux/ratelimit.h> +#include <linux/of_platform.h> #include <linux/uaccess.h> @@ -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); return dev; -- 2.8.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 2/7] tty: add support for "tty slave" devices 2016-08-13 3:14 ` [RFC 2/7] tty: add support for "tty slave" devices Sebastian Reichel @ 2016-08-13 10:03 ` Greg Kroah-Hartman 2016-08-13 20:31 ` Sebastian Reichel 2016-08-14 8:48 ` Pavel Machek 0 siblings, 2 replies; 37+ messages in thread From: Greg Kroah-Hartman @ 2016-08-13 10:03 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel, NeilBrown On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote: > From: NeilBrown <neilb@suse.de> > > 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 <neilb@suse.de> > 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(+) > > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt > index f5561ac7e17e..ecb74730f71b 100644 > --- a/Documentation/devicetree/bindings/serial/8250.txt > +++ b/Documentation/devicetree/bindings/serial/8250.txt > @@ -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: > ------------ > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > index 734a635e7363..39ff5dcdfd50 100644 > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -95,6 +95,7 @@ > #include <linux/seq_file.h> > #include <linux/serial.h> > #include <linux/ratelimit.h> > +#include <linux/of_platform.h> > > #include <linux/uaccess.h> > > @@ -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? thanks, greg k-h ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/7] tty: add support for "tty slave" devices 2016-08-13 10:03 ` Greg Kroah-Hartman @ 2016-08-13 20:31 ` Sebastian Reichel 2016-08-14 11:36 ` Greg Kroah-Hartman 2016-08-14 8:48 ` Pavel Machek 1 sibling, 1 reply; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 20:31 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel, NeilBrown [-- Attachment #1: Type: text/plain, Size: 2559 bytes --] 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: > > From: NeilBrown <neilb@suse.de> > > > > 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 <neilb@suse.de> > > 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(+) > > > > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt > > index f5561ac7e17e..ecb74730f71b 100644 > > --- a/Documentation/devicetree/bindings/serial/8250.txt > > +++ b/Documentation/devicetree/bindings/serial/8250.txt > > @@ -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: > > ------------ > > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > > index 734a635e7363..39ff5dcdfd50 100644 > > --- a/drivers/tty/tty_io.c > > +++ b/drivers/tty/tty_io.c > > @@ -95,6 +95,7 @@ > > #include <linux/seq_file.h> > > #include <linux/serial.h> > > #include <linux/ratelimit.h> > > +#include <linux/of_platform.h> > > > > #include <linux/uaccess.h> > > > > @@ -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? 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/7] tty: add support for "tty slave" devices 2016-08-13 20:31 ` Sebastian Reichel @ 2016-08-14 11:36 ` Greg Kroah-Hartman 0 siblings, 0 replies; 37+ messages in thread From: Greg Kroah-Hartman @ 2016-08-14 11:36 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel, NeilBrown 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: > > On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote: > > > From: NeilBrown <neilb@suse.de> > > > > > > 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 <neilb@suse.de> > > > 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(+) > > > > > > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt > > > index f5561ac7e17e..ecb74730f71b 100644 > > > --- a/Documentation/devicetree/bindings/serial/8250.txt > > > +++ b/Documentation/devicetree/bindings/serial/8250.txt > > > @@ -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: > > > ------------ > > > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > > > index 734a635e7363..39ff5dcdfd50 100644 > > > --- a/drivers/tty/tty_io.c > > > +++ b/drivers/tty/tty_io.c > > > @@ -95,6 +95,7 @@ > > > #include <linux/seq_file.h> > > > #include <linux/serial.h> > > > #include <linux/ratelimit.h> > > > +#include <linux/of_platform.h> > > > > > > #include <linux/uaccess.h> > > > > > > @@ -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? > > 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/7] tty: add support for "tty slave" devices 2016-08-13 10:03 ` Greg Kroah-Hartman 2016-08-13 20:31 ` Sebastian Reichel @ 2016-08-14 8:48 ` Pavel Machek 2016-08-14 11:35 ` Greg Kroah-Hartman 1 sibling, 1 reply; 37+ messages in thread From: Pavel Machek @ 2016-08-14 8:48 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel, NeilBrown 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: > > From: NeilBrown <neilb@suse.de> > > > > 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 <neilb@suse.de> > > Signed-off-by: Sebastian Reichel <sre@kernel.org> Acked-by: Pavel Machek <pavel@ucw.cz> > > @@ -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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/7] tty: add support for "tty slave" devices 2016-08-14 8:48 ` Pavel Machek @ 2016-08-14 11:35 ` Greg Kroah-Hartman 0 siblings, 0 replies; 37+ messages in thread From: Greg Kroah-Hartman @ 2016-08-14 11:35 UTC (permalink / raw) To: Pavel Machek Cc: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel, NeilBrown 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: > > On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote: > > > From: NeilBrown <neilb@suse.de> > > > > > > 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 <neilb@suse.de> > > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > > Acked-by: Pavel Machek <pavel@ucw.cz> > > > > @@ -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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 3/7] dt: bindings: Add nokia-bluetooth 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel 2016-08-13 3:14 ` [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init Sebastian Reichel 2016-08-13 3:14 ` [RFC 2/7] tty: add support for "tty slave" devices Sebastian Reichel @ 2016-08-13 3:14 ` Sebastian Reichel 2016-08-16 13:51 ` Rob Herring 2016-08-13 3:14 ` [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment Sebastian Reichel ` (4 subsequent siblings) 7 siblings, 1 reply; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 3:14 UTC (permalink / raw) To: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby Cc: Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel --- .../devicetree/bindings/net/nokia-bluetooth.txt | 43 ++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt new file mode 100644 index 000000000000..a0fceabb4cce --- /dev/null +++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt @@ -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"; + }; + +}; -- 2.8.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth 2016-08-13 3:14 ` [RFC 3/7] dt: bindings: Add nokia-bluetooth Sebastian Reichel @ 2016-08-16 13:51 ` Rob Herring 2016-08-16 23:28 ` Sebastian Reichel 0 siblings, 1 reply; 37+ messages in thread From: Rob Herring @ 2016-08-16 13:51 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel On Sat, Aug 13, 2016 at 05:14:34AM +0200, Sebastian Reichel wrote: > --- > .../devicetree/bindings/net/nokia-bluetooth.txt | 43 ++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt > > diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt > new file mode 100644 > index 000000000000..a0fceabb4cce > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt > @@ -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. > + 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"; > + }; > + > +}; > -- > 2.8.1 > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth 2016-08-16 13:51 ` Rob Herring @ 2016-08-16 23:28 ` Sebastian Reichel 2016-08-17 13:11 ` Rob Herring 0 siblings, 1 reply; 37+ messages in thread From: Sebastian Reichel @ 2016-08-16 23:28 UTC (permalink / raw) To: Rob Herring Cc: Tony Lindgren, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3406 bytes --] Hi Rob, On Tue, Aug 16, 2016 at 08:51:55AM -0500, Rob Herring wrote: > On Sat, Aug 13, 2016 at 05:14:34AM +0200, Sebastian Reichel wrote: > > --- > > .../devicetree/bindings/net/nokia-bluetooth.txt | 43 ++++++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt > > > > diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt > > new file mode 100644 > > index 000000000000..a0fceabb4cce > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt > > @@ -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. > > + - 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. > > + - bluetooth-wakeup-gpios: Should specify the gpio for bluetooth wakeup > > State direction and active state for gpios. ok. > > + - 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. > > + 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"; > > + }; > > + > > +}; -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth 2016-08-16 23:28 ` Sebastian Reichel @ 2016-08-17 13:11 ` Rob Herring 2016-08-17 15:54 ` Pavel Machek 0 siblings, 1 reply; 37+ messages in thread From: Rob Herring @ 2016-08-17 13:11 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, Ivaylo Dimitrov, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel On Tue, Aug 16, 2016 at 6:28 PM, Sebastian Reichel <sre@kernel.org> wrote: > Hi Rob, > > On Tue, Aug 16, 2016 at 08:51:55AM -0500, Rob Herring wrote: >> On Sat, Aug 13, 2016 at 05:14:34AM +0200, Sebastian Reichel wrote: >> > --- >> > .../devicetree/bindings/net/nokia-bluetooth.txt | 43 ++++++++++++++++++++++ >> > 1 file changed, 43 insertions(+) >> > create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt >> > >> > diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt >> > new file mode 100644 >> > index 000000000000..a0fceabb4cce >> > --- /dev/null >> > +++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt >> > @@ -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. >> > + - 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. > >> > + - bluetooth-wakeup-gpios: Should specify the gpio for bluetooth wakeup >> >> State direction and active state for gpios. > > ok. > >> > + - 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth 2016-08-17 13:11 ` Rob Herring @ 2016-08-17 15:54 ` Pavel Machek 0 siblings, 0 replies; 37+ messages in thread From: Pavel Machek @ 2016-08-17 15:54 UTC (permalink / raw) To: Rob Herring Cc: Sebastian Reichel, Tony Lindgren, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, Ivaylo Dimitrov, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel Hi! > >> > + - 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. What is wrong with GPIO? It is simply a piece of wire. Yes, we'll configure it to generate an interrupt, or maybe not, but that's software detail, the device tree just should not care.... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel ` (2 preceding siblings ...) 2016-08-13 3:14 ` [RFC 3/7] dt: bindings: Add nokia-bluetooth Sebastian Reichel @ 2016-08-13 3:14 ` Sebastian Reichel 2016-08-14 8:51 ` Pavel Machek 2016-08-16 7:05 ` Marcel Holtmann 2016-08-13 3:14 ` [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel ` (3 subsequent siblings) 7 siblings, 2 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 3:14 UTC (permalink / raw) To: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby Cc: Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel 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(+) diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c index 635597b6e168..a934e4eb692b 100644 --- a/drivers/bluetooth/hci_h4.c +++ b/drivers/bluetooth/hci_h4.c @@ -253,11 +253,21 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, } if (!dlen) { + if ((&pkts[i])->wordaligned && !(skb->len % 2)) { + buffer++; + count--; + } + /* No more data, complete frame */ (&pkts[i])->recv(hdev, skb); skb = NULL; } } else { + if ((&pkts[i])->wordaligned && !(skb->len % 2)) { + buffer++; + count--; + } + /* Complete frame */ (&pkts[i])->recv(hdev, skb); skb = NULL; diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h index 839bad1d8152..a7d67aec3632 100644 --- a/drivers/bluetooth/hci_uart.h +++ b/drivers/bluetooth/hci_uart.h @@ -121,6 +121,7 @@ struct h4_recv_pkt { 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 */ int (*recv)(struct hci_dev *hdev, struct sk_buff *skb); }; -- 2.8.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment 2016-08-13 3:14 ` [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment Sebastian Reichel @ 2016-08-14 8:51 ` Pavel Machek 2016-08-16 7:05 ` Marcel Holtmann 1 sibling, 0 replies; 37+ messages in thread From: Pavel Machek @ 2016-08-14 8:51 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel On Sat 2016-08-13 05:14:35, Sebastian Reichel wrote: > This will be used by Nokia's H4+ protocol, which > adds padding to packets to reach word alignment. Add a sign-off. Acked-by: Pavel Machek <pavel@ucw.cz> Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment 2016-08-13 3:14 ` [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment Sebastian Reichel 2016-08-14 8:51 ` Pavel Machek @ 2016-08-16 7:05 ` Marcel Holtmann 2016-08-16 7:51 ` Sebastian Reichel 1 sibling, 1 reply; 37+ messages in thread From: Marcel Holtmann @ 2016-08-16 7:05 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel Hi Sebatian, > 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(+) > > diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c > index 635597b6e168..a934e4eb692b 100644 > --- a/drivers/bluetooth/hci_h4.c > +++ b/drivers/bluetooth/hci_h4.c > @@ -253,11 +253,21 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, > } > > if (!dlen) { > + if ((&pkts[i])->wordaligned && !(skb->len % 2)) { > + buffer++; > + count--; > + } > + > /* No more data, complete frame */ > (&pkts[i])->recv(hdev, skb); > skb = NULL; > } > } else { > + if ((&pkts[i])->wordaligned && !(skb->len % 2)) { > + buffer++; > + count--; > + } > + > /* Complete frame */ > (&pkts[i])->recv(hdev, skb); > skb = NULL; > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h > index 839bad1d8152..a7d67aec3632 100644 > --- a/drivers/bluetooth/hci_uart.h > +++ b/drivers/bluetooth/hci_uart.h > @@ -121,6 +121,7 @@ struct h4_recv_pkt { > 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); Regards Marcel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment 2016-08-16 7:05 ` Marcel Holtmann @ 2016-08-16 7:51 ` Sebastian Reichel 0 siblings, 0 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-16 7:51 UTC (permalink / raw) To: Marcel Holtmann Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1940 bytes --] Hi Marcel, On Tue, Aug 16, 2016 at 09:05:19AM +0200, Marcel Holtmann wrote: > Hi Sebatian, > > > 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(+) > > > > diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c > > index 635597b6e168..a934e4eb692b 100644 > > --- a/drivers/bluetooth/hci_h4.c > > +++ b/drivers/bluetooth/hci_h4.c > > @@ -253,11 +253,21 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, > > } > > > > if (!dlen) { > > + if ((&pkts[i])->wordaligned && !(skb->len % 2)) { > > + buffer++; > > + count--; > > + } > > + > > /* No more data, complete frame */ > > (&pkts[i])->recv(hdev, skb); > > skb = NULL; > > } > > } else { > > + if ((&pkts[i])->wordaligned && !(skb->len % 2)) { > > + buffer++; > > + count--; > > + } > > + > > /* Complete frame */ > > (&pkts[i])->recv(hdev, skb); > > skb = NULL; > > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h > > index 839bad1d8152..a7d67aec3632 100644 > > --- a/drivers/bluetooth/hci_uart.h > > +++ b/drivers/bluetooth/hci_uart.h > > @@ -121,6 +121,7 @@ struct h4_recv_pkt { > > 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel ` (3 preceding siblings ...) 2016-08-13 3:14 ` [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment Sebastian Reichel @ 2016-08-13 3:14 ` Sebastian Reichel 2016-08-14 23:54 ` Paul Gortmaker ` (2 more replies) 2016-08-13 3:14 ` [RFC 6/7] ARM: dts: OMAP3-N900: Add bluetooth Sebastian Reichel ` (2 subsequent siblings) 7 siblings, 3 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 3:14 UTC (permalink / raw) To: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby Cc: Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel This driver adds support for Nokia H4+ procotol used for example by Nokia's internet tablets (N770 - N950). --- drivers/bluetooth/Kconfig | 10 + drivers/bluetooth/Makefile | 1 + drivers/bluetooth/hci_ldisc.c | 6 + drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++ drivers/bluetooth/hci_nokia.h | 140 ++++++++ drivers/bluetooth/hci_uart.h | 8 +- 6 files changed, 898 insertions(+), 1 deletion(-) create mode 100644 drivers/bluetooth/hci_nokia.c create mode 100644 drivers/bluetooth/hci_nokia.h diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index cf50fd2e96df..c32d9d5ad1d2 100644 --- a/drivers/bluetooth/Kconfig +++ b/drivers/bluetooth/Kconfig @@ -86,6 +86,16 @@ config BT_HCIUART_H4 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 diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile index 9c18939fc5c9..f7951646ee14 100644 --- a/drivers/bluetooth/Makefile +++ b/drivers/bluetooth/Makefile @@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL) += hci_intel.o hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o hci_uart-$(CONFIG_BT_HCIUART_QCA) += hci_qca.o hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o +hci_uart-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o hci_uart-objs := $(hci_uart-y) ccflags-y += -D__CHECK_ENDIAN__ diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c index dda97398c59a..83d0de94bf35 100644 --- a/drivers/bluetooth/hci_ldisc.c +++ b/drivers/bluetooth/hci_ldisc.c @@ -810,6 +810,9 @@ static int __init hci_uart_init(void) #ifdef CONFIG_BT_HCIUART_AG6XX ag6xx_init(); #endif +#ifdef CONFIG_BT_HCIUART_NOKIA + nokia_init(); +#endif return 0; } @@ -845,6 +848,9 @@ static void __exit hci_uart_exit(void) #ifdef CONFIG_BT_HCIUART_AG6XX ag6xx_deinit(); #endif +#ifdef CONFIG_BT_HCIUART_NOKIA + nokia_deinit(); +#endif /* Release tty registration of line discipline */ err = tty_unregister_ldisc(N_HCI); diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c new file mode 100644 index 000000000000..efd4dd320838 --- /dev/null +++ b/drivers/bluetooth/hci_nokia.c @@ -0,0 +1,734 @@ +/* + * + * Bluetooth HCI UART H4 driver with Nokia Extensions + * + * Copyright (C) 2015 Marcel Holtmann <marcel@holtmann.org> + * Copyright (C) 2016 Sebastian Reichel <sre@kernel.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/module.h> + +#include <linux/clk.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/types.h> +#include <linux/fcntl.h> +#include <linux/interrupt.h> +#include <linux/ptrace.h> +#include <linux/poll.h> +#include <linux/pm_runtime.h> +#include <linux/firmware.h> +#include <linux/slab.h> +#include <linux/tty.h> +#include <linux/errno.h> +#include <linux/string.h> +#include <linux/signal.h> +#include <linux/ioctl.h> +#include <linux/skbuff.h> +#include <linux/delay.h> +#include <linux/platform_device.h> + +#include <linux/gpio/consumer.h> + +#include <linux/unaligned/le_struct.h> +#include <net/bluetooth/bluetooth.h> +#include <net/bluetooth/hci_core.h> + +#include "hci_uart.h" +#include "hci_nokia.h" + +struct nokia_uart_dev { + struct device *dev; + struct tty_port *port; + struct gpio_desc *reset; + struct gpio_desc *wakeup_host; + struct gpio_desc *wakeup_bt; + unsigned long sysclk_speed; +}; + +struct nokia_bt_dev { + struct hci_uart *hu; + struct nokia_uart_dev *btdata; + int wake_irq; + bool wake_state; + struct sk_buff *rx_skb; + struct sk_buff_head txq; + bdaddr_t bdaddr; + + int init_error; + struct completion init_completion; + + uint8_t man_id; + uint8_t ver_id; +}; + +static char *nokia_get_fw_name(struct nokia_bt_dev *btdev) +{ + switch (btdev->man_id) { + case NOKIA_ID_CSR: + return FIRMWARE_CSR; + case NOKIA_ID_BCM2048: + return FIRMWARE_BCM2048; + case NOKIA_ID_TI1271: + return FIRMWARE_TI1271; + default: + return NULL; + } +} + +static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state, + int timeout_ms) +{ + unsigned long timeout; + int signal; + + timeout = jiffies + msecs_to_jiffies(timeout_ms); + for (;;) { + signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS; + if (!!signal == !!state) { + dev_dbg(hu->tty->dev, "wait for cts... received!\n"); + return 0; + } + if (time_after(jiffies, timeout)) { + dev_dbg(hu->tty->dev, "wait for cts... timeout!\n"); + return -ETIMEDOUT; + } + usleep_range(1000, 2000); + } +} + +static int btdev_match(struct device *child, void *data) +{ + if (!strcmp(child->driver->name, "nokia-bluetooth")) + return 1; + else + return 0; +} + +static irqreturn_t wakeup_handler(int irq, void *data) +{ + struct nokia_bt_dev *btdev = data; + struct device *serialdev = btdev->hu->tty->dev; + int wake_state = gpiod_get_value(btdev->btdata->wakeup_host); + + dev_dbg(serialdev, "wakeup received: %d -> %d\n", + btdev->wake_state, wake_state); + + if (btdev->wake_state == wake_state) + return IRQ_HANDLED; + + if (wake_state) + pm_runtime_get_sync(serialdev); + else if (!wake_state) + pm_runtime_put(serialdev); + + btdev->wake_state = wake_state; + + return IRQ_HANDLED; +} + +static int nokia_reset(struct hci_uart *hu) +{ + struct nokia_bt_dev *btdev = hu->priv; + int err; + + /* reset routine */ + gpiod_set_value_cansleep(btdev->btdata->reset, 0); + gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1); + + msleep(50); + + /* safety check */ + err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host); + if (err == 1) { + dev_err(hu->tty->dev, "reset: host wakeup not low!\n"); + return -EPROTO; + } + + /* flush queues */ + tty_ldisc_flush(hu->tty); + tty_driver_flush_buffer(hu->tty); + + /* init uart */ + hci_uart_init_tty(hu); + hci_uart_set_flow_control(hu, true); + hci_uart_set_baudrate(hu, INIT_SPEED); + + gpiod_set_value_cansleep(btdev->btdata->reset, 1); + gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 0); + + msleep(100); + + err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host); + if (err == 0) { + dev_err(hu->tty->dev, "reset: host wakeup not high!\n"); + return -EPROTO; + } + + /* wait for cts */ + err = hci_uart_wait_for_cts(hu, true, 100); + if (err < 0) { + dev_err(hu->tty->dev, "CTS not received: %d\n", err); + return err; + } + + gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1); + hci_uart_set_flow_control(hu, false); + + return 0; +} + +static int nokia_send_alive_packet(struct hci_uart *hu) +{ + struct nokia_bt_dev *btdev = hu->priv; + struct hci_nokia_alive_hdr *hdr; + struct hci_nokia_alive_pkt *pkt; + struct sk_buff *skb; + int len; + + dev_dbg(hu->tty->dev, "Sending alive packet...\n"); + + init_completion(&btdev->init_completion); + + len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt); + skb = bt_skb_alloc(len, GFP_KERNEL); + if (!skb) + return -ENOMEM; + + hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT; + memset(skb->data, 0x00, len); + + hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr)); + hdr->dlen = sizeof(*pkt); + pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt)); + pkt->mid = NOKIA_ALIVE_REQ; + + hu->hdev->send(hu->hdev, skb); + + if (!wait_for_completion_interruptible_timeout(&btdev->init_completion, + msecs_to_jiffies(1000))) { + return -ETIMEDOUT; + } + + if (btdev->init_error < 0) + return btdev->init_error; + + return 0; +} + +static int nokia_send_negotiation(struct hci_uart *hu) +{ + struct nokia_bt_dev *btdev = hu->priv; + struct hci_nokia_neg_cmd *neg_cmd; + struct hci_nokia_neg_hdr *neg_hdr; + struct sk_buff *skb; + int len, err; + u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE); + int sysclk = btdev->btdata->sysclk_speed / 1000; + + dev_dbg(hu->tty->dev, "Sending negotiation...\n"); + + len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd); + skb = bt_skb_alloc(len, GFP_KERNEL); + if (!skb) + return -ENOMEM; + + hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT; + + neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr)); + neg_hdr->dlen = sizeof(*neg_cmd); + + neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd)); + neg_cmd->ack = NOKIA_NEG_REQ; + neg_cmd->baud = cpu_to_le16(baud); + neg_cmd->unused1 = 0x0000; + neg_cmd->proto = NOKIA_PROTO_BYTE; + neg_cmd->sys_clk = cpu_to_le16(sysclk); + neg_cmd->unused2 = 0x0000; + + btdev->init_error = 0; + init_completion(&btdev->init_completion); + + hu->hdev->send(hu->hdev, skb); + + if (!wait_for_completion_interruptible_timeout(&btdev->init_completion, + msecs_to_jiffies(10000))) { + return -ETIMEDOUT; + } + + if (btdev->init_error < 0) + return btdev->init_error; + + /* Change to operational settings */ + hci_uart_set_flow_control(hu, true); // disable flow control + + /* setup negotiated max. baudrate */ + hci_uart_set_baudrate(hu, MAX_BAUD_RATE); + + err = hci_uart_wait_for_cts(hu, true, 100); + if (err < 0) + return err; + + hci_uart_set_flow_control(hu, false); // re-enable flow control + + dev_dbg(hu->tty->dev, "Negotiation successful...\n"); + + return 0; +} + +static int nokia_setup_fw(struct hci_uart *hu) +{ + struct nokia_bt_dev *btdev = hu->priv; + const struct firmware *fw; + const u8 *fw_ptr; + size_t fw_size; + int err; + + BT_DBG("hu %p", hu); + + err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev); + if (err < 0) { + BT_ERR("%s: Failed to load Nokia firmware file (%d)", + hu->hdev->name, err); + return err; + } + + fw_ptr = fw->data; + fw_size = fw->size; + + while (fw_size >= 4) { + u16 pkt_size = get_unaligned_le16(fw_ptr); + u8 pkt_type = fw_ptr[2]; + const struct hci_command_hdr *cmd; + u16 opcode; + struct sk_buff *skb; + + switch (pkt_type) { + case HCI_COMMAND_PKT: + cmd = (struct hci_command_hdr *)(fw_ptr + 3); + opcode = le16_to_cpu(cmd->opcode); + + skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen, + fw_ptr + 3 + HCI_COMMAND_HDR_SIZE, + HCI_INIT_TIMEOUT); + if (IS_ERR(skb)) { + err = PTR_ERR(skb); + BT_ERR("%s: Firmware command %04x failed (%d)", + hu->hdev->name, opcode, err); + goto done; + } + kfree_skb(skb); + break; + case HCI_NOKIA_RADIO_PKT: + case HCI_NOKIA_NEG_PKT: + case HCI_NOKIA_ALIVE_PKT: + break; + } + + fw_ptr += pkt_size + 2; + fw_size -= pkt_size + 2; + } + +done: + release_firmware(fw); + return err; +} + +static int nokia_setup(struct hci_uart *hu) +{ + int err; + + pm_runtime_get_sync(hu->tty->dev); + + dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n"); + + /* 0. reset connection */ + err = nokia_reset(hu); + if (err < 0) { + dev_err(hu->tty->dev, "Reset failed: %d\n", err); + goto out; + } + + /* 1. negotiate speed etc */ + err = nokia_send_negotiation(hu); + if (err < 0) { + dev_err(hu->tty->dev, "Negotiation failed: %d\n", err); + goto out; + } + + /* 2. verify correct setup using alive packet */ + err = nokia_send_alive_packet(hu); + if (err < 0) { + dev_err(hu->tty->dev, "Alive check failed: %d\n", err); + goto out; + } + + /* 3. send firmware */ + err = nokia_setup_fw(hu); + if (err < 0) { + dev_err(hu->tty->dev, "Could not setup FW: %d\n", err); + goto out; + } + + hci_uart_set_flow_control(hu, true); + hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE); + hci_uart_set_flow_control(hu, false); + + dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n"); + + /* + * TODO: + * disable wakeup_bt at this point and automatically enable it when + * data is about to be written until all data has been written (+ some + * delay). + * + * Since this is not yet support by the uart/tty kernel framework we + * will always keep enabled the wakeup_bt gpio for now, so that the + * bluetooth chip will never transit into idle modes. + */ + +out: + pm_runtime_put(hu->tty->dev); + + return err; +} + +static int nokia_open(struct hci_uart *hu) +{ + struct device *serialdev = hu->tty->dev; + struct nokia_bt_dev *btdev; + struct device *uartbtdev; + int err; + + btdev = kzalloc(sizeof(*btdev), GFP_KERNEL); + if (!btdev) + return -ENOMEM; + + btdev->hu = hu; + + skb_queue_head_init(&btdev->txq); + + uartbtdev = device_find_child(serialdev, NULL, btdev_match); + if (!uartbtdev) { + dev_err(serialdev, "bluetooth device node not found!\n"); + return -ENODEV; + } + + btdev->btdata = dev_get_drvdata(uartbtdev); + if (!btdev->btdata) + return -EINVAL; + + hu->priv = btdev; + + /* register handler for host wakeup gpio */ + btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host); + err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler, + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + "wakeup", btdev); + if (err) { + gpiod_set_value(btdev->btdata->reset, 0); + gpiod_set_value(btdev->btdata->wakeup_bt, 0); + return err; + } + + dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n", + dev_name(uartbtdev)); + + pm_runtime_enable(hu->tty->dev); + + return 0; +} + +static int nokia_flush(struct hci_uart *hu) +{ + struct nokia_bt_dev *btdev = hu->priv; + + BT_DBG("hu %p", hu); + + skb_queue_purge(&btdev->txq); + + return 0; +} + +static int nokia_close(struct hci_uart *hu) +{ + struct nokia_bt_dev *btdev = hu->priv; + + hu->priv = NULL; + + BT_DBG("hu %p", hu); + + skb_queue_purge(&btdev->txq); + + kfree_skb(btdev->rx_skb); + + free_irq(btdev->wake_irq, btdev); + + /* disable module */ + gpiod_set_value(btdev->btdata->reset, 0); + gpiod_set_value(btdev->btdata->wakeup_bt, 0); + + hu->priv = NULL; + kfree(btdev); + + pm_runtime_disable(hu->tty->dev); + + return 0; +} + +/* Enqueue frame for transmittion (padding, crc, etc) */ +static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb) +{ + struct nokia_bt_dev *btdev = hu->priv; + int err; + + BT_DBG("hu %p skb %p", hu, skb); + + /* Prepend skb with frame type */ + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1); + + /* Packets must be word aligned */ + if (skb->len % 2) { + err = skb_pad(skb, 1); + if (err) + return err; + *skb_put(skb, 1) = 0x00; + } + + skb_queue_tail(&btdev->txq, skb); + + return 0; +} + +static int nokia_recv_negotiation_packet(struct hci_dev *hdev, + struct sk_buff *skb) +{ + struct hci_uart *hu = hci_get_drvdata(hdev); + struct nokia_bt_dev *btdev = hu->priv; + struct hci_nokia_neg_hdr *hdr; + struct hci_nokia_neg_evt *evt; + int ret = 0; + + hdr = (struct hci_nokia_neg_hdr *)skb->data; + if (hdr->dlen != sizeof(*evt)) { + btdev->init_error = -EIO; + ret = -EIO; + goto finish_neg; + } + + evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr)); + + if (evt->ack != NOKIA_NEG_ACK) { + dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n"); + btdev->init_error = -EINVAL; + } + + btdev->man_id = evt->man_id; + btdev->ver_id = evt->ver_id; + + dev_dbg(hu->tty->dev, "NOKIA negotiation:\n"); + dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud); + dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk); + dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id); + dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id); + +finish_neg: + complete(&btdev->init_completion); + kfree_skb(skb); + return ret; +} + +static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb) +{ + struct hci_uart *hu = hci_get_drvdata(hdev); + struct nokia_bt_dev *btdev = hu->priv; + struct hci_nokia_alive_hdr *hdr; + struct hci_nokia_alive_pkt *pkt; + int ret = 0; + + hdr = (struct hci_nokia_alive_hdr *)skb->data; + if (hdr->dlen != sizeof(*pkt)) { + dev_err(hu->tty->dev, "Corrupted alive message\n"); + btdev->init_error = -EIO; + ret = -EIO; + goto finish_alive; + } + + pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr)); + + if (pkt->mid != NOKIA_ALIVE_RESP) { + dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n", + pkt->mid); + btdev->init_error = -EINVAL; + goto finish_alive; + } + + dev_dbg(hu->tty->dev, "Received alive packet!\n"); + +finish_alive: + complete(&btdev->init_completion); + kfree_skb(skb); + return ret; +} + +static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb) +{ + /* Packets received on the dedicated radio channel are + * HCI events and so feed them back into the core. + */ + bt_cb(skb)->pkt_type = HCI_EVENT_PKT; + return hci_recv_frame(hdev, skb); +} + +/* Recv data */ +static const struct h4_recv_pkt nokia_recv_pkts[] = { + { NOKIA_RECV_ACL, .recv = hci_recv_frame }, + { NOKIA_RECV_SCO, .recv = hci_recv_frame }, + { NOKIA_RECV_EVENT, .recv = hci_recv_frame }, + { NOKIA_RECV_ALIVE, .recv = nokia_recv_alive_packet }, + { NOKIA_RECV_NEG, .recv = nokia_recv_negotiation_packet }, + { NOKIA_RECV_RADIO, .recv = nokia_recv_radio }, +}; + +static int nokia_recv(struct hci_uart *hu, const void *data, int count) +{ + struct nokia_bt_dev *btdev = hu->priv; + int err; + + if (!test_bit(HCI_UART_REGISTERED, &hu->flags)) + return -EUNATCH; + + btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count, + nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts)); + if (IS_ERR(btdev->rx_skb)) { + err = PTR_ERR(btdev->rx_skb); + BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err); + btdev->rx_skb = NULL; + return err; + } + + return count; +} + +static struct sk_buff *nokia_dequeue(struct hci_uart *hu) +{ + struct nokia_bt_dev *btdev = hu->priv; + + return skb_dequeue(&btdev->txq); +} + +static const struct hci_uart_proto nokia_proto = { + .id = HCI_UART_NOKIA, + .name = "Nokia", + .open = nokia_open, + .close = nokia_close, + .recv = nokia_recv, + .enqueue = nokia_enqueue, + .dequeue = nokia_dequeue, + .flush = nokia_flush, + .setup = nokia_setup, +}; + +static int nokia_bluetooth_probe(struct platform_device *pdev) +{ + struct nokia_uart_dev *btdata; + struct device *bcmdev = &pdev->dev; + struct clk *sysclk; + int err = 0; + + if(!bcmdev->parent) { + dev_err(bcmdev, "parent device missing!\n"); + return -ENODEV; + } + + btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL); + if(!btdata) + return -ENOMEM; + + btdata->dev = bcmdev; + dev_set_drvdata(bcmdev, btdata); + + btdata->port = dev_get_drvdata(bcmdev->parent); + if(!btdata->port) { + dev_err(bcmdev, "port data missing in parent device!\n"); + return -ENODEV; + } + + btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(btdata->reset)) { + err = PTR_ERR(btdata->reset); + dev_err(bcmdev, "could not get reset gpio: %d\n", err); + return err; + } + + btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN); + if (IS_ERR(btdata->wakeup_host)) { + err = PTR_ERR(btdata->wakeup_host); + dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err); + return err; + } + + + btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup", + GPIOD_OUT_LOW); + if (IS_ERR(btdata->wakeup_bt)) { + err = PTR_ERR(btdata->wakeup_bt); + dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err); + return err; + } + + sysclk = devm_clk_get(bcmdev, "sysclk"); + if (IS_ERR(sysclk)) { + err = PTR_ERR(sysclk); + dev_err(bcmdev, "could not get sysclk: %d\n", err); + return err; + } + + clk_prepare_enable(sysclk); + btdata->sysclk_speed = clk_get_rate(sysclk); + clk_disable_unprepare(sysclk); + + dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent)); + dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000); + + /* TODO: open tty and setup line disector from kernel-side */ + + return err; +} + +static const struct of_device_id nokia_bluetooth_of_match[] = { + { .compatible = "nokia,brcm,bcm2048", }, + { .compatible = "nokia,ti,wl1271-bluetooth", }, + {}, +}; +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match); + +static struct platform_driver platform_nokia_driver = { + .driver = { + .name = "nokia-bluetooth", + .of_match_table = nokia_bluetooth_of_match, + }, + .probe = nokia_bluetooth_probe, +}; + +int __init nokia_init(void) +{ + platform_driver_register(&platform_nokia_driver); + return hci_uart_register_proto(&nokia_proto); +} + +int __exit nokia_deinit(void) +{ + platform_driver_unregister(&platform_nokia_driver); + return hci_uart_unregister_proto(&nokia_proto); +} diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h new file mode 100644 index 000000000000..8c4d307840e5 --- /dev/null +++ b/drivers/bluetooth/hci_nokia.h @@ -0,0 +1,140 @@ +/* + * Copyright (C) 2016 Sebastian Reichel <sre@kernel.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __HCI_NOKIA_H +#define __HCI_NOKIA_H + +#define NOKIA_ID_CSR 0x02 +#define NOKIA_ID_BCM2048 0x04 +#define NOKIA_ID_TI1271 0x31 + +#define FIRMWARE_CSR "nokia/bc4fw.bin" +#define FIRMWARE_BCM2048 "nokia/bcmfw.bin" +#define FIRMWARE_TI1271 "nokia/ti1273.bin" + +#define NOKIA_BCM_BDADDR 0xfc01 + +#define HCI_NOKIA_NEG_PKT 0x06 +#define HCI_NOKIA_ALIVE_PKT 0x07 +#define HCI_NOKIA_RADIO_PKT 0x08 + +#define HCI_NOKIA_NEG_HDR_SIZE 1 +#define HCI_NOKIA_MAX_NEG_SIZE 255 +#define HCI_NOKIA_ALIVE_HDR_SIZE 1 +#define HCI_NOKIA_MAX_ALIVE_SIZE 255 +#define HCI_NOKIA_RADIO_HDR_SIZE 2 +#define HCI_NOKIA_MAX_RADIO_SIZE 255 + +#define NOKIA_PROTO_PKT 0x44 +#define NOKIA_PROTO_BYTE 0x4c + +#define NOKIA_NEG_REQ 0x00 +#define NOKIA_NEG_ACK 0x20 +#define NOKIA_NEG_NAK 0x40 + +#define H4_TYPE_SIZE 1 + +#define NOKIA_RECV_ACL \ + H4_RECV_ACL, \ + .wordaligned = true + +#define NOKIA_RECV_SCO \ + H4_RECV_SCO, \ + .wordaligned = true + +#define NOKIA_RECV_EVENT \ + H4_RECV_EVENT, \ + .wordaligned = true + +#define NOKIA_RECV_ALIVE \ + .type = HCI_NOKIA_ALIVE_PKT, \ + .hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \ + .loff = 0, \ + .lsize = 1, \ + .maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \ + .wordaligned = true + +#define NOKIA_RECV_NEG \ + .type = HCI_NOKIA_NEG_PKT, \ + .hlen = HCI_NOKIA_NEG_HDR_SIZE, \ + .loff = 0, \ + .lsize = 1, \ + .maxlen = HCI_NOKIA_MAX_NEG_SIZE, \ + .wordaligned = true + +#define NOKIA_RECV_RADIO \ + .type = HCI_NOKIA_RADIO_PKT, \ + .hlen = HCI_NOKIA_RADIO_HDR_SIZE, \ + .loff = 1, \ + .lsize = 1, \ + .maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \ + .wordaligned = true + +struct hci_nokia_neg_hdr { + __u8 dlen; +} __packed; + +struct hci_nokia_neg_cmd { + __u8 ack; + __u16 baud; + __u16 unused1; + __u8 proto; + __u16 sys_clk; + __u16 unused2; +} __packed; + +static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb) +{ + return (struct hci_nokia_neg_hdr *) skb->data; +} + +#define NOKIA_ALIVE_REQ 0x55 +#define NOKIA_ALIVE_RESP 0xcc + +struct hci_nokia_alive_hdr { + __u8 dlen; +} __packed; + +struct hci_nokia_alive_pkt { + __u8 mid; + __u8 unused; +} __packed; + +static inline struct hci_nokia_alive_hdr *hci_nokia_alive_hdr(const struct sk_buff *skb) +{ + return (struct hci_nokia_alive_hdr *) skb->data; +} + +struct hci_nokia_neg_evt { + __u8 ack; + __u16 baud; + __u16 unused1; + __u8 proto; + __u16 sys_clk; + __u16 unused2; + __u8 man_id; + __u8 ver_id; +} __packed; + +#define BT_BAUDRATE_DIVIDER 384000000 +#define BC4_MAX_BAUD_RATE 3692300 +#define MAX_BAUD_RATE 921600 +#define INIT_SPEED 120000 + +struct hci_nokia_radio_hdr { + __u8 evt; + __u8 dlen; +} __packed; + +#endif diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h index a7d67aec3632..314b243df996 100644 --- a/drivers/bluetooth/hci_uart.h +++ b/drivers/bluetooth/hci_uart.h @@ -35,7 +35,7 @@ #define HCIUARTGETFLAGS _IOR('U', 204, int) /* UART protocols */ -#define HCI_UART_MAX_PROTO 10 +#define HCI_UART_MAX_PROTO 11 #define HCI_UART_H4 0 #define HCI_UART_BCSP 1 @@ -47,6 +47,7 @@ #define HCI_UART_BCM 7 #define HCI_UART_QCA 8 #define HCI_UART_AG6XX 9 +#define HCI_UART_NOKIA 10 #define HCI_UART_RAW_DEVICE 0 #define HCI_UART_RESET_ON_INIT 1 @@ -190,3 +191,8 @@ int qca_deinit(void); int ag6xx_init(void); int ag6xx_deinit(void); #endif + +#ifdef CONFIG_BT_HCIUART_NOKIA +int nokia_init(void); +int nokia_deinit(void); +#endif -- 2.8.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-13 3:14 ` [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel @ 2016-08-14 23:54 ` Paul Gortmaker 2016-08-15 1:12 ` Sebastian Reichel 2016-08-16 7:02 ` Marcel Holtmann 2016-08-16 8:10 ` Marcel Holtmann 2 siblings, 1 reply; 37+ messages in thread From: Paul Gortmaker @ 2016-08-14 23:54 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, LKML On Fri, Aug 12, 2016 at 11:14 PM, Sebastian Reichel <sre@kernel.org> wrote: > This driver adds support for Nokia H4+ procotol used > for example by Nokia's internet tablets (N770 - N950). > --- > drivers/bluetooth/Kconfig | 10 + > drivers/bluetooth/Makefile | 1 + > drivers/bluetooth/hci_ldisc.c | 6 + > drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++ > drivers/bluetooth/hci_nokia.h | 140 ++++++++ > drivers/bluetooth/hci_uart.h | 8 +- > 6 files changed, 898 insertions(+), 1 deletion(-) > create mode 100644 drivers/bluetooth/hci_nokia.c > create mode 100644 drivers/bluetooth/hci_nokia.h > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig > index cf50fd2e96df..c32d9d5ad1d2 100644 > --- a/drivers/bluetooth/Kconfig > +++ b/drivers/bluetooth/Kconfig > @@ -86,6 +86,16 @@ config BT_HCIUART_H4 > > Say Y here to compile support for HCI UART (H4) protocol. > > +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. 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 [...] > + { .compatible = "nokia,ti,wl1271-bluetooth", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match); > + > +static struct platform_driver platform_nokia_driver = { > + .driver = { > + .name = "nokia-bluetooth", > + .of_match_table = nokia_bluetooth_of_match, > + }, > + .probe = nokia_bluetooth_probe, > +}; > + > +int __init nokia_init(void) > +{ > + platform_driver_register(&platform_nokia_driver); > + return hci_uart_register_proto(&nokia_proto); > +} > + > +int __exit nokia_deinit(void) > +{ > + platform_driver_unregister(&platform_nokia_driver); > + return hci_uart_unregister_proto(&nokia_proto); > +} ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-14 23:54 ` Paul Gortmaker @ 2016-08-15 1:12 ` Sebastian Reichel 0 siblings, 0 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-15 1:12 UTC (permalink / raw) To: Paul Gortmaker Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, LKML [-- Attachment #1: Type: text/plain, Size: 779 bytes --] 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 <sre@kernel.org> wrote: > > 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-13 3:14 ` [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel 2016-08-14 23:54 ` Paul Gortmaker @ 2016-08-16 7:02 ` Marcel Holtmann 2016-08-16 7:52 ` Pali Rohár 2016-08-16 9:09 ` Sebastian Reichel 2016-08-16 8:10 ` Marcel Holtmann 2 siblings, 2 replies; 37+ messages in thread From: Marcel Holtmann @ 2016-08-16 7:02 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel Hi Sebastian, > This driver adds support for Nokia H4+ procotol used > for example by Nokia's internet tablets (N770 - N950). > --- > drivers/bluetooth/Kconfig | 10 + > drivers/bluetooth/Makefile | 1 + > drivers/bluetooth/hci_ldisc.c | 6 + > drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++ > drivers/bluetooth/hci_nokia.h | 140 ++++++++ > drivers/bluetooth/hci_uart.h | 8 +- > 6 files changed, 898 insertions(+), 1 deletion(-) > create mode 100644 drivers/bluetooth/hci_nokia.c > create mode 100644 drivers/bluetooth/hci_nokia.h > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig > index cf50fd2e96df..c32d9d5ad1d2 100644 > --- a/drivers/bluetooth/Kconfig > +++ b/drivers/bluetooth/Kconfig > @@ -86,6 +86,16 @@ config BT_HCIUART_H4 > > 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 > diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile > index 9c18939fc5c9..f7951646ee14 100644 > --- a/drivers/bluetooth/Makefile > +++ b/drivers/bluetooth/Makefile > @@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL) += hci_intel.o > hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o > hci_uart-$(CONFIG_BT_HCIUART_QCA) += hci_qca.o > hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o > +hci_uart-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o > hci_uart-objs := $(hci_uart-y) > > ccflags-y += -D__CHECK_ENDIAN__ > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c > index dda97398c59a..83d0de94bf35 100644 > --- a/drivers/bluetooth/hci_ldisc.c > +++ b/drivers/bluetooth/hci_ldisc.c > @@ -810,6 +810,9 @@ static int __init hci_uart_init(void) > #ifdef CONFIG_BT_HCIUART_AG6XX > ag6xx_init(); > #endif > +#ifdef CONFIG_BT_HCIUART_NOKIA > + nokia_init(); > +#endif > > return 0; > } > @@ -845,6 +848,9 @@ static void __exit hci_uart_exit(void) > #ifdef CONFIG_BT_HCIUART_AG6XX > ag6xx_deinit(); > #endif > +#ifdef CONFIG_BT_HCIUART_NOKIA > + nokia_deinit(); > +#endif > > /* Release tty registration of line discipline */ > err = tty_unregister_ldisc(N_HCI); > diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c > new file mode 100644 > index 000000000000..efd4dd320838 > --- /dev/null > +++ b/drivers/bluetooth/hci_nokia.c > @@ -0,0 +1,734 @@ > +/* > + * > + * Bluetooth HCI UART H4 driver with Nokia Extensions > + * > + * Copyright (C) 2015 Marcel Holtmann <marcel@holtmann.org> > + * Copyright (C) 2016 Sebastian Reichel <sre@kernel.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/module.h> > + > +#include <linux/clk.h> > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/types.h> > +#include <linux/fcntl.h> > +#include <linux/interrupt.h> > +#include <linux/ptrace.h> > +#include <linux/poll.h> > +#include <linux/pm_runtime.h> > +#include <linux/firmware.h> > +#include <linux/slab.h> > +#include <linux/tty.h> > +#include <linux/errno.h> > +#include <linux/string.h> > +#include <linux/signal.h> > +#include <linux/ioctl.h> > +#include <linux/skbuff.h> > +#include <linux/delay.h> > +#include <linux/platform_device.h> > + > +#include <linux/gpio/consumer.h> > + > +#include <linux/unaligned/le_struct.h> are you sure all these includes are needed? > +#include <net/bluetooth/bluetooth.h> > +#include <net/bluetooth/hci_core.h> > + > +#include "hci_uart.h" > +#include "hci_nokia.h" > + > +struct nokia_uart_dev { > + struct device *dev; > + struct tty_port *port; > + struct gpio_desc *reset; > + struct gpio_desc *wakeup_host; > + struct gpio_desc *wakeup_bt; > + unsigned long sysclk_speed; > +}; > + > +struct nokia_bt_dev { > + struct hci_uart *hu; > + struct nokia_uart_dev *btdata; > + int wake_irq; > + bool wake_state; > + struct sk_buff *rx_skb; > + struct sk_buff_head txq; > + bdaddr_t bdaddr; > + > + int init_error; > + struct completion init_completion; > + > + uint8_t man_id; > + uint8_t ver_id; > +}; > + > +static char *nokia_get_fw_name(struct nokia_bt_dev *btdev) > +{ > + switch (btdev->man_id) { > + case NOKIA_ID_CSR: > + return FIRMWARE_CSR; > + case NOKIA_ID_BCM2048: > + return FIRMWARE_BCM2048; > + case NOKIA_ID_TI1271: > + return FIRMWARE_TI1271; > + default: > + return NULL; > + } > +} > + > +static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state, > + int timeout_ms) > +{ > + unsigned long timeout; > + int signal; > + > + timeout = jiffies + msecs_to_jiffies(timeout_ms); > + for (;;) { > + signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS; > + if (!!signal == !!state) { > + dev_dbg(hu->tty->dev, "wait for cts... received!\n"); > + return 0; > + } > + if (time_after(jiffies, timeout)) { > + dev_dbg(hu->tty->dev, "wait for cts... timeout!\n"); > + return -ETIMEDOUT; > + } > + usleep_range(1000, 2000); > + } > +} This is a super odd function. No return value since we essentially have an endless loop. > + > +static int btdev_match(struct device *child, void *data) > +{ > + if (!strcmp(child->driver->name, "nokia-bluetooth")) > + return 1; > + else > + return 0; > +} Anything wrong with just return !strcmp? > + > +static irqreturn_t wakeup_handler(int irq, void *data) > +{ > + struct nokia_bt_dev *btdev = data; > + struct device *serialdev = btdev->hu->tty->dev; > + int wake_state = gpiod_get_value(btdev->btdata->wakeup_host); > + > + dev_dbg(serialdev, "wakeup received: %d -> %d\n", > + btdev->wake_state, wake_state); > + > + if (btdev->wake_state == wake_state) > + return IRQ_HANDLED; > + > + if (wake_state) > + pm_runtime_get_sync(serialdev); > + else if (!wake_state) > + pm_runtime_put(serialdev); > + > + btdev->wake_state = wake_state; > + > + return IRQ_HANDLED; > +} > + > +static int nokia_reset(struct hci_uart *hu) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + int err; > + > + /* reset routine */ > + gpiod_set_value_cansleep(btdev->btdata->reset, 0); > + gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1); > + > + msleep(50); > + > + /* safety check */ > + err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host); > + if (err == 1) { > + dev_err(hu->tty->dev, "reset: host wakeup not low!\n"); > + return -EPROTO; > + } > + > + /* flush queues */ > + tty_ldisc_flush(hu->tty); > + tty_driver_flush_buffer(hu->tty); > + > + /* init uart */ > + hci_uart_init_tty(hu); > + hci_uart_set_flow_control(hu, true); > + hci_uart_set_baudrate(hu, INIT_SPEED); > + > + gpiod_set_value_cansleep(btdev->btdata->reset, 1); > + gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 0); > + > + msleep(100); > + > + err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host); > + if (err == 0) { > + dev_err(hu->tty->dev, "reset: host wakeup not high!\n"); > + return -EPROTO; > + } > + > + /* wait for cts */ > + err = hci_uart_wait_for_cts(hu, true, 100); > + if (err < 0) { > + dev_err(hu->tty->dev, "CTS not received: %d\n", err); > + return err; > + } > + > + gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1); > + hci_uart_set_flow_control(hu, false); > + > + return 0; > +} > + > +static int nokia_send_alive_packet(struct hci_uart *hu) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + struct hci_nokia_alive_hdr *hdr; > + struct hci_nokia_alive_pkt *pkt; > + struct sk_buff *skb; > + int len; > + > + dev_dbg(hu->tty->dev, "Sending alive packet...\n"); > + > + init_completion(&btdev->init_completion); > + > + len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt); > + skb = bt_skb_alloc(len, GFP_KERNEL); > + if (!skb) > + return -ENOMEM; > + > + hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT; > + memset(skb->data, 0x00, len); > + > + hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr)); > + hdr->dlen = sizeof(*pkt); > + pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt)); > + pkt->mid = NOKIA_ALIVE_REQ; > + > + hu->hdev->send(hu->hdev, skb); 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. > + > + if (!wait_for_completion_interruptible_timeout(&btdev->init_completion, > + msecs_to_jiffies(1000))) { > + return -ETIMEDOUT; > + } > + > + if (btdev->init_error < 0) > + return btdev->init_error; > + > + return 0; > +} > + > +static int nokia_send_negotiation(struct hci_uart *hu) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + struct hci_nokia_neg_cmd *neg_cmd; > + struct hci_nokia_neg_hdr *neg_hdr; > + struct sk_buff *skb; > + int len, err; > + u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE); > + int sysclk = btdev->btdata->sysclk_speed / 1000; > + > + dev_dbg(hu->tty->dev, "Sending negotiation...\n"); > + > + len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd); > + skb = bt_skb_alloc(len, GFP_KERNEL); > + if (!skb) > + return -ENOMEM; > + > + hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT; > + > + neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr)); > + neg_hdr->dlen = sizeof(*neg_cmd); > + > + neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd)); > + neg_cmd->ack = NOKIA_NEG_REQ; > + neg_cmd->baud = cpu_to_le16(baud); > + neg_cmd->unused1 = 0x0000; > + neg_cmd->proto = NOKIA_PROTO_BYTE; > + neg_cmd->sys_clk = cpu_to_le16(sysclk); > + neg_cmd->unused2 = 0x0000; > + > + btdev->init_error = 0; > + init_completion(&btdev->init_completion); > + > + hu->hdev->send(hu->hdev, skb); > + > + if (!wait_for_completion_interruptible_timeout(&btdev->init_completion, > + msecs_to_jiffies(10000))) { > + return -ETIMEDOUT; > + } > + > + if (btdev->init_error < 0) > + return btdev->init_error; > + > + /* Change to operational settings */ > + hci_uart_set_flow_control(hu, true); // disable flow control Please use a proper comment that explains also disabling flow control. > + > + /* setup negotiated max. baudrate */ > + hci_uart_set_baudrate(hu, MAX_BAUD_RATE); > + > + err = hci_uart_wait_for_cts(hu, true, 100); > + if (err < 0) > + return err; > + > + hci_uart_set_flow_control(hu, false); // re-enable flow control > + > + dev_dbg(hu->tty->dev, "Negotiation successful...\n"); > + > + return 0; > +} > + > +static int nokia_setup_fw(struct hci_uart *hu) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + const struct firmware *fw; > + const u8 *fw_ptr; > + size_t fw_size; > + int err; > + > + BT_DBG("hu %p", hu); > + > + err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev); 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. > + if (err < 0) { > + BT_ERR("%s: Failed to load Nokia firmware file (%d)", > + hu->hdev->name, err); > + return err; > + } > + > + fw_ptr = fw->data; > + fw_size = fw->size; > + > + while (fw_size >= 4) { > + u16 pkt_size = get_unaligned_le16(fw_ptr); > + u8 pkt_type = fw_ptr[2]; > + const struct hci_command_hdr *cmd; > + u16 opcode; > + struct sk_buff *skb; > + > + switch (pkt_type) { > + case HCI_COMMAND_PKT: > + cmd = (struct hci_command_hdr *)(fw_ptr + 3); > + opcode = le16_to_cpu(cmd->opcode); > + > + skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen, > + fw_ptr + 3 + HCI_COMMAND_HDR_SIZE, > + HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + err = PTR_ERR(skb); > + BT_ERR("%s: Firmware command %04x failed (%d)", > + hu->hdev->name, opcode, err); > + goto done; > + } > + kfree_skb(skb); > + break; > + 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. > + 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. > + break; > + } > + > + fw_ptr += pkt_size + 2; > + fw_size -= pkt_size + 2; > + } > + > +done: > + release_firmware(fw); > + return err; > +} > + > +static int nokia_setup(struct hci_uart *hu) > +{ > + int err; > + > + pm_runtime_get_sync(hu->tty->dev); > + > + dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n"); > + > + /* 0. reset connection */ > + err = nokia_reset(hu); > + if (err < 0) { > + dev_err(hu->tty->dev, "Reset failed: %d\n", err); > + goto out; > + } > + > + /* 1. negotiate speed etc */ > + err = nokia_send_negotiation(hu); > + if (err < 0) { > + dev_err(hu->tty->dev, "Negotiation failed: %d\n", err); > + goto out; > + } > + > + /* 2. verify correct setup using alive packet */ > + err = nokia_send_alive_packet(hu); > + if (err < 0) { > + dev_err(hu->tty->dev, "Alive check failed: %d\n", err); > + goto out; > + } > + > + /* 3. send firmware */ > + err = nokia_setup_fw(hu); > + if (err < 0) { > + dev_err(hu->tty->dev, "Could not setup FW: %d\n", err); > + goto out; > + } > + > + hci_uart_set_flow_control(hu, true); > + hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE); I think this variable needs a better name if it is common for all vendors. > + hci_uart_set_flow_control(hu, false); > + > + dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n"); > + > + /* > + * TODO: > + * disable wakeup_bt at this point and automatically enable it when > + * data is about to be written until all data has been written (+ some > + * delay). > + * > + * Since this is not yet support by the uart/tty kernel framework we > + * will always keep enabled the wakeup_bt gpio for now, so that the > + * bluetooth chip will never transit into idle modes. > + */ > + > +out: > + pm_runtime_put(hu->tty->dev); > + > + return err; > +} > + > +static int nokia_open(struct hci_uart *hu) > +{ > + struct device *serialdev = hu->tty->dev; > + struct nokia_bt_dev *btdev; > + struct device *uartbtdev; > + int err; > + > + btdev = kzalloc(sizeof(*btdev), GFP_KERNEL); > + if (!btdev) > + return -ENOMEM; > + > + btdev->hu = hu; > + > + skb_queue_head_init(&btdev->txq); > + > + uartbtdev = device_find_child(serialdev, NULL, btdev_match); > + if (!uartbtdev) { > + dev_err(serialdev, "bluetooth device node not found!\n"); > + return -ENODEV; > + } > + > + btdev->btdata = dev_get_drvdata(uartbtdev); > + if (!btdev->btdata) > + return -EINVAL; > + > + hu->priv = btdev; > + > + /* register handler for host wakeup gpio */ > + btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host); > + err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler, > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "wakeup", btdev); > + if (err) { > + gpiod_set_value(btdev->btdata->reset, 0); > + gpiod_set_value(btdev->btdata->wakeup_bt, 0); > + return err; > + } > + > + dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n", > + dev_name(uartbtdev)); > + > + pm_runtime_enable(hu->tty->dev); > + > + return 0; > +} > + > +static int nokia_flush(struct hci_uart *hu) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + > + BT_DBG("hu %p", hu); > + > + skb_queue_purge(&btdev->txq); > + > + return 0; > +} > + > +static int nokia_close(struct hci_uart *hu) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + > + hu->priv = NULL; > + > + BT_DBG("hu %p", hu); > + > + skb_queue_purge(&btdev->txq); > + > + kfree_skb(btdev->rx_skb); > + > + free_irq(btdev->wake_irq, btdev); > + > + /* disable module */ > + gpiod_set_value(btdev->btdata->reset, 0); > + gpiod_set_value(btdev->btdata->wakeup_bt, 0); > + > + hu->priv = NULL; > + kfree(btdev); > + > + pm_runtime_disable(hu->tty->dev); > + > + return 0; > +} > + > +/* Enqueue frame for transmittion (padding, crc, etc) */ > +static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + int err; > + > + BT_DBG("hu %p skb %p", hu, skb); > + > + /* Prepend skb with frame type */ > + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1); > + > + /* Packets must be word aligned */ > + if (skb->len % 2) { > + err = skb_pad(skb, 1); > + if (err) > + return err; > + *skb_put(skb, 1) = 0x00; > + } > + > + skb_queue_tail(&btdev->txq, skb); > + > + return 0; > +} > + > +static int nokia_recv_negotiation_packet(struct hci_dev *hdev, > + struct sk_buff *skb) > +{ > + struct hci_uart *hu = hci_get_drvdata(hdev); > + struct nokia_bt_dev *btdev = hu->priv; > + struct hci_nokia_neg_hdr *hdr; > + struct hci_nokia_neg_evt *evt; > + int ret = 0; > + > + hdr = (struct hci_nokia_neg_hdr *)skb->data; > + if (hdr->dlen != sizeof(*evt)) { > + btdev->init_error = -EIO; > + ret = -EIO; > + goto finish_neg; > + } > + > + evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr)); > + > + if (evt->ack != NOKIA_NEG_ACK) { > + dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n"); > + btdev->init_error = -EINVAL; > + } > + > + btdev->man_id = evt->man_id; > + btdev->ver_id = evt->ver_id; > + > + dev_dbg(hu->tty->dev, "NOKIA negotiation:\n"); > + dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud); > + dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk); > + dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id); > + dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id); > + > +finish_neg: > + complete(&btdev->init_completion); > + kfree_skb(skb); > + return ret; > +} > + > +static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb) > +{ > + struct hci_uart *hu = hci_get_drvdata(hdev); > + struct nokia_bt_dev *btdev = hu->priv; > + struct hci_nokia_alive_hdr *hdr; > + struct hci_nokia_alive_pkt *pkt; > + int ret = 0; > + > + hdr = (struct hci_nokia_alive_hdr *)skb->data; > + if (hdr->dlen != sizeof(*pkt)) { > + dev_err(hu->tty->dev, "Corrupted alive message\n"); > + btdev->init_error = -EIO; > + ret = -EIO; > + goto finish_alive; > + } > + > + pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr)); > + > + if (pkt->mid != NOKIA_ALIVE_RESP) { > + dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n", > + pkt->mid); > + btdev->init_error = -EINVAL; > + goto finish_alive; > + } > + > + dev_dbg(hu->tty->dev, "Received alive packet!\n"); > + > +finish_alive: > + complete(&btdev->init_completion); > + kfree_skb(skb); > + return ret; > +} > + > +static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb) > +{ > + /* Packets received on the dedicated radio channel are > + * HCI events and so feed them back into the core. > + */ > + bt_cb(skb)->pkt_type = HCI_EVENT_PKT; I think using hci_skb_pkt_type(skb) is correct here as well. > + return hci_recv_frame(hdev, skb); > +} > + > +/* Recv data */ > +static const struct h4_recv_pkt nokia_recv_pkts[] = { > + { NOKIA_RECV_ACL, .recv = hci_recv_frame }, > + { NOKIA_RECV_SCO, .recv = hci_recv_frame }, > + { NOKIA_RECV_EVENT, .recv = hci_recv_frame }, > + { NOKIA_RECV_ALIVE, .recv = nokia_recv_alive_packet }, > + { NOKIA_RECV_NEG, .recv = nokia_recv_negotiation_packet }, > + { NOKIA_RECV_RADIO, .recv = nokia_recv_radio }, > +}; > + > +static int nokia_recv(struct hci_uart *hu, const void *data, int count) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + int err; > + > + if (!test_bit(HCI_UART_REGISTERED, &hu->flags)) > + return -EUNATCH; > + > + btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count, > + nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts)); > + if (IS_ERR(btdev->rx_skb)) { > + err = PTR_ERR(btdev->rx_skb); > + BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err); > + btdev->rx_skb = NULL; > + return err; > + } > + > + return count; > +} > + > +static struct sk_buff *nokia_dequeue(struct hci_uart *hu) > +{ > + struct nokia_bt_dev *btdev = hu->priv; > + > + return skb_dequeue(&btdev->txq); > +} > + > +static const struct hci_uart_proto nokia_proto = { > + .id = HCI_UART_NOKIA, > + .name = "Nokia", > + .open = nokia_open, > + .close = nokia_close, > + .recv = nokia_recv, > + .enqueue = nokia_enqueue, > + .dequeue = nokia_dequeue, > + .flush = nokia_flush, > + .setup = nokia_setup, > +}; > + > +static int nokia_bluetooth_probe(struct platform_device *pdev) > +{ > + struct nokia_uart_dev *btdata; > + struct device *bcmdev = &pdev->dev; > + struct clk *sysclk; > + int err = 0; > + > + if(!bcmdev->parent) { > + dev_err(bcmdev, "parent device missing!\n"); > + return -ENODEV; > + } > + > + btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL); > + if(!btdata) > + return -ENOMEM; > + > + btdata->dev = bcmdev; > + dev_set_drvdata(bcmdev, btdata); > + > + btdata->port = dev_get_drvdata(bcmdev->parent); > + if(!btdata->port) { > + dev_err(bcmdev, "port data missing in parent device!\n"); > + return -ENODEV; > + } > + > + btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(btdata->reset)) { > + err = PTR_ERR(btdata->reset); > + dev_err(bcmdev, "could not get reset gpio: %d\n", err); > + return err; > + } > + > + btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN); > + if (IS_ERR(btdata->wakeup_host)) { > + err = PTR_ERR(btdata->wakeup_host); > + dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err); > + return err; > + } > + > + > + btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup", > + GPIOD_OUT_LOW); > + if (IS_ERR(btdata->wakeup_bt)) { > + err = PTR_ERR(btdata->wakeup_bt); > + dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err); > + return err; > + } > + > + sysclk = devm_clk_get(bcmdev, "sysclk"); > + if (IS_ERR(sysclk)) { > + err = PTR_ERR(sysclk); > + dev_err(bcmdev, "could not get sysclk: %d\n", err); > + return err; > + } > + > + clk_prepare_enable(sysclk); > + btdata->sysclk_speed = clk_get_rate(sysclk); > + clk_disable_unprepare(sysclk); > + > + dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent)); > + dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000); > + > + /* TODO: open tty and setup line disector from kernel-side */ > + > + return err; > +} > + > +static const struct of_device_id nokia_bluetooth_of_match[] = { > + { .compatible = "nokia,brcm,bcm2048", }, > + { .compatible = "nokia,ti,wl1271-bluetooth", }, 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. > + {}, > +}; > +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match); > + > +static struct platform_driver platform_nokia_driver = { > + .driver = { > + .name = "nokia-bluetooth", > + .of_match_table = nokia_bluetooth_of_match, > + }, > + .probe = nokia_bluetooth_probe, > +}; > + > +int __init nokia_init(void) > +{ > + platform_driver_register(&platform_nokia_driver); > + return hci_uart_register_proto(&nokia_proto); > +} > + > +int __exit nokia_deinit(void) > +{ > + platform_driver_unregister(&platform_nokia_driver); > + return hci_uart_unregister_proto(&nokia_proto); > +} > diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h > new file mode 100644 > index 000000000000..8c4d307840e5 > --- /dev/null > +++ b/drivers/bluetooth/hci_nokia.h > @@ -0,0 +1,140 @@ > +/* > + * Copyright (C) 2016 Sebastian Reichel <sre@kernel.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef __HCI_NOKIA_H > +#define __HCI_NOKIA_H Lets not do a separate header here. Just move this all into hci_nokia.c. There is really zero benefit in the header. > + > +#define NOKIA_ID_CSR 0x02 > +#define NOKIA_ID_BCM2048 0x04 > +#define NOKIA_ID_TI1271 0x31 > + > +#define FIRMWARE_CSR "nokia/bc4fw.bin" If the CSR ones are not yet supported, then leave them out for now. We can add this later. > +#define FIRMWARE_BCM2048 "nokia/bcmfw.bin" > +#define FIRMWARE_TI1271 "nokia/ti1273.bin" > + > +#define NOKIA_BCM_BDADDR 0xfc01 We have btbcm.[ch] for this. > + > +#define HCI_NOKIA_NEG_PKT 0x06 > +#define HCI_NOKIA_ALIVE_PKT 0x07 > +#define HCI_NOKIA_RADIO_PKT 0x08 > + > +#define HCI_NOKIA_NEG_HDR_SIZE 1 > +#define HCI_NOKIA_MAX_NEG_SIZE 255 > +#define HCI_NOKIA_ALIVE_HDR_SIZE 1 > +#define HCI_NOKIA_MAX_ALIVE_SIZE 255 > +#define HCI_NOKIA_RADIO_HDR_SIZE 2 > +#define HCI_NOKIA_MAX_RADIO_SIZE 255 > + > +#define NOKIA_PROTO_PKT 0x44 > +#define NOKIA_PROTO_BYTE 0x4c > + > +#define NOKIA_NEG_REQ 0x00 > +#define NOKIA_NEG_ACK 0x20 > +#define NOKIA_NEG_NAK 0x40 > + > +#define H4_TYPE_SIZE 1 I am not sure this define adds any overall value to the code. > + > +#define NOKIA_RECV_ACL \ > + H4_RECV_ACL, \ > + .wordaligned = true > + > +#define NOKIA_RECV_SCO \ > + H4_RECV_SCO, \ > + .wordaligned = true > + > +#define NOKIA_RECV_EVENT \ > + H4_RECV_EVENT, \ > + .wordaligned = true > + > +#define NOKIA_RECV_ALIVE \ > + .type = HCI_NOKIA_ALIVE_PKT, \ > + .hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \ > + .loff = 0, \ > + .lsize = 1, \ > + .maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \ > + .wordaligned = true > + > +#define NOKIA_RECV_NEG \ > + .type = HCI_NOKIA_NEG_PKT, \ > + .hlen = HCI_NOKIA_NEG_HDR_SIZE, \ > + .loff = 0, \ > + .lsize = 1, \ > + .maxlen = HCI_NOKIA_MAX_NEG_SIZE, \ > + .wordaligned = true > + > +#define NOKIA_RECV_RADIO \ > + .type = HCI_NOKIA_RADIO_PKT, \ > + .hlen = HCI_NOKIA_RADIO_HDR_SIZE, \ > + .loff = 1, \ > + .lsize = 1, \ > + .maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \ > + .wordaligned = true 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. > + > +struct hci_nokia_neg_hdr { > + __u8 dlen; > +} __packed; > + > +struct hci_nokia_neg_cmd { > + __u8 ack; > + __u16 baud; > + __u16 unused1; > + __u8 proto; > + __u16 sys_clk; > + __u16 unused2; > +} __packed; > + > +static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb) > +{ > + return (struct hci_nokia_neg_hdr *) skb->data; > +} What good is this inline? A define would be way better, if really needed. > + > +#define NOKIA_ALIVE_REQ 0x55 > +#define NOKIA_ALIVE_RESP 0xcc > + > +struct hci_nokia_alive_hdr { > + __u8 dlen; > +} __packed; > + > +struct hci_nokia_alive_pkt { > + __u8 mid; > + __u8 unused; > +} __packed; > + > +static inline struct hci_nokia_alive_hdr *hci_nokia_alive_hdr(const struct sk_buff *skb) > +{ > + return (struct hci_nokia_alive_hdr *) skb->data; > +} > + > +struct hci_nokia_neg_evt { > + __u8 ack; > + __u16 baud; > + __u16 unused1; > + __u8 proto; > + __u16 sys_clk; > + __u16 unused2; > + __u8 man_id; > + __u8 ver_id; > +} __packed; > + > +#define BT_BAUDRATE_DIVIDER 384000000 > +#define BC4_MAX_BAUD_RATE 3692300 > +#define MAX_BAUD_RATE 921600 > +#define INIT_SPEED 120000 > + > +struct hci_nokia_radio_hdr { > + __u8 evt; > + __u8 dlen; > +} __packed; > + > +#endif > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h > index a7d67aec3632..314b243df996 100644 > --- a/drivers/bluetooth/hci_uart.h > +++ b/drivers/bluetooth/hci_uart.h > @@ -35,7 +35,7 @@ > #define HCIUARTGETFLAGS _IOR('U', 204, int) > > /* UART protocols */ > -#define HCI_UART_MAX_PROTO 10 > +#define HCI_UART_MAX_PROTO 11 > > #define HCI_UART_H4 0 > #define HCI_UART_BCSP 1 > @@ -47,6 +47,7 @@ > #define HCI_UART_BCM 7 > #define HCI_UART_QCA 8 > #define HCI_UART_AG6XX 9 > +#define HCI_UART_NOKIA 10 > > #define HCI_UART_RAW_DEVICE 0 > #define HCI_UART_RESET_ON_INIT 1 > @@ -190,3 +191,8 @@ int qca_deinit(void); > int ag6xx_init(void); > int ag6xx_deinit(void); > #endif > + > +#ifdef CONFIG_BT_HCIUART_NOKIA > +int nokia_init(void); > +int nokia_deinit(void); > +#endif Regards Marcel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-16 7:02 ` Marcel Holtmann @ 2016-08-16 7:52 ` Pali Rohár 2016-08-16 9:25 ` Sebastian Reichel 2016-08-16 9:09 ` Sebastian Reichel 1 sibling, 1 reply; 37+ messages in thread From: Pali Rohár @ 2016-08-16 7:52 UTC (permalink / raw) To: Marcel Holtmann Cc: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel On Tuesday 16 August 2016 09:02:14 Marcel Holtmann wrote: > > +static int nokia_setup_fw(struct hci_uart *hu) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + const struct firmware *fw; > > + const u8 *fw_ptr; > > + size_t fw_size; > > + int err; > > + > > + BT_DBG("hu %p", hu); > > + > > + err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev); > > 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. > > > + if (err < 0) { > > + BT_ERR("%s: Failed to load Nokia firmware file (%d)", > > + hu->hdev->name, err); > > + return err; > > + } > > + > > + fw_ptr = fw->data; > > + fw_size = fw->size; > > + > > + while (fw_size >= 4) { > > + u16 pkt_size = get_unaligned_le16(fw_ptr); > > + u8 pkt_type = fw_ptr[2]; > > + const struct hci_command_hdr *cmd; > > + u16 opcode; > > + struct sk_buff *skb; > > + > > + switch (pkt_type) { > > + case HCI_COMMAND_PKT: > > + cmd = (struct hci_command_hdr *)(fw_ptr + 3); > > + opcode = le16_to_cpu(cmd->opcode); > > + > > + skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen, > > + fw_ptr + 3 + HCI_COMMAND_HDR_SIZE, > > + HCI_INIT_TIMEOUT); > > + if (IS_ERR(skb)) { > > + err = PTR_ERR(skb); > > + BT_ERR("%s: Firmware command %04x failed (%d)", > > + hu->hdev->name, opcode, err); > > + goto done; > > + } > > + kfree_skb(skb); > > + break; > > + 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. > > > + 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-16 7:52 ` Pali Rohár @ 2016-08-16 9:25 ` Sebastian Reichel 0 siblings, 0 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-16 9:25 UTC (permalink / raw) To: Pali Rohár Cc: Marcel Holtmann, Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2233 bytes --] Hi, On Tue, Aug 16, 2016 at 09:52:17AM +0200, Pali Rohár wrote: > > > + 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. > > > > > + 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-16 7:02 ` Marcel Holtmann 2016-08-16 7:52 ` Pali Rohár @ 2016-08-16 9:09 ` Sebastian Reichel 2016-08-16 10:23 ` Marcel Holtmann 2016-08-16 10:23 ` Marcel Holtmann 1 sibling, 2 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-16 9:09 UTC (permalink / raw) To: Marcel Holtmann Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 25933 bytes --] Hi Marcel, On Tue, Aug 16, 2016 at 09:02:14AM +0200, Marcel Holtmann wrote: > [...] > > +#include <linux/module.h> > > + > > +#include <linux/clk.h> > > +#include <linux/kernel.h> > > +#include <linux/init.h> > > +#include <linux/types.h> > > +#include <linux/fcntl.h> > > +#include <linux/interrupt.h> > > +#include <linux/ptrace.h> > > +#include <linux/poll.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/firmware.h> > > +#include <linux/slab.h> > > +#include <linux/tty.h> > > +#include <linux/errno.h> > > +#include <linux/string.h> > > +#include <linux/signal.h> > > +#include <linux/ioctl.h> > > +#include <linux/skbuff.h> > > +#include <linux/delay.h> > > +#include <linux/platform_device.h> > > + > > +#include <linux/gpio/consumer.h> > > + > > +#include <linux/unaligned/le_struct.h> > > are you sure all these includes are needed? No. A few of them are from previous version of the driver. I will clean this up in the next version. > [...] > > +static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state, > > + int timeout_ms) > > +{ > > + unsigned long timeout; > > + int signal; > > + > > + timeout = jiffies + msecs_to_jiffies(timeout_ms); > > + for (;;) { > > + signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS; > > + if (!!signal == !!state) { > > + dev_dbg(hu->tty->dev, "wait for cts... received!\n"); > > + return 0; > > + } > > + if (time_after(jiffies, timeout)) { > > + dev_dbg(hu->tty->dev, "wait for cts... timeout!\n"); > > + return -ETIMEDOUT; > > + } > > + usleep_range(1000, 2000); > > + } > > +} > > This is a super odd function. No return value since we essentially > have an endless loop. I will rewrite it, so that the loop condition checks for timeout. > > + > > +static int btdev_match(struct device *child, void *data) > > +{ > > + if (!strcmp(child->driver->name, "nokia-bluetooth")) > > + return 1; > > + else > > + return 0; > > +} > > Anything wrong with just return !strcmp? No. I had initially debug prints in the cases. > > +static int nokia_send_alive_packet(struct hci_uart *hu) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + struct hci_nokia_alive_hdr *hdr; > > + struct hci_nokia_alive_pkt *pkt; > > + struct sk_buff *skb; > > + int len; > > + > > + dev_dbg(hu->tty->dev, "Sending alive packet...\n"); > > + > > + init_completion(&btdev->init_completion); > > + > > + len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt); > > + skb = bt_skb_alloc(len, GFP_KERNEL); > > + if (!skb) > > + return -ENOMEM; > > + > > + hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT; > > + memset(skb->data, 0x00, len); > > + > > + hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr)); > > + hdr->dlen = sizeof(*pkt); > > + pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt)); > > + pkt->mid = NOKIA_ALIVE_REQ; > > + > > + hu->hdev->send(hu->hdev, skb); > > 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. ok. I will directly call nokia_enqueue(). > > + > > + if (!wait_for_completion_interruptible_timeout(&btdev->init_completion, > > + msecs_to_jiffies(1000))) { > > + return -ETIMEDOUT; > > + } > > + > > + if (btdev->init_error < 0) > > + return btdev->init_error; > > + > > + return 0; > > +} > > + > > +static int nokia_send_negotiation(struct hci_uart *hu) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + struct hci_nokia_neg_cmd *neg_cmd; > > + struct hci_nokia_neg_hdr *neg_hdr; > > + struct sk_buff *skb; > > + int len, err; > > + u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE); > > + int sysclk = btdev->btdata->sysclk_speed / 1000; > > + > > + dev_dbg(hu->tty->dev, "Sending negotiation...\n"); > > + > > + len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd); > > + skb = bt_skb_alloc(len, GFP_KERNEL); > > + if (!skb) > > + return -ENOMEM; > > + > > + hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT; > > + > > + neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr)); > > + neg_hdr->dlen = sizeof(*neg_cmd); > > + > > + neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd)); > > + neg_cmd->ack = NOKIA_NEG_REQ; > > + neg_cmd->baud = cpu_to_le16(baud); > > + neg_cmd->unused1 = 0x0000; > > + neg_cmd->proto = NOKIA_PROTO_BYTE; > > + neg_cmd->sys_clk = cpu_to_le16(sysclk); > > + neg_cmd->unused2 = 0x0000; > > + > > + btdev->init_error = 0; > > + init_completion(&btdev->init_completion); > > + > > + hu->hdev->send(hu->hdev, skb); > > + > > + if (!wait_for_completion_interruptible_timeout(&btdev->init_completion, > > + msecs_to_jiffies(10000))) { > > + return -ETIMEDOUT; > > + } > > + > > + if (btdev->init_error < 0) > > + return btdev->init_error; > > + > > + /* Change to operational settings */ > > + hci_uart_set_flow_control(hu, true); // disable flow control > > Please use a proper comment that explains also > disabling flow control. ok. > > + > > + /* setup negotiated max. baudrate */ > > + hci_uart_set_baudrate(hu, MAX_BAUD_RATE); > > + > > + err = hci_uart_wait_for_cts(hu, true, 100); > > + if (err < 0) > > + return err; > > + > > + hci_uart_set_flow_control(hu, false); // re-enable flow control > > + > > + dev_dbg(hu->tty->dev, "Negotiation successful...\n"); > > + > > + return 0; > > +} > > + > > +static int nokia_setup_fw(struct hci_uart *hu) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + const struct firmware *fw; > > + const u8 *fw_ptr; > > + size_t fw_size; > > + int err; > > + > > + BT_DBG("hu %p", hu); > > + > > + err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev); > > 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. I inlined it and dropped CSR support. > > + if (err < 0) { > > + BT_ERR("%s: Failed to load Nokia firmware file (%d)", > > + hu->hdev->name, err); > > + return err; > > + } > > + > > + fw_ptr = fw->data; > > + fw_size = fw->size; > > + > > + while (fw_size >= 4) { > > + u16 pkt_size = get_unaligned_le16(fw_ptr); > > + u8 pkt_type = fw_ptr[2]; > > + const struct hci_command_hdr *cmd; > > + u16 opcode; > > + struct sk_buff *skb; > > + > > + switch (pkt_type) { > > + case HCI_COMMAND_PKT: > > + cmd = (struct hci_command_hdr *)(fw_ptr + 3); > > + opcode = le16_to_cpu(cmd->opcode); > > + > > + skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen, > > + fw_ptr + 3 + HCI_COMMAND_HDR_SIZE, > > + HCI_INIT_TIMEOUT); > > + if (IS_ERR(skb)) { > > + err = PTR_ERR(skb); > > + BT_ERR("%s: Firmware command %04x failed (%d)", > > + hu->hdev->name, opcode, err); > > + goto done; > > + } > > + kfree_skb(skb); > > + break; > > + 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. 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. > > + 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. > > + break; > > + } > > + > > + fw_ptr += pkt_size + 2; > > + fw_size -= pkt_size + 2; > > + } > > + > > +done: > > + release_firmware(fw); > > + return err; > > +} > > + > > +static int nokia_setup(struct hci_uart *hu) > > +{ > > + int err; > > + > > + pm_runtime_get_sync(hu->tty->dev); > > + > > + dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n"); > > + > > + /* 0. reset connection */ > > + err = nokia_reset(hu); > > + if (err < 0) { > > + dev_err(hu->tty->dev, "Reset failed: %d\n", err); > > + goto out; > > + } > > + > > + /* 1. negotiate speed etc */ > > + err = nokia_send_negotiation(hu); > > + if (err < 0) { > > + dev_err(hu->tty->dev, "Negotiation failed: %d\n", err); > > + goto out; > > + } > > + > > + /* 2. verify correct setup using alive packet */ > > + err = nokia_send_alive_packet(hu); > > + if (err < 0) { > > + dev_err(hu->tty->dev, "Alive check failed: %d\n", err); > > + goto out; > > + } > > + > > + /* 3. send firmware */ > > + err = nokia_setup_fw(hu); > > + if (err < 0) { > > + dev_err(hu->tty->dev, "Could not setup FW: %d\n", err); > > + goto out; > > + } > > + > > + hci_uart_set_flow_control(hu, true); > > + hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE); > > I think this variable needs a better name if > it is common for all vendors. It is common. I will rename it to MAX_BAUD_RATE and old MAX_BAUD_RATE to SETUP_BAUD_RATE. > > + hci_uart_set_flow_control(hu, false); > > + > > + dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n"); > > + > > + /* > > + * TODO: > > + * disable wakeup_bt at this point and automatically enable it when > > + * data is about to be written until all data has been written (+ some > > + * delay). > > + * > > + * Since this is not yet support by the uart/tty kernel framework we > > + * will always keep enabled the wakeup_bt gpio for now, so that the > > + * bluetooth chip will never transit into idle modes. > > + */ > > + > > +out: > > + pm_runtime_put(hu->tty->dev); > > + > > + return err; > > +} > > + > > +static int nokia_open(struct hci_uart *hu) > > +{ > > + struct device *serialdev = hu->tty->dev; > > + struct nokia_bt_dev *btdev; > > + struct device *uartbtdev; > > + int err; > > + > > + btdev = kzalloc(sizeof(*btdev), GFP_KERNEL); > > + if (!btdev) > > + return -ENOMEM; > > + > > + btdev->hu = hu; > > + > > + skb_queue_head_init(&btdev->txq); > > + > > + uartbtdev = device_find_child(serialdev, NULL, btdev_match); > > + if (!uartbtdev) { > > + dev_err(serialdev, "bluetooth device node not found!\n"); > > + return -ENODEV; > > + } > > + > > + btdev->btdata = dev_get_drvdata(uartbtdev); > > + if (!btdev->btdata) > > + return -EINVAL; > > + > > + hu->priv = btdev; > > + > > + /* register handler for host wakeup gpio */ > > + btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host); > > + err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler, > > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > + "wakeup", btdev); > > + if (err) { > > + gpiod_set_value(btdev->btdata->reset, 0); > > + gpiod_set_value(btdev->btdata->wakeup_bt, 0); > > + return err; > > + } > > + > > + dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n", > > + dev_name(uartbtdev)); > > + > > + pm_runtime_enable(hu->tty->dev); > > + > > + return 0; > > +} > > + > > +static int nokia_flush(struct hci_uart *hu) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + > > + BT_DBG("hu %p", hu); > > + > > + skb_queue_purge(&btdev->txq); > > + > > + return 0; > > +} > > + > > +static int nokia_close(struct hci_uart *hu) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + > > + hu->priv = NULL; > > + > > + BT_DBG("hu %p", hu); > > + > > + skb_queue_purge(&btdev->txq); > > + > > + kfree_skb(btdev->rx_skb); > > + > > + free_irq(btdev->wake_irq, btdev); > > + > > + /* disable module */ > > + gpiod_set_value(btdev->btdata->reset, 0); > > + gpiod_set_value(btdev->btdata->wakeup_bt, 0); > > + > > + hu->priv = NULL; > > + kfree(btdev); > > + > > + pm_runtime_disable(hu->tty->dev); > > + > > + return 0; > > +} > > + > > +/* Enqueue frame for transmittion (padding, crc, etc) */ > > +static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + int err; > > + > > + BT_DBG("hu %p skb %p", hu, skb); > > + > > + /* Prepend skb with frame type */ > > + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1); > > + > > + /* Packets must be word aligned */ > > + if (skb->len % 2) { > > + err = skb_pad(skb, 1); > > + if (err) > > + return err; > > + *skb_put(skb, 1) = 0x00; > > + } > > + > > + skb_queue_tail(&btdev->txq, skb); > > + > > + return 0; > > +} > > + > > +static int nokia_recv_negotiation_packet(struct hci_dev *hdev, > > + struct sk_buff *skb) > > +{ > > + struct hci_uart *hu = hci_get_drvdata(hdev); > > + struct nokia_bt_dev *btdev = hu->priv; > > + struct hci_nokia_neg_hdr *hdr; > > + struct hci_nokia_neg_evt *evt; > > + int ret = 0; > > + > > + hdr = (struct hci_nokia_neg_hdr *)skb->data; > > + if (hdr->dlen != sizeof(*evt)) { > > + btdev->init_error = -EIO; > > + ret = -EIO; > > + goto finish_neg; > > + } > > + > > + evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr)); > > + > > + if (evt->ack != NOKIA_NEG_ACK) { > > + dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n"); > > + btdev->init_error = -EINVAL; > > + } > > + > > + btdev->man_id = evt->man_id; > > + btdev->ver_id = evt->ver_id; > > + > > + dev_dbg(hu->tty->dev, "NOKIA negotiation:\n"); > > + dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud); > > + dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk); > > + dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id); > > + dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id); > > + > > +finish_neg: > > + complete(&btdev->init_completion); > > + kfree_skb(skb); > > + return ret; > > +} > > + > > +static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb) > > +{ > > + struct hci_uart *hu = hci_get_drvdata(hdev); > > + struct nokia_bt_dev *btdev = hu->priv; > > + struct hci_nokia_alive_hdr *hdr; > > + struct hci_nokia_alive_pkt *pkt; > > + int ret = 0; > > + > > + hdr = (struct hci_nokia_alive_hdr *)skb->data; > > + if (hdr->dlen != sizeof(*pkt)) { > > + dev_err(hu->tty->dev, "Corrupted alive message\n"); > > + btdev->init_error = -EIO; > > + ret = -EIO; > > + goto finish_alive; > > + } > > + > > + pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr)); > > + > > + if (pkt->mid != NOKIA_ALIVE_RESP) { > > + dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n", > > + pkt->mid); > > + btdev->init_error = -EINVAL; > > + goto finish_alive; > > + } > > + > > + dev_dbg(hu->tty->dev, "Received alive packet!\n"); > > + > > +finish_alive: > > + complete(&btdev->init_completion); > > + kfree_skb(skb); > > + return ret; > > +} > > + > > +static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb) > > +{ > > + /* Packets received on the dedicated radio channel are > > + * HCI events and so feed them back into the core. > > + */ > > + bt_cb(skb)->pkt_type = HCI_EVENT_PKT; > > I think using hci_skb_pkt_type(skb) is correct here as well. ok. > > + return hci_recv_frame(hdev, skb); > > +} > > + > > +/* Recv data */ > > +static const struct h4_recv_pkt nokia_recv_pkts[] = { > > + { NOKIA_RECV_ACL, .recv = hci_recv_frame }, > > + { NOKIA_RECV_SCO, .recv = hci_recv_frame }, > > + { NOKIA_RECV_EVENT, .recv = hci_recv_frame }, > > + { NOKIA_RECV_ALIVE, .recv = nokia_recv_alive_packet }, > > + { NOKIA_RECV_NEG, .recv = nokia_recv_negotiation_packet }, > > + { NOKIA_RECV_RADIO, .recv = nokia_recv_radio }, > > +}; > > + > > +static int nokia_recv(struct hci_uart *hu, const void *data, int count) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + int err; > > + > > + if (!test_bit(HCI_UART_REGISTERED, &hu->flags)) > > + return -EUNATCH; > > + > > + btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count, > > + nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts)); > > + if (IS_ERR(btdev->rx_skb)) { > > + err = PTR_ERR(btdev->rx_skb); > > + BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err); > > + btdev->rx_skb = NULL; > > + return err; > > + } > > + > > + return count; > > +} > > + > > +static struct sk_buff *nokia_dequeue(struct hci_uart *hu) > > +{ > > + struct nokia_bt_dev *btdev = hu->priv; > > + > > + return skb_dequeue(&btdev->txq); > > +} > > + > > +static const struct hci_uart_proto nokia_proto = { > > + .id = HCI_UART_NOKIA, > > + .name = "Nokia", > > + .open = nokia_open, > > + .close = nokia_close, > > + .recv = nokia_recv, > > + .enqueue = nokia_enqueue, > > + .dequeue = nokia_dequeue, > > + .flush = nokia_flush, > > + .setup = nokia_setup, > > +}; > > + > > +static int nokia_bluetooth_probe(struct platform_device *pdev) > > +{ > > + struct nokia_uart_dev *btdata; > > + struct device *bcmdev = &pdev->dev; > > + struct clk *sysclk; > > + int err = 0; > > + > > + if(!bcmdev->parent) { > > + dev_err(bcmdev, "parent device missing!\n"); > > + return -ENODEV; > > + } > > + > > + btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL); > > + if(!btdata) > > + return -ENOMEM; > > + > > + btdata->dev = bcmdev; > > + dev_set_drvdata(bcmdev, btdata); > > + > > + btdata->port = dev_get_drvdata(bcmdev->parent); > > + if(!btdata->port) { > > + dev_err(bcmdev, "port data missing in parent device!\n"); > > + return -ENODEV; > > + } > > + > > + btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(btdata->reset)) { > > + err = PTR_ERR(btdata->reset); > > + dev_err(bcmdev, "could not get reset gpio: %d\n", err); > > + return err; > > + } > > + > > + btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN); > > + if (IS_ERR(btdata->wakeup_host)) { > > + err = PTR_ERR(btdata->wakeup_host); > > + dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err); > > + return err; > > + } > > + > > + > > + btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(btdata->wakeup_bt)) { > > + err = PTR_ERR(btdata->wakeup_bt); > > + dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err); > > + return err; > > + } > > + > > + sysclk = devm_clk_get(bcmdev, "sysclk"); > > + if (IS_ERR(sysclk)) { > > + err = PTR_ERR(sysclk); > > + dev_err(bcmdev, "could not get sysclk: %d\n", err); > > + return err; > > + } > > + > > + clk_prepare_enable(sysclk); > > + btdata->sysclk_speed = clk_get_rate(sysclk); > > + clk_disable_unprepare(sysclk); > > + > > + dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent)); > > + dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000); > > + > > + /* TODO: open tty and setup line disector from kernel-side */ > > + > > + return err; > > +} > > + > > +static const struct of_device_id nokia_bluetooth_of_match[] = { > > + { .compatible = "nokia,brcm,bcm2048", }, > > + { .compatible = "nokia,ti,wl1271-bluetooth", }, > > 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. > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match); > > + > > +static struct platform_driver platform_nokia_driver = { > > + .driver = { > > + .name = "nokia-bluetooth", > > + .of_match_table = nokia_bluetooth_of_match, > > + }, > > + .probe = nokia_bluetooth_probe, > > +}; > > + > > +int __init nokia_init(void) > > +{ > > + platform_driver_register(&platform_nokia_driver); > > + return hci_uart_register_proto(&nokia_proto); > > +} > > + > > +int __exit nokia_deinit(void) > > +{ > > + platform_driver_unregister(&platform_nokia_driver); > > + return hci_uart_unregister_proto(&nokia_proto); > > +} > > diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h > > new file mode 100644 > > index 000000000000..8c4d307840e5 > > --- /dev/null > > +++ b/drivers/bluetooth/hci_nokia.h > > @@ -0,0 +1,140 @@ > > +/* > > + * Copyright (C) 2016 Sebastian Reichel <sre@kernel.org> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#ifndef __HCI_NOKIA_H > > +#define __HCI_NOKIA_H > > Lets not do a separate header here. Just move this all into > hci_nokia.c. There is really zero benefit in the header. ok. > > + > > +#define NOKIA_ID_CSR 0x02 > > +#define NOKIA_ID_BCM2048 0x04 > > +#define NOKIA_ID_TI1271 0x31 > > + > > +#define FIRMWARE_CSR "nokia/bc4fw.bin" > > If the CSR ones are not yet supported, then leave them out for > now. We can add this later. > > > +#define FIRMWARE_BCM2048 "nokia/bcmfw.bin" > > +#define FIRMWARE_TI1271 "nokia/ti1273.bin" > > + > > +#define NOKIA_BCM_BDADDR 0xfc01 > > We have btbcm.[ch] for this. 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. > > +#define HCI_NOKIA_NEG_PKT 0x06 > > +#define HCI_NOKIA_ALIVE_PKT 0x07 > > +#define HCI_NOKIA_RADIO_PKT 0x08 > > + > > +#define HCI_NOKIA_NEG_HDR_SIZE 1 > > +#define HCI_NOKIA_MAX_NEG_SIZE 255 > > +#define HCI_NOKIA_ALIVE_HDR_SIZE 1 > > +#define HCI_NOKIA_MAX_ALIVE_SIZE 255 > > +#define HCI_NOKIA_RADIO_HDR_SIZE 2 > > +#define HCI_NOKIA_MAX_RADIO_SIZE 255 > > + > > +#define NOKIA_PROTO_PKT 0x44 > > +#define NOKIA_PROTO_BYTE 0x4c > > + > > +#define NOKIA_NEG_REQ 0x00 > > +#define NOKIA_NEG_ACK 0x20 > > +#define NOKIA_NEG_NAK 0x40 > > + > > +#define H4_TYPE_SIZE 1 > > I am not sure this define adds any overall value to the code. > > > + > > +#define NOKIA_RECV_ACL \ > > + H4_RECV_ACL, \ > > + .wordaligned = true > > + > > +#define NOKIA_RECV_SCO \ > > + H4_RECV_SCO, \ > > + .wordaligned = true > > + > > +#define NOKIA_RECV_EVENT \ > > + H4_RECV_EVENT, \ > > + .wordaligned = true > > + > > +#define NOKIA_RECV_ALIVE \ > > + .type = HCI_NOKIA_ALIVE_PKT, \ > > + .hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \ > > + .loff = 0, \ > > + .lsize = 1, \ > > + .maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \ > > + .wordaligned = true > > + > > +#define NOKIA_RECV_NEG \ > > + .type = HCI_NOKIA_NEG_PKT, \ > > + .hlen = HCI_NOKIA_NEG_HDR_SIZE, \ > > + .loff = 0, \ > > + .lsize = 1, \ > > + .maxlen = HCI_NOKIA_MAX_NEG_SIZE, \ > > + .wordaligned = true > > + > > +#define NOKIA_RECV_RADIO \ > > + .type = HCI_NOKIA_RADIO_PKT, \ > > + .hlen = HCI_NOKIA_RADIO_HDR_SIZE, \ > > + .loff = 1, \ > > + .lsize = 1, \ > > + .maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \ > > + .wordaligned = true > > 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. > > + > > +struct hci_nokia_neg_hdr { > > + __u8 dlen; > > +} __packed; > > + > > +struct hci_nokia_neg_cmd { > > + __u8 ack; > > + __u16 baud; > > + __u16 unused1; > > + __u8 proto; > > + __u16 sys_clk; > > + __u16 unused2; > > +} __packed; > > + > > +static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb) > > +{ > > + return (struct hci_nokia_neg_hdr *) skb->data; > > +} > > What good is this inline? A define would be way better, if really > needed. I will drop hci_nokia_neg_hdr() and hci_nokia_alive_hdr() inlines > [...] -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-16 9:09 ` Sebastian Reichel @ 2016-08-16 10:23 ` Marcel Holtmann 2016-08-16 20:05 ` Pavel Machek 2016-08-16 10:23 ` Marcel Holtmann 1 sibling, 1 reply; 37+ messages in thread From: Marcel Holtmann @ 2016-08-16 10:23 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, Ivaylo Dimitrov, open list:BLUETOOTH DRIVERS, linux-serial, linux-omap, devicetree, linux-kernel Hi Sebastian, >>> + if (err < 0) { >>> + BT_ERR("%s: Failed to load Nokia firmware file (%d)", >>> + hu->hdev->name, err); >>> + return err; >>> + } >>> + >>> + fw_ptr = fw->data; >>> + fw_size = fw->size; >>> + >>> + while (fw_size >= 4) { >>> + u16 pkt_size = get_unaligned_le16(fw_ptr); >>> + u8 pkt_type = fw_ptr[2]; >>> + const struct hci_command_hdr *cmd; >>> + u16 opcode; >>> + struct sk_buff *skb; >>> + >>> + switch (pkt_type) { >>> + case HCI_COMMAND_PKT: >>> + cmd = (struct hci_command_hdr *)(fw_ptr + 3); >>> + opcode = le16_to_cpu(cmd->opcode); >>> + >>> + skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen, >>> + fw_ptr + 3 + HCI_COMMAND_HDR_SIZE, >>> + HCI_INIT_TIMEOUT); >>> + if (IS_ERR(skb)) { >>> + err = PTR_ERR(skb); >>> + BT_ERR("%s: Firmware command %04x failed (%d)", >>> + hu->hdev->name, opcode, err); >>> + goto done; >>> + } >>> + kfree_skb(skb); >>> + break; >>> + 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. > > 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. > >>> + 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. >>> + >>> +#define NOKIA_ID_CSR 0x02 >>> +#define NOKIA_ID_BCM2048 0x04 >>> +#define NOKIA_ID_TI1271 0x31 >>> + >>> +#define FIRMWARE_CSR "nokia/bc4fw.bin" >> >> If the CSR ones are not yet supported, then leave them out for >> now. We can add this later. >> >>> +#define FIRMWARE_BCM2048 "nokia/bcmfw.bin" >>> +#define FIRMWARE_TI1271 "nokia/ti1273.bin" >>> + >>> +#define NOKIA_BCM_BDADDR 0xfc01 >> >> We have btbcm.[ch] for this. > > 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. > >>> +#define HCI_NOKIA_NEG_PKT 0x06 >>> +#define HCI_NOKIA_ALIVE_PKT 0x07 >>> +#define HCI_NOKIA_RADIO_PKT 0x08 >>> + >>> +#define HCI_NOKIA_NEG_HDR_SIZE 1 >>> +#define HCI_NOKIA_MAX_NEG_SIZE 255 >>> +#define HCI_NOKIA_ALIVE_HDR_SIZE 1 >>> +#define HCI_NOKIA_MAX_ALIVE_SIZE 255 >>> +#define HCI_NOKIA_RADIO_HDR_SIZE 2 >>> +#define HCI_NOKIA_MAX_RADIO_SIZE 255 >>> + >>> +#define NOKIA_PROTO_PKT 0x44 >>> +#define NOKIA_PROTO_BYTE 0x4c >>> + >>> +#define NOKIA_NEG_REQ 0x00 >>> +#define NOKIA_NEG_ACK 0x20 >>> +#define NOKIA_NEG_NAK 0x40 >>> + >>> +#define H4_TYPE_SIZE 1 >> >> I am not sure this define adds any overall value to the code. >> >>> + >>> +#define NOKIA_RECV_ACL \ >>> + H4_RECV_ACL, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_SCO \ >>> + H4_RECV_SCO, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_EVENT \ >>> + H4_RECV_EVENT, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_ALIVE \ >>> + .type = HCI_NOKIA_ALIVE_PKT, \ >>> + .hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \ >>> + .loff = 0, \ >>> + .lsize = 1, \ >>> + .maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_NEG \ >>> + .type = HCI_NOKIA_NEG_PKT, \ >>> + .hlen = HCI_NOKIA_NEG_HDR_SIZE, \ >>> + .loff = 0, \ >>> + .lsize = 1, \ >>> + .maxlen = HCI_NOKIA_MAX_NEG_SIZE, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_RADIO \ >>> + .type = HCI_NOKIA_RADIO_PKT, \ >>> + .hlen = HCI_NOKIA_RADIO_HDR_SIZE, \ >>> + .loff = 1, \ >>> + .lsize = 1, \ >>> + .maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \ >>> + .wordaligned = true >> >> 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-16 10:23 ` Marcel Holtmann @ 2016-08-16 20:05 ` Pavel Machek 0 siblings, 0 replies; 37+ messages in thread From: Pavel Machek @ 2016-08-16 20:05 UTC (permalink / raw) To: Marcel Holtmann Cc: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, Ivaylo Dimitrov, open list:BLUETOOTH DRIVERS, linux-serial, linux-omap, devicetree, linux-kernel Hi! > > 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. > Editing firmware may raise some "interesting" legal questions... and its redistribution might be tricky, too. Microsoft is not likely to be too helpful :-(. Best, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-16 9:09 ` Sebastian Reichel 2016-08-16 10:23 ` Marcel Holtmann @ 2016-08-16 10:23 ` Marcel Holtmann 1 sibling, 0 replies; 37+ messages in thread From: Marcel Holtmann @ 2016-08-16 10:23 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, Ivaylo Dimitrov, open list:BLUETOOTH DRIVERS, linux-serial, linux-omap, devicetree, linux-kernel Hi Sebastian, >>> + if (err < 0) { >>> + BT_ERR("%s: Failed to load Nokia firmware file (%d)", >>> + hu->hdev->name, err); >>> + return err; >>> + } >>> + >>> + fw_ptr = fw->data; >>> + fw_size = fw->size; >>> + >>> + while (fw_size >= 4) { >>> + u16 pkt_size = get_unaligned_le16(fw_ptr); >>> + u8 pkt_type = fw_ptr[2]; >>> + const struct hci_command_hdr *cmd; >>> + u16 opcode; >>> + struct sk_buff *skb; >>> + >>> + switch (pkt_type) { >>> + case HCI_COMMAND_PKT: >>> + cmd = (struct hci_command_hdr *)(fw_ptr + 3); >>> + opcode = le16_to_cpu(cmd->opcode); >>> + >>> + skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen, >>> + fw_ptr + 3 + HCI_COMMAND_HDR_SIZE, >>> + HCI_INIT_TIMEOUT); >>> + if (IS_ERR(skb)) { >>> + err = PTR_ERR(skb); >>> + BT_ERR("%s: Firmware command %04x failed (%d)", >>> + hu->hdev->name, opcode, err); >>> + goto done; >>> + } >>> + kfree_skb(skb); >>> + break; >>> + 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. > > 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. > >>> + 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. >>> + >>> +#define NOKIA_ID_CSR 0x02 >>> +#define NOKIA_ID_BCM2048 0x04 >>> +#define NOKIA_ID_TI1271 0x31 >>> + >>> +#define FIRMWARE_CSR "nokia/bc4fw.bin" >> >> If the CSR ones are not yet supported, then leave them out for >> now. We can add this later. >> >>> +#define FIRMWARE_BCM2048 "nokia/bcmfw.bin" >>> +#define FIRMWARE_TI1271 "nokia/ti1273.bin" >>> + >>> +#define NOKIA_BCM_BDADDR 0xfc01 >> >> We have btbcm.[ch] for this. > > 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. > >>> +#define HCI_NOKIA_NEG_PKT 0x06 >>> +#define HCI_NOKIA_ALIVE_PKT 0x07 >>> +#define HCI_NOKIA_RADIO_PKT 0x08 >>> + >>> +#define HCI_NOKIA_NEG_HDR_SIZE 1 >>> +#define HCI_NOKIA_MAX_NEG_SIZE 255 >>> +#define HCI_NOKIA_ALIVE_HDR_SIZE 1 >>> +#define HCI_NOKIA_MAX_ALIVE_SIZE 255 >>> +#define HCI_NOKIA_RADIO_HDR_SIZE 2 >>> +#define HCI_NOKIA_MAX_RADIO_SIZE 255 >>> + >>> +#define NOKIA_PROTO_PKT 0x44 >>> +#define NOKIA_PROTO_BYTE 0x4c >>> + >>> +#define NOKIA_NEG_REQ 0x00 >>> +#define NOKIA_NEG_ACK 0x20 >>> +#define NOKIA_NEG_NAK 0x40 >>> + >>> +#define H4_TYPE_SIZE 1 >> >> I am not sure this define adds any overall value to the code. >> >>> + >>> +#define NOKIA_RECV_ACL \ >>> + H4_RECV_ACL, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_SCO \ >>> + H4_RECV_SCO, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_EVENT \ >>> + H4_RECV_EVENT, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_ALIVE \ >>> + .type = HCI_NOKIA_ALIVE_PKT, \ >>> + .hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \ >>> + .loff = 0, \ >>> + .lsize = 1, \ >>> + .maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_NEG \ >>> + .type = HCI_NOKIA_NEG_PKT, \ >>> + .hlen = HCI_NOKIA_NEG_HDR_SIZE, \ >>> + .loff = 0, \ >>> + .lsize = 1, \ >>> + .maxlen = HCI_NOKIA_MAX_NEG_SIZE, \ >>> + .wordaligned = true >>> + >>> +#define NOKIA_RECV_RADIO \ >>> + .type = HCI_NOKIA_RADIO_PKT, \ >>> + .hlen = HCI_NOKIA_RADIO_HDR_SIZE, \ >>> + .loff = 1, \ >>> + .lsize = 1, \ >>> + .maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \ >>> + .wordaligned = true >> >> 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-13 3:14 ` [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel 2016-08-14 23:54 ` Paul Gortmaker 2016-08-16 7:02 ` Marcel Holtmann @ 2016-08-16 8:10 ` Marcel Holtmann 2016-08-16 9:35 ` Sebastian Reichel 2 siblings, 1 reply; 37+ messages in thread From: Marcel Holtmann @ 2016-08-16 8:10 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, Ivaylo Dimitrov, open list:BLUETOOTH DRIVERS, linux-serial, linux-omap, devicetree, linux-kernel Hi Sebastien, > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h > index a7d67aec3632..314b243df996 100644 > --- a/drivers/bluetooth/hci_uart.h > +++ b/drivers/bluetooth/hci_uart.h > @@ -35,7 +35,7 @@ > #define HCIUARTGETFLAGS _IOR('U', 204, int) > > /* UART protocols */ > -#define HCI_UART_MAX_PROTO 10 > +#define HCI_UART_MAX_PROTO 11 > > #define HCI_UART_H4 0 > #define HCI_UART_BCSP 1 > @@ -47,6 +47,7 @@ > #define HCI_UART_BCM 7 > #define HCI_UART_QCA 8 > #define HCI_UART_AG6XX 9 > +#define HCI_UART_NOKIA 10 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver 2016-08-16 8:10 ` Marcel Holtmann @ 2016-08-16 9:35 ` Sebastian Reichel 0 siblings, 0 replies; 37+ messages in thread From: Sebastian Reichel @ 2016-08-16 9:35 UTC (permalink / raw) To: Marcel Holtmann Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, Ivaylo Dimitrov, open list:BLUETOOTH DRIVERS, linux-serial, linux-omap, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1352 bytes --] Hi Marcel, On Tue, Aug 16, 2016 at 10:10:07AM +0200, Marcel Holtmann wrote: > > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h > > index a7d67aec3632..314b243df996 100644 > > --- a/drivers/bluetooth/hci_uart.h > > +++ b/drivers/bluetooth/hci_uart.h > > @@ -35,7 +35,7 @@ > > #define HCIUARTGETFLAGS _IOR('U', 204, int) > > > > /* UART protocols */ > > -#define HCI_UART_MAX_PROTO 10 > > +#define HCI_UART_MAX_PROTO 11 > > > > #define HCI_UART_H4 0 > > #define HCI_UART_BCSP 1 > > @@ -47,6 +47,7 @@ > > #define HCI_UART_BCM 7 > > #define HCI_UART_QCA 8 > > #define HCI_UART_AG6XX 9 > > +#define HCI_UART_NOKIA 10 > > 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 6/7] ARM: dts: OMAP3-N900: Add bluetooth 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel ` (4 preceding siblings ...) 2016-08-13 3:14 ` [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel @ 2016-08-13 3:14 ` Sebastian Reichel 2016-08-14 8:53 ` Pavel Machek 2016-08-13 3:14 ` [RFC 7/7] ARM: dts: OMAP3-N950: " Sebastian Reichel 2016-08-16 7:10 ` [RFC 0/7] Nokia N9xx bluetooth driver Marcel Holtmann 7 siblings, 1 reply; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 3:14 UTC (permalink / raw) To: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby Cc: Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel 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(-) diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index 1d7b1d0d471a..1dba4a64d8fd 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -155,6 +155,13 @@ compatible = "nokia,n900-ir"; pwms = <&pwm9 0 26316 0>; /* 38000 Hz */ }; + + /* controlled (enabled/disabled) directly by bcm2048 and wl1251 */ + vctcxo: vctcxo { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <38400000>; + }; }; &omap3_pmx_core { @@ -162,8 +169,10 @@ uart2_pins: pinmux_uart2_pins { pinctrl-single,pins = < - OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0) /* uart2_rx */ + OMAP3_CORE1_IOPAD(0x2174, PIN_INPUT | MUX_MODE0) /* uart2_cts */ + OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT | MUX_MODE0) /* uart2_rts */ OMAP3_CORE1_IOPAD(0x2178, PIN_OUTPUT | MUX_MODE0) /* uart2_tx */ + OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0) /* uart2_rx */ >; }; @@ -917,6 +926,8 @@ interrupt-parent = <&gpio2>; interrupts = <10 IRQ_TYPE_NONE>; /* gpio line 42 */ + + clocks = <&vctcxo>; }; }; @@ -937,6 +948,15 @@ interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>; pinctrl-names = "default"; pinctrl-0 = <&uart2_pins>; + + bcm2048: bluetooth { + compatible = "nokia,brcm,bcm2048"; + reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* 91 */ + host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */ + bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */ + clocks = <&vctcxo>; + clock-names = "sysclk"; + }; }; &uart3 { -- 2.8.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 6/7] ARM: dts: OMAP3-N900: Add bluetooth 2016-08-13 3:14 ` [RFC 6/7] ARM: dts: OMAP3-N900: Add bluetooth Sebastian Reichel @ 2016-08-14 8:53 ` Pavel Machek 0 siblings, 0 replies; 37+ messages in thread From: Pavel Machek @ 2016-08-14 8:53 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel 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> As the driver is not currently working at N900, I agree that this patch is useful, but it should probably not go upstream. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 7/7] ARM: dts: OMAP3-N950: Add bluetooth 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel ` (5 preceding siblings ...) 2016-08-13 3:14 ` [RFC 6/7] ARM: dts: OMAP3-N900: Add bluetooth Sebastian Reichel @ 2016-08-13 3:14 ` Sebastian Reichel 2016-08-14 8:53 ` Pavel Machek 2016-08-16 7:10 ` [RFC 0/7] Nokia N9xx bluetooth driver Marcel Holtmann 7 siblings, 1 reply; 37+ messages in thread From: Sebastian Reichel @ 2016-08-13 3:14 UTC (permalink / raw) To: Sebastian Reichel, Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby Cc: Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel --- arch/arm/boot/dts/omap3-n950-n9.dtsi | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi index 927b17fc4ed8..7c0c53e2f679 100644 --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi @@ -58,6 +58,13 @@ pinctrl-0 = <&debug_leds>; }; }; + + /* controlled (enabled/disabled) directly by wl1271 */ + vctcxo: vctcxo { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <38400000>; + }; }; &omap3_pmx_core { @@ -125,6 +132,16 @@ OMAP3_CORE1_IOPAD(0x210a, PIN_OUTPUT | MUX_MODE4) /* gpio_93 (cmt_apeslpx) */ >; }; + + uart2_pins: pinmux_uart2_pins { + pinctrl-single,pins = < + OMAP3_CORE1_IOPAD(0x2174, PIN_INPUT | MUX_MODE0) /* uart2_cts */ + OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT | MUX_MODE0) /* uart2_rts */ + OMAP3_CORE1_IOPAD(0x2178, PIN_OUTPUT | MUX_MODE0) /* uart2_tx */ + OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0) /* uart2_rx */ + >; + }; + }; &omap3_pmx_core2 { @@ -435,3 +452,20 @@ &ssi_port2 { status = "disabled"; }; + +&uart2 { + pinctrl-names = "default"; + pinctrl-0 = <&uart2_pins>; + + 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"; + }; + +}; -- 2.8.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 7/7] ARM: dts: OMAP3-N950: Add bluetooth 2016-08-13 3:14 ` [RFC 7/7] ARM: dts: OMAP3-N950: " Sebastian Reichel @ 2016-08-14 8:53 ` Pavel Machek 0 siblings, 0 replies; 37+ messages in thread From: Pavel Machek @ 2016-08-14 8:53 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Marcel Holtmann, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel You'll need to sign off the patch. Acked-by: Pavel Machek <pavel@ucw.cz> Pavel On Sat 2016-08-13 05:14:38, Sebastian Reichel wrote: > --- > arch/arm/boot/dts/omap3-n950-n9.dtsi | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi > index 927b17fc4ed8..7c0c53e2f679 100644 > --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi > +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi > @@ -58,6 +58,13 @@ > pinctrl-0 = <&debug_leds>; > }; > }; > + > + /* controlled (enabled/disabled) directly by wl1271 */ > + vctcxo: vctcxo { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <38400000>; > + }; > }; > > &omap3_pmx_core { > @@ -125,6 +132,16 @@ > OMAP3_CORE1_IOPAD(0x210a, PIN_OUTPUT | MUX_MODE4) /* gpio_93 (cmt_apeslpx) */ > >; > }; > + > + uart2_pins: pinmux_uart2_pins { > + pinctrl-single,pins = < > + OMAP3_CORE1_IOPAD(0x2174, PIN_INPUT | MUX_MODE0) /* uart2_cts */ > + OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT | MUX_MODE0) /* uart2_rts */ > + OMAP3_CORE1_IOPAD(0x2178, PIN_OUTPUT | MUX_MODE0) /* uart2_tx */ > + OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0) /* uart2_rx */ > + >; > + }; > + > }; > > &omap3_pmx_core2 { > @@ -435,3 +452,20 @@ > &ssi_port2 { > status = "disabled"; > }; > + > +&uart2 { > + pinctrl-names = "default"; > + pinctrl-0 = <&uart2_pins>; > + > + 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"; > + }; > + > +}; -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/7] Nokia N9xx bluetooth driver 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel ` (6 preceding siblings ...) 2016-08-13 3:14 ` [RFC 7/7] ARM: dts: OMAP3-N950: " Sebastian Reichel @ 2016-08-16 7:10 ` Marcel Holtmann 2016-08-16 20:22 ` Rob Herring 7 siblings, 1 reply; 37+ messages in thread From: Marcel Holtmann @ 2016-08-16 7:10 UTC (permalink / raw) To: Sebastian Reichel Cc: Tony Lindgren, Rob Herring, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, ivo.g.dimitrov.75, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel Hi 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/7] Nokia N9xx bluetooth driver 2016-08-16 7:10 ` [RFC 0/7] Nokia N9xx bluetooth driver Marcel Holtmann @ 2016-08-16 20:22 ` Rob Herring 0 siblings, 0 replies; 37+ messages in thread From: Rob Herring @ 2016-08-16 20:22 UTC (permalink / raw) To: Marcel Holtmann Cc: Sebastian Reichel, Tony Lindgren, Mark Rutland, Greg Kroah-Hartman, Jiri Slaby, Ville Tervo, Filip Matijević, Aaro Koskinen, Pavel Machek, Pali Rohár, Ivaylo Dimitrov, linux-bluetooth, linux-serial, linux-omap, devicetree, linux-kernel On Tue, Aug 16, 2016 at 2:10 AM, Marcel Holtmann <marcel@holtmann.org> wrote: > Hi 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. 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2016-08-17 15:55 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-13 3:14 [RFC 0/7] Nokia N9xx bluetooth driver Sebastian Reichel 2016-08-13 3:14 ` [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init Sebastian Reichel 2016-08-14 8:49 ` Pavel Machek 2016-08-16 8:14 ` Sebastian Reichel 2016-08-13 3:14 ` [RFC 2/7] tty: add support for "tty slave" devices Sebastian Reichel 2016-08-13 10:03 ` Greg Kroah-Hartman 2016-08-13 20:31 ` Sebastian Reichel 2016-08-14 11:36 ` Greg Kroah-Hartman 2016-08-14 8:48 ` Pavel Machek 2016-08-14 11:35 ` Greg Kroah-Hartman 2016-08-13 3:14 ` [RFC 3/7] dt: bindings: Add nokia-bluetooth Sebastian Reichel 2016-08-16 13:51 ` Rob Herring 2016-08-16 23:28 ` Sebastian Reichel 2016-08-17 13:11 ` Rob Herring 2016-08-17 15:54 ` Pavel Machek 2016-08-13 3:14 ` [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment Sebastian Reichel 2016-08-14 8:51 ` Pavel Machek 2016-08-16 7:05 ` Marcel Holtmann 2016-08-16 7:51 ` Sebastian Reichel 2016-08-13 3:14 ` [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver Sebastian Reichel 2016-08-14 23:54 ` Paul Gortmaker 2016-08-15 1:12 ` Sebastian Reichel 2016-08-16 7:02 ` Marcel Holtmann 2016-08-16 7:52 ` Pali Rohár 2016-08-16 9:25 ` Sebastian Reichel 2016-08-16 9:09 ` Sebastian Reichel 2016-08-16 10:23 ` Marcel Holtmann 2016-08-16 20:05 ` Pavel Machek 2016-08-16 10:23 ` Marcel Holtmann 2016-08-16 8:10 ` Marcel Holtmann 2016-08-16 9:35 ` Sebastian Reichel 2016-08-13 3:14 ` [RFC 6/7] ARM: dts: OMAP3-N900: Add bluetooth Sebastian Reichel 2016-08-14 8:53 ` Pavel Machek 2016-08-13 3:14 ` [RFC 7/7] ARM: dts: OMAP3-N950: " Sebastian Reichel 2016-08-14 8:53 ` Pavel Machek 2016-08-16 7:10 ` [RFC 0/7] Nokia N9xx bluetooth driver Marcel Holtmann 2016-08-16 20:22 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).