__hci_cmd_sync() not suitable for nokia h4p

DISCUSSION Pavel Machek 3 participants 13 messages Dec 9, 2014 – Dec 13, 2014

Subsystems (3)

Device tree bindings Staging Bluetooth drivers

lore

Share & tools

lore

Pavel Machek

author

2014-12-09 19:02 UTC #0 lore raw

Hi!

Major problem with Nokia H4P driver was, that it uses custom functions instead of __hci_cmd_sync().

To recap, code was something like this: (You can see ifdefs with old code and my new replacements).

....

	len = sizeof(*neg_cmd) + sizeof(*neg_hdr) + H4_TYPE_SIZE;
#undef OLD 
#ifdef OLD
	skb = bt_skb_alloc(len, GFP_KERNEL);
	if (!skb)
		return -ENOMEM;

	memset(skb->data, 0x00, len);
	*skb_put(skb, 1) = H4_NEG_PKT;
	neg_hdr = (struct hci_h4p_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
	neg_cmd = (struct hci_h4p_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
	neg_hdr->dlen = sizeof(*neg_cmd);
#else      
	struct {
		struct hci_h4p_neg_cmd neg_cmd;
	} data;

	memset(&data, 0, len-1);
	neg_cmd = &data.neg_cmd;

#endif

	neg_cmd->ack = H4P_NEG_REQ;
	neg_cmd->baud = cpu_to_le16(BT_BAUDRATE_DIVIDER/MAX_BAUD_RATE);
	neg_cmd->proto = H4P_PROTO_BYTE;
	neg_cmd->sys_clk = cpu_to_le16(sysclk);

	hci_h4p_change_speed(info, INIT_SPEED);

	printk("Setting up packet\n");
	hci_h4p_set_rts(info, 1);
	info->init_error = 0;
	init_completion(&info->init_completion);
	printk("skb_queue_tail\n");

#ifdef OLD
	skb_queue_tail(&info->txq, 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);
#else
	printk("hci_cmd_sync\n");

// set_bit(HCI_RUNNING, &info->hdev->flags);

	info->initing = 2;
	
	skb = __hci_cmd_sync(info->hdev, H4_NEG_PKT << 8, sizeof(*neg_cmd), ((void *) &data), 20);

...but there's problem with __hci_cmd_sync(): the packet it produces is one byte too long; for initialization, I can work around that with << 8, so the added zero is@the begining (and device will be still initialized), but that is a) an extreme hack, b) only works for first packet and c) means that __hci_cmd_sync() will not recognize the reply, meaning it does not really help.

Are there different variants of the protocol I should switch to? Any ideas? Should I forget about __hci_cmd_sync()?

Best regards,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marcel Holtmann

maintainer

2014-12-09 19:07 UTC #1 lore raw

Hi Pavel,

Quoted 2 lines

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

Pavel Machek

author

2014-12-09 20:13 UTC #2 lore raw

Hi!

Quoted 6 lines

Quoted 2 lines
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

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marcel Holtmann

maintainer

2014-12-09 21:19 UTC #3 lore raw

Hi Pavel,

Quoted 17 lines

Quoted 6 lines
Quoted 2 lines
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);
# > +}
#

Quoted 8 lines

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

Quoted 6 lines

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

Pavel Machek

author

2014-12-10 13:15 UTC #4 lore raw

Hi!

Quoted 16 lines

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.

Quoted 5 lines

Quoted 2 lines
...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.

Quoted 9 lines

Quoted 6 lines
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.

Quoted 3 lines

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.

drivers/staging/nokia_h4p/nokia_core.c +0 -6

index 9ece2c8..5cdb86a 100644
@@ -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);

drivers/staging/nokia_h4p/nokia_fw-bcm.c +3 ~~-70~~

87 lines not on this page · raw message

drivers/staging/nokia_h4p/nokia_fw.c +24 ~~-40~~

97 lines not on this page · raw message

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marcel Holtmann

maintainer

2014-12-11 12:58 UTC #5 lore raw

Hi Pavel,

Quoted 16 lines

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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
...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.

Quoted 14 lines

Quoted 9 lines
Quoted 6 lines
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.
Quoted 1 line

Quoted 5 lines

Quoted 2 lines
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

Pavel Machek

author

2014-12-11 22:13 UTC #6 lore raw

Hi!

Quoted 6 lines

Quoted 2 lines
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).

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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

arch/arm/boot/dts/omap3-n900.dts +12 -0

index 9e0e5a2..201f21b 100644
@@ -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 {

drivers/staging/nokia_h4p/nokia_core.c +111 ~~-30~~

193 lines not on this page · raw message

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marcel Holtmann

maintainer

2014-12-11 22:25 UTC #7 lore raw

Hi Pavel,

Quoted 9 lines

Quoted 6 lines
Quoted 2 lines
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.

Quoted 11 lines

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.

Quoted 7 lines

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

Pavel Machek

author

2014-12-12 09:51 UTC #8 lore raw

Hi!

Quoted 14 lines

Quoted 14 lines
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 4 lines
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?

Quoted 3 lines

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.

Quoted 6 lines

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

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marcel Holtmann

maintainer

2014-12-12 12:28 UTC #9 lore raw

Hi Pavel,

Quoted 19 lines

Quoted 19 lines
Quoted 18 lines
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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?

Quoted 5 lines

Quoted 2 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 10 lines

Quoted 6 lines
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

Sebastian Reichel

2014-12-12 01:15 UTC #10 lore raw

In reply to Pavel Machek

Hi,

Quoted 11 lines · Pavel Machek

Hi!
Quoted 6 lines
Quoted 2 lines
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?

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 13 lines

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
arch/arm/boot/dts/omap3-n900.dts +0 -0
index 9e0e5a2..201f21b 100644

arch/arm/boot/dts/omap3-n900.dts line 779

Quoted 4 lines

arch/arm/boot/dts/omap3-n900.dts +1 -0

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

arch/arm/boot/dts/omap3-n900.dts line 785

Quoted 6 lines

arch/arm/boot/dts/omap3-n900.dts +3 -0

 	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.

arch/arm/boot/dts/omap3-n900.dts line 787

Quoted 2 lines

arch/arm/boot/dts/omap3-n900.dts +2 -0

+		  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 ;)

arch/arm/boot/dts/omap3-n900.dts line 788

Quoted 1 line

arch/arm/boot/dts/omap3-n900.dts +1 -0
+		  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.

arch/arm/boot/dts/omap3-n900.dts line 789

Quoted 1 line

arch/arm/boot/dts/omap3-n900.dts +1 -0
+		  chip-type = <3>;

This should be set in the driver based on the compatible value and not via DT data.

arch/arm/boot/dts/omap3-n900.dts line 792

Quoted 2 lines

arch/arm/boot/dts/omap3-n900.dts +2 -0
+		  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.

arch/arm/boot/dts/omap3-n900.dts line 790

Quoted 1 line

arch/arm/boot/dts/omap3-n900.dts +1 -0
+		  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 = <38400000>;
};

Then the bluetooth device can reference its clock device:

clocks = <&vctcxo_clock>;

The same clock reference should be added to the wl1251 DT node :)

Quoted 1 line

... [code] ...

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141212/e0496be9/attachment.sig>

Pavel Machek

author

2014-12-12 12:14 UTC #11 lore raw

Hi!

Quoted 7 lines

Quoted 2 lines
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.
Quoted 1 line

Quoted 7 lines

Quoted 2 lines
&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.

Quoted 8 lines

arch/arm/boot/dts/omap3-n900.dts line 785
Quoted 6 lines
arch/arm/boot/dts/omap3-n900.dts +3 -0
 	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.

Quoted 5 lines

arch/arm/boot/dts/omap3-n900.dts line 787
Quoted 2 lines
arch/arm/boot/dts/omap3-n900.dts +2 -0
+		  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.

Quoted 4 lines

arch/arm/boot/dts/omap3-n900.dts line 789
Quoted 1 line
arch/arm/boot/dts/omap3-n900.dts +1 -0
+		  chip-type = <3>;
This should be set in the driver based on the compatible value and not via DT data.

Quoted 5 lines

arch/arm/boot/dts/omap3-n900.dts line 792
Quoted 2 lines
arch/arm/boot/dts/omap3-n900.dts +2 -0
+		  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?

Quoted 10 lines

arch/arm/boot/dts/omap3-n900.dts line 790
Quoted 1 line
arch/arm/boot/dts/omap3-n900.dts +1 -0
+		  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 = <38400000>;
};

No. It seems that this selects baud rate during the chip init. I guess I can just remove that one.

Quoted 5 lines

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

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Sebastian Reichel

2014-12-13 17:35 UTC #12 lore raw

Hi,

Quoted 10 lines · Pavel Machek

Quoted 10 lines
Quoted 2 lines
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.
Quoted 3 lines
&uart2 {
+ compatible = "brcm,uart,bcm2048";

Quoted 7 lines

Quoted 4 lines
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).

Quoted 11 lines

Quoted 8 lines
arch/arm/boot/dts/omap3-n900.dts line 785
Quoted 6 lines
arch/arm/boot/dts/omap3-n900.dts +3 -0
 	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.

Quoted 7 lines

Quoted 5 lines
arch/arm/boot/dts/omap3-n900.dts line 787
Quoted 2 lines
arch/arm/boot/dts/omap3-n900.dts +2 -0
+		  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.

Quoted 7 lines

Quoted 4 lines
arch/arm/boot/dts/omap3-n900.dts line 789
Quoted 1 line
arch/arm/boot/dts/omap3-n900.dts +1 -0
+		  chip-type = <3>;
This should be set in the driver based on the compatible value and not via DT data.
Ok

Quoted 7 lines

Quoted 5 lines
arch/arm/boot/dts/omap3-n900.dts line 792
Quoted 2 lines
arch/arm/boot/dts/omap3-n900.dts +2 -0
+		  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.

Quoted 5 lines

Quoted 5 lines
arch/arm/boot/dts/omap3-n900.dts line 790
Quoted 1 line
arch/arm/boot/dts/omap3-n900.dts +1 -0
+		  bt-sysclk = <2>;
I think this should be mapped cleanly in DT by adding a new clock to the DTS file:

Quoted 8 lines

Quoted 5 lines
vctcxo_clock: clock  {
    compatible = "fixed-clock";
    #clock-cells = <0>;
    clock-frequency = <38400000>;
};
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 :)

Quoted 7 lines

Quoted 5 lines
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() ]