Re: __hci_cmd_sync() not suitable for nokia h4p
Hi,
On Fri, Dec 12, 2014 at 01:14:53PM +0100, Pavel Machek wrote:
I have created provisional device tree binding, and the driver still
works.
I don't have time to look at the code now, but I have some comments
regarding the binding.
uart2 {
+compatible = brcm,uart,bcm2048;
This does not look correct. The uart should not be overwritten. The
current h4p driver indeed implements a driver for the serial port,
but that's a) linux specific and does not belong in the DT and b)
should probably be changed in the mainline kernel.
Yes, bettter solution is needed here. But see the code, I don't see
how b) would be implemented.
I think it's probably a good idea to start from scratch. The h4p
driver in its current state does not fit to the current kernel's
style at all.
My suggestion would be to have a driver, which implements a hci_dev.
The driver would mostly look like the standard uart hci_dev driver,
but additionally handles the wakeup and reset gpios. Power
Management for the uart device is done via runtime pm, which is
supported by OMAP's uart driver.
Then there should be a second driver, which implements the h4
protocol extensions from Nokia as hci_uart_proto. It could be put
into something like drivers/bluetooth/hci_h4p.c and have support for
the additional packet types (namely H4_EVT_PKT, H4_NEG_PKT,
H4_ALIVE_PKT and H4_RADIO_PKT).
interrupts-extended = intc 73 omap3_pmx_core OMAP3_UART2_RX;
pinctrl-names = default;
pinctrl-0 = uart2_pins;
+ device {
+ compatible = brcm,bcm2048;
+ uart = uart2;
You don't need a phandle to the parent device.
Ok.
+ reset-gpios = gpio3 27 GPIO_ACTIVE_HIGH; /* want 91 */
+ host-wakeup-gpios = gpio4 5 GPIO_ACTIVE_HIGH; /* want 101
*/
The host-wakeup should be mapped as irq, gpio2irq conversion
will happen ;)
Why? It is accessed as gpio, too.
Ok. I only did a quick look and saw, that the gpio was converted to
irq.
+ chip-type = 3;
This should be set in the driver based on the compatible
value and not via DT data.
Ok
+ clocks = uart2_fck, uart2_ick;
+ clock-names = fck, ick;
These clocks you defined belong to the uart device and not to the
uart slave (bluetooth) device.
Ok. Why are they only needed in the bluetooth case?
A quick guess (without a deeper look at the code) is, that the ttys
use hwmod provided clock information (which is available from the
kernel). btw, h4p not using hwmod would be another problem, if it is
ok to reimplement a full serial driver.
I guess the clock data should be added to the uart devices in DT,
though. This is btw. true for almost all omap internal devices
described in DT, since the clock data has only been available in DT
for 1 or 2 kernel releases.
+ bt-sysclk = 2;
I think this should be mapped cleanly in DT by adding a new clock
to the DTS file:
vctcxo_clock: clock {
compatible = fixed-clock;
#clock-cells = 0;
clock-frequency = 3840;
};
No. It seems that this selects baud rate during the chip init. I guess
I can just remove that one.
you can see, that its a descriptor for the speed of the bcm2048's
system clock.
(from h4p driver)
bt-sysclk = 1 = sysclk = 12000;
bt-sysclk = 2 = sysclk = 38400;
and incidentally there is a 38.4 MHz clock connected to the bcm2048
and the wl1251 in the N900 :)
Then the bluetooth device can reference its clock device:
clocks = vctcxo_clock;
The same clock reference should be added to the wl1251 DT node :)
Feel free to do that, but I don't see how this one helps...?
The clock is not enabled via the CPU, instead wl1251 and bcm2048
enable it themselfes, so it's not strictly needed. But it means,
that
a) DT represents the hardware correctly
b) one can acquire the sysclk speed from the referenced clock
[ using devm_clk_get() and clk_get_rate() ]
-- Sebastian
signature.asc
Description: Digital signature
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi! What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. Speed changes at the end of firmware load, but I guess that's detail? Anyway, patch would look like this. You should really look into providing hdev-setup() callback. That is normally the callback where you want to load the firmware. I can provide setup() callback and load firmware from there. I have created provisional device tree binding, and the driver still works. Some time ago you mentioned that with the big issues fixed, you'd be willing to take it into the tree. What way forward do you see? Would it make sense to re-enable the driver in staging, so that big changes could be applied, followed by renames? If the driver is clean, there is no point in taking it through staging. It can be cleaned up and then merged all together. Ok, so you can revert a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd in your tree, and I can post patches against that? I think the important part is to focus on the N900 derivative with the Broadcom chip inside. And just ignore all the rest. So start small and do not try to carry the N770, N800, N810 hacks over. Well, I did remove non-relevant firmware loaders, and I can remove the switches for different firmware loaders, too, but spending time removing support for different hardware does not sound quite right. However you might want to check Neil Brown's patches for TTY-slave devices he just posted. Maybe the N900 devices should be exposed as TTY and we just attach N_HCI line discipline to it. If all the GPIO wiggling can be done automatically at TTY open, then that should be the way to go. I also send an email about using the new unconfigured stage to get this done cleanly. I seen them, but they don't help, I'm afraid. The GPIOs are used for power saving, and they are used in various places including the serial irq handler. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi!
I have created provisional device tree binding, and the driver still
works.
I don't have time to look at the code now, but I have some comments
regarding the binding.
uart2 {
+compatible = brcm,uart,bcm2048;
This does not look correct. The uart should not be overwritten. The
current h4p driver indeed implements a driver for the serial port,
but that's a) linux specific and does not belong in the DT and b)
should probably be changed in the mainline kernel.
Yes, bettter solution is needed here. But see the code, I don't see
how b) would be implemented.
interrupts-extended = intc 73 omap3_pmx_core OMAP3_UART2_RX;
pinctrl-names = default;
pinctrl-0 = uart2_pins;
+ device {
+ compatible = brcm,bcm2048;
+ uart = uart2;
You don't need a phandle to the parent device.
Ok.
+ reset-gpios = gpio3 27 GPIO_ACTIVE_HIGH; /* want 91 */
+ host-wakeup-gpios = gpio4 5 GPIO_ACTIVE_HIGH; /* want 101
*/
The host-wakeup should be mapped as irq, gpio2irq conversion
will happen ;)
Why? It is accessed as gpio, too.
+ chip-type = 3;
This should be set in the driver based on the compatible
value and not via DT data.
Ok
+ clocks = uart2_fck, uart2_ick;
+ clock-names = fck, ick;
These clocks you defined belong to the uart device and not to the
uart slave (bluetooth) device.
Ok. Why are they only needed in the bluetooth case?
+ bt-sysclk = 2;
I think this should be mapped cleanly in DT by adding a new clock
to the DTS file:
vctcxo_clock: clock {
compatible = fixed-clock;
#clock-cells = 0;
clock-frequency = 3840;
};
No. It seems that this selects baud rate during the chip init. I guess
I can just remove that one.
Then the bluetooth device can reference its clock device:
clocks = vctcxo_clock;
The same clock reference should be added to the wl1251 DT node :)
Feel free to do that, but I don't see how this one helps...?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi Pavel, What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. Speed changes at the end of firmware load, but I guess that's detail? Anyway, patch would look like this. You should really look into providing hdev-setup() callback. That is normally the callback where you want to load the firmware. I can provide setup() callback and load firmware from there. I have created provisional device tree binding, and the driver still works. Some time ago you mentioned that with the big issues fixed, you'd be willing to take it into the tree. What way forward do you see? Would it make sense to re-enable the driver in staging, so that big changes could be applied, followed by renames? If the driver is clean, there is no point in taking it through staging. It can be cleaned up and then merged all together. Ok, so you can revert a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd in your tree, and I can post patches against that? just treat this a submission of a new driver. I think the important part is to focus on the N900 derivative with the Broadcom chip inside. And just ignore all the rest. So start small and do not try to carry the N770, N800, N810 hacks over. Well, I did remove non-relevant firmware loaders, and I can remove the switches for different firmware loaders, too, but spending time removing support for different hardware does not sound quite right. Lets face it, you are not getting it upstream that way. If nobody has the hardware to test it or cares about the hardware anymore, then this should not be upstream in the first place. Make your life easier and get your hardware supported. Then someone can evolve this for older Nokia devices. But I am not taking code that nobody has tested. Keep it simple is really the only way to get this driver merged. There is so much old cruft in there that you are better off only caring about the N900 version of the hardware. However you might want to check Neil Brown's patches for TTY-slave devices he just posted. Maybe the N900 devices should be exposed as TTY and we just attach N_HCI line discipline to it. If all the GPIO wiggling can be done automatically at TTY open, then that should be the way to go. I also send an email about using the new unconfigured stage to get this done cleanly. I seen them, but they don't help, I'm afraid. The GPIOs are used for power saving, and they are used in various places including the serial irq handler. That is something that might need some extra work on it, but it seems that the general direction of TTY slaves is the right one. Anyway, the main order of business is really that it supports DT and that it can be compiled on every single platform and you could even load the driver on x86 without doing any harm. Regards Marcel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi Pavel, The TODO file says: # + # + skb_queue_tail(info-txq, fw_skb); # + spin_lock_irqsave(info-lock, flags); # + hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) | # + UART_IER_THRI); # + spin_unlock_irqrestore(info-lock, flags); # +} # # and as I explained before, this crazy can not continue. Bluetooth drivers can provide a # +hdev-setup callback for loading firmware and doing other setup details. You can just # +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the # +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the # +case with the Nokia firmware files. ...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is not suitable after all? __hci_cmd_sync is to be used in hdev-setup where you load the firmware. However when hdev-setup is run, we expect that the HCI transport is fully up and running and that the driver takes care of the transport. That is done via hdev-send and hci_recv_frame. h4p changes uart speed again after load of the firmware, but I guess that's ok. if you can do it the other way around it would result in a faster init. Depending on how many patches are actually required to be loaded. But I don't understand what you want me to do at this point. I guess skb_queue_tail+hci_h4p_outb should be moved to a helper function (that's easy), and I already moved initialization to hci_setup(). nokia_core.c uses test_bit(HCI_RUNNING, info-hdev-flags) to tell between initialization and data traffic, but I guess that's fine? I have no idea on how much more I can explain this. There should be code in the driver that handles the HCI transport. That means init of the transport and sending and receiving HCI frames. And then there is the piece to load the firmware etc. These are two independent things. Ok, it looks like __hci_cmd_sync() is indeed good match for the firmware load. What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. Speed changes at the end of firmware load, but I guess that's detail? Anyway, patch would look like this. You should really look into providing hdev-setup() callback. That is normally the callback where you want to load the firmware. Regards Marcel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi!
h4p changes uart speed again after load of the firmware, but I guess
that's ok.
if you can do it the other way around it would result in a faster
init. Depending on how many patches are actually required to be
loaded.
IIRC driver does two speed changes, so it looks to me like someone
already tried that (and it did not work).
What needs to be done is the bring up of the device including the proper
UART settings and speed and then just run the firmware downloads. All
firmware files on the Nokia devices where just HCI commands with vendor
specific details. Some from CSR, some from Broadcom and some from TI. You
can actually decode them if you really want to. Shouldn't be that hard.
Speed changes at the end of firmware load, but I guess that's detail?
Anyway, patch would look like this.
You should really look into providing hdev-setup() callback. That is
normally the callback where you want to load the firmware.
I can provide setup() callback and load firmware from there.
I have created provisional device tree binding, and the driver still
works.
Some time ago you mentioned that with the big issues fixed, you'd be
willing to take it into the tree. What way forward do you see? Would
it make sense to re-enable the driver in staging, so that big
changes could be applied, followed by renames?
Thanks,
Pavel
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index 9e0e5a2..201f21b 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -790,9 +776,21 @@
};
uart2 {
+compatible = brcm,uart,bcm2048;
interrupts-extended = intc 73 omap3_pmx_core OMAP3_UART2_RX;
pinctrl-names = default;
pinctrl-0 = uart2_pins;
+ device {
+ compatible = brcm,bcm2048;
+ uart = uart2;
+ reset-gpios = gpio3 27 GPIO_ACTIVE_HIGH; /* want 91 */
+ host-wakeup-gpios = gpio4 5 GPIO_ACTIVE_HIGH; /* want 101
*/
+ bluetooth-wakeup-gpios = gpio2 5 GPIO_ACTIVE_HIGH; /* want
37 */
+ chip-type = 3;
+ bt-sysclk = 2;
+ clocks = uart2_fck, uart2_ick;
+ clock-names = fck, ick;
+ };
};
uart3 {
diff --git a/drivers/staging/nokia_h4p/nokia_core.c
b/drivers/staging/nokia_h4p/nokia_core.c
index 5cdb86a..ac61cfd 100644
--- a/drivers/staging/nokia_h4p/nokia_core.c
+++ b/drivers/staging/nokia_h4p/nokia_core.c
@@ -37,6 +37,8 @@
#include linux/clk.h
#include linux/interrupt.h
#include linux/gpio.h
+#include linux/of_gpio.h
+#include linux/of_irq.h
#include linux/timer.h
#include linux/kthread.h
#include linux/io.h
@@ -1112,12 +1114,75 @@ free:
return -ENODEV;
}
+static int hci_h4p_probe_pdata(struct platform_device *pdev, struct
hci_h4p_info *info,
+ struct hci_h4p_platform_data *bt_plat_data)
+{
+ info-chip_type = bt_plat_data-chip_type;
+ info-bt_wakeup_gpio = bt_plat_data-bt_wakeup_gpio;
+ info-host_wakeup_gpio = bt_plat_data-host_wakeup_gpio;
+ info-reset_gpio = bt_plat_data-reset_gpio;
+ info-reset_gpio_shared = bt_plat_data-reset_gpio_shared;
+ info-bt_sysclk = bt_plat_data-bt_sysclk;
+
+ info-irq = bt_plat_data-uart_irq;
+ info-uart_base = devm_ioremap(pdev-dev, bt_plat_data-uart_base,
+ SZ_2K);
+ info-uart_iclk = devm_clk_get(pdev-dev, bt_plat_data-uart_iclk);
+ info-uart_fclk = devm_clk_get(pdev-dev, bt_plat_data-uart_fclk);
+ return 0;
+}
+
+static int hci_h4p_probe_dt(struct platform_device *pdev, struct hci_h4p_info
*info)
+{
+ struct device_node *node;
+ struct device_node *uart = pdev-dev.of_node;
+ u32 val;
+ struct resource *mem;
+
+ node = of_get_child_by_name(uart, device);
+
+ if (!node)
+ return -ENODATA;
+
+ if (of_property_read_u32(node, chip-type, val)) return -EINVAL;
+ info-chip_type = val;
+
+ if (of_property_read_u32(node, bt-sysclk, val)) return -EINVAL;
+ info-bt_sysclk = val;
+
+ info-reset_gpio = of_get_named_gpio(node, reset-gpios, 0);
+ info-host_wakeup_gpio = of_get_named_gpio(node, host-wakeup-gpios,
0);
+ info-bt_wakeup_gpio = of_get_named_gpio(node,
bluetooth-wakeup-gpios, 0);
+ //uart = of_parse_phandle(node, uart, 0);
+ if (!uart) {
+ dev_err(pdev-dev, UART link not provided\n);
+ return -EINVAL;
+ }
+
+ info-irq = irq_of_parse_and_map(uart, 0);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ info-uart_base = devm_ioremap_resource(pdev-dev, mem);
+
+ info-uart_iclk = of_clk_get_by_name(node, ick);
+ info-uart_fclk = of_clk_get_by_name(node, fck);
+
+ printk(DT: have neccessary data\n);
+ return
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi Pavel, h4p changes uart speed again after load of the firmware, but I guess that's ok. if you can do it the other way around it would result in a faster init. Depending on how many patches are actually required to be loaded. IIRC driver does two speed changes, so it looks to me like someone already tried that (and it did not work). normally it does not matter which way around. I think some people changed it in hciattach for Broadcom devices actually. What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. Speed changes at the end of firmware load, but I guess that's detail? Anyway, patch would look like this. You should really look into providing hdev-setup() callback. That is normally the callback where you want to load the firmware. I can provide setup() callback and load firmware from there. I have created provisional device tree binding, and the driver still works. Some time ago you mentioned that with the big issues fixed, you'd be willing to take it into the tree. What way forward do you see? Would it make sense to re-enable the driver in staging, so that big changes could be applied, followed by renames? If the driver is clean, there is no point in taking it through staging. It can be cleaned up and then merged all together. I think the important part is to focus on the N900 derivative with the Broadcom chip inside. And just ignore all the rest. So start small and do not try to carry the N770, N800, N810 hacks over. However you might want to check Neil Brown's patches for TTY-slave devices he just posted. Maybe the N900 devices should be exposed as TTY and we just attach N_HCI line discipline to it. If all the GPIO wiggling can be done automatically at TTY open, then that should be the way to go. I also send an email about using the new unconfigured stage to get this done cleanly. http://permalink.gmane.org/gmane.linux.bluez.kernel/50483 Regards Marcel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi,
On Thu, Dec 11, 2014 at 11:13:07PM +0100, Pavel Machek wrote:
Hi!
h4p changes uart speed again after load of the firmware, but I guess
that's ok.
if you can do it the other way around it would result in a faster
init. Depending on how many patches are actually required to be
loaded.
IIRC driver does two speed changes, so it looks to me like someone
already tried that (and it did not work).
maybe. maybe not. Should be easy to test it (again) and add a
comment, shouldn't it?
What needs to be done is the bring up of the device including the proper
UART settings and speed and then just run the firmware downloads. All
firmware files on the Nokia devices where just HCI commands with vendor
specific details. Some from CSR, some from Broadcom and some from TI.
You can actually decode them if you really want to. Shouldn't be that
hard.
Speed changes at the end of firmware load, but I guess that's detail?
Anyway, patch would look like this.
You should really look into providing hdev-setup() callback. That is
normally the callback where you want to load the firmware.
I can provide setup() callback and load firmware from there.
I have created provisional device tree binding, and the driver still
works.
I don't have time to look at the code now, but I have some comments
regarding the binding.
Some time ago you mentioned that with the big issues fixed, you'd be
willing to take it into the tree. What way forward do you see? Would
it make sense to re-enable the driver in staging, so that big
changes could be applied, followed by renames?
Thanks,
Pavel
diff --git a/arch/arm/boot/dts/omap3-n900.dts
b/arch/arm/boot/dts/omap3-n900.dts
index 9e0e5a2..201f21b 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -790,9 +776,21 @@
};
uart2 {
+compatible = brcm,uart,bcm2048;
This does not look correct. The uart should not be overwritten. The
current h4p driver indeed implements a driver for the serial port,
but that's a) linux specific and does not belong in the DT and b)
should probably be changed in the mainline kernel.
interrupts-extended = intc 73 omap3_pmx_core OMAP3_UART2_RX;
pinctrl-names = default;
pinctrl-0 = uart2_pins;
+ device {
+ compatible = brcm,bcm2048;
+ uart = uart2;
You don't need a phandle to the parent device.
+ reset-gpios = gpio3 27 GPIO_ACTIVE_HIGH; /* want 91 */
+ host-wakeup-gpios = gpio4 5 GPIO_ACTIVE_HIGH; /* want 101
*/
The host-wakeup should be mapped as irq, gpio2irq conversion
will happen ;)
+ bluetooth-wakeup-gpios = gpio2 5 GPIO_ACTIVE_HIGH; /* want
37 */
To be consistent with the n900 DTS file you should probably drop
want from the comments.
+ chip-type = 3;
This should be set in the driver based on the compatible
value and not via DT data.
+ clocks = uart2_fck, uart2_ick;
+ clock-names = fck, ick;
These clocks you defined belong to the uart device and not to the
uart slave (bluetooth) device.
+ bt-sysclk = 2;
I think this should be mapped cleanly in DT by adding a new clock
to the DTS file:
vctcxo_clock: clock {
compatible = fixed-clock;
#clock-cells = 0;
clock-frequency = 3840;
};
Then the bluetooth device can reference its clock device:
clocks = vctcxo_clock;
The same clock reference should be added to the wl1251 DT node :)
... [code] ...
-- Sebastian
signature.asc
Description: Digital signature
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi!
The TODO file says:
# +
# + skb_queue_tail(info-txq, fw_skb);
# + spin_lock_irqsave(info-lock, flags);
# + hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
# + UART_IER_THRI);
# + spin_unlock_irqrestore(info-lock, flags);
# +}
#
# and as I explained before, this crazy can not continue. Bluetooth drivers
can provide a
# +hdev-setup callback for loading firmware and doing other setup details.
You can just
# +bring up the HCI transport. We are providing __hci_cmd_sync for easy
loading of the
# +firmware. Especially if the firmware just consists of HCI commands.
Which is clearly the
# +case with the Nokia firmware files.
...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is
not suitable after all?
__hci_cmd_sync is to be used in hdev-setup where you load the firmware.
However when hdev-setup is run, we expect that the HCI transport is fully up
and running and that the driver takes care of the transport. That is done via
hdev-send and hci_recv_frame.
h4p changes uart speed again after load of the firmware, but I guess
that's ok.
But I don't understand what you want me to do at this point. I guess
skb_queue_tail+hci_h4p_outb should be moved to a helper function
(that's easy), and I already moved initialization to hci_setup().
nokia_core.c uses test_bit(HCI_RUNNING, info-hdev-flags) to tell
between initialization and data traffic, but I guess that's fine?
I have no idea on how much more I can explain this. There should be code in
the driver that handles the HCI transport. That means init of the transport
and sending and receiving HCI frames. And then there is the piece to load the
firmware etc. These are two independent things.
Ok, it looks like __hci_cmd_sync() is indeed good match for the
firmware load.
What needs to be done is the bring up of the device including the proper UART
settings and speed and then just run the firmware downloads. All firmware
files on the Nokia devices where just HCI commands with vendor specific
details. Some from CSR, some from Broadcom and some from TI. You can actually
decode them if you really want to. Shouldn't be that hard.
Speed changes at the end of firmware load, but I guess that's detail?
Anyway, patch would look like this.
diff --git a/drivers/staging/nokia_h4p/nokia_core.c
b/drivers/staging/nokia_h4p/nokia_core.c
index 9ece2c8..5cdb86a 100644
--- a/drivers/staging/nokia_h4p/nokia_core.c
+++ b/drivers/staging/nokia_h4p/nokia_core.c
@@ -475,12 +475,6 @@ static inline void hci_h4p_recv_frame(struct hci_h4p_info
*info,
info-rx_state = WAIT_FOR_PKT_TYPE;
return;
}
-
- if (!test_bit(HCI_UP, info-hdev-flags)) {
- BT_DBG(fw_event);
- hci_h4p_parse_fw_event(info, skb);
- return;
- }
}
hci_recv_frame(info-hdev, skb);
diff --git a/drivers/staging/nokia_h4p/nokia_fw-bcm.c
b/drivers/staging/nokia_h4p/nokia_fw-bcm.c
index 8066b21..89718d4 100644
--- a/drivers/staging/nokia_h4p/nokia_fw-bcm.c
+++ b/drivers/staging/nokia_h4p/nokia_fw-bcm.c
@@ -45,84 +45,17 @@ static int hci_h4p_bcm_set_bdaddr(struct hci_h4p_info *info,
return 0;
}
-void hci_h4p_bcm_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb)
-{
- struct sk_buff *fw_skb;
- int err;
- unsigned long flags;
-
- if (skb-data[5] != 0x00) {
- dev_err(info-dev, Firmware sending command failed 0x%.2x\n,
- skb-data[5]);
- info-fw_error = -EPROTO;
- }
-
- kfree_skb(skb);
-
- fw_skb = skb_dequeue(info-fw_q);
- if (fw_skb == NULL || info-fw_error) {
- complete(info-fw_completion);
- return;
- }
-
- if (fw_skb-data[1] == 0x01 fw_skb-data[2] == 0xfc
- fw_skb-len = 10) {
- BT_DBG(Setting bluetooth address);
- err = hci_h4p_bcm_set_bdaddr(info, fw_skb);
- if (err 0) {
- kfree_skb(fw_skb);
- info-fw_error = err;
- complete(info-fw_completion);
- return;
- }
- }
-
- hci_h4p_simple_send_frame(info, fw_skb);
-}
-
-
int hci_h4p_bcm_send_fw(struct hci_h4p_info *info,
struct sk_buff_head *fw_queue)
{
- struct sk_buff *skb;
- unsigned long flags, time;
+ unsigned long time;
info-fw_error = 0;
- BT_DBG(Sending firmware);
+ printk(Sending firmware (not really)\n);
time = jiffies;
-
- info-fw_q = fw_queue;
- skb = skb_dequeue(fw_queue);
- if (!skb)
- return -ENODATA;
-
- BT_DBG(Sending commands);
-
- /*
-* Disable smart-idle as UART TX interrupts
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi Pavel, Major problem with Nokia H4P driver was, that it uses custom functions instead of __hci_cmd_sync(). the __hci_cmd_sync is for sending HCI commands and not low-level protocol transports like H:4 or similar. So you want to separate the actual transport of HCI from the firmware loading. Regards Marcel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi! Major problem with Nokia H4P driver was, that it uses custom functions instead of __hci_cmd_sync(). the __hci_cmd_sync is for sending HCI commands and not low-level protocol transports like H:4 or similar. So you want to separate the actual transport of HCI from the firmware loading. The TODO file says: # + # + skb_queue_tail(info-txq, fw_skb); # + spin_lock_irqsave(info-lock, flags); # + hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) | # + UART_IER_THRI); # + spin_unlock_irqrestore(info-lock, flags); # +} # # and as I explained before, this crazy can not continue. Bluetooth drivers can provide a # +hdev-setup callback for loading firmware and doing other setup details. You can just # +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the # +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the # +case with the Nokia firmware files. ...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is not suitable after all? But I don't understand what you want me to do at this point. I guess skb_queue_tail+hci_h4p_outb should be moved to a helper function (that's easy), and I already moved initialization to hci_setup(). nokia_core.c uses test_bit(HCI_RUNNING, info-hdev-flags) to tell between initialization and data traffic, but I guess that's fine? Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: __hci_cmd_sync() not suitable for nokia h4p
Hi Pavel, Major problem with Nokia H4P driver was, that it uses custom functions instead of __hci_cmd_sync(). the __hci_cmd_sync is for sending HCI commands and not low-level protocol transports like H:4 or similar. So you want to separate the actual transport of HCI from the firmware loading. The TODO file says: # + # + skb_queue_tail(info-txq, fw_skb); # + spin_lock_irqsave(info-lock, flags); # + hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) | # + UART_IER_THRI); # + spin_unlock_irqrestore(info-lock, flags); # +} # # and as I explained before, this crazy can not continue. Bluetooth drivers can provide a # +hdev-setup callback for loading firmware and doing other setup details. You can just # +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the # +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the # +case with the Nokia firmware files. ...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is not suitable after all? __hci_cmd_sync is to be used in hdev-setup where you load the firmware. However when hdev-setup is run, we expect that the HCI transport is fully up and running and that the driver takes care of the transport. That is done via hdev-send and hci_recv_frame. But I don't understand what you want me to do at this point. I guess skb_queue_tail+hci_h4p_outb should be moved to a helper function (that's easy), and I already moved initialization to hci_setup(). nokia_core.c uses test_bit(HCI_RUNNING, info-hdev-flags) to tell between initialization and data traffic, but I guess that's fine? I have no idea on how much more I can explain this. There should be code in the driver that handles the HCI transport. That means init of the transport and sending and receiving HCI frames. And then there is the piece to load the firmware etc. These are two independent things. Look at how btusb.c is doing this. We only run __hci_cmd_sync there. The details on how commands and events are going over USB are not handling hdev-setup. That is done by the basics of the driver. What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. Regards Marcel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
